From: Frank Wunderlich <[email protected]>
After commit 868ff5f4944a
("net: dsa: mt7530-mdio: read PHY address of switch from device tree")
the mt7531 switch on Bananapi-R64 was not detected.
mt7530-mdio mdio-bus:00: reset timeout
mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110
Fix this by adding phy address in devicetree.
Signed-off-by: Frank Wunderlich <[email protected]>
---
arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts b/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts
index 224bb289660c..811b227d6f50 100644
--- a/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts
+++ b/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts
@@ -149,9 +149,9 @@ mdio: mdio-bus {
#address-cells = <1>;
#size-cells = <0>;
- switch@0 {
+ switch@1f {
compatible = "mediatek,mt7531";
- reg = <0>;
+ reg = <0x1f>;
interrupt-controller;
#interrupt-cells = <1>;
interrupts-extended = <&pio 53 IRQ_TYPE_LEVEL_HIGH>;
--
2.34.1
On 16/05/2024 23:48, Frank Wunderlich wrote:
> From: Frank Wunderlich <[email protected]>
>
> After commit 868ff5f4944a
> ("net: dsa: mt7530-mdio: read PHY address of switch from device tree")
> the mt7531 switch on Bananapi-R64 was not detected.
>
> mt7530-mdio mdio-bus:00: reset timeout
> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110
>
> Fix this by adding phy address in devicetree.
>
> Signed-off-by: Frank Wunderlich <[email protected]>
I don't like the mention of the Linux kernel driver on the patch log. What
you're fixing is the incorrect description of the switch's PHY address on
the DTS file. Whether or not any driver from any project is actually
reading it from the DTS file is irrelevant to this patch. That said, I
already have a patch series I've been meaning to send the next version of
that already addresses this. Please wait for that.
Arınç
Am 17. Mai 2024 04:17:47 MESZ schrieb "Arınç ÜNAL" <[email protected]>:
>On 16/05/2024 23:48, Frank Wunderlich wrote:
>> From: Frank Wunderlich <[email protected]>
>>
>> After commit 868ff5f4944a
>> ("net: dsa: mt7530-mdio: read PHY address of switch from device tree")
>> the mt7531 switch on Bananapi-R64 was not detected.
>>
>> mt7530-mdio mdio-bus:00: reset timeout
>> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110
>>
>> Fix this by adding phy address in devicetree.
>>
>> Signed-off-by: Frank Wunderlich <[email protected]>
>
>I don't like the mention of the Linux kernel driver on the patch log. What
>you're fixing is the incorrect description of the switch's PHY address on
>the DTS file. Whether or not any driver from any project is actually
>reading it from the DTS file is irrelevant to this patch. That said, I
>already have a patch series I've been meaning to send the next version of
>that already addresses this. Please wait for that.
>
>Arınç
Hi arinc,
From my PoV it is a regression in next/6.10 because the driver change was merged (without "broadcast" fallback) and the dts patch [1] is not.
I agree that my patch can be dropped because there was already one.
regards Frank
[1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240314-for-mediatek-mt7531-phy-address-v1-1-52f58db01acd@arinc9.com/
On Thu, 16 May 2024 22:48:47 +0200, Frank Wunderlich wrote:
> From: Frank Wunderlich <[email protected]>
>
> After commit 868ff5f4944a
> ("net: dsa: mt7530-mdio: read PHY address of switch from device tree")
> the mt7531 switch on Bananapi-R64 was not detected.
>
> mt7530-mdio mdio-bus:00: reset timeout
> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110
>
> Fix this by adding phy address in devicetree.
>
> Signed-off-by: Frank Wunderlich <[email protected]>
> ---
> arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
My bot found new DTB warnings on the .dts files added or changed in this
series.
Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.
If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:
pip3 install dtschema --upgrade
New warnings running 'make CHECK_DTBS=y mediatek/mt7622-bananapi-bpi-r64.dtb' for [email protected]:
arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dtb: switch@1f: 'interrupts' is a dependency of 'interrupt-controller'
from schema $id: http://devicetree.org/schemas/net/dsa/mediatek,mt7530.yaml#
[CCing the regression list, as it should be in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]
On 17.05.24 08:27, Frank Wunderlich wrote:
> Am 17. Mai 2024 04:17:47 MESZ schrieb "Arınç ÜNAL" <[email protected]>:
>> On 16/05/2024 23:48, Frank Wunderlich wrote:
>>> From: Frank Wunderlich <[email protected]>
>>>
>>> After commit 868ff5f4944a
>>> ("net: dsa: mt7530-mdio: read PHY address of switch from device tree")
>>> the mt7531 switch on Bananapi-R64 was not detected.
>>>
>>> mt7530-mdio mdio-bus:00: reset timeout
>>> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110
>>>
>>> Fix this by adding phy address in devicetree.
>>>
>>> Signed-off-by: Frank Wunderlich <[email protected]>
>>
>> I don't like the mention of the Linux kernel driver on the patch log. What
>> you're fixing is the incorrect description of the switch's PHY address on
>> the DTS file. Whether or not any driver from any project is actually
>> reading it from the DTS file is irrelevant to this patch. That said, I
>> already have a patch series I've been meaning to send the next version of
>> that already addresses this. Please wait for that.
Did you sent this? Because from what I see with my limited experience in
this subsystem...
> From my PoV it is a regression in next/6.10
..I have to agree with Frank here. So it would be good to get this
fixed before -rc1 is out to prevent more people from running into this.
> because the driver change was merged (without "broadcast" fallback) and the dts patch [1] is not.
>
> I agree that my patch can be dropped because there was already one.
>
> regards Frank
>
> [1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240314-for-mediatek-mt7531-phy-address-v1-1-52f58db01acd@arinc9.com/
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
[adding Paolo, who committed the culprit]
On 23.05.24 12:44, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 17.05.24 08:27, Frank Wunderlich wrote:
>> Am 17. Mai 2024 04:17:47 MESZ schrieb "Arınç ÜNAL" <[email protected]>:
>>> On 16/05/2024 23:48, Frank Wunderlich wrote:
>>>> From: Frank Wunderlich <[email protected]>
>>>>
>>>> After commit 868ff5f4944a
>>>> ("net: dsa: mt7530-mdio: read PHY address of switch from device tree")
>>>> the mt7531 switch on Bananapi-R64 was not detected.
>>>>
>>>> mt7530-mdio mdio-bus:00: reset timeout
>>>> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110
>>>>
>>>> Fix this by adding phy address in devicetree.
>>>>
Frank, am I right assuming the regression is still present in mainline?
As from here it looks like for two weeks now there was no progress at
all to fix this (or I missed it, which is quite possible).
Makes me wonder if the maintainers should revert the culprit or if the
arm64 dts folks should accept your fix despite Arınç ÜNAL's (who from a
quick look on lore hasn't posted anything for two weeks now) comment.
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
#regzbot poke
>>> I don't like the mention of the Linux kernel driver on the patch log. What
>>> you're fixing is the incorrect description of the switch's PHY address on
>>> the DTS file. Whether or not any driver from any project is actually
>>> reading it from the DTS file is irrelevant to this patch. That said, I
>>> already have a patch series I've been meaning to send the next version of
>>> that already addresses this. Please wait for that.
>
> Did you sent this? Because from what I see with my limited experience in
> this subsystem...
>
>> From my PoV it is a regression in next/6.10
>
> ...I have to agree with Frank here. So it would be good to get this
> fixed before -rc1 is out to prevent more people from running into this.
>
>> because the driver change was merged (without "broadcast" fallback) and the dts patch [1] is not.
>>
>> I agree that my patch can be dropped because there was already one.
>>
>> regards Frank
>>
>> [1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240314-for-mediatek-mt7531-phy-address-v1-1-52f58db01acd@arinc9.com/
>
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
>
On 31/05/2024 08.40, Thorsten Leemhuis wrote:
> [adding Paolo, who committed the culprit]
>
> On 23.05.24 12:44, Linux regression tracking (Thorsten Leemhuis) wrote:
>> On 17.05.24 08:27, Frank Wunderlich wrote:
>>> Am 17. Mai 2024 04:17:47 MESZ schrieb "Arınç ÜNAL" <[email protected]>:
>>>> On 16/05/2024 23:48, Frank Wunderlich wrote:
>>>>> From: Frank Wunderlich <[email protected]>
>>>>>
>>>>> After commit 868ff5f4944a
>>>>> ("net: dsa: mt7530-mdio: read PHY address of switch from device tree")
>>>>> the mt7531 switch on Bananapi-R64 was not detected.
>>>>>
>>>>> mt7530-mdio mdio-bus:00: reset timeout
>>>>> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110
>>>>>
>>>>> Fix this by adding phy address in devicetree.
>>>>>
>
> Frank, am I right assuming the regression is still present in mainline?
> As from here it looks like for two weeks now there was no progress at
> all to fix this (or I missed it, which is quite possible).
>
> Makes me wonder if the maintainers should revert the culprit or if the
> arm64 dts folks should accept your fix despite Arınç ÜNAL's (who from a
> quick look on lore hasn't posted anything for two weeks now) comment.
I'm not against the patch. I'm against the logic it entails on the patch
log. I had already submitted a patch series that would've prevented this
issue back in 14 March 2024 [1]. I've asked numerous times for the patch
series to be applied [2][3][4][5].
Eventually Daniel asked for some changes [6]. But I won't have time to do
that anytime soon and I think the patch series is good enough to be applied
as is.
[1] https://lore.kernel.org/all/20240314-for-mediatek-mt7531-phy-address-v1-0-52f58db01acd@arinc9.com/
[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/
[4] https://lore.kernel.org/all/[email protected]/
[5] https://lore.kernel.org/all/[email protected]/
[6] https://lore.kernel.org/all/[email protected]/
Arınç
On 17/05/2024 09.27, Frank Wunderlich wrote:
> Am 17. Mai 2024 04:17:47 MESZ schrieb "Arınç ÜNAL" <[email protected]>:
>> On 16/05/2024 23:48, Frank Wunderlich wrote:
>>> From: Frank Wunderlich <[email protected]>
>>>
>>> After commit 868ff5f4944a
>>> ("net: dsa: mt7530-mdio: read PHY address of switch from device tree")
>>> the mt7531 switch on Bananapi-R64 was not detected.
>>>
>>> mt7530-mdio mdio-bus:00: reset timeout
>>> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110
>>>
>>> Fix this by adding phy address in devicetree.
>>>
>>> Signed-off-by: Frank Wunderlich <[email protected]>
>>
>> I don't like the mention of the Linux kernel driver on the patch log. What
>> you're fixing is the incorrect description of the switch's PHY address on
>> the DTS file. Whether or not any driver from any project is actually
>> reading it from the DTS file is irrelevant to this patch. That said, I
>> already have a patch series I've been meaning to send the next version of
>> that already addresses this. Please wait for that.
>>
>> Arınç
>
> Hi arinc,
>
> From my PoV it is a regression in next/6.10 because the driver change was merged (without "broadcast" fallback) and the dts patch [1] is not.
What is a broadcast fallback? 0x1f is just another PHY address.
Arınç
Am 31. Mai 2024 08:12:06 MESZ schrieb "Arınç ÜNAL" <[email protected]>:
>On 17/05/2024 09.27, Frank Wunderlich wrote:
>> Am 17. Mai 2024 04:17:47 MESZ schrieb "Arınç ÜNAL" <[email protected]>:
>>> On 16/05/2024 23:48, Frank Wunderlich wrote:
>>>> From: Frank Wunderlich <[email protected]>
>>>>
>>>> After commit 868ff5f4944a
>>>> ("net: dsa: mt7530-mdio: read PHY address of switch from device tree")
>>>> the mt7531 switch on Bananapi-R64 was not detected.
>>>>
>>>> mt7530-mdio mdio-bus:00: reset timeout
>>>> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110
>>>>
>>>> Fix this by adding phy address in devicetree.
>>>>
>>>> Signed-off-by: Frank Wunderlich <[email protected]>
>>>
>>> I don't like the mention of the Linux kernel driver on the patch log. What
>>> you're fixing is the incorrect description of the switch's PHY address on
>>> the DTS file. Whether or not any driver from any project is actually
>>> reading it from the DTS file is irrelevant to this patch. That said, I
>>> already have a patch series I've been meaning to send the next version of
>>> that already addresses this. Please wait for that.
>>>
>>> Arınç
>>
>> Hi arinc,
>>
>> From my PoV it is a regression in next/6.10 because the driver change was merged (without "broadcast" fallback) and the dts patch [1] is not.
>
>What is a broadcast fallback? 0x1f is just another PHY address.
Afaik 0x0 is some kind of broadcast address if real phy address is not known. The driver change seems not allow this 0x0 adress and forces devicetree to have the real address.
Thats what i mean with broadcast fallback. Maybe the naming is wrong.
>Arınç
@thorsten i have not tested again,but i have not seen any further fix for it.
regards Frank
On 31/05/2024 09.18, Frank Wunderlich wrote:
> Am 31. Mai 2024 08:12:06 MESZ schrieb "Arınç ÜNAL" <[email protected]>:
>> On 17/05/2024 09.27, Frank Wunderlich wrote:
>>> Am 17. Mai 2024 04:17:47 MESZ schrieb "Arınç ÜNAL" <[email protected]>:
>>>> On 16/05/2024 23:48, Frank Wunderlich wrote:
>>>>> From: Frank Wunderlich <[email protected]>
>>>>>
>>>>> After commit 868ff5f4944a
>>>>> ("net: dsa: mt7530-mdio: read PHY address of switch from device tree")
>>>>> the mt7531 switch on Bananapi-R64 was not detected.
>>>>>
>>>>> mt7530-mdio mdio-bus:00: reset timeout
>>>>> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with error -110
>>>>>
>>>>> Fix this by adding phy address in devicetree.
>>>>>
>>>>> Signed-off-by: Frank Wunderlich <[email protected]>
>>>>
>>>> I don't like the mention of the Linux kernel driver on the patch log. What
>>>> you're fixing is the incorrect description of the switch's PHY address on
>>>> the DTS file. Whether or not any driver from any project is actually
>>>> reading it from the DTS file is irrelevant to this patch. That said, I
>>>> already have a patch series I've been meaning to send the next version of
>>>> that already addresses this. Please wait for that.
>>>>
>>>> Arınç
>>>
>>> Hi arinc,
>>>
>>> From my PoV it is a regression in next/6.10 because the driver change was merged (without "broadcast" fallback) and the dts patch [1] is not.
>>
>> What is a broadcast fallback? 0x1f is just another PHY address.
>
> Afaik 0x0 is some kind of broadcast address if real phy address is not known. The driver change seems not allow this 0x0 adress and forces devicetree to have the real address.
That's not true. 0x0 is just another PHY address. The driver change
actually allows reading 0x0 from the device tree and using that PHY address
to control the switch registers, instead of using 0x1f which was hardcoded
on the driver. But on the hardware, the switch actually listens on 0x1f.
Arınç
On 31.05.24 08:10, Arınç ÜNAL wrote:
> On 31/05/2024 08.40, Thorsten Leemhuis wrote:
>> [adding Paolo, who committed the culprit]
/me slowly wonders if the culprit should be reverted for now (see below)
and should be reapplied later together with the matching changes from
Arınç ÜNAL.
>> On 23.05.24 12:44, Linux regression tracking (Thorsten Leemhuis) wrote:
>>> On 17.05.24 08:27, Frank Wunderlich wrote:
>>>> Am 17. Mai 2024 04:17:47 MESZ schrieb "Arınç ÜNAL"
>>>> <[email protected]>:
>>>>> On 16/05/2024 23:48, Frank Wunderlich wrote:
>>>>>> From: Frank Wunderlich <[email protected]>
>>>>>>
>>>>>> After commit 868ff5f4944a
>>>>>> ("net: dsa: mt7530-mdio: read PHY address of switch from device
>>>>>> tree")
>>>>>> the mt7531 switch on Bananapi-R64 was not detected.
>>>>>>
>>>>>> mt7530-mdio mdio-bus:00: reset timeout
>>>>>> mt7530-mdio mdio-bus:00: probe with driver mt7530-mdio failed with
>>>>>> error -110
>>>>>>
>>>>>> Fix this by adding phy address in devicetree.
>>
>> Frank, am I right assuming the regression is still present in mainline?
>> As from here it looks like for two weeks now there was no progress at
>> all to fix this (or I missed it, which is quite possible).
>>
>> Makes me wonder if the maintainers should revert the culprit or if the
>> arm64 dts folks should accept your fix despite Arınç ÜNAL's (who from a
>> quick look on lore hasn't posted anything for two weeks now) comment.
>
> I'm not against the patch. I'm against the logic it entails on the patch
> log.
In that case: can you maybe help Frank with writing something better or
submit something based on this patch to resolve this and make everyone
happy?
> I had already submitted a patch series that would've prevented this
> issue back in 14 March 2024 [1]. I've asked numerous times for the patch
> series to be applied [2][3][4][5].
>> Eventually Daniel asked for some changes [6]. But I won't have time to do
> that anytime soon and I think the patch series is good enough to be applied
> as is.
Then I guess we need some other way to resolve this in mainline to unfix
Frank's device. The two obvious options are afaics:
* revert the culprit (868ff5f4944aa9 ("net: dsa: mt7530-mdio: read PHY
address of switch from device tree")) and reapply it in a later cycle
* apply Frank's patch (or an improved one) in this thread (and if needed
revert it when some better changes emerge.
Arınç ÜNAL, could you please comment on that and ideally handle the
necessary tasks, as it's your patch that causes the regression.
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
> [1]
> https://lore.kernel.org/all/20240314-for-mediatek-mt7531-phy-address-v1-0-52f58db01acd@arinc9.com/
> [2]
> https://lore.kernel.org/all/[email protected]/
> [3]
> https://lore.kernel.org/all/[email protected]/
> [4]
> https://lore.kernel.org/all/[email protected]/
> [5]
> https://lore.kernel.org/all/[email protected]/
> [6] https://lore.kernel.org/all/[email protected]/
>
> Arınç
>
>
On 06/06/2024 11.26, Thorsten Leemhuis wrote:
> On 31.05.24 08:10, Arınç ÜNAL wrote:
>> I had already submitted a patch series that would've prevented this
>> issue back in 14 March 2024 [1]. I've asked numerous times for the patch
>> series to be applied [2][3][4][5].
>>> Eventually Daniel asked for some changes [6]. But I won't have time to do
>> that anytime soon and I think the patch series is good enough to be applied
>> as is.
>
> Then I guess we need some other way to resolve this in mainline to unfix
I don't believe we need another way to resolve it. I've already told that
the patch series is good enough to be applied as is and I don't see any
responses with reasons against this here.
> Frank's device. The two obvious options are afaics:
>
> * revert the culprit (868ff5f4944aa9 ("net: dsa: mt7530-mdio: read PHY
> address of switch from device tree")) and reapply it in a later cycle
Sorry, no. There's nothing wrong with that patch. The actual cause of this
issue is the patch that introduced this device tree source file with the
wrong PHY address.
> * apply Frank's patch (or an improved one) in this thread (and if needed
> revert it when some better changes emerge.
>
> Arınç ÜNAL, could you please comment on that and ideally handle the
> necessary tasks, as it's your patch that causes the regression.
I don't see any necessary tasks for me to handle. AngeloGioacchino Del
Regno whom is the only person I know that maintains these device tree
source files can simply apply the patch series that I have already
submitted and we can all move on. I haven't heard from them whilst the
patch had been waiting since March. So I'm not sure who's going to apply
this patch, and to which tree.
Arınç
On 06.06.24 11:01, Arınç ÜNAL wrote:
> On 06/06/2024 11.26, Thorsten Leemhuis wrote:
>> On 31.05.24 08:10, Arınç ÜNAL wrote:
>>> I had already submitted a patch series that would've prevented this
>>> issue back in 14 March 2024 [1]. I've asked numerous times for the patch
>>> series to be applied [2][3][4][5].
>>>> Eventually Daniel asked for some changes [6]. But I won't have time
>>>> to do
>>> that anytime soon and I think the patch series is good enough to be
>>> applied
>>> as is.
>>
>> Then I guess we need some other way to resolve this in mainline to unfix
>
> I don't believe we need another way to resolve it. I've already told that
> the patch series is good enough to be applied as is and I don't see any
> responses with reasons against this here.
>
>> Frank's device. The two obvious options are afaics:
>>
>> * revert the culprit (868ff5f4944aa9 ("net: dsa: mt7530-mdio: read PHY
>> address of switch from device tree")) and reapply it in a later cycle
>
> Sorry, no. There's nothing wrong with that patch. The actual cause of this
> issue is the patch that introduced this device tree source file with the
> wrong PHY address.
Was that also merged for 6.10? Because if not, then what matters here
afaics is what patch exposed the problem. Of course ideally we wound fix
that problem -- but if nobody takes care of that any time soon it might
come down to a revert of the patch that exposed the problem. At least
that how Linus handles these things afaics.
>> * apply Frank's patch (or an improved one) in this thread (and if needed
>> revert it when some better changes emerge.
>>
>> Arınç ÜNAL, could you please comment on that and ideally handle the
>> necessary tasks, as it's your patch that causes the regression.
>
> I don't see any necessary tasks for me to handle. AngeloGioacchino Del
> Regno whom is the only person I know that maintains these device tree
> source files can simply apply the patch series that I have already
> submitted and we can all move on. I haven't heard from them whilst the
> patch had been waiting since March. So I'm not sure who's going to apply
> this patch, and to which tree.
AngeloGioacchino Del Regno, if you have a minute, could you please
comment if merging those changes for 6.10 is an option?
Ciao, Thorsten
On Thu, 2024-06-06 at 10:26 +0200, Thorsten Leemhuis wrote:
> On 31.05.24 08:10, Arınç ÜNAL wrote:
> > On 31/05/2024 08.40, Thorsten Leemhuis wrote:
> > > [adding Paolo, who committed the culprit]
>
> /me slowly wonders if the culprit should be reverted for now (see below)
> and should be reapplied later together with the matching changes from
> Arınç ÜNAL.
FWIS I think a revert should be avoided, given that a fix is available
and nicely small.
Thanks,
Paolo
On 07.06.24 16:03, Paolo Abeni wrote:
> On Thu, 2024-06-06 at 10:26 +0200, Thorsten Leemhuis wrote:
>> On 31.05.24 08:10, Arınç ÜNAL wrote:
>>> On 31/05/2024 08.40, Thorsten Leemhuis wrote:
>>>> [adding Paolo, who committed the culprit]
>>
>> /me slowly wonders if the culprit should be reverted for now (see below)
>> and should be reapplied later together with the matching changes from
>> Arınç ÜNAL.
>
> FWIS I think a revert should be avoided, given that a fix is available
> and nicely small.
Yeah, on one hand I agree; but on the other it seems that the
maintainers that would have to take care of the dt changes to fix this
until now remained silent in this thread, apart from Rob who sent the
mail regarding the warnings.
I put those maintainers in the To: field of this mail, maybe that might
lead to some reaction.
Ciao, Thorsten
On 07.06.24 16:15, Thorsten Leemhuis wrote:
> On 07.06.24 16:03, Paolo Abeni wrote:
>> On Thu, 2024-06-06 at 10:26 +0200, Thorsten Leemhuis wrote:
>>> On 31.05.24 08:10, Arınç ÜNAL wrote:
>>>> On 31/05/2024 08.40, Thorsten Leemhuis wrote:
>>>>> [adding Paolo, who committed the culprit]
>>>
>>> /me slowly wonders if the culprit should be reverted for now (see below)
>>> and should be reapplied later together with the matching changes from
>>> Arınç ÜNAL.
>>
>> FWIS I think a revert should be avoided, given that a fix is available
>> and nicely small.
>
> Yeah, on one hand I agree; but on the other it seems that the
> maintainers that would have to take care of the dt changes to fix this
> until now remained silent in this thread, apart from Rob who sent the
> mail regarding the warnings.
>
> I put those maintainers in the To: field of this mail, maybe that might
> lead to some reaction.
Still no reply from the DRS folks or any other progress I noticed. Guess
that means I will soon have no other choice than to get Linus involved,
as this looks stuck. :-( #sigh
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
On 11/06/2024 14:30, Thorsten Leemhuis wrote:
> On 07.06.24 16:15, Thorsten Leemhuis wrote:
>> On 07.06.24 16:03, Paolo Abeni wrote:
>>> On Thu, 2024-06-06 at 10:26 +0200, Thorsten Leemhuis wrote:
>>>> On 31.05.24 08:10, Arınç ÜNAL wrote:
>>>>> On 31/05/2024 08.40, Thorsten Leemhuis wrote:
>>>>>> [adding Paolo, who committed the culprit]
>>>>
>>>> /me slowly wonders if the culprit should be reverted for now (see below)
>>>> and should be reapplied later together with the matching changes from
>>>> Arınç ÜNAL.
>>>
>>> FWIS I think a revert should be avoided, given that a fix is available
>>> and nicely small.
>>
>> Yeah, on one hand I agree; but on the other it seems that the
>> maintainers that would have to take care of the dt changes to fix this
>> until now remained silent in this thread, apart from Rob who sent the
>> mail regarding the warnings.
>>
>> I put those maintainers in the To: field of this mail, maybe that might
>> lead to some reaction.
>
> Still no reply from the DRS folks or any other progress I noticed. Guess
> that means I will soon have no other choice than to get Linus involved,
> as this looks stuck. :-( #sigh
Does it have to be Linus that needs to apply "[PATCH 0/2] Set PHY address
of MT7531 switch to 0x1f on MediaTek arm64 boards"? Aren't there any other
ARM maintainers that can apply the fix to their tree?
Arınç
Il 11/06/24 13:38, Arınç ÜNAL ha scritto:
> On 11/06/2024 14:30, Thorsten Leemhuis wrote:
>> On 07.06.24 16:15, Thorsten Leemhuis wrote:
>>> On 07.06.24 16:03, Paolo Abeni wrote:
>>>> On Thu, 2024-06-06 at 10:26 +0200, Thorsten Leemhuis wrote:
>>>>> On 31.05.24 08:10, Arınç ÜNAL wrote:
>>>>>> On 31/05/2024 08.40, Thorsten Leemhuis wrote:
>>>>>>> [adding Paolo, who committed the culprit]
>>>>>
>>>>> /me slowly wonders if the culprit should be reverted for now (see below)
>>>>> and should be reapplied later together with the matching changes from
>>>>> Arınç ÜNAL.
>>>>
>>>> FWIS I think a revert should be avoided, given that a fix is available
>>>> and nicely small.
>>>
>>> Yeah, on one hand I agree; but on the other it seems that the
>>> maintainers that would have to take care of the dt changes to fix this
>>> until now remained silent in this thread, apart from Rob who sent the
>>> mail regarding the warnings.
>>>
>>> I put those maintainers in the To: field of this mail, maybe that might
>>> lead to some reaction.
>>
>> Still no reply from the DRS folks or any other progress I noticed. Guess
>> that means I will soon have no other choice than to get Linus involved,
>> as this looks stuck. :-( #sigh
>
> Does it have to be Linus that needs to apply "[PATCH 0/2] Set PHY address
> of MT7531 switch to 0x1f on MediaTek arm64 boards"? Aren't there any other
> ARM maintainers that can apply the fix to their tree?
>
> Arınç
You have feedback from two people on the series that you mentioned, and noone
is going to apply something that needs to be fixed.
I'm giving you the possibility of addressing the comments in your patch, but
I don't want to see any mention of the driver previously ignoring this or that
as this is irrelevant for a hardware description. Devicetree only describes HW.
Adding up, in commit 868ff5f4944a ("net: dsa: mt7530-mdio: read PHY address of
switch from device tree"),
you have created a regression.
Regressions should be fixed - as in - if the driver did work before with the old
devicetrees, it shall still work. You can't break ABI. Any changes that you do
to your driver must not break functionality with old devicetrees.
So...
------> Fix the driver that you broke <------
After you've fixed it - and I repeat - only after, *and* after someone (Frank?)
validates that the old devicetrees do work with the fixed driver, I will take
the device tree fixes for that MDIO address (as those are, again, fixing a
description of the hardware on those boards, so I agree that those must be fixed
AS WELL).
Regards,
Angelo
On 11/06/2024 15:28, AngeloGioacchino Del Regno wrote:
> Il 11/06/24 13:38, Arınç ÜNAL ha scritto:
>> On 11/06/2024 14:30, Thorsten Leemhuis wrote:
>>> On 07.06.24 16:15, Thorsten Leemhuis wrote:
>>>> On 07.06.24 16:03, Paolo Abeni wrote:
>>>>> On Thu, 2024-06-06 at 10:26 +0200, Thorsten Leemhuis wrote:
>>>>>> On 31.05.24 08:10, Arınç ÜNAL wrote:
>>>>>>> On 31/05/2024 08.40, Thorsten Leemhuis wrote:
>>>>>>>> [adding Paolo, who committed the culprit]
>>>>>>
>>>>>> /me slowly wonders if the culprit should be reverted for now (see below)
>>>>>> and should be reapplied later together with the matching changes from
>>>>>> Arınç ÜNAL.
>>>>>
>>>>> FWIS I think a revert should be avoided, given that a fix is available
>>>>> and nicely small.
>>>>
>>>> Yeah, on one hand I agree; but on the other it seems that the
>>>> maintainers that would have to take care of the dt changes to fix this
>>>> until now remained silent in this thread, apart from Rob who sent the
>>>> mail regarding the warnings.
>>>>
>>>> I put those maintainers in the To: field of this mail, maybe that might
>>>> lead to some reaction.
>>>
>>> Still no reply from the DRS folks or any other progress I noticed. Guess
>>> that means I will soon have no other choice than to get Linus involved,
>>> as this looks stuck. :-( #sigh
>>
>> Does it have to be Linus that needs to apply "[PATCH 0/2] Set PHY address
>> of MT7531 switch to 0x1f on MediaTek arm64 boards"? Aren't there any other
>> ARM maintainers that can apply the fix to their tree?
>>
>> Arınç
>
> You have feedback from two people on the series that you mentioned, and noone
> is going to apply something that needs to be fixed.
>
> I'm giving you the possibility of addressing the comments in your patch, but
> I don't want to see any mention of the driver previously ignoring this or that
> as this is irrelevant for a hardware description. Devicetree only describes HW.
>
> Adding up, in commit 868ff5f4944a ("net: dsa: mt7530-mdio: read PHY address of switch from device tree"),
> you have created a regression.
>
> Regressions should be fixed - as in - if the driver did work before with the old
> devicetrees, it shall still work. You can't break ABI. Any changes that you do
> to your driver must not break functionality with old devicetrees.
>
> So...
>
> ------> Fix the driver that you broke <------
The device tree ABI before the change on the driver:
The reg value represents the PHY address of the switch.
The device tree ABI after the change on the driver:
The reg value represents the PHY address of the switch.
I see no device tree ABI breakage. What I see instead is the driver
starting enforcing the device tree ABI. No change had been made on the
device tree ABI so any non-Linux driver that controls this switch continues
to work.
These old device tree source files in question did not abide by the device
tree ABI in the first place, which is why they don't work anymore as the
Linux driver now enforces the ABI. Device tree source files not conforming
to the ABI is not something to maintain but to fix. The patch series that
fixes them are already submitted.
>
> After you've fixed it - and I repeat - only after, *and* after someone (Frank?)
> validates that the old devicetrees do work with the fixed driver, I will take
> the device tree fixes for that MDIO address (as those are, again, fixing a
> description of the hardware on those boards, so I agree that those must be fixed
> AS WELL).
Sorry, this approach makes no sense to me.
Arınç
Il 11/06/24 14:56, Arınç ÜNAL ha scritto:
> On 11/06/2024 15:28, AngeloGioacchino Del Regno wrote:
>> Il 11/06/24 13:38, Arınç ÜNAL ha scritto:
>>> On 11/06/2024 14:30, Thorsten Leemhuis wrote:
>>>> On 07.06.24 16:15, Thorsten Leemhuis wrote:
>>>>> On 07.06.24 16:03, Paolo Abeni wrote:
>>>>>> On Thu, 2024-06-06 at 10:26 +0200, Thorsten Leemhuis wrote:
>>>>>>> On 31.05.24 08:10, Arınç ÜNAL wrote:
>>>>>>>> On 31/05/2024 08.40, Thorsten Leemhuis wrote:
>>>>>>>>> [adding Paolo, who committed the culprit]
>>>>>>>
>>>>>>> /me slowly wonders if the culprit should be reverted for now (see below)
>>>>>>> and should be reapplied later together with the matching changes from
>>>>>>> Arınç ÜNAL.
>>>>>>
>>>>>> FWIS I think a revert should be avoided, given that a fix is available
>>>>>> and nicely small.
>>>>>
>>>>> Yeah, on one hand I agree; but on the other it seems that the
>>>>> maintainers that would have to take care of the dt changes to fix this
>>>>> until now remained silent in this thread, apart from Rob who sent the
>>>>> mail regarding the warnings.
>>>>>
>>>>> I put those maintainers in the To: field of this mail, maybe that might
>>>>> lead to some reaction.
>>>>
>>>> Still no reply from the DRS folks or any other progress I noticed. Guess
>>>> that means I will soon have no other choice than to get Linus involved,
>>>> as this looks stuck. :-( #sigh
>>>
>>> Does it have to be Linus that needs to apply "[PATCH 0/2] Set PHY address
>>> of MT7531 switch to 0x1f on MediaTek arm64 boards"? Aren't there any other
>>> ARM maintainers that can apply the fix to their tree?
>>>
>>> Arınç
>>
>> You have feedback from two people on the series that you mentioned, and noone
>> is going to apply something that needs to be fixed.
>>
>> I'm giving you the possibility of addressing the comments in your patch, but
>> I don't want to see any mention of the driver previously ignoring this or that
>> as this is irrelevant for a hardware description. Devicetree only describes HW.
>>
>> Adding up, in commit 868ff5f4944a ("net: dsa: mt7530-mdio: read PHY address of
>> switch from device tree"),
>> you have created a regression.
>>
>> Regressions should be fixed - as in - if the driver did work before with the old
>> devicetrees, it shall still work. You can't break ABI. Any changes that you do
>> to your driver must not break functionality with old devicetrees.
>>
>> So...
>>
>> ------> Fix the driver that you broke <------
>
> The device tree ABI before the change on the driver:
>
> The reg value represents the PHY address of the switch.
>
> The device tree ABI after the change on the driver:
>
> The reg value represents the PHY address of the switch.
>
> I see no device tree ABI breakage. What I see instead is the driver
> starting enforcing the device tree ABI. No change had been made on the
> device tree ABI so any non-Linux driver that controls this switch continues
> to work.
>
> These old device tree source files in question did not abide by the device
> tree ABI in the first place, which is why they don't work anymore as the
> Linux driver now enforces the ABI. Device tree source files not conforming
> to the ABI is not something to maintain but to fix. The patch series that
> fixes them are already submitted.
As I said, the devicetree MUST describe the hardware correctly, and on that I do
agree, and I, again, said that I want to take the devicetree fix.
However, the driver regressed, and this broke functionality with old device trees.
Old device trees might have been wrong (and they are, yes), but functionality was
there and the switch was working.
I repeat, driver changes MUST be retro-compatible with older device trees, and your
driver changes ARE NOT; otherwise, this wouldn't be called *regression*.
Again, please fix the driver to be retro-compatible with old device trees.
Regards,
Angelo
On 11/06/2024 16:03, AngeloGioacchino Del Regno wrote:
> Il 11/06/24 14:56, Arınç ÜNAL ha scritto:
>> On 11/06/2024 15:28, AngeloGioacchino Del Regno wrote:
>>> Il 11/06/24 13:38, Arınç ÜNAL ha scritto:
>>>> On 11/06/2024 14:30, Thorsten Leemhuis wrote:
>>>>> On 07.06.24 16:15, Thorsten Leemhuis wrote:
>>>>>> On 07.06.24 16:03, Paolo Abeni wrote:
>>>>>>> On Thu, 2024-06-06 at 10:26 +0200, Thorsten Leemhuis wrote:
>>>>>>>> On 31.05.24 08:10, Arınç ÜNAL wrote:
>>>>>>>>> On 31/05/2024 08.40, Thorsten Leemhuis wrote:
>>>>>>>>>> [adding Paolo, who committed the culprit]
>>>>>>>>
>>>>>>>> /me slowly wonders if the culprit should be reverted for now (see below)
>>>>>>>> and should be reapplied later together with the matching changes from
>>>>>>>> Arınç ÜNAL.
>>>>>>>
>>>>>>> FWIS I think a revert should be avoided, given that a fix is available
>>>>>>> and nicely small.
>>>>>>
>>>>>> Yeah, on one hand I agree; but on the other it seems that the
>>>>>> maintainers that would have to take care of the dt changes to fix this
>>>>>> until now remained silent in this thread, apart from Rob who sent the
>>>>>> mail regarding the warnings.
>>>>>>
>>>>>> I put those maintainers in the To: field of this mail, maybe that might
>>>>>> lead to some reaction.
>>>>>
>>>>> Still no reply from the DRS folks or any other progress I noticed. Guess
>>>>> that means I will soon have no other choice than to get Linus involved,
>>>>> as this looks stuck. :-( #sigh
>>>>
>>>> Does it have to be Linus that needs to apply "[PATCH 0/2] Set PHY address
>>>> of MT7531 switch to 0x1f on MediaTek arm64 boards"? Aren't there any other
>>>> ARM maintainers that can apply the fix to their tree?
>>>>
>>>> Arınç
>>>
>>> You have feedback from two people on the series that you mentioned, and noone
>>> is going to apply something that needs to be fixed.
>>>
>>> I'm giving you the possibility of addressing the comments in your patch, but
>>> I don't want to see any mention of the driver previously ignoring this or that
>>> as this is irrelevant for a hardware description. Devicetree only describes HW.
>>>
>>> Adding up, in commit 868ff5f4944a ("net: dsa: mt7530-mdio: read PHY address of switch from device tree"),
>>> you have created a regression.
>>>
>>> Regressions should be fixed - as in - if the driver did work before with the old
>>> devicetrees, it shall still work. You can't break ABI. Any changes that you do
>>> to your driver must not break functionality with old devicetrees.
>>>
>>> So...
>>>
>>> ------> Fix the driver that you broke <------
>>
>> The device tree ABI before the change on the driver:
>>
>> The reg value represents the PHY address of the switch.
>>
>> The device tree ABI after the change on the driver:
>>
>> The reg value represents the PHY address of the switch.
>>
>> I see no device tree ABI breakage. What I see instead is the driver
>> starting enforcing the device tree ABI. No change had been made on the
>> device tree ABI so any non-Linux driver that controls this switch continues
>> to work.
>>
>> These old device tree source files in question did not abide by the device
>> tree ABI in the first place, which is why they don't work anymore as the
>> Linux driver now enforces the ABI. Device tree source files not conforming
>> to the ABI is not something to maintain but to fix. The patch series that
>> fixes them are already submitted.
>
> As I said, the devicetree MUST describe the hardware correctly, and on that I do
> agree, and I, again, said that I want to take the devicetree fix.
>
> However, the driver regressed, and this broke functionality with old device trees.
> Old device trees might have been wrong (and they are, yes), but functionality was
> there and the switch was working.
>
> I repeat, driver changes MUST be retro-compatible with older device trees, and your
> driver changes ARE NOT; otherwise, this wouldn't be called *regression*.
I'm going to argue that what caused the regression is the broken device
tree. The recent change on the driver only worked towards exposing the
broken device tree. The device tree files hosted on the Linux repository is
not only for use with the Linux drivers. Other projects use these device
tree files as well, as hardware description is not supposed to differ by
project. And for any non-Linux driver that would use this broken device
tree, there would be a regression.
So I don't understand why you demand a change on a Linux driver to be made
before applying the fix for a broken device tree.
That said, I don't understand the old device tree sentiment here. The
driver, after the change, still does support old device trees. Never in the
existence of this switch bindings, the PHY address was supposed to be
described a value other than the PHY address the switch listens on. Yes,
the driver now doesn't work with old and broken device trees. Which is why
we're fixing the said device trees. I don't see why it is necessary to make
the driver support broken device trees just because they used to work for a
certain range of time. This isn't about preserving ABI.
Arınç