diff mbox series

[2/6] dt-bindings: clk: mstar msc313 mpll binding description

Message ID 20201114135044.724385-3-daniel@0x0f.com
State Not Applicable
Headers show
Series ARM: mstar: Basic MPLL support | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema success

Commit Message

Daniel Palmer Nov. 14, 2020, 1:50 p.m. UTC
Add a binding description for the MStar/SigmaStar MPLL clock block.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 .../bindings/clock/mstar,msc313-mpll.yaml     | 58 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml

Comments

Rob Herring Dec. 7, 2020, 7:03 p.m. UTC | #1
On Sat, 14 Nov 2020 22:50:40 +0900, Daniel Palmer wrote:
> Add a binding description for the MStar/SigmaStar MPLL clock block.
> 
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> ---
>  .../bindings/clock/mstar,msc313-mpll.yaml     | 58 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Stephen Boyd Dec. 20, 2020, 3:39 a.m. UTC | #2
Quoting Daniel Palmer (2020-11-14 05:50:40)
> Add a binding description for the MStar/SigmaStar MPLL clock block.
> 
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> ---
>  .../bindings/clock/mstar,msc313-mpll.yaml     | 58 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml b/Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
> new file mode 100644
> index 000000000000..9ddc1163b31b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/mstar,msc313-mpll.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MStar/Sigmastar MSC313 MPLL
> +
> +maintainers:
> +  - Daniel Palmer <daniel@thingy.jp>
> +
> +description: |
> +  The MStar/SigmaStar MSC313 and later ARMv7 chips have an MPLL block that
> +  takes the external xtal input and multiplies it to create a high
> +  frequency clock and divides that down into a number of clocks that
> +  peripherals use.
> +
> +properties:
> +  compatible:
> +    const: mstar,msc313-mpll
> +
> +  "#clock-cells":
> +    const: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-output-names:
> +    minItems: 8
> +    maxItems: 8
> +    description: |
> +      This should provide a name for the internal PLL clock and then
> +      a name for each of the divided outputs.

Is this necessary?

> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - "#clock-cells"
> +  - clocks
> +  - clock-output-names
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    mpll@206000 {
> +        compatible = "mstar,msc313-mpll";
> +        reg = <0x206000 0x200>;
> +        #clock-cells = <1>;
> +        clocks = <&xtal>;
> +        clock-output-names = "mpll", "mpll_div_2",
> +                             "mpll_div_3", "mpll_div_4",
> +                             "mpll_div_5", "mpll_div_6",
> +                             "mpll_div_7", "mpll_div_10";

It looks like it can be derived in the driver.
Daniel Palmer Dec. 20, 2020, 6:35 a.m. UTC | #3
Hi Stephen,

On Sun, 20 Dec 2020 at 12:39, Stephen Boyd <sboyd@kernel.org> wrote:
> > +  clock-output-names:
> > +    minItems: 8
> > +    maxItems: 8
> > +    description: |
> > +      This should provide a name for the internal PLL clock and then
> > +      a name for each of the divided outputs.
>
> Is this necessary?

I found without the names specified in the dt probing of muxes that
depend on the outputs but appear earlier didn't work.
Also this same PLL layout seems to be used in some other places so
eventually I was thinking this driver would get used for those PLLs
with different output names.

Thanks,

Daniel
Stephen Boyd Dec. 20, 2020, 6:44 p.m. UTC | #4
Quoting Daniel Palmer (2020-12-19 22:35:41)
> Hi Stephen,
> 
> On Sun, 20 Dec 2020 at 12:39, Stephen Boyd <sboyd@kernel.org> wrote:
> > > +  clock-output-names:
> > > +    minItems: 8
> > > +    maxItems: 8
> > > +    description: |
> > > +      This should provide a name for the internal PLL clock and then
> > > +      a name for each of the divided outputs.
> >
> > Is this necessary?
> 
> I found without the names specified in the dt probing of muxes that
> depend on the outputs but appear earlier didn't work.
> Also this same PLL layout seems to be used in some other places so
> eventually I was thinking this driver would get used for those PLLs
> with different output names.

Still seems like it could be auto-generated based on dev_name() +
number. Now that we have a way to specify clk parents via the clocks
property in DT (without any clock-names required) we should be able to
avoid needing clock-output-names in general.
Daniel Palmer Dec. 21, 2020, 8:51 a.m. UTC | #5
Hi Stephen,

On Mon, 21 Dec 2020 at 03:44, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Daniel Palmer (2020-12-19 22:35:41)
> > Hi Stephen,
> >
> > On Sun, 20 Dec 2020 at 12:39, Stephen Boyd <sboyd@kernel.org> wrote:
> > > > +  clock-output-names:
> > > > +    minItems: 8
> > > > +    maxItems: 8
> > > > +    description: |
> > > > +      This should provide a name for the internal PLL clock and then
> > > > +      a name for each of the divided outputs.
> > >
> > > Is this necessary?
> >
> > I found without the names specified in the dt probing of muxes that
> > depend on the outputs but appear earlier didn't work.
> > Also this same PLL layout seems to be used in some other places so
> > eventually I was thinking this driver would get used for those PLLs
> > with different output names.
>
> Still seems like it could be auto-generated based on dev_name() +
> number.

At one point I had something similar to that where the output names
were generated at probe.
Without the clock outputs listed in the device tree clock muxes that
source clocks from the mpll couldn't probe properly as they couldn't
look up all of their parents if they probed before the mpll.
Maybe I'm doing something wrong there? I couldn't find a way to always
resolve all of the parents or defer the probe of the muxes until the
mpll clocks are registered.

Cheers,

Daniel
Stephen Boyd Feb. 10, 2021, 2:29 a.m. UTC | #6
Quoting Daniel Palmer (2020-12-21 00:51:56)
> Hi Stephen,
> 
> On Mon, 21 Dec 2020 at 03:44, Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Daniel Palmer (2020-12-19 22:35:41)
> > > Hi Stephen,
> > >
> > > On Sun, 20 Dec 2020 at 12:39, Stephen Boyd <sboyd@kernel.org> wrote:
> > > > > +  clock-output-names:
> > > > > +    minItems: 8
> > > > > +    maxItems: 8
> > > > > +    description: |
> > > > > +      This should provide a name for the internal PLL clock and then
> > > > > +      a name for each of the divided outputs.
> > > >
> > > > Is this necessary?
> > >
> > > I found without the names specified in the dt probing of muxes that
> > > depend on the outputs but appear earlier didn't work.
> > > Also this same PLL layout seems to be used in some other places so
> > > eventually I was thinking this driver would get used for those PLLs
> > > with different output names.
> >
> > Still seems like it could be auto-generated based on dev_name() +
> > number.
> 
> At one point I had something similar to that where the output names
> were generated at probe.
> Without the clock outputs listed in the device tree clock muxes that
> source clocks from the mpll couldn't probe properly as they couldn't
> look up all of their parents if they probed before the mpll.
> Maybe I'm doing something wrong there? I couldn't find a way to always
> resolve all of the parents or defer the probe of the muxes until the
> mpll clocks are registered.
> 

The child clks should be using clk_parent_data to point to the parent
clks through DT. That way the name of the clk doesn't matter except for
debug purposes.
Daniel Palmer Feb. 11, 2021, 2:28 a.m. UTC | #7
Hi Stephen,

On Wed, 10 Feb 2021 at 11:29, Stephen Boyd <sboyd@kernel.org> wrote:
> The child clks should be using clk_parent_data to point to the parent
> clks through DT. That way the name of the clk doesn't matter except for
> debug purposes.

I think I get it now. I was using of_clk_parent_fill() to get the
parent clocks sourced
from the mpll but I seems like I should be filling out an array of
struct clk_parent_data
with the indices of the parents and using
clk_register_composite_pdata() etc instead.

Thanks!

Daniel
Stephen Boyd Feb. 11, 2021, 2:55 a.m. UTC | #8
Quoting Daniel Palmer (2021-02-10 18:28:40)
> Hi Stephen,
> 
> On Wed, 10 Feb 2021 at 11:29, Stephen Boyd <sboyd@kernel.org> wrote:
> > The child clks should be using clk_parent_data to point to the parent
> > clks through DT. That way the name of the clk doesn't matter except for
> > debug purposes.
> 
> I think I get it now. I was using of_clk_parent_fill() to get the
> parent clocks sourced
> from the mpll but I seems like I should be filling out an array of
> struct clk_parent_data
> with the indices of the parents and using
> clk_register_composite_pdata() etc instead.
> 

Yes that's the idea. Thanks!
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml b/Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
new file mode 100644
index 000000000000..9ddc1163b31b
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
@@ -0,0 +1,58 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/mstar,msc313-mpll.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MStar/Sigmastar MSC313 MPLL
+
+maintainers:
+  - Daniel Palmer <daniel@thingy.jp>
+
+description: |
+  The MStar/SigmaStar MSC313 and later ARMv7 chips have an MPLL block that
+  takes the external xtal input and multiplies it to create a high
+  frequency clock and divides that down into a number of clocks that
+  peripherals use.
+
+properties:
+  compatible:
+    const: mstar,msc313-mpll
+
+  "#clock-cells":
+    const: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-output-names:
+    minItems: 8
+    maxItems: 8
+    description: |
+      This should provide a name for the internal PLL clock and then
+      a name for each of the divided outputs.
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - "#clock-cells"
+  - clocks
+  - clock-output-names
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    mpll@206000 {
+        compatible = "mstar,msc313-mpll";
+        reg = <0x206000 0x200>;
+        #clock-cells = <1>;
+        clocks = <&xtal>;
+        clock-output-names = "mpll", "mpll_div_2",
+                             "mpll_div_3", "mpll_div_4",
+                             "mpll_div_5", "mpll_div_6",
+                             "mpll_div_7", "mpll_div_10";
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 1e874fda810e..d1c98a6f9f24 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2131,6 +2131,7 @@  L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
 W:	http://linux-chenxing.org/
 F:	Documentation/devicetree/bindings/arm/mstar/*
+F:	Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
 F:	Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml
 F:	arch/arm/boot/dts/mstar-*
 F:	arch/arm/mach-mstar/