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

[Jose Abreu]

Hi Laurent,


On 05-01-2017 11:44, Laurent Pinchart wrote:

[snip]

>>> 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) ?
>> It should not be deasserted.
> At all ? Even when powering the PHY down ?

The source code I have here does not deassert the signal, even
when powering down BUT I am checking the docs and this signal can
optionally go low after the phy goes to power down mode (i.e.
after all power down sequence is correctly done, this includes
waiting for TX_READY to go low as Russell King previously said),
though its not mandatory. I think I miss explained how this mode
works: When deasserted the phy goes to low power mode, thats why
you need to assert it in power up. We can try to add this in
power down sequence but its very important to check the status of
TX_READY before changing SVSRET.

Best regards,
Jose Miguel Abreu

[snip]

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

[Laurent Pinchart]

Hi Jose,

On Thursday 05 Jan 2017 10:33:55 Jose Abreu wrote:
> On 05-01-2017 00:15, Laurent Pinchart wrote:
> > 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
> > 
> > ---
> > 
> >  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
> 
> What I have here for the most recent phy we tested is this:
> - Board specific configuration (should not be needed by you)
> - Set MC_PHY_RST high
> - Set TXPWRON = 0
> - Set PDDQ = 1
> - Set SVSRET = 0
> - Board specific impendance calibration reset (should not be
> needed by you)
> - Set SVSRET = 1
> - Set MC_PHY_RST low
> - Configure phy through I2C
> - Set PDDQ = 0
> - Set TXPWRON = 1,
> - Wait for phy pll lock

Thank you, I'll test that.

> > 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) ?
> 
> It should not be deasserted.

At all ? Even when powering the PHY down ?

> > 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.
> 
> Hmm, probably, but not sure. I never tested this in older phys.

One more item we'll fix when we'll be able to test 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

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

[Jose Abreu]

Hi Laurent,


On 05-01-2017 00:15, Laurent Pinchart wrote:
> 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
> 
> ---
>
>  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

What I have here for the most recent phy we tested is this:
- Board specific configuration (should not be needed by you)
- Set MC_PHY_RST high
- Set TXPWRON = 0
- Set PDDQ = 1
- Set SVSRET = 0
- Board specific impendance calibration reset (should not be
needed by you)
- Set SVSRET = 1
- Set MC_PHY_RST low
- Configure phy through I2C
- Set PDDQ = 0
- Set TXPWRON = 1,
- 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) ?

It should not be deasserted.

>
> 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.

Hmm, probably, but not sure. I never tested this in older phys.

>
>> [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.
>

Best regards,
Jose Miguel Abreu

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

[Laurent Pinchart]

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
> >>> 
> >>> ---
> >>> 
> >>>  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

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

[Laurent Pinchart]

Hi Jose,

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
> > 
> > ---
> > 
> >  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]

> > struct i2c_adapter *ddc;
> > @@ -1015,7 +1023,8 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi,
> > int cscon)> 
> > dw_hdmi_phy_gen2_txpwron(hdmi, 1);
> > dw_hdmi_phy_gen2_pddq(hdmi, 0);
> > 
> > -   if (hdmi->dev_type == RK3288_HDMI)
> > +   /* The DWC MHL and HDMI 2.0 PHYs need the SVSRET signal to be set. */
> > +   if (hdmi->phy->has_svsret)
> > dw_hdmi_phy_enable_svsret(hdmi, 1);
> 
> I didn't review the original code but according to the docs the
> svsret signal should be set before de-asserting phy reset.

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) ?

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.

I can move the SVSRET assertion before PHY reset and test it on RK3288 and R-
Car H3.

> > /*Wait for PHY PLL lock */
> > 
> > @@ -1840,6 +1849,54 @@ static irqreturn_t dw_hdmi_irq(int irq, void
> > *dev_id)
> > return IRQ_HANDLED;
> >  }
> > 
> > +static const struct dw_hdmi_phy_data dw_hdmi_phys[] = {
> > +   {
> > +   .type = DW_HDMI_PHY_DWC_HDMI_TX_PHY,
> > +   .name = "DWC HDMI TX PHY",
> > +   }, {
> > +   .type = DW_HDMI_PHY_DWC_MHL_PHY_HEAC,
> > +   .name = "DWC MHL PHY + HEAC PHY",
> > +   .has_svsret = true,
> > +   }, {
> > +   .type = DW_HDMI_PHY_DWC_MHL_PHY,
> > +   .name = "DWC MHL PHY",
> > +   .has_svsret = true,
> > +   }, {
> > +   .type = DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY_HEAC,
> > +   .name = "DWC HDMI 3D TX PHY + HEAC PHY",
> > +   }, {
> > +   .type = DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY,
> > +   .name = "DWC HDMI 3D TX PHY",
> > +   }, {
> > +   .type = DW_HDMI_PHY_DWC_HDMI20_TX_PHY,
> > +   .name = "DWC HDMI 2.0 TX PHY",
> > +   .has_svsret = true,
> 
> Hmm, what I have here is that only MHL phys have svsret. Is this
> case for your phy?

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 :-)

> > +   }
> > +};

[snip]

-- 
Regards,

Laurent Pinchart

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

[Jose Abreu]

Hi Laurent,


On 20-12-2016 13:11, Laurent Pinchart wrote:
> Hi Jose,
>
> 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
>>> 
>>> ---
>>>
>>>  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.

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 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.

[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.

Best regards,
Jose Miguel Abreu

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

[Jose Abreu]

Hi Laurent,


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 
> ---
>  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
> @@ -113,6 +113,12 @@ struct dw_hdmi_i2c {
>   boolis_regaddr;
>  };
>  
> +struct dw_hdmi_phy_data {
> + enum dw_hdmi_phy_type type;
> + const char *name;
> + bool has_svsret;
> +};
> +
>  struct dw_hdmi {
>   struct drm_connector connector;
>   struct drm_bridge bridge;
> @@ -134,7 +140,9 @@ struct dw_hdmi {
>   u8 edid[HDMI_EDID_LEN];
>   bool cable_plugin;
>  
> + const struct dw_hdmi_phy_data *phy;
>   bool phy_enabled;
> +
>   struct drm_display_mode previous_mode;
>  
>   struct i2c_adapter *ddc;
> @@ -1015,7 +1023,8 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi, int 
> cscon)
>   dw_hdmi_phy_gen2_txpwron(hdmi, 1);
>   dw_hdmi_phy_gen2_pddq(hdmi, 0);
>  
> - if (hdmi->dev_type == RK3288_HDMI)
> + /* The DWC MHL and HDMI 2.0 PHYs need the SVSRET signal to be set. */
> + if (hdmi->phy->has_svsret)
>   dw_hdmi_phy_enable_svsret(hdmi, 1);

I didn't review the original code but according to the docs the
svsret signal should be set before de-asserting phy reset.

>  
>   /*Wait for PHY PLL lock */
> @@ -1840,6 +1849,54 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>   return IRQ_HANDLED;
>  }
>  
> +static const struct dw_hdmi_phy_data dw_hdmi_phys[] = {
> + {
> + .type = DW_HDMI_PHY_DWC_HDMI_TX_PHY,
> + .name = "DWC HDMI TX PHY",
> + }, {
> + .type = DW_HDMI_PHY_DWC_MHL_PHY_HEAC,
> + .name = "DWC MHL PHY + HEAC PHY",
> + .has_svsret = true,
> + }, {
> + .type = DW_HDMI_PHY_DWC_MHL_PHY,
> + .name = "DWC MHL PHY",
> + .has_svsret = true,
> + }, {
> + .type = DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY_HEAC,
> + .name = "DWC HDMI 3D TX PHY + HEAC PHY",
> + }, {
> + .type = DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY,
> + .name = "DWC HDMI 3D TX PHY",
> + }, {
> + .type = DW_HDMI_PHY_DWC_HDMI20_TX_PHY,
> + .name = "DWC HDMI 2.0 TX PHY",
> + .has_svsret = true,

Hmm, what I have here is that only MHL phys have svsret. Is this
case for your phy?

Best regards,
Jose Miguel Abreu
> + }
> +};
> +
> +static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
> +{
> + unsigned int i;
> + u8 phy_type;
> +
> + phy_type = hdmi_readb(hdmi, HDMI_CONFIG2_ID);
> +
> + for (i = 0; i < ARRAY_SIZE(dw_hdmi_phys); ++i) {
> + if (dw_hdmi_phys[i].type == phy_type) {
> + hdmi->phy = _hdmi_phys[i];
> + return 0;
> + }
> + }
> +
> + if (phy_type == DW_HDMI_PHY_VENDOR_PHY)
> + dev_err(hdmi->dev, "Unsupported vendor HDMI PHY\n");
> + else
> + dev_err(hdmi->dev, "Unsupported HDMI PHY type (%02x)\n",
> + phy_type);
> +
> + return -ENODEV;
> +}
> +
>  static struct dw_hdmi *
>  __dw_hdmi_probe(struct platform_device *pdev,
>   const struct dw_hdmi_plat_data *plat_data)
> @@ -1950,9 +2007,14 @@ __dw_hdmi_probe(struct platform_device *pdev,
>   goto err_iahb;
>   }
>  
> - dev_info(dev, "Detected HDMI TX controller v%x.%03x %s HDCP\n",
> + ret = dw_hdmi_detect_phy(hdmi);
> + if (ret < 0)
> + goto err_iahb;
> +
> + dev_info(dev, "Detected HDMI TX controller v%x.%03x %s HDCP (%s)\n",
>hdmi->version >> 12, hdmi->version & 0xfff,
> -  prod_id1 & HDMI_PRODUCT_ID1_HDCP ? "with" : "without");
> +  prod_id1 & HDMI_PRODUCT_ID1_HDCP ? "with" : "without",
> +  hdmi->phy->name);
>  
>   initialize_hdmi_ih_mutes(hdmi);
>  
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 3bb22a849830..b080a171a23f 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -27,6 +27,16 @@ enum dw_hdmi_devtype {
>   RK3288_HDMI,
>  };
>  
> +enum dw_hdmi_phy_type {
> + DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00,
> + DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2,
> + DW_HDMI_PHY_DWC_MHL_PHY = 0xc2,
> + DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY_HEAC = 0xe2,
> + DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY = 0xf2,
> + DW_HDMI_PHY_DWC_HDMI20_TX_PHY = 0xf3,
> + DW_HDMI_PHY_VENDOR_PHY = 0xfe,
> +};
> +
>  struct dw_hdmi_mpll_config {
>   unsigned long 

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

[Laurent Pinchart]

Detect the PHY type and use it to handle the PHY type-specific SVSRET
signal.

Signed-off-by: Laurent Pinchart 
---
 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
@@ -113,6 +113,12 @@ struct dw_hdmi_i2c {
boolis_regaddr;
 };

+struct dw_hdmi_phy_data {
+   enum dw_hdmi_phy_type type;
+   const char *name;
+   bool has_svsret;
+};
+
 struct dw_hdmi {
struct drm_connector connector;
struct drm_bridge bridge;
@@ -134,7 +140,9 @@ struct dw_hdmi {
u8 edid[HDMI_EDID_LEN];
bool cable_plugin;

+   const struct dw_hdmi_phy_data *phy;
bool phy_enabled;
+
struct drm_display_mode previous_mode;

struct i2c_adapter *ddc;
@@ -1015,7 +1023,8 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi, int 
cscon)
dw_hdmi_phy_gen2_txpwron(hdmi, 1);
dw_hdmi_phy_gen2_pddq(hdmi, 0);

-   if (hdmi->dev_type == RK3288_HDMI)
+   /* The DWC MHL and HDMI 2.0 PHYs need the SVSRET signal to be set. */
+   if (hdmi->phy->has_svsret)
dw_hdmi_phy_enable_svsret(hdmi, 1);

/*Wait for PHY PLL lock */
@@ -1840,6 +1849,54 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
return IRQ_HANDLED;
 }

+static const struct dw_hdmi_phy_data dw_hdmi_phys[] = {
+   {
+   .type = DW_HDMI_PHY_DWC_HDMI_TX_PHY,
+   .name = "DWC HDMI TX PHY",
+   }, {
+   .type = DW_HDMI_PHY_DWC_MHL_PHY_HEAC,
+   .name = "DWC MHL PHY + HEAC PHY",
+   .has_svsret = true,
+   }, {
+   .type = DW_HDMI_PHY_DWC_MHL_PHY,
+   .name = "DWC MHL PHY",
+   .has_svsret = true,
+   }, {
+   .type = DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY_HEAC,
+   .name = "DWC HDMI 3D TX PHY + HEAC PHY",
+   }, {
+   .type = DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY,
+   .name = "DWC HDMI 3D TX PHY",
+   }, {
+   .type = DW_HDMI_PHY_DWC_HDMI20_TX_PHY,
+   .name = "DWC HDMI 2.0 TX PHY",
+   .has_svsret = true,
+   }
+};
+
+static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
+{
+   unsigned int i;
+   u8 phy_type;
+
+   phy_type = hdmi_readb(hdmi, HDMI_CONFIG2_ID);
+
+   for (i = 0; i < ARRAY_SIZE(dw_hdmi_phys); ++i) {
+   if (dw_hdmi_phys[i].type == phy_type) {
+   hdmi->phy = _hdmi_phys[i];
+   return 0;
+   }
+   }
+
+   if (phy_type == DW_HDMI_PHY_VENDOR_PHY)
+   dev_err(hdmi->dev, "Unsupported vendor HDMI PHY\n");
+   else
+   dev_err(hdmi->dev, "Unsupported HDMI PHY type (%02x)\n",
+   phy_type);
+
+   return -ENODEV;
+}
+
 static struct dw_hdmi *
 __dw_hdmi_probe(struct platform_device *pdev,
const struct dw_hdmi_plat_data *plat_data)
@@ -1950,9 +2007,14 @@ __dw_hdmi_probe(struct platform_device *pdev,
goto err_iahb;
}

-   dev_info(dev, "Detected HDMI TX controller v%x.%03x %s HDCP\n",
+   ret = dw_hdmi_detect_phy(hdmi);
+   if (ret < 0)
+   goto err_iahb;
+
+   dev_info(dev, "Detected HDMI TX controller v%x.%03x %s HDCP (%s)\n",
 hdmi->version >> 12, hdmi->version & 0xfff,
-prod_id1 & HDMI_PRODUCT_ID1_HDCP ? "with" : "without");
+prod_id1 & HDMI_PRODUCT_ID1_HDCP ? "with" : "without",
+hdmi->phy->name);

initialize_hdmi_ih_mutes(hdmi);

diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 3bb22a849830..b080a171a23f 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -27,6 +27,16 @@ enum dw_hdmi_devtype {
RK3288_HDMI,
 };

+enum dw_hdmi_phy_type {
+   DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00,
+   DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2,
+   DW_HDMI_PHY_DWC_MHL_PHY = 0xc2,
+   DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY_HEAC = 0xe2,
+   DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY = 0xf2,
+   DW_HDMI_PHY_DWC_HDMI20_TX_PHY = 0xf3,
+   DW_HDMI_PHY_VENDOR_PHY = 0xfe,
+};
+
 struct dw_hdmi_mpll_config {
unsigned long mpixelclock;
struct {
-- 
Regards,

Laurent Pinchart