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 <[email protected]>
> >
> > 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 <[email protected]>
> > Cc: Dan Williams <[email protected]>
> > Cc: Shawn Guo <[email protected]>
> > Cc: Sascha Hauer <[email protected]>
> > Cc: Marek Vasut <[email protected]>
> > Cc: Huang Shijie <[email protected]>
> > Signed-off-by: Dong Aisheng <[email protected]>
> > ---
> > 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
_______________________________________________
devicetree-discuss mailing list
[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss
