| Message ID | a6b00e84325bbe44919cc49509e837f2555367d0.1717539384.git.marcelo.schmitt@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for AD4000 series of ADCs | expand |
On Tue, 2024-06-04 at 19:43 -0300, Marcelo Schmitt wrote: > Implement MOSI idle low and MOSI idle high to better support peripherals > that request specific MOSI behavior. > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > --- Reviewed-by: Nuno Sa <nuno.sa@analog.com>
On 6/4/24 5:43 PM, Marcelo Schmitt wrote: > Implement MOSI idle low and MOSI idle high to better support peripherals > that request specific MOSI behavior. > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > --- > drivers/spi/spi-axi-spi-engine.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c > index 0aa31d745734..549f03069d0e 100644 > --- a/drivers/spi/spi-axi-spi-engine.c > +++ b/drivers/spi/spi-axi-spi-engine.c > @@ -41,6 +41,7 @@ > #define SPI_ENGINE_CONFIG_CPHA BIT(0) > #define SPI_ENGINE_CONFIG_CPOL BIT(1) > #define SPI_ENGINE_CONFIG_3WIRE BIT(2) > +#define SPI_ENGINE_CONFIG_SDO_IDLE BIT(3) > > #define SPI_ENGINE_INST_TRANSFER 0x0 > #define SPI_ENGINE_INST_ASSERT 0x1 > @@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct spi_device *spi) > config |= SPI_ENGINE_CONFIG_CPHA; > if (spi->mode & SPI_3WIRE) > config |= SPI_ENGINE_CONFIG_3WIRE; > + if (spi->mode & SPI_MOSI_IDLE_HIGH) > + config |= SPI_ENGINE_CONFIG_SDO_IDLE; > + if (spi->mode & SPI_MOSI_IDLE_LOW) > + config &= ~SPI_ENGINE_CONFIG_SDO_IDLE; > > return config; > } > @@ -645,7 +650,8 @@ static int spi_engine_probe(struct platform_device *pdev) > return ret; > > host->dev.of_node = pdev->dev.of_node; > - host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE; > + host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE | SPI_MOSI_IDLE_LOW > + | SPI_MOSI_IDLE_HIGH; > host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32); > host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2; > host->transfer_one_message = spi_engine_transfer_one_message; I think we need a version check instead of setting the flags unconditionally here since older versions of the AXI SPI Engine won't support this feature.
On Wed, 2024-06-05 at 12:03 -0500, David Lechner wrote: > On 6/4/24 5:43 PM, Marcelo Schmitt wrote: > > Implement MOSI idle low and MOSI idle high to better support peripherals > > that request specific MOSI behavior. > > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > > --- > > drivers/spi/spi-axi-spi-engine.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi- > > engine.c > > index 0aa31d745734..549f03069d0e 100644 > > --- a/drivers/spi/spi-axi-spi-engine.c > > +++ b/drivers/spi/spi-axi-spi-engine.c > > @@ -41,6 +41,7 @@ > > #define SPI_ENGINE_CONFIG_CPHA BIT(0) > > #define SPI_ENGINE_CONFIG_CPOL BIT(1) > > #define SPI_ENGINE_CONFIG_3WIRE BIT(2) > > +#define SPI_ENGINE_CONFIG_SDO_IDLE BIT(3) > > > > #define SPI_ENGINE_INST_TRANSFER 0x0 > > #define SPI_ENGINE_INST_ASSERT 0x1 > > @@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct > > spi_device *spi) > > config |= SPI_ENGINE_CONFIG_CPHA; > > if (spi->mode & SPI_3WIRE) > > config |= SPI_ENGINE_CONFIG_3WIRE; > > + if (spi->mode & SPI_MOSI_IDLE_HIGH) > > + config |= SPI_ENGINE_CONFIG_SDO_IDLE; > > + if (spi->mode & SPI_MOSI_IDLE_LOW) > > + config &= ~SPI_ENGINE_CONFIG_SDO_IDLE; > > > > return config; > > } > > @@ -645,7 +650,8 @@ static int spi_engine_probe(struct platform_device > > *pdev) > > return ret; > > > > host->dev.of_node = pdev->dev.of_node; > > - host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE; > > + host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE | > > SPI_MOSI_IDLE_LOW > > + | SPI_MOSI_IDLE_HIGH; > > host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32); > > host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2; > > host->transfer_one_message = spi_engine_transfer_one_message; > > I think we need a version check instead of setting the flags unconditionally > here since older versions of the AXI SPI Engine won't support this feature. Oh, was not aware of that... Then, we definitely need to do that. Marcelo, only add my r-b tag with the version change in place. - Nuno Sá
On 6/6/24 1:51 AM, Nuno Sá wrote: > On Wed, 2024-06-05 at 12:03 -0500, David Lechner wrote: >> On 6/4/24 5:43 PM, Marcelo Schmitt wrote: >>> Implement MOSI idle low and MOSI idle high to better support peripherals >>> that request specific MOSI behavior. >>> >>> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> >>> --- >>> drivers/spi/spi-axi-spi-engine.c | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi- >>> engine.c >>> index 0aa31d745734..549f03069d0e 100644 >>> --- a/drivers/spi/spi-axi-spi-engine.c >>> +++ b/drivers/spi/spi-axi-spi-engine.c >>> @@ -41,6 +41,7 @@ >>> #define SPI_ENGINE_CONFIG_CPHA BIT(0) >>> #define SPI_ENGINE_CONFIG_CPOL BIT(1) >>> #define SPI_ENGINE_CONFIG_3WIRE BIT(2) >>> +#define SPI_ENGINE_CONFIG_SDO_IDLE BIT(3) >>> >>> #define SPI_ENGINE_INST_TRANSFER 0x0 >>> #define SPI_ENGINE_INST_ASSERT 0x1 >>> @@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct >>> spi_device *spi) >>> config |= SPI_ENGINE_CONFIG_CPHA; >>> if (spi->mode & SPI_3WIRE) >>> config |= SPI_ENGINE_CONFIG_3WIRE; >>> + if (spi->mode & SPI_MOSI_IDLE_HIGH) >>> + config |= SPI_ENGINE_CONFIG_SDO_IDLE; >>> + if (spi->mode & SPI_MOSI_IDLE_LOW) >>> + config &= ~SPI_ENGINE_CONFIG_SDO_IDLE; >>> >>> return config; >>> } >>> @@ -645,7 +650,8 @@ static int spi_engine_probe(struct platform_device >>> *pdev) >>> return ret; >>> >>> host->dev.of_node = pdev->dev.of_node; >>> - host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE; >>> + host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE | >>> SPI_MOSI_IDLE_LOW >>> + | SPI_MOSI_IDLE_HIGH; >>> host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32); >>> host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2; >>> host->transfer_one_message = spi_engine_transfer_one_message; >> >> I think we need a version check instead of setting the flags unconditionally >> here since older versions of the AXI SPI Engine won't support this feature. > > Oh, was not aware of that... Then, we definitely need to do that. Marcelo, only > add my r-b tag with the version change in place. > > - Nuno Sá Actually, looking at [1], it looks like this could be a compile-time flag when the HDL is built. If it stays that way, then we would need a way to read that flag from a register instead of using the version. [1]: https://github.com/analogdevicesinc/hdl/pull/1320#issuecomment-2145744521
On 06/06, David Lechner wrote: > On 6/6/24 1:51 AM, Nuno Sá wrote: > > On Wed, 2024-06-05 at 12:03 -0500, David Lechner wrote: > >> On 6/4/24 5:43 PM, Marcelo Schmitt wrote: > >>> Implement MOSI idle low and MOSI idle high to better support peripherals > >>> that request specific MOSI behavior. > >>> > >>> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > >>> --- > >>> drivers/spi/spi-axi-spi-engine.c | 8 +++++++- > >>> 1 file changed, 7 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi- > >>> engine.c > >>> index 0aa31d745734..549f03069d0e 100644 > >>> --- a/drivers/spi/spi-axi-spi-engine.c > >>> +++ b/drivers/spi/spi-axi-spi-engine.c > >>> @@ -41,6 +41,7 @@ > >>> #define SPI_ENGINE_CONFIG_CPHA BIT(0) > >>> #define SPI_ENGINE_CONFIG_CPOL BIT(1) > >>> #define SPI_ENGINE_CONFIG_3WIRE BIT(2) > >>> +#define SPI_ENGINE_CONFIG_SDO_IDLE BIT(3) > >>> > >>> #define SPI_ENGINE_INST_TRANSFER 0x0 > >>> #define SPI_ENGINE_INST_ASSERT 0x1 > >>> @@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct > >>> spi_device *spi) > >>> config |= SPI_ENGINE_CONFIG_CPHA; > >>> if (spi->mode & SPI_3WIRE) > >>> config |= SPI_ENGINE_CONFIG_3WIRE; > >>> + if (spi->mode & SPI_MOSI_IDLE_HIGH) > >>> + config |= SPI_ENGINE_CONFIG_SDO_IDLE; > >>> + if (spi->mode & SPI_MOSI_IDLE_LOW) > >>> + config &= ~SPI_ENGINE_CONFIG_SDO_IDLE; > >>> > >>> return config; > >>> } > >>> @@ -645,7 +650,8 @@ static int spi_engine_probe(struct platform_device > >>> *pdev) > >>> return ret; > >>> > >>> host->dev.of_node = pdev->dev.of_node; > >>> - host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE; > >>> + host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE | > >>> SPI_MOSI_IDLE_LOW > >>> + | SPI_MOSI_IDLE_HIGH; > >>> host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32); > >>> host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2; > >>> host->transfer_one_message = spi_engine_transfer_one_message; > >> > >> I think we need a version check instead of setting the flags unconditionally > >> here since older versions of the AXI SPI Engine won't support this feature. > > > > Oh, was not aware of that... Then, we definitely need to do that. Marcelo, only > > add my r-b tag with the version change in place. > > > > - Nuno Sá Nuno, I think there will be more disscussion about this series. Maybe better I not add the tag at all so you may check to agree with the next patch version. > > Actually, looking at [1], it looks like this could be a compile-time > flag when the HDL is built. If it stays that way, then we would need > a way to read that flag from a register instead of using the version. > > > [1]: https://github.com/analogdevicesinc/hdl/pull/1320#issuecomment-2145744521 When is a driver version check needed? Yes, older versions of SPI-Engine won't support this, but the patch set should cause no regression. Even if loading the current ad4000 driver with older SPI-Engine HDL and driver, the ADC driver would get a warn (or error?) and do what's possible without MOSI idle feature (probably only be able to do reg access) or fail probing. We decided to have the MOSI idle state feature for SPI-Engine configured by writing to a dedicated bit [1] in the SPI Configuration Register [2]. Does this looks good? [1]: https://github.com/analogdevicesinc/hdl/pull/1320/commits/941937eedae6701d253b4930d8f279c21ef3f807#diff-dc9213744b55493ca9430cd02cd62212436c2379ca121d1a2681356e6a37e22dR257 [2]: https://analogdevicesinc.github.io/hdl/library/spi_engine/instruction-format.html#spi-configuration-register Thanks, Marcelo
On Thu, 2024-06-06 at 18:31 -0300, Marcelo Schmitt wrote: ... > > > > When is a driver version check needed? > Yes, older versions of SPI-Engine won't support this, but the patch set should > cause no regression. Even if loading the current ad4000 driver with > older SPI-Engine HDL and driver, the ADC driver would get a warn (or error?) > and do what's possible without MOSI idle feature (probably only be able to do > reg access) or fail probing. > Maybe I'm missing something but with the patchset we unconditionally set SPI_MOSI_IDLE_HIGH. So if we load an hdl which does not support it things will apparently be ok but it won't actually work, right? If I'm right we should have a bit in a RO config_register telling us that the feature is being supported or not. That way we only set the mode bit if we do support it... - Nuno Sá
On 06/07, Nuno Sá wrote: > On Thu, 2024-06-06 at 18:31 -0300, Marcelo Schmitt wrote: > > ... > > > > > > > > > When is a driver version check needed? > > Yes, older versions of SPI-Engine won't support this, but the patch set should > > cause no regression. Even if loading the current ad4000 driver with > > older SPI-Engine HDL and driver, the ADC driver would get a warn (or error?) > > and do what's possible without MOSI idle feature (probably only be able to do > > reg access) or fail probing. > > > > Maybe I'm missing something but with the patchset we unconditionally set > SPI_MOSI_IDLE_HIGH. So if we load an hdl which does not support it things will > apparently be ok but it won't actually work, right? If I'm right we should have Yes, that's correct. > a bit in a RO config_register telling us that the feature is being supported or > not. That way we only set the mode bit if we do support it... Ok, understood. Will do it for v4. Thanks, Marcelo > > - Nuno Sá > >
On Fri, 7 Jun 2024 11:40:23 -0300 Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > On 06/07, Nuno Sá wrote: > > On Thu, 2024-06-06 at 18:31 -0300, Marcelo Schmitt wrote: > > > > ... > > > > > > > > > > > > > > When is a driver version check needed? > > > Yes, older versions of SPI-Engine won't support this, but the patch set should > > > cause no regression. Even if loading the current ad4000 driver with > > > older SPI-Engine HDL and driver, the ADC driver would get a warn (or error?) > > > and do what's possible without MOSI idle feature (probably only be able to do > > > reg access) or fail probing. > > > > > > > Maybe I'm missing something but with the patchset we unconditionally set > > SPI_MOSI_IDLE_HIGH. So if we load an hdl which does not support it things will > > apparently be ok but it won't actually work, right? If I'm right we should have > Yes, that's correct. > > > a bit in a RO config_register telling us that the feature is being supported or > > not. That way we only set the mode bit if we do support it... > > Ok, understood. Will do it for v4. If you don't have such a mode bit, you will need to add a property to the dt-binding. Or a suitable compatible. Nasty, so fingers crossed you do have a capability flag to check! Jonathan > > Thanks, > Marcelo > > > > > - Nuno Sá > > > >
diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c index 0aa31d745734..549f03069d0e 100644 --- a/drivers/spi/spi-axi-spi-engine.c +++ b/drivers/spi/spi-axi-spi-engine.c @@ -41,6 +41,7 @@ #define SPI_ENGINE_CONFIG_CPHA BIT(0) #define SPI_ENGINE_CONFIG_CPOL BIT(1) #define SPI_ENGINE_CONFIG_3WIRE BIT(2) +#define SPI_ENGINE_CONFIG_SDO_IDLE BIT(3) #define SPI_ENGINE_INST_TRANSFER 0x0 #define SPI_ENGINE_INST_ASSERT 0x1 @@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct spi_device *spi) config |= SPI_ENGINE_CONFIG_CPHA; if (spi->mode & SPI_3WIRE) config |= SPI_ENGINE_CONFIG_3WIRE; + if (spi->mode & SPI_MOSI_IDLE_HIGH) + config |= SPI_ENGINE_CONFIG_SDO_IDLE; + if (spi->mode & SPI_MOSI_IDLE_LOW) + config &= ~SPI_ENGINE_CONFIG_SDO_IDLE; return config; } @@ -645,7 +650,8 @@ static int spi_engine_probe(struct platform_device *pdev) return ret; host->dev.of_node = pdev->dev.of_node; - host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE; + host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE | SPI_MOSI_IDLE_LOW + | SPI_MOSI_IDLE_HIGH; host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32); host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2; host->transfer_one_message = spi_engine_transfer_one_message;
Implement MOSI idle low and MOSI idle high to better support peripherals that request specific MOSI behavior. Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> --- drivers/spi/spi-axi-spi-engine.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)