On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
> Sometimes due to hw issues it takes some time to the
> host controller register to update. In order to verify the register
> has updated, a polling is done until its value is set.
>
> In addition the functions ufshcd_hba_stop() and
> ufshcd_wait_for_register() was updated with an additional input
> parameter, indicating the timeout between reads will
> be done by sleeping or spinning the cpu.
>
> Signed-off-by: Raviv Shvili <[email protected]>
> Signed-off-by: Yaniv Gardi <[email protected]>
>
> ---
> drivers/scsi/ufs/ufshcd.c | 53 ++++++++++++++++++++++++++++-------------------
> drivers/scsi/ufs/ufshcd.h | 12 +++--------
> 2 files changed, 35 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 3400ceb..80031e6 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -240,11 +240,13 @@ static inline void ufshcd_disable_irq(struct ufs_hba *hba)
> * @val - wait condition
> * @interval_us - polling interval in microsecs
> * @timeout_ms - timeout in millisecs
> + * @can_sleep - perform sleep or just spin
> *
> * Returns -ETIMEDOUT on error, zero on success
> */
> -static int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
> - u32 val, unsigned long interval_us, unsigned long timeout_ms)
> +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)
> {
> int err = 0;
> unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
> @@ -253,9 +255,10 @@ static int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
> val = val & mask;
>
> while ((ufshcd_readl(hba, reg) & mask) != val) {
> - /* wakeup within 50us of expiry */
> - usleep_range(interval_us, interval_us + 50);
> -
> + if (can_sleep)
> + usleep_range(interval_us, interval_us + 50);
> + else
> + udelay(interval_us);
> if (time_after(jiffies, timeout)) {
> if ((ufshcd_readl(hba, reg) & mask) != val)
> err = -ETIMEDOUT;
> @@ -1459,7 +1462,7 @@ ufshcd_clear_cmd(struct ufs_hba *hba, int tag)
> */
> err = ufshcd_wait_for_register(hba,
> REG_UTP_TRANSFER_REQ_DOOR_BELL,
> - mask, ~mask, 1000, 1000);
> + mask, ~mask, 1000, 1000, true);
>
> return err;
> }
> @@ -2815,6 +2818,23 @@ out:
> }
>
> /**
> + * ufshcd_hba_stop - Send controller to reset state
> + * @hba: per adapter instance
> + * @can_sleep: perform sleep or just spin
> + */
> +static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep)
> +{
> + int err;
> +
> + ufshcd_writel(hba, CONTROLLER_DISABLE, REG_CONTROLLER_ENABLE);
> + err = ufshcd_wait_for_register(hba, REG_CONTROLLER_ENABLE,
> + CONTROLLER_ENABLE, CONTROLLER_DISABLE,
> + 10, 1, can_sleep);
> + if (err)
> + dev_err(hba->dev, "%s: Controller disable failed\n", __func__);
> +}
> +
Shouldn't you return an error here?
If the controller disable failed you probably need a hard reset or
something, otherwise I would assume that every other command from that
point on will not work as expected.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
[email protected] +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: J. Hawn, J. Guild, F. Imend?rffer, HRB 16746 (AG N?rnberg)
> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
>> Sometimes due to hw issues it takes some time to the
>> host controller register to update. In order to verify the register
>> has updated, a polling is done until its value is set.
>>
>> In addition the functions ufshcd_hba_stop() and
>> ufshcd_wait_for_register() was updated with an additional input
>> parameter, indicating the timeout between reads will
>> be done by sleeping or spinning the cpu.
>>
>> Signed-off-by: Raviv Shvili <[email protected]>
>> Signed-off-by: Yaniv Gardi <[email protected]>
>>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 53
>> ++++++++++++++++++++++++++++-------------------
>> drivers/scsi/ufs/ufshcd.h | 12 +++--------
>> 2 files changed, 35 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 3400ceb..80031e6 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -240,11 +240,13 @@ static inline void ufshcd_disable_irq(struct
>> ufs_hba *hba)
>> * @val - wait condition
>> * @interval_us - polling interval in microsecs
>> * @timeout_ms - timeout in millisecs
>> + * @can_sleep - perform sleep or just spin
>> *
>> * Returns -ETIMEDOUT on error, zero on success
>> */
>> -static int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32
>> mask,
>> - u32 val, unsigned long interval_us, unsigned long timeout_ms)
>> +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)
>> {
>> int err = 0;
>> unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
>> @@ -253,9 +255,10 @@ static int ufshcd_wait_for_register(struct ufs_hba
>> *hba, u32 reg, u32 mask,
>> val = val & mask;
>>
>> while ((ufshcd_readl(hba, reg) & mask) != val) {
>> - /* wakeup within 50us of expiry */
>> - usleep_range(interval_us, interval_us + 50);
>> -
>> + if (can_sleep)
>> + usleep_range(interval_us, interval_us + 50);
>> + else
>> + udelay(interval_us);
>> if (time_after(jiffies, timeout)) {
>> if ((ufshcd_readl(hba, reg) & mask) != val)
>> err = -ETIMEDOUT;
>> @@ -1459,7 +1462,7 @@ ufshcd_clear_cmd(struct ufs_hba *hba, int tag)
>> */
>> err = ufshcd_wait_for_register(hba,
>> REG_UTP_TRANSFER_REQ_DOOR_BELL,
>> - mask, ~mask, 1000, 1000);
>> + mask, ~mask, 1000, 1000, true);
>>
>> return err;
>> }
>> @@ -2815,6 +2818,23 @@ out:
>> }
>>
>> /**
>> + * ufshcd_hba_stop - Send controller to reset state
>> + * @hba: per adapter instance
>> + * @can_sleep: perform sleep or just spin
>> + */
>> +static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep)
>> +{
>> + int err;
>> +
>> + ufshcd_writel(hba, CONTROLLER_DISABLE, REG_CONTROLLER_ENABLE);
>> + err = ufshcd_wait_for_register(hba, REG_CONTROLLER_ENABLE,
>> + CONTROLLER_ENABLE, CONTROLLER_DISABLE,
>> + 10, 1, can_sleep);
>> + if (err)
>> + dev_err(hba->dev, "%s: Controller disable failed\n", __func__);
>> +}
>> +
> Shouldn't you return an error here?
> If the controller disable failed you probably need a hard reset or
> something, otherwise I would assume that every other command from that
> point on will not work as expected.
>
> Cheers,
>
> Hannes
Hello Hannes,
The original routine signature is:
void ufshcd_hba_stop(struct ufs_hba *hba);
as you can see, no return value, the reason is simple - there is nothing
we can do if writing to the register fails.
all we wanted to do here, was to add a graceful time to change the
register value. also, we decided to add error msg in case the value is not
change within this timeout.
We can not do anything else, not to say, return error, as there is no
error handling in such case.
So, as far as i see it, we only improved the already exists logic, by
adding some graceful time to the register change, and also, by adding an
error message that was absent before.
thanks,
Yaniv
> --
> Dr. Hannes Reinecke zSeries & Storage
> [email protected] +49 911 74053 688
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N?rnberg
> GF: J. Hawn, J. Guild, F. Imend?rffer, HRB 16746 (AG N?rnberg)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On 03/01/2016 09:32 PM, [email protected] wrote:
>> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
>>> Sometimes due to hw issues it takes some time to the
>>> host controller register to update. In order to verify the register
>>> has updated, a polling is done until its value is set.
>>>
>>> In addition the functions ufshcd_hba_stop() and
>>> ufshcd_wait_for_register() was updated with an additional input
>>> parameter, indicating the timeout between reads will
>>> be done by sleeping or spinning the cpu.
>>>
>>> Signed-off-by: Raviv Shvili <[email protected]>
>>> Signed-off-by: Yaniv Gardi <[email protected]>
>>>
>>> ---
>>> drivers/scsi/ufs/ufshcd.c | 53
>>> ++++++++++++++++++++++++++++-------------------
>>> drivers/scsi/ufs/ufshcd.h | 12 +++--------
>>> 2 files changed, 35 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index 3400ceb..80031e6 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -240,11 +240,13 @@ static inline void ufshcd_disable_irq(struct
>>> ufs_hba *hba)
>>> * @val - wait condition
>>> * @interval_us - polling interval in microsecs
>>> * @timeout_ms - timeout in millisecs
>>> + * @can_sleep - perform sleep or just spin
>>> *
>>> * Returns -ETIMEDOUT on error, zero on success
>>> */
>>> -static int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32
>>> mask,
>>> - u32 val, unsigned long interval_us, unsigned long timeout_ms)
>>> +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)
>>> {
>>> int err = 0;
>>> unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
>>> @@ -253,9 +255,10 @@ static int ufshcd_wait_for_register(struct ufs_hba
>>> *hba, u32 reg, u32 mask,
>>> val = val & mask;
>>>
>>> while ((ufshcd_readl(hba, reg) & mask) != val) {
>>> - /* wakeup within 50us of expiry */
>>> - usleep_range(interval_us, interval_us + 50);
>>> -
>>> + if (can_sleep)
>>> + usleep_range(interval_us, interval_us + 50);
>>> + else
>>> + udelay(interval_us);
>>> if (time_after(jiffies, timeout)) {
>>> if ((ufshcd_readl(hba, reg) & mask) != val)
>>> err = -ETIMEDOUT;
>>> @@ -1459,7 +1462,7 @@ ufshcd_clear_cmd(struct ufs_hba *hba, int tag)
>>> */
>>> err = ufshcd_wait_for_register(hba,
>>> REG_UTP_TRANSFER_REQ_DOOR_BELL,
>>> - mask, ~mask, 1000, 1000);
>>> + mask, ~mask, 1000, 1000, true);
>>>
>>> return err;
>>> }
>>> @@ -2815,6 +2818,23 @@ out:
>>> }
>>>
>>> /**
>>> + * ufshcd_hba_stop - Send controller to reset state
>>> + * @hba: per adapter instance
>>> + * @can_sleep: perform sleep or just spin
>>> + */
>>> +static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep)
>>> +{
>>> + int err;
>>> +
>>> + ufshcd_writel(hba, CONTROLLER_DISABLE, REG_CONTROLLER_ENABLE);
>>> + err = ufshcd_wait_for_register(hba, REG_CONTROLLER_ENABLE,
>>> + CONTROLLER_ENABLE, CONTROLLER_DISABLE,
>>> + 10, 1, can_sleep);
>>> + if (err)
>>> + dev_err(hba->dev, "%s: Controller disable failed\n", __func__);
>>> +}
>>> +
>> Shouldn't you return an error here?
>> If the controller disable failed you probably need a hard reset or
>> something, otherwise I would assume that every other command from that
>> point on will not work as expected.
>>
>> Cheers,
>>
>> Hannes
>
>
> Hello Hannes,
> The original routine signature is:
> void ufshcd_hba_stop(struct ufs_hba *hba);
>
> as you can see, no return value, the reason is simple - there is nothing
> we can do if writing to the register fails.
>
> all we wanted to do here, was to add a graceful time to change the
> register value. also, we decided to add error msg in case the value is not
> change within this timeout.
> We can not do anything else, not to say, return error, as there is no
> error handling in such case.
>
> So, as far as i see it, we only improved the already exists logic, by
> adding some graceful time to the register change, and also, by adding an
> error message that was absent before.
>
Thanks for the explanation.
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
[email protected] +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: J. Hawn, J. Guild, F. Imend?rffer, HRB 16746 (AG N?rnberg)