[PATCH v3 0/2] "no-hpd" support in CDNS DP bridge driver

[PATCH v3 0/2] "no-hpd" support in CDNS DP bridge driver

[Harikrishna Shenoy]

In J721s2 EVM, DP0 HPD is not connected to correct HPD pin on SOC
which results in HPD detect as always connnected, so when display
is not connected driver continuously retries to read EDID and DPCD
registers.

To handle such cases add support for no hpd configuration in
cdns-mhdp driver.

Changelog v2->v3:
- update the mhdp->plugged status for no hpd configuration in
cdns_mhdp_bridge_detect()

Link to v2: 
<https://lore.kernel.org/all/20230406015207.GO9915@pendragon.ideasonboard.com/>

Rahul T R (2):
  dt-bindings: drm/bridge: Add no-hpd property
  drm: bridge: cdns-mhdp8546: Add support for no-hpd

 .../display/bridge/cdns,mhdp8546.yaml         |  6 +++
 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 54 ++++++++++++++++---
 .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  1 +
 3 files changed, 55 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] dt-bindings: drm/bridge: Add no-hpd property

[Harikrishna Shenoy]

From: Rahul T R <r-ravikumar@ti.com>

The mhdp bridge can work without its HPD pin hooked up to the connector,
but the current bridge driver throws an error when hpd line is not
connected to the connector. For such cases, we need an indication for
no-hpd, using which we can bypass the hpd detection and instead use the
auxiliary channels connected to the DP connector to confirm the
connection.
So add no-hpd property to the bindings, to disable hpd when not
connected or unusable due to DP0-HPD not connected to correct HPD 
pin on SOC like in case of J721S2.

Signed-off-by: Rahul T R <r-ravikumar@ti.com>
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
---
 .../devicetree/bindings/display/bridge/cdns,mhdp8546.yaml   | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
index c2b369456e4e..3a6c6d837593 100644
--- a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
@@ -57,6 +57,12 @@ properties:
   interrupts:
     maxItems: 1
 
+  cdns,no-hpd:
+    type: boolean
+    description:
+      Set if the HPD line on the bridge isn't hooked up to anything or is
+      otherwise unusable.
+
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
 
-- 
2.34.1

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: Add no-hpd property

[Krzysztof Kozlowski]

On 05/02/2025 12:50, Harikrishna Shenoy wrote:
> From: Rahul T R <r-ravikumar@ti.com>
> 
> The mhdp bridge can work without its HPD pin hooked up to the connector,
> but the current bridge driver throws an error when hpd line is not
> connected to the connector. For such cases, we need an indication for
> no-hpd, using which we can bypass the hpd detection and instead use the
> auxiliary channels connected to the DP connector to confirm the
> connection.
> So add no-hpd property to the bindings, to disable hpd when not
> connected or unusable due to DP0-HPD not connected to correct HPD 
> pin on SOC like in case of J721S2.
> 
> Signed-off-by: Rahul T R <r-ravikumar@ti.com>

Why are you sending over and over the same? You already got feedback.
Then you send v2. You got the same feedback.

Now you send v3?

So the same feedback, but this time: NAK

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: Add no-hpd property

[Dmitry Baryshkov]

On Wed, Feb 05, 2025 at 12:52:52PM +0100, Krzysztof Kozlowski wrote:
> On 05/02/2025 12:50, Harikrishna Shenoy wrote:
> > From: Rahul T R <r-ravikumar@ti.com>
> > 
> > The mhdp bridge can work without its HPD pin hooked up to the connector,
> > but the current bridge driver throws an error when hpd line is not
> > connected to the connector. For such cases, we need an indication for
> > no-hpd, using which we can bypass the hpd detection and instead use the
> > auxiliary channels connected to the DP connector to confirm the
> > connection.
> > So add no-hpd property to the bindings, to disable hpd when not
> > connected or unusable due to DP0-HPD not connected to correct HPD 
> > pin on SOC like in case of J721S2.
> > 
> > Signed-off-by: Rahul T R <r-ravikumar@ti.com>
> 
> Why are you sending over and over the same? You already got feedback.
> Then you send v2. You got the same feedback.
> 
> Now you send v3?
> 
> So the same feedback, but this time: NAK

Krzysztof's email forced me to take a look at the actual boards that you
are trying to enable. I couldn't stop by notice that the HPD signal
_is_ connected to a GPIO pin. Please stop hacking the bridge driver and
use the tools that are already provided to you: add the HPD pin to the
dp-controller device node. And then fix any possible issues coming from
the bridge driver not being able to handle HPD signals being delivered
by the DRM framework via the .hpd_notify() callback.

TL;DR: also a NAK from my side, add HPD gpio to dp-controller.

-- 
With best wishes
Dmitry

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: Add no-hpd property

[Harikrishna Shenoy]

On 05/02/25 19:03, Dmitry Baryshkov wrote:
> On Wed, Feb 05, 2025 at 12:52:52PM +0100, Krzysztof Kozlowski wrote:
>> On 05/02/2025 12:50, Harikrishna Shenoy wrote:
>>> From: Rahul T R <r-ravikumar@ti.com>
>>>
>>> The mhdp bridge can work without its HPD pin hooked up to the connector,
>>> but the current bridge driver throws an error when hpd line is not
>>> connected to the connector. For such cases, we need an indication for
>>> no-hpd, using which we can bypass the hpd detection and instead use the
>>> auxiliary channels connected to the DP connector to confirm the
>>> connection.
>>> So add no-hpd property to the bindings, to disable hpd when not
>>> connected or unusable due to DP0-HPD not connected to correct HPD
>>> pin on SOC like in case of J721S2.
>>>
>>> Signed-off-by: Rahul T R <r-ravikumar@ti.com>
>>
>> Why are you sending over and over the same? You already got feedback.
>> Then you send v2. You got the same feedback.
>>
>> Now you send v3?
>>
>> So the same feedback, but this time: NAK
> 
> Krzysztof's email forced me to take a look at the actual boards that you
> are trying to enable. I couldn't stop by notice that the HPD signal
> _is_ connected to a GPIO pin. Please stop hacking the bridge driver and
> use the tools that are already provided to you: add the HPD pin to the
> dp-controller device node. And then fix any possible issues coming from
> the bridge driver not being able to handle HPD signals being delivered
> by the DRM framework via the .hpd_notify() callback.
> 
> TL;DR: also a NAK from my side, add HPD gpio to dp-controller.
> 
We tried implementing a interrupt based HPD functionality as HPD signal 
is connected to GPIO0_18 pin, we were able to get interrupt based HPD 
working however to route this signal to SoC we are loosing audio 
capability due to MUX conflict. Due to board level limitations to
route the signal to SoC, we will not be able to support interrupt
based HPD and polling seems a possible way without loosing on audio 
capability.

Link to schematics zip:
https://www.ti.com/tool/J721S2XSOMXEVM#design-files
File:sprr439b/PROC118E4_RP/PROC118E4(001)_SCH.pdf, Page 17, MUX1

Regards,
Hari

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: Add no-hpd property

[Dmitry Baryshkov]

On Wed, Mar 12, 2025 at 11:56:41AM +0530, Harikrishna Shenoy wrote:
> 
> 
> On 05/02/25 19:03, Dmitry Baryshkov wrote:
> > On Wed, Feb 05, 2025 at 12:52:52PM +0100, Krzysztof Kozlowski wrote:
> > > On 05/02/2025 12:50, Harikrishna Shenoy wrote:
> > > > From: Rahul T R <r-ravikumar@ti.com>
> > > > 
> > > > The mhdp bridge can work without its HPD pin hooked up to the connector,
> > > > but the current bridge driver throws an error when hpd line is not
> > > > connected to the connector. For such cases, we need an indication for
> > > > no-hpd, using which we can bypass the hpd detection and instead use the
> > > > auxiliary channels connected to the DP connector to confirm the
> > > > connection.
> > > > So add no-hpd property to the bindings, to disable hpd when not
> > > > connected or unusable due to DP0-HPD not connected to correct HPD
> > > > pin on SOC like in case of J721S2.
> > > > 
> > > > Signed-off-by: Rahul T R <r-ravikumar@ti.com>
> > > 
> > > Why are you sending over and over the same? You already got feedback.
> > > Then you send v2. You got the same feedback.
> > > 
> > > Now you send v3?
> > > 
> > > So the same feedback, but this time: NAK
> > 
> > Krzysztof's email forced me to take a look at the actual boards that you
> > are trying to enable. I couldn't stop by notice that the HPD signal
> > _is_ connected to a GPIO pin. Please stop hacking the bridge driver and
> > use the tools that are already provided to you: add the HPD pin to the
> > dp-controller device node. And then fix any possible issues coming from
> > the bridge driver not being able to handle HPD signals being delivered
> > by the DRM framework via the .hpd_notify() callback.
> > 
> > TL;DR: also a NAK from my side, add HPD gpio to dp-controller.
> > 
> We tried implementing a interrupt based HPD functionality as HPD signal is
> connected to GPIO0_18 pin, we were able to get interrupt based HPD working
> however to route this signal to SoC we are loosing audio capability due to
> MUX conflict. Due to board level limitations to
> route the signal to SoC, we will not be able to support interrupt
> based HPD and polling seems a possible way without loosing on audio
> capability.

Still NAK for the no-hpd property. HPD pin is a requirement for
DisplayPort to work, as it is used e.g. for the 'attention' IRQs being
sent by the DP sink. I'm not sure what kind of idea you HW engineers had
in mind.

> Link to schematics zip:
> https://www.ti.com/tool/J721S2XSOMXEVM#design-files
> File:sprr439b/PROC118E4_RP/PROC118E4(001)_SCH.pdf, Page 17, MUX1

-- 
With best wishes
Dmitry

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: Add no-hpd property

[Tomi Valkeinen]

Hi,

On 12/03/2025 14:52, Dmitry Baryshkov wrote:
> On Wed, Mar 12, 2025 at 11:56:41AM +0530, Harikrishna Shenoy wrote:
>>
>>
>> On 05/02/25 19:03, Dmitry Baryshkov wrote:
>>> On Wed, Feb 05, 2025 at 12:52:52PM +0100, Krzysztof Kozlowski wrote:
>>>> On 05/02/2025 12:50, Harikrishna Shenoy wrote:
>>>>> From: Rahul T R <r-ravikumar@ti.com>
>>>>>
>>>>> The mhdp bridge can work without its HPD pin hooked up to the connector,
>>>>> but the current bridge driver throws an error when hpd line is not
>>>>> connected to the connector. For such cases, we need an indication for
>>>>> no-hpd, using which we can bypass the hpd detection and instead use the
>>>>> auxiliary channels connected to the DP connector to confirm the
>>>>> connection.
>>>>> So add no-hpd property to the bindings, to disable hpd when not
>>>>> connected or unusable due to DP0-HPD not connected to correct HPD
>>>>> pin on SOC like in case of J721S2.
>>>>>
>>>>> Signed-off-by: Rahul T R <r-ravikumar@ti.com>
>>>>
>>>> Why are you sending over and over the same? You already got feedback.
>>>> Then you send v2. You got the same feedback.
>>>>
>>>> Now you send v3?
>>>>
>>>> So the same feedback, but this time: NAK
>>>
>>> Krzysztof's email forced me to take a look at the actual boards that you
>>> are trying to enable. I couldn't stop by notice that the HPD signal
>>> _is_ connected to a GPIO pin. Please stop hacking the bridge driver and
>>> use the tools that are already provided to you: add the HPD pin to the
>>> dp-controller device node. And then fix any possible issues coming from
>>> the bridge driver not being able to handle HPD signals being delivered
>>> by the DRM framework via the .hpd_notify() callback.
>>>
>>> TL;DR: also a NAK from my side, add HPD gpio to dp-controller.
>>>
>> We tried implementing a interrupt based HPD functionality as HPD signal is
>> connected to GPIO0_18 pin, we were able to get interrupt based HPD working
>> however to route this signal to SoC we are loosing audio capability due to
>> MUX conflict. Due to board level limitations to
>> route the signal to SoC, we will not be able to support interrupt
>> based HPD and polling seems a possible way without loosing on audio
>> capability.
> 
> Still NAK for the no-hpd property. HPD pin is a requirement for
> DisplayPort to work, as it is used e.g. for the 'attention' IRQs being
> sent by the DP sink. I'm not sure what kind of idea you HW engineers had
> in mind.

It's true that for normal DP functionality the HPD is required, but 
afaik DP works "fine" without HPD too. This is not the first board that 
has DP connector, but doesn't have HPD, that I have seen or worked on. 
Polling can be used for the IRQs too.

For eDP HPD is optional, and some of the cases I've worked with involved 
a chip intended for eDP, but used with a full DP connector, and no HPD. 
However, in this particular case the DP chip supports full DP, so it's 
just a board design error.

My question is, is J721s2 EVM something that's used widely? Or is it a 
rare board? If it's a rare one, maybe there's no point in solving this 
in upstream? But if it's widely used, I don't see why we wouldn't 
support it in upstream. The HW is broken, but we need to live with it.

Another question is, if eDP support is added to the cdns-mhdp driver, 
and used with a panel that doesn't have an HPD, how would that code look 
like? If that would be solved with a "no-hpd" property, identical to the 
one proposed in this series, then... There's even less reason to not 
support this.

Disclaimer: I didn't study the schematics, and I haven't thought or 
looked at how eDP is implemented in other drm drivers.

  Tomi

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: Add no-hpd property

[Doug Anderson]

Hi,

On Tue, Mar 18, 2025 at 8:50 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> Hi,
>
> On 12/03/2025 14:52, Dmitry Baryshkov wrote:
> > On Wed, Mar 12, 2025 at 11:56:41AM +0530, Harikrishna Shenoy wrote:
> >>
> >>
> >> On 05/02/25 19:03, Dmitry Baryshkov wrote:
> >>> On Wed, Feb 05, 2025 at 12:52:52PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 05/02/2025 12:50, Harikrishna Shenoy wrote:
> >>>>> From: Rahul T R <r-ravikumar@ti.com>
> >>>>>
> >>>>> The mhdp bridge can work without its HPD pin hooked up to the connector,
> >>>>> but the current bridge driver throws an error when hpd line is not
> >>>>> connected to the connector. For such cases, we need an indication for
> >>>>> no-hpd, using which we can bypass the hpd detection and instead use the
> >>>>> auxiliary channels connected to the DP connector to confirm the
> >>>>> connection.
> >>>>> So add no-hpd property to the bindings, to disable hpd when not
> >>>>> connected or unusable due to DP0-HPD not connected to correct HPD
> >>>>> pin on SOC like in case of J721S2.
> >>>>>
> >>>>> Signed-off-by: Rahul T R <r-ravikumar@ti.com>
> >>>>
> >>>> Why are you sending over and over the same? You already got feedback.
> >>>> Then you send v2. You got the same feedback.
> >>>>
> >>>> Now you send v3?
> >>>>
> >>>> So the same feedback, but this time: NAK

I only spent a few minutes on it, but I couldn't find a v2. If there's
a link I'm happy to read it, but otherwise all my comments below are
without any context from prior verisons...


> >>> Krzysztof's email forced me to take a look at the actual boards that you
> >>> are trying to enable. I couldn't stop by notice that the HPD signal
> >>> _is_ connected to a GPIO pin. Please stop hacking the bridge driver and
> >>> use the tools that are already provided to you: add the HPD pin to the
> >>> dp-controller device node. And then fix any possible issues coming from
> >>> the bridge driver not being able to handle HPD signals being delivered
> >>> by the DRM framework via the .hpd_notify() callback.
> >>>
> >>> TL;DR: also a NAK from my side, add HPD gpio to dp-controller.
> >>>
> >> We tried implementing a interrupt based HPD functionality as HPD signal is
> >> connected to GPIO0_18 pin, we were able to get interrupt based HPD working
> >> however to route this signal to SoC we are loosing audio capability due to
> >> MUX conflict. Due to board level limitations to
> >> route the signal to SoC, we will not be able to support interrupt
> >> based HPD and polling seems a possible way without loosing on audio
> >> capability.
> >
> > Still NAK for the no-hpd property. HPD pin is a requirement for
> > DisplayPort to work, as it is used e.g. for the 'attention' IRQs being
> > sent by the DP sink. I'm not sure what kind of idea you HW engineers had
> > in mind.
>
> It's true that for normal DP functionality the HPD is required, but
> afaik DP works "fine" without HPD too. This is not the first board that
> has DP connector, but doesn't have HPD, that I have seen or worked on.
> Polling can be used for the IRQs too.

I have less familiarity with DP than with eDP, but from what I know
I'd agree with Tomi here that it would probably work "fine" by some
definition of "fine". As Dmitry says, the "attention" IRQ wouldn't
work, but as I understand it that's not really part of the normal flow
of using DP. As evidence, some people have made "ti-sn65dsi86" (which
is supposed to be for eDP only) work with DP. While the ti-sn65dsi86
hardware _does_ support HPD, because of the forced (slow) debouncing
it turned out not to be terribly useful for eDP and we designed our
boards to route HPD to a GPIO. ...and because of that nobody ever
wrote the code to handle the "attention" IRQ. Apparently people are
still using this bridge w/ some success on DP monitors.


> For eDP HPD is optional, and some of the cases I've worked with involved
> a chip intended for eDP, but used with a full DP connector, and no HPD.

I definitely agree. The eDP spec explicitly states that HPD is
optional even though it's also documented to be an "attention" IRQ
there. We've hooked up large numbers of eDP panels and the lack of the
attention IRQ wasn't a problem.


> However, in this particular case the DP chip supports full DP, so it's
> just a board design error.
>
> My question is, is J721s2 EVM something that's used widely? Or is it a
> rare board? If it's a rare one, maybe there's no point in solving this
> in upstream? But if it's widely used, I don't see why we wouldn't
> support it in upstream. The HW is broken, but we need to live with it.
>
> Another question is, if eDP support is added to the cdns-mhdp driver,
> and used with a panel that doesn't have an HPD, how would that code look
> like? If that would be solved with a "no-hpd" property, identical to the
> one proposed in this series, then... There's even less reason to not
> support this.
>
> Disclaimer: I didn't study the schematics, and I haven't thought or
> looked at how eDP is implemented in other drm drivers.

I spent lots of time working through this on ti-sn65dsi86. How it
works today (and how it's documented in the bindings) is that it's
possible to specify "no-hpd" on both the eDP panel node and on the
bridge chip. They mean different things.

The HPD-related properties that can be specified on the panel are
a) <nothing> - HPD hooked up to the bridge
b) no-hpd - HPD isn't hooked up at all
c) hpd-gpios - HPD is hooked up to a GPIO

The HPD-related properties that can be specified on ti-sn65dsi86 are:
a) <nothing> - HPD is hooked up to the bridge
b) no-hpd - HPD is not hooked up to the bridge

NOTE: The "ti-sn65dsi86" controller needs to be programmed to ignore
its HPD line if HPD isn't hooked up. IIRC the hardware itself will not
transfer things over the AUX bus unless you either tell the controller
to ignore HPD or HPD is asserted.


Here are the combinations:

1. Panel has no HPD-related properties, ti-sn65dsi86 has no
HPD-related properties

HPD is assumed to be hooked up to the dedicated HPD pin on the bridge.
Panel driver queries the bridge driver to know the status of HPD. In
Linux today ti-sn65dsi86 doesn't really implement this and the bridge
chip just has a big, fixed, non-optimized delay that tries to account
for the max delay any panel could need.

2. Panel has "hpd-gpios", ti-sn65dsi86 has no HPD-related properties

In theory, I guess this would say that HPD goes _both_ to a GPIO and
to the HPD of the bridge. Maybe handy if the bridge doesn't provide a
"debounced" signal but still wants HPD hooked up to get the
"attention" IRQ?

3. Panel has "no-hpd", ti-sn65dsi86 has no HPD-related properties

Doesn't really make sense. Says that panel should delay the max amount
but there's no good reason to do this if HPD is hooked up on
ti-sn65dsi86.

4. Panel has no HPD-related properties, ti-sn65dsi86 has "no-hpd"

Doesn't really make sense. Says that the panel should assume the
bridge has HPD hooked up but then the bridge doesn't.

5. Panel has "hpd-gpios", ti-sn65dsi86 has "no-hpd"

This is the sc7180-trogdor config. Says the panel should use the GPIO
to read HPD for power sequencing purposes. Tells us that HPD is not
hooked up to the bridge chip so we should program the bridge chip to
ignore HPD.

6. Panel has "no-hpd", ti-sn65dsi86 has "no-hpd"

Says HPD is just not hooked up at all. panel-edp will delay for
"hpd-absent-delay-ms". Bridge chip should be programmed to tell the
hardware to ignore the HPD signal.


How we got there was fairly organic and quite a long time ago, but it
all sorta makes sense even if it is a bit convoluted.

-Doug

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: Add no-hpd property

[Tomi Valkeinen]

Hi,

On 18/03/2025 21:51, Doug Anderson wrote:
> Hi,
> 
> On Tue, Mar 18, 2025 at 8:50 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
>> Hi,
>>
>> On 12/03/2025 14:52, Dmitry Baryshkov wrote:
>>> On Wed, Mar 12, 2025 at 11:56:41AM +0530, Harikrishna Shenoy wrote:
>>>>
>>>>
>>>> On 05/02/25 19:03, Dmitry Baryshkov wrote:
>>>>> On Wed, Feb 05, 2025 at 12:52:52PM +0100, Krzysztof Kozlowski wrote:
>>>>>> On 05/02/2025 12:50, Harikrishna Shenoy wrote:
>>>>>>> From: Rahul T R <r-ravikumar@ti.com>
>>>>>>>
>>>>>>> The mhdp bridge can work without its HPD pin hooked up to the connector,
>>>>>>> but the current bridge driver throws an error when hpd line is not
>>>>>>> connected to the connector. For such cases, we need an indication for
>>>>>>> no-hpd, using which we can bypass the hpd detection and instead use the
>>>>>>> auxiliary channels connected to the DP connector to confirm the
>>>>>>> connection.
>>>>>>> So add no-hpd property to the bindings, to disable hpd when not
>>>>>>> connected or unusable due to DP0-HPD not connected to correct HPD
>>>>>>> pin on SOC like in case of J721S2.
>>>>>>>
>>>>>>> Signed-off-by: Rahul T R <r-ravikumar@ti.com>
>>>>>>
>>>>>> Why are you sending over and over the same? You already got feedback.
>>>>>> Then you send v2. You got the same feedback.
>>>>>>
>>>>>> Now you send v3?
>>>>>>
>>>>>> So the same feedback, but this time: NAK
> 
> I only spent a few minutes on it, but I couldn't find a v2. If there's
> a link I'm happy to read it, but otherwise all my comments below are
> without any context from prior verisons...

There was a link in the intro letter, although it seems to point to a 
reply to the v2 thread... Here's v2 intro letter:

https://lore.kernel.org/all/20230405142440.191939-1-j-choudhary@ti.com/

>>>>> Krzysztof's email forced me to take a look at the actual boards that you
>>>>> are trying to enable. I couldn't stop by notice that the HPD signal
>>>>> _is_ connected to a GPIO pin. Please stop hacking the bridge driver and
>>>>> use the tools that are already provided to you: add the HPD pin to the
>>>>> dp-controller device node. And then fix any possible issues coming from
>>>>> the bridge driver not being able to handle HPD signals being delivered
>>>>> by the DRM framework via the .hpd_notify() callback.
>>>>>
>>>>> TL;DR: also a NAK from my side, add HPD gpio to dp-controller.
>>>>>
>>>> We tried implementing a interrupt based HPD functionality as HPD signal is
>>>> connected to GPIO0_18 pin, we were able to get interrupt based HPD working
>>>> however to route this signal to SoC we are loosing audio capability due to
>>>> MUX conflict. Due to board level limitations to
>>>> route the signal to SoC, we will not be able to support interrupt
>>>> based HPD and polling seems a possible way without loosing on audio
>>>> capability.
>>>
>>> Still NAK for the no-hpd property. HPD pin is a requirement for
>>> DisplayPort to work, as it is used e.g. for the 'attention' IRQs being
>>> sent by the DP sink. I'm not sure what kind of idea you HW engineers had
>>> in mind.
>>
>> It's true that for normal DP functionality the HPD is required, but
>> afaik DP works "fine" without HPD too. This is not the first board that
>> has DP connector, but doesn't have HPD, that I have seen or worked on.
>> Polling can be used for the IRQs too.
> 
> I have less familiarity with DP than with eDP, but from what I know
> I'd agree with Tomi here that it would probably work "fine" by some
> definition of "fine". As Dmitry says, the "attention" IRQ wouldn't
> work, but as I understand it that's not really part of the normal flow
> of using DP. As evidence, some people have made "ti-sn65dsi86" (which
> is supposed to be for eDP only) work with DP. While the ti-sn65dsi86
> hardware _does_ support HPD, because of the forced (slow) debouncing
> it turned out not to be terribly useful for eDP and we designed our
> boards to route HPD to a GPIO. ...and because of that nobody ever
> wrote the code to handle the "attention" IRQ. Apparently people are
> still using this bridge w/ some success on DP monitors.
> 
> 
>> For eDP HPD is optional, and some of the cases I've worked with involved
>> a chip intended for eDP, but used with a full DP connector, and no HPD.
> 
> I definitely agree. The eDP spec explicitly states that HPD is
> optional even though it's also documented to be an "attention" IRQ
> there. We've hooked up large numbers of eDP panels and the lack of the
> attention IRQ wasn't a problem.
> 
> 
>> However, in this particular case the DP chip supports full DP, so it's
>> just a board design error.
>>
>> My question is, is J721s2 EVM something that's used widely? Or is it a
>> rare board? If it's a rare one, maybe there's no point in solving this
>> in upstream? But if it's widely used, I don't see why we wouldn't
>> support it in upstream. The HW is broken, but we need to live with it.
>>
>> Another question is, if eDP support is added to the cdns-mhdp driver,
>> and used with a panel that doesn't have an HPD, how would that code look
>> like? If that would be solved with a "no-hpd" property, identical to the
>> one proposed in this series, then... There's even less reason to not
>> support this.
>>
>> Disclaimer: I didn't study the schematics, and I haven't thought or
>> looked at how eDP is implemented in other drm drivers.
> 
> I spent lots of time working through this on ti-sn65dsi86. How it
> works today (and how it's documented in the bindings) is that it's
> possible to specify "no-hpd" on both the eDP panel node and on the
> bridge chip. They mean different things.

As this text covers only eDP with Panel, I'll fill in some lines here 
about DP and HDMI connectors. I think we need to consider all the cases.

> The HPD-related properties that can be specified on the panel are
> a) <nothing> - HPD hooked up to the bridge
> b) no-hpd - HPD isn't hooked up at all
> c) hpd-gpios - HPD is hooked up to a GPIO

For DP and HDMI connectors (dp-connector.yaml, hdmi-connector.yaml) we 
have only 'hpd-gpios'. There hasn't been need for no-hpd.

> The HPD-related properties that can be specified on ti-sn65dsi86 are:
> a) <nothing> - HPD is hooked up to the bridge
> b) no-hpd - HPD is not hooked up to the bridge

More generally speaking (also with HDMI), I think this is device 
specific. E.g. TFP410 doesn't have any kind of HPD support, so 'no-hpd' 
flag doesn't make sense. That said, probably most of the chips do have 
HPD support, and no-hpd is needed.

> NOTE: The "ti-sn65dsi86" controller needs to be programmed to ignore
> its HPD line if HPD isn't hooked up. IIRC the hardware itself will not
> transfer things over the AUX bus unless you either tell the controller
> to ignore HPD or HPD is asserted.
> 
> 
> Here are the combinations:
> 
> 1. Panel has no HPD-related properties, ti-sn65dsi86 has no
> HPD-related properties
> 
> HPD is assumed to be hooked up to the dedicated HPD pin on the bridge.
> Panel driver queries the bridge driver to know the status of HPD. In
> Linux today ti-sn65dsi86 doesn't really implement this and the bridge
> chip just has a big, fixed, non-optimized delay that tries to account
> for the max delay any panel could need.

For the connector case, I don't think there's any assumption about HPD 
in this scenario. The connector does not handle the HPD, and it's up to 
the bridge to decide if it does something about it or not.

> 2. Panel has "hpd-gpios", ti-sn65dsi86 has no HPD-related properties
> 
> In theory, I guess this would say that HPD goes _both_ to a GPIO and
> to the HPD of the bridge. Maybe handy if the bridge doesn't provide a
> "debounced" signal but still wants HPD hooked up to get the
> "attention" IRQ?

Both the bridge and the panel handling HPD doesn't sound good to me...
For the connector case, this case would mean that the connector driver 
handles the HPD, and the bridge doesn't. If the bridge has HPD support, 
I think it would make sense to disable it with 'no-hpd' property (i.e. 
this would then be case 5).

> 3. Panel has "no-hpd", ti-sn65dsi86 has no HPD-related properties
> 
> Doesn't really make sense. Says that panel should delay the max amount
> but there's no good reason to do this if HPD is hooked up on
> ti-sn65dsi86.

The connectors don't have no-hpd, so this doesn't apply there.

> 4. Panel has no HPD-related properties, ti-sn65dsi86 has "no-hpd"
> 
> Doesn't really make sense. Says that the panel should assume the
> bridge has HPD hooked up but then the bridge doesn't.

For connectors, this would just mean no HPD at all connected (i.e. the 
case discussed in this series).

> 5. Panel has "hpd-gpios", ti-sn65dsi86 has "no-hpd"
> 
> This is the sc7180-trogdor config. Says the panel should use the GPIO
> to read HPD for power sequencing purposes. Tells us that HPD is not
> hooked up to the bridge chip so we should program the bridge chip to
> ignore HPD.

For the connector case, this would be the same as 2, except the bridge 
requires disabling the HPD support via a property.

> 6. Panel has "no-hpd", ti-sn65dsi86 has "no-hpd"
> 
> Says HPD is just not hooked up at all. panel-edp will delay for
> "hpd-absent-delay-ms". Bridge chip should be programmed to tell the
> hardware to ignore the HPD signal.

For connectors, this would be the same as 4.

> How we got there was fairly organic and quite a long time ago, but it
> all sorta makes sense even if it is a bit convoluted.

I think it makes sense, and is quite similar for connectors.

Going back to this series, I think the no-hpd property makes sense to 
solve the TI issue.

However, my question about "is this needed in upstream" is still 
unanswered. If these boards are widely available, let's add this. If 
there are just a few boards here and there, with customers who anyway 
use TI BSP kernel, and the next revision of the board has the issue 
fixed, maybe it's not worth it? This change doesn't exactly make the 
driver cleaner or easier to maintain =).

  Tomi

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: Add no-hpd property

[Dmitry Baryshkov]

On Tue, Apr 15, 2025 at 03:50:46PM +0300, Tomi Valkeinen wrote:
> Hi,
> 
> On 18/03/2025 21:51, Doug Anderson wrote:
> > Hi,
> > 
> > On Tue, Mar 18, 2025 at 8:50 AM Tomi Valkeinen
> > <tomi.valkeinen@ideasonboard.com> wrote:
> > > 
> > > Hi,
> > > 
> > > On 12/03/2025 14:52, Dmitry Baryshkov wrote:
> > > > On Wed, Mar 12, 2025 at 11:56:41AM +0530, Harikrishna Shenoy wrote:
> > > > > 
> > > > > 
> > > > > On 05/02/25 19:03, Dmitry Baryshkov wrote:
> > > > > > On Wed, Feb 05, 2025 at 12:52:52PM +0100, Krzysztof Kozlowski wrote:
> > > > > > > On 05/02/2025 12:50, Harikrishna Shenoy wrote:
> > > > > > > > From: Rahul T R <r-ravikumar@ti.com>
> > > > > > > > 
> > > > > > > > The mhdp bridge can work without its HPD pin hooked up to the connector,
> > > > > > > > but the current bridge driver throws an error when hpd line is not
> > > > > > > > connected to the connector. For such cases, we need an indication for
> > > > > > > > no-hpd, using which we can bypass the hpd detection and instead use the
> > > > > > > > auxiliary channels connected to the DP connector to confirm the
> > > > > > > > connection.
> > > > > > > > So add no-hpd property to the bindings, to disable hpd when not
> > > > > > > > connected or unusable due to DP0-HPD not connected to correct HPD
> > > > > > > > pin on SOC like in case of J721S2.
> > > > > > > > 
> > > > > > > > Signed-off-by: Rahul T R <r-ravikumar@ti.com>
> > > > > > > 
> > > > > > > Why are you sending over and over the same? You already got feedback.
> > > > > > > Then you send v2. You got the same feedback.
> > > > > > > 
> > > > > > > Now you send v3?
> > > > > > > 
> > > > > > > So the same feedback, but this time: NAK
> > 
> > I only spent a few minutes on it, but I couldn't find a v2. If there's
> > a link I'm happy to read it, but otherwise all my comments below are
> > without any context from prior verisons...
> 
> There was a link in the intro letter, although it seems to point to a reply
> to the v2 thread... Here's v2 intro letter:
> 
> https://lore.kernel.org/all/20230405142440.191939-1-j-choudhary@ti.com/
> 
> > > > > > Krzysztof's email forced me to take a look at the actual boards that you
> > > > > > are trying to enable. I couldn't stop by notice that the HPD signal
> > > > > > _is_ connected to a GPIO pin. Please stop hacking the bridge driver and
> > > > > > use the tools that are already provided to you: add the HPD pin to the
> > > > > > dp-controller device node. And then fix any possible issues coming from
> > > > > > the bridge driver not being able to handle HPD signals being delivered
> > > > > > by the DRM framework via the .hpd_notify() callback.
> > > > > > 
> > > > > > TL;DR: also a NAK from my side, add HPD gpio to dp-controller.
> > > > > > 
> > > > > We tried implementing a interrupt based HPD functionality as HPD signal is
> > > > > connected to GPIO0_18 pin, we were able to get interrupt based HPD working
> > > > > however to route this signal to SoC we are loosing audio capability due to
> > > > > MUX conflict. Due to board level limitations to
> > > > > route the signal to SoC, we will not be able to support interrupt
> > > > > based HPD and polling seems a possible way without loosing on audio
> > > > > capability.
> > > > 
> > > > Still NAK for the no-hpd property. HPD pin is a requirement for
> > > > DisplayPort to work, as it is used e.g. for the 'attention' IRQs being
> > > > sent by the DP sink. I'm not sure what kind of idea you HW engineers had
> > > > in mind.
> > > 
> > > It's true that for normal DP functionality the HPD is required, but
> > > afaik DP works "fine" without HPD too. This is not the first board that
> > > has DP connector, but doesn't have HPD, that I have seen or worked on.
> > > Polling can be used for the IRQs too.
> > 
> > I have less familiarity with DP than with eDP, but from what I know
> > I'd agree with Tomi here that it would probably work "fine" by some
> > definition of "fine". As Dmitry says, the "attention" IRQ wouldn't
> > work, but as I understand it that's not really part of the normal flow
> > of using DP. As evidence, some people have made "ti-sn65dsi86" (which
> > is supposed to be for eDP only) work with DP. While the ti-sn65dsi86
> > hardware _does_ support HPD, because of the forced (slow) debouncing
> > it turned out not to be terribly useful for eDP and we designed our
> > boards to route HPD to a GPIO. ...and because of that nobody ever
> > wrote the code to handle the "attention" IRQ. Apparently people are
> > still using this bridge w/ some success on DP monitors.
> > 
> > 
> > > For eDP HPD is optional, and some of the cases I've worked with involved
> > > a chip intended for eDP, but used with a full DP connector, and no HPD.
> > 
> > I definitely agree. The eDP spec explicitly states that HPD is
> > optional even though it's also documented to be an "attention" IRQ
> > there. We've hooked up large numbers of eDP panels and the lack of the
> > attention IRQ wasn't a problem.
> > 
> > 
> > > However, in this particular case the DP chip supports full DP, so it's
> > > just a board design error.
> > > 
> > > My question is, is J721s2 EVM something that's used widely? Or is it a
> > > rare board? If it's a rare one, maybe there's no point in solving this
> > > in upstream? But if it's widely used, I don't see why we wouldn't
> > > support it in upstream. The HW is broken, but we need to live with it.
> > > 
> > > Another question is, if eDP support is added to the cdns-mhdp driver,
> > > and used with a panel that doesn't have an HPD, how would that code look
> > > like? If that would be solved with a "no-hpd" property, identical to the
> > > one proposed in this series, then... There's even less reason to not
> > > support this.
> > > 
> > > Disclaimer: I didn't study the schematics, and I haven't thought or
> > > looked at how eDP is implemented in other drm drivers.
> > 
> > I spent lots of time working through this on ti-sn65dsi86. How it
> > works today (and how it's documented in the bindings) is that it's
> > possible to specify "no-hpd" on both the eDP panel node and on the
> > bridge chip. They mean different things.
> 
> As this text covers only eDP with Panel, I'll fill in some lines here about
> DP and HDMI connectors. I think we need to consider all the cases.
> 
> > The HPD-related properties that can be specified on the panel are
> > a) <nothing> - HPD hooked up to the bridge
> > b) no-hpd - HPD isn't hooked up at all
> > c) hpd-gpios - HPD is hooked up to a GPIO
> 
> For DP and HDMI connectors (dp-connector.yaml, hdmi-connector.yaml) we have
> only 'hpd-gpios'. There hasn't been need for no-hpd.
> 
> > The HPD-related properties that can be specified on ti-sn65dsi86 are:
> > a) <nothing> - HPD is hooked up to the bridge
> > b) no-hpd - HPD is not hooked up to the bridge
> 
> More generally speaking (also with HDMI), I think this is device specific.
> E.g. TFP410 doesn't have any kind of HPD support, so 'no-hpd' flag doesn't
> make sense. That said, probably most of the chips do have HPD support, and
> no-hpd is needed.

TFP410 has the EDGE/HTPLG pin, which should be used to monitor DVI /
HDMI hot plugging pin.

> 
> > NOTE: The "ti-sn65dsi86" controller needs to be programmed to ignore
> > its HPD line if HPD isn't hooked up. IIRC the hardware itself will not
> > transfer things over the AUX bus unless you either tell the controller
> > to ignore HPD or HPD is asserted.
> > 
> > 
> > Here are the combinations:
> > 
> > 1. Panel has no HPD-related properties, ti-sn65dsi86 has no
> > HPD-related properties
> > 
> > HPD is assumed to be hooked up to the dedicated HPD pin on the bridge.
> > Panel driver queries the bridge driver to know the status of HPD. In
> > Linux today ti-sn65dsi86 doesn't really implement this and the bridge
> > chip just has a big, fixed, non-optimized delay that tries to account
> > for the max delay any panel could need.
> 
> For the connector case, I don't think there's any assumption about HPD in
> this scenario. The connector does not handle the HPD, and it's up to the
> bridge to decide if it does something about it or not.

Hmm? display-connector definitely supports HPD for DVI, HDMI and DP
connectors.

> 
> > 2. Panel has "hpd-gpios", ti-sn65dsi86 has no HPD-related properties
> > 
> > In theory, I guess this would say that HPD goes _both_ to a GPIO and
> > to the HPD of the bridge. Maybe handy if the bridge doesn't provide a
> > "debounced" signal but still wants HPD hooked up to get the
> > "attention" IRQ?
> 
> Both the bridge and the panel handling HPD doesn't sound good to me...
> For the connector case, this case would mean that the connector driver
> handles the HPD, and the bridge doesn't. If the bridge has HPD support, I
> think it would make sense to disable it with 'no-hpd' property (i.e. this
> would then be case 5).

I'm not so sure. eDP / DP link has special meaning for HPD pin, so it
might be worth handling it on both sides. I can imaging the bridge
handling HPD pin to report attention IRQs, while GPIO is used for main
plug / unplug detection.

> 
> > 3. Panel has "no-hpd", ti-sn65dsi86 has no HPD-related properties
> > 
> > Doesn't really make sense. Says that panel should delay the max amount
> > but there's no good reason to do this if HPD is hooked up on
> > ti-sn65dsi86.
> 
> The connectors don't have no-hpd, so this doesn't apply there.

Connectors can simply skip the hpd-gpios property which should have the
same effect.

> 
> > 4. Panel has no HPD-related properties, ti-sn65dsi86 has "no-hpd"
> > 
> > Doesn't really make sense. Says that the panel should assume the
> > bridge has HPD hooked up but then the bridge doesn't.
> 
> For connectors, this would just mean no HPD at all connected (i.e. the case
> discussed in this series).

Same as above. Connectors don't require special handling for no HPD
case.

> 
> > 5. Panel has "hpd-gpios", ti-sn65dsi86 has "no-hpd"
> > 
> > This is the sc7180-trogdor config. Says the panel should use the GPIO
> > to read HPD for power sequencing purposes. Tells us that HPD is not
> > hooked up to the bridge chip so we should program the bridge chip to
> > ignore HPD.
> 
> For the connector case, this would be the same as 2, except the bridge
> requires disabling the HPD support via a property.

see above

> 
> > 6. Panel has "no-hpd", ti-sn65dsi86 has "no-hpd"
> > 
> > Says HPD is just not hooked up at all. panel-edp will delay for
> > "hpd-absent-delay-ms". Bridge chip should be programmed to tell the
> > hardware to ignore the HPD signal.
> 
> For connectors, this would be the same as 4.
> 
> > How we got there was fairly organic and quite a long time ago, but it
> > all sorta makes sense even if it is a bit convoluted.
> 
> I think it makes sense, and is quite similar for connectors.
> 
> Going back to this series, I think the no-hpd property makes sense to solve
> the TI issue.
> 
> However, my question about "is this needed in upstream" is still unanswered.
> If these boards are widely available, let's add this. If there are just a
> few boards here and there, with customers who anyway use TI BSP kernel, and
> the next revision of the board has the issue fixed, maybe it's not worth it?
> This change doesn't exactly make the driver cleaner or easier to maintain
> =).

I'd say, the driver needs some cleanup, if we are to land this patch.
I'd suggest to rework HPD enablement / disablement to use hpd_enable /
disable functions. Make no-hpd disable OP_HPD. Make actual detect / plug
handling tied to the hpd_notify callback, etc.

> 
>  Tomi
> 

-- 
With best wishes
Dmitry

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: Add no-hpd property

[Tomi Valkeinen]

Hi,

On 15/04/2025 18:40, Dmitry Baryshkov wrote:
> On Tue, Apr 15, 2025 at 03:50:46PM +0300, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 18/03/2025 21:51, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Tue, Mar 18, 2025 at 8:50 AM Tomi Valkeinen
>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 12/03/2025 14:52, Dmitry Baryshkov wrote:
>>>>> On Wed, Mar 12, 2025 at 11:56:41AM +0530, Harikrishna Shenoy wrote:
>>>>>>
>>>>>>
>>>>>> On 05/02/25 19:03, Dmitry Baryshkov wrote:
>>>>>>> On Wed, Feb 05, 2025 at 12:52:52PM +0100, Krzysztof Kozlowski wrote:
>>>>>>>> On 05/02/2025 12:50, Harikrishna Shenoy wrote:
>>>>>>>>> From: Rahul T R <r-ravikumar@ti.com>
>>>>>>>>>
>>>>>>>>> The mhdp bridge can work without its HPD pin hooked up to the connector,
>>>>>>>>> but the current bridge driver throws an error when hpd line is not
>>>>>>>>> connected to the connector. For such cases, we need an indication for
>>>>>>>>> no-hpd, using which we can bypass the hpd detection and instead use the
>>>>>>>>> auxiliary channels connected to the DP connector to confirm the
>>>>>>>>> connection.
>>>>>>>>> So add no-hpd property to the bindings, to disable hpd when not
>>>>>>>>> connected or unusable due to DP0-HPD not connected to correct HPD
>>>>>>>>> pin on SOC like in case of J721S2.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Rahul T R <r-ravikumar@ti.com>
>>>>>>>>
>>>>>>>> Why are you sending over and over the same? You already got feedback.
>>>>>>>> Then you send v2. You got the same feedback.
>>>>>>>>
>>>>>>>> Now you send v3?
>>>>>>>>
>>>>>>>> So the same feedback, but this time: NAK
>>>
>>> I only spent a few minutes on it, but I couldn't find a v2. If there's
>>> a link I'm happy to read it, but otherwise all my comments below are
>>> without any context from prior verisons...
>>
>> There was a link in the intro letter, although it seems to point to a reply
>> to the v2 thread... Here's v2 intro letter:
>>
>> https://lore.kernel.org/all/20230405142440.191939-1-j-choudhary@ti.com/
>>
>>>>>>> Krzysztof's email forced me to take a look at the actual boards that you
>>>>>>> are trying to enable. I couldn't stop by notice that the HPD signal
>>>>>>> _is_ connected to a GPIO pin. Please stop hacking the bridge driver and
>>>>>>> use the tools that are already provided to you: add the HPD pin to the
>>>>>>> dp-controller device node. And then fix any possible issues coming from
>>>>>>> the bridge driver not being able to handle HPD signals being delivered
>>>>>>> by the DRM framework via the .hpd_notify() callback.
>>>>>>>
>>>>>>> TL;DR: also a NAK from my side, add HPD gpio to dp-controller.
>>>>>>>
>>>>>> We tried implementing a interrupt based HPD functionality as HPD signal is
>>>>>> connected to GPIO0_18 pin, we were able to get interrupt based HPD working
>>>>>> however to route this signal to SoC we are loosing audio capability due to
>>>>>> MUX conflict. Due to board level limitations to
>>>>>> route the signal to SoC, we will not be able to support interrupt
>>>>>> based HPD and polling seems a possible way without loosing on audio
>>>>>> capability.
>>>>>
>>>>> Still NAK for the no-hpd property. HPD pin is a requirement for
>>>>> DisplayPort to work, as it is used e.g. for the 'attention' IRQs being
>>>>> sent by the DP sink. I'm not sure what kind of idea you HW engineers had
>>>>> in mind.
>>>>
>>>> It's true that for normal DP functionality the HPD is required, but
>>>> afaik DP works "fine" without HPD too. This is not the first board that
>>>> has DP connector, but doesn't have HPD, that I have seen or worked on.
>>>> Polling can be used for the IRQs too.
>>>
>>> I have less familiarity with DP than with eDP, but from what I know
>>> I'd agree with Tomi here that it would probably work "fine" by some
>>> definition of "fine". As Dmitry says, the "attention" IRQ wouldn't
>>> work, but as I understand it that's not really part of the normal flow
>>> of using DP. As evidence, some people have made "ti-sn65dsi86" (which
>>> is supposed to be for eDP only) work with DP. While the ti-sn65dsi86
>>> hardware _does_ support HPD, because of the forced (slow) debouncing
>>> it turned out not to be terribly useful for eDP and we designed our
>>> boards to route HPD to a GPIO. ...and because of that nobody ever
>>> wrote the code to handle the "attention" IRQ. Apparently people are
>>> still using this bridge w/ some success on DP monitors.
>>>
>>>
>>>> For eDP HPD is optional, and some of the cases I've worked with involved
>>>> a chip intended for eDP, but used with a full DP connector, and no HPD.
>>>
>>> I definitely agree. The eDP spec explicitly states that HPD is
>>> optional even though it's also documented to be an "attention" IRQ
>>> there. We've hooked up large numbers of eDP panels and the lack of the
>>> attention IRQ wasn't a problem.
>>>
>>>
>>>> However, in this particular case the DP chip supports full DP, so it's
>>>> just a board design error.
>>>>
>>>> My question is, is J721s2 EVM something that's used widely? Or is it a
>>>> rare board? If it's a rare one, maybe there's no point in solving this
>>>> in upstream? But if it's widely used, I don't see why we wouldn't
>>>> support it in upstream. The HW is broken, but we need to live with it.
>>>>
>>>> Another question is, if eDP support is added to the cdns-mhdp driver,
>>>> and used with a panel that doesn't have an HPD, how would that code look
>>>> like? If that would be solved with a "no-hpd" property, identical to the
>>>> one proposed in this series, then... There's even less reason to not
>>>> support this.
>>>>
>>>> Disclaimer: I didn't study the schematics, and I haven't thought or
>>>> looked at how eDP is implemented in other drm drivers.
>>>
>>> I spent lots of time working through this on ti-sn65dsi86. How it
>>> works today (and how it's documented in the bindings) is that it's
>>> possible to specify "no-hpd" on both the eDP panel node and on the
>>> bridge chip. They mean different things.
>>
>> As this text covers only eDP with Panel, I'll fill in some lines here about
>> DP and HDMI connectors. I think we need to consider all the cases.
>>
>>> The HPD-related properties that can be specified on the panel are
>>> a) <nothing> - HPD hooked up to the bridge
>>> b) no-hpd - HPD isn't hooked up at all
>>> c) hpd-gpios - HPD is hooked up to a GPIO
>>
>> For DP and HDMI connectors (dp-connector.yaml, hdmi-connector.yaml) we have
>> only 'hpd-gpios'. There hasn't been need for no-hpd.
>>
>>> The HPD-related properties that can be specified on ti-sn65dsi86 are:
>>> a) <nothing> - HPD is hooked up to the bridge
>>> b) no-hpd - HPD is not hooked up to the bridge
>>
>> More generally speaking (also with HDMI), I think this is device specific.
>> E.g. TFP410 doesn't have any kind of HPD support, so 'no-hpd' flag doesn't
>> make sense. That said, probably most of the chips do have HPD support, and
>> no-hpd is needed.
> 
> TFP410 has the EDGE/HTPLG pin, which should be used to monitor DVI /
> HDMI hot plugging pin.

Indeed! I had forgotten about that. All the uses of TFP410 I have seen 
have been without i2c, thus no usable HPD.

>>
>>> NOTE: The "ti-sn65dsi86" controller needs to be programmed to ignore
>>> its HPD line if HPD isn't hooked up. IIRC the hardware itself will not
>>> transfer things over the AUX bus unless you either tell the controller
>>> to ignore HPD or HPD is asserted.
>>>
>>>
>>> Here are the combinations:
>>>
>>> 1. Panel has no HPD-related properties, ti-sn65dsi86 has no
>>> HPD-related properties
>>>
>>> HPD is assumed to be hooked up to the dedicated HPD pin on the bridge.
>>> Panel driver queries the bridge driver to know the status of HPD. In
>>> Linux today ti-sn65dsi86 doesn't really implement this and the bridge
>>> chip just has a big, fixed, non-optimized delay that tries to account
>>> for the max delay any panel could need.
>>
>> For the connector case, I don't think there's any assumption about HPD in
>> this scenario. The connector does not handle the HPD, and it's up to the
>> bridge to decide if it does something about it or not.
> 
> Hmm? display-connector definitely supports HPD for DVI, HDMI and DP
> connectors.

Yes. I was mirroring the connector use case with what Doug described 
here: both the connector and the bridge with no HPD related properties. 
My point was that while Doug says HPD is assumed to be hooked up to the 
bridge, we don't have that kind of assumption in the general sense: the 
HPD may or may not be hooked up to the bridge, depends on the device. 
But the connector does not handle it.

>>
>>> 2. Panel has "hpd-gpios", ti-sn65dsi86 has no HPD-related properties
>>>
>>> In theory, I guess this would say that HPD goes _both_ to a GPIO and
>>> to the HPD of the bridge. Maybe handy if the bridge doesn't provide a
>>> "debounced" signal but still wants HPD hooked up to get the
>>> "attention" IRQ?
>>
>> Both the bridge and the panel handling HPD doesn't sound good to me...
>> For the connector case, this case would mean that the connector driver
>> handles the HPD, and the bridge doesn't. If the bridge has HPD support, I
>> think it would make sense to disable it with 'no-hpd' property (i.e. this
>> would then be case 5).
> 
> I'm not so sure. eDP / DP link has special meaning for HPD pin, so it
> might be worth handling it on both sides. I can imaging the bridge
> handling HPD pin to report attention IRQs, while GPIO is used for main
> plug / unplug detection.

Well... Maybe. One never knows what the HW guys come up with ;). But if 
the bridge handled HPD IRQ, I have hard time imagining a case where it 
couldn't also handle the plug/unplug HPD.

>>
>>> 3. Panel has "no-hpd", ti-sn65dsi86 has no HPD-related properties
>>>
>>> Doesn't really make sense. Says that panel should delay the max amount
>>> but there's no good reason to do this if HPD is hooked up on
>>> ti-sn65dsi86.
>>
>> The connectors don't have no-hpd, so this doesn't apply there.
> 
> Connectors can simply skip the hpd-gpios property which should have the
> same effect.

Right, that's what I meant. It is then case 1.

>>
>>> 4. Panel has no HPD-related properties, ti-sn65dsi86 has "no-hpd"
>>>
>>> Doesn't really make sense. Says that the panel should assume the
>>> bridge has HPD hooked up but then the bridge doesn't.
>>
>> For connectors, this would just mean no HPD at all connected (i.e. the case
>> discussed in this series).
> 
> Same as above. Connectors don't require special handling for no HPD
> case.
> 
>>
>>> 5. Panel has "hpd-gpios", ti-sn65dsi86 has "no-hpd"
>>>
>>> This is the sc7180-trogdor config. Says the panel should use the GPIO
>>> to read HPD for power sequencing purposes. Tells us that HPD is not
>>> hooked up to the bridge chip so we should program the bridge chip to
>>> ignore HPD.
>>
>> For the connector case, this would be the same as 2, except the bridge
>> requires disabling the HPD support via a property.
> 
> see above
> 
>>
>>> 6. Panel has "no-hpd", ti-sn65dsi86 has "no-hpd"
>>>
>>> Says HPD is just not hooked up at all. panel-edp will delay for
>>> "hpd-absent-delay-ms". Bridge chip should be programmed to tell the
>>> hardware to ignore the HPD signal.
>>
>> For connectors, this would be the same as 4.
>>
>>> How we got there was fairly organic and quite a long time ago, but it
>>> all sorta makes sense even if it is a bit convoluted.
>>
>> I think it makes sense, and is quite similar for connectors.
>>
>> Going back to this series, I think the no-hpd property makes sense to solve
>> the TI issue.
>>
>> However, my question about "is this needed in upstream" is still unanswered.
>> If these boards are widely available, let's add this. If there are just a
>> few boards here and there, with customers who anyway use TI BSP kernel, and
>> the next revision of the board has the issue fixed, maybe it's not worth it?
>> This change doesn't exactly make the driver cleaner or easier to maintain
>> =).
> 
> I'd say, the driver needs some cleanup, if we are to land this patch.
> I'd suggest to rework HPD enablement / disablement to use hpd_enable /
> disable functions. Make no-hpd disable OP_HPD. Make actual detect / plug
> handling tied to the hpd_notify callback, etc.

And it would be nice to get rid of the !DRM_BRIDGE_ATTACH_NO_CONNECTOR 
case in the driver.

  Tomi

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: Add no-hpd property

[Dmitry Baryshkov]

On Tue, Mar 18, 2025 at 05:49:54PM +0200, Tomi Valkeinen wrote:
> Hi,
> 
> On 12/03/2025 14:52, Dmitry Baryshkov wrote:
> > On Wed, Mar 12, 2025 at 11:56:41AM +0530, Harikrishna Shenoy wrote:
> > > 
> > > 
> > > On 05/02/25 19:03, Dmitry Baryshkov wrote:
> > > > On Wed, Feb 05, 2025 at 12:52:52PM +0100, Krzysztof Kozlowski wrote:
> > > > > On 05/02/2025 12:50, Harikrishna Shenoy wrote:
> > > > > > From: Rahul T R <r-ravikumar@ti.com>
> > > > > > 
> > > > > > The mhdp bridge can work without its HPD pin hooked up to the connector,
> > > > > > but the current bridge driver throws an error when hpd line is not
> > > > > > connected to the connector. For such cases, we need an indication for
> > > > > > no-hpd, using which we can bypass the hpd detection and instead use the
> > > > > > auxiliary channels connected to the DP connector to confirm the
> > > > > > connection.
> > > > > > So add no-hpd property to the bindings, to disable hpd when not
> > > > > > connected or unusable due to DP0-HPD not connected to correct HPD
> > > > > > pin on SOC like in case of J721S2.
> > > > > > 
> > > > > > Signed-off-by: Rahul T R <r-ravikumar@ti.com>
> > > > > 
> > > > > Why are you sending over and over the same? You already got feedback.
> > > > > Then you send v2. You got the same feedback.
> > > > > 
> > > > > Now you send v3?
> > > > > 
> > > > > So the same feedback, but this time: NAK
> > > > 
> > > > Krzysztof's email forced me to take a look at the actual boards that you
> > > > are trying to enable. I couldn't stop by notice that the HPD signal
> > > > _is_ connected to a GPIO pin. Please stop hacking the bridge driver and
> > > > use the tools that are already provided to you: add the HPD pin to the
> > > > dp-controller device node. And then fix any possible issues coming from
> > > > the bridge driver not being able to handle HPD signals being delivered
> > > > by the DRM framework via the .hpd_notify() callback.
> > > > 
> > > > TL;DR: also a NAK from my side, add HPD gpio to dp-controller.
> > > > 
> > > We tried implementing a interrupt based HPD functionality as HPD signal is
> > > connected to GPIO0_18 pin, we were able to get interrupt based HPD working
> > > however to route this signal to SoC we are loosing audio capability due to
> > > MUX conflict. Due to board level limitations to
> > > route the signal to SoC, we will not be able to support interrupt
> > > based HPD and polling seems a possible way without loosing on audio
> > > capability.
> > 
> > Still NAK for the no-hpd property. HPD pin is a requirement for
> > DisplayPort to work, as it is used e.g. for the 'attention' IRQs being
> > sent by the DP sink. I'm not sure what kind of idea you HW engineers had
> > in mind.
> 
> It's true that for normal DP functionality the HPD is required, but afaik DP
> works "fine" without HPD too. This is not the first board that has DP
> connector, but doesn't have HPD, that I have seen or worked on. Polling can
> be used for the IRQs too.

Just out of curiosity, is there a DP host / bridge that provide polling
for short HPD pulses (aka attention)?

> For eDP HPD is optional, and some of the cases I've worked with involved a
> chip intended for eDP, but used with a full DP connector, and no HPD.
> However, in this particular case the DP chip supports full DP, so it's just
> a board design error.

In such a case, if I'm not mistaken, the no-hpd is a part of the panel
interface rather than the eDP source. I see that SN65DSI86 has the
no-hpd property, but if I understood Doug correctly it is used to change
bridge's configuration rather than just skip the HPD processing.

> My question is, is J721s2 EVM something that's used widely? Or is it a rare
> board? If it's a rare one, maybe there's no point in solving this in
> upstream? But if it's widely used, I don't see why we wouldn't support it in
> upstream. The HW is broken, but we need to live with it.
> 
> Another question is, if eDP support is added to the cdns-mhdp driver, and
> used with a panel that doesn't have an HPD, how would that code look like?
> If that would be solved with a "no-hpd" property, identical to the one
> proposed in this series, then... There's even less reason to not support
> this.
> 
> Disclaimer: I didn't study the schematics, and I haven't thought or looked
> at how eDP is implemented in other drm drivers.

I hope that Doug can comment on eDP side. On the schematics side, there
a multi-pin mux, which switches several GPIO pins. One of the positions
of the mux is useful for audio connection. Unfortunately, DP HPD pin
gets connected in a different mux positition.


-- 
With best wishes
Dmitry

Re: [PATCH v3 1/2] dt-bindings: drm/bridge: Add no-hpd property

[Dominik Haller]

Hi,


On Wed, 2025-02-05 at 17:20 +0530, Harikrishna Shenoy wrote:
> From: Rahul T R <r-ravikumar@ti.com>
> 
> The mhdp bridge can work without its HPD pin hooked up to the
> connector,
> but the current bridge driver throws an error when hpd line is not
> connected to the connector. For such cases, we need an indication for
> no-hpd, using which we can bypass the hpd detection and instead use
> the
> auxiliary channels connected to the DP connector to confirm the
> connection.
> So add no-hpd property to the bindings, to disable hpd when not
> connected or unusable due to DP0-HPD not connected to correct HPD 
> pin on SOC like in case of J721S2.
out of curiosity. This series doesn't have anything to do with the
other DP HPD pin (AA24 MCASP1_ACLKX.DP0_HPD in case of J721S2) still
being mandatory to mux?
> Signed-off-by: Rahul T R <r-ravikumar@ti.com>
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> ---
>  .../devicetree/bindings/display/bridge/cdns,mhdp8546.yaml   | 6
> ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
> b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
> index c2b369456e4e..3a6c6d837593 100644
> ---
> a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
> +++
> b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
> @@ -57,6 +57,12 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  cdns,no-hpd:
> +    type: boolean
> +    description:
> +      Set if the HPD line on the bridge isn't hooked up to anything
> or is
> +      otherwise unusable.
> +
>    ports:
>      $ref: /schemas/graph.yaml#/properties/ports
>  

[PATCH v3 2/2] drm: bridge: cdns-mhdp8546: Add support for no-hpd

[Harikrishna Shenoy]

From: Rahul T R <r-ravikumar@ti.com>

In J721S2 EVMs DP0 hpd is not connected to correct hpd pin on SOC, to 
handle such cases, Add support for "no-hpd" property in the device 
tree node to disable hpd.

Also change the log level for dpcd read failuers to debug, since 
framework retries 32 times for each read.

Adding timeout in to wait asynchronously for state to change to 
MHDP_HW_READY. With HPD the driver acts only on the interrupt, 
which is enabled only after both have happened.Here the driver waits 
in the attach until everything is ready, and then probes the DP 
given the HPD interrupt is broken.

Add update_link_status in case of no_hpd in cdns_mhdp_bridge_detect() to 
update status of mhdp->plugged via polling, as cdns_mhdp_bridge_detect() 
is being polled by drm framework.

Signed-off-by: Rahul T R <r-ravikumar@ti.com>
[j-choudhary@ti.com: Fix cdns_mhdp_attach hook for no-hpd usecase]
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
[h-shenoy@ti.com: Updated mhdp->plugged status in
cdns_mhdp_bridge_detect for no-hpd usecase]
Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>

---
 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 54 ++++++++++++++++---
 .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  1 +
 2 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 6a121a2700d2..223370717ce8 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -53,6 +53,8 @@
 #include "cdns-mhdp8546-hdcp.h"
 #include "cdns-mhdp8546-j721e.h"
 
+static int cdns_mhdp_update_link_status(struct cdns_mhdp_device *mhdp);
+
 static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)
 {
 	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
@@ -768,7 +770,8 @@ static int cdns_mhdp_fw_activate(const struct firmware *fw,
 	 * MHDP_HW_STOPPED happens only due to driver removal when
 	 * bridge should already be detached.
 	 */
-	cdns_mhdp_bridge_hpd_enable(&mhdp->bridge);
+	if (!mhdp->no_hpd)
+		cdns_mhdp_bridge_hpd_enable(&mhdp->bridge);
 
 	spin_unlock(&mhdp->start_lock);
 
@@ -862,7 +865,7 @@ static ssize_t cdns_mhdp_transfer(struct drm_dp_aux *aux,
 		ret = cdns_mhdp_dpcd_read(mhdp, msg->address,
 					  msg->buffer, msg->size);
 		if (ret) {
-			dev_err(mhdp->dev,
+			dev_dbg(mhdp->dev,
 				"Failed to read DPCD addr %u\n",
 				msg->address);
 
@@ -1752,8 +1755,22 @@ static int cdns_mhdp_attach(struct drm_bridge *bridge,
 
 	spin_unlock(&mhdp->start_lock);
 
+	if (mhdp->no_hpd) {
+		ret = wait_event_timeout(mhdp->fw_load_wq,
+					 mhdp->hw_state == MHDP_HW_READY,
+					 msecs_to_jiffies(100));
+		if (ret == 0) {
+			dev_err(mhdp->dev, "%s: Timeout waiting for fw loading\n",
+				__func__);
+			return -ETIMEDOUT;
+		}
+
+		cdns_mhdp_update_link_status(mhdp);
+		return 0;
+	}
+
 	/* Enable SW event interrupts */
-	if (hw_ready)
+	if (hw_ready && !mhdp->no_hpd)
 		cdns_mhdp_bridge_hpd_enable(bridge);
 
 	return 0;
@@ -2217,6 +2234,19 @@ static enum drm_connector_status cdns_mhdp_bridge_detect(struct drm_bridge *brid
 {
 	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
 
+	if (mhdp->no_hpd) {
+		int ret = cdns_mhdp_update_link_status(mhdp);
+
+		if (mhdp->connector.dev) {
+			if (ret < 0)
+				schedule_work(&mhdp->modeset_retry_work);
+			else
+				drm_kms_helper_hotplug_event(mhdp->bridge.dev);
+		} else {
+			drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
+		}
+	}
+
 	return cdns_mhdp_detect(mhdp);
 }
 
@@ -2284,7 +2314,16 @@ static int cdns_mhdp_update_link_status(struct cdns_mhdp_device *mhdp)
 
 	mutex_lock(&mhdp->link_mutex);
 
-	mhdp->plugged = cdns_mhdp_detect_hpd(mhdp, &hpd_pulse);
+	if (mhdp->no_hpd) {
+		ret = drm_dp_dpcd_read_link_status(&mhdp->aux, status);
+		hpd_pulse = false;
+		if (ret < 0)
+			mhdp->plugged = false;
+		else
+			mhdp->plugged = true;
+	} else {
+		mhdp->plugged = cdns_mhdp_detect_hpd(mhdp, &hpd_pulse);
+	}
 
 	if (!mhdp->plugged) {
 		cdns_mhdp_link_down(mhdp);
@@ -2481,6 +2520,8 @@ static int cdns_mhdp_probe(struct platform_device *pdev)
 	mhdp->aux.dev = dev;
 	mhdp->aux.transfer = cdns_mhdp_transfer;
 
+	mhdp->no_hpd = of_property_read_bool(dev->of_node, "cdns,no-hpd");
+
 	mhdp->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(mhdp->regs)) {
 		dev_err(dev, "Failed to get memory resource\n");
@@ -2556,8 +2597,9 @@ static int cdns_mhdp_probe(struct platform_device *pdev)
 
 	mhdp->bridge.of_node = pdev->dev.of_node;
 	mhdp->bridge.funcs = &cdns_mhdp_bridge_funcs;
-	mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
-			   DRM_BRIDGE_OP_HPD;
+	mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
+	if (!mhdp->no_hpd)
+		mhdp->bridge.ops |= DRM_BRIDGE_OP_HPD;
 	mhdp->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
 
 	ret = phy_init(mhdp->phy);
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
index bad2fc0c7306..48517193cf0b 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
@@ -388,6 +388,7 @@ struct cdns_mhdp_device {
 
 	bool link_up;
 	bool plugged;
+	bool no_hpd;
 
 	/*
 	 * "start_lock" protects the access to bridge_attached and
-- 
2.34.1