Message-ID: <7385ca39-182e-42c1-80bf-fd2d0c0aabdd@arinc9.com>
Date: Sat, 30 Dec 2023 18:56:49 +0300
From: Arınç ÜNAL <arinc.unal@...nc9.com>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>, netdev@...r.kernel.org
Cc: linus.walleij@...aro.org, alsi@...g-olufsen.dk, andrew@...n.ch,
 f.fainelli@...il.com, olteanv@...il.com, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com
Subject: Re: [PATCH net-next v3 8/8] Revert "net: dsa: OF-ware slave_mii_bus"

On 30.12.2023 10:18, Arınç ÜNAL wrote:
> On 23.12.2023 03:46, Luiz Angelo Daros de Luca wrote:
>> This reverts commit fe7324b932222574a0721b80e72c6c5fe57960d1.
>>
>> The use of user_mii_bus is inappropriate when the hardware is described
>> with a device-tree [1].
>>
>> Since all drivers currently implementing ds_switch_ops.phy_{read,write}
>> were not updated to utilize the MDIO information from OF with the
>> generic "dsa user mii", they might not be affected by this change.
>>
>> [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u
> 
> This doesn't make sense to me: "The use of user_mii_bus is inappropriate
> when the hardware is described with a device-tree." I think this is the
> correct statement: "The use of user_mii_bus is inappropriate when the MDIO
> bus of the switch is described on the device-tree."
> 
> The patch log is also not clear on why this leads to the removal of
> OF-based registration from the DSA core driver.
> 
> I would change the patch log here to something like this:
> 
> net: dsa: do not populate user_mii_bus when switch MDIO bus is described
> 
> The use of ds->user_mii_bus is inappropriate when the MDIO bus of the
> switch is described on the device-tree [1].
> 
> To keep things simple, make [all subdrivers that control a switch [with
> MDIO bus] probed on OF] register the bus on the subdriver.
> 
> The subdrivers that control switches [with MDIO bus] probed on OF must
> follow this logic 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.
> 
> 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].
> 
> There can be a case where ds->user_mii_bus is not populated, but
> ds_switch_ops.phy_{read,write} is. That would mean the subdriver controls a
> switch probed on OF, and it lets the DSA core driver to populate
> ds->user_mii_bus and register the bus. We don't want this to happen.
> Therefore, ds_switch_ops.phy_{read,write} should be only used on the
> subdrivers that control switches probed on platform_data.
> 
> With that, the "!ds->user_mii_bus && ds->ops->phy_read" check under
> dsa_switch_setup() remains in use only for switches probed on
> platform_data. Therefore, remove OF-based registration as it's useless now.
> 
> ---
> 
> I think we should do all this in a single patch. I've done it on the MT7530
> DSA subdriver which I maintain.

Actually, there's no need to drag this patch further by including the
improvement of handling the MDIO bus on all relevant subdrivers.

That said, I'd like to submit this patch myself, if it is OK by everyone
here.

Here's the patch log I've prepared:

net: dsa: do not populate user_mii_bus when switch MDIO bus is described

The use of ds->user_mii_bus is inappropriate when the MDIO bus of the
switch is described on the device-tree [1].

To keep things simple, make [all subdrivers that control switches [with
MDIO bus] probed on OF] register the bus on the subdriver. This is already
the case on all of these subdrivers.

There can be a case where ds->user_mii_bus is not populated, but
ds_switch_ops.phy_{read,write} is. That would mean the subdriver controls
switches probed on OF, and it lets the DSA core driver to populate
ds->user_mii_bus and register the bus. We don't want this to happen.
Therefore, ds_switch_ops.phy_{read,write} should be only used on the
subdrivers that control switches probed on platform_data.

With that, the "!ds->user_mii_bus && ds->ops->phy_read" check under
dsa_switch_setup() remains in use only for switches probed on
platform_data. Therefore, remove OF-based registration as it's useless now.

Currently, none of the subdrivers that control switches [with MDIO bus]
probed on OF implements ds_switch_ops.phy_{read,write} so no subdriver will
be affected by this patch.

The subdrivers that control switches [with MDIO bus] probed on OF must
follow this logic 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].

Let's leave the improvement of handling the MDIO bus on relevant subdrivers
to future patches.

Arınç

Powered by blists - more mailing lists