| Message ID | E1qfiqd-007TPL-7K@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: add and use library for setting clock | expand |
On Mon, 2023-09-11 at 16:29 +0100, Russell King (Oracle) wrote: > Add a platform library of helper functions for common traits in the > platform drivers. Currently, this is setting the tx clock. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> FTR, two copies of this series landed on the ML. I'm marking the no- cover-letter version as Superseded, but please have a look at the comments there, too. Thanks, Paolo
On Mon, Sep 11, 2023 at 04:29:11PM +0100, Russell King (Oracle) wrote: > Add a platform library of helper functions for common traits in the > platform drivers. Currently, this is setting the tx clock. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Hi Russell, some minor issues raised by checkpatch follow. > --- > drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +- > .../ethernet/stmicro/stmmac/stmmac_plat_lib.c | 29 +++++++++++++++++++ > .../ethernet/stmicro/stmmac/stmmac_plat_lib.h | 8 +++++ > 3 files changed, 38 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c > create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.h > > diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile > index 5b57aee19267..ba2cbfa0c9d1 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/Makefile > +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile > @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o ring_mode.o \ > mmc_core.o stmmac_hwtstamp.o stmmac_ptp.o dwmac4_descs.o \ > dwmac4_dma.o dwmac4_lib.o dwmac4_core.o dwmac5.o hwif.o \ > stmmac_tc.o dwxgmac2_core.o dwxgmac2_dma.o dwxgmac2_descs.o \ > - stmmac_xdp.o \ > + stmmac_xdp.o stmmac_plat_lib.o \ > $(stmmac-y) > > stmmac-$(CONFIG_STMMAC_SELFTESTS) += stmmac_selftests.o > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c > new file mode 100644 > index 000000000000..abb9f512bb0e > --- /dev/null > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c > @@ -0,0 +1,29 @@ Is an SPDX identifier appropriate here? > +#include <linux/stmmac.h> > +#include <linux/clk.h> > + > +#include "stmmac_plat_lib.h" > + > +int dwmac_set_tx_clk_gmii(struct clk *tx_clk, int speed) > +{ > + unsigned long rate; > + > + switch (speed) { > + case SPEED_1000: > + rate = 125000000; > + break; > + > + case SPEED_100: > + rate = 25000000; > + break; > + > + case SPEED_10: > + rate = 2500000; > + break; > + > + default: > + return -ENOTSUPP; Checkpatch seems to think that EOPNOTSUPP would be more appropriate as "ENOTSUPP is not a SUSV4 error code". > + } > + > + return clk_set_rate(tx_clk, rate); > +} > +EXPORT_SYMBOL_GPL(dwmac_set_tx_clk_gmii); ...
On Tue, Sep 12, 2023 at 04:52:27PM +0200, Simon Horman wrote: > On Mon, Sep 11, 2023 at 04:29:11PM +0100, Russell King (Oracle) wrote: > > + default: > > + return -ENOTSUPP; > > Checkpatch seems to think that EOPNOTSUPP would be more appropriate > as "ENOTSUPP is not a SUSV4 error code". It needs to be an error code that clk_set_rate() below isn't going to return - because if clk_set_rate() does return it, then the users are going to end up issuing an incorrect error message to the user. I suspect clk_set_rate() could quite legitimately return -EOPNOTSUPP or -EINVAL. Sadly, the CCF implementation of clk_set_rate() doesn't detail what errors it could return, but it looks like -EBUSY, -EINVAL, or something from pm_runtime_resume_and_get(). Interestingly, while looking at this, pm_runtime_resume_and_get() can return '1' if e.g. rpm is disabled and the device is active. It looks to me like CCF treats that as an error in multiple locations. > > + } > > + > > + return clk_set_rate(tx_clk, rate); > > +} > > +EXPORT_SYMBOL_GPL(dwmac_set_tx_clk_gmii);
On Tue, Sep 12, 2023 at 11:20:55PM +0100, Russell King (Oracle) wrote: > On Tue, Sep 12, 2023 at 04:52:27PM +0200, Simon Horman wrote: > > On Mon, Sep 11, 2023 at 04:29:11PM +0100, Russell King (Oracle) wrote: > > > + default: > > > + return -ENOTSUPP; > > > > Checkpatch seems to think that EOPNOTSUPP would be more appropriate > > as "ENOTSUPP is not a SUSV4 error code". > > It needs to be an error code that clk_set_rate() below isn't going to > return - because if clk_set_rate() does return it, then the users are > going to end up issuing an incorrect error message to the user. I > suspect clk_set_rate() could quite legitimately return -EOPNOTSUPP > or -EINVAL. > > Sadly, the CCF implementation of clk_set_rate() doesn't detail what > errors it could return, but it looks like -EBUSY, -EINVAL, or something > from pm_runtime_resume_and_get(). Thanks Russell, Understood. In that case perhaps ENOTSUPP is not such a bad choice as: a) it seems rather unlikely CCF would use it; and b) the scope of usage is well contained - the helper and any direct callers. No further objections from my side :) > > Interestingly, while looking at this, pm_runtime_resume_and_get() can > return '1' if e.g. rpm is disabled and the device is active. It looks > to me like CCF treats that as an error in multiple locations. The plot thickens...
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile index 5b57aee19267..ba2cbfa0c9d1 100644 --- a/drivers/net/ethernet/stmicro/stmmac/Makefile +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o ring_mode.o \ mmc_core.o stmmac_hwtstamp.o stmmac_ptp.o dwmac4_descs.o \ dwmac4_dma.o dwmac4_lib.o dwmac4_core.o dwmac5.o hwif.o \ stmmac_tc.o dwxgmac2_core.o dwxgmac2_dma.o dwxgmac2_descs.o \ - stmmac_xdp.o \ + stmmac_xdp.o stmmac_plat_lib.o \ $(stmmac-y) stmmac-$(CONFIG_STMMAC_SELFTESTS) += stmmac_selftests.o diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c new file mode 100644 index 000000000000..abb9f512bb0e --- /dev/null +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c @@ -0,0 +1,29 @@ +#include <linux/stmmac.h> +#include <linux/clk.h> + +#include "stmmac_plat_lib.h" + +int dwmac_set_tx_clk_gmii(struct clk *tx_clk, int speed) +{ + unsigned long rate; + + switch (speed) { + case SPEED_1000: + rate = 125000000; + break; + + case SPEED_100: + rate = 25000000; + break; + + case SPEED_10: + rate = 2500000; + break; + + default: + return -ENOTSUPP; + } + + return clk_set_rate(tx_clk, rate); +} +EXPORT_SYMBOL_GPL(dwmac_set_tx_clk_gmii); diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.h new file mode 100644 index 000000000000..926fdce379b3 --- /dev/null +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.h @@ -0,0 +1,8 @@ +#ifndef STMMAC_PLAT_LIB_H +#define STMMAC_PLAT_LIB_H + +struct clk; + +int dwmac_set_tx_clk_gmii(struct clk *tx_clk, int speed); + +#endif
Add a platform library of helper functions for common traits in the platform drivers. Currently, this is setting the tx clock. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +- .../ethernet/stmicro/stmmac/stmmac_plat_lib.c | 29 +++++++++++++++++++ .../ethernet/stmicro/stmmac/stmmac_plat_lib.h | 8 +++++ 3 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.h