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

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

[Eugeniy Paltsev]

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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1513 bytes --]

Hi Eugeniy,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.8-rc2 next-20160815]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Eugeniy-Paltsev/DW-Read-is_memcpy-and-is_nollp-property-from-device-tree/20160816-193459
config: sparc64-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc64 

All errors (new ones prefixed by >>):

   drivers/dma/dw/platform.c: In function 'dw_dma_parse_dt':
>> drivers/dma/dw/platform.c:136:8: error: 'struct dw_dma_platform_data' has no member named 'is_nollp'
      pdata->is_nollp = true;
           ^

vim +136 drivers/dma/dw/platform.c

   130			pdata->is_private = true;
   131	
   132		if (of_property_read_bool(np, "is_memcpy"))
   133			pdata->is_memcpy = true;
   134	
   135		if (of_property_read_bool(np, "is_nollp"))
 > 136			pdata->is_nollp = true;
   137	
   138		if (!of_property_read_u32(np, "chan_allocation_order", &tmp))
   139			pdata->chan_allocation_order = (unsigned char)tmp;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 47053 bytes --]

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

[Andy Shevchenko]

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

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

[Eugeniy Paltsev]

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

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

[Andy Shevchenko]

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

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

[Vineet Gupta]

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