| Message ID | 1441088079-25809-1-git-send-email-ykk@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > >
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 >> >> > >
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
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
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
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
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? :)
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
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 > > >
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
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
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
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;
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(-)