| Message ID | e1d5d57f7a7481c84f64a764f9898122e278739b.1717539384.git.marcelo.schmitt@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for AD4000 series of ADCs | expand |
On Tue, 2024-06-04 at 19:41 -0300, Marcelo Schmitt wrote: > The behavior of an SPI controller data output line (SDO or MOSI or COPI > (Controller Output Peripheral Input) for disambiguation) is not specified > when the controller is not clocking out data on SCLK edges. However, there > exist SPI peripherals that require specific COPI line state when data is > not being clocked out of the controller. > Add SPI mode bit to allow pheripherals to request explicit COPI idle > behavior when needed. > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > --- > drivers/spi/spi.c | 6 ++++++ > include/uapi/linux/spi/spi.h | 3 ++- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 289feccca376..6072b6e93bef 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_warn(&spi->dev, > + "setup: erratic MOSI idle configuration. Set to idle > low\n"); > + spi->mode &= ~SPI_MOSI_IDLE_HIGH; > + } Should we assume such a thing? IOW, should this be treated as a warning or a real error? I would assume this should be a configuration error and return - EINVAL but... - Nuno Sá
On Wed, Jun 05, 2024 at 11:14:33AM +0200, Nuno Sá wrote: > On Tue, 2024-06-04 at 19:41 -0300, Marcelo Schmitt wrote: > > + /* Check against conflicting MOSI idle configuration */ > > + if ((spi->mode & SPI_MOSI_IDLE_LOW) && (spi->mode & > > SPI_MOSI_IDLE_HIGH)) { > > + dev_warn(&spi->dev, > > + "setup: erratic MOSI idle configuration. Set to idle > > low\n"); > > + spi->mode &= ~SPI_MOSI_IDLE_HIGH; > > + } > Should we assume such a thing? IOW, should this be treated as a warning or a > real error? I would assume this should be a configuration error and return - > EINVAL but... Right, and the error message isn't very clear.
On Tue, Jun 04, 2024 at 07:41:47PM -0300, Marcelo Schmitt wrote: > The behavior of an SPI controller data output line (SDO or MOSI or COPI > (Controller Output Peripheral Input) for disambiguation) is not specified > when the controller is not clocking out data on SCLK edges. However, there > exist SPI peripherals that require specific COPI line state when data is > not being clocked out of the controller. This is an optimisation for accelerating devices that need a specific value, really if these devices need a value they should send it. > #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 */ Realistically we'll have a large set of drivers that are expecting the line to be held low so I'm not sure we need that option. I would also expect to have an implementation of these options in the core which supplies buffers with the relevant data for use with controllers that don't have the feature (similar to how _MUST_TX and _MUST_RX are done). Even without that we'd need feature detection so that drivers that try to use this aren't just buggy when used with a controller that doesn't implement it, but once you're detecting you may as well just make things work.
On 6/5/24 7:24 AM, Mark Brown wrote: > On Tue, Jun 04, 2024 at 07:41:47PM -0300, Marcelo Schmitt wrote: > >> The behavior of an SPI controller data output line (SDO or MOSI or COPI >> (Controller Output Peripheral Input) for disambiguation) is not specified >> when the controller is not clocking out data on SCLK edges. However, there >> exist SPI peripherals that require specific COPI line state when data is >> not being clocked out of the controller. I think this description is missing a key detail that the tx data line needs to be high just before and also during the CS assertion at the start of each message. And it would be helpful to have this more detailed description in the source code somewhere and not just in the commit message. > > This is an optimisation for accelerating devices that need a specific > value, really if these devices need a value they should send it. > >> #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 */ > > Realistically we'll have a large set of drivers that are expecting the > line to be held low so I'm not sure we need that option. I would also > expect to have an implementation of these options in the core which > supplies buffers with the relevant data for use with controllers that > don't have the feature (similar to how _MUST_TX and _MUST_RX are done). > Even without that we'd need feature detection so that drivers that try > to use this aren't just buggy when used with a controller that doesn't > implement it, but once you're detecting you may as well just make things > work. I could see something like this working for controllers that leave the tx data line in the state of the last bit of a write transfer. I.e. do a write transfer of 0xff (using the smallest number of bits per word supported by the controller) with CS not asserted, then assert CS, then do the rest of actual the transfers requested by the peripheral. But it doesn't seem like it would work for controllers that always return the tx data line to a low state after a write since this would mean that the data line would still be low during the CS assertion which is what we need to prevent.
On Wed, Jun 05, 2024 at 11:37:48AM -0500, David Lechner wrote: > On 6/5/24 7:24 AM, Mark Brown wrote: > > On Tue, Jun 04, 2024 at 07:41:47PM -0300, Marcelo Schmitt wrote: > >> The behavior of an SPI controller data output line (SDO or MOSI or COPI > >> (Controller Output Peripheral Input) for disambiguation) is not specified > >> when the controller is not clocking out data on SCLK edges. However, there > >> exist SPI peripherals that require specific COPI line state when data is > >> not being clocked out of the controller. > I think this description is missing a key detail that the tx data line > needs to be high just before and also during the CS assertion at the start > of each message. > And it would be helpful to have this more detailed description in the > source code somewhere and not just in the commit message. Yes, there's no way anyone could infer this from any aspect of the series. I think the properties also need a clearer name since someone might want the accelerator functionality at some point. > > Even without that we'd need feature detection so that drivers that try > > to use this aren't just buggy when used with a controller that doesn't > > implement it, but once you're detecting you may as well just make things > > work. > I could see something like this working for controllers that leave the > tx data line in the state of the last bit of a write transfer. I.e. do a > write transfer of 0xff (using the smallest number of bits per word > supported by the controller) with CS not asserted, then assert CS, then > do the rest of actual the transfers requested by the peripheral. > But it doesn't seem like it would work for controllers that always > return the tx data line to a low state after a write since this would > mean that the data line would still be low during the CS assertion > which is what we need to prevent. With the additional requirement it's not emulatable, but we'd still need the checks in the core.
Hi, On 06/05, Mark Brown wrote: > On Tue, Jun 04, 2024 at 07:41:47PM -0300, Marcelo Schmitt wrote: > > > The behavior of an SPI controller data output line (SDO or MOSI or COPI > > (Controller Output Peripheral Input) for disambiguation) is not specified > > when the controller is not clocking out data on SCLK edges. However, there > > exist SPI peripherals that require specific COPI line state when data is > > not being clocked out of the controller. > > This is an optimisation for accelerating devices that need a specific > value, really if these devices need a value they should send it. I see it more like an extension of SPI controller functionality. Though I guess it might also be used for optimization if tx is known to be always 0s or always 1s for a device. > > > #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 */ > > Realistically we'll have a large set of drivers that are expecting the > line to be held low so I'm not sure we need that option. I would also Yes, I also think most SPI devices, if ever requiring anything, would expect the MOSI line to be low. But this patchset is about the exception to that. :) > expect to have an implementation of these options in the core which > supplies buffers with the relevant data for use with controllers that > don't have the feature (similar to how _MUST_TX and _MUST_RX are done). > Even without that we'd need feature detection so that drivers that try > to use this aren't just buggy when used with a controller that doesn't > implement it, but once you're detecting you may as well just make things > work. As far as I searched, the definitions for SPI protocol usually don't specify any behavior for the MOSI line when the controller is not clocking out data. So, I think SPI controllers that are not capable of implementing any type of MOSI idle configuration are anyway compliant to what is usual SPI. For those that can implement such feature, I thought peripherals could request it by setting SPI mode bits. If the controller can provide MOSI idle state configuration, then the controller sets itself to act according to what peripheral asked. If MOSI idle configuration is not supported, then we just move on and let peripheral driver adapt to what is supported? Guess we can't blame an SPI controller for it not supporting something that is not specified in usual SPI protocols. But yeah, it's not that evident what this patch set is all about and why this is wanted so I made a wiki page to explain the reasoning for this set. https://wiki.analog.com/software/linux/docs/spi/spi_copi_idle?rev=1717699755 Hopefully the figures with timing diagrams and transfer captures there will provide quicker understanding of this rather than I try to explain it with only text. If you still think we need feature detection for MOSI idle capability just let me know, I'll implement what be needed. Thanks, Marcelo
On 06/05, Mark Brown wrote: > On Wed, Jun 05, 2024 at 11:14:33AM +0200, Nuno Sá wrote: > > On Tue, 2024-06-04 at 19:41 -0300, Marcelo Schmitt wrote: > > > > + /* Check against conflicting MOSI idle configuration */ > > > + if ((spi->mode & SPI_MOSI_IDLE_LOW) && (spi->mode & > > > SPI_MOSI_IDLE_HIGH)) { > > > + dev_warn(&spi->dev, > > > + "setup: erratic MOSI idle configuration. Set to idle > > > low\n"); > > > + spi->mode &= ~SPI_MOSI_IDLE_HIGH; > > > + } > > > Should we assume such a thing? IOW, should this be treated as a warning or a > > real error? I would assume this should be a configuration error and return - > > EINVAL but... > > Right, and the error message isn't very clear. Yeah, the message is not all that clear. I'll think of something better. I'm biased towards having this as a warning because I don't see this as a feature of usual SPI protocol but not really sure about either ...
On 06/05, Mark Brown wrote: > On Wed, Jun 05, 2024 at 11:37:48AM -0500, David Lechner wrote: > > On 6/5/24 7:24 AM, Mark Brown wrote: > > > On Tue, Jun 04, 2024 at 07:41:47PM -0300, Marcelo Schmitt wrote: > > > >> The behavior of an SPI controller data output line (SDO or MOSI or COPI > > >> (Controller Output Peripheral Input) for disambiguation) is not specified > > >> when the controller is not clocking out data on SCLK edges. However, there > > >> exist SPI peripherals that require specific COPI line state when data is > > >> not being clocked out of the controller. > > > I think this description is missing a key detail that the tx data line > > needs to be high just before and also during the CS assertion at the start > > of each message. > > > And it would be helpful to have this more detailed description in the > > source code somewhere and not just in the commit message. > > Yes, there's no way anyone could infer this from any aspect of the > series. I think the properties also need a clearer name since someone > might want the accelerator functionality at some point. So, if I understand correctly, it would be desirable to also have flags and descriptions for the MOSI idle configuration capabilities in include/linux/spi/spi.h. Does the following definitions look good? #define SPI_CONTROLLER_MOSI_IDLE_LOW BIT(8) #define SPI_CONTROLLER_MOSI_IDLE_HIGH BIT(9) Maybe also document the MOSI idle configuration feature in spi-summary.rst? > > > > Even without that we'd need feature detection so that drivers that try > > > to use this aren't just buggy when used with a controller that doesn't > > > implement it, but once you're detecting you may as well just make things > > > work. > > > I could see something like this working for controllers that leave the > > tx data line in the state of the last bit of a write transfer. I.e. do a > > write transfer of 0xff (using the smallest number of bits per word > > supported by the controller) with CS not asserted, then assert CS, then > > do the rest of actual the transfers requested by the peripheral. > > > But it doesn't seem like it would work for controllers that always > > return the tx data line to a low state after a write since this would > > mean that the data line would still be low during the CS assertion > > which is what we need to prevent. > > With the additional requirement it's not emulatable, but we'd still need > the checks in the core.
On Thu, Jun 06, 2024 at 04:57:52PM -0300, Marcelo Schmitt wrote: > As far as I searched, the definitions for SPI protocol usually don't specify any > behavior for the MOSI line when the controller is not clocking out data. > So, I think SPI controllers that are not capable of implementing any type > of MOSI idle configuration are anyway compliant to what is usual SPI. > For those that can implement such feature, I thought peripherals could request > it by setting SPI mode bits. The issue here is the one that Richard highlighted with it not being clear exactly what the intended behaviour is. > But yeah, it's not that evident what this patch set is all about and why this is > wanted so I made a wiki page to explain the reasoning for this set. > https://wiki.analog.com/software/linux/docs/spi/spi_copi_idle?rev=1717699755 > Hopefully the figures with timing diagrams and transfer captures there will > provide quicker understanding of this rather than I try to explain it with > only text. It needs to be apparent to someone looking at the kernel what the code is intended to do. > If you still think we need feature detection for MOSI idle capability just let > me know, I'll implement what be needed. If the devices actually require this mode then we can't just randomly ignore them when they request it.
On Thu, Jun 06, 2024 at 07:08:30PM -0300, Marcelo Schmitt wrote: > So, if I understand correctly, it would be desirable to also have flags and > descriptions for the MOSI idle configuration capabilities in include/linux/spi/spi.h. > Does the following definitions look good? > #define SPI_CONTROLLER_MOSI_IDLE_LOW BIT(8) > #define SPI_CONTROLLER_MOSI_IDLE_HIGH BIT(9) Yes. > Maybe also document the MOSI idle configuration feature in spi-summary.rst? Yes.
On Thu, Jun 06, 2024 at 05:10:04PM -0300, Marcelo Schmitt wrote: > On 06/05, Mark Brown wrote: > > > Should we assume such a thing? IOW, should this be treated as a warning or a > > > real error? I would assume this should be a configuration error and return - > > > EINVAL but... > > Right, and the error message isn't very clear. > Yeah, the message is not all that clear. I'll think of something better. > I'm biased towards having this as a warning because I don't see this as a > feature of usual SPI protocol but not really sure about either ... The less usual it is the more likely it is that it won't be supported and we should actually check that to try to avoid data corruption.
On 06/07, Mark Brown wrote: > On Thu, Jun 06, 2024 at 04:57:52PM -0300, Marcelo Schmitt wrote: > > > As far as I searched, the definitions for SPI protocol usually don't specify any > > behavior for the MOSI line when the controller is not clocking out data. > > So, I think SPI controllers that are not capable of implementing any type > > of MOSI idle configuration are anyway compliant to what is usual SPI. > > For those that can implement such feature, I thought peripherals could request > > it by setting SPI mode bits. > > The issue here is the one that Richard highlighted with it not being > clear exactly what the intended behaviour is. > > > But yeah, it's not that evident what this patch set is all about and why this is > > wanted so I made a wiki page to explain the reasoning for this set. > > https://wiki.analog.com/software/linux/docs/spi/spi_copi_idle?rev=1717699755 > > Hopefully the figures with timing diagrams and transfer captures there will > > provide quicker understanding of this rather than I try to explain it with > > only text. > > It needs to be apparent to someone looking at the kernel what the code > is intended to do. Ack > > > If you still think we need feature detection for MOSI idle capability just let > > me know, I'll implement what be needed. > > If the devices actually require this mode then we can't just randomly > ignore them when they request it. Ok. Yes, when connected in that datasheet "3-wire" mode the MOSI idle high feature is pretty much required otherwise users won't be able to sample the ADC. Will document the behavior for the MOSI idle feature and make spi_setup() fail with better message if the controller can't support a device requesting it. Thanks, Marcelo
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 289feccca376..6072b6e93bef 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_warn(&spi->dev, + "setup: erratic MOSI idle configuration. Set to idle low\n"); + spi->mode &= ~SPI_MOSI_IDLE_HIGH; + } /* * Help drivers fail *cleanly* when they need options * that aren't supported with their current controller. diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h index ca56e477d161..ba9adba25927 100644 --- a/include/uapi/linux/spi/spi.h +++ b/include/uapi/linux/spi/spi.h @@ -29,6 +29,7 @@ #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_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 not specified when the controller is not clocking out data on SCLK edges. However, there exist SPI peripherals that require specific COPI line state when data is not being clocked out of the controller. Add SPI mode bit to allow pheripherals to request explicit COPI idle behavior when needed. Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> --- drivers/spi/spi.c | 6 ++++++ include/uapi/linux/spi/spi.h | 3 ++- 2 files changed, 8 insertions(+), 1 deletion(-)