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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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.

> 
> 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.

> 
> 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.

-- 
With best wishes
Dmitry