Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling
On Tue, Dec 20, 2016 at 03:33:48AM +0200, Laurent Pinchart wrote: > Instead of spreading version-dependent PHY power handling code around, > group it in two functions to power the PHY on and off and use them > through the driver. > > Powering off the PHY at the beginning of the setup phase is currently > split in two locations for first and second generation PHYs, group all > the operations in the dw_hdmi_phy_init() function. This changes the behaviour of the driver. > +static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) > +{ > + if (hdmi->phy->gen == 1) { > + dw_hdmi_phy_enable_tmds(hdmi, 0); > + dw_hdmi_phy_enable_powerdown(hdmi, true); > + } else { > + dw_hdmi_phy_gen2_txpwron(hdmi, 0); > + dw_hdmi_phy_gen2_pddq(hdmi, 1); > + } > +} > @@ -1290,9 +1302,7 @@ static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi) > if (!hdmi->phy_enabled) > return; > > - dw_hdmi_phy_enable_tmds(hdmi, 0); > - dw_hdmi_phy_enable_powerdown(hdmi, true); > - > + dw_hdmi_phy_power_off(hdmi); This makes dw_hdmi_phy_disable() power down a gen2 phy. The iMX6 has a DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY phy, which you list as a gen2 phy. I've been carrying this change for a while, which I've had to revert (and finally expunge), as it causes problems on iMX6: @@ -1112,6 +1112,14 @@ static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi) if (!hdmi->phy_enabled) return; + /* Actually set the phy into low power mode */ + dw_hdmi_phy_gen2_txpwron(hdmi, 0); + + /* FIXME: We should wait for TX_READY to be deasserted */ + + dw_hdmi_phy_gen2_pddq(hdmi, 1); + + /* This appears to have no effect on iMX6 */ dw_hdmi_phy_enable_tmds(hdmi, 0); dw_hdmi_phy_enable_powerdown(hdmi, true); So, I think your change here will cause problems for iMX6. >From what I remember, it seems that iMX6 has issues with RXSENSE/HPD bouncing when the PHY is powered down. I can't promise when I'll be able to check for that again. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling
Hi Russell, On 20-12-2016 11:45, Russell King - ARM Linux wrote: > On Tue, Dec 20, 2016 at 03:33:48AM +0200, Laurent Pinchart wrote: >> Instead of spreading version-dependent PHY power handling code around, >> group it in two functions to power the PHY on and off and use them >> through the driver. >> >> Powering off the PHY at the beginning of the setup phase is currently >> split in two locations for first and second generation PHYs, group all >> the operations in the dw_hdmi_phy_init() function. > This changes the behaviour of the driver. > >> +static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) >> +{ >> +if (hdmi->phy->gen == 1) { >> +dw_hdmi_phy_enable_tmds(hdmi, 0); >> +dw_hdmi_phy_enable_powerdown(hdmi, true); >> +} else { >> +dw_hdmi_phy_gen2_txpwron(hdmi, 0); >> +dw_hdmi_phy_gen2_pddq(hdmi, 1); >> +} >> +} >> @@ -1290,9 +1302,7 @@ static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi) >> if (!hdmi->phy_enabled) >> return; >> >> -dw_hdmi_phy_enable_tmds(hdmi, 0); >> -dw_hdmi_phy_enable_powerdown(hdmi, true); >> - >> +dw_hdmi_phy_power_off(hdmi); > This makes dw_hdmi_phy_disable() power down a gen2 phy. > > The iMX6 has a DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY phy, which you list as a > gen2 phy. I've been carrying this change for a while, which I've had > to revert (and finally expunge), as it causes problems on iMX6: > > @@ -1112,6 +1112,14 @@ static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi) > if (!hdmi->phy_enabled) > return; > > + /* Actually set the phy into low power mode */ > + dw_hdmi_phy_gen2_txpwron(hdmi, 0); > + > + /* FIXME: We should wait for TX_READY to be deasserted */ > + > + dw_hdmi_phy_gen2_pddq(hdmi, 1); > + > + /* This appears to have no effect on iMX6 */ > dw_hdmi_phy_enable_tmds(hdmi, 0); > dw_hdmi_phy_enable_powerdown(hdmi, true); > > So, I think your change here will cause problems for iMX6. > > From what I remember, it seems that iMX6 has issues with RXSENSE/HPD > bouncing when the PHY is powered down. I can't promise when I'll be > able to check for that again. > Indeed TX_READY must be low before asserting pddq. HPD and RXSENSE should work in power down mode by enabling enhpdrxsense bit in phy_conf0 BUT to enter power down you must disable TX_PWRON, enhpdrxsense and enable PDDQ and PHY _RESET. Only after doing this (and consequently entering power down mode) you can enable again enhpdrxsense. Best regards, Jose Miguel Abreu
Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling
Hi Russell, Thank you for the review. On Tuesday 20 Dec 2016 11:45:53 Russell King - ARM Linux wrote: > On Tue, Dec 20, 2016 at 03:33:48AM +0200, Laurent Pinchart wrote: > > Instead of spreading version-dependent PHY power handling code around, > > group it in two functions to power the PHY on and off and use them > > through the driver. > > > > Powering off the PHY at the beginning of the setup phase is currently > > split in two locations for first and second generation PHYs, group all > > the operations in the dw_hdmi_phy_init() function. > > This changes the behaviour of the driver. It does, but slightly only (I'm of course very aware that slightly can easily be way too much :-)). Let's analyse the differences, starting with PHY initialization in dw_hdmi_phy_init(). Before this patch, the function calls for (i = 0; i < 2; i++) { dw_hdmi_phy_sel_data_en_pol(hdmi, 1); dw_hdmi_phy_sel_interface_control(hdmi, 0); dw_hdmi_phy_enable_tmds(hdmi, 0); dw_hdmi_phy_enable_powerdown(hdmi, true); /* Enable CSC */ ret = hdmi_phy_configure(hdmi, cscon); if (ret) return ret; } and hdmi_phy_configure() starts with (I've removed lines that don't touch the hardware) /* Enable csc path */ if (cscon) val = HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH; else val = HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_BYPASS; hdmi_writeb(hdmi, val, HDMI_MC_FLOWCTRL); /* gen2 tx power off */ dw_hdmi_phy_gen2_txpwron(hdmi, 0); /* gen2 pddq */ dw_hdmi_phy_gen2_pddq(hdmi, 1); After the change, dw_hdmi_phy_init() calls for (i = 0; i < 2; i++) { dw_hdmi_phy_sel_data_en_pol(hdmi, 1); dw_hdmi_phy_sel_interface_control(hdmi, 0); /* Enable CSC */ ret = hdmi_phy_configure(hdmi, cscon); if (ret) return ret; } and hdmi_phy_configure() starts with /* Enable csc path */ if (cscon) val = HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH; else val = HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_BYPASS; hdmi_writeb(hdmi, val, HDMI_MC_FLOWCTRL); dw_hdmi_phy_power_off(hdmi); with dw_hdmi_phy_power_off() defined as if (hdmi->phy->gen == 1) { dw_hdmi_phy_enable_tmds(hdmi, 0); dw_hdmi_phy_enable_powerdown(hdmi, true); } else { dw_hdmi_phy_gen2_txpwron(hdmi, 0); dw_hdmi_phy_gen2_pddq(hdmi, 1); } This patch thus changes the behaviour in two ways: - Move the dw_hdmi_phy_enable_tmds() and dw_hdmi_phy_enable_powerdown() calls after CSC configuration (HDMI_MC_FLOWCTRL). - Only touch the power bits related to the PHY generation. I believe the first change to be harmless. Furthermore, given that the i.MX6 and Rockchip SoCs use a Gen2 PHY, the TMDS and POWERDOWN bits should be no- ops, so it should have no effect at all on those platforms. I also believe the second change to be harmless as the TMDS and POWERDOWN bits should be no-ops on i.MX6 and Rockchip. I have tested this patch series on RK3288 and i.MX6Q and haven't noticed any regression. The power on path should similarly be fine too, as the change - dw_hdmi_phy_enable_powerdown(hdmi, false); - - /* toggle TMDS enable */ - dw_hdmi_phy_enable_tmds(hdmi, 0); - dw_hdmi_phy_enable_tmds(hdmi, 1); - - /* gen2 tx power on */ - dw_hdmi_phy_gen2_txpwron(hdmi, 1); - dw_hdmi_phy_gen2_pddq(hdmi, 0); + dw_hdmi_phy_power_on(hdmi); results in static void dw_hdmi_phy_power_on(struct dw_hdmi *hdmi) { if (hdmi->phy->gen == 1) { dw_hdmi_phy_enable_powerdown(hdmi, false); /* Toggle TMDS enable. */ dw_hdmi_phy_enable_tmds(hdmi, 0); dw_hdmi_phy_enable_tmds(hdmi, 1); } else { dw_hdmi_phy_gen2_txpwron(hdmi, 1); dw_hdmi_phy_gen2_pddq(hdmi, 0); } } which splits the Gen1/Gen2 code but doesn't reorder anything. The last change is in dw_hdmi_phy_disable(), see below. > > +static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) > > +{ > > + if (hdmi->phy->gen == 1) { > > + dw_hdmi_phy_enable_tmds(hdmi, 0); > > + dw_hdmi_phy_enable_powerdown(hdmi, true); > > + } else { > > + dw_hdmi_phy_gen2_txpwron(hdmi, 0); > > + dw_hdmi_phy_gen2_pddq(hdmi, 1); > > + } > > +} > > > > @@ -1290,9 +1302,7 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > > *hdmi) > > if (!hdmi->phy_enabled) > > return; > > > > - dw_hdmi_phy_enable_tmds(hdmi, 0); > > - dw_hdmi_phy_enable_powerdown(hdmi, true); > > - > > + dw_hdmi_phy_power_off(hdmi); > > This makes dw_hdmi_phy_disable() power down
Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling
Hi Jose, On Tuesday 20 Dec 2016 12:17:23 Jose Abreu wrote: > On 20-12-2016 11:45, Russell King - ARM Linux wrote: > > On Tue, Dec 20, 2016 at 03:33:48AM +0200, Laurent Pinchart wrote: > >> Instead of spreading version-dependent PHY power handling code around, > >> group it in two functions to power the PHY on and off and use them > >> through the driver. > >> > >> Powering off the PHY at the beginning of the setup phase is currently > >> split in two locations for first and second generation PHYs, group all > >> the operations in the dw_hdmi_phy_init() function. > > > > This changes the behaviour of the driver. > > > >> +static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) > >> +{ > >> + if (hdmi->phy->gen == 1) { > >> + dw_hdmi_phy_enable_tmds(hdmi, 0); > >> + dw_hdmi_phy_enable_powerdown(hdmi, true); > >> + } else { > >> + dw_hdmi_phy_gen2_txpwron(hdmi, 0); > >> + dw_hdmi_phy_gen2_pddq(hdmi, 1); > >> + } > >> +} > >> @@ -1290,9 +1302,7 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > >> *hdmi) > >> > >>if (!hdmi->phy_enabled) > >> > >>return; > >> > >> - dw_hdmi_phy_enable_tmds(hdmi, 0); > >> - dw_hdmi_phy_enable_powerdown(hdmi, true); > >> - > >> + dw_hdmi_phy_power_off(hdmi); > > > > This makes dw_hdmi_phy_disable() power down a gen2 phy. > > > > The iMX6 has a DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY phy, which you list as a > > gen2 phy. I've been carrying this change for a while, which I've had > > to revert (and finally expunge), as it causes problems on iMX6: > > > > @@ -1112,6 +1112,14 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > > *hdmi)> > > if (!hdmi->phy_enabled) > > > > return; > > > > + /* Actually set the phy into low power mode */ > > + dw_hdmi_phy_gen2_txpwron(hdmi, 0); > > + > > + /* FIXME: We should wait for TX_READY to be deasserted */ > > + > > + dw_hdmi_phy_gen2_pddq(hdmi, 1); > > + > > + /* This appears to have no effect on iMX6 */ > > > > dw_hdmi_phy_enable_tmds(hdmi, 0); > > dw_hdmi_phy_enable_powerdown(hdmi, true); > > > > So, I think your change here will cause problems for iMX6. > > > > From what I remember, it seems that iMX6 has issues with RXSENSE/HPD > > bouncing when the PHY is powered down. I can't promise when I'll be > > able to check for that again. > > Indeed TX_READY must be low before asserting pddq. The TX_READY signal is documented in the i.MX6 datasheet as being a PHY output signal, but there seems to be no HDMI TX register from which its state can be read. Do I need to poll the HDMI_PHY_PTRPT_ENBL register through I2C ? How long is the PHY expected to take to set TX_READY to 0 ? > HPD and RXSENSE should work in power down mode by enabling enhpdrxsense > bit in phy_conf0 BUT to enter power down you must disable TX_PWRON, > enhpdrxsense and enable PDDQ and PHY_RESET. Only after doing this (and > consequently entering power down mode) you can enable again enhpdrxsense. Thanks, I'll give it a try. -- Regards, Laurent Pinchart
Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling
Hi Laurent, On 05-01-2017 12:29, Laurent Pinchart wrote: > Hi Jose, > > On Tuesday 20 Dec 2016 12:17:23 Jose Abreu wrote: >> On 20-12-2016 11:45, Russell King - ARM Linux wrote: >>> On Tue, Dec 20, 2016 at 03:33:48AM +0200, Laurent Pinchart wrote: Instead of spreading version-dependent PHY power handling code around, group it in two functions to power the PHY on and off and use them through the driver. Powering off the PHY at the beginning of the setup phase is currently split in two locations for first and second generation PHYs, group all the operations in the dw_hdmi_phy_init() function. >>> This changes the behaviour of the driver. >>> +static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) +{ + if (hdmi->phy->gen == 1) { + dw_hdmi_phy_enable_tmds(hdmi, 0); + dw_hdmi_phy_enable_powerdown(hdmi, true); + } else { + dw_hdmi_phy_gen2_txpwron(hdmi, 0); + dw_hdmi_phy_gen2_pddq(hdmi, 1); + } +} @@ -1290,9 +1302,7 @@ static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi) if (!hdmi->phy_enabled) return; - dw_hdmi_phy_enable_tmds(hdmi, 0); - dw_hdmi_phy_enable_powerdown(hdmi, true); - + dw_hdmi_phy_power_off(hdmi); >>> This makes dw_hdmi_phy_disable() power down a gen2 phy. >>> >>> The iMX6 has a DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY phy, which you list as a >>> gen2 phy. I've been carrying this change for a while, which I've had >>> to revert (and finally expunge), as it causes problems on iMX6: >>> >>> @@ -1112,6 +1112,14 @@ static void dw_hdmi_phy_disable(struct dw_hdmi >>> *hdmi)> >>> if (!hdmi->phy_enabled) >>> >>> return; >>> >>> + /* Actually set the phy into low power mode */ >>> + dw_hdmi_phy_gen2_txpwron(hdmi, 0); >>> + >>> + /* FIXME: We should wait for TX_READY to be deasserted */ >>> + >>> + dw_hdmi_phy_gen2_pddq(hdmi, 1); >>> + >>> + /* This appears to have no effect on iMX6 */ >>> >>> dw_hdmi_phy_enable_tmds(hdmi, 0); >>> dw_hdmi_phy_enable_powerdown(hdmi, true); >>> >>> So, I think your change here will cause problems for iMX6. >>> >>> From what I remember, it seems that iMX6 has issues with RXSENSE/HPD >>> bouncing when the PHY is powered down. I can't promise when I'll be >>> able to check for that again. >> Indeed TX_READY must be low before asserting pddq. > The TX_READY signal is documented in the i.MX6 datasheet as being a PHY > output > signal, but there seems to be no HDMI TX register from which its state can be > read. Do I need to poll the HDMI_PHY_PTRPT_ENBL register through I2C ? How > long is the PHY expected to take to set TX_READY to 0 ? TX_READY can be read from register 0x1A of phy, BIT(2) (through I2C). Not sure if same offset for all phys though. Best regards, Jose Miguel Abreu > >> HPD and RXSENSE should work in power down mode by enabling enhpdrxsense >> bit in phy_conf0 BUT to enter power down you must disable TX_PWRON, >> enhpdrxsense and enable PDDQ and PHY_RESET. Only after doing this (and >> consequently entering power down mode) you can enable again enhpdrxsense. > Thanks, I'll give it a try. >
Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling
Hi Jose, On Thursday 05 Jan 2017 15:06:49 Jose Abreu wrote: > On 05-01-2017 12:29, Laurent Pinchart wrote: > > On Tuesday 20 Dec 2016 12:17:23 Jose Abreu wrote: > >> On 20-12-2016 11:45, Russell King - ARM Linux wrote: > >>> On Tue, Dec 20, 2016 at 03:33:48AM +0200, Laurent Pinchart wrote: > Instead of spreading version-dependent PHY power handling code around, > group it in two functions to power the PHY on and off and use them > through the driver. > > Powering off the PHY at the beginning of the setup phase is currently > split in two locations for first and second generation PHYs, group all > the operations in the dw_hdmi_phy_init() function. > >>> > >>> This changes the behaviour of the driver. > >>> > +static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) > +{ > +if (hdmi->phy->gen == 1) { > +dw_hdmi_phy_enable_tmds(hdmi, 0); > +dw_hdmi_phy_enable_powerdown(hdmi, true); > +} else { > +dw_hdmi_phy_gen2_txpwron(hdmi, 0); > +dw_hdmi_phy_gen2_pddq(hdmi, 1); > +} > +} > @@ -1290,9 +1302,7 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > *hdmi) > > if (!hdmi->phy_enabled) > > return; > > -dw_hdmi_phy_enable_tmds(hdmi, 0); > -dw_hdmi_phy_enable_powerdown(hdmi, true); > - > +dw_hdmi_phy_power_off(hdmi); > >>> > >>> This makes dw_hdmi_phy_disable() power down a gen2 phy. > >>> > >>> The iMX6 has a DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY phy, which you list as a > >>> gen2 phy. I've been carrying this change for a while, which I've had > >>> to revert (and finally expunge), as it causes problems on iMX6: > >>> > >>> @@ -1112,6 +1112,14 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > >>> *hdmi)> > >>> > >>> if (!hdmi->phy_enabled) > >>> > >>> return; > >>> > >>> + /* Actually set the phy into low power mode */ > >>> + dw_hdmi_phy_gen2_txpwron(hdmi, 0); > >>> + > >>> + /* FIXME: We should wait for TX_READY to be deasserted */ > >>> + > >>> + dw_hdmi_phy_gen2_pddq(hdmi, 1); > >>> + > >>> + /* This appears to have no effect on iMX6 */ > >>> > >>> dw_hdmi_phy_enable_tmds(hdmi, 0); > >>> dw_hdmi_phy_enable_powerdown(hdmi, true); > >>> > >>> So, I think your change here will cause problems for iMX6. > >>> > >>> From what I remember, it seems that iMX6 has issues with RXSENSE/HPD > >>> bouncing when the PHY is powered down. I can't promise when I'll be > >>> able to check for that again. > >> > >> Indeed TX_READY must be low before asserting pddq. > > > > The TX_READY signal is documented in the i.MX6 datasheet as being a PHY > > output signal, but there seems to be no HDMI TX register from which its > > state can be read. Do I need to poll the HDMI_PHY_PTRPT_ENBL register > > through I2C ? How long is the PHY expected to take to set TX_READY to 0 ? > > TX_READY can be read from register 0x1A of phy, BIT(2) (through > I2C). That's what I thought, I'll poll that then. Do you have any idea how long it's supposed to take, to set an appropriate timeout ? > Not sure if same offset for all phys though. Most probably not, it would be too easy :-) I'll investigate (which will likely include lots of guesswork). If you can find any information about that (and especially about the MHL and HDMI 2.0 PHYs) that would be very appreciated, as I don't have access to any documentation that mentions a TX_READY bit for those. -- Regards, Laurent Pinchart
Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling
Hi Jose, On Thursday 05 Jan 2017 17:33:58 Laurent Pinchart wrote: > On Thursday 05 Jan 2017 15:06:49 Jose Abreu wrote: > > On 05-01-2017 12:29, Laurent Pinchart wrote: > >> On Tuesday 20 Dec 2016 12:17:23 Jose Abreu wrote: > >>> On 20-12-2016 11:45, Russell King - ARM Linux wrote: > On Tue, Dec 20, 2016 at 03:33:48AM +0200, Laurent Pinchart wrote: > > Instead of spreading version-dependent PHY power handling code > > around, group it in two functions to power the PHY on and off and use > > them through the driver. > > > > Powering off the PHY at the beginning of the setup phase is currently > > split in two locations for first and second generation PHYs, group > > all the operations in the dw_hdmi_phy_init() function. > > This changes the behaviour of the driver. > > > +static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) > > +{ > > + if (hdmi->phy->gen == 1) { > > + dw_hdmi_phy_enable_tmds(hdmi, 0); > > + dw_hdmi_phy_enable_powerdown(hdmi, true); > > + } else { > > + dw_hdmi_phy_gen2_txpwron(hdmi, 0); > > + dw_hdmi_phy_gen2_pddq(hdmi, 1); > > + } > > +} > > @@ -1290,9 +1302,7 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > > *hdmi) > > if (!hdmi->phy_enabled) > > return; > > > > - dw_hdmi_phy_enable_tmds(hdmi, 0); > > - dw_hdmi_phy_enable_powerdown(hdmi, true); > > - > > + dw_hdmi_phy_power_off(hdmi); > > This makes dw_hdmi_phy_disable() power down a gen2 phy. > > The iMX6 has a DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY phy, which you list as a > gen2 phy. I've been carrying this change for a while, which I've had > to revert (and finally expunge), as it causes problems on iMX6: > > @@ -1112,6 +1112,14 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > *hdmi) > if (!hdmi->phy_enabled) > return; > > + /* Actually set the phy into low power mode */ > + dw_hdmi_phy_gen2_txpwron(hdmi, 0); > + > + /* FIXME: We should wait for TX_READY to be deasserted */ > + > + dw_hdmi_phy_gen2_pddq(hdmi, 1); > + > + /* This appears to have no effect on iMX6 */ > dw_hdmi_phy_enable_tmds(hdmi, 0); > dw_hdmi_phy_enable_powerdown(hdmi, true); > > So, I think your change here will cause problems for iMX6. > > From what I remember, it seems that iMX6 has issues with RXSENSE/HPD > bouncing when the PHY is powered down. I can't promise when I'll be > able to check for that again. > >>> > >>> Indeed TX_READY must be low before asserting pddq. > >> > >> The TX_READY signal is documented in the i.MX6 datasheet as being a PHY > >> output signal, but there seems to be no HDMI TX register from which its > >> state can be read. Do I need to poll the HDMI_PHY_PTRPT_ENBL register > >> through I2C ? How long is the PHY expected to take to set TX_READY to 0 > >> ? > > > > TX_READY can be read from register 0x1A of phy, BIT(2) (through > > I2C). > > That's what I thought, I'll poll that then. Do you have any idea how long > it's supposed to take, to set an appropriate timeout ? On i.MX6 (3D TX PHY) register 0x1a reads as 0x0007 before powering down the PHY (by deasserting TXPWRON) and as 0x immediately after. On RK3288 (MHL PHY) the register reads as 0x0207 and as 0x immediately after deasserting TXPWRON. It seems that one I2C read is a sufficient delay for TX_READY to go low. > > Not sure if same offset for all phys though. > > Most probably not, it would be too easy :-) I'll investigate (which will > likely include lots of guesswork). If you can find any information about > that (and especially about the MHL and HDMI 2.0 PHYs) that would be very > appreciated, as I don't have access to any documentation that mentions a > TX_READY bit for those. I haven't tested the HDMI 2.0 PHY yet, but I'd be surprised if the TX_READY bit was in the same register at the same position. -- Regards, Laurent Pinchart
Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling
Hi Laurent, Sorry for the delayed answer but I am quite busy at the moment. On 06-01-2017 01:48, Laurent Pinchart wrote: [snip] The TX_READY signal is documented in the i.MX6 datasheet as being a PHY output signal, but there seems to be no HDMI TX register from which its state can be read. Do I need to poll the HDMI_PHY_PTRPT_ENBL register through I2C ? How long is the PHY expected to take to set TX_READY to 0 ? >>> TX_READY can be read from register 0x1A of phy, BIT(2) (through >>> I2C). >> That's what I thought, I'll poll that then. Do you have any idea how long >> it's supposed to take, to set an appropriate timeout ? For 3d tx phy and for 25 MHz input reference clock the power-up time is ~1ms, there is no data in the docs to power-down time but it should be similar. Reference clock value is board dependent and the minimum value for HDMI shall be 13.5MHz. > On i.MX6 (3D TX PHY) register 0x1a reads as 0x0007 before powering down the > PHY (by deasserting TXPWRON) and as 0x immediately after. On RK3288 (MHL > PHY) the register reads as 0x0207 and as 0x immediately after deasserting > TXPWRON. It seems that one I2C read is a sufficient delay for TX_READY to go > low. >>> Not sure if same offset for all phys though. >> Most probably not, it would be too easy :-) I'll investigate (which will >> likely include lots of guesswork). If you can find any information about >> that (and especially about the MHL and HDMI 2.0 PHYs) that would be very >> appreciated, as I don't have access to any documentation that mentions a >> TX_READY bit for those. > I haven't tested the HDMI 2.0 PHY yet, but I'd be surprised if the TX_READY > bit was in the same register at the same position. > The info I got the register offset is from an HDMI 2.0 phy :) Notice that there are a lot of phy versions and some of them (used in dw-hdmi) maybe customized, I don't think I have access to that custom phys documentation. Please test the HDMI 2.0 phy and check if the register is the same, it should be. In the meantime it would really be helpful if some of the developers that used dw-hdmi supplied this info about the registers as they should know exactly what phy was used. Best regards, Jose Miguel Abreu
Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling
Hi Jose, On Friday 06 Jan 2017 10:07:03 Jose Abreu wrote: > Hi Laurent, > > Sorry for the delayed answer but I am quite busy at the moment. No worries, your help is really appreciated. > On 06-01-2017 01:48, Laurent Pinchart wrote: > > [snip] > > The TX_READY signal is documented in the i.MX6 datasheet as being a PHY > output signal, but there seems to be no HDMI TX register from which its > state can be read. Do I need to poll the HDMI_PHY_PTRPT_ENBL register > through I2C ? How long is the PHY expected to take to set TX_READY to 0 > ? > >>> > >>> TX_READY can be read from register 0x1A of phy, BIT(2) (through > >>> I2C). > >> > >> That's what I thought, I'll poll that then. Do you have any idea how long > >> it's supposed to take, to set an appropriate timeout ? > > For 3d tx phy and for 25 MHz input reference clock the power-up > time is ~1ms, there is no data in the docs to power-down time but > it should be similar. Reference clock value is board dependent > and the minimum value for HDMI shall be 13.5MHz. > > > On i.MX6 (3D TX PHY) register 0x1a reads as 0x0007 before powering down > > the PHY (by deasserting TXPWRON) and as 0x immediately after. On > > RK3288 (MHL PHY) the register reads as 0x0207 and as 0x immediately > > after deasserting TXPWRON. It seems that one I2C read is a sufficient > > delay for TX_READY to go low. > > > >>> Not sure if same offset for all phys though. > >> > >> Most probably not, it would be too easy :-) I'll investigate (which will > >> likely include lots of guesswork). If you can find any information about > >> that (and especially about the MHL and HDMI 2.0 PHYs) that would be very > >> appreciated, as I don't have access to any documentation that mentions a > >> TX_READY bit for those. > > > > I haven't tested the HDMI 2.0 PHY yet, but I'd be surprised if the > > TX_READY bit was in the same register at the same position. > > The info I got the register offset is from an HDMI 2.0 phy :) That's good news :-) > Notice that there are a lot of phy versions and some of them > (used in dw-hdmi) maybe customized, I don't think I have access > to that custom phys documentation. I think we will eventually have to implement PHY-specific power up and power down sequences, but for now a common sequence should work. > Please test the HDMI 2.0 phy and check if the register is the same, it > should be. I did, and it is \o/ I'll send patches. > In the meantime it would really be helpful if some of the developers > that used dw-hdmi supplied this info about the registers as they > should know exactly what phy was used. I will ask internally for the R-Car H3 SoC. -- Regards, Laurent Pinchart
Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling
Hi Jose, On Thursday 05 Jan 2017 15:06:49 Jose Abreu wrote: > On 05-01-2017 12:29, Laurent Pinchart wrote: > > On Tuesday 20 Dec 2016 12:17:23 Jose Abreu wrote: > >> On 20-12-2016 11:45, Russell King - ARM Linux wrote: > >>> On Tue, Dec 20, 2016 at 03:33:48AM +0200, Laurent Pinchart wrote: > Instead of spreading version-dependent PHY power handling code around, > group it in two functions to power the PHY on and off and use them > through the driver. > > Powering off the PHY at the beginning of the setup phase is currently > split in two locations for first and second generation PHYs, group all > the operations in the dw_hdmi_phy_init() function. > >>> > >>> This changes the behaviour of the driver. > >>> > +static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) > +{ > +if (hdmi->phy->gen == 1) { > +dw_hdmi_phy_enable_tmds(hdmi, 0); > +dw_hdmi_phy_enable_powerdown(hdmi, true); > +} else { > +dw_hdmi_phy_gen2_txpwron(hdmi, 0); > +dw_hdmi_phy_gen2_pddq(hdmi, 1); > +} > +} > @@ -1290,9 +1302,7 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > *hdmi) > if (!hdmi->phy_enabled) > return; > > -dw_hdmi_phy_enable_tmds(hdmi, 0); > -dw_hdmi_phy_enable_powerdown(hdmi, true); > - > +dw_hdmi_phy_power_off(hdmi); > >>> > >>> This makes dw_hdmi_phy_disable() power down a gen2 phy. > >>> > >>> The iMX6 has a DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY phy, which you list as a > >>> gen2 phy. I've been carrying this change for a while, which I've had > >>> to revert (and finally expunge), as it causes problems on iMX6: > >>> > >>> @@ -1112,6 +1112,14 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > >>> *hdmi) > >>> if (!hdmi->phy_enabled) > >>> return; > >>> > >>> + /* Actually set the phy into low power mode */ > >>> + dw_hdmi_phy_gen2_txpwron(hdmi, 0); > >>> + > >>> + /* FIXME: We should wait for TX_READY to be deasserted */ > >>> + > >>> + dw_hdmi_phy_gen2_pddq(hdmi, 1); > >>> + > >>> + /* This appears to have no effect on iMX6 */ > >>> dw_hdmi_phy_enable_tmds(hdmi, 0); > >>> dw_hdmi_phy_enable_powerdown(hdmi, true); > >>> > >>> So, I think your change here will cause problems for iMX6. > >>> > >>> From what I remember, it seems that iMX6 has issues with RXSENSE/HPD > >>> bouncing when the PHY is powered down. I can't promise when I'll be > >>> able to check for that again. > >> > >> Indeed TX_READY must be low before asserting pddq. > > > > The TX_READY signal is documented in the i.MX6 datasheet as being a PHY > > output signal, but there seems to be no HDMI TX register from which its > > state can be read. Do I need to poll the HDMI_PHY_PTRPT_ENBL register > > through I2C ? How long is the PHY expected to take to set TX_READY to 0 ? > > TX_READY can be read from register 0x1A of phy, BIT(2) (through > I2C). Not sure if same offset for all phys though. I have been told that another option is to poll the TX_PHY_LOCK bit in the phy_stat0 register. That would be a simpler solution that doesn't require I2C access. Could you confirm (or disconfirm) this ? > >> HPD and RXSENSE should work in power down mode by enabling enhpdrxsense > >> bit in phy_conf0 BUT to enter power down you must disable TX_PWRON, > >> enhpdrxsense and enable PDDQ and PHY_RESET. Only after doing this (and > >> consequently entering power down mode) you can enable again enhpdrxsense. > > > > Thanks, I'll give it a try. -- Regards, Laurent Pinchart
Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling
Hi Laurent, On 01-03-2017 11:09, Laurent Pinchart wrote: > Hi Jose, > > On Thursday 05 Jan 2017 15:06:49 Jose Abreu wrote: >> On 05-01-2017 12:29, Laurent Pinchart wrote: >>> On Tuesday 20 Dec 2016 12:17:23 Jose Abreu wrote: On 20-12-2016 11:45, Russell King - ARM Linux wrote: > On Tue, Dec 20, 2016 at 03:33:48AM +0200, Laurent Pinchart wrote: >> Instead of spreading version-dependent PHY power handling code around, >> group it in two functions to power the PHY on and off and use them >> through the driver. >> >> Powering off the PHY at the beginning of the setup phase is currently >> split in two locations for first and second generation PHYs, group all >> the operations in the dw_hdmi_phy_init() function. > This changes the behaviour of the driver. > >> +static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) >> +{ >> +if (hdmi->phy->gen == 1) { >> +dw_hdmi_phy_enable_tmds(hdmi, 0); >> +dw_hdmi_phy_enable_powerdown(hdmi, true); >> +} else { >> +dw_hdmi_phy_gen2_txpwron(hdmi, 0); >> +dw_hdmi_phy_gen2_pddq(hdmi, 1); >> +} >> +} >> @@ -1290,9 +1302,7 @@ static void dw_hdmi_phy_disable(struct dw_hdmi >> *hdmi) >> if (!hdmi->phy_enabled) >> return; >> >> -dw_hdmi_phy_enable_tmds(hdmi, 0); >> -dw_hdmi_phy_enable_powerdown(hdmi, true); >> - >> +dw_hdmi_phy_power_off(hdmi); > This makes dw_hdmi_phy_disable() power down a gen2 phy. > > The iMX6 has a DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY phy, which you list as a > gen2 phy. I've been carrying this change for a while, which I've had > to revert (and finally expunge), as it causes problems on iMX6: > > @@ -1112,6 +1112,14 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > *hdmi) > if (!hdmi->phy_enabled) > return; > > + /* Actually set the phy into low power mode */ > + dw_hdmi_phy_gen2_txpwron(hdmi, 0); > + > + /* FIXME: We should wait for TX_READY to be deasserted */ > + > + dw_hdmi_phy_gen2_pddq(hdmi, 1); > + > + /* This appears to have no effect on iMX6 */ > dw_hdmi_phy_enable_tmds(hdmi, 0); > dw_hdmi_phy_enable_powerdown(hdmi, true); > > So, I think your change here will cause problems for iMX6. > > From what I remember, it seems that iMX6 has issues with RXSENSE/HPD > bouncing when the PHY is powered down. I can't promise when I'll be > able to check for that again. Indeed TX_READY must be low before asserting pddq. >>> The TX_READY signal is documented in the i.MX6 datasheet as being a PHY >>> output signal, but there seems to be no HDMI TX register from which its >>> state can be read. Do I need to poll the HDMI_PHY_PTRPT_ENBL register >>> through I2C ? How long is the PHY expected to take to set TX_READY to 0 ? >> TX_READY can be read from register 0x1A of phy, BIT(2) (through >> I2C). Not sure if same offset for all phys though. > I have been told that another option is to poll the TX_PHY_LOCK bit in the > phy_stat0 register. That would be a simpler solution that doesn't require I2C > access. Could you confirm (or disconfirm) this ? Yes (In our implementation we have TX_PHY_LOCK wired up to TX_READY that comes from phy. But if using a custom phy or an unusual implementation then this may not hold). Best regards, Jose Miguel Abreu HPD and RXSENSE should work in power down mode by enabling enhpdrxsense bit in phy_conf0 BUT to enter power down you must disable TX_PWRON, enhpdrxsense and enable PDDQ and PHY_RESET. Only after doing this (and consequently entering power down mode) you can enable again enhpdrxsense. >>> Thanks, I'll give it a try.
Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling
Hi Jose, On Wednesday 01 Mar 2017 16:25:46 Jose Abreu wrote: > On 01-03-2017 11:09, Laurent Pinchart wrote: > > On Thursday 05 Jan 2017 15:06:49 Jose Abreu wrote: > >> On 05-01-2017 12:29, Laurent Pinchart wrote: > >>> On Tuesday 20 Dec 2016 12:17:23 Jose Abreu wrote: > On 20-12-2016 11:45, Russell King - ARM Linux wrote: > > On Tue, Dec 20, 2016 at 03:33:48AM +0200, Laurent Pinchart wrote: > >> Instead of spreading version-dependent PHY power handling code > >> around, group it in two functions to power the PHY on and off and use > >> them through the driver. > >> > >> Powering off the PHY at the beginning of the setup phase is currently > >> split in two locations for first and second generation PHYs, group > >> all the operations in the dw_hdmi_phy_init() function. > > > > This changes the behaviour of the driver. > > > >> +static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) > >> +{ > >> + if (hdmi->phy->gen == 1) { > >> + dw_hdmi_phy_enable_tmds(hdmi, 0); > >> + dw_hdmi_phy_enable_powerdown(hdmi, true); > >> + } else { > >> + dw_hdmi_phy_gen2_txpwron(hdmi, 0); > >> + dw_hdmi_phy_gen2_pddq(hdmi, 1); > >> + } > >> +} > >> @@ -1290,9 +1302,7 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > >> *hdmi) > >>if (!hdmi->phy_enabled) > >>return; > >> > >> - dw_hdmi_phy_enable_tmds(hdmi, 0); > >> - dw_hdmi_phy_enable_powerdown(hdmi, true); > >> - > >> + dw_hdmi_phy_power_off(hdmi); > > > > This makes dw_hdmi_phy_disable() power down a gen2 phy. > > > > The iMX6 has a DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY phy, which you list as a > > gen2 phy. I've been carrying this change for a while, which I've had > > to revert (and finally expunge), as it causes problems on iMX6: > > > > @@ -1112,6 +1112,14 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > > *hdmi) > > if (!hdmi->phy_enabled) > > return; > > > > + /* Actually set the phy into low power mode */ > > + dw_hdmi_phy_gen2_txpwron(hdmi, 0); > > + > > + /* FIXME: We should wait for TX_READY to be deasserted */ > > + > > + dw_hdmi_phy_gen2_pddq(hdmi, 1); > > + > > + /* This appears to have no effect on iMX6 */ > > dw_hdmi_phy_enable_tmds(hdmi, 0); > > dw_hdmi_phy_enable_powerdown(hdmi, true); > > > > So, I think your change here will cause problems for iMX6. > > > > From what I remember, it seems that iMX6 has issues with RXSENSE/HPD > > bouncing when the PHY is powered down. I can't promise when I'll be > > able to check for that again. > > Indeed TX_READY must be low before asserting pddq. > >>> > >>> The TX_READY signal is documented in the i.MX6 datasheet as being a PHY > >>> output signal, but there seems to be no HDMI TX register from which its > >>> state can be read. Do I need to poll the HDMI_PHY_PTRPT_ENBL register > >>> through I2C ? How long is the PHY expected to take to set TX_READY to 0 > >>> ? > >> > >> TX_READY can be read from register 0x1A of phy, BIT(2) (through > >> I2C). Not sure if same offset for all phys though. > > > > I have been told that another option is to poll the TX_PHY_LOCK bit in the > > phy_stat0 register. That would be a simpler solution that doesn't require > > I2C access. Could you confirm (or disconfirm) this ? > > Yes (In our implementation we have TX_PHY_LOCK wired up to > TX_READY that comes from phy. But if using a custom phy or an > unusual implementation then this may not hold). Thank you for the information. I've submitted a v4 that poll the TX_PHY_LOCK bit. With the patch series applied the vendor PHY handling code already delegates PHY power control to the glue code, there's thus no issue there. For platforms using a Synopsys PHY, all the ones we have to support today have TX_READY wired to the PHY lock signal so we should be good as well there. If that doesn't remain true in the future we'll fix it. > HPD and RXSENSE should work in power down mode by enabling enhpdrxsense > bit in phy_conf0 BUT to enter power down you must disable TX_PWRON, > enhpdrxsense and enable PDDQ and PHY_RESET. Only after doing this (and > consequently entering power down mode) you can enable again > enhpdrxsense. > >>> > >>> Thanks, I'll give it a try. -- Regards, Laurent Pinchart