| Message ID | 36eefb860f660e2cadb13b00aca04b5a65498993.1718749981.git.marcelo.schmitt@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for AD4000 series of ADCs | expand |
On Tue, Jun 18, 2024 at 08:10:44PM -0300, Marcelo Schmitt wrote: > First replies to v3 brought the idea of having a feature detection mechanism. > I didn't really get how to do that. If feature detection is required, can > somebody please provide some pointers on how to implement that? Look at the checks in spi_setup() for bad_bits.
On 06/19, Mark Brown wrote: > On Tue, Jun 18, 2024 at 08:10:44PM -0300, Marcelo Schmitt wrote: > > > First replies to v3 brought the idea of having a feature detection mechanism. > > I didn't really get how to do that. If feature detection is required, can > > somebody please provide some pointers on how to implement that? > > Look at the checks in spi_setup() for bad_bits. Yes, I added MOSI idle configuration bits to bad_bits so spi_setup() fails if the feature is requested but not supported: bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD | SPI_NO_TX | SPI_NO_RX | SPI_MOSI_IDLE_LOW | SPI_MOSI_IDLE_HIGH); am I still missing anything? Thanks, Marcelo
On Wed, Jun 19, 2024 at 09:42:23AM -0300, Marcelo Schmitt wrote: > On 06/19, Mark Brown wrote: > > On Tue, Jun 18, 2024 at 08:10:44PM -0300, Marcelo Schmitt wrote: > > > First replies to v3 brought the idea of having a feature detection mechanism. > > > I didn't really get how to do that. If feature detection is required, can > > > somebody please provide some pointers on how to implement that? > > Look at the checks in spi_setup() for bad_bits. > Yes, I added MOSI idle configuration bits to bad_bits so spi_setup() fails > if the feature is requested but not supported: > bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD | > SPI_NO_TX | SPI_NO_RX | SPI_MOSI_IDLE_LOW | > SPI_MOSI_IDLE_HIGH); > am I still missing anything? That should be fine.
On 6/18/24 6:10 PM, Marcelo Schmitt wrote: > + > +MOSI idle state configuration > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > +Common SPI protocol implementations don't specify any state or behavior for the > +MOSI line when the controller is not clocking out data. However, there do exist > +peripherals that require specific MOSI line state when data is not being clocked > +out. For example, if the peripheral expects the MOSI line to be high when the > +controller is not clocking out data (SPI_MOSI_IDLE_HIGH), then a transfer in SPI > +mode 0 would look like the following: > + > +:: > + > + nCSx ___ ___ > + \_________________________________________________________________/ > + • • > + • • > + SCLK ___ ___ ___ ___ ___ ___ ___ ___ > + _______/ \___/ \___/ \___/ \___/ \___/ \___/ \___/ \_____ > + • : ; : ; : ; : ; : ; : ; : ; : ; • > + • : ; : ; : ; : ; : ; : ; : ; : ; • > + MOSI _____ _______ _______ _______________ ___ > + 0x56 \_0_____/ 1 \_0_____/ 1 \_0_____/ 1 1 \_0_____/ > + • ; ; ; ; ; ; ; ; • > + • ; ; ; ; ; ; ; ; • > + MISO XXX__________ _______________________ _______ XXX > + 0xBA XXX__/ 1 \_____0_/ 1 1 1 \_____0__/ 1 \____0__XXX > + > +Legend:: > + > + • marks the start/end of transmission; > + : marks when data is clocked into the peripheral; > + ; marks when data is clocked into the controller; > + X marks when line states are not specified. > + > +In this extension to the usual SPI protocol, the MOSI line state is specified to > +be kept high when CS is active but the controller is not clocking out data to I think it would be less ambiguous to say "asserted" instead of "active". > +the peripheral and also when CS is inactive. As I mentioned in a previous review, I think the key detail here is that the MOSI line has to be in the required state during the CS line assertion (falling edge). I didn't really get that from the current wording. The current wording makes it sound like MOSI needs to be high indefinitely longer. > + > +Peripherals that require this extension must request it by setting the > +SPI_MOSI_IDLE_HIGH bit into the mode attribute of their struct spi_device and Could use inline code formatting for C code bits, e.g. ``struct spi_device`` ``SPI_MOSI_IDLE_HIGH``, etc. > +call spi_setup(). Controllers that support this extension should indicate it by> +setting SPI_MOSI_IDLE_HIGH in the mode_bits attribute of their struct > +spi_controller. The configuration to idle MOSI low is analogous but uses the > +SPI_MOSI_IDLE_LOW mode bit. > + > + > THANKS TO > --------- > Contributors to Linux-SPI discussions include (in alphabetical order, ... > index e8e1e798924f..8e50a8559225 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -599,6 +599,12 @@ struct spi_controller { > * assert/de-assert more than one chip select at once. > */ > #define SPI_CONTROLLER_MULTI_CS BIT(7) > + /* > + * The spi-controller is capable of keeping the MOSI line low or high > + * when not clocking out data. > + */ > +#define SPI_CONTROLLER_MOSI_IDLE_LOW BIT(8) /* Can idle MOSI low */ > +#define SPI_CONTROLLER_MOSI_IDLE_HIGH BIT(9) /* Can idle MOSI high */ I don't see where these are used anywhere else in the series. They seem redundant with SPI_MOSI_IDLE_LOW and SPI_MOSI_IDLE_HIGH. > > /* Flag indicating if the allocation of this struct is devres-managed */ > bool devm_allocated; > diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h > index ca56e477d161..ee4ac812b8f8 100644 > --- a/include/uapi/linux/spi/spi.h > +++ b/include/uapi/linux/spi/spi.h > @@ -28,7 +28,8 @@ > #define SPI_RX_OCTAL _BITUL(14) /* receive with 8 wires */ > #define SPI_3WIRE_HIZ _BITUL(15) /* high impedance turnaround */ > #define SPI_RX_CPHA_FLIP _BITUL(16) /* flip CPHA on Rx only xfer */ > -#define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave mosi line low when idle */ > +#define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave MOSI line low when idle */ > +#define SPI_MOSI_IDLE_HIGH _BITUL(18) /* leave MOSI line high when idle */ > > /* > * All the bits defined above should be covered by SPI_MODE_USER_MASK. > @@ -38,6 +39,6 @@ > * These bits must not overlap. A static assert check should make sure of that. > * If adding extra bits, make sure to increase the bit index below as well. > */ > -#define SPI_MODE_USER_MASK (_BITUL(18) - 1) > +#define SPI_MODE_USER_MASK (_BITUL(19) - 1) > > #endif /* _UAPI_SPI_H */
On 6/18/24 6:10 PM, Marcelo Schmitt wrote: ... > @@ -3928,7 +3934,8 @@ int spi_setup(struct spi_device *spi) > * so it is ignored here. > */ > bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD | > - SPI_NO_TX | SPI_NO_RX); > + SPI_NO_TX | SPI_NO_RX | SPI_MOSI_IDLE_LOW | > + SPI_MOSI_IDLE_HIGH); This looks wrong to me. Adding flags here causes them to be ignored rather than to be checked. I also did a runtime check with a random driver and a SPI controller that does not have the flag. spi->mode |= SPI_MOSI_IDLE_LOW; ret = spi_setup(spi); if (ret) return ret; It incorrectly passes when used with this change but correctly fails without this change. > ugly_bits = bad_bits & > (SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL | > SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL);
On 06/19, David Lechner wrote: > On 6/18/24 6:10 PM, Marcelo Schmitt wrote: > > > > + > > +MOSI idle state configuration > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > + > > +Common SPI protocol implementations don't specify any state or behavior for the > > +MOSI line when the controller is not clocking out data. However, there do exist > > +peripherals that require specific MOSI line state when data is not being clocked > > +out. For example, if the peripheral expects the MOSI line to be high when the > > +controller is not clocking out data (SPI_MOSI_IDLE_HIGH), then a transfer in SPI > > +mode 0 would look like the following: > > + > > +:: > > + > > + nCSx ___ ___ > > + \_________________________________________________________________/ > > + • • > > + • • > > + SCLK ___ ___ ___ ___ ___ ___ ___ ___ > > + _______/ \___/ \___/ \___/ \___/ \___/ \___/ \___/ \_____ > > + • : ; : ; : ; : ; : ; : ; : ; : ; • > > + • : ; : ; : ; : ; : ; : ; : ; : ; • > > + MOSI _____ _______ _______ _______________ ___ > > + 0x56 \_0_____/ 1 \_0_____/ 1 \_0_____/ 1 1 \_0_____/ > > + • ; ; ; ; ; ; ; ; • > > + • ; ; ; ; ; ; ; ; • > > + MISO XXX__________ _______________________ _______ XXX > > + 0xBA XXX__/ 1 \_____0_/ 1 1 1 \_____0__/ 1 \____0__XXX > > + > > +Legend:: > > + > > + • marks the start/end of transmission; > > + : marks when data is clocked into the peripheral; > > + ; marks when data is clocked into the controller; > > + X marks when line states are not specified. > > + > > +In this extension to the usual SPI protocol, the MOSI line state is specified to > > +be kept high when CS is active but the controller is not clocking out data to > > I think it would be less ambiguous to say "asserted" instead of "active". I'm not sure. IMHO, it looks less ambiguous to say a CS is active. I think the most common for CS lines is to have a CS that is active low (i.e. the line is at a low voltage level when the controller is selecting the device). To me, "assert" sounds closer to the idea o setting something (like a bit to 1), which is the opposite of active low CS. Though, no strong opinion about it. I go with what the maintainers prefer. > > > +the peripheral and also when CS is inactive. > > As I mentioned in a previous review, I think the key detail here is that the > MOSI line has to be in the required state during the CS line assertion > (falling edge). I didn't really get that from the current wording. The current > wording makes it sound like MOSI needs to be high indefinitely longer. It may be that we only need MOSI high just before bringing CS low. Though, I don't see that info in the datasheets. How much time would MOSI be required to be high prior to bringing CS low? The timing diagrams for register access and ADC sampling in "3-wire" mode all start and end with MOSI at logical 1 (high). I think reg access work if MOSI is brought low after CS gets low, but sample read definitely don't work. From the info available in datasheets, it looks like MOSI is indeed expected to be high indefinitely amount of time. Except when the controller is clocking out data to the peripheral. Even if find out the amount of time MOSI would be required high prior to CS low, then we would need some sort of MOSI high/low state set with a delay prior to active CS. That might be enough to support the AD4000 series of devices but, would it be worth the added complexity? > > > + > > +Peripherals that require this extension must request it by setting the > > +SPI_MOSI_IDLE_HIGH bit into the mode attribute of their struct spi_device and > > Could use inline code formatting for C code bits, e.g. ``struct spi_device`` > ``SPI_MOSI_IDLE_HIGH``, etc. ok, updated those for v5. > > > +call spi_setup(). Controllers that support this extension should indicate it by> +setting SPI_MOSI_IDLE_HIGH in the mode_bits attribute of their struct > > +spi_controller. The configuration to idle MOSI low is analogous but uses the > > +SPI_MOSI_IDLE_LOW mode bit. > > + > > + > > THANKS TO > > --------- > > Contributors to Linux-SPI discussions include (in alphabetical order, > > ... > > > index e8e1e798924f..8e50a8559225 100644 > > --- a/include/linux/spi/spi.h > > +++ b/include/linux/spi/spi.h > > @@ -599,6 +599,12 @@ struct spi_controller { > > * assert/de-assert more than one chip select at once. > > */ > > #define SPI_CONTROLLER_MULTI_CS BIT(7) > > + /* > > + * The spi-controller is capable of keeping the MOSI line low or high > > + * when not clocking out data. > > + */ > > +#define SPI_CONTROLLER_MOSI_IDLE_LOW BIT(8) /* Can idle MOSI low */ > > +#define SPI_CONTROLLER_MOSI_IDLE_HIGH BIT(9) /* Can idle MOSI high */ > > I don't see where these are used anywhere else in the series. They > seem redundant with SPI_MOSI_IDLE_LOW and SPI_MOSI_IDLE_HIGH. > Good point. They are currently not being used. Comparing with what we have for SPI_CONTROLLER_MULTI_CS, I'm thinking it may be handy to have dt properties to indicate controller MOSI idle capabilities. Does that sound reasonable? > > > > /* Flag indicating if the allocation of this struct is devres-managed */ > > bool devm_allocated; > > diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h > > index ca56e477d161..ee4ac812b8f8 100644 > > --- a/include/uapi/linux/spi/spi.h > > +++ b/include/uapi/linux/spi/spi.h > > @@ -28,7 +28,8 @@ > > #define SPI_RX_OCTAL _BITUL(14) /* receive with 8 wires */ > > #define SPI_3WIRE_HIZ _BITUL(15) /* high impedance turnaround */ > > #define SPI_RX_CPHA_FLIP _BITUL(16) /* flip CPHA on Rx only xfer */ > > -#define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave mosi line low when idle */ > > +#define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave MOSI line low when idle */ > > +#define SPI_MOSI_IDLE_HIGH _BITUL(18) /* leave MOSI line high when idle */ > > > > /* > > * All the bits defined above should be covered by SPI_MODE_USER_MASK. > > @@ -38,6 +39,6 @@ > > * These bits must not overlap. A static assert check should make sure of that. > > * If adding extra bits, make sure to increase the bit index below as well. > > */ > > -#define SPI_MODE_USER_MASK (_BITUL(18) - 1) > > +#define SPI_MODE_USER_MASK (_BITUL(19) - 1) > > > > #endif /* _UAPI_SPI_H */ >
On 6/19/24 1:58 PM, Marcelo Schmitt wrote: > On 06/19, David Lechner wrote: >> On 6/18/24 6:10 PM, Marcelo Schmitt wrote: >> >> ... >> >>> +the peripheral and also when CS is inactive. >> >> As I mentioned in a previous review, I think the key detail here is that the >> MOSI line has to be in the required state during the CS line assertion >> (falling edge). I didn't really get that from the current wording. The current >> wording makes it sound like MOSI needs to be high indefinitely longer. > > It may be that we only need MOSI high just before bringing CS low. Though, > I don't see that info in the datasheets. How much time would MOSI be required > to be high prior to bringing CS low? The timing diagrams for register access and > ADC sampling in "3-wire" mode all start and end with MOSI at logical 1 (high). > I think reg access work if MOSI is brought low after CS gets low, but sample > read definitely don't work. > > From the info available in datasheets, it looks like MOSI is indeed expected > to be high indefinitely amount of time. Except when the controller is clocking > out data to the peripheral. > > Even if find out the amount of time MOSI would be required high prior to CS low, > then we would need some sort of MOSI high/low state set with a delay prior to > active CS. That might be enough to support the AD4000 series of devices but, > would it be worth the added complexity? > It needs to happen at the same time as setting CPOL for the SCLK line for the device that is about to have the CS asserted. So I don't think we are breaking new ground here. Typically, in most datasheets I've seen they tend to say something like 2 ns before the CS change. So in most cases, I don't think anyone bothers adding a delay. But if a longer delay was really needed for a specific peripheral, we could add a SPI xfer with no read/write that has cs_off=1 and a delay to get the correct state of both MOSI and SCLK a longer time before the CS change. >> >>> index e8e1e798924f..8e50a8559225 100644 >>> --- a/include/linux/spi/spi.h >>> +++ b/include/linux/spi/spi.h >>> @@ -599,6 +599,12 @@ struct spi_controller { >>> * assert/de-assert more than one chip select at once. >>> */ >>> #define SPI_CONTROLLER_MULTI_CS BIT(7) >>> + /* >>> + * The spi-controller is capable of keeping the MOSI line low or high >>> + * when not clocking out data. >>> + */ >>> +#define SPI_CONTROLLER_MOSI_IDLE_LOW BIT(8) /* Can idle MOSI low */ >>> +#define SPI_CONTROLLER_MOSI_IDLE_HIGH BIT(9) /* Can idle MOSI high */ >> >> I don't see where these are used anywhere else in the series. They >> seem redundant with SPI_MOSI_IDLE_LOW and SPI_MOSI_IDLE_HIGH. >> > Good point. > They are currently not being used. > Comparing with what we have for SPI_CONTROLLER_MULTI_CS, I'm thinking it may be > handy to have dt properties to indicate controller MOSI idle capabilities. > Does that sound reasonable? I don't see any properties in spi-controller.yaml related to SPI_CONTROLLER_MULTI_CS. So I don't see how a property for the idle capabilities would be useful there.
On Wed, Jun 19, 2024 at 03:58:00PM -0300, Marcelo Schmitt wrote: > On 06/19, David Lechner wrote: > > On 6/18/24 6:10 PM, Marcelo Schmitt wrote: > > > +In this extension to the usual SPI protocol, the MOSI line state is specified to > > > +be kept high when CS is active but the controller is not clocking out data to > > I think it would be less ambiguous to say "asserted" instead of "active". > I'm not sure. IMHO, it looks less ambiguous to say a CS is active. > I think the most common for CS lines is to have a CS that is active low (i.e. > the line is at a low voltage level when the controller is selecting the device). > To me, "assert" sounds closer to the idea o setting something (like a bit to 1), > which is the opposite of active low CS. > Though, no strong opinion about it. > I go with what the maintainers prefer. I think they're synonyms but asserted is the more common term for chip selects. > > > +#define SPI_CONTROLLER_MOSI_IDLE_LOW BIT(8) /* Can idle MOSI low */ > > > +#define SPI_CONTROLLER_MOSI_IDLE_HIGH BIT(9) /* Can idle MOSI high */ > > I don't see where these are used anywhere else in the series. They > > seem redundant with SPI_MOSI_IDLE_LOW and SPI_MOSI_IDLE_HIGH. > Good point. > They are currently not being used. > Comparing with what we have for SPI_CONTROLLER_MULTI_CS, I'm thinking it may be > handy to have dt properties to indicate controller MOSI idle capabilities. > Does that sound reasonable? We shouldn't need DT properties, we should just know if the controller supports this based on knowing what controller is, and I'd not expect it to depend on board wiring.
On 06/19, David Lechner wrote: > On 6/18/24 6:10 PM, Marcelo Schmitt wrote: > > ... > > > @@ -3928,7 +3934,8 @@ int spi_setup(struct spi_device *spi) > > * so it is ignored here. > > */ > > bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD | > > - SPI_NO_TX | SPI_NO_RX); > > + SPI_NO_TX | SPI_NO_RX | SPI_MOSI_IDLE_LOW | > > + SPI_MOSI_IDLE_HIGH); > > This looks wrong to me. Adding flags here causes them to be ignored > rather than to be checked. > > I also did a runtime check with a random driver and a SPI controller > that does not have the flag. > > spi->mode |= SPI_MOSI_IDLE_LOW; > ret = spi_setup(spi); > if (ret) > return ret; > > It incorrectly passes when used with this change but correctly fails > without this change. That's right, adding flags to bad_bits makes spi_setup() ignore those mode bits instead of failing when they don't match. Changed bad_bits assignment back to what it was in v3 (i.e. no change to bad_bits). bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD | SPI_NO_TX | SPI_NO_RX); > > > ugly_bits = bad_bits & > > (SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL | > > SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL); >
On 06/19, David Lechner wrote: > On 6/19/24 1:58 PM, Marcelo Schmitt wrote: > > On 06/19, David Lechner wrote: > >> On 6/18/24 6:10 PM, Marcelo Schmitt wrote: > >> > >> > > ... > > >> > >>> +the peripheral and also when CS is inactive. > >> > >> As I mentioned in a previous review, I think the key detail here is that the > >> MOSI line has to be in the required state during the CS line assertion > >> (falling edge). I didn't really get that from the current wording. The current > >> wording makes it sound like MOSI needs to be high indefinitely longer. > > > > It may be that we only need MOSI high just before bringing CS low. Though, > > I don't see that info in the datasheets. How much time would MOSI be required > > to be high prior to bringing CS low? The timing diagrams for register access and > > ADC sampling in "3-wire" mode all start and end with MOSI at logical 1 (high). > > I think reg access work if MOSI is brought low after CS gets low, but sample > > read definitely don't work. > > > > From the info available in datasheets, it looks like MOSI is indeed expected > > to be high indefinitely amount of time. Except when the controller is clocking > > out data to the peripheral. > > > > Even if find out the amount of time MOSI would be required high prior to CS low, > > then we would need some sort of MOSI high/low state set with a delay prior to > > active CS. That might be enough to support the AD4000 series of devices but, > > would it be worth the added complexity? > > > > It needs to happen at the same time as setting CPOL for the SCLK line for the > device that is about to have the CS asserted. So I don't think we are breaking > new ground here. Typically, in most datasheets I've seen they tend to say > something like 2 ns before the CS change. So in most cases, I don't think which datasheets? Are any of those for devices supported by the ad4000 driver? > anyone bothers adding a delay. But if a longer delay was really needed for > a specific peripheral, we could add a SPI xfer with no read/write that has > cs_off=1 and a delay to get the correct state of both MOSI and SCLK a longer > time before the CS change. I don't know if that would actually work. I have not tested doing something like that. This also implies the controller will be able to start the next transfer right after the first preparatory transfer ends and it will meet that inter-transfer timing requirement (which I still didn't find documented anywhere). I'm not convinced that would be the best way to support those devices.
On 06/19, Mark Brown wrote: > On Wed, Jun 19, 2024 at 03:58:00PM -0300, Marcelo Schmitt wrote: > > On 06/19, David Lechner wrote: > > > On 6/18/24 6:10 PM, Marcelo Schmitt wrote: > > > > > +In this extension to the usual SPI protocol, the MOSI line state is specified to > > > > +be kept high when CS is active but the controller is not clocking out data to > > > > I think it would be less ambiguous to say "asserted" instead of "active". ack, replaced "active" by "asserted" when describing CS state for v5. > > > I'm not sure. IMHO, it looks less ambiguous to say a CS is active. > > I think the most common for CS lines is to have a CS that is active low (i.e. > > the line is at a low voltage level when the controller is selecting the device). > > To me, "assert" sounds closer to the idea o setting something (like a bit to 1), > > which is the opposite of active low CS. > > Though, no strong opinion about it. > > I go with what the maintainers prefer. > > I think they're synonyms but asserted is the more common term for chip > selects. > > > > > > +#define SPI_CONTROLLER_MOSI_IDLE_LOW BIT(8) /* Can idle MOSI low */ > > > > +#define SPI_CONTROLLER_MOSI_IDLE_HIGH BIT(9) /* Can idle MOSI high */ > > > > I don't see where these are used anywhere else in the series. They > > > seem redundant with SPI_MOSI_IDLE_LOW and SPI_MOSI_IDLE_HIGH. > > > Good point. > > They are currently not being used. > > Comparing with what we have for SPI_CONTROLLER_MULTI_CS, I'm thinking it may be > > handy to have dt properties to indicate controller MOSI idle capabilities. > > Does that sound reasonable? > > We shouldn't need DT properties, we should just know if the controller > supports this based on knowing what controller is, and I'd not expect it > to depend on board wiring. Okay, though, I fail to see the need for #define SPI_CONTROLLER_MOSI_IDLE_LOW BIT(8) /* Can idle MOSI low */ #define SPI_CONTROLLER_MOSI_IDLE_HIGH BIT(9) /* Can idle MOSI high */ It looks like SPI_CONTROLLER bits are used to tweak controller operation in various ways. Right now, I'm not aware of any additional tweak needed to support the MOSI idle feature. I have tested that on Raspberry Pi with bitbang/gpio controller and on CoraZ7 with spi-engine and it did work fine in those setups. Anyway, I'm prone to implement any additional changes to make this set better. Thanks, Marcelo
On 6/20/24 10:12 AM, Marcelo Schmitt wrote: > On 06/19, David Lechner wrote: >> On 6/19/24 1:58 PM, Marcelo Schmitt wrote: >>> On 06/19, David Lechner wrote: >>>> On 6/18/24 6:10 PM, Marcelo Schmitt wrote: >>>> >>>> >> >> ... >> >>>> >>>>> +the peripheral and also when CS is inactive. >>>> >>>> As I mentioned in a previous review, I think the key detail here is that the >>>> MOSI line has to be in the required state during the CS line assertion >>>> (falling edge). I didn't really get that from the current wording. The current >>>> wording makes it sound like MOSI needs to be high indefinitely longer. >>> >>> It may be that we only need MOSI high just before bringing CS low. Though, >>> I don't see that info in the datasheets. How much time would MOSI be required >>> to be high prior to bringing CS low? The timing diagrams for register access and >>> ADC sampling in "3-wire" mode all start and end with MOSI at logical 1 (high). >>> I think reg access work if MOSI is brought low after CS gets low, but sample >>> read definitely don't work. >>> >>> From the info available in datasheets, it looks like MOSI is indeed expected >>> to be high indefinitely amount of time. Except when the controller is clocking >>> out data to the peripheral. >>> >>> Even if find out the amount of time MOSI would be required high prior to CS low, >>> then we would need some sort of MOSI high/low state set with a delay prior to >>> active CS. That might be enough to support the AD4000 series of devices but, >>> would it be worth the added complexity? >>> >> >> It needs to happen at the same time as setting CPOL for the SCLK line for the >> device that is about to have the CS asserted. So I don't think we are breaking >> new ground here. Typically, in most datasheets I've seen they tend to say >> something like 2 ns before the CS change. So in most cases, I don't think > which datasheets? Are any of those for devices supported by the ad4000 driver? In the AD4000 datasheet, Figure 59, it shows the time needed for SDI setup before CS assertion, labeled as t_SSDICNV. Table 2 gives this value to be 2 ns. So unless a SPI controller has a functional clock of > 500 MHz or somehow sets the SDI state and the CS assertion in the same register write this minimum delay will always be met. I highly suspect noting like this has happened before so no one ever needed to worry about the timing and it just works (for the similar case of CPOL). > >> anyone bothers adding a delay. But if a longer delay was really needed for >> a specific peripheral, we could add a SPI xfer with no read/write that has >> cs_off=1 and a delay to get the correct state of both MOSI and SCLK a longer >> time before the CS change. > > I don't know if that would actually work. I have not tested doing something like that. > This also implies the controller will be able to start the next transfer right > after the first preparatory transfer ends and it will meet that inter-transfer > timing requirement (which I still didn't find documented anywhere). > I'm not convinced that would be the best way to support those devices. I did something like this in the ad7944 driver where we needed an up to 500ns delay before asserting CS. On SPI controllers without a hardware sleep or FIFO, the delay will of course be much longer. But the delay is just a minimum delay, so longer doesn't hurt. It just affects the max sample rate that can be reliably achieved.
On 06/20, David Lechner wrote: > On 6/20/24 10:12 AM, Marcelo Schmitt wrote: > > On 06/19, David Lechner wrote: > >> On 6/19/24 1:58 PM, Marcelo Schmitt wrote: > >>> On 06/19, David Lechner wrote: > >>>> On 6/18/24 6:10 PM, Marcelo Schmitt wrote: > >>>> > >>>> > >> > >> ... > >> > >>>> > >>>>> +the peripheral and also when CS is inactive. > >>>> > >>>> As I mentioned in a previous review, I think the key detail here is that the > >>>> MOSI line has to be in the required state during the CS line assertion > >>>> (falling edge). I didn't really get that from the current wording. The current > >>>> wording makes it sound like MOSI needs to be high indefinitely longer. > >>> > >>> It may be that we only need MOSI high just before bringing CS low. Though, > >>> I don't see that info in the datasheets. How much time would MOSI be required > >>> to be high prior to bringing CS low? The timing diagrams for register access and > >>> ADC sampling in "3-wire" mode all start and end with MOSI at logical 1 (high). > >>> I think reg access work if MOSI is brought low after CS gets low, but sample > >>> read definitely don't work. > >>> > >>> From the info available in datasheets, it looks like MOSI is indeed expected > >>> to be high indefinitely amount of time. Except when the controller is clocking > >>> out data to the peripheral. > >>> > >>> Even if find out the amount of time MOSI would be required high prior to CS low, > >>> then we would need some sort of MOSI high/low state set with a delay prior to > >>> active CS. That might be enough to support the AD4000 series of devices but, > >>> would it be worth the added complexity? > >>> > >> > >> It needs to happen at the same time as setting CPOL for the SCLK line for the > >> device that is about to have the CS asserted. So I don't think we are breaking > >> new ground here. Typically, in most datasheets I've seen they tend to say > >> something like 2 ns before the CS change. So in most cases, I don't think > > which datasheets? Are any of those for devices supported by the ad4000 driver? > > In the AD4000 datasheet, Figure 59, it shows the time needed for SDI setup > before CS assertion, labeled as t_SSDICNV. Table 2 gives this value to be > 2 ns. That delay is for "4-wire" mode and it specifies the delay before bringing CS high. In "3-wire" mode, we are bringing CS low to start transfers so it doesn't look like t_SSDICNV applies to the "3-wire" mode setup. > > So unless a SPI controller has a functional clock of > 500 MHz or somehow > sets the SDI state and the CS assertion in the same register write this > minimum delay will always be met. I highly suspect noting like this has > happened before so no one ever needed to worry about the timing and it > just works (for the similar case of CPOL). > > > > >> anyone bothers adding a delay. But if a longer delay was really needed for > >> a specific peripheral, we could add a SPI xfer with no read/write that has > >> cs_off=1 and a delay to get the correct state of both MOSI and SCLK a longer > >> time before the CS change. > > > > I don't know if that would actually work. I have not tested doing something like that. > > This also implies the controller will be able to start the next transfer right > > after the first preparatory transfer ends and it will meet that inter-transfer > > timing requirement (which I still didn't find documented anywhere). > > I'm not convinced that would be the best way to support those devices. > > I did something like this in the ad7944 driver where we needed an up to > 500ns delay before asserting CS. On SPI controllers without a hardware > sleep or FIFO, the delay will of course be much longer. But the delay > is just a minimum delay, so longer doesn't hurt. It just affects the > max sample rate that can be reliably achieved. > In ad7944_3wire_cs_mode_init_msg(), xfers[1] is prepared with spi_transfer.delay which is the "delay to be introduced after this transfer before (optionally) changing the chipselect status, then starting the next transfer or completing this @spi_message." That should work for ad7944 because it has ADC SDI physically connected to VIO in that setup. For ad4000, we would want to set MOSI high (by writing 1s) such that MOSI is high when CS is brought high (if I'm getting what you are suggesting). But spi_transfer.delay is introduced after the transfer and before changing CS so I think MOSI may return to low if the controller idles MOSI low. I can't see how this would work for ad4000. Other delays we have for spi_transfer (cs_change_delay, word_delay) don't seem to help for this particular case either.
On 6/20/24 1:21 PM, Marcelo Schmitt wrote: > On 06/20, David Lechner wrote: >> On 6/20/24 10:12 AM, Marcelo Schmitt wrote: >>> On 06/19, David Lechner wrote: >>>> On 6/19/24 1:58 PM, Marcelo Schmitt wrote: >>>>> On 06/19, David Lechner wrote: >>>>>> On 6/18/24 6:10 PM, Marcelo Schmitt wrote: >>>>>> >>>>>> >>>> >>>> ... >>>> >>> I don't know if that would actually work. I have not tested doing something like that. >>> This also implies the controller will be able to start the next transfer right >>> after the first preparatory transfer ends and it will meet that inter-transfer >>> timing requirement (which I still didn't find documented anywhere). >>> I'm not convinced that would be the best way to support those devices. >> >> I did something like this in the ad7944 driver where we needed an up to >> 500ns delay before asserting CS. On SPI controllers without a hardware >> sleep or FIFO, the delay will of course be much longer. But the delay >> is just a minimum delay, so longer doesn't hurt. It just affects the >> max sample rate that can be reliably achieved. >> > In ad7944_3wire_cs_mode_init_msg(), xfers[1] is prepared with spi_transfer.delay > which is the "delay to be introduced after this transfer before > (optionally) changing the chipselect status, then starting the next transfer or > completing this @spi_message." That should work for ad7944 because > it has ADC SDI physically connected to VIO in that setup. > For ad4000, we would want to set MOSI high (by writing 1s) such that MOSI > is high when CS is brought high (if I'm getting what you are suggesting). > But spi_transfer.delay is introduced after the transfer and before changing CS > so I think MOSI may return to low if the controller idles MOSI low. > I can't see how this would work for ad4000. > Other delays we have for spi_transfer (cs_change_delay, word_delay) don't seem > to help for this particular case either. I was actually referring to ad7944_4wire_mode_init_msg(). In the case of ad4000, the SPI controller will be required to support the new SPI_MOSI_IDLE_HIGH flag. So at the beginning of the message, before any of the individual xfers, the controller driver will configure SCLK base on CPOL and MOSI based on SPI_MOSI_IDLE_HIGH. Then it will do whatever the xfers say. For most SPI controllers in Linux, this SCLK/MOSI config will happen in ctlr->prepare_message() which happens before xfers are processed in ctlr->transfer_one_message(). In the AXI SPI Engine, the SCLK/MOSI config is in a separate instruction that happens before anything else in the message. So if the first xfer is just a delay with no read/write, the delay will always happen after SCLK and MOSI are configured. We don't have to write 1s because SPI_MOSI_IDLE_HIGH already does the right thing.
diff --git a/Documentation/spi/spi-summary.rst b/Documentation/spi/spi-summary.rst index 7f8accfae6f9..49346708b522 100644 --- a/Documentation/spi/spi-summary.rst +++ b/Documentation/spi/spi-summary.rst @@ -614,6 +614,89 @@ queue, and then start some asynchronous transfer engine (unless it's already running). +Extensions to the SPI protocol +------------------------------ +The fact that SPI doesn't have a formal specification or standard permits chip +manufacturers to implement the SPI protocol in slightly different ways. In most +cases, SPI protocol implementations from different vendors are compatible among +each other. For example, in SPI mode 0 (CPOL=0, CPHA=0) the bus lines may behave +like the following: + +:: + + nCSx ___ ___ + \_________________________________________________________________/ + • • + • • + SCLK ___ ___ ___ ___ ___ ___ ___ ___ + _______/ \___/ \___/ \___/ \___/ \___/ \___/ \___/ \_____ + • : ; : ; : ; : ; : ; : ; : ; : ; • + • : ; : ; : ; : ; : ; : ; : ; : ; • + MOSI XXX__________ _______ _______ ________XXX + 0xA5 XXX__/ 1 \_0_____/ 1 \_0_______0_____/ 1 \_0_____/ 1 \_XXX + • ; ; ; ; ; ; ; ; • + • ; ; ; ; ; ; ; ; • + MISO XXX__________ _______________________ _______ XXX + 0xBA XXX__/ 1 \_____0_/ 1 1 1 \_____0__/ 1 \____0__XXX + +Legend:: + + • marks the start/end of transmission; + : marks when data is clocked into the peripheral; + ; marks when data is clocked into the controller; + X marks when line states are not specified. + +In some few cases, chips extend the SPI protocol by specifying line behaviors +that other SPI protocols don't (e.g. data line state for when CS is inactive). +Those distinct SPI protocols, modes, and configurations are supported by +different SPI mode flags. + +MOSI idle state configuration +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Common SPI protocol implementations don't specify any state or behavior for the +MOSI line when the controller is not clocking out data. However, there do exist +peripherals that require specific MOSI line state when data is not being clocked +out. For example, if the peripheral expects the MOSI line to be high when the +controller is not clocking out data (SPI_MOSI_IDLE_HIGH), then a transfer in SPI +mode 0 would look like the following: + +:: + + nCSx ___ ___ + \_________________________________________________________________/ + • • + • • + SCLK ___ ___ ___ ___ ___ ___ ___ ___ + _______/ \___/ \___/ \___/ \___/ \___/ \___/ \___/ \_____ + • : ; : ; : ; : ; : ; : ; : ; : ; • + • : ; : ; : ; : ; : ; : ; : ; : ; • + MOSI _____ _______ _______ _______________ ___ + 0x56 \_0_____/ 1 \_0_____/ 1 \_0_____/ 1 1 \_0_____/ + • ; ; ; ; ; ; ; ; • + • ; ; ; ; ; ; ; ; • + MISO XXX__________ _______________________ _______ XXX + 0xBA XXX__/ 1 \_____0_/ 1 1 1 \_____0__/ 1 \____0__XXX + +Legend:: + + • marks the start/end of transmission; + : marks when data is clocked into the peripheral; + ; marks when data is clocked into the controller; + X marks when line states are not specified. + +In this extension to the usual SPI protocol, the MOSI line state is specified to +be kept high when CS is active but the controller is not clocking out data to +the peripheral and also when CS is inactive. + +Peripherals that require this extension must request it by setting the +SPI_MOSI_IDLE_HIGH bit into the mode attribute of their struct spi_device and +call spi_setup(). Controllers that support this extension should indicate it by +setting SPI_MOSI_IDLE_HIGH in the mode_bits attribute of their struct +spi_controller. The configuration to idle MOSI low is analogous but uses the +SPI_MOSI_IDLE_LOW mode bit. + + THANKS TO --------- Contributors to Linux-SPI discussions include (in alphabetical order, diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 289feccca376..8e567d5b1945 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -3921,6 +3921,12 @@ int spi_setup(struct spi_device *spi) (SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL | SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL))) return -EINVAL; + /* Check against conflicting MOSI idle configuration */ + if ((spi->mode & SPI_MOSI_IDLE_LOW) && (spi->mode & SPI_MOSI_IDLE_HIGH)) { + dev_err(&spi->dev, + "setup: MOSI configured to simultaneously idle low and high.\n"); + return -EINVAL; + } /* * Help drivers fail *cleanly* when they need options * that aren't supported with their current controller. @@ -3928,7 +3934,8 @@ int spi_setup(struct spi_device *spi) * so it is ignored here. */ bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD | - SPI_NO_TX | SPI_NO_RX); + SPI_NO_TX | SPI_NO_RX | SPI_MOSI_IDLE_LOW | + SPI_MOSI_IDLE_HIGH); ugly_bits = bad_bits & (SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL | SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL); diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index e8e1e798924f..8e50a8559225 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -599,6 +599,12 @@ struct spi_controller { * assert/de-assert more than one chip select at once. */ #define SPI_CONTROLLER_MULTI_CS BIT(7) + /* + * The spi-controller is capable of keeping the MOSI line low or high + * when not clocking out data. + */ +#define SPI_CONTROLLER_MOSI_IDLE_LOW BIT(8) /* Can idle MOSI low */ +#define SPI_CONTROLLER_MOSI_IDLE_HIGH BIT(9) /* Can idle MOSI high */ /* Flag indicating if the allocation of this struct is devres-managed */ bool devm_allocated; diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h index ca56e477d161..ee4ac812b8f8 100644 --- a/include/uapi/linux/spi/spi.h +++ b/include/uapi/linux/spi/spi.h @@ -28,7 +28,8 @@ #define SPI_RX_OCTAL _BITUL(14) /* receive with 8 wires */ #define SPI_3WIRE_HIZ _BITUL(15) /* high impedance turnaround */ #define SPI_RX_CPHA_FLIP _BITUL(16) /* flip CPHA on Rx only xfer */ -#define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave mosi line low when idle */ +#define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave MOSI line low when idle */ +#define SPI_MOSI_IDLE_HIGH _BITUL(18) /* leave MOSI line high when idle */ /* * All the bits defined above should be covered by SPI_MODE_USER_MASK. @@ -38,6 +39,6 @@ * These bits must not overlap. A static assert check should make sure of that. * If adding extra bits, make sure to increase the bit index below as well. */ -#define SPI_MODE_USER_MASK (_BITUL(18) - 1) +#define SPI_MODE_USER_MASK (_BITUL(19) - 1) #endif /* _UAPI_SPI_H */
The behavior of an SPI controller data output line (SDO or MOSI or COPI (Controller Output Peripheral Input) for disambiguation) is usually not specified when the controller is not clocking out data on SCLK edges. However, there do exist SPI peripherals that require specific MOSI line state when data is not being clocked out of the controller. A SPI controller may set the MOSI line on SCLK edges then bring it low when no data is going out or leave the line the state of the last transfer bit. More elaborated controllers are capable to set the MOSI idle state according to different configurable levels and thus are more suitable for interfacing with restrictive peripherals. Add SPI mode bits to allow peripherals to request explicit MOSI idle state when needed. When supporting a particular MOSI idle configuration, the data output line state is expected to remain at the configured level when the controller is not clocking out data. When a device that needs a specific MOSI idle state is identified, its driver should request the MOSI idle configuration by setting the proper SPI mode bit. Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> --- Hi, so, this is an improved version of MOSI idle configuration support based on comments to the previous set. I'm actually not sure I did everything requested for the SPI subsystem. First replies to v3 brought the idea of having a feature detection mechanism. I didn't really get how to do that. If feature detection is required, can somebody please provide some pointers on how to implement that? Documentation/spi/spi-summary.rst | 83 +++++++++++++++++++++++++++++++ drivers/spi/spi.c | 9 +++- include/linux/spi/spi.h | 6 +++ include/uapi/linux/spi/spi.h | 5 +- 4 files changed, 100 insertions(+), 3 deletions(-)