* Re: [PATCH v5] bus: mhi: core: Add support for processing priority of event ring
[not found] ` <9300cf49a498521f471d6106131bd675@codeaurora.org>
@ 2021-07-29 0:06 ` Bhaumik Bhatt
2021-09-16 5:28 ` Kalle Valo
0 siblings, 1 reply; 2+ messages in thread
From: Bhaumik Bhatt @ 2021-07-29 0:06 UTC (permalink / raw)
To: kvalo, ath11k
Cc: linux-arm-msm, hemantk, linux-kernel, Manivannan Sadhasivam,
linux-wireless
Hi Kalle,
On 2021-07-16 11:22 AM, Bhaumik Bhatt wrote:
> Hi Mani,
>
> On 2021-07-16 04:49 AM, Manivannan Sadhasivam wrote:
>> On Fri, Jun 25, 2021 at 10:22:08AM -0700, Bhaumik Bhatt wrote:
>>> From: Hemant Kumar <hemantk@codeaurora.org>
>>>
>>> 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@linaro.org>
>>
>> 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
>>
> Idea for this patch came from 914b72a6948b ("bus: mhi: Wait for M2
> state during system resume").
>
> If WLAN ath11k controller driver wants to avoid the scenario mentioned
> in
> the above patch, it will help them to have a high priority for their
> dedicated
> control events ring.
>
> I would defer to Kalle and others in ath11k, whether or not they are
> OK to take that route.
> as an update to priority will just help return from resume faster.
> It will also depend on system load/reproducibility rate of the
> scenario.
>
> I can provide a patch for them to review/test since I do not have the
> setup for it.
>
Would you like to try this patch out? It basically increases the
priority
at which the control events for M0/M1/M3 state changes are handled.
Let me know if you have any questions.
>>> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
>>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>>> ---
>>> 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
>>>
>
> Thanks,
> Bhaumik
> ---
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
> a Linux Foundation Collaborative Project
Thanks,
Bhaumik
---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 2+ messages in thread