From mboxrd@z Thu Jan 1 00:00:00 1970 From: Seungwon Jeon Subject: RE: [PATCH V3 1/2] scsi: ufs: Add support for sending NOP OUT UPIU Date: Fri, 19 Jul 2013 22:47:57 +0900 Message-ID: <001c01ce8486$9357c510$ba074f30$%jun@samsung.com> References: <1373361310-30674-1-git-send-email-sthumma@codeaurora.org> <1373361310-30674-2-git-send-email-sthumma@codeaurora.org> <002501ce7d71$4c7a8f40$e56fadc0$%jun@samsung.com> <51DE7D08.9000306@codeaurora.org> <001a01ce82c5$8bd47d50$a37d77f0$%jun@samsung.com> <51E76596.5090509@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Return-path: In-reply-to: <51E76596.5090509@codeaurora.org> Content-language: ko Sender: linux-scsi-owner@vger.kernel.org To: 'Sujit Reddy Thumma' Cc: 'Vinayak Holikatti' , 'Santosh Y' , "'James E.J. Bottomley'" , linux-scsi@vger.kernel.org, linux-arm-msm@vger.kernel.org, 'Dolev Raviv' List-Id: linux-arm-msm@vger.kernel.org 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