| Message ID | 20240523125827.818-3-avri.altman@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: ufs: Allow RTT negotiation | expand |
On Thu, May 23, 2024 at 03:58:25PM +0300, Avri Altman wrote: > Allow platform vendors to take precedence having their own rtt > negotiation mechanism. This makes sense because the host controller's > nortt characteristic may vary among vendors. Platform vendors have absolutelyt no business saying anything. Fortunately that's not what you're actually doing, but I really don't understand your vendor fetish.
> On Thu, May 23, 2024 at 03:58:25PM +0300, Avri Altman wrote: > > Allow platform vendors to take precedence having their own rtt > > negotiation mechanism. This makes sense because the host controller's > > nortt characteristic may vary among vendors. > > Platform vendors have absolutelyt no business saying anything. > > Fortunately that's not what you're actually doing, but I really don't understand > your vendor fetish. It was a specific request from MTK to allow override their host controller capabilities. Thanks, Avri
On Thu, May 23, 2024 at 01:09:25PM +0000, Avri Altman wrote: > > On Thu, May 23, 2024 at 03:58:25PM +0300, Avri Altman wrote: > > > Allow platform vendors to take precedence having their own rtt > > > negotiation mechanism. This makes sense because the host controller's > > > nortt characteristic may vary among vendors. > > > > Platform vendors have absolutelyt no business saying anything. > > > > Fortunately that's not what you're actually doing, but I really don't understand > > your vendor fetish. > It was a specific request from MTK to allow override their host controller capabilities. Then they need to submit a patch just like anyone who wants to improve Linux. And not trick their NAND supplier into adding an unused hook…
On Thu, 2024-05-23 at 06:11 -0700, hch@infradead.org wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On Thu, May 23, 2024 at 01:09:25PM +0000, Avri Altman wrote: > > > On Thu, May 23, 2024 at 03:58:25PM +0300, Avri Altman wrote: > > > > Allow platform vendors to take precedence having their own rtt > > > > negotiation mechanism. This makes sense because the host > controller's > > > > nortt characteristic may vary among vendors. > > > > > > Platform vendors have absolutelyt no business saying anything. > > > > > > Fortunately that's not what you're actually doing, but I really > don't understand > > > your vendor fetish. > > It was a specific request from MTK to allow override their host > controller capabilities. > > Then they need to submit a patch just like anyone who wants to > improve > Linux. And not trick their NAND supplier into adding an unused hook… Hi Arvi, Could you help add below patch for mediatek? With below mediatek patch, "Allow RTT negotiation" patch series will more complete and mediatek platform not affect by this patch series. Thanks Peter diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs- mediatek.c index c4f997196c57..f8725f3374f7 100644 --- a/drivers/ufs/host/ufs-mediatek.c +++ b/drivers/ufs/host/ufs-mediatek.c @@ -1777,6 +1777,32 @@ static int ufs_mtk_config_esi(struct ufs_hba *hba) return ufs_mtk_config_mcq(hba, true); } +static void ufs_mtk_set_rtt(struct ufs_hba *hba) +{ + struct ufs_dev_info *dev_info = &hba->dev_info; + u32 rtt = 0; + u32 dev_rtt = 0; + + /* RTT override makes sense only for UFS-4.0 and above */ + if (dev_info->wspecversion < 0x400) + return; + + if (ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR, + QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0, 0, &dev_rtt)) { + dev_err(hba->dev, "failed reading bMaxNumOfRTT\n"); + return; + } + + /* override if not mediatek support */ + if (dev_rtt == MTK_MAX_NUM_RTT) + return; + + rtt = MTK_MAX_NUM_RTT; + if (ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR, + QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0, 0, &rtt)) + dev_err(hba->dev, "failed writing bMaxNumOfRTT\n"); +} + /* * struct ufs_hba_mtk_vops - UFS MTK specific variant operations * @@ -1805,6 +1831,7 @@ static const struct ufs_hba_variant_ops ufs_hba_mtk_vops = { .op_runtime_config = ufs_mtk_op_runtime_config, .mcq_config_resource = ufs_mtk_mcq_config_resource, .config_esi = ufs_mtk_config_esi, + .set_rtt = ufs_mtk_set_rtt, }; /** 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 */
On 5/23/24 23:06, Peter Wang (王信友) wrote: > diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs- > mediatek.c > index c4f997196c57..f8725f3374f7 100644 > --- a/drivers/ufs/host/ufs-mediatek.c > +++ b/drivers/ufs/host/ufs-mediatek.c > @@ -1777,6 +1777,32 @@ static int ufs_mtk_config_esi(struct ufs_hba > *hba) > return ufs_mtk_config_mcq(hba, true); > } > > +static void ufs_mtk_set_rtt(struct ufs_hba *hba) > +{ > + struct ufs_dev_info *dev_info = &hba->dev_info; > + u32 rtt = 0; > + u32 dev_rtt = 0; > + > + /* RTT override makes sense only for UFS-4.0 and above */ > + if (dev_info->wspecversion < 0x400) > + return; > + > + if (ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR, > + QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0, > 0, &dev_rtt)) { > + dev_err(hba->dev, "failed reading bMaxNumOfRTT\n"); > + return; > + } > + > + /* override if not mediatek support */ > + if (dev_rtt == MTK_MAX_NUM_RTT) > + return; > + > + rtt = MTK_MAX_NUM_RTT; > + if (ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR, > + QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0, > 0, &rtt)) > + dev_err(hba->dev, "failed writing bMaxNumOfRTT\n"); > +} > + > /* > * struct ufs_hba_mtk_vops - UFS MTK specific variant operations > * > @@ -1805,6 +1831,7 @@ static const struct ufs_hba_variant_ops > ufs_hba_mtk_vops = { > .op_runtime_config = ufs_mtk_op_runtime_config, > .mcq_config_resource = ufs_mtk_mcq_config_resource, > .config_esi = ufs_mtk_config_esi, > + .set_rtt = ufs_mtk_set_rtt, > }; > > /** > 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 */ The above patch would result in duplication of code. We should avoid code duplication if that's reasonably possible. Instead of applying the above patch, I propose to add a callback function pointer in struct ufs_hba_variant_ops that returns the maximum RTT value supported by the host driver. Thanks, Bart.
> > On 5/23/24 23:06, Peter Wang (王信友) wrote: > > diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs- > > mediatek.c > > index c4f997196c57..f8725f3374f7 100644 > > --- a/drivers/ufs/host/ufs-mediatek.c > > +++ b/drivers/ufs/host/ufs-mediatek.c > > @@ -1777,6 +1777,32 @@ static int ufs_mtk_config_esi(struct ufs_hba > > *hba) > > return ufs_mtk_config_mcq(hba, true); > > } > > > > +static void ufs_mtk_set_rtt(struct ufs_hba *hba) > > +{ > > + struct ufs_dev_info *dev_info = &hba->dev_info; > > + u32 rtt = 0; > > + u32 dev_rtt = 0; > > + > > + /* RTT override makes sense only for UFS-4.0 and above */ > > + if (dev_info->wspecversion < 0x400) > > + return; > > + > > + if (ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR, > > + QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0, > > 0, &dev_rtt)) { > > + dev_err(hba->dev, "failed reading bMaxNumOfRTT\n"); > > + return; > > + } > > + > > + /* override if not mediatek support */ > > + if (dev_rtt == MTK_MAX_NUM_RTT) > > + return; > > + > > + rtt = MTK_MAX_NUM_RTT; > > + if (ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR, > > + QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0, > > 0, &rtt)) > > + dev_err(hba->dev, "failed writing bMaxNumOfRTT\n"); > > +} > > + > > /* > > * struct ufs_hba_mtk_vops - UFS MTK specific variant operations > > * > > @@ -1805,6 +1831,7 @@ static const struct ufs_hba_variant_ops > > ufs_hba_mtk_vops = { > > .op_runtime_config = ufs_mtk_op_runtime_config, > > .mcq_config_resource = ufs_mtk_mcq_config_resource, > > .config_esi = ufs_mtk_config_esi, > > + .set_rtt = ufs_mtk_set_rtt, > > }; > > > > /** > > 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 */ > > The above patch would result in duplication of code. We should avoid > code duplication if that's reasonably possible. Instead of applying the > above patch, I propose to add a callback function pointer in struct > ufs_hba_variant_ops that returns the maximum RTT value supported by the > host driver. Yes. That would simplify things. Would replace that with the nortt capability. Thanks, Avri > > Thanks, > > Bart.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 7df8bcacbe7e..d8e0e80d66a5 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -8542,7 +8542,10 @@ static int ufshcd_device_params_init(struct ufs_hba *hba) goto out; } - ufshcd_set_rtt(hba); + if (hba->vops && hba->vops->set_rtt) + hba->vops->set_rtt(hba); + else + ufshcd_set_rtt(hba); ufshcd_get_ref_clk_gating_wait(hba); diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index d74bd2d67b06..495b50a72f9f 100644 --- a/include/ufs/ufshcd.h +++ b/include/ufs/ufshcd.h @@ -329,6 +329,7 @@ struct ufs_pwr_mode_info { * @get_outstanding_cqs: called to get outstanding completion queues * @config_esi: called to config Event Specific Interrupt * @config_scsi_dev: called to configure SCSI device parameters + * @set_rtt: negotiate rtt */ struct ufs_hba_variant_ops { const char *name; @@ -374,6 +375,7 @@ struct ufs_hba_variant_ops { int (*get_outstanding_cqs)(struct ufs_hba *hba, unsigned long *ocqs); int (*config_esi)(struct ufs_hba *hba); + void (*set_rtt)(struct ufs_hba *hba); }; /* clock gating state */
Allow platform vendors to take precedence having their own rtt negotiation mechanism. This makes sense because the host controller's nortt characteristic may vary among vendors. Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/ufs/core/ufshcd.c | 5 ++++- include/ufs/ufshcd.h | 2 ++ 2 files changed, 6 insertions(+), 1 deletion(-)