Messages in this thread | | | From | Alvin Šipraga <> | Subject | Re: [PATCH net] net: dsa: realtek: rtl8365mb: fix GMII caps for ports with internal PHY | Date | Mon, 6 Jun 2022 13:47:08 +0000 |
| |
On Mon, Jun 06, 2022 at 02:31:16PM +0100, Russell King (Oracle) wrote: > On Mon, Jun 06, 2022 at 03:01:30PM +0200, Alvin Šipraga wrote: > > From: Alvin Šipraga <alsi@bang-olufsen.dk> > > > > phylib defaults to GMII when no phy-mode or phy-connection-type property > > is specified in a DSA port node. > > > > Commit a5dba0f207e5 ("net: dsa: rtl8365mb: add GMII as user port mode") > > introduced implicit support for GMII mode on ports with internal PHY to > > allow a PHY connection for device trees where the phy-mode is not > > explicitly set to "internal". > > > > Commit 6ff6064605e9 ("net: dsa: realtek: convert to phylink_generic_validate()") > > then broke this behaviour by discarding the usage of > > rtl8365mb_phy_mode_supported() - where this GMII support was indicated - > > while switching to the new .phylink_get_caps API. > > > > With the new API, rtl8365mb_phy_mode_supported() is no longer needed. > > Remove it altogether and add back the GMII capability - this time to > > rtl8365mb_phylink_get_caps() - so that the above default behaviour works > > for ports with internal PHY again. > > Oops - I guess this has been caused by the delay between my patch being > initially prepared, it sitting around in my tree for many months while > other patches get merged, and it eventually seeing the light of day. > > Sorry about that.
No problem, it's actually my bad, but I am still getting to grips with the responsibility of maintaining a Linux driver :)
> > > Fixes: 6ff6064605e9 ("net: dsa: realtek: convert to phylink_generic_validate()") > > Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk> > > --- > > > > Luiz, Russel: > > > > Commit a5dba0f207e5 ought to have had a Fixes: tag I think, because it > > claims to have been fixing a regression in the net-next tree - is that > > right? I seem to have missed both referenced commits when they were > > posted and never hit this issue personally. I only found things now > > during some other refactoring and the test for GMII looked weird to me > > so I went and investigated. > > > > Could you please help me identify that Fixes: tag? Just for my own > > understanding of what caused this added requirement for GMII on ports > > with internal PHY. > > I have absolutely no idea. I don't think any "requirement" has ever been > added - phylib has always defaulted to GMII, so as the driver stood when > it was first submitted on Oct 18 2021, I don't see how it could have > worked, unless the DT it was being tested with specified a phy-mode of > "internal". As you were the one who submitted it, you would have a > better idea. > > The only suggestion I have is to bisect to find out exactly what caused > the GMII vs INTERNAL issue to crop up.
Alright, thanks for the quick response. Maybe Luiz has a better idea, otherwise I will try bisecting if I find the time.
For what it's worth, I believe the patch is correct and applicable in its current form - it just lacks an explanation as to why this ever worked in the first place.
> > > > > --- > > drivers/net/dsa/realtek/rtl8365mb.c | 38 +++++++---------------------- > > 1 file changed, 9 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > > index 3bb42a9f236d..769f672e9128 100644 > > --- a/drivers/net/dsa/realtek/rtl8365mb.c > > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > > @@ -955,35 +955,21 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port, > > return 0; > > } > > > > -static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port, > > - phy_interface_t interface) > > -{ > > - int ext_int; > > - > > - ext_int = rtl8365mb_extint_port_map[port]; > > - > > - if (ext_int < 0 && > > - (interface == PHY_INTERFACE_MODE_NA || > > - interface == PHY_INTERFACE_MODE_INTERNAL || > > - interface == PHY_INTERFACE_MODE_GMII)) > > - /* Internal PHY */ > > - return true; > > - else if ((ext_int >= 1) && > > - phy_interface_mode_is_rgmii(interface)) > > - /* Extension MAC */ > > - return true; > > - > > - return false; > > -} > > - > > static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port, > > struct phylink_config *config) > > { > > - if (dsa_is_user_port(ds, port)) > > + if (dsa_is_user_port(ds, port)) { > > Given the updates to rtl8365mb_phy_mode_supported(), this misses out on > the check of rtl8365mb_extint_port_map[port] introduced in commit > 6147631c079f ("net: dsa: realtek: rtl8365mb: allow non-cpu extint > ports").
You are right, but since this is targetting net and not net-next, I figured I would not introduce any additional changes. I have a separate series lined up for net-next (just submitted) which fixes this in a much cleaner way.
My rationale here is: yes, this test is incorrect and is a shortcoming of the driver, but it does not represent a real bug worthy of inclusion in the stable trees.
> > > __set_bit(PHY_INTERFACE_MODE_INTERNAL, > > config->supported_interfaces); > > - else if (dsa_is_cpu_port(ds, port)) > > + > > + /* GMII is the default interface mode for phylib, so > > + * we have to support it for ports with integrated PHY. > > + */ > > + __set_bit(PHY_INTERFACE_MODE_GMII, > > + config->supported_interfaces); > > + } else if (dsa_is_cpu_port(ds, port)) { > > This test also needs to be updated. > > Not sure what rtl8365mb_extint_port_map[port] == 0 is supposed to > signify - maybe port unusable? Looks that way to me.
Check the other series I just sent for a cleaner implementation of this. | |