| Message ID | 20201230103309.8956-1-ckeepax@opensource.cirrus.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] net: macb: Correct usage of MACB_CAPS_CLK_HW_CHG flag | expand |
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
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
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
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) {
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(-)