| Message ID | 000401cd0cd3$4a6be470$df43ad50$@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
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 > > 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 >
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" > >> >> 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 >>
> -----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
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. 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. > > 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@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 >
> -----Original Message----- > From: Saugata Das [mailto:saugata.das@linaro.org] > Sent: Monday, April 02, 2012 1:20 PM > To: Subhash Jadavani > Cc: Girish K S; 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 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. > > 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@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 > >
On 2 April 2012 16:24, Subhash Jadavani <subhashj@codeaurora.org> wrote: > > >> -----Original Message----- >> From: Saugata Das [mailto:saugata.das@linaro.org] >> Sent: Monday, April 02, 2012 1:20 PM >> To: Subhash Jadavani >> Cc: Girish K S; 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 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. >> >> 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@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 >> > >
--- 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: