Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

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

Hi Russell,

On Friday 02 Dec 2016 14:24:01 Russell King - ARM Linux wrote:
> On Fri, Dec 02, 2016 at 01:43:28AM +0200, Laurent Pinchart wrote:
> > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 
> > The dw-hdmi driver declares a dev_type to distinguish platform specific
> > changes. Replace this with a quirk field, so that the platform can
> > specify the required quirks for the driver, rather than the driver
> > becoming conditional on multiple platforms.
> > 
> > As part of this, we rename the dw-hdmi 'spare' which is defined as the
> > SVSRET bit in later documentation.
> 
> I'd really prefer that we did not go down the broken route of adding
> a set of "quirk" flags - look at what a mess SDHCI has become through
> allowing that kind of practice.
> 
> I'd much rather we find a saner structure to this - and we know that
> the hardware has ID registers in it which can be used (so far) to
> identify the buggy hardware.

I'd much prefer something that would allow runtime identification of the 
device and the corresponding actions to be taken. However, the amount of 
documentation we have on the DWC HDMI TX IP core (and the associated PHY) is 
pretty limited, given that Synopsys doesn't make the documentation available 
publicly. Changes made to the IP core by integrators could complicate this 
further. I'm trying to gather as much information as possible to make clean 
the code up, for instance by trying to identify the PHYs used on the various 
platforms we support. Progress is slow on that front, there isn't enough 
leaked information available online :-) I haven't given up though, but I'll 
need more time.

I don't like quirks much either. They are however already used today, even if 
we trigger them through dev_type instead of quirk flags. This patch came from 
a previous version found in a BSP that simply sprinkled several if (hdmi-
>dev_type == RCAR_HDMI) through the code. For instance,

-	if (hdmi->dev_type == RK3288_HDMI)
+	if (hdmi->dev_type == RK3288_HDMI || hdmi->dev_type == RCAR_HDMI)
		dw_hdmi_phy_enable_spare(hdmi, 1);

which I think is worse than flags as it would quickly degenerate to spaghetti 
code.

For this specific case, we've managed to identify that on Renesas platforms 
the bit set by this function is called SVSRET. Its usage isn't clear yet, but 
I suspect it to control one of the PHY input control signals, like the other 
bits in the same register. I'm trying to get more information to clean the 
implementation further, hopefully with a way to determine whether the signal 
is used based on PHY identification.

This is all work in progress, and if anyone has access to any documentation 
and can provide additional information I'll be grateful.

> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/bridge/dw-hdmi.c            | 14 ++++++--------
> >  drivers/gpu/drm/bridge/dw-hdmi.h            |  4 ++--
> >  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  3 +--
> >  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
> >  include/drm/bridge/dw_hdmi.h                | 12 +++++-------
> >  5 files changed, 15 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
> > b/drivers/gpu/drm/bridge/dw-hdmi.c index 06a44a2cdf3b..26607185722f
> > 100644
> > --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> > @@ -118,7 +118,6 @@ struct dw_hdmi {
> >  	struct drm_bridge bridge;
> >  	struct platform_device *audio;
> > 
> > -	enum dw_hdmi_devtype dev_type;
> >  	struct device *dev;
> >  	struct clk *isfr_clk;
> >  	struct clk *iahb_clk;
> > @@ -896,11 +895,11 @@ static void dw_hdmi_phy_enable_tmds(struct dw_hdmi
> > *hdmi, u8 enable)
> >  			 HDMI_PHY_CONF0_ENTMDS_MASK);
> >  }
> > 
> > -static void dw_hdmi_phy_enable_spare(struct dw_hdmi *hdmi, u8 enable)
> > +static void dw_hdmi_phy_enable_svsret(struct dw_hdmi *hdmi, u8 enable)
> >  {
> >  	hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
> > -			 HDMI_PHY_CONF0_SPARECTRL_OFFSET,
> > -			 HDMI_PHY_CONF0_SPARECTRL_MASK);
> > +			 HDMI_PHY_CONF0_SVSRET_OFFSET,
> > +			 HDMI_PHY_CONF0_SVSRET_MASK);
> >  }
> >  
> >  static void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable)
> > @@ -1031,8 +1030,8 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi,
> >  	dw_hdmi_phy_gen2_txpwron(hdmi, 1);
> >  	dw_hdmi_phy_gen2_pddq(hdmi, 0);
> > 
> > -	if (hdmi->dev_type == RK3288_HDMI)
> > -		dw_hdmi_phy_enable_spare(hdmi, 1);
> > +	if (pdata->quirks & DW_HDMI_QUIRK_PHY_SVSRET)
> > +		dw_hdmi_phy_enable_svsret(hdmi, 1);
> 
> If we get a proper split between the encoder and the PHY, this should be
> dealt with at the PHY side of the driver.

That's possible, and some other PHY-related setup code might be better placed 
in the PHY side as well. My problem, as explained above, is that I don't have 
enough information yet to design a perfect API. I think this patch is a good 
step forward, but it should not be the last one. I wish I could spend more 
time writing clean code instead of trying to reverse engineer the behaviour of 
the PHY :-)

> >  	/*Wait for PHY PLL lock */
> >  	msec = 5;
> > 
> > @@ -1348,7 +1347,7 @@ static void dw_hdmi_clear_overflow(struct dw_hdmi
> > *hdmi)
> >  	hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ, HDMI_MC_SWRSTZ);
> >  	
> >  	val = hdmi_readb(hdmi, HDMI_FC_INVIDCONF);
> > -	if (hdmi->dev_type == IMX6DL_HDMI) {
> > +	if (hdmi->plat_data->quirks & DW_HDMI_QUIRK_FC_INVIDCONF) {
> >  		hdmi_writeb(hdmi, val, HDMI_FC_INVIDCONF);
> >  		return;
> >  	}
> 
> This is a bug found on iMX6DL versions of the IP - I don't have a 6DL
> board powered up at the moment to grab its revision information, but
> it would be nice to make that conditional on the revision.  From what
> I gather, it's a workaround issued by Synopsis rather than specific
> to the (ex)FSL implementation.

DW_HDMI_QUIRK_FC_INVIDCONF is indeed a bad name, I'll fix that.

Do you know why this function needs to write to the HDMI_FC_INVIDCONF register 
four times in the normal case, and once only for IMX6DL ?

-- 
Regards,

Laurent Pinchart