On 3 April 2012 22:14, Subhash Jadavani <subhashj@xxxxxxxxxxxxxx> wrote: > > >> -----Original Message----- >> From: linux-mmc-owner@xxxxxxxxxxxxxxx [mailto:linux-mmc- >> owner@xxxxxxxxxxxxxxx] On Behalf Of Saugata Das >> Sent: Tuesday, April 03, 2012 9:48 PM >> To: Subhash Jadavani >> Cc: linux-mmc@xxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; >> girish.shivananjappa@xxxxxxxxxx >> Subject: Re: [PATCH v1 1/1] mmc: core: fix power class selection >> >> On 3 April 2012 21:20, Subhash Jadavani <subhashj@xxxxxxxxxxxxxx> wrote: >> > >> > >> >> -----Original Message----- >> >> From: Saugata Das [mailto:saugata.das@xxxxxxxxxx] >> >> Sent: Tuesday, April 03, 2012 8:55 PM >> >> To: Subhash Jadavani >> >> Cc: linux-mmc@xxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; >> >> girish.shivananjappa@xxxxxxxxxx >> >> Subject: Re: [PATCH v1 1/1] mmc: core: fix power class selection >> >> >> >> On 3 April 2012 12:25, Subhash Jadavani <subhashj@xxxxxxxxxxxxxx> >> wrote: >> >> > mmc_select_powerclass() function returns error if eMMC VDD level >> >> > supported by host is between 2.7v to 3.2v. >> >> > >> >> > According to eMMC specification, valid voltage for high voltage >> >> > cards is 2.7v to 3.6v. This patch ensures that 2.7v to 3.6v VDD >> >> > range is treated as valid range. >> >> > >> >> > Also, failure to set the power class shouldn't be treated as fatal >> >> > error because even if setting the power class fails, card can still >> >> > work in default power class. >> >> > If mmc_select_powerclass() returns error, just print the warning >> >> > message and go ahead with rest of the card initialization. >> >> > >> >> >> >> Have you checked why mmc_select_powerclass returned error ? Today, >> in >> >> the mmc_select_powerclass, it is just setting the value of power >> >> class, >> > which >> >> the eMMC expects. So, it should not fail. >> >> >> >> Another point is that, today mmc_select_powerclass is assuming that >> >> host can support the maximum possible power classes and it does not >> >> check host controllers capability in driving higher current (mA). But >> >> I think in >> > future we >> >> need to add this check for host controller ability and return error >> >> from mmc_select_powerclass so that mmc_init_card can choose the next >> >> speed >> > >> > Agreed with this point. >> > >> >> mode. From that perspective, the approach to ignore the error return >> >> from mmc_select_powerclass looks wrong. >> > >> > My patch was just intended to fix the issue existing power class >> > implementation. As commit text says: >> > - don't consider 2.7v to 3.2v as invalid range >> > - We are already ignoring the error returned by >> > mmc_set_power_class() function for DS/HS/DDR cards. So this patch has >> > extended that to HS200 cards. >> > >> > So as far as this patch is concerned, I don't see anything wrong here. >> > >> > As you have mentioned, currently we are not taking the host controller >> > capability into account (as it doesn't exist as of now) so we should >> > not see >> > mmc_select_powerclass() fail any time. >> >> This is actually the main concern. The fail of mmc_select_powerclass is >> something to be checked and not ignored since it should not fail under the >> current implementation. > > Yes, this makes sense. With current implementation, mmc_select_powerclass() > should never really fail which means failure should be treated as fatal and > we should really skip the card initialization. > This patch is already pushed to mmc-next. So I will post another patch (by > next week as I will be on vacation in this week) to skip the card > initialization if mmc_select_powerclass fails. > > Is this ok? It is OK. Thanks, > > Regards, > Subhash > >> >> >> > But yes, next thing should be to take into account the host current >> > sourcing capability before deciding which bus speed mode to choose. >> > But that may be a big change and should be as separate patch. Girish >> > had already posted one patch for this which needs to be extended to >> achieve this. >> > >> > Regards, >> > Subhash >> > >> >> >> >> >> >> > Signed-off-by: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx> >> >> > --- >> >> > drivers/mmc/core/mmc.c | 30 +++++++++++++++++------------- >> >> > 1 files changed, 17 insertions(+), 13 deletions(-) >> >> > >> >> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index >> >> > 02914d6..54df5ad 100644 >> >> > --- a/drivers/mmc/core/mmc.c >> >> > +++ b/drivers/mmc/core/mmc.c >> >> > @@ -695,6 +695,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: >> >> > @@ -1111,11 +1116,10 @@ static int mmc_init_card(struct mmc_host >> >> > *host, u32 ocr, >> >> > ext_csd_bits = (bus_width == MMC_BUS_WIDTH_8) ? >> >> > EXT_CSD_BUS_WIDTH_8 : >> >> > EXT_CSD_BUS_WIDTH_4; >> >> > err = mmc_select_powerclass(card, ext_csd_bits, >> >> > ext_csd); >> >> > - if (err) { >> >> > - pr_err("%s: power class selection to bus >> >> > width %d failed\n", >> >> > - mmc_hostname(card->host), 1 << >> >> > bus_width); >> >> > - goto err; >> >> > - } >> >> > + if (err) >> >> > + pr_warning("%s: power class selection to >> >> > + bus >> > width %d" >> >> > + " failed\n", >> >> > + mmc_hostname(card->host), >> >> > + 1 << bus_width); >> >> > } >> >> > >> >> > /* >> >> > @@ -1147,10 +1151,10 @@ static int mmc_init_card(struct mmc_host >> >> > *host, u32 ocr, >> >> > err = mmc_select_powerclass(card, >> >> > ext_csd_bits[idx][0], >> >> > ext_csd); >> >> > if (err) >> >> > - pr_err("%s: power class selection to > " >> >> > - "bus width %d failed\n", >> >> > - mmc_hostname(card->host), >> >> > - 1 << bus_width); >> >> > + pr_warning("%s: power class >> >> > + selection to >> > " >> >> > + "bus width %d failed\n", >> >> > + >> >> > + mmc_hostname(card->host), >> >> > + 1 << bus_width); >> >> > >> >> > err = mmc_switch(card, >> >> > EXT_CSD_CMD_SET_NORMAL, >> >> > EXT_CSD_BUS_WIDTH, @@ >> >> > -1178,10 >> >> > +1182,10 @@ static int mmc_init_card(struct mmc_host *host, u32 >> >> > +ocr, >> >> > err = mmc_select_powerclass(card, >> >> > ext_csd_bits[idx][1], >> >> > ext_csd); >> >> > if (err) >> >> > - pr_err("%s: power class selection to > " >> >> > - "bus width %d ddr %d >> >> > failed\n", >> >> > - mmc_hostname(card->host), >> >> > - 1 << bus_width, ddr); >> >> > + pr_warning("%s: power class >> >> > + selection to >> > " >> >> > + "bus width %d ddr %d >> >> > + failed\n", >> >> > + >> >> > + mmc_hostname(card->host), >> >> > + 1 << bus_width, ddr); >> >> > >> >> > err = mmc_switch(card, >> >> > EXT_CSD_CMD_SET_NORMAL, >> >> > EXT_CSD_BUS_WIDTH, >> >> > -- >> >> > 1.7.1.1 >> >> > >> >> > -- >> >> > Sent by a consultant of the Qualcomm Innovation Center, Inc. >> >> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora >> >> Forum. >> >> > -- >> >> > 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
