> On 04.12.24 18:56, E Shattow wrote:
> On Tue, Dec 3, 2024 at 10:52 PM Hal Feng <hal.f...@starfivetech.com>
> wrote:
> >
> > > On 04.12.24 03:16, E Shattow wrote:
> > > On Mon, Dec 2, 2024 at 7:06 PM Hal Feng <hal.f...@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.am...@gmail.com>
> > > > Tested-by: E Shattow <luc...@gmail.com>
> > > > Reviewed-by: E Shattow <luc...@gmail.com>
> > > > Signed-off-by: Hal Feng <hal.f...@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@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@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.
OK. Yeah, I tested on VF2 1.3b and VF2 1.2a is very similar to VF2 1.3b.
Best regards,
Hal
