Messages in this thread | | | Date | Wed, 13 Jul 2022 08:52:57 +0200 | Subject | Re: [PATCH v4] amba: Remove deferred device addition | From | Marek Szyprowski <> |
| |
Hi Saravana,
On 12.07.2022 21:38, Saravana Kannan wrote: > On Tue, Jul 12, 2022 at 5:34 AM Marek Szyprowski > <m.szyprowski@samsung.com> wrote: >> On 12.07.2022 14:25, Marek Szyprowski wrote: >>> On 05.07.2022 10:39, Saravana Kannan wrote: >>>> The uevents generated for an amba device need PID and CID information >>>> that's available only when the amba device is powered on, clocked and >>>> out of reset. So, if those resources aren't available, the information >>>> can't be read to generate the uevents. To workaround this requirement, >>>> if the resources weren't available, the device addition was deferred and >>>> retried periodically. >>>> >>>> However, this deferred addition retry isn't based on resources becoming >>>> available. Instead, it's retried every 5 seconds and causes arbitrary >>>> probe delays for amba devices and their consumers. >>>> >>>> Also, maintaining a separate deferred-probe like mechanism is >>>> maintenance headache. >>>> >>>> With this commit, instead of deferring the device addition, we simply >>>> defer the generation of uevents for the device and probing of the device >>>> (because drivers needs PID and CID to match) until the PID and CID >>>> information can be read. This allows us to delete all the amba specific >>>> deferring code and also avoid the arbitrary probing delays. >>>> >>>> Cc: Rob Herring <robh@kernel.org> >>>> Cc: Ulf Hansson <ulf.hansson@linaro.org> >>>> Cc: John Stultz <john.stultz@linaro.org> >>>> Cc: Saravana Kannan <saravanak@google.com> >>>> Cc: Linus Walleij <linus.walleij@linaro.org> >>>> Cc: Sudeep Holla <sudeep.holla@arm.com> >>>> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> >>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >>>> Cc: Marek Szyprowski <m.szyprowski@samsung.com> >>>> Cc: Kefeng Wang <wangkefeng.wang@huawei.com> >>>> Cc: Russell King <linux@armlinux.org.uk> >>>> Signed-off-by: Saravana Kannan <saravanak@google.com> >>>> --- >>>> >>>> v1 -> v2: >>>> - Dropped RFC tag >>>> - Complete rewrite to not use stub devices. >>>> >>>> v2 -> v3: >>>> - Flipped the if() condition for hard-coded periphids. >>>> - Added a stub driver to handle the case where all amba drivers are >>>> modules loaded by uevents. >>>> - Cc Marek after I realized I forgot to add him. >>>> >>>> v3 -> v4: >>>> - Finally figured out and fixed the issue reported by Kefeng (bus match >>>> can't return an error other than -EPROBE_DEFER). >>>> - I tested the patch on "V2P-CA15" on qemu >>>> - Marek tested v3, but that was so long ago and the rebase wasn't clean, >>>> so I didn't include the tested-by. >>>> >>>> Marek/Kefeng, >>>> >>>> Mind giving a Tested-by? >>> >>> Yes, it looks that it still works fine. >>> >>> I've tested it by changing the Exynos power domain driver to >>> initialize from late_initcall. This in turn lead me to a bug in >>> generic pm_domains code in __genpd_dev_pm_attach(), which returns -2 >>> if the pm domain driver is not yet registered. After fixing that, I've >>> successfully observed the deferred probe of PL330 driver on Exynos >>> 4210 based boards both with this patch and without (with the old timer >>> based code). > Thanks for testing it again Marek! I was hoping you'll hit the crash > that Sudeep was hitting and it would give me some more clues. > > Sudeep, > > This makes me think the issue you are seeing is related to your > hardware drivers. Can you look into those please? I'm leaning towards > merging this amba clean up and adding delays (say 1ms) to your > clock/power domain drivers to avoid the crash you are seeing. And then > you can figure out the actual delays needed and update it. > >> While preparing a fix for the above issue in genpd I found that it has >> been introduced by your commit 5a46079a9645 ("PM: domains: Delete usage >> of driver_deferred_probe_check_state()"). I didn't analyze it enough, >> but it looks that something is missing there if we are trying to probe >> amba device. I assume that returning -EPROBE_DEFER unconditionally there >> is also not a valid solution? > Yeah, the unconditionally returning -EPROBE_DEFER wouldn't work > because if the supplier is optional but not present, the consumer > driver would never stop waiting for it. I'm looking into issues > similar to the one you saw in other threads [1]. The problem always > boils down to the supplier device's DT node not having "compatible" > property and therefore fw_devlink creating the device link between the > consumer and the supplier's parent. > > Basically if the drivers/DT are implemented "properly", you would > never get to the failure case (-2) if the driver is actually present. Well, I don't get what do you mean by not having the proper 'comaptible' property. Both affected devices (amba's pl330 and its power domain) have compatible strings: 'arm,pl330' and 'samsung,exynos4210-pd', but the devlinks doesn't help. Is it related to the custom device addition code in the amba bus? > I have some other ideas on how to get these to work better (not sure > if it'll be for 100% of the cases), but until I get those ideas sorted > out, I might do a partial revert of the change you mentioned. > > [1] - https://lore.kernel.org/lkml/4799738.LvFx2qVVIh@steina-w/
> ...
Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
| |