Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add enable delay property

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 09/12/2022 10:50, Krzysztof Kozlowski wrote:
> On 09/12/2022 10:36, Alexander Stein wrote:
>> Hello Krzysztof,
>>
>> Am Freitag, 9. Dezember 2022, 10:07:45 CET schrieb Krzysztof Kozlowski:
>>> On 09/12/2022 09:54, Alexander Stein wrote:
>>>> Hello Krzysztof,
>>>>
>>>> thanks for the fast feedback.
>>>>
>>>> Am Freitag, 9. Dezember 2022, 09:39:49 CET schrieb Krzysztof Kozlowski:
>>>>> On 09/12/2022 09:33, Alexander Stein wrote:
>>>>>> It takes some time until the enable GPIO has settled when turning on.
>>>>>> This delay is platform specific and may be caused by e.g. voltage
>>>>>> shifts, capacitors etc.
>>>>>>
>>>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>>>>>> ---
>>>>>>
>>>>>>  .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml      | 4 ++++
>>>>>>  1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
>>>>>> b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
>>>>>> index 48a97bb3e2e0d..3f50d497cf8ac 100644
>>>>>> --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
>>>>>>
>>>>>> @@ -32,6 +32,10 @@ properties:
>>>>>>      maxItems: 1
>>>>>>      description: GPIO specifier for bridge_en pin (active high).
>>>>>>
>>>>>> +  ti,enable-delay-us:
>>>>>> +    default: 10000
>>>>>> +    description: Enable time delay for enable-gpios
>>>>>
>>>>> Aren't you now mixing two separate delays? One for entire block on (I
>>>>> would assume mostly fixed delay) and one depending on regulators
>>>>> (regulator-ramp-delay, regulator-enable-ramp-delay). Maybe you miss the
>>>>> second delays in your power supply? If so, the first one might be fixed
>>>>> and hard-coded in the driver?
>>>>
>>>> Apparently there are two different delays: reset time (t_reset) of 10ms as
>>>> specified by datasheet. This is already ensured by a following delay after
>>>> requesting enable_gpio as low and switching the GPIO to low in disable
>>>> path.
>>>>
>>>> When enabling this GPIO it takes some time until it is valid on the chip,
>>>> this is what this series is about. It's highly platform specific.
>>>>
>>>> Unfortunately this is completely unrelated to the vcc-supply regulator.
>>>> This one has to be enabled before the enable GPIO can be enabled. So
>>>> there is no regulator-ramp-delay.
>>>
>>> Your driver does one after another - regulator followed immediately by
>>> gpio - so this as well can be a delay from regulator (maybe not ramp but
>>> enable delay).
>>
>> But this will introduce a section which must not be interrupted or delayed. 
>> This is impossible as the enable gpio is attached to an i2c expander in my 
>> case.
>>
>> Given the following time chart:
>>
>>  vcc                  set             EN
>> enable               GPIO             PAD
>>   |                    |               |
>>   |                    |<-- t_raise -->|
>>   | <-- t_vcc_gpio --> |               |
>>   | <--        t_enable_delay      --> |
>>
>> t_raise is the time from changing the GPIO output at the expander until 
>> voltage on the EN (input) pad from the bridge has reached high voltage level.
>> This is an electrical characteristic I can not change and have to take into 
>> account.
>> t_vcc_gpio is the time from enabling supply voltage to enabling the bridge 
>> (removing from reset). Minimum t_vcc_gpio is something which can be addressed 
>> by the regulator and is no problem so far. But there is no upper bound to it.
>>
>> If I understand you correctly, you want to specify t_enable_delay in a 
>> regulator property. This only works if you can upper bound t_vcc_gpio which is 
>> not possible due to e.g. scheduling and i2c bus contention.
>>
>> IMHO that's why there needs to be an configurable delay in the bridge driver.
> 
> What I am saying that you might be here mixing two separate delays:
> regulator enable and/or ramp delay (which more or less matches your
> t_vcc_gpio) and t_raise. I don't know about which board we talk, but the
> mainline users of this bridge do not have even regulator supply,
> therefore its enable time might be not factored.
> 
> Why this all raising questions? Because only your t_vcc_gpio should be
> board dependent, right? Your bridge has fixed internal delays - from
> datasheet: ten, tdis and treset. Nothing in your device is board
> specific, thus I assume any enable delay is coming from power supply.
> 
> Probably experiment to prove it would be to keep power supply enabled
> always and check on the scope of EN pin.
> 
> Anyway, even if this is variable delay on your EN input pin, it is still
> input to the device and based on your time-diagram it is not a property
> of the device. Property of the device could be:
> 
> EN-pad goes high <--------------> output pins stable
> 
> which is either:
> 1. ten already described in datasheet,
> 2. not the case here.


Ah, I forgot, otherwise this is a generic property of every system not
specific to your bridge. Thus it should be modeled somewhere else or in
in generic way.

Best regards,
Krzysztof