-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
i2c: move nordic IPs to sda-gpios, scl-gpios #31965
i2c: move nordic IPs to sda-gpios, scl-gpios #31965
Conversation
c01a03a
to
3d79626
Compare
@pabigot @anangl this is picking up where we left off in #30769. I'm only doing TWI and TWIM for now to keep this relatively small since it's the first one. I've discussed with @carlescufi and I can commit to updating all of the following properties for Zephyr 2.6 in this and follow-up PRs:
|
7b689b2
to
2e0f3a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks like a great start to standardizing Nordic pin specification, just wondering if one piece needs to be pulled out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea looks good, and goes in the direction of having a more standard way to define gpios for I2C. So it is clear from the documentation how, let's say, P0.3 or P1.9 is transalted. Nevertheless it is difficult to me to review all the DTS change, as they are too many.
2e0f3a7
to
455f13d
Compare
Rebased onto #32001. |
a1fa309
to
c80b1e9
Compare
ae47040
to
97958d9
Compare
Seeing if temporarily increasing the buildkite parallelism will avoid CI timeouts. DNM since that patch should not be merged. |
134f2c8
to
3b9cf3b
Compare
Added a missing include to the TWI driver and rebased. |
7a49062
to
fb20bae
Compare
Remaining CI failures are due to #32619 as far as I can tell. |
Add some helper macros that will be convenient to use from device drivers for accessing and error checking pin mux information in the devicetree: - NRF_DT_PSEL(): get a PSEL value out of the DT from either a 'foo-pin' or a 'foo-gpios' style property. - NRF_DT_PSEL_CHECK_NOT_BOTH(), NRF_DT_PSEL_CHECK_EXACTLY_ONE(): helpers for checking that a given devicetree is OK according to different criteria for setting PSEL properties (NAND or XOR on whether the properties exist, respectively). See comments in the patch for more details. Signed-off-by: Martí Bolívar <[email protected]>
Deprecate the scl-pin and sda-pin properties in the devicetree. Provide new scl-gpios and sda-gpios properties instead. This lets the user specify SCL and SDA like this: &i2c0 { scl-gpios = <&gpio0 1 0>; sda-gpios = <&gpio1 4 0>; }; Instead of having to use: &i2c0 { scl-pin = <1>; sda-pin = <36>; }; Provide error checking and understandable error messages for invalid configurations. Signed-off-by: Martí Bolívar <[email protected]>
Move the BOARD.dts files for Nordic-based boards to use the new I2C devicetree properties for specifying the SDA and SCL pins. This was done with a script. Signed-off-by: Martí Bolívar <[email protected]>
Use tabs. Signed-off-by: Martí Bolívar <[email protected]>
I only see one sample with an overlay using the now legacy sda-pin and scl-pin properties. Move it to the new style. Signed-off-by: Martí Bolívar <[email protected]>
fb20bae
to
77aa842
Compare
All of the remaining CI failures contain the same message and look like timing nondeterminism to me:
I'm going to consider this green CI and will work with @carlescufi to get this merged tomorrow after removing the DNM patch increasing the bitkeeper parallelism. Hopefully this is it; thanks to everyone who reviewed. |
77aa842
to
25f910c
Compare
The PR has been merged, so following is just for the record. I believe the implementation presented in this PR does not follow Linux device tree specification. The Pins controlled by other hardware modules, i2c, uart, spi, etc. should use pinctrl-bindings. Unfortunately, it doesn't seem to be a good match for requirements imposed by the Nordic HAL so the now deprecated In general it's important to be able to distinguish if the pin is controlled by the the hardware module, e.g. i2c, spi or the GPIO module since there exist configurations when the same pin e.g. SS pin of the SPI bus is switched at runtime between SPI and GPIO hardware module. DT node describing such configuration would contain both: |
@mnkp I'm working through all of our bindings during this release to move them to However, I think it's important to be able to specify a GPIO controller phandle, pin, and flags. For some additional relevant background, I tried to get a build system target merged which produced a report of which pins were used by which peripherals in #24132. That used the existing I think a real phandle is also important so the I2C -> GPIO device dependency is tracked in things like DT_REQUIRES_DEP_ORDS(). A pin number is important for obvious reasons. As for flags, I was planning on replacing things like our miso-pull-up property with the relevant GPIO flags.
These are not simply HAL requirements. Each peripheral has registers which are programmed with the port and pin to use which were the actual This PR's approach of converting a phandle and pin to the value that goes into these registers is more readable. I am trying to avoid past support problems with people who didn't get how to encode and decode the Beyond these PSEL.MOSI style registers, the pins themselves are configured using the GPIO registers. I can rename these properties to something that's more appropriate than |
At first short introduction to the Linux pinctrl DT bindings. The Linux description of pin controller node in DT was developed around the notion that the SoC has a dedicated pin controller block, most SoCs do, that has its own register address and its main purpose is to route signals between pins and different SoC peripherals. It quickly become clear that the actual hardware design of pin muxing in SoCs put on the market by different vendors differs so much that it's not feasible to have one way to describe pin controller nodes in DT. That's why Linux pinctrl bindings make recommendations, show how to describe typical hardware but officially leave the actual implementation in the hands of the developer. That is, it's OK to have vendor specific implementation. About the only requirement that Linux DT imposes is that pin configuration on the client side, e.g. I2C module, will be defined using
It would be nice to have it done this way also for Nordic but it's not that easy and probably it's not worth it. For me it's fine if those families, like Nordic or SiLabs, that don't actually have pin controller hardware module use a vendor specific approach. The drawback here is that they will never have support for the official Zephyr pinctrl API. But surely they could have a vendor specific one. That said, the
Yes, sounds good. I believe we should use whatever is easiest, most natural for the user.
That looks good, it would be great to have it supported on all the platforms. However, a general solution won't be easy. Likely, the implementation would always need to be family specific. |
Tagging @carlescufi and @anangl on this feedback from @mnkp about the I think this is a rather serious drawback and we need to rethink our approach. |
@mnkp Just throwing an idea out here to check my understanding. What's wrong with something like this?
|
Nordic pinmuxing for I2C SDA and SCL pins is done with
sda-pin
andscl-pin
DTS properties. Move these to a more idiomaticsda-gpios
andscl-gpios
style instead.A prototypical example for how the devicetree should be updated is:
The old properties continue to be supported, but are now deprecated. Move in-tree boards and sample overlays to the new style.