diff mbox series

[11/30] spi: dw: Add DWC SSI capability

Message ID 20200920112914.26501-12-Sergey.Semin@baikalelectronics.ru (mailing list archive)
State Superseded
Headers show
Series spi: dw: Add full Baikal-T1 SPI Controllers support | expand

Commit Message

Serge Semin Sept. 20, 2020, 11:28 a.m. UTC 
Currently DWC SSI core is supported by means of setting up the
core-specific update_cr0() callback. It isn't suitable for multiple
reasons. First of all having exported several methods doing the same thing
but for different chips makes the code harder to maintain. Secondly the
spi-dw-core driver exports the methods, then the spi-dw-mmio driver sets
the private data callback with one of them so to be called by the core
driver again. That makes the code logic too complicated. Thirdly using
callbacks for just updating the CR0 register is problematic, since in case
if the register needed to be updated from different parts of the code,
we'd have to create another callback (for instance the SPI device-specific
parameters don't need to be calculated each time the SPI transfer is
submitted, so it's better to pre-calculate the CR0 data at the SPI-device
setup stage).

So keeping all the above in mind let's discard the update_cr0() callbacks,
define a generic and static dw_spi_update_cr0() method and create the
DW_SPI_CAP_DWC_SSI capability, which when enabled would activate the
alternative CR0 register layout.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/spi/spi-dw-core.c | 69 ++++++++++++---------------------------
 drivers/spi/spi-dw-mmio.c | 20 ++----------
 drivers/spi/spi-dw.h      |  9 +----
 3 files changed, 23 insertions(+), 75 deletions(-)

Comments

Mark Brown Sept. 29, 2020, 1:52 p.m. UTC | #1 
On Sun, Sep 20, 2020 at 02:28:55PM +0300, Serge Semin wrote:

> -	/*
> -	 * SPI mode (SCPOL|SCPH)
> -	 * CTRLR0[ 8] Serial Clock Phase
> -	 * CTRLR0[ 9] Serial Clock Polarity
> -	 */
> -	cr0 |= ((spi->mode & SPI_CPOL) ? 1 : 0) << DWC_SSI_CTRLR0_SCPOL_OFFSET;
> -	cr0 |= ((spi->mode & SPI_CPHA) ? 1 : 0) << DWC_SSI_CTRLR0_SCPH_OFFSET;

> +		cr0 |= SSI_MOTO_SPI << DWC_SSI_CTRLR0_FRF_OFFSET;
> +		cr0 |= ((spi->mode & SPI_CPOL) ? 1 : 0) << DWC_SSI_CTRLR0_SCPOL_OFFSET;
> +		cr0 |= ((spi->mode & SPI_CPHA) ? 1 : 0) << DWC_SSI_CTRLR0_SCPH_OFFSET;

The new code seems less well commented than the old code here.
Serge Semin Sept. 29, 2020, 10:17 p.m. UTC | #2 
On Tue, Sep 29, 2020 at 02:52:33PM +0100, Mark Brown wrote:
> On Sun, Sep 20, 2020 at 02:28:55PM +0300, Serge Semin wrote:
> 
> > -	/*
> > -	 * SPI mode (SCPOL|SCPH)
> > -	 * CTRLR0[ 8] Serial Clock Phase
> > -	 * CTRLR0[ 9] Serial Clock Polarity
> > -	 */
> > -	cr0 |= ((spi->mode & SPI_CPOL) ? 1 : 0) << DWC_SSI_CTRLR0_SCPOL_OFFSET;
> > -	cr0 |= ((spi->mode & SPI_CPHA) ? 1 : 0) << DWC_SSI_CTRLR0_SCPH_OFFSET;
> 

> > +		cr0 |= SSI_MOTO_SPI << DWC_SSI_CTRLR0_FRF_OFFSET;
> > +		cr0 |= ((spi->mode & SPI_CPOL) ? 1 : 0) << DWC_SSI_CTRLR0_SCPOL_OFFSET;
> > +		cr0 |= ((spi->mode & SPI_CPHA) ? 1 : 0) << DWC_SSI_CTRLR0_SCPH_OFFSET;
> 
> The new code seems less well commented than the old code here.

You are right. The comments are omitted. The thing is that they are absolutely
redundant here, for the same reason they haven't been added to the standard
update_cr0() method. Both the DWC SSI-capable and standard DW APB SSI-specific
part of the code do the same thing: setup the CTRLR0 fields, which are described
by the macro definitions. So there is no need to duplicate that information in
the comments, moreover seeing it can be inferred from the code.

-Sergey
Serge Semin Sept. 30, 2020, 3:03 p.m. UTC | #3 
Mark,
A concrete question is below of my previous comment.

On Wed, Sep 30, 2020 at 01:17:37AM +0300, Serge Semin wrote:
> On Tue, Sep 29, 2020 at 02:52:33PM +0100, Mark Brown wrote:
> > On Sun, Sep 20, 2020 at 02:28:55PM +0300, Serge Semin wrote:
> > 
> > > -	/*
> > > -	 * SPI mode (SCPOL|SCPH)
> > > -	 * CTRLR0[ 8] Serial Clock Phase
> > > -	 * CTRLR0[ 9] Serial Clock Polarity
> > > -	 */
> > > -	cr0 |= ((spi->mode & SPI_CPOL) ? 1 : 0) << DWC_SSI_CTRLR0_SCPOL_OFFSET;
> > > -	cr0 |= ((spi->mode & SPI_CPHA) ? 1 : 0) << DWC_SSI_CTRLR0_SCPH_OFFSET;
> > 
> 
> > > +		cr0 |= SSI_MOTO_SPI << DWC_SSI_CTRLR0_FRF_OFFSET;
> > > +		cr0 |= ((spi->mode & SPI_CPOL) ? 1 : 0) << DWC_SSI_CTRLR0_SCPOL_OFFSET;
> > > +		cr0 |= ((spi->mode & SPI_CPHA) ? 1 : 0) << DWC_SSI_CTRLR0_SCPH_OFFSET;
> > 
> > The new code seems less well commented than the old code here.
> 
> You are right. The comments are omitted. The thing is that they are absolutely
> redundant here, for the same reason they haven't been added to the standard
> update_cr0() method. Both the DWC SSI-capable and standard DW APB SSI-specific
> part of the code do the same thing: setup the CTRLR0 fields, which are described
> by the macro definitions. So there is no need to duplicate that information in
> the comments, moreover seeing it can be inferred from the code.
> 
> -Sergey

My response to your comment was that those in-code comments have been absolutely
redundant. So I just removed them, since I was touching that part of the driver
anyway. If you are agree with me having that done here, then please, accept the
patch the way it is. If you disagree, or have any other though, please give me
your answer, why.

-Sergey
Mark Brown Sept. 30, 2020, 3:41 p.m. UTC | #4 
On Wed, Sep 30, 2020 at 06:03:12PM +0300, Serge Semin wrote:
> On Wed, Sep 30, 2020 at 01:17:37AM +0300, Serge Semin wrote:

> > > > -	/*
> > > > -	 * SPI mode (SCPOL|SCPH)
> > > > -	 * CTRLR0[ 8] Serial Clock Phase
> > > > -	 * CTRLR0[ 9] Serial Clock Polarity
> > > > -	 */
> > > > -	cr0 |= ((spi->mode & SPI_CPOL) ? 1 : 0) << DWC_SSI_CTRLR0_SCPOL_OFFSET;

> anyway. If you are agree with me having that done here, then please, accept the
> patch the way it is. If you disagree, or have any other though, please give me
> your answer, why.

Those comments did seem to help mitigate the wall of acronym soup issue
that the code has, it seems a shame to drop them.
Serge Semin Sept. 30, 2020, 3:53 p.m. UTC | #5 
On Wed, Sep 30, 2020 at 04:41:49PM +0100, Mark Brown wrote:
> On Wed, Sep 30, 2020 at 06:03:12PM +0300, Serge Semin wrote:
> > On Wed, Sep 30, 2020 at 01:17:37AM +0300, Serge Semin wrote:
> 
> > > > > -	/*
> > > > > -	 * SPI mode (SCPOL|SCPH)
> > > > > -	 * CTRLR0[ 8] Serial Clock Phase
> > > > > -	 * CTRLR0[ 9] Serial Clock Polarity
> > > > > -	 */
> > > > > -	cr0 |= ((spi->mode & SPI_CPOL) ? 1 : 0) << DWC_SSI_CTRLR0_SCPOL_OFFSET;
> 
> > anyway. If you are agree with me having that done here, then please, accept the
> > patch the way it is. If you disagree, or have any other though, please give me
> > your answer, why.
> 
> Those comments did seem to help mitigate the wall of acronym soup issue
> that the code has, it seems a shame to drop them.

I see your point, but still don't think that those comment give much help like you
said, because the mode->register mapping can be easily derived from the macro
naming and values.

Anyway since you insist on having the comments left here, I'll get them back and
add the similar ones for the standard DW-APB-SSI version of the controller so
the code would look coherent.

-Sergey
diff mbox series

Patch

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 8f9737640ec1..c21641a485ce 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -228,60 +228,33 @@  static irqreturn_t dw_spi_irq(int irq, void *dev_id)
 	return dws->transfer_handler(dws);
 }
 
-/* Configure CTRLR0 for DW_apb_ssi */
-u32 dw_spi_update_cr0(struct spi_controller *master, struct spi_device *spi,
-		      struct spi_transfer *transfer)
+static void dw_spi_update_cr0(struct dw_spi *dws, struct spi_device *spi,
+			      struct spi_transfer *transfer)
 {
 	struct chip_data *chip = spi_get_ctldata(spi);
 	u32 cr0;
 
-	/* Default SPI mode is SCPOL = 0, SCPH = 0 */
-	cr0 = (transfer->bits_per_word - 1)
-		| (SSI_MOTO_SPI << SPI_FRF_OFFSET)
-		| ((((spi->mode & SPI_CPOL) ? 1 : 0) << SPI_SCOL_OFFSET) |
-		   (((spi->mode & SPI_CPHA) ? 1 : 0) << SPI_SCPH_OFFSET) |
-		   (((spi->mode & SPI_LOOP) ? 1 : 0) << SPI_SRL_OFFSET))
-		| (chip->tmode << SPI_TMOD_OFFSET);
-
-	return cr0;
-}
-EXPORT_SYMBOL_GPL(dw_spi_update_cr0);
-
-/* Configure CTRLR0 for DWC_ssi */
-u32 dw_spi_update_cr0_v1_01a(struct spi_controller *master,
-			     struct spi_device *spi,
-			     struct spi_transfer *transfer)
-{
-	struct dw_spi *dws = spi_controller_get_devdata(master);
-	struct chip_data *chip = spi_get_ctldata(spi);
-	u32 cr0;
-
-	/* CTRLR0[ 4: 0] Data Frame Size */
 	cr0 = (transfer->bits_per_word - 1);
 
-	/* CTRLR0[ 7: 6] Frame Format */
-	cr0 |= SSI_MOTO_SPI << DWC_SSI_CTRLR0_FRF_OFFSET;
-
-	/*
-	 * SPI mode (SCPOL|SCPH)
-	 * CTRLR0[ 8] Serial Clock Phase
-	 * CTRLR0[ 9] Serial Clock Polarity
-	 */
-	cr0 |= ((spi->mode & SPI_CPOL) ? 1 : 0) << DWC_SSI_CTRLR0_SCPOL_OFFSET;
-	cr0 |= ((spi->mode & SPI_CPHA) ? 1 : 0) << DWC_SSI_CTRLR0_SCPH_OFFSET;
-
-	/* CTRLR0[11:10] Transfer Mode */
-	cr0 |= chip->tmode << DWC_SSI_CTRLR0_TMOD_OFFSET;
-
-	/* CTRLR0[13] Shift Register Loop */
-	cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) << DWC_SSI_CTRLR0_SRL_OFFSET;
-
-	if (dws->caps & DW_SPI_CAP_KEEMBAY_MST)
-		cr0 |= DWC_SSI_CTRLR0_KEEMBAY_MST;
+	if (!(dws->caps & DW_SPI_CAP_DWC_SSI)) {
+		cr0 |= SSI_MOTO_SPI << SPI_FRF_OFFSET;
+		cr0 |= ((spi->mode & SPI_CPOL) ? 1 : 0) << SPI_SCOL_OFFSET;
+		cr0 |= ((spi->mode & SPI_CPHA) ? 1 : 0) << SPI_SCPH_OFFSET;
+		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) << SPI_SRL_OFFSET;
+		cr0 |= chip->tmode << SPI_TMOD_OFFSET;
+	} else {
+		cr0 |= SSI_MOTO_SPI << DWC_SSI_CTRLR0_FRF_OFFSET;
+		cr0 |= ((spi->mode & SPI_CPOL) ? 1 : 0) << DWC_SSI_CTRLR0_SCPOL_OFFSET;
+		cr0 |= ((spi->mode & SPI_CPHA) ? 1 : 0) << DWC_SSI_CTRLR0_SCPH_OFFSET;
+		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) << DWC_SSI_CTRLR0_SRL_OFFSET;
+		cr0 |= chip->tmode << DWC_SSI_CTRLR0_TMOD_OFFSET;
+
+		if (dws->caps & DW_SPI_CAP_KEEMBAY_MST)
+			cr0 |= DWC_SSI_CTRLR0_KEEMBAY_MST;
+	}
 
-	return cr0;
+	dw_writel(dws, DW_SPI_CTRLR0, cr0);
 }
-EXPORT_SYMBOL_GPL(dw_spi_update_cr0_v1_01a);
 
 static int dw_spi_transfer_one(struct spi_controller *master,
 		struct spi_device *spi, struct spi_transfer *transfer)
@@ -290,7 +263,6 @@  static int dw_spi_transfer_one(struct spi_controller *master,
 	struct chip_data *chip = spi_get_ctldata(spi);
 	u8 imask = 0;
 	u16 txlevel = 0;
-	u32 cr0;
 	int ret;
 
 	dws->dma_mapped = 0;
@@ -319,8 +291,7 @@  static int dw_spi_transfer_one(struct spi_controller *master,
 
 	transfer->effective_speed_hz = dws->max_freq / chip->clk_div;
 
-	cr0 = dws->update_cr0(master, spi, transfer);
-	dw_writel(dws, DW_SPI_CTRLR0, cr0);
+	dw_spi_update_cr0(dws, spi, transfer);
 
 	/* Check if current transfer is a DMA transaction */
 	if (master->can_dma && master->can_dma(master, spi, transfer))
diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index c0d351fde782..d0cc5bf4fa4e 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -110,9 +110,6 @@  static int dw_spi_mscc_init(struct platform_device *pdev,
 	dwsmmio->dws.set_cs = dw_spi_mscc_set_cs;
 	dwsmmio->priv = dwsmscc;
 
-	/* Register hook to configure CTRLR0 */
-	dwsmmio->dws.update_cr0 = dw_spi_update_cr0;
-
 	return 0;
 }
 
@@ -188,9 +185,6 @@  static int dw_spi_mscc_sparx5_init(struct platform_device *pdev,
 	dwsmmio->dws.set_cs = dw_spi_sparx5_set_cs;
 	dwsmmio->priv = dwsmscc;
 
-	/* Register hook to configure CTRLR0 */
-	dwsmmio->dws.update_cr0 = dw_spi_update_cr0;
-
 	return 0;
 }
 
@@ -199,18 +193,12 @@  static int dw_spi_alpine_init(struct platform_device *pdev,
 {
 	dwsmmio->dws.caps = DW_SPI_CAP_CS_OVERRIDE;
 
-	/* Register hook to configure CTRLR0 */
-	dwsmmio->dws.update_cr0 = dw_spi_update_cr0;
-
 	return 0;
 }
 
 static int dw_spi_dw_apb_init(struct platform_device *pdev,
 			      struct dw_spi_mmio *dwsmmio)
 {
-	/* Register hook to configure CTRLR0 */
-	dwsmmio->dws.update_cr0 = dw_spi_update_cr0;
-
 	dw_spi_dma_setup_generic(&dwsmmio->dws);
 
 	return 0;
@@ -219,8 +207,7 @@  static int dw_spi_dw_apb_init(struct platform_device *pdev,
 static int dw_spi_dwc_ssi_init(struct platform_device *pdev,
 			       struct dw_spi_mmio *dwsmmio)
 {
-	/* Register hook to configure CTRLR0 */
-	dwsmmio->dws.update_cr0 = dw_spi_update_cr0_v1_01a;
+	dwsmmio->dws.caps = DW_SPI_CAP_DWC_SSI;
 
 	dw_spi_dma_setup_generic(&dwsmmio->dws);
 
@@ -230,10 +217,7 @@  static int dw_spi_dwc_ssi_init(struct platform_device *pdev,
 static int dw_spi_keembay_init(struct platform_device *pdev,
 			       struct dw_spi_mmio *dwsmmio)
 {
-	dwsmmio->dws.caps = DW_SPI_CAP_KEEMBAY_MST;
-
-	/* Register hook to configure CTRLR0 */
-	dwsmmio->dws.update_cr0 = dw_spi_update_cr0_v1_01a;
+	dwsmmio->dws.caps = DW_SPI_CAP_KEEMBAY_MST | DW_SPI_CAP_DWC_SSI;
 
 	return 0;
 }
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index da9b543322c9..c02351cf2f99 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -109,6 +109,7 @@  enum dw_ssi_type {
 /* DW SPI capabilities */
 #define DW_SPI_CAP_CS_OVERRIDE		BIT(0)
 #define DW_SPI_CAP_KEEMBAY_MST		BIT(1)
+#define DW_SPI_CAP_DWC_SSI		BIT(2)
 
 struct dw_spi;
 struct dw_spi_dma_ops {
@@ -136,8 +137,6 @@  struct dw_spi {
 	u16			bus_num;
 	u16			num_cs;		/* supported slave numbers */
 	void (*set_cs)(struct spi_device *spi, bool enable);
-	u32 (*update_cr0)(struct spi_controller *master, struct spi_device *spi,
-			  struct spi_transfer *transfer);
 
 	/* Current message transfer state info */
 	size_t			len;
@@ -254,12 +253,6 @@  extern int dw_spi_add_host(struct device *dev, struct dw_spi *dws);
 extern void dw_spi_remove_host(struct dw_spi *dws);
 extern int dw_spi_suspend_host(struct dw_spi *dws);
 extern int dw_spi_resume_host(struct dw_spi *dws);
-extern u32 dw_spi_update_cr0(struct spi_controller *master,
-			     struct spi_device *spi,
-			     struct spi_transfer *transfer);
-extern u32 dw_spi_update_cr0_v1_01a(struct spi_controller *master,
-				    struct spi_device *spi,
-				    struct spi_transfer *transfer);
 
 #ifdef CONFIG_SPI_DW_DMA