diff mbox series

[v2,5/6] clk: sunxi-ng: sun6i-rtc: split main oscillator div and gate.

Message ID 20260629-a733-rtc-v2-5-7b72112784f8@baylibre.com (mailing list archive)
State New
Headers
Series clk: sun6i-rtc: Add support for Allwinner A733 SoC |

Commit Message

Jerome Brunet June 29, 2026, 12:42 p.m. UTC 
On the a733 the "osc24M-32k" clock has the same gate bits as the previously
supported SoC but a different divider implementation.

Instead of a fixed 750 divider, the divider is selected based on the
rate of the oscillator. It can be seen as a simple read-only divider.

To easily replace the divider part depending the SoC, split the divider
and gate into two separate clock entities.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/sunxi-ng/ccu-sun6i-rtc.c | 20 +++++++++++---------
 drivers/clk/sunxi-ng/ccu-sun6i-rtc.h |  3 ++-
 2 files changed, 13 insertions(+), 10 deletions(-)

Comments

sashiko-bot@kernel.org June 29, 2026, 1:12 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the error path in `sunxi_ccu_probe` unregisters clocks using the wrong loop limit, causing either memory leaks or out-of-bounds array reads depending on the struct array sizes of the specific CCU driver.
--

commit fe4850822f9e2e522aaac703e02ecb786fb47f26
Author: Jerome Brunet <jbrunet@baylibre.com>

clk: sunxi-ng: sun6i-rtc: split main oscillator div and gate.

This commit splits the oscillator divider and gate into two separate clock 
entities to support the A733 SoC. It introduces a new read-only divider 
based on the oscillator rate to replace the fixed 750 divider.

> diff --git a/drivers/clk/sunxi-ng/ccu-sun6i-rtc.c b/drivers/clk/sunxi-ng/ccu-sun6i-rtc.c
> index b24c8b196e664..25dd87e78eb7a 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun6i-rtc.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun6i-rtc.c

[ ... ]

> @@ -286,6 +287,7 @@ static struct clk_hw_onecell_data sun6i_rtc_ccu_hw_clks = {
>  		[CLK_EXT_OSC32K_GATE]	= &ext_osc32k_gate_clk.common.hw,
>  		[CLK_OSC24M_32K]	= &osc24M_32k_clk.common.hw,
>  		[CLK_RTC_32K]		= &rtc_32k_clk.common.hw,
> +		[CLK_OSC24M_32K_DIV]	= &osc24M_32k_div_clk.hw,
>  	},
>  };

[Severity: High]
This isn't a bug introduced by this patch, but does adding an extra clock
here expose an issue in the error handling path of sunxi_ccu_probe()?

In drivers/clk/sunxi-ng/ccu_common.c:sunxi_ccu_probe(), the err_clk_unreg
path unregisters clocks using a loop limit that appears incorrect:

err_clk_unreg:
	while (--i >= 0) {
		struct clk_hw *hw = desc->hw_clks->hws[i];

The variable 'i' incorrectly retains the value of 'desc->num_ccu_clks' from
the preceding loop, rather than using the correct bound 'desc->hw_clks->num'.

For drivers where num_ccu_clks < hw_clks->num (such as sun6i-rtc after this
patch), will this skip unregistering the extra clocks if probe fails? And for
drivers where num_ccu_clks > hw_clks->num, could this read past the end of
the hws array?
Jerome Brunet June 29, 2026, 2:15 p.m. UTC | #2 
On lun. 29 juin 2026 at 13:12, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the error path
> in `sunxi_ccu_probe` unregisters clocks using the wrong loop limit, causing
> either memory leaks or out-of-bounds array reads depending on the struct
> array sizes of the specific CCU driver.
> --
>
> commit fe4850822f9e2e522aaac703e02ecb786fb47f26
> Author: Jerome Brunet <jbrunet@baylibre.com>
>
> clk: sunxi-ng: sun6i-rtc: split main oscillator div and gate.
>
> This commit splits the oscillator divider and gate into two separate clock 
> entities to support the A733 SoC. It introduces a new read-only divider 
> based on the oscillator rate to replace the fixed 750 divider.
>
>> diff --git a/drivers/clk/sunxi-ng/ccu-sun6i-rtc.c b/drivers/clk/sunxi-ng/ccu-sun6i-rtc.c
>> index b24c8b196e664..25dd87e78eb7a 100644
>> --- a/drivers/clk/sunxi-ng/ccu-sun6i-rtc.c
>> +++ b/drivers/clk/sunxi-ng/ccu-sun6i-rtc.c
>
> [ ... ]
>
>> @@ -286,6 +287,7 @@ static struct clk_hw_onecell_data sun6i_rtc_ccu_hw_clks = {
>>  		[CLK_EXT_OSC32K_GATE]	= &ext_osc32k_gate_clk.common.hw,
>>  		[CLK_OSC24M_32K]	= &osc24M_32k_clk.common.hw,
>>  		[CLK_RTC_32K]		= &rtc_32k_clk.common.hw,
>> +		[CLK_OSC24M_32K_DIV]	= &osc24M_32k_div_clk.hw,
>>  	},
>>  };
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does adding an extra clock
> here expose an issue in the error handling path of sunxi_ccu_probe()?
>
> In drivers/clk/sunxi-ng/ccu_common.c:sunxi_ccu_probe(), the err_clk_unreg
> path unregisters clocks using a loop limit that appears incorrect:
>
> err_clk_unreg:
> 	while (--i >= 0) {
> 		struct clk_hw *hw = desc->hw_clks->hws[i];
>
> The variable 'i' incorrectly retains the value of 'desc->num_ccu_clks' from
> the preceding loop, rather than using the correct bound 'desc->hw_clks->num'.
>
> For drivers where num_ccu_clks < hw_clks->num (such as sun6i-rtc after this
> patch), will this skip unregistering the extra clocks if probe fails? And for
> drivers where num_ccu_clks > hw_clks->num, could this read past the end of
> the hws array?

Good catch, using a different iterator should fix the problem
diff mbox series

Patch

diff --git a/drivers/clk/sunxi-ng/ccu-sun6i-rtc.c b/drivers/clk/sunxi-ng/ccu-sun6i-rtc.c
index b24c8b196e66..25dd87e78eb7 100644
--- a/drivers/clk/sunxi-ng/ccu-sun6i-rtc.c
+++ b/drivers/clk/sunxi-ng/ccu-sun6i-rtc.c
@@ -218,17 +218,18 @@  static const struct clk_parent_data osc24M[] = {
 	{ .fw_name = "hosc", .name = "osc24M" }
 };
 
-static struct ccu_gate osc24M_32k_clk = {
-	.enable	= BIT(16),
-	.common	= {
-		.reg		= LOSC_OUT_GATING_REG,
-		.prediv		= 750,
-		.features	= CCU_FEATURE_ALL_PREDIV,
-		.hw.init	= CLK_HW_INIT_PARENTS_DATA("osc24M-32k", osc24M,
-							   &ccu_gate_ops, 0),
-	},
+static struct clk_fixed_factor osc24M_32k_div_clk = {
+	.mult = 1,
+	.div = 750,
+	.hw.init = CLK_HW_INIT_PARENTS_DATA("osc24M-32k-div",
+					    osc24M,
+					    &clk_fixed_factor_ops,
+					    0),
 };
 
+static SUNXI_CCU_GATE_HW(osc24M_32k_clk, "osc24M-32k", &osc24M_32k_div_clk.hw,
+			 LOSC_OUT_GATING_REG, BIT(16), 0);
+
 static const struct clk_hw *rtc_32k_parents[] = {
 	&osc32k_clk.common.hw,
 	&osc24M_32k_clk.common.hw
@@ -286,6 +287,7 @@  static struct clk_hw_onecell_data sun6i_rtc_ccu_hw_clks = {
 		[CLK_EXT_OSC32K_GATE]	= &ext_osc32k_gate_clk.common.hw,
 		[CLK_OSC24M_32K]	= &osc24M_32k_clk.common.hw,
 		[CLK_RTC_32K]		= &rtc_32k_clk.common.hw,
+		[CLK_OSC24M_32K_DIV]	= &osc24M_32k_div_clk.hw,
 	},
 };
 
diff --git a/drivers/clk/sunxi-ng/ccu-sun6i-rtc.h b/drivers/clk/sunxi-ng/ccu-sun6i-rtc.h
index 9ae821fc2599..ab7b92b47f59 100644
--- a/drivers/clk/sunxi-ng/ccu-sun6i-rtc.h
+++ b/drivers/clk/sunxi-ng/ccu-sun6i-rtc.h
@@ -9,7 +9,8 @@ 
 #define CLK_EXT_OSC32K_GATE	4
 #define CLK_OSC24M_32K		5
 #define CLK_RTC_32K		6
+#define CLK_OSC24M_32K_DIV	7
 
-#define CLK_NUMBER		(CLK_RTC_32K + 1)
+#define CLK_NUMBER		(CLK_OSC24M_32K_DIV + 1)
 
 #endif /* _CCU_SUN6I_RTC_H */