Re: [PATCH 1/4] clk: sunxi-ng: h6-r: Add R_APB2_RSB clock and reset

[André Przywara]

On 15/12/2020 03:25, Samuel Holland wrote:
> On 12/14/20 8:57 AM, Maxime Ripard wrote:
>> Hi Samuel,
>>
>> On Sun, Dec 13, 2020 at 05:55:03PM -0600, Samuel Holland wrote:
>>> While no information about the H6 RSB controller is included in the
>>> datasheet or manual, the vendor BSP and power management blob both
>>> reference the RSB clock parent and register address. These values were
>>> verified by experimentation.
>>>
>>> Since this clock/reset are added late, the specifier is added at the end
>>> to maintain the existing DT binding. The code is kept in register order.
>>>
>>> Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>
>>> ---
>>>  drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c      | 5 +++++
>>>  drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h      | 2 +-
>>>  include/dt-bindings/clock/sun50i-h6-r-ccu.h | 1 +
>>>  include/dt-bindings/reset/sun50i-h6-r-ccu.h | 1 +
>>>  4 files changed, 8 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 50f8d1bc7046..56e351b513f3 100644
>>> --- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
>>> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
>>> @@ -91,6 +91,8 @@ static SUNXI_CCU_GATE(r_apb2_uart_clk,	"r-apb2-uart",	"r-apb2",
>>>  		      0x18c, BIT(0), 0);
>>>  static SUNXI_CCU_GATE(r_apb2_i2c_clk,	"r-apb2-i2c",	"r-apb2",
>>>  		      0x19c, BIT(0), 0);
>>> +static SUNXI_CCU_GATE(r_apb2_rsb_clk,	"r-apb2-rsb",	"r-apb2",
>>> +		      0x1bc, BIT(0), 0);
>>>  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",
>>> @@ -130,6 +132,7 @@ static struct ccu_common *sun50i_h6_r_ccu_clks[] = {
>>>  	&r_apb1_pwm_clk.common,
>>>  	&r_apb2_uart_clk.common,
>>>  	&r_apb2_i2c_clk.common,
>>> +	&r_apb2_rsb_clk.common,
>>>  	&r_apb1_ir_clk.common,
>>>  	&r_apb1_w1_clk.common,
>>>  	&ir_clk.common,
>>> @@ -147,6 +150,7 @@ static struct clk_hw_onecell_data sun50i_h6_r_hw_clks = {
>>>  		[CLK_R_APB1_PWM]	= &r_apb1_pwm_clk.common.hw,
>>>  		[CLK_R_APB2_UART]	= &r_apb2_uart_clk.common.hw,
>>>  		[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_W1]		= &r_apb1_w1_clk.common.hw,
>>>  		[CLK_IR]		= &ir_clk.common.hw,
>>> @@ -161,6 +165,7 @@ static struct ccu_reset_map sun50i_h6_r_ccu_resets[] = {
>>>  	[RST_R_APB1_PWM]	=  { 0x13c, BIT(16) },
>>>  	[RST_R_APB2_UART]	=  { 0x18c, BIT(16) },
>>>  	[RST_R_APB2_I2C]	=  { 0x19c, BIT(16) },
>>> +	[RST_R_APB2_RSB]	=  { 0x1bc, BIT(16) },
>>>  	[RST_R_APB1_IR]		=  { 0x1cc, BIT(16) },
>>>  	[RST_R_APB1_W1]		=  { 0x1ec, BIT(16) },
>>>  };
>>> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.h
>>> index 782117dc0b28..7e290b840803 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_W1 + 1)
>>> +#define CLK_NUMBER	(CLK_R_APB2_RSB + 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 76136132a13e..f46ec03848ca 100644
>>> --- a/include/dt-bindings/clock/sun50i-h6-r-ccu.h
>>> +++ b/include/dt-bindings/clock/sun50i-h6-r-ccu.h
>>> @@ -15,6 +15,7 @@
>>>  #define CLK_R_APB1_PWM		6
>>>  #define CLK_R_APB2_UART		7
>>>  #define CLK_R_APB2_I2C		8
>>> +#define CLK_R_APB2_RSB		13
>>>  #define CLK_R_APB1_IR		9
>>>  #define CLK_R_APB1_W1		10
>>>  
>>> diff --git a/include/dt-bindings/reset/sun50i-h6-r-ccu.h b/include/dt-bindings/reset/sun50i-h6-r-ccu.h
>>> index 01c84dba49a4..6fe199a7969d 100644
>>> --- a/include/dt-bindings/reset/sun50i-h6-r-ccu.h
>>> +++ b/include/dt-bindings/reset/sun50i-h6-r-ccu.h
>>> @@ -11,6 +11,7 @@
>>>  #define RST_R_APB1_PWM		2
>>>  #define RST_R_APB2_UART		3
>>>  #define RST_R_APB2_I2C		4
>>> +#define RST_R_APB2_RSB		7
>>>  #define RST_R_APB1_IR		5
>>>  #define RST_R_APB1_W1		6
>>
>> I think for the clock and reset binding, we'll want to sort by number.
>> It's fairly easy to miss otherwise and if we end up adding another one
>> it wouldn't be far fetched to assume the same indices would be used

I agree here. Admittedly this whole approach is really fragile. I ended
up with some tiny tool to check for consecutive numbers, reporting any
outliers (some gaps are legit). Of course the "-r" versions of the CCU
are not really a big issue, but with the 100+ clocks in the main CCU
this is a problem. Also it becomes ABI, so is hard to fix.

I guess there is no nice kernel CPP hack to re-use enums as preprocessor
symbols?

> I think GCC would complain about the duplicate array initialization in
> the driver, but I can move them for v2.

It doesn't with -Wall, you need -Wextra for that. I actually had a
subtle bug in my H616 CCU patch due to a double numbering.

Cheers,
Andre