Hi Yakir,
On Thu, Oct 29, 2015 at 09:58:38AM +0800, Yakir Yang wrote:
> Add phy driver for the Rockchip DisplayPort PHY module. This
> is required to get DisplayPort working in Rockchip SoCs.
>
> Reviewed-by: Heiko Stuebner <[email protected]>
> Signed-off-by: Yakir Yang <[email protected]>
> ---
...
> diff --git a/drivers/phy/phy-rockchip-dp.c b/drivers/phy/phy-rockchip-dp.c
> new file mode 100644
> index 0000000..f3e0058
> --- /dev/null
> +++ b/drivers/phy/phy-rockchip-dp.c
> @@ -0,0 +1,151 @@
> +/*
> + * Rockchip DP PHY driver
> + *
> + * Copyright (C) 2015 FuZhou Rockchip Co., Ltd.
> + * Author: Yakir Yang <ykk@@rock-chips.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/clk.h>
> +#include <linux/phy/phy.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +
> +#define GRF_SOC_CON12 0x0274
> +
> +#define GRF_EDP_REF_CLK_SEL_INTER_HIWORD_MASK BIT(4)
> +#define GRF_EDP_REF_CLK_SEL_INTER BIT(4)
Why are the above two macros the same? Judging by the RK3288 manual and
other downstream drivers, it seems like you want the _MASK one to be
shifted left by 16 (i.e., BIT(20)).
> +
> +#define GRF_EDP_PHY_SIDDQ_HIWORD_MASK BIT(21)
> +#define GRF_EDP_PHY_SIDDQ_ON 0
> +#define GRF_EDP_PHY_SIDDQ_OFF BIT(5)
> +
...
> + ret = regmap_write(dp->grf, GRF_SOC_CON12, GRF_EDP_REF_CLK_SEL_INTER |
> + GRF_EDP_REF_CLK_SEL_INTER_HIWORD_MASK);
IOW, the above is writing:
BIT(4) | BIT(4)
whereas I think you want:
BIT(4) | BIT(20)
> + if (ret != 0) {
> + dev_err(dp->dev, "Could not config GRF edp ref clk: %d\n", ret);
> + return ret;
> + }
> +
...
(FYI, I came across this by inspection when comparing Heiko's
'somewhat-stable' branch [1] with this series. The former brings up eDP
fine on veyron-jaq, whereas this one doesn't yet, even with the above
change. Still debugging the issue.)
Brian
[1] https://github.com/mmind/linux-rockchip/tree/devel/somewhat-stable
Hi Brain,
On 11/03/2015 12:38 PM, Brian Norris wrote:
> Hi Yakir,
>
> On Thu, Oct 29, 2015 at 09:58:38AM +0800, Yakir Yang wrote:
>> Add phy driver for the Rockchip DisplayPort PHY module. This
>> is required to get DisplayPort working in Rockchip SoCs.
>>
>> Reviewed-by: Heiko Stuebner<[email protected]>
>> Signed-off-by: Yakir Yang<[email protected]>
>> ---
> ...
>> diff --git a/drivers/phy/phy-rockchip-dp.c b/drivers/phy/phy-rockchip-dp.c
>> new file mode 100644
>> index 0000000..f3e0058
>> --- /dev/null
>> +++ b/drivers/phy/phy-rockchip-dp.c
>> @@ -0,0 +1,151 @@
>> +/*
>> + * Rockchip DP PHY driver
>> + *
>> + * Copyright (C) 2015 FuZhou Rockchip Co., Ltd.
>> + * Author: Yakir Yang<ykk@@rock-chips.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/clk.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/platform_device.h>
>> +
>> +#define GRF_SOC_CON12 0x0274
>> +
>> +#define GRF_EDP_REF_CLK_SEL_INTER_HIWORD_MASK BIT(4)
>> +#define GRF_EDP_REF_CLK_SEL_INTER BIT(4)
> Why are the above two macros the same? Judging by the RK3288 manual and
> other downstream drivers, it seems like you want the _MASK one to be
> shifted left by 16 (i.e., BIT(20)).
Oops, yes, you're right, it should be BIT(20), thanks for the catch.
>> +
>> +#define GRF_EDP_PHY_SIDDQ_HIWORD_MASK BIT(21)
>> +#define GRF_EDP_PHY_SIDDQ_ON 0
>> +#define GRF_EDP_PHY_SIDDQ_OFF BIT(5)
>> +
> ...
>
>> + ret = regmap_write(dp->grf, GRF_SOC_CON12, GRF_EDP_REF_CLK_SEL_INTER |
>> + GRF_EDP_REF_CLK_SEL_INTER_HIWORD_MASK);
> IOW, the above is writing:
>
> BIT(4) | BIT(4)
>
> whereas I think you want:
>
> BIT(4) | BIT(20)
>
>> + if (ret != 0) {
>> + dev_err(dp->dev, "Could not config GRF edp ref clk: %d\n", ret);
>> + return ret;
>> + }
>> +
> ...
>
> (FYI, I came across this by inspection when comparing Heiko's
> 'somewhat-stable' branch [1] with this series. The former brings up eDP
> fine on veyron-jaq, whereas this one doesn't yet, even with the above
> change. Still debugging the issue.)
Hmm... I'm not sure whether your eDP screen have the hotplug signal, so I
think you can try to add "analogix,force-hpd" flag into
rk3288-veyron-jaq.dts
&edp {
analogix,need-force-hpd;
}
If that changes couldn't fix the problem, guess you may need max the panel
power up delay time which pointed by Heiko. Like:
Thanks,
- Yakir
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 4fa5f69..546a506 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -82,6 +82,13 @@ static int analogix_dp_detect_hpd(struct
analogix_dp_device *dp)
*/
dev_dbg(dp->dev, "failed to get hpd plug status, try to force
hpd\n");
+ /*
+ * Hotplug signal would indicate the right time to operate
+ * panel after poweron, but if the hotplug is missing, then
+ * panel would need wait hundreds of milliseconds at here.
+ */
+ mdelay(100);
+
analogix_dp_force_hpd(dp);
if (analogix_dp_get_plug_in_status(dp) != 0) {
> Brian
>
> [1]https://github.com/mmind/linux-rockchip/tree/devel/somewhat-stable
>
>
>
Hi Yakir,
On Wed, Nov 04, 2015 at 08:48:38AM +0800, Yakir Yang wrote:
> On 11/03/2015 12:38 PM, Brian Norris wrote:
> >On Thu, Oct 29, 2015 at 09:58:38AM +0800, Yakir Yang wrote:
> >(FYI, I came across this by inspection when comparing Heiko's
> >'somewhat-stable' branch [1] with this series. The former brings up eDP
> >fine on veyron-jaq, whereas this one doesn't yet, even with the above
> >change. Still debugging the issue.)
>
> Hmm... I'm not sure whether your eDP screen have the hotplug signal, so I
I believe hotplug is hooked up but...
> think you can try to add "analogix,force-hpd" flag into
> rk3288-veyron-jaq.dts
>
> &edp {
> analogix,need-force-hpd;
> }
...already tried, just in case. No luck.
> If that changes couldn't fix the problem, guess you may need max the panel
> power up delay time which pointed by Heiko. Like:
>
> Thanks,
> - Yakir
>
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 4fa5f69..546a506 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -82,6 +82,13 @@ static int analogix_dp_detect_hpd(struct
> analogix_dp_device *dp)
> */
> dev_dbg(dp->dev, "failed to get hpd plug status, try to
> force hpd\n");
>
> + /*
> + * Hotplug signal would indicate the right time to operate
> + * panel after poweron, but if the hotplug is missing, then
> + * panel would need wait hundreds of milliseconds at here.
> + */
> + mdelay(100);
> +
> analogix_dp_force_hpd(dp);
>
> if (analogix_dp_get_plug_in_status(dp) != 0) {
I'll give that a try. Per Heiko's suggestion, I've already hacked around
with adding delays in the regulator node, but this could give slightly
different behavior. I doubt it'll help, but I'll let you know if it
does.
Regards,
Brian
Hi,
A few updates:
On Tue, Nov 03, 2015 at 05:13:48PM -0800, Brian Norris wrote:
> On Wed, Nov 04, 2015 at 08:48:38AM +0800, Yakir Yang wrote:
> > On 11/03/2015 12:38 PM, Brian Norris wrote:
> > >On Thu, Oct 29, 2015 at 09:58:38AM +0800, Yakir Yang wrote:
> > >(FYI, I came across this by inspection when comparing Heiko's
> > >'somewhat-stable' branch [1] with this series. The former brings up eDP
> > >fine on veyron-jaq, whereas this one doesn't yet, even with the above
> > >change. Still debugging the issue.)
Some time after the above comment, I managed to kill the panel on my
Jaq :( I think the wiring around the hinge was a bit flaky, and it
finally went out for good.
> > Hmm... I'm not sure whether your eDP screen have the hotplug signal, so I
>
> I believe hotplug is hooked up but...
>
> > think you can try to add "analogix,force-hpd" flag into
> > rk3288-veyron-jaq.dts
> >
> > &edp {
> > analogix,need-force-hpd;
> > }
>
> ...already tried, just in case. No luck.
However, now when testing a different Jaq device, now this series +
Heiko's DTS updates + the "analogix,force-hpd" (i.e., [1]) works fine,
modulo a few log warnings, some of which are probably expected (for
instance, I believe the EDID is known not-so-helpful). Snippets:
[ 3.170176] rockchip-dp ff970000.dp: AUX CH command reply failed!
[ 3.178058] rockchip-dp ff970000.dp: AUX CH command reply failed!
[ 3.184166] rockchip-dp ff970000.dp: unable to handle edid
and later:
[ 3.953300] rockchip-dp ff970000.dp: EDID data does not include any extensions.
[ 3.966731] rockchip-dp ff970000.dp: EDID data does not include any extensions.
[ 3.979409] rockchip-dp ff970000.dp: EDID data does not include any extensions.
[ 3.998730] rockchip-dp ff970000.dp: Link Training Clock Recovery success
[ 4.007046] rockchip-dp ff970000.dp: Link Training success!
[ 4.115040] rockchip-dp ff970000.dp: Timeout of video streamclk ok
[ 4.121211] rockchip-dp ff970000.dp: unable to config video
[ 4.127616] rockchip-dp ff970000.dp: EDID data does not include any extensions.
So, I'll chalk that earlier failure up to a hardware failure (or
possibly a still yet-undiagnosed hardware difference; my new Jaq has
some small differences from the previous unit).
Also, it's still not real clear why HPD isn't working upstream (and we
have to use the "force-hpd" property), when it appears to work on our
downstream Chrome OS tree.
Finally, I'll leave you with some small bits I've noticed from exploring
this issue on Jaq:
* The Chrome OS driver for this IP has a much longer timeout in (the
equivalent of) analogix_dp_detect_hpd; it polls in 10-20 ms intervals
(rather than 10-11 us) and takes something around 60 to 120 ms to
notice the panel.
* AFAICT, the Chrome OS driver never actually used the HPD interrupt;
it was only polling the HPD status bit. So I can't claim that the
functionality that Yakir is supporting here has ever been tested on
these platforms. (Now, I'm not sure this is extremely important,
since we still can fall back to polled status checks; see
drm_kms_helper_poll_init().)
That's all I've got for now.
Regards,
Brian
[1] https://github.com/mmind/linux-rockchip/commits/tmp/analogixdp-veyron
plus this diff:
diff --git a/arch/arm/boot/dts/rk3288-veyron-jaq.dts b/arch/arm/boot/dts/rk3288-veyron-jaq.dts
index 5c97e3153526..e77ae4c5531e 100644
--- a/arch/arm/boot/dts/rk3288-veyron-jaq.dts
+++ b/arch/arm/boot/dts/rk3288-veyron-jaq.dts
@@ -88,6 +88,18 @@
};
};
+&backlight {
+ power-supply = <&backlight_regulator>;
+};
+
+&panel {
+ power-supply = <&panel_regulator>;
+};
+
+&edp {
+ analogix,need-force-hpd;
+};
+
&rk808 {
pinctrl-names = "default";
pinctrl-0 = <&pmic_int_l &dvs_1 &dvs_2>;
diff --git a/drivers/phy/phy-rockchip-dp.c b/drivers/phy/phy-rockchip-dp.c
index c82c22f3d0e1..994189f49db5 100644
--- a/drivers/phy/phy-rockchip-dp.c
+++ b/drivers/phy/phy-rockchip-dp.c
@@ -22,7 +22,7 @@
#define GRF_SOC_CON12 0x0274
-#define GRF_EDP_REF_CLK_SEL_INTER_HIWORD_MASK BIT(4)
+#define GRF_EDP_REF_CLK_SEL_INTER_HIWORD_MASK BIT(20)
#define GRF_EDP_REF_CLK_SEL_INTER BIT(4)
#define GRF_EDP_PHY_SIDDQ_HIWORD_MASK BIT(21)
Hi Brian,
Thank you for debugging, and fell sorry for the delay reply
On 11/06/2015 07:45 AM, Brian Norris wrote:
> Hi,
>
> A few updates:
>
> On Tue, Nov 03, 2015 at 05:13:48PM -0800, Brian Norris wrote:
>> On Wed, Nov 04, 2015 at 08:48:38AM +0800, Yakir Yang wrote:
>>> On 11/03/2015 12:38 PM, Brian Norris wrote:
>>>> On Thu, Oct 29, 2015 at 09:58:38AM +0800, Yakir Yang wrote:
>>>> (FYI, I came across this by inspection when comparing Heiko's
>>>> 'somewhat-stable' branch [1] with this series. The former brings up eDP
>>>> fine on veyron-jaq, whereas this one doesn't yet, even with the above
>>>> change. Still debugging the issue.)
> Some time after the above comment, I managed to kill the panel on my
> Jaq :( I think the wiring around the hinge was a bit flaky, and it
> finally went out for good.
>
>>> Hmm... I'm not sure whether your eDP screen have the hotplug signal, so I
>> I believe hotplug is hooked up but...
>>
>>> think you can try to add "analogix,force-hpd" flag into
>>> rk3288-veyron-jaq.dts
>>>
>>> &edp {
>>> analogix,need-force-hpd;
>>> }
>> ...already tried, just in case. No luck.
> However, now when testing a different Jaq device, now this series +
> Heiko's DTS updates + the "analogix,force-hpd" (i.e., [1]) works fine,
> modulo a few log warnings, some of which are probably expected (for
> instance, I believe the EDID is known not-so-helpful). Snippets:
>
> [ 3.170176] rockchip-dp ff970000.dp: AUX CH command reply failed!
> [ 3.178058] rockchip-dp ff970000.dp: AUX CH command reply failed!
> [ 3.184166] rockchip-dp ff970000.dp: unable to handle edid
>
> and later:
>
> [ 3.953300] rockchip-dp ff970000.dp: EDID data does not include any extensions.
> [ 3.966731] rockchip-dp ff970000.dp: EDID data does not include any extensions.
> [ 3.979409] rockchip-dp ff970000.dp: EDID data does not include any extensions.
> [ 3.998730] rockchip-dp ff970000.dp: Link Training Clock Recovery success
> [ 4.007046] rockchip-dp ff970000.dp: Link Training success!
> [ 4.115040] rockchip-dp ff970000.dp: Timeout of video streamclk ok
> [ 4.121211] rockchip-dp ff970000.dp: unable to config video
> [ 4.127616] rockchip-dp ff970000.dp: EDID data does not include any extensions.
>
>
> So, I'll chalk that earlier failure up to a hardware failure (or
> possibly a still yet-undiagnosed hardware difference; my new Jaq has
> some small differences from the previous unit).
Yeah, I have saw this failed on some chromebook too, but finally it transmit
successfully, so I guess this maybe caused by hardware different.
>
> Also, it's still not real clear why HPD isn't working upstream (and we
> have to use the "force-hpd" property), when it appears to work on our
> downstream Chrome OS tree.
I have tested that driver do receive the hpd interrupt when i
plugged/unplugged the
eDP screen, and at that time driver would read the HPD connected status
rightly.
But I haven't fingered out why driver couldn't get the right HPD status
in the early time,
need debug more...
> Finally, I'll leave you with some small bits I've noticed from exploring
> this issue on Jaq:
>
> * The Chrome OS driver for this IP has a much longer timeout in (the
> equivalent of) analogix_dp_detect_hpd; it polls in 10-20 ms intervals
> (rather than 10-11 us) and takes something around 60 to 120 ms to
> notice the panel.
Thanks, I have noticed it too, also I try to expand the hotplug timeout,
but Jingoo
suggested not to do that, cause it would bring external delay on Exynos
platform.
Besides from what Heiko reply, expand this delay time wouldn't give any help
on those Jaq no display case, delay the panel power up time would be the
right
way to fix that, so maybe no need to expand the hotpulg timeout.
- Thanks
> * AFAICT, the Chrome OS driver never actually used the HPD interrupt;
> it was only polling the HPD status bit. So I can't claim that the
> functionality that Yakir is supporting here has ever been tested on
> these platforms. (Now, I'm not sure this is extremely important,
> since we still can fall back to polled status checks; see
> drm_kms_helper_poll_init().)
> That's all I've got for now.
>
> Regards,
> Brian
>
> [1] https://github.com/mmind/linux-rockchip/commits/tmp/analogixdp-veyron
>
> plus this diff:
>
> diff --git a/arch/arm/boot/dts/rk3288-veyron-jaq.dts b/arch/arm/boot/dts/rk3288-veyron-jaq.dts
> index 5c97e3153526..e77ae4c5531e 100644
> --- a/arch/arm/boot/dts/rk3288-veyron-jaq.dts
> +++ b/arch/arm/boot/dts/rk3288-veyron-jaq.dts
> @@ -88,6 +88,18 @@
> };
> };
>
> +&backlight {
> + power-supply = <&backlight_regulator>;
> +};
> +
> +&panel {
> + power-supply = <&panel_regulator>;
> +};
> +
> +&edp {
> + analogix,need-force-hpd;
> +};
> +
> &rk808 {
> pinctrl-names = "default";
> pinctrl-0 = <&pmic_int_l &dvs_1 &dvs_2>;
> diff --git a/drivers/phy/phy-rockchip-dp.c b/drivers/phy/phy-rockchip-dp.c
> index c82c22f3d0e1..994189f49db5 100644
> --- a/drivers/phy/phy-rockchip-dp.c
> +++ b/drivers/phy/phy-rockchip-dp.c
> @@ -22,7 +22,7 @@
>
> #define GRF_SOC_CON12 0x0274
>
> -#define GRF_EDP_REF_CLK_SEL_INTER_HIWORD_MASK BIT(4)
> +#define GRF_EDP_REF_CLK_SEL_INTER_HIWORD_MASK BIT(20)
> #define GRF_EDP_REF_CLK_SEL_INTER BIT(4)
>
> #define GRF_EDP_PHY_SIDDQ_HIWORD_MASK BIT(21)
>
>
>