Re: [PATCH v2] drm: bridge: synopsys/dw-hdmi: Provide default configuration function for HDMI 2.0 PHY

[Laurent Pinchart]

Hi Jose,

On Friday 23 Jun 2017 10:36:44 Jose Abreu wrote:
> Currently HDMI 2.0 PHYs do not have a default configuration function.
> 
> As *some* of the HDMI 2.0 PHYs have the same register layout as the 3D
> PHYs we can provide the same default configuration function for both
> and still let user overwrite this with custom configuration function
> if needed.
> 
> If, for some reason, the PHY is custom or has a register different
> register layout then custom configuration function *must* be provided
> in order for the system to work correctly. As we prefer the pdata
> provided configuration function over the internal one this change
> will not make any impact in custom platforms.
> 
> This patch is based on today's drm-misc-next branch.
> 
> Signed-off-by: Jose Abreu <joabreu@xxxxxxxxxxxx>
> Tested-by: Mark Yao <mark.yao@xxxxxxxxxxxxxx>
> Cc: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> Cc: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> Cc: Archit Taneja <architt@xxxxxxxxxxxxxx>
> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> Cc: Mark Yao <mark.yao@xxxxxxxxxxxxxx>
> Cc: Carlos Palminha <palminha@xxxxxxxxxxxx>
> Cc: Heiko Stübner <heiko@xxxxxxxxx>
> 
> Changes in v2:
> 	- Rebased and refrased commit message
> ---
> 
> Hi All,
> 
> There as been a little confusion about dw-hdmi phys so I will expand a
> little bit here so that I can base my decision about this patch and why
> does it only works in some platforms.
> 
> First, if you read dw-hdmi.c code, you will see that there is an
> identification register for the phy type being used. Unfortunatelly, this
> only states the phy type and not the phy version.
> 
> Second, we have many HDMI 2.0 phys (so, same phy type: 0xf3) but, as you may
> have guessed, HW team decided to change regbank between some versions.
> 
> Third and last, each phy in a SoC has unique characteristics, so each phy
> (event if they are the same version) will have different PLL configuration
> parameters.
> 
> Given all this I managed to conclude that Mark's phy is still an HDMI 2.0
> phy but with the same register layout as previous 3D PHY's. I found at
> least 2 phys with the same register layout and only 1 phy which has a
> different layout, so I think majority wins here and we should let the
> default configuration function for HDMI 2.0 phys be the same one as the 3D.
> 
> Short story: There is no way to correctly identify, at runtime, the phy
> version being used by the controller so we can't provide a default
> configuration function.

:'-(

> We can, however assume that most of the HDMI 2.0 phys will have the 3D
> layout BUT each developer must confirm that the layout in its SoC is the
> expected one and if not, provide a custom configuration function.

This seems fair to me, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>

However, if the PHY version can't be identified at runtime, I wonder whether 
it would make sense to add it to the PHY DT node. The driver could then pick 
the right configuration function accordingly. SoC-specific parameters (such as 
PLL parameters) would still be passed through platform glue code, but all the 
rest could be moved to the core driver.

>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index ead1124..10c8d8c 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -2170,6 +2170,7 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>  		.name = "DWC HDMI 2.0 TX PHY",
>  		.gen = 2,
>  		.has_svsret = true,
> +		.configure = hdmi_phy_configure_dwc_hdmi_3d_tx,
>  	}, {
>  		.type = DW_HDMI_PHY_VENDOR_PHY,
>  		.name = "Vendor PHY",

-- 
Regards,

Laurent Pinchart