Hello,
This series adds support to configure the CPSW Ethernet Switch in SGMII
mode, using the am65-cpsw-nuss driver. SGMII mode is supported by the
CPSWxG instances on TI's J7200 and J721E SoCs. Thus, SGMII mode is added
in the list of extra_modes for the appropriate compatibles corresponding
to the aforementioned SoCs.
Additionally, the method of setting the supported interface via struct
"phylink_config" is simplified by converting the IF/ELSE statements to
SWITCH statements.
Regards,
Siddharth.
Siddharth Vadapalli (4):
net: ethernet: ti: am65-cpsw: Simplify setting supported interface
net: ethernet: ti: am65-cpsw: Add support for SGMII mode
net: ethernet: ti: am65-cpsw: Enable SGMII mode for J7200
net: ethernet: ti: am65-cpsw: Enable SGMII mode for J721E
drivers/net/ethernet/ti/am65-cpsw-nuss.c | 42 +++++++++++++++++++-----
1 file changed, 33 insertions(+), 9 deletions(-)
--
2.25.1
Convert the existing IF/ELSE statement based approach of setting the
supported_interfaces member of struct "phylink_config", to SWITCH
statements. This will help scale to newer PHY-MODES as well as newer
compatibles.
Signed-off-by: Siddharth Vadapalli <[email protected]>
---
drivers/net/ethernet/ti/am65-cpsw-nuss.c | 27 ++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 4cfbc1c2b1c4..cba8db14e160 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2143,15 +2143,30 @@ 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;
port->slave.phylink_config.mac_managed_pm = true; /* MAC does PM */
- if (phy_interface_mode_is_rgmii(port->slave.phy_if)) {
+ switch (port->slave.phy_if) {
+ case PHY_INTERFACE_MODE_RGMII:
+ case PHY_INTERFACE_MODE_RGMII_ID:
+ case PHY_INTERFACE_MODE_RGMII_RXID:
+ case PHY_INTERFACE_MODE_RGMII_TXID:
phy_interface_set_rgmii(port->slave.phylink_config.supported_interfaces);
- } else if (port->slave.phy_if == PHY_INTERFACE_MODE_RMII) {
+ break;
+
+ case PHY_INTERFACE_MODE_RMII:
__set_bit(PHY_INTERFACE_MODE_RMII,
port->slave.phylink_config.supported_interfaces);
- } else if (common->pdata.extra_modes & BIT(port->slave.phy_if)) {
- __set_bit(PHY_INTERFACE_MODE_QSGMII,
- port->slave.phylink_config.supported_interfaces);
- } else {
+ break;
+
+ case PHY_INTERFACE_MODE_QSGMII:
+ if (common->pdata.extra_modes & BIT(port->slave.phy_if)) {
+ __set_bit(port->slave.phy_if,
+ port->slave.phylink_config.supported_interfaces);
+ } else {
+ dev_err(dev, "selected phy-mode is not supported\n");
+ return -EOPNOTSUPP;
+ }
+ break;
+
+ default:
dev_err(dev, "selected phy-mode is not supported\n");
return -EOPNOTSUPP;
}
--
2.25.1
Add support for configuring the CPSW Ethernet Switch in SGMII mode.
Depending on the SoC, allow selecting SGMII mode as a supported interface,
based on the compatible used.
Signed-off-by: Siddharth Vadapalli <[email protected]>
---
drivers/net/ethernet/ti/am65-cpsw-nuss.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index cba8db14e160..d2ca1f2035f4 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -76,6 +76,7 @@
#define AM65_CPSW_PORTN_REG_TS_CTL_LTYPE2 0x31C
#define AM65_CPSW_SGMII_CONTROL_REG 0x010
+#define AM65_CPSW_SGMII_MR_ADV_ABILITY_REG 0x018
#define AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE BIT(0)
#define AM65_CPSW_CTL_VLAN_AWARE BIT(1)
@@ -1496,9 +1497,14 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in
struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave);
struct am65_cpsw_common *common = port->common;
- if (common->pdata.extra_modes & BIT(state->interface))
+ if (common->pdata.extra_modes & BIT(state->interface)) {
+ if (state->interface == PHY_INTERFACE_MODE_SGMII)
+ writel(ADVERTISE_SGMII,
+ port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG);
+
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,
@@ -1539,6 +1545,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
if (speed == SPEED_1000)
mac_control |= CPSW_SL_CTL_GIG;
+ if (interface == PHY_INTERFACE_MODE_SGMII)
+ mac_control |= CPSW_SL_CTL_EXT_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;
@@ -2157,6 +2165,7 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
break;
case PHY_INTERFACE_MODE_QSGMII:
+ case PHY_INTERFACE_MODE_SGMII:
if (common->pdata.extra_modes & BIT(port->slave.phy_if)) {
__set_bit(port->slave.phy_if,
port->slave.phylink_config.supported_interfaces);
--
2.25.1
TI's J7200 SoC supports SGMII mode with the CPSW5G instance of the CPSW
Ethernet Switch. Thus, enable it by adding SGMII mode to the extra_modes
member of the "j7200_cpswxg_pdata" SoC data.
Signed-off-by: Siddharth Vadapalli <[email protected]>
---
drivers/net/ethernet/ti/am65-cpsw-nuss.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index d2ca1f2035f4..66e1fe58b895 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2777,7 +2777,7 @@ static const struct am65_cpsw_pdata j7200_cpswxg_pdata = {
.quirks = 0,
.ale_dev_id = "am64-cpswxg",
.fdqring_mode = K3_RINGACC_RING_MODE_RING,
- .extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII),
+ .extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII) | BIT(PHY_INTERFACE_MODE_SGMII),
};
static const struct am65_cpsw_pdata j721e_cpswxg_pdata = {
--
2.25.1
TI's J721E SoC supports SGMII mode with the CPSW9G instance of the CPSW
Ethernet Switch. Thus, enable it by adding SGMII mode to the extra_modes
member of the "j721e_cpswxg_pdata" SoC data.
Signed-off-by: Siddharth Vadapalli <[email protected]>
---
drivers/net/ethernet/ti/am65-cpsw-nuss.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 66e1fe58b895..9ddb79776c88 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2784,7 +2784,7 @@ static const struct am65_cpsw_pdata j721e_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_SGMII),
};
static const struct of_device_id am65_cpsw_nuss_of_mtable[] = {
--
2.25.1
On Tue, Mar 21, 2023 at 04:49:56PM +0530, Siddharth Vadapalli wrote:
> Add support for configuring the CPSW Ethernet Switch in SGMII mode.
>
> Depending on the SoC, allow selecting SGMII mode as a supported interface,
> based on the compatible used.
>
> Signed-off-by: Siddharth Vadapalli <[email protected]>
> ---
> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index cba8db14e160..d2ca1f2035f4 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -76,6 +76,7 @@
> #define AM65_CPSW_PORTN_REG_TS_CTL_LTYPE2 0x31C
>
> #define AM65_CPSW_SGMII_CONTROL_REG 0x010
> +#define AM65_CPSW_SGMII_MR_ADV_ABILITY_REG 0x018
> #define AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE BIT(0)
Isn't this misplaced? Shouldn't AM65_CPSW_SGMII_MR_ADV_ABILITY_REG come
after AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, rather than splitting that
from its register offset definition?
If the advertisement register is at 0x18, and the lower 16 bits is the
advertisement, are the link partner advertisement found in the upper
16 bits?
> #define AM65_CPSW_CTL_VLAN_AWARE BIT(1)
> @@ -1496,9 +1497,14 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in
> struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave);
> struct am65_cpsw_common *common = port->common;
>
> - if (common->pdata.extra_modes & BIT(state->interface))
> + if (common->pdata.extra_modes & BIT(state->interface)) {
> + if (state->interface == PHY_INTERFACE_MODE_SGMII)
> + writel(ADVERTISE_SGMII,
> + port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG);
> +
I think we can do better with this, by implementing proper PCS support.
It seems manufacturers tend to use bought-in IP for this, so have a
look at drivers/net/pcs/ to see whether any of those (or the one in
the Mediatek patch set on netdev that has recently been applied) will
idrive your hardware.
However, given the definition of AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE,
I suspect you won't find a compatible implementation.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hello Russell,
On 21-03-2023 16:59, Russell King (Oracle) wrote:
> On Tue, Mar 21, 2023 at 04:49:56PM +0530, Siddharth Vadapalli wrote:
>> Add support for configuring the CPSW Ethernet Switch in SGMII mode.
>>
>> Depending on the SoC, allow selecting SGMII mode as a supported interface,
>> based on the compatible used.
>>
>> Signed-off-by: Siddharth Vadapalli <[email protected]>
>> ---
>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> index cba8db14e160..d2ca1f2035f4 100644
>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> @@ -76,6 +76,7 @@
>> #define AM65_CPSW_PORTN_REG_TS_CTL_LTYPE2 0x31C
>>
>> #define AM65_CPSW_SGMII_CONTROL_REG 0x010
>> +#define AM65_CPSW_SGMII_MR_ADV_ABILITY_REG 0x018
>> #define AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE BIT(0)
>
> Isn't this misplaced? Shouldn't AM65_CPSW_SGMII_MR_ADV_ABILITY_REG come
> after AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, rather than splitting that
> from its register offset definition?
Thank you for reviewing the patch. The registers are as follows:
CONTROL_REG offset 0x10
STATUS_REG offset 0x14
MR_ADV_REG offset 0x18
Since the STATUS_REG is not used in the driver, its offset is omitted.
The next register is the MR_ADV_REG, which I placed after the
CONTROL_REG. I grouped the register offsets together, to represent the
order in which the registers are placed. Due to this, the
MR_ADV_ABILITY_REG offset is placed after the CONTROL_REG offset define.
Please let me know if I should move it after the CONTROL_MR_AN_ENABLE
define instead.
>
> If the advertisement register is at 0x18, and the lower 16 bits is the
> advertisement, are the link partner advertisement found in the upper
> 16 bits?
The MR_LP_ADV_ABILITY_REG is at offset 0x020, which is the the register
corresponding to the Link Partner advertised value. Also, the
AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE Bit is in the CONTROL_REG. The CPSW
Hardware specification describes the process of configuring the CPSW MAC
for SGMII mode as follows:
1. Write 0x1 (ADVERTISE_SGMII) to the MR_ADV_ABILITY_REG register.
2. Enable auto-negotiation in the CONTROL_REG by setting the
AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE bit.
>
>> #define AM65_CPSW_CTL_VLAN_AWARE BIT(1)
>> @@ -1496,9 +1497,14 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in
>> struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave);
>> struct am65_cpsw_common *common = port->common;
>>
>> - if (common->pdata.extra_modes & BIT(state->interface))
>> + if (common->pdata.extra_modes & BIT(state->interface)) {
>> + if (state->interface == PHY_INTERFACE_MODE_SGMII)
>> + writel(ADVERTISE_SGMII,
>> + port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG);
>> +
>
> I think we can do better with this, by implementing proper PCS support.
>
> It seems manufacturers tend to use bought-in IP for this, so have a
> look at drivers/net/pcs/ to see whether any of those (or the one in
> the Mediatek patch set on netdev that has recently been applied) will
> idrive your hardware.
>
> However, given the definition of AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE,
> I suspect you won't find a compatible implementation.
I have tested with an SGMII Ethernet PHY in the standard SGMII MAC2PHY
configuration. I am not sure if PCS support will be required or not. I
hope that the information shared above by me regarding the CPSW
Hardware's specification for configuring it in SGMII mode will help
determine what the right approach might be. Please let me know whether
the current implementation is acceptable or PCS support is necessary.
Regards,
Siddharth.
On Tue, Mar 21, 2023 at 07:04:50PM +0530, Siddharth Vadapalli wrote:
> Hello Russell,
>
> On 21-03-2023 16:59, Russell King (Oracle) wrote:
> > On Tue, Mar 21, 2023 at 04:49:56PM +0530, Siddharth Vadapalli wrote:
> >> Add support for configuring the CPSW Ethernet Switch in SGMII mode.
> >>
> >> Depending on the SoC, allow selecting SGMII mode as a supported interface,
> >> based on the compatible used.
> >>
> >> Signed-off-by: Siddharth Vadapalli <[email protected]>
> >> ---
> >> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 11 ++++++++++-
> >> 1 file changed, 10 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >> index cba8db14e160..d2ca1f2035f4 100644
> >> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >> @@ -76,6 +76,7 @@
> >> #define AM65_CPSW_PORTN_REG_TS_CTL_LTYPE2 0x31C
> >>
> >> #define AM65_CPSW_SGMII_CONTROL_REG 0x010
> >> +#define AM65_CPSW_SGMII_MR_ADV_ABILITY_REG 0x018
> >> #define AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE BIT(0)
> >
> > Isn't this misplaced? Shouldn't AM65_CPSW_SGMII_MR_ADV_ABILITY_REG come
> > after AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, rather than splitting that
> > from its register offset definition?
>
> Thank you for reviewing the patch. The registers are as follows:
> CONTROL_REG offset 0x10
> STATUS_REG offset 0x14
> MR_ADV_REG offset 0x18
>
> Since the STATUS_REG is not used in the driver, its offset is omitted.
> The next register is the MR_ADV_REG, which I placed after the
> CONTROL_REG. I grouped the register offsets together, to represent the
> order in which the registers are placed. Due to this, the
> MR_ADV_ABILITY_REG offset is placed after the CONTROL_REG offset define.
>
> Please let me know if I should move it after the CONTROL_MR_AN_ENABLE
> define instead.
Well, it's up to you - whether you wish to group the register offsets
separately from the bit definitions for those registers, or whether
you wish to describe the register offset and its associated bit
definitions in one group before moving on to the next register.
> > If the advertisement register is at 0x18, and the lower 16 bits is the
> > advertisement, are the link partner advertisement found in the upper
> > 16 bits?
>
> The MR_LP_ADV_ABILITY_REG is at offset 0x020, which is the the register
> corresponding to the Link Partner advertised value. Also, the
> AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE Bit is in the CONTROL_REG. The CPSW
> Hardware specification describes the process of configuring the CPSW MAC
> for SGMII mode as follows:
> 1. Write 0x1 (ADVERTISE_SGMII) to the MR_ADV_ABILITY_REG register.
> 2. Enable auto-negotiation in the CONTROL_REG by setting the
> AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE bit.
Good to hear that there is a link partner register.
> >> #define AM65_CPSW_CTL_VLAN_AWARE BIT(1)
> >> @@ -1496,9 +1497,14 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in
> >> struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave);
> >> struct am65_cpsw_common *common = port->common;
> >>
> >> - if (common->pdata.extra_modes & BIT(state->interface))
> >> + if (common->pdata.extra_modes & BIT(state->interface)) {
> >> + if (state->interface == PHY_INTERFACE_MODE_SGMII)
> >> + writel(ADVERTISE_SGMII,
> >> + port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG);
> >> +
> >
> > I think we can do better with this, by implementing proper PCS support.
> >
> > It seems manufacturers tend to use bought-in IP for this, so have a
> > look at drivers/net/pcs/ to see whether any of those (or the one in
> > the Mediatek patch set on netdev that has recently been applied) will
> > idrive your hardware.
> >
> > However, given the definition of AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE,
> > I suspect you won't find a compatible implementation.
>
> I have tested with an SGMII Ethernet PHY in the standard SGMII MAC2PHY
> configuration. I am not sure if PCS support will be required or not. I
> hope that the information shared above by me regarding the CPSW
> Hardware's specification for configuring it in SGMII mode will help
> determine what the right approach might be. Please let me know whether
> the current implementation is acceptable or PCS support is necessary.
Nevertheless, this SGMII block is a PCS, and if you're going to want to
support inband mode (e.g. to read the SGMII word from the PHY), or if
someone ever wants to use 1000base-X, you're going to need to implement
this properly as a PCS.
That said, it can be converted later, so isn't a blocking sisue.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hello Russell,
On 21-03-2023 21:08, Russell King (Oracle) wrote:
> On Tue, Mar 21, 2023 at 07:04:50PM +0530, Siddharth Vadapalli wrote:
>> Hello Russell,
>>
>> On 21-03-2023 16:59, Russell King (Oracle) wrote:
>>> On Tue, Mar 21, 2023 at 04:49:56PM +0530, Siddharth Vadapalli wrote:
>>>> Add support for configuring the CPSW Ethernet Switch in SGMII mode.
>>>>
>>>> Depending on the SoC, allow selecting SGMII mode as a supported interface,
>>>> based on the compatible used.
>>>>
>>>> Signed-off-by: Siddharth Vadapalli <[email protected]>
>>>> ---
>>>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 11 ++++++++++-
>>>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>> index cba8db14e160..d2ca1f2035f4 100644
>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>> @@ -76,6 +76,7 @@
>>>> #define AM65_CPSW_PORTN_REG_TS_CTL_LTYPE2 0x31C
>>>>
>>>> #define AM65_CPSW_SGMII_CONTROL_REG 0x010
>>>> +#define AM65_CPSW_SGMII_MR_ADV_ABILITY_REG 0x018
>>>> #define AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE BIT(0)
>>>
>>> Isn't this misplaced? Shouldn't AM65_CPSW_SGMII_MR_ADV_ABILITY_REG come
>>> after AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, rather than splitting that
>>> from its register offset definition?
>>
>> Thank you for reviewing the patch. The registers are as follows:
>> CONTROL_REG offset 0x10
>> STATUS_REG offset 0x14
>> MR_ADV_REG offset 0x18
>>
>> Since the STATUS_REG is not used in the driver, its offset is omitted.
>> The next register is the MR_ADV_REG, which I placed after the
>> CONTROL_REG. I grouped the register offsets together, to represent the
>> order in which the registers are placed. Due to this, the
>> MR_ADV_ABILITY_REG offset is placed after the CONTROL_REG offset define.
>>
>> Please let me know if I should move it after the CONTROL_MR_AN_ENABLE
>> define instead.
>
> Well, it's up to you - whether you wish to group the register offsets
> separately from the bit definitions for those registers, or whether
> you wish to describe the register offset and its associated bit
> definitions in one group before moving on to the next register.
>
>>> If the advertisement register is at 0x18, and the lower 16 bits is the
>>> advertisement, are the link partner advertisement found in the upper
>>> 16 bits?
>>
>> The MR_LP_ADV_ABILITY_REG is at offset 0x020, which is the the register
>> corresponding to the Link Partner advertised value. Also, the
>> AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE Bit is in the CONTROL_REG. The CPSW
>> Hardware specification describes the process of configuring the CPSW MAC
>> for SGMII mode as follows:
>> 1. Write 0x1 (ADVERTISE_SGMII) to the MR_ADV_ABILITY_REG register.
>> 2. Enable auto-negotiation in the CONTROL_REG by setting the
>> AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE bit.
>
> Good to hear that there is a link partner register.
>
>>>> #define AM65_CPSW_CTL_VLAN_AWARE BIT(1)
>>>> @@ -1496,9 +1497,14 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in
>>>> struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave);
>>>> struct am65_cpsw_common *common = port->common;
>>>>
>>>> - if (common->pdata.extra_modes & BIT(state->interface))
>>>> + if (common->pdata.extra_modes & BIT(state->interface)) {
>>>> + if (state->interface == PHY_INTERFACE_MODE_SGMII)
>>>> + writel(ADVERTISE_SGMII,
>>>> + port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG);
>>>> +
>>>
>>> I think we can do better with this, by implementing proper PCS support.
>>>
>>> It seems manufacturers tend to use bought-in IP for this, so have a
>>> look at drivers/net/pcs/ to see whether any of those (or the one in
>>> the Mediatek patch set on netdev that has recently been applied) will
>>> idrive your hardware.
>>>
>>> However, given the definition of AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE,
>>> I suspect you won't find a compatible implementation.
>>
>> I have tested with an SGMII Ethernet PHY in the standard SGMII MAC2PHY
>> configuration. I am not sure if PCS support will be required or not. I
>> hope that the information shared above by me regarding the CPSW
>> Hardware's specification for configuring it in SGMII mode will help
>> determine what the right approach might be. Please let me know whether
>> the current implementation is acceptable or PCS support is necessary.
>
> Nevertheless, this SGMII block is a PCS, and if you're going to want to
> support inband mode (e.g. to read the SGMII word from the PHY), or if
> someone ever wants to use 1000base-X, you're going to need to implement
> this properly as a PCS.
>
> That said, it can be converted later, so isn't a blocking sisue.
Thank you for clarifying. I will work on converting it to PCS in a
future series.
Regards,
Siddharth.
Hi Russell,
On Tue, 2023-03-21 at 15:38 +0000, Russell King (Oracle) wrote:
> On Tue, Mar 21, 2023 at 07:04:50PM +0530, Siddharth Vadapalli wrote:
> > Hello Russell,
> >
> > On 21-03-2023 16:59, Russell King (Oracle) wrote:
> > > On Tue, Mar 21, 2023 at 04:49:56PM +0530, Siddharth Vadapalli wrote:
> > > > Add support for configuring the CPSW Ethernet Switch in SGMII mode.
> > > >
> > > > Depending on the SoC, allow selecting SGMII mode as a supported interface,
> > > > based on the compatible used.
> > > >
> > > > Signed-off-by: Siddharth Vadapalli <[email protected]>
> > > > ---
> > > > drivers/net/ethernet/ti/am65-cpsw-nuss.c | 11 ++++++++++-
> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > > index cba8db14e160..d2ca1f2035f4 100644
> > > > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > > @@ -76,6 +76,7 @@
> > > > #define AM65_CPSW_PORTN_REG_TS_CTL_LTYPE2 0x31C
> > > >
> > > > #define AM65_CPSW_SGMII_CONTROL_REG 0x010
> > > > +#define AM65_CPSW_SGMII_MR_ADV_ABILITY_REG 0x018
> > > > #define AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE BIT(0)
> > >
> > > Isn't this misplaced? Shouldn't AM65_CPSW_SGMII_MR_ADV_ABILITY_REG come
> > > after AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, rather than splitting that
> > > from its register offset definition?
> >
> > Thank you for reviewing the patch. The registers are as follows:
> > CONTROL_REG offset 0x10
> > STATUS_REG offset 0x14
> > MR_ADV_REG offset 0x18
> >
> > Since the STATUS_REG is not used in the driver, its offset is omitted.
> > The next register is the MR_ADV_REG, which I placed after the
> > CONTROL_REG. I grouped the register offsets together, to represent the
> > order in which the registers are placed. Due to this, the
> > MR_ADV_ABILITY_REG offset is placed after the CONTROL_REG offset define.
> >
> > Please let me know if I should move it after the CONTROL_MR_AN_ENABLE
> > define instead.
>
> Well, it's up to you - whether you wish to group the register offsets
> separately from the bit definitions for those registers, or whether
> you wish to describe the register offset and its associated bit
> definitions in one group before moving on to the next register.
>
> > > If the advertisement register is at 0x18, and the lower 16 bits is the
> > > advertisement, are the link partner advertisement found in the upper
> > > 16 bits?
> >
> > The MR_LP_ADV_ABILITY_REG is at offset 0x020, which is the the register
> > corresponding to the Link Partner advertised value. Also, the
> > AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE Bit is in the CONTROL_REG. The CPSW
> > Hardware specification describes the process of configuring the CPSW MAC
> > for SGMII mode as follows:
> > 1. Write 0x1 (ADVERTISE_SGMII) to the MR_ADV_ABILITY_REG register.
> > 2. Enable auto-negotiation in the CONTROL_REG by setting the
> > AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE bit.
>
> Good to hear that there is a link partner register.
>
> > > > #define AM65_CPSW_CTL_VLAN_AWARE BIT(1)
> > > > @@ -1496,9 +1497,14 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in
> > > > struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave);
> > > > struct am65_cpsw_common *common = port->common;
> > > >
> > > > - if (common->pdata.extra_modes & BIT(state->interface))
> > > > + if (common->pdata.extra_modes & BIT(state->interface)) {
> > > > + if (state->interface == PHY_INTERFACE_MODE_SGMII)
> > > > + writel(ADVERTISE_SGMII,
> > > > + port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG);
> > > > +
> > >
> > > I think we can do better with this, by implementing proper PCS support.
> > >
> > > It seems manufacturers tend to use bought-in IP for this, so have a
> > > look at drivers/net/pcs/ to see whether any of those (or the one in
> > > the Mediatek patch set on netdev that has recently been applied) will
> > > idrive your hardware.
> > >
> > > However, given the definition of AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE,
> > > I suspect you won't find a compatible implementation.
> >
> > I have tested with an SGMII Ethernet PHY in the standard SGMII MAC2PHY
> > configuration. I am not sure if PCS support will be required or not. I
> > hope that the information shared above by me regarding the CPSW
> > Hardware's specification for configuring it in SGMII mode will help
> > determine what the right approach might be. Please let me know whether
> > the current implementation is acceptable or PCS support is necessary.
>
> Nevertheless, this SGMII block is a PCS, and if you're going to want to
> support inband mode (e.g. to read the SGMII word from the PHY), or if
> someone ever wants to use 1000base-X, you're going to need to implement
> this properly as a PCS.
>
> That said, it can be converted later, so isn't a blocking sisue.
Just to be on the same page, I read all the above as you do accept/do
not oppose to this series in the current form, am I correct?
Thanks,
Paolo
On Wed, Mar 22, 2023 at 03:49:41PM +0100, Paolo Abeni wrote:
> Hi Russell,
>
> On Tue, 2023-03-21 at 15:38 +0000, Russell King (Oracle) wrote:
> > On Tue, Mar 21, 2023 at 07:04:50PM +0530, Siddharth Vadapalli wrote:
> > > Hello Russell,
> > >
> > > On 21-03-2023 16:59, Russell King (Oracle) wrote:
> > > > On Tue, Mar 21, 2023 at 04:49:56PM +0530, Siddharth Vadapalli wrote:
> > > > > Add support for configuring the CPSW Ethernet Switch in SGMII mode.
> > > > >
> > > > > Depending on the SoC, allow selecting SGMII mode as a supported interface,
> > > > > based on the compatible used.
> > > > >
> > > > > Signed-off-by: Siddharth Vadapalli <[email protected]>
> > > > > ---
> > > > > drivers/net/ethernet/ti/am65-cpsw-nuss.c | 11 ++++++++++-
> > > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > > > index cba8db14e160..d2ca1f2035f4 100644
> > > > > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > > > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > > > @@ -76,6 +76,7 @@
> > > > > #define AM65_CPSW_PORTN_REG_TS_CTL_LTYPE2 0x31C
> > > > >
> > > > > #define AM65_CPSW_SGMII_CONTROL_REG 0x010
> > > > > +#define AM65_CPSW_SGMII_MR_ADV_ABILITY_REG 0x018
> > > > > #define AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE BIT(0)
> > > >
> > > > Isn't this misplaced? Shouldn't AM65_CPSW_SGMII_MR_ADV_ABILITY_REG come
> > > > after AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, rather than splitting that
> > > > from its register offset definition?
> > >
> > > Thank you for reviewing the patch. The registers are as follows:
> > > CONTROL_REG offset 0x10
> > > STATUS_REG offset 0x14
> > > MR_ADV_REG offset 0x18
> > >
> > > Since the STATUS_REG is not used in the driver, its offset is omitted.
> > > The next register is the MR_ADV_REG, which I placed after the
> > > CONTROL_REG. I grouped the register offsets together, to represent the
> > > order in which the registers are placed. Due to this, the
> > > MR_ADV_ABILITY_REG offset is placed after the CONTROL_REG offset define.
> > >
> > > Please let me know if I should move it after the CONTROL_MR_AN_ENABLE
> > > define instead.
> >
> > Well, it's up to you - whether you wish to group the register offsets
> > separately from the bit definitions for those registers, or whether
> > you wish to describe the register offset and its associated bit
> > definitions in one group before moving on to the next register.
> >
> > > > If the advertisement register is at 0x18, and the lower 16 bits is the
> > > > advertisement, are the link partner advertisement found in the upper
> > > > 16 bits?
> > >
> > > The MR_LP_ADV_ABILITY_REG is at offset 0x020, which is the the register
> > > corresponding to the Link Partner advertised value. Also, the
> > > AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE Bit is in the CONTROL_REG. The CPSW
> > > Hardware specification describes the process of configuring the CPSW MAC
> > > for SGMII mode as follows:
> > > 1. Write 0x1 (ADVERTISE_SGMII) to the MR_ADV_ABILITY_REG register.
> > > 2. Enable auto-negotiation in the CONTROL_REG by setting the
> > > AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE bit.
> >
> > Good to hear that there is a link partner register.
> >
> > > > > #define AM65_CPSW_CTL_VLAN_AWARE BIT(1)
> > > > > @@ -1496,9 +1497,14 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in
> > > > > struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave);
> > > > > struct am65_cpsw_common *common = port->common;
> > > > >
> > > > > - if (common->pdata.extra_modes & BIT(state->interface))
> > > > > + if (common->pdata.extra_modes & BIT(state->interface)) {
> > > > > + if (state->interface == PHY_INTERFACE_MODE_SGMII)
> > > > > + writel(ADVERTISE_SGMII,
> > > > > + port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG);
> > > > > +
> > > >
> > > > I think we can do better with this, by implementing proper PCS support.
> > > >
> > > > It seems manufacturers tend to use bought-in IP for this, so have a
> > > > look at drivers/net/pcs/ to see whether any of those (or the one in
> > > > the Mediatek patch set on netdev that has recently been applied) will
> > > > idrive your hardware.
> > > >
> > > > However, given the definition of AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE,
> > > > I suspect you won't find a compatible implementation.
> > >
> > > I have tested with an SGMII Ethernet PHY in the standard SGMII MAC2PHY
> > > configuration. I am not sure if PCS support will be required or not. I
> > > hope that the information shared above by me regarding the CPSW
> > > Hardware's specification for configuring it in SGMII mode will help
> > > determine what the right approach might be. Please let me know whether
> > > the current implementation is acceptable or PCS support is necessary.
> >
> > Nevertheless, this SGMII block is a PCS, and if you're going to want to
> > support inband mode (e.g. to read the SGMII word from the PHY), or if
> > someone ever wants to use 1000base-X, you're going to need to implement
> > this properly as a PCS.
> >
> > That said, it can be converted later, so isn't a blocking sisue.
>
> Just to be on the same page, I read all the above as you do accept/do
> not oppose to this series in the current form, am I correct?
I don't oppose this series.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:
On Tue, 21 Mar 2023 16:49:54 +0530 you wrote:
> Hello,
>
> This series adds support to configure the CPSW Ethernet Switch in SGMII
> mode, using the am65-cpsw-nuss driver. SGMII mode is supported by the
> CPSWxG instances on TI's J7200 and J721E SoCs. Thus, SGMII mode is added
> in the list of extra_modes for the appropriate compatibles corresponding
> to the aforementioned SoCs.
>
> [...]
Here is the summary with links:
- [net-next,1/4] net: ethernet: ti: am65-cpsw: Simplify setting supported interface
https://git.kernel.org/netdev/net-next/c/a2935a1cd85f
- [net-next,2/4] net: ethernet: ti: am65-cpsw: Add support for SGMII mode
https://git.kernel.org/netdev/net-next/c/e0f72db37547
- [net-next,3/4] net: ethernet: ti: am65-cpsw: Enable SGMII mode for J7200
https://git.kernel.org/netdev/net-next/c/2e20e764f24e
- [net-next,4/4] net: ethernet: ti: am65-cpsw: Enable SGMII mode for J721E
https://git.kernel.org/netdev/net-next/c/186016da9cca
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html