Sync qspi flash setting to read-delay=2 and spi-max-frequency 100MHz for
better compatibility with operating system and downstream boot loader SPL
secondary program loader.

Signed-off-by: E Shattow <e@freeshell.de>
Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
---
 arch/riscv/boot/dts/starfive/jh7110-common.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
index a5661b677687..8a59c3001339 100644
--- a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
@@ -317,8 +317,8 @@ &qspi {
 	nor_flash: flash@0 {
 		compatible = "jedec,spi-nor";
 		reg = <0>;
-		cdns,read-delay = <5>;
-		spi-max-frequency = <12000000>;
+		cdns,read-delay = <2>;
+		spi-max-frequency = <100000000>;
 		cdns,tshsl-ns = <1>;
 		cdns,tsd2d-ns = <1>;
 		cdns,tchsh-ns = <1>;
-- 
2.47.2

E Shattow wrote:
> Sync qspi flash setting to read-delay=2 and spi-max-frequency 100MHz for
> better compatibility with operating system and downstream boot loader SPL
> secondary program loader.

Here you should be explaining why these values better describe the hardware. To
me this just reads as "u-boot does this, so let's do the same" whith doesn't
really explain anything.

/Emil

>
> Signed-off-by: E Shattow <e@freeshell.de>
> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  arch/riscv/boot/dts/starfive/jh7110-common.dtsi | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> index a5661b677687..8a59c3001339 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> @@ -317,8 +317,8 @@ &qspi {
>  	nor_flash: flash@0 {
>  		compatible = "jedec,spi-nor";
>  		reg = <0>;
> -		cdns,read-delay = <5>;
> -		spi-max-frequency = <12000000>;
> +		cdns,read-delay = <2>;
> +		spi-max-frequency = <100000000>;
>  		cdns,tshsl-ns = <1>;
>  		cdns,tsd2d-ns = <1>;
>  		cdns,tchsh-ns = <1>;
> --
> 2.47.2
>

Hi Emil,

On 2/5/25 02:18, Emil Renner Berthing wrote:
> E Shattow wrote:
>> Sync qspi flash setting to read-delay=2 and spi-max-frequency 100MHz for
>> better compatibility with operating system and downstream boot loader SPL
>> secondary program loader.
> 
> Here you should be explaining why these values better describe the hardware. To
> me this just reads as "u-boot does this, so let's do the same" whith doesn't
> really explain anything.
> 
> /Emil
> 
>>
>> Signed-off-by: E Shattow <e@freeshell.de>
>> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
>> ---
>>  arch/riscv/boot/dts/starfive/jh7110-common.dtsi | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
>> index a5661b677687..8a59c3001339 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
>> @@ -317,8 +317,8 @@ &qspi {
>>  	nor_flash: flash@0 {
>>  		compatible = "jedec,spi-nor";
>>  		reg = <0>;
>> -		cdns,read-delay = <5>;
>> -		spi-max-frequency = <12000000>;
>> +		cdns,read-delay = <2>;
>> +		spi-max-frequency = <100000000>;
>>  		cdns,tshsl-ns = <1>;
>>  		cdns,tsd2d-ns = <1>;
>>  		cdns,tchsh-ns = <1>;
>> --
>> 2.47.2
>>

That is true, there's not much to explain. It works at delay 2 and
100MHz max. I tried many delay values with the pre-existing 12MHz max,
they did not work. I don't have more information than that except what I
wrote about on the U-Boot mailing list [1]

[1]
https://lore.kernel.org/u-boot/ZQ2PR01MB13072E932737DD9D65E3A250E637A@ZQ2PR01MB1307.CHNPR01.prod.partner.outlook.cn/

The parameter training code in U-Boot when given max 100MHz has this at
somewhere between delay 2 and delay 3, where delay 3 almost appears to
work but there's data corruption, and delay 1 is totally non-functional.
With all testing so far the delay 2 and max frequency 100MHz is reliable
and also is a common occurrence in the Linux code base for similar qspi
access.

My testing methodology was pretty extensive and I wrote about the result
on the U-Boot mailing list, however here we're taking the success of
that in U-Boot and literally just applying it to Linux where it also
happens to align with some common similar assignments for other boards
in the devicetree code base. It works and there's no deeper insight here.

Welcome to hear from anyone that might actually understand this hardware?

-E

E Shattow wrote:
> Hi Emil,
>
> On 2/5/25 02:18, Emil Renner Berthing wrote:
> > E Shattow wrote:
> >> Sync qspi flash setting to read-delay=2 and spi-max-frequency 100MHz for
> >> better compatibility with operating system and downstream boot loader SPL
> >> secondary program loader.
> >
> > Here you should be explaining why these values better describe the hardware. To
> > me this just reads as "u-boot does this, so let's do the same" whith doesn't
> > really explain anything.
> >
> > /Emil
> >
> >>
> >> Signed-off-by: E Shattow <e@freeshell.de>
> >> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
> >> ---
> >>  arch/riscv/boot/dts/starfive/jh7110-common.dtsi | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> >> index a5661b677687..8a59c3001339 100644
> >> --- a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> >> +++ b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> >> @@ -317,8 +317,8 @@ &qspi {
> >>  	nor_flash: flash@0 {
> >>  		compatible = "jedec,spi-nor";
> >>  		reg = <0>;
> >> -		cdns,read-delay = <5>;
> >> -		spi-max-frequency = <12000000>;
> >> +		cdns,read-delay = <2>;
> >> +		spi-max-frequency = <100000000>;
> >>  		cdns,tshsl-ns = <1>;
> >>  		cdns,tsd2d-ns = <1>;
> >>  		cdns,tchsh-ns = <1>;
> >> --
> >> 2.47.2
> >>
>
> That is true, there's not much to explain. It works at delay 2 and
> 100MHz max. I tried many delay values with the pre-existing 12MHz max,
> they did not work. I don't have more information than that except what I
> wrote about on the U-Boot mailing list [1]
>
> [1]
> https://lore.kernel.org/u-boot/ZQ2PR01MB13072E932737DD9D65E3A250E637A@ZQ2PR01MB1307.CHNPR01.prod.partner.outlook.cn/
>
> The parameter training code in U-Boot when given max 100MHz has this at
> somewhere between delay 2 and delay 3, where delay 3 almost appears to
> work but there's data corruption, and delay 1 is totally non-functional.
> With all testing so far the delay 2 and max frequency 100MHz is reliable
> and also is a common occurrence in the Linux code base for similar qspi
> access.
>
> My testing methodology was pretty extensive and I wrote about the result
> on the U-Boot mailing list, however here we're taking the success of
> that in U-Boot and literally just applying it to Linux where it also
> happens to align with some common similar assignments for other boards
> in the devicetree code base. It works and there's no deeper insight here.

This explanation is fine, but it should be in the commit message so we can go
back and check why those numbers were chosen.

/Emil