| Message ID | ME3P282MB270323F98B97A1A98A50F8F7BBF1A@ME3P282MB2703.AUSP282.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: wwan: t7xx: fw flashing & coredump support | expand |
Tue, Sep 12, 2023 at 11:48:40AM CEST, songjinjian@hotmail.com wrote: >Adds support for t7xx wwan device firmware flashing & coredump collection >using devlink. I don't believe that use of devlink is correct here. It seems like a misfit. IIUC, what you need is to communicate with the modem. Basically a communication channel to modem. The other wwan drivers implement these channels in _ctrl.c files, using multiple protocols. Why can't you do something similar and let devlink out of this please? Until you put in arguments why you really need devlink and why is it a good fit, I'm against this. Please don't send any other versions of this patchset that use devlink. NACK.
Tue, Sep 12, 2023 at 11:48:40AM CEST, songjinjian@hotmail.com wrote: pw-bot: changes-requested
On Wed, 13 Sept 2023 at 11:17, Jiri Pirko <jiri@resnulli.us> wrote: > > Tue, Sep 12, 2023 at 11:48:40AM CEST, songjinjian@hotmail.com wrote: > >Adds support for t7xx wwan device firmware flashing & coredump collection > >using devlink. > > I don't believe that use of devlink is correct here. It seems like a > misfit. IIUC, what you need is to communicate with the modem. Basically > a communication channel to modem. The other wwan drivers implement these > channels in _ctrl.c files, using multiple protocols. Why can't you do > something similar and let devlink out of this please? > > Until you put in arguments why you really need devlink and why is it a > good fit, I'm against this. Please don't send any other versions of this > patchset that use devlink. The t7xx driver already has regular wwan data and control interfaces registered with the wwan framework, making it functional. Here the exposed low level resources are not really wwan/class specific as it is for firmware upgrade and coredump, so I think that is why Jinjian chose the 'feature agnostic' devlink framework. IMHO I think it makes sense to rely on such a framework, or maybe on the devcoredump class. That said, I see the protocol for flashing and doing the coreboot is fastboot, which is already supported on the user side with the fastboot tool, so I'm not sure abstracting it here makes sense. If the protocol is really fasboot compliant, Wouldn't it be simpler to directly expose it as a new device/channel? and rely on a userspace tool for regular fastboot operations (flash, boot, dump). This may require slightly modifying the fastboot tool to detect and support that new transport (in addition to the existing usb and ethernet support). Regards, Loic
Thu, Sep 21, 2023 at 11:36:26AM CEST, loic.poulain@linaro.org wrote: >On Wed, 13 Sept 2023 at 11:17, Jiri Pirko <jiri@resnulli.us> wrote: >> >> Tue, Sep 12, 2023 at 11:48:40AM CEST, songjinjian@hotmail.com wrote: >> >Adds support for t7xx wwan device firmware flashing & coredump collection >> >using devlink. >> >> I don't believe that use of devlink is correct here. It seems like a >> misfit. IIUC, what you need is to communicate with the modem. Basically >> a communication channel to modem. The other wwan drivers implement these >> channels in _ctrl.c files, using multiple protocols. Why can't you do >> something similar and let devlink out of this please? >> >> Until you put in arguments why you really need devlink and why is it a >> good fit, I'm against this. Please don't send any other versions of this >> patchset that use devlink. > >The t7xx driver already has regular wwan data and control interfaces >registered with the wwan framework, making it functional. Here the >exposed low level resources are not really wwan/class specific as it >is for firmware upgrade and coredump, so I think that is why Jinjian >chose the 'feature agnostic' devlink framework. IMHO I think it makes >sense to rely on such a framework, or maybe on the devcoredump class. > >That said, I see the protocol for flashing and doing the coreboot is >fastboot, which is already supported on the user side with the >fastboot tool, so I'm not sure abstracting it here makes sense. If the >protocol is really fasboot compliant, Wouldn't it be simpler to >directly expose it as a new device/channel? and rely on a userspace >tool for regular fastboot operations (flash, boot, dump). This may >require slightly modifying the fastboot tool to detect and support >that new transport (in addition to the existing usb and ethernet >support). Sounds sane. Please let devlink out of this. > >Regards, >Loic
Adds support for t7xx wwan device firmware flashing & coredump collection using devlink. On early detection of wwan device in fastboot mode driver sets up CLDMA0 HW tx/rx queues for raw data transfer and then registers to devlink framework. On user space application issuing command for firmware update the driver sends fastboot flash command & firmware to program NAND. In flashing procedure the fastboot command & response are exchanged between driver and device. Once firmware flashing is success, user space application get modem event by sysfs interface. The devlink param fastboot is set to true via devlink param command. $ devlink dev param set pci/0000:bdf name fastboot value 1 cmode driverinit The wwan device is put into fastboot mode via devlink reload command, by passing `driver_reinit`. $ devlink dev reload pci/0000:$bdf action driver_reinit Note: user space application get the fastboot download event of devcie from /sys/bus/pci/devices/${bdf}/t7xx_event then do remove(echo 1 > /sys/bus/pci/devices/${bdf}/remove) and rescan(echo 1 > /sys/bus/pci/rescan) to let driver goes to firmware flash process. Below is the devlink command usage for firmware flashing $ devlink dev flash pci/$BDF file ABC.img component ABC Note: ABC.img is the firmware to be programmed to "ABC" partition. In case of coredump collection when wwan device encounters an exception it reboots & stays in fastboot mode for coredump collection by host driver. On detecting exception state driver collects the core dump, creates the devlink region & reports an event to user space application for dump collection. The user space application invokes devlink region read command for dump collection. Below are the devlink commands used for coredump collection. $ devlink region new pci/$BDF/mr_dump $ devlink region read pci/$BDF/mr_dump snapshot $ID address $ADD length $LEN $ devlink region del pci/$BDF/mr_dump snapshot $ID Upon completion of firmware flashing or coredump collection the wwan device is reset to normal mode using devlink reload command, by passing `fw_activate`. $ devlink dev reload pci/0000:$bdf action fw_activate Note: user space application get the reset event of devcie from /sys/bus/pci/devices/${bdf}/t7xx_event then do remove(echo 1 > /sys/bus/pci/devices/${bdf}/remove) and rescan(echo 1 > /sys/bus/pci/rescan) to let driver goes to normal process. Jinjian Song (5): net: wwan: t7xx: Infrastructure for early port configuration net: wwan: t7xx: Register with devlink and implement firmware flashing net: wwan: t7xx: Creates region & snapshot for coredump log collection net: wwan: t7xx: Adds sysfs attribute of modem event net: wwan: t7xx: Devlink documentation Documentation/networking/devlink/index.rst | 1 + Documentation/networking/devlink/t7xx.rst | 232 +++++++ drivers/net/wwan/Kconfig | 1 + drivers/net/wwan/t7xx/Makefile | 4 +- drivers/net/wwan/t7xx/t7xx_hif_cldma.c | 47 +- drivers/net/wwan/t7xx/t7xx_hif_cldma.h | 18 +- drivers/net/wwan/t7xx/t7xx_modem_ops.c | 5 +- drivers/net/wwan/t7xx/t7xx_pci.c | 79 ++- drivers/net/wwan/t7xx/t7xx_pci.h | 19 + drivers/net/wwan/t7xx/t7xx_port.h | 6 + drivers/net/wwan/t7xx/t7xx_port_ap_msg.c | 78 +++ drivers/net/wwan/t7xx/t7xx_port_ap_msg.h | 11 + drivers/net/wwan/t7xx/t7xx_port_flash_dump.c | 695 +++++++++++++++++++ drivers/net/wwan/t7xx/t7xx_port_flash_dump.h | 85 +++ drivers/net/wwan/t7xx/t7xx_port_proxy.c | 118 +++- drivers/net/wwan/t7xx/t7xx_port_proxy.h | 14 + drivers/net/wwan/t7xx/t7xx_port_wwan.c | 27 +- drivers/net/wwan/t7xx/t7xx_reg.h | 28 +- drivers/net/wwan/t7xx/t7xx_state_monitor.c | 137 +++- drivers/net/wwan/t7xx/t7xx_state_monitor.h | 1 + 20 files changed, 1528 insertions(+), 78 deletions(-) create mode 100644 Documentation/networking/devlink/t7xx.rst create mode 100644 drivers/net/wwan/t7xx/t7xx_port_ap_msg.c create mode 100644 drivers/net/wwan/t7xx/t7xx_port_ap_msg.h create mode 100644 drivers/net/wwan/t7xx/t7xx_port_flash_dump.c create mode 100644 drivers/net/wwan/t7xx/t7xx_port_flash_dump.h