| Message ID | 20240202-for-netnext-mt7530-improvements-2-v3-4-63d5adae99ca@arinc9.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | MT7530 DSA Subdriver Improvements Act II | expand |
On Fri, Feb 02, 2024 at 12:19:10PM +0300, Arınç ÜNAL via B4 Relay wrote: > From: Arınç ÜNAL <arinc.unal@arinc9.com> > > The crystal frequency concerns the switch core. The frequency should be > checked when the switch is being set up so the driver can reject the > unsupported hardware earlier and without requiring port 6 to be used. > > Move it to mt7530_setup(). Drop the unnecessary function printing. > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Reviewed-by: Vladimir Oltean <olteanv@gmail.com> I would prefer this to be earlier in the series, before patch 2 which moves mt7530_setup_port6() to be called from mac_config(). mac_config() is supposed to be configuration error-free - in other words, all state should have been checked before hand. Thanks.
On 2.02.2024 14:48, Russell King (Oracle) wrote: > On Fri, Feb 02, 2024 at 12:19:10PM +0300, Arınç ÜNAL via B4 Relay wrote: >> From: Arınç ÜNAL <arinc.unal@arinc9.com> >> >> The crystal frequency concerns the switch core. The frequency should be >> checked when the switch is being set up so the driver can reject the >> unsupported hardware earlier and without requiring port 6 to be used. >> >> Move it to mt7530_setup(). Drop the unnecessary function printing. >> >> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> >> Reviewed-by: Andrew Lunn <andrew@lunn.ch> >> Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > > I would prefer this to be earlier in the series, before patch 2 which > moves mt7530_setup_port6() to be called from mac_config(). mac_config() > is supposed to be configuration error-free - in other words, all state > should have been checked before hand. I agree but mt7530_mac_config() is not a void function yet. The mac_port_config member of the mt753x_info structure points to this function. My next patch series gets rid of all useless error returns on the phylink path and change mac_port_config to void. So I don't think working on this patch series further will worth the effort. I'd rather have this version applied as is. Arınç
On Fri, Feb 02, 2024 at 09:16:02PM +0300, Arınç ÜNAL wrote: > On 2.02.2024 14:48, Russell King (Oracle) wrote: > > On Fri, Feb 02, 2024 at 12:19:10PM +0300, Arınç ÜNAL via B4 Relay wrote: > > > From: Arınç ÜNAL <arinc.unal@arinc9.com> > > > > > > The crystal frequency concerns the switch core. The frequency should be > > > checked when the switch is being set up so the driver can reject the > > > unsupported hardware earlier and without requiring port 6 to be used. > > > > > > Move it to mt7530_setup(). Drop the unnecessary function printing. > > > > > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > > > > I would prefer this to be earlier in the series, before patch 2 which > > moves mt7530_setup_port6() to be called from mac_config(). mac_config() > > is supposed to be configuration error-free - in other words, all state > > should have been checked before hand. > > I agree but mt7530_mac_config() is not a void function yet. The > mac_port_config member of the mt753x_info structure points to this > function. My next patch series gets rid of all useless error returns on the > phylink path and change mac_port_config to void. So I don't think working > on this patch series further will worth the effort. I'd rather have this > version applied as is. I agree regarding not changing the patch itself, but I also agree with Russell regarding the patch ordering. I know it's a 10-minute git headache to rebase the patches on top of each other in a different order, but you can easily compare the end result being identical to what you had before and hence don't need to retest.
On 2.02.2024 21:39, Daniel Golle wrote: > On Fri, Feb 02, 2024 at 09:16:02PM +0300, Arınç ÜNAL wrote: >> On 2.02.2024 14:48, Russell King (Oracle) wrote: >>> On Fri, Feb 02, 2024 at 12:19:10PM +0300, Arınç ÜNAL via B4 Relay wrote: >>>> From: Arınç ÜNAL <arinc.unal@arinc9.com> >>>> >>>> The crystal frequency concerns the switch core. The frequency should be >>>> checked when the switch is being set up so the driver can reject the >>>> unsupported hardware earlier and without requiring port 6 to be used. >>>> >>>> Move it to mt7530_setup(). Drop the unnecessary function printing. >>>> >>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> >>>> Reviewed-by: Andrew Lunn <andrew@lunn.ch> >>>> Reviewed-by: Vladimir Oltean <olteanv@gmail.com> >>> >>> I would prefer this to be earlier in the series, before patch 2 which >>> moves mt7530_setup_port6() to be called from mac_config(). mac_config() >>> is supposed to be configuration error-free - in other words, all state >>> should have been checked before hand. >> >> I agree but mt7530_mac_config() is not a void function yet. The >> mac_port_config member of the mt753x_info structure points to this >> function. My next patch series gets rid of all useless error returns on the >> phylink path and change mac_port_config to void. So I don't think working >> on this patch series further will worth the effort. I'd rather have this >> version applied as is. > > I agree regarding not changing the patch itself, but I also agree > with Russell regarding the patch ordering. I know it's a 10-minute > git headache to rebase the patches on top of each other in a different > order, but you can easily compare the end result being identical to > what you had before and hence don't need to retest. This is not about laziness. This is before patch 2: phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config() -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config() -> mt753x_mac_config() -> mt753x_info :: mac_port_config() -> mt7530_mac_config() -> mt7530_setup_port5() -> mt753x_pad_setup() -> mt753x_info :: pad_setup() -> mt7530_pad_clk_setup() This is after: phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config() -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config() -> mt753x_mac_config() -> mt753x_info :: mac_port_config() -> mt7530_mac_config() -> mt7530_setup_port5() -> mt7530_setup_port6() Patch 2 does not move mt7530_setup_port6() to be called from phylink_mac_ops :: mac_config(), it already is. There is no valid reason to reorder the patches. My response to Russell should've stated this instead of focusing on his second sentence. pw-bot: under-review Arınç
On Sun, Feb 04, 2024 at 04:55:40PM +0300, Arınç ÜNAL wrote: > On 2.02.2024 21:39, Daniel Golle wrote: > > On Fri, Feb 02, 2024 at 09:16:02PM +0300, Arınç ÜNAL wrote: > > > On 2.02.2024 14:48, Russell King (Oracle) wrote: > > > > On Fri, Feb 02, 2024 at 12:19:10PM +0300, Arınç ÜNAL via B4 Relay wrote: > > > > > From: Arınç ÜNAL <arinc.unal@arinc9.com> > > > > > > > > > > The crystal frequency concerns the switch core. The frequency should be > > > > > checked when the switch is being set up so the driver can reject the > > > > > unsupported hardware earlier and without requiring port 6 to be used. > > > > > > > > > > Move it to mt7530_setup(). Drop the unnecessary function printing. > > > > > > > > > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> > > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > > > Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > > > > > > > > I would prefer this to be earlier in the series, before patch 2 which > > > > moves mt7530_setup_port6() to be called from mac_config(). mac_config() > > > > is supposed to be configuration error-free - in other words, all state > > > > should have been checked before hand. > > > > > > I agree but mt7530_mac_config() is not a void function yet. The > > > mac_port_config member of the mt753x_info structure points to this > > > function. My next patch series gets rid of all useless error returns on the > > > phylink path and change mac_port_config to void. So I don't think working > > > on this patch series further will worth the effort. I'd rather have this > > > version applied as is. > > > > I agree regarding not changing the patch itself, but I also agree > > with Russell regarding the patch ordering. I know it's a 10-minute > > git headache to rebase the patches on top of each other in a different > > order, but you can easily compare the end result being identical to > > what you had before and hence don't need to retest. > > This is not about laziness. This is before patch 2: > > phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config() > -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config() > -> mt753x_mac_config() > -> mt753x_info :: mac_port_config() -> mt7530_mac_config() > -> mt7530_setup_port5() > -> mt753x_pad_setup() > -> mt753x_info :: pad_setup() -> mt7530_pad_clk_setup() > > This is after: > > phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config() > -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config() > -> mt753x_mac_config() > -> mt753x_info :: mac_port_config() -> mt7530_mac_config() > -> mt7530_setup_port5() > -> mt7530_setup_port6() > > Patch 2 does not move mt7530_setup_port6() to be called from > phylink_mac_ops :: mac_config(), it already is. There is no valid reason to > reorder the patches. > > My response to Russell should've stated this instead of focusing on his > second sentence. This patch moves the test for a 20MHz crystal to mt7530_setup(), which is something that is entirely orthogonal to patch 2, which can be done cleanly (I've just applied the patches in the original order and then reordered them: 98c481f5d706 net: dsa: mt7530: do not clear config->supported_interfaces 93c6b53b17f4 net: dsa: mt7530: correct port capabilities of MT7988 c9c6d4c51a1d net: dsa: mt7530: simplify mt7530_setup_port6() and change to void adfa948253e0 net: dsa: mt7530: remove pad_setup function pointer 57e21e6c2fc0 net: dsa: mt7530: call port 6 setup from mt7530_mac_config() 959a0f9323c8 net: dsa: mt7530: move XTAL check to mt7530_setup() 856ab64a22ef net: dsa: mt7530: empty default case on mt7530_setup_port5() No problems. The end result is identical comparing the git tree at the original "move XTAL" patch with adfa948253e0. Now, if we look at "net: dsa: mt7530: remove pad_setup function pointer" we can see that yes, the pad_setup() method was called from mac_confing, but this is the exact contents of that patch removing the callsite: - mt753x_pad_setup(ds, state); This returns an integer, which may be an error code, which is ignored. Therefore, if the XTAL frequency check fires, and mt753x_pad_setup() returns an error, it is ignored today. After "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()" the renamed pad_setup() method is now called from mac_config() thusly: + ret = mt7530_setup_port6(priv->ds, interface); + if (ret) + return ret; So now the error checks cause mt7530_mac_config() to return an error which in turn causes mt753x_mac_config() to fail, and therefore mt753x_phylink_mac_config() has different behaviour. So, patch 2 changes the driver behaviour in the case of a 20MHz XTAL, which is then changed again by patch 4. It would be better to have only one change of behaviour by moving patch 4 before patch 2.
On 4.02.2024 17:18, Russell King (Oracle) wrote: > On Sun, Feb 04, 2024 at 04:55:40PM +0300, Arınç ÜNAL wrote: >> This is not about laziness. This is before patch 2: >> >> phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config() >> -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config() >> -> mt753x_mac_config() >> -> mt753x_info :: mac_port_config() -> mt7530_mac_config() >> -> mt7530_setup_port5() >> -> mt753x_pad_setup() >> -> mt753x_info :: pad_setup() -> mt7530_pad_clk_setup() >> >> This is after: >> >> phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config() >> -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config() >> -> mt753x_mac_config() >> -> mt753x_info :: mac_port_config() -> mt7530_mac_config() >> -> mt7530_setup_port5() >> -> mt7530_setup_port6() >> >> Patch 2 does not move mt7530_setup_port6() to be called from >> phylink_mac_ops :: mac_config(), it already is. There is no valid reason to >> reorder the patches. >> >> My response to Russell should've stated this instead of focusing on his >> second sentence. > > This patch moves the test for a 20MHz crystal to mt7530_setup(), > which is something that is entirely orthogonal to patch 2, which > can be done cleanly (I've just applied the patches in the original > order and then reordered them: > > 98c481f5d706 net: dsa: mt7530: do not clear config->supported_interfaces > 93c6b53b17f4 net: dsa: mt7530: correct port capabilities of MT7988 > c9c6d4c51a1d net: dsa: mt7530: simplify mt7530_setup_port6() and change to void > adfa948253e0 net: dsa: mt7530: remove pad_setup function pointer > 57e21e6c2fc0 net: dsa: mt7530: call port 6 setup from mt7530_mac_config() > 959a0f9323c8 net: dsa: mt7530: move XTAL check to mt7530_setup() > 856ab64a22ef net: dsa: mt7530: empty default case on mt7530_setup_port5() > > No problems. The end result is identical comparing the git tree at the > original "move XTAL" patch with adfa948253e0. > > Now, if we look at "net: dsa: mt7530: remove pad_setup function pointer" > we can see that yes, the pad_setup() method was called from mac_confing, > but this is the exact contents of that patch removing the callsite: > > - mt753x_pad_setup(ds, state); > > This returns an integer, which may be an error code, which is ignored. > Therefore, if the XTAL frequency check fires, and mt753x_pad_setup() > returns an error, it is ignored today. > > After "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()" > the renamed pad_setup() method is now called from mac_config() thusly: > > + ret = mt7530_setup_port6(priv->ds, interface); > + if (ret) > + return ret; > > So now the error checks cause mt7530_mac_config() to return an error > which in turn causes mt753x_mac_config() to fail, and therefore > mt753x_phylink_mac_config() has different behaviour. > > So, patch 2 changes the driver behaviour in the case of a 20MHz XTAL, > which is then changed again by patch 4. > > It would be better to have only one change of behaviour by moving > patch 4 before patch 2. If the idea is to not bring any more error returns to mt753x_mac_config() because the return code is actually checked for that, I should do a bit more effort and put patch 5 before patch 2 as well, to live up to what you originally requested. Because, to get rid of all error returns on mt7530_setup_port6(), both "net: dsa: mt7530: move XTAL check to mt7530_setup()" and "net: dsa: mt7530: simplify mt7530_setup_port6() and change to void" patches are needed. After these patches, I can move mt7530_setup_port6() to mt7530_mac_config() and there won't be any error returns being brought under mt753x_mac_config(). pw-bot: cr Arınç
On Sun, Feb 04, 2024 at 06:55:14PM +0300, Arınç ÜNAL wrote: > On 4.02.2024 17:18, Russell King (Oracle) wrote: > > On Sun, Feb 04, 2024 at 04:55:40PM +0300, Arınç ÜNAL wrote: > > > This is not about laziness. This is before patch 2: > > > > > > phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config() > > > -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config() > > > -> mt753x_mac_config() > > > -> mt753x_info :: mac_port_config() -> mt7530_mac_config() > > > -> mt7530_setup_port5() > > > -> mt753x_pad_setup() > > > -> mt753x_info :: pad_setup() -> mt7530_pad_clk_setup() > > > > > > This is after: > > > > > > phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config() > > > -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config() > > > -> mt753x_mac_config() > > > -> mt753x_info :: mac_port_config() -> mt7530_mac_config() > > > -> mt7530_setup_port5() > > > -> mt7530_setup_port6() > > > > > > Patch 2 does not move mt7530_setup_port6() to be called from > > > phylink_mac_ops :: mac_config(), it already is. There is no valid reason to > > > reorder the patches. > > > > > > My response to Russell should've stated this instead of focusing on his > > > second sentence. > > > > This patch moves the test for a 20MHz crystal to mt7530_setup(), > > which is something that is entirely orthogonal to patch 2, which > > can be done cleanly (I've just applied the patches in the original > > order and then reordered them: > > > > 98c481f5d706 net: dsa: mt7530: do not clear config->supported_interfaces > > 93c6b53b17f4 net: dsa: mt7530: correct port capabilities of MT7988 > > c9c6d4c51a1d net: dsa: mt7530: simplify mt7530_setup_port6() and change to void > > adfa948253e0 net: dsa: mt7530: remove pad_setup function pointer > > 57e21e6c2fc0 net: dsa: mt7530: call port 6 setup from mt7530_mac_config() > > 959a0f9323c8 net: dsa: mt7530: move XTAL check to mt7530_setup() > > 856ab64a22ef net: dsa: mt7530: empty default case on mt7530_setup_port5() > > > > No problems. The end result is identical comparing the git tree at the > > original "move XTAL" patch with adfa948253e0. > > > > Now, if we look at "net: dsa: mt7530: remove pad_setup function pointer" > > we can see that yes, the pad_setup() method was called from mac_confing, > > but this is the exact contents of that patch removing the callsite: > > > > - mt753x_pad_setup(ds, state); > > > > This returns an integer, which may be an error code, which is ignored. > > Therefore, if the XTAL frequency check fires, and mt753x_pad_setup() > > returns an error, it is ignored today. > > > > After "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()" > > the renamed pad_setup() method is now called from mac_config() thusly: > > > > + ret = mt7530_setup_port6(priv->ds, interface); > > + if (ret) > > + return ret; > > > > So now the error checks cause mt7530_mac_config() to return an error > > which in turn causes mt753x_mac_config() to fail, and therefore > > mt753x_phylink_mac_config() has different behaviour. > > > > So, patch 2 changes the driver behaviour in the case of a 20MHz XTAL, > > which is then changed again by patch 4. > > > > It would be better to have only one change of behaviour by moving > > patch 4 before patch 2. > > If the idea is to not bring any more error returns to mt753x_mac_config() > because the return code is actually checked for that, I should do a bit > more effort and put patch 5 before patch 2 as well, to live up to what you > originally requested. I assume you are referring to getting rid of the default case in mt7530_pad_clk_setup(). In patch "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()" where you move this to be called from mt7530_mac_config(), you add it as: + } else if (port == 6) { + ret = mt7530_setup_port6(priv->ds, interface); + if (ret) + return ret; + } So it is only called for port 6. The switch within the called function deals with PHY_INTERFACE_MODE_RGMII and PHY_INTERFACE_MODE_TRGMII. Anything else results in the use of the default case, and thus returning an error. Since mt7530_mac_port_get_caps() does this for port 6: __set_bit(PHY_INTERFACE_MODE_RGMII, config->supported_interfaces); __set_bit(PHY_INTERFACE_MODE_TRGMII, config->supported_interfaces); mt7530_setup_port6() will only ever be called for these two modes, which means that the default case is unreachable, thus we will never execute that path, thus whether that path returns an error or not is completely irrelevant. The only case in mt7530_setup_port6() / mt7530_pad_clk_setup() which can today return an error is the XTAL check. Therefore, my suggestion makes complete sense, and there is no need to also move patch 5.
On 4.02.2024 19:38, Russell King (Oracle) wrote: > On Sun, Feb 04, 2024 at 06:55:14PM +0300, Arınç ÜNAL wrote: >> On 4.02.2024 17:18, Russell King (Oracle) wrote: >>> On Sun, Feb 04, 2024 at 04:55:40PM +0300, Arınç ÜNAL wrote: >>>> This is not about laziness. This is before patch 2: >>>> >>>> phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config() >>>> -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config() >>>> -> mt753x_mac_config() >>>> -> mt753x_info :: mac_port_config() -> mt7530_mac_config() >>>> -> mt7530_setup_port5() >>>> -> mt753x_pad_setup() >>>> -> mt753x_info :: pad_setup() -> mt7530_pad_clk_setup() >>>> >>>> This is after: >>>> >>>> phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config() >>>> -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config() >>>> -> mt753x_mac_config() >>>> -> mt753x_info :: mac_port_config() -> mt7530_mac_config() >>>> -> mt7530_setup_port5() >>>> -> mt7530_setup_port6() >>>> >>>> Patch 2 does not move mt7530_setup_port6() to be called from >>>> phylink_mac_ops :: mac_config(), it already is. There is no valid reason to >>>> reorder the patches. >>>> >>>> My response to Russell should've stated this instead of focusing on his >>>> second sentence. >>> >>> This patch moves the test for a 20MHz crystal to mt7530_setup(), >>> which is something that is entirely orthogonal to patch 2, which >>> can be done cleanly (I've just applied the patches in the original >>> order and then reordered them: >>> >>> 98c481f5d706 net: dsa: mt7530: do not clear config->supported_interfaces >>> 93c6b53b17f4 net: dsa: mt7530: correct port capabilities of MT7988 >>> c9c6d4c51a1d net: dsa: mt7530: simplify mt7530_setup_port6() and change to void >>> adfa948253e0 net: dsa: mt7530: remove pad_setup function pointer >>> 57e21e6c2fc0 net: dsa: mt7530: call port 6 setup from mt7530_mac_config() >>> 959a0f9323c8 net: dsa: mt7530: move XTAL check to mt7530_setup() >>> 856ab64a22ef net: dsa: mt7530: empty default case on mt7530_setup_port5() >>> >>> No problems. The end result is identical comparing the git tree at the >>> original "move XTAL" patch with adfa948253e0. >>> >>> Now, if we look at "net: dsa: mt7530: remove pad_setup function pointer" >>> we can see that yes, the pad_setup() method was called from mac_confing, >>> but this is the exact contents of that patch removing the callsite: >>> >>> - mt753x_pad_setup(ds, state); >>> >>> This returns an integer, which may be an error code, which is ignored. >>> Therefore, if the XTAL frequency check fires, and mt753x_pad_setup() >>> returns an error, it is ignored today. >>> >>> After "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()" >>> the renamed pad_setup() method is now called from mac_config() thusly: >>> >>> + ret = mt7530_setup_port6(priv->ds, interface); >>> + if (ret) >>> + return ret; >>> >>> So now the error checks cause mt7530_mac_config() to return an error >>> which in turn causes mt753x_mac_config() to fail, and therefore >>> mt753x_phylink_mac_config() has different behaviour. >>> >>> So, patch 2 changes the driver behaviour in the case of a 20MHz XTAL, >>> which is then changed again by patch 4. >>> >>> It would be better to have only one change of behaviour by moving >>> patch 4 before patch 2. >> >> If the idea is to not bring any more error returns to mt753x_mac_config() >> because the return code is actually checked for that, I should do a bit >> more effort and put patch 5 before patch 2 as well, to live up to what you >> originally requested. > > I assume you are referring to getting rid of the default case in > mt7530_pad_clk_setup(). > > In patch "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()" > where you move this to be called from mt7530_mac_config(), you add it > as: > > + } else if (port == 6) { > + ret = mt7530_setup_port6(priv->ds, interface); > + if (ret) > + return ret; > + } > > So it is only called for port 6. The switch within the called function > deals with PHY_INTERFACE_MODE_RGMII and PHY_INTERFACE_MODE_TRGMII. > Anything else results in the use of the default case, and thus > returning an error. > > Since mt7530_mac_port_get_caps() does this for port 6: > > __set_bit(PHY_INTERFACE_MODE_RGMII, > config->supported_interfaces); > __set_bit(PHY_INTERFACE_MODE_TRGMII, > config->supported_interfaces); > > mt7530_setup_port6() will only ever be called for these two modes, > which means that the default case is unreachable, thus we will never > execute that path, thus whether that path returns an error or not is > completely irrelevant. > > The only case in mt7530_setup_port6() / mt7530_pad_clk_setup() which > can today return an error is the XTAL check. > > Therefore, my suggestion makes complete sense, and there is no need > to also move patch 5. Understood. I've already submitted v4 which moves patch 5. The remaining benefit is that there're fewer code changes as I don't need to add an error return for mt7530_setup_port6() and then remove it. Arınç
On Sun, Feb 04, 2024 at 07:51:49PM +0300, Arınç ÜNAL wrote: > On 4.02.2024 19:38, Russell King (Oracle) wrote: > > On Sun, Feb 04, 2024 at 06:55:14PM +0300, Arınç ÜNAL wrote: > > > On 4.02.2024 17:18, Russell King (Oracle) wrote: > > > > On Sun, Feb 04, 2024 at 04:55:40PM +0300, Arınç ÜNAL wrote: > > > > > This is not about laziness. This is before patch 2: > > > > > > > > > > phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config() > > > > > -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config() > > > > > -> mt753x_mac_config() > > > > > -> mt753x_info :: mac_port_config() -> mt7530_mac_config() > > > > > -> mt7530_setup_port5() > > > > > -> mt753x_pad_setup() > > > > > -> mt753x_info :: pad_setup() -> mt7530_pad_clk_setup() > > > > > > > > > > This is after: > > > > > > > > > > phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config() > > > > > -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config() > > > > > -> mt753x_mac_config() > > > > > -> mt753x_info :: mac_port_config() -> mt7530_mac_config() > > > > > -> mt7530_setup_port5() > > > > > -> mt7530_setup_port6() > > > > > > > > > > Patch 2 does not move mt7530_setup_port6() to be called from > > > > > phylink_mac_ops :: mac_config(), it already is. There is no valid reason to > > > > > reorder the patches. > > > > > > > > > > My response to Russell should've stated this instead of focusing on his > > > > > second sentence. > > > > > > > > This patch moves the test for a 20MHz crystal to mt7530_setup(), > > > > which is something that is entirely orthogonal to patch 2, which > > > > can be done cleanly (I've just applied the patches in the original > > > > order and then reordered them: > > > > > > > > 98c481f5d706 net: dsa: mt7530: do not clear config->supported_interfaces > > > > 93c6b53b17f4 net: dsa: mt7530: correct port capabilities of MT7988 > > > > c9c6d4c51a1d net: dsa: mt7530: simplify mt7530_setup_port6() and change to void > > > > adfa948253e0 net: dsa: mt7530: remove pad_setup function pointer > > > > 57e21e6c2fc0 net: dsa: mt7530: call port 6 setup from mt7530_mac_config() > > > > 959a0f9323c8 net: dsa: mt7530: move XTAL check to mt7530_setup() > > > > 856ab64a22ef net: dsa: mt7530: empty default case on mt7530_setup_port5() > > > > > > > > No problems. The end result is identical comparing the git tree at the > > > > original "move XTAL" patch with adfa948253e0. > > > > > > > > Now, if we look at "net: dsa: mt7530: remove pad_setup function pointer" > > > > we can see that yes, the pad_setup() method was called from mac_confing, > > > > but this is the exact contents of that patch removing the callsite: > > > > > > > > - mt753x_pad_setup(ds, state); > > > > > > > > This returns an integer, which may be an error code, which is ignored. > > > > Therefore, if the XTAL frequency check fires, and mt753x_pad_setup() > > > > returns an error, it is ignored today. > > > > > > > > After "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()" > > > > the renamed pad_setup() method is now called from mac_config() thusly: > > > > > > > > + ret = mt7530_setup_port6(priv->ds, interface); > > > > + if (ret) > > > > + return ret; > > > > > > > > So now the error checks cause mt7530_mac_config() to return an error > > > > which in turn causes mt753x_mac_config() to fail, and therefore > > > > mt753x_phylink_mac_config() has different behaviour. > > > > > > > > So, patch 2 changes the driver behaviour in the case of a 20MHz XTAL, > > > > which is then changed again by patch 4. > > > > > > > > It would be better to have only one change of behaviour by moving > > > > patch 4 before patch 2. > > > > > > If the idea is to not bring any more error returns to mt753x_mac_config() > > > because the return code is actually checked for that, I should do a bit > > > more effort and put patch 5 before patch 2 as well, to live up to what you > > > originally requested. > > > > I assume you are referring to getting rid of the default case in > > mt7530_pad_clk_setup(). > > > > In patch "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()" > > where you move this to be called from mt7530_mac_config(), you add it > > as: > > > > + } else if (port == 6) { > > + ret = mt7530_setup_port6(priv->ds, interface); > > + if (ret) > > + return ret; > > + } > > > > So it is only called for port 6. The switch within the called function > > deals with PHY_INTERFACE_MODE_RGMII and PHY_INTERFACE_MODE_TRGMII. > > Anything else results in the use of the default case, and thus > > returning an error. > > > > Since mt7530_mac_port_get_caps() does this for port 6: > > > > __set_bit(PHY_INTERFACE_MODE_RGMII, > > config->supported_interfaces); > > __set_bit(PHY_INTERFACE_MODE_TRGMII, > > config->supported_interfaces); > > > > mt7530_setup_port6() will only ever be called for these two modes, > > which means that the default case is unreachable, thus we will never > > execute that path, thus whether that path returns an error or not is > > completely irrelevant. > > > > The only case in mt7530_setup_port6() / mt7530_pad_clk_setup() which > > can today return an error is the XTAL check. > > > > Therefore, my suggestion makes complete sense, and there is no need > > to also move patch 5. > > Understood. I've already submitted v4 which moves patch 5. The remaining > benefit is that there're fewer code changes as I don't need to add an error > return for mt7530_setup_port6() and then remove it. I'm pretty sure netdev has a rule _not_ to submit the next version of a patch series if discussion about the previous is still ongoing... but I can't find any of the netdev rules in Documentation/networking anymore, and nothing stands out in Documentation/process... ah, Documentation/process/maintainer-netdev.rst, and here we are: "Make sure you address all the feedback in your new posting. Do not post a new version of the code if the discussion about the previous version is still ongoing, unless directly instructed by a reviewer." Discussion was still ongoing over this point, so ideally you should have waited instead of presenting a fait-accompli _during_ the ongoing discussion.
On 4.02.2024 20:07, Russell King (Oracle) wrote: > On Sun, Feb 04, 2024 at 07:51:49PM +0300, Arınç ÜNAL wrote: >> On 4.02.2024 19:38, Russell King (Oracle) wrote: >>> On Sun, Feb 04, 2024 at 06:55:14PM +0300, Arınç ÜNAL wrote: >>>> On 4.02.2024 17:18, Russell King (Oracle) wrote: >>>>> On Sun, Feb 04, 2024 at 04:55:40PM +0300, Arınç ÜNAL wrote: >>>>>> This is not about laziness. This is before patch 2: >>>>>> >>>>>> phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config() >>>>>> -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config() >>>>>> -> mt753x_mac_config() >>>>>> -> mt753x_info :: mac_port_config() -> mt7530_mac_config() >>>>>> -> mt7530_setup_port5() >>>>>> -> mt753x_pad_setup() >>>>>> -> mt753x_info :: pad_setup() -> mt7530_pad_clk_setup() >>>>>> >>>>>> This is after: >>>>>> >>>>>> phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config() >>>>>> -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config() >>>>>> -> mt753x_mac_config() >>>>>> -> mt753x_info :: mac_port_config() -> mt7530_mac_config() >>>>>> -> mt7530_setup_port5() >>>>>> -> mt7530_setup_port6() >>>>>> >>>>>> Patch 2 does not move mt7530_setup_port6() to be called from >>>>>> phylink_mac_ops :: mac_config(), it already is. There is no valid reason to >>>>>> reorder the patches. >>>>>> >>>>>> My response to Russell should've stated this instead of focusing on his >>>>>> second sentence. >>>>> >>>>> This patch moves the test for a 20MHz crystal to mt7530_setup(), >>>>> which is something that is entirely orthogonal to patch 2, which >>>>> can be done cleanly (I've just applied the patches in the original >>>>> order and then reordered them: >>>>> >>>>> 98c481f5d706 net: dsa: mt7530: do not clear config->supported_interfaces >>>>> 93c6b53b17f4 net: dsa: mt7530: correct port capabilities of MT7988 >>>>> c9c6d4c51a1d net: dsa: mt7530: simplify mt7530_setup_port6() and change to void >>>>> adfa948253e0 net: dsa: mt7530: remove pad_setup function pointer >>>>> 57e21e6c2fc0 net: dsa: mt7530: call port 6 setup from mt7530_mac_config() >>>>> 959a0f9323c8 net: dsa: mt7530: move XTAL check to mt7530_setup() >>>>> 856ab64a22ef net: dsa: mt7530: empty default case on mt7530_setup_port5() >>>>> >>>>> No problems. The end result is identical comparing the git tree at the >>>>> original "move XTAL" patch with adfa948253e0. >>>>> >>>>> Now, if we look at "net: dsa: mt7530: remove pad_setup function pointer" >>>>> we can see that yes, the pad_setup() method was called from mac_confing, >>>>> but this is the exact contents of that patch removing the callsite: >>>>> >>>>> - mt753x_pad_setup(ds, state); >>>>> >>>>> This returns an integer, which may be an error code, which is ignored. >>>>> Therefore, if the XTAL frequency check fires, and mt753x_pad_setup() >>>>> returns an error, it is ignored today. >>>>> >>>>> After "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()" >>>>> the renamed pad_setup() method is now called from mac_config() thusly: >>>>> >>>>> + ret = mt7530_setup_port6(priv->ds, interface); >>>>> + if (ret) >>>>> + return ret; >>>>> >>>>> So now the error checks cause mt7530_mac_config() to return an error >>>>> which in turn causes mt753x_mac_config() to fail, and therefore >>>>> mt753x_phylink_mac_config() has different behaviour. >>>>> >>>>> So, patch 2 changes the driver behaviour in the case of a 20MHz XTAL, >>>>> which is then changed again by patch 4. >>>>> >>>>> It would be better to have only one change of behaviour by moving >>>>> patch 4 before patch 2. >>>> >>>> If the idea is to not bring any more error returns to mt753x_mac_config() >>>> because the return code is actually checked for that, I should do a bit >>>> more effort and put patch 5 before patch 2 as well, to live up to what you >>>> originally requested. >>> >>> I assume you are referring to getting rid of the default case in >>> mt7530_pad_clk_setup(). >>> >>> In patch "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()" >>> where you move this to be called from mt7530_mac_config(), you add it >>> as: >>> >>> + } else if (port == 6) { >>> + ret = mt7530_setup_port6(priv->ds, interface); >>> + if (ret) >>> + return ret; >>> + } >>> >>> So it is only called for port 6. The switch within the called function >>> deals with PHY_INTERFACE_MODE_RGMII and PHY_INTERFACE_MODE_TRGMII. >>> Anything else results in the use of the default case, and thus >>> returning an error. >>> >>> Since mt7530_mac_port_get_caps() does this for port 6: >>> >>> __set_bit(PHY_INTERFACE_MODE_RGMII, >>> config->supported_interfaces); >>> __set_bit(PHY_INTERFACE_MODE_TRGMII, >>> config->supported_interfaces); >>> >>> mt7530_setup_port6() will only ever be called for these two modes, >>> which means that the default case is unreachable, thus we will never >>> execute that path, thus whether that path returns an error or not is >>> completely irrelevant. >>> >>> The only case in mt7530_setup_port6() / mt7530_pad_clk_setup() which >>> can today return an error is the XTAL check. >>> >>> Therefore, my suggestion makes complete sense, and there is no need >>> to also move patch 5. >> >> Understood. I've already submitted v4 which moves patch 5. The remaining >> benefit is that there're fewer code changes as I don't need to add an error >> return for mt7530_setup_port6() and then remove it. > > I'm pretty sure netdev has a rule _not_ to submit the next version of > a patch series if discussion about the previous is still ongoing... > but I can't find any of the netdev rules in Documentation/networking > anymore, and nothing stands out in Documentation/process... ah, > Documentation/process/maintainer-netdev.rst, and here we are: > > "Make sure you address all the feedback in your new posting. Do not post > a new > version of the code if the discussion about the previous version is > still > ongoing, unless directly instructed by a reviewer." > > Discussion was still ongoing over this point, so ideally you should > have waited instead of presenting a fait-accompli _during_ the > ongoing discussion. Thank you for bringing this to my attention. Next time I will be more careful when deciding whether the discussion was over. Arınç
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index fdaf65b58b72..c4d492e29fdf 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -422,13 +422,6 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface) xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK; - if (xtal == HWTRAP_XTAL_20MHZ) { - dev_err(priv->dev, - "%s: MT7530 with a 20MHz XTAL is not supported!\n", - __func__); - return -EINVAL; - } - switch (interface) { case PHY_INTERFACE_MODE_RGMII: trgint = 0; @@ -2253,6 +2246,12 @@ mt7530_setup(struct dsa_switch *ds) return -ENODEV; } + if ((val & HWTRAP_XTAL_MASK) == HWTRAP_XTAL_20MHZ) { + dev_err(priv->dev, + "MT7530 with a 20MHz XTAL is not supported!\n"); + return -EINVAL; + } + /* Reset the switch through internal reset */ mt7530_write(priv, MT7530_SYS_CTRL, SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |