mmc: core: Add support for the eMMC RTC feature in mmc_ops

This patch adds support to set the RTC information in
the eMMC device. This is based on the JEDEC specification.

There is no way however, to read the RTC time from the
device. Hence we rely on the response of the CMD49 to
confirm the completion of the operation.

This patch has been tested successfully with the ioctl
interface. This patch has also been tested suceessfully
with all the three RTC_INFO_TYPEs.

Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
---
 drivers/mmc/core/mmc_ops.c | 59 ++++++++++++++++++++++++++++++++++++++
 drivers/mmc/core/mmc_ops.h |  2 ++
 include/linux/mmc/mmc.h    |  1 +
 3 files changed, 62 insertions(+)

On Mon, Dec 06, 2021 at 12:44:14PM -0800, Stephen Boyd wrote:
> Quoting Nishad Kamdar (2021-12-05 11:10:08)
> > diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> > index d63d1c735335..490372341b3b 100644
> > --- a/drivers/mmc/core/mmc_ops.c
> > +++ b/drivers/mmc/core/mmc_ops.c
> > @@ -1063,3 +1063,62 @@ int mmc_sanitize(struct mmc_card *card, unsigned int timeout_ms)
> >         return err;
> >  }
> >  EXPORT_SYMBOL_GPL(mmc_sanitize);
> > +
> > +int mmc_set_time(struct mmc_card *card, struct mmc_host *host,
> > +                u8 rtc_info_type, u64 seconds)
> > +{
> > +       struct mmc_request mrq = {};
> > +       struct mmc_command cmd = {};
> > +       struct mmc_data data = {};
> > +       struct scatterlist sg;
> > +       int err = 0;
> > +       u8 *data_buf;
> > +
> > +       data_buf = kzalloc(512, GFP_KERNEL);
> 
> Use some #define for 512 because it's used three times in here?
ok, but there is not #define for 512 as it is the variable block size
value. Hence, other functions in the same file like mmc_get_ext_csd() also use
the 512 value directly.

> > +       if (!data_buf)
> > +               return -ENOMEM;
> > +
> > +       if (rtc_info_type == 0x01 || rtc_info_type == 0x02 ||
> > +           rtc_info_type == 0x03) {
> > +               data_buf[0] = 0x01;
> > +               data_buf[1] = rtc_info_type;
> > +               memcpy(&data_buf[2], &seconds, sizeof(u64));
> 
> Use sizeof(seconds) instead?
> 
ok, I will do that.

> > +       } else {
> > +               pr_err("%s: invalid rtc_info_type %d\n",
> > +                      mmc_hostname(host), rtc_info_type);
> > +               kfree(data_buf);
> > +               return -EINVAL;
> > +       }
> > +
> > +       mrq.cmd = &cmd;
> > +       mrq.data = &data;
> > +
> > +       cmd.opcode = MMC_SET_TIME;
> > +       cmd.arg = 0;
> > +       cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> > +
> > +       data.blksz = 512;
> > +       data.blocks = 1;
> > +       data.flags = MMC_DATA_WRITE;
> > +       data.sg = &sg;
> > +       data.sg_len = 1;
> > +       sg_init_one(&sg, data_buf, 512);
> > +
> > +       mmc_set_data_timeout(&data, card);
> > +
> > +       mmc_wait_for_req(host, &mrq);
> > +
> > +       if (cmd.error) {
> > +               err = cmd.error;
> > +               goto out;
> > +       }
> > +
> > +       if (data.error) {
> > +               err = data.error;
> > +               goto out;
> > +       }
> 
> Why not
> 
> 	if (cmd.error) {
> 		err = cmd.error;
> 	} else if (data.error) {
> 		err = data.error;
> 	} else {
> 		err = 0;
> 	}
> 
> > +out:
> 
> And then drop out: and the assignment of err to 0 up above?
ok, I will do that.

> 
> > +       kfree(data_buf);
> > +       return err;
> > +}
> > +EXPORT_SYMBOL_GPL(mmc_set_time);

> On Tue, Dec 07, 2021 at 08:28:42AM +0000, Avri Altman wrote:
> >
> > > This patch adds support to set the RTC information in the eMMC
> > > device. This is based on the JEDEC specification.
> > >
> > > There is no way however, to read the RTC time from the device. Hence
> > > we rely on the response of the CMD49 to confirm the completion of
> > > the operation.
> > >
> > > This patch has been tested successfully with the ioctl interface.
> > > This patch has also been tested suceessfully with all the three
> > > RTC_INFO_TYPEs.
> > If this is triggered from user-space via ioctl anyway, Why do we need
> > this command to be implemented in the kernel?
> > Why not just add this to mmc-utils?
> >
> > Thanks,
> > Avri
> As per the spec, B51: Section 6.6.35:
> Providing RTC info may be useful for internal maintainance operations.
> And the host should send it on the following events:
> - power-up
> - wake-up
> - Periodically
> Hence IMO, the Kernel would be the right place of peforming this operation.
But your patch doesn't do that, is it?

Thanks,
Avri

On Tue, Dec 07, 2021 at 09:33:46AM +0000, Avri Altman wrote:
> > On Tue, Dec 07, 2021 at 08:28:42AM +0000, Avri Altman wrote:
> > >
> > > > This patch adds support to set the RTC information in the eMMC
> > > > device. This is based on the JEDEC specification.
> > > >
> > > > There is no way however, to read the RTC time from the device. Hence
> > > > we rely on the response of the CMD49 to confirm the completion of
> > > > the operation.
> > > >
> > > > This patch has been tested successfully with the ioctl interface.
> > > > This patch has also been tested suceessfully with all the three
> > > > RTC_INFO_TYPEs.
> > > If this is triggered from user-space via ioctl anyway, Why do we need
> > > this command to be implemented in the kernel?
> > > Why not just add this to mmc-utils?
> > >
> > > Thanks,
> > > Avri
> > As per the spec, B51: Section 6.6.35:
> > Providing RTC info may be useful for internal maintainance operations.
> > And the host should send it on the following events:
> > - power-up
> > - wake-up
> > - Periodically
> > Hence IMO, the Kernel would be the right place of peforming this operation.
> But your patch doesn't do that, is it?
>
Yes, That's because this operation may be device specific. In order to know when
to call this function may require eMMC firmware info.
This patch only adds support so that if the info is made available
in the future, a separate patch can be added to introduce the calling mechanism.

Thanks and Regards,
Nishad

> Thanks,
> Avri