On Mon, Mar 20, 2023 at 6:36 PM Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote: > > On 20/03/2023 18:24, Sergio Paracuellos wrote: > > 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. > > I don't understand how this is answers about redundant piece of subject. > Amount of files are not related to repeating pieces of subject prefix. True :). > > > > >> > >>> > >>> 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 > > 21 patches, so what exactly I should see (except that I was involved in > that discussions)? > > Plus nothing from this thread warrants here exception from naming style. > > > > > > Not all of compatible currently exist. > > Then clearly state this. Sure. Will do in the commit message. > > > 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 > > Nothing like this was said in commit msg, so how can we know? Will add a comment about this also. > > > 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. > > Anyway, none of this explains exception from naming convention - vendor, > device or family name. > > > > >> > >>> @@ -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'. > > Then clock provider, but are you sure? You included there syscon and > reset controller. You are right. This is a system controller node even though it provides clocks and resets for the rest of the world. > > > > >> > >>> + > >>> +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'. > > But what is a system controller node? Some separate device? This is > description for this device - called "Clock" or "Clocks" - and "system > controller" appears for the first time. > > > > >> > >>> + > >>> + 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. > > I don't understand why. It is used in DTS, so what this node represents > there? True, this only has to be present in driver code. Not related to bindings at all. Will drop it, then. > > > 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. > > We talk here about hardware, not drivers, so moving driver code around > does not help me understand the rationale behind bindings. Understood. > > > 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. > > Sure, but it is not related to bindings. > > > > >> > >> Also, order these by name. > > > > All are ordered but I maintained the 'ralink,rt2880-reset' at the end. > > No, it is not. m is before r in alphabet. In my mind all of these were ordered taking into account the year since they exist :). So you are right. I will order this alphabetically. > > Best regards, > Krzysztof > Thanks, Sergio Paracuellos
