diff mbox series

[v2,3/8] mhi: pci_generic: Enable burst mode for hardware channels

Message ID 1606140666-4986-4-git-send-email-loic.poulain@linaro.org
State Superseded
Headers show
Series mhi: pci_generic: Misc improvements | expand

Commit Message

Loic Poulain Nov. 23, 2020, 2:11 p.m. UTC
Hardware channels have a feature called burst mode that allows to
queue transfer ring element(s) (TRE) to a channel without ringing
the device doorbell. In that mode, the device is polling the channel
context for new elements. This reduces the frequency of host initiated
doorbells and increase throughput.

Create a new dedicated macro for hardware channels with burst enabled.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

---
 drivers/bus/mhi/pci_generic.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

-- 
2.7.4

Comments

Bhaumik Bhatt Nov. 24, 2020, 12:58 a.m. UTC | #1
Hi Loic,
On 2020-11-23 06:11 AM, Loic Poulain wrote:
> Hardware channels have a feature called burst mode that allows to

> queue transfer ring element(s) (TRE) to a channel without ringing

> the device doorbell. In that mode, the device is polling the channel

> context for new elements. This reduces the frequency of host initiated

> doorbells and increase throughput.

> 

> Create a new dedicated macro for hardware channels with burst enabled.

> 

> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

> ---

>  drivers/bus/mhi/pci_generic.c | 34 ++++++++++++++++++++++++++++++++--

>  1 file changed, 32 insertions(+), 2 deletions(-)

> 

> diff --git a/drivers/bus/mhi/pci_generic.c 

> b/drivers/bus/mhi/pci_generic.c

> index 09c6b26..0c07cf5 100644

> --- a/drivers/bus/mhi/pci_generic.c

> +++ b/drivers/bus/mhi/pci_generic.c

> @@ -78,6 +78,36 @@ struct mhi_pci_dev_info {

>  		.offload_channel = false,	\

>  	}

> 

> +#define MHI_CHANNEL_CONFIG_HW_UL(ch_num, ch_name, el_count, ev_ring) \

> +	{						\

> +		.num = ch_num,				\

> +		.name = ch_name,			\

> +		.num_elements = el_count,		\

> +		.event_ring = ev_ring,			\

> +		.dir = DMA_TO_DEVICE,			\

> +		.ee_mask = BIT(MHI_EE_AMSS),		\

> +		.pollcfg = 0,				\

> +		.doorbell = MHI_DB_BRST_ENABLE,	\

> +		.lpm_notify = false,			\

> +		.offload_channel = false,		\

> +		.doorbell_mode_switch = false,		\

> +	}						\

> +

> +#define MHI_CHANNEL_CONFIG_HW_DL(ch_num, ch_name, el_count, ev_ring) \

> +	{						\

> +		.num = ch_num,				\

> +		.name = ch_name,			\

> +		.num_elements = el_count,		\

> +		.event_ring = ev_ring,			\

> +		.dir = DMA_FROM_DEVICE,			\

> +		.ee_mask = BIT(MHI_EE_AMSS),		\

> +		.pollcfg = 0,				\

> +		.doorbell = MHI_DB_BRST_ENABLE,	\

> +		.lpm_notify = false,			\

> +		.offload_channel = false,		\

> +		.doorbell_mode_switch = false,		\

> +	}

> +

>  #define MHI_EVENT_CONFIG_DATA(ev_ring)		\

>  	{					\

>  		.num_elements = 128,		\

> @@ -112,8 +142,8 @@ static const struct mhi_channel_config

> modem_qcom_v1_mhi_channels[] = {

>  	MHI_CHANNEL_CONFIG_DL(15, "QMI", 4, 0),

>  	MHI_CHANNEL_CONFIG_UL(20, "IPCR", 8, 0),

>  	MHI_CHANNEL_CONFIG_DL(21, "IPCR", 8, 0),

> -	MHI_CHANNEL_CONFIG_UL(100, "IP_HW0", 128, 1),

> -	MHI_CHANNEL_CONFIG_DL(101, "IP_HW0", 128, 2),

> +	MHI_CHANNEL_CONFIG_HW_UL(100, "IP_HW0", 128, 1),

> +	MHI_CHANNEL_CONFIG_HW_DL(101, "IP_HW0", 128, 2),

>  };

> 

>  static const struct mhi_event_config modem_qcom_v1_mhi_events[] = {

Did you somehow miss my email with comments on this macro from the 
previous
submission?

If not, then any reason you want to submit this way for now and change 
it
later when more HW channels are added?

I don't think it's a good idea to have doorbell_mode_switch as false for
channel 100 as we need to ring the DB every time we come out of M3.

The current proposal will become inconsistent when more HW channels with
different requirements are added so my suggestion was to accommodate 
these
now. Also, I realized we need to update the regular macros as well but 
it
can be dealt with separately.

If you would like recommendations or would want to discuss this 
configuration
thing further, please let me know.

Thanks,
Bhaumik
---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project
Loic Poulain Nov. 24, 2020, 8:05 a.m. UTC | #2
On Tue, 24 Nov 2020 at 01:58, Bhaumik Bhatt <bbhatt@codeaurora.org> wrote:
>

> Hi Loic,

> On 2020-11-23 06:11 AM, Loic Poulain wrote:

> > Hardware channels have a feature called burst mode that allows to

> > queue transfer ring element(s) (TRE) to a channel without ringing

> > the device doorbell. In that mode, the device is polling the channel

> > context for new elements. This reduces the frequency of host initiated

> > doorbells and increase throughput.

> >

> > Create a new dedicated macro for hardware channels with burst enabled.

> >

> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

> > ---

> >  drivers/bus/mhi/pci_generic.c | 34 ++++++++++++++++++++++++++++++++--

> >  1 file changed, 32 insertions(+), 2 deletions(-)

> >

> > diff --git a/drivers/bus/mhi/pci_generic.c

> > b/drivers/bus/mhi/pci_generic.c

> > index 09c6b26..0c07cf5 100644

> > --- a/drivers/bus/mhi/pci_generic.c

> > +++ b/drivers/bus/mhi/pci_generic.c

> > @@ -78,6 +78,36 @@ struct mhi_pci_dev_info {

> >               .offload_channel = false,       \

> >       }

> >

> > +#define MHI_CHANNEL_CONFIG_HW_UL(ch_num, ch_name, el_count, ev_ring) \

> > +     {                                               \

> > +             .num = ch_num,                          \

> > +             .name = ch_name,                        \

> > +             .num_elements = el_count,               \

> > +             .event_ring = ev_ring,                  \

> > +             .dir = DMA_TO_DEVICE,                   \

> > +             .ee_mask = BIT(MHI_EE_AMSS),            \

> > +             .pollcfg = 0,                           \

> > +             .doorbell = MHI_DB_BRST_ENABLE, \

> > +             .lpm_notify = false,                    \

> > +             .offload_channel = false,               \

> > +             .doorbell_mode_switch = false,          \

> > +     }                                               \

> > +

> > +#define MHI_CHANNEL_CONFIG_HW_DL(ch_num, ch_name, el_count, ev_ring) \

> > +     {                                               \

> > +             .num = ch_num,                          \

> > +             .name = ch_name,                        \

> > +             .num_elements = el_count,               \

> > +             .event_ring = ev_ring,                  \

> > +             .dir = DMA_FROM_DEVICE,                 \

> > +             .ee_mask = BIT(MHI_EE_AMSS),            \

> > +             .pollcfg = 0,                           \

> > +             .doorbell = MHI_DB_BRST_ENABLE, \

> > +             .lpm_notify = false,                    \

> > +             .offload_channel = false,               \

> > +             .doorbell_mode_switch = false,          \

> > +     }

> > +

> >  #define MHI_EVENT_CONFIG_DATA(ev_ring)               \

> >       {                                       \

> >               .num_elements = 128,            \

> > @@ -112,8 +142,8 @@ static const struct mhi_channel_config

> > modem_qcom_v1_mhi_channels[] = {

> >       MHI_CHANNEL_CONFIG_DL(15, "QMI", 4, 0),

> >       MHI_CHANNEL_CONFIG_UL(20, "IPCR", 8, 0),

> >       MHI_CHANNEL_CONFIG_DL(21, "IPCR", 8, 0),

> > -     MHI_CHANNEL_CONFIG_UL(100, "IP_HW0", 128, 1),

> > -     MHI_CHANNEL_CONFIG_DL(101, "IP_HW0", 128, 2),

> > +     MHI_CHANNEL_CONFIG_HW_UL(100, "IP_HW0", 128, 1),

> > +     MHI_CHANNEL_CONFIG_HW_DL(101, "IP_HW0", 128, 2),

> >  };

> >

> >  static const struct mhi_event_config modem_qcom_v1_mhi_events[] = {

> Did you somehow miss my email with comments on this macro from the

> previous

> submission?

>

> If not, then any reason you want to submit this way for now and change

> it

> later when more HW channels are added?

>

> I don't think it's a good idea to have doorbell_mode_switch as false for

> channel 100 as we need to ring the DB every time we come out of M3.


My bad, I missed that point. I'm going to fix that. BTW would it no
make sense to always enabled that from MHI core (and not let the
controller driver to choose)?

>

> The current proposal will become inconsistent when more HW channels with

> different requirements are added so my suggestion was to accommodate

> these

> now. Also, I realized we need to update the regular macros as well but

> it

> can be dealt with separately.

>

> If you would like recommendations or would want to discuss this

> configuration

> thing further, please let me know.

>

> Thanks,

> Bhaumik

> ---

> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora

> Forum,

> a Linux Foundation Collaborative Project
Loic Poulain Nov. 24, 2020, 8:18 a.m. UTC | #3
On Tue, 24 Nov 2020 at 09:05, Loic Poulain <loic.poulain@linaro.org> wrote:
>

> On Tue, 24 Nov 2020 at 01:58, Bhaumik Bhatt <bbhatt@codeaurora.org> wrote:

> >

> > Hi Loic,

> > On 2020-11-23 06:11 AM, Loic Poulain wrote:

> > > Hardware channels have a feature called burst mode that allows to

> > > queue transfer ring element(s) (TRE) to a channel without ringing

> > > the device doorbell. In that mode, the device is polling the channel

> > > context for new elements. This reduces the frequency of host initiated

> > > doorbells and increase throughput.

> > >

> > > Create a new dedicated macro for hardware channels with burst enabled.

> > >

> > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

> > > ---

> > >  drivers/bus/mhi/pci_generic.c | 34 ++++++++++++++++++++++++++++++++--

> > >  1 file changed, 32 insertions(+), 2 deletions(-)

> > >

> > > diff --git a/drivers/bus/mhi/pci_generic.c

> > > b/drivers/bus/mhi/pci_generic.c

> > > index 09c6b26..0c07cf5 100644

> > > --- a/drivers/bus/mhi/pci_generic.c

> > > +++ b/drivers/bus/mhi/pci_generic.c

> > > @@ -78,6 +78,36 @@ struct mhi_pci_dev_info {

> > >               .offload_channel = false,       \

> > >       }

> > >

> > > +#define MHI_CHANNEL_CONFIG_HW_UL(ch_num, ch_name, el_count, ev_ring) \

> > > +     {                                               \

> > > +             .num = ch_num,                          \

> > > +             .name = ch_name,                        \

> > > +             .num_elements = el_count,               \

> > > +             .event_ring = ev_ring,                  \

> > > +             .dir = DMA_TO_DEVICE,                   \

> > > +             .ee_mask = BIT(MHI_EE_AMSS),            \

> > > +             .pollcfg = 0,                           \

> > > +             .doorbell = MHI_DB_BRST_ENABLE, \

> > > +             .lpm_notify = false,                    \

> > > +             .offload_channel = false,               \

> > > +             .doorbell_mode_switch = false,          \

> > > +     }                                               \

> > > +

> > > +#define MHI_CHANNEL_CONFIG_HW_DL(ch_num, ch_name, el_count, ev_ring) \

> > > +     {                                               \

> > > +             .num = ch_num,                          \

> > > +             .name = ch_name,                        \

> > > +             .num_elements = el_count,               \

> > > +             .event_ring = ev_ring,                  \

> > > +             .dir = DMA_FROM_DEVICE,                 \

> > > +             .ee_mask = BIT(MHI_EE_AMSS),            \

> > > +             .pollcfg = 0,                           \

> > > +             .doorbell = MHI_DB_BRST_ENABLE, \

> > > +             .lpm_notify = false,                    \

> > > +             .offload_channel = false,               \

> > > +             .doorbell_mode_switch = false,          \

> > > +     }

> > > +

> > >  #define MHI_EVENT_CONFIG_DATA(ev_ring)               \

> > >       {                                       \

> > >               .num_elements = 128,            \

> > > @@ -112,8 +142,8 @@ static const struct mhi_channel_config

> > > modem_qcom_v1_mhi_channels[] = {

> > >       MHI_CHANNEL_CONFIG_DL(15, "QMI", 4, 0),

> > >       MHI_CHANNEL_CONFIG_UL(20, "IPCR", 8, 0),

> > >       MHI_CHANNEL_CONFIG_DL(21, "IPCR", 8, 0),

> > > -     MHI_CHANNEL_CONFIG_UL(100, "IP_HW0", 128, 1),

> > > -     MHI_CHANNEL_CONFIG_DL(101, "IP_HW0", 128, 2),

> > > +     MHI_CHANNEL_CONFIG_HW_UL(100, "IP_HW0", 128, 1),

> > > +     MHI_CHANNEL_CONFIG_HW_DL(101, "IP_HW0", 128, 2),

> > >  };

> > >

> > >  static const struct mhi_event_config modem_qcom_v1_mhi_events[] = {

> > Did you somehow miss my email with comments on this macro from the

> > previous

> > submission?

> >

> > If not, then any reason you want to submit this way for now and change

> > it

> > later when more HW channels are added?

> >

> > I don't think it's a good idea to have doorbell_mode_switch as false for

> > channel 100 as we need to ring the DB every time we come out of M3.

>

> My bad, I missed that point. I'm going to fix that. BTW would it no

> make sense to always enabled that from MHI core (and not let the

> controller driver to choose)?


And let me know if there is any reason to not enable it
doorbell_mode_switch when burst mode is enabled. I would like macro
paramaeters as minimal as possible so that they simplify channel
configuration.

Regards,
Loic
diff mbox series

Patch

diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
index 09c6b26..0c07cf5 100644
--- a/drivers/bus/mhi/pci_generic.c
+++ b/drivers/bus/mhi/pci_generic.c
@@ -78,6 +78,36 @@  struct mhi_pci_dev_info {
 		.offload_channel = false,	\
 	}
 
+#define MHI_CHANNEL_CONFIG_HW_UL(ch_num, ch_name, el_count, ev_ring) \
+	{						\
+		.num = ch_num,				\
+		.name = ch_name,			\
+		.num_elements = el_count,		\
+		.event_ring = ev_ring,			\
+		.dir = DMA_TO_DEVICE,			\
+		.ee_mask = BIT(MHI_EE_AMSS),		\
+		.pollcfg = 0,				\
+		.doorbell = MHI_DB_BRST_ENABLE,	\
+		.lpm_notify = false,			\
+		.offload_channel = false,		\
+		.doorbell_mode_switch = false,		\
+	}						\
+
+#define MHI_CHANNEL_CONFIG_HW_DL(ch_num, ch_name, el_count, ev_ring) \
+	{						\
+		.num = ch_num,				\
+		.name = ch_name,			\
+		.num_elements = el_count,		\
+		.event_ring = ev_ring,			\
+		.dir = DMA_FROM_DEVICE,			\
+		.ee_mask = BIT(MHI_EE_AMSS),		\
+		.pollcfg = 0,				\
+		.doorbell = MHI_DB_BRST_ENABLE,	\
+		.lpm_notify = false,			\
+		.offload_channel = false,		\
+		.doorbell_mode_switch = false,		\
+	}
+
 #define MHI_EVENT_CONFIG_DATA(ev_ring)		\
 	{					\
 		.num_elements = 128,		\
@@ -112,8 +142,8 @@  static const struct mhi_channel_config modem_qcom_v1_mhi_channels[] = {
 	MHI_CHANNEL_CONFIG_DL(15, "QMI", 4, 0),
 	MHI_CHANNEL_CONFIG_UL(20, "IPCR", 8, 0),
 	MHI_CHANNEL_CONFIG_DL(21, "IPCR", 8, 0),
-	MHI_CHANNEL_CONFIG_UL(100, "IP_HW0", 128, 1),
-	MHI_CHANNEL_CONFIG_DL(101, "IP_HW0", 128, 2),
+	MHI_CHANNEL_CONFIG_HW_UL(100, "IP_HW0", 128, 1),
+	MHI_CHANNEL_CONFIG_HW_DL(101, "IP_HW0", 128, 2),
 };
 
 static const struct mhi_event_config modem_qcom_v1_mhi_events[] = {