| Message ID | 20230407134626.47928-3-arinc.unal@arinc9.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v2,net-next,01/14] net: dsa: mt7530: fix comments regarding port 5 and 6 for both switches | expand |
On Fri, Apr 07, 2023 at 04:46:14PM +0300, arinc9.unal@gmail.com wrote: > From: Arınç ÜNAL <arinc.unal@arinc9.com> > > There're two code paths for setting up port 5: > > mt7530_setup() > -> mt7530_setup_port5() > > mt753x_phylink_mac_config() > -> mt753x_mac_config() > -> mt7530_mac_config() > -> mt7530_setup_port5() > > The first code path is supposed to run when PHY muxing is being used. In > this case, port 5 is somewhat of a hidden port. It won't be defined on the > devicetree so phylink can't be used to manage the port. > > The second code path used to call mt7530_setup_port5() directly under case > 5 on mt7530_phylink_mac_config() before it was moved to mt7530_mac_config() > with 88bdef8be9f6 ("net: dsa: mt7530: Extend device data ready for adding a > new hardware"). mt7530_setup_port5() will never run through this code path > because the current code on mt7530_setup() bypasses phylink for all cases > of port 5. > > Fix this by leaving it to phylink if port 5 is used as a CPU, DSA, or user > port. For the cases of PHY muxing or the port being disabled, call > mt7530_setup_port5() directly from mt7530_setup() without involving > phylink. > > Move setting the interface and P5_DISABLED mode to a more specific > location. They're supposed to be overwritten if PHY muxing is detected. > > Add comments which explain the process. > > Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com> > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> > --- We have a natural language processing engine (AUTOSEL) which automatically picks up as candidates for "stable" those patches which weren't explicitly submitted through the proper process for that (and they contain words like "fix", "bug", "crash", "leak" etc). Your chosen wording, both in the commit title and message, would most likely trigger that bot, and then you'd have to explain why you chose this language and not something else more descriptive of your change. It would be nice if you could rewrite the commit messages for your entire series to be a bit more succint as to what is the purpose of the change you are making, and don't use the word "fix" when there is no problem to be observed.
On 11.04.2023 18:00, Vladimir Oltean wrote: > On Fri, Apr 07, 2023 at 04:46:14PM +0300, arinc9.unal@gmail.com wrote: >> From: Arınç ÜNAL <arinc.unal@arinc9.com> >> >> There're two code paths for setting up port 5: >> >> mt7530_setup() >> -> mt7530_setup_port5() >> >> mt753x_phylink_mac_config() >> -> mt753x_mac_config() >> -> mt7530_mac_config() >> -> mt7530_setup_port5() >> >> The first code path is supposed to run when PHY muxing is being used. In >> this case, port 5 is somewhat of a hidden port. It won't be defined on the >> devicetree so phylink can't be used to manage the port. >> >> The second code path used to call mt7530_setup_port5() directly under case >> 5 on mt7530_phylink_mac_config() before it was moved to mt7530_mac_config() >> with 88bdef8be9f6 ("net: dsa: mt7530: Extend device data ready for adding a >> new hardware"). mt7530_setup_port5() will never run through this code path >> because the current code on mt7530_setup() bypasses phylink for all cases >> of port 5. >> >> Fix this by leaving it to phylink if port 5 is used as a CPU, DSA, or user >> port. For the cases of PHY muxing or the port being disabled, call >> mt7530_setup_port5() directly from mt7530_setup() without involving >> phylink. >> >> Move setting the interface and P5_DISABLED mode to a more specific >> location. They're supposed to be overwritten if PHY muxing is detected. >> >> Add comments which explain the process. >> >> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com> >> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> >> --- > > We have a natural language processing engine (AUTOSEL) which > automatically picks up as candidates for "stable" those patches which > weren't explicitly submitted through the proper process for that (and > they contain words like "fix", "bug", "crash", "leak" etc). > > Your chosen wording, both in the commit title and message, would most > likely trigger that bot, and then you'd have to explain why you chose > this language and not something else more descriptive of your change. > It would be nice if you could rewrite the commit messages for your > entire series to be a bit more succint as to what is the purpose of the > change you are making, and don't use the word "fix" when there is no > problem to be observed. Will do, thanks. Arınç
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 31ef70f0cd12..a00aabe4987e 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -2288,16 +2288,19 @@ mt7530_setup(struct dsa_switch *ds) return ret; /* Setup port 5 */ - priv->p5_intf_sel = P5_DISABLED; - interface = PHY_INTERFACE_MODE_NA; - if (!dsa_is_unused_port(ds, 5)) { + /* Set the interface selection of port 5 to GMAC5 when it's used + * as a CPU, DSA, or user port. Let phylink handle the rest. + */ priv->p5_intf_sel = P5_INTF_SEL_GMAC5; - ret = of_get_phy_mode(dsa_to_port(ds, 5)->dn, &interface); - if (ret && ret != -ENODEV) - return ret; } else { - /* Scan the ethernet nodes. look for GMAC1, lookup used phy */ + /* Scan the ethernet nodes. Look for GMAC1, lookup the used PHY. + * Set priv->p5_intf_sel to P5_DISABLED first, then overwrite it + * if PHY muxing is detected. + */ + priv->p5_intf_sel = P5_DISABLED; + interface = PHY_INTERFACE_MODE_NA; + for_each_child_of_node(dn, mac_np) { if (!of_device_is_compatible(mac_np, "mediatek,eth-mac")) @@ -2328,6 +2331,8 @@ mt7530_setup(struct dsa_switch *ds) of_node_put(phy_node); break; } + + mt7530_setup_port5(ds, interface); } #ifdef CONFIG_GPIOLIB @@ -2338,8 +2343,6 @@ mt7530_setup(struct dsa_switch *ds) } #endif /* CONFIG_GPIOLIB */ - mt7530_setup_port5(ds, interface); - /* Flush the FDB table */ ret = mt7530_fdb_cmd(priv, MT7530_FDB_FLUSH, NULL); if (ret < 0)