| Message ID | 20230403110106.983994-2-s-vadapalli@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for J784S4 CPSW9G | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 18 this patch: 18 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 18 this patch: 18 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 18 this patch: 18 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 27 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, Apr 03, 2023 at 04:31:04PM +0530, Siddharth Vadapalli wrote: > Move the interface mode specific configuration to the mac_config() > callback am65_cpsw_nuss_mac_config(). > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > --- > drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > index d17757ecbf42..74e099828978 100644 > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in > phylink_config); > struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave); > struct am65_cpsw_common *common = port->common; > + u32 mac_control = 0; > > if (common->pdata.extra_modes & BIT(state->interface)) { > - if (state->interface == PHY_INTERFACE_MODE_SGMII) > + if (state->interface == PHY_INTERFACE_MODE_SGMII) { > + mac_control |= CPSW_SL_CTL_EXT_EN; > writel(ADVERTISE_SGMII, > port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG); > + } > > + if (mac_control) > + cpsw_sl_ctl_set(port->slave.mac_sl, mac_control); > writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, > port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG); > } > @@ -1553,8 +1558,7 @@ 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; > + /* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */ > 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; I'm afraid I can see you haven't thought this patch through properly. am65_cpsw_nuss_mac_link_down() will call cpsw_sl_ctl_reset(port->slave.mac_sl); which has the effect of clearing to zero the entire MAC control register. This will clear CPSW_SL_CTL_EXT_EN that was set in am65_cpsw_nuss_mac_config() which is not what you want to be doing. Given that we have the 10Mbps issue with RGMII, I think what you want to be doing is: 1. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_config() if in SGMII mode, otherwise clear this bit. 2. Clear the mac_control register in am65_cpsw_nuss_mac_link_down() if in RMGII mode, otherwise preserve the state of CPSW_SL_CTL_EXT_EN but clear all other bits. 3. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_link_up() if in RGMII mode and 10Mbps.
Hello Russell, Thank you for reviewing the patch. On 03/04/23 16:38, Russell King (Oracle) wrote: > On Mon, Apr 03, 2023 at 04:31:04PM +0530, Siddharth Vadapalli wrote: >> Move the interface mode specific configuration to the mac_config() >> callback am65_cpsw_nuss_mac_config(). >> >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >> --- >> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> index d17757ecbf42..74e099828978 100644 >> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in >> phylink_config); >> struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave); >> struct am65_cpsw_common *common = port->common; >> + u32 mac_control = 0; >> >> if (common->pdata.extra_modes & BIT(state->interface)) { >> - if (state->interface == PHY_INTERFACE_MODE_SGMII) >> + if (state->interface == PHY_INTERFACE_MODE_SGMII) { >> + mac_control |= CPSW_SL_CTL_EXT_EN; >> writel(ADVERTISE_SGMII, >> port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG); >> + } >> >> + if (mac_control) >> + cpsw_sl_ctl_set(port->slave.mac_sl, mac_control); >> writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, >> port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG); >> } >> @@ -1553,8 +1558,7 @@ 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; >> + /* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */ >> 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; > > I'm afraid I can see you haven't thought this patch through properly. > > am65_cpsw_nuss_mac_link_down() will call > cpsw_sl_ctl_reset(port->slave.mac_sl); which has the effect of clearing > to zero the entire MAC control register. This will clear > CPSW_SL_CTL_EXT_EN that was set in am65_cpsw_nuss_mac_config() which is > not what you want to be doing. Right! I missed noticing this. It appeared to me that simply moving the code from mac_link_up() to mac_config() would suffice. However, as rightly pointed out by you, it doesn't account for the case where the interface state is toggled. > > Given that we have the 10Mbps issue with RGMII, I think what you want > to be doing is: > > 1. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_config() if in SGMII > mode, otherwise clear this bit. > > 2. Clear the mac_control register in am65_cpsw_nuss_mac_link_down() > if in RMGII mode, otherwise preserve the state of > CPSW_SL_CTL_EXT_EN but clear all other bits. > > 3. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_link_up() if in > RGMII mode and 10Mbps. > I will implement these changes and post the v3 series. Thank you for the feedback. Please let me know if I may add a "Suggested-by" tag. Regards, Siddharth.
On 03-04-2023 16:38, Russell King (Oracle) wrote: > On Mon, Apr 03, 2023 at 04:31:04PM +0530, Siddharth Vadapalli wrote: >> Move the interface mode specific configuration to the mac_config() >> callback am65_cpsw_nuss_mac_config(). >> >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >> --- >> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> index d17757ecbf42..74e099828978 100644 >> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in >> phylink_config); >> struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave); >> struct am65_cpsw_common *common = port->common; >> + u32 mac_control = 0; >> >> if (common->pdata.extra_modes & BIT(state->interface)) { >> - if (state->interface == PHY_INTERFACE_MODE_SGMII) >> + if (state->interface == PHY_INTERFACE_MODE_SGMII) { >> + mac_control |= CPSW_SL_CTL_EXT_EN; >> writel(ADVERTISE_SGMII, >> port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG); >> + } >> >> + if (mac_control) >> + cpsw_sl_ctl_set(port->slave.mac_sl, mac_control); >> writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, >> port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG); >> } >> @@ -1553,8 +1558,7 @@ 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; >> + /* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */ >> 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; > > I'm afraid I can see you haven't thought this patch through properly. > > am65_cpsw_nuss_mac_link_down() will call > cpsw_sl_ctl_reset(port->slave.mac_sl); which has the effect of clearing > to zero the entire MAC control register. This will clear > CPSW_SL_CTL_EXT_EN that was set in am65_cpsw_nuss_mac_config() which is > not what you want to be doing. > > Given that we have the 10Mbps issue with RGMII, I think what you want > to be doing is: > > 1. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_config() if in SGMII > mode, otherwise clear this bit. > > 2. Clear the mac_control register in am65_cpsw_nuss_mac_link_down() > if in RMGII mode, otherwise preserve the state of > CPSW_SL_CTL_EXT_EN but clear all other bits. > > 3. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_link_up() if in > RGMII mode and 10Mbps. I plan to implement it as follows: 1. Add a member "u32 mode_config" to "struct am65_cpsw_slave_data" in "am65-cpsw-nuss.h". 2. In am65_cpsw_nuss_mac_config(), store the value of mac_control in "port->slave.mode_config". 3. In am65_cpsw_nuss_mac_link_down(), after the reset via cpsw_sl_ctl_reset(), execute: cpsw_sl_ctl_set(port->slave.mac_sl, port->slave.mode_config) in order to restore the configuration performed in am65_cpsw_nuss_mac_config(). Please let me know in case of any suggestions to implement it in a better manner. Regards, Siddharth.
On Mon, Apr 03, 2023 at 06:31:52PM +0530, Siddharth Vadapalli wrote: > > > On 03-04-2023 16:38, Russell King (Oracle) wrote: > > On Mon, Apr 03, 2023 at 04:31:04PM +0530, Siddharth Vadapalli wrote: > >> Move the interface mode specific configuration to the mac_config() > >> callback am65_cpsw_nuss_mac_config(). > >> > >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > >> --- > >> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++--- > >> 1 file changed, 7 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >> index d17757ecbf42..74e099828978 100644 > >> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >> @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in > >> phylink_config); > >> struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave); > >> struct am65_cpsw_common *common = port->common; > >> + u32 mac_control = 0; > >> > >> if (common->pdata.extra_modes & BIT(state->interface)) { > >> - if (state->interface == PHY_INTERFACE_MODE_SGMII) > >> + if (state->interface == PHY_INTERFACE_MODE_SGMII) { > >> + mac_control |= CPSW_SL_CTL_EXT_EN; > >> writel(ADVERTISE_SGMII, > >> port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG); > >> + } > >> > >> + if (mac_control) > >> + cpsw_sl_ctl_set(port->slave.mac_sl, mac_control); > >> writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, > >> port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG); > >> } > >> @@ -1553,8 +1558,7 @@ 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; > >> + /* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */ > >> 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; > > > > I'm afraid I can see you haven't thought this patch through properly. > > > > am65_cpsw_nuss_mac_link_down() will call > > cpsw_sl_ctl_reset(port->slave.mac_sl); which has the effect of clearing > > to zero the entire MAC control register. This will clear > > CPSW_SL_CTL_EXT_EN that was set in am65_cpsw_nuss_mac_config() which is > > not what you want to be doing. > > > > Given that we have the 10Mbps issue with RGMII, I think what you want > > to be doing is: > > > > 1. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_config() if in SGMII > > mode, otherwise clear this bit. > > > > 2. Clear the mac_control register in am65_cpsw_nuss_mac_link_down() > > if in RMGII mode, otherwise preserve the state of > > CPSW_SL_CTL_EXT_EN but clear all other bits. > > > > 3. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_link_up() if in > > RGMII mode and 10Mbps. > > I plan to implement it as follows: > 1. Add a member "u32 mode_config" to "struct am65_cpsw_slave_data" in > "am65-cpsw-nuss.h". > 2. In am65_cpsw_nuss_mac_config(), store the value of mac_control in > "port->slave.mode_config". > 3. In am65_cpsw_nuss_mac_link_down(), after the reset via > cpsw_sl_ctl_reset(), execute: > cpsw_sl_ctl_set(port->slave.mac_sl, port->slave.mode_config) in order to > restore the configuration performed in am65_cpsw_nuss_mac_config(). > > Please let me know in case of any suggestions to implement it in a > better manner. Do you think this complexity is really worth it? Let's look at what's available: cpsw_sl_ctl_set() - sets bits in the mac control register cpsw_sl_ctl_clr() - clears bits in the mac control register cpsw_sl_ctl_reset() - sets the mac control register to zero So, in mac_config(), we can do: if (interface == SGMII) cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN); else cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN); in mac_link_down(): u32 ctl; ctl = CPSW_SL_CTL_GMII_EN | CPSW_SL_CTL_GIG | CPSW_SL_CTL_IFCTL_A | CPSW_SL_CTL_FULLDUPLEX | CPSW_SL_CTL_RX_FLOW_EN | CPSW_SL_CTL_TX_FLOW_EN; if (phy_interface_mode_is_rgmii(interface)) ctl |= CPSW_SL_CTL_EXT_EN; cpsw_sl_ctl_clr(ctl); This ensures that we don't touch any bits in mac_link_down() which we aren't modifying in the corresponding mac_link_up() implementation. Q: do we really need to clear the mac control register on link down? If we don't, then we can do better, but we need an additional helper which allows read-modify-write of the mac control register using a mask value and a value of bits to set. Then we can have mac_link_up() setting and clearing the bits as necessary - but I would still keep the clearing of CPSW_SL_CTL_EXT_EN for RGMII modes in mac_link_down() for now.
On 03-04-2023 18:43, Russell King (Oracle) wrote: > On Mon, Apr 03, 2023 at 06:31:52PM +0530, Siddharth Vadapalli wrote: >> >> >> On 03-04-2023 16:38, Russell King (Oracle) wrote: >>> On Mon, Apr 03, 2023 at 04:31:04PM +0530, Siddharth Vadapalli wrote: >>>> Move the interface mode specific configuration to the mac_config() >>>> callback am65_cpsw_nuss_mac_config(). >>>> >>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>> --- >>>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++--- >>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>> index d17757ecbf42..74e099828978 100644 >>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>> @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in >>>> phylink_config); >>>> struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave); >>>> struct am65_cpsw_common *common = port->common; >>>> + u32 mac_control = 0; >>>> >>>> if (common->pdata.extra_modes & BIT(state->interface)) { >>>> - if (state->interface == PHY_INTERFACE_MODE_SGMII) >>>> + if (state->interface == PHY_INTERFACE_MODE_SGMII) { >>>> + mac_control |= CPSW_SL_CTL_EXT_EN; >>>> writel(ADVERTISE_SGMII, >>>> port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG); >>>> + } >>>> >>>> + if (mac_control) >>>> + cpsw_sl_ctl_set(port->slave.mac_sl, mac_control); >>>> writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, >>>> port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG); >>>> } >>>> @@ -1553,8 +1558,7 @@ 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; >>>> + /* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */ >>>> 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; >>> >>> I'm afraid I can see you haven't thought this patch through properly. >>> >>> am65_cpsw_nuss_mac_link_down() will call >>> cpsw_sl_ctl_reset(port->slave.mac_sl); which has the effect of clearing >>> to zero the entire MAC control register. This will clear >>> CPSW_SL_CTL_EXT_EN that was set in am65_cpsw_nuss_mac_config() which is >>> not what you want to be doing. >>> >>> Given that we have the 10Mbps issue with RGMII, I think what you want >>> to be doing is: >>> >>> 1. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_config() if in SGMII >>> mode, otherwise clear this bit. >>> >>> 2. Clear the mac_control register in am65_cpsw_nuss_mac_link_down() >>> if in RMGII mode, otherwise preserve the state of >>> CPSW_SL_CTL_EXT_EN but clear all other bits. >>> >>> 3. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_link_up() if in >>> RGMII mode and 10Mbps. >> >> I plan to implement it as follows: >> 1. Add a member "u32 mode_config" to "struct am65_cpsw_slave_data" in >> "am65-cpsw-nuss.h". >> 2. In am65_cpsw_nuss_mac_config(), store the value of mac_control in >> "port->slave.mode_config". >> 3. In am65_cpsw_nuss_mac_link_down(), after the reset via >> cpsw_sl_ctl_reset(), execute: >> cpsw_sl_ctl_set(port->slave.mac_sl, port->slave.mode_config) in order to >> restore the configuration performed in am65_cpsw_nuss_mac_config(). >> >> Please let me know in case of any suggestions to implement it in a >> better manner. > > Do you think this complexity is really worth it? > > Let's look at what's available: > > cpsw_sl_ctl_set() - sets bits in the mac control register > cpsw_sl_ctl_clr() - clears bits in the mac control register > cpsw_sl_ctl_reset() - sets the mac control register to zero > > So, in mac_config(), we can do: > > if (interface == SGMII) > cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN); > else > cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN); While this will work for patch 1/3, once I add support for USXGMII mode as in patch 3/3, I believe that I have to invert it, beginning by invoking a cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN) at the start in mac_config() followed by switching through the modes. If the mode is SGMII, then I invoke cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN), along with the write to the MR_ADV_ABILITY_REG register to advertise SGMII. If the mode is USXGMII, then I invoke: cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN) > > > in mac_link_down(): > > u32 ctl; > > ctl = CPSW_SL_CTL_GMII_EN | CPSW_SL_CTL_GIG | > CPSW_SL_CTL_IFCTL_A | CPSW_SL_CTL_FULLDUPLEX | > CPSW_SL_CTL_RX_FLOW_EN | CPSW_SL_CTL_TX_FLOW_EN; > if (phy_interface_mode_is_rgmii(interface)) > ctl |= CPSW_SL_CTL_EXT_EN; > > cpsw_sl_ctl_clr(ctl); > > This ensures that we don't touch any bits in mac_link_down() which we > aren't modifying in the corresponding mac_link_up() implementation. This is very helpful. Thank you for the suggestion :) > > Q: do we really need to clear the mac control register on link down? > If we don't, then we can do better, but we need an additional helper > which allows read-modify-write of the mac control register using a > mask value and a value of bits to set. Then we can have mac_link_up() > setting and clearing the bits as necessary - but I would still keep > the clearing of CPSW_SL_CTL_EXT_EN for RGMII modes in mac_link_down() > for now. Clearing the entire register is not necessary. As long as those bits that were set during mac_link_up() are cleared, it should be sufficient. For now, I will implement the changes assuming that the mac control register will be cleared on link down, based on the above suggestions provided by you. Regards, Siddharth.
On Mon, Apr 03, 2023 at 07:20:21PM +0530, Siddharth Vadapalli wrote: > > > On 03-04-2023 18:43, Russell King (Oracle) wrote: > > On Mon, Apr 03, 2023 at 06:31:52PM +0530, Siddharth Vadapalli wrote: > >> > >> > >> On 03-04-2023 16:38, Russell King (Oracle) wrote: > >>> On Mon, Apr 03, 2023 at 04:31:04PM +0530, Siddharth Vadapalli wrote: > >>>> Move the interface mode specific configuration to the mac_config() > >>>> callback am65_cpsw_nuss_mac_config(). > >>>> > >>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > >>>> --- > >>>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++--- > >>>> 1 file changed, 7 insertions(+), 3 deletions(-) > >>>> > >>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >>>> index d17757ecbf42..74e099828978 100644 > >>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >>>> @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in > >>>> phylink_config); > >>>> struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave); > >>>> struct am65_cpsw_common *common = port->common; > >>>> + u32 mac_control = 0; > >>>> > >>>> if (common->pdata.extra_modes & BIT(state->interface)) { > >>>> - if (state->interface == PHY_INTERFACE_MODE_SGMII) > >>>> + if (state->interface == PHY_INTERFACE_MODE_SGMII) { > >>>> + mac_control |= CPSW_SL_CTL_EXT_EN; > >>>> writel(ADVERTISE_SGMII, > >>>> port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG); > >>>> + } > >>>> > >>>> + if (mac_control) > >>>> + cpsw_sl_ctl_set(port->slave.mac_sl, mac_control); > >>>> writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, > >>>> port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG); > >>>> } > >>>> @@ -1553,8 +1558,7 @@ 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; > >>>> + /* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */ > >>>> 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; > >>> > >>> I'm afraid I can see you haven't thought this patch through properly. > >>> > >>> am65_cpsw_nuss_mac_link_down() will call > >>> cpsw_sl_ctl_reset(port->slave.mac_sl); which has the effect of clearing > >>> to zero the entire MAC control register. This will clear > >>> CPSW_SL_CTL_EXT_EN that was set in am65_cpsw_nuss_mac_config() which is > >>> not what you want to be doing. > >>> > >>> Given that we have the 10Mbps issue with RGMII, I think what you want > >>> to be doing is: > >>> > >>> 1. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_config() if in SGMII > >>> mode, otherwise clear this bit. > >>> > >>> 2. Clear the mac_control register in am65_cpsw_nuss_mac_link_down() > >>> if in RMGII mode, otherwise preserve the state of > >>> CPSW_SL_CTL_EXT_EN but clear all other bits. > >>> > >>> 3. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_link_up() if in > >>> RGMII mode and 10Mbps. > >> > >> I plan to implement it as follows: > >> 1. Add a member "u32 mode_config" to "struct am65_cpsw_slave_data" in > >> "am65-cpsw-nuss.h". > >> 2. In am65_cpsw_nuss_mac_config(), store the value of mac_control in > >> "port->slave.mode_config". > >> 3. In am65_cpsw_nuss_mac_link_down(), after the reset via > >> cpsw_sl_ctl_reset(), execute: > >> cpsw_sl_ctl_set(port->slave.mac_sl, port->slave.mode_config) in order to > >> restore the configuration performed in am65_cpsw_nuss_mac_config(). > >> > >> Please let me know in case of any suggestions to implement it in a > >> better manner. > > > > Do you think this complexity is really worth it? > > > > Let's look at what's available: > > > > cpsw_sl_ctl_set() - sets bits in the mac control register > > cpsw_sl_ctl_clr() - clears bits in the mac control register > > cpsw_sl_ctl_reset() - sets the mac control register to zero > > > > So, in mac_config(), we can do: > > > > if (interface == SGMII) > > cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN); > > else > > cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN); > > While this will work for patch 1/3, once I add support for USXGMII mode > as in patch 3/3, I believe that I have to invert it, beginning by > invoking a cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN) at the start in > mac_config() followed by switching through the modes. If the mode is > SGMII, then I invoke cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN), along with the > write to the MR_ADV_ABILITY_REG register to advertise SGMII. If the mode > is USXGMII, then I invoke: > cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN) For patch 1, I did leave out the write for MR_ADV_ABILITY_REG, I had assumed you'd get the idea on that and merge the if() condition you already had with my suggestion above (which isn't literal code!) In patch 3, you simply need to add: if (interface == USXGMII) cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN); else cpsw_sl_ctl_clr(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN); Thanks.
On 03-04-2023 19:25, Russell King (Oracle) wrote: > On Mon, Apr 03, 2023 at 07:20:21PM +0530, Siddharth Vadapalli wrote: >> >> >> On 03-04-2023 18:43, Russell King (Oracle) wrote: >>> On Mon, Apr 03, 2023 at 06:31:52PM +0530, Siddharth Vadapalli wrote: >>>> >>>> >>>> On 03-04-2023 16:38, Russell King (Oracle) wrote: >>>>> On Mon, Apr 03, 2023 at 04:31:04PM +0530, Siddharth Vadapalli wrote: >>>>>> Move the interface mode specific configuration to the mac_config() >>>>>> callback am65_cpsw_nuss_mac_config(). >>>>>> >>>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>>>> --- >>>>>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++--- >>>>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>>> index d17757ecbf42..74e099828978 100644 >>>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>>> @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in >>>>>> phylink_config); >>>>>> struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave); >>>>>> struct am65_cpsw_common *common = port->common; >>>>>> + u32 mac_control = 0; >>>>>> >>>>>> if (common->pdata.extra_modes & BIT(state->interface)) { >>>>>> - if (state->interface == PHY_INTERFACE_MODE_SGMII) >>>>>> + if (state->interface == PHY_INTERFACE_MODE_SGMII) { >>>>>> + mac_control |= CPSW_SL_CTL_EXT_EN; >>>>>> writel(ADVERTISE_SGMII, >>>>>> port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG); >>>>>> + } >>>>>> >>>>>> + if (mac_control) >>>>>> + cpsw_sl_ctl_set(port->slave.mac_sl, mac_control); >>>>>> writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, >>>>>> port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG); >>>>>> } >>>>>> @@ -1553,8 +1558,7 @@ 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; >>>>>> + /* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */ >>>>>> 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; >>>>> >>>>> I'm afraid I can see you haven't thought this patch through properly. >>>>> >>>>> am65_cpsw_nuss_mac_link_down() will call >>>>> cpsw_sl_ctl_reset(port->slave.mac_sl); which has the effect of clearing >>>>> to zero the entire MAC control register. This will clear >>>>> CPSW_SL_CTL_EXT_EN that was set in am65_cpsw_nuss_mac_config() which is >>>>> not what you want to be doing. >>>>> >>>>> Given that we have the 10Mbps issue with RGMII, I think what you want >>>>> to be doing is: >>>>> >>>>> 1. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_config() if in SGMII >>>>> mode, otherwise clear this bit. >>>>> >>>>> 2. Clear the mac_control register in am65_cpsw_nuss_mac_link_down() >>>>> if in RMGII mode, otherwise preserve the state of >>>>> CPSW_SL_CTL_EXT_EN but clear all other bits. >>>>> >>>>> 3. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_link_up() if in >>>>> RGMII mode and 10Mbps. >>>> >>>> I plan to implement it as follows: >>>> 1. Add a member "u32 mode_config" to "struct am65_cpsw_slave_data" in >>>> "am65-cpsw-nuss.h". >>>> 2. In am65_cpsw_nuss_mac_config(), store the value of mac_control in >>>> "port->slave.mode_config". >>>> 3. In am65_cpsw_nuss_mac_link_down(), after the reset via >>>> cpsw_sl_ctl_reset(), execute: >>>> cpsw_sl_ctl_set(port->slave.mac_sl, port->slave.mode_config) in order to >>>> restore the configuration performed in am65_cpsw_nuss_mac_config(). >>>> >>>> Please let me know in case of any suggestions to implement it in a >>>> better manner. >>> >>> Do you think this complexity is really worth it? >>> >>> Let's look at what's available: >>> >>> cpsw_sl_ctl_set() - sets bits in the mac control register >>> cpsw_sl_ctl_clr() - clears bits in the mac control register >>> cpsw_sl_ctl_reset() - sets the mac control register to zero >>> >>> So, in mac_config(), we can do: >>> >>> if (interface == SGMII) >>> cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN); >>> else >>> cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN); >> >> While this will work for patch 1/3, once I add support for USXGMII mode >> as in patch 3/3, I believe that I have to invert it, beginning by >> invoking a cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN) at the start in >> mac_config() followed by switching through the modes. If the mode is >> SGMII, then I invoke cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN), along with the >> write to the MR_ADV_ABILITY_REG register to advertise SGMII. If the mode >> is USXGMII, then I invoke: >> cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN) > > For patch 1, I did leave out the write for MR_ADV_ABILITY_REG, I had > assumed you'd get the idea on that and merge the if() condition you > already had with my suggestion above (which isn't literal code!) > > In patch 3, you simply need to add: > > if (interface == USXGMII) > cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN); > else > cpsw_sl_ctl_clr(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN); Thank you for answering all my questions and sharing your valuable feedback. I will implement all the suggestions in the v3 series. Regards, Siddharth.
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c index d17757ecbf42..74e099828978 100644 --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in phylink_config); struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave); struct am65_cpsw_common *common = port->common; + u32 mac_control = 0; if (common->pdata.extra_modes & BIT(state->interface)) { - if (state->interface == PHY_INTERFACE_MODE_SGMII) + if (state->interface == PHY_INTERFACE_MODE_SGMII) { + mac_control |= CPSW_SL_CTL_EXT_EN; writel(ADVERTISE_SGMII, port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG); + } + if (mac_control) + cpsw_sl_ctl_set(port->slave.mac_sl, mac_control); writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG); } @@ -1553,8 +1558,7 @@ 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; + /* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */ 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;
Move the interface mode specific configuration to the mac_config() callback am65_cpsw_nuss_mac_config(). Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> --- drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)