linux-kernel - Re: [PATCH v2 03/15] spi: dw: update SPI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230109170610.6ndmqi57leb5scm7@mobilestation>
Date:   Mon, 9 Jan 2023 20:06:10 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Sudip Mukherjee <sudip.mukherjee@...ive.com>
Cc:     Mark Brown <broonie@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        jude.onyenegecha@...ive.com, ben.dooks@...ive.com,
        jeegar.lakhani@...ive.com, linux-spi@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 03/15] spi: dw: update SPI_CTRLR0 register

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@...ive.com>
> ---
>  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
>