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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 1/15/25 01:35, Conor Dooley wrote:
On Wed, Jan 15, 2025 at 06:33:08AM +0000, Hal Feng wrote:
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@xxxxxxxxxxxx>
---
   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/

If you check the wording there, it says:
   To skip setting parent or rate of a clock its corresponding entry should be
   set to 0, or can be omitted if it is not followed by any non-zero entry.
Since all clocks are being set to 0 above, we should be in the "can be
omitted" case for the entire property, no? That would...

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.

Conor / Hal,  how does this work:

Are these the minimum and maximum values for CPU frequency, CPU CORE and PLL0 OUT ?


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>;
};

...make this one a reasonable change...

For u-boot, if there is no requirement to run u-boot at 1500MHz, just keep
&syscrg {
	assigned-clock-rates = <0>, <0>, <0>, <0>;
};

...but not this one.

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

Hal,

What voltage (and speed) has less heat output?

I think it must be configured to be *reliable first* as it is pictured being a developer board on a person's desk (with no extra accessory - no fans, heatsink, etc.) and for example doing work as of a Linux kernel compile with using all cores.

We should not force a configuration that will fail for the common situation. What voltage and clock speed is the best for continuous reliable operation of a JH7110 CPU with no added heatsink or fan and at ambient temperature?

Does CPU frequency range need to be given in devicetree? Can instead this be done with userspace tool or Linux subsystem at runtime?

The thermal dissipation of JH7110 CPU will be depend on the board layout and physical application. So, my opinion here: I think any minimum and maximum CPU frequency limit that is given by manufacturer recommendation of safe continuous operation is okay for dts include. If some part of this frequency range is not reliable (from undervolt, or overheat) then it is a "no" from me in Linux and in U-Boot. The fact of failing to boot (in U-Boot) because JH7110 CPU default voltage is not high enough for 1500MHz operation tells me this is a wrong idea or there is missing code in U-Boot to select a default sane clock speed and voltage.

-E




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux