Patchwork [2/6] dmaengine: sun6i: Add a quirk for additional mbus clock

login
register
mail settings
Submitter Jernej Škrabec
Date March 7, 2019, 4:58 p.m.
Message ID <20190307165829.9086-3-jernej.skrabec@siol.net>
Download mbox | patch
Permalink /patch/743801/
State New
Headers show

Comments

Jernej Škrabec - March 7, 2019, 4:58 p.m.
H6 DMA controller needs additional mbus clock to be enabled.

Add a quirk for it and handle it accordingly.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/dma/sun6i-dma.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)
Chen-Yu Tsai - March 11, 2019, 5:47 a.m.
On Fri, Mar 8, 2019 at 12:58 AM Jernej Skrabec <jernej.skrabec@siol.net> wrote:
>
> H6 DMA controller needs additional mbus clock to be enabled.
>
> Add a quirk for it and handle it accordingly.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/dma/sun6i-dma.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 0cd13f17fc11..761555080325 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -129,6 +129,7 @@ struct sun6i_dma_config {
>         u32 dst_burst_lengths;
>         u32 src_addr_widths;
>         u32 dst_addr_widths;
> +       bool mbus_clk;

Nit: has_mbus_clk. Be explicit. Leave nothing to assumptions.

>  };
>
>  /*
> @@ -182,6 +183,7 @@ struct sun6i_dma_dev {
>         struct dma_device       slave;
>         void __iomem            *base;
>         struct clk              *clk;
> +       struct clk              *clk_mbus;
>         int                     irq;
>         spinlock_t              lock;
>         struct reset_control    *rstc;
> @@ -1208,6 +1210,14 @@ static int sun6i_dma_probe(struct platform_device *pdev)
>                 return PTR_ERR(sdc->clk);
>         }
>
> +       if (sdc->cfg->mbus_clk) {
> +               sdc->clk_mbus = devm_clk_get(&pdev->dev, "mbus");
> +               if (IS_ERR(sdc->clk_mbus)) {
> +                       dev_err(&pdev->dev, "No mbus clock specified\n");
> +                       return PTR_ERR(sdc->clk_mbus);
> +               }
> +       }
> +
>         sdc->rstc = devm_reset_control_get(&pdev->dev, NULL);
>         if (IS_ERR(sdc->rstc)) {
>                 dev_err(&pdev->dev, "No reset controller specified\n");
> @@ -1312,11 +1322,19 @@ static int sun6i_dma_probe(struct platform_device *pdev)
>                 goto err_reset_assert;
>         }
>
> +       if (sdc->cfg->mbus_clk) {
> +               ret = clk_prepare_enable(sdc->clk_mbus);

The clk API checks for NULL pointer, so you could just drop the
conditional here.

> +               if (ret) {
> +                       dev_err(&pdev->dev, "Couldn't enable mbus clock\n");
> +                       goto err_clk_disable;
> +               }
> +       }
> +
>         ret = devm_request_irq(&pdev->dev, sdc->irq, sun6i_dma_interrupt, 0,
>                                dev_name(&pdev->dev), sdc);
>         if (ret) {
>                 dev_err(&pdev->dev, "Cannot request IRQ\n");
> -               goto err_clk_disable;
> +               goto err_mbus_clk_disable;
>         }
>
>         ret = dma_async_device_register(&sdc->slave);
> @@ -1341,6 +1359,8 @@ static int sun6i_dma_probe(struct platform_device *pdev)
>         dma_async_device_unregister(&sdc->slave);
>  err_irq_disable:
>         sun6i_kill_tasklet(sdc);
> +err_mbus_clk_disable:
> +       clk_disable_unprepare(sdc->clk_mbus);

Since you aren't using it here either.

ChenYu


>  err_clk_disable:
>         clk_disable_unprepare(sdc->clk);
>  err_reset_assert:
> @@ -1359,6 +1379,7 @@ static int sun6i_dma_remove(struct platform_device *pdev)
>
>         sun6i_kill_tasklet(sdc);
>
> +       clk_disable_unprepare(sdc->clk_mbus);
>         clk_disable_unprepare(sdc->clk);
>         reset_control_assert(sdc->rstc);
>
> --
> 2.21.0
>
Vinod Koul - March 16, 2019, 11:07 a.m.
On 07-03-19, 17:58, Jernej Skrabec wrote:
> H6 DMA controller needs additional mbus clock to be enabled.
> 
> Add a quirk for it and handle it accordingly.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/dma/sun6i-dma.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 0cd13f17fc11..761555080325 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -129,6 +129,7 @@ struct sun6i_dma_config {
>  	u32 dst_burst_lengths;
>  	u32 src_addr_widths;
>  	u32 dst_addr_widths;
> +	bool mbus_clk;
>  };
>  
>  /*
> @@ -182,6 +183,7 @@ struct sun6i_dma_dev {
>  	struct dma_device	slave;
>  	void __iomem		*base;
>  	struct clk		*clk;
> +	struct clk		*clk_mbus;

So rather than have mbus_clk and then a ptr, why not use the ptr and use
NULL value to check for..?

>  	int			irq;
>  	spinlock_t		lock;
>  	struct reset_control	*rstc;
> @@ -1208,6 +1210,14 @@ static int sun6i_dma_probe(struct platform_device *pdev)
>  		return PTR_ERR(sdc->clk);
>  	}
>  
> +	if (sdc->cfg->mbus_clk) {

where is the populated? I was expecting this to be set based on DT!

> +		sdc->clk_mbus = devm_clk_get(&pdev->dev, "mbus");
> +		if (IS_ERR(sdc->clk_mbus)) {
> +			dev_err(&pdev->dev, "No mbus clock specified\n");
> +			return PTR_ERR(sdc->clk_mbus);
> +		}
> +	}
> +
>  	sdc->rstc = devm_reset_control_get(&pdev->dev, NULL);
>  	if (IS_ERR(sdc->rstc)) {
>  		dev_err(&pdev->dev, "No reset controller specified\n");
> @@ -1312,11 +1322,19 @@ static int sun6i_dma_probe(struct platform_device *pdev)
>  		goto err_reset_assert;
>  	}
>  
> +	if (sdc->cfg->mbus_clk) {
> +		ret = clk_prepare_enable(sdc->clk_mbus);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Couldn't enable mbus clock\n");
> +			goto err_clk_disable;
> +		}
> +	}
> +
>  	ret = devm_request_irq(&pdev->dev, sdc->irq, sun6i_dma_interrupt, 0,
>  			       dev_name(&pdev->dev), sdc);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Cannot request IRQ\n");
> -		goto err_clk_disable;
> +		goto err_mbus_clk_disable;
>  	}
>  
>  	ret = dma_async_device_register(&sdc->slave);
> @@ -1341,6 +1359,8 @@ static int sun6i_dma_probe(struct platform_device *pdev)
>  	dma_async_device_unregister(&sdc->slave);
>  err_irq_disable:
>  	sun6i_kill_tasklet(sdc);
> +err_mbus_clk_disable:
> +	clk_disable_unprepare(sdc->clk_mbus);
>  err_clk_disable:
>  	clk_disable_unprepare(sdc->clk);
>  err_reset_assert:
> @@ -1359,6 +1379,7 @@ static int sun6i_dma_remove(struct platform_device *pdev)
>  
>  	sun6i_kill_tasklet(sdc);
>  
> +	clk_disable_unprepare(sdc->clk_mbus);
>  	clk_disable_unprepare(sdc->clk);
>  	reset_control_assert(sdc->rstc);
>  
> -- 
> 2.21.0
Jernej Škrabec - March 16, 2019, 11:23 a.m.
Hi!

Dne sobota, 16. marec 2019 ob 12:07:53 CET je Vinod Koul napisal(a):
> On 07-03-19, 17:58, Jernej Skrabec wrote:
> > H6 DMA controller needs additional mbus clock to be enabled.
> > 
> > Add a quirk for it and handle it accordingly.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > 
> >  drivers/dma/sun6i-dma.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index 0cd13f17fc11..761555080325 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -129,6 +129,7 @@ struct sun6i_dma_config {
> > 
> >  	u32 dst_burst_lengths;
> >  	u32 src_addr_widths;
> >  	u32 dst_addr_widths;
> > 
> > +	bool mbus_clk;
> > 
> >  };
> >  
> >  /*
> > 
> > @@ -182,6 +183,7 @@ struct sun6i_dma_dev {
> > 
> >  	struct dma_device	slave;
> >  	void __iomem		*base;
> >  	struct clk		*clk;
> > 
> > +	struct clk		*clk_mbus;
> 
> So rather than have mbus_clk and then a ptr, why not use the ptr and use
> NULL value to check for..?
> 

I'm not sure what you mean here. clk_mbus will hold a reference to a clock 
retrieved by devm_clk_get() so it has to be "struct clk *".

What I'm missing here?

> >  	int			irq;
> >  	spinlock_t		lock;
> >  	struct reset_control	*rstc;
> > 
> > @@ -1208,6 +1210,14 @@ static int sun6i_dma_probe(struct platform_device
> > *pdev)> 
> >  		return PTR_ERR(sdc->clk);
> >  	
> >  	}
> > 
> > +	if (sdc->cfg->mbus_clk) {
> 
> where is the populated? I was expecting this to be set based on DT!

Of course it is based on DT. Check patch 5, where quirks structure attached to 
H6 DMA compatible contains ".mbus_clk = true,". "sdc->cfg" points to this 
quirk structure.

> 
> > +		sdc->clk_mbus = devm_clk_get(&pdev->dev, "mbus");
> > +		if (IS_ERR(sdc->clk_mbus)) {
> > +			dev_err(&pdev->dev, "No mbus clock 
specified\n");
> > +			return PTR_ERR(sdc->clk_mbus);
> > +		}
> > +	}
> > +
> > 
> >  	sdc->rstc = devm_reset_control_get(&pdev->dev, NULL);
> >  	if (IS_ERR(sdc->rstc)) {
> >  	
> >  		dev_err(&pdev->dev, "No reset controller specified\n");
> > 
> > @@ -1312,11 +1322,19 @@ static int sun6i_dma_probe(struct platform_device
> > *pdev)> 
> >  		goto err_reset_assert;
> >  	
> >  	}
> > 
> > +	if (sdc->cfg->mbus_clk) {
> > +		ret = clk_prepare_enable(sdc->clk_mbus);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "Couldn't enable mbus 
clock\n");
> > +			goto err_clk_disable;
> > +		}
> > +	}
> > +
> > 
> >  	ret = devm_request_irq(&pdev->dev, sdc->irq, sun6i_dma_interrupt, 
0,
> >  	
> >  			       dev_name(&pdev->dev), sdc);
> >  	
> >  	if (ret) {
> >  	
> >  		dev_err(&pdev->dev, "Cannot request IRQ\n");
> > 
> > -		goto err_clk_disable;
> > +		goto err_mbus_clk_disable;
> > 
> >  	}
> >  	
> >  	ret = dma_async_device_register(&sdc->slave);
> > 
> > @@ -1341,6 +1359,8 @@ static int sun6i_dma_probe(struct platform_device
> > *pdev)> 
> >  	dma_async_device_unregister(&sdc->slave);
> >  
> >  err_irq_disable:
> >  	sun6i_kill_tasklet(sdc);
> > 
> > +err_mbus_clk_disable:
> > +	clk_disable_unprepare(sdc->clk_mbus);
> > 
> >  err_clk_disable:
> >  	clk_disable_unprepare(sdc->clk);
> >  
> >  err_reset_assert:
> > @@ -1359,6 +1379,7 @@ static int sun6i_dma_remove(struct platform_device
> > *pdev)> 
> >  	sun6i_kill_tasklet(sdc);
> > 
> > +	clk_disable_unprepare(sdc->clk_mbus);
> > 
> >  	clk_disable_unprepare(sdc->clk);
> >  	reset_control_assert(sdc->rstc);
Vinod Koul - March 22, 2019, 12:59 p.m.
On 16-03-19, 12:23, Jernej Škrabec wrote:
> Hi!
> 
> Dne sobota, 16. marec 2019 ob 12:07:53 CET je Vinod Koul napisal(a):
> > On 07-03-19, 17:58, Jernej Skrabec wrote:
> > > H6 DMA controller needs additional mbus clock to be enabled.
> > > 
> > > Add a quirk for it and handle it accordingly.
> > > 
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > ---
> > > 
> > >  drivers/dma/sun6i-dma.c | 23 ++++++++++++++++++++++-
> > >  1 file changed, 22 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > > index 0cd13f17fc11..761555080325 100644
> > > --- a/drivers/dma/sun6i-dma.c
> > > +++ b/drivers/dma/sun6i-dma.c
> > > @@ -129,6 +129,7 @@ struct sun6i_dma_config {
> > > 
> > >  	u32 dst_burst_lengths;
> > >  	u32 src_addr_widths;
> > >  	u32 dst_addr_widths;
> > > 
> > > +	bool mbus_clk;
> > > 
> > >  };
> > >  
> > >  /*
> > > 
> > > @@ -182,6 +183,7 @@ struct sun6i_dma_dev {
> > > 
> > >  	struct dma_device	slave;
> > >  	void __iomem		*base;
> > >  	struct clk		*clk;
> > > 
> > > +	struct clk		*clk_mbus;
> > 
> > So rather than have mbus_clk and then a ptr, why not use the ptr and use
> > NULL value to check for..?
> > 
> 
> I'm not sure what you mean here. clk_mbus will hold a reference to a clock 
> retrieved by devm_clk_get() so it has to be "struct clk *".
> 
> What I'm missing here?

IIRC there were two variable one clk ptr and one an integer to mark
presence, you may be able to skip variable and use ptr..
Maxime Ripard - March 22, 2019, 1:03 p.m.
On Fri, Mar 22, 2019 at 06:29:40PM +0530, Vinod Koul wrote:
> On 16-03-19, 12:23, Jernej Škrabec wrote:
> > Hi!
> >
> > Dne sobota, 16. marec 2019 ob 12:07:53 CET je Vinod Koul napisal(a):
> > > On 07-03-19, 17:58, Jernej Skrabec wrote:
> > > > H6 DMA controller needs additional mbus clock to be enabled.
> > > >
> > > > Add a quirk for it and handle it accordingly.
> > > >
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > ---
> > > >
> > > >  drivers/dma/sun6i-dma.c | 23 ++++++++++++++++++++++-
> > > >  1 file changed, 22 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > > > index 0cd13f17fc11..761555080325 100644
> > > > --- a/drivers/dma/sun6i-dma.c
> > > > +++ b/drivers/dma/sun6i-dma.c
> > > > @@ -129,6 +129,7 @@ struct sun6i_dma_config {
> > > >
> > > >  	u32 dst_burst_lengths;
> > > >  	u32 src_addr_widths;
> > > >  	u32 dst_addr_widths;
> > > >
> > > > +	bool mbus_clk;
> > > >
> > > >  };
> > > >
> > > >  /*
> > > >
> > > > @@ -182,6 +183,7 @@ struct sun6i_dma_dev {
> > > >
> > > >  	struct dma_device	slave;
> > > >  	void __iomem		*base;
> > > >  	struct clk		*clk;
> > > >
> > > > +	struct clk		*clk_mbus;
> > >
> > > So rather than have mbus_clk and then a ptr, why not use the ptr and use
> > > NULL value to check for..?
> > >
> >
> > I'm not sure what you mean here. clk_mbus will hold a reference to a clock
> > retrieved by devm_clk_get() so it has to be "struct clk *".
> >
> > What I'm missing here?
>
> IIRC there were two variable one clk ptr and one an integer to mark
> presence, you may be able to skip variable and use ptr..

If we're doing that, then we would effectively make it optional. That
DMA engine cannot operate without it.

Maxime

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

Patch

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 0cd13f17fc11..761555080325 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -129,6 +129,7 @@  struct sun6i_dma_config {
 	u32 dst_burst_lengths;
 	u32 src_addr_widths;
 	u32 dst_addr_widths;
+	bool mbus_clk;
 };
 
 /*
@@ -182,6 +183,7 @@  struct sun6i_dma_dev {
 	struct dma_device	slave;
 	void __iomem		*base;
 	struct clk		*clk;
+	struct clk		*clk_mbus;
 	int			irq;
 	spinlock_t		lock;
 	struct reset_control	*rstc;
@@ -1208,6 +1210,14 @@  static int sun6i_dma_probe(struct platform_device *pdev)
 		return PTR_ERR(sdc->clk);
 	}
 
+	if (sdc->cfg->mbus_clk) {
+		sdc->clk_mbus = devm_clk_get(&pdev->dev, "mbus");
+		if (IS_ERR(sdc->clk_mbus)) {
+			dev_err(&pdev->dev, "No mbus clock specified\n");
+			return PTR_ERR(sdc->clk_mbus);
+		}
+	}
+
 	sdc->rstc = devm_reset_control_get(&pdev->dev, NULL);
 	if (IS_ERR(sdc->rstc)) {
 		dev_err(&pdev->dev, "No reset controller specified\n");
@@ -1312,11 +1322,19 @@  static int sun6i_dma_probe(struct platform_device *pdev)
 		goto err_reset_assert;
 	}
 
+	if (sdc->cfg->mbus_clk) {
+		ret = clk_prepare_enable(sdc->clk_mbus);
+		if (ret) {
+			dev_err(&pdev->dev, "Couldn't enable mbus clock\n");
+			goto err_clk_disable;
+		}
+	}
+
 	ret = devm_request_irq(&pdev->dev, sdc->irq, sun6i_dma_interrupt, 0,
 			       dev_name(&pdev->dev), sdc);
 	if (ret) {
 		dev_err(&pdev->dev, "Cannot request IRQ\n");
-		goto err_clk_disable;
+		goto err_mbus_clk_disable;
 	}
 
 	ret = dma_async_device_register(&sdc->slave);
@@ -1341,6 +1359,8 @@  static int sun6i_dma_probe(struct platform_device *pdev)
 	dma_async_device_unregister(&sdc->slave);
 err_irq_disable:
 	sun6i_kill_tasklet(sdc);
+err_mbus_clk_disable:
+	clk_disable_unprepare(sdc->clk_mbus);
 err_clk_disable:
 	clk_disable_unprepare(sdc->clk);
 err_reset_assert:
@@ -1359,6 +1379,7 @@  static int sun6i_dma_remove(struct platform_device *pdev)
 
 	sun6i_kill_tasklet(sdc);
 
+	clk_disable_unprepare(sdc->clk_mbus);
 	clk_disable_unprepare(sdc->clk);
 	reset_control_assert(sdc->rstc);