linux-kernel - RE: [PATCH v1 1/5] riscv: dts: starfive: jh7110-common: replace syscrg clock assignments

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <ZQ2PR01MB1307A9E106AE06E144875D6EE619A@ZQ2PR01MB1307.CHNPR01.prod.partner.outlook.cn>
Date: Wed, 15 Jan 2025 06:33:08 +0000
From: Hal Feng <hal.feng@...rfivetech.com>
To: Conor Dooley <conor@...nel.org>, E Shattow <e@...eshell.de>
CC: Emil Renner Berthing <kernel@...il.dk>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Paul Walmsley
	<paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, Albert Ou
	<aou@...s.berkeley.edu>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-riscv@...ts.infradead.org"
	<linux-riscv@...ts.infradead.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, Minda Chen <minda.chen@...rfivetech.com>
Subject: RE: [PATCH v1 1/5] riscv: dts: starfive: jh7110-common: replace
 syscrg clock assignments

> On 07.01.25 04:08, Conor Dooley wrote: 
> On Sat, Jan 04, 2025 at 01:04:30PM -0800, E Shattow wrote:
> > Hi, Conor  (added CC: Minda Chen, Hal Feng)
> >
> > On 1/4/25 10:33, Conor Dooley wrote:
> > > On Thu, Jan 02, 2025 at 11:45:07AM -0800, E Shattow wrote:
> > > > Replace syscrg assignments of clocks, clock parents, and rates,
> > > > for compatibility with downstream boot loader SPL secondary
> > > > program loader.
> > > >
> > > > Signed-off-by: E Shattow <e@...eshell.de>
> > > > ---
> > > >   arch/riscv/boot/dts/starfive/jh7110-common.dtsi | 12 +++++++++---
> > > >   1 file changed, 9 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> > > > b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> > > > index 48fb5091b817..55c6743100a7 100644
> > > > --- a/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> > > > +++ b/arch/riscv/boot/dts/starfive/jh7110-common.dtsi
> > > > @@ -359,9 +359,15 @@ spi_dev0: spi@0 {
> > > >   };
> > > >   &syscrg {
> > > > -	assigned-clocks = <&syscrg JH7110_SYSCLK_CPU_CORE>,
> > > > -			  <&pllclk JH7110_PLLCLK_PLL0_OUT>;
> > > > -	assigned-clock-rates = <500000000>, <1500000000>;
> > > > +	assigned-clocks = <&syscrg JH7110_SYSCLK_CPU_ROOT>,
> > > > +			  <&syscrg JH7110_SYSCLK_BUS_ROOT>,
> > > > +			  <&syscrg JH7110_SYSCLK_PERH_ROOT>,
> > > > +			  <&syscrg JH7110_SYSCLK_QSPI_REF>;
> > > > +	assigned-clock-parents = <&pllclk JH7110_PLLCLK_PLL0_OUT>,
> > > > +				 <&pllclk JH7110_PLLCLK_PLL2_OUT>,
> > > > +				 <&pllclk JH7110_PLLCLK_PLL2_OUT>,
> > > > +				 <&syscrg JH7110_SYSCLK_QSPI_REF_SRC>;
> > > > +	assigned-clock-rates = <0>, <0>, <0>, <0>;
> > >
> > > Why is assigned rates here 0s, rather than the property just removed?
> > >
> > > >   };
> > > >   &sysgpio {
> > > > --
> > > > 2.45.2
> > > >
> >
> > Assigned rates all zeroes is how it is in U-Boot. Removing the
> > assigned-clock-rates property as suggested does work in U-Boot and
> > Linux both.
> >
> > For context, U-Boot fails when replacing assigned-clocks to
> > JH7110_SYSCLK_CPU_CORE (500MHz) and JH7110_PLLCLK_PLL0_OUT
> (1500MHz)
> > from Linux. So I tried to merge all properties together and in testing
> > then U-Boot failed (or I did it wrong). However replacing the Linux
> > properties with the U-Boot configuration (above) on Linux does work for
> both.
> >
> > I do not know if this is correct but I can test any suggestions and
> > report if they are working.
> >
> > Do these changes make sense? Are there other variations I should test?
> 
> I'd like the commit message to at least explain why these clocks need to be
> set to zero (I assume that means disabled?). Maybe the StarFive folks know
> why it is required?

Here "assigned-clock-rates = <0>, ..." means skipping setting clock rates.
You can refer to
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/clock/clock.yaml/

Linux here setting JH7110_SYSCLK_CPU_CORE to 500MHz and JH7110_PLLCLK_PLL0_OUT
to 1500MHz are for increasing the CPU frequency to 1500MHz.

VF2 u-boot is still running at 1000MHz now. You failed to set JH7110_PLLCLK_PLL0_OUT
to 1500MHz, because CPU power supply voltage needs to be increased before running at
1500MHz.

I think a better choice now is changing Linux device tree as follows:

&syscrg {
	assigned-clocks = <&syscrg JH7110_SYSCLK_CPU_ROOT>,
			  <&syscrg JH7110_SYSCLK_BUS_ROOT>,
			  <&syscrg JH7110_SYSCLK_PERH_ROOT>,
			  <&syscrg JH7110_SYSCLK_QSPI_REF>,
			  <&syscrg JH7110_SYSCLK_CPU_CORE>,
			  <&pllclk JH7110_PLLCLK_PLL0_OUT>;
	assigned-clock-parents = <&pllclk JH7110_PLLCLK_PLL0_OUT>,
				 <&pllclk JH7110_PLLCLK_PLL2_OUT>,
				 <&pllclk JH7110_PLLCLK_PLL2_OUT>,
				 <&syscrg JH7110_SYSCLK_QSPI_REF_SRC>;
	assigned-clock-rates = <0>, <0>, <0>, <0>, <500000000>, <1500000000>;
};

For u-boot, if there is no requirement to run u-boot at 1500MHz, just keep
&syscrg {
	assigned-clock-rates = <0>, <0>, <0>, <0>;
};
in u-boot device tree. If we need running 1500MHz in u-boot, I will send a patch
to implement it and then &syscrg{...} in u-boot device tree can be dropped.

Best regards,
Hal