Re: [PATCH v2 16/29] drm: bridge: dw-hdmi: Detect PHY type at runtime

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jose,

On Tuesday 20 Dec 2016 15:01:52 Jose Abreu wrote:
> On 20-12-2016 13:11, Laurent Pinchart wrote:
> > On Tuesday 20 Dec 2016 11:39:21 Jose Abreu wrote:
> >> On 20-12-2016 01:33, Laurent Pinchart wrote:
> >>> Detect the PHY type and use it to handle the PHY type-specific SVSRET
> >>> signal.
> >>> 
> >>> Signed-off-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> ---
> >>> 
> >>>  drivers/gpu/drm/bridge/dw-hdmi.c | 68 +++++++++++++++++++++++++++++++--
> >>>  include/drm/bridge/dw_hdmi.h     | 10 ++++++
> >>>  2 files changed, 75 insertions(+), 3 deletions(-)
> >>> 
> >>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
> >>> b/drivers/gpu/drm/bridge/dw-hdmi.c index f4faa14213e5..ef4f2f96ed2c
> >>> 100644
> >>> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> >>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> 
> [snip]
> 
> > I don't have access to the documentation so I can't comment on that :-)
> > What does the SVSRET signal control (and what does the name stand for) ?
>
> SVSRET stands for SVSRET :) (no idea what it means) Its a low
> power mode of consumption.
> 
> > By de-asserting PHY reset, do you mean
> > 
> >         /* PHY reset */
> >         hdmi_writeb(hdmi, HDMI_MC_PHYRSTZ_DEASSERT, HDMI_MC_PHYRSTZ);
> >         hdmi_writeb(hdmi, HDMI_MC_PHYRSTZ_ASSERT, HDMI_MC_PHYRSTZ);
> > 
> > ? HDMI_MC_PHYRSTZ_DEASSERT is defined as 0x01 and HDMI_MC_PHYRSTZ_ASSERT
> > as 0x00, which I believe leads to correct operation on Gen2 PHYs, but is
> > incorrect on Gen1 PHYs that have an active low reset signal. Could you
> > confirm that ? The DEASSERT and ASSERT macros should be renamed as
> > they're obviously incorrect.
> 
> Correct. Older phys require PHYRSTZ to be deasserted (i.e. low)
> for a PHY-dependent time. Newer phys require PHYRSTZ to be
> asserted (i.e. high) for, again, a PHY-dependent time.

Thanks for the confirmation, I'll rename the macros.

> This is the kind of things that made me suggest you to extract
> all the phy configuration from dw-hdmi. I think that having a
> bunch of if's because of all the phy's that we need to support
> does not make much sense. The downside is, of course, having code
> duplicated.

I agree with you in principle, but I'd rather do that when I'll have devices 
to test the code on. The three devices (soon to be) supported in mainline by 
the dw-hdmi drivers, i.MX6, RK3288 and R-Car Gen3, all use Gen2 PHYs, so I 
can't test the Gen1 code paths meaningfully at the moment.

> > I can move the SVSRET assertion before PHY reset and test it on RK3288 and
> > R-Car H3.
> 
> Probably wont make much difference unless you have a way to
> measure how much power the phy is consuming. But I think it is
> the right thing to do according to docs.

The init sequence is currently

- Power the PHY off (TXPWRON = 0, PDDQ = 1)
- Reset the PHY (through HDMI_MC_PHYRSTZ and HDMI_MC_HEACPHY_RST)
- Configure the PHY through I2C
- Power the PHY on (TXPWRON = 1, PDDQ = 0)
- Set SVSRET
- Wait for PHY PLL lock

I've tried moving SVSRET right before the reset and everything still seems to 
work fine, so I'll submit a patch.

Is the rest of the sequence correct ? When should SVSRET be deasserted (the 
driver currently keeps it asserted at all times) ?

Speaking of reset, I believe support for HEAC PHYs is broken, given that the 
driver asserts the reset signal (through the HDMI_MC_HEACPHY_RST register) but 
never deasserts it.

> [snip]
> 
> > The SoC datasheet mentions the SVSRET bit in the HDMI TX registers but
> > doesn't document it any further. If I don't set SVSRET the HDMI output
> > stays dead, so I assume I need to set it :-)
> 
> Hmm, ok. I still haven't figured out which phy you are using so
> can't comment much further.

I'll then leave has_svsret set to true for DW_HDMI_PHY_DWC_HDMI20_TX_PHY as 
tests show it's needed.

-- 
Regards,

Laurent Pinchart