Re: [PATCH 2/2] i2c: k1: enable all clocks for I2C controllers

Yixun Lan <[email protected]>
Newsgroups org.u-boot-project.lists.u-boot
Message-ID <[email protected]> 
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)
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.