Re: [PATCH v3 2/5] spi: dw: Add SSTE support for DWC SSI controller

[Serge Semin]

On Thu, Nov 11, 2021 at 02:51:58PM +0800, nandhini.srikandan@xxxxxxxxx wrote:
> From: Nandhini Srikandan <nandhini.srikandan@xxxxxxxxx>
> 
> Add support for Slave Select Toggle Enable (SSTE) in DWC SSI controller
> via DTS. The slave select line will toggle between consecutive data frames,
> with the serial clock being held to its default value while slave
> select line is high.
> 
> Signed-off-by: Nandhini Srikandan <nandhini.srikandan@xxxxxxxxx>
> ---
>  drivers/spi/spi-dw-core.c | 11 +++++++++++
>  drivers/spi/spi-dw.h      |  2 ++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index a305074c482e..bfa075a4f779 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -27,6 +27,7 @@
>  struct chip_data {
>  	u32 cr0;
>  	u32 rx_sample_dly;	/* RX sample delay */

> +	bool sste;		/* Slave select Toggle flag */

As Mark said there is no need in the new DT-property thus there is no
need in the sste flag being preserved in the chip-data structure
seeing there is a dedicated flag has been defined for this mode.

>  };
>  
>  #ifdef CONFIG_DEBUG_FS
> @@ -269,6 +270,7 @@ static irqreturn_t dw_spi_irq(int irq, void *dev_id)
>  
>  static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct spi_device *spi)
>  {
> +	struct chip_data *chip = spi_get_ctldata(spi);
>  	u32 cr0 = 0;
>  
>  	if (!(dws->caps & DW_SPI_CAP_DWC_SSI)) {
> @@ -285,6 +287,9 @@ static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct spi_device *spi)
>  
>  		/* CTRLR0[11] Shift Register Loop */
>  		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) << SPI_SRL_OFFSET;

> +
> +		/* CTRLR0[24] Slave Select Toggle Enable */
> +		cr0 |= chip->sste << SPI_SSTE_OFFSET;

Just check for the SPI_CS_WORD flag state here directly. Like this:
+ cr0 |= ((spi->mode & SPI_CS_WORD) ? 1 : 0) << SPI_SSTE_OFFSET;

>  	} else {
>  		/* CTRLR0[ 7: 6] Frame Format */
>  		cr0 |= SSI_MOTO_SPI << DWC_SSI_CTRLR0_FRF_OFFSET;
> @@ -300,6 +305,9 @@ static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct spi_device *spi)
>  		/* CTRLR0[13] Shift Register Loop */
>  		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) << DWC_SSI_CTRLR0_SRL_OFFSET;
>  

> +		/* CTRLR0[14] Slave Select Toggle Enable */
> +		cr0 |= chip->sste << DWC_SSI_CTRLR0_SSTE_OFFSET;
> +

the same as above.

>  		if (dws->caps & DW_SPI_CAP_KEEMBAY_MST)
>  			cr0 |= DWC_SSI_CTRLR0_KEEMBAY_MST;
>  	}
> @@ -789,6 +797,9 @@ static int dw_spi_setup(struct spi_device *spi)
>  		chip->rx_sample_dly = DIV_ROUND_CLOSEST(rx_sample_dly_ns,
>  							NSEC_PER_SEC /
>  							dws->max_freq);

> +
> +		/* Get slave select toggling feature requirement */
> +		chip->sste = device_property_read_bool(&spi->dev, "snps,sste");

As Mark said there is no need in this new DT-property.

-Sergey

>  	}
>  
>  	/*
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index b665e040862c..2ee3f839de39 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -65,8 +65,10 @@
>  #define SPI_SLVOE_OFFSET		10
>  #define SPI_SRL_OFFSET			11
>  #define SPI_CFS_OFFSET			12
> +#define SPI_SSTE_OFFSET			24
>  
>  /* Bit fields in CTRLR0 based on DWC_ssi_databook.pdf v1.01a */
> +#define DWC_SSI_CTRLR0_SSTE_OFFSET	14
>  #define DWC_SSI_CTRLR0_SRL_OFFSET	13
>  #define DWC_SSI_CTRLR0_TMOD_OFFSET	10
>  #define DWC_SSI_CTRLR0_TMOD_MASK	GENMASK(11, 10)
> -- 
> 2.17.1
>