MIME-Version: 1.0
References: <20221213090047.3805-1-victor.shih@genesyslogic.com.tw>
 <20221213090047.3805-7-victor.shih@genesyslogic.com.tw> <CAPDyKFoV3Ch-xzXxiT2RnDeLvsO454Pwq1vQL_bdNLptM+amAg@mail.gmail.com>
 <CAK00qKDKA1h2O92HHDKhhWY3rwmGGHXQfTpuajs2RcobAaFzqA@mail.gmail.com>
In-Reply-To: <CAK00qKDKA1h2O92HHDKhhWY3rwmGGHXQfTpuajs2RcobAaFzqA@mail.gmail.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Mon, 27 Feb 2023 16:19:54 +0100
Message-ID: <CAPDyKFquNQ4GpanTDpbgheYVL9N9GJCa60p0iqN6R7ydjYy3fw@mail.gmail.com>
Subject: Re: [PATCH V6 06/24] mmc: core: Support UHS-II card control and access
To:     Victor Shih <victorshihgli@gmail.com>
Cc:     adrian.hunter@intel.com, linux-mmc@vger.kernel.org,
        linux-kernel@vger.kernel.org, benchuanggli@gmail.com,
        HL.Liu@genesyslogic.com.tw, Greg.tu@genesyslogic.com.tw,
        takahiro.akashi@linaro.org, dlunev@chromium.org,
        Victor Shih <victor.shih@genesyslogic.com.tw>,
        Jason Lai <jason.lai@genesyslogic.com.tw>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

[...]

> > > +}
> > > +
> > > +/*
> > > + * Mask off any voltages we don't support and select
> > > + * the lowest voltage
> > > + */
> > > +u32 sd_uhs2_select_voltage(struct mmc_host *host, u32 ocr)
> > > +{
> > > +       int bit;
> > > +       int err;
> > > +
> > > +       /*
> > > +        * Sanity check the voltages that the card claims to
> > > +        * support.
> > > +        */
> > > +       if (ocr & 0x7F) {
> > > +               dev_warn(mmc_dev(host), "card claims to support voltages below defined range\n");
> > > +               ocr &= ~0x7F;
> > > +       }
> > > +
> > > +       ocr &= host->ocr_avail;
> > > +       if (!ocr) {
> > > +               dev_warn(mmc_dev(host), "no support for card's volts\n");
> > > +               return 0;
> > > +       }
> > > +
> > > +       if (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) {
> > > +               bit = ffs(ocr) - 1;
> > > +               ocr &= 3 << bit;
> > > +               /* Power cycle */
> > > +               err = sd_uhs2_power_off(host);
> > > +               if (err)
> > > +                       return 0;
> > > +               err = sd_uhs2_reinit(host);
> >
> > As also pointed out by Adrian, this path becomes circular and doesn't
> > work as expected.
> >
> > At this point, I would prefer to keep things as simple as possible, so
> > I suggest that we drop this path entirely for UHS2.
> >
>
> Can we use sd_uhs2_power and sd_uhs2_power_up instead of the
> sd_uhs2_reinit after I reference the mmc_power_cycle function?

Sorry, but I don't quite understand how that would help to make this work!?

Anyway, please have a look at the corresponding comment below too. The
point is, I think it's better if we can update (or refactor) and make
use of mmc_select_voltage() instead.

>
> > > +               if (err)
> > > +                       return 0;
> > > +       } else {
> > > +               bit = fls(ocr) - 1;
> > > +               ocr &= 3 << bit;
> >
> > Note that, in mmc_select_voltage() the corresponding code has been
> > updated in commit 39a72dbfe188 ("mmc: core: properly select voltage
> > range without power cycle") that fixes a real problem for us.
> >
> > By taking the above observations into consideration, it looks like it
> > may be better to reuse mmc_select_voltage() for UHS-II after all. To
> > prevent the power cycle path for UHS-II, we can just add a check for
> > the ios->timings in there. That should work, I think.
> >
> > > +               if (bit != host->ios.vdd)
> > > +                       dev_warn(mmc_dev(host), "exceeding card's volts\n");
> > > +       }
> > > +
> > > +       return ocr;
> > > +}
> > > +

[...]

Kind regards
Uffe