Message-ID: <20211002092301.ue77mzzwcel76umx@skbuf>
Date:   Sat, 2 Oct 2021 09:23:02 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Michael Walle <michael@...le.cc>
CC:     "linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Ashish Kumar <ashish.kumar@....com>,
        Yogesh Gaur <yogeshgaur.83@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Kuldeep Singh <kuldeep.singh@....com>
Subject: Re: [PATCH] spi: spi-nxp-fspi: don't depend on a specific node name
 erratum workaround

On Sat, Oct 02, 2021 at 10:58:31AM +0200, Michael Walle wrote:
> Am 2021-10-02 03:37, schrieb Vladimir Oltean:
> > On Fri, Oct 01, 2021 at 11:27:26PM +0200, Michael Walle wrote:
> 
> > > Make the workaround more reliable and just drop the unneeded sysclk
> > > lookup.
> > > 
> > > For reference, the error during the bootup is the following:
> > > [    4.898400] nxp-fspi 20c0000.spi: Errata cannot be executed. Read
> > > via IP bus may not work
> > 
> > Well, in Kuldeep's defence, at least this part is sane, right? I mean we
> > cannot prove an issue => we don't disable reads via the AHB. So it's
> > just the error message (which I didn't notice TBH, sorry).
> 
> Its just an error message in case the platform clock is 400Mhz. But
> if you have a 300MHz platform clock the workaround wouldn't be applied.

Understood, that's why I asked...

> The reference is just there if someone stumbles over this error and
> searches for it on google.
> 
> > On the other hand, is anyone using LS1028A with a platform clock of 300
> > MHz? :)

...this.

> > > Fixes: 82ce7d0e74b6 ("spi: spi-nxp-fspi: Implement errata workaround
> > > for LS1028A")
> > > Cc: Vladimir Oltean <vladimir.oltean@....com>
> > > Signed-off-by: Michael Walle <michael@...le.cc>
> > > ---
> > >  drivers/spi/spi-nxp-fspi.c | 26 +++++++-------------------
> > >  1 file changed, 7 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> > > index a66fa97046ee..2b0301fc971c 100644
> > > --- a/drivers/spi/spi-nxp-fspi.c
> > > +++ b/drivers/spi/spi-nxp-fspi.c
> > > @@ -33,6 +33,7 @@
> > > 
> > >  #include <linux/acpi.h>
> > >  #include <linux/bitops.h>
> > > +#include <linux/bitfield.h>
> > >  #include <linux/clk.h>
> > >  #include <linux/completion.h>
> > >  #include <linux/delay.h>
> > > @@ -315,6 +316,7 @@
> > >  #define NXP_FSPI_MIN_IOMAP	SZ_4M
> > > 
> > >  #define DCFG_RCWSR1		0x100
> > > +#define SYS_PLL_RAT		GENMASK(6, 2)
> > 
> > Ugh. So your solution still makes a raw read of the platform PLL value
> > from the DCFG, now it just adds a nice definition for it. Not nice.
> 
> Keep in mind that this is intended to be a fixes commit. I agree with
> you that having a new clock in the device tree and checking that would
> have been better. Feel free to change the workaround after this fix
> is applied (without a fixes tag), but I don't think introducing a new
> clock (and you forgot to update the bindings) will qualify as a fixes
> commit. Esp. when you change the compatible string.

I think it could be justified as a fixes commit to Shawn Guo - the
LS1028A is not "compatible" with LX2160A in the sense that it has
software-visible errata which LX2160A doesn't have.

> > >  /* Access flash memory using IP bus only */
> > >  #define FSPI_QUIRK_USE_IP_ONLY	BIT(0)
> > > @@ -926,9 +928,8 @@ static void erratum_err050568(struct nxp_fspi *f)
> > >  		{ .family = "QorIQ LS1028A" },
> > >  		{ /* sentinel */ }
> > >  	};
> > > -	struct device_node *np;
> > >  	struct regmap *map;
> > > -	u32 val = 0, sysclk = 0;
> > > +	u32 val, sys_pll_ratio;
> > >  	int ret;
> > > 
> > >  	/* Check for LS1028A family */
> > > @@ -937,7 +938,6 @@ static void erratum_err050568(struct nxp_fspi *f)
> > >  		return;
> > >  	}
> > > 
> > > -	/* Compute system clock frequency multiplier ratio */
> > >  	map = syscon_regmap_lookup_by_compatible("fsl,ls1028a-dcfg");
> > >  	if (IS_ERR(map)) {
> > >  		dev_err(f->dev, "No syscon regmap\n");
> > > @@ -948,23 +948,11 @@ static void erratum_err050568(struct nxp_fspi
> > > *f)
> > >  	if (ret < 0)
> > >  		goto err;
> > > 
> > > -	/* Strap bits 6:2 define SYS_PLL_RAT i.e frequency multiplier
> > > ratio */
> > > -	val = (val >> 2) & 0x1F;
> > > -	WARN(val == 0, "Strapping is zero: Cannot determine ratio");
> > > +	sys_pll_ratio = FIELD_GET(SYS_PLL_RAT, val);
> > > +	dev_dbg(f->dev, "val: 0x%08x, sys_pll_ratio: %d\n", val,
> > > sys_pll_ratio);
> > 
> > Do we really feel that this dev_dbg is valuable?
> 
> No, I just briefly looked at it to see it prints 4 ;)
> 
> > > -	/* Compute system clock frequency */
> > > -	np = of_find_node_by_name(NULL, "clock-sysclk");
> > > -	if (!np)
> > > -		goto err;
> > > -
> > > -	if (of_property_read_u32(np, "clock-frequency", &sysclk))
> > > -		goto err;
> > > -
> > > -	sysclk = (sysclk * val) / 1000000; /* Convert sysclk to Mhz */
> > > -	dev_dbg(f->dev, "val: 0x%08x, sysclk: %dMhz\n", val, sysclk);
> > > -
> > > -	/* Use IP bus only if PLL is 300MHz */
> > > -	if (sysclk == 300)
> > > +	/* Use IP bus only if platform clock is 300MHz */
> > > +	if (sys_pll_ratio == 3)
> > >  		f->devtype_data->quirks |= FSPI_QUIRK_USE_IP_ONLY;
> > > 
> > >  	return;
> > > --
> > > 2.30.2
> > > 
> > 
> > How about:
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> > b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> > index 343ecf0e8973..ffe820c22719 100644
> > --- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> > @@ -326,15 +326,17 @@ i2c7: i2c@...0000 {
> >  		};
> > 
> >  		fspi: spi@...0000 {
> > -			compatible = "nxp,lx2160a-fspi";
> > +			compatible = "nxp,ls1028a-fspi";
> 
> Why not
>   compatible = "nxp,ls1028a-fspi", "nxp,lx2160a-fspi";
> to keep at least some compatibility.

Of course that would be even better. I just wanted to rush to get here
before Mark, and it looks like I still didn't make it in time.

Worst case, new (cleaned up to not calculate the platform clock on its own)
driver will still probe with old device tree, but not apply the ERR
workaround for 300 MHz systems.

I may be ignorant here, but I just don't know how many systems use 300
MHz platform in practice. Anyway, it's always difficult to fix up
something that came to depend on DT bindings in a certain way.

> >  			#address-cells = <1>;
> >  			#size-cells = <0>;
> >  			reg = <0x0 0x20c0000 0x0 0x10000>,
> >  			      <0x0 0x20000000 0x0 0x10000000>;
> >  			reg-names = "fspi_base", "fspi_mmap";
> >  			interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
> > -			clocks = <&fspi_clk>, <&fspi_clk>;
> > -			clock-names = "fspi_en", "fspi";
> > +			clocks = <&fspi_clk>, <&fspi_clk>,
> > +				 <&clockgen QORIQ_CLK_PLATFORM_PLL
> > +					    QORIQ_CLK_PLL_DIV(2)>;
> > +			clock-names = "fspi_en", "fspi", "base";
> >  			status = "disabled";
> >  		};
> > 
> > diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> > index a66fa97046ee..f2815e6cae2c 100644
> > --- a/drivers/spi/spi-nxp-fspi.c
> > +++ b/drivers/spi/spi-nxp-fspi.c
> > @@ -314,8 +314,6 @@
> >  #define NXP_FSPI_MAX_CHIPSELECT		4
> >  #define NXP_FSPI_MIN_IOMAP	SZ_4M
> > 
> > -#define DCFG_RCWSR1		0x100
> > -
> >  /* Access flash memory using IP bus only */
> >  #define FSPI_QUIRK_USE_IP_ONLY	BIT(0)
> > 
> > @@ -922,55 +920,18 @@ static int nxp_fspi_adjust_op_size(struct
> > spi_mem *mem, struct spi_mem_op *op)
> > 
> >  static void erratum_err050568(struct nxp_fspi *f)
> >  {
> > -	const struct soc_device_attribute ls1028a_soc_attr[] = {
> > -		{ .family = "QorIQ LS1028A" },
> > -		{ /* sentinel */ }
> > -	};
> 
> Mh, I see how you came to the conclusion to rename the compatible
> string. But normally, this also contains a revision check,
> which is missing here IMHO. It might as well be fixed in the
> next revision (though we both know, this is highly unlikely; its
> still wrong). So while you could rename the compatible (oh no!)
> you'd still have to do the rev 1.0 check here.

So you want a compatible string a la "fsl,ls1021a-v1.0-dspi", right?
I don't know, no strong opinion, as you said, we both know that no
LS1028A rev 2 seems to be planned.

> > -	struct device_node *np;
> > -	struct regmap *map;
> > -	u32 val = 0, sysclk = 0;
> > -	int ret;
> > +	struct clk *clk_base;
> > 
> > -	/* Check for LS1028A family */
> > -	if (!soc_device_match(ls1028a_soc_attr)) {
> > -		dev_dbg(f->dev, "Errata applicable only for LS1028A\n");
> > +	clk_base = clk_get(f->dev, "base");
> > +	if (IS_ERR(clk_base)) {
> > +		dev_err(f->dev, "Errata cannot be executed. Read via IP bus may not
> > work\n");
> >  		return;
> >  	}
> > 
> > -	/* Compute system clock frequency multiplier ratio */
> > -	map = syscon_regmap_lookup_by_compatible("fsl,ls1028a-dcfg");
> > -	if (IS_ERR(map)) {
> > -		dev_err(f->dev, "No syscon regmap\n");
> > -		goto err;
> > -	}
> > -
> > -	ret = regmap_read(map, DCFG_RCWSR1, &val);
> > -	if (ret < 0)
> > -		goto err;
> > -
> > -	/* Strap bits 6:2 define SYS_PLL_RAT i.e frequency multiplier ratio */
> > -	val = (val >> 2) & 0x1F;
> > -	WARN(val == 0, "Strapping is zero: Cannot determine ratio");
> > -
> > -	/* Compute system clock frequency */
> > -	np = of_find_node_by_name(NULL, "clock-sysclk");
> > -	if (!np)
> > -		goto err;
> > -
> > -	if (of_property_read_u32(np, "clock-frequency", &sysclk))
> > -		goto err;
> > -
> > -	sysclk = (sysclk * val) / 1000000; /* Convert sysclk to Mhz */
> > -	dev_dbg(f->dev, "val: 0x%08x, sysclk: %dMhz\n", val, sysclk);
> > -
> > -	/* Use IP bus only if PLL is 300MHz */
> > -	if (sysclk == 300)
> > +	/* Use IP bus only if platform PLL is configured for 300 MHz, hence
> > we get 150 MHz */
> > +	if (clk_get_rate(clk_base) == 150000000)
> >  		f->devtype_data->quirks |= FSPI_QUIRK_USE_IP_ONLY;
> > -
> > -	return;
> > -
> > -err:
> > -	dev_err(f->dev, "Errata cannot be executed. Read via IP bus may not
> > work\n");
> > +	clk_put(clk_base);
> >  }
> > 
> >  static int nxp_fspi_default_setup(struct nxp_fspi *f)
> > @@ -994,10 +955,8 @@ static int nxp_fspi_default_setup(struct nxp_fspi
> > *f)
> >  	/*
> >  	 * ERR050568: Flash access by FlexSPI AHB command may not work with
> >  	 * platform frequency equal to 300 MHz on LS1028A.
> > -	 * LS1028A reuses LX2160A compatible entry. Make errata applicable for
> > -	 * Layerscape LS1028A platform.
> >  	 */
> > -	if (of_device_is_compatible(f->dev->of_node, "nxp,lx2160a-fspi"))
> > +	if (of_device_is_compatible(f->dev->of_node, "nxp,ls1028a-fspi"))
> >  		erratum_err050568(f);
> > 
> >  	/* Reset the module */
> 
> -- 
> -michael

Powered by blists - more mailing lists