RE: [“PATCH” 2/2] spi: dw: Add support for Intel Thunder Bay SPI

["Srikandan, Nandhini" <nandhini.srikandan@xxxxxxxxx>]

> -----Original Message-----
> From: Serge Semin <fancer.lancer@xxxxxxxxx>
> Sent: Friday, July 23, 2021 8:03 AM
> To: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Cc: Srikandan, Nandhini <nandhini.srikandan@xxxxxxxxx>;
> 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” 2/2] spi: dw: Add support for Intel Thunder Bay SPI
> 
> On Fri, Jul 23, 2021 at 01:31:07AM +0300, Andy Shevchenko wrote:
> > On Thursday, July 22, 2021, Serge Semin <fancer.lancer@xxxxxxxxx>
> wrote:
> >
> > > On Thu, Jul 22, 2021 at 01:33:58PM +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 added 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.
> > >
> > > Could you elaborate what this bit mean?
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. 

This bit is needed for SPI-slave devices like TPM used in Thunder Bay, which needs SSTE bit to be set in order to work. 
Hence SSTE is enabled only for Thunder Bay.

- Nandhini
> > >
> > > > Added reset of SPI controller required for Thunder Bay.
> > >
> > > If it's really required (is it?) then you were supposed to reflect
> > > that in the code by returning a negative error if the driver fails
> > > to retrieve the reset control handler. In accordance with that the
> > > bindings should have been also updated so the dtbs_check procedure
> > > would make sure the Thunder Bay SPI DT-node comply to the
> > > requirements in that matter.
> > >
The existing reset code in spi-dw-mmio.c file will be reused to de-assert the SPI core by passing reset-names = "spi" in DTB. So the reset code added here will be removed.

- Nandhini

> > > Anyway I've got a few comments regarding this part of your patch.
> > > Please see them below.
> > >
> > > >
> > > > Signed-off-by: Nandhini Srikandan <nandhini.srikandan@xxxxxxxxx>
> > > > ---
> > > >  drivers/spi/spi-dw-core.c |  6 ++++++  drivers/spi/spi-dw-mmio.c
> > > > | 20 ++++++++++++++++++++
> > > >  drivers/spi/spi-dw.h      | 15 +++++++++++++++
> > > >  3 files changed, 41 insertions(+)
> > > >
> > > > diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> > > > index a305074c482e..eecf8dcd0677 100644
> > > > --- a/drivers/spi/spi-dw-core.c
> > > > +++ b/drivers/spi/spi-dw-core.c
> > > > @@ -302,6 +302,12 @@ static u32 dw_spi_prepare_cr0(struct dw_spi
> > > > *dws,
> > > struct spi_device *spi)
> > > >
> > > >               if (dws->caps & DW_SPI_CAP_KEEMBAY_MST)
> > > >                       cr0 |= DWC_SSI_CTRLR0_KEEMBAY_MST;
> > > > +
> > >
> > > > +             if (dws->caps & DW_SPI_CAP_THUNDERBAY_MST)
> > > > +                     cr0 |= DWC_SSI_CTRLR0_THUNDERBAY_MST;
> > >
> > > I guess that KeemBay and ThunderBay SPI controllers have been
> > > synthesized based on the same IP-core with a few differences. Is
> > > that true? Could you tell us what is the difference between them?
> > >
> > > Anyway regarding this the Master/Slave part. Is the ThunderBay
> > > implementation of the Master/Slave capability the same as it was
> > > embedded in the KeemBay controller? If so then what do you think
> > > about just renaming DW_SPI_CAP_KEEMBAY_MST to something like
> > > DW_SPI_CAP_INTEL_MST and using it then for both Keembay and
> > > ThunderBay versions of the SPI-controllers? (The similar renaming
> > > needs to be provided for the DWC_SSI_CTRLR0_KEEMBAY_MST macro
> then.)
> > > You can implement it as a preparation patch posted before this one
> > > in the series.
> >
> >
> 
Yes, they are synthesized based on Designware SPI IP core. 
The master/slave capability is same for both Keem Bay and Thunder Bay. 
Hence I will rename the macros and make the code generic and it will be part of upcoming patch.

- Nandhini

> >
> > Please, if you go this way add some more specific definition, b/c this
> > IP is being used on other intel SoCs which have nothing to do with these
> two.
> >
> 
> Does it have the same Master/Slave capability? If it does then we can stick
> with suggested name like DW_SPI_CAP_INTEL_MST, which could be
> perceived as "Intel-specific MST capability implemented for DW SPI".
> If it doesn't then does it have another type of the Master/Slave capability? If
> it does, then indeed we need to think on a better naming here. What name
> would you suggest in that case?
> 
> -Sergey
> 
Since both Keem Bay and Thunder Bay has same Master/Slave capability, I will keep the macro names generic.

- Nandhini 

> >
> > >
> > > > +
> > > > +             if (dws->caps & DW_SPI_CAP_THUNDERBAY_SSTE)
> > > > +                     cr0 |= DWC_SSI_CTRLR0_THUNDERBAY_SSTE;
> > >
> > > Similar question regarding the SSTE bit. Is it something ThunderBay
> > > specific only? Was the corresponding functionality embedded into the
> > > KeemBay or any other Intel version of the DW SPI controller?
> > >
SSTE bit is needed only for slave devices like TPM which are connected on Thunder Bay. 
There is no such slaves with this SSTE requirement on Keem Bay.

- Nandhini

> > > >       }
> > > >
> > > >       return cr0;
> > > > diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> > > > index 3379720cfcb8..ca9aad078752 100644
> > > > --- a/drivers/spi/spi-dw-mmio.c
> > > > +++ b/drivers/spi/spi-dw-mmio.c
> > > > @@ -222,6 +222,15 @@ static int dw_spi_keembay_init(struct
> > > platform_device *pdev,
> > > >       return 0;
> > > >  }
> > > >
> > > > +static int dw_spi_thunderbay_init(struct platform_device *pdev,
> > > > +                               struct dw_spi_mmio *dwsmmio) {
> > >
> > > > +     dwsmmio->dws.caps = DW_SPI_CAP_THUNDERBAY_MST |
> > > DW_SPI_CAP_THUNDERBAY_RST |
> > > > +                         DW_SPI_CAP_THUNDERBAY_SSTE |
> > > DW_SPI_CAP_DWC_SSI;
> > > > +
> > >
> > > Originally the DW_SPI_CAP-functionality was provided to modify the
> > > DW SPI core driver behavior when it was required. For instance it
> > > was mostly connected with the platform-specific CR0-register
> > > configurations. So as I see it the reset part can be successfully
> > > handled fully in the framework of the MMIO-platform glue-driver.
> > > Instead of defining a new capability you could have just added the
> > > next code in the ThunderBay init-method:
> > >
> > > +       if (!dwsmmio->rstc) {
> > > +               dev_err(&pdev->dev, "Reset control is missing\n");
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       reset_control_assert(dwsmmio->rstc);
> > > +       udelay(2);
> > > +       reset_control_deassert(dwsmmio->rstc);
> > > +
> > >
> > > Thus you'd reuse the already implemented reset-controller handler
> > > defined in the dw_spi_mmio structure with no need of implementing a
> > > new capability.
> > >
The existing reset code in spi-dw-mmio.c will be reused. 
So, this portion of the code will be removed in upcoming patch.

-Nandhini

> > > > +     return 0;
> > > > +}
> > > > +
> > > >  static int dw_spi_canaan_k210_init(struct platform_device *pdev,
> > > >                                  struct dw_spi_mmio *dwsmmio)  {
> > > > @@ -243,6 +252,7 @@ static int dw_spi_mmio_probe(struct
> > > > platform_device
> > > *pdev)
> > > >                        struct dw_spi_mmio *dwsmmio);
> > > >       struct dw_spi_mmio *dwsmmio;
> > > >       struct resource *mem;
> > > > +     struct reset_control *rst;
> > > >       struct dw_spi *dws;
> > > >       int ret;
> > > >       int num_cs;
> > > > @@ -309,6 +319,15 @@ static int dw_spi_mmio_probe(struct
> > > > platform_device
> > > *pdev)
> > > >                       goto out;
> > > >       }
> > > >
> > >
> > > > +     if (dws->caps & DW_SPI_CAP_THUNDERBAY_RST) {
> > > > +             rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> > > > +             if (!IS_ERR(rst)) {
> > > > +                     reset_control_assert(rst);
> > > > +                     udelay(2);
> > > > +                     reset_control_deassert(rst);
> > > > +             }
> > > > +     }
> > > > +
> > >
> > > Please see my comment above. We don't need to have this code here if
> > > you get to implement what I suggest there.
> > >

Since, existing reset code will be reused, this reset code will be removed as mentioned earlier.

- Nandhini

> > > >       pm_runtime_enable(&pdev->dev);
> > > >
> > > >       ret = dw_spi_add_host(&pdev->dev, dws); @@ -349,6 +368,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..bfe1d5edc25a 100644
> > > > --- a/drivers/spi/spi-dw.h
> > > > +++ b/drivers/spi/spi-dw.h
> > > > @@ -82,6 +82,18 @@
> > > >   */
> > > >  #define DWC_SSI_CTRLR0_KEEMBAY_MST   BIT(31)
> > > >
> > >
> > > > +/*
> > > > + * For Thunder Bay, CTRLR0[14] should be set to 1.
> > > > + */
> > >
> > > Could you provide a bit more details what this bit has been
> > > implemented for?
> > >
> > > > +#define DWC_SSI_CTRLR0_THUNDERBAY_SSTE       BIT(14)
> > > > +
> > >
> > > > +/*
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.

> > > > + * For Thunder Bay, CTRLR0[31] is used to select controller mode.
> > > > + * 0: SSI is slave
> > > > + * 1: SSI is master
> > > > + */
> > > > +#define DWC_SSI_CTRLR0_THUNDERBAY_MST        BIT(31)
> > >
> > > Please see my suggestion regarding the Master/Slave capability in
> > > one of the comments above.
> > >
Sure, this will be renamed in a generic way.

- Nandhini

> > > Regards
> > > -Serge
> > >
> > > > +
> > > >  /* Bit fields in CTRLR1 */
> > > >  #define SPI_NDF_MASK                 GENMASK(15, 0)
> > > >
> > > > @@ -125,6 +137,9 @@ enum dw_ssi_type {
> > > >  #define DW_SPI_CAP_KEEMBAY_MST               BIT(1)
> > > >  #define DW_SPI_CAP_DWC_SSI           BIT(2)
> > > >  #define DW_SPI_CAP_DFS32             BIT(3)
> > > > +#define DW_SPI_CAP_THUNDERBAY_MST    BIT(4)
> > > > +#define DW_SPI_CAP_THUNDERBAY_RST    BIT(5)
> > > > +#define DW_SPI_CAP_THUNDERBAY_SSTE   BIT(6)
> > > >
> > > >  /* Slave spi_transfer/spi_mem_op related */  struct dw_spi_cfg {
> > > > --
> > > > 2.17.1
> > > >
> > >
> >
> >
> > --
> > With Best Regards,
> > Andy Shevchenko

Regards,
Nandhini