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 <[email protected]>
> ---
> .../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 <[email protected]>
> +
> +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.
Hi Stephen,
On Sun, 20 Dec 2020 at 12:39, Stephen Boyd <[email protected]> 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
Quoting Daniel Palmer (2020-12-19 22:35:41)
> Hi Stephen,
>
> On Sun, 20 Dec 2020 at 12:39, Stephen Boyd <[email protected]> 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.
Hi Stephen,
On Mon, 21 Dec 2020 at 03:44, Stephen Boyd <[email protected]> wrote:
>
> Quoting Daniel Palmer (2020-12-19 22:35:41)
> > Hi Stephen,
> >
> > On Sun, 20 Dec 2020 at 12:39, Stephen Boyd <[email protected]> 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
Quoting Daniel Palmer (2020-12-21 00:51:56)
> Hi Stephen,
>
> On Mon, 21 Dec 2020 at 03:44, Stephen Boyd <[email protected]> wrote:
> >
> > Quoting Daniel Palmer (2020-12-19 22:35:41)
> > > Hi Stephen,
> > >
> > > On Sun, 20 Dec 2020 at 12:39, Stephen Boyd <[email protected]> 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.
Hi Stephen,
On Wed, 10 Feb 2021 at 11:29, Stephen Boyd <[email protected]> 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
Quoting Daniel Palmer (2021-02-10 18:28:40)
> Hi Stephen,
>
> On Wed, 10 Feb 2021 at 11:29, Stephen Boyd <[email protected]> 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!