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
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 <[email protected]>
---
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
Convert device tree properties reading to unified device
property API, update properties names as ordered by DT policy.
Signed-off-by: Eugeniy Paltsev <[email protected]>
---
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
Add "DW_DMA_IS_LLP_SUPPORTED" flag. Use this flag to get rid
of "dwc->nollp" as separate variable.
Signed-off-by: Eugeniy Paltsev <[email protected]>
---
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
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 <[email protected]>
Intel Finland Oy
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
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 <[email protected]>
Intel Finland Oy
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
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 <[email protected]>
Intel Finland Oy
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
On Thu, Nov 10, 2016 at 6:28 PM, Eugeniy Paltsev
<[email protected]> 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