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