Message ID | 20231208103940.153734-1-beanhuo@iokpp.de |
---|---|
Headers | show |
Series | Add UFS RTC support | expand |
On 2023-12-08 11:39:37+0100, Bean Huo wrote: > [..] > 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(-) For the full series: Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
On Fri, Dec 08, 2023 at 11:39:39AM +0100, Bean Huo wrote: > From: Bean Huo <beanhuo@micron.com> > > 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 <beanhuo@micron.com> > Signed-off-by: Mike Bi <mikebi@micron.com> > Signed-off-by: Luca Porzio <lporzio@micron.com> > Acked-by: Avri Altman <avri.altman@wdc.com> > --- > 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 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