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

["Subhash Jadavani" <subhashj@codeaurora.org>]

> -----Original Message-----
> From: Saugata Das [mailto:saugata.das@linaro.org]
> Sent: Monday, April 02, 2012 1:20 PM
> To: Subhash Jadavani
> Cc: Girish K S; linux-mmc@vger.kernel.org; patches@linaro.org; linux-
> samsung-soc@vger.kernel.org; Chris Ball
> Subject: Re: [PATCH V2] mmc: core: Add host capability check for power
class
> 
> On 28 March 2012 16:39, Subhash Jadavani <subhashj@codeaurora.org>
> wrote:
> >
> >
> >> -----Original Message-----
> >> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> >> owner@vger.kernel.org] On Behalf Of Saugata Das
> >> Sent: Thursday, December 15, 2011 6:35 PM
> >> To: Girish K S
> >> Cc: linux-mmc@vger.kernel.org; patches@linaro.org; linux-samsung-
> >> soc@vger.kernel.org; subhashj@codeaurora.org; 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@linaro.org>
> >> wrote:
> >> > On 15 December 2011 15:34, Saugata Das <saugata.das@linaro.org>
> wrote:
> >> >> On 15 December 2011 09:28, Girish K S
> >> >> <girish.shivananjappa@linaro.org>
> >> 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@laptop.org>
> >> >>> Signed-off-by: Girish K S <girish.shivananjappa@linaro.org>
> >> >>> ---
> >> >>> 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?

> 
> 
> >
> > 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@vger.kernel.org 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@vger.kernel.org More majordomo info
> at
> >> http://vger.kernel.org/majordomo-info.html
> >