Wrong "nollp" DW DMAC parameter value on ARC SDP.

Wrong "nollp" DW DMAC parameter value on ARC SDP.

[Eugeniy Paltsev]

Hi,

"nollp" parameter defines if DW DMAC channel supports multi block
transfer or not.

It is calculated in runtime, but differently depending on on
availability of pdata. If pdata is absent "nollp" is calculated using
autoconfig hardware registers. Otherwise "nollp" is calculated using
the next code construction:
channel_writel(dwc, LLP, DWC_LLP_LOC(0xffffffff));
dwc->nollp = DWC_LLP_LOC(channel_readl(dwc, LLP)) == 0;
channel_writel(dwc, LLP, 0);

I realized that these methods give different results.
For example on ARC AXS101 SDP in case of using autoconfig "nollp" was
calculated as "true" (and DMAC works fine), 
otherwise "nollp" was calculated as "false" (and DMAC doesn't work).

So I'm wondering how the code in question really works?
>From DW AHB DMAC databook I wasn't able to find anything relevant to
this tricky implementation. Could you please clarify a little but what
happens here?
Maybe we should add "nollp" field in pdata structure and receive it
from pdata/device tree (like we use "is_private" or "is_memcpu" fields)

-- 
 Paltsev Eugeniy

Re: Wrong "nollp" DW DMAC parameter value on ARC SDP.

[Andy Shevchenko]

On Fri, 2016-08-12 at 08:03 +0000, Eugeniy Paltsev wrote:
> Hi,
> 
> "nollp" parameter defines if DW DMAC channel supports multi block
> transfer or not.
> 
> It is calculated in runtime, but differently depending on on
> availability of pdata. If pdata is absent "nollp" is calculated using
> autoconfig hardware registers. Otherwise "nollp" is calculated using
> the next code construction:
> channel_writel(dwc, LLP, DWC_LLP_LOC(0xffffffff));
> dwc->nollp = DWC_LLP_LOC(channel_readl(dwc, LLP)) == 0;
> channel_writel(dwc, LLP, 0);
> 
> I realized that these methods give different results.
> For example on ARC AXS101 SDP in case of using autoconfig "nollp" was
> calculated as "true" (and DMAC works fine), 
> otherwise "nollp" was calculated as "false" (and DMAC doesn't work).

Can you show out what the value you read back?

> 
> So I'm wondering how the code in question really works?
> From DW AHB DMAC databook I wasn't able to find anything relevant to
> this tricky implementation. Could you please clarify a little but what
> happens here?

"Table 4-1:
...
Hardcode Channel x LLP register to 0?
...
Description: If set to 1, hardcodes channel x Linked List Pointer
register to 0 (LLPx.LOC == 0), ..."


> Maybe we should add "nollp" field in pdata structure and receive it
> from pdata/device tree (like we use "is_private" or "is_memcpu"
> fields)

Yeah, perhaps we can remove that trick since we need this flag to be set
on Intel Quark which might have the same issue as your case [1].

[1] http://www.spinics.net/lists/linux-serial/msg22948.html

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

Re: Wrong "nollp" DW DMAC parameter value on ARC SDP.

[Eugeniy Paltsev]

On Fri, 2016-08-12 at 13:59 +0300, Andy Shevchenko wrote:
> On Fri, 2016-08-12 at 08:03 +0000, Eugeniy Paltsev wrote:
> > 
> > Hi,
> > 
> > "nollp" parameter defines if DW DMAC channel supports multi block
> > transfer or not.
> > 
> > It is calculated in runtime, but differently depending on on
> > availability of pdata. If pdata is absent "nollp" is calculated
> > using
> > autoconfig hardware registers. Otherwise "nollp" is calculated
> > using
> > the next code construction:
> > channel_writel(dwc, LLP, DWC_LLP_LOC(0xffffffff));
> > dwc->nollp = DWC_LLP_LOC(channel_readl(dwc, LLP)) == 0;
> > channel_writel(dwc, LLP, 0);
> > 
> > I realized that these methods give different results.
> > For example on ARC AXS101 SDP in case of using autoconfig "nollp"
> > was
> > calculated as "true" (and DMAC works fine), 
> > otherwise "nollp" was calculated as "false" (and DMAC doesn't
> > work).
> Can you show out what the value you read back?

channel_readl(dwc, LLP) return 0xfffffffc

> > So I'm wondering how the code in question really works?
> > From DW AHB DMAC databook I wasn't able to find anything relevant
> > to
> > this tricky implementation. Could you please clarify a little but
> > what
> > happens here?
> "Table 4-1:
> ...
> Hardcode Channel x LLP register to 0?
> ...
> Description: If set to 1, hardcodes channel x Linked List Pointer
> register to 0 (LLPx.LOC == 0), ..."
> 
> 
> > 
> > Maybe we should add "nollp" field in pdata structure and receive it
> > from pdata/device tree (like we use "is_private" or "is_memcpu"
> > fields)
> Yeah, perhaps we can remove that trick since we need this flag to be
> set
> on Intel Quark which might have the same issue as your case [1].
> 
> [1] http://www.spinics.net/lists/linux-serial/msg22948.html
> 

In which tree I can find this patch applied, so I may base my work on
it?

-- 
 Paltsev Eugeniy

Re: Wrong "nollp" DW DMAC parameter value on ARC SDP.

[Andy Shevchenko]

On Fri, 2016-08-12 at 13:36 +0000, Eugeniy Paltsev wrote:
> On Fri, 2016-08-12 at 13:59 +0300, Andy Shevchenko wrote:
> > 
> > On Fri, 2016-08-12 at 08:03 +0000, Eugeniy Paltsev wrote:
> > > 
> > > 
> > > Hi,
> > > 
> > > "nollp" parameter defines if DW DMAC channel supports multi block
> > > transfer or not.
> > > 
> > > It is calculated in runtime, but differently depending on on
> > > availability of pdata. If pdata is absent "nollp" is calculated
> > > using
> > > autoconfig hardware registers. Otherwise "nollp" is calculated
> > > using
> > > the next code construction:
> > > channel_writel(dwc, LLP, DWC_LLP_LOC(0xffffffff));
> > > dwc->nollp = DWC_LLP_LOC(channel_readl(dwc, LLP)) == 0;
> > > channel_writel(dwc, LLP, 0);
> > > 
> > > I realized that these methods give different results.
> > > For example on ARC AXS101 SDP in case of using autoconfig "nollp"
> > > was
> > > calculated as "true" (and DMAC works fine), 
> > > otherwise "nollp" was calculated as "false" (and DMAC doesn't
> > > work).
> > Can you show out what the value you read back?
> 
> channel_readl(dwc, LLP) return 0xfffffffc

Nice.

Oh, forgot to ask, what are the DW_PARAMS and DWC_PARAMS[x] are on the
same hardware?

I assume we are talking about that one which has no hardware LLP
support.

> > > So I'm wondering how the code in question really works?
> > > From DW AHB DMAC databook I wasn't able to find anything relevant
> > > to
> > > this tricky implementation. Could you please clarify a little but
> > > what
> > > happens here?
> > "Table 4-1:
> > ...
> > Hardcode Channel x LLP register to 0?
> > ...
> > Description: If set to 1, hardcodes channel x Linked List Pointer
> > register to 0 (LLPx.LOC == 0), ..."

So, any comment on this one? I suppose you may have an access to some
internal Synopsys documentation which might shed a light. Or maybe I
missed something else which should be considered.

> > > Maybe we should add "nollp" field in pdata structure and receive
> > > it
> > > from pdata/device tree (like we use "is_private" or "is_memcpu"
> > > fields)
> > Yeah, perhaps we can remove that trick since we need this flag to be
> > set
> > on Intel Quark which might have the same issue as your case [1].
> > 
> > [1] http://www.spinics.net/lists/linux-serial/msg22948.html
> > 
> 
> In which tree I can find this patch applied, so I may base my work on
> it?

The series is under review. I'm preparing v10, so, I would like to re-
make this patch with regarding to your input.

For now I would prefer just to remove the trick, but I still wonder what
the circumstances are to bring it not working.

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

Re: Wrong "nollp" DW DMAC parameter value on ARC SDP.

[Eugeniy Paltsev]

On Fri, 2016-08-12 at 17:08 +0300, Andy Shevchenko wrote:
> On Fri, 2016-08-12 at 13:36 +0000, Eugeniy Paltsev wrote:
> > 
> > On Fri, 2016-08-12 at 13:59 +0300, Andy Shevchenko wrote:
> > > 
> > > 
> > > On Fri, 2016-08-12 at 08:03 +0000, Eugeniy Paltsev wrote:
> > > > 
> > > > 
> > > > 
> > > > Hi,
> > > > 
> > > > "nollp" parameter defines if DW DMAC channel supports multi
> > > > block
> > > > transfer or not.
> > > > 
> > > > It is calculated in runtime, but differently depending on on
> > > > availability of pdata. If pdata is absent "nollp" is calculated
> > > > using
> > > > autoconfig hardware registers. Otherwise "nollp" is calculated
> > > > using
> > > > the next code construction:
> > > > channel_writel(dwc, LLP, DWC_LLP_LOC(0xffffffff));
> > > > dwc->nollp = DWC_LLP_LOC(channel_readl(dwc, LLP)) == 0;
> > > > channel_writel(dwc, LLP, 0);
> > > > 
> > > > I realized that these methods give different results.
> > > > For example on ARC AXS101 SDP in case of using autoconfig
> > > > "nollp"
> > > > was
> > > > calculated as "true" (and DMAC works fine), 
> > > > otherwise "nollp" was calculated as "false" (and DMAC doesn't
> > > > work).
> > > Can you show out what the value you read back?
> > channel_readl(dwc, LLP) return 0xfffffffc
> Nice.
> 
> Oh, forgot to ask, what are the DW_PARAMS and DWC_PARAMS[x] are on
> the same hardware?
DW_PARAMS: 0x38280b0c
DWC_PARAMS[0]: 0x4926d300
DWC_PARAMS[1]: 0x4926d300
DWC_PARAMS[2]: 0x4926d300
DWC_PARAMS[3]: 0x4926d300
> I assume we are talking about that one which has no hardware LLP
> support.
Yep.
> > 
> > > 
> > > > 
> > > > So I'm wondering how the code in question really works?
> > > > From DW AHB DMAC databook I wasn't able to find anything
> > > > relevant
> > > > to
> > > > this tricky implementation. Could you please clarify a little
> > > > but
> > > > what
> > > > happens here?
> > > "Table 4-1:
> > > ...
> > > Hardcode Channel x LLP register to 0?
> > > ...
> > > Description: If set to 1, hardcodes channel x Linked List Pointer
> > > register to 0 (LLPx.LOC == 0), ..."
> So, any comment on this one? I suppose you may have an access to some
> internal Synopsys documentation which might shed a light. Or maybe I
> missed something else which should be considered.
Looks like this code based on idea, what if DMA ip-core doesn't have 
LLP support it will not have LLP registers. It is not necessarily true.
>
> > 
> > > 
> > > > 
> > > > Maybe we should add "nollp" field in pdata structure and
> > > > receive
> > > > it
> > > > from pdata/device tree (like we use "is_private" or "is_memcpu"
> > > > fields)
> > > Yeah, perhaps we can remove that trick since we need this flag to
> > > be
> > > set
> > > on Intel Quark which might have the same issue as your case [1].
> > > 
> > > [1] http://www.spinics.net/lists/linux-serial/msg22948.html
> > > 
> > In which tree I can find this patch applied, so I may base my work
> > on
> > it?
> The series is under review. I'm preparing v10, so, I would like to
> re-
> make this patch with regarding to your input.
> 
> For now I would prefer just to remove the trick, but I still wonder
> what
> the circumstances are to bring it not working.
> 
Please add this code to read "is_memcpy" and "is_nollp" property 
from device tree.
----------------------------->8------------------------------
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;
 
----------------------------->8------------------------------
-- 
 Paltsev Eugeniy

Re: Wrong "nollp" DW DMAC parameter value on ARC SDP.

[Andy Shevchenko]

On Mon, 2016-08-15 at 11:10 +0000, Eugeniy Paltsev wrote:
> On Fri, 2016-08-12 at 17:08 +0300, Andy Shevchenko wrote:
> >  
> > > > > "nollp" parameter defines if DW DMAC channel supports multi
> > > > > block
> > > > > transfer or not.
> > > > >  
> > > > > It is calculated in runtime, but differently depending on on
> > > > > availability of pdata. If pdata is absent "nollp" is
> > > > > calculated
> > > > > using
> > > > > autoconfig hardware registers. Otherwise "nollp" is calculated
> > > > > using
> > > > > the next code construction:
> > > > > channel_writel(dwc, LLP, DWC_LLP_LOC(0xffffffff));
> > > > > dwc->nollp = DWC_LLP_LOC(channel_readl(dwc, LLP)) == 0;
> > > > > channel_writel(dwc, LLP, 0);
> > > > >  
> > > > > I realized that these methods give different results.
> > > > > For example on ARC AXS101 SDP in case of using autoconfig
> > > > > "nollp"
> > > > > was
> > > > > calculated as "true" (and DMAC works fine), 
> > > > > otherwise "nollp" was calculated as "false" (and DMAC doesn't
> > > > > work).
> > > > Can you show out what the value you read back?
> > > channel_readl(dwc, LLP) return 0xfffffffc
> > Nice.
> >  
> > Oh, forgot to ask, what are the DW_PARAMS and DWC_PARAMS[x] are on
> > the same hardware?
> DW_PARAMS: 0x38280b0c

Wow, you have two masters with number 3 and 4? (Just out of curiosity)

> DWC_PARAMS[0]: 0x4926d300
> DWC_PARAMS[1]: 0x4926d300
> DWC_PARAMS[2]: 0x4926d300
> DWC_PARAMS[3]: 0x4926d300

A-ha, seems you have CHx_HC_LLP == 0. That may explain.
 
> > > > > So I'm wondering how the code in question really works?
> > > > > From DW AHB DMAC databook I wasn't able to find anything
> > > > > relevant
> > > > > to
> > > > > this tricky implementation. Could you please clarify a little
> > > > > but
> > > > > what
> > > > > happens here?
> > > > "Table 4-1:
> > > > ...
> > > > Hardcode Channel x LLP register to 0?
> > > > ...
> > > > Description: If set to 1, hardcodes channel x Linked List
> > > > Pointer
> > > > register to 0 (LLPx.LOC == 0), ..."
> > So, any comment on this one? I suppose you may have an access to
> > some
> > internal Synopsys documentation which might shed a light. Or maybe I
> > missed something else which should be considered.
> Looks like this code based on idea, what if DMA ip-core doesn't have 
> LLP support it will not have LLP registers. It is not necessarily
> true.

Yep.

Consider to review 
http://www.spinics.net/lists/dmaengine/msg10682.html


> >  
> Please add this code to read "is_memcpy" and "is_nollp" property 
> from device tree.
> ----------------------------->8------------------------------
> 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;

Please, make this as a patch and submit either separately, or with your
series mentioning that it has a dependency to my 8250 LPSS series.

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

Re: Wrong "nollp" DW DMAC parameter value on ARC SDP.

[Eugeniy Paltsev]

On Mon, 2016-08-15 at 15:41 +0300, Andy Shevchenko wrote:
> On Mon, 2016-08-15 at 11:10 +0000, Eugeniy Paltsev wrote:
> > 
> > On Fri, 2016-08-12 at 17:08 +0300, Andy Shevchenko wrote:
> > > 
> > >  
> > > > 
> > > > > 
> > > > > > 
> > > > > > "nollp" parameter defines if DW DMAC channel supports multi
> > > > > > block
> > > > > > transfer or not.
> > > > > >  
> > > > > > It is calculated in runtime, but differently depending on
> > > > > > on
> > > > > > availability of pdata. If pdata is absent "nollp" is
> > > > > > calculated
> > > > > > using
> > > > > > autoconfig hardware registers. Otherwise "nollp" is
> > > > > > calculated
> > > > > > using
> > > > > > the next code construction:
> > > > > > channel_writel(dwc, LLP, DWC_LLP_LOC(0xffffffff));
> > > > > > dwc->nollp = DWC_LLP_LOC(channel_readl(dwc, LLP)) == 0;
> > > > > > channel_writel(dwc, LLP, 0);
> > > > > >  
> > > > > > I realized that these methods give different results.
> > > > > > For example on ARC AXS101 SDP in case of using autoconfig
> > > > > > "nollp"
> > > > > > was
> > > > > > calculated as "true" (and DMAC works fine), 
> > > > > > otherwise "nollp" was calculated as "false" (and DMAC
> > > > > > doesn't
> > > > > > work).
> > > > > Can you show out what the value you read back?
> > > > channel_readl(dwc, LLP) return 0xfffffffc
> > > Nice.
> > >  
> > > Oh, forgot to ask, what are the DW_PARAMS and DWC_PARAMS[x] are
> > > on
> > > the same hardware?
> > DW_PARAMS: 0x38280b0c
> Wow, you have two masters with number 3 and 4? (Just out of
> curiosity)
> 
> > 
> > DWC_PARAMS[0]: 0x4926d300
> > DWC_PARAMS[1]: 0x4926d300
> > DWC_PARAMS[2]: 0x4926d300
> > DWC_PARAMS[3]: 0x4926d300
> A-ha, seems you have CHx_HC_LLP == 0. That may explain.
>  
> > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > So I'm wondering how the code in question really works?
> > > > > > From DW AHB DMAC databook I wasn't able to find anything
> > > > > > relevant
> > > > > > to
> > > > > > this tricky implementation. Could you please clarify a
> > > > > > little
> > > > > > but
> > > > > > what
> > > > > > happens here?
> > > > > "Table 4-1:
> > > > > ...
> > > > > Hardcode Channel x LLP register to 0?
> > > > > ...
> > > > > Description: If set to 1, hardcodes channel x Linked List
> > > > > Pointer
> > > > > register to 0 (LLPx.LOC == 0), ..."
> > > So, any comment on this one? I suppose you may have an access to
> > > some
> > > internal Synopsys documentation which might shed a light. Or
> > > maybe I
> > > missed something else which should be considered.
> > Looks like this code based on idea, what if DMA ip-core doesn't
> > have 
> > LLP support it will not have LLP registers. It is not necessarily
> > true.
> Yep.
> 
> Consider to review 
> http://www.spinics.net/lists/dmaengine/msg10682.html
Looks good to me.
>
> > 
> > > 
> > >  
> > Please add this code to read "is_memcpy" and "is_nollp" property 
> > from device tree.
> > ----------------------------->8------------------------------
> > 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;
> Please, make this as a patch and submit either separately, or with
> your
> series mentioning that it has a dependency to my 8250 LPSS series.
> 
Reviewed-by: Eugeniy Platsev <Eugeniy.Paltsev@synopsys.com>

Re: Wrong "nollp" DW DMAC parameter value on ARC SDP.

[Andy Shevchenko]

On Tue, 2016-08-16 at 11:32 +0000, Eugeniy Paltsev wrote:
> On Mon, 2016-08-15 at 15:41 +0300, Andy Shevchenko wrote:
> > On Mon, 2016-08-15 at 11:10 +0000, Eugeniy Paltsev wrote:

> > Consider to review 
> > http://www.spinics.net/lists/dmaengine/msg10682.html
> Looks good to me.

> Reviewed-by: Eugeniy Platsev <Eugeniy.Paltsev@synopsys.com>

I assume that was related to my patch. Please, send this as an answer to
that thread.

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