Re: [PATCH v2 03/15] spi: dw: update SPI_CTRLR0 register

[Serge Semin <fancer.lancer@xxxxxxxxx>]

On Mon, Dec 12, 2022 at 06:07:20PM +0000, Sudip Mukherjee wrote:
> If the SPI transfer is being done in enhanced mode then SPI_CTRLR0
> register needs to be updated to mention the instruction length, address
> length, address and instruction transfer format, wait cycles. And, we
> also need to enable clock stretching.
> 
> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@xxxxxxxxxx>
> ---
>  drivers/spi/spi-dw-core.c | 14 +++++++++++++-
>  drivers/spi/spi-dw.h      | 11 +++++++++++
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 8c47a4d14b666..d59401f16c47a 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c

> @@ -320,7 +320,7 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
>  {
>  	struct dw_spi_chip_data *chip = spi_get_ctldata(spi);
>  	u32 cr0 = chip->cr0;

I suggest to update the dw_spi_update_config() semantic to accepting
optional eSPI configs by means of passing an additional argument
struct dw_spi_enh_cfg *ecfg. If it's null, then no need in updating
the SPI_CTRLR0 register.

> -	u32 speed_hz;
> +	u32 speed_hz, spi_ctrlr0;

Just reuse the cr0 variable.

>  	u16 clk_div;
>  
>  	/* CTRLR0[ 4/3: 0] or CTRLR0[ 20: 16] Data Frame Size */
> @@ -365,6 +365,18 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
>  		dw_writel(dws, DW_SPI_RX_SAMPLE_DLY, chip->rx_sample_dly);
>  		dws->cur_rx_sample_dly = chip->rx_sample_dly;
>  	}
> +
> +	if (cfg->spi_frf != DW_SPI_CTRLR0_SPI_FRF_STD_SPI) {
> +		spi_ctrlr0 = DW_SPI_SPI_CTRLR0_CLK_STRETCH_EN;
> +		spi_ctrlr0 |= FIELD_PREP(DW_SPI_SPI_CTRLR0_WAIT_CYCLE_MASK,
> +					 cfg->wait_c);
> +		spi_ctrlr0 |= FIELD_PREP(DW_SPI_SPI_CTRLR0_INST_L_MASK,
> +					 cfg->inst_l);
> +		spi_ctrlr0 |= FIELD_PREP(DW_SPI_SPI_CTRLR0_ADDR_L_MASK,
> +					 cfg->addr_l);

> +		spi_ctrlr0 |= cfg->trans_t;

Should be also handled by the FIELD_PREP() macro.

> +		dw_writel(dws, DW_SPI_SPI_CTRLR0, spi_ctrlr0);
> +	}
>  }
>  EXPORT_SYMBOL_NS_GPL(dw_spi_update_config, SPI_DW_CORE);
>  
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index 414a415deb42a..f29d89d05f34b 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -63,6 +63,7 @@
>  #define DW_SPI_DR			0x60
>  #define DW_SPI_RX_SAMPLE_DLY		0xf0
                                            <-+
>  #define DW_SPI_CS_OVERRIDE		0xf4  |
                                              |
> +#define DW_SPI_SPI_CTRLR0		0xf4 -+

Please replace SPI_CTRLR0 with ENH_CTRLR0 and move the macro
definition to where the arrow points to.

>  
>  /* Bit fields in CTRLR0 (DWC APB SSI) */
>  #define DW_PSSI_CTRLR0_DFS_MASK			GENMASK(3, 0)
> @@ -126,6 +127,12 @@
>  #define DW_SPI_DMACR_RDMAE			BIT(0)
>  #define DW_SPI_DMACR_TDMAE			BIT(1)
>  

> +/* Bit fields in SPI_CTRLR0 */
> +#define DW_SPI_SPI_CTRLR0_CLK_STRETCH_EN	BIT(30)
> +#define DW_SPI_SPI_CTRLR0_WAIT_CYCLE_MASK	GENMASK(15, 11)
> +#define DW_SPI_SPI_CTRLR0_INST_L_MASK		GENMASK(9, 8)
> +#define DW_SPI_SPI_CTRLR0_ADDR_L_MASK		GENMASK(5, 2)

First add DW_SPI_ENH_CTRLR0_TRANS_T_MASK macro too. Second please
replace SPI_CTRLR0 with ENH_CTRLR0.

> +
>  /* Mem/DMA operations helpers */
>  #define DW_SPI_WAIT_RETRIES			5
>  #define DW_SPI_BUF_SIZE \
> @@ -141,6 +148,10 @@ struct dw_spi_cfg {
>  	u32 ndf;
>  	u32 freq;
>  	u8 spi_frf;

> +	u8 trans_t;
> +	u8 inst_l;
> +	u8 addr_l;
> +	u8 wait_c;

Please move these to a separate structure:
struct dw_spi_enh_cfg {
	u8 wait_l;
	u8 inst_l;
	u8 addr_l;
	u8 trans_t;
};
Thus we'll be able to add an optional argument to the
dw_spi_update_config() method.

-Serge(y)

>  };
>  
>  struct dw_spi;
> -- 
> 2.30.2
>