From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Sat, 22 Sep 2018 15:06:02 +0300 Subject: [PATCH v2 1/8] drm/bridge: use bus flags in bridge timings In-Reply-To: References: <20180912183222.25414-1-stefan@agner.ch> <6714863.eJMplaj6yU@avalon> Message-ID: <3905999.JCVWYgpCCG@avalon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Stefan, On Tuesday, 18 September 2018 21:14:22 EEST Stefan Agner wrote: > On 14.09.2018 02:23, Laurent Pinchart wrote: > > On Wednesday, 12 September 2018 21:32:15 EEST Stefan Agner wrote: > >> The DRM bus flags conveys additional information on pixel data on > >> the bus. All currently available bus flags might be of interest for > >> a bridge. In the case at hand a dumb VGA bridge needs a specific > >> data enable polarity (DRM_BUS_FLAG_DE_LOW). > >> > >> Replace the sampling_edge field with input_bus_flags and allow all > >> currently documented bus flags. > >> > >> This changes the perspective from sampling side to the driving > >> side for the currently supported flags. We assume that the sampling > >> edge is always the opposite of the driving edge (hence we need to > >> invert the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE flags). This is an > >> assumption we already make for displays. For all we know it is a > >> safe assumption for bridges too. > > > > Please don't, that will get confusing very quickly. Flag names such as > > DRM_BUS_FLAG_PIXDATA_NEGEDGE don't mention sampling or driving. There's > > only a small comment next to their definition, which will easily be > > overlooked. I'd rather update the definition to cover both sampling and > > driving, and document the input_bus_flags field to explain that all flags > > refer to sampling. > > There is history to that, and I'd really rather prefer to keep that similar > across the kernel. There is already precedence in the kernel, the display > timings (which is a consumer) defines it from the driving perspective too > (see DISPLAY_FLAGS_PIXDATA_POSEDGE). > > That is why I introduced the bus flags using the same perspective. > > If we _really_ want it from sampling side, we should create new defines > which are explicit about that, e.g.: DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE. I'm not opposed to that. > But again, since even the display flags use the driving perspective, I'd > rather prefer to have it that way also for bridges too. But look at the following diff: - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, + .input_bus_flags = DRM_BUS_FLAG_PIXDATA_NEGEDGE, The first line makes it very explicit that the data signals are sampled on the rising edge. The second line reads to me as sampling on the negative edge, as it reports input information from a bridge perspective. I'll have to read the documentation to get it right, and I'm sure I and other will overlook it from time to time. The following would be much more explicit: .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLING_POSEDGE, and we could define macros as follows: /* * Don't use those two flags directly, use the DRM_BUS_FLAG_PIXDATA_DRIVE_* * and DRM_BUS_FLAG_PIXDATA_SAMPLE_* variants to qualify the flags explicitly. * The DRM_BUS_FLAG_PIXDATA_SAMPLE_* flags are defined as the opposite of the * DRM_BUS_FLAG_PIXDATA_DRIVE_* flags to make code simpler, as signals are * usually to be sampled on the opposite edge of the driving edge. */ #define DRM_BUS_FLAG_PIXDATA_POSEDGE (1<<2) #define DRM_BUS_FLAG_PIXDATA_NEGEDGE (1<<3) /* Drive data on rising edge */ #define DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE DRM_BUS_FLAG_PIXDATA_POSEDGE /* Drive data on falling edge */ #define DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE DRM_BUS_FLAG_PIXDATA_NEGEDGE /* Sample data on rising edge */ #define DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE DRM_BUS_FLAG_PIXDATA_NEGEDGE /* Sample data on falling edge */ #define DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE DRM_BUS_FLAG_PIXDATA_POSEDGE I'll submit a patch series. > >> Signed-off-by: Stefan Agner > >> --- > >> > >> drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 +++--- > >> include/drm/drm_bridge.h | 11 +++++------ > >> 2 files changed, 8 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c > >> b/drivers/gpu/drm/bridge/dumb-vga-dac.c index 9b706789a341..d5aa0f931ef2 > >> 100644 > >> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c > >> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c > >> @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device > >> *pdev) > >> */ > >> static const struct drm_bridge_timings default_dac_timings = { > >> /* Timing specifications, datasheet page 7 */ > >> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, > >> + .input_bus_flags = DRM_BUS_FLAG_PIXDATA_NEGEDGE, > >> .setup_time_ps = 500, > >> .hold_time_ps = 1500, > >> }; > >> @@ -245,7 +245,7 @@ static const struct drm_bridge_timings > >> default_dac_timings = { > >> */ > >> static const struct drm_bridge_timings ti_ths8134_dac_timings = { > >> /* From timing diagram, datasheet page 9 */ > >> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, > >> + .input_bus_flags = DRM_BUS_FLAG_PIXDATA_NEGEDGE, > >> /* From datasheet, page 12 */ > >> .setup_time_ps = 3000, > >> /* I guess this means latched input */ > >> @@ -258,7 +258,7 @@ static const struct drm_bridge_timings > >> ti_ths8134_dac_timings = { > >> */ > >> static const struct drm_bridge_timings ti_ths8135_dac_timings = { > >> /* From timing diagram, datasheet page 14 */ > >> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, > >> + .input_bus_flags = DRM_BUS_FLAG_PIXDATA_NEGEDGE, > >> /* From datasheet, page 16 */ > >> .setup_time_ps = 2000, > >> .hold_time_ps = 500, > >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > >> index bd850747ce54..45e90f4b46c3 100644 > >> --- a/include/drm/drm_bridge.h > >> +++ b/include/drm/drm_bridge.h > >> @@ -244,14 +244,13 @@ struct drm_bridge_funcs { > >> */ > >> struct drm_bridge_timings { > >> /** > >> - * @sampling_edge: > >> + * @input_bus_flags: > >> * > >> - * Tells whether the bridge samples the digital input signal > >> - * from the display engine on the positive or negative edge of the > >> - * clock, this should reuse the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE > >> - * bitwise flags from the DRM connector (bit 2 and 3 valid). > >> + * Additional settings this bridge requires for the pixel data on > >> + * the input bus (e.g. pixel signal polarity). See also > >> + * &drm_display_info->bus_flags. > >> */ > >> - u32 sampling_edge; > >> + u32 input_bus_flags; > >> > >> /** > >> * @setup_time_ps: > >> * -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v2 1/8] drm/bridge: use bus flags in bridge timings Date: Sat, 22 Sep 2018 15:06:02 +0300 Message-ID: <3905999.JCVWYgpCCG@avalon> References: <20180912183222.25414-1-stefan@agner.ch> <6714863.eJMplaj6yU@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: 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: Stefan Agner Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, max.krummenacher@toradex.com, kernel@pengutronix.de, marcel.ziswiler@toradex.com, airlied@linux.ie, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, robh+dt@kernel.org, linux-imx@nxp.com, fabio.estevam@nxp.com, sean@poorly.run, shawnguo@kernel.org, linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org SGkgU3RlZmFuLAoKT24gVHVlc2RheSwgMTggU2VwdGVtYmVyIDIwMTggMjE6MTQ6MjIgRUVTVCBT dGVmYW4gQWduZXIgd3JvdGU6Cj4gT24gMTQuMDkuMjAxOCAwMjoyMywgTGF1cmVudCBQaW5jaGFy dCB3cm90ZToKPiA+IE9uIFdlZG5lc2RheSwgMTIgU2VwdGVtYmVyIDIwMTggMjE6MzI6MTUgRUVT VCBTdGVmYW4gQWduZXIgd3JvdGU6Cj4gPj4gVGhlIERSTSBidXMgZmxhZ3MgY29udmV5cyBhZGRp dGlvbmFsIGluZm9ybWF0aW9uIG9uIHBpeGVsIGRhdGEgb24KPiA+PiB0aGUgYnVzLiBBbGwgY3Vy cmVudGx5IGF2YWlsYWJsZSBidXMgZmxhZ3MgbWlnaHQgYmUgb2YgaW50ZXJlc3QgZm9yCj4gPj4g YSBicmlkZ2UuIEluIHRoZSBjYXNlIGF0IGhhbmQgYSBkdW1iIFZHQSBicmlkZ2UgbmVlZHMgYSBz cGVjaWZpYwo+ID4+IGRhdGEgZW5hYmxlIHBvbGFyaXR5IChEUk1fQlVTX0ZMQUdfREVfTE9XKS4K PiA+PiAKPiA+PiBSZXBsYWNlIHRoZSBzYW1wbGluZ19lZGdlIGZpZWxkIHdpdGggaW5wdXRfYnVz X2ZsYWdzIGFuZCBhbGxvdyBhbGwKPiA+PiBjdXJyZW50bHkgZG9jdW1lbnRlZCBidXMgZmxhZ3Mu Cj4gPj4gCj4gPj4gVGhpcyBjaGFuZ2VzIHRoZSBwZXJzcGVjdGl2ZSBmcm9tIHNhbXBsaW5nIHNp ZGUgdG8gdGhlIGRyaXZpbmcKPiA+PiBzaWRlIGZvciB0aGUgY3VycmVudGx5IHN1cHBvcnRlZCBm bGFncy4gV2UgYXNzdW1lIHRoYXQgdGhlIHNhbXBsaW5nCj4gPj4gZWRnZSBpcyBhbHdheXMgdGhl IG9wcG9zaXRlIG9mIHRoZSBkcml2aW5nIGVkZ2UgKGhlbmNlIHdlIG5lZWQgdG8KPiA+PiBpbnZl cnQgdGhlIERSTV9CVVNfRkxBR19QSVhEQVRBX1tQT1N8TkVHXUVER0UgZmxhZ3MpLiBUaGlzIGlz IGFuCj4gPj4gYXNzdW1wdGlvbiB3ZSBhbHJlYWR5IG1ha2UgZm9yIGRpc3BsYXlzLiBGb3IgYWxs IHdlIGtub3cgaXQgaXMgYQo+ID4+IHNhZmUgYXNzdW1wdGlvbiBmb3IgYnJpZGdlcyB0b28uCj4g PiAKPiA+IFBsZWFzZSBkb24ndCwgdGhhdCB3aWxsIGdldCBjb25mdXNpbmcgdmVyeSBxdWlja2x5 LiBGbGFnIG5hbWVzIHN1Y2ggYXMKPiA+IERSTV9CVVNfRkxBR19QSVhEQVRBX05FR0VER0UgZG9u J3QgbWVudGlvbiBzYW1wbGluZyBvciBkcml2aW5nLiBUaGVyZSdzCj4gPiBvbmx5IGEgc21hbGwg Y29tbWVudCBuZXh0IHRvIHRoZWlyIGRlZmluaXRpb24sIHdoaWNoIHdpbGwgZWFzaWx5IGJlCj4g PiBvdmVybG9va2VkLiBJJ2QgcmF0aGVyIHVwZGF0ZSB0aGUgZGVmaW5pdGlvbiB0byBjb3ZlciBi b3RoIHNhbXBsaW5nIGFuZAo+ID4gZHJpdmluZywgYW5kIGRvY3VtZW50IHRoZSBpbnB1dF9idXNf ZmxhZ3MgZmllbGQgdG8gZXhwbGFpbiB0aGF0IGFsbCBmbGFncwo+ID4gcmVmZXIgdG8gc2FtcGxp bmcuCj4gCj4gVGhlcmUgaXMgaGlzdG9yeSB0byB0aGF0LCBhbmQgSSdkIHJlYWxseSByYXRoZXIg cHJlZmVyIHRvIGtlZXAgdGhhdCBzaW1pbGFyCj4gYWNyb3NzIHRoZSBrZXJuZWwuIFRoZXJlIGlz IGFscmVhZHkgcHJlY2VkZW5jZSBpbiB0aGUga2VybmVsLCB0aGUgZGlzcGxheQo+IHRpbWluZ3Mg KHdoaWNoIGlzIGEgY29uc3VtZXIpIGRlZmluZXMgaXQgZnJvbSB0aGUgZHJpdmluZyBwZXJzcGVj dGl2ZSB0b28KPiAoc2VlIERJU1BMQVlfRkxBR1NfUElYREFUQV9QT1NFREdFKS4KPiAKPiBUaGF0 IGlzIHdoeSBJIGludHJvZHVjZWQgdGhlIGJ1cyBmbGFncyB1c2luZyB0aGUgc2FtZSBwZXJzcGVj dGl2ZS4KPiAKPiBJZiB3ZSBfcmVhbGx5XyB3YW50IGl0IGZyb20gc2FtcGxpbmcgc2lkZSwgd2Ug c2hvdWxkIGNyZWF0ZSBuZXcgZGVmaW5lcwo+IHdoaWNoIGFyZSBleHBsaWNpdCBhYm91dCB0aGF0 LCBlLmcuOiBEUk1fQlVTX0ZMQUdfUElYREFUQV9TQU1QTEVfTkVHRURHRS4KCkknbSBub3Qgb3Bw b3NlZCB0byB0aGF0LgoKPiBCdXQgYWdhaW4sIHNpbmNlIGV2ZW4gdGhlIGRpc3BsYXkgZmxhZ3Mg dXNlIHRoZSBkcml2aW5nIHBlcnNwZWN0aXZlLCBJJ2QKPiByYXRoZXIgcHJlZmVyIHRvIGhhdmUg aXQgdGhhdCB3YXkgYWxzbyBmb3IgYnJpZGdlcyB0b28uCgpCdXQgbG9vayBhdCB0aGUgZm9sbG93 aW5nIGRpZmY6CgotCS5zYW1wbGluZ19lZGdlID0gRFJNX0JVU19GTEFHX1BJWERBVEFfUE9TRURH RSwKKwkuaW5wdXRfYnVzX2ZsYWdzID0gRFJNX0JVU19GTEFHX1BJWERBVEFfTkVHRURHRSwKClRo ZSBmaXJzdCBsaW5lIG1ha2VzIGl0IHZlcnkgZXhwbGljaXQgdGhhdCB0aGUgZGF0YSBzaWduYWxz IGFyZSBzYW1wbGVkIG9uIHRoZSAKcmlzaW5nIGVkZ2UuIFRoZSBzZWNvbmQgbGluZSByZWFkcyB0 byBtZSBhcyBzYW1wbGluZyBvbiB0aGUgbmVnYXRpdmUgZWRnZSwgYXMgCml0IHJlcG9ydHMgaW5w dXQgaW5mb3JtYXRpb24gZnJvbSBhIGJyaWRnZSBwZXJzcGVjdGl2ZS4gSSdsbCBoYXZlIHRvIHJl YWQgdGhlIApkb2N1bWVudGF0aW9uIHRvIGdldCBpdCByaWdodCwgYW5kIEknbSBzdXJlIEkgYW5k IG90aGVyIHdpbGwgb3Zlcmxvb2sgaXQgZnJvbSAKdGltZSB0byB0aW1lLgoKVGhlIGZvbGxvd2lu ZyB3b3VsZCBiZSBtdWNoIG1vcmUgZXhwbGljaXQ6CgoJLmlucHV0X2J1c19mbGFncyA9IERSTV9C VVNfRkxBR19QSVhEQVRBX1NBTVBMSU5HX1BPU0VER0UsCgphbmQgd2UgY291bGQgZGVmaW5lIG1h Y3JvcyBhcyBmb2xsb3dzOgoKLyoKICogRG9uJ3QgdXNlIHRob3NlIHR3byBmbGFncyBkaXJlY3Rs eSwgdXNlIHRoZSBEUk1fQlVTX0ZMQUdfUElYREFUQV9EUklWRV8qCiAqIGFuZCBEUk1fQlVTX0ZM QUdfUElYREFUQV9TQU1QTEVfKiB2YXJpYW50cyB0byBxdWFsaWZ5IHRoZSBmbGFncyBleHBsaWNp dGx5LgogKiBUaGUgRFJNX0JVU19GTEFHX1BJWERBVEFfU0FNUExFXyogZmxhZ3MgYXJlIGRlZmlu ZWQgYXMgdGhlIG9wcG9zaXRlIG9mIHRoZQogKiBEUk1fQlVTX0ZMQUdfUElYREFUQV9EUklWRV8q IGZsYWdzIHRvIG1ha2UgY29kZSBzaW1wbGVyLCBhcyBzaWduYWxzIGFyZQogKiB1c3VhbGx5IHRv IGJlIHNhbXBsZWQgb24gdGhlIG9wcG9zaXRlIGVkZ2Ugb2YgdGhlIGRyaXZpbmcgZWRnZS4KICov CiNkZWZpbmUgRFJNX0JVU19GTEFHX1BJWERBVEFfUE9TRURHRSAgICAoMTw8MikKI2RlZmluZSBE Uk1fQlVTX0ZMQUdfUElYREFUQV9ORUdFREdFICAgICgxPDwzKQoKLyogRHJpdmUgZGF0YSBvbiBy aXNpbmcgZWRnZSAqLwojZGVmaW5lIERSTV9CVVNfRkxBR19QSVhEQVRBX0RSSVZFX1BPU0VER0Ug ICAgRFJNX0JVU19GTEFHX1BJWERBVEFfUE9TRURHRQovKiBEcml2ZSBkYXRhIG9uIGZhbGxpbmcg ZWRnZSAqLwojZGVmaW5lIERSTV9CVVNfRkxBR19QSVhEQVRBX0RSSVZFX05FR0VER0UgICAgRFJN X0JVU19GTEFHX1BJWERBVEFfTkVHRURHRQovKiBTYW1wbGUgZGF0YSBvbiByaXNpbmcgZWRnZSAq LwojZGVmaW5lIERSTV9CVVNfRkxBR19QSVhEQVRBX1NBTVBMRV9QT1NFREdFICAgRFJNX0JVU19G TEFHX1BJWERBVEFfTkVHRURHRQovKiBTYW1wbGUgZGF0YSBvbiBmYWxsaW5nIGVkZ2UgKi8KI2Rl ZmluZSBEUk1fQlVTX0ZMQUdfUElYREFUQV9TQU1QTEVfTkVHRURHRSAgIERSTV9CVVNfRkxBR19Q SVhEQVRBX1BPU0VER0UKCkknbGwgc3VibWl0IGEgcGF0Y2ggc2VyaWVzLgoKPiA+PiBTaWduZWQt b2ZmLWJ5OiBTdGVmYW4gQWduZXIgPHN0ZWZhbkBhZ25lci5jaD4KPiA+PiAtLS0KPiA+PiAKPiA+ PiAgZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9kdW1iLXZnYS1kYWMuYyB8ICA2ICsrKy0tLQo+ID4+ ICBpbmNsdWRlL2RybS9kcm1fYnJpZGdlLmggICAgICAgICAgICAgIHwgMTEgKysrKystLS0tLS0K PiA+PiAgMiBmaWxlcyBjaGFuZ2VkLCA4IGluc2VydGlvbnMoKyksIDkgZGVsZXRpb25zKC0pCj4g Pj4gCj4gPj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9icmlkZ2UvZHVtYi12Z2EtZGFj LmMKPiA+PiBiL2RyaXZlcnMvZ3B1L2RybS9icmlkZ2UvZHVtYi12Z2EtZGFjLmMgaW5kZXggOWI3 MDY3ODlhMzQxLi5kNWFhMGY5MzFlZjIKPiA+PiAxMDA2NDQKPiA+PiAtLS0gYS9kcml2ZXJzL2dw dS9kcm0vYnJpZGdlL2R1bWItdmdhLWRhYy5jCj4gPj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2Jy aWRnZS9kdW1iLXZnYS1kYWMuYwo+ID4+IEBAIC0yMzQsNyArMjM0LDcgQEAgc3RhdGljIGludCBk dW1iX3ZnYV9yZW1vdmUoc3RydWN0IHBsYXRmb3JtX2RldmljZQo+ID4+ICpwZGV2KQo+ID4+ICAq Lwo+ID4+ICBzdGF0aWMgY29uc3Qgc3RydWN0IGRybV9icmlkZ2VfdGltaW5ncyBkZWZhdWx0X2Rh Y190aW1pbmdzID0gewo+ID4+ICAJLyogVGltaW5nIHNwZWNpZmljYXRpb25zLCBkYXRhc2hlZXQg cGFnZSA3ICovCj4gPj4gLQkuc2FtcGxpbmdfZWRnZSA9IERSTV9CVVNfRkxBR19QSVhEQVRBX1BP U0VER0UsCj4gPj4gKwkuaW5wdXRfYnVzX2ZsYWdzID0gRFJNX0JVU19GTEFHX1BJWERBVEFfTkVH RURHRSwKPiA+PiAgCS5zZXR1cF90aW1lX3BzID0gNTAwLAo+ID4+ICAJLmhvbGRfdGltZV9wcyA9 IDE1MDAsCj4gPj4gIH07Cj4gPj4gQEAgLTI0NSw3ICsyNDUsNyBAQCBzdGF0aWMgY29uc3Qgc3Ry dWN0IGRybV9icmlkZ2VfdGltaW5ncwo+ID4+IGRlZmF1bHRfZGFjX3RpbWluZ3MgPSB7Cj4gPj4g ICovCj4gPj4gIHN0YXRpYyBjb25zdCBzdHJ1Y3QgZHJtX2JyaWRnZV90aW1pbmdzIHRpX3Roczgx MzRfZGFjX3RpbWluZ3MgPSB7Cj4gPj4gIAkvKiBGcm9tIHRpbWluZyBkaWFncmFtLCBkYXRhc2hl ZXQgcGFnZSA5ICovCj4gPj4gLQkuc2FtcGxpbmdfZWRnZSA9IERSTV9CVVNfRkxBR19QSVhEQVRB X1BPU0VER0UsCj4gPj4gKwkuaW5wdXRfYnVzX2ZsYWdzID0gRFJNX0JVU19GTEFHX1BJWERBVEFf TkVHRURHRSwKPiA+PiAgCS8qIEZyb20gZGF0YXNoZWV0LCBwYWdlIDEyICovCj4gPj4gIAkuc2V0 dXBfdGltZV9wcyA9IDMwMDAsCj4gPj4gIAkvKiBJIGd1ZXNzIHRoaXMgbWVhbnMgbGF0Y2hlZCBp bnB1dCAqLwo+ID4+IEBAIC0yNTgsNyArMjU4LDcgQEAgc3RhdGljIGNvbnN0IHN0cnVjdCBkcm1f YnJpZGdlX3RpbWluZ3MKPiA+PiB0aV90aHM4MTM0X2RhY190aW1pbmdzID0gewo+ID4+ICAqLwo+ ID4+ICBzdGF0aWMgY29uc3Qgc3RydWN0IGRybV9icmlkZ2VfdGltaW5ncyB0aV90aHM4MTM1X2Rh Y190aW1pbmdzID0gewo+ID4+ICAJLyogRnJvbSB0aW1pbmcgZGlhZ3JhbSwgZGF0YXNoZWV0IHBh Z2UgMTQgKi8KPiA+PiAtCS5zYW1wbGluZ19lZGdlID0gRFJNX0JVU19GTEFHX1BJWERBVEFfUE9T RURHRSwKPiA+PiArCS5pbnB1dF9idXNfZmxhZ3MgPSBEUk1fQlVTX0ZMQUdfUElYREFUQV9ORUdF REdFLAo+ID4+ICAJLyogRnJvbSBkYXRhc2hlZXQsIHBhZ2UgMTYgKi8KPiA+PiAgCS5zZXR1cF90 aW1lX3BzID0gMjAwMCwKPiA+PiAgCS5ob2xkX3RpbWVfcHMgPSA1MDAsCj4gPj4gZGlmZiAtLWdp dCBhL2luY2x1ZGUvZHJtL2RybV9icmlkZ2UuaCBiL2luY2x1ZGUvZHJtL2RybV9icmlkZ2UuaAo+ ID4+IGluZGV4IGJkODUwNzQ3Y2U1NC4uNDVlOTBmNGI0NmMzIDEwMDY0NAo+ID4+IC0tLSBhL2lu Y2x1ZGUvZHJtL2RybV9icmlkZ2UuaAo+ID4+ICsrKyBiL2luY2x1ZGUvZHJtL2RybV9icmlkZ2Uu aAo+ID4+IEBAIC0yNDQsMTQgKzI0NCwxMyBAQCBzdHJ1Y3QgZHJtX2JyaWRnZV9mdW5jcyB7Cj4g Pj4gICAqLwo+ID4+ICBzdHJ1Y3QgZHJtX2JyaWRnZV90aW1pbmdzIHsKPiA+PiAgCS8qKgo+ID4+ IC0JICogQHNhbXBsaW5nX2VkZ2U6Cj4gPj4gKwkgKiBAaW5wdXRfYnVzX2ZsYWdzOgo+ID4+ICAJ ICoKPiA+PiAtCSAqIFRlbGxzIHdoZXRoZXIgdGhlIGJyaWRnZSBzYW1wbGVzIHRoZSBkaWdpdGFs IGlucHV0IHNpZ25hbAo+ID4+IC0JICogZnJvbSB0aGUgZGlzcGxheSBlbmdpbmUgb24gdGhlIHBv c2l0aXZlIG9yIG5lZ2F0aXZlIGVkZ2Ugb2YgdGhlCj4gPj4gLQkgKiBjbG9jaywgdGhpcyBzaG91 bGQgcmV1c2UgdGhlIERSTV9CVVNfRkxBR19QSVhEQVRBX1tQT1N8TkVHXUVER0UKPiA+PiAtCSAq IGJpdHdpc2UgZmxhZ3MgZnJvbSB0aGUgRFJNIGNvbm5lY3RvciAoYml0IDIgYW5kIDMgdmFsaWQp Lgo+ID4+ICsJICogQWRkaXRpb25hbCBzZXR0aW5ncyB0aGlzIGJyaWRnZSByZXF1aXJlcyBmb3Ig dGhlIHBpeGVsIGRhdGEgb24KPiA+PiArCSAqIHRoZSBpbnB1dCBidXMgKGUuZy4gcGl4ZWwgc2ln bmFsIHBvbGFyaXR5KS4gU2VlIGFsc28KPiA+PiArCSAqICZkcm1fZGlzcGxheV9pbmZvLT5idXNf ZmxhZ3MuCj4gPj4gIAkgKi8KPiA+PiAtCXUzMiBzYW1wbGluZ19lZGdlOwo+ID4+ICsJdTMyIGlu cHV0X2J1c19mbGFnczsKPiA+PiAKPiA+PiAgCS8qKgo+ID4+ICAJICogQHNldHVwX3RpbWVfcHM6 Cj4gPj4gIAkgKgoKLS0gClJlZ2FyZHMsCgpMYXVyZW50IFBpbmNoYXJ0CgoKCl9fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxp c3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNr dG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= 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=-6.9 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,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 641ADECE561 for ; Sat, 22 Sep 2018 12:05:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id F180021522 for ; Sat, 22 Sep 2018 12:05:54 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="eMO+QM/l" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F180021522 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 S1729183AbeIVR7N (ORCPT ); Sat, 22 Sep 2018 13:59:13 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:35078 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726022AbeIVR7N (ORCPT ); Sat, 22 Sep 2018 13:59:13 -0400 Received: from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi [IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id F00901280; Sat, 22 Sep 2018 14:05:47 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1537617949; bh=d40oEcNqf6lymkdSS32YI+UzJY6gnwSMxNFaVoWgtdI=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=eMO+QM/lSR09UA4Hc9v6PejguUWJHr5KxaWyo8UaSAkEitB6R47vzKemUUKqQKTew GJEyciJLUwP/wRrwo1MOu8OYnJdCFO6nlZe+49QKgTyThuxWRZZX0Awt7FgFFvo3W7 DurwNWCbOb6jnTm1wIZjY8+ugHreOfkUy8cp84PU= From: Laurent Pinchart To: Stefan Agner Cc: linus.walleij@linaro.org, airlied@linux.ie, robh+dt@kernel.org, mark.rutland@arm.com, shawnguo@kernel.org, s.hauer@pengutronix.de, p.zabel@pengutronix.de, kernel@pengutronix.de, fabio.estevam@nxp.com, linux-imx@nxp.com, architt@codeaurora.org, a.hajda@samsung.com, gustavo@padovan.org, maarten.lankhorst@linux.intel.com, sean@poorly.run, marcel.ziswiler@toradex.com, max.krummenacher@toradex.com, dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/8] drm/bridge: use bus flags in bridge timings Date: Sat, 22 Sep 2018 15:06:02 +0300 Message-ID: <3905999.JCVWYgpCCG@avalon> Organization: Ideas on Board Oy In-Reply-To: References: <20180912183222.25414-1-stefan@agner.ch> <6714863.eJMplaj6yU@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 Hi Stefan, On Tuesday, 18 September 2018 21:14:22 EEST Stefan Agner wrote: > On 14.09.2018 02:23, Laurent Pinchart wrote: > > On Wednesday, 12 September 2018 21:32:15 EEST Stefan Agner wrote: > >> The DRM bus flags conveys additional information on pixel data on > >> the bus. All currently available bus flags might be of interest for > >> a bridge. In the case at hand a dumb VGA bridge needs a specific > >> data enable polarity (DRM_BUS_FLAG_DE_LOW). > >> > >> Replace the sampling_edge field with input_bus_flags and allow all > >> currently documented bus flags. > >> > >> This changes the perspective from sampling side to the driving > >> side for the currently supported flags. We assume that the sampling > >> edge is always the opposite of the driving edge (hence we need to > >> invert the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE flags). This is an > >> assumption we already make for displays. For all we know it is a > >> safe assumption for bridges too. > > > > Please don't, that will get confusing very quickly. Flag names such as > > DRM_BUS_FLAG_PIXDATA_NEGEDGE don't mention sampling or driving. There's > > only a small comment next to their definition, which will easily be > > overlooked. I'd rather update the definition to cover both sampling and > > driving, and document the input_bus_flags field to explain that all flags > > refer to sampling. > > There is history to that, and I'd really rather prefer to keep that similar > across the kernel. There is already precedence in the kernel, the display > timings (which is a consumer) defines it from the driving perspective too > (see DISPLAY_FLAGS_PIXDATA_POSEDGE). > > That is why I introduced the bus flags using the same perspective. > > If we _really_ want it from sampling side, we should create new defines > which are explicit about that, e.g.: DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE. I'm not opposed to that. > But again, since even the display flags use the driving perspective, I'd > rather prefer to have it that way also for bridges too. But look at the following diff: - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, + .input_bus_flags = DRM_BUS_FLAG_PIXDATA_NEGEDGE, The first line makes it very explicit that the data signals are sampled on the rising edge. The second line reads to me as sampling on the negative edge, as it reports input information from a bridge perspective. I'll have to read the documentation to get it right, and I'm sure I and other will overlook it from time to time. The following would be much more explicit: .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLING_POSEDGE, and we could define macros as follows: /* * Don't use those two flags directly, use the DRM_BUS_FLAG_PIXDATA_DRIVE_* * and DRM_BUS_FLAG_PIXDATA_SAMPLE_* variants to qualify the flags explicitly. * The DRM_BUS_FLAG_PIXDATA_SAMPLE_* flags are defined as the opposite of the * DRM_BUS_FLAG_PIXDATA_DRIVE_* flags to make code simpler, as signals are * usually to be sampled on the opposite edge of the driving edge. */ #define DRM_BUS_FLAG_PIXDATA_POSEDGE (1<<2) #define DRM_BUS_FLAG_PIXDATA_NEGEDGE (1<<3) /* Drive data on rising edge */ #define DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE DRM_BUS_FLAG_PIXDATA_POSEDGE /* Drive data on falling edge */ #define DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE DRM_BUS_FLAG_PIXDATA_NEGEDGE /* Sample data on rising edge */ #define DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE DRM_BUS_FLAG_PIXDATA_NEGEDGE /* Sample data on falling edge */ #define DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE DRM_BUS_FLAG_PIXDATA_POSEDGE I'll submit a patch series. > >> Signed-off-by: Stefan Agner > >> --- > >> > >> drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 +++--- > >> include/drm/drm_bridge.h | 11 +++++------ > >> 2 files changed, 8 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c > >> b/drivers/gpu/drm/bridge/dumb-vga-dac.c index 9b706789a341..d5aa0f931ef2 > >> 100644 > >> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c > >> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c > >> @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device > >> *pdev) > >> */ > >> static const struct drm_bridge_timings default_dac_timings = { > >> /* Timing specifications, datasheet page 7 */ > >> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, > >> + .input_bus_flags = DRM_BUS_FLAG_PIXDATA_NEGEDGE, > >> .setup_time_ps = 500, > >> .hold_time_ps = 1500, > >> }; > >> @@ -245,7 +245,7 @@ static const struct drm_bridge_timings > >> default_dac_timings = { > >> */ > >> static const struct drm_bridge_timings ti_ths8134_dac_timings = { > >> /* From timing diagram, datasheet page 9 */ > >> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, > >> + .input_bus_flags = DRM_BUS_FLAG_PIXDATA_NEGEDGE, > >> /* From datasheet, page 12 */ > >> .setup_time_ps = 3000, > >> /* I guess this means latched input */ > >> @@ -258,7 +258,7 @@ static const struct drm_bridge_timings > >> ti_ths8134_dac_timings = { > >> */ > >> static const struct drm_bridge_timings ti_ths8135_dac_timings = { > >> /* From timing diagram, datasheet page 14 */ > >> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, > >> + .input_bus_flags = DRM_BUS_FLAG_PIXDATA_NEGEDGE, > >> /* From datasheet, page 16 */ > >> .setup_time_ps = 2000, > >> .hold_time_ps = 500, > >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > >> index bd850747ce54..45e90f4b46c3 100644 > >> --- a/include/drm/drm_bridge.h > >> +++ b/include/drm/drm_bridge.h > >> @@ -244,14 +244,13 @@ struct drm_bridge_funcs { > >> */ > >> struct drm_bridge_timings { > >> /** > >> - * @sampling_edge: > >> + * @input_bus_flags: > >> * > >> - * Tells whether the bridge samples the digital input signal > >> - * from the display engine on the positive or negative edge of the > >> - * clock, this should reuse the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE > >> - * bitwise flags from the DRM connector (bit 2 and 3 valid). > >> + * Additional settings this bridge requires for the pixel data on > >> + * the input bus (e.g. pixel signal polarity). See also > >> + * &drm_display_info->bus_flags. > >> */ > >> - u32 sampling_edge; > >> + u32 input_bus_flags; > >> > >> /** > >> * @setup_time_ps: > >> * -- Regards, Laurent Pinchart