[PATCH v2 0/9] drm/sun4i: Allwinner R40 MIPI-DSI support

[PATCH v2 0/9] drm/sun4i: Allwinner R40 MIPI-DSI support

[Jagan Teki]

This is v2 version for supporting MIPI-DSI on Allwinner R40 from
initial version[4].

The controller look similar like, Allwinnwe A64 so it has
dependencies with A64 MIPI DSI changes, DSI fixes and R40 pwm 
driver [1] [2] [3]. 

PLL-MIPI, dclk divders logic bpp/lanes are properly work for R40,
like A31, A64.

Changes for v2:
- drop tcon top lcd clock patch
- add TODO text while adding tcon lcd support
- add patch for registering tcon top clock gates in probe
- change tcon-ch0 in tcon_lcd0 to CLK_TCON_LCD0
- change mod clock in dphy to tcon_top with index 3 

[1] https://patchwork.freedesktop.org/series/61310/
[2] https://patchwork.freedesktop.org/series/60847/ 
[3] https://lore.kernel.org/patchwork/cover/862766/ 
[4] https://patchwork.freedesktop.org/series/62062/

Any inputs?
Jagan.

Jagan Teki (9):
  dt-bindings: display: Add TCON LCD compatible for R40
  drm/sun4i: tcon: Add TCON LCD support for R40
  ARM: dts: sun8i: r40: Use tcon top clock index macros
  drm/sun4i: tcon_top: Use clock name index macros
  drm/sun4i: tcon_top: Register clock gates in probe
  dt-bindings: sun6i-dsi: Add R40 MIPI-DSI compatible (w/ A64 fallback)
  dt-bindings: sun6i-dsi: Add R40 DPHY compatible (w/ A31 fallback)
  ARM: dts: sun8i: r40: Add MIPI DSI pipeline
  [DO NOT MERGE] ARM: dts: sun8i-r40: bananapi-m2-ultra: Enable Bananapi S070WV20-CT16 DSI panel

 .../bindings/display/sunxi/sun4i-drm.txt      |   1 +
 .../bindings/display/sunxi/sun6i-dsi.txt      |   2 +
 .../boot/dts/sun8i-r40-bananapi-m2-ultra.dts  |  36 ++++++
 arch/arm/boot/dts/sun8i-r40.dtsi              |  78 ++++++++++++-
 drivers/gpu/drm/sun4i/sun4i_tcon.c            |   8 ++
 drivers/gpu/drm/sun4i/sun8i_tcon_top.c        | 103 ++++++++++--------
 6 files changed, 178 insertions(+), 50 deletions(-)

-- 
2.18.0.321.gffc6fa0e3

[PATCH v2 1/9] dt-bindings: display: Add TCON LCD compatible for R40

[Jagan Teki]

Like TCON TV0, TV1 allwinner R40 has TCON LCD0, LCD1 which
are managed via TCON TOP.

Add tcon lcd compatible R40, the same compatible can handle
TCON LCD0, LCD1.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
---
 Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
index 31ab72cba3d4..9e9c7f934202 100644
--- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
+++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
@@ -160,6 +160,7 @@ Required properties:
    * allwinner,sun8i-a33-tcon
    * allwinner,sun8i-a83t-tcon-lcd
    * allwinner,sun8i-a83t-tcon-tv
+   * allwinner,sun8i-r40-tcon-lcd
    * allwinner,sun8i-r40-tcon-tv
    * allwinner,sun8i-v3s-tcon
    * allwinner,sun9i-a80-tcon-lcd
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH v2 1/9] dt-bindings: display: Add TCON LCD compatible for R40

[Rob Herring]

On Fri, 14 Jun 2019 22:13:16 +0530, Jagan Teki wrote:
> Like TCON TV0, TV1 allwinner R40 has TCON LCD0, LCD1 which
> are managed via TCON TOP.
> 
> Add tcon lcd compatible R40, the same compatible can handle
> TCON LCD0, LCD1.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> Acked-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v2 2/9] drm/sun4i: tcon: Add TCON LCD support for R40

[Jagan Teki]

TCON LCD0, LCD1 in allwinner R40, are used for managing
LCD interfaces like RGB, LVDS and DSI.

Like TCON TV0, TV1 these LCD0, LCD1 are also managed via
tcon top.

Add support for it, in tcon driver.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 9e9d08ee8387..9838913305a0 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -1471,6 +1471,13 @@ static const struct sun4i_tcon_quirks sun8i_a83t_tv_quirks = {
 	.has_channel_1		= true,
 };
 
+static const struct sun4i_tcon_quirks sun8i_r40_lcd_quirks = {
+	.supports_lvds		= true,
+	.has_channel_0		= true,
+	/* TODO Need to support TCON output muxing via GPIO pins */
+	.set_mux		= sun8i_r40_tcon_tv_set_mux,
+};
+
 static const struct sun4i_tcon_quirks sun8i_r40_tv_quirks = {
 	.has_channel_1		= true,
 	.set_mux		= sun8i_r40_tcon_tv_set_mux,
@@ -1501,6 +1508,7 @@ const struct of_device_id sun4i_tcon_of_table[] = {
 	{ .compatible = "allwinner,sun8i-a33-tcon", .data = &sun8i_a33_quirks },
 	{ .compatible = "allwinner,sun8i-a83t-tcon-lcd", .data = &sun8i_a83t_lcd_quirks },
 	{ .compatible = "allwinner,sun8i-a83t-tcon-tv", .data = &sun8i_a83t_tv_quirks },
+	{ .compatible = "allwinner,sun8i-r40-tcon-lcd", .data = &sun8i_r40_lcd_quirks },
 	{ .compatible = "allwinner,sun8i-r40-tcon-tv", .data = &sun8i_r40_tv_quirks },
 	{ .compatible = "allwinner,sun8i-v3s-tcon", .data = &sun8i_v3s_quirks },
 	{ .compatible = "allwinner,sun9i-a80-tcon-lcd", .data = &sun9i_a80_tcon_lcd_quirks },
-- 
2.18.0.321.gffc6fa0e3

[PATCH v2 5/9] drm/sun4i: tcon_top: Register clock gates in probe

[Jagan Teki]

TCON TOP have clock gates for TV0, TV1, dsi and right
now these are register during bind call.

Of which, dsi clock gate would required during DPHY probe
but same can miss to get since tcon top is not bound at
that time.

To solve, this circular dependency move the clock gate
registration from bind to probe so-that DPHY can get the
dsi gate clock on time.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 94 ++++++++++++++------------
 1 file changed, 49 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
index 465e9b0cdfee..a8978b3fe851 100644
--- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
+++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
@@ -124,7 +124,53 @@ static struct clk_hw *sun8i_tcon_top_register_gate(struct device *dev,
 static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
 			       void *data)
 {
-	struct platform_device *pdev = to_platform_device(dev);
+	struct sun8i_tcon_top *tcon_top = dev_get_drvdata(dev);
+	int ret;
+
+	ret = reset_control_deassert(tcon_top->rst);
+	if (ret) {
+		dev_err(dev, "Could not deassert ctrl reset control\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(tcon_top->bus);
+	if (ret) {
+		dev_err(dev, "Could not enable bus clock\n");
+		goto err_assert_reset;
+	}
+
+	return 0;
+
+err_assert_reset:
+	reset_control_assert(tcon_top->rst);
+
+	return ret;
+}
+
+static void sun8i_tcon_top_unbind(struct device *dev, struct device *master,
+				  void *data)
+{
+	struct sun8i_tcon_top *tcon_top = dev_get_drvdata(dev);
+	struct clk_hw_onecell_data *clk_data = tcon_top->clk_data;
+	int i;
+
+	of_clk_del_provider(dev->of_node);
+	for (i = 0; i < CLK_NUM; i++)
+		if (clk_data->hws[i])
+			clk_hw_unregister_gate(clk_data->hws[i]);
+
+	clk_disable_unprepare(tcon_top->bus);
+	reset_control_assert(tcon_top->rst);
+}
+
+static const struct component_ops sun8i_tcon_top_ops = {
+	.bind	= sun8i_tcon_top_bind,
+	.unbind	= sun8i_tcon_top_unbind,
+};
+
+static int sun8i_tcon_top_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
 	struct clk_hw_onecell_data *clk_data;
 	struct sun8i_tcon_top *tcon_top;
 	const struct sun8i_tcon_top_quirks *quirks;
@@ -132,7 +178,7 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
 	void __iomem *regs;
 	int ret, i;
 
-	quirks = of_device_get_match_data(&pdev->dev);
+	quirks = of_device_get_match_data(dev);
 
 	tcon_top = devm_kzalloc(dev, sizeof(*tcon_top), GFP_KERNEL);
 	if (!tcon_top)
@@ -164,18 +210,6 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
 	if (IS_ERR(regs))
 		return PTR_ERR(regs);
 
-	ret = reset_control_deassert(tcon_top->rst);
-	if (ret) {
-		dev_err(dev, "Could not deassert ctrl reset control\n");
-		return ret;
-	}
-
-	ret = clk_prepare_enable(tcon_top->bus);
-	if (ret) {
-		dev_err(dev, "Could not enable bus clock\n");
-		goto err_assert_reset;
-	}
-
 	/*
 	 * At least on H6, some registers have some bits set by default
 	 * which may cause issues. Clear them here.
@@ -226,45 +260,15 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
 
 	dev_set_drvdata(dev, tcon_top);
 
-	return 0;
+	return component_add(dev, &sun8i_tcon_top_ops);
 
 err_unregister_gates:
 	for (i = 0; i < CLK_NUM; i++)
 		if (!IS_ERR_OR_NULL(clk_data->hws[i]))
 			clk_hw_unregister_gate(clk_data->hws[i]);
-	clk_disable_unprepare(tcon_top->bus);
-err_assert_reset:
-	reset_control_assert(tcon_top->rst);
-
 	return ret;
 }
 
-static void sun8i_tcon_top_unbind(struct device *dev, struct device *master,
-				  void *data)
-{
-	struct sun8i_tcon_top *tcon_top = dev_get_drvdata(dev);
-	struct clk_hw_onecell_data *clk_data = tcon_top->clk_data;
-	int i;
-
-	of_clk_del_provider(dev->of_node);
-	for (i = 0; i < CLK_NUM; i++)
-		if (clk_data->hws[i])
-			clk_hw_unregister_gate(clk_data->hws[i]);
-
-	clk_disable_unprepare(tcon_top->bus);
-	reset_control_assert(tcon_top->rst);
-}
-
-static const struct component_ops sun8i_tcon_top_ops = {
-	.bind	= sun8i_tcon_top_bind,
-	.unbind	= sun8i_tcon_top_unbind,
-};
-
-static int sun8i_tcon_top_probe(struct platform_device *pdev)
-{
-	return component_add(&pdev->dev, &sun8i_tcon_top_ops);
-}
-
 static int sun8i_tcon_top_remove(struct platform_device *pdev)
 {
 	component_del(&pdev->dev, &sun8i_tcon_top_ops);
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH v2 5/9] drm/sun4i: tcon_top: Register clock gates in probe

[Chen-Yu Tsai]

On Sat, Jun 15, 2019 at 12:44 AM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
>
> TCON TOP have clock gates for TV0, TV1, dsi and right
> now these are register during bind call.
>
> Of which, dsi clock gate would required during DPHY probe
> but same can miss to get since tcon top is not bound at
> that time.
>
> To solve, this circular dependency move the clock gate
> registration from bind to probe so-that DPHY can get the
> dsi gate clock on time.
>
> Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> ---
>  drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 94 ++++++++++++++------------
>  1 file changed, 49 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> index 465e9b0cdfee..a8978b3fe851 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> @@ -124,7 +124,53 @@ static struct clk_hw *sun8i_tcon_top_register_gate(struct device *dev,
>  static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
>                                void *data)
>  {
> -       struct platform_device *pdev = to_platform_device(dev);
> +       struct sun8i_tcon_top *tcon_top = dev_get_drvdata(dev);
> +       int ret;
> +
> +       ret = reset_control_deassert(tcon_top->rst);
> +       if (ret) {
> +               dev_err(dev, "Could not deassert ctrl reset control\n");
> +               return ret;
> +       }
> +
> +       ret = clk_prepare_enable(tcon_top->bus);
> +       if (ret) {
> +               dev_err(dev, "Could not enable bus clock\n");
> +               goto err_assert_reset;
> +       }

You have to de-assert the reset control and enable the clock before the
clocks it provides are registered. Otherwise a consumer may come in and
ask for the provided clock to be enabled, but since the TCON TOP's own
reset and clock are still disabled, you can't actually access the registers
that controls the provided clock.

> +
> +       return 0;
> +
> +err_assert_reset:
> +       reset_control_assert(tcon_top->rst);
> +
> +       return ret;
> +}
> +
> +static void sun8i_tcon_top_unbind(struct device *dev, struct device *master,
> +                                 void *data)
> +{
> +       struct sun8i_tcon_top *tcon_top = dev_get_drvdata(dev);
> +       struct clk_hw_onecell_data *clk_data = tcon_top->clk_data;
> +       int i;
> +
> +       of_clk_del_provider(dev->of_node);
> +       for (i = 0; i < CLK_NUM; i++)
> +               if (clk_data->hws[i])
> +                       clk_hw_unregister_gate(clk_data->hws[i]);

And this should be in the remove function.

So instead, just move _everything_ to the probe and remove functions,
and provide empty bind/unbind functions so the component system still
works.

ChenYu

> +
> +       clk_disable_unprepare(tcon_top->bus);
> +       reset_control_assert(tcon_top->rst);
> +}
> +
> +static const struct component_ops sun8i_tcon_top_ops = {
> +       .bind   = sun8i_tcon_top_bind,
> +       .unbind = sun8i_tcon_top_unbind,
> +};
> +
> +static int sun8i_tcon_top_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
>         struct clk_hw_onecell_data *clk_data;
>         struct sun8i_tcon_top *tcon_top;
>         const struct sun8i_tcon_top_quirks *quirks;
> @@ -132,7 +178,7 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
>         void __iomem *regs;
>         int ret, i;
>
> -       quirks = of_device_get_match_data(&pdev->dev);
> +       quirks = of_device_get_match_data(dev);
>
>         tcon_top = devm_kzalloc(dev, sizeof(*tcon_top), GFP_KERNEL);
>         if (!tcon_top)
> @@ -164,18 +210,6 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
>         if (IS_ERR(regs))
>                 return PTR_ERR(regs);
>
> -       ret = reset_control_deassert(tcon_top->rst);
> -       if (ret) {
> -               dev_err(dev, "Could not deassert ctrl reset control\n");
> -               return ret;
> -       }
> -
> -       ret = clk_prepare_enable(tcon_top->bus);
> -       if (ret) {
> -               dev_err(dev, "Could not enable bus clock\n");
> -               goto err_assert_reset;
> -       }
> -
>         /*
>          * At least on H6, some registers have some bits set by default
>          * which may cause issues. Clear them here.
> @@ -226,45 +260,15 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
>
>         dev_set_drvdata(dev, tcon_top);
>
> -       return 0;
> +       return component_add(dev, &sun8i_tcon_top_ops);
>
>  err_unregister_gates:
>         for (i = 0; i < CLK_NUM; i++)
>                 if (!IS_ERR_OR_NULL(clk_data->hws[i]))
>                         clk_hw_unregister_gate(clk_data->hws[i]);
> -       clk_disable_unprepare(tcon_top->bus);
> -err_assert_reset:
> -       reset_control_assert(tcon_top->rst);
> -
>         return ret;
>  }
>
> -static void sun8i_tcon_top_unbind(struct device *dev, struct device *master,
> -                                 void *data)
> -{
> -       struct sun8i_tcon_top *tcon_top = dev_get_drvdata(dev);
> -       struct clk_hw_onecell_data *clk_data = tcon_top->clk_data;
> -       int i;
> -
> -       of_clk_del_provider(dev->of_node);
> -       for (i = 0; i < CLK_NUM; i++)
> -               if (clk_data->hws[i])
> -                       clk_hw_unregister_gate(clk_data->hws[i]);
> -
> -       clk_disable_unprepare(tcon_top->bus);
> -       reset_control_assert(tcon_top->rst);
> -}
> -
> -static const struct component_ops sun8i_tcon_top_ops = {
> -       .bind   = sun8i_tcon_top_bind,
> -       .unbind = sun8i_tcon_top_unbind,
> -};
> -
> -static int sun8i_tcon_top_probe(struct platform_device *pdev)
> -{
> -       return component_add(&pdev->dev, &sun8i_tcon_top_ops);
> -}
> -
>  static int sun8i_tcon_top_remove(struct platform_device *pdev)
>  {
>         component_del(&pdev->dev, &sun8i_tcon_top_ops);
> --
> 2.18.0.321.gffc6fa0e3
>
> --
> 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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190614164324.9427-6-jagan%40amarulasolutions.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v2 5/9] drm/sun4i: tcon_top: Register clock gates in probe

[Jagan Teki]

On Sun, Jun 16, 2019 at 11:01 AM Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
>
> On Sat, Jun 15, 2019 at 12:44 AM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
> >
> > TCON TOP have clock gates for TV0, TV1, dsi and right
> > now these are register during bind call.
> >
> > Of which, dsi clock gate would required during DPHY probe
> > but same can miss to get since tcon top is not bound at
> > that time.
> >
> > To solve, this circular dependency move the clock gate
> > registration from bind to probe so-that DPHY can get the
> > dsi gate clock on time.
> >
> > Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> > ---
> >  drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 94 ++++++++++++++------------
> >  1 file changed, 49 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > index 465e9b0cdfee..a8978b3fe851 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > @@ -124,7 +124,53 @@ static struct clk_hw *sun8i_tcon_top_register_gate(struct device *dev,
> >  static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
> >                                void *data)
> >  {
> > -       struct platform_device *pdev = to_platform_device(dev);
> > +       struct sun8i_tcon_top *tcon_top = dev_get_drvdata(dev);
> > +       int ret;
> > +
> > +       ret = reset_control_deassert(tcon_top->rst);
> > +       if (ret) {
> > +               dev_err(dev, "Could not deassert ctrl reset control\n");
> > +               return ret;
> > +       }
> > +
> > +       ret = clk_prepare_enable(tcon_top->bus);
> > +       if (ret) {
> > +               dev_err(dev, "Could not enable bus clock\n");
> > +               goto err_assert_reset;
> > +       }
>
> You have to de-assert the reset control and enable the clock before the
> clocks it provides are registered. Otherwise a consumer may come in and
> ask for the provided clock to be enabled, but since the TCON TOP's own
> reset and clock are still disabled, you can't actually access the registers
> that controls the provided clock.

These rst and bus are common reset and bus clocks not tcon top clocks
that are trying to register here. ie reason I have not moved it in
top.

Re: [PATCH v2 5/9] drm/sun4i: tcon_top: Register clock gates in probe

[Chen-Yu Tsai]

On Mon, Jun 17, 2019 at 6:30 PM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
>
> On Sun, Jun 16, 2019 at 11:01 AM Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> >
> > On Sat, Jun 15, 2019 at 12:44 AM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
> > >
> > > TCON TOP have clock gates for TV0, TV1, dsi and right
> > > now these are register during bind call.
> > >
> > > Of which, dsi clock gate would required during DPHY probe
> > > but same can miss to get since tcon top is not bound at
> > > that time.
> > >
> > > To solve, this circular dependency move the clock gate
> > > registration from bind to probe so-that DPHY can get the
> > > dsi gate clock on time.
> > >
> > > Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> > > ---
> > >  drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 94 ++++++++++++++------------
> > >  1 file changed, 49 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > index 465e9b0cdfee..a8978b3fe851 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > @@ -124,7 +124,53 @@ static struct clk_hw *sun8i_tcon_top_register_gate(struct device *dev,
> > >  static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
> > >                                void *data)
> > >  {
> > > -       struct platform_device *pdev = to_platform_device(dev);
> > > +       struct sun8i_tcon_top *tcon_top = dev_get_drvdata(dev);
> > > +       int ret;
> > > +
> > > +       ret = reset_control_deassert(tcon_top->rst);
> > > +       if (ret) {
> > > +               dev_err(dev, "Could not deassert ctrl reset control\n");
> > > +               return ret;
> > > +       }
> > > +
> > > +       ret = clk_prepare_enable(tcon_top->bus);
> > > +       if (ret) {
> > > +               dev_err(dev, "Could not enable bus clock\n");
> > > +               goto err_assert_reset;
> > > +       }
> >
> > You have to de-assert the reset control and enable the clock before the
> > clocks it provides are registered. Otherwise a consumer may come in and
> > ask for the provided clock to be enabled, but since the TCON TOP's own
> > reset and clock are still disabled, you can't actually access the registers
> > that controls the provided clock.
>
> These rst and bus are common reset and bus clocks not tcon top clocks
> that are trying to register here. ie reason I have not moved it in
> top.

And you're sure that toggling bits in the TCON TOP block doesn't require
the reset to be de-asserted and the bus clock enabled?

Somehow I doubt that.

Once the driver register the clocks it provides, they absolutely must work.
They can't only work after the bind phase when the reset gets de-asserted
and the bus clock enabled. Or you should provide proper error reporting
in the clock ops. I doubt you want to go that way either.

ChenYu

Re: [linux-sunxi] [PATCH v2 5/9] drm/sun4i: tcon_top: Register clock gates in probe

[Jagan Teki]

On Tue, Jun 18, 2019 at 12:49 PM Chen-Yu Tsai <wens@csie.org> wrote:
>
> On Mon, Jun 17, 2019 at 6:30 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Sun, Jun 16, 2019 at 11:01 AM Chen-Yu Tsai <wens@csie.org> wrote:
> > >
> > > On Sat, Jun 15, 2019 at 12:44 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > >
> > > > TCON TOP have clock gates for TV0, TV1, dsi and right
> > > > now these are register during bind call.
> > > >
> > > > Of which, dsi clock gate would required during DPHY probe
> > > > but same can miss to get since tcon top is not bound at
> > > > that time.
> > > >
> > > > To solve, this circular dependency move the clock gate
> > > > registration from bind to probe so-that DPHY can get the
> > > > dsi gate clock on time.
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > ---
> > > >  drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 94 ++++++++++++++------------
> > > >  1 file changed, 49 insertions(+), 45 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > index 465e9b0cdfee..a8978b3fe851 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > @@ -124,7 +124,53 @@ static struct clk_hw *sun8i_tcon_top_register_gate(struct device *dev,
> > > >  static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
> > > >                                void *data)
> > > >  {
> > > > -       struct platform_device *pdev = to_platform_device(dev);
> > > > +       struct sun8i_tcon_top *tcon_top = dev_get_drvdata(dev);
> > > > +       int ret;
> > > > +
> > > > +       ret = reset_control_deassert(tcon_top->rst);
> > > > +       if (ret) {
> > > > +               dev_err(dev, "Could not deassert ctrl reset control\n");
> > > > +               return ret;
> > > > +       }
> > > > +
> > > > +       ret = clk_prepare_enable(tcon_top->bus);
> > > > +       if (ret) {
> > > > +               dev_err(dev, "Could not enable bus clock\n");
> > > > +               goto err_assert_reset;
> > > > +       }
> > >
> > > You have to de-assert the reset control and enable the clock before the
> > > clocks it provides are registered. Otherwise a consumer may come in and
> > > ask for the provided clock to be enabled, but since the TCON TOP's own
> > > reset and clock are still disabled, you can't actually access the registers
> > > that controls the provided clock.
> >
> > These rst and bus are common reset and bus clocks not tcon top clocks
> > that are trying to register here. ie reason I have not moved it in
> > top.
>
> And you're sure that toggling bits in the TCON TOP block doesn't require
> the reset to be de-asserted and the bus clock enabled?
>
> Somehow I doubt that.
>
> Once the driver register the clocks it provides, they absolutely must work.
> They can't only work after the bind phase when the reset gets de-asserted
> and the bus clock enabled. Or you should provide proper error reporting
> in the clock ops. I doubt you want to go that way either.

Why would they won't work after bind phase? unlike tcon top gates,
these reset, and bus are common like  what we have in other DE block
so enable them in bind won't be an issue as per as I understand. let
me know if you want me to check in other directions.

Log:
[    1.381410] sun6i-mipi-dsi 1ca0000.dsi: Attached panel s070wv20-ct16-icn62
[    1.398405] sun4i-drm display-engine: bound 1100000.mixer (ops 0xc074ce64)
[    1.407134] sun4i-drm display-engine: bound 1200000.mixer (ops 0xc074ce64)
[    1.414043] sun4i-drm display-engine: bound 1c70000.tcon-top (ops 0xc0750e80)
[    1.421407] sun4i_dclk_recalc_rate: val = 1, rate = 297000000
[    1.427358] sun4i-drm display-engine: No panel or bridge found...
RGB output disabled
[    1.435217] sun4i-drm display-engine: bound 1c71000.lcd-controller
(ops 0xc0749594)
[    1.442891] 0.0 drm_connector_init
[    1.446294] 0. -1066106880-1-0 (null)
[    1.449965] 0.1 drm_connector_init
[    1.453368] 0.2 drm_connector_init
[    1.456768] 1. drm_connector_init
[    1.460094] 2. drm_connector_init
[    1.463413] drm_connector_init: connector name = DSI-1
[    1.468560] sun4i-drm display-engine: bound 1ca0000.dsi (ops 0xc074c0e4)
[    1.475272] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
[    1.481892] [drm] No driver support for vblank timestamp query.
[    1.488240] [drm] Initialized sun4i-drm 1.0.0 20150629 for
display-engine on minor 0
[    1.497996] sun4i_dclk_round_rate: min_div = 6 max_div = 6, rate = 30000000
[    1.498106] ideal = 1800000, rounded = 180000000
[    1.498111] sun4i_dclk_round_rate: div = 6 rate = 29700000
[    1.498116] sun4i_dclk_round_rate: min_div = 6 max_div = 6, rate = 30000000
[    1.498154] ideal = 1800000, rounded = 180000000
[    1.498158] sun4i_dclk_round_rate: div = 6 rate = 29700000
[    1.498217] sun4i_dclk_recalc_rate: val = 1, rate = 178200000
[    1.498251] rate = 178200000
[    1.498253] parent_rate = 297000000
[    1.498256] reg = 0x80c00000
[    1.498259] _nkm.n = 3, nkm->n.offset = 0x1, nkm->n.shift = 8
[    1.498262] _nkm.k = 2, nkm->k.offset = 0x1, nkm->k.shift = 4
[    1.498265] _nkm.m = 10, nkm->m.offset = 0x1, nkm->m.shift = 0
[    1.499594] sun4i_dclk_set_rate div 6
[    1.499603] sun4i_dclk_recalc_rate: val = 6, rate = 29700000
[    1.499680] sun6i_dsi_get_video_start_delay: delay = 513
[    1.499687] sun6i_dsi_setup_inst_loop: delay = 49
[    1.499706] hsa = 134, hbp = 114, hfp = 114, hblk = 2630, vblk = 0
[    1.509103] mmc0: host does not support reading read-only switch,
assuming write-enable
[    1.512693] mmc0: new high speed SDHC card at address 4001
[    1.513723] mmcblk0: mmc0:4001 R04GS 3.71 GiB
[    1.515577]  mmcblk0: p1 p2
[    1.658838] mmc2: new DDR MMC card at address 0001
[    1.659934] mmcblk2: mmc2:0001 8WPD3R 7.28 GiB
[    1.660657] mmcblk2boot0: mmc2:0001 8WPD3R partition 1 4.00 MiB
[    1.661387] mmcblk2boot1: mmc2:0001 8WPD3R partition 2 4.00 MiB
[    1.819586] TYPE#0x23, BYTE0#0x7a00, BYTE1#0xc10000
[    1.849570] TYPE#0x23, BYTE0#0x2000, BYTE1#0x200000
[    1.879569] TYPE#0x23, BYTE0#0x2100, BYTE1#0xe00000
[    1.909580] TYPE#0x23, BYTE0#0x2200, BYTE1#0x130000
[    1.939569] TYPE#0x23, BYTE0#0x2300, BYTE1#0x280000
[    1.969569] TYPE#0x23, BYTE0#0x2400, BYTE1#0x300000
[    1.999569] TYPE#0x23, BYTE0#0x2500, BYTE1#0x280000
[    2.029569] TYPE#0x23, BYTE0#0x2600, BYTE1#0x0
[    2.059569] TYPE#0x23, BYTE0#0x2700, BYTE1#0xd0000
[    2.089598] TYPE#0x23, BYTE0#0x2800, BYTE1#0x30000
[    2.119579] TYPE#0x23, BYTE0#0x2900, BYTE1#0x1d0000
[    2.149569] TYPE#0x23, BYTE0#0x3400, BYTE1#0x800000
[    2.179569] TYPE#0x23, BYTE0#0x3600, BYTE1#0x280000
[    2.209569] TYPE#0x23, BYTE0#0xb500, BYTE1#0xa00000
[    2.239569] TYPE#0x23, BYTE0#0x5c00, BYTE1#0xff0000
[    2.269569] TYPE#0x23, BYTE0#0x2a00, BYTE1#0x10000
[    2.299569] TYPE#0x23, BYTE0#0x5600, BYTE1#0x920000
[    2.329578] TYPE#0x23, BYTE0#0x6b00, BYTE1#0x710000
[    2.359569] TYPE#0x23, BYTE0#0x6900, BYTE1#0x2b0000
[    2.389569] TYPE#0x23, BYTE0#0x1000, BYTE1#0x400000
[    2.419569] TYPE#0x23, BYTE0#0x1100, BYTE1#0x980000
[    2.449569] TYPE#0x23, BYTE0#0xb600, BYTE1#0x200000
[    2.479569] TYPE#0x23, BYTE0#0x5100, BYTE1#0x200000
[    2.509569] TYPE#0x23, BYTE0#0x900, BYTE1#0x100000
[    2.679570] TYPE#0x5, BYTE0#0x2900, BYTE1#0x940000
[    2.767213] Console: switching to colour frame buffer device 100x30
[    3.144604] sun4i-drm display-engine: fb0: sun4i-drmdrmfb frame buffer device

Re: [PATCH v2 5/9] drm/sun4i: tcon_top: Register clock gates in probe

[Chen-Yu Tsai]

On Tue, Jun 18, 2019 at 3:45 PM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
>
> On Tue, Jun 18, 2019 at 12:49 PM Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> >
> > On Mon, Jun 17, 2019 at 6:30 PM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
> > >
> > > On Sun, Jun 16, 2019 at 11:01 AM Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> > > >
> > > > On Sat, Jun 15, 2019 at 12:44 AM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
> > > > >
> > > > > TCON TOP have clock gates for TV0, TV1, dsi and right
> > > > > now these are register during bind call.
> > > > >
> > > > > Of which, dsi clock gate would required during DPHY probe
> > > > > but same can miss to get since tcon top is not bound at
> > > > > that time.
> > > > >
> > > > > To solve, this circular dependency move the clock gate
> > > > > registration from bind to probe so-that DPHY can get the
> > > > > dsi gate clock on time.
> > > > >
> > > > > Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> > > > > ---
> > > > >  drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 94 ++++++++++++++------------
> > > > >  1 file changed, 49 insertions(+), 45 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > > index 465e9b0cdfee..a8978b3fe851 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > > @@ -124,7 +124,53 @@ static struct clk_hw *sun8i_tcon_top_register_gate(struct device *dev,
> > > > >  static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
> > > > >                                void *data)
> > > > >  {
> > > > > -       struct platform_device *pdev = to_platform_device(dev);
> > > > > +       struct sun8i_tcon_top *tcon_top = dev_get_drvdata(dev);
> > > > > +       int ret;
> > > > > +
> > > > > +       ret = reset_control_deassert(tcon_top->rst);
> > > > > +       if (ret) {
> > > > > +               dev_err(dev, "Could not deassert ctrl reset control\n");
> > > > > +               return ret;
> > > > > +       }
> > > > > +
> > > > > +       ret = clk_prepare_enable(tcon_top->bus);
> > > > > +       if (ret) {
> > > > > +               dev_err(dev, "Could not enable bus clock\n");
> > > > > +               goto err_assert_reset;
> > > > > +       }
> > > >
> > > > You have to de-assert the reset control and enable the clock before the
> > > > clocks it provides are registered. Otherwise a consumer may come in and
> > > > ask for the provided clock to be enabled, but since the TCON TOP's own
> > > > reset and clock are still disabled, you can't actually access the registers
> > > > that controls the provided clock.
> > >
> > > These rst and bus are common reset and bus clocks not tcon top clocks
> > > that are trying to register here. ie reason I have not moved it in
> > > top.
> >
> > And you're sure that toggling bits in the TCON TOP block doesn't require
> > the reset to be de-asserted and the bus clock enabled?
> >
> > Somehow I doubt that.
> >
> > Once the driver register the clocks it provides, they absolutely must work.
> > They can't only work after the bind phase when the reset gets de-asserted
> > and the bus clock enabled. Or you should provide proper error reporting
> > in the clock ops. I doubt you want to go that way either.
>
> Why would they won't work after bind phase? unlike tcon top gates,
> these reset, and bus are common like  what we have in other DE block
> so enable them in bind won't be an issue as per as I understand. let
> me know if you want me to check in other directions.

You misunderstood. When you moved the clock registering parts to the probe
phase, but didn't move the clock enable and reset de-assert parts to go with,
the clock ops will not work as expected between probe and bind time.

Simple way to verify it: Just use devmem to disable the TCON TOP bus gate
and/or assert its reset control. Then try to toggle any of the bits in the
TCON TOP block and see if it works, or if the bits stick.

Whether another driver actually does so is not the question. It is just bad
implementation.

> Log:
> [    1.381410] sun6i-mipi-dsi 1ca0000.dsi: Attached panel s070wv20-ct16-icn62
> [    1.398405] sun4i-drm display-engine: bound 1100000.mixer (ops 0xc074ce64)
> [    1.407134] sun4i-drm display-engine: bound 1200000.mixer (ops 0xc074ce64)
> [    1.414043] sun4i-drm display-engine: bound 1c70000.tcon-top (ops 0xc0750e80)
> [    1.421407] sun4i_dclk_recalc_rate: val = 1, rate = 297000000
> [    1.427358] sun4i-drm display-engine: No panel or bridge found...
> RGB output disabled
> [    1.435217] sun4i-drm display-engine: bound 1c71000.lcd-controller
> (ops 0xc0749594)
> [    1.442891] 0.0 drm_connector_init
> [    1.446294] 0. -1066106880-1-0 (null)
> [    1.449965] 0.1 drm_connector_init
> [    1.453368] 0.2 drm_connector_init
> [    1.456768] 1. drm_connector_init
> [    1.460094] 2. drm_connector_init
> [    1.463413] drm_connector_init: connector name = DSI-1
> [    1.468560] sun4i-drm display-engine: bound 1ca0000.dsi (ops 0xc074c0e4)
> [    1.475272] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> [    1.481892] [drm] No driver support for vblank timestamp query.
> [    1.488240] [drm] Initialized sun4i-drm 1.0.0 20150629 for
> display-engine on minor 0
> [    1.497996] sun4i_dclk_round_rate: min_div = 6 max_div = 6, rate = 30000000
> [    1.498106] ideal = 1800000, rounded = 180000000
> [    1.498111] sun4i_dclk_round_rate: div = 6 rate = 29700000
> [    1.498116] sun4i_dclk_round_rate: min_div = 6 max_div = 6, rate = 30000000
> [    1.498154] ideal = 1800000, rounded = 180000000
> [    1.498158] sun4i_dclk_round_rate: div = 6 rate = 29700000
> [    1.498217] sun4i_dclk_recalc_rate: val = 1, rate = 178200000
> [    1.498251] rate = 178200000
> [    1.498253] parent_rate = 297000000
> [    1.498256] reg = 0x80c00000
> [    1.498259] _nkm.n = 3, nkm->n.offset = 0x1, nkm->n.shift = 8
> [    1.498262] _nkm.k = 2, nkm->k.offset = 0x1, nkm->k.shift = 4
> [    1.498265] _nkm.m = 10, nkm->m.offset = 0x1, nkm->m.shift = 0
> [    1.499594] sun4i_dclk_set_rate div 6
> [    1.499603] sun4i_dclk_recalc_rate: val = 6, rate = 29700000
> [    1.499680] sun6i_dsi_get_video_start_delay: delay = 513
> [    1.499687] sun6i_dsi_setup_inst_loop: delay = 49
> [    1.499706] hsa = 134, hbp = 114, hfp = 114, hblk = 2630, vblk = 0
> [    1.509103] mmc0: host does not support reading read-only switch,
> assuming write-enable
> [    1.512693] mmc0: new high speed SDHC card at address 4001
> [    1.513723] mmcblk0: mmc0:4001 R04GS 3.71 GiB
> [    1.515577]  mmcblk0: p1 p2
> [    1.658838] mmc2: new DDR MMC card at address 0001
> [    1.659934] mmcblk2: mmc2:0001 8WPD3R 7.28 GiB
> [    1.660657] mmcblk2boot0: mmc2:0001 8WPD3R partition 1 4.00 MiB
> [    1.661387] mmcblk2boot1: mmc2:0001 8WPD3R partition 2 4.00 MiB
> [    1.819586] TYPE#0x23, BYTE0#0x7a00, BYTE1#0xc10000
> [    1.849570] TYPE#0x23, BYTE0#0x2000, BYTE1#0x200000
> [    1.879569] TYPE#0x23, BYTE0#0x2100, BYTE1#0xe00000
> [    1.909580] TYPE#0x23, BYTE0#0x2200, BYTE1#0x130000
> [    1.939569] TYPE#0x23, BYTE0#0x2300, BYTE1#0x280000
> [    1.969569] TYPE#0x23, BYTE0#0x2400, BYTE1#0x300000
> [    1.999569] TYPE#0x23, BYTE0#0x2500, BYTE1#0x280000
> [    2.029569] TYPE#0x23, BYTE0#0x2600, BYTE1#0x0
> [    2.059569] TYPE#0x23, BYTE0#0x2700, BYTE1#0xd0000
> [    2.089598] TYPE#0x23, BYTE0#0x2800, BYTE1#0x30000
> [    2.119579] TYPE#0x23, BYTE0#0x2900, BYTE1#0x1d0000
> [    2.149569] TYPE#0x23, BYTE0#0x3400, BYTE1#0x800000
> [    2.179569] TYPE#0x23, BYTE0#0x3600, BYTE1#0x280000
> [    2.209569] TYPE#0x23, BYTE0#0xb500, BYTE1#0xa00000
> [    2.239569] TYPE#0x23, BYTE0#0x5c00, BYTE1#0xff0000
> [    2.269569] TYPE#0x23, BYTE0#0x2a00, BYTE1#0x10000
> [    2.299569] TYPE#0x23, BYTE0#0x5600, BYTE1#0x920000
> [    2.329578] TYPE#0x23, BYTE0#0x6b00, BYTE1#0x710000
> [    2.359569] TYPE#0x23, BYTE0#0x6900, BYTE1#0x2b0000
> [    2.389569] TYPE#0x23, BYTE0#0x1000, BYTE1#0x400000
> [    2.419569] TYPE#0x23, BYTE0#0x1100, BYTE1#0x980000
> [    2.449569] TYPE#0x23, BYTE0#0xb600, BYTE1#0x200000
> [    2.479569] TYPE#0x23, BYTE0#0x5100, BYTE1#0x200000
> [    2.509569] TYPE#0x23, BYTE0#0x900, BYTE1#0x100000
> [    2.679570] TYPE#0x5, BYTE0#0x2900, BYTE1#0x940000
> [    2.767213] Console: switching to colour frame buffer device 100x30
> [    3.144604] sun4i-drm display-engine: fb0: sun4i-drmdrmfb frame buffer device
>
> --
> 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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/CAMty3ZBUrGEi%2Be62sFe7GkXinK3q076sGLwpEVz67qeoV%2B1ZeA%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [linux-sunxi] [PATCH v2 5/9] drm/sun4i: tcon_top: Register clock gates in probe

[Jagan Teki]

On Tue, Jun 18, 2019 at 1:23 PM Chen-Yu Tsai <wens@csie.org> wrote:
>
> On Tue, Jun 18, 2019 at 3:45 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Tue, Jun 18, 2019 at 12:49 PM Chen-Yu Tsai <wens@csie.org> wrote:
> > >
> > > On Mon, Jun 17, 2019 at 6:30 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > >
> > > > On Sun, Jun 16, 2019 at 11:01 AM Chen-Yu Tsai <wens@csie.org> wrote:
> > > > >
> > > > > On Sat, Jun 15, 2019 at 12:44 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > > >
> > > > > > TCON TOP have clock gates for TV0, TV1, dsi and right
> > > > > > now these are register during bind call.
> > > > > >
> > > > > > Of which, dsi clock gate would required during DPHY probe
> > > > > > but same can miss to get since tcon top is not bound at
> > > > > > that time.
> > > > > >
> > > > > > To solve, this circular dependency move the clock gate
> > > > > > registration from bind to probe so-that DPHY can get the
> > > > > > dsi gate clock on time.
> > > > > >
> > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 94 ++++++++++++++------------
> > > > > >  1 file changed, 49 insertions(+), 45 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > > > index 465e9b0cdfee..a8978b3fe851 100644
> > > > > > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > > > @@ -124,7 +124,53 @@ static struct clk_hw *sun8i_tcon_top_register_gate(struct device *dev,
> > > > > >  static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
> > > > > >                                void *data)
> > > > > >  {
> > > > > > -       struct platform_device *pdev = to_platform_device(dev);
> > > > > > +       struct sun8i_tcon_top *tcon_top = dev_get_drvdata(dev);
> > > > > > +       int ret;
> > > > > > +
> > > > > > +       ret = reset_control_deassert(tcon_top->rst);
> > > > > > +       if (ret) {
> > > > > > +               dev_err(dev, "Could not deassert ctrl reset control\n");
> > > > > > +               return ret;
> > > > > > +       }
> > > > > > +
> > > > > > +       ret = clk_prepare_enable(tcon_top->bus);
> > > > > > +       if (ret) {
> > > > > > +               dev_err(dev, "Could not enable bus clock\n");
> > > > > > +               goto err_assert_reset;
> > > > > > +       }
> > > > >
> > > > > You have to de-assert the reset control and enable the clock before the
> > > > > clocks it provides are registered. Otherwise a consumer may come in and
> > > > > ask for the provided clock to be enabled, but since the TCON TOP's own
> > > > > reset and clock are still disabled, you can't actually access the registers
> > > > > that controls the provided clock.
> > > >
> > > > These rst and bus are common reset and bus clocks not tcon top clocks
> > > > that are trying to register here. ie reason I have not moved it in
> > > > top.
> > >
> > > And you're sure that toggling bits in the TCON TOP block doesn't require
> > > the reset to be de-asserted and the bus clock enabled?
> > >
> > > Somehow I doubt that.
> > >
> > > Once the driver register the clocks it provides, they absolutely must work.
> > > They can't only work after the bind phase when the reset gets de-asserted
> > > and the bus clock enabled. Or you should provide proper error reporting
> > > in the clock ops. I doubt you want to go that way either.
> >
> > Why would they won't work after bind phase? unlike tcon top gates,
> > these reset, and bus are common like  what we have in other DE block
> > so enable them in bind won't be an issue as per as I understand. let
> > me know if you want me to check in other directions.
>
> You misunderstood. When you moved the clock registering parts to the probe
> phase, but didn't move the clock enable and reset de-assert parts to go with,
> the clock ops will not work as expected between probe and bind time.

If I understand correctly, I have moved tcon clock gates, not the bus
clock or the reset. Both have independent enablement phase, the bus
clock is enable in tcon top bind and the clock gate ("dsi") enable in
init call of phy_ops. is both bus clock and clock gates are same and
related that is what you are saying?

>
> Simple way to verify it: Just use devmem to disable the TCON TOP bus gate
> and/or assert its reset control. Then try to toggle any of the bits in the
> TCON TOP block and see if it works, or if the bits stick.

Yes I have verified "dsi" gate enablement before via devmem. Below is
the bus, reset disablement and re-enablement and result is similar for
the reset, bus clock in bind and even in probe.

00. get the existing value

# devmem 0x1c70020
0x00010000
# devmem 0x1c20064
0x44021000
# devmem 0x1c202c4
0x44021000

01: disable bus, and assert reset

# devmem 0x1c20064 32 0x4021000
# devmem 0x1c202c4 32 0x4021000
# devmem 0x1c20064
0x04021000
# devmem 0x1c202c4
0x04021000
# devmem 0x1c70020
0x00000000

02: enable bus, and dessert reset

# devmem 0x1c20064 32 0x44021000
# devmem 0x1c202c4 32 0x44021000
# devmem 0x1c20064
0x44021000
# devmem 0x1c202c4
0x44021000
# devmem 0x1c70020
0x00000000

03: enable gate

# devmem 0x1c70020 32 0x00010000
# devmem 0x1c70020
0x00010000

>
> Whether another driver actually does so is not the question. It is just bad
> implementation.

Not sure, I understand this.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/9] drm/sun4i: tcon_top: Register clock gates in probe

[Chen-Yu Tsai]

On Tue, Jun 18, 2019 at 6:34 PM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
>
> On Tue, Jun 18, 2019 at 1:23 PM Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> >
> > On Tue, Jun 18, 2019 at 3:45 PM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
> > >
> > > On Tue, Jun 18, 2019 at 12:49 PM Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> > > >
> > > > On Mon, Jun 17, 2019 at 6:30 PM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
> > > > >
> > > > > On Sun, Jun 16, 2019 at 11:01 AM Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> > > > > >
> > > > > > On Sat, Jun 15, 2019 at 12:44 AM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
> > > > > > >
> > > > > > > TCON TOP have clock gates for TV0, TV1, dsi and right
> > > > > > > now these are register during bind call.
> > > > > > >
> > > > > > > Of which, dsi clock gate would required during DPHY probe
> > > > > > > but same can miss to get since tcon top is not bound at
> > > > > > > that time.
> > > > > > >
> > > > > > > To solve, this circular dependency move the clock gate
> > > > > > > registration from bind to probe so-that DPHY can get the
> > > > > > > dsi gate clock on time.
> > > > > > >
> > > > > > > Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 94 ++++++++++++++------------
> > > > > > >  1 file changed, 49 insertions(+), 45 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > > > > index 465e9b0cdfee..a8978b3fe851 100644
> > > > > > > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > > > > @@ -124,7 +124,53 @@ static struct clk_hw *sun8i_tcon_top_register_gate(struct device *dev,
> > > > > > >  static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
> > > > > > >                                void *data)
> > > > > > >  {
> > > > > > > -       struct platform_device *pdev = to_platform_device(dev);
> > > > > > > +       struct sun8i_tcon_top *tcon_top = dev_get_drvdata(dev);
> > > > > > > +       int ret;
> > > > > > > +
> > > > > > > +       ret = reset_control_deassert(tcon_top->rst);
> > > > > > > +       if (ret) {
> > > > > > > +               dev_err(dev, "Could not deassert ctrl reset control\n");
> > > > > > > +               return ret;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       ret = clk_prepare_enable(tcon_top->bus);
> > > > > > > +       if (ret) {
> > > > > > > +               dev_err(dev, "Could not enable bus clock\n");
> > > > > > > +               goto err_assert_reset;
> > > > > > > +       }
> > > > > >
> > > > > > You have to de-assert the reset control and enable the clock before the
> > > > > > clocks it provides are registered. Otherwise a consumer may come in and
> > > > > > ask for the provided clock to be enabled, but since the TCON TOP's own
> > > > > > reset and clock are still disabled, you can't actually access the registers
> > > > > > that controls the provided clock.
> > > > >
> > > > > These rst and bus are common reset and bus clocks not tcon top clocks
> > > > > that are trying to register here. ie reason I have not moved it in
> > > > > top.
> > > >
> > > > And you're sure that toggling bits in the TCON TOP block doesn't require
> > > > the reset to be de-asserted and the bus clock enabled?
> > > >
> > > > Somehow I doubt that.
> > > >
> > > > Once the driver register the clocks it provides, they absolutely must work.
> > > > They can't only work after the bind phase when the reset gets de-asserted
> > > > and the bus clock enabled. Or you should provide proper error reporting
> > > > in the clock ops. I doubt you want to go that way either.
> > >
> > > Why would they won't work after bind phase? unlike tcon top gates,
> > > these reset, and bus are common like  what we have in other DE block
> > > so enable them in bind won't be an issue as per as I understand. let
> > > me know if you want me to check in other directions.
> >
> > You misunderstood. When you moved the clock registering parts to the probe
> > phase, but didn't move the clock enable and reset de-assert parts to go with,
> > the clock ops will not work as expected between probe and bind time.
>
> If I understand correctly, I have moved tcon clock gates, not the bus
> clock or the reset. Both have independent enablement phase, the bus
> clock is enable in tcon top bind and the clock gate ("dsi") enable in
> init call of phy_ops. is both bus clock and clock gates are same and
> related that is what you are saying?

I am saying that you may need the tcon top bus gates and resets properly
configured to be able to read/write the tcon top address range. That includes
enabling/disabling the clocks that the tcon top driver registers.

In other words, the TCON TOP's bus gate and reset control have everything to do
with what you can do within the TCON TOP block or address range.

> >
> > Simple way to verify it: Just use devmem to disable the TCON TOP bus gate
> > and/or assert its reset control. Then try to toggle any of the bits in the
> > TCON TOP block and see if it works, or if the bits stick.
>
> Yes I have verified "dsi" gate enablement before via devmem. Below is
> the bus, reset disablement and re-enablement and result is similar for
> the reset, bus clock in bind and even in probe.
>
> 00. get the existing value
>
> # devmem 0x1c70020
> 0x00010000
> # devmem 0x1c20064
> 0x44021000
> # devmem 0x1c202c4
> 0x44021000
>
> 01: disable bus, and assert reset
>
> # devmem 0x1c20064 32 0x4021000
> # devmem 0x1c202c4 32 0x4021000
> # devmem 0x1c20064
> 0x04021000
> # devmem 0x1c202c4
> 0x04021000
> # devmem 0x1c70020
> 0x00000000

See here. The value became 0 when it was still 0x10000 in the previous phase.
Any guesses to why this happened, assuming you didn't touch it?

Now if you keep the bus gate disabled and the reset control asserted, and
try to write some non-zero value to 0x1c70020, and read it back, does the
value stick?

If you don't have the bus gate enabled and the reset control de-asserted,
any operations you do to the TCON TOP is essentially not happening. Including
bit operations that the clocks you registered are required to do.

Get what I'm saying?

You need to have the bus gate enabled and the reset control de-asserted
BEFORE you register the clocks you are providing, or something is going
to go very wrong.

Worst case scenario: the reset control was left de-asserted by the bootloader
but the bus gate was disabled. When you register the clocks, the CCF tries
to read back the current status of the clocks, and the I/O stalls because
the bus gate wasn't enabled. System stalls.

Do I need to draw a time flow chart for you?

Also see the very simple example:

    https://elixir.bootlin.com/linux/latest/source/drivers/clk/sunxi-ng/ccu-sun9i-a80-usb.c#L113

where the bus gate is enabled before registering the clocks. This hardware
block doesn't have a reset control for it, but the same principle applies.

> 02: enable bus, and dessert reset
>
> # devmem 0x1c20064 32 0x44021000
> # devmem 0x1c202c4 32 0x44021000
> # devmem 0x1c20064
> 0x44021000
> # devmem 0x1c202c4
> 0x44021000
> # devmem 0x1c70020
> 0x00000000

And it's still zero here, meaning the reset control does have an effect
on the TCON TOP registers.

> 03: enable gate
>
> # devmem 0x1c70020 32 0x00010000
> # devmem 0x1c70020
> 0x00010000

This is irrelevant and not what I wanted you to try.

> >
> > Whether another driver actually does so is not the question. It is just bad
> > implementation.
>
> Not sure, I understand this.

Re: [linux-sunxi] [PATCH v2 5/9] drm/sun4i: tcon_top: Register clock gates in probe

[Jagan Teki]

On Tue, Jun 18, 2019 at 4:24 PM Chen-Yu Tsai <wens@csie.org> wrote:
>
> On Tue, Jun 18, 2019 at 6:34 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Tue, Jun 18, 2019 at 1:23 PM Chen-Yu Tsai <wens@csie.org> wrote:
> > >
> > > On Tue, Jun 18, 2019 at 3:45 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > >
> > > > On Tue, Jun 18, 2019 at 12:49 PM Chen-Yu Tsai <wens@csie.org> wrote:
> > > > >
> > > > > On Mon, Jun 17, 2019 at 6:30 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > > >
> > > > > > On Sun, Jun 16, 2019 at 11:01 AM Chen-Yu Tsai <wens@csie.org> wrote:
> > > > > > >
> > > > > > > On Sat, Jun 15, 2019 at 12:44 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > > > > >
> > > > > > > > TCON TOP have clock gates for TV0, TV1, dsi and right
> > > > > > > > now these are register during bind call.
> > > > > > > >
> > > > > > > > Of which, dsi clock gate would required during DPHY probe
> > > > > > > > but same can miss to get since tcon top is not bound at
> > > > > > > > that time.
> > > > > > > >
> > > > > > > > To solve, this circular dependency move the clock gate
> > > > > > > > registration from bind to probe so-that DPHY can get the
> > > > > > > > dsi gate clock on time.
> > > > > > > >
> > > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 94 ++++++++++++++------------
> > > > > > > >  1 file changed, 49 insertions(+), 45 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > > > > > index 465e9b0cdfee..a8978b3fe851 100644
> > > > > > > > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > > > > > @@ -124,7 +124,53 @@ static struct clk_hw *sun8i_tcon_top_register_gate(struct device *dev,
> > > > > > > >  static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
> > > > > > > >                                void *data)
> > > > > > > >  {
> > > > > > > > -       struct platform_device *pdev = to_platform_device(dev);
> > > > > > > > +       struct sun8i_tcon_top *tcon_top = dev_get_drvdata(dev);
> > > > > > > > +       int ret;
> > > > > > > > +
> > > > > > > > +       ret = reset_control_deassert(tcon_top->rst);
> > > > > > > > +       if (ret) {
> > > > > > > > +               dev_err(dev, "Could not deassert ctrl reset control\n");
> > > > > > > > +               return ret;
> > > > > > > > +       }
> > > > > > > > +
> > > > > > > > +       ret = clk_prepare_enable(tcon_top->bus);
> > > > > > > > +       if (ret) {
> > > > > > > > +               dev_err(dev, "Could not enable bus clock\n");
> > > > > > > > +               goto err_assert_reset;
> > > > > > > > +       }
> > > > > > >
> > > > > > > You have to de-assert the reset control and enable the clock before the
> > > > > > > clocks it provides are registered. Otherwise a consumer may come in and
> > > > > > > ask for the provided clock to be enabled, but since the TCON TOP's own
> > > > > > > reset and clock are still disabled, you can't actually access the registers
> > > > > > > that controls the provided clock.
> > > > > >
> > > > > > These rst and bus are common reset and bus clocks not tcon top clocks
> > > > > > that are trying to register here. ie reason I have not moved it in
> > > > > > top.
> > > > >
> > > > > And you're sure that toggling bits in the TCON TOP block doesn't require
> > > > > the reset to be de-asserted and the bus clock enabled?
> > > > >
> > > > > Somehow I doubt that.
> > > > >
> > > > > Once the driver register the clocks it provides, they absolutely must work.
> > > > > They can't only work after the bind phase when the reset gets de-asserted
> > > > > and the bus clock enabled. Or you should provide proper error reporting
> > > > > in the clock ops. I doubt you want to go that way either.
> > > >
> > > > Why would they won't work after bind phase? unlike tcon top gates,
> > > > these reset, and bus are common like  what we have in other DE block
> > > > so enable them in bind won't be an issue as per as I understand. let
> > > > me know if you want me to check in other directions.
> > >
> > > You misunderstood. When you moved the clock registering parts to the probe
> > > phase, but didn't move the clock enable and reset de-assert parts to go with,
> > > the clock ops will not work as expected between probe and bind time.
> >
> > If I understand correctly, I have moved tcon clock gates, not the bus
> > clock or the reset. Both have independent enablement phase, the bus
> > clock is enable in tcon top bind and the clock gate ("dsi") enable in
> > init call of phy_ops. is both bus clock and clock gates are same and
> > related that is what you are saying?
>
> I am saying that you may need the tcon top bus gates and resets properly
> configured to be able to read/write the tcon top address range. That includes
> enabling/disabling the clocks that the tcon top driver registers.
>
> In other words, the TCON TOP's bus gate and reset control have everything to do
> with what you can do within the TCON TOP block or address range.
>
> > >
> > > Simple way to verify it: Just use devmem to disable the TCON TOP bus gate
> > > and/or assert its reset control. Then try to toggle any of the bits in the
> > > TCON TOP block and see if it works, or if the bits stick.
> >
> > Yes I have verified "dsi" gate enablement before via devmem. Below is
> > the bus, reset disablement and re-enablement and result is similar for
> > the reset, bus clock in bind and even in probe.
> >
> > 00. get the existing value
> >
> > # devmem 0x1c70020
> > 0x00010000
> > # devmem 0x1c20064
> > 0x44021000
> > # devmem 0x1c202c4
> > 0x44021000
> >
> > 01: disable bus, and assert reset
> >
> > # devmem 0x1c20064 32 0x4021000
> > # devmem 0x1c202c4 32 0x4021000
> > # devmem 0x1c20064
> > 0x04021000
> > # devmem 0x1c202c4
> > 0x04021000
> > # devmem 0x1c70020
> > 0x00000000
>
> See here. The value became 0 when it was still 0x10000 in the previous phase.
> Any guesses to why this happened, assuming you didn't touch it?

Yes, I didn't touch anything here. and it indeed expected since the
bus and reset line goes disabled and asserted.

>
> Now if you keep the bus gate disabled and the reset control asserted, and
> try to write some non-zero value to 0x1c70020, and read it back, does the
> value stick?

No, value is not stick. what ever I wrote on on 0x1c70020 it is not taking.

>
> If you don't have the bus gate enabled and the reset control de-asserted,
> any operations you do to the TCON TOP is essentially not happening. Including
> bit operations that the clocks you registered are required to do.
>
> Get what I'm saying?

I understand it, the for accessing tcon space we have bus and reset
line to be enabled and desserted. But the thing is I didn't see any
difference in the behavior even If I enable or deassert the bus and
reset in probe or in bind.

The devmem numbers which I have listed above is same for both the
cases, one with this patch and another one is handle via probe
https://paste.ubuntu.com/p/ndHj9wHzvX/

>
> You need to have the bus gate enabled and the reset control de-asserted
> BEFORE you register the clocks you are providing, or something is going
> to go very wrong.
>
> Worst case scenario: the reset control was left de-asserted by the bootloader
> but the bus gate was disabled. When you register the clocks, the CCF tries
> to read back the current status of the clocks, and the I/O stalls because
> the bus gate wasn't enabled. System stalls.
>
> Do I need to draw a time flow chart for you?

Sure, please.

>
> Also see the very simple example:
>
>     https://elixir.bootlin.com/linux/latest/source/drivers/clk/sunxi-ng/ccu-sun9i-a80-usb.c#L113
>
> where the bus gate is enabled before registering the clocks. This hardware
> block doesn't have a reset control for it, but the same principle applies.

Got it, thanks.

Re: [PATCH v2 5/9] drm/sun4i: tcon_top: Register clock gates in probe

[Chen-Yu Tsai]

On Fri, Jun 21, 2019 at 12:24 AM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
>
> On Tue, Jun 18, 2019 at 4:24 PM Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> >
> > On Tue, Jun 18, 2019 at 6:34 PM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
> > >
> > > On Tue, Jun 18, 2019 at 1:23 PM Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> > > >
> > > > On Tue, Jun 18, 2019 at 3:45 PM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
> > > > >
> > > > > On Tue, Jun 18, 2019 at 12:49 PM Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> > > > > >
> > > > > > On Mon, Jun 17, 2019 at 6:30 PM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
> > > > > > >
> > > > > > > On Sun, Jun 16, 2019 at 11:01 AM Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> > > > > > > >
> > > > > > > > On Sat, Jun 15, 2019 at 12:44 AM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
> > > > > > > > >
> > > > > > > > > TCON TOP have clock gates for TV0, TV1, dsi and right
> > > > > > > > > now these are register during bind call.
> > > > > > > > >
> > > > > > > > > Of which, dsi clock gate would required during DPHY probe
> > > > > > > > > but same can miss to get since tcon top is not bound at
> > > > > > > > > that time.
> > > > > > > > >
> > > > > > > > > To solve, this circular dependency move the clock gate
> > > > > > > > > registration from bind to probe so-that DPHY can get the
> > > > > > > > > dsi gate clock on time.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> > > > > > > > > ---
> > > > > > > > >  drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 94 ++++++++++++++------------
> > > > > > > > >  1 file changed, 49 insertions(+), 45 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > > > > > > index 465e9b0cdfee..a8978b3fe851 100644
> > > > > > > > > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > > > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > > > > > > > @@ -124,7 +124,53 @@ static struct clk_hw *sun8i_tcon_top_register_gate(struct device *dev,
> > > > > > > > >  static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
> > > > > > > > >                                void *data)
> > > > > > > > >  {
> > > > > > > > > -       struct platform_device *pdev = to_platform_device(dev);
> > > > > > > > > +       struct sun8i_tcon_top *tcon_top = dev_get_drvdata(dev);
> > > > > > > > > +       int ret;
> > > > > > > > > +
> > > > > > > > > +       ret = reset_control_deassert(tcon_top->rst);
> > > > > > > > > +       if (ret) {
> > > > > > > > > +               dev_err(dev, "Could not deassert ctrl reset control\n");
> > > > > > > > > +               return ret;
> > > > > > > > > +       }
> > > > > > > > > +
> > > > > > > > > +       ret = clk_prepare_enable(tcon_top->bus);
> > > > > > > > > +       if (ret) {
> > > > > > > > > +               dev_err(dev, "Could not enable bus clock\n");
> > > > > > > > > +               goto err_assert_reset;
> > > > > > > > > +       }
> > > > > > > >
> > > > > > > > You have to de-assert the reset control and enable the clock before the
> > > > > > > > clocks it provides are registered. Otherwise a consumer may come in and
> > > > > > > > ask for the provided clock to be enabled, but since the TCON TOP's own
> > > > > > > > reset and clock are still disabled, you can't actually access the registers
> > > > > > > > that controls the provided clock.
> > > > > > >
> > > > > > > These rst and bus are common reset and bus clocks not tcon top clocks
> > > > > > > that are trying to register here. ie reason I have not moved it in
> > > > > > > top.
> > > > > >
> > > > > > And you're sure that toggling bits in the TCON TOP block doesn't require
> > > > > > the reset to be de-asserted and the bus clock enabled?
> > > > > >
> > > > > > Somehow I doubt that.
> > > > > >
> > > > > > Once the driver register the clocks it provides, they absolutely must work.
> > > > > > They can't only work after the bind phase when the reset gets de-asserted
> > > > > > and the bus clock enabled. Or you should provide proper error reporting
> > > > > > in the clock ops. I doubt you want to go that way either.
> > > > >
> > > > > Why would they won't work after bind phase? unlike tcon top gates,
> > > > > these reset, and bus are common like  what we have in other DE block
> > > > > so enable them in bind won't be an issue as per as I understand. let
> > > > > me know if you want me to check in other directions.
> > > >
> > > > You misunderstood. When you moved the clock registering parts to the probe
> > > > phase, but didn't move the clock enable and reset de-assert parts to go with,
> > > > the clock ops will not work as expected between probe and bind time.
> > >
> > > If I understand correctly, I have moved tcon clock gates, not the bus
> > > clock or the reset. Both have independent enablement phase, the bus
> > > clock is enable in tcon top bind and the clock gate ("dsi") enable in
> > > init call of phy_ops. is both bus clock and clock gates are same and
> > > related that is what you are saying?
> >
> > I am saying that you may need the tcon top bus gates and resets properly
> > configured to be able to read/write the tcon top address range. That includes
> > enabling/disabling the clocks that the tcon top driver registers.
> >
> > In other words, the TCON TOP's bus gate and reset control have everything to do
> > with what you can do within the TCON TOP block or address range.
> >
> > > >
> > > > Simple way to verify it: Just use devmem to disable the TCON TOP bus gate
> > > > and/or assert its reset control. Then try to toggle any of the bits in the
> > > > TCON TOP block and see if it works, or if the bits stick.
> > >
> > > Yes I have verified "dsi" gate enablement before via devmem. Below is
> > > the bus, reset disablement and re-enablement and result is similar for
> > > the reset, bus clock in bind and even in probe.
> > >
> > > 00. get the existing value
> > >
> > > # devmem 0x1c70020
> > > 0x00010000
> > > # devmem 0x1c20064
> > > 0x44021000
> > > # devmem 0x1c202c4
> > > 0x44021000
> > >
> > > 01: disable bus, and assert reset
> > >
> > > # devmem 0x1c20064 32 0x4021000
> > > # devmem 0x1c202c4 32 0x4021000
> > > # devmem 0x1c20064
> > > 0x04021000
> > > # devmem 0x1c202c4
> > > 0x04021000
> > > # devmem 0x1c70020
> > > 0x00000000
> >
> > See here. The value became 0 when it was still 0x10000 in the previous phase.
> > Any guesses to why this happened, assuming you didn't touch it?
>
> Yes, I didn't touch anything here. and it indeed expected since the
> bus and reset line goes disabled and asserted.

OK. So you understand what's going on in the hardware.

> >
> > Now if you keep the bus gate disabled and the reset control asserted, and
> > try to write some non-zero value to 0x1c70020, and read it back, does the
> > value stick?
>
> No, value is not stick. what ever I wrote on on 0x1c70020 it is not taking.

Yes. And that is expected since the reset is asserted.

> >
> > If you don't have the bus gate enabled and the reset control de-asserted,
> > any operations you do to the TCON TOP is essentially not happening. Including
> > bit operations that the clocks you registered are required to do.
> >
> > Get what I'm saying?
>
> I understand it, the for accessing tcon space we have bus and reset
> line to be enabled and desserted. But the thing is I didn't see any
> difference in the behavior even If I enable or deassert the bus and
> reset in probe or in bind.

You mean the display pipeline bind as usual, regardless of whether
you reset in probe or in bind, right?

That is because the TCON and DSI blocks bind _after_ the TCON TOP block.
The display component is traversed breadth first, and components bind
in that order.

What I'm trying to tell you is not that the display pipeline will stop
working, but that you have a problem with how you use the API in the
TCON TOP driver.


When you call clk_hw_register_gate(), you are saying that from this point
onward, the clock that got registered is available and will work properly,
or it will return an error if not.

That is not the case here. Because the TCON TOP reset control is deasserted
and the bus clock enabled much later, right _now_ any attempts to control
the newly registered gate will silently fail, because writes to the register
do not go through.

> The devmem numbers which I have listed above is same for both the
> cases, one with this patch and another one is handle via probe
> https://paste.ubuntu.com/p/ndHj9wHzvX/
>
> >
> > You need to have the bus gate enabled and the reset control de-asserted
> > BEFORE you register the clocks you are providing, or something is going
> > to go very wrong.
> >
> > Worst case scenario: the reset control was left de-asserted by the bootloader
> > but the bus gate was disabled. When you register the clocks, the CCF tries
> > to read back the current status of the clocks, and the I/O stalls because
> > the bus gate wasn't enabled. System stalls.
> >
> > Do I need to draw a time flow chart for you?
>
> Sure, please.

    tcon top probe                        tcon top bind
--[--------------|-]------------>-------[------|--------]------------->
                 |                             |
                 clk_hw_register_gate()        |
                                               reset_control_deassert()
                                               clk_prepare_enable()


So the above is what is happening with your patch:

Between the two square bracket blocks, if any consumer tries to use the
clocks created by clk_hw_register_gate(), it will silently fail, because
the function calls enabling the TCON TOP block happen much later.

Again, the problem is not that the display pipeline stops working. Instead
it is more fundamental: the code is not living up to the API contract that
it declared it is following.

There is also another register/unregister mismatch in this driver. You
moved clk_hw_register_gate(), but the corresponding clk_hw_unregister_gate()
calls that need to happen are still in the unbind function.

So if I were to probe but not bind the driver, and then subsequently
remove the driver, it would be leaking the registered clocks. (At this
point the clocks might not even have code backing them, since I might
have removed the module. I'm not sure if that's possible.) If I try
to probe the driver again, it will fail because it tries to register
clocks with the same names as the ones leaked in the previous round.


ChenYu

> > Also see the very simple example:
> >
> >     https://elixir.bootlin.com/linux/latest/source/drivers/clk/sunxi-ng/ccu-sun9i-a80-usb.c#L113
> >
> > where the bus gate is enabled before registering the clocks. This hardware
> > block doesn't have a reset control for it, but the same principle applies.
>
> Got it, thanks.

Re: [PATCH v2 5/9] drm/sun4i: tcon_top: Register clock gates in probe

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

On Fri, Jun 14, 2019 at 10:13:20PM +0530, Jagan Teki wrote:
> TCON TOP have clock gates for TV0, TV1, dsi and right
> now these are register during bind call.
>
> Of which, dsi clock gate would required during DPHY probe
> but same can miss to get since tcon top is not bound at
> that time.
>
> To solve, this circular dependency move the clock gate
> registration from bind to probe so-that DPHY can get the
> dsi gate clock on time.

It's not really clear to me what the circular dependency is?

if you have a chain that is:

tcon-top +-> DSI
         +-> D-PHY

There's no loop, right?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: Re: [PATCH v2 5/9] drm/sun4i: tcon_top: Register clock gates in probe

[Chen-Yu Tsai]

On Mon, Jun 17, 2019 at 7:45 PM Maxime Ripard <maxime.ripard-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org> wrote:
>
> On Fri, Jun 14, 2019 at 10:13:20PM +0530, Jagan Teki wrote:
> > TCON TOP have clock gates for TV0, TV1, dsi and right
> > now these are register during bind call.
> >
> > Of which, dsi clock gate would required during DPHY probe
> > but same can miss to get since tcon top is not bound at
> > that time.
> >
> > To solve, this circular dependency move the clock gate
> > registration from bind to probe so-that DPHY can get the
> > dsi gate clock on time.
>
> It's not really clear to me what the circular dependency is?
>
> if you have a chain that is:
>
> tcon-top +-> DSI
>          +-> D-PHY
>
> There's no loop, right?

Looking at how the DTSI patch structures things (without going into
whether it is correct or accurate):

The D-PHY is not part of the component graph. However it requests
the DSI gate clock from the TCON-TOP.

The TCON-TOP driver, in its current form, only registers the clocks
it provides at component bind time. Thus the D-PHY can't successfully
probe until the TCON-TOP has been bound.

The DSI interface requires the D-PHY to bind. It will return -EPROBE_DEFER
if it cannot request it. This in turn goes into the error path of
component_bind_all, which unbinds all previous components.

So it's actually

    D-PHY -> TCON-TOP -> DSI
      ^                   |
      |--------------------

I've not checked, but I suspect there's no possibility of having other
drivers probe (to deal with deferred probing) within component_bind_all.
Otherwise we shouldn't run into this weird circular dependency issue.

So the question for Jagan is that is this indeed the case? Does this
patch solve it, or at least work around it.

ChenYu

Re: Re: [PATCH v2 5/9] drm/sun4i: tcon_top: Register clock gates in probe

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

On Mon, Jun 17, 2019 at 09:01:33PM +0800, Chen-Yu Tsai wrote:
> On Mon, Jun 17, 2019 at 7:45 PM Maxime Ripard <maxime.ripard-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org> wrote:
> >
> > On Fri, Jun 14, 2019 at 10:13:20PM +0530, Jagan Teki wrote:
> > > TCON TOP have clock gates for TV0, TV1, dsi and right
> > > now these are register during bind call.
> > >
> > > Of which, dsi clock gate would required during DPHY probe
> > > but same can miss to get since tcon top is not bound at
> > > that time.
> > >
> > > To solve, this circular dependency move the clock gate
> > > registration from bind to probe so-that DPHY can get the
> > > dsi gate clock on time.
> >
> > It's not really clear to me what the circular dependency is?
> >
> > if you have a chain that is:
> >
> > tcon-top +-> DSI
> >          +-> D-PHY
> >
> > There's no loop, right?
>
> Looking at how the DTSI patch structures things (without going into
> whether it is correct or accurate):
>
> The D-PHY is not part of the component graph. However it requests
> the DSI gate clock from the TCON-TOP.
>
> The TCON-TOP driver, in its current form, only registers the clocks
> it provides at component bind time. Thus the D-PHY can't successfully
> probe until the TCON-TOP has been bound.
>
> The DSI interface requires the D-PHY to bind. It will return -EPROBE_DEFER
> if it cannot request it. This in turn goes into the error path of
> component_bind_all, which unbinds all previous components.
>
> So it's actually
>
>     D-PHY -> TCON-TOP -> DSI
>       ^                   |
>       |--------------------
>
> I've not checked, but I suspect there's no possibility of having other
> drivers probe (to deal with deferred probing) within component_bind_all.
> Otherwise we shouldn't run into this weird circular dependency issue.
>

Ah, yes, that makes sense. It should be cleraer in the commit log then.

Thanks!
Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [linux-sunxi] Re: [PATCH v2 5/9] drm/sun4i: tcon_top: Register clock gates in probe

[Jagan Teki]

On Mon, Jun 17, 2019 at 6:31 PM Chen-Yu Tsai <wens@csie.org> wrote:
>
> On Mon, Jun 17, 2019 at 7:45 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Fri, Jun 14, 2019 at 10:13:20PM +0530, Jagan Teki wrote:
> > > TCON TOP have clock gates for TV0, TV1, dsi and right
> > > now these are register during bind call.
> > >
> > > Of which, dsi clock gate would required during DPHY probe
> > > but same can miss to get since tcon top is not bound at
> > > that time.
> > >
> > > To solve, this circular dependency move the clock gate
> > > registration from bind to probe so-that DPHY can get the
> > > dsi gate clock on time.
> >
> > It's not really clear to me what the circular dependency is?
> >
> > if you have a chain that is:
> >
> > tcon-top +-> DSI
> >          +-> D-PHY
> >
> > There's no loop, right?
>
> Looking at how the DTSI patch structures things (without going into
> whether it is correct or accurate):
>
> The D-PHY is not part of the component graph. However it requests
> the DSI gate clock from the TCON-TOP.
>
> The TCON-TOP driver, in its current form, only registers the clocks
> it provides at component bind time. Thus the D-PHY can't successfully
> probe until the TCON-TOP has been bound.
>
> The DSI interface requires the D-PHY to bind. It will return -EPROBE_DEFER
> if it cannot request it. This in turn goes into the error path of
> component_bind_all, which unbinds all previous components.
>
> So it's actually
>
>     D-PHY -> TCON-TOP -> DSI
>       ^                   |
>       |--------------------
>
> I've not checked, but I suspect there's no possibility of having other
> drivers probe (to deal with deferred probing) within component_bind_all.
> Otherwise we shouldn't run into this weird circular dependency issue.
>
> So the question for Jagan is that is this indeed the case? Does this
> patch solve it, or at least work around it.

Yes, this is what I was mentioned in initial version, since the "dsi"
gate in tcon top is registering during bind, the dphy of dsi
controller won't get the associated clock for "mod" so it is keep on
returning -EPROBE_DEFER. By moving the clock gate registration to
probe, everything bound as expected.

Re: Re: [PATCH v2 5/9] drm/sun4i: tcon_top: Register clock gates in probe

[Chen-Yu Tsai]

On Tue, Jun 18, 2019 at 3:12 PM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
>
> On Mon, Jun 17, 2019 at 6:31 PM Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> >
> > On Mon, Jun 17, 2019 at 7:45 PM Maxime Ripard <maxime.ripard-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org> wrote:
> > >
> > > On Fri, Jun 14, 2019 at 10:13:20PM +0530, Jagan Teki wrote:
> > > > TCON TOP have clock gates for TV0, TV1, dsi and right
> > > > now these are register during bind call.
> > > >
> > > > Of which, dsi clock gate would required during DPHY probe
> > > > but same can miss to get since tcon top is not bound at
> > > > that time.
> > > >
> > > > To solve, this circular dependency move the clock gate
> > > > registration from bind to probe so-that DPHY can get the
> > > > dsi gate clock on time.
> > >
> > > It's not really clear to me what the circular dependency is?
> > >
> > > if you have a chain that is:
> > >
> > > tcon-top +-> DSI
> > >          +-> D-PHY
> > >
> > > There's no loop, right?
> >
> > Looking at how the DTSI patch structures things (without going into
> > whether it is correct or accurate):
> >
> > The D-PHY is not part of the component graph. However it requests
> > the DSI gate clock from the TCON-TOP.
> >
> > The TCON-TOP driver, in its current form, only registers the clocks
> > it provides at component bind time. Thus the D-PHY can't successfully
> > probe until the TCON-TOP has been bound.
> >
> > The DSI interface requires the D-PHY to bind. It will return -EPROBE_DEFER
> > if it cannot request it. This in turn goes into the error path of
> > component_bind_all, which unbinds all previous components.
> >
> > So it's actually
> >
> >     D-PHY -> TCON-TOP -> DSI
> >       ^                   |
> >       |--------------------
> >
> > I've not checked, but I suspect there's no possibility of having other
> > drivers probe (to deal with deferred probing) within component_bind_all.
> > Otherwise we shouldn't run into this weird circular dependency issue.
> >
> > So the question for Jagan is that is this indeed the case? Does this
> > patch solve it, or at least work around it.
>
> Yes, this is what I was mentioned in initial version, since the "dsi"
> gate in tcon top is registering during bind, the dphy of dsi
> controller won't get the associated clock for "mod" so it is keep on
> returning -EPROBE_DEFER. By moving the clock gate registration to
> probe, everything bound as expected.

I believe you failed to mention the DSI block, which is the part that
completes the circular dependency. Don't expect others to have full
awareness of the context. You have to provide it in your commit log.

ChenYu

Re: [linux-sunxi] Re: [PATCH v2 5/9] drm/sun4i: tcon_top: Register clock gates in probe

[Jagan Teki]

On Tue, Jun 18, 2019 at 12:53 PM Chen-Yu Tsai <wens@csie.org> wrote:
>
> On Tue, Jun 18, 2019 at 3:12 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Mon, Jun 17, 2019 at 6:31 PM Chen-Yu Tsai <wens@csie.org> wrote:
> > >
> > > On Mon, Jun 17, 2019 at 7:45 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > On Fri, Jun 14, 2019 at 10:13:20PM +0530, Jagan Teki wrote:
> > > > > TCON TOP have clock gates for TV0, TV1, dsi and right
> > > > > now these are register during bind call.
> > > > >
> > > > > Of which, dsi clock gate would required during DPHY probe
> > > > > but same can miss to get since tcon top is not bound at
> > > > > that time.
> > > > >
> > > > > To solve, this circular dependency move the clock gate
> > > > > registration from bind to probe so-that DPHY can get the
> > > > > dsi gate clock on time.
> > > >
> > > > It's not really clear to me what the circular dependency is?
> > > >
> > > > if you have a chain that is:
> > > >
> > > > tcon-top +-> DSI
> > > >          +-> D-PHY
> > > >
> > > > There's no loop, right?
> > >
> > > Looking at how the DTSI patch structures things (without going into
> > > whether it is correct or accurate):
> > >
> > > The D-PHY is not part of the component graph. However it requests
> > > the DSI gate clock from the TCON-TOP.
> > >
> > > The TCON-TOP driver, in its current form, only registers the clocks
> > > it provides at component bind time. Thus the D-PHY can't successfully
> > > probe until the TCON-TOP has been bound.
> > >
> > > The DSI interface requires the D-PHY to bind. It will return -EPROBE_DEFER
> > > if it cannot request it. This in turn goes into the error path of
> > > component_bind_all, which unbinds all previous components.
> > >
> > > So it's actually
> > >
> > >     D-PHY -> TCON-TOP -> DSI
> > >       ^                   |
> > >       |--------------------
> > >
> > > I've not checked, but I suspect there's no possibility of having other
> > > drivers probe (to deal with deferred probing) within component_bind_all.
> > > Otherwise we shouldn't run into this weird circular dependency issue.
> > >
> > > So the question for Jagan is that is this indeed the case? Does this
> > > patch solve it, or at least work around it.
> >
> > Yes, this is what I was mentioned in initial version, since the "dsi"
> > gate in tcon top is registering during bind, the dphy of dsi
> > controller won't get the associated clock for "mod" so it is keep on
> > returning -EPROBE_DEFER. By moving the clock gate registration to
> > probe, everything bound as expected.
>
> I believe you failed to mention the DSI block, which is the part that
> completes the circular dependency. Don't expect others to have full
> awareness of the context. You have to provide it in your commit log.

I have mentioned DPHY and yes it is possible to give more information
will update in next version, no problem. thanks for mentioning that.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/9] ARM: dts: sun8i: r40: Use tcon top clock index macros

[Jagan Teki]

tcon_tv0, tcon_tv1 nodes have a clock names of tcon-ch0,
tcon-ch1 which are referring tcon_top clocks via index
numbers like 0, 1 with CLK_TCON_TV0 and CLK_TCON_TV1
respectively.

Use the macro in place of index numbers, for more code
readability.

Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
Reviewed-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun8i-r40.dtsi | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi
index bb856e53b806..219d2dca16b3 100644
--- a/arch/arm/boot/dts/sun8i-r40.dtsi
+++ b/arch/arm/boot/dts/sun8i-r40.dtsi
@@ -44,6 +44,7 @@
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/clock/sun8i-de2.h>
 #include <dt-bindings/clock/sun8i-r40-ccu.h>
+#include <dt-bindings/clock/sun8i-tcon-top.h>
 #include <dt-bindings/reset/sun8i-r40-ccu.h>
 #include <dt-bindings/reset/sun8i-de2.h>
 
@@ -704,7 +705,7 @@
 			compatible = "allwinner,sun8i-r40-tcon-tv";
 			reg = <0x01c73000 0x1000>;
 			interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>;
-			clocks = <&ccu CLK_BUS_TCON_TV0>, <&tcon_top 0>;
+			clocks = <&ccu CLK_BUS_TCON_TV0>, <&tcon_top CLK_TCON_TOP_TV0>;
 			clock-names = "ahb", "tcon-ch1";
 			resets = <&ccu RST_BUS_TCON_TV0>;
 			reset-names = "lcd";
@@ -747,7 +748,7 @@
 			compatible = "allwinner,sun8i-r40-tcon-tv";
 			reg = <0x01c74000 0x1000>;
 			interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>;
-			clocks = <&ccu CLK_BUS_TCON_TV1>, <&tcon_top 1>;
+			clocks = <&ccu CLK_BUS_TCON_TV1>, <&tcon_top CLK_TCON_TOP_TV1>;
 			clock-names = "ahb", "tcon-ch1";
 			resets = <&ccu RST_BUS_TCON_TV1>;
 			reset-names = "lcd";
-- 
2.18.0.321.gffc6fa0e3

[PATCH v2 4/9] drm/sun4i: tcon_top: Use clock name index macros

[Jagan Teki]

TCON TOP mux blocks in R40 are registering clock using
tcon top clock index numbers.

Right now the code is using, real numbers start with 0, but
we have proper macros that defined these name index numbers.

Use the existing macros, instead of real numbers for more
code readability.

Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
---
 drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
index 3267d0f9b9b2..465e9b0cdfee 100644
--- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
+++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
@@ -194,19 +194,22 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
 	clk_data->hws[CLK_TCON_TOP_TV0] =
 		sun8i_tcon_top_register_gate(dev, "tcon-tv0", regs,
 					     &tcon_top->reg_lock,
-					     TCON_TOP_TCON_TV0_GATE, 0);
+					     TCON_TOP_TCON_TV0_GATE,
+					     CLK_TCON_TOP_TV0);
 
 	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);
+						     TCON_TOP_TCON_TV1_GATE,
+						     CLK_TCON_TOP_TV1);
 
 	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);
+						     TCON_TOP_TCON_DSI_GATE,
+						     CLK_TCON_TOP_DSI);
 
 	for (i = 0; i < CLK_NUM; i++)
 		if (IS_ERR(clk_data->hws[i])) {
-- 
2.18.0.321.gffc6fa0e3

[PATCH v2 6/9] dt-bindings: sun6i-dsi: Add R40 MIPI-DSI compatible (w/ A64 fallback)

[Jagan Teki]

The MIPI DSI controller on Allwinner R40 is similar on
the one on A64 like doesn't associate any DSI_SCLK gating.

So, add R40 compatible and append A64 compatible as fallback.

Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
---
 Documentation/devicetree/bindings/display/sunxi/sun6i-dsi.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/sunxi/sun6i-dsi.txt b/Documentation/devicetree/bindings/display/sunxi/sun6i-dsi.txt
index d0ce51fea103..438f1f999aeb 100644
--- a/Documentation/devicetree/bindings/display/sunxi/sun6i-dsi.txt
+++ b/Documentation/devicetree/bindings/display/sunxi/sun6i-dsi.txt
@@ -13,6 +13,7 @@ Required properties:
   - compatible: value must be one of:
     * allwinner,sun6i-a31-mipi-dsi
     * allwinner,sun50i-a64-mipi-dsi
+    * allwinner,sun8i-r40-mipi-dsi, allwinner,sun50i-a64-mipi-dsi
   - reg: base address and size of memory-mapped region
   - interrupts: interrupt associated to this IP
   - clocks: phandles to the clocks feeding the DSI encoder
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH v2 6/9] dt-bindings: sun6i-dsi: Add R40 MIPI-DSI compatible (w/ A64 fallback)

[Rob Herring]

On Fri, 14 Jun 2019 22:13:21 +0530, Jagan Teki wrote:
> The MIPI DSI controller on Allwinner R40 is similar on
> the one on A64 like doesn't associate any DSI_SCLK gating.
> 
> So, add R40 compatible and append A64 compatible as fallback.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  Documentation/devicetree/bindings/display/sunxi/sun6i-dsi.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v2 7/9] dt-bindings: sun6i-dsi: Add R40 DPHY compatible (w/ A31 fallback)

[Jagan Teki]

The MIPI DSI PHY controller on Allwinner R40 is similar
on the one on A31.

Add R40 compatible and append A31 compatible as fallback.

Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
---
 Documentation/devicetree/bindings/display/sunxi/sun6i-dsi.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/sunxi/sun6i-dsi.txt b/Documentation/devicetree/bindings/display/sunxi/sun6i-dsi.txt
index 438f1f999aeb..b7ad1be33008 100644
--- a/Documentation/devicetree/bindings/display/sunxi/sun6i-dsi.txt
+++ b/Documentation/devicetree/bindings/display/sunxi/sun6i-dsi.txt
@@ -40,6 +40,7 @@ Required properties:
   - compatible: value must be one of:
     * allwinner,sun6i-a31-mipi-dphy
     * allwinner,sun50i-a64-mipi-dphy, allwinner,sun6i-a31-mipi-dphy
+    * allwinner,sun8i-r40-mipi-dphy, allwinner,sun6i-a31-mipi-dphy
   - reg: base address and size of memory-mapped region
   - clocks: phandles to the clocks feeding the DSI encoder
     * bus: the DSI interface clock
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH v2 7/9] dt-bindings: sun6i-dsi: Add R40 DPHY compatible (w/ A31 fallback)

[Rob Herring]

On Fri, 14 Jun 2019 22:13:22 +0530, Jagan Teki wrote:
> The MIPI DSI PHY controller on Allwinner R40 is similar
> on the one on A31.
> 
> Add R40 compatible and append A31 compatible as fallback.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  Documentation/devicetree/bindings/display/sunxi/sun6i-dsi.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v2 8/9] ARM: dts: sun8i: r40: Add MIPI DSI pipeline

[Jagan Teki]

Add MIPI DSI pipeline for Allwinner R40.

Unlike conventional Display pipeline in allwinner, R40 have
TCON TCOP which would interact various block like muxes,
tcon lcd, tcon_tv for better pipeline fitting.

For MIPI DSI pipeline, we have to configure the tcon_lcd0
block which would interact with tcon_top for upper pipeline
connections and dsi block for lower pipeline connections.

So, this patch created that pipeline by adding new nodes
for tcon_lcd0, dsi, dphy and connet them to make proper
pipeline fitting.

Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
---
 arch/arm/boot/dts/sun8i-r40.dtsi | 73 ++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi
index 219d2dca16b3..eb6b2960c700 100644
--- a/arch/arm/boot/dts/sun8i-r40.dtsi
+++ b/arch/arm/boot/dts/sun8i-r40.dtsi
@@ -623,6 +623,7 @@
 
 					tcon_top_mixer0_out_tcon_lcd0: endpoint@0 {
 						reg = <0>;
+						remote-endpoint = <&tcon_lcd0_in_tcon_top_mixer0>;
 					};
 
 					tcon_top_mixer0_out_tcon_lcd1: endpoint@1 {
@@ -701,6 +702,45 @@
 			};
 		};
 
+		tcon_lcd0: lcd-controller@1c71000 {
+			compatible = "allwinner,sun8i-r40-tcon-lcd";
+			reg = <0x01c71000 0x1000>;
+			interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_TCON_LCD0>, <&ccu CLK_TCON_LCD0>;
+			clock-names = "ahb", "tcon-ch0";
+			clock-output-names = "tcon-pixel-clock";
+			resets = <&ccu RST_BUS_TCON_LCD0>, <&ccu RST_BUS_LVDS>;
+			reset-names = "lcd", "lvds";
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				tcon_lcd0_in: port@0 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <0>;
+
+					tcon_lcd0_in_tcon_top_mixer0: endpoint@0 {
+						reg = <0>;
+						remote-endpoint = <&tcon_top_mixer0_out_tcon_lcd0>;
+					};
+				};
+
+				tcon_lcd0_out: port@1 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <1>;
+
+					tcon_lcd0_out_dsi_out: endpoint@1 {
+						reg = <1>;
+						remote-endpoint = <&dsi_in_tcon_lcd0_out>;
+					};
+				};
+			};
+		};
+
 		tcon_tv0: lcd-controller@1c73000 {
 			compatible = "allwinner,sun8i-r40-tcon-tv";
 			reg = <0x01c73000 0x1000>;
@@ -798,6 +838,39 @@
 			interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
 		};
 
+		dsi: dsi@1ca0000 {
+			compatible = "allwinner,sun8i-r40-mipi-dsi",
+				     "allwinner,sun50i-a64-mipi-dsi";
+			reg = <0x01ca0000 0x1000>;
+			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_MIPI_DSI>;
+			clock-names = "bus";
+			resets = <&ccu RST_BUS_MIPI_DSI>;
+			phys = <&dphy>;
+			phy-names = "dphy";
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port {
+				dsi_in_tcon_lcd0_out: endpoint {
+					remote-endpoint = <&tcon_lcd0_out_dsi_out>;
+				};
+			};
+		};
+
+		dphy: d-phy@1ca1000 {
+			compatible = "allwinner,sun8i-r40-mipi-dphy",
+				     "allwinner,sun6i-a31-mipi-dphy";
+			reg = <0x01ca1000 0x1000>;
+			clocks = <&ccu CLK_BUS_MIPI_DSI>,
+				 <&tcon_top CLK_TCON_TOP_DSI>;
+			clock-names = "bus", "mod";
+			resets = <&ccu RST_BUS_MIPI_DSI>;
+			status = "disabled";
+			#phy-cells = <0>;
+		};
+
 		hdmi: hdmi@1ee0000 {
 			compatible = "allwinner,sun8i-r40-dw-hdmi",
 				     "allwinner,sun8i-a83t-dw-hdmi";
-- 
2.18.0.321.gffc6fa0e3

[DO NOT MERGE] [PATCH v2 9/9] ARM: dts: sun8i-r40: bananapi-m2-ultra: Enable Bananapi S070WV20-CT16 DSI panel

[Jagan Teki]

This patch add support for Bananapi S070WV20-CT16 DSI panel to
BPI-M2U board.

DSI panel connected via board DSI port with,
- DCDC1 as VCC-DSI supply
- PH18 gpio for lcd enable pin
- PD17 gpio for lcd reset pin
- PD16 gpio for backlight enable pin

Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
---
 .../boot/dts/sun8i-r40-bananapi-m2-ultra.dts  | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts b/arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts
index c488aaacbd68..5f39317b783e 100644
--- a/arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts
+++ b/arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts
@@ -45,6 +45,7 @@
 #include "sun8i-r40.dtsi"
 
 #include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/pwm/pwm.h>
 
 / {
 	model = "Banana Pi BPI-M2-Ultra";
@@ -55,6 +56,14 @@
 		serial0 = &uart0;
 	};
 
+	backlight: backlight {
+		compatible = "pwm-backlight";
+		pwms = <&pwm 0 50000 PWM_POLARITY_INVERTED>;
+		brightness-levels = <1 2 4 8 16 32 64 128 255>;
+		default-brightness-level = <8>;
+		enable-gpios = <&pio 7 16 GPIO_ACTIVE_HIGH>; /* LCD-BL-EN: PH16 */
+	};
+
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
@@ -117,6 +126,23 @@
 	status = "okay";
 };
 
+&dphy {
+	status = "okay";
+};
+
+&dsi {
+	vcc-dsi-supply = <&reg_dcdc1>;		/* VCC-DSI */
+	status = "okay";
+
+	panel@0 {
+		compatible = "bananapi,s070wv20-ct16-icn6211";
+		reg = <0>;
+		enable-gpio = <&pio 7 18 GPIO_ACTIVE_HIGH>; /* LCD-PWR-EN: PH18 */
+		reset-gpios = <&pio 7 17 GPIO_ACTIVE_HIGH>; /* LCD-RST: PH17 */
+		backlight = <&backlight>;
+	};
+};
+
 &ehci1 {
 	status = "okay";
 };
@@ -203,6 +229,12 @@
 	pinctrl-0 = <&clk_out_a_pin>;
 };
 
+&pwm {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pwm_pins>;
+	status = "okay";
+};
+
 &reg_aldo2 {
 	regulator-always-on;
 	regulator-min-microvolt = <2500000>;
@@ -290,6 +322,10 @@
 	regulator-name = "vdd1v2-sata";
 };
 
+&tcon_lcd0 {
+	status = "okay";
+};
+
 &tcon_tv0 {
 	status = "okay";
 };
-- 
2.18.0.321.gffc6fa0e3