| Message ID | 20211001212726.159437-1-michael@walle.cc (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 67a12ae52599c9f2f24ef14adb43fc3b164792b5 |
| Headers | show |
| Series | spi: spi-nxp-fspi: don't depend on a specific node name erratum workaround | expand |
On Fri, 1 Oct 2021 23:27:26 +0200, Michael Walle wrote: > In commit 7e71b85473f8 ("arm64: dts: ls1028a: fix node name for the > sysclk") the sysclk node name was renamed and broke the erratum > workaround because it tries to fetch a device tree node by its name, > which is very fragile in general. We don't even need the sysclk node > because the only possible sysclk frequency input is 100MHz. In fact, the > erratum says it applies if SYS_PLL_RAT is 3, not that the platform clock > is 300 MHz. Make the workaround more reliable and just drop the unneeded > sysclk lookup. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: spi-nxp-fspi: don't depend on a specific node name erratum workaround commit: 67a12ae52599c9f2f24ef14adb43fc3b164792b5 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Fri, Oct 01, 2021 at 11:27:26PM +0200, Michael Walle wrote: > In commit 7e71b85473f8 ("arm64: dts: ls1028a: fix node name for the > sysclk") the sysclk node name was renamed and broke the erratum > workaround because it tries to fetch a device tree node by its name, > which is very fragile in general. It seems to me that the honest approach for the patch that got broken would have been to add a separate "nxp,ls1028a-fspi" compatible string instead of keeping banging "nxp,lx2160a-fspi", and for this special compatible string, require the platform PLL as a clock provider, and use clk_get() and clk_get_rate(). > We don't even need the sysclk node because the only possible sysclk > frequency input is 100MHz. True. In the RCW, every SYSCLK_FREQ value except 0b1001011000 - 100 MHz is reserved. That goes to show how shallow my patch which broke this ERR workaround was. The sysclk frequency in the device tree was 100 MHz before _and_ after the U-Boot "fixup", but the warning message was just annoying me. > In fact, the erratum says it applies if SYS_PLL_RAT is 3, not that the > platform clock is 300 MHz. Correct, although one implies the other. And probably the platform clock does not matter either, just the clock that the block gets, which just "happens" to be platform clock / 2. Anyway. > 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). On the other hand, is anyone using LS1028A with a platform clock of 300 MHz? :) > Fixes: 82ce7d0e74b6 ("spi: spi-nxp-fspi: Implement errata workaround for LS1028A") > Cc: Vladimir Oltean <vladimir.oltean@nxp.com> > Signed-off-by: Michael Walle <michael@walle.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. > > /* 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? > > - /* 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@2070000 { }; fspi: spi@20c0000 { - compatible = "nxp,lx2160a-fspi"; + compatible = "nxp,ls1028a-fspi"; #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 */ } - }; - 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 */
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. 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? :) > >> Fixes: 82ce7d0e74b6 ("spi: spi-nxp-fspi: Implement errata workaround >> for LS1028A") >> Cc: Vladimir Oltean <vladimir.oltean@nxp.com> >> Signed-off-by: Michael Walle <michael@walle.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. >> >> /* 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@2070000 { > }; > > fspi: spi@20c0000 { > - compatible = "nxp,lx2160a-fspi"; > + compatible = "nxp,ls1028a-fspi"; Why not compatible = "nxp,ls1028a-fspi", "nxp,lx2160a-fspi"; to keep at least some compatibility. > #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. > - 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 */
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@nxp.com> > > > Signed-off-by: Michael Walle <michael@walle.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@2070000 { > > }; > > > > fspi: spi@20c0000 { > > - 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
Am 2021-10-02 11:23, schrieb Vladimir Oltean: > 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. Which I can't answer ;) > >> > > Fixes: 82ce7d0e74b6 ("spi: spi-nxp-fspi: Implement errata workaround >> > > for LS1028A") >> > > Cc: Vladimir Oltean <vladimir.oltean@nxp.com> >> > > Signed-off-by: Michael Walle <michael@walle.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. And you'd need to get Rob into the boat for the dt bindings "fixes", no? For the new clock. >> > > /* 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@2070000 { >> > }; >> > >> > fspi: spi@20c0000 { >> > - 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. No worst case is, the flexspi driver doesn't probe at all (new devicetree, old kernel ;). > 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. Nooo. No revisions in the compatible string. const struct soc_device_attribute ls1028a_soc_attr[] = { { .family = "QorIQ LS1028A", .revision = "1.0" }, { } }; Thus you'd still need that check above. -michael
On Sat, Oct 02, 2021 at 11:34:12AM +0200, Michael Walle wrote: > > > > 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) I don't think I'm the one who forgot to update the bindings, btw. In Documentation/devicetree/bindings/spi/spi-nxp-fspi.txt (still not using JSON schema), the "clocks" are not documented as to what they want and why do both "fspi" and "fspi_en" even exist. The only mention you see of the "fspi" and "fspi_en" clocks in that file is an _example_. And that example remains correct, because it is for the LX2160A. > > > 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. > > And you'd need to get Rob into the boat for the dt bindings "fixes", > no? For the new clock. Yup. > > > > 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@2070000 { > > > > }; > > > > > > > > fspi: spi@20c0000 { > > > > - 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. > > No worst case is, the flexspi driver doesn't probe at all (new devicetree, > old kernel ;). Well, if you're going to take my patch as is, sure. But the device tree can still be modified in such a way that old kernels keep seeing what they saw before - the fallback compatibility string, and ignore the third clock provider. With even more care and consideration for new kernels operating with old DT blobs, the ERR workaround could check for the clock provider in the device tree first, then fall back to open-coding its own deductions of the platform clock if that fails. After a grace period of one or two years or so, maybe the open-coding could then be removed. > > 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. > > Nooo. No revisions in the compatible string. > > const struct soc_device_attribute ls1028a_soc_attr[] = { > { .family = "QorIQ LS1028A", .revision = "1.0" }, > { } > }; > > Thus you'd still need that check above. Ok, the idea of changing the compatible string was to make the driver search for the third "base" clock. But the bindings document can simply say that the "base" clock is optional for all SoCs - with the caveat that the LS1028A ERR workarounds will not be applied if not provided. And in that case, not even a SoC specific compatible string is needed.
Am 2021-10-02 11:55, schrieb Vladimir Oltean: > On Sat, Oct 02, 2021 at 11:34:12AM +0200, Michael Walle wrote: >> > > > 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) > > I don't think I'm the one who forgot to update the bindings, btw. > In Documentation/devicetree/bindings/spi/spi-nxp-fspi.txt (still not > using JSON schema), the "clocks" are not documented as to what they > want > and why do both "fspi" and "fspi_en" even exist. The only mention you > see of the "fspi" and "fspi_en" clocks in that file is an _example_. > And that example remains correct, because it is for the LX2160A. That doesn't mean you shouldn't document it in the bindings. I just wanted to point out that for a fixes commit, you'd have to consider that change, too. >> > > 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. >> >> And you'd need to get Rob into the boat for the dt bindings "fixes", >> no? For the new clock. > > Yup. > >> > > > 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@2070000 { >> > > > }; >> > > > >> > > > fspi: spi@20c0000 { >> > > > - 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. >> >> No worst case is, the flexspi driver doesn't probe at all (new >> devicetree, >> old kernel ;). > > Well, if you're going to take my patch as is, sure. But the device tree > can still be modified in such a way that old kernels keep seeing what > they saw before - the fallback compatibility string, and ignore the > third clock provider. Ack. > With even more care and consideration for new kernels operating with > old > DT blobs, the ERR workaround could check for the clock provider in the > device tree first, then fall back to open-coding its own deductions of > the platform clock if that fails. After a grace period of one or two > years or so, maybe the open-coding could then be removed. Mh, do you really want to go that extra mile for something you don't even know is used? It's up to you, I wouldn't do it ;) >> > 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. >> >> Nooo. No revisions in the compatible string. >> >> const struct soc_device_attribute ls1028a_soc_attr[] = { >> { .family = "QorIQ LS1028A", .revision = "1.0" }, >> { } >> }; >> >> Thus you'd still need that check above. > > Ok, the idea of changing the compatible string was to make the driver > search for the third "base" clock. But the bindings document can simply > say that the "base" clock is optional for all SoCs - with the caveat > that the LS1028A ERR workarounds will not be applied if not provided. > And in that case, not even a SoC specific compatible string is needed. Ack. -michael
Hi Vladimir and Michael, > -----Original Message----- > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Sent: Saturday, October 2, 2021 2:53 PM > To: Michael Walle <michael@walle.cc> > Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; Ashish Kumar > <ashish.kumar@nxp.com>; Yogesh Gaur <yogeshgaur.83@gmail.com>; Mark > Brown <broonie@kernel.org>; Kuldeep Singh <kuldeep.singh@nxp.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. I didn't receive emails in proper order not sure why. Anyway, I will try to pitch in details which I have. There was one customer(cannot recall name) using LS1028A with 300Mhz. They reported AHB failure issue and later on it turns out to be an errata. I didn't hear anything from them whether they pursued with 300Mhz or fallback to 400Mhz. > > > > > Fixes: 82ce7d0e74b6 ("spi: spi-nxp-fspi: Implement errata > > > > workaround for LS1028A") > > > > Cc: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > Signed-off-by: Michael Walle <michael@walle.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. dt-bindings are updated to json schema and already posted upstream awaiting Mark's feedback. Moreover, using third optional clock seems a viable solution. Since this is the first time we have stumbled such issue and isn't a frequent one, We can keep current changes as is and if face similar issue later, can switch to new proposed solution. Similar changes are also done for u-boot[1] in an absurd manner as I couldn't find simpler way to do it. I'll be glad to receive feedback on those patches. Regards Kuldeep [1] https://patchwork.ozlabs.org/project/uboot/list/?series=256398 > > 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@2070000 { > > > }; > > > > > > fspi: spi@20c0000 { > > > - 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
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) /* 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); - /* 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;
In commit 7e71b85473f8 ("arm64: dts: ls1028a: fix node name for the sysclk") the sysclk node name was renamed and broke the erratum workaround because it tries to fetch a device tree node by its name, which is very fragile in general. We don't even need the sysclk node because the only possible sysclk frequency input is 100MHz. In fact, the erratum says it applies if SYS_PLL_RAT is 3, not that the platform clock is 300 MHz. 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 Fixes: 82ce7d0e74b6 ("spi: spi-nxp-fspi: Implement errata workaround for LS1028A") Cc: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/spi/spi-nxp-fspi.c | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-)