Hi Krzysztof, On Mon, Mar 20, 2023 at 5:36 PM Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote: > > On 20/03/2023 17:18, Sergio Paracuellos wrote: > > Adds device tree binding documentation for clocks and resets in the > > Mediatek MIPS and Ralink SOCs. This covers RT2880, RT3050, RT3052, RT3350, > > RT3883, RT5350, MT7620, MT7628 and MT7688 SoCs. > > Use subject prefixes matching the subsystem (which you can get for > example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory > your patch is touching). > > Subject: drop second/last, redundant "device tree binding > documentation". The "dt-bindings" prefix is already stating that these > are bindings. Sure, will do. Sorry for the inconvenience. > (BTW, that's the longest redundant component I ever saw) I thought it was better to just list compatible strings inside one single file than adding the same binding in multiple files. > > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx> > > --- > > .../bindings/clock/mtmips-clock.yaml | 68 +++++++++++++++++++ > > 1 file changed, 68 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/clock/mtmips-clock.yaml > > > > diff --git a/Documentation/devicetree/bindings/clock/mtmips-clock.yaml b/Documentation/devicetree/bindings/clock/mtmips-clock.yaml > > new file mode 100644 > > index 000000000000..c92969ce231d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/clock/mtmips-clock.yaml > > Filename matching compatible, so vendor prefix and device name (or > family of names). I used mtmips here but list compatibles starting with ralink. As I have said in the cover letter I am inspired by the last merged pinctrl series for these SoCs. See: - https://lore.kernel.org/linux-gpio/e9e6ad87-2db5-9767-ff39-64a302b06185@xxxxxxxxxx/T/#t Not all of compatible currently exist. All of these are at the end the way we can properly match compatible-data to write a proper driver. The current ralink dtsi files which are in tree now are totally incomplete and not documented so we are planning to align all of this with openWRT used files and others soon. That's the reason we are not touching 'arch/mips/boot/dts' at all now. I don't think anybody is using any of this but mt7621 which is properly completed and documented. > > > @@ -0,0 +1,68 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/clock/mtmips-clock.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: MTMIPS SoCs Clock > > One clock? Are you sure these describe exactly one clock? I will change this to 'Clocks'. > > > + > > +maintainers: > > + - Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx> > > + > > +description: | > > + MediaTek MIPS and Ralink SoCs have an XTAL from where the cpu clock is > > + provided as well as derived clocks for the bus and the peripherals. > > + > > + Each clock is assigned an identifier and client nodes use this identifier > > + to specify the clock which they consume. > > Drop useless or obvious pieces of description. Describe the hardware. > > > + > > + The clocks are provided inside a system controller node. > > ??? I meant, this node is a syscon from where both clock and reset related registers are used. I think writing in this way was enough since it has a pretty similar description like the one in 'mediatek,mt7621-sysc.yaml'. > > > + > > + This node is also a reset provider for all the peripherals. > > ??? Does it mean it is not only "Clock" but also reset controller? Yes, this node is a clock and reset controller for all the SoC. > > > + > > +properties: > > + compatible: > > + items: > > + - enum: > > + - ralink,rt2880-sysc > > + - ralink,rt3050-sysc > > + - ralink,rt3052-sysc > > + - ralink,rt3352-sysc > > + - ralink,rt3883-sysc > > + - ralink,rt5350-sysc > > + - ralink,mt7620-sysc > > + - ralink,mt7620a-sysc > > + - ralink,mt7628-sysc' > > + - ralink,mt7688-sysc > > + - ralink,rt2880-reset > > That's odd. rt2880 is sysc and reset? One device with two compatibles? This 'ralink,rt2880-reset' is for compatibility reasons. Reset related code was inside 'arch/mips/ralink' folder reset.c file but it is moved to this new driver, so we have maintained this reset stuff for the reset compatibility. All of the rest are the new possible stuff for both reset and clocks. Clock driver is instantiated in two phases. The earlier one set up the clocks via CLK_OF_DECLARE macro. Resets are set up as a platform driver. Is only inside this where 'ralink,rt2880-reset' is used. See patch 2 of the series for details. > > Also, order these by name. All are ordered but I maintained the 'ralink,rt2880-reset' at the end. > > > > + - const: syscon > > + > > + reg: > > + maxItems: 1 > > + > > + '#clock-cells': > > + description: > > + The first cell indicates the clock number. > > + const: 1 > > + > > + '#reset-cells': > > + description: > > + The first cell indicates the reset bit within the register. > > + const: 1 > > Wait, only rt2880-reset is reset controller? This is confusing. No, that is the reset compatibility one. All the rest are both clock and reset controllers from now on. > > > + > > +required: > > + - compatible > > + - reg > > + - '#clock-cells' > > + - '#reset-cells' > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + sysc: sysc@0 { > > Drop label. Sure, thanks. > > Node names should be generic, clock-controller or reset-controller or > system-controller sometimes. > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > > > + compatible = "ralink,rt5350-sysc", "syscon"; > > + reg = <0x0 0x100>; > > + #clock-cells = <1>; > > + #reset-cells = <1>; > > + }; Ok, so I will set this as 'syscon@' if you are ok with it. > > Best regards, > Krzysztof > Thanks to you for the review. Best regards, Sergio Paracuellos
