Adding RTC support for embedded storage device UFS in its driver, it is
important for a few key reasons:
1. Helps with Regular Maintenance:
The RTC provides a basic way to keep track of time, making it useful for
scheduling routine maintenance tasks in the storage device. This includes
things like making sure data is spread
evenly across the storage to extend its life.
2. Figuring Out How Old Data Is:
The RTC helps the device estimate how long ago certain parts of the storage
were last used. This is handy for deciding when to do maintenance tasks to
keep the storage working well over time.
3. Making Devices Last Longer:
By using the RTC for regular upkeep, we can make sure the storage device lasts
longer and stays reliable. This is especially important for devices that need
to work well for a long time.
4.Fitting In with Other Devices:
The inclusion of RTC support aligns with existing UFS specifications (starting
from UFS Spec 2.0) and is consistent with the prevalent industry practice. Many
UFS devices currently on the market utilize RTC for internal timekeeping. By
ensuring compatibility with this widely adopted standard, the embedded storage
device becomes seamlessly integrable with existing hardware and software
ecosystems, reducing the risk of compatibility issues.
In short, adding RTC support to embedded storage device UFS helps with regular
upkeep, extends the device's life, ensures compatibility, and keeps everything
running smoothly with the rest of the system.
Changelog:
v3--v4:
1. Incorporate a check for "Current time precedes previous setting" in ufshcd_update_rtc()
2. Eliminate redundant return value code in ufshcd_update_rtc().
v2--v3:
1. Move ufshcd_is_ufs_dev_busy() to the source file ufshcd.c in patch 1/3.
2. Format commit statement in patch 2/3.
v1--v2:
1. Add a new patch "scsi: ufs: core: Add ufshcd_is_ufs_dev_busy()"
2. RTC periodic update work is disabled by default
3. Address several issues raised by Avri, Bart, and Thomas.
Bean Huo (3):
scsi: ufs: core: Add ufshcd_is_ufs_dev_busy()
scsi: ufs: core: Add UFS RTC support
scsi: ufs: core: Add sysfs node for UFS RTC update
Documentation/ABI/testing/sysfs-driver-ufs | 7 ++
drivers/ufs/core/ufs-sysfs.c | 31 +++++++
drivers/ufs/core/ufshcd.c | 103 ++++++++++++++++++++-
include/ufs/ufs.h | 15 +++
include/ufs/ufshcd.h | 4 +
5 files changed, 156 insertions(+), 4 deletions(-)
--
2.34.1
From: Bean Huo <[email protected]>
Add helper inline for retrieving whether UFS device is busy or not.
Signed-off-by: Bean Huo <[email protected]>
Reviewed-by: Avri Altman <[email protected]>
---
drivers/ufs/core/ufshcd.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index f0b837cb0c2b..32cfcba66d60 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -235,6 +235,13 @@ ufs_get_desired_pm_lvl_for_dev_link_state(enum ufs_dev_pwr_mode dev_state,
return UFS_PM_LVL_0;
}
+static inline bool ufshcd_is_ufs_dev_busy(struct ufs_hba *hba)
+{
+ return (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);
+}
+
static const struct ufs_dev_quirk ufs_fixups[] = {
/* UFS cards deviations table */
{ .wmanufacturerid = UFS_VENDOR_MICRON,
@@ -1917,10 +1924,7 @@ static void ufshcd_gate_work(struct work_struct *work)
goto rel_lock;
}
- 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)
+ if (ufshcd_is_ufs_dev_busy(hba))
goto rel_lock;
spin_unlock_irqrestore(hba->host->host_lock, flags);
--
2.34.1
From: Bean Huo <[email protected]>
Add Real Time Clock (RTC) support for 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 Spec,
ensuring the accuracy of RTC information for the device's internal processes.
RTC and qTimestamp serve distinct purposes. The RTC provides a coarse level
of granularity with, at best, approximate single-second resolution. This makes
the RTC well-suited for the device to determine the approximate age of programmed
blocks after being updated by the host. On the other hand, qTimestamp offers
nanosecond granularity and is specifically designed for synchronizing Device
Error Log entries with corresponding host-side logs.
Given that the RTC has been a standard feature since UFS Spec 2.0, and qTimestamp
was introduced in UFS Spec 4.0, the majority of UFS devices currently on the
market rely on RTC. Therefore, it is advisable to continue supporting RTC in
the Linux kernel. This ensures compatibility with the prevailing UFS device
implementations and facilitates seamless integration with existing hardware.
By maintaining support for RTC, we ensure broad compatibility and avoid potential
issues arising from deviations in device specifications across different UFS
versions.
Signed-off-by: Bean Huo <[email protected]>
Signed-off-by: Mike Bi <[email protected]>
Signed-off-by: Luca Porzio <[email protected]>
Acked-by: Avri Altman <[email protected]>
---
drivers/ufs/core/ufshcd.c | 84 +++++++++++++++++++++++++++++++++++++++
include/ufs/ufs.h | 14 +++++++
include/ufs/ufshcd.h | 4 ++
3 files changed, 102 insertions(+)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 32cfcba66d60..dedb0c08363b 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_INTERVAL_MS (10 * MSEC_PER_SEC)
+
/* UFSHC 4.0 compliant HC support this mode. */
static bool use_mcq_mode = true;
@@ -684,6 +687,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);
@@ -8191,6 +8196,77 @@ static void ufs_fixup_device_setup(struct ufs_hba *hba)
ufshcd_vops_fixup_dev_quirks(hba);
}
+static void ufshcd_update_rtc(struct ufs_hba *hba)
+{
+ int err;
+ u32 val;
+ struct timespec64 ts64;
+
+ ktime_get_real_ts64(&ts64);
+
+ if (ts64.tv_sec < hba->dev_info.rtc_time_baseline) {
+ dev_warn(hba->dev, "%s: Current time precedes previous setting!\n", __func__);
+ return;
+ }
+ /*
+ * Absolute RTC mode has 136-year limit as of 2010. Modify UFS Spec or choosing relative
+ * RTC mode for longer (beyond year 2146) time spans.
+ */
+ 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 update rtc %d\n", __func__, err);
+ else if (hba->dev_info.rtc_type == UFS_RTC_RELATIVE)
+ hba->dev_info.rtc_time_baseline = ts64.tv_sec;
+}
+
+static void ufshcd_rtc_work(struct work_struct *work)
+{
+ struct ufs_hba *hba;
+ bool is_busy;
+
+ hba = container_of(to_delayed_work(work), struct ufs_hba, ufs_rtc_update_work);
+
+ is_busy = ufshcd_is_ufs_dev_busy(hba);
+ /*
+ * RTC updates should not interfere with normal IO requests; we should only update the RTC
+ * when there are no ongoing requests.
+ */
+ if (!is_busy)
+ ufshcd_update_rtc(hba);
+
+ if (ufshcd_is_ufs_dev_active(hba))
+ schedule_delayed_work(&hba->ufs_rtc_update_work,
+ msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));
+}
+
+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 & UFS_RTC_TIME_BASELINE) {
+ 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;
+ }
+
+ INIT_DELAYED_WORK(&hba->ufs_rtc_update_work, ufshcd_rtc_work);
+}
+
static int ufs_get_device_desc(struct ufs_hba *hba)
{
int err;
@@ -8243,6 +8319,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);
@@ -8796,6 +8874,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_update_work,
+ msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));
/* Gear up to HS gear if supported */
if (hba->max_pwr_info.is_valid) {
@@ -9753,6 +9833,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_sync(&hba->ufs_rtc_update_work);
goto out;
set_link_active:
@@ -9847,6 +9929,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_update_work,
+ msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));
}
if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
index e77ab1786856..8022d267fe8a 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/time64.h>
/*
* Using static_assert() is not allowed in UAPI header files. Hence the check
@@ -551,6 +552,15 @@ struct ufs_vreg_info {
struct ufs_vreg *vdd_hba;
};
+/*
+ * UFS device descriptor wPeriodicRTCUpdate bit9 defines RTC time baseline.
+ */
+#define UFS_RTC_TIME_BASELINE BIT(9)
+enum ufs_rtc_time {
+ UFS_RTC_RELATIVE,
+ UFS_RTC_ABSOLUTE
+};
+
struct ufs_dev_info {
bool f_power_on_wp_en;
/* Keeps information if any of the LU is power on write protected */
@@ -578,6 +588,10 @@ 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;
};
/*
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index d862c8ddce03..727bdf296b34 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -912,6 +912,8 @@ enum ufshcd_mcq_opr {
* @mcq_base: Multi circular queue registers base address
* @uhq: array of supported hardware queues
* @dev_cmd_queue: Queue for issuing device management commands
+ * @mcq_opr: MCQ operation and runtime registers
+ * @ufs_rtc_update_work: A work for UFS RTC periodic update
*/
struct ufs_hba {
void __iomem *mmio_base;
@@ -1076,6 +1078,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_update_work;
};
/**
--
2.34.1
On Fri, Dec 08, 2023 at 11:39:38AM +0100, Bean Huo wrote:
> From: Bean Huo <[email protected]>
>
> Add helper inline for retrieving whether UFS device is busy or not.
>
> Signed-off-by: Bean Huo <[email protected]>
> Reviewed-by: Avri Altman <[email protected]>
> ---
> drivers/ufs/core/ufshcd.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index f0b837cb0c2b..32cfcba66d60 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -235,6 +235,13 @@ ufs_get_desired_pm_lvl_for_dev_link_state(enum ufs_dev_pwr_mode dev_state,
> return UFS_PM_LVL_0;
> }
>
> +static inline bool ufshcd_is_ufs_dev_busy(struct ufs_hba *hba)
No need to use 'inline' keyword in '.c' files. Compiler has its own decisions.
> +{
> + return (hba->clk_gating.active_reqs || hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
I feel like checking the ufshcd state inside ufshcd_is_ufs_dev_busy() is
somewhat confusing. Because, if "ufshcd_state != UFSHCD_STATE_OPERATIONAL"
returns true, then it means that the ufshc is in non-operational state. But,
non-operational state doesn't mean that the ufs is busy doing some work.
So I propose to move this check outside of ufshcd_is_ufs_dev_busy().
Like,
if (hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
ufshcd_is_ufs_dev_busy(hba))
- Mani
> + hba->outstanding_reqs || hba->outstanding_tasks || hba->active_uic_cmd ||
> + hba->uic_async_done);
> +}
> +
> static const struct ufs_dev_quirk ufs_fixups[] = {
> /* UFS cards deviations table */
> { .wmanufacturerid = UFS_VENDOR_MICRON,
> @@ -1917,10 +1924,7 @@ static void ufshcd_gate_work(struct work_struct *work)
> goto rel_lock;
> }
>
> - 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)
> + if (ufshcd_is_ufs_dev_busy(hba))
> goto rel_lock;
>
> spin_unlock_irqrestore(hba->host->host_lock, flags);
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
On Fri, Dec 08, 2023 at 11:39:39AM +0100, Bean Huo wrote:
> From: Bean Huo <[email protected]>
>
> Add Real Time Clock (RTC) support for 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 Spec,
> ensuring the accuracy of RTC information for the device's internal processes.
>
> RTC and qTimestamp serve distinct purposes. The RTC provides a coarse level
> of granularity with, at best, approximate single-second resolution. This makes
> the RTC well-suited for the device to determine the approximate age of programmed
> blocks after being updated by the host. On the other hand, qTimestamp offers
> nanosecond granularity and is specifically designed for synchronizing Device
> Error Log entries with corresponding host-side logs.
>
> Given that the RTC has been a standard feature since UFS Spec 2.0, and qTimestamp
> was introduced in UFS Spec 4.0, the majority of UFS devices currently on the
> market rely on RTC. Therefore, it is advisable to continue supporting RTC in
> the Linux kernel. This ensures compatibility with the prevailing UFS device
> implementations and facilitates seamless integration with existing hardware.
> By maintaining support for RTC, we ensure broad compatibility and avoid potential
> issues arising from deviations in device specifications across different UFS
> versions.
>
> Signed-off-by: Bean Huo <[email protected]>
> Signed-off-by: Mike Bi <[email protected]>
> Signed-off-by: Luca Porzio <[email protected]>
> Acked-by: Avri Altman <[email protected]>
> ---
> drivers/ufs/core/ufshcd.c | 84 +++++++++++++++++++++++++++++++++++++++
> include/ufs/ufs.h | 14 +++++++
> include/ufs/ufshcd.h | 4 ++
> 3 files changed, 102 insertions(+)
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 32cfcba66d60..dedb0c08363b 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_INTERVAL_MS (10 * MSEC_PER_SEC)
> +
> /* UFSHC 4.0 compliant HC support this mode. */
> static bool use_mcq_mode = true;
>
> @@ -684,6 +687,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);
> @@ -8191,6 +8196,77 @@ static void ufs_fixup_device_setup(struct ufs_hba *hba)
> ufshcd_vops_fixup_dev_quirks(hba);
> }
>
> +static void ufshcd_update_rtc(struct ufs_hba *hba)
> +{
> + int err;
> + u32 val;
> + struct timespec64 ts64;
Reverse Xmas order please. Here and in other functions.
> +
> + ktime_get_real_ts64(&ts64);
> +
> + if (ts64.tv_sec < hba->dev_info.rtc_time_baseline) {
Double space after 'if'
> + dev_warn(hba->dev, "%s: Current time precedes previous setting!\n", __func__);
If there is no RTC on the host, this warning will be printed for 40 years. More below...
> + return;
> + }
Newline
> + /*
> + * Absolute RTC mode has 136-year limit as of 2010. Modify UFS Spec or choosing relative
> + * RTC mode for longer (beyond year 2146) time spans.
I feel like this comment is not clear enough.
Maybe something like,
"The code is bound to work for 136 years with relative mode and till year 2146
with absolute mode."
> + */
> + val = ts64.tv_sec - hba->dev_info.rtc_time_baseline;
> +
This logic will work if the host has RTC. But if there is no RTC, then tv_sec
will return time elapsed since boot. The spec clearly states that host should
use absolute mode if it has RTC and relative otherwise.
Maybe you should add a logic to detect whether RTC is present or not and
override the mode in device?
> + 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 update rtc %d\n", __func__, err);
> + else if (hba->dev_info.rtc_type == UFS_RTC_RELATIVE)
> + hba->dev_info.rtc_time_baseline = ts64.tv_sec;
> +}
> +
> +static void ufshcd_rtc_work(struct work_struct *work)
> +{
> + struct ufs_hba *hba;
> + bool is_busy;
> +
> + hba = container_of(to_delayed_work(work), struct ufs_hba, ufs_rtc_update_work);
> +
> + is_busy = ufshcd_is_ufs_dev_busy(hba);
Newline
> + /*
> + * RTC updates should not interfere with normal IO requests; we should only update the RTC
No semicolon within comments please. Use full stop for sentence breaks.
> + * when there are no ongoing requests.
> + */
> + if (!is_busy)
> + ufshcd_update_rtc(hba);
> +
> + if (ufshcd_is_ufs_dev_active(hba))
> + schedule_delayed_work(&hba->ufs_rtc_update_work,
> + msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));
> +}
> +
> +static void ufs_init_rtc(struct ufs_hba *hba, u8 *desc_buf)
Double space after void.
> +{
> + 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 & UFS_RTC_TIME_BASELINE) {
> + dev_info->rtc_type = UFS_RTC_ABSOLUTE;
Newline
- Mani
> + /*
> + * 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;
> + }
> +
> + INIT_DELAYED_WORK(&hba->ufs_rtc_update_work, ufshcd_rtc_work);
> +}
> +
> static int ufs_get_device_desc(struct ufs_hba *hba)
> {
> int err;
> @@ -8243,6 +8319,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);
>
> @@ -8796,6 +8874,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_update_work,
> + msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));
>
> /* Gear up to HS gear if supported */
> if (hba->max_pwr_info.is_valid) {
> @@ -9753,6 +9833,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_sync(&hba->ufs_rtc_update_work);
> goto out;
>
> set_link_active:
> @@ -9847,6 +9929,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_update_work,
> + msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));
> }
>
> if (ufshcd_keep_autobkops_enabled_except_suspend(hba))
> diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
> index e77ab1786856..8022d267fe8a 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/time64.h>
>
> /*
> * Using static_assert() is not allowed in UAPI header files. Hence the check
> @@ -551,6 +552,15 @@ struct ufs_vreg_info {
> struct ufs_vreg *vdd_hba;
> };
>
> +/*
> + * UFS device descriptor wPeriodicRTCUpdate bit9 defines RTC time baseline.
> + */
> +#define UFS_RTC_TIME_BASELINE BIT(9)
> +enum ufs_rtc_time {
> + UFS_RTC_RELATIVE,
> + UFS_RTC_ABSOLUTE
> +};
> +
> struct ufs_dev_info {
> bool f_power_on_wp_en;
> /* Keeps information if any of the LU is power on write protected */
> @@ -578,6 +588,10 @@ 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;
> };
>
> /*
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index d862c8ddce03..727bdf296b34 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -912,6 +912,8 @@ enum ufshcd_mcq_opr {
> * @mcq_base: Multi circular queue registers base address
> * @uhq: array of supported hardware queues
> * @dev_cmd_queue: Queue for issuing device management commands
> + * @mcq_opr: MCQ operation and runtime registers
> + * @ufs_rtc_update_work: A work for UFS RTC periodic update
> */
> struct ufs_hba {
> void __iomem *mmio_base;
> @@ -1076,6 +1078,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_update_work;
> };
>
> /**
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
On Fri, 2023-12-08 at 20:20 +0530, Manivannan Sadhasivam wrote:
> > + */
> > + val = ts64.tv_sec - hba->dev_info.rtc_time_baseline;
> > +
>
> This logic will work if the host has RTC. But if there is no RTC,
> then tv_sec
> will return time elapsed since boot. The spec clearly states that
> host should
> use absolute mode if it has RTC and relative otherwise.
>
> Maybe you should add a logic to detect whether RTC is present or not
> and
> override the mode in device?
Thank you for your reviews. I will incorporate the suggested changes
into the patch, addressing all comments except for the RTC mode switch.
The proposal is to perform the RTC mode switch during UFS provisioning,
not at runtime in the UFS online phase. This approach ensures that the
UFS configuration is populated based on the RTC configuration
established during provisioning. It is advisable not to change the RTC
mode after provisioning is complete. Additionally, the usage of tv_sec,
which returns time elapsed since boot, suggests that there is no issue
with utilizing the RTC in this context.
Kind regards,
Bean
On Fri, Dec 08, 2023 at 04:12:44PM +0100, Bean Huo wrote:
> On Fri, 2023-12-08 at 20:20 +0530, Manivannan Sadhasivam wrote:
> > > + */
> > > + val = ts64.tv_sec - hba->dev_info.rtc_time_baseline;
> > > +
> >
> > This logic will work if the host has RTC. But if there is no RTC,
> > then tv_sec
> > will return time elapsed since boot. The spec clearly states that
> > host should
> > use absolute mode if it has RTC and relative otherwise.
> >
> > Maybe you should add a logic to detect whether RTC is present or not
> > and
> > override the mode in device?
>
> Thank you for your reviews. I will incorporate the suggested changes
> into the patch, addressing all comments except for the RTC mode switch.
> The proposal is to perform the RTC mode switch during UFS provisioning,
> not at runtime in the UFS online phase. This approach ensures that the
> UFS configuration is populated based on the RTC configuration
> established during provisioning. It is advisable not to change the RTC
> mode after provisioning is complete. Additionally, the usage of tv_sec,
> which returns time elapsed since boot, suggests that there is no issue
> with utilizing the RTC in this context.
Except that the warning will be issued to users after each 10s for 40 years.
Atleast get rid of that.
- Mani
>
> Kind regards,
> Bean
--
மணிவண்ணன் சதாசிவம்
On Fri, Dec 08, 2023 at 10:36:24PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Dec 08, 2023 at 04:12:44PM +0100, Bean Huo wrote:
> > On Fri, 2023-12-08 at 20:20 +0530, Manivannan Sadhasivam wrote:
> > > > + */
> > > > + val = ts64.tv_sec - hba->dev_info.rtc_time_baseline;
> > > > +
> > >
> > > This logic will work if the host has RTC. But if there is no RTC,
> > > then tv_sec
> > > will return time elapsed since boot. The spec clearly states that
> > > host should
> > > use absolute mode if it has RTC and relative otherwise.
> > >
> > > Maybe you should add a logic to detect whether RTC is present or not
> > > and
> > > override the mode in device?
> >
> > Thank you for your reviews. I will incorporate the suggested changes
> > into the patch, addressing all comments except for the RTC mode switch.
> > The proposal is to perform the RTC mode switch during UFS provisioning,
> > not at runtime in the UFS online phase. This approach ensures that the
> > UFS configuration is populated based on the RTC configuration
> > established during provisioning. It is advisable not to change the RTC
> > mode after provisioning is complete. Additionally, the usage of tv_sec,
> > which returns time elapsed since boot, suggests that there is no issue
> > with utilizing the RTC in this context.
>
> Except that the warning will be issued to users after each 10s for 40 years.
> Atleast get rid of that.
>
I tried this series on Qcom RB5 board and found the issue due to the usage of
UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH flag. When this flag is set,
ufshcd_device_init() will be called twice due to reinit of the controller and
PHY.
Since RTC work is initialized and scheduled from ufshcd_device_init(), panic
happens during second time. Is it possible to move RTC init outside of
ufshcd_device_init(). Maybe you can parse RTC params in ufshcd_device_init()
and initialize the work elsewhere. Or you can cancel the work before calling
ufshcd_device_init() second time.
- Mani
> - Mani
>
> >
> > Kind regards,
> > Bean
>
> --
> மணிவண்ணன் சதாசிவம்
--
மணிவண்ணன் சதாசிவம்
On Fri, 2023-12-08 at 23:01 +0530, Manivannan Sadhasivam wrote:
> > >
> > > Thank you for your reviews. I will incorporate the suggested
> > > changes
> > > into the patch, addressing all comments except for the RTC mode
> > > switch.
> > > The proposal is to perform the RTC mode switch during UFS
> > > provisioning,
> > > not at runtime in the UFS online phase. This approach ensures
> > > that the
> > > UFS configuration is populated based on the RTC configuration
> > > established during provisioning. It is advisable not to change
> > > the RTC
> > > mode after provisioning is complete. Additionally, the usage of
> > > tv_sec,
> > > which returns time elapsed since boot, suggests that there is no
> > > issue
> > > with utilizing the RTC in this context.
> >
> > Except that the warning will be issued to users after each 10s for
> > 40 years.
> > Atleast get rid of that.
> >
>
> I tried this series on Qcom RB5 board and found the issue due to the
> usage of
> UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH flag. When this flag is
> set,
> ufshcd_device_init() will be called twice due to reinit of the
> controller and
> PHY.
>
> Since RTC work is initialized and scheduled from
> ufshcd_device_init(), panic
> happens during second time. Is it possible to move RTC init outside
> of
> ufshcd_device_init(). Maybe you can parse RTC params in
> ufshcd_device_init()
> and initialize the work elsewhere. Or you can cancel the work before
> calling
> ufshcd_device_init() second time.
>
> - Mani
Thank you for your review. I have moved the INIT_DELAYED_WORK(&hba-
>ufs_rtc_update_work, ufshcd_rtc_work) to ufshcd_init() from
ufs_init_rtc(). This modification has been tested on the Qcom platform.
Regarding the warning, instead of removing it entirely, I have switched
it to dev_dbg. This adjustment is made with the consideration that some
form of customer notification is still necessary.
changes as below:
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 953d50cc4256..cb6b0c286367 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8205,7 +8205,7 @@ static void ufshcd_update_rtc(struct ufs_hba
*hba)
ktime_get_real_ts64(&ts64);
if (ts64.tv_sec < hba->dev_info.rtc_time_baseline) {
- dev_warn(hba->dev, "%s: Current time precedes previous
setting!\n", __func__);
+ dev_dbg(hba->dev, "%s: Current time precedes previous
setting!\n", __func__);
return;
}
/*
@@ -8270,8 +8270,6 @@ static void ufs_init_rtc(struct ufs_hba *hba, u8
*desc_buf)
* update work, and let user configure by sysfs node according
to specific circumstance.
*/
hba->dev_info.rtc_update_period = 0;
-
- INIT_DELAYED_WORK(&hba->ufs_rtc_update_work, ufshcd_rtc_work);
}
static int ufs_get_device_desc(struct ufs_hba *hba)
@@ -10634,8 +10632,8 @@ int ufshcd_init(struct ufs_hba *hba, void
__iomem *mmio_base, unsigned int irq)
UFS_SLEEP_PWR_MODE,
UIC_LINK_HIBERN8_STATE);
- INIT_DELAYED_WORK(&hba->rpm_dev_flush_recheck_work,
- ufshcd_rpm_dev_flush_recheck_work);
+ INIT_DELAYED_WORK(&hba->rpm_dev_flush_recheck_work,
ufshcd_rpm_dev_flush_recheck_work);
+ INIT_DELAYED_WORK(&hba->ufs_rtc_update_work, ufshcd_rtc_work);
/* Set the default auto-hiberate idle timer value to 150 ms */
if (ufshcd_is_auto_hibern8_supported(hba) && !hba->ahit) {
Kind regards,
Bean
On Fri, 2023-12-08 at 22:36 +0530, Manivannan Sadhasivam wrote:
> Except that the warning will be issued to users after each 10s for 40
> years.
> Atleast get rid of that.
how about using dev_warn_once(), instead of dev_warn, using
dev_warn_once() ensures the warning is issued only once.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 953d50cc4256..b2287d2f9bf3 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8205,7 +8205,7 @@ static void ufshcd_update_rtc(struct ufs_hba
*hba)
ktime_get_real_ts64(&ts64);
if (ts64.tv_sec < hba->dev_info.rtc_time_baseline) {
- dev_warn(hba->dev, "%s: Current time precedes previous
setting!\n", __func__);
+ dev_warn_once(hba->dev, "%s: Current time precedes
previous setting!\n", __func__);
Kind regards,
Bean
On Sun, Dec 10, 2023 at 08:28:21PM +0100, Bean Huo wrote:
> On Fri, 2023-12-08 at 22:36 +0530, Manivannan Sadhasivam wrote:
> > Except that the warning will be issued to users after each 10s for 40
> > years.
> > Atleast get rid of that.
>
> how about using dev_warn_once(), instead of dev_warn, using
> dev_warn_once() ensures the warning is issued only once.
>
Sounds good to me.
- Mani
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 953d50cc4256..b2287d2f9bf3 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8205,7 +8205,7 @@ static void ufshcd_update_rtc(struct ufs_hba
> *hba)
> ktime_get_real_ts64(&ts64);
>
> if (ts64.tv_sec < hba->dev_info.rtc_time_baseline) {
> - dev_warn(hba->dev, "%s: Current time precedes previous
> setting!\n", __func__);
> + dev_warn_once(hba->dev, "%s: Current time precedes
> previous setting!\n", __func__);
>
>
>
> Kind regards,
> Bean
--
மணிவண்ணன் சதாசிவம்
On Sun, Dec 10, 2023 at 08:15:15PM +0100, Bean Huo wrote:
> On Fri, 2023-12-08 at 23:01 +0530, Manivannan Sadhasivam wrote:
> > > >
> > > > Thank you for your reviews. I will incorporate the suggested
> > > > changes
> > > > into the patch, addressing all comments except for the RTC mode
> > > > switch.
> > > > The proposal is to perform the RTC mode switch during UFS
> > > > provisioning,
> > > > not at runtime in the UFS online phase. This approach ensures
> > > > that the
> > > > UFS configuration is populated based on the RTC configuration
> > > > established during provisioning. It is advisable not to change
> > > > the RTC
> > > > mode after provisioning is complete. Additionally, the usage of
> > > > tv_sec,
> > > > which returns time elapsed since boot, suggests that there is no
> > > > issue
> > > > with utilizing the RTC in this context.
> > >
> > > Except that the warning will be issued to users after each 10s for
> > > 40 years.
> > > Atleast get rid of that.
> > >
> >
> > I tried this series on Qcom RB5 board and found the issue due to the
> > usage of
> > UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH flag. When this flag is
> > set,
> > ufshcd_device_init() will be called twice due to reinit of the
> > controller and
> > PHY.
> >
> > Since RTC work is initialized and scheduled from
> > ufshcd_device_init(), panic
> > happens during second time. Is it possible to move RTC init outside
> > of
> > ufshcd_device_init(). Maybe you can parse RTC params in
> > ufshcd_device_init()
> > and initialize the work elsewhere. Or you can cancel the work before
> > calling
> > ufshcd_device_init() second time.
> >
> > - Mani
>
>
> Thank you for your review. I have moved the INIT_DELAYED_WORK(&hba-
> >ufs_rtc_update_work, ufshcd_rtc_work) to ufshcd_init() from
> ufs_init_rtc(). This modification has been tested on the Qcom platform.
This works, thanks!
- Mani
> Regarding the warning, instead of removing it entirely, I have switched
> it to dev_dbg. This adjustment is made with the consideration that some
> form of customer notification is still necessary.
>
> changes as below:
>
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 953d50cc4256..cb6b0c286367 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8205,7 +8205,7 @@ static void ufshcd_update_rtc(struct ufs_hba
> *hba)
> ktime_get_real_ts64(&ts64);
>
> if (ts64.tv_sec < hba->dev_info.rtc_time_baseline) {
> - dev_warn(hba->dev, "%s: Current time precedes previous
> setting!\n", __func__);
> + dev_dbg(hba->dev, "%s: Current time precedes previous
> setting!\n", __func__);
> return;
> }
> /*
> @@ -8270,8 +8270,6 @@ static void ufs_init_rtc(struct ufs_hba *hba, u8
> *desc_buf)
> * update work, and let user configure by sysfs node according
> to specific circumstance.
> */
> hba->dev_info.rtc_update_period = 0;
> -
> - INIT_DELAYED_WORK(&hba->ufs_rtc_update_work, ufshcd_rtc_work);
> }
>
> static int ufs_get_device_desc(struct ufs_hba *hba)
> @@ -10634,8 +10632,8 @@ int ufshcd_init(struct ufs_hba *hba, void
> __iomem *mmio_base, unsigned int irq)
> UFS_SLEEP_PWR_MODE,
>
> UIC_LINK_HIBERN8_STATE);
>
> - INIT_DELAYED_WORK(&hba->rpm_dev_flush_recheck_work,
> - ufshcd_rpm_dev_flush_recheck_work);
> + INIT_DELAYED_WORK(&hba->rpm_dev_flush_recheck_work,
> ufshcd_rpm_dev_flush_recheck_work);
> + INIT_DELAYED_WORK(&hba->ufs_rtc_update_work, ufshcd_rtc_work);
>
> /* Set the default auto-hiberate idle timer value to 150 ms */
> if (ufshcd_is_auto_hibern8_supported(hba) && !hba->ahit) {
>
> Kind regards,
> Bean
--
மணிவண்ணன் சதாசிவம்