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