Re: [PATCH 2/4] dmaengine: dw_dmac: move to generic DMA binding
On Sat, Feb 16, 2013 at 4:00 PM, Arnd Bergmann wrote: > On Saturday 16 February 2013, Andy Shevchenko wrote: >> On Fri, Feb 15, 2013 at 8:21 PM, Arnd Bergmann wrote: >> > + if (WARN_ON(fargs.req >= 16 || fargs.src >= 2 || fargs.dst >= 2)) >> > + return NULL; >> >> 16 here is a magic number for me. I would like to see something like >> #define DW_MAX_REQUEST_LINES 16 in the dw_dmac_regs.h. > > Ok. > >> Besides of that, what 2 does come from? > > I thought that Viresh had commented that there could only be two masters. > It's probably best to compare against dw->nr_masters here. Right. >> > @@ -1765,6 +1751,10 @@ static int dw_probe(struct platform_device *pdev) >> > >> > dma_async_device_register(>dma); >> > >> > + err = of_dma_controller_register(pdev->dev.of_node, dw_dma_xlate, >> > dw); >> > + if (err) >> > + dev_err(>dev, "could not register >> > of_dma_controller\n"); >> >> It's not an error, dev_dbg. Consider case when !CONFIG_OF. > > Ah right. I expected of_dma_controller_register to return 0 in that case, but > it returns -ENODEV. How about I change this to this? > > if (pdev->dev.of_node) > err = of_dma_controller_register(pdev->dev.of_node, > dw_dma_xlate, dw); > if (err && err != -ENODEV) > dev_err(>dev, "could not register of_dma_controller\n"); > > That would warn only when we have a dw_dmac device that was registered from > device tree but does not follow the binding or gets an -ENOMEM. I'm okay with it. > Thanks for your review! Thanks for the patch! -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] dmaengine: dw_dmac: move to generic DMA binding
On Saturday 16 February 2013, Andy Shevchenko wrote:
> On Fri, Feb 15, 2013 at 8:21 PM, Arnd Bergmann wrote:
>
> > @@ -168,7 +169,13 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
> > if (dwc->initialized == true)
> > return;
> >
> > - if (dws) {
> > + if (dws && dws->cfg_hi == 0x && dws->cfg_lo == 0x) {
> > + /* autoconfigure based on request line from DT */
> > + if (dwc->dma_sconfig.direction == DMA_MEM_TO_DEV)
> > + cfghi = DWC_CFGH_DST_PER(dwc->req);
> > + else if (dwc->dma_sconfig.direction == DMA_DEV_TO_MEM)
> > + cfghi = DWC_CFGH_SRC_PER(dwc->req);
>
> Please, use dwc->direction instead of field in the slave_config. If I
> remember correctly it's marked like obsoleted/deprecated.
Ok, that's easy to change. I was copying from the code you added
a few lines below, but was using an older version than the one where
you had made the change to use dwc->direction.
> > @@ -1179,49 +1186,61 @@ static void dwc_free_chan_resources(struct dma_chan
> > *chan)
> > dev_vdbg(chan2dev(chan), "%s: done\n", __func__);
> > }
> >
> > -bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
> > +struct dw_dma_filter_args {
> > + struct dw_dma *dw;
> > + u32 req;
>
> Why this is u32 and not unsigned int?
>
> > + u32 src;
> > + u32 dst;
>
> And this could be also just unsigned int.
I was using u32 since I copied the values from a 32 bit
DT property value. I'll change it to unsigned int.
> > +static bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
> > {
>
> > + dws->cfg_hi = 0x;
> > + dws->cfg_lo = 0x;
>
> Agree with Russell about ~0.
ok.
> > +static struct dma_chan *dw_dma_xlate(struct of_phandle_args *dma_spec,
> > +struct of_dma *ofdma)
> > +{
> > + struct dw_dma *dw = ofdma->of_dma_data;
> > + struct dw_dma_filter_args fargs = {
> > + .dw = dw,
> > + };
> > + dma_cap_mask_t cap;
> > +
> > + if (dma_spec->args_count != 3)
> > + return NULL;
> > +
> > + fargs.req = be32_to_cpup(dma_spec->args+0);
> > + fargs.src = be32_to_cpup(dma_spec->args+1);
> > + fargs.dst = be32_to_cpup(dma_spec->args+2);
>
> You could cast them to usual C types like unsigned int. I see u32 in
> rare cases in the driver like for reading/writting from/to hw and when
> API contains it. Here I doubt we have to leave them as u32.
Right.
> > + if (WARN_ON(fargs.req >= 16 || fargs.src >= 2 || fargs.dst >= 2))
> > + return NULL;
>
> 16 here is a magic number for me. I would like to see something like
> #define DW_MAX_REQUEST_LINES 16 in the dw_dmac_regs.h.
Ok.
> Besides of that, what 2 does come from?
I thought that Viresh had commented that there could only be two masters.
It's probably best to compare against dw->nr_masters here.
> > @@ -1765,6 +1751,10 @@ static int dw_probe(struct platform_device *pdev)
> >
> > dma_async_device_register(>dma);
> >
> > + err = of_dma_controller_register(pdev->dev.of_node, dw_dma_xlate,
> > dw);
> > + if (err)
> > + dev_err(>dev, "could not register
> > of_dma_controller\n");
>
> It's not an error, dev_dbg. Consider case when !CONFIG_OF.
Ah right. I expected of_dma_controller_register to return 0 in that case, but
it returns -ENODEV. How about I change this to this?
if (pdev->dev.of_node)
err = of_dma_controller_register(pdev->dev.of_node,
dw_dma_xlate, dw);
if (err && err != -ENODEV)
dev_err(>dev, "could not register of_dma_controller\n");
That would warn only when we have a dw_dmac device that was registered from
device tree but does not follow the binding or gets an -ENOMEM.
> > --- a/drivers/dma/dw_dmac_regs.h
> > +++ b/drivers/dma/dw_dmac_regs.h
> > @@ -213,6 +213,10 @@ struct dw_dma_chan {
> > /* configuration passed via DMA_SLAVE_CONFIG */
> > struct dma_slave_config dma_sconfig;
> >
> > + /* slave configuration from DT */
> > + unsigned intreq;
>
> Could you use here full name like "request_line"? And I think the
> better place for it in subsection "hardware configuration" (consider
> non-DT cases of use).
Ok
>
> > /* backlink to dw_dma */
> > struct dw_dma *dw;
> > };
>
> We should not have this in linux-next. Are you sure you rebased it on
> top of recent one?
I was basing on the earliest commit that had Viresh's changes in it.
I'll rebase on top of Vinod's branch now.
Thanks for your review!
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] dmaengine: dw_dmac: move to generic DMA binding
On Saturday 16 February 2013, Russell King - ARM Linux wrote:
> On Sat, Feb 16, 2013 at 10:07:39AM +, Arnd Bergmann wrote:
> > On Saturday 16 February 2013, Viresh Kumar wrote:
> > > On 15 February 2013 23:51, Arnd Bergmann wrote:
> > > > +static bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
> > > > {
> > >
> > > > + dws->cfg_hi = 0x;
> > > > + dws->cfg_lo = 0x;
> > >
> > > s/0x/-1 ?
> >
> > It's an 'unsigned int'. While -1 would work here, I always find it a little
> > odd to rely on that feature of the C language.
>
> However, relying on an 'int' being 32-bits is also rather odd, and
> probably much more dubious too. If you want to set all bits in an
> int, the portable way to do that is ~0.
Right, I can do that, too. All I really need here though is to make sure
I use the same value in this place and when comparing it in dwc_initialize,
and it needs to be something that cannot be a valid register setting.
Thanks,
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] dmaengine: dw_dmac: move to generic DMA binding
On Fri, Feb 15, 2013 at 8:21 PM, Arnd Bergmann wrote:
> The original device tree binding for this driver, from Viresh Kumar
> unfortunately conflicted with the generic DMA binding, and did not allow
> to completely seperate slave device configuration from the controller.
>
> This is an attempt to replace it with an implementation of the generic
> binding, but it is currently completely untested, because I do not have
> any hardware with this particular controller.
>
> The patch applies on top of linux-next, which contains both the base
> support for the generic DMA binding, as well as the earlier attempt from
> Viresh. Both of these are currently not merged upstream however.
>
> There are a couple of TODO items that are left remaining and are open
> for ideas from other people.
> --- a/drivers/dma/dw_dmac.c
> +++ b/drivers/dma/dw_dmac.c
> @@ -168,7 +169,13 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
> if (dwc->initialized == true)
> return;
>
> - if (dws) {
> + if (dws && dws->cfg_hi == 0x && dws->cfg_lo == 0x) {
> + /* autoconfigure based on request line from DT */
> + if (dwc->dma_sconfig.direction == DMA_MEM_TO_DEV)
> + cfghi = DWC_CFGH_DST_PER(dwc->req);
> + else if (dwc->dma_sconfig.direction == DMA_DEV_TO_MEM)
> + cfghi = DWC_CFGH_SRC_PER(dwc->req);
Please, use dwc->direction instead of field in the slave_config. If I
remember correctly it's marked like obsoleted/deprecated.
> @@ -1179,49 +1186,61 @@ static void dwc_free_chan_resources(struct dma_chan
> *chan)
> dev_vdbg(chan2dev(chan), "%s: done\n", __func__);
> }
>
> -bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
> +struct dw_dma_filter_args {
> + struct dw_dma *dw;
> + u32 req;
Why this is u32 and not unsigned int?
> + u32 src;
> + u32 dst;
And this could be also just unsigned int.
> +static bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
> {
> + dws->cfg_hi = 0x;
> + dws->cfg_lo = 0x;
Agree with Russell about ~0.
> +static struct dma_chan *dw_dma_xlate(struct of_phandle_args *dma_spec,
> +struct of_dma *ofdma)
> +{
> + struct dw_dma *dw = ofdma->of_dma_data;
> + struct dw_dma_filter_args fargs = {
> + .dw = dw,
> + };
> + dma_cap_mask_t cap;
> +
> + if (dma_spec->args_count != 3)
> + return NULL;
> +
> + fargs.req = be32_to_cpup(dma_spec->args+0);
> + fargs.src = be32_to_cpup(dma_spec->args+1);
> + fargs.dst = be32_to_cpup(dma_spec->args+2);
You could cast them to usual C types like unsigned int. I see u32 in
rare cases in the driver like for reading/writting from/to hw and when
API contains it. Here I doubt we have to leave them as u32.
> +
> + if (WARN_ON(fargs.req >= 16 || fargs.src >= 2 || fargs.dst >= 2))
> + return NULL;
16 here is a magic number for me. I would like to see something like
#define DW_MAX_REQUEST_LINES 16 in the dw_dmac_regs.h.
Besides of that, what 2 does come from?
> @@ -1765,6 +1751,10 @@ static int dw_probe(struct platform_device *pdev)
>
> dma_async_device_register(>dma);
>
> + err = of_dma_controller_register(pdev->dev.of_node, dw_dma_xlate, dw);
> + if (err)
> + dev_err(>dev, "could not register of_dma_controller\n");
It's not an error, dev_dbg. Consider case when !CONFIG_OF.
> --- a/drivers/dma/dw_dmac_regs.h
> +++ b/drivers/dma/dw_dmac_regs.h
> @@ -213,6 +213,10 @@ struct dw_dma_chan {
> /* configuration passed via DMA_SLAVE_CONFIG */
> struct dma_slave_config dma_sconfig;
>
> + /* slave configuration from DT */
> + unsigned intreq;
Could you use here full name like "request_line"? And I think the
better place for it in subsection "hardware configuration" (consider
non-DT cases of use).
> /* backlink to dw_dma */
> struct dw_dma *dw;
> };
We should not have this in linux-next. Are you sure you rebased it on
top of recent one?
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] dmaengine: dw_dmac: move to generic DMA binding
On Sat, Feb 16, 2013 at 10:07:39AM +, Arnd Bergmann wrote:
> On Saturday 16 February 2013, Viresh Kumar wrote:
> > On 15 February 2013 23:51, Arnd Bergmann wrote:
> > > +static bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
> > > {
> >
> > > + dws->cfg_hi = 0x;
> > > + dws->cfg_lo = 0x;
> >
> > s/0x/-1 ?
>
> It's an 'unsigned int'. While -1 would work here, I always find it a little
> odd to rely on that feature of the C language.
However, relying on an 'int' being 32-bits is also rather odd, and
probably much more dubious too. If you want to set all bits in an
int, the portable way to do that is ~0.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] dmaengine: dw_dmac: move to generic DMA binding
On Saturday 16 February 2013, Viresh Kumar wrote:
> On 15 February 2013 23:51, Arnd Bergmann wrote:
> > diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt
> > b/Documentation/devicetree/bindings/dma/snps-dma.txt
> > +- #dma-cells: must be <3>
>
> > +DMA clients connected to the Designware DMA controller must use the format
> > +described in the dma.txt file, using a five-cell specifier for each
> > channel.
>
> s/five/four ?
Right. I thought I had fixed up all instances.
> > +static bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
> > {
>
> > + dws->cfg_hi = 0x;
> > + dws->cfg_lo = 0x;
>
> s/0x/-1 ?
It's an 'unsigned int'. While -1 would work here, I always find it a little
odd to rely on that feature of the C language.
Thannks for the review.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] dmaengine: dw_dmac: move to generic DMA binding
On Sat, Feb 16, 2013 at 10:07:39AM +, Arnd Bergmann wrote:
On Saturday 16 February 2013, Viresh Kumar wrote:
On 15 February 2013 23:51, Arnd Bergmann a...@arndb.de wrote:
+static bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
{
+ dws-cfg_hi = 0x;
+ dws-cfg_lo = 0x;
s/0x/-1 ?
It's an 'unsigned int'. While -1 would work here, I always find it a little
odd to rely on that feature of the C language.
However, relying on an 'int' being 32-bits is also rather odd, and
probably much more dubious too. If you want to set all bits in an
int, the portable way to do that is ~0.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] dmaengine: dw_dmac: move to generic DMA binding
On Fri, Feb 15, 2013 at 8:21 PM, Arnd Bergmann a...@arndb.de wrote:
The original device tree binding for this driver, from Viresh Kumar
unfortunately conflicted with the generic DMA binding, and did not allow
to completely seperate slave device configuration from the controller.
This is an attempt to replace it with an implementation of the generic
binding, but it is currently completely untested, because I do not have
any hardware with this particular controller.
The patch applies on top of linux-next, which contains both the base
support for the generic DMA binding, as well as the earlier attempt from
Viresh. Both of these are currently not merged upstream however.
There are a couple of TODO items that are left remaining and are open
for ideas from other people.
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -168,7 +169,13 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
if (dwc-initialized == true)
return;
- if (dws) {
+ if (dws dws-cfg_hi == 0x dws-cfg_lo == 0x) {
+ /* autoconfigure based on request line from DT */
+ if (dwc-dma_sconfig.direction == DMA_MEM_TO_DEV)
+ cfghi = DWC_CFGH_DST_PER(dwc-req);
+ else if (dwc-dma_sconfig.direction == DMA_DEV_TO_MEM)
+ cfghi = DWC_CFGH_SRC_PER(dwc-req);
Please, use dwc-direction instead of field in the slave_config. If I
remember correctly it's marked like obsoleted/deprecated.
@@ -1179,49 +1186,61 @@ static void dwc_free_chan_resources(struct dma_chan
*chan)
dev_vdbg(chan2dev(chan), %s: done\n, __func__);
}
-bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
+struct dw_dma_filter_args {
+ struct dw_dma *dw;
+ u32 req;
Why this is u32 and not unsigned int?
+ u32 src;
+ u32 dst;
And this could be also just unsigned int.
+static bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
{
+ dws-cfg_hi = 0x;
+ dws-cfg_lo = 0x;
Agree with Russell about ~0.
+static struct dma_chan *dw_dma_xlate(struct of_phandle_args *dma_spec,
+struct of_dma *ofdma)
+{
+ struct dw_dma *dw = ofdma-of_dma_data;
+ struct dw_dma_filter_args fargs = {
+ .dw = dw,
+ };
+ dma_cap_mask_t cap;
+
+ if (dma_spec-args_count != 3)
+ return NULL;
+
+ fargs.req = be32_to_cpup(dma_spec-args+0);
+ fargs.src = be32_to_cpup(dma_spec-args+1);
+ fargs.dst = be32_to_cpup(dma_spec-args+2);
You could cast them to usual C types like unsigned int. I see u32 in
rare cases in the driver like for reading/writting from/to hw and when
API contains it. Here I doubt we have to leave them as u32.
+
+ if (WARN_ON(fargs.req = 16 || fargs.src = 2 || fargs.dst = 2))
+ return NULL;
16 here is a magic number for me. I would like to see something like
#define DW_MAX_REQUEST_LINES 16 in the dw_dmac_regs.h.
Besides of that, what 2 does come from?
@@ -1765,6 +1751,10 @@ static int dw_probe(struct platform_device *pdev)
dma_async_device_register(dw-dma);
+ err = of_dma_controller_register(pdev-dev.of_node, dw_dma_xlate, dw);
+ if (err)
+ dev_err(pdev-dev, could not register of_dma_controller\n);
It's not an error, dev_dbg. Consider case when !CONFIG_OF.
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -213,6 +213,10 @@ struct dw_dma_chan {
/* configuration passed via DMA_SLAVE_CONFIG */
struct dma_slave_config dma_sconfig;
+ /* slave configuration from DT */
+ unsigned intreq;
Could you use here full name like request_line? And I think the
better place for it in subsection hardware configuration (consider
non-DT cases of use).
/* backlink to dw_dma */
struct dw_dma *dw;
};
We should not have this in linux-next. Are you sure you rebased it on
top of recent one?
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] dmaengine: dw_dmac: move to generic DMA binding
On Saturday 16 February 2013, Russell King - ARM Linux wrote:
On Sat, Feb 16, 2013 at 10:07:39AM +, Arnd Bergmann wrote:
On Saturday 16 February 2013, Viresh Kumar wrote:
On 15 February 2013 23:51, Arnd Bergmann a...@arndb.de wrote:
+static bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
{
+ dws-cfg_hi = 0x;
+ dws-cfg_lo = 0x;
s/0x/-1 ?
It's an 'unsigned int'. While -1 would work here, I always find it a little
odd to rely on that feature of the C language.
However, relying on an 'int' being 32-bits is also rather odd, and
probably much more dubious too. If you want to set all bits in an
int, the portable way to do that is ~0.
Right, I can do that, too. All I really need here though is to make sure
I use the same value in this place and when comparing it in dwc_initialize,
and it needs to be something that cannot be a valid register setting.
Thanks,
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] dmaengine: dw_dmac: move to generic DMA binding
On Saturday 16 February 2013, Andy Shevchenko wrote:
On Fri, Feb 15, 2013 at 8:21 PM, Arnd Bergmann a...@arndb.de wrote:
@@ -168,7 +169,13 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
if (dwc-initialized == true)
return;
- if (dws) {
+ if (dws dws-cfg_hi == 0x dws-cfg_lo == 0x) {
+ /* autoconfigure based on request line from DT */
+ if (dwc-dma_sconfig.direction == DMA_MEM_TO_DEV)
+ cfghi = DWC_CFGH_DST_PER(dwc-req);
+ else if (dwc-dma_sconfig.direction == DMA_DEV_TO_MEM)
+ cfghi = DWC_CFGH_SRC_PER(dwc-req);
Please, use dwc-direction instead of field in the slave_config. If I
remember correctly it's marked like obsoleted/deprecated.
Ok, that's easy to change. I was copying from the code you added
a few lines below, but was using an older version than the one where
you had made the change to use dwc-direction.
@@ -1179,49 +1186,61 @@ static void dwc_free_chan_resources(struct dma_chan
*chan)
dev_vdbg(chan2dev(chan), %s: done\n, __func__);
}
-bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
+struct dw_dma_filter_args {
+ struct dw_dma *dw;
+ u32 req;
Why this is u32 and not unsigned int?
+ u32 src;
+ u32 dst;
And this could be also just unsigned int.
I was using u32 since I copied the values from a 32 bit
DT property value. I'll change it to unsigned int.
+static bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
{
+ dws-cfg_hi = 0x;
+ dws-cfg_lo = 0x;
Agree with Russell about ~0.
ok.
+static struct dma_chan *dw_dma_xlate(struct of_phandle_args *dma_spec,
+struct of_dma *ofdma)
+{
+ struct dw_dma *dw = ofdma-of_dma_data;
+ struct dw_dma_filter_args fargs = {
+ .dw = dw,
+ };
+ dma_cap_mask_t cap;
+
+ if (dma_spec-args_count != 3)
+ return NULL;
+
+ fargs.req = be32_to_cpup(dma_spec-args+0);
+ fargs.src = be32_to_cpup(dma_spec-args+1);
+ fargs.dst = be32_to_cpup(dma_spec-args+2);
You could cast them to usual C types like unsigned int. I see u32 in
rare cases in the driver like for reading/writting from/to hw and when
API contains it. Here I doubt we have to leave them as u32.
Right.
+ if (WARN_ON(fargs.req = 16 || fargs.src = 2 || fargs.dst = 2))
+ return NULL;
16 here is a magic number for me. I would like to see something like
#define DW_MAX_REQUEST_LINES 16 in the dw_dmac_regs.h.
Ok.
Besides of that, what 2 does come from?
I thought that Viresh had commented that there could only be two masters.
It's probably best to compare against dw-nr_masters here.
@@ -1765,6 +1751,10 @@ static int dw_probe(struct platform_device *pdev)
dma_async_device_register(dw-dma);
+ err = of_dma_controller_register(pdev-dev.of_node, dw_dma_xlate,
dw);
+ if (err)
+ dev_err(pdev-dev, could not register
of_dma_controller\n);
It's not an error, dev_dbg. Consider case when !CONFIG_OF.
Ah right. I expected of_dma_controller_register to return 0 in that case, but
it returns -ENODEV. How about I change this to this?
if (pdev-dev.of_node)
err = of_dma_controller_register(pdev-dev.of_node,
dw_dma_xlate, dw);
if (err err != -ENODEV)
dev_err(pdev-dev, could not register of_dma_controller\n);
That would warn only when we have a dw_dmac device that was registered from
device tree but does not follow the binding or gets an -ENOMEM.
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -213,6 +213,10 @@ struct dw_dma_chan {
/* configuration passed via DMA_SLAVE_CONFIG */
struct dma_slave_config dma_sconfig;
+ /* slave configuration from DT */
+ unsigned intreq;
Could you use here full name like request_line? And I think the
better place for it in subsection hardware configuration (consider
non-DT cases of use).
Ok
/* backlink to dw_dma */
struct dw_dma *dw;
};
We should not have this in linux-next. Are you sure you rebased it on
top of recent one?
I was basing on the earliest commit that had Viresh's changes in it.
I'll rebase on top of Vinod's branch now.
Thanks for your review!
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] dmaengine: dw_dmac: move to generic DMA binding
On Sat, Feb 16, 2013 at 4:00 PM, Arnd Bergmann a...@arndb.de wrote: On Saturday 16 February 2013, Andy Shevchenko wrote: On Fri, Feb 15, 2013 at 8:21 PM, Arnd Bergmann a...@arndb.de wrote: + if (WARN_ON(fargs.req = 16 || fargs.src = 2 || fargs.dst = 2)) + return NULL; 16 here is a magic number for me. I would like to see something like #define DW_MAX_REQUEST_LINES 16 in the dw_dmac_regs.h. Ok. Besides of that, what 2 does come from? I thought that Viresh had commented that there could only be two masters. It's probably best to compare against dw-nr_masters here. Right. @@ -1765,6 +1751,10 @@ static int dw_probe(struct platform_device *pdev) dma_async_device_register(dw-dma); + err = of_dma_controller_register(pdev-dev.of_node, dw_dma_xlate, dw); + if (err) + dev_err(pdev-dev, could not register of_dma_controller\n); It's not an error, dev_dbg. Consider case when !CONFIG_OF. Ah right. I expected of_dma_controller_register to return 0 in that case, but it returns -ENODEV. How about I change this to this? if (pdev-dev.of_node) err = of_dma_controller_register(pdev-dev.of_node, dw_dma_xlate, dw); if (err err != -ENODEV) dev_err(pdev-dev, could not register of_dma_controller\n); That would warn only when we have a dw_dmac device that was registered from device tree but does not follow the binding or gets an -ENOMEM. I'm okay with it. Thanks for your review! Thanks for the patch! -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] dmaengine: dw_dmac: move to generic DMA binding
On 15 February 2013 23:51, Arnd Bergmann wrote:
> diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt
> b/Documentation/devicetree/bindings/dma/snps-dma.txt
> +- #dma-cells: must be <3>
> +DMA clients connected to the Designware DMA controller must use the format
> +described in the dma.txt file, using a five-cell specifier for each channel.
s/five/four ?
> +The four cells in order are:
> +
> +1. A phandle pointing to the DMA controller
> +2. The DMA request line number
> +3. Source master for transfers on allocated channel
> +4. Destination master for transfers on allocated channel
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> +static bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
> {
> + dws->cfg_hi = 0x;
> + dws->cfg_lo = 0x;
s/0x/-1 ?
> + dws->src_master = fargs->src;
> + dws->dst_master = fargs->dst;
> + dwc->req= fargs->req;
>
> - return true;
> - }
> - }
> + chan->private = dws;
> +
> + return true;
> +}
> +
> +static struct dma_chan *dw_dma_xlate(struct of_phandle_args *dma_spec,
> +struct of_dma *ofdma)
> +{
> + struct dw_dma *dw = ofdma->of_dma_data;
> + struct dw_dma_filter_args fargs = {
> + .dw = dw,
> + };
> + dma_cap_mask_t cap;
> +
> + if (dma_spec->args_count != 3)
> + return NULL;
> +
> + fargs.req = be32_to_cpup(dma_spec->args+0);
> + fargs.src = be32_to_cpup(dma_spec->args+1);
> + fargs.dst = be32_to_cpup(dma_spec->args+2);
> +
> + if (WARN_ON(fargs.req >= 16 || fargs.src >= 2 || fargs.dst >= 2))
> + return NULL;
>
> - last_dw = dw;
> - last_bus_id = param;
> - return false;
> + dma_cap_zero(cap);
> + dma_cap_set(DMA_SLAVE, cap);
> +
> + /* TODO: there should be a simpler way to do this */
> + return dma_request_channel(cap, dw_dma_generic_filter,
> _spec->args[0]);
> }
> -EXPORT_SYMBOL(dw_dma_generic_filter);
>
> /* - Cyclic DMA API extensions */
>
> @@ -1510,9 +1529,8 @@ static void dw_dma_off(struct dw_dma *dw)
> static struct dw_dma_platform_data *
> dw_dma_parse_dt(struct platform_device *pdev)
> {
> - struct device_node *sn, *cn, *np = pdev->dev.of_node;
> + struct device_node *np = pdev->dev.of_node;
> struct dw_dma_platform_data *pdata;
> - struct dw_dma_slave *sd;
> u32 tmp, arr[4];
>
> if (!np) {
> @@ -1524,7 +1542,7 @@ dw_dma_parse_dt(struct platform_device *pdev)
> if (!pdata)
> return NULL;
>
> - if (of_property_read_u32(np, "nr_channels", >nr_channels))
> + if (of_property_read_u32(np, "dma-channels", >nr_channels))
> return NULL;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] dmaengine: dw_dmac: move to generic DMA binding
On 15 February 2013 23:51, Arnd Bergmann a...@arndb.de wrote:
diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt
b/Documentation/devicetree/bindings/dma/snps-dma.txt
+- #dma-cells: must be 3
+DMA clients connected to the Designware DMA controller must use the format
+described in the dma.txt file, using a five-cell specifier for each channel.
s/five/four ?
+The four cells in order are:
+
+1. A phandle pointing to the DMA controller
+2. The DMA request line number
+3. Source master for transfers on allocated channel
+4. Destination master for transfers on allocated channel
diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
+static bool dw_dma_generic_filter(struct dma_chan *chan, void *param)
{
+ dws-cfg_hi = 0x;
+ dws-cfg_lo = 0x;
s/0x/-1 ?
+ dws-src_master = fargs-src;
+ dws-dst_master = fargs-dst;
+ dwc-req= fargs-req;
- return true;
- }
- }
+ chan-private = dws;
+
+ return true;
+}
+
+static struct dma_chan *dw_dma_xlate(struct of_phandle_args *dma_spec,
+struct of_dma *ofdma)
+{
+ struct dw_dma *dw = ofdma-of_dma_data;
+ struct dw_dma_filter_args fargs = {
+ .dw = dw,
+ };
+ dma_cap_mask_t cap;
+
+ if (dma_spec-args_count != 3)
+ return NULL;
+
+ fargs.req = be32_to_cpup(dma_spec-args+0);
+ fargs.src = be32_to_cpup(dma_spec-args+1);
+ fargs.dst = be32_to_cpup(dma_spec-args+2);
+
+ if (WARN_ON(fargs.req = 16 || fargs.src = 2 || fargs.dst = 2))
+ return NULL;
- last_dw = dw;
- last_bus_id = param;
- return false;
+ dma_cap_zero(cap);
+ dma_cap_set(DMA_SLAVE, cap);
+
+ /* TODO: there should be a simpler way to do this */
+ return dma_request_channel(cap, dw_dma_generic_filter,
dma_spec-args[0]);
}
-EXPORT_SYMBOL(dw_dma_generic_filter);
/* - Cyclic DMA API extensions */
@@ -1510,9 +1529,8 @@ static void dw_dma_off(struct dw_dma *dw)
static struct dw_dma_platform_data *
dw_dma_parse_dt(struct platform_device *pdev)
{
- struct device_node *sn, *cn, *np = pdev-dev.of_node;
+ struct device_node *np = pdev-dev.of_node;
struct dw_dma_platform_data *pdata;
- struct dw_dma_slave *sd;
u32 tmp, arr[4];
if (!np) {
@@ -1524,7 +1542,7 @@ dw_dma_parse_dt(struct platform_device *pdev)
if (!pdata)
return NULL;
- if (of_property_read_u32(np, nr_channels, pdata-nr_channels))
+ if (of_property_read_u32(np, dma-channels, pdata-nr_channels))
return NULL;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
