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
