WIP: RFC: Add timing and other parameters to spi peripherals #87427
Conversation
decsny
requested review from
henrikbrixandersen,
tbursztyka and
PetervdPerk-NXP
zephyrbot
added
platform: NXP Drivers
zephyrbot
requested review from
dbaluta,
dleach02,
manuargue,
mmahadevan108,
Raymond0225 and
teburd
decsny
force-pushed
the
spi_config_timing
branch
from
61ad693 to
8b0a93f
Compare
There was a problem hiding this comment.
This needs further thoughts.
For instance, you set the size of the ns delay to uint8_t, will that be sufficient in all cases?
Another issue as well: how device drivers are going to know which from the delay to the anonymous struct to use? there is no flag anywhere for mitigation. Unless we want to move to a new delay system, which case we will need to make delay attribute obsolete etc...
a few additional comments inline
| * This can be used to control a CS line via a GPIO line, instead of | ||
| * using the controller inner CS logic. | ||
| * This describes how the SPI CS line should be controlled, including whether | ||
| * to use GPIO or native hardware CS, and timing parameters. | ||
| * | ||
| */ | ||
| struct spi_cs_control { |
There was a problem hiding this comment.
Perhaps it's time to rename this to spi_cs_and_delay_control
There was a problem hiding this comment.
renaming would be an API change instead of an extension though, are you sure it's worth doing that, as opposed to just having a couple extra bytes in the overall spi_config struct (whether the way I have in this PR or introduce a new spi_timing struct)
There was a problem hiding this comment.
If we need an API change (whatever that could be), so be it. Better having a sane API rather than a half-hacked one.
There was a problem hiding this comment.
I see the effort to avoid api change, fine by me then
There was a problem hiding this comment.
there is some API change because of the macro change, but I did try to keep the struct backward compatible, and it is actually still used by gpio cs the same way therefore. So hopefully only breaking change is an obvious compile error due to macro having one less argument.
include/zephyr/drivers/spi.h
Outdated
|
|
||
| /** @brief Delay between data frames in nanoseconds (0 means 50% of frequency) */ | ||
| uint16_t dfs_delay_ns; |
There was a problem hiding this comment.
and this could go into the anonymous struct you added, in the union.
That way: to change in structure size.
That said, this is very much spi controller specific. Some controller do not offer anything to configure this.
And 0 as 50% of the frequency, why? why not just "hardware default"?
There was a problem hiding this comment.
I thought about this too but since the struct was called spi_cs_control and this was not related I thought it would be confusing
the reason for 50% of period is because of the same reason that is motivating this PR, which is that the hardware default for the NXP spi is the shortest possible, which causes problems and looks strange, and people often report it as a bug because the users actually expect that the behavior will be 50% of period by default, and don't even think about configuring it.
And this expectation is I think , not specific to people using the NXP part. A 50% delay of clk period between words means that the clock signal does not have any blips between words. Also this makes perhaps applications more portable because the hardware default can be totally different between parts but like I said maybe they just always expect 50% of period like has been reported to me
include/zephyr/drivers/spi.h
Outdated
| * Delay in microseconds to wait before starting the | ||
| * transmission and before releasing the CS line. | ||
| * Timing configuration. | ||
| * For GPIO CS, delay is microseconds to wait before starting |
There was a problem hiding this comment.
this was not only meant for gpio cs actually. thus why the structure was called spi_cs_control, and not spi_gpio_cs_control (for instance)
There was a problem hiding this comment.
yeah, that was convenient that it was already named generically, I noticed
I think almost all data sheets I have seen have CS delay parameters of < 10 ns, but if someone knows about some relevant hardware that is going to need delays of over 1/4 of a microsecond then I am open to putting them at uint16_t. I thought about doing this but the reason I put at uint8_t is because there are also some other CS related timing parameters that people might want to add in the future. But maybe not since I think many of them such as like pcs-pcs delay are not relevant to zephyr API since we expect CS asserted through whole spi_buf_set.
Actually, this is not an issue, and I did think it out, but does require a change on this PR, which is on my TODO list in the PR message, maybe I didn't explain it well or should have finished the work before submitting the PR, but I'm just low on bandwidth so wanted the idea reviewed before I did it. Basically this information about the chip select pretty much always comes from DT already right now, for GPIO CS. And there is no source of information for native CS. GPIO cs information comes from the cs-gpios property on the DT node and is initialized by the SPI_DT_SPEC_GET macro. That macro can just be edited to use initialize the struct using the new spi timing properties on the peripheral nodes if they are there and there is no cs-gpios, to initialize the struct to be gpio port as null and use the hw timing members of the union |
|
I forgot to put DNM since this PR is really only a WIP draft but I opened it as non-draft to get feedback on the idea from relevant reviewers |
The source of native CS is reg in spi-device.yaml. Not very much intuitive I agree as is also selects the right slot of cs-gpios when this one is present. |
I'm not seeing anything there right now, which property are you talking about? To be clear I am saying I will add it on this PR if we agree about this. Right now I just see To be clear, the main point of what I am saying here is that the spi clock timing parameters should be defined on the device/slave/peripheral DT node, and not the controller/master node. Right now we have at least spi-max-frequency on the SPI device nodes, but we also should put these other timing parameters which would come from the datasheets of spi devices to describe the digital AC characteristics of the signals required to interface properly to the device. Now as for whether to use a gpio or not for CS, that should remain on the controller node |
include/zephyr/drivers/spi.h
Outdated
| /** @brief Delay from PCS assertion to first SCK edge in nanoseconds. | ||
| * If set to 0, hardware default will be used. | ||
| */ | ||
| uint8_t pcs_to_sck_delay_ns; |
There was a problem hiding this comment.
Just curious what the standard is here, was looking at this relative to dw_spi and the delay is clock speed relative, not time relative. Might be worth also adding some helpers.
There was a problem hiding this comment.
For NXP, the register programming model is also relative to an equation relating some clock speed and transfer prescalers, and not nanoseconds. But since this is generic API it seems better to use a standard unit which would mean the same no matter the platform, and driver can calculate how to program hardware.
There was a problem hiding this comment.
Then that follows up that we should create some generic helpers if common platforms are going to be doing the same math
There was a problem hiding this comment.
No, because the math is completely platform specific, that's the point of why we need to specify in ns in the first place
|
FYI I plan to flesh out this PR based on discussion and remaining TODO I mentioned, but not a top priority for a few weeks probably |
|
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. |
|
I think I am going to try to work on this PR again, I think the goal specifically should be to associate timing parameters with the peripherals of the bus, as opposed to the controller, and then have the controller be able to tell what it should do based on what the peripheral needs. That's what is actually important I think. And not worrying so much about native vs gpio control etc. |
decsny
changed the title
decsny
force-pushed
the
spi_config_timing
branch
from
8b0a93f to
37016ed
Compare
|
@tbursztyka the PR is not quite finished but it is most of the way there I think. And the idea is much more mature now and I already completed a lot of the rework, so would appreciate your re-review before I go any further with it. |
zephyrbot
requested review from
avisconti,
MaureenHelm,
tristan-google and
yperess
decsny
force-pushed
the
spi_config_timing
branch
from
37016ed to
5d755c6
Compare
decsny
force-pushed
the
spi_config_timing
branch
from
5d755c6 to
2f42835
Compare
include/zephyr/drivers/spi.h
Outdated
| /* To keep track of which form of this struct is valid */ | ||
| bool cs_is_gpio; | ||
| /** True if the CS should be active high. False for active low. */ | ||
| bool active_high; |
There was a problem hiding this comment.
Isn't bit 14 in spi_config's operation the same? see SPI_CS_ACTIVE_HIGH
There was a problem hiding this comment.
true, I will see what to do about this
|
|
||
| #define SPI_CS_CONTROL_INIT_GPIO(node_id) \ | ||
| .gpio = SPI_CS_GPIOS_DT_SPEC_GET(node_id), \ | ||
| .delay = DIV_ROUND_UP(SPI_CS_CONTROL_MAX_DELAY(node_id), 1000), \ |
There was a problem hiding this comment.
I was wondering how the delay was set, that's nice
There was a problem hiding this comment.
yes for gpio I kept it as microseconds to avoid breakage, but for native controller control, it's now nanoseconds, which is more useful. most in tree cases use 1 microsecond probably because the datasheets usually specify minimum in nanoseconds and 1 microsecond was the smallest value to use before.
| * This can be used to control a CS line via a GPIO line, instead of | ||
| * using the controller inner CS logic. | ||
| * This describes how the SPI CS line should be controlled, including whether | ||
| * to use GPIO or native hardware CS, and timing parameters. | ||
| * | ||
| */ | ||
| struct spi_cs_control { |
There was a problem hiding this comment.
I see the effort to avoid api change, fine by me then
decsny
force-pushed
the
spi_config_timing
branch
2 times, most recently
from
107b083 to
a954537
Compare
|
@henrikbrixandersen would this need to go to Arch WG since it is a breaking change of stable API |
Add spi parameters for spi peripherals to spi-device.yaml. These properties and their names are mostly inspired by linux spi-peripheral-props.yaml. Signed-off-by: Declan Snyder <[email protected]>
The CS delay parameter did not make a distinction between the setup and hold time of the CS, and also did not specify very fine control which can be done usually by a native controller CS. So use the new nanosecond DT properties to get the delay values and make distinction. Signed-off-by: Declan Snyder <[email protected]>
Get bit ordering from DT property in SPI_CONFIG_DT. More precisely, set LSB first if set in DT. Get CS polarity from DT property in SPI_CONFIG_DT. Signed-off-by: Declan Snyder <[email protected]>
Add inter-word delay parameter to spi_config struct. If value is 0, the value will be half of the period. Signed-off-by: Declan Snyder <[email protected]>
Use the timing params from spi_config that are specific to the slave instead of using the same timing for the controller for all slaves. Remove these properties from the LPSPI DT binding. Signed-off-by: Declan Snyder <[email protected]>
This commit is TODO, I will probably convert all these RT overlays to inherit a common overlay but the idea will be like this. Signed-off-by: Declan Snyder <[email protected]>
decsny
force-pushed
the
spi_config_timing
branch
from
a954537 to
bdb61bd
Compare
|
Yes, it needs to follow the proces described here: https://docs.zephyrproject.org/latest/develop/api/api_lifecycle.html#introducing-breaking-api-changes |
|
@tbursztyka , the process brix linked above looks pretty heavy. I think the only breaking change is the number of arguments to the macro. Would it make sense to keep the macro the same and introduce a new macro that doesnt have the delay parameter, or is it worth making this breaking change for consistency? |
I overlooked this aspect, but in any case you will need a new macro and mark the old one obsolete. I don't think you can avoid the arch wg then. (I should be able to join, I'll try to follow the agenda) |
|
I am not trying to avoid the arch wg but avoid the TSC xD. I think the only breaking change is the macro, so keeping the old one with 3 arguments and obsoleting it and using a new one with two arguments would make it a non breaking API change |
Expand the struct spi_cs_control to be able to specify hardware timing parameters for a SPI peripheral, primarily intended to be generated from properties in DT on the spi device nodes, inspired by linux binding at https://www.kernel.org/doc/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
Support these parameters in NXP LPSPI driver
If the PR approach is okay, then TODO on this PR: