Hi Jose, On Monday 05 Dec 2016 12:31:30 Jose Abreu wrote: > On 05-12-2016 11:32, Laurent Pinchart wrote: > > 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@xxxxxxxxxxxxxxxx> > >>>>> > >>>>> 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. Thank you. That's the value reported by Allwinner platforms, which expose their PHY control registers through APB instead of the internal I2C bus. It all starts making sense :-) > > 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. Thank you. > >>> 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@xxxxxxxxxxxxxxxx> > >>>>> Signed-off-by: Laurent Pinchart > >>>>> <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx> > >>>>> --- > >>>>> > >>>>> 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(-) -- Regards, Laurent Pinchart
