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@xxxxxxxxxx>
> ---
> 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
>
