| Message ID | 20190307165829.9086-3-jernej.skrabec@siol.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allwinner H6 DMA support | expand |
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 >
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
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);
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..
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
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);
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(-)