On 12/15/2011 03:56 PM, Huang Changming-R66093 wrote:
>
>
>> -----Original Message-----
>> From: Philip Rakity [mailto:[email protected]]
>> Sent: Thursday, December 15, 2011 2:42 PM
>> To: Huang Changming-R66093
>> Cc: [email protected]; 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:[email protected]]
>>>> Sent: Wednesday, December 14, 2011 12:52 PM
>>>> To: Huang Changming-R66093
>>>> Cc: [email protected]; 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, <[email protected]> wrote:
>>>>
>>>>> From: Jerry Huang <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> CC: Chris Ball <[email protected]>
>>>>> ---
>>>>> 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 [email protected]
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
