| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20210722170435.y6fla7ixfgzwkje2@mobilestation>
Date: Thu, 22 Jul 2021 20:04:35 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: nandhini.srikandan@...el.com
Cc: broonie@...nel.org, robh+dt@...nel.org, linux-spi@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
mgross@...ux.intel.com, kris.pan@...el.com,
kenchappa.demakkanavar@...el.com, furong.zhou@...el.com,
mallikarjunappa.sangannavar@...el.com, mahesh.r.vaidya@...el.com,
rashmi.a@...el.com
Subject: Re: [“PATCH” 2/2] spi: dw: Add
support for Intel Thunder Bay SPI
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.
> +
> + 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
>
Powered by blists - more mailing lists