[PATCH net-next 0/2] Add support for J784S4 CPSW9G

[PATCH net-next 0/2] Add support for J784S4 CPSW9G

[Siddharth Vadapalli]

Hello,

This series adds a new compatible to am65-cpsw driver for the CPSW9G
instance of the CPSW Ethernet Switch on TI's J784S4 SoC which has 8
external ports and 1 internal host port.

The CPSW9G instance supports QSGMII and USXGMII modes for which driver
support is added.

Regards,
Siddharth.

Siddharth Vadapalli (2):
  net: ethernet: ti: am65-cpsw: Enable QSGMII for J784S4 CPSW9G
  net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH net-next 1/2] net: ethernet: ti: am65-cpsw: Enable QSGMII for J784S4 CPSW9G

[Siddharth Vadapalli]

TI's J784S4 SoC supports QSGMII mode with the CPSW9G instance of the
CPSW Ethernet Switch. Add a new compatible for J784S4 SoC and enable
QSGMII support for it by adding QSGMII mode to the extra_modes member of
the "j784s4_cpswxg_pdata" SoC data.

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

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index d17757ecbf42..4b4d06199b45 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2796,12 +2796,20 @@ static const struct am65_cpsw_pdata j721e_cpswxg_pdata = {
 	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII) | BIT(PHY_INTERFACE_MODE_SGMII),
 };
 
+static const struct am65_cpsw_pdata j784s4_cpswxg_pdata = {
+	.quirks = 0,
+	.ale_dev_id = "am64-cpswxg",
+	.fdqring_mode = K3_RINGACC_RING_MODE_MESSAGE,
+	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII),
+};
+
 static const struct of_device_id am65_cpsw_nuss_of_mtable[] = {
 	{ .compatible = "ti,am654-cpsw-nuss", .data = &am65x_sr1_0},
 	{ .compatible = "ti,j721e-cpsw-nuss", .data = &j721e_pdata},
 	{ .compatible = "ti,am642-cpsw-nuss", .data = &am64x_cpswxg_pdata},
 	{ .compatible = "ti,j7200-cpswxg-nuss", .data = &j7200_cpswxg_pdata},
 	{ .compatible = "ti,j721e-cpswxg-nuss", .data = &j721e_cpswxg_pdata},
+	{ .compatible = "ti,j784s4-cpswxg-nuss", .data = &j784s4_cpswxg_pdata},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, am65_cpsw_nuss_of_mtable);
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Siddharth Vadapalli]

TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
extra_modes member of the J784S4 SoC data. Additionally, configure the
MAC Control register for supporting USXGMII mode. Also, for USXGMII
mode, include MAC_5000FD in the "mac_capabilities" member of struct
"phylink_config".

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

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 4b4d06199b45..ab33e6fe5b1a 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
 		mac_control |= CPSW_SL_CTL_GIG;
 	if (interface == PHY_INTERFACE_MODE_SGMII)
 		mac_control |= CPSW_SL_CTL_EXT_EN;
+	if (interface == PHY_INTERFACE_MODE_USXGMII)
+		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
 	if (speed == SPEED_10 && phy_interface_mode_is_rgmii(interface))
 		/* Can be used with in band mode only */
 		mac_control |= CPSW_SL_CTL_EXT_EN;
@@ -2175,6 +2177,7 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
 
 	case PHY_INTERFACE_MODE_QSGMII:
 	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_USXGMII:
 		if (common->pdata.extra_modes & BIT(port->slave.phy_if)) {
 			__set_bit(port->slave.phy_if,
 				  port->slave.phylink_config.supported_interfaces);
@@ -2182,6 +2185,9 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
 			dev_err(dev, "selected phy-mode is not supported\n");
 			return -EOPNOTSUPP;
 		}
+		/* For USXGMII mode, enable MAC_5000FD */
+		if (port->slave.phy_if == PHY_INTERFACE_MODE_USXGMII)
+			port->slave.phylink_config.mac_capabilities |= MAC_5000FD;
 		break;
 
 	default:
@@ -2800,7 +2806,7 @@ static const struct am65_cpsw_pdata j784s4_cpswxg_pdata = {
 	.quirks = 0,
 	.ale_dev_id = "am64-cpswxg",
 	.fdqring_mode = K3_RINGACC_RING_MODE_MESSAGE,
-	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII),
+	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII) | BIT(PHY_INTERFACE_MODE_USXGMII),
 };
 
 static const struct of_device_id am65_cpsw_nuss_of_mtable[] = {
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Russell King (Oracle)]

On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
> extra_modes member of the J784S4 SoC data. Additionally, configure the
> MAC Control register for supporting USXGMII mode. Also, for USXGMII
> mode, include MAC_5000FD in the "mac_capabilities" member of struct
> "phylink_config".

I don't think TI "get" phylink at all...

> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index 4b4d06199b45..ab33e6fe5b1a 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
>  		mac_control |= CPSW_SL_CTL_GIG;
>  	if (interface == PHY_INTERFACE_MODE_SGMII)
>  		mac_control |= CPSW_SL_CTL_EXT_EN;
> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;

The configuration of the interface mode should *not* happen in
mac_link_up(), but should happen in e.g. mac_config().

>  	if (speed == SPEED_10 && phy_interface_mode_is_rgmii(interface))
>  		/* Can be used with in band mode only */
>  		mac_control |= CPSW_SL_CTL_EXT_EN;
> @@ -2175,6 +2177,7 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
>  
>  	case PHY_INTERFACE_MODE_QSGMII:
>  	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_USXGMII:
>  		if (common->pdata.extra_modes & BIT(port->slave.phy_if)) {
>  			__set_bit(port->slave.phy_if,
>  				  port->slave.phylink_config.supported_interfaces);
> @@ -2182,6 +2185,9 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
>  			dev_err(dev, "selected phy-mode is not supported\n");
>  			return -EOPNOTSUPP;
>  		}
> +		/* For USXGMII mode, enable MAC_5000FD */
> +		if (port->slave.phy_if == PHY_INTERFACE_MODE_USXGMII)
> +			port->slave.phylink_config.mac_capabilities |= MAC_5000FD;

MAC capabilities should not be conditional in the interface mode.
Phylink already knows the capabilities of each interface mode, and
will mask the mac_capabilities accordingly. Phylink wants to know
what speeds the MAC itself is capable of unbound by the interface
mode.

The interface modes that you already support (RGMII, RMII, QSGMII
and SGMII) do not support anything faster than 1G, so only
mac_capabilities up to and including 1G speeds will be permitted
for those interface modes internally by phylink.

So, making this conditional on USXGMII is just repeating logic that
is already present internally in phylink.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Siddharth Vadapalli]

Hello Russell,

Thank you for reviewing the patch.

On 31/03/23 13:27, Russell King (Oracle) wrote:
> On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
>> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
>> extra_modes member of the J784S4 SoC data. Additionally, configure the
>> MAC Control register for supporting USXGMII mode. Also, for USXGMII
>> mode, include MAC_5000FD in the "mac_capabilities" member of struct
>> "phylink_config".
> 
> I don't think TI "get" phylink at all...
> 
>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> index 4b4d06199b45..ab33e6fe5b1a 100644
>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
>>  		mac_control |= CPSW_SL_CTL_GIG;
>>  	if (interface == PHY_INTERFACE_MODE_SGMII)
>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
>> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
>> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
> 
> The configuration of the interface mode should *not* happen in
> mac_link_up(), but should happen in e.g. mac_config().

I will move all the interface mode associated configurations to mac_config() in
the v2 series.

> 
>>  	if (speed == SPEED_10 && phy_interface_mode_is_rgmii(interface))
>>  		/* Can be used with in band mode only */
>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
>> @@ -2175,6 +2177,7 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
>>  
>>  	case PHY_INTERFACE_MODE_QSGMII:
>>  	case PHY_INTERFACE_MODE_SGMII:
>> +	case PHY_INTERFACE_MODE_USXGMII:
>>  		if (common->pdata.extra_modes & BIT(port->slave.phy_if)) {
>>  			__set_bit(port->slave.phy_if,
>>  				  port->slave.phylink_config.supported_interfaces);
>> @@ -2182,6 +2185,9 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
>>  			dev_err(dev, "selected phy-mode is not supported\n");
>>  			return -EOPNOTSUPP;
>>  		}
>> +		/* For USXGMII mode, enable MAC_5000FD */
>> +		if (port->slave.phy_if == PHY_INTERFACE_MODE_USXGMII)
>> +			port->slave.phylink_config.mac_capabilities |= MAC_5000FD;
> 
> MAC capabilities should not be conditional in the interface mode.
> Phylink already knows the capabilities of each interface mode, and
> will mask the mac_capabilities accordingly. Phylink wants to know
> what speeds the MAC itself is capable of unbound by the interface
> mode.
> 
> The interface modes that you already support (RGMII, RMII, QSGMII
> and SGMII) do not support anything faster than 1G, so only
> mac_capabilities up to and including 1G speeds will be permitted
> for those interface modes internally by phylink.
> 
> So, making this conditional on USXGMII is just repeating logic that
> is already present internally in phylink.

Thank you for clarifying. I will fix this in the v2 series.

Regards,
Siddharth.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Russell King (Oracle)]

On Fri, Mar 31, 2023 at 01:35:10PM +0530, Siddharth Vadapalli wrote:
> Hello Russell,
> 
> Thank you for reviewing the patch.
> 
> On 31/03/23 13:27, Russell King (Oracle) wrote:
> > On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
> >> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
> >> extra_modes member of the J784S4 SoC data. Additionally, configure the
> >> MAC Control register for supporting USXGMII mode. Also, for USXGMII
> >> mode, include MAC_5000FD in the "mac_capabilities" member of struct
> >> "phylink_config".
> > 
> > I don't think TI "get" phylink at all...
> > 
> >> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >> index 4b4d06199b45..ab33e6fe5b1a 100644
> >> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
> >>  		mac_control |= CPSW_SL_CTL_GIG;
> >>  	if (interface == PHY_INTERFACE_MODE_SGMII)
> >>  		mac_control |= CPSW_SL_CTL_EXT_EN;
> >> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
> >> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
> > 
> > The configuration of the interface mode should *not* happen in
> > mac_link_up(), but should happen in e.g. mac_config().
> 
> I will move all the interface mode associated configurations to mac_config() in
> the v2 series.

Looking at the whole of mac_link_up(), could you please describe what
effect these bits are having:

	CPSW_SL_CTL_GIG
	CPSW_SL_CTL_EXT_EN
	CPSW_SL_CTL_IFCTL_A

Thanks.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Siddharth Vadapalli]

Russell,

On 31/03/23 13:54, Russell King (Oracle) wrote:
> On Fri, Mar 31, 2023 at 01:35:10PM +0530, Siddharth Vadapalli wrote:
>> Hello Russell,
>>
>> Thank you for reviewing the patch.
>>
>> On 31/03/23 13:27, Russell King (Oracle) wrote:
>>> On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
>>>> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
>>>> extra_modes member of the J784S4 SoC data. Additionally, configure the
>>>> MAC Control register for supporting USXGMII mode. Also, for USXGMII
>>>> mode, include MAC_5000FD in the "mac_capabilities" member of struct
>>>> "phylink_config".
>>>
>>> I don't think TI "get" phylink at all...
>>>
>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>> index 4b4d06199b45..ab33e6fe5b1a 100644
>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
>>>>  		mac_control |= CPSW_SL_CTL_GIG;
>>>>  	if (interface == PHY_INTERFACE_MODE_SGMII)
>>>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
>>>> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
>>>> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
>>>
>>> The configuration of the interface mode should *not* happen in
>>> mac_link_up(), but should happen in e.g. mac_config().
>>
>> I will move all the interface mode associated configurations to mac_config() in
>> the v2 series.
> 
> Looking at the whole of mac_link_up(), could you please describe what
> effect these bits are having:
> 
> 	CPSW_SL_CTL_GIG
> 	CPSW_SL_CTL_EXT_EN
> 	CPSW_SL_CTL_IFCTL_A

CPSW_SL_CTL_GIG corresponds to enabling Gigabit mode (full duplex only).
CPSW_SL_CTL_EXT_EN when set enables in-band mode of operation and when cleared
enables forced mode of operation.
CPSW_SL_CTL_IFCTL_A is used to set the RMII link speed (0=10 mbps, 1=100 mbps).

Regards,
Siddharth.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Russell King (Oracle)]

On Fri, Mar 31, 2023 at 02:55:56PM +0530, Siddharth Vadapalli wrote:
> Russell,
> 
> On 31/03/23 13:54, Russell King (Oracle) wrote:
> > On Fri, Mar 31, 2023 at 01:35:10PM +0530, Siddharth Vadapalli wrote:
> >> Hello Russell,
> >>
> >> Thank you for reviewing the patch.
> >>
> >> On 31/03/23 13:27, Russell King (Oracle) wrote:
> >>> On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
> >>>> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
> >>>> extra_modes member of the J784S4 SoC data. Additionally, configure the
> >>>> MAC Control register for supporting USXGMII mode. Also, for USXGMII
> >>>> mode, include MAC_5000FD in the "mac_capabilities" member of struct
> >>>> "phylink_config".
> >>>
> >>> I don't think TI "get" phylink at all...
> >>>
> >>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>> index 4b4d06199b45..ab33e6fe5b1a 100644
> >>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
> >>>>  		mac_control |= CPSW_SL_CTL_GIG;
> >>>>  	if (interface == PHY_INTERFACE_MODE_SGMII)
> >>>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
> >>>> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
> >>>> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
> >>>
> >>> The configuration of the interface mode should *not* happen in
> >>> mac_link_up(), but should happen in e.g. mac_config().
> >>
> >> I will move all the interface mode associated configurations to mac_config() in
> >> the v2 series.
> > 
> > Looking at the whole of mac_link_up(), could you please describe what
> > effect these bits are having:
> > 
> > 	CPSW_SL_CTL_GIG
> > 	CPSW_SL_CTL_EXT_EN
> > 	CPSW_SL_CTL_IFCTL_A
> 
> CPSW_SL_CTL_GIG corresponds to enabling Gigabit mode (full duplex only).
> CPSW_SL_CTL_EXT_EN when set enables in-band mode of operation and when cleared
> enables forced mode of operation.
> CPSW_SL_CTL_IFCTL_A is used to set the RMII link speed (0=10 mbps, 1=100 mbps).

Okay, so I would do in mac_link_up():

	/* RMII needs to be manually configured for 10/100Mbps */
	if (interface == PHY_INTERFACE_MODE_RMII && speed == SPEED_100)
		mac_control |= CPSW_SL_CTL_IFCTL_A;

	if (speed == SPEED_1000)
		mac_control |= CPSW_SL_CTL_GIG;
	if (duplex)
		mac_control |= CPSW_SL_CTL_FULLDUPLEX;

I would also make mac_link_up() do a read-modify-write operation to
only affect the bits that it is changing.

Now, for SGMII, I would move setting CPSW_SL_CTL_EXT_EN to mac_config()
to enable in-band mode - don't we want in-band mode enabled all the
time while in SGMII mode so the PHY gets the response from the MAC?

Lastly, for RGMII at 10Mbps, you seem to suggest that you need RGMII
in-band mode enabled for that - but if you need RGMII in-band for
10Mbps, wouldn't it make sense for the other speeds as well? If so,
wouldn't that mean that CPSW_SL_CTL_EXT_EN can always be set for
RGMII no matter what speed is being used?

Thanks.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Siddharth Vadapalli]

On 31/03/23 15:16, Russell King (Oracle) wrote:
> On Fri, Mar 31, 2023 at 02:55:56PM +0530, Siddharth Vadapalli wrote:
>> Russell,
>>
>> On 31/03/23 13:54, Russell King (Oracle) wrote:
>>> On Fri, Mar 31, 2023 at 01:35:10PM +0530, Siddharth Vadapalli wrote:
>>>> Hello Russell,
>>>>
>>>> Thank you for reviewing the patch.
>>>>
>>>> On 31/03/23 13:27, Russell King (Oracle) wrote:
>>>>> On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
>>>>>> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
>>>>>> extra_modes member of the J784S4 SoC data. Additionally, configure the
>>>>>> MAC Control register for supporting USXGMII mode. Also, for USXGMII
>>>>>> mode, include MAC_5000FD in the "mac_capabilities" member of struct
>>>>>> "phylink_config".
>>>>>
>>>>> I don't think TI "get" phylink at all...
>>>>>
>>>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>> index 4b4d06199b45..ab33e6fe5b1a 100644
>>>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
>>>>>>  		mac_control |= CPSW_SL_CTL_GIG;
>>>>>>  	if (interface == PHY_INTERFACE_MODE_SGMII)
>>>>>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
>>>>>> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
>>>>>> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
>>>>>
>>>>> The configuration of the interface mode should *not* happen in
>>>>> mac_link_up(), but should happen in e.g. mac_config().
>>>>
>>>> I will move all the interface mode associated configurations to mac_config() in
>>>> the v2 series.
>>>
>>> Looking at the whole of mac_link_up(), could you please describe what
>>> effect these bits are having:
>>>
>>> 	CPSW_SL_CTL_GIG
>>> 	CPSW_SL_CTL_EXT_EN
>>> 	CPSW_SL_CTL_IFCTL_A
>>
>> CPSW_SL_CTL_GIG corresponds to enabling Gigabit mode (full duplex only).
>> CPSW_SL_CTL_EXT_EN when set enables in-band mode of operation and when cleared
>> enables forced mode of operation.
>> CPSW_SL_CTL_IFCTL_A is used to set the RMII link speed (0=10 mbps, 1=100 mbps).
> 
> Okay, so I would do in mac_link_up():
> 
> 	/* RMII needs to be manually configured for 10/100Mbps */
> 	if (interface == PHY_INTERFACE_MODE_RMII && speed == SPEED_100)
> 		mac_control |= CPSW_SL_CTL_IFCTL_A;
> 
> 	if (speed == SPEED_1000)
> 		mac_control |= CPSW_SL_CTL_GIG;
> 	if (duplex)
> 		mac_control |= CPSW_SL_CTL_FULLDUPLEX;
> 
> I would also make mac_link_up() do a read-modify-write operation to
> only affect the bits that it is changing.

This is the current implementation except for the SGMII mode associated
operation that I had recently added. I will fix that. Also, the
cpsw_sl_ctl_set() function which writes the mac_control value performs a read
modify write operation.

> 
> Now, for SGMII, I would move setting CPSW_SL_CTL_EXT_EN to mac_config()
> to enable in-band mode - don't we want in-band mode enabled all the
> time while in SGMII mode so the PHY gets the response from the MAC?

Thank you for pointing it out. I will move that to mac_config().

> 
> Lastly, for RGMII at 10Mbps, you seem to suggest that you need RGMII
> in-band mode enabled for that - but if you need RGMII in-band for
> 10Mbps, wouldn't it make sense for the other speeds as well? If so,
> wouldn't that mean that CPSW_SL_CTL_EXT_EN can always be set for
> RGMII no matter what speed is being used?

The CPSW MAC does not support forced mode at 10 Mbps RGMII. For this reason, if
RGMII 10 Mbps is requested, it is set to in-band mode.

Regards,
Siddharth.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Russell King (Oracle)]

On Fri, Mar 31, 2023 at 04:23:16PM +0530, Siddharth Vadapalli wrote:
> 
> 
> On 31/03/23 15:16, Russell King (Oracle) wrote:
> > On Fri, Mar 31, 2023 at 02:55:56PM +0530, Siddharth Vadapalli wrote:
> >> Russell,
> >>
> >> On 31/03/23 13:54, Russell King (Oracle) wrote:
> >>> On Fri, Mar 31, 2023 at 01:35:10PM +0530, Siddharth Vadapalli wrote:
> >>>> Hello Russell,
> >>>>
> >>>> Thank you for reviewing the patch.
> >>>>
> >>>> On 31/03/23 13:27, Russell King (Oracle) wrote:
> >>>>> On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
> >>>>>> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
> >>>>>> extra_modes member of the J784S4 SoC data. Additionally, configure the
> >>>>>> MAC Control register for supporting USXGMII mode. Also, for USXGMII
> >>>>>> mode, include MAC_5000FD in the "mac_capabilities" member of struct
> >>>>>> "phylink_config".
> >>>>>
> >>>>> I don't think TI "get" phylink at all...
> >>>>>
> >>>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>>>> index 4b4d06199b45..ab33e6fe5b1a 100644
> >>>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>>>> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
> >>>>>>  		mac_control |= CPSW_SL_CTL_GIG;
> >>>>>>  	if (interface == PHY_INTERFACE_MODE_SGMII)
> >>>>>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
> >>>>>> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
> >>>>>> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
> >>>>>
> >>>>> The configuration of the interface mode should *not* happen in
> >>>>> mac_link_up(), but should happen in e.g. mac_config().
> >>>>
> >>>> I will move all the interface mode associated configurations to mac_config() in
> >>>> the v2 series.
> >>>
> >>> Looking at the whole of mac_link_up(), could you please describe what
> >>> effect these bits are having:
> >>>
> >>> 	CPSW_SL_CTL_GIG
> >>> 	CPSW_SL_CTL_EXT_EN
> >>> 	CPSW_SL_CTL_IFCTL_A
> >>
> >> CPSW_SL_CTL_GIG corresponds to enabling Gigabit mode (full duplex only).
> >> CPSW_SL_CTL_EXT_EN when set enables in-band mode of operation and when cleared
> >> enables forced mode of operation.
> >> CPSW_SL_CTL_IFCTL_A is used to set the RMII link speed (0=10 mbps, 1=100 mbps).
> > 
> > Okay, so I would do in mac_link_up():
> > 
> > 	/* RMII needs to be manually configured for 10/100Mbps */
> > 	if (interface == PHY_INTERFACE_MODE_RMII && speed == SPEED_100)
> > 		mac_control |= CPSW_SL_CTL_IFCTL_A;
> > 
> > 	if (speed == SPEED_1000)
> > 		mac_control |= CPSW_SL_CTL_GIG;
> > 	if (duplex)
> > 		mac_control |= CPSW_SL_CTL_FULLDUPLEX;
> > 
> > I would also make mac_link_up() do a read-modify-write operation to
> > only affect the bits that it is changing.
> 
> This is the current implementation except for the SGMII mode associated
> operation that I had recently added. I will fix that. Also, the
> cpsw_sl_ctl_set() function which writes the mac_control value performs a read
> modify write operation.
> 
> > 
> > Now, for SGMII, I would move setting CPSW_SL_CTL_EXT_EN to mac_config()
> > to enable in-band mode - don't we want in-band mode enabled all the
> > time while in SGMII mode so the PHY gets the response from the MAC?
> 
> Thank you for pointing it out. I will move that to mac_config().
> 
> > 
> > Lastly, for RGMII at 10Mbps, you seem to suggest that you need RGMII
> > in-band mode enabled for that - but if you need RGMII in-band for
> > 10Mbps, wouldn't it make sense for the other speeds as well? If so,
> > wouldn't that mean that CPSW_SL_CTL_EXT_EN can always be set for
> > RGMII no matter what speed is being used?
> 
> The CPSW MAC does not support forced mode at 10 Mbps RGMII. For this reason, if
> RGMII 10 Mbps is requested, it is set to in-band mode.

What I'm saying is that if we have in-band signalling that is reliable
for a particular interface mode, why not always use it, rather than
singling out one specific speed as an exception? Does it not work in
100Mbps and 1Gbps?

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Siddharth Vadapalli]

On 31-03-2023 16:42, Russell King (Oracle) wrote:
> On Fri, Mar 31, 2023 at 04:23:16PM +0530, Siddharth Vadapalli wrote:
>>
>>
>> On 31/03/23 15:16, Russell King (Oracle) wrote:
>>> On Fri, Mar 31, 2023 at 02:55:56PM +0530, Siddharth Vadapalli wrote:
>>>> Russell,
>>>>
>>>> On 31/03/23 13:54, Russell King (Oracle) wrote:
>>>>> On Fri, Mar 31, 2023 at 01:35:10PM +0530, Siddharth Vadapalli wrote:
>>>>>> Hello Russell,
>>>>>>
>>>>>> Thank you for reviewing the patch.
>>>>>>
>>>>>> On 31/03/23 13:27, Russell King (Oracle) wrote:
>>>>>>> On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
>>>>>>>> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
>>>>>>>> extra_modes member of the J784S4 SoC data. Additionally, configure the
>>>>>>>> MAC Control register for supporting USXGMII mode. Also, for USXGMII
>>>>>>>> mode, include MAC_5000FD in the "mac_capabilities" member of struct
>>>>>>>> "phylink_config".
>>>>>>>
>>>>>>> I don't think TI "get" phylink at all...
>>>>>>>
>>>>>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>>>> index 4b4d06199b45..ab33e6fe5b1a 100644
>>>>>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>>>> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
>>>>>>>>  		mac_control |= CPSW_SL_CTL_GIG;
>>>>>>>>  	if (interface == PHY_INTERFACE_MODE_SGMII)
>>>>>>>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
>>>>>>>> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
>>>>>>>> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
>>>>>>>
>>>>>>> The configuration of the interface mode should *not* happen in
>>>>>>> mac_link_up(), but should happen in e.g. mac_config().
>>>>>>
>>>>>> I will move all the interface mode associated configurations to mac_config() in
>>>>>> the v2 series.
>>>>>
>>>>> Looking at the whole of mac_link_up(), could you please describe what
>>>>> effect these bits are having:
>>>>>
>>>>> 	CPSW_SL_CTL_GIG
>>>>> 	CPSW_SL_CTL_EXT_EN
>>>>> 	CPSW_SL_CTL_IFCTL_A
>>>>
>>>> CPSW_SL_CTL_GIG corresponds to enabling Gigabit mode (full duplex only).
>>>> CPSW_SL_CTL_EXT_EN when set enables in-band mode of operation and when cleared
>>>> enables forced mode of operation.
>>>> CPSW_SL_CTL_IFCTL_A is used to set the RMII link speed (0=10 mbps, 1=100 mbps).
>>>
>>> Okay, so I would do in mac_link_up():
>>>
>>> 	/* RMII needs to be manually configured for 10/100Mbps */
>>> 	if (interface == PHY_INTERFACE_MODE_RMII && speed == SPEED_100)
>>> 		mac_control |= CPSW_SL_CTL_IFCTL_A;
>>>
>>> 	if (speed == SPEED_1000)
>>> 		mac_control |= CPSW_SL_CTL_GIG;
>>> 	if (duplex)
>>> 		mac_control |= CPSW_SL_CTL_FULLDUPLEX;
>>>
>>> I would also make mac_link_up() do a read-modify-write operation to
>>> only affect the bits that it is changing.
>>
>> This is the current implementation except for the SGMII mode associated
>> operation that I had recently added. I will fix that. Also, the
>> cpsw_sl_ctl_set() function which writes the mac_control value performs a read
>> modify write operation.
>>
>>>
>>> Now, for SGMII, I would move setting CPSW_SL_CTL_EXT_EN to mac_config()
>>> to enable in-band mode - don't we want in-band mode enabled all the
>>> time while in SGMII mode so the PHY gets the response from the MAC?
>>
>> Thank you for pointing it out. I will move that to mac_config().
>>
>>>
>>> Lastly, for RGMII at 10Mbps, you seem to suggest that you need RGMII
>>> in-band mode enabled for that - but if you need RGMII in-band for
>>> 10Mbps, wouldn't it make sense for the other speeds as well? If so,
>>> wouldn't that mean that CPSW_SL_CTL_EXT_EN can always be set for
>>> RGMII no matter what speed is being used?
>>
>> The CPSW MAC does not support forced mode at 10 Mbps RGMII. For this reason, if
>> RGMII 10 Mbps is requested, it is set to in-band mode.
> 
> What I'm saying is that if we have in-band signalling that is reliable
> for a particular interface mode, why not always use it, rather than
> singling out one specific speed as an exception? Does it not work in
> 100Mbps and 1Gbps?

In-band RGMII is supported for speeds of 10, 100 and 1000 Mbps.
Unfortunately, I am not aware of the reason why RGMII at speeds 100 and
1000 Mbps was implemented in the driver in forced mode. As suggested by
you, I will work on implementing it in in-band mode for all speeds and
verify that it works, following which I will post the v2 of this series,
with the following changes based on your feedback:
1. All interface mode specific configuration will be moved to mac_config().
2. Since CPSW MAC supports USXGMII mode, MAC_5000FD will be added to the
list of mac_capabilites unconditionally, unlike the current implementation.
3. In-band mode of operation will be enabled for all interface modes by
default.

Regards,
Siddharth.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Siddharth Vadapalli]

Hello Russell,

On 31/03/23 19:16, Siddharth Vadapalli wrote:
> 
> 
> On 31-03-2023 16:42, Russell King (Oracle) wrote:
>> On Fri, Mar 31, 2023 at 04:23:16PM +0530, Siddharth Vadapalli wrote:
>>>
>>>
>>> On 31/03/23 15:16, Russell King (Oracle) wrote:
>>>> On Fri, Mar 31, 2023 at 02:55:56PM +0530, Siddharth Vadapalli wrote:
>>>>> Russell,
>>>>>
>>>>> On 31/03/23 13:54, Russell King (Oracle) wrote:
>>>>>> On Fri, Mar 31, 2023 at 01:35:10PM +0530, Siddharth Vadapalli wrote:
>>>>>>> Hello Russell,
>>>>>>>
>>>>>>> Thank you for reviewing the patch.
>>>>>>>
>>>>>>> On 31/03/23 13:27, Russell King (Oracle) wrote:
>>>>>>>> On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
>>>>>>>>> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
>>>>>>>>> extra_modes member of the J784S4 SoC data. Additionally, configure the
>>>>>>>>> MAC Control register for supporting USXGMII mode. Also, for USXGMII
>>>>>>>>> mode, include MAC_5000FD in the "mac_capabilities" member of struct
>>>>>>>>> "phylink_config".
>>>>>>>>
>>>>>>>> I don't think TI "get" phylink at all...
>>>>>>>>
>>>>>>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>>>>> index 4b4d06199b45..ab33e6fe5b1a 100644
>>>>>>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>>>>> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
>>>>>>>>>  		mac_control |= CPSW_SL_CTL_GIG;
>>>>>>>>>  	if (interface == PHY_INTERFACE_MODE_SGMII)
>>>>>>>>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
>>>>>>>>> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
>>>>>>>>> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
>>>>>>>>
>>>>>>>> The configuration of the interface mode should *not* happen in
>>>>>>>> mac_link_up(), but should happen in e.g. mac_config().
>>>>>>>
>>>>>>> I will move all the interface mode associated configurations to mac_config() in
>>>>>>> the v2 series.
>>>>>>
>>>>>> Looking at the whole of mac_link_up(), could you please describe what
>>>>>> effect these bits are having:
>>>>>>
>>>>>> 	CPSW_SL_CTL_GIG
>>>>>> 	CPSW_SL_CTL_EXT_EN
>>>>>> 	CPSW_SL_CTL_IFCTL_A
>>>>>
>>>>> CPSW_SL_CTL_GIG corresponds to enabling Gigabit mode (full duplex only).
>>>>> CPSW_SL_CTL_EXT_EN when set enables in-band mode of operation and when cleared
>>>>> enables forced mode of operation.
>>>>> CPSW_SL_CTL_IFCTL_A is used to set the RMII link speed (0=10 mbps, 1=100 mbps).
>>>>
>>>> Okay, so I would do in mac_link_up():
>>>>
>>>> 	/* RMII needs to be manually configured for 10/100Mbps */
>>>> 	if (interface == PHY_INTERFACE_MODE_RMII && speed == SPEED_100)
>>>> 		mac_control |= CPSW_SL_CTL_IFCTL_A;
>>>>
>>>> 	if (speed == SPEED_1000)
>>>> 		mac_control |= CPSW_SL_CTL_GIG;
>>>> 	if (duplex)
>>>> 		mac_control |= CPSW_SL_CTL_FULLDUPLEX;
>>>>
>>>> I would also make mac_link_up() do a read-modify-write operation to
>>>> only affect the bits that it is changing.
>>>
>>> This is the current implementation except for the SGMII mode associated
>>> operation that I had recently added. I will fix that. Also, the
>>> cpsw_sl_ctl_set() function which writes the mac_control value performs a read
>>> modify write operation.
>>>
>>>>
>>>> Now, for SGMII, I would move setting CPSW_SL_CTL_EXT_EN to mac_config()
>>>> to enable in-band mode - don't we want in-band mode enabled all the
>>>> time while in SGMII mode so the PHY gets the response from the MAC?
>>>
>>> Thank you for pointing it out. I will move that to mac_config().
>>>
>>>>
>>>> Lastly, for RGMII at 10Mbps, you seem to suggest that you need RGMII
>>>> in-band mode enabled for that - but if you need RGMII in-band for
>>>> 10Mbps, wouldn't it make sense for the other speeds as well? If so,
>>>> wouldn't that mean that CPSW_SL_CTL_EXT_EN can always be set for
>>>> RGMII no matter what speed is being used?
>>>
>>> The CPSW MAC does not support forced mode at 10 Mbps RGMII. For this reason, if
>>> RGMII 10 Mbps is requested, it is set to in-band mode.
>>
>> What I'm saying is that if we have in-band signalling that is reliable
>> for a particular interface mode, why not always use it, rather than
>> singling out one specific speed as an exception? Does it not work in
>> 100Mbps and 1Gbps?

While the CPSW MAC supports RGMII in-band status operation, the link partner
might not support it. I have also observed that forced mode is preferred to
in-band mode as implemented for another driver:
commit ade64eb5be9768e40c90ecb01295416abb2ddbac
net: dsa: microchip: Disable RGMII in-band status on KSZ9893

and in the mail thread at:
https://lore.kernel.org/netdev/20200905160647.GJ3164319@lunn.ch/
based on Andrew's suggestion, using forced mode appears to be better.

Additionally, I have verified that switching to in-band status causes a
regression. Thus, I will prefer keeping it in forced mode for 100 and 1000 Mbps
RGMII mode which is the existing implementation in the driver. Please let me know.

Regards,
Siddharth.

> 
> In-band RGMII is supported for speeds of 10, 100 and 1000 Mbps.
> Unfortunately, I am not aware of the reason why RGMII at speeds 100 and
> 1000 Mbps was implemented in the driver in forced mode. As suggested by
> you, I will work on implementing it in in-band mode for all speeds and
> verify that it works, following which I will post the v2 of this series,
> with the following changes based on your feedback:
> 1. All interface mode specific configuration will be moved to mac_config().
> 2. Since CPSW MAC supports USXGMII mode, MAC_5000FD will be added to the
> list of mac_capabilites unconditionally, unlike the current implementation.
> 3. In-band mode of operation will be enabled for all interface modes by
> default.
> 
> Regards,
> Siddharth.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Russell King (Oracle)]

On Mon, Apr 03, 2023 at 11:57:21AM +0530, Siddharth Vadapalli wrote:
> Hello Russell,
> 
> On 31/03/23 19:16, Siddharth Vadapalli wrote:
> > 
> > 
> > On 31-03-2023 16:42, Russell King (Oracle) wrote:
> >> On Fri, Mar 31, 2023 at 04:23:16PM +0530, Siddharth Vadapalli wrote:
> >>>
> >>>
> >>> On 31/03/23 15:16, Russell King (Oracle) wrote:
> >>>> On Fri, Mar 31, 2023 at 02:55:56PM +0530, Siddharth Vadapalli wrote:
> >>>>> Russell,
> >>>>>
> >>>>> On 31/03/23 13:54, Russell King (Oracle) wrote:
> >>>>>> On Fri, Mar 31, 2023 at 01:35:10PM +0530, Siddharth Vadapalli wrote:
> >>>>>>> Hello Russell,
> >>>>>>>
> >>>>>>> Thank you for reviewing the patch.
> >>>>>>>
> >>>>>>> On 31/03/23 13:27, Russell King (Oracle) wrote:
> >>>>>>>> On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
> >>>>>>>>> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
> >>>>>>>>> extra_modes member of the J784S4 SoC data. Additionally, configure the
> >>>>>>>>> MAC Control register for supporting USXGMII mode. Also, for USXGMII
> >>>>>>>>> mode, include MAC_5000FD in the "mac_capabilities" member of struct
> >>>>>>>>> "phylink_config".
> >>>>>>>>
> >>>>>>>> I don't think TI "get" phylink at all...
> >>>>>>>>
> >>>>>>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>>>>>>> index 4b4d06199b45..ab33e6fe5b1a 100644
> >>>>>>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>>>>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>>>>>>> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
> >>>>>>>>>  		mac_control |= CPSW_SL_CTL_GIG;
> >>>>>>>>>  	if (interface == PHY_INTERFACE_MODE_SGMII)
> >>>>>>>>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
> >>>>>>>>> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
> >>>>>>>>> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
> >>>>>>>>
> >>>>>>>> The configuration of the interface mode should *not* happen in
> >>>>>>>> mac_link_up(), but should happen in e.g. mac_config().
> >>>>>>>
> >>>>>>> I will move all the interface mode associated configurations to mac_config() in
> >>>>>>> the v2 series.
> >>>>>>
> >>>>>> Looking at the whole of mac_link_up(), could you please describe what
> >>>>>> effect these bits are having:
> >>>>>>
> >>>>>> 	CPSW_SL_CTL_GIG
> >>>>>> 	CPSW_SL_CTL_EXT_EN
> >>>>>> 	CPSW_SL_CTL_IFCTL_A
> >>>>>
> >>>>> CPSW_SL_CTL_GIG corresponds to enabling Gigabit mode (full duplex only).
> >>>>> CPSW_SL_CTL_EXT_EN when set enables in-band mode of operation and when cleared
> >>>>> enables forced mode of operation.
> >>>>> CPSW_SL_CTL_IFCTL_A is used to set the RMII link speed (0=10 mbps, 1=100 mbps).
> >>>>
> >>>> Okay, so I would do in mac_link_up():
> >>>>
> >>>> 	/* RMII needs to be manually configured for 10/100Mbps */
> >>>> 	if (interface == PHY_INTERFACE_MODE_RMII && speed == SPEED_100)
> >>>> 		mac_control |= CPSW_SL_CTL_IFCTL_A;
> >>>>
> >>>> 	if (speed == SPEED_1000)
> >>>> 		mac_control |= CPSW_SL_CTL_GIG;
> >>>> 	if (duplex)
> >>>> 		mac_control |= CPSW_SL_CTL_FULLDUPLEX;
> >>>>
> >>>> I would also make mac_link_up() do a read-modify-write operation to
> >>>> only affect the bits that it is changing.
> >>>
> >>> This is the current implementation except for the SGMII mode associated
> >>> operation that I had recently added. I will fix that. Also, the
> >>> cpsw_sl_ctl_set() function which writes the mac_control value performs a read
> >>> modify write operation.
> >>>
> >>>>
> >>>> Now, for SGMII, I would move setting CPSW_SL_CTL_EXT_EN to mac_config()
> >>>> to enable in-band mode - don't we want in-band mode enabled all the
> >>>> time while in SGMII mode so the PHY gets the response from the MAC?
> >>>
> >>> Thank you for pointing it out. I will move that to mac_config().
> >>>
> >>>>
> >>>> Lastly, for RGMII at 10Mbps, you seem to suggest that you need RGMII
> >>>> in-band mode enabled for that - but if you need RGMII in-band for
> >>>> 10Mbps, wouldn't it make sense for the other speeds as well? If so,
> >>>> wouldn't that mean that CPSW_SL_CTL_EXT_EN can always be set for
> >>>> RGMII no matter what speed is being used?
> >>>
> >>> The CPSW MAC does not support forced mode at 10 Mbps RGMII. For this reason, if
> >>> RGMII 10 Mbps is requested, it is set to in-band mode.
> >>
> >> What I'm saying is that if we have in-band signalling that is reliable
> >> for a particular interface mode, why not always use it, rather than
> >> singling out one specific speed as an exception? Does it not work in
> >> 100Mbps and 1Gbps?
> 
> While the CPSW MAC supports RGMII in-band status operation, the link partner
> might not support it. I have also observed that forced mode is preferred to
> in-band mode as implemented for another driver:
> commit ade64eb5be9768e40c90ecb01295416abb2ddbac
> net: dsa: microchip: Disable RGMII in-band status on KSZ9893
> 
> and in the mail thread at:
> https://lore.kernel.org/netdev/20200905160647.GJ3164319@lunn.ch/
> based on Andrew's suggestion, using forced mode appears to be better.
> 
> Additionally, I have verified that switching to in-band status causes a
> regression. Thus, I will prefer keeping it in forced mode for 100 and 1000 Mbps
> RGMII mode which is the existing implementation in the driver. Please let me know.

Okay, so what this seems to mean is if you have a PHY that does not
support in-band status in RGMII mode, then 10Mbps isn't possible -
because the MAC requires in-band status mode to select 10Mbps.
To put it another way, in such a combination, 10Mbps link modes
should not be advertised, nor should they be reported to userspace
as being supported.

Is that correct?

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Siddharth Vadapalli]

On 03/04/23 14:02, Russell King (Oracle) wrote:
> On Mon, Apr 03, 2023 at 11:57:21AM +0530, Siddharth Vadapalli wrote:
>> Hello Russell,
>>
>> On 31/03/23 19:16, Siddharth Vadapalli wrote:
>>>
>>>
>>> On 31-03-2023 16:42, Russell King (Oracle) wrote:
>>>> On Fri, Mar 31, 2023 at 04:23:16PM +0530, Siddharth Vadapalli wrote:
>>>>>
>>>>>
>>>>> On 31/03/23 15:16, Russell King (Oracle) wrote:
>>>>>> On Fri, Mar 31, 2023 at 02:55:56PM +0530, Siddharth Vadapalli wrote:
>>>>>>> Russell,
>>>>>>>
>>>>>>> On 31/03/23 13:54, Russell King (Oracle) wrote:
>>>>>>>> On Fri, Mar 31, 2023 at 01:35:10PM +0530, Siddharth Vadapalli wrote:
>>>>>>>>> Hello Russell,
>>>>>>>>>
>>>>>>>>> Thank you for reviewing the patch.
>>>>>>>>>
>>>>>>>>> On 31/03/23 13:27, Russell King (Oracle) wrote:
>>>>>>>>>> On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
>>>>>>>>>>> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
>>>>>>>>>>> extra_modes member of the J784S4 SoC data. Additionally, configure the
>>>>>>>>>>> MAC Control register for supporting USXGMII mode. Also, for USXGMII
>>>>>>>>>>> mode, include MAC_5000FD in the "mac_capabilities" member of struct
>>>>>>>>>>> "phylink_config".
>>>>>>>>>>
>>>>>>>>>> I don't think TI "get" phylink at all...
>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>>>>>>> index 4b4d06199b45..ab33e6fe5b1a 100644
>>>>>>>>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>>>>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>>>>>>> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
>>>>>>>>>>>  		mac_control |= CPSW_SL_CTL_GIG;
>>>>>>>>>>>  	if (interface == PHY_INTERFACE_MODE_SGMII)
>>>>>>>>>>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
>>>>>>>>>>> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
>>>>>>>>>>> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
>>>>>>>>>>
>>>>>>>>>> The configuration of the interface mode should *not* happen in
>>>>>>>>>> mac_link_up(), but should happen in e.g. mac_config().
>>>>>>>>>
>>>>>>>>> I will move all the interface mode associated configurations to mac_config() in
>>>>>>>>> the v2 series.
>>>>>>>>
>>>>>>>> Looking at the whole of mac_link_up(), could you please describe what
>>>>>>>> effect these bits are having:
>>>>>>>>
>>>>>>>> 	CPSW_SL_CTL_GIG
>>>>>>>> 	CPSW_SL_CTL_EXT_EN
>>>>>>>> 	CPSW_SL_CTL_IFCTL_A
>>>>>>>
>>>>>>> CPSW_SL_CTL_GIG corresponds to enabling Gigabit mode (full duplex only).
>>>>>>> CPSW_SL_CTL_EXT_EN when set enables in-band mode of operation and when cleared
>>>>>>> enables forced mode of operation.
>>>>>>> CPSW_SL_CTL_IFCTL_A is used to set the RMII link speed (0=10 mbps, 1=100 mbps).
>>>>>>
>>>>>> Okay, so I would do in mac_link_up():
>>>>>>
>>>>>> 	/* RMII needs to be manually configured for 10/100Mbps */
>>>>>> 	if (interface == PHY_INTERFACE_MODE_RMII && speed == SPEED_100)
>>>>>> 		mac_control |= CPSW_SL_CTL_IFCTL_A;
>>>>>>
>>>>>> 	if (speed == SPEED_1000)
>>>>>> 		mac_control |= CPSW_SL_CTL_GIG;
>>>>>> 	if (duplex)
>>>>>> 		mac_control |= CPSW_SL_CTL_FULLDUPLEX;
>>>>>>
>>>>>> I would also make mac_link_up() do a read-modify-write operation to
>>>>>> only affect the bits that it is changing.
>>>>>
>>>>> This is the current implementation except for the SGMII mode associated
>>>>> operation that I had recently added. I will fix that. Also, the
>>>>> cpsw_sl_ctl_set() function which writes the mac_control value performs a read
>>>>> modify write operation.
>>>>>
>>>>>>
>>>>>> Now, for SGMII, I would move setting CPSW_SL_CTL_EXT_EN to mac_config()
>>>>>> to enable in-band mode - don't we want in-band mode enabled all the
>>>>>> time while in SGMII mode so the PHY gets the response from the MAC?
>>>>>
>>>>> Thank you for pointing it out. I will move that to mac_config().
>>>>>
>>>>>>
>>>>>> Lastly, for RGMII at 10Mbps, you seem to suggest that you need RGMII
>>>>>> in-band mode enabled for that - but if you need RGMII in-band for
>>>>>> 10Mbps, wouldn't it make sense for the other speeds as well? If so,
>>>>>> wouldn't that mean that CPSW_SL_CTL_EXT_EN can always be set for
>>>>>> RGMII no matter what speed is being used?
>>>>>
>>>>> The CPSW MAC does not support forced mode at 10 Mbps RGMII. For this reason, if
>>>>> RGMII 10 Mbps is requested, it is set to in-band mode.
>>>>
>>>> What I'm saying is that if we have in-band signalling that is reliable
>>>> for a particular interface mode, why not always use it, rather than
>>>> singling out one specific speed as an exception? Does it not work in
>>>> 100Mbps and 1Gbps?
>>
>> While the CPSW MAC supports RGMII in-band status operation, the link partner
>> might not support it. I have also observed that forced mode is preferred to
>> in-band mode as implemented for another driver:
>> commit ade64eb5be9768e40c90ecb01295416abb2ddbac
>> net: dsa: microchip: Disable RGMII in-band status on KSZ9893
>>
>> and in the mail thread at:
>> https://lore.kernel.org/netdev/20200905160647.GJ3164319@lunn.ch/
>> based on Andrew's suggestion, using forced mode appears to be better.
>>
>> Additionally, I have verified that switching to in-band status causes a
>> regression. Thus, I will prefer keeping it in forced mode for 100 and 1000 Mbps
>> RGMII mode which is the existing implementation in the driver. Please let me know.
> 
> Okay, so what this seems to mean is if you have a PHY that does not
> support in-band status in RGMII mode, then 10Mbps isn't possible -
> because the MAC requires in-band status mode to select 10Mbps.
> To put it another way, in such a combination, 10Mbps link modes
> should not be advertised, nor should they be reported to userspace
> as being supported.
> 
> Is that correct?

Yes, if the PHY does not support in-band status, 10 Mbps RGMII will not work,
despite the MAC supporting 10 Mbps in-band RGMII. However, I notice the following:
If the RGMII interface speed is set to 10 Mbps via ethtool, but the:
managed = "in-band-status";
property is not mentioned in the device-tree, the interface is able to work with
10 Mbps mode with the PHY. This is with the CPSW MAC configured for in-band mode
of operation at 10 Mbps RGMII mode. Please let me know what this indicates,
since it appears to me that 10 Mbps is functional in this special case (It might
be an erroneous configuration).

Regards,
Siddharth.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Russell King (Oracle)]

On Mon, Apr 03, 2023 at 02:11:08PM +0530, Siddharth Vadapalli wrote:
> 
> 
> On 03/04/23 14:02, Russell King (Oracle) wrote:
> > On Mon, Apr 03, 2023 at 11:57:21AM +0530, Siddharth Vadapalli wrote:
> >> Hello Russell,
> >>
> >> On 31/03/23 19:16, Siddharth Vadapalli wrote:
> >>>
> >>>
> >>> On 31-03-2023 16:42, Russell King (Oracle) wrote:
> >>>> On Fri, Mar 31, 2023 at 04:23:16PM +0530, Siddharth Vadapalli wrote:
> >>>>>
> >>>>>
> >>>>> On 31/03/23 15:16, Russell King (Oracle) wrote:
> >>>>>> On Fri, Mar 31, 2023 at 02:55:56PM +0530, Siddharth Vadapalli wrote:
> >>>>>>> Russell,
> >>>>>>>
> >>>>>>> On 31/03/23 13:54, Russell King (Oracle) wrote:
> >>>>>>>> On Fri, Mar 31, 2023 at 01:35:10PM +0530, Siddharth Vadapalli wrote:
> >>>>>>>>> Hello Russell,
> >>>>>>>>>
> >>>>>>>>> Thank you for reviewing the patch.
> >>>>>>>>>
> >>>>>>>>> On 31/03/23 13:27, Russell King (Oracle) wrote:
> >>>>>>>>>> On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
> >>>>>>>>>>> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
> >>>>>>>>>>> extra_modes member of the J784S4 SoC data. Additionally, configure the
> >>>>>>>>>>> MAC Control register for supporting USXGMII mode. Also, for USXGMII
> >>>>>>>>>>> mode, include MAC_5000FD in the "mac_capabilities" member of struct
> >>>>>>>>>>> "phylink_config".
> >>>>>>>>>>
> >>>>>>>>>> I don't think TI "get" phylink at all...
> >>>>>>>>>>
> >>>>>>>>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>>>>>>>>> index 4b4d06199b45..ab33e6fe5b1a 100644
> >>>>>>>>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>>>>>>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>>>>>>>>> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
> >>>>>>>>>>>  		mac_control |= CPSW_SL_CTL_GIG;
> >>>>>>>>>>>  	if (interface == PHY_INTERFACE_MODE_SGMII)
> >>>>>>>>>>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
> >>>>>>>>>>> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
> >>>>>>>>>>> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
> >>>>>>>>>>
> >>>>>>>>>> The configuration of the interface mode should *not* happen in
> >>>>>>>>>> mac_link_up(), but should happen in e.g. mac_config().
> >>>>>>>>>
> >>>>>>>>> I will move all the interface mode associated configurations to mac_config() in
> >>>>>>>>> the v2 series.
> >>>>>>>>
> >>>>>>>> Looking at the whole of mac_link_up(), could you please describe what
> >>>>>>>> effect these bits are having:
> >>>>>>>>
> >>>>>>>> 	CPSW_SL_CTL_GIG
> >>>>>>>> 	CPSW_SL_CTL_EXT_EN
> >>>>>>>> 	CPSW_SL_CTL_IFCTL_A
> >>>>>>>
> >>>>>>> CPSW_SL_CTL_GIG corresponds to enabling Gigabit mode (full duplex only).
> >>>>>>> CPSW_SL_CTL_EXT_EN when set enables in-band mode of operation and when cleared
> >>>>>>> enables forced mode of operation.
> >>>>>>> CPSW_SL_CTL_IFCTL_A is used to set the RMII link speed (0=10 mbps, 1=100 mbps).
> >>>>>>
> >>>>>> Okay, so I would do in mac_link_up():
> >>>>>>
> >>>>>> 	/* RMII needs to be manually configured for 10/100Mbps */
> >>>>>> 	if (interface == PHY_INTERFACE_MODE_RMII && speed == SPEED_100)
> >>>>>> 		mac_control |= CPSW_SL_CTL_IFCTL_A;
> >>>>>>
> >>>>>> 	if (speed == SPEED_1000)
> >>>>>> 		mac_control |= CPSW_SL_CTL_GIG;
> >>>>>> 	if (duplex)
> >>>>>> 		mac_control |= CPSW_SL_CTL_FULLDUPLEX;
> >>>>>>
> >>>>>> I would also make mac_link_up() do a read-modify-write operation to
> >>>>>> only affect the bits that it is changing.
> >>>>>
> >>>>> This is the current implementation except for the SGMII mode associated
> >>>>> operation that I had recently added. I will fix that. Also, the
> >>>>> cpsw_sl_ctl_set() function which writes the mac_control value performs a read
> >>>>> modify write operation.
> >>>>>
> >>>>>>
> >>>>>> Now, for SGMII, I would move setting CPSW_SL_CTL_EXT_EN to mac_config()
> >>>>>> to enable in-band mode - don't we want in-band mode enabled all the
> >>>>>> time while in SGMII mode so the PHY gets the response from the MAC?
> >>>>>
> >>>>> Thank you for pointing it out. I will move that to mac_config().
> >>>>>
> >>>>>>
> >>>>>> Lastly, for RGMII at 10Mbps, you seem to suggest that you need RGMII
> >>>>>> in-band mode enabled for that - but if you need RGMII in-band for
> >>>>>> 10Mbps, wouldn't it make sense for the other speeds as well? If so,
> >>>>>> wouldn't that mean that CPSW_SL_CTL_EXT_EN can always be set for
> >>>>>> RGMII no matter what speed is being used?
> >>>>>
> >>>>> The CPSW MAC does not support forced mode at 10 Mbps RGMII. For this reason, if
> >>>>> RGMII 10 Mbps is requested, it is set to in-band mode.
> >>>>
> >>>> What I'm saying is that if we have in-band signalling that is reliable
> >>>> for a particular interface mode, why not always use it, rather than
> >>>> singling out one specific speed as an exception? Does it not work in
> >>>> 100Mbps and 1Gbps?
> >>
> >> While the CPSW MAC supports RGMII in-band status operation, the link partner
> >> might not support it. I have also observed that forced mode is preferred to
> >> in-band mode as implemented for another driver:
> >> commit ade64eb5be9768e40c90ecb01295416abb2ddbac
> >> net: dsa: microchip: Disable RGMII in-band status on KSZ9893
> >>
> >> and in the mail thread at:
> >> https://lore.kernel.org/netdev/20200905160647.GJ3164319@lunn.ch/
> >> based on Andrew's suggestion, using forced mode appears to be better.
> >>
> >> Additionally, I have verified that switching to in-band status causes a
> >> regression. Thus, I will prefer keeping it in forced mode for 100 and 1000 Mbps
> >> RGMII mode which is the existing implementation in the driver. Please let me know.
> > 
> > Okay, so what this seems to mean is if you have a PHY that does not
> > support in-band status in RGMII mode, then 10Mbps isn't possible -
> > because the MAC requires in-band status mode to select 10Mbps.
> > To put it another way, in such a combination, 10Mbps link modes
> > should not be advertised, nor should they be reported to userspace
> > as being supported.
> > 
> > Is that correct?
> 
> Yes, if the PHY does not support in-band status, 10 Mbps RGMII will not work,
> despite the MAC supporting 10 Mbps in-band RGMII. However, I notice the following:
> If the RGMII interface speed is set to 10 Mbps via ethtool, but the:
> managed = "in-band-status";
> property is not mentioned in the device-tree, the interface is able to work with
> 10 Mbps mode with the PHY. This is with the CPSW MAC configured for in-band mode
> of operation at 10 Mbps RGMII mode. Please let me know what this indicates,
> since it appears to me that 10 Mbps is functional in this special case (It might
> be an erroneous configuration).

I think you need to check carefully what is going on.

Firstly, if you as the MAC is choosing to enable in-band status mode,
but phylink isn't using in-band status mode, that is entirely a matter
for your MAC driver.

Secondly, you need to research what the PHY does during the inter-frame
time (when in-band status would be transferred). This is when RX_CTL
is 0,0, RX_DV is 0, RX_ER is 0.

For in-band 10Mbps mode to work, RXD nibbles would need to be x001
(middle two bits indicate RX clock = 2.5MHz clock for 10Mbps, lsb
indicates link up). MSB determines duplex. Remember that 10Mbps can
appear to work with mismatched duplex settings but can cause chaos on
networks when it disagrees with what the rest of the network is doing.

So, I think before one says "setting in-band mode for 10Mbps with a
PHY that doesn't support in-band" really needs caution and research
to check what _actually_ ends up happening, and whether it is really
correct to do this.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Siddharth Vadapalli]

On 03/04/23 14:29, Russell King (Oracle) wrote:
> On Mon, Apr 03, 2023 at 02:11:08PM +0530, Siddharth Vadapalli wrote:
>>
>>
>> On 03/04/23 14:02, Russell King (Oracle) wrote:
>>> On Mon, Apr 03, 2023 at 11:57:21AM +0530, Siddharth Vadapalli wrote:
>>>> Hello Russell,
>>>>
>>>> On 31/03/23 19:16, Siddharth Vadapalli wrote:
>>>>>
>>>>>
>>>>> On 31-03-2023 16:42, Russell King (Oracle) wrote:
>>>>>> On Fri, Mar 31, 2023 at 04:23:16PM +0530, Siddharth Vadapalli wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 31/03/23 15:16, Russell King (Oracle) wrote:
>>>>>>>> On Fri, Mar 31, 2023 at 02:55:56PM +0530, Siddharth Vadapalli wrote:
>>>>>>>>> Russell,
>>>>>>>>>
>>>>>>>>> On 31/03/23 13:54, Russell King (Oracle) wrote:
>>>>>>>>>> On Fri, Mar 31, 2023 at 01:35:10PM +0530, Siddharth Vadapalli wrote:
>>>>>>>>>>> Hello Russell,
>>>>>>>>>>>
>>>>>>>>>>> Thank you for reviewing the patch.
>>>>>>>>>>>
>>>>>>>>>>> On 31/03/23 13:27, Russell King (Oracle) wrote:
>>>>>>>>>>>> On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
>>>>>>>>>>>>> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
>>>>>>>>>>>>> extra_modes member of the J784S4 SoC data. Additionally, configure the
>>>>>>>>>>>>> MAC Control register for supporting USXGMII mode. Also, for USXGMII
>>>>>>>>>>>>> mode, include MAC_5000FD in the "mac_capabilities" member of struct
>>>>>>>>>>>>> "phylink_config".
>>>>>>>>>>>>
>>>>>>>>>>>> I don't think TI "get" phylink at all...
>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>>>>>>>>> index 4b4d06199b45..ab33e6fe5b1a 100644
>>>>>>>>>>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>>>>>>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>>>>>>>>> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
>>>>>>>>>>>>>  		mac_control |= CPSW_SL_CTL_GIG;
>>>>>>>>>>>>>  	if (interface == PHY_INTERFACE_MODE_SGMII)
>>>>>>>>>>>>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
>>>>>>>>>>>>> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
>>>>>>>>>>>>> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
>>>>>>>>>>>>
>>>>>>>>>>>> The configuration of the interface mode should *not* happen in
>>>>>>>>>>>> mac_link_up(), but should happen in e.g. mac_config().
>>>>>>>>>>>
>>>>>>>>>>> I will move all the interface mode associated configurations to mac_config() in
>>>>>>>>>>> the v2 series.
>>>>>>>>>>
>>>>>>>>>> Looking at the whole of mac_link_up(), could you please describe what
>>>>>>>>>> effect these bits are having:
>>>>>>>>>>
>>>>>>>>>> 	CPSW_SL_CTL_GIG
>>>>>>>>>> 	CPSW_SL_CTL_EXT_EN
>>>>>>>>>> 	CPSW_SL_CTL_IFCTL_A
>>>>>>>>>
>>>>>>>>> CPSW_SL_CTL_GIG corresponds to enabling Gigabit mode (full duplex only).
>>>>>>>>> CPSW_SL_CTL_EXT_EN when set enables in-band mode of operation and when cleared
>>>>>>>>> enables forced mode of operation.
>>>>>>>>> CPSW_SL_CTL_IFCTL_A is used to set the RMII link speed (0=10 mbps, 1=100 mbps).
>>>>>>>>
>>>>>>>> Okay, so I would do in mac_link_up():
>>>>>>>>
>>>>>>>> 	/* RMII needs to be manually configured for 10/100Mbps */
>>>>>>>> 	if (interface == PHY_INTERFACE_MODE_RMII && speed == SPEED_100)
>>>>>>>> 		mac_control |= CPSW_SL_CTL_IFCTL_A;
>>>>>>>>
>>>>>>>> 	if (speed == SPEED_1000)
>>>>>>>> 		mac_control |= CPSW_SL_CTL_GIG;
>>>>>>>> 	if (duplex)
>>>>>>>> 		mac_control |= CPSW_SL_CTL_FULLDUPLEX;
>>>>>>>>
>>>>>>>> I would also make mac_link_up() do a read-modify-write operation to
>>>>>>>> only affect the bits that it is changing.
>>>>>>>
>>>>>>> This is the current implementation except for the SGMII mode associated
>>>>>>> operation that I had recently added. I will fix that. Also, the
>>>>>>> cpsw_sl_ctl_set() function which writes the mac_control value performs a read
>>>>>>> modify write operation.
>>>>>>>
>>>>>>>>
>>>>>>>> Now, for SGMII, I would move setting CPSW_SL_CTL_EXT_EN to mac_config()
>>>>>>>> to enable in-band mode - don't we want in-band mode enabled all the
>>>>>>>> time while in SGMII mode so the PHY gets the response from the MAC?
>>>>>>>
>>>>>>> Thank you for pointing it out. I will move that to mac_config().
>>>>>>>
>>>>>>>>
>>>>>>>> Lastly, for RGMII at 10Mbps, you seem to suggest that you need RGMII
>>>>>>>> in-band mode enabled for that - but if you need RGMII in-band for
>>>>>>>> 10Mbps, wouldn't it make sense for the other speeds as well? If so,
>>>>>>>> wouldn't that mean that CPSW_SL_CTL_EXT_EN can always be set for
>>>>>>>> RGMII no matter what speed is being used?
>>>>>>>
>>>>>>> The CPSW MAC does not support forced mode at 10 Mbps RGMII. For this reason, if
>>>>>>> RGMII 10 Mbps is requested, it is set to in-band mode.
>>>>>>
>>>>>> What I'm saying is that if we have in-band signalling that is reliable
>>>>>> for a particular interface mode, why not always use it, rather than
>>>>>> singling out one specific speed as an exception? Does it not work in
>>>>>> 100Mbps and 1Gbps?
>>>>
>>>> While the CPSW MAC supports RGMII in-band status operation, the link partner
>>>> might not support it. I have also observed that forced mode is preferred to
>>>> in-band mode as implemented for another driver:
>>>> commit ade64eb5be9768e40c90ecb01295416abb2ddbac
>>>> net: dsa: microchip: Disable RGMII in-band status on KSZ9893
>>>>
>>>> and in the mail thread at:
>>>> https://lore.kernel.org/netdev/20200905160647.GJ3164319@lunn.ch/
>>>> based on Andrew's suggestion, using forced mode appears to be better.
>>>>
>>>> Additionally, I have verified that switching to in-band status causes a
>>>> regression. Thus, I will prefer keeping it in forced mode for 100 and 1000 Mbps
>>>> RGMII mode which is the existing implementation in the driver. Please let me know.
>>>
>>> Okay, so what this seems to mean is if you have a PHY that does not
>>> support in-band status in RGMII mode, then 10Mbps isn't possible -
>>> because the MAC requires in-band status mode to select 10Mbps.
>>> To put it another way, in such a combination, 10Mbps link modes
>>> should not be advertised, nor should they be reported to userspace
>>> as being supported.
>>>
>>> Is that correct?
>>
>> Yes, if the PHY does not support in-band status, 10 Mbps RGMII will not work,
>> despite the MAC supporting 10 Mbps in-band RGMII. However, I notice the following:
>> If the RGMII interface speed is set to 10 Mbps via ethtool, but the:
>> managed = "in-band-status";
>> property is not mentioned in the device-tree, the interface is able to work with
>> 10 Mbps mode with the PHY. This is with the CPSW MAC configured for in-band mode
>> of operation at 10 Mbps RGMII mode. Please let me know what this indicates,
>> since it appears to me that 10 Mbps is functional in this special case (It might
>> be an erroneous configuration).
> 
> I think you need to check carefully what is going on.
> 
> Firstly, if you as the MAC is choosing to enable in-band status mode,
> but phylink isn't using in-band status mode, that is entirely a matter
> for your MAC driver.
> 
> Secondly, you need to research what the PHY does during the inter-frame
> time (when in-band status would be transferred). This is when RX_CTL
> is 0,0, RX_DV is 0, RX_ER is 0.
> 
> For in-band 10Mbps mode to work, RXD nibbles would need to be x001
> (middle two bits indicate RX clock = 2.5MHz clock for 10Mbps, lsb
> indicates link up). MSB determines duplex. Remember that 10Mbps can
> appear to work with mismatched duplex settings but can cause chaos on
> networks when it disagrees with what the rest of the network is doing.
> 
> So, I think before one says "setting in-band mode for 10Mbps with a
> PHY that doesn't support in-band" really needs caution and research
> to check what _actually_ ends up happening, and whether it is really
> correct to do this.

Thank you for the detailed explanation. I will analyze it and fix this. In the
meanwhile, is it acceptable for me to post the v2 of this series, with the other
suggestions implemented, while maintaining the status quo for the 10 Mbps RGMII
configuration in the driver? Please let me know.

Regards,
Siddharth.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Russell King (Oracle)]

On Mon, Apr 03, 2023 at 03:19:24PM +0530, Siddharth Vadapalli wrote:
> 
> 
> On 03/04/23 14:29, Russell King (Oracle) wrote:
> > On Mon, Apr 03, 2023 at 02:11:08PM +0530, Siddharth Vadapalli wrote:
> >>
> >>
> >> On 03/04/23 14:02, Russell King (Oracle) wrote:
> >>> On Mon, Apr 03, 2023 at 11:57:21AM +0530, Siddharth Vadapalli wrote:
> >>>> Hello Russell,
> >>>>
> >>>> On 31/03/23 19:16, Siddharth Vadapalli wrote:
> >>>>>
> >>>>>
> >>>>> On 31-03-2023 16:42, Russell King (Oracle) wrote:
> >>>>>> On Fri, Mar 31, 2023 at 04:23:16PM +0530, Siddharth Vadapalli wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On 31/03/23 15:16, Russell King (Oracle) wrote:
> >>>>>>>> On Fri, Mar 31, 2023 at 02:55:56PM +0530, Siddharth Vadapalli wrote:
> >>>>>>>>> Russell,
> >>>>>>>>>
> >>>>>>>>> On 31/03/23 13:54, Russell King (Oracle) wrote:
> >>>>>>>>>> On Fri, Mar 31, 2023 at 01:35:10PM +0530, Siddharth Vadapalli wrote:
> >>>>>>>>>>> Hello Russell,
> >>>>>>>>>>>
> >>>>>>>>>>> Thank you for reviewing the patch.
> >>>>>>>>>>>
> >>>>>>>>>>> On 31/03/23 13:27, Russell King (Oracle) wrote:
> >>>>>>>>>>>> On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
> >>>>>>>>>>>>> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
> >>>>>>>>>>>>> extra_modes member of the J784S4 SoC data. Additionally, configure the
> >>>>>>>>>>>>> MAC Control register for supporting USXGMII mode. Also, for USXGMII
> >>>>>>>>>>>>> mode, include MAC_5000FD in the "mac_capabilities" member of struct
> >>>>>>>>>>>>> "phylink_config".
> >>>>>>>>>>>>
> >>>>>>>>>>>> I don't think TI "get" phylink at all...
> >>>>>>>>>>>>
> >>>>>>>>>>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>>>>>>>>>>> index 4b4d06199b45..ab33e6fe5b1a 100644
> >>>>>>>>>>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>>>>>>>>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>>>>>>>>>>> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
> >>>>>>>>>>>>>  		mac_control |= CPSW_SL_CTL_GIG;
> >>>>>>>>>>>>>  	if (interface == PHY_INTERFACE_MODE_SGMII)
> >>>>>>>>>>>>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
> >>>>>>>>>>>>> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
> >>>>>>>>>>>>> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
> >>>>>>>>>>>>
> >>>>>>>>>>>> The configuration of the interface mode should *not* happen in
> >>>>>>>>>>>> mac_link_up(), but should happen in e.g. mac_config().
> >>>>>>>>>>>
> >>>>>>>>>>> I will move all the interface mode associated configurations to mac_config() in
> >>>>>>>>>>> the v2 series.
> >>>>>>>>>>
> >>>>>>>>>> Looking at the whole of mac_link_up(), could you please describe what
> >>>>>>>>>> effect these bits are having:
> >>>>>>>>>>
> >>>>>>>>>> 	CPSW_SL_CTL_GIG
> >>>>>>>>>> 	CPSW_SL_CTL_EXT_EN
> >>>>>>>>>> 	CPSW_SL_CTL_IFCTL_A
> >>>>>>>>>
> >>>>>>>>> CPSW_SL_CTL_GIG corresponds to enabling Gigabit mode (full duplex only).
> >>>>>>>>> CPSW_SL_CTL_EXT_EN when set enables in-band mode of operation and when cleared
> >>>>>>>>> enables forced mode of operation.
> >>>>>>>>> CPSW_SL_CTL_IFCTL_A is used to set the RMII link speed (0=10 mbps, 1=100 mbps).
> >>>>>>>>
> >>>>>>>> Okay, so I would do in mac_link_up():
> >>>>>>>>
> >>>>>>>> 	/* RMII needs to be manually configured for 10/100Mbps */
> >>>>>>>> 	if (interface == PHY_INTERFACE_MODE_RMII && speed == SPEED_100)
> >>>>>>>> 		mac_control |= CPSW_SL_CTL_IFCTL_A;
> >>>>>>>>
> >>>>>>>> 	if (speed == SPEED_1000)
> >>>>>>>> 		mac_control |= CPSW_SL_CTL_GIG;
> >>>>>>>> 	if (duplex)
> >>>>>>>> 		mac_control |= CPSW_SL_CTL_FULLDUPLEX;
> >>>>>>>>
> >>>>>>>> I would also make mac_link_up() do a read-modify-write operation to
> >>>>>>>> only affect the bits that it is changing.
> >>>>>>>
> >>>>>>> This is the current implementation except for the SGMII mode associated
> >>>>>>> operation that I had recently added. I will fix that. Also, the
> >>>>>>> cpsw_sl_ctl_set() function which writes the mac_control value performs a read
> >>>>>>> modify write operation.
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Now, for SGMII, I would move setting CPSW_SL_CTL_EXT_EN to mac_config()
> >>>>>>>> to enable in-band mode - don't we want in-band mode enabled all the
> >>>>>>>> time while in SGMII mode so the PHY gets the response from the MAC?
> >>>>>>>
> >>>>>>> Thank you for pointing it out. I will move that to mac_config().
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Lastly, for RGMII at 10Mbps, you seem to suggest that you need RGMII
> >>>>>>>> in-band mode enabled for that - but if you need RGMII in-band for
> >>>>>>>> 10Mbps, wouldn't it make sense for the other speeds as well? If so,
> >>>>>>>> wouldn't that mean that CPSW_SL_CTL_EXT_EN can always be set for
> >>>>>>>> RGMII no matter what speed is being used?
> >>>>>>>
> >>>>>>> The CPSW MAC does not support forced mode at 10 Mbps RGMII. For this reason, if
> >>>>>>> RGMII 10 Mbps is requested, it is set to in-band mode.
> >>>>>>
> >>>>>> What I'm saying is that if we have in-band signalling that is reliable
> >>>>>> for a particular interface mode, why not always use it, rather than
> >>>>>> singling out one specific speed as an exception? Does it not work in
> >>>>>> 100Mbps and 1Gbps?
> >>>>
> >>>> While the CPSW MAC supports RGMII in-band status operation, the link partner
> >>>> might not support it. I have also observed that forced mode is preferred to
> >>>> in-band mode as implemented for another driver:
> >>>> commit ade64eb5be9768e40c90ecb01295416abb2ddbac
> >>>> net: dsa: microchip: Disable RGMII in-band status on KSZ9893
> >>>>
> >>>> and in the mail thread at:
> >>>> https://lore.kernel.org/netdev/20200905160647.GJ3164319@lunn.ch/
> >>>> based on Andrew's suggestion, using forced mode appears to be better.
> >>>>
> >>>> Additionally, I have verified that switching to in-band status causes a
> >>>> regression. Thus, I will prefer keeping it in forced mode for 100 and 1000 Mbps
> >>>> RGMII mode which is the existing implementation in the driver. Please let me know.
> >>>
> >>> Okay, so what this seems to mean is if you have a PHY that does not
> >>> support in-band status in RGMII mode, then 10Mbps isn't possible -
> >>> because the MAC requires in-band status mode to select 10Mbps.
> >>> To put it another way, in such a combination, 10Mbps link modes
> >>> should not be advertised, nor should they be reported to userspace
> >>> as being supported.
> >>>
> >>> Is that correct?
> >>
> >> Yes, if the PHY does not support in-band status, 10 Mbps RGMII will not work,
> >> despite the MAC supporting 10 Mbps in-band RGMII. However, I notice the following:
> >> If the RGMII interface speed is set to 10 Mbps via ethtool, but the:
> >> managed = "in-band-status";
> >> property is not mentioned in the device-tree, the interface is able to work with
> >> 10 Mbps mode with the PHY. This is with the CPSW MAC configured for in-band mode
> >> of operation at 10 Mbps RGMII mode. Please let me know what this indicates,
> >> since it appears to me that 10 Mbps is functional in this special case (It might
> >> be an erroneous configuration).
> > 
> > I think you need to check carefully what is going on.
> > 
> > Firstly, if you as the MAC is choosing to enable in-band status mode,
> > but phylink isn't using in-band status mode, that is entirely a matter
> > for your MAC driver.
> > 
> > Secondly, you need to research what the PHY does during the inter-frame
> > time (when in-band status would be transferred). This is when RX_CTL
> > is 0,0, RX_DV is 0, RX_ER is 0.
> > 
> > For in-band 10Mbps mode to work, RXD nibbles would need to be x001
> > (middle two bits indicate RX clock = 2.5MHz clock for 10Mbps, lsb
> > indicates link up). MSB determines duplex. Remember that 10Mbps can
> > appear to work with mismatched duplex settings but can cause chaos on
> > networks when it disagrees with what the rest of the network is doing.
> > 
> > So, I think before one says "setting in-band mode for 10Mbps with a
> > PHY that doesn't support in-band" really needs caution and research
> > to check what _actually_ ends up happening, and whether it is really
> > correct to do this.
> 
> Thank you for the detailed explanation. I will analyze it and fix this. In the
> meanwhile, is it acceptable for me to post the v2 of this series, with the other
> suggestions implemented, while maintaining the status quo for the 10 Mbps RGMII
> configuration in the driver? Please let me know.

Yes, but I would like a comment against the bit of code that enables
in-band mode indicating that it's questionable whether it is correct.

Thanks.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Siddharth Vadapalli]

On 03/04/23 15:27, Russell King (Oracle) wrote:
> On Mon, Apr 03, 2023 at 03:19:24PM +0530, Siddharth Vadapalli wrote:
>>
>>
>> On 03/04/23 14:29, Russell King (Oracle) wrote:
>>> On Mon, Apr 03, 2023 at 02:11:08PM +0530, Siddharth Vadapalli wrote:
>>>>
>>>>
>>>> On 03/04/23 14:02, Russell King (Oracle) wrote:
>>>>> On Mon, Apr 03, 2023 at 11:57:21AM +0530, Siddharth Vadapalli wrote:
>>>>>> Hello Russell,
>>>>>>
>>>>>> On 31/03/23 19:16, Siddharth Vadapalli wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 31-03-2023 16:42, Russell King (Oracle) wrote:
>>>>>>>> On Fri, Mar 31, 2023 at 04:23:16PM +0530, Siddharth Vadapalli wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 31/03/23 15:16, Russell King (Oracle) wrote:
>>>>>>>>>> On Fri, Mar 31, 2023 at 02:55:56PM +0530, Siddharth Vadapalli wrote:
>>>>>>>>>>> Russell,
>>>>>>>>>>>
>>>>>>>>>>> On 31/03/23 13:54, Russell King (Oracle) wrote:
>>>>>>>>>>>> On Fri, Mar 31, 2023 at 01:35:10PM +0530, Siddharth Vadapalli wrote:
>>>>>>>>>>>>> Hello Russell,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thank you for reviewing the patch.
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 31/03/23 13:27, Russell King (Oracle) wrote:
>>>>>>>>>>>>>> On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
>>>>>>>>>>>>>>> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
>>>>>>>>>>>>>>> extra_modes member of the J784S4 SoC data. Additionally, configure the
>>>>>>>>>>>>>>> MAC Control register for supporting USXGMII mode. Also, for USXGMII
>>>>>>>>>>>>>>> mode, include MAC_5000FD in the "mac_capabilities" member of struct
>>>>>>>>>>>>>>> "phylink_config".
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I don't think TI "get" phylink at all...
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>>>>>>>>>>> index 4b4d06199b45..ab33e6fe5b1a 100644
>>>>>>>>>>>>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>>>>>>>>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>>>>>>>>>>>>> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
>>>>>>>>>>>>>>>  		mac_control |= CPSW_SL_CTL_GIG;
>>>>>>>>>>>>>>>  	if (interface == PHY_INTERFACE_MODE_SGMII)
>>>>>>>>>>>>>>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
>>>>>>>>>>>>>>> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
>>>>>>>>>>>>>>> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The configuration of the interface mode should *not* happen in
>>>>>>>>>>>>>> mac_link_up(), but should happen in e.g. mac_config().
>>>>>>>>>>>>>
>>>>>>>>>>>>> I will move all the interface mode associated configurations to mac_config() in
>>>>>>>>>>>>> the v2 series.
>>>>>>>>>>>>
>>>>>>>>>>>> Looking at the whole of mac_link_up(), could you please describe what
>>>>>>>>>>>> effect these bits are having:
>>>>>>>>>>>>
>>>>>>>>>>>> 	CPSW_SL_CTL_GIG
>>>>>>>>>>>> 	CPSW_SL_CTL_EXT_EN
>>>>>>>>>>>> 	CPSW_SL_CTL_IFCTL_A
>>>>>>>>>>>
>>>>>>>>>>> CPSW_SL_CTL_GIG corresponds to enabling Gigabit mode (full duplex only).
>>>>>>>>>>> CPSW_SL_CTL_EXT_EN when set enables in-band mode of operation and when cleared
>>>>>>>>>>> enables forced mode of operation.
>>>>>>>>>>> CPSW_SL_CTL_IFCTL_A is used to set the RMII link speed (0=10 mbps, 1=100 mbps).
>>>>>>>>>>
>>>>>>>>>> Okay, so I would do in mac_link_up():
>>>>>>>>>>
>>>>>>>>>> 	/* RMII needs to be manually configured for 10/100Mbps */
>>>>>>>>>> 	if (interface == PHY_INTERFACE_MODE_RMII && speed == SPEED_100)
>>>>>>>>>> 		mac_control |= CPSW_SL_CTL_IFCTL_A;
>>>>>>>>>>
>>>>>>>>>> 	if (speed == SPEED_1000)
>>>>>>>>>> 		mac_control |= CPSW_SL_CTL_GIG;
>>>>>>>>>> 	if (duplex)
>>>>>>>>>> 		mac_control |= CPSW_SL_CTL_FULLDUPLEX;
>>>>>>>>>>
>>>>>>>>>> I would also make mac_link_up() do a read-modify-write operation to
>>>>>>>>>> only affect the bits that it is changing.
>>>>>>>>>
>>>>>>>>> This is the current implementation except for the SGMII mode associated
>>>>>>>>> operation that I had recently added. I will fix that. Also, the
>>>>>>>>> cpsw_sl_ctl_set() function which writes the mac_control value performs a read
>>>>>>>>> modify write operation.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Now, for SGMII, I would move setting CPSW_SL_CTL_EXT_EN to mac_config()
>>>>>>>>>> to enable in-band mode - don't we want in-band mode enabled all the
>>>>>>>>>> time while in SGMII mode so the PHY gets the response from the MAC?
>>>>>>>>>
>>>>>>>>> Thank you for pointing it out. I will move that to mac_config().
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Lastly, for RGMII at 10Mbps, you seem to suggest that you need RGMII
>>>>>>>>>> in-band mode enabled for that - but if you need RGMII in-band for
>>>>>>>>>> 10Mbps, wouldn't it make sense for the other speeds as well? If so,
>>>>>>>>>> wouldn't that mean that CPSW_SL_CTL_EXT_EN can always be set for
>>>>>>>>>> RGMII no matter what speed is being used?
>>>>>>>>>
>>>>>>>>> The CPSW MAC does not support forced mode at 10 Mbps RGMII. For this reason, if
>>>>>>>>> RGMII 10 Mbps is requested, it is set to in-band mode.
>>>>>>>>
>>>>>>>> What I'm saying is that if we have in-band signalling that is reliable
>>>>>>>> for a particular interface mode, why not always use it, rather than
>>>>>>>> singling out one specific speed as an exception? Does it not work in
>>>>>>>> 100Mbps and 1Gbps?
>>>>>>
>>>>>> While the CPSW MAC supports RGMII in-band status operation, the link partner
>>>>>> might not support it. I have also observed that forced mode is preferred to
>>>>>> in-band mode as implemented for another driver:
>>>>>> commit ade64eb5be9768e40c90ecb01295416abb2ddbac
>>>>>> net: dsa: microchip: Disable RGMII in-band status on KSZ9893
>>>>>>
>>>>>> and in the mail thread at:
>>>>>> https://lore.kernel.org/netdev/20200905160647.GJ3164319@lunn.ch/
>>>>>> based on Andrew's suggestion, using forced mode appears to be better.
>>>>>>
>>>>>> Additionally, I have verified that switching to in-band status causes a
>>>>>> regression. Thus, I will prefer keeping it in forced mode for 100 and 1000 Mbps
>>>>>> RGMII mode which is the existing implementation in the driver. Please let me know.
>>>>>
>>>>> Okay, so what this seems to mean is if you have a PHY that does not
>>>>> support in-band status in RGMII mode, then 10Mbps isn't possible -
>>>>> because the MAC requires in-band status mode to select 10Mbps.
>>>>> To put it another way, in such a combination, 10Mbps link modes
>>>>> should not be advertised, nor should they be reported to userspace
>>>>> as being supported.
>>>>>
>>>>> Is that correct?
>>>>
>>>> Yes, if the PHY does not support in-band status, 10 Mbps RGMII will not work,
>>>> despite the MAC supporting 10 Mbps in-band RGMII. However, I notice the following:
>>>> If the RGMII interface speed is set to 10 Mbps via ethtool, but the:
>>>> managed = "in-band-status";
>>>> property is not mentioned in the device-tree, the interface is able to work with
>>>> 10 Mbps mode with the PHY. This is with the CPSW MAC configured for in-band mode
>>>> of operation at 10 Mbps RGMII mode. Please let me know what this indicates,
>>>> since it appears to me that 10 Mbps is functional in this special case (It might
>>>> be an erroneous configuration).
>>>
>>> I think you need to check carefully what is going on.
>>>
>>> Firstly, if you as the MAC is choosing to enable in-band status mode,
>>> but phylink isn't using in-band status mode, that is entirely a matter
>>> for your MAC driver.
>>>
>>> Secondly, you need to research what the PHY does during the inter-frame
>>> time (when in-band status would be transferred). This is when RX_CTL
>>> is 0,0, RX_DV is 0, RX_ER is 0.
>>>
>>> For in-band 10Mbps mode to work, RXD nibbles would need to be x001
>>> (middle two bits indicate RX clock = 2.5MHz clock for 10Mbps, lsb
>>> indicates link up). MSB determines duplex. Remember that 10Mbps can
>>> appear to work with mismatched duplex settings but can cause chaos on
>>> networks when it disagrees with what the rest of the network is doing.
>>>
>>> So, I think before one says "setting in-band mode for 10Mbps with a
>>> PHY that doesn't support in-band" really needs caution and research
>>> to check what _actually_ ends up happening, and whether it is really
>>> correct to do this.
>>
>> Thank you for the detailed explanation. I will analyze it and fix this. In the
>> meanwhile, is it acceptable for me to post the v2 of this series, with the other
>> suggestions implemented, while maintaining the status quo for the 10 Mbps RGMII
>> configuration in the driver? Please let me know.
> 
> Yes, but I would like a comment against the bit of code that enables
> in-band mode indicating that it's questionable whether it is correct.

Sure, thank you. I will add a TODO in that section, indicating that it needs to
be verified and fixed.

Regards,
Siddharth.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 0/2] Add support for J784S4 CPSW9G

[Roger Quadros]

On 31/03/2023 09:51, Siddharth Vadapalli wrote:
> Hello,
> 
> This series adds a new compatible to am65-cpsw driver for the CPSW9G
> instance of the CPSW Ethernet Switch on TI's J784S4 SoC which has 8
> external ports and 1 internal host port.
> 
> The CPSW9G instance supports QSGMII and USXGMII modes for which driver
> support is added.
> 
> Regards,
> Siddharth.
> 
> Siddharth Vadapalli (2):
>   net: ethernet: ti: am65-cpsw: Enable QSGMII for J784S4 CPSW9G
>   net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G
> 
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 

For this series,

Reviewed-by: Roger Quadros <rogerq@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel