mbox series

[0/1] net: dsa: b53: mmap: add dsa switch ops

Message ID 20230323170238.210687-1-noltari@gmail.com (mailing list archive)
Headers show
Series net: dsa: b53: mmap: add dsa switch ops | expand

Message

Álvaro Fernández Rojas March 23, 2023, 5:02 p.m. UTC
B53 MMAP switches have a MDIO Mux bus controller which should be used instead
of the default phy_read/phy_write ops used in the rest of the B53 controllers.
Therefore, in order to use the proper MDIO Mux bus controller we need to
replicate the default B53 DSA switch ops removing the phy_read/phy_write
entries.
Without this, when external switches are configured together with B53 MMAP
internal switches the device will hang on phy_read/phy_write ops.

This is an alternative to:
- https://patchwork.kernel.org/project/netdevbpf/cover/20230317113427.302162-1-noltari@gmail.com/
- https://patchwork.kernel.org/project/netdevbpf/patch/20230317113427.302162-2-noltari@gmail.com/
- https://patchwork.kernel.org/project/netdevbpf/patch/20230317113427.302162-3-noltari@gmail.com/
- https://patchwork.kernel.org/project/netdevbpf/patch/20230317113427.302162-4-noltari@gmail.com/
As discussed, it was an ABI break and not the correct way of fixing the issue.

And also to:
- https://patchwork.kernel.org/project/netdevbpf/patch/20230320182813.963508-1-noltari@gmail.com/

Álvaro Fernández Rojas (1):
  net: dsa: b53: mmap: add dsa switch ops

 drivers/net/dsa/b53/b53_common.c | 22 +++++++++---------
 drivers/net/dsa/b53/b53_mmap.c   | 40 ++++++++++++++++++++++++++++++++
 drivers/net/dsa/b53/b53_priv.h   | 11 +++++++++
 3 files changed, 62 insertions(+), 11 deletions(-)

Comments

Florian Fainelli March 23, 2023, 6:19 p.m. UTC | #1
On 3/23/23 10:02, Álvaro Fernández Rojas wrote:
> B53 MMAP switches have a MDIO Mux bus controller which should be used instead
> of the default phy_read/phy_write ops used in the rest of the B53 controllers.
> Therefore, in order to use the proper MDIO Mux bus controller we need to
> replicate the default B53 DSA switch ops removing the phy_read/phy_write
> entries.

Did you try to implement b53_mmap_ops::phy_read16/phy_write16 and have 
them return -EIO such that you do not fallback to the else path:

                   ret = b53_read16(priv, B53_PORT_MII_PAGE(addr),
                                    reg * 2, &value);

The reason for the hang I believe is because the B53_PORT_MII_PAGE is 
simply not mapped into the switch register space, and there is no logic 
within the switch block to return an error when you read at that invalid 
location.

Re-implementing dsa_switch_ops is usually done when you have a very 
different switch integration logic, ala bcm_sf2, here it seems a bit of 
a tall order for simply not using the phy_read16/phy_write16 functions.
Álvaro Fernández Rojas March 23, 2023, 7:42 p.m. UTC | #2
El jue, 23 mar 2023 a las 19:19, Florian Fainelli
(<f.fainelli@gmail.com>) escribió:
>
> On 3/23/23 10:02, Álvaro Fernández Rojas wrote:
> > B53 MMAP switches have a MDIO Mux bus controller which should be used instead
> > of the default phy_read/phy_write ops used in the rest of the B53 controllers.
> > Therefore, in order to use the proper MDIO Mux bus controller we need to
> > replicate the default B53 DSA switch ops removing the phy_read/phy_write
> > entries.
>
> Did you try to implement b53_mmap_ops::phy_read16/phy_write16 and have
> them return -EIO such that you do not fallback to the else path:

Actually I tried -EINVAL and it didn't work, but I've just tried -EIO
and it works!
Many thanks for the suggestion!

I will send another patch adding phy_read/write ops and returning
-EIO, so please ignore this patch and sorry for the noise, but it took
a while until we reached a good solution for this :(...

>
>                    ret = b53_read16(priv, B53_PORT_MII_PAGE(addr),
>                                     reg * 2, &value);
>
> The reason for the hang I believe is because the B53_PORT_MII_PAGE is
> simply not mapped into the switch register space, and there is no logic
> within the switch block to return an error when you read at that invalid
> location.
>
> Re-implementing dsa_switch_ops is usually done when you have a very
> different switch integration logic, ala bcm_sf2, here it seems a bit of
> a tall order for simply not using the phy_read16/phy_write16 functions.
> --
> Florian
>

--
Álvaro
Florian Fainelli March 23, 2023, 7:45 p.m. UTC | #3
On 3/23/23 12:42, Álvaro Fernández Rojas wrote:
> El jue, 23 mar 2023 a las 19:19, Florian Fainelli
> (<f.fainelli@gmail.com>) escribió:
>>
>> On 3/23/23 10:02, Álvaro Fernández Rojas wrote:
>>> B53 MMAP switches have a MDIO Mux bus controller which should be used instead
>>> of the default phy_read/phy_write ops used in the rest of the B53 controllers.
>>> Therefore, in order to use the proper MDIO Mux bus controller we need to
>>> replicate the default B53 DSA switch ops removing the phy_read/phy_write
>>> entries.
>>
>> Did you try to implement b53_mmap_ops::phy_read16/phy_write16 and have
>> them return -EIO such that you do not fallback to the else path:
> 
> Actually I tried -EINVAL and it didn't work, but I've just tried -EIO
> and it works!
> Many thanks for the suggestion!
> 
> I will send another patch adding phy_read/write ops and returning
> -EIO, so please ignore this patch and sorry for the noise, but it took
> a while until we reached a good solution for this :(...

No worries, -EIO works because it is treated specially through the PHY 
library to indicate a read error occurred, whereas the other return 
codes do not necessarily produce that effect.

Thanks for your persistence!