[PATCH 0/3] Add support for QSGMII mode to am65-cpsw driver

[PATCH 0/3] Add support for QSGMII mode to am65-cpsw driver

[Siddharth Vadapalli]

Add support for QSGMII mode to am65-cpsw driver.

For full functionality of QSGMII mode in am65-cpsw driver, phy-gmii-sel
driver has to be configured. This has been implemented in another series
at:
https://lore.kernel.org/r/20220531111221.22963-1-s-vadapalli@ti.com

There is no direct dependency on the above series being merged.

Siddharth Vadapalli (3):
  dt-bindings: net: ti: k3-am654-cpsw-nuss: Update bindings for J7200
    CPSW5G
  net: ethernet: ti: am65-cpsw: Add support for QSGMII mode
  net: ethernet: ti: am65-cpsw: Move phy_set_mode_ext() to correct
    location

 .../bindings/net/ti,k3-am654-cpsw-nuss.yaml   |  4 ++--
 drivers/net/ethernet/ti/am65-cpsw-nuss.c      | 22 ++++++++++++++-----
 drivers/net/ethernet/ti/am65-cpsw-nuss.h      |  1 +
 3 files changed, 19 insertions(+), 8 deletions(-)

-- 
2.36.1

[PATCH 1/3] dt-bindings: net: ti: k3-am654-cpsw-nuss: Update bindings for J7200 CPSW5G

[Siddharth Vadapalli]

Update bindings for TI K3 J7200 SoC which contains 5 ports (4 external
ports) in order to support CPSW5G.

Changes made:
    - Change pattern properties to support 4 ports.
    - Change maximum number of CPSW ports to 4.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 .../devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml        | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
index b8281d8be940..f9e6eb600b41 100644
--- a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
+++ b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
@@ -110,7 +110,7 @@ properties:
         const: 0
 
     patternProperties:
-      port@[1-2]:
+      port@[1-4]:
         type: object
         description: CPSWxG NUSS external ports
 
@@ -119,7 +119,7 @@ properties:
         properties:
           reg:
             minimum: 1
-            maximum: 2
+            maximum: 4
             description: CPSW port number
 
           phys:
-- 
2.36.1

[PATCH 2/3] net: ethernet: ti: am65-cpsw: Add support for QSGMII mode

[Siddharth Vadapalli]

Enable QSGMII mode in am65-cpsw driver.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 13 ++++++++++++-
 drivers/net/ethernet/ti/am65-cpsw-nuss.h |  1 +
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 77bdda97b2b0..462f63313fb3 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -74,6 +74,9 @@
 #define AM65_CPSW_PORTN_REG_TS_VLAN_LTYPE_REG	0x318
 #define AM65_CPSW_PORTN_REG_TS_CTL_LTYPE2       0x31C
 
+#define AM65_CPSW_SGMII_CONTROL_REG		0x010
+#define AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE	BIT(0)
+
 #define AM65_CPSW_CTL_VLAN_AWARE		BIT(1)
 #define AM65_CPSW_CTL_P0_ENABLE			BIT(2)
 #define AM65_CPSW_CTL_P0_TX_CRC_REMOVE		BIT(13)
@@ -1409,7 +1412,13 @@ static const struct net_device_ops am65_cpsw_nuss_netdev_ops = {
 static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned int mode,
 				      const struct phylink_link_state *state)
 {
-	/* Currently not used */
+	struct am65_cpsw_slave_data *slave = container_of(config, struct am65_cpsw_slave_data,
+							  phylink_config);
+	struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave);
+
+	if (state->interface == PHY_INTERFACE_MODE_QSGMII)
+		writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE,
+		       port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG);
 }
 
 static void am65_cpsw_nuss_mac_link_down(struct phylink_config *config, unsigned int mode,
@@ -1846,6 +1855,7 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
 		port->common = common;
 		port->port_base = common->cpsw_base + AM65_CPSW_NU_PORTS_BASE +
 				  AM65_CPSW_NU_PORTS_OFFSET * (port_id);
+		port->sgmii_base = common->ss_base + AM65_CPSW_SGMII_BASE * (port_id);
 		port->stat_base = common->cpsw_base + AM65_CPSW_NU_STATS_BASE +
 				  (AM65_CPSW_NU_STATS_PORT_OFFSET * port_id);
 		port->name = of_get_property(port_np, "label", NULL);
@@ -1981,6 +1991,7 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
 	port->slave.phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000FD;
 
 	phy_interface_set_rgmii(port->slave.phylink_config.supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_QSGMII, port->slave.phylink_config.supported_interfaces);
 
 	phylink = phylink_create(&port->slave.phylink_config,
 				 of_node_to_fwnode(port->slave.phy_node),
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.h b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
index ac945631bf2f..8b6297e268ec 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.h
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
@@ -46,6 +46,7 @@ struct am65_cpsw_port {
 	const char			*name;
 	u32				port_id;
 	void __iomem			*port_base;
+	void __iomem			*sgmii_base;
 	void __iomem			*stat_base;
 	void __iomem			*fetch_ram_base;
 	bool				disabled;
-- 
2.36.1

Re: [PATCH 2/3] net: ethernet: ti: am65-cpsw: Add support for QSGMII mode

[Russell King (Oracle)]

On Tue, May 31, 2022 at 05:00:57PM +0530, Siddharth Vadapalli wrote:
>  static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned int mode,
>  				      const struct phylink_link_state *state)
>  {
> -	/* Currently not used */
> +	struct am65_cpsw_slave_data *slave = container_of(config, struct am65_cpsw_slave_data,
> +							  phylink_config);
> +	struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave);
> +
> +	if (state->interface == PHY_INTERFACE_MODE_QSGMII)
> +		writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE,
> +		       port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG);

What about writing this register when the interface mode isn't QSGMII?

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

Re: [PATCH 2/3] net: ethernet: ti: am65-cpsw: Add support for QSGMII mode

[Siddharth Vadapalli]

Hello Russell,

On 31/05/22 17:20, Russell King (Oracle) wrote:
> On Tue, May 31, 2022 at 05:00:57PM +0530, Siddharth Vadapalli wrote:
>>  static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned int mode,
>>  				      const struct phylink_link_state *state)
>>  {
>> -	/* Currently not used */
>> +	struct am65_cpsw_slave_data *slave = container_of(config, struct am65_cpsw_slave_data,
>> +							  phylink_config);
>> +	struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave);
>> +
>> +	if (state->interface == PHY_INTERFACE_MODE_QSGMII)
>> +		writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE,
>> +		       port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG);
> 
> What about writing this register when the interface mode isn't QSGMII?

In TI's J7200 device, there are two CPSW MACs namely CPSW2G and CPSW5G. While
CPSW5G supports QSGMII mode, CPSW2G does not. The same am65-cpsw-nuss driver is
used to control both CPSW2G and CPSW5G. Thus, the am65_cpsw_nuss_mac_config()
function is called for both CPSW2G and CPSW5G MACs. The SGMII CONTROL Register
is only present for CPSW5G. Thus, the write should only be performed for QSGMII
mode which is supported only by CPSW5G.

Functionally, always writing to the register even if the mode is not QSGMII does
not cause any problems, as long as it is CPSW5G MAC that is being used.

Thinking about it again, I will add a compatible to differentiate CPSW2G ports
from CPSW5G ports in order to make it cleaner, so that the register can always
be written to if the CPSW5G ports are used, irrespective of the interface mode.

Thanks,
Siddharth.

[PATCH 3/3] net: ethernet: ti: am65-cpsw: Move phy_set_mode_ext() to correct location

[Siddharth Vadapalli]

In TI's J7200 SoC CPSW5G ports, each of the 4 ports can be configured
as a QSGMII main or QSGMII-SUB port. This configuration is performed
by phy-gmii-sel driver on invoking the phy_set_mode_ext() function.

It is necessary for the QSGMII main port to be configured before any of
the QSGMII-SUB interfaces are brought up. Currently, the QSGMII-SUB
interfaces come up before the QSGMII main port is configured.

Fix this by moving the call to phy_set_mode_ext() from
am65_cpsw_nuss_ndo_slave_open() to am65_cpsw_nuss_init_slave_ports(),
thereby ensuring that the QSGMII main port is configured before any of
the QSGMII-SUB ports are brought up.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 462f63313fb3..c5ee636c4208 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -593,11 +593,6 @@ static int am65_cpsw_nuss_ndo_slave_open(struct net_device *ndev)
 	/* mac_sl should be configured via phy-link interface */
 	am65_cpsw_sl_ctl_reset(port);
 
-	ret = phy_set_mode_ext(port->slave.ifphy, PHY_MODE_ETHERNET,
-			       port->slave.phy_if);
-	if (ret)
-		goto error_cleanup;
-
 	ret = phylink_of_phy_connect(port->slave.phylink, port->slave.phy_node, 0);
 	if (ret)
 		goto error_cleanup;
@@ -1895,6 +1890,10 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
 			goto of_node_put;
 		}
 
+		ret = phy_set_mode_ext(port->slave.ifphy, PHY_MODE_ETHERNET, port->slave.phy_if);
+		if (ret)
+			goto of_node_put;
+
 		ret = of_get_mac_address(port_np, port->slave.mac_addr);
 		if (ret) {
 			am65_cpsw_am654_get_efuse_macid(port_np,
-- 
2.36.1

Re: [PATCH 3/3] net: ethernet: ti: am65-cpsw: Move phy_set_mode_ext() to correct location

[Russell King (Oracle)]

On Tue, May 31, 2022 at 05:00:58PM +0530, Siddharth Vadapalli wrote:
> In TI's J7200 SoC CPSW5G ports, each of the 4 ports can be configured
> as a QSGMII main or QSGMII-SUB port. This configuration is performed
> by phy-gmii-sel driver on invoking the phy_set_mode_ext() function.
> 
> It is necessary for the QSGMII main port to be configured before any of
> the QSGMII-SUB interfaces are brought up. Currently, the QSGMII-SUB
> interfaces come up before the QSGMII main port is configured.
> 
> Fix this by moving the call to phy_set_mode_ext() from
> am65_cpsw_nuss_ndo_slave_open() to am65_cpsw_nuss_init_slave_ports(),
> thereby ensuring that the QSGMII main port is configured before any of
> the QSGMII-SUB ports are brought up.

This sounds like "if we're configured via port->slave.phy_if to be in
QSGMII mode, then the serdes PHY needs to be configured before any of
the QSGMII ports are used". Doesn't that mean that if
port->slave.phy_if is QSGMII, then the port _only_ supports QSGMII
mode, and conversely, the port doesn't support QSGMII unless firmware
said it could be.

So, doesn't that mean am65_cpsw_nuss_init_port_ndev() should indicate
only QSGMII, or only the RGMII modes, but never both together?

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

Re: [PATCH 3/3] net: ethernet: ti: am65-cpsw: Move phy_set_mode_ext() to correct location

[Siddharth Vadapalli]

Hello Russell,

On 31/05/22 17:25, Russell King (Oracle) wrote:
> On Tue, May 31, 2022 at 05:00:58PM +0530, Siddharth Vadapalli wrote:
>> In TI's J7200 SoC CPSW5G ports, each of the 4 ports can be configured
>> as a QSGMII main or QSGMII-SUB port. This configuration is performed
>> by phy-gmii-sel driver on invoking the phy_set_mode_ext() function.
>>
>> It is necessary for the QSGMII main port to be configured before any of
>> the QSGMII-SUB interfaces are brought up. Currently, the QSGMII-SUB
>> interfaces come up before the QSGMII main port is configured.
>>
>> Fix this by moving the call to phy_set_mode_ext() from
>> am65_cpsw_nuss_ndo_slave_open() to am65_cpsw_nuss_init_slave_ports(),
>> thereby ensuring that the QSGMII main port is configured before any of
>> the QSGMII-SUB ports are brought up.
> 
> This sounds like "if we're configured via port->slave.phy_if to be in
> QSGMII mode, then the serdes PHY needs to be configured before any of
> the QSGMII ports are used". Doesn't that mean that if
> port->slave.phy_if is QSGMII, then the port _only_ supports QSGMII
> mode, and conversely, the port doesn't support QSGMII unless firmware
> said it could be.
> 
> So, doesn't that mean am65_cpsw_nuss_init_port_ndev() should indicate
> only QSGMII, or only the RGMII modes, but never both together?

The phy-gmii-sel driver called by phy_set_mode_ext() configures the CPSW5G MAC
rather than the SerDes Phy. In the CPSW5G MAC, the QSGMII mode is further split
up as two modes that are TI SoC specific, namely QSGMII main and QSGMII-SUB. Of
the 4 ports present in CPSW5G (4 external ports), only one can be the main port
while the rest are the QSGMII-SUB ports. Only the QSGMII main interface is
responsible for auto-negotiation between the MAC and PHY. For this reason, the
writes to the CPSW5G MAC, mentioning which of the interfaces is the QSGMII main
interface and which ones are the QSGMII-SUB interfaces has to be done before any
of the interfaces are brought up. Otherwise, it would result in a QSGMII-SUB
interface being brought up before the QSGMII main interface is determined,
resulting in the failure of auto-negotiation process, thereby making the
QSGMII-SUB interfaces non-functional.

Thanks,
Siddharth.

Re: [PATCH 3/3] net: ethernet: ti: am65-cpsw: Move phy_set_mode_ext() to correct location

[Russell King (Oracle)]

On Wed, Jun 01, 2022 at 11:39:57AM +0530, Siddharth Vadapalli wrote:
> Hello Russell,
> 
> On 31/05/22 17:25, Russell King (Oracle) wrote:
> > On Tue, May 31, 2022 at 05:00:58PM +0530, Siddharth Vadapalli wrote:
> >> In TI's J7200 SoC CPSW5G ports, each of the 4 ports can be configured
> >> as a QSGMII main or QSGMII-SUB port. This configuration is performed
> >> by phy-gmii-sel driver on invoking the phy_set_mode_ext() function.
> >>
> >> It is necessary for the QSGMII main port to be configured before any of
> >> the QSGMII-SUB interfaces are brought up. Currently, the QSGMII-SUB
> >> interfaces come up before the QSGMII main port is configured.
> >>
> >> Fix this by moving the call to phy_set_mode_ext() from
> >> am65_cpsw_nuss_ndo_slave_open() to am65_cpsw_nuss_init_slave_ports(),
> >> thereby ensuring that the QSGMII main port is configured before any of
> >> the QSGMII-SUB ports are brought up.
> > 
> > This sounds like "if we're configured via port->slave.phy_if to be in
> > QSGMII mode, then the serdes PHY needs to be configured before any of
> > the QSGMII ports are used". Doesn't that mean that if
> > port->slave.phy_if is QSGMII, then the port _only_ supports QSGMII
> > mode, and conversely, the port doesn't support QSGMII unless firmware
> > said it could be.
> > 
> > So, doesn't that mean am65_cpsw_nuss_init_port_ndev() should indicate
> > only QSGMII, or only the RGMII modes, but never both together?
> 
> The phy-gmii-sel driver called by phy_set_mode_ext() configures the CPSW5G MAC
> rather than the SerDes Phy. In the CPSW5G MAC, the QSGMII mode is further split
> up as two modes that are TI SoC specific, namely QSGMII main and QSGMII-SUB. Of
> the 4 ports present in CPSW5G (4 external ports), only one can be the main port
> while the rest are the QSGMII-SUB ports. Only the QSGMII main interface is
> responsible for auto-negotiation between the MAC and PHY. For this reason, the
> writes to the CPSW5G MAC, mentioning which of the interfaces is the QSGMII main
> interface and which ones are the QSGMII-SUB interfaces has to be done before any
> of the interfaces are brought up. Otherwise, it would result in a QSGMII-SUB
> interface being brought up before the QSGMII main interface is determined,
> resulting in the failure of auto-negotiation process, thereby making the
> QSGMII-SUB interfaces non-functional.

That confirms my suspicion - if an interface is in QSGMII mode, then
RGMII should not be marked as a supported interface to phylink. If the
"QSGMII main interface" were to be switched to RGMII mode, then this
would break the other ports. So RGMII isn't supported if in QSGMII
mode.

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

Re: [PATCH 3/3] net: ethernet: ti: am65-cpsw: Move phy_set_mode_ext() to correct location

[Siddharth Vadapalli]

Hello Russell,

On 01/06/22 13:59, Russell King (Oracle) wrote:
> On Wed, Jun 01, 2022 at 11:39:57AM +0530, Siddharth Vadapalli wrote:
>> Hello Russell,
>>
>> On 31/05/22 17:25, Russell King (Oracle) wrote:
>>> On Tue, May 31, 2022 at 05:00:58PM +0530, Siddharth Vadapalli wrote:
>>>> In TI's J7200 SoC CPSW5G ports, each of the 4 ports can be configured
>>>> as a QSGMII main or QSGMII-SUB port. This configuration is performed
>>>> by phy-gmii-sel driver on invoking the phy_set_mode_ext() function.
>>>>
>>>> It is necessary for the QSGMII main port to be configured before any of
>>>> the QSGMII-SUB interfaces are brought up. Currently, the QSGMII-SUB
>>>> interfaces come up before the QSGMII main port is configured.
>>>>
>>>> Fix this by moving the call to phy_set_mode_ext() from
>>>> am65_cpsw_nuss_ndo_slave_open() to am65_cpsw_nuss_init_slave_ports(),
>>>> thereby ensuring that the QSGMII main port is configured before any of
>>>> the QSGMII-SUB ports are brought up.
>>>
>>> This sounds like "if we're configured via port->slave.phy_if to be in
>>> QSGMII mode, then the serdes PHY needs to be configured before any of
>>> the QSGMII ports are used". Doesn't that mean that if
>>> port->slave.phy_if is QSGMII, then the port _only_ supports QSGMII
>>> mode, and conversely, the port doesn't support QSGMII unless firmware
>>> said it could be.
>>>
>>> So, doesn't that mean am65_cpsw_nuss_init_port_ndev() should indicate
>>> only QSGMII, or only the RGMII modes, but never both together?
>>
>> The phy-gmii-sel driver called by phy_set_mode_ext() configures the CPSW5G MAC
>> rather than the SerDes Phy. In the CPSW5G MAC, the QSGMII mode is further split
>> up as two modes that are TI SoC specific, namely QSGMII main and QSGMII-SUB. Of
>> the 4 ports present in CPSW5G (4 external ports), only one can be the main port
>> while the rest are the QSGMII-SUB ports. Only the QSGMII main interface is
>> responsible for auto-negotiation between the MAC and PHY. For this reason, the
>> writes to the CPSW5G MAC, mentioning which of the interfaces is the QSGMII main
>> interface and which ones are the QSGMII-SUB interfaces has to be done before any
>> of the interfaces are brought up. Otherwise, it would result in a QSGMII-SUB
>> interface being brought up before the QSGMII main interface is determined,
>> resulting in the failure of auto-negotiation process, thereby making the
>> QSGMII-SUB interfaces non-functional.
> 
> That confirms my suspicion - if an interface is in QSGMII mode, then
> RGMII should not be marked as a supported interface to phylink. If the

CPSW5G MAC supports both RGMII and QSGMII modes, so wouldn't it be correct to
mark both RGMII and QSGMII modes as supported? The mode is specified in the
device-tree and configured in CPSW5G MAC accordingly.

> "QSGMII main interface" were to be switched to RGMII mode, then this
> would break the other ports. So RGMII isn't supported if in QSGMII
> mode.

Yes, if the QSGMII main interface were to be switched to RGMII mode, then it
would break the other ports. However, the am65-cpsw driver currently has no
provision to dynamically change the port modes once the driver is initialized.

Thanks,
Siddharth.

Re: [PATCH 3/3] net: ethernet: ti: am65-cpsw: Move phy_set_mode_ext() to correct location

[Russell King (Oracle)]

On Wed, Jun 01, 2022 at 02:59:47PM +0530, Siddharth Vadapalli wrote:
> Hello Russell,
> 
> On 01/06/22 13:59, Russell King (Oracle) wrote:
> > On Wed, Jun 01, 2022 at 11:39:57AM +0530, Siddharth Vadapalli wrote:
> >> Hello Russell,
> >>
> >> On 31/05/22 17:25, Russell King (Oracle) wrote:
> >>> On Tue, May 31, 2022 at 05:00:58PM +0530, Siddharth Vadapalli wrote:
> >>>> In TI's J7200 SoC CPSW5G ports, each of the 4 ports can be configured
> >>>> as a QSGMII main or QSGMII-SUB port. This configuration is performed
> >>>> by phy-gmii-sel driver on invoking the phy_set_mode_ext() function.
> >>>>
> >>>> It is necessary for the QSGMII main port to be configured before any of
> >>>> the QSGMII-SUB interfaces are brought up. Currently, the QSGMII-SUB
> >>>> interfaces come up before the QSGMII main port is configured.
> >>>>
> >>>> Fix this by moving the call to phy_set_mode_ext() from
> >>>> am65_cpsw_nuss_ndo_slave_open() to am65_cpsw_nuss_init_slave_ports(),
> >>>> thereby ensuring that the QSGMII main port is configured before any of
> >>>> the QSGMII-SUB ports are brought up.
> >>>
> >>> This sounds like "if we're configured via port->slave.phy_if to be in
> >>> QSGMII mode, then the serdes PHY needs to be configured before any of
> >>> the QSGMII ports are used". Doesn't that mean that if
> >>> port->slave.phy_if is QSGMII, then the port _only_ supports QSGMII
> >>> mode, and conversely, the port doesn't support QSGMII unless firmware
> >>> said it could be.
> >>>
> >>> So, doesn't that mean am65_cpsw_nuss_init_port_ndev() should indicate
> >>> only QSGMII, or only the RGMII modes, but never both together?
> >>
> >> The phy-gmii-sel driver called by phy_set_mode_ext() configures the CPSW5G MAC
> >> rather than the SerDes Phy. In the CPSW5G MAC, the QSGMII mode is further split
> >> up as two modes that are TI SoC specific, namely QSGMII main and QSGMII-SUB. Of
> >> the 4 ports present in CPSW5G (4 external ports), only one can be the main port
> >> while the rest are the QSGMII-SUB ports. Only the QSGMII main interface is
> >> responsible for auto-negotiation between the MAC and PHY. For this reason, the
> >> writes to the CPSW5G MAC, mentioning which of the interfaces is the QSGMII main
> >> interface and which ones are the QSGMII-SUB interfaces has to be done before any
> >> of the interfaces are brought up. Otherwise, it would result in a QSGMII-SUB
> >> interface being brought up before the QSGMII main interface is determined,
> >> resulting in the failure of auto-negotiation process, thereby making the
> >> QSGMII-SUB interfaces non-functional.
> > 
> > That confirms my suspicion - if an interface is in QSGMII mode, then
> > RGMII should not be marked as a supported interface to phylink. If the
> 
> CPSW5G MAC supports both RGMII and QSGMII modes, so wouldn't it be correct to
> mark both RGMII and QSGMII modes as supported? The mode is specified in the
> device-tree and configured in CPSW5G MAC accordingly.
> 
> > "QSGMII main interface" were to be switched to RGMII mode, then this
> > would break the other ports. So RGMII isn't supported if in QSGMII
> > mode.
> 
> Yes, if the QSGMII main interface were to be switched to RGMII mode, then it
> would break the other ports. However, the am65-cpsw driver currently has no
> provision to dynamically change the port modes once the driver is initialized.

If there is no provision to change the port mode, then as far as
phylink is concerned, you should not advertise that it supports
anything but the current mode - because if phylink were to request
the driver change the mode, the driver can't do it.

So, you want there, at the very least:

	if (phy_interface_mode_is_rgmii(port->slave.phy_if))
		phy_interface_set_rgmii(port->slave.phylink_config.supported_interfaces);
	else
		__set_bit(PHY_INTERFACE_MODE_QSGMII, port->slave.phylink_config.supported_interfaces);

which will still ensure that port->slave.phy_if is either a RGMII
mode or QSGMII.

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

Re: [PATCH 3/3] net: ethernet: ti: am65-cpsw: Move phy_set_mode_ext() to correct location

[Siddharth Vadapalli]

Hello Russell,

On 01/06/22 15:25, Russell King (Oracle) wrote:
> On Wed, Jun 01, 2022 at 02:59:47PM +0530, Siddharth Vadapalli wrote:
>> Hello Russell,
>>
>> On 01/06/22 13:59, Russell King (Oracle) wrote:
>>> On Wed, Jun 01, 2022 at 11:39:57AM +0530, Siddharth Vadapalli wrote:
>>>> Hello Russell,
>>>>
>>>> On 31/05/22 17:25, Russell King (Oracle) wrote:
>>>>> On Tue, May 31, 2022 at 05:00:58PM +0530, Siddharth Vadapalli wrote:
>>>>>> In TI's J7200 SoC CPSW5G ports, each of the 4 ports can be configured
>>>>>> as a QSGMII main or QSGMII-SUB port. This configuration is performed
>>>>>> by phy-gmii-sel driver on invoking the phy_set_mode_ext() function.
>>>>>>
>>>>>> It is necessary for the QSGMII main port to be configured before any of
>>>>>> the QSGMII-SUB interfaces are brought up. Currently, the QSGMII-SUB
>>>>>> interfaces come up before the QSGMII main port is configured.
>>>>>>
>>>>>> Fix this by moving the call to phy_set_mode_ext() from
>>>>>> am65_cpsw_nuss_ndo_slave_open() to am65_cpsw_nuss_init_slave_ports(),
>>>>>> thereby ensuring that the QSGMII main port is configured before any of
>>>>>> the QSGMII-SUB ports are brought up.
>>>>>
>>>>> This sounds like "if we're configured via port->slave.phy_if to be in
>>>>> QSGMII mode, then the serdes PHY needs to be configured before any of
>>>>> the QSGMII ports are used". Doesn't that mean that if
>>>>> port->slave.phy_if is QSGMII, then the port _only_ supports QSGMII
>>>>> mode, and conversely, the port doesn't support QSGMII unless firmware
>>>>> said it could be.
>>>>>
>>>>> So, doesn't that mean am65_cpsw_nuss_init_port_ndev() should indicate
>>>>> only QSGMII, or only the RGMII modes, but never both together?
>>>>
>>>> The phy-gmii-sel driver called by phy_set_mode_ext() configures the CPSW5G MAC
>>>> rather than the SerDes Phy. In the CPSW5G MAC, the QSGMII mode is further split
>>>> up as two modes that are TI SoC specific, namely QSGMII main and QSGMII-SUB. Of
>>>> the 4 ports present in CPSW5G (4 external ports), only one can be the main port
>>>> while the rest are the QSGMII-SUB ports. Only the QSGMII main interface is
>>>> responsible for auto-negotiation between the MAC and PHY. For this reason, the
>>>> writes to the CPSW5G MAC, mentioning which of the interfaces is the QSGMII main
>>>> interface and which ones are the QSGMII-SUB interfaces has to be done before any
>>>> of the interfaces are brought up. Otherwise, it would result in a QSGMII-SUB
>>>> interface being brought up before the QSGMII main interface is determined,
>>>> resulting in the failure of auto-negotiation process, thereby making the
>>>> QSGMII-SUB interfaces non-functional.
>>>
>>> That confirms my suspicion - if an interface is in QSGMII mode, then
>>> RGMII should not be marked as a supported interface to phylink. If the
>>
>> CPSW5G MAC supports both RGMII and QSGMII modes, so wouldn't it be correct to
>> mark both RGMII and QSGMII modes as supported? The mode is specified in the
>> device-tree and configured in CPSW5G MAC accordingly.
>>
>>> "QSGMII main interface" were to be switched to RGMII mode, then this
>>> would break the other ports. So RGMII isn't supported if in QSGMII
>>> mode.
>>
>> Yes, if the QSGMII main interface were to be switched to RGMII mode, then it
>> would break the other ports. However, the am65-cpsw driver currently has no
>> provision to dynamically change the port modes once the driver is initialized.
> 
> If there is no provision to change the port mode, then as far as
> phylink is concerned, you should not advertise that it supports
> anything but the current mode - because if phylink were to request
> the driver change the mode, the driver can't do it.
> 
> So, you want there, at the very least:
> 
> 	if (phy_interface_mode_is_rgmii(port->slave.phy_if))
> 		phy_interface_set_rgmii(port->slave.phylink_config.supported_interfaces);
> 	else
> 		__set_bit(PHY_INTERFACE_MODE_QSGMII, port->slave.phylink_config.supported_interfaces);
> 
> which will still ensure that port->slave.phy_if is either a RGMII
> mode or QSGMII.

Thank you for reviewing the patch. I will send v2 for this series implementing
the fix suggested above.

Thanks,
Siddharth.