Hi, Doug.
Add one comment.
On 10/16/2014 05:19 PM, Jaehoon Chung wrote:
> Hi, Doug.
>
> Looks good to me.
>
> Tested-by: Jaehoon Chung <jh80.ch...@samsung.com>
> with exynos3/4 series.
>
> Need to check exynos5 with using other "card detect" mechanism.
>
> Best Regards,
> Jaehoon Chung
>
> On 10/15/2014 01:33 AM, Doug Anderson wrote:
>> The dw_mmc driver had a bunch of code that ran whenever a card was
>> ejected and inserted. However, this code was old and crufty and
>> should be removed. Some evidence that it's really not needed:
>>
>> 1. Is is supposed to be legal to use 'cd-gpio' on dw_mmc instead of
>> using the built-in card detect mechanism. The 'cd-gpio' code
>> doesn't run any of the crufty old code but yet still works.
>>
>> 2. While looking at this, I realized that my old change (369ac86 mmc:
>> dw_mmc: don't queue up a card detect at slot startup) actually
>> castrated the old code a little bit already and nobody noticed.
>> Specifically "last_detect_state" was left as 0 at bootup. That
>> means that on the first card removal none of the crufty code ran.
>>
>> 3. I can run "while true; do dd if=/dev/mmcblk1 of=/dev/null; done"
>> while ejecting and inserting an SD Card and the world doesn't
>> explode.
>>
>> If some of the crufty old code is actually needed, we should justify
>> it and also put it in some place where it will be run even with
>> "cd-gpio".
>>
>> Note that in my case I'm using the "cd-gpio" mechanism but for various
>> reasons the hardware triggers a dw_mmc "card detect" at bootup. That
>> was actually causing a real bug. The card detect workqueue was
>> running while the system was trying to enumerate the card. The
>> "present != slot->last_detect_state" triggered and we were doing all
>> kinds of crazy stuff and messing up enumeration. The new mechanism of
>> just asking the core to check the card is much safer and then the
>> bogus interrupt doesn't hurt.
Did you read the Card-detect Recommendation?
There is a Recommendation for Card-detect(CDT) at TRM.
We don't read the CDETECT register(0x50) anywhere. Could you share this
register value after detecting?
And I think "last_detect_state" is used for "software should read card detect
register and store in memory."
Best Regards,
Jaehoon Chung
>>
>> Signed-off-by: Doug Anderson <diand...@chromium.org>
>> ---
>> drivers/mmc/host/dw_mmc.c | 121
>> ++++++++-------------------------------------
>> drivers/mmc/host/dw_mmc.h | 2 -
>> include/linux/mmc/dw_mmc.h | 2 -
>> 3 files changed, 20 insertions(+), 105 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 69f0cc6..6a86495 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -34,7 +34,6 @@
>> #include <linux/mmc/dw_mmc.h>
>> #include <linux/bitops.h>
>> #include <linux/regulator/consumer.h>
>> -#include <linux/workqueue.h>
>> #include <linux/of.h>
>> #include <linux/of_gpio.h>
>> #include <linux/mmc/slot-gpio.h>
>> @@ -1954,6 +1953,23 @@ static void dw_mci_cmd_interrupt(struct dw_mci *host,
>> u32 status)
>> tasklet_schedule(&host->tasklet);
>> }
>>
>> +static void dw_mci_handle_cd(struct dw_mci *host)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < host->num_slots; i++) {
>> + struct dw_mci_slot *slot = host->slot[i];
>> +
>> + if (!slot)
>> + continue;
>> +
>> + if (slot->mmc->ops->card_event)
>> + slot->mmc->ops->card_event(slot->mmc);
>> + mmc_detect_change(slot->mmc,
>> + msecs_to_jiffies(host->pdata->detect_delay_ms));
>> + }
>> +}
>> +
>> static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>> {
>> struct dw_mci *host = dev_id;
>> @@ -2029,7 +2045,7 @@ static irqreturn_t dw_mci_interrupt(int irq, void
>> *dev_id)
>>
>> if (pending & SDMMC_INT_CD) {
>> mci_writel(host, RINTSTS, SDMMC_INT_CD);
>> - queue_work(host->card_workqueue, &host->card_work);
>> + dw_mci_handle_cd(host);
>> }
>>
>> /* Handle SDIO Interrupts */
>> @@ -2056,88 +2072,6 @@ static irqreturn_t dw_mci_interrupt(int irq, void
>> *dev_id)
>> return IRQ_HANDLED;
>> }
>>
>> -static void dw_mci_work_routine_card(struct work_struct *work)
>> -{
>> - struct dw_mci *host = container_of(work, struct dw_mci, card_work);
>> - int i;
>> -
>> - for (i = 0; i < host->num_slots; i++) {
>> - struct dw_mci_slot *slot = host->slot[i];
>> - struct mmc_host *mmc = slot->mmc;
>> - struct mmc_request *mrq;
>> - int present;
>> -
>> - present = dw_mci_get_cd(mmc);
>> - while (present != slot->last_detect_state) {
>> - dev_dbg(&slot->mmc->class_dev, "card %s\n",
>> - present ? "inserted" : "removed");
>> -
>> - spin_lock_bh(&host->lock);
>> -
>> - /* Card change detected */
>> - slot->last_detect_state = present;
>> -
>> - /* Clean up queue if present */
>> - mrq = slot->mrq;
>> - if (mrq) {
>> - if (mrq == host->mrq) {
>> - host->data = NULL;
>> - host->cmd = NULL;
>> -
>> - switch (host->state) {
>> - case STATE_IDLE:
>> - case STATE_WAITING_CMD11_DONE:
>> - break;
>> - case STATE_SENDING_CMD11:
>> - case STATE_SENDING_CMD:
>> - mrq->cmd->error = -ENOMEDIUM;
>> - if (!mrq->data)
>> - break;
>> - /* fall through */
>> - case STATE_SENDING_DATA:
>> - mrq->data->error = -ENOMEDIUM;
>> - dw_mci_stop_dma(host);
>> - break;
>> - case STATE_DATA_BUSY:
>> - case STATE_DATA_ERROR:
>> - if (mrq->data->error ==
>> -EINPROGRESS)
>> - mrq->data->error =
>> -ENOMEDIUM;
>> - /* fall through */
>> - case STATE_SENDING_STOP:
>> - if (mrq->stop)
>> - mrq->stop->error =
>> -ENOMEDIUM;
>> - break;
>> - }
>> -
>> - dw_mci_request_end(host, mrq);
>> - } else {
>> - list_del(&slot->queue_node);
>> - mrq->cmd->error = -ENOMEDIUM;
>> - if (mrq->data)
>> - mrq->data->error = -ENOMEDIUM;
>> - if (mrq->stop)
>> - mrq->stop->error = -ENOMEDIUM;
>> -
>> - spin_unlock(&host->lock);
>> - mmc_request_done(slot->mmc, mrq);
>> - spin_lock(&host->lock);
>> - }
>> - }
>> -
>> - /* Power down slot */
>> - if (present == 0)
>> - dw_mci_reset(host);
>> -
>> - spin_unlock_bh(&host->lock);
>> -
>> - present = dw_mci_get_cd(mmc);
>> - }
>> -
>> - mmc_detect_change(slot->mmc,
>> - msecs_to_jiffies(host->pdata->detect_delay_ms));
>> - }
>> -}
>> -
>> #ifdef CONFIG_OF
>> /* given a slot id, find out the device node representing that slot */
>> static struct device_node *dw_mci_of_find_slot_node(struct device *dev, u8
>> slot)
>> @@ -2289,9 +2223,6 @@ static int dw_mci_init_slot(struct dw_mci *host,
>> unsigned int id)
>> dw_mci_init_debugfs(slot);
>> #endif
>>
>> - /* Card initially undetected */
>> - slot->last_detect_state = 0;
>> -
>> return 0;
>>
>> err_host_allocated:
>> @@ -2672,17 +2603,10 @@ int dw_mci_probe(struct dw_mci *host)
>> host->data_offset = DATA_240A_OFFSET;
>>
>> tasklet_init(&host->tasklet, dw_mci_tasklet_func, (unsigned long)host);
>> - host->card_workqueue = alloc_workqueue("dw-mci-card",
>> - WQ_MEM_RECLAIM, 1);
>> - if (!host->card_workqueue) {
>> - ret = -ENOMEM;
>> - goto err_dmaunmap;
>> - }
>> - INIT_WORK(&host->card_work, dw_mci_work_routine_card);
>> ret = devm_request_irq(host->dev, host->irq, dw_mci_interrupt,
>> host->irq_flags, "dw-mci", host);
>> if (ret)
>> - goto err_workqueue;
>> + goto err_dmaunmap;
>>
>> if (host->pdata->num_slots)
>> host->num_slots = host->pdata->num_slots;
>> @@ -2718,7 +2642,7 @@ int dw_mci_probe(struct dw_mci *host)
>> } else {
>> dev_dbg(host->dev, "attempted to initialize %d slots, "
>> "but failed on all\n", host->num_slots);
>> - goto err_workqueue;
>> + goto err_dmaunmap;
>> }
>>
>> if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO)
>> @@ -2726,9 +2650,6 @@ int dw_mci_probe(struct dw_mci *host)
>>
>> return 0;
>>
>> -err_workqueue:
>> - destroy_workqueue(host->card_workqueue);
>> -
>> err_dmaunmap:
>> if (host->use_dma && host->dma_ops->exit)
>> host->dma_ops->exit(host);
>> @@ -2762,8 +2683,6 @@ void dw_mci_remove(struct dw_mci *host)
>> mci_writel(host, CLKENA, 0);
>> mci_writel(host, CLKSRC, 0);
>>
>> - destroy_workqueue(host->card_workqueue);
>> -
>> if (host->use_dma && host->dma_ops->exit)
>> host->dma_ops->exit(host);
>>
>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>> index 01b99e8..71d4995 100644
>> --- a/drivers/mmc/host/dw_mmc.h
>> +++ b/drivers/mmc/host/dw_mmc.h
>> @@ -214,7 +214,6 @@ extern int dw_mci_resume(struct dw_mci *host);
>> * with CONFIG_MMC_CLKGATE.
>> * @flags: Random state bits associated with the slot.
>> * @id: Number of this slot.
>> - * @last_detect_state: Most recently observed card detect state.
>> */
>> struct dw_mci_slot {
>> struct mmc_host *mmc;
>> @@ -234,7 +233,6 @@ struct dw_mci_slot {
>> #define DW_MMC_CARD_PRESENT 0
>> #define DW_MMC_CARD_NEED_INIT 1
>> int id;
>> - int last_detect_state;
>> };
>>
>> struct dw_mci_tuning_data {
>> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
>> index 0013669..69d0814 100644
>> --- a/include/linux/mmc/dw_mmc.h
>> +++ b/include/linux/mmc/dw_mmc.h
>> @@ -135,7 +135,6 @@ struct dw_mci {
>> struct mmc_command stop_abort;
>> unsigned int prev_blksz;
>> unsigned char timing;
>> - struct workqueue_struct *card_workqueue;
>>
>> /* DMA interface members*/
>> int use_dma;
>> @@ -154,7 +153,6 @@ struct dw_mci {
>> u32 stop_cmdr;
>> u32 dir_status;
>> struct tasklet_struct tasklet;
>> - struct work_struct card_work;
>> unsigned long pending_events;
>> unsigned long completed_events;
>> enum dw_mci_state state;
>>
>
--
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
