Hello,
This series has code some cleanups to the Qcom UFS driver. No functional
change. In this version, I've removed code supporting legacy controllers
ver < 2.0, as the respective platforms were never supported in upstream.
Tested on: RB5 development board based on Qcom SM8250 SoC.
- Mani
Changes in v2:
* Collected review tags
* Fixed the comments from Andrew
* Added a few more patches, most notably one removing the code for old
controllers (ver < v2.0)
Manivannan Sadhasivam (17):
scsi: ufs: qcom: Use clk_bulk APIs for managing lane clocks
scsi: ufs: qcom: Fix the return value of ufs_qcom_ice_program_key()
scsi: ufs: qcom: Fix the return value when
platform_get_resource_byname() fails
scsi: ufs: qcom: Remove superfluous variable assignments
scsi: ufs: qcom: Remove the warning message when core_reset is not
available
scsi: ufs: qcom: Export ufshcd_{enable/disable}_irq helpers and make
use of them
scsi: ufs: qcom: Fail ufs_qcom_power_up_sequence() when core_reset
fails
scsi: ufs: qcom: Check the return value of
ufs_qcom_power_up_sequence()
scsi: ufs: qcom: Remove redundant error print for devm_kzalloc()
failure
scsi: ufs: qcom: Use dev_err_probe() to simplify error handling of
devm_gpiod_get_optional()
scsi: ufs: qcom: Remove unused ufs_qcom_hosts struct array
scsi: ufs: qcom: Sort includes alphabetically
scsi: ufs: qcom: Initialize cycles_in_1us variable in
ufs_qcom_set_core_clk_ctrl()
scsi: ufs: qcom: Simplify ufs_qcom_{assert/deassert}_reset
scsi: ufs: qcom: Remove support for host controllers older than v2.0
scsi: ufs: qcom: Use ufshcd_rmwl() where applicable
scsi: ufs: qcom: Remove unused definitions
drivers/ufs/core/ufshcd.c | 6 +-
drivers/ufs/host/ufs-qcom.c | 377 +++++-------------------------------
drivers/ufs/host/ufs-qcom.h | 52 +----
include/ufs/ufshcd.h | 2 +
4 files changed, 66 insertions(+), 371 deletions(-)
--
2.25.1
Lane clock handling can be simplified by using the clk_bulk APIs. So let's
make use of them. This also get's rid of the clock validation in the driver
as kernel should just rely on the firmware (DT/ACPI) to provide the clocks
required for proper functioning.
Reviewed-by: Andrew Halaney <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/host/ufs-qcom.c | 94 ++-----------------------------------
drivers/ufs/host/ufs-qcom.h | 6 +--
2 files changed, 7 insertions(+), 93 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 96cb8b5b4e66..cbb6a696cd97 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -194,52 +194,12 @@ static inline int ufs_qcom_ice_suspend(struct ufs_qcom_host *host)
}
#endif
-static int ufs_qcom_host_clk_get(struct device *dev,
- const char *name, struct clk **clk_out, bool optional)
-{
- struct clk *clk;
- int err = 0;
-
- clk = devm_clk_get(dev, name);
- if (!IS_ERR(clk)) {
- *clk_out = clk;
- return 0;
- }
-
- err = PTR_ERR(clk);
-
- if (optional && err == -ENOENT) {
- *clk_out = NULL;
- return 0;
- }
-
- if (err != -EPROBE_DEFER)
- dev_err(dev, "failed to get %s err %d\n", name, err);
-
- return err;
-}
-
-static int ufs_qcom_host_clk_enable(struct device *dev,
- const char *name, struct clk *clk)
-{
- int err = 0;
-
- err = clk_prepare_enable(clk);
- if (err)
- dev_err(dev, "%s: %s enable failed %d\n", __func__, name, err);
-
- return err;
-}
-
static void ufs_qcom_disable_lane_clks(struct ufs_qcom_host *host)
{
if (!host->is_lane_clks_enabled)
return;
- clk_disable_unprepare(host->tx_l1_sync_clk);
- clk_disable_unprepare(host->tx_l0_sync_clk);
- clk_disable_unprepare(host->rx_l1_sync_clk);
- clk_disable_unprepare(host->rx_l0_sync_clk);
+ clk_bulk_disable_unprepare(host->num_clks, host->clks);
host->is_lane_clks_enabled = false;
}
@@ -247,43 +207,14 @@ static void ufs_qcom_disable_lane_clks(struct ufs_qcom_host *host)
static int ufs_qcom_enable_lane_clks(struct ufs_qcom_host *host)
{
int err;
- struct device *dev = host->hba->dev;
-
- if (host->is_lane_clks_enabled)
- return 0;
- err = ufs_qcom_host_clk_enable(dev, "rx_lane0_sync_clk",
- host->rx_l0_sync_clk);
+ err = clk_bulk_prepare_enable(host->num_clks, host->clks);
if (err)
return err;
- err = ufs_qcom_host_clk_enable(dev, "tx_lane0_sync_clk",
- host->tx_l0_sync_clk);
- if (err)
- goto disable_rx_l0;
-
- err = ufs_qcom_host_clk_enable(dev, "rx_lane1_sync_clk",
- host->rx_l1_sync_clk);
- if (err)
- goto disable_tx_l0;
-
- err = ufs_qcom_host_clk_enable(dev, "tx_lane1_sync_clk",
- host->tx_l1_sync_clk);
- if (err)
- goto disable_rx_l1;
-
host->is_lane_clks_enabled = true;
return 0;
-
-disable_rx_l1:
- clk_disable_unprepare(host->rx_l1_sync_clk);
-disable_tx_l0:
- clk_disable_unprepare(host->tx_l0_sync_clk);
-disable_rx_l0:
- clk_disable_unprepare(host->rx_l0_sync_clk);
-
- return err;
}
static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
@@ -294,26 +225,11 @@ static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
if (has_acpi_companion(dev))
return 0;
- err = ufs_qcom_host_clk_get(dev, "rx_lane0_sync_clk",
- &host->rx_l0_sync_clk, false);
- if (err)
- return err;
-
- err = ufs_qcom_host_clk_get(dev, "tx_lane0_sync_clk",
- &host->tx_l0_sync_clk, false);
- if (err)
+ err = devm_clk_bulk_get_all(dev, &host->clks);
+ if (err <= 0)
return err;
- /* In case of single lane per direction, don't read lane1 clocks */
- if (host->hba->lanes_per_direction > 1) {
- err = ufs_qcom_host_clk_get(dev, "rx_lane1_sync_clk",
- &host->rx_l1_sync_clk, false);
- if (err)
- return err;
-
- err = ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
- &host->tx_l1_sync_clk, true);
- }
+ host->num_clks = err;
return 0;
}
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 9950a0089475..e2df4c528a2a 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -213,10 +213,8 @@ struct ufs_qcom_host {
struct phy *generic_phy;
struct ufs_hba *hba;
struct ufs_pa_layer_attr dev_req_params;
- struct clk *rx_l0_sync_clk;
- struct clk *tx_l0_sync_clk;
- struct clk *rx_l1_sync_clk;
- struct clk *tx_l1_sync_clk;
+ struct clk_bulk_data *clks;
+ u32 num_clks;
bool is_lane_clks_enabled;
struct icc_path *icc_ddr;
--
2.25.1
The return value should be -ENODEV indicating that the resource is not
provided in DT, not -ENOMEM. Fix it!
Fixes: c263b4ef737e ("scsi: ufs: core: mcq: Configure resource regions")
Reviewed-by: Andrew Halaney <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/host/ufs-qcom.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 852179e456f2..778df0a9c65e 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1701,7 +1701,7 @@ static int ufs_qcom_mcq_config_resource(struct ufs_hba *hba)
if (!res->resource) {
dev_info(hba->dev, "Resource %s not provided\n", res->name);
if (i == RES_UFS)
- return -ENOMEM;
+ return -ENODEV;
continue;
} else if (i == RES_UFS) {
res_mem = res->resource;
--
2.25.1
Currently, the function returns -EINVAL if algorithm other than AES-256-XTS
is requested. But the correct error code is -EOPNOTSUPP. Fix it!
Cc: Abel Vesa <[email protected]>
Fixes: 56541c7c4468 ("scsi: ufs: ufs-qcom: Switch to the new ICE API")
Reviewed-by: Abel Vesa <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/host/ufs-qcom.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index cbb6a696cd97..852179e456f2 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -158,7 +158,7 @@ static int ufs_qcom_ice_program_key(struct ufs_hba *hba,
cap = hba->crypto_cap_array[cfg->crypto_cap_idx];
if (cap.algorithm_id != UFS_CRYPTO_ALG_AES_XTS ||
cap.key_size != UFS_CRYPTO_KEY_SIZE_256)
- return -EINVAL;
+ return -EOPNOTSUPP;
if (config_enable)
return qcom_ice_program_key(host->ice,
--
2.25.1
Instead of duplicating the enable/disable IRQ part, let's export the
helpers available in ufshcd driver and make use of them. This also fixes
the possible redundant IRQ disable before asserting reset (when IRQ was
already disabled).
Fixes: 4a791574a0cc ("scsi: ufs: ufs-qcom: Disable interrupt in reset path")
Reviewed-by: Bart Van Assche <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/core/ufshcd.c | 6 ++++--
drivers/ufs/host/ufs-qcom.c | 9 +++------
include/ufs/ufshcd.h | 2 ++
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 8b1031fb0a44..671facc73921 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -289,21 +289,23 @@ static void ufshcd_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba,
static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba);
-static inline void ufshcd_enable_irq(struct ufs_hba *hba)
+void ufshcd_enable_irq(struct ufs_hba *hba)
{
if (!hba->is_irq_enabled) {
enable_irq(hba->irq);
hba->is_irq_enabled = true;
}
}
+EXPORT_SYMBOL_GPL(ufshcd_enable_irq);
-static inline void ufshcd_disable_irq(struct ufs_hba *hba)
+void ufshcd_disable_irq(struct ufs_hba *hba)
{
if (hba->is_irq_enabled) {
disable_irq(hba->irq);
hba->is_irq_enabled = false;
}
}
+EXPORT_SYMBOL_GPL(ufshcd_disable_irq);
static void ufshcd_configure_wb(struct ufs_hba *hba)
{
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index d474de0739e4..604273a22afd 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -300,8 +300,7 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
return 0;
reenable_intr = hba->is_irq_enabled;
- disable_irq(hba->irq);
- hba->is_irq_enabled = false;
+ ufshcd_disable_irq(hba);
ret = reset_control_assert(host->core_reset);
if (ret) {
@@ -324,10 +323,8 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
usleep_range(1000, 1100);
- if (reenable_intr) {
- enable_irq(hba->irq);
- hba->is_irq_enabled = true;
- }
+ if (reenable_intr)
+ ufshcd_enable_irq(hba);
return 0;
}
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 7f0b2c5599cd..f1fc16ac6af2 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1231,6 +1231,8 @@ static inline void ufshcd_rmwl(struct ufs_hba *hba, u32 mask, u32 val, u32 reg)
ufshcd_writel(hba, tmp, reg);
}
+void ufshcd_enable_irq(struct ufs_hba *hba);
+void ufshcd_disable_irq(struct ufs_hba *hba);
int ufshcd_alloc_host(struct device *, struct ufs_hba **);
void ufshcd_dealloc_host(struct ufs_hba *);
int ufshcd_hba_enable(struct ufs_hba *hba);
--
2.25.1
There are many instances where the variable assignments are not needed.
Remove them.
Reviewed-by: Andrew Halaney <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/host/ufs-qcom.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 778df0a9c65e..dc93b1c5ca74 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -219,7 +219,7 @@ static int ufs_qcom_enable_lane_clks(struct ufs_qcom_host *host)
static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
{
- int err = 0;
+ int err;
struct device *dev = host->hba->dev;
if (has_acpi_companion(dev))
@@ -237,7 +237,7 @@ static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
static int ufs_qcom_check_hibern8(struct ufs_hba *hba)
{
int err;
- u32 tx_fsm_val = 0;
+ u32 tx_fsm_val;
unsigned long timeout = jiffies + msecs_to_jiffies(HBRN8_POLL_TOUT_MS);
do {
@@ -292,9 +292,9 @@ static void ufs_qcom_select_unipro_mode(struct ufs_qcom_host *host)
*/
static int ufs_qcom_host_reset(struct ufs_hba *hba)
{
- int ret = 0;
+ int ret;
struct ufs_qcom_host *host = ufshcd_get_variant(hba);
- bool reenable_intr = false;
+ bool reenable_intr;
if (!host->core_reset) {
dev_warn(hba->dev, "%s: reset control not set\n", __func__);
@@ -417,7 +417,7 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
enum ufs_notify_change_status status)
{
struct ufs_qcom_host *host = ufshcd_get_variant(hba);
- int err = 0;
+ int err;
switch (status) {
case PRE_CHANGE:
@@ -463,7 +463,7 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
u32 core_clk_period_in_ns;
u32 tx_clk_cycles_per_us = 0;
unsigned long core_clk_rate = 0;
- u32 core_clk_cycles_per_us = 0;
+ u32 core_clk_cycles_per_us;
static u32 pwm_fr_table[][2] = {
{UFS_PWM_G1, 0x1},
@@ -1418,7 +1418,7 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
bool scale_up, enum ufs_notify_change_status status)
{
struct ufs_qcom_host *host = ufshcd_get_variant(hba);
- int err = 0;
+ int err;
/* check the host controller state before sending hibern8 cmd */
if (!ufshcd_is_hba_active(hba))
@@ -1689,7 +1689,7 @@ static int ufs_qcom_mcq_config_resource(struct ufs_hba *hba)
struct platform_device *pdev = to_platform_device(hba->dev);
struct ufshcd_res_info *res;
struct resource *res_mem, *res_mcq;
- int i, ret = 0;
+ int i, ret;
memcpy(hba->res, ufs_res_info, sizeof(ufs_res_info));
--
2.25.1
Even though core_reset is optional, a failure during assert/deassert should
be considered fatal, if core_reset is available. So fail
ufs_qcom_power_up_sequence() if an error happens during reset and also get
rid of the redundant warning as the ufs_qcom_host_reset() function itself
prints error messages.
Reviewed-by: Andrew Halaney <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/host/ufs-qcom.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 604273a22afd..365a61dbf7ea 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -317,9 +317,11 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
usleep_range(200, 210);
ret = reset_control_deassert(host->core_reset);
- if (ret)
+ if (ret) {
dev_err(hba->dev, "%s: core_reset deassert failed, err = %d\n",
__func__, ret);
+ return ret;
+ }
usleep_range(1000, 1100);
@@ -359,8 +361,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
/* Reset UFS Host Controller and PHY */
ret = ufs_qcom_host_reset(hba);
if (ret)
- dev_warn(hba->dev, "%s: host reset returned %d\n",
- __func__, ret);
+ return ret;
/* phy initialization - calibrate the phy */
ret = phy_init(phy);
--
2.25.1
Sort includes alphabetically.
Reviewed-by: Nitin Rawat <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/host/ufs-qcom.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 18ea41f5bae9..7fbd35a3eb81 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -4,26 +4,26 @@
*/
#include <linux/acpi.h>
-#include <linux/time.h>
#include <linux/clk.h>
#include <linux/delay.h>
+#include <linux/devfreq.h>
+#include <linux/gpio/consumer.h>
#include <linux/interconnect.h>
#include <linux/module.h>
#include <linux/of.h>
-#include <linux/platform_device.h>
#include <linux/phy/phy.h>
-#include <linux/gpio/consumer.h>
+#include <linux/platform_device.h>
#include <linux/reset-controller.h>
-#include <linux/devfreq.h>
+#include <linux/time.h>
#include <soc/qcom/ice.h>
#include <ufs/ufshcd.h>
-#include "ufshcd-pltfrm.h"
-#include <ufs/unipro.h>
-#include "ufs-qcom.h"
#include <ufs/ufshci.h>
#include <ufs/ufs_quirks.h>
+#include <ufs/unipro.h>
+#include "ufshcd-pltfrm.h"
+#include "ufs-qcom.h"
#define MCQ_QCFGPTR_MASK GENMASK(7, 0)
#define MCQ_QCFGPTR_UNIT 0x200
--
2.25.1
ufs_qcom_hosts array is assigned, but not used anywhere. So let's remove
it.
Reviewed-by: Andrew Halaney <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/host/ufs-qcom.c | 5 -----
drivers/ufs/host/ufs-qcom.h | 1 -
2 files changed, 6 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index ae9fafa49ded..18ea41f5bae9 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -90,8 +90,6 @@ static const struct __ufs_qcom_bw_table {
[MODE_MAX][0][0] = { 7643136, 307200 },
};
-static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
-
static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up);
@@ -1194,9 +1192,6 @@ static int ufs_qcom_init(struct ufs_hba *hba)
ufs_qcom_setup_clocks(hba, true, POST_CHANGE);
- if (hba->dev->id < MAX_UFS_QCOM_HOSTS)
- ufs_qcom_hosts[hba->dev->id] = host;
-
ufs_qcom_get_default_testbus_cfg(host);
err = ufs_qcom_testbus_config(host);
if (err)
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index e2df4c528a2a..53db424a0bcb 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -10,7 +10,6 @@
#include <soc/qcom/ice.h>
#include <ufs/ufshcd.h>
-#define MAX_UFS_QCOM_HOSTS 1
#define MAX_U32 (~(u32)0)
#define MPHY_TX_FSM_STATE 0x41
#define TX_FSM_HIBERN8 0x1
--
2.25.1
As done in other places, let's use dev_err_probe() to simplify the error
handling while acquiring the device reset gpio using
devm_gpiod_get_optional().
While at it, let's reword the error message to make it clear that the
failure is due to acquiring "device reset gpio".
Reviewed-by: Andrew Halaney <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/host/ufs-qcom.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 05a9a25bc34c..ae9fafa49ded 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1148,9 +1148,8 @@ static int ufs_qcom_init(struct ufs_hba *hba)
host->device_reset = devm_gpiod_get_optional(dev, "reset",
GPIOD_OUT_HIGH);
if (IS_ERR(host->device_reset)) {
- err = PTR_ERR(host->device_reset);
- if (err != -EPROBE_DEFER)
- dev_err(dev, "failed to acquire reset gpio: %d\n", err);
+ err = dev_err_probe(dev, PTR_ERR(host->device_reset),
+ "Failed to acquire device reset gpio\n");
goto out_variant_clear;
}
--
2.25.1
If ufs_qcom_power_up_sequence() fails, then it makes no sense to enable
the lane clocks and continue ufshcd_hba_enable(). So let's check the return
value of ufs_qcom_power_up_sequence().
Reviewed-by: Andrew Halaney <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/host/ufs-qcom.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 365a61dbf7ea..b141dd2a9346 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -417,7 +417,10 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
switch (status) {
case PRE_CHANGE:
- ufs_qcom_power_up_sequence(hba);
+ err = ufs_qcom_power_up_sequence(hba);
+ if (err)
+ return err;
+
/*
* The PHY PLL output is the source of tx/rx lane symbol
* clocks, hence, enable the lane clocks only after PHY
--
2.25.1
core_reset is optional, so there is no need to warn the user if it is not
available.
Reviewed-by: Andrew Halaney <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/host/ufs-qcom.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index dc93b1c5ca74..d474de0739e4 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -296,10 +296,8 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
struct ufs_qcom_host *host = ufshcd_get_variant(hba);
bool reenable_intr;
- if (!host->core_reset) {
- dev_warn(hba->dev, "%s: reset control not set\n", __func__);
+ if (!host->core_reset)
return 0;
- }
reenable_intr = hba->is_irq_enabled;
disable_irq(hba->irq);
--
2.25.1
devm_kzalloc() will itself print the error message on failure. So let's get
rid of the redundant error message in ufs_qcom_init().
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/host/ufs-qcom.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index b141dd2a9346..05a9a25bc34c 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1109,10 +1109,8 @@ static int ufs_qcom_init(struct ufs_hba *hba)
struct ufs_clk_info *clki;
host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
- if (!host) {
- dev_err(dev, "%s: no memory for qcom ufs host\n", __func__);
+ if (!host)
return -ENOMEM;
- }
/* Make a two way bind between the qcom host and the hba */
host->hba = hba;
--
2.25.1
In both the functions, UFS_PHY_SOFT_RESET contains the mask of the reset
bit. So this can be passed directly as the value to be written for
asserting the reset. For deasserting, 0 can be passed.
This gets rid of the FIELD_PREP() inside these functions and also
UFS_PHY_RESET_{ENABLE/DISABLE} definitions.
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/host/ufs-qcom.h | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 53db424a0bcb..a109d3359db4 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -92,9 +92,6 @@ enum {
#define TEST_BUS_SEL GENMASK(22, 19)
#define UFS_REG_TEST_BUS_EN BIT(30)
-#define UFS_PHY_RESET_ENABLE 1
-#define UFS_PHY_RESET_DISABLE 0
-
/* bit definitions for REG_UFS_CFG2 register */
#define UAWM_HW_CGC_EN BIT(0)
#define UARM_HW_CGC_EN BIT(1)
@@ -157,8 +154,7 @@ ufs_qcom_get_controller_revision(struct ufs_hba *hba,
static inline void ufs_qcom_assert_reset(struct ufs_hba *hba)
{
- ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, FIELD_PREP(UFS_PHY_SOFT_RESET, UFS_PHY_RESET_ENABLE),
- REG_UFS_CFG1);
+ ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, UFS_PHY_SOFT_RESET, REG_UFS_CFG1);
/*
* Make sure assertion of ufs phy reset is written to
@@ -169,8 +165,7 @@ static inline void ufs_qcom_assert_reset(struct ufs_hba *hba)
static inline void ufs_qcom_deassert_reset(struct ufs_hba *hba)
{
- ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, FIELD_PREP(UFS_PHY_SOFT_RESET, UFS_PHY_RESET_DISABLE),
- REG_UFS_CFG1);
+ ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, 0, REG_UFS_CFG1);
/*
* Make sure de-assertion of ufs phy reset is written to
--
2.25.1
In case the "core_clk_unipro" clock is not provided, "cycles_in_1us"
variable will be used as uninitialized. So initialize it with 0.
Issue reported by Smatch tool:
drivers/ufs/host/ufs-qcom.c:1336 ufs_qcom_set_core_clk_ctrl() error: uninitialized symbol 'cycles_in_1us'.
drivers/ufs/host/ufs-qcom.c:1341 ufs_qcom_set_core_clk_ctrl() error: uninitialized symbol 'cycles_in_1us'.
Reviewed-by: Nitin Rawat <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/host/ufs-qcom.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 7fbd35a3eb81..9b3d6d3609c9 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1298,7 +1298,7 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
struct ufs_qcom_host *host = ufshcd_get_variant(hba);
struct list_head *head = &hba->clk_list_head;
struct ufs_clk_info *clki;
- u32 cycles_in_1us;
+ u32 cycles_in_1us = 0;
u32 core_clk_ctrl_reg;
int err;
--
2.25.1
Instead of using both ufshcd_readl() and ufshcd_writel() to read/modify/
write a register, let's make use of the existing helper.
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/host/ufs-qcom.c | 12 ++++--------
drivers/ufs/host/ufs-qcom.h | 3 +++
2 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 26aa8904c823..549a08645391 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -387,9 +387,8 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
*/
static void ufs_qcom_enable_hw_clk_gating(struct ufs_hba *hba)
{
- ufshcd_writel(hba,
- ufshcd_readl(hba, REG_UFS_CFG2) | REG_UFS_CFG2_CGC_EN_ALL,
- REG_UFS_CFG2);
+ ufshcd_rmwl(hba, REG_UFS_CFG2_CGC_EN_ALL, REG_UFS_CFG2_CGC_EN_ALL,
+ REG_UFS_CFG2);
/* Ensure that HW clock gating is enabled before next operations */
mb();
@@ -1689,11 +1688,8 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
platform_msi_domain_free_irqs(hba->dev);
} else {
if (host->hw_ver.major == 6 && host->hw_ver.minor == 0 &&
- host->hw_ver.step == 0) {
- ufshcd_writel(hba,
- ufshcd_readl(hba, REG_UFS_CFG3) | 0x1F000,
- REG_UFS_CFG3);
- }
+ host->hw_ver.step == 0)
+ ufshcd_rmwl(hba, ESI_VEC_MASK, 0x1f00, REG_UFS_CFG3);
ufshcd_mcq_enable_esi(hba);
}
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 385480499e71..2ce63a1c7f2f 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -102,6 +102,9 @@ enum {
#define TMRLUT_HW_CGC_EN BIT(6)
#define OCSC_HW_CGC_EN BIT(7)
+/* bit definitions for REG_UFS_CFG3 register */
+#define ESI_VEC_MASK GENMASK(22, 12)
+
/* bit definitions for REG_UFS_PARAM0 */
#define MAX_HS_GEAR_MASK GENMASK(6, 4)
#define UFS_QCOM_MAX_GEAR(x) FIELD_GET(MAX_HS_GEAR_MASK, (x))
--
2.25.1
Remove unused definitions.
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/host/ufs-qcom.h | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 2ce63a1c7f2f..cdceeb795e70 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -10,22 +10,16 @@
#include <soc/qcom/ice.h>
#include <ufs/ufshcd.h>
-#define MAX_U32 (~(u32)0)
#define MPHY_TX_FSM_STATE 0x41
#define TX_FSM_HIBERN8 0x1
#define HBRN8_POLL_TOUT_MS 100
#define DEFAULT_CLK_RATE_HZ 1000000
-#define BUS_VECTOR_NAME_LEN 32
#define MAX_SUPP_MAC 64
#define UFS_HW_VER_MAJOR_MASK GENMASK(31, 28)
#define UFS_HW_VER_MINOR_MASK GENMASK(27, 16)
#define UFS_HW_VER_STEP_MASK GENMASK(15, 0)
-/* vendor specific pre-defined parameters */
-#define SLOW 1
-#define FAST 2
-
#define UFS_QCOM_LIMIT_HS_RATE PA_HS_MODE_B
/* QCOM UFS host controller vendor specific registers */
--
2.25.1
The legacy platforms making use of host controllers older than version 2.0
are not supported in upstream. So there is no need to carry code to support
them.
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/ufs/host/ufs-qcom.c | 196 ++----------------------------------
drivers/ufs/host/ufs-qcom.h | 27 -----
2 files changed, 11 insertions(+), 212 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 9b3d6d3609c9..26aa8904c823 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -274,9 +274,7 @@ static int ufs_qcom_check_hibern8(struct ufs_hba *hba)
static void ufs_qcom_select_unipro_mode(struct ufs_qcom_host *host)
{
- ufshcd_rmwl(host->hba, QUNIPRO_SEL,
- ufs_qcom_cap_qunipro(host) ? QUNIPRO_SEL : 0,
- REG_UFS_CFG1);
+ ufshcd_rmwl(host->hba, QUNIPRO_SEL, QUNIPRO_SEL, REG_UFS_CFG1);
if (host->hw_ver.major >= 0x05)
ufshcd_rmwl(host->hba, QUNIPRO_G4_SEL, 0, REG_UFS_CFG0);
@@ -333,18 +331,8 @@ static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba)
{
struct ufs_qcom_host *host = ufshcd_get_variant(hba);
- if (host->hw_ver.major == 0x1) {
- /*
- * HS-G3 operations may not reliably work on legacy QCOM
- * UFS host controller hardware even though capability
- * exchange during link startup phase may end up
- * negotiating maximum supported gear as G3.
- * Hence downgrade the maximum supported gear to HS-G2.
- */
- return UFS_HS_G2;
- } else if (host->hw_ver.major >= 0x4) {
+ if (host->hw_ver.major >= 0x4)
return UFS_QCOM_MAX_GEAR(ufshcd_readl(hba, REG_UFS_PARAM0));
- }
/* Default is HS-G3 */
return UFS_HS_G3;
@@ -457,41 +445,16 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
{
struct ufs_qcom_host *host = ufshcd_get_variant(hba);
struct ufs_clk_info *clki;
- u32 core_clk_period_in_ns;
- u32 tx_clk_cycles_per_us = 0;
unsigned long core_clk_rate = 0;
u32 core_clk_cycles_per_us;
- static u32 pwm_fr_table[][2] = {
- {UFS_PWM_G1, 0x1},
- {UFS_PWM_G2, 0x1},
- {UFS_PWM_G3, 0x1},
- {UFS_PWM_G4, 0x1},
- };
-
- static u32 hs_fr_table_rA[][2] = {
- {UFS_HS_G1, 0x1F},
- {UFS_HS_G2, 0x3e},
- {UFS_HS_G3, 0x7D},
- };
-
- static u32 hs_fr_table_rB[][2] = {
- {UFS_HS_G1, 0x24},
- {UFS_HS_G2, 0x49},
- {UFS_HS_G3, 0x92},
- };
-
/*
- * The Qunipro controller does not use following registers:
- * SYS1CLK_1US_REG, TX_SYMBOL_CLK_1US_REG, CLK_NS_REG &
- * UFS_REG_PA_LINK_STARTUP_TIMER.
- * However UTP controller uses SYS1CLK_1US_REG register for Interrupt
+ * UTP controller uses SYS1CLK_1US_REG register for Interrupt
* Aggregation logic.
* It is mandatory to write SYS1CLK_1US_REG register on UFS host
* controller V4.0.0 onwards.
*/
- if (host->hw_ver.major < 4 && ufs_qcom_cap_qunipro(host) &&
- !ufshcd_is_intr_aggr_allowed(hba))
+ if (host->hw_ver.major < 4 && !ufshcd_is_intr_aggr_allowed(hba))
return 0;
if (gear == 0) {
@@ -524,79 +487,6 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
mb();
}
- if (ufs_qcom_cap_qunipro(host))
- return 0;
-
- core_clk_period_in_ns = NSEC_PER_SEC / core_clk_rate;
- core_clk_period_in_ns <<= OFFSET_CLK_NS_REG;
- core_clk_period_in_ns &= MASK_CLK_NS_REG;
-
- switch (hs) {
- case FASTAUTO_MODE:
- case FAST_MODE:
- if (rate == PA_HS_MODE_A) {
- if (gear > ARRAY_SIZE(hs_fr_table_rA)) {
- dev_err(hba->dev,
- "%s: index %d exceeds table size %zu\n",
- __func__, gear,
- ARRAY_SIZE(hs_fr_table_rA));
- return -EINVAL;
- }
- tx_clk_cycles_per_us = hs_fr_table_rA[gear-1][1];
- } else if (rate == PA_HS_MODE_B) {
- if (gear > ARRAY_SIZE(hs_fr_table_rB)) {
- dev_err(hba->dev,
- "%s: index %d exceeds table size %zu\n",
- __func__, gear,
- ARRAY_SIZE(hs_fr_table_rB));
- return -EINVAL;
- }
- tx_clk_cycles_per_us = hs_fr_table_rB[gear-1][1];
- } else {
- dev_err(hba->dev, "%s: invalid rate = %d\n",
- __func__, rate);
- return -EINVAL;
- }
- break;
- case SLOWAUTO_MODE:
- case SLOW_MODE:
- if (gear > ARRAY_SIZE(pwm_fr_table)) {
- dev_err(hba->dev,
- "%s: index %d exceeds table size %zu\n",
- __func__, gear,
- ARRAY_SIZE(pwm_fr_table));
- return -EINVAL;
- }
- tx_clk_cycles_per_us = pwm_fr_table[gear-1][1];
- break;
- case UNCHANGED:
- default:
- dev_err(hba->dev, "%s: invalid mode = %d\n", __func__, hs);
- return -EINVAL;
- }
-
- if (ufshcd_readl(hba, REG_UFS_TX_SYMBOL_CLK_NS_US) !=
- (core_clk_period_in_ns | tx_clk_cycles_per_us)) {
- /* this register 2 fields shall be written at once */
- ufshcd_writel(hba, core_clk_period_in_ns | tx_clk_cycles_per_us,
- REG_UFS_TX_SYMBOL_CLK_NS_US);
- /*
- * make sure above write gets applied before we return from
- * this function.
- */
- mb();
- }
-
- if (update_link_startup_timer && host->hw_ver.major != 0x5) {
- ufshcd_writel(hba, ((core_clk_rate / MSEC_PER_SEC) * 100),
- REG_UFS_CFG0);
- /*
- * make sure that this configuration is applied before
- * we return
- */
- mb();
- }
-
return 0;
}
@@ -604,7 +494,6 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
enum ufs_notify_change_status status)
{
int err = 0;
- struct ufs_qcom_host *host = ufshcd_get_variant(hba);
switch (status) {
case PRE_CHANGE:
@@ -615,11 +504,9 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
return -EINVAL;
}
- if (ufs_qcom_cap_qunipro(host)) {
- err = ufs_qcom_set_core_clk_ctrl(hba, true);
- if (err)
- dev_err(hba->dev, "cfg core clk ctrl failed\n");
- }
+ err = ufs_qcom_set_core_clk_ctrl(hba, true);
+ if (err)
+ dev_err(hba->dev, "cfg core clk ctrl failed\n");
/*
* Some UFS devices (and may be host) have issues if LCC is
* enabled. So we are setting PA_Local_TX_LCC_Enable to 0
@@ -918,12 +805,7 @@ static int ufs_qcom_apply_dev_quirks(struct ufs_hba *hba)
static u32 ufs_qcom_get_ufs_hci_version(struct ufs_hba *hba)
{
- struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-
- if (host->hw_ver.major == 0x1)
- return ufshci_version(1, 1);
- else
- return ufshci_version(2, 0);
+ return ufshci_version(2, 0);
}
/**
@@ -939,46 +821,21 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba)
{
struct ufs_qcom_host *host = ufshcd_get_variant(hba);
- if (host->hw_ver.major == 0x01) {
- hba->quirks |= UFSHCD_QUIRK_DELAY_BEFORE_DME_CMDS
- | UFSHCD_QUIRK_BROKEN_PA_RXHSUNTERMCAP
- | UFSHCD_QUIRK_DME_PEER_ACCESS_AUTO_MODE;
-
- if (host->hw_ver.minor == 0x0001 && host->hw_ver.step == 0x0001)
- hba->quirks |= UFSHCD_QUIRK_BROKEN_INTR_AGGR;
-
- hba->quirks |= UFSHCD_QUIRK_BROKEN_LCC;
- }
-
- if (host->hw_ver.major == 0x2) {
+ if (host->hw_ver.major == 0x2)
hba->quirks |= UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION;
- if (!ufs_qcom_cap_qunipro(host))
- /* Legacy UniPro mode still need following quirks */
- hba->quirks |= (UFSHCD_QUIRK_DELAY_BEFORE_DME_CMDS
- | UFSHCD_QUIRK_DME_PEER_ACCESS_AUTO_MODE
- | UFSHCD_QUIRK_BROKEN_PA_RXHSUNTERMCAP);
- }
-
if (host->hw_ver.major > 0x3)
hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH;
}
static void ufs_qcom_set_caps(struct ufs_hba *hba)
{
- struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-
hba->caps |= UFSHCD_CAP_CLK_GATING | UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
hba->caps |= UFSHCD_CAP_CLK_SCALING | UFSHCD_CAP_WB_WITH_CLK_SCALING;
hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
hba->caps |= UFSHCD_CAP_WB_EN;
hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
-
- if (host->hw_ver.major >= 0x2) {
- host->caps = UFS_QCOM_CAP_QUNIPRO |
- UFS_QCOM_CAP_RETAIN_SEC_CFG_AFTER_PWR_COLLAPSE;
- }
}
/**
@@ -1101,9 +958,7 @@ static int ufs_qcom_init(struct ufs_hba *hba)
{
int err;
struct device *dev = hba->dev;
- struct platform_device *pdev = to_platform_device(dev);
struct ufs_qcom_host *host;
- struct resource *res;
struct ufs_clk_info *clki;
host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
@@ -1154,25 +1009,8 @@ static int ufs_qcom_init(struct ufs_hba *hba)
ufs_qcom_get_controller_revision(hba, &host->hw_ver.major,
&host->hw_ver.minor, &host->hw_ver.step);
- /*
- * for newer controllers, device reference clock control bit has
- * moved inside UFS controller register address space itself.
- */
- if (host->hw_ver.major >= 0x02) {
- host->dev_ref_clk_ctrl_mmio = hba->mmio_base + REG_UFS_CFG1;
- host->dev_ref_clk_en_mask = BIT(26);
- } else {
- /* "dev_ref_clk_ctrl_mem" is optional resource */
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
- "dev_ref_clk_ctrl_mem");
- if (res) {
- host->dev_ref_clk_ctrl_mmio =
- devm_ioremap_resource(dev, res);
- if (IS_ERR(host->dev_ref_clk_ctrl_mmio))
- host->dev_ref_clk_ctrl_mmio = NULL;
- host->dev_ref_clk_en_mask = BIT(5);
- }
- }
+ host->dev_ref_clk_ctrl_mmio = hba->mmio_base + REG_UFS_CFG1;
+ host->dev_ref_clk_en_mask = BIT(26);
list_for_each_entry(clki, &hba->clk_list_head, list) {
if (!strcmp(clki->name, "core_clk_unipro"))
@@ -1351,9 +1189,6 @@ static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba)
struct ufs_pa_layer_attr *attr = &host->dev_req_params;
int ret;
- if (!ufs_qcom_cap_qunipro(host))
- return 0;
-
ret = ufs_qcom_cfg_timers(hba, attr->gear_rx, attr->pwr_rx,
attr->hs_rate, false, true);
if (ret) {
@@ -1371,13 +1206,9 @@ static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
{
- struct ufs_qcom_host *host = ufshcd_get_variant(hba);
int err;
u32 core_clk_ctrl_reg;
- if (!ufs_qcom_cap_qunipro(host))
- return 0;
-
err = ufshcd_dme_get(hba,
UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
&core_clk_ctrl_reg);
@@ -1396,11 +1227,6 @@ static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
{
- struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-
- if (!ufs_qcom_cap_qunipro(host))
- return 0;
-
/* set unipro core clock attributes and clear clock divider */
return ufs_qcom_set_core_clk_ctrl(hba, false);
}
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index a109d3359db4..385480499e71 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -114,13 +114,6 @@ enum {
DFC_HW_CGC_EN | TRLUT_HW_CGC_EN |\
TMRLUT_HW_CGC_EN | OCSC_HW_CGC_EN)
-/* bit offset */
-#define OFFSET_CLK_NS_REG 0xa
-
-/* bit masks */
-#define MASK_TX_SYMBOL_CLK_1US_REG GENMASK(9, 0)
-#define MASK_CLK_NS_REG GENMASK(23, 10)
-
/* QUniPro Vendor specific attributes */
#define PA_VS_CONFIG_REG1 0x9000
#define DME_VS_CORE_CLK_CTRL 0xD002
@@ -189,21 +182,6 @@ struct ufs_qcom_testbus {
struct gpio_desc;
struct ufs_qcom_host {
- /*
- * Set this capability if host controller supports the QUniPro mode
- * and if driver wants the Host controller to operate in QUniPro mode.
- * Note: By default this capability will be kept enabled if host
- * controller supports the QUniPro mode.
- */
- #define UFS_QCOM_CAP_QUNIPRO 0x1
-
- /*
- * Set this capability if host controller can retain the secure
- * configuration even after UFS controller core power collapse.
- */
- #define UFS_QCOM_CAP_RETAIN_SEC_CFG_AFTER_PWR_COLLAPSE 0x2
- u32 caps;
-
struct phy *generic_phy;
struct ufs_hba *hba;
struct ufs_pa_layer_attr dev_req_params;
@@ -253,9 +231,4 @@ ufs_qcom_get_debug_reg_offset(struct ufs_qcom_host *host, u32 reg)
int ufs_qcom_testbus_config(struct ufs_qcom_host *host);
-static inline bool ufs_qcom_cap_qunipro(struct ufs_qcom_host *host)
-{
- return host->caps & UFS_QCOM_CAP_QUNIPRO;
-}
-
#endif /* UFS_QCOM_H_ */
--
2.25.1
On 12/8/2023 12:28 PM, Manivannan Sadhasivam wrote:
> core_reset is optional, so there is no need to warn the user if it is not
> available.
>
> Reviewed-by: Andrew Halaney <[email protected]>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> ---
> drivers/ufs/host/ufs-qcom.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index dc93b1c5ca74..d474de0739e4 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -296,10 +296,8 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
> struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> bool reenable_intr;
>
> - if (!host->core_reset) {
> - dev_warn(hba->dev, "%s: reset control not set\n", __func__);
> + if (!host->core_reset)
> return 0;
> - }
>
> reenable_intr = hba->is_irq_enabled;
> disable_irq(hba->irq);
Hi Mani,
I think core reset is not frequent. It happen during only probe ,error
handler.
core reset is needed in kernel to cleanup UFS phy and controller
configuration before UFS HLOS operation starts as per HPG.
Having existing warn print can be used to to debug or atleast know
core_reset is missed in device tree to give indication complete reset
hasn't been done and we could still be operating in bootloader
configuration.
Regards,
Nitin
On 12/8/2023 12:28 PM, Manivannan Sadhasivam wrote:
> devm_kzalloc() will itself print the error message on failure. So let's get
> rid of the redundant error message in ufs_qcom_init().
>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> ---
> drivers/ufs/host/ufs-qcom.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index b141dd2a9346..05a9a25bc34c 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1109,10 +1109,8 @@ static int ufs_qcom_init(struct ufs_hba *hba)
> struct ufs_clk_info *clki;
>
> host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
> - if (!host) {
> - dev_err(dev, "%s: no memory for qcom ufs host\n", __func__);
> + if (!host)
> return -ENOMEM;
> - }
>
> /* Make a two way bind between the qcom host and the hba */
> host->hba = hba;
Reviewed-by: Nitin Rawat <[email protected]>
On Fri, Dec 08, 2023 at 02:55:21PM +0530, Nitin Rawat wrote:
>
>
> On 12/8/2023 12:28 PM, Manivannan Sadhasivam wrote:
> > core_reset is optional, so there is no need to warn the user if it is not
> > available.
> >
> > Reviewed-by: Andrew Halaney <[email protected]>
> > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > ---
> > drivers/ufs/host/ufs-qcom.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > index dc93b1c5ca74..d474de0739e4 100644
> > --- a/drivers/ufs/host/ufs-qcom.c
> > +++ b/drivers/ufs/host/ufs-qcom.c
> > @@ -296,10 +296,8 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
> > struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> > bool reenable_intr;
> > - if (!host->core_reset) {
> > - dev_warn(hba->dev, "%s: reset control not set\n", __func__);
> > + if (!host->core_reset)
> > return 0;
> > - }
> > reenable_intr = hba->is_irq_enabled;
> > disable_irq(hba->irq);
>
>
> Hi Mani,
>
> I think core reset is not frequent. It happen during only probe ,error
> handler.
>
> core reset is needed in kernel to cleanup UFS phy and controller
> configuration before UFS HLOS operation starts as per HPG.
>
This sounds like core reset is not an optional property but a required one. I
just checked the upstream DT files for all SoCs, and looks like pretty much all
of them support core reset.
Only MSM8996 doesn't have the reset property, but the reset is available in GCC.
So we should be able to use it in dtsi.
I also skimmed through the HPG and looks like core reset is not optional. Please
confirm.
- Mani
> Having existing warn print can be used to to debug or atleast know
> core_reset is missed in device tree to give indication complete reset hasn't
> been done and we could still be operating in bootloader configuration.
>
>
> Regards,
> Nitin
>
--
மணிவண்ணன் சதாசிவம்
On 12/8/2023 3:58 PM, Manivannan Sadhasivam wrote:
> On Fri, Dec 08, 2023 at 02:55:21PM +0530, Nitin Rawat wrote:
>>
>>
>> On 12/8/2023 12:28 PM, Manivannan Sadhasivam wrote:
>>> core_reset is optional, so there is no need to warn the user if it is not
>>> available.
>>>
>>> Reviewed-by: Andrew Halaney <[email protected]>
>>> Signed-off-by: Manivannan Sadhasivam <[email protected]>
>>> ---
>>> drivers/ufs/host/ufs-qcom.c | 4 +---
>>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>> index dc93b1c5ca74..d474de0739e4 100644
>>> --- a/drivers/ufs/host/ufs-qcom.c
>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>> @@ -296,10 +296,8 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
>>> struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>>> bool reenable_intr;
>>> - if (!host->core_reset) {
>>> - dev_warn(hba->dev, "%s: reset control not set\n", __func__);
>>> + if (!host->core_reset)
>>> return 0;
>>> - }
>>> reenable_intr = hba->is_irq_enabled;
>>> disable_irq(hba->irq);
>>
>>
>> Hi Mani,
>>
>> I think core reset is not frequent. It happen during only probe ,error
>> handler.
>>
>> core reset is needed in kernel to cleanup UFS phy and controller
>> configuration before UFS HLOS operation starts as per HPG.
>>
>
> This sounds like core reset is not an optional property but a required one. I
> just checked the upstream DT files for all SoCs, and looks like pretty much all
> of them support core reset.
>
> Only MSM8996 doesn't have the reset property, but the reset is available in GCC.
> So we should be able to use it in dtsi.
>
> I also skimmed through the HPG and looks like core reset is not optional. Please
> confirm.
>
> - Mani
Hi Mani,
Yes Core_reset is part of HPG sequence and is needed.
Regards,
Nitin
>
>> Having existing warn print can be used to to debug or atleast know
>> core_reset is missed in device tree to give indication complete reset hasn't
>> been done and we could still be operating in bootloader configuration.
>>
>>
>> Regards,
>> Nitin
>>
>
On Fri, Dec 08, 2023 at 12:28:59PM +0530, Manivannan Sadhasivam wrote:
> In both the functions, UFS_PHY_SOFT_RESET contains the mask of the reset
> bit. So this can be passed directly as the value to be written for
> asserting the reset. For deasserting, 0 can be passed.
>
> This gets rid of the FIELD_PREP() inside these functions and also
> UFS_PHY_RESET_{ENABLE/DISABLE} definitions.
>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Andrew Halaney <[email protected]>
> ---
> drivers/ufs/host/ufs-qcom.h | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 53db424a0bcb..a109d3359db4 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -92,9 +92,6 @@ enum {
> #define TEST_BUS_SEL GENMASK(22, 19)
> #define UFS_REG_TEST_BUS_EN BIT(30)
>
> -#define UFS_PHY_RESET_ENABLE 1
> -#define UFS_PHY_RESET_DISABLE 0
> -
> /* bit definitions for REG_UFS_CFG2 register */
> #define UAWM_HW_CGC_EN BIT(0)
> #define UARM_HW_CGC_EN BIT(1)
> @@ -157,8 +154,7 @@ ufs_qcom_get_controller_revision(struct ufs_hba *hba,
>
> static inline void ufs_qcom_assert_reset(struct ufs_hba *hba)
> {
> - ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, FIELD_PREP(UFS_PHY_SOFT_RESET, UFS_PHY_RESET_ENABLE),
> - REG_UFS_CFG1);
> + ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, UFS_PHY_SOFT_RESET, REG_UFS_CFG1);
>
> /*
> * Make sure assertion of ufs phy reset is written to
> @@ -169,8 +165,7 @@ static inline void ufs_qcom_assert_reset(struct ufs_hba *hba)
>
> static inline void ufs_qcom_deassert_reset(struct ufs_hba *hba)
> {
> - ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, FIELD_PREP(UFS_PHY_SOFT_RESET, UFS_PHY_RESET_DISABLE),
> - REG_UFS_CFG1);
> + ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, 0, REG_UFS_CFG1);
>
> /*
> * Make sure de-assertion of ufs phy reset is written to
> --
> 2.25.1
>
On Fri, Dec 08, 2023 at 12:29:01PM +0530, Manivannan Sadhasivam wrote:
> Instead of using both ufshcd_readl() and ufshcd_writel() to read/modify/
> write a register, let's make use of the existing helper.
>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> ---
> drivers/ufs/host/ufs-qcom.c | 12 ++++--------
> drivers/ufs/host/ufs-qcom.h | 3 +++
> 2 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 26aa8904c823..549a08645391 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -387,9 +387,8 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> */
> static void ufs_qcom_enable_hw_clk_gating(struct ufs_hba *hba)
> {
> - ufshcd_writel(hba,
> - ufshcd_readl(hba, REG_UFS_CFG2) | REG_UFS_CFG2_CGC_EN_ALL,
> - REG_UFS_CFG2);
> + ufshcd_rmwl(hba, REG_UFS_CFG2_CGC_EN_ALL, REG_UFS_CFG2_CGC_EN_ALL,
> + REG_UFS_CFG2);
>
> /* Ensure that HW clock gating is enabled before next operations */
> mb();
> @@ -1689,11 +1688,8 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
> platform_msi_domain_free_irqs(hba->dev);
> } else {
> if (host->hw_ver.major == 6 && host->hw_ver.minor == 0 &&
> - host->hw_ver.step == 0) {
> - ufshcd_writel(hba,
> - ufshcd_readl(hba, REG_UFS_CFG3) | 0x1F000,
> - REG_UFS_CFG3);
> - }
> + host->hw_ver.step == 0)
> + ufshcd_rmwl(hba, ESI_VEC_MASK, 0x1f00, REG_UFS_CFG3);
Is this right? I feel like you accidentally just shifted what was written
prior >> 4 bits.
Before: 0x1f000
After: 0x01f00
> ufshcd_mcq_enable_esi(hba);
> }
>
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 385480499e71..2ce63a1c7f2f 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -102,6 +102,9 @@ enum {
> #define TMRLUT_HW_CGC_EN BIT(6)
> #define OCSC_HW_CGC_EN BIT(7)
>
> +/* bit definitions for REG_UFS_CFG3 register */
> +#define ESI_VEC_MASK GENMASK(22, 12)
> +
I'll leave it to someone with the docs to review this field. It at least
contains the bits set above, fwiw. It would be neat to see that
converted to using the field + a FIELD_PREP to set the bits IMO, but I
honestly don't have a lot of experience using those APIs so feel free to
reject that.
> /* bit definitions for REG_UFS_PARAM0 */
> #define MAX_HS_GEAR_MASK GENMASK(6, 4)
> #define UFS_QCOM_MAX_GEAR(x) FIELD_GET(MAX_HS_GEAR_MASK, (x))
> --
> 2.25.1
>
On Fri, Dec 08, 2023 at 12:29:02PM +0530, Manivannan Sadhasivam wrote:
> Remove unused definitions.
>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Andrew Halaney <[email protected]>
> ---
> drivers/ufs/host/ufs-qcom.h | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 2ce63a1c7f2f..cdceeb795e70 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -10,22 +10,16 @@
> #include <soc/qcom/ice.h>
> #include <ufs/ufshcd.h>
>
> -#define MAX_U32 (~(u32)0)
> #define MPHY_TX_FSM_STATE 0x41
> #define TX_FSM_HIBERN8 0x1
> #define HBRN8_POLL_TOUT_MS 100
> #define DEFAULT_CLK_RATE_HZ 1000000
> -#define BUS_VECTOR_NAME_LEN 32
> #define MAX_SUPP_MAC 64
>
> #define UFS_HW_VER_MAJOR_MASK GENMASK(31, 28)
> #define UFS_HW_VER_MINOR_MASK GENMASK(27, 16)
> #define UFS_HW_VER_STEP_MASK GENMASK(15, 0)
>
> -/* vendor specific pre-defined parameters */
> -#define SLOW 1
> -#define FAST 2
> -
> #define UFS_QCOM_LIMIT_HS_RATE PA_HS_MODE_B
>
> /* QCOM UFS host controller vendor specific registers */
> --
> 2.25.1
>
On Fri, Dec 08, 2023 at 12:28:45PM +0530, Manivannan Sadhasivam wrote:
> Hello,
>
> This series has code some cleanups to the Qcom UFS driver. No functional
> change. In this version, I've removed code supporting legacy controllers
> ver < 2.0, as the respective platforms were never supported in upstream.
>
> Tested on: RB5 development board based on Qcom SM8250 SoC.
>
> - Mani
>
> Changes in v2:
>
> * Collected review tags
> * Fixed the comments from Andrew
> * Added a few more patches, most notably one removing the code for old
> controllers (ver < v2.0)
>
I took this for a spin on sa8775p-ride when developing another patch
today with no issues. Certainly doesn't hit all the cases here, but:
Tested-by: Andrew Halaney <[email protected]> # sa8775p-ride
> Manivannan Sadhasivam (17):
> scsi: ufs: qcom: Use clk_bulk APIs for managing lane clocks
> scsi: ufs: qcom: Fix the return value of ufs_qcom_ice_program_key()
> scsi: ufs: qcom: Fix the return value when
> platform_get_resource_byname() fails
> scsi: ufs: qcom: Remove superfluous variable assignments
> scsi: ufs: qcom: Remove the warning message when core_reset is not
> available
> scsi: ufs: qcom: Export ufshcd_{enable/disable}_irq helpers and make
> use of them
> scsi: ufs: qcom: Fail ufs_qcom_power_up_sequence() when core_reset
> fails
> scsi: ufs: qcom: Check the return value of
> ufs_qcom_power_up_sequence()
> scsi: ufs: qcom: Remove redundant error print for devm_kzalloc()
> failure
> scsi: ufs: qcom: Use dev_err_probe() to simplify error handling of
> devm_gpiod_get_optional()
> scsi: ufs: qcom: Remove unused ufs_qcom_hosts struct array
> scsi: ufs: qcom: Sort includes alphabetically
> scsi: ufs: qcom: Initialize cycles_in_1us variable in
> ufs_qcom_set_core_clk_ctrl()
> scsi: ufs: qcom: Simplify ufs_qcom_{assert/deassert}_reset
> scsi: ufs: qcom: Remove support for host controllers older than v2.0
> scsi: ufs: qcom: Use ufshcd_rmwl() where applicable
> scsi: ufs: qcom: Remove unused definitions
>
> drivers/ufs/core/ufshcd.c | 6 +-
> drivers/ufs/host/ufs-qcom.c | 377 +++++-------------------------------
> drivers/ufs/host/ufs-qcom.h | 52 +----
> include/ufs/ufshcd.h | 2 +
> 4 files changed, 66 insertions(+), 371 deletions(-)
>
> --
> 2.25.1
>
On 8.12.2023 07:58, Manivannan Sadhasivam wrote:
> Hello,
>
> This series has code some cleanups to the Qcom UFS driver. No functional
> change. In this version, I've removed code supporting legacy controllers
> ver < 2.0, as the respective platforms were never supported in upstream.
>
> Tested on: RB5 development board based on Qcom SM8250 SoC.
>
> - Mani
>
> Changes in v2:
>
> * Collected review tags
> * Fixed the comments from Andrew
> * Added a few more patches, most notably one removing the code for old
> controllers (ver < v2.0)
FWIW i found this snipped from a downstream commit from 2014:
8084 : 1.1.1
8994v1 : 1.2.0
8994v2 : 1.3.0
I'm yet to see any 8994 production device utilizing UFS (it wasn't
very good or affordable in 2014/15 IIRC), so I think it's gtg.
Konrad
Manivannan,
> This series has code some cleanups to the Qcom UFS driver. No functional
> change. In this version, I've removed code supporting legacy controllers
> ver < 2.0, as the respective platforms were never supported in upstream.
>
> Tested on: RB5 development board based on Qcom SM8250 SoC.
Applied to 6.8/scsi-staging, thanks!
--
Martin K. Petersen Oracle Linux Engineering
On Wed, Dec 13, 2023 at 11:10:41PM -0500, Martin K. Petersen wrote:
>
> Manivannan,
>
> > This series has code some cleanups to the Qcom UFS driver. No functional
> > change. In this version, I've removed code supporting legacy controllers
> > ver < 2.0, as the respective platforms were never supported in upstream.
> >
> > Tested on: RB5 development board based on Qcom SM8250 SoC.
>
> Applied to 6.8/scsi-staging, thanks!
>
Thanks Martin! Andrew spotted an issue on patch 16/17 and I'm going to submit a
patch fixing that. Please either squash it or apply it separately at your own
convenience.
- Mani
> --
> Martin K. Petersen Oracle Linux Engineering
--
மணிவண்ணன் சதாசிவம்
On Fri, Dec 08, 2023 at 12:02:03PM -0600, Andrew Halaney wrote:
> On Fri, Dec 08, 2023 at 12:29:01PM +0530, Manivannan Sadhasivam wrote:
> > Instead of using both ufshcd_readl() and ufshcd_writel() to read/modify/
> > write a register, let's make use of the existing helper.
> >
> > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > ---
> > drivers/ufs/host/ufs-qcom.c | 12 ++++--------
> > drivers/ufs/host/ufs-qcom.h | 3 +++
> > 2 files changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > index 26aa8904c823..549a08645391 100644
> > --- a/drivers/ufs/host/ufs-qcom.c
> > +++ b/drivers/ufs/host/ufs-qcom.c
> > @@ -387,9 +387,8 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> > */
> > static void ufs_qcom_enable_hw_clk_gating(struct ufs_hba *hba)
> > {
> > - ufshcd_writel(hba,
> > - ufshcd_readl(hba, REG_UFS_CFG2) | REG_UFS_CFG2_CGC_EN_ALL,
> > - REG_UFS_CFG2);
> > + ufshcd_rmwl(hba, REG_UFS_CFG2_CGC_EN_ALL, REG_UFS_CFG2_CGC_EN_ALL,
> > + REG_UFS_CFG2);
> >
> > /* Ensure that HW clock gating is enabled before next operations */
> > mb();
> > @@ -1689,11 +1688,8 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
> > platform_msi_domain_free_irqs(hba->dev);
> > } else {
> > if (host->hw_ver.major == 6 && host->hw_ver.minor == 0 &&
> > - host->hw_ver.step == 0) {
> > - ufshcd_writel(hba,
> > - ufshcd_readl(hba, REG_UFS_CFG3) | 0x1F000,
> > - REG_UFS_CFG3);
> > - }
> > + host->hw_ver.step == 0)
> > + ufshcd_rmwl(hba, ESI_VEC_MASK, 0x1f00, REG_UFS_CFG3);
>
> Is this right? I feel like you accidentally just shifted what was written
> prior >> 4 bits.
>
> Before: 0x1f000
> After: 0x01f00
>
Doh! you are right, I accidentally missed one 0. Since the series is already
merged, I will send a follow up patch.
> > ufshcd_mcq_enable_esi(hba);
> > }
> >
> > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> > index 385480499e71..2ce63a1c7f2f 100644
> > --- a/drivers/ufs/host/ufs-qcom.h
> > +++ b/drivers/ufs/host/ufs-qcom.h
> > @@ -102,6 +102,9 @@ enum {
> > #define TMRLUT_HW_CGC_EN BIT(6)
> > #define OCSC_HW_CGC_EN BIT(7)
> >
> > +/* bit definitions for REG_UFS_CFG3 register */
> > +#define ESI_VEC_MASK GENMASK(22, 12)
> > +
>
> I'll leave it to someone with the docs to review this field. It at least
> contains the bits set above, fwiw. It would be neat to see that
> converted to using the field + a FIELD_PREP to set the bits IMO, but I
> honestly don't have a lot of experience using those APIs so feel free to
> reject that.
>
Fair point. The reason for hardcoding the value in this patch was I didn't get
information on how the value works. But now I got that info, so will address
this in the follow up patch I mentioned above.
- Mani
> > /* bit definitions for REG_UFS_PARAM0 */
> > #define MAX_HS_GEAR_MASK GENMASK(6, 4)
> > #define UFS_QCOM_MAX_GEAR(x) FIELD_GET(MAX_HS_GEAR_MASK, (x))
> > --
> > 2.25.1
> >
>
--
மணிவண்ணன் சதாசிவம்
On Sat, Dec 09, 2023 at 06:42:31PM +0100, Konrad Dybcio wrote:
> On 8.12.2023 07:58, Manivannan Sadhasivam wrote:
> > Hello,
> >
> > This series has code some cleanups to the Qcom UFS driver. No functional
> > change. In this version, I've removed code supporting legacy controllers
> > ver < 2.0, as the respective platforms were never supported in upstream.
> >
> > Tested on: RB5 development board based on Qcom SM8250 SoC.
> >
> > - Mani
> >
> > Changes in v2:
> >
> > * Collected review tags
> > * Fixed the comments from Andrew
> > * Added a few more patches, most notably one removing the code for old
> > controllers (ver < v2.0)
> FWIW i found this snipped from a downstream commit from 2014:
>
> 8084 : 1.1.1
> 8994v1 : 1.2.0
> 8994v2 : 1.3.0
>
> I'm yet to see any 8994 production device utilizing UFS (it wasn't
> very good or affordable in 2014/15 IIRC), so I think it's gtg.
>
Thanks for digging! I was told that SoCs based on UFS 1.x controllers were not
widely used in production, though I don't know why.
- Mani
> Konrad
--
மணிவண்ணன் சதாசிவம்
On 12/8/2023 6:59 PM, Nitin Rawat wrote:
>
>
> On 12/8/2023 3:58 PM, Manivannan Sadhasivam wrote:
>> On Fri, Dec 08, 2023 at 02:55:21PM +0530, Nitin Rawat wrote:
>>>
>>>
>>> On 12/8/2023 12:28 PM, Manivannan Sadhasivam wrote:
>>>> core_reset is optional, so there is no need to warn the user if it
>>>> is not
>>>> available.
>>>>
>>>> Reviewed-by: Andrew Halaney <[email protected]>
>>>> Signed-off-by: Manivannan Sadhasivam <[email protected]>
>>>> ---
>>>> drivers/ufs/host/ufs-qcom.c | 4 +---
>>>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>>> index dc93b1c5ca74..d474de0739e4 100644
>>>> --- a/drivers/ufs/host/ufs-qcom.c
>>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>>> @@ -296,10 +296,8 @@ static int ufs_qcom_host_reset(struct ufs_hba
>>>> *hba)
>>>> struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>>>> bool reenable_intr;
>>>> - if (!host->core_reset) {
>>>> - dev_warn(hba->dev, "%s: reset control not set\n", __func__);
>>>> + if (!host->core_reset)
>>>> return 0;
>>>> - }
>>>> reenable_intr = hba->is_irq_enabled;
>>>> disable_irq(hba->irq);
>>>
>>>
>>> Hi Mani,
>>>
>>> I think core reset is not frequent. It happen during only probe ,error
>>> handler.
>>>
>>> core reset is needed in kernel to cleanup UFS phy and controller
>>> configuration before UFS HLOS operation starts as per HPG.
>>>
>>
>> This sounds like core reset is not an optional property but a required
>> one. I
>> just checked the upstream DT files for all SoCs, and looks like pretty
>> much all
>> of them support core reset.
>>
>> Only MSM8996 doesn't have the reset property, but the reset is
>> available in GCC.
>> So we should be able to use it in dtsi.
>>
>> I also skimmed through the HPG and looks like core reset is not
>> optional. Please
>> confirm.
>>
>> - Mani
>
>
> Hi Mani,
>
> Yes Core_reset is part of HPG sequence and is needed.
>
> Regards,
> Nitin
Hi Mani,
I see this patch series is merged . So planning to keep the warn message
based on above discussion.
Regards,
Nitin
>
>
>>
>>> Having existing warn print can be used to to debug or atleast know
>>> core_reset is missed in device tree to give indication complete reset
>>> hasn't
>>> been done and we could still be operating in bootloader configuration.
>>>
>>>
>>> Regards,
>>> Nitin
>>>
>>
>
On Thu, Dec 14, 2023 at 12:43:02PM +0530, Nitin Rawat wrote:
>
>
> On 12/8/2023 6:59 PM, Nitin Rawat wrote:
> >
> >
> > On 12/8/2023 3:58 PM, Manivannan Sadhasivam wrote:
> > > On Fri, Dec 08, 2023 at 02:55:21PM +0530, Nitin Rawat wrote:
> > > >
> > > >
> > > > On 12/8/2023 12:28 PM, Manivannan Sadhasivam wrote:
> > > > > core_reset is optional, so there is no need to warn the user
> > > > > if it is not
> > > > > available.
> > > > >
> > > > > Reviewed-by: Andrew Halaney <[email protected]>
> > > > > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > > > > ---
> > > > > drivers/ufs/host/ufs-qcom.c | 4 +---
> > > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > > > > index dc93b1c5ca74..d474de0739e4 100644
> > > > > --- a/drivers/ufs/host/ufs-qcom.c
> > > > > +++ b/drivers/ufs/host/ufs-qcom.c
> > > > > @@ -296,10 +296,8 @@ static int ufs_qcom_host_reset(struct
> > > > > ufs_hba *hba)
> > > > > struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> > > > > bool reenable_intr;
> > > > > - if (!host->core_reset) {
> > > > > - dev_warn(hba->dev, "%s: reset control not set\n", __func__);
> > > > > + if (!host->core_reset)
> > > > > return 0;
> > > > > - }
> > > > > reenable_intr = hba->is_irq_enabled;
> > > > > disable_irq(hba->irq);
> > > >
> > > >
> > > > Hi Mani,
> > > >
> > > > I think core reset is not frequent. It happen during only probe ,error
> > > > handler.
> > > >
> > > > core reset is needed in kernel to cleanup UFS phy and controller
> > > > configuration before UFS HLOS operation starts as per HPG.
> > > >
> > >
> > > This sounds like core reset is not an optional property but a
> > > required one. I
> > > just checked the upstream DT files for all SoCs, and looks like
> > > pretty much all
> > > of them support core reset.
> > >
> > > Only MSM8996 doesn't have the reset property, but the reset is
> > > available in GCC.
> > > So we should be able to use it in dtsi.
> > >
> > > I also skimmed through the HPG and looks like core reset is not
> > > optional. Please
> > > confirm.
> > >
> > > - Mani
> >
> >
> > Hi Mani,
> >
> > Yes Core_reset is part of HPG sequence and is needed.
> >
> > Regards,
> > Nitin
>
>
> Hi Mani,
>
> I see this patch series is merged . So planning to keep the warn message
> based on above discussion.
>
No, you should not add that warning back. As per our discussion, we concluded
that core_reset is not optional, so devm_reset_control_get_exclusive() should be
used to acquire the reset.
If the reset property is not present in DT, then ufs_qcom_init() will fail. This
also means that we should fix the binding and DT of SoCs missing the reset
property.
- Mani
> Regards,
> Nitin
> >
> >
> > >
> > > > Having existing warn print can be used to to debug or atleast know
> > > > core_reset is missed in device tree to give indication complete
> > > > reset hasn't
> > > > been done and we could still be operating in bootloader configuration.
> > > >
> > > >
> > > > Regards,
> > > > Nitin
> > > >
> > >
> >
--
மணிவண்ணன் சதாசிவம்
On Fri, 08 Dec 2023 12:28:45 +0530, Manivannan Sadhasivam wrote:
> This series has code some cleanups to the Qcom UFS driver. No functional
> change. In this version, I've removed code supporting legacy controllers
> ver < 2.0, as the respective platforms were never supported in upstream.
>
> Tested on: RB5 development board based on Qcom SM8250 SoC.
>
> - Mani
>
> [...]
Applied to 6.8/scsi-queue, thanks!
[01/17] scsi: ufs: qcom: Use clk_bulk APIs for managing lane clocks
https://git.kernel.org/mkp/scsi/c/9caef8568831
[02/17] scsi: ufs: qcom: Fix the return value of ufs_qcom_ice_program_key()
https://git.kernel.org/mkp/scsi/c/3bf7ab4ac30c
[03/17] scsi: ufs: qcom: Fix the return value when platform_get_resource_byname() fails
https://git.kernel.org/mkp/scsi/c/3a747c5cf9b6
[04/17] scsi: ufs: qcom: Remove superfluous variable assignments
https://git.kernel.org/mkp/scsi/c/1f165c87ec3e
[05/17] scsi: ufs: qcom: Remove the warning message when core_reset is not available
https://git.kernel.org/mkp/scsi/c/d42d368647da
[06/17] scsi: ufs: qcom: Export ufshcd_{enable/disable}_irq helpers and make use of them
https://git.kernel.org/mkp/scsi/c/0ae7a02726bc
[07/17] scsi: ufs: qcom: Fail ufs_qcom_power_up_sequence() when core_reset fails
https://git.kernel.org/mkp/scsi/c/d11954711499
[08/17] scsi: ufs: qcom: Check the return value of ufs_qcom_power_up_sequence()
https://git.kernel.org/mkp/scsi/c/e430c0e08957
[09/17] scsi: ufs: qcom: Remove redundant error print for devm_kzalloc() failure
https://git.kernel.org/mkp/scsi/c/8291652ed8a2
[10/17] scsi: ufs: qcom: Use dev_err_probe() to simplify error handling of devm_gpiod_get_optional()
https://git.kernel.org/mkp/scsi/c/c7afadacc180
[11/17] scsi: ufs: qcom: Remove unused ufs_qcom_hosts struct array
https://git.kernel.org/mkp/scsi/c/e7458beab809
[12/17] scsi: ufs: qcom: Sort includes alphabetically
https://git.kernel.org/mkp/scsi/c/be2e06c81a31
[13/17] scsi: ufs: qcom: Initialize cycles_in_1us variable in ufs_qcom_set_core_clk_ctrl()
https://git.kernel.org/mkp/scsi/c/3b60f4564ff5
[14/17] scsi: ufs: qcom: Simplify ufs_qcom_{assert/deassert}_reset
https://git.kernel.org/mkp/scsi/c/6b481af25ec0
[15/17] scsi: ufs: qcom: Remove support for host controllers older than v2.0
https://git.kernel.org/mkp/scsi/c/104cd58d9af8
[16/17] scsi: ufs: qcom: Use ufshcd_rmwl() where applicable
https://git.kernel.org/mkp/scsi/c/0e9f4375db1c
[17/17] scsi: ufs: qcom: Remove unused definitions
https://git.kernel.org/mkp/scsi/c/cac50d04fffe
--
Martin K. Petersen Oracle Linux Engineering