[prev in list] [next in list] [prev in thread] [next in thread] 

List:       dri-devel
Subject:    Re: [PATCH] drm/panel: simple: Initialize unprepared_time in probe
From:       Marek Vasut <marex () denx ! de>
Date:       2023-07-31 21:53:57
Message-ID: cf4e61a8-c3f5-0ba9-d7b4-c48f1ef0985e () denx ! de
[Download RAW message or body]

On 7/31/23 23:34, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jul 31, 2023 at 2:15 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 7/31/23 21:50, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Mon, Jul 31, 2023 at 11:03 AM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 7/24/23 15:49, Doug Anderson wrote:
>>>>
>>>> Hi,
>>>>
>>>> [...]
>>>>
>>>>>> Maybe the EPROBE_DEFER actually happens and triggers the failure ?
>>>>>
>>>>> I could certainly believe that EPROBE_DEFER is involved.
>>>>
>>>> So no, it is not. It is difficult to set this up and access the signals,
>>>> but so I did.
>>>>
>>>> What happens is this:
>>>> panel_simple_probe() calls devm_regulator_get()
>>>>      -> If the regulator was ENABLED, then it is now DISABLED
>>>
>>> As per my previous response, I don't think this is true.
>>
>> I just measured that with a scope on actual hardware .
>>
>> reg_fixed_voltage_probe() is the code which turns the regulator OFF,
>> specifically in the part gpiod_get_optional(...GPIOD_OUT_LOW);
> 
> Great, at least we're on the same page here now.
> 
> 
>>> So just as a point of order, do you agree that prior to the commit
>>> that you are "Fixing" that we _weren't_ doing the full delay at probe
>>> time? That means that if things worked before they were working by
>>> some amount of luck, right? The old code used to do a delay after
>>> turning _off_ the regulator (at unprepare time).
>>
>> Yes, that's well possible.
> 
> ...so assuming we agree that this is _not_ a regression introduced by
> e5e30dfcf3db ("drm: panel: simple: Defer unprepare delay till next
> prepare to shorten it"), that means that all other preexisting users
> of panel-simple were fine with the old behavior

No, it does not mean that all the previous users were fine with that 
behavior. It means the driver operates panels out of spec, we cannot 
know which ones, but we do know it does. Whether users noticed that 
defect or not is another question, which we cannot answer.

> where the unprepare
> delay was only enforced when the panel driver itself turned things off
> and then back on and no extra delay was needed at probe. The one board
> we know about that has a problem with this behavior is the one you're
> reproducing on, which we think only worked previously by chance.

There is at least one device now which shows a problem with the current 
state of driver.

>>> I will also continue to assert that trying to fix the problem via a
>>> delay in the probe of the panel code is the wrong place to fix the
>>> code. The problem is that you need a board-level constraint on this
>>> regulator (off-on-delay-us) preventing it from turning on again within
>>> a certain amount of time after it is turned off. This allows the
>>> regulator framework, which is what decided to turn this rail off
>>> during the regulator probe, to enforce this constraint.
>>
>> No, the driver must respect the unprepare delay, that is what is
>> currently not happening. Trying to somehow work around that by adding
>> random changes to DT is not going to fix the fact that panel-simple is
>> not respecting its own internal built-in constraints.
> 
> Well, except that the panel _isn't_ actually unpreparing the panel.
> The constant you're talking about is a delay between unpreparing the
> panel and then preparing it again and we're not doing that here. Here,
> you are trying to account for an implicit unprepare that happened
> outside the context of the panel driver (in the regulator framework).
> The regulator framework is the one disabling the regulator on its own
> and without the knowledge of the panel driver.

I agree until this point.

> The problem should be
> addressed at the regulator framework, which might involve the
> off-on-delay.

I disagree with this point.

> I would even go further and say that, perhaps, when the regulator
> framework turns this regulator off at bootup then you might be
> violating the power sequencing requirements in the panel anyway. If
> the bootloader left the panel on and _also_ left an enable GPIO on
> then it's entirely possible that you've got a power leak through the
> enable GPIO where you're backpowering the panel's logic when the
> regulator framework turns things off. This is something that would be
> impossible for the panel driver to solve because the panel driver
> hasn't even probed yet.

I agree with this part as well.

> In any case, at this point it seems unlikely that I'll convince you or
> that you'll convince me. I wonder if it's time for someone else on
> this thread to provide an opinion.

I agree with this part as well.
[prev in list] [next in list] [prev in thread] [next in thread]