Hi Will,
On Wed, Sep 28, 2011 at 9:03 PM, Will Newton <will.newton@xxxxxxxxx> wrote:
> On Wed, Sep 28, 2011 at 4:02 PM, Shashidhar Hiremath
> <shashidharh@xxxxxxxxxxxxxxx> wrote:
>>
>> Signed-off-by: Shashidhar Hiremath <shashidharh@xxxxxxxxxxxxxxx>
>> ---
>> drivers/mmc/host/Kconfig | 11 ++++
>> drivers/mmc/host/dw_mmc.c | 126 ++++++++++++++++++++++++++++++++++++++++++++
>> drivers/mmc/host/dw_mmc.h | 12 ++++
>> include/linux/mmc/dw_mmc.h | 4 ++
>> 4 files changed, 153 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 8c87096..84d8908 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -526,6 +526,17 @@ config MMC_DW
>> block, this provides host support for SD and MMC interfaces, in both
>> PIO and external DMA modes.
>>
>> +config MMC_DW_PCI
>> + bool "MMC_DW Support On PCI bus"
>> + depends on MMC_DW && PCI
>> + help
>> + This selects the PCI for the Synopsys Designware Mobile Storage IP.
>> +
>> + If you have a controller with this interface, say Y or M here.
>> +
>> + If unsure, say N.
>> +
>> +
>> config MMC_DW_IDMAC
>> bool "Internal DMAC interface"
>> depends on MMC_DW
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index ff0f714..74f28a5 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -21,7 +21,11 @@
>> #include <linux/interrupt.h>
>> #include <linux/ioport.h>
>> #include <linux/module.h>
>> +#ifdef CONFIG_MMC_DW_PCI
>> +#include <linux/pci.h>
>> +#else
>> #include <linux/platform_device.h>
>> +#endif
>
> This doesn't need an #ifdef.
>
>> #include <linux/scatterlist.h>
>> #include <linux/seq_file.h>
>> #include <linux/slab.h>
>> @@ -101,6 +105,18 @@ struct dw_mci_slot {
>> int last_detect_state;
>> };
>>
>> +#ifdef CONFIG_MMC_DW_PCI
>> +static struct pci_device_id dw_mci_id = { PCI_DEVICE(DEVICE_ID, VENDOR_ID) };
>> +
>> +struct dw_mci_board pci_board_data = {
>> + .num_slots = 1,
>> + .caps = DW_MCI_CAPABILITIES,
>> + .bus_hz = 33 * 1000 * 1000,
>
> Is this value correct? The bus this refers to is the SD bus, so 33MHz
> is not fast.
I got the driver working with this frequency.Will check the optimal value.
>
>> + .detect_delay_ms = 200,
>> + .fifo_depth = 32,
>> +};
>> +#endif
>> +
>> static struct workqueue_struct *dw_mci_card_workqueue;
>>
>> #if defined(CONFIG_DEBUG_FS)
>> @@ -682,6 +698,9 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>> {
>> struct dw_mci_slot *slot = mmc_priv(mmc);
>> u32 regs;
>> +#ifdef CONFIG_MMC_DW_PCI
>> + u32 card_detect;
>> +#endif
>>
>> /* set default 1 bit mode */
>> slot->ctype = SDMMC_CTYPE_1BIT;
>> @@ -716,7 +735,15 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>> switch (ios->power_mode) {
>> case MMC_POWER_UP:
>> set_bit(DW_MMC_CARD_NEED_INIT, &slot->flags);
>> +#ifdef CONFIG_MMC_DW_PCI
>> + /* Enable Power to the card that has been detected */
>> + card_detect = mci_readl(slot->host, CDETECT);
>> + mci_writel(slot->host, PWREN, ((~card_detect) & 0x1));
>> + break;
>> + case MMC_POWER_OFF:
>> + mci_writel(slot->host, PWREN, 0);
>> break;
>> +#endif
>
> It may be safe to not #ifdef this, all the hardware I have uses an
> external regulator so the PWREN bits do nothing. I could imagine
> hardware that was non-PCI that could use PWREN however.
The hardware that I was using required this to be done .else there was
not power to the card.
>
>> default:
>> break;
>> }
>> @@ -1775,7 +1802,12 @@ static bool mci_wait_reset(struct device *dev, struct dw_mci *host)
>> return false;
>> }
>>
>> +#ifdef CONFIG_MMC_DW_PCI
>> +static int __devinit dw_mci_probe(struct pci_dev *pdev,
>> + const struct pci_device_id *entries)
>> +#else
>> static int dw_mci_probe(struct platform_device *pdev)
>> +#endif
>
> Possibly it would be better to split this into three functions:
>
> dw_mci_pci_probe
> dw_mci_platform_probe
> dw_mci_probe
>
> And then the pci/platform probe functions can call the generic probe
> function to do the common parts of the setup.
>
>> +#ifdef CONFIG_MMC_DW_PCI
>> +static void __devexit dw_mci_remove(struct pci_dev *pdev)
>> +#else
>> static int __exit dw_mci_remove(struct platform_device *pdev)
>> +#endif
>
> Again this should probably be split into separate functions to reduce
> the number of #ifdefs.
>
>> {
>> +
>> +#ifdef CONFIG_MMC_DW_PCI
>> + struct dw_mci *host = pci_get_drvdata(pdev);
>> +#else
>> struct dw_mci *host = platform_get_drvdata(pdev);
>> +#endif
>> int i;
>>
>> mci_writel(host, RINTSTS, 0xFFFFFFFF);
>> mci_writel(host, INTMASK, 0); /* disable all mmc interrupt first */
>>
>> +#ifdef CONFIG_MMC_DW_PCI
>> + pci_set_drvdata(pdev, NULL);
>> +#else
>> platform_set_drvdata(pdev, NULL);
>> +#endif
>>
>> for (i = 0; i < host->num_slots; i++) {
>> dev_dbg(&pdev->dev, "remove slot %d\n", i);
>> @@ -1992,7 +2074,11 @@ static int __exit dw_mci_remove(struct platform_device *pdev)
>> mci_writel(host, CLKENA, 0);
>> mci_writel(host, CLKSRC, 0);
>>
>> +#ifdef CONFIG_MMC_DW_PCI
>> + free_irq(pdev->irq, host);
>> +#else
>> free_irq(platform_get_irq(pdev, 0), host);
>> +#endif
>> destroy_workqueue(dw_mci_card_workqueue);
>> dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
>>
>> @@ -2006,18 +2092,31 @@ static int __exit dw_mci_remove(struct platform_device *pdev)
>>
>> iounmap(host->regs);
>>
>> +#ifdef CONFIG_MMC_DW_PCI
>> + pci_release_regions(pdev);
>> +#endif
>> kfree(host);
>> +#ifndef CONFIG_MMC_DW_PCI
>> return 0;
>> +#endif
>> }
>>
>> #ifdef CONFIG_PM
>> /*
>> * TODO: we should probably disable the clock to the card in the suspend path.
>> */
>> +#ifdef CONFIG_MMC_DW_PCI
>> +static int dw_mci_suspend(struct pci_dev *pdev, pm_message_t mesg)
>> +#else
>> static int dw_mci_suspend(struct platform_device *pdev, pm_message_t mesg)
>> +#endif
>> {
>> int i, ret;
>> +#ifdef CONFIG_MMC_DW_PCI
>> + struct dw_mci *host = pci_get_drvdata(pdev);
>> +#else
>> struct dw_mci *host = platform_get_drvdata(pdev);
>> +#endif
>>
>> for (i = 0; i < host->num_slots; i++) {
>> struct dw_mci_slot *slot = host->slot[i];
>> @@ -2040,10 +2139,18 @@ static int dw_mci_suspend(struct platform_device *pdev, pm_message_t mesg)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_MMC_DW_PCI
>> +static int dw_mci_resume(struct pci_dev *pdev)
>> +#else
>> static int dw_mci_resume(struct platform_device *pdev)
>> +#endif
>> {
>> int i, ret;
>> +#ifdef CONFIG_MMC_DW_PCI
>> + struct dw_mci *host = pci_get_drvdata(pdev);
>> +#else
>> struct dw_mci *host = platform_get_drvdata(pdev);
>> +#endif
>
> Even though these functions are quite simple it would be better to code them as
>
> dw_mci_platform_resume(struct platform_device *pdev)
> calling dw_mci_resume(struct dw_mci *host)
>
> dw_mci_pci_resume(struct pci_dev *pdev)
> calling dw_mci_resume(struct dw_mci *host)
>
> To reduce the #ifdef overhead.
I will add separate functions for probe/suspend/resume/remove etc for
PCI/non-PCI.
>
>>
>> if (host->vmmc)
>> regulator_enable(host->vmmc);
>> @@ -2081,6 +2188,16 @@ static int dw_mci_resume(struct platform_device *pdev)
>> #define dw_mci_resume NULL
>> #endif /* CONFIG_PM */
>>
>> +#ifdef CONFIG_MMC_DW_PCI
>> +static struct pci_driver dw_mci_driver = {
>> + .name = "dw_mmc",
>> + .id_table = &dw_mci_id,
>> + .probe = dw_mci_probe,
>> + .remove = dw_mci_remove,
>> + .suspend = dw_mci_suspend,
>> + .resume = dw_mci_resume,
>> +};
>> +#else
>> static struct platform_driver dw_mci_driver = {
>> .remove = __exit_p(dw_mci_remove),
>> .suspend = dw_mci_suspend,
>> @@ -2089,15 +2206,24 @@ static struct platform_driver dw_mci_driver = {
>> .name = "dw_mmc",
>> },
>> };
>> +#endif
>>
>> static int __init dw_mci_init(void)
>> {
>> +#ifdef CONFIG_MMC_DW_PCI
>> + return pci_register_driver(&dw_mci_driver);
>> +#else
>> return platform_driver_probe(&dw_mci_driver, dw_mci_probe);
>> +#endif
>> }
>>
>> static void __exit dw_mci_exit(void)
>> {
>> +#ifdef CONFIG_MMC_DW_PCI
>> + pci_unregister_driver(&dw_mci_driver);
>> +#else
>> platform_driver_unregister(&dw_mci_driver);
>> +#endif
>> }
>>
>> module_init(dw_mci_init);
>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>> index 027d377..3b1e459 100644
>> --- a/drivers/mmc/host/dw_mmc.h
>> +++ b/drivers/mmc/host/dw_mmc.h
>> @@ -164,4 +164,16 @@
>> (*(volatile u64 __force *)((dev)->regs + SDMMC_##reg) = (value))
>> #endif
>>
>> +/* PCI Specific macros */
>> +#ifdef CONFIG_MMC_DW_PCI
>> +#define PCI_BAR_NO 2
>> +#define COMPLETE_BAR 0
>> +/* Dummy Device Id and Vendor Id */
>> +#define VENDOR_ID 0x700
>> +#define DEVICE_ID 0x1107
>> +/* Defining the Capabilities */
>> +#define DW_MCI_CAPABILITIES (MMC_CAP_4_BIT_DATA | MMC_CAP_MMC_HIGHSPEED |\
>> + MMC_CAP_SD_HIGHSPEED | MMC_CAP_8_BIT_DATA | MMC_CAP_SDIO_IRQ)
>> +#endif
>> +
>
> Are these all necessary?
I feel the defines other than capabilities are necessary are a
must,but will validate them once again
>
--
regards,
Shashidhar Hiremath
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
