Re: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties
On Thu, Nov 10, 2016 at 6:28 PM, Eugeniy Paltsev
wrote:
> On Tue, 2016-11-08 at 15:36 +0200, Andy Shevchenko wrote:
>> On Tue, 2016-11-08 at 12:22 +, Eugeniy Paltsev wrote:
>> > On Mon, 2016-11-07 at 15:55 +0200, Andy Shevchenko wrote:
>> > > > @@ -52,6 +55,7 @@ struct dw_dma_platform_data {
>> > > > unsigned intnr_channels;
>> > > > boolis_private;
>> > > > boolis_memcpy;
>> > > >
>> > > > + boolonly_quirks_used;
>> > > Perhaps add if at the end of quirk list and name just
>> > > > boolis_nollp;
>> > > ...here
>> > >
>> > > bool use_quirks;
>> What do think about shorten name?
>>
> I don't know better short and understandable name for "use_quirks"
> variable. You can suggest your ideas if you want.
This is my suggestion. In your patch I saw longer one.
>
>> >
>> > I don't treat "is_nollp" as quirks like "is_private" or
>> > "is_memcpy".
>> > It is like general pdata field: we can easily read it from
>> > autoconfig
>> > registers (and we don't have any problem with that) in case of
>> > pdata/device-tree absence (as opposed to quirks like "is_private"
>> > or
>> > "is_memcpy")
>> >
>> > So, in PATCH v3 series "is_nollp" used as regular pdata field.
>> I still would consider is_nollp as a quirk since nothing prevents to
>> override the hardware value (see Intel Quark case).
>>
> Do you mean this issue:
> http://www.spinics.net/lists/linux-serial/msg22948.html
> ?
>
> As I remember, we had problems with next code:
> >8--
> channel_writel(dwc, LLP, DWC_LLP_LOC(0x));
> dwc->nollp = DWC_LLP_LOC(channel_readl(dwc, LLP)) == 0;
> channel_writel(dwc, LLP, 0);
> >8--
> which was executed if we didn't use autoconfig registers.
> This code doesn't used anymore.
>
> And we don't have any problems with autoconfig registers!
Yeah, but we have a quirk.
> So in case of Intel Quark we will read "nollp" parameter from pdata or
> from autoconfig registers (in case of pdata absence). It should work
> fine in both cases.
> Please correct me if I'm wrong.
>
> So, in my opinion, "is_nollp" should be used as regular pdata field.
Let's discuss it a bit later.
--
With Best Regards,
Andy Shevchenko
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties
On Tue, 2016-11-08 at 15:36 +0200, Andy Shevchenko wrote:
> On Tue, 2016-11-08 at 12:22 +, Eugeniy Paltsev wrote:
> >
> > On Mon, 2016-11-07 at 15:55 +0200, Andy Shevchenko wrote:
>
> >
> > >
> > > >
> > > > + * @only_quirks_used: Only read quirks (like "is_private" or
> > > > "is_memcpy") from
> > > > + * platform data structure. Read other parameters from
> > > > device
> > > > tree
> > > > + * node (if exists) or from hardware autoconfig
> > > > registers.
> > > Can you somehow be more clear that all listed quirks will be
> > > copied
> > > from
> > > platform data.
> > See comment below.
> >
> > >
> > >
> > > >
> > > >
> > > > * @is_nollp: The device channels does not support multi
> > > > block
> > > > transfers.
> > > > * @chan_allocation_order: Allocate channels starting from 0
> > > > or
> > > > 7
> > > > * @chan_priority: Set channel priority increasing from 0 to
> > > > 7
> > > > or
> > > > 7
> > > > to 0.
> > > > @@ -52,6 +55,7 @@ struct dw_dma_platform_data {
> > > > unsigned intnr_channels;
> > > > boolis_private;
> > > > boolis_memcpy;
> > > >
> > > > + boolonly_quirks_used;
> > > Perhaps add if at the end of quirk list and name just
> > >
> > > >
> > > >
> > > > boolis_nollp;
> > > ...here
> > >
> > > bool use_quirks;
> What do think about shorten name?
>
I don't know better short and understandable name for "use_quirks"
variable. You can suggest your ideas if you want.
> >
> > I don't treat "is_nollp" as quirks like "is_private" or
> > "is_memcpy".
> > It is like general pdata field: we can easily read it from
> > autoconfig
> > registers (and we don't have any problem with that) in case of
> > pdata/device-tree absence (as opposed to quirks like "is_private"
> > or
> > "is_memcpy")
> >
> > So, in PATCH v3 series "is_nollp" used as regular pdata field.
> I still would consider is_nollp as a quirk since nothing prevents to
> override the hardware value (see Intel Quark case).
>
Do you mean this issue:
http://www.spinics.net/lists/linux-serial/msg22948.html
?
As I remember, we had problems with next code:
>8--
channel_writel(dwc, LLP, DWC_LLP_LOC(0x));
dwc->nollp = DWC_LLP_LOC(channel_readl(dwc, LLP)) == 0;
channel_writel(dwc, LLP, 0);
>8--
which was executed if we didn't use autoconfig registers.
This code doesn't used anymore.
And we don't have any problems with autoconfig registers!
So in case of Intel Quark we will read "nollp" parameter from pdata or
from autoconfig registers (in case of pdata absence). It should work
fine in both cases.
Please correct me if I'm wrong.
So, in my opinion, "is_nollp" should be used as regular pdata field.
--
Paltsev Eugeniy
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties
On Tue, 2016-11-08 at 12:22 +, Eugeniy Paltsev wrote:
> On Mon, 2016-11-07 at 15:55 +0200, Andy Shevchenko wrote:
> > > + * @only_quirks_used: Only read quirks (like "is_private" or
> > > "is_memcpy") from
> > > + * platform data structure. Read other parameters from
> > > device
> > > tree
> > > + * node (if exists) or from hardware autoconfig registers.
> >
> > Can you somehow be more clear that all listed quirks will be copied
> > from
> > platform data.
>
> See comment below.
>
> >
> > >
> > > * @is_nollp: The device channels does not support multi block
> > > transfers.
> > > * @chan_allocation_order: Allocate channels starting from 0 or
> > > 7
> > > * @chan_priority: Set channel priority increasing from 0 to 7
> > > or
> > > 7
> > > to 0.
> > > @@ -52,6 +55,7 @@ struct dw_dma_platform_data {
> > > unsigned intnr_channels;
> > > boolis_private;
> > > boolis_memcpy;
> > >
> > > + boolonly_quirks_used;
> >
> > Perhaps add if at the end of quirk list and name just
> >
> > >
> > > boolis_nollp;
> >
> > ...here
> >
> > bool use_quirks;
What do think about shorten name?
>
> I don't treat "is_nollp" as quirks like "is_private" or "is_memcpy".
> It is like general pdata field: we can easily read it from autoconfig
> registers (and we don't have any problem with that) in case of
> pdata/device-tree absence (as opposed to quirks like "is_private" or
> "is_memcpy")
>
> So, in PATCH v3 series "is_nollp" used as regular pdata field.
I still would consider is_nollp as a quirk since nothing prevents to
override the hardware value (see Intel Quark case).
--
Andy Shevchenko
Intel Finland Oy
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties
On Mon, 2016-11-07 at 15:55 +0200, Andy Shevchenko wrote:
>
> Thanks for an update, but, please, answer to all my comments to your
> patch v2. Either you are okay with them, then you didn't address few,
> or
> you are not okay, I didn't get why. Deffer newer version until we get
> an
> agreement on the implementation.
>
Thanks for response.
My comments are given inline below.
---
> > Changes for v2:
> >- use separate bool values for quirks in "dw_dma_platform_data"
> > instead of
> > common bit field.
> >
> >- convert device tree properties reading to unified device
> > property
> > API.
> This should be a separate patch.
>
Agree. Implemented as separate patch in PATCH v3 series.
> >
> >
> > I was wrong about DW_DMA_IS_SOFT_LLP flag - it is used to check
> > about
> > ongoing soft llp transfer. So DW_DMA_IS_SOFT_LLP flag and "dwc-
> > >
> > > nollp"
> > variable have different functions and I couldn't just get rid of
> > "dwc-
> > >
> > > nollp"
> > and use DW_DMA_IS_SOFT_LLP flag instead. So I left "dwc->nollp"
> > untouched.
> So, then perhaps we may convert it to another flag let's say
> DW_DMA_IS_LLP_SUPPORTED.
>
> But this is other story independent of the subject.
Implemented in PATCH v3 series.
"dwc->nollp" was converted to "DW_DMA_IS_LLP_SUPPORTED" flag.
> >
> > --- a/drivers/dma/dw/core.c
> > +++ b/drivers/dma/dw/core.c
> > @@ -1452,9 +1452,24 @@ int dw_dma_probe(struct dw_dma_chip *chip)
> > dw->regs = chip->regs;
> > chip->dw = dw;
> >
> > + /* Reassign the platform data pointer */
> > + pdata = dw->pdata;
> > +
> > pm_runtime_get_sync(chip->dev);
> >
> > - if (!chip->pdata) {
> > + if ((!chip->pdata) || (chip->pdata && chip->pdata-
> > >
> > > only_quirks_used)) {
> It's simple as
> if (!chip->pdata || chip->pdata->only_quirks_used)
>
> > [--sources--]
> >
> Would we leave the first part in the place it is now and add new
> piece
> after?
>
> > [--sources--]
> >
> ...like
>
> /* Apply platform defined quirks */
> if (chip->data && chip->data->only_quirks_used) {
> ...
> }
Agree. That looks better.
>
> >
> > - if (of_property_read_u32(np, "dma-channels",
> > &nr_channels))
> > - return NULL;
> > + if (device_property_read_bool(dev, "is-private"))
> As I mentioned above, please do a separate patch for this.
Implemented as separate patch in PATCH v3 series.
>
> >
> > @@ -183,7 +186,7 @@ static int dw_probe(struct platform_device
> > *pdev)
> >
> > pdata = dev_get_platdata(dev);
> > if (!pdata)
> > - pdata = dw_dma_parse_dt(pdev);
> > + pdata = dw_dma_parse_dt(dev);
> Perhaps you might rename the function to something like
>
> dw_dma_parse_properties(dev);
Implemented in PATCH v3 series.
>
> >
> > + * @only_quirks_used: Only read quirks (like "is_private" or
> > "is_memcpy") from
> > + * platform data structure. Read other parameters from
> > device
> > tree
> > + * node (if exists) or from hardware autoconfig registers.
> Can you somehow be more clear that all listed quirks will be copied
> from
> platform data.
See comment below.
>
> >
> > * @is_nollp: The device channels does not support multi block
> > transfers.
> > * @chan_allocation_order: Allocate channels starting from 0 or 7
> > * @chan_priority: Set channel priority increasing from 0 to 7 or
> > 7
> > to 0.
> > @@ -52,6 +55,7 @@ struct dw_dma_platform_data {
> > unsigned intnr_channels;
> > boolis_private;
> > boolis_memcpy;
> >
> > + boolonly_quirks_used;
> Perhaps add if at the end of quirk list and name just
>
> >
> > boolis_nollp;
> ...here
>
> bool use_quirks;
>
I don't treat "is_nollp" as quirks like "is_private" or "is_memcpy".
It is like general pdata field: we can easily read it from autoconfig
registers (and we don't have any problem with that) in case of
pdata/device-tree absence (as opposed to quirks like "is_private" or
"is_memcpy")
So, in PATCH v3 series "is_nollp" used as regular pdata field.
--
Paltsev Eugeniy
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties
On Wed, 2016-11-02 at 11:55 +, Eugeniy Paltsev wrote: > Hi Andy, > Could you possibly tell me your ideas about these changes? > Thanks. Thanks for an update, but, please, answer to all my comments to your patch v2. Either you are okay with them, then you didn't address few, or you are not okay, I didn't get why. Deffer newer version until we get an agreement on the implementation. > > On Fri, 2016-10-28 at 18:59 +0300, Eugeniy Paltsev wrote: > > This series is to address a proposal by Andy in these threads: > > http://www.spinics.net/lists/dmaengine/msg11506.html > > http://www.spinics.net/lists/dmaengine/msg11541.html > > Split platform data to actual hardware properties, and platform > > quirks. > > Now we able to use quirks and hardware properties separately from > > different sources (pdata, device tree or autoconfig registers) > > > > Changes for v3: > > - Split changes to separate patches. > > - Add "DW_DMA_IS_LLP_SUPPORTED" flag and get rid of "dwc->nollp" > > as separate variable. > > - Make "dw_dma_slave" documentation comments more clear about > > quirks. > > "is_memcpy" and "is_private" are quirks, "is_nollp" is regular > > pdata property. > > > > Eugeniy Paltsev (3): > > dmaengine: DW DMAC: split pdata to hardware properties and > > platform > > quirks > > dmaengine: DW DMAC: convert to unified device property API > > dmaengine: DW DMAC: move "nollp" to "dwc->flags" > > > > drivers/dma/dw/core.c| 34 +-- > > drivers/dma/dw/platform.c| 53 +++ > > --- > > -- > > drivers/dma/dw/regs.h| 2 +- > > include/linux/platform_data/dma-dw.h | 5 > > 4 files changed, 54 insertions(+), 40 deletions(-) > > > > -- > Paltsev Eugeniy -- Andy Shevchenko Intel Finland Oy ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties
Hi Andy, Could you possibly tell me your ideas about these changes? Thanks. On Fri, 2016-10-28 at 18:59 +0300, Eugeniy Paltsev wrote: > This series is to address a proposal by Andy in these threads: > http://www.spinics.net/lists/dmaengine/msg11506.html > http://www.spinics.net/lists/dmaengine/msg11541.html > Split platform data to actual hardware properties, and platform > quirks. > Now we able to use quirks and hardware properties separately from > different sources (pdata, device tree or autoconfig registers) > > Changes for v3: > - Split changes to separate patches. > - Add "DW_DMA_IS_LLP_SUPPORTED" flag and get rid of "dwc->nollp" > as separate variable. > - Make "dw_dma_slave" documentation comments more clear about > quirks. > "is_memcpy" and "is_private" are quirks, "is_nollp" is regular > pdata property. > > Eugeniy Paltsev (3): > dmaengine: DW DMAC: split pdata to hardware properties and platform > quirks > dmaengine: DW DMAC: convert to unified device property API > dmaengine: DW DMAC: move "nollp" to "dwc->flags" > > drivers/dma/dw/core.c| 34 +-- > drivers/dma/dw/platform.c| 53 +++--- > -- > drivers/dma/dw/regs.h| 2 +- > include/linux/platform_data/dma-dw.h | 5 > 4 files changed, 54 insertions(+), 40 deletions(-) > -- Paltsev Eugeniy ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties
On Fri, 2016-10-28 at 18:59 +0300, Eugeniy Paltsev wrote: > This series is to address a proposal by Andy in these threads: > http://www.spinics.net/lists/dmaengine/msg11506.html > http://www.spinics.net/lists/dmaengine/msg11541.html > Split platform data to actual hardware properties, and platform > quirks. > Now we able to use quirks and hardware properties separately from > different sources (pdata, device tree or autoconfig registers) Thanks for an update I will comment them later. > > Changes for v3: > - Split changes to separate patches. > - Add "DW_DMA_IS_LLP_SUPPORTED" flag and get rid of "dwc->nollp" > as separate variable. > - Make "dw_dma_slave" documentation comments more clear about > quirks. > "is_memcpy" and "is_private" are quirks, "is_nollp" is regular > pdata property. > > Eugeniy Paltsev (3): > dmaengine: DW DMAC: split pdata to hardware properties and platform > quirks > dmaengine: DW DMAC: convert to unified device property API > dmaengine: DW DMAC: move "nollp" to "dwc->flags" > > drivers/dma/dw/core.c| 34 +-- > drivers/dma/dw/platform.c| 53 +++-- > --- > drivers/dma/dw/regs.h| 2 +- > include/linux/platform_data/dma-dw.h | 5 > 4 files changed, 54 insertions(+), 40 deletions(-) > -- Andy Shevchenko Intel Finland Oy ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties
This series is to address a proposal by Andy in these threads: http://www.spinics.net/lists/dmaengine/msg11506.html http://www.spinics.net/lists/dmaengine/msg11541.html Split platform data to actual hardware properties, and platform quirks. Now we able to use quirks and hardware properties separately from different sources (pdata, device tree or autoconfig registers) Changes for v3: - Split changes to separate patches. - Add "DW_DMA_IS_LLP_SUPPORTED" flag and get rid of "dwc->nollp" as separate variable. - Make "dw_dma_slave" documentation comments more clear about quirks. "is_memcpy" and "is_private" are quirks, "is_nollp" is regular pdata property. Eugeniy Paltsev (3): dmaengine: DW DMAC: split pdata to hardware properties and platform quirks dmaengine: DW DMAC: convert to unified device property API dmaengine: DW DMAC: move "nollp" to "dwc->flags" drivers/dma/dw/core.c| 34 +-- drivers/dma/dw/platform.c| 53 +++- drivers/dma/dw/regs.h| 2 +- include/linux/platform_data/dma-dw.h | 5 4 files changed, 54 insertions(+), 40 deletions(-) -- 2.5.5 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
