Hi, Subhash.
> Hi Kim,
>
> On 2016-10-06 18:38, Kiwoong Kim wrote:
> > Hi, Subhash.
> >
> > Thank for your comments.
> > I added my opinions below.
> >
> > Regards
> >
> >> Hi Kim,
> >>
> >>
> >> On 2016-10-06 01:17, Kiwoong Kim wrote:
> >> > Some UFS host controller may need to require some sequences before
> >> > used clocks are turned on or off.
> >> > e.g) control internal gates in UFS host
> >>
> >>
> >> I believe we are doing couple of other things as well other than
> >> clock control in this patch, such as hibern8 notify, *nexus_t*
> >> notify. You might want to separate each in different patches. Also,
> >> please find more
>
> are you going to divide this patch?
Yes.
>
> >> additional below.
> >>
> >> >
> >> > Signed-off-by: Kiwoong Kim <kwmad.kim@xxxxxxxxxxx>
> >> > ---
> >> > drivers/scsi/ufs/ufshcd.c | 15 +++++++++++++--
> >> > drivers/scsi/ufs/ufshcd.h | 37
> +++++++++++++++++++++++++++++++++++++
> >> > 2 files changed, 50 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> >> > index 05c7456..82c9a40 100644
> >> > --- a/drivers/scsi/ufs/ufshcd.c
> >> > +++ b/drivers/scsi/ufs/ufshcd.c
> >> > @@ -1474,6 +1474,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_vops_set_nexus_t_xfer_req(hba, tag, lrbp->cmd);
> >> > ufshcd_send_command(hba, tag);
> >> > out_unlock:
> >> > spin_unlock_irqrestore(hba->host->host_lock, flags); @@
-1683,6
> >> > +1684,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
> >> > /* Make sure descriptors are ready before ringing the
doorbell */
> >> > wmb();
> >> > spin_lock_irqsave(hba->host->host_lock, flags);
> >> > + ufshcd_vops_set_nexus_t_xfer_req(hba, tag, lrbp->cmd);
> >>
> >> what does "next_t_xfer_req" means? why do we need this? and do you
> >> think
> >> it may be useful/needed for non-exynos UFS host controllers as well?
> >
> > Exynos UFS controller has a logic to determine which slot owns
> > received
> > UPIU,
> > to handle it properly.
> > This only run when software set a exynos-specific SFR to bitwise-1.
> > If the controller sends non-dependent request, such as NOP,
> > software would set the SFR to 0 because it don't need to determine.
>
> Ok, this is very specific to your controller. But i guess still the
> callback names should be generic enough so that it may be used by other
> host controller drivers if required in future.
> Can we think of different name? something like notify_cmd_issue?
Okay, that's good.
> Also, why do we need to pass whole pointer to pass scsi command pointer?
> what all scsi cmnd parameters will be used by exynos driver? may be your
> full patch series might give more idea?
I get it. How about using (void *) type pointer as an additional argument
To pass anything away to platform-specific driver ?
Thank you
Regards
>
>
> >
> > An answer of your question depends on whether other controllers
> > has a logic to function that.
> >
> >>
> >>
> >>
> >> > ufshcd_send_command(hba, tag);
> >> > spin_unlock_irqrestore(hba->host->host_lock, flags);
> >> >
> >> > @@ -2651,6 +2653,8 @@ static int __ufshcd_uic_hibern8_enter(struct
> >> > ufs_hba *hba)
> >> > int ret;
> >> > struct uic_command uic_cmd = {0};
> >> >
> >> > + ufshcd_vops_hibern8_notify(hba, true, PRE_CHANGE);
> >> > +
> >> > uic_cmd.command = UIC_CMD_DME_HIBER_ENTER;
> >> > ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
> >> >
> >> > @@ -2664,7 +2668,8 @@ static int __ufshcd_uic_hibern8_enter(struct
> >> > ufs_hba *hba)
> >> > */
> >> > if (ufshcd_link_recovery(hba))
> >> > ret = -ENOLINK;
> >> > - }
> >> > + } else
> >> > + ufshcd_vops_hibern8_notify(hba, true, POST_CHANGE);
> >> >
> >> > return ret;
> >> > }
> >> > @@ -2687,13 +2692,16 @@ static int ufshcd_uic_hibern8_exit(struct
> >> > ufs_hba *hba)
> >> > struct uic_command uic_cmd = {0};
> >> > int ret;
> >> >
> >> > + ufshcd_vops_hibern8_notify(hba, false, PRE_CHANGE);
> >> > +
> >> > uic_cmd.command = UIC_CMD_DME_HIBER_EXIT;
> >> > ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
> >> > if (ret) {
> >> > dev_err(hba->dev, "%s: hibern8 exit failed. ret =
%d\n",
> >> > __func__, ret);
> >> > ret = ufshcd_link_recovery(hba);
> >> > - }
> >> > + } else
> >> > + ufshcd_vops_hibern8_notify(hba, false, POST_CHANGE);
> >> >
> >> > return ret;
> >> > }
> >> > @@ -4312,6 +4320,7 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba
> >> > *hba, int lun_id, int task_id,
> >> > task_req_upiup->input_param2 = cpu_to_be32(task_id);
> >> >
> >> > /* send command to the controller */
> >> > + ufshcd_vops_set_nexus_t_task_mgmt(hba, free_slot,
tm_function);
>
> Same as above, we need to think of more generic name?
>
>
>
> Thanks,
> Subhash
>
> >>
> >>
> >> Same question as "next_t_xfer_req". what does "next_t_task_mgmt"
> >> means?
> >> why do we need this? and do you think it may be useful/needed for
> >> non-exynos UFS host controllers as well?
> >
> > Please refer to what I mentioned above.
> >
> >>
> >>
> >> > __set_bit(free_slot, &hba->outstanding_tasks);
> >> >
> >> > /* Make sure descriptors are ready before ringing the task
doorbell
> >> > */
> >> > @@ -5389,6 +5398,8 @@ static int __ufshcd_setup_clocks(struct ufs_hba
> >> > *hba, bool on,
> >> > if (!head || list_empty(head))
> >> > goto out;
> >> >
> >> > + ufshcd_vops_pre_setup_clocks(hba, on);
> >> > +
> >>
> >> I don't think we need a different op for this, we can simply use the
> >> existing op and add one more argument to it to indicate
> >> PRE_CHANGE/POST_CHANGE. If you agree with my response to "[PATCH v2]
> >> scsi: ufshcd: fix possible unclocked register access" patch, i can
> >> update send out v3 of my patch and then you can rebase your patch
> >> series
> >> on top of my patch.
> >
> > Okay, I agree with you. Then I assume that you would do that
> > and will change the patch.
> >
> >>
> >>
> >> > list_for_each_entry(clki, head, list) {
> >> > if (!IS_ERR_OR_NULL(clki->clk)) {
> >> > if (skip_ref_clk && !strcmp(clki->name,
"ref_clk"))
> >> > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> >> > index 430bef1..b97f7c2 100644
> >> > --- a/drivers/scsi/ufs/ufshcd.h
> >> > +++ b/drivers/scsi/ufs/ufshcd.h
> >> > @@ -252,6 +252,7 @@ struct ufs_pwr_mode_info {
> >> > * @exit: called to cleanup everything done in init
> >> > * @get_ufs_hci_version: called to get UFS HCI version
> >> > * @clk_scale_notify: notifies that clks are scaled up/down
> >> > + * @pre_setup_clocks: called before controlling used clocks
> >> > * @setup_clocks: called before touching any of the controller
> >> > registers
> >> > * @setup_regulators: called before accessing the host controller
> >> > * @hce_enable_notify: called before and after HCE enable bit is set
> >> > to allow
> >> > @@ -261,6 +262,9 @@ struct ufs_pwr_mode_info {
> >> > * @pwr_change_notify: called before and after a power mode change
> >> > * is carried out to allow vendor spesific
> capabilities
> >> > * to be set.
> >> > + * @set_nexus_t_xfer_req:
> >> > + * @set_nexus_t_task_mgmt:
> >> > + * @hibern8_notify:
> >> > * @suspend: called during host controller PM callback
> >> > * @resume: called during host controller PM callback
> >> > * @dbg_register_dump: used to dump controller debug information
> >> > @@ -273,6 +277,7 @@ struct ufs_hba_variant_ops {
> >> > u32 (*get_ufs_hci_version)(struct ufs_hba *);
> >> > int (*clk_scale_notify)(struct ufs_hba *, bool,
> >> > enum ufs_notify_change_status);
> >> > + int (*pre_setup_clocks)(struct ufs_hba *, bool);
> >> > int (*setup_clocks)(struct ufs_hba *, bool);
> >> > int (*setup_regulators)(struct ufs_hba *, bool);
> >> > int (*hce_enable_notify)(struct ufs_hba *,
> >> > @@ -283,6 +288,10 @@ struct ufs_hba_variant_ops {
> >> > enum
ufs_notify_change_status
> > status,
> >> > struct ufs_pa_layer_attr *,
> >> > struct ufs_pa_layer_attr *);
> >> > + void (*set_nexus_t_xfer_req)(struct ufs_hba *,
> >> > + int, struct scsi_cmnd *);
> >> > + void (*set_nexus_t_task_mgmt)(struct ufs_hba *, int, u8);
> >> > + void (*hibern8_notify)(struct ufs_hba *, u8, bool);
> >> > int (*suspend)(struct ufs_hba *, enum ufs_pm_op);
> >> > int (*resume)(struct ufs_hba *, enum ufs_pm_op);
> >> > void (*dbg_register_dump)(struct ufs_hba *hba);
> >> > @@ -755,6 +764,13 @@ static inline int
> >> > ufshcd_vops_clk_scale_notify(struct ufs_hba *hba,
> >> > return 0;
> >> > }
> >> >
> >> > +static inline int ufshcd_vops_pre_setup_clocks(struct ufs_hba *hba,
> >> > bool on)
> >> > +{
> >> > + if (hba->vops && hba->vops->pre_setup_clocks)
> >> > + return hba->vops->pre_setup_clocks(hba, on);
> >> > + return 0;
> >> > +}
> >> > +
> >> > static inline int ufshcd_vops_setup_clocks(struct ufs_hba *hba, bool
> >> > on)
> >> > {
> >> > if (hba->vops && hba->vops->setup_clocks)
> >> > @@ -799,6 +815,27 @@ static inline int
> >> > ufshcd_vops_pwr_change_notify(struct ufs_hba *hba,
> >> > return -ENOTSUPP;
> >> > }
> >> >
> >> > +static inline void ufshcd_vops_set_nexus_t_xfer_req(struct ufs_hba
> >> > *hba,
> >> > + int tag, struct scsi_cmnd
*cmd)
> >> > +{
> >> > + if (hba->vops && hba->vops->set_nexus_t_xfer_req)
> >> > + return hba->vops->set_nexus_t_xfer_req(hba, tag,
cmd);
> >> > +}
> >> > +
> >> > +static inline void ufshcd_vops_set_nexus_t_task_mgmt(struct ufs_hba
> >> > *hba,
> >> > + int tag, u8 tm_function)
> >> > +{
> >> > + if (hba->vops && hba->vops->set_nexus_t_task_mgmt)
> >> > + return hba->vops->set_nexus_t_task_mgmt(hba, tag,
> >> tm_function);
> >> > +}
> >> > +
> >> > +static inline void ufshcd_vops_hibern8_notify(struct ufs_hba *hba,
> >> > + u8 enter, bool notify)
> >> > +{
> >> > + if (hba->vops && hba->vops->hibern8_notify)
> >> > + return hba->vops->hibern8_notify(hba, enter,
notify);
> >> > +}
> >> > +
> >> > static inline int ufshcd_vops_suspend(struct ufs_hba *hba, enum
> >> > ufs_pm_op op)
> >> > {
> >> > if (hba->vops && hba->vops->suspend)
> >>
> >> --
> >> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> >> a Linux Foundation Collaborative Project
> >> --
> >> 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
>
> --
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> --
> 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
