On 12/15/2011 03:56 PM, Huang Changming-R66093 wrote: > > >> -----Original Message----- >> From: Philip Rakity [mailto:prakity@xxxxxxxxxxx] >> Sent: Thursday, December 15, 2011 2:42 PM >> To: Huang Changming-R66093 >> Cc: linux-mmc@xxxxxxxxxxxxxxx; Chris Ball >> Subject: Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect >> the card >> >> >> On Dec 14, 2011, at 6:32 PM, Huang Changming-R66093 wrote: >> >>> >>> >>>> -----Original Message----- >>>> From: Philip Rakity [mailto:prakity@xxxxxxxxxxx] >>>> Sent: Wednesday, December 14, 2011 12:52 PM >>>> To: Huang Changming-R66093 >>>> Cc: linux-mmc@xxxxxxxxxxxxxxx; Huang Changming-R66093; Chris Ball >>>> Subject: Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to >>>> detect the card >>>> >>>> >>>> On Dec 13, 2011, at 6:18 PM, <r66093@xxxxxxxxxxxxx> wrote: >>>> >>>>> From: Jerry Huang <Chang-Ming.Huang@xxxxxxxxxxxxx> >>>>> >>>>> Add callback function sdhci_get_cd to detect the card. >>>>> And one new callback added to implement the card detect in sdhci >>>> struncture. >>>>> If special platform has the card detect callback, it will return the >>>>> card state, the value zero is for absent cardi and one is for >>>>> present >>>> card. >>>>> If the controller don't support card detect, sdhci_get_cd will >>>>> return - >>>> ENOSYS. >>>>> >>>>> Signed-off-by: Jerry Huang <Chang-Ming.Huang@xxxxxxxxxxxxx> >>>>> CC: Chris Ball <cjb@xxxxxxxxxx> >>>>> --- >>>>> changes for v2: >>>>> - when controller don't support get_cd, return -ENOSYS >>>>> - add new callback for sdhci to detect the card >>>>> - add the CC >>>>> changes for v3: >>>>> - use pin_lock only when get_cd defined changes for v4: >>>>> - enalbe the controller clock in platform, instead of core changes >>>>> for v5: >>>>> - remove the copyright >>>>> >>>>> drivers/mmc/host/sdhci.c | 21 ++++++++++++++++++++++ >>>>> drivers/mmc/host/sdhci.h | 2 ++ >>>>> 2 files changed, 23 insertions(+), 0 deletions(-) >>>>> >>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>>>> index >>>>> 6d8eea3..fbe2f46 100644 >>>>> --- a/drivers/mmc/host/sdhci.c >>>>> +++ b/drivers/mmc/host/sdhci.c >>>>> @@ -1518,6 +1519,26 @@ static int sdhci_get_ro(struct mmc_host *mmc) >>>>> return ret; >>>>> } >>>>> >>>>> +/* Return values for the sdjco_get_cd callback: >>>>> + * 0 for a absent card >>>>> + * 1 for a present card >>>>> + * -ENOSYS when not supported (equal to NULL callback) >>>>> + */ >>>>> +static int sdhci_get_cd(struct mmc_host *mmc) { >>>>> + struct sdhci_host *host = mmc_priv(mmc); >>>>> + unsigned long flags; >>>>> + int present = -ENOSYS; >>>>> + >>>>> + if (host->ops->get_cd) { >>>>> + spin_lock_irqsave(&host->lock, flags); >>>>> + present = host->ops->get_cd(host); >>>>> + spin_unlock_irqrestore(&host->lock, flags); >>>>> + } >>>>> + >>>>> + return present; >>>>> +} >>>> >>>> In static void sdhci_request(struct mmc_host *mmc, struct >>>> mmc_request >>>> *mrq) >>>> >>>> there is code below to handle broken card detect. >>>> >>>> 1257 >>>> 1258 /* If polling, assume that the card is always present. */ >>>> 1259 if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) >>>> 1260 present = true; >>>> 1261 else >>>> 1262 present = sdhci_readl(host, SDHCI_PRESENT_STATE) >> & >>>> 1263 SDHCI_CARD_PRESENT; >>>> >>>> The problem with this code is that it assumes broken card detect is >>>> broken since the present state register cannot be read. >>>> >>>> would it make more sense to do something like >>>> if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) { >>>> if (host->ops->get_cd) >>>> present = present = host->ops->get_cd(host); >>>> else >>>> present = true; >>>> >>>> and adjust the code in host->ops->get_cd() to not -ENOSYS. >>> >>> I think we should not detect the card present state in sdhci_request >>> function, Because if we do it, this detection will be performed with >>> any command to card, which will down the performance. >>> >>> And if the quirks has SDHCI_QUIRK_BROKEN_CARD_DETECTION, that means >>> the card is always present, We don't need to detect the card state. I think SDHCI_QUIRK_BROKEN_CARD_DETECTION is just _assume_ "the card is present." >> >> currently WE do read the present state register on every request. All >> the above suggestion does is try to use your code >> >> maybe we want something like >>>> if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) >>>> present = true; >>>> else if (host->ops->get_cd) >>>> present = present = host->ops->get_cd(host); >>>> else >>>> present = sdhci_readl(host, SDHCI_PRESENT_STATE) & >>>> SDHCI_CARD_PRESENT; >> > I am very confused, why do we read the present state register on every request? How long time read the present state register? > My codes are added to the function mmc_sd_detect in file core/sd.c > Function mmc_rescan has detect the card present state repeatedly, so I think we don't need to detect the card state on every request. > So I think the codes to detect the card present state in sdhci_request should be removed to improve the performance. How did you get the performance benefit? And mmc_rescan is repeatedly for SD-card? If mmc_rescan is repeatedly, that reason is the below code. (just my thinking) if (host->caps & MMC_CAP_NEEDS_POLL) mmc_schedule_delayed_work(&host->detect, HZ); what is your point related with mmc_rescan? I didn't understand yours.. Thanks, Jaehoon Chung > > > -- > 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 > -- 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
