| Message ID | 20211101035635.26999-1-ricardo.martinez@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: wwan: t7xx: PCIe driver for MediaTek M.2 modem | expand |
11/1/21 6:56 AM, Ricardo Martinez пишет: > t7xx is the PCIe host device driver for Intel 5G 5000 M.2 solution which > is based on MediaTek's T700 modem to provide WWAN connectivity. > The driver uses the WWAN framework infrastructure to create the following > control ports and network interfaces: > * /dev/wwan0mbim0 - Interface conforming to the MBIM protocol. > Applications like libmbim [1] or Modem Manager [2] from v1.16 onwards > with [3][4] can use it to enable data communication towards WWAN. > * /dev/wwan0at0 - Interface that supports AT commands. > * wwan0 - Primary network interface for IP traffic. That should be prefixed with net-next > > The main blocks in t7xx driver are: > * PCIe layer - Implements probe, removal, and power management callbacks. > * Port-proxy - Provides a common interface to interact with different types > of ports such as WWAN ports. > * Modem control & status monitor - Implements the entry point for modem > initialization, reset and exit, as well as exception handling. > * CLDMA (Control Layer DMA) - Manages the HW used by the port layer to send > control messages to the modem using MediaTek's CCCI (Cross-Core > Communication Interface) protocol. > * DPMAIF (Data Plane Modem AP Interface) - Controls the HW that provides > uplink and downlink queues for the data path. The data exchange takes > place using circular buffers to share data buffer addresses and metadata > to describe the packets. > * MHCCIF (Modem Host Cross-Core Interface) - Provides interrupt channels > for bidirectional event notification such as handshake, exception, PM and > port enumeration. > > The compilation of the t7xx driver is enabled by the CONFIG_MTK_T7XX config > option which depends on CONFIG_WWAN. > This driver was originally developed by MediaTek. Intel adapted t7xx to > the WWAN framework, optimized and refactored the driver source in close > collaboration with MediaTek. This will enable getting the t7xx driver on > Approved Vendor List for interested OEM's and ODM's productization plans > with Intel 5G 5000 M.2 solution. > > List of contributors: > Amir Hanania <amir.hanania@intel.com> > Andriy Shevchenko <andriy.shevchenko@linux.intel.com> > Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> > Dinesh Sharma <dinesh.sharma@intel.com> > Eliot Lee <eliot.lee@intel.com> > Haijun Liu <haijun.liu@mediatek.com> > M Chetan Kumar <m.chetan.kumar@intel.com> > Mika Westerberg <mika.westerberg@linux.intel.com> > Moises Veleta <moises.veleta@intel.com> > Pierre-louis Bossart <pierre-louis.bossart@intel.com> > Chiranjeevi Rapolu <chiranjeevi.rapolu@intel.com> > Ricardo Martinez <ricardo.martinez@linux.intel.com> > Muralidharan Sethuraman <muralidharan.sethuraman@intel.com> > Soumya Prakash Mishra <Soumya.Prakash.Mishra@intel.com> > Sreehari Kancharla <sreehari.kancharla@intel.com> > Suresh Nagaraj <suresh.nagaraj@intel.com> > > [1] https://www.freedesktop.org/software/libmbim/ > [2] https://www.freedesktop.org/software/ModemManager/ > [3] https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/merge_requests/582 > [4] https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/merge_requests/523 > > v2: > - Replace pdev->driver->name with dev_driver_string(&pdev->dev). > - Replace random_ether_addr() with eth_random_addr(). > - Update kernel-doc comment for enum data_policy. > - Indicate the driver is 'Supported' instead of 'Maintained'. > - Fix the Signed-of-by and Co-developed-by tags in the patches. > - Added authors and contributors in the top comment of the src files. > > Chandrashekar Devegowda (1): > net: wwan: t7xx: Add AT and MBIM WWAN ports > > Haijun Lio (11): > net: wwan: t7xx: Add control DMA interface > net: wwan: t7xx: Add core components > net: wwan: t7xx: Add port proxy infrastructure > net: wwan: t7xx: Add control port > net: wwan: t7xx: Data path HW layer > net: wwan: t7xx: Add data path interface > net: wwan: t7xx: Add WWAN network interface > net: wwan: t7xx: Introduce power management support > net: wwan: t7xx: Runtime PM > net: wwan: t7xx: Device deep sleep lock/unlock > net: wwan: t7xx: Add debug and test ports > > Ricardo Martinez (2): > net: wwan: Add default MTU size > net: wwan: t7xx: Add maintainers and documentation > > .../networking/device_drivers/wwan/index.rst | 1 + > .../networking/device_drivers/wwan/t7xx.rst | 120 ++ > MAINTAINERS | 11 + > drivers/net/wwan/Kconfig | 14 + > drivers/net/wwan/Makefile | 1 + > drivers/net/wwan/t7xx/Makefile | 24 + > drivers/net/wwan/t7xx/t7xx_cldma.c | 277 +++ > drivers/net/wwan/t7xx/t7xx_cldma.h | 168 ++ > drivers/net/wwan/t7xx/t7xx_common.h | 76 + > drivers/net/wwan/t7xx/t7xx_dpmaif.c | 1524 +++++++++++++++ > drivers/net/wwan/t7xx/t7xx_dpmaif.h | 168 ++ > drivers/net/wwan/t7xx/t7xx_hif_cldma.c | 1663 +++++++++++++++++ > drivers/net/wwan/t7xx/t7xx_hif_cldma.h | 156 ++ > drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c | 638 +++++++ > drivers/net/wwan/t7xx/t7xx_hif_dpmaif.h | 279 +++ > drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c | 1562 ++++++++++++++++ > drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.h | 117 ++ > drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c | 842 +++++++++ > drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.h | 82 + > drivers/net/wwan/t7xx/t7xx_mhccif.c | 124 ++ > drivers/net/wwan/t7xx/t7xx_mhccif.h | 35 + > drivers/net/wwan/t7xx/t7xx_modem_ops.c | 747 ++++++++ > drivers/net/wwan/t7xx/t7xx_modem_ops.h | 92 + > drivers/net/wwan/t7xx/t7xx_monitor.h | 147 ++ > drivers/net/wwan/t7xx/t7xx_netdev.c | 545 ++++++ > drivers/net/wwan/t7xx/t7xx_netdev.h | 63 + > drivers/net/wwan/t7xx/t7xx_pci.c | 789 ++++++++ > drivers/net/wwan/t7xx/t7xx_pci.h | 121 ++ > drivers/net/wwan/t7xx/t7xx_pcie_mac.c | 277 +++ > drivers/net/wwan/t7xx/t7xx_pcie_mac.h | 36 + > drivers/net/wwan/t7xx/t7xx_port.h | 163 ++ > drivers/net/wwan/t7xx/t7xx_port_char.c | 424 +++++ > drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c | 150 ++ > drivers/net/wwan/t7xx/t7xx_port_proxy.c | 829 ++++++++ > drivers/net/wwan/t7xx/t7xx_port_proxy.h | 102 + > drivers/net/wwan/t7xx/t7xx_port_tty.c | 191 ++ > drivers/net/wwan/t7xx/t7xx_port_wwan.c | 281 +++ > drivers/net/wwan/t7xx/t7xx_reg.h | 398 ++++ > drivers/net/wwan/t7xx/t7xx_skb_util.c | 362 ++++ > drivers/net/wwan/t7xx/t7xx_skb_util.h | 110 ++ > drivers/net/wwan/t7xx/t7xx_state_monitor.c | 627 +++++++ > drivers/net/wwan/t7xx/t7xx_tty_ops.c | 205 ++ > drivers/net/wwan/t7xx/t7xx_tty_ops.h | 44 + > include/linux/wwan.h | 5 + > 44 files changed, 14590 insertions(+) > create mode 100644 Documentation/networking/device_drivers/wwan/t7xx.rst > create mode 100644 drivers/net/wwan/t7xx/Makefile > create mode 100644 drivers/net/wwan/t7xx/t7xx_cldma.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_cldma.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_common.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_dpmaif.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_dpmaif.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_cldma.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_cldma.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_dpmaif.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_mhccif.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_mhccif.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_modem_ops.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_modem_ops.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_monitor.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_netdev.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_netdev.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_pci.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_pci.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_pcie_mac.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_pcie_mac.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_port.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_port_char.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_port_proxy.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_port_proxy.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_port_tty.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_port_wwan.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_reg.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_skb_util.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_skb_util.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_state_monitor.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_tty_ops.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_tty_ops.h >
Hello Ricardo, On Mon, Nov 1, 2021 at 6:57 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > t7xx is the PCIe host device driver for Intel 5G 5000 M.2 solution which > is based on MediaTek's T700 modem to provide WWAN connectivity. > The driver uses the WWAN framework infrastructure to create the following > control ports and network interfaces: > * /dev/wwan0mbim0 - Interface conforming to the MBIM protocol. > Applications like libmbim [1] or Modem Manager [2] from v1.16 onwards > with [3][4] can use it to enable data communication towards WWAN. > * /dev/wwan0at0 - Interface that supports AT commands. > * wwan0 - Primary network interface for IP traffic. > > The main blocks in t7xx driver are: > * PCIe layer - Implements probe, removal, and power management callbacks. > * Port-proxy - Provides a common interface to interact with different types > of ports such as WWAN ports. > * Modem control & status monitor - Implements the entry point for modem > initialization, reset and exit, as well as exception handling. > * CLDMA (Control Layer DMA) - Manages the HW used by the port layer to send > control messages to the modem using MediaTek's CCCI (Cross-Core > Communication Interface) protocol. > * DPMAIF (Data Plane Modem AP Interface) - Controls the HW that provides > uplink and downlink queues for the data path. The data exchange takes > place using circular buffers to share data buffer addresses and metadata > to describe the packets. > * MHCCIF (Modem Host Cross-Core Interface) - Provides interrupt channels > for bidirectional event notification such as handshake, exception, PM and > port enumeration. > > The compilation of the t7xx driver is enabled by the CONFIG_MTK_T7XX config > option which depends on CONFIG_WWAN. > This driver was originally developed by MediaTek. Intel adapted t7xx to > the WWAN framework, optimized and refactored the driver source in close > collaboration with MediaTek. This will enable getting the t7xx driver on > Approved Vendor List for interested OEM's and ODM's productization plans > with Intel 5G 5000 M.2 solution. Nice work! The driver generally looks good for me. But at the same time the driver looks a bit raw, needs some style and functionality improvements, and a lot of cleanup. Please find general thoughts below and per-patch comments. A one nitpick that is common for the entire series. Please consider using a common prefix for all driver function names (e.g. t7xx_) to make them more specific. This should improve the code readability. Thus, any reader will know for sure that the called functions belong to the driver, and not to a generic kernel API. E.g. use the t7xx_cldma_hw_init() name for the CLDMA initialization function instead of the too generic cldma_hw_init() name, etc. Interestingly, that you are using the common 't7xx_' prefix for all driver file names. This is Ok, but it does not add to the specifics as all driver files are already located in a common directory with the specific name. But function names at the same time lack a common prefix. Another common drawback is that the driver should break as soon as two modems are connected simultaneously. This should happen due to the use of multiple _global_ variables that keeps pointers to a modem runtime state. Out of curiosity, did you test the driver with two or more modems connected simultaneously? Next, the driver entirely lacks the multibyte field endians handling. Looks like it will be unable to run on a big-endians CPU. To fix this, it is needed to find all the structures that are passed to the modem and replace the multibyte fields of types u16/u32 with __le16/__le32. Then examine all the field accesses and use cpu_to_le{16,32}()/le{16,32}_to_cpu() to update/read field contents. As soon as you change the types to __le16/__le32, sparse (a static analyzing utility) will warn you about every unsafe field access. Just build your kernel with make C=1. Ricardo, please consider submitting at the next iteration a patch series with the driver that will be cleaned from debug stuff and questionable optimizations. Just a bare minimum functional: AT/MBIM control ports and network interface for data communications. This will cut the code in half. What will greatly facilitate the reviewing process. And then extend the driver functionality with follow up patches. -- Sergey
On 11/6/2021 11:10 AM, Sergey Ryazanov wrote: > Hello Ricardo, > > On Mon, Nov 1, 2021 at 6:57 AM Ricardo Martinez > <ricardo.martinez@linux.intel.com> wrote: ... > Nice work! The driver generally looks good for me. But at the same > time the driver looks a bit raw, needs some style and functionality > improvements, and a lot of cleanup. Please find general thoughts below > and per-patch comments. Thanks for the feedback Sergey, we will work on it. > > A one nitpick that is common for the entire series. Please consider > using a common prefix for all driver function names (e.g. t7xx_) to > make them more specific. This should improve the code readability. > Thus, any reader will know for sure that the called functions belong > to the driver, and not to a generic kernel API. E.g. use the > t7xx_cldma_hw_init() name for the CLDMA initialization function > instead of the too generic cldma_hw_init() name, etc. Does this apply to static functions as well? > > Interestingly, that you are using the common 't7xx_' prefix for all > driver file names. This is Ok, but it does not add to the specifics as > all driver files are already located in a common directory with the > specific name. But function names at the same time lack a common > prefix. > > Another common drawback is that the driver should break as soon as two > modems are connected simultaneously. This should happen due to the use > of multiple _global_ variables that keeps pointers to a modem runtime > state. Out of curiosity, did you test the driver with two or more > modems connected simultaneously? We haven't tested such configurations, we are focusing on platforms with one single modem. > > Next, the driver entirely lacks the multibyte field endians handling. > Looks like it will be unable to run on a big-endians CPU. To fix this, > it is needed to find all the structures that are passed to the modem > and replace the multibyte fields of types u16/u32 with __le16/__le32. > Then examine all the field accesses and use > cpu_to_le{16,32}()/le{16,32}_to_cpu() to update/read field contents. > As soon as you change the types to __le16/__le32, sparse (a static > analyzing utility) will warn you about every unsafe field access. Just > build your kernel with make C=1. > > Ricardo, please consider submitting at the next iteration a patch > series with the driver that will be cleaned from debug stuff and > questionable optimizations. Just a bare minimum functional: AT/MBIM > control ports and network interface for data communications. This will > cut the code in half. What will greatly facilitate the reviewing > process. And then extend the driver functionality with follow up > patches. > > -- > Sergey >
On Tue, Nov 9, 2021 at 8:26 AM Martinez, Ricardo wrote: > On 11/6/2021 11:10 AM, Sergey Ryazanov wrote: >> A one nitpick that is common for the entire series. Please consider >> using a common prefix for all driver function names (e.g. t7xx_) to >> make them more specific. This should improve the code readability. >> Thus, any reader will know for sure that the called functions belong >> to the driver, and not to a generic kernel API. E.g. use the >> t7xx_cldma_hw_init() name for the CLDMA initialization function >> instead of the too generic cldma_hw_init() name, etc. > > Does this apply to static functions as well? As I wrote, this is a nitpick. As you can see in Documentation/process/coding-style.rst, there are no general rules for functions naming. My personal rule of thumb is that if a function performs a very general operation (like averaging, interpolation, etc.), then a prefix can be omitted. If a function operation is specific for a module, then add a common module prefix to the function name. But again, this is my personal rule. As for the driver, it was quite difficult to read the code that calls functions such as cldma_alloc(), cldma_init(). It was hard to figure out whether these functions are new kernel API or they are specific to the driver. A common way to solve such ambiguity issues is to prefix the driver function names. But again, this was just an attempt to draw your attention to the function naming. Feel free to name functions as you would like, just make the code readable for developers who are not familiar with the specific HW chip. >> Another common drawback is that the driver should break as soon as two >> modems are connected simultaneously. This should happen due to the use >> of multiple _global_ variables that keeps pointers to a modem runtime >> state. Out of curiosity, did you test the driver with two or more >> modems connected simultaneously? > > We haven't tested such configurations, we are focusing on platforms with one single modem. Now you are aware of the potential kernel crash due to the global variables misuse. Please fix it.