Re: [PATCH v3 net-next 2/5] net: wwan: t7xx: Infrastructure for early port configuration

Hi Jesse,
Thank you for the feedback.

On 1/7/2023 12:01 AM, Jesse Brandeburg wrote:

On 1/6/2023 8:26 AM, m.chetan.kumar@linux.intel.com wrote:

quoted

From: M Chetan Kumar <redacted>

To support cases such as FW update or Core dump, the t7xx
device is capable of signaling the host that a special port
needs to be created before the handshake phase.

This patch adds the infrastructure required to create the
early ports which also requires a different configuration of
CLDMA queues.

nit: use imperative voice in your commit messages: no "this patch".
instead:
"Add the infrastructure..."

Sure. Will correct it.

quoted

Signed-off-by: Haijun Liu <haijun.liu@mediatek.com>
Co-developed-by: Madhusmita Sahu <redacted>
Signed-off-by: Madhusmita Sahu <redacted>
Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
Signed-off-by: Devegowda Chandrashekar 
[off-list ref]
Signed-off-by: M Chetan Kumar <redacted>
-- 
v3:
  * No Change.
v2:
  * Move recv_skb handler to cldma_queue.
  * Drop cldma_queue_type.
  * Restore prototype of t7xx_port_send_raw_skb().
  * Remove PORT_CFG_ID_INVALID check in t7xx_port_proxy_set_cfg().
  * Add space before */.
  * Drop unnecessary logs.
  * Use WARN_ON on early port.
  * Use new MISC_DEV_STATUS_INVALID instead of MISC_DEV_STATUS.
  * Use macros instead of const identifiers.
  * Change ports member type from pointer to array type.
  * Prefix LK_EVENT_XX with MISC prefix.
  * Use t7xx prefix for device_stage enums.
  * Correct log messages.
  * Don’t override pkt_size for non-download port under dedicated Queue.
  * Drop cldma_txq_rxq_ids.
  * Use macro for txq/rxq index.
  * Use warn_on for rxq_idx comparison.
  * Drop t7xx_port_proxy_get_port_by_name().
  * Replace fsm poll with read_poll_timeout().
  * Use "\n" consistently across log message.
  * Remove local var _dev prefixes in fsm_routine_start().
  * Use max_t.

...

quoted

diff --git a/drivers/net/wwan/t7xx/t7xx_reg.h 

b/drivers/net/wwan/t7xx/t7xx_reg.h
index c41d7d094c08..44352cd02460 100644

--- a/drivers/net/wwan/t7xx/t7xx_reg.h
+++ b/drivers/net/wwan/t7xx/t7xx_reg.h

@@ -102,10 +102,28 @@ enum t7xx_pm_resume_state {

  };
  #define T7XX_PCIE_MISC_DEV_STATUS        0x0d1c
-#define MISC_STAGE_MASK                GENMASK(2, 0)
-#define MISC_RESET_TYPE_PLDR            BIT(26)
  #define MISC_RESET_TYPE_FLDR            BIT(27)
-#define LINUX_STAGE                4
+#define MISC_RESET_TYPE_PLDR            BIT(26)
+#define MISC_DEV_STATUS_MASK            GENMASK(15, 0)
+#define MISC_DEV_STATUS_INVALID            GENMASK(15, 0)

I don't see any uses of this, even though it's mentioned in the commit 
message. The only reason I looked was because it was weird to have 
DEV_STATUS_MASK and STATUS_INVALID be the same values, is that correct?

Both are unused. Will drop it.

quoted

+#define MISC_LK_EVENT_MASK            GENMASK(11, 8)
+
+enum lk_event_id {
+    LK_EVENT_NORMAL = 0,
+    LK_EVENT_CREATE_PD_PORT = 1,
+    LK_EVENT_CREATE_POST_DL_PORT = 2,
+    LK_EVENT_RESET = 7,
+};
+
+#define MISC_STAGE_MASK                GENMASK(2, 0)
+
+enum t7xx_device_stage {
+    T7XX_DEV_STAGE_INIT = 0,
+    T7XX_DEV_STAGE_BROM_PRE = 1,
+    T7XX_DEV_STAGE_BROM_POST = 2,
+    T7XX_DEV_STAGE_LK = 3,
+    T7XX_DEV_STAGE_LINUX = 4,
+};
  #define T7XX_PCIE_RESOURCE_STATUS        0x0d28
  #define T7XX_PCIE_RESOURCE_STS_MSK        GENMASK(4, 0)

diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.c 

b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
index 80edb8e75a6a..76fb5d57d4d7 100644

--- a/drivers/net/wwan/t7xx/t7xx_state_monitor.c
+++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.c

@@ -206,6 +206,34 @@ static void fsm_routine_exception(struct 

t7xx_fsm_ctl *ctl, struct t7xx_fsm_comm
          fsm_finish_command(ctl, cmd, 0);
  }
+static void t7xx_lk_stage_event_handling(struct t7xx_fsm_ctl *ctl, 
unsigned int status)
+{
+    struct t7xx_modem *md = ctl->md;
+    struct cldma_ctrl *md_ctrl;
+    enum lk_event_id lk_event;
+    struct device *dev;
+
+    dev = &md->t7xx_dev->pdev->dev;
+    lk_event = FIELD_GET(MISC_LK_EVENT_MASK, status);
+    switch (lk_event) {
+    case LK_EVENT_NORMAL:
+    case LK_EVENT_RESET:
+        break;
+
+    case LK_EVENT_CREATE_PD_PORT:
+        md_ctrl = md->md_ctrl[CLDMA_ID_AP];
+        t7xx_cldma_hif_hw_init(md_ctrl);
+        t7xx_cldma_stop(md_ctrl);
+        t7xx_cldma_switch_cfg(md_ctrl, CLDMA_DEDICATED_Q_CFG);
+        t7xx_cldma_start(md_ctrl);
+        break;
+
+    default:
+        dev_err(dev, "Invalid LK event %d\n", lk_event);
+        break;
+    }
+}
+
  static int fsm_stopped_handler(struct t7xx_fsm_ctl *ctl)
  {
      ctl->curr_state = FSM_STATE_STOPPED;

@@ -317,8 +345,9 @@ static int fsm_routine_starting(struct 

t7xx_fsm_ctl *ctl)
  static void fsm_routine_start(struct t7xx_fsm_ctl *ctl, struct 
t7xx_fsm_command *cmd)
  {
      struct t7xx_modem *md = ctl->md;
-    u32 dev_status;
-    int ret;
+    struct device *dev;
+    u32 status, stage;
+    int ret = 0;
      if (!md)
          return;

@@ -329,23 +358,55 @@ static void fsm_routine_start(struct 

t7xx_fsm_ctl *ctl, struct t7xx_fsm_command
          return;
      }
+    dev = &md->t7xx_dev->pdev->dev;
      ctl->curr_state = FSM_STATE_PRE_START;
      t7xx_md_event_notify(md, FSM_PRE_START);
-    ret = read_poll_timeout(ioread32, dev_status,
-                (dev_status & MISC_STAGE_MASK) == LINUX_STAGE, 20000, 
2000000,
-                false, IREG_BASE(md->t7xx_dev) + 
T7XX_PCIE_MISC_DEV_STATUS);
+    ret = read_poll_timeout(ioread32, status,
+                ((status & MISC_STAGE_MASK) == T7XX_DEV_STAGE_LINUX) ||
+                ((status & MISC_STAGE_MASK) == T7XX_DEV_STAGE_LK), 
100000,
+                20000000, false, IREG_BASE(md->t7xx_dev) +
+                T7XX_PCIE_MISC_DEV_STATUS);
+
      if (ret) {
-        struct device *dev = &md->t7xx_dev->pdev->dev;
+        ret = -ETIMEDOUT;
+        dev_err(dev, "read poll %d\n", ret);
+        goto finish_command;
+    }
-        fsm_finish_command(ctl, cmd, -ETIMEDOUT);
-        dev_err(dev, "Invalid device status 0x%lx\n", dev_status & 
MISC_STAGE_MASK);
-        return;
+    if (status != ctl->prev_status) {
+        stage = FIELD_GET(MISC_STAGE_MASK, status);

if stage is only used down here you can declare it locally. cppcheck has 
a check that will find these for you.

Ok. Will consider it.

quoted

+        switch (stage) {
+        case T7XX_DEV_STAGE_INIT:
+        case T7XX_DEV_STAGE_BROM_PRE:
+        case T7XX_DEV_STAGE_BROM_POST:
+            dev_info(dev, "BROM_STAGE Entered\n");
+            ret = t7xx_fsm_append_cmd(ctl, FSM_CMD_START, 0);
+            break;
+
+        case T7XX_DEV_STAGE_LK:
+            dev_info(dev, "LK_STAGE Entered\n");
+            t7xx_lk_stage_event_handling(ctl, status);
+            break;
+
+        case T7XX_DEV_STAGE_LINUX:
+            dev_info(dev, "LINUX_STAGE Entered\n");
+            t7xx_cldma_hif_hw_init(md->md_ctrl[CLDMA_ID_AP]);
+            t7xx_cldma_hif_hw_init(md->md_ctrl[CLDMA_ID_MD]);
+            t7xx_mhccif_mask_clr(md->t7xx_dev, D2H_INT_PORT_ENUM |
+                         D2H_INT_ASYNC_MD_HK | D2H_INT_ASYNC_AP_HK);
+            t7xx_port_proxy_set_cfg(md, PORT_CFG_ID_NORMAL);
+            ret = fsm_routine_starting(ctl);
+            break;
+
+        default:
+            break;
+        }
+        ctl->prev_status = status;
      }
-    t7xx_cldma_hif_hw_init(md->md_ctrl[CLDMA_ID_AP]);
-    t7xx_cldma_hif_hw_init(md->md_ctrl[CLDMA_ID_MD]);
-    fsm_finish_command(ctl, cmd, fsm_routine_starting(ctl));
+finish_command:
+    fsm_finish_command(ctl, cmd, ret);
  }
  static int fsm_main_thread(void *data)

@@ -516,6 +577,7 @@ void t7xx_fsm_reset(struct t7xx_modem *md)

      fsm_flush_event_cmd_qs(ctl);
      ctl->curr_state = FSM_STATE_STOPPED;
      ctl->exp_flg = false;
+    ctl->prev_status = 0;
  }
  int t7xx_fsm_init(struct t7xx_modem *md)

diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.h 

b/drivers/net/wwan/t7xx/t7xx_state_monitor.h
index b6e76f3903c8..5e8012567ba1 100644

--- a/drivers/net/wwan/t7xx/t7xx_state_monitor.h
+++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.h

@@ -96,6 +96,7 @@ struct t7xx_fsm_ctl {

      bool            exp_flg;
      spinlock_t        notifier_lock;        /* Protects notifier 
list */
      struct list_head    notifier_list;
+    u32                     prev_status;
  };
  struct t7xx_fsm_event {

-- 
Chetan