> -----Original Message----- > From: Serge Semin <fancer.lancer@xxxxxxxxx> > Sent: Wednesday, September 8, 2021 2:59 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 > > 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. > Sure, I will make the macro to a generic one and include it in the upcoming patchset. > > > > > > > + > > > > + 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. > Sure, I will set the bit for each DWC SSI controller. Regard, Nandhini > > > > > > > > 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 > > > >
