Re: [PATCH v1 1/8] ufs: add some vendor specific operations

[Subhash Jadavani <subhashj@codeaurora.org>]

On 2016-10-06 22:04, Kiwoong Kim wrote:
> 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@samsung.com>
>> >> > ---
>> >> >  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 ?

Should be ok.


> 
> 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@vger.kernel.org
>> >> 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@vger.kernel.org
>> 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@vger.kernel.org
> 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