Re: [PATCH v9 2/6] dt: bindings: add mt7621-clk device tree binding documentation

[Sergio Paracuellos]

Hi,
On Sat, Mar 6, 2021 at 10:54 AM Sergio Paracuellos
<sergio.paracuellos@xxxxxxxxx> wrote:
>
> Hi again,
>
> On Sat, Mar 6, 2021 at 8:12 AM Sergio Paracuellos
> <sergio.paracuellos@xxxxxxxxx> wrote:
> >
> > Hi Rob,
> >
> > On Fri, Mar 5, 2021 at 11:47 PM Rob Herring <robh@xxxxxxxxxx> wrote:
> > [snip]
> > > > +
> > > > +  ralink,sysctl:
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > +    description:
> > > > +      phandle of syscon used to control system registers
> > > > +
> > > > +  ralink,memctl:
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > +    description:
> > > > +      phandle of syscon used to control memory registers
> > >
> > > I assume one of these phandles are the main registers for the clocks?
> > > Make this a child node and drop that phandle.
> >
> > The 'ralink,sysctl' phandle is to read bootstrap register to be able
> > to derive xtal and a clk gate register for the peripherals.
> > The 'ralink,memctl' phandle is to read the cpu clock frequency from
> > the memory controller.
> >
> > So there is not "main registers". I already put this as a child node
> > in v4 and I was told to get rid of child nodes. I need this as a
> > regmap to other DT node registers (sysctl, and memctl) to be able to
> > use the driver without specific architecture operations and properly
> > enable for COMPILE_TEST without dirty Makefile arch flags. Both sysctl
> > and memctl has no other child nodes, and I think that's why I was told
> > to avoid child nodes at the end. I explained here [0] current sysctl
> > and memctl in the mt7621 device tree and my view of the need for this
> > two syscons:
> >
> > [0]: https://lkml.org/lkml/2021/1/2/9
> >
> > So to avoid to send again "a previous version" on this patch, please
> > guide me in the correct thing to do. Stephen, Rob, I will be really
> > happy with your help :)
>
> Since there are no other child nodes for this sysc, should merge clock
> properties
> with this node in the following way a valid approach:
>
>  sysc: sysc@0 {
>      compatible = "mediatek,mt7621-sysc", "syscon";
>      reg = <0x0 0x100>;
>      #clock-cells = <1>;
>      ralink,memctl = <&memc>;
>      clock-output-names = "xtal", "cpu", "bus",
>                                         "50m", "125m", "150m",
>                                         "250m", "270m";
> };
>
> Consumer clock:
>
> node: node@0 {
>   ...
>   clocks = <&sysc MT7621_CLK_WHATEVER>;
>  ...
> };

I have been reviewing bindings review comments along the time and I
was already suggested to do this I am saying here (see [0]) but my
mind seems that filtered it for any reason I don't really understand.
Maybe I should sleep a bit more :).

I will send v10 with these changes that hopefully will be the correct ones.

Thanks and sorry for bothering you with already suggested things.

Best regards,
    Sergio Paracuellos

[0]: https://lkml.org/lkml/2020/12/31/206

>
> If that is the case... and since 'sysc' is used as system control
> registers for all the rest of the world, where should be the yaml file
> with bindings placed?
>
> Thanks in advance again for your help.
>
> Best regards,
>     Sergio Paracuellos
>
> >
> > Best regards,
> >     Sergio Paracuellos
> > >
> > > > +
> > > > +  clock-output-names:
> > > > +    maxItems: 8
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - '#clock-cells'
> > > > +  - ralink,sysctl
> > > > +  - ralink,memctl
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/clock/mt7621-clk.h>
> > > > +
> > > > +    pll {
> > > > +      compatible = "mediatek,mt7621-clk";
> > > > +      #clock-cells = <1>;
> > > > +      ralink,sysctl = <&sysc>;
> > > > +      ralink,memctl = <&memc>;
> > > > +      clock-output-names = "xtal", "cpu", "bus",
> > > > +                           "50m", "125m", "150m",
> > > > +                           "250m", "270m";
> > > > +    };
> > > > --
> > > > 2.25.1
> > > >