[PATCH v1 0/2] Add UFS RTC support

[PATCH v1 0/2] Add UFS RTC support

[Bean Huo]

*** BLURB HERE ***

Bean Huo (2):
  scsi: ufs: core: Add UFS RTC support
  scsi: ufs: core: Add sysfs node for UFS RTC update

 drivers/ufs/core/ufs-sysfs.c | 31 ++++++++++++
 drivers/ufs/core/ufshcd.c    | 92 ++++++++++++++++++++++++++++++++++++
 include/ufs/ufs.h            | 20 ++++++++
 include/ufs/ufshcd.h         |  2 +
 4 files changed, 145 insertions(+)

-- 
2.34.1

[PATCH v1 1/2] scsi: ufs: core: Add UFS RTC support

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

The objective of this patch is to incorporate Real Time Clock (RTC) support in Universal
Flash Storage (UFS) device. This enhancement is crucial for the internal maintenance
operations of the UFS device. The patch enables the device to handle both absolute and
relative time information. Furthermore, it includes periodic task to update the RTC in
accordance with the UFS specification, ensuring the accuracy of RTC information for the
device's internal processes.

Signed-off-by: Mike Bi <mikebi@micron.com>
Signed-off-by: Luca Porzio <lporzio@micron.com>
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/ufs/core/ufshcd.c | 92 +++++++++++++++++++++++++++++++++++++++
 include/ufs/ufs.h         | 20 +++++++++
 include/ufs/ufshcd.h      |  2 +
 3 files changed, 114 insertions(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 68d7da02944f..f0e3dd3dd280 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -99,6 +99,9 @@
 /* Polling time to wait for fDeviceInit */
 #define FDEVICEINIT_COMPL_TIMEOUT 1500 /* millisecs */
 
+/* Default RTC update every 10 seconds */
+#define UFS_RTC_UPDATE_EVERY_MS  10*1000
+
 /* UFSHC 4.0 compliant HC support this mode. */
 static bool use_mcq_mode = true;
 
@@ -677,6 +680,8 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
 			hba->dev_info.wb_enabled = false;
 			hba->dev_info.wb_buf_flush_enabled = false;
 		}
+		if (hba->dev_info.rtc_type == UFS_RTC_RELATIVE)
+			hba->dev_info.rtc_time_baseline = 0;
 	}
 	if (err != -EOPNOTSUPP)
 		ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, err);
@@ -8185,6 +8190,85 @@ static void ufs_fixup_device_setup(struct ufs_hba *hba)
 	ufshcd_vops_fixup_dev_quirks(hba);
 }
 
+static int ufshcd_update_rtc(struct ufs_hba *hba)
+{
+	int err = 0;
+	u32 val;
+	struct timespec64 ts64;
+
+	ktime_get_real_ts64(&ts64);
+	val = ts64.tv_sec - hba->dev_info.rtc_time_baseline;
+
+	ufshcd_rpm_get_sync(hba);
+	err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR, QUERY_ATTR_IDN_SECONDS_PASSED,
+					0, 0, &val);
+	ufshcd_rpm_put_sync(hba);
+
+	if (err)
+	        dev_err(hba->dev, "%s: failed to send rtc %d\n",  __func__, err);
+	else if (hba->dev_info.rtc_type == UFS_RTC_RELATIVE)
+		hba->dev_info.rtc_time_baseline = ts64.tv_sec;
+
+	return err;
+}
+
+static void ufshcd_rtc_work(struct work_struct *work)
+{
+	unsigned long flags;
+	struct ufs_hba *hba;
+
+	hba = container_of(to_delayed_work(work), struct ufs_hba, ufs_rtc_delayed_work);
+
+	spin_lock_irqsave(hba->host->host_lock, flags);
+	if (hba->clk_gating.active_reqs || hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
+		hba->outstanding_reqs || hba->outstanding_tasks || hba->active_uic_cmd ||
+		hba->uic_async_done) {
+		/*
+		 * RTC updates should not interfere with normal IO requests; we should only update
+		 * the RTC when there are no ongoing requestsUFS is not idle
+		 */
+		spin_unlock_irqrestore(hba->host->host_lock, flags);
+		goto out;
+	}
+	spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+	ufshcd_update_rtc(hba);
+out:
+	if (ufshcd_is_ufs_dev_active(hba))
+		schedule_delayed_work(&hba->ufs_rtc_delayed_work,
+							msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS));
+	return;
+}
+
+static void  ufs_init_rtc(struct ufs_hba *hba, u8 *desc_buf)
+{
+	struct ufs_dev_info *dev_info = &hba->dev_info;
+	u16 periodic_rtc_update = get_unaligned_be16(&desc_buf[DEVICE_DESC_PARAM_FRQ_RTC]);
+
+	if (periodic_rtc_update & BIT(9)) {
+		dev_info->rtc_type = UFS_RTC_ABSOLUTE;
+		/*
+		 * The concept of measuring time in Linux as the number of seconds elapsed since
+		 * 00:00:00 UTC on January 1, 1970, and UFS ABS RTC is elapsed from January 1st
+		 * 2010 00:00, here we need to adjust ABS baseline.
+		 */
+		dev_info->rtc_time_baseline = mktime64(2010, 1, 1, 0, 0, 0) -
+								mktime64(1970, 1, 1, 0, 0, 0);
+	} else {
+		dev_info->rtc_type = UFS_RTC_RELATIVE;
+		dev_info->rtc_time_baseline = 0;
+	}
+
+	/*
+	 * We ignore TIME_PERIOD defined in wPeriodicRTCUpdate because Spec does not clearly
+	 * define how to calculate the specific update period for each time unit. Here we simply
+	 * set the update period to 1 minute by default.
+	 */
+	hba->dev_info.rtc_update_period = UFS_RTC_UPDATE_EVERY_MS;
+
+	INIT_DELAYED_WORK(&hba->ufs_rtc_delayed_work, ufshcd_rtc_work);
+}
+
 static int ufs_get_device_desc(struct ufs_hba *hba)
 {
 	int err;
@@ -8237,6 +8321,8 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
 
 	ufshcd_temp_notif_probe(hba, desc_buf);
 
+	ufs_init_rtc(hba, desc_buf);
+
 	if (hba->ext_iid_sup)
 		ufshcd_ext_iid_probe(hba, desc_buf);
 
@@ -8790,6 +8876,8 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 	ufshcd_force_reset_auto_bkops(hba);
 
 	ufshcd_set_timestamp_attr(hba);
+	schedule_delayed_work(&hba->ufs_rtc_delayed_work,
+							msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS));
 
 	/* Gear up to HS gear if supported */
 	if (hba->max_pwr_info.is_valid) {
@@ -9746,6 +9834,8 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	ret = ufshcd_vops_suspend(hba, pm_op, POST_CHANGE);
 	if (ret)
 		goto set_link_active;
+
+	cancel_delayed_work(&hba->ufs_rtc_delayed_work);
 	goto out;
 
 set_link_active:
@@ -9840,6 +9930,8 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 		if (ret)
 			goto set_old_link_state;
 		ufshcd_set_timestamp_attr(hba);
+		schedule_delayed_work(&hba->ufs_rtc_delayed_work,
+							msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS));
 	}
 
 	if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
index e77ab1786856..18b39c6b3a97 100644
--- a/include/ufs/ufs.h
+++ b/include/ufs/ufs.h
@@ -14,6 +14,7 @@
 #include <linux/bitops.h>
 #include <linux/types.h>
 #include <uapi/scsi/scsi_bsg_ufs.h>
+#include <linux/rtc.h>
 
 /*
  * Using static_assert() is not allowed in UAPI header files. Hence the check
@@ -551,6 +552,20 @@ struct ufs_vreg_info {
 	struct ufs_vreg *vdd_hba;
 };
 
+enum ufs_rtc_time {
+	UFS_RTC_RELATIVE,
+	UFS_RTC_ABSOLUTE
+};
+
+enum ufs_rtc_time_unit {
+	UFS_RTC_TIME_UNIT_UNDEFINE	= 0x0,
+	UFS_RTC_TIME_UNIT_MONTHS	= 0x1,
+	UFS_RTC_TIME_UNIT_WEEKS		= 0x2,
+	UFS_RTC_TIME_UNIT_DAYS		= 0x3,
+	UFS_RTC_TIME_UNIT_HOURS		= 0x4,
+	UFS_RTC_TIME_UNIT_MINUTES	= 0x5
+};
+
 struct ufs_dev_info {
 	bool	f_power_on_wp_en;
 	/* Keeps information if any of the LU is power on write protected */
@@ -578,6 +593,11 @@ struct ufs_dev_info {
 
 	/* UFS EXT_IID Enable */
 	bool	b_ext_iid_en;
+
+	/* UFS RTC */
+	enum ufs_rtc_time rtc_type;
+	time64_t rtc_time_baseline;
+	u32 rtc_update_period;
 };
 
 /*
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 7f0b2c5599cd..11da11d6870c 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1071,6 +1071,8 @@ struct ufs_hba {
 	struct ufs_hw_queue *uhq;
 	struct ufs_hw_queue *dev_cmd_queue;
 	struct ufshcd_mcq_opr_info_t mcq_opr[OPR_MAX];
+
+	struct delayed_work ufs_rtc_delayed_work;
 };
 
 /**
-- 
2.34.1

[PATCH v1 2/2] scsi: ufs: core: Add sysfs node for UFS RTC update

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

This patch introduces a sysfs node named 'rtc_update_ms' within the kernel, enabling users to
adjust the RTC periodic update frequency to suit the specific requirements of the system and
UFS. Also, this patch allows the user to disable periodic update RTC  in the UFS idle time.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/ufs/core/ufs-sysfs.c | 31 +++++++++++++++++++++++++++++++
 drivers/ufs/core/ufshcd.c    |  4 ++--
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index c95906443d5f..d42846316a86 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -255,6 +255,35 @@ static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
 	return res < 0 ? res : count;
 }
 
+static ssize_t rtc_update_ms_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%d\n", hba->dev_info.rtc_update_period);
+}
+
+static ssize_t rtc_update_ms_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	unsigned int ms;
+	bool resume_period_update;
+
+	if (kstrtouint(buf, 0, &ms))
+		return -EINVAL;
+
+	if (!hba->dev_info.rtc_update_period && ms > 0)
+		resume_period_update =  true;
+	/* Minimum and maximum update frequency should be synchronized with all UFS vendors */
+	hba->dev_info.rtc_update_period = ms;
+
+	if (resume_period_update)
+		schedule_delayed_work(&hba->ufs_rtc_delayed_work,
+						msecs_to_jiffies(hba->dev_info.rtc_update_period));
+	return count;
+}
+
 static ssize_t enable_wb_buf_flush_show(struct device *dev,
 				    struct device_attribute *attr,
 				    char *buf)
@@ -339,6 +368,7 @@ static DEVICE_ATTR_RW(auto_hibern8);
 static DEVICE_ATTR_RW(wb_on);
 static DEVICE_ATTR_RW(enable_wb_buf_flush);
 static DEVICE_ATTR_RW(wb_flush_threshold);
+static DEVICE_ATTR_RW(rtc_update_ms);
 
 static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_rpm_lvl.attr,
@@ -351,6 +381,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_wb_on.attr,
 	&dev_attr_enable_wb_buf_flush.attr,
 	&dev_attr_wb_flush_threshold.attr,
+	&dev_attr_rtc_update_ms.attr,
 	NULL
 };
 
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index f0e3dd3dd280..ae9b60619fd3 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8234,9 +8234,9 @@ static void ufshcd_rtc_work(struct work_struct *work)
 
 	ufshcd_update_rtc(hba);
 out:
-	if (ufshcd_is_ufs_dev_active(hba))
+	if (ufshcd_is_ufs_dev_active(hba) && hba->dev_info.rtc_update_period)
 		schedule_delayed_work(&hba->ufs_rtc_delayed_work,
-							msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS));
+						msecs_to_jiffies(hba->dev_info.rtc_update_period));
 	return;
 }
 
-- 
2.34.1

Re: [PATCH v1 1/2] scsi: ufs: core: Add UFS RTC support

[Thomas Weißschuh]

On 2023-11-09 13:52:16+0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> The objective of this patch is to incorporate Real Time Clock (RTC) support in Universal

No need to talk about "the patch". Use imperative language.

"Add Real Time Clock (RTC) support to Universal ..."

> Flash Storage (UFS) device. This enhancement is crucial for the internal maintenance
> operations of the UFS device. The patch enables the device to handle both absolute and
> relative time information. Furthermore, it includes periodic task to update the RTC in
> accordance with the UFS specification, ensuring the accuracy of RTC information for the
> device's internal processes.
> 
> Signed-off-by: Mike Bi <mikebi@micron.com>
> Signed-off-by: Luca Porzio <lporzio@micron.com>
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/ufs/core/ufshcd.c | 92 +++++++++++++++++++++++++++++++++++++++
>  include/ufs/ufs.h         | 20 +++++++++
>  include/ufs/ufshcd.h      |  2 +
>  3 files changed, 114 insertions(+)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 68d7da02944f..f0e3dd3dd280 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -99,6 +99,9 @@
>  /* Polling time to wait for fDeviceInit */
>  #define FDEVICEINIT_COMPL_TIMEOUT 1500 /* millisecs */
>  
> +/* Default RTC update every 10 seconds */
> +#define UFS_RTC_UPDATE_EVERY_MS  10*1000

                                    (10 * MSEC_PER_SEC)

> +
>  /* UFSHC 4.0 compliant HC support this mode. */
>  static bool use_mcq_mode = true;
>  
> @@ -677,6 +680,8 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
>  			hba->dev_info.wb_enabled = false;
>  			hba->dev_info.wb_buf_flush_enabled = false;
>  		}
> +		if (hba->dev_info.rtc_type == UFS_RTC_RELATIVE)
> +			hba->dev_info.rtc_time_baseline = 0;
>  	}
>  	if (err != -EOPNOTSUPP)
>  		ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, err);
> @@ -8185,6 +8190,85 @@ static void ufs_fixup_device_setup(struct ufs_hba *hba)
>  	ufshcd_vops_fixup_dev_quirks(hba);
>  }
>  
> +static int ufshcd_update_rtc(struct ufs_hba *hba)
> +{
> +	int err = 0;

Initialization is not needed.

> +	u32 val;
> +	struct timespec64 ts64;
> +
> +	ktime_get_real_ts64(&ts64);
> +	val = ts64.tv_sec - hba->dev_info.rtc_time_baseline;
> +
> +	ufshcd_rpm_get_sync(hba);
> +	err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR, QUERY_ATTR_IDN_SECONDS_PASSED,
> +					0, 0, &val);
> +	ufshcd_rpm_put_sync(hba);
> +
> +	if (err)
> +	        dev_err(hba->dev, "%s: failed to send rtc %d\n",  __func__, err);
> +	else if (hba->dev_info.rtc_type == UFS_RTC_RELATIVE)
> +		hba->dev_info.rtc_time_baseline = ts64.tv_sec;
> +
> +	return err;
> +}
> +
> +static void ufshcd_rtc_work(struct work_struct *work)
> +{
> +	unsigned long flags;
> +	struct ufs_hba *hba;
> +
> +	hba = container_of(to_delayed_work(work), struct ufs_hba, ufs_rtc_delayed_work);
> +
> +	spin_lock_irqsave(hba->host->host_lock, flags);
> +	if (hba->clk_gating.active_reqs || hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
> +		hba->outstanding_reqs || hba->outstanding_tasks || hba->active_uic_cmd ||
> +		hba->uic_async_done) {
> +		/*
> +		 * RTC updates should not interfere with normal IO requests; we should only update
> +		 * the RTC when there are no ongoing requestsUFS is not idle
> +		 */

Put this comment above the code it describes.

> +		spin_unlock_irqrestore(hba->host->host_lock, flags);
> +		goto out;
> +	}
> +	spin_unlock_irqrestore(hba->host->host_lock, flags);

Weird control flow, could be:

spin_lock();
is_busy = (hb->clk_gating.active_reqs ...);
spin_unlock();

if (!is_busy)
	ufshcd_update_rtc(hba);

if (ufshcd_is_ufs_dev_active(hba))
	schedule_delayed_work();

> +
> +	ufshcd_update_rtc(hba);
> +out:
> +	if (ufshcd_is_ufs_dev_active(hba))
> +		schedule_delayed_work(&hba->ufs_rtc_delayed_work,
> +							msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS));
> +	return;

No need for this return.

> +}
> +
> +static void  ufs_init_rtc(struct ufs_hba *hba, u8 *desc_buf)
> +{
> +	struct ufs_dev_info *dev_info = &hba->dev_info;
> +	u16 periodic_rtc_update = get_unaligned_be16(&desc_buf[DEVICE_DESC_PARAM_FRQ_RTC]);
> +
> +	if (periodic_rtc_update & BIT(9)) {

BIT(9) should get a name.

> +		dev_info->rtc_type = UFS_RTC_ABSOLUTE;
> +		/*
> +		 * The concept of measuring time in Linux as the number of seconds elapsed since
> +		 * 00:00:00 UTC on January 1, 1970, and UFS ABS RTC is elapsed from January 1st
> +		 * 2010 00:00, here we need to adjust ABS baseline.
> +		 */
> +		dev_info->rtc_time_baseline = mktime64(2010, 1, 1, 0, 0, 0) -
> +								mktime64(1970, 1, 1, 0, 0, 0);
> +	} else {
> +		dev_info->rtc_type = UFS_RTC_RELATIVE;
> +		dev_info->rtc_time_baseline = 0;
> +	}
> +
> +	/*
> +	 * We ignore TIME_PERIOD defined in wPeriodicRTCUpdate because Spec does not clearly
> +	 * define how to calculate the specific update period for each time unit. Here we simply
> +	 * set the update period to 1 minute by default.
> +	 */
> +	hba->dev_info.rtc_update_period = UFS_RTC_UPDATE_EVERY_MS;
> +
> +	INIT_DELAYED_WORK(&hba->ufs_rtc_delayed_work, ufshcd_rtc_work);
> +}
> +
>  static int ufs_get_device_desc(struct ufs_hba *hba)
>  {
>  	int err;
> @@ -8237,6 +8321,8 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
>  
>  	ufshcd_temp_notif_probe(hba, desc_buf);
>  
> +	ufs_init_rtc(hba, desc_buf);
> +

As somebody with no idea and no access to the specs:

Is this available for all devices and all protocol versions?

>  	if (hba->ext_iid_sup)
>  		ufshcd_ext_iid_probe(hba, desc_buf);
>  
> @@ -8790,6 +8876,8 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
>  	ufshcd_force_reset_auto_bkops(hba);
>  
>  	ufshcd_set_timestamp_attr(hba);
> +	schedule_delayed_work(&hba->ufs_rtc_delayed_work,
> +							msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS));
>  
>  	/* Gear up to HS gear if supported */
>  	if (hba->max_pwr_info.is_valid) {
> @@ -9746,6 +9834,8 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  	ret = ufshcd_vops_suspend(hba, pm_op, POST_CHANGE);
>  	if (ret)
>  		goto set_link_active;
> +
> +	cancel_delayed_work(&hba->ufs_rtc_delayed_work);

cancel_delayed_work_sync() ?

>  	goto out;
>  
>  set_link_active:
> @@ -9840,6 +9930,8 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  		if (ret)
>  			goto set_old_link_state;
>  		ufshcd_set_timestamp_attr(hba);
> +		schedule_delayed_work(&hba->ufs_rtc_delayed_work,
> +							msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS));
>  	}
>  
>  	if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
> diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
> index e77ab1786856..18b39c6b3a97 100644
> --- a/include/ufs/ufs.h
> +++ b/include/ufs/ufs.h
> @@ -14,6 +14,7 @@
>  #include <linux/bitops.h>
>  #include <linux/types.h>
>  #include <uapi/scsi/scsi_bsg_ufs.h>
> +#include <linux/rtc.h>

Seems unnecessary.

>  
>  /*
>   * Using static_assert() is not allowed in UAPI header files. Hence the check
> @@ -551,6 +552,20 @@ struct ufs_vreg_info {
>  	struct ufs_vreg *vdd_hba;
>  };
>  
> +enum ufs_rtc_time {
> +	UFS_RTC_RELATIVE,
> +	UFS_RTC_ABSOLUTE
> +};
> +
> +enum ufs_rtc_time_unit {
> +	UFS_RTC_TIME_UNIT_UNDEFINE	= 0x0,
> +	UFS_RTC_TIME_UNIT_MONTHS	= 0x1,
> +	UFS_RTC_TIME_UNIT_WEEKS		= 0x2,
> +	UFS_RTC_TIME_UNIT_DAYS		= 0x3,
> +	UFS_RTC_TIME_UNIT_HOURS		= 0x4,
> +	UFS_RTC_TIME_UNIT_MINUTES	= 0x5
> +};

Not used.

> +
>  struct ufs_dev_info {
>  	bool	f_power_on_wp_en;
>  	/* Keeps information if any of the LU is power on write protected */
> @@ -578,6 +593,11 @@ struct ufs_dev_info {
>  
>  	/* UFS EXT_IID Enable */
>  	bool	b_ext_iid_en;
> +
> +	/* UFS RTC */
> +	enum ufs_rtc_time rtc_type;
> +	time64_t rtc_time_baseline;
> +	u32 rtc_update_period;

It's useful to append the unit to the variable name:
rtc_update_period_ms.

Also this patch is not yet reading this field anywhere.
Should only be introduced in the second patch.

>  };
>  
>  /*
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index 7f0b2c5599cd..11da11d6870c 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -1071,6 +1071,8 @@ struct ufs_hba {
>  	struct ufs_hw_queue *uhq;
>  	struct ufs_hw_queue *dev_cmd_queue;
>  	struct ufshcd_mcq_opr_info_t mcq_opr[OPR_MAX];
> +
> +	struct delayed_work ufs_rtc_delayed_work;
>  };
>  
>  /**
> -- 
> 2.34.1
> 

Re: [PATCH v1 2/2] scsi: ufs: core: Add sysfs node for UFS RTC update

[Thomas Weißschuh]

On 2023-11-09 13:52:17+0100, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> This patch introduces a sysfs node named 'rtc_update_ms' within the kernel, enabling users to
> adjust the RTC periodic update frequency to suit the specific requirements of the system and
> UFS. Also, this patch allows the user to disable periodic update RTC  in the UFS idle time.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/ufs/core/ufs-sysfs.c | 31 +++++++++++++++++++++++++++++++
>  drivers/ufs/core/ufshcd.c    |  4 ++--
>  2 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
> index c95906443d5f..d42846316a86 100644
> --- a/drivers/ufs/core/ufs-sysfs.c
> +++ b/drivers/ufs/core/ufs-sysfs.c
> @@ -255,6 +255,35 @@ static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
>  	return res < 0 ? res : count;
>  }
>  
> +static ssize_t rtc_update_ms_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%d\n", hba->dev_info.rtc_update_period);
> +}
> +
> +static ssize_t rtc_update_ms_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	unsigned int ms;
> +	bool resume_period_update;
> +
> +	if (kstrtouint(buf, 0, &ms))
> +		return -EINVAL;
> +
> +	if (!hba->dev_info.rtc_update_period && ms > 0)
> +		resume_period_update =  true;
> +	/* Minimum and maximum update frequency should be synchronized with all UFS vendors */
> +	hba->dev_info.rtc_update_period = ms;
> +
> +	if (resume_period_update)

Variable will be unitialized when if() above did not trigger.

> +		schedule_delayed_work(&hba->ufs_rtc_delayed_work,
> +						msecs_to_jiffies(hba->dev_info.rtc_update_period));

What about the other work that has already been scheduled?

> +	return count;
> +}
> +
>  static ssize_t enable_wb_buf_flush_show(struct device *dev,
>  				    struct device_attribute *attr,
>  				    char *buf)
> @@ -339,6 +368,7 @@ static DEVICE_ATTR_RW(auto_hibern8);
>  static DEVICE_ATTR_RW(wb_on);
>  static DEVICE_ATTR_RW(enable_wb_buf_flush);
>  static DEVICE_ATTR_RW(wb_flush_threshold);
> +static DEVICE_ATTR_RW(rtc_update_ms);

The whole attribute needs documentation.
>  
>  static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
>  	&dev_attr_rpm_lvl.attr,
> @@ -351,6 +381,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
>  	&dev_attr_wb_on.attr,
>  	&dev_attr_enable_wb_buf_flush.attr,
>  	&dev_attr_wb_flush_threshold.attr,
> +	&dev_attr_rtc_update_ms.attr,
>  	NULL
>  };
>  
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index f0e3dd3dd280..ae9b60619fd3 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8234,9 +8234,9 @@ static void ufshcd_rtc_work(struct work_struct *work)
>  
>  	ufshcd_update_rtc(hba);
>  out:
> -	if (ufshcd_is_ufs_dev_active(hba))
> +	if (ufshcd_is_ufs_dev_active(hba) && hba->dev_info.rtc_update_period)
>  		schedule_delayed_work(&hba->ufs_rtc_delayed_work,
> -							msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS));
> +						msecs_to_jiffies(hba->dev_info.rtc_update_period));
>  	return;
>  }
>  
> -- 
> 2.34.1
> 

RE: [PATCH v1 1/2] scsi: ufs: core: Add UFS RTC support

[Avri Altman]

> From: Bean Huo <beanhuo@micron.com>
> 
> The objective of this patch is to incorporate Real Time Clock (RTC) support in
> Universal Flash Storage (UFS) device. This enhancement is crucial for the
> internal maintenance operations of the UFS device. The patch enables the
> device to handle both absolute and relative time information. Furthermore, it
> includes periodic task to update the RTC in accordance with the UFS
> specification, ensuring the accuracy of RTC information for the device's
> internal processes.
Maybe add some reference to qTimestamp and explain why RTC in seconds is still needed,
when RTC in nanoseconds is already implemented?

Thanks,
Avri

> 
> Signed-off-by: Mike Bi <mikebi@micron.com>
> Signed-off-by: Luca Porzio <lporzio@micron.com>
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/ufs/core/ufshcd.c | 92
> +++++++++++++++++++++++++++++++++++++++
>  include/ufs/ufs.h         | 20 +++++++++
>  include/ufs/ufshcd.h      |  2 +
>  3 files changed, 114 insertions(+)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> 68d7da02944f..f0e3dd3dd280 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -99,6 +99,9 @@
>  /* Polling time to wait for fDeviceInit */  #define
> FDEVICEINIT_COMPL_TIMEOUT 1500 /* millisecs */
> 
> +/* Default RTC update every 10 seconds */ #define
> +UFS_RTC_UPDATE_EVERY_MS  10*1000
> +
>  /* UFSHC 4.0 compliant HC support this mode. */  static bool
> use_mcq_mode = true;
> 
> @@ -677,6 +680,8 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
>                         hba->dev_info.wb_enabled = false;
>                         hba->dev_info.wb_buf_flush_enabled = false;
>                 }
> +               if (hba->dev_info.rtc_type == UFS_RTC_RELATIVE)
> +                       hba->dev_info.rtc_time_baseline = 0;
>         }
>         if (err != -EOPNOTSUPP)
>                 ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, err); @@ -
> 8185,6 +8190,85 @@ static void ufs_fixup_device_setup(struct ufs_hba
> *hba)
>         ufshcd_vops_fixup_dev_quirks(hba);
>  }
> 
> +static int ufshcd_update_rtc(struct ufs_hba *hba) {
> +       int err = 0;
> +       u32 val;
> +       struct timespec64 ts64;
> +
> +       ktime_get_real_ts64(&ts64);
> +       val = ts64.tv_sec - hba->dev_info.rtc_time_baseline;
> +
> +       ufshcd_rpm_get_sync(hba);
> +       err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> QUERY_ATTR_IDN_SECONDS_PASSED,
> +                                       0, 0, &val);
> +       ufshcd_rpm_put_sync(hba);
> +
> +       if (err)
> +               dev_err(hba->dev, "%s: failed to send rtc %d\n",  __func__, err);
> +       else if (hba->dev_info.rtc_type == UFS_RTC_RELATIVE)
> +               hba->dev_info.rtc_time_baseline = ts64.tv_sec;
> +
> +       return err;
> +}
> +
> +static void ufshcd_rtc_work(struct work_struct *work) {
> +       unsigned long flags;
> +       struct ufs_hba *hba;
> +
> +       hba = container_of(to_delayed_work(work), struct ufs_hba,
> + ufs_rtc_delayed_work);
> +
> +       spin_lock_irqsave(hba->host->host_lock, flags);
> +       if (hba->clk_gating.active_reqs || hba->ufshcd_state !=
> UFSHCD_STATE_OPERATIONAL ||
> +               hba->outstanding_reqs || hba->outstanding_tasks || hba-
> >active_uic_cmd ||
> +               hba->uic_async_done) {
> +               /*
> +                * RTC updates should not interfere with normal IO requests; we
> should only update
> +                * the RTC when there are no ongoing requestsUFS is not idle
> +                */
> +               spin_unlock_irqrestore(hba->host->host_lock, flags);
> +               goto out;
> +       }
> +       spin_unlock_irqrestore(hba->host->host_lock, flags);
> +
> +       ufshcd_update_rtc(hba);
> +out:
> +       if (ufshcd_is_ufs_dev_active(hba))
> +               schedule_delayed_work(&hba->ufs_rtc_delayed_work,
> +
> msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS));
> +       return;
> +}
> +
> +static void  ufs_init_rtc(struct ufs_hba *hba, u8 *desc_buf) {
> +       struct ufs_dev_info *dev_info = &hba->dev_info;
> +       u16 periodic_rtc_update =
> +get_unaligned_be16(&desc_buf[DEVICE_DESC_PARAM_FRQ_RTC]);
> +
> +       if (periodic_rtc_update & BIT(9)) {
> +               dev_info->rtc_type = UFS_RTC_ABSOLUTE;
> +               /*
> +                * The concept of measuring time in Linux as the number of seconds
> elapsed since
> +                * 00:00:00 UTC on January 1, 1970, and UFS ABS RTC is elapsed
> from January 1st
> +                * 2010 00:00, here we need to adjust ABS baseline.
> +                */
> +               dev_info->rtc_time_baseline = mktime64(2010, 1, 1, 0, 0, 0) -
> +                                                               mktime64(1970, 1, 1, 0, 0, 0);
> +       } else {
> +               dev_info->rtc_type = UFS_RTC_RELATIVE;
> +               dev_info->rtc_time_baseline = 0;
> +       }
> +
> +       /*
> +        * We ignore TIME_PERIOD defined in wPeriodicRTCUpdate because
> Spec does not clearly
> +        * define how to calculate the specific update period for each time unit.
> Here we simply
> +        * set the update period to 1 minute by default.
> +        */
> +       hba->dev_info.rtc_update_period = UFS_RTC_UPDATE_EVERY_MS;
> +
> +       INIT_DELAYED_WORK(&hba->ufs_rtc_delayed_work,
> ufshcd_rtc_work);
> +}
> +
>  static int ufs_get_device_desc(struct ufs_hba *hba)  {
>         int err;
> @@ -8237,6 +8321,8 @@ static int ufs_get_device_desc(struct ufs_hba
> *hba)
> 
>         ufshcd_temp_notif_probe(hba, desc_buf);
> 
> +       ufs_init_rtc(hba, desc_buf);
> +
>         if (hba->ext_iid_sup)
>                 ufshcd_ext_iid_probe(hba, desc_buf);
> 
> @@ -8790,6 +8876,8 @@ static int ufshcd_device_init(struct ufs_hba *hba,
> bool init_dev_params)
>         ufshcd_force_reset_auto_bkops(hba);
> 
>         ufshcd_set_timestamp_attr(hba);
> +       schedule_delayed_work(&hba->ufs_rtc_delayed_work,
> +
> + msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS));
> 
>         /* Gear up to HS gear if supported */
>         if (hba->max_pwr_info.is_valid) { @@ -9746,6 +9834,8 @@ static int
> __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>         ret = ufshcd_vops_suspend(hba, pm_op, POST_CHANGE);
>         if (ret)
>                 goto set_link_active;
> +
> +       cancel_delayed_work(&hba->ufs_rtc_delayed_work);
>         goto out;
> 
>  set_link_active:
> @@ -9840,6 +9930,8 @@ static int __ufshcd_wl_resume(struct ufs_hba
> *hba, enum ufs_pm_op pm_op)
>                 if (ret)
>                         goto set_old_link_state;
>                 ufshcd_set_timestamp_attr(hba);
> +               schedule_delayed_work(&hba->ufs_rtc_delayed_work,
> +
> + msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS));
>         }
> 
>         if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
> diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h index
> e77ab1786856..18b39c6b3a97 100644
> --- a/include/ufs/ufs.h
> +++ b/include/ufs/ufs.h
> @@ -14,6 +14,7 @@
>  #include <linux/bitops.h>
>  #include <linux/types.h>
>  #include <uapi/scsi/scsi_bsg_ufs.h>
> +#include <linux/rtc.h>
> 
>  /*
>   * Using static_assert() is not allowed in UAPI header files. Hence the check
> @@ -551,6 +552,20 @@ struct ufs_vreg_info {
>         struct ufs_vreg *vdd_hba;
>  };
> 
> +enum ufs_rtc_time {
> +       UFS_RTC_RELATIVE,
> +       UFS_RTC_ABSOLUTE
> +};
> +
> +enum ufs_rtc_time_unit {
> +       UFS_RTC_TIME_UNIT_UNDEFINE      = 0x0,
> +       UFS_RTC_TIME_UNIT_MONTHS        = 0x1,
> +       UFS_RTC_TIME_UNIT_WEEKS         = 0x2,
> +       UFS_RTC_TIME_UNIT_DAYS          = 0x3,
> +       UFS_RTC_TIME_UNIT_HOURS         = 0x4,
> +       UFS_RTC_TIME_UNIT_MINUTES       = 0x5
> +};
> +
>  struct ufs_dev_info {
>         bool    f_power_on_wp_en;
>         /* Keeps information if any of the LU is power on write protected */
> @@ -578,6 +593,11 @@ struct ufs_dev_info {
> 
>         /* UFS EXT_IID Enable */
>         bool    b_ext_iid_en;
> +
> +       /* UFS RTC */
> +       enum ufs_rtc_time rtc_type;
> +       time64_t rtc_time_baseline;
> +       u32 rtc_update_period;
>  };
> 
>  /*
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index
> 7f0b2c5599cd..11da11d6870c 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -1071,6 +1071,8 @@ struct ufs_hba {
>         struct ufs_hw_queue *uhq;
>         struct ufs_hw_queue *dev_cmd_queue;
>         struct ufshcd_mcq_opr_info_t mcq_opr[OPR_MAX];
> +
> +       struct delayed_work ufs_rtc_delayed_work;
>  };
> 
>  /**
> --
> 2.34.1

RE: [PATCH v1 0/2] Add UFS RTC support

[Avri Altman]

> 
> *** BLURB HERE ***
Typo?

> 
> Bean Huo (2):
>   scsi: ufs: core: Add UFS RTC support
>   scsi: ufs: core: Add sysfs node for UFS RTC update
> 
>  drivers/ufs/core/ufs-sysfs.c | 31 ++++++++++++
>  drivers/ufs/core/ufshcd.c    | 92
> ++++++++++++++++++++++++++++++++++++
>  include/ufs/ufs.h            | 20 ++++++++
>  include/ufs/ufshcd.h         |  2 +
>  4 files changed, 145 insertions(+)
> 
> --
> 2.34.1

RE: [PATCH v1 2/2] scsi: ufs: core: Add sysfs node for UFS RTC update

[Avri Altman]

> From: Bean Huo <beanhuo@micron.com>
> 
> This patch introduces a sysfs node named 'rtc_update_ms' within the kernel,
> enabling users to adjust the RTC periodic update frequency to suit the
> specific requirements of the system and UFS. Also, this patch allows the user
> to disable periodic update RTC  in the UFS idle time.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
Forgot to add doc?

Thanks,
Avri

> ---
>  drivers/ufs/core/ufs-sysfs.c | 31 +++++++++++++++++++++++++++++++
>  drivers/ufs/core/ufshcd.c    |  4 ++--
>  2 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c index
> c95906443d5f..d42846316a86 100644
> --- a/drivers/ufs/core/ufs-sysfs.c
> +++ b/drivers/ufs/core/ufs-sysfs.c
> @@ -255,6 +255,35 @@ static ssize_t wb_on_store(struct device *dev,
> struct device_attribute *attr,
>         return res < 0 ? res : count;
>  }
> 
> +static ssize_t rtc_update_ms_show(struct device *dev, struct
> device_attribute *attr,
> +                         char *buf)
> +{
> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> +       return sysfs_emit(buf, "%d\n", hba->dev_info.rtc_update_period);
> +}
> +
> +static ssize_t rtc_update_ms_store(struct device *dev, struct
> device_attribute *attr,
> +                          const char *buf, size_t count) {
> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> +       unsigned int ms;
> +       bool resume_period_update;
> +
> +       if (kstrtouint(buf, 0, &ms))
> +               return -EINVAL;
> +
> +       if (!hba->dev_info.rtc_update_period && ms > 0)
> +               resume_period_update =  true;
> +       /* Minimum and maximum update frequency should be synchronized
> with all UFS vendors */
> +       hba->dev_info.rtc_update_period = ms;
> +
> +       if (resume_period_update)
> +               schedule_delayed_work(&hba->ufs_rtc_delayed_work,
> +                                               msecs_to_jiffies(hba-
> >dev_info.rtc_update_period));
> +       return count;
> +}
> +
>  static ssize_t enable_wb_buf_flush_show(struct device *dev,
>                                     struct device_attribute *attr,
>                                     char *buf) @@ -339,6 +368,7 @@ static
> DEVICE_ATTR_RW(auto_hibern8);  static DEVICE_ATTR_RW(wb_on);  static
> DEVICE_ATTR_RW(enable_wb_buf_flush);
>  static DEVICE_ATTR_RW(wb_flush_threshold);
> +static DEVICE_ATTR_RW(rtc_update_ms);
> 
>  static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
>         &dev_attr_rpm_lvl.attr,
> @@ -351,6 +381,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
>         &dev_attr_wb_on.attr,
>         &dev_attr_enable_wb_buf_flush.attr,
>         &dev_attr_wb_flush_threshold.attr,
> +       &dev_attr_rtc_update_ms.attr,
>         NULL
>  };
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> f0e3dd3dd280..ae9b60619fd3 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8234,9 +8234,9 @@ static void ufshcd_rtc_work(struct work_struct
> *work)
> 
>         ufshcd_update_rtc(hba);
>  out:
> -       if (ufshcd_is_ufs_dev_active(hba))
> +       if (ufshcd_is_ufs_dev_active(hba) &&
> + hba->dev_info.rtc_update_period)
>                 schedule_delayed_work(&hba->ufs_rtc_delayed_work,
> -
> msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS));
> +
> + msecs_to_jiffies(hba->dev_info.rtc_update_period));
>         return;
>  }
> 
> --
> 2.34.1

Re: [PATCH v1 1/2] scsi: ufs: core: Add UFS RTC support

[Bart Van Assche]

On 11/9/23 04:52, Bean Huo wrote:
> The objective of this patch is to incorporate Real Time Clock (RTC) support in Universal
> Flash Storage (UFS) device. This enhancement is crucial for the internal maintenance
> operations of the UFS device.

That sounds vague. Please explain why a UFS device should know the real
time since other storage devices don't need to know the real time.

> +		dev_info->rtc_time_baseline = mktime64(2010, 1, 1, 0, 0, 0) -
> +								mktime64(1970, 1, 1, 0, 0, 0);

The formatting of the above code is not compliant with the kernel coding
style. Please consider reformatting this patch with e.g.
git clang-format HEAD^.

> +	schedule_delayed_work(&hba->ufs_rtc_delayed_work,
> +							msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS));

The formatting of the above code is not compliant with the style guide
either.

> @@ -9746,6 +9834,8 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>   	ret = ufshcd_vops_suspend(hba, pm_op, POST_CHANGE);
>   	if (ret)
>   		goto set_link_active;
> +
> +	cancel_delayed_work(&hba->ufs_rtc_delayed_work);

Why cancel_delayed_work() instead of cancel_delayed_work_sync()?

> @@ -9840,6 +9930,8 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>   		if (ret)
>   			goto set_old_link_state;
>   		ufshcd_set_timestamp_attr(hba);
> +		schedule_delayed_work(&hba->ufs_rtc_delayed_work,
> +							msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS));

The name of the constant "UFS_RTC_UPDATE_EVERY_MS" suggests that the
real-time clock is updated 1000 times per second while the comment above
the UFS_RTC_UPDATE_EVERY_MS constant says that it is updated once every
ten seconds. Please choose a better name for UFS_RTC_UPDATE_EVERY_MS,
e.g. UFS_RTC_UPDATE_INTERVAL_MS.

> diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
> index e77ab1786856..18b39c6b3a97 100644
> --- a/include/ufs/ufs.h
> +++ b/include/ufs/ufs.h
> @@ -14,6 +14,7 @@
>   #include <linux/bitops.h>
>   #include <linux/types.h>
>   #include <uapi/scsi/scsi_bsg_ufs.h>
> +#include <linux/rtc.h>

The include/ufs/ufs.h header file does not depend on anything declared
in <linux/rtc.h> so please move the above include directive into a .c
file.

> +	struct delayed_work ufs_rtc_delayed_work;

Please change the name of this structure member into
ufs_rtc_update_work.

Thanks,

Bart.

Re: [PATCH v1 0/2] Add UFS RTC support

[Bart Van Assche]

On 11/9/23 04:52, Bean Huo wrote:
> *** BLURB HERE ***

Please include a description of the patch series in the cover letter.

Thanks,

Bart.

Re: [PATCH v1 2/2] scsi: ufs: core: Add sysfs node for UFS RTC update

[Bart Van Assche]

On 11/9/23 04:52, Bean Huo wrote:
> This patch introduces a sysfs node named 'rtc_update_ms' within the kernel, enabling users to
> adjust the RTC periodic update frequency to suit the specific requirements of the system and
> UFS. Also, this patch allows the user to disable periodic update RTC  in the UFS idle time.

Why is this behavior enabled by default instead of disabled by default?

Thanks,

Bart.

Re: [PATCH v1 2/2] scsi: ufs: core: Add sysfs node for UFS RTC update

[kernel test robot]

Hi Bean,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on jejb-scsi/for-next linus/master v6.6 next-20231110]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bean-Huo/scsi-ufs-core-Add-UFS-RTC-support/20231110-051048
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link:    https://lore.kernel.org/r/20231109125217.185462-3-beanhuo%40iokpp.de
patch subject: [PATCH v1 2/2] scsi: ufs: core: Add sysfs node for UFS RTC update
config: arm64-allmodconfig (https://download.01.org/0day-ci/archive/20231112/202311120923.S1Zbpb0s-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231112/202311120923.S1Zbpb0s-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311120923.S1Zbpb0s-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/ufs/core/ufs-sysfs.c:276:6: warning: variable 'resume_period_update' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     276 |         if (!hba->dev_info.rtc_update_period && ms > 0)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/ufs/core/ufs-sysfs.c:281:6: note: uninitialized use occurs here
     281 |         if (resume_period_update)
         |             ^~~~~~~~~~~~~~~~~~~~
   drivers/ufs/core/ufs-sysfs.c:276:2: note: remove the 'if' if its condition is always true
     276 |         if (!hba->dev_info.rtc_update_period && ms > 0)
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     277 |                 resume_period_update =  true;
         | ~~~~~~~~~~~~~~~~
>> drivers/ufs/core/ufs-sysfs.c:276:6: warning: variable 'resume_period_update' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
     276 |         if (!hba->dev_info.rtc_update_period && ms > 0)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/ufs/core/ufs-sysfs.c:281:6: note: uninitialized use occurs here
     281 |         if (resume_period_update)
         |             ^~~~~~~~~~~~~~~~~~~~
   drivers/ufs/core/ufs-sysfs.c:276:6: note: remove the '&&' if its condition is always true
     276 |         if (!hba->dev_info.rtc_update_period && ms > 0)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/ufs/core/ufs-sysfs.c:271:27: note: initialize the variable 'resume_period_update' to silence this warning
     271 |         bool resume_period_update;
         |                                  ^
         |                                   = 0
   2 warnings generated.


vim +276 drivers/ufs/core/ufs-sysfs.c

   265	
   266	static ssize_t rtc_update_ms_store(struct device *dev, struct device_attribute *attr,
   267				   const char *buf, size_t count)
   268	{
   269		struct ufs_hba *hba = dev_get_drvdata(dev);
   270		unsigned int ms;
   271		bool resume_period_update;
   272	
   273		if (kstrtouint(buf, 0, &ms))
   274			return -EINVAL;
   275	
 > 276		if (!hba->dev_info.rtc_update_period && ms > 0)
   277			resume_period_update =  true;
   278		/* Minimum and maximum update frequency should be synchronized with all UFS vendors */
   279		hba->dev_info.rtc_update_period = ms;
   280	
   281		if (resume_period_update)
   282			schedule_delayed_work(&hba->ufs_rtc_delayed_work,
   283							msecs_to_jiffies(hba->dev_info.rtc_update_period));
   284		return count;
   285	}
   286	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 1/2] scsi: ufs: core: Add UFS RTC support

[Bean Huo]

Hi Thomas,

Thank you for your review. I will resolve the highlighted issue in the
upcoming version. Two separate questions that require individual
answers as below: 


On Thu, 2023-11-09 at 15:05 +0100, Thomas Weißschuh wrote:
> >   static int ufs_get_device_desc(struct ufs_hba *hba)
> >   {
> >         int err;
> > @@ -8237,6 +8321,8 @@ static int ufs_get_device_desc(struct ufs_hba
> > *hba)
> >   
> >         ufshcd_temp_notif_probe(hba, desc_buf);
> >   
> > +       ufs_init_rtc(hba, desc_buf);
> > +
> 
> As somebody with no idea and no access to the specs:
> 
> Is this available for all devices and all protocol versions?
> 
> > 
I would like to mention that while I cannot confirm that RTC works on
all protocol versions, it has been consistently functional on all
devices in the market since the introduction of UFS 2.0, which also
introduced RTC. I am not aware of any UFS version lower than 2.0
currently available on the market. In the event that a vendor has a
product with a lower UFS version, we can consider implementing a
version check.

> > 
> > 

> > 
> 
> >         goto out;
> >   
> >   set_link_active:
> > @@ -9840,6 +9930,8 @@ static int __ufshcd_wl_resume(struct ufs_hba
> > *hba, enum ufs_pm_op pm_op)
> >                 if (ret)
> >                         goto set_old_link_state;
> >                 ufshcd_set_timestamp_attr(hba);
> > +               schedule_delayed_work(&hba->ufs_rtc_delayed_work,
> > +                                                       msecs_to_ji
> > ffies(UFS_RTC_UPDATE_EVERY_MS));
> >         }
> >   
> >         if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
> > diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
> > index e77ab1786856..18b39c6b3a97 100644
> > --- a/include/ufs/ufs.h
> > +++ b/include/ufs/ufs.h
> > @@ -14,6 +14,7 @@
> >   #include <linux/bitops.h>
> >   #include <linux/types.h>
> >   #include <uapi/scsi/scsi_bsg_ufs.h>
> > +#include <linux/rtc.h>
> 
> Seems unnecessary.

seems it's needed, otherwise, I will get:
./include/ufs/ufs.h:599:9: error: unknown type name ‘time64_t’


Kind regards,
Bean

Re: [PATCH v1 2/2] scsi: ufs: core: Add sysfs node for UFS RTC update

[Bean Huo]

Hi Thomas, 


On Thu, 2023-11-09 at 15:05 +0100, Thomas Weißschuh wrote:
> > +               schedule_delayed_work(&hba->ufs_rtc_delayed_work,
> > +                                               msecs_to_jiffies(hb
> > a->dev_info.rtc_update_period));
> 
> What about the other work that has already been scheduled?

I don't quite understand your concern here,  I need to schedule the
work when needed because of resume_period_update.

Work may have been scheduled, but that's okay, there will be one more
RTC update as expected.

Kind regards,
Bean

Re: [PATCH v1 1/2] scsi: ufs: core: Add UFS RTC support

[Bean Huo]

Hi Avri, 


On Thu, 2023-11-09 at 15:09 +0000, Avri Altman wrote:
> > The objective of this patch is to incorporate Real Time Clock (RTC)
> > support in
> > Universal Flash Storage (UFS) device. This enhancement is crucial
> > for the
> > internal maintenance operations of the UFS device. The patch
> > enables the
> > device to handle both absolute and relative time information.
> > Furthermore, it
> > includes periodic task to update the RTC in accordance with the UFS
> > specification, ensuring the accuracy of RTC information for the
> > device's
> > internal processes.
> Maybe add some reference to qTimestamp and explain why RTC in seconds
> is still needed,
> when RTC in nanoseconds is already implemented?

Yes, I will add an explanation in the next version.

Kind regards,
Bean

Re: [PATCH v1 2/2] scsi: ufs: core: Add sysfs node for UFS RTC update

[Bean Huo]

Hi Bart,


On Thu, 2023-11-09 at 10:07 -0800, Bart Van Assche wrote:
> On 11/9/23 04:52, Bean Huo wrote:
> > This patch introduces a sysfs node named 'rtc_update_ms' within the
> > kernel, enabling users to
> > adjust the RTC periodic update frequency to suit the specific
> > requirements of the system and
> > UFS. Also, this patch allows the user to disable periodic update
> > RTC  in the UFS idle time.
> 
> Why is this behavior enabled by default instead of disabled by
> default?

No problem, I will disable it by default in the next version and let
customers choose on a case-by-case basis.

Kind regards,
Bean

Re: [PATCH v1 2/2] scsi: ufs: core: Add sysfs node for UFS RTC update

[Thomas Weißschuh]

On 2023-11-14 19:39:55+0100, Bean Huo wrote:
> On Thu, 2023-11-09 at 15:05 +0100, Thomas Weißschuh wrote:
> > > +               schedule_delayed_work(&hba->ufs_rtc_delayed_work,
> > > +                                               msecs_to_jiffies(hb
> > > a->dev_info.rtc_update_period));
> > 
> > What about the other work that has already been scheduled?
> 
> I don't quite understand your concern here,  I need to schedule the
> work when needed because of resume_period_update.
> 
> Work may have been scheduled, but that's okay, there will be one more
> RTC update as expected.

If it's fine to have this additional update, all is good.

Thomas

Re: [PATCH v1 1/2] scsi: ufs: core: Add UFS RTC support

[Thomas Weißschuh]

On 2023-11-14 19:27:37+0100, Bean Huo wrote:
> Thank you for your review. I will resolve the highlighted issue in the
> upcoming version. Two separate questions that require individual
> answers as below: 

Thanks for taking the feedback into account!

> On Thu, 2023-11-09 at 15:05 +0100, Thomas Weißschuh wrote:
> > >   static int ufs_get_device_desc(struct ufs_hba *hba)
> > >   {
> > >         int err;
> > > @@ -8237,6 +8321,8 @@ static int ufs_get_device_desc(struct ufs_hba
> > > *hba)
> > >   
> > >         ufshcd_temp_notif_probe(hba, desc_buf);
> > >   
> > > +       ufs_init_rtc(hba, desc_buf);
> > > +
> > 
> > As somebody with no idea and no access to the specs:
> > 
> > Is this available for all devices and all protocol versions?
> > 
> > > 
> I would like to mention that while I cannot confirm that RTC works on
> all protocol versions, it has been consistently functional on all
> devices in the market since the introduction of UFS 2.0, which also
> introduced RTC. I am not aware of any UFS version lower than 2.0
> currently available on the market. In the event that a vendor has a
> product with a lower UFS version, we can consider implementing a
> version check.

Thanks for the info!
Could you mention this somewhere in the commit message?

> > >         goto out;
> > >   
> > >   set_link_active:
> > > @@ -9840,6 +9930,8 @@ static int __ufshcd_wl_resume(struct ufs_hba
> > > *hba, enum ufs_pm_op pm_op)
> > >                 if (ret)
> > >                         goto set_old_link_state;
> > >                 ufshcd_set_timestamp_attr(hba);
> > > +               schedule_delayed_work(&hba->ufs_rtc_delayed_work,
> > > +                                                       msecs_to_ji
> > > ffies(UFS_RTC_UPDATE_EVERY_MS));
> > >         }
> > >   
> > >         if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
> > > diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
> > > index e77ab1786856..18b39c6b3a97 100644
> > > --- a/include/ufs/ufs.h
> > > +++ b/include/ufs/ufs.h
> > > @@ -14,6 +14,7 @@
> > >   #include <linux/bitops.h>
> > >   #include <linux/types.h>
> > >   #include <uapi/scsi/scsi_bsg_ufs.h>
> > > +#include <linux/rtc.h>
> > 
> > Seems unnecessary.
> 
> seems it's needed, otherwise, I will get:
> ./include/ufs/ufs.h:599:9: error: unknown type name ‘time64_t’

Then it should be "#include <linux/time64.h>", which is more specific.


Thomas