[PATCHv1,1/2] net: ethernet: stmmac: dwmac-rk: fix optional clock handling
Commit Message
Right now any clock errors are printed and otherwise ignored.
This has multiple disadvantages:
1. it prints errors for clocks that do not exist (e.g. rk3588
reports errors for "mac_clk_rx", "mac_clk_tx" and "clk_mac_speed")
2. it does not handle errors like -EPROBE_DEFER correctly
This series fixes it by switching to devm_clk_get_optional(),
so that missing clocks are not considered an error and then
passing on any other errors using dev_err_probe().
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
.../net/ethernet/stmicro/stmmac/dwmac-rk.c | 47 ++++++++++---------
1 file changed, 24 insertions(+), 23 deletions(-)
Comments
On Fri, Mar 17, 2023 at 06:42:42PM +0100, Sebastian Reichel wrote:
> Right now any clock errors are printed and otherwise ignored.
> This has multiple disadvantages:
>
> 1. it prints errors for clocks that do not exist (e.g. rk3588
> reports errors for "mac_clk_rx", "mac_clk_tx" and "clk_mac_speed")
>
> 2. it does not handle errors like -EPROBE_DEFER correctly
>
> This series fixes it by switching to devm_clk_get_optional(),
> so that missing clocks are not considered an error and then
> passing on any other errors using dev_err_probe().
>
Fixes tag would help here.
Piotr
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
> .../net/ethernet/stmicro/stmmac/dwmac-rk.c | 47 ++++++++++---------
> 1 file changed, 24 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index 4b8fd11563e4..126812cd17e6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> @@ -1479,49 +1479,50 @@ static int rk_gmac_clk_init(struct plat_stmmacenet_data *plat)
>
> bsp_priv->clk_enabled = false;
>
> - bsp_priv->mac_clk_rx = devm_clk_get(dev, "mac_clk_rx");
> + bsp_priv->mac_clk_rx = devm_clk_get_optional(dev, "mac_clk_rx");
> if (IS_ERR(bsp_priv->mac_clk_rx))
> - dev_err(dev, "cannot get clock %s\n",
> - "mac_clk_rx");
> + return dev_err_probe(dev, PTR_ERR(bsp_priv->mac_clk_rx),
> + "cannot get clock %s\n", "mac_clk_rx");
>
> - bsp_priv->mac_clk_tx = devm_clk_get(dev, "mac_clk_tx");
> + bsp_priv->mac_clk_tx = devm_clk_get_optional(dev, "mac_clk_tx");
> if (IS_ERR(bsp_priv->mac_clk_tx))
> - dev_err(dev, "cannot get clock %s\n",
> - "mac_clk_tx");
> + return dev_err_probe(dev, PTR_ERR(bsp_priv->mac_clk_tx),
> + "cannot get clock %s\n", "mac_clk_tx");
>
> - bsp_priv->aclk_mac = devm_clk_get(dev, "aclk_mac");
> + bsp_priv->aclk_mac = devm_clk_get_optional(dev, "aclk_mac");
> if (IS_ERR(bsp_priv->aclk_mac))
> - dev_err(dev, "cannot get clock %s\n",
> - "aclk_mac");
> + return dev_err_probe(dev, PTR_ERR(bsp_priv->aclk_mac),
> + "cannot get clock %s\n", "aclk_mac");
>
> - bsp_priv->pclk_mac = devm_clk_get(dev, "pclk_mac");
> + bsp_priv->pclk_mac = devm_clk_get_optional(dev, "pclk_mac");
> if (IS_ERR(bsp_priv->pclk_mac))
> - dev_err(dev, "cannot get clock %s\n",
> - "pclk_mac");
> + return dev_err_probe(dev, PTR_ERR(bsp_priv->pclk_mac),
> + "cannot get clock %s\n", "pclk_mac");
>
> - bsp_priv->clk_mac = devm_clk_get(dev, "stmmaceth");
> + bsp_priv->clk_mac = devm_clk_get_optional(dev, "stmmaceth");
> if (IS_ERR(bsp_priv->clk_mac))
> - dev_err(dev, "cannot get clock %s\n",
> - "stmmaceth");
> + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac),
> + "cannot get clock %s\n", "stmmaceth");
>
> if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII) {
> - bsp_priv->clk_mac_ref = devm_clk_get(dev, "clk_mac_ref");
> + bsp_priv->clk_mac_ref = devm_clk_get_optional(dev, "clk_mac_ref");
> if (IS_ERR(bsp_priv->clk_mac_ref))
> - dev_err(dev, "cannot get clock %s\n",
> - "clk_mac_ref");
> + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_ref),
> + "cannot get clock %s\n", "clk_mac_ref");
>
> if (!bsp_priv->clock_input) {
> bsp_priv->clk_mac_refout =
> - devm_clk_get(dev, "clk_mac_refout");
> + devm_clk_get_optional(dev, "clk_mac_refout");
> if (IS_ERR(bsp_priv->clk_mac_refout))
> - dev_err(dev, "cannot get clock %s\n",
> - "clk_mac_refout");
> + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_refout),
> + "cannot get clock %s\n", "clk_mac_refout");
> }
> }
>
> - bsp_priv->clk_mac_speed = devm_clk_get(dev, "clk_mac_speed");
> + bsp_priv->clk_mac_speed = devm_clk_get_optional(dev, "clk_mac_speed");
> if (IS_ERR(bsp_priv->clk_mac_speed))
> - dev_err(dev, "cannot get clock %s\n", "clk_mac_speed");
> + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_speed),
> + "cannot get clock %s\n", "clk_mac_speed");
>
> if (bsp_priv->clock_input) {
> dev_info(dev, "clock input from PHY\n");
> --
> 2.39.2
>
Hi,
On Fri, Mar 17, 2023 at 07:24:31PM +0100, Piotr Raczynski wrote:
> On Fri, Mar 17, 2023 at 06:42:42PM +0100, Sebastian Reichel wrote:
> > Right now any clock errors are printed and otherwise ignored.
> > This has multiple disadvantages:
> >
> > 1. it prints errors for clocks that do not exist (e.g. rk3588
> > reports errors for "mac_clk_rx", "mac_clk_tx" and "clk_mac_speed")
> >
> > 2. it does not handle errors like -EPROBE_DEFER correctly
> >
> > This series fixes it by switching to devm_clk_get_optional(),
> > so that missing clocks are not considered an error and then
> > passing on any other errors using dev_err_probe().
> >
>
> Fixes tag would help here.
Fixes: 7ad269ea1a2b7 ("GMAC: add driver for Rockchip RK3288 SoCs integrated GMAC")
That commit is from 2014-12-29. I only noticed the dev_err()
messages on RK3588, so going via -next without backporting should be fine.
Please tell me if I should resend with the Fixes tag.
-- Sebastian
> Piotr
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> > .../net/ethernet/stmicro/stmmac/dwmac-rk.c | 47 ++++++++++---------
> > 1 file changed, 24 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > index 4b8fd11563e4..126812cd17e6 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > @@ -1479,49 +1479,50 @@ static int rk_gmac_clk_init(struct plat_stmmacenet_data *plat)
> >
> > bsp_priv->clk_enabled = false;
> >
> > - bsp_priv->mac_clk_rx = devm_clk_get(dev, "mac_clk_rx");
> > + bsp_priv->mac_clk_rx = devm_clk_get_optional(dev, "mac_clk_rx");
> > if (IS_ERR(bsp_priv->mac_clk_rx))
> > - dev_err(dev, "cannot get clock %s\n",
> > - "mac_clk_rx");
> > + return dev_err_probe(dev, PTR_ERR(bsp_priv->mac_clk_rx),
> > + "cannot get clock %s\n", "mac_clk_rx");
> >
> > - bsp_priv->mac_clk_tx = devm_clk_get(dev, "mac_clk_tx");
> > + bsp_priv->mac_clk_tx = devm_clk_get_optional(dev, "mac_clk_tx");
> > if (IS_ERR(bsp_priv->mac_clk_tx))
> > - dev_err(dev, "cannot get clock %s\n",
> > - "mac_clk_tx");
> > + return dev_err_probe(dev, PTR_ERR(bsp_priv->mac_clk_tx),
> > + "cannot get clock %s\n", "mac_clk_tx");
> >
> > - bsp_priv->aclk_mac = devm_clk_get(dev, "aclk_mac");
> > + bsp_priv->aclk_mac = devm_clk_get_optional(dev, "aclk_mac");
> > if (IS_ERR(bsp_priv->aclk_mac))
> > - dev_err(dev, "cannot get clock %s\n",
> > - "aclk_mac");
> > + return dev_err_probe(dev, PTR_ERR(bsp_priv->aclk_mac),
> > + "cannot get clock %s\n", "aclk_mac");
> >
> > - bsp_priv->pclk_mac = devm_clk_get(dev, "pclk_mac");
> > + bsp_priv->pclk_mac = devm_clk_get_optional(dev, "pclk_mac");
> > if (IS_ERR(bsp_priv->pclk_mac))
> > - dev_err(dev, "cannot get clock %s\n",
> > - "pclk_mac");
> > + return dev_err_probe(dev, PTR_ERR(bsp_priv->pclk_mac),
> > + "cannot get clock %s\n", "pclk_mac");
> >
> > - bsp_priv->clk_mac = devm_clk_get(dev, "stmmaceth");
> > + bsp_priv->clk_mac = devm_clk_get_optional(dev, "stmmaceth");
> > if (IS_ERR(bsp_priv->clk_mac))
> > - dev_err(dev, "cannot get clock %s\n",
> > - "stmmaceth");
> > + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac),
> > + "cannot get clock %s\n", "stmmaceth");
> >
> > if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII) {
> > - bsp_priv->clk_mac_ref = devm_clk_get(dev, "clk_mac_ref");
> > + bsp_priv->clk_mac_ref = devm_clk_get_optional(dev, "clk_mac_ref");
> > if (IS_ERR(bsp_priv->clk_mac_ref))
> > - dev_err(dev, "cannot get clock %s\n",
> > - "clk_mac_ref");
> > + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_ref),
> > + "cannot get clock %s\n", "clk_mac_ref");
> >
> > if (!bsp_priv->clock_input) {
> > bsp_priv->clk_mac_refout =
> > - devm_clk_get(dev, "clk_mac_refout");
> > + devm_clk_get_optional(dev, "clk_mac_refout");
> > if (IS_ERR(bsp_priv->clk_mac_refout))
> > - dev_err(dev, "cannot get clock %s\n",
> > - "clk_mac_refout");
> > + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_refout),
> > + "cannot get clock %s\n", "clk_mac_refout");
> > }
> > }
> >
> > - bsp_priv->clk_mac_speed = devm_clk_get(dev, "clk_mac_speed");
> > + bsp_priv->clk_mac_speed = devm_clk_get_optional(dev, "clk_mac_speed");
> > if (IS_ERR(bsp_priv->clk_mac_speed))
> > - dev_err(dev, "cannot get clock %s\n", "clk_mac_speed");
> > + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_speed),
> > + "cannot get clock %s\n", "clk_mac_speed");
> >
> > if (bsp_priv->clock_input) {
> > dev_info(dev, "clock input from PHY\n");
> > --
> > 2.39.2
> >
>
> --
> To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk.
On Fri, 17 Mar 2023 18:42:42 +0100 Sebastian Reichel wrote:
> - bsp_priv->mac_clk_rx = devm_clk_get(dev, "mac_clk_rx");
> + bsp_priv->mac_clk_rx = devm_clk_get_optional(dev, "mac_clk_rx");
> if (IS_ERR(bsp_priv->mac_clk_rx))
> - dev_err(dev, "cannot get clock %s\n",
> - "mac_clk_rx");
> + return dev_err_probe(dev, PTR_ERR(bsp_priv->mac_clk_rx),
> + "cannot get clock %s\n", "mac_clk_rx");
>
> - bsp_priv->mac_clk_tx = devm_clk_get(dev, "mac_clk_tx");
> + bsp_priv->mac_clk_tx = devm_clk_get_optional(dev, "mac_clk_tx");
> if (IS_ERR(bsp_priv->mac_clk_tx))
> - dev_err(dev, "cannot get clock %s\n",
> - "mac_clk_tx");
> + return dev_err_probe(dev, PTR_ERR(bsp_priv->mac_clk_tx),
> + "cannot get clock %s\n", "mac_clk_tx");
>
> - bsp_priv->aclk_mac = devm_clk_get(dev, "aclk_mac");
> + bsp_priv->aclk_mac = devm_clk_get_optional(dev, "aclk_mac");
> if (IS_ERR(bsp_priv->aclk_mac))
> - dev_err(dev, "cannot get clock %s\n",
> - "aclk_mac");
> + return dev_err_probe(dev, PTR_ERR(bsp_priv->aclk_mac),
> + "cannot get clock %s\n", "aclk_mac");
Can we turn this into a loop
struct {
struct whatever **ptr;
const char *name;
} clocks[] = {
{ &bsp_priv->mac_clk_rx, "mac_clk_rx" },
{ &bsp_priv->mac_clk_tx, "mac_clk_tx" },
...
}
for (i=0; i < ARRAY_SIZE...) {
*clocks[i]->ptr = devm_clk_get_optional(dev, clocks[i]->name);
if (IS_ERR(*clocks[i]->ptr))
return dev_err_probe(dev, PTR_ERR(*clocks[i]->ptr),
"cannot get clock %s\n",
*clocks[i]->name);
}
?
Or alternatively inline the name of the clock into the error message?
Right now the %s format printing looks neither here nor there, and also
the continuation line is misaligned (should start right under "dev").
FWIW seems like it should be fine for net-next without the fixes tag.
@@ -1479,49 +1479,50 @@ static int rk_gmac_clk_init(struct plat_stmmacenet_data *plat)
bsp_priv->clk_enabled = false;
- bsp_priv->mac_clk_rx = devm_clk_get(dev, "mac_clk_rx");
+ bsp_priv->mac_clk_rx = devm_clk_get_optional(dev, "mac_clk_rx");
if (IS_ERR(bsp_priv->mac_clk_rx))
- dev_err(dev, "cannot get clock %s\n",
- "mac_clk_rx");
+ return dev_err_probe(dev, PTR_ERR(bsp_priv->mac_clk_rx),
+ "cannot get clock %s\n", "mac_clk_rx");
- bsp_priv->mac_clk_tx = devm_clk_get(dev, "mac_clk_tx");
+ bsp_priv->mac_clk_tx = devm_clk_get_optional(dev, "mac_clk_tx");
if (IS_ERR(bsp_priv->mac_clk_tx))
- dev_err(dev, "cannot get clock %s\n",
- "mac_clk_tx");
+ return dev_err_probe(dev, PTR_ERR(bsp_priv->mac_clk_tx),
+ "cannot get clock %s\n", "mac_clk_tx");
- bsp_priv->aclk_mac = devm_clk_get(dev, "aclk_mac");
+ bsp_priv->aclk_mac = devm_clk_get_optional(dev, "aclk_mac");
if (IS_ERR(bsp_priv->aclk_mac))
- dev_err(dev, "cannot get clock %s\n",
- "aclk_mac");
+ return dev_err_probe(dev, PTR_ERR(bsp_priv->aclk_mac),
+ "cannot get clock %s\n", "aclk_mac");
- bsp_priv->pclk_mac = devm_clk_get(dev, "pclk_mac");
+ bsp_priv->pclk_mac = devm_clk_get_optional(dev, "pclk_mac");
if (IS_ERR(bsp_priv->pclk_mac))
- dev_err(dev, "cannot get clock %s\n",
- "pclk_mac");
+ return dev_err_probe(dev, PTR_ERR(bsp_priv->pclk_mac),
+ "cannot get clock %s\n", "pclk_mac");
- bsp_priv->clk_mac = devm_clk_get(dev, "stmmaceth");
+ bsp_priv->clk_mac = devm_clk_get_optional(dev, "stmmaceth");
if (IS_ERR(bsp_priv->clk_mac))
- dev_err(dev, "cannot get clock %s\n",
- "stmmaceth");
+ return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac),
+ "cannot get clock %s\n", "stmmaceth");
if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII) {
- bsp_priv->clk_mac_ref = devm_clk_get(dev, "clk_mac_ref");
+ bsp_priv->clk_mac_ref = devm_clk_get_optional(dev, "clk_mac_ref");
if (IS_ERR(bsp_priv->clk_mac_ref))
- dev_err(dev, "cannot get clock %s\n",
- "clk_mac_ref");
+ return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_ref),
+ "cannot get clock %s\n", "clk_mac_ref");
if (!bsp_priv->clock_input) {
bsp_priv->clk_mac_refout =
- devm_clk_get(dev, "clk_mac_refout");
+ devm_clk_get_optional(dev, "clk_mac_refout");
if (IS_ERR(bsp_priv->clk_mac_refout))
- dev_err(dev, "cannot get clock %s\n",
- "clk_mac_refout");
+ return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_refout),
+ "cannot get clock %s\n", "clk_mac_refout");
}
}
- bsp_priv->clk_mac_speed = devm_clk_get(dev, "clk_mac_speed");
+ bsp_priv->clk_mac_speed = devm_clk_get_optional(dev, "clk_mac_speed");
if (IS_ERR(bsp_priv->clk_mac_speed))
- dev_err(dev, "cannot get clock %s\n", "clk_mac_speed");
+ return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_speed),
+ "cannot get clock %s\n", "clk_mac_speed");
if (bsp_priv->clock_input) {
dev_info(dev, "clock input from PHY\n");