RE: [PATCH v1 1/1] mmc: core: fix power class selection

["Subhash Jadavani" <subhashj@codeaurora.org>]

> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Saugata Das
> Sent: Tuesday, April 03, 2012 9:48 PM
> To: Subhash Jadavani
> Cc: linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org;
> girish.shivananjappa@linaro.org
> Subject: Re: [PATCH v1 1/1] mmc: core: fix power class selection
> 
> On 3 April 2012 21:20, Subhash Jadavani <subhashj@codeaurora.org> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Saugata Das [mailto:saugata.das@linaro.org]
> >> Sent: Tuesday, April 03, 2012 8:55 PM
> >> To: Subhash Jadavani
> >> Cc: linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org;
> >> girish.shivananjappa@linaro.org
> >> Subject: Re: [PATCH v1 1/1] mmc: core: fix power class selection
> >>
> >> On 3 April 2012 12:25, Subhash Jadavani <subhashj@codeaurora.org>
> 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@codeaurora.org>
> >> > ---
> >> >  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@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