| Message ID | 20221226071425.3895915-2-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: renesas: rswitch: Modify initialization for SERDES and PHY | 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: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| 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: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 7 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, Dec 26, 2022 at 04:14:23PM +0900, Yoshihiro Shimoda wrote: > Set phy_dev->host_interfaces by pl->link_interface in > phylink_fwnode_phy_connect() for a non-sfp PHY. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/net/phy/phylink.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index 09cc65c0da93..1958d6cc9ef9 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -1809,6 +1809,7 @@ int phylink_fwnode_phy_connect(struct phylink *pl, > pl->link_interface = phy_dev->interface; > pl->link_config.interface = pl->link_interface; > } > + __set_bit(pl->link_interface, phy_dev->host_interfaces); This is probably going to break Macchiatobin platforms, since we declare that the link mode there is 10GBASE-R, we'll end up with host_interfaces containing just this mode. This will cause the 88x3310 driver to select a rate matching interface mode, which the mvpp2 MAC can't support. If we want to fill host_interfaces in, then it needs to be filled in properly - and by that I mean with all the host interface modes that can be electrically supported - otherwise platforms will break. So, sorry, but NAK on this change.
Hello Russell, > From: Russell King, Sent: Tuesday, January 3, 2023 6:54 PM > > On Mon, Dec 26, 2022 at 04:14:23PM +0900, Yoshihiro Shimoda wrote: > > Set phy_dev->host_interfaces by pl->link_interface in > > phylink_fwnode_phy_connect() for a non-sfp PHY. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > drivers/net/phy/phylink.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > > index 09cc65c0da93..1958d6cc9ef9 100644 > > --- a/drivers/net/phy/phylink.c > > +++ b/drivers/net/phy/phylink.c > > @@ -1809,6 +1809,7 @@ int phylink_fwnode_phy_connect(struct phylink *pl, > > pl->link_interface = phy_dev->interface; > > pl->link_config.interface = pl->link_interface; > > } > > + __set_bit(pl->link_interface, phy_dev->host_interfaces); > > This is probably going to break Macchiatobin platforms, since we > declare that the link mode there is 10GBASE-R, we'll end up with > host_interfaces containing just this mode. This will cause the > 88x3310 driver to select a rate matching interface mode, which the > mvpp2 MAC can't support. > > If we want to fill host_interfaces in, then it needs to be filled in > properly - and by that I mean with all the host interface modes that > can be electrically supported - otherwise platforms will break. > > So, sorry, but NAK on this change. Thank you for your review! I understood why NAK on this change: --- static int mv3310_select_mactype(unsigned long *interfaces) { ... else if (test_bit(PHY_INTERFACE_MODE_SGMII, interfaces) && test_bit(PHY_INTERFACE_MODE_10GBASER, interfaces)) return MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER; ... else if (test_bit(PHY_INTERFACE_MODE_10GBASER, interfaces)) return MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_RATE_MATCH; ... --- - On this change, since the interfaces is set to PHY_INTERFACE_MODE_10GBASER only, this function will return MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_RATE_MATCH. - Without this change, since the host_interfaces value is zero, the mv3310_select_mactype() will not called. I'll investigate phylink and marvell10 codes again. Best regards, Yoshihiro Shimoda
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 09cc65c0da93..1958d6cc9ef9 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -1809,6 +1809,7 @@ int phylink_fwnode_phy_connect(struct phylink *pl, pl->link_interface = phy_dev->interface; pl->link_config.interface = pl->link_interface; } + __set_bit(pl->link_interface, phy_dev->host_interfaces); ret = phy_attach_direct(pl->netdev, phy_dev, flags, pl->link_interface);
Set phy_dev->host_interfaces by pl->link_interface in phylink_fwnode_phy_connect() for a non-sfp PHY. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/net/phy/phylink.c | 1 + 1 file changed, 1 insertion(+)