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