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