| Message ID | e1d5d57f7a7481c84f64a764f9898122e278739b.1717539384.git.marcelo.schmitt@analog.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/6] spi: Add SPI mode bit for MOSI idle state configuration | expand |
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.
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 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 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(-)