Re: [PATCH] spi: spi-nxp-fspi: don't depend on a specific node name erratum workaround

From: Michael Walle
Date: Sat Oct 02 2021 - 08:41:16 EST

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