Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1463722 > unrolled thread

[PATCH] DW: Read "is_memcpy" and "is_nollp" property from device tree.

Back to article view | Back to linux.kernel

Contents

  [PATCH] DW: Read "is_memcpy" and "is_nollp" property from device tree. Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> - 2016-08-16 13:40 +0200
    Re: [PATCH] DW: Read "is_memcpy" and "is_nollp" property from  device tree. Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-08-19 16:40 +0200
      Re: [PATCH] DW: Read "is_memcpy" and "is_nollp" property from  device tree. Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> - 2016-08-23 17:20 +0200
        Re: [PATCH] DW: Read "is_memcpy" and "is_nollp" property from  device tree. Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-08-23 19:10 +0200
          Re: [PATCH] DW: Read "is_memcpy" and "is_nollp" property from device  tree. Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-08-23 19:20 +0200

#1463722 — [PATCH] DW: Read "is_memcpy" and "is_nollp" property from device tree.

DW DMAC on ARC SDP became broken after df5c7386 ("dmaengine: dw: some Intel
devices has no memcpy support") and 30cb2639 ("dmaengine: dw: don't override
platform data with autocfg") commits.

* After df5c7386 commit "DMA_MEMCPY" capability option doesn't get set
correctly in platform driver version.
* After 30cb2639 commit "nollp" parameters don't get set correctly in
platform driver version.

This happens because in old driver version there are three sources of
parameters: pdata, device tree and autoconfig hardware registers. Some
parameters were read from pdata and others from autoconfig hardware
registers. If pdata was absent some pdata structure fields were filled
with parameters from device tree.
But 30cb2639 commit disabled overriding pdata with autocfg, so if we
use platform driver version without pdata some parameters will not be set.
This leads to inoperability of DW DMAC.

This patch adds reading missed parameters from device tree.

Note there's a prerequisite http://www.spinics.net/lists/dmaengine/msg10682.html

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 drivers/dma/dw/platform.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
index 5bda0eb..2712602 100644
--- a/drivers/dma/dw/platform.c
+++ b/drivers/dma/dw/platform.c
@@ -129,6 +129,12 @@ dw_dma_parse_dt(struct platform_device *pdev)
 	if (of_property_read_bool(np, "is_private"))
 		pdata->is_private = true;
 
+	if (of_property_read_bool(np, "is_memcpy"))
+		pdata->is_memcpy = true;
+
+	if (of_property_read_bool(np, "is_nollp"))
+		pdata->is_nollp = true;
+
 	if (!of_property_read_u32(np, "chan_allocation_order", &tmp))
 		pdata->chan_allocation_order = (unsigned char)tmp;
 
-- 
2.5.5

[toc] | [next] | [standalone]

#1466465 — Re: [PATCH] DW: Read "is_memcpy" and "is_nollp" property from device tree.

On Tue, 2016-08-16 at 14:31 +0300, Eugeniy Paltsev wrote:
> DW DMAC on ARC SDP became broken after df5c7386 ("dmaengine: dw: some
> Intel
> devices has no memcpy support") and 30cb2639 ("dmaengine: dw: don't
> override
> platform data with autocfg") commits.

I'm not sure that word 'broken' is a correct one here. Is the platform
code using this driver in the upstream already? If so, where is it
located?

> 
> * After df5c7386 commit "DMA_MEMCPY" capability option doesn't get set
> correctly in platform driver version.
> * After 30cb2639 commit "nollp" parameters don't get set correctly in
> platform driver version.

> 
> This happens because in old driver version there are three sources of
> parameters: pdata, device tree and autoconfig hardware registers. Some
> parameters were read from pdata and others from autoconfig hardware
> registers. If pdata was absent some pdata structure fields were filled
> with parameters from device tree.


> But 30cb2639 commit disabled overriding pdata with autocfg, so if we
> use platform driver version without pdata some parameters will not be
> set.
> This leads to inoperability of DW DMAC.

My suggestion is still the same, i.e. split platform data to actual
hardware properties and platform quirks. We might be able to use quirks
even in case of auto configuration.

> 
> This patch adds reading missed parameters from device tree.
> 
> Note there's a prerequisite http://www.spinics.net/lists/dmaengine/msg
> 10682.html
> 
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> ---
>  drivers/dma/dw/platform.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
> index 5bda0eb..2712602 100644
> --- a/drivers/dma/dw/platform.c
> +++ b/drivers/dma/dw/platform.c
> @@ -129,6 +129,12 @@ dw_dma_parse_dt(struct platform_device *pdev)
>  	if (of_property_read_bool(np, "is_private"))
>  		pdata->is_private = true;
>  
> +	if (of_property_read_bool(np, "is_memcpy"))
> +		pdata->is_memcpy = true;
> +
> +	if (of_property_read_bool(np, "is_nollp"))
> +		pdata->is_nollp = true;

I'm pretty sure this one (besides that fact that it misses documentation
update and '-' instead of '_' as ordered by DT policy) is unacceptable
in DT since it represents *OS related* quirks. (Btw, is_private is also
should not be there in the first place)

Rob Herring (Cc'ed) might shed a light how to proceed in this case.

> +
>  	if (!of_property_read_u32(np, "chan_allocation_order", &tmp))
>  		pdata->chan_allocation_order = (unsigned char)tmp;
>  

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1468626 — Re: [PATCH] DW: Read "is_memcpy" and "is_nollp" property from device tree.

On Fri, 2016-08-19 at 17:39 +0300, Andy Shevchenko wrote:
> On Tue, 2016-08-16 at 14:31 +0300, Eugeniy Paltsev wrote:
> > 
> > DW DMAC on ARC SDP became broken after df5c7386 ("dmaengine: dw:
> > some
> > Intel
> > devices has no memcpy support") and 30cb2639 ("dmaengine: dw: don't
> > override
> > platform data with autocfg") commits.
> I'm not sure that word 'broken' is a correct one here. Is the
> platform
> code using this driver in the upstream already? If so, where is it
> located?
> 
I'm not sure is it, but, at least, it changed driver behavior for ARC
SDP boards.
> > 
> > 
> > * After df5c7386 commit "DMA_MEMCPY" capability option doesn't get
> > set
> > correctly in platform driver version.
> > * After 30cb2639 commit "nollp" parameters don't get set correctly
> > in
> > platform driver version.
> > 
> > 
> > This happens because in old driver version there are three sources
> > of
> > parameters: pdata, device tree and autoconfig hardware registers.
> > Some
> > parameters were read from pdata and others from autoconfig hardware
> > registers. If pdata was absent some pdata structure fields were
> > filled
> > with parameters from device tree.
> 
> > 
> > But 30cb2639 commit disabled overriding pdata with autocfg, so if
> > we
> > use platform driver version without pdata some parameters will not
> > be
> > set.
> > This leads to inoperability of DW DMAC.
> My suggestion is still the same, i.e. split platform data to actual
> hardware properties and platform quirks. We might be able to use
> quirks
> even in case of auto configuration.
Do you have any idea about better way to do it?
Do you suggest to split pdata structure in two different structures?
> 
> > 
> > 
> > This patch adds reading missed parameters from device tree.
> > 
> > Note there's a prerequisite http://www.spinics.net/lists/dmaengine/
> > msg
> > 10682.html
> > 
> > Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> > ---
> >  drivers/dma/dw/platform.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
> > index 5bda0eb..2712602 100644
> > --- a/drivers/dma/dw/platform.c
> > +++ b/drivers/dma/dw/platform.c
> > @@ -129,6 +129,12 @@ dw_dma_parse_dt(struct platform_device *pdev)
> >  	if (of_property_read_bool(np, "is_private"))
> >  		pdata->is_private = true;
> >  
> > +	if (of_property_read_bool(np, "is_memcpy"))
> > +		pdata->is_memcpy = true;
> > +
> > +	if (of_property_read_bool(np, "is_nollp"))
> > +		pdata->is_nollp = true;
> I'm pretty sure this one (besides that fact that it misses
> documentation
> update and '-' instead of '_' as ordered by DT policy) is
> unacceptable
> in DT since it represents *OS related* quirks. (Btw, is_private is
> also
> should not be there in the first place)
Could you possibly tell me, why you call this quirks *OS related* ?
For example:
If I know, what DMAC in any chip on any board doesn't support memory-
to-memory transfers, I can disable "is_memcpy" in this board .dts file.
Am I wrong? 
> 
> Rob Herring (Cc'ed) might shed a light how to proceed in this case.
> 
> > 
> > +
> >  	if (!of_property_read_u32(np, "chan_allocation_order",
> > &tmp))
> >  		pdata->chan_allocation_order = (unsigned char)tmp;
> >  
-- 
 Paltsev Eugeniy

[toc] | [prev] | [next] | [standalone]

#1468725 — Re: [PATCH] DW: Read "is_memcpy" and "is_nollp" property from device tree.

On Tue, 2016-08-23 at 15:14 +0000, Eugeniy Paltsev wrote:

> DW DMAC on ARC SDP became broken after df5c7386 ("dmaengine: dw:
> > > some Intel devices has no memcpy support") and 30cb2639
> > > ("dmaengine: dw: don't override platform data with autocfg")
> > > commits.
> > I'm not sure that word 'broken' is a correct one here. Is the
> > platform
> > code using this driver in the upstream already? If so, where is it
> > located?
> > 
> I'm not sure is it, but, at least, it changed driver behavior for ARC
> SDP boards.

The rule of common sense here: if it was never upstreamed it has never
been broken.

I hardly remember any user of DW DMAC by ARC architecture in upstream.

> > > But 30cb2639 commit disabled overriding pdata with autocfg, so if
> > > we use platform driver version without pdata some parameters will
> > > not be set. This leads to inoperability of DW DMAC.
> > My suggestion is still the same, i.e. split platform data to actual
> > hardware properties and platform quirks. We might be able to use
> > quirks
> > even in case of auto configuration.
> Do you have any idea about better way to do it?
> Do you suggest to split pdata structure in two different structures

There might be at least couple of ways how to implement this.
1. Convert booleans to bits in one variable (let's say unsigned int
quirks).
2. Split all quirks to separate quirks to something like struct
dw_dma_platform_quirks.

By obvious reasons I think first solution might be better.

> > > +	if (of_property_read_bool(np, "is_memcpy"))
> > > +		pdata->is_memcpy = true;
> > > +
> > > +	if (of_property_read_bool(np, "is_nollp"))
> > > +		pdata->is_nollp = true;
> > I'm pretty sure this one (besides that fact that it misses
> > documentation update and '-' instead of '_' as ordered by DT
> > policy) is unacceptable in DT since it represents *OS related*
> > quirks. (Btw,is_private is also should not be there in the first
> > place)

> Could you possibly tell me, why you call this quirks *OS related* ?
> For example:
> If I know, what DMAC in any chip on any board doesn't support memory-
> to-memory transfers, I can disable "is_memcpy" in this board .dts
> file.
> Am I wrong? 

Some of the properties are set or unset due to support in the driver and
/ or issues of the hardware _related_ to the driver in question.

Though if anyone has different opinion I would appreciate to listen to.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1468728 — Re: [PATCH] DW: Read "is_memcpy" and "is_nollp" property from device tree.

On 08/23/2016 10:02 AM, Andy Shevchenko wrote:
> On Tue, 2016-08-23 at 15:14 +0000, Eugeniy Paltsev wrote:
>
>> DW DMAC on ARC SDP became broken after df5c7386 ("dmaengine: dw:
>>>> some Intel devices has no memcpy support") and 30cb2639
>>>> ("dmaengine: dw: don't override platform data with autocfg")
>>>> commits.
>>> I'm not sure that word 'broken' is a correct one here. Is the
>>> platform
>>> code using this driver in the upstream already? If so, where is it
>>> located?
>>>
>> I'm not sure is it, but, at least, it changed driver behavior for ARC
>> SDP boards.
> The rule of common sense here: if it was never upstreamed it has never
> been broken.

Right !

> I hardly remember any user of DW DMAC by ARC architecture in upstream.

The ARC SDP platform is provided by arch/arc/plat-axs and arch/arc/boot/ax*
The IP Proto-typing kit folks here would just add a DT binding in there and things
would just work out of the box - and that stopped recently - hence the notion of
broken. But I agree one can't fix what can't be seen as broken. I just intervened
to make this comment - I'm sure you and Eugeniy can agree on a workable solution.

Thx,
-Vineet

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web