All of lore.kernel.org
 help / color / mirror / Atom feed
From: Linus Walleij <linus.walleij@linaro.org>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: linux-mmc <linux-mmc@vger.kernel.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Wolfram Sang <wsa+renesas@sang-engineering.com>,
	Shawn Lin <shawn.lin@rock-chips.com>,
	Avri Altman <avri.altman@wdc.com>,
	Masami Hiramatsu <masami.hiramatsu@linaro.org>,
	linux-block <linux-block@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] mmc: core: Add support for cache ctrl for SD cards
Date: Sun, 9 May 2021 21:01:27 +0200	[thread overview]
Message-ID: <CACRpkdZ2JRapr5x2zn3ABwiGJw-8kbfG4K2oZVD0Lfr7KHG8Jw@mail.gmail.com> (raw)
In-Reply-To: <20210506145829.198823-3-ulf.hansson@linaro.org>

On Thu, May 6, 2021 at 4:58 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:

> In SD spec v6.x the SD function extension registers for performance
> enhancements were introduced. As a part of this an optional internal cache
> on the SD card, can be used to improve performance.
>
> The let the SD card use the cache, the host needs to enable it and manage
> flushing of the cache, so let's add support for this.
>
> Note that for an SD card supporting the cache it's mandatory for it, to
> also support the poweroff notification feature. According to the SD spec,
> if the cache has been enabled and a poweroff notification is sent to the
> card, that implicitly also means that the card should flush its internal
> cache. Therefore, dealing with cache flushing for REQ_OP_FLUSH block
> requests is sufficient.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
(...)

> +       /*
> +        * Set the Flush Cache bit in the performance enhancement register at
> +        * 261 bytes offset.
> +        */
> +       fno = card->ext_perf.fno;
> +       page = card->ext_perf.page;
> +       offset = card->ext_perf.offset + 261;

261 looks a bit magic, can we add a define of some sort?
I guess it has a name in the spec?

> +       err = sd_write_ext_reg(card, fno, page, offset, 0x1);
> +       if (err) {
> +               pr_warn("%s: error %d writing Cache Flush bit\n",
> +                       mmc_hostname(host), err);
> +               goto out;
> +       }

So this offset contains a single bit.

> +       if (reg_buf[0] & 0x1)
> +               err = -ETIMEDOUT;

And that same bit is checked here.

Is it always going to be one bit only or do we want to

#include <linux/bits.h>
#define SD_CACHE_FLUSH_FLAG BIT(0)

Does it have a name in the spec we can use?

> +       /*
> +        * 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);

Same here we want to #define 260 to something symbolic,

And here some define for BIT(0) as well. At least with BIT(0)
in the call to sd_write_ext_reg() rather than 0x1 if I can say
something.

With the above nitpicking fixed up (I trust you):
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

  reply	other threads:[~2021-05-09 19:02 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 [this message]
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
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=CACRpkdZ2JRapr5x2zn3ABwiGJw-8kbfG4K2oZVD0Lfr7KHG8Jw@mail.gmail.com \
    --to=linus.walleij@linaro.org \
    --cc=adrian.hunter@intel.com \
    --cc=avri.altman@wdc.com \
    --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=ulf.hansson@linaro.org \
    --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.