Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Defining `mbus_clk` with a 0-width `P` prescaler causes undefined behavior in `ccu_mp_set_rate()`, leading to the critical memory bus clock register being completely cleared.
- [High] The MBUS clock requires an update bit to commit rate changes, but it is defined using `ccu_mp` operations which completely ignore the `CCU_FEATURE_UPDATE_BIT` flag.
--
--- Patch [5]: [PATCH v2 5/8] clk: sunxi-ng: a733: Add bus clocks support ---
commit 40f96b18647c7ab7dc42f9a7456046beab510ac6
Author: Junhui Liu <junhui.liu@pigmoral.tech>
clk: sunxi-ng: a733: Add bus clocks support
This commit adds the essential bus clocks in the Allwinner A733 CCU,
including AHB, APB0, APB1, APB_UART, and MBUS. It also marks the MBUS
clock as critical to ensure the memory bus remains operational.
> diff --git a/drivers/clk/sunxi-ng/ccu-sun60i-a733.c b/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
> --- a/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
[ ... ]
> +static const struct clk_parent_data mbus_parents[] = {
> + { .hw = &sys_24M_clk.hw },
> + { .hw = &pll_periph1_600M_clk.hw },
> + { .hw = &pll_ddr_clk.common.hw },
> + { .hw = &pll_periph1_480M_clk.common.hw },
> + { .hw = &pll_periph1_400M_clk.hw },
> + { .hw = &pll_npu_clk.common.hw },
> +};
> +static SUNXI_CCU_MP_DATA_WITH_MUX_GATE_FEAT(mbus_clk, "mbus", mbus_parents, 0x588,
> + 0, 5, /* M */
> + 0, 0, /* no P */
[Severity: Critical]
This is a pre-existing issue in the framework, but does defining mbus_clk
with a 0-width P prescaler cause undefined behavior when the rate is set?
When ccu_mp_set_rate() is called in drivers/clk/sunxi-ng/ccu_mp.c, it
evaluates the mask for the P prescaler:
drivers/clk/sunxi-ng/ccu_mp.c:ccu_mp_set_rate() {
...
reg &= ~GENMASK(cmp->p.width + cmp->p.shift - 1, cmp->p.shift);
...
}
Since p.width is 0, this expands to GENMASK(-1, 0). On 64-bit architectures,
the shift by 64 is masked to 0, which makes ~GENMASK evaluate to 0 and clears
the entire mbus_clk register, including the critical clock gate bit.
Could this lead to a system crash during boot or runtime frequency scaling?
> + 24, 3, /* mux */
> + BIT(31), /* gate */
> + CLK_IS_CRITICAL,
> + CCU_FEATURE_UPDATE_BIT);
[Severity: High]
This is also a pre-existing issue in the framework, but will the hardware
actually commit rate changes for this clock?
The commit message notes that the hardware requires bit 27 to be set for
the configuration to take effect. However, ccu_mp_set_rate() does not
process the CCU_FEATURE_UPDATE_BIT flag before writing the register:
drivers/clk/sunxi-ng/ccu_mp.c:ccu_mp_set_rate() {
...
writel(reg, cmp->common.base + cmp->common.reg);
spin_unlock_irqrestore(cmp->common.lock, flags);
...
}
Since the update bit is never toggled by the ccu_mp operations, does this
leave the software state permanently out of sync with the hardware state
when the mbus_clk rate is configured?
@@ -19,6 +19,7 @@
#include "ccu_common.h"
#include "ccu_div.h"
+#include "ccu_mp.h"
#include "ccu_mult.h"
#include "ccu_nkmp.h"
#include "ccu_nm.h"
@@ -69,6 +70,16 @@ static const struct clk_hw *pll_ref_hws[] = {
&pll_ref_clk.common.hw
};
+/*
+ * There is a non-software-configurable mux selecting between the DCXO and the
+ * PLL_REF in hardware, whose output is fed to the sys-24M clock. Although both
+ * sys-24M and pll-ref are fixed at 24 MHz, define a 1:1 fixed factor clock to
+ * provide logical separation:
+ * - pll-ref is dedicated to feeding other PLLs
+ * - sys-24M serves as reference clock for downstream functional modules
+ */
+static CLK_FIXED_FACTOR_HWS(sys_24M_clk, "sys-24M", pll_ref_hws, 1, 1, 0);
+
#define SUN60I_A733_PLL_DDR_REG 0x020
static struct ccu_nkmp pll_ddr_clk = {
.enable = BIT(27),
@@ -375,6 +386,73 @@ static SUNXI_CCU_M_HWS(pll_de_4x_clk, "pll-de-4x", pll_de_hws,
static SUNXI_CCU_M_HWS(pll_de_3x_clk, "pll-de-3x", pll_de_hws,
SUN60I_A733_PLL_DE_REG, 16, 3, 0);
+/**************************************************************************
+ * bus clocks *
+ **************************************************************************/
+
+static const struct clk_parent_data ahb_apb_parents[] = {
+ { .hw = &sys_24M_clk.hw },
+ { .fw_name = "losc" },
+ { .fw_name = "iosc" },
+ { .hw = &pll_periph0_600M_clk.hw },
+};
+
+static SUNXI_CCU_M_DATA_WITH_MUX(ahb_clk, "ahb", ahb_apb_parents, 0x500,
+ 0, 5, /* M */
+ 24, 2, /* mux */
+ 0);
+
+static SUNXI_CCU_M_DATA_WITH_MUX(apb0_clk, "apb0", ahb_apb_parents, 0x510,
+ 0, 5, /* M */
+ 24, 2, /* mux */
+ 0);
+
+static SUNXI_CCU_M_DATA_WITH_MUX(apb1_clk, "apb1", ahb_apb_parents, 0x518,
+ 0, 5, /* M */
+ 24, 2, /* mux */
+ 0);
+
+static const struct clk_parent_data apb_uart_parents[] = {
+ { .hw = &sys_24M_clk.hw },
+ { .fw_name = "losc" },
+ { .fw_name = "iosc" },
+ { .hw = &pll_periph0_600M_clk.hw },
+ { .hw = &pll_periph0_480M_clk.common.hw },
+};
+static SUNXI_CCU_M_DATA_WITH_MUX(apb_uart_clk, "apb-uart", apb_uart_parents, 0x538,
+ 0, 5, /* M */
+ 24, 3, /* mux */
+ 0);
+
+static const struct clk_parent_data trace_parents[] = {
+ { .hw = &sys_24M_clk.hw },
+ { .fw_name = "losc" },
+ { .fw_name = "iosc" },
+ { .hw = &pll_periph0_300M_clk.hw },
+ { .hw = &pll_periph0_400M_clk.hw },
+};
+static SUNXI_CCU_M_DATA_WITH_MUX_GATE(trace_clk, "trace", trace_parents, 0x540,
+ 0, 5, /* M */
+ 24, 3, /* mux */
+ BIT(31), /* gate */
+ 0);
+
+static const struct clk_parent_data mbus_parents[] = {
+ { .hw = &sys_24M_clk.hw },
+ { .hw = &pll_periph1_600M_clk.hw },
+ { .hw = &pll_ddr_clk.common.hw },
+ { .hw = &pll_periph1_480M_clk.common.hw },
+ { .hw = &pll_periph1_400M_clk.hw },
+ { .hw = &pll_npu_clk.common.hw },
+};
+static SUNXI_CCU_MP_DATA_WITH_MUX_GATE_FEAT(mbus_clk, "mbus", mbus_parents, 0x588,
+ 0, 5, /* M */
+ 0, 0, /* no P */
+ 24, 3, /* mux */
+ BIT(31), /* gate */
+ CLK_IS_CRITICAL,
+ CCU_FEATURE_UPDATE_BIT);
+
/*
* Contains all clocks that are controlled by a hardware register. They
* have a (sunxi) .common member, which needs to be initialised by the common
@@ -411,11 +489,18 @@ static struct ccu_common *sun60i_a733_ccu_clks[] = {
&pll_de_12x_clk.common,
&pll_de_4x_clk.common,
&pll_de_3x_clk.common,
+ &ahb_clk.common,
+ &apb0_clk.common,
+ &apb1_clk.common,
+ &apb_uart_clk.common,
+ &trace_clk.common,
+ &mbus_clk.common,
};
static struct clk_hw_onecell_data sun60i_a733_hw_clks = {
.hws = {
[CLK_PLL_REF] = &pll_ref_clk.common.hw,
+ [CLK_SYS_24M] = &sys_24M_clk.hw,
[CLK_PLL_DDR] = &pll_ddr_clk.common.hw,
[CLK_PLL_PERIPH0_4X] = &pll_periph0_4x_clk.common.hw,
[CLK_PLL_PERIPH0_2X] = &pll_periph0_2x_clk.common.hw,
@@ -457,6 +542,12 @@ static struct clk_hw_onecell_data sun60i_a733_hw_clks = {
[CLK_PLL_DE_12X] = &pll_de_12x_clk.common.hw,
[CLK_PLL_DE_4X] = &pll_de_4x_clk.common.hw,
[CLK_PLL_DE_3X] = &pll_de_3x_clk.common.hw,
+ [CLK_AHB] = &ahb_clk.common.hw,
+ [CLK_APB0] = &apb0_clk.common.hw,
+ [CLK_APB1] = &apb1_clk.common.hw,
+ [CLK_APB_UART] = &apb_uart_clk.common.hw,
+ [CLK_TRACE] = &trace_clk.common.hw,
+ [CLK_MBUS] = &mbus_clk.common.hw,
},
.num = CLK_FANOUT3 + 1,
};