[PATCH v2] net: macb: Correct usage of MACB_CAPS_CLK_HW_CHG flag

[PATCH v2] net: macb: Correct usage of MACB_CAPS_CLK_HW_CHG flag

[Charles Keepax]

A new flag MACB_CAPS_CLK_HW_CHG was added and all callers of
macb_set_tx_clk were gated on the presence of this flag.

-   if (!clk)
+ if (!bp->tx_clk || !(bp->caps & MACB_CAPS_CLK_HW_CHG))

However the flag was not added to anything other than the new
sama7g5_gem, turning that function call into a no op for all other
systems. This breaks the networking on Zynq.

The commit message adding this states: a new capability so that
macb_set_tx_clock() to not be called for IPs having this
capability

This strongly implies that present of the flag was intended to skip
the function not absence of the flag. Update the if statement to
this effect, which repairs the existing users.

Fixes: daafa1d33cc9 ("net: macb: add capability to not set the clock rate")
Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Changes since v1:
 - Updated flag semantics to skip function, as appears to have been
   intended by the initial commit.

Thanks,
Charles

 drivers/net/ethernet/cadence/macb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index d5d910916c2e8..814a5b10141d1 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -467,7 +467,7 @@ static void macb_set_tx_clk(struct macb *bp, int speed)
 {
 	long ferr, rate, rate_rounded;
 
-	if (!bp->tx_clk || !(bp->caps & MACB_CAPS_CLK_HW_CHG))
+	if (!bp->tx_clk || (bp->caps & MACB_CAPS_CLK_HW_CHG))
 		return;
 
 	switch (speed) {
-- 
2.11.0

Re: [PATCH v2] net: macb: Correct usage of MACB_CAPS_CLK_HW_CHG flag

[Andrew Lunn]

On Wed, Dec 30, 2020 at 10:33:09AM +0000, Charles Keepax wrote:
> A new flag MACB_CAPS_CLK_HW_CHG was added and all callers of
> macb_set_tx_clk were gated on the presence of this flag.
> 
> -   if (!clk)
> + if (!bp->tx_clk || !(bp->caps & MACB_CAPS_CLK_HW_CHG))
> 
> However the flag was not added to anything other than the new
> sama7g5_gem, turning that function call into a no op for all other
> systems. This breaks the networking on Zynq.
> 
> The commit message adding this states: a new capability so that
> macb_set_tx_clock() to not be called for IPs having this
> capability
> 
> This strongly implies that present of the flag was intended to skip
> the function not absence of the flag. Update the if statement to
> this effect, which repairs the existing users.
> 
> Fixes: daafa1d33cc9 ("net: macb: add capability to not set the clock rate")
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Hi Charles

Since this is a fix, this should be based on the net tree. And you
indicate this in the subject line with [PATCH net]. If this applies
cleanly to net, Dave will probably just accept it, but please keep
this in mind for any more submissions you make to netdev.

    Andrew

Re: [PATCH v2] net: macb: Correct usage of MACB_CAPS_CLK_HW_CHG flag

[Charles Keepax]

On Wed, Dec 30, 2020 at 05:44:41PM +0100, Andrew Lunn wrote:
> On Wed, Dec 30, 2020 at 10:33:09AM +0000, Charles Keepax wrote:
> > A new flag MACB_CAPS_CLK_HW_CHG was added and all callers of
> > macb_set_tx_clk were gated on the presence of this flag.
> > 
> > -   if (!clk)
> > + if (!bp->tx_clk || !(bp->caps & MACB_CAPS_CLK_HW_CHG))
> > 
> > However the flag was not added to anything other than the new
> > sama7g5_gem, turning that function call into a no op for all other
> > systems. This breaks the networking on Zynq.
> > 
> > The commit message adding this states: a new capability so that
> > macb_set_tx_clock() to not be called for IPs having this
> > capability
> > 
> > This strongly implies that present of the flag was intended to skip
> > the function not absence of the flag. Update the if statement to
> > this effect, which repairs the existing users.
> > 
> > Fixes: daafa1d33cc9 ("net: macb: add capability to not set the clock rate")
> > Suggested-by: Andrew Lunn <andrew@lunn.ch>
> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> Hi Charles
> 
> Since this is a fix, this should be based on the net tree. And you
> indicate this in the subject line with [PATCH net]. If this applies
> cleanly to net, Dave will probably just accept it, but please keep
> this in mind for any more submissions you make to netdev.
> 

Is already based on the net tree, sorry wasn't aware I needed to add
that into the subject line. Will do a respin today.

Thanks,
Charles

Re: [PATCH v2] net: macb: Correct usage of MACB_CAPS_CLK_HW_CHG flag

[Charles Keepax]

On Wed, Dec 30, 2020 at 05:44:41PM +0100, Andrew Lunn wrote:
> On Wed, Dec 30, 2020 at 10:33:09AM +0000, Charles Keepax wrote:
> > A new flag MACB_CAPS_CLK_HW_CHG was added and all callers of
> > macb_set_tx_clk were gated on the presence of this flag.
> > 
> > -   if (!clk)
> > + if (!bp->tx_clk || !(bp->caps & MACB_CAPS_CLK_HW_CHG))
> > 
> > However the flag was not added to anything other than the new
> > sama7g5_gem, turning that function call into a no op for all other
> > systems. This breaks the networking on Zynq.
> > 
> > The commit message adding this states: a new capability so that
> > macb_set_tx_clock() to not be called for IPs having this
> > capability
> > 
> > This strongly implies that present of the flag was intended to skip
> > the function not absence of the flag. Update the if statement to
> > this effect, which repairs the existing users.
> > 
> > Fixes: daafa1d33cc9 ("net: macb: add capability to not set the clock rate")
> > Suggested-by: Andrew Lunn <andrew@lunn.ch>
> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> Hi Charles
> 
> Since this is a fix, this should be based on the net tree. And you
> indicate this in the subject line with [PATCH net]. If this applies
> cleanly to net, Dave will probably just accept it, but please keep
> this in mind for any more submissions you make to netdev.

Apologies I totally forgot to add your Reviewed-by on the new
version. Too much turkey on the brain after the fattening season.

Thanks,
Charles