[PATCH v2 1/2] bus: mhi: host: Import mux_id item

[PATCH v2 1/2] bus: mhi: host: Import mux_id item

[Slark Xiao]

For SDX72 MBIM mode, it starts data mux id from 112 instead of 0.
This would lead to device can't ping outside successfully.
Also MBIM side would report "bad packet session (112)".
So we add a default mux_id value for SDX72. And this value
would be transferred to wwan mbim side.

Signed-off-by: Slark Xiao <slark_xiao@163.com>
---
 drivers/bus/mhi/host/pci_generic.c | 3 +++
 include/linux/mhi.h                | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
index 0b483c7c76a1..9e9adf8320d2 100644
--- a/drivers/bus/mhi/host/pci_generic.c
+++ b/drivers/bus/mhi/host/pci_generic.c
@@ -53,6 +53,7 @@ struct mhi_pci_dev_info {
 	unsigned int dma_data_width;
 	unsigned int mru_default;
 	bool sideband_wake;
+	unsigned int mux_id;
 };
 
 #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
@@ -469,6 +470,7 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx72_info = {
 	.dma_data_width = 32,
 	.mru_default = 32768,
 	.sideband_wake = false,
+	.mux_id = 112,
 };
 
 static const struct mhi_channel_config mhi_mv3x_channels[] = {
@@ -1035,6 +1037,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	mhi_cntrl->runtime_get = mhi_pci_runtime_get;
 	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
 	mhi_cntrl->mru = info->mru_default;
+	mhi_cntrl->link_id = info->mux_id;
 
 	if (info->edl_trigger)
 		mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index b573f15762f8..499c735fb1a3 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -361,6 +361,7 @@ struct mhi_controller_config {
  * @wake_set: Device wakeup set flag
  * @irq_flags: irq flags passed to request_irq (optional)
  * @mru: the default MRU for the MHI device
+ * @link_id: the default link
  *
  * Fields marked as (required) need to be populated by the controller driver
  * before calling mhi_register_controller(). For the fields marked as (optional)
@@ -445,6 +446,7 @@ struct mhi_controller {
 	bool wake_set;
 	unsigned long irq_flags;
 	u32 mru;
+	u32 link_id;
 };
 
 /**
-- 
2.25.1

Re: [PATCH v2 1/2] bus: mhi: host: Import mux_id item

[Manivannan Sadhasivam]

On Wed, Jun 12, 2024 at 05:38:42PM +0800, Slark Xiao wrote:

Subject could be improved:

bus: mhi: host: Add configurable mux_id for MBIM mode

> For SDX72 MBIM mode, it starts data mux id from 112 instead of 0.
> This would lead to device can't ping outside successfully.
> Also MBIM side would report "bad packet session (112)".
> So we add a default mux_id value for SDX72. And this value
> would be transferred to wwan mbim side.
> 
> Signed-off-by: Slark Xiao <slark_xiao@163.com>
> ---
>  drivers/bus/mhi/host/pci_generic.c | 3 +++
>  include/linux/mhi.h                | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> index 0b483c7c76a1..9e9adf8320d2 100644
> --- a/drivers/bus/mhi/host/pci_generic.c
> +++ b/drivers/bus/mhi/host/pci_generic.c
> @@ -53,6 +53,7 @@ struct mhi_pci_dev_info {
>  	unsigned int dma_data_width;
>  	unsigned int mru_default;
>  	bool sideband_wake;
> +	unsigned int mux_id;
>  };
>  
>  #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
> @@ -469,6 +470,7 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx72_info = {
>  	.dma_data_width = 32,
>  	.mru_default = 32768,
>  	.sideband_wake = false,
> +	.mux_id = 112,
>  };
>  
>  static const struct mhi_channel_config mhi_mv3x_channels[] = {
> @@ -1035,6 +1037,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	mhi_cntrl->runtime_get = mhi_pci_runtime_get;
>  	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>  	mhi_cntrl->mru = info->mru_default;
> +	mhi_cntrl->link_id = info->mux_id;

Again, 'link_id' is just a WWAN term. Use 'mux_id' here also.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re:Re: [PATCH v2 1/2] bus: mhi: host: Import mux_id item

[Slark Xiao]

At 2024-06-12 17:46:09, "Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org> wrote:
>On Wed, Jun 12, 2024 at 05:38:42PM +0800, Slark Xiao wrote:
>
>Subject could be improved:
>
>bus: mhi: host: Add configurable mux_id for MBIM mode
>

Would be updated in v3 version.

>> For SDX72 MBIM mode, it starts data mux id from 112 instead of 0.
>> This would lead to device can't ping outside successfully.
>> Also MBIM side would report "bad packet session (112)".
>> So we add a default mux_id value for SDX72. And this value
>> would be transferred to wwan mbim side.
>> 
>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
>> ---
>>  drivers/bus/mhi/host/pci_generic.c | 3 +++
>>  include/linux/mhi.h                | 2 ++
>>  2 files changed, 5 insertions(+)
>> 
>> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
>> index 0b483c7c76a1..9e9adf8320d2 100644
>> --- a/drivers/bus/mhi/host/pci_generic.c
>> +++ b/drivers/bus/mhi/host/pci_generic.c
>> @@ -53,6 +53,7 @@ struct mhi_pci_dev_info {
>>  	unsigned int dma_data_width;
>>  	unsigned int mru_default;
>>  	bool sideband_wake;
>> +	unsigned int mux_id;
>>  };
>>  
>>  #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
>> @@ -469,6 +470,7 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx72_info = {
>>  	.dma_data_width = 32,
>>  	.mru_default = 32768,
>>  	.sideband_wake = false,
>> +	.mux_id = 112,
>>  };
>>  
>>  static const struct mhi_channel_config mhi_mv3x_channels[] = {
>> @@ -1035,6 +1037,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>  	mhi_cntrl->runtime_get = mhi_pci_runtime_get;
>>  	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>>  	mhi_cntrl->mru = info->mru_default;
>> +	mhi_cntrl->link_id = info->mux_id;
>
>Again, 'link_id' is just a WWAN term. Use 'mux_id' here also.
>
>- Mani

I have updated the item name in mhi side but kept in wwan side. The value 
"mhi_cntrl->link_id" would be called in function mhi_mbim_probe() of 
mhi_wwan_mbim.c. According the description of last parameter of function 
'wwan_register_ops()' in mhi_mbim_probe() :

* @def_link_id: id of default link that will be automatically created by
*     the WWAN core for the WWAN device.

So are you sure we shall use mux_id in wwan side? Please help reconfirm it.

Thanks!
>
>-- 
>மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 1/2] bus: mhi: host: Import mux_id item

[Jeffrey Hugo]

On 6/12/2024 3:46 AM, Manivannan Sadhasivam wrote:
> On Wed, Jun 12, 2024 at 05:38:42PM +0800, Slark Xiao wrote:
> 
> Subject could be improved:
> 
> bus: mhi: host: Add configurable mux_id for MBIM mode
> 
>> For SDX72 MBIM mode, it starts data mux id from 112 instead of 0.
>> This would lead to device can't ping outside successfully.
>> Also MBIM side would report "bad packet session (112)".
>> So we add a default mux_id value for SDX72. And this value
>> would be transferred to wwan mbim side.
>>
>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
>> ---
>>   drivers/bus/mhi/host/pci_generic.c | 3 +++
>>   include/linux/mhi.h                | 2 ++
>>   2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
>> index 0b483c7c76a1..9e9adf8320d2 100644
>> --- a/drivers/bus/mhi/host/pci_generic.c
>> +++ b/drivers/bus/mhi/host/pci_generic.c
>> @@ -53,6 +53,7 @@ struct mhi_pci_dev_info {
>>   	unsigned int dma_data_width;
>>   	unsigned int mru_default;
>>   	bool sideband_wake;
>> +	unsigned int mux_id;
>>   };
>>   
>>   #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
>> @@ -469,6 +470,7 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx72_info = {
>>   	.dma_data_width = 32,
>>   	.mru_default = 32768,
>>   	.sideband_wake = false,
>> +	.mux_id = 112,
>>   };
>>   
>>   static const struct mhi_channel_config mhi_mv3x_channels[] = {
>> @@ -1035,6 +1037,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   	mhi_cntrl->runtime_get = mhi_pci_runtime_get;
>>   	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>>   	mhi_cntrl->mru = info->mru_default;
>> +	mhi_cntrl->link_id = info->mux_id;
> 
> Again, 'link_id' is just a WWAN term. Use 'mux_id' here also.

Does this really belong in MHI?  If this was DT, I don't think we would 
put this value in DT, but rather have the driver (MBIM) detect the 
device and code in the required value.

Furthermore, if this is included in MHI, it seems to be a property of 
the channel, and not the controller.

-Jeff

Re: [PATCH v2 1/2] bus: mhi: host: Import mux_id item

[Manivannan Sadhasivam]

On Wed, Jun 12, 2024 at 08:19:13AM -0600, Jeffrey Hugo wrote:
> On 6/12/2024 3:46 AM, Manivannan Sadhasivam wrote:
> > On Wed, Jun 12, 2024 at 05:38:42PM +0800, Slark Xiao wrote:
> > 
> > Subject could be improved:
> > 
> > bus: mhi: host: Add configurable mux_id for MBIM mode
> > 
> > > For SDX72 MBIM mode, it starts data mux id from 112 instead of 0.
> > > This would lead to device can't ping outside successfully.
> > > Also MBIM side would report "bad packet session (112)".
> > > So we add a default mux_id value for SDX72. And this value
> > > would be transferred to wwan mbim side.
> > > 
> > > Signed-off-by: Slark Xiao <slark_xiao@163.com>
> > > ---
> > >   drivers/bus/mhi/host/pci_generic.c | 3 +++
> > >   include/linux/mhi.h                | 2 ++
> > >   2 files changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> > > index 0b483c7c76a1..9e9adf8320d2 100644
> > > --- a/drivers/bus/mhi/host/pci_generic.c
> > > +++ b/drivers/bus/mhi/host/pci_generic.c
> > > @@ -53,6 +53,7 @@ struct mhi_pci_dev_info {
> > >   	unsigned int dma_data_width;
> > >   	unsigned int mru_default;
> > >   	bool sideband_wake;
> > > +	unsigned int mux_id;
> > >   };
> > >   #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
> > > @@ -469,6 +470,7 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx72_info = {
> > >   	.dma_data_width = 32,
> > >   	.mru_default = 32768,
> > >   	.sideband_wake = false,
> > > +	.mux_id = 112,
> > >   };
> > >   static const struct mhi_channel_config mhi_mv3x_channels[] = {
> > > @@ -1035,6 +1037,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > >   	mhi_cntrl->runtime_get = mhi_pci_runtime_get;
> > >   	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
> > >   	mhi_cntrl->mru = info->mru_default;
> > > +	mhi_cntrl->link_id = info->mux_id;
> > 
> > Again, 'link_id' is just a WWAN term. Use 'mux_id' here also.
> 
> Does this really belong in MHI?  If this was DT, I don't think we would put
> this value in DT, but rather have the driver (MBIM) detect the device and
> code in the required value.
> 

I believe this is a modem value rather than MHI. But I was OK with keeping it in
MHI driver since we kind of keep modem specific config.

But if WWAN can detect the device and apply the config, I'm all over it.

- Mani

> Furthermore, if this is included in MHI, it seems to be a property of the
> channel, and not the controller.
> 
> -Jeff

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 1/2] bus: mhi: host: Import mux_id item

[Loic Poulain]

On Wed, 12 Jun 2024 at 16:51, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Wed, Jun 12, 2024 at 08:19:13AM -0600, Jeffrey Hugo wrote:
> > On 6/12/2024 3:46 AM, Manivannan Sadhasivam wrote:
> > > On Wed, Jun 12, 2024 at 05:38:42PM +0800, Slark Xiao wrote:
> > >
> > > Subject could be improved:
> > >
> > > bus: mhi: host: Add configurable mux_id for MBIM mode
> > >
> > > > For SDX72 MBIM mode, it starts data mux id from 112 instead of 0.
> > > > This would lead to device can't ping outside successfully.
> > > > Also MBIM side would report "bad packet session (112)".
> > > > So we add a default mux_id value for SDX72. And this value
> > > > would be transferred to wwan mbim side.
> > > >
> > > > Signed-off-by: Slark Xiao <slark_xiao@163.com>
> > > > ---
> > > >   drivers/bus/mhi/host/pci_generic.c | 3 +++
> > > >   include/linux/mhi.h                | 2 ++
> > > >   2 files changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> > > > index 0b483c7c76a1..9e9adf8320d2 100644
> > > > --- a/drivers/bus/mhi/host/pci_generic.c
> > > > +++ b/drivers/bus/mhi/host/pci_generic.c
> > > > @@ -53,6 +53,7 @@ struct mhi_pci_dev_info {
> > > >           unsigned int dma_data_width;
> > > >           unsigned int mru_default;
> > > >           bool sideband_wake;
> > > > + unsigned int mux_id;
> > > >   };
> > > >   #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
> > > > @@ -469,6 +470,7 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx72_info = {
> > > >           .dma_data_width = 32,
> > > >           .mru_default = 32768,
> > > >           .sideband_wake = false,
> > > > + .mux_id = 112,
> > > >   };
> > > >   static const struct mhi_channel_config mhi_mv3x_channels[] = {
> > > > @@ -1035,6 +1037,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > >           mhi_cntrl->runtime_get = mhi_pci_runtime_get;
> > > >           mhi_cntrl->runtime_put = mhi_pci_runtime_put;
> > > >           mhi_cntrl->mru = info->mru_default;
> > > > + mhi_cntrl->link_id = info->mux_id;
> > >
> > > Again, 'link_id' is just a WWAN term. Use 'mux_id' here also.
> >
> > Does this really belong in MHI?  If this was DT, I don't think we would put
> > this value in DT, but rather have the driver (MBIM) detect the device and
> > code in the required value.
> >
>
> I believe this is a modem value rather than MHI. But I was OK with keeping it in
> MHI driver since we kind of keep modem specific config.
>
> But if WWAN can detect the device and apply the config, I'm all over it.

That would require at least some information from the MHI bus for the
MBIM driver
to make a decision, such as a generic device ID, or quirk flags...

Regards,
Loic

Re: [PATCH v2 1/2] bus: mhi: host: Import mux_id item

[Jeffrey Hugo]

On 6/14/2024 4:17 AM, Loic Poulain wrote:
> On Wed, 12 Jun 2024 at 16:51, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
>>
>> On Wed, Jun 12, 2024 at 08:19:13AM -0600, Jeffrey Hugo wrote:
>>> On 6/12/2024 3:46 AM, Manivannan Sadhasivam wrote:
>>>> On Wed, Jun 12, 2024 at 05:38:42PM +0800, Slark Xiao wrote:
>>>>
>>>> Subject could be improved:
>>>>
>>>> bus: mhi: host: Add configurable mux_id for MBIM mode
>>>>
>>>>> For SDX72 MBIM mode, it starts data mux id from 112 instead of 0.
>>>>> This would lead to device can't ping outside successfully.
>>>>> Also MBIM side would report "bad packet session (112)".
>>>>> So we add a default mux_id value for SDX72. And this value
>>>>> would be transferred to wwan mbim side.
>>>>>
>>>>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
>>>>> ---
>>>>>    drivers/bus/mhi/host/pci_generic.c | 3 +++
>>>>>    include/linux/mhi.h                | 2 ++
>>>>>    2 files changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
>>>>> index 0b483c7c76a1..9e9adf8320d2 100644
>>>>> --- a/drivers/bus/mhi/host/pci_generic.c
>>>>> +++ b/drivers/bus/mhi/host/pci_generic.c
>>>>> @@ -53,6 +53,7 @@ struct mhi_pci_dev_info {
>>>>>            unsigned int dma_data_width;
>>>>>            unsigned int mru_default;
>>>>>            bool sideband_wake;
>>>>> + unsigned int mux_id;
>>>>>    };
>>>>>    #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
>>>>> @@ -469,6 +470,7 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx72_info = {
>>>>>            .dma_data_width = 32,
>>>>>            .mru_default = 32768,
>>>>>            .sideband_wake = false,
>>>>> + .mux_id = 112,
>>>>>    };
>>>>>    static const struct mhi_channel_config mhi_mv3x_channels[] = {
>>>>> @@ -1035,6 +1037,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>>>>            mhi_cntrl->runtime_get = mhi_pci_runtime_get;
>>>>>            mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>>>>>            mhi_cntrl->mru = info->mru_default;
>>>>> + mhi_cntrl->link_id = info->mux_id;
>>>>
>>>> Again, 'link_id' is just a WWAN term. Use 'mux_id' here also.
>>>
>>> Does this really belong in MHI?  If this was DT, I don't think we would put
>>> this value in DT, but rather have the driver (MBIM) detect the device and
>>> code in the required value.
>>>
>>
>> I believe this is a modem value rather than MHI. But I was OK with keeping it in
>> MHI driver since we kind of keep modem specific config.
>>
>> But if WWAN can detect the device and apply the config, I'm all over it.
> 
> That would require at least some information from the MHI bus for the
> MBIM driver
> to make a decision, such as a generic device ID, or quirk flags...

I don't see why.

The "simple" way to do it would be to have the controller define a 
different channel name, and then have the MBIM driver probe on that. 
The MBIM driver could attach driver data saying that it needs to have a 
specific mux_id.

Or, with zero MHI/Controller changes, the MBIM driver could parse the 
mhi_device struct, get to the struct device, for the underlying device, 
and extract the PCIe Device ID and match that to a white list of known 
devices that need this property.

I guess if the controller could attach a private void * to the 
mhi_device that is opaque to MHI, but allows MBIM to make a decision, 
that would be ok.  Such a mechanism would be generic, and extensible to 
other usecases of the same "class".

-Jeff

Re:Re: [PATCH v2 1/2] bus: mhi: host: Import mux_id item

[Slark Xiao]

At 2024-06-14 22:31:03, "Jeffrey Hugo" <quic_jhugo@quicinc.com> wrote:
>On 6/14/2024 4:17 AM, Loic Poulain wrote:
>> On Wed, 12 Jun 2024 at 16:51, Manivannan Sadhasivam
>> <manivannan.sadhasivam@linaro.org> wrote:
>>>
>>> On Wed, Jun 12, 2024 at 08:19:13AM -0600, Jeffrey Hugo wrote:
>>>> On 6/12/2024 3:46 AM, Manivannan Sadhasivam wrote:
>>>>> On Wed, Jun 12, 2024 at 05:38:42PM +0800, Slark Xiao wrote:
>>>>>
>>>>> Subject could be improved:
>>>>>
>>>>> bus: mhi: host: Add configurable mux_id for MBIM mode
>>>>>
>>>>>> For SDX72 MBIM mode, it starts data mux id from 112 instead of 0.
>>>>>> This would lead to device can't ping outside successfully.
>>>>>> Also MBIM side would report "bad packet session (112)".
>>>>>> So we add a default mux_id value for SDX72. And this value
>>>>>> would be transferred to wwan mbim side.
>>>>>>
>>>>>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
>>>>>> ---
>>>>>>    drivers/bus/mhi/host/pci_generic.c | 3 +++
>>>>>>    include/linux/mhi.h                | 2 ++
>>>>>>    2 files changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
>>>>>> index 0b483c7c76a1..9e9adf8320d2 100644
>>>>>> --- a/drivers/bus/mhi/host/pci_generic.c
>>>>>> +++ b/drivers/bus/mhi/host/pci_generic.c
>>>>>> @@ -53,6 +53,7 @@ struct mhi_pci_dev_info {
>>>>>>            unsigned int dma_data_width;
>>>>>>            unsigned int mru_default;
>>>>>>            bool sideband_wake;
>>>>>> + unsigned int mux_id;
>>>>>>    };
>>>>>>    #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
>>>>>> @@ -469,6 +470,7 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx72_info = {
>>>>>>            .dma_data_width = 32,
>>>>>>            .mru_default = 32768,
>>>>>>            .sideband_wake = false,
>>>>>> + .mux_id = 112,
>>>>>>    };
>>>>>>    static const struct mhi_channel_config mhi_mv3x_channels[] = {
>>>>>> @@ -1035,6 +1037,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>>>>>            mhi_cntrl->runtime_get = mhi_pci_runtime_get;
>>>>>>            mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>>>>>>            mhi_cntrl->mru = info->mru_default;
>>>>>> + mhi_cntrl->link_id = info->mux_id;
>>>>>
>>>>> Again, 'link_id' is just a WWAN term. Use 'mux_id' here also.
>>>>
>>>> Does this really belong in MHI?  If this was DT, I don't think we would put
>>>> this value in DT, but rather have the driver (MBIM) detect the device and
>>>> code in the required value.
>>>>
>>>
>>> I believe this is a modem value rather than MHI. But I was OK with keeping it in
>>> MHI driver since we kind of keep modem specific config.
>>>
>>> But if WWAN can detect the device and apply the config, I'm all over it.
>> 
>> That would require at least some information from the MHI bus for the
>> MBIM driver
>> to make a decision, such as a generic device ID, or quirk flags...
>
>I don't see why.
>
>The "simple" way to do it would be to have the controller define a 
>different channel name, and then have the MBIM driver probe on that. 
>The MBIM driver could attach driver data saying that it needs to have a 
>specific mux_id.
>
>Or, with zero MHI/Controller changes, the MBIM driver could parse the 
>mhi_device struct, get to the struct device, for the underlying device, 
>and extract the PCIe Device ID and match that to a white list of known 
>devices that need this property.
>
>I guess if the controller could attach a private void * to the 
>mhi_device that is opaque to MHI, but allows MBIM to make a decision, 
>that would be ok.  Such a mechanism would be generic, and extensible to 
>other usecases of the same "class".
>
>-Jeff

Hi guys,
This patch mainly refer to the feature of mru setting between mhi and wwan side.
We ransfer this value to wwan side if we define it in mhi side, otherwise a default
value would be used in wwan side. Why don't we just align with that?

Thanks

Re: Re: [PATCH v2 1/2] bus: mhi: host: Import mux_id item

[Manivannan Sadhasivam]

On Fri, Jun 21, 2024 at 11:17:16AM +0800, Slark Xiao wrote:
> 
> At 2024-06-14 22:31:03, "Jeffrey Hugo" <quic_jhugo@quicinc.com> wrote:
> >On 6/14/2024 4:17 AM, Loic Poulain wrote:
> >> On Wed, 12 Jun 2024 at 16:51, Manivannan Sadhasivam
> >> <manivannan.sadhasivam@linaro.org> wrote:
> >>>
> >>> On Wed, Jun 12, 2024 at 08:19:13AM -0600, Jeffrey Hugo wrote:
> >>>> On 6/12/2024 3:46 AM, Manivannan Sadhasivam wrote:
> >>>>> On Wed, Jun 12, 2024 at 05:38:42PM +0800, Slark Xiao wrote:
> >>>>>
> >>>>> Subject could be improved:
> >>>>>
> >>>>> bus: mhi: host: Add configurable mux_id for MBIM mode
> >>>>>
> >>>>>> For SDX72 MBIM mode, it starts data mux id from 112 instead of 0.
> >>>>>> This would lead to device can't ping outside successfully.
> >>>>>> Also MBIM side would report "bad packet session (112)".
> >>>>>> So we add a default mux_id value for SDX72. And this value
> >>>>>> would be transferred to wwan mbim side.
> >>>>>>
> >>>>>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
> >>>>>> ---
> >>>>>>    drivers/bus/mhi/host/pci_generic.c | 3 +++
> >>>>>>    include/linux/mhi.h                | 2 ++
> >>>>>>    2 files changed, 5 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> >>>>>> index 0b483c7c76a1..9e9adf8320d2 100644
> >>>>>> --- a/drivers/bus/mhi/host/pci_generic.c
> >>>>>> +++ b/drivers/bus/mhi/host/pci_generic.c
> >>>>>> @@ -53,6 +53,7 @@ struct mhi_pci_dev_info {
> >>>>>>            unsigned int dma_data_width;
> >>>>>>            unsigned int mru_default;
> >>>>>>            bool sideband_wake;
> >>>>>> + unsigned int mux_id;
> >>>>>>    };
> >>>>>>    #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
> >>>>>> @@ -469,6 +470,7 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx72_info = {
> >>>>>>            .dma_data_width = 32,
> >>>>>>            .mru_default = 32768,
> >>>>>>            .sideband_wake = false,
> >>>>>> + .mux_id = 112,
> >>>>>>    };
> >>>>>>    static const struct mhi_channel_config mhi_mv3x_channels[] = {
> >>>>>> @@ -1035,6 +1037,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >>>>>>            mhi_cntrl->runtime_get = mhi_pci_runtime_get;
> >>>>>>            mhi_cntrl->runtime_put = mhi_pci_runtime_put;
> >>>>>>            mhi_cntrl->mru = info->mru_default;
> >>>>>> + mhi_cntrl->link_id = info->mux_id;
> >>>>>
> >>>>> Again, 'link_id' is just a WWAN term. Use 'mux_id' here also.
> >>>>
> >>>> Does this really belong in MHI?  If this was DT, I don't think we would put
> >>>> this value in DT, but rather have the driver (MBIM) detect the device and
> >>>> code in the required value.
> >>>>
> >>>
> >>> I believe this is a modem value rather than MHI. But I was OK with keeping it in
> >>> MHI driver since we kind of keep modem specific config.
> >>>
> >>> But if WWAN can detect the device and apply the config, I'm all over it.
> >> 
> >> That would require at least some information from the MHI bus for the
> >> MBIM driver
> >> to make a decision, such as a generic device ID, or quirk flags...
> >
> >I don't see why.
> >
> >The "simple" way to do it would be to have the controller define a 
> >different channel name, and then have the MBIM driver probe on that. 
> >The MBIM driver could attach driver data saying that it needs to have a 
> >specific mux_id.
> >
> >Or, with zero MHI/Controller changes, the MBIM driver could parse the 
> >mhi_device struct, get to the struct device, for the underlying device, 
> >and extract the PCIe Device ID and match that to a white list of known 
> >devices that need this property.
> >
> >I guess if the controller could attach a private void * to the 
> >mhi_device that is opaque to MHI, but allows MBIM to make a decision, 
> >that would be ok.  Such a mechanism would be generic, and extensible to 
> >other usecases of the same "class".
> >
> >-Jeff
> 
> Hi guys,
> This patch mainly refer to the feature of mru setting between mhi and wwan side.
> We ransfer this value to wwan side if we define it in mhi side, otherwise a default
> value would be used in wwan side. Why don't we just align with that?
> 

Well, the problem is that MRU has nothing to do with MHI. I initially thought
that it could fit inside the controller config, but thinking more I agree with
Jeff that this doesn't belong to MHI at all.

At the same time, I also do not want to extract the PCI info from the client
drivers since the underlying transport could change with MHI. So the best
solution I can think of is exposing the modem name in 'mhi_controller_config' so
that the client drivers can do a match.

Please try to implement that.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re:Re: Re: [PATCH v2 1/2] bus: mhi: host: Import mux_id item

[Slark Xiao]

At 2024-06-23 21:44:30, "Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org> wrote:
>On Fri, Jun 21, 2024 at 11:17:16AM +0800, Slark Xiao wrote:
>> 
>> At 2024-06-14 22:31:03, "Jeffrey Hugo" <quic_jhugo@quicinc.com> wrote:
>> >On 6/14/2024 4:17 AM, Loic Poulain wrote:
>> >> On Wed, 12 Jun 2024 at 16:51, Manivannan Sadhasivam
>> >> <manivannan.sadhasivam@linaro.org> wrote:
>> >>>
>> >>> On Wed, Jun 12, 2024 at 08:19:13AM -0600, Jeffrey Hugo wrote:
>> >>>> On 6/12/2024 3:46 AM, Manivannan Sadhasivam wrote:
>> >>>>> On Wed, Jun 12, 2024 at 05:38:42PM +0800, Slark Xiao wrote:
>> >>>>>
>> >>>>> Subject could be improved:
>> >>>>>
>> >>>>> bus: mhi: host: Add configurable mux_id for MBIM mode
>> >>>>>
>> >>>>>> For SDX72 MBIM mode, it starts data mux id from 112 instead of 0.
>> >>>>>> This would lead to device can't ping outside successfully.
>> >>>>>> Also MBIM side would report "bad packet session (112)".
>> >>>>>> So we add a default mux_id value for SDX72. And this value
>> >>>>>> would be transferred to wwan mbim side.
>> >>>>>>
>> >>>>>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
>> >>>>>> ---
>> >>>>>>    drivers/bus/mhi/host/pci_generic.c | 3 +++
>> >>>>>>    include/linux/mhi.h                | 2 ++
>> >>>>>>    2 files changed, 5 insertions(+)
>> >>>>>>
>> >>>>>> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
>> >>>>>> index 0b483c7c76a1..9e9adf8320d2 100644
>> >>>>>> --- a/drivers/bus/mhi/host/pci_generic.c
>> >>>>>> +++ b/drivers/bus/mhi/host/pci_generic.c
>> >>>>>> @@ -53,6 +53,7 @@ struct mhi_pci_dev_info {
>> >>>>>>            unsigned int dma_data_width;
>> >>>>>>            unsigned int mru_default;
>> >>>>>>            bool sideband_wake;
>> >>>>>> + unsigned int mux_id;
>> >>>>>>    };
>> >>>>>>    #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
>> >>>>>> @@ -469,6 +470,7 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx72_info = {
>> >>>>>>            .dma_data_width = 32,
>> >>>>>>            .mru_default = 32768,
>> >>>>>>            .sideband_wake = false,
>> >>>>>> + .mux_id = 112,
>> >>>>>>    };
>> >>>>>>    static const struct mhi_channel_config mhi_mv3x_channels[] = {
>> >>>>>> @@ -1035,6 +1037,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> >>>>>>            mhi_cntrl->runtime_get = mhi_pci_runtime_get;
>> >>>>>>            mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>> >>>>>>            mhi_cntrl->mru = info->mru_default;
>> >>>>>> + mhi_cntrl->link_id = info->mux_id;
>> >>>>>
>> >>>>> Again, 'link_id' is just a WWAN term. Use 'mux_id' here also.
>> >>>>
>> >>>> Does this really belong in MHI?  If this was DT, I don't think we would put
>> >>>> this value in DT, but rather have the driver (MBIM) detect the device and
>> >>>> code in the required value.
>> >>>>
>> >>>
>> >>> I believe this is a modem value rather than MHI. But I was OK with keeping it in
>> >>> MHI driver since we kind of keep modem specific config.
>> >>>
>> >>> But if WWAN can detect the device and apply the config, I'm all over it.
>> >> 
>> >> That would require at least some information from the MHI bus for the
>> >> MBIM driver
>> >> to make a decision, such as a generic device ID, or quirk flags...
>> >
>> >I don't see why.
>> >
>> >The "simple" way to do it would be to have the controller define a 
>> >different channel name, and then have the MBIM driver probe on that. 
>> >The MBIM driver could attach driver data saying that it needs to have a 
>> >specific mux_id.
>> >
>> >Or, with zero MHI/Controller changes, the MBIM driver could parse the 
>> >mhi_device struct, get to the struct device, for the underlying device, 
>> >and extract the PCIe Device ID and match that to a white list of known 
>> >devices that need this property.
>> >
>> >I guess if the controller could attach a private void * to the 
>> >mhi_device that is opaque to MHI, but allows MBIM to make a decision, 
>> >that would be ok.  Such a mechanism would be generic, and extensible to 
>> >other usecases of the same "class".
>> >
>> >-Jeff
>> 
>> Hi guys,
>> This patch mainly refer to the feature of mru setting between mhi and wwan side.
>> We ransfer this value to wwan side if we define it in mhi side, otherwise a default
>> value would be used in wwan side. Why don't we just align with that?
>> 
>
>Well, the problem is that MRU has nothing to do with MHI. I initially thought
>that it could fit inside the controller config, but thinking more I agree with
>Jeff that this doesn't belong to MHI at all.
>
>At the same time, I also do not want to extract the PCI info from the client
>drivers since the underlying transport could change with MHI. So the best
>solution I can think of is exposing the modem name in 'mhi_controller_config' so
>that the client drivers can do a match.
>
>Please try to implement that.
>
>- Mani
>
>-- 
>மணிவண்ணன் சதாசிவம்
Hi Mani,
Currently there are many products share a same mhi_controller_config
settings. For example, all foxconn device use modem_foxconn_sdx55_config.
But my device may be a SDX24, or SDX72, or even SDX65.  Any other idea?

Thanks

Re: Re: Re: [PATCH v2 1/2] bus: mhi: host: Import mux_id item

[Manivannan Sadhasivam]

On Tue, Jun 25, 2024 at 10:10:17AM +0800, Slark Xiao wrote:
> At 2024-06-23 21:44:30, "Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org> wrote:
> >On Fri, Jun 21, 2024 at 11:17:16AM +0800, Slark Xiao wrote:
> >> 
> >> At 2024-06-14 22:31:03, "Jeffrey Hugo" <quic_jhugo@quicinc.com> wrote:
> >> >On 6/14/2024 4:17 AM, Loic Poulain wrote:
> >> >> On Wed, 12 Jun 2024 at 16:51, Manivannan Sadhasivam
> >> >> <manivannan.sadhasivam@linaro.org> wrote:
> >> >>>
> >> >>> On Wed, Jun 12, 2024 at 08:19:13AM -0600, Jeffrey Hugo wrote:
> >> >>>> On 6/12/2024 3:46 AM, Manivannan Sadhasivam wrote:
> >> >>>>> On Wed, Jun 12, 2024 at 05:38:42PM +0800, Slark Xiao wrote:
> >> >>>>>
> >> >>>>> Subject could be improved:
> >> >>>>>
> >> >>>>> bus: mhi: host: Add configurable mux_id for MBIM mode
> >> >>>>>
> >> >>>>>> For SDX72 MBIM mode, it starts data mux id from 112 instead of 0.
> >> >>>>>> This would lead to device can't ping outside successfully.
> >> >>>>>> Also MBIM side would report "bad packet session (112)".
> >> >>>>>> So we add a default mux_id value for SDX72. And this value
> >> >>>>>> would be transferred to wwan mbim side.
> >> >>>>>>
> >> >>>>>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
> >> >>>>>> ---
> >> >>>>>>    drivers/bus/mhi/host/pci_generic.c | 3 +++
> >> >>>>>>    include/linux/mhi.h                | 2 ++
> >> >>>>>>    2 files changed, 5 insertions(+)
> >> >>>>>>
> >> >>>>>> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> >> >>>>>> index 0b483c7c76a1..9e9adf8320d2 100644
> >> >>>>>> --- a/drivers/bus/mhi/host/pci_generic.c
> >> >>>>>> +++ b/drivers/bus/mhi/host/pci_generic.c
> >> >>>>>> @@ -53,6 +53,7 @@ struct mhi_pci_dev_info {
> >> >>>>>>            unsigned int dma_data_width;
> >> >>>>>>            unsigned int mru_default;
> >> >>>>>>            bool sideband_wake;
> >> >>>>>> + unsigned int mux_id;
> >> >>>>>>    };
> >> >>>>>>    #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
> >> >>>>>> @@ -469,6 +470,7 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx72_info = {
> >> >>>>>>            .dma_data_width = 32,
> >> >>>>>>            .mru_default = 32768,
> >> >>>>>>            .sideband_wake = false,
> >> >>>>>> + .mux_id = 112,
> >> >>>>>>    };
> >> >>>>>>    static const struct mhi_channel_config mhi_mv3x_channels[] = {
> >> >>>>>> @@ -1035,6 +1037,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >> >>>>>>            mhi_cntrl->runtime_get = mhi_pci_runtime_get;
> >> >>>>>>            mhi_cntrl->runtime_put = mhi_pci_runtime_put;
> >> >>>>>>            mhi_cntrl->mru = info->mru_default;
> >> >>>>>> + mhi_cntrl->link_id = info->mux_id;
> >> >>>>>
> >> >>>>> Again, 'link_id' is just a WWAN term. Use 'mux_id' here also.
> >> >>>>
> >> >>>> Does this really belong in MHI?  If this was DT, I don't think we would put
> >> >>>> this value in DT, but rather have the driver (MBIM) detect the device and
> >> >>>> code in the required value.
> >> >>>>
> >> >>>
> >> >>> I believe this is a modem value rather than MHI. But I was OK with keeping it in
> >> >>> MHI driver since we kind of keep modem specific config.
> >> >>>
> >> >>> But if WWAN can detect the device and apply the config, I'm all over it.
> >> >> 
> >> >> That would require at least some information from the MHI bus for the
> >> >> MBIM driver
> >> >> to make a decision, such as a generic device ID, or quirk flags...
> >> >
> >> >I don't see why.
> >> >
> >> >The "simple" way to do it would be to have the controller define a 
> >> >different channel name, and then have the MBIM driver probe on that. 
> >> >The MBIM driver could attach driver data saying that it needs to have a 
> >> >specific mux_id.
> >> >
> >> >Or, with zero MHI/Controller changes, the MBIM driver could parse the 
> >> >mhi_device struct, get to the struct device, for the underlying device, 
> >> >and extract the PCIe Device ID and match that to a white list of known 
> >> >devices that need this property.
> >> >
> >> >I guess if the controller could attach a private void * to the 
> >> >mhi_device that is opaque to MHI, but allows MBIM to make a decision, 
> >> >that would be ok.  Such a mechanism would be generic, and extensible to 
> >> >other usecases of the same "class".
> >> >
> >> >-Jeff
> >> 
> >> Hi guys,
> >> This patch mainly refer to the feature of mru setting between mhi and wwan side.
> >> We ransfer this value to wwan side if we define it in mhi side, otherwise a default
> >> value would be used in wwan side. Why don't we just align with that?
> >> 
> >
> >Well, the problem is that MRU has nothing to do with MHI. I initially thought
> >that it could fit inside the controller config, but thinking more I agree with
> >Jeff that this doesn't belong to MHI at all.
> >
> >At the same time, I also do not want to extract the PCI info from the client
> >drivers since the underlying transport could change with MHI. So the best
> >solution I can think of is exposing the modem name in 'mhi_controller_config' so
> >that the client drivers can do a match.
> >
> >Please try to implement that.
> >
> >- Mani
> >
> >-- 
> >மணிவண்ணன் சதாசிவம்
> Hi Mani,
> Currently there are many products share a same mhi_controller_config
> settings. For example, all foxconn device use modem_foxconn_sdx55_config.
> But my device may be a SDX24, or SDX72, or even SDX65.  Any other idea?
> 

Hmm, sadly we shouldn't have used the same controller config for all these
devices across different product families. I didn't really paid attention to the
device name which is supposed to be unique (that's my bad).

For instance, because of the controller config reuse, your SDX62 modem would
print:

"MHI PCI device found: foxconn-sdx65"

which clearly is misleading the users...

I've submitted a patch that uses unique product name across the product families
[1]. Please take a look. After this patch, you can use the modem name to
differentiate in client drivers.

- Mani

[1] https://lore.kernel.org/mhi/20240625074148.7412-1-manivannan.sadhasivam@linaro.org/

-- 
மணிவண்ணன் சதாசிவம்

Re:Re: Re: Re: [PATCH v2 1/2] bus: mhi: host: Import mux_id item

[Slark Xiao]

At 2024-06-25 15:44:49, "Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org> wrote:
>On Tue, Jun 25, 2024 at 10:10:17AM +0800, Slark Xiao wrote:
>> At 2024-06-23 21:44:30, "Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org> wrote:
>> >On Fri, Jun 21, 2024 at 11:17:16AM +0800, Slark Xiao wrote:
>> >> 
>> >> At 2024-06-14 22:31:03, "Jeffrey Hugo" <quic_jhugo@quicinc.com> wrote:
>> >> >On 6/14/2024 4:17 AM, Loic Poulain wrote:
>> >> >> On Wed, 12 Jun 2024 at 16:51, Manivannan Sadhasivam
>> >> >> <manivannan.sadhasivam@linaro.org> wrote:
>> >> >>>
>> >> >>> On Wed, Jun 12, 2024 at 08:19:13AM -0600, Jeffrey Hugo wrote:
>> >> >>>> On 6/12/2024 3:46 AM, Manivannan Sadhasivam wrote:
>> >> >>>>> On Wed, Jun 12, 2024 at 05:38:42PM +0800, Slark Xiao wrote:
>> >> >>>>>
>> >> >>>>> Subject could be improved:
>> >> >>>>>
>> >> >>>>> bus: mhi: host: Add configurable mux_id for MBIM mode
>> >> >>>>>
>> >> >>>>>> For SDX72 MBIM mode, it starts data mux id from 112 instead of 0.
>> >> >>>>>> This would lead to device can't ping outside successfully.
>> >> >>>>>> Also MBIM side would report "bad packet session (112)".
>> >> >>>>>> So we add a default mux_id value for SDX72. And this value
>> >> >>>>>> would be transferred to wwan mbim side.
>> >> >>>>>>
>> >> >>>>>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
>> >> >>>>>> ---
>> >> >>>>>>    drivers/bus/mhi/host/pci_generic.c | 3 +++
>> >> >>>>>>    include/linux/mhi.h                | 2 ++
>> >> >>>>>>    2 files changed, 5 insertions(+)
>> >> >>>>>>
>> >> >>>>>> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
>> >> >>>>>> index 0b483c7c76a1..9e9adf8320d2 100644
>> >> >>>>>> --- a/drivers/bus/mhi/host/pci_generic.c
>> >> >>>>>> +++ b/drivers/bus/mhi/host/pci_generic.c
>> >> >>>>>> @@ -53,6 +53,7 @@ struct mhi_pci_dev_info {
>> >> >>>>>>            unsigned int dma_data_width;
>> >> >>>>>>            unsigned int mru_default;
>> >> >>>>>>            bool sideband_wake;
>> >> >>>>>> + unsigned int mux_id;
>> >> >>>>>>    };
>> >> >>>>>>    #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
>> >> >>>>>> @@ -469,6 +470,7 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx72_info = {
>> >> >>>>>>            .dma_data_width = 32,
>> >> >>>>>>            .mru_default = 32768,
>> >> >>>>>>            .sideband_wake = false,
>> >> >>>>>> + .mux_id = 112,
>> >> >>>>>>    };
>> >> >>>>>>    static const struct mhi_channel_config mhi_mv3x_channels[] = {
>> >> >>>>>> @@ -1035,6 +1037,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> >> >>>>>>            mhi_cntrl->runtime_get = mhi_pci_runtime_get;
>> >> >>>>>>            mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>> >> >>>>>>            mhi_cntrl->mru = info->mru_default;
>> >> >>>>>> + mhi_cntrl->link_id = info->mux_id;
>> >> >>>>>
>> >> >>>>> Again, 'link_id' is just a WWAN term. Use 'mux_id' here also.
>> >> >>>>
>> >> >>>> Does this really belong in MHI?  If this was DT, I don't think we would put
>> >> >>>> this value in DT, but rather have the driver (MBIM) detect the device and
>> >> >>>> code in the required value.
>> >> >>>>
>> >> >>>
>> >> >>> I believe this is a modem value rather than MHI. But I was OK with keeping it in
>> >> >>> MHI driver since we kind of keep modem specific config.
>> >> >>>
>> >> >>> But if WWAN can detect the device and apply the config, I'm all over it.
>> >> >> 
>> >> >> That would require at least some information from the MHI bus for the
>> >> >> MBIM driver
>> >> >> to make a decision, such as a generic device ID, or quirk flags...
>> >> >
>> >> >I don't see why.
>> >> >
>> >> >The "simple" way to do it would be to have the controller define a 
>> >> >different channel name, and then have the MBIM driver probe on that. 
>> >> >The MBIM driver could attach driver data saying that it needs to have a 
>> >> >specific mux_id.
>> >> >
>> >> >Or, with zero MHI/Controller changes, the MBIM driver could parse the 
>> >> >mhi_device struct, get to the struct device, for the underlying device, 
>> >> >and extract the PCIe Device ID and match that to a white list of known 
>> >> >devices that need this property.
>> >> >
>> >> >I guess if the controller could attach a private void * to the 
>> >> >mhi_device that is opaque to MHI, but allows MBIM to make a decision, 
>> >> >that would be ok.  Such a mechanism would be generic, and extensible to 
>> >> >other usecases of the same "class".
>> >> >
>> >> >-Jeff
>> >> 
>> >> Hi guys,
>> >> This patch mainly refer to the feature of mru setting between mhi and wwan side.
>> >> We ransfer this value to wwan side if we define it in mhi side, otherwise a default
>> >> value would be used in wwan side. Why don't we just align with that?
>> >> 
>> >
>> >Well, the problem is that MRU has nothing to do with MHI. I initially thought
>> >that it could fit inside the controller config, but thinking more I agree with
>> >Jeff that this doesn't belong to MHI at all.
>> >
>> >At the same time, I also do not want to extract the PCI info from the client
>> >drivers since the underlying transport could change with MHI. So the best
>> >solution I can think of is exposing the modem name in 'mhi_controller_config' so
>> >that the client drivers can do a match.
>> >
>> >Please try to implement that.
>> >
>> >- Mani
>> >
>> >-- 
>> >மணிவண்ணன் சதாசிவம்
>> Hi Mani,
>> Currently there are many products share a same mhi_controller_config
>> settings. For example, all foxconn device use modem_foxconn_sdx55_config.
>> But my device may be a SDX24, or SDX72, or even SDX65.  Any other idea?
>> 
>
>Hmm, sadly we shouldn't have used the same controller config for all these
>devices across different product families. I didn't really paid attention to the
>device name which is supposed to be unique (that's my bad).
>
>For instance, because of the controller config reuse, your SDX62 modem would
>print:
>
>"MHI PCI device found: foxconn-sdx65"
>
>which clearly is misleading the users...
>
>I've submitted a patch that uses unique product name across the product families
>[1]. Please take a look. After this patch, you can use the modem name to
>differentiate in client drivers.
>
>- Mani
>
>[1] https://lore.kernel.org/mhi/20240625074148.7412-1-manivannan.sadhasivam@linaro.org/
>
>-- 
>மணிவண்ணன் சதாசிவம்
For same chip platform, I don't think it's necessary to separate into different parts.
Like t99w368 and DW5932e, all things are same except the 'name'. For previous
mux_id settings, we would like to add it for sdx72/sdx75 platforms, but shall
no difference on T99W515 and DW5934e. 
Otherwise, we must to update both mhi and wwan side if we have a new foxconn
SDX72 device support since the name is different with foxconn-t99w515 or
foxconn-dw5934e.

Thanks.

Re: Re: Re: Re: [PATCH v2 1/2] bus: mhi: host: Import mux_id item

[Manivannan Sadhasivam]

On Tue, Jun 25, 2024 at 04:28:25PM +0800, Slark Xiao wrote:

[...]

> >Hmm, sadly we shouldn't have used the same controller config for all these
> >devices across different product families. I didn't really paid attention to the
> >device name which is supposed to be unique (that's my bad).
> >
> >For instance, because of the controller config reuse, your SDX62 modem would
> >print:
> >
> >"MHI PCI device found: foxconn-sdx65"
> >
> >which clearly is misleading the users...
> >
> >I've submitted a patch that uses unique product name across the product families
> >[1]. Please take a look. After this patch, you can use the modem name to
> >differentiate in client drivers.
> >
> >- Mani
> >
> >[1] https://lore.kernel.org/mhi/20240625074148.7412-1-manivannan.sadhasivam@linaro.org/
> >
> >-- 
> >மணிவண்ணன் சதாசிவம்
> For same chip platform, I don't think it's necessary to separate into different parts.
> Like t99w368 and DW5932e, all things are same except the 'name'. For previous
> mux_id settings, we would like to add it for sdx72/sdx75 platforms, but shall
> no difference on T99W515 and DW5934e. 
> Otherwise, we must to update both mhi and wwan side if we have a new foxconn
> SDX72 device support since the name is different with foxconn-t99w515 or
> foxconn-dw5934e.
> 

Name is an important factor for an end user. Because, even though both products
are same in functionality, they are marketed as different products. So the users
should be provided with the actual product name, not baseline.

Even though it requires an update to the pci_generic driver, it ought to happen
for correctness.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 1/2] bus: mhi: host: Import mux_id item

[kernel test robot]

Hi Slark,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mani-mhi/mhi-next]
[also build test WARNING on linus/master v6.10-rc3 next-20240612]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Slark-Xiao/net-wwan-mhi-make-default-data-link-id-configurable/20240612-174242
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mani/mhi.git mhi-next
patch link:    https://lore.kernel.org/r/20240612093842.359805-1-slark_xiao%40163.com
patch subject: [PATCH v2 1/2] bus: mhi: host: Import mux_id item
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20240613/202406130220.gq15Sjzk-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240613/202406130220.gq15Sjzk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406130220.gq15Sjzk-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/bus/mhi/host/pci_generic.c:57: warning: Function parameter or struct member 'mux_id' not described in 'mhi_pci_dev_info'


vim +57 drivers/bus/mhi/host/pci_generic.c

48f98496b1de132f drivers/bus/mhi/host/pci_generic.c Qiang Yu      2024-04-24  32  
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  33  /**
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  34   * struct mhi_pci_dev_info - MHI PCI device specific information
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  35   * @config: MHI controller configuration
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  36   * @name: name of the PCI module
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  37   * @fw: firmware path (if any)
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  38   * @edl: emergency download mode firmware path (if any)
48f98496b1de132f drivers/bus/mhi/host/pci_generic.c Qiang Yu      2024-04-24  39   * @edl_trigger: capable of triggering EDL mode in the device (if supported)
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  40   * @bar_num: PCI base address register to use for MHI MMIO register space
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  41   * @dma_data_width: DMA transfer word size (32 or 64 bits)
5c2c85315948c42c drivers/bus/mhi/pci_generic.c      Richard Laing 2021-07-15  42   * @mru_default: default MRU size for MBIM network packets
56f6f4c4eb2a710e drivers/bus/mhi/pci_generic.c      Bhaumik Bhatt 2021-07-16  43   * @sideband_wake: Devices using dedicated sideband GPIO for wakeup instead
56f6f4c4eb2a710e drivers/bus/mhi/pci_generic.c      Bhaumik Bhatt 2021-07-16  44   *		   of inband wake support (such as sdx24)
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  45   */
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  46  struct mhi_pci_dev_info {
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  47  	const struct mhi_controller_config *config;
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  48  	const char *name;
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  49  	const char *fw;
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  50  	const char *edl;
48f98496b1de132f drivers/bus/mhi/host/pci_generic.c Qiang Yu      2024-04-24  51  	bool edl_trigger;
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  52  	unsigned int bar_num;
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  53  	unsigned int dma_data_width;
5c2c85315948c42c drivers/bus/mhi/pci_generic.c      Richard Laing 2021-07-15  54  	unsigned int mru_default;
56f6f4c4eb2a710e drivers/bus/mhi/pci_generic.c      Bhaumik Bhatt 2021-07-16  55  	bool sideband_wake;
2b153f167f41516b drivers/bus/mhi/host/pci_generic.c Slark Xiao    2024-06-12  56  	unsigned int mux_id;
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21 @57  };
855a70c12021bdc5 drivers/bus/mhi/pci_generic.c      Loic Poulain  2020-10-21  58  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki