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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
