| Message ID | 20221117105249.115649-2-tudor.ambarus@microchip.com |
|---|---|
| State | Accepted |
| Commit | f6c911f3308c1cfb97ae1da6654080d7104e2df2 |
| Headers | show |
| Series | spi: Introduce spi-cs-setup-ns dt property | expand |
Hi, On 18.11.2022 17:30, Mark Brown wrote: > On Fri, Nov 18, 2022 at 03:14:58PM +0100, Michael Walle wrote: >> From: Tudor Ambarus <tudor.ambarus@microchip.com> > >>> + spi-cs-setup-ns: >>> + description: >>> + Delay in nanosecods to be introduced by the controller after CS is >>> + asserted. > >> Does this need a type as the spi-cs-setup-ns is apparently just 16bit? At >> least the driver uses it that way. > >> But IMHO this should just be a normal uint32 value to be consistent with >> all the other properties. Also the max value with 16bit will be 'just' >> 65us. > > Making it 32 bit does seem safer. I've applied the series Thanks. There are few implications to consider before making this prop a u32, and I'd like to check them with you. struct spi_delay will have to be updated to have a u32 value, now it's a u16. This means that we'll have to update spi_delay_to_ns() to either return a s64 or to add a u64 *delay parameter to the function so that we can still handle the conversions from usecs and the error codes in the SPI_DELAY_UNIT_SCK case. Then all its callers have to be updated to consider the u64 delay. I don't know what to say, I'm in between. 65us delays are improbable, but I'm fine to update this as well. Let me know your preference. Thanks, ta
Hi, Am 2023-01-02 10:37, schrieb Tudor Ambarus: > Hi, > > On 18.11.2022 17:30, Mark Brown wrote: >> On Fri, Nov 18, 2022 at 03:14:58PM +0100, Michael Walle wrote: >>> From: Tudor Ambarus <tudor.ambarus@microchip.com> >> >>>> + spi-cs-setup-ns: >>>> + description: >>>> + Delay in nanosecods to be introduced by the controller after >>>> CS is >>>> + asserted. >> >>> Does this need a type as the spi-cs-setup-ns is apparently just >>> 16bit? At >>> least the driver uses it that way. >> >>> But IMHO this should just be a normal uint32 value to be consistent >>> with >>> all the other properties. Also the max value with 16bit will be >>> 'just' >>> 65us. >> >> Making it 32 bit does seem safer. I've applied the series > > Thanks. There are few implications to consider before making this prop > a > u32, and I'd like to check them with you. > > struct spi_delay will have to be updated to have a u32 value, now it's > a > u16. This means that we'll have to update spi_delay_to_ns() to either > return a s64 or to add a u64 *delay parameter to the function so that > we > can still handle the conversions from usecs and the error codes in the > SPI_DELAY_UNIT_SCK case. Then all its callers have to be updated to > consider the u64 delay. I was talking about the device tree property. Even if the driver continue to use just 16bit, the DT property could be 32bit IMHO. At the moment, the schema says its 32bit (if I'm not mistaken, because it doesn't have a type), but the driver will parse the property as 16bit and your device tree also has this /bits/ thingy. So regardless if the driver is using 16bit or 32bit for the value, there seems to be a discrepancy between the schema and the devicetree (and driver). All other properties are just the regular 32bit values, thus I was suggesting to change the DT property to 32bit. -michael > I don't know what to say, I'm in between. 65us delays are improbable, > but I'm fine to update this as well. Let me know your preference. > > Thanks, > ta
On 02.01.2023 13:48, Michael Walle wrote: > Hi, Hi, > > Am 2023-01-02 10:37, schrieb Tudor Ambarus: >> Hi, >> >> On 18.11.2022 17:30, Mark Brown wrote: >>> On Fri, Nov 18, 2022 at 03:14:58PM +0100, Michael Walle wrote: >>>> From: Tudor Ambarus <tudor.ambarus@microchip.com> >>> >>>>> + spi-cs-setup-ns: >>>>> + description: >>>>> + Delay in nanosecods to be introduced by the controller after >>>>> CS is >>>>> + asserted. >>> >>>> Does this need a type as the spi-cs-setup-ns is apparently just >>>> 16bit? At >>>> least the driver uses it that way. >>> >>>> But IMHO this should just be a normal uint32 value to be consistent >>>> with >>>> all the other properties. Also the max value with 16bit will be 'just' >>>> 65us. >>> >>> Making it 32 bit does seem safer. I've applied the series >> >> Thanks. There are few implications to consider before making this prop a >> u32, and I'd like to check them with you. >> >> struct spi_delay will have to be updated to have a u32 value, now it's a >> u16. This means that we'll have to update spi_delay_to_ns() to either >> return a s64 or to add a u64 *delay parameter to the function so that we >> can still handle the conversions from usecs and the error codes in the >> SPI_DELAY_UNIT_SCK case. Then all its callers have to be updated to >> consider the u64 delay. > > I was talking about the device tree property. Even if the driver continue > to use just 16bit, the DT property could be 32bit IMHO. but then you'll have an implicit cast to u16 at: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi.c#n2314 which will make the u32 dt prop misleading. > > At the moment, the schema says its 32bit (if I'm not mistaken, because > it doesn't have a type), but the driver will parse the property as > 16bit and your device tree also has this /bits/ thingy. So regardless > if the driver is using 16bit or 32bit for the value, there seems to be > a discrepancy between the schema and the devicetree (and driver). okay, thanks for pointing it out. Let's decide how we fix this. > > All other properties are just the regular 32bit values, thus I was > suggesting to change the DT property to 32bit. If we want to change the dt prop to 32bit I think we should also handle the parsed value as u32, not as u16.
>>>>>> + spi-cs-setup-ns: >>>>>> + description: >>>>>> + Delay in nanosecods to be introduced by the controller >>>>>> after CS is >>>>>> + asserted. >>>> >>>>> Does this need a type as the spi-cs-setup-ns is apparently just >>>>> 16bit? At >>>>> least the driver uses it that way. >>>> >>>>> But IMHO this should just be a normal uint32 value to be consistent >>>>> with >>>>> all the other properties. Also the max value with 16bit will be >>>>> 'just' >>>>> 65us. >>>> >>>> Making it 32 bit does seem safer. I've applied the series >>> >>> Thanks. There are few implications to consider before making this >>> prop a >>> u32, and I'd like to check them with you. >>> >>> struct spi_delay will have to be updated to have a u32 value, now >>> it's a >>> u16. This means that we'll have to update spi_delay_to_ns() to either >>> return a s64 or to add a u64 *delay parameter to the function so that >>> we >>> can still handle the conversions from usecs and the error codes in >>> the >>> SPI_DELAY_UNIT_SCK case. Then all its callers have to be updated to >>> consider the u64 delay. >> >> I was talking about the device tree property. Even if the driver >> continue >> to use just 16bit, the DT property could be 32bit IMHO. > > but then you'll have an implicit cast to u16 at: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi.c#n2314 > which will make the u32 dt prop misleading. Nothing will prevent you from checking for a valid range and return an error :) But I agree, that converting the u16 to u32 in the driver is probably the better way. >> At the moment, the schema says its 32bit (if I'm not mistaken, because >> it doesn't have a type), but the driver will parse the property as >> 16bit and your device tree also has this /bits/ thingy. So regardless >> if the driver is using 16bit or 32bit for the value, there seems to be >> a discrepancy between the schema and the devicetree (and driver). > > okay, thanks for pointing it out. Let's decide how we fix this. > >> >> All other properties are just the regular 32bit values, thus I was >> suggesting to change the DT property to 32bit. > > If we want to change the dt prop to 32bit I think we should also handle > the parsed value as u32, not as u16. Strictly speaking, your device tree is wrong, because the schema already says it's 32bit. -michael
diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml index dca677f9e1b9..ead2cccf658f 100644 --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml @@ -44,6 +44,11 @@ properties: description: Maximum SPI clocking speed of the device in Hz. + spi-cs-setup-ns: + description: + Delay in nanosecods to be introduced by the controller after CS is + asserted. + spi-rx-bus-width: description: Bus width to the SPI bus used for read transfers.
SPI NOR flashes have specific cs-setup time requirements without which they can't work at frequencies close to their maximum supported frequency, as they miss the first bits of the instruction command. Unrecognized commands are ignored, thus the flash will be unresponsive. Introduce the spi-cs-setup-ns property to allow spi devices to specify their cs setup time. Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- .../devicetree/bindings/spi/spi-peripheral-props.yaml | 5 +++++ 1 file changed, 5 insertions(+)