| Message ID | 1547658629-25378-4-git-send-email-john.stultz@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | k3dma patches to add support for hi3660/HiKey960 | expand |
On 16-01-19, 09:10, John Stultz wrote: > From: Youlin Wang <wwx575822@notesmail.huawei.com> > > On the hi3660 hardware there are two (at least) DMA controllers, > the DMA-P (Peripherial DMA) and the DMA-A (Audio DMA). The ^^^ typo > two blocks are similar, but have some slight differences. This > resulted in the vendor implementing two separate drivers, which > after review, they have been able to condense and re-use the > existing k3dma driver. > > Thus, this patch adds support for the new "hisi-pcm-asp-dma-1.0" > compatible string in the binding. > > One difference with the DMA-A controller, is that it does not > need to initialize a clock. So we skip this by adding and using > soc data flags. > > After above this driver will support both k3 and hisi_asp dma > hardware. Great thanks for the effort! > > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Vinod Koul <vkoul@kernel.org> > Cc: Zhuangluan Su <suzhuangluan@hisilicon.com> > Cc: Ryan Grachek <ryan@edited.us> > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Cc: dmaengine@vger.kernel.org > Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Signed-off-by: Youlin Wang <wwx575822@notesmail.huawei.com> > Signed-off-by: Tanglei Han <hantanglei@huawei.com> > [jstultz: Reworked to use of_match_data, commit msg improvements] > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > v2: > * Reworked to use of_match_data > v3: > * Further rework of the commit message > --- > drivers/dma/k3dma.c | 37 ++++++++++++++++++++++++++++++++----- > 1 file changed, 32 insertions(+), 5 deletions(-) > > diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c > index fdec2b6..df61406 100644 > --- a/drivers/dma/k3dma.c > +++ b/drivers/dma/k3dma.c > @@ -116,6 +116,13 @@ struct k3_dma_dev { > unsigned int irq; > }; > > + > +#define K3_FLAG_NOCLK (1<<0) why not use BIT() space between two please > +struct k3dma_soc_data { > + unsigned long flags; > +}; > + > + > #define to_k3_dma(dmadev) container_of(dmadev, struct k3_dma_dev, slave) > > static int k3_dma_config_write(struct dma_chan *chan, > @@ -790,8 +797,21 @@ static int k3_dma_transfer_resume(struct dma_chan *chan) > return 0; > } > > +static const struct k3dma_soc_data k3_v1_dma_data = { > + .flags = 0, > +}; So basically this is default right, do we need to set dma_data and not assume default.. > + > +static const struct k3dma_soc_data asp_v1_dma_data = { > + .flags = K3_FLAG_NOCLK, > +}; > + > static const struct of_device_id k3_pdma_dt_ids[] = { > - { .compatible = "hisilicon,k3-dma-1.0", }, > + { .compatible = "hisilicon,k3-dma-1.0", > + .data = &k3_v1_dma_data > + }, > + { .compatible = "hisilicon,hisi-pcm-asp-dma-1.0", > + .data = &asp_v1_dma_data > + }, > {} > }; > MODULE_DEVICE_TABLE(of, k3_pdma_dt_ids); > @@ -810,6 +830,7 @@ static struct dma_chan *k3_of_dma_simple_xlate(struct of_phandle_args *dma_spec, > > static int k3_dma_probe(struct platform_device *op) > { > + const struct k3dma_soc_data *soc_data; > struct k3_dma_dev *d; > const struct of_device_id *of_id; > struct resource *iores; > @@ -823,6 +844,10 @@ static int k3_dma_probe(struct platform_device *op) > if (!d) > return -ENOMEM; > > + soc_data = device_get_match_data(&op->dev); > + if (!soc_data) > + return -EINVAL; So this is not optional, either ways fine by me :) -- ~Vinod
On Sun, Jan 20, 2019 at 3:12 AM Vinod Koul <vkoul@kernel.org> wrote: > > On 16-01-19, 09:10, John Stultz wrote: > > From: Youlin Wang <wwx575822@notesmail.huawei.com> > > > > On the hi3660 hardware there are two (at least) DMA controllers, > > the DMA-P (Peripherial DMA) and the DMA-A (Audio DMA). The > ^^^ > typo Thanks so much for the review! Fixed! > > + > > +#define K3_FLAG_NOCLK (1<<0) > > why not use BIT() > > space between two please Fixed. > > +static const struct k3dma_soc_data k3_v1_dma_data = { > > + .flags = 0, > > +}; > > So basically this is default right, do we need to set dma_data and not > assume default.. I'm not sure I fully understand you here. Basically I'm just creating a per-variant data structure. The k3_v1_dma_data isn't really the default, but is the first variant supported. There may be future variants that cause some new flag that the k3_v3_dma_data may need to set. But for now that variant doesn't have any flags set. > > + > > +static const struct k3dma_soc_data asp_v1_dma_data = { > > + .flags = K3_FLAG_NOCLK, > > +}; > > + > > static const struct of_device_id k3_pdma_dt_ids[] = { > > - { .compatible = "hisilicon,k3-dma-1.0", }, > > + { .compatible = "hisilicon,k3-dma-1.0", > > + .data = &k3_v1_dma_data > > + }, > > + { .compatible = "hisilicon,hisi-pcm-asp-dma-1.0", > > + .data = &asp_v1_dma_data > > + }, > > {} > > }; > > MODULE_DEVICE_TABLE(of, k3_pdma_dt_ids); > > @@ -810,6 +830,7 @@ static struct dma_chan *k3_of_dma_simple_xlate(struct of_phandle_args *dma_spec, > > > > static int k3_dma_probe(struct platform_device *op) > > { > > + const struct k3dma_soc_data *soc_data; > > struct k3_dma_dev *d; > > const struct of_device_id *of_id; > > struct resource *iores; > > @@ -823,6 +844,10 @@ static int k3_dma_probe(struct platform_device *op) > > if (!d) > > return -ENOMEM; > > > > + soc_data = device_get_match_data(&op->dev); > > + if (!soc_data) > > + return -EINVAL; > > So this is not optional, either ways fine by me :) I think this way makes sense, but maybe I'm missing a better alternative? Do let me know if there's an example you'd rather I follow. Thanks so much again for the review! -john
On 22-01-19, 15:48, John Stultz wrote: > On Sun, Jan 20, 2019 at 3:12 AM Vinod Koul <vkoul@kernel.org> wrote: > > > > On 16-01-19, 09:10, John Stultz wrote: > > > From: Youlin Wang <wwx575822@notesmail.huawei.com> > > > > > > On the hi3660 hardware there are two (at least) DMA controllers, > > > the DMA-P (Peripherial DMA) and the DMA-A (Audio DMA). The > > ^^^ > > typo > > Thanks so much for the review! > > Fixed! > > > > + > > > +#define K3_FLAG_NOCLK (1<<0) > > > > why not use BIT() > > > > space between two please > > Fixed. > > > > +static const struct k3dma_soc_data k3_v1_dma_data = { > > > + .flags = 0, > > > +}; > > > > So basically this is default right, do we need to set dma_data and not > > assume default.. > > I'm not sure I fully understand you here. Basically I'm just creating > a per-variant data structure. The k3_v1_dma_data isn't really the > default, but is the first variant supported. There may be future > variants that cause some new flag that the k3_v3_dma_data may need to > set. But for now that variant doesn't have any flags set. So my point was we can skip this and treat driver data NULL as default i.e. no flags, no big deal though, saves you dummy k3_v1_dma_data. > > > > > + > > > +static const struct k3dma_soc_data asp_v1_dma_data = { > > > + .flags = K3_FLAG_NOCLK, > > > +}; > > > + > > > static const struct of_device_id k3_pdma_dt_ids[] = { > > > - { .compatible = "hisilicon,k3-dma-1.0", }, > > > + { .compatible = "hisilicon,k3-dma-1.0", > > > + .data = &k3_v1_dma_data > > > + }, > > > + { .compatible = "hisilicon,hisi-pcm-asp-dma-1.0", > > > + .data = &asp_v1_dma_data > > > + }, > > > {} > > > }; > > > MODULE_DEVICE_TABLE(of, k3_pdma_dt_ids); > > > @@ -810,6 +830,7 @@ static struct dma_chan *k3_of_dma_simple_xlate(struct of_phandle_args *dma_spec, > > > > > > static int k3_dma_probe(struct platform_device *op) > > > { > > > + const struct k3dma_soc_data *soc_data; > > > struct k3_dma_dev *d; > > > const struct of_device_id *of_id; > > > struct resource *iores; > > > @@ -823,6 +844,10 @@ static int k3_dma_probe(struct platform_device *op) > > > if (!d) > > > return -ENOMEM; > > > > > > + soc_data = device_get_match_data(&op->dev); > > > + if (!soc_data) > > > + return -EINVAL; > > > > So this is not optional, either ways fine by me :) > > I think this way makes sense, but maybe I'm missing a better alternative? > > Do let me know if there's an example you'd rather I follow. To elaborate I was thinking of alternate scheme with: compatible = "hisilicon,k3-dma-1.0", NULL compatible = "hisilicon,hisi-pcm-asp-dma-1.0", .data = &asp_v1_dma_data and soc_data = device_get_match_data(&op->dev); if (!soc_data) { /* no data so flags are null */ dev_warn(... "no driver data specified, assuming no flags\n" k3_dma->flags = 0; } -- ~Vinod
On Wed, Jan 23, 2019 at 4:57 AM Vinod Koul <vkoul@kernel.org> wrote: > On 22-01-19, 15:48, John Stultz wrote: > > Do let me know if there's an example you'd rather I follow. > > To elaborate I was thinking of alternate scheme with: > > compatible = "hisilicon,k3-dma-1.0", NULL > compatible = "hisilicon,hisi-pcm-asp-dma-1.0", .data = &asp_v1_dma_data > > and > > soc_data = device_get_match_data(&op->dev); > if (!soc_data) { > /* no data so flags are null */ > dev_warn(... "no driver data specified, assuming no flags\n" > k3_dma->flags = 0; > } Thanks for the clarification! Hmm. I can do this, though the trouble is all the soc_data-> references have to switch to a struct k3dma_soc_data on the stack, which we have to initialize/copy over depending on the soc_data return, which I'm not sure really simplifies much (other then saving a ulong off the heap). But I'm ok with either. thanks -john
diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c index fdec2b6..df61406 100644 --- a/drivers/dma/k3dma.c +++ b/drivers/dma/k3dma.c @@ -116,6 +116,13 @@ struct k3_dma_dev { unsigned int irq; }; + +#define K3_FLAG_NOCLK (1<<0) +struct k3dma_soc_data { + unsigned long flags; +}; + + #define to_k3_dma(dmadev) container_of(dmadev, struct k3_dma_dev, slave) static int k3_dma_config_write(struct dma_chan *chan, @@ -790,8 +797,21 @@ static int k3_dma_transfer_resume(struct dma_chan *chan) return 0; } +static const struct k3dma_soc_data k3_v1_dma_data = { + .flags = 0, +}; + +static const struct k3dma_soc_data asp_v1_dma_data = { + .flags = K3_FLAG_NOCLK, +}; + static const struct of_device_id k3_pdma_dt_ids[] = { - { .compatible = "hisilicon,k3-dma-1.0", }, + { .compatible = "hisilicon,k3-dma-1.0", + .data = &k3_v1_dma_data + }, + { .compatible = "hisilicon,hisi-pcm-asp-dma-1.0", + .data = &asp_v1_dma_data + }, {} }; MODULE_DEVICE_TABLE(of, k3_pdma_dt_ids); @@ -810,6 +830,7 @@ static struct dma_chan *k3_of_dma_simple_xlate(struct of_phandle_args *dma_spec, static int k3_dma_probe(struct platform_device *op) { + const struct k3dma_soc_data *soc_data; struct k3_dma_dev *d; const struct of_device_id *of_id; struct resource *iores; @@ -823,6 +844,10 @@ static int k3_dma_probe(struct platform_device *op) if (!d) return -ENOMEM; + soc_data = device_get_match_data(&op->dev); + if (!soc_data) + return -EINVAL; + d->base = devm_ioremap_resource(&op->dev, iores); if (IS_ERR(d->base)) return PTR_ERR(d->base); @@ -835,10 +860,12 @@ static int k3_dma_probe(struct platform_device *op) "dma-requests", &d->dma_requests); } - d->clk = devm_clk_get(&op->dev, NULL); - if (IS_ERR(d->clk)) { - dev_err(&op->dev, "no dma clk\n"); - return PTR_ERR(d->clk); + if (!(soc_data->flags & K3_FLAG_NOCLK)) { + d->clk = devm_clk_get(&op->dev, NULL); + if (IS_ERR(d->clk)) { + dev_err(&op->dev, "no dma clk\n"); + return PTR_ERR(d->clk); + } } irq = platform_get_irq(op, 0);