* Re: [PATCH] mmc: core: Add support for the eMMC RTC feature in mmc_ops
2021-12-05 19:10 [PATCH] mmc: core: Add support for the eMMC RTC feature in mmc_ops Nishad Kamdar
@ 2021-12-06 20:44 ` Stephen Boyd
2021-12-07 5:26 ` Nishad Kamdar
2021-12-07 8:28 ` Avri Altman
2021-12-07 8:52 ` Christian Löhle
2 siblings, 1 reply; 9+ messages in thread
From: Stephen Boyd @ 2021-12-06 20:44 UTC (permalink / raw)
To: Adrian Hunter, Avri Altman, Bean Huo, Huijin Park, Jens Axboe,
Nishad Kamdar, Shawn Lin, Ulf Hansson, Wolfram Sang, Yue Hu
Cc: Greg Kroah-Hartman, linux-mmc, linux-kernel
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?
> + 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?
> + } 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?
> + kfree(data_buf);
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(mmc_set_time);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mmc: core: Add support for the eMMC RTC feature in mmc_ops
2021-12-06 20:44 ` Stephen Boyd
@ 2021-12-07 5:26 ` Nishad Kamdar
0 siblings, 0 replies; 9+ messages in thread
From: Nishad Kamdar @ 2021-12-07 5:26 UTC (permalink / raw)
To: Stephen Boyd, Ulf Hansson, Jens Axboe, Adrian Hunter, Bean Huo,
Shawn Lin, Avri Altman, Huijin Park, Yue Hu, Wolfram Sang
Cc: Greg Kroah-Hartman, linux-mmc, linux-kernel
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);
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] mmc: core: Add support for the eMMC RTC feature in mmc_ops
2021-12-05 19:10 [PATCH] mmc: core: Add support for the eMMC RTC feature in mmc_ops Nishad Kamdar
2021-12-06 20:44 ` Stephen Boyd
@ 2021-12-07 8:28 ` Avri Altman
2021-12-07 9:30 ` Nishad Kamdar
2021-12-07 8:52 ` Christian Löhle
2 siblings, 1 reply; 9+ messages in thread
From: Avri Altman @ 2021-12-07 8:28 UTC (permalink / raw)
To: Nishad Kamdar, Ulf Hansson, Jens Axboe, Adrian Hunter, Bean Huo,
Shawn Lin, Stephen Boyd, Huijin Park, Yue Hu, Wolfram Sang
Cc: Greg Kroah-Hartman, linux-mmc, linux-kernel
> 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
>
> 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(+)
>
> 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);
> + 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));
> + } 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;
> + }
> +out:
> + kfree(data_buf);
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(mmc_set_time);
> diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
> index 9c813b851d0b..0c8695d1b363 100644
> --- a/drivers/mmc/core/mmc_ops.h
> +++ b/drivers/mmc/core/mmc_ops.h
> @@ -55,6 +55,8 @@ void mmc_run_bkops(struct mmc_card *card);
> int mmc_cmdq_enable(struct mmc_card *card);
> int mmc_cmdq_disable(struct mmc_card *card);
> int mmc_sanitize(struct mmc_card *card, unsigned int timeout_ms);
> +int mmc_set_time(struct mmc_card *card, struct mmc_host *host,
> + u8 rtc_info_type, u64 seconds);
>
> #endif
>
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> index d9a65c6a8816..52a3bf873d50 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -64,6 +64,7 @@
> #define MMC_WRITE_MULTIPLE_BLOCK 25 /* adtc R1 */
> #define MMC_PROGRAM_CID 26 /* adtc R1 */
> #define MMC_PROGRAM_CSD 27 /* adtc R1 */
> +#define MMC_SET_TIME 49 /* adtc R1 */
>
> /* class 6 */
> #define MMC_SET_WRITE_PROT 28 /* ac [31:0] data addr R1b */
> --
> 2.17.1
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mmc: core: Add support for the eMMC RTC feature in mmc_ops
2021-12-07 8:28 ` Avri Altman
@ 2021-12-07 9:30 ` Nishad Kamdar
2021-12-07 9:33 ` Avri Altman
0 siblings, 1 reply; 9+ messages in thread
From: Nishad Kamdar @ 2021-12-07 9:30 UTC (permalink / raw)
To: Avri Altman, Ulf Hansson, Jens Axboe, Adrian Hunter, Bean Huo,
Shawn Lin, Stephen Boyd, Huijin Park, Yue Hu, Wolfram Sang,
Christian Löhle
Cc: Greg Kroah-Hartman, linux-mmc, linux-kernel, nishadkamdar
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.
Thanks for the response,
Nisha
> >
> > 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(+)
> >
> > 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);
> > + 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));
> > + } 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;
> > + }
> > +out:
> > + kfree(data_buf);
> > + return err;
> > +}
> > +EXPORT_SYMBOL_GPL(mmc_set_time);
> > diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
> > index 9c813b851d0b..0c8695d1b363 100644
> > --- a/drivers/mmc/core/mmc_ops.h
> > +++ b/drivers/mmc/core/mmc_ops.h
> > @@ -55,6 +55,8 @@ void mmc_run_bkops(struct mmc_card *card);
> > int mmc_cmdq_enable(struct mmc_card *card);
> > int mmc_cmdq_disable(struct mmc_card *card);
> > int mmc_sanitize(struct mmc_card *card, unsigned int timeout_ms);
> > +int mmc_set_time(struct mmc_card *card, struct mmc_host *host,
> > + u8 rtc_info_type, u64 seconds);
> >
> > #endif
> >
> > diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> > index d9a65c6a8816..52a3bf873d50 100644
> > --- a/include/linux/mmc/mmc.h
> > +++ b/include/linux/mmc/mmc.h
> > @@ -64,6 +64,7 @@
> > #define MMC_WRITE_MULTIPLE_BLOCK 25 /* adtc R1 */
> > #define MMC_PROGRAM_CID 26 /* adtc R1 */
> > #define MMC_PROGRAM_CSD 27 /* adtc R1 */
> > +#define MMC_SET_TIME 49 /* adtc R1 */
> >
> > /* class 6 */
> > #define MMC_SET_WRITE_PROT 28 /* ac [31:0] data addr R1b */
> > --
> > 2.17.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [PATCH] mmc: core: Add support for the eMMC RTC feature in mmc_ops
2021-12-07 9:30 ` Nishad Kamdar
@ 2021-12-07 9:33 ` Avri Altman
2021-12-07 9:43 ` Nishad Kamdar
0 siblings, 1 reply; 9+ messages in thread
From: Avri Altman @ 2021-12-07 9:33 UTC (permalink / raw)
To: Nishad Kamdar, Ulf Hansson, Jens Axboe, Adrian Hunter, Bean Huo,
Shawn Lin, Stephen Boyd, Huijin Park, Yue Hu, Wolfram Sang,
Christian Löhle
Cc: Greg Kroah-Hartman, linux-mmc, linux-kernel
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mmc: core: Add support for the eMMC RTC feature in mmc_ops
2021-12-07 9:33 ` Avri Altman
@ 2021-12-07 9:43 ` Nishad Kamdar
2021-12-07 9:51 ` Greg Kroah-Hartman
0 siblings, 1 reply; 9+ messages in thread
From: Nishad Kamdar @ 2021-12-07 9:43 UTC (permalink / raw)
To: Avri Altman, Ulf Hansson, Jens Axboe, Adrian Hunter, Bean Huo,
Shawn Lin, Stephen Boyd, Huijin Park, Yue Hu, Wolfram Sang,
Christian Löhle
Cc: Greg Kroah-Hartman, linux-mmc, linux-kernel, nishadkamdar
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mmc: core: Add support for the eMMC RTC feature in mmc_ops
2021-12-07 9:43 ` Nishad Kamdar
@ 2021-12-07 9:51 ` Greg Kroah-Hartman
0 siblings, 0 replies; 9+ messages in thread
From: Greg Kroah-Hartman @ 2021-12-07 9:51 UTC (permalink / raw)
To: Nishad Kamdar
Cc: Avri Altman, Ulf Hansson, Jens Axboe, Adrian Hunter, Bean Huo,
Shawn Lin, Stephen Boyd, Huijin Park, Yue Hu, Wolfram Sang,
Christian Löhle, linux-mmc, linux-kernel
On Tue, Dec 07, 2021 at 03:13:08PM +0530, Nishad Kamdar wrote:
> 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.
We do not add code that is not actually used in the kernel tree.
Please submit a user of this new function, otherwise there is no need
for it at all.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mmc: core: Add support for the eMMC RTC feature in mmc_ops
2021-12-05 19:10 [PATCH] mmc: core: Add support for the eMMC RTC feature in mmc_ops Nishad Kamdar
2021-12-06 20:44 ` Stephen Boyd
2021-12-07 8:28 ` Avri Altman
@ 2021-12-07 8:52 ` Christian Löhle
2 siblings, 0 replies; 9+ messages in thread
From: Christian Löhle @ 2021-12-07 8:52 UTC (permalink / raw)
To: Nishad Kamdar, Ulf Hansson, Jens Axboe, Adrian Hunter, Bean Huo,
Shawn Lin, Avri Altman, Stephen Boyd, Huijin Park, Yue Hu,
Wolfram Sang
Cc: Greg Kroah-Hartman, linux-mmc, linux-kernel
>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.
The spec does not mention anything special about error cases where the device can reject CMD49,
if you want some meaningful possibility of this anyway you should get the R1 of the command following on the CMD49, though,
as the raised error would likely be of detection mechanism type X, see table 68.
IMO it would likely only be bit 19 ("ERROR"), but maybe try it with some cards.
So CMD49->CMD13 would be an option.
You could also check if and how some cards reject CMD49 by e.g. setting
Regards,
Christian
From: Nishad Kamdar <nishadkamdar@gmail.com>
Sent: Sunday, December 5, 2021 8:10 PM
To: Ulf Hansson; Jens Axboe; Adrian Hunter; Bean Huo; Shawn Lin; Avri Altman; Stephen Boyd; Huijin Park; Yue Hu; Wolfram Sang
Cc: Nishad Kamdar; Greg Kroah-Hartman; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: [PATCH] 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(+)
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);
+ 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));
+ } 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;
+ }
+out:
+ kfree(data_buf);
+ return err;
+}
+EXPORT_SYMBOL_GPL(mmc_set_time);
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 9c813b851d0b..0c8695d1b363 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -55,6 +55,8 @@ void mmc_run_bkops(struct mmc_card *card);
int mmc_cmdq_enable(struct mmc_card *card);
int mmc_cmdq_disable(struct mmc_card *card);
int mmc_sanitize(struct mmc_card *card, unsigned int timeout_ms);
+int mmc_set_time(struct mmc_card *card, struct mmc_host *host,
+ u8 rtc_info_type, u64 seconds);
#endif
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index d9a65c6a8816..52a3bf873d50 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -64,6 +64,7 @@
#define MMC_WRITE_MULTIPLE_BLOCK 25 /* adtc R1 */
#define MMC_PROGRAM_CID 26 /* adtc R1 */
#define MMC_PROGRAM_CSD 27 /* adtc R1 */
+#define MMC_SET_TIME 49 /* adtc R1 */
/* class 6 */
#define MMC_SET_WRITE_PROT 28 /* ac [31:0] data addr R1b */
--
2.17.1
=
Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782
^ permalink raw reply related [flat|nested] 9+ messages in thread