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

[Andre Przywara <andre.przywara@arm.com>]

On Sun, 24 Apr 2022 19:05:02 -0500
Samuel Holland <samuel@sholland.org> wrote:

> Hi Andre,
> 
> On 4/24/22 6:36 PM, Andre Przywara wrote:
> > On Tue, 22 Feb 2022 21:22:07 -0600
> > Samuel Holland <samuel@sholland.org> 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.

Ouch, sorry for that, and thanks for the report. But that's a fragile
feature of the sunxi clock driver in general, isn't it? I wonder if we
should always end those arrays with:
	[CLK_NUMBER]		= NULL,
to be on the safe side. This would increase the flexible array
be one element, but that doesn't really harm, since .num limits
the traversal.
Otherwise we would always rely on the last clock to be always
explicitly listed, while we are fine with clocks in the middle missing.
Or in this case we could also say:
	[CLK_R_APB1_RTC]	= NULL,

But I guess since this particular clock is actually there in the H6, you
prefer it to be listed properly?

> 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.

CLK_IGNORE_UNUSED sounds better to me, in this case.

I will try and test this on an H6 as well, this time ;-)

Thanks,
Andre

> 
> 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@arm.com>
> >>> ---
> >>>  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_ */
> >>>     
> >>
> >>  
> >   
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel