[PATCH 00/24] clk: Remove passing of negative errors through unsigned return
Philipp Tomsich
philipp.tomsich at vrull.eu
Sat Oct 18 20:16:59 CEST 2025
On Sat, 18 Oct 2025 at 16: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.
Just one concern: this series drops all indications of what happened
(as in "cause"). We shouldn't postpone the changes to refactor the
error reporting/handling.
As is, this change is error-prone: this series would wipe out all the
cause-indications and checking the later adding of error handling
would have to be manually checked to see if the same error causes have
been added back in for the new mechanism.
I would prefer to see the code changes sequenced such that any
additional error reporting functions are introduced and wired up,
before wrapping those up in a clk_get_rate() that omits the cause for
failures.
Cheers,
Philipp.
> > 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.
>
> > > 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.
>
> --
> Tom
More information about the U-Boot
mailing list