Re: [PATCH 1/1] scsi: ufs: Generalize UFS Interconnect Layer (UIC)command support

[Sujit Reddy Thumma]

On 5/2/2013 12:49 PM, Santosh Y wrote:
> > -static inline void
> > -ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
> > +static int ufshcd_send_uic_command(struct ufs_hba *hba,
> > +               struct uic_command *uic_cmnd, int retries)
> >   {
> > +       int err = 0;
> > +       unsigned long flags;
> > +
> > +       if (unlikely(mutex_trylock(&hba->uic_cmd_mutex))) {
> > +               mutex_unlock(&hba->uic_cmd_mutex);
> > +               dev_err(hba->dev, "%s: called without prepare command\n",
> > +                               __func__);
> > +               BUG();
> > +       }
> > +
> > +retry:
> > +       /* check if controller is ready to accept UIC commands */
> > +       if (!(readl(hba->mmio_base + REG_CONTROLLER_STATUS) &
> > +                               UIC_COMMAND_READY)) {
> > +               dev_err(hba->dev, "Controller not ready to accept UIC commands\n");
> > +               err = -EIO;
> > +               goto out;
> > +       }
> > +
> > +       init_completion(&uic_cmnd->completion);
> > +
> > +       spin_lock_irqsave(hba->host->host_lock, flags);
> > +
> > +       /* enable UIC related interrupts */
> > +       if (!(hba->int_enable_mask & UIC_COMMAND_COMPL)) {
> > +               hba->int_enable_mask |= UIC_COMMAND_COMPL;
> > +               ufshcd_int_config(hba, UFSHCD_INT_ENABLE);
> > +       }
> > +
>
> No need to check if the interrupt is already enabled, it can be
> directly enabled.

That would be unnecessary device access (writel) every time we send a 
UIC command.

> >          /* Write Args */
> >          writel(uic_cmnd->argument1,
> >                (hba->mmio_base + REG_UIC_COMMAND_ARG_1));
> > @@ -415,6 +491,45 @@ ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
> >          /* Write UIC Cmd */
> >          writel((uic_cmnd->command & COMMAND_OPCODE_MASK),
> >                 (hba->mmio_base + REG_UIC_COMMAND));
> > +
> > +       /* flush to make sure h/w sees the write */
> > +       readl(hba->mmio_base + REG_UIC_COMMAND);
> > +
> > +       spin_unlock_irqrestore(hba->host->host_lock, flags);
> > +
>
> mutex lock/unlock can be done in the send_uic_command, It will
> simplify the process for the caller.

 ufshcd_dme_get_attr()
 {
   1) ufshcd_prepare_uic_command_lck();
   2) ufshcd_send_uic_command();
   3) Read arg3 to know the value of Uni-pro attribute.
   4) ufshcd_unprepare_uic_command_unlck();
 }

Step 3 above is optional for some of the UIC commands but when it is 
required it should be done with mutex lock held as the 
hba->active_uic_cmd might be modified by some one else before value 
present hba->active_uic_cmd->argument3 is read.

But thinking more on this holding lock only in send_uic_command() is 
feasible.

> > +       err = wait_for_completion_timeout(
> > +                       &uic_cmnd->completion, UIC_COMMAND_TIMEOUT);
> > +       if (!err) {
> > +               err = -ETIMEDOUT;
> > +       } else if (uic_cmnd->argument2 & MASK_UIC_COMMAND_RESULT) {
> > +               /* something bad with h/w or arguments, try again */
> > +               if (--retries)
> > +                       goto retry;
> > +

--
Regards,
Sujit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/