[PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling

[Laurent Pinchart]

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 0x0000 immediately after. On RK3288 (MHL 
PHY) the register reads as 0x0207 and as 0x0000 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