Re: [PATCH] clk: sunxi-ng: h3/h5: Model H3 CLK_DRAM as a fixed clock

[Samuel Holland]

On 12/28/22 23:22, Icenowy Zheng wrote:
> 在 2022-12-28星期三的 22:22 -0600，Samuel Holland写道：
>> The DRAM controller clock is only allowed to change frequency while
>> the
>> DRAM chips are in self-refresh. To support this, changes to the
>> CLK_DRAM
>> mux and divider have no effect until acknowledged by the memory
>> dynamic
>> frequency scaling (MDFS) hardware inside the DRAM controller. (There
>> is
>> a SDRCLK_UPD bit in DRAM_CFG_REG which should serve a similar
>> purpose,
>> but this bit actually does nothing.)
>>
>> However, the MDFS hardware in H3 appears to be broken. Triggering a
>> frequency change using the procedure from similar SoCs (A64/H5) hangs
>> the hardware. Additionally, the vendor BSP specifically avoids using
>> the
>> MDFS hardware on H3, instead performing all DRAM PHY parameter
>> updates
>> and resets in software.
>>
>> Thus, it is effectively impossible to change the CLK_DRAM
>> mux/divider,
>> so those features should not be modeled. Add CLK_SET_RATE_PARENT so
>> frequency changes apply to PLL_DDR instead.
>>
>> Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>
>> ---
>>
>>  drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c b/drivers/clk/sunxi-
>> ng/ccu-sun8i-h3.c
>> index d3fcb983c17c..bfebe8dbbe65 100644
>> --- a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
>> +++ b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
>> @@ -434,8 +434,13 @@ static SUNXI_CCU_GATE(usb_ohci2_clk,       "usb-
>> ohci2",    "osc24M",
>>  static SUNXI_CCU_GATE(usb_ohci3_clk,   "usb-ohci3",    "osc24M",
>>                       0x0cc, BIT(19), 0);
>>  
>> -static const char * const dram_parents[] = { "pll-ddr", "pll-
>> periph0-2x" };
>> -static SUNXI_CCU_M_WITH_MUX(dram_clk, "dram", dram_parents,
>> +/* H3 has broken MDFS hardware, so the mux/divider cannot be
>> changed. */
>> +static CLK_FIXED_FACTOR_HW(h3_dram_clk, "dram",
>> +                          &pll_ddr_clk.common.hw,
>> +                          1, 1, CLK_SET_RATE_PARENT |
>> CLK_IS_CRITICAL);
> 
> Should we do some sanity check on the values when probing CCU?

It is not necessary, because the register value is ignored. The register
is interpreted as if it contains 0x80000000, regardless of what you
write to it. So the parent/divider listed here will always be correct.

Regards,
Samuel