2022-12-02 10:06:15

by Edmund Berenson

Subject: [PATCH 3/3] spi: dw: Write chip configuration before cs is set

Using chips with different cpol, causes first communication
to fail on cpol change.
To avoid this issue write cr0 register before cs is set.

Suggested-by: Lukasz Zemla <[email protected]>
Signed-off-by: Edmund Berenson <[email protected]>
---
drivers/spi/spi-dw-core.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 57c9e384d6d4..c3da4fe3e510 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -93,6 +93,7 @@ static inline void dw_spi_debugfs_remove(struct dw_spi *dws)
void dw_spi_set_cs(struct spi_device *spi, bool enable)
{
struct dw_spi *dws = spi_controller_get_devdata(spi->controller);
+ struct dw_spi_chip_data *chip = spi_get_ctldata(spi);
bool cs_high = !!(spi->mode & SPI_CS_HIGH);
u8 enable_cs = 0;

@@ -106,8 +107,13 @@ void dw_spi_set_cs(struct spi_device *spi, bool enable)
* Enable register no matter whether the SPI core is configured to
* support active-high or active-low CS level.
*/
- if (cs_high == enable)
+ if (cs_high == enable) {
+ dw_spi_enable_chip(dws, 0);
+ dw_writel(dws, DW_SPI_CTRLR0, chip->cr0);
+ dw_spi_enable_chip(dws, 1);
+
dw_writel(dws, DW_SPI_SER, BIT(enable_cs));
+ }
else
dw_writel(dws, DW_SPI_SER, 0);
}
--
2.37.4

2022-12-09 13:27:44

by Serge Semin

Subject: Re: [PATCH 3/3] spi: dw: Write chip configuration before cs is set

On Fri, Dec 02, 2022 at 10:49:34AM +0100, Edmund Berenson wrote:
> Using chips with different cpol, causes first communication
> to fail on cpol change.
> To avoid this issue write cr0 register before cs is set.

I need more details on the problem otherwise this patch seems more
like a hack than a fix. The dw_spi_update_config() method already
updates the mode before start to execute each SPI-transfer. Basically
what you suggest here is the double CR0 register update before the
first transfer submission. This doesn't seem like a proper solution.
Moreover updating the device setting in the set_cs callback makes it
less coherent. So there must be a firm justification why the update is
supposed to be done in the dw_spi_set_cs method.

>
> Suggested-by: Lukasz Zemla <[email protected]>
> Signed-off-by: Edmund Berenson <[email protected]>
> ---
> drivers/spi/spi-dw-core.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 57c9e384d6d4..c3da4fe3e510 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -93,6 +93,7 @@ static inline void dw_spi_debugfs_remove(struct dw_spi *dws)
> void dw_spi_set_cs(struct spi_device *spi, bool enable)
> {
> struct dw_spi *dws = spi_controller_get_devdata(spi->controller);
> + struct dw_spi_chip_data *chip = spi_get_ctldata(spi);
> bool cs_high = !!(spi->mode & SPI_CS_HIGH);
> u8 enable_cs = 0;
>
> @@ -106,8 +107,13 @@ void dw_spi_set_cs(struct spi_device *spi, bool enable)
> * Enable register no matter whether the SPI core is configured to
> * support active-high or active-low CS level.
> */
> - if (cs_high == enable)
> + if (cs_high == enable) {
> + dw_spi_enable_chip(dws, 0);
> + dw_writel(dws, DW_SPI_CTRLR0, chip->cr0);
> + dw_spi_enable_chip(dws, 1);
> +
> dw_writel(dws, DW_SPI_SER, BIT(enable_cs));

> + }
> else

Documentation/process/coding-style.rst: 3) Placing Braces and Spaces

-Sergey

> dw_writel(dws, DW_SPI_SER, 0);
> }
> --
> 2.37.4
>