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 2 April 2012 16:24, Subhash Jadavani <subhashj@xxxxxxxxxxxxxx> wrote:
>
>
>> -----Original Message-----
>> From: Saugata Das [mailto:saugata.das@xxxxxxxxxx]
>> Sent: Monday, April 02, 2012 1:20 PM
>> To: Subhash Jadavani
>> Cc: Girish K S; linux-mmc@xxxxxxxxxxxxxxx; patches@xxxxxxxxxx; linux-
>> samsung-soc@xxxxxxxxxxxxxxx; Chris Ball
>> Subject: Re: [PATCH V2] mmc: core: Add host capability check for power
> class
>>
>> 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.
>>
>> Please refer to section 6.6.5 of MMC-4.5. It says that the valid values
> are
>> defined in the PWR_CL_ff_vvv and PWR_CL_DDR_ff_vvv. We are allowed to
>> set only that value, depending on the mode, to POWER_CLASS.
>> Any other value will result in SWITCH_ERROR.
>
> Thanks for the details. Just curious, do you have any cards where you have
> seen such SWITCH_ERROR while setting the power class?
>

Typically, the host design takes care of the maximum power budget
needed for the highest speed modes. So, I have not encountered such
issues so far.


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