Message-ID: <20210723023321.h63awes3kyigu7mx@mobilestation>
Date:   Fri, 23 Jul 2021 05:33:21 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     "nandhini.srikandan@...el.com" <nandhini.srikandan@...el.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "mgross@...ux.intel.com" <mgross@...ux.intel.com>,
        "kris.pan@...el.com" <kris.pan@...el.com>,
        "kenchappa.demakkanavar@...el.com" <kenchappa.demakkanavar@...el.com>,
        "furong.zhou@...el.com" <furong.zhou@...el.com>,
        "mallikarjunappa.sangannavar@...el.com" 
        <mallikarjunappa.sangannavar@...el.com>,
        "mahesh.r.vaidya@...el.com" <mahesh.r.vaidya@...el.com>,
        "rashmi.a@...el.com" <rashmi.a@...el.com>
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@...il.com> wrote:
> 
> > On Thu, Jul 22, 2021 at 01:33:58PM +0800, nandhini.srikandan@...el.com
> > wrote:
> > > From: Nandhini Srikandan <nandhini.srikandan@...el.com>
> > >
> > > 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?
> >
> > > 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.
> >
> > Anyway I've got a few comments regarding this part of your patch.
> > Please see them below.
> >
> > >
> > > Signed-off-by: Nandhini Srikandan <nandhini.srikandan@...el.com>
> > > ---
> > >  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.
> 
> 

> 
> 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

> 
> >
> > > +
> > > +             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?
> >
> > >       }
> > >
> > >       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.
> >
> > > +     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.
> >
> > >       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)
> > > +
> >
> > > +/*
> > > + * 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.
> >
> > 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

Powered by blists - more mailing lists