[v6,2/3] scsi: ufs: Maximum RTT supported by the host driver

Allow platform vendors to take precedence having their own max rtt
support.  This makes sense because the host controller's nortt
characteristic may vary among vendors.

while at it, set this value for Mediatek, as requested by Peter -
https://lore.kernel.org/all/0a57d6bab739d6a10584f2baba115d00dfc9c94c.camel@mediatek.com/

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/ufs/core/ufshcd.c       | 6 +++++-
 drivers/ufs/host/ufs-mediatek.c | 1 +
 drivers/ufs/host/ufs-mediatek.h | 3 +++
 include/ufs/ufshcd.h            | 2 ++
 4 files changed, 11 insertions(+), 1 deletion(-)

On Sun, 2024-05-26 at 11:16 +0300, Avri Altman wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Allow platform vendors to take precedence having their own max rtt
> support.  This makes sense because the host controller's nortt
> characteristic may vary among vendors.
> 
> while at it, set this value for Mediatek, as requested by Peter -
> 
https://lore.kernel.org/all/0a57d6bab739d6a10584f2baba115d00dfc9c94c.camel@mediatek.com/
> 
> Signed-off-by: Avri Altman <avri.altman@wdc.com>
> ---
>  drivers/ufs/core/ufshcd.c       | 6 +++++-
>  drivers/ufs/host/ufs-mediatek.c | 1 +
>  drivers/ufs/host/ufs-mediatek.h | 3 +++
>  include/ufs/ufshcd.h            | 2 ++
>  4 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 7df8bcacbe7e..b62023a6c306 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8144,7 +8144,11 @@ static void ufshcd_set_rtt(struct ufs_hba
> *hba)
>  	if (dev_rtt != DEFAULT_MAX_NUM_RTT)
>  		return;
>  
> -	rtt = min_t(int, dev_info->rtt_cap, hba->nortt);
> +	if (hba->vops && hba->vops->max_num_rtt)
> +		rtt = hba->vops->max_num_rtt;
> +	else
> +		rtt = min_t(int, dev_info->rtt_cap, hba->nortt);
> +
>  	if (rtt == dev_rtt)
>  		return;
>  
> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-
> mediatek.c
> index c4f997196c57..c7a0ab9b1f59 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -1785,6 +1785,7 @@ static int ufs_mtk_config_esi(struct ufs_hba
> *hba)
>   */
>  static const struct ufs_hba_variant_ops ufs_hba_mtk_vops = {
>  	.name                = "mediatek.ufshci",
> +	.max_num_rtt         = MTK_MAX_NUM_RTT,
>  	.init                = ufs_mtk_init,
>  	.get_ufs_hci_version = ufs_mtk_get_ufs_hci_version,
>  	.setup_clocks        = ufs_mtk_setup_clocks,
> diff --git a/drivers/ufs/host/ufs-mediatek.h b/drivers/ufs/host/ufs-
> mediatek.h
> index 3ff17e95afab..05d76a6bd772 100644
> --- a/drivers/ufs/host/ufs-mediatek.h
> +++ b/drivers/ufs/host/ufs-mediatek.h
> @@ -189,4 +189,7 @@ struct ufs_mtk_host {
>  /* MTK delay of autosuspend: 500 ms */
>  #define MTK_RPM_AUTOSUSPEND_DELAY_MS 500
>  
> +/* MTK RTT support number */
> +#define MTK_MAX_NUM_RTT 2
> +
>  #endif /* !_UFS_MEDIATEK_H */
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index d74bd2d67b06..ef04ec8aad69 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -295,6 +295,7 @@ struct ufs_pwr_mode_info {
>  /**
>   * struct ufs_hba_variant_ops - variant specific callbacks
>   * @name: variant name
> + * @max_num_rtt: maximum RTT supported by the host
>   * @init: called when the driver is initialized
>   * @exit: called to cleanup everything done in init
>   * @get_ufs_hci_version: called to get UFS HCI version
> @@ -332,6 +333,7 @@ struct ufs_pwr_mode_info {
>   */
>  struct ufs_hba_variant_ops {
>  	const char *name;
> +	int	max_num_rtt;
>  	int	(*init)(struct ufs_hba *);
>  	void    (*exit)(struct ufs_hba *);
>  	u32	(*get_ufs_hci_version)(struct ufs_hba *);
> -- 
> 2.34.1

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

On 5/26/24 01:16, Avri Altman wrote:
> -	rtt = min_t(int, dev_info->rtt_cap, hba->nortt);
> +	if (hba->vops && hba->vops->max_num_rtt)
> +		rtt = hba->vops->max_num_rtt;
> +	else
> +		rtt = min_t(int, dev_info->rtt_cap, hba->nortt);
> +

Shouldn't what the controller supports be compared with what the device supports,
e.g. as follows?

min_t(int, dev_info->rtt_cap, hba->vops->max_num_rtt ? : hba->nortt);

>   struct ufs_hba_variant_ops {
>   	const char *name;
> +	int	max_num_rtt;

Hmm ... why 'int' instead of an unsigned type? If the type would be changed
into 'u8' (the type of rtt_cap) then the above min_t() can be changed into
min().

Thanks,

Bart.

> 
> On 5/26/24 01:16, Avri Altman wrote:
> > -     rtt = min_t(int, dev_info->rtt_cap, hba->nortt);
> > +     if (hba->vops && hba->vops->max_num_rtt)
> > +             rtt = hba->vops->max_num_rtt;
> > +     else
> > +             rtt = min_t(int, dev_info->rtt_cap, hba->nortt);
> > +
> 
> Shouldn't what the controller supports be compared with what the device
> supports, e.g. as follows?
> 
> min_t(int, dev_info->rtt_cap, hba->vops->max_num_rtt ? : hba->nortt);
Yes, this is an option too.
The one that I proposed allows to entirely overrides the negotiation.
I think your suggestion is better.
Will change.

> 
> >   struct ufs_hba_variant_ops {
> >       const char *name;
> > +     int     max_num_rtt;
> 
> Hmm ... why 'int' instead of an unsigned type? If the type would be changed
> into 'u8' (the type of rtt_cap) then the above min_t() can be changed into
> min().
Nortt is 0 based, meaning it can be 256, which some of the platforms in the market do use,  so u8 is not enough.

Thanks,
Avri

> 
> Thanks,
> 
> Bart.