Message-Id: <D7WHQQM7K8NI.3JS09W4941HY1@bootlin.com>
Date: Wed, 19 Feb 2025 15:27:11 +0100
From: "Thomas Bonnefille" <thomas.bonnefille@...tlin.com>
To: "Geert Uytterhoeven" <geert@...ux-m68k.org>
Cc: "Miquel Raynal" <miquel.raynal@...tlin.com>, "Romain Gantois"
 <romain.gantois@...tlin.com>, "Magnus Damm" <magnus.damm@...il.com>, "Rob
 Herring" <robh+dt@...nel.org>, "Krzysztof Kozlowski"
 <krzysztof.kozlowski+dt@...aro.org>, "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>, "Herve Codina" <herve.codina@...tlin.com>,
 "Milan Stevanovic" <milan.stevanovic@...com>, "Jimmy Lalande"
 <jimmy.lalande@...com>, "Pascal Eberhard" <pascal.eberhard@...com>,
 <linux-renesas-soc@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, "Gareth Williams"
 <gareth.williams.jx@...esas.com>, "Wolfram Sang"
 <wsa+renesas@...g-engineering.com>
Subject: Re: [PATCH v2 2/2] ARM: dts: r9a06g032: add r9a06g032-rzn1d400-eb
 board device-tree

Hello Geert,

> On Fri, 14 Feb 2025 at 14:20, Thomas Bonnefille
> <thomas.bonnefille@...tlin.com> wrote:
>> >> On Wed, Feb 15, 2023 at 12:31 PM Geert Uytterhoeven
>> >> <geert@...ux-m68k.org> wrote:
>> >>> On Wed, Feb 15, 2023 at 11:52 AM Clément Léger
>> >>> <clement.leger@...tlin.com> wrote:
>> >>> > Le Wed, 15 Feb 2023 09:29:33 +0100,
>> >>> > Clément Léger <clement.leger@...tlin.com> a écrit :
>> >>> > > Le Tue, 14 Feb 2023 17:25:14 +0100,
>> >>> > > Geert Uytterhoeven <geert@...ux-m68k.org> a écrit :
>> >>> > > > On Thu, Feb 9, 2023 at 2:32 PM Clément Léger <clement.leger@...tlin.com> wrote:
>> >>> > > > > The EB board (Expansion board) supports both RZ/N1D and RZ-N1S. Since this
>> >>> > > > > configuration targets only the RZ/N1D, it is named r9a06g032-rzn1d400-eb.
>> >>> > > > > It adds support for the 2 additional switch ports (port C and D) that are
>> >>> > > > > available on that board.
>> >>> > > > >
>> >>> > > > > Signed-off-by: Clément Léger <clement.leger@...tlin.com>
>> >>> > > >
>> >>> > > > Thanks for your patch!
>> >>> > > >
>> >>> > > > > --- /dev/null
>> >>> > > > > +++ b/arch/arm/boot/dts/r9a06g032-rzn1d400-eb.dts
>> >>>
>> >>> > > > > +       pinctrl-0 = <&pins_eth1>, <&pins_eth2>, <&pins_eth3>, <&pins_eth4>,
>> >>> > > > > +                   <&pins_mdio1>;
>> >>> > > > > +
>> >>> > > > > +       mdio {
>> >>> > > > > +               /* CN15 and CN16 switches must be configured in MDIO2 mode */
>> >>> > > > > +               switch0phy1: ethernet-phy@1 {
>> >>> > > > > +                       reg = <1>;
>> >>> > > > > +                       marvell,reg-init = <3 16 0 0x1010>;
>> >>> > > >
>> >>> > > > marvell,reg-init is not documented in any DT bindings document?
>> >>> > >
>> >>> > > Indeed, this is not somethiong that should be made available here. It's
>> >>> > > only inverting the LED polarity but supported by some internal patch.
>> >>> > > I'll remove that.
>> >>>
>> >>> > I actually was confused by a property I added in another device-tree but
>> >>> > marvell,reg-init exists, is handled by the marvell phy driver and used
>> >>> > in a few device-trees. Strangely, it is not documented anywhere. So I
>> >>> > can either remove that (and the LED won't work properly) or let it live
>> >>> > depending on what you prefer.
>> >>>
>> >>> In that case, please keep it.
>> >>> But the property really should be documented, one day...
>>
>> As Clément mentioned, this property is used to set up the LEDs for
>> Marvell PHY. However, Marvell's PHYs have no dedicated bindings; only
>> their associated switches do. PHY's usually don't have their own yaml,
>> so there is no easy place where to add this property. We could however
>> describe them in the numerous switch bindings that embed a Marvell PHY,
>> which are: Qualcomm ETHQOS, Cadence MACB/GEM, Gianfar, Freescale FEC,
>> Renesas switches and of course Marvell switches.
>>
>> I already thought of doing it in the binding of the renesas switch, like
>> this :
>>
>> Documentation/devicetree/bindings/net/dsa/renesas,rzn1-a5psw.yaml:
>> ```
>>
>> \[...\]
>>
>> mdio:
>> $ref: /schemas/net/mdio.yaml#
>> patternProperties:
>> '@\[0-9a-f\]+$':
>> properties:
>> marvel,reg-init:
>> - description: Lorem Ipsum
>>
>> unevaluatedProperties: false
>>
>> \[...\]
>>
>> ```
>> but it would document it only for this particular switch.
>> It is also possible to do it in the main mdio.yaml on the model of this:
>> https://elixir.bootlin.com/linux/v6.13.1/source/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml#L121
>>
>> What's your opinion on this ?
>
> Oh, so this is a similar issue as the one preventing us from converting
> the Micrel PHY bindings to dt-schema[1]?
>
> You could still document it in a text binding file:
> Documentation/devicetree/bindings/net/marvell,phy.txt
> That cannot be used for validation, but at least people can find the
> property using git grep...
>

Ack, I'll do that.

However, after the third version this series will no longer be related
to the marvell,reg-init property. Therefore, I might document it in a
different series.

>> Moreover, everywhere this property is used in the kernel, it is to set
>> up the LEDs. Nowadays, the Marvell PHY driver should be able to handle
>> LEDs without this property. Therefore, this property should be
>> deprecated in this case.
>
> So the LED works now on this board without the property?
> Then the property can be dropped?

Yes it does, I "just" have to add an LED as a child node of the phy in
the device-tree.
I'll send a third version using this mechanism soon.
However, I will not be able to test it on the real evaluation board but
on a proprietary board using the same SoC and PHY. It should not cause
any problem as I will reproduce exactly what the marvell,reg-init
property was doing.

Regards,
Thomas

Powered by blists - more mailing lists