El Mon, Sep 04, 2023 at 11:25:06AM +0200, Fabrice Gasnier deia:
IMHO, the OHCI should have failed too in this example, instead of silently ignoring the error. Hopefully it has probed.
The clk_get_bulk() code does a similar job compared to ohci current code. It counts all clock entries. So, when trying to get them, these should be found.
Having a clock listed, but it can't be found or taken rather looks like a real error, that needs to be fixed. (e.g. missing config for a clk/reset controller ? Or could it be a bug in such a driver ? Or a miss-configured device-tree ? something else?) Ignoring such error may be miss-leading (as you pointed out, one was working).
I hope I don't miss your point. If this is the case, could you point more precise example, or how it used to fail ?
No, you don't miss my point. I'll give you pointers to the case I meant, but I'm afraid it might mislead, because it's already solved, and for current U-Boot it should pose no problem with or without your patch.
The general problem might be that dts come from linux, and the drivers come from U-Boot, so U-Boot might ignore some hardware described in the linux dts that it doesn't need. Now this is more typical for, say, a VPU than a clock or reset. But it once was a missing clock driver in U-Boot that linux used for suspend/resume and happened to be at the end of the clock list. So it worked when ohci probe ignored the missing clock, because U-Boot doesn't need suspend, but it didn't work for ehci that called clk_get_bulk().
There might be other cases like that example somewhere, but I'm not saying it's likely. I guess we'll know if some board breaks.
If you really want the gory details...
https://patchwork.ozlabs.org/project/uboot/patch/20220701185959.GC1700@begut... https://patchwork.ozlabs.org/project/uboot/patch/Y44+ayJfUlI08ptM@localhost/ https://patchwork.ozlabs.org/project/uboot/patch/Y5IWpjYLB4aXMy9o@localhost/ https://lists.denx.de/pipermail/u-boot/2022-December/501811.html https://lists.denx.de/pipermail/u-boot/2023-February/510676.html https://lists.denx.de/pipermail/u-boot/2023-February/510678.html https://patchwork.ozlabs.org/project/uboot/patch/202013db5a47ecbac4a53c360ed... https://patchwork.ozlabs.org/project/uboot/patch/464111fca83008503022e8ada53...
In that case, a fix by ignoring the missing clock in ehci was rejected, so maybe that criteria applies here as well and your patch is deemed correct. I don't know. That case won't break now, I think, either with or without your patch, because after another fix, that clock will be found.
If I understand correctly, this used to fixed elsewhere (e.g. there used to be a real bug fixed) ?
Yes. See above. Or don't, it's not that important. A clock driver was missing, only needed for suspend/resume. ohci ignored it and worked (U-Boot doesn't suspend) ehci failed probing and dind't work. Current situation is this particular clock driver is no longer missing.
But I don't know if there might be similar cases.
I just wanted to point out the change in behaviour. If the change is intended, then all is fine.
IMHO, this should be fine. I hope you agree with this change and the rationale.
I do.
I just wanted to point it out in case anyone knew why ohci wasn't calling clk_get_bulk(). It might have been on purpose.
In fact Kever Yang once proposed to change ehci to be tolerant to a missing clock like ohci was (but with an explicit warning). But Marek Vasut proposed adding a clock driver and Kever didn't complain, so I don't think this is his very strong opinion, he may just be happy when things work and others are happy, I can't read minds.
https://lists.denx.de/pipermail/u-boot/2022-December/501811.html
FWIW
ohci_probe introduced: fee331f66c9 (Alexey Brodkin 2015-12-14 17:18:50 +0300
loop for clocks introduced in ohci_probe: 155d9f65d3b (Patrice Chotard 2017-07-18 11:57:12 +0200
clk_get_bulk introduced: a855be87da4 (Neil Armstrong 2018-04-03 11:44:18 +0200 156)
So ochi_probe() didn't call clk_get_bulk() most likely because it din't exist back then.
So, unless someone else has a failing case, I agree to your change.
I'd welcome if the commit message would say that the new policy is any missing clocks or resets cause the probe to fail. But since you already sent v2, it doesn't matter.