Re: [PATCH v1 4/4] of: platform: Batch fwnode parsing when adding all top level devices

[Marek Szyprowski]

Hi Saravana,

On 19.05.2020 20:02, Saravana Kannan wrote:
> On Tue, May 19, 2020 at 3:32 AM Marek Szyprowski
> <m.szyprowski@xxxxxxxxxxx> wrote:
>> On 19.05.2020 09:11, Marek Szyprowski wrote:
>>> On 19.05.2020 08:48, Saravana Kannan wrote:
>>>> On Mon, May 18, 2020 at 11:25 PM Marek Szyprowski
>>>> <m.szyprowski@xxxxxxxxxxx> wrote:
>>>>> On 15.05.2020 07:35, Saravana Kannan wrote:
>>>>>> The fw_devlink_pause() and fw_devlink_resume() APIs allow batching the
>>>>>> parsing of the device tree nodes when a lot of devices are added. This
>>>>>> will significantly cut down parsing time (as much a 1 second on some
>>>>>> systems). So, use them when adding devices for all the top level
>>>>>> device
>>>>>> tree nodes in a system.
>>>>>>
>>>>>> Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
>>>>> This patch recently landed in linux-next 20200518. Sadly, it causes
>>>>> regression on Samsung Exynos5433-based TM2e board:
>>>>>
>>>>> ...
>>>>>
>>>>> Both issues, the lack of DMA for SPI device and Synchronous abort in
>>>>> I2S
>>>>> probe are new after applying this patch. I'm trying to investigate
>>>>> which
>>>>> resources are missing and why. The latter issue means typically that
>>>>> the
>>>>> registers for the given device has been accessed without enabling the
>>>>> needed clocks or power domains.
>>>> Did you try this copy-pasta fix that I sent later?
>>>> https://lore.kernel.org/lkml/20200517173453.157703-1-saravanak@xxxxxxxxxx/
>>>>
>>>>
>>>> Not every system would need it (my test setup didn't), but it helps
>>>> some cases.
>>>>
>>>> If that fix doesn't help, then some tips for debugging the failing
>>>> drivers.
>>>> What this pause/resume patch effectively (not explicitly) does is:
>>>> 1. Doesn't immediately probe the devices as they are added in
>>>> of_platform_default_populate_init()
>>>> 2. Adds them in order to the deferred probe list.
>>>> 3. Then kicks off deferred probe on them in the order they were added.
>>>>
>>>> These drivers are just not handling -EPROBE_DEFER correctly or
>>>> assuming probe order and that's causing these issues.
>>>>
>>>> So, we can either fix that or you can try adding some code to flush
>>>> the deferred probe workqueue at the end of fw_devlink_resume().
>>>>
>>>> Let me know how it goes.
>>> So far it looks that your patch revealed a hidden issue in exynos5433
>>> clocks configuration, because adding clk_ignore_unused parameter to
>>> kernel command line fixes the boot. I'm still investigating it, so
>>> probable you can ignore my regression report. I will let you know asap
>>> I finish checking it.
>>>
>> Okay, I confirm that the issue is in the Exynos I2S driver and
>> Exynos5433 clock provider. I've posted a quick workaround. I'm sorry for
>> the noise, your patch is fine.
> Thanks for debugging and finding the real issue. I tried finding your
> patches, but couldn't. Can you point me to a lore.kernel.org link? I'm
> just curious to see what the issue was.

https://lore.kernel.org/linux-samsung-soc/f67db8c1-453b-4c70-67b9-59762ac34f64@xxxxxxxxxx/T/#t

It looks that one more clock has to be enabled to properly read init 
configuration. So far it worked, because that device was probed much 
earlier, before the unused clocks are turned off. Your patch changed the 
probe order, so that device is probed later.

> I'm guessing you didn't need to pick up this one?
> https://lore.kernel.org/lkml/20200517173453.157703-1-saravanak@xxxxxxxxxx/

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland