Re: [PATCH 2/2] mmc: core: Add support for cache ctrl for SD cards

[Ulf Hansson]

On Sun, 9 May 2021 at 21:01, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> On Thu, May 6, 2021 at 4:58 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> 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@xxxxxxxxxx>
> (...)
>
> > +       /*
> > +        * 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?

We could, but I am not sure it really improves things. At least it
would not be consistent with the way we treat other magic numbers.

I think it's better to look into this as wider cleanup instead.

> I guess it has a name in the spec?

It's called the "Power Management Setting Register".

>
> > +       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.

Correct.

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

Well, it just says "Cache Flush" bit.

It seems to be one bit always for these features. The remaining bits
in the same byte are unused/reserved.

Each feature has at least one dedicated byte, so there are no bytes
being shared between features.

>
> > +       /*
> > +        * 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.

The conversion to BIT(0) in the argument is clearly an improvement. I
do that change when applying, but leave the defines for the other
magics to be considered as a future cleanup.

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

Thanks!

Kind regards
Uffe