Re: [PATCH] arm64: dts: mediatek: mt8192-asurada: Add off-on-delay-us for pp3300_mipibrdg

From: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
To: Pin-yen Lin <treapking@chromium.org>
Cc: "Matthias Brugger" <matthias.bgg@gmail.com>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
	<devicetree@vger.kernel.org>,
	"moderated list:ARM/Mediatek SoC support"
	<linux-arm-kernel@lists.infradead.org>,
	"moderated list:ARM/Mediatek SoC support"
	<linux-mediatek@lists.infradead.org>,
	"Nícolas F . R . A . Prado" <nfraprado@collabora.com>,
	"open list:ARM/Mediatek SoC support"
	<linux-kernel@vger.kernel.org>,
	"Hsin-Te Yuan" <yuanhsinte@chromium.org>
Subject: Re: [PATCH] arm64: dts: mediatek: mt8192-asurada: Add off-on-delay-us for pp3300_mipibrdg
Date: Tue, 30 Apr 2024 11:52:35 +0200	[thread overview]
Message-ID: <8c4f32db-5c80-4fef-8e8d-76f74d3c6bd4@collabora.com> (raw)
In-Reply-To: <CAEXTbpf2HOQj_AxHGbsgOXVF_HyKttL=z7Mi8QStcmuOS+yN7g@mail.gmail.com>

Il 30/04/24 11:32, Pin-yen Lin ha scritto:
> Hi Angelo,
> 
> On Tue, Apr 30, 2024 at 4:17 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 29/04/24 11:53, Pin-yen Lin ha scritto:
>>> Set off-on-delay-us to 500000 us for pp3300_mipibrdg to make sure it
>>> complies with the panel sequence. Explicit configuration on the
>>> regulator node is required because mt8192-asurada uses the same power
>>> supply for the panel and the anx7625 DP bridge. So powering on/off the
>>> DP bridge could break the power sequence requirement for the panel.
>>>
>>> Fixes: f9f00b1f6b9b ("arm64: dts: mediatek: asurada: Add display regulators")
>>> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
>>>
>>
>> Uhm, there might be more to it - I don't think that this should ever happen.
>>
>> The regulator is refcounted, so...
>>    * Bridge on: panel goes off, but regulator doesn't turn off (refcount=1)
>>      * Panel resume -> sequence respected (refcount=2 -> wait -> more vregs, etc)
>>    * Bridge off: panel is already off (refcount=0)
>>      * Bridge resume -> refcount=1, no panel commands yet
> 
> The off-on-delay could be violated because the bridge driver does not
> check the delay.
> 
>>      * Panel resume -> refcount=2, wait -> more vregs, etc
>>
>> Can you please describe the issue that you're getting?
> 
> The symptom we observed is that the device has a small chance to
> reboot to a black panel, and we think the panel's unprepare delay (the
> time to power down completely) might not be satisfied because the
> bridge doesn't check that when it enables the regulator. Even if the
> regulator is enabled by the panel driver, the delay can also be
> violated in the following sequence:
> 
> * t=0ms, bridge on: panel goes off, but regulator doesn't turn off
> (refcount=1). The .unprepared_time in panel_edp is updated
> * t=300ms, bridge off, regulator goes off (refcount=0)
> * t=600ms, panel on, the panel driver thinks the unprepare delay
> (500ms) is satisfied, but the regulator was disabled 300ms ago.
> 
> Did I miss anything here? Or should I add more detail to the commit message?
> 

Heh, no you didn't miss anything, this time it's just me :-)

If you can please add that description to the commit message for a v2 that'd be
appreciated on my side.

In any case

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

>>
>> Cheers,
>> Angelo
>>
> Regards,
> Pin-yen
> 
>>> ---
>>>
>>>    arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
>>> index 7a704246678f..08d71ddf3668 100644
>>> --- a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
>>> @@ -147,6 +147,7 @@ pp3300_mipibrdg: regulator-3v3-mipibrdg {
>>>                regulator-boot-on;
>>>                gpio = <&pio 127 GPIO_ACTIVE_HIGH>;
>>>                vin-supply = <&pp3300_g>;
>>> +             off-on-delay-us = <500000>;
>>>        };
>>>
>>>        /* separately switched 3.3V power rail */
>>