On Mon, 10 May 2021 at 11:10, Avri Altman <Avri.Altman@xxxxxxx> 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
