Re: [PATCH v2 2/2] spi: dw: Add support for Intel Thunder Bay SPI

[Serge Semin <fancer.lancer@xxxxxxxxx>]

On Tue, Sep 07, 2021 at 10:54:10AM +0000, Srikandan, Nandhini wrote:
> 
> 
> > -----Original Message-----
> > From: Serge Semin <fancer.lancer@xxxxxxxxx>
> > Sent: Sunday, September 5, 2021 8:04 PM
> > To: Srikandan, Nandhini <nandhini.srikandan@xxxxxxxxx>
> > Cc: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>; broonie@xxxxxxxxxx;
> > robh+dt@xxxxxxxxxx; linux-spi@xxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > mgross@xxxxxxxxxxxxxxx; Pan, Kris <kris.pan@xxxxxxxxx>; Demakkanavar,
> > Kenchappa <kenchappa.demakkanavar@xxxxxxxxx>; Zhou, Furong
> > <furong.zhou@xxxxxxxxx>; Sangannavar, Mallikarjunappa
> > <mallikarjunappa.sangannavar@xxxxxxxxx>; Vaidya, Mahesh R
> > <mahesh.r.vaidya@xxxxxxxxx>; A, Rashmi <rashmi.a@xxxxxxxxx>
> > Subject: Re: [PATCH v2 2/2] spi: dw: Add support for Intel Thunder Bay SPI
> > 
> > Hi Nandhini
> > 
> > On Tue, Aug 24, 2021 at 04:58:56PM +0800, nandhini.srikandan@xxxxxxxxx
> > wrote:
> > > From: Nandhini Srikandan <nandhini.srikandan@xxxxxxxxx>
> > >
> > > Add support for Intel Thunder Bay SPI controller, which uses
> > > DesignWare DWC_ssi core.
> > > Bit 31 of CTRLR0 register is set for Thunder Bay, to configure the
> > > device as a master or as a slave serial peripheral.
> > > Bit 14(SSTE) of CTRLR0 register should be set(1) for Thunder Bay.
> > 
> > After reading your response to my v1 comments, I've got a better notion of
> > the features you are trying to implement here. Please see my comments
> > below.
> > 
> > >
> > > Signed-off-by: Nandhini Srikandan <nandhini.srikandan@xxxxxxxxx>
> > > ---
> > 
> > Just to note for your future patchwork. Instead of having a single general
> > changelog text in the cover letter it is much more convenient for reviewers to
> > see both the summary changelog and a changelog of individual patches here
> > under '---' delimiter.
> Sure, I will add changelog for individual patches also.
> 
> > 
> > >  drivers/spi/spi-dw-core.c |  7 +++++--  drivers/spi/spi-dw-mmio.c |
> > > 20 +++++++++++++++++++-
> > >  drivers/spi/spi-dw.h      | 12 +++++++++---
> > >  3 files changed, 33 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> > > index a305074c482e..f7d45318db8a 100644
> > > --- a/drivers/spi/spi-dw-core.c
> > > +++ b/drivers/spi/spi-dw-core.c
> > > @@ -300,8 +300,11 @@ static u32 dw_spi_prepare_cr0(struct dw_spi
> > *dws, struct spi_device *spi)
> > >  		/* 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_MST)
> > > +			cr0 |= DWC_SSI_CTRLR0_MST;
> > 
> > Since you've used a generic suffix here, are you sure the MST/SLV feature
> > toggled by the BIT(31) bit is generic for all DWC SSI controllers?
> > I am asking because I don't have DWC SSI IP manual, but there is a
> > CTRL0 register layout posted by your colleague Wan Ahmad Zainie a year
> > ago: https://patches.linaro.org/patch/214693/ . It doesn't have that bit
> > defined.
> > 
> > If you are and it's specific to all DWC SSI controllers of v1.01a and newer,
> > then why not to implement that flag setting up in the framework of the
> > "DW_SPI_CAP_DWC_SSI" capability? Thus we'd have all "snps,dwc-ssi-
> > 1.01a"-compatible devices and devices with the DW_SPI_CAP_DWC_SSI flag
> > set working well if for some reason they have got slave-mode enabled by
> > default.
> 

> Intel Keem Bay and Thunder Bay uses v1.02a version of DWC SSI controller. According to v1.02a, BIT31 of CTRLR0 is used for selecting Master or slave mode. In earlier versions, it was reserved. Both Keem Bay and Thunder Bay has to work in master mode, so this bit is set. The dwc_ssi controller can either function in master or slave (default) mode as per the spec. The bit31 requirement is only for Keem Bay and Thunder bay and other controllers can have a requirement to function in slave mode as well. Hence the bit is set only for Keem Bay/Thunder Bay. Please let me know if it should be set default to master mode.
> Wan Ahmed Zainie has posted that patch based on earlier version of the controller and later up streamed the DW_SPI_CAP_KEEMBAY_MST capability flag. This will become generic now.

I see. Thanks for clarification. IIUC BIT(31) is indeed specific to
all DWC SSI (not only Keem/Thunder Bay SPI IPs) and indeed determines
the Master/Slave mode of the controller. Then I don't really
understand why Wan Ahmed didn't make it set generically in CR0 for all
DWC SSI v1.01a instead of marking it as "intel,keembay-ssi"-specific
seeing he provided a generic "snps,dwc-ssi-1.01a" compatible code in
that same patchset.

That decision might have been caused by having different default
states of CTRLR0.31 bit in generic DWC SSI and Keem/Thunder Bay SSI...
Anyway I believe it won't hurt to set that bit for each DWC SSI
especially seeing the DW APB SSI driver doesn't support the SPI slave
mode at the moment. So please do that in a dedicated patch by converting
the DWC_SSI_CTRLR0_KEEMBAY_MST macro to a generic DWC_SSI_CTRLR0_MST and
applying it for CTRLR0.31 for each DW_SPI_CAP_DWC_SSI controller.

> > 
> > > +
> > > +		if (dws->caps & DW_SPI_CAP_DWC_SSTE)
> > > +			cr0 |= DWC_SSI_CTRLR0_SSTE;
> > 
> > Regarding SSTE flag and feature implemented behind it. First of all AFAICS
> > from the Wan Ahmad Zainie post sited above it is indeed generic for both
> > DWC SSI and DW APB SSI IP-cores of the controllers. Thus we don't need an
> > additional DWC SSI capability flag defined for it, but need to have it
> > generically implemented in the DW SPI core driver.
> > Secondly as you said it two weeks ago it defines a slave-specific protocol, the
> > way the SSI and CLK signals are driven between consecutive
> > frames:
> > >> SSTE (Slave Select Toggle Enable)
> > >> When SSTE bit is set to 1, the slave select line will toggle between
> > >> consecutive data frames, with the serial clock being held to its
> > >> default  value while slave select line is high.
> > >> When SSTE bit is set to 0, slave select line will stay low and clock
> > >> will  run continuously for the duration of the transfer.
> > In general DWC SSI/DW APB SSI controller can be connected to slave devices
> > with SSTE and normal communication protocol requirements at the same
> > time by using different CS-lanes. Therefore the SSTE feature turns to be
> > Slave/Peripheral-device specific rather than controller-specific and needs to
> > be enabled/disabled when it's required by a slave device.
> > 
> > Thus here is what I'd suggest to implement the SSTE feature generically:
> > 1) Add a new SPI-slave Synopsys-specific DT-property into the bindings file
> > like this:
> > --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> > +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> > @@ -143,6 +143,12 @@ patternProperties:
> >            is an optional feature of the designware controller, and the
> >            upper limit is also subject to controller configuration.
> > 
> > +      snps,sste:
> > +        description: Slave select line will toggle between consecutive
> > +          data frames, with the serial clock being held to its default
> > +          value while slave select line is high.
> > +        type: boolean
> > +
> >  unevaluatedProperties: false
> > 
> >  required:
> > 
> > Please do that in a separate preparation patch submitted before the
> > "dt-bindings: spi: Add bindings for Intel Thunder Bay SoC" patch in this
> > series.
> Sure, will modify SSTE as DT-property and do the necessary changes in both code and in DT.
> > 
> > 2) Add that property support into the driver like this:
> > diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c index
> > a305074c482e..5caa74b9aa74 100644
> > --- a/drivers/spi/spi-dw-core.c
> > +++ b/drivers/spi/spi-dw-core.c
> > @@ -27,6 +27,7 @@
> >  struct chip_data {
> >  	u32 cr0;
> >  	u32 rx_sample_dly;	/* RX sample delay */
> > +	bool sste;		/* Slave Select Toggle flag */
> >  };
> > 
> >  #ifdef CONFIG_DEBUG_FS
> > @@ -269,6 +270,7 @@ static irqreturn_t dw_spi_irq(int irq, void *dev_id)
> > 
> >  static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct spi_device *spi)  {
> > +	struct chip_data *chip = spi_get_ctldata(spi);
> >  	u32 cr0 = 0;
> > 
> >  	if (!(dws->caps & DW_SPI_CAP_DWC_SSI)) { @@ -285,6 +287,9 @@
> > static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct spi_device *spi)
> > 
> >  		/* CTRLR0[11] Shift Register Loop */
> >  		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) << SPI_SRL_OFFSET;
> > +
> > +		/* CTRLR0[24] Slave Select Toggle Enable */
> > +		cr0 |= chip->sste << SPI_SSTE_OFFSET;
> >  	} else {
> >  		/* CTRLR0[ 7: 6] Frame Format */
> >  		cr0 |= SSI_MOTO_SPI << DWC_SSI_CTRLR0_FRF_OFFSET; @@
> > -300,6 +305,9 @@ static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct
> > spi_device *spi)
> >  		/* CTRLR0[13] Shift Register Loop */
> >  		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) <<
> > DWC_SSI_CTRLR0_SRL_OFFSET;
> > 
> > +		/* CTRLR0[14] Slave Select Toggle Enable */
> > +		cr0 |= chip->sste << DWC_SSI_CTRLR0_SSTE_OFFSET;
> > +
> >  		if (dws->caps & DW_SPI_CAP_KEEMBAY_MST)
> >  			cr0 |= DWC_SSI_CTRLR0_KEEMBAY_MST;
> >  	}
> > @@ -789,6 +797,9 @@ static int dw_spi_setup(struct spi_device *spi)
> >  		chip->rx_sample_dly =
> > DIV_ROUND_CLOSEST(rx_sample_dly_ns,
> >  							NSEC_PER_SEC /
> >  							dws->max_freq);
> > +
> > +		/* Get slave select toggling feature requirement */
> > +		chip->sste = device_property_read_bool(&spi->dev,
> > "snps,sste");
> >  	}
> > 
> >  	/*
> > diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h index
> > b665e040862c..2ee3f839de39 100644
> > --- a/drivers/spi/spi-dw.h
> > +++ b/drivers/spi/spi-dw.h
> > @@ -65,8 +65,10 @@
> >  #define SPI_SLVOE_OFFSET		10
> >  #define SPI_SRL_OFFSET			11
> >  #define SPI_CFS_OFFSET			12
> > +#define SPI_SSTE_OFFSET			24
> > 
> >  /* Bit fields in CTRLR0 based on DWC_ssi_databook.pdf v1.01a */
> > +#define DWC_SSI_CTRLR0_SSTE_OFFSET	14
> >  #define DWC_SSI_CTRLR0_SRL_OFFSET	13
> >  #define DWC_SSI_CTRLR0_TMOD_OFFSET	10
> >  #define DWC_SSI_CTRLR0_TMOD_MASK	GENMASK(11, 10)
> > 
> > Please also do that in a separate preparation patch.
> > 
> > 3) If MST BIT(31) feature is generic, then please discard the
> > DW_SPI_CAP_KEEMBAY_MST capability flag and set the MST bit for each
> > DWC SSI device with DW_SPI_CAP_DWC_SSI capability set. If it's Intel-
> > specific, then convert the DW_SPI_CAP_KEEMBAY_MST capability macro
> > name to DW_SPI_CAP_INTEL_MST.
> > 
> > Please also do that in a separate preparation patch.

> The feature is for the controller version v1.02a and above. The controller can function on master or slave mode, default being slave mode. So, it is modified to master only in Keem bay and Thunder bay. 
> The difference between v1.01a and v1.02a w.r.t CTRLR0 is BIT31 selection of master/slave mode. Though the feature is generic but BIT31 is needed to be set only for bay, I will rename the macros to a generic name. 

Please, see my comment above. Let's set that bit for each DWC SSI
controller, so to have the driver protected from having the inverted
default state on any other vendor-specific controller.

> 
> > 
> > 4) After all of that you can add the "Thunder Bay SPI" controller support into
> > the DW SPI MMIO driver by placing the "intel,thunderbay-ssi" compatibility
> > string into the OF-device table.
> > Since both Thunder and Keembay SPIs are based on the same IP-core then
> > you can just reuse the dw_spi_keembay_init() for both of them after
> > renaming it to something like dw_spi_intel_init().
> > 

> Sure, will do the same.

Thanks.

Regards,
-Sergey

> 
> Regards,
> Nandhini
> > 
> > >  	}
> > >
> > >  	return cr0;
> > > diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> > > index 3379720cfcb8..2bd1dedd90b0 100644
> > > --- a/drivers/spi/spi-dw-mmio.c
> > > +++ b/drivers/spi/spi-dw-mmio.c
> > > @@ -217,7 +217,24 @@ 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 |
> > DW_SPI_CAP_DWC_SSI;
> > > +	/*
> > > +	 * Set MST to make keem bay SPI as master.
> > > +	 */
> > > +	dwsmmio->dws.caps = DW_SPI_CAP_DWC_MST |
> > DW_SPI_CAP_DWC_SSI;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int dw_spi_thunderbay_init(struct platform_device *pdev,
> > > +				  struct dw_spi_mmio *dwsmmio)
> > > +{
> > > +	/*
> > > +	 * Set MST to make thunder bay SPI as master.
> > > +	 * Set SSTE to enable slave select toggle bit which is required
> > > +	 * for the slave devices connected to the thunder bay SPI controller.
> > > +	 */
> > > +	dwsmmio->dws.caps = DW_SPI_CAP_DWC_MST |
> > DW_SPI_CAP_DWC_SSTE |
> > > +			    DW_SPI_CAP_DWC_SSI;
> > >
> > >  	return 0;
> > >  }
> > > @@ -349,6 +366,7 @@ static const struct of_device_id
> > dw_spi_mmio_of_match[] = {
> > >  	{ .compatible = "renesas,rzn1-spi", .data = dw_spi_dw_apb_init},
> > >  	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
> > >  	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
> > > +	{ .compatible = "intel,thunderbay-ssi", .data =
> > > +dw_spi_thunderbay_init},
> > >  	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
> > >  	{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
> > >  	{ /* end of table */}
> > > diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h index
> > > b665e040862c..9fffe0a02f3a 100644
> > > --- a/drivers/spi/spi-dw.h
> > > +++ b/drivers/spi/spi-dw.h
> > > @@ -76,11 +76,16 @@
> > >  #define DWC_SSI_CTRLR0_DFS_OFFSET	0
> > >
> > >  /*
> > > - * For Keem Bay, CTRLR0[31] is used to select controller mode.
> > > + * CTRLR0[31] is used to select controller mode.
> > >   * 0: SSI is slave
> > >   * 1: SSI is master
> > >   */
> > > -#define DWC_SSI_CTRLR0_KEEMBAY_MST	BIT(31)
> > > +#define DWC_SSI_CTRLR0_MST		BIT(31)
> > > +
> > > +/*
> > > + * CTRLR0[14] is used to enable/disable Slave Select Toggle bit  */
> > > +#define DWC_SSI_CTRLR0_SSTE		BIT(14)
> > >
> > >  /* Bit fields in CTRLR1 */
> > >  #define SPI_NDF_MASK			GENMASK(15, 0)
> > > @@ -122,9 +127,10 @@ 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_MST		BIT(1)
> > >  #define DW_SPI_CAP_DWC_SSI		BIT(2)
> > >  #define DW_SPI_CAP_DFS32		BIT(3)
> > > +#define DW_SPI_CAP_DWC_SSTE		BIT(4)
> > >
> > >  /* Slave spi_transfer/spi_mem_op related */  struct dw_spi_cfg {
> > > --
> > > 2.17.1
> > >