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

[E Shattow <e@freeshell.de>]

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@freeshell.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/
> 
> 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