* [PATCH net-next v3 0/2] dsa: lan9303: Move to PHYLINK @ 2022-12-06 18:34 Jerry Ray 2022-12-06 18:34 ` [PATCH net-next v3 1/2] dsa: lan9303: Add port_max_mtu API Jerry Ray ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Jerry Ray @ 2022-12-06 18:34 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, linux, Jerry Ray This patch series moves the lan9303 driver to use the phylink api away from phylib. 1) adds port_max_mtu api support. 2) Replace .adjust_link with .phylink_get_caps dsa api Clearing the Turbo Mode bit previously done in the adjust_link API is moved to the driver initialization immediately following the successful detection of a LAN93xx device. It is forced to a disabled state and never enabled. At this point, I do not see anything this driver needs from the other phylink APIs. Signed-off-by: Jerry Ray <jerry.ray@microchip.com> --- v2-> v3: Added back in disabling Turbo Mode on the CPU MII interface. Removed the unnecessary clearing of the phyvsupported interfaces. v1-> v2: corrected the reported mtu size, removing ETH_HLEN and ETH_FCS_LEN drivers/net/dsa/lan9303-core.c | 93 ++++++++++++-------- 1 file changed, 56 insertions(+), 37 deletions(-) ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next v3 1/2] dsa: lan9303: Add port_max_mtu API 2022-12-06 18:34 [PATCH net-next v3 0/2] dsa: lan9303: Move to PHYLINK Jerry Ray @ 2022-12-06 18:34 ` Jerry Ray 2022-12-06 18:56 ` Vladimir Oltean 2022-12-06 18:35 ` [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK Jerry Ray 2022-12-06 18:57 ` [PATCH net-next v3 0/2] " Vladimir Oltean 2 siblings, 1 reply; 13+ messages in thread From: Jerry Ray @ 2022-12-06 18:34 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, linux, Jerry Ray Adding in support for reporting the max mtu for a given port. Signed-off-by: Jerry Ray <jerry.ray@microchip.com> --- drivers/net/dsa/lan9303-core.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c index 80f07bd20593..baa336bb9d15 100644 --- a/drivers/net/dsa/lan9303-core.c +++ b/drivers/net/dsa/lan9303-core.c @@ -1279,6 +1279,25 @@ static int lan9303_port_mdb_del(struct dsa_switch *ds, int port, return 0; } +/* For non-cpu ports, the max frame size is 1518. + * The CPU port supports a max frame size of 1522. + * There is a JUMBO flag to make the max size 2048, but this driver + * presently does not support using it. + */ +static int lan9303_port_max_mtu(struct dsa_switch *ds, int port) +{ + struct net_device *p = dsa_port_to_master(dsa_to_port(ds, port)); + struct lan9303 *chip = ds->priv; + + dev_dbg(chip->dev, "%s(%d) entered. NET max_mtu is %d", + __func__, port, p->max_mtu); + + if (dsa_port_is_cpu(dsa_to_port(ds, port))) + return 1522 - ETH_HLEN - ETH_FCS_LEN; + else + return 1518 - ETH_HLEN - ETH_FCS_LEN; +} + static const struct dsa_switch_ops lan9303_switch_ops = { .get_tag_protocol = lan9303_get_tag_protocol, .setup = lan9303_setup, @@ -1299,6 +1318,7 @@ static const struct dsa_switch_ops lan9303_switch_ops = { .port_fdb_dump = lan9303_port_fdb_dump, .port_mdb_add = lan9303_port_mdb_add, .port_mdb_del = lan9303_port_mdb_del, + .port_max_mtu = lan9303_port_max_mtu, }; static int lan9303_register_switch(struct lan9303 *chip) -- 2.17.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net-next v3 1/2] dsa: lan9303: Add port_max_mtu API 2022-12-06 18:34 ` [PATCH net-next v3 1/2] dsa: lan9303: Add port_max_mtu API Jerry Ray @ 2022-12-06 18:56 ` Vladimir Oltean 2022-12-06 23:44 ` Jerry.Ray 0 siblings, 1 reply; 13+ messages in thread From: Vladimir Oltean @ 2022-12-06 18:56 UTC (permalink / raw) To: Jerry Ray Cc: Andrew Lunn, Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, linux On Tue, Dec 06, 2022 at 12:34:59PM -0600, Jerry Ray wrote: > +/* For non-cpu ports, the max frame size is 1518. > + * The CPU port supports a max frame size of 1522. > + * There is a JUMBO flag to make the max size 2048, but this driver > + * presently does not support using it. > + */ > +static int lan9303_port_max_mtu(struct dsa_switch *ds, int port) > +{ > + struct net_device *p = dsa_port_to_master(dsa_to_port(ds, port)); You can put debugging prints in the code, but please, in the code that you submit, do remove gratuitous poking in the master net_device. > + struct lan9303 *chip = ds->priv; > + > + dev_dbg(chip->dev, "%s(%d) entered. NET max_mtu is %d", > + __func__, port, p->max_mtu); > + > + if (dsa_port_is_cpu(dsa_to_port(ds, port))) The ds->ops->port_max_mtu() function is never called for the CPU port. You must know this, you put a debugging print right above. If this would have been called for anything other than user ports, dsa_port_to_master() would have triggered a NULL pointer dereference (dp->cpu_dp is set to NULL for CPU ports). So please remove dead code. > + return 1522 - ETH_HLEN - ETH_FCS_LEN; > + else > + return 1518 - ETH_HLEN - ETH_FCS_LEN; Please replace "1518 - ETH_HLEN - ETH_FCS_LEN" with "ETH_DATA_LEN". Which brings me to a more serious question. If you say that the max_mtu is equal to the default interface MTU (1500), and you provide no means for the user to change the MTU to a different value, then why write the patch? What behaves differently with and without it? > +} > + > static const struct dsa_switch_ops lan9303_switch_ops = { > .get_tag_protocol = lan9303_get_tag_protocol, > .setup = lan9303_setup, > @@ -1299,6 +1318,7 @@ static const struct dsa_switch_ops lan9303_switch_ops = { > .port_fdb_dump = lan9303_port_fdb_dump, > .port_mdb_add = lan9303_port_mdb_add, > .port_mdb_del = lan9303_port_mdb_del, > + .port_max_mtu = lan9303_port_max_mtu, > }; ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH net-next v3 1/2] dsa: lan9303: Add port_max_mtu API 2022-12-06 18:56 ` Vladimir Oltean @ 2022-12-06 23:44 ` Jerry.Ray 2022-12-07 11:39 ` Vladimir Oltean 0 siblings, 1 reply; 13+ messages in thread From: Jerry.Ray @ 2022-12-06 23:44 UTC (permalink / raw) To: olteanv Cc: andrew, f.fainelli, davem, edumazet, kuba, pabeni, netdev, linux-kernel, linux > > +/* For non-cpu ports, the max frame size is 1518. > > + * The CPU port supports a max frame size of 1522. > > + * There is a JUMBO flag to make the max size 2048, but this driver > > + * presently does not support using it. > > + */ > > +static int lan9303_port_max_mtu(struct dsa_switch *ds, int port) > > +{ > > + struct net_device *p = dsa_port_to_master(dsa_to_port(ds, port)); > > You can put debugging prints in the code, but please, in the code that > you submit, do remove gratuitous poking in the master net_device. > > > + struct lan9303 *chip = ds->priv; > > + > > + dev_dbg(chip->dev, "%s(%d) entered. NET max_mtu is %d", > > + __func__, port, p->max_mtu); > > + > > + if (dsa_port_is_cpu(dsa_to_port(ds, port))) > > The ds->ops->port_max_mtu() function is never called for the CPU port. > You must know this, you put a debugging print right above. If this would > have been called for anything other than user ports, dsa_port_to_master() > would have triggered a NULL pointer dereference (dp->cpu_dp is set to > NULL for CPU ports). > > So please remove dead code. > I've written the function to handle being called with any port. While I couldn't directly exercise calling the port_max_mtu with the cpu port, I did simulate it to verify it would work. I'm using the dsa_to_port() rather than the dsa_port_to_master() function. I'd rather include support for calling the api with the cpu port. I didn't want to assume otherwise. That's why I don't consider this dead code. > > + return 1522 - ETH_HLEN - ETH_FCS_LEN; > > + else > > + return 1518 - ETH_HLEN - ETH_FCS_LEN; > > Please replace "1518 - ETH_HLEN - ETH_FCS_LEN" with "ETH_DATA_LEN". > > Which brings me to a more serious question. If you say that the max_mtu > is equal to the default interface MTU (1500), and you provide no means > for the user to change the MTU to a different value, then why write the > patch? What behaves differently with and without it? > I began adding the port_max_mtu api to attempt to get rid of the following error message: "macb f802c000.ethernet eth0: error -22 setting MTU to 1504 to include DSA overhead" If someone were to check the max_mtu supported on the CPU port of the LAN9303, they would see that 1504 is okay. > > +} > > + > > static const struct dsa_switch_ops lan9303_switch_ops = { > > .get_tag_protocol = lan9303_get_tag_protocol, > > .setup = lan9303_setup, > > @@ -1299,6 +1318,7 @@ static const struct dsa_switch_ops lan9303_switch_ops = { > > .port_fdb_dump = lan9303_port_fdb_dump, > > .port_mdb_add = lan9303_port_mdb_add, > > .port_mdb_del = lan9303_port_mdb_del, > > + .port_max_mtu = lan9303_port_max_mtu, > > }; > Regards, Jerry. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next v3 1/2] dsa: lan9303: Add port_max_mtu API 2022-12-06 23:44 ` Jerry.Ray @ 2022-12-07 11:39 ` Vladimir Oltean 0 siblings, 0 replies; 13+ messages in thread From: Vladimir Oltean @ 2022-12-07 11:39 UTC (permalink / raw) To: Jerry.Ray Cc: andrew, f.fainelli, davem, edumazet, kuba, pabeni, netdev, linux-kernel, linux On Tue, Dec 06, 2022 at 11:44:58PM +0000, Jerry.Ray@microchip.com wrote: > > > +/* For non-cpu ports, the max frame size is 1518. > > > + * The CPU port supports a max frame size of 1522. > > > + * There is a JUMBO flag to make the max size 2048, but this driver > > > + * presently does not support using it. > > > + */ > > > +static int lan9303_port_max_mtu(struct dsa_switch *ds, int port) > > > +{ > > > + struct net_device *p = dsa_port_to_master(dsa_to_port(ds, port)); > > > > You can put debugging prints in the code, but please, in the code that > > you submit, do remove gratuitous poking in the master net_device. > > > > > + struct lan9303 *chip = ds->priv; > > > + > > > + dev_dbg(chip->dev, "%s(%d) entered. NET max_mtu is %d", > > > + __func__, port, p->max_mtu); > > > + > > > + if (dsa_port_is_cpu(dsa_to_port(ds, port))) > > > > The ds->ops->port_max_mtu() function is never called for the CPU port. > > You must know this, you put a debugging print right above. If this would > > have been called for anything other than user ports, dsa_port_to_master() > > would have triggered a NULL pointer dereference (dp->cpu_dp is set to > > NULL for CPU ports). > > > > So please remove dead code. > > > > I've written the function to handle being called with any port. While I > couldn't directly exercise calling the port_max_mtu with the cpu port, I did > simulate it to verify it would work. > > I'm using the dsa_to_port() rather than the dsa_port_to_master() function. No, you're using the dsa_to_port() *and* the dsa_port_to_master() functions. See? It's in the code you posted: static int lan9303_port_max_mtu(struct dsa_switch *ds, int port) { struct net_device *p = dsa_port_to_master(dsa_to_port(ds, port)); ~~~~~~~~~~~~~~~~~~ > I'd rather include support for calling the api with the cpu port. I didn't > want to assume otherwise. That's why I don't consider this dead code. > > > > + return 1522 - ETH_HLEN - ETH_FCS_LEN; > > > + else > > > + return 1518 - ETH_HLEN - ETH_FCS_LEN; > > > > Please replace "1518 - ETH_HLEN - ETH_FCS_LEN" with "ETH_DATA_LEN". > > > > Which brings me to a more serious question. If you say that the max_mtu > > is equal to the default interface MTU (1500), and you provide no means > > for the user to change the MTU to a different value, then why write the > > patch? What behaves differently with and without it? > > > > I began adding the port_max_mtu api to attempt to get rid of the following > error message: > "macb f802c000.ethernet eth0: error -22 setting MTU to 1504 to include DSA overhead" And how well did that go? That error message is saying that the macb driver (drivers/net/ethernet/cadence/macb_main.c) does not accept the MTU of 1504. Maybe because it doesn't have MACB_CAPS_JUMBO, I don't know. But this patch is clearly unrelated to the problem you've observed. > If someone were to check the max_mtu supported on the CPU port of the LAN9303, > they would see that 1504 is okay. No, they would not see that 1504 is okay. They would get a NULL pointer dereference in your function, if port_max_mtu() was ever called for a CPU port. Don't believe me? You don't even have to. Please look at this patch, study it, run it, and see what happens with your port_max_mtu() implementation. diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index e5f156940c67..636e4b4df79a 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -473,6 +473,12 @@ static int dsa_port_setup(struct dsa_port *dp) break; dsa_port_enabled = true; + if (ds->ops->port_max_mtu) { + dev_info(ds->dev, "max MTU of CPU port %d is %d\n", + dp->index, + ds->ops->port_max_mtu(ds, dp->index)); + } + break; case DSA_PORT_TYPE_DSA: if (dp->dn) { The max_mtu of the CPU port is simply a question that the DSA core does not ask, so there's no reason to report it. How things are supposed to work is that the max_mtu of the user ports is propagated to their net_devices, and when the MTU of any user port is changed, the port_change_mtu() of that user port is called, and the maximum MTU of all user ports is recalculated and all CPU and DSA ports also get a port_change_mtu() call with that maximum value. If those ports need to program their hardware with something that also includes their tagging protocol overhead, they do so privately. ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK 2022-12-06 18:34 [PATCH net-next v3 0/2] dsa: lan9303: Move to PHYLINK Jerry Ray 2022-12-06 18:34 ` [PATCH net-next v3 1/2] dsa: lan9303: Add port_max_mtu API Jerry Ray @ 2022-12-06 18:35 ` Jerry Ray 2022-12-06 19:32 ` Vladimir Oltean 2022-12-06 18:57 ` [PATCH net-next v3 0/2] " Vladimir Oltean 2 siblings, 1 reply; 13+ messages in thread From: Jerry Ray @ 2022-12-06 18:35 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, linux, Jerry Ray This patch replaces the .adjust_link api with the .phylink_get_caps api. Signed-off-by: Jerry Ray <jerry.ray@microchip.com> --- v2-> v3: Added back in disabling Turbo Mode on the CPU MII interface. Removed the unnecessary clearing of the phy supported interfaces. --- drivers/net/dsa/lan9303-core.c | 79 ++++++++++++++++++---------------- 1 file changed, 42 insertions(+), 37 deletions(-) diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c index baa336bb9d15..c6236b328ed8 100644 --- a/drivers/net/dsa/lan9303-core.c +++ b/drivers/net/dsa/lan9303-core.c @@ -886,6 +886,13 @@ static int lan9303_check_device(struct lan9303 *chip) return ret; } + /* Virtual Phy: Always disable Turbo 200Mbit mode */ + ret = lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, ®); + if (ret) + return ret; + reg &= ~LAN9303_VIRT_SPECIAL_TURBO; + regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, reg); + return 0; } @@ -1047,42 +1054,6 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum, return chip->ops->phy_write(chip, phy, regnum, val); } -static void lan9303_adjust_link(struct dsa_switch *ds, int port, - struct phy_device *phydev) -{ - struct lan9303 *chip = ds->priv; - int ctl; - - if (!phy_is_pseudo_fixed_link(phydev)) - return; - - ctl = lan9303_phy_read(ds, port, MII_BMCR); - - ctl &= ~BMCR_ANENABLE; - - if (phydev->speed == SPEED_100) - ctl |= BMCR_SPEED100; - else if (phydev->speed == SPEED_10) - ctl &= ~BMCR_SPEED100; - else - dev_err(ds->dev, "unsupported speed: %d\n", phydev->speed); - - if (phydev->duplex == DUPLEX_FULL) - ctl |= BMCR_FULLDPLX; - else - ctl &= ~BMCR_FULLDPLX; - - lan9303_phy_write(ds, port, MII_BMCR, ctl); - - if (port == chip->phy_addr_base) { - /* Virtual Phy: Remove Turbo 200Mbit mode */ - lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &ctl); - - ctl &= ~LAN9303_VIRT_SPECIAL_TURBO; - regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, ctl); - } -} - static int lan9303_port_enable(struct dsa_switch *ds, int port, struct phy_device *phy) { @@ -1279,6 +1250,40 @@ static int lan9303_port_mdb_del(struct dsa_switch *ds, int port, return 0; } +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port, + struct phylink_config *config) +{ + struct lan9303 *chip = ds->priv; + + dev_dbg(chip->dev, "%s(%d) entered.", __func__, port); + + config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE | + MAC_SYM_PAUSE; + + if (dsa_port_is_cpu(dsa_to_port(ds, port))) { + /* cpu port */ + __set_bit(PHY_INTERFACE_MODE_RMII, + config->supported_interfaces); + __set_bit(PHY_INTERFACE_MODE_MII, + config->supported_interfaces); + } else { + /* internal ports */ + __set_bit(PHY_INTERFACE_MODE_INTERNAL, + config->supported_interfaces); + /* Compatibility for phylib's default interface type when the + * phy-mode property is absent + */ + __set_bit(PHY_INTERFACE_MODE_GMII, + config->supported_interfaces); + } + + /* This driver does not make use of the speed, duplex, pause or the + * advertisement in its mac_config, so it is safe to mark this driver + * as non-legacy. + */ + config->legacy_pre_march2020 = false; +} + /* For non-cpu ports, the max frame size is 1518. * The CPU port supports a max frame size of 1522. * There is a JUMBO flag to make the max size 2048, but this driver @@ -1304,7 +1309,7 @@ static const struct dsa_switch_ops lan9303_switch_ops = { .get_strings = lan9303_get_strings, .phy_read = lan9303_phy_read, .phy_write = lan9303_phy_write, - .adjust_link = lan9303_adjust_link, + .phylink_get_caps = lan9303_phylink_get_caps, .get_ethtool_stats = lan9303_get_ethtool_stats, .get_sset_count = lan9303_get_sset_count, .port_enable = lan9303_port_enable, -- 2.17.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK 2022-12-06 18:35 ` [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK Jerry Ray @ 2022-12-06 19:32 ` Vladimir Oltean 2022-12-06 21:07 ` Russell King (Oracle) 0 siblings, 1 reply; 13+ messages in thread From: Vladimir Oltean @ 2022-12-06 19:32 UTC (permalink / raw) To: Jerry Ray Cc: Andrew Lunn, Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, linux On Tue, Dec 06, 2022 at 12:35:00PM -0600, Jerry Ray wrote: > This patch replaces the .adjust_link api with the .phylink_get_caps api. Am I supposed to read this commit description and understand what the change does? You can't "replace" adjust_link with phylink_get_caps, since they don't do the same thing. The equivalent set of operations are roughly phylink_mac_config and phylink_mac_link_up, probably just the latter in your case. By deleting adjust_link and not replacing with any of the above, the change is telling me that nothing from adjust_link was needed? > > Signed-off-by: Jerry Ray <jerry.ray@microchip.com> > --- > v2-> v3: > Added back in disabling Turbo Mode on the CPU MII interface. > Removed the unnecessary clearing of the phy supported interfaces. > --- > drivers/net/dsa/lan9303-core.c | 79 ++++++++++++++++++---------------- > 1 file changed, 42 insertions(+), 37 deletions(-) > > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c > index baa336bb9d15..c6236b328ed8 100644 > --- a/drivers/net/dsa/lan9303-core.c > +++ b/drivers/net/dsa/lan9303-core.c > @@ -886,6 +886,13 @@ static int lan9303_check_device(struct lan9303 *chip) > return ret; > } > > + /* Virtual Phy: Always disable Turbo 200Mbit mode */ > + ret = lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, ®); > + if (ret) > + return ret; > + reg &= ~LAN9303_VIRT_SPECIAL_TURBO; > + regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, reg); Separate patch which moves this, please. > + > return 0; > } > > @@ -1047,42 +1054,6 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum, > return chip->ops->phy_write(chip, phy, regnum, val); > } > > -static void lan9303_adjust_link(struct dsa_switch *ds, int port, > - struct phy_device *phydev) > -{ > - struct lan9303 *chip = ds->priv; > - int ctl; > - > - if (!phy_is_pseudo_fixed_link(phydev)) > - return; > - > - ctl = lan9303_phy_read(ds, port, MII_BMCR); > - > - ctl &= ~BMCR_ANENABLE; > - > - if (phydev->speed == SPEED_100) > - ctl |= BMCR_SPEED100; > - else if (phydev->speed == SPEED_10) > - ctl &= ~BMCR_SPEED100; > - else > - dev_err(ds->dev, "unsupported speed: %d\n", phydev->speed); > - > - if (phydev->duplex == DUPLEX_FULL) > - ctl |= BMCR_FULLDPLX; > - else > - ctl &= ~BMCR_FULLDPLX; > - > - lan9303_phy_write(ds, port, MII_BMCR, ctl); Are you going to explain why modifying this register is no longer needed? > - > - if (port == chip->phy_addr_base) { > - /* Virtual Phy: Remove Turbo 200Mbit mode */ > - lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &ctl); > - > - ctl &= ~LAN9303_VIRT_SPECIAL_TURBO; > - regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, ctl); > - } > -} > - > static int lan9303_port_enable(struct dsa_switch *ds, int port, > struct phy_device *phy) > { > @@ -1279,6 +1250,40 @@ static int lan9303_port_mdb_del(struct dsa_switch *ds, int port, > return 0; > } > > +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port, > + struct phylink_config *config) > +{ > + struct lan9303 *chip = ds->priv; > + > + dev_dbg(chip->dev, "%s(%d) entered.", __func__, port); > + > + config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE | > + MAC_SYM_PAUSE; > + > + if (dsa_port_is_cpu(dsa_to_port(ds, port))) { > + /* cpu port */ This comment and the "internal ports" are absolutely redundant, they bring nothing to the understanding of the code. > + __set_bit(PHY_INTERFACE_MODE_RMII, > + config->supported_interfaces); > + __set_bit(PHY_INTERFACE_MODE_MII, > + config->supported_interfaces); > + } else { > + /* internal ports */ > + __set_bit(PHY_INTERFACE_MODE_INTERNAL, > + config->supported_interfaces); > + /* Compatibility for phylib's default interface type when the > + * phy-mode property is absent > + */ > + __set_bit(PHY_INTERFACE_MODE_GMII, > + config->supported_interfaces); > + } > + > + /* This driver does not make use of the speed, duplex, pause or the > + * advertisement in its mac_config, so it is safe to mark this driver > + * as non-legacy. > + */ > + config->legacy_pre_march2020 = false; > +} > + > /* For non-cpu ports, the max frame size is 1518. > * The CPU port supports a max frame size of 1522. > * There is a JUMBO flag to make the max size 2048, but this driver > @@ -1304,7 +1309,7 @@ static const struct dsa_switch_ops lan9303_switch_ops = { > .get_strings = lan9303_get_strings, > .phy_read = lan9303_phy_read, > .phy_write = lan9303_phy_write, > - .adjust_link = lan9303_adjust_link, > + .phylink_get_caps = lan9303_phylink_get_caps, Some of the lan9303_switch_ops are not aligned, and some are aligned with spaces. None with tabs. Please be consistent with something that exists, or create a preparatory patch which brings some more consistence with the way in which you want things to be. > .get_ethtool_stats = lan9303_get_ethtool_stats, > .get_sset_count = lan9303_get_sset_count, > .port_enable = lan9303_port_enable, > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK 2022-12-06 19:32 ` Vladimir Oltean @ 2022-12-06 21:07 ` Russell King (Oracle) 2022-12-06 22:12 ` Vladimir Oltean 2022-12-06 22:58 ` Jerry.Ray 0 siblings, 2 replies; 13+ messages in thread From: Russell King (Oracle) @ 2022-12-06 21:07 UTC (permalink / raw) To: Vladimir Oltean Cc: Jerry Ray, Andrew Lunn, Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel On Tue, Dec 06, 2022 at 09:32:24PM +0200, Vladimir Oltean wrote: > On Tue, Dec 06, 2022 at 12:35:00PM -0600, Jerry Ray wrote: > > This patch replaces the .adjust_link api with the .phylink_get_caps api. > > Am I supposed to read this commit description and understand what the > change does? > > You can't "replace" adjust_link with phylink_get_caps, since they don't > do the same thing. The equivalent set of operations are roughly > phylink_mac_config and phylink_mac_link_up, probably just the latter in > your case. > > By deleting adjust_link and not replacing with any of the above, the > change is telling me that nothing from adjust_link was needed? ... > > -static void lan9303_adjust_link(struct dsa_switch *ds, int port, > > - struct phy_device *phydev) > > -{ > > - struct lan9303 *chip = ds->priv; > > - int ctl; > > - > > - if (!phy_is_pseudo_fixed_link(phydev)) > > - return; If this is a not a fixed link, adjust_link does nothing. > > - > > - ctl = lan9303_phy_read(ds, port, MII_BMCR); > > - > > - ctl &= ~BMCR_ANENABLE; > > - > > - if (phydev->speed == SPEED_100) > > - ctl |= BMCR_SPEED100; > > - else if (phydev->speed == SPEED_10) > > - ctl &= ~BMCR_SPEED100; > > - else > > - dev_err(ds->dev, "unsupported speed: %d\n", phydev->speed); > > - > > - if (phydev->duplex == DUPLEX_FULL) > > - ctl |= BMCR_FULLDPLX; > > - else > > - ctl &= ~BMCR_FULLDPLX; > > - > > - lan9303_phy_write(ds, port, MII_BMCR, ctl); > > Are you going to explain why modifying this register is no longer needed? ... otherwise it is a fixed link, so the PHY is configured for the fixed link setting - which I think would end up writing to the an emulation of the PHY, and would end up writing the same settings back to the PHY as the PHY was already configured. So, I don't think adjust_link does anything useful, and I think this is an entirely appropriate change. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK 2022-12-06 21:07 ` Russell King (Oracle) @ 2022-12-06 22:12 ` Vladimir Oltean 2022-12-06 22:58 ` Jerry.Ray 1 sibling, 0 replies; 13+ messages in thread From: Vladimir Oltean @ 2022-12-06 22:12 UTC (permalink / raw) To: Russell King (Oracle) Cc: Jerry Ray, Andrew Lunn, Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel On Tue, Dec 06, 2022 at 09:07:54PM +0000, Russell King (Oracle) wrote: > > Are you going to explain why modifying this register is no longer needed? > > ... otherwise it is a fixed link, so the PHY is configured for the fixed > link setting - which I think would end up writing to the an emulation of > the PHY, and would end up writing the same settings back to the PHY as > the PHY was already configured. To be clear, when you say "an emulation of the PHY", are you talking about the swphy behind the fixed-link, or about the LAN9303_VIRT_PHY_BASE registers, which correspond to the RevMII Virtual PHY of the switch CPU port? As far as I can understand the Microchip LAN9303 documentation, the DSA master can have a phy-handle to the switch node (which devicetree/bindings/net/dsa/lan9303.txt seems to confirm), and the switch can pretend it's a PHY when accessed by a switch-unaware (Generic) PHY driver at the usual PHY MDIO registers. Through the Virtual PHY feature and registers, it can also pretend it's the "other" PHY, and this MII_BMCR register of the Virtual PHY can ultimately autoneg with "itself" and control what the DSA master sees in terms of reported speed, duplex, and AN complete. Prior to this change, the driver, when given a DT blob with a fixed-link on the switch CPU port, would disable BMCR_ANENABLE in the Virtual PHY. After the change, it would leave things as they are (which is not necessarily the way things are out of reset). Which way is better? Does it matter? Is it a stupid question? No clue. > So, I don't think adjust_link does anything useful, and I think this is > an entirely appropriate change. That it may well be, but its presentation is entirely inappropriate. Andrew has told Jerry before that it's important to split, describe and justify his changes accordingly, so it's not like the things I'm complaining about are news to him. Things would go a lot smoother if Jerry explained his patches better. Reviewing patches which do stuff that isn't explained in the commit message reminds me of Forrest Gump. Life is like a box of chocolates, you never know what you're going to get. That's not how it's supposed to work, a box of chocolates should contain chocolates, at least here. ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK 2022-12-06 21:07 ` Russell King (Oracle) 2022-12-06 22:12 ` Vladimir Oltean @ 2022-12-06 22:58 ` Jerry.Ray 2022-12-07 14:01 ` Vladimir Oltean 1 sibling, 1 reply; 13+ messages in thread From: Jerry.Ray @ 2022-12-06 22:58 UTC (permalink / raw) To: linux, olteanv Cc: andrew, f.fainelli, davem, edumazet, kuba, pabeni, netdev, linux-kernel > > > This patch replaces the .adjust_link api with the .phylink_get_caps api. > > > > Am I supposed to read this commit description and understand what the > > change does? > > > > You can't "replace" adjust_link with phylink_get_caps, since they don't > > do the same thing. The equivalent set of operations are roughly > > phylink_mac_config and phylink_mac_link_up, probably just the latter in > > your case. > > > > By deleting adjust_link and not replacing with any of the above, the > > change is telling me that nothing from adjust_link was needed? > > ... > > > > -static void lan9303_adjust_link(struct dsa_switch *ds, int port, > > > - struct phy_device *phydev) > > > -{ > > > - struct lan9303 *chip = ds->priv; > > > - int ctl; > > > - > > > - if (!phy_is_pseudo_fixed_link(phydev)) > > > - return; > > If this is a not a fixed link, adjust_link does nothing. > > > > - > > > - ctl = lan9303_phy_read(ds, port, MII_BMCR); > > > - > > > - ctl &= ~BMCR_ANENABLE; > > > - > > > - if (phydev->speed == SPEED_100) > > > - ctl |= BMCR_SPEED100; > > > - else if (phydev->speed == SPEED_10) > > > - ctl &= ~BMCR_SPEED100; > > > - else > > > - dev_err(ds->dev, "unsupported speed: %d\n", phydev->speed); > > > - > > > - if (phydev->duplex == DUPLEX_FULL) > > > - ctl |= BMCR_FULLDPLX; > > > - else > > > - ctl &= ~BMCR_FULLDPLX; > > > - > > > - lan9303_phy_write(ds, port, MII_BMCR, ctl); > > > > Are you going to explain why modifying this register is no longer needed? > > ... otherwise it is a fixed link, so the PHY is configured for the fixed > link setting - which I think would end up writing to the an emulation of > the PHY, and would end up writing the same settings back to the PHY as > the PHY was already configured. > > So, I don't think adjust_link does anything useful, and I think this is > an entirely appropriate change. > > I, too, thought I would need phylink_mac_config and phylink_mac_link_up to completely migrate away from PHYLIB to PHYLINK. But it seems as though the phylink layer is now taking care of the speed / duplex / etc settings of the phy registers. There is no DSA driver housecleaning needed at these other phylink_ api hook functions. At least, that's my current level of understanding... ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK 2022-12-06 22:58 ` Jerry.Ray @ 2022-12-07 14:01 ` Vladimir Oltean 0 siblings, 0 replies; 13+ messages in thread From: Vladimir Oltean @ 2022-12-07 14:01 UTC (permalink / raw) To: Jerry.Ray Cc: linux, andrew, f.fainelli, davem, edumazet, kuba, pabeni, netdev, linux-kernel On Tue, Dec 06, 2022 at 10:58:25PM +0000, Jerry.Ray@microchip.com wrote: > I, too, thought I would need phylink_mac_config and phylink_mac_link_up to > completely migrate away from PHYLIB to PHYLINK. But it seems as though the > phylink layer is now taking care of the speed / duplex / etc settings of the > phy registers. There is no DSA driver housecleaning needed at these other > phylink_ api hook functions. At least, that's my current level of > understanding... Did you understand my request? To create a separate patch, not phylink conversion, which deletes the MII_BMCR writes to the Virtual PHY, which says that you don't know why they're there and you don't think that they're needed, but at least you thought about what you're doing? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next v3 0/2] dsa: lan9303: Move to PHYLINK 2022-12-06 18:34 [PATCH net-next v3 0/2] dsa: lan9303: Move to PHYLINK Jerry Ray 2022-12-06 18:34 ` [PATCH net-next v3 1/2] dsa: lan9303: Add port_max_mtu API Jerry Ray 2022-12-06 18:35 ` [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK Jerry Ray @ 2022-12-06 18:57 ` Vladimir Oltean 2022-12-06 23:58 ` Jerry.Ray 2 siblings, 1 reply; 13+ messages in thread From: Vladimir Oltean @ 2022-12-06 18:57 UTC (permalink / raw) To: Jerry Ray Cc: Andrew Lunn, Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, linux On Tue, Dec 06, 2022 at 12:34:58PM -0600, Jerry Ray wrote: > This patch series moves the lan9303 driver to use the phylink > api away from phylib. > > 1) adds port_max_mtu api support. > 2) Replace .adjust_link with .phylink_get_caps dsa api What does the max MTU have to do with phylink? What is it that makes these two patches related? > > Clearing the Turbo Mode bit previously done in the adjust_link > API is moved to the driver initialization immediately following > the successful detection of a LAN93xx device. It is forced to a > disabled state and never enabled. > > At this point, I do not see anything this driver needs from the other > phylink APIs. > > Signed-off-by: Jerry Ray <jerry.ray@microchip.com> You don't need to put your sign off on the cover letter. ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH net-next v3 0/2] dsa: lan9303: Move to PHYLINK 2022-12-06 18:57 ` [PATCH net-next v3 0/2] " Vladimir Oltean @ 2022-12-06 23:58 ` Jerry.Ray 0 siblings, 0 replies; 13+ messages in thread From: Jerry.Ray @ 2022-12-06 23:58 UTC (permalink / raw) To: olteanv Cc: andrew, f.fainelli, davem, edumazet, kuba, pabeni, netdev, linux-kernel, linux > > This patch series moves the lan9303 driver to use the phylink > > api away from phylib. > > > > 1) adds port_max_mtu api support. > > 2) Replace .adjust_link with .phylink_get_caps dsa api > > What does the max MTU have to do with phylink? What is it that makes > these two patches related? > I'm touching the same file, so I created this series of patches to avoid piecewise patching conflicts that might have resulted if they were independent patches. > > > > Clearing the Turbo Mode bit previously done in the adjust_link > > API is moved to the driver initialization immediately following > > the successful detection of a LAN93xx device. It is forced to a > > disabled state and never enabled. > > > > At this point, I do not see anything this driver needs from the other > > phylink APIs. > > > > Signed-off-by: Jerry Ray <jerry.ray@microchip.com> > > You don't need to put your sign off on the cover letter. > Understood. Thx, J. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-12-07 14:03 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-06 18:34 [PATCH net-next v3 0/2] dsa: lan9303: Move to PHYLINK Jerry Ray 2022-12-06 18:34 ` [PATCH net-next v3 1/2] dsa: lan9303: Add port_max_mtu API Jerry Ray 2022-12-06 18:56 ` Vladimir Oltean 2022-12-06 23:44 ` Jerry.Ray 2022-12-07 11:39 ` Vladimir Oltean 2022-12-06 18:35 ` [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK Jerry Ray 2022-12-06 19:32 ` Vladimir Oltean 2022-12-06 21:07 ` Russell King (Oracle) 2022-12-06 22:12 ` Vladimir Oltean 2022-12-06 22:58 ` Jerry.Ray 2022-12-07 14:01 ` Vladimir Oltean 2022-12-06 18:57 ` [PATCH net-next v3 0/2] " Vladimir Oltean 2022-12-06 23:58 ` Jerry.Ray
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).