[PATCH net] net: dsa: mv88e6xxx: Disable AN on 2500base-x for Amethyst

[PATCH net] net: dsa: mv88e6xxx: Disable AN on 2500base-x for Amethyst

[Marek Behún]

Amethyst does not support autonegotiation in 2500base-x mode.
It does not link with AN enabled with other devices.
Disable autonegotiation for Amethyst in 2500base-x mode.

Fixes: de776d0d316f ("net: dsa: mv88e6xxx: add support for mv88e6393x family")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/net/dsa/mv88e6xxx/chip.c   |  2 +-
 drivers/net/dsa/mv88e6xxx/serdes.c | 12 ++++++++++++
 drivers/net/dsa/mv88e6xxx/serdes.h |  4 ++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index f00cbf5753b9..7ed420128cea 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4945,7 +4945,7 @@ static const struct mv88e6xxx_ops mv88e6393x_ops = {
 	.serdes_power = mv88e6393x_serdes_power,
 	.serdes_get_lane = mv88e6393x_serdes_get_lane,
 	.serdes_pcs_get_state = mv88e6393x_serdes_pcs_get_state,
-	.serdes_pcs_config = mv88e6390_serdes_pcs_config,
+	.serdes_pcs_config = mv88e6393x_serdes_pcs_config,
 	.serdes_pcs_an_restart = mv88e6390_serdes_pcs_an_restart,
 	.serdes_pcs_link_up = mv88e6390_serdes_pcs_link_up,
 	.serdes_irq_mapping = mv88e6390_serdes_irq_mapping,
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 6ea003678798..b70979bd07df 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -880,6 +880,18 @@ int mv88e6390_serdes_pcs_config(struct mv88e6xxx_chip *chip, int port,
 				      MV88E6390_SGMII_BMCR, bmcr);
 }
 
+int mv88e6393x_serdes_pcs_config(struct mv88e6xxx_chip *chip, int port,
+				 int lane, unsigned int mode,
+				 phy_interface_t interface,
+				 const unsigned long *advertise)
+{
+	if (interface == PHY_INTERFACE_MODE_2500BASEX)
+		return 0;
+
+	return mv88e6390_serdes_pcs_config(chip, port, lane, mode, interface,
+					   advertise);
+}
+
 static int mv88e6390_serdes_pcs_get_state_sgmii(struct mv88e6xxx_chip *chip,
 	int port, int lane, struct phylink_link_state *state)
 {
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
index cbb3ba30caea..fdf56377013b 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -111,6 +111,10 @@ int mv88e6390_serdes_pcs_config(struct mv88e6xxx_chip *chip, int port,
 				int lane, unsigned int mode,
 				phy_interface_t interface,
 				const unsigned long *advertise);
+int mv88e6393x_serdes_pcs_config(struct mv88e6xxx_chip *chip, int port,
+				 int lane, unsigned int mode,
+				 phy_interface_t interface,
+				 const unsigned long *advertise);
 int mv88e6185_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, int port,
 				   int lane, struct phylink_link_state *state);
 int mv88e6352_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, int port,
-- 
2.32.0

Re: [PATCH net] net: dsa: mv88e6xxx: Disable AN on 2500base-x for Amethyst

[Andrew Lunn]

On Thu, Nov 25, 2021 at 03:43:59PM +0100, Marek Behún wrote:
> Amethyst does not support autonegotiation in 2500base-x mode.

Hi Marek

I tried to avoid using Marvells internal names for these devices. I
don't think these names exist in the datasheet? They are visible in
the SDK, but that is not so widely available. So if you do want to use
these names, please also reference the name we use in the kernel,
mv88e6393x.

> It does not link with AN enabled with other devices.
> Disable autonegotiation for Amethyst in 2500base-x mode.
> 
> +int mv88e6393x_serdes_pcs_config(struct mv88e6xxx_chip *chip, int port,
> +				 int lane, unsigned int mode,
> +				 phy_interface_t interface,
> +				 const unsigned long *advertise)
> +{
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		return 0;
> +
> +	return mv88e6390_serdes_pcs_config(chip, port, lane, mode, interface,
> +					   advertise);
> +}

What happens when changing from say 1000BaseX to 2500BaseX? Do you
need to disable the advertisement which 1000BaseX might of enabled?

     Andrew

Re: [PATCH net] net: dsa: mv88e6xxx: Disable AN on 2500base-x for Amethyst

[Marek Behún]

On Thu, 25 Nov 2021 16:37:40 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Nov 25, 2021 at 03:43:59PM +0100, Marek Behún wrote:
> > Amethyst does not support autonegotiation in 2500base-x mode.  
> 
> Hi Marek
> 
> I tried to avoid using Marvells internal names for these devices. I
> don't think these names exist in the datasheet? They are visible in
> the SDK, but that is not so widely available. So if you do want to use
> these names, please also reference the name we use in the kernel,
> mv88e6393x.

I used these names in previous commit that are already merged (search
for Amethyst, Topaz, Peridot). But if you want, I can send v2. Please
let me know if I should send v2.

> > It does not link with AN enabled with other devices.
> > Disable autonegotiation for Amethyst in 2500base-x mode.
> > 
> > +int mv88e6393x_serdes_pcs_config(struct mv88e6xxx_chip *chip, int port,
> > +				 int lane, unsigned int mode,
> > +				 phy_interface_t interface,
> > +				 const unsigned long *advertise)
> > +{
> > +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> > +		return 0;
> > +
> > +	return mv88e6390_serdes_pcs_config(chip, port, lane, mode, interface,
> > +					   advertise);
> > +}  
> 
> What happens when changing from say 1000BaseX to 2500BaseX? Do you
> need to disable the advertisement which 1000BaseX might of enabled?
> 
>      Andrew

I don't need to disable it, it is disabled on it's own by cmode change.

Moreover I did some experiments on 88E6393X vs 88E6190.

It is a little weird for 6393x.

On 6190
- resetting the SerDes (BMCR_RESET) does not have impact on the
  BMCR_ANENABLE bit, but changing cmode does

  when cmode is changed to SGMII or 1000base-x, it is enabled, for
  2500base-x it is disabled

- resetting the SerDes (BMCR_RESET) when cmode is
  - sgmii, changes value of the MV88E6390_SGMII_ADVERTISE to 0x0001
    automatically
  - 1000base-x or 2500base-x does not change the value of adv register

  moreover it seems that changing cmode also resets the the SerDes

On 6393x:
- the BMCR behaves the same as in 6190: the switch defaults to AN
  disabled for 2500base-x, and enabled for 1000base-x and SGMII

- the difference is that there are weird values written to
  MV88E6390_SGMII_ADVERTISE on SerDes reset (which is done by switch
  when changing cmode)

  for 1000base-x, the value is 0x9120
  for 2500base-x, the value is 0x9f41

  I don't understand why they changed it so for 6393x.

Marek

Re: [PATCH net] net: dsa: mv88e6xxx: Disable AN on 2500base-x for Amethyst

[Andrew Lunn]

On Thu, Nov 25, 2021 at 06:45:51PM +0100, Marek Behún wrote:
> On Thu, 25 Nov 2021 16:37:40 +0100
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > On Thu, Nov 25, 2021 at 03:43:59PM +0100, Marek Behún wrote:
> > > Amethyst does not support autonegotiation in 2500base-x mode.  
> > 
> > Hi Marek
> > 
> > I tried to avoid using Marvells internal names for these devices. I
> > don't think these names exist in the datasheet? They are visible in
> > the SDK, but that is not so widely available. So if you do want to use
> > these names, please also reference the name we use in the kernel,
> > mv88e6393x.
> 
> I used these names in previous commit that are already merged (search
> for Amethyst, Topaz, Peridot). But if you want, I can send v2. Please
> let me know if I should send v2.

I'm not against these names, but i don't like them used on there own,
thats all.

> > What happens when changing from say 1000BaseX to 2500BaseX? Do you
> > need to disable the advertisement which 1000BaseX might of enabled?

> 
> I don't need to disable it, it is disabled on it's own by cmode change.

O.K, that is important information to put into the commit message,
since it makes it clear you have thought about this, and a reviewer
does not need to ask the question.

> Moreover I did some experiments on 88E6393X vs 88E6190.
> 
> It is a little weird for 6393x.
> 
> On 6190
> - resetting the SerDes (BMCR_RESET) does not have impact on the
>   BMCR_ANENABLE bit, but changing cmode does
> 
>   when cmode is changed to SGMII or 1000base-x, it is enabled, for
>   2500base-x it is disabled
> 
> - resetting the SerDes (BMCR_RESET) when cmode is
>   - sgmii, changes value of the MV88E6390_SGMII_ADVERTISE to 0x0001
>     automatically
>   - 1000base-x or 2500base-x does not change the value of adv register
> 
>   moreover it seems that changing cmode also resets the the SerDes
> 
> On 6393x:
> - the BMCR behaves the same as in 6190: the switch defaults to AN
>   disabled for 2500base-x, and enabled for 1000base-x and SGMII
> 
> - the difference is that there are weird values written to
>   MV88E6390_SGMII_ADVERTISE on SerDes reset (which is done by switch
>   when changing cmode)
> 
>   for 1000base-x, the value is 0x9120
>   for 2500base-x, the value is 0x9f41
> 
>   I don't understand why they changed it so for 6393x.

Tobias found something which might be relevant here. On the 6390
family, if you change cmode while the SERDES is powered off, bad
things happen. What you could be seeing is another symptom of
that. Tobias has a WIP patch which changes the order of operations
when changing the cmode. It would be interesting to see if that makes
a difference here as well.

  Andrew

Re: [PATCH net] net: dsa: mv88e6xxx: Disable AN on 2500base-x for Amethyst

[Marek Behún]

On Thu, 25 Nov 2021 18:56:39 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Nov 25, 2021 at 06:45:51PM +0100, Marek Behún wrote:
> > On Thu, 25 Nov 2021 16:37:40 +0100
> > Andrew Lunn <andrew@lunn.ch> wrote:
> >   
> > > On Thu, Nov 25, 2021 at 03:43:59PM +0100, Marek Behún wrote:  
> > > > Amethyst does not support autonegotiation in 2500base-x mode.    
> > > 
> > > Hi Marek
> > > 
> > > I tried to avoid using Marvells internal names for these devices. I
> > > don't think these names exist in the datasheet? They are visible in
> > > the SDK, but that is not so widely available. So if you do want to use
> > > these names, please also reference the name we use in the kernel,
> > > mv88e6393x.  
> > 
> > I used these names in previous commit that are already merged (search
> > for Amethyst, Topaz, Peridot). But if you want, I can send v2. Please
> > let me know if I should send v2.  
> 
> I'm not against these names, but i don't like them used on there own,
> thats all.
> 
> > > What happens when changing from say 1000BaseX to 2500BaseX? Do you
> > > need to disable the advertisement which 1000BaseX might of enabled?  
> 
> > 
> > I don't need to disable it, it is disabled on it's own by cmode change.  
> 
> O.K, that is important information to put into the commit message,
> since it makes it clear you have thought about this, and a reviewer
> does not need to ask the question.
> 
> > Moreover I did some experiments on 88E6393X vs 88E6190.
> > 
> > It is a little weird for 6393x.
> > 
> > On 6190
> > - resetting the SerDes (BMCR_RESET) does not have impact on the
> >   BMCR_ANENABLE bit, but changing cmode does
> > 
> >   when cmode is changed to SGMII or 1000base-x, it is enabled, for
> >   2500base-x it is disabled
> > 
> > - resetting the SerDes (BMCR_RESET) when cmode is
> >   - sgmii, changes value of the MV88E6390_SGMII_ADVERTISE to 0x0001
> >     automatically
> >   - 1000base-x or 2500base-x does not change the value of adv register
> > 
> >   moreover it seems that changing cmode also resets the the SerDes
> > 
> > On 6393x:
> > - the BMCR behaves the same as in 6190: the switch defaults to AN
> >   disabled for 2500base-x, and enabled for 1000base-x and SGMII
> > 
> > - the difference is that there are weird values written to
> >   MV88E6390_SGMII_ADVERTISE on SerDes reset (which is done by switch
> >   when changing cmode)
> > 
> >   for 1000base-x, the value is 0x9120
> >   for 2500base-x, the value is 0x9f41
> > 
> >   I don't understand why they changed it so for 6393x.  
> 
> Tobias found something which might be relevant here. On the 6390
> family, if you change cmode while the SERDES is powered off, bad
> things happen. What you could be seeing is another symptom of
> that. Tobias has a WIP patch which changes the order of operations
> when changing the cmode. It would be interesting to see if that makes
> a difference here as well.

Thanks. I will try this tomorrow, both on 6190 and 6393x.

For now, ignore this patch.

Marek