Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 03/12/21 14:14, Dmitry Baryshkov ha scritto:
> On 03/12/2021 13:43, AngeloGioacchino Del Regno wrote:
>> Il 01/12/21 21:20, Dmitry Baryshkov ha scritto:
>>> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
>>> DSI host gets initialized earlier, but this caused unability to probe
>>> the entire stack of components because they all depend on interrupts
>>> coming from the main `mdss` node (mdp5, or dpu1).
>>>
>>> To fix this issue, move mdss device initialization (which include irq
>>> domain setup) to msm_mdev_probe() time, as to make sure that the
>>> interrupt controller is available before dsi and/or other components try
>>> to initialize, finally satisfying the dependency.
>>>
>>> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
>>> Co-Developed-By: AngeloGioacchino Del Regno 
>>> <angelogioacchino.delregno@collabora.com>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>
>>> When checking your patch, I noticed that IRQ domain is created before
>>> respective MDSS clocks are enabled. This does not look like causing any
>>> issues at this time, but it did not look good. So I started moving
>>> clocks parsing to early_init() callbacks. And at some point it looked
>>> like we can drop the init()/destroy() callbacks in favour of
>>> early_init() and remove(). Which promted me to move init()/destroy() in
>>> place of early_init()/remove() with few minor fixes here and there.
>>>
>>
>>
>> Hey Dmitry,
>> I wanted to make the least amount of changes to Rob's logic... I know that
>> the clocks aren't up before registering the domain, but my logic was implying
>> that if the handlers aren't registered, then there's no interrupt coming, hence
>> no risk of getting issues. Same if the hardware is down, you can't get any
>> interrupt, because it can't generate any (but if bootloader leaves it up.. eh.)
> 
> We can get spurious interrupts for any reason, which puts us at risk of peeking 
> into unpowered registers. So, while your approach was working, it did not seem 
> fully correct.
> 

Yeah, that's right and I totally agree.

>>
>> I recognize that such approach is "fragile enough", lastly, I agree with this
>> patch which is, in the end, something that is in the middle between my first
>> and last approach.
>>
>> I've tested this one on trogdor-lazor-limozeen and seems to be working in an
>> analogous way to my v2/v3, so on my side it's validated.
>>
>>
>> Let's go for this one!
>> How do we proceeed now? Are you sending a new series with the new patches, or
>> should I?
> 
> I'll run a few more tests and then I'll probably include both patches into the next 
> series to be sent to Rob.
> 

That's perfect!

>>
>> Also, I don't think this is relevant, since I'm in co-dev, but in case it is:
>> Tested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>
>> P.S.: Sorry for the 1-day delay, got busy with other tasks!
> 
> No problem, it was just single day, no worries.
> 

Alright, thank you! :D