date:20130812

Re: [PATCH V5 0/4] scsi: ufs: Improve UFS error handling

2013-08-12 Thread Dolev Raviv

Hi,

I tested the new set of patches (V5 1-4) and it works.

Thanks,
Dolev

 The first patch fixes many issues with current task management handling
 in UFSHCD driver. Others improve error handling in various scenarios.

 These patches are rebased on:
 [PATCH 9/9] drivers/scsi/ufs: don't check resource with
 devm_ioremap_resource

 Changes from v4:
   - Addressed comments from Seungwon Jeon on V3
   - Updated with proper locking while ufshcd state changes
   - Retained LUN reset instead of device reset with DME_END_POINT_RESET
   - Removed error handling decisions dependent on OCS value
   - Simplified fatal error handling to abort the requests first
 and then carry out reset.
 Changes from v3:
   - Rebased.
 Changes from v2:
   - [PATCH V3 1/4]: Make the task management command task tag unique
 across SCSI/NOP/QUERY request tags.
   - [PATCH V3 3/4]: While handling device/host reset, wait for
 pending fatal handler to return if running.
 Changes from v1:
   - [PATCH V2 1/4]: Fix a race condition because of overloading
 outstanding_tasks variable to lock the slots. A new variable
 tm_slots_in_use will track which slots are in use by the driver.
   - [PATCH V2 2/4]: Commit text update to clarify the hardware race
 with more details.
   - [PATCH V2 3/4]: Minor cleanup and rebase
   - [PATCH V2 4/4]: Fix a bug - sleeping in atomic context

 Sujit Reddy Thumma (4):
   scsi: ufs: Fix broken task management command implementation
   scsi: ufs: Fix hardware race conditions while aborting a command
   scsi: ufs: Fix device and host reset methods
   scsi: ufs: Improve UFS fatal error handling

  drivers/scsi/ufs/ufshcd.c |  684
 -
  drivers/scsi/ufs/ufshcd.h |   20 +-
  2 files changed, 501 insertions(+), 203 deletions(-)

 --
 QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
 of Code Aurora Forum, hosted by The Linux Foundation.

 --
 To unsubscribe from this list: send the line unsubscribe linux-scsi in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation


--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V5 1/4] scsi: ufs: Fix broken task management command implementation

2013-08-12 Thread Dolev Raviv

Tested-by: Dolev Raviv dra...@codeaurora.org

 Currently, sending Task Management (TM) command to the card might
 be broken in some scenarios as listed below:

 Problem: If there are more than 8 TM commands the implementation
  returns error to the caller.
 Fix: Wait for one of the slots to be emptied and send the command.

 Problem: Sometimes it is necessary for the caller to know the TM service
  response code to determine the task status.
 Fix: Propogate the service response to the caller.

 Problem: If the TM command times out no proper error recovery is
  implemented.
 Fix: Clear the command in the controller door-bell register, so that
  further commands for the same slot don't fail.

 Problem: While preparing the TM command descriptor, the task tag used
  should be unique across SCSI/NOP/QUERY/TM commands and not the
task tag of the command which the TM command is trying to manage.
 Fix: Use a unique task tag instead of task tag of SCSI command.

 Problem: Since the TM command involves H/W communication, abruptly ending
  the request on kill interrupt signal might cause h/w malfunction.
 Fix: Wait for hardware completion interrupt with TASK_UNINTERRUPTIBLE
  set.

 Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org
 ---
  drivers/scsi/ufs/ufshcd.c |  173
 ++---
  drivers/scsi/ufs/ufshcd.h |8 ++-
  2 files changed, 122 insertions(+), 59 deletions(-)

 diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
 index b36ca9a..d7f3746 100644
 --- a/drivers/scsi/ufs/ufshcd.c
 +++ b/drivers/scsi/ufs/ufshcd.c
 @@ -53,6 +53,9 @@
  /* Query request timeout */
  #define QUERY_REQ_TIMEOUT 30 /* msec */

 +/* Task management command timeout */
 +#define TM_CMD_TIMEOUT   100 /* msecs */
 +
  /* Expose the flag value from utp_upiu_query.value */
  #define MASK_QUERY_UPIU_FLAG_LOC 0xFF

 @@ -183,13 +186,35 @@ ufshcd_get_tmr_ocs(struct utp_task_req_desc
 *task_req_descp)
  /**
   * ufshcd_get_tm_free_slot - get a free slot for task management request
   * @hba: per adapter instance
 + * @free_slot: pointer to variable with available slot value
   *
 - * Returns maximum number of task management request slots in case of
 - * task management queue full or returns the free slot number
 + * Get a free tag and lock it until ufshcd_put_tm_slot() is called.
 + * Returns 0 if free slot is not available, else return 1 with tag value
 + * in @free_slot.
   */
 -static inline int ufshcd_get_tm_free_slot(struct ufs_hba *hba)
 +static bool ufshcd_get_tm_free_slot(struct ufs_hba *hba, int *free_slot)
  {
 - return find_first_zero_bit(hba-outstanding_tasks, hba-nutmrs);
 + int tag;
 + bool ret = false;
 +
 + if (!free_slot)
 + goto out;
 +
 + do {
 + tag = find_first_zero_bit(hba-tm_slots_in_use, hba-nutmrs);
 + if (tag = hba-nutmrs)
 + goto out;
 + } while (test_and_set_bit_lock(tag, hba-tm_slots_in_use));
 +
 + *free_slot = tag;
 + ret = true;
 +out:
 + return ret;
 +}
 +
 +static inline void ufshcd_put_tm_slot(struct ufs_hba *hba, int slot)
 +{
 + clear_bit_unlock(slot, hba-tm_slots_in_use);
  }

  /**
 @@ -1700,10 +1725,11 @@ static void ufshcd_slave_destroy(struct
 scsi_device *sdev)
   * ufshcd_task_req_compl - handle task management request completion
   * @hba: per adapter instance
   * @index: index of the completed request
 + * @resp: task management service response
   *
 - * Returns SUCCESS/FAILED
 + * Returns non-zero value on error, zero on success
   */
 -static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index)
 +static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index, u8
 *resp)
  {
   struct utp_task_req_desc *task_req_descp;
   struct utp_upiu_task_rsp *task_rsp_upiup;
 @@ -1724,19 +1750,15 @@ static int ufshcd_task_req_compl(struct ufs_hba
 *hba, u32 index)
   task_req_descp[index].task_rsp_upiu;
   task_result = be32_to_cpu(task_rsp_upiup-header.dword_1);
   task_result = ((task_result  MASK_TASK_RESPONSE)  8);
 -
 - if (task_result != UPIU_TASK_MANAGEMENT_FUNC_COMPL 
 - task_result != UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED)
 - task_result = FAILED;
 - else
 - task_result = SUCCESS;
 + if (resp)
 + *resp = (u8)task_result;
   } else {
 - task_result = FAILED;
 - dev_err(hba-dev,
 - trc: Invalid ocs = %x\n, ocs_value);
 + dev_err(hba-dev, %s: failed, ocs = 0x%x\n,
 + __func__, ocs_value);
   }
   spin_unlock_irqrestore(hba-host-host_lock, flags);
 - return task_result;
 +
 + return ocs_value;
  }

  /**
 @@ -2237,7 +2259,7 @@ static void ufshcd_tmc_handler(struct ufs_hba *hba)

Re: [PATCH V5 2/4] scsi: ufs: Fix hardware race conditions while aborting a command

2013-08-12 Thread Dolev Raviv

Tested-by: Dolev Raviv dra...@codeaurora.org

 There is a possible race condition in the hardware when the abort
 command is issued to terminate the ongoing SCSI command as described
 below:

 - A bit in the door-bell register is set in the controller for a
   new SCSI command.
 - In some rare situations, before controller get a chance to issue
   the command to the device, the software issued an abort command.
 - If the device recieves abort command first then it returns success
   because the command itself is not present.
 - Now if the controller commits the command to device it will be
   processed.
 - Software thinks that command is aborted and proceed while still
   the device is processing it.
 - The software, controller and device may go out of sync because of
   this race condition.

 To avoid this, query task presence in the device before sending abort
 task command so that after the abort operation, the command is guaranteed
 to be non-existent in both controller and the device.

 Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org
 ---
  drivers/scsi/ufs/ufshcd.c |   70
 +++-
  1 files changed, 55 insertions(+), 15 deletions(-)

 diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
 index d7f3746..d4ee48d 100644
 --- a/drivers/scsi/ufs/ufshcd.c
 +++ b/drivers/scsi/ufs/ufshcd.c
 @@ -2485,6 +2485,12 @@ static int ufshcd_host_reset(struct scsi_cmnd *cmd)
   * ufshcd_abort - abort a specific command
   * @cmd: SCSI command pointer
   *
 + * Abort the pending command in device by sending UFS_ABORT_TASK task
 management
 + * command, and in host controller by clearing the door-bell register.
 There can
 + * be race between controller sending the command to the device while
 abort is
 + * issued. To avoid that, first issue UFS_QUERY_TASK to check if the
 command is
 + * really issued and then try to abort it.
 + *
   * Returns SUCCESS/FAILED
   */
  static int ufshcd_abort(struct scsi_cmnd *cmd)
 @@ -2493,7 +2499,8 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
   struct ufs_hba *hba;
   unsigned long flags;
   unsigned int tag;
 - int err;
 + int err = 0;
 + int poll_cnt;
   u8 resp = 0xF;
   struct ufshcd_lrb *lrbp;

 @@ -2501,33 +2508,59 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
   hba = shost_priv(host);
   tag = cmd-request-tag;

 - spin_lock_irqsave(host-host_lock, flags);
 + /* If command is already aborted/completed, return SUCCESS */
 + if (!(test_bit(tag, hba-outstanding_reqs)))
 + goto out;

 - /* check if command is still pending */
 - if (!(test_bit(tag, hba-outstanding_reqs))) {
 - err = FAILED;
 - spin_unlock_irqrestore(host-host_lock, flags);
 + lrbp = hba-lrb[tag];
 + for (poll_cnt = 100; poll_cnt; poll_cnt--) {
 + err = ufshcd_issue_tm_cmd(hba, lrbp-lun, lrbp-task_tag,
 + UFS_QUERY_TASK, resp);
 + if (!err  resp == UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED) {
 + /* cmd pending in the device */
 + break;
 + } else if (!err  resp == UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
 + u32 reg;
 +
 + /*
 +  * cmd not pending in the device, check if it is
 +  * in transition.
 +  */
 + reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
 + if (reg  (1  tag)) {
 + /* sleep for max. 2ms to stabilize */
 + usleep_range(1000, 2000);
 + continue;
 + }
 + /* command completed already */
 + goto out;
 + } else {
 + if (!err)
 + err = resp; /* service response error */
 + goto out;
 + }
 + }
 +
 + if (!poll_cnt) {
 + err = -EBUSY;
   goto out;
   }
 - spin_unlock_irqrestore(host-host_lock, flags);

 - lrbp = hba-lrb[tag];
   err = ufshcd_issue_tm_cmd(hba, lrbp-lun, lrbp-task_tag,
   UFS_ABORT_TASK, resp);
   if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
 - err = FAILED;
 + if (!err)
 + err = resp; /* service response error */
   goto out;
 - } else {
 - err = SUCCESS;
   }

 + err = ufshcd_clear_cmd(hba, tag);
 + if (err)
 + goto out;
 +
   scsi_dma_unmap(cmd);

   spin_lock_irqsave(host-host_lock, flags);
 -
 - /* clear the respective UTRLCLR register bit */
 - ufshcd_utrl_clear(hba, tag);
 -
   __clear_bit(tag, hba-outstanding_reqs);
   hba-lrb[tag].cmd = NULL;
   spin_unlock_irqrestore(host-host_lock, flags);
 @@ -2535,6 +2568,13 @@ static int ufshcd_abort(struct

Re: [PATCH V5 3/4] scsi: ufs: Fix device and host reset methods

2013-08-12 Thread Dolev Raviv

Tested-by: Dolev Raviv dra...@codeaurora.org

 As of now SCSI initiated error handling is broken because,
 the reset APIs don't try to bring back the device initialized and
 ready for further transfers.

 In case of timeouts, the scsi error handler takes care of handling aborts
 and resets. Improve the error handling in such scenario by resetting the
 device and host and re-initializing them in proper manner.

 Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org
 ---
  drivers/scsi/ufs/ufshcd.c |  240
 +++--
  drivers/scsi/ufs/ufshcd.h |2 +
  2 files changed, 189 insertions(+), 53 deletions(-)

 diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
 index d4ee48d..c577e6e 100644
 --- a/drivers/scsi/ufs/ufshcd.c
 +++ b/drivers/scsi/ufs/ufshcd.c
 @@ -69,9 +69,14 @@ enum {

  /* UFSHCD states */
  enum {
 - UFSHCD_STATE_OPERATIONAL,
   UFSHCD_STATE_RESET,
   UFSHCD_STATE_ERROR,
 + UFSHCD_STATE_OPERATIONAL,
 +};
 +
 +/* UFSHCD error handling flags */
 +enum {
 + UFSHCD_EH_IN_PROGRESS = (1  0),
  };

  /* Interrupt configuration options */
 @@ -87,6 +92,16 @@ enum {
   INT_AGGR_CONFIG,
  };

 +#define ufshcd_set_eh_in_progress(h) \
 + (h-eh_flags |= UFSHCD_EH_IN_PROGRESS)
 +#define ufshcd_eh_in_progress(h) \
 + (h-eh_flags  UFSHCD_EH_IN_PROGRESS)
 +#define ufshcd_clear_eh_in_progress(h) \
 + (h-eh_flags = ~UFSHCD_EH_IN_PROGRESS)
 +
 +static void ufshcd_tmc_handler(struct ufs_hba *hba);
 +static void ufshcd_async_scan(void *data, async_cookie_t cookie);
 +
  /*
   * ufshcd_wait_for_register - wait for register value to change
   * @hba - per-adapter interface
 @@ -840,10 +855,25 @@ static int ufshcd_queuecommand(struct Scsi_Host
 *host, struct scsi_cmnd *cmd)

   tag = cmd-request-tag;

 - if (hba-ufshcd_state != UFSHCD_STATE_OPERATIONAL) {
 + spin_lock_irqsave(hba-host-host_lock, flags);
 + switch (hba-ufshcd_state) {
 + case UFSHCD_STATE_OPERATIONAL:
 + break;
 + case UFSHCD_STATE_RESET:
   err = SCSI_MLQUEUE_HOST_BUSY;
 - goto out;
 + goto out_unlock;
 + case UFSHCD_STATE_ERROR:
 + set_host_byte(cmd, DID_ERROR);
 + cmd-scsi_done(cmd);
 + goto out_unlock;
 + default:
 + dev_WARN_ONCE(hba-dev, 1, %s: invalid state %d\n,
 + __func__, hba-ufshcd_state);
 + set_host_byte(cmd, DID_BAD_TARGET);
 + cmd-scsi_done(cmd);
 + goto out_unlock;
   }
 + spin_unlock_irqrestore(hba-host-host_lock, flags);

   /* acquire the tag to make sure device cmds don't use it */
   if (test_and_set_bit_lock(tag, hba-lrb_in_use)) {
 @@ -880,6 +910,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host,
 struct scsi_cmnd *cmd)
   /* issue command to the controller */
   spin_lock_irqsave(hba-host-host_lock, flags);
   ufshcd_send_command(hba, tag);
 +out_unlock:
   spin_unlock_irqrestore(hba-host-host_lock, flags);
  out:
   return err;
 @@ -1495,8 +1526,6 @@ static int ufshcd_make_hba_operational(struct
 ufs_hba *hba)
   if (hba-ufshcd_state == UFSHCD_STATE_RESET)
   scsi_unblock_requests(hba-host);

 - hba-ufshcd_state = UFSHCD_STATE_OPERATIONAL;
 -
  out:
   return err;
  }
 @@ -2245,8 +2274,12 @@ static void ufshcd_err_handler(struct ufs_hba *hba)
   }
   return;
  fatal_eh:
 - hba-ufshcd_state = UFSHCD_STATE_ERROR;
 - schedule_work(hba-feh_workq);
 + /* handle fatal errors only when link is functional */
 + if (hba-ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
 + /* block commands at driver layer until error is handled */
 + hba-ufshcd_state = UFSHCD_STATE_ERROR;
 + schedule_work(hba-feh_workq);
 + }
  }

  /**
 @@ -2411,12 +2444,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba
 *hba, int lun_id, int task_id,
  }

  /**
 - * ufshcd_device_reset - reset device and abort all the pending commands
 + * ufshcd_eh_device_reset_handler - device reset handler registered to
 + *scsi layer.
   * @cmd: SCSI command pointer
   *
   * Returns SUCCESS/FAILED
   */
 -static int ufshcd_device_reset(struct scsi_cmnd *cmd)
 +static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd)
  {
   struct Scsi_Host *host;
   struct ufs_hba *hba;
 @@ -2425,6 +2459,7 @@ static int ufshcd_device_reset(struct scsi_cmnd
 *cmd)
   int err;
   u8 resp = 0xF;
   struct ufshcd_lrb *lrbp;
 + unsigned long flags;

   host = cmd-device-host;
   hba = shost_priv(host);
 @@ -2433,55 +2468,33 @@ static int ufshcd_device_reset(struct scsi_cmnd
 *cmd)
   lrbp = hba-lrb[tag];
   err = ufshcd_issue_tm_cmd(hba, lrbp-lun, 0, UFS_LOGICAL_RESET, resp);
   if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
 - err = FAILED;
 + if (!err)
 + err

Re: [PATCH V5 4/4] scsi: ufs: Improve UFS fatal error handling

2013-08-12 Thread Dolev Raviv

Tested-by: Dolev Raviv dra...@codeaurora.org

 Error handling in UFS driver is broken and resets the host controller
 for fatal errors without re-initialization. Correct the fatal error
 handling sequence according to UFS Host Controller Interface (HCI)
 v1.1 specification.

 o Processed requests which are completed w/wo error are reported to
   SCSI layer and any pending commands that are not started are aborted
   in the controller and re-queued into scsi mid-layer queue.

 o Upon determining fatal error condition the host controller may hang
   forever until a reset is applied. Block SCSI layer for sending new
   requests and apply reset in a separate error handling work.

 o SCSI is informed about the expected Unit-Attention exception from the
   device for the immediate command after a reset so that the SCSI layer
   take necessary steps to establish communication with the device.

 Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org
 ---
  drivers/scsi/ufs/ufshcd.c |  229
 -
  drivers/scsi/ufs/ufshcd.h |   10 ++-
  2 files changed, 149 insertions(+), 90 deletions(-)

 diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
 index c577e6e..4dca9b4 100644
 --- a/drivers/scsi/ufs/ufshcd.c
 +++ b/drivers/scsi/ufs/ufshcd.c
 @@ -79,6 +79,14 @@ enum {
   UFSHCD_EH_IN_PROGRESS = (1  0),
  };

 +/* UFSHCD UIC layer error flags */
 +enum {
 + UFSHCD_UIC_DL_PA_INIT_ERROR = (1  0), /* Data link layer error */
 + UFSHCD_UIC_NL_ERROR = (1  1), /* Network layer error */
 + UFSHCD_UIC_TL_ERROR = (1  2), /* Transport Layer error */
 + UFSHCD_UIC_DME_ERROR = (1  3), /* DME error */
 +};
 +
  /* Interrupt configuration options */
  enum {
   UFSHCD_INT_DISABLE,
 @@ -101,6 +109,8 @@ enum {

  static void ufshcd_tmc_handler(struct ufs_hba *hba);
  static void ufshcd_async_scan(void *data, async_cookie_t cookie);
 +static int ufshcd_reset_and_restore(struct ufs_hba *hba);
 +static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag);

  /*
   * ufshcd_wait_for_register - wait for register value to change
 @@ -1523,9 +1533,6 @@ static int ufshcd_make_hba_operational(struct
 ufs_hba *hba)
   goto out;
   }

 - if (hba-ufshcd_state == UFSHCD_STATE_RESET)
 - scsi_unblock_requests(hba-host);
 -
  out:
   return err;
  }
 @@ -1651,66 +1658,6 @@ static int ufshcd_verify_dev_init(struct ufs_hba
 *hba)
  }

  /**
 - * ufshcd_do_reset - reset the host controller
 - * @hba: per adapter instance
 - *
 - * Returns SUCCESS/FAILED
 - */
 -static int ufshcd_do_reset(struct ufs_hba *hba)
 -{
 - struct ufshcd_lrb *lrbp;
 - unsigned long flags;
 - int tag;
 -
 - /* block commands from midlayer */
 - scsi_block_requests(hba-host);
 -
 - spin_lock_irqsave(hba-host-host_lock, flags);
 - hba-ufshcd_state = UFSHCD_STATE_RESET;
 -
 - /* send controller to reset state */
 - ufshcd_hba_stop(hba);
 - spin_unlock_irqrestore(hba-host-host_lock, flags);
 -
 - /* abort outstanding commands */
 - for (tag = 0; tag  hba-nutrs; tag++) {
 - if (test_bit(tag, hba-outstanding_reqs)) {
 - lrbp = hba-lrb[tag];
 - if (lrbp-cmd) {
 - scsi_dma_unmap(lrbp-cmd);
 - lrbp-cmd-result = DID_RESET  16;
 - lrbp-cmd-scsi_done(lrbp-cmd);
 - lrbp-cmd = NULL;
 - clear_bit_unlock(tag, hba-lrb_in_use);
 - }
 - }
 - }
 -
 - /* complete device management command */
 - if (hba-dev_cmd.complete)
 - complete(hba-dev_cmd.complete);
 -
 - /* clear outstanding request/task bit maps */
 - hba-outstanding_reqs = 0;
 - hba-outstanding_tasks = 0;
 -
 - /* Host controller enable */
 - if (ufshcd_hba_enable(hba)) {
 - dev_err(hba-dev,
 - Reset: Controller initialization failed\n);
 - return FAILED;
 - }
 -
 - if (ufshcd_link_startup(hba)) {
 - dev_err(hba-dev,
 - Reset: Link start-up failed\n);
 - return FAILED;
 - }
 -
 - return SUCCESS;
 -}
 -
 -/**
   * ufshcd_slave_alloc - handle initial SCSI device configurations
   * @sdev: pointer to SCSI device
   *
 @@ -1727,6 +1674,9 @@ static int ufshcd_slave_alloc(struct scsi_device
 *sdev)
   sdev-use_10_for_ms = 1;
   scsi_set_tag_type(sdev, MSG_SIMPLE_TAG);

 + /* allow SCSI layer to restart the device in case of errors */
 + sdev-allow_restart = 1;
 +
   /*
* Inform SCSI Midlayer that the LUN queue depth is same as the
* controller queue depth. If a LUN queue depth is less than the
 @@ -1930,6 +1880,9 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba,
 struct ufshcd_lrb *lrbp)
   case OCS_ABORTED:
   result |= DID_ABORT  16;
   break;
 + case

Re: [RESEND PATCH 2/4] ARM: msm: Re-organize platsmp to make it extensible

2013-08-12 Thread Mark Rutland

Hi,

Apologies for the long delay for review on this.

I really like the direction this is going, but I have some qualms with
the implementation.

On Fri, Aug 02, 2013 at 03:15:23AM +0100, Rohit Vaswani wrote:
 This makes it easy to add SMP support for new targets
 by adding cpus property and the release sequence.
 We add the enable-method property for the cpus property to
 specify which release sequence to use.
 While at it, add the 8660 cpus bindings to make SMP work.
 
 Signed-off-by: Rohit Vaswani rvasw...@codeaurora.org
 ---
  Documentation/devicetree/bindings/arm/cpus.txt |  6 ++
  Documentation/devicetree/bindings/arm/msm/scss.txt | 15 
  arch/arm/boot/dts/msm8660-surf.dts | 23 +-
  arch/arm/mach-msm/platsmp.c| 94 
 +-
  4 files changed, 115 insertions(+), 23 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/arm/msm/scss.txt
 
 diff --git a/Documentation/devicetree/bindings/arm/cpus.txt 
 b/Documentation/devicetree/bindings/arm/cpus.txt
 index f32494d..327aad2 100644
 --- a/Documentation/devicetree/bindings/arm/cpus.txt
 +++ b/Documentation/devicetree/bindings/arm/cpus.txt
 @@ -44,6 +44,12 @@ For the ARM architecture every CPU node must contain the 
 following properties:
   marvell,mohawk
   marvell,xsc3
   marvell,xscale
 + qcom,scorpion
 +- enable-method: Specifies the method used to enable or take the secondary 
 cores
 +  out of reset. This allows different reset sequence for
 +  different types of cpus.
 +  This should be one of:
 +  qcom,scss

While I'd certainly like to see a move to using enable-method for
32-bit, I think this needs a bit more thought:

What does qcom,scss mean, precisely? It seems to be that we poke some
registers in a qcom,scss device. I think we need to document
enable-methods *very* carefully (and we need to do that for 64-bit as
well with psci), as it's very likely there'll be subtle
incompatibilities between platforms, especially if firmware is involved.
We should try to leaves as little room for error as possible.

I don't want to see this devolve into meaning whatever the Linux driver
for this happens to do at the current point in time, as that just leads
to breakage as we have no clear distinction between actual requirements
and implementation details.

Given we already have platforms without an enable-method, we can't make
it a required property either -- those platforms are already booting by
figuring out an enable method from the platform's compatible string.

With PSCI, enable-method also implies a method for disabling a
particular CPU, so it would be nice for the description to cover this.

  
  Example:
  
 diff --git a/Documentation/devicetree/bindings/arm/msm/scss.txt 
 b/Documentation/devicetree/bindings/arm/msm/scss.txt
 new file mode 100644
 index 000..21c3e26
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/arm/msm/scss.txt
 @@ -0,0 +1,15 @@
 +* SCSS - Scorpion Sub-system
 +
 +Properties
 +
 +- compatible : Should contain qcom,scss.
 +
 +- reg: Specifies the base address for the SCSS registers used for
 +   booting up secondary cores.
 +
 +Example:
 +
 + scss@902000 {
 + compatible = qcom,scss;
 + reg = 0x00902000 0x2000;
 + };
 diff --git a/arch/arm/boot/dts/msm8660-surf.dts 
 b/arch/arm/boot/dts/msm8660-surf.dts
 index cdc010e..203e51a 100644
 --- a/arch/arm/boot/dts/msm8660-surf.dts
 +++ b/arch/arm/boot/dts/msm8660-surf.dts
 @@ -7,6 +7,22 @@
   compatible = qcom,msm8660-surf, qcom,msm8660;
   interrupt-parent = intc;
  
 + cpus {
 + #address-cells = 1;
 + #size-cells = 0;
 + compatible = qcom,scorpion;
 + device_type = cpu;
 + enable-method = qcom,scss;
 +
 + cpu@0 {
 + reg = 0;
 + };
 +
 + cpu@1 {
 + reg = 1;
 + };
 + };
 +

I *really* like moving the common properties out into the /cpus node --
ePAPR suggests it, it's less error prone, and it takes up less space.
However, I'm not sure our generic/arch code handles it properly, and I
think we need to audit that before we can start writing dts that depend
on it. I'd be happy to be wrong here if anyone can correct me. :)

We probably need to factor out the way we read properties for cpu nodes,
falling back to ones present in /cpus if not present. There's already a
lot of pain getting the node for a logical (rather than physical) cpu
id.

Sudeep recently factored out finding the cpu node for a logical cpu id
[1,2] in generic code with a per-arch callback, it shouldn't be too hard
to have shims around that to read (optionally) common properties.

[...]

 -static void prepare_cold_cpu(unsigned int cpu)
 +static int scorpion_release_secondary(void)
  {
 - int ret;
 - ret =

Re: [PATCH 3/4] ARM: msm: Add SMP support for 8960

2013-08-12 Thread Mark Rutland

On Fri, Aug 02, 2013 at 03:15:24AM +0100, Rohit Vaswani wrote:
 Add the cpus bindings and the Krait release sequence
 to make SMP work for MSM8960
 
 Signed-off-by: Rohit Vaswani rvasw...@codeaurora.org
 ---
  Documentation/devicetree/bindings/arm/cpus.txt |  2 +
  Documentation/devicetree/bindings/arm/msm/kpss.txt | 16 ++
  arch/arm/boot/dts/msm8960-cdp.dts  | 22 +
  arch/arm/mach-msm/platsmp.c| 57 
 ++
  arch/arm/mach-msm/scm-boot.h   |  8 +--
  5 files changed, 102 insertions(+), 3 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/arm/msm/kpss.txt
 
 diff --git a/Documentation/devicetree/bindings/arm/cpus.txt 
 b/Documentation/devicetree/bindings/arm/cpus.txt
 index 327aad2..1132eac 100644
 --- a/Documentation/devicetree/bindings/arm/cpus.txt
 +++ b/Documentation/devicetree/bindings/arm/cpus.txt
 @@ -45,11 +45,13 @@ For the ARM architecture every CPU node must contain the 
 following properties:
   marvell,xsc3
   marvell,xscale
   qcom,scorpion
 + qcom,krait
  - enable-method: Specifies the method used to enable or take the secondary 
 cores
out of reset. This allows different reset sequence for
different types of cpus.
This should be one of:
qcom,scss
 +  qcom,kpssv1

Hopefully (though this series implies otherwise) we won't have an
explosion of enable-methods. We haven't listed any common ones yet (e.g.
PSCI), and both this and qcom,scss are poke some cpu-specific
registers.

I take it by the v1 suffix you're expecting more variation here?

  
  Example:
  
 diff --git a/Documentation/devicetree/bindings/arm/msm/kpss.txt 
 b/Documentation/devicetree/bindings/arm/msm/kpss.txt
 new file mode 100644
 index 000..7272340
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/arm/msm/kpss.txt
 @@ -0,0 +1,16 @@
 +* KPSS - Krait Processor Sub-system
 +
 +Properties
 +
 +- compatible : Should contain qcom,kpss.
 +
 +- reg: Specifies the base address for the KPSS registers used for
 +   booting up secondary cores.
 +
 +Example:
 +
 + kpss@2088000 {
 + compatible = qcom,kpss;
 + reg = 0x02088000 0x1000
 + 0x02098000 0x2000;
 + };

What's the secondary bank of registers? The binding only mentions one...

Is this a register bank per-cpu? There's no linkage to CPU ID, which
means that handling logical mapping is going to get quite painful.

For the vaguely standard spin-table enable-method, the address to poke
(cpu-release-addr) may be stored inside a specific cpu node. Following
that style may make more sense here, unless the kpss hardware is used
for anything more than processor hotplug.

We could have the cpu node refer to the specific kpss/register combo,
which would also allow for future expansion if the kpss unit is
per-cluster:

/ {
cpus {
device_type = cpu;
compatible = qcom,krait;
enable-method = qcom,kpssv1;

cpu@0 {
reg = 0;
qcom,kpss-reg = kpss 1; /* reg[1] in kpss */
};

cpu@1 {
reg = 1;
qcom,kpss-reg = kpss 0; /* reg[0] in kpss */
};
}

kpss: kpss@2088000 {
compatible = qcom,kpss;
reg = 0x02088000 0x1000,
  0x02098000 0x2000;
};
}

 diff --git a/arch/arm/boot/dts/msm8960-cdp.dts 
 b/arch/arm/boot/dts/msm8960-cdp.dts
 index db2060c..8c82d5e 100644
 --- a/arch/arm/boot/dts/msm8960-cdp.dts
 +++ b/arch/arm/boot/dts/msm8960-cdp.dts
 @@ -7,6 +7,22 @@
   compatible = qcom,msm8960-cdp, qcom,msm8960;
   interrupt-parent = intc;
  
 + cpus {
 + #address-cells = 1;
 + #size-cells = 0;
 + compatible = qcom,krait;
 + device_type = cpu;
 + enable-method = qcom,kpssv1;
 +
 + cpu@0 {
 + reg = 0;
 + };
 +
 + cpu@1 {
 + reg = 1;
 + };
 + };

Similarly to my comments on the first patch, I like making properties
shared, but we *need* to have common infrastructure before we can do
things this way.

 +
   intc: interrupt-controller@200 {
   compatible = qcom,msm-qgic2;
   interrupt-controller;
 @@ -37,6 +53,12 @@
   reg = 0xfd51 0x4000;
   };
  
 + kpss@2088000 {
 + compatible = qcom,kpss;
 + reg = 0x02088000 0x1000
 + 0x02098000 0x2000;
 + };
 +
   serial@1644 {
   compatible = qcom,msm-hsuart, qcom,msm-uart;
   reg = 0x1644 0x1000,
 diff --git a/arch/arm/mach-msm/platsmp.c b/arch/arm/mach-msm/platsmp.c
 index 17022e0..82eb079 100644
 ---

Re: [PATCH] hwspinlock/msm: Add support for Qualcomm MSM HW Mutex block

2013-08-12 Thread Stephen Boyd

On 08/10/13 12:11, Ohad Ben-Cohen wrote:
 Otherwise, Stephen - do we have your Ack here? I was happy to see your
 review but not sure what's the latest status.

The smp_mb() should be removed. Otherwise I'm willing to accept that we
only build this driver on ARCH_MSM for now. We can fix that in the
future to be available to hexagon if they ever want it. After that it
just needs a DT review/ack so I'll be happy to add a reviewed-by on v3.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] hwspinlock/msm: Add support for Qualcomm MSM HW Mutex block

2013-08-12 Thread Stephen Boyd

On 07/29/13 15:00, Kumar Gala wrote:
 diff --git a/drivers/hwspinlock/msm_hwspinlock.c 
 b/drivers/hwspinlock/msm_hwspinlock.c
 new file mode 100644
 index 000..dbd9a69
 --- /dev/null
 +++ b/drivers/hwspinlock/msm_hwspinlock.c
 @@ -0,0 +1,150 @@
 +/*
 + * Copyright (c) 2013, The Linux Foundation. All rights reserved.
 + *
 + * This software is licensed under the terms of the GNU General Public
 + * License version 2, as published by the Free Software Foundation, and
 + * may be copied, distributed, and modified under those terms.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + */
 +
 +#include linux/err.h
 +#include linux/kernel.h
 +#include linux/slab.h
 +#include linux/device.h
 +#include linux/module.h
 +#include linux/platform_device.h
 +#include linux/of.h
 +#include linux/of_address.h
 +#include linux/of_device.h
 +#include linux/hwspinlock.h
 +#include linux/io.h
 +
 +#include hwspinlock_internal.h
 +
 +#define SPINLOCK_ID_APPS_PROC1

Is this id only for the apps processor? What about hexagon? Does it need
a different number?

 +#define BASE_ID  0
 +
 +static int msm_hwspinlock_trylock(struct hwspinlock *lock)
 +{
 + void __iomem *lock_addr = lock-priv;
 +
 + writel_relaxed(SPINLOCK_ID_APPS_PROC, lock_addr);
 + smp_mb();
 + return readl_relaxed(lock_addr) == SPINLOCK_ID_APPS_PROC;
 +}
 +
 +static void msm_hwspinlock_unlock(struct hwspinlock *lock)
 +{
 + int lock_owner;

This should probably be u32 to be explicit about the size of the register.

 + void __iomem *lock_addr = lock-priv;
 +
 + lock_owner = readl_relaxed(lock_addr);
 + if (lock_owner != SPINLOCK_ID_APPS_PROC) {
 + pr_err(%s: spinlock not owned by Apps (actual owner is %d)\n,

Maybe you should just say spinlock not owned by us (actual owner is
%d) so that this driver is agnostic to the processor it runs on?

 + __func__, lock_owner);
 + }
 +
 + writel_relaxed(0, lock_addr);
 + smp_mb();
 +}


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND PATCH 4/4] ARM: msm: Add support for 8974 SMP

2013-08-12 Thread Mark Rutland

On Fri, Aug 02, 2013 at 03:15:25AM +0100, Rohit Vaswani wrote:
 Add the cpus bindings and the Kraitv2 release sequence
 to make SMP work for 2 cores on MSM8974.
 
 Signed-off-by: Rohit Vaswani rvasw...@codeaurora.org
 ---
  Documentation/devicetree/bindings/arm/cpus.txt |  1 +
  arch/arm/boot/dts/msm8974.dts  | 23 
  arch/arm/mach-msm/board-dt-8974.c  |  3 +
  arch/arm/mach-msm/platsmp.c| 79 
 ++
  4 files changed, 106 insertions(+)
 
 diff --git a/Documentation/devicetree/bindings/arm/cpus.txt 
 b/Documentation/devicetree/bindings/arm/cpus.txt
 index 1132eac..7c3c677 100644
 --- a/Documentation/devicetree/bindings/arm/cpus.txt
 +++ b/Documentation/devicetree/bindings/arm/cpus.txt
 @@ -52,6 +52,7 @@ For the ARM architecture every CPU node must contain the 
 following properties:
This should be one of:
qcom,scss
qcom,kpssv1
 +  qcom,kpssv2

I guess I should've looked at the whole series before responding, that
answers my prior question about what variation we expect.

  
  Example:
  
 diff --git a/arch/arm/boot/dts/msm8974.dts b/arch/arm/boot/dts/msm8974.dts
 index c31c097..ef35a9b 100644
 --- a/arch/arm/boot/dts/msm8974.dts
 +++ b/arch/arm/boot/dts/msm8974.dts
 @@ -7,6 +7,22 @@
   compatible = qcom,msm8974;
   interrupt-parent = intc;
  
 + cpus {
 + #address-cells = 1;
 + #size-cells = 0;
 + compatible = qcom,krait;
 + device_type = cpu;
 + enable-method = qcom,kpssv2;
 +
 + cpu@0 {
 + reg = 0;
 + };
 +
 + cpu@1 {
 + reg = 1;
 + };
 + };
 +
   intc: interrupt-controller@f900 {
   compatible = qcom,msm-qgic2;
   interrupt-controller;
 @@ -23,4 +39,11 @@
1 1 0xf08;
   clock-frequency = 1920;
   };
 +
 + kpss@f9012000 {
 + compatible = qcom,kpss;
 + reg = 0xf9012000 0x1000,
 +   0xf9088000 0x1000,
 +   0xf9098000 0x1000;
 + };

In the previous examples, the number of CPUs was equal to the number of
kpss reg values. Why does it differ here. Either:

* We always have the extra regsiter here, and it should be described
  even if we don't use it.

* It's a different hardware block, and needs a more specific
  compatible string.

Eitehr way this strengthens my feeling that we need to define the linkage
from a CPU to the portion of the kpss which affects it.

  };
 diff --git a/arch/arm/mach-msm/board-dt-8974.c 
 b/arch/arm/mach-msm/board-dt-8974.c
 index d7f84f2..06119f9 100644
 --- a/arch/arm/mach-msm/board-dt-8974.c
 +++ b/arch/arm/mach-msm/board-dt-8974.c
 @@ -13,11 +13,14 @@
  #include linux/of_platform.h
  #include asm/mach/arch.h
  
 +#include common.h
 +
  static const char * const msm8974_dt_match[] __initconst = {
   qcom,msm8974,
   NULL
  };
  
  DT_MACHINE_START(MSM8974_DT, Qualcomm MSM (Flattened Device Tree))
 + .smp = smp_ops(msm_smp_ops),
   .dt_compat = msm8974_dt_match,
  MACHINE_END
 diff --git a/arch/arm/mach-msm/platsmp.c b/arch/arm/mach-msm/platsmp.c
 index 82eb079..0fdae69 100644
 --- a/arch/arm/mach-msm/platsmp.c
 +++ b/arch/arm/mach-msm/platsmp.c
 @@ -124,6 +124,80 @@ static int msm8960_release_secondary(unsigned int cpu)
   return 0;
  }
  
 +static int msm8974_release_secondary(unsigned int cpu)
 +{
 + void __iomem *reg;
 + void __iomem *l2_saw_base;
 + struct device_node *dn = NULL;
 + unsigned apc_pwr_gate_ctl = 0x14;
 + unsigned reg_val;
 +
 + if (cpu == 0 || cpu = num_possible_cpus())
 + return -EINVAL;
 +
 + dn = of_find_compatible_node(dn, NULL, qcom,kpss);
 + if (!dn) {
 + pr_err(%s : Missing kpss node from device tree\n, __func__);
 + return -ENXIO;
 + }
 +
 + reg = of_iomap(dn, cpu+1);

This looks very fishy given the prior patch being one off from this.
why is reg[0] now different?

 + if (!reg)
 + return -ENOMEM;
 +
 + pr_debug(Starting secondary CPU %d\n, cpu);
 +
 + /* Turn on the BHS, turn off LDO Bypass and power down LDO */
 + reg_val =  0x403f0001;

Magic number?

 + writel_relaxed(reg_val, reg + apc_pwr_gate_ctl);
 +
 + /* complete the above write before the delay */
 + mb();

Use writel?

 + /* wait for the bhs to settle */
 + udelay(1);
 +
 + /* Turn on BHS segments */
 + reg_val |= 0x3f  1;
 + writel_relaxed(reg_val, reg + apc_pwr_gate_ctl);
 +
 + /* complete the above write before the delay */
 + mb();

Use writel again?

 +  /* wait for the bhs to settle */
 + udelay(1);
 +
 + /* Finally turn on the bypass so that BHS supplies power */
 + reg_val |= 0x3f  8;
 + writel_relaxed(reg_val, reg + apc_pwr_gate_ctl);
 +
 + /* enable max phases */

Re: [PATCH] hwspinlock/msm: Add support for Qualcomm MSM HW Mutex block

2013-08-12 Thread Kumar Gala


On Aug 10, 2013, at 2:11 PM, Ohad Ben-Cohen wrote:

 + Grant
 
 On Thu, Aug 1, 2013 at 5:10 PM, Kumar Gala ga...@codeaurora.org wrote:
 On Jul 29, 2013, at 4:40 PM, Stephen Boyd wrote:
 On 07/29, Kumar Gala wrote:
 diff --git a/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt 
 b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
 new file mode 100644
 index 000..ddd6889
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
 
 Maybe this should go under a new hwspinlock directory?
 
 Will look for Ohad to comment on this.
 
 
 @@ -0,0 +1,20 @@
 +Qualcomm MSM Hardware Mutex Block:
 +
 +The hardware block provides mutexes utilized between different 
 processors
 +on the SoC as part of the communication protocol used by these 
 processors.
 +
 +Required properties:
 +- compatible: should be qcom,tcsr-mutex
 +- reg: Should contain registers location and length of mutex registers
 +- reg-names:
 +  mutex-base  - string to identify mutex registers
 +- qcom,num-locks: the number of locks/mutexes supported
 +
 +Example:
 +
 +  qcom,tcsr-mutex@fd484000 {
 
 Maybe it should be hw-mutex@fd484000?
 
 again, will look for Ohad to make some comment on this.
 
 +  compatible = qcom,tcsr-mutex;
 +  reg = 0xfd484000 0x1000;
 +  reg-names = mutex-base;
 +  qcom,num-locks = 32;
 +  };
 
 Ohad, ping.
 
 I'd prefer a DT maintainer to take a look at your two open questions
 above, especially if I'm to merge a new file into the devicetree
 Documentation folder.
 
 Grant, any chance you have a moment to take a look?
 
 Otherwise, Stephen - do we have your Ack here? I was happy to see your
 review but not sure what's the latest status.
 
 Thanks,
 Ohad.
 --

So I think I'd ask you to recommend a name, should we just us 'hwspinlock'.  
The general view from ePAPR and dts is the node name should be a bit more 
generic (like ethernet or pci).  So what would you suggest?

- k

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by 
The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 2/2] usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET

2013-08-12 Thread Felipe Balbi

Hi,

On Fri, Aug 09, 2013 at 11:04:48AM -0400, Alan Stern wrote:
heh, it doesn't need to be entirely in the core. Core could have the
generic calls and HCDs could implement some callbacks, but I think quite
a bit of the code will be similar if we implement the same thing on all
HCDs.
   
   What generic calls and callbacks would you suggest?  I assume you want 
   enough to cover not just this one test but the entire USB-CV suite.
  
  maybe a single callback for supporting 'testmodes' ? which receives the
  test mode as argument ?
 
 I don't have a clear picture of how you would apply such an approach to 
 this case.  There would have to be a way to tell the HCD to insert a
 15-second delay between the Setup and Data stages of a particular
 control URB.  How would you do that?  Whatever method you choose,

each test is started after enumerating a known Vid/Pid pair. Based on
that, you *know* which test to run.

 implementing it in every HCD would be a huge amount of work.

sure, we can support different HCDs with time, but once SINGLE_STEP is
implemented in e.g. EHCI, it should be simple to port it to
OHCI/xHCI/MUSB/etc.

 What other test modes would you want to support?

anything that USB[23]0CV supports today. There are even link layer tests
for USB3 and a bunch of others. This SINGLE_STEP_SET_FEATURE is but one
of a large(-ish) test suite which needs to be supported.

 Is it worth adding this support to the standard host controller
 drivers, or should there be a special version (a Kconfig option like
 CONFIG_RCU_TORTURE_TEST) to enable it?  Putting a lot of testing code
 in distribution kernels where it will never be used seems like a big
 waste.

right, I think it should be hidden by Kconfig, not arguing against that.

-- 
balbi


signature.asc
Description: Digital signature

Re: [RFC PATCH v2 1/3] usb: dwc3: msm: Add device tree binding information

2013-08-12 Thread Felipe Balbi

On Fri, Aug 09, 2013 at 10:31:58AM -0500, Kumar Gala wrote:
 
 On Aug 9, 2013, at 4:53 AM, Ivan T. Ivanov wrote:
 
  From: Ivan T. Ivanov iiva...@mm-sol.com
  
  MSM USB3.0 core wrapper consist of USB3.0 IP (SNPS)
 
 probably good to spell out Synopsys rather than SNPS

Synopsys (the company) has always used snps in their bindings though.

  +Required properities :
  +- compatible : sould be qcom,dwc3-hsphy;
  +- reg : offset and length of the register set in the memory map
  +- clocks : phandles to clock instances of the device tree nodes
  +- clock-names :
  +   xo : External reference clock 19 MHz
  +   sleep_a_clk : Sleep clock, used when USB3 core goes into low
  +   power mode (U3).
  +supply-name-supply : phandle to the regulator device tree node
  +Required supply-name are:
  +   v1p8 : 1.8v supply for HSPHY
  +   v3p3 : 3.3v supply for HSPHY
  +   vbus : vbus supply for host mode
  +   vddcx : vdd supply for HS-PHY digital circuit operation

I believe these regulators belong to the PHY, not DWC3. Please write a
PHY driver.

  +Required properities :
  +- compatible : sould be qcom,dwc3-ssphy;
  +- reg : offset and length of the register set in the memory map
  +- clocks : phandles to clock instances of the device tree nodes
  +- clock-names :
  +   xo : External reference clock 19 MHz
  +   ref_clk : Reference clock - used in host mode.
  +supply-name-supply : phandle to the regulator device tree node
  +Required supply-name are:
  +   v1p8 : 1.8v supply for SS-PHY
  +   vddcx : vdd supply for SS-PHY digital circuit operation

likewise, these regulators should be moved to the PHY driver.

  +Required properties :
  +- compatible : should be qcom,dwc3
  +- reg : offset and length of the register set in the memory map
  +   offset and length of the TCSR register for routing USB
  +   signals to either picoPHY0 or picoPHY1.
  +- clocks : phandles to clock instances of the device tree nodes
  +- clock-names :
  +   core_clk : Master/Core clock, have to be = 125 MHz for SS
  +   operation and = 60MHz for HS operation
  +   iface_clk : System bus AXI clock
  +   sleep_clk : Sleep clock, used when USB3 core goes into low
  +   power mode (U3).
  +   utmi_clk : Generated by HS-PHY. Used to clock the low power
  +   parts of thr HS Link layer.
  +
  +Optional properties :
  +- gdsc-supply : phandle to the globally distributed switch controller
  +  regulator node to the USB controller.
  +
  +Sub nodes:
  +- Sub node for DWC3 USB3 controller.
  +  This sub node is required property for device node. The properties
  +  of this subnode are specified in dwc3.txt.
 
 Is tx-fifo-resize required for qcom,dwc3? if so we should call that
 out in the binding.

AFAICT that's only needed for OMAP5 ES1.0. Unless Qualcomm also screwed
up default TX FIFO sizes :-p

-- 
balbi


signature.asc
Description: Digital signature

Re: [RFC/PATCH 2/2] usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET

2013-08-12 Thread Alan Stern

On Mon, 12 Aug 2013, Felipe Balbi wrote:

   maybe a single callback for supporting 'testmodes' ? which receives the
   test mode as argument ?
  
  I don't have a clear picture of how you would apply such an approach to 
  this case.  There would have to be a way to tell the HCD to insert a
  15-second delay between the Setup and Data stages of a particular
  control URB.  How would you do that?  Whatever method you choose,
 
 each test is started after enumerating a known Vid/Pid pair. Based on
 that, you *know* which test to run.

That's not what I meant.  Yes, the test-device driver knows what test
it wants to run, based on the VID/PID.  I was asking how it would
communicate this knowledge to the HCD.

For example, it doesn't make sense to have a callback that means 
Insert a 15-second delay into the next URB that I submit, because the 
HCD doesn't know where URBs come from.

  What other test modes would you want to support?
 
 anything that USB[23]0CV supports today. There are even link layer tests
 for USB3 and a bunch of others. This SINGLE_STEP_SET_FEATURE is but one
 of a large(-ish) test suite which needs to be supported.

That's what I'm trying to find out.  What are the special features that 
we would need to implement in order to support the entire test suite?

  Is it worth adding this support to the standard host controller
  drivers, or should there be a special version (a Kconfig option like
  CONFIG_RCU_TORTURE_TEST) to enable it?  Putting a lot of testing code
  in distribution kernels where it will never be used seems like a big
  waste.
 
 right, I think it should be hidden by Kconfig, not arguing against that.

Therefore we both agree the $SUBJECT patch should not be accepted in
its current form.  At the very least, the new code in ehci-hcd should
be conditional on a CONFIG_USB_HCD_TEST_MODE option.  In addition, we
may want some of the work (though at this point I'm not still clear on
exactly what parts) to be moved into usbcore.

Alan Stern

--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 09/14] clk: msm: Add support for MSM8960's global clock controller (GCC)

2013-08-12 Thread Stephen Boyd

On 08/08, Mark Rutland wrote:
 Hi Stephen,
 
 On Thu, Jul 25, 2013 at 01:43:37AM +0100, Stephen Boyd wrote:
  Fill in the data and wire up the global clock controller to the
  MSM clock driver. This should allow most non-multimedia device
  drivers to control their clocks on 8960 based platforms.
  
  Cc: devicet...@vger.kernel.org
  Signed-off-by: Stephen Boyd sb...@codeaurora.org
  ---
   .../devicetree/bindings/clock/qcom,gcc.txt |  55 +++
   drivers/clk/msm/Kconfig|  10 ++
   drivers/clk/msm/Makefile   |   2 +
   drivers/clk/msm/core.c |   3 +
   drivers/clk/msm/gcc-8960.c | 174 
  +
   drivers/clk/msm/internal.h |   2 +
   6 files changed, 246 insertions(+)
   create mode 100644 Documentation/devicetree/bindings/clock/qcom,gcc.txt
   create mode 100644 drivers/clk/msm/gcc-8960.c
  
  diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc.txt 
  b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
  new file mode 100644
  index 000..2311e1a
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
  @@ -0,0 +1,55 @@
  +MSM Global Clock Controller Binding
  +---
  +
  +Required properties :
  +- compatible : shall contain at least qcom,gcc and only one of the
  +  following:
  +
  +   qcom,gcc-8660
  +   qcom,gcc-8960
  +
  +- reg : shall contain base register location and length
  +- clocks : shall contain clocks supplied by the clock controller
  +
  +Example:
  +   clock-controller@90 {
  +   compatible = qcom,gcc-8960, qcom,gcc;
  +   reg = 0x90 0x4000;
  +
  +   clocks {
  +   pxo: pxo {
  +   #clock-cells = 0;
  +   compatible = fixed-clock;
  +   clock-frequency = 2700;
  +   };
  +
  +   pll8: pll8 {
  +   #clock-cells = 0;
  +   compatible = qcom,pll;
  +   clocks = pxo;
  +   };
  +
  +   vpll8: vpll8 {
  +   #clock-cells = 0;
  +   compatible = qcom,pll-vote;
  +   clocks = pll8;
  +   };
  +
  +   gsbi5_uart_rcg: gsbi5_uart_rcg {
  +   #clock-cells = 0;
  +   compatible = qcom,p2-mn16-clock;
  +   clocks = pxo, vpll8;
  +   };
  +
  +   gsbi5_uart_clk: gsbi5_uart_cxc {
  +   #clock-cells = 0;
  +   compatible = qcom,cxc-clock;
  +   clocks = gsbi5_uart_rcg;
  +   };
  +
  +   gsbi5_uart_ahb: gsbi5_uart_ahb {
  +   #clock-cells = 0;
  +   compatible = qcom,cxc-hg-clock;
  +   };
  +   };
  +   };
 
 I'm slightly confused by this. How is each of the clocks described in
 the clocks node related to a portion of the register set?

The registers to control clocks and determine their state are
scattered throughout the registers in the gcc (in this example
from 0x90 to 0x903fff). If you match up gsbi5_uart_rcg with
its C struct counterpart you'll notice that there are multiple
registers used to configure the clock. It isn't as simple as one
reg property per clock even for the case where we're just
toggling a bit to turn a clock on and off either. And it isn't as
simple as saying the clock has a base register that we can offset
from because offsets are almost always different (we've tried
to correct this in future chip versions).

 
 If the set of clocks is fixed, surely the gcc node gives you enough
 information alone, and the whole block can be modelled as a single
 provider of multiple clock outputs, or it's not fixed, and some linkage
 needs to be defined?
 
 The code seems to imply the former, unless only a subset of clocks may
 be present? In that case, the set of clocks which might be present
 should be described in the binding.

The clock controller is hardware and the number of clock outputs
is fixed. Isn't all hardware fixed until you start talking about
FPGAs? The next minor revision of the clock controller may add
more clocks or remove clocks from that base design, but otherwise
the two are 90% the same and generally software compatible. It
isn't until we start a new generation of chips that we make major
changes to the design. Is that loose enough to qualify?

These bindings attempt to follow the regulator bindings. With
regulators there is a

Re: [PATCH v1 03/14] clk: Add of_clk_match() for device drivers

2013-08-12 Thread Stephen Boyd

On 08/12, Mike Turquette wrote:
 Quoting Stephen Boyd (2013-07-24 17:43:31)
  In similar fashion as of_regulator_match() add an of_clk_match()
  function that finds an initializes clock init_data structs from
  devicetree. Drivers should use this API to find clocks that their
  device is providing and then iterate over their match table
  registering the clocks with the init data parsed.
  
  Signed-off-by: Stephen Boyd sb...@codeaurora.org
 
 Stephen,
 
 In general I like this approach. Writing real device drivers for clock
 controllers is The Right Way and of_clk_match helps.
 
 Am I reading this correctly that the base register addresses/offsets for
 the clock nodes still come from C files? For example I still see
 pll8_desc defining reg stuff in drivers/clk/msm/gcc-8960.c.

I think we may be able to put the registers in DT but I don't
know why we need to do that if we're already matching up nodes
with C structs. It also made me want to introduce devm_of_iomap()
which just seemed wrong (if you have a dev struct why can't you
use devm_ioremap()).

 
 What do you think about fetching this data from DT? My thinking here is
 that the definition of that PLL structure would be in C, as would all of
 the control logic. But any per-clock data whatsoever should live in DTS.
 This means the clock data you supply in the DTS files in patches #9 and
 #10 would have base addresses or offsets per-clock. I think this echoes
 Mark R's concerns as well.
 
 In the future if new chips have more of that type of PLL it would not
 require changes to your clock driver, only new DTS data for the new
 chip.
 
 I could have that wrong though, there is a fair amount of indirection in
 this series...

Let's take the PLL example and see if I follow what would be in
DT and what would be in C.

Right now we have

pll8: pll8 {
#clock-cells = 0;
compatible = qcom,pll;
clocks = pxo;
};

in DT and

static struct pll_desc pll8_desc = {
.l_reg = 0x3144,
.m_reg = 0x3148,
.n_reg = 0x314c,
.config_reg = 0x3154,
.mode_reg = 0x3140,
.status_reg = 0x3158,
.status_bit = 16,
};

in C. Do you want everything to be in DT? Something like:

pll8: pll8@3140 {
#clock-cells = 0;
compatible = qcom,pll;
clocks = pxo;
reg = 0x3140 0x20;
};

and then assume that all those registers are offset from the base
register and that the status bit is 16 (it usually is but not
always)?

The problem I see is this quickly breaks down with more
complicated clocks like the RCGs.

We have

gsbi5_uart_rcg: gsbi5_uart_rcg {
#clock-cells = 0;
compatible = qcom,p2-mn16-clock;
clocks = pxo, vpll8;
};

in DT and

static struct freq_tbl clk_tbl_gsbi_uart[] = {
{  1843200, PLL8, 2,  6, 625 },
{  3686400, PLL8, 2, 12, 625 },
{  7372800, PLL8, 2, 24, 625 },
{ 14745600, PLL8, 2, 48, 625 },
{ 1600, PLL8, 4,  1,   6 },
{ 2400, PLL8, 4,  1,   4 },
{ 3200, PLL8, 4,  1,   3 },
{ 4000, PLL8, 1,  5,  48 },
{ 4640, PLL8, 1, 29, 240 },
{ 4800, PLL8, 4,  1,   2 },
{ 5120, PLL8, 1,  2,  15 },
{ 5600, PLL8, 1,  7,  48 },
{ 58982400, PLL8, 1, 96, 625 },
{ 6400, PLL8, 2,  1,   3 },
{ }
};

static struct rcg_desc gsbi5_uart_rcg = {
.ctl_reg = 0x2a54,
.ns_reg = 0x2a54,
.md_reg = 0x2a50,
.ctl_bit = 11,
.mnctr_en_bit = 8,
.mnctr_reset_bit = 7,
.mnctr_mode_shift = 5,
.pre_div_shift = 3,
.src_sel_shift = 0,
.n_val_shift = 16,
.m_val_shift = 16,
.parent_map = gcc_pxo_pll8_map,
.freq_tbl = clk_tbl_gsbi_uart,
};

in C. It starts to get pretty unwieldy when you put this all in
DT, plus you'll notice that the ns_reg and ctl_reg are the same
here because we've generalized the code to work with different
types of software interfaces (technically this clock has no ctl
register, just an NS and MD register). Our multimedia clock
controllers don't follow any standard base/offset pair and so the
ctl_reg can be a different offset from the md_reg depending on
which clock we're talking about. My initial try at translating
this into DT pretty much just made every struct member into a
property, including the duplicate register, expect for the
frequency table, which could probably also be DT-ified with some
work.

gsbi5_uart_rcg: gsbi5_uart_rcg@2a54 {

Re: [PATCH V5 0/4] scsi: ufs: Improve UFS error handling

Re: [PATCH V5 1/4] scsi: ufs: Fix broken task management command implementation

Re: [PATCH V5 2/4] scsi: ufs: Fix hardware race conditions while aborting a command

Re: [PATCH V5 3/4] scsi: ufs: Fix device and host reset methods

Re: [PATCH V5 4/4] scsi: ufs: Improve UFS fatal error handling

Re: [RESEND PATCH 2/4] ARM: msm: Re-organize platsmp to make it extensible

Re: [PATCH 3/4] ARM: msm: Add SMP support for 8960

Re: [PATCH] hwspinlock/msm: Add support for Qualcomm MSM HW Mutex block

Re: [PATCH v2] hwspinlock/msm: Add support for Qualcomm MSM HW Mutex block

Re: [RESEND PATCH 4/4] ARM: msm: Add support for 8974 SMP

Re: [PATCH] hwspinlock/msm: Add support for Qualcomm MSM HW Mutex block

Re: [RFC/PATCH 2/2] usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET

Re: [RFC PATCH v2 1/3] usb: dwc3: msm: Add device tree binding information

Re: [RFC/PATCH 2/2] usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET

Re: [PATCH v1 09/14] clk: msm: Add support for MSM8960's global clock controller (GCC)

Re: [PATCH v1 03/14] clk: Add of_clk_match() for device drivers

16 matches

Site Navigation

Mail list logo

Footer information