mbox series

[0/2] TI SN65DSI83 GPIO enable delay support

Message ID 20221209083339.3780776-1-alexander.stein@ew.tq-group.com
Headers show
Series TI SN65DSI83 GPIO enable delay support | expand

Message

Alexander Stein Dec. 9, 2022, 8:33 a.m. UTC
Hi all,

this small series adds a new optional property for specifying a platform
spefic enable delay. The LVDS Bridge requires at least a reset time of
10ms, but this is just the low pulse width. The actual rising time is a
different matter and is highly platform specific. My platform has a rising
time of ~110ms until the SN signal reaches VCC x 0.7 voltage level. Thus
make this time platform configurable.

Alexander Stein (2):
  dt-bindings: drm/bridge: ti-sn65dsi83: Add enable delay property
  drm: bridge: ti-sn65dsi83: Add enable delay support

 .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml   | 4 ++++
 drivers/gpu/drm/bridge/ti-sn65dsi83.c                      | 7 ++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart Dec. 11, 2022, 6:50 p.m. UTC | #1
Hi Alexander,

On Fri, Dec 09, 2022 at 01:21:36PM +0100, Alexander Stein wrote:
> Am Freitag, 9. Dezember 2022, 13:02:10 CET schrieb Marek Vasut:
> > On 12/9/22 10:36, Alexander Stein wrote:
> > > Am Freitag, 9. Dezember 2022, 10:07:45 CET schrieb Krzysztof Kozlowski:
> > >> On 09/12/2022 09:54, Alexander Stein wrote:
> > >>> 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).
> > 
> > The chip has two separate input pins:
> > 
> > VCC -- power supply that's regulator
> > EN -- reset line, that's GPIO
> > 
> > Alexander is talking about EN line here.
> > 
> > > 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.
> >
> > What exactly is your EN signal rise time (should be ns or so)? Can you
> > look at that with a scope , maybe even with relation to the VCC regulator ?
> 
> I checked EN rise time using a scope, it's ~110ms. I not an expert in hardware 
> but on the mainboard there is some capacitor attached to this line, which 
> increased the time, independent from the internal pull-up.

This is board-specific, and not a property of the SND65DSI83. If the
same circuit was attached to any control input of any chip, you would
need to modify the corresponding driver in a similar way. I don't think
this is right.

How about adding ramp-up (and ramp-down I suppose) delay DT properties
to the GPIO provider instead ? This wouldn't scale very well if all GPIO
providers had to be patched, but with some luck it may be possible to
handle that in the GPIO core ?

Another option would be to add a "GPIO delay" node in DT, between the
GPIO provider and consumer. It could be handled with a small driver that
forwards the GPIO calls with a delay.

> > The DSI84 EN pin already has a built-in pullup per DSI84 datasheet (see
> > Table 5-1. Pin Functions), so that should make the signal rise fast,
> > certainly not for seconds.
> 
> Here it is >100ms, so the current waiting time is far too less. This results 
> in errors regarding PLL lock failure.
Frieder Schrempf Dec. 12, 2022, 9:09 a.m. UTC | #2
On 09.12.22 15:49, Marek Vasut wrote:
> On 12/9/22 14:38, Alexander Stein wrote:
>> Am Freitag, 9. Dezember 2022, 13:43:02 CET schrieb Marek Vasut:
>>> On 12/9/22 13:21, Alexander Stein wrote:
>>>> Hi Marek,
>>>>
>>>> Am Freitag, 9. Dezember 2022, 13:02:10 CET schrieb Marek Vasut:
>>>>> On 12/9/22 10:36, Alexander Stein wrote:
>>>>>> Hello Krzysztof,
>>>>>
>>>>> Hi,
>>>>>
>>>>>> 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).
>>>>>
>>>>> The chip has two separate input pins:
>>>>>
>>>>> VCC -- power supply that's regulator
>>>>> EN -- reset line, that's GPIO
>>>>>
>>>>> Alexander is talking about EN line here.
>>>>>
>>>>>> 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.
>>>>>
>>>>> What exactly is your EN signal rise time (should be ns or so)? Can you
>>>>> look at that with a scope , maybe even with relation to the VCC
>>>>> regulator
>>>>> ?
>>>>
>>>> I checked EN rise time using a scope, it's ~110ms. I not an expert in
>>>> hardware but on the mainboard there is some capacitor attached to this
>>>> line, which increased the time, independent from the internal pull-up.
>>>
>>> This does seem like a hardware bug right there, can you double-check
>>> this with the hardware engineer ?
>>
>> Yep, checked with hardware engineer. An 470nF is attached, together
>> with an
>> open drain output and only the internal pull-up. So yes ~113ms rising
>> time
>> until 0.7 x VCC.
> 
> I don't suppose you can have that capacitor reduced or better yet, some
> external pull up added, can you ?

Actually our HW engineers have implemented a similar RC circuit to
provide a hardware delay for the EN signal. I think this is due to a
design note in the datasheet (see chapter 7.4.1) and therefore it's
probably widely spread.
Marek Vasut Dec. 12, 2022, 12:50 p.m. UTC | #3
On 12/12/22 13:29, Frieder Schrempf wrote:
> On 12.12.22 10:23, Krzysztof Kozlowski wrote:
>> On 12/12/2022 10:09, Frieder Schrempf wrote:
>>>>>> This does seem like a hardware bug right there, can you double-check
>>>>>> this with the hardware engineer ?
>>>>>
>>>>> Yep, checked with hardware engineer. An 470nF is attached, together
>>>>> with an
>>>>> open drain output and only the internal pull-up. So yes ~113ms rising
>>>>> time
>>>>> until 0.7 x VCC.
>>>>
>>>> I don't suppose you can have that capacitor reduced or better yet, some
>>>> external pull up added, can you ?
>>>
>>> Actually our HW engineers have implemented a similar RC circuit to
>>> provide a hardware delay for the EN signal. I think this is due to a
>>> design note in the datasheet (see chapter 7.4.1) and therefore it's
>>> probably widely spread.
>>
>> If I read section 7.4.1 correctly, it would be enough to just add delay
>> Ten=1ms instead of the capacitor, right? And that would be
>> device-specific. But if one chooses the capacitor solution, it becomes
>> now board specific.
> 
> Yes, seems like that's the case.

Can you still fix the board instead ? It would even save you on BOM.