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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMhs-H879dSo3CouwA7OPo-W=BO3-mMSx225+q2G1wR3E-DHdA@mail.gmail.com>
Date:   Fri, 1 Jan 2021 00:51:42 +0100
From:   Sergio Paracuellos <sergio.paracuellos@...il.com>
To:     Rob Herring <robh@...nel.org>
Cc:     Stephen Boyd <sboyd@...nel.org>, John Crispin <john@...ozen.org>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Greg KH <gregkh@...uxfoundation.org>,
        Chuanhong Guo <gch981213@...il.com>,
        Weijie Gao <hackpascal@...il.com>,
        "open list:COMMON CLK FRAMEWORK" <linux-clk@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "open list:MIPS" <linux-mips@...r.kernel.org>,
        "open list:STAGING SUBSYSTEM" <devel@...verdev.osuosl.org>,
        NeilBrown <neil@...wn.name>
Subject: Re: [PATCH v5 2/6] dt: bindings: add mt7621-clk device tree binding documentation

Hi Rob,

Thanks for the review.

On Thu, Dec 31, 2020 at 11:38 PM Rob Herring <robh@...nel.org> 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@...il.com>
> > ---
> >  .../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@...il.com>
> > +
> > +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?

>
> Rob

Best regards,
    Sergio Paracuellos