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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230309121414.6ay47dn57f2p26nh@mobilestation>
Date:   Thu, 9 Mar 2023 15:14:14 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Brad Larson <blarson@....com>
Cc:     adrian.hunter@...el.com, alcooperx@...il.com,
        andy.shevchenko@...il.com, arnd@...db.de,
        brendan.higgins@...ux.dev, briannorris@...omium.org,
        brijeshkumar.singh@....com, broonie@...nel.org,
        catalin.marinas@....com, davidgow@...gle.com,
        devicetree@...r.kernel.org, gerg@...ux-m68k.org, gsomlo@...il.com,
        krzk@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        lee.jones@...aro.org, lee@...nel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-mmc@...r.kernel.org, linux-spi@...r.kernel.org,
        p.yadav@...com, p.zabel@...gutronix.de, piotrs@...ence.com,
        rdunlap@...radead.org, robh+dt@...nel.org, samuel@...lland.org,
        skhan@...uxfoundation.org, suravee.suthikulpanit@....com,
        thomas.lendacky@....com, tonyhuang.sunplus@...il.com,
        ulf.hansson@...aro.org, vaishnav.a@...com, will@...nel.org,
        yamada.masahiro@...ionext.com
Subject: Re: [PATCH v10 10/15] spi: dw: Add support for AMD Pensando Elba SoC

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@....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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ