From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Fri, 14 Sep 2018 12:57:45 +0300 Subject: [PATCH 1/6] drm/bridge: use bus flags in bridge timings In-Reply-To: <2015108.bT6A7b8iLH@avalon> References: <20180905052113.21262-1-stefan@agner.ch> <2015108.bT6A7b8iLH@avalon> Message-ID: <4320623.81Q3F1DtM1@avalon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Friday, 14 September 2018 12:49:40 EEST Laurent Pinchart wrote: > Hi Stefan, > > On Thursday, 6 September 2018 23:25:56 EEST Stefan Agner wrote: > > On 06.09.2018 04:07, Linus Walleij wrote: > > > On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner wrote: > > >> On 05.09.2018 00:44, Laurent Pinchart wrote: > > >> > > >> Good point! I actually really don't like that we use the same flags > > >> here > > >> but from a different perspective. Especially since the flags defines > > >> document things differently: > > >> > > >> /* drive data on pos. edge */ > > >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE (1<<2) > > >> /* drive data on neg. edge */ > > >> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE (1<<3) > > > > > > Maybe a stupid comment from my side, but can't we just change the > > > documentation to match the usecases? > > > > > > /* Trigger pixel data latch on positive edge */ > > > #define DRM_BUS_FLAG_PIXDATA_POSEDGE (1<<2) > > > > > >> Using the opposite perspective would also need translation in crtc > > >> drivers... So far no driver uses sampling_edge. > > >> > > >> I would prefer if we always use the meaning as documented by the flags. > > >> > > >> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE -> > > >> DRM_BUS_FLAG_PIXDATA_NEGEDGE. > > >> > > >> Linus Walleij, you added sampling edge, any thoughts? > > > > > > I just thought it was generally useful to have triggering edge encoded > > > into the bridge as it makes it clear that this edge is something > > > that is a delayed version of the driving edge which is subject to > > > clock skew caused by the speed of electrons in silicon and > > > copper and slew rate caused by parasitic capacitance. > > > > Ok, I read a bit up on the history of bridge timing, especially: > > https://www.spinics.net/lists/dri-devel/msg155618.html > > > > IMHO, this got overengineered. For displays we do not need all that > > setup/sample delay timing information, and much longer cables are in > > use. So why is all that needed for bridges? > > > > For Linus case, the THS8134(A/B) data sheet I found (revised March 2010) > > clearly states: > > Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7. > > > > So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE > > should be used, which makes the pl111 driver setting TIM2_IPC: > > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index > > .h tml > > > > > DRM_BUS_FLAG_PIXDATA_POSEDGE is the right value for my use cases, but it > > > doesn't match how the ADV7123 operates. Using > > > DRM_BUS_FLAG_PIXDATA_NEGEDGE > > > would match the hardware, but would break display for some modes, > > > depending on the display clock frequency as the internal 8.5ns output > > > delay applied to a falling clock edge would fall right into the 1.7ns > > > setup + hold time window of the ADV7123 around the rising edge. I can't > > > test this right now as I don't have local access to boards using the > > > ADV7123, but from a quick calculation that ignores the PCB transmission > > > delay modes with frequencies between 57MHz and 71MHz could break if the > > > data was output on the falling edge of the clock. > > > > If clocks vs. data signal are really that much off on R-Car DU, then > > parallel displays must have the very same issue... > > > > Are you sure that only the clock signal has an output delay? And that > > this output delay is a fixed value, clock independent? > > > > Typically, delays apply to all signals equally, and often are clock > > frequency dependent... > > > > Without really looking at the signals, I would not jump to conclusions > > here! I am pretty sure that driving on negative edge works just as well. > > I've tested Linus' original patch and it broke display on R-Car, so, no, it > doesn't work :-) > > The R-Car display engine delays the clock internally (in some cases that > delay is even configurable, and that's not uncommon in display > controllers). We really need all this information, and I believe we need it > for panels too, not just for bridges. The fact that we managed to get away > without adding it to panels is likely due to the large number of panels out > there, which makes it less likely that the same panel gets used by > different systems in mainline with different clock delays. I expect that > some panel drivers report the wrong clock edge to make things work on the > board they were tested with, and I expect we'll eventually need to add the > same information for panels too. > > So please don't remove this useful API, otherwise you'll break my board, and > I won't be happy. Or, to be precise, the board won't break now, but as soon as I need to implement support for configuring the output clock edge, it will break as the ADV7123 driver won't give me the right information to make the right choice. -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings Date: Fri, 14 Sep 2018 12:57:45 +0300 Message-ID: <4320623.81Q3F1DtM1@avalon> References: <20180905052113.21262-1-stefan@agner.ch> <2015108.bT6A7b8iLH@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <2015108.bT6A7b8iLH@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Stefan Agner Cc: Mark Rutland , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , max.krummenacher@toradex.com, Sascha Hauer , Marcel Ziswiler , Dave Airlie , linux-kernel@vger.kernel.org, "open list:DRM PANEL DRIVERS" , Rob Herring , NXP Linux Team , Fabio Estevam , sean@poorly.run, Shawn Guo , Linux ARM List-Id: devicetree@vger.kernel.org T24gRnJpZGF5LCAxNCBTZXB0ZW1iZXIgMjAxOCAxMjo0OTo0MCBFRVNUIExhdXJlbnQgUGluY2hh cnQgd3JvdGU6Cj4gSGkgU3RlZmFuLAo+IAo+IE9uIFRodXJzZGF5LCA2IFNlcHRlbWJlciAyMDE4 IDIzOjI1OjU2IEVFU1QgU3RlZmFuIEFnbmVyIHdyb3RlOgo+ID4gT24gMDYuMDkuMjAxOCAwNDow NywgTGludXMgV2FsbGVpaiB3cm90ZToKPiA+ID4gT24gV2VkLCBTZXAgNSwgMjAxOCBhdCA4OjMy IFBNIFN0ZWZhbiBBZ25lciA8c3RlZmFuQGFnbmVyLmNoPiB3cm90ZToKPiA+ID4+IE9uIDA1LjA5 LjIwMTggMDA6NDQsIExhdXJlbnQgUGluY2hhcnQgd3JvdGU6Cj4gPiA+PiAKPiA+ID4+IEdvb2Qg cG9pbnQhIEkgYWN0dWFsbHkgcmVhbGx5IGRvbid0IGxpa2UgdGhhdCB3ZSB1c2UgdGhlIHNhbWUg ZmxhZ3MKPiA+ID4+IGhlcmUKPiA+ID4+IGJ1dCBmcm9tIGEgZGlmZmVyZW50IHBlcnNwZWN0aXZl LiBFc3BlY2lhbGx5IHNpbmNlIHRoZSBmbGFncyBkZWZpbmVzCj4gPiA+PiBkb2N1bWVudCB0aGlu Z3MgZGlmZmVyZW50bHk6Cj4gPiA+PiAKPiA+ID4+IC8qIGRyaXZlIGRhdGEgb24gcG9zLiBlZGdl ICovCj4gPiA+PiAjZGVmaW5lIERSTV9CVVNfRkxBR19QSVhEQVRBX1BPU0VER0UgICAgKDE8PDIp Cj4gPiA+PiAvKiBkcml2ZSBkYXRhIG9uIG5lZy4gZWRnZSAqLwo+ID4gPj4gI2RlZmluZSBEUk1f QlVTX0ZMQUdfUElYREFUQV9ORUdFREdFICAgICgxPDwzKQo+ID4gPiAKPiA+ID4gTWF5YmUgYSBz dHVwaWQgY29tbWVudCBmcm9tIG15IHNpZGUsIGJ1dCBjYW4ndCB3ZSBqdXN0IGNoYW5nZSB0aGUK PiA+ID4gZG9jdW1lbnRhdGlvbiB0byBtYXRjaCB0aGUgdXNlY2FzZXM/Cj4gPiA+IAo+ID4gPiAv KiBUcmlnZ2VyIHBpeGVsIGRhdGEgbGF0Y2ggb24gcG9zaXRpdmUgZWRnZSAqLwo+ID4gPiAjZGVm aW5lIERSTV9CVVNfRkxBR19QSVhEQVRBX1BPU0VER0UgICAgKDE8PDIpCj4gPiA+IAo+ID4gPj4g VXNpbmcgdGhlIG9wcG9zaXRlIHBlcnNwZWN0aXZlIHdvdWxkIGFsc28gbmVlZCB0cmFuc2xhdGlv biBpbiBjcnRjCj4gPiA+PiBkcml2ZXJzLi4uIFNvIGZhciBubyBkcml2ZXIgdXNlcyBzYW1wbGlu Z19lZGdlLgo+ID4gPj4gCj4gPiA+PiBJIHdvdWxkIHByZWZlciBpZiB3ZSBhbHdheXMgdXNlIHRo ZSBtZWFuaW5nIGFzIGRvY3VtZW50ZWQgYnkgdGhlIGZsYWdzLgo+ID4gPj4gCj4gPiA+PiBJIGd1 ZXNzIHdlIHdvdWxkIG5lZWQgdG8gY29udmVydCBEUk1fQlVTX0ZMQUdfUElYREFUQV9QT1NFREdF IC0+Cj4gPiA+PiBEUk1fQlVTX0ZMQUdfUElYREFUQV9ORUdFREdFLgo+ID4gPj4gCj4gPiA+PiBM aW51cyBXYWxsZWlqLCB5b3UgYWRkZWQgc2FtcGxpbmcgZWRnZSwgYW55IHRob3VnaHRzPwo+ID4g PiAKPiA+ID4gSSBqdXN0IHRob3VnaHQgaXQgd2FzIGdlbmVyYWxseSB1c2VmdWwgdG8gaGF2ZSB0 cmlnZ2VyaW5nIGVkZ2UgZW5jb2RlZAo+ID4gPiBpbnRvIHRoZSBicmlkZ2UgYXMgaXQgbWFrZXMg aXQgY2xlYXIgdGhhdCB0aGlzIGVkZ2UgaXMgc29tZXRoaW5nCj4gPiA+IHRoYXQgaXMgYSBkZWxh eWVkIHZlcnNpb24gb2YgdGhlIGRyaXZpbmcgZWRnZSB3aGljaCBpcyBzdWJqZWN0IHRvCj4gPiA+ IGNsb2NrIHNrZXcgY2F1c2VkIGJ5IHRoZSBzcGVlZCBvZiBlbGVjdHJvbnMgaW4gc2lsaWNvbiBh bmQKPiA+ID4gY29wcGVyIGFuZCBzbGV3IHJhdGUgY2F1c2VkIGJ5IHBhcmFzaXRpYyBjYXBhY2l0 YW5jZS4KPiA+IAo+ID4gT2ssIEkgcmVhZCBhIGJpdCB1cCBvbiB0aGUgaGlzdG9yeSBvZiBicmlk Z2UgdGltaW5nLCBlc3BlY2lhbGx5Ogo+ID4gaHR0cHM6Ly93d3cuc3Bpbmljcy5uZXQvbGlzdHMv ZHJpLWRldmVsL21zZzE1NTYxOC5odG1sCj4gPiAKPiA+IElNSE8sIHRoaXMgZ290IG92ZXJlbmdp bmVlcmVkLiBGb3IgZGlzcGxheXMgd2UgZG8gbm90IG5lZWQgYWxsIHRoYXQKPiA+IHNldHVwL3Nh bXBsZSBkZWxheSB0aW1pbmcgaW5mb3JtYXRpb24sIGFuZCBtdWNoIGxvbmdlciBjYWJsZXMgYXJl IGluCj4gPiB1c2UuIFNvIHdoeSBpcyBhbGwgdGhhdCBuZWVkZWQgZm9yIGJyaWRnZXM/Cj4gPiAK PiA+IEZvciBMaW51cyBjYXNlLCB0aGUgVEhTODEzNChBL0IpIGRhdGEgc2hlZXQgSSBmb3VuZCAo cmV2aXNlZCBNYXJjaCAyMDEwKQo+ID4gY2xlYXJseSBzdGF0ZXM6Cj4gPiBDbG9jayBpbnB1dC4g QSByaXNpbmcgZWRnZSBvbiBDTEsgbGF0Y2hlcyBSUHIwLTcsIEdZMC03LCBCUGIwLTcuCj4gPiAK PiA+IFNvIHdlIG5lZWQgdG8gZHJpdmUgb24gbmVnYXRpdmUgZWRnZSwgaGVuY2UgRFJNX0JVU19G TEFHX1BJWERBVEFfTkVHRURHRQo+ID4gc2hvdWxkIGJlIHVzZWQsIHdoaWNoIG1ha2VzIHRoZSBw bDExMSBkcml2ZXIgc2V0dGluZyBUSU0yX0lQQzoKPiA+IGh0dHA6Ly9pbmZvY2VudGVyLmFybS5j b20vaGVscC9pbmRleC5qc3A/dG9waWM9L2NvbS5hcm0uZG9jLmRkaTAxMjFkL2luZGV4Cj4gPiAu aCB0bWwKPiA+IAo+ID4gPiBEUk1fQlVTX0ZMQUdfUElYREFUQV9QT1NFREdFIGlzIHRoZSByaWdo dCB2YWx1ZSBmb3IgbXkgdXNlIGNhc2VzLCBidXQgaXQKPiA+ID4gZG9lc24ndCBtYXRjaCBob3cg dGhlIEFEVjcxMjMgb3BlcmF0ZXMuIFVzaW5nCj4gPiA+IERSTV9CVVNfRkxBR19QSVhEQVRBX05F R0VER0UKPiA+ID4gd291bGQgbWF0Y2ggdGhlIGhhcmR3YXJlLCBidXQgd291bGQgYnJlYWsgZGlz cGxheSBmb3Igc29tZSBtb2RlcywKPiA+ID4gZGVwZW5kaW5nIG9uIHRoZSBkaXNwbGF5IGNsb2Nr IGZyZXF1ZW5jeSBhcyB0aGUgaW50ZXJuYWwgOC41bnMgb3V0cHV0Cj4gPiA+IGRlbGF5IGFwcGxp ZWQgdG8gYSBmYWxsaW5nIGNsb2NrIGVkZ2Ugd291bGQgZmFsbCByaWdodCBpbnRvIHRoZSAxLjdu cwo+ID4gPiBzZXR1cCArIGhvbGQgdGltZSB3aW5kb3cgb2YgdGhlIEFEVjcxMjMgYXJvdW5kIHRo ZSByaXNpbmcgZWRnZS4gSSBjYW4ndAo+ID4gPiB0ZXN0IHRoaXMgcmlnaHQgbm93IGFzIEkgZG9u J3QgaGF2ZSBsb2NhbCBhY2Nlc3MgdG8gYm9hcmRzIHVzaW5nIHRoZQo+ID4gPiBBRFY3MTIzLCBi dXQgZnJvbSBhIHF1aWNrIGNhbGN1bGF0aW9uIHRoYXQgaWdub3JlcyB0aGUgUENCIHRyYW5zbWlz c2lvbgo+ID4gPiBkZWxheSBtb2RlcyB3aXRoIGZyZXF1ZW5jaWVzIGJldHdlZW4gNTdNSHogYW5k IDcxTUh6IGNvdWxkIGJyZWFrIGlmIHRoZQo+ID4gPiBkYXRhIHdhcyBvdXRwdXQgb24gdGhlIGZh bGxpbmcgZWRnZSBvZiB0aGUgY2xvY2suCj4gPiAKPiA+IElmIGNsb2NrcyB2cy4gZGF0YSBzaWdu YWwgYXJlIHJlYWxseSB0aGF0IG11Y2ggb2ZmIG9uIFItQ2FyIERVLCB0aGVuCj4gPiBwYXJhbGxl bCBkaXNwbGF5cyBtdXN0IGhhdmUgdGhlIHZlcnkgc2FtZSBpc3N1ZS4uLgo+ID4gCj4gPiBBcmUg eW91IHN1cmUgdGhhdCBvbmx5IHRoZSBjbG9jayBzaWduYWwgaGFzIGFuIG91dHB1dCBkZWxheT8g QW5kIHRoYXQKPiA+IHRoaXMgb3V0cHV0IGRlbGF5IGlzIGEgZml4ZWQgdmFsdWUsIGNsb2NrIGlu ZGVwZW5kZW50Pwo+ID4gCj4gPiBUeXBpY2FsbHksIGRlbGF5cyBhcHBseSB0byBhbGwgc2lnbmFs cyBlcXVhbGx5LCBhbmQgb2Z0ZW4gYXJlIGNsb2NrCj4gPiBmcmVxdWVuY3kgZGVwZW5kZW50Li4u Cj4gPiAKPiA+IFdpdGhvdXQgcmVhbGx5IGxvb2tpbmcgYXQgdGhlIHNpZ25hbHMsIEkgd291bGQg bm90IGp1bXAgdG8gY29uY2x1c2lvbnMKPiA+IGhlcmUhIEkgYW0gcHJldHR5IHN1cmUgdGhhdCBk cml2aW5nIG9uIG5lZ2F0aXZlIGVkZ2Ugd29ya3MganVzdCBhcyB3ZWxsLgo+IAo+IEkndmUgdGVz dGVkIExpbnVzJyBvcmlnaW5hbCBwYXRjaCBhbmQgaXQgYnJva2UgZGlzcGxheSBvbiBSLUNhciwg c28sIG5vLCBpdAo+IGRvZXNuJ3Qgd29yayA6LSkKPiAKPiBUaGUgUi1DYXIgZGlzcGxheSBlbmdp bmUgZGVsYXlzIHRoZSBjbG9jayBpbnRlcm5hbGx5IChpbiBzb21lIGNhc2VzIHRoYXQKPiBkZWxh eSBpcyBldmVuIGNvbmZpZ3VyYWJsZSwgYW5kIHRoYXQncyBub3QgdW5jb21tb24gaW4gZGlzcGxh eQo+IGNvbnRyb2xsZXJzKS4gV2UgcmVhbGx5IG5lZWQgYWxsIHRoaXMgaW5mb3JtYXRpb24sIGFu ZCBJIGJlbGlldmUgd2UgbmVlZCBpdAo+IGZvciBwYW5lbHMgdG9vLCBub3QganVzdCBmb3IgYnJp ZGdlcy4gVGhlIGZhY3QgdGhhdCB3ZSBtYW5hZ2VkIHRvIGdldCBhd2F5Cj4gd2l0aG91dCBhZGRp bmcgaXQgdG8gcGFuZWxzIGlzIGxpa2VseSBkdWUgdG8gdGhlIGxhcmdlIG51bWJlciBvZiBwYW5l bHMgb3V0Cj4gdGhlcmUsIHdoaWNoIG1ha2VzIGl0IGxlc3MgbGlrZWx5IHRoYXQgdGhlIHNhbWUg cGFuZWwgZ2V0cyB1c2VkIGJ5Cj4gZGlmZmVyZW50IHN5c3RlbXMgaW4gbWFpbmxpbmUgd2l0aCBk aWZmZXJlbnQgY2xvY2sgZGVsYXlzLiBJIGV4cGVjdCB0aGF0Cj4gc29tZSBwYW5lbCBkcml2ZXJz IHJlcG9ydCB0aGUgd3JvbmcgY2xvY2sgZWRnZSB0byBtYWtlIHRoaW5ncyB3b3JrIG9uIHRoZQo+ IGJvYXJkIHRoZXkgd2VyZSB0ZXN0ZWQgd2l0aCwgYW5kIEkgZXhwZWN0IHdlJ2xsIGV2ZW50dWFs bHkgbmVlZCB0byBhZGQgdGhlCj4gc2FtZSBpbmZvcm1hdGlvbiBmb3IgcGFuZWxzIHRvby4KPiAK PiBTbyBwbGVhc2UgZG9uJ3QgcmVtb3ZlIHRoaXMgdXNlZnVsIEFQSSwgb3RoZXJ3aXNlIHlvdSds bCBicmVhayBteSBib2FyZCwgYW5kCj4gSSB3b24ndCBiZSBoYXBweS4KCk9yLCB0byBiZSBwcmVj aXNlLCB0aGUgYm9hcmQgd29uJ3QgYnJlYWsgbm93LCBidXQgYXMgc29vbiBhcyBJIG5lZWQgdG8g CmltcGxlbWVudCBzdXBwb3J0IGZvciBjb25maWd1cmluZyB0aGUgb3V0cHV0IGNsb2NrIGVkZ2Us IGl0IHdpbGwgYnJlYWsgYXMgdGhlIApBRFY3MTIzIGRyaXZlciB3b24ndCBnaXZlIG1lIHRoZSBy aWdodCBpbmZvcm1hdGlvbiB0byBtYWtlIHRoZSByaWdodCBjaG9pY2UuCgotLSAKUmVnYXJkcywK CkxhdXJlbnQgUGluY2hhcnQKCgoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRl c2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8v ZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E7BD4FC6182 for ; Fri, 14 Sep 2018 09:57:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 92B7220853 for ; Fri, 14 Sep 2018 09:57:36 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="ipAnRTmK" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 92B7220853 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728132AbeINPLT (ORCPT ); Fri, 14 Sep 2018 11:11:19 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:33082 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726618AbeINPLT (ORCPT ); Fri, 14 Sep 2018 11:11:19 -0400 Received: from avalon.localnet (unknown [IPv6:2a02:a03f:44f6:3500:d929:375b:d608:66c7]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 302D0CE; Fri, 14 Sep 2018 11:57:31 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1536919051; bh=GE9SI6+56XDaRMPGUGwASxINiCDmuL1FVpWvUwqbgJ4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=ipAnRTmKXYLl0d407TzF8BxI/XjP5j2cGs3soWm33+cbaknNjXLvufggGXTcxWM7W B9d7+DN2qW2mOsLNtmuwbF3wul2XfZzwtRuiRJJWj2UvPb5Wfl8ySwKC+gn5N5xBGF TDpKS4tcw/TByYKLwoQWzRVNbPjwvZXaGFnSSIPc= From: Laurent Pinchart To: Stefan Agner Cc: Linus Walleij , Dave Airlie , Rob Herring , Mark Rutland , Shawn Guo , Sascha Hauer , Philipp Zabel , Sascha Hauer , Fabio Estevam , NXP Linux Team , Archit Taneja , Andrzej Hajda , Gustavo Padovan , Maarten Lankhorst , sean@poorly.run, Marcel Ziswiler , max.krummenacher@toradex.com, "open list:DRM PANEL DRIVERS" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Linux ARM , linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings Date: Fri, 14 Sep 2018 12:57:45 +0300 Message-ID: <4320623.81Q3F1DtM1@avalon> Organization: Ideas on Board Oy In-Reply-To: <2015108.bT6A7b8iLH@avalon> References: <20180905052113.21262-1-stefan@agner.ch> <2015108.bT6A7b8iLH@avalon> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Friday, 14 September 2018 12:49:40 EEST Laurent Pinchart wrote: > Hi Stefan, > > On Thursday, 6 September 2018 23:25:56 EEST Stefan Agner wrote: > > On 06.09.2018 04:07, Linus Walleij wrote: > > > On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner wrote: > > >> On 05.09.2018 00:44, Laurent Pinchart wrote: > > >> > > >> Good point! I actually really don't like that we use the same flags > > >> here > > >> but from a different perspective. Especially since the flags defines > > >> document things differently: > > >> > > >> /* drive data on pos. edge */ > > >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE (1<<2) > > >> /* drive data on neg. edge */ > > >> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE (1<<3) > > > > > > Maybe a stupid comment from my side, but can't we just change the > > > documentation to match the usecases? > > > > > > /* Trigger pixel data latch on positive edge */ > > > #define DRM_BUS_FLAG_PIXDATA_POSEDGE (1<<2) > > > > > >> Using the opposite perspective would also need translation in crtc > > >> drivers... So far no driver uses sampling_edge. > > >> > > >> I would prefer if we always use the meaning as documented by the flags. > > >> > > >> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE -> > > >> DRM_BUS_FLAG_PIXDATA_NEGEDGE. > > >> > > >> Linus Walleij, you added sampling edge, any thoughts? > > > > > > I just thought it was generally useful to have triggering edge encoded > > > into the bridge as it makes it clear that this edge is something > > > that is a delayed version of the driving edge which is subject to > > > clock skew caused by the speed of electrons in silicon and > > > copper and slew rate caused by parasitic capacitance. > > > > Ok, I read a bit up on the history of bridge timing, especially: > > https://www.spinics.net/lists/dri-devel/msg155618.html > > > > IMHO, this got overengineered. For displays we do not need all that > > setup/sample delay timing information, and much longer cables are in > > use. So why is all that needed for bridges? > > > > For Linus case, the THS8134(A/B) data sheet I found (revised March 2010) > > clearly states: > > Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7. > > > > So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE > > should be used, which makes the pl111 driver setting TIM2_IPC: > > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index > > .h tml > > > > > DRM_BUS_FLAG_PIXDATA_POSEDGE is the right value for my use cases, but it > > > doesn't match how the ADV7123 operates. Using > > > DRM_BUS_FLAG_PIXDATA_NEGEDGE > > > would match the hardware, but would break display for some modes, > > > depending on the display clock frequency as the internal 8.5ns output > > > delay applied to a falling clock edge would fall right into the 1.7ns > > > setup + hold time window of the ADV7123 around the rising edge. I can't > > > test this right now as I don't have local access to boards using the > > > ADV7123, but from a quick calculation that ignores the PCB transmission > > > delay modes with frequencies between 57MHz and 71MHz could break if the > > > data was output on the falling edge of the clock. > > > > If clocks vs. data signal are really that much off on R-Car DU, then > > parallel displays must have the very same issue... > > > > Are you sure that only the clock signal has an output delay? And that > > this output delay is a fixed value, clock independent? > > > > Typically, delays apply to all signals equally, and often are clock > > frequency dependent... > > > > Without really looking at the signals, I would not jump to conclusions > > here! I am pretty sure that driving on negative edge works just as well. > > I've tested Linus' original patch and it broke display on R-Car, so, no, it > doesn't work :-) > > The R-Car display engine delays the clock internally (in some cases that > delay is even configurable, and that's not uncommon in display > controllers). We really need all this information, and I believe we need it > for panels too, not just for bridges. The fact that we managed to get away > without adding it to panels is likely due to the large number of panels out > there, which makes it less likely that the same panel gets used by > different systems in mainline with different clock delays. I expect that > some panel drivers report the wrong clock edge to make things work on the > board they were tested with, and I expect we'll eventually need to add the > same information for panels too. > > So please don't remove this useful API, otherwise you'll break my board, and > I won't be happy. Or, to be precise, the board won't break now, but as soon as I need to implement support for configuring the output clock edge, it will break as the ADV7123 driver won't give me the right information to make the right choice. -- Regards, Laurent Pinchart