[PATCH 00/24] clk: Remove passing of negative errors through unsigned return
Simon Glass
sjg at chromium.org
Sun Oct 19 20:28:52 CEST 2025
Hi Tom,
On Sun, 19 Oct 2025 at 17:21, Tom Rini <trini at konsulko.com> wrote:
>
> On Sun, Oct 19, 2025 at 04:45:24PM +0300, Svyatoslav Ryhel wrote:
> > нд, 19 жовт. 2025 р. о 16:05 Simon Glass <sjg at chromium.org> пише:
> > >
> > > Hi Tom, Andrew,
> > >
> > > On Sat, 18 Oct 2025 at 15:04, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Sat, Oct 18, 2025 at 09:34:42AM +0100, Simon Glass wrote:
> > > > > Hi Andrew,
> > > > >
> > > > > On Wed, 15 Oct 2025 at 15:32, Andrew Goodbody
> > > > > <andrew.goodbody at linaro.org> wrote:
> > > > > >
> > > > > > This series removes the passing of negative errors through the .get_rate
> > > > > > function in the clk_ops struct. This function returns an unsigned long.
> > > > > > The only value guaranteed to not be a valid clock rate is 0. This will
> > > > > > also bring the drivers more in sync with Linux to allow for easier code
> > > > > > porting and other maintenance in the future.
> > > > > > Another series will address the calling of clk_get_rate and associated
> > > > > > error handling.
> > > > >
> > > > > Some indication of the problem you ran into would be useful here.
> > > >
> > > > The problem statement is in the paragraph you're quoting. The numerical
> > > > value of -ENOENT is a valid clock rate.
> > >
> > > No, I mean a problem with a board, or something like that. We are
> > > talking here about not being able to return a valid clock rate between
> > > 4294967040 and 4294967295, which is only even a theoretical problem on
> > > 32-bit machines. So I think it is reasonable to include a motivation.
> > >
> > >
> > > >
> > > > > > Signed-off-by: Andrew Goodbody <andrew.goodbody at linaro.org>
> > > > > > ---
> > > > > > Andrew Goodbody (24):
> > > > > > clk: meson: Remove negative error returns from clk_get_rate
> > > > > > clk: sifive: Remove negative error returns from clk_get_rate
> > > > > > clk: armada-37xx: Remove negative error returns from clk_get_rate
> > > > > > clk: thead: th1520-ap: Remove negative error returns from clk_get_rate
> > > > > > clk: ccf: Remove negative error returns from clk_get_rate
> > > > > > clk: at91: Remove negative error returns from clk_get_rate
> > > > > > clk: renesas: Remove negative error returns from clk_get_rate
> > > > > > clk: rockchip: Remove negative error returns from clk_get_rate
> > > > > > clk: Remove negative error returns from clk_get_rate
> > > > > > clk: starfive: Remove negative error returns from clk_get_rate
> > > > > > clk: altera: Remove negative error returns from clk_get_rate
> > > > > > clk: uniphier: Remove negative error returns from clk_get_rate
> > > > > > clk: aspeed: Remove negative error returns from clk_get_rate
> > > > > > clk: nuvoton: Remove negative error returns from clk_get_rate
> > > > > > clk: exynos: Remove negative error returns from clk_get_rate
> > > > > > clk: imx: Remove negative error returns from clk_get_rate
> > > > > > clk: ti: Remove negative error returns from clk_get_rate
> > > > > > clk: mediatek: Remove negative error returns from clk_get_rate
> > > > > > clk: owl: Remove negative error returns from clk_get_rate
> > > > > > clk: tegra: Remove negative error returns from clk_get_rate
> > > > > > clk: adi: Remove negative error returns from clk_get_rate
> > > > > > clk: sophgo: Remove negative error returns from clk_get_rate
> > > > > > clk: stm32: Remove negative error returns from clk_get_rate
> > > > > > clk: x86: Remove negative error returns from clk_get_rate
> > > > > >
> > > > > > drivers/clk/adi/clk-shared.c | 2 +-
> > > > > > drivers/clk/altera/clk-agilex.c | 2 +-
> > > > > > drivers/clk/altera/clk-agilex5.c | 2 +-
> > > > > > drivers/clk/altera/clk-n5x.c | 2 +-
> > > > > > drivers/clk/aspeed/clk_ast2500.c | 2 +-
> > > > > > drivers/clk/aspeed/clk_ast2600.c | 2 +-
> > > > > > drivers/clk/at91/compat.c | 6 ++--
> > > > > > drivers/clk/clk-hsdk-cgu.c | 2 +-
> > > > > > drivers/clk/clk-uclass.c | 4 +--
> > > > > > drivers/clk/clk.c | 2 +-
> > > > > > drivers/clk/clk_fixed_factor.c | 4 +--
> > > > > > drivers/clk/clk_k210.c | 6 ++--
> > > > > > drivers/clk/clk_sandbox.c | 4 +--
> > > > > > drivers/clk/clk_scmi.c | 4 +--
> > > > > > drivers/clk/clk_vexpress_osc.c | 2 +-
> > > > > > drivers/clk/clk_zynq.c | 4 +--
> > > > > > drivers/clk/clk_zynqmp.c | 40 ++++++++++-----------
> > > > > > drivers/clk/exynos/clk-exynos7420.c | 2 +-
> > > > > > drivers/clk/imx/clk-imx8qm.c | 6 ++--
> > > > > > drivers/clk/imx/clk-imx8qxp.c | 6 ++--
> > > > > > drivers/clk/imx/clk-imxrt1170.c | 2 +-
> > > > > > drivers/clk/imx/clk-pllv3.c | 2 +-
> > > > > > drivers/clk/intel/clk_intel.c | 2 +-
> > > > > > drivers/clk/mediatek/clk-mtk.c | 2 +-
> > > > > > drivers/clk/meson/a1.c | 10 +++---
> > > > > > drivers/clk/meson/axg.c | 10 +++---
> > > > > > drivers/clk/meson/g12a.c | 36 +++++++++----------
> > > > > > drivers/clk/meson/gxbb.c | 20 +++++------
> > > > > > drivers/clk/mvebu/armada-37xx-periph.c | 2 +-
> > > > > > drivers/clk/mvebu/armada-37xx-tbg.c | 2 +-
> > > > > > drivers/clk/nuvoton/clk_npcm.c | 10 +++---
> > > > > > drivers/clk/owl/clk_owl.c | 2 +-
> > > > > > drivers/clk/renesas/clk-rcar-gen2.c | 8 ++---
> > > > > > drivers/clk/renesas/rzg2l-cpg.c | 8 ++---
> > > > > > drivers/clk/rockchip/clk_px30.c | 24 ++++++-------
> > > > > > drivers/clk/rockchip/clk_rk3036.c | 2 +-
> > > > > > drivers/clk/rockchip/clk_rk3066.c | 8 ++---
> > > > > > drivers/clk/rockchip/clk_rk3128.c | 6 ++--
> > > > > > drivers/clk/rockchip/clk_rk3188.c | 6 ++--
> > > > > > drivers/clk/rockchip/clk_rk322x.c | 4 +--
> > > > > > drivers/clk/rockchip/clk_rk3288.c | 6 ++--
> > > > > > drivers/clk/rockchip/clk_rk3308.c | 26 +++++++-------
> > > > > > drivers/clk/rockchip/clk_rk3328.c | 6 ++--
> > > > > > drivers/clk/rockchip/clk_rk3368.c | 8 ++---
> > > > > > drivers/clk/rockchip/clk_rk3399.c | 12 +++----
> > > > > > drivers/clk/rockchip/clk_rk3528.c | 20 +++++------
> > > > > > drivers/clk/rockchip/clk_rk3568.c | 62 ++++++++++++++++----------------
> > > > > > drivers/clk/rockchip/clk_rk3576.c | 36 +++++++++----------
> > > > > > drivers/clk/rockchip/clk_rk3588.c | 32 ++++++++---------
> > > > > > drivers/clk/rockchip/clk_rv1108.c | 4 +--
> > > > > > drivers/clk/rockchip/clk_rv1126.c | 52 +++++++++++++--------------
> > > > > > drivers/clk/sifive/sifive-prci.c | 8 ++---
> > > > > > drivers/clk/sophgo/clk-cv1800b.c | 2 +-
> > > > > > drivers/clk/starfive/clk-jh7110-pll.c | 2 +-
> > > > > > drivers/clk/stm32/clk-stm32-core.c | 4 +--
> > > > > > drivers/clk/stm32/clk-stm32f.c | 6 ++--
> > > > > > drivers/clk/stm32/clk-stm32h7.c | 4 +--
> > > > > > drivers/clk/tegra/tegra-car-clk.c | 2 +-
> > > > > > drivers/clk/tegra/tegra186-clk.c | 2 +-
> > > > > > drivers/clk/thead/clk-th1520-ap.c | 2 +-
> > > > > > drivers/clk/ti/clk-am3-dpll-x2.c | 4 +--
> > > > > > drivers/clk/ti/clk-divider.c | 4 +--
> > > > > > drivers/clk/ti/clk-mux.c | 2 +-
> > > > > > drivers/clk/ti/clk-sci.c | 2 +-
> > > > > > drivers/clk/uniphier/clk-uniphier-core.c | 2 +-
> > > > > > 65 files changed, 290 insertions(+), 290 deletions(-)
> > > > > > ---
> > > > > > base-commit: ecdc3872a767fb045be3296d4317ae978a14b022
> > > > > > change-id: 20251010-clk_ops-3b7cc9ccd070
> > > > > >
> > > > > > Best regards,
> > > > > > --
> > > > > > Andrew Goodbody <andrew.goodbody at linaro.org>
> > > > > >
> > > > >
> > > > > If you don't return an error, we cannot tell if the operation
> > > > > succeeded, or not. U-Boot needs to be deterministic and we need to be
> > > > > able to debug errors and detect them at runtime.
> > > > >
> > > > > We use ulong for the return value as a bit of a compromise, since it
> > > > > is inefficient to use 64-bit on a 32-bit machine. Ideally it would be
> > > > > long, but some clock rates are 3GHz and it would be confusing to cast
> > > > > to ulong before using the value.
> > > > >
> > > > > An alternative we discussed was to return an integer error with the
> > > > > clock rate returned in a parameter, but that seemed less efficient.
> > > > >
> > > > > With 64-bit machines, there really isn't a problem. Just checking for
> > > > > a negative value is good enough, since the clock rate isn't going to
> > > > > be 9 exahertz(?). Values between -CONFIG_ERR_PTR_OFFSET and 0 are
> > > > > errors and are defined to be so.
> > > > >
> > > > > If you want clk_get_rate() to work like Linux (suppress / ignore
> > > > > errors?), that's fine, but please create a clk_get_rate_err() (or
> > > > > similar) which actually returns the correct error, and keep the error
> > > > > return on the uclass interface. It is not uncommon to have the uclass
> > > > > do some processing on values passed to/from driver. This allows people
> > > > > who care to obtain the error.
> > > >
> > > > This is moving things in the right direction of having the error
> > > > reporting and handling done where it can be done correctly. If there's
> > > > further parts of the Linux kernel-like API we need, we can take those
> > > > next.
> > >
> > > Here is part of the patch:
> > >
> > > --- a/drivers/clk/meson/a1.c
> > > +++ b/drivers/clk/meson/a1.c
> > > @@ -359,7 +359,7 @@ static ulong meson_div_get_rate(struct clk *clk,
> > > unsigned long id)
> > >
> > > info = meson_clk_get_info(clk, id, MESON_CLK_DIV);
> > > if (IS_ERR(info))
> > > - return PTR_ERR(info);
> > > + return 0;
> > >
> > > I don't see anything in that fragment other than just ignoring errors.
> > >
> >
> > I agree with Simon. This patchset may cause a lot of problems for all
> > boards it changes in case clk ops fail, since it silences all error
> > returns. Any clock error will be untrackable unless you know where and
> > what to look specifically.
>
> I think it's worth going back to the original thread:
> https://lore.kernel.org/u-boot/f5c94319-8ef8-459d-88b2-836702779cfb@linaro.org/
Specifically Heinrich's response which seems to make sense to me -
i.e. IS_ERR_VALUE(). A series that introduced that everywhere would
get my vote :-)
>
> As part of the problem is that what we have today does not work. This is
> why I'm think it's OK to first return 0, always, as the invalid clock
> rate and then re-introduce error checking that can work.
OK, but actually what doesn't work today? Which board is broken? Is
the problem that no one checks the errors?
- Simon
More information about the U-Boot
mailing list