Re: [PATCH v5] bus: mhi: core: Add support for processing priority of event ring

From: Manivannan Sadhasivam
Date: Fri Jul 16 2021 - 07:49:40 EST

On Fri, Jun 25, 2021 at 10:22:08AM -0700, Bhaumik Bhatt wrote:
> From: Hemant Kumar <hemantk@xxxxxxxxxxxxxx>
>
> Event ring priorities are currently set to 1 and are unused.
> Default processing priority for event rings is set to regular
> tasklet. Controllers can choose to use high priority tasklet
> scheduling for certain event rings critical for processing such
> as ones transporting control information if they wish to avoid
> system scheduling delays for those packets. In order to support
> these use cases, allow controllers to set event ring priority to
> high.
>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>

Just curious, what are the event rings you are going to make as high
priority? If you are going to do that for existing controllers, please
submit a patch now itself.

Thanks,
Mani

> Signed-off-by: Hemant Kumar <hemantk@xxxxxxxxxxxxxx>
> Signed-off-by: Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx>
> ---
> v5:
> -Add controller changes and enable usage of event ring configuration priorities
> -Fix nitpick, use high instead of hi in kdoc
>
> v4:
> -Update fixed priority for all events to default to fix bug in v3
> -Supply changelog
>
> v3:
> -Revert to enum approach
> -Use 0 as default and 1 as high in enum
> -Do not use config values for event rings
>
> v2:
> -Use boolean approach for easy maintenance as controllers do not need updates
>
>
>
> drivers/bus/mhi/core/init.c | 3 +-
> drivers/bus/mhi/core/internal.h | 2 +-
> drivers/bus/mhi/core/main.c | 19 ++++++++--
> drivers/bus/mhi/pci_generic.c | 66 +++++++++++++++++------------------
> drivers/net/wireless/ath/ath11k/mhi.c | 4 +--
> include/linux/mhi.h | 14 ++++++--
> 6 files changed, 65 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index c81b377..4446760 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -673,8 +673,7 @@ static int parse_ev_cfg(struct mhi_controller *mhi_cntrl,
> &mhi_cntrl->mhi_chan[mhi_event->chan];
> }
>
> - /* Priority is fixed to 1 for now */
> - mhi_event->priority = 1;
> + mhi_event->priority = event_cfg->priority;
>
> mhi_event->db_cfg.brstmode = event_cfg->mode;
> if (MHI_INVALID_BRSTMODE(mhi_event->db_cfg.brstmode))
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index 672052f..666e102 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -535,7 +535,7 @@ struct mhi_event {
> u32 intmod;
> u32 irq;
> int chan; /* this event ring is dedicated to a channel (optional) */
> - u32 priority;
> + enum mhi_er_priority priority;
> enum mhi_er_data_type data_type;
> struct mhi_ring ring;
> struct db_cfg db_cfg;
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 8ac73f9..bfc9776 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -425,10 +425,11 @@ void mhi_create_devices(struct mhi_controller *mhi_cntrl)
> }
> }
>
> -irqreturn_t mhi_irq_handler(int irq_number, void *dev)
> +irqreturn_t mhi_irq_handler(int irq_number, void *priv)
> {
> - struct mhi_event *mhi_event = dev;
> + struct mhi_event *mhi_event = priv;
> struct mhi_controller *mhi_cntrl = mhi_event->mhi_cntrl;
> + struct device *dev = &mhi_cntrl->mhi_dev->dev;
> struct mhi_event_ctxt *er_ctxt =
> &mhi_cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index];
> struct mhi_ring *ev_ring = &mhi_event->ring;
> @@ -454,8 +455,20 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
>
> if (mhi_dev)
> mhi_notify(mhi_dev, MHI_CB_PENDING_DATA);
> - } else {
> +
> + return IRQ_HANDLED;
> + }
> +
> + switch (mhi_event->priority) {
> + case MHI_ER_PRIORITY_HI:
> + tasklet_hi_schedule(&mhi_event->task);
> + break;
> + case MHI_ER_PRIORITY_DEFAULT:
> tasklet_schedule(&mhi_event->task);
> + break;
> + default:
> + dev_err(dev, "Skip event of unknown priority\n");
> + break;
> }
>
> return IRQ_HANDLED;
> diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
> index 31360a2..5886547 100644
> --- a/drivers/bus/mhi/pci_generic.c
> +++ b/drivers/bus/mhi/pci_generic.c
> @@ -74,17 +74,17 @@ struct mhi_pci_dev_info {
> .doorbell_mode_switch = false, \
> }
>
> -#define MHI_EVENT_CONFIG_CTRL(ev_ring, el_count) \
> - { \
> - .num_elements = el_count, \
> - .irq_moderation_ms = 0, \
> - .irq = (ev_ring) + 1, \
> - .priority = 1, \
> - .mode = MHI_DB_BRST_DISABLE, \
> - .data_type = MHI_ER_CTRL, \
> - .hardware_event = false, \
> - .client_managed = false, \
> - .offload_channel = false, \
> +#define MHI_EVENT_CONFIG_CTRL(ev_ring, el_count) \
> + { \
> + .num_elements = el_count, \
> + .irq_moderation_ms = 0, \
> + .irq = (ev_ring) + 1, \
> + .priority = MHI_ER_PRIORITY_DEFAULT, \
> + .mode = MHI_DB_BRST_DISABLE, \
> + .data_type = MHI_ER_CTRL, \
> + .hardware_event = false, \
> + .client_managed = false, \
> + .offload_channel = false, \
> }
>
> #define MHI_CHANNEL_CONFIG_HW_UL(ch_num, ch_name, el_count, ev_ring) \
> @@ -177,31 +177,31 @@ struct mhi_pci_dev_info {
> .doorbell_mode_switch = false, \
> }
>
> -#define MHI_EVENT_CONFIG_DATA(ev_ring, el_count) \
> - { \
> - .num_elements = el_count, \
> - .irq_moderation_ms = 5, \
> - .irq = (ev_ring) + 1, \
> - .priority = 1, \
> - .mode = MHI_DB_BRST_DISABLE, \
> - .data_type = MHI_ER_DATA, \
> - .hardware_event = false, \
> - .client_managed = false, \
> - .offload_channel = false, \
> +#define MHI_EVENT_CONFIG_DATA(ev_ring, el_count) \
> + { \
> + .num_elements = el_count, \
> + .irq_moderation_ms = 5, \
> + .irq = (ev_ring) + 1, \
> + .priority = MHI_ER_PRIORITY_DEFAULT, \
> + .mode = MHI_DB_BRST_DISABLE, \
> + .data_type = MHI_ER_DATA, \
> + .hardware_event = false, \
> + .client_managed = false, \
> + .offload_channel = false, \
> }
>
> #define MHI_EVENT_CONFIG_HW_DATA(ev_ring, el_count, ch_num) \
> - { \
> - .num_elements = el_count, \
> - .irq_moderation_ms = 1, \
> - .irq = (ev_ring) + 1, \
> - .priority = 1, \
> - .mode = MHI_DB_BRST_DISABLE, \
> - .data_type = MHI_ER_DATA, \
> - .hardware_event = true, \
> - .client_managed = false, \
> - .offload_channel = false, \
> - .channel = ch_num, \
> + { \
> + .num_elements = el_count, \
> + .irq_moderation_ms = 1, \
> + .irq = (ev_ring) + 1, \
> + .priority = MHI_ER_PRIORITY_DEFAULT, \
> + .mode = MHI_DB_BRST_DISABLE, \
> + .data_type = MHI_ER_DATA, \
> + .hardware_event = true, \
> + .client_managed = false, \
> + .offload_channel = false, \
> + .channel = ch_num, \
> }
>
> static const struct mhi_channel_config modem_qcom_v1_mhi_channels[] = {
> diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
> index 27b394d..b7864fc 100644
> --- a/drivers/net/wireless/ath/ath11k/mhi.c
> +++ b/drivers/net/wireless/ath/ath11k/mhi.c
> @@ -86,7 +86,7 @@ static struct mhi_event_config ath11k_mhi_events_qca6390[] = {
> .irq_moderation_ms = 1,
> .irq = 2,
> .mode = MHI_DB_BRST_DISABLE,
> - .priority = 1,
> + .priority = MHI_ER_PRIORITY_DEFAULT,
> .hardware_event = false,
> .client_managed = false,
> .offload_channel = false,
> @@ -179,7 +179,7 @@ static struct mhi_event_config ath11k_mhi_events_qcn9074[] = {
> .irq_moderation_ms = 1,
> .irq = 2,
> .mode = MHI_DB_BRST_DISABLE,
> - .priority = 1,
> + .priority = MHI_ER_PRIORITY_DEFAULT,
> .hardware_event = false,
> .client_managed = false,
> .offload_channel = false,
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 86cea52..3e92e85 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -198,6 +198,16 @@ enum mhi_er_data_type {
> };
>
> /**
> + * enum mhi_er_priority - Event ring processing priority
> + * @MHI_ER_PRIORITY_DEFAULT: processed by regular tasklet
> + * @MHI_ER_PRIORITY_HI: processed by high priority tasklet
> + */
> +enum mhi_er_priority {
> + MHI_ER_PRIORITY_DEFAULT,
> + MHI_ER_PRIORITY_HI,
> +};
> +
> +/**
> * enum mhi_db_brst_mode - Doorbell mode
> * @MHI_DB_BRST_DISABLE: Burst mode disable
> * @MHI_DB_BRST_ENABLE: Burst mode enable
> @@ -250,7 +260,7 @@ struct mhi_channel_config {
> * @irq_moderation_ms: Delay irq for additional events to be aggregated
> * @irq: IRQ associated with this ring
> * @channel: Dedicated channel number. U32_MAX indicates a non-dedicated ring
> - * @priority: Priority of this ring. Use 1 for now
> + * @priority: Processing priority of this ring.
> * @mode: Doorbell mode
> * @data_type: Type of data this ring will process
> * @hardware_event: This ring is associated with hardware channels
> @@ -262,7 +272,7 @@ struct mhi_event_config {
> u32 irq_moderation_ms;
> u32 irq;
> u32 channel;
> - u32 priority;
> + enum mhi_er_priority priority;
> enum mhi_db_brst_mode mode;
> enum mhi_er_data_type data_type;
> bool hardware_event;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>