EOS S3 Hal i2c and wishbone bus #4

Willmish · 2023-06-29T09:29:11Z

This PR adds HAL definitions for I2C driver which uses the WIshbone bus onboard the EOS S3 SoC, used in the QuickFeather board (https://docs.zephyrproject.org/latest/boards/arm/quick_feather/doc/index.html) and the QuickThing+ board (https://github.com/sparkfun/QuickLogic_Thing_Plus).

it is based on the EOS S3 HAL for FreeRTOS: https://github.com/antmicro/eos-s3-hal

There is a small fix I need to introduce (currently hardcoded Clock dividor value), which needs to be read and calculated from the proper register at runtime.

Opening this PR as I am currently at EOSS and want to get initial feedback, also will be opening a PR to main zephyr repo that will require this module to be updated before it can be merged in (I2C Zephyr driver for EOS S3 SoC)

fkokosinski · 2023-06-30T07:22:22Z

Hi, thanks for your contribution!

There are several things that I'd like to see change in the commit history. IMO it should roughly follow this pattern:

First commit - add new files
Subsequent commits with fixups (one commit for each fix)

I'd also like to see changes to commit messages, i.e. it should have information about:

Where these files were taken from (origin name and url)
What version of the HAL/SDK they were taken from
License of these files
Who maintains these files (are they maintained externally, i.e. by the vendor?)

Here's a template:

Origin: 
URL:
Version:
Purpose:
License:
Maintained-by:

Commits with fixes do not need to follow this pattern (because they are your own contribution), but commit message is still needed.

One additional thing that I'd also want is a PR to Zephyr main repository that uses these changes. Without that, there's no real of testing your contribution in a automated way.

Willmish · 2023-07-02T14:10:48Z

Thanks for the initial review @fkokosinski !

I will try to follow your suggested flow and add all of that info. Will ping again when this is ready. For now - opened this zephyrproject-rtos/zephyr#59905 as a draft, as requested so you have a visibility of where these changes would be used.

This commit introduces HAL for EOS S3 from Quicklogic for the I2C driver. Origin: QuickLogic-Corp/qorc-sdk URL: https://github.com/QuickLogic-Corp/qorc-sdk Version: v1.10.0 Purpose: Starting point for the I2C HAL for EOS S3 License: Apache 2.0 Maintained-by: QuickLogic Corporation I2C HAL: Add working I2C HAL for zephyr. Signed-off-by: Szymon Duchniewicz <szduchniewicz@gmail.com>

Willmish · 2023-07-17T18:14:56Z

Hi @fkokosinski , Updated as requested, ready for further review. Had issues with Line endings when adding the unedited file, as the files in this repo seem to have CRLFs rather than LFs as EOL. (In short - didn't realise git allows you to have different EOL in index vs in working directory)

is it okay for me to amend all file endings to LF in this repo to avoid future issues?

fkokosinski · 2023-07-20T08:58:36Z

is it okay for me to amend all file endings to LF in this repo to avoid future issues?

That IMO sounds more like a configuration issue (text editor, environment) on your end, not a problem with how files in this repo are formatted ;)

As for individual commits:

7f2916b - this one looks ok
7212769 - previous commit adds I²C sources. What does this commit do? It looks like it has the clock-related fix you mentioned in the initial message, but it also contains some weird commend rearrangements and deletes some lines. Please justify/describe better in the commit message what's happening here.
015be1b - is this something that we could possibly move to the Zephyr I²C driver?
d75acc6 - described as CMakeLists.txt update, but contains WB changes. Why?

* Remove FreeRTOS dependency imports * Remove commented code * Remove FFE clock enabling (Now done in EOS S3 SoC setup). Signed-off-by: Szymon Duchniewicz <szduchniewicz@gmail.com>

Signed-off-by: Szymon Duchniewicz <szduchniewicz@gmail.com>

* Update CMakeLists.txt. * Update Copyright. * Fix imports. * Clean up comments. Signed-off-by: Szymon Duchniewicz <szduchniewicz@gmail.com>

Willmish · 2023-07-20T11:28:06Z

is it okay for me to amend all file endings to LF in this repo to avoid future issues?

That IMO sounds more like a configuration issue (text editor, environment) on your end, not a problem with how files in this repo are formatted ;)

Yeah was a config issue, simply that in the git index for this repo some files are stored with file endings of CRLF, some of LF, whereas in the main zephyr repo only some binary files or svgs are stored as CRLF in the index - no code files are stored as CRLF. Not a very big issue, but sometimes can prove to be a nuisance. Im ok to leave it as is as well, just wanted to point it out.

As for individual commits:

1. [7f2916b](https://github.com/zephyrproject-rtos/hal_quicklogic/commit/7f2916b388e26ee4bace4779da0ce5c2695aafca) - this one looks ok

💯

2. [7212769](https://github.com/zephyrproject-rtos/hal_quicklogic/commit/721276998a9d63f5de4e5e8bc358d9e0e1f6ff60) - previous commit adds I²C sources. What does this commit do? It looks like it has the clock-related fix you mentioned in the initial message, but it also contains some weird commend rearrangements and deletes some lines. Please justify/describe better in the commit message what's happening here.

Now e348ffd - Cleaned it up (no longer changes the licensing comments at the top, that was an artifact since I originally copied the HAL files over from this repo: https://github.com/antmicro/eos-s3-hal . The commit message now should better reflect the contents of the commit. The FFE clock enabling which was previously handled in WB_init that this commit removes is now handled in the SoCs soc.c file, as it had some FreeRTOS function dependancies and soc.c already has other clock domain related operations and setup, so thought it would be fine to move it there: https://github.com/zephyrproject-rtos/zephyr/pull/59905/files#diff-59656af559176dc4f6527fc38573de806d1b43b0aeec0531bf5bd7c6a1c14305R62-R87

3. [015be1b](https://github.com/zephyrproject-rtos/hal_quicklogic/commit/015be1b27a5012fc4338c3c85cc12d4de1d90751) - is this something that we could possibly move to the Zephyr I²C driver?

I am not sure how this could be moved to the Zephyr driver exactly as it introduces checks for FFE CSR register whether it is busy or not interleaved in between other function calls ( https://www.quicklogic.com/wp-content/uploads/2020/06/QL-S3-Technical-Reference-Manual-Vol-1.pdf#page=254 ). The only way I could think of for now was to completely rewrite the entire HAL and move all of this upstream - there is no dependancy on any binary blobs after all so this is possible, simply would take some time. For now porting the HAL seemed like the easiest and most efficient way to add I2C support.

4. [d75acc6](https://github.com/zephyrproject-rtos/hal_quicklogic/commit/d75acc6f3dadd720c3f64bb938420736b51b18f2) - described as CMakeLists.txt update, but contains WB changes. Why?

Updated the commit message to better reflect the changes - it is a CMakeLists.txt update plus some smaller refactors such as comment removals, import fixes and copyright attribution (all now described in the commit message)

@fkokosinski Let me know if there are any further changes required

fkokosinski

Hi, sorry for the late response!

fkokosinski · 2023-07-27T11:42:09Z

HAL/src/CMakeLists.txt

+	zephyr_sources(eoss3_hal_i2c.c)
+	zephyr_sources(eoss3_hal_wb.c)


These two lines should be conditionally compiled when CONFIG_I2C_EOS_S3 is defined, not when CONFIG_GPIO_EOS_S3 is.

fkokosinski · 2023-07-27T11:46:08Z

HAL/src/eoss3_hal_wb.c

@@ -148,8 +125,6 @@ HAL_StatusTypeDef HAL_WB_Init(UINT8_t ucSlaveSel)

                HAL_PAD_Config(&padcfg);

-        //dbg_str_hex32("MOSI - pad6 =", IO_MUX->PAD_6_CTRL);
-


Please reduce the changes in the original file to a minimum, even if it means leaving commented out lines.

Sure, what is the specific reason for keeping the commented out code? Is it to keep the HAL as aligned to original as possible, or something else?

Is it to keep the HAL as aligned to original as possible

Exactly

fkokosinski · 2023-07-27T11:46:14Z

HAL/src/eoss3_hal_wb.c

@@ -161,7 +136,6 @@ HAL_StatusTypeDef HAL_WB_Init(UINT8_t ucSlaveSel)
                padcfg.ucSmtTrg = PAD_SMT_TRIG_DIS;

                HAL_PAD_Config(&padcfg);
-        //dbg_str_hex32("MISO - pad8", IO_MUX->PAD_8_CTRL);


fkokosinski · 2023-07-27T11:46:19Z

HAL/src/eoss3_hal_wb.c

@@ -174,7 +148,6 @@ HAL_StatusTypeDef HAL_WB_Init(UINT8_t ucSlaveSel)
                padcfg.ucSmtTrg = PAD_SMT_TRIG_DIS;

                HAL_PAD_Config(&padcfg);
-        //dbg_str_hex32("CLK - pad10", IO_MUX->PAD_10_CTRL);


fkokosinski · 2023-07-27T11:46:26Z

HAL/src/eoss3_hal_wb.c

@@ -188,7 +161,6 @@ HAL_StatusTypeDef HAL_WB_Init(UINT8_t ucSlaveSel)

                HAL_PAD_Config(&padcfg);

-        //dbg_str_hex32("CS - pad9", IO_MUX->PAD_9_CTRL);


fkokosinski · 2023-07-27T11:49:56Z

HAL/src/eoss3_hal_i2c.c

-#include "Fw_global_config.h"
-
 #include <stdio.h>
 #include <stdint.h>
 #include <string.h>
 #include <stddef.h>
 #include <eoss3_dev.h>
 #include <eoss3_hal_def.h>
-#include <s3x_clock_hal.h>
-//#include <eoss3_hal_rcc.h>
 #include <eoss3_hal_i2c.h>
 #include <eoss3_hal_pad_config.h>
 #include "test_types.h"
 #include <eoss3_hal_wb.h>
-//#include "eoss3_hal_power.h"
-#include "Fw_global_config.h"


Please remove FreeRTOS-dependent includes only.

To clarify - I should remove all FreeRTOS includes, but keep the commented out hal includes?

HAL/src/eoss3_hal_wb.c

fkokosinski · 2023-07-27T11:57:56Z

HAL/src/eoss3_hal_i2c.c

+     * default it is not) 
+     * #define OSC_GET_FREQ_INC()	(((AIP->OSC_CTRL_1 & 0xFFF) + 3) * 32768)
+    */
+    uiClock = (((AIP->OSC_CTRL_1 & 0xFFF) + 3) * 32768)/4/6; // TODO: Find a programatic way to get the dividor (6) from CLK_CTRL_C_0, supposedly done here: https://github.com/QuickLogic-Corp/qorc-sdk/blob/d61d064146c0ee927aa12b088b3bbbce60615f4d/Libraries/Power/src/s3x_clock.c#L685


Not a fan of this change directly in the HAL tbh. It looks like HAL_I2C_SetClockFreq is only called by HAL_I2C_Init. Can we move this configuration to the Zephyr I2C driver after it calls the HAL_I2C_Init function?

Sure, do you suggest then removing the entire HAL_I2C_SetClockFreq function, and rewriting it in the Zephyr I2C driver then? I'm happy to move it, if that's the case.

FYI, after adding this clock frequency configuration to the Zephyr I2C driver, its not possible to keep the HAL_I2C_SetClockFreq functionbefore my changes as it relies on many FreeRTOS elements, so I would remove it in its entirety from this file.

Yup, sounds good! Thanks for the continued effort

fkokosinski · 2023-07-27T12:02:35Z

HAL/src/eoss3_hal_i2c.c

@@ -230,6 +237,7 @@ HAL_StatusTypeDef HAL_I2C_Write(UINT8_t ucDevAddress, UINT8_t ucAddress, UINT8_t
      	eI2CState = I2C_READY;
 	return HAL_ERROR;
    }
+    waitFFEReady(MAX_CYCLES_FFE);


So how does the original code from qorc-sdk work, if they are not checking the FFE CSR value?

When running it on the Quick Thing Plus board, it did not work with qorc-sdk samples either - I2C transmissions were failing. However, I will try and reflash the board with the original qorc-sdk code without my changes to confirm this and will share my findings. If indeed it is broken, I might also suggest this change to qorc-sdk.

without doing these checks however, the transmission failed every time on Zehyr as well. My suspicion are:

this is broken on qorc-sdk as well, and if it is working then there is some delay introduced by other threads, which is not present on Zephyr (hence for stability, this wait is required)

this is working on qorc-sdk and I misconfigured the I2C for qorc-sdk when testing it for QuickThing Plus with qorc-sdk. In this case, maybe the clock speed configured here https://github.com/zephyrproject-rtos/hal_quicklogic/pull/4/files/7f2916b388e26ee4bace4779da0ce5c2695aafca..e348ffd18778fc1aa212f830bd2b97b61a6cbcc4#diff-87abd87ab59fba2c37f505e3cb82a2aefdefdcd07126f4391195a971889dc29eR137 is incorrect, and if I reconfigure it properly, the issue will go away?

This particular issue is slightly beyond my knowledge of the underlying hardware, but I will try my best to give you a better answer, and test results

fkokosinski · 2023-07-27T12:04:38Z

HAL/src/eoss3_hal_i2c.c

-#include <eoss3_dev.h>
-#include <eoss3_hal_def.h>
-#include <eoss3_hal_i2c.h>
-#include <eoss3_hal_pad_config.h>
+#include "eoss3_dev.h"
+#include "eoss3_hal_def.h"
+#include "eoss3_hal_i2c.h"
+#include "eoss3_hal_pad_config.h"
 #include "test_types.h"
-#include <eoss3_hal_wb.h>
+#include "eoss3_hal_wb.h"


Please don't refactor the original code - we want to keep it as similar to the original code as possible.

MicroTransactionsMatterToo · 2023-09-30T08:26:53Z

Any updates on this? Am working on a project that needs the I2C interface, and I'd prefer not to have to switch to FreeRTOS at this stage.

Willmish · 2023-10-03T12:57:44Z

Hi @MicroTransactionsMatterToo - I didn't have time to continue this over summer but picking this back up along with @JDuchniewicz. at the moment my fork and the driver in : zephyrproject-rtos/zephyr#59905 work, but are not up to the standard of the repos yet. (And I just saw I need to rebase the PR to the main zephyr repo as well)

JDuchniewicz · 2023-10-03T13:44:39Z

We will rebase later this week and try to close the PRs by the end of the month. @MicroTransactionsMatterToo

MicroTransactionsMatterToo · 2023-10-06T05:20:30Z

That'd be great, in the meantime I pulled it into my local copy of the zephyr source, and it works just fine. If I find the time, I'll try getting the clocks configurable on Zephyr as well. I need that functionality to be able to provide hardware clocking to FPGA IPs.

fkokosinski self-requested a review June 30, 2023 07:15

Willmish mentioned this pull request Jul 2, 2023

drivers: i2c: i2c_eos_s3: Add I2C driver support for EOS S3 zephyrproject-rtos/zephyr#59905

Closed

12 tasks

Willmish force-pushed the hal_i2c branch 2 times, most recently from f8a2c93 to d75acc6 Compare July 17, 2023 18:12

Willmish added 3 commits July 20, 2023 12:44

Add hardcoded I2C clock calculation

e348ffd

* Remove FreeRTOS dependency imports * Remove commented code * Remove FFE clock enabling (Now done in EOS S3 SoC setup). Signed-off-by: Szymon Duchniewicz <szduchniewicz@gmail.com>

I2C HAL: Add FFE CSR check after TXRX command.

d49d738

Signed-off-by: Szymon Duchniewicz <szduchniewicz@gmail.com>

Update CMakeLists.txt + Refactor

dc1fb16

* Update CMakeLists.txt. * Update Copyright. * Fix imports. * Clean up comments. Signed-off-by: Szymon Duchniewicz <szduchniewicz@gmail.com>

Willmish force-pushed the hal_i2c branch from d75acc6 to dc1fb16 Compare July 20, 2023 11:06

fkokosinski requested changes Jul 27, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EOS S3 Hal i2c and wishbone bus #4

EOS S3 Hal i2c and wishbone bus #4

Willmish commented Jun 29, 2023

fkokosinski commented Jun 30, 2023

Willmish commented Jul 2, 2023

Willmish commented Jul 17, 2023

fkokosinski commented Jul 20, 2023

Willmish commented Jul 20, 2023 •

edited

fkokosinski left a comment

fkokosinski Jul 27, 2023

fkokosinski Jul 27, 2023

Willmish Jul 27, 2023

fkokosinski Jul 28, 2023

fkokosinski Jul 27, 2023

fkokosinski Jul 27, 2023

fkokosinski Jul 27, 2023

fkokosinski Jul 27, 2023

Willmish Jul 27, 2023

fkokosinski Jul 28, 2023

fkokosinski Jul 27, 2023

Willmish Jul 27, 2023

fkokosinski Jul 28, 2023

fkokosinski Jul 27, 2023

Willmish Jul 27, 2023

fkokosinski Jul 27, 2023

MicroTransactionsMatterToo commented Sep 30, 2023

Willmish commented Oct 3, 2023

JDuchniewicz commented Oct 3, 2023

MicroTransactionsMatterToo commented Oct 6, 2023

		zephyr_sources(eoss3_hal_i2c.c)
		zephyr_sources(eoss3_hal_wb.c)

		@@ -148,8 +125,6 @@ HAL_StatusTypeDef HAL_WB_Init(UINT8_t ucSlaveSel)

		HAL_PAD_Config(&padcfg);

		//dbg_str_hex32("MOSI - pad6 =", IO_MUX->PAD_6_CTRL);

		@@ -188,7 +161,6 @@ HAL_StatusTypeDef HAL_WB_Init(UINT8_t ucSlaveSel)

		HAL_PAD_Config(&padcfg);

		//dbg_str_hex32("CS - pad9", IO_MUX->PAD_9_CTRL);

EOS S3 Hal i2c and wishbone bus #4

Are you sure you want to change the base?

EOS S3 Hal i2c and wishbone bus #4

Conversation

Willmish commented Jun 29, 2023

fkokosinski commented Jun 30, 2023

Willmish commented Jul 2, 2023

Willmish commented Jul 17, 2023

fkokosinski commented Jul 20, 2023

Willmish commented Jul 20, 2023 • edited

fkokosinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MicroTransactionsMatterToo commented Sep 30, 2023

Willmish commented Oct 3, 2023

JDuchniewicz commented Oct 3, 2023

MicroTransactionsMatterToo commented Oct 6, 2023

Willmish commented Jul 20, 2023 •

edited