[v4,14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed
diff mbox

Message ID 1441088079-25809-1-git-send-email-ykk@rock-chips.com
State New
Headers show

Commit Message

Yakir Yang Sept. 1, 2015, 6:14 a.m. UTC
Some edp screen do not have hpd signal, so we can't just return
failed when hpd plug in detect failed.

This is an hardware property, so we need add a devicetree property
"analogix,need-force-hpd" to indicate this sutiation.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v4: None
Changes in v3:
- Add "analogix,need-force-hpd" to indicate whether driver need foce
  hpd when hpd detect failed.

Changes in v2: None

 .../devicetree/bindings/drm/bridge/analogix_dp.txt |  4 ++-
 .../bindings/video/analogix_dp-rockchip.txt        |  1 +
 .../devicetree/bindings/video/exynos_dp.txt        |  1 +
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 36 +++++++++++++++++++---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  2 ++
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  |  9 ++++++
 6 files changed, 47 insertions(+), 6 deletions(-)

Comments

Rob Herring Sept. 2, 2015, 8:17 p.m. UTC | #1
On Tue, Sep 1, 2015 at 1:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> Some edp screen do not have hpd signal, so we can't just return
> failed when hpd plug in detect failed.

This is a property of the panel (or connector perhaps), so this
property should be located there. At least, it is a common issue and
not specific to this chip. We could have an HDMI connector and failed
to hook up HPD for example. A connector node is also where hpd-gpios
should be located instead (and are already defined by
../bindings/video/hdmi-connector.txt). Perhaps we need a eDP connector
binding, too.

Are there any eDP panels which don't have EDID and need panel details in DT?

Rob

> This is an hardware property, so we need add a devicetree property
> "analogix,need-force-hpd" to indicate this sutiation.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
> Changes in v4: None
> Changes in v3:
> - Add "analogix,need-force-hpd" to indicate whether driver need foce
>   hpd when hpd detect failed.
>
> Changes in v2: None
>
>  .../devicetree/bindings/drm/bridge/analogix_dp.txt |  4 ++-
>  .../bindings/video/analogix_dp-rockchip.txt        |  1 +
>  .../devicetree/bindings/video/exynos_dp.txt        |  1 +
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 36 +++++++++++++++++++---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  2 ++
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  |  9 ++++++
>  6 files changed, 47 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt b/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
> index f54dc3e..c310367 100644
> --- a/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
> +++ b/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
> @@ -22,6 +22,9 @@ Required properties for dp-controller:
>                 from general PHY binding: Should be "dp".
>
>  Optional properties for dp-controller:
> +       -analogix,need-force-hpd:
> +               Indicate driver need force hpd when hpd detect failed, this
> +               is used for some eDP screen which don't have hpd signal.
>         -hpd-gpios:
>                 Hotplug detect GPIO.
>                 Indicates which GPIO should be used for hotplug detection
> @@ -31,7 +34,6 @@ Optional properties for dp-controller:
>                 * Documentation/devicetree/bindings/video/exynos_dp.txt
>                 * Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
>
> -
>  [1]: Documentation/devicetree/bindings/media/video-interfaces.txt
>  -------------------------------------------------------------------------------
>
> diff --git a/Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt b/Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
> index 502483e..8b9ed7d 100644
> --- a/Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
> +++ b/Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
> @@ -28,6 +28,7 @@ For the below properties, please refer to Analogix DP binding document:
>  - phys (required)
>  - phy-names (required)
>  - hpd-gpios (optional)
> +- analogix,need-force-hpd (optional)
>  -------------------------------------------------------------------------------
>
>  Example:
> diff --git a/Documentation/devicetree/bindings/video/exynos_dp.txt b/Documentation/devicetree/bindings/video/exynos_dp.txt
> index ea03b3a..4f06e80 100644
> --- a/Documentation/devicetree/bindings/video/exynos_dp.txt
> +++ b/Documentation/devicetree/bindings/video/exynos_dp.txt
> @@ -41,6 +41,7 @@ For the below properties, please refer to Analogix DP binding document:
>         -phys (required)
>         -phy-names (required)
>         -hpd-gpios (optional)
> +       -analogix,need-force-hpd (optional)
>         -video interfaces (optional)
>
>  Deprecated properties for DisplayPort:
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index f7227ec..e6b328a 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -63,15 +63,38 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
>  {
>         int timeout_loop = 0;
>
> -       while (analogix_dp_get_plug_in_status(dp) != 0) {
> +       while (timeout_loop < DP_TIMEOUT_LOOP_COUNT) {
> +               if (analogix_dp_get_plug_in_status(dp) == 0)
> +                       return 0;
> +
>                 timeout_loop++;
> -               if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
> -                       dev_err(dp->dev, "failed to get hpd plug status\n");
> -                       return -ETIMEDOUT;
> -               }
>                 usleep_range(10, 11);
>         }
>
> +       /*
> +        * Some edp screen do not have hpd signal, so we can't just
> +        * return failed when hpd plug in detect failed, DT property
> +        * "need-force-hpd" would indicate whether driver need this.
> +        */
> +       if (!dp->need_force_hpd)
> +               return -ETIMEDOUT;
> +
> +       /*
> +        * The eDP TRM indicate that if HPD_STATUS(RO) is 0, AUX CH
> +        * will not work, so we need to give a force hpd action to
> +        * set HPD_STATUS manually.
> +        */
> +       dev_dbg(dp->dev, "failed to get hpd plug status, try to force hpd\n");
> +
> +       analogix_dp_force_hpd(dp);
> +
> +       if (analogix_dp_get_plug_in_status(dp) != 0) {
> +               dev_err(dp->dev, "failed to get hpd plug in status\n");
> +               return -EINVAL;
> +       }
> +
> +       dev_dbg(dp->dev, "success to get plug in status after force hpd\n");
> +
>         return 0;
>  }
>
> @@ -1287,6 +1310,9 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
>         if (IS_ERR(dp->reg_base))
>                 return PTR_ERR(dp->reg_base);
>
> +       dp->need_force_hpd =
> +               of_property_read_bool(dev->of_node, "analogix,need-force-hpd");
> +
>         dp->hpd_gpio = of_get_named_gpio(dev->of_node, "hpd-gpios", 0);
>         if (gpio_is_valid(dp->hpd_gpio))
>                 dp->hpd_gpio = of_get_named_gpio(dev->of_node,
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> index d8945e2..6960ab3 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> @@ -160,6 +160,7 @@ struct analogix_dp_device {
>         struct phy              *phy;
>         int                     dpms_mode;
>         int                     hpd_gpio;
> +       bool                    need_force_hpd;
>
>         struct analogix_dp_plat_data *plat_data;
>  };
> @@ -180,6 +181,7 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
>                                        bool enable);
>  void analogix_dp_init_analog_func(struct analogix_dp_device *dp);
>  void analogix_dp_init_hpd(struct analogix_dp_device *dp);
> +void analogix_dp_force_hpd(struct analogix_dp_device *dp);
>  enum dp_irq_type analogix_dp_get_irq_type(struct analogix_dp_device *dp);
>  void analogix_dp_clear_hotplug_interrupts(struct analogix_dp_device *dp);
>  void analogix_dp_reset_aux(struct analogix_dp_device *dp);
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index 15346fe..3086afc 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -365,6 +365,15 @@ void analogix_dp_init_hpd(struct analogix_dp_device *dp)
>         writel(reg, dp->reg_base + ANALOGIX_DP_SYS_CTL_3);
>  }
>
> +void analogix_dp_force_hpd(struct analogix_dp_device *dp)
> +{
> +       u32 reg;
> +
> +       reg = readl(dp->reg_base + ANALOGIX_DP_SYS_CTL_3);
> +       reg = (F_HPD | HPD_CTRL);
> +       writel(reg, dp->reg_base + ANALOGIX_DP_SYS_CTL_3);
> +}
> +
>  enum dp_irq_type analogix_dp_get_irq_type(struct analogix_dp_device *dp)
>  {
>         u32 reg;
> --
> 2.1.2
>
>
Yakir Yang Sept. 3, 2015, 4:27 a.m. UTC | #2
Hi Rob,

? 09/03/2015 04:17 AM, Rob Herring ??:
> On Tue, Sep 1, 2015 at 1:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>> Some edp screen do not have hpd signal, so we can't just return
>> failed when hpd plug in detect failed.
> This is a property of the panel (or connector perhaps), so this
> property should be located there. At least, it is a common issue and
> not specific to this chip. We could have an HDMI connector and failed
> to hook up HPD for example. A connector node is also where hpd-gpios
> should be located instead (and are already defined by
> ../bindings/video/hdmi-connector.txt). Perhaps we need a eDP connector
> binding, too.

Yep, I agree with your front point, it is a property of panel, not specific
to eDP controller, so this code should handle in connector logic.

But another question, if we just leave this property to connector,
then we would need to parse this property in analogix_dp-rockchip.c,
and then make an hook in analogix_dp_core.c driver. This is not nice,
and if there are some coming platform which alse want to use analogix_dp
code and meet this "no-hpd" situation,  then they would need duplicate
to parse this property and fill the hook in analogix_dp_core.c driver.
So it's little bit conflict  :-)

Beside I can not understand your example very well. Do you mean
that there are some HDMI monitor which also do not have HPD
signal (just like some eDP panel do not have hpd too), and then
the "hpd-gpios" ? Hmm... I don't know how the "hpd-gpios" want
to help in this case, would you mind show some sample code  :-D

> Are there any eDP panels which don't have EDID and need panel details in DT?

Oh, I think you want to collect some info that belong to panel
property but no indicate in panel EDID message, so those can
be collect in eDP connector binding, is it right ?

As for me, I just meet this "no-hpd" special situation.

- Yakir

>
> Rob
>
>> This is an hardware property, so we need add a devicetree property
>> "analogix,need-force-hpd" to indicate this sutiation.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> ---
>> Changes in v4: None
>> Changes in v3:
>> - Add "analogix,need-force-hpd" to indicate whether driver need foce
>>    hpd when hpd detect failed.
>>
>> Changes in v2: None
>>
>>   .../devicetree/bindings/drm/bridge/analogix_dp.txt |  4 ++-
>>   .../bindings/video/analogix_dp-rockchip.txt        |  1 +
>>   .../devicetree/bindings/video/exynos_dp.txt        |  1 +
>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 36 +++++++++++++++++++---
>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  2 ++
>>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  |  9 ++++++
>>   6 files changed, 47 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt b/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
>> index f54dc3e..c310367 100644
>> --- a/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
>> +++ b/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
>> @@ -22,6 +22,9 @@ Required properties for dp-controller:
>>                  from general PHY binding: Should be "dp".
>>
>>   Optional properties for dp-controller:
>> +       -analogix,need-force-hpd:
>> +               Indicate driver need force hpd when hpd detect failed, this
>> +               is used for some eDP screen which don't have hpd signal.
>>          -hpd-gpios:
>>                  Hotplug detect GPIO.
>>                  Indicates which GPIO should be used for hotplug detection
>> @@ -31,7 +34,6 @@ Optional properties for dp-controller:
>>                  * Documentation/devicetree/bindings/video/exynos_dp.txt
>>                  * Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
>>
>> -
>>   [1]: Documentation/devicetree/bindings/media/video-interfaces.txt
>>   -------------------------------------------------------------------------------
>>
>> diff --git a/Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt b/Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
>> index 502483e..8b9ed7d 100644
>> --- a/Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
>> +++ b/Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
>> @@ -28,6 +28,7 @@ For the below properties, please refer to Analogix DP binding document:
>>   - phys (required)
>>   - phy-names (required)
>>   - hpd-gpios (optional)
>> +- analogix,need-force-hpd (optional)
>>   -------------------------------------------------------------------------------
>>
>>   Example:
>> diff --git a/Documentation/devicetree/bindings/video/exynos_dp.txt b/Documentation/devicetree/bindings/video/exynos_dp.txt
>> index ea03b3a..4f06e80 100644
>> --- a/Documentation/devicetree/bindings/video/exynos_dp.txt
>> +++ b/Documentation/devicetree/bindings/video/exynos_dp.txt
>> @@ -41,6 +41,7 @@ For the below properties, please refer to Analogix DP binding document:
>>          -phys (required)
>>          -phy-names (required)
>>          -hpd-gpios (optional)
>> +       -analogix,need-force-hpd (optional)
>>          -video interfaces (optional)
>>
>>   Deprecated properties for DisplayPort:
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> index f7227ec..e6b328a 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> @@ -63,15 +63,38 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
>>   {
>>          int timeout_loop = 0;
>>
>> -       while (analogix_dp_get_plug_in_status(dp) != 0) {
>> +       while (timeout_loop < DP_TIMEOUT_LOOP_COUNT) {
>> +               if (analogix_dp_get_plug_in_status(dp) == 0)
>> +                       return 0;
>> +
>>                  timeout_loop++;
>> -               if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
>> -                       dev_err(dp->dev, "failed to get hpd plug status\n");
>> -                       return -ETIMEDOUT;
>> -               }
>>                  usleep_range(10, 11);
>>          }
>>
>> +       /*
>> +        * Some edp screen do not have hpd signal, so we can't just
>> +        * return failed when hpd plug in detect failed, DT property
>> +        * "need-force-hpd" would indicate whether driver need this.
>> +        */
>> +       if (!dp->need_force_hpd)
>> +               return -ETIMEDOUT;
>> +
>> +       /*
>> +        * The eDP TRM indicate that if HPD_STATUS(RO) is 0, AUX CH
>> +        * will not work, so we need to give a force hpd action to
>> +        * set HPD_STATUS manually.
>> +        */
>> +       dev_dbg(dp->dev, "failed to get hpd plug status, try to force hpd\n");
>> +
>> +       analogix_dp_force_hpd(dp);
>> +
>> +       if (analogix_dp_get_plug_in_status(dp) != 0) {
>> +               dev_err(dp->dev, "failed to get hpd plug in status\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       dev_dbg(dp->dev, "success to get plug in status after force hpd\n");
>> +
>>          return 0;
>>   }
>>
>> @@ -1287,6 +1310,9 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
>>          if (IS_ERR(dp->reg_base))
>>                  return PTR_ERR(dp->reg_base);
>>
>> +       dp->need_force_hpd =
>> +               of_property_read_bool(dev->of_node, "analogix,need-force-hpd");
>> +
>>          dp->hpd_gpio = of_get_named_gpio(dev->of_node, "hpd-gpios", 0);
>>          if (gpio_is_valid(dp->hpd_gpio))
>>                  dp->hpd_gpio = of_get_named_gpio(dev->of_node,
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>> index d8945e2..6960ab3 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>> @@ -160,6 +160,7 @@ struct analogix_dp_device {
>>          struct phy              *phy;
>>          int                     dpms_mode;
>>          int                     hpd_gpio;
>> +       bool                    need_force_hpd;
>>
>>          struct analogix_dp_plat_data *plat_data;
>>   };
>> @@ -180,6 +181,7 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
>>                                         bool enable);
>>   void analogix_dp_init_analog_func(struct analogix_dp_device *dp);
>>   void analogix_dp_init_hpd(struct analogix_dp_device *dp);
>> +void analogix_dp_force_hpd(struct analogix_dp_device *dp);
>>   enum dp_irq_type analogix_dp_get_irq_type(struct analogix_dp_device *dp);
>>   void analogix_dp_clear_hotplug_interrupts(struct analogix_dp_device *dp);
>>   void analogix_dp_reset_aux(struct analogix_dp_device *dp);
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> index 15346fe..3086afc 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> @@ -365,6 +365,15 @@ void analogix_dp_init_hpd(struct analogix_dp_device *dp)
>>          writel(reg, dp->reg_base + ANALOGIX_DP_SYS_CTL_3);
>>   }
>>
>> +void analogix_dp_force_hpd(struct analogix_dp_device *dp)
>> +{
>> +       u32 reg;
>> +
>> +       reg = readl(dp->reg_base + ANALOGIX_DP_SYS_CTL_3);
>> +       reg = (F_HPD | HPD_CTRL);
>> +       writel(reg, dp->reg_base + ANALOGIX_DP_SYS_CTL_3);
>> +}
>> +
>>   enum dp_irq_type analogix_dp_get_irq_type(struct analogix_dp_device *dp)
>>   {
>>          u32 reg;
>> --
>> 2.1.2
>>
>>
>
>
Thierry Reding Sept. 3, 2015, 8:47 a.m. UTC | #3
On Wed, Sep 02, 2015 at 03:17:57PM -0500, Rob Herring wrote:
[...]
> Are there any eDP panels which don't have EDID and need panel details in DT?

Most panels need information other than EDID. They typically have some
requirements regarding the power up sequence that aren't to be found
anywhere in EDID or detectable by some other mechanism. A decision was
therefore made a long time ago to require panels to be listed in DT with
a specific compatible string. That way all of these details can be
stashed away in drivers that know how to deal with these kinds of
details.

Thierry
Thierry Reding Sept. 3, 2015, 9:04 a.m. UTC | #4
On Thu, Sep 03, 2015 at 12:27:47PM +0800, Yakir Yang wrote:
> Hi Rob,
> 
> ? 09/03/2015 04:17 AM, Rob Herring ??:
> >On Tue, Sep 1, 2015 at 1:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> >>Some edp screen do not have hpd signal, so we can't just return
> >>failed when hpd plug in detect failed.
> >This is a property of the panel (or connector perhaps), so this
> >property should be located there. At least, it is a common issue and
> >not specific to this chip. We could have an HDMI connector and failed
> >to hook up HPD for example. A connector node is also where hpd-gpios
> >should be located instead (and are already defined by
> >../bindings/video/hdmi-connector.txt). Perhaps we need a eDP connector
> >binding, too.
> 
> Yep, I agree with your front point, it is a property of panel, not specific
> to eDP controller, so this code should handle in connector logic.

From your description it sounds more like this is in fact a property of
the panel. Or maybe I should say "quirk". If the panel doesn't generate
the HPD signal, then that should be a property of the panel, not the
connector. The eDP specification mandates that connectors have a HPD
signal, though it allows the "HPD conductor in the connector cable" to
be omitted if not used by the source. I'd consider the cable to belong
to the panel rather than the connector, so absence of HPD, either
because the cable doesn't have the conductor or because the panel does
not generate the signal, should be a quirk of the panel.

That said you could have a panel that supports HPD connected via a cable
that doesn't transmit it, so this would be a per-board variant and hence
should be a device tree property rather than hard-coded in some panel
driver.

Conversely, if the panel isn't capable of generating an HPD signal, then
I don't think it would be appropriate to make it a DT property. It would
be better to hard-code it in the driver, lest someone forget to set the
property in DT and get stuck with a device that isn't operational.

Thierry
Rob Herring Sept. 3, 2015, 9:55 p.m. UTC | #5
On Thu, Sep 3, 2015 at 3:47 AM, Thierry Reding <treding@nvidia.com> wrote:
> On Wed, Sep 02, 2015 at 03:17:57PM -0500, Rob Herring wrote:
> [...]
>> Are there any eDP panels which don't have EDID and need panel details in DT?
>
> Most panels need information other than EDID. They typically have some
> requirements regarding the power up sequence that aren't to be found
> anywhere in EDID or detectable by some other mechanism. A decision was
> therefore made a long time ago to require panels to be listed in DT with
> a specific compatible string. That way all of these details can be
> stashed away in drivers that know how to deal with these kinds of
> details.

I guess I was being hopeful that eDP was improving that situation.

Rob
Thierry Reding Sept. 4, 2015, 10:01 a.m. UTC | #6
On Thu, Sep 03, 2015 at 04:55:59PM -0500, Rob Herring wrote:
> On Thu, Sep 3, 2015 at 3:47 AM, Thierry Reding <treding@nvidia.com> wrote:
> > On Wed, Sep 02, 2015 at 03:17:57PM -0500, Rob Herring wrote:
> > [...]
> >> Are there any eDP panels which don't have EDID and need panel details in DT?
> >
> > Most panels need information other than EDID. They typically have some
> > requirements regarding the power up sequence that aren't to be found
> > anywhere in EDID or detectable by some other mechanism. A decision was
> > therefore made a long time ago to require panels to be listed in DT with
> > a specific compatible string. That way all of these details can be
> > stashed away in drivers that know how to deal with these kinds of
> > details.
> 
> I guess I was being hopeful that eDP was improving that situation.

Unfortunately not. eDP helps with a number of things (DPCD and link
training take care of a lot of the issues), but it doesn't provide a
solution for everything.

To my knowledge in most cases the power up sequence isn't strictly
necessary to get the panel to operate. But there have been instances
where this is absolutely required if you want to avoid visual artifacts
as you set a mode. A lot of panels that I've come across require
receiving a couple of frames before they guarantee that something will
actually be displayed on the screen. So if your code is too quickly
enabling backlight, and your backlight doesn't happen to have just the
right amount of ramp-up time you might end up seeing garbage on the
screen for a couple of frames.

It would be great if somebody came up with, say, an EDID extension to
describe these requirements, but I'm not sure if even that would give
us panels that could be driven with a generic driver. Often the power
supplies or reset/enable signals are very different across panels. So
describing it all generically would become messy rather quickly.

Thierry
Russell King - ARM Linux Sept. 4, 2015, 10:20 a.m. UTC | #7
On Thu, Sep 03, 2015 at 11:04:40AM +0200, Thierry Reding wrote:
> Conversely, if the panel isn't capable of generating an HPD signal, then
> I don't think it would be appropriate to make it a DT property. It would
> be better to hard-code it in the driver, lest someone forget to set the
> property in DT and get stuck with a device that isn't operational.

There is another way to deal with this: DRM supports the idea of connector
forcing - where you can force the connector to think that it's connected
or disconnected.

One of the problems is that not many ARM DRM drivers implement it - maybe
it should be a requirement for code to be accepted? :)
Rob Herring Sept. 4, 2015, 9:46 p.m. UTC | #8
On Wed, Sep 2, 2015 at 11:27 PM, Yakir Yang <ykk@rock-chips.com> wrote:
> Hi Rob,
>
> ? 09/03/2015 04:17 AM, Rob Herring ??:
>>
>> On Tue, Sep 1, 2015 at 1:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>>>
>>> Some edp screen do not have hpd signal, so we can't just return
>>> failed when hpd plug in detect failed.
>>
>> This is a property of the panel (or connector perhaps), so this
>> property should be located there. At least, it is a common issue and
>> not specific to this chip. We could have an HDMI connector and failed
>> to hook up HPD for example. A connector node is also where hpd-gpios
>> should be located instead (and are already defined by
>> ../bindings/video/hdmi-connector.txt). Perhaps we need a eDP connector
>> binding, too.
>
>
> Yep, I agree with your front point, it is a property of panel, not specific
> to eDP controller, so this code should handle in connector logic.
>
> But another question, if we just leave this property to connector,
> then we would need to parse this property in analogix_dp-rockchip.c,
> and then make an hook in analogix_dp_core.c driver. This is not nice,
> and if there are some coming platform which alse want to use analogix_dp
> code and meet this "no-hpd" situation,  then they would need duplicate
> to parse this property and fill the hook in analogix_dp_core.c driver.
> So it's little bit conflict  :-)

Ideally, you would be able to just retrieve this quirk from the
connector or panel. Getting this property from your node vs. the port
you are attached to should not be much harder. Either the connector
struct can have this info or there can be a DT function that can walk
the graph and get it. Just don't open code the graph traversal in your
driver.

> Beside I can not understand your example very well. Do you mean
> that there are some HDMI monitor which also do not have HPD
> signal (just like some eDP panel do not have hpd too), and then
> the "hpd-gpios" ? Hmm... I don't know how the "hpd-gpios" want
> to help in this case, would you mind show some sample code  :-D

I don't know that there is h/w, but it is always possible HPD is not
hooked up or hooked up incorrectly some how.

If there is no HPD support, then hpd-gpios is not going to help you.

I think there are 3 cases to handle:
- HPD handled by bridge chip - The bridge driver knows it has this
capability. No DT properties are needed and no HPD properties on the
connector node imply the bridge chip should handle HPD functions.
- HPD handled by GPIO line (or some other block) - Indicated by
hpd-gpios present
- No or broken HPD - Indicated by "hpd-force" property.

>
>> Are there any eDP panels which don't have EDID and need panel details in
>> DT?
>
>
> Oh, I think you want to collect some info that belong to panel
> property but no indicate in panel EDID message, so those can
> be collect in eDP connector binding, is it right ?

Yes, and as Thierry pointed out we may need to know the exact panel even.

Rob
Yakir Yang Sept. 6, 2015, 8:20 a.m. UTC | #9
Hi Rob,

? 09/05/2015 05:46 AM, Rob Herring ??:
> On Wed, Sep 2, 2015 at 11:27 PM, Yakir Yang <ykk@rock-chips.com> wrote:
>> Hi Rob,
>>
>> ? 09/03/2015 04:17 AM, Rob Herring ??:
>>> On Tue, Sep 1, 2015 at 1:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>>>> Some edp screen do not have hpd signal, so we can't just return
>>>> failed when hpd plug in detect failed.
>>> This is a property of the panel (or connector perhaps), so this
>>> property should be located there. At least, it is a common issue and
>>> not specific to this chip. We could have an HDMI connector and failed
>>> to hook up HPD for example. A connector node is also where hpd-gpios
>>> should be located instead (and are already defined by
>>> ../bindings/video/hdmi-connector.txt). Perhaps we need a eDP connector
>>> binding, too.
>>
>> Yep, I agree with your front point, it is a property of panel, not specific
>> to eDP controller, so this code should handle in connector logic.
>>
>> But another question, if we just leave this property to connector,
>> then we would need to parse this property in analogix_dp-rockchip.c,
>> and then make an hook in analogix_dp_core.c driver. This is not nice,
>> and if there are some coming platform which alse want to use analogix_dp
>> code and meet this "no-hpd" situation,  then they would need duplicate
>> to parse this property and fill the hook in analogix_dp_core.c driver.
>> So it's little bit conflict  :-)
> Ideally, you would be able to just retrieve this quirk from the
> connector or panel. Getting this property from your node vs. the port
> you are attached to should not be much harder. Either the connector
> struct can have this info or there can be a DT function that can walk
> the graph and get it. Just don't open code the graph traversal in your
> driver.
>
>> Beside I can not understand your example very well. Do you mean
>> that there are some HDMI monitor which also do not have HPD
>> signal (just like some eDP panel do not have hpd too), and then
>> the "hpd-gpios" ? Hmm... I don't know how the "hpd-gpios" want
>> to help in this case, would you mind show some sample code  :-D
> I don't know that there is h/w, but it is always possible HPD is not
> hooked up or hooked up incorrectly some how.
>
> If there is no HPD support, then hpd-gpios is not going to help you.
>
> I think there are 3 cases to handle:
> - HPD handled by bridge chip - The bridge driver knows it has this
> capability. No DT properties are needed and no HPD properties on the
> connector node imply the bridge chip should handle HPD functions.
> - HPD handled by GPIO line (or some other block) - Indicated by
> hpd-gpios present
> - No or broken HPD - Indicated by "hpd-force" property.

Oh, I think the first/second case isn't suitable in this case, my panel
"cnm,n116bgeea2" haven't included HPD signal.

>
>>> Are there any eDP panels which don't have EDID and need panel details in
>>> DT?
>>
>> Oh, I think you want to collect some info that belong to panel
>> property but no indicate in panel EDID message, so those can
>> be collect in eDP connector binding, is it right ?
> Yes, and as Thierry pointed out we may need to know the exact panel even.

Yeah, just like I reply to Thierry, this is a panel quirk. And he 
suggest we should
handle this in panel driver, but I have no idea how to handle this in 
common panel
driver.   :)

- Yakir

> Rob
>
>
>
Thierry Reding Sept. 7, 2015, 8:20 a.m. UTC | #10
On Sun, Sep 06, 2015 at 11:59:08AM +0800, Yakir Yang wrote:
> Hi Thierry,
> 
> ? 09/03/2015 05:04 PM, Thierry Reding ??:
> >On Thu, Sep 03, 2015 at 12:27:47PM +0800, Yakir Yang wrote:
> >>Hi Rob,
> >>
> >>? 09/03/2015 04:17 AM, Rob Herring ??:
> >>>On Tue, Sep 1, 2015 at 1:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> >>>>Some edp screen do not have hpd signal, so we can't just return
> >>>>failed when hpd plug in detect failed.
> >>>This is a property of the panel (or connector perhaps), so this
> >>>property should be located there. At least, it is a common issue and
> >>>not specific to this chip. We could have an HDMI connector and failed
> >>>to hook up HPD for example. A connector node is also where hpd-gpios
> >>>should be located instead (and are already defined by
> >>>../bindings/video/hdmi-connector.txt). Perhaps we need a eDP connector
> >>>binding, too.
> >>Yep, I agree with your front point, it is a property of panel, not specific
> >>to eDP controller, so this code should handle in connector logic.
> > From your description it sounds more like this is in fact a property of
> >the panel. Or maybe I should say "quirk". If the panel doesn't generate
> >the HPD signal, then that should be a property of the panel, not the
> >connector. The eDP specification mandates that connectors have a HPD
> >signal, though it allows the "HPD conductor in the connector cable" to
> >be omitted if not used by the source. I'd consider the cable to belong
> >to the panel rather than the connector, so absence of HPD, either
> >because the cable doesn't have the conductor or because the panel does
> >not generate the signal, should be a quirk of the panel.
> >
> >That said you could have a panel that supports HPD connected via a cable
> >that doesn't transmit it, so this would be a per-board variant and hence
> >should be a device tree property rather than hard-coded in some panel
> >driver.
> >
> >Conversely, if the panel isn't capable of generating an HPD signal, then
> >I don't think it would be appropriate to make it a DT property. It would
> >be better to hard-code it in the driver, lest someone forget to set the
> >property in DT and get stuck with a device that isn't operational.
> 
> Oh, you're right, if it's a cable quirk, then DT property would be okay, if
> it
> is a problem of panel, then maybe hard-code in driver would be better.
> 
> After look up for the document of panel "innolux,n116bge", I haven't see
> any description of hot plug signal, and even not found in PIN ASSIGNMENT.
> So I believe it's a panel problem, that's to say it should handle in panel
> driver.

The datasheet that I have for that panel lists HPD as pin 17. Also I
used to have a setup with that panel and I distinctly remember hotplug
working just fine. Perhaps this is an issue with a specific variant of
the panel? Or perhaps this is indeed a problem with the cable that's
connecting the panel to the board. It could be one of those cases where
they left out the HPD conductor to save money.

Thierry
Thierry Reding Sept. 7, 2015, 8:39 a.m. UTC | #11
On Sun, Sep 06, 2015 at 04:20:39PM +0800, Yakir Yang wrote:
> Hi Rob,
> 
> ? 09/05/2015 05:46 AM, Rob Herring ??:
> >On Wed, Sep 2, 2015 at 11:27 PM, Yakir Yang <ykk@rock-chips.com> wrote:
> >>Hi Rob,
> >>
> >>? 09/03/2015 04:17 AM, Rob Herring ??:
> >>>On Tue, Sep 1, 2015 at 1:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> >>>>Some edp screen do not have hpd signal, so we can't just return
> >>>>failed when hpd plug in detect failed.
> >>>This is a property of the panel (or connector perhaps), so this
> >>>property should be located there. At least, it is a common issue and
> >>>not specific to this chip. We could have an HDMI connector and failed
> >>>to hook up HPD for example. A connector node is also where hpd-gpios
> >>>should be located instead (and are already defined by
> >>>../bindings/video/hdmi-connector.txt). Perhaps we need a eDP connector
> >>>binding, too.
> >>
> >>Yep, I agree with your front point, it is a property of panel, not specific
> >>to eDP controller, so this code should handle in connector logic.
> >>
> >>But another question, if we just leave this property to connector,
> >>then we would need to parse this property in analogix_dp-rockchip.c,
> >>and then make an hook in analogix_dp_core.c driver. This is not nice,
> >>and if there are some coming platform which alse want to use analogix_dp
> >>code and meet this "no-hpd" situation,  then they would need duplicate
> >>to parse this property and fill the hook in analogix_dp_core.c driver.
> >>So it's little bit conflict  :-)
> >Ideally, you would be able to just retrieve this quirk from the
> >connector or panel. Getting this property from your node vs. the port
> >you are attached to should not be much harder. Either the connector
> >struct can have this info or there can be a DT function that can walk
> >the graph and get it. Just don't open code the graph traversal in your
> >driver.
> >
> >>Beside I can not understand your example very well. Do you mean
> >>that there are some HDMI monitor which also do not have HPD
> >>signal (just like some eDP panel do not have hpd too), and then
> >>the "hpd-gpios" ? Hmm... I don't know how the "hpd-gpios" want
> >>to help in this case, would you mind show some sample code  :-D
> >I don't know that there is h/w, but it is always possible HPD is not
> >hooked up or hooked up incorrectly some how.
> >
> >If there is no HPD support, then hpd-gpios is not going to help you.
> >
> >I think there are 3 cases to handle:
> >- HPD handled by bridge chip - The bridge driver knows it has this
> >capability. No DT properties are needed and no HPD properties on the
> >connector node imply the bridge chip should handle HPD functions.
> >- HPD handled by GPIO line (or some other block) - Indicated by
> >hpd-gpios present
> >- No or broken HPD - Indicated by "hpd-force" property.
> 
> Oh, I think the first/second case isn't suitable in this case, my panel
> "cnm,n116bgeea2" haven't included HPD signal.

Note that if your panel doesn't signal HPD you're supposed to poll the
sink to make sure you catch loss of link integrity. I've always found it
odd that people would want to save a couple of bucks on the HPD signal
conductor when that means that you will consume more power because you
need to periodically poll the link for integrity.

> >>>Are there any eDP panels which don't have EDID and need panel details in
> >>>DT?
> >>
> >>Oh, I think you want to collect some info that belong to panel
> >>property but no indicate in panel EDID message, so those can
> >>be collect in eDP connector binding, is it right ?
> >Yes, and as Thierry pointed out we may need to know the exact panel even.
> 
> Yeah, just like I reply to Thierry, this is a panel quirk. And he suggest we
> should
> handle this in panel driver, but I have no idea how to handle this in common
> panel
> driver.   :)

Like I said in another subthread, this panel does have an HPD line in a
variant that I've used in the past. So if your variant doesn't have the
HPD line, we're dealing with quirks within the same family of panels.
That would make this some sort of quirk, and I'm not sure if there is a
standard way for defining quirks in DT. Rob, do you know of any
precedent for this?

I suppose we could always add a variant-specific compatible string that
would allow this quirk to be encoded in the driver, though I'm not sure
if the model string in datasheets has enough detail to tell them apart.

Thierry
Thierry Reding Sept. 7, 2015, 9:01 a.m. UTC | #12
On Fri, Sep 04, 2015 at 11:20:03AM +0100, Russell King - ARM Linux wrote:
> On Thu, Sep 03, 2015 at 11:04:40AM +0200, Thierry Reding wrote:
> > Conversely, if the panel isn't capable of generating an HPD signal, then
> > I don't think it would be appropriate to make it a DT property. It would
> > be better to hard-code it in the driver, lest someone forget to set the
> > property in DT and get stuck with a device that isn't operational.
> 
> There is another way to deal with this: DRM supports the idea of connector
> forcing - where you can force the connector to think that it's connected
> or disconnected.

Yes, this could work well for RGB/LVDS or DSI connectors perhaps. For
eDP there is added complexity because the HPD interrupt function is also
used to signal loss of link integrity. That is, after receiving an HPD
interrupt you are supposed to retrain the link (or at least check the
link status to see if the interrupt cause is loss of integrity). While
the eDP specification makes HPD optional, it also says that if HPD isn't
available, then the source must use polling to monitor link integrity
instead.

DRM does provide a mechanism for that as well. You can set the
connector's ->polled field to DRM_CONNECTOR_POLL_CONNECT |
DRM_CONNECTOR_POLL_DISCONNECT and have the core actively poll for the
connector status (i.e. call ->detect() every 100 ms).

I think use of polling would be more appropriate in case of eDP.

> One of the problems is that not many ARM DRM drivers implement it - maybe
> it should be a requirement for code to be accepted? :)

I suspect that many drivers may roll their own. In fact I'm guilty of
that myself. On Tegra we have a default implementation for outputs which
will default to the state of an HPD GPIO where available and fall back
to "always connected" for outputs that have a panel connected. Outputs
that have a separate mechanism to signal hotplug detection (such as DP)
simply use a custom ->detect() implementation.

The overhead of rolling one's own is almost zero and connector forcing
has the disadvantage of being available via sysfs and debugfs, so the
default set by drivers could be overwritten by users at runtime with no
easy way back.

Given the above I'm not sure enforcing connector forcing would be
beneficial.

Thierry

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt b/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
index f54dc3e..c310367 100644
--- a/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
+++ b/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
@@ -22,6 +22,9 @@  Required properties for dp-controller:
 		from general PHY binding: Should be "dp".
 
 Optional properties for dp-controller:
+	-analogix,need-force-hpd:
+		Indicate driver need force hpd when hpd detect failed, this
+		is used for some eDP screen which don't have hpd signal.
 	-hpd-gpios:
 		Hotplug detect GPIO.
 		Indicates which GPIO should be used for hotplug detection
@@ -31,7 +34,6 @@  Optional properties for dp-controller:
 		* Documentation/devicetree/bindings/video/exynos_dp.txt
 		* Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
 
-
 [1]: Documentation/devicetree/bindings/media/video-interfaces.txt
 -------------------------------------------------------------------------------
 
diff --git a/Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt b/Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
index 502483e..8b9ed7d 100644
--- a/Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
+++ b/Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
@@ -28,6 +28,7 @@  For the below properties, please refer to Analogix DP binding document:
 - phys (required)
 - phy-names (required)
 - hpd-gpios (optional)
+- analogix,need-force-hpd (optional)
 -------------------------------------------------------------------------------
 
 Example:
diff --git a/Documentation/devicetree/bindings/video/exynos_dp.txt b/Documentation/devicetree/bindings/video/exynos_dp.txt
index ea03b3a..4f06e80 100644
--- a/Documentation/devicetree/bindings/video/exynos_dp.txt
+++ b/Documentation/devicetree/bindings/video/exynos_dp.txt
@@ -41,6 +41,7 @@  For the below properties, please refer to Analogix DP binding document:
 	-phys (required)
 	-phy-names (required)
 	-hpd-gpios (optional)
+	-analogix,need-force-hpd (optional)
 	-video interfaces (optional)
 
 Deprecated properties for DisplayPort:
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index f7227ec..e6b328a 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -63,15 +63,38 @@  static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
 {
 	int timeout_loop = 0;
 
-	while (analogix_dp_get_plug_in_status(dp) != 0) {
+	while (timeout_loop < DP_TIMEOUT_LOOP_COUNT) {
+		if (analogix_dp_get_plug_in_status(dp) == 0)
+			return 0;
+
 		timeout_loop++;
-		if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
-			dev_err(dp->dev, "failed to get hpd plug status\n");
-			return -ETIMEDOUT;
-		}
 		usleep_range(10, 11);
 	}
 
+	/*
+	 * Some edp screen do not have hpd signal, so we can't just
+	 * return failed when hpd plug in detect failed, DT property
+	 * "need-force-hpd" would indicate whether driver need this.
+	 */
+	if (!dp->need_force_hpd)
+		return -ETIMEDOUT;
+
+	/*
+	 * The eDP TRM indicate that if HPD_STATUS(RO) is 0, AUX CH
+	 * will not work, so we need to give a force hpd action to
+	 * set HPD_STATUS manually.
+	 */
+	dev_dbg(dp->dev, "failed to get hpd plug status, try to force hpd\n");
+
+	analogix_dp_force_hpd(dp);
+
+	if (analogix_dp_get_plug_in_status(dp) != 0) {
+		dev_err(dp->dev, "failed to get hpd plug in status\n");
+		return -EINVAL;
+	}
+
+	dev_dbg(dp->dev, "success to get plug in status after force hpd\n");
+
 	return 0;
 }
 
@@ -1287,6 +1310,9 @@  int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 	if (IS_ERR(dp->reg_base))
 		return PTR_ERR(dp->reg_base);
 
+	dp->need_force_hpd =
+		of_property_read_bool(dev->of_node, "analogix,need-force-hpd");
+
 	dp->hpd_gpio = of_get_named_gpio(dev->of_node, "hpd-gpios", 0);
 	if (gpio_is_valid(dp->hpd_gpio))
 		dp->hpd_gpio = of_get_named_gpio(dev->of_node,
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index d8945e2..6960ab3 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -160,6 +160,7 @@  struct analogix_dp_device {
 	struct phy		*phy;
 	int			dpms_mode;
 	int			hpd_gpio;
+	bool                    need_force_hpd;
 
 	struct analogix_dp_plat_data *plat_data;
 };
@@ -180,6 +181,7 @@  void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
 				       bool enable);
 void analogix_dp_init_analog_func(struct analogix_dp_device *dp);
 void analogix_dp_init_hpd(struct analogix_dp_device *dp);
+void analogix_dp_force_hpd(struct analogix_dp_device *dp);
 enum dp_irq_type analogix_dp_get_irq_type(struct analogix_dp_device *dp);
 void analogix_dp_clear_hotplug_interrupts(struct analogix_dp_device *dp);
 void analogix_dp_reset_aux(struct analogix_dp_device *dp);
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
index 15346fe..3086afc 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
@@ -365,6 +365,15 @@  void analogix_dp_init_hpd(struct analogix_dp_device *dp)
 	writel(reg, dp->reg_base + ANALOGIX_DP_SYS_CTL_3);
 }
 
+void analogix_dp_force_hpd(struct analogix_dp_device *dp)
+{
+	u32 reg;
+
+	reg = readl(dp->reg_base + ANALOGIX_DP_SYS_CTL_3);
+	reg = (F_HPD | HPD_CTRL);
+	writel(reg, dp->reg_base + ANALOGIX_DP_SYS_CTL_3);
+}
+
 enum dp_irq_type analogix_dp_get_irq_type(struct analogix_dp_device *dp)
 {
 	u32 reg;