Hi Rob, Thanks for the review. 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? > > Rob Best regards, Sergio Paracuellos
