RE: [PATCH V3 1/2] scsi: ufs: Add support for sending NOP OUT UPIU

[Seungwon Jeon <tgih.jun@samsung.com>]

On Thu, July 18, 2013, Sujit Reddy Thumma wrote:
> >>>> + * ufshcd_wait_for_register - wait for register value to change
> >>>> + * @hba - per-adapter interface
> >>>> + * @reg - mmio register offset
> >>>> + * @mask - mask to apply to read register value
> >>>> + * @val - wait condition
> >>>> + * @interval_us - polling interval in microsecs
> >>>> + * @timeout_ms - timeout in millisecs
> >>>> + *
> >>>> + * Returns final register value after iteration
> >>>> + */
> >>>> +static u32 ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
> >>>> +		u32 val, unsigned long interval_us, unsigned long timeout_ms)
> >>> I feel like this function's role is to wait for clearing register (specially, doorbells).
> >>> If you really don't intend to apply other all register, I think it would better to change the
> >> function name.
> >>> ex> ufshcd_wait_for_clear_doorbell or ufshcd_wait_for_clear_reg?
> >>
> >> Although, this API is introduced in this patch and used only for
> >> clearing the doorbell, I have intentionally made it generic to avoid
> >> duplication of wait condition code in future (not just clearing but
> >> also for setting a bit). For example, when we write to HCE.ENABLE we
> >> wait for it turn to 1.
> >>
> >>
> >>> And if you like it, it could be more simple like below
> >>>
> >>> static bool ufshcd_wait_for_clear_reg(struct ufs_hba *hba, u32 reg, u32 mask,
> >>>                                            unsigned long interval_us,
> >>>                                            unsigned int timeout_ms)
> >>> {
> >>>           unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
> >>
> >> Jiffies on 100Hz systems have minimum granularity of 10ms. So we really
> >> wait for more 10ms even though the timeout_ms < 10ms.
> > Yes. Real timeout depends on system.
> > But normally actual wait will be maintained until 'ufshcd_readl' is done.
> > Timeout is valid for failure case.  If we don't need to apply a strict timeout value, it's not bad.
> 
> Hmm.. make sense. Will take care in the next patchset.
> 
> >
> >>
> >>>           /* wakeup within 50us of expiry */
> >>>           const unsigned int expiry = 50;
> >>>
> >>>           while (ufshcd_readl(hba, reg) & mask) {
> >>>                   usleep_range(interval_us, interval_us + expiry);
> >>>                   if (time_after(jiffies, timeout)) {
> >>>                           if (ufshcd_readl(hba, reg) & mask)
> >>>                                   return false;
> >>
> >> I really want the caller to decide on what to do after the timeout.
> >> This helps in error handling cases where we try to clear off the entire
> >> door-bell register but only a few of the bits are cleared after timeout.
> > I don't know what we can do with the report of the partial success on clearing bit.
> > It's just failure case. Isn't enough with false/true?
> 
> The point is, if a bit can't be cleared it can be regarded as a
> permanent failure (only for that request), otherwise, it can be
> retried with the same tag value.
> 
> >
> >>
> >>>                           else
> >>>                                   return true;
> >>>                   }
> >>>           }
> >>>
> >>>           return true;
> >>> }
> >>>> +{
> >>>> +	u32 tmp;
> >>>> +	ktime_t start;
> >>>> +	unsigned long diff;
> >>>> +
> >>>> +	tmp = ufshcd_readl(hba, reg);
> >>>> +
> >>>> +	if ((val & mask) != val) {
> >>>> +		dev_err(hba->dev, "%s: Invalid wait condition 0x%x\n",
> >>>> +				__func__, val);
> >>>> +		goto out;
> >>>> +	}
> >>>> +
> >>>> +	start = ktime_get();
> >>>> +	while ((tmp & mask) != val) {
> >>>> +		/* wakeup within 50us of expiry */
> >>>> +		usleep_range(interval_us, interval_us + 50);
> >>>> +		tmp = ufshcd_readl(hba, reg);
> >>>> +		diff = ktime_to_ms(ktime_sub(ktime_get(), start));
> >>>> +		if (diff > timeout_ms) {
> >>>> +			tmp = ufshcd_readl(hba, reg);
> >>>> +			break;
> >>>> +		}
> >>>> +	}
> >>>> +out:
> >>>> +	return tmp;
> >>>> +}
> >>>> +
> ..
> >>>> +static int
> >>>> +ufshcd_clear_cmd(struct ufs_hba *hba, int tag)
> >>>> +{
> >>>> +	int err = 0;
> >>>> +	unsigned long flags;
> >>>> +	u32 reg;
> >>>> +	u32 mask = 1 << tag;
> >>>> +
> >>>> +	/* clear outstanding transaction before retry */
> >>>> +	spin_lock_irqsave(hba->host->host_lock, flags);
> >>>> +	ufshcd_utrl_clear(hba, tag);
> >>>> +	spin_unlock_irqrestore(hba->host->host_lock, flags);
> >>>> +
> >>>> +	/*
> >>>> +	 * wait for for h/w to clear corresponding bit in door-bell.
> >>>> +	 * max. wait is 1 sec.
> >>>> +	 */
> >>>> +	reg = ufshcd_wait_for_register(hba,
> >>>> +			REG_UTP_TRANSFER_REQ_DOOR_BELL,
> >>>> +			mask, 0, 1000, 1000);
> >>> 4th argument should be (~mask) instead of '0', right?
> >>
> >> True, but not really for this implementation. It breaks the check in
> >> in wait_for_register -
> >> if ((val & mask) != val)
> >>              dev_err(...);
> > Hmm, it seems complicated to use.
> > Ok. Is right the following about val as 4th argument?
> > - clear: val  should be '0' regardless corresponding bit.
> > - set: val should be same with mask.
> > If the related comment is added, it will be helpful.
> 
> Thinking again it looks like it is complicated. How about changing
> the check to -
> 
> val = val & mask; /* ignore the bits we don't intend to wait on */
> while (ufshcd_readl() & mask != val) {
>   sleep
> }
> 
> Usage will be ~mask for clearing the bits, mask for setting the bits
> in the fourth argument.
Ok.
It's better for the caller.

> 
> >
> >>
> >>> Actually, mask value involves the corresponding bit to be cleared.
> >>> So, 4th argument may be unnecessary.
> >>
> >> As I described above, the wait_for_register can also be used to
> >> check if the value is set or not. In which case, we need 4th argument.
> >>
> >>>
> >>>> +	if ((reg & mask) == mask)
> >>>> +		err = -ETIMEDOUT;
> >>> Also, checking the result can be involved in ufshcd_wait_for_register.
> > Any comment?
> 
> Sorry I missed this. But the point was the same. To make
> wait_for_register() just to wait a definite time and not return any
> error condition when the bits don't turn as expected.
Comparing reg with mask can be done in 'ufshcd_wait_for_register' commonly.
Currently the caller do the same.

Thanks,
Seungwon Jeon