[v13,4/7] drm/sun4i: dsi: Handle bus clock via regmap_mmio_attach_clk

Message ID 20191218191017.2895-5-jagan@amarulasolutions.com
State New
Headers show
Series
  • drm/sun4i: Allwinner A64 MIPI-DSI support
Related show

Commit Message

Jagan Teki Dec. 18, 2019, 7:10 p.m. UTC
regmap has special API to enable the controller bus clock while
initializing register space, and current driver is using
devm_regmap_init_mmio_clk which require to specify bus
clk_id argument as "bus"

But, the 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.

Since A64 support only single bus clock, it is optional to
specify the clock-names on the controller device tree node.
So using NULL on clk_id would get the attached clock.

To support clk_id as "bus" and "NULL" during clock enablement
between controllers, this patch add generic code to handle
the bus clock using regmap_mmio_attach_clk with associated
regmap APIs.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v13:
- update the changes since has_mod_clk is dropped in previous patch

 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 45 +++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 8 deletions(-)

Comments

Maxime Ripard Dec. 18, 2019, 10:05 p.m. UTC | #1
On Thu, Dec 19, 2019 at 12:40:14AM +0530, Jagan Teki wrote:
> regmap has special API to enable the controller bus clock while
> initializing register space, and current driver is using
> devm_regmap_init_mmio_clk which require to specify bus
> clk_id argument as "bus"
>
> But, the 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.
>
> Since A64 support only single bus clock, it is optional to
> specify the clock-names on the controller device tree node.
> So using NULL on clk_id would get the attached clock.
>
> To support clk_id as "bus" and "NULL" during clock enablement
> between controllers, this patch add generic code to handle
> the bus clock using regmap_mmio_attach_clk with associated
> regmap APIs.
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v13:
> - update the changes since has_mod_clk is dropped in previous patch
>
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 45 +++++++++++++++++++++-----
>  1 file changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 68b88a3dc4c5..de8955fbeb00 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -1081,6 +1081,7 @@ static const struct component_ops sun6i_dsi_ops = {
>  static int sun6i_dsi_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	const char *bus_clk_name = NULL;
>  	struct sun6i_dsi *dsi;
>  	struct resource *res;
>  	void __iomem *base;
> @@ -1094,6 +1095,10 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
>  	dsi->host.ops = &sun6i_dsi_host_ops;
>  	dsi->host.dev = dev;
>
> +	if (of_device_is_compatible(dev->of_node,
> +				    "allwinner,sun6i-a31-mipi-dsi"))
> +		bus_clk_name = "bus";
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	base = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(base)) {
> @@ -1107,25 +1112,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 = devm_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, bus_clk_name);
> +	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 {
> +		ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk);
> +		if (ret)
> +			goto err_bus_clk;
> +	}
> +
>  	if (of_device_is_compatible(dev->of_node,
>  				    "allwinner,sun6i-a31-mipi-dsi")) {
>  		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;
>  		}
>  	}
>
> @@ -1164,6 +1180,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
>  	pm_runtime_disable(dev);
>  err_unprotect_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);

You still have an unbalanced clk_get / clk_put here

> +err_regmap:
> +	regmap_exit(dsi->regs);

That's not needed.

>  	return ret;
>  }
>
> @@ -1177,6 +1201,11 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
>  	pm_runtime_disable(dev);
>  	clk_rate_exclusive_put(dsi->mod_clk);
>
> +	if (!IS_ERR(dsi->bus_clk))
> +		regmap_mmio_detach_clk(dsi->regs);
> +
> +	regmap_exit(dsi->regs);

Same thing here.

Maxime
Jagan Teki Dec. 21, 2019, 11:41 a.m. UTC | #2
On Thu, Dec 19, 2019 at 3:35 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Thu, Dec 19, 2019 at 12:40:14AM +0530, Jagan Teki wrote:
> > regmap has special API to enable the controller bus clock while
> > initializing register space, and current driver is using
> > devm_regmap_init_mmio_clk which require to specify bus
> > clk_id argument as "bus"
> >
> > But, the 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.
> >
> > Since A64 support only single bus clock, it is optional to
> > specify the clock-names on the controller device tree node.
> > So using NULL on clk_id would get the attached clock.
> >
> > To support clk_id as "bus" and "NULL" during clock enablement
> > between controllers, this patch add generic code to handle
> > the bus clock using regmap_mmio_attach_clk with associated
> > regmap APIs.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> > Changes for v13:
> > - update the changes since has_mod_clk is dropped in previous patch
> >
> >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 45 +++++++++++++++++++++-----
> >  1 file changed, 37 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > index 68b88a3dc4c5..de8955fbeb00 100644
> > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > @@ -1081,6 +1081,7 @@ static const struct component_ops sun6i_dsi_ops = {
> >  static int sun6i_dsi_probe(struct platform_device *pdev)
> >  {
> >       struct device *dev = &pdev->dev;
> > +     const char *bus_clk_name = NULL;
> >       struct sun6i_dsi *dsi;
> >       struct resource *res;
> >       void __iomem *base;
> > @@ -1094,6 +1095,10 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> >       dsi->host.ops = &sun6i_dsi_host_ops;
> >       dsi->host.dev = dev;
> >
> > +     if (of_device_is_compatible(dev->of_node,
> > +                                 "allwinner,sun6i-a31-mipi-dsi"))
> > +             bus_clk_name = "bus";
> > +
> >       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >       base = devm_ioremap_resource(dev, res);
> >       if (IS_ERR(base)) {
> > @@ -1107,25 +1112,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 = devm_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, bus_clk_name);
> > +     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 {
> > +             ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk);
> > +             if (ret)
> > +                     goto err_bus_clk;
> > +     }
> > +
> >       if (of_device_is_compatible(dev->of_node,
> >                                   "allwinner,sun6i-a31-mipi-dsi")) {
> >               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;
> >               }
> >       }
> >
> > @@ -1164,6 +1180,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> >       pm_runtime_disable(dev);
> >  err_unprotect_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);
>
> You still have an unbalanced clk_get / clk_put here

You mean it is not needed right since devm_clk_get has release call
via devres_alloc? or the wrong position for clk_put?

>
> > +err_regmap:
> > +     regmap_exit(dsi->regs);
>
> That's not needed.

Yes. look like __devm_regmap_init has release call with regmap_exit.

Thanks for the comments, I will update these and send next version.
Let me know if you have any more comments?
Maxime Ripard Dec. 21, 2019, 4:02 p.m. UTC | #3
On Sat, Dec 21, 2019 at 05:11:00PM +0530, Jagan Teki wrote:
> On Thu, Dec 19, 2019 at 3:35 AM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Thu, Dec 19, 2019 at 12:40:14AM +0530, Jagan Teki wrote:
> > > regmap has special API to enable the controller bus clock while
> > > initializing register space, and current driver is using
> > > devm_regmap_init_mmio_clk which require to specify bus
> > > clk_id argument as "bus"
> > >
> > > But, the 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.
> > >
> > > Since A64 support only single bus clock, it is optional to
> > > specify the clock-names on the controller device tree node.
> > > So using NULL on clk_id would get the attached clock.
> > >
> > > To support clk_id as "bus" and "NULL" during clock enablement
> > > between controllers, this patch add generic code to handle
> > > the bus clock using regmap_mmio_attach_clk with associated
> > > regmap APIs.
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > ---
> > > Changes for v13:
> > > - update the changes since has_mod_clk is dropped in previous patch
> > >
> > >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 45 +++++++++++++++++++++-----
> > >  1 file changed, 37 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > index 68b88a3dc4c5..de8955fbeb00 100644
> > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > @@ -1081,6 +1081,7 @@ static const struct component_ops sun6i_dsi_ops = {
> > >  static int sun6i_dsi_probe(struct platform_device *pdev)
> > >  {
> > >       struct device *dev = &pdev->dev;
> > > +     const char *bus_clk_name = NULL;
> > >       struct sun6i_dsi *dsi;
> > >       struct resource *res;
> > >       void __iomem *base;
> > > @@ -1094,6 +1095,10 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> > >       dsi->host.ops = &sun6i_dsi_host_ops;
> > >       dsi->host.dev = dev;
> > >
> > > +     if (of_device_is_compatible(dev->of_node,
> > > +                                 "allwinner,sun6i-a31-mipi-dsi"))
> > > +             bus_clk_name = "bus";
> > > +
> > >       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >       base = devm_ioremap_resource(dev, res);
> > >       if (IS_ERR(base)) {
> > > @@ -1107,25 +1112,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 = devm_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, bus_clk_name);
> > > +     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 {
> > > +             ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk);
> > > +             if (ret)
> > > +                     goto err_bus_clk;
> > > +     }
> > > +
> > >       if (of_device_is_compatible(dev->of_node,
> > >                                   "allwinner,sun6i-a31-mipi-dsi")) {
> > >               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;
> > >               }
> > >       }
> > >
> > > @@ -1164,6 +1180,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> > >       pm_runtime_disable(dev);
> > >  err_unprotect_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);
> >
> > You still have an unbalanced clk_get / clk_put here
>
> You mean it is not needed right since devm_clk_get has release call
> via devres_alloc?

Yes

Patch

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 68b88a3dc4c5..de8955fbeb00 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -1081,6 +1081,7 @@  static const struct component_ops sun6i_dsi_ops = {
 static int sun6i_dsi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	const char *bus_clk_name = NULL;
 	struct sun6i_dsi *dsi;
 	struct resource *res;
 	void __iomem *base;
@@ -1094,6 +1095,10 @@  static int sun6i_dsi_probe(struct platform_device *pdev)
 	dsi->host.ops = &sun6i_dsi_host_ops;
 	dsi->host.dev = dev;
 
+	if (of_device_is_compatible(dev->of_node,
+				    "allwinner,sun6i-a31-mipi-dsi"))
+		bus_clk_name = "bus";
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(base)) {
@@ -1107,25 +1112,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 = devm_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, bus_clk_name);
+	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 {
+		ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk);
+		if (ret)
+			goto err_bus_clk;
+	}
+
 	if (of_device_is_compatible(dev->of_node,
 				    "allwinner,sun6i-a31-mipi-dsi")) {
 		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;
 		}
 	}
 
@@ -1164,6 +1180,14 @@  static int sun6i_dsi_probe(struct platform_device *pdev)
 	pm_runtime_disable(dev);
 err_unprotect_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;
 }
 
@@ -1177,6 +1201,11 @@  static int sun6i_dsi_remove(struct platform_device *pdev)
 	pm_runtime_disable(dev);
 	clk_rate_exclusive_put(dsi->mod_clk);
 
+	if (!IS_ERR(dsi->bus_clk))
+		regmap_mmio_detach_clk(dsi->regs);
+
+	regmap_exit(dsi->regs);
+
 	return 0;
 }