On 09:46 Fri 14 Aug , Junhui Liu wrote:
> The K1 clock driver modeled the bus clock enable bit as part of the
> functional clock for regular TWSI controllers. The I2C driver then only
> enabled the functional clock, ignoring the separate bus clock described
> by the device tree. Although this happened to work, it did not match the
> hardware clock topology or the device tree binding.
>
> Model the functional and bus clocks separately and make the I2C driver
> acquire and enable all clocks provided by the device tree. Also add the
> TWSI2 and TWSI8 bus clocks, together with their APB clock dependencies,
> to the SPL clock tree.
>
> Keep the combined functional and bus gate for TWSI8, whose clock control
> register is write-only, and retain its bus clock as a fixed-factor
> placeholder.
>
> The clock-provider and I2C-consumer changes should be applied together,
> as either change on its own would leave I2C unusable.
>
> Fixes: 3aa2882a3e1a ("clk: spacemit: Add support for K1 SoC")
> Fixes: 271546fb8e54 ("i2c: k1: add I2C driver support")
> Signed-off-by: Junhui Liu <[email protected]>
> ---
> drivers/clk/spacemit/clk-k1.c | 41 +++++++++++++++++++++++++++++++++--------
> drivers/i2c/k1_i2c.c | 10 ++++------
I'd suggest to have separate patch for clock and i2c changes, as they
belong to different subsystem
> 2 files changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/clk/spacemit/clk-k1.c b/drivers/clk/spacemit/clk-k1.c
> index 07adc126ee39..20b8595aa3c4 100644
> --- a/drivers/clk/spacemit/clk-k1.c
> +++ b/drivers/clk/spacemit/clk-k1.c
> @@ -154,11 +154,27 @@ CCU_GATE_DEFINE(CLK_PLL1_409P6, pll1_d6_409p6,
> pll1_d6_409p6, "pll1_d6",
> MPMU_ACGR, BIT(0), 0);
> CCU_GATE_DEFINE(CLK_PLL1_307P2, pll1_d8_307p2, pll1_d8_307p2, "pll1_d8",
> MPMU_ACGR, BIT(13), 0);
> +CCU_FACTOR_GATE_DEFINE(CLK_PLL1_102P4, pll1_d24_102p4, pll1_d24_102p4,
> + "pll1_d8", MPMU_ACGR, BIT(12), 3, 1);
> +CCU_FACTOR_GATE_DEFINE(CLK_PLL1_51P2, pll1_d48_51p2, pll1_d48_51p2,
> + "pll1_d8", MPMU_ACGR, BIT(7), 6, 1);
> +CCU_FACTOR_GATE_DEFINE(CLK_PLL1_25P6, pll1_d96_25p6, pll1_d96_25p6,
> + "pll1_d8", MPMU_ACGR, BIT(4), 12, 1);
> CCU_FACTOR_GATE_DEFINE(CLK_PLL1_31P5, pll1_d78_31p5, pll1_d78_31p5,
> "pll1_d4", MPMU_ACGR, BIT(6), 39, 2);
> CCU_DDN_DEFINE(CLK_SLOW_UART2, slow_uart2_48, slow_uart2_48,
> "pll1_d4_614p4", MPMU_SUCCR_1,
> CCU_DDN_MASK(16, 13), 16, CCU_DDN_MASK(0, 13), 0, 2, 0);
> +
> +static const char * const apb_parents[] = {
> + "pll1_d96_25p6",
> + "pll1_d48_51p2",
> + "pll1_d96_25p6",
> + "pll1_d24_102p4",
> +};
> +
> +CCU_MUX_DEFINE(CLK_APB, apb_clk, apb_clk, apb_parents,
> ARRAY_SIZE(apb_parents),
> + MPMU_APBCSCR, 0, 2, 0);
> #else
> CCU_GATE_DEFINE(CLK_PLL1_307P2, pll1_d8_307p2, pll1_d8_307p2, "pll1_d8",
> MPMU_ACGR, BIT(13), 0);
> @@ -298,7 +314,7 @@ static const char * const twsi_parents[] = {
>
> CCU_MUX_GATE_DEFINE(CLK_TWSI2, twsi2_clk, twsi2_clk, twsi_parents,
> ARRAY_SIZE(twsi_parents), APBC_TWSI2_CLK_RST,
> - 4, 3, BIT(1) | BIT(0), 0);
> + 4, 3, BIT(1), 0);
> /*
> * APBC_TWSI8_CLK_RST has a quirk that reading always results in zero.
> * Combine functional and bus bits together as a gate to avoid sharing the
> @@ -306,6 +322,9 @@ CCU_MUX_GATE_DEFINE(CLK_TWSI2, twsi2_clk, twsi2_clk,
> twsi_parents,
> */
> CCU_GATE_DEFINE(CLK_TWSI8, twsi8_clk, twsi8_clk, "pll1_d78_31p5",
> APBC_TWSI8_CLK_RST, BIT(1) | BIT(0), 0);
> +CCU_GATE_DEFINE(CLK_TWSI2_BUS, twsi2_bus_clk, twsi2_bus_clk, "apb_clk",
> + APBC_TWSI2_CLK_RST, BIT(0), 0);
> +CCU_FACTOR_DEFINE(CLK_TWSI8_BUS, twsi8_bus_clk, twsi8_bus_clk, "apb_clk", 1,
> 1);
>
> #else
> static const char * const uart_clk_parents[] = {
> @@ -326,7 +345,7 @@ static const char * const twsi_parents[] = {
>
> CCU_MUX_GATE_DEFINE(CLK_TWSI2, twsi2_clk, twsi2_clk, twsi_parents,
> ARRAY_SIZE(twsi_parents), APBC_TWSI2_CLK_RST,
> - 4, 3, BIT(1) | BIT(0), 0);
> + 4, 3, BIT(1), 0);
> /*
> * APBC_TWSI8_CLK_RST has a quirk that reading always results in zero.
> * Combine functional and bus bits together as a gate to avoid sharing the
> @@ -448,22 +467,22 @@ CCU_GATE_DEFINE(CLK_RTC, rtc_clk, rtc_clk, "clock-32k",
> APBC_RTC_CLK_RST,
>
> CCU_MUX_GATE_DEFINE(CLK_TWSI0, twsi0_clk, twsi0_clk, twsi_parents,
> ARRAY_SIZE(twsi_parents), APBC_TWSI0_CLK_RST,
> - 4, 3, BIT(1) | BIT(0), 0);
> + 4, 3, BIT(1), 0);
> CCU_MUX_GATE_DEFINE(CLK_TWSI1, twsi1_clk, twsi1_clk, twsi_parents,
> ARRAY_SIZE(twsi_parents), APBC_TWSI1_CLK_RST,
> - 4, 3, BIT(1) | BIT(0), 0);
> + 4, 3, BIT(1), 0);
> CCU_MUX_GATE_DEFINE(CLK_TWSI4, twsi4_clk, twsi4_clk, twsi_parents,
> ARRAY_SIZE(twsi_parents), APBC_TWSI4_CLK_RST,
> - 4, 3, BIT(1) | BIT(0), 0);
> + 4, 3, BIT(1), 0);
> CCU_MUX_GATE_DEFINE(CLK_TWSI5, twsi5_clk, twsi5_clk, twsi_parents,
> ARRAY_SIZE(twsi_parents), APBC_TWSI5_CLK_RST,
> - 4, 3, BIT(1) | BIT(0), 0);
> + 4, 3, BIT(1), 0);
> CCU_MUX_GATE_DEFINE(CLK_TWSI6, twsi6_clk, twsi6_clk, twsi_parents,
> ARRAY_SIZE(twsi_parents), APBC_TWSI6_CLK_RST,
> - 4, 3, BIT(1) | BIT(0), 0);
> + 4, 3, BIT(1), 0);
> CCU_MUX_GATE_DEFINE(CLK_TWSI7, twsi7_clk, twsi7_clk, twsi_parents,
> ARRAY_SIZE(twsi_parents), APBC_TWSI7_CLK_RST,
> - 4, 3, BIT(1) | BIT(0), 0);
> + 4, 3, BIT(1), 0);
>
> static const char * const timer_parents[] = {
> "pll1_d192_12p8",
> @@ -1232,8 +1251,12 @@ static struct clk *k1_ccu_mpmu_clks[] = {
> &pll1_d4_614p4.common.clk,
> &pll1_d6_409p6.common.clk,
> &pll1_d8_307p2.common.clk,
> + &pll1_d24_102p4.common.clk,
> + &pll1_d48_51p2.common.clk,
> + &pll1_d96_25p6.common.clk,
> &pll1_d78_31p5.common.clk,
> &slow_uart2_48.common.clk,
> + &apb_clk.common.clk,
> };
> #else
> static struct clk *k1_ccu_mpmu_clks[] = {
> @@ -1288,6 +1311,8 @@ static struct clk *k1_ccu_apbc_clks[] = {
> &uart0_clk.common.clk,
> &twsi2_clk.common.clk,
> &twsi8_clk.common.clk,
> + &twsi2_bus_clk.common.clk,
> + &twsi8_bus_clk.common.clk,
> };
> #else
> static struct clk *k1_ccu_apbc_clks[] = {
> diff --git a/drivers/i2c/k1_i2c.c b/drivers/i2c/k1_i2c.c
> index 2c7a1e0d3775..b11f73959a08 100644
> --- a/drivers/i2c/k1_i2c.c
> +++ b/drivers/i2c/k1_i2c.c
> @@ -51,7 +51,7 @@ struct k1_i2c {
> struct k1_i2c_priv {
> int id;
> void __iomem *base;
> - struct clk clk;
> + struct clk_bulk clks;
> };
>
> /*
> @@ -487,15 +487,13 @@ static int k1_i2c_probe(struct udevice *bus)
> return ret;
> }
>
> - ret = clk_get_by_index(bus, 0, &priv->clk);
> + ret = clk_get_bulk(bus, &priv->clks);
> if (ret)
> return ret;
>
> - ret = clk_enable(&priv->clk);
> - if (ret && ret != -ENOSYS && ret != -EOPNOTSUPP) {
> - debug("%s: failed to enable clock\n", __func__);
> + ret = clk_enable_bulk(&priv->clks);
I'd suggest to not use bulk api, to align with k1 i2c kernel driver,
also to make it easy to set func frequency if needed (a weak reason)
> + if (ret)
> return ret;
> - }
>
> priv->base = (void *)devfdt_get_addr_ptr(bus);
>
>
> --
> 2.55.0
>
--
Yixun Lan (dlan)
