[PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties

[PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties

[Eugeniy Paltsev]

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

[PATCH v3 1/3] dmaengine: DW DMAC: split pdata to hardware properties and platform quirks

[Eugeniy Paltsev]

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)

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 drivers/dma/dw/core.c                | 23 ++++++++++++++---------
 drivers/dma/dw/platform.c            | 34 +++++++++++++++++++++-------------
 include/linux/platform_data/dma-dw.h |  6 ++++++
 3 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index c2c0a61..6281009 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -1452,9 +1452,22 @@ 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->only_quirks_used) {
+		/* Fill quirks with the default values */
+		pdata->is_private = true;
+		pdata->is_memcpy = true;
+
+		/* Apply platform defined quirks */
+		if (chip->pdata && chip->pdata->only_quirks_used) {
+			pdata->is_private = chip->pdata->is_private;
+			pdata->is_memcpy = chip->pdata->is_memcpy;
+		}
+
 		dw_params = dma_readl(dw, DW_PARAMS);
 		dev_dbg(chip->dev, "DW_PARAMS: 0x%08x\n", dw_params);
 
@@ -1464,9 +1477,6 @@ int dw_dma_probe(struct dw_dma_chip *chip)
 			goto err_pdata;
 		}
 
-		/* Reassign the platform data pointer */
-		pdata = dw->pdata;
-
 		/* Get hardware configuration parameters */
 		pdata->nr_channels = (dw_params >> DW_PARAMS_NR_CHAN & 7) + 1;
 		pdata->nr_masters = (dw_params >> DW_PARAMS_NR_MASTER & 3) + 1;
@@ -1477,8 +1487,6 @@ int dw_dma_probe(struct dw_dma_chip *chip)
 		pdata->block_size = dma_readl(dw, MAX_BLK_SIZE);
 
 		/* Fill platform data with the default values */
-		pdata->is_private = true;
-		pdata->is_memcpy = true;
 		pdata->chan_allocation_order = CHAN_ALLOCATION_ASCENDING;
 		pdata->chan_priority = CHAN_PRIORITY_ASCENDING;
 	} else if (chip->pdata->nr_channels > DW_DMA_MAX_NR_CHANNELS) {
@@ -1486,9 +1494,6 @@ int dw_dma_probe(struct dw_dma_chip *chip)
 		goto err_pdata;
 	} else {
 		memcpy(dw->pdata, chip->pdata, sizeof(*dw->pdata));
-
-		/* Reassign the platform data pointer */
-		pdata = dw->pdata;
 	}
 
 	dw->chan = devm_kcalloc(chip->dev, pdata->nr_channels, sizeof(*dw->chan),
diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
index 5bda0eb..77cde375 100644
--- a/drivers/dma/dw/platform.c
+++ b/drivers/dma/dw/platform.c
@@ -111,24 +111,33 @@ dw_dma_parse_dt(struct platform_device *pdev)
 		return NULL;
 	}
 
-	if (of_property_read_u32(np, "dma-masters", &nr_masters))
-		return NULL;
-	if (nr_masters < 1 || nr_masters > DW_DMA_MAX_NR_MASTERS)
-		return NULL;
-
-	if (of_property_read_u32(np, "dma-channels", &nr_channels))
-		return NULL;
-
 	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
 		return NULL;
 
-	pdata->nr_masters = nr_masters;
-	pdata->nr_channels = nr_channels;
+	pdata->only_quirks_used = true;
 
 	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_u32(np, "dma-masters", &nr_masters))
+		return pdata;
+	if (nr_masters < 1 || nr_masters > DW_DMA_MAX_NR_MASTERS)
+		return pdata;
+
+	pdata->nr_masters = nr_masters;
+
+	if (of_property_read_u32(np, "dma-channels", &nr_channels))
+		return pdata;
+
+	pdata->nr_channels = nr_channels;
+
+	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;
 
@@ -141,11 +150,10 @@ dw_dma_parse_dt(struct platform_device *pdev)
 	if (!of_property_read_u32_array(np, "data-width", arr, nr_masters)) {
 		for (tmp = 0; tmp < nr_masters; tmp++)
 			pdata->data_width[tmp] = arr[tmp];
-	} else if (!of_property_read_u32_array(np, "data_width", arr, nr_masters)) {
-		for (tmp = 0; tmp < nr_masters; tmp++)
-			pdata->data_width[tmp] = BIT(arr[tmp] & 0x07);
 	}
 
+	pdata->only_quirks_used = false;
+
 	return pdata;
 }
 #else
diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h
index 5f0e11e..6cc1d7c 100644
--- a/include/linux/platform_data/dma-dw.h
+++ b/include/linux/platform_data/dma-dw.h
@@ -40,6 +40,11 @@ struct dw_dma_slave {
  * @is_private: The device channels should be marked as private and not for
  *	by the general purpose DMA channel allocator.
  * @is_memcpy: The device channels do support memory-to-memory transfers.
+ * @only_quirks_used: Only read quirks from platform data structure.
+ *	Read other parameters from device tree node (if exists) or from
+ *	hardware autoconfig registers. Only properties "is_private" and
+ *	"is_memcpy" are quirks for now. Note that all listed quirks will
+ *	be copied from platform data.
  * @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 +57,7 @@ struct dw_dma_platform_data {
 	unsigned int	nr_channels;
 	bool		is_private;
 	bool		is_memcpy;
+	bool		only_quirks_used;
 	bool		is_nollp;
 #define CHAN_ALLOCATION_ASCENDING	0	/* zero to seven */
 #define CHAN_ALLOCATION_DESCENDING	1	/* seven to zero */
-- 
2.5.5

[PATCH v3 2/3] dmaengine: DW DMAC: convert to unified device property API

[Eugeniy Paltsev]

Convert device tree properties reading to unified device
property API, update properties names as ordered by DT policy.

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

diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
index 77cde375..85368bc 100644
--- a/drivers/dma/dw/platform.c
+++ b/drivers/dma/dw/platform.c
@@ -98,56 +98,51 @@ static inline void dw_dma_acpi_controller_register(struct dw_dma *dw) {}
 
 #ifdef CONFIG_OF
 static struct dw_dma_platform_data *
-dw_dma_parse_dt(struct platform_device *pdev)
+dw_dma_parse_properties(struct device *dev)
 {
-	struct device_node *np = pdev->dev.of_node;
 	struct dw_dma_platform_data *pdata;
 	u32 tmp, arr[DW_DMA_MAX_NR_MASTERS];
 	u32 nr_masters;
 	u32 nr_channels;
 
-	if (!np) {
-		dev_err(&pdev->dev, "Missing DT data\n");
-		return NULL;
-	}
-
-	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
 		return NULL;
 
 	pdata->only_quirks_used = true;
 
-	if (of_property_read_bool(np, "is_private"))
+	if (device_property_read_bool(dev, "is-private"))
 		pdata->is_private = true;
 
-	if (of_property_read_bool(np, "is-memcpy"))
+	if (device_property_read_bool(dev, "is-memcpy"))
 		pdata->is_memcpy = true;
 
-	if (of_property_read_u32(np, "dma-masters", &nr_masters))
+	if (device_property_read_u32(dev, "dma-masters", &nr_masters))
 		return pdata;
 	if (nr_masters < 1 || nr_masters > DW_DMA_MAX_NR_MASTERS)
 		return pdata;
 
 	pdata->nr_masters = nr_masters;
 
-	if (of_property_read_u32(np, "dma-channels", &nr_channels))
+	if (device_property_read_u32(dev, "dma-channels", &nr_channels))
 		return pdata;
 
 	pdata->nr_channels = nr_channels;
 
-	if (of_property_read_bool(np, "is-nollp"))
+	if (device_property_read_bool(dev, "is-nollp"))
 		pdata->is_nollp = true;
 
-	if (!of_property_read_u32(np, "chan_allocation_order", &tmp))
+	if (!device_property_read_u32(dev, "chan-allocation-order", &tmp))
 		pdata->chan_allocation_order = (unsigned char)tmp;
 
-	if (!of_property_read_u32(np, "chan_priority", &tmp))
+	if (!device_property_read_u32(dev, "chan-priority", &tmp))
 		pdata->chan_priority = tmp;
 
-	if (!of_property_read_u32(np, "block_size", &tmp))
+	if (!device_property_read_u32(dev, "block-size", &tmp))
 		pdata->block_size = tmp;
 
-	if (!of_property_read_u32_array(np, "data-width", arr, nr_masters)) {
+	if (!device_property_read_u32_array(dev, "data-width", arr,
+					    nr_masters)) {
 		for (tmp = 0; tmp < nr_masters; tmp++)
 			pdata->data_width[tmp] = arr[tmp];
 	}
@@ -158,7 +153,7 @@ dw_dma_parse_dt(struct platform_device *pdev)
 }
 #else
 static inline struct dw_dma_platform_data *
-dw_dma_parse_dt(struct platform_device *pdev)
+dw_dma_parse_properties(struct device *dev)
 {
 	return NULL;
 }
@@ -191,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_properties(dev);
 
 	chip->dev = dev;
 	chip->pdata = pdata;
-- 
2.5.5

[PATCH v3 3/3] dmaengine: DW DMAC: move "nollp" to "dwc->flags"

[Eugeniy Paltsev]

Add "DW_DMA_IS_LLP_SUPPORTED" flag. Use this flag to get rid
of "dwc->nollp" as separate variable.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 drivers/dma/dw/core.c | 11 ++++++-----
 drivers/dma/dw/regs.h |  2 +-
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 6281009..98beb16 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -227,7 +227,7 @@ static void dwc_dostart(struct dw_dma_chan *dwc, struct dw_desc *first)
 		return;
 	}
 
-	if (dwc->nollp) {
+	if (!test_bit(DW_DMA_IS_LLP_SUPPORTED, &dwc->flags)) {
 		was_soft_llp = test_and_set_bit(DW_DMA_IS_SOFT_LLP,
 						&dwc->flags);
 		if (was_soft_llp) {
@@ -1265,7 +1265,7 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
 	unsigned long			flags;
 
 	spin_lock_irqsave(&dwc->lock, flags);
-	if (dwc->nollp) {
+	if (!test_bit(DW_DMA_IS_LLP_SUPPORTED, &dwc->flags)) {
 		spin_unlock_irqrestore(&dwc->lock, flags);
 		dev_dbg(chan2dev(&dwc->chan),
 				"channel doesn't support LLP transfers\n");
@@ -1570,11 +1570,12 @@ int dw_dma_probe(struct dw_dma_chip *chip)
 			 */
 			dwc->block_size =
 				(4 << ((pdata->block_size >> 4 * i) & 0xf)) - 1;
-			dwc->nollp =
-				(dwc_params >> DWC_PARAMS_MBLK_EN & 0x1) == 0;
+			if ((dwc_params >> DWC_PARAMS_MBLK_EN & 0x1) != 0)
+				set_bit(DW_DMA_IS_LLP_SUPPORTED, &dwc->flags);
 		} else {
 			dwc->block_size = pdata->block_size;
-			dwc->nollp = pdata->is_nollp;
+			if (!pdata->is_nollp)
+				set_bit(DW_DMA_IS_LLP_SUPPORTED, &dwc->flags);
 		}
 	}
 
diff --git a/drivers/dma/dw/regs.h b/drivers/dma/dw/regs.h
index f65dd10..f50c0fc 100644
--- a/drivers/dma/dw/regs.h
+++ b/drivers/dma/dw/regs.h
@@ -218,6 +218,7 @@ enum dw_dmac_flags {
 	DW_DMA_IS_SOFT_LLP = 1,
 	DW_DMA_IS_PAUSED = 2,
 	DW_DMA_IS_INITIALIZED = 3,
+	DW_DMA_IS_LLP_SUPPORTED = 4,
 };
 
 struct dw_dma_chan {
@@ -242,7 +243,6 @@ struct dw_dma_chan {
 
 	/* hardware configuration */
 	unsigned int		block_size;
-	bool			nollp;
 
 	/* custom slave configuration */
 	struct dw_dma_slave	dws;
-- 
2.5.5

Re: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties

[Eugeniy Paltsev]

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

Re: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties

[Andy Shevchenko]

On Wed, 2016-11-02 at 11:55 +0000, 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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties

[Eugeniy Paltsev]

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 int	nr_channels;
> >  	bool		is_private;
> >  	bool		is_memcpy;
> > 
> > +	bool		only_quirks_used;
> Perhaps add if at the end of quirk list and name just 
> 
> > 
> >  	bool		is_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

Re: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties

[Andy Shevchenko]

On Tue, 2016-11-08 at 12:22 +0000, 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 int	nr_channels;
> > >   	bool		is_private;
> > >   	bool		is_memcpy;
> > >  
> > > +	bool		only_quirks_used;
> > 
> > Perhaps add if at the end of quirk list and name just 
> >  
> > >  
> > >   	bool		is_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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties

[Eugeniy Paltsev]

On Tue, 2016-11-08 at 15:36 +0200, Andy Shevchenko wrote:
> On Tue, 2016-11-08 at 12:22 +0000, 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 int	nr_channels;
> > > >   	bool		is_private;
> > > >   	bool		is_memcpy;
> > > >  
> > > > +	bool		only_quirks_used;
> > > Perhaps add if at the end of quirk list and name just 
> > >  
> > > > 
> > > >  
> > > >   	bool		is_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(0xffffffff));
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

Re: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties

[Andy Shevchenko]

On Thu, Nov 10, 2016 at 6:28 PM, Eugeniy Paltsev
<Eugeniy.Paltsev@synopsys.com> wrote:
> On Tue, 2016-11-08 at 15:36 +0200, Andy Shevchenko wrote:
>> On Tue, 2016-11-08 at 12:22 +0000, 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 int    nr_channels;
>> > > >         bool            is_private;
>> > > >         bool            is_memcpy;
>> > > >
>> > > > +       bool            only_quirks_used;

>> > > Perhaps add if at the end of quirk list and name just

>> > > >         bool            is_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(0xffffffff));
> 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