| Message ID | 20250105124819.6950-1-zaihan@unrealasia.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v4] wwan dev: Add port for NMEA channel for WWAN devices | expand |
Hi Muhammad, + Slark On Sun, 5 Jan 2025 at 13:53, Muhammad Nuzaihan <zaihan@unrealasia.net> wrote: > > Based on the code: drivers/bus/mhi/host/pci_generic.c > which already has NMEA channel (mhi_quectel_em1xx_channels) > support in recent kernels but it is never exposed > as a port. > > This commit exposes that NMEA channel to a port > to allow tty/gpsd programs to read through > the /dev/wwan0nmea0 port. > > Tested this change on a new kernel and module > built and now NMEA (mhi0_NMEA) statements are > available (attached) through /dev/wwan0nmea0 port on bootup. > > Signed-off-by: Muhammad Nuzaihan Bin Kamal Luddin <zaihan@unrealasia.net> This works for sure but I'm not entirely convinced NMEA should be exposed as a modem control port. In your previous patch version Sergey pointed to a discussion we had regarding exposing that port as WWAN child device through the regular GNSS subsystem, which would require some generic bridge in the WWAN subsystem. Slark, did you have an opportunity to look at the GNSS solution? Regards, Loic
At 2025-01-07 03:44:35, "Loic Poulain" <loic.poulain@linaro.org> wrote: >Hi Muhammad, > >+ Slark > >On Sun, 5 Jan 2025 at 13:53, Muhammad Nuzaihan <zaihan@unrealasia.net> wrote: >> >> Based on the code: drivers/bus/mhi/host/pci_generic.c >> which already has NMEA channel (mhi_quectel_em1xx_channels) >> support in recent kernels but it is never exposed >> as a port. >> >> This commit exposes that NMEA channel to a port >> to allow tty/gpsd programs to read through >> the /dev/wwan0nmea0 port. >> >> Tested this change on a new kernel and module >> built and now NMEA (mhi0_NMEA) statements are >> available (attached) through /dev/wwan0nmea0 port on bootup. >> >> Signed-off-by: Muhammad Nuzaihan Bin Kamal Luddin <zaihan@unrealasia.net> > >This works for sure but I'm not entirely convinced NMEA should be >exposed as a modem control port. In your previous patch version Sergey >pointed to a discussion we had regarding exposing that port as WWAN >child device through the regular GNSS subsystem, which would require >some generic bridge in the WWAN subsystem. > >Slark, did you have an opportunity to look at the GNSS solution? > >Regards, >Loic Hi Loic, This solution same as what I did in last time. We got a wwan0nmea0 device but this device can't support flow control. Also, this is not the solution what Sergey expected, I think. Please refer to the target we talked last time: ///////////////////// >>> Basically, components should interact like this: >>> >>> Modem PCIe driver <-> WWAN core <-> GNSS core <-> /dev/gnss0 >>> >>> We need the GNSS core to export the modem NMEA port as instance of >>> 'gnss' class. >>> >>> We need WWAN core between the modem driver and the GSNN core because we >>> need a common parent for GNSS port and modem control port (e.g. AT, >>> MBIM). Since we are already exporting control ports via the WWAN >>> subsystem, the GNSS port should also be exported through the WWAN >>> subsystem. To keep devices hierarchy like this: >>> >>> .--> AT port >>> PCIe dev -> WWAN dev -| >>> '--> GNSS port >>> >>> Back to the implementation. Probably we should introduce a new port >>> type, e.g. WWAN_PORT_NMEA. And this port type should have a special >>> handling in the WWAN core. >>> >> Similar like what I did in my local. I named it as WWAN_PORT_GNSS and >> put it as same level with AT port. >> >>> wwan_create_port() function should not directly create a char device. >>> Instead, it should call gnss_allocate_device() and >>> gnss_register_device() to register the port with GNSS subsystem. //////////////////// Thanks
Hi Everyone, Sorry for the last (HTML) email. Seemed i forgot Gmail sends HTML email by default and Geary client was broken. Slark - I've got some vague idea on how it can be implemented with GNSS according to your helpful last email which helps clear some things. But the patch i'm giving does not work. (NULL deference err, possibly due to gdev being NULL). Just sharing on some progress i've made. I'm still looking at it and trying to figure out though. On Tue, Jan 7 2025 at 02:05:38 PM +0800, Slark Xiao <slark_xiao@163.com> wrote: > > At 2025-01-07 03:44:35, "Loic Poulain" <loic.poulain@linaro.org> > wrote: >> Hi Muhammad, >> >> + Slark >> >> On Sun, 5 Jan 2025 at 13:53, Muhammad Nuzaihan >> <zaihan@unrealasia.net> wrote: >>> >>> Based on the code: drivers/bus/mhi/host/pci_generic.c >>> which already has NMEA channel (mhi_quectel_em1xx_channels) >>> support in recent kernels but it is never exposed >>> as a port. >>> >>> This commit exposes that NMEA channel to a port >>> to allow tty/gpsd programs to read through >>> the /dev/wwan0nmea0 port. >>> >>> Tested this change on a new kernel and module >>> built and now NMEA (mhi0_NMEA) statements are >>> available (attached) through /dev/wwan0nmea0 port on bootup. >>> >>> Signed-off-by: Muhammad Nuzaihan Bin Kamal Luddin >>> <zaihan@unrealasia.net> >> >> This works for sure but I'm not entirely convinced NMEA should be >> exposed as a modem control port. In your previous patch version >> Sergey >> pointed to a discussion we had regarding exposing that port as WWAN >> child device through the regular GNSS subsystem, which would require >> some generic bridge in the WWAN subsystem. >> >> Slark, did you have an opportunity to look at the GNSS solution? >> >> Regards, >> Loic > > Hi Loic, > This solution same as what I did in last time. We got a wwan0nmea0 > device but this > device can't support flow control. > Also, this is not the solution what Sergey expected, I think. > Please refer to the target we talked last time: > ///////////////////// >>>> Basically, components should interact like this: >>>> >>>> Modem PCIe driver <-> WWAN core <-> GNSS core <-> /dev/gnss0 >>>> >>>> We need the GNSS core to export the modem NMEA port as instance of >>>> 'gnss' class. >>>> >>>> We need WWAN core between the modem driver and the GSNN core >>>> because we >>>> need a common parent for GNSS port and modem control port (e.g. >>>> AT, >>>> MBIM). Since we are already exporting control ports via the WWAN >>>> subsystem, the GNSS port should also be exported through the WWAN >>>> subsystem. To keep devices hierarchy like this: >>>> >>>> .--> AT port >>>> PCIe dev -> WWAN dev -| >>>> '--> GNSS port >>>> >>>> Back to the implementation. Probably we should introduce a new >>>> port >>>> type, e.g. WWAN_PORT_NMEA. And this port type should have a >>>> special >>>> handling in the WWAN core. >>>> >>> Similar like what I did in my local. I named it as WWAN_PORT_GNSS >>> and >>> put it as same level with AT port. >>> >>>> wwan_create_port() function should not directly create a char >>>> device. >>>> Instead, it should call gnss_allocate_device() and >>>> gnss_register_device() to register the port with GNSS subsystem. > //////////////////// > > Thanks >
Hi Muhammad, Slark, On 07.01.2025 22:19, Muhammad Nuzaihan wrote: > Hi Everyone, Sorry for the last (HTML) email. > > Seemed i forgot Gmail sends HTML email by default and Geary client was > broken. > > Slark - I've got some vague idea on how it can be implemented with GNSS > according to your helpful last email which helps clear some things. I should clarify here a bit. Export through GNSS does not mean creating a dedicated PCI driver. It means call the gnss_register_device() function to export the device to the user space as instance of the GNSS class. This should help for ModemManager as well to avoid the NMEA port access. Since the device class will clearly indicate that the NMEA port has nothing common with a cell modem control. Still, as it was pointed by Loic, it is a good idea to call gnss_register_device() from WWAN core in order to make the WWAN device a common parent of the NMEA port. This should help user space applications as well. A user space application (e.g. GPSd) can easily find a control AT/MBIM/etc port to activate the GNSS functionality of the physical device by checking the NMEA port siblings. > But the patch i'm giving does not work. (NULL deference err, > possibly due to gdev being NULL). > > Just sharing on some progress i've made. A small hint. If a patch is not going to be merged here and now, it's good idea put "RFC" keyword in the subject. E.g.: [RFC v4] wwan dev: Add port for NMEA channel for WWAN devices And another small hint. Use the bottom/inline posting style please. It's really hard to read the email conversation backward. > I'm still looking at it and trying to figure out though. It will be great if you will manage to create the discussed infrastructure inside the WWAN core code. I've already promised Slark to make a prototype, but have a hard time to find a time to do it properly. Sorry :( > On Tue, Jan 7 2025 at 02:05:38 PM +0800, Slark Xiao <slark_xiao@163.com> > wrote: >> >> At 2025-01-07 03:44:35, "Loic Poulain" <loic.poulain@linaro.org> wrote: >>> Hi Muhammad, >>> >>> + Slark >>> >>> On Sun, 5 Jan 2025 at 13:53, Muhammad Nuzaihan >>> <zaihan@unrealasia.net> wrote: >>>> >>>> Based on the code: drivers/bus/mhi/host/pci_generic.c >>>> which already has NMEA channel (mhi_quectel_em1xx_channels) >>>> support in recent kernels but it is never exposed >>>> as a port. >>>> >>>> This commit exposes that NMEA channel to a port >>>> to allow tty/gpsd programs to read through >>>> the /dev/wwan0nmea0 port. >>>> >>>> Tested this change on a new kernel and module >>>> built and now NMEA (mhi0_NMEA) statements are >>>> available (attached) through /dev/wwan0nmea0 port on bootup. >>>> >>>> Signed-off-by: Muhammad Nuzaihan Bin Kamal Luddin >>>> <zaihan@unrealasia.net> >>> >>> This works for sure but I'm not entirely convinced NMEA should be >>> exposed as a modem control port. In your previous patch version Sergey >>> pointed to a discussion we had regarding exposing that port as WWAN >>> child device through the regular GNSS subsystem, which would require >>> some generic bridge in the WWAN subsystem. >>> >>> Slark, did you have an opportunity to look at the GNSS solution? >>> >>> Regards, >>> Loic >> >> Hi Loic, >> This solution same as what I did in last time. We got a wwan0nmea0 >> device but this >> device can't support flow control. >> Also, this is not the solution what Sergey expected, I think. >> Please refer to the target we talked last time: >> ///////////////////// >>>>> Basically, components should interact like this: >>>>> >>>>> Modem PCIe driver <-> WWAN core <-> GNSS core <-> /dev/gnss0 >>>>> >>>>> We need the GNSS core to export the modem NMEA port as instance of >>>>> 'gnss' class. >>>>> >>>>> We need WWAN core between the modem driver and the GSNN core >>>>> because we >>>>> need a common parent for GNSS port and modem control port (e.g. AT, >>>>> MBIM). Since we are already exporting control ports via the WWAN >>>>> subsystem, the GNSS port should also be exported through the WWAN >>>>> subsystem. To keep devices hierarchy like this: >>>>> >>>>> .--> AT port >>>>> PCIe dev -> WWAN dev -| >>>>> '--> GNSS port >>>>> >>>>> Back to the implementation. Probably we should introduce a new port >>>>> type, e.g. WWAN_PORT_NMEA. And this port type should have a special >>>>> handling in the WWAN core. >>>>> >>>> Similar like what I did in my local. I named it as WWAN_PORT_GNSS and >>>> put it as same level with AT port. >>>> >>>>> wwan_create_port() function should not directly create a char device. >>>>> Instead, it should call gnss_allocate_device() and >>>>> gnss_register_device() to register the port with GNSS subsystem.
Hi Sergey, On Wed, Jan 8 2025 at 01:42:00 AM +0200, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote: > Hi Muhammad, Slark, > > I should clarify here a bit. Export through GNSS does not mean > creating a dedicated PCI driver. It means call the > gnss_register_device() function to export the device to the user > space as instance of the GNSS class. This should help for > ModemManager as well to avoid the NMEA port access. Since the device > class will clearly indicate that the NMEA port has nothing common > with a cell modem control. Yes, after reading Slark's email, i began to understand the requirements and i agree with your explanation since I use ModemManager and support for PCIe devices with MHI seems lacking in ModemManager compared to USB based WWAN devices because of the issue above. I did not try to create a dedicated PCI driver, after looking at the code i was trying to allocate the device from the parent driver. Which didn't work. Unlike most other use-case in IoT and Embedded, my use case is more for Desktop usage where interaction with ModemManager is important. > > Still, as it was pointed by Loic, it is a good idea to call > gnss_register_device() from WWAN core in order to make the WWAN > device a common parent of the NMEA port. This should help user space > applications as well. A user space application (e.g. GPSd) can easily > find a control AT/MBIM/etc port to activate the GNSS functionality of > the physical device by checking the NMEA port siblings. Which is what my patch was trying to achieve in wwan_core.c - with the wwan_create_port() function with this code. + /* NMEA check to attach GNSS port */ + if (type == WWAN_PORT_NMEA) { + struct gnss_device *gdev = gnss_allocate_device(parent); + + if (gdev) + gnss_register_device(gdev); + } This was what was suggested in the email by Slark. Maybe i am missing something. Do i need to allocate the gnss device with gnss_allocate_device()? I see from gnss code that i need a "gnss_device" to be able to register. Also this is a just a tip of the iceberg, i would have to look at deregistering the device - removal as well, along the way > >> But the patch i'm giving does not work. (NULL deference err, >> possibly due to gdev being NULL). >> >> Just sharing on some progress i've made. > > A small hint. If a patch is not going to be merged here and now, it's > good idea put "RFC" keyword in the subject. E.g.: > > [RFC v4] wwan dev: Add port for NMEA channel for WWAN devices Will do. > > And another small hint. Use the bottom/inline posting style please. > It's really hard to read the email conversation backward. Sorry about that, i'll follow the list etiquette from now on. > >> I'm still looking at it and trying to figure out though. > > It will be great if you will manage to create the discussed > infrastructure inside the WWAN core code. I've already promised Slark > to make a prototype, but have a hard time to find a time to do it > properly. Sorry :( You mean a new function to pass the parent device, allocate and register? Tried: Modem PCIe driver <-> WWAN core <-> GNSS core <-> /dev/gnss0 I can work on this but i would need some guidance on what needs to be done. The gnss patch above i did was all the understanding i had with what Slark mentioned in this email. The gnss patch did try to initialize gnss in dmesg but it just crashes. No /dev/gnss0 device. Eventhough it's not that urgent for me because i still can read gps data on the current setup but i hope to implement this for sake of completeness since USB WWAN support with GPS is already well supported with ModemManager. (mmcli CLI for GPS subcommands gets error all the time for my PCIe device but that problem's specific to ModemManager) and since i have the hardware, so why not i try to implement to support it. ;-) I think i'll figure something out. :-) > >> On Tue, Jan 7 2025 at 02:05:38 PM +0800, Slark Xiao >> <slark_xiao@163.com> wrote: >>> >>> At 2025-01-07 03:44:35, "Loic Poulain" <loic.poulain@linaro.org> >>> wrote: >>>> Hi Muhammad, >>>> >>>> + Slark
diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c index e9f979d2d851..e13c0b078175 100644 --- a/drivers/net/wwan/mhi_wwan_ctrl.c +++ b/drivers/net/wwan/mhi_wwan_ctrl.c @@ -263,6 +263,7 @@ static const struct mhi_device_id mhi_wwan_ctrl_match_table[] = { { .chan = "QMI", .driver_data = WWAN_PORT_QMI }, { .chan = "DIAG", .driver_data = WWAN_PORT_QCDM }, { .chan = "FIREHOSE", .driver_data = WWAN_PORT_FIREHOSE }, + { .chan = "NMEA", .driver_data = WWAN_PORT_NMEA }, {}, }; MODULE_DEVICE_TABLE(mhi, mhi_wwan_ctrl_match_table); diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c index a51e2755991a..ebf574f2b126 100644 --- a/drivers/net/wwan/wwan_core.c +++ b/drivers/net/wwan/wwan_core.c @@ -342,6 +342,10 @@ static const struct { .name = "MIPC", .devsuf = "mipc", }, + [WWAN_PORT_NMEA] = { + .name = "NMEA", + .devsuf = "nmea", + }, }; static ssize_t type_show(struct device *dev, struct device_attribute *attr, diff --git a/include/linux/wwan.h b/include/linux/wwan.h index a4d6cc0c9f68..ca38a2fe0987 100644 --- a/include/linux/wwan.h +++ b/include/linux/wwan.h @@ -19,6 +19,7 @@ * @WWAN_PORT_FASTBOOT: Fastboot protocol control * @WWAN_PORT_ADB: ADB protocol control * @WWAN_PORT_MIPC: MTK MIPC diagnostic interface + * @WWAN_PORT_NMEA: NMEA GPS statements interface * * @WWAN_PORT_MAX: Highest supported port types * @WWAN_PORT_UNKNOWN: Special value to indicate an unknown port type @@ -34,6 +35,7 @@ enum wwan_port_type { WWAN_PORT_FASTBOOT, WWAN_PORT_ADB, WWAN_PORT_MIPC, + WWAN_PORT_NMEA, /* Add new port types above this line */
Based on the code: drivers/bus/mhi/host/pci_generic.c which already has NMEA channel (mhi_quectel_em1xx_channels) support in recent kernels but it is never exposed as a port. This commit exposes that NMEA channel to a port to allow tty/gpsd programs to read through the /dev/wwan0nmea0 port. Tested this change on a new kernel and module built and now NMEA (mhi0_NMEA) statements are available (attached) through /dev/wwan0nmea0 port on bootup. Signed-off-by: Muhammad Nuzaihan Bin Kamal Luddin <zaihan@unrealasia.net> --- v4: - use git send-mail because of mangled tabs in sent v3 patch v3: - Removed unnecessary code added to "iosm" and "AT IOCTL" which is not relevant. - https://lore.kernel.org/netdev/PVOKPS.9BTDD92U5KK72@unrealasia.net/ v2: - https://lore.kernel.org/netdev/5LHFPS.G3DNPFBCDKCL2@unrealasia.net/ v1: - https://lore.kernel.org/netdev/R8AFPS.THYVK2DKSEE83@unrealasia.net/ --- drivers/net/wwan/mhi_wwan_ctrl.c | 1 + drivers/net/wwan/wwan_core.c | 4 ++++ include/linux/wwan.h | 2 ++ 3 files changed, 7 insertions(+)