Re: [PATCH 1/1] mmc: Support of PCI mode for the dw_mmc driver This Patch adds the support for the scenario where the Synopsys Designware IP is present on the PCI bus.The patch adds the minimal modifications necessary for the driver to work on PCI pl

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux