* [PATCH v2 1/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks
2021-07-27 14:17 [PATCH v2 0/4] Add GbEthernet Clock support Biju Das
@ 2021-07-27 14:17 ` Biju Das
2021-07-27 14:17 ` [PATCH v2 2/4] drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources Biju Das
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Biju Das @ 2021-07-27 14:17 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: Biju Das, Geert Uytterhoeven, Sergei Shtylyov, linux-renesas-soc,
linux-clk, Chris Paterson, Biju Das, Prabhakar Mahadev Lad
Add support to handle mux clocks in order to select a clock source
from multiple sources.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v1->v2:
* Moved SEL_PLL_PACK macro to here
* Fixed the commit message and extra blank line as pointed by Sergei
* Added Geert's Rb tag
v1:
* New patch.
---
drivers/clk/renesas/rzg2l-cpg.c | 23 +++++++++++++++++++++++
drivers/clk/renesas/rzg2l-cpg.h | 12 ++++++++++++
2 files changed, 35 insertions(+)
diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index 3b3b2c3347f3..597efc2504eb 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -130,6 +130,26 @@ rzg2l_cpg_div_clk_register(const struct cpg_core_clk *core,
return clk_hw->clk;
}
+static struct clk * __init
+rzg2l_cpg_mux_clk_register(const struct cpg_core_clk *core,
+ void __iomem *base,
+ struct rzg2l_cpg_priv *priv)
+{
+ const struct clk_hw *clk_hw;
+
+ clk_hw = devm_clk_hw_register_mux(priv->dev, core->name,
+ core->parent_names, core->num_parents,
+ core->flag,
+ base + GET_REG_OFFSET(core->conf),
+ GET_SHIFT(core->conf),
+ GET_WIDTH(core->conf),
+ core->mux_flags, &priv->rmw_lock);
+ if (IS_ERR(clk_hw))
+ return ERR_CAST(clk_hw);
+
+ return clk_hw->clk;
+}
+
struct pll_clk {
struct clk_hw hw;
unsigned int conf;
@@ -288,6 +308,9 @@ rzg2l_cpg_register_core_clk(const struct cpg_core_clk *core,
clk = rzg2l_cpg_div_clk_register(core, priv->clks,
priv->base, priv);
break;
+ case CLK_TYPE_MUX:
+ clk = rzg2l_cpg_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 63695280ce8b..f538ffa3371c 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -24,6 +24,9 @@
#define DIVPL3A DDIV_PACK(CPG_PL3A_DDIV, 0, 3)
#define DIVPL3B DDIV_PACK(CPG_PL3A_DDIV, 4, 3)
+#define SEL_PLL_PACK(offset, bitpos, size) \
+ (((offset) << 20) | ((bitpos) << 12) | ((size) << 8))
+
/**
* Definitions of CPG Core Clocks
*
@@ -43,6 +46,7 @@ struct cpg_core_clk {
const struct clk_div_table *dtable;
const char * const *parent_names;
int flag;
+ int mux_flags;
int num_parents;
};
@@ -54,6 +58,9 @@ enum clk_types {
/* Clock with divider */
CLK_TYPE_DIV,
+
+ /* Clock with clock source selector */
+ CLK_TYPE_MUX,
};
#define DEF_TYPE(_name, _id, _type...) \
@@ -69,6 +76,11 @@ enum clk_types {
#define DEF_DIV(_name, _id, _parent, _conf, _dtable, _flag) \
DEF_TYPE(_name, _id, CLK_TYPE_DIV, .conf = _conf, \
.parent = _parent, .dtable = _dtable, .flag = _flag)
+#define DEF_MUX(_name, _id, _conf, _parent_names, _num_parents, _flag, \
+ _mux_flags) \
+ DEF_TYPE(_name, _id, CLK_TYPE_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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/4] drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources
2021-07-27 14:17 [PATCH v2 0/4] Add GbEthernet Clock support Biju Das
2021-07-27 14:17 ` [PATCH v2 1/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks Biju Das
@ 2021-07-27 14:17 ` Biju Das
2021-07-27 14:17 ` [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks Biju Das
2021-07-27 14:17 ` [PATCH v2 4/4] drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset Biju Das
3 siblings, 0 replies; 11+ messages in thread
From: Biju Das @ 2021-07-27 14:17 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: Biju Das, Geert Uytterhoeven, linux-renesas-soc, linux-clk,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad
Ethernet reference clock can be sourced from PLL5_2 or PLL6_2. Add support
for ethernet source clock selection using SEL_PLL_6_2 mux.
This patch also renames the PLL5_DIV2 core clock to PLL5_2_DIV12 to match
with the register description as mentioned in RZ/G2L HW manual (Rev.0.50).
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2:
* Moved SEL_PLL_PACK macro to Mux handling support
* Renamed PLL5_DIV2 core clock to PLL5_2_DIV12
v1:
* New patch.
---
drivers/clk/renesas/r9a07g044-cpg.c | 21 ++++++++++++++++++++-
drivers/clk/renesas/rzg2l-cpg.h | 3 +++
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index 4c94b94c4125..acf19a6cde31 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -35,8 +35,11 @@ enum clk_ids {
CLK_PLL3_DIV4,
CLK_PLL4,
CLK_PLL5,
- CLK_PLL5_DIV2,
+ CLK_PLL5_2,
+ CLK_PLL5_2_DIV12,
CLK_PLL6,
+ CLK_PLL6_2,
+ CLK_PLL6_2_DIV2,
CLK_P1_DIV2,
/* Module Clocks */
@@ -53,6 +56,9 @@ static const struct clk_div_table dtable_1_32[] = {
{0, 0},
};
+/* Mux clock tables */
+static const char * const sel_pll6_2[] = { ".pll6_2_div2", ".pll5_2_div12" };
+
static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
/* External Clock Inputs */
DEF_INPUT("extal", CLK_EXTAL),
@@ -64,6 +70,12 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
DEF_FIXED(".pll2", CLK_PLL2, CLK_EXTAL, 133, 2),
DEF_FIXED(".pll3", CLK_PLL3, CLK_EXTAL, 133, 2),
+ DEF_FIXED(".pll5", CLK_PLL5, CLK_EXTAL, 125, 1),
+ DEF_FIXED(".pll5_2", CLK_PLL5_2, CLK_PLL5, 1, 6),
+
+ DEF_FIXED(".pll6", CLK_PLL6, CLK_EXTAL, 125, 6),
+ DEF_FIXED(".pll6_2", CLK_PLL6_2, CLK_PLL6, 1, 1),
+
DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 1, 2),
DEF_FIXED(".pll2_div16", CLK_PLL2_DIV16, CLK_PLL2, 1, 16),
DEF_FIXED(".pll2_div20", CLK_PLL2_DIV20, CLK_PLL2, 1, 20),
@@ -73,6 +85,9 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
DEF_FIXED(".pll3_div2_4_2", CLK_PLL3_DIV2_4_2, CLK_PLL3_DIV2_4, 1, 2),
DEF_FIXED(".pll3_div4", CLK_PLL3_DIV4, CLK_PLL3, 1, 4),
+ DEF_FIXED(".pll5_2_div12", CLK_PLL5_2_DIV12, CLK_PLL5_2, 1, 2),
+ DEF_FIXED(".pll6_2_div2", CLK_PLL6_2_DIV2, CLK_PLL6_2, 1, 2),
+
/* Core output clk */
DEF_FIXED("I", R9A07G044_CLK_I, CLK_PLL1, 1, 1),
DEF_DIV("P0", R9A07G044_CLK_P0, CLK_PLL2_DIV16, DIVPL2A,
@@ -84,6 +99,10 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
DEF_FIXED("P1_DIV2", CLK_P1_DIV2, R9A07G044_CLK_P1, 1, 2),
DEF_DIV("P2", R9A07G044_CLK_P2, CLK_PLL3_DIV2_4_2,
DIVPL3A, dtable_1_32, CLK_DIVIDER_HIWORD_MASK),
+ DEF_FIXED("M0", R9A07G044_CLK_M0, CLK_PLL3_DIV2_4, 1, 1),
+ 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),
};
static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index f538ffa3371c..5202c0512483 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_PL6_ETH_SSEL (0x418)
/* n = 0/1/2 for PLL1/4/6 */
#define CPG_SAMPLL_CLK1(n) (0x04 + (16 * n))
@@ -27,6 +28,8 @@
#define SEL_PLL_PACK(offset, bitpos, size) \
(((offset) << 20) | ((bitpos) << 12) | ((size) << 8))
+#define SEL_PLL6_2 SEL_PLL_PACK(CPG_PL6_ETH_SSEL, 0, 1)
+
/**
* Definitions of CPG Core Clocks
*
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks
2021-07-27 14:17 [PATCH v2 0/4] Add GbEthernet Clock support Biju Das
2021-07-27 14:17 ` [PATCH v2 1/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks Biju Das
2021-07-27 14:17 ` [PATCH v2 2/4] drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources Biju Das
@ 2021-07-27 14:17 ` Biju Das
2021-08-11 10:18 ` Geert Uytterhoeven
2021-07-27 14:17 ` [PATCH v2 4/4] drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset Biju Das
3 siblings, 1 reply; 11+ messages in thread
From: Biju Das @ 2021-07-27 14:17 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: Biju Das, Geert Uytterhoeven, linux-renesas-soc, linux-clk,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad
The AXI and CHI clocks use the same register bit for controlling clock
output. Add a new clock type for coupled clocks, which sets the
CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled, and
clears the bit only when both clocks are disabled.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2:-
* New patch
---
drivers/clk/renesas/rzg2l-cpg.c | 31 +++++++++++++++++++++++++++++++
drivers/clk/renesas/rzg2l-cpg.h | 11 ++++++++++-
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index 597efc2504eb..4d2af113b54e 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -333,12 +333,16 @@ rzg2l_cpg_register_core_clk(const struct cpg_core_clk *core,
* @hw: handle between common and hardware-specific interfaces
* @off: register offset
* @bit: ON/MON bit
+ * @is_coupled: flag to indicate coupled clock
+ * @on_cnt: ON count for coupled clocks
* @priv: CPG/MSTP private data
*/
struct mstp_clock {
struct clk_hw hw;
u16 off;
u8 bit;
+ bool is_coupled;
+ u8 on_cnt;
struct rzg2l_cpg_priv *priv;
};
@@ -392,11 +396,37 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
static int rzg2l_mod_clock_enable(struct clk_hw *hw)
{
+ struct mstp_clock *clock = to_mod_clock(hw);
+ struct rzg2l_cpg_priv *priv = clock->priv;
+ unsigned long flags;
+
+ spin_lock_irqsave(&priv->rmw_lock, flags);
+ clock->on_cnt++;
+ if (clock->is_coupled && clock->on_cnt > 1) {
+ spin_unlock_irqrestore(&priv->rmw_lock, flags);
+ return 1;
+ }
+
+ spin_unlock_irqrestore(&priv->rmw_lock, flags);
+
return rzg2l_mod_clock_endisable(hw, true);
}
static void rzg2l_mod_clock_disable(struct clk_hw *hw)
{
+ struct mstp_clock *clock = to_mod_clock(hw);
+ struct rzg2l_cpg_priv *priv = clock->priv;
+ unsigned long flags;
+
+ spin_lock_irqsave(&priv->rmw_lock, flags);
+ clock->on_cnt--;
+ if (clock->is_coupled && clock->on_cnt) {
+ spin_unlock_irqrestore(&priv->rmw_lock, flags);
+ return;
+ }
+
+ spin_unlock_irqrestore(&priv->rmw_lock, flags);
+
rzg2l_mod_clock_endisable(hw, false);
}
@@ -475,6 +505,7 @@ rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
clock->off = mod->off;
clock->bit = mod->bit;
+ clock->is_coupled = mod->is_coupled;
clock->priv = priv;
clock->hw.init = &init;
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index 5202c0512483..191c403aa52f 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -93,6 +93,7 @@ enum clk_types {
* @parent: id of parent clock
* @off: register offset
* @bit: ON/MON bit
+ * @is_coupled: flag to indicate coupled clock
*/
struct rzg2l_mod_clk {
const char *name;
@@ -100,17 +101,25 @@ struct rzg2l_mod_clk {
unsigned int parent;
u16 off;
u8 bit;
+ bool is_coupled;
};
-#define DEF_MOD(_name, _id, _parent, _off, _bit) \
+#define DEF_MOD_BASE(_name, _id, _parent, _off, _bit, _is_coupled) \
{ \
.name = _name, \
.id = MOD_CLK_BASE + (_id), \
.parent = (_parent), \
.off = (_off), \
.bit = (_bit), \
+ .is_coupled = (_is_coupled), \
}
+#define DEF_MOD(_name, _id, _parent, _off, _bit) \
+ DEF_MOD_BASE(_name, _id, _parent, _off, _bit, false)
+
+#define DEF_COUPLED(_name, _id, _parent, _off, _bit) \
+ DEF_MOD_BASE(_name, _id, _parent, _off, _bit, true)
+
/**
* struct rzg2l_reset - Reset definitions
*
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks
2021-07-27 14:17 ` [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks Biju Das
@ 2021-08-11 10:18 ` Geert Uytterhoeven
2021-08-12 6:59 ` Biju Das
0 siblings, 1 reply; 11+ messages in thread
From: Geert Uytterhoeven @ 2021-08-11 10:18 UTC (permalink / raw)
To: Biju Das
Cc: Michael Turquette, Stephen Boyd, Linux-Renesas, linux-clk,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad
Hi Biju,
On Tue, Jul 27, 2021 at 4:18 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> The AXI and CHI clocks use the same register bit for controlling clock
> output. Add a new clock type for coupled clocks, which sets the
> CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled, and
> clears the bit only when both clocks are disabled.
>
> 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
> @@ -333,12 +333,16 @@ rzg2l_cpg_register_core_clk(const struct cpg_core_clk *core,
> * @hw: handle between common and hardware-specific interfaces
> * @off: register offset
> * @bit: ON/MON bit
> + * @is_coupled: flag to indicate coupled clock
> + * @on_cnt: ON count for coupled clocks
> * @priv: CPG/MSTP private data
> */
> struct mstp_clock {
> struct clk_hw hw;
> u16 off;
> u8 bit;
> + bool is_coupled;
> + u8 on_cnt;
While u8 is probably sufficient, you may want to use unsigned int,
as there will be a gap anyway due to alignment rules.
> struct rzg2l_cpg_priv *priv;
> };
>
> @@ -392,11 +396,37 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
>
> static int rzg2l_mod_clock_enable(struct clk_hw *hw)
> {
> + struct mstp_clock *clock = to_mod_clock(hw);
> + struct rzg2l_cpg_priv *priv = clock->priv;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->rmw_lock, flags);
> + clock->on_cnt++;
> + if (clock->is_coupled && clock->on_cnt > 1) {
> + spin_unlock_irqrestore(&priv->rmw_lock, flags);
> + return 1;
> + }
> +
> + spin_unlock_irqrestore(&priv->rmw_lock, flags);
I think you can avoid taking the spinlock and touching the counter
if the is_coupled flag is not set.
> +
> return rzg2l_mod_clock_endisable(hw, true);
> }
However, I'm wondering how this can work?
DEF_COUPLED("eth0_axi", R9A07G044_ETH0_CLK_AXI, R9A07G044_CLK_M0,
0x57c, 0),
DEF_COUPLED("eth0_chi", R9A07G044_ETH0_CLK_CHI, R9A07G044_CLK_ZT,
0x57c, 0),
This will create 2 independent clocks, each with their own mstp_clock
structure that has the is_coupled flag set. Hence each clock has
its own counter. Shouldn't the counter be shared?
Am I missing something?
And what about rzg2l_mod_clock_is_enabled()?
Shouldn't it reflect the soft state instead of the shared hardware
state?
> static void rzg2l_mod_clock_disable(struct clk_hw *hw)
> {
> + struct mstp_clock *clock = to_mod_clock(hw);
> + struct rzg2l_cpg_priv *priv = clock->priv;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->rmw_lock, flags);
> + clock->on_cnt--;
> + if (clock->is_coupled && clock->on_cnt) {
> + spin_unlock_irqrestore(&priv->rmw_lock, flags);
> + return;
> + }
> +
> + spin_unlock_irqrestore(&priv->rmw_lock, flags);
> +
> rzg2l_mod_clock_endisable(hw, false);
> }
>
> @@ -475,6 +505,7 @@ rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
>
> clock->off = mod->off;
> clock->bit = mod->bit;
> + clock->is_coupled = mod->is_coupled;
> clock->priv = priv;
> clock->hw.init = &init;
>
> diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
> index 5202c0512483..191c403aa52f 100644
> --- a/drivers/clk/renesas/rzg2l-cpg.h
> +++ b/drivers/clk/renesas/rzg2l-cpg.h
> @@ -93,6 +93,7 @@ enum clk_types {
> * @parent: id of parent clock
> * @off: register offset
> * @bit: ON/MON bit
> + * @is_coupled: flag to indicate coupled clock
> */
> struct rzg2l_mod_clk {
> const char *name;
> @@ -100,17 +101,25 @@ struct rzg2l_mod_clk {
> unsigned int parent;
> u16 off;
> u8 bit;
> + bool is_coupled;
> };
>
> -#define DEF_MOD(_name, _id, _parent, _off, _bit) \
> +#define DEF_MOD_BASE(_name, _id, _parent, _off, _bit, _is_coupled) \
> { \
> .name = _name, \
> .id = MOD_CLK_BASE + (_id), \
> .parent = (_parent), \
> .off = (_off), \
> .bit = (_bit), \
> + .is_coupled = (_is_coupled), \
> }
>
> +#define DEF_MOD(_name, _id, _parent, _off, _bit) \
> + DEF_MOD_BASE(_name, _id, _parent, _off, _bit, false)
> +
> +#define DEF_COUPLED(_name, _id, _parent, _off, _bit) \
> + DEF_MOD_BASE(_name, _id, _parent, _off, _bit, true)
> +
> /**
> * struct rzg2l_reset - Reset definitions
> *
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks
2021-08-11 10:18 ` Geert Uytterhoeven
@ 2021-08-12 6:59 ` Biju Das
2021-08-12 7:54 ` Geert Uytterhoeven
0 siblings, 1 reply; 11+ messages in thread
From: Biju Das @ 2021-08-12 6:59 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Michael Turquette, Stephen Boyd, Linux-Renesas, linux-clk,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad
Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support
> to handle coupled clocks
>
> Hi Biju,
>
> On Tue, Jul 27, 2021 at 4:18 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > The AXI and CHI clocks use the same register bit for controlling clock
> > output. Add a new clock type for coupled clocks, which sets the
> > CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled, and
> > clears the bit only when both clocks are disabled.
> >
> > 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
> > @@ -333,12 +333,16 @@ rzg2l_cpg_register_core_clk(const struct
> cpg_core_clk *core,
> > * @hw: handle between common and hardware-specific interfaces
> > * @off: register offset
> > * @bit: ON/MON bit
> > + * @is_coupled: flag to indicate coupled clock
> > + * @on_cnt: ON count for coupled clocks
> > * @priv: CPG/MSTP private data
> > */
> > struct mstp_clock {
> > struct clk_hw hw;
> > u16 off;
> > u8 bit;
> > + bool is_coupled;
> > + u8 on_cnt;
>
> While u8 is probably sufficient, you may want to use unsigned int, as
> there will be a gap anyway due to alignment rules.
>
> > struct rzg2l_cpg_priv *priv;
> > };
> >
> > @@ -392,11 +396,37 @@ static int rzg2l_mod_clock_endisable(struct
> > clk_hw *hw, bool enable)
> >
> > static int rzg2l_mod_clock_enable(struct clk_hw *hw) {
> > + struct mstp_clock *clock = to_mod_clock(hw);
> > + struct rzg2l_cpg_priv *priv = clock->priv;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&priv->rmw_lock, flags);
> > + clock->on_cnt++;
> > + if (clock->is_coupled && clock->on_cnt > 1) {
> > + spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > + return 1;
> > + }
> > +
> > + spin_unlock_irqrestore(&priv->rmw_lock, flags);
>
> I think you can avoid taking the spinlock and touching the counter if the
> is_coupled flag is not set.
OK.
>
> > +
> > return rzg2l_mod_clock_endisable(hw, true); }
>
> However, I'm wondering how this can work?
>
> DEF_COUPLED("eth0_axi", R9A07G044_ETH0_CLK_AXI, R9A07G044_CLK_M0,
> 0x57c, 0),
> DEF_COUPLED("eth0_chi", R9A07G044_ETH0_CLK_CHI, R9A07G044_CLK_ZT,
> 0x57c, 0),
>
> This will create 2 independent clocks, each with their own mstp_clock
> structure that has the is_coupled flag set. Hence each clock has its own
> counter. Shouldn't the counter be shared?
> Am I missing something?
Oops. You are correct. I need to add this counter to priv instead of mstp_clocks.
>
> And what about rzg2l_mod_clock_is_enabled()?
> Shouldn't it reflect the soft state instead of the shared hardware state?
OK, will return Soft state for coupled clocks.
Cheers,
Biju
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks
2021-08-12 6:59 ` Biju Das
@ 2021-08-12 7:54 ` Geert Uytterhoeven
2021-08-13 12:16 ` Biju Das
0 siblings, 1 reply; 11+ messages in thread
From: Geert Uytterhoeven @ 2021-08-12 7:54 UTC (permalink / raw)
To: Biju Das
Cc: Michael Turquette, Stephen Boyd, Linux-Renesas, linux-clk,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad
Hi Biju,
On Thu, Aug 12, 2021 at 9:00 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support
> > to handle coupled clocks
> > On Tue, Jul 27, 2021 at 4:18 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > The AXI and CHI clocks use the same register bit for controlling clock
> > > output. Add a new clock type for coupled clocks, which sets the
> > > CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled, and
> > > clears the bit only when both clocks are disabled.
> > >
> > > 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
> > > @@ -333,12 +333,16 @@ rzg2l_cpg_register_core_clk(const struct
> > cpg_core_clk *core,
> > > * @hw: handle between common and hardware-specific interfaces
> > > * @off: register offset
> > > * @bit: ON/MON bit
> > > + * @is_coupled: flag to indicate coupled clock
> > > + * @on_cnt: ON count for coupled clocks
> > > * @priv: CPG/MSTP private data
> > > */
> > > struct mstp_clock {
> > > struct clk_hw hw;
> > > u16 off;
> > > u8 bit;
> > > + bool is_coupled;
> > > + u8 on_cnt;
> >
> > While u8 is probably sufficient, you may want to use unsigned int, as
> > there will be a gap anyway due to alignment rules.
> >
> > > struct rzg2l_cpg_priv *priv;
> > > };
> > >
> > > @@ -392,11 +396,37 @@ static int rzg2l_mod_clock_endisable(struct
> > > clk_hw *hw, bool enable)
> > >
> > > static int rzg2l_mod_clock_enable(struct clk_hw *hw) {
> > > + struct mstp_clock *clock = to_mod_clock(hw);
> > > + struct rzg2l_cpg_priv *priv = clock->priv;
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&priv->rmw_lock, flags);
> > > + clock->on_cnt++;
> > > + if (clock->is_coupled && clock->on_cnt > 1) {
> > > + spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > > + return 1;
> > > + }
> > > +
> > > + spin_unlock_irqrestore(&priv->rmw_lock, flags);
> >
> > I think you can avoid taking the spinlock and touching the counter if the
> > is_coupled flag is not set.
>
> OK.
>
> >
> > > +
> > > return rzg2l_mod_clock_endisable(hw, true); }
> >
> > However, I'm wondering how this can work?
> >
> > DEF_COUPLED("eth0_axi", R9A07G044_ETH0_CLK_AXI, R9A07G044_CLK_M0,
> > 0x57c, 0),
> > DEF_COUPLED("eth0_chi", R9A07G044_ETH0_CLK_CHI, R9A07G044_CLK_ZT,
> > 0x57c, 0),
> >
> > This will create 2 independent clocks, each with their own mstp_clock
> > structure that has the is_coupled flag set. Hence each clock has its own
> > counter. Shouldn't the counter be shared?
> > Am I missing something?
>
> Oops. You are correct. I need to add this counter to priv instead of mstp_clocks.
On second thought, a counter is overkill. A simple flag should be
sufficient, as the clk core only calls .{en,dis}able() when the clock is
{dis,en}enabled.
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks
2021-08-12 7:54 ` Geert Uytterhoeven
@ 2021-08-13 12:16 ` Biju Das
2021-08-13 12:46 ` Geert Uytterhoeven
0 siblings, 1 reply; 11+ messages in thread
From: Biju Das @ 2021-08-13 12:16 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Michael Turquette, Stephen Boyd, Linux-Renesas, linux-clk,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad
Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support
> to handle coupled clocks
>
> Hi Biju,
>
> On Thu, Aug 12, 2021 at 9:00 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add
> > > support to handle coupled clocks On Tue, Jul 27, 2021 at 4:18 PM
> > > Biju Das <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > The AXI and CHI clocks use the same register bit for controlling
> > > > clock output. Add a new clock type for coupled clocks, which sets
> > > > the CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is
> > > > enabled, and clears the bit only when both clocks are disabled.
> > > >
> > > > 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
> > > > @@ -333,12 +333,16 @@ rzg2l_cpg_register_core_clk(const struct
> > > cpg_core_clk *core,
> > > > * @hw: handle between common and hardware-specific interfaces
> > > > * @off: register offset
> > > > * @bit: ON/MON bit
> > > > + * @is_coupled: flag to indicate coupled clock
> > > > + * @on_cnt: ON count for coupled clocks
> > > > * @priv: CPG/MSTP private data
> > > > */
> > > > struct mstp_clock {
> > > > struct clk_hw hw;
> > > > u16 off;
> > > > u8 bit;
> > > > + bool is_coupled;
> > > > + u8 on_cnt;
> > >
> > > While u8 is probably sufficient, you may want to use unsigned int,
> > > as there will be a gap anyway due to alignment rules.
> > >
> > > > struct rzg2l_cpg_priv *priv; };
> > > >
> > > > @@ -392,11 +396,37 @@ static int rzg2l_mod_clock_endisable(struct
> > > > clk_hw *hw, bool enable)
> > > >
> > > > static int rzg2l_mod_clock_enable(struct clk_hw *hw) {
> > > > + struct mstp_clock *clock = to_mod_clock(hw);
> > > > + struct rzg2l_cpg_priv *priv = clock->priv;
> > > > + unsigned long flags;
> > > > +
> > > > + spin_lock_irqsave(&priv->rmw_lock, flags);
> > > > + clock->on_cnt++;
> > > > + if (clock->is_coupled && clock->on_cnt > 1) {
> > > > + spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > > > + return 1;
> > > > + }
> > > > +
> > > > + spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > >
> > > I think you can avoid taking the spinlock and touching the counter
> > > if the is_coupled flag is not set.
> >
> > OK.
> >
> > >
> > > > +
> > > > return rzg2l_mod_clock_endisable(hw, true); }
> > >
> > > However, I'm wondering how this can work?
> > >
> > > DEF_COUPLED("eth0_axi", R9A07G044_ETH0_CLK_AXI,
> R9A07G044_CLK_M0,
> > > 0x57c, 0),
> > > DEF_COUPLED("eth0_chi", R9A07G044_ETH0_CLK_CHI,
> R9A07G044_CLK_ZT,
> > > 0x57c, 0),
> > >
> > > This will create 2 independent clocks, each with their own
> > > mstp_clock structure that has the is_coupled flag set. Hence each
> > > clock has its own counter. Shouldn't the counter be shared?
> > > Am I missing something?
> >
> > Oops. You are correct. I need to add this counter to priv instead of
> mstp_clocks.
>
> On second thought, a counter is overkill. A simple flag should be
> sufficient, as the clk core only calls .{en,dis}able() when the clock is
> {dis,en}enabled.
Just to clarify, simple flag, did you mean to use bit flag? (ie, 2 bits , since we have 2 module clocks)
when core clock calls enable, set a bit and reset the bit during disable.
Then based on the 2bits, either turn on/off clock or just return the status.
Please correct me, if my understanding wrong?
Cheers,
Biju
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks
2021-08-13 12:16 ` Biju Das
@ 2021-08-13 12:46 ` Geert Uytterhoeven
2021-08-13 18:01 ` Biju Das
0 siblings, 1 reply; 11+ messages in thread
From: Geert Uytterhoeven @ 2021-08-13 12:46 UTC (permalink / raw)
To: Biju Das
Cc: Michael Turquette, Stephen Boyd, Linux-Renesas, linux-clk,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad
Hi Biju,
On Fri, Aug 13, 2021 at 2:17 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support
> > to handle coupled clocks
> > On Thu, Aug 12, 2021 at 9:00 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > > Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add
> > > > support to handle coupled clocks On Tue, Jul 27, 2021 at 4:18 PM
> > > > Biju Das <biju.das.jz@bp.renesas.com>
> > > > wrote:
> > > > > The AXI and CHI clocks use the same register bit for controlling
> > > > > clock output. Add a new clock type for coupled clocks, which sets
> > > > > the CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is
> > > > > enabled, and clears the bit only when both clocks are disabled.
> > > > >
> > > > > 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
> > > > > @@ -333,12 +333,16 @@ rzg2l_cpg_register_core_clk(const struct
> > > > cpg_core_clk *core,
> > > > > * @hw: handle between common and hardware-specific interfaces
> > > > > * @off: register offset
> > > > > * @bit: ON/MON bit
> > > > > + * @is_coupled: flag to indicate coupled clock
> > > > > + * @on_cnt: ON count for coupled clocks
> > > > > * @priv: CPG/MSTP private data
> > > > > */
> > > > > struct mstp_clock {
> > > > > struct clk_hw hw;
> > > > > u16 off;
> > > > > u8 bit;
> > > > > + bool is_coupled;
> > > > > + u8 on_cnt;
> > > >
> > > > While u8 is probably sufficient, you may want to use unsigned int,
> > > > as there will be a gap anyway due to alignment rules.
> > > >
> > > > > struct rzg2l_cpg_priv *priv; };
> > > > >
> > > > > @@ -392,11 +396,37 @@ static int rzg2l_mod_clock_endisable(struct
> > > > > clk_hw *hw, bool enable)
> > > > >
> > > > > static int rzg2l_mod_clock_enable(struct clk_hw *hw) {
> > > > > + struct mstp_clock *clock = to_mod_clock(hw);
> > > > > + struct rzg2l_cpg_priv *priv = clock->priv;
> > > > > + unsigned long flags;
> > > > > +
> > > > > + spin_lock_irqsave(&priv->rmw_lock, flags);
> > > > > + clock->on_cnt++;
> > > > > + if (clock->is_coupled && clock->on_cnt > 1) {
> > > > > + spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > > > > + return 1;
> > > > > + }
> > > > > +
> > > > > + spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > > >
> > > > I think you can avoid taking the spinlock and touching the counter
> > > > if the is_coupled flag is not set.
> > >
> > > OK.
> > >
> > > >
> > > > > +
> > > > > return rzg2l_mod_clock_endisable(hw, true); }
> > > >
> > > > However, I'm wondering how this can work?
> > > >
> > > > DEF_COUPLED("eth0_axi", R9A07G044_ETH0_CLK_AXI,
> > R9A07G044_CLK_M0,
> > > > 0x57c, 0),
> > > > DEF_COUPLED("eth0_chi", R9A07G044_ETH0_CLK_CHI,
> > R9A07G044_CLK_ZT,
> > > > 0x57c, 0),
> > > >
> > > > This will create 2 independent clocks, each with their own
> > > > mstp_clock structure that has the is_coupled flag set. Hence each
> > > > clock has its own counter. Shouldn't the counter be shared?
> > > > Am I missing something?
> > >
> > > Oops. You are correct. I need to add this counter to priv instead of
> > mstp_clocks.
> >
> > On second thought, a counter is overkill. A simple flag should be
> > sufficient, as the clk core only calls .{en,dis}able() when the clock is
> > {dis,en}enabled.
>
> Just to clarify, simple flag, did you mean to use bit flag? (ie, 2 bits , since we have 2 module clocks)
> when core clock calls enable, set a bit and reset the bit during disable.
>
> Then based on the 2bits, either turn on/off clock or just return the status.
>
> Please correct me, if my understanding wrong?
Just one bool or bit in a bitfield, the second flag will be in the
other struct mstp_clock (can there be three coupled clocks?).
So I think something like below should work:
struct mstp_clock {
struct clk_hw hw;
u16 off;
u8 bit;
+ bool enabled;
struct rzg2l_cpg_priv *priv;
+ struct mstp_clock *siblings;
};
.enabled needs to track the soft state of the clock.
The actual coupling is handled through .siblings, which points to the
other coupled clock (or forms a circular list if you can have more than
two coupled clocks). When registering a clock, if mod->is_coupled
is set, you walk all already registered module clocks to find one
with the same off and bit, and link them together.
In .{en,dis}able(), you only {en,dis}able the hardware clock if all
other clocks in the list are disabled.
if it turns out too costly to add a pointer to each clock (depends
on slab granularity), you can also use a different struct for coupled
clocks:
struct mstp_coupled_clock {
struct mstp_clock mstp;
struct mstp_coupled_clock *siblings;
};
but then you do need another flag in mstp_clock to indicate it is
a coupled clock, that can be converted to mstp_coupled_clock using
container_of().
Does that make sense?
Have a nice weekend!
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks
2021-08-13 12:46 ` Geert Uytterhoeven
@ 2021-08-13 18:01 ` Biju Das
0 siblings, 0 replies; 11+ messages in thread
From: Biju Das @ 2021-08-13 18:01 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Michael Turquette, Stephen Boyd, Linux-Renesas, linux-clk,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad
Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support
> to handle coupled clocks
>
> Hi Biju,
>
> On Fri, Aug 13, 2021 at 2:17 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add
> > > support to handle coupled clocks On Thu, Aug 12, 2021 at 9:00 AM
> > > Biju Das <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > > Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg:
> > > > > Add support to handle coupled clocks On Tue, Jul 27, 2021 at
> > > > > 4:18 PM Biju Das <biju.das.jz@bp.renesas.com>
> > > > > wrote:
> > > > > > The AXI and CHI clocks use the same register bit for
> > > > > > controlling clock output. Add a new clock type for coupled
> > > > > > clocks, which sets the CPG_CLKON_ETH.CLK[01]_ON bit when at
> > > > > > least one clock is enabled, and clears the bit only when both
> clocks are disabled.
> > > > > >
> > > > > > 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
> > > > > > @@ -333,12 +333,16 @@ rzg2l_cpg_register_core_clk(const struct
> > > > > cpg_core_clk *core,
> > > > > > * @hw: handle between common and hardware-specific interfaces
> > > > > > * @off: register offset
> > > > > > * @bit: ON/MON bit
> > > > > > + * @is_coupled: flag to indicate coupled clock
> > > > > > + * @on_cnt: ON count for coupled clocks
> > > > > > * @priv: CPG/MSTP private data
> > > > > > */
> > > > > > struct mstp_clock {
> > > > > > struct clk_hw hw;
> > > > > > u16 off;
> > > > > > u8 bit;
> > > > > > + bool is_coupled;
> > > > > > + u8 on_cnt;
> > > > >
> > > > > While u8 is probably sufficient, you may want to use unsigned
> > > > > int, as there will be a gap anyway due to alignment rules.
> > > > >
> > > > > > struct rzg2l_cpg_priv *priv; };
> > > > > >
> > > > > > @@ -392,11 +396,37 @@ static int
> > > > > > rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
> > > > > >
> > > > > > static int rzg2l_mod_clock_enable(struct clk_hw *hw) {
> > > > > > + struct mstp_clock *clock = to_mod_clock(hw);
> > > > > > + struct rzg2l_cpg_priv *priv = clock->priv;
> > > > > > + unsigned long flags;
> > > > > > +
> > > > > > + spin_lock_irqsave(&priv->rmw_lock, flags);
> > > > > > + clock->on_cnt++;
> > > > > > + if (clock->is_coupled && clock->on_cnt > 1) {
> > > > > > + spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > > > > > + return 1;
> > > > > > + }
> > > > > > +
> > > > > > + spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > > > >
> > > > > I think you can avoid taking the spinlock and touching the
> > > > > counter if the is_coupled flag is not set.
> > > >
> > > > OK.
> > > >
> > > > >
> > > > > > +
> > > > > > return rzg2l_mod_clock_endisable(hw, true); }
> > > > >
> > > > > However, I'm wondering how this can work?
> > > > >
> > > > > DEF_COUPLED("eth0_axi", R9A07G044_ETH0_CLK_AXI,
> > > R9A07G044_CLK_M0,
> > > > > 0x57c, 0),
> > > > > DEF_COUPLED("eth0_chi", R9A07G044_ETH0_CLK_CHI,
> > > R9A07G044_CLK_ZT,
> > > > > 0x57c, 0),
> > > > >
> > > > > This will create 2 independent clocks, each with their own
> > > > > mstp_clock structure that has the is_coupled flag set. Hence
> > > > > each clock has its own counter. Shouldn't the counter be shared?
> > > > > Am I missing something?
> > > >
> > > > Oops. You are correct. I need to add this counter to priv instead
> > > > of
> > > mstp_clocks.
> > >
> > > On second thought, a counter is overkill. A simple flag should be
> > > sufficient, as the clk core only calls .{en,dis}able() when the
> > > clock is {dis,en}enabled.
> >
> > Just to clarify, simple flag, did you mean to use bit flag? (ie, 2
> > bits , since we have 2 module clocks) when core clock calls enable, set
> a bit and reset the bit during disable.
> >
> > Then based on the 2bits, either turn on/off clock or just return the
> status.
> >
> > Please correct me, if my understanding wrong?
>
> Just one bool or bit in a bitfield, the second flag will be in the other
> struct mstp_clock (can there be three coupled clocks?).
RZ/G2L have maximum 2 coupled clocks.
>
> So I think something like below should work:
>
> struct mstp_clock {
> struct clk_hw hw;
> u16 off;
> u8 bit;
> + bool enabled;
> struct rzg2l_cpg_priv *priv;
> + struct mstp_clock *siblings;
> };
>
> .enabled needs to track the soft state of the clock.
> The actual coupling is handled through .siblings, which points to the
> other coupled clock (or forms a circular list if you can have more than
> two coupled clocks). When registering a clock, if mod->is_coupled is set,
> you walk all already registered module clocks to find one with the same
> off and bit, and link them together.
Thanks, Will prototype based on the above solution.
>
> In .{en,dis}able(), you only {en,dis}able the hardware clock if all other
> clocks in the list are disabled.
>
> if it turns out too costly to add a pointer to each clock (depends on slab
> granularity), you can also use a different struct for coupled
> clocks:
>
> struct mstp_coupled_clock {
> struct mstp_clock mstp;
> struct mstp_coupled_clock *siblings;
> };
>
> but then you do need another flag in mstp_clock to indicate it is a
> coupled clock, that can be converted to mstp_coupled_clock using
> container_of().
>
> Does that make sense?
Yes. Thanks for pointers.
Regards,
Biju
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 4/4] drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset
2021-07-27 14:17 [PATCH v2 0/4] Add GbEthernet Clock support Biju Das
` (2 preceding siblings ...)
2021-07-27 14:17 ` [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks Biju Das
@ 2021-07-27 14:17 ` Biju Das
3 siblings, 0 replies; 11+ messages in thread
From: Biju Das @ 2021-07-27 14:17 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: Biju Das, Geert Uytterhoeven, linux-renesas-soc, linux-clk,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad
Add ETH{0,1} clock/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>
---
v1->v2:
* Register axi/chi clock as coupled clocks
v1:-
* New patch
---
drivers/clk/renesas/r9a07g044-cpg.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index acf19a6cde31..1745e363e5a6 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -140,6 +140,14 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
0x578, 2),
DEF_MOD("usb_pclk", R9A07G044_USB_PCLK, R9A07G044_CLK_P1,
0x578, 3),
+ DEF_COUPLED("eth0_axi", R9A07G044_ETH0_CLK_AXI, R9A07G044_CLK_M0,
+ 0x57c, 0),
+ DEF_COUPLED("eth0_chi", R9A07G044_ETH0_CLK_CHI, R9A07G044_CLK_ZT,
+ 0x57c, 0),
+ DEF_COUPLED("eth1_axi", R9A07G044_ETH1_CLK_AXI, R9A07G044_CLK_M0,
+ 0x57c, 1),
+ DEF_COUPLED("eth1_chi", R9A07G044_ETH1_CLK_CHI, R9A07G044_CLK_ZT,
+ 0x57c, 1),
DEF_MOD("i2c0", R9A07G044_I2C0_PCLK, R9A07G044_CLK_P0,
0x580, 0),
DEF_MOD("i2c1", R9A07G044_I2C1_PCLK, R9A07G044_CLK_P0,
@@ -184,6 +192,8 @@ static struct rzg2l_reset r9a07g044_resets[] = {
DEF_RST(R9A07G044_USB_U2H1_HRESETN, 0x878, 1),
DEF_RST(R9A07G044_USB_U2P_EXL_SYSRST, 0x878, 2),
DEF_RST(R9A07G044_USB_PRESETN, 0x878, 3),
+ DEF_RST(R9A07G044_ETH0_RST_HW_N, 0x87c, 0),
+ DEF_RST(R9A07G044_ETH1_RST_HW_N, 0x87c, 1),
DEF_RST(R9A07G044_I2C0_MRST, 0x880, 0),
DEF_RST(R9A07G044_I2C1_MRST, 0x880, 1),
DEF_RST(R9A07G044_I2C2_MRST, 0x880, 2),
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread