| Message ID | 20240412174158.3050361-1-bvanassche@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: ufs: Remove .get_hba_mac() | expand |
On 4/13/24 04:48, Avri Altman wrote: >> + nutrs = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1; > Isn't this already hba->nutrs? Enabling MCQ causes the value of NUTRS to change. >> + WARN_ONCE(nutrs < 32, "nutrs: %d < 32\n", nutrs); > redundant Why is this considered redundant? Thanks, Bart.
> On 4/13/24 04:48, Avri Altman wrote: > >> + nutrs = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1; > > Isn't this already hba->nutrs? > > Enabling MCQ causes the value of NUTRS to change. But not by the time ufshcd_mcq_decide_queue_depth is called. ufshcd_mcq_decide_queue_depth is where hba->nutrs may change. > > >> + WARN_ONCE(nutrs < 32, "nutrs: %d < 32\n", nutrs); > > redundant > > Why is this considered redundant? I see no point checking the hw. Both In legacy & MCQ it is allowed to be < 32 - why the WARN? Thanks, Avri > > Thanks, > > Bart.
On Fri, Apr 12, 2024 at 10:41:29AM -0700, Bart Van Assche wrote: > Simplify the UFSHCI core and also the UFSHCI host drivers by removing > the .get_hba_mac() callback and by reading the NUTRS register field > instead. > This quite clearly states that the change is merely a cleanup. Can you confirm that (capabilities & 0xff) + 1 read back as 64 on both Mediatek and Qualcomm platforms? Such that this indeed is merely a cleanup... If not, please split the change in two; one that drops the callback in favor of always using MAX_SUPP_MAC, and a separate change that instead reads the value back. Please provide a clear problem description in your commit messages. Regards, Bjorn
On Fri, 2024-04-12 at 10:41 -0700, Bart Van Assche wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Simplify the UFSHCI core and also the UFSHCI host drivers by > removing > the .get_hba_mac() callback and by reading the NUTRS register field > instead. > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/ufs/core/ufs-mcq.c | 14 +++++--------- > drivers/ufs/core/ufshcd-priv.h | 8 -------- > drivers/ufs/host/ufs-mediatek.c | 13 ------------- > drivers/ufs/host/ufs-qcom.c | 7 ------- > drivers/ufs/host/ufs-qcom.h | 1 - > include/ufs/ufshcd.h | 2 -- > include/ufs/ufshci.h | 2 +- > 7 files changed, 6 insertions(+), 41 deletions(-) > > diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c > index 768bf87cd80d..228975caf68e 100644 > --- a/drivers/ufs/core/ufs-mcq.c > +++ b/drivers/ufs/core/ufs-mcq.c > @@ -125,20 +125,16 @@ struct ufs_hw_queue > *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba, > * > * MAC - Max. Active Command of the Host Controller (HC) > * HC wouldn't send more than this commands to the device. > - * It is mandatory to implement get_hba_mac() to enable MCQ mode. > * Calculates and adjusts the queue depth based on the depth > * supported by the HC and ufs device. > */ > int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba) > { > - int mac; > + int nutrs; > > - /* Mandatory to implement get_hba_mac() */ > - mac = ufshcd_mcq_vops_get_hba_mac(hba); > - if (mac < 0) { > - dev_err(hba->dev, "Failed to get mac, err=%d\n", mac); > - return mac; > - } > + WARN_ON_ONCE(!hba->mcq_enabled); > + nutrs = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1; > Hi Bart, Mediatek some platform read nutrs in MCQ mode is 0x1F, not 0x3F. Which means we still need this ops to get correct MAC. > + WARN_ONCE(nutrs < 32, "nutrs: %d < 32\n", nutrs); > > WARN_ON_ONCE(!hba->dev_info.bqueuedepth); > /* > @@ -146,7 +142,7 @@ int ufshcd_mcq_decide_queue_depth(struct ufs_hba > *hba) > * It is mandatory for UFS device to define bQueueDepth if > * shared queuing architecture is enabled. > */ > - return min_t(int, mac, hba->dev_info.bqueuedepth); > + return min_t(int, nutrs, hba->dev_info.bqueuedepth); > } > > static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba) > diff --git a/drivers/ufs/core/ufshcd-priv.h > b/drivers/ufs/core/ufshcd-priv.h > index f42d99ce5bf1..a1add22205db 100644 > --- a/drivers/ufs/core/ufshcd-priv.h > +++ b/drivers/ufs/core/ufshcd-priv.h > @@ -255,14 +255,6 @@ static inline int > ufshcd_vops_mcq_config_resource(struct ufs_hba *hba) > return -EOPNOTSUPP; > } > > -static inline int ufshcd_mcq_vops_get_hba_mac(struct ufs_hba *hba) > -{ > - if (hba->vops && hba->vops->get_hba_mac) > - return hba->vops->get_hba_mac(hba); > - > - return -EOPNOTSUPP; > -} > - > static inline int ufshcd_mcq_vops_op_runtime_config(struct ufs_hba > *hba) > { > if (hba->vops && hba->vops->op_runtime_config) > diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs- > mediatek.c > index c4f997196c57..0a52917e7fe6 100644 > --- a/drivers/ufs/host/ufs-mediatek.c > +++ b/drivers/ufs/host/ufs-mediatek.c > @@ -34,7 +34,6 @@ static int ufs_mtk_config_mcq(struct ufs_hba *hba, > bool irq); > #include "ufs-mediatek-trace.h" > #undef CREATE_TRACE_POINTS > > -#define MAX_SUPP_MAC 64 > #define MCQ_QUEUE_OFFSET(c) ((((c) >> 16) & 0xFF) * 0x200) > > static const struct ufs_dev_quirk ufs_mtk_dev_fixups[] = { > @@ -1656,17 +1655,6 @@ static int ufs_mtk_clk_scale_notify(struct > ufs_hba *hba, bool scale_up, > return 0; > } > > -static int ufs_mtk_get_hba_mac(struct ufs_hba *hba) > -{ > - struct ufs_mtk_host *host = ufshcd_get_variant(hba); > - > - /* MCQ operation not permitted */ > - if (host->caps & UFS_MTK_CAP_DISABLE_MCQ) > - return -EPERM; > - > Beside, mediatek also have a host caps to disable MCQ even hw support. Thanks. Peter > - return MAX_SUPP_MAC; > -} > - > static int ufs_mtk_op_runtime_config(struct ufs_hba *hba) > { > struct ufshcd_mcq_opr_info_t *opr; > @@ -1801,7 +1789,6 @@ static const struct ufs_hba_variant_ops > ufs_hba_mtk_vops = { > .config_scaling_param = ufs_mtk_config_scaling_param, > .clk_scale_notify = ufs_mtk_clk_scale_notify, > /* mcq vops */ > - .get_hba_mac = ufs_mtk_get_hba_mac, > .op_runtime_config = ufs_mtk_op_runtime_config, > .mcq_config_resource = ufs_mtk_mcq_config_resource, > .config_esi = ufs_mtk_config_esi, > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs- > qcom.c > index 0b02e697ea5b..100f5f0b9da6 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -1673,12 +1673,6 @@ static int ufs_qcom_op_runtime_config(struct > ufs_hba *hba) > return 0; > } > > -static int ufs_qcom_get_hba_mac(struct ufs_hba *hba) > -{ > - /* Qualcomm HC supports up to 64 */ > - return MAX_SUPP_MAC; > -} > - > static int ufs_qcom_get_outstanding_cqs(struct ufs_hba *hba, > unsigned long *ocqs) > { > @@ -1798,7 +1792,6 @@ static const struct ufs_hba_variant_ops > ufs_hba_qcom_vops = { > .program_key = ufs_qcom_ice_program_key, > .reinit_notify = ufs_qcom_reinit_notify, > .mcq_config_resource = ufs_qcom_mcq_config_resource, > - .get_hba_mac = ufs_qcom_get_hba_mac, > .op_runtime_config = ufs_qcom_op_runtime_config, > .get_outstanding_cqs = ufs_qcom_get_outstanding_cqs, > .config_esi = ufs_qcom_config_esi, > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs- > qcom.h > index b9de170983c9..7951421b9921 100644 > --- a/drivers/ufs/host/ufs-qcom.h > +++ b/drivers/ufs/host/ufs-qcom.h > @@ -14,7 +14,6 @@ > #define TX_FSM_HIBERN8 0x1 > #define HBRN8_POLL_TOUT_MS 100 > #define DEFAULT_CLK_RATE_HZ 1000000 > -#define MAX_SUPP_MAC 64 > #define MAX_ESI_VEC 32 > > #define UFS_HW_VER_MAJOR_MASK GENMASK(31, 28) > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h > index bad88bd91995..3f50621b8564 100644 > --- a/include/ufs/ufshcd.h > +++ b/include/ufs/ufshcd.h > @@ -324,7 +324,6 @@ struct ufs_pwr_mode_info { > * @event_notify: called to notify important events > * @reinit_notify: called to notify reinit of UFSHCD during max gear > switch > * @mcq_config_resource: called to configure MCQ platform resources > - * @get_hba_mac: called to get vendor specific mac value, mandatory > for mcq mode > * @op_runtime_config: called to config Operation and runtime regs > Pointers > * @get_outstanding_cqs: called to get outstanding completion queues > * @config_esi: called to config Event Specific Interrupt > @@ -369,7 +368,6 @@ struct ufs_hba_variant_ops { > enum ufs_event_type evt, void *data); > void (*reinit_notify)(struct ufs_hba *); > int (*mcq_config_resource)(struct ufs_hba *hba); > - int (*get_hba_mac)(struct ufs_hba *hba); > int (*op_runtime_config)(struct ufs_hba *hba); > int (*get_outstanding_cqs)(struct ufs_hba *hba, > unsigned long *ocqs); > diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h > index 385e1c6b8d60..6c28177113e1 100644 > --- a/include/ufs/ufshci.h > +++ b/include/ufs/ufshci.h > @@ -67,7 +67,7 @@ enum { > > /* Controller capability masks */ > enum { > - MASK_TRANSFER_REQUESTS_SLOTS = 0x0000001F, > + MASK_TRANSFER_REQUESTS_SLOTS = 0x000000FF, > MASK_TASK_MANAGEMENT_REQUEST_SLOTS = 0x00070000, > MASK_EHSLUTRD_SUPPORTED = 0x00400000, > MASK_AUTO_HIBERN8_SUPPORT = 0x00800000,
On 4/14/24 18:33, Bjorn Andersson wrote: > On Fri, Apr 12, 2024 at 10:41:29AM -0700, Bart Van Assche wrote: >> Simplify the UFSHCI core and also the UFSHCI host drivers by removing >> the .get_hba_mac() callback and by reading the NUTRS register field >> instead. > > This quite clearly states that the change is merely a cleanup. Can you > confirm that (capabilities & 0xff) + 1 read back as 64 on both Mediatek > and Qualcomm platforms? Such that this indeed is merely a cleanup... I'm waiting for feedback from MediaTek and Qualcomm to learn whether or not their controllers comply with this aspect of the UFSHCI 4.0 standard. Thanks, Bart.
On 4/15/24 00:22, Peter Wang (王信友) wrote: > Mediatek some platform read nutrs in MCQ mode is 0x1F, not 0x3F. > Which means we still need this ops to get correct MAC. Hi Peter, Thanks for the feedback. I will prepare a new version of this patch that preserves the .get_hba_mac() callback. Thanks, Bart.
> > On 4/13/24 04:48, Avri Altman wrote: > > >> + nutrs = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1; > > > Isn't this already hba->nutrs? > > > > Enabling MCQ causes the value of NUTRS to change. > But not by the time ufshcd_mcq_decide_queue_depth is called. > ufshcd_mcq_decide_queue_depth is where hba->nutrs may change. > > > > > >> + WARN_ONCE(nutrs < 32, "nutrs: %d < 32\n", nutrs); > > > redundant > > > > Why is this considered redundant? > I see no point checking the hw. > Both In legacy & MCQ it is allowed to be < 32 - why the WARN? If any, the check proposed here - https://lore.kernel.org/lkml/DM6PR04MB6575720218D06C3EF9721BF2FC639@DM6PR04MB6575.namprd04.prod.outlook.com/ Makes more sense to me. Thanks, Avri > > Thanks, > Avri > > > > Thanks, > > > > Bart.
diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c index 768bf87cd80d..228975caf68e 100644 --- a/drivers/ufs/core/ufs-mcq.c +++ b/drivers/ufs/core/ufs-mcq.c @@ -125,20 +125,16 @@ struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba, * * MAC - Max. Active Command of the Host Controller (HC) * HC wouldn't send more than this commands to the device. - * It is mandatory to implement get_hba_mac() to enable MCQ mode. * Calculates and adjusts the queue depth based on the depth * supported by the HC and ufs device. */ int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba) { - int mac; + int nutrs; - /* Mandatory to implement get_hba_mac() */ - mac = ufshcd_mcq_vops_get_hba_mac(hba); - if (mac < 0) { - dev_err(hba->dev, "Failed to get mac, err=%d\n", mac); - return mac; - } + WARN_ON_ONCE(!hba->mcq_enabled); + nutrs = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1; + WARN_ONCE(nutrs < 32, "nutrs: %d < 32\n", nutrs); WARN_ON_ONCE(!hba->dev_info.bqueuedepth); /* @@ -146,7 +142,7 @@ int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba) * It is mandatory for UFS device to define bQueueDepth if * shared queuing architecture is enabled. */ - return min_t(int, mac, hba->dev_info.bqueuedepth); + return min_t(int, nutrs, hba->dev_info.bqueuedepth); } static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba) diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h index f42d99ce5bf1..a1add22205db 100644 --- a/drivers/ufs/core/ufshcd-priv.h +++ b/drivers/ufs/core/ufshcd-priv.h @@ -255,14 +255,6 @@ static inline int ufshcd_vops_mcq_config_resource(struct ufs_hba *hba) return -EOPNOTSUPP; } -static inline int ufshcd_mcq_vops_get_hba_mac(struct ufs_hba *hba) -{ - if (hba->vops && hba->vops->get_hba_mac) - return hba->vops->get_hba_mac(hba); - - return -EOPNOTSUPP; -} - static inline int ufshcd_mcq_vops_op_runtime_config(struct ufs_hba *hba) { if (hba->vops && hba->vops->op_runtime_config) diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c index c4f997196c57..0a52917e7fe6 100644 --- a/drivers/ufs/host/ufs-mediatek.c +++ b/drivers/ufs/host/ufs-mediatek.c @@ -34,7 +34,6 @@ static int ufs_mtk_config_mcq(struct ufs_hba *hba, bool irq); #include "ufs-mediatek-trace.h" #undef CREATE_TRACE_POINTS -#define MAX_SUPP_MAC 64 #define MCQ_QUEUE_OFFSET(c) ((((c) >> 16) & 0xFF) * 0x200) static const struct ufs_dev_quirk ufs_mtk_dev_fixups[] = { @@ -1656,17 +1655,6 @@ static int ufs_mtk_clk_scale_notify(struct ufs_hba *hba, bool scale_up, return 0; } -static int ufs_mtk_get_hba_mac(struct ufs_hba *hba) -{ - struct ufs_mtk_host *host = ufshcd_get_variant(hba); - - /* MCQ operation not permitted */ - if (host->caps & UFS_MTK_CAP_DISABLE_MCQ) - return -EPERM; - - return MAX_SUPP_MAC; -} - static int ufs_mtk_op_runtime_config(struct ufs_hba *hba) { struct ufshcd_mcq_opr_info_t *opr; @@ -1801,7 +1789,6 @@ static const struct ufs_hba_variant_ops ufs_hba_mtk_vops = { .config_scaling_param = ufs_mtk_config_scaling_param, .clk_scale_notify = ufs_mtk_clk_scale_notify, /* mcq vops */ - .get_hba_mac = ufs_mtk_get_hba_mac, .op_runtime_config = ufs_mtk_op_runtime_config, .mcq_config_resource = ufs_mtk_mcq_config_resource, .config_esi = ufs_mtk_config_esi, diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index 0b02e697ea5b..100f5f0b9da6 100644 --- a/drivers/ufs/host/ufs-qcom.c +++ b/drivers/ufs/host/ufs-qcom.c @@ -1673,12 +1673,6 @@ static int ufs_qcom_op_runtime_config(struct ufs_hba *hba) return 0; } -static int ufs_qcom_get_hba_mac(struct ufs_hba *hba) -{ - /* Qualcomm HC supports up to 64 */ - return MAX_SUPP_MAC; -} - static int ufs_qcom_get_outstanding_cqs(struct ufs_hba *hba, unsigned long *ocqs) { @@ -1798,7 +1792,6 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = { .program_key = ufs_qcom_ice_program_key, .reinit_notify = ufs_qcom_reinit_notify, .mcq_config_resource = ufs_qcom_mcq_config_resource, - .get_hba_mac = ufs_qcom_get_hba_mac, .op_runtime_config = ufs_qcom_op_runtime_config, .get_outstanding_cqs = ufs_qcom_get_outstanding_cqs, .config_esi = ufs_qcom_config_esi, diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h index b9de170983c9..7951421b9921 100644 --- a/drivers/ufs/host/ufs-qcom.h +++ b/drivers/ufs/host/ufs-qcom.h @@ -14,7 +14,6 @@ #define TX_FSM_HIBERN8 0x1 #define HBRN8_POLL_TOUT_MS 100 #define DEFAULT_CLK_RATE_HZ 1000000 -#define MAX_SUPP_MAC 64 #define MAX_ESI_VEC 32 #define UFS_HW_VER_MAJOR_MASK GENMASK(31, 28) diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index bad88bd91995..3f50621b8564 100644 --- a/include/ufs/ufshcd.h +++ b/include/ufs/ufshcd.h @@ -324,7 +324,6 @@ struct ufs_pwr_mode_info { * @event_notify: called to notify important events * @reinit_notify: called to notify reinit of UFSHCD during max gear switch * @mcq_config_resource: called to configure MCQ platform resources - * @get_hba_mac: called to get vendor specific mac value, mandatory for mcq mode * @op_runtime_config: called to config Operation and runtime regs Pointers * @get_outstanding_cqs: called to get outstanding completion queues * @config_esi: called to config Event Specific Interrupt @@ -369,7 +368,6 @@ struct ufs_hba_variant_ops { enum ufs_event_type evt, void *data); void (*reinit_notify)(struct ufs_hba *); int (*mcq_config_resource)(struct ufs_hba *hba); - int (*get_hba_mac)(struct ufs_hba *hba); int (*op_runtime_config)(struct ufs_hba *hba); int (*get_outstanding_cqs)(struct ufs_hba *hba, unsigned long *ocqs); diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h index 385e1c6b8d60..6c28177113e1 100644 --- a/include/ufs/ufshci.h +++ b/include/ufs/ufshci.h @@ -67,7 +67,7 @@ enum { /* Controller capability masks */ enum { - MASK_TRANSFER_REQUESTS_SLOTS = 0x0000001F, + MASK_TRANSFER_REQUESTS_SLOTS = 0x000000FF, MASK_TASK_MANAGEMENT_REQUEST_SLOTS = 0x00070000, MASK_EHSLUTRD_SUPPORTED = 0x00400000, MASK_AUTO_HIBERN8_SUPPORT = 0x00800000,
Simplify the UFSHCI core and also the UFSHCI host drivers by removing the .get_hba_mac() callback and by reading the NUTRS register field instead. Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/ufs/core/ufs-mcq.c | 14 +++++--------- drivers/ufs/core/ufshcd-priv.h | 8 -------- drivers/ufs/host/ufs-mediatek.c | 13 ------------- drivers/ufs/host/ufs-qcom.c | 7 ------- drivers/ufs/host/ufs-qcom.h | 1 - include/ufs/ufshcd.h | 2 -- include/ufs/ufshci.h | 2 +- 7 files changed, 6 insertions(+), 41 deletions(-)