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 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

>
> 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