[PATCH] bus: mhi: pci_generic: Apply no-op for wake using inband wake support flag

[PATCH] bus: mhi: pci_generic: Apply no-op for wake using inband wake support flag

[Bhaumik Bhatt]

Devices such as SDX24 do not have the provision for inband wake
doorbell in the form of channel 127. Newer devices such as SDX55
or SDX65 have it by default. Ensure the functionality is used
based on this such that device wake stays held when a client
driver uses mhi_device_get() API or the equivalent debugfs entry.

Fixes: e3e5e6508fc1 ("bus: mhi: pci_generic: No-Op for device_wake operations")
Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/pci_generic.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
index d84b743..31360a2 100644
--- a/drivers/bus/mhi/pci_generic.c
+++ b/drivers/bus/mhi/pci_generic.c
@@ -32,6 +32,7 @@
  * @edl: emergency download mode firmware path (if any)
  * @bar_num: PCI base address register to use for MHI MMIO register space
  * @dma_data_width: DMA transfer word size (32 or 64 bits)
+ * @no_inband_wake: Devices without inband wake support (such as sdx24)
  */
 struct mhi_pci_dev_info {
 	const struct mhi_controller_config *config;
@@ -40,6 +41,7 @@ struct mhi_pci_dev_info {
 	const char *edl;
 	unsigned int bar_num;
 	unsigned int dma_data_width;
+	bool no_inband_wake;
 };
 
 #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
@@ -242,7 +244,8 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx65_info = {
 	.edl = "qcom/sdx65m/edl.mbn",
 	.config = &modem_qcom_v1_mhiv_config,
 	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
-	.dma_data_width = 32
+	.dma_data_width = 32,
+	.no_inband_wake = false
 };
 
 static const struct mhi_pci_dev_info mhi_qcom_sdx55_info = {
@@ -251,7 +254,8 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx55_info = {
 	.edl = "qcom/sdx55m/edl.mbn",
 	.config = &modem_qcom_v1_mhiv_config,
 	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
-	.dma_data_width = 32
+	.dma_data_width = 32,
+	.no_inband_wake = false
 };
 
 static const struct mhi_pci_dev_info mhi_qcom_sdx24_info = {
@@ -259,7 +263,8 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx24_info = {
 	.edl = "qcom/prog_firehose_sdx24.mbn",
 	.config = &modem_qcom_v1_mhiv_config,
 	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
-	.dma_data_width = 32
+	.dma_data_width = 32,
+	.no_inband_wake = true
 };
 
 static const struct mhi_channel_config mhi_quectel_em1xx_channels[] = {
@@ -301,7 +306,8 @@ static const struct mhi_pci_dev_info mhi_quectel_em1xx_info = {
 	.edl = "qcom/prog_firehose_sdx24.mbn",
 	.config = &modem_quectel_em1xx_config,
 	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
-	.dma_data_width = 32
+	.dma_data_width = 32,
+	.no_inband_wake = true
 };
 
 static const struct mhi_channel_config mhi_foxconn_sdx55_channels[] = {
@@ -339,7 +345,8 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx55_info = {
 	.edl = "qcom/sdx55m/edl.mbn",
 	.config = &modem_foxconn_sdx55_config,
 	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
-	.dma_data_width = 32
+	.dma_data_width = 32,
+	.no_inband_wake = false
 };
 
 static const struct pci_device_id mhi_pci_id_table[] = {
@@ -640,9 +647,12 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	mhi_cntrl->status_cb = mhi_pci_status_cb;
 	mhi_cntrl->runtime_get = mhi_pci_runtime_get;
 	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
-	mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
-	mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
-	mhi_cntrl->wake_toggle = mhi_pci_wake_toggle_nop;
+
+	if (info->no_inband_wake) {
+		mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
+		mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
+		mhi_cntrl->wake_toggle = mhi_pci_wake_toggle_nop;
+	}
 
 	err = mhi_pci_claim(mhi_cntrl, info->bar_num, DMA_BIT_MASK(info->dma_data_width));
 	if (err)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] bus: mhi: pci_generic: Apply no-op for wake using inband wake support flag

[Loic Poulain]

On Thu, 17 Jun 2021 at 20:24, Bhaumik Bhatt <bbhatt@codeaurora.org> wrote:
>
> Devices such as SDX24 do not have the provision for inband wake
> doorbell in the form of channel 127. Newer devices such as SDX55
> or SDX65 have it by default. Ensure the functionality is used
> based on this such that device wake stays held when a client
> driver uses mhi_device_get() API or the equivalent debugfs entry.
>
> Fixes: e3e5e6508fc1 ("bus: mhi: pci_generic: No-Op for device_wake operations")
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>

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

Re: [PATCH] bus: mhi: pci_generic: Apply no-op for wake using inband wake support flag

[Hemant Kumar]

On Thu, 2021-06-17 at 11:23 -0700, Bhaumik Bhatt wrote:
> Devices such as SDX24 do not have the provision for inband wake
> doorbell in the form of channel 127. Newer devices such as SDX55
> or SDX65 have it by default. Ensure the functionality is used
> based on this such that device wake stays held when a client
> driver uses mhi_device_get() API or the equivalent debugfs entry.
> 
> Fixes: e3e5e6508fc1 ("bus: mhi: pci_generic: No-Op for device_wake
> operations")
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>

Re: [PATCH] bus: mhi: pci_generic: Apply no-op for wake using inband wake support flag

[Manivannan Sadhasivam]

On Thu, Jun 17, 2021 at 11:23:53AM -0700, Bhaumik Bhatt wrote:
> Devices such as SDX24 do not have the provision for inband wake
> doorbell in the form of channel 127. Newer devices such as SDX55
> or SDX65 have it by default. Ensure the functionality is used
> based on this such that device wake stays held when a client
> driver uses mhi_device_get() API or the equivalent debugfs entry.
> 
> Fixes: e3e5e6508fc1 ("bus: mhi: pci_generic: No-Op for device_wake operations")
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/pci_generic.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
> index d84b743..31360a2 100644
> --- a/drivers/bus/mhi/pci_generic.c
> +++ b/drivers/bus/mhi/pci_generic.c
> @@ -32,6 +32,7 @@
>   * @edl: emergency download mode firmware path (if any)
>   * @bar_num: PCI base address register to use for MHI MMIO register space
>   * @dma_data_width: DMA transfer word size (32 or 64 bits)
> + * @no_inband_wake: Devices without inband wake support (such as sdx24)

I'd rather like this field to be "inband_wake" and set to false/true
based on the capability of the devices. Rest looks good.

Thanks,
Mani

>   */
>  struct mhi_pci_dev_info {
>  	const struct mhi_controller_config *config;
> @@ -40,6 +41,7 @@ struct mhi_pci_dev_info {
>  	const char *edl;
>  	unsigned int bar_num;
>  	unsigned int dma_data_width;
> +	bool no_inband_wake;
>  };
>  
>  #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
> @@ -242,7 +244,8 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx65_info = {
>  	.edl = "qcom/sdx65m/edl.mbn",
>  	.config = &modem_qcom_v1_mhiv_config,
>  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> -	.dma_data_width = 32
> +	.dma_data_width = 32,
> +	.no_inband_wake = false
>  };
>  
>  static const struct mhi_pci_dev_info mhi_qcom_sdx55_info = {
> @@ -251,7 +254,8 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx55_info = {
>  	.edl = "qcom/sdx55m/edl.mbn",
>  	.config = &modem_qcom_v1_mhiv_config,
>  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> -	.dma_data_width = 32
> +	.dma_data_width = 32,
> +	.no_inband_wake = false
>  };
>  
>  static const struct mhi_pci_dev_info mhi_qcom_sdx24_info = {
> @@ -259,7 +263,8 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx24_info = {
>  	.edl = "qcom/prog_firehose_sdx24.mbn",
>  	.config = &modem_qcom_v1_mhiv_config,
>  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> -	.dma_data_width = 32
> +	.dma_data_width = 32,
> +	.no_inband_wake = true
>  };
>  
>  static const struct mhi_channel_config mhi_quectel_em1xx_channels[] = {
> @@ -301,7 +306,8 @@ static const struct mhi_pci_dev_info mhi_quectel_em1xx_info = {
>  	.edl = "qcom/prog_firehose_sdx24.mbn",
>  	.config = &modem_quectel_em1xx_config,
>  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> -	.dma_data_width = 32
> +	.dma_data_width = 32,
> +	.no_inband_wake = true
>  };
>  
>  static const struct mhi_channel_config mhi_foxconn_sdx55_channels[] = {
> @@ -339,7 +345,8 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx55_info = {
>  	.edl = "qcom/sdx55m/edl.mbn",
>  	.config = &modem_foxconn_sdx55_config,
>  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> -	.dma_data_width = 32
> +	.dma_data_width = 32,
> +	.no_inband_wake = false
>  };
>  
>  static const struct pci_device_id mhi_pci_id_table[] = {
> @@ -640,9 +647,12 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	mhi_cntrl->status_cb = mhi_pci_status_cb;
>  	mhi_cntrl->runtime_get = mhi_pci_runtime_get;
>  	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
> -	mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
> -	mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
> -	mhi_cntrl->wake_toggle = mhi_pci_wake_toggle_nop;
> +
> +	if (info->no_inband_wake) {
> +		mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
> +		mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
> +		mhi_cntrl->wake_toggle = mhi_pci_wake_toggle_nop;
> +	}
>  
>  	err = mhi_pci_claim(mhi_cntrl, info->bar_num, DMA_BIT_MASK(info->dma_data_width));
>  	if (err)
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH] bus: mhi: pci_generic: Apply no-op for wake using inband wake support flag

[Bhaumik Bhatt]

Hi Mani,
On 2021-06-17 11:52 PM, Manivannan Sadhasivam wrote:
> On Thu, Jun 17, 2021 at 11:23:53AM -0700, Bhaumik Bhatt wrote:
>> Devices such as SDX24 do not have the provision for inband wake
>> doorbell in the form of channel 127. Newer devices such as SDX55
>> or SDX65 have it by default. Ensure the functionality is used
>> based on this such that device wake stays held when a client
>> driver uses mhi_device_get() API or the equivalent debugfs entry.
>> 
>> Fixes: e3e5e6508fc1 ("bus: mhi: pci_generic: No-Op for device_wake 
>> operations")
>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> ---
>>  drivers/bus/mhi/pci_generic.c | 26 ++++++++++++++++++--------
>>  1 file changed, 18 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/bus/mhi/pci_generic.c 
>> b/drivers/bus/mhi/pci_generic.c
>> index d84b743..31360a2 100644
>> --- a/drivers/bus/mhi/pci_generic.c
>> +++ b/drivers/bus/mhi/pci_generic.c
>> @@ -32,6 +32,7 @@
>>   * @edl: emergency download mode firmware path (if any)
>>   * @bar_num: PCI base address register to use for MHI MMIO register 
>> space
>>   * @dma_data_width: DMA transfer word size (32 or 64 bits)
>> + * @no_inband_wake: Devices without inband wake support (such as 
>> sdx24)
> 
> I'd rather like this field to be "inband_wake" and set to false/true
> based on the capability of the devices. Rest looks good.
> 
> Thanks,
> Mani
> 
I should have known this was coming :)

Can I use sideband_wake instead of no_inband_wake and leave the booleans 
as is?
By default, inband_wake will have to be true for any and all devices 
moving
forward.

Please let me know your preference.
>>   */
>>  struct mhi_pci_dev_info {
>>  	const struct mhi_controller_config *config;
>> @@ -40,6 +41,7 @@ struct mhi_pci_dev_info {
>>  	const char *edl;
>>  	unsigned int bar_num;
>>  	unsigned int dma_data_width;
>> +	bool no_inband_wake;
>>  };
>> 
>>  #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
>> @@ -242,7 +244,8 @@ static const struct mhi_pci_dev_info 
>> mhi_qcom_sdx65_info = {
>>  	.edl = "qcom/sdx65m/edl.mbn",
>>  	.config = &modem_qcom_v1_mhiv_config,
>>  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>> -	.dma_data_width = 32
>> +	.dma_data_width = 32,
>> +	.no_inband_wake = false
>>  };
>> 
>>  static const struct mhi_pci_dev_info mhi_qcom_sdx55_info = {
>> @@ -251,7 +254,8 @@ static const struct mhi_pci_dev_info 
>> mhi_qcom_sdx55_info = {
>>  	.edl = "qcom/sdx55m/edl.mbn",
>>  	.config = &modem_qcom_v1_mhiv_config,
>>  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>> -	.dma_data_width = 32
>> +	.dma_data_width = 32,
>> +	.no_inband_wake = false
>>  };
>> 
>>  static const struct mhi_pci_dev_info mhi_qcom_sdx24_info = {
>> @@ -259,7 +263,8 @@ static const struct mhi_pci_dev_info 
>> mhi_qcom_sdx24_info = {
>>  	.edl = "qcom/prog_firehose_sdx24.mbn",
>>  	.config = &modem_qcom_v1_mhiv_config,
>>  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>> -	.dma_data_width = 32
>> +	.dma_data_width = 32,
>> +	.no_inband_wake = true
>>  };
>> 
>>  static const struct mhi_channel_config mhi_quectel_em1xx_channels[] = 
>> {
>> @@ -301,7 +306,8 @@ static const struct mhi_pci_dev_info 
>> mhi_quectel_em1xx_info = {
>>  	.edl = "qcom/prog_firehose_sdx24.mbn",
>>  	.config = &modem_quectel_em1xx_config,
>>  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>> -	.dma_data_width = 32
>> +	.dma_data_width = 32,
>> +	.no_inband_wake = true
>>  };
>> 
>>  static const struct mhi_channel_config mhi_foxconn_sdx55_channels[] = 
>> {
>> @@ -339,7 +345,8 @@ static const struct mhi_pci_dev_info 
>> mhi_foxconn_sdx55_info = {
>>  	.edl = "qcom/sdx55m/edl.mbn",
>>  	.config = &modem_foxconn_sdx55_config,
>>  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>> -	.dma_data_width = 32
>> +	.dma_data_width = 32,
>> +	.no_inband_wake = false
>>  };
>> 
>>  static const struct pci_device_id mhi_pci_id_table[] = {
>> @@ -640,9 +647,12 @@ static int mhi_pci_probe(struct pci_dev *pdev, 
>> const struct pci_device_id *id)
>>  	mhi_cntrl->status_cb = mhi_pci_status_cb;
>>  	mhi_cntrl->runtime_get = mhi_pci_runtime_get;
>>  	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>> -	mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
>> -	mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
>> -	mhi_cntrl->wake_toggle = mhi_pci_wake_toggle_nop;
>> +
>> +	if (info->no_inband_wake) {
>> +		mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
>> +		mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
>> +		mhi_cntrl->wake_toggle = mhi_pci_wake_toggle_nop;
>> +	}
>> 
>>  	err = mhi_pci_claim(mhi_cntrl, info->bar_num, 
>> DMA_BIT_MASK(info->dma_data_width));
>>  	if (err)
>> --
>> 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

Re: [PATCH] bus: mhi: pci_generic: Apply no-op for wake using inband wake support flag

[Manivannan Sadhasivam]

On Fri, Jun 18, 2021 at 10:06:28AM -0700, Bhaumik Bhatt wrote:
> Hi Mani,
> On 2021-06-17 11:52 PM, Manivannan Sadhasivam wrote:
> > On Thu, Jun 17, 2021 at 11:23:53AM -0700, Bhaumik Bhatt wrote:
> > > Devices such as SDX24 do not have the provision for inband wake
> > > doorbell in the form of channel 127. Newer devices such as SDX55
> > > or SDX65 have it by default. Ensure the functionality is used
> > > based on this such that device wake stays held when a client
> > > driver uses mhi_device_get() API or the equivalent debugfs entry.
> > > 
> > > Fixes: e3e5e6508fc1 ("bus: mhi: pci_generic: No-Op for device_wake
> > > operations")
> > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > ---
> > >  drivers/bus/mhi/pci_generic.c | 26 ++++++++++++++++++--------
> > >  1 file changed, 18 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/bus/mhi/pci_generic.c
> > > b/drivers/bus/mhi/pci_generic.c
> > > index d84b743..31360a2 100644
> > > --- a/drivers/bus/mhi/pci_generic.c
> > > +++ b/drivers/bus/mhi/pci_generic.c
> > > @@ -32,6 +32,7 @@
> > >   * @edl: emergency download mode firmware path (if any)
> > >   * @bar_num: PCI base address register to use for MHI MMIO register
> > > space
> > >   * @dma_data_width: DMA transfer word size (32 or 64 bits)
> > > + * @no_inband_wake: Devices without inband wake support (such as
> > > sdx24)
> > 
> > I'd rather like this field to be "inband_wake" and set to false/true
> > based on the capability of the devices. Rest looks good.
> > 
> > Thanks,
> > Mani
> > 
> I should have known this was coming :)
> 
> Can I use sideband_wake instead of no_inband_wake and leave the booleans as
> is?

Do you mean, the older devices uses a dedicated sideband GPIO for
controlling the wakeup? If so, what is it?

> By default, inband_wake will have to be true for any and all devices moving
> forward.
> 

Right but still the functionality is not present in older devices. So I
think it makes sense to stay with "inband_wake".

Thanks,
Mani

> Please let me know your preference.
> > >   */
> > >  struct mhi_pci_dev_info {
> > >  	const struct mhi_controller_config *config;
> > > @@ -40,6 +41,7 @@ struct mhi_pci_dev_info {
> > >  	const char *edl;
> > >  	unsigned int bar_num;
> > >  	unsigned int dma_data_width;
> > > +	bool no_inband_wake;
> > >  };
> > > 
> > >  #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
> > > @@ -242,7 +244,8 @@ static const struct mhi_pci_dev_info
> > > mhi_qcom_sdx65_info = {
> > >  	.edl = "qcom/sdx65m/edl.mbn",
> > >  	.config = &modem_qcom_v1_mhiv_config,
> > >  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> > > -	.dma_data_width = 32
> > > +	.dma_data_width = 32,
> > > +	.no_inband_wake = false
> > >  };
> > > 
> > >  static const struct mhi_pci_dev_info mhi_qcom_sdx55_info = {
> > > @@ -251,7 +254,8 @@ static const struct mhi_pci_dev_info
> > > mhi_qcom_sdx55_info = {
> > >  	.edl = "qcom/sdx55m/edl.mbn",
> > >  	.config = &modem_qcom_v1_mhiv_config,
> > >  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> > > -	.dma_data_width = 32
> > > +	.dma_data_width = 32,
> > > +	.no_inband_wake = false
> > >  };
> > > 
> > >  static const struct mhi_pci_dev_info mhi_qcom_sdx24_info = {
> > > @@ -259,7 +263,8 @@ static const struct mhi_pci_dev_info
> > > mhi_qcom_sdx24_info = {
> > >  	.edl = "qcom/prog_firehose_sdx24.mbn",
> > >  	.config = &modem_qcom_v1_mhiv_config,
> > >  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> > > -	.dma_data_width = 32
> > > +	.dma_data_width = 32,
> > > +	.no_inband_wake = true
> > >  };
> > > 
> > >  static const struct mhi_channel_config mhi_quectel_em1xx_channels[]
> > > = {
> > > @@ -301,7 +306,8 @@ static const struct mhi_pci_dev_info
> > > mhi_quectel_em1xx_info = {
> > >  	.edl = "qcom/prog_firehose_sdx24.mbn",
> > >  	.config = &modem_quectel_em1xx_config,
> > >  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> > > -	.dma_data_width = 32
> > > +	.dma_data_width = 32,
> > > +	.no_inband_wake = true
> > >  };
> > > 
> > >  static const struct mhi_channel_config mhi_foxconn_sdx55_channels[]
> > > = {
> > > @@ -339,7 +345,8 @@ static const struct mhi_pci_dev_info
> > > mhi_foxconn_sdx55_info = {
> > >  	.edl = "qcom/sdx55m/edl.mbn",
> > >  	.config = &modem_foxconn_sdx55_config,
> > >  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> > > -	.dma_data_width = 32
> > > +	.dma_data_width = 32,
> > > +	.no_inband_wake = false
> > >  };
> > > 
> > >  static const struct pci_device_id mhi_pci_id_table[] = {
> > > @@ -640,9 +647,12 @@ static int mhi_pci_probe(struct pci_dev *pdev,
> > > const struct pci_device_id *id)
> > >  	mhi_cntrl->status_cb = mhi_pci_status_cb;
> > >  	mhi_cntrl->runtime_get = mhi_pci_runtime_get;
> > >  	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
> > > -	mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
> > > -	mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
> > > -	mhi_cntrl->wake_toggle = mhi_pci_wake_toggle_nop;
> > > +
> > > +	if (info->no_inband_wake) {
> > > +		mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
> > > +		mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
> > > +		mhi_cntrl->wake_toggle = mhi_pci_wake_toggle_nop;
> > > +	}
> > > 
> > >  	err = mhi_pci_claim(mhi_cntrl, info->bar_num,
> > > DMA_BIT_MASK(info->dma_data_width));
> > >  	if (err)
> > > --
> > > 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

Re: [PATCH] bus: mhi: pci_generic: Apply no-op for wake using inband wake support flag

[Bhaumik Bhatt]

On 2021-06-18 10:27 AM, Manivannan Sadhasivam wrote:
> On Fri, Jun 18, 2021 at 10:06:28AM -0700, Bhaumik Bhatt wrote:
>> Hi Mani,
>> On 2021-06-17 11:52 PM, Manivannan Sadhasivam wrote:
>> > On Thu, Jun 17, 2021 at 11:23:53AM -0700, Bhaumik Bhatt wrote:
>> > > Devices such as SDX24 do not have the provision for inband wake
>> > > doorbell in the form of channel 127. Newer devices such as SDX55
>> > > or SDX65 have it by default. Ensure the functionality is used
>> > > based on this such that device wake stays held when a client
>> > > driver uses mhi_device_get() API or the equivalent debugfs entry.
>> > >
>> > > Fixes: e3e5e6508fc1 ("bus: mhi: pci_generic: No-Op for device_wake
>> > > operations")
>> > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> > > ---
>> > >  drivers/bus/mhi/pci_generic.c | 26 ++++++++++++++++++--------
>> > >  1 file changed, 18 insertions(+), 8 deletions(-)
>> > >
>> > > diff --git a/drivers/bus/mhi/pci_generic.c
>> > > b/drivers/bus/mhi/pci_generic.c
>> > > index d84b743..31360a2 100644
>> > > --- a/drivers/bus/mhi/pci_generic.c
>> > > +++ b/drivers/bus/mhi/pci_generic.c
>> > > @@ -32,6 +32,7 @@
>> > >   * @edl: emergency download mode firmware path (if any)
>> > >   * @bar_num: PCI base address register to use for MHI MMIO register
>> > > space
>> > >   * @dma_data_width: DMA transfer word size (32 or 64 bits)
>> > > + * @no_inband_wake: Devices without inband wake support (such as
>> > > sdx24)
>> >
>> > I'd rather like this field to be "inband_wake" and set to false/true
>> > based on the capability of the devices. Rest looks good.
>> >
>> > Thanks,
>> > Mani
>> >
>> I should have known this was coming :)
>> 
>> Can I use sideband_wake instead of no_inband_wake and leave the 
>> booleans as
>> is?
> 
> Do you mean, the older devices uses a dedicated sideband GPIO for
> controlling the wakeup? If so, what is it?
> 
 From what I know, this used to be the DEVICE_WAKE GPIO for SDX24. We 
moved to use
the channel 127 doorbell as WAKE doorbell for this purpose SDX55 onwards 
for ease
of deployment.

>> By default, inband_wake will have to be true for any and all devices 
>> moving
>> forward.
>> 
> 
> Right but still the functionality is not present in older devices. So I
> think it makes sense to stay with "inband_wake".
> 
> Thanks,
> Mani
> 
So, by default we would not have to add explicit "true" for inband_wake 
as
newer devices will have the feature and controller drivers do not need 
to
populate the entry for a static const struct.

>> Please let me know your preference.
>> > >   */
>> > >  struct mhi_pci_dev_info {
>> > >  	const struct mhi_controller_config *config;
>> > > @@ -40,6 +41,7 @@ struct mhi_pci_dev_info {
>> > >  	const char *edl;
>> > >  	unsigned int bar_num;
>> > >  	unsigned int dma_data_width;
>> > > +	bool no_inband_wake;
>> > >  };
>> > >
>> > >  #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
>> > > @@ -242,7 +244,8 @@ static const struct mhi_pci_dev_info
>> > > mhi_qcom_sdx65_info = {
>> > >  	.edl = "qcom/sdx65m/edl.mbn",
>> > >  	.config = &modem_qcom_v1_mhiv_config,
>> > >  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>> > > -	.dma_data_width = 32
>> > > +	.dma_data_width = 32,
>> > > +	.no_inband_wake = false
>> > >  };
>> > >
>> > >  static const struct mhi_pci_dev_info mhi_qcom_sdx55_info = {
>> > > @@ -251,7 +254,8 @@ static const struct mhi_pci_dev_info
>> > > mhi_qcom_sdx55_info = {
>> > >  	.edl = "qcom/sdx55m/edl.mbn",
>> > >  	.config = &modem_qcom_v1_mhiv_config,
>> > >  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>> > > -	.dma_data_width = 32
>> > > +	.dma_data_width = 32,
>> > > +	.no_inband_wake = false
>> > >  };
>> > >
>> > >  static const struct mhi_pci_dev_info mhi_qcom_sdx24_info = {
>> > > @@ -259,7 +263,8 @@ static const struct mhi_pci_dev_info
>> > > mhi_qcom_sdx24_info = {
>> > >  	.edl = "qcom/prog_firehose_sdx24.mbn",
>> > >  	.config = &modem_qcom_v1_mhiv_config,
>> > >  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>> > > -	.dma_data_width = 32
>> > > +	.dma_data_width = 32,
>> > > +	.no_inband_wake = true
>> > >  };
>> > >
>> > >  static const struct mhi_channel_config mhi_quectel_em1xx_channels[]
>> > > = {
>> > > @@ -301,7 +306,8 @@ static const struct mhi_pci_dev_info
>> > > mhi_quectel_em1xx_info = {
>> > >  	.edl = "qcom/prog_firehose_sdx24.mbn",
>> > >  	.config = &modem_quectel_em1xx_config,
>> > >  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>> > > -	.dma_data_width = 32
>> > > +	.dma_data_width = 32,
>> > > +	.no_inband_wake = true
>> > >  };
>> > >
>> > >  static const struct mhi_channel_config mhi_foxconn_sdx55_channels[]
>> > > = {
>> > > @@ -339,7 +345,8 @@ static const struct mhi_pci_dev_info
>> > > mhi_foxconn_sdx55_info = {
>> > >  	.edl = "qcom/sdx55m/edl.mbn",
>> > >  	.config = &modem_foxconn_sdx55_config,
>> > >  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>> > > -	.dma_data_width = 32
>> > > +	.dma_data_width = 32,
>> > > +	.no_inband_wake = false
>> > >  };
>> > >
>> > >  static const struct pci_device_id mhi_pci_id_table[] = {
>> > > @@ -640,9 +647,12 @@ static int mhi_pci_probe(struct pci_dev *pdev,
>> > > const struct pci_device_id *id)
>> > >  	mhi_cntrl->status_cb = mhi_pci_status_cb;
>> > >  	mhi_cntrl->runtime_get = mhi_pci_runtime_get;
>> > >  	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>> > > -	mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
>> > > -	mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
>> > > -	mhi_cntrl->wake_toggle = mhi_pci_wake_toggle_nop;
>> > > +
>> > > +	if (info->no_inband_wake) {
>> > > +		mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
>> > > +		mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
>> > > +		mhi_cntrl->wake_toggle = mhi_pci_wake_toggle_nop;
>> > > +	}
>> > >
>> > >  	err = mhi_pci_claim(mhi_cntrl, info->bar_num,
>> > > DMA_BIT_MASK(info->dma_data_width));
>> > >  	if (err)
>> > > --
>> > > 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