| Message ID | 20240612093842.359805-1-slark_xiao@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] bus: mhi: host: Import mux_id item | expand |
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
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
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; }; /**
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(+)