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
