| Message ID | 20230306040739.51488-11-blarson@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support AMD Pensando Elba SoC | expand |
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
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; + struct device_node *node; + struct regmap *syscon; + + node = of_parse_phandle(np, syscon_name, 0); + if (!node) + return dev_err_probe(&pdev->dev, -ENODEV, "failed to find %s\n", + syscon_name); + + syscon = syscon_node_to_regmap(node); + 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; + + 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);
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(+)