On Sat, Nov 21, 2020 at 3:50 PM Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx> wrote: > > Hi Rob, > > Thanks for the review. > > On Sat, Nov 21, 2020 at 2:34 PM Rob Herring <robh@xxxxxxxxxx> wrote: > > > > On Fri, Nov 13, 2020 at 04:46:29PM +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 | 61 +++++++++++++++++++ > > > 1 file changed, 61 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..363bd9880e29 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml > > > @@ -0,0 +1,61 @@ > > > +# 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 > > > + > > > + ralink,sysctl: > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > + description: > > > + phandle to the syscon which is in the same address area with syscon > > > + device. > > > + > > > + "#clock-cells": > > > + description: > > > + The first cell indicates the clock gate number, see [1] for available > > > + clocks. > > > + const: 1 > > > + > > > + clock-output-names: > > > + maxItems: 8 > > > + > > > +required: > > > + - compatible > > > + - ralink,sysctl > > > + - '#clock-cells' > > > + - clock-output-names > > > + > > > +additionalProperties: false > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/clock/mt7621-clk.h> > > > + > > > + pll { > > > + compatible = "mediatek,mt7621-clk"; > > > + ralink,sysctl = <&sysc>; > > > > If this is the only control interface, then make this a child of 'sysc'. > > And use 'reg' if there's a dedicated range of registers. > > This is the only one now in the device tree which is still in staging > but there are several places where this sys control registers are > accessed from. In the case of the clocks we need: > > #define SYSC_REG_SYSTEM_CONFIG0 0x10 > #define SYSC_REG_SYSTEM_CONFIG1 0x14 > #define SYSC_REG_CLKCFG0 0x2c > #define SYSC_REG_CLKCFG1 0x30 > #define SYSC_REG_CUR_CLK_STS 0x44 > > where there is not a range as it is but several different registers > from where we need to read or write things. I wrote the driver using > syscon and regmap because I thought in that way it might be more > maintainable but this architecture also has operations to read and > write registers from sysc and not using regmap at all. This operations > are defined in arch/mips/include/asm/mach-ralink/ralink_regs.h. But > because this sysc is currently mapped I cannot request its registers > using reg in the device tree. If you prefer me to avoid the use of > this syscon and regmap and use operations defined in ralink_regs.h, > this will become in a node without "regs" or "ralink,sysctl" > property: > > pll { > compatible = "mediatek,mt7621-clk"; > #clock-cells = <1>; > clock-output-names = "xtal", "cpu", "bus", > "50m", "125m", "150m", > "250m", "270m"; > }; > > What should I do then? Ok, I think I understand now what you are saying. You meant just move this as a child if 'sysc' and avoid the phandle: sysc: sysc@0 { compatible = "mtk,mt7621-sysc", "syscon"; reg = <0x0 0x100>; pll: pll { compatible = "mediatek,mt7621-clk"; #clock-cells = <1>; clock-output-names = "xtal", "cpu", "bus", "50m", "125m", "150m", "250m", "270m"; }; }; And because there is not a range of registers 'reg' property is not needed, right? And in the driver side make use of 'syscon_node_to_regmap(node->parent)' instead of 'syscon_regmap_lookup_by_phandle(node, "ralink,sysctl")'. Ok, thanks for the clarification. Will change when v4 is sent. Best regards, Sergio Paracuellos > > Best regards, > Sergio Paracuellos > > > > > > + #clock-cells = <1>; > > > + clock-output-names = "xtal", "cpu", "bus", > > > + "50m", "125m", "150m", > > > + "250m", "270m"; > > > + }; > > > -- > > > 2.25.1 > > >
