Re: [PATCH v2 2/2] ARM: dts: r9a06g032: add r9a06g032-rzn1d400-eb board device-tree

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Geert,

I've started working on this issue, however I have one problem :

>> Hi Clément,
>>
>> On Wed, Feb 15, 2023 at 12:31 PM Geert Uytterhoeven
>> <geert@xxxxxxxxxxxxxx> wrote:
>>> On Wed, Feb 15, 2023 at 11:52 AM Clément Léger
>>> <clement.leger@xxxxxxxxxxx> wrote:
>>> > Le Wed, 15 Feb 2023 09:29:33 +0100,
>>> > Clément Léger <clement.leger@xxxxxxxxxxx> a écrit :
>>> > > Le Tue, 14 Feb 2023 17:25:14 +0100,
>>> > > Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> a écrit :
>>> > > > On Thu, Feb 9, 2023 at 2:32 PM Clément Léger <clement.leger@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>>> > > >
>>> > > > 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 ?

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.

Regards,
Thomas





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux