[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <764cf372-fcf3-410d-b48e-3fadc01f4707@redhat.com>
Date: Tue, 13 Aug 2024 11:50:20 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jinjian Song <jinjian.song@...ocom.com>,
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
Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
linux-doc@...r.kernel.org, angelogioacchino.delregno@...labora.com,
linux-arm-kernel@...ts.infradead.org, matthias.bgg@...il.com,
corbet@....net, linux-mediatek@...ts.infradead.org, helgaas@...nel.org,
danielwinkler@...gle.com, korneld@...gle.com
Subject: Re: [net-next v2] net: wwan: t7xx: PCIe reset rescan
On 8/9/24 05:37, Jinjian Song wrote:
> WWAN device is programmed to boot in normal mode or fastboot mode,
> when triggering a device reset through ACPI call or fastboot switch
> command.Maintain state machine synchronization and reprobe logic
^^^ ^^^
Minor nits: missing space and missing article above.
> after a device reset.
>
> Suggestion from Bjorn:
> Link: https://lore.kernel.org/all/20230127133034.GA1364550@bhelgaas/
>
> Signed-off-by: Jinjian Song <jinjian.song@...ocom.com>
> ---
> V2:
> * initialize the variable 'ret' in t7xx_reset_device() function
> ---
> drivers/net/wwan/t7xx/t7xx_modem_ops.c | 47 ++++++++++++++++---
> drivers/net/wwan/t7xx/t7xx_modem_ops.h | 9 +++-
> drivers/net/wwan/t7xx/t7xx_pci.c | 53 ++++++++++++++++++----
> drivers/net/wwan/t7xx/t7xx_pci.h | 3 ++
> drivers/net/wwan/t7xx/t7xx_port_proxy.c | 1 -
> drivers/net/wwan/t7xx/t7xx_port_trace.c | 1 +
> drivers/net/wwan/t7xx/t7xx_state_monitor.c | 34 +++++---------
> 7 files changed, 105 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/net/wwan/t7xx/t7xx_modem_ops.c b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
> index 8d864d4ed77f..79f17100f70b 100644
> --- a/drivers/net/wwan/t7xx/t7xx_modem_ops.c
> +++ b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
> @@ -53,6 +53,7 @@
>
> #define RGU_RESET_DELAY_MS 10
> #define PORT_RESET_DELAY_MS 2000
> +#define FASTBOOT_RESET_DELAY_MS 2000
> #define EX_HS_TIMEOUT_MS 5000
> #define EX_HS_POLL_DELAY_MS 10
>
> @@ -167,19 +168,52 @@ static int t7xx_acpi_reset(struct t7xx_pci_dev *t7xx_dev, char *fn_name)
> }
>
> kfree(buffer.pointer);
> +#else
> + struct device *dev = &t7xx_dev->pdev->dev;
> + int ret;
>
> + ret = pci_reset_function(t7xx_dev->pdev);
> + if (ret) {
> + dev_err(dev, "Failed to reset device, error:%d\n", ret);
> + return ret;
> + }
> #endif
> return 0;
> }
>
> -int t7xx_acpi_fldr_func(struct t7xx_pci_dev *t7xx_dev)
> +static void t7xx_host_event_notify(struct t7xx_pci_dev *t7xx_dev, unsigned int event_id)
> {
> - return t7xx_acpi_reset(t7xx_dev, "_RST");
> + u32 value;
> +
> + value = ioread32(IREG_BASE(t7xx_dev) + T7XX_PCIE_MISC_DEV_STATUS);
> + value &= ~HOST_EVENT_MASK;
> + value |= FIELD_PREP(HOST_EVENT_MASK, event_id);
> + iowrite32(value, IREG_BASE(t7xx_dev) + T7XX_PCIE_MISC_DEV_STATUS);
> }
>
> -int t7xx_acpi_pldr_func(struct t7xx_pci_dev *t7xx_dev)
> +int t7xx_reset_device(struct t7xx_pci_dev *t7xx_dev, enum reset_type type)
> {
> - return t7xx_acpi_reset(t7xx_dev, "MRST._RST");
> + int ret = 0;
> +
> + pci_save_state(t7xx_dev->pdev);
> + t7xx_pci_reprobe_early(t7xx_dev);
> + t7xx_mode_update(t7xx_dev, T7XX_RESET);
> +
> + if (type == FLDR) {
> + ret = t7xx_acpi_reset(t7xx_dev, "_RST");
> + } else if (type == PLDR) {
> + ret = t7xx_acpi_reset(t7xx_dev, "MRST._RST");
> + } else if (type == FASTBOOT) {
> + t7xx_host_event_notify(t7xx_dev, FASTBOOT_DL_NOTIFY);
> + t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_DEVICE_RESET);
> + msleep(FASTBOOT_RESET_DELAY_MS);
Did you test this with CONFIG_DEBUG_ATOMIC_SLEEP?
AFAICS the above adds a 2s (FASTBOOT_RESET_DELAY_MS) unconditional sleep
for 'fastboot' inside am IRQ thread function via:
t7xx_rgu_isr_thread() -> t7xx_reset_device_via_pmic() -> t7xx_reset_device()
Well, the existing code already looks broken with a 10 ms sleep there,
still...
> diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
> index 10a8c1080b10..2398f41046ce 100644
> --- a/drivers/net/wwan/t7xx/t7xx_pci.c
> +++ b/drivers/net/wwan/t7xx/t7xx_pci.c
> @@ -67,6 +67,7 @@ static ssize_t t7xx_mode_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> + enum t7xx_mode mode;
> struct t7xx_pci_dev *t7xx_dev;
> struct pci_dev *pdev;
> int index = 0;
Minor nit: please respect the reverse xmas tree above.
Cheers,
Paolo
Powered by blists - more mailing lists