From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from galahad.ideasonboard.com ([185.26.127.97]:58980 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752617AbdCBQeJ (ORCPT ); Thu, 2 Mar 2017 11:34:09 -0500 From: Laurent Pinchart To: Jose Abreu Cc: Laurent Pinchart , dri-devel@lists.freedesktop.org, Andy Yan , Fabio Estevam , Kieran Bingham , Neil Armstrong , Nickey Yang , Russell King , Vladimir Zapolskiy , linux-renesas-soc@vger.kernel.org Subject: Re: [PATCH v4 7/9] drm: bridge: dw-hdmi: Add support for custom PHY configuration Date: Thu, 02 Mar 2017 17:38:51 +0200 Message-ID: <20121862.XIvFFZlWoF@avalon> In-Reply-To: References: <20170301223915.29888-1-laurent.pinchart+renesas@ideasonboard.com> <1616563.9xlBnf0tlo@avalon> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-renesas-soc-owner@vger.kernel.org List-ID: Hi Jose, On Thursday 02 Mar 2017 14:50:02 Jose Abreu wrote: > On 02-03-2017 13:41, Laurent Pinchart wrote: > >> Hmm, this is kind of confusing. Why do you need a phy_ops and > >> then a separate configure_phy function? Can't we just use phy_ops > >> always? If its external phy then it would need to implement all > >> phy_ops functions. > > > > The phy_ops are indeed meant to support vendor PHYs. The .configure_phy() > > operation is meant to support Synopsys PHYs that don't comply with the > > HDMI TX MHL and 3D PHYs I2C register layout and thus need a different > > configure function, but can share the other operations with the HDMI TX > > MHL and 3D PHYs. Ideally I'd like to move that code to the dw-hdmi core, > > but at the moment I don't have enough information to decide whether the > > register layout corresponds to the standard Synopsys HDMI TX 2.0 PHY or > > if it has been modified by the vendor. Once we'll have a second SoC using > > the same PHY we should have more information to consolidate the code. Of > > course, if you can share the HDMI TX 2.0 PHY datasheet, I won't mind > > reworking the code now ;-) > > Well, if I want to keep my job I can't share the datasheet, and I > do want to keep my job :) That's so selfish :-D > As far as I know this shouldn't change much. I already used (it > was like a year ago) the dw-hdmi driver in a HDMI TX 2.0 PHY. I really wonder what exactly has been integrated in the Renesas R-Car Gen3 SoC. The PHY is certainly reported as an HDMI TX 2.0 PHY, but the registers seem different compared to the 2.0 PHY you've tested. > But I am not following your logic here, sorry: You are mentioning a > custom phy, right? Custom is probably a bad name. From what I've been told it's a standard Synopsys PHY, but I can't rule out vendor-specific customizations. > If its custom then it should implement its own phy_ops. And I don't think > that phy logic should go into core, this should all be extracted because I > really believe it will make the code easier to read. Imagine this > organization: > > Folders/Files: > drm/bridge/dw-hdmi/dw-hdmi-core.c > drm/bridge/dw-hdmi/dw-hdmi-phy-synopsys.c > drm/bridge/dw-hdmi/dw-hdmi-phy-*renesas*.c > drm/bridge/dw-hdmi/dw-hdmi-phy-something.c > Structures: > dw_hdmi_pdata > dw_hdmi_phy_pdata > dw_hdmi_phy_ops That looks good to me. > As the phy is interacted using controller we would need to pass > some callbacks to the phy, but ultimately the phy would be a > platform driver which could have its own compatible string that > would be associated with controller (not sure exactly about this > because I only used this in non-dt). We already have glue code, having to provide separate glue and PHY drivers seems a bit overkill to me. If we could get rid of glue code by adding a node for the PHY in DT that would be nice, but if we have to keep the glue then we can as well use platform data there. > This is just an idea though. I followed this logic in the RX side > and its very easy to add a new phy now, its a matter of creating > a new file, implement ops and associate it with controller. Of > course some phys will be very similar, for that we can use minor > tweaks via id detection. > > What do you think? It sounds neat overall, but I wonder whether it should really block this series, or be added on top of it :-) I think we're already moving in the right direction here. -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v4 7/9] drm: bridge: dw-hdmi: Add support for custom PHY configuration Date: Thu, 02 Mar 2017 17:38:51 +0200 Message-ID: <20121862.XIvFFZlWoF@avalon> References: <20170301223915.29888-1-laurent.pinchart+renesas@ideasonboard.com> <1616563.9xlBnf0tlo@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 53EA86EB9D for ; Thu, 2 Mar 2017 15:38:18 +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 , Neil Armstrong , Kieran Bingham , dri-devel@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, Nickey Yang , Russell King , Andy Yan , Vladimir Zapolskiy List-Id: dri-devel@lists.freedesktop.org SGkgSm9zZSwKCk9uIFRodXJzZGF5IDAyIE1hciAyMDE3IDE0OjUwOjAyIEpvc2UgQWJyZXUgd3Jv dGU6Cj4gT24gMDItMDMtMjAxNyAxMzo0MSwgTGF1cmVudCBQaW5jaGFydCB3cm90ZToKPiA+PiBI bW0sIHRoaXMgaXMga2luZCBvZiBjb25mdXNpbmcuIFdoeSBkbyB5b3UgbmVlZCBhIHBoeV9vcHMg YW5kCj4gPj4gdGhlbiBhIHNlcGFyYXRlIGNvbmZpZ3VyZV9waHkgZnVuY3Rpb24/IENhbid0IHdl IGp1c3QgdXNlIHBoeV9vcHMKPiA+PiBhbHdheXM/IElmIGl0cyBleHRlcm5hbCBwaHkgdGhlbiBp dCB3b3VsZCBuZWVkIHRvIGltcGxlbWVudCBhbGwKPiA+PiBwaHlfb3BzIGZ1bmN0aW9ucy4KPiA+ IAo+ID4gVGhlIHBoeV9vcHMgYXJlIGluZGVlZCBtZWFudCB0byBzdXBwb3J0IHZlbmRvciBQSFlz LiBUaGUgLmNvbmZpZ3VyZV9waHkoKQo+ID4gb3BlcmF0aW9uIGlzIG1lYW50IHRvIHN1cHBvcnQg U3lub3BzeXMgUEhZcyB0aGF0IGRvbid0IGNvbXBseSB3aXRoIHRoZQo+ID4gSERNSSBUWCBNSEwg YW5kIDNEIFBIWXMgSTJDIHJlZ2lzdGVyIGxheW91dCBhbmQgdGh1cyBuZWVkIGEgZGlmZmVyZW50 Cj4gPiBjb25maWd1cmUgZnVuY3Rpb24sIGJ1dCBjYW4gc2hhcmUgdGhlIG90aGVyIG9wZXJhdGlv bnMgd2l0aCB0aGUgSERNSSBUWAo+ID4gTUhMIGFuZCAzRCBQSFlzLiBJZGVhbGx5IEknZCBsaWtl IHRvIG1vdmUgdGhhdCBjb2RlIHRvIHRoZSBkdy1oZG1pIGNvcmUsCj4gPiBidXQgYXQgdGhlIG1v bWVudCBJIGRvbid0IGhhdmUgZW5vdWdoIGluZm9ybWF0aW9uIHRvIGRlY2lkZSB3aGV0aGVyIHRo ZQo+ID4gcmVnaXN0ZXIgbGF5b3V0IGNvcnJlc3BvbmRzIHRvIHRoZSBzdGFuZGFyZCBTeW5vcHN5 cyBIRE1JIFRYIDIuMCBQSFkgb3IKPiA+IGlmIGl0IGhhcyBiZWVuIG1vZGlmaWVkIGJ5IHRoZSB2 ZW5kb3IuIE9uY2Ugd2UnbGwgaGF2ZSBhIHNlY29uZCBTb0MgdXNpbmcKPiA+IHRoZSBzYW1lIFBI WSB3ZSBzaG91bGQgaGF2ZSBtb3JlIGluZm9ybWF0aW9uIHRvIGNvbnNvbGlkYXRlIHRoZSBjb2Rl LiBPZgo+ID4gY291cnNlLCBpZiB5b3UgY2FuIHNoYXJlIHRoZSBIRE1JIFRYIDIuMCBQSFkgZGF0 YXNoZWV0LCBJIHdvbid0IG1pbmQKPiA+IHJld29ya2luZyB0aGUgY29kZSBub3cgOy0pCj4KPiBX ZWxsLCBpZiBJIHdhbnQgdG8ga2VlcCBteSBqb2IgSSBjYW4ndCBzaGFyZSB0aGUgZGF0YXNoZWV0 LCBhbmQgSQo+IGRvIHdhbnQgdG8ga2VlcCBteSBqb2IgOikKClRoYXQncyBzbyBzZWxmaXNoIDot RAoKPiBBcyBmYXIgYXMgSSBrbm93IHRoaXMgc2hvdWxkbid0IGNoYW5nZSBtdWNoLiBJIGFscmVh ZHkgdXNlZCAoaXQKPiB3YXMgbGlrZSBhIHllYXIgYWdvKSB0aGUgZHctaGRtaSBkcml2ZXIgaW4g YSBIRE1JIFRYIDIuMCBQSFkuCgpJIHJlYWxseSB3b25kZXIgd2hhdCBleGFjdGx5IGhhcyBiZWVu IGludGVncmF0ZWQgaW4gdGhlIFJlbmVzYXMgUi1DYXIgR2VuMyAKU29DLiBUaGUgUEhZIGlzIGNl cnRhaW5seSByZXBvcnRlZCBhcyBhbiBIRE1JIFRYIDIuMCBQSFksIGJ1dCB0aGUgcmVnaXN0ZXJz IApzZWVtIGRpZmZlcmVudCBjb21wYXJlZCB0byB0aGUgMi4wIFBIWSB5b3UndmUgdGVzdGVkLgoK PiBCdXQgSSBhbSBub3QgZm9sbG93aW5nIHlvdXIgbG9naWMgaGVyZSwgc29ycnk6IFlvdSBhcmUg bWVudGlvbmluZyBhCj4gY3VzdG9tIHBoeSwgcmlnaHQ/CgpDdXN0b20gaXMgcHJvYmFibHkgYSBi YWQgbmFtZS4gRnJvbSB3aGF0IEkndmUgYmVlbiB0b2xkIGl0J3MgYSBzdGFuZGFyZCAKU3lub3Bz eXMgUEhZLCBidXQgSSBjYW4ndCBydWxlIG91dCB2ZW5kb3Itc3BlY2lmaWMgY3VzdG9taXphdGlv bnMuCgo+IElmIGl0cyBjdXN0b20gdGhlbiBpdCBzaG91bGQgaW1wbGVtZW50IGl0cyBvd24gcGh5 X29wcy4gQW5kIEkgZG9uJ3QgdGhpbmsKPiB0aGF0IHBoeSBsb2dpYyBzaG91bGQgZ28gaW50byBj b3JlLCB0aGlzIHNob3VsZCBhbGwgYmUgZXh0cmFjdGVkIGJlY2F1c2UgSQo+IHJlYWxseSBiZWxp ZXZlIGl0IHdpbGwgbWFrZSB0aGUgY29kZSBlYXNpZXIgdG8gcmVhZC4gSW1hZ2luZSB0aGlzCj4g b3JnYW5pemF0aW9uOgo+IAo+ICAgICBGb2xkZXJzL0ZpbGVzOgo+ICAgICAgICAgZHJtL2JyaWRn ZS9kdy1oZG1pL2R3LWhkbWktY29yZS5jCj4gICAgICAgICBkcm0vYnJpZGdlL2R3LWhkbWkvZHct aGRtaS1waHktc3lub3BzeXMuYwo+ICAgICAgICAgZHJtL2JyaWRnZS9kdy1oZG1pL2R3LWhkbWkt cGh5LSpyZW5lc2FzKi5jCj4gICAgICAgICBkcm0vYnJpZGdlL2R3LWhkbWkvZHctaGRtaS1waHkt c29tZXRoaW5nLmMKPiAgICAgU3RydWN0dXJlczoKPiAgICAgICAgIGR3X2hkbWlfcGRhdGEKPiAg ICAgICAgIGR3X2hkbWlfcGh5X3BkYXRhCj4gICAgICAgICBkd19oZG1pX3BoeV9vcHMKClRoYXQg bG9va3MgZ29vZCB0byBtZS4KCj4gQXMgdGhlIHBoeSBpcyBpbnRlcmFjdGVkIHVzaW5nIGNvbnRy b2xsZXIgd2Ugd291bGQgbmVlZCB0byBwYXNzCj4gc29tZSBjYWxsYmFja3MgdG8gdGhlIHBoeSwg YnV0IHVsdGltYXRlbHkgdGhlIHBoeSB3b3VsZCBiZSBhCj4gcGxhdGZvcm0gZHJpdmVyIHdoaWNo IGNvdWxkIGhhdmUgaXRzIG93biBjb21wYXRpYmxlIHN0cmluZyB0aGF0Cj4gd291bGQgYmUgYXNz b2NpYXRlZCB3aXRoIGNvbnRyb2xsZXIgKG5vdCBzdXJlIGV4YWN0bHkgYWJvdXQgdGhpcwo+IGJl Y2F1c2UgSSBvbmx5IHVzZWQgdGhpcyBpbiBub24tZHQpLgoKV2UgYWxyZWFkeSBoYXZlIGdsdWUg Y29kZSwgaGF2aW5nIHRvIHByb3ZpZGUgc2VwYXJhdGUgZ2x1ZSBhbmQgUEhZIGRyaXZlcnMgCnNl ZW1zIGEgYml0IG92ZXJraWxsIHRvIG1lLiBJZiB3ZSBjb3VsZCBnZXQgcmlkIG9mIGdsdWUgY29k ZSBieSBhZGRpbmcgYSBub2RlIApmb3IgdGhlIFBIWSBpbiBEVCB0aGF0IHdvdWxkIGJlIG5pY2Us IGJ1dCBpZiB3ZSBoYXZlIHRvIGtlZXAgdGhlIGdsdWUgdGhlbiB3ZSAKY2FuIGFzIHdlbGwgdXNl IHBsYXRmb3JtIGRhdGEgdGhlcmUuCgo+IFRoaXMgaXMganVzdCBhbiBpZGVhIHRob3VnaC4gSSBm b2xsb3dlZCB0aGlzIGxvZ2ljIGluIHRoZSBSWCBzaWRlCj4gYW5kIGl0cyB2ZXJ5IGVhc3kgdG8g YWRkIGEgbmV3IHBoeSBub3csIGl0cyBhIG1hdHRlciBvZiBjcmVhdGluZwo+IGEgbmV3IGZpbGUs IGltcGxlbWVudCBvcHMgYW5kIGFzc29jaWF0ZSBpdCB3aXRoIGNvbnRyb2xsZXIuIE9mCj4gY291 cnNlIHNvbWUgcGh5cyB3aWxsIGJlIHZlcnkgc2ltaWxhciwgZm9yIHRoYXQgd2UgY2FuIHVzZSBt aW5vcgo+IHR3ZWFrcyB2aWEgaWQgZGV0ZWN0aW9uLgo+IAo+IFdoYXQgZG8geW91IHRoaW5rPwoK SXQgc291bmRzIG5lYXQgb3ZlcmFsbCwgYnV0IEkgd29uZGVyIHdoZXRoZXIgaXQgc2hvdWxkIHJl YWxseSBibG9jayB0aGlzIApzZXJpZXMsIG9yIGJlIGFkZGVkIG9uIHRvcCBvZiBpdCA6LSkgSSB0 aGluayB3ZSdyZSBhbHJlYWR5IG1vdmluZyBpbiB0aGUgcmlnaHQgCmRpcmVjdGlvbiBoZXJlLgoK LS0gClJlZ2FyZHMsCgpMYXVyZW50IFBpbmNoYXJ0CgpfX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBs aXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1h bi9saXN0aW5mby9kcmktZGV2ZWwK