| Message ID | 20240314-for-mediatek-mt7531-phy-address-v1-2-52f58db01acd@arinc9.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Set PHY address of MT7531 switch to 0x1f on MediaTek arm64 boards | expand |
Hi Arınç, On Thu, Mar 14, 2024 at 03:20:05PM +0300, Arınç ÜNAL via B4 Relay wrote: > From: Arınç ÜNAL <arinc.unal@arinc9.com> > > The MT7531 switch listens on PHY address 0x1f on an MDIO bus. I've got two > findings that support this. There's no bootstrapping option to change the > PHY address of the switch. The Linux driver hardcodes 0x1f as the PHY > address of the switch. So the reg property on the device tree is currently > ignored by the Linux driver. > > Therefore, describe the correct PHY address on boards that have this > switch. This is already the case on all MT7986 boards here, so use > hexadecimal numbering and align the switch node name with the reg value. Sorry for the late reply to this series which I had not noticed earlier. My comment below applies for the whole series. While this is generally correct, you are mixing cosmetic with functional changes here. Replacing reg = <31>; with reg = <0x1f>; is a purely cosmetic change (and it's up to maintainers taste to like one or the other notation more). On the other hand replacing switch@0 or switch@31 with switch@1f is fixing a bug. Yet even that doesn't have any functional impact on the affected boards as there aren't any other DT nodes which would collide with that incorrect address, nor does the driver actually care about the node name (not before and not after your patch "net: dsa: mt7530-mdio: read PHY address of switch from device tree"). Anyway, there *is* something wrong and everybody should agree it's a good thing to fix it. So imho you should start with a patch only replacing all instances of mt7530/mt7531 switch nodes named 'switch@0' or 'switch@31' with 'switch@1f' as that's fixing an actual (minor) bug. The other thing, using hexadecimal notation for the 'reg' property is a mere matter of (statistically unusual) taste. I fully get the point that using hexdecimal for both, the address in the node name as well as the 'reg' property avoids the exact divergence of the two you are fixing now. Byt statistically unusual I mean: $ find -name \*.dts\* | while read line; do grep 'reg.*=.*<[0-9]*>' $line ; done | wc -l 37284 $ find -name \*.dts\* | while read line; do grep 'reg.*=.*<0x[0-9a-fA-F]*>' $line ; done | wc -l 10339 (I know the above regexp could be done more accurately, but it's good enough to demonstrate my point) So please make this four patches. Two fixing the wrong node names. And another two opening the (purely cosmetic) debate to use hexademical notation for the 'reg' property. Cheers Daniel > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> > --- > arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts | 4 ++-- > arch/arm64/boot/dts/mediatek/mt7986a-rfb.dts | 4 ++-- > arch/arm64/boot/dts/mediatek/mt7986b-rfb.dts | 4 ++-- > 3 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts > index e04b1c0c0ebb..2f92f8cfd8a3 100644 > --- a/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts > +++ b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts > @@ -200,9 +200,9 @@ mdio: mdio-bus { > }; > > &mdio { > - switch: switch@31 { > + switch: switch@1f { > compatible = "mediatek,mt7531"; > - reg = <31>; > + reg = <0x1f>; > interrupt-controller; > #interrupt-cells = <1>; > interrupts-extended = <&pio 66 IRQ_TYPE_LEVEL_HIGH>; > diff --git a/arch/arm64/boot/dts/mediatek/mt7986a-rfb.dts b/arch/arm64/boot/dts/mediatek/mt7986a-rfb.dts > index 5d8e3d3f6c20..47f75ece1872 100644 > --- a/arch/arm64/boot/dts/mediatek/mt7986a-rfb.dts > +++ b/arch/arm64/boot/dts/mediatek/mt7986a-rfb.dts > @@ -84,9 +84,9 @@ mdio: mdio-bus { > }; > > &mdio { > - switch: switch@0 { > + switch: switch@1f { > compatible = "mediatek,mt7531"; > - reg = <31>; > + reg = <0x1f>; > reset-gpios = <&pio 5 0>; > }; > }; > diff --git a/arch/arm64/boot/dts/mediatek/mt7986b-rfb.dts b/arch/arm64/boot/dts/mediatek/mt7986b-rfb.dts > index 58f77d932429..5148a69f4729 100644 > --- a/arch/arm64/boot/dts/mediatek/mt7986b-rfb.dts > +++ b/arch/arm64/boot/dts/mediatek/mt7986b-rfb.dts > @@ -61,9 +61,9 @@ mdio: mdio-bus { > #address-cells = <1>; > #size-cells = <0>; > > - switch@0 { > + switch@1f { > compatible = "mediatek,mt7531"; > - reg = <31>; > + reg = <0x1f>; > reset-gpios = <&pio 5 0>; > > ports { > > -- > 2.40.1 > >
diff --git a/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts index e04b1c0c0ebb..2f92f8cfd8a3 100644 --- a/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts +++ b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts @@ -200,9 +200,9 @@ mdio: mdio-bus { }; &mdio { - switch: switch@31 { + switch: switch@1f { compatible = "mediatek,mt7531"; - reg = <31>; + reg = <0x1f>; interrupt-controller; #interrupt-cells = <1>; interrupts-extended = <&pio 66 IRQ_TYPE_LEVEL_HIGH>; diff --git a/arch/arm64/boot/dts/mediatek/mt7986a-rfb.dts b/arch/arm64/boot/dts/mediatek/mt7986a-rfb.dts index 5d8e3d3f6c20..47f75ece1872 100644 --- a/arch/arm64/boot/dts/mediatek/mt7986a-rfb.dts +++ b/arch/arm64/boot/dts/mediatek/mt7986a-rfb.dts @@ -84,9 +84,9 @@ mdio: mdio-bus { }; &mdio { - switch: switch@0 { + switch: switch@1f { compatible = "mediatek,mt7531"; - reg = <31>; + reg = <0x1f>; reset-gpios = <&pio 5 0>; }; }; diff --git a/arch/arm64/boot/dts/mediatek/mt7986b-rfb.dts b/arch/arm64/boot/dts/mediatek/mt7986b-rfb.dts index 58f77d932429..5148a69f4729 100644 --- a/arch/arm64/boot/dts/mediatek/mt7986b-rfb.dts +++ b/arch/arm64/boot/dts/mediatek/mt7986b-rfb.dts @@ -61,9 +61,9 @@ mdio: mdio-bus { #address-cells = <1>; #size-cells = <0>; - switch@0 { + switch@1f { compatible = "mediatek,mt7531"; - reg = <31>; + reg = <0x1f>; reset-gpios = <&pio 5 0>; ports {