[net-next,v3,4/7] net: dsa: mt7530: move XTAL check to mt7530_setup()

Commit Message

Arınç ÜNAL via B4 Relay Feb. 2, 2024, 9:19 a.m. UTC

From: Arınç ÜNAL <[e-mail]>

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 <[e-mail]>
Reviewed-by: Andrew Lunn <[e-mail]>
Reviewed-by: Vladimir Oltean <[e-mail]>
---
 drivers/net/dsa/mt7530.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Comments

Russell King (Oracle) Feb. 2, 2024, 11:48 a.m. UTC | #1

On Fri, Feb 02, 2024 at 12:19:10PM +0300, Arınç ÜNAL via B4 Relay wrote:
> From: Arınç ÜNAL <[e-mail]>
> 
> 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 <[e-mail]>
> Reviewed-by: Andrew Lunn <[e-mail]>
> Reviewed-by: Vladimir Oltean <[e-mail]>

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.

Arınç ÜNAL Feb. 2, 2024, 6:16 p.m. UTC | #2

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 <[e-mail]>
>>
>> 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 <[e-mail]>
>> Reviewed-by: Andrew Lunn <[e-mail]>
>> Reviewed-by: Vladimir Oltean <[e-mail]>
> 
> 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ç

Daniel Golle Feb. 2, 2024, 6:39 p.m. UTC | #3

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 <[e-mail]>
> > > 
> > > 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 <[e-mail]>
> > > Reviewed-by: Andrew Lunn <[e-mail]>
> > > Reviewed-by: Vladimir Oltean <[e-mail]>
> > 
> > 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.

Arınç ÜNAL Feb. 4, 2024, 1:55 p.m. UTC | #4

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 <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>> Reviewed-by: Andrew Lunn <[e-mail]>
>>>> Reviewed-by: Vladimir Oltean <[e-mail]>
>>>
>>> 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ç

Russell King (Oracle) Feb. 4, 2024, 2:18 p.m. UTC | #5

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 <[e-mail]>
> > > > > 
> > > > > 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 <[e-mail]>
> > > > > Reviewed-by: Andrew Lunn <[e-mail]>
> > > > > Reviewed-by: Vladimir Oltean <[e-mail]>
> > > > 
> > > > 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.

Arınç ÜNAL Feb. 4, 2024, 3:55 p.m. UTC | #6

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ç

Russell King (Oracle) Feb. 4, 2024, 4:38 p.m. UTC | #7

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.

Arınç ÜNAL Feb. 4, 2024, 4:51 p.m. UTC | #8

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ç

Russell King (Oracle) Feb. 4, 2024, 5:07 p.m. UTC | #9

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.

Arınç ÜNAL Feb. 4, 2024, 5:14 p.m. UTC | #10

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ç

Patch

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 |