Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: i2c: i2c_eos_s3: Add I2C driver support for EOS S3 #59905

Closed
wants to merge 7 commits into from

Conversation

Willmish
Copy link

@Willmish Willmish commented Jul 2, 2023

Module dependent PR

Warning
This PR depends on https://github.com/zephyrproject-rtos/hal_quicklogic being updated
DO NOT merge this PR until zephyrproject-rtos/hal_quicklogic#4 is merged

Description

This PR adds basic i2c driver implementation for the EOS S3 SoC, used with the Sparkfun QuickLogic Thing Plus board (which is very similar to the intree QuickLogic QuickFeather ) based on the HAL module ported from QuickLogic's QORC SDK.

Since there is an API mismatch between the actual HAL and the zephyr i2c driver API, I left a comment explaining my current approach in drivers/i2c/i2c_eos_s3.c file (https://github.com/zephyrproject-rtos/zephyr/compare/main...Willmish:zephyr:eos_s3_fixes?expand=1#diff-586321414cf511e0b1f1b99e041b736fe987a5d03a43eb9073e47c7ca2943f26R152). I would strongly appreciate guidance whether this is a correct approach or not.

Additionally this PR adds a tiny change to set up pinmuxing for the builtin button pad by default (this is normally muxed by the default bootloader provided by QuickLogic, but IMO the board port should be bootloader independent, so planning to add future PRs for elements of the bootloader that are taken for granted/add support for MCUBoot)

Comprehensive list of changes:

  • Update Kconfig.defconfig to include a BUILD_OUTPUT_BIN boolean flag - not sure if this is absolutely necessary, but current flasher for all EOS S3 based boards - TinyFPGAProgrammer requires a binary file for flashing
  • Add Button pad pinmuxing to board.c file for quickfeather board
  • add necessary defines to board.h and soc_pinmap.h files for the abovementioned pinmuxing for the quickfeather board
  • Add all required files for i2c driver, including:
    • Updated CMakeLists.txt
    • Updated Kconfig
    • Added Kconfig.eos_s3
    • Added i2c_eos_s3.c
    • Added quicklogic,eos-s3-i2c.yaml dts binding
  • Additionally : Updated soc/arm/quicklogic_eos_s3/soc.c to enable Flexible Fusion Engine Clocks, setup its dividors and properly gate/route them to be used by I2C peripherals (based on EOS S3 Technical Reference Manual, Chapter 36.1 Setup FFE clocks, page 340
  • Updated SoC .dtsi file to quicklogic_eos_s3.dtsi to define the i2c0 device (there are two, should I also add a skeleton definition for i2c1?)
  • Updated QuickFeather board dts to enable i2c0 by default

Additional info

Opening as draft for now to be visible in zephyrproject-rtos/hal_quicklogic#4 , but requires commit cleanup and code cleanup to conform to Zephyr's Contribution guidelines

Created in collaboration with @JDuchniewicz in preparation for Embedded Open Source Summit 2023

@henrikbrixandersen
Copy link
Member

Warning
This PR depends on https://github.com/zephyrproject-rtos/hal_quicklogic being updated
DO NOT merge this PR until zephyrproject-rtos/hal_quicklogic#4 is merged

In order to get this right, this PR needs to pull in zephyrproject-rtos/hal_quicklogic#4 using west.yml, as we need a CI run containing the combined the changes before merging zephyrproject-rtos/hal_quicklogic#4

@zephyrbot
Copy link
Collaborator

zephyrbot commented Jul 3, 2023

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
hal_quicklogic zephyrproject-rtos/hal_quicklogic@b3a66fe zephyrproject-rtos/hal_quicklogic#4 zephyrproject-rtos/hal_quicklogic#4/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@Willmish
Copy link
Author

Willmish commented Jul 3, 2023

In order to get this right, this PR needs to pull in zephyrproject-rtos/hal_quicklogic#4 using west.yml, as we need a CI run containing the combined the changes before merging zephyrproject-rtos/hal_quicklogic#4

Ah! I was wondering if I can somehow add this module to CI, thanks for the pointer @henrikbrixandersen . Since my fork is sitting outside of the zephyrproject org, I modified the SHA revision and changed pointer to repo from repo-path to url pointing to my fork. This is done in: 9509d0e

Q1: Is that correct? and then after merging zephyrproject-rtos/hal_quicklogic#4 but before merging this, I'd change the commit to point to the tip commit SHA and revert the repo reference back to repo-path for the module?

Q2: I remember us talking during EOSS about rewriting the driver to directly do memory writes for the WishBone bus, rather than using a HAL. My current plan was to first conform with the QuickLogic HAL, and then when feeling confident enough dealing with Zephyr drivers and the SoC itself, update it to remove that abstraction layer. I am still unsure how to abstract out the common Wishbone bus initialisation, transmit and receive functions (https://github.com/Willmish/hal_quicklogic/blob/hal_i2c/HAL/src/eoss3_hal_wb.c) so that they would be accessible by all drivers (I2C, SPI etc), rather than maintaining copies of it for each driver. Possibly this is what would be kept in the module?
Does that sound like a good roadmap plan?

west.yml Outdated Show resolved Hide resolved
@zephyrbot zephyrbot added the DNM This PR should not be merged (Do Not Merge) label Jul 17, 2023
@Willmish
Copy link
Author

What would be the best approach to conform to the coding standards? should I apply a fix patch or squash most commits and apply the appropriate coding styles to each squashed commit in history? Looking at: https://docs.zephyrproject.org/latest/contribute/guidelines.html#clang-format

@fkokosinski
Copy link
Member

What would be the best approach to conform to the coding standards? should I apply a fix patch or squash most commits and apply the appropriate coding styles to each squashed commit in history? Looking at: https://docs.zephyrproject.org/latest/contribute/guidelines.html#clang-format

Hi! The latter sounds much better. In general, all commits in your PR should confront to the various checks and practices that Zephyr requires. You can run compliance checks locally with:

./scripts/ci/check_compliance.py -c main..

Modifies board initialisation file to setup proper pinmuxing for the
onboard user-programmable button. (Currently this was assumed to be
handled in the bootloader) Also modifies
soc/arm/quicklogic_eos_s3/soc_pinmap.h to follow the same macro format
as uart pinmux.

Signed-off-by: Szymon Duchniewicz <szduchniewicz@gmail.com>
* Add Flexible Fusion Engine Clock enabling for Wishbone bus communication
and I2C communication.

Signed-off-by: Szymon Duchniewicz <szduchniewicz@gmail.com>
Adds initial modifications to CMakeLists.txt file, Kconfig file,
creates a Kconfig.eos_s3 file and introduces a skeleton code for the
driver based on drivers/i2c/i2c_sifive.c.

Signed-off-by: Jakub Duchniewicz <j.duchniewicz@gmail.com>
Add initial I2C definition to the quicklogic_eos_s3.dtsi Device Tree
overlay. Enable i2c0 for quick_feather board.

Signed-off-by: Jakub Duchniewicz <j.duchniewicz@gmail.com>
* Add dts bindings for i2c driver for EOS S3.
* Add fixs to EOS S3 dts/dtsi files.
* Fix wrong Kconfig Macro - not matching the one generated from
dts/bbinding/quicklogic,eos-s3-i2c.yaml.
* drivers: i2c: i2c_eos_s3.c: Fix compilation errors.

Signed-off-by: Szymon Duchniewicz <szduchniewicz@gmail.com>
Update quick_feather Kconfig.defconfig to add BUILD_OUTPUT_BIN config
option. Update quick_feather dts to add copyright attribution.

Signed-off-by: Szymon Duchniewicz <szduchniewicz@gmail.com>
This commit changes module revision to point to the pull request adding
required hal changes.

Signed-off-by: Szymon Duchniewicz <szduchniewicz@gmail.com>
@Willmish
Copy link
Author

Cool, thanks @fkokosinski . Squashed some commits, cleaned up a bit and fixed commit messages, so should be ready for review? I may need some guidance in properly coding this function (as you might be able to tell by the comments): https://github.com/zephyrproject-rtos/zephyr/pull/59905/files#diff-586321414cf511e0b1f1b99e041b736fe987a5d03a43eb9073e47c7ca2943f26R131

But this should be passing all compliance checks and tests

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Sep 19, 2023
@Willmish
Copy link
Author

Hi, could I request the stale label to be removed so the pr doesnt close out? I was out for some of the summer so was unable to finish this off, but planning to do so in the next few weeks

@fkokosinski fkokosinski removed the Stale label Sep 19, 2023
@fkokosinski
Copy link
Member

Hi, could I request the stale label to be removed so the pr doesnt close out? I was out for some of the summer so was unable to finish this off, but planning to do so in the next few weeks

Sure thing. :) Done

@erwango erwango removed their assignment Sep 20, 2023
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Nov 20, 2023
@github-actions github-actions bot closed this Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DNM This PR should not be merged (Do Not Merge) manifest manifest-hal_quicklogic Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants