Re: [PATCH v1 1/4] bus: mhi: core: Add support for processing priority of event ring

[Bhaumik Bhatt]

Hi Mani,

On 2021-06-18 12:03 AM, Manivannan Sadhasivam wrote:
> On Thu, Jun 17, 2021 at 02:30:32PM -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
> > with 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>
> > ---
> >  drivers/bus/mhi/core/internal.h |  2 +-
> >  drivers/bus/mhi/core/main.c     | 19 ++++++++++++++++---
> >  include/linux/mhi.h             | 14 ++++++++++++--
> >  3 files changed, 29 insertions(+), 6 deletions(-)
> >
> > 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;
>
> Instead of using enum for priorities, can we just make use of the
> existing "priority" field? Since the existing controllers set it to 
> "1",
> can we use "0" as the high priority?
>
> This way we don't need to change the controller drivers.
I agree but the reasons to do the enum approach was to allow for future
expansion of the handling if it becomes necessary and provide clarity 
for
the field.

I can always do it this way for now and we can have the enum for another
time but would prefer updating what we have now.
> >  	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:
>
> This could be,
>
> 	/* Use high priority tasklet for high priority event ring */
> 	if (!mhi_event->priority)
> 		tasklet_hi_schedule(&mhi_event->task);
> 	else
> 		tasklet_schedule(&mhi_event->task);
>
> Thanks,
> Mani
Yes, this works too if we keep the current non-enum approach.
> > +		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..25ee312 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_HI: processed by hi-priority tasklet
> > + * @MHI_ER_PRIORITY_DEFAULT: processed by regular tasklet
> > + */
> > +enum mhi_er_priority {
> > +	MHI_ER_PRIORITY_HI,
> > +	MHI_ER_PRIORITY_DEFAULT,
> > +};
> > +
> > +/**
> >   * 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
Existing controllers would be fine.

Do you think we have a problem if a new controller blindly inputs a "0" 
in
the priority not knowing the impact of it?

If you give me a go ahead, I can make these changes in v2 and leave the 
enum
stuff out.

Thanks,
Bhaumik
---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project