From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v2 3/8] drm/bridge: dw-hdmi: allow overriding of phy-type reading Date: Wed, 21 Feb 2018 21:15:26 +0200 Message-ID: <4888467.l85TM0qZuY@avalon> References: <20180216204158.29839-1-heiko@sntech.de> <16512170.VqSpgYmDPS@avalon> <3085916.UFWCNBIZfU@phil> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <3085916.UFWCNBIZfU@phil> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Heiko Stuebner Cc: mark.rutland@arm.com, Jose.Abreu@synopsys.com, algea.cao@rock-chips.com, devicetree@vger.kernel.org, airlied@linux.ie, dri-devel@lists.freedesktop.org, kishon@ti.com, robh+dt@kernel.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org, zhengyang@rock-chips.com List-Id: linux-rockchip.vger.kernel.org SGkgSGVpa28sCgpPbiBXZWRuZXNkYXksIDIxIEZlYnJ1YXJ5IDIwMTggMjA6NTU6MTIgRUVUIEhl aWtvIFN0dWVibmVyIHdyb3RlOgo+IEFtIE1vbnRhZywgMTkuIEZlYnJ1YXIgMjAxOCwgMjA6MDY6 NDAgQ0VUIHNjaHJpZWIgTGF1cmVudCBQaW5jaGFydDoKPiA+IE9uIE1vbmRheSwgMTkgRmVicnVh cnkgMjAxOCAyMDo0Njo0NiBFRVQgSGVpa28gU3R1ZWJuZXIgd3JvdGU6Cj4gPiA+IEFtIE1vbnRh ZywgMTkuIEZlYnJ1YXIgMjAxOCwgMTc6NTk6MDIgQ0VUIHNjaHJpZWIgTGF1cmVudCBQaW5jaGFy dDoKPiA+ID4gPiBPbiBGcmlkYXksIDE2IEZlYnJ1YXJ5IDIwMTggMjI6NDE6NTMgRUVUIEhlaWtv IFN0dWVibmVyIHdyb3RlOgo+ID4gPiA+PiBJbiBzb21lIElQIGltcGxlbWVudGF0aW9ucyB0aGUg cmVhZGluZyBvZiB0aGUgcGh5LXR5cGUgbWF5IGJlIGJyb2tlbi4KPiA+ID4gPj4gT25lIGV4YW1w bGUgYXJlIHRoZSBSb2NrY2hpcCByazMyMjggYW5kIHJrMzMyOCBzb2NzIHRoYXQgdXNlIGEKPiA+ ID4gPj4gc2VwYXJhdGUKPiA+ID4gPj4gcGh5IGZyb20gSW5ub3NpbGljb24gYnV0IHN0aWxsIHJl cG9ydCB0aGUgSERNSTIwX1RYIHR5cGUuCj4gPiA+ID4+IAo+ID4gPiA+PiBTbyBhbGxvdyB0aGUg Z2x1ZSBkcml2ZXIgdG8gZm9yY2UgYSBzcGVjaWZpYyB0eXBlLCBsaWtlIHRoZQo+ID4gPiA+PiB2 ZW5kb3ItcGh5Cj4gPiA+ID4+IGZvciB0aGVzZSBjYXNlcy4KPiA+ID4gPj4gCj4gPiA+ID4+IFNp Z25lZC1vZmYtYnk6IEhlaWtvIFN0dWVibmVyIDxoZWlrb0BzbnRlY2guZGU+Cj4gPiA+ID4gCj4g PiA+ID4gVGVzdGVkLWJ5OiBMYXVyZW50IFBpbmNoYXJ0IDxsYXVyZW50LnBpbmNoYXJ0QGlkZWFz b25ib2FyZC5jb20+Cj4gPiA+IAo+ID4gPiB0aGFua3MgZm9yIHRlc3RpbmcgOi0pCj4gPiA+IAo+ ID4gPiA+PiAtLS0KPiA+ID4gPj4gCj4gPiA+ID4+ICBkcml2ZXJzL2dwdS9kcm0vYnJpZGdlL3N5 bm9wc3lzL2R3LWhkbWkuYyB8IDQgKysrLQo+ID4gPiA+PiAgaW5jbHVkZS9kcm0vYnJpZGdlL2R3 X2hkbWkuaCAgICAgICAgICAgICAgfCAxICsKPiA+ID4gPj4gIDIgZmlsZXMgY2hhbmdlZCwgNCBp bnNlcnRpb25zKCspLCAxIGRlbGV0aW9uKC0pCj4gPiA+ID4+IAo+ID4gPiA+PiBkaWZmIC0tZ2l0 IGEvZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9zeW5vcHN5cy9kdy1oZG1pLmMKPiA+ID4gPj4gYi9k cml2ZXJzL2dwdS9kcm0vYnJpZGdlL3N5bm9wc3lzL2R3LWhkbWkuYyBpbmRleAo+ID4gPiA+PiBm OTgwMjM5OWNjMGQuLjUwZDIzMTYyNmM0ZCAxMDA2NDQKPiA+ID4gPj4gLS0tIGEvZHJpdmVycy9n cHUvZHJtL2JyaWRnZS9zeW5vcHN5cy9kdy1oZG1pLmMKPiA+ID4gPj4gKysrIGIvZHJpdmVycy9n cHUvZHJtL2JyaWRnZS9zeW5vcHN5cy9kdy1oZG1pLmMKPiA+ID4gPj4gQEAgLTIyMTgsNyArMjIx OCw5IEBAIHN0YXRpYyBpbnQgZHdfaGRtaV9kZXRlY3RfcGh5KHN0cnVjdCBkd19oZG1pCj4gPiA+ ID4+ICpoZG1pKQo+ID4gPiA+PiAKPiA+ID4gPj4gIAl1bnNpZ25lZCBpbnQgaTsKPiA+ID4gPj4g IAl1OCBwaHlfdHlwZTsKPiA+ID4gPj4gCj4gPiA+ID4+IC0JcGh5X3R5cGUgPSBoZG1pX3JlYWRi KGhkbWksIEhETUlfQ09ORklHMl9JRCk7Cj4gPiA+ID4+ICsJcGh5X3R5cGUgPSAoaGRtaS0+cGxh dF9kYXRhLT5waHlfZm9yY2VfdHlwZSkgPwo+ID4gPiA+PiArCQkJCWhkbWktPnBsYXRfZGF0YS0+ cGh5X2ZvcmNlX3R5cGUgOgo+ID4gPiA+PiArCQkJCWhkbWlfcmVhZGIoaGRtaSwgSERNSV9DT05G SUcyX0lEKTsKPiA+ID4gPiAKPiA+ID4gPiBObyBuZWVkIGZvciBwYXJlbnRoZXNlcy4gWW91IGNv dWxkIGV2ZW4gd3JpdGUgdGhpcwo+ID4gPiA+IAo+ID4gPiA+ICAgICAgICAgcGh5X3R5cGUgPSBo ZG1pLT5wbGF0X2RhdGEtPnBoeV9mb3JjZV90eXBlID8KPiA+ID4gPiAgICAgICAgIAo+ID4gPiA+ ICAgICAgICAgICAgICAgICAgOiBoZG1pX3JlYWRiKGhkbWksIEhETUlfQ09ORklHMl9JRCk7Cj4g PiA+ID4gCj4gPiA+ID4gYnV0IHRoYXQncyB1cCB0byB5b3UuCj4gPiA+ID4gCj4gPiA+ID4gV2hh dCBpZiBhIGRyaXZlciB3YW50cyB0byBmb3JjZSB0aGUgUEhZIHR5cGUgdG8KPiA+ID4gPiBEV19I RE1JX1BIWV9EV0NfSERNSV9UWF9QSFkgPyBPciBkbyB5b3UgZXhwZWN0IG9ubHkgdGhlCj4gPiA+ ID4gRFdfSERNSV9QSFlfVkVORE9SX1BIWSB0eXBlIHRvIGJlIGZvcmNlZCA/IElmIHNvLCB3ZSBj b3VsZCBhbHNvIHVzZSBhCj4gPiA+ID4gZm9yY2VfdmVuZG9yX3BoeSBib29sZWFuIGZpZWxkIGlu c3RlYWQgb2YgcGh5X2ZvcmNlX3R5cGUuCj4gPiA+IAo+ID4gPiBJbml0aWFsbHkgSSB0aG91Z2h0 IG9mIGltcGxpY2l0bHkgb3ZlcnJpZGluZyB0aGUgcGh5LXR5cGUgd2hlbiB0aGUKPiA+ID4gZXh0 ZXJuYWwKPiA+ID4gcGh5LW9wcyBhcmUgc2V0LCBidXQgZGVjaWRlZCBhZ2FpbnN0IGl0IGJlY2F1 c2UgdGhhdCBtaWdodCBicmVhawo+ID4gPiAodGhlb3JldGljYWwpIGNhc2VzIHdoZXJlIHBoeS1v cHMgbWF5IGJlIGFsd2F5cyBzZXQgYnV0IG9ubHkgdXNlZCB3aGVuCj4gPiA+IHRoZSBpcCByZXR1 cm5zIGEgbWF0Y2hpbmcgcGh5LXR5cGUgYW5kIHRodXMgY2FtZSB0byBqdXN0IGFsbG93Cj4gPiA+ IG92ZXJyaWRpbmcKPiA+ID4gdGhhdCB0eXBlIHJlYWRpbmcuCj4gPiA+IAo+ID4gPiBBcyBmb3Ig bGltaXRpbmcgdG8gb25seSBhbGxvdyBmb3JjaW5nIHRoZSB2ZW5kb3IgdHlwZSwgbXkgcGVyc29u YWwKPiA+ID4gZmVlbGluZwo+ID4gPiB3b3VsZCBiZSB0byBhbGxvdyBnbHVlIGRyaXZlcnMgdG8g anVzdCBvdmVycmlkZSB0aGUgdHlwZSBhcyBuZWVkZWQKPiA+ID4gbGlrZSBkb25lIGluIHRoZSBw YXRjaC4gQXMgY2FuIGJlIHNlZW4gb24gdGhlIHJrMzMyOCwgdGhlIGR3LWhkbWkKPiA+ID4gcmVw b3J0cyBvbmUgb2YgdGhlIHJlZ3VsYXIgcGh5cyAoZm9yZ290IHdoaWNoIG9uZSkgYnV0IGluc3Rl YWQgaGFzIGEKPiA+ID4gY29tcGxldGVseSBzZXBhcmF0ZSBpcCBmb3IgaXQsIHNvIEknZCBndWVz cyB3ZSBtYXkgdmVyeSB3ZWxsIHNlZQo+ID4gPiBpbXBsZW1lbnRhdGlvbnMgdGhhdCBqdXN0IHJl cG9ydCBhIHdyb25nIHR5cGUgKG5vIHZlbmRvci10eXBlKS4KPiA+ID4gCj4gPiA+IFNvIEkgZG9u J3Qgc2VlIGFuIGlzc3VlIHdpdGggZHJpdmVycyBiZWluZyBhbGxvd2VkIHRvIHNldCB0aGUgdHlw ZSB0bwo+ID4gPiBEV19IRE1JX1BIWV9EV0NfSERNSV9UWF9QSFksIGJlY2F1c2Ugc3VjaCBjYXNl cyBtYXkgZXhpc3QgaW4gdGhlCj4gPiA+IGZ1dHVyZSBhbmQgSSdkIGV4cGVjdCBkcml2ZXIgYXV0 aG9ycyB0byBzb21ld2hhdCBrbm93IHdoYXQgdGhleSdyZQo+ID4gPiBkb2luZy4KPiA+IAo+ID4g TXkgcG9pbnQgd2FzIHRoYXQgRFdfSERNSV9QSFlfRFdDX0hETUlfVFhfUEhZID09IDAsIHNvIHRy eWluZyB0byBmb3JjZQo+ID4gRFdfSERNSV9QSFlfRFdDX0hETUlfVFhfUEhZIHRocm91Z2ggcGh5 X2ZvcmNlX3R5cGUgd29uJ3Qgd29yay4KPiAKPiBBcmdoLCBvayBub3cgSSBnZXQgaXQuCj4gCj4g U28gaXQgd2lsbCBuZWVkIHNvbWUgYWRhcHRpb24gZm9yIHRoYXQsIGJlY2F1c2UgYWxsb3dpbmcg ZXZlcnl0aGluZyBidXQKPiBEV19IRE1JX1BIWV9EV0NfSERNSV9UWF9QSFkgd291bGQgYmUgcXVp dGUgY291bnRlci1pbnR1aXRpdmUgOi0pIC4KPiAKPiBFeHBlY3RpbmcgcGh5X2ZvcmNlX3R5cGUg PT0gLTEgZm9yIHJlZ3VsYXIgcmVhZHMgc291bmRzIGJhZCBhcyB3ZWxsLAo+IGJlY2F1c2UgdGhl biBldmVyeSBnbHVlIGRyaXZlciB3b3VsZCBuZWVkIHRvIHNldCB0aGF0LCB3aGljaCBjdXJyZW50 bHkKPiBnZXRzIHNvbHZlcyBhdXRvbWF0aWNhbGx5IHdoZW4gdGhlIGZpZWxkIGlzIG5vdCBleHBs aWNpdGx5IHNldC4KPiAKPiBTbyBnb2luZyB3aXRoIHlvdXIgZm9yY2VfdmVuZG9yX3BoeSBpZGVh IHNvdW5kcyBsZXNzIGludHJ1c2l2ZSBmb3IgdGhlCj4gdGltZSBiZWluZyBhbmQgdW50aWwgdGhl cmUgaXMgYW4gYWN0dWFsIGNhc2Ugd2hlcmUgdGhlcmUgaXMgYSBkaWZmZXJlbnQKPiBpbnRlcm5h bCBwaHktdHlwZSBuZWNlc3Nhcnk/CgpUaGF0J3MgYXQgbGVhc3Qgd2hhdCBJJ2QgcmVjb21tZW5k LCBhcyBJIGRvbid0IHNlZSBhbnkgdXNlIGNhc2Ugbm93IGZvciAKb3ZlcnJpZGluZyB0aGUgaGFy ZHdhcmUtcmVwb3J0ZWQgUEhZIHR5cGUgdG8gYSBzdGFuZGFyZCBQSFkgdHlwZS4gSWYgd2UgZXZl ciAKZW5kIHVwIG5lZWRpbmcgdGhhdCBpbiB0aGUgZnV0dXJlIHdlIHdpbGwgb2YgY291cnNlIHN1 cHBvcnQgaXQuCgotLSAKUmVnYXJkcywKCkxhdXJlbnQgUGluY2hhcnQKCl9fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QK ZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9w Lm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Wed, 21 Feb 2018 21:15:26 +0200 Subject: [PATCH v2 3/8] drm/bridge: dw-hdmi: allow overriding of phy-type reading In-Reply-To: <3085916.UFWCNBIZfU@phil> References: <20180216204158.29839-1-heiko@sntech.de> <16512170.VqSpgYmDPS@avalon> <3085916.UFWCNBIZfU@phil> Message-ID: <4888467.l85TM0qZuY@avalon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Heiko, On Wednesday, 21 February 2018 20:55:12 EET Heiko Stuebner wrote: > Am Montag, 19. Februar 2018, 20:06:40 CET schrieb Laurent Pinchart: > > On Monday, 19 February 2018 20:46:46 EET Heiko Stuebner wrote: > > > Am Montag, 19. Februar 2018, 17:59:02 CET schrieb Laurent Pinchart: > > > > On Friday, 16 February 2018 22:41:53 EET Heiko Stuebner wrote: > > > >> In some IP implementations the reading of the phy-type may be broken. > > > >> One example are the Rockchip rk3228 and rk3328 socs that use a > > > >> separate > > > >> phy from Innosilicon but still report the HDMI20_TX type. > > > >> > > > >> So allow the glue driver to force a specific type, like the > > > >> vendor-phy > > > >> for these cases. > > > >> > > > >> Signed-off-by: Heiko Stuebner > > > > > > > > Tested-by: Laurent Pinchart > > > > > > thanks for testing :-) > > > > > > >> --- > > > >> > > > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 +++- > > > >> include/drm/bridge/dw_hdmi.h | 1 + > > > >> 2 files changed, 4 insertions(+), 1 deletion(-) > > > >> > > > >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > > >> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > > > >> f9802399cc0d..50d231626c4d 100644 > > > >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > > >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > > >> @@ -2218,7 +2218,9 @@ static int dw_hdmi_detect_phy(struct dw_hdmi > > > >> *hdmi) > > > >> > > > >> unsigned int i; > > > >> u8 phy_type; > > > >> > > > >> - phy_type = hdmi_readb(hdmi, HDMI_CONFIG2_ID); > > > >> + phy_type = (hdmi->plat_data->phy_force_type) ? > > > >> + hdmi->plat_data->phy_force_type : > > > >> + hdmi_readb(hdmi, HDMI_CONFIG2_ID); > > > > > > > > No need for parentheses. You could even write this > > > > > > > > phy_type = hdmi->plat_data->phy_force_type ? > > > > > > > > : hdmi_readb(hdmi, HDMI_CONFIG2_ID); > > > > > > > > but that's up to you. > > > > > > > > What if a driver wants to force the PHY type to > > > > DW_HDMI_PHY_DWC_HDMI_TX_PHY ? Or do you expect only the > > > > DW_HDMI_PHY_VENDOR_PHY type to be forced ? If so, we could also use a > > > > force_vendor_phy boolean field instead of phy_force_type. > > > > > > Initially I thought of implicitly overriding the phy-type when the > > > external > > > phy-ops are set, but decided against it because that might break > > > (theoretical) cases where phy-ops may be always set but only used when > > > the ip returns a matching phy-type and thus came to just allow > > > overriding > > > that type reading. > > > > > > As for limiting to only allow forcing the vendor type, my personal > > > feeling > > > would be to allow glue drivers to just override the type as needed > > > like done in the patch. As can be seen on the rk3328, the dw-hdmi > > > reports one of the regular phys (forgot which one) but instead has a > > > completely separate ip for it, so I'd guess we may very well see > > > implementations that just report a wrong type (no vendor-type). > > > > > > So I don't see an issue with drivers being allowed to set the type to > > > DW_HDMI_PHY_DWC_HDMI_TX_PHY, because such cases may exist in the > > > future and I'd expect driver authors to somewhat know what they're > > > doing. > > > > My point was that DW_HDMI_PHY_DWC_HDMI_TX_PHY == 0, so trying to force > > DW_HDMI_PHY_DWC_HDMI_TX_PHY through phy_force_type won't work. > > Argh, ok now I get it. > > So it will need some adaption for that, because allowing everything but > DW_HDMI_PHY_DWC_HDMI_TX_PHY would be quite counter-intuitive :-) . > > Expecting phy_force_type == -1 for regular reads sounds bad as well, > because then every glue driver would need to set that, which currently > gets solves automatically when the field is not explicitly set. > > So going with your force_vendor_phy idea sounds less intrusive for the > time being and until there is an actual case where there is a different > internal phy-type necessary? That's at least what I'd recommend, as I don't see any use case now for overriding the hardware-reported PHY type to a standard PHY type. If we ever end up needing that in the future we will of course support it. -- Regards, Laurent Pinchart