Designware (DW) I2C Slave Support and Raspberry Pi Pico I2C Support #42513

andrei-edward-popa · 2022-02-06T22:19:16Z

Added designware I2C support for slave mode
Added I2C support for Raspberry Pi Pico board. I2C chip on RP2040 is DW based.

added pinctrl include support for I2C designware chip Signed-off-by: Andrei-Edward Popa <[email protected]>

gmarull

Thanks for this PR, I have a few comments

drivers/i2c/i2c_dw.c

drivers/i2c/i2c_dw.h

boards/arm/rpi_pico/rpi_pico-pinctrl.dtsi

boards/arm/rpi_pico/rpi_pico.dts

added pinctrl support for designware i2c driver Signed-off-by: Andrei-Edward Popa <[email protected]>

gmarull

LGTM, pinctrl and board parts. Note: please check my I2C pull-up comment.

andrei-edward-popa · 2022-02-11T15:49:11Z

LGTM, pinctrl and board parts. Note: please check my I2C pull-up comment.

So, in this case, bias-pull-up needs to be removed?

gmarull · 2022-02-11T16:53:25Z

LGTM, pinctrl and board parts. Note: please check my I2C pull-up comment.

So, in this case, bias-pull-up needs to be removed?

I'm not 100% familiar with pico, so maybe its internal pull-ups are fine when working with I2C standard bitrate. You can check https://electronics.stackexchange.com/questions/1849/is-there-a-correct-resistance-value-for-i2c-pull-up-resistors

teburd

looks great to me, now I need to get a pico at some point

added slave mode support for I2C designware chip Signed-off-by: Andrei-Edward Popa <[email protected]>

added I2C pin macros for pinctrl of Raspberry Pi Pico board Signed-off-by: Andrei-Edward Popa <[email protected]>

yonsch

A few comments/questions about the RPi part (I didn't look at the I2C driver itself).

boards/arm/rpi_pico/rpi_pico-pinctrl.dtsi

yonsch · 2022-02-16T18:18:37Z

boards/arm/rpi_pico/rpi_pico.dts

I don't think this should be enabled by default. If I'm not wrong, peripherals should be disabled by default, and enabled in an overlay file (with the console UART and GPIO being the exceptions). But input from others on this topic is welcomed.

It's ok to have status = "okay";. Board doesn't set CONFIG_I2C=y, so i2c driver won't be enabled unless applications do.

But the rp2040 has two i2c peripherals, one might enable i2c to use one and unintentionally enable the other. It happened to me once on an STM32 board. When I enabled i2c, UART suddenly stopped working because an i2c peripheral using the same pin was enabled by default. It was quite a misery to debug.

So what do you suggest? To remove those ref nodes from dts or to make status disabled for both of them?

Applications can always override using an overlay, so if they want i2c1 and not i2c0, an overlay can be used:

&i2c0 { status = "disabled"; };

I tend to agree with @yonsch. Here is our policy:

https://docs.zephyrproject.org/latest/guides/porting/board_porting.html#general-recommendations

In particular (emphasis mine):

Unless explicitly recommended otherwise by this section, leave peripherals and their drivers disabled by default.

So @gmarull 's comment that the driver isn't enabled by default does not meet this requirement.

It's ok to have status = "okay";. Board doesn't set CONFIG_I2C=y, so i2c driver won't be enabled unless applications do.

@gmarull 's comment is correct, decisive is CONFIG_I2C and reflects our general practice, it is also consistent with board porting guide.

If we let boards add status = "disabled";, it means that we'll need many overlays in samples that are board-specific, whereas CONFIG_I2C=y in prj.conf works for all boards that have an I2C enabled in DT. I think it's a bit confusing, but I can't think of a better way.

Opened an issue to continue this discussion: #42998

Note that this would likely not happen with a custom out-of-tree board. In those cases, you generally describe what you have/need, and the application tends to be specifically designed for that hardware. Upstream, we have lots of eval kits or demo boards that do not have a specific purpose. Or sometimes, boards are meant to be plugged into an expansion kit, where hardware really gets defined from a board perspective (see for example https://www.tindie.com/products/arturo182/rp2040-stamp/). For maintainability reasons, I'd avoid going the path of disabling peripherals and require sample overlays.

boards/arm/rpi_pico/rpi_pico.yaml

added I2C device tree nodes based on I2C designware chip Signed-off-by: Andrei-Edward Popa <[email protected]>

added Raspberry Pi Pico for testing it as I2C slave Signed-off-by: Andrei-Edward Popa <[email protected]>

mbolivar-nordic

Unless the I2C peripherals are being enabled in some way that the board porting guidelines allow, I don't think we should use other boards as an excuse for behavior against policy here.

mbolivar-nordic · 2022-02-17T22:34:26Z

boards/arm/rpi_pico/rpi_pico.dts

I tend to agree with @yonsch. Here is our policy:

https://docs.zephyrproject.org/latest/guides/porting/board_porting.html#general-recommendations

In particular (emphasis mine):

Unless explicitly recommended otherwise by this section, leave peripherals and their drivers disabled by default.

So @gmarull 's comment that the driver isn't enabled by default does not meet this requirement.

Not worth blocking

mbolivar-nordic · 2022-02-17T22:42:52Z

On second thought, I think it's not worth blocking this PR over this, but I do want to make it clear that existing bad behavior is still bad behavior.

dts: bindings: i2c: added pinctrl include for DW

e4be9b4

added pinctrl include support for I2C designware chip Signed-off-by: Andrei-Edward Popa <[email protected]>

andrei-edward-popa requested review from MaureenHelm, carlescufi, dcpleung, galak and nashif as code owners February 6, 2022 22:19

github-actions bot added area: API Changes to public APIs area: Boards area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding area: I2C area: Tests Issues related to a particular existing or missing test labels Feb 6, 2022

zephyrbot requested review from mbolivar-nordic and teburd February 7, 2022 13:35

zephyrbot assigned teburd Feb 7, 2022

gmarull requested changes Feb 8, 2022

View reviewed changes

drivers: i2c: i2c_dw: added pinctrl support

b295133

added pinctrl support for designware i2c driver Signed-off-by: Andrei-Edward Popa <[email protected]>

andrei-edward-popa force-pushed the rpi_pico_i2c branch from a7d837b to ded0e15 Compare February 8, 2022 10:56

andrei-edward-popa requested a review from gmarull February 8, 2022 10:59

gmarull approved these changes Feb 8, 2022

View reviewed changes

teburd approved these changes Feb 11, 2022

View reviewed changes

Andrei-Edward Popa added 2 commits February 14, 2022 18:02

drivers: i2c: added slave support for DW

4996a9b

added slave mode support for I2C designware chip Signed-off-by: Andrei-Edward Popa <[email protected]>

include: dt-bindings: pinctrl: added I2C macros for Raspberry Pi Pico

3007cd2

added I2C pin macros for pinctrl of Raspberry Pi Pico board Signed-off-by: Andrei-Edward Popa <[email protected]>

andrei-edward-popa force-pushed the rpi_pico_i2c branch from ded0e15 to a977fd2 Compare February 14, 2022 16:02

yonsch reviewed Feb 16, 2022

View reviewed changes

andrei-edward-popa force-pushed the rpi_pico_i2c branch from a977fd2 to 6f272aa Compare February 16, 2022 19:55

github-actions bot added the area: Documentation label Feb 16, 2022

andrei-edward-popa force-pushed the rpi_pico_i2c branch from 6f272aa to 5a0dead Compare February 17, 2022 11:45

Andrei-Edward Popa added 2 commits February 17, 2022 14:12

dts: arm: rpi_pico: added I2C support for Raspberry Pi Pico board

97354b3

added I2C device tree nodes based on I2C designware chip Signed-off-by: Andrei-Edward Popa <[email protected]>

tests: drivers: i2c: slave: added Raspberry Pi Pico board

7a75180

added Raspberry Pi Pico for testing it as I2C slave Signed-off-by: Andrei-Edward Popa <[email protected]>

andrei-edward-popa force-pushed the rpi_pico_i2c branch from 5a0dead to 7a75180 Compare February 17, 2022 12:12

mbolivar-nordic previously requested changes Feb 17, 2022

View reviewed changes

yonsch mentioned this pull request Feb 19, 2022

Should board.dts enable peripherals by default? #42998

Closed

nashif merged commit 34d921b into zephyrproject-rtos:main Feb 22, 2022

andrei-edward-popa deleted the rpi_pico_i2c branch February 22, 2022 01:00

andrei-edward-popa restored the rpi_pico_i2c branch May 4, 2022 13:03

andrei-edward-popa deleted the rpi_pico_i2c branch May 4, 2022 13:04

str4t0m added the platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) label Jul 18, 2022

soburi mentioned this pull request Jan 16, 2023

TaskList for Supporting RaspberryPi Pico peripherals #53810

Open

57 tasks

RafaelLaya mentioned this pull request Apr 5, 2024

Fixes Master Mode for I2C DW Driver #71115

Merged

Designware (DW) I2C Slave Support and Raspberry Pi Pico I2C Support #42513

Designware (DW) I2C Slave Support and Raspberry Pi Pico I2C Support #42513

Uh oh!

Conversation

andrei-edward-popa commented Feb 6, 2022

Uh oh!

gmarull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gmarull left a comment

Choose a reason for hiding this comment

Uh oh!

andrei-edward-popa commented Feb 11, 2022

Uh oh!

gmarull commented Feb 11, 2022

Uh oh!

teburd left a comment

Choose a reason for hiding this comment

Uh oh!

yonsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yonsch Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbolivar-nordic Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mbolivar-nordic left a comment

Choose a reason for hiding this comment

Uh oh!

mbolivar-nordic Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbolivar-nordic commented Feb 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

yonsch Feb 16, 2022 •

edited

Loading

mbolivar-nordic Feb 17, 2022 •

edited

Loading

mbolivar-nordic Feb 17, 2022 •

edited

Loading