[PATCH net-next] net: dsa: mv88e6xxx: enable automedia on 6190x and 6390x devices

[Ante Knezic]

On Tue, 25 Jul 2023 23:22:06 +0200, Andrew Lunn wrote:
>On Tue, Jul 25, 2023 at 11:57:12AM +0200, Ante Knezic wrote:
>> On Mon, 24 Jul 2023 20:34:27 +0200 Anrew Lunn wrote:
>> >By auto-media, you mean both a copper PHY and an SFP? And whichever
>> >gets link first wins the MAC?
>> >
>> 
>> Yes, that is correct.
>> 
>> On Mon, 24 Jul 2023 20:34:27 +0200 Anrew Lunn wrote:
>> >auto-media has been discussed a few times, and rejected, since Linux
>> >has no concept of multiple 'phy like devices' connected to one MAC.
>> >
>> >How are you representing this in DT? I assume you have both an SFP
>> >socket, and a phy-handle pointing to a PHY? phylink will not drive
>> >both at the same time. So you cannot have them admin up at the same
>> >time? How do you get the SFP out of TX disable, when phylink sees a
>> >PHY? What does ethtool return? What the PHY is advertising as its link
>> >modes? Or nothing since an SFP does not advertise speeds?
>> 
>> Patch simply covers the automedia aspect of the device while the
>> exact mode is specified by the DT.
>
>So i would not call this automedia. You are not supporting both copper
>and SFP at the same time, and you are not supporting first win. You
>are just allowing the lower ports to use the SERDES interfaces for
>SFPs.
>

That is true, but in order to be able to use SERDES interface for lower
ports the switch needs to be operating in automedia mode. Automedia here 
simply means power up serdes lane and connect to whichever links first,
but there is no way to deny linking the PHY and allow only SERDES (as
far as I am aware).
I agree that commit message is misleading and wrongly assumes that 
reader has any knowledge of chip details.

>> So for example if you would like
>> to connect an SFP to port 3 of the device you would create a "regular"
>> sfp node just like for ports 9/10 along the lines of:
>>                         port@3 {
>>                                 reg = <3>;
>>                                 label = "SFP";
>>                                 phy-mode = "1000base-x";
>>                                 managed = "in-band-status";
>>                                 sfp = <&sfp1>;
>>                         };
>> 
>> >From then on, phylink will handle the sfp just as if it was connected
>> to ports 9/10 - the ethtool reports advertised and supported link mode
>> as 1000baseX, "Port" is "FIBRE", etc.
>> 
>> Patch looks for "1000base-x" phy-mode in the dt node so in case it
>> is not found the device can be linked only against a copper PHY.
>
>So this used to work. It got broken at some point, and i have a much
>simpler patch in one of the branches:
>
>https://github.com/lunn/linux/commit/74e2c2a9a56fd4e2baeee4d5fbe897c21f394ede
>
>Please try this and see if it works for you.
>

The provided patch does work, however I believe there are some drawbacks to
such simplification:
Firstly, it writes data to READ ONLY bits of the Port Status Register. As
indicated by the datasheet, the CMODE is R/W only for ports 9 and 10, other
ports actually reflect the actual connection via CMODE bits.
Secondly, there is no filtering on setting the ports phy mode. You could be
setting cmode of port 3 to sgmii for example, which is not supported.
And thirdly, during probing of the device set_cmode is called with argument
PHY_INTERFACE_MODE_NA from mv88e6xxx_setup_port. As we would now allow lower
ports to set the cmode, it would be automatically assigned a cmode of 
1000BASE-X because of:

static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
                                    phy_interface_t mode, bool force)
{
        u16 cmode;
        u16 reg;
        int err;

        /* Default to a slow mode, so freeing up SERDES interfaces for
         * other ports which might use them for SFPs.
         */
        if (mode == PHY_INTERFACE_MODE_NA)
                mode = PHY_INTERFACE_MODE_1000BASEX;

So ultimately, when the pcs->init is called it sees that cmode for
all ports is set to 1000BASE-X and proceeds with intialisation (assuming
ports 9 or 10 are configured in XAUI/RXAUI depending on which lane is
being accessed).

If however you believe the above points are irrelevant and there is 
nothing to be gained by the proposed patch than there is nothing more 
to be done here.