On 2 April 2012 16:24, Subhash Jadavani <[email protected]> wrote:
>
>
>> -----Original Message-----
>> From: Saugata Das [mailto:[email protected]]
>> Sent: Monday, April 02, 2012 1:20 PM
>> To: Subhash Jadavani
>> Cc: Girish K S; [email protected]; [email protected]; linux-
>> [email protected]; Chris Ball
>> Subject: Re: [PATCH V2] mmc: core: Add host capability check for power
> class
>>
>> On 28 March 2012 16:39, Subhash Jadavani <[email protected]>
>> wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: [email protected] [mailto:linux-mmc-
>> >> [email protected]] On Behalf Of Saugata Das
>> >> Sent: Thursday, December 15, 2011 6:35 PM
>> >> To: Girish K S
>> >> Cc: [email protected]; [email protected]; linux-samsung-
>> >> [email protected]; [email protected]; Chris Ball
>> >> Subject: Re: [PATCH V2] mmc: core: Add host capability check for
>> >> power
>> > class
>> >>
>> >> On 15 December 2011 16:22, Girish K S
>> >> <[email protected]>
>> >> wrote:
>> >> > On 15 December 2011 15:34, Saugata Das <[email protected]>
>> wrote:
>> >> >> On 15 December 2011 09:28, Girish K S
>> >> >> <[email protected]>
>> >> 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 <[email protected]>
>> >> >>> Signed-off-by: Girish K S <[email protected]>
>> >> >>> ---
>> >> >>> 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 [email protected] 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 [email protected] 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
