Re: [PATCH 00/24] clk: Remove passing of negative errors through unsigned return

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 10708 bytes --]

On Sun, Oct 19, 2025 at 04:45:24PM +0300, Svyatoslav Ryhel wrote:
> нд, 19 жовт. 2025 р. о 16:05 Simon Glass <sjg@chromium.org> пише:
> >
> > Hi Tom, Andrew,
> >
> > On Sat, 18 Oct 2025 at 15:04, Tom Rini <trini@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@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@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@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/

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]