Hi,
This serie is about adding support for the RGB to VGA bridge found in the A13-Olinuxino and the CHIP VGA adapter.
Both these boards rely on an entirely passive bridge made out of resitor ladders that do not require any initialisation. The only thing needed is to get the timings from the screen if available (and if not, fall back on XGA standards), set up the display pipeline to output on the RGB bus with the proper timings, and you're done.
This serie also fixes a bunch of bugs uncovered when trying to increase the resolution, and hence the pixel clock, of our pipeline. It also fixes a few bugs in the DRM driver itself that went unnoticed before.
Let me know what you think, Maxime
Maxime Ripard (20): clk: fixed-factor: Pass clk rates change to the parent clk: multiplier: Prevent the multiplier from under / over flowing clk: sunxi: tcon-ch1: Do not return a negative error in get_parent clk: sunxi: display: Add per-clock flags drm/sun4i: request exact rates to our parents drm/sun4i: allow dclk to modify its parent rate drm/sun4i: rgb: Validate the clock rate drm/sun4i: rgb: panel is an error pointer drm/sun4i: defer only if we didn't find our panel drm/sun4i: remove simplefb at probe drm/sun4i: Convert to connector register helpers drm/sun4i: Add bridge support drm/bridge: Add RGB to VGA bridge support ARM: sun5i: a13: Add LCD pins ARM: sun5i: Move display blocks to A13 ARM: sun5i: a13-olinuxino: Enable VGA bridge ARM: multi_v7: Enable sun4i DRM driver ARM: multi_v7: enable VGA bridge ARM: sunxi: Enable sun4i DRM driver ARM: sunxi: Enable VGA bridge
.../bindings/display/bridge/dumb-vga.txt | 40 +++++ arch/arm/boot/dts/sun5i-a13-olinuxino.dts | 34 ++++ arch/arm/boot/dts/sun5i-a13.dtsi | 122 ++++++++++++++ arch/arm/boot/dts/sun5i-r8.dtsi | 120 +------------ arch/arm/configs/multi_v7_defconfig | 2 + arch/arm/configs/sunxi_defconfig | 3 + drivers/clk/clk-fixed-factor.c | 3 +- drivers/clk/clk-multiplier.c | 20 ++- drivers/clk/sunxi/clk-sun4i-display.c | 5 +- drivers/clk/sunxi/clk-sun4i-tcon-ch1.c | 3 - drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/dumb-vga.c | 186 +++++++++++++++++++++ drivers/gpu/drm/sun4i/sun4i_dotclock.c | 31 +++- drivers/gpu/drm/sun4i/sun4i_drv.c | 54 +++--- drivers/gpu/drm/sun4i/sun4i_rgb.c | 70 ++++++-- drivers/gpu/drm/sun4i/sun4i_tcon.c | 59 +++++-- drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + 18 files changed, 578 insertions(+), 182 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/bridge/dumb-vga.txt create mode 100644 drivers/gpu/drm/bridge/dumb-vga.c
A fixed factor clock, if it needs to change its rate, by definition do not have any choice but to modify its parent rate.
Add the CLK_SET_RATE_PARENT flag to that clock so that it can happen
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/clk/clk-fixed-factor.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c index 75cd6c792cb8..3363abd9b4ae 100644 --- a/drivers/clk/clk-fixed-factor.c +++ b/drivers/clk/clk-fixed-factor.c @@ -167,7 +167,8 @@ void __init of_fixed_factor_clk_setup(struct device_node *node) of_property_read_string(node, "clock-output-names", &clk_name); parent_name = of_clk_get_parent_name(node, 0);
- clk = clk_register_fixed_factor(NULL, clk_name, parent_name, 0, + clk = clk_register_fixed_factor(NULL, clk_name, parent_name, + CLK_SET_RATE_PARENT, mult, div); if (!IS_ERR(clk)) of_clk_add_provider(node, of_clk_src_simple_get, clk);
On Mon, May 16, 2016 at 02:47:01PM +0200, Maxime Ripard wrote:
A fixed factor clock, if it needs to change its rate, by definition do not have any choice but to modify its parent rate.
Add the CLK_SET_RATE_PARENT flag to that clock so that it can happen
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
Mike, Stephen, any comments on this one?
Thanks! Maxime
Quoting Maxime Ripard (2016-05-16 05:47:01)
A fixed factor clock, if it needs to change its rate, by definition do not have any choice but to modify its parent rate.
Logically it makes sense to always propagate the rate-change request up to the parent for a fixed-factor clock if we desire to change its rate. However, I wonder if doing this for all users of fixed-factor-clock in DT is safe? Some users may be counting on it not changing.
There are 397 instances of fixed-factor-clock in .dts[i] today, so this change worries me a bit.
Add the CLK_SET_RATE_PARENT flag to that clock so that it can happen
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
drivers/clk/clk-fixed-factor.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c index 75cd6c792cb8..3363abd9b4ae 100644 --- a/drivers/clk/clk-fixed-factor.c +++ b/drivers/clk/clk-fixed-factor.c @@ -167,7 +167,8 @@ void __init of_fixed_factor_clk_setup(struct device_node *node) of_property_read_string(node, "clock-output-names", &clk_name); parent_name = of_clk_get_parent_name(node, 0);
clk = clk_register_fixed_factor(NULL, clk_name, parent_name, 0,
clk = clk_register_fixed_factor(NULL, clk_name, parent_name,CLK_SET_RATE_PARENT,
An alternative would be to pass in the flags you want, somehow. For the clock you are trying to fix, is it inside of the SoC or external? If it is internal, and part of a larger clock controller driver, is this for the legacy style allwinner clock drivers that put everything in DT? If not, it would better to initialize it statically and shove this flag into the struct clk_init_data storage.
Regards, Mike
mult, div); if (!IS_ERR(clk)) of_clk_add_provider(node, of_clk_src_simple_get, clk);-- 2.8.2
On Fri, Jun 17, 2016 at 04:05:33PM -0700, Michael Turquette wrote:
Quoting Maxime Ripard (2016-05-16 05:47:01)
A fixed factor clock, if it needs to change its rate, by definition do not have any choice but to modify its parent rate.
Logically it makes sense to always propagate the rate-change request up to the parent for a fixed-factor clock if we desire to change its rate. However, I wonder if doing this for all users of fixed-factor-clock in DT is safe? Some users may be counting on it not changing.
There are 397 instances of fixed-factor-clock in .dts[i] today, so this change worries me a bit.
Understood.
Add the CLK_SET_RATE_PARENT flag to that clock so that it can happen
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
drivers/clk/clk-fixed-factor.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c index 75cd6c792cb8..3363abd9b4ae 100644 --- a/drivers/clk/clk-fixed-factor.c +++ b/drivers/clk/clk-fixed-factor.c @@ -167,7 +167,8 @@ void __init of_fixed_factor_clk_setup(struct device_node *node) of_property_read_string(node, "clock-output-names", &clk_name); parent_name = of_clk_get_parent_name(node, 0);
clk = clk_register_fixed_factor(NULL, clk_name, parent_name, 0,
clk = clk_register_fixed_factor(NULL, clk_name, parent_name,CLK_SET_RATE_PARENT,An alternative would be to pass in the flags you want, somehow. For the clock you are trying to fix, is it inside of the SoC or external? If it is internal, and part of a larger clock controller driver, is this for the legacy style allwinner clock drivers that put everything in DT?
It is :(
What we could do, is have an extra compatible for that clock (like "allwinner,sun4i-a10-pll3-x2" in that case), and set the flag only for that compatible.
Would that work for you?
If not, it would better to initialize it statically and shove this flag into the struct clk_init_data storage.
Hopefully, yes, that should be addressed by the new framework, but I need reviews to get it merged ;)
Thanks! Maxime
Quoting Maxime Ripard (2016-06-20 01:54:58)
On Fri, Jun 17, 2016 at 04:05:33PM -0700, Michael Turquette wrote:
Quoting Maxime Ripard (2016-05-16 05:47:01)
A fixed factor clock, if it needs to change its rate, by definition do not have any choice but to modify its parent rate.
Logically it makes sense to always propagate the rate-change request up to the parent for a fixed-factor clock if we desire to change its rate. However, I wonder if doing this for all users of fixed-factor-clock in DT is safe? Some users may be counting on it not changing.
There are 397 instances of fixed-factor-clock in .dts[i] today, so this change worries me a bit.
Understood.
Add the CLK_SET_RATE_PARENT flag to that clock so that it can happen
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
drivers/clk/clk-fixed-factor.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c index 75cd6c792cb8..3363abd9b4ae 100644 --- a/drivers/clk/clk-fixed-factor.c +++ b/drivers/clk/clk-fixed-factor.c @@ -167,7 +167,8 @@ void __init of_fixed_factor_clk_setup(struct device_node *node) of_property_read_string(node, "clock-output-names", &clk_name); parent_name = of_clk_get_parent_name(node, 0);
clk = clk_register_fixed_factor(NULL, clk_name, parent_name, 0,
clk = clk_register_fixed_factor(NULL, clk_name, parent_name,CLK_SET_RATE_PARENT,An alternative would be to pass in the flags you want, somehow. For the clock you are trying to fix, is it inside of the SoC or external? If it is internal, and part of a larger clock controller driver, is this for the legacy style allwinner clock drivers that put everything in DT?
It is :(
What we could do, is have an extra compatible for that clock (like "allwinner,sun4i-a10-pll3-x2" in that case), and set the flag only for that compatible.
Would that work for you
Yes, I think that is the best way forward.
If not, it would better to initialize it statically and shove this flag into the struct clk_init_data storage.
Hopefully, yes, that should be addressed by the new framework, but I need reviews to get it merged ;)
Will do!
Regards, Mike
Thanks! Maxime
-- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
In the current multiplier base clock implementation, if the CLK_SET_RATE_PARENT flag isn't set, the code will not make sure that the multiplier computed remains within the boundaries of our clock.
This means that if the clock we want to reach is below the parent rate, or if the multiplier is above the maximum that we can reach, we will end up with a completely bogus one that the clock cannot achieve.
Fixes: f2e0a53271a4 ("clk: Add a basic multiplier clock") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/clk/clk-multiplier.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/clk-multiplier.c b/drivers/clk/clk-multiplier.c index 9e449c7b751c..dc037c957acd 100644 --- a/drivers/clk/clk-multiplier.c +++ b/drivers/clk/clk-multiplier.c @@ -52,14 +52,28 @@ static unsigned long __bestmult(struct clk_hw *hw, unsigned long rate, unsigned long *best_parent_rate, u8 width, unsigned long flags) { + struct clk_multiplier *mult = to_clk_multiplier(hw); unsigned long orig_parent_rate = *best_parent_rate; unsigned long parent_rate, current_rate, best_rate = ~0; unsigned int i, bestmult = 0; + unsigned int maxmult = (1 << width) - 1; + + if (!(clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT)) { + bestmult = rate / orig_parent_rate; + + /* Make sure we don't end up with a 0 multiplier */ + if ((bestmult == 0) && + !(mult->flags & CLK_MULTIPLIER_ZERO_BYPASS)) + bestmult = 1;
- if (!(clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT)) - return rate / *best_parent_rate; + /* Make sure we don't overflow the multiplier */ + if (bestmult > maxmult) + bestmult = maxmult; + + return bestmult; + }
- for (i = 1; i < ((1 << width) - 1); i++) { + for (i = 1; i < maxmult; i++) { if (rate == orig_parent_rate * i) { /* * This is the best case for us if we have a
Hi Mike, Stephen,
On Mon, May 16, 2016 at 02:47:02PM +0200, Maxime Ripard wrote:
In the current multiplier base clock implementation, if the CLK_SET_RATE_PARENT flag isn't set, the code will not make sure that the multiplier computed remains within the boundaries of our clock.
This means that if the clock we want to reach is below the parent rate, or if the multiplier is above the maximum that we can reach, we will end up with a completely bogus one that the clock cannot achieve.
Fixes: f2e0a53271a4 ("clk: Add a basic multiplier clock") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
Any comments?
Thanks! Maxime
Quoting Maxime Ripard (2016-05-16 05:47:02)
In the current multiplier base clock implementation, if the CLK_SET_RATE_PARENT flag isn't set, the code will not make sure that the multiplier computed remains within the boundaries of our clock.
This means that if the clock we want to reach is below the parent rate, or if the multiplier is above the maximum that we can reach, we will end up with a completely bogus one that the clock cannot achieve.
Fixes: f2e0a53271a4 ("clk: Add a basic multiplier clock") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
Applied.
Regards, Mike
drivers/clk/clk-multiplier.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/clk-multiplier.c b/drivers/clk/clk-multiplier.c index 9e449c7b751c..dc037c957acd 100644 --- a/drivers/clk/clk-multiplier.c +++ b/drivers/clk/clk-multiplier.c @@ -52,14 +52,28 @@ static unsigned long __bestmult(struct clk_hw *hw, unsigned long rate, unsigned long *best_parent_rate, u8 width, unsigned long flags) {
struct clk_multiplier *mult = to_clk_multiplier(hw); unsigned long orig_parent_rate = *best_parent_rate; unsigned long parent_rate, current_rate, best_rate = ~0; unsigned int i, bestmult = 0;unsigned int maxmult = (1 << width) - 1;if (!(clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT)) {bestmult = rate / orig_parent_rate;/* Make sure we don't end up with a 0 multiplier */if ((bestmult == 0) &&!(mult->flags & CLK_MULTIPLIER_ZERO_BYPASS))bestmult = 1;
if (!(clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT))return rate / *best_parent_rate;
/* Make sure we don't overflow the multiplier */if (bestmult > maxmult)bestmult = maxmult;return bestmult;}
for (i = 1; i < ((1 << width) - 1); i++) {
for (i = 1; i < maxmult; i++) { if (rate == orig_parent_rate * i) { /* * This is the best case for us if we have a-- 2.8.2
On Mon, Jun 20, 2016 at 01:50:30PM -0700, Michael Turquette wrote:
Quoting Maxime Ripard (2016-05-16 05:47:02)
In the current multiplier base clock implementation, if the CLK_SET_RATE_PARENT flag isn't set, the code will not make sure that the multiplier computed remains within the boundaries of our clock.
This means that if the clock we want to reach is below the parent rate, or if the multiplier is above the maximum that we can reach, we will end up with a completely bogus one that the clock cannot achieve.
Fixes: f2e0a53271a4 ("clk: Add a basic multiplier clock") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
Applied.
Thanks, but apparently you merged it in clk-next, while it should go in clk-fixes, shouldn't it?
Maxime
get_parent is supposed to return an unsigned 8 bit integer, so returning -EINVAL is a bad idea.
Remove it.
Reported-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/clk/sunxi/clk-sun4i-tcon-ch1.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/clk/sunxi/clk-sun4i-tcon-ch1.c b/drivers/clk/sunxi/clk-sun4i-tcon-ch1.c index 98a4582de56a..248585238b1c 100644 --- a/drivers/clk/sunxi/clk-sun4i-tcon-ch1.c +++ b/drivers/clk/sunxi/clk-sun4i-tcon-ch1.c @@ -85,9 +85,6 @@ static u8 tcon_ch1_get_parent(struct clk_hw *hw) reg = readl(tclk->reg) >> TCON_CH1_SCLK2_MUX_SHIFT; reg &= reg >> TCON_CH1_SCLK2_MUX_MASK;
- if (reg >= num_parents) - return -EINVAL; - return reg; }
On Mon, May 16, 2016 at 8:47 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
get_parent is supposed to return an unsigned 8 bit integer, so returning -EINVAL is a bad idea.
Remove it.
Reported-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
Acked-by: Chen-Yu Tsai wens@csie.org
On Tue, May 17, 2016 at 12:05:18AM +0800, Chen-Yu Tsai wrote:
On Mon, May 16, 2016 at 8:47 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
get_parent is supposed to return an unsigned 8 bit integer, so returning -EINVAL is a bad idea.
Remove it.
Reported-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
Acked-by: Chen-Yu Tsai wens@csie.org
Applied,
Maxime
The TCON channel 0 clock that is the parent clock of our pixel clock is expected to change its rate depending on the resolution we want to output in our display engine.
However, since it's only a mux, the only way it can do that is by changing its parents rate.
Allow to give flags in our display clocks description, and add the CLK_SET_RATE_PARENT flag for the TCON channel 0 flag.
Fixes: a3b4956ee6d9 ("clk: sunxi: display: Add per-clock flags") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/clk/sunxi/clk-sun4i-display.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/sunxi/clk-sun4i-display.c b/drivers/clk/sunxi/clk-sun4i-display.c index 445a7498d6df..9780fac6d029 100644 --- a/drivers/clk/sunxi/clk-sun4i-display.c +++ b/drivers/clk/sunxi/clk-sun4i-display.c @@ -33,6 +33,8 @@ struct sun4i_a10_display_clk_data {
u8 width_div; u8 width_mux; + + u32 flags; };
struct reset_data { @@ -166,7 +168,7 @@ static void __init sun4i_a10_display_init(struct device_node *node, data->has_div ? &div->hw : NULL, data->has_div ? &clk_divider_ops : NULL, &gate->hw, &clk_gate_ops, - 0); + data->flags); if (IS_ERR(clk)) { pr_err("%s: Couldn't register the clock\n", clk_name); goto free_div; @@ -232,6 +234,7 @@ static const struct sun4i_a10_display_clk_data sun4i_a10_tcon_ch0_data __initcon .offset_rst = 29, .offset_mux = 24, .width_mux = 2, + .flags = CLK_SET_RATE_PARENT, };
static void __init sun4i_a10_tcon_ch0_setup(struct device_node *node)
Hi,
On Mon, May 16, 2016 at 8:47 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
The TCON channel 0 clock that is the parent clock of our pixel clock is expected to change its rate depending on the resolution we want to output in our display engine.
However, since it's only a mux, the only way it can do that is by changing its parents rate.
Allow to give flags in our display clocks description, and add the CLK_SET_RATE_PARENT flag for the TCON channel 0 flag.
Fixes: a3b4956ee6d9 ("clk: sunxi: display: Add per-clock flags") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
drivers/clk/sunxi/clk-sun4i-display.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/sunxi/clk-sun4i-display.c b/drivers/clk/sunxi/clk-sun4i-display.c index 445a7498d6df..9780fac6d029 100644 --- a/drivers/clk/sunxi/clk-sun4i-display.c +++ b/drivers/clk/sunxi/clk-sun4i-display.c @@ -33,6 +33,8 @@ struct sun4i_a10_display_clk_data {
u8 width_div; u8 width_mux;
Don't really need this separator, but I'm ok either way.
u32 flags;};
struct reset_data { @@ -166,7 +168,7 @@ static void __init sun4i_a10_display_init(struct device_node *node, data->has_div ? &div->hw : NULL, data->has_div ? &clk_divider_ops : NULL, &gate->hw, &clk_gate_ops,
0);
data->flags); if (IS_ERR(clk)) { pr_err("%s: Couldn't register the clock\n", clk_name); goto free_div;@@ -232,6 +234,7 @@ static const struct sun4i_a10_display_clk_data sun4i_a10_tcon_ch0_data __initcon .offset_rst = 29, .offset_mux = 24, .width_mux = 2,
.flags = CLK_SET_RATE_PARENT,};
static void __init sun4i_a10_tcon_ch0_setup(struct device_node *node)
2.8.2
Acked-by: Chen-Yu Tsai wens@csie.org
On Mon, May 16, 2016 at 11:21:41PM +0800, Chen-Yu Tsai wrote:
Hi,
On Mon, May 16, 2016 at 8:47 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
The TCON channel 0 clock that is the parent clock of our pixel clock is expected to change its rate depending on the resolution we want to output in our display engine.
However, since it's only a mux, the only way it can do that is by changing its parents rate.
Allow to give flags in our display clocks description, and add the CLK_SET_RATE_PARENT flag for the TCON channel 0 flag.
Fixes: a3b4956ee6d9 ("clk: sunxi: display: Add per-clock flags") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
drivers/clk/sunxi/clk-sun4i-display.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/sunxi/clk-sun4i-display.c b/drivers/clk/sunxi/clk-sun4i-display.c index 445a7498d6df..9780fac6d029 100644 --- a/drivers/clk/sunxi/clk-sun4i-display.c +++ b/drivers/clk/sunxi/clk-sun4i-display.c @@ -33,6 +33,8 @@ struct sun4i_a10_display_clk_data {
u8 width_div; u8 width_mux;
Don't really need this separator, but I'm ok either way.
u32 flags;};
struct reset_data { @@ -166,7 +168,7 @@ static void __init sun4i_a10_display_init(struct device_node *node, data->has_div ? &div->hw : NULL, data->has_div ? &clk_divider_ops : NULL, &gate->hw, &clk_gate_ops,
0);
data->flags); if (IS_ERR(clk)) { pr_err("%s: Couldn't register the clock\n", clk_name); goto free_div;@@ -232,6 +234,7 @@ static const struct sun4i_a10_display_clk_data sun4i_a10_tcon_ch0_data __initcon .offset_rst = 29, .offset_mux = 24, .width_mux = 2,
.flags = CLK_SET_RATE_PARENT,};
static void __init sun4i_a10_tcon_ch0_setup(struct device_node *node)
2.8.2
Acked-by: Chen-Yu Tsai wens@csie.org
Applied,
Maxime
Our pixel clock currently only tries to deal with the current parent rate.
While that works when the resolution is the same than the one already program, or when we can compute directly the rate from the current parent rate, it cannot work in most situation when we want to change the frequency, and we end up with an improper pixel clock rate, which obviously doesn't work as expected.
Ask our parent for all the possible dividers if it can reach that frequency, and return the best parent rate to the clock framework so that we can use it.
Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/gpu/drm/sun4i/sun4i_dotclock.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/drm/sun4i/sun4i_dotclock.c index 6c9c090a8006..1ddf6d7a7107 100644 --- a/drivers/gpu/drm/sun4i/sun4i_dotclock.c +++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c @@ -72,14 +72,40 @@ static unsigned long sun4i_dclk_recalc_rate(struct clk_hw *hw, static long sun4i_dclk_round_rate(struct clk_hw *hw, unsigned long rate, unsigned long *parent_rate) { - return *parent_rate / DIV_ROUND_CLOSEST(*parent_rate, rate); + unsigned long best_parent = 0; + u8 best_div = 1; + int i; + + for (i = 6; i < 127; i++) { + unsigned long ideal = rate * i; + unsigned long rounded; + + rounded = clk_hw_round_rate(clk_hw_get_parent(hw), + ideal); + + if (rounded == ideal) { + best_parent = rounded; + best_div = i; + goto out; + } + + if ((rounded < ideal) && (rounded > best_parent)) { + best_parent = rounded; + best_div = i; + } + } + +out: + *parent_rate = best_parent; + + return best_parent / best_div; }
static int sun4i_dclk_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate) { struct sun4i_dclk *dclk = hw_to_dclk(hw); - int div = DIV_ROUND_CLOSEST(parent_rate, rate); + u8 div = parent_rate / rate;
return regmap_update_bits(dclk->regmap, SUN4I_TCON0_DCLK_REG, GENMASK(6, 0), div);
The pixel clock being only a divider of its parent clock, depending on the resolution, it's expected to change its parent rate. Add that flag so that the clock framework knows it.
Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/gpu/drm/sun4i/sun4i_dotclock.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/drm/sun4i/sun4i_dotclock.c index 1ddf6d7a7107..5b3463197c48 100644 --- a/drivers/gpu/drm/sun4i/sun4i_dotclock.c +++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c @@ -170,6 +170,7 @@ int sun4i_dclk_create(struct device *dev, struct sun4i_tcon *tcon) init.ops = &sun4i_dclk_ops; init.parent_names = &parent_name; init.num_parents = 1; + init.flags = CLK_SET_RATE_PARENT;
dclk->regmap = tcon->regs; dclk->hw.init = &init;
Hi,
On Mon, May 16, 2016 at 8:47 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
The pixel clock being only a divider of its parent clock, depending on the resolution, it's expected to change its parent rate. Add that flag so that the clock framework knows it.
This should be squashed into the previous patch. Otherwise, the previous one doesn't really fix things, and probably makes things worse as it assumes the parent clock would be changed.
ChenYu
Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
drivers/gpu/drm/sun4i/sun4i_dotclock.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/drm/sun4i/sun4i_dotclock.c index 1ddf6d7a7107..5b3463197c48 100644 --- a/drivers/gpu/drm/sun4i/sun4i_dotclock.c +++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c @@ -170,6 +170,7 @@ int sun4i_dclk_create(struct device *dev, struct sun4i_tcon *tcon) init.ops = &sun4i_dclk_ops; init.parent_names = &parent_name; init.num_parents = 1;
init.flags = CLK_SET_RATE_PARENT; dclk->regmap = tcon->regs; dclk->hw.init = &init;-- 2.8.2
On Tue, May 17, 2016 at 01:18:44AM +0800, Chen-Yu Tsai wrote:
Hi,
On Mon, May 16, 2016 at 8:47 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
The pixel clock being only a divider of its parent clock, depending on the resolution, it's expected to change its parent rate. Add that flag so that the clock framework knows it.
This should be squashed into the previous patch. Otherwise, the previous one doesn't really fix things, and probably makes things worse as it assumes the parent clock would be changed.
Merged the two patches and applied, thanks! Maxime
Our pixel clock cannot reach a high enough rate for some rather high while common resolutions (like 1080p60).
Make sure we filter the resolutions we cannot reach in our mode_valid function.
Fixes: 29e57fab97fc ("drm: sun4i: Add RGB output") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/gpu/drm/sun4i/sun4i_rgb.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index ab6494818050..923f55039cb1 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -54,8 +54,13 @@ static int sun4i_rgb_get_modes(struct drm_connector *connector) static int sun4i_rgb_mode_valid(struct drm_connector *connector, struct drm_display_mode *mode) { + struct sun4i_rgb *rgb = drm_connector_to_sun4i_rgb(connector); + struct sun4i_drv *drv = rgb->drv; + struct sun4i_tcon *tcon = drv->tcon; u32 hsync = mode->hsync_end - mode->hsync_start; u32 vsync = mode->vsync_end - mode->vsync_start; + unsigned long rate = mode->crtc_clock * 1000; + long rounded_rate;
DRM_DEBUG_DRIVER("Validating modes...\n");
@@ -87,6 +92,15 @@ static int sun4i_rgb_mode_valid(struct drm_connector *connector,
DRM_DEBUG_DRIVER("Vertical parameters OK\n");
+ rounded_rate = clk_round_rate(tcon->dclk, rate); + if (rounded_rate < rate) + return MODE_CLOCK_LOW; + + if (rounded_rate > rate) + return MODE_CLOCK_HIGH; + + DRM_DEBUG_DRIVER("Clock rate OK\n"); + return MODE_OK; }
In case of an error, our pointer to the drm_panel structure attached to our encoder will hold an error pointer, not a NULL pointer.
Make sure we check the right thing.
Fixes: 29e57fab97fc ("drm: sun4i: Add RGB output") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/gpu/drm/sun4i/sun4i_rgb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index 923f55039cb1..b46d2c15dc95 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -217,7 +217,7 @@ int sun4i_rgb_init(struct drm_device *drm) int ret;
/* If we don't have a panel, there's no point in going on */ - if (!tcon->panel) + if (IS_ERR(tcon->panel)) return -ENODEV;
rgb = devm_kzalloc(drm->dev, sizeof(*rgb), GFP_KERNEL);
Hi,
On Mon, May 16, 2016 at 8:47 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
In case of an error, our pointer to the drm_panel structure attached to our encoder will hold an error pointer, not a NULL pointer.
Make sure we check the right thing.
Fixes: 29e57fab97fc ("drm: sun4i: Add RGB output") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
drivers/gpu/drm/sun4i/sun4i_rgb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index 923f55039cb1..b46d2c15dc95 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -217,7 +217,7 @@ int sun4i_rgb_init(struct drm_device *drm) int ret;
/* If we don't have a panel, there's no point in going on */
if (!tcon->panel)
if (IS_ERR(tcon->panel))
Without the next patch, tcon->panel could be the result of of_drm_find_panel(), which could be NULL.
Use IS_ERR_OR_NULL here, or reorder/squash the patches.
Regards ChenYu
return -ENODEV; rgb = devm_kzalloc(drm->dev, sizeof(*rgb), GFP_KERNEL);-- 2.8.2
On Tue, May 17, 2016 at 11:51:36AM +0800, Chen-Yu Tsai wrote:
Hi,
On Mon, May 16, 2016 at 8:47 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
In case of an error, our pointer to the drm_panel structure attached to our encoder will hold an error pointer, not a NULL pointer.
Make sure we check the right thing.
Fixes: 29e57fab97fc ("drm: sun4i: Add RGB output") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
drivers/gpu/drm/sun4i/sun4i_rgb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index 923f55039cb1..b46d2c15dc95 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -217,7 +217,7 @@ int sun4i_rgb_init(struct drm_device *drm) int ret;
/* If we don't have a panel, there's no point in going on */
if (!tcon->panel)
if (IS_ERR(tcon->panel))Without the next patch, tcon->panel could be the result of of_drm_find_panel(), which could be NULL.
Use IS_ERR_OR_NULL here, or reorder/squash the patches.
Switched the order, and applied both, thanks! Maxime
Our code currently defers our probe on any error, even if we were not expecting to have one at all.
Make sure we return -EPROBE_DEFER only when we were supposed to have a panel, but it's not probed yet.
Fixes: 29e57fab97fc ("drm: sun4i: Add RGB output") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/gpu/drm/sun4i/sun4i_tcon.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 9f19b0e08560..eed6a9e8d9a6 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -429,7 +429,7 @@ static struct drm_panel *sun4i_tcon_find_panel(struct device_node *node) return ERR_PTR(-EINVAL); }
- return of_drm_find_panel(remote); + return of_drm_find_panel(remote) ?: ERR_PTR(-EPROBE_DEFER); }
static int sun4i_tcon_bind(struct device *dev, struct device *master, @@ -522,12 +522,13 @@ static int sun4i_tcon_probe(struct platform_device *pdev) * Defer the probe. */ panel = sun4i_tcon_find_panel(node); - if (IS_ERR(panel)) { - /* - * If we don't have a panel endpoint, just go on - */ - if (PTR_ERR(panel) != -ENODEV) - return -EPROBE_DEFER; + + /* + * If we don't have a panel endpoint, just go on + */ + if (PTR_ERR(panel) == -EPROBE_DEFER) { + DRM_DEBUG_DRIVER("Still waiting for our panel. Deferring...\n"); + return -EPROBE_DEFER; }
return component_add(&pdev->dev, &sun4i_tcon_ops);
Hi,
On Mon, May 16, 2016 at 8:47 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Our code currently defers our probe on any error, even if we were not expecting to have one at all.
Make sure we return -EPROBE_DEFER only when we were supposed to have a panel, but it's not probed yet.
Fixes: 29e57fab97fc ("drm: sun4i: Add RGB output") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
drivers/gpu/drm/sun4i/sun4i_tcon.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 9f19b0e08560..eed6a9e8d9a6 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -429,7 +429,7 @@ static struct drm_panel *sun4i_tcon_find_panel(struct device_node *node) return ERR_PTR(-EINVAL); }
return of_drm_find_panel(remote);
return of_drm_find_panel(remote) ?: ERR_PTR(-EPROBE_DEFER);
There's also a typo in the DRM_DEBUG_DRIVER call a few lines up. Could you fix it as well?
ChenYu
}
static int sun4i_tcon_bind(struct device *dev, struct device *master, @@ -522,12 +522,13 @@ static int sun4i_tcon_probe(struct platform_device *pdev) * Defer the probe. */ panel = sun4i_tcon_find_panel(node);
if (IS_ERR(panel)) {/** If we don't have a panel endpoint, just go on*/if (PTR_ERR(panel) != -ENODEV)return -EPROBE_DEFER;
/** If we don't have a panel endpoint, just go on*/if (PTR_ERR(panel) == -EPROBE_DEFER) {DRM_DEBUG_DRIVER("Still waiting for our panel. Deferring...\n");return -EPROBE_DEFER; } return component_add(&pdev->dev, &sun4i_tcon_ops);-- 2.8.2
On Tue, May 17, 2016 at 11:52:10AM +0800, Chen-Yu Tsai wrote:
Hi,
On Mon, May 16, 2016 at 8:47 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Our code currently defers our probe on any error, even if we were not expecting to have one at all.
Make sure we return -EPROBE_DEFER only when we were supposed to have a panel, but it's not probed yet.
Fixes: 29e57fab97fc ("drm: sun4i: Add RGB output") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
drivers/gpu/drm/sun4i/sun4i_tcon.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 9f19b0e08560..eed6a9e8d9a6 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -429,7 +429,7 @@ static struct drm_panel *sun4i_tcon_find_panel(struct device_node *node) return ERR_PTR(-EINVAL); }
return of_drm_find_panel(remote);
return of_drm_find_panel(remote) ?: ERR_PTR(-EPROBE_DEFER);There's also a typo in the DRM_DEBUG_DRIVER call a few lines up. Could you fix it as well?
Done, thanks! Maxime
If simplefb was setup by our bootloader and enabled in the DT, we will have a first framebuffer loaded in our system.
However, as soon as our DRM driver will load, it will reset the controller, initialise it and, if the framebuffer emulation is enabled, register a second framebuffer device.
This is obviously pretty bad, since the first framebuffer will be some kind of a black hole, with memory still reserved that we can write to safely, but not displayed anywhere.
Make sure we remove that framebuffer when we probe so we don't end up in that situation.
Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support") Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/gpu/drm/sun4i/sun4i_drv.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c index 76e922bb60e5..af62b24c39b1 100644 --- a/drivers/gpu/drm/sun4i/sun4i_drv.c +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c @@ -125,6 +125,22 @@ static struct drm_driver sun4i_drv_driver = { .disable_vblank = sun4i_drv_disable_vblank, };
+static void sun4i_remove_framebuffers(void) +{ + struct apertures_struct *ap; + + ap = alloc_apertures(1); + if (!ap) + return; + + /* The framebuffer can be located anywhere in RAM */ + ap->ranges[0].base = 0; + ap->ranges[0].size = ~0; + + remove_conflicting_framebuffers(ap, "sun4i-drm-fb", false); + kfree(ap); +} + static int sun4i_drv_bind(struct device *dev) { struct drm_device *drm; @@ -172,6 +188,9 @@ static int sun4i_drv_bind(struct device *dev) } drm->irq_enabled = true;
+ /* Remove early framebuffers (ie. simplefb) */ + sun4i_remove_framebuffers(); + /* Create our framebuffer */ drv->fbdev = sun4i_framebuffer_init(drm); if (IS_ERR(drv->fbdev)) {
Now that connector register helpers have been created, switch to them.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/gpu/drm/sun4i/sun4i_drv.c | 31 ++----------------------------- 1 file changed, 2 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c index af62b24c39b1..257d2b4f3645 100644 --- a/drivers/gpu/drm/sun4i/sun4i_drv.c +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c @@ -24,34 +24,6 @@ #include "sun4i_layer.h" #include "sun4i_tcon.h"
-static int sun4i_drv_connector_plug_all(struct drm_device *drm) -{ - struct drm_connector *connector, *failed; - int ret; - - mutex_lock(&drm->mode_config.mutex); - list_for_each_entry(connector, &drm->mode_config.connector_list, head) { - ret = drm_connector_register(connector); - if (ret) { - failed = connector; - goto err; - } - } - mutex_unlock(&drm->mode_config.mutex); - return 0; - -err: - list_for_each_entry(connector, &drm->mode_config.connector_list, head) { - if (failed == connector) - break; - - drm_connector_unregister(connector); - } - mutex_unlock(&drm->mode_config.mutex); - - return ret; -} - static int sun4i_drv_enable_vblank(struct drm_device *drm, unsigned int pipe) { struct sun4i_drv *drv = drm->dev_private; @@ -206,7 +178,7 @@ static int sun4i_drv_bind(struct device *dev) if (ret) goto free_drm;
- ret = sun4i_drv_connector_plug_all(drm); + ret = drm_connector_register_all(drm); if (ret) goto unregister_drm;
@@ -223,6 +195,7 @@ static void sun4i_drv_unbind(struct device *dev) { struct drm_device *drm = dev_get_drvdata(dev);
+ drm_connector_unregister_all(drm); drm_dev_unregister(drm); drm_kms_helper_poll_fini(drm); sun4i_framebuffer_free(drm);
Our RGB bus can be either connected to a bridge or a panel. While the panel support was already there, the bridge was not.
Fix that.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/gpu/drm/sun4i/sun4i_drv.c | 4 +-- drivers/gpu/drm/sun4i/sun4i_rgb.c | 56 +++++++++++++++++++++++++++----------- drivers/gpu/drm/sun4i/sun4i_tcon.c | 48 ++++++++++++++++++++++++++++---- drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + 4 files changed, 86 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c index 257d2b4f3645..1f9e00db747d 100644 --- a/drivers/gpu/drm/sun4i/sun4i_drv.c +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c @@ -268,8 +268,8 @@ static int sun4i_drv_add_endpoints(struct device *dev, }
/* - * If the node is our TCON, the first port is used for our - * panel, and will not be part of the + * If the node is our TCON, the first port is used for + * panel or bridges, and will not be part of the * component framework. */ if (sun4i_drv_node_is_tcon(node)) { diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index b46d2c15dc95..c3c611b08269 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -161,7 +161,12 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
DRM_DEBUG_DRIVER("Enabling RGB output\n");
- drm_panel_enable(tcon->panel); + if (!IS_ERR(tcon->panel)) + drm_panel_enable(tcon->panel); + + if (!IS_ERR(tcon->bridge)) + drm_bridge_enable(tcon->bridge); + sun4i_tcon_channel_enable(tcon, 0); }
@@ -174,7 +179,12 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) DRM_DEBUG_DRIVER("Disabling RGB output\n");
sun4i_tcon_channel_disable(tcon, 0); - drm_panel_disable(tcon->panel); + + if (!IS_ERR(tcon->bridge)) + drm_bridge_disable(tcon->bridge); + + if (!IS_ERR(tcon->panel)) + drm_panel_disable(tcon->panel); }
static void sun4i_rgb_encoder_mode_set(struct drm_encoder *encoder, @@ -216,10 +226,6 @@ int sun4i_rgb_init(struct drm_device *drm) struct sun4i_rgb *rgb; int ret;
- /* If we don't have a panel, there's no point in going on */ - if (IS_ERR(tcon->panel)) - return -ENODEV; - rgb = devm_kzalloc(drm->dev, sizeof(*rgb), GFP_KERNEL); if (!rgb) return -ENOMEM; @@ -240,19 +246,37 @@ int sun4i_rgb_init(struct drm_device *drm) /* The RGB encoder can only work with the TCON channel 0 */ rgb->encoder.possible_crtcs = BIT(0);
- drm_connector_helper_add(&rgb->connector, - &sun4i_rgb_con_helper_funcs); - ret = drm_connector_init(drm, &rgb->connector, - &sun4i_rgb_con_funcs, - DRM_MODE_CONNECTOR_Unknown); - if (ret) { - dev_err(drm->dev, "Couldn't initialise the rgb connector\n"); - goto err_cleanup_connector; + if (!IS_ERR(tcon->panel)) { + drm_connector_helper_add(&rgb->connector, + &sun4i_rgb_con_helper_funcs); + ret = drm_connector_init(drm, &rgb->connector, + &sun4i_rgb_con_funcs, + DRM_MODE_CONNECTOR_Unknown); + if (ret) { + dev_err(drm->dev, "Couldn't initialise the rgb connector\n"); + goto err_cleanup_connector; + } + + drm_mode_connector_attach_encoder(&rgb->connector, + &rgb->encoder); + + ret = drm_panel_attach(tcon->panel, &rgb->connector); + if (ret) { + dev_err(drm->dev, "Couldn't attach our panel\n"); + goto err_cleanup_connector; + } }
- drm_mode_connector_attach_encoder(&rgb->connector, &rgb->encoder); + if (!IS_ERR(tcon->bridge)) { + rgb->encoder.bridge = tcon->bridge; + tcon->bridge->encoder = &rgb->encoder;
- drm_panel_attach(tcon->panel, &rgb->connector); + ret = drm_bridge_attach(drm, tcon->bridge); + if (ret) { + dev_err(drm->dev, "Couldn't attach our bridge\n"); + goto err_cleanup_connector; + } + }
return 0;
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index eed6a9e8d9a6..4618d98913b4 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -432,6 +432,40 @@ static struct drm_panel *sun4i_tcon_find_panel(struct device_node *node) return of_drm_find_panel(remote) ?: ERR_PTR(-EPROBE_DEFER); }
+static struct drm_bridge *sun4i_tcon_find_bridge(struct device_node *node) +{ + struct device_node *port, *remote, *child; + struct device_node *end_node = NULL; + + /* Inputs are listed first, then outputs */ + port = of_graph_get_port_by_id(node, 1); + + /* + * Our first output is the RGB interface where the panel will + * be connected. + */ + for_each_child_of_node(port, child) { + u32 reg; + + of_property_read_u32(child, "reg", ®); + if (reg == 0) + end_node = child; + } + + if (!end_node) { + DRM_DEBUG_DRIVER("Missing bridge endpoint\n"); + return ERR_PTR(-ENODEV); + } + + remote = of_graph_get_remote_port_parent(end_node); + if (!remote) { + DRM_DEBUG_DRIVER("Enable to parse remote node\n"); + return ERR_PTR(-EINVAL); + } + + return of_drm_find_bridge(remote) ?: ERR_PTR(-EPROBE_DEFER); +} + static int sun4i_tcon_bind(struct device *dev, struct device *master, void *data) { @@ -485,8 +519,9 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master, }
tcon->panel = sun4i_tcon_find_panel(dev->of_node); - if (IS_ERR(tcon->panel)) { - dev_info(dev, "No panel found... RGB output disabled\n"); + tcon->bridge = sun4i_tcon_find_bridge(dev->of_node); + if (IS_ERR(tcon->panel) && IS_ERR(tcon->bridge)) { + dev_info(dev, "No panel or bridge found... RGB output disabled\n"); return 0; }
@@ -515,19 +550,22 @@ static struct component_ops sun4i_tcon_ops = { static int sun4i_tcon_probe(struct platform_device *pdev) { struct device_node *node = pdev->dev.of_node; + struct drm_bridge *bridge; struct drm_panel *panel;
/* - * The panel is not ready. + * Neither the bridge or the panel is ready. * Defer the probe. */ panel = sun4i_tcon_find_panel(node); + bridge = sun4i_tcon_find_bridge(node);
/* * If we don't have a panel endpoint, just go on */ - if (PTR_ERR(panel) == -EPROBE_DEFER) { - DRM_DEBUG_DRIVER("Still waiting for our panel. Deferring...\n"); + if ((PTR_ERR(panel) == -EPROBE_DEFER) && + (PTR_ERR(bridge) == -EPROBE_DEFER)) { + DRM_DEBUG_DRIVER("Still waiting for our panel/bridge. Deferring...\n"); return -EPROBE_DEFER; }
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h index 0e0b11db401b..31069027d7fb 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h @@ -162,6 +162,7 @@ struct sun4i_tcon { /* Platform adjustments */ bool has_mux;
+ struct drm_bridge *bridge; struct drm_panel *panel; };
On Mon, May 16, 2016 at 2:47 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Our RGB bus can be either connected to a bridge or a panel. While the panel support was already there, the bridge was not.
Fix that.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
For bridge support the only thing you need is to set drm_encoder->bridge. Why do you need to hand-roll your own bridge support? That pretty muchmeans you'll do it slightly differently (e.g. you don't bother with a lot of the calls), which will make sharing bridge drivers ever so harder. And also reviewing code, since using shared code but slightly differently is really confusing. -Daniel
drivers/gpu/drm/sun4i/sun4i_drv.c | 4 +-- drivers/gpu/drm/sun4i/sun4i_rgb.c | 56 +++++++++++++++++++++++++++----------- drivers/gpu/drm/sun4i/sun4i_tcon.c | 48 ++++++++++++++++++++++++++++---- drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + 4 files changed, 86 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c index 257d2b4f3645..1f9e00db747d 100644 --- a/drivers/gpu/drm/sun4i/sun4i_drv.c +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c @@ -268,8 +268,8 @@ static int sun4i_drv_add_endpoints(struct device *dev, }
/*
* If the node is our TCON, the first port is used for our* panel, and will not be part of the
* If the node is our TCON, the first port is used for* panel or bridges, and will not be part of the * component framework. */ if (sun4i_drv_node_is_tcon(node)) {diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index b46d2c15dc95..c3c611b08269 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -161,7 +161,12 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
DRM_DEBUG_DRIVER("Enabling RGB output\n");
drm_panel_enable(tcon->panel);
if (!IS_ERR(tcon->panel))drm_panel_enable(tcon->panel);if (!IS_ERR(tcon->bridge))drm_bridge_enable(tcon->bridge);sun4i_tcon_channel_enable(tcon, 0);}
@@ -174,7 +179,12 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) DRM_DEBUG_DRIVER("Disabling RGB output\n");
sun4i_tcon_channel_disable(tcon, 0);
drm_panel_disable(tcon->panel);
if (!IS_ERR(tcon->bridge))drm_bridge_disable(tcon->bridge);if (!IS_ERR(tcon->panel))drm_panel_disable(tcon->panel);}
static void sun4i_rgb_encoder_mode_set(struct drm_encoder *encoder, @@ -216,10 +226,6 @@ int sun4i_rgb_init(struct drm_device *drm) struct sun4i_rgb *rgb; int ret;
/* If we don't have a panel, there's no point in going on */if (IS_ERR(tcon->panel))return -ENODEV;rgb = devm_kzalloc(drm->dev, sizeof(*rgb), GFP_KERNEL); if (!rgb) return -ENOMEM;@@ -240,19 +246,37 @@ int sun4i_rgb_init(struct drm_device *drm) /* The RGB encoder can only work with the TCON channel 0 */ rgb->encoder.possible_crtcs = BIT(0);
drm_connector_helper_add(&rgb->connector,&sun4i_rgb_con_helper_funcs);ret = drm_connector_init(drm, &rgb->connector,&sun4i_rgb_con_funcs,DRM_MODE_CONNECTOR_Unknown);if (ret) {dev_err(drm->dev, "Couldn't initialise the rgb connector\n");goto err_cleanup_connector;
if (!IS_ERR(tcon->panel)) {drm_connector_helper_add(&rgb->connector,&sun4i_rgb_con_helper_funcs);ret = drm_connector_init(drm, &rgb->connector,&sun4i_rgb_con_funcs,DRM_MODE_CONNECTOR_Unknown);if (ret) {dev_err(drm->dev, "Couldn't initialise the rgb connector\n");goto err_cleanup_connector;}drm_mode_connector_attach_encoder(&rgb->connector,&rgb->encoder);ret = drm_panel_attach(tcon->panel, &rgb->connector);if (ret) {dev_err(drm->dev, "Couldn't attach our panel\n");goto err_cleanup_connector;} }
drm_mode_connector_attach_encoder(&rgb->connector, &rgb->encoder);
if (!IS_ERR(tcon->bridge)) {rgb->encoder.bridge = tcon->bridge;tcon->bridge->encoder = &rgb->encoder;
drm_panel_attach(tcon->panel, &rgb->connector);
ret = drm_bridge_attach(drm, tcon->bridge);if (ret) {dev_err(drm->dev, "Couldn't attach our bridge\n");goto err_cleanup_connector;}} return 0;diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index eed6a9e8d9a6..4618d98913b4 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -432,6 +432,40 @@ static struct drm_panel *sun4i_tcon_find_panel(struct device_node *node) return of_drm_find_panel(remote) ?: ERR_PTR(-EPROBE_DEFER); }
+static struct drm_bridge *sun4i_tcon_find_bridge(struct device_node *node) +{
struct device_node *port, *remote, *child;struct device_node *end_node = NULL;/* Inputs are listed first, then outputs */port = of_graph_get_port_by_id(node, 1);/** Our first output is the RGB interface where the panel will* be connected.*/for_each_child_of_node(port, child) {u32 reg;of_property_read_u32(child, "reg", ®);if (reg == 0)end_node = child;}if (!end_node) {DRM_DEBUG_DRIVER("Missing bridge endpoint\n");return ERR_PTR(-ENODEV);}remote = of_graph_get_remote_port_parent(end_node);if (!remote) {DRM_DEBUG_DRIVER("Enable to parse remote node\n");return ERR_PTR(-EINVAL);}return of_drm_find_bridge(remote) ?: ERR_PTR(-EPROBE_DEFER);+}
static int sun4i_tcon_bind(struct device *dev, struct device *master, void *data) { @@ -485,8 +519,9 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master, }
tcon->panel = sun4i_tcon_find_panel(dev->of_node);
if (IS_ERR(tcon->panel)) {dev_info(dev, "No panel found... RGB output disabled\n");
tcon->bridge = sun4i_tcon_find_bridge(dev->of_node);if (IS_ERR(tcon->panel) && IS_ERR(tcon->bridge)) {dev_info(dev, "No panel or bridge found... RGB output disabled\n"); return 0; }@@ -515,19 +550,22 @@ static struct component_ops sun4i_tcon_ops = { static int sun4i_tcon_probe(struct platform_device *pdev) { struct device_node *node = pdev->dev.of_node;
struct drm_bridge *bridge; struct drm_panel *panel; /*
* The panel is not ready.
* Neither the bridge or the panel is ready. * Defer the probe. */ panel = sun4i_tcon_find_panel(node);bridge = sun4i_tcon_find_bridge(node); /* * If we don't have a panel endpoint, just go on */
if (PTR_ERR(panel) == -EPROBE_DEFER) {DRM_DEBUG_DRIVER("Still waiting for our panel. Deferring...\n");
if ((PTR_ERR(panel) == -EPROBE_DEFER) &&(PTR_ERR(bridge) == -EPROBE_DEFER)) {DRM_DEBUG_DRIVER("Still waiting for our panel/bridge. Deferring...\n"); return -EPROBE_DEFER; }diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h index 0e0b11db401b..31069027d7fb 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h @@ -162,6 +162,7 @@ struct sun4i_tcon { /* Platform adjustments */ bool has_mux;
struct drm_bridge *bridge; struct drm_panel *panel;};
-- 2.8.2
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel,
On Mon, May 16, 2016 at 03:12:15PM +0200, Daniel Vetter wrote:
On Mon, May 16, 2016 at 2:47 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Our RGB bus can be either connected to a bridge or a panel. While the panel support was already there, the bridge was not.
Fix that.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
For bridge support the only thing you need is to set drm_encoder->bridge. Why do you need to hand-roll your own bridge support? That pretty muchmeans you'll do it slightly differently (e.g. you don't bother with a lot of the calls), which will make sharing bridge drivers ever so harder. And also reviewing code, since using shared code but slightly differently is really confusing.
Because I overlooked it :)
I'll change that.
Thanks! Maxime
Some boards have an entirely passive RGB to VGA bridge, based on either DACs or resistor ladders.
Those might or might not have an i2c bus routed to the VGA connector in order to access the screen EDIDs.
Add a bridge that doesn't do anything but expose the modes available on the screen, either based on the EDIDs if available, or based on the XGA standards.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- .../bindings/display/bridge/dumb-vga.txt | 40 +++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/dumb-vga.c | 186 +++++++++++++++++++++ 4 files changed, 233 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/bridge/dumb-vga.txt create mode 100644 drivers/gpu/drm/bridge/dumb-vga.c
diff --git a/Documentation/devicetree/bindings/display/bridge/dumb-vga.txt b/Documentation/devicetree/bindings/display/bridge/dumb-vga.txt new file mode 100644 index 000000000000..757f04de97f3 --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga.txt @@ -0,0 +1,40 @@ +Passive RGB to VGA bridge +------------------------- + +This binding is aimed for entirely passive RGB to VGA bridges that do not +require any configuration. + +Required properties: + +- compatible: Should be "dumb-vga-bridge" + +Optional properties: + +- ddc-i2c-bus: phandle to the i2c bus used to communicate with the monitor + +Required nodes: + +This device has one video port. Its connection is modelled using the OF +graph bindings specified in Documentation/devicetree/bindings/graph.txt. This +connection is meant to be the RGB input bus. + + +Example +------- + +bridge { + compatible = "dumb-vga-bridge"; + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + #address-cells = <1>; + #size-cells = <0>; + + vga_input: endpoint@0 { + reg = <0>; + remote-endpoint = <&tcon0_out_vga>; + }; + }; +}; diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 8f7423f18da5..cf279956cbec 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -17,6 +17,12 @@ config DRM_ANALOGIX_ANX78XX the HDMI output of an application processor to MyDP or DisplayPort.
+config DRM_DUMB_VGA + tristate "Dumb RGB to VGA Bridge support" + select DRM_KMS_HELPER + help + Support for passive RGB to VGA bridges + config DRM_DW_HDMI tristate select DRM_KMS_HELPER diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index 96b13b30e6ab..63f21120e8f8 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -1,6 +1,7 @@ ccflags-y := -Iinclude/drm
obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o +obj-$(CONFIG_DRM_DUMB_VGA) += dumb-vga.o obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o diff --git a/drivers/gpu/drm/bridge/dumb-vga.c b/drivers/gpu/drm/bridge/dumb-vga.c new file mode 100644 index 000000000000..2864fa535bf8 --- /dev/null +++ b/drivers/gpu/drm/bridge/dumb-vga.c @@ -0,0 +1,186 @@ + +#include <linux/module.h> + +#include <drm/drmP.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_crtc.h> +#include <drm/drm_crtc_helper.h> + +struct dumb_vga { + struct drm_bridge bridge; + struct drm_connector connector; + + struct i2c_adapter *ddc; +}; + +static inline struct dumb_vga * +drm_bridge_to_dumb_vga(struct drm_bridge *bridge) +{ + return container_of(bridge, struct dumb_vga, bridge); +} + +static inline struct dumb_vga * +drm_connector_to_dumb_vga(struct drm_connector *connector) +{ + return container_of(connector, struct dumb_vga, connector); +} + +static int dumb_vga_get_modes(struct drm_connector *connector) +{ + struct dumb_vga *vga = drm_connector_to_dumb_vga(connector); + struct edid *edid; + int ret; + + if (!vga->ddc) + goto fallback; + + edid = drm_get_edid(connector, vga->ddc); + if (!edid) { + DRM_INFO("EDID readout failed, falling back to standard modes\n"); + goto fallback; + } + + drm_mode_connector_update_edid_property(connector, edid); + return drm_add_edid_modes(connector, edid); + +fallback: + /* + * In case we cannot retrieve the EDIDs (broken or missing i2c + * bus), fallback on the XGA standards + */ + ret = drm_add_modes_noedid(connector, 1920, 1200); + + /* And prefer a mode pretty much anyone can handle */ + drm_set_preferred_mode(connector, 1024, 768); + + return ret; +} + +static struct drm_encoder * +dumb_vga_best_encoder(struct drm_connector *connector) +{ + struct dumb_vga *vga = drm_connector_to_dumb_vga(connector); + + return vga->bridge.encoder; +} + +static struct drm_connector_helper_funcs dumb_vga_con_helper_funcs = { + .get_modes = dumb_vga_get_modes, + .best_encoder = dumb_vga_best_encoder, +}; + +static enum drm_connector_status +dumb_vga_connector_detect(struct drm_connector *connector, bool force) +{ + return connector_status_connected; +} + +static void +dumb_vga_connector_destroy(struct drm_connector *connector) +{ + drm_connector_cleanup(connector); +} + +static struct drm_connector_funcs dumb_vga_con_funcs = { + .dpms = drm_atomic_helper_connector_dpms, + .detect = dumb_vga_connector_detect, + .fill_modes = drm_helper_probe_single_connector_modes, + .destroy = dumb_vga_connector_destroy, + .reset = drm_atomic_helper_connector_reset, + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, +}; + +static int dumb_vga_attach(struct drm_bridge *bridge) +{ + struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge); + int ret; + + if (!bridge->encoder) { + DRM_ERROR("Missing encoder\n"); + return -ENODEV; + } + + drm_connector_helper_add(&vga->connector, + &dumb_vga_con_helper_funcs); + ret = drm_connector_init(bridge->dev, &vga->connector, + &dumb_vga_con_funcs, DRM_MODE_CONNECTOR_VGA); + if (ret) { + DRM_ERROR("Failed to initialize connector\n"); + return ret; + } + + drm_mode_connector_attach_encoder(&vga->connector, + bridge->encoder); + + return 0; +} + +static void dumb_vga_nop(struct drm_bridge *bridge) {}; + +static struct drm_bridge_funcs dumb_vga_bridge_funcs = { + .attach = dumb_vga_attach, + .enable = dumb_vga_nop, + .disable = dumb_vga_nop, + .pre_enable = dumb_vga_nop, + .post_disable = dumb_vga_nop, +}; + +static int dumb_vga_probe(struct platform_device *pdev) +{ + struct dumb_vga *vga; + struct device_node *ddc; + + vga = devm_kzalloc(&pdev->dev, sizeof(*vga), GFP_KERNEL); + if (!vga) + return -ENOMEM; + platform_set_drvdata(pdev, vga); + + ddc = of_parse_phandle(pdev->dev.of_node, "ddc-i2c-bus", 0); + if (ddc) { + vga->ddc = of_find_i2c_adapter_by_node(ddc); + of_node_put(ddc); + + if (!vga->ddc) { + dev_err(&pdev->dev, "Couldn't retrieve i2c bus\n"); + return -EPROBE_DEFER; + } + } else { + dev_info(&pdev->dev, + "No i2c bus specified... Disabling EDID readout\n"); + } + + vga->bridge.funcs = &dumb_vga_bridge_funcs; + vga->bridge.of_node = pdev->dev.of_node; + + return drm_bridge_add(&vga->bridge); +} + +static int dumb_vga_remove(struct platform_device *pdev) +{ + struct dumb_vga *vga = platform_get_drvdata(pdev); + + drm_bridge_remove(&vga->bridge); + + return 0; +} + +static const struct of_device_id dumb_vga_match[] = { + { .compatible = "dumb-vga-bridge" }, + {}, +}; +MODULE_DEVICE_TABLE(of, dumb_vga_match); + +struct platform_driver dumb_vga_driver = { + .probe = dumb_vga_probe, + .remove = dumb_vga_remove, + .driver = { + .name = "dumb-vga-bridge", + .of_match_table = dumb_vga_match, + }, +}; +module_platform_driver(dumb_vga_driver); + +MODULE_AUTHOR("Maxime Ripard maxime.ripard@free-electrons.com"); +MODULE_DESCRIPTION("Dumb RGB to VGA bridge driver"); +MODULE_LICENSE("GPL");
Hi Maxime,
Thank you for the patch.
On Monday 16 May 2016 14:47:13 Maxime Ripard wrote:
Some boards have an entirely passive RGB to VGA bridge, based on either DACs or resistor ladders.
Those might or might not have an i2c bus routed to the VGA connector in order to access the screen EDIDs.
Add a bridge that doesn't do anything but expose the modes available on the screen, either based on the EDIDs if available, or based on the XGA standards.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
.../bindings/display/bridge/dumb-vga.txt | 40 +++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/dumb-vga.c | 186 ++++++++++++++++++ 4 files changed, 233 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/bridge/dumb-vga.txt create mode 100644 drivers/gpu/drm/bridge/dumb-vga.c
diff --git a/Documentation/devicetree/bindings/display/bridge/dumb-vga.txt b/Documentation/devicetree/bindings/display/bridge/dumb-vga.txt new file mode 100644 index 000000000000..757f04de97f3 --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga.txt @@ -0,0 +1,40 @@ +Passive RGB to VGA bridge +-------------------------
+This binding is aimed for entirely passive RGB to VGA bridges that do not +require any configuration.
+Required properties:
+- compatible: Should be "dumb-vga-bridge"
+Optional properties:
+- ddc-i2c-bus: phandle to the i2c bus used to communicate with the monitor
+Required nodes:
+This device has one video port. Its connection is modelled using the OF +graph bindings specified in Documentation/devicetree/bindings/graph.txt. This +connection is meant to be the RGB input bus.
Should the bridge have two ports, one input port connected to a display engine and one output port connected to a VGA connector ?
+Example +-------
+bridge {
compatible = "dumb-vga-bridge";#address-cells = <1>;#size-cells = <0>;port@0 {reg = <0>;#address-cells = <1>;#size-cells = <0>;vga_input: endpoint@0 {reg = <0>;remote-endpoint = <&tcon0_out_vga>;};};+}; diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 8f7423f18da5..cf279956cbec 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -17,6 +17,12 @@ config DRM_ANALOGIX_ANX78XX the HDMI output of an application processor to MyDP or DisplayPort.
+config DRM_DUMB_VGA
- tristate "Dumb RGB to VGA Bridge support"
- select DRM_KMS_HELPER
- help
Support for passive RGB to VGA bridgesconfig DRM_DW_HDMI tristate select DRM_KMS_HELPER diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index 96b13b30e6ab..63f21120e8f8 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -1,6 +1,7 @@ ccflags-y := -Iinclude/drm
obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o +obj-$(CONFIG_DRM_DUMB_VGA) += dumb-vga.o obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o diff --git a/drivers/gpu/drm/bridge/dumb-vga.c b/drivers/gpu/drm/bridge/dumb-vga.c new file mode 100644 index 000000000000..2864fa535bf8 --- /dev/null +++ b/drivers/gpu/drm/bridge/dumb-vga.c @@ -0,0 +1,186 @@
+#include <linux/module.h>
+#include <drm/drmP.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_crtc.h> +#include <drm/drm_crtc_helper.h>
+struct dumb_vga {
- struct drm_bridge bridge;
- struct drm_connector connector;
- struct i2c_adapter *ddc;
+};
+static inline struct dumb_vga * +drm_bridge_to_dumb_vga(struct drm_bridge *bridge) +{
- return container_of(bridge, struct dumb_vga, bridge);
+}
+static inline struct dumb_vga * +drm_connector_to_dumb_vga(struct drm_connector *connector) +{
- return container_of(connector, struct dumb_vga, connector);
+}
+static int dumb_vga_get_modes(struct drm_connector *connector) +{
- struct dumb_vga *vga = drm_connector_to_dumb_vga(connector);
- struct edid *edid;
- int ret;
- if (!vga->ddc)
goto fallback;- edid = drm_get_edid(connector, vga->ddc);
- if (!edid) {
DRM_INFO("EDID readout failed, falling back to standard modes\n");goto fallback;- }
- drm_mode_connector_update_edid_property(connector, edid);
- return drm_add_edid_modes(connector, edid);
+fallback:
- /*
* In case we cannot retrieve the EDIDs (broken or missing i2c* bus), fallback on the XGA standards*/- ret = drm_add_modes_noedid(connector, 1920, 1200);
The DRM core adds modes up to 1024x768 in drm_helper_probe_single_connector_modes(). I wonder if it really makes sense to override that in drivers, compared to increasing the maximum resolution in the core. What we can reasonable expect from a VGA monitor doesn't really depend on which display engine it is connected to.
- /* And prefer a mode pretty much anyone can handle */
- drm_set_preferred_mode(connector, 1024, 768);
- return ret;
+}
+static struct drm_encoder * +dumb_vga_best_encoder(struct drm_connector *connector) +{
- struct dumb_vga *vga = drm_connector_to_dumb_vga(connector);
- return vga->bridge.encoder;
+}
+static struct drm_connector_helper_funcs dumb_vga_con_helper_funcs = {
- .get_modes = dumb_vga_get_modes,
- .best_encoder = dumb_vga_best_encoder,
+};
+static enum drm_connector_status +dumb_vga_connector_detect(struct drm_connector *connector, bool force) +{
- return connector_status_connected;
Shouldn't that be connector_status_unknown if you have no information about the connection status ?
Would it make sense to add an optional GPIO-based connection detection to this driver ?
+}
+static void +dumb_vga_connector_destroy(struct drm_connector *connector) +{
- drm_connector_cleanup(connector);
+}
+static struct drm_connector_funcs dumb_vga_con_funcs = {
- .dpms = drm_atomic_helper_connector_dpms,
- .detect = dumb_vga_connector_detect,
- .fill_modes = drm_helper_probe_single_connector_modes,
- .destroy = dumb_vga_connector_destroy,
- .reset = drm_atomic_helper_connector_reset,
- .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
- .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+static int dumb_vga_attach(struct drm_bridge *bridge) +{
- struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
- int ret;
- if (!bridge->encoder) {
DRM_ERROR("Missing encoder\n");return -ENODEV;- }
- drm_connector_helper_add(&vga->connector,
&dumb_vga_con_helper_funcs);- ret = drm_connector_init(bridge->dev, &vga->connector,
&dumb_vga_con_funcs, DRM_MODE_CONNECTOR_VGA);- if (ret) {
DRM_ERROR("Failed to initialize connector\n");return ret;- }
- drm_mode_connector_attach_encoder(&vga->connector,
bridge->encoder);- return 0;
+}
+static void dumb_vga_nop(struct drm_bridge *bridge) {};
+static struct drm_bridge_funcs dumb_vga_bridge_funcs = {
- .attach = dumb_vga_attach,
- .enable = dumb_vga_nop,
- .disable = dumb_vga_nop,
- .pre_enable = dumb_vga_nop,
- .post_disable = dumb_vga_nop,
+};
+static int dumb_vga_probe(struct platform_device *pdev) +{
- struct dumb_vga *vga;
- struct device_node *ddc;
- vga = devm_kzalloc(&pdev->dev, sizeof(*vga), GFP_KERNEL);
- if (!vga)
return -ENOMEM;- platform_set_drvdata(pdev, vga);
- ddc = of_parse_phandle(pdev->dev.of_node, "ddc-i2c-bus", 0);
- if (ddc) {
vga->ddc = of_find_i2c_adapter_by_node(ddc);
You're leaking the reference to the I2C adapter.
Also, shouldn't you use of_get_i2c_adapter_by_node() ? Otherwise you won't take a reference to the adapter module.
of_node_put(ddc);if (!vga->ddc) {dev_err(&pdev->dev, "Couldn't retrieve i2c bus\n");return -EPROBE_DEFER;}- } else {
dev_info(&pdev->dev,"No i2c bus specified... Disabling EDID readout\n");- }
- vga->bridge.funcs = &dumb_vga_bridge_funcs;
- vga->bridge.of_node = pdev->dev.of_node;
- return drm_bridge_add(&vga->bridge);
+}
+static int dumb_vga_remove(struct platform_device *pdev) +{
- struct dumb_vga *vga = platform_get_drvdata(pdev);
- drm_bridge_remove(&vga->bridge);
- return 0;
+}
+static const struct of_device_id dumb_vga_match[] = {
- { .compatible = "dumb-vga-bridge" },
- {},
+}; +MODULE_DEVICE_TABLE(of, dumb_vga_match);
+struct platform_driver dumb_vga_driver = {
- .probe = dumb_vga_probe,
- .remove = dumb_vga_remove,
- .driver = {
.name = "dumb-vga-bridge",.of_match_table = dumb_vga_match,- },
+}; +module_platform_driver(dumb_vga_driver);
+MODULE_AUTHOR("Maxime Ripard maxime.ripard@free-electrons.com"); +MODULE_DESCRIPTION("Dumb RGB to VGA bridge driver"); +MODULE_LICENSE("GPL");
Hi Laurent,
On Mon, May 16, 2016 at 04:24:15PM +0300, Laurent Pinchart wrote:
Hi Maxime,
Thank you for the patch.
On Monday 16 May 2016 14:47:13 Maxime Ripard wrote:
Some boards have an entirely passive RGB to VGA bridge, based on either DACs or resistor ladders.
Those might or might not have an i2c bus routed to the VGA connector in order to access the screen EDIDs.
Add a bridge that doesn't do anything but expose the modes available on the screen, either based on the EDIDs if available, or based on the XGA standards.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
.../bindings/display/bridge/dumb-vga.txt | 40 +++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/dumb-vga.c | 186 ++++++++++++++++++ 4 files changed, 233 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/bridge/dumb-vga.txt create mode 100644 drivers/gpu/drm/bridge/dumb-vga.c
diff --git a/Documentation/devicetree/bindings/display/bridge/dumb-vga.txt b/Documentation/devicetree/bindings/display/bridge/dumb-vga.txt new file mode 100644 index 000000000000..757f04de97f3 --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga.txt @@ -0,0 +1,40 @@ +Passive RGB to VGA bridge +-------------------------
+This binding is aimed for entirely passive RGB to VGA bridges that do not +require any configuration.
+Required properties:
+- compatible: Should be "dumb-vga-bridge"
+Optional properties:
+- ddc-i2c-bus: phandle to the i2c bus used to communicate with the monitor
+Required nodes:
+This device has one video port. Its connection is modelled using the OF +graph bindings specified in Documentation/devicetree/bindings/graph.txt. This +connection is meant to be the RGB input bus.
Should the bridge have two ports, one input port connected to a display engine and one output port connected to a VGA connector ?
And a separate node for the VGA connector? As far as I can see, all the other bridges are represented using a single node.
+static int dumb_vga_get_modes(struct drm_connector *connector)
+{
- struct dumb_vga *vga = drm_connector_to_dumb_vga(connector);
- struct edid *edid;
- int ret;
- if (!vga->ddc)
goto fallback;- edid = drm_get_edid(connector, vga->ddc);
- if (!edid) {
DRM_INFO("EDID readout failed, falling back to standard modes\n");goto fallback;- }
- drm_mode_connector_update_edid_property(connector, edid);
- return drm_add_edid_modes(connector, edid);
+fallback:
- /*
* In case we cannot retrieve the EDIDs (broken or missing i2c* bus), fallback on the XGA standards*/- ret = drm_add_modes_noedid(connector, 1920, 1200);
The DRM core adds modes up to 1024x768 in drm_helper_probe_single_connector_modes(). I wonder if it really makes sense to override that in drivers, compared to increasing the maximum resolution in the core. What we can reasonable expect from a VGA monitor doesn't really depend on which display engine it is connected to.
Actually, I would expect it to come from the connectors. Nowadays, the core add modes that might not even be relevant at all for a given connector. Take TV as an example, there's not a single TV output that can reach 1024x768, but actually rely on very different resolutions.
- /* And prefer a mode pretty much anyone can handle */
- drm_set_preferred_mode(connector, 1024, 768);
- return ret;
+}
+static struct drm_encoder * +dumb_vga_best_encoder(struct drm_connector *connector) +{
- struct dumb_vga *vga = drm_connector_to_dumb_vga(connector);
- return vga->bridge.encoder;
+}
+static struct drm_connector_helper_funcs dumb_vga_con_helper_funcs = {
- .get_modes = dumb_vga_get_modes,
- .best_encoder = dumb_vga_best_encoder,
+};
+static enum drm_connector_status +dumb_vga_connector_detect(struct drm_connector *connector, bool force) +{
- return connector_status_connected;
Shouldn't that be connector_status_unknown if you have no information about the connection status ?
Probably :)
What are the side effects from such a change? Does it change anything related to how the fbdev emulation and how the rest of the display stack detects and enables the outputs whose status are unknown?
Would it make sense to add an optional GPIO-based connection detection to this driver ?
I asked for it in our prototypes, and apparently, doing VGA cable detection is really non-trivial, so I don't really expect it to be found in a lot of designs.
I looked at three different designs done by different vendors, and none of them have it, so I don't really expect anyone to have it, nor do I have any hardware to test the code with. However, if there's an i2c bus, we can always try to probe for the edid, and see if we have an error. If there's none, then we know something is connected. If there's an error, we can return connector_status_unknown.
On a set of three samples, only one has that i2c bus, so it might be just a marginal improvement, but still.
+}
+static void +dumb_vga_connector_destroy(struct drm_connector *connector) +{
- drm_connector_cleanup(connector);
+}
+static struct drm_connector_funcs dumb_vga_con_funcs = {
- .dpms = drm_atomic_helper_connector_dpms,
- .detect = dumb_vga_connector_detect,
- .fill_modes = drm_helper_probe_single_connector_modes,
- .destroy = dumb_vga_connector_destroy,
- .reset = drm_atomic_helper_connector_reset,
- .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
- .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+static int dumb_vga_attach(struct drm_bridge *bridge) +{
- struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
- int ret;
- if (!bridge->encoder) {
DRM_ERROR("Missing encoder\n");return -ENODEV;- }
- drm_connector_helper_add(&vga->connector,
&dumb_vga_con_helper_funcs);- ret = drm_connector_init(bridge->dev, &vga->connector,
&dumb_vga_con_funcs, DRM_MODE_CONNECTOR_VGA);- if (ret) {
DRM_ERROR("Failed to initialize connector\n");return ret;- }
- drm_mode_connector_attach_encoder(&vga->connector,
bridge->encoder);- return 0;
+}
+static void dumb_vga_nop(struct drm_bridge *bridge) {};
+static struct drm_bridge_funcs dumb_vga_bridge_funcs = {
- .attach = dumb_vga_attach,
- .enable = dumb_vga_nop,
- .disable = dumb_vga_nop,
- .pre_enable = dumb_vga_nop,
- .post_disable = dumb_vga_nop,
+};
+static int dumb_vga_probe(struct platform_device *pdev) +{
- struct dumb_vga *vga;
- struct device_node *ddc;
- vga = devm_kzalloc(&pdev->dev, sizeof(*vga), GFP_KERNEL);
- if (!vga)
return -ENOMEM;- platform_set_drvdata(pdev, vga);
- ddc = of_parse_phandle(pdev->dev.of_node, "ddc-i2c-bus", 0);
- if (ddc) {
vga->ddc = of_find_i2c_adapter_by_node(ddc);You're leaking the reference to the I2C adapter.
Also, shouldn't you use of_get_i2c_adapter_by_node() ? Otherwise you won't take a reference to the adapter module.
Indeed, I'll fix it.
Thanks! Maxime
On Thu, May 26, 2016 at 10:53:30AM +0200, Maxime Ripard wrote:
Hi Laurent,
On Mon, May 16, 2016 at 04:24:15PM +0300, Laurent Pinchart wrote:
Hi Maxime,
Thank you for the patch.
On Monday 16 May 2016 14:47:13 Maxime Ripard wrote:
+fallback:
- /*
* In case we cannot retrieve the EDIDs (broken or missing i2c* bus), fallback on the XGA standards*/- ret = drm_add_modes_noedid(connector, 1920, 1200);
The DRM core adds modes up to 1024x768 in drm_helper_probe_single_connector_modes(). I wonder if it really makes sense to override that in drivers, compared to increasing the maximum resolution in the core. What we can reasonable expect from a VGA monitor doesn't really depend on which display engine it is connected to.
Actually, I would expect it to come from the connectors. Nowadays, the core add modes that might not even be relevant at all for a given connector. Take TV as an example, there's not a single TV output that can reach 1024x768, but actually rely on very different resolutions.
There is the requirement to support 640x480 almost universally.
Would it make sense to add an optional GPIO-based connection detection to this driver ?
I asked for it in our prototypes, and apparently, doing VGA cable detection is really non-trivial, so I don't really expect it to be found in a lot of designs.
... and prone to problems. Intel designs try to measure the termination resistance to detect whether something is connected (they pulse the RGB signals and measure the current.) This mostly works fine, except if you have some KVM boxes which can make this very unreliable.
I ended up replacing the bipolar transistors switching the RGB signals in my KVM switch with FETs because of this problem.
On Mon, May 16, 2016 at 7:47 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Some boards have an entirely passive RGB to VGA bridge, based on either DACs or resistor ladders.
Those might or might not have an i2c bus routed to the VGA connector in order to access the screen EDIDs.
Add a bridge that doesn't do anything but expose the modes available on the screen, either based on the EDIDs if available, or based on the XGA standards.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
.../bindings/display/bridge/dumb-vga.txt | 40 +++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/dumb-vga.c | 186 +++++++++++++++++++++ 4 files changed, 233 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/bridge/dumb-vga.txt create mode 100644 drivers/gpu/drm/bridge/dumb-vga.c
diff --git a/Documentation/devicetree/bindings/display/bridge/dumb-vga.txt b/Documentation/devicetree/bindings/display/bridge/dumb-vga.txt new file mode 100644 index 000000000000..757f04de97f3 --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga.txt @@ -0,0 +1,40 @@ +Passive RGB to VGA bridge +-------------------------
+This binding is aimed for entirely passive RGB to VGA bridges that do not +require any configuration.
No reset or enable lines or regulators?
+Required properties:
+- compatible: Should be "dumb-vga-bridge"
+Optional properties:
+- ddc-i2c-bus: phandle to the i2c bus used to communicate with the monitor
The i2c bus is a property of the connector, not the bridge chip, so it belongs in a VGA connector node (which we already have binding for).
I think for the really "dumb" (or transparent case), you should only have a connector. For anything more complex, then you should have both a DAC and connector node.
On the driver side, I think we could handle multiple connectors with a single driver. Ideally, that would work with any DRM driver that has a connector node.
+Required nodes:
+This device has one video port. Its connection is modelled using the OF +graph bindings specified in Documentation/devicetree/bindings/graph.txt. This +connection is meant to be the RGB input bus.
+Example +-------
+bridge {
compatible = "dumb-vga-bridge";#address-cells = <1>;#size-cells = <0>;port@0 {reg = <0>;#address-cells = <1>;#size-cells = <0>;vga_input: endpoint@0 {reg = <0>;remote-endpoint = <&tcon0_out_vga>;};};+};
Hi Rob,
On Mon, May 16, 2016 at 09:07:18AM -0500, Rob Herring wrote:
On Mon, May 16, 2016 at 7:47 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Some boards have an entirely passive RGB to VGA bridge, based on either DACs or resistor ladders.
Those might or might not have an i2c bus routed to the VGA connector in order to access the screen EDIDs.
Add a bridge that doesn't do anything but expose the modes available on the screen, either based on the EDIDs if available, or based on the XGA standards.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
.../bindings/display/bridge/dumb-vga.txt | 40 +++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/dumb-vga.c | 186 +++++++++++++++++++++ 4 files changed, 233 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/bridge/dumb-vga.txt create mode 100644 drivers/gpu/drm/bridge/dumb-vga.c
diff --git a/Documentation/devicetree/bindings/display/bridge/dumb-vga.txt b/Documentation/devicetree/bindings/display/bridge/dumb-vga.txt new file mode 100644 index 000000000000..757f04de97f3 --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga.txt @@ -0,0 +1,40 @@ +Passive RGB to VGA bridge +-------------------------
+This binding is aimed for entirely passive RGB to VGA bridges that do not +require any configuration.
No reset or enable lines or regulators?
On the three designs I've seen: - One doesn't need any of these - One has a GPIO enable pin but with it's resistor not populated - One has a regulator controlled by a GPIO
So I guess in most cases, we don't need anything, but we still might to cover all cases.
+Required properties:
+- compatible: Should be "dumb-vga-bridge"
+Optional properties:
+- ddc-i2c-bus: phandle to the i2c bus used to communicate with the monitor
The i2c bus is a property of the connector, not the bridge chip, so it belongs in a VGA connector node (which we already have binding for).
I think for the really "dumb" (or transparent case), you should only have a connector. For anything more complex, then you should have both a DAC and connector node.
So, in essence, something like: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/ar...
On the driver side, I think we could handle multiple connectors with a single driver. Ideally, that would work with any DRM driver that has a connector node.
I'm not sure what you mean by that. I really think it should be a separate driver in order to share the common code that so far the current implementations put in some driver specific code (even though it might be completely generic).
Thanks! Maxime
The RGB bus can be used in several configurations, one of which being the RGB666.
Add a pinctrl group for that case.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- arch/arm/boot/dts/sun5i-a13.dtsi | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi index 263d46dbc7e6..79b5d513c142 100644 --- a/arch/arm/boot/dts/sun5i-a13.dtsi +++ b/arch/arm/boot/dts/sun5i-a13.dtsi @@ -237,6 +237,16 @@ &pio { compatible = "allwinner,sun5i-a13-pinctrl";
+ lcd_rgb666_pins: lcd_rgb666@0 { + allwinner,pins = "PD2", "PD3", "PD4", "PD5", "PD6", "PD7", + "PD10", "PD11", "PD12", "PD13", "PD14", "PD15", + "PD18", "PD19", "PD20", "PD21", "PD22", "PD23", + "PD24", "PD25", "PD26", "PD27"; + allwinner,function = "lcd0"; + allwinner,drive = <SUN4I_PINCTRL_10_MA>; + allwinner,pull = <SUN4I_PINCTRL_NO_PULL>; + }; + uart1_pins_a: uart1@0 { allwinner,pins = "PE10", "PE11"; allwinner,function = "uart1";
On Mon, May 16, 2016 at 8:47 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
The RGB bus can be used in several configurations, one of which being the RGB666.
Add a pinctrl group for that case.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
Acked-by: Chen-Yu Tsai wens@csie.org
arch/arm/boot/dts/sun5i-a13.dtsi | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi index 263d46dbc7e6..79b5d513c142 100644 --- a/arch/arm/boot/dts/sun5i-a13.dtsi +++ b/arch/arm/boot/dts/sun5i-a13.dtsi @@ -237,6 +237,16 @@ &pio { compatible = "allwinner,sun5i-a13-pinctrl";
lcd_rgb666_pins: lcd_rgb666@0 {allwinner,pins = "PD2", "PD3", "PD4", "PD5", "PD6", "PD7","PD10", "PD11", "PD12", "PD13", "PD14", "PD15","PD18", "PD19", "PD20", "PD21", "PD22", "PD23","PD24", "PD25", "PD26", "PD27";allwinner,function = "lcd0";allwinner,drive = <SUN4I_PINCTRL_10_MA>;
Just a thought: would we ever need to increase the drive strength for very high resolutions?
allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;};uart1_pins_a: uart1@0 { allwinner,pins = "PE10", "PE11"; allwinner,function = "uart1";-- 2.8.2
On Tue, May 17, 2016 at 01:13:01AM +0800, Chen-Yu Tsai wrote:
arch/arm/boot/dts/sun5i-a13.dtsi | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi index 263d46dbc7e6..79b5d513c142 100644 --- a/arch/arm/boot/dts/sun5i-a13.dtsi +++ b/arch/arm/boot/dts/sun5i-a13.dtsi @@ -237,6 +237,16 @@ &pio { compatible = "allwinner,sun5i-a13-pinctrl";
lcd_rgb666_pins: lcd_rgb666@0 {allwinner,pins = "PD2", "PD3", "PD4", "PD5", "PD6", "PD7","PD10", "PD11", "PD12", "PD13", "PD14", "PD15","PD18", "PD19", "PD20", "PD21", "PD22", "PD23","PD24", "PD25", "PD26", "PD27";allwinner,function = "lcd0";allwinner,drive = <SUN4I_PINCTRL_10_MA>;Just a thought: would we ever need to increase the drive strength for very high resolutions?
I don't think we need to, but we can always change that later.
Applied, thanks! Maxime
Most of the display engine is shared between the R8 and the A13. Move the common parts to the Á13 DTSI.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- arch/arm/boot/dts/sun5i-a13.dtsi | 112 ++++++++++++++++++++++++++++++++++++ arch/arm/boot/dts/sun5i-r8.dtsi | 120 ++------------------------------------- 2 files changed, 117 insertions(+), 115 deletions(-)
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi index 79b5d513c142..e012890e0cf2 100644 --- a/arch/arm/boot/dts/sun5i-a13.dtsi +++ b/arch/arm/boot/dts/sun5i-a13.dtsi @@ -207,7 +207,50 @@ }; };
+ display-engine { + compatible = "allwinner,sun5i-a13-display-engine"; + allwinner,pipelines = <&fe0>; + }; + soc@01c00000 { + tcon0: lcd-controller@01c0c000 { + compatible = "allwinner,sun5i-a13-tcon"; + reg = <0x01c0c000 0x1000>; + interrupts = <44>; + resets = <&tcon_ch0_clk 1>; + reset-names = "lcd"; + clocks = <&ahb_gates 36>, + <&tcon_ch0_clk>, + <&tcon_ch1_clk>; + clock-names = "ahb", + "tcon-ch0", + "tcon-ch1"; + clock-output-names = "tcon-pixel-clock"; + status = "disabled"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + tcon0_in: port@0 { + #address-cells = <1>; + #size-cells = <0>; + reg = <0>; + + tcon0_in_be0: endpoint@0 { + reg = <0>; + remote-endpoint = <&be0_out_tcon0>; + }; + }; + + tcon0_out: port@1 { + #address-cells = <1>; + #size-cells = <0>; + reg = <1>; + }; + }; + }; + pwm: pwm@01c20e00 { compatible = "allwinner,sun5i-a13-pwm"; reg = <0x01c20e00 0xc>; @@ -215,6 +258,75 @@ #pwm-cells = <3>; status = "disabled"; }; + + fe0: display-frontend@01e00000 { + compatible = "allwinner,sun5i-a13-display-frontend"; + reg = <0x01e00000 0x20000>; + interrupts = <47>; + clocks = <&ahb_gates 46>, <&de_fe_clk>, + <&dram_gates 25>; + clock-names = "ahb", "mod", + "ram"; + resets = <&de_fe_clk>; + status = "disabled"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + fe0_out: port@1 { + #address-cells = <1>; + #size-cells = <0>; + reg = <1>; + + fe0_out_be0: endpoint@0 { + reg = <0>; + remote-endpoint = <&be0_in_fe0>; + }; + }; + }; + }; + + be0: display-backend@01e60000 { + compatible = "allwinner,sun5i-a13-display-backend"; + reg = <0x01e60000 0x10000>; + clocks = <&ahb_gates 44>, <&de_be_clk>, + <&dram_gates 26>; + clock-names = "ahb", "mod", + "ram"; + resets = <&de_be_clk>; + status = "disabled"; + + assigned-clocks = <&de_be_clk>; + assigned-clock-rates = <300000000>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + be0_in: port@0 { + #address-cells = <1>; + #size-cells = <0>; + reg = <0>; + + be0_in_fe0: endpoint@0 { + reg = <0>; + remote-endpoint = <&fe0_out_be0>; + }; + }; + + be0_out: port@1 { + #address-cells = <1>; + #size-cells = <0>; + reg = <1>; + + be0_out_tcon0: endpoint@0 { + reg = <0>; + remote-endpoint = <&tcon0_in_be0>; + }; + }; + }; + }; }; };
diff --git a/arch/arm/boot/dts/sun5i-r8.dtsi b/arch/arm/boot/dts/sun5i-r8.dtsi index c04cf690b858..8b058f53b7dc 100644 --- a/arch/arm/boot/dts/sun5i-r8.dtsi +++ b/arch/arm/boot/dts/sun5i-r8.dtsi @@ -76,122 +76,12 @@ }; }; }; - - tcon0: lcd-controller@01c0c000 { - compatible = "allwinner,sun5i-a13-tcon"; - reg = <0x01c0c000 0x1000>; - interrupts = <44>; - resets = <&tcon_ch0_clk 1>; - reset-names = "lcd"; - clocks = <&ahb_gates 36>, - <&tcon_ch0_clk>, - <&tcon_ch1_clk>; - clock-names = "ahb", - "tcon-ch0", - "tcon-ch1"; - clock-output-names = "tcon-pixel-clock"; - status = "disabled"; - - ports { - #address-cells = <1>; - #size-cells = <0>; - - tcon0_in: port@0 { - #address-cells = <1>; - #size-cells = <0>; - reg = <0>; - - tcon0_in_be0: endpoint@0 { - reg = <0>; - remote-endpoint = <&be0_out_tcon0>; - }; - }; - - tcon0_out: port@1 { - #address-cells = <1>; - #size-cells = <0>; - reg = <1>; - - tcon0_out_tve0: endpoint@1 { - reg = <1>; - remote-endpoint = <&tve0_in_tcon0>; - }; - }; - }; - }; - - fe0: display-frontend@01e00000 { - compatible = "allwinner,sun5i-a13-display-frontend"; - reg = <0x01e00000 0x20000>; - interrupts = <47>; - clocks = <&ahb_gates 46>, <&de_fe_clk>, - <&dram_gates 25>; - clock-names = "ahb", "mod", - "ram"; - resets = <&de_fe_clk>; - status = "disabled"; - - ports { - #address-cells = <1>; - #size-cells = <0>; - - fe0_out: port@1 { - #address-cells = <1>; - #size-cells = <0>; - reg = <1>; - - fe0_out_be0: endpoint@0 { - reg = <0>; - remote-endpoint = <&be0_in_fe0>; - }; - }; - }; - }; - - be0: display-backend@01e60000 { - compatible = "allwinner,sun5i-a13-display-backend"; - reg = <0x01e60000 0x10000>; - clocks = <&ahb_gates 44>, <&de_be_clk>, - <&dram_gates 26>; - clock-names = "ahb", "mod", - "ram"; - resets = <&de_be_clk>; - status = "disabled"; - - assigned-clocks = <&de_be_clk>; - assigned-clock-rates = <300000000>; - - ports { - #address-cells = <1>; - #size-cells = <0>; - - be0_in: port@0 { - #address-cells = <1>; - #size-cells = <0>; - reg = <0>; - - be0_in_fe0: endpoint@0 { - reg = <0>; - remote-endpoint = <&fe0_out_be0>; - }; - }; - - be0_out: port@1 { - #address-cells = <1>; - #size-cells = <0>; - reg = <1>; - - be0_out_tcon0: endpoint@0 { - reg = <0>; - remote-endpoint = <&tcon0_in_be0>; - }; - }; - }; - }; }; +};
- display-engine { - compatible = "allwinner,sun5i-a13-display-engine"; - allwinner,pipelines = <&fe0>; +&tcon0_out { + tcon0_out_tve0: endpoint@1 { + reg = <1>; + remote-endpoint = <&tve0_in_tcon0>; }; };
Now that we have support for the VGA bridges using our DRM driver, enable the display engine for the Olimex A13-Olinuxino.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- arch/arm/boot/dts/sun5i-a13-olinuxino.dts | 34 +++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/arch/arm/boot/dts/sun5i-a13-olinuxino.dts b/arch/arm/boot/dts/sun5i-a13-olinuxino.dts index b3c234c65ea1..b3298d231258 100644 --- a/arch/arm/boot/dts/sun5i-a13-olinuxino.dts +++ b/arch/arm/boot/dts/sun5i-a13-olinuxino.dts @@ -72,6 +72,27 @@ default-state = "on"; }; }; + + bridge { + compatible = "dumb-vga-bridge"; + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + #address-cells = <1>; + #size-cells = <0>; + + vga_input: endpoint@0 { + reg = <0>; + remote-endpoint = <&tcon0_out_vga>; + }; + }; + }; +}; + +&be0 { + status = "okay"; };
&ehci0 { @@ -211,6 +232,19 @@ status = "okay"; };
+&tcon0 { + pinctrl-names = "default"; + pinctrl-0 = <&lcd_rgb666_pins>; + status = "okay"; +}; + +&tcon0_out { + tcon0_out_vga: endpoint@0 { + reg = <0>; + remote-endpoint = <&vga_input>; + }; +}; + &uart1 { pinctrl-names = "default"; pinctrl-0 = <&uart1_pins_b>;
Enable the new DRM driver in the multi_v7 defconfig
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- arch/arm/configs/multi_v7_defconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig index f7e0d49b515a..80591f3e867e 100644 --- a/arch/arm/configs/multi_v7_defconfig +++ b/arch/arm/configs/multi_v7_defconfig @@ -552,6 +552,7 @@ CONFIG_DRM_ATMEL_HLCDC=m CONFIG_DRM_RCAR_DU=m CONFIG_DRM_RCAR_HDMI=y CONFIG_DRM_RCAR_LVDS=y +CONFIG_DRM_SUN4I=m CONFIG_DRM_TEGRA=y CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0=m CONFIG_DRM_PANEL_SIMPLE=y
On Mon, May 16, 2016 at 8:47 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Enable the new DRM driver in the multi_v7 defconfig
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
Acked-by: Chen-Yu Tsai wens@csie.org
Enable the RGB to VGA bridge driver in the defconfig
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- arch/arm/configs/multi_v7_defconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig index 80591f3e867e..5987688dea81 100644 --- a/arch/arm/configs/multi_v7_defconfig +++ b/arch/arm/configs/multi_v7_defconfig @@ -536,6 +536,7 @@ CONFIG_DRM=y CONFIG_DRM_I2C_ADV7511=m # CONFIG_DRM_I2C_CH7006 is not set # CONFIG_DRM_I2C_SIL164 is not set +CONFIG_DRM_DUMB_VGA=m CONFIG_DRM_NXP_PTN3460=m CONFIG_DRM_PARADE_PS8622=m CONFIG_DRM_NOUVEAU=m
Enable the new DRM driver in the sunxi default configuration
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- arch/arm/configs/sunxi_defconfig | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/arm/configs/sunxi_defconfig b/arch/arm/configs/sunxi_defconfig index e20ae3925e36..6a1447cf8feb 100644 --- a/arch/arm/configs/sunxi_defconfig +++ b/arch/arm/configs/sunxi_defconfig @@ -97,6 +97,8 @@ CONFIG_MEDIA_SUPPORT=y CONFIG_MEDIA_RC_SUPPORT=y CONFIG_RC_DEVICES=y CONFIG_IR_SUNXI=y +CONFIG_DRM=y +CONFIG_DRM_SUN4I=y CONFIG_FB=y CONFIG_FB_SIMPLE=y CONFIG_FRAMEBUFFER_CONSOLE=y
On Mon, May 16, 2016 at 8:47 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Enable the new DRM driver in the sunxi default configuration
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
Acked-by: Chen-Yu Tsai wens@csie.org
Enable the VGA bridge used on the A13-Olinuxino in the sunxi defconfig
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- arch/arm/configs/sunxi_defconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/sunxi_defconfig b/arch/arm/configs/sunxi_defconfig index 6a1447cf8feb..3676cc2db2eb 100644 --- a/arch/arm/configs/sunxi_defconfig +++ b/arch/arm/configs/sunxi_defconfig @@ -98,6 +98,7 @@ CONFIG_MEDIA_RC_SUPPORT=y CONFIG_RC_DEVICES=y CONFIG_IR_SUNXI=y CONFIG_DRM=y +CONFIG_DRM_DUMB_VGA=y CONFIG_DRM_SUN4I=y CONFIG_FB=y CONFIG_FB_SIMPLE=y
dri-devel@lists.freedesktop.org
-
Chen-Yu Tsai -
Daniel Vetter -
Laurent Pinchart -
Maxime Ripard -
Michael Turquette -
Rob Herring -
Russell King - ARM Linux