On Thursday, April 25, 2013 , Sujit Reddy Thumma wrote:
> On 4/24/2013 9:36 PM, Seungwon Jeon wrote:
> > Link start-up requires long time with multiphase handshakes
> > between UFS host and device. This affects driver's probe time.
> > This patch let link start-up run asynchronously.
> > And completion time of uic command is defined to avoid a
> > permanent wait.
>
> I have similar patch posted few days back "scsi: ufs: Generalize UFS
> Interconnect Layer (UIC) command support" which does a bit more (mutex,
> error handling) than what is done here. Can that be used/improved?
I completed to check your patch to compare it now.
Though it's just my thought, the patch I sent is more intuitive on the whole.
Considering other dme operations which I have introduced, it looks like matched.
Of course, you may disagree.
But I think the part of mutex is needed. It's a good point.
In case of error handling, I didn't catch nothing special.
Rather, handling link lost case is not proper.
When ufs host meets link lost status, it should start with dme_reset not retried dme_linkstartup.
And it would be good if link start-up procedure is done in separate process, not in driver probe.
If it's all right with you, I'd like to update lock mechanism for uic command.
I can add your signed-off. Please let me know your opinion.
Thanks,
Seungwon Jeon
>
> >
> > Signed-off-by: Seungwon Jeon <tgih.jun@xxxxxxxxxxx>
> > ---
> > drivers/scsi/ufs/ufshcd.c | 114 +++++++++++++++++++++++++++++++++-----------
> > drivers/scsi/ufs/ufshcd.h | 6 ++-
> > 2 files changed, 89 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index efe2256..76ff332 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -38,6 +38,7 @@
> > #define UFSHCD_ENABLE_INTRS (UTP_TRANSFER_REQ_COMPL |\
> > UTP_TASK_REQ_COMPL |\
> > UFSHCD_ERROR_MASK)
> > +#define UIC_CMD_TIMEOUT 100
> >
> > enum {
> > UFSHCD_MAX_CHANNEL = 0,
> > @@ -357,13 +358,15 @@ static inline void ufshcd_hba_capabilities(struct ufs_hba *hba)
> > }
> >
> > /**
> > - * ufshcd_send_uic_command - Send UIC commands to unipro layers
> > + * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
> > * @hba: per adapter instance
> > * @uic_command: UIC command
> > */
> > static inline void
> > -ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
> > +ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmnd)
> > {
> > + init_completion(&uic_cmnd->done);
> > +
> > /* Write Args */
> > ufshcd_writel(hba, REG_UIC_COMMAND_ARG_1, uic_cmnd->argument1);
> > ufshcd_writel(hba, REG_UIC_COMMAND_ARG_2, uic_cmnd->argument2);
> > @@ -375,6 +378,45 @@ ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
> > }
> >
> > /**
> > + * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
> > + * @hba: per adapter instance
> > + * @uic_command: UIC command
> > + *
> > + * Returns 0 only if success.
> > + */
> > +static int ufshcd_wait_for_uic_cmd(struct ufs_hba *hba)
> > +{
> > + struct uic_command *uic_cmd = &hba->active_uic_cmd;
> > + int ret;
> > +
> > + if (wait_for_completion_timeout(&uic_cmd->done,
> > + msecs_to_jiffies(UIC_CMD_TIMEOUT)))
> > + ret = ufshcd_get_uic_cmd_result(hba);
> > + else
> > + ret = -ETIMEDOUT;
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * ufshcd_ready_uic_cmd - Check if controller is ready
> > + * to accept UIC commands
> > + * @hba: per adapter instance
> > + * Return true on success, else false
> > + */
> > +static inline bool ufshcd_ready_uic_cmd(struct ufs_hba *hba)
> > +{
> > + if (ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY) {
> > + return true;
> > + } else {
> > + dev_err(hba->dev,
> > + "Controller not ready"
> > + " to accept UIC commands\n");
> > + return false;
> > + }
> > +}
> > +
> > +/**
> > * ufshcd_map_sg - Map scatter-gather list to prdt
> > * @lrbp - pointer to local reference block
> > *
> > @@ -735,15 +777,10 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
> > {
> > struct uic_command *uic_cmd;
> > unsigned long flags;
> > + int ret;
> >
> > - /* check if controller is ready to accept UIC commands */
> > - if (((ufshcd_readl(hba, REG_CONTROLLER_STATUS)) &
> > - UIC_COMMAND_READY) == 0x0) {
> > - dev_err(hba->dev,
> > - "Controller not ready"
> > - " to accept UIC commands\n");
> > + if (!ufshcd_ready_uic_cmd(hba))
> > return -EIO;
> > - }
> >
> > spin_lock_irqsave(hba->host->host_lock, flags);
> >
> > @@ -754,13 +791,16 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
> > uic_cmd->argument2 = 0;
> > uic_cmd->argument3 = 0;
> >
> > - /* enable UIC related interrupts */
> > - ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
> > + /* Dispatching UIC commands to controller */
> > + ufshcd_dispatch_uic_cmd(hba, uic_cmd);
> >
> > - /* sending UIC commands to controller */
> > - ufshcd_send_uic_command(hba, uic_cmd);
> > spin_unlock_irqrestore(hba->host->host_lock, flags);
> > - return 0;
> > +
> > + ret = ufshcd_wait_for_uic_cmd(hba);
> > + if (ret)
> > + dev_err(hba->dev, "link startup: error code %d returned\n", ret);
> > +
> > + return ret;
> > }
> >
> > /**
> > @@ -898,6 +938,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
> > if (ufshcd_hba_enable(hba))
> > return -EIO;
> >
> > + /* enable UIC related interrupts */
> > + ufshcd_enable_intr(hba, UIC_COMMAND_COMPL | UIC_ERROR);
> > +
> > /* Configure UTRL and UTMRL base address registers */
> > ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_BASE_L,
> > lower_32_bits(hba->utrdl_dma_addr));
> > @@ -909,7 +952,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
> > upper_32_bits(hba->utmrdl_dma_addr));
> >
> > /* Initialize unipro link startup procedure */
> > - return ufshcd_dme_link_startup(hba);
> > + schedule_work(&hba->link_startup_wq);
> > +
> > + return 0;
> > }
> >
> > /**
> > @@ -1186,6 +1231,16 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
> > }
> >
> > /**
> > + * ufshcd_uic_cmd_compl - handle completion of uic command
> > + * @hba: per adapter instance
> > + */
> > +static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
> > +{
> > + if (intr_status & UIC_COMMAND_COMPL)
> > + complete(&hba->active_uic_cmd.done);
> > +}
> > +
> > +/**
> > * ufshcd_transfer_req_compl - handle SCSI and query command completion
> > * @hba: per adapter instance
> > */
> > @@ -1225,25 +1280,26 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
> > }
> >
> > /**
> > - * ufshcd_uic_cc_handler - handle UIC command completion
> > + * ufshcd_link_startup - link initialization
> > * @work: pointer to a work queue structure
> > - *
> > - * Returns 0 on success, non-zero value on failure
> > */
> > -static void ufshcd_uic_cc_handler (struct work_struct *work)
> > +static void ufshcd_link_startup(struct work_struct *work)
> > {
> > struct ufs_hba *hba;
> > + int ret;
> >
> > - hba = container_of(work, struct ufs_hba, uic_workq);
> > + hba = container_of(work, struct ufs_hba, link_startup_wq);
> >
> > - if ((hba->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) &&
> > - !(ufshcd_get_uic_cmd_result(hba))) {
> > + ret = ufshcd_dme_link_startup(hba);
> > + if (ret)
> > + goto out;
> >
> > - if (ufshcd_make_hba_operational(hba))
> > - dev_err(hba->dev,
> > - "cc: hba not operational state\n");
> > - return;
> > - }
> > + ret = ufshcd_make_hba_operational(hba);
> > + if (ret)
> > + goto out;
> > + return;
> > +out:
> > + dev_err(hba->dev, "link startup failed %d\n", ret);
> > }
> >
> > /**
> > @@ -1307,7 +1363,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
> > ufshcd_err_handler(hba);
> >
> > if (intr_status & UIC_COMMAND_COMPL)
> > - schedule_work(&hba->uic_workq);
> > + ufshcd_uic_cmd_compl(hba, intr_status);
> >
> > if (intr_status & UTP_TASK_REQ_COMPL)
> > ufshcd_tmc_handler(hba);
> > @@ -1694,7 +1750,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
> > init_waitqueue_head(&hba->ufshcd_tm_wait_queue);
> >
> > /* Initialize work queues */
> > - INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
> > + INIT_WORK(&hba->link_startup_wq, ufshcd_link_startup);
> > INIT_WORK(&hba->feh_workq, ufshcd_fatal_err_handler);
> >
> > /* IRQ registration */
> > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> > index 87d5a94..2fb4d94 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -51,6 +51,7 @@
> > #include <linux/bitops.h>
> > #include <linux/pm_runtime.h>
> > #include <linux/clk.h>
> > +#include <linux/completion.h>
> >
> > #include <asm/irq.h>
> > #include <asm/byteorder.h>
> > @@ -83,6 +84,7 @@ struct uic_command {
> > u32 argument3;
> > int cmd_active;
> > int result;
> > + struct completion done;
> > };
> >
> > /**
> > @@ -140,7 +142,7 @@ struct ufshcd_lrb {
> > * @tm_condition: condition variable for task management
> > * @ufshcd_state: UFSHCD states
> > * @intr_mask: Interrupt Mask Bits
> > - * @uic_workq: Work queue for UIC completion handling
> > + * @link_startup_wq: Work queue for link start-up
> > * @feh_workq: Work queue for fatal controller error handling
> > * @errors: HBA errors
> > */
> > @@ -179,7 +181,7 @@ struct ufs_hba {
> > u32 intr_mask;
> >
> > /* Work Queues */
> > - struct work_struct uic_workq;
> > + struct work_struct link_startup_wq;
> > struct work_struct feh_workq;
> >
> > /* HBA Errors */
> >
>
> --
> Regards,
> Sujit
> --
> 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
--
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
