| Message ID | 20181007093905.11253-1-jernej.skrabec@siol.net |
|---|---|
| Headers | show |
| Series | Allwinner H6 DE3 and HDMI support | expand |
Dne nedelja, 07. oktober 2018 ob 11:38:36 CEST je Jernej Skrabec napisal(a): > This series adds support for Display Engine 3.0 and HDMI 2.0a, which > can be found on H6 SoC. > > Display Engine 3.0 in comparison to 2.0 mostly adds features needed for > displaying and processing 10-bit and AFBC formats, which are not yet > supported by this series. > > This series is based on linux-next at next-20180828, which has working > R40 display pipeline support. I'll rebase series on later linux-next, if > needed, once R40 display pipeline support is reintroduced. Sorry, I forgot to update above text. It is based on next-20181004 and R40 HDMI support is already fixed. Best regards, Jernej
On Sun, Oct 07, 2018 at 11:39:01AM +0200, Jernej Skrabec wrote: > From: Icenowy Zheng <icenowy@aosc.io> > > Some SoCs, such as H6, doesn't have a full-featured TCON TOP. > > Add quirks support for TCON TOP. > > Currently the presence of TCON_TV1 and DSI is controlled via the quirks > structure. > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > --- > drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 43 ++++++++++++++++++++------ > 1 file changed, 34 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > index 37158548b447..ed13233cad88 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > @@ -9,11 +9,17 @@ > #include <linux/component.h> > #include <linux/device.h> > #include <linux/module.h> > +#include <linux/of_device.h> > #include <linux/of_graph.h> > #include <linux/platform_device.h> > > #include "sun8i_tcon_top.h" > > +struct sun8i_tcon_top_quirks { > + bool has_tcon_tv1; > + bool has_dsi; > +}; > + > static bool sun8i_tcon_top_node_is_tcon_top(struct device_node *node) > { > return !!of_match_node(sun8i_tcon_top_of_table, node); > @@ -121,10 +127,13 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master, > struct platform_device *pdev = to_platform_device(dev); > struct clk_hw_onecell_data *clk_data; > struct sun8i_tcon_top *tcon_top; > + const struct sun8i_tcon_top_quirks *quirks; > struct resource *res; > void __iomem *regs; > int ret, i; > > + quirks = of_device_get_match_data(&pdev->dev); > + > tcon_top = devm_kzalloc(dev, sizeof(*tcon_top), GFP_KERNEL); > if (!tcon_top) > return -ENOMEM; > @@ -187,15 +196,23 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master, > &tcon_top->reg_lock, > TCON_TOP_TCON_TV0_GATE, 0); > > - clk_data->hws[CLK_TCON_TOP_TV1] = > - sun8i_tcon_top_register_gate(dev, "tcon-tv1", regs, > - &tcon_top->reg_lock, > - TCON_TOP_TCON_TV1_GATE, 1); > + if (quirks->has_tcon_tv1) { > + clk_data->hws[CLK_TCON_TOP_TV1] = > + sun8i_tcon_top_register_gate(dev, "tcon-tv1", regs, > + &tcon_top->reg_lock, > + TCON_TOP_TCON_TV1_GATE, 1); > + } else { > + clk_data->hws[CLK_TCON_TOP_TV1] = NULL; > + } > > - clk_data->hws[CLK_TCON_TOP_DSI] = > - sun8i_tcon_top_register_gate(dev, "dsi", regs, > - &tcon_top->reg_lock, > - TCON_TOP_TCON_DSI_GATE, 2); > + if (quirks->has_dsi) { > + clk_data->hws[CLK_TCON_TOP_DSI] = > + sun8i_tcon_top_register_gate(dev, "dsi", regs, > + &tcon_top->reg_lock, > + TCON_TOP_TCON_DSI_GATE, 2); > + } else { > + clk_data->hws[CLK_TCON_TOP_DSI] = NULL; clk_data has been kzalloc'd so its content is already NULL. And you shouldn't have brackets for single line blocks. with that fixed, Acked-by: Maxime Ripard <maxime.ripard@bootlin.com> Maxime
On Mon, Oct 8, 2018 at 4:51 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Sun, Oct 07, 2018 at 11:39:01AM +0200, Jernej Skrabec wrote: > > From: Icenowy Zheng <icenowy@aosc.io> > > > > Some SoCs, such as H6, doesn't have a full-featured TCON TOP. > > > > Add quirks support for TCON TOP. > > > > Currently the presence of TCON_TV1 and DSI is controlled via the quirks > > structure. > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > --- > > drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 43 ++++++++++++++++++++------ > > 1 file changed, 34 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > index 37158548b447..ed13233cad88 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > @@ -9,11 +9,17 @@ > > #include <linux/component.h> > > #include <linux/device.h> > > #include <linux/module.h> > > +#include <linux/of_device.h> > > #include <linux/of_graph.h> > > #include <linux/platform_device.h> > > > > #include "sun8i_tcon_top.h" > > > > +struct sun8i_tcon_top_quirks { > > + bool has_tcon_tv1; > > + bool has_dsi; > > +}; > > + > > static bool sun8i_tcon_top_node_is_tcon_top(struct device_node *node) > > { > > return !!of_match_node(sun8i_tcon_top_of_table, node); > > @@ -121,10 +127,13 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master, > > struct platform_device *pdev = to_platform_device(dev); > > struct clk_hw_onecell_data *clk_data; > > struct sun8i_tcon_top *tcon_top; > > + const struct sun8i_tcon_top_quirks *quirks; > > struct resource *res; > > void __iomem *regs; > > int ret, i; > > > > + quirks = of_device_get_match_data(&pdev->dev); > > + > > tcon_top = devm_kzalloc(dev, sizeof(*tcon_top), GFP_KERNEL); > > if (!tcon_top) > > return -ENOMEM; > > @@ -187,15 +196,23 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master, > > &tcon_top->reg_lock, > > TCON_TOP_TCON_TV0_GATE, 0); > > > > - clk_data->hws[CLK_TCON_TOP_TV1] = > > - sun8i_tcon_top_register_gate(dev, "tcon-tv1", regs, > > - &tcon_top->reg_lock, > > - TCON_TOP_TCON_TV1_GATE, 1); > > + if (quirks->has_tcon_tv1) { > > + clk_data->hws[CLK_TCON_TOP_TV1] = > > + sun8i_tcon_top_register_gate(dev, "tcon-tv1", regs, > > + &tcon_top->reg_lock, > > + TCON_TOP_TCON_TV1_GATE, 1); > > + } else { > > + clk_data->hws[CLK_TCON_TOP_TV1] = NULL; > > + } > > > > - clk_data->hws[CLK_TCON_TOP_DSI] = > > - sun8i_tcon_top_register_gate(dev, "dsi", regs, > > - &tcon_top->reg_lock, > > - TCON_TOP_TCON_DSI_GATE, 2); > > + if (quirks->has_dsi) { > > + clk_data->hws[CLK_TCON_TOP_DSI] = > > + sun8i_tcon_top_register_gate(dev, "dsi", regs, > > + &tcon_top->reg_lock, > > + TCON_TOP_TCON_DSI_GATE, 2); > > + } else { > > + clk_data->hws[CLK_TCON_TOP_DSI] = NULL; > > clk_data has been kzalloc'd so its content is already NULL. > > And you shouldn't have brackets for single line blocks. > > with that fixed, FYI checkpatch.pl complains if you use brackets for the if block but not for the else block. They should be matching. ChenYu > Acked-by: Maxime Ripard <maxime.ripard@bootlin.com> > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Hi, On Sun, Oct 07, 2018 at 11:38:54AM +0200, Jernej Skrabec wrote: > It turns out that H6 HDMI BSP kernel driver doesn't change TMDS rate at > all. At this point it is not clear whether it is just not necessary or > it would cause some kind of issues. > > Add a quirk for it. > > Reviewed-by: Chen-Yu Tsai <wens@csie.org> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 5 ++++- > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 + > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > index ec122136ee9d..e9e93f174b35 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > @@ -165,7 +165,9 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master, > goto err_disable_clk_tmds; > } > > - drm_encoder_helper_add(encoder, &sun8i_dw_hdmi_encoder_helper_funcs); > + if (hdmi->quirks->set_rate) > + drm_encoder_helper_add(encoder, > + &sun8i_dw_hdmi_encoder_helper_funcs); That seems a bit backward, it only works because we only have mode_set at the moment, and the only thing it does is changing the clock rate. As soon as we change one of these two assumptions, the code will break. Why not just return in mode_set if that boolean is true? > drm_encoder_init(drm, encoder, &sun8i_dw_hdmi_encoder_funcs, > DRM_MODE_ENCODER_TMDS, NULL); > > @@ -235,6 +237,7 @@ static int sun8i_dw_hdmi_remove(struct platform_device *pdev) > > static const struct sun8i_dw_hdmi_quirks sun8i_a83t_quirks = { > .mode_valid = sun8i_dw_hdmi_mode_valid_a83t, > + .set_rate = true, > }; > > static const struct of_device_id sun8i_dw_hdmi_dt_ids[] = { > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > index a645b8bc9f58..f9eb663865a4 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > @@ -173,6 +173,7 @@ struct sun8i_hdmi_phy { > struct sun8i_dw_hdmi_quirks { > enum drm_mode_status (*mode_valid)(struct drm_connector *connector, > const struct drm_display_mode *mode); > + bool set_rate; This triggers a check in checkpatch. You should address them (and there's several in your series). With both addressed, Acked-by: Maxime Ripard <maxime.ripard@bootlin.com> Thanks!
Hi, On Sun, Oct 07, 2018 at 11:38:46AM +0200, Jernej Skrabec wrote: > In preparation to introduce DE3 support, change prefix from "SUN8I_" to > "DE2_". Current prefix suggest that it's valid only for one family, > whereas in reality, DE2 unit is used also on sun50i family. > Additionally, it will be easier to distinguish DE3 specific macros by > using "DE3_" prefix. > > No functional change in this commit. I'm not too sure about this one. There's basically two ways to look at this: you described the first one, and the second one would be to treat it as we do for the compatibles: the IP was introduced on one SoC family, and then got used on some other ones. Trying to always match the one you have however have a quite big maintainance cost, as can be shown by your patch: you always have to adapt comments, function names, defines, etc. This creates a lot of useless churns (ie, non-functional changes) in the drivers, that need to be written in the first place, and then reviewed. It's just not worth it. Maxime
On Mon, Oct 08, 2018 at 05:06:45PM +0800, Chen-Yu Tsai wrote: > On Mon, Oct 8, 2018 at 4:51 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Sun, Oct 07, 2018 at 11:39:01AM +0200, Jernej Skrabec wrote: > > > From: Icenowy Zheng <icenowy@aosc.io> > > > > > > Some SoCs, such as H6, doesn't have a full-featured TCON TOP. > > > > > > Add quirks support for TCON TOP. > > > > > > Currently the presence of TCON_TV1 and DSI is controlled via the quirks > > > structure. > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > --- > > > drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 43 ++++++++++++++++++++------ > > > 1 file changed, 34 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > > index 37158548b447..ed13233cad88 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > > @@ -9,11 +9,17 @@ > > > #include <linux/component.h> > > > #include <linux/device.h> > > > #include <linux/module.h> > > > +#include <linux/of_device.h> > > > #include <linux/of_graph.h> > > > #include <linux/platform_device.h> > > > > > > #include "sun8i_tcon_top.h" > > > > > > +struct sun8i_tcon_top_quirks { > > > + bool has_tcon_tv1; > > > + bool has_dsi; > > > +}; > > > + > > > static bool sun8i_tcon_top_node_is_tcon_top(struct device_node *node) > > > { > > > return !!of_match_node(sun8i_tcon_top_of_table, node); > > > @@ -121,10 +127,13 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master, > > > struct platform_device *pdev = to_platform_device(dev); > > > struct clk_hw_onecell_data *clk_data; > > > struct sun8i_tcon_top *tcon_top; > > > + const struct sun8i_tcon_top_quirks *quirks; > > > struct resource *res; > > > void __iomem *regs; > > > int ret, i; > > > > > > + quirks = of_device_get_match_data(&pdev->dev); > > > + > > > tcon_top = devm_kzalloc(dev, sizeof(*tcon_top), GFP_KERNEL); > > > if (!tcon_top) > > > return -ENOMEM; > > > @@ -187,15 +196,23 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master, > > > &tcon_top->reg_lock, > > > TCON_TOP_TCON_TV0_GATE, 0); > > > > > > - clk_data->hws[CLK_TCON_TOP_TV1] = > > > - sun8i_tcon_top_register_gate(dev, "tcon-tv1", regs, > > > - &tcon_top->reg_lock, > > > - TCON_TOP_TCON_TV1_GATE, 1); > > > + if (quirks->has_tcon_tv1) { > > > + clk_data->hws[CLK_TCON_TOP_TV1] = > > > + sun8i_tcon_top_register_gate(dev, "tcon-tv1", regs, > > > + &tcon_top->reg_lock, > > > + TCON_TOP_TCON_TV1_GATE, 1); > > > + } else { > > > + clk_data->hws[CLK_TCON_TOP_TV1] = NULL; > > > + } > > > > > > - clk_data->hws[CLK_TCON_TOP_DSI] = > > > - sun8i_tcon_top_register_gate(dev, "dsi", regs, > > > - &tcon_top->reg_lock, > > > - TCON_TOP_TCON_DSI_GATE, 2); > > > + if (quirks->has_dsi) { > > > + clk_data->hws[CLK_TCON_TOP_DSI] = > > > + sun8i_tcon_top_register_gate(dev, "dsi", regs, > > > + &tcon_top->reg_lock, > > > + TCON_TOP_TCON_DSI_GATE, 2); > > > + } else { > > > + clk_data->hws[CLK_TCON_TOP_DSI] = NULL; > > > > clk_data has been kzalloc'd so its content is already NULL. > > > > And you shouldn't have brackets for single line blocks. > > > > with that fixed, > > FYI checkpatch.pl complains if you use brackets for the if block > but not for the else block. They should be matching. Checkpatch might not warn on this, but https://www.kernel.org/doc/Documentation/process/coding-style.rst, section 3 is pretty clear on whether we should use them or not. Maxime
On Mon, Oct 8, 2018 at 6:20 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Mon, Oct 08, 2018 at 05:06:45PM +0800, Chen-Yu Tsai wrote: > > On Mon, Oct 8, 2018 at 4:51 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > On Sun, Oct 07, 2018 at 11:39:01AM +0200, Jernej Skrabec wrote: > > > > From: Icenowy Zheng <icenowy@aosc.io> > > > > > > > > Some SoCs, such as H6, doesn't have a full-featured TCON TOP. > > > > > > > > Add quirks support for TCON TOP. > > > > > > > > Currently the presence of TCON_TV1 and DSI is controlled via the quirks > > > > structure. > > > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > > --- > > > > drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 43 ++++++++++++++++++++------ > > > > 1 file changed, 34 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > > > index 37158548b447..ed13233cad88 100644 > > > > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > > > @@ -9,11 +9,17 @@ > > > > #include <linux/component.h> > > > > #include <linux/device.h> > > > > #include <linux/module.h> > > > > +#include <linux/of_device.h> > > > > #include <linux/of_graph.h> > > > > #include <linux/platform_device.h> > > > > > > > > #include "sun8i_tcon_top.h" > > > > > > > > +struct sun8i_tcon_top_quirks { > > > > + bool has_tcon_tv1; > > > > + bool has_dsi; > > > > +}; > > > > + > > > > static bool sun8i_tcon_top_node_is_tcon_top(struct device_node *node) > > > > { > > > > return !!of_match_node(sun8i_tcon_top_of_table, node); > > > > @@ -121,10 +127,13 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master, > > > > struct platform_device *pdev = to_platform_device(dev); > > > > struct clk_hw_onecell_data *clk_data; > > > > struct sun8i_tcon_top *tcon_top; > > > > + const struct sun8i_tcon_top_quirks *quirks; > > > > struct resource *res; > > > > void __iomem *regs; > > > > int ret, i; > > > > > > > > + quirks = of_device_get_match_data(&pdev->dev); > > > > + > > > > tcon_top = devm_kzalloc(dev, sizeof(*tcon_top), GFP_KERNEL); > > > > if (!tcon_top) > > > > return -ENOMEM; > > > > @@ -187,15 +196,23 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master, > > > > &tcon_top->reg_lock, > > > > TCON_TOP_TCON_TV0_GATE, 0); > > > > > > > > - clk_data->hws[CLK_TCON_TOP_TV1] = > > > > - sun8i_tcon_top_register_gate(dev, "tcon-tv1", regs, > > > > - &tcon_top->reg_lock, > > > > - TCON_TOP_TCON_TV1_GATE, 1); > > > > + if (quirks->has_tcon_tv1) { > > > > + clk_data->hws[CLK_TCON_TOP_TV1] = > > > > + sun8i_tcon_top_register_gate(dev, "tcon-tv1", regs, > > > > + &tcon_top->reg_lock, > > > > + TCON_TOP_TCON_TV1_GATE, 1); > > > > + } else { > > > > + clk_data->hws[CLK_TCON_TOP_TV1] = NULL; > > > > + } > > > > > > > > - clk_data->hws[CLK_TCON_TOP_DSI] = > > > > - sun8i_tcon_top_register_gate(dev, "dsi", regs, > > > > - &tcon_top->reg_lock, > > > > - TCON_TOP_TCON_DSI_GATE, 2); > > > > + if (quirks->has_dsi) { > > > > + clk_data->hws[CLK_TCON_TOP_DSI] = > > > > + sun8i_tcon_top_register_gate(dev, "dsi", regs, > > > > + &tcon_top->reg_lock, > > > > + TCON_TOP_TCON_DSI_GATE, 2); > > > > + } else { > > > > + clk_data->hws[CLK_TCON_TOP_DSI] = NULL; > > > > > > clk_data has been kzalloc'd so its content is already NULL. > > > > > > And you shouldn't have brackets for single line blocks. > > > > > > with that fixed, > > > > FYI checkpatch.pl complains if you use brackets for the if block > > but not for the else block. They should be matching. > > Checkpatch might not warn on this, but > https://www.kernel.org/doc/Documentation/process/coding-style.rst, > section 3 is pretty clear on whether we should use them or not. Right. What I'm pointing out what checkpatch.pl complains about is shown in the second last example in section 3: This does not apply if only one branch of a conditional statement is a single statement; in the latter case use braces in both branches: Which is where I think your comment on "shouldn't have brackets for single line blocks" is pointing in the opposite direction. ChenYu
On Mon, Oct 08, 2018 at 06:50:44PM +0800, Chen-Yu Tsai wrote: > On Mon, Oct 8, 2018 at 6:20 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Mon, Oct 08, 2018 at 05:06:45PM +0800, Chen-Yu Tsai wrote: > > > On Mon, Oct 8, 2018 at 4:51 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > > > On Sun, Oct 07, 2018 at 11:39:01AM +0200, Jernej Skrabec wrote: > > > > > From: Icenowy Zheng <icenowy@aosc.io> > > > > > > > > > > Some SoCs, such as H6, doesn't have a full-featured TCON TOP. > > > > > > > > > > Add quirks support for TCON TOP. > > > > > > > > > > Currently the presence of TCON_TV1 and DSI is controlled via the quirks > > > > > structure. > > > > > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > > > --- > > > > > drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 43 ++++++++++++++++++++------ > > > > > 1 file changed, 34 insertions(+), 9 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > > > > index 37158548b447..ed13233cad88 100644 > > > > > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > > > > @@ -9,11 +9,17 @@ > > > > > #include <linux/component.h> > > > > > #include <linux/device.h> > > > > > #include <linux/module.h> > > > > > +#include <linux/of_device.h> > > > > > #include <linux/of_graph.h> > > > > > #include <linux/platform_device.h> > > > > > > > > > > #include "sun8i_tcon_top.h" > > > > > > > > > > +struct sun8i_tcon_top_quirks { > > > > > + bool has_tcon_tv1; > > > > > + bool has_dsi; > > > > > +}; > > > > > + > > > > > static bool sun8i_tcon_top_node_is_tcon_top(struct device_node *node) > > > > > { > > > > > return !!of_match_node(sun8i_tcon_top_of_table, node); > > > > > @@ -121,10 +127,13 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master, > > > > > struct platform_device *pdev = to_platform_device(dev); > > > > > struct clk_hw_onecell_data *clk_data; > > > > > struct sun8i_tcon_top *tcon_top; > > > > > + const struct sun8i_tcon_top_quirks *quirks; > > > > > struct resource *res; > > > > > void __iomem *regs; > > > > > int ret, i; > > > > > > > > > > + quirks = of_device_get_match_data(&pdev->dev); > > > > > + > > > > > tcon_top = devm_kzalloc(dev, sizeof(*tcon_top), GFP_KERNEL); > > > > > if (!tcon_top) > > > > > return -ENOMEM; > > > > > @@ -187,15 +196,23 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master, > > > > > &tcon_top->reg_lock, > > > > > TCON_TOP_TCON_TV0_GATE, 0); > > > > > > > > > > - clk_data->hws[CLK_TCON_TOP_TV1] = > > > > > - sun8i_tcon_top_register_gate(dev, "tcon-tv1", regs, > > > > > - &tcon_top->reg_lock, > > > > > - TCON_TOP_TCON_TV1_GATE, 1); > > > > > + if (quirks->has_tcon_tv1) { > > > > > + clk_data->hws[CLK_TCON_TOP_TV1] = > > > > > + sun8i_tcon_top_register_gate(dev, "tcon-tv1", regs, > > > > > + &tcon_top->reg_lock, > > > > > + TCON_TOP_TCON_TV1_GATE, 1); > > > > > + } else { > > > > > + clk_data->hws[CLK_TCON_TOP_TV1] = NULL; > > > > > + } > > > > > > > > > > - clk_data->hws[CLK_TCON_TOP_DSI] = > > > > > - sun8i_tcon_top_register_gate(dev, "dsi", regs, > > > > > - &tcon_top->reg_lock, > > > > > - TCON_TOP_TCON_DSI_GATE, 2); > > > > > + if (quirks->has_dsi) { > > > > > + clk_data->hws[CLK_TCON_TOP_DSI] = > > > > > + sun8i_tcon_top_register_gate(dev, "dsi", regs, > > > > > + &tcon_top->reg_lock, > > > > > + TCON_TOP_TCON_DSI_GATE, 2); > > > > > + } else { > > > > > + clk_data->hws[CLK_TCON_TOP_DSI] = NULL; > > > > > > > > clk_data has been kzalloc'd so its content is already NULL. > > > > > > > > And you shouldn't have brackets for single line blocks. > > > > > > > > with that fixed, > > > > > > FYI checkpatch.pl complains if you use brackets for the if block > > > but not for the else block. They should be matching. > > > > Checkpatch might not warn on this, but > > https://www.kernel.org/doc/Documentation/process/coding-style.rst, > > section 3 is pretty clear on whether we should use them or not. > > Right. What I'm pointing out what checkpatch.pl complains about is > shown in the second last example in section 3: > > This does not apply if only one branch of a conditional statement > is a single > statement; in the latter case use braces in both branches: > > Which is where I think your comment on "shouldn't have brackets for > single line blocks" > is pointing in the opposite direction. I think we have a communication failure :) The two blocks above are single line blocks, even though the line is wrapped. So whether or not there is an else condition or not doesn't matter, you shouldn't have braces at all. Maxime
On Mon, Oct 8, 2018 at 8:33 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Mon, Oct 08, 2018 at 06:50:44PM +0800, Chen-Yu Tsai wrote: > > On Mon, Oct 8, 2018 at 6:20 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > On Mon, Oct 08, 2018 at 05:06:45PM +0800, Chen-Yu Tsai wrote: > > > > On Mon, Oct 8, 2018 at 4:51 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > > > > > On Sun, Oct 07, 2018 at 11:39:01AM +0200, Jernej Skrabec wrote: > > > > > > From: Icenowy Zheng <icenowy@aosc.io> > > > > > > > > > > > > Some SoCs, such as H6, doesn't have a full-featured TCON TOP. > > > > > > > > > > > > Add quirks support for TCON TOP. > > > > > > > > > > > > Currently the presence of TCON_TV1 and DSI is controlled via the quirks > > > > > > structure. > > > > > > > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > > > > --- > > > > > > drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 43 ++++++++++++++++++++------ > > > > > > 1 file changed, 34 insertions(+), 9 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > > > > > index 37158548b447..ed13233cad88 100644 > > > > > > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > > > > > @@ -9,11 +9,17 @@ > > > > > > #include <linux/component.h> > > > > > > #include <linux/device.h> > > > > > > #include <linux/module.h> > > > > > > +#include <linux/of_device.h> > > > > > > #include <linux/of_graph.h> > > > > > > #include <linux/platform_device.h> > > > > > > > > > > > > #include "sun8i_tcon_top.h" > > > > > > > > > > > > +struct sun8i_tcon_top_quirks { > > > > > > + bool has_tcon_tv1; > > > > > > + bool has_dsi; > > > > > > +}; > > > > > > + > > > > > > static bool sun8i_tcon_top_node_is_tcon_top(struct device_node *node) > > > > > > { > > > > > > return !!of_match_node(sun8i_tcon_top_of_table, node); > > > > > > @@ -121,10 +127,13 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master, > > > > > > struct platform_device *pdev = to_platform_device(dev); > > > > > > struct clk_hw_onecell_data *clk_data; > > > > > > struct sun8i_tcon_top *tcon_top; > > > > > > + const struct sun8i_tcon_top_quirks *quirks; > > > > > > struct resource *res; > > > > > > void __iomem *regs; > > > > > > int ret, i; > > > > > > > > > > > > + quirks = of_device_get_match_data(&pdev->dev); > > > > > > + > > > > > > tcon_top = devm_kzalloc(dev, sizeof(*tcon_top), GFP_KERNEL); > > > > > > if (!tcon_top) > > > > > > return -ENOMEM; > > > > > > @@ -187,15 +196,23 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master, > > > > > > &tcon_top->reg_lock, > > > > > > TCON_TOP_TCON_TV0_GATE, 0); > > > > > > > > > > > > - clk_data->hws[CLK_TCON_TOP_TV1] = > > > > > > - sun8i_tcon_top_register_gate(dev, "tcon-tv1", regs, > > > > > > - &tcon_top->reg_lock, > > > > > > - TCON_TOP_TCON_TV1_GATE, 1); > > > > > > + if (quirks->has_tcon_tv1) { > > > > > > + clk_data->hws[CLK_TCON_TOP_TV1] = > > > > > > + sun8i_tcon_top_register_gate(dev, "tcon-tv1", regs, > > > > > > + &tcon_top->reg_lock, > > > > > > + TCON_TOP_TCON_TV1_GATE, 1); > > > > > > + } else { > > > > > > + clk_data->hws[CLK_TCON_TOP_TV1] = NULL; > > > > > > + } > > > > > > > > > > > > - clk_data->hws[CLK_TCON_TOP_DSI] = > > > > > > - sun8i_tcon_top_register_gate(dev, "dsi", regs, > > > > > > - &tcon_top->reg_lock, > > > > > > - TCON_TOP_TCON_DSI_GATE, 2); > > > > > > + if (quirks->has_dsi) { > > > > > > + clk_data->hws[CLK_TCON_TOP_DSI] = > > > > > > + sun8i_tcon_top_register_gate(dev, "dsi", regs, > > > > > > + &tcon_top->reg_lock, > > > > > > + TCON_TOP_TCON_DSI_GATE, 2); > > > > > > + } else { > > > > > > + clk_data->hws[CLK_TCON_TOP_DSI] = NULL; > > > > > > > > > > clk_data has been kzalloc'd so its content is already NULL. > > > > > > > > > > And you shouldn't have brackets for single line blocks. > > > > > > > > > > with that fixed, > > > > > > > > FYI checkpatch.pl complains if you use brackets for the if block > > > > but not for the else block. They should be matching. > > > > > > Checkpatch might not warn on this, but > > > https://www.kernel.org/doc/Documentation/process/coding-style.rst, > > > section 3 is pretty clear on whether we should use them or not. > > > > Right. What I'm pointing out what checkpatch.pl complains about is > > shown in the second last example in section 3: > > > > This does not apply if only one branch of a conditional statement > > is a single > > statement; in the latter case use braces in both branches: > > > > Which is where I think your comment on "shouldn't have brackets for > > single line blocks" > > is pointing in the opposite direction. > > I think we have a communication failure :) > > The two blocks above are single line blocks, even though the line is > wrapped. So whether or not there is an else condition or not doesn't > matter, you shouldn't have braces at all. Ah... It was a single line split wrapped to two lines... Sorry for the noise. ChenYu
Dne ponedeljek, 08. oktober 2018 ob 12:18:28 CEST je Maxime Ripard napisal(a): > Hi, > > On Sun, Oct 07, 2018 at 11:38:46AM +0200, Jernej Skrabec wrote: > > In preparation to introduce DE3 support, change prefix from "SUN8I_" to > > "DE2_". Current prefix suggest that it's valid only for one family, > > whereas in reality, DE2 unit is used also on sun50i family. > > Additionally, it will be easier to distinguish DE3 specific macros by > > using "DE3_" prefix. > > > > No functional change in this commit. > > I'm not too sure about this one. There's basically two ways to look at > this: you described the first one, and the second one would be to > treat it as we do for the compatibles: the IP was introduced on one > SoC family, and then got used on some other ones. > > Trying to always match the one you have however have a quite big > maintainance cost, as can be shown by your patch: you always have to > adapt comments, function names, defines, etc. This creates a lot of > useless churns (ie, non-functional changes) in the drivers, that need > to be written in the first place, and then reviewed. > > It's just not worth it. Well, using family neutral way would mean that there is no need anymore for adaptation in the future, but as you wish. What prefix should I use for DE3 specific registers? Best regards, Jernej
Dne ponedeljek, 08. oktober 2018 ob 10:51:12 CEST je Maxime Ripard napisal(a): > On Sun, Oct 07, 2018 at 11:39:01AM +0200, Jernej Skrabec wrote: > > From: Icenowy Zheng <icenowy@aosc.io> > > > > Some SoCs, such as H6, doesn't have a full-featured TCON TOP. > > > > Add quirks support for TCON TOP. > > > > Currently the presence of TCON_TV1 and DSI is controlled via the quirks > > structure. > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > --- > > > > drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 43 ++++++++++++++++++++------ > > 1 file changed, 34 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c index 37158548b447..ed13233cad88 > > 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > > @@ -9,11 +9,17 @@ > > > > #include <linux/component.h> > > #include <linux/device.h> > > #include <linux/module.h> > > > > +#include <linux/of_device.h> > > > > #include <linux/of_graph.h> > > #include <linux/platform_device.h> > > > > #include "sun8i_tcon_top.h" > > > > +struct sun8i_tcon_top_quirks { > > + bool has_tcon_tv1; > > + bool has_dsi; > > +}; > > + > > > > static bool sun8i_tcon_top_node_is_tcon_top(struct device_node *node) > > { > > > > return !!of_match_node(sun8i_tcon_top_of_table, node); > > > > @@ -121,10 +127,13 @@ static int sun8i_tcon_top_bind(struct device *dev, > > struct device *master,> > > struct platform_device *pdev = to_platform_device(dev); > > struct clk_hw_onecell_data *clk_data; > > struct sun8i_tcon_top *tcon_top; > > > > + const struct sun8i_tcon_top_quirks *quirks; > > > > struct resource *res; > > void __iomem *regs; > > int ret, i; > > > > + quirks = of_device_get_match_data(&pdev->dev); > > + > > > > tcon_top = devm_kzalloc(dev, sizeof(*tcon_top), GFP_KERNEL); > > if (!tcon_top) > > > > return -ENOMEM; > > > > @@ -187,15 +196,23 @@ static int sun8i_tcon_top_bind(struct device *dev, > > struct device *master,> > > &tcon_top->reg_lock, > > TCON_TOP_TCON_TV0_GATE, 0); > > > > - clk_data->hws[CLK_TCON_TOP_TV1] = > > - sun8i_tcon_top_register_gate(dev, "tcon-tv1", regs, > > - &tcon_top->reg_lock, > > - TCON_TOP_TCON_TV1_GATE, 1); > > + if (quirks->has_tcon_tv1) { > > + clk_data->hws[CLK_TCON_TOP_TV1] = > > + sun8i_tcon_top_register_gate(dev, "tcon-tv1", regs, > > + &tcon_top->reg_lock, > > + TCON_TOP_TCON_TV1_GATE, 1); > > + } else { > > + clk_data->hws[CLK_TCON_TOP_TV1] = NULL; > > + } > > > > - clk_data->hws[CLK_TCON_TOP_DSI] = > > - sun8i_tcon_top_register_gate(dev, "dsi", regs, > > - &tcon_top->reg_lock, > > - TCON_TOP_TCON_DSI_GATE, 2); > > + if (quirks->has_dsi) { > > + clk_data->hws[CLK_TCON_TOP_DSI] = > > + sun8i_tcon_top_register_gate(dev, "dsi", regs, > > + &tcon_top->reg_lock, > > + TCON_TOP_TCON_DSI_GATE, 2); > > + } else { > > + clk_data->hws[CLK_TCON_TOP_DSI] = NULL; > > clk_data has been kzalloc'd so its content is already NULL. > > And you shouldn't have brackets for single line blocks. Ah, yes. I'm not original author so I missed that during a review. I'll fix it in new revision. Best regards, Jernej > > with that fixed, > > Acked-by: Maxime Ripard <maxime.ripard@bootlin.com> > > Maxime
Dne ponedeljek, 08. oktober 2018 ob 11:14:06 CEST je Maxime Ripard napisal(a): > Hi, > > On Sun, Oct 07, 2018 at 11:38:54AM +0200, Jernej Skrabec wrote: > > It turns out that H6 HDMI BSP kernel driver doesn't change TMDS rate at > > all. At this point it is not clear whether it is just not necessary or > > it would cause some kind of issues. > > > > Add a quirk for it. > > > > Reviewed-by: Chen-Yu Tsai <wens@csie.org> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 5 ++++- > > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 + > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > > b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c index ec122136ee9d..e9e93f174b35 > > 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > > @@ -165,7 +165,9 @@ static int sun8i_dw_hdmi_bind(struct device *dev, > > struct device *master,> > > goto err_disable_clk_tmds; > > > > } > > > > - drm_encoder_helper_add(encoder, &sun8i_dw_hdmi_encoder_helper_funcs); > > + if (hdmi->quirks->set_rate) > > + drm_encoder_helper_add(encoder, > > + &sun8i_dw_hdmi_encoder_helper_funcs); > > That seems a bit backward, it only works because we only have mode_set > at the moment, and the only thing it does is changing the clock > rate. As soon as we change one of these two assumptions, the code will > break. > > Why not just return in mode_set if that boolean is true? My original intention was to optimize execution time a bit. If there is no helpers registered, there is no need to call callback which does nothing. But your way is probably more reasonable approach. However, I'm pretty sure that even older HDMI controller doesn't need this rate changing code. All tests shown that changing HDMI controller divider doesn't have an effect on video or audio output. It's only there because BSP kernel driver set rate and AW engineer said it's necessary. > > > drm_encoder_init(drm, encoder, &sun8i_dw_hdmi_encoder_funcs, > > > > DRM_MODE_ENCODER_TMDS, NULL); > > > > @@ -235,6 +237,7 @@ static int sun8i_dw_hdmi_remove(struct platform_device > > *pdev)> > > static const struct sun8i_dw_hdmi_quirks sun8i_a83t_quirks = { > > > > .mode_valid = sun8i_dw_hdmi_mode_valid_a83t, > > > > + .set_rate = true, > > > > }; > > > > static const struct of_device_id sun8i_dw_hdmi_dt_ids[] = { > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > > b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index a645b8bc9f58..f9eb663865a4 > > 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > > @@ -173,6 +173,7 @@ struct sun8i_hdmi_phy { > > > > struct sun8i_dw_hdmi_quirks { > > > > enum drm_mode_status (*mode_valid)(struct drm_connector *connector, > > > > const struct drm_display_mode *mode); > > > > + bool set_rate; > > This triggers a check in checkpatch. You should address them (and > there's several in your series). Till now we used bools in driver internal structures in spite of this warnings. So should we start using unsigned int? Or maybe bitfield with unsigned int as a base according to https://lkml.org/lkml/2017/11/21/384 As far as other issues reported by strict checkpatch go, most of them are DT lines over 80 charachters. This was tolerated till now. Last type of issues is that macro parameters weren't enclosed with braces. These issues won't be reported anymore in new series because sun8i_csc.h won't be touched if I drop patch which renames DE2 register macros (but original issue will remain). Best regards, Jernej > > With both addressed, > Acked-by: Maxime Ripard <maxime.ripard@bootlin.com> > > Thanks!
On Mon, Oct 08, 2018 at 05:09:42PM +0200, Jernej Škrabec wrote: > Dne ponedeljek, 08. oktober 2018 ob 11:14:06 CEST je Maxime Ripard napisal(a): > > Hi, > > > > On Sun, Oct 07, 2018 at 11:38:54AM +0200, Jernej Skrabec wrote: > > > It turns out that H6 HDMI BSP kernel driver doesn't change TMDS rate at > > > all. At this point it is not clear whether it is just not necessary or > > > it would cause some kind of issues. > > > > > > Add a quirk for it. > > > > > > Reviewed-by: Chen-Yu Tsai <wens@csie.org> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > --- > > > > > > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 5 ++++- > > > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 + > > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > > > b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c index ec122136ee9d..e9e93f174b35 > > > 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > > > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > > > @@ -165,7 +165,9 @@ static int sun8i_dw_hdmi_bind(struct device *dev, > > > struct device *master,> > > > goto err_disable_clk_tmds; > > > > > > } > > > > > > - drm_encoder_helper_add(encoder, &sun8i_dw_hdmi_encoder_helper_funcs); > > > + if (hdmi->quirks->set_rate) > > > + drm_encoder_helper_add(encoder, > > > + &sun8i_dw_hdmi_encoder_helper_funcs); > > > > That seems a bit backward, it only works because we only have mode_set > > at the moment, and the only thing it does is changing the clock > > rate. As soon as we change one of these two assumptions, the code will > > break. > > > > Why not just return in mode_set if that boolean is true? > > My original intention was to optimize execution time a bit. If there is no > helpers registered, there is no need to call callback which does nothing. But > your way is probably more reasonable approach. > > However, I'm pretty sure that even older HDMI controller doesn't need this > rate changing code. All tests shown that changing HDMI controller divider > doesn't have an effect on video or audio output. It's only there because BSP > kernel driver set rate and AW engineer said it's necessary. Maybe we can simply remove it then, and see if it breaks. Given the feedback from Allwinner, I'd prefer to have the behaviour they recommend though, at least from MMC experience, it proved to be needed. > > > > > drm_encoder_init(drm, encoder, &sun8i_dw_hdmi_encoder_funcs, > > > > > > DRM_MODE_ENCODER_TMDS, NULL); > > > > > > @@ -235,6 +237,7 @@ static int sun8i_dw_hdmi_remove(struct platform_device > > > *pdev)> > > > static const struct sun8i_dw_hdmi_quirks sun8i_a83t_quirks = { > > > > > > .mode_valid = sun8i_dw_hdmi_mode_valid_a83t, > > > > > > + .set_rate = true, > > > > > > }; > > > > > > static const struct of_device_id sun8i_dw_hdmi_dt_ids[] = { > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > > > b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index a645b8bc9f58..f9eb663865a4 > > > 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > > > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > > > @@ -173,6 +173,7 @@ struct sun8i_hdmi_phy { > > > > > > struct sun8i_dw_hdmi_quirks { > > > > > > enum drm_mode_status (*mode_valid)(struct drm_connector *connector, > > > > > > const struct drm_display_mode *mode); > > > > > > + bool set_rate; > > > > This triggers a check in checkpatch. You should address them (and > > there's several in your series). > > Till now we used bools in driver internal structures in spite of this > warnings. So should we start using unsigned int? Or maybe bitfield with > unsigned int as a base according to https://lkml.org/lkml/2017/11/21/384 That warning has been introduced pretty recently, so we do indeed have some drivers that use a bool in their structure. > As far as other issues reported by strict checkpatch go, most of them are DT > lines over 80 charachters. This was tolerated till now. Last type of issues is > that macro parameters weren't enclosed with braces. These issues won't be > reported anymore in new series because sun8i_csc.h won't be touched if I drop > patch which renames DE2 register macros (but original issue will remain). Yeah, I don't worry about those. Thanks! Maxime
On Mon, Oct 08, 2018 at 04:28:41PM +0200, Jernej Škrabec wrote: > Dne ponedeljek, 08. oktober 2018 ob 12:18:28 CEST je Maxime Ripard napisal(a): > > Hi, > > > > On Sun, Oct 07, 2018 at 11:38:46AM +0200, Jernej Skrabec wrote: > > > In preparation to introduce DE3 support, change prefix from "SUN8I_" to > > > "DE2_". Current prefix suggest that it's valid only for one family, > > > whereas in reality, DE2 unit is used also on sun50i family. > > > Additionally, it will be easier to distinguish DE3 specific macros by > > > using "DE3_" prefix. > > > > > > No functional change in this commit. > > > > I'm not too sure about this one. There's basically two ways to look at > > this: you described the first one, and the second one would be to > > treat it as we do for the compatibles: the IP was introduced on one > > SoC family, and then got used on some other ones. > > > > Trying to always match the one you have however have a quite big > > maintainance cost, as can be shown by your patch: you always have to > > adapt comments, function names, defines, etc. This creates a lot of > > useless churns (ie, non-functional changes) in the drivers, that need > > to be written in the first place, and then reviewed. > > > > It's just not worth it. > > Well, using family neutral way would mean that there is no need > anymore for adaptation in the future, but as you wish. If you consider only the DE case, where it has been numbered by design, yes. If you consider all the other controllers that haven't been (like, SPI, or the USB PHY, for example), then you end up with two drivers that are using the same names, with no easy way to tell which is which > What prefix should I use for DE3 specific registers? We should just follow the same pattern, and use the first family where it was introduced. So sun50i, I guess? Maxime
Hi Jernej, Thank you for the patch. On Sunday, 7 October 2018 12:38:51 EEST Jernej Skrabec wrote: > It turns out that even new DW HDMI controllers exhibits same magenta > line issues as older versions. > > Enable workaround for v2.12a. This doesn't affect the platforms I maintain, so I can't really test this, but I'm wondering whether there could be other platforms using a v2.12a DW HDMI that wouldn't need the workaround. My platforms use a previous version, namely v2.01a. The workaround for that version has been enabled by commit 9c305eb442f3b371fc722ade827bbf673514123e Author: Neil Armstrong <narmstrong@baylibre.com> Date: Fri Feb 23 12:44:37 2018 +0100 drm: bridge: dw-hdmi: Fix overflow workaround for Amlogic Meson GX SoCs I haven't paid too much attention to the patch back then, and have now double- checked the HDMI output on R-Car Gen3. Enabling the workaround doesn't cause any regression, and reverting the commit doesn't cause any issue either. I thus wonder whether we shouldn't enable the workaround with count = 1 in the default case instead of adding new IP core versions to the list. It would be nice if someone from Synopsys could comment on this. > Reviewed-by: Chen-Yu Tsai <wens@csie.org> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > 5971976284bf..df1c7a2d6961 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -1664,6 +1664,7 @@ static void dw_hdmi_clear_overflow(struct dw_hdmi > *hdmi) case 0x131a: > case 0x132a: > case 0x201a: > + case 0x212a: > count = 1; > break; > default:
On Tue, Oct 9, 2018 at 1:40 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Jernej, > > Thank you for the patch. > > On Sunday, 7 October 2018 12:38:51 EEST Jernej Skrabec wrote: > > It turns out that even new DW HDMI controllers exhibits same magenta > > line issues as older versions. > > > > Enable workaround for v2.12a. > > This doesn't affect the platforms I maintain, so I can't really test this, but > I'm wondering whether there could be other platforms using a v2.12a DW HDMI > that wouldn't need the workaround. > > My platforms use a previous version, namely v2.01a. The workaround for that > version has been enabled by > > commit 9c305eb442f3b371fc722ade827bbf673514123e > Author: Neil Armstrong <narmstrong@baylibre.com> > Date: Fri Feb 23 12:44:37 2018 +0100 > > drm: bridge: dw-hdmi: Fix overflow workaround for Amlogic Meson GX SoCs > > I haven't paid too much attention to the patch back then, and have now double- > checked the HDMI output on R-Car Gen3. Enabling the workaround doesn't cause > any regression, and reverting the commit doesn't cause any issue either. I > thus wonder whether we shouldn't enable the workaround with count = 1 in the > default case instead of adding new IP core versions to the list. It would be > nice if someone from Synopsys could comment on this. I hope this comment isn't *incredibly* off-topic, but we encountered a similar issue with NVIDIA (and I believe AMD) hardware a while back, related to HDMI. This was due to infoframes not being sent, but (perhaps) HDMI Audio being enabled. This was a single vertical(!) line. It was described as "purple", but not sure that's distinguishable from "magenta" by most people. [ Fixed by a522946174 on nouveau, sample bug report https://bugs.freedesktop.org/show_bug.cgi?id=79912 ] Cheers, -ilia
On Tue, Oct 09, 2018 at 01:56:04PM -0400, Ilia Mirkin wrote: > On Tue, Oct 9, 2018 at 1:40 PM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > commit 9c305eb442f3b371fc722ade827bbf673514123e > > Author: Neil Armstrong <narmstrong@baylibre.com> > > Date: Fri Feb 23 12:44:37 2018 +0100 > > > > drm: bridge: dw-hdmi: Fix overflow workaround for Amlogic Meson GX SoCs > > > > I haven't paid too much attention to the patch back then, and have now double- > > checked the HDMI output on R-Car Gen3. Enabling the workaround doesn't cause > > any regression, and reverting the commit doesn't cause any issue either. I > > thus wonder whether we shouldn't enable the workaround with count = 1 in the > > default case instead of adding new IP core versions to the list. It would be > > nice if someone from Synopsys could comment on this. > > I hope this comment isn't *incredibly* off-topic, but we encountered a > similar issue with NVIDIA (and I believe AMD) hardware a while back, > related to HDMI. This was due to infoframes not being sent, but > (perhaps) HDMI Audio being enabled. You're probably right about the infoframes. The errata documentation for this workaround on iMX6 states: Each time one writes to some FC registers, and depending on the clock relation of sfr clk and tmds clk, some of these train of pulses (when these registers are configured in sequence), may not be caught by the arithmetic unit while it is busy processing/updating the first ones, so, it gets wrong video timing values, although the registers FC_* hold correct values. Even a soft reset will not make the arithmetic unit update correctly. Video will still pass correctly to the HDMI, but packets would not because the frame composer is holding internally incorrect video timing and this will quickly build up and overflow the packet FIFOs. So, the workaround is about kicking the frame composer so that the packets (iow, non-video data) are passed through correctly.
On Sunday 07 October 2018 03:08 PM, Jernej Skrabec wrote: > Video PLL factors can be set in a way that final PLL rate is outside > stable range. H6 user manual specifically says that N factor should not > be below 12. While it doesn't says anything about maximum stable rate, it Manual says "In application, PLL_FACTOR_N should be more than or equal to 11" can't it be 11? > is clear that PLL doesn't work at 6.096 GHz (254 * 24 MHz). > > Set minimum allowed PLL video rate to 288 MHz (12 * 24 MHz) and maximum > to 2.4 GHz, which is maximum in BSP driver. Is this max freq from here [1] [1] https://github.com/Allwinner-Homlet/H6-BSP4.9-linux/blob/master/drivers/clk/sunxi/clk-sun50iw6.c#L164
On Fri, Oct 12, 2018 at 4:14 PM Jagan Teki <jagan@openedev.com> wrote: > > On Sunday 07 October 2018 03:08 PM, Jernej Skrabec wrote: > > Video PLL factors can be set in a way that final PLL rate is outside > > stable range. H6 user manual specifically says that N factor should not > > be below 12. While it doesn't says anything about maximum stable rate, it > > Manual says "In application, PLL_FACTOR_N should be more than or equal > to 11" can't it be 11? PLL_FACTOR_N is the raw value, which starts from 0. That translates to an actual factor of 12 used in the calculations. > > is clear that PLL doesn't work at 6.096 GHz (254 * 24 MHz). > > > > Set minimum allowed PLL video rate to 288 MHz (12 * 24 MHz) and maximum > > to 2.4 GHz, which is maximum in BSP driver. > > Is this max freq from here [1] > > [1] > https://github.com/Allwinner-Homlet/H6-BSP4.9-linux/blob/master/drivers/clk/sunxi/clk-sun50iw6.c#L164 > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout.
Hi, Dne torek, 09. oktober 2018 ob 19:40:44 CEST je Laurent Pinchart napisal(a): > Hi Jernej, > > Thank you for the patch. > > On Sunday, 7 October 2018 12:38:51 EEST Jernej Skrabec wrote: > > It turns out that even new DW HDMI controllers exhibits same magenta > > line issues as older versions. > > > > Enable workaround for v2.12a. > > This doesn't affect the platforms I maintain, so I can't really test this, > but I'm wondering whether there could be other platforms using a v2.12a DW > HDMI that wouldn't need the workaround. > > My platforms use a previous version, namely v2.01a. The workaround for that > version has been enabled by > > commit 9c305eb442f3b371fc722ade827bbf673514123e > Author: Neil Armstrong <narmstrong@baylibre.com> > Date: Fri Feb 23 12:44:37 2018 +0100 > > drm: bridge: dw-hdmi: Fix overflow workaround for Amlogic Meson GX SoCs > > I haven't paid too much attention to the patch back then, and have now > double- checked the HDMI output on R-Car Gen3. Enabling the workaround > doesn't cause any regression, and reverting the commit doesn't cause any > issue either. I thus wonder whether we shouldn't enable the workaround with > count = 1 in the default case instead of adding new IP core versions to the > list. It would be nice if someone from Synopsys could comment on this. I was thinking about that too, or even having parameter in dw_hdmi_plat_data which would tell how many times to repeat workaround procedure for a specific platform. But this might be an overkill. Best regards, Jernej > > > Reviewed-by: Chen-Yu Tsai <wens@csie.org> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > > 5971976284bf..df1c7a2d6961 100644 > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > @@ -1664,6 +1664,7 @@ static void dw_hdmi_clear_overflow(struct dw_hdmi > > > > *hdmi) case 0x131a: > > case 0x132a: > > > > case 0x201a: > > + case 0x212a: > > count = 1; > > break; > > > > default: