This v2 shifted a bit more to rework bridge timing support. While my use case mainly cares about complete bus flag support, it seems to me that bus timings have ended up to be more complex than necessary. Patch 2/3 are an attempt to simplify bus timings. This also aligns bridge timings with how display timings are specified today.
@Linus Walleij, Laurent Pinchart: Since you have been involved in the initial bus timings discussion, I would like to have your Ack on at least patch 2/3... If we want to keep the setup/hold timings, the patchset should also work without those two patches.
-- Stefan
Changes in v2: - Rename bus_flags to simple_bus_flags - Reword dt-bindings for de-active - Add patch 2/3 which attempts to simplify bridge timings
Stefan Agner (8): drm/bridge: use bus flags in bridge timings drm/pl111: simplify bridge timing support drm/bridge: simplify bridge timing info drm/bridge: allow to specify data-enable polarity dt-bindings: display: add data-enable polarity property drm/imx: support handling bridge timings bus flags ARM: dts: imx6qdl-apalis: add VGA support ARM: dts: imx6qdl-apalis: add GPIO I2C node for DDC
.../bindings/display/bridge/dumb-vga-dac.txt | 2 + arch/arm/boot/dts/imx6q-apalis-eval.dts | 28 ++++++++ arch/arm/boot/dts/imx6qdl-apalis.dtsi | 72 +++++++++++++++++++ drivers/gpu/drm/bridge/dumb-vga-dac.c | 45 +++++++----- drivers/gpu/drm/imx/parallel-display.c | 3 + drivers/gpu/drm/pl111/pl111_display.c | 22 ++---- include/drm/drm_bridge.h | 25 ++----- 7 files changed, 142 insertions(+), 55 deletions(-)
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.
Signed-off-by: Stefan Agner stefan@agner.ch --- 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: *
Hi Stefan,
Thankk you for the patch.
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.
Signed-off-by: Stefan Agner stefan@agner.ch
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:
On 14.09.2018 02:23, Laurent Pinchart wrote:
Hi Stefan,
Thankk you for the patch.
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.
But again, since even the display flags use the driving perspective, I'd rather prefer to have it that way also for bridges too.
-- Stefan
Signed-off-by: Stefan Agner stefan@agner.ch
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:
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 stefan@agner.ch
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:
Simplify bridge timing support by only supporting pixel clock polarity. This aligns pixel clock polarity handling for bridges with displays.
Signed-off-by: Stefan Agner stefan@agner.ch --- drivers/gpu/drm/pl111/pl111_display.c | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c index 754f6b25f265..31eb62e4476f 100644 --- a/drivers/gpu/drm/pl111/pl111_display.c +++ b/drivers/gpu/drm/pl111/pl111_display.c @@ -196,23 +196,13 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe, const struct drm_bridge_timings *btimings = bridge->timings;
/* - * Here is when things get really fun. Sometimes the bridge - * timings are such that the signal out from PL11x is not - * stable before the receiving bridge (such as a dumb VGA DAC - * or similar) samples it. If that happens, we compensate by - * the only method we have: output the data on the opposite - * edge of the clock so it is for sure stable when it gets - * sampled. - * - * The PL111 manual does not contain proper timining diagrams - * or data for these details, but we know from experiments - * that the setup time is more than 3000 picoseconds (3 ns). - * If we have a bridge that requires the signal to be stable - * earlier than 3000 ps before the clock pulse, we have to - * output the data on the opposite edge to avoid flicker. + * Use LCD Timing 2 Register Invert Pixel Clock (IPC) bit + * to make sure to drive data on falling edge if requested + * by bridge. */ - if (btimings && btimings->setup_time_ps >= 3000) - tim2 ^= TIM2_IPC; + if (btimings && btimings->input_bus_flags & + DRM_BUS_FLAG_PIXDATA_NEGEDGE) + tim2 |= TIM2_IPC; }
tim2 |= cpl << 16;
Bridges are typically connected to a parallel display signal with pixel clock, sync signals and data lines. Parallel display signals are also used in lower-end embedded display panels. For parallel display panels we currently do not specify setup/hold times. From discussions on the mailing list it seems not convincing that this is currently really required for bridges either.
Remove setup/hold timings again to better align timing information of displays and briges.
Signed-off-by: Stefan Agner stefan@agner.ch --- drivers/gpu/drm/bridge/dumb-vga-dac.c | 17 +++++------------ include/drm/drm_bridge.h | 14 -------------- 2 files changed, 5 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c index d5aa0f931ef2..b2309ad228cf 100644 --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c @@ -229,14 +229,14 @@ static int dumb_vga_remove(struct platform_device *pdev) /* * We assume the ADV7123 DAC is the "default" for historical reasons * Information taken from the ADV7123 datasheet, revision D. - * NOTE: the ADV7123EP seems to have other timings and need a new timings - * set if used. */ static const struct drm_bridge_timings default_dac_timings = { - /* Timing specifications, datasheet page 7 */ + /* + * From Timing diagram, datasheet page 7. The bridge samples + * on pixel clocks positive edge, hence the display controller + * should drive signals on the negative edge. + */ .input_bus_flags = DRM_BUS_FLAG_PIXDATA_NEGEDGE, - .setup_time_ps = 500, - .hold_time_ps = 1500, };
/* @@ -246,10 +246,6 @@ 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 */ .input_bus_flags = DRM_BUS_FLAG_PIXDATA_NEGEDGE, - /* From datasheet, page 12 */ - .setup_time_ps = 3000, - /* I guess this means latched input */ - .hold_time_ps = 0, };
/* @@ -259,9 +255,6 @@ 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 */ .input_bus_flags = DRM_BUS_FLAG_PIXDATA_NEGEDGE, - /* From datasheet, page 16 */ - .setup_time_ps = 2000, - .hold_time_ps = 500, };
static const struct of_device_id dumb_vga_match[] = { diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 45e90f4b46c3..1a1d08350eaf 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -251,20 +251,6 @@ struct drm_bridge_timings { * &drm_display_info->bus_flags. */ u32 input_bus_flags; - /** - * @setup_time_ps: - * - * Defines the time in picoseconds the input data lines must be - * stable before the clock edge. - */ - u32 setup_time_ps; - /** - * @hold_time_ps: - * - * Defines the time in picoseconds taken for the bridge to sample the - * input signal after the clock edge. - */ - u32 hold_time_ps; };
/**
Hi Stefan,
On Wednesday, 12 September 2018 21:32:17 EEST Stefan Agner wrote:
Bridges are typically connected to a parallel display signal with pixel clock, sync signals and data lines. Parallel display signals are also used in lower-end embedded display panels. For parallel display panels we currently do not specify setup/hold times. From discussions on the mailing list it seems not convincing that this is currently really required for bridges either.
Remove setup/hold timings again to better align timing information of displays and briges.
The setup and hold timings were the result of a long discussion with Linux Walleij, who was confronted with a system that didn't work properly unless he flipped the clock polarity. His initial patch contradicted the bridge datasheet, and after investigating we found out that timings played a major role.
In a nutshell, given the setup and hold time requirements, the polarity on the driving side depends on the pixel clock frequency. As the frequency increases, when the half clock period reaches the setup time, you can't latch on the opposite edge anymore or you will violate the setup time. The component connected to the bridge thus needs to select a driving edge based on the sampling edge of the bridge, on the bridge's setup time requirement, and on the pixel clock frequency.
Signed-off-by: Stefan Agner stefan@agner.ch
drivers/gpu/drm/bridge/dumb-vga-dac.c | 17 +++++------------ include/drm/drm_bridge.h | 14 -------------- 2 files changed, 5 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c index d5aa0f931ef2..b2309ad228cf 100644 --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c @@ -229,14 +229,14 @@ static int dumb_vga_remove(struct platform_device *pdev) /*
- We assume the ADV7123 DAC is the "default" for historical reasons
- Information taken from the ADV7123 datasheet, revision D.
- NOTE: the ADV7123EP seems to have other timings and need a new timings
*/
- set if used.
static const struct drm_bridge_timings default_dac_timings = {
- /* Timing specifications, datasheet page 7 */
- /*
* From Timing diagram, datasheet page 7. The bridge samples* on pixel clocks positive edge, hence the display controller* should drive signals on the negative edge.*/
- .setup_time_ps = 500,
- .hold_time_ps = 1500,
};
/* @@ -246,10 +246,6 @@ 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 */ .input_bus_flags = DRM_BUS_FLAG_PIXDATA_NEGEDGE,
- /* From datasheet, page 12 */
- .setup_time_ps = 3000,
- /* I guess this means latched input */
- .hold_time_ps = 0,
};
/* @@ -259,9 +255,6 @@ 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 */ .input_bus_flags = DRM_BUS_FLAG_PIXDATA_NEGEDGE,
- /* From datasheet, page 16 */
- .setup_time_ps = 2000,
- .hold_time_ps = 500,
};
static const struct of_device_id dumb_vga_match[] = { diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 45e90f4b46c3..1a1d08350eaf 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -251,20 +251,6 @@ struct drm_bridge_timings { * &drm_display_info->bus_flags. */ u32 input_bus_flags;
- /**
* @setup_time_ps:** Defines the time in picoseconds the input data lines must be* stable before the clock edge.*/- u32 setup_time_ps;
- /**
* @hold_time_ps:** Defines the time in picoseconds taken for the bridge to sample the* input signal after the clock edge.*/- u32 hold_time_ps;
};
/**
Support boards with a passive RGB to VGA bridge which require a low active data-enable polarity.
Signed-off-by: Stefan Agner stefan@agner.ch --- drivers/gpu/drm/bridge/dumb-vga-dac.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c index b2309ad228cf..de001e322ba8 100644 --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c @@ -179,6 +179,7 @@ static struct i2c_adapter *dumb_vga_retrieve_ddc(struct device *dev) static int dumb_vga_probe(struct platform_device *pdev) { struct dumb_vga *vga; + u32 de;
vga = devm_kzalloc(&pdev->dev, sizeof(*vga), GFP_KERNEL); if (!vga) @@ -194,6 +195,23 @@ static int dumb_vga_probe(struct platform_device *pdev) dev_dbg(&pdev->dev, "No vdd regulator found: %d\n", ret); }
+ vga->bridge.funcs = &dumb_vga_bridge_funcs; + vga->bridge.of_node = pdev->dev.of_node; + vga->bridge.timings = of_device_get_match_data(&pdev->dev); + + if (!vga->bridge.timings && + !of_property_read_u32(pdev->dev.of_node, "de-active", &de)) { + struct drm_bridge_timings *timings; + + timings = devm_kzalloc(&pdev->dev, sizeof(*timings), GFP_KERNEL); + if (!timings) + return -ENOMEM; + + timings->input_bus_flags = de ? DRM_BUS_FLAG_DE_HIGH : + DRM_BUS_FLAG_DE_LOW; + vga->bridge.timings = timings; + } + vga->ddc = dumb_vga_retrieve_ddc(&pdev->dev); if (IS_ERR(vga->ddc)) { if (PTR_ERR(vga->ddc) == -ENODEV) { @@ -205,10 +223,6 @@ static int dumb_vga_probe(struct platform_device *pdev) } }
- vga->bridge.funcs = &dumb_vga_bridge_funcs; - vga->bridge.of_node = pdev->dev.of_node; - vga->bridge.timings = of_device_get_match_data(&pdev->dev); - drm_bridge_add(&vga->bridge);
return 0;
Allow to specify the data-enable polarity required by a dumb VGA DAC converting parallel RGB to VGA.
Signed-off-by: Stefan Agner stefan@agner.ch --- .../devicetree/bindings/display/bridge/dumb-vga-dac.txt | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt index 164cbb15f04c..727111ade203 100644 --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt @@ -18,6 +18,8 @@ graph bindings specified in Documentation/devicetree/bindings/graph.txt.
Optional properties: - vdd-supply: Power supply for DAC +- de-active: Polarity of the data enable signal. 0 for active low, 1 for + active high, unset for system-specific defaults.
Example -------
On Wed, 12 Sep 2018 11:32:19 -0700, Stefan Agner wrote:
Allow to specify the data-enable polarity required by a dumb VGA DAC converting parallel RGB to VGA.
Signed-off-by: Stefan Agner stefan@agner.ch
.../devicetree/bindings/display/bridge/dumb-vga-dac.txt | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Rob Herring robh@kernel.org
A bridge might require specific settings for the pixel data on the bus. Copy the bus flags from the bridge timings if a bridge is in use.
Signed-off-by: Stefan Agner stefan@agner.ch --- drivers/gpu/drm/imx/parallel-display.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c index aefd04e18f93..7798a0621df7 100644 --- a/drivers/gpu/drm/imx/parallel-display.c +++ b/drivers/gpu/drm/imx/parallel-display.c @@ -239,6 +239,9 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data) if (ret && ret != -ENODEV) return ret;
+ if (imxpd->bridge && imxpd->bridge->timings) + imxpd->bus_flags = imxpd->bridge->timings->input_bus_flags; + imxpd->dev = dev;
ret = imx_pd_register(drm, imxpd);
On Wed, 2018-09-12 at 11:32 -0700, Stefan Agner wrote:
A bridge might require specific settings for the pixel data on the bus.
On which bus? The bridge has input and output.
Copy the bus flags from the bridge timings if a bridge is in use.
Signed-off-by: Stefan Agner stefan@agner.ch
drivers/gpu/drm/imx/parallel-display.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c index aefd04e18f93..7798a0621df7 100644 --- a/drivers/gpu/drm/imx/parallel-display.c +++ b/drivers/gpu/drm/imx/parallel-display.c @@ -239,6 +239,9 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data) if (ret && ret != -ENODEV) return ret;
- if (imxpd->bridge && imxpd->bridge->timings)
imxpd->bus_flags = imxpd->bridge->timings->input_bus_flags;
Oh, ok. I'd also specify input bus in the commit message.
imxpd->dev = dev;
ret = imx_pd_register(drm, imxpd);
regards Philipp
On 13.09.2018 01:38, Philipp Zabel wrote:
On Wed, 2018-09-12 at 11:32 -0700, Stefan Agner wrote:
A bridge might require specific settings for the pixel data on the bus.
On which bus? The bridge has input and output.
Copy the bus flags from the bridge timings if a bridge is in use.
Signed-off-by: Stefan Agner stefan@agner.ch
drivers/gpu/drm/imx/parallel-display.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c index aefd04e18f93..7798a0621df7 100644 --- a/drivers/gpu/drm/imx/parallel-display.c +++ b/drivers/gpu/drm/imx/parallel-display.c @@ -239,6 +239,9 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data) if (ret && ret != -ENODEV) return ret;
- if (imxpd->bridge && imxpd->bridge->timings)
imxpd->bus_flags = imxpd->bridge->timings->input_bus_flags;Oh, ok. I'd also specify input bus in the commit message.
Good point, will change in v3 to:
"A bridge might require specific settings for the pixel data on its input bus. Those are specified through optional bus timings. Copy the bridges input bus flags as to the imxpd bus flags."
-- Stefan
imxpd->dev = dev;
ret = imx_pd_register(drm, imxpd);
regards Philipp
Hi Stefan,
Thank you for the patch.
On Wednesday, 12 September 2018 21:32:20 EEST Stefan Agner wrote:
A bridge might require specific settings for the pixel data on the bus. Copy the bus flags from the bridge timings if a bridge is in use.
Signed-off-by: Stefan Agner stefan@agner.ch
drivers/gpu/drm/imx/parallel-display.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c index aefd04e18f93..7798a0621df7 100644 --- a/drivers/gpu/drm/imx/parallel-display.c +++ b/drivers/gpu/drm/imx/parallel-display.c @@ -239,6 +239,9 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data) if (ret && ret != -ENODEV) return ret;
- if (imxpd->bridge && imxpd->bridge->timings)
imxpd->bus_flags = imxpd->bridge->timings->input_bus_flags;
As explained in different replies in this mail thread (and in v1), we need something more complex than this.
How about creating a helper function that would take a sampling edge, setup and hold times, clock frequency and internal delay on the driving side, and return which edge to drive the data on ? I agree with you that the logic is complex, so we shouldn't duplicate it in multiple drivers. If you submit such a patch I'll implement support for configuring the clock edge in the R-Car DU driver and test it with the ADV7123.
imxpd->dev = dev;
ret = imx_pd_register(drm, imxpd);
The Apalis iMX6 modules use a simple DAC using resistor ladders to convert parallel RGB to VGA. Add support for this output using the dumb VGA driver.
Signed-off-by: Stefan Agner stefan@agner.ch --- arch/arm/boot/dts/imx6q-apalis-eval.dts | 28 +++++++++++++ arch/arm/boot/dts/imx6qdl-apalis.dtsi | 52 +++++++++++++++++++++++++ 2 files changed, 80 insertions(+)
diff --git a/arch/arm/boot/dts/imx6q-apalis-eval.dts b/arch/arm/boot/dts/imx6q-apalis-eval.dts index 707ac9a46115..ae07b3a105ef 100644 --- a/arch/arm/boot/dts/imx6q-apalis-eval.dts +++ b/arch/arm/boot/dts/imx6q-apalis-eval.dts @@ -140,6 +140,16 @@ regulator-max-microvolt = <3300000>; regulator-always-on; }; + + vga { + compatible = "vga-connector"; + + port { + vga_con_in: endpoint { + remote-endpoint = <&vga_bridge_out>; + }; + }; + }; };
&backlight { @@ -281,6 +291,24 @@ status = "okay"; };
+&vgabridge { + status = "okay"; + + ports { + port@1 { + reg = <1>; + + vga_bridge_out: endpoint { + remote-endpoint = <&vga_con_in>; + }; + }; + }; +}; + +&vgadisplay { + status = "okay"; +}; + &iomuxc { /* * Mux the Apalis GPIOs diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi index 05f07ea3e8c8..e8d0407e3e18 100644 --- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi +++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi @@ -138,6 +138,54 @@ spdif-out; status = "disabled"; }; + + vgabridge: bridge { + compatible = "dumb-vga-dac"; + #address-cells = <1>; + #size-cells = <0>; + de-active = <0>; + status = "disabled"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + + vga_bridge_in: endpoint { + remote-endpoint = <&vga_display_out>; + }; + }; + }; + }; + + vgadisplay: display@di0 { + compatible = "fsl,imx-parallel-display"; + #address-cells = <1>; + #size-cells = <0>; + interface-pix-fmt = "rgb565"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_ipu2_vdac>; + status = "disabled"; + + port@0 { + reg = <0>; + + vga_display_in: endpoint { + remote-endpoint = <&ipu2_di0_disp0>; + }; + }; + + port@1 { + reg = <1>; + + vga_display_out: endpoint { + remote-endpoint = <&vga_bridge_in>; + }; + }; + }; + };
&audmux { @@ -373,6 +421,10 @@ status = "disabled"; };
+&ipu2_di0_disp0 { + remote-endpoint = <&vga_display_in>; +}; + &pwm1 { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_pwm1>;
Currently, the DDC signals are controlled by the DWC HDMI I2C master to be used for HDMI (DVI on the Apalis Evaluation Board). However, the Apalis Evaluation board also allows to route the Apalis DDC I2C to the LVDS or the VGA connector. By default, the signal is routed to DVI (HDMI), and therefor the current default settings are sensible.
To ease customization and to prepare for carrier boards with other needs, add a GPIO I2C DDC node.
E.g. to reroute the Apalis DDC to the VGA connector on the Apalis Evaluation Board, the following changes can be used:
vga { ... ddc-i2c-bus = <&i2cddc>; };
&hdmi { /delete-property/ pinctrl-0; };
&i2cddc { status = "okay"; };
Signed-off-by: Stefan Agner stefan@agner.ch --- arch/arm/boot/dts/imx6qdl-apalis.dtsi | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi index e8d0407e3e18..8340391796df 100644 --- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi +++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi @@ -61,6 +61,18 @@ status = "disabled"; };
+ /* DDC_I2C: I2C2_SDA/SCL on MXM3 205/207 */ + i2cddc: i2c@0 { + compatible = "i2c-gpio"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_i2c_ddc>; + gpios = <&gpio3 16 GPIO_ACTIVE_HIGH /* sda */ + &gpio2 30 GPIO_ACTIVE_HIGH /* scl */ + >; + i2c-gpio,delay-us = <2>; /* ~100 kHz */ + status = "disabled"; + }; + reg_module_3v3: regulator-module-3v3 { compatible = "regulator-fixed"; regulator-name = "+V3.3"; @@ -688,6 +700,14 @@ >; };
+ pinctrl_i2c_ddc: gpioi2cddcgrp { + fsl,pins = < + /* DDC bitbang */ + MX6QDL_PAD_EIM_EB2__GPIO2_IO30 0x1b0b0 + MX6QDL_PAD_EIM_D16__GPIO3_IO16 0x1b0b0 + >; + }; + pinctrl_i2c1: i2c1grp { fsl,pins = < MX6QDL_PAD_CSI0_DAT8__I2C1_SDA 0x4001b8b1
On Wed, Sep 12, 2018 at 8:32 PM Stefan Agner stefan@agner.ch wrote:
@Linus Walleij, Laurent Pinchart: Since you have been involved in the initial bus timings discussion, I would like to have your Ack on at least patch 2/3... If we want to keep the setup/hold timings, the patchset should also work without those two patches.
AFAICT this ends up with the PL111 code identical to what I proposed in my first patch set to support the latching properly, so I am pretty much happy as long as my hardware does not flicker. (Rough consensus and running code.) I'm not very invested in either idea to represent these timings, so Acked-by: Linus Walleij linus.walleij@linaro.org
Yours, Linus Walleij
Hello,
On Friday, 14 September 2018 12:07:03 EEST Linus Walleij wrote:
On Wed, Sep 12, 2018 at 8:32 PM Stefan Agner stefan@agner.ch wrote:
@Linus Walleij, Laurent Pinchart: Since you have been involved in the initial bus timings discussion, I would like to have your Ack on at least patch 2/3... If we want to keep the setup/hold timings, the patchset should also work without those two patches.
AFAICT this ends up with the PL111 code identical to what I proposed in my first patch set to support the latching properly, so I am pretty much happy as long as my hardware does not flicker. (Rough consensus and running code.) I'm not very invested in either idea to represent these timings, so Acked-by: Linus Walleij linus.walleij@linaro.org
I've just replied to patch 3/8 with an explanation of why we have added this particular API, and why we need to keep it.
dri-devel@lists.freedesktop.org
-
Laurent Pinchart -
Linus Walleij -
Philipp Zabel -
Rob Herring -
Stefan Agner