[PATCH v7 05/12] riscv: dts: jh7110: Move common code to the new jh7110-common-u-boot.dtsi

[E Shattow]

On Tue, Dec 3, 2024 at 10:52 PM Hal Feng <hal.feng at starfivetech.com> wrote:
>
> > On 04.12.24 03:16, E Shattow wrote:
> > On Mon, Dec 2, 2024 at 7:06 PM Hal Feng <hal.feng at starfivetech.com>
> > wrote:
> > >
> > > To support JH7110 based boards besides v1.3B, add a common dtsi and
> > > add common code to it.
> > >
> > > Tested-by: Anand Moon <linux.amoon at gmail.com>
> > > Tested-by: E Shattow <lucent at gmail.com>
> > > Reviewed-by: E Shattow <lucent at gmail.com>
> > > Signed-off-by: Hal Feng <hal.feng at starfivetech.com>
> > > ---
> > >  arch/riscv/dts/jh7110-common-u-boot.dtsi      | 142
> > ++++++++++++++++++
> > >  ...10-starfive-visionfive-2-v1.3b-u-boot.dtsi | 138 +----------------
> > >  2 files changed, 143 insertions(+), 137 deletions(-)  create mode
> > > 100644 arch/riscv/dts/jh7110-common-u-boot.dtsi
> > >
> > > diff --git a/arch/riscv/dts/jh7110-common-u-boot.dtsi
> > > b/arch/riscv/dts/jh7110-common-u-boot.dtsi
> > > new file mode 100644
> > > index 0000000000..7953459e67
> > > --- /dev/null
> > > +++ b/arch/riscv/dts/jh7110-common-u-boot.dtsi
> > > @@ -0,0 +1,142 @@
> > > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > > +/*
> > > + * Copyright (C) 2023 StarFive Technology Co., Ltd.
> > > + */
> > > +
> > > +#include "binman.dtsi"
> > > +#include "jh7110-u-boot.dtsi"
> > > +/ {
> > > +       aliases {
> > > +               spi0 = &qspi;
> > > +       };
> > > +
> > > +       chosen {
> > > +               bootph-pre-ram;
> > > +       };
> > > +
> > > +       firmware {
> > > +               spi0 = &qspi;
> > > +               bootph-pre-ram;
> > > +       };
> > > +
> > > +       config {
> > > +               bootph-pre-ram;
> > > +               u-boot,spl-payload-offset = <0x100000>;
> > > +       };
> > > +
> > > +       memory at 40000000 {
> > > +               bootph-pre-ram;
> > > +       };
> > > +};
> > > +
> > > +&uart0 {
> > > +       bootph-pre-ram;
> > > +       reg-offset = <0>;
> > > +       current-speed = <115200>;
> > > +       clock-frequency = <24000000>;
> > > +};
> > > +
> > > +&mmc0 {
> > > +       bootph-pre-ram;
> > > +};
> > > +
> > > +&mmc1 {
> > > +       bootph-pre-ram;
> > > +};
> > > +
> > > +&qspi {
> > > +       bootph-pre-ram;
> >
> > > +       spi-max-frequency = <250000000>;
> >
> > Drop this line spi-max-frequency 250Mhz. The driver expects spi-max-
> > frequency in any sub-node, and not here?
>
> Yes, this property is used in sub-node, we can drop this later.
>
> >
> > > +
> > > +       flash at 0 {
> > > +               bootph-pre-ram;
> >
> > > +               /delete-property/ cdns,read-delay;
> >
> > Drop this line /delete-property/ because it is a problem for the parser if we
> > need to re-define 'cdns,read-delay' later in dts files downstream of jh7110-
> > common-u-boot.dtsi
> >
> > If you want drivers/spi/cadence_qspi.c:spi_calibration() to do this work then it
> > needs to be the same upstream. In my testing on 4GB
> > Star64 the calibration (via /delete-property/ cdns,read-delay) method with
> > spi-max-frequency less than exactly 49.8MHz results in a failure to write SPI-
> > NOR correctly. When spi-max-frequency is approximately 25MHz up to
> > 49.799999MHz and written with a known good tool then the boot may be
> > successful but "Main section boot fail,use backup section"
> > or other errors on boot after using that unstable configuration of U-Boot to
> > write U-Boot image to the SPI-NOR. No problems writing and boot during
> > testing with spi-max-frequency 49.8MHz up to 100MHz (not tested above
> > 100MHz).
> >
> > The upstream dts has spi-max-frequency 12MHz which is not successful with
> > any of the read-delay values I tested: 0 1 2 3 4 5
> >
> > > +               spi-max-frequency = <100000000>;
> >
> > Why 12MHz upstream and 100MHz here? Something is wrong but I do not
> > know is it U-Boot or Linux?
> >
> > Success:
> > +             /delete-property/ cdns,read-delay;
> > +             spi-max-frequency = <100000000>;
> >
> > Success:
> > +             cdns,read-delay = <2>;
> > -             /delete-property/ cdns,read-delay;
> > +             spi-max-frequency = <100000000>;
> >
> > Failure:
> > +             cdns,read-delay = <2>;
> > +             spi-max-frequency = <50000000>;
> >
> > The actual fix is going to need testing Linux and U-Boot on all the boards I do
> > not have access to.
> >
> > For the moment we can:
> > -             /delete-property/ cdns,read-delay;
> > +             cdns,read-delay = <2>;
> > +             spi-max-frequency = <100000000>;
> >
> > or do nothing (but yes please drop that 250Mhz line it is non-op I think).
> >
> > What do you think?  Or else we discuss here and follow with patches in future
> > to fix the mess. I really do not want any /delete-property/ to be in dtsi but I
> > don't know why it is so different between U-Boot and Linux and cannot
> > personally test all the possible boards to verify a change.
>
> I don't know the cause of this problem either. But all properties added in this
> patch already existed in U-Boot DT before. I tried my best to keep the U-Boot
> DT the same as the one before OF_UPSTREAM applied.
>
> I tested and found that the qspi flash can work successfully in Linux and U-Boot
> with the settings you suggested above.
>
> cdns,read-delay = <2>;
> spi-max-frequency = <100000000>;

Good report on testing Linux and U-Boot, thank you Hal.

>
> If /delete-property/ is really not suitable to exist in this dtsi, we can modify as
> above in U-Boot and upstream this change to Liunx. After this change is accepted
> by Linux, I will drop these two properties in U-Boot.
>
> Best regards,
> Hal

Yes let's do that. So long as it all works on VF2 1.2a, VF 1.3b, Mars,
and Star64. I have tested Star64. You have tested I guess VF2 1.3b?
And if we miss some, it's okay even if there is a problem for some
board we did not test, the workaround is easy in u-boot specific dtsi
per-board because we have avoided '/delete-property/' in the common
dtsi.

-E