On 03/01/2016 09:32 PM, ygardi@xxxxxxxxxxxxxx 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 <rshvili@xxxxxxxxxxxxxx>
>>> Signed-off-by: Yaniv Gardi <ygardi@xxxxxxxxxxxxxx>
>>>
>>> ---
>>> 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 <hare@xxxxxxxx>
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@xxxxxxx +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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
