[v10,10/15] spi: dw: Add support for AMD Pensando Elba SoC

  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 <blarson@amd.com>
---

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 | 65 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)
  

On Sun, Mar 05, 2023 at 08:07:34PM -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 <blarson@amd.com>
> ---
> 
> 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 | 65 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> index 26c40ea6dd12..2076cb83a11b 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,56 @@ 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)
> +{
> +	const char *syscon_name = "amd,pensando-elba-syscon";

> +	struct device_node *np = pdev->dev.of_node;

Drop this since it's used only once below. 
                                                                          
> +	struct device_node *node;                                         
> +	struct regmap *syscon;                                            
> +                                                                       
> -	node = of_parse_phandle(np, syscon_name, 0);                      

	node = of_parse_phandle(dev_of_node(pdev->dev), syscon_name, 0);

> +	if (!node)

> +		return dev_err_probe(&pdev->dev, -ENODEV, "failed to find %s\n",
> +				     syscon_name);

Hm, using dev_err_probe() with known error value seems overkill.

> +

> +	syscon = syscon_node_to_regmap(node);
> +	if (IS_ERR(syscon))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(syscon),
> +				     "syscon regmap lookup failed\n");

of_node_put() is missing in the error and success paths.

-Serge(y)

> +
> +	dwsmmio->priv = syscon;
> +	dwsmmio->dws.set_cs = dw_spi_elba_set_cs;
> +
> +	return 0;
> +}
> +
>  static int dw_spi_mmio_probe(struct platform_device *pdev)
>  {
>  	int (*init_func)(struct platform_device *pdev,
> @@ -352,6 +416,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
>
  

On Mon, Mar 6, 2023 at 6:00 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> On Sun, Mar 05, 2023 at 08:07:34PM -0800, Brad Larson wrote:

...

> > -     node = of_parse_phandle(np, syscon_name, 0);
>
>         node = of_parse_phandle(dev_of_node(pdev->dev), syscon_name, 0);

Side note: I would rather see syscon_fwnode_to_regmap() instead of
this. And IIRC syscon already has an API to find by name.

> > +     if (!node)
>
> > +             return dev_err_probe(&pdev->dev, -ENODEV, "failed to find %s\n",
> > +                                  syscon_name);
>
> Hm, using dev_err_probe() with known error value seems overkill.

It's allowed use and it helps to drop a few unnecessary lines of code.

> > +     syscon = syscon_node_to_regmap(node);
> > +     if (IS_ERR(syscon))
> > +             return dev_err_probe(&pdev->dev, PTR_ERR(syscon),
> > +                                  "syscon regmap lookup failed\n");
  

On Mon, Mar 06, 2023 at 09:59:29PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 6, 2023 at 6:00 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > On Sun, Mar 05, 2023 at 08:07:34PM -0800, Brad Larson wrote:
> 
> ...
> 
> > > -     node = of_parse_phandle(np, syscon_name, 0);
> >
> >         node = of_parse_phandle(dev_of_node(pdev->dev), syscon_name, 0);
> 

> Side note: I would rather see syscon_fwnode_to_regmap() instead of
> this. And IIRC syscon already has an API to find by name.

Ah, right. Though it's called syscon_regmap_lookup_by_phandle() and it
can be used to replace the entire pattern: "Parse property; Find node
by phandle; Get regmap by node". Basically the of_parse_phandle() and
syscon_node_to_regmap() methods invocation can be replaced with just a
single function call. Thus there won't be need to worry about
decrementing the found DT-node ref-counter.

> 
> > > +     if (!node)
> >
> > > +             return dev_err_probe(&pdev->dev, -ENODEV, "failed to find %s\n",
> > > +                                  syscon_name);
> >
> > Hm, using dev_err_probe() with known error value seems overkill.
> 

> It's allowed use and it helps to drop a few unnecessary lines of code.

Ok, seeing there is no alternative to that method which would return
the passed error but wouldn't have a name implying the silent
deferred-probe error semantics.

-Serge(y)

> 
> > > +     syscon = syscon_node_to_regmap(node);
> > > +     if (IS_ERR(syscon))
> > > +             return dev_err_probe(&pdev->dev, PTR_ERR(syscon),
> > > +                                  "syscon regmap lookup failed\n");
> 
> -- 
> With Best Regards,
> Andy Shevchenko
  

On Mon, Mar 06, 2023 at 16:00, Serge Semin wrote:
> On Sun, Mar 05, 2023 at 08:07:34PM -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 <blarson@amd.com>
>> ---
>> 
>> 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 | 65 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 65 insertions(+)
>> 
>> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
>> index 26c40ea6dd12..2076cb83a11b 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,56 @@ 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)
>> +{
>> +	const char *syscon_name = "amd,pensando-elba-syscon";
>
>> +	struct device_node *np = pdev->dev.of_node;
>
> Drop this since it's used only once below. 
>

Removed

>> +	struct device_node *node;                                         

Renamed *node to *np

>> +	struct regmap *syscon;                                            
>> +                                                                       
>> -	node = of_parse_phandle(np, syscon_name, 0);                      
>
>	node = of_parse_phandle(dev_of_node(pdev->dev), syscon_name, 0);
>
> +	if (!node)
>
>> +		return dev_err_probe(&pdev->dev, -ENODEV, "failed to find %s\n",
>> +				     syscon_name);
>
> Hm, using dev_err_probe() with known error value seems overkill.

Changed to: return -ENODEV

>> +
>
>> +	syscon = syscon_node_to_regmap(node);
>> +	if (IS_ERR(syscon))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(syscon),
>> +				     "syscon regmap lookup failed\n");
>
> of_node_put() is missing in the error and success paths.

Result of the above changes are:

+       const char *syscon_name = "amd,pensando-elba-syscon";
+       struct device_node *np;
+       struct regmap *syscon;
+
+       np = of_parse_phandle(pdev->dev.of_node, syscon_name, 0);
+       if (!np)
+               return -ENODEV;
+
+       syscon = syscon_node_to_regmap(np);
+       of_node_put(np);
+       if (IS_ERR(syscon))
+               return dev_err_probe(&pdev->dev, PTR_ERR(syscon),
+                                    "syscon regmap lookup failed\n");

Regards,
Brad
  

On Mon, Mar 06, 2023 at 06:20:02PM -0800, Brad Larson wrote:
> On Mon, Mar 06, 2023 at 16:00, Serge Semin wrote:
> > On Sun, Mar 05, 2023 at 08:07:34PM -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 <blarson@amd.com>
> >> ---
> >> 
> >> 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 | 65 +++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 65 insertions(+)
> >> 
> >> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> >> index 26c40ea6dd12..2076cb83a11b 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,56 @@ 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)
> >> +{
> >> +	const char *syscon_name = "amd,pensando-elba-syscon";
> >
> >> +	struct device_node *np = pdev->dev.of_node;
> >
> > Drop this since it's used only once below. 
> >
> 
> Removed
> 
> >> +	struct device_node *node;                                         
> 
> Renamed *node to *np
> 
> >> +	struct regmap *syscon;                                            
> >> +                                                                       
> >> -	node = of_parse_phandle(np, syscon_name, 0);                      
> >
> >	node = of_parse_phandle(dev_of_node(pdev->dev), syscon_name, 0);
> >
> > +	if (!node)
> >
> >> +		return dev_err_probe(&pdev->dev, -ENODEV, "failed to find %s\n",
> >> +				     syscon_name);
> >
> > Hm, using dev_err_probe() with known error value seems overkill.
> 
> Changed to: return -ENODEV
> 
> >> +
> >
> >> +	syscon = syscon_node_to_regmap(node);
> >> +	if (IS_ERR(syscon))
> >> +		return dev_err_probe(&pdev->dev, PTR_ERR(syscon),
> >> +				     "syscon regmap lookup failed\n");
> >
> > of_node_put() is missing in the error and success paths.
> 
> Result of the above changes are:
> 
> +       const char *syscon_name = "amd,pensando-elba-syscon";
> +       struct device_node *np;
> +       struct regmap *syscon;
> +

> +       np = of_parse_phandle(pdev->dev.of_node, syscon_name, 0);
> +       if (!np)
> +               return -ENODEV;
> +
> +       syscon = syscon_node_to_regmap(np);
> +       of_node_put(np);
> +       if (IS_ERR(syscon))
> +               return dev_err_probe(&pdev->dev, PTR_ERR(syscon),
> +                                    "syscon regmap lookup failed\n");

As Andy correctly noted this can be fully converted to just a single call:
syscon_regmap_lookup_by_phandle().

and replace pdev->dev.of_node with dev_of_node(pdev->dev).

-Serge(y)

> 
> Regards,
> Brad