diff mbox series

[03/11] spi: dw: define spi_frf for dual/quad/octal modes

Message ID 20220802175755.6530-4-sudip.mukherjee@sifive.com (mailing list archive)
State New, archived
Headers show
Series Add support for enhanced SPI for Designware SPI controllers | expand

Commit Message

Sudip Mukherjee Aug. 2, 2022, 5:57 p.m. UTC
The SPI mode needs to be mentioned in CTRLR0[23:22] register. Define a
configuration variable to keep the mode based on the buswidth, which will
then be used to update CR0. If the transfer is using dual/quad/octal
mode then mark enhanced_spi as true.

Signed-off-by: Sudip Mukherjee <sudip.mukherjee@sifive.com>
---
 drivers/spi/spi-dw-core.c | 29 +++++++++++++++++++++++++++++
 drivers/spi/spi-dw.h      |  7 +++++++
 2 files changed, 36 insertions(+)

Comments

Serge Semin Aug. 26, 2022, 10:03 p.m. UTC | #1
On Tue, Aug 02, 2022 at 06:57:47PM +0100, Sudip Mukherjee wrote:
> The SPI mode needs to be mentioned in CTRLR0[23:22] register. Define a
> configuration variable to keep the mode based on the buswidth, which will
> then be used to update CR0. If the transfer is using dual/quad/octal
> mode then mark enhanced_spi as true.
> 
> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@sifive.com>
> ---
>  drivers/spi/spi-dw-core.c | 29 +++++++++++++++++++++++++++++
>  drivers/spi/spi-dw.h      |  7 +++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 77529e359b6d..8c84a2e991b5 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -333,6 +333,14 @@ 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 (dws->caps & DW_SPI_CAP_EXT_SPI) {

Drop this conditional statement. Just always set the spi_frf field.


> +		if (cfg->spi_frf)

Drop this conditional statement. Just always set the spi_frf. It shall
be zero if the Enhanced SPI modes are unsupported.

> +			cr0 |= FIELD_PREP(DW_HSSI_CTRLR0_SPI_FRF_MASK,

Since the SPI_FRF field has different offset in the DW APB and DW AHB
SSI controllers, you'll need to initialize the mode based on the
IP-core ID using the dw_spi_ip_is() macro (see the as it's done for
tmode).

> +				cfg->spi_frf);
> +		else

> +			cr0 &= ~DW_HSSI_CTRLR0_SPI_FRF_MASK;

This is redundant. The initial value never has the SPI_FRF field set
(see the dw_spi_setup() and dw_spi_prepare_cr0() methods for details).

> +	}
> +
>  	dw_writel(dws, DW_SPI_CTRLR0, cr0);
>  
>  	if (cfg->tmode == DW_SPI_CTRLR0_TMOD_EPROMREAD ||
> @@ -679,10 +687,31 @@ 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);
> +	bool enhanced_spi = false;
>  	struct dw_spi_cfg cfg;
>  	unsigned long flags;
>  	int ret;
>  

> +	if (dws->caps & DW_SPI_CAP_EXT_SPI) {
> +		switch (op->data.buswidth) {
> +		case 2:
> +			cfg.spi_frf = DW_SSI_CTRLR0_SPI_FRF_DUAL_SPI;
> +			enhanced_spi = true;
> +			break;
> +		case 4:
> +			cfg.spi_frf = DW_SSI_CTRLR0_SPI_FRF_QUAD_SPI;
> +			enhanced_spi = true;
> +			break;
> +		case 8:
> +			cfg.spi_frf = DW_SSI_CTRLR0_SPI_FRF_OCT_SPI;
> +			enhanced_spi = true;
> +			break;
> +		default:
> +			cfg.spi_frf = 0;
> +			break;
> +		}
> +	}
> +

Please create a separate dw_spi_exec_enh_mem_op() method for the
Enhanced SPI communications and parse the data.buswidth field there.
Also it would be better to define a new macro
DW_SSI_CTRLR0_SPI_FRF_STD_SPI and use it instead of the explicit zero
SPI_FRF value.

>  	/*
>  	 * Collect the outbound data into a single buffer to speed the
>  	 * transmission up at least on the initial stage.
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index 71d18e9291a3..b8cc20e0deaa 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -96,6 +96,12 @@
>  #define DW_HSSI_CTRLR0_SRL			BIT(13)
>  #define DW_HSSI_CTRLR0_MST			BIT(31)
>  

> +/* Bit fields in CTRLR0 for enhanced SPI */

No need in the comment. The macros name is descriptive enough.

> +#define DW_HSSI_CTRLR0_SPI_FRF_MASK		GENMASK(23, 22)

Please also add the next new macros
#define DW_PSSI_CTRLR0_SPI_FRF_MASK		GENMASK(22, 21)
#define DW_SSI_CTRLR0_SPI_FRF_STD_SPI		0x0

and group them together with the rest of the APB SSI CTRL0 macros.

> +#define DW_SSI_CTRLR0_SPI_FRF_DUAL_SPI		0x1
> +#define DW_SSI_CTRLR0_SPI_FRF_QUAD_SPI		0x2
> +#define DW_SSI_CTRLR0_SPI_FRF_OCT_SPI		0x3

Please move these to be defined together with the APB SSI
CTRL0.SPI_FRF-related macros.

-Sergey

> +
>  /* Bit fields in CTRLR1 */
>  #define DW_SPI_NDF_MASK				GENMASK(15, 0)
>  
> @@ -136,6 +142,7 @@ struct dw_spi_cfg {
>  	u8 dfs;
>  	u32 ndf;
>  	u32 freq;
> +	u8 spi_frf;
>  };
>  
>  struct dw_spi;
> -- 
> 2.30.2
>
Serge Semin Aug. 26, 2022, 10:22 p.m. UTC | #2
On Sat, Aug 27, 2022 at 01:03:22AM +0300, Serge Semin wrote:
> On Tue, Aug 02, 2022 at 06:57:47PM +0100, Sudip Mukherjee wrote:
> > The SPI mode needs to be mentioned in CTRLR0[23:22] register. Define a
> > configuration variable to keep the mode based on the buswidth, which will
> > then be used to update CR0. If the transfer is using dual/quad/octal
> > mode then mark enhanced_spi as true.
> > 
> > Signed-off-by: Sudip Mukherjee <sudip.mukherjee@sifive.com>
> > ---
> >  drivers/spi/spi-dw-core.c | 29 +++++++++++++++++++++++++++++
> >  drivers/spi/spi-dw.h      |  7 +++++++
> >  2 files changed, 36 insertions(+)
> > 
> > diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> > index 77529e359b6d..8c84a2e991b5 100644
> > --- a/drivers/spi/spi-dw-core.c
> > +++ b/drivers/spi/spi-dw-core.c
> > @@ -333,6 +333,14 @@ 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 (dws->caps & DW_SPI_CAP_EXT_SPI) {
> 
> Drop this conditional statement. Just always set the spi_frf field.
> 
> 
> > +		if (cfg->spi_frf)
> 
> Drop this conditional statement. Just always set the spi_frf. It shall
> be zero if the Enhanced SPI modes are unsupported.
> 
> > +			cr0 |= FIELD_PREP(DW_HSSI_CTRLR0_SPI_FRF_MASK,
> 
> Since the SPI_FRF field has different offset in the DW APB and DW AHB
> SSI controllers, you'll need to initialize the mode based on the
> IP-core ID using the dw_spi_ip_is() macro (see the as it's done for
> tmode).
> 
> > +				cfg->spi_frf);
> > +		else
> 
> > +			cr0 &= ~DW_HSSI_CTRLR0_SPI_FRF_MASK;
> 
> This is redundant. The initial value never has the SPI_FRF field set
> (see the dw_spi_setup() and dw_spi_prepare_cr0() methods for details).
> 
> > +	}
> > +
> >  	dw_writel(dws, DW_SPI_CTRLR0, cr0);
> >  
> >  	if (cfg->tmode == DW_SPI_CTRLR0_TMOD_EPROMREAD ||
> > @@ -679,10 +687,31 @@ 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);
> > +	bool enhanced_spi = false;
> >  	struct dw_spi_cfg cfg;
> >  	unsigned long flags;
> >  	int ret;
> >  
> 
> > +	if (dws->caps & DW_SPI_CAP_EXT_SPI) {
> > +		switch (op->data.buswidth) {
> > +		case 2:
> > +			cfg.spi_frf = DW_SSI_CTRLR0_SPI_FRF_DUAL_SPI;
> > +			enhanced_spi = true;
> > +			break;
> > +		case 4:
> > +			cfg.spi_frf = DW_SSI_CTRLR0_SPI_FRF_QUAD_SPI;
> > +			enhanced_spi = true;
> > +			break;
> > +		case 8:
> > +			cfg.spi_frf = DW_SSI_CTRLR0_SPI_FRF_OCT_SPI;
> > +			enhanced_spi = true;
> > +			break;
> > +		default:
> > +			cfg.spi_frf = 0;
> > +			break;
> > +		}
> > +	}
> > +
> 
> Please create a separate dw_spi_exec_enh_mem_op() method for the
> Enhanced SPI communications and parse the data.buswidth field there.
> Also it would be better to define a new macro
> DW_SSI_CTRLR0_SPI_FRF_STD_SPI and use it instead of the explicit zero
> SPI_FRF value.
> 
> >  	/*
> >  	 * Collect the outbound data into a single buffer to speed the
> >  	 * transmission up at least on the initial stage.
> > diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> > index 71d18e9291a3..b8cc20e0deaa 100644
> > --- a/drivers/spi/spi-dw.h
> > +++ b/drivers/spi/spi-dw.h
> > @@ -96,6 +96,12 @@
> >  #define DW_HSSI_CTRLR0_SRL			BIT(13)
> >  #define DW_HSSI_CTRLR0_MST			BIT(31)
> >  
> 
> > +/* Bit fields in CTRLR0 for enhanced SPI */
> 
> No need in the comment. The macros name is descriptive enough.
> 
> > +#define DW_HSSI_CTRLR0_SPI_FRF_MASK		GENMASK(23, 22)
> 
> Please also add the next new macros
> #define DW_PSSI_CTRLR0_SPI_FRF_MASK		GENMASK(22, 21)

> #define DW_SSI_CTRLR0_SPI_FRF_STD_SPI		0x0
> 
> and group them together with the rest of the APB SSI CTRL0 macros.
> 
> > +#define DW_SSI_CTRLR0_SPI_FRF_DUAL_SPI		0x1
> > +#define DW_SSI_CTRLR0_SPI_FRF_QUAD_SPI		0x2
> > +#define DW_SSI_CTRLR0_SPI_FRF_OCT_SPI		0x3

One more thing s/DW_SSI/DW_SPI . All the common macros are defined
with the DW_SPI prefix.

-Sergey

> 
> Please move these to be defined together with the APB SSI
> CTRL0.SPI_FRF-related macros.
> 
> -Sergey
> 
> > +
> >  /* Bit fields in CTRLR1 */
> >  #define DW_SPI_NDF_MASK				GENMASK(15, 0)
> >  
> > @@ -136,6 +142,7 @@ struct dw_spi_cfg {
> >  	u8 dfs;
> >  	u32 ndf;
> >  	u32 freq;
> > +	u8 spi_frf;
> >  };
> >  
> >  struct dw_spi;
> > -- 
> > 2.30.2
> >
diff mbox series

Patch

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 77529e359b6d..8c84a2e991b5 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -333,6 +333,14 @@  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 (dws->caps & DW_SPI_CAP_EXT_SPI) {
+		if (cfg->spi_frf)
+			cr0 |= FIELD_PREP(DW_HSSI_CTRLR0_SPI_FRF_MASK,
+				cfg->spi_frf);
+		else
+			cr0 &= ~DW_HSSI_CTRLR0_SPI_FRF_MASK;
+	}
+
 	dw_writel(dws, DW_SPI_CTRLR0, cr0);
 
 	if (cfg->tmode == DW_SPI_CTRLR0_TMOD_EPROMREAD ||
@@ -679,10 +687,31 @@  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);
+	bool enhanced_spi = false;
 	struct dw_spi_cfg cfg;
 	unsigned long flags;
 	int ret;
 
+	if (dws->caps & DW_SPI_CAP_EXT_SPI) {
+		switch (op->data.buswidth) {
+		case 2:
+			cfg.spi_frf = DW_SSI_CTRLR0_SPI_FRF_DUAL_SPI;
+			enhanced_spi = true;
+			break;
+		case 4:
+			cfg.spi_frf = DW_SSI_CTRLR0_SPI_FRF_QUAD_SPI;
+			enhanced_spi = true;
+			break;
+		case 8:
+			cfg.spi_frf = DW_SSI_CTRLR0_SPI_FRF_OCT_SPI;
+			enhanced_spi = true;
+			break;
+		default:
+			cfg.spi_frf = 0;
+			break;
+		}
+	}
+
 	/*
 	 * Collect the outbound data into a single buffer to speed the
 	 * transmission up at least on the initial stage.
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 71d18e9291a3..b8cc20e0deaa 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -96,6 +96,12 @@ 
 #define DW_HSSI_CTRLR0_SRL			BIT(13)
 #define DW_HSSI_CTRLR0_MST			BIT(31)
 
+/* Bit fields in CTRLR0 for enhanced SPI */
+#define DW_HSSI_CTRLR0_SPI_FRF_MASK		GENMASK(23, 22)
+#define DW_SSI_CTRLR0_SPI_FRF_DUAL_SPI		0x1
+#define DW_SSI_CTRLR0_SPI_FRF_QUAD_SPI		0x2
+#define DW_SSI_CTRLR0_SPI_FRF_OCT_SPI		0x3
+
 /* Bit fields in CTRLR1 */
 #define DW_SPI_NDF_MASK				GENMASK(15, 0)
 
@@ -136,6 +142,7 @@  struct dw_spi_cfg {
 	u8 dfs;
 	u32 ndf;
 	u32 freq;
+	u8 spi_frf;
 };
 
 struct dw_spi;