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

[Sergio Paracuellos]

On Fri, Jan 1, 2021 at 12:51 AM Sergio Paracuellos
<sergio.paracuellos@xxxxxxxxx> wrote:
>
> Hi Rob,
>
> Thanks for the review.

Hi again,

>
> On Thu, Dec 31, 2020 at 11:38 PM Rob Herring <robh@xxxxxxxxxx> wrote:
> >
> > On Sun, Dec 20, 2020 at 10:37:20AM +0100, Sergio Paracuellos wrote:
> > > Adds device tree binding documentation for clocks in the
> > > MT7621 SOC.
> > >
> > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
> > > ---
> > >  .../bindings/clock/mediatek,mt7621-clk.yaml   | 52 +++++++++++++++++++
> > >  1 file changed, 52 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml b/Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml
> > > new file mode 100644
> > > index 000000000000..f58d01bdc82c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml
> > > @@ -0,0 +1,52 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/clock/mediatek,mt7621-clk.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: MT7621 Clock Device Tree Bindings
> > > +
> > > +maintainers:
> > > +  - Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
> > > +
> > > +description: |
> > > +  The MT7621 has a PLL controller from where the cpu clock is provided
> > > +  as well as derived clocks for the bus and the peripherals. It also
> > > +  can gate SoC device clocks.
> > > +
> > > +  Each clock is assigned an identifier and client nodes use this identifier
> > > +  to specify the clock which they consume.
> > > +
> > > +  All these identifiers could be found in:
> > > +  [1]: <include/dt-bindings/clock/mt7621-clk.h>.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: mediatek,mt7621-clk
> > > +
> > > +  "#clock-cells":
> > > +    description:
> > > +      The first cell indicates the clock number, see [1] for available
> > > +      clocks.
> > > +    const: 1
> > > +
> > > +  clock-output-names:
> > > +    maxItems: 8
> > > +
> > > +required:
> > > +  - compatible
> > > +  - '#clock-cells'
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/clock/mt7621-clk.h>
> > > +
> > > +    pll {
> > > +      compatible = "mediatek,mt7621-clk";
> > > +      #clock-cells = <1>;
> > > +      clock-output-names = "xtal", "cpu", "bus",
> > > +                           "50m", "125m", "150m",
> > > +                           "250m", "270m";
> >
> > How do you access this h/w. There's nothing defined like 'reg' or
> > a parent node or...
>
> Through read write operations defined in
> "asm/mach-ralink/ralink_regs.h. Please, see my explanation below.
>
> >
> > The suggestion on v4 was to get rid of the child node by merging it with
> > the parent like this:
> >
> > +    sysc: sysc@0 {
> > +      compatible = "mediatek,mt7621-sysc", "syscon";
> > +      reg = <0x0 0x100>;
> > +      #clock-cells = <1>;
> > +      clock-output-names = "xtal", "cpu", "bus",
> > +                             "50m", "125m", "150m",
> > +                             "250m", "270m";
> > +    };
> >
> > Whether you need child nodes or not really depends on what all is in the
> > 'mt7621-sysc' h/w block.
>
> All the drivers in this platform make use of arch operations defined
> in "asm/mach-ralink/ralink_regs.h". This mediatek,mt7621-sysc is
> directly mapped by the architecture
> in arch/mips/ralink/mt7621.c in function void __init
> ralink_of_remap(void). This is the first address in the virtual space
> and from here "rt_sysc_membase" and "rt_memc_membase" are used to
> access the hardware control registers through read and write
> operations. So "mediatek,mt7621-sysc" cannot be remapped from clock
> driver. The benefits I found at first of using the syscon as child
> node was to avoid the use of architecture dependant operations but at
> the end I realized that we need to access another register which is
> not in the syscon block and it is not also well documented so the use
> of arch operations is mandatory to make things work. That's why I end
> up in just follow the architecture driver style and use this in the
> same way, trying to maintain as clean as possible. Is it ok then to
> declare it as it is in this way?

Just to add a bit of more of information. So in the device tree we
currently have two nodes (drivers/staging/mt7621-dts/mt7621.dtsi):

sysc: sysc@0 {
    compatible = "mediatek,mt7621-sysc";
    reg = <0x0 0x100>;
};

memc: memc@5000 {
    compatible = "mediatek,mt7621-memc";
    reg = <0x5000 0x1000>;
};

from here "rt_sysc_membase" and "rt_memc_membase" are set and mapped
in arch/mips/ralink/mt7621.c in function ralink_of_remap:

rt_sysc_membase = plat_of_remap_node("mediatek,mt7621-sysc");
rt_memc_membase = plat_of_remap_node("mediatek,mt7621-memc");

'plat_of_remap_node' is a function common to all ramips architectures
that is implemented in arch/mips/ralink/of.c. There these nodes are
got from DT, a memory region is requested and at the end 'ioremap' is
called using the obtained resource range.

In clock driver, to make things work, we need to access six registers:
five in the sysc address space and one in the memc space. To do this
we can directly use (as I did in current v5 we are discussing here)
operations defined in "asm/mach-ralink/ralink_regs.h" that is the way
other drivers do. I don't know if there is another approach more
device-tree friendly. At the end I also pretend to get out from
staging all the stuff related with this platform and that also
includes its device tree, so I'd like to follow a correct approach
also for this clock node and driver which IMHO is something that we
really need.

Rob, can you please guide me in the correct approach to follow? Maybe
mark these two nodes as "syscon" and from driver code obtain a phandle
to them and use regmap in both of them to avoid the use of
asm/mach-ralink/ralink_regs.h operations? I think doing in this way
this include won't be needed in driver making things a bit "generic"
but I have never seen two syscon handlers in a DT node so I don't know
if this way is worse or better... What seems to be clear is that since
clock driver need to access both of address space from these two
nodes, child nodes seems to be a no way at all. Something like (not
tested at all):

#include <dt-bindings/clock/mt7621-clk.h>

sysc: sysc@0 {
    compatible = "mediatek,mt7621-sysc", "syscon";
    reg = <0x0 0x100>;
};

memc: memc@5000 {
    compatible = "mediatek,mt7621-memc", "syscon";
    reg = <0x5000 0x1000>;
};

pll {
    compatible = "mediatek,mt7621-clk";
    ralink,sysctl = <&sysc>;
    ralink,memctl = <&memc>;
    #clock-cells = <1>;
    clock-output-names = "xtal", "cpu", "bus",
                                        "50m", "125m", "150m",
                                         "250m", "270m";
};

Thanks in advance for your time and help.

Best regards,
    Sergio Paracuellos