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

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

[Pin-yen Lin]

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>

---

 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 */
-- 
2.44.0.769.g3c40516874-goog

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

[AngeloGioacchino Del Regno]

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
    * Panel resume -> refcount=2, wait -> more vregs, etc

Can you please describe the issue that you're getting?

Cheers,
Angelo

> ---
> 
>   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 */

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

[Pin-yen Lin]

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?

>
> 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 */
>

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

[AngeloGioacchino Del Regno]

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 */
>>