The code block under the "!ds->user_mii_bus && ds->ops->phy_read" check
under dsa_switch_setup() populates ds->user_mii_bus. The use of
ds->user_mii_bus is inappropriate when the MDIO bus of the switch is
described on the device tree [1].
For this reason, use this code block only for switches [with MDIO bus]
probed on platform_data, and OF which the switch MDIO bus isn't described
on the device tree. Therefore, remove OF-based MDIO bus registration as
it's useless for these cases.
These subdrivers which control switches [with MDIO bus] probed on OF, will
lose the ability to register the MDIO bus OF-based:
drivers/net/dsa/b53/b53_common.c
drivers/net/dsa/lan9303-core.c
drivers/net/dsa/realtek/realtek-mdio.c
drivers/net/dsa/vitesse-vsc73xx-core.c
These subdrivers let the DSA core driver register the bus:
- ds->ops->phy_read() and ds->ops->phy_write() are present.
- ds->user_mii_bus is not populated.
The commit fe7324b93222 ("net: dsa: OF-ware slave_mii_bus") which brought
OF-based MDIO bus registration on the DSA core driver is reasonably recent
and, in this time frame, there have been no device trees in the Linux
repository that started describing the MDIO bus, or dt-bindings defining
the MDIO bus for the switches these subdrivers control. So I don't expect
any devices to be affected.
The logic we encourage is that all subdrivers should register the switch
MDIO bus on their own [2]. And, for subdrivers which control switches [with
MDIO bus] probed on OF, this logic must be followed to support all cases
properly:
No switch MDIO bus defined: Populate ds->user_mii_bus, register the MDIO
bus, set the interrupts for PHYs if "interrupt-controller" is defined at
the switch node. This case should only be covered for the switches which
their dt-bindings documentation didn't document the MDIO bus from the
start. This is to keep supporting the device trees that do not describe the
MDIO bus on the device tree but the MDIO bus is being used nonetheless.
Switch MDIO bus defined: Don't populate ds->user_mii_bus, register the MDIO
bus, set the interrupts for PHYs if ["interrupt-controller" is defined at
the switch node and "interrupts" is defined at the PHY nodes under the
switch MDIO bus node].
Switch MDIO bus defined but explicitly disabled: If the device tree says
status = "disabled" for the MDIO bus, we shouldn't need an MDIO bus at all.
Instead, just exit as early as possible and do not call any MDIO API.
After all subdrivers that control switches with MDIO buses are made to
register the MDIO buses on their own, we will be able to get rid of
dsa_switch_ops :: phy_read() and :: phy_write(), and the code block for
registering the MDIO bus on the DSA core driver.
Link: https://lore.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/ [1]
Link: https://lore.kernel.org/netdev/20240103184459.dcbh57wdnlox6w7d@skbuf/ [2]
Suggested-by: Luiz Angelo Daros de Luca <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
net/dsa/dsa.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index ac7be864e80d..09d2f5d4b3dd 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -15,7 +15,6 @@
#include <linux/slab.h>
#include <linux/rtnetlink.h>
#include <linux/of.h>
-#include <linux/of_mdio.h>
#include <linux/of_net.h>
#include <net/dsa_stubs.h>
#include <net/sch_generic.h>
@@ -626,7 +625,6 @@ static void dsa_switch_teardown_tag_protocol(struct dsa_switch *ds)
static int dsa_switch_setup(struct dsa_switch *ds)
{
- struct device_node *dn;
int err;
if (ds->setup)
@@ -666,10 +664,7 @@ static int dsa_switch_setup(struct dsa_switch *ds)
dsa_user_mii_bus_init(ds);
- dn = of_get_child_by_name(ds->dev->of_node, "mdio");
-
- err = of_mdiobus_register(ds->user_mii_bus, dn);
- of_node_put(dn);
+ err = mdiobus_register(ds->user_mii_bus);
if (err < 0)
goto free_user_mii_bus;
}
--
2.40.1
On Mon, Jan 22, 2024 at 08:33:48AM +0300, Arınç ÜNAL wrote:
> These subdrivers which control switches [with MDIO bus] probed on OF, will
> lose the ability to register the MDIO bus OF-based:
>
> drivers/net/dsa/b53/b53_common.c
> drivers/net/dsa/lan9303-core.c
> drivers/net/dsa/realtek/realtek-mdio.c
> drivers/net/dsa/vitesse-vsc73xx-core.c
>
> These subdrivers let the DSA core driver register the bus:
> - ds->ops->phy_read() and ds->ops->phy_write() are present.
> - ds->user_mii_bus is not populated.
>
> The commit fe7324b93222 ("net: dsa: OF-ware slave_mii_bus") which brought
> OF-based MDIO bus registration on the DSA core driver is reasonably recent
> and, in this time frame, there have been no device trees in the Linux
> repository that started describing the MDIO bus, or dt-bindings defining
> the MDIO bus for the switches these subdrivers control. So I don't expect
> any devices to be affected.
IIUC, Luiz made the original patch for the realtek switches. Shouldn't
we wait until realtek registers ds->user_mii_bus on its own, before
reverting? Otherwise, you're basically saying that Luiz made the DSA
core patch without needing it.
On 23.01.2024 18:44, Vladimir Oltean wrote:
> On Mon, Jan 22, 2024 at 08:33:48AM +0300, Arınç ÜNAL wrote:
>> These subdrivers which control switches [with MDIO bus] probed on OF, will
>> lose the ability to register the MDIO bus OF-based:
>>
>> drivers/net/dsa/b53/b53_common.c
>> drivers/net/dsa/lan9303-core.c
>> drivers/net/dsa/realtek/realtek-mdio.c
>> drivers/net/dsa/vitesse-vsc73xx-core.c
>>
>> These subdrivers let the DSA core driver register the bus:
>> - ds->ops->phy_read() and ds->ops->phy_write() are present.
>> - ds->user_mii_bus is not populated.
>>
>> The commit fe7324b93222 ("net: dsa: OF-ware slave_mii_bus") which brought
>> OF-based MDIO bus registration on the DSA core driver is reasonably recent
>> and, in this time frame, there have been no device trees in the Linux
>> repository that started describing the MDIO bus, or dt-bindings defining
>> the MDIO bus for the switches these subdrivers control. So I don't expect
>> any devices to be affected.
>
> IIUC, Luiz made the original patch for the realtek switches. Shouldn't
> we wait until realtek registers ds->user_mii_bus on its own, before
> reverting? Otherwise, you're basically saying that Luiz made the DSA
> core patch without needing it.
My findings point to that. Luiz made the patch to optionally register the
MDIO bus of the MDIO controlled Realtek switches OF-based. So it's not
necessary to wait.
Arınç
On Wed, Jan 24, 2024 at 08:30:11AM +0300, Arınç ÜNAL wrote:
> On 23.01.2024 18:44, Vladimir Oltean wrote:
> > On Mon, Jan 22, 2024 at 08:33:48AM +0300, Arınç ÜNAL wrote:
> > > These subdrivers which control switches [with MDIO bus] probed on OF, will
> > > lose the ability to register the MDIO bus OF-based:
> > >
> > > drivers/net/dsa/b53/b53_common.c
> > > drivers/net/dsa/lan9303-core.c
> > > drivers/net/dsa/realtek/realtek-mdio.c
> > > drivers/net/dsa/vitesse-vsc73xx-core.c
> > >
> > > These subdrivers let the DSA core driver register the bus:
> > > - ds->ops->phy_read() and ds->ops->phy_write() are present.
> > > - ds->user_mii_bus is not populated.
> > >
> > > The commit fe7324b93222 ("net: dsa: OF-ware slave_mii_bus") which brought
> > > OF-based MDIO bus registration on the DSA core driver is reasonably recent
> > > and, in this time frame, there have been no device trees in the Linux
> > > repository that started describing the MDIO bus, or dt-bindings defining
> > > the MDIO bus for the switches these subdrivers control. So I don't expect
> > > any devices to be affected.
> >
> > IIUC, Luiz made the original patch for the realtek switches. Shouldn't
> > we wait until realtek registers ds->user_mii_bus on its own, before
> > reverting? Otherwise, you're basically saying that Luiz made the DSA
> > core patch without needing it.
>
> My findings point to that. Luiz made the patch to optionally register the
> MDIO bus of the MDIO controlled Realtek switches OF-based. So it's not
> necessary to wait.
>
> Arınç
Well, Luiz is copied, he can ack or nack if so.
> > IIUC, Luiz made the original patch for the realtek switches. Shouldn't
> > we wait until realtek registers ds->user_mii_bus on its own, before
> > reverting? Otherwise, you're basically saying that Luiz made the DSA
> > core patch without needing it.
>
> My findings point to that. Luiz made the patch to optionally register the
> MDIO bus of the MDIO controlled Realtek switches OF-based. So it's not
> necessary to wait.
Back in the time when I wrote that code, with the phy_read/write in
dsa_switch_ops, the OF node was only required to associate IRQ to each
port. Until my patch to register its own mdiobus driver lands (I hope
that happens before the next version), the port status will fall back
to polling. I don't think it is a critical feature but I'll let the
maintainers decide. ACK for me.
Regards,
Luiz
On Sat, Jan 27, 2024 at 11:23:33PM -0300, Luiz Angelo Daros de Luca wrote:
> > > IIUC, Luiz made the original patch for the realtek switches. Shouldn't
> > > we wait until realtek registers ds->user_mii_bus on its own, before
> > > reverting? Otherwise, you're basically saying that Luiz made the DSA
> > > core patch without needing it.
> >
> > My findings point to that. Luiz made the patch to optionally register the
> > MDIO bus of the MDIO controlled Realtek switches OF-based. So it's not
> > necessary to wait.
>
> Back in the time when I wrote that code, with the phy_read/write in
> dsa_switch_ops, the OF node was only required to associate IRQ to each
> port. Until my patch to register its own mdiobus driver lands (I hope
> that happens before the next version), the port status will fall back
> to polling. I don't think it is a critical feature but I'll let the
> maintainers decide. ACK for me.
>
> Regards,
>
> Luiz
It isn't really great that this loses IRQ support for Realtek internal PHYs,
especially since Arınç's commit message did not estimate this would happen.
I don't see why this patch could not wait until you resubmit the realtek
consolidation set and it gets accepted.
On 29.01.2024 17:56, Vladimir Oltean wrote:
> On Sat, Jan 27, 2024 at 11:23:33PM -0300, Luiz Angelo Daros de Luca wrote:
>>>> IIUC, Luiz made the original patch for the realtek switches. Shouldn't
>>>> we wait until realtek registers ds->user_mii_bus on its own, before
>>>> reverting? Otherwise, you're basically saying that Luiz made the DSA
>>>> core patch without needing it.
>>>
>>> My findings point to that. Luiz made the patch to optionally register the
>>> MDIO bus of the MDIO controlled Realtek switches OF-based. So it's not
>>> necessary to wait.
>>
>> Back in the time when I wrote that code, with the phy_read/write in
>> dsa_switch_ops, the OF node was only required to associate IRQ to each
>> port. Until my patch to register its own mdiobus driver lands (I hope
>> that happens before the next version), the port status will fall back
>> to polling. I don't think it is a critical feature but I'll let the
>> maintainers decide. ACK for me.
>>
>> Regards,
>>
>> Luiz
>
> It isn't really great that this loses IRQ support for Realtek internal PHYs,
> especially since Arınç's commit message did not estimate this would happen.
>
> I don't see why this patch could not wait until you resubmit the realtek
> consolidation set and it gets accepted.
I agree. I didn't anticipate that realtek-mdio didn't set IRQs on PHYs for
the MDIO bus registered non-OF-based. I'd much rather wait and then send v2
with the mention to realtek-mdio removed.
Arınç
On 1/29/2024 7:53 AM, Arınç ÜNAL wrote:
> On 29.01.2024 17:56, Vladimir Oltean wrote:
>> On Sat, Jan 27, 2024 at 11:23:33PM -0300, Luiz Angelo Daros de Luca
>> wrote:
>>>>> IIUC, Luiz made the original patch for the realtek switches. Shouldn't
>>>>> we wait until realtek registers ds->user_mii_bus on its own, before
>>>>> reverting? Otherwise, you're basically saying that Luiz made the DSA
>>>>> core patch without needing it.
>>>>
>>>> My findings point to that. Luiz made the patch to optionally
>>>> register the
>>>> MDIO bus of the MDIO controlled Realtek switches OF-based. So it's not
>>>> necessary to wait.
>>>
>>> Back in the time when I wrote that code, with the phy_read/write in
>>> dsa_switch_ops, the OF node was only required to associate IRQ to each
>>> port. Until my patch to register its own mdiobus driver lands (I hope
>>> that happens before the next version), the port status will fall back
>>> to polling. I don't think it is a critical feature but I'll let the
>>> maintainers decide. ACK for me.
>>>
>>> Regards,
>>>
>>> Luiz
>>
>> It isn't really great that this loses IRQ support for Realtek internal
>> PHYs,
>> especially since Arınç's commit message did not estimate this would
>> happen.
>>
>> I don't see why this patch could not wait until you resubmit the realtek
>> consolidation set and it gets accepted.
>
> I agree. I didn't anticipate that realtek-mdio didn't set IRQs on PHYs for
> the MDIO bus registered non-OF-based. I'd much rather wait and then send v2
> with the mention to realtek-mdio removed.
Sounds good, thanks!
--
Florian
> >> I don't see why this patch could not wait until you resubmit the realtek
> >> consolidation set and it gets accepted.
> >
> > I agree. I didn't anticipate that realtek-mdio didn't set IRQs on PHYs for
> > the MDIO bus registered non-OF-based. I'd much rather wait and then send v2
> > with the mention to realtek-mdio removed.
Hello Arinç,
Realtek-mdio now uses its own mdiobus driver. The revert is good to go.
Acked-by: Luiz Angelo Daros de Luca <[email protected]>
Regards,
Luiz