From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Stuebner Subject: Re: [PATCH v2 3/8] drm/bridge: dw-hdmi: allow overriding of phy-type reading Date: Wed, 21 Feb 2018 19:55:12 +0100 Message-ID: <3085916.UFWCNBIZfU@phil> References: <20180216204158.29839-1-heiko@sntech.de> <3104429.3dFhngZnWQ@phil> <16512170.VqSpgYmDPS@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <16512170.VqSpgYmDPS@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Laurent Pinchart 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 SGkgTGF1cmVudCwKCkFtIE1vbnRhZywgMTkuIEZlYnJ1YXIgMjAxOCwgMjA6MDY6NDAgQ0VUIHNj aHJpZWIgTGF1cmVudCBQaW5jaGFydDoKPiBPbiBNb25kYXksIDE5IEZlYnJ1YXJ5IDIwMTggMjA6 NDY6NDYgRUVUIEhlaWtvIFN0dWVibmVyIHdyb3RlOgo+ID4gQW0gTW9udGFnLCAxOS4gRmVicnVh ciAyMDE4LCAxNzo1OTowMiBDRVQgc2NocmllYiBMYXVyZW50IFBpbmNoYXJ0Ogo+ID4gPiBPbiBG cmlkYXksIDE2IEZlYnJ1YXJ5IDIwMTggMjI6NDE6NTMgRUVUIEhlaWtvIFN0dWVibmVyIHdyb3Rl Ogo+ID4gPj4gSW4gc29tZSBJUCBpbXBsZW1lbnRhdGlvbnMgdGhlIHJlYWRpbmcgb2YgdGhlIHBo eS10eXBlIG1heSBiZSBicm9rZW4uCj4gPiA+PiBPbmUgZXhhbXBsZSBhcmUgdGhlIFJvY2tjaGlw IHJrMzIyOCBhbmQgcmszMzI4IHNvY3MgdGhhdCB1c2UgYSBzZXBhcmF0ZQo+ID4gPj4gcGh5IGZy b20gSW5ub3NpbGljb24gYnV0IHN0aWxsIHJlcG9ydCB0aGUgSERNSTIwX1RYIHR5cGUuCj4gPiA+ PiAKPiA+ID4+IFNvIGFsbG93IHRoZSBnbHVlIGRyaXZlciB0byBmb3JjZSBhIHNwZWNpZmljIHR5 cGUsIGxpa2UgdGhlIHZlbmRvci1waHkKPiA+ID4+IGZvciB0aGVzZSBjYXNlcy4KPiA+ID4+IAo+ ID4gPj4gU2lnbmVkLW9mZi1ieTogSGVpa28gU3R1ZWJuZXIgPGhlaWtvQHNudGVjaC5kZT4KPiA+ ID4gCj4gPiA+IFRlc3RlZC1ieTogTGF1cmVudCBQaW5jaGFydCA8bGF1cmVudC5waW5jaGFydEBp ZGVhc29uYm9hcmQuY29tPgo+ID4gCj4gPiB0aGFua3MgZm9yIHRlc3RpbmcgOi0pCj4gPiAKPiA+ ID4+IC0tLQo+ID4gPj4gCj4gPiA+PiAgZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9zeW5vcHN5cy9k dy1oZG1pLmMgfCA0ICsrKy0KPiA+ID4+ICBpbmNsdWRlL2RybS9icmlkZ2UvZHdfaGRtaS5oICAg ICAgICAgICAgICB8IDEgKwo+ID4gPj4gIDIgZmlsZXMgY2hhbmdlZCwgNCBpbnNlcnRpb25zKCsp LCAxIGRlbGV0aW9uKC0pCj4gPiA+PiAKPiA+ID4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9k cm0vYnJpZGdlL3N5bm9wc3lzL2R3LWhkbWkuYwo+ID4gPj4gYi9kcml2ZXJzL2dwdS9kcm0vYnJp ZGdlL3N5bm9wc3lzL2R3LWhkbWkuYyBpbmRleAo+ID4gPj4gZjk4MDIzOTljYzBkLi41MGQyMzE2 MjZjNGQgMTAwNjQ0Cj4gPiA+PiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0vYnJpZGdlL3N5bm9wc3lz L2R3LWhkbWkuYwo+ID4gPj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9zeW5vcHN5cy9k dy1oZG1pLmMKPiA+ID4+IEBAIC0yMjE4LDcgKzIyMTgsOSBAQCBzdGF0aWMgaW50IGR3X2hkbWlf ZGV0ZWN0X3BoeShzdHJ1Y3QgZHdfaGRtaQo+ID4gPj4gKmhkbWkpCj4gPiA+PiAgCXVuc2lnbmVk IGludCBpOwo+ID4gPj4gIAl1OCBwaHlfdHlwZTsKPiA+ID4+IAo+ID4gPj4gLQlwaHlfdHlwZSA9 IGhkbWlfcmVhZGIoaGRtaSwgSERNSV9DT05GSUcyX0lEKTsKPiA+ID4+ICsJcGh5X3R5cGUgPSAo aGRtaS0+cGxhdF9kYXRhLT5waHlfZm9yY2VfdHlwZSkgPwo+ID4gPj4gKwkJCQloZG1pLT5wbGF0 X2RhdGEtPnBoeV9mb3JjZV90eXBlIDoKPiA+ID4+ICsJCQkJaGRtaV9yZWFkYihoZG1pLCBIRE1J X0NPTkZJRzJfSUQpOwo+ID4gPiAKPiA+ID4gTm8gbmVlZCBmb3IgcGFyZW50aGVzZXMuIFlvdSBj b3VsZCBldmVuIHdyaXRlIHRoaXMKPiA+ID4gCj4gPiA+ICAgICAgICAgcGh5X3R5cGUgPSBoZG1p LT5wbGF0X2RhdGEtPnBoeV9mb3JjZV90eXBlID8KPiA+ID4gICAgICAgICAgICAgICAgICA6IGhk bWlfcmVhZGIoaGRtaSwgSERNSV9DT05GSUcyX0lEKTsKPiA+ID4gCj4gPiA+IGJ1dCB0aGF0J3Mg dXAgdG8geW91Lgo+ID4gPiAKPiA+ID4gV2hhdCBpZiBhIGRyaXZlciB3YW50cyB0byBmb3JjZSB0 aGUgUEhZIHR5cGUgdG8KPiA+ID4gRFdfSERNSV9QSFlfRFdDX0hETUlfVFhfUEhZID8gT3IgZG8g eW91IGV4cGVjdCBvbmx5IHRoZQo+ID4gPiBEV19IRE1JX1BIWV9WRU5ET1JfUEhZIHR5cGUgdG8g YmUgZm9yY2VkID8gSWYgc28sIHdlIGNvdWxkIGFsc28gdXNlIGEKPiA+ID4gZm9yY2VfdmVuZG9y X3BoeSBib29sZWFuIGZpZWxkIGluc3RlYWQgb2YgcGh5X2ZvcmNlX3R5cGUuCj4gPiAKPiA+IElu aXRpYWxseSBJIHRob3VnaHQgb2YgaW1wbGljaXRseSBvdmVycmlkaW5nIHRoZSBwaHktdHlwZSB3 aGVuIHRoZSBleHRlcm5hbAo+ID4gcGh5LW9wcyBhcmUgc2V0LCBidXQgZGVjaWRlZCBhZ2FpbnN0 IGl0IGJlY2F1c2UgdGhhdCBtaWdodCBicmVhawo+ID4gKHRoZW9yZXRpY2FsKSBjYXNlcyB3aGVy ZSBwaHktb3BzIG1heSBiZSBhbHdheXMgc2V0IGJ1dCBvbmx5IHVzZWQgd2hlbgo+ID4gdGhlIGlw IHJldHVybnMgYSBtYXRjaGluZyBwaHktdHlwZSBhbmQgdGh1cyBjYW1lIHRvIGp1c3QgYWxsb3cg b3ZlcnJpZGluZwo+ID4gdGhhdCB0eXBlIHJlYWRpbmcuCj4gPiAKPiA+IEFzIGZvciBsaW1pdGlu ZyB0byBvbmx5IGFsbG93IGZvcmNpbmcgdGhlIHZlbmRvciB0eXBlLCBteSBwZXJzb25hbCBmZWVs aW5nCj4gPiB3b3VsZCBiZSB0byBhbGxvdyBnbHVlIGRyaXZlcnMgdG8ganVzdCBvdmVycmlkZSB0 aGUgdHlwZSBhcyBuZWVkZWQKPiA+IGxpa2UgZG9uZSBpbiB0aGUgcGF0Y2guIEFzIGNhbiBiZSBz ZWVuIG9uIHRoZSByazMzMjgsIHRoZSBkdy1oZG1pCj4gPiByZXBvcnRzIG9uZSBvZiB0aGUgcmVn dWxhciBwaHlzIChmb3Jnb3Qgd2hpY2ggb25lKSBidXQgaW5zdGVhZCBoYXMgYQo+ID4gY29tcGxl dGVseSBzZXBhcmF0ZSBpcCBmb3IgaXQsIHNvIEknZCBndWVzcyB3ZSBtYXkgdmVyeSB3ZWxsIHNl ZQo+ID4gaW1wbGVtZW50YXRpb25zIHRoYXQganVzdCByZXBvcnQgYSB3cm9uZyB0eXBlIChubyB2 ZW5kb3ItdHlwZSkuCj4gPiAKPiA+IFNvIEkgZG9uJ3Qgc2VlIGFuIGlzc3VlIHdpdGggZHJpdmVy cyBiZWluZyBhbGxvd2VkIHRvIHNldCB0aGUgdHlwZSB0bwo+ID4gRFdfSERNSV9QSFlfRFdDX0hE TUlfVFhfUEhZLCBiZWNhdXNlIHN1Y2ggY2FzZXMgbWF5IGV4aXN0IGluIHRoZQo+ID4gZnV0dXJl IGFuZCBJJ2QgZXhwZWN0IGRyaXZlciBhdXRob3JzIHRvIHNvbWV3aGF0IGtub3cgd2hhdCB0aGV5 J3JlIGRvaW5nLgo+IAo+IE15IHBvaW50IHdhcyB0aGF0IERXX0hETUlfUEhZX0RXQ19IRE1JX1RY X1BIWSA9PSAwLCBzbyB0cnlpbmcgdG8gZm9yY2UgCj4gRFdfSERNSV9QSFlfRFdDX0hETUlfVFhf UEhZIHRocm91Z2ggcGh5X2ZvcmNlX3R5cGUgd29uJ3Qgd29yay4KCkFyZ2gsIG9rIG5vdyBJIGdl dCBpdC4KClNvIGl0IHdpbGwgbmVlZCBzb21lIGFkYXB0aW9uIGZvciB0aGF0LCBiZWNhdXNlIGFs bG93aW5nIGV2ZXJ5dGhpbmcgYnV0CkRXX0hETUlfUEhZX0RXQ19IRE1JX1RYX1BIWSB3b3VsZCBi ZSBxdWl0ZSBjb3VudGVyLWludHVpdGl2ZSA6LSkgLgoKRXhwZWN0aW5nIHBoeV9mb3JjZV90eXBl ID09IC0xIGZvciByZWd1bGFyIHJlYWRzIHNvdW5kcyBiYWQgYXMgd2VsbCwKYmVjYXVzZSB0aGVu IGV2ZXJ5IGdsdWUgZHJpdmVyIHdvdWxkIG5lZWQgdG8gc2V0IHRoYXQsIHdoaWNoIGN1cnJlbnRs eQpnZXRzIHNvbHZlcyBhdXRvbWF0aWNhbGx5IHdoZW4gdGhlIGZpZWxkIGlzIG5vdCBleHBsaWNp dGx5IHNldC4KClNvIGdvaW5nIHdpdGggeW91ciBmb3JjZV92ZW5kb3JfcGh5IGlkZWEgc291bmRz IGxlc3MgaW50cnVzaXZlIGZvciB0aGUKdGltZSBiZWluZyBhbmQgdW50aWwgdGhlcmUgaXMgYW4g YWN0dWFsIGNhc2Ugd2hlcmUgdGhlcmUgaXMgYSBkaWZmZXJlbnQKaW50ZXJuYWwgcGh5LXR5cGUg bmVjZXNzYXJ5PwpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f XwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcK aHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: heiko@sntech.de (Heiko Stuebner) Date: Wed, 21 Feb 2018 19:55:12 +0100 Subject: [PATCH v2 3/8] drm/bridge: dw-hdmi: allow overriding of phy-type reading In-Reply-To: <16512170.VqSpgYmDPS@avalon> References: <20180216204158.29839-1-heiko@sntech.de> <3104429.3dFhngZnWQ@phil> <16512170.VqSpgYmDPS@avalon> Message-ID: <3085916.UFWCNBIZfU@phil> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Laurent, 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?