| Message ID | 20221209224713.19980-6-jerry.ray@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | dsa: lan9303: Move to PHYLINK | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/build_clang | success | Errors and warnings before: 2 this patch: 2 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 12 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, Dec 09, 2022 at 04:47:12PM -0600, Jerry Ray wrote: > In preparing to move the adjust_link logic into the phylink_mac_link_up > api, change the macro used to check for the cpu port. No justification given for why this change is made. As a counter argument, I could point out that DSA can support configurations where the CPU port is one of the 100BASE-TX PHY ports, and the MII port can be used as a regular user port where a PHY has been connected. Some people are already doing this, for example connecting a Beaglebone Black (can also be Raspberry Pi or what have you) over SPI to a VSC7512 switch evaluation board. The LAN9303 documentation makes it rather clear to me that such a configuration would be possible, because the Switch Engine Ingress Port Type Register allows any of the 3 switch ports to expect DSA tags. It's lan9303_setup_tagging() the one who hardcodes the driver configuration to be that where port 0 is the only acceptable CPU port (as well as the early check from lan9303_setup()). DSA's understanding of a CPU port is only that - a port which carries DSA-tagged traffic, and is not exposed as a net_device to user space. Nothing about MII vs internal PHY ports - that is a separate classification, and a dsa_is_cpu_port() test is simply not something relevant from phylink's perspective, to put it simply. What we see in other device drivers for phylink handling is that there is driver-level knowledge as to which ports have internal PHYs and which have xMII pins. See priv->info->supports_mii[] in sja1105, dev->info->supports_mii[] in the ksz drivers, mv88e6xxx_phy_is_internal() in mv88e6xxx, felix->info->port_modes[] in the ocelot drivers, etc etc. Hardcoding port number 0 is also better from my perspective than looking for the CPU port. That's because the switch IP was built with the idea in mind that port 0 is MII. I would very much appreciate if this driver did not make any assumptions that the internal PHY ports cannot carry DSA-tagged traffic. This assumption was true when the driver was introduced, but it changed with commit 0e27921816ad ("net: dsa: Use PHYLINK for the CPU/DSA ports"). Coincidentally, that is also the commit which started prompting the lan9303 driver for an update, via the dmesg warnings you are seeing. It looks like there is potentially more code to unlock than this simple API change, which is something you could look at. > > Signed-off-by: Jerry Ray <jerry.ray@microchip.com> > --- > drivers/net/dsa/lan9303-core.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c > index 694249aa1f19..1d22e4b74308 100644 > --- a/drivers/net/dsa/lan9303-core.c > +++ b/drivers/net/dsa/lan9303-core.c > @@ -1064,7 +1064,11 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port, > struct lan9303 *chip = ds->priv; > int ctl; > > - if (!phy_is_pseudo_fixed_link(phydev)) > + /* On this device, we are only interested in doing something here if > + * this is the CPU port. All other ports are 10/100 phys using MDIO > + * to control there link settings. > + */ > + if (!dsa_is_cpu_port(ds, port)) > return; > > ctl = lan9303_phy_read(ds, port, MII_BMCR); > -- > 2.17.1 >
> > In preparing to move the adjust_link logic into the phylink_mac_link_up > > api, change the macro used to check for the cpu port. > > No justification given for why this change is made. > The phydev parameter being passed into phylink_mac_link_up for the CPU port was NULL, so I have to move to something else. > As a counter argument, I could point out that DSA can support configurations > where the CPU port is one of the 100BASE-TX PHY ports, and the MII port > can be used as a regular user port where a PHY has been connected. Some > people are already doing this, for example connecting a Beaglebone Black > (can also be Raspberry Pi or what have you) over SPI to a VSC7512 switch > evaluation board. > > The LAN9303 documentation makes it rather clear to me that such a > configuration would be possible, because the Switch Engine Ingress Port > Type Register allows any of the 3 switch ports to expect DSA tags. It's > lan9303_setup_tagging() the one who hardcodes the driver configuration > to be that where port 0 is the only acceptable CPU port (as well as the > early check from lan9303_setup()). > > DSA's understanding of a CPU port is only that - a port which carries > DSA-tagged traffic, and is not exposed as a net_device to user space. > Nothing about MII vs internal PHY ports - that is a separate classification, > and a dsa_is_cpu_port() test is simply not something relevant from > phylink's perspective, to put it simply. > > What we see in other device drivers for phylink handling is that there > is driver-level knowledge as to which ports have internal PHYs and which > have xMII pins. See priv->info->supports_mii[] in sja1105, dev->info->supports_mii[] > in the ksz drivers, mv88e6xxx_phy_is_internal() in mv88e6xxx, > felix->info->port_modes[] in the ocelot drivers, etc etc. Hardcoding > port number 0 is also better from my perspective than looking for the > CPU port. That's because the switch IP was built with the idea in mind > that port 0 is MII. > > I would very much appreciate if this driver did not make any assumptions > that the internal PHY ports cannot carry DSA-tagged traffic. This > assumption was true when the driver was introduced, but it changed with > commit 0e27921816ad ("net: dsa: Use PHYLINK for the CPU/DSA ports"). > Coincidentally, that is also the commit which started prompting the > lan9303 driver for an update, via the dmesg warnings you are seeing. > > It looks like there is potentially more code to unlock than this simple > API change, which is something you could look at. > I understand your point. The LAN9303 is very flexible, supporting an I2C method for managing the switch and allowing the xMII to operate as either MAC or PHY. The original driver was written targeting the primary configuration most widely used by our customers. The host CPU has an xMII interface and the MDIO bus is used for control. Adding in the flexibility to support other configurations is something I will investigate as I expand the driver to support LAN9353/LAN9354/LAN9355 devices. Adding the private->info->supports_mii[] as was done in the ksz drivers is the most likely approach. I see this as a separate patch series. Would you agree? I will hardcode for port 0 at this point rather than looking at CPU port. Regards, Jerry. > > > > Signed-off-by: Jerry Ray <jerry.ray@microchip.com> > > --- > > drivers/net/dsa/lan9303-core.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c > > index 694249aa1f19..1d22e4b74308 100644 > > --- a/drivers/net/dsa/lan9303-core.c > > +++ b/drivers/net/dsa/lan9303-core.c > > @@ -1064,7 +1064,11 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port, > > struct lan9303 *chip = ds->priv; > > int ctl; > > > > - if (!phy_is_pseudo_fixed_link(phydev)) > > + /* On this device, we are only interested in doing something here if > > + * this is the CPU port. All other ports are 10/100 phys using MDIO > > + * to control there link settings. > > + */ > > + if (!dsa_is_cpu_port(ds, port)) > > return; > > > > ctl = lan9303_phy_read(ds, port, MII_BMCR); > > -- > > 2.17.1 > > >
On Mon, Dec 12, 2022 at 05:42:01PM +0000, Jerry.Ray@microchip.com wrote: > > It looks like there is potentially more code to unlock than this simple > > API change, which is something you could look at. > > I understand your point. The LAN9303 is very flexible, supporting an I2C > method for managing the switch and allowing the xMII to operate as either > MAC or PHY. > > The original driver was written targeting the primary configuration most > widely used by our customers. The host CPU has an xMII interface and the > MDIO bus is used for control. Adding in the flexibility to support other > configurations is something I will investigate as I expand the driver to > support LAN9353/LAN9354/LAN9355 devices. Adding the > private->info->supports_mii[] as was done in the ksz drivers is the most > likely approach. I see this as a separate patch series. Would you agree? > > I will hardcode for port 0 at this point rather than looking at CPU port. Yes, I think that would be ok. Thanks for at least not baking in any more assumptions in the driver that already exist.
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c index 694249aa1f19..1d22e4b74308 100644 --- a/drivers/net/dsa/lan9303-core.c +++ b/drivers/net/dsa/lan9303-core.c @@ -1064,7 +1064,11 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port, struct lan9303 *chip = ds->priv; int ctl; - if (!phy_is_pseudo_fixed_link(phydev)) + /* On this device, we are only interested in doing something here if + * this is the CPU port. All other ports are 10/100 phys using MDIO + * to control there link settings. + */ + if (!dsa_is_cpu_port(ds, port)) return; ctl = lan9303_phy_read(ds, port, MII_BMCR);
In preparing to move the adjust_link logic into the phylink_mac_link_up api, change the macro used to check for the cpu port. Signed-off-by: Jerry Ray <jerry.ray@microchip.com> --- drivers/net/dsa/lan9303-core.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)