| Message ID | 20230317174243.61500-2-sebastian.reichel@collabora.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix RK3588 error prints | expand |
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.
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");
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(-)