[RFC PATCH 0/3] Extend lan966x clock driver for clock gating support

[RFC PATCH 0/3] Extend lan966x clock driver for clock gating support

[Horatiu Vultur]

This patch series depends on the following series, therefor keep it as RFC.
https://www.spinics.net/lists/linux-clk/msg62237.html

This patch series extend the clock driver to support also clock gating.

Horatiu Vultur (3):
  dt-bindings: clock: lan966x: Extend for clock gate support
  dt-bindings: clock: lan966x: Extend includes with clock gates
  clk: lan966x: Extend lan966x clock driver for clock gating support

 .../bindings/clock/microchip,lan966x-gck.yaml | 13 +++-
 drivers/clk/clk-lan966x.c                     | 72 +++++++++++++++++--
 include/dt-bindings/clock/microchip,lan966x.h |  8 ++-
 3 files changed, 86 insertions(+), 7 deletions(-)

-- 
2.33.0

[RFC PATCH 1/3] dt-bindings: clock: lan966x: Extend for clock gate support

[Horatiu Vultur]

It is required to add a new resource to be able to access the clock gate
registers. Now that we have 2 resources, add also reg-names property to
make more clear.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../bindings/clock/microchip,lan966x-gck.yaml       | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/microchip,lan966x-gck.yaml b/Documentation/devicetree/bindings/clock/microchip,lan966x-gck.yaml
index fca83bd68e26..047c77e049f1 100644
--- a/Documentation/devicetree/bindings/clock/microchip,lan966x-gck.yaml
+++ b/Documentation/devicetree/bindings/clock/microchip,lan966x-gck.yaml
@@ -19,7 +19,14 @@ properties:
     const: microchip,lan966x-gck
 
   reg:
-    maxItems: 1
+    items:
+      - description: core registers
+      - description: gate registers
+
+  reg-names:
+    items:
+      - const: core
+      - const: gate
 
   clocks:
     items:
@@ -39,6 +46,7 @@ properties:
 required:
   - compatible
   - reg
+  - reg-names
   - clocks
   - clock-names
   - '#clock-cells'
@@ -52,6 +60,7 @@ examples:
         #clock-cells = <1>;
         clocks = <&cpu_clk>, <&ddr_clk>, <&sys_clk>;
         clock-names = "cpu", "ddr", "sys";
-        reg = <0xe00c00a8 0x38>;
+        reg = <0xe00c00a8 0x38>, <0xe00c02cc 0x4>;
+        reg-names = "core", "gate";
     };
 ...
-- 
2.33.0

Re: [RFC PATCH 1/3] dt-bindings: clock: lan966x: Extend for clock gate support

[Rob Herring]

On Tue, Oct 19, 2021 at 10:44:47AM +0200, Horatiu Vultur wrote:
> It is required to add a new resource to be able to access the clock gate
> registers. Now that we have 2 resources, add also reg-names property to
> make more clear.

It is an ABI breakage to require 2 reg entries. If that's okay for this 
binding, you need to explain why. The binding requiring 2 so that DT 
files get updated, but the driver allowing for 1 is okay.

> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  .../bindings/clock/microchip,lan966x-gck.yaml       | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/microchip,lan966x-gck.yaml b/Documentation/devicetree/bindings/clock/microchip,lan966x-gck.yaml
> index fca83bd68e26..047c77e049f1 100644
> --- a/Documentation/devicetree/bindings/clock/microchip,lan966x-gck.yaml
> +++ b/Documentation/devicetree/bindings/clock/microchip,lan966x-gck.yaml
> @@ -19,7 +19,14 @@ properties:
>      const: microchip,lan966x-gck
>  
>    reg:
> -    maxItems: 1
> +    items:
> +      - description: core registers
> +      - description: gate registers
> +
> +  reg-names:
> +    items:
> +      - const: core
> +      - const: gate
>  
>    clocks:
>      items:
> @@ -39,6 +46,7 @@ properties:
>  required:
>    - compatible
>    - reg
> +  - reg-names
>    - clocks
>    - clock-names
>    - '#clock-cells'
> @@ -52,6 +60,7 @@ examples:
>          #clock-cells = <1>;
>          clocks = <&cpu_clk>, <&ddr_clk>, <&sys_clk>;
>          clock-names = "cpu", "ddr", "sys";
> -        reg = <0xe00c00a8 0x38>;
> +        reg = <0xe00c00a8 0x38>, <0xe00c02cc 0x4>;
> +        reg-names = "core", "gate";
>      };
>  ...
> -- 
> 2.33.0
> 
> 

Re: [RFC PATCH 1/3] dt-bindings: clock: lan966x: Extend for clock gate support

[Horatiu Vultur]

The 10/26/2021 22:08, Rob Herring wrote:

Hi Rob,

> 
> On Tue, Oct 19, 2021 at 10:44:47AM +0200, Horatiu Vultur wrote:
> > It is required to add a new resource to be able to access the clock gate
> > registers. Now that we have 2 resources, add also reg-names property to
> > make more clear.
> 
> It is an ABI breakage to require 2 reg entries. If that's okay for this
> binding, you need to explain why. The binding requiring 2 so that DT
> files get updated, but the driver allowing for 1 is okay.

I will make the resource optional not to break any ABI.

> 
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  .../bindings/clock/microchip,lan966x-gck.yaml       | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/microchip,lan966x-gck.yaml b/Documentation/devicetree/bindings/clock/microchip,lan966x-gck.yaml
> > index fca83bd68e26..047c77e049f1 100644
> > --- a/Documentation/devicetree/bindings/clock/microchip,lan966x-gck.yaml
> > +++ b/Documentation/devicetree/bindings/clock/microchip,lan966x-gck.yaml
> > @@ -19,7 +19,14 @@ properties:
> >      const: microchip,lan966x-gck
> >
> >    reg:
> > -    maxItems: 1
> > +    items:
> > +      - description: core registers
> > +      - description: gate registers
> > +
> > +  reg-names:
> > +    items:
> > +      - const: core
> > +      - const: gate
> >
> >    clocks:
> >      items:
> > @@ -39,6 +46,7 @@ properties:
> >  required:
> >    - compatible
> >    - reg
> > +  - reg-names
> >    - clocks
> >    - clock-names
> >    - '#clock-cells'
> > @@ -52,6 +60,7 @@ examples:
> >          #clock-cells = <1>;
> >          clocks = <&cpu_clk>, <&ddr_clk>, <&sys_clk>;
> >          clock-names = "cpu", "ddr", "sys";
> > -        reg = <0xe00c00a8 0x38>;
> > +        reg = <0xe00c00a8 0x38>, <0xe00c02cc 0x4>;
> > +        reg-names = "core", "gate";
> >      };
> >  ...
> > --
> > 2.33.0
> >
> >

-- 
/Horatiu

[RFC PATCH 2/3] dt-bindings: clock: lan966x: Extend includes with clock gates

[Horatiu Vultur]

On lan966x it is allow to control the clock to some peripherals like
USB. So extend the include file with these clocks.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 include/dt-bindings/clock/microchip,lan966x.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/dt-bindings/clock/microchip,lan966x.h b/include/dt-bindings/clock/microchip,lan966x.h
index fe36ed6d8b5f..6f9d43d76d5a 100644
--- a/include/dt-bindings/clock/microchip,lan966x.h
+++ b/include/dt-bindings/clock/microchip,lan966x.h
@@ -23,6 +23,12 @@
 #define GCK_ID_TIMER		12
 #define GCK_ID_USB_REFCLK	13
 
-#define N_CLOCKS		14
+/* Gate clocks */
+#define GCK_GATE_UHPHS		14
+#define GCK_GATE_UDPHS		15
+#define GCK_GATE_MCRAMC		16
+#define GCK_GATE_HMATRIX	17
+
+#define N_CLOCKS		18
 
 #endif
-- 
2.33.0

Re: [RFC PATCH 2/3] dt-bindings: clock: lan966x: Extend includes with clock gates

[Rob Herring]

On Tue, 19 Oct 2021 10:44:48 +0200, Horatiu Vultur wrote:
> On lan966x it is allow to control the clock to some peripherals like
> USB. So extend the include file with these clocks.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  include/dt-bindings/clock/microchip,lan966x.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

[RFC PATCH 3/3] clk: lan966x: Extend lan966x clock driver for clock gating support

[Horatiu Vultur]

Extend the clock driver to add support also for clock gating. The
following peripherals can be gated: UHPHS, UDPHS, MCRAMC, HMATRIX.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/clk/clk-lan966x.c | 72 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 68 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-lan966x.c b/drivers/clk/clk-lan966x.c
index 19bec94e1551..40be47092a31 100644
--- a/drivers/clk/clk-lan966x.c
+++ b/drivers/clk/clk-lan966x.c
@@ -48,6 +48,20 @@ static struct clk_init_data init = {
 	.num_parents = ARRAY_SIZE(lan966x_gck_pdata),
 };
 
+struct clk_gate_soc_desc {
+	const char *name;
+	int bit_idx;
+};
+
+static const struct clk_gate_soc_desc clk_gate_desc[] = {
+	{ "uhphs", 11 },
+	{ "udphs", 10 },
+	{ "mcramc", 9 },
+	{ "hmatrix", 8 },
+	{ }
+};
+
+static DEFINE_SPINLOCK(clk_gate_lock);
 static void __iomem *base;
 
 static int lan966x_gck_enable(struct clk_hw *hw)
@@ -188,26 +202,64 @@ static struct clk_hw *lan966x_gck_clk_register(struct device *dev, int i)
 	return &priv->hw;
 };
 
+static int lan966x_gate_clk_register(struct device *dev,
+				     struct clk_hw_onecell_data *hw_data,
+				     void __iomem *gate_base)
+{
+	int i;
+
+	for (i = GCK_GATE_UHPHS; i < N_CLOCKS; ++i) {
+		int idx = i - GCK_GATE_UHPHS;
+
+		hw_data->hws[i] =
+			clk_hw_register_gate(dev, clk_gate_desc[idx].name,
+					     "lan966x", 0, base,
+					     clk_gate_desc[idx].bit_idx,
+					     0, &clk_gate_lock);
+
+		if (IS_ERR(hw_data->hws[i]))
+			return dev_err_probe(dev, PTR_ERR(hw_data->hws[i]),
+					     "failed to register %s clock\n",
+					     clk_gate_desc[idx].name);
+	}
+
+	return 0;
+}
+
+static void lan966x_gate_clk_unregister(struct clk_hw_onecell_data *hw_data)
+{
+	int i;
+
+	for (i = GCK_GATE_UHPHS; i < N_CLOCKS; ++i)
+		if (!IS_ERR(hw_data->hws[i]))
+			clk_hw_unregister(hw_data->hws[i]);
+}
+
 static int lan966x_clk_probe(struct platform_device *pdev)
 {
 	struct clk_hw_onecell_data *hw_data;
 	struct device *dev = &pdev->dev;
-	int i;
+	void __iomem *gate_base;
+	int i, ret;
 
 	hw_data = devm_kzalloc(dev, struct_size(hw_data, hws, N_CLOCKS),
 			       GFP_KERNEL);
 	if (!hw_data)
 		return -ENOMEM;
 
-	base = devm_platform_ioremap_resource(pdev, 0);
+	base = devm_platform_ioremap_resource_byname(pdev, "core");
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
+	gate_base = devm_platform_ioremap_resource_byname(pdev, "gate");
+	if (IS_ERR(gate_base))
+		return PTR_ERR(gate_base);
+
 	init.ops = &lan966x_gck_ops;
 
 	hw_data->num = N_CLOCKS;
 
-	for (i = 0; i < N_CLOCKS; i++) {
+	for (i = 0; i < GCK_ID_USB_REFCLK; i++) {
 		init.name = clk_names[i];
 		hw_data->hws[i] = lan966x_gck_clk_register(dev, i);
 		if (IS_ERR(hw_data->hws[i])) {
@@ -217,7 +269,19 @@ static int lan966x_clk_probe(struct platform_device *pdev)
 		}
 	}
 
-	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, hw_data);
+	ret = lan966x_gate_clk_register(dev, hw_data, gate_base);
+	if (ret)
+		goto unregister;
+
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, hw_data);
+	if (ret)
+		goto unregister;
+
+	return 0;
+
+unregister:
+	lan966x_gate_clk_unregister(hw_data);
+	return ret;
 }
 
 static const struct of_device_id lan966x_clk_dt_ids[] = {
-- 
2.33.0

Re: [RFC PATCH 3/3] clk: lan966x: Extend lan966x clock driver for clock gating support

[Stephen Boyd]

Quoting Horatiu Vultur (2021-10-19 01:44:49)
> diff --git a/drivers/clk/clk-lan966x.c b/drivers/clk/clk-lan966x.c
> index 19bec94e1551..40be47092a31 100644
> --- a/drivers/clk/clk-lan966x.c
> +++ b/drivers/clk/clk-lan966x.c
> @@ -188,26 +202,64 @@ static struct clk_hw *lan966x_gck_clk_register(struct device *dev, int i)
>         return &priv->hw;
>  };
>  
> +static int lan966x_gate_clk_register(struct device *dev,
> +                                    struct clk_hw_onecell_data *hw_data,
> +                                    void __iomem *gate_base)
> +{
> +       int i;
> +
> +       for (i = GCK_GATE_UHPHS; i < N_CLOCKS; ++i) {
> +               int idx = i - GCK_GATE_UHPHS;
> +
> +               hw_data->hws[i] =
> +                       clk_hw_register_gate(dev, clk_gate_desc[idx].name,

Use devm?

> +                                            "lan966x", 0, base,
> +                                            clk_gate_desc[idx].bit_idx,
> +                                            0, &clk_gate_lock);
> +
> +               if (IS_ERR(hw_data->hws[i]))
> +                       return dev_err_probe(dev, PTR_ERR(hw_data->hws[i]),
> +                                            "failed to register %s clock\n",
> +                                            clk_gate_desc[idx].name);
> +       }
> +
> +       return 0;
> +}
> +
> +static void lan966x_gate_clk_unregister(struct clk_hw_onecell_data *hw_data)
> +{
> +       int i;
> +
> +       for (i = GCK_GATE_UHPHS; i < N_CLOCKS; ++i)

for (int i = 

should suffice

> +               if (!IS_ERR(hw_data->hws[i]))
> +                       clk_hw_unregister(hw_data->hws[i]);
> +}
> +

Re: [RFC PATCH 3/3] clk: lan966x: Extend lan966x clock driver for clock gating support

[Horatiu Vultur]

The 10/28/2021 23:41, Stephen Boyd wrote:

Hi Stephen,

> 
> Quoting Horatiu Vultur (2021-10-19 01:44:49)
> > diff --git a/drivers/clk/clk-lan966x.c b/drivers/clk/clk-lan966x.c
> > index 19bec94e1551..40be47092a31 100644
> > --- a/drivers/clk/clk-lan966x.c
> > +++ b/drivers/clk/clk-lan966x.c
> > @@ -188,26 +202,64 @@ static struct clk_hw *lan966x_gck_clk_register(struct device *dev, int i)
> >         return &priv->hw;
> >  };
> >
> > +static int lan966x_gate_clk_register(struct device *dev,
> > +                                    struct clk_hw_onecell_data *hw_data,
> > +                                    void __iomem *gate_base)
> > +{
> > +       int i;
> > +
> > +       for (i = GCK_GATE_UHPHS; i < N_CLOCKS; ++i) {
> > +               int idx = i - GCK_GATE_UHPHS;
> > +
> > +               hw_data->hws[i] =
> > +                       clk_hw_register_gate(dev, clk_gate_desc[idx].name,
> 
> Use devm?

I couldn't find any devm_clk_hw_register_gate or something similar for
the gate.

> 
> > +                                            "lan966x", 0, base,
> > +                                            clk_gate_desc[idx].bit_idx,
> > +                                            0, &clk_gate_lock);
> > +
> > +               if (IS_ERR(hw_data->hws[i]))
> > +                       return dev_err_probe(dev, PTR_ERR(hw_data->hws[i]),
> > +                                            "failed to register %s clock\n",
> > +                                            clk_gate_desc[idx].name);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static void lan966x_gate_clk_unregister(struct clk_hw_onecell_data *hw_data)
> > +{
> > +       int i;
> > +
> > +       for (i = GCK_GATE_UHPHS; i < N_CLOCKS; ++i)
> 
> for (int i =
> 
> should suffice

That would not work. I will get the error:
error: ‘for’ loop initial declarations are only allowed in C99 or C11 mode

> 
> > +               if (!IS_ERR(hw_data->hws[i]))
> > +                       clk_hw_unregister(hw_data->hws[i]);
> > +}
> > +

-- 
/Horatiu

Re: [RFC PATCH 3/3] clk: lan966x: Extend lan966x clock driver for clock gating support

[Stephen Boyd]

Quoting Horatiu Vultur (2021-10-29 02:35:56)
> The 10/28/2021 23:41, Stephen Boyd wrote:
> 
> Hi Stephen,
> 
> > 
> > Quoting Horatiu Vultur (2021-10-19 01:44:49)
> > > diff --git a/drivers/clk/clk-lan966x.c b/drivers/clk/clk-lan966x.c
> > > index 19bec94e1551..40be47092a31 100644
> > > --- a/drivers/clk/clk-lan966x.c
> > > +++ b/drivers/clk/clk-lan966x.c
> > > @@ -188,26 +202,64 @@ static struct clk_hw *lan966x_gck_clk_register(struct device *dev, int i)
> > >         return &priv->hw;
> > >  };
> > >
> > > +static int lan966x_gate_clk_register(struct device *dev,
> > > +                                    struct clk_hw_onecell_data *hw_data,
> > > +                                    void __iomem *gate_base)
> > > +{
> > > +       int i;
> > > +
> > > +       for (i = GCK_GATE_UHPHS; i < N_CLOCKS; ++i) {
> > > +               int idx = i - GCK_GATE_UHPHS;
> > > +
> > > +               hw_data->hws[i] =
> > > +                       clk_hw_register_gate(dev, clk_gate_desc[idx].name,
> > 
> > Use devm?
> 
> I couldn't find any devm_clk_hw_register_gate or something similar for
> the gate.

Add one?

> 
> > 
> > > +                                            "lan966x", 0, base,
> > > +                                            clk_gate_desc[idx].bit_idx,
> > > +                                            0, &clk_gate_lock);
> > > +
> > > +               if (IS_ERR(hw_data->hws[i]))
> > > +                       return dev_err_probe(dev, PTR_ERR(hw_data->hws[i]),
> > > +                                            "failed to register %s clock\n",
> > > +                                            clk_gate_desc[idx].name);
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static void lan966x_gate_clk_unregister(struct clk_hw_onecell_data *hw_data)
> > > +{
> > > +       int i;
> > > +
> > > +       for (i = GCK_GATE_UHPHS; i < N_CLOCKS; ++i)
> > 
> > for (int i =
> > 
> > should suffice
> 
> That would not work. I will get the error:
> error: ‘for’ loop initial declarations are only allowed in C99 or C11 mode

Ah ok

Re: [RFC PATCH 3/3] clk: lan966x: Extend lan966x clock driver for clock gating support

[Horatiu Vultur]

The 10/29/2021 16:32, Stephen Boyd wrote:
> 
> Quoting Horatiu Vultur (2021-10-29 02:35:56)
> > The 10/28/2021 23:41, Stephen Boyd wrote:
> >
> > Hi Stephen,
> >
> > >
> > > Quoting Horatiu Vultur (2021-10-19 01:44:49)
> > > > diff --git a/drivers/clk/clk-lan966x.c b/drivers/clk/clk-lan966x.c
> > > > index 19bec94e1551..40be47092a31 100644
> > > > --- a/drivers/clk/clk-lan966x.c
> > > > +++ b/drivers/clk/clk-lan966x.c
> > > > @@ -188,26 +202,64 @@ static struct clk_hw *lan966x_gck_clk_register(struct device *dev, int i)
> > > >         return &priv->hw;
> > > >  };
> > > >
> > > > +static int lan966x_gate_clk_register(struct device *dev,
> > > > +                                    struct clk_hw_onecell_data *hw_data,
> > > > +                                    void __iomem *gate_base)
> > > > +{
> > > > +       int i;
> > > > +
> > > > +       for (i = GCK_GATE_UHPHS; i < N_CLOCKS; ++i) {
> > > > +               int idx = i - GCK_GATE_UHPHS;
> > > > +
> > > > +               hw_data->hws[i] =
> > > > +                       clk_hw_register_gate(dev, clk_gate_desc[idx].name,
> > >
> > > Use devm?
> >
> > I couldn't find any devm_clk_hw_register_gate or something similar for
> > the gate.
> 
> Add one?

Yes, I will do that.

> 
> >
> > >
> > > > +                                            "lan966x", 0, base,
> > > > +                                            clk_gate_desc[idx].bit_idx,
> > > > +                                            0, &clk_gate_lock);
> > > > +
> > > > +               if (IS_ERR(hw_data->hws[i]))
> > > > +                       return dev_err_probe(dev, PTR_ERR(hw_data->hws[i]),
> > > > +                                            "failed to register %s clock\n",
> > > > +                                            clk_gate_desc[idx].name);
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static void lan966x_gate_clk_unregister(struct clk_hw_onecell_data *hw_data)
> > > > +{
> > > > +       int i;
> > > > +
> > > > +       for (i = GCK_GATE_UHPHS; i < N_CLOCKS; ++i)
> > >
> > > for (int i =
> > >
> > > should suffice
> >
> > That would not work. I will get the error:
> > error: ‘for’ loop initial declarations are only allowed in C99 or C11 mode
> 
> Ah ok

-- 
/Horatiu