Re: [net-next v7 2/2] net: wwan: t7xx: Add debug port
From: Jinjian Song
Date: Tue Oct 29 2024 - 04:23:00 EST
From: Sergey Ryazanov <ryazanov.s.a@xxxxxxxxx>
Hi Sergey,
Hello Jinjian,
On 26.10.2024 12:09, Jinjian Song wrote:
[skiped]
Application can use MIPC (Modem Information Process Center) port
to debug antenna tuner or noise profiling through this MTK modem
diagnostic interface.
By default, debug ports are not exposed, so using the command
to enable or disable debug ports.
Switch on debug port:
- debug: 'echo debug > /sys/bus/pci/devices/${bdf}/t7xx_mode
Switch off debug port:
- normal: 'echo normal > /sys/bus/pci/devices/${bdf}/t7xx_mode
Looks like this part of the message needs an update. Now driver uses a
dedicated file for this operation.
Yes, please let me update it, thanks.
diff --git a/Documentation/networking/device_drivers/wwan/t7xx.rst b/Documentation/networking/device_drivers/wwan/t7xx.rst
index f346f5f85f15..6071dee8c186 100644
--- a/Documentation/networking/device_drivers/wwan/t7xx.rst
+++ b/Documentation/networking/device_drivers/wwan/t7xx.rst
@@ -7,12 +7,13 @@
============================================
t7xx driver for MTK PCIe based T700 5G modem
============================================
-The t7xx driver is a WWAN PCIe host driver developed for linux or Chrome OS platforms
-for data exchange over PCIe interface between Host platform & MediaTek's T700 5G modem.
-The driver exposes an interface conforming to the MBIM protocol [1]. Any front end
-application (e.g. Modem Manager) could easily manage the MBIM interface to enable
-data communication towards WWAN. The driver also provides an interface to interact
-with the MediaTek's modem via AT commands.
+The t7xx driver is a WWAN PCIe host driver developed for linux or Chrome OS
+platforms for data exchange over PCIe interface between Host platform &
+MediaTek's T700 5G modem.
+The driver exposes an interface conforming to the MBIM protocol [1]. Any front
+end application (e.g. Modem Manager) could easily manage the MBIM interface to
+enable data communication towards WWAN. The driver also provides an interface
+to interact with the MediaTek's modem via AT commands.
Thank you for taking care and unifying documentation, still, I believe,
this change doesn't belong to this specific patch, what introduced debug
ports toggling knob. Could you factor our these formating updating
changes into a dedicated patch? E.g. add a new patch "2/3: unify
documentation" and make this patch third in the series.
Got it, please let me do it.
@@ -67,6 +68,28 @@ Write from userspace to set the device mode.
::
$ echo fastboot_switching > /sys/bus/pci/devices/${bdf}/t7xx_mode
+t7xx_port_mode
I believe we should use the plural form - portS, since this knob
controls behaviour of the group of ports.
Fastboot switching is a bit special, this will trigger the WWAN device
reboot to fastboot mode and only a fastboot port in this mode, How about keep it
as before?
And I have one more suggestion. "mode" sounds too generic, can we
consider renaming this option to something, what includes more details
about the mode. E.g. can we rename this knob to 't7xx_debug_ports' and
make it simple boolean (on/off) option?
Yes, rename to 't7xx_debug_ports' and make it boolean is reasonablei, please let
me change it.
-static struct attribute *t7xx_mode_attr[] = {
+static ssize_t t7xx_port_mode_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct t7xx_pci_dev *t7xx_dev;
+ struct pci_dev *pdev;
+ int index = 0;
+
+ pdev = to_pci_dev(dev);
This assignment should be done along the variable declaration to make
code shorter:
Yes, please let me modify it.
struct pci_dev *pdev = to_pci_dev(dev);
+ t7xx_dev = pci_get_drvdata(pdev);
+ if (!t7xx_dev)
+ return -ENODEV;
+
+ index = sysfs_match_string(t7xx_port_mode_names, buf);
+ if (index == T7XX_DEBUG) {
+ t7xx_proxy_port_debug(t7xx_dev, true);
Another one nit picking question. It is unclear what is going to happen
after this call. Can we rename this function to something what clearly
indicates the desired reaction? E.g. t7xx_proxy_debug_ports_show(...).
After t7xx_proxy_port_debug(t7xx_dev, true) the adb and mipc port will be
shown directly, let me rename to t7xx_proxy_debug_ports_show more clearly.
+static ssize_t t7xx_port_mode_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ enum t7xx_port_mode port_mode = T7XX_NORMAL;
+ struct t7xx_pci_dev *t7xx_dev;
+ struct pci_dev *pdev;
+
+ pdev = to_pci_dev(dev);
Also should be assigned on declaration.
Yes, please let me modify it.
+enum t7xx_port_mode {
+ T7XX_NORMAL,
+ T7XX_DEBUG,
+ T7XX_PORT_MODE_LAST, /* must always be last */
+};
+
/* struct t7xx_pci_dev - MTK device context structure
* @intr_handler: array of handler function for request_threaded_irq
* @intr_thread: array of thread_fn for request_threaded_irq
@@ -94,6 +100,7 @@ struct t7xx_pci_dev {
struct dentry *debugfs_dir;
#endif
u32 mode;
+ u32 port_mode;
If we agree to rename the sysfs file to 't7xx_debug_ports', this field
can be renamed to something more specific like 'debug_ports_show'.
Yes, let me rename sysfs file to 't7xx_debug_ports' and rename this feild
to 'debug_ports_show'.
struct t7xx_port {
diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
index 35743e7de0c3..26d3f57732cc 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
@@ -39,6 +39,8 @@
#define Q_IDX_CTRL 0
#define Q_IDX_MBIM 2
+#define Q_IDX_MIPC 2
Are you sure that we should define a new name for the same queue id? Can
we just specify Q_IDX_MBIM in the port description or rename Q_IDX_MBIM
to Q_IDX_MBIM_MIPC to avoid id names duplication?
Since MBIM and MIPC use the same queue id, please let me rename to Q_IDX_MBIM_MIPC.
+void t7xx_proxy_port_debug(struct t7xx_pci_dev *t7xx_dev, bool show)
+{
+ struct port_proxy *port_prox = t7xx_dev->md->port_prox;
+ struct t7xx_port *port;
+ int i;
+
+ for_each_proxy_port(i, port, port_prox) {
+ const struct t7xx_port_conf *port_conf = port->port_conf;
+
+ if (port_conf->debug && port_conf->ops && port_conf->ops->init) {
+ if (show)
+ port_conf->ops->init(port);
+ else
+ port_conf->ops->uninit(port);
I still do not like the assumption that if .init method is defined then
the .uninit method is defined too. Is it make sense to compose these
checks like below. Mmove the check for .init/.uninit inside and add a
comment justifying absense of a check of a current port state.
/* NB: it is safe to call init/uninit multiple times */
if (port_conf->debug && port_conf->ops) {
if (show && port_conf->ops->init)
port_conf->ops->init(port);
else if (!show && port_conf->ops->uninit)
port_conf->ops->uninit(port);
}
Yes, please let me modify it.
Best Regards,
Jinjian.