| Message ID | 20230522121532.86610-9-arinc.unal@arinc9.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: dsa: mt7530: improve, trap BPDU & LLDP, and prefer CPU port | expand |
On Mon, May 22, 2023 at 03:15:10PM +0300, arinc9.unal@gmail.com wrote: > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index 710c6622d648..d837aa20968c 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -2728,25 +2722,20 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode, > goto unsupported; > break; > case 5: /* Port 5, can be used as a CPU port. */ > - if (priv->p5_interface == state->interface) > + if (priv->p5_configured) > break; > > if (mt753x_mac_config(ds, port, mode, state) < 0) > goto unsupported; > - > - if (priv->p5_intf_sel != P5_DISABLED) > - priv->p5_interface = state->interface; If you don't replace this bit with anything, who will set priv->p5_configured for mt7530? > break; > case 6: /* Port 6, can be used as a CPU port. */ > - if (priv->p6_interface == state->interface) > + if (priv->p6_configured) > break; > > mt753x_pad_setup(ds, state); > > if (mt753x_mac_config(ds, port, mode, state) < 0) > goto unsupported; > - > - priv->p6_interface = state->interface; Similar question for port 6.
On 24.05.2023 20:51, Vladimir Oltean wrote: > On Mon, May 22, 2023 at 03:15:10PM +0300, arinc9.unal@gmail.com wrote: >> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c >> index 710c6622d648..d837aa20968c 100644 >> --- a/drivers/net/dsa/mt7530.c >> +++ b/drivers/net/dsa/mt7530.c >> @@ -2728,25 +2722,20 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode, >> goto unsupported; >> break; >> case 5: /* Port 5, can be used as a CPU port. */ >> - if (priv->p5_interface == state->interface) >> + if (priv->p5_configured) >> break; >> >> if (mt753x_mac_config(ds, port, mode, state) < 0) >> goto unsupported; >> - >> - if (priv->p5_intf_sel != P5_DISABLED) >> - priv->p5_interface = state->interface; > > If you don't replace this bit with anything, who will set priv->p5_configured > for mt7530? I intend priv->p5_configured and priv->p6_configured to be only used for MT7531 as I have stated on the mt7530_priv description. Arınç
On Thu, May 25, 2023 at 09:49:58AM +0300, Arınç ÜNAL wrote: > On 24.05.2023 20:51, Vladimir Oltean wrote: > > On Mon, May 22, 2023 at 03:15:10PM +0300, arinc9.unal@gmail.com wrote: > > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > > > index 710c6622d648..d837aa20968c 100644 > > > --- a/drivers/net/dsa/mt7530.c > > > +++ b/drivers/net/dsa/mt7530.c > > > @@ -2728,25 +2722,20 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode, > > > goto unsupported; > > > break; > > > case 5: /* Port 5, can be used as a CPU port. */ > > > - if (priv->p5_interface == state->interface) > > > + if (priv->p5_configured) > > > break; > > > if (mt753x_mac_config(ds, port, mode, state) < 0) > > > goto unsupported; > > > - > > > - if (priv->p5_intf_sel != P5_DISABLED) > > > - priv->p5_interface = state->interface; > > > > If you don't replace this bit with anything, who will set priv->p5_configured > > for mt7530? > > I intend priv->p5_configured and priv->p6_configured to be only used for > MT7531 as I have stated on the mt7530_priv description. Ok, but given the premise of this patch set, that phylink is always available, does it make sense for mt7531_cpu_port_config() and mt7988_cpu_port_config() to manually call phylink methods?
On 26.05.2023 16:01, Vladimir Oltean wrote: > On Thu, May 25, 2023 at 09:49:58AM +0300, Arınç ÜNAL wrote: >> On 24.05.2023 20:51, Vladimir Oltean wrote: >>> On Mon, May 22, 2023 at 03:15:10PM +0300, arinc9.unal@gmail.com wrote: >>>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c >>>> index 710c6622d648..d837aa20968c 100644 >>>> --- a/drivers/net/dsa/mt7530.c >>>> +++ b/drivers/net/dsa/mt7530.c >>>> @@ -2728,25 +2722,20 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode, >>>> goto unsupported; >>>> break; >>>> case 5: /* Port 5, can be used as a CPU port. */ >>>> - if (priv->p5_interface == state->interface) >>>> + if (priv->p5_configured) >>>> break; >>>> if (mt753x_mac_config(ds, port, mode, state) < 0) >>>> goto unsupported; >>>> - >>>> - if (priv->p5_intf_sel != P5_DISABLED) >>>> - priv->p5_interface = state->interface; >>> >>> If you don't replace this bit with anything, who will set priv->p5_configured >>> for mt7530? >> >> I intend priv->p5_configured and priv->p6_configured to be only used for >> MT7531 as I have stated on the mt7530_priv description. > > Ok, but given the premise of this patch set, that phylink is always available, > does it make sense for mt7531_cpu_port_config() and mt7988_cpu_port_config() > to manually call phylink methods? All I know is that that's how the implementation of phylink's PCS support in this driver works. It expects the MAC to be set up before calling mt753x_phylink_pcs_link_up() and mt753x_phylink_mac_link_up(). Arınç
On Sat, Jun 03, 2023 at 03:15:52PM +0300, Arınç ÜNAL wrote: > On 26.05.2023 16:01, Vladimir Oltean wrote: > > Ok, but given the premise of this patch set, that phylink is always available, > > does it make sense for mt7531_cpu_port_config() and mt7988_cpu_port_config() > > to manually call phylink methods? > > All I know is that that's how the implementation of phylink's PCS support in > this driver works. It expects the MAC to be set up before calling > mt753x_phylink_pcs_link_up() and mt753x_phylink_mac_link_up(). First, do you see a message printed for the DSA device indicating that a link is up, without identifying the interface? For example, with mv88e6xxx: mv88e6085 f1072004.mdio-mii:04: Link is Up - 1Gbps/Full - flow control off as opposed to a user port which will look like this: mv88e6085 f1072004.mdio-mii:04 lan1: Link is Up - 1Gbps/Full - flow control rx/tx If you do, that's likely for the CPU port, and indicates that phylink is being used for the CPU port. If not, then you need to investigate whether you've provided the full description in DT for the CPU port. In other words, phy-mode and a fixed-link specification or in-band mode. Given that, you should have no need to make explicit calls to your mac_config, pcs_link_up and mac_link_up functions. If you need to make these calls, it suggests that phylink is not being used for the CPU port.
On Sat, Jun 03, 2023 at 03:15:52PM +0300, Arınç ÜNAL wrote: > On 26.05.2023 16:01, Vladimir Oltean wrote: > > Ok, but given the premise of this patch set, that phylink is always available, > > does it make sense for mt7531_cpu_port_config() and mt7988_cpu_port_config() > > to manually call phylink methods? > > All I know is that that's how the implementation of phylink's PCS support in > this driver works. It expects the MAC to be set up before calling > mt753x_phylink_pcs_link_up() and mt753x_phylink_mac_link_up(). No, but I mean, won't phylink call mt7531_mac_config(), mt753x_phylink_pcs_link_up() and mt753x_phylink_mac_link_up() automatically and in the expected order already, and if not, what prevents that from happening? I just don't understand why the cpu_port_config() methods of MT7531 and MT7988 call phylink methods manually from the driver.
On 3.06.2023 15:26, Russell King (Oracle) wrote: > On Sat, Jun 03, 2023 at 03:15:52PM +0300, Arınç ÜNAL wrote: >> On 26.05.2023 16:01, Vladimir Oltean wrote: >>> Ok, but given the premise of this patch set, that phylink is always available, >>> does it make sense for mt7531_cpu_port_config() and mt7988_cpu_port_config() >>> to manually call phylink methods? >> >> All I know is that that's how the implementation of phylink's PCS support in >> this driver works. It expects the MAC to be set up before calling >> mt753x_phylink_pcs_link_up() and mt753x_phylink_mac_link_up(). > > First, do you see a message printed for the DSA device indicating that > a link is up, without identifying the interface? For example, with > mv88e6xxx: > > mv88e6085 f1072004.mdio-mii:04: Link is Up - 1Gbps/Full - flow control off > > as opposed to a user port which will look like this: > > mv88e6085 f1072004.mdio-mii:04 lan1: Link is Up - 1Gbps/Full - flow control rx/tx > > If you do, that's likely for the CPU port, and indicates that phylink > is being used for the CPU port. If not, then you need to investigate > whether you've provided the full description in DT for the CPU port. > In other words, phy-mode and a fixed-link specification or in-band > mode. Yes I do see this. The DT is properly defined and the port is properly set up as a CPU port. > > Given that, you should have no need to make explicit calls to your > mac_config, pcs_link_up and mac_link_up functions. If you need to > make these calls, it suggests that phylink is not being used for the > CPU port. Your own commit does this so I don't know what to tell you. https://github.com/torvalds/linux/commit/cbd1f243bc41056c76fcfc5f3380cfac1f00d37b Arınç
On Sun, Jun 04, 2023 at 01:46:46PM +0300, Arınç ÜNAL wrote: > On 3.06.2023 15:26, Russell King (Oracle) wrote: > > Given that, you should have no need to make explicit calls to your > > mac_config, pcs_link_up and mac_link_up functions. If you need to > > make these calls, it suggests that phylink is not being used for the > > CPU port. > > Your own commit does this so I don't know what to tell you. > > https://github.com/torvalds/linux/commit/cbd1f243bc41056c76fcfc5f3380cfac1f00d37b I would like to make a comment here to explain why in that commit I added a call into mt7531_cpu_port_config() to mt7531_cpu_port_config(). When I'm making changes to drivers, then I follow a golden rule: do not change the behaviour unless it is an intentional change. This is exactly what is going on in this commit. mt7531_cpu_port_config() called mt753x_phylink_mac_link_up(), which then, as the very first thing, called mt753x_mac_pcs_link_up(). mt753x_mac_pcs_link_up() called priv->info->mac_pcs_link_up() if it is defined. Since converting to phylink_pcs involves the removal of the direct call from mt753x_phylink_mac_link_up() to the priv->info->mac_pcs_link_up() method, in order to preserve the behaviour of the driver, it is necessary to ensure that mt7531_cpu_port_config() still makes that call. mt753x_phylink_pcs_link_up() is the new function replacing mt753x_mac_pcs_link_up() which makes that call, so it is entirely appropriate to add that call into mt7531_cpu_port_config() so that mt7531_cpu_port_config() behaves _precisely_ the same as it did before and after this change. In that sense, as far as mt7531_cpu_port_config() is concerned, this change is entirely idempotent. I don't know whether mt7531_cpu_port_config() is necessary to properly bring up a CPU port, or whether _all_ firmware descriptions for mt7531 include all the necessary properties so that DSA will always bring up a phylink instance for the CPU port - that really is not relevant for the change you point out. What is relevant is only making the intended change, and the intended change is to split the PCS-specific code from the MAC-specific code. This principle of only making one change in a patch, and ensuring that parts of the code which merely need to be re-organised to achieve that change are done in an idempotent way is fundamental to good code maintenance, especially when modifying drivers that one does not have the hardware to be able to test. You have provided new information - that basically indicates that phylink is used for your setup. If we can get to a position where we can confidently say that phylink will always be used for the CPU port, the code in mt7531_cpu_port_config() that bypasses phylink, calling methods in phylink's mac_ops and pcs_ops, should be removed. I don't remember whether Vladimir's firmware validator will fail for mt753x if CPU ports are not fully described, but that would be well worth checking. If it does, then we can be confident that phylink will always be used, and those bypassing calls should not be necessary. I hope this explains the situation better.
On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote: > I don't remember whether Vladimir's firmware validator will fail for > mt753x if CPU ports are not fully described, but that would be well > worth checking. If it does, then we can be confident that phylink > will always be used, and those bypassing calls should not be necessary. It does, I've just retested this: [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22
On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote: > On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote: > > I don't remember whether Vladimir's firmware validator will fail for > > mt753x if CPU ports are not fully described, but that would be well > > worth checking. If it does, then we can be confident that phylink > > will always be used, and those bypassing calls should not be necessary. > > It does, I've just retested this: > > [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties > [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch > [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22 ... which isn't listed in dsa_switches_apply_workarounds[], and neither is mt753x. Thanks. So, that should be sufficient to know that the CPU port will always properly described, and thus bypassing phylink in mt753x for the CPU port should not be necessary.
On 4.06.2023 16:07, Russell King (Oracle) wrote: > On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote: >> On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote: >>> I don't remember whether Vladimir's firmware validator will fail for >>> mt753x if CPU ports are not fully described, but that would be well >>> worth checking. If it does, then we can be confident that phylink >>> will always be used, and those bypassing calls should not be necessary. >> >> It does, I've just retested this: >> >> [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties >> [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch >> [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22 > > ... which isn't listed in dsa_switches_apply_workarounds[], and > neither is mt753x. Thanks. > > So, that should be sufficient to know that the CPU port will always > properly described, and thus bypassing phylink in mt753x for the CPU > port should not be necessary. Perfect! If I understand correctly, there's this code - specific to MT7531 and MT7988 ports being used as CPU ports - which runs in addition to what's in mt753x_phylink_mac_config(): mt7530_write(priv, MT7530_PMCR_P(port), PMCR_CPU_PORT_SETTING(priv->id)); This should be put on mt753x_phylink_mac_config(), under priv->id == ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port) checks? Arınç
On Sun, Jun 04, 2023 at 04:14:31PM +0300, Arınç ÜNAL wrote: > On 4.06.2023 16:07, Russell King (Oracle) wrote: > > On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote: > > > On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote: > > > > I don't remember whether Vladimir's firmware validator will fail for > > > > mt753x if CPU ports are not fully described, but that would be well > > > > worth checking. If it does, then we can be confident that phylink > > > > will always be used, and those bypassing calls should not be necessary. > > > > > > It does, I've just retested this: > > > > > > [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties > > > [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch > > > [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22 > > > > ... which isn't listed in dsa_switches_apply_workarounds[], and > > neither is mt753x. Thanks. > > > > So, that should be sufficient to know that the CPU port will always > > properly described, and thus bypassing phylink in mt753x for the CPU > > port should not be necessary. > > Perfect! If I understand correctly, there's this code - specific to MT7531 > and MT7988 ports being used as CPU ports - which runs in addition to what's > in mt753x_phylink_mac_config(): > > mt7530_write(priv, MT7530_PMCR_P(port), > PMCR_CPU_PORT_SETTING(priv->id)); > > This should be put on mt753x_phylink_mac_config(), under priv->id == > ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port) checks? > > Arınç Given that mt753x_phylink_mac_config() and mt753x_phylink_mac_link_up() also both modifies MT7530_PMCR_P(port), have you studied the code to see what really is changed compared to what's in the PMCR_CPU_PORT_SETTING() macro, after both phylink methods have run?
On Sun, Jun 04, 2023 at 04:14:31PM +0300, Arınç ÜNAL wrote: > On 4.06.2023 16:07, Russell King (Oracle) wrote: > > On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote: > > > On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote: > > > > I don't remember whether Vladimir's firmware validator will fail for > > > > mt753x if CPU ports are not fully described, but that would be well > > > > worth checking. If it does, then we can be confident that phylink > > > > will always be used, and those bypassing calls should not be necessary. > > > > > > It does, I've just retested this: > > > > > > [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties > > > [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch > > > [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22 > > > > ... which isn't listed in dsa_switches_apply_workarounds[], and > > neither is mt753x. Thanks. > > > > So, that should be sufficient to know that the CPU port will always > > properly described, and thus bypassing phylink in mt753x for the CPU > > port should not be necessary. > > Perfect! If I understand correctly, there's this code - specific to MT7531 > and MT7988 ports being used as CPU ports - which runs in addition to what's > in mt753x_phylink_mac_config(): > > mt7530_write(priv, MT7530_PMCR_P(port), > PMCR_CPU_PORT_SETTING(priv->id)); > > This should be put on mt753x_phylink_mac_config(), under priv->id == > ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port) checks? Please remember that I have very little knowledge of MT753x, so in order to answer this question, I've read through the mt7530 driver code. Looking at mt7530.h: #define PMCR_CPU_PORT_SETTING(id) (PMCR_FORCE_MODE_ID((id)) | \ PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \ PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \ PMCR_TX_EN | PMCR_RX_EN | \ PMCR_TX_FC_EN | PMCR_RX_FC_EN | \ PMCR_FORCE_SPEED_1000 | \ PMCR_FORCE_FDX | PMCR_FORCE_LNK) This seems to be some kind of port control register that sets amongst other things parameters such as whether flow control is enabled, the port speed, the duplex setting, whether link is forced up, etc. Looking at what mt753x_phylink_mac_link_up() does: 1. it sets PMCR_RX_EN | PMCR_TX_EN | PMCR_FORCE_LNK. 2. it sets PMCR_FORCE_SPEED_1000 if speed was 1000Mbps, or if using an internal, TRGMII, 1000base-X or 2500base-X phy interface mode. 3. it sets PMCR_FORCE_FDX if full duplex was requested. 4. it sets PMCR_TX_FC_EN if full duplex was requested with tx pause. 5. it sets PMCR_RX_FC_EN if full duplex was requested with rx pause. So, provided this is called with the appropriate parameters, for a fixed link, that will leave the following: PMCR_FORCE_MODE_ID(id) PMCR_IFG_XMIT(1) PMCR_MAC_MODE PMCR_BACKOFF_EN PMCR_BACKPR_EN If we now look at mt753x_phylink_mac_config(), this sets PMCR_IFG_XMIT(1), PMCR_MAC_MODE, PMCR_BACKOFF_EN, PMCR_BACKPR_EN, and PMCR_FORCE_MODE_ID(priv->id), which I believe is everything that PMCR_CPU_PORT_SETTING(priv->id) is doing. So, Wouldn't a fixed-link description indicating 1Gbps, full duplex with pause cause phylink to call both mt753x_phylink_mac_config() and mt753x_phylink_mac_link_up() with appropriate arguments to set all of these parameters in PMCR? Now, I'm going to analyse something else. mt7531_cpu_port_config() is called from mt753x_cpu_port_enable(), which is itself called from mt7531_setup_common(). That is ultimately called from the DSA switch ops .setup() method. This method is called from dsa_switch_setup() for each switch in the DSA tree. dsa_tree_setup_switches() calls this, and is called from dsa_tree_setup(). Once dsa_tree_setup_switches() finishes successfully, dsa_tree_setup_ports() will be called. This will then setup DSA and CPU ports, which will then setup a phylink instance for these ports. phylink will parse the firmware description for the port. DSA will then call dsa_port_enable(). dsa_port_enable() will then call any port_enable() method in the mt7530.c driver, which will be mt7530_port_enable(). This then... mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK); which is: #define PMCR_LINK_SETTINGS_MASK (PMCR_TX_EN | PMCR_FORCE_SPEED_1000 | \ PMCR_RX_EN | PMCR_FORCE_SPEED_100 | \ PMCR_TX_FC_EN | PMCR_RX_FC_EN | \ PMCR_FORCE_FDX | PMCR_FORCE_LNK | \ PMCR_FORCE_EEE1G | PMCR_FORCE_EEE100) So it wipes out all the PMCR settings that mt7531_cpu_port_config() performed - undoing *everything* below that switch() statement in mt7531_cpu_port_config()! Once the port_enable() method returns, DSA will then call phylink_start(), which will trigger phylink to bring up the link according to the settings it has, which will mean phylink calls the mac_config(), pcs_config(), pcs_link_up() and mac_link_up() with the appropriate parameters for the firmware described link. So I think I have the answer to my initial thought: do the calls in mt7531_cpu_port_config() to the phylink methods have any use what so ever? The answer is no, they are entirely useless. The same goes for the other cpu_port_config() methods that do something similar. The same goes for the PMCR register write that's changing any bits included in PMCR_LINK_SETTINGS_MASK. What that means is that mt7988_cpu_port_config() can be entirely removed, it serves no useful purpose what so ever. For mt7531_cpu_port_config(), it only needs to set priv->p[56]_interface which, as far as I can see, probably only avoids mac_config() doing any pad setup (that's a guess.) At least that's what I gather from reading through the driver and DSA code. It may be I've missed something, but currently, I think that these cpu_port_config() functions aren't doing too much that is actually useful work.
On Sun, Jun 04, 2023 at 04:13:39PM +0100, Russell King (Oracle) wrote: > On Sun, Jun 04, 2023 at 04:14:31PM +0300, Arınç ÜNAL wrote: > > On 4.06.2023 16:07, Russell King (Oracle) wrote: > > > On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote: > > > > On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote: > > > > > I don't remember whether Vladimir's firmware validator will fail for > > > > > mt753x if CPU ports are not fully described, but that would be well > > > > > worth checking. If it does, then we can be confident that phylink > > > > > will always be used, and those bypassing calls should not be necessary. > > > > > > > > It does, I've just retested this: > > > > > > > > [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties > > > > [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch > > > > [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22 > > > > > > ... which isn't listed in dsa_switches_apply_workarounds[], and > > > neither is mt753x. Thanks. > > > > > > So, that should be sufficient to know that the CPU port will always > > > properly described, and thus bypassing phylink in mt753x for the CPU > > > port should not be necessary. > > > > Perfect! If I understand correctly, there's this code - specific to MT7531 > > and MT7988 ports being used as CPU ports - which runs in addition to what's > > in mt753x_phylink_mac_config(): > > > > mt7530_write(priv, MT7530_PMCR_P(port), > > PMCR_CPU_PORT_SETTING(priv->id)); > > > > This should be put on mt753x_phylink_mac_config(), under priv->id == > > ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port) checks? > > Please remember that I have very little knowledge of MT753x, so in > order to answer this question, I've read through the mt7530 driver > code. > > Looking at mt7530.h: > > #define PMCR_CPU_PORT_SETTING(id) (PMCR_FORCE_MODE_ID((id)) | \ > PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \ > PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \ > PMCR_TX_EN | PMCR_RX_EN | \ > PMCR_TX_FC_EN | PMCR_RX_FC_EN | \ > PMCR_FORCE_SPEED_1000 | \ > PMCR_FORCE_FDX | PMCR_FORCE_LNK) > > This seems to be some kind of port control register that sets amongst > other things parameters such as whether flow control is enabled, the > port speed, the duplex setting, whether link is forced up, etc. > > Looking at what mt753x_phylink_mac_link_up() does: > > 1. it sets PMCR_RX_EN | PMCR_TX_EN | PMCR_FORCE_LNK. > 2. it sets PMCR_FORCE_SPEED_1000 if speed was 1000Mbps, or if using > an internal, TRGMII, 1000base-X or 2500base-X phy interface mode. > 3. it sets PMCR_FORCE_FDX if full duplex was requested. > 4. it sets PMCR_TX_FC_EN if full duplex was requested with tx pause. > 5. it sets PMCR_RX_FC_EN if full duplex was requested with rx pause. > > So, provided this is called with the appropriate parameters, for a > fixed link, that will leave the following: > > PMCR_FORCE_MODE_ID(id) > PMCR_IFG_XMIT(1) > PMCR_MAC_MODE > PMCR_BACKOFF_EN > PMCR_BACKPR_EN > > If we now look at mt753x_phylink_mac_config(), this sets > PMCR_IFG_XMIT(1), PMCR_MAC_MODE, PMCR_BACKOFF_EN, PMCR_BACKPR_EN, > and PMCR_FORCE_MODE_ID(priv->id), which I believe is everything that > PMCR_CPU_PORT_SETTING(priv->id) is doing. > > So, Wouldn't a fixed-link description indicating 1Gbps, full duplex > with pause cause phylink to call both mt753x_phylink_mac_config() and > mt753x_phylink_mac_link_up() with appropriate arguments to set all > of these parameters in PMCR? > > Now, I'm going to analyse something else. mt7531_cpu_port_config() > is called from mt753x_cpu_port_enable(), which is itself called from > mt7531_setup_common(). That is ultimately called from the DSA switch > ops .setup() method. > > This method is called from dsa_switch_setup() for each switch in the > DSA tree. dsa_tree_setup_switches() calls this, and is called from > dsa_tree_setup(). Once dsa_tree_setup_switches() finishes > successfully, dsa_tree_setup_ports() will be called. This will then > setup DSA and CPU ports, which will then setup a phylink instance > for these ports. phylink will parse the firmware description for > the port. DSA will then call dsa_port_enable(). > > dsa_port_enable() will then call any port_enable() method in the > mt7530.c driver, which will be mt7530_port_enable(). This then... > > mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK); > > which is: > > #define PMCR_LINK_SETTINGS_MASK (PMCR_TX_EN | PMCR_FORCE_SPEED_1000 | \ > PMCR_RX_EN | PMCR_FORCE_SPEED_100 | \ > PMCR_TX_FC_EN | PMCR_RX_FC_EN | \ > PMCR_FORCE_FDX | PMCR_FORCE_LNK | \ > PMCR_FORCE_EEE1G | PMCR_FORCE_EEE100) > > So it wipes out all the PMCR settings that mt7531_cpu_port_config() > performed - undoing *everything* below that switch() statement in > mt7531_cpu_port_config()! > > Once the port_enable() method returns, DSA will then call > phylink_start(), which will trigger phylink to bring up the link > according to the settings it has, which will mean phylink calls > the mac_config(), pcs_config(), pcs_link_up() and mac_link_up() > with the appropriate parameters for the firmware described link. > > So I think I have the answer to my initial thought: do the calls in > mt7531_cpu_port_config() to the phylink methods have any use what so > ever? The answer is no, they are entirely useless. The same goes for > the other cpu_port_config() methods that do something similar. The > same goes for the PMCR register write that's changing any bits > included in PMCR_LINK_SETTINGS_MASK. > > What that means is that mt7988_cpu_port_config() can be entirely > removed, it serves no useful purpose what so ever. For > mt7531_cpu_port_config(), it only needs to set priv->p[56]_interface > which, as far as I can see, probably only avoids mac_config() doing > any pad setup (that's a guess.) > > At least that's what I gather from reading through the driver and > DSA code. It may be I've missed something, but currently, I think > that these cpu_port_config() functions aren't doing too much that > is actually useful work. Essentially, I think this change will have no effect at all on the driver, because any effect this code has is totally undone when the driver's port_enable() method is called: diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 9bc54e1348cb..447e63d74e0c 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -2859,8 +2859,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port) { struct mt7530_priv *priv = ds->priv; phy_interface_t interface; - int speed; - int ret; switch (port) { case 5: @@ -2880,36 +2878,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port) return -EINVAL; } - if (interface == PHY_INTERFACE_MODE_2500BASEX) - speed = SPEED_2500; - else - speed = SPEED_1000; - - ret = mt7531_mac_config(ds, port, MLO_AN_FIXED, interface); - if (ret) - return ret; - mt7530_write(priv, MT7530_PMCR_P(port), - PMCR_CPU_PORT_SETTING(priv->id)); - mt753x_phylink_pcs_link_up(&priv->pcs[port].pcs, MLO_AN_FIXED, - interface, speed, DUPLEX_FULL); - mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED, interface, NULL, - speed, DUPLEX_FULL, true, true); - - return 0; -} - -static int -mt7988_cpu_port_config(struct dsa_switch *ds, int port) -{ - struct mt7530_priv *priv = ds->priv; - - mt7530_write(priv, MT7530_PMCR_P(port), - PMCR_CPU_PORT_SETTING(priv->id)); - - mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED, - PHY_INTERFACE_MODE_INTERNAL, NULL, - SPEED_10000, DUPLEX_FULL, true, true); - return 0; } @@ -3165,7 +3133,6 @@ const struct mt753x_info mt753x_table[] = { .phy_read_c45 = mt7531_ind_c45_phy_read, .phy_write_c45 = mt7531_ind_c45_phy_write, .pad_setup = mt7988_pad_setup, - .cpu_port_config = mt7988_cpu_port_config, .mac_port_get_caps = mt7988_mac_port_get_caps, .mac_port_config = mt7988_mac_config, },
On Sun, Jun 04, 2023 at 05:00:11PM +0100, Russell King (Oracle) wrote: > On Sun, Jun 04, 2023 at 04:13:39PM +0100, Russell King (Oracle) wrote: > > On Sun, Jun 04, 2023 at 04:14:31PM +0300, Arınç ÜNAL wrote: > > > On 4.06.2023 16:07, Russell King (Oracle) wrote: > > > > On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote: > > > > > On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote: > > > > > > I don't remember whether Vladimir's firmware validator will fail for > > > > > > mt753x if CPU ports are not fully described, but that would be well > > > > > > worth checking. If it does, then we can be confident that phylink > > > > > > will always be used, and those bypassing calls should not be necessary. > > > > > > > > > > It does, I've just retested this: > > > > > > > > > > [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties > > > > > [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch > > > > > [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22 > > > > > > > > ... which isn't listed in dsa_switches_apply_workarounds[], and > > > > neither is mt753x. Thanks. > > > > > > > > So, that should be sufficient to know that the CPU port will always > > > > properly described, and thus bypassing phylink in mt753x for the CPU > > > > port should not be necessary. > > > > > > Perfect! If I understand correctly, there's this code - specific to MT7531 > > > and MT7988 ports being used as CPU ports - which runs in addition to what's > > > in mt753x_phylink_mac_config(): > > > > > > mt7530_write(priv, MT7530_PMCR_P(port), > > > PMCR_CPU_PORT_SETTING(priv->id)); > > > > > > This should be put on mt753x_phylink_mac_config(), under priv->id == > > > ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port) checks? > > > > Please remember that I have very little knowledge of MT753x, so in > > order to answer this question, I've read through the mt7530 driver > > code. > > > > Looking at mt7530.h: > > > > #define PMCR_CPU_PORT_SETTING(id) (PMCR_FORCE_MODE_ID((id)) | \ > > PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \ > > PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \ > > PMCR_TX_EN | PMCR_RX_EN | \ > > PMCR_TX_FC_EN | PMCR_RX_FC_EN | \ > > PMCR_FORCE_SPEED_1000 | \ > > PMCR_FORCE_FDX | PMCR_FORCE_LNK) > > > > This seems to be some kind of port control register that sets amongst > > other things parameters such as whether flow control is enabled, the > > port speed, the duplex setting, whether link is forced up, etc. > > > > Looking at what mt753x_phylink_mac_link_up() does: > > > > 1. it sets PMCR_RX_EN | PMCR_TX_EN | PMCR_FORCE_LNK. > > 2. it sets PMCR_FORCE_SPEED_1000 if speed was 1000Mbps, or if using > > an internal, TRGMII, 1000base-X or 2500base-X phy interface mode. > > 3. it sets PMCR_FORCE_FDX if full duplex was requested. > > 4. it sets PMCR_TX_FC_EN if full duplex was requested with tx pause. > > 5. it sets PMCR_RX_FC_EN if full duplex was requested with rx pause. > > > > So, provided this is called with the appropriate parameters, for a > > fixed link, that will leave the following: > > > > PMCR_FORCE_MODE_ID(id) > > PMCR_IFG_XMIT(1) > > PMCR_MAC_MODE > > PMCR_BACKOFF_EN > > PMCR_BACKPR_EN > > > > If we now look at mt753x_phylink_mac_config(), this sets > > PMCR_IFG_XMIT(1), PMCR_MAC_MODE, PMCR_BACKOFF_EN, PMCR_BACKPR_EN, > > and PMCR_FORCE_MODE_ID(priv->id), which I believe is everything that > > PMCR_CPU_PORT_SETTING(priv->id) is doing. > > > > So, Wouldn't a fixed-link description indicating 1Gbps, full duplex > > with pause cause phylink to call both mt753x_phylink_mac_config() and > > mt753x_phylink_mac_link_up() with appropriate arguments to set all > > of these parameters in PMCR? > > > > Now, I'm going to analyse something else. mt7531_cpu_port_config() > > is called from mt753x_cpu_port_enable(), which is itself called from > > mt7531_setup_common(). That is ultimately called from the DSA switch > > ops .setup() method. > > > > This method is called from dsa_switch_setup() for each switch in the > > DSA tree. dsa_tree_setup_switches() calls this, and is called from > > dsa_tree_setup(). Once dsa_tree_setup_switches() finishes > > successfully, dsa_tree_setup_ports() will be called. This will then > > setup DSA and CPU ports, which will then setup a phylink instance > > for these ports. phylink will parse the firmware description for > > the port. DSA will then call dsa_port_enable(). > > > > dsa_port_enable() will then call any port_enable() method in the > > mt7530.c driver, which will be mt7530_port_enable(). This then... > > > > mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK); > > > > which is: > > > > #define PMCR_LINK_SETTINGS_MASK (PMCR_TX_EN | PMCR_FORCE_SPEED_1000 | \ > > PMCR_RX_EN | PMCR_FORCE_SPEED_100 | \ > > PMCR_TX_FC_EN | PMCR_RX_FC_EN | \ > > PMCR_FORCE_FDX | PMCR_FORCE_LNK | \ > > PMCR_FORCE_EEE1G | PMCR_FORCE_EEE100) > > > > So it wipes out all the PMCR settings that mt7531_cpu_port_config() > > performed - undoing *everything* below that switch() statement in > > mt7531_cpu_port_config()! > > > > Once the port_enable() method returns, DSA will then call > > phylink_start(), which will trigger phylink to bring up the link > > according to the settings it has, which will mean phylink calls > > the mac_config(), pcs_config(), pcs_link_up() and mac_link_up() > > with the appropriate parameters for the firmware described link. > > > > So I think I have the answer to my initial thought: do the calls in > > mt7531_cpu_port_config() to the phylink methods have any use what so > > ever? The answer is no, they are entirely useless. The same goes for > > the other cpu_port_config() methods that do something similar. The > > same goes for the PMCR register write that's changing any bits > > included in PMCR_LINK_SETTINGS_MASK. > > > > What that means is that mt7988_cpu_port_config() can be entirely > > removed, it serves no useful purpose what so ever. For > > mt7531_cpu_port_config(), it only needs to set priv->p[56]_interface > > which, as far as I can see, probably only avoids mac_config() doing > > any pad setup (that's a guess.) > > > > At least that's what I gather from reading through the driver and > > DSA code. It may be I've missed something, but currently, I think > > that these cpu_port_config() functions aren't doing too much that > > is actually useful work. > > Essentially, I think this change will have no effect at all on the > driver, because any effect this code has is totally undone when the > driver's port_enable() method is called: > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index 9bc54e1348cb..447e63d74e0c 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -2859,8 +2859,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port) > { > struct mt7530_priv *priv = ds->priv; > phy_interface_t interface; > - int speed; > - int ret; > > switch (port) { > case 5: > @@ -2880,36 +2878,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port) > return -EINVAL; > } > > - if (interface == PHY_INTERFACE_MODE_2500BASEX) > - speed = SPEED_2500; > - else > - speed = SPEED_1000; > - > - ret = mt7531_mac_config(ds, port, MLO_AN_FIXED, interface); > - if (ret) > - return ret; > - mt7530_write(priv, MT7530_PMCR_P(port), > - PMCR_CPU_PORT_SETTING(priv->id)); > - mt753x_phylink_pcs_link_up(&priv->pcs[port].pcs, MLO_AN_FIXED, > - interface, speed, DUPLEX_FULL); > - mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED, interface, NULL, > - speed, DUPLEX_FULL, true, true); > - > - return 0; > -} > - > -static int > -mt7988_cpu_port_config(struct dsa_switch *ds, int port) > -{ > - struct mt7530_priv *priv = ds->priv; > - > - mt7530_write(priv, MT7530_PMCR_P(port), > - PMCR_CPU_PORT_SETTING(priv->id)); > - > - mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED, > - PHY_INTERFACE_MODE_INTERNAL, NULL, > - SPEED_10000, DUPLEX_FULL, true, true); > - > return 0; > } > > @@ -3165,7 +3133,6 @@ const struct mt753x_info mt753x_table[] = { > .phy_read_c45 = mt7531_ind_c45_phy_read, > .phy_write_c45 = mt7531_ind_c45_phy_write, > .pad_setup = mt7988_pad_setup, > - .cpu_port_config = mt7988_cpu_port_config, > .mac_port_get_caps = mt7988_mac_port_get_caps, > .mac_port_config = mt7988_mac_config, > }, ... and with that patch we can remove the definition of PMCR_CPU_PORT_SETTING() as well! There is one possibility why we may not be able to remove this code - whether there's something in this which requires the CPU port to be setup prior to something else. Only someone knowledgeable of the hardware, or who has the hardware in front and can test would be able to work that out.
On 4.06.2023 19:06, Russell King (Oracle) wrote: > On Sun, Jun 04, 2023 at 05:00:11PM +0100, Russell King (Oracle) wrote: >> On Sun, Jun 04, 2023 at 04:13:39PM +0100, Russell King (Oracle) wrote: >>> On Sun, Jun 04, 2023 at 04:14:31PM +0300, Arınç ÜNAL wrote: >>>> On 4.06.2023 16:07, Russell King (Oracle) wrote: >>>>> On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote: >>>>>> On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote: >>>>>>> I don't remember whether Vladimir's firmware validator will fail for >>>>>>> mt753x if CPU ports are not fully described, but that would be well >>>>>>> worth checking. If it does, then we can be confident that phylink >>>>>>> will always be used, and those bypassing calls should not be necessary. >>>>>> >>>>>> It does, I've just retested this: >>>>>> >>>>>> [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties >>>>>> [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch >>>>>> [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22 >>>>> >>>>> ... which isn't listed in dsa_switches_apply_workarounds[], and >>>>> neither is mt753x. Thanks. >>>>> >>>>> So, that should be sufficient to know that the CPU port will always >>>>> properly described, and thus bypassing phylink in mt753x for the CPU >>>>> port should not be necessary. >>>> >>>> Perfect! If I understand correctly, there's this code - specific to MT7531 >>>> and MT7988 ports being used as CPU ports - which runs in addition to what's >>>> in mt753x_phylink_mac_config(): >>>> >>>> mt7530_write(priv, MT7530_PMCR_P(port), >>>> PMCR_CPU_PORT_SETTING(priv->id)); >>>> >>>> This should be put on mt753x_phylink_mac_config(), under priv->id == >>>> ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port) checks? >>> >>> Please remember that I have very little knowledge of MT753x, so in >>> order to answer this question, I've read through the mt7530 driver >>> code. >>> >>> Looking at mt7530.h: >>> >>> #define PMCR_CPU_PORT_SETTING(id) (PMCR_FORCE_MODE_ID((id)) | \ >>> PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \ >>> PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \ >>> PMCR_TX_EN | PMCR_RX_EN | \ >>> PMCR_TX_FC_EN | PMCR_RX_FC_EN | \ >>> PMCR_FORCE_SPEED_1000 | \ >>> PMCR_FORCE_FDX | PMCR_FORCE_LNK) >>> >>> This seems to be some kind of port control register that sets amongst >>> other things parameters such as whether flow control is enabled, the >>> port speed, the duplex setting, whether link is forced up, etc. >>> >>> Looking at what mt753x_phylink_mac_link_up() does: >>> >>> 1. it sets PMCR_RX_EN | PMCR_TX_EN | PMCR_FORCE_LNK. >>> 2. it sets PMCR_FORCE_SPEED_1000 if speed was 1000Mbps, or if using >>> an internal, TRGMII, 1000base-X or 2500base-X phy interface mode. >>> 3. it sets PMCR_FORCE_FDX if full duplex was requested. >>> 4. it sets PMCR_TX_FC_EN if full duplex was requested with tx pause. >>> 5. it sets PMCR_RX_FC_EN if full duplex was requested with rx pause. >>> >>> So, provided this is called with the appropriate parameters, for a >>> fixed link, that will leave the following: >>> >>> PMCR_FORCE_MODE_ID(id) >>> PMCR_IFG_XMIT(1) >>> PMCR_MAC_MODE >>> PMCR_BACKOFF_EN >>> PMCR_BACKPR_EN >>> >>> If we now look at mt753x_phylink_mac_config(), this sets >>> PMCR_IFG_XMIT(1), PMCR_MAC_MODE, PMCR_BACKOFF_EN, PMCR_BACKPR_EN, >>> and PMCR_FORCE_MODE_ID(priv->id), which I believe is everything that >>> PMCR_CPU_PORT_SETTING(priv->id) is doing. >>> >>> So, Wouldn't a fixed-link description indicating 1Gbps, full duplex >>> with pause cause phylink to call both mt753x_phylink_mac_config() and >>> mt753x_phylink_mac_link_up() with appropriate arguments to set all >>> of these parameters in PMCR? >>> >>> Now, I'm going to analyse something else. mt7531_cpu_port_config() >>> is called from mt753x_cpu_port_enable(), which is itself called from >>> mt7531_setup_common(). That is ultimately called from the DSA switch >>> ops .setup() method. >>> >>> This method is called from dsa_switch_setup() for each switch in the >>> DSA tree. dsa_tree_setup_switches() calls this, and is called from >>> dsa_tree_setup(). Once dsa_tree_setup_switches() finishes >>> successfully, dsa_tree_setup_ports() will be called. This will then >>> setup DSA and CPU ports, which will then setup a phylink instance >>> for these ports. phylink will parse the firmware description for >>> the port. DSA will then call dsa_port_enable(). >>> >>> dsa_port_enable() will then call any port_enable() method in the >>> mt7530.c driver, which will be mt7530_port_enable(). This then... >>> >>> mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK); >>> >>> which is: >>> >>> #define PMCR_LINK_SETTINGS_MASK (PMCR_TX_EN | PMCR_FORCE_SPEED_1000 | \ >>> PMCR_RX_EN | PMCR_FORCE_SPEED_100 | \ >>> PMCR_TX_FC_EN | PMCR_RX_FC_EN | \ >>> PMCR_FORCE_FDX | PMCR_FORCE_LNK | \ >>> PMCR_FORCE_EEE1G | PMCR_FORCE_EEE100) >>> >>> So it wipes out all the PMCR settings that mt7531_cpu_port_config() >>> performed - undoing *everything* below that switch() statement in >>> mt7531_cpu_port_config()! >>> >>> Once the port_enable() method returns, DSA will then call >>> phylink_start(), which will trigger phylink to bring up the link >>> according to the settings it has, which will mean phylink calls >>> the mac_config(), pcs_config(), pcs_link_up() and mac_link_up() >>> with the appropriate parameters for the firmware described link. >>> >>> So I think I have the answer to my initial thought: do the calls in >>> mt7531_cpu_port_config() to the phylink methods have any use what so >>> ever? The answer is no, they are entirely useless. The same goes for >>> the other cpu_port_config() methods that do something similar. The >>> same goes for the PMCR register write that's changing any bits >>> included in PMCR_LINK_SETTINGS_MASK. >>> >>> What that means is that mt7988_cpu_port_config() can be entirely >>> removed, it serves no useful purpose what so ever. For >>> mt7531_cpu_port_config(), it only needs to set priv->p[56]_interface >>> which, as far as I can see, probably only avoids mac_config() doing >>> any pad setup (that's a guess.) >>> >>> At least that's what I gather from reading through the driver and >>> DSA code. It may be I've missed something, but currently, I think >>> that these cpu_port_config() functions aren't doing too much that >>> is actually useful work. >> >> Essentially, I think this change will have no effect at all on the >> driver, because any effect this code has is totally undone when the >> driver's port_enable() method is called: >> >> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c >> index 9bc54e1348cb..447e63d74e0c 100644 >> --- a/drivers/net/dsa/mt7530.c >> +++ b/drivers/net/dsa/mt7530.c >> @@ -2859,8 +2859,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port) >> { >> struct mt7530_priv *priv = ds->priv; >> phy_interface_t interface; >> - int speed; >> - int ret; >> >> switch (port) { >> case 5: >> @@ -2880,36 +2878,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port) >> return -EINVAL; >> } >> >> - if (interface == PHY_INTERFACE_MODE_2500BASEX) >> - speed = SPEED_2500; >> - else >> - speed = SPEED_1000; >> - >> - ret = mt7531_mac_config(ds, port, MLO_AN_FIXED, interface); >> - if (ret) >> - return ret; >> - mt7530_write(priv, MT7530_PMCR_P(port), >> - PMCR_CPU_PORT_SETTING(priv->id)); >> - mt753x_phylink_pcs_link_up(&priv->pcs[port].pcs, MLO_AN_FIXED, >> - interface, speed, DUPLEX_FULL); >> - mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED, interface, NULL, >> - speed, DUPLEX_FULL, true, true); >> - >> - return 0; >> -} >> - >> -static int >> -mt7988_cpu_port_config(struct dsa_switch *ds, int port) >> -{ >> - struct mt7530_priv *priv = ds->priv; >> - >> - mt7530_write(priv, MT7530_PMCR_P(port), >> - PMCR_CPU_PORT_SETTING(priv->id)); >> - >> - mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED, >> - PHY_INTERFACE_MODE_INTERNAL, NULL, >> - SPEED_10000, DUPLEX_FULL, true, true); >> - >> return 0; >> } >> >> @@ -3165,7 +3133,6 @@ const struct mt753x_info mt753x_table[] = { >> .phy_read_c45 = mt7531_ind_c45_phy_read, >> .phy_write_c45 = mt7531_ind_c45_phy_write, >> .pad_setup = mt7988_pad_setup, >> - .cpu_port_config = mt7988_cpu_port_config, >> .mac_port_get_caps = mt7988_mac_port_get_caps, >> .mac_port_config = mt7988_mac_config, >> }, > > ... and with that patch we can remove the definition of > PMCR_CPU_PORT_SETTING() as well! > > There is one possibility why we may not be able to remove this code - > whether there's something in this which requires the CPU port to be > setup prior to something else. Only someone knowledgeable of the > hardware, or who has the hardware in front and can test would be able > to work that out. I am on the same page with your explanation so far. I will test this out on MT7531. Thanks a lot for looking at this! Arınç
On 4.06.2023 18:13, Russell King (Oracle) wrote: > On Sun, Jun 04, 2023 at 04:14:31PM +0300, Arınç ÜNAL wrote: >> On 4.06.2023 16:07, Russell King (Oracle) wrote: >>> On Sun, Jun 04, 2023 at 03:55:17PM +0300, Vladimir Oltean wrote: >>>> On Sun, Jun 04, 2023 at 01:18:04PM +0100, Russell King (Oracle) wrote: >>>>> I don't remember whether Vladimir's firmware validator will fail for >>>>> mt753x if CPU ports are not fully described, but that would be well >>>>> worth checking. If it does, then we can be confident that phylink >>>>> will always be used, and those bypassing calls should not be necessary. >>>> >>>> It does, I've just retested this: >>>> >>>> [ 8.469152] mscc_felix 0000:00:00.5: OF node /soc/pcie@1f0000000/ethernet-switch@0,5/ports/port@4 of CPU port 4 lacks the required "phy-handle", "fixed-link" or "managed" properties >>>> [ 8.494571] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch >>>> [ 8.502151] mscc_felix: probe of 0000:00:00.5 failed with error -22 >>> >>> ... which isn't listed in dsa_switches_apply_workarounds[], and >>> neither is mt753x. Thanks. >>> >>> So, that should be sufficient to know that the CPU port will always >>> properly described, and thus bypassing phylink in mt753x for the CPU >>> port should not be necessary. >> >> Perfect! If I understand correctly, there's this code - specific to MT7531 >> and MT7988 ports being used as CPU ports - which runs in addition to what's >> in mt753x_phylink_mac_config(): >> >> mt7530_write(priv, MT7530_PMCR_P(port), >> PMCR_CPU_PORT_SETTING(priv->id)); >> >> This should be put on mt753x_phylink_mac_config(), under priv->id == >> ID_MT7531, priv->id == ID_MT7988, and dsa_is_cpu_port(ds, port) checks? > > Please remember that I have very little knowledge of MT753x, so in > order to answer this question, I've read through the mt7530 driver > code. > > Looking at mt7530.h: > > #define PMCR_CPU_PORT_SETTING(id) (PMCR_FORCE_MODE_ID((id)) | \ > PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \ > PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \ > PMCR_TX_EN | PMCR_RX_EN | \ > PMCR_TX_FC_EN | PMCR_RX_FC_EN | \ > PMCR_FORCE_SPEED_1000 | \ > PMCR_FORCE_FDX | PMCR_FORCE_LNK) > > This seems to be some kind of port control register that sets amongst > other things parameters such as whether flow control is enabled, the > port speed, the duplex setting, whether link is forced up, etc. > > Looking at what mt753x_phylink_mac_link_up() does: > > 1. it sets PMCR_RX_EN | PMCR_TX_EN | PMCR_FORCE_LNK. > 2. it sets PMCR_FORCE_SPEED_1000 if speed was 1000Mbps, or if using > an internal, TRGMII, 1000base-X or 2500base-X phy interface mode. > 3. it sets PMCR_FORCE_FDX if full duplex was requested. > 4. it sets PMCR_TX_FC_EN if full duplex was requested with tx pause. > 5. it sets PMCR_RX_FC_EN if full duplex was requested with rx pause. > > So, provided this is called with the appropriate parameters, for a > fixed link, that will leave the following: > > PMCR_FORCE_MODE_ID(id) > PMCR_IFG_XMIT(1) > PMCR_MAC_MODE > PMCR_BACKOFF_EN > PMCR_BACKPR_EN > > If we now look at mt753x_phylink_mac_config(), this sets > PMCR_IFG_XMIT(1), PMCR_MAC_MODE, PMCR_BACKOFF_EN, PMCR_BACKPR_EN, > and PMCR_FORCE_MODE_ID(priv->id), which I believe is everything that > PMCR_CPU_PORT_SETTING(priv->id) is doing. > > So, Wouldn't a fixed-link description indicating 1Gbps, full duplex > with pause cause phylink to call both mt753x_phylink_mac_config() and > mt753x_phylink_mac_link_up() with appropriate arguments to set all > of these parameters in PMCR? > > Now, I'm going to analyse something else. mt7531_cpu_port_config() > is called from mt753x_cpu_port_enable(), which is itself called from > mt7531_setup_common(). That is ultimately called from the DSA switch > ops .setup() method. > > This method is called from dsa_switch_setup() for each switch in the > DSA tree. dsa_tree_setup_switches() calls this, and is called from > dsa_tree_setup(). Once dsa_tree_setup_switches() finishes > successfully, dsa_tree_setup_ports() will be called. This will then > setup DSA and CPU ports, which will then setup a phylink instance > for these ports. phylink will parse the firmware description for > the port. DSA will then call dsa_port_enable(). > > dsa_port_enable() will then call any port_enable() method in the > mt7530.c driver, which will be mt7530_port_enable(). This then... > > mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK); > > which is: > > #define PMCR_LINK_SETTINGS_MASK (PMCR_TX_EN | PMCR_FORCE_SPEED_1000 | \ > PMCR_RX_EN | PMCR_FORCE_SPEED_100 | \ > PMCR_TX_FC_EN | PMCR_RX_FC_EN | \ > PMCR_FORCE_FDX | PMCR_FORCE_LNK | \ > PMCR_FORCE_EEE1G | PMCR_FORCE_EEE100) > > So it wipes out all the PMCR settings that mt7531_cpu_port_config() > performed - undoing *everything* below that switch() statement in > mt7531_cpu_port_config()! > > Once the port_enable() method returns, DSA will then call > phylink_start(), which will trigger phylink to bring up the link > according to the settings it has, which will mean phylink calls > the mac_config(), pcs_config(), pcs_link_up() and mac_link_up() > with the appropriate parameters for the firmware described link. I'm slowly learning how DSA and phylink works, this is the full code path I could make up for the MT7530 DSA subdriver: mt7530_probe() & mt7988_probe() -> mt7530_probe_common() -> dsa_register_switch() -> dsa_switch_probe() -> dsa_tree_setup() -> dsa_tree_setup_switches() -> dsa_switch_setup() -> ds->ops->setup(): mt753x_setup() -> mt7530_setup() -> mt753x_cpu_port_enable() -> mt7531_setup() -> mt7531_setup_common() -> mt753x_cpu_port_enable() -> priv->info->cpu_port_config(): mt7531_cpu_port_config() -> mt7988_setup() -> mt7531_setup_common() -> mt753x_cpu_port_enable() -> priv->info->cpu_port_config(): mt7988_cpu_port_config() -> dsa_tree_setup_ports() -> dsa_port_setup() -> dsa_shared_port_link_register_of() -> dsa_shared_port_link_register_of() -> dsa_shared_port_phylink_register() -> dsa_port_phylink_create() -> ds->ops->phylink_get_caps(): mt753x_phylink_get_caps() -> phylink_create() -> INIT_WORK(&pl->resolve, phylink_resolve) -> dsa_port_enable() -> dsa_port_enable_rt() -> ds->ops->port_enable(): mt7530_port_enable() -> phylink_start() -> phylink_mac_initial_config() -> phylink_major_config() -> phylink_mac_config() -> pl->mac_ops->mac_config(): dsa_port_phylink_mac_config() -> ds->ops->phylink_mac_config(): mt753x_phylink_mac_config() -> pl->pcs->ops->pcs_config(): mt753x_pcs_config() -> phylink_enable_and_run_resolve() -> phylink_run_resolve() -> queue_work(system_power_efficient_wq, &pl->resolve) -> phylink_link_up() -> pl->pcs->ops->pcs_link_up(): mtk_pcs_lynxi_link_up() -> pl->mac_ops->mac_link_up(): dsa_port_phylink_mac_link_up() -> ds->ops->phylink_mac_link_up(): mt753x_phylink_mac_link_up() > > So I think I have the answer to my initial thought: do the calls in > mt7531_cpu_port_config() to the phylink methods have any use what so > ever? The answer is no, they are entirely useless. The same goes for > the other cpu_port_config() methods that do something similar. The > same goes for the PMCR register write that's changing any bits > included in PMCR_LINK_SETTINGS_MASK. > > What that means is that mt7988_cpu_port_config() can be entirely > removed, it serves no useful purpose what so ever. For > mt7531_cpu_port_config(), it only needs to set priv->p[56]_interface > which, as far as I can see, probably only avoids mac_config() doing > any pad setup (that's a guess.) This is what I also believe and the reason why I made this patch to simplify it. Looks like I'll just remove priv->info->cpu_port_config() instead. Arınç
On 11.06.2023 10:23, Arınç ÜNAL wrote: > > On 10.06.2023 20:55, Vladimir Oltean wrote: >> On Sat, Jun 10, 2023 at 01:57:27PM +0300, Arınç ÜNAL wrote: >>> I was able to confirm all user ports of the MT7531BE switch transmit/receive >>> traffic to/from the SGMII CPU port and computer fine after getting rid of >>> priv->info->cpu_port_config(). >>> >>> Tried all user ports being affine to the RGMII CPU port, that works too. >>> >>> https://github.com/arinc9/linux/commit/4e79313a95d45950cab526456ef0030286ba4d4e >> >> Did you do black-box testing after removing the code, or were you >> also able to independently confirm that the configurations done by >> cpu_port_config() were later overwritten? I'm trying to disambiguate >> between "works by coincidence" and "works because the analysis was >> correct". > > I did my testing, merely to make sure we didn't miss anything as Russell already stated that the configuration from cpu_port_config() is later overwritten. > > I could put some dev_info around to confirm the code path that overwrites the configuration. I have finally tested this. diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index a4468468b53c..7b60a67d016a 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -968,9 +968,11 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port) /* Setup max capability of CPU port at first */ if (priv->info->cpu_port_config) { + dev_info(priv->dev, "running cpu_port_config()\n"); ret = priv->info->cpu_port_config(ds, port); if (ret) return ret; + dev_info(priv->dev, "cpu_port_config() ran\n"); } /* Enable Mediatek header mode on the cpu port */ @@ -1024,6 +1026,9 @@ mt7530_port_enable(struct dsa_switch *ds, int port, priv->ports[port].pm); mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK); + if ((port == 5 || port == 6) && dsa_port_is_cpu(dp)) + dev_info(priv->dev, "MT7530_PMCR_P%d PMCR_LINK_SETTINGS_MASK is cleared\n", port); + mutex_unlock(&priv->reg_mutex); return 0; @@ -2693,6 +2698,9 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode, mcr_new |= PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN | PMCR_BACKPR_EN | PMCR_FORCE_MODE_ID(priv->id); + if ((port == 5 && dsa_is_cpu_port(ds, 5)) || (port == 6 && dsa_is_cpu_port(ds, 6))) + dev_info(priv->dev, "MT7530_PMCR_P%d PMCR_CPU_PORT_SETTING equivalent is set\n", port); + /* Are we connected to external phy */ if (port == 5 && dsa_is_user_port(ds, 5)) mcr_new |= PMCR_EXT_PHY; @@ -2760,6 +2768,9 @@ static void mt753x_phylink_mac_link_up(struct dsa_switch *ds, int port, } mt7530_set(priv, MT7530_PMCR_P(port), mcr); + + if ((port == 5 && dsa_is_cpu_port(ds, 5)) || (port == 6 && dsa_is_cpu_port(ds, 6))) + dev_info(priv->dev, "MT7530_PMCR_P%d PMCR_LINK_SETTINGS_MASK equivalent is set\n", port); } static int @@ -2796,6 +2807,9 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port) mt7530_write(priv, MT7530_PMCR_P(port), PMCR_CPU_PORT_SETTING(priv->id)); + + dev_info(priv->dev, "MT7530_PMCR_P%d PMCR_CPU_PORT_SETTING is set\n", port); + mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED, interface, NULL, speed, DUPLEX_FULL, true, true); [ 1.763066] mt7530-mdio mdio-bus:00: running cpu_port_config() [ 1.769237] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING is set [ 1.776724] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set [ 1.785254] mt7530-mdio mdio-bus:00: cpu_port_config() ran [ 1.792098] mt7530-mdio mdio-bus:00: running cpu_port_config() [ 1.798019] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING is set [ 1.805502] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set [ 1.814023] mt7530-mdio mdio-bus:00: cpu_port_config() ran [ 1.844941] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK is cleared [ 1.852972] mt7530-mdio mdio-bus:00: configuring for fixed/rgmii link mode [ 1.859944] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING equivalent is set [ 1.868658] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set [ 1.868913] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK is cleared [ 1.877190] mt7530-mdio mdio-bus:00: Link is Up - 1Gbps/Full - flow control rx/tx [ 1.885179] mt7530-mdio mdio-bus:00: configuring for fixed/2500base-x link mode [ 1.899973] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING equivalent is set [ 1.910147] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set [ 1.918681] mt7530-mdio mdio-bus:00: Link is Up - 2.5Gbps/Full - flow control rx/tx [ 1.920654] mt7530-mdio mdio-bus:00 wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7531 PHY] (irq=137) [ 1.948453] mt7530-mdio mdio-bus:00 lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7531 PHY] (irq=138) [ 1.970382] mt7530-mdio mdio-bus:00 lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7531 PHY] (irq=139) [ 1.992423] mt7530-mdio mdio-bus:00 lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7531 PHY] (irq=140) [ 2.014310] mt7530-mdio mdio-bus:00 lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7531 PHY] (irq=141) [ 2.025396] mtk_soc_eth 1b100000.ethernet eth1: entered promiscuous mode [ 2.032160] mtk_soc_eth 1b100000.ethernet eth0: entered promiscuous mode [ 2.038912] DSA: tree 0 setup Arınç
On Wed, Jan 10, 2024 at 02:15:57PM +0300, Arınç ÜNAL wrote: > On 11.06.2023 10:23, Arınç ÜNAL wrote: > > > > On 10.06.2023 20:55, Vladimir Oltean wrote: > > > On Sat, Jun 10, 2023 at 01:57:27PM +0300, Arınç ÜNAL wrote: > > > > I was able to confirm all user ports of the MT7531BE switch transmit/receive > > > > traffic to/from the SGMII CPU port and computer fine after getting rid of > > > > priv->info->cpu_port_config(). > > > > > > > > Tried all user ports being affine to the RGMII CPU port, that works too. > > > > > > > > https://github.com/arinc9/linux/commit/4e79313a95d45950cab526456ef0030286ba4d4e > > > > > > Did you do black-box testing after removing the code, or were you > > > also able to independently confirm that the configurations done by > > > cpu_port_config() were later overwritten? I'm trying to disambiguate > > > between "works by coincidence" and "works because the analysis was > > > correct". > > > > I did my testing, merely to make sure we didn't miss anything as Russell already stated that the configuration from cpu_port_config() is later overwritten. > > > > I could put some dev_info around to confirm the code path that overwrites the configuration. > > I have finally tested this. Replying to a question from 6 months ago is nice of you, like replying to any question is. But everybody's short memory is by now hit like a cold cache, everything has been forgotten. I don't even have this thread in my inbox anymore, it's in the "seen" folder. There's something to be said about having to re-read a long thread and the code for 30 minutes, just to reply "Ok". I think you need to develop a better feeling for when to let go of past discussions when they become stale, summarize the essence in the commit description of a patch, and then just resubmit that new patch. People will have to open the code and make a fresh analysis anyway, so just help them skip reading past discussions and just focus on the conclusion. > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index a4468468b53c..7b60a67d016a 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -968,9 +968,11 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port) > /* Setup max capability of CPU port at first */ > if (priv->info->cpu_port_config) { > + dev_info(priv->dev, "running cpu_port_config()\n"); > ret = priv->info->cpu_port_config(ds, port); > if (ret) > return ret; > + dev_info(priv->dev, "cpu_port_config() ran\n"); > } > /* Enable Mediatek header mode on the cpu port */ > @@ -1024,6 +1026,9 @@ mt7530_port_enable(struct dsa_switch *ds, int port, > priv->ports[port].pm); > mt7530_clear(priv, MT7530_PMCR_P(port), PMCR_LINK_SETTINGS_MASK); > + if ((port == 5 || port == 6) && dsa_port_is_cpu(dp)) > + dev_info(priv->dev, "MT7530_PMCR_P%d PMCR_LINK_SETTINGS_MASK is cleared\n", port); > + FYI, you can prefix your prints with something like this to make the log easier to follow in terms of code paths taken. "%s called from %pS <- %pS: ...\n", __func__, __builtin_return_address(0), __builtin_return_address(1) > mutex_unlock(&priv->reg_mutex); > return 0; > @@ -2693,6 +2698,9 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode, > mcr_new |= PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN | > PMCR_BACKPR_EN | PMCR_FORCE_MODE_ID(priv->id); > + if ((port == 5 && dsa_is_cpu_port(ds, 5)) || (port == 6 && dsa_is_cpu_port(ds, 6))) > + dev_info(priv->dev, "MT7530_PMCR_P%d PMCR_CPU_PORT_SETTING equivalent is set\n", port); > + > /* Are we connected to external phy */ > if (port == 5 && dsa_is_user_port(ds, 5)) > mcr_new |= PMCR_EXT_PHY; > @@ -2760,6 +2768,9 @@ static void mt753x_phylink_mac_link_up(struct dsa_switch *ds, int port, > } > mt7530_set(priv, MT7530_PMCR_P(port), mcr); > + > + if ((port == 5 && dsa_is_cpu_port(ds, 5)) || (port == 6 && dsa_is_cpu_port(ds, 6))) > + dev_info(priv->dev, "MT7530_PMCR_P%d PMCR_LINK_SETTINGS_MASK equivalent is set\n", port); > } > static int > @@ -2796,6 +2807,9 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port) > mt7530_write(priv, MT7530_PMCR_P(port), > PMCR_CPU_PORT_SETTING(priv->id)); > + > + dev_info(priv->dev, "MT7530_PMCR_P%d PMCR_CPU_PORT_SETTING is set\n", port); > + > mt753x_phylink_mac_link_up(ds, port, MLO_AN_FIXED, interface, NULL, > speed, DUPLEX_FULL, true, true); > > [ 1.763066] mt7530-mdio mdio-bus:00: running cpu_port_config() > [ 1.769237] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING is set > [ 1.776724] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set > [ 1.785254] mt7530-mdio mdio-bus:00: cpu_port_config() ran This is from mt7531_setup_common(), for port 5. > [ 1.792098] mt7530-mdio mdio-bus:00: running cpu_port_config() > [ 1.798019] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING is set > [ 1.805502] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set > [ 1.814023] mt7530-mdio mdio-bus:00: cpu_port_config() ran This is from mt7531_setup_common(), for port 6. > [ 1.844941] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK is cleared This is from mt7530_port_enable() for port 5, undoing what mt7531_setup_common() has done. It also seems bogus BTW, the enable() function is doing the same "clear" as mt7530_port_disable() is doing, rather than mt7530_set(). Were it not for what's to come [1], this would be a bug with an actual user impact. > [ 1.852972] mt7530-mdio mdio-bus:00: configuring for fixed/rgmii link mode > [ 1.859944] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING equivalent is set This is from mt753x_phylink_mac_config(), for port 5, partially overwriting what mt7531_setup_common() has done. > [ 1.868658] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set [1] This is from mt753x_phylink_mac_link_up(), for port 5, overwriting what mt7530_port_enable() has done. I suspect that, in addition to Russell's analysis, modifying PMCR_LINK_SETTINGS_MASK from the port_enable()/ port_disable() ops is also something that can be removed. > [ 1.868913] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK is cleared > [ 1.877190] mt7530-mdio mdio-bus:00: Link is Up - 1Gbps/Full - flow control rx/tx > [ 1.885179] mt7530-mdio mdio-bus:00: configuring for fixed/2500base-x link mode > [ 1.899973] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING equivalent is set > [ 1.910147] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set > [ 1.918681] mt7530-mdio mdio-bus:00: Link is Up - 2.5Gbps/Full - flow control rx/tx > [ 1.920654] mt7530-mdio mdio-bus:00 wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7531 PHY] (irq=137) > [ 1.948453] mt7530-mdio mdio-bus:00 lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7531 PHY] (irq=138) > [ 1.970382] mt7530-mdio mdio-bus:00 lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7531 PHY] (irq=139) > [ 1.992423] mt7530-mdio mdio-bus:00 lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7531 PHY] (irq=140) > [ 2.014310] mt7530-mdio mdio-bus:00 lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7531 PHY] (irq=141) > [ 2.025396] mtk_soc_eth 1b100000.ethernet eth1: entered promiscuous mode > [ 2.032160] mtk_soc_eth 1b100000.ethernet eth0: entered promiscuous mode > [ 2.038912] DSA: tree 0 setup > > Arınç And this is all the same for port 6. So yes, it would be good to consolidate the code to follow a simple principle. Any register fields should be modified only by the set of methods that they pertain to. In this case, MT7530_PMCR_P appears to only hold link control information, so it pertains to phylink's methods. Otherwise, the natural consequence is that they will get unexpectedly overwritten. It seems outside of the competence of ds->ops->port_enable() and ds->ops->port_disable(). Those would be appropriate, for example, to control the switching matrix settings between a user port and its corresponding CPU port (but not any more switching matrix settings - those pertain to port_bridge_join() and port_bridge_leave()). I hope this helps.
On 10.01.2024 17:27, Vladimir Oltean wrote: > On Wed, Jan 10, 2024 at 02:15:57PM +0300, Arınç ÜNAL wrote: >> I have finally tested this. > > Replying to a question from 6 months ago is nice of you, like replying > to any question is. But everybody's short memory is by now hit like a > cold cache, everything has been forgotten. I don't even have this thread > in my inbox anymore, it's in the "seen" folder. > > There's something to be said about having to re-read a long thread and > the code for 30 minutes, just to reply "Ok". > > I think you need to develop a better feeling for when to let go of past > discussions when they become stale, summarize the essence in the commit > description of a patch, and then just resubmit that new patch. People > will have to open the code and make a fresh analysis anyway, so just > help them skip reading past discussions and just focus on the conclusion. I agree, thanks for bearing with me here. > > FYI, you can prefix your prints with something like this to make the log > easier to follow in terms of code paths taken. > > "%s called from %pS <- %pS: ...\n", > __func__, __builtin_return_address(0), __builtin_return_address(1) __builtin_return_address(1) doesn't seem to work. I'm running this on arm64. Apart from that, it works well. Thank you. [ 1.863034] mt7530-mdio mdio-bus:00: mt753x_cpu_port_enable called from mt7531_setup_common+0x32c/0x370 <- 0x0: running cpu_port_config() [ 1.875736] mt7530-mdio mdio-bus:00: mt7531_cpu_port_config called from mt753x_cpu_port_enable+0x64/0x23c <- 0x0: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING is set [ 1.889922] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_link_up called from mt7531_cpu_port_config+0xe8/0x12c <- 0x0: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set [ 1.905491] mt7530-mdio mdio-bus:00: cpu_port_config() ran [ 1.912336] mt7530-mdio mdio-bus:00: mt753x_cpu_port_enable called from mt7531_setup_common+0x32c/0x370 <- 0x0: running cpu_port_config() [ 1.924777] mt7530-mdio mdio-bus:00: mt7531_cpu_port_config called from mt753x_cpu_port_enable+0x64/0x23c <- 0x0: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING is set [ 1.938953] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_link_up called from mt7531_cpu_port_config+0xe8/0x12c <- 0x0: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set [ 1.954525] mt7530-mdio mdio-bus:00: cpu_port_config() ran [ 1.985409] mt7530-mdio mdio-bus:00: mt7530_port_enable called from dsa_port_enable_rt+0x2c/0x98 <- 0x0: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK is cleared [ 1.999378] mt7530-mdio mdio-bus:00: configuring for fixed/rgmii link mode [ 2.006347] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_config called from dsa_port_phylink_mac_config+0x30/0x3c <- 0x0: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING equivalent is set [ 2.022197] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_link_up called from dsa_port_phylink_mac_link_up+0x48/0x74 <- 0x0: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set [ 2.022645] mt7530-mdio mdio-bus:00: mt7530_port_enable called from dsa_port_enable_rt+0x2c/0x98 <- 0x0: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK is cleared [ 2.038203] mt7530-mdio mdio-bus:00: Link is Up - 1Gbps/Full - flow control rx/tx [ 2.052090] mt7530-mdio mdio-bus:00: configuring for fixed/2500base-x link mode [ 2.066894] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_config called from dsa_port_phylink_mac_config+0x30/0x3c <- 0x0: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING equivalent is set [ 2.084406] mt7530-mdio mdio-bus:00: mt753x_phylink_mac_link_up called from dsa_port_phylink_mac_link_up+0x48/0x74 <- 0x0: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set [ 2.095093] mt7530-mdio mdio-bus:00 wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7531 PHY] (irq=137) [ 2.100427] mt7530-mdio mdio-bus:00: Link is Up - 2.5Gbps/Full - flow control rx/tx > >> [ 1.763066] mt7530-mdio mdio-bus:00: running cpu_port_config() >> [ 1.769237] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING is set >> [ 1.776724] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set >> [ 1.785254] mt7530-mdio mdio-bus:00: cpu_port_config() ran > > This is from mt7531_setup_common(), for port 5. > >> [ 1.792098] mt7530-mdio mdio-bus:00: running cpu_port_config() >> [ 1.798019] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING is set >> [ 1.805502] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set >> [ 1.814023] mt7530-mdio mdio-bus:00: cpu_port_config() ran > > This is from mt7531_setup_common(), for port 6. > >> [ 1.844941] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK is cleared > > This is from mt7530_port_enable() for port 5, undoing what mt7531_setup_common() has done. > It also seems bogus BTW, the enable() function is doing the same "clear" > as mt7530_port_disable() is doing, rather than mt7530_set(). Were it not > for what's to come [1], this would be a bug with an actual user impact. > >> [ 1.852972] mt7530-mdio mdio-bus:00: configuring for fixed/rgmii link mode >> [ 1.859944] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_CPU_PORT_SETTING equivalent is set > > This is from mt753x_phylink_mac_config(), for port 5, partially > overwriting what mt7531_setup_common() has done. > >> [ 1.868658] mt7530-mdio mdio-bus:00: MT7530_PMCR_P5 PMCR_LINK_SETTINGS_MASK equivalent is set > > [1] This is from mt753x_phylink_mac_link_up(), for port 5, overwriting what > mt7530_port_enable() has done. I suspect that, in addition to Russell's > analysis, modifying PMCR_LINK_SETTINGS_MASK from the port_enable()/ > port_disable() ops is also something that can be removed. > >> [ 1.868913] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK is cleared >> [ 1.877190] mt7530-mdio mdio-bus:00: Link is Up - 1Gbps/Full - flow control rx/tx >> [ 1.885179] mt7530-mdio mdio-bus:00: configuring for fixed/2500base-x link mode >> [ 1.899973] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_CPU_PORT_SETTING equivalent is set >> [ 1.910147] mt7530-mdio mdio-bus:00: MT7530_PMCR_P6 PMCR_LINK_SETTINGS_MASK equivalent is set >> [ 1.918681] mt7530-mdio mdio-bus:00: Link is Up - 2.5Gbps/Full - flow control rx/tx >> [ 1.920654] mt7530-mdio mdio-bus:00 wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7531 PHY] (irq=137) >> [ 1.948453] mt7530-mdio mdio-bus:00 lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7531 PHY] (irq=138) >> [ 1.970382] mt7530-mdio mdio-bus:00 lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7531 PHY] (irq=139) >> [ 1.992423] mt7530-mdio mdio-bus:00 lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7531 PHY] (irq=140) >> [ 2.014310] mt7530-mdio mdio-bus:00 lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7531 PHY] (irq=141) >> [ 2.025396] mtk_soc_eth 1b100000.ethernet eth1: entered promiscuous mode >> [ 2.032160] mtk_soc_eth 1b100000.ethernet eth0: entered promiscuous mode >> [ 2.038912] DSA: tree 0 setup >> >> Arınç > > And this is all the same for port 6. > > So yes, it would be good to consolidate the code to follow a simple principle. > Any register fields should be modified only by the set of methods that > they pertain to. In this case, MT7530_PMCR_P appears to only hold link > control information, so it pertains to phylink's methods. Otherwise, > the natural consequence is that they will get unexpectedly overwritten. > > It seems outside of the competence of ds->ops->port_enable() and > ds->ops->port_disable(). Those would be appropriate, for example, to > control the switching matrix settings between a user port and its > corresponding CPU port (but not any more switching matrix settings - > those pertain to port_bridge_join() and port_bridge_leave()). > > I hope this helps. This is very helpful, thank you very much. This is what I deduct I should do: First patch: Get rid of cpu_port_config(). Second patch: Collect port link control register operations from port_enable/port_disable and phylink_mac_config to phylink_mac_link_up/phylink_mac_link_down. Arınç
On Wed, Jan 10, 2024 at 08:15:20PM +0300, Arınç ÜNAL wrote: > __builtin_return_address(1) doesn't seem to work. I'm running this on arm64. I can't tell you why either, I'm sorry. I can just point to the documentation, which does specify that "On some machines it may be impossible to determine the return address of any function other than the current one". If somebody knows what this depends on, feel free to interject. https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html On my NXP LS1028A (also arm64) plus clang-16 compiler, __builtin_return_address() does work with multiple nesting levels. > This is very helpful, thank you very much. This is what I deduct I should > do: > > First patch: Get rid of cpu_port_config(). > > Second patch: Collect port link control register operations from > port_enable/port_disable and phylink_mac_config to > phylink_mac_link_up/phylink_mac_link_down. I guess. Sounds good.
On Wed, Jan 10, 2024 at 08:05:25PM +0200, Vladimir Oltean wrote: > On Wed, Jan 10, 2024 at 08:15:20PM +0300, Arınç ÜNAL wrote: > > __builtin_return_address(1) doesn't seem to work. I'm running this on arm64. > > I can't tell you why either, I'm sorry. I can just point to the > documentation, which does specify that "On some machines it may be > impossible to determine the return address of any function other than > the current one". If somebody knows what this depends on, feel free to > interject. > https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html > > On my NXP LS1028A (also arm64) plus clang-16 compiler, __builtin_return_address() > does work with multiple nesting levels. gcc will probably need to be using frame pointers so it can walk the stack, if gcc even implements non-zero values to __builtin_return_address(). Without frame pointers, it would need an unwinder.
On Wed, Jan 10, 2024 at 06:31:11PM +0000, Russell King (Oracle) wrote: > On Wed, Jan 10, 2024 at 08:05:25PM +0200, Vladimir Oltean wrote: > > On Wed, Jan 10, 2024 at 08:15:20PM +0300, Arınç ÜNAL wrote: > > > __builtin_return_address(1) doesn't seem to work. I'm running this on arm64. > > > > I can't tell you why either, I'm sorry. I can just point to the > > documentation, which does specify that "On some machines it may be > > impossible to determine the return address of any function other than > > the current one". If somebody knows what this depends on, feel free to > > interject. > > https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html > > > > On my NXP LS1028A (also arm64) plus clang-16 compiler, __builtin_return_address() > > does work with multiple nesting levels. > > gcc will probably need to be using frame pointers so it can walk the > stack, if gcc even implements non-zero values to > __builtin_return_address(). Without frame pointers, it would need an > unwinder. Yeah, I guess it's a gcc limitation. I recompiled the kernel for the same platform with gcc 11.2 from Arm, and I get the same result as Arınç now.
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 710c6622d648..d837aa20968c 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -2234,8 +2234,6 @@ mt7530_setup(struct dsa_switch *ds) val |= MHWTRAP_MANUAL; mt7530_write(priv, MT7530_MHWTRAP, val); - priv->p6_interface = PHY_INTERFACE_MODE_NA; - /* Enable and reset MIB counters */ mt7530_mib_reset(ds); @@ -2455,10 +2453,6 @@ mt7531_setup(struct dsa_switch *ds) mt7530_rmw(priv, MT7531_GPIO_MODE0, MT7531_GPIO0_MASK, MT7531_GPIO0_INTERRUPT); - /* Let phylink decide the interface later. */ - priv->p5_interface = PHY_INTERFACE_MODE_NA; - priv->p6_interface = PHY_INTERFACE_MODE_NA; - /* Enable PHY core PLL, since phy_device has not yet been created * provided for phy_[read,write]_mmd_indirect is called, we provide * our own mt7531_ind_mmd_phy_[read,write] to complete this @@ -2728,25 +2722,20 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode, goto unsupported; break; case 5: /* Port 5, can be used as a CPU port. */ - if (priv->p5_interface == state->interface) + if (priv->p5_configured) break; if (mt753x_mac_config(ds, port, mode, state) < 0) goto unsupported; - - if (priv->p5_intf_sel != P5_DISABLED) - priv->p5_interface = state->interface; break; case 6: /* Port 6, can be used as a CPU port. */ - if (priv->p6_interface == state->interface) + if (priv->p6_configured) break; mt753x_pad_setup(ds, state); if (mt753x_mac_config(ds, port, mode, state) < 0) goto unsupported; - - priv->p6_interface = state->interface; break; default: unsupported: @@ -2854,12 +2843,12 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port) else interface = PHY_INTERFACE_MODE_2500BASEX; - priv->p5_interface = interface; + priv->p5_configured = true; break; case 6: interface = PHY_INTERFACE_MODE_2500BASEX; - priv->p6_interface = interface; + priv->p6_configured = true; break; default: return -EINVAL; diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h index 2602c95fd3a5..06037be5882c 100644 --- a/drivers/net/dsa/mt7530.h +++ b/drivers/net/dsa/mt7530.h @@ -745,8 +745,10 @@ struct mt753x_info { * @ports: Holding the state among ports * @reg_mutex: The lock for protecting among process accessing * registers - * @p6_interface: Holding the current port 6 interface - * @p5_interface: Holding the current port 5 interface + * @p6_configured: Flag for distinguishing if port 6 of the MT7531 switch + * is already configured + * @p5_configured: Flag for distinguishing if port 5 of the MT7531 switch + * is already configured * @p5_intf_sel: Holding the current port 5 interface select * @p5_sgmii: Flag for distinguishing if port 5 of the MT7531 switch * has got SGMII @@ -767,8 +769,8 @@ struct mt7530_priv { const struct mt753x_info *info; unsigned int id; bool mcm; - phy_interface_t p6_interface; - phy_interface_t p5_interface; + bool p6_configured; + bool p5_configured; enum p5_interface_select p5_intf_sel; bool p5_sgmii; u8 mirror_rx;