Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759733AbcJTKs0 (ORCPT ); Thu, 20 Oct 2016 06:48:26 -0400 Received: from mga11.intel.com ([192.55.52.93]:2042 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754752AbcJTKsZ (ORCPT ); Thu, 20 Oct 2016 06:48:25 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,518,1473145200"; d="scan'208";a="774985155" Message-ID: <1476960501.3693.19.camel@linux.intel.com> Subject: Re: [PATCH] dmaengine: DW DMAC: split pdata to hardware properties and platform quirks From: Andy Shevchenko To: Eugeniy Paltsev , dmaengine@vger.kernel.org Cc: linux-kernel@vger.kernel.org, vinod.koul@intel.com, dan.j.williams@intel.com, vireshk@kernel.org, linux-snps-arc@lists.infradead.org Date: Thu, 20 Oct 2016 13:48:21 +0300 In-Reply-To: <1473945253-16649-1-git-send-email-Eugeniy.Paltsev@synopsys.com> References: <1473945253-16649-1-git-send-email-Eugeniy.Paltsev@synopsys.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.1-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8519 Lines: 262 On Thu, 2016-09-15 at 16:14 +0300, Eugeniy Paltsev wrote: > This patch is to address a proposal by Andy in this thread: > http://www.spinics.net/lists/dmaengine/msg10754.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) > My comments below. Sorry for delayed answer. > Signed-off-by: Eugeniy Paltsev > --- >  drivers/dma/dw/core.c                | 31 +++++++++++++++----------- >  drivers/dma/dw/platform.c            | 42 +++++++++++++++++++++---- > ----------- >  include/linux/platform_data/dma-dw.h | 20 +++++++++++------ >  3 files changed, 57 insertions(+), 36 deletions(-) > > diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c > index c2c0a61..9352735 100644 > --- a/drivers/dma/dw/core.c > +++ b/drivers/dma/dw/core.c > @@ -1451,10 +1451,25 @@ int dw_dma_probe(struct dw_dma_chip *chip) >   >   dw->regs = chip->regs; >   chip->dw = dw; + empty line. > + /* Reassign the platform data pointer */ > + pdata = dw->pdata; >   >   pm_runtime_get_sync(chip->dev); >   > - if (!chip->pdata) { > + if ((!chip->pdata) || > +    (chip->pdata && test_bit(QUIRKS_ONLY_USED, &chip->pdata- I don't think you need atomic test / set of those bits. > >quirks))) { > + > + /* > +  * Fill quirks with the default values in case of > pdata absence /* * Multi-line comments should include full sentences * (punctuation matters). */ > +  */ > + if (!chip->pdata) { > + set_bit(QUIRKS_IS_PRIVATE, &pdata->quirks); > + set_bit(QUIRKS_IS_MEMCPY, &pdata->quirks); > + set_bit(QUIRKS_IS_NOLLP, &pdata->quirks); > + } else { > + pdata->quirks = chip->pdata->quirks; > + } > + >   dw_params = dma_readl(dw, DW_PARAMS); >   dev_dbg(chip->dev, "DW_PARAMS: 0x%08x\n", dw_params); >   > @@ -1464,9 +1479,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 +1489,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 +1496,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), > @@ -1569,7 +1576,7 @@ int dw_dma_probe(struct dw_dma_chip *chip) >   (dwc_params >> DWC_PARAMS_MBLK_EN & > 0x1) == 0; >   } else { >   dwc->block_size = pdata->block_size; > - dwc->nollp = pdata->is_nollp; > + dwc->nollp = test_bit(QUIRKS_IS_NOLLP, > &pdata->quirks); Perhaps you need another patch which actually moves nollp to dwc->flags. >   } >   } >   > @@ -1582,9 +1589,9 @@ int dw_dma_probe(struct dw_dma_chip *chip) >   >   /* Set capabilities */ >   dma_cap_set(DMA_SLAVE, dw->dma.cap_mask); > - if (pdata->is_private) > + if (test_bit(QUIRKS_IS_PRIVATE, &pdata->quirks)) >   dma_cap_set(DMA_PRIVATE, dw->dma.cap_mask); > - if (pdata->is_memcpy) > + if (test_bit(QUIRKS_IS_MEMCPY, &pdata->quirks)) >   dma_cap_set(DMA_MEMCPY, dw->dma.cap_mask); >   >   dw->dma.dev = chip->dev; > diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c > index 5bda0eb..308b977 100644 > --- a/drivers/dma/dw/platform.c > +++ b/drivers/dma/dw/platform.c > @@ -12,6 +12,7 @@ >   * published by the Free Software Foundation. >   */ >   > +#include >  #include >  #include >  #include > @@ -111,41 +112,48 @@ 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; >   > + set_bit(QUIRKS_ONLY_USED, &pdata->quirks); > + > + if (of_property_read_bool(np, "is-private")) > + set_bit(QUIRKS_IS_PRIVATE, &pdata->quirks); > + > + if (of_property_read_bool(np, "is-memcpy")) > + set_bit(QUIRKS_IS_MEMCPY, &pdata->quirks); > + > + if (of_property_read_bool(np, "is-nollp")) > + set_bit(QUIRKS_IS_NOLLP, &pdata->quirks); I would suggest to convert to unified device property API first. > + > + 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; > - pdata->nr_channels = nr_channels; >   > - if (of_property_read_bool(np, "is_private")) > - pdata->is_private = true; > + if (of_property_read_u32(np, "dma-channels", &nr_channels)) > + return pdata; >   > - if (!of_property_read_u32(np, "chan_allocation_order", &tmp)) > + pdata->nr_channels = nr_channels; > + > + if (!of_property_read_u32(np, "chan-allocation-order", &tmp)) >   pdata->chan_allocation_order = (unsigned char)tmp; >   > - if (!of_property_read_u32(np, "chan_priority", &tmp)) > + if (!of_property_read_u32(np, "chan-priority", &tmp)) >   pdata->chan_priority = tmp; >   > - if (!of_property_read_u32(np, "block_size", &tmp)) > + if (!of_property_read_u32(np, "block-size", &tmp)) >   pdata->block_size = tmp; >   >   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); >   } >   > + clear_bit(QUIRKS_ONLY_USED, &pdata->quirks); >   return pdata; >  } >  #else > diff --git a/include/linux/platform_data/dma-dw.h > b/include/linux/platform_data/dma-dw.h > index 5f0e11e..9cd8199 100644 > --- a/include/linux/platform_data/dma-dw.h > +++ b/include/linux/platform_data/dma-dw.h > @@ -37,10 +37,7 @@ struct dw_dma_slave { >  /** >   * struct dw_dma_platform_data - Controller configuration parameters >   * @nr_channels: Number of channels supported by hardware (max 8) > - * @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. > - * @is_nollp: The device channels does not support multi block > transfers. > + * @quirks: Bit field with platform quirks >   * @chan_allocation_order: Allocate channels starting from 0 or 7 >   * @chan_priority: Set channel priority increasing from 0 to 7 or 7 > to 0. >   * @block_size: Maximum block size supported by the controller > @@ -50,9 +47,18 @@ struct dw_dma_slave { >   */ >  struct dw_dma_platform_data { >   unsigned int nr_channels; > - bool is_private; > - bool is_memcpy; > - bool is_nollp; > +/* Only use quirks from platform data structure */ > +#define QUIRKS_ONLY_USED 0 > +/* > + * The device channels should be marked as private and not for > + * by the general purpose DMA channel allocator. > + */ > +#define QUIRKS_IS_PRIVATE 1 > +/* The device channels do support memory-to-memory transfers. */ > +#define QUIRKS_IS_MEMCPY 2 > +/* The device channels do not support multi block transfers. */ > +#define QUIRKS_IS_NOLLP 3 You may move descriptions to above struct description field. It will be consolidated and moreover visible in kernel-doc processed documents. > + unsigned long quirks; >  #define CHAN_ALLOCATION_ASCENDING 0 /* zero to seven */ >  #define CHAN_ALLOCATION_DESCENDING 1 /* seven to zero > */ >   unsigned char chan_allocation_order; -- Andy Shevchenko Intel Finland Oy