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