Re: [PATCH net v4] net: dsa: mt7530: fix impossible MDIO address and issue warning

[Arınç ÜNAL]

On 04/07/2024 20:48, Daniel Golle wrote:
> Hi Vladimir,
>
> On Thu, Jul 04, 2024 at 08:16:04PM +0300, Vladimir Oltean wrote:
> > On Thu, Jul 04, 2024 at 04:08:22PM +0100, Daniel Golle wrote:
> > > The MDIO address of the MT7530 and MT7531 switch ICs can be configured
> > > using bootstrap pins. However, there are only 4 possible options for the
> > > switch itself: 7, 15, 23 and 31. As in MediaTek's SDK the address of the
> > > switch is wrongly stated in the device tree as 0 (while in reality it is
> > > 31), warn the user about such broken device tree and make a good guess
> > > what was actually intended.
> >
> > Zero is the MDIO broadcast address. Doesn't the switch respond to it, or
> > what's exactly the problem?
>
> No, MT7530 main device (ie. the switch itself, not the built-in PHYs
> which on MT7530 can also be exposed on the same bus) only responds to
> address 31 (default), 7, 15 or 23 (the latter 3 via non-default
> bootstrap configuration).
>
> MT7531 always uses address 31 by default and also doesn't respond on
> address 0.
>
> See also https://lkml.org/lkml/2024/5/31/236

To address my incorrect "0x0 is just another PHY address" remark there; in
22.2.4.5.5 of IEEE Std 802.3-2022, it is described that a PHY that is
connected to the station management entity via the mechanical interface
defined in 22.6 shall always respond to transactions addressed to PHY
Address zero <00000>. A station management entity that is attached to
multiple PHYs has to have prior knowledge of the appropriate PHY Address
for each PHY.

The MT7530 switch has the function to make its PHYs appear on the MDIO bus
which the switch also listens on. This feature is controlled by the
relevant bootstrap pin or by modifying the relevant bit on the modifiable
trap register. The MT7530 DSA subdriver currently configures the modifiable
trap register to enable this function. So one of the switch PHYs listens at
the PHY address 0x0. I don't know whether the switch would respond to
transactions addressed to the PHY address 0x0, if this function was
disabled. Finding this out doesn't seem too relevant to this topic.

> > > as with commit 868ff5f4944a ("net: dsa: mt7530-mdio: read PHY address of
> > > switch from device tree") the address in device tree will be taken into
> > > account, while before it was hard-coded to 0x1f.
> > >
> > > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> >
> > I fail to understand the logic behind blaming this commit. There was no
> > observable issue prior to 868ff5f4944a ("net: dsa: mt7530-mdio: read PHY
> > address of switch from device tree"), was there?
>
> Please see the lengthy debate here:
>
> https://lore.kernel.org/linux-arm-kernel/af561268-9793-4b5d-aa0f-d09698fd6fb0@xxxxxxxxxx/T/#mc967f795a062f6aaedea7375a3be104266e88cc4

This thread may not directly answer the question. The understanding Daniel
and I have come to is that the fact that the issue appeared after commit
868ff5f4944a ("net: dsa: mt7530-mdio: read PHY address of switch from
device tree") doesn't make it the commit to blame. But rather, the commit
that introduced a hardcoded 0x1f PHY address which, in result, allowed
broken device trees to work is to blame.

> I should have provided a reference to that in the commit message or
> cover letter.

I agree, it would be a good idea to put it in the commit message.

Arınç