From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dong Aisheng Subject: Re: [PATCH 2/5] dma: mxs-dma: make platform_device_id more generic Date: Mon, 23 Apr 2012 11:58:48 +0800 Message-ID: <20120423035846.GB24843@shlinux2.ap.freescale.net> References: <1334753197-12032-1-git-send-email-b29396@freescale.com> <1334753197-12032-3-git-send-email-b29396@freescale.com> <20120423030114.GI26306@S2101-09.ap.freescale.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20120423030114.GI26306-rvtDTF3kK1ictlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org Sender: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org To: Shawn Guo Cc: "marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org" , "vinod.koul-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org" , "devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org" , "rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org" , Huang Shijie-B32955 , "kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org" , "dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org" , "s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org" , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" List-Id: devicetree@vger.kernel.org On Mon, Apr 23, 2012 at 11:01:18AM +0800, Shawn Guo wrote: > On Wed, Apr 18, 2012 at 08:46:34PM +0800, Dong Aisheng wrote: > > From: Dong Aisheng > > > > Rewrite mxs_dma_is_apbh and mxs_dma_is_apbx in order to support > > other SoCs like imx6q and reform the platform_device_id for the > > better further dt support. > > > Nice cleanup. But I would like to ask more. > > > Cc: Vinod Koul > > Cc: Dan Williams > > Cc: Shawn Guo > > Cc: Sascha Hauer > > Cc: Marek Vasut > > Cc: Huang Shijie > > Signed-off-by: Dong Aisheng > > --- > > arch/arm/mach-mxs/clock-mx23.c | 4 +- > > arch/arm/mach-mxs/clock-mx28.c | 4 +- > > arch/arm/mach-mxs/devices/platform-dma.c | 14 ++-- > > drivers/dma/mxs-dma.c | 111 +++++++++++++++++++++++------- > > include/linux/fsl/mxs-dma.h | 12 +--- > > 5 files changed, 101 insertions(+), 44 deletions(-) > > > > diff --git a/arch/arm/mach-mxs/clock-mx23.c b/arch/arm/mach-mxs/clock-mx23.c > > index e3ac52c..1e67b85 100644 > > --- a/arch/arm/mach-mxs/clock-mx23.c > > +++ b/arch/arm/mach-mxs/clock-mx23.c > > @@ -427,8 +427,8 @@ static struct clk_lookup lookups[] = { > > _REGISTER_CLOCK("duart", NULL, uart_clk) > > _REGISTER_CLOCK("mxs-auart.0", NULL, uart_clk) > > _REGISTER_CLOCK("rtc", NULL, rtc_clk) > > - _REGISTER_CLOCK("mxs-dma-apbh", NULL, hbus_clk) > > - _REGISTER_CLOCK("mxs-dma-apbx", NULL, xbus_clk) > > + _REGISTER_CLOCK("imx23-dma-apbh", NULL, hbus_clk) > > + _REGISTER_CLOCK("imx23-dma-apbx", NULL, xbus_clk) > > _REGISTER_CLOCK("mxs-mmc.0", NULL, ssp_clk) > > _REGISTER_CLOCK("mxs-mmc.1", NULL, ssp_clk) > > _REGISTER_CLOCK(NULL, "usb", usb_clk) > > diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c > > index 1867a17..cca2cf7 100644 > > --- a/arch/arm/mach-mxs/clock-mx28.c > > +++ b/arch/arm/mach-mxs/clock-mx28.c > > @@ -625,8 +625,8 @@ static struct clk_lookup lookups[] = { > > _REGISTER_CLOCK("mxs-auart.4", NULL, uart_clk) > > _REGISTER_CLOCK("rtc", NULL, rtc_clk) > > _REGISTER_CLOCK("pll2", NULL, pll2_clk) > > - _REGISTER_CLOCK("mxs-dma-apbh", NULL, hbus_clk) > > - _REGISTER_CLOCK("mxs-dma-apbx", NULL, xbus_clk) > > + _REGISTER_CLOCK("imx28-dma-apbh", NULL, hbus_clk) > > + _REGISTER_CLOCK("imx28-dma-apbx", NULL, xbus_clk) > > _REGISTER_CLOCK("mxs-mmc.0", NULL, ssp0_clk) > > _REGISTER_CLOCK("mxs-mmc.1", NULL, ssp1_clk) > > _REGISTER_CLOCK("mxs-mmc.2", NULL, ssp2_clk) > > diff --git a/arch/arm/mach-mxs/devices/platform-dma.c b/arch/arm/mach-mxs/devices/platform-dma.c > > index 6a0202b..aff4813 100644 > > --- a/arch/arm/mach-mxs/devices/platform-dma.c > > +++ b/arch/arm/mach-mxs/devices/platform-dma.c > > @@ -32,17 +32,19 @@ static struct platform_device *__init mxs_add_dma(const char *devid, > > > > static int __init mxs_add_mxs_dma(void) > > { > > - char *apbh = "mxs-dma-apbh"; > > - char *apbx = "mxs-dma-apbx"; > > + char *mx23_apbh = "imx23-dma-apbh"; > > + char *mx23_apbx = "imx23-dma-apbx"; > > + char *mx28_apbh = "imx28-dma-apbh"; > > + char *mx28_apbx = "imx28-dma-apbx"; > > > > if (cpu_is_mx23()) { > > - mxs_add_dma(apbh, MX23_APBH_DMA_BASE_ADDR); > > - mxs_add_dma(apbx, MX23_APBX_DMA_BASE_ADDR); > > + mxs_add_dma(mx23_apbh, MX23_APBH_DMA_BASE_ADDR); > > + mxs_add_dma(mx23_apbx, MX23_APBX_DMA_BASE_ADDR); > > } > > > > if (cpu_is_mx28()) { > > - mxs_add_dma(apbh, MX28_APBH_DMA_BASE_ADDR); > > - mxs_add_dma(apbx, MX28_APBX_DMA_BASE_ADDR); > > + mxs_add_dma(mx28_apbh, MX28_APBH_DMA_BASE_ADDR); > > + mxs_add_dma(mx28_apbx, MX28_APBX_DMA_BASE_ADDR); > > } > > > > return 0; > > With soc specific hook introduced, we can remove this mxs_add_mxs_dma > call completely. > Yes, it's true. A mx23/mx28_soc_init can do it well. > > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c > > index b0051a8..51a29f9 100644 > > --- a/drivers/dma/mxs-dma.c > > +++ b/drivers/dma/mxs-dma.c > > @@ -50,7 +50,8 @@ > > #define HW_APBHX_CTRL2 0x020 > > #define HW_APBHX_CHANNEL_CTRL 0x030 > > #define BP_APBHX_CHANNEL_CTRL_RESET_CHANNEL 16 > > -#define HW_APBH_VERSION (cpu_is_mx23() ? 0x3f0 : 0x800) > > +#define HW_APBH_VERSION_IMX23 0x3f0 > > +#define HW_APBH_VERSION_IMX28 0x800 > > Since the VERSION is broken for identifying the dma type and we are > introduce dma type field, I would suggest to remove the use of VERSION > completely and use mxs_dma_type pbh_is_old() (mxs_dma->everywhere. > Remove HW_APBH_VERSION_*? But i see the some other places in original code still need to use it. Like: #define apbh_is_old() (mxs_dma->version < APBH_VERSION_LATEST) .. #define HW_APBHX_CHn_NXTCMDAR(n) \ (((dma_is_apbh() && apbh_is_old()) ? 0x050 : 0x110) + (n) * 0x70) #define HW_APBHX_CHn_SEMA(n) \ (((dma_is_apbh() && apbh_is_old()) ? 0x080 : 0x140) + (n) * 0x70) Do you mean apbh_is_old represents the same as SoC type and can be replaced by mxs_dma_type? > #define HW_APBX_VERSION 0x800 > > #define BP_APBHX_VERSION_MAJOR 24 > > #define HW_APBHX_CHn_NXTCMDAR(n) \ > > @@ -121,7 +122,8 @@ struct mxs_dma_chan { > > #define MXS_DMA_CHANNELS_MASK 0xffff > > > > struct mxs_dma_engine { > > - int dev_id; > > + unsigned int type; > > + unsigned int dev_id; > > unsigned int version; > > void __iomem *base; > > struct clk *clk; > > @@ -130,6 +132,74 @@ struct mxs_dma_engine { > > struct mxs_dma_chan mxs_chans[MXS_DMA_CHANNELS]; > > }; > > > > +enum mxs_dma_devtype { > > + IMX23_DMA, > > + IMX28_DMA, > > +}; > > + > > +struct mxs_dma_type { > > + unsigned int type; > > + unsigned int id; > > +}; > > + > > +static struct mxs_dma_type mxs_dma_types[] = { > > + { > > + .type = IMX23_DMA, > > + .id = MXS_DMA_APBH, > > To me, IMX23_DMA is more like a id, while MXS_DMA_APBH is more like > a type. > Well, i'm more like IMX23_DMA since they're different devices. Id to me is more like a same device but different id. And you see i already have enum mxs_dma_devtype. > > + }, { > > + .type = IMX23_DMA, > > + .id = MXS_DMA_APBX, > > + }, { > > + .type = IMX28_DMA, > > + .id = MXS_DMA_APBH, > > + }, { > > + .type = IMX28_DMA, > > + .id = MXS_DMA_APBX, > > + }, { > > + /* end of list */ > > + } > > This end sign is not needed, I think. > Ok to me. btw i always had this question before, Can you tell what's purpose of adding a '/* end of list */'? > > +}; > > + > > +static struct platform_device_id mxs_dma_idt[] = { > > s/mxs_dma_idt/mxs_dma_ids? > Ok to me. > > + { > > + .name = "imx23-dma-apbh", > > + .driver_data = (kernel_ulong_t) &mxs_dma_types[0], > > + }, { > > + .name = "imx23-dma-apbx", > > + .driver_data = (kernel_ulong_t) &mxs_dma_types[1], > > + }, { > > + .name = "imx28-dma-apbh", > > + .driver_data = (kernel_ulong_t) &mxs_dma_types[2], > > + }, { > > + .name = "imx28-dma-apbx", > > + .driver_data = (kernel_ulong_t) &mxs_dma_types[3], > > + }, { > > + /* end of list */ > > + } > > +}; > > + > > +static struct mxs_dma_chan *to_mxs_dma_chan(struct dma_chan *chan) > > +{ > > + return container_of(chan, struct mxs_dma_chan, chan); > > +} > > + > > +int mxs_dma_is_apbh(struct dma_chan *chan) > > +{ > > + struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan); > > + struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma; > > + > > + return dma_is_apbh(); > > I would expect it simply be > > return (mxs_dma->dev_id == MXS_DMA_APBH); > > And dma_is_apbh() can be removed completely now. > Yes, it's ture. Basically what i did in this patch is only convert the driver to a more generic platform_device_id, seems i need to do more... > > +} > > + > > +int mxs_dma_is_apbx(struct dma_chan *chan) > > +{ > > + struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan); > > + struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma; > > + > > + return !dma_is_apbh(); > > return (mxs_dma->dev_id == MXS_DMA_APBX); > > > +} > > + > > + > > static void mxs_dma_reset_chan(struct mxs_dma_chan *mxs_chan) > > { > > struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma; > > @@ -193,11 +263,6 @@ static void mxs_dma_resume_chan(struct mxs_dma_chan *mxs_chan) > > mxs_chan->status = DMA_IN_PROGRESS; > > } > > > > -static struct mxs_dma_chan *to_mxs_dma_chan(struct dma_chan *chan) > > -{ > > - return container_of(chan, struct mxs_dma_chan, chan); > > -} > > - > > static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx) > > { > > struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan); > > @@ -574,11 +639,18 @@ static int __init mxs_dma_init(struct mxs_dma_engine *mxs_dma) > > if (ret) > > goto err_out; > > > > - /* only major version matters */ > > - mxs_dma->version = readl(mxs_dma->base + > > + if (mxs_dma->type == IMX23_DMA) { > > + /* only major version matters */ > > + mxs_dma->version = readl(mxs_dma->base + > > ((mxs_dma->dev_id == MXS_DMA_APBX) ? > > - HW_APBX_VERSION : HW_APBH_VERSION)) >> > > + HW_APBX_VERSION : HW_APBH_VERSION_IMX23)) >> > > BP_APBHX_VERSION_MAJOR; > > + } else if (mxs_dma->type == IMX28_DMA) { > > + mxs_dma->version = readl(mxs_dma->base + > > + ((mxs_dma->dev_id == MXS_DMA_APBX) ? > > + HW_APBX_VERSION : HW_APBH_VERSION_IMX28)) >> > > + BP_APBHX_VERSION_MAJOR; > > + } > > Again, let's just get rid of the use of VERSION completely. > > > > > /* enable apbh burst */ > > if (dma_is_apbh()) { > > @@ -601,6 +673,8 @@ static int __init mxs_dma_probe(struct platform_device *pdev) > > { > > const struct platform_device_id *id_entry = > > platform_get_device_id(pdev); > > + const struct mxs_dma_type *mtype = > > + (struct mxs_dma_type *)id_entry->driver_data; > > struct mxs_dma_engine *mxs_dma; > > struct resource *iores; > > int ret, i; > > @@ -609,7 +683,8 @@ static int __init mxs_dma_probe(struct platform_device *pdev) > > if (!mxs_dma) > > return -ENOMEM; > > > > - mxs_dma->dev_id = id_entry->driver_data; > > + mxs_dma->type = mtype->type; > > + mxs_dma->dev_id = mtype->id; > > > > iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > @@ -692,23 +767,11 @@ err_request_region: > > return ret; > > } > > > > -static struct platform_device_id mxs_dma_type[] = { > > - { > > - .name = "mxs-dma-apbh", > > - .driver_data = MXS_DMA_APBH, > > - }, { > > - .name = "mxs-dma-apbx", > > - .driver_data = MXS_DMA_APBX, > > - }, { > > - /* end of list */ > > - } > > -}; > > - > > static struct platform_driver mxs_dma_driver = { > > .driver = { > > .name = "mxs-dma", > > }, > > - .id_table = mxs_dma_type, > > + .id_table = mxs_dma_idt, > > }; > > > > static int __init mxs_dma_module_init(void) > > diff --git a/include/linux/fsl/mxs-dma.h b/include/linux/fsl/mxs-dma.h > > index 203d7c4..55d8702 100644 > > --- a/include/linux/fsl/mxs-dma.h > > +++ b/include/linux/fsl/mxs-dma.h > > @@ -15,14 +15,6 @@ struct mxs_dma_data { > > int chan_irq; > > }; > > > > -static inline int mxs_dma_is_apbh(struct dma_chan *chan) > > -{ > > - return !strcmp(dev_name(chan->device->dev), "mxs-dma-apbh"); > > -} > > - > > -static inline int mxs_dma_is_apbx(struct dma_chan *chan) > > -{ > > - return !strcmp(dev_name(chan->device->dev), "mxs-dma-apbx"); > > -} > > - > > +extern int mxs_dma_is_apbh(struct dma_chan *chan); > > +extern int mxs_dma_is_apbx(struct dma_chan *chan); > > I would expect these two calls still be inline here. > Ok to me. Regards Dong Aisheng