by Serge Semin

[permalink] [raw]

Subject: Re: [PATCH v11 10/15] spi: dw: Add support for AMD Pensando Elba SoC

On Sat, Mar 11, 2023 at 04:44:40PM -0800, Brad Larson wrote:
> The AMD Pensando Elba SoC includes a DW apb_ssi v4 controller
> with device specific chip-select control. The Elba SoC
> provides four chip-selects where the native DW IP supports
> two chip-selects. The Elba DW_SPI instance has two native
> CS signals that are always overridden.
>
> Signed-off-by: Brad Larson <[email protected]>
> ---
>
> v11 changes:
> - Simplify dw_spi_elb_init by using syscon_regmap_lookup_by_phandle()
>
> v10 changes:
> - Delete struct dw_spi_elba, use regmap directly in priv
>
> v9 changes:
> - Add use of macros GENMASK() and BIT()
> - Change ELBA_SPICS_SHIFT() to ELBA_SPICS_OFFSET()
>
> ---
> drivers/spi/spi-dw-mmio.c | 57 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
>
> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> index 26c40ea6dd12..5851ecc6e1e9 100644
> --- a/drivers/spi/spi-dw-mmio.c
> +++ b/drivers/spi/spi-dw-mmio.c
> @@ -53,6 +53,20 @@ struct dw_spi_mscc {
> void __iomem *spi_mst; /* Not sparx5 */
> };
>
> +/*
> + * Elba SoC does not use ssi, pin override is used for cs 0,1 and
> + * gpios for cs 2,3 as defined in the device tree.
> + *
> + * cs: | 1 0
> + * bit: |---3-------2-------1-------0
> + * | cs1 cs1_ovr cs0 cs0_ovr
> + */
> +#define ELBA_SPICS_REG 0x2468
> +#define ELBA_SPICS_OFFSET(cs) ((cs) << 1)
> +#define ELBA_SPICS_MASK(cs) (GENMASK(1, 0) << ELBA_SPICS_OFFSET(cs))
> +#define ELBA_SPICS_SET(cs, val) \
> + ((((val) << 1) | BIT(0)) << ELBA_SPICS_OFFSET(cs))
> +
> /*
> * The Designware SPI controller (referred to as master in the documentation)
> * automatically deasserts chip select when the tx fifo is empty. The chip
> @@ -237,6 +251,48 @@ static int dw_spi_canaan_k210_init(struct platform_device *pdev,
> return 0;
> }
>
> +static void dw_spi_elba_override_cs(struct regmap *syscon, int cs, int enable)
> +{
> + regmap_update_bits(syscon, ELBA_SPICS_REG, ELBA_SPICS_MASK(cs),
> + ELBA_SPICS_SET(cs, enable));
> +}
> +
> +static void dw_spi_elba_set_cs(struct spi_device *spi, bool enable)
> +{
> + struct dw_spi *dws = spi_master_get_devdata(spi->master);
> + struct dw_spi_mmio *dwsmmio = container_of(dws, struct dw_spi_mmio, dws);
> + struct regmap *syscon = dwsmmio->priv;
> + u8 cs;
> +
> + cs = spi->chip_select;
> + if (cs < 2)
> + dw_spi_elba_override_cs(syscon, spi->chip_select, enable);
> +
> + /*
> + * The DW SPI controller needs a native CS bit selected to start
> + * the serial engine.
> + */
> + spi->chip_select = 0;
> + dw_spi_set_cs(spi, enable);
> + spi->chip_select = cs;
> +}
> +
> +static int dw_spi_elba_init(struct platform_device *pdev,
> + struct dw_spi_mmio *dwsmmio)
> +{
> + struct regmap *syscon;
> +
> + syscon = syscon_regmap_lookup_by_phandle(dev_of_node(&pdev->dev),
> + "amd,pensando-elba-syscon");
> + if (IS_ERR(syscon))
> + return dev_err_probe(&pdev->dev, PTR_ERR(syscon),
> + "syscon regmap lookup failed\n");
<----+
> + dwsmmio->priv = syscon; |
> + dwsmmio->dws.set_cs = dw_spi_elba_set_cs; |
+-------------------------------------+
|
Newline here + would be nice. Other than that looks good.
Reviewed-by: Serge Semin <[email protected]>

-Serge(y)

> +
> + return 0;
> +}
> +
> static int dw_spi_mmio_probe(struct platform_device *pdev)
> {
> int (*init_func)(struct platform_device *pdev,
> @@ -352,6 +408,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
> { .compatible = "intel,thunderbay-ssi", .data = dw_spi_intel_init},
> { .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
> { .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
> + { .compatible = "amd,pensando-elba-spi", .data = dw_spi_elba_init},
> { /* end of table */}
> };
> MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
> --
> 2.17.1
>

2023-03-12 21:42:15

by Serge Semin

[permalink] [raw]

Subject: Re: [PATCH v11 04/15] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller

On Sat, Mar 11, 2023 at 04:44:34PM -0800, Brad Larson wrote:
> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
>
> Signed-off-by: Brad Larson <[email protected]>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
> ---
>
> v10 changes:
> - Move definition of amd,pensando-elba-syscon into properties
> with a better description
> - Add amd,pensando-elba-syscon: false for non elba designs
>
> v9 changes:
> - Define property amd,pensando-elba-syscon
> - Move compatible amd,pensando-elba-spi ahead of baikal,bt1-ssi
>
> ---
> .../bindings/spi/snps,dw-apb-ssi.yaml | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> index a132b5fc56e0..2383d6497b1e 100644
> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> @@ -37,6 +37,17 @@ allOf:
> else:
> required:
> - interrupts
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: amd,pensando-elba-spi
> + then:
> + required:
> + - amd,pensando-elba-syscon
> + else:
> + properties:
> + amd,pensando-elba-syscon: false
>
> properties:
> compatible:
> @@ -63,6 +74,8 @@ properties:
> const: intel,keembay-ssi
> - description: Intel Thunder Bay SPI Controller
> const: intel,thunderbay-ssi
> + - description: AMD Pensando Elba SoC SPI Controller
> + const: amd,pensando-elba-spi
> - description: Baikal-T1 SPI Controller
> const: baikal,bt1-ssi
> - description: Baikal-T1 System Boot SPI Controller
> @@ -136,6 +149,12 @@ properties:
> of the designware controller, and the upper limit is also subject to
> controller configuration.
>
> + amd,pensando-elba-syscon:
> + $ref: /schemas/types.yaml#/definitions/phandle-array

> + description: |
^
+--------------+
This + modifier is redundant.

> + Block address to control SPI chip-selects. The Elba SoC
> + does not use ssi. ^
|
1. Drop one of the whitespaces ---------------------+
2. The description is misleading. SSI means "Synchronous Serial
Interface" which basically means SPI. If you meant SS (slave-select)
signals then Elba SoC do use them. What would sound correctly here is
that Elba SoC system controller provides an interface to override the
native DWC SSI CS control.

Please fix the notes above. Then feel free to add
Reviewed-by: Serge Semin <[email protected]>

-Serge(y)

> +
> patternProperties:
> "^.*@[0-9a-f]+$":
> type: object
> --
> 2.17.1
>