[PATCH 0/2] Add SDHI clock and reset entries in cpg driver

[PATCH 0/2] Add SDHI clock and reset entries in cpg driver

[Biju Das]

Add SDHI clock and reset entries in cpg driver.

As per the HW manual, we should not directly switch from 533 MHz
to 400 MHz and vice versa. To change the setting from 533 MHz to 400 MHz
or vice versa, Switch to 266 MHz first,and then switch to the target
setting 533 MHz or 400 MHz. So added support in mux to handle this
condition.

This patch series is based on renesas-clk-for-v5.15 [1] 
[1] https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/?h=renesas-clk-for-v5.15

This patch series depend upon [2]
[2] https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=522063

Biju Das (2):
  drivers: clk: renesas: rzg2l-cpg: Add SDHI clk mux support
  drivers: clk: renesas: r9a07g044-cpg: Add SDHI clock and reset entries

 drivers/clk/renesas/r9a07g044-cpg.c |  37 ++++++++++
 drivers/clk/renesas/rzg2l-cpg.c     | 106 ++++++++++++++++++++++++++++
 drivers/clk/renesas/rzg2l-cpg.h     |  12 ++++
 3 files changed, 155 insertions(+)

-- 
2.17.1

[PATCH 1/2] drivers: clk: renesas: rzg2l-cpg: Add SDHI clk mux support

[Biju Das]

Add SDHI clk mux support to select SDHI clock from different clock
sources.

As per HW manual, direct clock switching from 533MHz to 400MHz and
vice versa is not recommended. So added support for handling this
in mux.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
This patch sereies depend upon [1]
[1] https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=522063
---
 drivers/clk/renesas/rzg2l-cpg.c | 106 ++++++++++++++++++++++++++++++++
 drivers/clk/renesas/rzg2l-cpg.h |   8 +++
 2 files changed, 114 insertions(+)

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index 4d2af113b54e..524d5384b070 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -55,6 +55,15 @@
 #define GET_REG_SAMPLL_CLK1(val)	((val >> 22) & 0xfff)
 #define GET_REG_SAMPLL_CLK2(val)	((val >> 12) & 0xfff)
 
+struct sd_hw_data {
+	struct clk_hw hw;
+	u32 conf;
+	u32 mux_flags;
+	struct rzg2l_cpg_priv *priv;
+};
+
+#define to_sd_hw_data(_hw)	container_of(_hw, struct sd_hw_data, hw)
+
 /**
  * struct rzg2l_cpg_priv - Clock Pulse Generator Private Data
  *
@@ -150,6 +159,100 @@ rzg2l_cpg_mux_clk_register(const struct cpg_core_clk *core,
 	return clk_hw->clk;
 }
 
+static int rzg2l_cpg_sd_clk_mux_determine_rate(struct clk_hw *hw,
+					       struct clk_rate_request *req)
+{
+	struct sd_hw_data *hwdata = to_sd_hw_data(hw);
+
+	return clk_mux_determine_rate_flags(hw, req, hwdata->mux_flags);
+}
+
+static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct sd_hw_data *hwdata = to_sd_hw_data(hw);
+	struct rzg2l_cpg_priv *priv = hwdata->priv;
+	u32 off = GET_REG_OFFSET(hwdata->conf);
+	u32 shift = GET_SHIFT(hwdata->conf);
+	const u32 clk_src_266 = 2;
+	u32 bitmask;
+
+	/*
+	 * As per the HW manual, we should not directly switch from 533 MHz to
+	 * 400 MHz and vice versa. To change the setting from 2’b01 (533 MHz)
+	 * to 2’b10 (400 MHz) or vice versa, Switch to 2’b11 (266 MHz) first,
+	 * and then switch to the target setting (2’b01 (533 MHz) or 2’b10
+	 * (400 MHz)).
+	 * Setting a value of '0' to the SEL_SDHI0_SET or SEL_SDHI1_SET clock
+	 * switching register is prohibited.
+	 * The clock mux has 3 input clocks(533 MHz,400 MHz, and 266 MHz), and
+	 * the index to value mapping is done by adding 1 to the index.
+	 */
+	bitmask = (GENMASK(GET_WIDTH(hwdata->conf) - 1, 0) << shift) << 16;
+	if (index != clk_src_266)
+		writel(bitmask | ((clk_src_266 + 1) << shift), priv->base + off);
+
+	writel(bitmask | ((index + 1) << shift), priv->base + off);
+
+	return 0;
+}
+
+static u8 rzg2l_cpg_sd_clk_mux_get_parent(struct clk_hw *hw)
+{
+	struct sd_hw_data *hwdata = to_sd_hw_data(hw);
+	struct rzg2l_cpg_priv *priv = hwdata->priv;
+	u32 val = readl(priv->base + GET_REG_OFFSET(hwdata->conf));
+
+	val >>= GET_SHIFT(hwdata->conf);
+	val &= GENMASK(GET_WIDTH(hwdata->conf) - 1, 0);
+	if (val)
+		val--;
+	else
+		/* Prohibited clk source, change it to 533 MHz(reset value) */
+		rzg2l_cpg_sd_clk_mux_set_parent(hw, 0);
+
+	return val;
+}
+
+static const struct clk_ops rzg2l_cpg_sd_clk_mux_ops = {
+	.determine_rate = rzg2l_cpg_sd_clk_mux_determine_rate,
+	.set_parent	= rzg2l_cpg_sd_clk_mux_set_parent,
+	.get_parent	= rzg2l_cpg_sd_clk_mux_get_parent,
+};
+
+static struct clk * __init
+rzg2l_cpg_sd_mux_clk_register(const struct cpg_core_clk *core,
+			      void __iomem *base,
+			      struct rzg2l_cpg_priv *priv)
+{
+	struct sd_hw_data *clk_hw_data;
+	struct clk_init_data init;
+	struct clk_hw *clk_hw;
+	int ret;
+
+	clk_hw_data = devm_kzalloc(priv->dev, sizeof(*clk_hw_data), GFP_KERNEL);
+	if (!clk_hw_data)
+		return ERR_PTR(-ENOMEM);
+
+	clk_hw_data->priv = priv;
+	clk_hw_data->conf = core->conf;
+	clk_hw_data->mux_flags = core->mux_flags;
+
+	init.name = GET_SHIFT(core->conf) ? "sd1" : "sd0";
+	init.ops = &rzg2l_cpg_sd_clk_mux_ops;
+	init.flags = core->flag;
+	init.num_parents = core->num_parents;
+	init.parent_names = core->parent_names;
+
+	clk_hw = &clk_hw_data->hw;
+	clk_hw->init = &init;
+
+	ret = devm_clk_hw_register(priv->dev, clk_hw);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return clk_hw->clk;
+}
+
 struct pll_clk {
 	struct clk_hw hw;
 	unsigned int conf;
@@ -311,6 +414,9 @@ rzg2l_cpg_register_core_clk(const struct cpg_core_clk *core,
 	case CLK_TYPE_MUX:
 		clk = rzg2l_cpg_mux_clk_register(core, priv->base, priv);
 		break;
+	case CLK_TYPE_SD_MUX:
+		clk = rzg2l_cpg_sd_mux_clk_register(core, priv->base, priv);
+		break;
 	default:
 		goto fail;
 	}
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index 191c403aa52f..7411e3f365c3 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -64,6 +64,9 @@ enum clk_types {
 
 	/* Clock with clock source selector */
 	CLK_TYPE_MUX,
+
+	/* Clock with SD clock source selector */
+	CLK_TYPE_SD_MUX,
 };
 
 #define DEF_TYPE(_name, _id, _type...) \
@@ -84,6 +87,11 @@ enum clk_types {
 	DEF_TYPE(_name, _id, CLK_TYPE_MUX, .conf = _conf, \
 		 .parent_names = _parent_names, .num_parents = _num_parents, \
 		 .flag = _flag, .mux_flags = _mux_flags)
+#define DEF_SD_MUX(_name, _id, _conf, _parent_names, _num_parents, _flag, \
+		   _mux_flags) \
+	DEF_TYPE(_name, _id, CLK_TYPE_SD_MUX, .conf = _conf, \
+		 .parent_names = _parent_names, .num_parents = _num_parents, \
+		 .flag = _flag, .mux_flags = _mux_flags)
 
 /**
  * struct rzg2l_mod_clk - Module Clocks definitions
-- 
2.17.1

[PATCH 2/2] drivers: clk: renesas: r9a07g044-cpg: Add SDHI clock and reset entries

[Biju Das]

Add SDHI{0,1} mux, clock and reset entries to CPG driver.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/clk/renesas/r9a07g044-cpg.c | 37 +++++++++++++++++++++++++++++
 drivers/clk/renesas/rzg2l-cpg.h     |  4 ++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index 1745e363e5a6..f893db47434a 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -41,6 +41,12 @@ enum clk_ids {
 	CLK_PLL6_2,
 	CLK_PLL6_2_DIV2,
 	CLK_P1_DIV2,
+	CLK_800FIX_C,
+	CLK_533FIX_C,
+	CLK_DIV_PLL2_DIV8,
+	CLK_DIV_PLL2_DIV12,
+	CLK_SD0_DIV4,
+	CLK_SD1_DIV4,
 
 	/* Module Clocks */
 	MOD_CLK_BASE,
@@ -58,6 +64,8 @@ static const struct clk_div_table dtable_1_32[] = {
 
 /* Mux clock tables */
 static const char * const sel_pll6_2[]	= { ".pll6_2_div2", ".pll5_2_div12" };
+static const char * const sel_shdi[] = { ".clk533fix_c", ".div_pll2_div8",
+					 ".div_pll2_div12" };
 
 static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
 	/* External Clock Inputs */
@@ -77,6 +85,11 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
 	DEF_FIXED(".pll6_2", CLK_PLL6_2, CLK_PLL6, 1, 1),
 
 	DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 1, 2),
+	DEF_FIXED(".clk800fix_c", CLK_800FIX_C, CLK_PLL2, 1, 2),
+	DEF_FIXED(".clk533fix_c", CLK_533FIX_C, CLK_PLL2, 2, 6),
+	DEF_FIXED(".div_pll2_div8", CLK_DIV_PLL2_DIV8, CLK_800FIX_C, 1, 2),
+	DEF_FIXED(".div_pll2_div12", CLK_DIV_PLL2_DIV12, CLK_533FIX_C, 1, 2),
+
 	DEF_FIXED(".pll2_div16", CLK_PLL2_DIV16, CLK_PLL2, 1, 16),
 	DEF_FIXED(".pll2_div20", CLK_PLL2_DIV20, CLK_PLL2, 1, 20),
 
@@ -103,6 +116,12 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
 	DEF_FIXED("ZT", R9A07G044_CLK_ZT, CLK_PLL3_DIV2_4_2, 1, 1),
 	DEF_MUX("HP", R9A07G044_CLK_HP, SEL_PLL6_2,
 		sel_pll6_2, ARRAY_SIZE(sel_pll6_2), 0, CLK_MUX_HIWORD_MASK),
+	DEF_SD_MUX("SD0", R9A07G044_CLK_SD0, SEL_SDHI0,
+		   sel_shdi, ARRAY_SIZE(sel_shdi), 0, 0),
+	DEF_SD_MUX("SD1", R9A07G044_CLK_SD1, SEL_SDHI1,
+		   sel_shdi, ARRAY_SIZE(sel_shdi), 0, 0),
+	DEF_FIXED("SD0_DIV4", CLK_SD0_DIV4, R9A07G044_CLK_SD0, 1, 4),
+	DEF_FIXED("SD1_DIV4", CLK_SD1_DIV4, R9A07G044_CLK_SD1, 1, 4),
 };
 
 static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
@@ -116,6 +135,22 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
 				0x52c, 0),
 	DEF_MOD("dmac_pclk",	R9A07G044_DMAC_PCLK, CLK_P1_DIV2,
 				0x52c, 1),
+	DEF_MOD("sdhi0_imclk",	R9A07G044_SDHI0_IMCLK, CLK_SD0_DIV4,
+				0x554, 0),
+	DEF_MOD("sdhi0_imclk2",	R9A07G044_SDHI0_IMCLK2, CLK_SD0_DIV4,
+				0x554, 1),
+	DEF_MOD("sdhi0_clk_hs",	R9A07G044_SDHI0_CLK_HS, R9A07G044_CLK_SD0,
+				0x554, 2),
+	DEF_MOD("sdhi0_aclk",	R9A07G044_SDHI0_ACLK, R9A07G044_CLK_P1,
+				0x554, 3),
+	DEF_MOD("sdhi1_imclk",	R9A07G044_SDHI1_IMCLK, CLK_SD1_DIV4,
+				0x554, 4),
+	DEF_MOD("sdhi1_imclk2",	R9A07G044_SDHI1_IMCLK2, CLK_SD1_DIV4,
+				0x554, 5),
+	DEF_MOD("sdhi1_clk_hs",	R9A07G044_SDHI1_CLK_HS, R9A07G044_CLK_SD1,
+				0x554, 6),
+	DEF_MOD("sdhi1_aclk",	R9A07G044_SDHI1_ACLK, R9A07G044_CLK_P1,
+				0x554, 7),
 	DEF_MOD("ssi0_pclk",	R9A07G044_SSI0_PCLK2, R9A07G044_CLK_P0,
 				0x570, 0),
 	DEF_MOD("ssi0_sfr",	R9A07G044_SSI0_PCLK_SFR, R9A07G044_CLK_P0,
@@ -184,6 +219,8 @@ static struct rzg2l_reset r9a07g044_resets[] = {
 	DEF_RST(R9A07G044_IA55_RESETN, 0x818, 0),
 	DEF_RST(R9A07G044_DMAC_ARESETN, 0x82c, 0),
 	DEF_RST(R9A07G044_DMAC_RST_ASYNC, 0x82c, 1),
+	DEF_RST(R9A07G044_SDHI0_IXRST, 0x854, 0),
+	DEF_RST(R9A07G044_SDHI1_IXRST, 0x854, 1),
 	DEF_RST(R9A07G044_SSI0_RST_M2_REG, 0x870, 0),
 	DEF_RST(R9A07G044_SSI1_RST_M2_REG, 0x870, 1),
 	DEF_RST(R9A07G044_SSI2_RST_M2_REG, 0x870, 2),
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index 7411e3f365c3..680974fc37bf 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -11,6 +11,7 @@
 
 #define CPG_PL2_DDIV		(0x204)
 #define CPG_PL3A_DDIV		(0x208)
+#define CPG_PL2SDHI_DSEL	(0x218)
 #define CPG_PL6_ETH_SSEL	(0x418)
 
 /* n = 0/1/2 for PLL1/4/6 */
@@ -30,6 +31,9 @@
 
 #define SEL_PLL6_2	SEL_PLL_PACK(CPG_PL6_ETH_SSEL, 0, 1)
 
+#define SEL_SDHI0	DDIV_PACK(CPG_PL2SDHI_DSEL, 0, 2)
+#define SEL_SDHI1	DDIV_PACK(CPG_PL2SDHI_DSEL, 4, 2)
+
 /**
  * Definitions of CPG Core Clocks
  *
-- 
2.17.1

Re: [PATCH 1/2] drivers: clk: renesas: rzg2l-cpg: Add SDHI clk mux support

[Geert Uytterhoeven]

Hi Biju,

On Wed, Aug 4, 2021 at 8:08 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Add SDHI clk mux support to select SDHI clock from different clock
> sources.
>
> As per HW manual, direct clock switching from 533MHz to 400MHz and
> vice versa is not recommended. So added support for handling this
> in mux.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -55,6 +55,15 @@
>  #define GET_REG_SAMPLL_CLK1(val)       ((val >> 22) & 0xfff)
>  #define GET_REG_SAMPLL_CLK2(val)       ((val >> 12) & 0xfff)
>
> +struct sd_hw_data {
> +       struct clk_hw hw;
> +       u32 conf;
> +       u32 mux_flags;

Do you need mux_flags? Or can this be hardcoded to zero?

> +       struct rzg2l_cpg_priv *priv;
> +};
> +
> +#define to_sd_hw_data(_hw)     container_of(_hw, struct sd_hw_data, hw)
> +
>  /**
>   * struct rzg2l_cpg_priv - Clock Pulse Generator Private Data
>   *
> @@ -150,6 +159,100 @@ rzg2l_cpg_mux_clk_register(const struct cpg_core_clk *core,
>         return clk_hw->clk;
>  }
>
> +static int rzg2l_cpg_sd_clk_mux_determine_rate(struct clk_hw *hw,
> +                                              struct clk_rate_request *req)
> +{
> +       struct sd_hw_data *hwdata = to_sd_hw_data(hw);
> +
> +       return clk_mux_determine_rate_flags(hw, req, hwdata->mux_flags);
> +}
> +
> +static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index)
> +{
> +       struct sd_hw_data *hwdata = to_sd_hw_data(hw);
> +       struct rzg2l_cpg_priv *priv = hwdata->priv;
> +       u32 off = GET_REG_OFFSET(hwdata->conf);
> +       u32 shift = GET_SHIFT(hwdata->conf);
> +       const u32 clk_src_266 = 2;
> +       u32 bitmask;
> +
> +       /*
> +        * As per the HW manual, we should not directly switch from 533 MHz to
> +        * 400 MHz and vice versa. To change the setting from 2’b01 (533 MHz)
> +        * to 2’b10 (400 MHz) or vice versa, Switch to 2’b11 (266 MHz) first,
> +        * and then switch to the target setting (2’b01 (533 MHz) or 2’b10
> +        * (400 MHz)).
> +        * Setting a value of '0' to the SEL_SDHI0_SET or SEL_SDHI1_SET clock
> +        * switching register is prohibited.
> +        * The clock mux has 3 input clocks(533 MHz,400 MHz, and 266 MHz), and
> +        * the index to value mapping is done by adding 1 to the index.
> +        */
> +       bitmask = (GENMASK(GET_WIDTH(hwdata->conf) - 1, 0) << shift) << 16;
> +       if (index != clk_src_266)
> +               writel(bitmask | ((clk_src_266 + 1) << shift), priv->base + off);

I'm wondering if you should poll (using readl_poll_timeout()) until
the CPG_CLKSTATUS.SELSDHIx_STS bit is cleared, to indicate switching
has completed?

> +
> +       writel(bitmask | ((index + 1) << shift), priv->base + off);
> +
> +       return 0;
> +}
> +
> +static u8 rzg2l_cpg_sd_clk_mux_get_parent(struct clk_hw *hw)
> +{
> +       struct sd_hw_data *hwdata = to_sd_hw_data(hw);
> +       struct rzg2l_cpg_priv *priv = hwdata->priv;
> +       u32 val = readl(priv->base + GET_REG_OFFSET(hwdata->conf));
> +
> +       val >>= GET_SHIFT(hwdata->conf);
> +       val &= GENMASK(GET_WIDTH(hwdata->conf) - 1, 0);
> +       if (val)
> +               val--;
> +       else
> +               /* Prohibited clk source, change it to 533 MHz(reset value) */
> +               rzg2l_cpg_sd_clk_mux_set_parent(hw, 0);

Please add curly braces (to both branches).

> +
> +       return val;
> +}

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] drivers: clk: renesas: r9a07g044-cpg: Add SDHI clock and reset entries

[Geert Uytterhoeven]

Hi Biju,

On Wed, Aug 4, 2021 at 8:08 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Add SDHI{0,1} mux, clock and reset entries to CPG driver.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/r9a07g044-cpg.c
> +++ b/drivers/clk/renesas/r9a07g044-cpg.c

> @@ -77,6 +85,11 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
>         DEF_FIXED(".pll6_2", CLK_PLL6_2, CLK_PLL6, 1, 1),
>
>         DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 1, 2),
> +       DEF_FIXED(".clk800fix_c", CLK_800FIX_C, CLK_PLL2, 1, 2),
> +       DEF_FIXED(".clk533fix_c", CLK_533FIX_C, CLK_PLL2, 2, 6),

"2, 6" can be simplified to "1, 3".

> +       DEF_FIXED(".div_pll2_div8", CLK_DIV_PLL2_DIV8, CLK_800FIX_C, 1, 2),
> +       DEF_FIXED(".div_pll2_div12", CLK_DIV_PLL2_DIV12, CLK_533FIX_C, 1, 2),

I just love the confusing clock naming in the User's Manual!
DIV_PLL2_DIV8 runs at PLL2 / 4, and DIV_PLL2_DIV12 runs at PLL2 / 6 :-(

> +
>         DEF_FIXED(".pll2_div16", CLK_PLL2_DIV16, CLK_PLL2, 1, 16),
>         DEF_FIXED(".pll2_div20", CLK_PLL2_DIV20, CLK_PLL2, 1, 20),
>
> @@ -103,6 +116,12 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
>         DEF_FIXED("ZT", R9A07G044_CLK_ZT, CLK_PLL3_DIV2_4_2, 1, 1),
>         DEF_MUX("HP", R9A07G044_CLK_HP, SEL_PLL6_2,
>                 sel_pll6_2, ARRAY_SIZE(sel_pll6_2), 0, CLK_MUX_HIWORD_MASK),
> +       DEF_SD_MUX("SD0", R9A07G044_CLK_SD0, SEL_SDHI0,
> +                  sel_shdi, ARRAY_SIZE(sel_shdi), 0, 0),
> +       DEF_SD_MUX("SD1", R9A07G044_CLK_SD1, SEL_SDHI1,
> +                  sel_shdi, ARRAY_SIZE(sel_shdi), 0, 0),

Looks like both .flag and .mux_flags are unneeded?

> +       DEF_FIXED("SD0_DIV4", CLK_SD0_DIV4, R9A07G044_CLK_SD0, 1, 4),
> +       DEF_FIXED("SD1_DIV4", CLK_SD1_DIV4, R9A07G044_CLK_SD1, 1, 4),
>  };
>
>  static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {

The rest looks good to me.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 2/2] drivers: clk: renesas: r9a07g044-cpg: Add SDHI clock and reset entries

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 2/2] drivers: clk: renesas: r9a07g044-cpg: Add SDHI
> clock and reset entries
> 
> Hi Biju,
> 
> On Wed, Aug 4, 2021 at 8:08 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Add SDHI{0,1} mux, clock and reset entries to CPG driver.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> 
> > @@ -77,6 +85,11 @@ static const struct cpg_core_clk
> r9a07g044_core_clks[] __initconst = {
> >         DEF_FIXED(".pll6_2", CLK_PLL6_2, CLK_PLL6, 1, 1),
> >
> >         DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 1, 2),
> > +       DEF_FIXED(".clk800fix_c", CLK_800FIX_C, CLK_PLL2, 1, 2),
> > +       DEF_FIXED(".clk533fix_c", CLK_533FIX_C, CLK_PLL2, 2, 6),
> 
> "2, 6" can be simplified to "1, 3".

OK, Will change it to 1,3.

> 
> > +       DEF_FIXED(".div_pll2_div8", CLK_DIV_PLL2_DIV8, CLK_800FIX_C, 1,
> 2),
> > +       DEF_FIXED(".div_pll2_div12", CLK_DIV_PLL2_DIV12, CLK_533FIX_C,
> > + 1, 2),
> 
> I just love the confusing clock naming in the User's Manual!
> DIV_PLL2_DIV8 runs at PLL2 / 4, and DIV_PLL2_DIV12 runs at PLL2 / 6 :-(
> 

There is an update on latest HW manual(Rev1.00, sep,2021)

As per this, it is just 400 MHZ and 266 MHz. I will send new patch based on this.


> > +
> >         DEF_FIXED(".pll2_div16", CLK_PLL2_DIV16, CLK_PLL2, 1, 16),
> >         DEF_FIXED(".pll2_div20", CLK_PLL2_DIV20, CLK_PLL2, 1, 20),
> >
> > @@ -103,6 +116,12 @@ static const struct cpg_core_clk
> r9a07g044_core_clks[] __initconst = {
> >         DEF_FIXED("ZT", R9A07G044_CLK_ZT, CLK_PLL3_DIV2_4_2, 1, 1),
> >         DEF_MUX("HP", R9A07G044_CLK_HP, SEL_PLL6_2,
> >                 sel_pll6_2, ARRAY_SIZE(sel_pll6_2), 0,
> > CLK_MUX_HIWORD_MASK),
> > +       DEF_SD_MUX("SD0", R9A07G044_CLK_SD0, SEL_SDHI0,
> > +                  sel_shdi, ARRAY_SIZE(sel_shdi), 0, 0),
> > +       DEF_SD_MUX("SD1", R9A07G044_CLK_SD1, SEL_SDHI1,
> > +                  sel_shdi, ARRAY_SIZE(sel_shdi), 0, 0),
> 
> Looks like both .flag and .mux_flags are unneeded?
OK. Will remove it.

Regards,
Biju

> 
> > +       DEF_FIXED("SD0_DIV4", CLK_SD0_DIV4, R9A07G044_CLK_SD0, 1, 4),
> > +       DEF_FIXED("SD1_DIV4", CLK_SD1_DIV4, R9A07G044_CLK_SD1, 1, 4),
> >  };
> >
> >  static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
> 
> The rest looks good to me.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker.
> But when I'm talking to journalists I just say "programmer" or something
> like that.
>                                 -- Linus Torvalds

RE: [PATCH 1/2] drivers: clk: renesas: rzg2l-cpg: Add SDHI clk mux support

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 1/2] drivers: clk: renesas: rzg2l-cpg: Add SDHI clk
> mux support
> 
> Hi Biju,
> 
> On Wed, Aug 4, 2021 at 8:08 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Add SDHI clk mux support to select SDHI clock from different clock
> > sources.
> >
> > As per HW manual, direct clock switching from 533MHz to 400MHz and
> > vice versa is not recommended. So added support for handling this in
> > mux.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -55,6 +55,15 @@
> >  #define GET_REG_SAMPLL_CLK1(val)       ((val >> 22) & 0xfff)
> >  #define GET_REG_SAMPLL_CLK2(val)       ((val >> 12) & 0xfff)
> >
> > +struct sd_hw_data {
> > +       struct clk_hw hw;
> > +       u32 conf;
> > +       u32 mux_flags;
> 
> Do you need mux_flags? Or can this be hardcoded to zero?

OK will hardcoded to zero.

> 
> > +       struct rzg2l_cpg_priv *priv;
> > +};
> > +
> > +#define to_sd_hw_data(_hw)     container_of(_hw, struct sd_hw_data, hw)
> > +
> >  /**
> >   * struct rzg2l_cpg_priv - Clock Pulse Generator Private Data
> >   *
> > @@ -150,6 +159,100 @@ rzg2l_cpg_mux_clk_register(const struct
> cpg_core_clk *core,
> >         return clk_hw->clk;
> >  }
> >
> > +static int rzg2l_cpg_sd_clk_mux_determine_rate(struct clk_hw *hw,
> > +                                              struct clk_rate_request
> > +*req) {
> > +       struct sd_hw_data *hwdata = to_sd_hw_data(hw);
> > +
> > +       return clk_mux_determine_rate_flags(hw, req,
> > +hwdata->mux_flags); }
> > +
> > +static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8
> > +index) {
> > +       struct sd_hw_data *hwdata = to_sd_hw_data(hw);
> > +       struct rzg2l_cpg_priv *priv = hwdata->priv;
> > +       u32 off = GET_REG_OFFSET(hwdata->conf);
> > +       u32 shift = GET_SHIFT(hwdata->conf);
> > +       const u32 clk_src_266 = 2;
> > +       u32 bitmask;
> > +
> > +       /*
> > +        * As per the HW manual, we should not directly switch from 533
> MHz to
> > +        * 400 MHz and vice versa. To change the setting from 2’b01 (533
> MHz)
> > +        * to 2’b10 (400 MHz) or vice versa, Switch to 2’b11 (266 MHz)
> first,
> > +        * and then switch to the target setting (2’b01 (533 MHz) or
> 2’b10
> > +        * (400 MHz)).
> > +        * Setting a value of '0' to the SEL_SDHI0_SET or SEL_SDHI1_SET
> clock
> > +        * switching register is prohibited.
> > +        * The clock mux has 3 input clocks(533 MHz,400 MHz, and 266
> MHz), and
> > +        * the index to value mapping is done by adding 1 to the index.
> > +        */
> > +       bitmask = (GENMASK(GET_WIDTH(hwdata->conf) - 1, 0) << shift) <<
> 16;
> > +       if (index != clk_src_266)
> > +               writel(bitmask | ((clk_src_266 + 1) << shift),
> > + priv->base + off);
> 
> I'm wondering if you should poll (using readl_poll_timeout()) until the
> CPG_CLKSTATUS.SELSDHIx_STS bit is cleared, to indicate switching has
> completed?

OK will check this.

> 
> > +
> > +       writel(bitmask | ((index + 1) << shift), priv->base + off);
> > +
> > +       return 0;
> > +}
> > +
> > +static u8 rzg2l_cpg_sd_clk_mux_get_parent(struct clk_hw *hw) {
> > +       struct sd_hw_data *hwdata = to_sd_hw_data(hw);
> > +       struct rzg2l_cpg_priv *priv = hwdata->priv;
> > +       u32 val = readl(priv->base + GET_REG_OFFSET(hwdata->conf));
> > +
> > +       val >>= GET_SHIFT(hwdata->conf);
> > +       val &= GENMASK(GET_WIDTH(hwdata->conf) - 1, 0);
> > +       if (val)
> > +               val--;
> > +       else
> > +               /* Prohibited clk source, change it to 533 MHz(reset
> value) */
> > +               rzg2l_cpg_sd_clk_mux_set_parent(hw, 0);
> 
> Please add curly braces (to both branches).

OK.

Regards,
Biju

> 
> > +
> > +       return val;
> > +}
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker.
> But when I'm talking to journalists I just say "programmer" or something
> like that.
>                                 -- Linus Torvalds