Message-ID: <ZbvCorTYXK1o_sdV@nanopsycho>
Date: Thu, 1 Feb 2024 17:11:14 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Jinjian Song <songjinjian@...mail.com>
Cc: netdev@...r.kernel.org, chandrashekar.devegowda@...el.com,
	chiranjeevi.rapolu@...ux.intel.com, haijun.liu@...iatek.com,
	m.chetan.kumar@...ux.intel.com, ricardo.martinez@...ux.intel.com,
	loic.poulain@...aro.org, ryazanov.s.a@...il.com,
	johannes@...solutions.net, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, linux-kernel@...r.kernel.com,
	vsankar@...ovo.com, letitia.tsai@...com, pin-hao.huang@...com,
	danielwinkler@...gle.com, nmarupaka@...gle.com,
	joey.zhao@...ocom.com, liuqf@...ocom.com, felix.yan@...ocom.com,
	angel.huang@...ocom.com, freddy.lin@...ocom.com,
	alan.zhang1@...ocom.com, zhangrc@...ocom.com,
	Jinjian Song <jinjian.song@...ocom.com>
Subject: Re: [net-next v7 2/4] net: wwan: t7xx: Add sysfs attribute for
 device state machine

Thu, Feb 01, 2024 at 04:13:38PM CET, songjinjian@...mail.com wrote:
>From: Jinjian Song <jinjian.song@...ocom.com>
>
>Add support for userspace to get/set the device mode, device's state machine
>changes between (UNKNOWN/READY/RESET/FASTBOOT_DL_MODE/FASTBOOT_DUMP_MODE).
>
>Get the device state mode:
> - 'cat /sys/bus/pci/devices/${bdf}/t7xx_mode'
>
>Set the device state mode:
> - reset(cold reset): 'echo RESET > /sys/bus/pci/devices/${bdf}/t7xx_mode'
> - fastboot: 'echo FASTBOOT_DL_SWITCHING > /sys/bus/pci/devices/${bdf}/t7xx_mode'
>Reload driver to get the new device state after setting operation.
>
>Signed-off-by: Jinjian Song <jinjian.song@...ocom.com>
>---
>v7:
> * add sysfs description to commit info 
> * update t7xx_dev->mode after reset by sysfs t7xx_mode
>v6:
> * change code style in t7xx_mode_store()
>v5:
> * add cold reset support via sysfs t7xx_mode
>v4:
> * narrow down the set of accepted values in t7xx_mode_store()
> * change mode type atomic to u32 with READ_ONCE()/WRITE_ONCE()
> * delete 'T7XX_MODEM' prefix and using sysfs_emit in t7xx_mode_show()
> * add description of sysfs t7xx_mode in document t7xx.rst
>v2:
> * optimizing using goto label in t7xx_pci_probe
>---
> .../networking/device_drivers/wwan/t7xx.rst   | 28 ++++++
> drivers/net/wwan/t7xx/t7xx_modem_ops.c        |  6 ++
> drivers/net/wwan/t7xx/t7xx_modem_ops.h        |  1 +
> drivers/net/wwan/t7xx/t7xx_pci.c              | 98 ++++++++++++++++++-
> drivers/net/wwan/t7xx/t7xx_pci.h              | 14 ++-
> drivers/net/wwan/t7xx/t7xx_state_monitor.c    |  1 +
> 6 files changed, 143 insertions(+), 5 deletions(-)
>
>diff --git a/Documentation/networking/device_drivers/wwan/t7xx.rst b/Documentation/networking/device_drivers/wwan/t7xx.rst
>index dd5b731957ca..d13624a52d8b 100644
>--- a/Documentation/networking/device_drivers/wwan/t7xx.rst
>+++ b/Documentation/networking/device_drivers/wwan/t7xx.rst
>@@ -39,6 +39,34 @@ command and receive response:
> 
> - open the AT control channel using a UART tool or a special user tool
> 
>+Sysfs
>+=====
>+The driver provides sysfs interfaces to userspace.
>+
>+t7xx_mode
>+---------
>+The sysfs interface provides userspace with access to the device mode, this interface
>+supports read and write operations.
>+
>+Device mode:
>+
>+- ``UNKNOW`` represents that device in unknown status

should be "unknown", missing "n".

Btw, why are you using capitals for the mode names?



>+- ``READY`` represents that device in ready status
>+- ``RESET`` represents that device in reset status
>+- ``FASTBOOT_DL_SWITCHING`` represents that device in fastboot switching status
>+- ``FASTBOOT_DL_MODE`` represents that device in fastboot download status
>+- ``FASTBOOT_DL_DUMP_MODE`` represents that device in fastboot dump status
>+
>+Read from userspace to get the current device mode.
>+
>+::
>+  $ cat /sys/bus/pci/devices/${bdf}/t7xx_mode
>+
>+Write from userspace to set the device mode.
>+
>+::
>+  $ echo FASTBOOT_DL_SWITCHING > /sys/bus/pci/devices/${bdf}/t7xx_mode
>+
> Management application development
> ==================================
> The driver and userspace interfaces are described below. The MBIM protocol is
>diff --git a/drivers/net/wwan/t7xx/t7xx_modem_ops.c b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
>index 24e7d491468e..ca262d2961ed 100644
>--- a/drivers/net/wwan/t7xx/t7xx_modem_ops.c
>+++ b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
>@@ -177,6 +177,11 @@ int t7xx_acpi_fldr_func(struct t7xx_pci_dev *t7xx_dev)
> 	return t7xx_acpi_reset(t7xx_dev, "_RST");
> }
> 
>+int t7xx_acpi_pldr_func(struct t7xx_pci_dev *t7xx_dev)
>+{
>+	return t7xx_acpi_reset(t7xx_dev, "MRST._RST");
>+}
>+
> static void t7xx_reset_device_via_pmic(struct t7xx_pci_dev *t7xx_dev)
> {
> 	u32 val;
>@@ -192,6 +197,7 @@ static irqreturn_t t7xx_rgu_isr_thread(int irq, void *data)
> {
> 	struct t7xx_pci_dev *t7xx_dev = data;
> 
>+	t7xx_mode_update(t7xx_dev, T7XX_RESET);
> 	msleep(RGU_RESET_DELAY_MS);
> 	t7xx_reset_device_via_pmic(t7xx_dev);
> 	return IRQ_HANDLED;
>diff --git a/drivers/net/wwan/t7xx/t7xx_modem_ops.h b/drivers/net/wwan/t7xx/t7xx_modem_ops.h
>index abe633cf7adc..b39e945a92e0 100644
>--- a/drivers/net/wwan/t7xx/t7xx_modem_ops.h
>+++ b/drivers/net/wwan/t7xx/t7xx_modem_ops.h
>@@ -85,6 +85,7 @@ int t7xx_md_init(struct t7xx_pci_dev *t7xx_dev);
> void t7xx_md_exit(struct t7xx_pci_dev *t7xx_dev);
> void t7xx_clear_rgu_irq(struct t7xx_pci_dev *t7xx_dev);
> int t7xx_acpi_fldr_func(struct t7xx_pci_dev *t7xx_dev);
>+int t7xx_acpi_pldr_func(struct t7xx_pci_dev *t7xx_dev);
> int t7xx_pci_mhccif_isr(struct t7xx_pci_dev *t7xx_dev);
> 
> #endif	/* __T7XX_MODEM_OPS_H__ */
>diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
>index 91256e005b84..1a10afd948c7 100644
>--- a/drivers/net/wwan/t7xx/t7xx_pci.c
>+++ b/drivers/net/wwan/t7xx/t7xx_pci.c
>@@ -52,6 +52,81 @@
> #define PM_RESOURCE_POLL_TIMEOUT_US	10000
> #define PM_RESOURCE_POLL_STEP_US	100
> 
>+static const char * const mode_names[] = {

t7xx_mode_names


>+	[T7XX_UNKNOWN] = "UNKNOWN",
>+	[T7XX_READY] = "READY",
>+	[T7XX_RESET] = "RESET",
>+	[T7XX_FASTBOOT_DL_SWITCHING] = "FASTBOOT_DL_SWITCHING",
>+	[T7XX_FASTBOOT_DL_MODE] = "FASTBOOT_DL_MODE",
>+	[T7XX_FASTBOOT_DUMP_MODE] = "FASTBOOT_DUMP_MODE",
>+};
>+
>+static_assert(ARRAY_SIZE(mode_names) == T7XX_MODE_LAST);
>+
>+static ssize_t t7xx_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);
>+	t7xx_dev = pci_get_drvdata(pdev);
>+	if (!t7xx_dev)
>+		return -ENODEV;
>+
>+	index = sysfs_match_string(mode_names, buf);
>+	if (index == T7XX_FASTBOOT_DL_SWITCHING) {
>+		WRITE_ONCE(t7xx_dev->mode, T7XX_FASTBOOT_DL_SWITCHING);
>+	} else if (index == T7XX_RESET) {
>+		WRITE_ONCE(t7xx_dev->mode, T7XX_RESET);
>+		t7xx_acpi_pldr_func(t7xx_dev);
>+	}
>+
>+	return count;
>+};
>+
>+static ssize_t t7xx_mode_show(struct device *dev,
>+			      struct device_attribute *attr,
>+			      char *buf)
>+{
>+	enum t7xx_mode mode = T7XX_UNKNOWN;
>+	struct t7xx_pci_dev *t7xx_dev;
>+	struct pci_dev *pdev;
>+
>+	pdev = to_pci_dev(dev);
>+	t7xx_dev = pci_get_drvdata(pdev);
>+	if (!t7xx_dev)
>+		return -ENODEV;
>+
>+	mode = READ_ONCE(t7xx_dev->mode);
>+	if (mode < T7XX_MODE_LAST)
>+		return sysfs_emit(buf, "%s\n", mode_names[mode]);
>+
>+	return sysfs_emit(buf, "%s\n", mode_names[T7XX_UNKNOWN]);
>+}
>+
>+static DEVICE_ATTR_RW(t7xx_mode);
>+
>+static struct attribute *t7xx_mode_attr[] = {
>+	&dev_attr_t7xx_mode.attr,
>+	NULL
>+};
>+
>+static const struct attribute_group t7xx_mode_attribute_group = {
>+	.attrs = t7xx_mode_attr,
>+};
>+
>+void t7xx_mode_update(struct t7xx_pci_dev *t7xx_dev, enum t7xx_mode mode)
>+{
>+	if (!t7xx_dev)
>+		return;
>+
>+	WRITE_ONCE(t7xx_dev->mode, mode);
>+	sysfs_notify(&t7xx_dev->pdev->dev.kobj, NULL, "t7xx_mode");
>+}
>+
> enum t7xx_pm_state {
> 	MTK_PM_EXCEPTION,
> 	MTK_PM_INIT,		/* Device initialized, but handshake not completed */
>@@ -729,16 +804,28 @@ static int t7xx_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> 
> 	t7xx_pcie_mac_interrupts_dis(t7xx_dev);
> 
>+	ret = sysfs_create_group(&t7xx_dev->pdev->dev.kobj,
>+				 &t7xx_mode_attribute_group);
>+	if (ret)
>+		goto err_md_exit;
>+
> 	ret = t7xx_interrupt_init(t7xx_dev);
>-	if (ret) {
>-		t7xx_md_exit(t7xx_dev);
>-		return ret;
>-	}
>+	if (ret)
>+		goto err_remove_group;
>+
> 
> 	t7xx_pcie_mac_set_int(t7xx_dev, MHCCIF_INT);
> 	t7xx_pcie_mac_interrupts_en(t7xx_dev);
> 
> 	return 0;
>+
>+err_remove_group:
>+	sysfs_remove_group(&t7xx_dev->pdev->dev.kobj,
>+			   &t7xx_mode_attribute_group);
>+
>+err_md_exit:
>+	t7xx_md_exit(t7xx_dev);
>+	return ret;
> }
> 
> static void t7xx_pci_remove(struct pci_dev *pdev)
>@@ -747,6 +834,9 @@ static void t7xx_pci_remove(struct pci_dev *pdev)
> 	int i;
> 
> 	t7xx_dev = pci_get_drvdata(pdev);
>+
>+	sysfs_remove_group(&t7xx_dev->pdev->dev.kobj,
>+			   &t7xx_mode_attribute_group);
> 	t7xx_md_exit(t7xx_dev);
> 
> 	for (i = 0; i < EXT_INT_NUM; i++) {
>diff --git a/drivers/net/wwan/t7xx/t7xx_pci.h b/drivers/net/wwan/t7xx/t7xx_pci.h
>index f08f1ab74469..0abba7e6f8aa 100644
>--- a/drivers/net/wwan/t7xx/t7xx_pci.h
>+++ b/drivers/net/wwan/t7xx/t7xx_pci.h
>@@ -43,6 +43,16 @@ struct t7xx_addr_base {
> 
> typedef irqreturn_t (*t7xx_intr_callback)(int irq, void *param);
> 
>+enum t7xx_mode {
>+	T7XX_UNKNOWN,
>+	T7XX_READY,
>+	T7XX_RESET,
>+	T7XX_FASTBOOT_DL_SWITCHING,
>+	T7XX_FASTBOOT_DL_MODE,
>+	T7XX_FASTBOOT_DUMP_MODE,
>+	T7XX_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
>@@ -59,6 +69,7 @@ typedef irqreturn_t (*t7xx_intr_callback)(int irq, void *param);
>  * @md_pm_lock: protects PCIe sleep lock
>  * @sleep_disable_count: PCIe L1.2 lock counter
>  * @sleep_lock_acquire: indicates that sleep has been disabled
>+ * @mode: indicates the device mode
>  */
> struct t7xx_pci_dev {
> 	t7xx_intr_callback	intr_handler[EXT_INT_NUM];
>@@ -82,6 +93,7 @@ struct t7xx_pci_dev {
> #ifdef CONFIG_WWAN_DEBUGFS
> 	struct dentry		*debugfs_dir;
> #endif
>+	u32			mode;
> };
> 
> enum t7xx_pm_id {
>@@ -120,5 +132,5 @@ int t7xx_pci_pm_entity_register(struct t7xx_pci_dev *t7xx_dev, struct md_pm_enti
> int t7xx_pci_pm_entity_unregister(struct t7xx_pci_dev *t7xx_dev, struct md_pm_entity *pm_entity);
> void t7xx_pci_pm_init_late(struct t7xx_pci_dev *t7xx_dev);
> void t7xx_pci_pm_exp_detected(struct t7xx_pci_dev *t7xx_dev);
>-
>+void t7xx_mode_update(struct t7xx_pci_dev *t7xx_dev, enum t7xx_mode mode);
> #endif /* __T7XX_PCI_H__ */
>diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.c b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>index 0bc97430211b..c5d46f45fa62 100644
>--- a/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>+++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>@@ -272,6 +272,7 @@ static void fsm_routine_ready(struct t7xx_fsm_ctl *ctl)
> 
> 	ctl->curr_state = FSM_STATE_READY;
> 	t7xx_fsm_broadcast_ready_state(ctl);
>+	t7xx_mode_update(md->t7xx_dev, T7XX_READY);
> 	t7xx_md_event_notify(md, FSM_READY);
> }
> 
>-- 
>2.34.1
>
>

Powered by blists - more mailing lists