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

[Manivannan Sadhasivam]

On Thu, Jun 24, 2021 at 08:06:19PM -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. This patch only adds support and does not enable usage of
> these priorities.
> 
> Signed-off-by: Hemant Kumar <hemantk@xxxxxxxxxxxxxx>
> Signed-off-by: Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx>

Please add the controller driver changes in this patch itself.

Couple of nits below...

Thanks,
Mani

> ---
> 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     |  4 ++--
>  drivers/bus/mhi/core/internal.h |  2 +-
>  drivers/bus/mhi/core/main.c     | 19 ++++++++++++++++---
>  include/linux/mhi.h             | 14 ++++++++++++--
>  4 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index c81b377..23386b8 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -673,8 +673,8 @@ 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;
> +		/* Priority is fixed to deafult for now */

default

> +		mhi_event->priority = MHI_ER_PRIORITY_DEFAULT;
>  
>  		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/include/linux/mhi.h b/include/linux/mhi.h
> index 86cea52..62ddead 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 hi-priority tasklet

s/hi/high

> + */
> +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
>