2018-05-06 10:27:23

by Alim Akhtar

[permalink] [raw]
Subject: [PATCH 0/4] Add required changes to ufshcd to support exynos ufs hci

Hi All

These patches are part of a larger patch series [1] which attempts upstreaming
EXYNOS UFS driver support. There was not much activities after v5 of that
series. In between I saw there were other teams in Samsung tried upstreaming
the same, but that has not really gone anywhere.
I have taken this task again and here is another attempt to upstream exynos-ufs
driver support. I have divided the patches into two series, one which adds
required infra in the ufshcd core needed by exynos-ufs driver and other part
will have actual exynos-ufs driver. Splitting this has a advantage of less
reviewing over head.

I am floating these as a new patch set.

[1] https://www.spinics.net/lists/linux-scsi/msg90292.html

These patches are based on mainline v4.17-rc3.

Alim Akhtar (4):
scsi: ufs: add quirk to fix mishandling utrlclr/utmrlclr
scsi: ufs: add quirk not to allow reset of interrupt aggregation
scsi: ufs: add quirk to enable host controller without hce
scsi: ufs: make ufshcd_config_pwr_mode of non-static func

drivers/scsi/ufs/ufshcd.c | 104 ++++++++++++++++++++++++++++++++++++++++++----
drivers/scsi/ufs/ufshcd.h | 18 ++++++++
2 files changed, 114 insertions(+), 8 deletions(-)

--
2.7.4



2018-05-06 10:27:48

by Alim Akhtar

[permalink] [raw]
Subject: [PATCH 3/4] scsi: ufs: add quirk to enable host controller without hce

Some host controller doesn't support host controller enable via HCE.

Signed-off-by: Seungwon Jeon <[email protected]>
Signed-off-by: Alim Akhtar <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 75 +++++++++++++++++++++++++++++++++++++++++++++--
drivers/scsi/ufs/ufshcd.h | 5 ++++
2 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 253257c..5bfd385 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3400,6 +3400,52 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
"dme-link-startup: error code %d\n", ret);
return ret;
}
+/**
+ * ufshcd_dme_reset - UIC command for DME_RESET
+ * @hba: per adapter instance
+ *
+ * DME_RESET command is issued in order to reset UniPro stack.
+ * This function now deal with cold reset.
+ *
+ * Returns 0 on success, non-zero value on failure
+ */
+static int ufshcd_dme_reset(struct ufs_hba *hba)
+{
+ struct uic_command uic_cmd = {0};
+ int ret;
+
+ uic_cmd.command = UIC_CMD_DME_RESET;
+
+ ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
+ if (ret)
+ dev_err(hba->dev,
+ "dme-reset: error code %d\n", ret);
+
+ return ret;
+}
+
+/**
+ * ufshcd_dme_enable - UIC command for DME_ENABLE
+ * @hba: per adapter instance
+ *
+ * DME_ENABLE command is issued in order to enable UniPro stack.
+ *
+ * Returns 0 on success, non-zero value on failure
+ */
+static int ufshcd_dme_enable(struct ufs_hba *hba)
+{
+ struct uic_command uic_cmd = {0};
+ int ret;
+
+ uic_cmd.command = UIC_CMD_DME_ENABLE;
+
+ ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
+ if (ret)
+ dev_err(hba->dev,
+ "dme-reset: error code %d\n", ret);
+
+ return ret;
+}

static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba)
{
@@ -4058,7 +4104,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep)
}

/**
- * ufshcd_hba_enable - initialize the controller
+ * ufshcd_hba_execute_hce - initialize the controller
* @hba: per adapter instance
*
* The controller resets itself and controller firmware initialization
@@ -4067,7 +4113,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep)
*
* Returns 0 on success, non-zero value on failure
*/
-static int ufshcd_hba_enable(struct ufs_hba *hba)
+static int ufshcd_hba_execute_hce(struct ufs_hba *hba)
{
int retry;

@@ -4122,6 +4168,31 @@ static int ufshcd_hba_enable(struct ufs_hba *hba)
return 0;
}

+static int ufshcd_hba_enable(struct ufs_hba *hba)
+{
+ int ret;
+
+ if (hba->quirks & UFSHCI_QUIRK_BROKEN_HCE) {
+ ufshcd_set_link_off(hba);
+ ufshcd_vops_hce_enable_notify(hba, PRE_CHANGE);
+
+ /* enable UIC related interrupts */
+ ufshcd_enable_intr(hba, UFSHCD_UIC_MASK);
+ ret = ufshcd_dme_reset(hba);
+ if (!ret) {
+ ret = ufshcd_dme_enable(hba);
+ if (!ret)
+ ufshcd_vops_hce_enable_notify(hba, POST_CHANGE);
+ if (ret)
+ dev_err(hba->dev,
+ "Host controller enable failed with non-hce\n");
+ }
+ } else {
+ ret = ufshcd_hba_execute_hce(hba);
+ }
+
+ return ret;
+}
static int ufshcd_disable_tx_lcc(struct ufs_hba *hba, bool peer)
{
int tx_lanes, i, err = 0;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 5c91ff1..013a07e 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -606,6 +606,11 @@ struct ufs_hba {
*/
#define UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR 0x200

+ /*
+ * This quirks needs to be enabled if host controller cannot be
+ * enabled via HCE register.
+ */
+ #define UFSHCI_QUIRK_BROKEN_HCE 0x400
unsigned int quirks; /* Deviations from standard UFSHCI spec. */

/* Device deviations from standard UFS device spec. */
--
2.7.4


2018-05-06 10:28:05

by Alim Akhtar

[permalink] [raw]
Subject: [PATCH 4/4] scsi: ufs: make ufshcd_config_pwr_mode of non-static func

This makes ufshcd_config_pwr_mode non-static so that other vendors
like exynos can use the same.

Signed-off-by: Seungwon Jeon <[email protected]>
Signed-off-by: Alim Akhtar <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 5 ++---
drivers/scsi/ufs/ufshcd.h | 2 ++
2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 5bfd385..68aefcd 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -233,8 +233,6 @@ static void ufshcd_suspend_clkscaling(struct ufs_hba *hba);
static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba);
static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up);
static irqreturn_t ufshcd_intr(int irq, void *__hba);
-static int ufshcd_config_pwr_mode(struct ufs_hba *hba,
- struct ufs_pa_layer_attr *desired_pwr_mode);
static int ufshcd_change_power_mode(struct ufs_hba *hba,
struct ufs_pa_layer_attr *pwr_mode);
static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)
@@ -3969,7 +3967,7 @@ static int ufshcd_change_power_mode(struct ufs_hba *hba,
* @hba: per-adapter instance
* @desired_pwr_mode: desired power configuration
*/
-static int ufshcd_config_pwr_mode(struct ufs_hba *hba,
+int ufshcd_config_pwr_mode(struct ufs_hba *hba,
struct ufs_pa_layer_attr *desired_pwr_mode)
{
struct ufs_pa_layer_attr final_params = { 0 };
@@ -3987,6 +3985,7 @@ static int ufshcd_config_pwr_mode(struct ufs_hba *hba,

return ret;
}
+EXPORT_SYMBOL_GPL(ufshcd_config_pwr_mode);

/**
* ufshcd_complete_dev_init() - checks device readiness
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 013a07e..b42a5a3 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -805,6 +805,8 @@ extern int ufshcd_dme_set_attr(struct ufs_hba *hba, u32 attr_sel,
u8 attr_set, u32 mib_val, u8 peer);
extern int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
u32 *mib_val, u8 peer);
+extern int ufshcd_config_pwr_mode(struct ufs_hba *hba,
+ struct ufs_pa_layer_attr *desired_pwr_mode);

/* UIC command interfaces for DME primitives */
#define DME_LOCAL 0
--
2.7.4


2018-05-06 10:28:36

by Alim Akhtar

[permalink] [raw]
Subject: [PATCH 2/4] scsi: ufs: add quirk not to allow reset of interrupt aggregation

Some host controller supports interrupt aggregation, but doesn't
allow to reset counter and timer by s/w.

Signed-off-by: Seungwon Jeon <[email protected]>
Signed-off-by: Alim Akhtar <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 3 ++-
drivers/scsi/ufs/ufshcd.h | 6 ++++++
2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9898ce5..253257c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4695,7 +4695,8 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
* false interrupt if device completes another request after resetting
* aggregation and before reading the DB.
*/
- if (ufshcd_is_intr_aggr_allowed(hba))
+ if (ufshcd_is_intr_aggr_allowed(hba) &&
+ !(hba->quirks & UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR))
ufshcd_reset_intr_aggr(hba);

tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 43035f8..5c91ff1 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -600,6 +600,12 @@ struct ufs_hba {
*/
#define UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR 0x100

+ /*
+ * This quirk needs to be enabled if host controller doesn't allow
+ * that the interrupt aggregation timer and counter are reset by s/w.
+ */
+ #define UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR 0x200
+
unsigned int quirks; /* Deviations from standard UFSHCI spec. */

/* Device deviations from standard UFS device spec. */
--
2.7.4


2018-05-06 10:30:35

by Alim Akhtar

[permalink] [raw]
Subject: [PATCH 1/4] scsi: ufs: add quirk to fix mishandling utrlclr/utmrlclr

In the right behavior, setting the bit to '0' indicates clear and
'1' indicates no change. If host controller handles this the other way,
UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR can be used.

Signed-off-by: Seungwon Jeon <[email protected]>
Signed-off-by: Alim Akhtar <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 21 +++++++++++++++++++--
drivers/scsi/ufs/ufshcd.h | 5 +++++
2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 00e7905..9898ce5 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -675,7 +675,24 @@ static inline void ufshcd_put_tm_slot(struct ufs_hba *hba, int slot)
*/
static inline void ufshcd_utrl_clear(struct ufs_hba *hba, u32 pos)
{
- ufshcd_writel(hba, ~(1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
+ if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
+ ufshcd_writel(hba, (1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
+ else
+ ufshcd_writel(hba, ~(1 << pos),
+ REG_UTP_TRANSFER_REQ_LIST_CLEAR);
+}
+
+/**
+ * ufshcd_utmrl_clear - Clear a bit in UTRMLCLR register
+ * @hba: per adapter instance
+ * @pos: position of the bit to be cleared
+ */
+static inline void ufshcd_utmrl_clear(struct ufs_hba *hba, u32 pos)
+{
+ if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
+ ufshcd_writel(hba, (1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
+ else
+ ufshcd_writel(hba, ~(1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
}

/**
@@ -5398,7 +5415,7 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag)
goto out;

spin_lock_irqsave(hba->host->host_lock, flags);
- ufshcd_writel(hba, ~(1 << tag), REG_UTP_TASK_REQ_LIST_CLEAR);
+ ufshcd_utmrl_clear(hba, tag);
spin_unlock_irqrestore(hba->host->host_lock, flags);

/* poll for max. 1 sec to clear door bell register by h/w */
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 8110dcd..43035f8 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -595,6 +595,11 @@ struct ufs_hba {
*/
#define UFSHCD_QUIRK_PRDT_BYTE_GRAN 0x80

+ /*
+ * Cleaer handling for transfer/task request list is just opposite.
+ */
+ #define UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR 0x100
+
unsigned int quirks; /* Deviations from standard UFSHCI spec. */

/* Device deviations from standard UFS device spec. */
--
2.7.4


2018-05-16 04:32:07

by Alim Akhtar

[permalink] [raw]
Subject: Re: [PATCH 0/4] Add required changes to ufshcd to support exynos ufs hci

Hi All,

Any thought on this patch set?


On Sun, May 6, 2018 at 3:44 PM, Alim Akhtar <[email protected]> wrote:
> Hi All
>
> These patches are part of a larger patch series [1] which attempts upstreaming
> EXYNOS UFS driver support. There was not much activities after v5 of that
> series. In between I saw there were other teams in Samsung tried upstreaming
> the same, but that has not really gone anywhere.
> I have taken this task again and here is another attempt to upstream exynos-ufs
> driver support. I have divided the patches into two series, one which adds
> required infra in the ufshcd core needed by exynos-ufs driver and other part
> will have actual exynos-ufs driver. Splitting this has a advantage of less
> reviewing over head.
>
> I am floating these as a new patch set.
>
> [1] https://www.spinics.net/lists/linux-scsi/msg90292.html
>
> These patches are based on mainline v4.17-rc3.
>
> Alim Akhtar (4):
> scsi: ufs: add quirk to fix mishandling utrlclr/utmrlclr
> scsi: ufs: add quirk not to allow reset of interrupt aggregation
> scsi: ufs: add quirk to enable host controller without hce
> scsi: ufs: make ufshcd_config_pwr_mode of non-static func
>
> drivers/scsi/ufs/ufshcd.c | 104 ++++++++++++++++++++++++++++++++++++++++++----
> drivers/scsi/ufs/ufshcd.h | 18 ++++++++
> 2 files changed, 114 insertions(+), 8 deletions(-)
>
> --
> 2.7.4
>



--
Regards,
Alim

2018-05-16 07:52:08

by Avri Altman

[permalink] [raw]
Subject: RE: [PATCH 4/4] scsi: ufs: make ufshcd_config_pwr_mode of non-static func



> -----Original Message-----
> From: [email protected] <[email protected]>
> On Behalf Of Alim Akhtar
> Sent: Sunday, May 06, 2018 1:14 PM
> To: [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: [PATCH 4/4] scsi: ufs: make ufshcd_config_pwr_mode of non-static
> func
>
> This makes ufshcd_config_pwr_mode non-static so that other vendors like
> exynos can use the same.
>
> Signed-off-by: Seungwon Jeon <[email protected]>
> Signed-off-by: Alim Akhtar <[email protected]>
Acked-by: Avri Altman <[email protected]>

Might be also useful exporting an API for all uic commands?

Thanks,
Avri

2018-05-16 21:24:19

by Subhash Jadavani

[permalink] [raw]
Subject: Re: [PATCH 1/4] scsi: ufs: add quirk to fix mishandling utrlclr/utmrlclr

On 2018-05-06 03:14, Alim Akhtar wrote:
> In the right behavior, setting the bit to '0' indicates clear and
> '1' indicates no change. If host controller handles this the other way,
> UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR can be used.
>
> Signed-off-by: Seungwon Jeon <[email protected]>
> Signed-off-by: Alim Akhtar <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 21 +++++++++++++++++++--
> drivers/scsi/ufs/ufshcd.h | 5 +++++
> 2 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 00e7905..9898ce5 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -675,7 +675,24 @@ static inline void ufshcd_put_tm_slot(struct
> ufs_hba *hba, int slot)
> */
> static inline void ufshcd_utrl_clear(struct ufs_hba *hba, u32 pos)
> {
> - ufshcd_writel(hba, ~(1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> + if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
> + ufshcd_writel(hba, (1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> + else
> + ufshcd_writel(hba, ~(1 << pos),
> + REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> +}
> +
> +/**
> + * ufshcd_utmrl_clear - Clear a bit in UTRMLCLR register
> + * @hba: per adapter instance
> + * @pos: position of the bit to be cleared
> + */
> +static inline void ufshcd_utmrl_clear(struct ufs_hba *hba, u32 pos)
> +{
> + if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
> + ufshcd_writel(hba, (1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
> + else
> + ufshcd_writel(hba, ~(1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
> }
>
> /**
> @@ -5398,7 +5415,7 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba
> *hba, int tag)
> goto out;
>
> spin_lock_irqsave(hba->host->host_lock, flags);
> - ufshcd_writel(hba, ~(1 << tag), REG_UTP_TASK_REQ_LIST_CLEAR);
> + ufshcd_utmrl_clear(hba, tag);
> spin_unlock_irqrestore(hba->host->host_lock, flags);
>
> /* poll for max. 1 sec to clear door bell register by h/w */
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 8110dcd..43035f8 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -595,6 +595,11 @@ struct ufs_hba {
> */
> #define UFSHCD_QUIRK_PRDT_BYTE_GRAN 0x80
>
> + /*
> + * Cleaer handling for transfer/task request list is just opposite.
> + */
> + #define UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR 0x100
> +
> unsigned int quirks; /* Deviations from standard UFSHCI spec. */
>
> /* Device deviations from standard UFS device spec. */

Looks good to me.
Reviewed-by: Subhash Jadavani <[email protected]>

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2018-05-16 21:25:01

by Subhash Jadavani

[permalink] [raw]
Subject: Re: [PATCH 2/4] scsi: ufs: add quirk not to allow reset of interrupt aggregation

On 2018-05-06 03:14, Alim Akhtar wrote:
> Some host controller supports interrupt aggregation, but doesn't
> allow to reset counter and timer by s/w.
>
> Signed-off-by: Seungwon Jeon <[email protected]>
> Signed-off-by: Alim Akhtar <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 3 ++-
> drivers/scsi/ufs/ufshcd.h | 6 ++++++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 9898ce5..253257c 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4695,7 +4695,8 @@ static void ufshcd_transfer_req_compl(struct
> ufs_hba *hba)
> * false interrupt if device completes another request after
> resetting
> * aggregation and before reading the DB.
> */
> - if (ufshcd_is_intr_aggr_allowed(hba))
> + if (ufshcd_is_intr_aggr_allowed(hba) &&
> + !(hba->quirks & UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR))
> ufshcd_reset_intr_aggr(hba);
>
> tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 43035f8..5c91ff1 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -600,6 +600,12 @@ struct ufs_hba {
> */
> #define UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR 0x100
>
> + /*
> + * This quirk needs to be enabled if host controller doesn't allow
> + * that the interrupt aggregation timer and counter are reset by s/w.
> + */
> + #define UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR 0x200
> +
> unsigned int quirks; /* Deviations from standard UFSHCI spec. */
>
> /* Device deviations from standard UFS device spec. */

Looks good to me.
Reviewed-by: Subhash Jadavani <[email protected]>

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2018-05-16 21:27:31

by Subhash Jadavani

[permalink] [raw]
Subject: Re: [PATCH 4/4] scsi: ufs: make ufshcd_config_pwr_mode of non-static func

On 2018-05-06 03:14, Alim Akhtar wrote:
> This makes ufshcd_config_pwr_mode non-static so that other vendors
> like exynos can use the same.
>
> Signed-off-by: Seungwon Jeon <[email protected]>
> Signed-off-by: Alim Akhtar <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 5 ++---
> drivers/scsi/ufs/ufshcd.h | 2 ++
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 5bfd385..68aefcd 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -233,8 +233,6 @@ static void ufshcd_suspend_clkscaling(struct
> ufs_hba *hba);
> static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba);
> static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up);
> static irqreturn_t ufshcd_intr(int irq, void *__hba);
> -static int ufshcd_config_pwr_mode(struct ufs_hba *hba,
> - struct ufs_pa_layer_attr *desired_pwr_mode);
> static int ufshcd_change_power_mode(struct ufs_hba *hba,
> struct ufs_pa_layer_attr *pwr_mode);
> static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)
> @@ -3969,7 +3967,7 @@ static int ufshcd_change_power_mode(struct
> ufs_hba *hba,
> * @hba: per-adapter instance
> * @desired_pwr_mode: desired power configuration
> */
> -static int ufshcd_config_pwr_mode(struct ufs_hba *hba,
> +int ufshcd_config_pwr_mode(struct ufs_hba *hba,
> struct ufs_pa_layer_attr *desired_pwr_mode)
> {
> struct ufs_pa_layer_attr final_params = { 0 };
> @@ -3987,6 +3985,7 @@ static int ufshcd_config_pwr_mode(struct ufs_hba
> *hba,
>
> return ret;
> }
> +EXPORT_SYMBOL_GPL(ufshcd_config_pwr_mode);
>
> /**
> * ufshcd_complete_dev_init() - checks device readiness
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 013a07e..b42a5a3 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -805,6 +805,8 @@ extern int ufshcd_dme_set_attr(struct ufs_hba
> *hba, u32 attr_sel,
> u8 attr_set, u32 mib_val, u8 peer);
> extern int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
> u32 *mib_val, u8 peer);
> +extern int ufshcd_config_pwr_mode(struct ufs_hba *hba,
> + struct ufs_pa_layer_attr *desired_pwr_mode);
>
> /* UIC command interfaces for DME primitives */
> #define DME_LOCAL 0

Looks good to me.
Reviewed-by: Subhash Jadavani <[email protected]>

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2018-05-16 21:28:08

by Subhash Jadavani

[permalink] [raw]
Subject: Re: [PATCH 3/4] scsi: ufs: add quirk to enable host controller without hce

On 2018-05-06 03:14, Alim Akhtar wrote:
> Some host controller doesn't support host controller enable via HCE.
>
> Signed-off-by: Seungwon Jeon <[email protected]>
> Signed-off-by: Alim Akhtar <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 75
> +++++++++++++++++++++++++++++++++++++++++++++--
> drivers/scsi/ufs/ufshcd.h | 5 ++++
> 2 files changed, 78 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 253257c..5bfd385 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3400,6 +3400,52 @@ static int ufshcd_dme_link_startup(struct
> ufs_hba *hba)
> "dme-link-startup: error code %d\n", ret);
> return ret;
> }
> +/**
> + * ufshcd_dme_reset - UIC command for DME_RESET
> + * @hba: per adapter instance
> + *
> + * DME_RESET command is issued in order to reset UniPro stack.
> + * This function now deal with cold reset.
> + *
> + * Returns 0 on success, non-zero value on failure
> + */
> +static int ufshcd_dme_reset(struct ufs_hba *hba)
> +{
> + struct uic_command uic_cmd = {0};
> + int ret;
> +
> + uic_cmd.command = UIC_CMD_DME_RESET;
> +
> + ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
> + if (ret)
> + dev_err(hba->dev,
> + "dme-reset: error code %d\n", ret);
> +
> + return ret;
> +}
> +
> +/**
> + * ufshcd_dme_enable - UIC command for DME_ENABLE
> + * @hba: per adapter instance
> + *
> + * DME_ENABLE command is issued in order to enable UniPro stack.
> + *
> + * Returns 0 on success, non-zero value on failure
> + */
> +static int ufshcd_dme_enable(struct ufs_hba *hba)
> +{
> + struct uic_command uic_cmd = {0};
> + int ret;
> +
> + uic_cmd.command = UIC_CMD_DME_ENABLE;
> +
> + ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
> + if (ret)
> + dev_err(hba->dev,
> + "dme-reset: error code %d\n", ret);
> +
> + return ret;
> +}
>
> static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba
> *hba)
> {
> @@ -4058,7 +4104,7 @@ static inline void ufshcd_hba_stop(struct
> ufs_hba *hba, bool can_sleep)
> }
>
> /**
> - * ufshcd_hba_enable - initialize the controller
> + * ufshcd_hba_execute_hce - initialize the controller
> * @hba: per adapter instance
> *
> * The controller resets itself and controller firmware initialization
> @@ -4067,7 +4113,7 @@ static inline void ufshcd_hba_stop(struct
> ufs_hba *hba, bool can_sleep)
> *
> * Returns 0 on success, non-zero value on failure
> */
> -static int ufshcd_hba_enable(struct ufs_hba *hba)
> +static int ufshcd_hba_execute_hce(struct ufs_hba *hba)
> {
> int retry;
>
> @@ -4122,6 +4168,31 @@ static int ufshcd_hba_enable(struct ufs_hba
> *hba)
> return 0;
> }
>
> +static int ufshcd_hba_enable(struct ufs_hba *hba)
> +{
> + int ret;
> +
> + if (hba->quirks & UFSHCI_QUIRK_BROKEN_HCE) {
> + ufshcd_set_link_off(hba);
> + ufshcd_vops_hce_enable_notify(hba, PRE_CHANGE);
> +
> + /* enable UIC related interrupts */
> + ufshcd_enable_intr(hba, UFSHCD_UIC_MASK);
> + ret = ufshcd_dme_reset(hba);
> + if (!ret) {
> + ret = ufshcd_dme_enable(hba);
> + if (!ret)
> + ufshcd_vops_hce_enable_notify(hba, POST_CHANGE);
> + if (ret)
> + dev_err(hba->dev,
> + "Host controller enable failed with non-hce\n");
> + }
> + } else {
> + ret = ufshcd_hba_execute_hce(hba);
> + }
> +
> + return ret;
> +}
> static int ufshcd_disable_tx_lcc(struct ufs_hba *hba, bool peer)
> {
> int tx_lanes, i, err = 0;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 5c91ff1..013a07e 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -606,6 +606,11 @@ struct ufs_hba {
> */
> #define UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR 0x200
>
> + /*
> + * This quirks needs to be enabled if host controller cannot be
> + * enabled via HCE register.
> + */
> + #define UFSHCI_QUIRK_BROKEN_HCE 0x400
> unsigned int quirks; /* Deviations from standard UFSHCI spec. */
>
> /* Device deviations from standard UFS device spec. */

Looks good to me.
Reviewed-by: Subhash Jadavani <[email protected]>

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2018-05-17 04:07:25

by Asutosh Das (asd)

[permalink] [raw]
Subject: Re: [PATCH 1/4] scsi: ufs: add quirk to fix mishandling utrlclr/utmrlclr

On 5/6/2018 3:44 PM, Alim Akhtar wrote:
> In the right behavior, setting the bit to '0' indicates clear and
> '1' indicates no change. If host controller handles this the other way,
> UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR can be used.
>
> Signed-off-by: Seungwon Jeon <[email protected]>
> Signed-off-by: Alim Akhtar <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 21 +++++++++++++++++++--
> drivers/scsi/ufs/ufshcd.h | 5 +++++
> 2 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 00e7905..9898ce5 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -675,7 +675,24 @@ static inline void ufshcd_put_tm_slot(struct ufs_hba *hba, int slot)
> */
> static inline void ufshcd_utrl_clear(struct ufs_hba *hba, u32 pos)
> {
> - ufshcd_writel(hba, ~(1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> + if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
> + ufshcd_writel(hba, (1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> + else
> + ufshcd_writel(hba, ~(1 << pos),
> + REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> +}
> +
> +/**
> + * ufshcd_utmrl_clear - Clear a bit in UTRMLCLR register
> + * @hba: per adapter instance
> + * @pos: position of the bit to be cleared
> + */
> +static inline void ufshcd_utmrl_clear(struct ufs_hba *hba, u32 pos)
> +{
> + if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
> + ufshcd_writel(hba, (1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
> + else
> + ufshcd_writel(hba, ~(1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
> }
>
> /**
> @@ -5398,7 +5415,7 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag)
> goto out;
>
> spin_lock_irqsave(hba->host->host_lock, flags);
> - ufshcd_writel(hba, ~(1 << tag), REG_UTP_TASK_REQ_LIST_CLEAR);
> + ufshcd_utmrl_clear(hba, tag);
> spin_unlock_irqrestore(hba->host->host_lock, flags);
>
> /* poll for max. 1 sec to clear door bell register by h/w */
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 8110dcd..43035f8 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -595,6 +595,11 @@ struct ufs_hba {
> */
> #define UFSHCD_QUIRK_PRDT_BYTE_GRAN 0x80
>
> + /*
> + * Cleaer handling for transfer/task request list is just opposite.
> + */
Spell check - should be 'Clear'
> + #define UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR 0x100
> +
> unsigned int quirks; /* Deviations from standard UFSHCI spec. */
>
> /* Device deviations from standard UFS device spec. */
>

Looks good to me, except the spell-check.

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project

2018-05-18 14:54:40

by Martin K. Petersen

[permalink] [raw]
Subject: Re: [PATCH 0/4] Add required changes to ufshcd to support exynos ufs hci


Alim,

> These patches are part of a larger patch series [1] which attempts
> upstreaming EXYNOS UFS driver support. There was not much activities
> after v5 of that series. In between I saw there were other teams in
> Samsung tried upstreaming the same, but that has not really gone
> anywhere. I have taken this task again and here is another attempt to
> upstream exynos-ufs driver support. I have divided the patches into
> two series, one which adds required infra in the ufshcd core needed by
> exynos-ufs driver and other part will have actual exynos-ufs
> driver. Splitting this has a advantage of less reviewing over head.

Applied to 4.18/scsi-queue. Thank you!

--
Martin K. Petersen Oracle Linux Engineering