From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: From: Laurent Pinchart To: Jose Abreu Cc: Laurent Pinchart , dri-devel@lists.freedesktop.org, Andy Yan , Fabio Estevam , Kieran Bingham , Russell King , Ulrich Hecht , Vladimir Zapolskiy , linux-renesas-soc@vger.kernel.org Subject: Re: [PATCH v2 16/29] drm: bridge: dw-hdmi: Detect PHY type at runtime Date: Thu, 05 Jan 2017 02:15:04 +0200 Message-ID: <6935724.YkYy7FE9s6@avalon> In-Reply-To: References: <20161220013400.28317-1-laurent.pinchart+renesas@ideasonboard.com> <1706424.IHm6bGF9sZ@avalon> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" List-ID: 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v2 16/29] drm: bridge: dw-hdmi: Detect PHY type at runtime Date: Thu, 05 Jan 2017 02:15:04 +0200 Message-ID: <6935724.YkYy7FE9s6@avalon> References: <20161220013400.28317-1-laurent.pinchart+renesas@ideasonboard.com> <1706424.IHm6bGF9sZ@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from galahad.ideasonboard.com (galahad.ideasonboard.com [IPv6:2001:4b98:dc2:45:216:3eff:febb:480d]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3817E6E201 for ; Thu, 5 Jan 2017 00:15:00 +0000 (UTC) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Jose Abreu Cc: Fabio Estevam , Laurent Pinchart , Kieran Bingham , dri-devel@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, Ulrich Hecht , Russell King , Andy Yan , Vladimir Zapolskiy List-Id: dri-devel@lists.freedesktop.org SGkgSm9zZSwKCk9uIFR1ZXNkYXkgMjAgRGVjIDIwMTYgMTU6MDE6NTIgSm9zZSBBYnJldSB3cm90 ZToKPiBPbiAyMC0xMi0yMDE2IDEzOjExLCBMYXVyZW50IFBpbmNoYXJ0IHdyb3RlOgo+ID4gT24g VHVlc2RheSAyMCBEZWMgMjAxNiAxMTozOToyMSBKb3NlIEFicmV1IHdyb3RlOgo+ID4+IE9uIDIw LTEyLTIwMTYgMDE6MzMsIExhdXJlbnQgUGluY2hhcnQgd3JvdGU6Cj4gPj4+IERldGVjdCB0aGUg UEhZIHR5cGUgYW5kIHVzZSBpdCB0byBoYW5kbGUgdGhlIFBIWSB0eXBlLXNwZWNpZmljIFNWU1JF VAo+ID4+PiBzaWduYWwuCj4gPj4+IAo+ID4+PiBTaWduZWQtb2ZmLWJ5OiBMYXVyZW50IFBpbmNo YXJ0Cj4gPj4+IDxsYXVyZW50LnBpbmNoYXJ0K3JlbmVzYXNAaWRlYXNvbmJvYXJkLmNvbT4KPiA+ Pj4gLS0tCj4gPj4+IAo+ID4+PiAgZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9kdy1oZG1pLmMgfCA2 OCArKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrLS0KPiA+Pj4gIGluY2x1ZGUvZHJtL2Jy aWRnZS9kd19oZG1pLmggICAgIHwgMTAgKysrKysrCj4gPj4+ICAyIGZpbGVzIGNoYW5nZWQsIDc1 IGluc2VydGlvbnMoKyksIDMgZGVsZXRpb25zKC0pCj4gPj4+IAo+ID4+PiBkaWZmIC0tZ2l0IGEv ZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9kdy1oZG1pLmMKPiA+Pj4gYi9kcml2ZXJzL2dwdS9kcm0v YnJpZGdlL2R3LWhkbWkuYyBpbmRleCBmNGZhYTE0MjEzZTUuLmVmNGYyZjk2ZWQyYwo+ID4+PiAx MDA2NDQKPiA+Pj4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9kdy1oZG1pLmMKPiA+Pj4g KysrIGIvZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9kdy1oZG1pLmMKPiAKPiBbc25pcF0KPiAKPiA+ IEkgZG9uJ3QgaGF2ZSBhY2Nlc3MgdG8gdGhlIGRvY3VtZW50YXRpb24gc28gSSBjYW4ndCBjb21t ZW50IG9uIHRoYXQgOi0pCj4gPiBXaGF0IGRvZXMgdGhlIFNWU1JFVCBzaWduYWwgY29udHJvbCAo YW5kIHdoYXQgZG9lcyB0aGUgbmFtZSBzdGFuZCBmb3IpID8KPgo+IFNWU1JFVCBzdGFuZHMgZm9y IFNWU1JFVCA6KSAobm8gaWRlYSB3aGF0IGl0IG1lYW5zKSBJdHMgYSBsb3cKPiBwb3dlciBtb2Rl IG9mIGNvbnN1bXB0aW9uLgo+IAo+ID4gQnkgZGUtYXNzZXJ0aW5nIFBIWSByZXNldCwgZG8geW91 IG1lYW4KPiA+IAo+ID4gICAgICAgICAvKiBQSFkgcmVzZXQgKi8KPiA+ICAgICAgICAgaGRtaV93 cml0ZWIoaGRtaSwgSERNSV9NQ19QSFlSU1RaX0RFQVNTRVJULCBIRE1JX01DX1BIWVJTVFopOwo+ ID4gICAgICAgICBoZG1pX3dyaXRlYihoZG1pLCBIRE1JX01DX1BIWVJTVFpfQVNTRVJULCBIRE1J X01DX1BIWVJTVFopOwo+ID4gCj4gPiA/IEhETUlfTUNfUEhZUlNUWl9ERUFTU0VSVCBpcyBkZWZp bmVkIGFzIDB4MDEgYW5kIEhETUlfTUNfUEhZUlNUWl9BU1NFUlQKPiA+IGFzIDB4MDAsIHdoaWNo IEkgYmVsaWV2ZSBsZWFkcyB0byBjb3JyZWN0IG9wZXJhdGlvbiBvbiBHZW4yIFBIWXMsIGJ1dCBp cwo+ID4gaW5jb3JyZWN0IG9uIEdlbjEgUEhZcyB0aGF0IGhhdmUgYW4gYWN0aXZlIGxvdyByZXNl dCBzaWduYWwuIENvdWxkIHlvdQo+ID4gY29uZmlybSB0aGF0ID8gVGhlIERFQVNTRVJUIGFuZCBB U1NFUlQgbWFjcm9zIHNob3VsZCBiZSByZW5hbWVkIGFzCj4gPiB0aGV5J3JlIG9idmlvdXNseSBp bmNvcnJlY3QuCj4gCj4gQ29ycmVjdC4gT2xkZXIgcGh5cyByZXF1aXJlIFBIWVJTVFogdG8gYmUg ZGVhc3NlcnRlZCAoaS5lLiBsb3cpCj4gZm9yIGEgUEhZLWRlcGVuZGVudCB0aW1lLiBOZXdlciBw aHlzIHJlcXVpcmUgUEhZUlNUWiB0byBiZQo+IGFzc2VydGVkIChpLmUuIGhpZ2gpIGZvciwgYWdh aW4sIGEgUEhZLWRlcGVuZGVudCB0aW1lLgoKVGhhbmtzIGZvciB0aGUgY29uZmlybWF0aW9uLCBJ J2xsIHJlbmFtZSB0aGUgbWFjcm9zLgoKPiBUaGlzIGlzIHRoZSBraW5kIG9mIHRoaW5ncyB0aGF0 IG1hZGUgbWUgc3VnZ2VzdCB5b3UgdG8gZXh0cmFjdAo+IGFsbCB0aGUgcGh5IGNvbmZpZ3VyYXRp b24gZnJvbSBkdy1oZG1pLiBJIHRoaW5rIHRoYXQgaGF2aW5nIGEKPiBidW5jaCBvZiBpZidzIGJl Y2F1c2Ugb2YgYWxsIHRoZSBwaHkncyB0aGF0IHdlIG5lZWQgdG8gc3VwcG9ydAo+IGRvZXMgbm90 IG1ha2UgbXVjaCBzZW5zZS4gVGhlIGRvd25zaWRlIGlzLCBvZiBjb3Vyc2UsIGhhdmluZyBjb2Rl Cj4gZHVwbGljYXRlZC4KCkkgYWdyZWUgd2l0aCB5b3UgaW4gcHJpbmNpcGxlLCBidXQgSSdkIHJh dGhlciBkbyB0aGF0IHdoZW4gSSdsbCBoYXZlIGRldmljZXMgCnRvIHRlc3QgdGhlIGNvZGUgb24u IFRoZSB0aHJlZSBkZXZpY2VzIChzb29uIHRvIGJlKSBzdXBwb3J0ZWQgaW4gbWFpbmxpbmUgYnkg CnRoZSBkdy1oZG1pIGRyaXZlcnMsIGkuTVg2LCBSSzMyODggYW5kIFItQ2FyIEdlbjMsIGFsbCB1 c2UgR2VuMiBQSFlzLCBzbyBJIApjYW4ndCB0ZXN0IHRoZSBHZW4xIGNvZGUgcGF0aHMgbWVhbmlu Z2Z1bGx5IGF0IHRoZSBtb21lbnQuCgo+ID4gSSBjYW4gbW92ZSB0aGUgU1ZTUkVUIGFzc2VydGlv biBiZWZvcmUgUEhZIHJlc2V0IGFuZCB0ZXN0IGl0IG9uIFJLMzI4OCBhbmQKPiA+IFItQ2FyIEgz Lgo+IAo+IFByb2JhYmx5IHdvbnQgbWFrZSBtdWNoIGRpZmZlcmVuY2UgdW5sZXNzIHlvdSBoYXZl IGEgd2F5IHRvCj4gbWVhc3VyZSBob3cgbXVjaCBwb3dlciB0aGUgcGh5IGlzIGNvbnN1bWluZy4g QnV0IEkgdGhpbmsgaXQgaXMKPiB0aGUgcmlnaHQgdGhpbmcgdG8gZG8gYWNjb3JkaW5nIHRvIGRv Y3MuCgpUaGUgaW5pdCBzZXF1ZW5jZSBpcyBjdXJyZW50bHkKCi0gUG93ZXIgdGhlIFBIWSBvZmYg KFRYUFdST04gPSAwLCBQRERRID0gMSkKLSBSZXNldCB0aGUgUEhZICh0aHJvdWdoIEhETUlfTUNf UEhZUlNUWiBhbmQgSERNSV9NQ19IRUFDUEhZX1JTVCkKLSBDb25maWd1cmUgdGhlIFBIWSB0aHJv dWdoIEkyQwotIFBvd2VyIHRoZSBQSFkgb24gKFRYUFdST04gPSAxLCBQRERRID0gMCkKLSBTZXQg U1ZTUkVUCi0gV2FpdCBmb3IgUEhZIFBMTCBsb2NrCgpJJ3ZlIHRyaWVkIG1vdmluZyBTVlNSRVQg cmlnaHQgYmVmb3JlIHRoZSByZXNldCBhbmQgZXZlcnl0aGluZyBzdGlsbCBzZWVtcyB0byAKd29y ayBmaW5lLCBzbyBJJ2xsIHN1Ym1pdCBhIHBhdGNoLgoKSXMgdGhlIHJlc3Qgb2YgdGhlIHNlcXVl bmNlIGNvcnJlY3QgPyBXaGVuIHNob3VsZCBTVlNSRVQgYmUgZGVhc3NlcnRlZCAodGhlIApkcml2 ZXIgY3VycmVudGx5IGtlZXBzIGl0IGFzc2VydGVkIGF0IGFsbCB0aW1lcykgPwoKU3BlYWtpbmcg b2YgcmVzZXQsIEkgYmVsaWV2ZSBzdXBwb3J0IGZvciBIRUFDIFBIWXMgaXMgYnJva2VuLCBnaXZl biB0aGF0IHRoZSAKZHJpdmVyIGFzc2VydHMgdGhlIHJlc2V0IHNpZ25hbCAodGhyb3VnaCB0aGUg SERNSV9NQ19IRUFDUEhZX1JTVCByZWdpc3RlcikgYnV0IApuZXZlciBkZWFzc2VydHMgaXQuCgo+ IFtzbmlwXQo+IAo+ID4gVGhlIFNvQyBkYXRhc2hlZXQgbWVudGlvbnMgdGhlIFNWU1JFVCBiaXQg aW4gdGhlIEhETUkgVFggcmVnaXN0ZXJzIGJ1dAo+ID4gZG9lc24ndCBkb2N1bWVudCBpdCBhbnkg ZnVydGhlci4gSWYgSSBkb24ndCBzZXQgU1ZTUkVUIHRoZSBIRE1JIG91dHB1dAo+ID4gc3RheXMg ZGVhZCwgc28gSSBhc3N1bWUgSSBuZWVkIHRvIHNldCBpdCA6LSkKPiAKPiBIbW0sIG9rLiBJIHN0 aWxsIGhhdmVuJ3QgZmlndXJlZCBvdXQgd2hpY2ggcGh5IHlvdSBhcmUgdXNpbmcgc28KPiBjYW4n dCBjb21tZW50IG11Y2ggZnVydGhlci4KCkknbGwgdGhlbiBsZWF2ZSBoYXNfc3ZzcmV0IHNldCB0 byB0cnVlIGZvciBEV19IRE1JX1BIWV9EV0NfSERNSTIwX1RYX1BIWSBhcyAKdGVzdHMgc2hvdyBp dCdzIG5lZWRlZC4KCi0tIApSZWdhcmRzLAoKTGF1cmVudCBQaW5jaGFydAoKX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlz dApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0 b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg==