Hi
On 10/03/2011 07:50 AM, Shashidhar Hiremath wrote:
> Hi James,
> Can I add separate files for PCI/Platform and from there call
> common probe/remove api's similar to SDHCI driver in kernel ?
yes, that sounds like a good idea.
In what way are the PCI device and vendor ids dummy?
Cheers
James
>
> On Fri, Sep 30, 2011 at 2:12 PM, James Hogan <james.ho...@imgtec.com> wrote:
>> Hi
>>
>> On 09/29/2011 06:40 PM, Shashidhar Hiremath wrote:
>>> 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 platform. The Driver has also been tested for on the PCI platform with
>>> single Card Slot.
>>>
>>> Signed-off-by: Shashidhar Hiremath <shashidh...@vayavyalabs.com>
>>> ---
>>> v2:
>>> *As per Suggestions by Will Newton and James Hogan
>>> -Reduced the number of ifdefs
>>>
>>> drivers/mmc/host/Kconfig | 11 ++
>>> drivers/mmc/host/dw_mmc.c | 352
>>> ++++++++++++++++++++++++++++++++++++++++++++
>>> drivers/mmc/host/dw_mmc.h | 13 ++
>>> include/linux/mmc/dw_mmc.h | 4 +
>>> 4 files changed, 380 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..0bd9e16 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -21,6 +21,7 @@
>>> #include <linux/interrupt.h>
>>> #include <linux/ioport.h>
>>> #include <linux/module.h>
>>> +#include <linux/pci.h>
>>> #include <linux/platform_device.h>
>>> #include <linux/scatterlist.h>
>>> #include <linux/seq_file.h>
>>> @@ -101,6 +102,7 @@ struct dw_mci_slot {
>>> int last_detect_state;
>>> };
>>>
>>> +
>>> static struct workqueue_struct *dw_mci_card_workqueue;
>>>
>>> #if defined(CONFIG_DEBUG_FS)
>>> @@ -682,6 +684,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct
>>> mmc_ios *ios)
>>> {
>>> struct dw_mci_slot *slot = mmc_priv(mmc);
>>> u32 regs;
>>> + u32 card_detect;
>>>
>>> /* set default 1 bit mode */
>>> slot->ctype = SDMMC_CTYPE_1BIT;
>>> @@ -716,6 +719,13 @@ 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);
>>> + /* Enable Power to the card that has been detected */
>>> + card_detect = mci_readl(slot->host, CDETECT);
>>> + /* Enabling power for card 0 when PCI is the interface */
>>> + mci_writel(slot->host, PWREN, ((~card_detect) & 0x1));
>>> + break;
>>> + case MMC_POWER_OFF:
>>> + mci_writel(slot->host, PWREN, 0);
>>> break;
>>> default:
>>> break;
>>> @@ -1775,6 +1785,345 @@ static bool mci_wait_reset(struct device *dev,
>>> struct dw_mci *host)
>>> return false;
>>> }
>>>
>>> +#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,
>>> + .detect_delay_ms = 200,
>>> + .fifo_depth = 32,
>>> +};
>>> +
>>> +static int __devinit dw_mci_probe(struct pci_dev *pdev,
>>> + const struct pci_device_id *entries)
>>> +{
>>> + struct dw_mci *host;
>>> + struct resource *regs;
>>> + struct dw_mci_board *pdata;
>>> + int irq, ret, i, width;
>>> + u32 fifo_size;
>>> +
>>> + ret = pci_enable_device(pdev);
>>> + if (ret)
>>> + return ret;
>>> + if (pci_request_regions(pdev, "dw_mmc")) {
>>> + ret = -ENODEV;
>>> + goto err_freehost;
>>> + }
>>> + irq = pdev->irq;
>>> +
>>> + host = kzalloc(sizeof(struct dw_mci), GFP_KERNEL);
>>> + if (!host)
>>> + return -ENOMEM;
>>> +
>>> + host->pdev = pdev;
>>> + host->pdata = pdata = &pci_board_data;
>>> +
>>> + if (!pdata->select_slot && pdata->num_slots > 1) {
>>> + dev_err(&pdev->dev,
>>> + "Platform data must supply select_slot function\n");
>>> + ret = -ENODEV;
>>> + goto err_freehost;
>>> + }
>>> +
>>> + if (!pdata->bus_hz) {
>>> + dev_err(&pdev->dev,
>>> + "Platform data must supply bus speed\n");
>>> + ret = -ENODEV;
>>> + goto err_freehost;
>>> + }
>>> +
>>> + host->bus_hz = pdata->bus_hz;
>>> + host->quirks = pdata->quirks;
>>> +
>>> + spin_lock_init(&host->lock);
>>> + INIT_LIST_HEAD(&host->queue);
>>> +
>>> + ret = -ENOMEM;
>>> + host->regs = pci_iomap(pdev, PCI_BAR_NO, COMPLETE_BAR);
>>> + if (!host->regs) {
>>> + ret = -EIO;
>>> + goto err_free_res;
>>> + }
>>> +
>>> + host->dma_ops = pdata->dma_ops;
>>> + dw_mci_init_dma(host);
>>> +
>>> + /*
>>> + * Get the host data width - this assumes that HCON has been set with
>>> + * the correct values.
>>> + */
>>> + i = (mci_readl(host, HCON) >> 7) & 0x7;
>>> + if (!i) {
>>> + host->push_data = dw_mci_push_data16;
>>> + host->pull_data = dw_mci_pull_data16;
>>> + width = 16;
>>> + host->data_shift = 1;
>>> + } else if (i == 2) {
>>> + host->push_data = dw_mci_push_data64;
>>> + host->pull_data = dw_mci_pull_data64;
>>> + width = 64;
>>> + host->data_shift = 3;
>>> + } else {
>>> + /* Check for a reserved value, and warn if it is */
>>> + WARN((i != 1),
>>> + "HCON reports a reserved host data width!\n"
>>> + "Defaulting to 32-bit access.\n");
>>> + host->push_data = dw_mci_push_data32;
>>> + host->pull_data = dw_mci_pull_data32;
>>> + width = 32;
>>> + host->data_shift = 2;
>>> + }
>>> +
>>> + /* Reset all blocks */
>>> + if (!mci_wait_reset(&pdev->dev, host)) {
>>> + ret = -ENODEV;
>>> + goto err_dmaunmap;
>>> + }
>>> +
>>> + /* Clear the interrupts for the host controller */
>>> + mci_writel(host, RINTSTS, 0xFFFFFFFF);
>>> + mci_writel(host, INTMASK, 0); /* disable all mmc interrupt first */
>>> +
>>> + /* Put in max timeout */
>>> + mci_writel(host, TMOUT, 0xFFFFFFFF);
>>> +
>>> + /*
>>> + * FIFO threshold settings RxMark = fifo_size / 2 - 1,
>>> + * Tx Mark = fifo_size / 2 DMA Size = 8
>>> + */
>>> + if (!host->pdata->fifo_depth) {
>>> + /*
>>> + * Power-on value of RX_WMark is FIFO_DEPTH-1, but this may
>>> + * have been overwritten by the bootloader, just like we're
>>> + * about to do, so if you know the value for your hardware,
>>> you
>>> + * should put it in the platform data.
>>> + */
>>> + fifo_size = mci_readl(host, FIFOTH);
>>> + fifo_size = 1 + ((fifo_size >> 16) & 0x7ff);
>>> + } else {
>>> + fifo_size = host->pdata->fifo_depth;
>>> + }
>>> + host->fifo_depth = fifo_size;
>>> + host->fifoth_val = ((0x2 << 28) | ((fifo_size/2 - 1) << 16) |
>>> + ((fifo_size/2) << 0));
>>> + mci_writel(host, FIFOTH, host->fifoth_val);
>>> +
>>> + /* disable clock to CIU */
>>> + mci_writel(host, CLKENA, 0);
>>> + mci_writel(host, CLKSRC, 0);
>>> +
>>> + tasklet_init(&host->tasklet, dw_mci_tasklet_func, (unsigned
>>> long)host);
>>> + dw_mci_card_workqueue = alloc_workqueue("dw-mci-card",
>>> + WQ_MEM_RECLAIM | WQ_NON_REENTRANT, 1);
>>> + if (!dw_mci_card_workqueue)
>>> + goto err_dmaunmap;
>>> + INIT_WORK(&host->card_work, dw_mci_work_routine_card);
>>> +
>>> + ret = request_irq(irq, dw_mci_interrupt, IRQF_SHARED, "dw-mci", host);
>>> + if (ret)
>>> + goto err_workqueue;
>>> +
>>> + pci_set_drvdata(pdev, host);
>>> +
>>> + if (host->pdata->num_slots)
>>> + host->num_slots = host->pdata->num_slots;
>>> + else
>>> + host->num_slots = ((mci_readl(host, HCON) >> 1) & 0x1F) + 1;
>>> +
>>> + /* We need at least one slot to succeed */
>>> + for (i = 0; i < host->num_slots; i++) {
>>> + ret = dw_mci_init_slot(host, i);
>>> + if (ret) {
>>> + ret = -ENODEV;
>>> + goto err_init_slot;
>>> + }
>>> + }
>>> +
>>> + /*
>>> + * Enable interrupts for command done, data over, data empty, card
>>> det,
>>> + * receive ready and error such as transmit, receive timeout, crc
>>> error
>>> + */
>>> + mci_writel(host, RINTSTS, 0xFFFFFFFF);
>>> + mci_writel(host, INTMASK, SDMMC_INT_CMD_DONE | SDMMC_INT_DATA_OVER |
>>> + SDMMC_INT_TXDR | SDMMC_INT_RXDR |
>>> + DW_MCI_ERROR_FLAGS | SDMMC_INT_CD);
>>> + mci_writel(host, CTRL, SDMMC_CTRL_INT_ENABLE); /* Enable mci
>>> interrupt */
>>> +
>>> + dev_info(&pdev->dev, "DW MMC controller at irq %d, "
>>> + "%d bit host data width, "
>>> + "%u deep fifo\n",
>>> + irq, width, fifo_size);
>>> + if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO)
>>> + dev_info(&pdev->dev, "Internal DMAC interrupt fix
>>> enabled.\n");
>>> +
>>> + return 0;
>>> +
>>> +err_init_slot:
>>> + /* De-init any initialized slots */
>>> + while (i > 0) {
>>> + if (host->slot[i])
>>> + dw_mci_cleanup_slot(host->slot[i], i);
>>> + i--;
>>> + }
>>> + free_irq(irq, host);
>>> +
>>> +err_workqueue:
>>> + destroy_workqueue(dw_mci_card_workqueue);
>>> +
>>> +err_dmaunmap:
>>> + if (host->use_dma && host->dma_ops->exit)
>>> + host->dma_ops->exit(host);
>>> + dma_free_coherent(&host->pdev->dev, PAGE_SIZE,
>>> + host->sg_cpu, host->sg_dma);
>>> + iounmap(host->regs);
>>> +
>>> + if (host->vmmc) {
>>> + regulator_disable(host->vmmc);
>>> + regulator_put(host->vmmc);
>>> + }
>>> +
>>> +err_free_res:
>>> + pci_release_regions(pdev);
>>> +
>>> +err_freehost:
>>> + kfree(host);
>>> + return ret;
>>> +}
>>> +
>>> +static void __devexit dw_mci_remove(struct pci_dev *pdev)
>>> +{
>>> +
>>> + struct dw_mci *host = pci_get_drvdata(pdev);
>>> + int i;
>>> +
>>> + mci_writel(host, RINTSTS, 0xFFFFFFFF);
>>> + mci_writel(host, INTMASK, 0); /* disable all mmc interrupt first */
>>> +
>>> + pci_set_drvdata(pdev, NULL);
>>> +
>>> + for (i = 0; i < host->num_slots; i++) {
>>> + dev_dbg(&pdev->dev, "remove slot %d\n", i);
>>> + if (host->slot[i])
>>> + dw_mci_cleanup_slot(host->slot[i], i);
>>> + }
>>> +
>>> + /* disable clock to CIU */
>>> + mci_writel(host, CLKENA, 0);
>>> + mci_writel(host, CLKSRC, 0);
>>> +
>>> + free_irq(pdev->irq, host);
>>> + destroy_workqueue(dw_mci_card_workqueue);
>>> + dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
>>> +
>>> + if (host->use_dma && host->dma_ops->exit)
>>> + host->dma_ops->exit(host);
>>> +
>>> + if (host->vmmc) {
>>> + regulator_disable(host->vmmc);
>>> + regulator_put(host->vmmc);
>>> + }
>>> +
>>> + iounmap(host->regs);
>>> +
>>> + pci_release_regions(pdev);
>>> + kfree(host);
>>> +}
>>> +
>>> +#ifdef CONFIG_PM
>>> +/*
>>> + * TODO: we should probably disable the clock to the card in the suspend
>>> path.
>>> + */
>>> +static int dw_mci_suspend(struct pci_dev *pdev, pm_message_t mesg)
>>> +{
>>> + int i, ret;
>>> + struct dw_mci *host = pci_get_drvdata(pdev);
>>> +
>>> + for (i = 0; i < host->num_slots; i++) {
>>> + struct dw_mci_slot *slot = host->slot[i];
>>> + if (!slot)
>>> + continue;
>>> + ret = mmc_suspend_host(slot->mmc);
>>> + if (ret < 0) {
>>> + while (--i >= 0) {
>>> + slot = host->slot[i];
>>> + if (slot)
>>> + mmc_resume_host(host->slot[i]->mmc);
>>> + }
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + if (host->vmmc)
>>> + regulator_disable(host->vmmc);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int dw_mci_resume(struct pci_dev *pdev)
>>> +{
>>> + int i, ret;
>>> + struct dw_mci *host = pci_get_drvdata(pdev);
>>> +
>>> + if (host->vmmc)
>>> + regulator_enable(host->vmmc);
>>> +
>>> + if (host->dma_ops->init)
>>> + host->dma_ops->init(host);
>>> +
>>> + if (!mci_wait_reset(&pdev->dev, host)) {
>>> + ret = -ENODEV;
>>> + return ret;
>>> + }
>>> +
>>> + /* Restore the old value at FIFOTH register */
>>> + mci_writel(host, FIFOTH, host->fifoth_val);
>>> +
>>> + mci_writel(host, RINTSTS, 0xFFFFFFFF);
>>> + mci_writel(host, INTMASK, SDMMC_INT_CMD_DONE | SDMMC_INT_DATA_OVER |
>>> + SDMMC_INT_TXDR | SDMMC_INT_RXDR |
>>> + DW_MCI_ERROR_FLAGS | SDMMC_INT_CD);
>>> + mci_writel(host, CTRL, SDMMC_CTRL_INT_ENABLE);
>>> +
>>> + for (i = 0; i < host->num_slots; i++) {
>>> + struct dw_mci_slot *slot = host->slot[i];
>>> + if (!slot)
>>> + continue;
>>> + ret = mmc_resume_host(host->slot[i]->mmc);
>>> + if (ret < 0)
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +#else
>>> +#define dw_mci_suspend NULL
>>> +#define dw_mci_resume NULL
>>> +#endif /* CONFIG_PM */
>>> +
>>> +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,
>>> +};
>>> +
>>> +static int __init dw_mci_init(void)
>>> +{
>>> + return pci_register_driver(&dw_mci_driver);
>>> +}
>>> +
>>> +static void __exit dw_mci_exit(void)
>>> +{
>>> + pci_unregister_driver(&dw_mci_driver);
>>> +}
>>> +#else
>>> +
>>
>> Duplication isn't the same as abstraction, and it's much worse to
>> maintain than lots of #ifdefs. What we meant was to abstract the common
>> code into separate static functions which are used by both pci/platform
>> probe/remove/suspend/resume functions (as Will has already described).
>>
>> These functions should also have pci/platform in their names to
>> distinguish them more easily. Also I see no reason why pci and platform
>> registration should be mutually exclusive, i.e. the platform driver can
>> still be registered even if PCI is enabled.
>>
>>> static int dw_mci_probe(struct platform_device *pdev)
>>> {
>>> struct dw_mci *host;
>>> @@ -1974,6 +2323,7 @@ err_freehost:
>>>
>>> static int __exit dw_mci_remove(struct platform_device *pdev)
>>> {
>>> +
>>> struct dw_mci *host = platform_get_drvdata(pdev);
>>> int i;
>>>
>>> @@ -2100,6 +2450,8 @@ static void __exit dw_mci_exit(void)
>>> platform_driver_unregister(&dw_mci_driver);
>>> }
>>>
>>> +#endif/* CONFIG_MMC_DW_PCI */
>>> +
>>> module_init(dw_mci_init);
>>> module_exit(dw_mci_exit);
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>>> index 027d377..54bc604 100644
>>> --- a/drivers/mmc/host/dw_mmc.h
>>> +++ b/drivers/mmc/host/dw_mmc.h
>>> @@ -164,4 +164,17 @@
>>> (*(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
>>
>> Again, in what way are these values dummy? if somebody were to want to
>> use this, would they have to change them for their specific hardware? Is
>> that a common thing to have to do for other drivers like this?
>>
>>> +/* 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 /* CONFIG_MMC_DW_PCI */
>>> +
>>> #endif /* _DW_MMC_H_ */
>>> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
>>> index 6b46819..87af5a6 100644
>>> --- a/include/linux/mmc/dw_mmc.h
>>> +++ b/include/linux/mmc/dw_mmc.h
>>> @@ -147,7 +147,11 @@ struct dw_mci {
>>> u32 current_speed;
>>> u32 num_slots;
>>> u32 fifoth_val;
>>> +#ifdef CONFIG_MMC_DW_PCI
>>> + struct pci_dev *pdev;
>>> +#else
>>> struct platform_device *pdev;
>>> +#endif
>>
>> The only time this is used is to find &host->pdev->dev for logging
>> calls. It'd be better to replace these with a single struct device *,
>> then it'll work with both platform and pci enabled.
>>
>>> struct dw_mci_board *pdata;
>>> struct dw_mci_slot *slot[MAX_MCI_SLOTS];
>>>
>>
>>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
