| Message ID | 1306315367-15602-1-git-send-email-zhangfei.gao@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 25 May 2011, Zhangfei Gao wrote: > Instead of sharing sdhci-pxa.c, use sdhci-mmp2.c specificlly for mmp2. > sdhci-pxa.c is used to share among pxa serious, like pxa910, mmp2, etc. > The platfrom difference are put under arch/arm, while is not easy to track. > > Signed-off-by: Zhangfei Gao <zhangfei.gao@marvell.com> > --- > arch/arm/plat-pxa/include/plat/sdhci.h | 43 ++++- > drivers/mmc/host/Kconfig | 11 +- > drivers/mmc/host/Makefile | 2 +- > drivers/mmc/host/sdhci-mmp2.c | 303 ++++++++++++++++++++++++++++++++ > drivers/mmc/host/sdhci-pxa.c | 303 -------------------------------- > 5 files changed, 349 insertions(+), 313 deletions(-) > create mode 100644 drivers/mmc/host/sdhci-mmp2.c > delete mode 100644 drivers/mmc/host/sdhci-pxa.c Yes, looks good for the most part. I was rather confused by the fact that the old and new file are both 303 lines, so I assumed they would be identical, when they are really completely different. There is a little room for simplification, I think: > +static unsigned int mmp2_get_ro(struct sdhci_host *host) > +{ > + /* Micro SD does not support write-protect feature */ > + return 0; > +} You shouldn't need to provide an empty get_ro function, the default is that there is no write-protect. > +static void mmp2_set_clock(struct sdhci_host *host, unsigned int clock) > +{ > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_pxa *pxa = pltfm_host->priv; > + > + if (clock) > + mmp2_soc_set_timing(host, pxa->pdata); > +} The mmp2_soc_set_timing() function is only called here, so you can easily merge the two into one, starting with if (!clock) return; > +static int __devinit sdhci_mmp2_probe(struct platform_device *pdev) > +{ > + struct sdhci_pltfm_host *pltfm_host; > + struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data; > + struct device *dev = &pdev->dev; > + struct sdhci_host *host = NULL; > + struct sdhci_pxa *pxa = NULL; > + int ret; > + struct clk *clk; The probe and release functions for mmp2 and pxa910 are almost identical. I'd suggest you leave them in sdhci-pxa.c as a library, and export them using EXPORT_SYMBOL_GPL. Then you can call them from the respective probe functions, e.g. static struct sdhci_mmp2_ops = { .set_clock = mmp2_set_clock, .set_uhs_signaling = mmp2_set_uhs_signaling, .get_ro = mmp2_get_ro, }; static int __devinit sdhci_mmp2_probe(struct platform_device *pdev) { unsigned int quirks = SDHCI_QUIRK_BROKEN_ADMA | SDHCI_QUIRK_BROKEN_TIMEOUT_VAL | SDHCI_QUIRK_32BIT_DMA_ADDR | SDHCI_QUIRK_32BIT_DMA_SIZE | SDHCI_QUIRK_32BIT_ADMA_SIZE | SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC; return sdhci_pxa_probe(pdev, quirks, sdhci_mmp2_ops); } > + pxa = kzalloc(sizeof(struct sdhci_pxa), GFP_KERNEL); > + if (!pxa) { > + ret = -ENOMEM; > + goto out; > + } > + pxa->ops = kzalloc(sizeof(struct sdhci_ops), GFP_KERNEL); > + if (!pxa->ops) { > + ret = -ENOMEM; > + goto out; > + } I think you really shouldn't allocate the sdhci_ops dynamically. In fact, it would be much better if we were able to mark them as const in all drivers. > + > +#ifdef CONFIG_PM > +static int sdhci_mmp2_suspend(struct platform_device *dev, pm_message_t state) > +{ > + struct sdhci_host *host = platform_get_drvdata(dev); > + > + return sdhci_suspend_host(host, state); > +} > + > +static int sdhci_mmp2_resume(struct platform_device *dev) > +{ > + struct sdhci_host *host = platform_get_drvdata(dev); > + > + return sdhci_resume_host(host); > +} > +#else > +#define sdhci_mmp2_suspend NULL > +#define sdhci_mmp2_resume NULL > +#endif Similarly, I think it would be good if sdhci-pltfm.c would simply export these functions so you could refer to them in your driver. There is no need to have identical copies in each variant. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 27, 2011 at 11:46 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 25 May 2011, Zhangfei Gao wrote: >> Instead of sharing sdhci-pxa.c, use sdhci-mmp2.c specificlly for mmp2. >> sdhci-pxa.c is used to share among pxa serious, like pxa910, mmp2, etc. >> The platfrom difference are put under arch/arm, while is not easy to track. >> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@marvell.com> >> --- >> arch/arm/plat-pxa/include/plat/sdhci.h | 43 ++++- >> drivers/mmc/host/Kconfig | 11 +- >> drivers/mmc/host/Makefile | 2 +- >> drivers/mmc/host/sdhci-mmp2.c | 303 ++++++++++++++++++++++++++++++++ >> drivers/mmc/host/sdhci-pxa.c | 303 -------------------------------- >> 5 files changed, 349 insertions(+), 313 deletions(-) >> create mode 100644 drivers/mmc/host/sdhci-mmp2.c >> delete mode 100644 drivers/mmc/host/sdhci-pxa.c > > Yes, looks good for the most part. I was rather confused by the fact that the old > and new file are both 303 lines, so I assumed they would be identical, when they > are really completely different. > > There is a little room for simplification, I think: > >> +static unsigned int mmp2_get_ro(struct sdhci_host *host) >> +{ >> + /* Micro SD does not support write-protect feature */ >> + return 0; >> +} > > You shouldn't need to provide an empty get_ro function, the > default is that there is no write-protect. Thanks Arnd for review. The reason to put get_ro here is some board use micro sd, while some board design is general sd card. The micro sd do not use write-protect, while SDHCI_WRITE_PROTECT will return 1 in our controller, so it shows read only. So add one call back for the board with micro sd card via flag. > >> +static void mmp2_set_clock(struct sdhci_host *host, unsigned int clock) >> +{ >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> + struct sdhci_pxa *pxa = pltfm_host->priv; >> + >> + if (clock) >> + mmp2_soc_set_timing(host, pxa->pdata); >> +} > > The mmp2_soc_set_timing() function is only called here, so you can easily > merge the two into one, starting with > > if (!clock) > return; OK, thanks, > >> +static int __devinit sdhci_mmp2_probe(struct platform_device *pdev) >> +{ >> + struct sdhci_pltfm_host *pltfm_host; >> + struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data; >> + struct device *dev = &pdev->dev; >> + struct sdhci_host *host = NULL; >> + struct sdhci_pxa *pxa = NULL; >> + int ret; >> + struct clk *clk; > > The probe and release functions for mmp2 and pxa910 are almost identical. > I'd suggest you leave them in sdhci-pxa.c as a library, and export them > using EXPORT_SYMBOL_GPL. Then you can call them from the respective > probe functions, e.g. > > static struct sdhci_mmp2_ops = { > .set_clock = mmp2_set_clock, > .set_uhs_signaling = mmp2_set_uhs_signaling, > .get_ro = mmp2_get_ro, > }; > static int __devinit sdhci_mmp2_probe(struct platform_device *pdev) > { > unsigned int quirks = SDHCI_QUIRK_BROKEN_ADMA > | SDHCI_QUIRK_BROKEN_TIMEOUT_VAL > | SDHCI_QUIRK_32BIT_DMA_ADDR > | SDHCI_QUIRK_32BIT_DMA_SIZE > | SDHCI_QUIRK_32BIT_ADMA_SIZE > | SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC; > > return sdhci_pxa_probe(pdev, quirks, sdhci_mmp2_ops); > } > >> + pxa = kzalloc(sizeof(struct sdhci_pxa), GFP_KERNEL); >> + if (!pxa) { >> + ret = -ENOMEM; >> + goto out; >> + } >> + pxa->ops = kzalloc(sizeof(struct sdhci_ops), GFP_KERNEL); >> + if (!pxa->ops) { >> + ret = -ENOMEM; >> + goto out; >> + } > > I think you really shouldn't allocate the sdhci_ops dynamically. > In fact, it would be much better if we were able to mark them > as const in all drivers. We found some issues when supporting multi-slot, each slot may have different ops. So use the method of allocating the sdhci_ops dynamically instead of using static ops. For example, emmc has 74_clocks call back, while mmc and sdio slot does not have such ops. If not dynamically allocate sdhci_ops, all slot ops may become same, and 74_clocks may be called for every slot. Also, some board may have get_ro, while other board may not, so transfer the ops via flags. Not sure whether it is worthy to add additional common files to share probe and remove function. Also the init the ops part are different. > >> + >> +#ifdef CONFIG_PM >> +static int sdhci_mmp2_suspend(struct platform_device *dev, pm_message_t state) >> +{ >> + struct sdhci_host *host = platform_get_drvdata(dev); >> + >> + return sdhci_suspend_host(host, state); >> +} >> + >> +static int sdhci_mmp2_resume(struct platform_device *dev) >> +{ >> + struct sdhci_host *host = platform_get_drvdata(dev); >> + >> + return sdhci_resume_host(host); >> +} >> +#else >> +#define sdhci_mmp2_suspend NULL >> +#define sdhci_mmp2_resume NULL >> +#endif > > Similarly, I think it would be good if sdhci-pltfm.c would simply > export these functions so you could refer to them in your driver. > There is no need to have identical copies in each variant. > There are some additional code for suspend and resume, so sdhci_pltfm_suspend may not enough. For example, when enable wifi host sleep feature, additional register have to be configured. > Arnd > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On May 29, 2011, at 10:42 PM, zhangfei gao wrote: > On Fri, May 27, 2011 at 11:46 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Wednesday 25 May 2011, Zhangfei Gao wrote: >>> Instead of sharing sdhci-pxa.c, use sdhci-mmp2.c specificlly for mmp2. >>> sdhci-pxa.c is used to share among pxa serious, like pxa910, mmp2, etc. >>> The platfrom difference are put under arch/arm, while is not easy to track. >>> >>> Signed-off-by: Zhangfei Gao <zhangfei.gao@marvell.com> >>> --- >>> arch/arm/plat-pxa/include/plat/sdhci.h | 43 ++++- >>> drivers/mmc/host/Kconfig | 11 +- >>> drivers/mmc/host/Makefile | 2 +- >>> drivers/mmc/host/sdhci-mmp2.c | 303 ++++++++++++++++++++++++++++++++ >>> drivers/mmc/host/sdhci-pxa.c | 303 -------------------------------- >>> 5 files changed, 349 insertions(+), 313 deletions(-) >>> create mode 100644 drivers/mmc/host/sdhci-mmp2.c >>> delete mode 100644 drivers/mmc/host/sdhci-pxa.c >> >> Yes, looks good for the most part. I was rather confused by the fact that the old >> and new file are both 303 lines, so I assumed they would be identical, when they >> are really completely different. >> >> There is a little room for simplification, I think: >> >>> +static unsigned int mmp2_get_ro(struct sdhci_host *host) >>> +{ >>> + /* Micro SD does not support write-protect feature */ >>> + return 0; >>> +} >> >> You shouldn't need to provide an empty get_ro function, the >> default is that there is no write-protect. > > Thanks Arnd for review. > The reason to put get_ro here is some board use micro sd, while some > board design is general sd card. > The micro sd do not use write-protect, while SDHCI_WRITE_PROTECT will > return 1 in our controller, so it shows read only. > So add one call back for the board with micro sd card via flag. > >> >>> +static void mmp2_set_clock(struct sdhci_host *host, unsigned int clock) >>> +{ >>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>> + struct sdhci_pxa *pxa = pltfm_host->priv; >>> + >>> + if (clock) >>> + mmp2_soc_set_timing(host, pxa->pdata); >>> +} >> >> The mmp2_soc_set_timing() function is only called here, so you can easily >> merge the two into one, starting with >> >> if (!clock) >> return; > OK, thanks, >> >>> +static int __devinit sdhci_mmp2_probe(struct platform_device *pdev) >>> +{ >>> + struct sdhci_pltfm_host *pltfm_host; >>> + struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data; >>> + struct device *dev = &pdev->dev; >>> + struct sdhci_host *host = NULL; >>> + struct sdhci_pxa *pxa = NULL; >>> + int ret; >>> + struct clk *clk; >> >> The probe and release functions for mmp2 and pxa910 are almost identical. >> I'd suggest you leave them in sdhci-pxa.c as a library, and export them >> using EXPORT_SYMBOL_GPL. Then you can call them from the respective >> probe functions, e.g. >> >> static struct sdhci_mmp2_ops = { >> .set_clock = mmp2_set_clock, >> .set_uhs_signaling = mmp2_set_uhs_signaling, >> .get_ro = mmp2_get_ro, >> }; >> static int __devinit sdhci_mmp2_probe(struct platform_device *pdev) >> { >> unsigned int quirks = SDHCI_QUIRK_BROKEN_ADMA >> | SDHCI_QUIRK_BROKEN_TIMEOUT_VAL >> | SDHCI_QUIRK_32BIT_DMA_ADDR >> | SDHCI_QUIRK_32BIT_DMA_SIZE >> | SDHCI_QUIRK_32BIT_ADMA_SIZE >> | SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC; >> >> return sdhci_pxa_probe(pdev, quirks, sdhci_mmp2_ops); >> } >> >>> + pxa = kzalloc(sizeof(struct sdhci_pxa), GFP_KERNEL); >>> + if (!pxa) { >>> + ret = -ENOMEM; >>> + goto out; >>> + } >>> + pxa->ops = kzalloc(sizeof(struct sdhci_ops), GFP_KERNEL); >>> + if (!pxa->ops) { >>> + ret = -ENOMEM; >>> + goto out; >>> + } >> >> I think you really shouldn't allocate the sdhci_ops dynamically. >> In fact, it would be much better if we were able to mark them >> as const in all drivers. > > We found some issues when supporting multi-slot, each slot may have > different ops. > So use the method of allocating the sdhci_ops dynamically instead of > using static ops. > For example, emmc has 74_clocks call back, while mmc and sdio slot > does not have such ops. > If not dynamically allocate sdhci_ops, all slot ops may become same, > and 74_clocks may be called for every slot. > Also, some board may have get_ro, while other board may not, so > transfer the ops via flags. 74 clock code causes no harm for SD/eMMC/SDIO. You can save 74 clocks if that really is important for SDIO. I do not know what a SD slot is. You cannot make any assumptions about what card the user will put into a slot. > > Not sure whether it is worthy to add additional common files to share > probe and remove function. > Also the init the ops part are different. > >> >>> + >>> +#ifdef CONFIG_PM >>> +static int sdhci_mmp2_suspend(struct platform_device *dev, pm_message_t state) >>> +{ >>> + struct sdhci_host *host = platform_get_drvdata(dev); >>> + >>> + return sdhci_suspend_host(host, state); >>> +} >>> + >>> +static int sdhci_mmp2_resume(struct platform_device *dev) >>> +{ >>> + struct sdhci_host *host = platform_get_drvdata(dev); >>> + >>> + return sdhci_resume_host(host); >>> +} >>> +#else >>> +#define sdhci_mmp2_suspend NULL >>> +#define sdhci_mmp2_resume NULL >>> +#endif >> >> Similarly, I think it would be good if sdhci-pltfm.c would simply >> export these functions so you could refer to them in your driver. >> There is no need to have identical copies in each variant. >> > > There are some additional code for suspend and resume, so > sdhci_pltfm_suspend may not enough. > For example, when enable wifi host sleep feature, additional register > have to be configured. > >> Arnd >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 30 May 2011 07:42:04 zhangfei gao wrote: > On Fri, May 27, 2011 at 11:46 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Wednesday 25 May 2011, Zhangfei Gao wrote: > >> +static unsigned int mmp2_get_ro(struct sdhci_host *host) > >> +{ > >> + /* Micro SD does not support write-protect feature */ > >> + return 0; > >> +} > > > > You shouldn't need to provide an empty get_ro function, the > > default is that there is no write-protect. > > Thanks Arnd for review. > The reason to put get_ro here is some board use micro sd, while some > board design is general sd card. > The micro sd do not use write-protect, while SDHCI_WRITE_PROTECT will > return 1 in our controller, so it shows read only. > So add one call back for the board with micro sd card via flag. Ok, I see. > >> + pxa = kzalloc(sizeof(struct sdhci_pxa), GFP_KERNEL); > >> + if (!pxa) { > >> + ret = -ENOMEM; > >> + goto out; > >> + } > >> + pxa->ops = kzalloc(sizeof(struct sdhci_ops), GFP_KERNEL); > >> + if (!pxa->ops) { > >> + ret = -ENOMEM; > >> + goto out; > >> + } > > > > I think you really shouldn't allocate the sdhci_ops dynamically. > > In fact, it would be much better if we were able to mark them > > as const in all drivers. > > We found some issues when supporting multi-slot, each slot may have > different ops. > So use the method of allocating the sdhci_ops dynamically instead of > using static ops. > For example, emmc has 74_clocks call back, while mmc and sdio slot > does not have such ops. > If not dynamically allocate sdhci_ops, all slot ops may become same, > and 74_clocks may be called for every slot. > Also, some board may have get_ro, while other board may not, so > transfer the ops via flags. > > Not sure whether it is worthy to add additional common files to share > probe and remove function. > Also the init the ops part are different. IMHO, we should try much harder to keep the ops constant. For the two examples you gave, I would probably put a flag into private data for the get_ro operation to signify that it's never read-only, and provide a second set of operations for emmc, so that in the probe function you check the type of device and then set the appropriate one. You could also do the same for all three cases: if (emmc) host->ops = &sdhci_mmp2_emmc_ops; else if (!has_ro_switch) host->ops = &sdhci_mmp2_rw_ops; else host->ops = &sdhci_mmp2_default_ops; > >> + > >> +#ifdef CONFIG_PM > >> +static int sdhci_mmp2_suspend(struct platform_device *dev, pm_message_t state) > >> +{ > >> + struct sdhci_host *host = platform_get_drvdata(dev); > >> + > >> + return sdhci_suspend_host(host, state); > >> +} > >> + > >> +static int sdhci_mmp2_resume(struct platform_device *dev) > >> +{ > >> + struct sdhci_host *host = platform_get_drvdata(dev); > >> + > >> + return sdhci_resume_host(host); > >> +} > >> +#else > >> +#define sdhci_mmp2_suspend NULL > >> +#define sdhci_mmp2_resume NULL > >> +#endif > > > > Similarly, I think it would be good if sdhci-pltfm.c would simply > > export these functions so you could refer to them in your driver. > > There is no need to have identical copies in each variant. > > > > There are some additional code for suspend and resume, so > sdhci_pltfm_suspend may not enough. > For example, when enable wifi host sleep feature, additional register > have to be configured. Well, not all drivers would have to use the common functions, but right now there are a number of identical copies, so it's certainly worth sharing. Note that this comment wasn't directed at your patch as much as at the overall design of the sdhci sub-drivers. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 30, 2011 at 3:11 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Monday 30 May 2011 07:42:04 zhangfei gao wrote: >> On Fri, May 27, 2011 at 11:46 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Wednesday 25 May 2011, Zhangfei Gao wrote: >> >> +static unsigned int mmp2_get_ro(struct sdhci_host *host) >> >> +{ >> >> + /* Micro SD does not support write-protect feature */ >> >> + return 0; >> >> +} >> > >> > You shouldn't need to provide an empty get_ro function, the >> > default is that there is no write-protect. >> >> Thanks Arnd for review. >> The reason to put get_ro here is some board use micro sd, while some >> board design is general sd card. >> The micro sd do not use write-protect, while SDHCI_WRITE_PROTECT will >> return 1 in our controller, so it shows read only. >> So add one call back for the board with micro sd card via flag. > > Ok, I see. > >> >> + pxa = kzalloc(sizeof(struct sdhci_pxa), GFP_KERNEL); >> >> + if (!pxa) { >> >> + ret = -ENOMEM; >> >> + goto out; >> >> + } >> >> + pxa->ops = kzalloc(sizeof(struct sdhci_ops), GFP_KERNEL); >> >> + if (!pxa->ops) { >> >> + ret = -ENOMEM; >> >> + goto out; >> >> + } >> > >> > I think you really shouldn't allocate the sdhci_ops dynamically. >> > In fact, it would be much better if we were able to mark them >> > as const in all drivers. >> >> We found some issues when supporting multi-slot, each slot may have >> different ops. >> So use the method of allocating the sdhci_ops dynamically instead of >> using static ops. >> For example, emmc has 74_clocks call back, while mmc and sdio slot >> does not have such ops. >> If not dynamically allocate sdhci_ops, all slot ops may become same, >> and 74_clocks may be called for every slot. >> Also, some board may have get_ro, while other board may not, so >> transfer the ops via flags. >> >> Not sure whether it is worthy to add additional common files to share >> probe and remove function. >> Also the init the ops part are different. > > IMHO, we should try much harder to keep the ops constant. For the > two examples you gave, I would probably put a flag into private data for > the get_ro operation to signify that it's never read-only, and provide > a second set of operations for emmc, so that in the probe function > you check the type of device and then set the appropriate one. > > You could also do the same for all three cases: > > if (emmc) > host->ops = &sdhci_mmp2_emmc_ops; > else if (!has_ro_switch) > host->ops = &sdhci_mmp2_rw_ops; > else > host->ops = &sdhci_mmp2_default_ops; Thanks, will take this method to make the ops constant. > >> >> + >> >> +#ifdef CONFIG_PM >> >> +static int sdhci_mmp2_suspend(struct platform_device *dev, pm_message_t state) >> >> +{ >> >> + struct sdhci_host *host = platform_get_drvdata(dev); >> >> + >> >> + return sdhci_suspend_host(host, state); >> >> +} >> >> + >> >> +static int sdhci_mmp2_resume(struct platform_device *dev) >> >> +{ >> >> + struct sdhci_host *host = platform_get_drvdata(dev); >> >> + >> >> + return sdhci_resume_host(host); >> >> +} >> >> +#else >> >> +#define sdhci_mmp2_suspend NULL >> >> +#define sdhci_mmp2_resume NULL >> >> +#endif >> > >> > Similarly, I think it would be good if sdhci-pltfm.c would simply >> > export these functions so you could refer to them in your driver. >> > There is no need to have identical copies in each variant. >> > >> >> There are some additional code for suspend and resume, so >> sdhci_pltfm_suspend may not enough. >> For example, when enable wifi host sleep feature, additional register >> have to be configured. > > Well, not all drivers would have to use the common functions, but > right now there are a number of identical copies, so it's certainly > worth sharing. Then we use sdhci_pltfm_suspend first, and change to private suspend & resume when supporting additional feature. > > Note that this comment wasn't directed at your patch as much as at > the overall design of the sdhci sub-drivers. > > Arnd > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On May 29, 2011, at 10:42 PM, zhangfei gao wrote: >> >> There is a little room for simplification, I think: >> >>> +static unsigned int mmp2_get_ro(struct sdhci_host *host) >>> +{ >>> + /* Micro SD does not support write-protect feature */ >>> + return 0; >>> +} >> >> You shouldn't need to provide an empty get_ro function, the >> default is that there is no write-protect. > > Thanks Arnd for review. > The reason to put get_ro here is some board use micro sd, while some > board design is general sd card. > The micro sd do not use write-protect, while SDHCI_WRITE_PROTECT will > return 1 in our controller, so it shows read only. > So add one call back for the board with micro sd card via flag. The code sets get_ro is for all the controllers. Some board designs may connect the WP signal. The host->ops field should be filled in using information in the board file using a flag such as the example with PXA_FLAG_CARD_PERMANENT. eg if (pdata && pdata->flags & PXA_FLAG_NO_WP) { ***** etc **** } -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 8, 2011 at 7:51 AM, Philip Rakity <prakity@marvell.com> wrote: > > On May 29, 2011, at 10:42 PM, zhangfei gao wrote: > >>> >>> There is a little room for simplification, I think: >>> >>>> +static unsigned int mmp2_get_ro(struct sdhci_host *host) >>>> +{ >>>> + /* Micro SD does not support write-protect feature */ >>>> + return 0; >>>> +} >>> >>> You shouldn't need to provide an empty get_ro function, the >>> default is that there is no write-protect. >> >> Thanks Arnd for review. >> The reason to put get_ro here is some board use micro sd, while some >> board design is general sd card. >> The micro sd do not use write-protect, while SDHCI_WRITE_PROTECT will >> return 1 in our controller, so it shows read only. >> So add one call back for the board with micro sd card via flag. > > > > The code sets get_ro is for all the controllers. Some board designs may connect the WP signal. > > The host->ops field should be filled in using information in the board > file using a flag such as the example with PXA_FLAG_CARD_PERMANENT. > > eg > > if (pdata && pdata->flags & PXA_FLAG_NO_WP) { > ***** etc **** > } > > This method is used in v1, The method makes sdhci_ops non-const and have to dynamically alloc sdhci_ops, since host->ops is pointer. Will keep use const sdhci_ops, recommended by Arnd. > > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Jun 7, 2011, at 7:56 PM, zhangfei gao wrote: > On Wed, Jun 8, 2011 at 7:51 AM, Philip Rakity <prakity@marvell.com> wrote: >> >> On May 29, 2011, at 10:42 PM, zhangfei gao wrote: >> >>>> >>>> There is a little room for simplification, I think: >>>> >>>>> +static unsigned int mmp2_get_ro(struct sdhci_host *host) >>>>> +{ >>>>> + /* Micro SD does not support write-protect feature */ >>>>> + return 0; >>>>> +} >>>> >>>> You shouldn't need to provide an empty get_ro function, the >>>> default is that there is no write-protect. >>> >>> Thanks Arnd for review. >>> The reason to put get_ro here is some board use micro sd, while some >>> board design is general sd card. >>> The micro sd do not use write-protect, while SDHCI_WRITE_PROTECT will >>> return 1 in our controller, so it shows read only. >>> So add one call back for the board with micro sd card via flag. >> >> >> >> The code sets get_ro is for all the controllers. Some board designs may connect the WP signal. >> >> The host->ops field should be filled in using information in the board >> file using a flag such as the example with PXA_FLAG_CARD_PERMANENT. >> >> eg >> >> if (pdata && pdata->flags & PXA_FLAG_NO_WP) { >> ***** etc **** >> } >> >> > > This method is used in v1, > The method makes sdhci_ops non-const and have to dynamically alloc > sdhci_ops, since host->ops is pointer. > Will keep use const sdhci_ops, recommended by Arnd. Not acceptable. There are 3 cases for WP signal a) They are connected to the controller using GPIO b( They are NOT connected and the corresponding pin is not used c) They are NOT connected and the corresponding pin is used by something else. case a) program GPIO as is done today in brownstone and aspen case b) configure the MFP pin to be pull high/low to indicate NO WP. The pin cannot be left floating case c) you need a quirk and the call back. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/plat-pxa/include/plat/sdhci.h b/arch/arm/plat-pxa/include/plat/sdhci.h index 1ab332e..c23b6b5 100644 --- a/arch/arm/plat-pxa/include/plat/sdhci.h +++ b/arch/arm/plat-pxa/include/plat/sdhci.h @@ -13,23 +13,58 @@ #ifndef __PLAT_PXA_SDHCI_H #define __PLAT_PXA_SDHCI_H +#include <linux/mmc/sdhci.h> +#include <linux/platform_device.h> + /* pxa specific flag */ /* Require clock free running */ #define PXA_FLAG_DISABLE_CLOCK_GATING (1<<0) - +/* card alwayes wired to host, like on-chip emmc */ +#define PXA_FLAG_CARD_PERMANENT (1<<1) /* Board design supports 8-bit data on SD/SDIO BUS */ #define PXA_FLAG_SD_8_BIT_CAPABLE_SLOT (1<<2) +/* card not use wp, such as micro sd card */ +#define PXA_FLAG_CARD_NO_WP (1<<3) +struct sdhci_pxa; /* * struct pxa_sdhci_platdata() - Platform device data for PXA SDHCI - * @max_speed: the maximum speed supported - * @quirks: quirks of specific device * @flags: flags for platform requirement + * @clk_delay_cycles: + * mmp2: each step is roughly 100ps, 5bits width + * pxa910: each step is 1ns, 4bits width + * @clk_delay_sel: select clk_delay, used on pxa910 + * 0: choose feedback clk + * 1: choose feedback clk + delay value + * 2: choose internal clk + * @ext_cd_gpio: gpio pin used for external CD line + * @ext_cd_gpio_invert: invert values for external CD gpio line + * @clk_delay_enable: enable clk_delay or not, used on pxa910 + * @max_speed: the maximum speed supported + * @host_caps: Standard MMC host capabilities bit field. + * @quirks: quirks of platfrom + * @pm_caps: pm_caps of platfrom */ struct sdhci_pxa_platdata { + unsigned int flags; + unsigned int clk_delay_cycles; + unsigned int clk_delay_sel; + unsigned int ext_cd_gpio; + bool ext_cd_gpio_invert; + bool clk_delay_enable; unsigned int max_speed; + unsigned int host_caps; unsigned int quirks; - unsigned int flags; + unsigned int pm_caps; +}; + +struct sdhci_pxa { + struct sdhci_pxa_platdata *pdata; + struct clk *clk; + struct sdhci_ops *ops; + + u8 clk_enable; + u8 power_mode; }; #endif /* __PLAT_PXA_SDHCI_H */ diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig index 862235e..513df18 100644 --- a/drivers/mmc/host/Kconfig +++ b/drivers/mmc/host/Kconfig @@ -181,14 +181,15 @@ config MMC_SDHCI_S3C If unsure, say N. -config MMC_SDHCI_PXA - tristate "Marvell PXA168/PXA910/MMP2 SD Host Controller support" - depends on ARCH_PXA || ARCH_MMP +config MMC_SDHCI_MMP2 + tristate "Marvell MMP2 SD Host Controller support" + depends on ARCH_MMP select MMC_SDHCI + select MMC_SDHCI_PLTFM select MMC_SDHCI_IO_ACCESSORS help - This selects the Marvell(R) PXA168/PXA910/MMP2 SD Host Controller. - If you have a PXA168/PXA910/MMP2 platform with SD Host Controller + This selects the Marvell(R) MMP2 SD Host Controller. + If you have a MMP2 platform with SD Host Controller and a card slot, say Y or M here. If unsure, say N. diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile index 4933004..f8650e0 100644 --- a/drivers/mmc/host/Makefile +++ b/drivers/mmc/host/Makefile @@ -9,7 +9,7 @@ obj-$(CONFIG_MMC_MXC) += mxcmmc.o obj-$(CONFIG_MMC_MXS) += mxs-mmc.o obj-$(CONFIG_MMC_SDHCI) += sdhci.o obj-$(CONFIG_MMC_SDHCI_PCI) += sdhci-pci.o -obj-$(CONFIG_MMC_SDHCI_PXA) += sdhci-pxa.o +obj-$(CONFIG_MMC_SDHCI_MMP2) += sdhci-mmp2.o obj-$(CONFIG_MMC_SDHCI_S3C) += sdhci-s3c.o obj-$(CONFIG_MMC_SDHCI_SPEAR) += sdhci-spear.o obj-$(CONFIG_MMC_WBSD) += wbsd.o diff --git a/drivers/mmc/host/sdhci-mmp2.c b/drivers/mmc/host/sdhci-mmp2.c new file mode 100644 index 0000000..efb5751 --- /dev/null +++ b/drivers/mmc/host/sdhci-mmp2.c @@ -0,0 +1,303 @@ +/* + * Copyright (C) 2010 Marvell International Ltd. + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#include <linux/err.h> +#include <linux/init.h> +#include <linux/platform_device.h> +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/gpio.h> +#include <linux/mmc/card.h> +#include <linux/mmc/host.h> +#include <plat/sdhci.h> +#include <linux/slab.h> +#include "sdhci.h" +#include "sdhci-pltfm.h" + +#define MMP2_SD_FIFO_PARAM 0x104 +#define MMP2_DIS_PAD_SD_CLK_GATE 0x400 + +#define MMP2_SD_CLOCK_AND_BURST_SIZE_SETUP 0x10A +#define MMP2_SDCLK_SEL 0x100 +#define MMP2_SDCLK_DELAY_SHIFT 9 +#define MMP2_SDCLK_DELAY_MASK 0x1f + +#define SD_CFG_FIFO_PARAM 0x100 +#define SDCFG_GEN_PAD_CLK_ON (1<<6) +#define SDCFG_GEN_PAD_CLK_CNT_MASK 0xFF +#define SDCFG_GEN_PAD_CLK_CNT_SHIFT 24 + +#define SD_SPI_MODE 0x108 +#define SD_CE_ATA_1 0x10C + +#define SD_CE_ATA_2 0x10E +#define SDCE_MISC_INT (1<<2) +#define SDCE_MISC_INT_EN (1<<1) + +static void mmp2_soc_set_timing(struct sdhci_host *host, + struct sdhci_pxa_platdata *pdata) +{ + /* + * tune timing of read data/command when crc error happen + * no performance impact + */ + if (pdata && 0 != pdata->clk_delay_cycles) { + u16 tmp; + + tmp = readw(host->ioaddr + MMP2_SD_CLOCK_AND_BURST_SIZE_SETUP); + tmp |= (pdata->clk_delay_cycles & MMP2_SDCLK_DELAY_MASK) + << MMP2_SDCLK_DELAY_SHIFT; + tmp |= MMP2_SDCLK_SEL; + writew(tmp, host->ioaddr + MMP2_SD_CLOCK_AND_BURST_SIZE_SETUP); + } + + /* + * disable clock gating per some cards requirement + */ + + if (pdata && pdata->flags & PXA_FLAG_DISABLE_CLOCK_GATING) { + u32 tmp = 0; + + tmp = readl(host->ioaddr + MMP2_SD_FIFO_PARAM); + tmp |= MMP2_DIS_PAD_SD_CLK_GATE; + writel(tmp, host->ioaddr + MMP2_SD_FIFO_PARAM); + } +} + +static unsigned int mmp2_get_ro(struct sdhci_host *host) +{ + /* Micro SD does not support write-protect feature */ + return 0; +} + +static void mmp2_set_clock(struct sdhci_host *host, unsigned int clock) +{ + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_pxa *pxa = pltfm_host->priv; + + if (clock) + mmp2_soc_set_timing(host, pxa->pdata); +} + +static int mmp2_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) +{ + u16 ctrl_2; + + /* + * Set V18_EN -- UHS modes do not work without this. + * does not change signaling voltage + */ + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2); + + /* Select Bus Speed Mode for host */ + ctrl_2 &= ~SDHCI_CTRL_UHS_MASK; + switch (uhs) { + case MMC_TIMING_UHS_SDR12: + ctrl_2 |= SDHCI_CTRL_UHS_SDR12; + break; + case MMC_TIMING_UHS_SDR25: + ctrl_2 |= SDHCI_CTRL_UHS_SDR25; + break; + case MMC_TIMING_UHS_SDR50: + ctrl_2 |= SDHCI_CTRL_UHS_SDR50 | SDHCI_CTRL_VDD_180; + break; + case MMC_TIMING_UHS_SDR104: + ctrl_2 |= SDHCI_CTRL_UHS_SDR104 | SDHCI_CTRL_VDD_180; + break; + case MMC_TIMING_UHS_DDR50: + ctrl_2 |= SDHCI_CTRL_UHS_DDR50 | SDHCI_CTRL_VDD_180; + break; + } + + sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2); + dev_dbg(mmc_dev(host->mmc), + "%s:%s uhs = %d, ctrl_2 = %04X\n", + __func__, mmc_hostname(host->mmc), uhs, ctrl_2); + + return 0; +} + +static int __devinit sdhci_mmp2_probe(struct platform_device *pdev) +{ + struct sdhci_pltfm_host *pltfm_host; + struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data; + struct device *dev = &pdev->dev; + struct sdhci_host *host = NULL; + struct sdhci_pxa *pxa = NULL; + int ret; + struct clk *clk; + + pxa = kzalloc(sizeof(struct sdhci_pxa), GFP_KERNEL); + if (!pxa) { + ret = -ENOMEM; + goto out; + } + pxa->ops = kzalloc(sizeof(struct sdhci_ops), GFP_KERNEL); + if (!pxa->ops) { + ret = -ENOMEM; + goto out; + } + pxa->pdata = pdata; + + clk = clk_get(dev, "PXA-SDHCLK"); + if (IS_ERR(clk)) { + dev_err(dev, "failed to get io clock\n"); + ret = PTR_ERR(clk); + goto out; + } + pxa->clk = clk; + clk_enable(clk); + + host = sdhci_pltfm_init(pdev, NULL); + if (IS_ERR(host)) + return PTR_ERR(host); + + pltfm_host = sdhci_priv(host); + pltfm_host->priv = pxa; + + host->quirks = SDHCI_QUIRK_BROKEN_ADMA + | SDHCI_QUIRK_BROKEN_TIMEOUT_VAL + | SDHCI_QUIRK_32BIT_DMA_ADDR + | SDHCI_QUIRK_32BIT_DMA_SIZE + | SDHCI_QUIRK_32BIT_ADMA_SIZE + | SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC; + + /* enable 1/8V DDR capable */ + host->mmc->caps |= MMC_CAP_1_8V_DDR; + + if (pdata && pdata->flags & PXA_FLAG_CARD_PERMANENT) { + /* on-chip device */ + host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION; + host->mmc->caps |= MMC_CAP_NONREMOVABLE; + } + + /* If slot design supports 8 bit data, indicate this to MMC. */ + if (pdata->flags & PXA_FLAG_SD_8_BIT_CAPABLE_SLOT) + host->mmc->caps |= MMC_CAP_8_BIT_DATA; + + if (pdata && pdata->quirks) + host->quirks |= pdata->quirks; + if (pdata && pdata->host_caps) + host->mmc->caps |= pdata->host_caps; + if (pdata && pdata->pm_caps) + host->mmc->pm_caps |= pdata->pm_caps; + + pxa->ops->set_clock = mmp2_set_clock; + pxa->ops->set_uhs_signaling = mmp2_set_uhs_signaling; + if (pdata && pdata->flags & PXA_FLAG_CARD_NO_WP) + pxa->ops->get_ro = mmp2_get_ro; + + host->ops = pxa->ops; + + ret = sdhci_add_host(host); + if (ret) { + dev_err(&pdev->dev, "failed to add host\n"); + goto out; + } + + platform_set_drvdata(pdev, host); + + return 0; +out: + if (host) { + clk_disable(pltfm_host->clk); + clk_put(pltfm_host->clk); + sdhci_pltfm_free(pdev); + } + + if (pxa) + kfree(pxa->ops); + kfree(pxa); + + return ret; +} + +static int __devexit sdhci_mmp2_remove(struct platform_device *pdev) +{ + struct sdhci_host *host = platform_get_drvdata(pdev); + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_pxa *pxa = pltfm_host->priv; + int dead = 0; + u32 scratch; + + if (host) { + scratch = readl(host->ioaddr + SDHCI_INT_STATUS); + if (scratch == (u32)-1) + dead = 1; + + sdhci_remove_host(host, dead); + + clk_disable(pxa->clk); + clk_put(pxa->clk); + sdhci_pltfm_free(pdev); + + platform_set_drvdata(pdev, NULL); + } + + if (pxa) + kfree(pxa->ops); + kfree(pxa); + + return 0; +} + +#ifdef CONFIG_PM +static int sdhci_mmp2_suspend(struct platform_device *dev, pm_message_t state) +{ + struct sdhci_host *host = platform_get_drvdata(dev); + + return sdhci_suspend_host(host, state); +} + +static int sdhci_mmp2_resume(struct platform_device *dev) +{ + struct sdhci_host *host = platform_get_drvdata(dev); + + return sdhci_resume_host(host); +} +#else +#define sdhci_mmp2_suspend NULL +#define sdhci_mmp2_resume NULL +#endif + + +static struct platform_driver sdhci_mmp2_driver = { + .driver = { + .name = "sdhci-mmp2", + .owner = THIS_MODULE, + }, + .probe = sdhci_mmp2_probe, + .remove = __devexit_p(sdhci_mmp2_remove), +#ifdef CONFIG_PM + .suspend = sdhci_mmp2_suspend, + .resume = sdhci_mmp2_resume, +#endif +}; +static int __init sdhci_mmp2_init(void) +{ + return platform_driver_register(&sdhci_mmp2_driver); +} + +static void __exit sdhci_mmp2_exit(void) +{ + platform_driver_unregister(&sdhci_mmp2_driver); +} + +module_init(sdhci_mmp2_init); +module_exit(sdhci_mmp2_exit); + +MODULE_DESCRIPTION("SDHCI driver for mmp2"); +MODULE_AUTHOR("Marvell International Ltd."); +MODULE_LICENSE("GPL v2"); + diff --git a/drivers/mmc/host/sdhci-pxa.c b/drivers/mmc/host/sdhci-pxa.c deleted file mode 100644 index 089c9a6..0000000 --- a/drivers/mmc/host/sdhci-pxa.c +++ /dev/null @@ -1,303 +0,0 @@ -/* linux/drivers/mmc/host/sdhci-pxa.c - * - * Copyright (C) 2010 Marvell International Ltd. - * Zhangfei Gao <zhangfei.gao@marvell.com> - * Kevin Wang <dwang4@marvell.com> - * Mingwei Wang <mwwang@marvell.com> - * Philip Rakity <prakity@marvell.com> - * Mark Brown <markb@marvell.com> - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License version 2 as - * published by the Free Software Foundation. - */ - -/* Supports: - * SDHCI support for MMP2/PXA910/PXA168 - * - * Refer to sdhci-s3c.c. - */ - -#include <linux/delay.h> -#include <linux/platform_device.h> -#include <linux/mmc/host.h> -#include <linux/clk.h> -#include <linux/io.h> -#include <linux/err.h> -#include <plat/sdhci.h> -#include "sdhci.h" - -#define DRIVER_NAME "sdhci-pxa" - -#define SD_FIFO_PARAM 0x104 -#define DIS_PAD_SD_CLK_GATE 0x400 - -struct sdhci_pxa { - struct sdhci_host *host; - struct sdhci_pxa_platdata *pdata; - struct clk *clk; - struct resource *res; - - u8 clk_enable; -}; - -/*****************************************************************************\ - * * - * SDHCI core callbacks * - * * -\*****************************************************************************/ -static void set_clock(struct sdhci_host *host, unsigned int clock) -{ - struct sdhci_pxa *pxa = sdhci_priv(host); - u32 tmp = 0; - - if (clock == 0) { - if (pxa->clk_enable) { - clk_disable(pxa->clk); - pxa->clk_enable = 0; - } - } else { - if (0 == pxa->clk_enable) { - if (pxa->pdata->flags & PXA_FLAG_DISABLE_CLOCK_GATING) { - tmp = readl(host->ioaddr + SD_FIFO_PARAM); - tmp |= DIS_PAD_SD_CLK_GATE; - writel(tmp, host->ioaddr + SD_FIFO_PARAM); - } - clk_enable(pxa->clk); - pxa->clk_enable = 1; - } - } -} - -static int set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) -{ - u16 ctrl_2; - - /* - * Set V18_EN -- UHS modes do not work without this. - * does not change signaling voltage - */ - ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2); - - /* Select Bus Speed Mode for host */ - ctrl_2 &= ~SDHCI_CTRL_UHS_MASK; - switch (uhs) { - case MMC_TIMING_UHS_SDR12: - ctrl_2 |= SDHCI_CTRL_UHS_SDR12; - break; - case MMC_TIMING_UHS_SDR25: - ctrl_2 |= SDHCI_CTRL_UHS_SDR25; - break; - case MMC_TIMING_UHS_SDR50: - ctrl_2 |= SDHCI_CTRL_UHS_SDR50 | SDHCI_CTRL_VDD_180; - break; - case MMC_TIMING_UHS_SDR104: - ctrl_2 |= SDHCI_CTRL_UHS_SDR104 | SDHCI_CTRL_VDD_180; - break; - case MMC_TIMING_UHS_DDR50: - ctrl_2 |= SDHCI_CTRL_UHS_DDR50 | SDHCI_CTRL_VDD_180; - break; - } - - sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2); - pr_debug("%s:%s uhs = %d, ctrl_2 = %04X\n", - __func__, mmc_hostname(host->mmc), uhs, ctrl_2); - - return 0; -} - -static struct sdhci_ops sdhci_pxa_ops = { - .set_uhs_signaling = set_uhs_signaling, - .set_clock = set_clock, -}; - -/*****************************************************************************\ - * * - * Device probing/removal * - * * -\*****************************************************************************/ - -static int __devinit sdhci_pxa_probe(struct platform_device *pdev) -{ - struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data; - struct device *dev = &pdev->dev; - struct sdhci_host *host = NULL; - struct resource *iomem = NULL; - struct sdhci_pxa *pxa = NULL; - int ret, irq; - - irq = platform_get_irq(pdev, 0); - if (irq < 0) { - dev_err(dev, "no irq specified\n"); - return irq; - } - - iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!iomem) { - dev_err(dev, "no memory specified\n"); - return -ENOENT; - } - - host = sdhci_alloc_host(&pdev->dev, sizeof(struct sdhci_pxa)); - if (IS_ERR(host)) { - dev_err(dev, "failed to alloc host\n"); - return PTR_ERR(host); - } - - pxa = sdhci_priv(host); - pxa->host = host; - pxa->pdata = pdata; - pxa->clk_enable = 0; - - pxa->clk = clk_get(dev, "PXA-SDHCLK"); - if (IS_ERR(pxa->clk)) { - dev_err(dev, "failed to get io clock\n"); - ret = PTR_ERR(pxa->clk); - goto out; - } - - pxa->res = request_mem_region(iomem->start, resource_size(iomem), - mmc_hostname(host->mmc)); - if (!pxa->res) { - dev_err(&pdev->dev, "cannot request region\n"); - ret = -EBUSY; - goto out; - } - - host->ioaddr = ioremap(iomem->start, resource_size(iomem)); - if (!host->ioaddr) { - dev_err(&pdev->dev, "failed to remap registers\n"); - ret = -ENOMEM; - goto out; - } - - host->hw_name = "MMC"; - host->ops = &sdhci_pxa_ops; - host->irq = irq; - host->quirks = SDHCI_QUIRK_BROKEN_ADMA - | SDHCI_QUIRK_BROKEN_TIMEOUT_VAL - | SDHCI_QUIRK_32BIT_DMA_ADDR - | SDHCI_QUIRK_32BIT_DMA_SIZE - | SDHCI_QUIRK_32BIT_ADMA_SIZE - | SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC; - - if (pdata->quirks) - host->quirks |= pdata->quirks; - - /* enable 1/8V DDR capable */ - host->mmc->caps |= MMC_CAP_1_8V_DDR; - - /* If slot design supports 8 bit data, indicate this to MMC. */ - if (pdata->flags & PXA_FLAG_SD_8_BIT_CAPABLE_SLOT) - host->mmc->caps |= MMC_CAP_8_BIT_DATA; - - ret = sdhci_add_host(host); - if (ret) { - dev_err(&pdev->dev, "failed to add host\n"); - goto out; - } - - if (pxa->pdata->max_speed) - host->mmc->f_max = pxa->pdata->max_speed; - - platform_set_drvdata(pdev, host); - - return 0; -out: - if (host) { - clk_put(pxa->clk); - if (host->ioaddr) - iounmap(host->ioaddr); - if (pxa->res) - release_mem_region(pxa->res->start, - resource_size(pxa->res)); - sdhci_free_host(host); - } - - return ret; -} - -static int __devexit sdhci_pxa_remove(struct platform_device *pdev) -{ - struct sdhci_host *host = platform_get_drvdata(pdev); - struct sdhci_pxa *pxa = sdhci_priv(host); - int dead = 0; - u32 scratch; - - if (host) { - scratch = readl(host->ioaddr + SDHCI_INT_STATUS); - if (scratch == (u32)-1) - dead = 1; - - sdhci_remove_host(host, dead); - - if (host->ioaddr) - iounmap(host->ioaddr); - if (pxa->res) - release_mem_region(pxa->res->start, - resource_size(pxa->res)); - if (pxa->clk_enable) { - clk_disable(pxa->clk); - pxa->clk_enable = 0; - } - clk_put(pxa->clk); - - sdhci_free_host(host); - platform_set_drvdata(pdev, NULL); - } - - return 0; -} - -#ifdef CONFIG_PM -static int sdhci_pxa_suspend(struct platform_device *dev, pm_message_t state) -{ - struct sdhci_host *host = platform_get_drvdata(dev); - - return sdhci_suspend_host(host, state); -} - -static int sdhci_pxa_resume(struct platform_device *dev) -{ - struct sdhci_host *host = platform_get_drvdata(dev); - - return sdhci_resume_host(host); -} -#else -#define sdhci_pxa_suspend NULL -#define sdhci_pxa_resume NULL -#endif - -static struct platform_driver sdhci_pxa_driver = { - .probe = sdhci_pxa_probe, - .remove = __devexit_p(sdhci_pxa_remove), - .suspend = sdhci_pxa_suspend, - .resume = sdhci_pxa_resume, - .driver = { - .name = DRIVER_NAME, - .owner = THIS_MODULE, - }, -}; - -/*****************************************************************************\ - * * - * Driver init/exit * - * * -\*****************************************************************************/ - -static int __init sdhci_pxa_init(void) -{ - return platform_driver_register(&sdhci_pxa_driver); -} - -static void __exit sdhci_pxa_exit(void) -{ - platform_driver_unregister(&sdhci_pxa_driver); -} - -module_init(sdhci_pxa_init); -module_exit(sdhci_pxa_exit); - -MODULE_DESCRIPTION("SDH controller driver for PXA168/PXA910/MMP2"); -MODULE_AUTHOR("Zhangfei Gao <zhangfei.gao@marvell.com>"); -MODULE_LICENSE("GPL v2");
Instead of sharing sdhci-pxa.c, use sdhci-mmp2.c specificlly for mmp2. sdhci-pxa.c is used to share among pxa serious, like pxa910, mmp2, etc. The platfrom difference are put under arch/arm, while is not easy to track. Signed-off-by: Zhangfei Gao <zhangfei.gao@marvell.com> --- arch/arm/plat-pxa/include/plat/sdhci.h | 43 ++++- drivers/mmc/host/Kconfig | 11 +- drivers/mmc/host/Makefile | 2 +- drivers/mmc/host/sdhci-mmp2.c | 303 ++++++++++++++++++++++++++++++++ drivers/mmc/host/sdhci-pxa.c | 303 -------------------------------- 5 files changed, 349 insertions(+), 313 deletions(-) create mode 100644 drivers/mmc/host/sdhci-mmp2.c delete mode 100644 drivers/mmc/host/sdhci-pxa.c