[v11,4/7] drm/sun4i: dsi: Handle bus clock explicitly

884 diff mbox series

Usage of clocks are varies between different Allwinner
DSI controllers. Clocking in A33 would need bus and
mod clocks where as A64 would need only bus clock.

To support this kind of clocking structure variants
in the same dsi driver, explicit handling of common
clock would require since the A64 doesn't need to
mention the clock-names explicitly in dts since it
support only one bus clock.

Also pass clk_id NULL instead "bus" to regmap clock
init function since the single clock variants no need
to mention clock-names explicitly.

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

On Fri, Oct 25, 2019 at 11:26:22PM +0530, Jagan Teki wrote:
> Usage of clocks are varies between different Allwinner
> DSI controllers. Clocking in A33 would need bus and
> mod clocks where as A64 would need only bus clock.
>
> To support this kind of clocking structure variants
> in the same dsi driver,

There's no variance in the clock structure as far as the bus clock is
concerned.

> explicit handling of common clock would require since the A64
> doesn't need to mention the clock-names explicitly in dts since it
> support only one bus clock.
>
> Also pass clk_id NULL instead "bus" to regmap clock init function
> since the single clock variants no need to mention clock-names
> explicitly.

You don't need explicit clock handling. Passing NULL as the argument
in regmap_init_mmio_clk will make it use the first clock, which is the
bus clock.

Maxime

Hi Maxime,

On Mon, Oct 28, 2019 at 9:06 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Fri, Oct 25, 2019 at 11:26:22PM +0530, Jagan Teki wrote:
> > Usage of clocks are varies between different Allwinner
> > DSI controllers. Clocking in A33 would need bus and
> > mod clocks where as A64 would need only bus clock.
> >
> > To support this kind of clocking structure variants
> > in the same dsi driver,
>
> There's no variance in the clock structure as far as the bus clock is
> concerned.
>
> > explicit handling of common clock would require since the A64
> > doesn't need to mention the clock-names explicitly in dts since it
> > support only one bus clock.
> >
> > Also pass clk_id NULL instead "bus" to regmap clock init function
> > since the single clock variants no need to mention clock-names
> > explicitly.
>
> You don't need explicit clock handling. Passing NULL as the argument
> in regmap_init_mmio_clk will make it use the first clock, which is the
> bus clock.

Indeed I tried that, since NULL clk_id wouldn't enable the bus clock
during regmap_mmio_gen_context code, passing NULL triggering vblank
timeout.

On Tue, Oct 29, 2019 at 04:03:56AM +0530, Jagan Teki wrote:
> > > explicit handling of common clock would require since the A64
> > > doesn't need to mention the clock-names explicitly in dts since it
> > > support only one bus clock.
> > >
> > > Also pass clk_id NULL instead "bus" to regmap clock init function
> > > since the single clock variants no need to mention clock-names
> > > explicitly.
> >
> > You don't need explicit clock handling. Passing NULL as the argument
> > in regmap_init_mmio_clk will make it use the first clock, which is the
> > bus clock.
>
> Indeed I tried that, since NULL clk_id wouldn't enable the bus clock
> during regmap_mmio_gen_context code, passing NULL triggering vblank
> timeout.

There's a bunch of users of NULL in tree, so finding out why NULL
doesn't work is the way forward.

Maxime

Hi Maxime,

On Tue, Oct 29, 2019 at 2:24 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Tue, Oct 29, 2019 at 04:03:56AM +0530, Jagan Teki wrote:
> > > > explicit handling of common clock would require since the A64
> > > > doesn't need to mention the clock-names explicitly in dts since it
> > > > support only one bus clock.
> > > >
> > > > Also pass clk_id NULL instead "bus" to regmap clock init function
> > > > since the single clock variants no need to mention clock-names
> > > > explicitly.
> > >
> > > You don't need explicit clock handling. Passing NULL as the argument
> > > in regmap_init_mmio_clk will make it use the first clock, which is the
> > > bus clock.
> >
> > Indeed I tried that, since NULL clk_id wouldn't enable the bus clock
> > during regmap_mmio_gen_context code, passing NULL triggering vblank
> > timeout.
>
> There's a bunch of users of NULL in tree, so finding out why NULL
> doesn't work is the way forward.

I'd have looked the some of the users before checking the code as
well. As I said passing NULL clk_id to devm_regmap_init_mmio_clk =>
__devm_regmap_init_mmio_clk would return before processing the clock.

Here is the code snippet on the tree just to make sure I'm on the same
page or not.

static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
                                        const char *clk_id,
                                        void __iomem *regs,
                                        const struct regmap_config *config)
{
        -----------------------
        --------------
        if (clk_id == NULL)
                return ctx;

        ctx->clk = clk_get(dev, clk_id);
        if (IS_ERR(ctx->clk)) {
                ret = PTR_ERR(ctx->clk);
                goto err_free;
        }

        ret = clk_prepare(ctx->clk);
        if (ret < 0) {
                clk_put(ctx->clk);
                goto err_free;
        }
        -------------
        ---------------
}

Yes, I did check on the driver in the tree before committing explicit
clock handle, which make similar requirements like us in [1]. this
imx2 wdt driver is handling the explicit clock as well. I'm sure this
driver is updated as I have seen few changes related to this driver in
ML.

Let me know if I still miss any key change or note here, I will dig
further on this for sure.

[1] https://elixir.bootlin.com/linux/v5.4-rc4/source/drivers/watchdog/imx2_wdt.c#L264

thanks,
Jagan.

On Fri, Nov 01, 2019 at 07:42:55PM +0530, Jagan Teki wrote:
> Hi Maxime,
>
> On Tue, Oct 29, 2019 at 2:24 PM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Tue, Oct 29, 2019 at 04:03:56AM +0530, Jagan Teki wrote:
> > > > > explicit handling of common clock would require since the A64
> > > > > doesn't need to mention the clock-names explicitly in dts since it
> > > > > support only one bus clock.
> > > > >
> > > > > Also pass clk_id NULL instead "bus" to regmap clock init function
> > > > > since the single clock variants no need to mention clock-names
> > > > > explicitly.
> > > >
> > > > You don't need explicit clock handling. Passing NULL as the argument
> > > > in regmap_init_mmio_clk will make it use the first clock, which is the
> > > > bus clock.
> > >
> > > Indeed I tried that, since NULL clk_id wouldn't enable the bus clock
> > > during regmap_mmio_gen_context code, passing NULL triggering vblank
> > > timeout.
> >
> > There's a bunch of users of NULL in tree, so finding out why NULL
> > doesn't work is the way forward.
>
> I'd have looked the some of the users before checking the code as
> well. As I said passing NULL clk_id to devm_regmap_init_mmio_clk =>
> __devm_regmap_init_mmio_clk would return before processing the clock.
>
> Here is the code snippet on the tree just to make sure I'm on the same
> page or not.
>
> static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
>                                         const char *clk_id,
>                                         void __iomem *regs,
>                                         const struct regmap_config *config)
> {
>         -----------------------
>         --------------
>         if (clk_id == NULL)
>                 return ctx;
>
>         ctx->clk = clk_get(dev, clk_id);
>         if (IS_ERR(ctx->clk)) {
>                 ret = PTR_ERR(ctx->clk);
>                 goto err_free;
>         }
>
>         ret = clk_prepare(ctx->clk);
>         if (ret < 0) {
>                 clk_put(ctx->clk);
>                 goto err_free;
>         }
>         -------------
>         ---------------
> }
>
> Yes, I did check on the driver in the tree before committing explicit
> clock handle, which make similar requirements like us in [1]. this
> imx2 wdt driver is handling the explicit clock as well. I'm sure this
> driver is updated as I have seen few changes related to this driver in
> ML.

I guess we have two ways to go at this then.

Either we remove the return, but it might have a few side-effects, or
we call clk_get with NULL or bus depending on the case, and then call
regmap_mmio_attach_clk.

Maxime

Hi Maxime,

On Sun, Nov 3, 2019 at 11:02 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Fri, Nov 01, 2019 at 07:42:55PM +0530, Jagan Teki wrote:
> > Hi Maxime,
> >
> > On Tue, Oct 29, 2019 at 2:24 PM Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > On Tue, Oct 29, 2019 at 04:03:56AM +0530, Jagan Teki wrote:
> > > > > > explicit handling of common clock would require since the A64
> > > > > > doesn't need to mention the clock-names explicitly in dts since it
> > > > > > support only one bus clock.
> > > > > >
> > > > > > Also pass clk_id NULL instead "bus" to regmap clock init function
> > > > > > since the single clock variants no need to mention clock-names
> > > > > > explicitly.
> > > > >
> > > > > You don't need explicit clock handling. Passing NULL as the argument
> > > > > in regmap_init_mmio_clk will make it use the first clock, which is the
> > > > > bus clock.
> > > >
> > > > Indeed I tried that, since NULL clk_id wouldn't enable the bus clock
> > > > during regmap_mmio_gen_context code, passing NULL triggering vblank
> > > > timeout.
> > >
> > > There's a bunch of users of NULL in tree, so finding out why NULL
> > > doesn't work is the way forward.
> >
> > I'd have looked the some of the users before checking the code as
> > well. As I said passing NULL clk_id to devm_regmap_init_mmio_clk =>
> > __devm_regmap_init_mmio_clk would return before processing the clock.
> >
> > Here is the code snippet on the tree just to make sure I'm on the same
> > page or not.
> >
> > static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
> >                                         const char *clk_id,
> >                                         void __iomem *regs,
> >                                         const struct regmap_config *config)
> > {
> >         -----------------------
> >         --------------
> >         if (clk_id == NULL)
> >                 return ctx;
> >
> >         ctx->clk = clk_get(dev, clk_id);
> >         if (IS_ERR(ctx->clk)) {
> >                 ret = PTR_ERR(ctx->clk);
> >                 goto err_free;
> >         }
> >
> >         ret = clk_prepare(ctx->clk);
> >         if (ret < 0) {
> >                 clk_put(ctx->clk);
> >                 goto err_free;
> >         }
> >         -------------
> >         ---------------
> > }
> >
> > Yes, I did check on the driver in the tree before committing explicit
> > clock handle, which make similar requirements like us in [1]. this
> > imx2 wdt driver is handling the explicit clock as well. I'm sure this
> > driver is updated as I have seen few changes related to this driver in
> > ML.
>
> I guess we have two ways to go at this then.
>
> Either we remove the return, but it might have a few side-effects, or
> we call clk_get with NULL or bus depending on the case, and then call
> regmap_mmio_attach_clk.

Thanks for the inputs.

Please have a look at this snippet, I have used your second
suggestions. let me know if you have any comments?

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 8fa90cfc2ac8..91c95e56d870 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -1109,24 +1109,36 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
         return PTR_ERR(dsi->regulator);
     }

-    dsi->regs = devm_regmap_init_mmio_clk(dev, "bus", base,
-                          &sun6i_dsi_regmap_config);
-    if (IS_ERR(dsi->regs)) {
-        dev_err(dev, "Couldn't create the DSI encoder regmap\n");
-        return PTR_ERR(dsi->regs);
-    }
-
     dsi->reset = devm_reset_control_get_shared(dev, NULL);
     if (IS_ERR(dsi->reset)) {
         dev_err(dev, "Couldn't get our reset line\n");
         return PTR_ERR(dsi->reset);
     }

+    dsi->regs = regmap_init_mmio(dev, base, &sun6i_dsi_regmap_config);
+    if (IS_ERR(dsi->regs)) {
+        dev_err(dev, "Couldn't init regmap\n");
+        return PTR_ERR(dsi->regs);
+    }
+
+    dsi->bus_clk = devm_clk_get(dev, NULL);
+    if (IS_ERR(dsi->bus_clk)) {
+        dev_err(dev, "Couldn't get the DSI bus clock\n");
+        ret = PTR_ERR(dsi->bus_clk);
+        goto err_regmap;
+    } else {
+        printk("Jagan.. Got the BUS clock\n");
+        ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk);
+        if (ret)
+            goto err_bus_clk;
+    }
+
     if (dsi->variant->has_mod_clk) {
         dsi->mod_clk = devm_clk_get(dev, "mod");
         if (IS_ERR(dsi->mod_clk)) {
             dev_err(dev, "Couldn't get the DSI mod clock\n");
-            return PTR_ERR(dsi->mod_clk);
+            ret = PTR_ERR(dsi->mod_clk);
+            goto err_attach_clk;
         }
     }

@@ -1167,6 +1179,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
 err_unprotect_clk:
     if (dsi->variant->has_mod_clk)
         clk_rate_exclusive_put(dsi->mod_clk);
+err_attach_clk:
+    if (!IS_ERR(dsi->bus_clk))
+        regmap_mmio_detach_clk(dsi->regs);
+err_bus_clk:
+    if (!IS_ERR(dsi->bus_clk))
+        clk_put(dsi->bus_clk);
+err_regmap:
+    regmap_exit(dsi->regs);
     return ret;
 }

@@ -1181,6 +1201,13 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
     if (dsi->variant->has_mod_clk)
         clk_rate_exclusive_put(dsi->mod_clk);

+    if (!IS_ERR(dsi->bus_clk)) {
+        regmap_mmio_detach_clk(dsi->regs);
+        clk_put(dsi->bus_clk);
+    }
+
+    regmap_exit(dsi->regs);
+
     return 0;
 }


Jagan.

Hi,

On Thu, Nov 21, 2019 at 05:24:47PM +0530, Jagan Teki wrote:
> On Sun, Nov 3, 2019 at 11:02 PM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Fri, Nov 01, 2019 at 07:42:55PM +0530, Jagan Teki wrote:
> > > Hi Maxime,
> > >
> > > On Tue, Oct 29, 2019 at 2:24 PM Maxime Ripard <mripard@kernel.org> wrote:
> > > >
> > > > On Tue, Oct 29, 2019 at 04:03:56AM +0530, Jagan Teki wrote:
> > > > > > > explicit handling of common clock would require since the A64
> > > > > > > doesn't need to mention the clock-names explicitly in dts since it
> > > > > > > support only one bus clock.
> > > > > > >
> > > > > > > Also pass clk_id NULL instead "bus" to regmap clock init function
> > > > > > > since the single clock variants no need to mention clock-names
> > > > > > > explicitly.
> > > > > >
> > > > > > You don't need explicit clock handling. Passing NULL as the argument
> > > > > > in regmap_init_mmio_clk will make it use the first clock, which is the
> > > > > > bus clock.
> > > > >
> > > > > Indeed I tried that, since NULL clk_id wouldn't enable the bus clock
> > > > > during regmap_mmio_gen_context code, passing NULL triggering vblank
> > > > > timeout.
> > > >
> > > > There's a bunch of users of NULL in tree, so finding out why NULL
> > > > doesn't work is the way forward.
> > >
> > > I'd have looked the some of the users before checking the code as
> > > well. As I said passing NULL clk_id to devm_regmap_init_mmio_clk =>
> > > __devm_regmap_init_mmio_clk would return before processing the clock.
> > >
> > > Here is the code snippet on the tree just to make sure I'm on the same
> > > page or not.
> > >
> > > static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
> > >                                         const char *clk_id,
> > >                                         void __iomem *regs,
> > >                                         const struct regmap_config *config)
> > > {
> > >         -----------------------
> > >         --------------
> > >         if (clk_id == NULL)
> > >                 return ctx;
> > >
> > >         ctx->clk = clk_get(dev, clk_id);
> > >         if (IS_ERR(ctx->clk)) {
> > >                 ret = PTR_ERR(ctx->clk);
> > >                 goto err_free;
> > >         }
> > >
> > >         ret = clk_prepare(ctx->clk);
> > >         if (ret < 0) {
> > >                 clk_put(ctx->clk);
> > >                 goto err_free;
> > >         }
> > >         -------------
> > >         ---------------
> > > }
> > >
> > > Yes, I did check on the driver in the tree before committing explicit
> > > clock handle, which make similar requirements like us in [1]. this
> > > imx2 wdt driver is handling the explicit clock as well. I'm sure this
> > > driver is updated as I have seen few changes related to this driver in
> > > ML.
> >
> > I guess we have two ways to go at this then.
> >
> > Either we remove the return, but it might have a few side-effects, or
> > we call clk_get with NULL or bus depending on the case, and then call
> > regmap_mmio_attach_clk.
>
> Thanks for the inputs.
>
> Please have a look at this snippet, I have used your second
> suggestions. let me know if you have any comments?
>
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 8fa90cfc2ac8..91c95e56d870 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -1109,24 +1109,36 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
>          return PTR_ERR(dsi->regulator);
>      }
>
> -    dsi->regs = devm_regmap_init_mmio_clk(dev, "bus", base,
> -                          &sun6i_dsi_regmap_config);
> -    if (IS_ERR(dsi->regs)) {
> -        dev_err(dev, "Couldn't create the DSI encoder regmap\n");
> -        return PTR_ERR(dsi->regs);
> -    }
> -
>      dsi->reset = devm_reset_control_get_shared(dev, NULL);
>      if (IS_ERR(dsi->reset)) {
>          dev_err(dev, "Couldn't get our reset line\n");
>          return PTR_ERR(dsi->reset);
>      }
>
> +    dsi->regs = regmap_init_mmio(dev, base, &sun6i_dsi_regmap_config);

You should use the devm variant here

> +    if (IS_ERR(dsi->regs)) {
> +        dev_err(dev, "Couldn't init regmap\n");
> +        return PTR_ERR(dsi->regs);
> +    }
> +
> +    dsi->bus_clk = devm_clk_get(dev, NULL);

I guess you still need to pass 'bus' here?

> +    if (IS_ERR(dsi->bus_clk)) {
> +        dev_err(dev, "Couldn't get the DSI bus clock\n");
> +        ret = PTR_ERR(dsi->bus_clk);
> +        goto err_regmap;
> +    } else {
> +        printk("Jagan.. Got the BUS clock\n");
> +        ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk);
> +        if (ret)
> +            goto err_bus_clk;
> +    }
> +
>      if (dsi->variant->has_mod_clk) {
>          dsi->mod_clk = devm_clk_get(dev, "mod");
>          if (IS_ERR(dsi->mod_clk)) {
>              dev_err(dev, "Couldn't get the DSI mod clock\n");
> -            return PTR_ERR(dsi->mod_clk);
> +            ret = PTR_ERR(dsi->mod_clk);
> +            goto err_attach_clk;
>          }
>      }
>
> @@ -1167,6 +1179,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
>  err_unprotect_clk:
>      if (dsi->variant->has_mod_clk)
>          clk_rate_exclusive_put(dsi->mod_clk);
> +err_attach_clk:
> +    if (!IS_ERR(dsi->bus_clk))
> +        regmap_mmio_detach_clk(dsi->regs);
> +err_bus_clk:
> +    if (!IS_ERR(dsi->bus_clk))
> +        clk_put(dsi->bus_clk);
> +err_regmap:
> +    regmap_exit(dsi->regs);
>      return ret;
>  }
>
> @@ -1181,6 +1201,13 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
>      if (dsi->variant->has_mod_clk)
>          clk_rate_exclusive_put(dsi->mod_clk);
>
> +    if (!IS_ERR(dsi->bus_clk)) {
> +        regmap_mmio_detach_clk(dsi->regs);
> +        clk_put(dsi->bus_clk);

This will trigger a warning, you put down the reference twice

Maxime

Hi,

On Fri, Nov 22, 2019 at 11:48 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> On Thu, Nov 21, 2019 at 05:24:47PM +0530, Jagan Teki wrote:
> > On Sun, Nov 3, 2019 at 11:02 PM Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > On Fri, Nov 01, 2019 at 07:42:55PM +0530, Jagan Teki wrote:
> > > > Hi Maxime,
> > > >
> > > > On Tue, Oct 29, 2019 at 2:24 PM Maxime Ripard <mripard@kernel.org> wrote:
> > > > >
> > > > > On Tue, Oct 29, 2019 at 04:03:56AM +0530, Jagan Teki wrote:
> > > > > > > > explicit handling of common clock would require since the A64
> > > > > > > > doesn't need to mention the clock-names explicitly in dts since it
> > > > > > > > support only one bus clock.
> > > > > > > >
> > > > > > > > Also pass clk_id NULL instead "bus" to regmap clock init function
> > > > > > > > since the single clock variants no need to mention clock-names
> > > > > > > > explicitly.
> > > > > > >
> > > > > > > You don't need explicit clock handling. Passing NULL as the argument
> > > > > > > in regmap_init_mmio_clk will make it use the first clock, which is the
> > > > > > > bus clock.
> > > > > >
> > > > > > Indeed I tried that, since NULL clk_id wouldn't enable the bus clock
> > > > > > during regmap_mmio_gen_context code, passing NULL triggering vblank
> > > > > > timeout.
> > > > >
> > > > > There's a bunch of users of NULL in tree, so finding out why NULL
> > > > > doesn't work is the way forward.
> > > >
> > > > I'd have looked the some of the users before checking the code as
> > > > well. As I said passing NULL clk_id to devm_regmap_init_mmio_clk =>
> > > > __devm_regmap_init_mmio_clk would return before processing the clock.
> > > >
> > > > Here is the code snippet on the tree just to make sure I'm on the same
> > > > page or not.
> > > >
> > > > static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
> > > >                                         const char *clk_id,
> > > >                                         void __iomem *regs,
> > > >                                         const struct regmap_config *config)
> > > > {
> > > >         -----------------------
> > > >         --------------
> > > >         if (clk_id == NULL)
> > > >                 return ctx;
> > > >
> > > >         ctx->clk = clk_get(dev, clk_id);
> > > >         if (IS_ERR(ctx->clk)) {
> > > >                 ret = PTR_ERR(ctx->clk);
> > > >                 goto err_free;
> > > >         }
> > > >
> > > >         ret = clk_prepare(ctx->clk);
> > > >         if (ret < 0) {
> > > >                 clk_put(ctx->clk);
> > > >                 goto err_free;
> > > >         }
> > > >         -------------
> > > >         ---------------
> > > > }
> > > >
> > > > Yes, I did check on the driver in the tree before committing explicit
> > > > clock handle, which make similar requirements like us in [1]. this
> > > > imx2 wdt driver is handling the explicit clock as well. I'm sure this
> > > > driver is updated as I have seen few changes related to this driver in
> > > > ML.
> > >
> > > I guess we have two ways to go at this then.
> > >
> > > Either we remove the return, but it might have a few side-effects, or
> > > we call clk_get with NULL or bus depending on the case, and then call
> > > regmap_mmio_attach_clk.
> >
> > Thanks for the inputs.
> >
> > Please have a look at this snippet, I have used your second
> > suggestions. let me know if you have any comments?
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > index 8fa90cfc2ac8..91c95e56d870 100644
> > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > @@ -1109,24 +1109,36 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> >          return PTR_ERR(dsi->regulator);
> >      }
> >
> > -    dsi->regs = devm_regmap_init_mmio_clk(dev, "bus", base,
> > -                          &sun6i_dsi_regmap_config);
> > -    if (IS_ERR(dsi->regs)) {
> > -        dev_err(dev, "Couldn't create the DSI encoder regmap\n");
> > -        return PTR_ERR(dsi->regs);
> > -    }
> > -
> >      dsi->reset = devm_reset_control_get_shared(dev, NULL);
> >      if (IS_ERR(dsi->reset)) {
> >          dev_err(dev, "Couldn't get our reset line\n");
> >          return PTR_ERR(dsi->reset);
> >      }
> >
> > +    dsi->regs = regmap_init_mmio(dev, base, &sun6i_dsi_regmap_config);
>
> You should use the devm variant here

Sure.

>
> > +    if (IS_ERR(dsi->regs)) {
> > +        dev_err(dev, "Couldn't init regmap\n");
> > +        return PTR_ERR(dsi->regs);
> > +    }
> > +
> > +    dsi->bus_clk = devm_clk_get(dev, NULL);
>
> I guess you still need to pass 'bus' here?

But the idea here is not to specify clock name explicitly to support
A64. otherwise A64 would fail as we are not specifying the clock-names
explicitly on dsi node.

dsi: dsi@1ca0000 {
       compatible = "allwinner,sun50i-a64-mipi-dsi";
       reg = <0x01ca0000 0x1000>;
       interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
       clocks = <&ccu CLK_BUS_MIPI_DSI>;
       resets = <&ccu RST_BUS_MIPI_DSI>;
      phys = <&dphy>;
      phy-names = "dphy";
.....
};

>
> > +    if (IS_ERR(dsi->bus_clk)) {
> > +        dev_err(dev, "Couldn't get the DSI bus clock\n");
> > +        ret = PTR_ERR(dsi->bus_clk);
> > +        goto err_regmap;
> > +    } else {
> > +        printk("Jagan.. Got the BUS clock\n");
> > +        ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk);
> > +        if (ret)
> > +            goto err_bus_clk;
> > +    }
> > +
> >      if (dsi->variant->has_mod_clk) {
> >          dsi->mod_clk = devm_clk_get(dev, "mod");
> >          if (IS_ERR(dsi->mod_clk)) {
> >              dev_err(dev, "Couldn't get the DSI mod clock\n");
> > -            return PTR_ERR(dsi->mod_clk);
> > +            ret = PTR_ERR(dsi->mod_clk);
> > +            goto err_attach_clk;
> >          }
> >      }
> >
> > @@ -1167,6 +1179,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> >  err_unprotect_clk:
> >      if (dsi->variant->has_mod_clk)
> >          clk_rate_exclusive_put(dsi->mod_clk);
> > +err_attach_clk:
> > +    if (!IS_ERR(dsi->bus_clk))
> > +        regmap_mmio_detach_clk(dsi->regs);
> > +err_bus_clk:
> > +    if (!IS_ERR(dsi->bus_clk))
> > +        clk_put(dsi->bus_clk);
> > +err_regmap:
> > +    regmap_exit(dsi->regs);
> >      return ret;
> >  }
> >
> > @@ -1181,6 +1201,13 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
> >      if (dsi->variant->has_mod_clk)
> >          clk_rate_exclusive_put(dsi->mod_clk);
> >
> > +    if (!IS_ERR(dsi->bus_clk)) {
> > +        regmap_mmio_detach_clk(dsi->regs);
> > +        clk_put(dsi->bus_clk);
>
> This will trigger a warning, you put down the reference twice

You mean regmap_mmio_detach_clk will put the clk?

Jagan.

On Sat, Nov 23, 2019 at 01:20:21AM +0530, Jagan Teki wrote:
> > > Please have a look at this snippet, I have used your second
> > > suggestions. let me know if you have any comments?
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > index 8fa90cfc2ac8..91c95e56d870 100644
> > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > @@ -1109,24 +1109,36 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> > >          return PTR_ERR(dsi->regulator);
> > >      }
> > >
> > > -    dsi->regs = devm_regmap_init_mmio_clk(dev, "bus", base,
> > > -                          &sun6i_dsi_regmap_config);
> > > -    if (IS_ERR(dsi->regs)) {
> > > -        dev_err(dev, "Couldn't create the DSI encoder regmap\n");
> > > -        return PTR_ERR(dsi->regs);
> > > -    }
> > > -
> > >      dsi->reset = devm_reset_control_get_shared(dev, NULL);
> > >      if (IS_ERR(dsi->reset)) {
> > >          dev_err(dev, "Couldn't get our reset line\n");
> > >          return PTR_ERR(dsi->reset);
> > >      }
> > >
> > > +    dsi->regs = regmap_init_mmio(dev, base, &sun6i_dsi_regmap_config);
> >
> > You should use the devm variant here
>
> Sure.
>
> >
> > > +    if (IS_ERR(dsi->regs)) {
> > > +        dev_err(dev, "Couldn't init regmap\n");
> > > +        return PTR_ERR(dsi->regs);
> > > +    }
> > > +
> > > +    dsi->bus_clk = devm_clk_get(dev, NULL);
> >
> > I guess you still need to pass 'bus' here?
>
> But the idea here is not to specify clock name explicitly to support
> A64. otherwise A64 would fail as we are not specifying the clock-names
> explicitly on dsi node.

Right. But you have no guarantee that the bus clock is going to be the
first one on the other SoCs either.

What about something like that instead:

char *clk_name = NULL;
if (dsi->has_mod_clk)
    clk_name = "bus";

clk = devm_clk_get(dev, clk_name);
if (IS_ERR(clk))
    return PTR_ERR(clk));

regmap_mmio_attach_clk(regmap, clk);

>
> dsi: dsi@1ca0000 {
>        compatible = "allwinner,sun50i-a64-mipi-dsi";
>        reg = <0x01ca0000 0x1000>;
>        interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
>        clocks = <&ccu CLK_BUS_MIPI_DSI>;
>        resets = <&ccu RST_BUS_MIPI_DSI>;
>       phys = <&dphy>;
>       phy-names = "dphy";
> .....
> };
>
> >
> > > +    if (IS_ERR(dsi->bus_clk)) {
> > > +        dev_err(dev, "Couldn't get the DSI bus clock\n");
> > > +        ret = PTR_ERR(dsi->bus_clk);
> > > +        goto err_regmap;
> > > +    } else {
> > > +        printk("Jagan.. Got the BUS clock\n");
> > > +        ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk);
> > > +        if (ret)
> > > +            goto err_bus_clk;
> > > +    }
> > > +
> > >      if (dsi->variant->has_mod_clk) {
> > >          dsi->mod_clk = devm_clk_get(dev, "mod");
> > >          if (IS_ERR(dsi->mod_clk)) {
> > >              dev_err(dev, "Couldn't get the DSI mod clock\n");
> > > -            return PTR_ERR(dsi->mod_clk);
> > > +            ret = PTR_ERR(dsi->mod_clk);
> > > +            goto err_attach_clk;
> > >          }
> > >      }
> > >
> > > @@ -1167,6 +1179,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> > >  err_unprotect_clk:
> > >      if (dsi->variant->has_mod_clk)
> > >          clk_rate_exclusive_put(dsi->mod_clk);
> > > +err_attach_clk:
> > > +    if (!IS_ERR(dsi->bus_clk))
> > > +        regmap_mmio_detach_clk(dsi->regs);
> > > +err_bus_clk:
> > > +    if (!IS_ERR(dsi->bus_clk))
> > > +        clk_put(dsi->bus_clk);
> > > +err_regmap:
> > > +    regmap_exit(dsi->regs);
> > >      return ret;
> > >  }
> > >
> > > @@ -1181,6 +1201,13 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
> > >      if (dsi->variant->has_mod_clk)
> > >          clk_rate_exclusive_put(dsi->mod_clk);
> > >
> > > +    if (!IS_ERR(dsi->bus_clk)) {
> > > +        regmap_mmio_detach_clk(dsi->regs);
> > > +        clk_put(dsi->bus_clk);
> >
> > This will trigger a warning, you put down the reference twice
>
> You mean regmap_mmio_detach_clk will put the clk?

No, devm_clk_get will.

Maxime

On Thu, Nov 28, 2019 at 11:21 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Sat, Nov 23, 2019 at 01:20:21AM +0530, Jagan Teki wrote:
> > > > Please have a look at this snippet, I have used your second
> > > > suggestions. let me know if you have any comments?
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > index 8fa90cfc2ac8..91c95e56d870 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > @@ -1109,24 +1109,36 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> > > >          return PTR_ERR(dsi->regulator);
> > > >      }
> > > >
> > > > -    dsi->regs = devm_regmap_init_mmio_clk(dev, "bus", base,
> > > > -                          &sun6i_dsi_regmap_config);
> > > > -    if (IS_ERR(dsi->regs)) {
> > > > -        dev_err(dev, "Couldn't create the DSI encoder regmap\n");
> > > > -        return PTR_ERR(dsi->regs);
> > > > -    }
> > > > -
> > > >      dsi->reset = devm_reset_control_get_shared(dev, NULL);
> > > >      if (IS_ERR(dsi->reset)) {
> > > >          dev_err(dev, "Couldn't get our reset line\n");
> > > >          return PTR_ERR(dsi->reset);
> > > >      }
> > > >
> > > > +    dsi->regs = regmap_init_mmio(dev, base, &sun6i_dsi_regmap_config);
> > >
> > > You should use the devm variant here
> >
> > Sure.
> >
> > >
> > > > +    if (IS_ERR(dsi->regs)) {
> > > > +        dev_err(dev, "Couldn't init regmap\n");
> > > > +        return PTR_ERR(dsi->regs);
> > > > +    }
> > > > +
> > > > +    dsi->bus_clk = devm_clk_get(dev, NULL);
> > >
> > > I guess you still need to pass 'bus' here?
> >
> > But the idea here is not to specify clock name explicitly to support
> > A64. otherwise A64 would fail as we are not specifying the clock-names
> > explicitly on dsi node.
>
> Right. But you have no guarantee that the bus clock is going to be the
> first one on the other SoCs either.
>
> What about something like that instead:
>
> char *clk_name = NULL;
> if (dsi->has_mod_clk)
>     clk_name = "bus";
>
> clk = devm_clk_get(dev, clk_name);
> if (IS_ERR(clk))
>     return PTR_ERR(clk));
>
> regmap_mmio_attach_clk(regmap, clk);

This makes sense, thanks for your input. I have tested in A33, A64.

>
> >
> > dsi: dsi@1ca0000 {
> >        compatible = "allwinner,sun50i-a64-mipi-dsi";
> >        reg = <0x01ca0000 0x1000>;
> >        interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> >        clocks = <&ccu CLK_BUS_MIPI_DSI>;
> >        resets = <&ccu RST_BUS_MIPI_DSI>;
> >       phys = <&dphy>;
> >       phy-names = "dphy";
> > .....
> > };
> >
> > >
> > > > +    if (IS_ERR(dsi->bus_clk)) {
> > > > +        dev_err(dev, "Couldn't get the DSI bus clock\n");
> > > > +        ret = PTR_ERR(dsi->bus_clk);
> > > > +        goto err_regmap;
> > > > +    } else {
> > > > +        printk("Jagan.. Got the BUS clock\n");
> > > > +        ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk);
> > > > +        if (ret)
> > > > +            goto err_bus_clk;
> > > > +    }
> > > > +
> > > >      if (dsi->variant->has_mod_clk) {
> > > >          dsi->mod_clk = devm_clk_get(dev, "mod");
> > > >          if (IS_ERR(dsi->mod_clk)) {
> > > >              dev_err(dev, "Couldn't get the DSI mod clock\n");
> > > > -            return PTR_ERR(dsi->mod_clk);
> > > > +            ret = PTR_ERR(dsi->mod_clk);
> > > > +            goto err_attach_clk;
> > > >          }
> > > >      }
> > > >
> > > > @@ -1167,6 +1179,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> > > >  err_unprotect_clk:
> > > >      if (dsi->variant->has_mod_clk)
> > > >          clk_rate_exclusive_put(dsi->mod_clk);
> > > > +err_attach_clk:
> > > > +    if (!IS_ERR(dsi->bus_clk))
> > > > +        regmap_mmio_detach_clk(dsi->regs);
> > > > +err_bus_clk:
> > > > +    if (!IS_ERR(dsi->bus_clk))
> > > > +        clk_put(dsi->bus_clk);
> > > > +err_regmap:
> > > > +    regmap_exit(dsi->regs);
> > > >      return ret;
> > > >  }
> > > >
> > > > @@ -1181,6 +1201,13 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
> > > >      if (dsi->variant->has_mod_clk)
> > > >          clk_rate_exclusive_put(dsi->mod_clk);
> > > >
> > > > +    if (!IS_ERR(dsi->bus_clk)) {
> > > > +        regmap_mmio_detach_clk(dsi->regs);
> > > > +        clk_put(dsi->bus_clk);
> > >
> > > This will trigger a warning, you put down the reference twice
> >
> > You mean regmap_mmio_detach_clk will put the clk?
>
> No, devm_clk_get will.

Got it. Will update and send v12.

Jagan.