Re: [PATCH V2] mmc: core: Add host capability check for power class

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

 



On 29 March 2012 11:17, Girish K S <girish.shivananjappa@xxxxxxxxxx> wrote:
> On 28 March 2012 16:39, Subhash Jadavani <subhashj@xxxxxxxxxxxxxx> wrote:
>>
>>
>>> -----Original Message-----
>>> From: linux-mmc-owner@xxxxxxxxxxxxxxx [mailto:linux-mmc-
>>> owner@xxxxxxxxxxxxxxx] On Behalf Of Saugata Das
>>> Sent: Thursday, December 15, 2011 6:35 PM
>>> To: Girish K S
>>> Cc: linux-mmc@xxxxxxxxxxxxxxx; patches@xxxxxxxxxx; linux-samsung-
>>> soc@xxxxxxxxxxxxxxx; subhashj@xxxxxxxxxxxxxx; Chris Ball
>>> Subject: Re: [PATCH V2] mmc: core: Add host capability check for power
>> class
>>>
>>> On 15 December 2011 16:22, Girish K S <girish.shivananjappa@xxxxxxxxxx>
>>> wrote:
>>> > On 15 December 2011 15:34, Saugata Das <saugata.das@xxxxxxxxxx> wrote:
>>> >> On 15 December 2011 09:28, Girish K S <girish.shivananjappa@xxxxxxxxxx>
>>> wrote:
>>> >>> This patch adds a check whether the host supports maximum current
>>> >>> value obtained from the device's extended csd register for a
>>> >>> selected interface voltage and frequency.
>>> >>>
>>> >>> cc: Chris Ball <cjb@xxxxxxxxxx>
>>> >>> Signed-off-by: Girish K S <girish.shivananjappa@xxxxxxxxxx>
>>> >>> ---
>>> >>> Changes in v2:
>>> >>>        deleted a unnecessary if else condition identified by subhash
>>> >>> J Changes in v1:
>>> >>>       reduced the number of comparisons as per Hein's suggestion
>>> >>>
>>> >>>  drivers/mmc/core/mmc.c   |   19 +++++++++++++++++++
>>> >>>  include/linux/mmc/card.h |    4 ++++
>>> >>>  2 files changed, 23 insertions(+), 0 deletions(-)
>>> >>>
>>> >>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index
>>> >>> 006e932..b9ef777 100644
>>> >>> --- a/drivers/mmc/core/mmc.c
>>> >>> +++ b/drivers/mmc/core/mmc.c
>>> >>> @@ -688,6 +688,25 @@ static int mmc_select_powerclass(struct
>>> >>> mmc_card *card,
>>> >>>                pwrclass_val = (pwrclass_val &
>>> >>> EXT_CSD_PWR_CL_4BIT_MASK) >>
>>> >>>                                EXT_CSD_PWR_CL_4BIT_SHIFT;
>>> >>>
>>> >>> +       if (pwrclass_val >= MMC_MAX_CURRENT_800)
>>> >>> +               pwrclass_val = MMC_MAX_CURRENT_800;
>>> >>> +       else if (pwrclass_val >= MMC_MAX_CURRENT_600)
>>> >>> +               pwrclass_val = MMC_MAX_CURRENT_600;
>>> >>> +       else if (pwrclass_val >= MMC_MAX_CURRENT_400)
>>> >>> +               pwrclass_val = MMC_MAX_CURRENT_400;
>>> >>> +       else
>>> >>> +               pwrclass_val = MMC_MAX_CURRENT_200;
>>> >>> +
>>> >>> +       if ((pwrclass_val == MMC_MAX_CURRENT_800) &&
>>> >>> +           !(card->host->caps & MMC_CAP_MAX_CURRENT_800))
>>> >>> +               pwrclass_val = MMC_MAX_CURRENT_600;
>>> >>> +       if ((pwrclass_val == MMC_MAX_CURRENT_600) &&
>>> >>> +           !(card->host->caps & MMC_CAP_MAX_CURRENT_600))
>>> >>> +               pwrclass_val = MMC_MAX_CURRENT_400;
>>> >>> +       if ((pwrclass_val == MMC_MAX_CURRENT_400) &&
>>> >>> +           !(card->host->caps & MMC_CAP_MAX_CURRENT_400))
>>> >>> +               pwrclass_val = MMC_MAX_CURRENT_200;
>>> >>> +
>>> >>>        /* If the power class is different from the default value */
>>> >>>        if (pwrclass_val > 0) {
>>> >>>                err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>> >>
>>> >> It is not allowed to set the POWER_CLASS with any value other than
>>> >> what is mentioned in the PWR_CL_ff_vvv or PWR_CL_DDR_ff_vvv  for
>>> the
>>> >> corresponding frequency, voltage. That is, if PWR_CL_200_195 is 14
>>> >> and we want to operate at HS200 then the only value allowed for
>>> >> POWER_CLASS is 14. So, we need to check the PWR_CL numbers and
>>> choose
>>> >> the operating mode (HS200/DDR50/..) based on the platform capability
>>> >> to support the current consumption and set the corresponding
>>> >> POWER_CLASS value.
>>> >>
>>> >> Please refer to section 6.6.5 of the 4.5 spec.
>>> >
>>> > The upstreamed code reads the extended csd value based on the already
>>> > set voltage level and frequency of host. So it will get the required
>>> > power class value which can be set directly. Is my understanding
>>> > correct?
>>> >
>>>
>>> It is not enough to just check the voltage level and frequency.
>>> Consider this example, host has capability to support
>>> MMC_CAP_MAX_CURRENT_400, the PWR_CL_DDR_52_360 has the value 9
>>> (400mA) and PWR_CL_200_360 has the value 14 (800mA). Then even though
>>> the host might be capable to run 200MHz clock and 3.6V, it can only enable
>>> DDR at 52MHz and set 9 in POWER_CLASS.
>>>
>>> I think, in mmc_select_powerclass, we need to loop through the power
>>> classes of all supported modes of transfer (HS200, DDR52, ... ) and choose
>> the
>>> mode which gives maximum bandwidth but falls within host capability of
>>> current consumption. Then set this to POWER_CLASS byte and also use the
>>> same information when setting HS_TIMING in mmc_init_card.
>>
>> Hi Saugata,
>>
>> Does the spec mandates you to set the power class to what is needed by
>> frequency/voltage combination? I can't see that mentioned anywhere
>> explicitly in eMMC4.5 spec (if it is mentioned in spec, please let me know
>> section and line number). It may be still possible to set the power class
>> lower than what is needed by frequency/voltage combination. Say for example,
>> 8-bit HS200 (200MHz) with high voltage cards may specify power class
>> (PWR_CL_200_3_6) value of 14 (800 mA) but that doesn't mean if you want the
>> card to work in 8-bit HS200 mode, its POWER_CLASS value must be 14 (800mA).
>> If host's VDD regulator is only capable of say 600mA then it may still set
>> the POWER_CLASS to 12 (600 mA) which should be the indication to card that
>> host power supply is capable of sourcing only 600mA and I would assume card
>> will make sure that it doesn't draw anything more than that.
>>
>> Hi Girish,
>>
>> I see couple of other issues with your original power_class selection patch.
>> 1. mmc_select_powerclass() function considers 2.7v to 3.2v VDD range as
>> invalid. That's not correct. Valid high voltage range is from 2.7v to 3.6v.
>> Is there a specific reason you have skipped the 2.7v to 3.2v VDD range? If
>> not, I should post following change:
>
> can post a patch for this
>
>>
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -626,6 +626,11 @@ static int mmc_select_powerclass(struct mmc_card *card,
>>                else if (host->ios.clock <= 200000000)
>>                        index = EXT_CSD_PWR_CL_200_195;
>>                break;
>> +       case MMC_VDD_27_28:
>> +       case MMC_VDD_28_29:
>> +       case MMC_VDD_29_30:
>> +       case MMC_VDD_30_31:
>> +       case MMC_VDD_31_32:
>>        case MMC_VDD_32_33:
>>        case MMC_VDD_33_34:
>>        case MMC_VDD_34_35:
>>
>> 2.  Currently in mmc_init_card(), power class selection is done if it's
>> normal (DS or HS) or DDR or HS200 card.
>>    If power class selection fails (mmc_select_powerclass() returns error)
>> for DS/HS/DDR cards, we are just printing the error and going ahead with the
>> rest of the initialization where as if power class selection fails for HS200
>> cards, we are simply aborting the
>>    initialization and mark it as failed.
>>
>>   There are my points:
>>       1.  Power class failure must be treated in same manner for
>> DS/HS/DDR/HS200 cards
> good find.
> Yes will modify and repost for the HS200 case.
>
>>       2.  If you agree on first point then we need to decide whether power
>> class selection failure is fatal enough to abort the MMC initialization? or
>> we can just print the warning and go ahead with rest of the initialization
>> in same bus speed mode?
>
> Should not be treated as fatal, should continue the initialization
> with default power class
Instead of making 2 different patches, can you add this modification
in your above patch. "you need to remove only the return err;
statement"
>
>>
>> Regards,
>> Subhash
>>
>>>
>>> >>
>>> >>
>>> >>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>>> >>> index 9478a6b..c5e031a 100644
>>> >>> --- a/include/linux/mmc/card.h
>>> >>> +++ b/include/linux/mmc/card.h
>>> >>> @@ -195,6 +195,10 @@ struct mmc_part {
>>> >>>  #define MMC_BLK_DATA_AREA_GP   (1<<2)
>>> >>>  };
>>> >>>
>>> >>> +#define MMC_MAX_CURRENT_200    (0)
>>> >>> +#define MMC_MAX_CURRENT_400    (7)
>>> >>> +#define MMC_MAX_CURRENT_600    (11) #define
>>> MMC_MAX_CURRENT_800
>>> >>> +(13)
>>> >>>  /*
>>> >>>  * MMC device
>>> >>>  */
>>> >>> --
>>> >>> 1.7.1
>>> >>>
>>> >>> --
>>> >>> 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
>>
--
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