[PATCH net-next v6 0/6] dsa: lan9303: Move to PHYLINK

[PATCH net-next v6 0/6] dsa: lan9303: Move to PHYLINK

[Jerry Ray]

This patch series moves the lan9303 driver to use the phylink
api away from phylib.

Migrating to phylink means removing the .adjust_link api. The
functionality from the adjust_link is moved to the phylink_mac_link_up
api.  The code being removed only affected the cpu port.  The other
ports on the LAN9303 do not need anything from the phylink_mac_link_up
api.

Patches:
 0001 - Whitespace only change aligning the dsa_switch_ops members.
	No code changes.
 0002 - Moves the Turbo bit initialization out of the adjust_link api and
	places it in a driver initialization execution path. It only needs
	to be initialized once, it is never changed, and it is not a
	per-port flag.
 0003 - Adds exception handling logic in the extremely unlikely event that
	the read of the device fails.
 0004 - Performance optimization that skips a slow register write if there
	is no need to perform it.
 0005 - Change the way we identify the xMII port as phydev will be NULL
	when this logic is moved into phylink_mac_link_up.
 0006 - Removes adjust_link and begins using the phylink dsa_switch_ops
	apis.
---
v5->v6:
  - Moved to using port number to identify xMII port for the LAN9303.
v4->v5:
  - Created prep patches to better show how things migrate.
  - cleaned up comments.
v3->v4:
  - Addressed whitespace issues as a separate patch.
  - Removed port_max_mtu api patch as it is unrelated to phylink migration.
  - Reworked the implementation to preserve the adjust_link functionality
    by including it in the phylink_mac_link_up api.
v2->v3:
  Added back in disabling Turbo Mode on the CPU MII interface.
  Removed the unnecessary clearing of the phyvsupported interfaces.
v1->v2:
  corrected the reported mtu size, removing ETH_HLEN and ETH_FCS_LEN

 drivers/net/dsa/lan9303-core.c | xx ++++++++++++--------
 1 file changed

[PATCH net-next v6 1/6] dsa: lan9303: align dsa_switch_ops members

[Jerry Ray]

Whitespace preparatory patch, making the dsa_switch_ops table consistent.
No code is added or removed.

Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
---
v4->v5:
 changed patch title to be less generic
 Addressed space-tab issue missed in the initial patch.
---
 drivers/net/dsa/lan9303-core.c | 38 +++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 80f07bd20593..5a21fc96d479 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1280,25 +1280,25 @@ static int lan9303_port_mdb_del(struct dsa_switch *ds, int port,
 }
 
 static const struct dsa_switch_ops lan9303_switch_ops = {
-	.get_tag_protocol = lan9303_get_tag_protocol,
-	.setup = lan9303_setup,
-	.get_strings = lan9303_get_strings,
-	.phy_read = lan9303_phy_read,
-	.phy_write = lan9303_phy_write,
-	.adjust_link = lan9303_adjust_link,
-	.get_ethtool_stats = lan9303_get_ethtool_stats,
-	.get_sset_count = lan9303_get_sset_count,
-	.port_enable = lan9303_port_enable,
-	.port_disable = lan9303_port_disable,
-	.port_bridge_join       = lan9303_port_bridge_join,
-	.port_bridge_leave      = lan9303_port_bridge_leave,
-	.port_stp_state_set     = lan9303_port_stp_state_set,
-	.port_fast_age          = lan9303_port_fast_age,
-	.port_fdb_add           = lan9303_port_fdb_add,
-	.port_fdb_del           = lan9303_port_fdb_del,
-	.port_fdb_dump          = lan9303_port_fdb_dump,
-	.port_mdb_add           = lan9303_port_mdb_add,
-	.port_mdb_del           = lan9303_port_mdb_del,
+	.get_tag_protocol	= lan9303_get_tag_protocol,
+	.setup			= lan9303_setup,
+	.get_strings		= lan9303_get_strings,
+	.phy_read		= lan9303_phy_read,
+	.phy_write		= lan9303_phy_write,
+	.adjust_link		= lan9303_adjust_link,
+	.get_ethtool_stats	= lan9303_get_ethtool_stats,
+	.get_sset_count		= lan9303_get_sset_count,
+	.port_enable		= lan9303_port_enable,
+	.port_disable		= lan9303_port_disable,
+	.port_bridge_join	= lan9303_port_bridge_join,
+	.port_bridge_leave	= lan9303_port_bridge_leave,
+	.port_stp_state_set	= lan9303_port_stp_state_set,
+	.port_fast_age		= lan9303_port_fast_age,
+	.port_fdb_add		= lan9303_port_fdb_add,
+	.port_fdb_del		= lan9303_port_fdb_del,
+	.port_fdb_dump		= lan9303_port_fdb_dump,
+	.port_mdb_add		= lan9303_port_mdb_add,
+	.port_mdb_del		= lan9303_port_mdb_del,
 };
 
 static int lan9303_register_switch(struct lan9303 *chip)
-- 
2.17.1

Re: [PATCH net-next v6 1/6] dsa: lan9303: align dsa_switch_ops members

[Vladimir Oltean]

On Mon, Jan 09, 2023 at 03:18:44PM -0600, Jerry Ray wrote:
> Whitespace preparatory patch, making the dsa_switch_ops table consistent.
> No code is added or removed.
> 
> Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v6 1/6] dsa: lan9303: align dsa_switch_ops members

[Florian Fainelli]

On 1/9/23 13:18, Jerry Ray wrote:
> Whitespace preparatory patch, making the dsa_switch_ops table consistent.
> No code is added or removed.
> 
> Signed-off-by: Jerry Ray <jerry.ray@microchip.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next v6 2/6] dsa: lan9303: move Turbo Mode bit init

[Jerry Ray]

In preparing to remove the .adjust_link api, I am moving the one-time
initialization of the device's Turbo Mode bit into a different execution
path. This code clears (disables) the Turbo Mode bit which is never used
by this driver. Turbo Mode is a non-standard mode that would allow the
100Mbps RMII interface to run at 200Mbps.

Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
---
 drivers/net/dsa/lan9303-core.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 5a21fc96d479..50470fb09cb4 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -886,6 +886,12 @@ static int lan9303_check_device(struct lan9303 *chip)
 		return ret;
 	}
 
+	/* Virtual Phy: Remove Turbo 200Mbit mode */
+	lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &reg);
+
+	reg &= ~LAN9303_VIRT_SPECIAL_TURBO;
+	regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, reg);
+
 	return 0;
 }
 
@@ -1050,7 +1056,6 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
 static void lan9303_adjust_link(struct dsa_switch *ds, int port,
 				struct phy_device *phydev)
 {
-	struct lan9303 *chip = ds->priv;
 	int ctl;
 
 	if (!phy_is_pseudo_fixed_link(phydev))
@@ -1073,14 +1078,6 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
 		ctl &= ~BMCR_FULLDPLX;
 
 	lan9303_phy_write(ds, port, MII_BMCR, ctl);
-
-	if (port == chip->phy_addr_base) {
-		/* Virtual Phy: Remove Turbo 200Mbit mode */
-		lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &ctl);
-
-		ctl &= ~LAN9303_VIRT_SPECIAL_TURBO;
-		regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, ctl);
-	}
 }
 
 static int lan9303_port_enable(struct dsa_switch *ds, int port,
-- 
2.17.1

Re: [PATCH net-next v6 2/6] dsa: lan9303: move Turbo Mode bit init

[Vladimir Oltean]

On Mon, Jan 09, 2023 at 03:18:45PM -0600, Jerry Ray wrote:
> In preparing to remove the .adjust_link api, I am moving the one-time
> initialization of the device's Turbo Mode bit into a different execution
> path. This code clears (disables) the Turbo Mode bit which is never used
> by this driver. Turbo Mode is a non-standard mode that would allow the
> 100Mbps RMII interface to run at 200Mbps.
> 
> Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> ---
>  drivers/net/dsa/lan9303-core.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 5a21fc96d479..50470fb09cb4 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -886,6 +886,12 @@ static int lan9303_check_device(struct lan9303 *chip)
>  		return ret;
>  	}
>  
> +	/* Virtual Phy: Remove Turbo 200Mbit mode */
> +	lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &reg);
> +
> +	reg &= ~LAN9303_VIRT_SPECIAL_TURBO;
> +	regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, reg);
> +

Isn't a function whose name is lan9303_check_device() being abused for
this purpose (device initialization)?

>  	return 0;
>  }
>  
> @@ -1050,7 +1056,6 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
>  static void lan9303_adjust_link(struct dsa_switch *ds, int port,
>  				struct phy_device *phydev)
>  {
> -	struct lan9303 *chip = ds->priv;
>  	int ctl;
>  
>  	if (!phy_is_pseudo_fixed_link(phydev))
> @@ -1073,14 +1078,6 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
>  		ctl &= ~BMCR_FULLDPLX;
>  
>  	lan9303_phy_write(ds, port, MII_BMCR, ctl);
> -
> -	if (port == chip->phy_addr_base) {
> -		/* Virtual Phy: Remove Turbo 200Mbit mode */
> -		lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &ctl);
> -
> -		ctl &= ~LAN9303_VIRT_SPECIAL_TURBO;
> -		regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, ctl);
> -	}
>  }
>  
>  static int lan9303_port_enable(struct dsa_switch *ds, int port,
> -- 
> 2.17.1
> 

RE: [PATCH net-next v6 2/6] dsa: lan9303: move Turbo Mode bit init

[Jerry.Ray]

> > In preparing to remove the .adjust_link api, I am moving the one-time
> > initialization of the device's Turbo Mode bit into a different execution
> > path. This code clears (disables) the Turbo Mode bit which is never used
> > by this driver. Turbo Mode is a non-standard mode that would allow the
> > 100Mbps RMII interface to run at 200Mbps.
> >
> > Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> > ---
> >  drivers/net/dsa/lan9303-core.c | 15 ++++++---------
> >  1 file changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> > index 5a21fc96d479..50470fb09cb4 100644
> > --- a/drivers/net/dsa/lan9303-core.c
> > +++ b/drivers/net/dsa/lan9303-core.c
> > @@ -886,6 +886,12 @@ static int lan9303_check_device(struct lan9303 *chip)
> >               return ret;
> >       }
> >
> > +     /* Virtual Phy: Remove Turbo 200Mbit mode */
> > +     lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &reg);
> > +
> > +     reg &= ~LAN9303_VIRT_SPECIAL_TURBO;
> > +     regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, reg);
> > +
> 
> Isn't a function whose name is lan9303_check_device() being abused for
> this purpose (device initialization)?
> 
I will move this into lan9303_setup.

Regards,
Jerry.

> >       return 0;
> >  }
> >
> > @@ -1050,7 +1056,6 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
> >  static void lan9303_adjust_link(struct dsa_switch *ds, int port,
> >                               struct phy_device *phydev)
> >  {
> > -     struct lan9303 *chip = ds->priv;
> >       int ctl;
> >
> >       if (!phy_is_pseudo_fixed_link(phydev))
> > @@ -1073,14 +1078,6 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
> >               ctl &= ~BMCR_FULLDPLX;
> >
> >       lan9303_phy_write(ds, port, MII_BMCR, ctl);
> > -
> > -     if (port == chip->phy_addr_base) {
> > -             /* Virtual Phy: Remove Turbo 200Mbit mode */
> > -             lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &ctl);
> > -
> > -             ctl &= ~LAN9303_VIRT_SPECIAL_TURBO;
> > -             regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, ctl);
> > -     }
> >  }
> >
> >  static int lan9303_port_enable(struct dsa_switch *ds, int port,
> > --
> > 2.17.1

RE: [PATCH net-next v6 2/6] dsa: lan9303: move Turbo Mode bit init

[Jerry.Ray]

> > In preparing to remove the .adjust_link api, I am moving the one-time
> > initialization of the device's Turbo Mode bit into a different execution
> > path. This code clears (disables) the Turbo Mode bit which is never used
> > by this driver. Turbo Mode is a non-standard mode that would allow the
> > 100Mbps RMII interface to run at 200Mbps.
> >
> > Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> > ---
> >  drivers/net/dsa/lan9303-core.c | 15 ++++++---------
> >  1 file changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> > index 5a21fc96d479..50470fb09cb4 100644
> > --- a/drivers/net/dsa/lan9303-core.c
> > +++ b/drivers/net/dsa/lan9303-core.c
> > @@ -886,6 +886,12 @@ static int lan9303_check_device(struct lan9303 *chip)
> >               return ret;
> >       }
> >
> > +     /* Virtual Phy: Remove Turbo 200Mbit mode */
> > +     lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &reg);
> > +
> > +     reg &= ~LAN9303_VIRT_SPECIAL_TURBO;
> > +     regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, reg);
> > +
> 
> Isn't a function whose name is lan9303_check_device() being abused for
> this purpose (device initialization)?
> 
I will move this into lan9303_setup.

Regards,
Jerry.

> >       return 0;
> >  }
> >
> > @@ -1050,7 +1056,6 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
> >  static void lan9303_adjust_link(struct dsa_switch *ds, int port,
> >                               struct phy_device *phydev)
> >  {
> > -     struct lan9303 *chip = ds->priv;
> >       int ctl;
> >
> >       if (!phy_is_pseudo_fixed_link(phydev))
> > @@ -1073,14 +1078,6 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
> >               ctl &= ~BMCR_FULLDPLX;
> >
> >       lan9303_phy_write(ds, port, MII_BMCR, ctl);
> > -
> > -     if (port == chip->phy_addr_base) {
> > -             /* Virtual Phy: Remove Turbo 200Mbit mode */
> > -             lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &ctl);
> > -
> > -             ctl &= ~LAN9303_VIRT_SPECIAL_TURBO;
> > -             regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, ctl);
> > -     }
> >  }
> >
> >  static int lan9303_port_enable(struct dsa_switch *ds, int port,
> > --
> > 2.17.1

[PATCH net-next v6 3/6] dsa: lan9303: Add exception logic for read failure

[Jerry Ray]

While it is highly unlikely a read will ever fail, This code fragment is
now in a function that allows us to return an error code. A read failure
here will cause the lan9303_probe to fail.

Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
---
 drivers/net/dsa/lan9303-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 50470fb09cb4..8eee340f6464 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -887,7 +887,9 @@ static int lan9303_check_device(struct lan9303 *chip)
 	}
 
 	/* Virtual Phy: Remove Turbo 200Mbit mode */
-	lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &reg);
+	ret = lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &reg);
+	if (ret)
+		return (ret);
 
 	reg &= ~LAN9303_VIRT_SPECIAL_TURBO;
 	regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, reg);
-- 
2.17.1

[PATCH net-next v6 4/6] dsa: lan9303: write reg only if necessary

[Jerry Ray]

As the regmap_write() is over a slow bus that will sleep, we can speed up
the boot-up time a bit by not bothering to clear a bit that is already
clear.

Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
---
 drivers/net/dsa/lan9303-core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 8eee340f6464..792ce6a26a6a 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -891,8 +891,11 @@ static int lan9303_check_device(struct lan9303 *chip)
 	if (ret)
 		return (ret);
 
-	reg &= ~LAN9303_VIRT_SPECIAL_TURBO;
-	regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, reg);
+	/* Clear the TURBO Mode bit if it was set. */
+	if (reg & LAN9303_VIRT_SPECIAL_TURBO) {
+		reg &= ~LAN9303_VIRT_SPECIAL_TURBO;
+		regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, reg);
+	}
 
 	return 0;
 }
-- 
2.17.1

[PATCH net-next v6 5/6] dsa: lan9303: Port 0 is xMII port

[Jerry Ray]

In preparing to move the adjust_link logic into the phylink_mac_link_up
api, change the macro used to check for the cpu port. In
phylink_mac_link_up, the phydev pointer passed in for the CPU port is
NULL, so we can't keep using phy_is_pseudo_fixed_link(phydev).

Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
---
v5->v6:
  Using port 0 to identify the xMII port on the LAN9303.
---
 drivers/net/dsa/lan9303-core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 792ce6a26a6a..7be4c491e5d9 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1063,7 +1063,11 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
 {
 	int ctl;
 
-	if (!phy_is_pseudo_fixed_link(phydev))
+	/* On this device, we are only interested in doing something here if
+	 * this is an xMII port. All other ports are 10/100 phys using MDIO
+	 * to control there link settings.
+	 */
+	if (port != 0)
 		return;
 
 	ctl = lan9303_phy_read(ds, port, MII_BMCR);
-- 
2.17.1

Re: [PATCH net-next v6 5/6] dsa: lan9303: Port 0 is xMII port

[Vladimir Oltean]

On Mon, Jan 09, 2023 at 03:18:48PM -0600, Jerry Ray wrote:
> In preparing to move the adjust_link logic into the phylink_mac_link_up
> api, change the macro used to check for the cpu port. In
> phylink_mac_link_up, the phydev pointer passed in for the CPU port is
> NULL, so we can't keep using phy_is_pseudo_fixed_link(phydev).
> 
> Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> ---
> v5->v6:
>   Using port 0 to identify the xMII port on the LAN9303.
> ---
>  drivers/net/dsa/lan9303-core.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 792ce6a26a6a..7be4c491e5d9 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -1063,7 +1063,11 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
>  {
>  	int ctl;
>  
> -	if (!phy_is_pseudo_fixed_link(phydev))
> +	/* On this device, we are only interested in doing something here if
> +	 * this is an xMII port. All other ports are 10/100 phys using MDIO
> +	 * to control there link settings.
> +	 */
> +	if (port != 0)

Maybe a macro LAN9303_XMII_PORT would be good, if it was also
consistently used in lan9303_setup()?

>  		return;
>  
>  	ctl = lan9303_phy_read(ds, port, MII_BMCR);
> -- 
> 2.17.1
> 

RE: [PATCH net-next v6 5/6] dsa: lan9303: Port 0 is xMII port

[Jerry.Ray]

> > In preparing to move the adjust_link logic into the phylink_mac_link_up
> > api, change the macro used to check for the cpu port. In
> > phylink_mac_link_up, the phydev pointer passed in for the CPU port is
> > NULL, so we can't keep using phy_is_pseudo_fixed_link(phydev).
> >
> > Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> > ---
> > v5->v6:
> >   Using port 0 to identify the xMII port on the LAN9303.
> > ---
> >  drivers/net/dsa/lan9303-core.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> > index 792ce6a26a6a..7be4c491e5d9 100644
> > --- a/drivers/net/dsa/lan9303-core.c
> > +++ b/drivers/net/dsa/lan9303-core.c
> > @@ -1063,7 +1063,11 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
> >  {
> >       int ctl;
> >
> > -     if (!phy_is_pseudo_fixed_link(phydev))
> > +     /* On this device, we are only interested in doing something here if
> > +      * this is an xMII port. All other ports are 10/100 phys using MDIO
> > +      * to control there link settings.
> > +      */
> > +     if (port != 0)
> 
> Maybe a macro LAN9303_XMII_PORT would be good, if it was also
> consistently used in lan9303_setup()?
> 

Agreed.  As I add more devices that have different capabilities, this will
change. I was hesitant to add this logic now as it is uncalled for until
multiple device types are supported by the driver.

> >               return;
> >
> >       ctl = lan9303_phy_read(ds, port, MII_BMCR);
> > --
> > 2.17.1
> >

[PATCH net-next v6 6/6] dsa: lan9303: Migrate to PHYLINK

[Jerry Ray]

This patch replaces the adjust_link api with the phylink apis that provide
equivalent functionality.

The remaining functionality from the adjust_link is now covered in the
phylink_mac_link_up api.

Removes:
.adjust_link
Adds:
.phylink_get_caps
.phylink_mac_link_up

Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
---
v5->v6:
  - Moved to using port number to identify xMII port for the LAN9303.
v4->v5:
  - Added various prep patches to better show the movement of the logic.
v3->v4:
  - Reworked the implementation to preserve the adjust_link functionality
    by including it in the phylink_mac_link_up api.
v2->v3:
  Added back in disabling Turbo Mode on the CPU MII interface.
  Removed the unnecessary clearing of the phy supported interfaces.
---
 drivers/net/dsa/lan9303-core.c | 101 ++++++++++++++++++++++-----------
 1 file changed, 69 insertions(+), 32 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 7be4c491e5d9..e514fff81af6 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1058,37 +1058,6 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
 	return chip->ops->phy_write(chip, phy, regnum, val);
 }
 
-static void lan9303_adjust_link(struct dsa_switch *ds, int port,
-				struct phy_device *phydev)
-{
-	int ctl;
-
-	/* On this device, we are only interested in doing something here if
-	 * this is an xMII port. All other ports are 10/100 phys using MDIO
-	 * to control there link settings.
-	 */
-	if (port != 0)
-		return;
-
-	ctl = lan9303_phy_read(ds, port, MII_BMCR);
-
-	ctl &= ~BMCR_ANENABLE;
-
-	if (phydev->speed == SPEED_100)
-		ctl |= BMCR_SPEED100;
-	else if (phydev->speed == SPEED_10)
-		ctl &= ~BMCR_SPEED100;
-	else
-		dev_err(ds->dev, "unsupported speed: %d\n", phydev->speed);
-
-	if (phydev->duplex == DUPLEX_FULL)
-		ctl |= BMCR_FULLDPLX;
-	else
-		ctl &= ~BMCR_FULLDPLX;
-
-	lan9303_phy_write(ds, port, MII_BMCR, ctl);
-}
-
 static int lan9303_port_enable(struct dsa_switch *ds, int port,
 			       struct phy_device *phy)
 {
@@ -1285,13 +1254,81 @@ static int lan9303_port_mdb_del(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
+				     struct phylink_config *config)
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
+
+	config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
+				   MAC_SYM_PAUSE;
+
+	if (port == 0) {
+		__set_bit(PHY_INTERFACE_MODE_RMII,
+			  config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_MII,
+			  config->supported_interfaces);
+	} else {
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  config->supported_interfaces);
+		/* Compatibility for phylib's default interface type when the
+		 * phy-mode property is absent
+		 */
+		__set_bit(PHY_INTERFACE_MODE_GMII,
+			  config->supported_interfaces);
+	}
+
+	/* This driver does not make use of the speed, duplex, pause or the
+	 * advertisement in its mac_config, so it is safe to mark this driver
+	 * as non-legacy.
+	 */
+	config->legacy_pre_march2020 = false;
+}
+
+static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int port,
+					unsigned int mode,
+					phy_interface_t interface,
+					struct phy_device *phydev, int speed,
+					int duplex, bool tx_pause,
+					bool rx_pause)
+{
+	u32 ctl;
+
+	/* On this device, we are only interested in doing something here if
+	 * this is the xMII port. All other ports are 10/100 phys using MDIO
+	 * to control there link settings.
+	 */
+	if (port != 0)
+		return;
+
+	ctl = lan9303_phy_read(ds, port, MII_BMCR);
+
+	ctl &= ~BMCR_ANENABLE;
+
+	if (speed == SPEED_100)
+		ctl |= BMCR_SPEED100;
+	else if (speed == SPEED_10)
+		ctl &= ~BMCR_SPEED100;
+	else
+		dev_err(ds->dev, "unsupported speed: %d\n", speed);
+
+	if (duplex == DUPLEX_FULL)
+		ctl |= BMCR_FULLDPLX;
+	else
+		ctl &= ~BMCR_FULLDPLX;
+
+	lan9303_phy_write(ds, port, MII_BMCR, ctl);
+}
+
 static const struct dsa_switch_ops lan9303_switch_ops = {
 	.get_tag_protocol	= lan9303_get_tag_protocol,
 	.setup			= lan9303_setup,
 	.get_strings		= lan9303_get_strings,
 	.phy_read		= lan9303_phy_read,
 	.phy_write		= lan9303_phy_write,
-	.adjust_link		= lan9303_adjust_link,
+	.phylink_get_caps	= lan9303_phylink_get_caps,
+	.phylink_mac_link_up	= lan9303_phylink_mac_link_up,
 	.get_ethtool_stats	= lan9303_get_ethtool_stats,
 	.get_sset_count		= lan9303_get_sset_count,
 	.port_enable		= lan9303_port_enable,
-- 
2.17.1

Re: [PATCH net-next v6 6/6] dsa: lan9303: Migrate to PHYLINK

[Arun.Ramadoss]

Hi Jerry,
On Mon, 2023-01-09 at 15:18 -0600, Jerry Ray wrote:
> 
> diff --git a/drivers/net/dsa/lan9303-core.c
> b/drivers/net/dsa/lan9303-core.c
> index 7be4c491e5d9..e514fff81af6 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -1058,37 +1058,6 @@ static int lan9303_phy_write(struct dsa_switch
> *ds, int phy, int regnum,
>  	return chip->ops->phy_write(chip, phy, regnum, val);
>  }
>    
> +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int
> port,
> +				     struct phylink_config *config)
> +{
> +	struct lan9303 *chip = ds->priv;
> +
> +	dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> +
> +	config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> +				   MAC_SYM_PAUSE;
> +
> +	if (port == 0) {
> +		__set_bit(PHY_INTERFACE_MODE_RMII,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_MII,
> +			  config->supported_interfaces);
> +	} else {
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  config->supported_interfaces);
> +		/* Compatibility for phylib's default interface type
> when the
> +		 * phy-mode property is absent
> +		 */
> +		__set_bit(PHY_INTERFACE_MODE_GMII,
> +			  config->supported_interfaces);
> +	}
> +
> +	/* This driver does not make use of the speed, duplex, pause or
> the
> +	 * advertisement in its mac_config, so it is safe to mark this
> driver
> +	 * as non-legacy.
> +	 */
> +	config->legacy_pre_march2020 = false;
> +}
> +
> +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int
> port,
> +					unsigned int mode,
> +					phy_interface_t interface,
> +					struct phy_device *phydev, int
> speed,
> +					int duplex, bool tx_pause,
> +					bool rx_pause)
> +{
> +	u32 ctl;
> +
> +	/* On this device, we are only interested in doing something
> here if
> +	 * this is the xMII port. All other ports are 10/100 phys using
> MDIO
> +	 * to control there link settings.
> +	 */
> +	if (port != 0)
> +		return;
> +
> +	ctl = lan9303_phy_read(ds, port, MII_BMCR);
> +
> +	ctl &= ~BMCR_ANENABLE;
> +
> +	if (speed == SPEED_100)
> +		ctl |= BMCR_SPEED100;
> +	else if (speed == SPEED_10)
> +		ctl &= ~BMCR_SPEED100;
> +	else
> +		dev_err(ds->dev, "unsupported speed: %d\n", speed);

I think, We will not reach in the error part, since in the
phylink_get_caps we specified only 10 and 100 Mbps speed. Phylink layer
will validate and if the value is beyond the speed supported, it will 
return back.

> +
> +	if (duplex == DUPLEX_FULL)
> +		ctl |= BMCR_FULLDPLX;
> +	else
> +		ctl &= ~BMCR_FULLDPLX;
> +
> +	lan9303_phy_write(ds, port, MII_BMCR, ctl);
> +}
> +
> 

RE: [PATCH net-next v6 6/6] dsa: lan9303: Migrate to PHYLINK

[Jerry.Ray]

> > diff --git a/drivers/net/dsa/lan9303-core.c
> > b/drivers/net/dsa/lan9303-core.c
> > index 7be4c491e5d9..e514fff81af6 100644
> > --- a/drivers/net/dsa/lan9303-core.c
> > +++ b/drivers/net/dsa/lan9303-core.c
> > @@ -1058,37 +1058,6 @@ static int lan9303_phy_write(struct dsa_switch
> > *ds, int phy, int regnum,
> >  	return chip->ops->phy_write(chip, phy, regnum, val);
> >  }
> >    
> > +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int
> > port,
> > +				     struct phylink_config *config)
> > +{
> > +	struct lan9303 *chip = ds->priv;
> > +
> > +	dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> > +
> > +	config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> > +				   MAC_SYM_PAUSE;
> > +
> > +	if (port == 0) {
> > +		__set_bit(PHY_INTERFACE_MODE_RMII,
> > +			  config->supported_interfaces);
> > +		__set_bit(PHY_INTERFACE_MODE_MII,
> > +			  config->supported_interfaces);
> > +	} else {
> > +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > +			  config->supported_interfaces);
> > +		/* Compatibility for phylib's default interface type
> > when the
> > +		 * phy-mode property is absent
> > +		 */
> > +		__set_bit(PHY_INTERFACE_MODE_GMII,
> > +			  config->supported_interfaces);
> > +	}
> > +
> > +	/* This driver does not make use of the speed, duplex, pause or
> > the
> > +	 * advertisement in its mac_config, so it is safe to mark this
> > driver
> > +	 * as non-legacy.
> > +	 */
> > +	config->legacy_pre_march2020 = false;
> > +}
> > +
> > +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int
> > port,
> > +					unsigned int mode,
> > +					phy_interface_t interface,
> > +					struct phy_device *phydev, int
> > speed,
> > +					int duplex, bool tx_pause,
> > +					bool rx_pause)
> > +{
> > +	u32 ctl;
> > +
> > +	/* On this device, we are only interested in doing something
> > here if
> > +	 * this is the xMII port. All other ports are 10/100 phys using
> > MDIO
> > +	 * to control there link settings.
> > +	 */
> > +	if (port != 0)
> > +		return;
> > +
> > +	ctl = lan9303_phy_read(ds, port, MII_BMCR);
> > +
> > +	ctl &= ~BMCR_ANENABLE;
> > +
> > +	if (speed == SPEED_100)
> > +		ctl |= BMCR_SPEED100;
> > +	else if (speed == SPEED_10)
> > +		ctl &= ~BMCR_SPEED100;
> > +	else
> > +		dev_err(ds->dev, "unsupported speed: %d\n", speed);
> 
> I think, We will not reach in the error part, since in the
> phylink_get_caps we specified only 10 and 100 Mbps speed. Phylink layer
> will validate and if the value is beyond the speed supported, it will 
> return back.
> 

I will remove the error check and will go with either 10 or 100.
	ctl &= ~BMCR_SPEED100;
	if ((speed == SPEED_100)
		ctl |= BMCR_SPEED100;

Regards,
Jerry.

> > +
> > +	if (duplex == DUPLEX_FULL)
> > +		ctl |= BMCR_FULLDPLX;
> > +	else
> > +		ctl &= ~BMCR_FULLDPLX;
> > +
> > +	lan9303_phy_write(ds, port, MII_BMCR, ctl);
> > +}
> > +

Re: [PATCH net-next v6 6/6] dsa: lan9303: Migrate to PHYLINK

[Russell King (Oracle)]

On Mon, Jan 09, 2023 at 03:18:49PM -0600, Jerry Ray wrote:
> +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
> +				     struct phylink_config *config)
> +{
> +	struct lan9303 *chip = ds->priv;
> +
> +	dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> +
> +	config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> +				   MAC_SYM_PAUSE;

You indicate that pause modes are supported, but...

> +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int port,
> +					unsigned int mode,
> +					phy_interface_t interface,
> +					struct phy_device *phydev, int speed,
> +					int duplex, bool tx_pause,
> +					bool rx_pause)
> +{
> +	u32 ctl;
> +
> +	/* On this device, we are only interested in doing something here if
> +	 * this is the xMII port. All other ports are 10/100 phys using MDIO
> +	 * to control there link settings.
> +	 */
> +	if (port != 0)
> +		return;
> +
> +	ctl = lan9303_phy_read(ds, port, MII_BMCR);
> +
> +	ctl &= ~BMCR_ANENABLE;
> +
> +	if (speed == SPEED_100)
> +		ctl |= BMCR_SPEED100;
> +	else if (speed == SPEED_10)
> +		ctl &= ~BMCR_SPEED100;
> +	else
> +		dev_err(ds->dev, "unsupported speed: %d\n", speed);
> +
> +	if (duplex == DUPLEX_FULL)
> +		ctl |= BMCR_FULLDPLX;
> +	else
> +		ctl &= ~BMCR_FULLDPLX;
> +
> +	lan9303_phy_write(ds, port, MII_BMCR, ctl);

There is no code here to program the resolved pause modes. Is it handled
internally within the switch? (Please add a comment to this effect
either in get_caps or here.)

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH net-next v6 6/6] dsa: lan9303: Migrate to PHYLINK

[Jerry.Ray]

> > +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
> > +                                  struct phylink_config *config)
> > +{
> > +     struct lan9303 *chip = ds->priv;
> > +
> > +     dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> > +
> > +     config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> > +                                MAC_SYM_PAUSE;
> 
> You indicate that pause modes are supported, but...
> 
> > +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int port,
> > +                                     unsigned int mode,
> > +                                     phy_interface_t interface,
> > +                                     struct phy_device *phydev, int speed,
> > +                                     int duplex, bool tx_pause,
> > +                                     bool rx_pause)
> > +{
> > +     u32 ctl;
> > +
> > +     /* On this device, we are only interested in doing something here if
> > +      * this is the xMII port. All other ports are 10/100 phys using MDIO
> > +      * to control there link settings.
> > +      */
> > +     if (port != 0)
> > +             return;
> > +
> > +     ctl = lan9303_phy_read(ds, port, MII_BMCR);
> > +
> > +     ctl &= ~BMCR_ANENABLE;
> > +
> > +     if (speed == SPEED_100)
> > +             ctl |= BMCR_SPEED100;
> > +     else if (speed == SPEED_10)
> > +             ctl &= ~BMCR_SPEED100;
> > +     else
> > +             dev_err(ds->dev, "unsupported speed: %d\n", speed);
> > +
> > +     if (duplex == DUPLEX_FULL)
> > +             ctl |= BMCR_FULLDPLX;
> > +     else
> > +             ctl &= ~BMCR_FULLDPLX;
> > +
> > +     lan9303_phy_write(ds, port, MII_BMCR, ctl);
> 
> There is no code here to program the resolved pause modes. Is it handled
> internally within the switch? (Please add a comment to this effect
> either in get_caps or here.)
> 
> Thanks.
> 

As I look into this, the part does have control flags for advertising
Symmetric and Asymmetric pause toward the link partner. The default is set
by a configuration strap on power-up. I am having trouble mapping the rx and
tx pause parameters into symmetric and asymmetric controls. Where can I find
the proper definitions and mappings?

  ctl &= ~( ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_AYM);
  if(tx_pause)
    ctl |= ADVERTISE_PAUSE_CAP;
  if(rx_pause)
    ctl |= ADVERTISE_PAUSE_AYM;

If I can pause my transmissions (receive pause requests), then advertise
symmetric whether I ever sent pause requests or not.
If I can send pause requests (using flow control on my receive side), then make
sure asymmetric support is also advertised as rx_pause might have been clear.
Not that if both rx and tx pause is supported, we can support either symmetric
or asymmetric operating modes.

If I receive a pause request, it affects my transmit data flow. So one could
argue the rx_pause flag controls my ability to receive pause requests. I tend
to overthink and almost always get these 50/50 propositions wrong.

Regards,
Jerry

Re: [PATCH net-next v6 6/6] dsa: lan9303: Migrate to PHYLINK

[Russell King (Oracle)]

On Mon, Jan 16, 2023 at 05:22:05PM +0000, Jerry.Ray@microchip.com wrote:
> > > +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
> > > +                                  struct phylink_config *config)
> > > +{
> > > +     struct lan9303 *chip = ds->priv;
> > > +
> > > +     dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> > > +
> > > +     config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> > > +                                MAC_SYM_PAUSE;
> > 
> > You indicate that pause modes are supported, but...
> > 
> > > +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int port,
> > > +                                     unsigned int mode,
> > > +                                     phy_interface_t interface,
> > > +                                     struct phy_device *phydev, int speed,
> > > +                                     int duplex, bool tx_pause,
> > > +                                     bool rx_pause)
> > > +{
> > > +     u32 ctl;
> > > +
> > > +     /* On this device, we are only interested in doing something here if
> > > +      * this is the xMII port. All other ports are 10/100 phys using MDIO
> > > +      * to control there link settings.
> > > +      */
> > > +     if (port != 0)
> > > +             return;
> > > +
> > > +     ctl = lan9303_phy_read(ds, port, MII_BMCR);
> > > +
> > > +     ctl &= ~BMCR_ANENABLE;
> > > +
> > > +     if (speed == SPEED_100)
> > > +             ctl |= BMCR_SPEED100;
> > > +     else if (speed == SPEED_10)
> > > +             ctl &= ~BMCR_SPEED100;
> > > +     else
> > > +             dev_err(ds->dev, "unsupported speed: %d\n", speed);
> > > +
> > > +     if (duplex == DUPLEX_FULL)
> > > +             ctl |= BMCR_FULLDPLX;
> > > +     else
> > > +             ctl &= ~BMCR_FULLDPLX;
> > > +
> > > +     lan9303_phy_write(ds, port, MII_BMCR, ctl);
> > 
> > There is no code here to program the resolved pause modes. Is it handled
> > internally within the switch? (Please add a comment to this effect
> > either in get_caps or here.)
> > 
> > Thanks.
> > 
> 
> As I look into this, the part does have control flags for advertising
> Symmetric and Asymmetric pause toward the link partner. The default is set
> by a configuration strap on power-up. I am having trouble mapping the rx and
> tx pause parameters into symmetric and asymmetric controls. Where can I find
> the proper definitions and mappings?
> 
>   ctl &= ~( ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_AYM);
>   if(tx_pause)
>     ctl |= ADVERTISE_PAUSE_CAP;
>   if(rx_pause)
>     ctl |= ADVERTISE_PAUSE_AYM;

lan9303_phylink_mac_link_up() has nothing to do with what might be
advertised to the link partner - this is called when the link has been
negotiated and established, and it's purpose is to program the results
of the resolution into the MAC.

That means programming the MAC to operate at the negotiated speed and
duplex, and also permitting the MAC to generate pause frames when its
receive side becomes full (tx_pause) and whether to act on pause frames
received over the network (rx_pause).

If there's nowhere to program the MAC to accept and/or generate pause
frames, how are they controlled? Does the MAC always accept and/or
generate them? Or does the MAC always ignore them and never generates
them?

If the latter, then that suggests pause frames are not supported, and
thus MAC_SYM_PAUSE and MAC_ASYM_PAUSE should not be set in the get_caps
method.

This leads me on to another question - in the above quoted code, what
device's BMCR is being accessed in lan9303_phylink_mac_link_up() ? Is
it a PCS? If it is, please use the phylink_pcs support, as the
pcs_config() method gives you what is necessary to program the PCS
advertisement.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH net-next v6 6/6] dsa: lan9303: Migrate to PHYLINK

[Jerry.Ray]

> > > > +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
> > > > +                                  struct phylink_config *config)
> > > > +{
> > > > +     struct lan9303 *chip = ds->priv;
> > > > +
> > > > +     dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> > > > +
> > > > +     config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> > > > +                                MAC_SYM_PAUSE;
> > >
> > > You indicate that pause modes are supported, but...
> > >
> > > > +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int port,
> > > > +                                     unsigned int mode,
> > > > +                                     phy_interface_t interface,
> > > > +                                     struct phy_device *phydev, int speed,
> > > > +                                     int duplex, bool tx_pause,
> > > > +                                     bool rx_pause)
> > > > +{
> > > > +     u32 ctl;
> > > > +
> > > > +     /* On this device, we are only interested in doing something here if
> > > > +      * this is the xMII port. All other ports are 10/100 phys using MDIO
> > > > +      * to control there link settings.
> > > > +      */
> > > > +     if (port != 0)
> > > > +             return;
> > > > +
> > > > +     ctl = lan9303_phy_read(ds, port, MII_BMCR);
> > > > +
> > > > +     ctl &= ~BMCR_ANENABLE;
> > > > +
> > > > +     if (speed == SPEED_100)
> > > > +             ctl |= BMCR_SPEED100;
> > > > +     else if (speed == SPEED_10)
> > > > +             ctl &= ~BMCR_SPEED100;
> > > > +     else
> > > > +             dev_err(ds->dev, "unsupported speed: %d\n", speed);
> > > > +
> > > > +     if (duplex == DUPLEX_FULL)
> > > > +             ctl |= BMCR_FULLDPLX;
> > > > +     else
> > > > +             ctl &= ~BMCR_FULLDPLX;
> > > > +
> > > > +     lan9303_phy_write(ds, port, MII_BMCR, ctl);
> > >
> > > There is no code here to program the resolved pause modes. Is it handled
> > > internally within the switch? (Please add a comment to this effect
> > > either in get_caps or here.)
> > >
> > > Thanks.
> > >
> >
> > As I look into this, the part does have control flags for advertising
> > Symmetric and Asymmetric pause toward the link partner. The default is set
> > by a configuration strap on power-up. I am having trouble mapping the rx and
> > tx pause parameters into symmetric and asymmetric controls. Where can I find
> > the proper definitions and mappings?
> >
> >   ctl &= ~( ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_AYM);
> >   if(tx_pause)
> >     ctl |= ADVERTISE_PAUSE_CAP;
> >   if(rx_pause)
> >     ctl |= ADVERTISE_PAUSE_AYM;
> 
> lan9303_phylink_mac_link_up() has nothing to do with what might be
> advertised to the link partner - this is called when the link has been
> negotiated and established, and it's purpose is to program the results
> of the resolution into the MAC.
> 
> That means programming the MAC to operate at the negotiated speed and
> duplex, and also permitting the MAC to generate pause frames when its
> receive side becomes full (tx_pause) and whether to act on pause frames
> received over the network (rx_pause).
> 
> If there's nowhere to program the MAC to accept and/or generate pause
> frames, how are they controlled? Does the MAC always accept and/or
> generate them? Or does the MAC always ignore them and never generates
> them?
> 
> If the latter, then that suggests pause frames are not supported, and
> thus MAC_SYM_PAUSE and MAC_ASYM_PAUSE should not be set in the get_caps
> method.
> 
> This leads me on to another question - in the above quoted code, what
> device's BMCR is being accessed in lan9303_phylink_mac_link_up() ? Is
> it a PCS? If it is, please use the phylink_pcs support, as the
> pcs_config() method gives you what is necessary to program the PCS
> advertisement.
> 
> Thanks.
> 
> --

On this device, the XMII connection is the rev-xmii port connected to the CPU.
This is the DSA port. This device 'emulates' a phy (virtual phy) allowing the
CPU to use standard phy registers to set things up.

Let me back up for a moment.
The device supports half-duplex BackPressure as well as full-duplex Flow
Control.
The device has bootstrapping options that will configure the Settings for
BP and FC. On port 0, these strapping options also affect the Virtual Phys
Auto-Negotiation Link Partner Base Page Ability Register.
If auto-negotiation is enabled, the flow control is enabled/disabled based
on the Sym/Asym settings of the Advertised and Link Partner's capabilities
registers.
If Manual Flow Control is enabled, then flow control is programmed into the
Manual_FC_0 register directly and the auto-neg registers are ignored. The
device can be strapped to use (default to) the Manual FC register.

So this is why I'm trying to reflect the flow control settings as provided in
the mac_link_up hook api into the emulated phy's aneg settings.

Question:  In the get capabilities API, should I report the device's
flow control capabilities independent of how the device is bootstrapped or
should I reflect the bootstrapped settings? I consider the bootstrap setting
to affect the register default rather than limit what the device is physically
capable of supporting.

Thanks for helping me get this right.
Regards,
Jerry.

Re: [PATCH net-next v6 6/6] dsa: lan9303: Migrate to PHYLINK

[Russell King (Oracle)]

On Mon, Jan 16, 2023 at 10:48:47PM +0000, Jerry.Ray@microchip.com wrote:
> > > > > +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
> > > > > +                                  struct phylink_config *config)
> > > > > +{
> > > > > +     struct lan9303 *chip = ds->priv;
> > > > > +
> > > > > +     dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> > > > > +
> > > > > +     config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> > > > > +                                MAC_SYM_PAUSE;
> > > >
> > > > You indicate that pause modes are supported, but...
> > > >
> > > > > +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int port,
> > > > > +                                     unsigned int mode,
> > > > > +                                     phy_interface_t interface,
> > > > > +                                     struct phy_device *phydev, int speed,
> > > > > +                                     int duplex, bool tx_pause,
> > > > > +                                     bool rx_pause)
> > > > > +{
> > > > > +     u32 ctl;
> > > > > +
> > > > > +     /* On this device, we are only interested in doing something here if
> > > > > +      * this is the xMII port. All other ports are 10/100 phys using MDIO
> > > > > +      * to control there link settings.
> > > > > +      */
> > > > > +     if (port != 0)
> > > > > +             return;
> > > > > +
> > > > > +     ctl = lan9303_phy_read(ds, port, MII_BMCR);
> > > > > +
> > > > > +     ctl &= ~BMCR_ANENABLE;
> > > > > +
> > > > > +     if (speed == SPEED_100)
> > > > > +             ctl |= BMCR_SPEED100;
> > > > > +     else if (speed == SPEED_10)
> > > > > +             ctl &= ~BMCR_SPEED100;
> > > > > +     else
> > > > > +             dev_err(ds->dev, "unsupported speed: %d\n", speed);
> > > > > +
> > > > > +     if (duplex == DUPLEX_FULL)
> > > > > +             ctl |= BMCR_FULLDPLX;
> > > > > +     else
> > > > > +             ctl &= ~BMCR_FULLDPLX;
> > > > > +
> > > > > +     lan9303_phy_write(ds, port, MII_BMCR, ctl);
> > > >
> > > > There is no code here to program the resolved pause modes. Is it handled
> > > > internally within the switch? (Please add a comment to this effect
> > > > either in get_caps or here.)
> > > >
> > > > Thanks.
> > > >
> > >
> > > As I look into this, the part does have control flags for advertising
> > > Symmetric and Asymmetric pause toward the link partner. The default is set
> > > by a configuration strap on power-up. I am having trouble mapping the rx and
> > > tx pause parameters into symmetric and asymmetric controls. Where can I find
> > > the proper definitions and mappings?
> > >
> > >   ctl &= ~( ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_AYM);
> > >   if(tx_pause)
> > >     ctl |= ADVERTISE_PAUSE_CAP;
> > >   if(rx_pause)
> > >     ctl |= ADVERTISE_PAUSE_AYM;
> > 
> > lan9303_phylink_mac_link_up() has nothing to do with what might be
> > advertised to the link partner - this is called when the link has been
> > negotiated and established, and it's purpose is to program the results
> > of the resolution into the MAC.
> > 
> > That means programming the MAC to operate at the negotiated speed and
> > duplex, and also permitting the MAC to generate pause frames when its
> > receive side becomes full (tx_pause) and whether to act on pause frames
> > received over the network (rx_pause).
> > 
> > If there's nowhere to program the MAC to accept and/or generate pause
> > frames, how are they controlled? Does the MAC always accept and/or
> > generate them? Or does the MAC always ignore them and never generates
> > them?
> > 
> > If the latter, then that suggests pause frames are not supported, and
> > thus MAC_SYM_PAUSE and MAC_ASYM_PAUSE should not be set in the get_caps
> > method.
> > 
> > This leads me on to another question - in the above quoted code, what
> > device's BMCR is being accessed in lan9303_phylink_mac_link_up() ? Is
> > it a PCS? If it is, please use the phylink_pcs support, as the
> > pcs_config() method gives you what is necessary to program the PCS
> > advertisement.
> > 
> > Thanks.
> > 
> > --
> 
> On this device, the XMII connection is the rev-xmii port connected to the CPU.
> This is the DSA port. This device 'emulates' a phy (virtual phy) allowing the
> CPU to use standard phy registers to set things up.
> 
> Let me back up for a moment.
> The device supports half-duplex BackPressure as well as full-duplex Flow
> Control.
> The device has bootstrapping options that will configure the Settings for
> BP and FC. On port 0, these strapping options also affect the Virtual Phys
> Auto-Negotiation Link Partner Base Page Ability Register.
> If auto-negotiation is enabled, the flow control is enabled/disabled based
> on the Sym/Asym settings of the Advertised and Link Partner's capabilities
> registers.
> If Manual Flow Control is enabled, then flow control is programmed into the
> Manual_FC_0 register directly and the auto-neg registers are ignored. The
> device can be strapped to use (default to) the Manual FC register.
> 
> So this is why I'm trying to reflect the flow control settings as provided in
> the mac_link_up hook api into the emulated phy's aneg settings.

But it's wrong to be trying to do that.

The advertisement (in other words _our_ capabilities) should be
configured at one of the _config() stages - which includes the speed,
duplex and pause capabilities.

When the link partner wants to connect, the advertisement is exchanged
between each ends, and these advertisements are then used to determine
the final properties of the link. At this point, the link comes up,
and the link_up() methods will be called.

If, in the link_up() method, you want to change the advertisement, then
you would need to program that, and _then_ trigger a renegotiation of
the link, which would cause the link to go down. The above process would
be repeated, and ultimately link_up() would be called again. You'd then
reprogram the advertisement and trigger another renegotiation, and the
link would go down, up, down, up, down, up indefinitely.

> Question:  In the get capabilities API, should I report the device's
> flow control capabilities independent of how the device is bootstrapped or
> should I reflect the bootstrapped settings? I consider the bootstrap setting
> to affect the register default rather than limit what the device is physically
> capable of supporting.

I would suggest that the bootstrapping should in this case be reflected
in the MAC_*_PAUSE settings passed to phylink, so phylink knows how that
is configured and should end up with the same resolution as the
hardware. Things can go wrong if ethtool is then used to force manual
pause settings, but in such a case, you will be provided with the new
advertisement using the standard algorithm for determining the ASYM and
SYM bits that the kernel uses (which is not perfect, since it's
ambiguous.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!