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

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

> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Girish K S
> Sent: Friday, March 30, 2012 10:29 AM
> To: Subhash Jadavani
> Cc: Saugata Das; 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 29 March 2012 11:17, Girish K S <girish.shivananjappa@linaro.org>
wrote:
> > 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.
> >>
> >> 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"

No issues. I will do that.

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