| Message ID | 20201213235506.25201-2-samuel@sholland.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Allwinner H6 RSB support | expand |
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@sholland.org> > --- > 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 maxime
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@sholland.org> >> --- >> 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 think GCC would complain about the duplicate array initialization in the driver, but I can move them for v2. Cheers, Samuel
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@sholland.org> >>> --- >>> 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
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
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@sholland.org> --- 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(-)