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
>>>>> <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
>>>>> ---
>>>>>
>>>>> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
