2020-03-18 10:41:32

by Stanley Chu

[permalink] [raw]
Subject: [PATCH v7 0/7] scsi: ufs: some cleanups and make the delay for host enabling customizable

v6 -> v7
- Fix patch #3 "scsi: ufs: introduce common delay function" (Bart Van Assche)
- Remove "can_sleep" related changes.
- Limit the usage of common delay function, for example, if delay time
is fixed and larger than 10 us, using introduced common delay function is not required.
- Other related changes according to patch #3 changes

v5 -> v6
- Drop patch #2 "scsi: ufs: remove init_prefetch_data in struct ufs_hba" in v5
because Can Guo has similar cleanup earlier in patch "scsi: ufs: Do not rely on prefetched data"

v4 -> v5
- Fix patch #7: Fix typo "initializatoin" in title

v3 -> v4
- In patch #8, fix incorrect condition of customized delay for host enabling

v2 -> v3
- Remove /arch/arm64/configs/defconfig chnage because it is for local test only

v1 -> v2
- Add patch #1 "scsi: ufs: fix uninitialized tx_lanes in ufshcd_disable_tx_lcc"
- Remove struct ufs_init_prefetch in patch #2 "scsi: ufs: remove init_prefetch_data in struct ufs_hba"
- Introduce common delay function in patch #4
- Replace all delay places by common delay function in ufs-mediatek in patch #5
- Use common delay function instead for host enabling delay in patch #6
- Add patch #7 "scsi: ufs: make HCE polling more compact to improve initializatoin latency"
- In patch #8, customize the delay in ufs_mtk_hce_enable_notify callback instead of ufs_mtk_init (Avri Altman)

Stanley Chu (7):
scsi: ufs: fix uninitialized tx_lanes in ufshcd_disable_tx_lcc()
scsi: ufs: use an enum for host capabilities
scsi: ufs: introduce common and flexible delay function
scsi: ufs-mediatek: use common delay function for required places
scsi: ufs: allow customized delay for host enabling
scsi: ufs: make HCE polling more compact to improve initialization
latency
scsi: ufs-mediatek: customize the delay for host enabling

drivers/scsi/ufs/ufs-mediatek.c | 58 +++++++++++++++++-----------
drivers/scsi/ufs/ufs-mediatek.h | 1 +
drivers/scsi/ufs/ufshcd.c | 21 +++++++++--
drivers/scsi/ufs/ufshcd.h | 67 +++++++++++++++++++--------------
4 files changed, 93 insertions(+), 54 deletions(-)

--
2.18.0


2020-03-18 10:41:47

by Stanley Chu

[permalink] [raw]
Subject: [PATCH v7 1/7] scsi: ufs: fix uninitialized tx_lanes in ufshcd_disable_tx_lcc()

In ufshcd_disable_tx_lcc(), if ufshcd_dme_get() or ufshcd_dme_peer_get()
get fail, uninitialized variable "tx_lanes" may be used as unexpected lane
ID for DME configuration.

Fix this issue by initializing "tx_lanes".

Signed-off-by: Stanley Chu <[email protected]>
Reviewed-by: Asutosh Das <[email protected]>
Reviewed-by: Avri Altman <[email protected]>
Reviewed-by: Can Guo <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 5698f1164a5e..314e808b0d4e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4315,7 +4315,7 @@ EXPORT_SYMBOL_GPL(ufshcd_hba_enable);

static int ufshcd_disable_tx_lcc(struct ufs_hba *hba, bool peer)
{
- int tx_lanes, i, err = 0;
+ int tx_lanes = 0, i, err = 0;

if (!peer)
ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES),
--
2.18.0

2020-03-18 10:42:24

by Stanley Chu

[permalink] [raw]
Subject: [PATCH v7 3/7] scsi: ufs: introduce common and flexible delay function

Introduce a common delay function to provide flexible way for users
to take choices of udelay and usleep_range into consideration according
to the required delay time.

Signed-off-by: Stanley Chu <[email protected]>
Reviewed-by: Avri Altman <[email protected]>
Reviewed-by: Can Guo <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 12 ++++++++++++
drivers/scsi/ufs/ufshcd.h | 1 +
2 files changed, 13 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 314e808b0d4e..a42a84164dec 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -597,6 +597,18 @@ static void ufshcd_print_pwr_info(struct ufs_hba *hba)
hba->pwr_info.hs_rate);
}

+void ufshcd_delay_us(unsigned long us, unsigned long tolerance)
+{
+ if (!us)
+ return;
+
+ if (us < 10)
+ udelay(us);
+ else
+ usleep_range(us, us + tolerance);
+}
+EXPORT_SYMBOL_GPL(ufshcd_delay_us);
+
/*
* ufshcd_wait_for_register - wait for register value to change
* @hba - per-adapter interface
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 52425371082a..9a14ff3d5f8b 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -792,6 +792,7 @@ int ufshcd_init(struct ufs_hba * , void __iomem * , unsigned int);
int ufshcd_make_hba_operational(struct ufs_hba *hba);
void ufshcd_remove(struct ufs_hba *);
int ufshcd_uic_hibern8_exit(struct ufs_hba *hba);
+void ufshcd_delay_us(unsigned long us, unsigned long tolerance);
int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
u32 val, unsigned long interval_us,
unsigned long timeout_ms, bool can_sleep);
--
2.18.0

2020-03-18 10:42:52

by Stanley Chu

[permalink] [raw]
Subject: [PATCH v7 5/7] scsi: ufs: allow customized delay for host enabling

Currently a 1 ms delay is applied before polling CONTROLLER_ENABLE
bit. This delay may not be required or can be changed in different
controllers. Make the delay as a changeable value in struct ufs_hba to
allow it customized by vendors.

Signed-off-by: Stanley Chu <[email protected]>
Reviewed-by: Avri Altman <[email protected]>
Reviewed-by: Can Guo <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 3 ++-
drivers/scsi/ufs/ufshcd.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a42a84164dec..c5ee77a5bfc7 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4301,7 +4301,7 @@ int ufshcd_hba_enable(struct ufs_hba *hba)
* instruction might be read back.
* This delay can be changed based on the controller.
*/
- usleep_range(1000, 1100);
+ ufshcd_delay_us(hba->hba_enable_delay_us, 100);

/* wait for the host controller to complete initialization */
retry = 10;
@@ -8424,6 +8424,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)

hba->mmio_base = mmio_base;
hba->irq = irq;
+ hba->hba_enable_delay_us = 1000;

err = ufshcd_hba_init(hba);
if (err)
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 9a14ff3d5f8b..fa81dac9ae5a 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -663,6 +663,7 @@ struct ufs_hba {
u32 eh_flags;
u32 intr_mask;
u16 ee_ctrl_mask;
+ u16 hba_enable_delay_us;
bool is_powered;
struct ufs_init_prefetch init_prefetch_data;

--
2.18.0

2020-03-18 22:11:37

by Bean Huo (beanhuo)

[permalink] [raw]
Subject: RE: [EXT] [PATCH v7 3/7] scsi: ufs: introduce common and flexible delay function

Hi, Stanley
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 314e808b0d4e..a42a84164dec 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -597,6 +597,18 @@ static void ufshcd_print_pwr_info(struct ufs_hba *hba)
> hba->pwr_info.hs_rate);
> }
>
> +void ufshcd_delay_us(unsigned long us, unsigned long tolerance) {
> + if (!us)
> + return;
> +
> + if (us < 10)
> + udelay(us);
> + else
> + usleep_range(us, us + tolerance);
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_delay_us);
> +
In this way, the callers of ufshcd_delay_us(), can directly call udelay() or usleep_range(), what is exist meaning of ufshcd_delay_us()?

//Bean

2020-03-19 00:11:32

by Stanley Chu

[permalink] [raw]
Subject: RE: [EXT] [PATCH v7 3/7] scsi: ufs: introduce common and flexible delay function

Hi Bean,

On Wed, 2020-03-18 at 22:10 +0000, Bean Huo (beanhuo) wrote:
> Hi, Stanley
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> > 314e808b0d4e..a42a84164dec 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -597,6 +597,18 @@ static void ufshcd_print_pwr_info(struct ufs_hba *hba)
> > hba->pwr_info.hs_rate);
> > }
> >
> > +void ufshcd_delay_us(unsigned long us, unsigned long tolerance) {
> > + if (!us)
> > + return;
> > +
> > + if (us < 10)
> > + udelay(us);
> > + else
> > + usleep_range(us, us + tolerance);
> > +}
> > +EXPORT_SYMBOL_GPL(ufshcd_delay_us);
> > +
> In this way, the callers of ufshcd_delay_us(), can directly call udelay() or usleep_range(), what is exist meaning of ufshcd_delay_us()?

Sure, the callers always can directly call udelay() or usleep_range().

The customizable delay (either by hosts or devices) value in UFS driver
is becoming more and more, like "reference clock gating delay" and
introduced "hce_enable_delay". The customized delay time could be 0, <
10 us, or >= 10 us in real cases. Hence this function can help driver
simplify the driver and user's decision of "just passed without any
delay" or "choosing a suitable delay function according to the delay
time".

Thanks,
Stanley Chu

2020-03-27 02:12:28

by Martin K. Petersen

[permalink] [raw]
Subject: Re: [PATCH v7 0/7] scsi: ufs: some cleanups and make the delay for host enabling customizable


Stanley,

> Stanley Chu (7):
> scsi: ufs: fix uninitialized tx_lanes in ufshcd_disable_tx_lcc()
> scsi: ufs: use an enum for host capabilities
> scsi: ufs: introduce common and flexible delay function
> scsi: ufs-mediatek: use common delay function for required places
> scsi: ufs: allow customized delay for host enabling
> scsi: ufs: make HCE polling more compact to improve initialization latency
> scsi: ufs-mediatek: customize the delay for host enabling

Applied to 5.7/scsi-queue. Thanks!

--
Martin K. Petersen Oracle Linux Engineering