From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: From: Laurent Pinchart To: Jose Abreu Cc: dri-devel@lists.freedesktop.org, Fabio Estevam , Laurent Pinchart , Russell King - ARM Linux , Kieran Bingham , linux-renesas-soc@vger.kernel.org, Ulrich Hecht , Andy Yan , Vladimir Zapolskiy Subject: Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling Date: Fri, 06 Jan 2017 03:48:29 +0200 Message-ID: <30295846.0iPnmn0IgG@avalon> In-Reply-To: <21211642.vKbn0SAu3E@avalon> References: <20161220013400.28317-1-laurent.pinchart+renesas@ideasonboard.com> <21211642.vKbn0SAu3E@avalon> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" List-ID: Hi Jose, On Thursday 05 Jan 2017 17:33:58 Laurent Pinchart wrote: > On Thursday 05 Jan 2017 15:06:49 Jose Abreu wrote: > > On 05-01-2017 12:29, Laurent Pinchart wrote: > >> On Tuesday 20 Dec 2016 12:17:23 Jose Abreu wrote: > >>> On 20-12-2016 11:45, Russell King - ARM Linux wrote: > >>>> On Tue, Dec 20, 2016 at 03:33:48AM +0200, Laurent Pinchart wrote: > >>>>> Instead of spreading version-dependent PHY power handling code > >>>>> around, group it in two functions to power the PHY on and off and use > >>>>> them through the driver. > >>>>> > >>>>> Powering off the PHY at the beginning of the setup phase is currently > >>>>> split in two locations for first and second generation PHYs, group > >>>>> all the operations in the dw_hdmi_phy_init() function. > >>>> > >>>> This changes the behaviour of the driver. > >>>> > >>>>> +static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) > >>>>> +{ > >>>>> + if (hdmi->phy->gen == 1) { > >>>>> + dw_hdmi_phy_enable_tmds(hdmi, 0); > >>>>> + dw_hdmi_phy_enable_powerdown(hdmi, true); > >>>>> + } else { > >>>>> + dw_hdmi_phy_gen2_txpwron(hdmi, 0); > >>>>> + dw_hdmi_phy_gen2_pddq(hdmi, 1); > >>>>> + } > >>>>> +} > >>>>> @@ -1290,9 +1302,7 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > >>>>> *hdmi) > >>>>> if (!hdmi->phy_enabled) > >>>>> return; > >>>>> > >>>>> - dw_hdmi_phy_enable_tmds(hdmi, 0); > >>>>> - dw_hdmi_phy_enable_powerdown(hdmi, true); > >>>>> - > >>>>> + dw_hdmi_phy_power_off(hdmi); > >>>> > >>>> This makes dw_hdmi_phy_disable() power down a gen2 phy. > >>>> > >>>> The iMX6 has a DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY phy, which you list as a > >>>> gen2 phy. I've been carrying this change for a while, which I've had > >>>> to revert (and finally expunge), as it causes problems on iMX6: > >>>> > >>>> @@ -1112,6 +1112,14 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > >>>> *hdmi) > >>>> if (!hdmi->phy_enabled) > >>>> return; > >>>> > >>>> + /* Actually set the phy into low power mode */ > >>>> + dw_hdmi_phy_gen2_txpwron(hdmi, 0); > >>>> + > >>>> + /* FIXME: We should wait for TX_READY to be deasserted */ > >>>> + > >>>> + dw_hdmi_phy_gen2_pddq(hdmi, 1); > >>>> + > >>>> + /* This appears to have no effect on iMX6 */ > >>>> dw_hdmi_phy_enable_tmds(hdmi, 0); > >>>> dw_hdmi_phy_enable_powerdown(hdmi, true); > >>>> > >>>> So, I think your change here will cause problems for iMX6. > >>>> > >>>> From what I remember, it seems that iMX6 has issues with RXSENSE/HPD > >>>> bouncing when the PHY is powered down. I can't promise when I'll be > >>>> able to check for that again. > >>> > >>> Indeed TX_READY must be low before asserting pddq. > >> > >> The TX_READY signal is documented in the i.MX6 datasheet as being a PHY > >> output signal, but there seems to be no HDMI TX register from which its > >> state can be read. Do I need to poll the HDMI_PHY_PTRPT_ENBL register > >> through I2C ? How long is the PHY expected to take to set TX_READY to 0 > >> ? > > > > TX_READY can be read from register 0x1A of phy, BIT(2) (through > > I2C). > > That's what I thought, I'll poll that then. Do you have any idea how long > it's supposed to take, to set an appropriate timeout ? On i.MX6 (3D TX PHY) register 0x1a reads as 0x0007 before powering down the PHY (by deasserting TXPWRON) and as 0x0000 immediately after. On RK3288 (MHL PHY) the register reads as 0x0207 and as 0x0000 immediately after deasserting TXPWRON. It seems that one I2C read is a sufficient delay for TX_READY to go low. > > Not sure if same offset for all phys though. > > Most probably not, it would be too easy :-) I'll investigate (which will > likely include lots of guesswork). If you can find any information about > that (and especially about the MHL and HDMI 2.0 PHYs) that would be very > appreciated, as I don't have access to any documentation that mentions a > TX_READY bit for those. I haven't tested the HDMI 2.0 PHY yet, but I'd be surprised if the TX_READY bit was in the same register at the same position. -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling Date: Fri, 06 Jan 2017 03:48:29 +0200 Message-ID: <30295846.0iPnmn0IgG@avalon> References: <20161220013400.28317-1-laurent.pinchart+renesas@ideasonboard.com> <21211642.vKbn0SAu3E@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 66C366E362 for ; Fri, 6 Jan 2017 01:48:26 +0000 (UTC) In-Reply-To: <21211642.vKbn0SAu3E@avalon> 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 , Russell King - ARM Linux , dri-devel@lists.freedesktop.org, Kieran Bingham , linux-renesas-soc@vger.kernel.org, Ulrich Hecht , Andy Yan , Vladimir Zapolskiy List-Id: dri-devel@lists.freedesktop.org SGkgSm9zZSwKCk9uIFRodXJzZGF5IDA1IEphbiAyMDE3IDE3OjMzOjU4IExhdXJlbnQgUGluY2hh cnQgd3JvdGU6Cj4gT24gVGh1cnNkYXkgMDUgSmFuIDIwMTcgMTU6MDY6NDkgSm9zZSBBYnJldSB3 cm90ZToKPiA+IE9uIDA1LTAxLTIwMTcgMTI6MjksIExhdXJlbnQgUGluY2hhcnQgd3JvdGU6Cj4g Pj4gT24gVHVlc2RheSAyMCBEZWMgMjAxNiAxMjoxNzoyMyBKb3NlIEFicmV1IHdyb3RlOgo+ID4+ PiBPbiAyMC0xMi0yMDE2IDExOjQ1LCBSdXNzZWxsIEtpbmcgLSBBUk0gTGludXggd3JvdGU6Cj4g Pj4+PiBPbiBUdWUsIERlYyAyMCwgMjAxNiBhdCAwMzozMzo0OEFNICswMjAwLCBMYXVyZW50IFBp bmNoYXJ0IHdyb3RlOgo+ID4+Pj4+IEluc3RlYWQgb2Ygc3ByZWFkaW5nIHZlcnNpb24tZGVwZW5k ZW50IFBIWSBwb3dlciBoYW5kbGluZyBjb2RlCj4gPj4+Pj4gYXJvdW5kLCBncm91cCBpdCBpbiB0 d28gZnVuY3Rpb25zIHRvIHBvd2VyIHRoZSBQSFkgb24gYW5kIG9mZiBhbmQgdXNlCj4gPj4+Pj4g dGhlbSB0aHJvdWdoIHRoZSBkcml2ZXIuCj4gPj4+Pj4gCj4gPj4+Pj4gUG93ZXJpbmcgb2ZmIHRo ZSBQSFkgYXQgdGhlIGJlZ2lubmluZyBvZiB0aGUgc2V0dXAgcGhhc2UgaXMgY3VycmVudGx5Cj4g Pj4+Pj4gc3BsaXQgaW4gdHdvIGxvY2F0aW9ucyBmb3IgZmlyc3QgYW5kIHNlY29uZCBnZW5lcmF0 aW9uIFBIWXMsIGdyb3VwCj4gPj4+Pj4gYWxsIHRoZSBvcGVyYXRpb25zIGluIHRoZSBkd19oZG1p X3BoeV9pbml0KCkgZnVuY3Rpb24uCj4gPj4+PiAKPiA+Pj4+IFRoaXMgY2hhbmdlcyB0aGUgYmVo YXZpb3VyIG9mIHRoZSBkcml2ZXIuCj4gPj4+PiAKPiA+Pj4+PiArc3RhdGljIHZvaWQgZHdfaGRt aV9waHlfcG93ZXJfb2ZmKHN0cnVjdCBkd19oZG1pICpoZG1pKQo+ID4+Pj4+ICt7Cj4gPj4+Pj4g KwlpZiAoaGRtaS0+cGh5LT5nZW4gPT0gMSkgewo+ID4+Pj4+ICsJCWR3X2hkbWlfcGh5X2VuYWJs ZV90bWRzKGhkbWksIDApOwo+ID4+Pj4+ICsJCWR3X2hkbWlfcGh5X2VuYWJsZV9wb3dlcmRvd24o aGRtaSwgdHJ1ZSk7Cj4gPj4+Pj4gKwl9IGVsc2Ugewo+ID4+Pj4+ICsJCWR3X2hkbWlfcGh5X2dl bjJfdHhwd3JvbihoZG1pLCAwKTsKPiA+Pj4+PiArCQlkd19oZG1pX3BoeV9nZW4yX3BkZHEoaGRt aSwgMSk7Cj4gPj4+Pj4gKwl9Cj4gPj4+Pj4gK30KPiA+Pj4+PiBAQCAtMTI5MCw5ICsxMzAyLDcg QEAgc3RhdGljIHZvaWQgZHdfaGRtaV9waHlfZGlzYWJsZShzdHJ1Y3QgZHdfaGRtaQo+ID4+Pj4+ ICpoZG1pKQo+ID4+Pj4+ICAJaWYgKCFoZG1pLT5waHlfZW5hYmxlZCkKPiA+Pj4+PiAgCQlyZXR1 cm47Cj4gPj4+Pj4gCj4gPj4+Pj4gLQlkd19oZG1pX3BoeV9lbmFibGVfdG1kcyhoZG1pLCAwKTsK PiA+Pj4+PiAtCWR3X2hkbWlfcGh5X2VuYWJsZV9wb3dlcmRvd24oaGRtaSwgdHJ1ZSk7Cj4gPj4+ Pj4gLQo+ID4+Pj4+ICsJZHdfaGRtaV9waHlfcG93ZXJfb2ZmKGhkbWkpOwo+ID4+Pj4gCj4gPj4+ PiBUaGlzIG1ha2VzIGR3X2hkbWlfcGh5X2Rpc2FibGUoKSBwb3dlciBkb3duIGEgZ2VuMiBwaHku Cj4gPj4+PiAKPiA+Pj4+IFRoZSBpTVg2IGhhcyBhIERXX0hETUlfUEhZX0RXQ19IRE1JXzNEX1RY X1BIWSBwaHksIHdoaWNoIHlvdSBsaXN0IGFzIGEKPiA+Pj4+IGdlbjIgcGh5LiAgSSd2ZSBiZWVu IGNhcnJ5aW5nIHRoaXMgY2hhbmdlIGZvciBhIHdoaWxlLCB3aGljaCBJJ3ZlIGhhZAo+ID4+Pj4g dG8gcmV2ZXJ0IChhbmQgZmluYWxseSBleHB1bmdlKSwgYXMgaXQgY2F1c2VzIHByb2JsZW1zIG9u IGlNWDY6Cj4gPj4+PiAKPiA+Pj4+IEBAIC0xMTEyLDYgKzExMTIsMTQgQEAgc3RhdGljIHZvaWQg ZHdfaGRtaV9waHlfZGlzYWJsZShzdHJ1Y3QgZHdfaGRtaQo+ID4+Pj4gKmhkbWkpCj4gPj4+PiAg ICAgICAgIGlmICghaGRtaS0+cGh5X2VuYWJsZWQpCj4gPj4+PiAgICAgICAgICAgICAgICAgcmV0 dXJuOwo+ID4+Pj4gCj4gPj4+PiArICAgICAgIC8qIEFjdHVhbGx5IHNldCB0aGUgcGh5IGludG8g bG93IHBvd2VyIG1vZGUgKi8KPiA+Pj4+ICsgICAgICAgZHdfaGRtaV9waHlfZ2VuMl90eHB3cm9u KGhkbWksIDApOwo+ID4+Pj4gKwo+ID4+Pj4gKyAgICAgICAvKiBGSVhNRTogV2Ugc2hvdWxkIHdh aXQgZm9yIFRYX1JFQURZIHRvIGJlIGRlYXNzZXJ0ZWQgKi8KPiA+Pj4+ICsKPiA+Pj4+ICsgICAg ICAgZHdfaGRtaV9waHlfZ2VuMl9wZGRxKGhkbWksIDEpOwo+ID4+Pj4gKwo+ID4+Pj4gKyAgICAg ICAvKiBUaGlzIGFwcGVhcnMgdG8gaGF2ZSBubyBlZmZlY3Qgb24gaU1YNiAqLwo+ID4+Pj4gICAg ICAgICBkd19oZG1pX3BoeV9lbmFibGVfdG1kcyhoZG1pLCAwKTsKPiA+Pj4+ICAgICAgICAgZHdf aGRtaV9waHlfZW5hYmxlX3Bvd2VyZG93bihoZG1pLCB0cnVlKTsKPiA+Pj4+IAo+ID4+Pj4gU28s IEkgdGhpbmsgeW91ciBjaGFuZ2UgaGVyZSB3aWxsIGNhdXNlIHByb2JsZW1zIGZvciBpTVg2Lgo+ ID4+Pj4gCj4gPj4+PiBGcm9tIHdoYXQgSSByZW1lbWJlciwgaXQgc2VlbXMgdGhhdCBpTVg2IGhh cyBpc3N1ZXMgd2l0aCBSWFNFTlNFL0hQRAo+ID4+Pj4gYm91bmNpbmcgd2hlbiB0aGUgUEhZIGlz IHBvd2VyZWQgZG93bi4gIEkgY2FuJ3QgcHJvbWlzZSB3aGVuIEknbGwgYmUKPiA+Pj4+IGFibGUg dG8gY2hlY2sgZm9yIHRoYXQgYWdhaW4uCj4gPj4+IAo+ID4+PiBJbmRlZWQgVFhfUkVBRFkgbXVz dCBiZSBsb3cgYmVmb3JlIGFzc2VydGluZyBwZGRxLgo+ID4+IAo+ID4+IFRoZSBUWF9SRUFEWSBz aWduYWwgaXMgZG9jdW1lbnRlZCBpbiB0aGUgaS5NWDYgZGF0YXNoZWV0IGFzIGJlaW5nIGEgUEhZ Cj4gPj4gb3V0cHV0IHNpZ25hbCwgYnV0IHRoZXJlIHNlZW1zIHRvIGJlIG5vIEhETUkgVFggcmVn aXN0ZXIgZnJvbSB3aGljaCBpdHMKPiA+PiBzdGF0ZSBjYW4gYmUgcmVhZC4gRG8gSSBuZWVkIHRv IHBvbGwgdGhlIEhETUlfUEhZX1BUUlBUX0VOQkwgcmVnaXN0ZXIKPiA+PiB0aHJvdWdoIEkyQyA/ IEhvdyBsb25nIGlzIHRoZSBQSFkgZXhwZWN0ZWQgdG8gdGFrZSB0byBzZXQgVFhfUkVBRFkgdG8g MAo+ID4+ID8KPiA+IAo+ID4gVFhfUkVBRFkgY2FuIGJlIHJlYWQgZnJvbSByZWdpc3RlciAweDFB IG9mIHBoeSwgQklUKDIpICh0aHJvdWdoCj4gPiBJMkMpLgo+IAo+IFRoYXQncyB3aGF0IEkgdGhv dWdodCwgSSdsbCBwb2xsIHRoYXQgdGhlbi4gRG8geW91IGhhdmUgYW55IGlkZWEgaG93IGxvbmcK PiBpdCdzIHN1cHBvc2VkIHRvIHRha2UsIHRvIHNldCBhbiBhcHByb3ByaWF0ZSB0aW1lb3V0ID8K Ck9uIGkuTVg2ICgzRCBUWCBQSFkpIHJlZ2lzdGVyIDB4MWEgcmVhZHMgYXMgMHgwMDA3IGJlZm9y ZSBwb3dlcmluZyBkb3duIHRoZSAKUEhZIChieSBkZWFzc2VydGluZyBUWFBXUk9OKSBhbmQgYXMg MHgwMDAwIGltbWVkaWF0ZWx5IGFmdGVyLiBPbiBSSzMyODggKE1ITCAKUEhZKSB0aGUgcmVnaXN0 ZXIgcmVhZHMgYXMgMHgwMjA3IGFuZCBhcyAweDAwMDAgaW1tZWRpYXRlbHkgYWZ0ZXIgZGVhc3Nl cnRpbmcgClRYUFdST04uIEl0IHNlZW1zIHRoYXQgb25lIEkyQyByZWFkIGlzIGEgc3VmZmljaWVu dCBkZWxheSBmb3IgVFhfUkVBRFkgdG8gZ28gCmxvdy4KCj4gPiBOb3Qgc3VyZSBpZiBzYW1lIG9m ZnNldCBmb3IgYWxsIHBoeXMgdGhvdWdoLgo+IAo+IE1vc3QgcHJvYmFibHkgbm90LCBpdCB3b3Vs ZCBiZSB0b28gZWFzeSA6LSkgSSdsbCBpbnZlc3RpZ2F0ZSAod2hpY2ggd2lsbAo+IGxpa2VseSBp bmNsdWRlIGxvdHMgb2YgZ3Vlc3N3b3JrKS4gSWYgeW91IGNhbiBmaW5kIGFueSBpbmZvcm1hdGlv biBhYm91dAo+IHRoYXQgKGFuZCBlc3BlY2lhbGx5IGFib3V0IHRoZSBNSEwgYW5kIEhETUkgMi4w IFBIWXMpIHRoYXQgd291bGQgYmUgdmVyeQo+IGFwcHJlY2lhdGVkLCBhcyBJIGRvbid0IGhhdmUg YWNjZXNzIHRvIGFueSBkb2N1bWVudGF0aW9uIHRoYXQgbWVudGlvbnMgYQo+IFRYX1JFQURZIGJp dCBmb3IgdGhvc2UuCgpJIGhhdmVuJ3QgdGVzdGVkIHRoZSBIRE1JIDIuMCBQSFkgeWV0LCBidXQg SSdkIGJlIHN1cnByaXNlZCBpZiB0aGUgVFhfUkVBRFkgCmJpdCB3YXMgaW4gdGhlIHNhbWUgcmVn aXN0ZXIgYXQgdGhlIHNhbWUgcG9zaXRpb24uCgotLSAKUmVnYXJkcywKCkxhdXJlbnQgUGluY2hh cnQKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1k ZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczov L2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo=