* [“PATCH” 0/2] Add support for Intel Thunder Bay SPI
@ 2021-07-22 5:33 nandhini.srikandan
2021-07-22 5:33 ` [“PATCH” 1/2] dt-bindings: spi: Add bindings for Intel Thunder Bay SoC nandhini.srikandan
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: nandhini.srikandan @ 2021-07-22 5:33 UTC (permalink / raw)
To: fancer.lancer, broonie, robh+dt, linux-spi, linux-kernel
Cc: devicetree, mgross, kris.pan, kenchappa.demakkanavar,
furong.zhou, mallikarjunappa.sangannavar, mahesh.r.vaidya,
nandhini.srikandan, rashmi.a
From: Nandhini Srikandan <nandhini.srikandan@intel.com>
Hi,
This patch set enables the support for Designware SPI on the Intel Thunder Bay SoC.
Patch 1: SPI DT bindings for Intel Thunder Bay SoC
Patch 2: Adds support for Designware SPI on Intel Thunderbay SoC
Please help to review this patch set.
Thanks & Regards,
Nandhini
Nandhini Srikandan (2):
dt-bindings: spi: Add bindings for Intel Thunder Bay SoC
spi: dw: Add support for Intel Thunder Bay SPI
.../bindings/spi/snps,dw-apb-ssi.yaml | 2 ++
drivers/spi/spi-dw-core.c | 6 ++++++
drivers/spi/spi-dw-mmio.c | 20 +++++++++++++++++++
drivers/spi/spi-dw.h | 15 ++++++++++++++
4 files changed, 43 insertions(+)
--
2.17.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [“PATCH” 1/2] dt-bindings: spi: Add bindings for Intel Thunder Bay SoC 2021-07-22 5:33 [“PATCH” 0/2] Add support for Intel Thunder Bay SPI nandhini.srikandan @ 2021-07-22 5:33 ` nandhini.srikandan 2021-07-22 17:11 ` Serge Semin 2021-07-22 5:33 ` [“PATCH” 2/2] spi: dw: Add support for Intel Thunder Bay SPI nandhini.srikandan ` (2 subsequent siblings) 3 siblings, 1 reply; 12+ messages in thread From: nandhini.srikandan @ 2021-07-22 5:33 UTC (permalink / raw) To: fancer.lancer, broonie, robh+dt, linux-spi, linux-kernel Cc: devicetree, mgross, kris.pan, kenchappa.demakkanavar, furong.zhou, mallikarjunappa.sangannavar, mahesh.r.vaidya, nandhini.srikandan, rashmi.a From: Nandhini Srikandan <nandhini.srikandan@intel.com> Add documentation for SPI controller in Intel Thunder Bay SoC. Signed-off-by: Nandhini Srikandan <nandhini.srikandan@intel.com> --- Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml index ca91201a9926..88532bf8ba85 100644 --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml @@ -61,6 +61,8 @@ properties: - const: snps,dw-apb-ssi - description: Intel Keem Bay SPI Controller const: intel,keembay-ssi + - description: Intel Thunder Bay SPI Controller + const: intel,thunderbay-ssi - description: Baikal-T1 SPI Controller const: baikal,bt1-ssi - description: Baikal-T1 System Boot SPI Controller -- 2.17.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [“PATCH” 1/2] dt-bindings: spi: Add bindings for Intel Thunder Bay SoC 2021-07-22 5:33 ` [“PATCH” 1/2] dt-bindings: spi: Add bindings for Intel Thunder Bay SoC nandhini.srikandan @ 2021-07-22 17:11 ` Serge Semin 0 siblings, 0 replies; 12+ messages in thread From: Serge Semin @ 2021-07-22 17:11 UTC (permalink / raw) To: nandhini.srikandan Cc: broonie, robh+dt, linux-spi, linux-kernel, devicetree, mgross, kris.pan, kenchappa.demakkanavar, furong.zhou, mallikarjunappa.sangannavar, mahesh.r.vaidya, rashmi.a On Thu, Jul 22, 2021 at 01:33:57PM +0800, nandhini.srikandan@intel.com wrote: > From: Nandhini Srikandan <nandhini.srikandan@intel.com> > > Add documentation for SPI controller in Intel Thunder Bay SoC. In the driver-part of the patchset you said that the reset control is required for this controller. Then the bindings file needs to be accordingly altered. See the way it's done in the "allOf:" block here. If it isn't required then this part looks ok to me. Regards, -Sergey > > Signed-off-by: Nandhini Srikandan <nandhini.srikandan@intel.com> > --- > Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml > index ca91201a9926..88532bf8ba85 100644 > --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml > +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml > @@ -61,6 +61,8 @@ properties: > - const: snps,dw-apb-ssi > - description: Intel Keem Bay SPI Controller > const: intel,keembay-ssi > + - description: Intel Thunder Bay SPI Controller > + const: intel,thunderbay-ssi > - description: Baikal-T1 SPI Controller > const: baikal,bt1-ssi > - description: Baikal-T1 System Boot SPI Controller > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [“PATCH” 2/2] spi: dw: Add support for Intel Thunder Bay SPI 2021-07-22 5:33 [“PATCH” 0/2] Add support for Intel Thunder Bay SPI nandhini.srikandan 2021-07-22 5:33 ` [“PATCH” 1/2] dt-bindings: spi: Add bindings for Intel Thunder Bay SoC nandhini.srikandan @ 2021-07-22 5:33 ` nandhini.srikandan 2021-07-22 17:04 ` Serge Semin 2021-07-22 16:09 ` [“PATCH” 0/2] " Serge Semin 2021-07-22 17:07 ` Serge Semin 3 siblings, 1 reply; 12+ messages in thread From: nandhini.srikandan @ 2021-07-22 5:33 UTC (permalink / raw) To: fancer.lancer, broonie, robh+dt, linux-spi, linux-kernel Cc: devicetree, mgross, kris.pan, kenchappa.demakkanavar, furong.zhou, mallikarjunappa.sangannavar, mahesh.r.vaidya, nandhini.srikandan, rashmi.a From: Nandhini Srikandan <nandhini.srikandan@intel.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. Added reset of SPI controller required for Thunder Bay. Signed-off-by: Nandhini Srikandan <nandhini.srikandan@intel.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; + + if (dws->caps & DW_SPI_CAP_THUNDERBAY_SSTE) + cr0 |= DWC_SSI_CTRLR0_THUNDERBAY_SSTE; } 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; + + 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); + } + } + 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. + */ +#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) + /* 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [“PATCH” 2/2] spi: dw: Add support for Intel Thunder Bay SPI 2021-07-22 5:33 ` [“PATCH” 2/2] spi: dw: Add support for Intel Thunder Bay SPI nandhini.srikandan @ 2021-07-22 17:04 ` Serge Semin 2021-07-22 18:26 ` Serge Semin [not found] ` <CAHp75VeQ0Q174gUww0oqw9MmmE76AGMxcNLj4UkgjLZzhGy6Mw@mail.gmail.com> 0 siblings, 2 replies; 12+ messages in thread From: Serge Semin @ 2021-07-22 17:04 UTC (permalink / raw) To: nandhini.srikandan Cc: broonie, robh+dt, linux-spi, linux-kernel, devicetree, mgross, kris.pan, kenchappa.demakkanavar, furong.zhou, mallikarjunappa.sangannavar, mahesh.r.vaidya, rashmi.a On Thu, Jul 22, 2021 at 01:33:58PM +0800, nandhini.srikandan@intel.com wrote: > From: Nandhini Srikandan <nandhini.srikandan@intel.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@intel.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. > + > + 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [“PATCH” 2/2] spi: dw: Add support for Intel Thunder Bay SPI 2021-07-22 17:04 ` Serge Semin @ 2021-07-22 18:26 ` Serge Semin [not found] ` <CAHp75VeQ0Q174gUww0oqw9MmmE76AGMxcNLj4UkgjLZzhGy6Mw@mail.gmail.com> 1 sibling, 0 replies; 12+ messages in thread From: Serge Semin @ 2021-07-22 18:26 UTC (permalink / raw) To: nandhini.srikandan Cc: broonie, robh+dt, linux-spi, linux-kernel, devicetree, mgross, kris.pan, kenchappa.demakkanavar, furong.zhou, mallikarjunappa.sangannavar, mahesh.r.vaidya, rashmi.a One more nitpick below. On Thu, Jul 22, 2021 at 08:04:35PM +0300, Serge Semin wrote: > On Thu, Jul 22, 2021 at 01:33:58PM +0800, nandhini.srikandan@intel.com wrote: > > From: Nandhini Srikandan <nandhini.srikandan@intel.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@intel.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. > > > + > > + 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); Please, don't forget to add a header file with udelay() declaration to this module. -Sergey > + 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <CAHp75VeQ0Q174gUww0oqw9MmmE76AGMxcNLj4UkgjLZzhGy6Mw@mail.gmail.com>]
* Re: [“PATCH” 2/2] spi: dw: Add support for Intel Thunder Bay SPI [not found] ` <CAHp75VeQ0Q174gUww0oqw9MmmE76AGMxcNLj4UkgjLZzhGy6Mw@mail.gmail.com> @ 2021-07-23 2:33 ` Serge Semin 2021-08-11 6:15 ` Srikandan, Nandhini 0 siblings, 1 reply; 12+ messages in thread From: Serge Semin @ 2021-07-23 2:33 UTC (permalink / raw) To: Andy Shevchenko Cc: nandhini.srikandan, broonie, robh+dt, linux-spi, linux-kernel, devicetree, mgross, kris.pan, kenchappa.demakkanavar, furong.zhou, mallikarjunappa.sangannavar, mahesh.r.vaidya, rashmi.a On Fri, Jul 23, 2021 at 01:31:07AM +0300, Andy Shevchenko wrote: > On Thursday, July 22, 2021, Serge Semin <fancer.lancer@gmail.com> wrote: > > > On Thu, Jul 22, 2021 at 01:33:58PM +0800, nandhini.srikandan@intel.com > > wrote: > > > From: Nandhini Srikandan <nandhini.srikandan@intel.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@intel.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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [“PATCH” 2/2] spi: dw: Add support for Intel Thunder Bay SPI 2021-07-23 2:33 ` Serge Semin @ 2021-08-11 6:15 ` Srikandan, Nandhini 0 siblings, 0 replies; 12+ messages in thread From: Srikandan, Nandhini @ 2021-08-11 6:15 UTC (permalink / raw) To: Serge Semin, Andy Shevchenko Cc: broonie, robh+dt, linux-spi, linux-kernel, devicetree, mgross, Pan, Kris, Demakkanavar, Kenchappa, Zhou, Furong, Sangannavar, Mallikarjunappa, Vaidya, Mahesh R, A, Rashmi > -----Original Message----- > From: Serge Semin <fancer.lancer@gmail.com> > Sent: Friday, July 23, 2021 8:03 AM > To: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Srikandan, Nandhini <nandhini.srikandan@intel.com>; > broonie@kernel.org; robh+dt@kernel.org; linux-spi@vger.kernel.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; > mgross@linux.intel.com; Pan, Kris <kris.pan@intel.com>; Demakkanavar, > Kenchappa <kenchappa.demakkanavar@intel.com>; Zhou, Furong > <furong.zhou@intel.com>; Sangannavar, Mallikarjunappa > <mallikarjunappa.sangannavar@intel.com>; Vaidya, Mahesh R > <mahesh.r.vaidya@intel.com>; A, Rashmi <rashmi.a@intel.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@gmail.com> > wrote: > > > > > On Thu, Jul 22, 2021 at 01:33:58PM +0800, > > > nandhini.srikandan@intel.com > > > wrote: > > > > From: Nandhini Srikandan <nandhini.srikandan@intel.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? 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@intel.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. > > > > > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [“PATCH” 0/2] Add support for Intel Thunder Bay SPI 2021-07-22 5:33 [“PATCH” 0/2] Add support for Intel Thunder Bay SPI nandhini.srikandan 2021-07-22 5:33 ` [“PATCH” 1/2] dt-bindings: spi: Add bindings for Intel Thunder Bay SoC nandhini.srikandan 2021-07-22 5:33 ` [“PATCH” 2/2] spi: dw: Add support for Intel Thunder Bay SPI nandhini.srikandan @ 2021-07-22 16:09 ` Serge Semin 2021-08-10 9:24 ` Srikandan, Nandhini 2021-07-22 17:07 ` Serge Semin 3 siblings, 1 reply; 12+ messages in thread From: Serge Semin @ 2021-07-22 16:09 UTC (permalink / raw) To: nandhini.srikandan Cc: broonie, robh+dt, linux-spi, linux-kernel, devicetree, mgross, kris.pan, kenchappa.demakkanavar, furong.zhou, mallikarjunappa.sangannavar, mahesh.r.vaidya, rashmi.a Hello Nandhini On Thu, Jul 22, 2021 at 01:33:56PM +0800, nandhini.srikandan@intel.com wrote: > From: Nandhini Srikandan <nandhini.srikandan@intel.com> > > Hi, > > This patch set enables the support for Designware SPI on the Intel Thunder Bay SoC. > > Patch 1: SPI DT bindings for Intel Thunder Bay SoC > Patch 2: Adds support for Designware SPI on Intel Thunderbay SoC > > Please help to review this patch set. Thanks for the patchset. I'll send you my comments to the corresponding patches soon. Regards, -Sergey > > Thanks & Regards, > Nandhini > > Nandhini Srikandan (2): > dt-bindings: spi: Add bindings for Intel Thunder Bay SoC > spi: dw: Add support for Intel Thunder Bay SPI > > .../bindings/spi/snps,dw-apb-ssi.yaml | 2 ++ > drivers/spi/spi-dw-core.c | 6 ++++++ > drivers/spi/spi-dw-mmio.c | 20 +++++++++++++++++++ > drivers/spi/spi-dw.h | 15 ++++++++++++++ > 4 files changed, 43 insertions(+) > > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [“PATCH” 0/2] Add support for Intel Thunder Bay SPI 2021-07-22 16:09 ` [“PATCH” 0/2] " Serge Semin @ 2021-08-10 9:24 ` Srikandan, Nandhini 2021-08-10 9:52 ` Serge Semin 0 siblings, 1 reply; 12+ messages in thread From: Srikandan, Nandhini @ 2021-08-10 9:24 UTC (permalink / raw) To: Serge Semin Cc: broonie, robh+dt, linux-spi, linux-kernel, devicetree, mgross, Pan, Kris, Demakkanavar, Kenchappa, Zhou, Furong, Sangannavar, Mallikarjunappa, Vaidya, Mahesh R, A, Rashmi > -----Original Message----- > From: Serge Semin <fancer.lancer@gmail.com> > Sent: Thursday, July 22, 2021 9:39 PM > To: Srikandan, Nandhini <nandhini.srikandan@intel.com> > Cc: broonie@kernel.org; robh+dt@kernel.org; linux-spi@vger.kernel.org; > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; > mgross@linux.intel.com; Pan, Kris <kris.pan@intel.com>; Demakkanavar, > Kenchappa <kenchappa.demakkanavar@intel.com>; Zhou, Furong > <furong.zhou@intel.com>; Sangannavar, Mallikarjunappa > <mallikarjunappa.sangannavar@intel.com>; Vaidya, Mahesh R > <mahesh.r.vaidya@intel.com>; A, Rashmi <rashmi.a@intel.com> > Subject: Re: [“PATCH” 0/2] Add support for Intel Thunder Bay SPI > > Hello Nandhini > > On Thu, Jul 22, 2021 at 01:33:56PM +0800, nandhini.srikandan@intel.com > wrote: > > From: Nandhini Srikandan <nandhini.srikandan@intel.com> > > > > Hi, > > > > This patch set enables the support for Designware SPI on the Intel Thunder > Bay SoC. > > > > Patch 1: SPI DT bindings for Intel Thunder Bay SoC Patch 2: Adds > > support for Designware SPI on Intel Thunderbay SoC > > > > Please help to review this patch set. > > Thanks for the patchset. I'll send you my comments to the corresponding > patches soon. > Thank you for your review comments. I am working on it and I will share another patchset shortly. Regards, Nandhini > > > > Thanks & Regards, > > Nandhini > > > > Nandhini Srikandan (2): > > dt-bindings: spi: Add bindings for Intel Thunder Bay SoC > > spi: dw: Add support for Intel Thunder Bay SPI > > > > .../bindings/spi/snps,dw-apb-ssi.yaml | 2 ++ > > drivers/spi/spi-dw-core.c | 6 ++++++ > > drivers/spi/spi-dw-mmio.c | 20 +++++++++++++++++++ > > drivers/spi/spi-dw.h | 15 ++++++++++++++ > > 4 files changed, 43 insertions(+) > > > > -- > > 2.17.1 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [“PATCH” 0/2] Add support for Intel Thunder Bay SPI 2021-08-10 9:24 ` Srikandan, Nandhini @ 2021-08-10 9:52 ` Serge Semin 0 siblings, 0 replies; 12+ messages in thread From: Serge Semin @ 2021-08-10 9:52 UTC (permalink / raw) To: Srikandan, Nandhini Cc: Serge Semin, broonie, robh+dt, linux-spi, linux-kernel, devicetree, mgross, Pan, Kris, Demakkanavar, Kenchappa, Zhou, Furong, Sangannavar, Mallikarjunappa, Vaidya, Mahesh R, A, Rashmi On Tue, Aug 10, 2021 at 09:24:08AM +0000, Srikandan, Nandhini wrote: > > > > -----Original Message----- > > From: Serge Semin <fancer.lancer@gmail.com> > > Sent: Thursday, July 22, 2021 9:39 PM > > To: Srikandan, Nandhini <nandhini.srikandan@intel.com> > > Cc: broonie@kernel.org; robh+dt@kernel.org; linux-spi@vger.kernel.org; > > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; > > mgross@linux.intel.com; Pan, Kris <kris.pan@intel.com>; Demakkanavar, > > Kenchappa <kenchappa.demakkanavar@intel.com>; Zhou, Furong > > <furong.zhou@intel.com>; Sangannavar, Mallikarjunappa > > <mallikarjunappa.sangannavar@intel.com>; Vaidya, Mahesh R > > <mahesh.r.vaidya@intel.com>; A, Rashmi <rashmi.a@intel.com> > > Subject: Re: [“PATCH” 0/2] Add support for Intel Thunder Bay SPI > > > > Hello Nandhini > > > > On Thu, Jul 22, 2021 at 01:33:56PM +0800, nandhini.srikandan@intel.com > > wrote: > > > From: Nandhini Srikandan <nandhini.srikandan@intel.com> > > > > > > Hi, > > > > > > This patch set enables the support for Designware SPI on the Intel Thunder > > Bay SoC. > > > > > > Patch 1: SPI DT bindings for Intel Thunder Bay SoC Patch 2: Adds > > > support for Designware SPI on Intel Thunderbay SoC > > > > > > Please help to review this patch set. > > > > Thanks for the patchset. I'll send you my comments to the corresponding > > patches soon. > > > Thank you for your review comments. I am working on it and I will share another patchset shortly. I'd suggest to answer on the review questions first... -Sergey > > Regards, > Nandhini > > > > > > > Thanks & Regards, > > > Nandhini > > > > > > Nandhini Srikandan (2): > > > dt-bindings: spi: Add bindings for Intel Thunder Bay SoC > > > spi: dw: Add support for Intel Thunder Bay SPI > > > > > > .../bindings/spi/snps,dw-apb-ssi.yaml | 2 ++ > > > drivers/spi/spi-dw-core.c | 6 ++++++ > > > drivers/spi/spi-dw-mmio.c | 20 +++++++++++++++++++ > > > drivers/spi/spi-dw.h | 15 ++++++++++++++ > > > 4 files changed, 43 insertions(+) > > > > > > -- > > > 2.17.1 > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [“PATCH” 0/2] Add support for Intel Thunder Bay SPI 2021-07-22 5:33 [“PATCH” 0/2] Add support for Intel Thunder Bay SPI nandhini.srikandan ` (2 preceding siblings ...) 2021-07-22 16:09 ` [“PATCH” 0/2] " Serge Semin @ 2021-07-22 17:07 ` Serge Semin 3 siblings, 0 replies; 12+ messages in thread From: Serge Semin @ 2021-07-22 17:07 UTC (permalink / raw) To: nandhini.srikandan Cc: broonie, robh+dt, linux-spi, linux-kernel, devicetree, mgross, kris.pan, kenchappa.demakkanavar, furong.zhou, mallikarjunappa.sangannavar, mahesh.r.vaidya, rashmi.a BTW it's a first time I've met quotes around the PATCH word in the subject: [“PATCH” 0/2] Add support for Intel Thunder Bay SPI ^ ^ could you drop them? Regards, -Serge On Thu, Jul 22, 2021 at 01:33:56PM +0800, nandhini.srikandan@intel.com wrote: > From: Nandhini Srikandan <nandhini.srikandan@intel.com> > > Hi, > > This patch set enables the support for Designware SPI on the Intel Thunder Bay SoC. > > Patch 1: SPI DT bindings for Intel Thunder Bay SoC > Patch 2: Adds support for Designware SPI on Intel Thunderbay SoC > > Please help to review this patch set. > > Thanks & Regards, > Nandhini > > Nandhini Srikandan (2): > dt-bindings: spi: Add bindings for Intel Thunder Bay SoC > spi: dw: Add support for Intel Thunder Bay SPI > > .../bindings/spi/snps,dw-apb-ssi.yaml | 2 ++ > drivers/spi/spi-dw-core.c | 6 ++++++ > drivers/spi/spi-dw-mmio.c | 20 +++++++++++++++++++ > drivers/spi/spi-dw.h | 15 ++++++++++++++ > 4 files changed, 43 insertions(+) > > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-08-11 6:15 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-22 5:33 [“PATCH” 0/2] Add support for Intel Thunder Bay SPI nandhini.srikandan
2021-07-22 5:33 ` [“PATCH” 1/2] dt-bindings: spi: Add bindings for Intel Thunder Bay SoC nandhini.srikandan
2021-07-22 17:11 ` Serge Semin
2021-07-22 5:33 ` [“PATCH” 2/2] spi: dw: Add support for Intel Thunder Bay SPI nandhini.srikandan
2021-07-22 17:04 ` Serge Semin
2021-07-22 18:26 ` Serge Semin
[not found] ` <CAHp75VeQ0Q174gUww0oqw9MmmE76AGMxcNLj4UkgjLZzhGy6Mw@mail.gmail.com>
2021-07-23 2:33 ` Serge Semin
2021-08-11 6:15 ` Srikandan, Nandhini
2021-07-22 16:09 ` [“PATCH” 0/2] " Serge Semin
2021-08-10 9:24 ` Srikandan, Nandhini
2021-08-10 9:52 ` Serge Semin
2021-07-22 17:07 ` Serge Semin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).