Re: [PATCH v10 01/18] clk: sunxi-ng: h616-r: Add RTC gate clock

[Samuel Holland]

Hi Andre,

On 4/24/22 6:36 PM, Andre Przywara wrote:
> On Tue, 22 Feb 2022 21:22:07 -0600
> Samuel Holland <samuel@xxxxxxxxxxxx> wrote:
> 
> Hi Samuel,
> 
>> On 2/11/22 6:26 AM, Andre Przywara wrote:
>>> The H616 features an (undocumented) bus clock gate for accessing the RTC
>>> registers. This seems to be enabled at reset (or by the BootROM), but is
>>> there anyway.
>>> Since the new RTC clock binding for the H616 requires this "bus" clock
>>> to be specified in the DT, add this to R_CCU clock driver and expose it
>>> on the DT side with a new number.  
>>
>> It would be good to note why you didn't add this clock to H6, even though it
>> exists in that hardware.
> 
> What explanation do you prefer here? The main reason I expose this is
> because of the H616 binding, so this is not required for the H6.
> Plus is would break compatibility with older kernels, which is not so
> much an issue for the H616.
> Do you want to expose the clock on the H6 side as well, and mark it
> as CLK_IS_CRITICAL there? I guess otherwise it would get turned off.
> Or were you just after some kind of rationale as above, for the
> commit log records?

Today I found out that this commit breaks booting on H6 because the clock is not
added there. clk_hw_onecell_data::hws is a flexible array member, so
incrementing CLK_NUMBER doesn't actually make the array larger. That means .num
gets interpreted as .hws[CLK_R_APB1_RTC], and that ends with a NULL dereference.

The easiest solution seems to be adding the clock on H6 as well. I think
CLK_IGNORE_UNUSED is sufficient, but CLK_IS_CRITICAL would work as well.

The rationale I'm looking for is something along the lines of "the H6 binding
prevents us from referencing this clock from the devicetree, and the way the
driver is written prevents us from changing the binding."

>>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>>> ---
>>>  drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c      | 4 ++++
>>>  drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h      | 2 +-
>>>  include/dt-bindings/clock/sun50i-h6-r-ccu.h | 1 +
>>>  3 files changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
>>> index 712e103382d8..26fb092f6df6 100644
>>> --- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
>>> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
>>> @@ -98,6 +98,8 @@ static SUNXI_CCU_GATE(r_apb1_ir_clk,	"r-apb1-ir",	"r-apb1",
>>>  		      0x1cc, BIT(0), 0);
>>>  static SUNXI_CCU_GATE(r_apb1_w1_clk,	"r-apb1-w1",	"r-apb1",
>>>  		      0x1ec, BIT(0), 0);
>>> +static SUNXI_CCU_GATE(r_apb1_rtc_clk,	"r-apb1-rtc",	"r-apb1",
>>> +		      0x20c, BIT(0), 0);  
>>
>> All of the documentation I have found (manuals, A100 driver, BSP D1 driver)
>> points to this clock coming off of R_AHB, not R_APB1.
> 
> Really, can you provide some pointer? In the H616 manual I see
> AHBS->AHB2APB->APBS1BUS->RTC, next to the other R_ peripherals. Also
> typically *register access* is done via APB busses, not AHB.
> Is any of those documentation sources actually reliable? And
> regardless, the AHB vs. APB parenthood is mostly academic, isn't it?

You are right. I'm looking at the "RTC Application Diagram". For both H6 and
H616, the diagram and the caption have register access via APB1. A100/R329/D1
have register access via AHBS (and this matches the CCU bus tree diagram). So it
looks like this was changed for A100.

Regards,
Samuel

>>>  
>>>  /* Information of IR(RX) mod clock is gathered from BSP source code */
>>>  static const char * const r_mod0_default_parents[] = { "osc32k", "osc24M" };
>>> @@ -147,6 +149,7 @@ static struct ccu_common *sun50i_h616_r_ccu_clks[] = {
>>>  	&r_apb2_i2c_clk.common,
>>>  	&r_apb2_rsb_clk.common,
>>>  	&r_apb1_ir_clk.common,
>>> +	&r_apb1_rtc_clk.common,
>>>  	&ir_clk.common,
>>>  };
>>>  
>>> @@ -179,6 +182,7 @@ static struct clk_hw_onecell_data sun50i_h616_r_hw_clks = {
>>>  		[CLK_R_APB2_I2C]	= &r_apb2_i2c_clk.common.hw,
>>>  		[CLK_R_APB2_RSB]	= &r_apb2_rsb_clk.common.hw,
>>>  		[CLK_R_APB1_IR]		= &r_apb1_ir_clk.common.hw,
>>> +		[CLK_R_APB1_RTC]	= &r_apb1_rtc_clk.common.hw,
>>>  		[CLK_IR]		= &ir_clk.common.hw,
>>>  	},
>>>  	.num	= CLK_NUMBER,
>>> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h
>>> index 7e290b840803..10e9b66afc6a 100644
>>> --- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h
>>> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h
>>> @@ -14,6 +14,6 @@
>>>  
>>>  #define CLK_R_APB2	3
>>>  
>>> -#define CLK_NUMBER	(CLK_R_APB2_RSB + 1)
>>> +#define CLK_NUMBER	(CLK_R_APB1_RTC + 1)
>>>  
>>>  #endif /* _CCU_SUN50I_H6_R_H */
>>> diff --git a/include/dt-bindings/clock/sun50i-h6-r-ccu.h b/include/dt-bindings/clock/sun50i-h6-r-ccu.h
>>> index 890368d252c4..a96087abc86f 100644
>>> --- a/include/dt-bindings/clock/sun50i-h6-r-ccu.h
>>> +++ b/include/dt-bindings/clock/sun50i-h6-r-ccu.h
>>> @@ -22,5 +22,6 @@
>>>  #define CLK_W1			12
>>>  
>>>  #define CLK_R_APB2_RSB		13
>>> +#define CLK_R_APB1_RTC		14
>>>  
>>>  #endif /* _DT_BINDINGS_CLK_SUN50I_H6_R_CCU_H_ */
>>>   
>>
>>
>