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