Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 9 Jan 2023 19:43:15 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Sudip Mukherjee <sudip.mukherjee@sifive.com>
Cc:     Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        jude.onyenegecha@sifive.com, ben.dooks@sifive.com,
        jeegar.lakhani@sifive.com, linux-spi@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 01/15] spi: dw: Introduce spi_frf and STD_SPI
Message-ID: <20230109164315.au3nkqny7bh3bnr7@mobilestation>
References: <20221212180732.79167-1-sudip.mukherjee@sifive.com>
 <20221212180732.79167-2-sudip.mukherjee@sifive.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221212180732.79167-2-sudip.mukherjee@sifive.com>
Precedence: bulk

On Mon, Dec 12, 2022 at 06:07:18PM +0000, Sudip Mukherjee wrote:
> The DW APB SSI controllers of v4.x and newer and DW AHB SSI controllers
> supports enhanced SPI modes which can be defined from SPI_FRF of
> DW_SPI_CTRLR0 register. Without enhanced mode, these controllers will
> work in the standard spi mode.
> 
> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@sifive.com>
> ---
>  drivers/spi/spi-dw-core.c | 13 ++++++++++++-
>  drivers/spi/spi-dw.h      |  6 ++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 99edddf9958b9..77c23772bb3d9 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -333,6 +333,16 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
>  		/* CTRLR0[11:10] Transfer Mode */
>  		cr0 |= FIELD_PREP(DW_HSSI_CTRLR0_TMOD_MASK, cfg->tmode);
>  

> +	if (dw_spi_ver_is_ge(dws, HSSI, 103A)) {

eSPI has been available most likely since 1.00a (at least 1.01a
has that feature).

> +		cr0 &= ~DW_HSSI_CTRLR0_SPI_FRF_MASK;

No need in masking that field because the cr0 variable is
pre-initialized with the device-specific value anyway.

> +		cr0 |= FIELD_PREP(DW_HSSI_CTRLR0_SPI_FRF_MASK,

> +				  cfg->spi_frf);

The HW-manual defines that field as SPI_FRF, but the SPI_ prefix looks
vague because it doesn't differentiate it from just "frf" field. I'd
suggest to use the "enh_frf" name instead.

> +	} else if (dw_spi_ver_is_ge(dws, PSSI, 400A)) {

> +		cr0 &= ~DW_PSSI_CTRLR0_SPI_FRF_MASK;
> +		cr0 |= FIELD_PREP(DW_PSSI_CTRLR0_SPI_FRF_MASK,
> +				  cfg->spi_frf);

The same comments as above.

> +	}
> +
>  	dw_writel(dws, DW_SPI_CTRLR0, cr0);
>  
>  	if (cfg->tmode == DW_SPI_CTRLR0_TMOD_EPROMREAD ||
> @@ -422,6 +432,7 @@ static int dw_spi_transfer_one(struct spi_controller *master,
                                                 <--------+
>  		.tmode = DW_SPI_CTRLR0_TMOD_TR,           |
>  		.dfs = transfer->bits_per_word,           |
>  		.freq = transfer->speed_hz,               |
                                                          |
> +		.spi_frf = DW_SPI_CTRLR0_SPI_FRF_STD_SPI, +

You also forgot to update the spi-dw-bt1.c driver.

>  	};
>  	int ret;
>  
> @@ -664,7 +675,7 @@ static void dw_spi_stop_mem_op(struct dw_spi *dws, struct spi_device *spi)
>  static int dw_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  {
>  	struct dw_spi *dws = spi_controller_get_devdata(mem->spi->controller);

> -	struct dw_spi_cfg cfg;
> +	struct dw_spi_cfg cfg = {0};

Please explicitly initialize the enh_frf field in the method below in
the same way as it's done for the rest of the fields.

>  	unsigned long flags;
>  	int ret;
>  
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index 9e8eb2b52d5c7..414a415deb42a 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -17,6 +17,8 @@
>  
>  /* Synopsys DW SSI component versions (FourCC sequence) */
                                                  <-+
>  #define DW_HSSI_102A			0x3130322a  |
> +#define DW_HSSI_103A			0x3130332a  |
                                                    |
> +#define DW_PSSI_400A			0x3430302a -+

Please define the PSSI-macros above the HSSI ones.

>  
>  /* DW SSI IP-core ID and version check helpers */
>  #define dw_spi_ip_is(_dws, _ip) \
> @@ -94,6 +96,9 @@
>  #define DW_HSSI_CTRLR0_TMOD_MASK		GENMASK(11, 10)
>  #define DW_HSSI_CTRLR0_SRL			BIT(13)
                                                       <---------+
>  #define DW_HSSI_CTRLR0_MST			BIT(31)          |
                                                                 |
> +#define DW_HSSI_CTRLR0_SPI_FRF_MASK		GENMASK(23, 22) -+

This macro should be placed above the DW_HSSI_CTRLR0_MST one. Also
rename SPI_FRF to ENH_FRF.

> +#define DW_PSSI_CTRLR0_SPI_FRF_MASK		GENMASK(22, 21)
> +#define DW_SPI_CTRLR0_SPI_FRF_STD_SPI		0x0

1. Move these macros to the DW APB SSI group of the CSR fields macros.
2. Drop the SPI suffix from the DW_SPI_CTRLR0_SPI_FRF_STD_SPI macro.
3. Replace SPI_FRF with ENH_FRF name.

>  
>  /* Bit fields in CTRLR1 */
>  #define DW_SPI_NDF_MASK				GENMASK(15, 0)
> @@ -135,6 +140,7 @@ struct dw_spi_cfg {
>  	u8 dfs;
>  	u32 ndf;
>  	u32 freq;

> +	u8 spi_frf;

Please move it to the head of the structure and rename to "enh_frf".

-Serge(y)

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