All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ulf Hansson <ulf.hansson@linaro.org>
To: Avri Altman <Avri.Altman@wdc.com>
Cc: "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Wolfram Sang <wsa+renesas@sang-engineering.com>,
	Shawn Lin <shawn.lin@rock-chips.com>,
	Masami Hiramatsu <masami.hiramatsu@linaro.org>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] mmc: core: Add support for cache ctrl for SD cards
Date: Mon, 10 May 2021 16:41:43 +0200	[thread overview]
Message-ID: <CAPDyKFrvhNntCAWdyN5Kw8znq4RKE6yQS5G38XYY5iBhqMW0Pw@mail.gmail.com> (raw)
In-Reply-To: <DM6PR04MB6575E3D63982278D2F1CA022FC549@DM6PR04MB6575.namprd04.prod.outlook.com>

On Mon, 10 May 2021 at 11:10, Avri Altman <Avri.Altman@wdc.com> wrote:
>
> > +static int sd_enable_cache(struct mmc_card *card)
> > +{
> > +       u8 *reg_buf;
> > +       int err;
> > +
> > +       reg_buf = kzalloc(512, GFP_KERNEL);
> > +       if (!reg_buf)
> > +               return -ENOMEM;
> > +
> > +       /*
> > +        * Set the Cache Enable bit in the performance enhancement register at
> > +        * 260 bytes offset.
> > +        */
> > +       err = sd_write_ext_reg(card, card->ext_perf.fno, card->ext_perf.page,
> > +                              card->ext_perf.offset + 260, 0x1);
> > +       if (err) {
> > +               pr_warn("%s: error %d writing Cache Enable bit\n",
> > +                       mmc_hostname(card->host), err);
> > +               goto out;
> > +       }
> > +
> > +       err = mmc_poll_for_busy(card,
> > SD_WRITE_EXTR_SINGLE_TIMEOUT_MS, false,
> > +                               MMC_BUSY_EXTR_SINGLE);
> I think 1sec is for flush cache, but I guess it makes sense to use it here as well.

The spec talks about generic busy signaling time for CMD49 of one
second. That's why I added this here.

>
> > +       if (!err)
> > +               card->ext_perf.feature_enabled |= SD_EXT_PERF_CACHE;
> Maybe
> If (err)
>     card->ext_perf.feature_enabled &= ~SD_EXT_PERF_CACHE;
>
> and move to out: to catch the sd_write_ext_reg err ?
>
> > +
> > +out:
> > +       kfree(reg_buf);
> > +       return err;
> > +}
> > +
> >  /*
> >   * Handle the detection and initialisation of a card.
> >   *
> > @@ -1442,6 +1531,13 @@ static int mmc_sd_init_card(struct mmc_host
> > *host, u32 ocr,
> >                         goto free_card;
> >         }
> >
> > +       /* Enable internal SD cache if supported. */
> > +       if (card->ext_perf.feature_support & SD_EXT_PERF_CACHE) {
> > +               err = sd_enable_cache(card);
> > +               if (err)
> > +                       goto free_card;
> If cache enablement failed, is it worthwhile to bail out?
> Maybe disabling the cache with the appropriate message is enough?

Right, good point.

Let me also think about how we best reset the .feature_enabled field
after a power cycle. Theoretically we could fail to enable a feature
after the system has resumed, but then we would still have the
correspond bits set.

>
> > +       }
> > +
> >         if (host->cqe_ops && !host->cqe_enabled) {
> >                 err = host->cqe_ops->cqe_enable(host, card);
> >                 if (!err) {
> > @@ -1694,6 +1790,8 @@ static const struct mmc_bus_ops mmc_sd_ops = {
> >         .alive = mmc_sd_alive,
> >         .shutdown = mmc_sd_suspend,
> >         .hw_reset = mmc_sd_hw_reset,
> > +       .cache_enabled = sd_cache_enabled,
> > +       .flush_cache = sd_flush_cache,
> >  };
>
> I would expect 2 more patches in this series:
>  - flush cache on power down

According to the spec that should not be needed, because that should
be managed internally in the SD card when we send a poweroff
notification.

Did I get that wrong? Do you prefer to send a flush cache as well
before the poweroff notification?

>  - cache disablement events?

This I don't know about. Can you elaborate?

>
> Thanks,
> Avri

Kind regards
Uffe

  parent reply	other threads:[~2021-05-10 15:16 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-06 14:58 [PATCH 0/2] mmc: core: Implement " Ulf Hansson
2021-05-06 14:58 ` [PATCH 1/2] mmc: core: Move eMMC cache flushing to a new bus_ops callback Ulf Hansson
2021-05-07  7:15   ` Avri Altman
2021-05-09 18:48   ` Linus Walleij
2021-05-06 14:58 ` [PATCH 2/2] mmc: core: Add support for cache ctrl for SD cards Ulf Hansson
2021-05-09 19:01   ` Linus Walleij
2021-05-10 14:32     ` Ulf Hansson
2021-05-10  9:10   ` Avri Altman
2021-05-10 10:41     ` Avri Altman
2021-05-10 14:41     ` Ulf Hansson [this message]
2021-05-11  8:22       ` Avri Altman
2021-05-10  7:39 ` [PATCH 0/2] mmc: core: Implement " Avri Altman
2021-05-10  9:44   ` Ulf Hansson
2021-05-10 10:12     ` Avri Altman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPDyKFrvhNntCAWdyN5Kw8znq4RKE6yQS5G38XYY5iBhqMW0Pw@mail.gmail.com \
    --to=ulf.hansson@linaro.org \
    --cc=Avri.Altman@wdc.com \
    --cc=adrian.hunter@intel.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mmc@vger.kernel.org \
    --cc=masami.hiramatsu@linaro.org \
    --cc=shawn.lin@rock-chips.com \
    --cc=wsa+renesas@sang-engineering.com \
    --subject='Re: [PATCH 2/2] mmc: core: Add support for cache ctrl for SD cards' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.