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

[Jose Abreu]

Hi Laurent,


On 05-12-2016 11:32, Laurent Pinchart wrote:
> Hi Jose,
>
> On Monday 05 Dec 2016 10:50:19 Jose Abreu wrote:
>> On 02-12-2016 15:43, Laurent Pinchart wrote:
>>> 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 at 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.
>> SVSRET is a low power mode consumption and is a PHY input signal
>> as you suggested.
> Thank you for the confirmation. Would you happen to know what SVSRET stands 
> for ?

Have no info about that. Sorry.

>
>> Most of the configurable input signals of the PHY are available by the
>> controller regbank. I don't think it is possible to detect this at runtime,
>> I think you have at least to hardcode which version of the PHY you are
>> using.
>>
>> I would suggest that maybe all the PHY logic should be extracted and then
>> use callbacks to glue controller and phy. Then, depending on the PHY you
>> could use empty stubs if, for example, a given PHY did not support SVSRET.
>> Still, I don't know if this is the best option. What I do know is that there
>> are a large number of PHY's with different flavors that can use the same
>> controller. The controller has different versions also, and each version can
>> have quirks but I think it would be easier to manage this driver if we had a
>> clear distinction between PHY and controller.
> Agreed, I'd like to go in that direction. What makes it quite difficult is the 
> lack of documentation about the PHYs :-) I've found six different PHY types 
> that can be identified by the CONFIG2_ID register:
>
> Bits    | Field         	| Description
> ------------------------------------------------------------------------------
> 7-0     | phytype       	| PHY interface
>         |               	| 0x00: Legacy PHY (HDMI TX PHY)
>         |               	| 0xb2: MHL PHY + HEAC PHY
>         |               	| 0xc2: MHL PHY
>         |               	| 0xe2: HDMI 3D TX PHY + HEAC PHY
>         |               	| 0xf2: HDMI 3D TX PHY
>         |               	| 0xf3: HDMI2 TX PHY
>
> I'm sure there's more than that. In particular I wonder how external vendor 
> PHYs are identified.

0xFE.

>
> I'm also wondering whether there's a need to keep support for the legacy PHY 
> signals (ENTMDS and PDZ in the PHY_CONF0 register). As far as I understand 
> they're not used by the Gen2 PHYs (including the external vendor PHYs), but I 
> can't confirm that without more documentation (although I could test that on 
> the platforms I have access to).

You are correct. Not available on Gen2 and on external phys.

>>> 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 at ideasonboard.com>
>>>>> Signed-off-by: Laurent Pinchart
>>>>> <laurent.pinchart+renesas at 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(-)

Best regards,
Jose Miguel Abreu