* [net-next 1/6] net: wwan: t7xx: Infrastructure for early port configuration
[not found] <20230803021812.6126-1-songjinjian@hotmail.com>
@ 2023-08-03 2:18 ` Jinjian Song
2023-08-03 2:18 ` [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework Jinjian Song
2023-08-03 2:18 ` [net-next 3/6] net: wwan: t7xx: Implements devlink ops of firmware flashing Jinjian Song
2 siblings, 0 replies; 17+ messages in thread
From: Jinjian Song @ 2023-08-03 2:18 UTC (permalink / raw)
To: netdev
Cc: kuba, davem, johannes, ryazanov.s.a, loic.poulain, ilpo.jarvinen,
ricardo.martinez, chiranjeevi.rapolu, haijun.liu, edumazet,
pabeni, chandrashekar.devegowda, m.chetan.kumar, linuxwwan,
linuxwwan_5g, soumya.prakash.mishra, jesse.brandeburg,
danielwinkler, Jinjian Song
From: Jinjian Song <jinjian.song@fibocom.com>
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.
Adds the infrastructure required to create the early ports
which also requires a different configuration of CLDMA queues.
Base on the v5 patch version of follow series:
'net: wwan: t7xx: fw flashing & coredump support'
(https://patchwork.kernel.org/project/netdevbpf/patch/3777bb382f4b0395cb594a602c5c79dbab86c9e0.1674307425.git.m.chetan.kumar@linux.intel.com/)
Signed-off-by: Jinjian Song <jinjian.song@fibocom.com>
---
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 | 4 +-
drivers/net/wwan/t7xx/t7xx_port.h | 4 +
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_proxy.c | 86 +++++++++++++++++++---
drivers/net/wwan/t7xx/t7xx_port_proxy.h | 10 +++
drivers/net/wwan/t7xx/t7xx_port_wwan.c | 5 +-
drivers/net/wwan/t7xx/t7xx_reg.h | 22 +++++-
drivers/net/wwan/t7xx/t7xx_state_monitor.c | 86 +++++++++++++++++++---
drivers/net/wwan/t7xx/t7xx_state_monitor.h | 1 +
12 files changed, 324 insertions(+), 48 deletions(-)
create mode 100644 drivers/net/wwan/t7xx/t7xx_port_ap_msg.c
create mode 100644 drivers/net/wwan/t7xx/t7xx_port_ap_msg.h
diff --git a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
index cc70360364b7..abc41a7089fa 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
@@ -57,8 +57,6 @@
#define CHECK_Q_STOP_TIMEOUT_US 1000000
#define CHECK_Q_STOP_STEP_US 10000
-#define CLDMA_JUMBO_BUFF_SZ (63 * 1024 + sizeof(struct ccci_header))
-
static void md_cd_queue_struct_reset(struct cldma_queue *queue, struct cldma_ctrl *md_ctrl,
enum mtk_txrx tx_rx, unsigned int index)
{
@@ -161,7 +159,7 @@ static int t7xx_cldma_gpd_rx_from_q(struct cldma_queue *queue, int budget, bool
skb_reset_tail_pointer(skb);
skb_put(skb, le16_to_cpu(gpd->data_buff_len));
- ret = md_ctrl->recv_skb(queue, skb);
+ ret = queue->recv_skb(queue, skb);
/* Break processing, will try again later */
if (ret < 0)
return ret;
@@ -897,13 +895,13 @@ static void t7xx_cldma_hw_start_send(struct cldma_ctrl *md_ctrl, int qno,
/**
* t7xx_cldma_set_recv_skb() - Set the callback to handle RX packets.
- * @md_ctrl: CLDMA context structure.
+ * @queue: CLDMA queue.
* @recv_skb: Receiving skb callback.
*/
-void t7xx_cldma_set_recv_skb(struct cldma_ctrl *md_ctrl,
+void t7xx_cldma_set_recv_skb(struct cldma_queue *queue,
int (*recv_skb)(struct cldma_queue *queue, struct sk_buff *skb))
{
- md_ctrl->recv_skb = recv_skb;
+ queue->recv_skb = recv_skb;
}
/**
@@ -993,6 +991,28 @@ int t7xx_cldma_send_skb(struct cldma_ctrl *md_ctrl, int qno, struct sk_buff *skb
return ret;
}
+static void t7xx_cldma_adjust_config(struct cldma_ctrl *md_ctrl, enum cldma_cfg cfg_id)
+{
+ int qno;
+
+ for (qno = 0; qno < CLDMA_RXQ_NUM; qno++) {
+ md_ctrl->rx_ring[qno].pkt_size = CLDMA_SHARED_Q_BUFF_SZ;
+ t7xx_cldma_set_recv_skb(&md_ctrl->rxq[qno], t7xx_port_proxy_recv_skb);
+ }
+
+ md_ctrl->rx_ring[CLDMA_RXQ_NUM - 1].pkt_size = CLDMA_JUMBO_BUFF_SZ;
+
+ for (qno = 0; qno < CLDMA_TXQ_NUM; qno++)
+ md_ctrl->tx_ring[qno].pkt_size = CLDMA_SHARED_Q_BUFF_SZ;
+
+ if (cfg_id == CLDMA_DEDICATED_Q_CFG) {
+ md_ctrl->tx_ring[CLDMA_Q_IDX_DUMP].pkt_size = CLDMA_DEDICATED_Q_BUFF_SZ;
+ md_ctrl->rx_ring[CLDMA_Q_IDX_DUMP].pkt_size = CLDMA_DEDICATED_Q_BUFF_SZ;
+ t7xx_cldma_set_recv_skb(&md_ctrl->rxq[CLDMA_Q_IDX_DUMP],
+ t7xx_port_proxy_recv_skb_from_dedicated_queue);
+ }
+}
+
static int t7xx_cldma_late_init(struct cldma_ctrl *md_ctrl)
{
char dma_pool_name[32];
@@ -1018,16 +1038,9 @@ static int t7xx_cldma_late_init(struct cldma_ctrl *md_ctrl)
dev_err(md_ctrl->dev, "control TX ring init fail\n");
goto err_free_tx_ring;
}
-
- md_ctrl->tx_ring[i].pkt_size = CLDMA_MTU;
}
for (j = 0; j < CLDMA_RXQ_NUM; j++) {
- md_ctrl->rx_ring[j].pkt_size = CLDMA_MTU;
-
- if (j == CLDMA_RXQ_NUM - 1)
- md_ctrl->rx_ring[j].pkt_size = CLDMA_JUMBO_BUFF_SZ;
-
ret = t7xx_cldma_rx_ring_init(md_ctrl, &md_ctrl->rx_ring[j]);
if (ret) {
dev_err(md_ctrl->dev, "Control RX ring init fail\n");
@@ -1094,6 +1107,7 @@ int t7xx_cldma_alloc(enum cldma_id hif_id, struct t7xx_pci_dev *t7xx_dev)
{
struct device *dev = &t7xx_dev->pdev->dev;
struct cldma_ctrl *md_ctrl;
+ int qno;
md_ctrl = devm_kzalloc(dev, sizeof(*md_ctrl), GFP_KERNEL);
if (!md_ctrl)
@@ -1102,7 +1116,9 @@ int t7xx_cldma_alloc(enum cldma_id hif_id, struct t7xx_pci_dev *t7xx_dev)
md_ctrl->t7xx_dev = t7xx_dev;
md_ctrl->dev = dev;
md_ctrl->hif_id = hif_id;
- md_ctrl->recv_skb = t7xx_cldma_default_recv_skb;
+ for (qno = 0; qno < CLDMA_RXQ_NUM; qno++)
+ md_ctrl->rxq[qno].recv_skb = t7xx_cldma_default_recv_skb;
+
t7xx_hw_info_init(md_ctrl);
t7xx_dev->md->md_ctrl[hif_id] = md_ctrl;
return 0;
@@ -1332,9 +1348,10 @@ int t7xx_cldma_init(struct cldma_ctrl *md_ctrl)
return -ENOMEM;
}
-void t7xx_cldma_switch_cfg(struct cldma_ctrl *md_ctrl)
+void t7xx_cldma_switch_cfg(struct cldma_ctrl *md_ctrl, enum cldma_cfg cfg_id)
{
t7xx_cldma_late_release(md_ctrl);
+ t7xx_cldma_adjust_config(md_ctrl, cfg_id);
t7xx_cldma_late_init(md_ctrl);
}
diff --git a/drivers/net/wwan/t7xx/t7xx_hif_cldma.h b/drivers/net/wwan/t7xx/t7xx_hif_cldma.h
index 4410bac6993a..5453cfecbe19 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_cldma.h
+++ b/drivers/net/wwan/t7xx/t7xx_hif_cldma.h
@@ -31,6 +31,10 @@
#include "t7xx_cldma.h"
#include "t7xx_pci.h"
+#define CLDMA_JUMBO_BUFF_SZ (63 * 1024 + sizeof(struct ccci_header))
+#define CLDMA_SHARED_Q_BUFF_SZ 3584
+#define CLDMA_DEDICATED_Q_BUFF_SZ 2048
+
/**
* enum cldma_id - Identifiers for CLDMA HW units.
* @CLDMA_ID_MD: Modem control channel.
@@ -55,6 +59,11 @@ struct cldma_gpd {
__le16 not_used2;
};
+enum cldma_cfg {
+ CLDMA_SHARED_Q_CFG,
+ CLDMA_DEDICATED_Q_CFG,
+};
+
struct cldma_request {
struct cldma_gpd *gpd; /* Virtual address for CPU */
dma_addr_t gpd_addr; /* Physical address for DMA */
@@ -82,6 +91,7 @@ struct cldma_queue {
wait_queue_head_t req_wq; /* Only for TX */
struct workqueue_struct *worker;
struct work_struct cldma_work;
+ int (*recv_skb)(struct cldma_queue *queue, struct sk_buff *skb);
};
struct cldma_ctrl {
@@ -101,24 +111,22 @@ struct cldma_ctrl {
struct md_pm_entity *pm_entity;
struct t7xx_cldma_hw hw_info;
bool is_late_init;
- int (*recv_skb)(struct cldma_queue *queue, struct sk_buff *skb);
};
+#define CLDMA_Q_IDX_DUMP 1
#define GPD_FLAGS_HWO BIT(0)
#define GPD_FLAGS_IOC BIT(7)
#define GPD_DMAPOOL_ALIGN 16
-#define CLDMA_MTU 3584 /* 3.5kB */
-
int t7xx_cldma_alloc(enum cldma_id hif_id, struct t7xx_pci_dev *t7xx_dev);
void t7xx_cldma_hif_hw_init(struct cldma_ctrl *md_ctrl);
int t7xx_cldma_init(struct cldma_ctrl *md_ctrl);
void t7xx_cldma_exit(struct cldma_ctrl *md_ctrl);
-void t7xx_cldma_switch_cfg(struct cldma_ctrl *md_ctrl);
+void t7xx_cldma_switch_cfg(struct cldma_ctrl *md_ctrl, enum cldma_cfg cfg_id);
void t7xx_cldma_start(struct cldma_ctrl *md_ctrl);
int t7xx_cldma_stop(struct cldma_ctrl *md_ctrl);
void t7xx_cldma_reset(struct cldma_ctrl *md_ctrl);
-void t7xx_cldma_set_recv_skb(struct cldma_ctrl *md_ctrl,
+void t7xx_cldma_set_recv_skb(struct cldma_queue *queue,
int (*recv_skb)(struct cldma_queue *queue, struct sk_buff *skb));
int t7xx_cldma_send_skb(struct cldma_ctrl *md_ctrl, int qno, struct sk_buff *skb);
void t7xx_cldma_stop_all_qs(struct cldma_ctrl *md_ctrl, enum mtk_txrx tx_rx);
diff --git a/drivers/net/wwan/t7xx/t7xx_modem_ops.c b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
index 24e7d491468e..cbd65aa48721 100644
--- a/drivers/net/wwan/t7xx/t7xx_modem_ops.c
+++ b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
@@ -529,7 +529,7 @@ static void t7xx_md_hk_wq(struct work_struct *work)
/* Clear the HS2 EXIT event appended in core_reset() */
t7xx_fsm_clr_event(ctl, FSM_EVENT_MD_HS2_EXIT);
- t7xx_cldma_switch_cfg(md->md_ctrl[CLDMA_ID_MD]);
+ t7xx_cldma_switch_cfg(md->md_ctrl[CLDMA_ID_MD], CLDMA_SHARED_Q_CFG);
t7xx_cldma_start(md->md_ctrl[CLDMA_ID_MD]);
t7xx_fsm_broadcast_state(ctl, MD_STATE_WAITING_FOR_HS2);
md->core_md.handshake_ongoing = true;
@@ -544,7 +544,7 @@ static void t7xx_ap_hk_wq(struct work_struct *work)
/* Clear the HS2 EXIT event appended in t7xx_core_reset(). */
t7xx_fsm_clr_event(ctl, FSM_EVENT_AP_HS2_EXIT);
t7xx_cldma_stop(md->md_ctrl[CLDMA_ID_AP]);
- t7xx_cldma_switch_cfg(md->md_ctrl[CLDMA_ID_AP]);
+ t7xx_cldma_switch_cfg(md->md_ctrl[CLDMA_ID_AP], CLDMA_SHARED_Q_CFG);
t7xx_cldma_start(md->md_ctrl[CLDMA_ID_AP]);
md->core_ap.handshake_ongoing = true;
t7xx_core_hk_handler(md, &md->core_ap, ctl, FSM_EVENT_AP_HS2, FSM_EVENT_AP_HS2_EXIT);
diff --git a/drivers/net/wwan/t7xx/t7xx_port.h b/drivers/net/wwan/t7xx/t7xx_port.h
index 4ae8a00a8532..09acb1ef144d 100644
--- a/drivers/net/wwan/t7xx/t7xx_port.h
+++ b/drivers/net/wwan/t7xx/t7xx_port.h
@@ -75,6 +75,8 @@ enum port_ch {
PORT_CH_DSS6_TX = 0x20df,
PORT_CH_DSS7_RX = 0x20e0,
PORT_CH_DSS7_TX = 0x20e1,
+
+ PORT_CH_ID_UNIMPORTANT = 0xffff,
};
struct t7xx_port;
@@ -135,9 +137,11 @@ struct t7xx_port {
};
};
+int t7xx_get_port_mtu(struct t7xx_port *port);
struct sk_buff *t7xx_port_alloc_skb(int payload);
struct sk_buff *t7xx_ctrl_alloc_skb(int payload);
int t7xx_port_enqueue_skb(struct t7xx_port *port, struct sk_buff *skb);
+int t7xx_port_send_raw_skb(struct t7xx_port *port, struct sk_buff *skb);
int t7xx_port_send_skb(struct t7xx_port *port, struct sk_buff *skb, unsigned int pkt_header,
unsigned int ex_msg);
int t7xx_port_send_ctl_skb(struct t7xx_port *port, struct sk_buff *skb, unsigned int msg,
diff --git a/drivers/net/wwan/t7xx/t7xx_port_ap_msg.c b/drivers/net/wwan/t7xx/t7xx_port_ap_msg.c
new file mode 100644
index 000000000000..599fb09912de
--- /dev/null
+++ b/drivers/net/wwan/t7xx/t7xx_port_ap_msg.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022-2023, Intel Corporation.
+ */
+
+#include "t7xx_port.h"
+#include "t7xx_port_proxy.h"
+#include "t7xx_port_devlink.h"
+#include "t7xx_state_monitor.h"
+#include "t7xx_port_ap_msg.h"
+
+int t7xx_port_ap_msg_tx(struct t7xx_port *port, char *buff, size_t len)
+{
+ const struct t7xx_port_conf *port_conf;
+ size_t offset, chunk_len = 0, txq_mtu;
+ struct t7xx_fsm_ctl *ctl;
+ struct sk_buff *skb_ccci;
+ enum md_state md_state;
+ int ret;
+
+ if (!len || !port->chan_enable)
+ return -EINVAL;
+
+ port_conf = port->port_conf;
+ ctl = port->t7xx_dev->md->fsm_ctl;
+ md_state = t7xx_fsm_get_md_state(ctl);
+ if (md_state == MD_STATE_WAITING_FOR_HS1 || md_state == MD_STATE_WAITING_FOR_HS2) {
+ dev_warn(port->dev, "Cannot write to %s port when md_state=%d\n",
+ port_conf->name, md_state);
+ return -ENODEV;
+ }
+
+ txq_mtu = t7xx_get_port_mtu(port);
+ for (offset = 0; offset < len; offset += chunk_len) {
+ chunk_len = min(len - offset, txq_mtu - sizeof(struct ccci_header));
+ skb_ccci = t7xx_port_alloc_skb(chunk_len);
+ if (!skb_ccci)
+ return -ENOMEM;
+
+ skb_put_data(skb_ccci, buff + offset, chunk_len);
+ ret = t7xx_port_send_skb(port, skb_ccci, 0, 0);
+ if (ret) {
+ dev_kfree_skb_any(skb_ccci);
+ dev_err(port->dev, "Write error on %s port, %d\n",
+ port_conf->name, ret);
+ return ret;
+ }
+ }
+
+ return len;
+}
+
+static int t7xx_port_ap_msg_init(struct t7xx_port *port)
+{
+ struct t7xx_devlink *dl = port->t7xx_dev->dl;
+
+ port->rx_length_th = T7XX_MAX_QUEUE_LENGTH;
+ dl->status = T7XX_DEVLINK_IDLE;
+ dl->port = port;
+
+ return 0;
+}
+
+static void t7xx_port_ap_msg_uninit(struct t7xx_port *port)
+{
+ struct t7xx_devlink *dl = port->t7xx_dev->dl;
+
+ dl->mode = T7XX_NORMAL_MODE;
+ skb_queue_purge(&port->rx_skb_list);
+}
+
+struct port_ops ap_msg_port_ops = {
+ .init = &t7xx_port_ap_msg_init,
+ .recv_skb = &t7xx_port_enqueue_skb,
+ .uninit = &t7xx_port_ap_msg_uninit,
+ .enable_chl = &t7xx_port_enable_chl,
+ .disable_chl = &t7xx_port_disable_chl,
+};
diff --git a/drivers/net/wwan/t7xx/t7xx_port_ap_msg.h b/drivers/net/wwan/t7xx/t7xx_port_ap_msg.h
new file mode 100644
index 000000000000..4838d87d86cf
--- /dev/null
+++ b/drivers/net/wwan/t7xx/t7xx_port_ap_msg.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0-only
+ *
+ * Copyright (c) 2022-2023, Intel Corporation.
+ */
+
+#ifndef __T7XX_PORT_AP_MSG_H__
+#define __T7XX_PORT_AP_MSG_H__
+
+int t7xx_port_ap_msg_tx(struct t7xx_port *port, char *buff, size_t len);
+
+#endif /* __T7XX_PORT_AP_MSG_H__ */
diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
index 274846d39fbf..bdfeb10e0c51 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
@@ -100,6 +100,18 @@ static const struct t7xx_port_conf t7xx_port_conf[] = {
},
};
+static struct t7xx_port_conf t7xx_early_port_conf[] = {
+ {
+ .tx_ch = PORT_CH_ID_UNIMPORTANT,
+ .rx_ch = PORT_CH_ID_UNIMPORTANT,
+ .txq_index = CLDMA_Q_IDX_DUMP,
+ .rxq_index = CLDMA_Q_IDX_DUMP,
+ .txq_exp_index = CLDMA_Q_IDX_DUMP,
+ .rxq_exp_index = CLDMA_Q_IDX_DUMP,
+ .path_id = CLDMA_ID_AP,
+ },
+};
+
static struct t7xx_port *t7xx_proxy_get_port_by_ch(struct port_proxy *port_prox, enum port_ch ch)
{
const struct t7xx_port_conf *port_conf;
@@ -214,7 +226,17 @@ int t7xx_port_enqueue_skb(struct t7xx_port *port, struct sk_buff *skb)
return 0;
}
-static int t7xx_port_send_raw_skb(struct t7xx_port *port, struct sk_buff *skb)
+int t7xx_get_port_mtu(struct t7xx_port *port)
+{
+ enum cldma_id path_id = port->port_conf->path_id;
+ int tx_qno = t7xx_port_get_queue_no(port);
+ struct cldma_ctrl *md_ctrl;
+
+ md_ctrl = port->t7xx_dev->md->md_ctrl[path_id];
+ return md_ctrl->tx_ring[tx_qno].pkt_size;
+}
+
+int t7xx_port_send_raw_skb(struct t7xx_port *port, struct sk_buff *skb)
{
enum cldma_id path_id = port->port_conf->path_id;
struct cldma_ctrl *md_ctrl;
@@ -329,6 +351,30 @@ static void t7xx_proxy_setup_ch_mapping(struct port_proxy *port_prox)
}
}
+int t7xx_port_proxy_recv_skb_from_dedicated_queue(struct cldma_queue *queue, struct sk_buff *skb)
+{
+ struct t7xx_pci_dev *t7xx_dev = queue->md_ctrl->t7xx_dev;
+ struct port_proxy *port_prox = t7xx_dev->md->port_prox;
+ const struct t7xx_port_conf *port_conf;
+ struct t7xx_port *port;
+ int ret;
+
+ port = &port_prox->ports[0];
+ if (WARN_ON_ONCE(port->port_conf->rxq_index != queue->index)) {
+ dev_kfree_skb_any(skb);
+ return -EINVAL;
+ }
+
+ port_conf = port->port_conf;
+ ret = port_conf->ops->recv_skb(port, skb);
+ if (ret < 0 && ret != -ENOBUFS) {
+ dev_err(port->dev, "drop on RX ch %d, %d\n", port_conf->rx_ch, ret);
+ dev_kfree_skb_any(skb);
+ }
+
+ return ret;
+}
+
static struct t7xx_port *t7xx_port_proxy_find_port(struct t7xx_pci_dev *t7xx_dev,
struct cldma_queue *queue, u16 channel)
{
@@ -359,7 +405,7 @@ static struct t7xx_port *t7xx_port_proxy_find_port(struct t7xx_pci_dev *t7xx_dev
** 0 - Packet consumed.
** -ERROR - Failed to process skb.
*/
-static int t7xx_port_proxy_recv_skb(struct cldma_queue *queue, struct sk_buff *skb)
+int t7xx_port_proxy_recv_skb(struct cldma_queue *queue, struct sk_buff *skb)
{
struct ccci_header *ccci_h = (struct ccci_header *)skb->data;
struct t7xx_pci_dev *t7xx_dev = queue->md_ctrl->t7xx_dev;
@@ -451,13 +497,39 @@ static void t7xx_proxy_init_all_ports(struct t7xx_modem *md)
t7xx_proxy_setup_ch_mapping(port_prox);
}
+void t7xx_port_proxy_set_cfg(struct t7xx_modem *md, enum port_cfg_id cfg_id)
+{
+ struct port_proxy *port_prox = md->port_prox;
+ const struct t7xx_port_conf *port_conf;
+ u32 port_count;
+ int i;
+
+ t7xx_port_proxy_uninit(port_prox);
+
+ if (cfg_id == PORT_CFG_ID_EARLY) {
+ port_conf = t7xx_early_port_conf;
+ port_count = ARRAY_SIZE(t7xx_early_port_conf);
+ } else {
+ port_conf = t7xx_port_conf;
+ port_count = ARRAY_SIZE(t7xx_port_conf);
+ }
+
+ for (i = 0; i < port_count; i++)
+ port_prox->ports[i].port_conf = &port_conf[i];
+
+ port_prox->cfg_id = cfg_id;
+ port_prox->port_count = port_count;
+ t7xx_proxy_init_all_ports(md);
+}
+
static int t7xx_proxy_alloc(struct t7xx_modem *md)
{
+ unsigned int early_port_count = ARRAY_SIZE(t7xx_early_port_conf);
unsigned int port_count = ARRAY_SIZE(t7xx_port_conf);
struct device *dev = &md->t7xx_dev->pdev->dev;
struct port_proxy *port_prox;
- int i;
+ port_count = max(port_count, early_port_count);
port_prox = devm_kzalloc(dev, sizeof(*port_prox) + sizeof(struct t7xx_port) * port_count,
GFP_KERNEL);
if (!port_prox)
@@ -465,12 +537,8 @@ static int t7xx_proxy_alloc(struct t7xx_modem *md)
md->port_prox = port_prox;
port_prox->dev = dev;
+ t7xx_port_proxy_set_cfg(md, PORT_CFG_ID_EARLY);
- for (i = 0; i < port_count; i++)
- port_prox->ports[i].port_conf = &t7xx_port_conf[i];
-
- port_prox->port_count = port_count;
- t7xx_proxy_init_all_ports(md);
return 0;
}
@@ -492,8 +560,6 @@ int t7xx_port_proxy_init(struct t7xx_modem *md)
if (ret)
return ret;
- t7xx_cldma_set_recv_skb(md->md_ctrl[CLDMA_ID_AP], t7xx_port_proxy_recv_skb);
- t7xx_cldma_set_recv_skb(md->md_ctrl[CLDMA_ID_MD], t7xx_port_proxy_recv_skb);
return 0;
}
diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.h b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
index 81d059fbc0fb..7f5706811445 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.h
+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
@@ -31,11 +31,18 @@
#define RX_QUEUE_MAXLEN 32
#define CTRL_QUEUE_MAXLEN 16
+enum port_cfg_id {
+ PORT_CFG_ID_INVALID,
+ PORT_CFG_ID_NORMAL,
+ PORT_CFG_ID_EARLY,
+};
+
struct port_proxy {
int port_count;
struct list_head rx_ch_ports[PORT_CH_ID_MASK + 1];
struct list_head queue_ports[CLDMA_NUM][MTK_QUEUES];
struct device *dev;
+ enum port_cfg_id cfg_id;
struct t7xx_port ports[];
};
@@ -98,5 +105,8 @@ void t7xx_port_proxy_md_status_notify(struct port_proxy *port_prox, unsigned int
int t7xx_port_enum_msg_handler(struct t7xx_modem *md, void *msg);
int t7xx_port_proxy_chl_enable_disable(struct port_proxy *port_prox, unsigned int ch_id,
bool en_flag);
+void t7xx_port_proxy_set_cfg(struct t7xx_modem *md, enum port_cfg_id cfg_id);
+int t7xx_port_proxy_recv_skb(struct cldma_queue *queue, struct sk_buff *skb);
+int t7xx_port_proxy_recv_skb_from_dedicated_queue(struct cldma_queue *queue, struct sk_buff *skb);
#endif /* __T7XX_PORT_PROXY_H__ */
diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
index 17389c8f6600..ddc20ddfa734 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_wwan.c
+++ b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
@@ -152,14 +152,15 @@ static int t7xx_port_wwan_disable_chl(struct t7xx_port *port)
static void t7xx_port_wwan_md_state_notify(struct t7xx_port *port, unsigned int state)
{
const struct t7xx_port_conf *port_conf = port->port_conf;
- unsigned int header_len = sizeof(struct ccci_header);
+ unsigned int header_len = sizeof(struct ccci_header), mtu;
struct wwan_port_caps caps;
if (state != MD_STATE_READY)
return;
if (!port->wwan.wwan_port) {
- caps.frag_len = CLDMA_MTU - header_len;
+ mtu = t7xx_get_port_mtu(port);
+ caps.frag_len = mtu - header_len;
caps.headroom_len = header_len;
port->wwan.wwan_port = wwan_create_port(port->dev, port_conf->port_type,
&wwan_ops, &caps, port);
diff --git a/drivers/net/wwan/t7xx/t7xx_reg.h b/drivers/net/wwan/t7xx/t7xx_reg.h
index c41d7d094c08..3b665c6116fe 100644
--- a/drivers/net/wwan/t7xx/t7xx_reg.h
+++ b/drivers/net/wwan/t7xx/t7xx_reg.h
@@ -102,10 +102,26 @@ 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_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..9c51e332e7c5 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,7 +345,8 @@ 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;
+ struct device *dev;
+ u32 status;
int ret;
if (!md)
@@ -329,23 +358,57 @@ 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;
+ dev_err(dev, "read poll timeout %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 || cmd->flag != 0) {
+ u32 stage = FIELD_GET(MISC_STAGE_MASK, status);
+
+ switch (stage) {
+ case T7XX_DEV_STAGE_INIT:
+ case T7XX_DEV_STAGE_BROM_PRE:
+ case T7XX_DEV_STAGE_BROM_POST:
+ dev_dbg(dev, "BROM_STAGE Entered\n");
+ ret = t7xx_fsm_append_cmd(ctl, FSM_CMD_START, 0);
+ break;
+
+ case T7XX_DEV_STAGE_LK:
+ dev_dbg(dev, "LK_STAGE Entered\n");
+ t7xx_lk_stage_event_handling(ctl, status);
+ break;
+
+ case T7XX_DEV_STAGE_LINUX:
+ dev_dbg(dev, "LINUX_STAGE Entered\n");
+ t7xx_mhccif_mask_clr(md->t7xx_dev, D2H_INT_PORT_ENUM |
+ D2H_INT_ASYNC_MD_HK | D2H_INT_ASYNC_AP_HK);
+ if (cmd->flag == 0)
+ break;
+ t7xx_cldma_hif_hw_init(md->md_ctrl[CLDMA_ID_AP]);
+ t7xx_cldma_hif_hw_init(md->md_ctrl[CLDMA_ID_MD]);
+ 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 +579,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 {
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework
[not found] <20230803021812.6126-1-songjinjian@hotmail.com>
2023-08-03 2:18 ` [net-next 1/6] net: wwan: t7xx: Infrastructure for early port configuration Jinjian Song
@ 2023-08-03 2:18 ` Jinjian Song
2023-08-03 9:31 ` Jiri Pirko
` (3 more replies)
2023-08-03 2:18 ` [net-next 3/6] net: wwan: t7xx: Implements devlink ops of firmware flashing Jinjian Song
2 siblings, 4 replies; 17+ messages in thread
From: Jinjian Song @ 2023-08-03 2:18 UTC (permalink / raw)
To: netdev
Cc: kuba, davem, johannes, ryazanov.s.a, loic.poulain, ilpo.jarvinen,
ricardo.martinez, chiranjeevi.rapolu, haijun.liu, edumazet,
pabeni, chandrashekar.devegowda, m.chetan.kumar, linuxwwan,
linuxwwan_5g, soumya.prakash.mishra, jesse.brandeburg,
danielwinkler, Jinjian Song
From: Jinjian Song <jinjian.song@fibocom.com>
Adds support for t7xx wwan device firmware flashing 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.
Base on the v5 patch version of follow series:
'net: wwan: t7xx: fw flashing & coredump support'
(https://patchwork.kernel.org/project/netdevbpf/patch/fc8bbb0b66a5ff3a489ea9857d79b374508090ef.1674307425.git.m.chetan.kumar@linux.intel.com/)
Signed-off-by: Jinjian Song <jinjian.song@fibocom.com>
---
drivers/net/wwan/Kconfig | 1 +
drivers/net/wwan/t7xx/Makefile | 3 +-
drivers/net/wwan/t7xx/t7xx_pci.c | 15 ++-
drivers/net/wwan/t7xx/t7xx_pci.h | 2 +
drivers/net/wwan/t7xx/t7xx_port_devlink.c | 149 ++++++++++++++++++++++
drivers/net/wwan/t7xx/t7xx_port_devlink.h | 29 +++++
drivers/net/wwan/t7xx/t7xx_port_proxy.c | 20 +++
drivers/net/wwan/t7xx/t7xx_port_proxy.h | 3 +
8 files changed, 218 insertions(+), 4 deletions(-)
create mode 100644 drivers/net/wwan/t7xx/t7xx_port_devlink.c
create mode 100644 drivers/net/wwan/t7xx/t7xx_port_devlink.h
diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
index 410b0245114e..dd7a9883c1ff 100644
--- a/drivers/net/wwan/Kconfig
+++ b/drivers/net/wwan/Kconfig
@@ -108,6 +108,7 @@ config IOSM
config MTK_T7XX
tristate "MediaTek PCIe 5G WWAN modem T7xx device"
depends on PCI
+ select NET_DEVLINK
select RELAY if WWAN_DEBUGFS
help
Enables MediaTek PCIe based 5G WWAN modem (T7xx series) device.
diff --git a/drivers/net/wwan/t7xx/Makefile b/drivers/net/wwan/t7xx/Makefile
index 2652cd00504e..bb28e03eea68 100644
--- a/drivers/net/wwan/t7xx/Makefile
+++ b/drivers/net/wwan/t7xx/Makefile
@@ -15,7 +15,8 @@ mtk_t7xx-y:= t7xx_pci.o \
t7xx_hif_dpmaif_tx.o \
t7xx_hif_dpmaif_rx.o \
t7xx_dpmaif.o \
- t7xx_netdev.o
+ t7xx_netdev.o \
+ t7xx_port_devlink.o
mtk_t7xx-$(CONFIG_WWAN_DEBUGFS) += \
t7xx_port_trace.o \
diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
index 91256e005b84..831819267989 100644
--- a/drivers/net/wwan/t7xx/t7xx_pci.c
+++ b/drivers/net/wwan/t7xx/t7xx_pci.c
@@ -39,6 +39,7 @@
#include "t7xx_modem_ops.h"
#include "t7xx_pci.h"
#include "t7xx_pcie_mac.h"
+#include "t7xx_port_devlink.h"
#include "t7xx_reg.h"
#include "t7xx_state_monitor.h"
@@ -108,7 +109,7 @@ static int t7xx_pci_pm_init(struct t7xx_pci_dev *t7xx_dev)
pm_runtime_set_autosuspend_delay(&pdev->dev, PM_AUTOSUSPEND_MS);
pm_runtime_use_autosuspend(&pdev->dev);
- return t7xx_wait_pm_config(t7xx_dev);
+ return 0;
}
void t7xx_pci_pm_init_late(struct t7xx_pci_dev *t7xx_dev)
@@ -723,22 +724,30 @@ static int t7xx_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
t7xx_pci_infracfg_ao_calc(t7xx_dev);
t7xx_mhccif_init(t7xx_dev);
- ret = t7xx_md_init(t7xx_dev);
+ ret = t7xx_devlink_register(t7xx_dev);
if (ret)
return ret;
+ ret = t7xx_md_init(t7xx_dev);
+ if (ret)
+ goto err_devlink_unregister;
+
t7xx_pcie_mac_interrupts_dis(t7xx_dev);
ret = t7xx_interrupt_init(t7xx_dev);
if (ret) {
t7xx_md_exit(t7xx_dev);
- return ret;
+ goto err_devlink_unregister;
}
t7xx_pcie_mac_set_int(t7xx_dev, MHCCIF_INT);
t7xx_pcie_mac_interrupts_en(t7xx_dev);
return 0;
+
+err_devlink_unregister:
+ t7xx_devlink_unregister(t7xx_dev);
+ return ret;
}
static void t7xx_pci_remove(struct pci_dev *pdev)
diff --git a/drivers/net/wwan/t7xx/t7xx_pci.h b/drivers/net/wwan/t7xx/t7xx_pci.h
index f08f1ab74469..6777581cd540 100644
--- a/drivers/net/wwan/t7xx/t7xx_pci.h
+++ b/drivers/net/wwan/t7xx/t7xx_pci.h
@@ -59,6 +59,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
+ * @dl: devlink struct
*/
struct t7xx_pci_dev {
t7xx_intr_callback intr_handler[EXT_INT_NUM];
@@ -82,6 +83,7 @@ struct t7xx_pci_dev {
#ifdef CONFIG_WWAN_DEBUGFS
struct dentry *debugfs_dir;
#endif
+ struct t7xx_devlink *dl;
};
enum t7xx_pm_id {
diff --git a/drivers/net/wwan/t7xx/t7xx_port_devlink.c b/drivers/net/wwan/t7xx/t7xx_port_devlink.c
new file mode 100644
index 000000000000..9c09464b28ee
--- /dev/null
+++ b/drivers/net/wwan/t7xx/t7xx_port_devlink.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022-2023, Intel Corporation.
+ */
+
+#include <linux/vmalloc.h>
+
+#include "t7xx_port_proxy.h"
+#include "t7xx_port_devlink.h"
+
+static int t7xx_devlink_flash_update(struct devlink *devlink,
+ struct devlink_flash_update_params *params,
+ struct netlink_ext_ack *extack)
+{
+ return 0;
+}
+
+enum t7xx_devlink_param_id {
+ T7XX_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
+ T7XX_DEVLINK_PARAM_ID_FASTBOOT,
+};
+
+static const struct devlink_param t7xx_devlink_params[] = {
+ DEVLINK_PARAM_DRIVER(T7XX_DEVLINK_PARAM_ID_FASTBOOT,
+ "fastboot", DEVLINK_PARAM_TYPE_BOOL,
+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT),
+ NULL, NULL, NULL),
+};
+
+bool t7xx_devlink_param_get_fastboot(struct devlink *devlink)
+{
+ union devlink_param_value saved_value;
+
+ devl_param_driverinit_value_get(devlink, T7XX_DEVLINK_PARAM_ID_FASTBOOT,
+ &saved_value);
+ return saved_value.vbool;
+}
+
+static int t7xx_devlink_reload_down(struct devlink *devlink, bool netns_change,
+ enum devlink_reload_action action,
+ enum devlink_reload_limit limit,
+ struct netlink_ext_ack *extack)
+{
+ return 0;
+}
+
+static int t7xx_devlink_reload_up(struct devlink *devlink,
+ enum devlink_reload_action action,
+ enum devlink_reload_limit limit,
+ u32 *actions_performed,
+ struct netlink_ext_ack *extack)
+{
+ return 0;
+}
+
+static int t7xx_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
+ struct netlink_ext_ack *extack)
+{
+ return 0;
+}
+
+/* Call back function for devlink ops */
+static const struct devlink_ops devlink_flash_ops = {
+ .supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK,
+ .flash_update = t7xx_devlink_flash_update,
+ .reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT) |
+ BIT(DEVLINK_RELOAD_ACTION_FW_ACTIVATE),
+ .info_get = t7xx_devlink_info_get,
+ .reload_down = t7xx_devlink_reload_down,
+ .reload_up = t7xx_devlink_reload_up,
+};
+
+int t7xx_devlink_register(struct t7xx_pci_dev *t7xx_dev)
+{
+ union devlink_param_value value;
+ struct devlink *dl_ctx;
+
+ dl_ctx = devlink_alloc(&devlink_flash_ops, sizeof(struct t7xx_devlink),
+ &t7xx_dev->pdev->dev);
+ if (!dl_ctx)
+ return -ENOMEM;
+
+ t7xx_dev->dl = devlink_priv(dl_ctx);
+ t7xx_dev->dl->ctx = dl_ctx;
+ t7xx_dev->dl->t7xx_dev = t7xx_dev;
+
+ devl_lock(dl_ctx);
+ devl_params_register(dl_ctx, t7xx_devlink_params, ARRAY_SIZE(t7xx_devlink_params));
+ value.vbool = false;
+ devl_param_driverinit_value_set(dl_ctx, T7XX_DEVLINK_PARAM_ID_FASTBOOT, value);
+ devl_register(dl_ctx);
+ devl_unlock(dl_ctx);
+
+ return 0;
+}
+
+void t7xx_devlink_unregister(struct t7xx_pci_dev *t7xx_dev)
+{
+ struct devlink *dl_ctx = t7xx_dev->dl->ctx;
+
+ devl_lock(dl_ctx);
+ devl_unregister(dl_ctx);
+ devl_params_unregister(dl_ctx, t7xx_devlink_params, ARRAY_SIZE(t7xx_devlink_params));
+ devl_unlock(dl_ctx);
+ devlink_free(dl_ctx);
+}
+
+/**
+ * t7xx_devlink_init - Initialize devlink to t7xx driver
+ * @port: Pointer to port structure
+ *
+ * Returns: 0 on success and error values on failure
+ */
+static int t7xx_devlink_init(struct t7xx_port *port)
+{
+ struct t7xx_devlink *dl = port->t7xx_dev->dl;
+
+ port->rx_length_th = T7XX_MAX_QUEUE_LENGTH;
+
+ dl->mode = T7XX_NORMAL_MODE;
+ dl->status = T7XX_DEVLINK_IDLE;
+ dl->port = port;
+
+ return 0;
+}
+
+static void t7xx_devlink_uninit(struct t7xx_port *port)
+{
+ struct t7xx_devlink *dl = port->t7xx_dev->dl;
+
+ dl->mode = T7XX_NORMAL_MODE;
+
+ skb_queue_purge(&port->rx_skb_list);
+}
+
+static int t7xx_devlink_enable_chl(struct t7xx_port *port)
+{
+ t7xx_port_enable_chl(port);
+
+ return 0;
+}
+
+struct port_ops devlink_port_ops = {
+ .init = &t7xx_devlink_init,
+ .recv_skb = &t7xx_port_enqueue_skb,
+ .uninit = &t7xx_devlink_uninit,
+ .enable_chl = &t7xx_devlink_enable_chl,
+ .disable_chl = &t7xx_port_disable_chl,
+};
diff --git a/drivers/net/wwan/t7xx/t7xx_port_devlink.h b/drivers/net/wwan/t7xx/t7xx_port_devlink.h
new file mode 100644
index 000000000000..12e5a63203af
--- /dev/null
+++ b/drivers/net/wwan/t7xx/t7xx_port_devlink.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0-only
+ *
+ * Copyright (c) 2022-2023, Intel Corporation.
+ */
+
+#ifndef __T7XX_PORT_DEVLINK_H__
+#define __T7XX_PORT_DEVLINK_H__
+
+#include <net/devlink.h>
+#include <linux/string.h>
+
+#define T7XX_MAX_QUEUE_LENGTH 32
+
+#define T7XX_DEVLINK_IDLE 0
+#define T7XX_NORMAL_MODE 0
+
+struct t7xx_devlink {
+ struct t7xx_pci_dev *t7xx_dev;
+ struct t7xx_port *port;
+ struct devlink *ctx;
+ unsigned long status;
+ u8 mode;
+};
+
+bool t7xx_devlink_param_get_fastboot(struct devlink *devlink);
+int t7xx_devlink_register(struct t7xx_pci_dev *t7xx_dev);
+void t7xx_devlink_unregister(struct t7xx_pci_dev *t7xx_dev);
+
+#endif /*__T7XX_PORT_DEVLINK_H__*/
diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
index bdfeb10e0c51..f185a7fb0265 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
@@ -109,6 +109,8 @@ static struct t7xx_port_conf t7xx_early_port_conf[] = {
.txq_exp_index = CLDMA_Q_IDX_DUMP,
.rxq_exp_index = CLDMA_Q_IDX_DUMP,
.path_id = CLDMA_ID_AP,
+ .ops = &devlink_port_ops,
+ .name = "devlink",
},
};
@@ -325,6 +327,24 @@ int t7xx_port_send_skb(struct t7xx_port *port, struct sk_buff *skb, unsigned int
return t7xx_port_send_ccci_skb(port, skb, pkt_header, ex_msg);
}
+int t7xx_port_enable_chl(struct t7xx_port *port)
+{
+ spin_lock(&port->port_update_lock);
+ port->chan_enable = true;
+ spin_unlock(&port->port_update_lock);
+
+ return 0;
+}
+
+int t7xx_port_disable_chl(struct t7xx_port *port)
+{
+ spin_lock(&port->port_update_lock);
+ port->chan_enable = false;
+ spin_unlock(&port->port_update_lock);
+
+ return 0;
+}
+
static void t7xx_proxy_setup_ch_mapping(struct port_proxy *port_prox)
{
struct t7xx_port *port;
diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.h b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
index 7f5706811445..c4cd1078ee92 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.h
+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
@@ -93,6 +93,7 @@ struct ctrl_msg_header {
/* Port operations mapping */
extern struct port_ops wwan_sub_port_ops;
extern struct port_ops ctl_port_ops;
+extern struct port_ops devlink_port_ops;
#ifdef CONFIG_WWAN_DEBUGFS
extern struct port_ops t7xx_trace_port_ops;
@@ -108,5 +109,7 @@ int t7xx_port_proxy_chl_enable_disable(struct port_proxy *port_prox, unsigned in
void t7xx_port_proxy_set_cfg(struct t7xx_modem *md, enum port_cfg_id cfg_id);
int t7xx_port_proxy_recv_skb(struct cldma_queue *queue, struct sk_buff *skb);
int t7xx_port_proxy_recv_skb_from_dedicated_queue(struct cldma_queue *queue, struct sk_buff *skb);
+int t7xx_port_enable_chl(struct t7xx_port *port);
+int t7xx_port_disable_chl(struct t7xx_port *port);
#endif /* __T7XX_PORT_PROXY_H__ */
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [net-next 3/6] net: wwan: t7xx: Implements devlink ops of firmware flashing
[not found] <20230803021812.6126-1-songjinjian@hotmail.com>
2023-08-03 2:18 ` [net-next 1/6] net: wwan: t7xx: Infrastructure for early port configuration Jinjian Song
2023-08-03 2:18 ` [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework Jinjian Song
@ 2023-08-03 2:18 ` Jinjian Song
2023-08-03 9:52 ` Jiri Pirko
2023-08-05 12:05 ` Jinjian Song
2 siblings, 2 replies; 17+ messages in thread
From: Jinjian Song @ 2023-08-03 2:18 UTC (permalink / raw)
To: netdev
Cc: kuba, davem, johannes, ryazanov.s.a, loic.poulain, ilpo.jarvinen,
ricardo.martinez, chiranjeevi.rapolu, haijun.liu, edumazet,
pabeni, chandrashekar.devegowda, m.chetan.kumar, linuxwwan,
linuxwwan_5g, soumya.prakash.mishra, jesse.brandeburg,
danielwinkler, Jinjian Song
From: Jinjian Song <jinjian.song@fibocom.com>
Adds support for t7xx wwan device firmware flashing using devlink.
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.
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.
Base on the v5 patch version of follow series:
'net: wwan: t7xx: fw flashing & coredump support'
(https://patchwork.kernel.org/project/netdevbpf/patch/fc8bbb0b66a5ff3a489ea9857d79b374508090ef.1674307425.git.m.chetan.kumar@linux.intel.com/)
Signed-off-by: Jinjian Song <jinjian.song@fibocom.com>
---
drivers/net/wwan/t7xx/Makefile | 3 +-
drivers/net/wwan/t7xx/t7xx_pci.c | 2 +
drivers/net/wwan/t7xx/t7xx_port.h | 2 +
drivers/net/wwan/t7xx/t7xx_port_devlink.c | 325 ++++++++++++++++++++-
drivers/net/wwan/t7xx/t7xx_port_devlink.h | 16 +
drivers/net/wwan/t7xx/t7xx_port_proxy.c | 12 +
drivers/net/wwan/t7xx/t7xx_port_proxy.h | 1 +
drivers/net/wwan/t7xx/t7xx_port_wwan.c | 22 +-
drivers/net/wwan/t7xx/t7xx_reg.h | 6 +
drivers/net/wwan/t7xx/t7xx_state_monitor.c | 42 ++-
10 files changed, 400 insertions(+), 31 deletions(-)
diff --git a/drivers/net/wwan/t7xx/Makefile b/drivers/net/wwan/t7xx/Makefile
index bb28e03eea68..1f98e28011fd 100644
--- a/drivers/net/wwan/t7xx/Makefile
+++ b/drivers/net/wwan/t7xx/Makefile
@@ -16,7 +16,8 @@ mtk_t7xx-y:= t7xx_pci.o \
t7xx_hif_dpmaif_rx.o \
t7xx_dpmaif.o \
t7xx_netdev.o \
- t7xx_port_devlink.o
+ t7xx_port_devlink.o \
+ t7xx_port_ap_msg.o
mtk_t7xx-$(CONFIG_WWAN_DEBUGFS) += \
t7xx_port_trace.o \
diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
index 831819267989..5e9b954f39ce 100644
--- a/drivers/net/wwan/t7xx/t7xx_pci.c
+++ b/drivers/net/wwan/t7xx/t7xx_pci.c
@@ -766,6 +766,8 @@ static void t7xx_pci_remove(struct pci_dev *pdev)
}
pci_free_irq_vectors(t7xx_dev->pdev);
+
+ t7xx_devlink_unregister(t7xx_dev);
}
static const struct pci_device_id t7xx_pci_table[] = {
diff --git a/drivers/net/wwan/t7xx/t7xx_port.h b/drivers/net/wwan/t7xx/t7xx_port.h
index 09acb1ef144d..dfa7ad2a9796 100644
--- a/drivers/net/wwan/t7xx/t7xx_port.h
+++ b/drivers/net/wwan/t7xx/t7xx_port.h
@@ -42,6 +42,8 @@ enum port_ch {
/* to AP */
PORT_CH_AP_CONTROL_RX = 0x1000,
PORT_CH_AP_CONTROL_TX = 0x1001,
+ PORT_CH_AP_MSG_RX = 0x101E,
+ PORT_CH_AP_MSG_TX = 0x101F,
/* to MD */
PORT_CH_CONTROL_RX = 0x2000,
diff --git a/drivers/net/wwan/t7xx/t7xx_port_devlink.c b/drivers/net/wwan/t7xx/t7xx_port_devlink.c
index 9c09464b28ee..f10804a2c0d7 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_devlink.c
+++ b/drivers/net/wwan/t7xx/t7xx_port_devlink.c
@@ -6,12 +6,216 @@
#include <linux/vmalloc.h>
#include "t7xx_port_proxy.h"
+#include "t7xx_port_ap_msg.h"
#include "t7xx_port_devlink.h"
+static int t7xx_devlink_port_read(struct t7xx_port *port, char *buf, size_t count)
+{
+ struct sk_buff *skb;
+ int read_len;
+
+ spin_lock_irq(&port->rx_wq.lock);
+ if (skb_queue_empty(&port->rx_skb_list)) {
+ int ret = wait_event_interruptible_locked_irq(port->rx_wq,
+ !skb_queue_empty(&port->rx_skb_list));
+ if (ret == -ERESTARTSYS) {
+ spin_unlock_irq(&port->rx_wq.lock);
+ return ret;
+ }
+ }
+ skb = skb_dequeue(&port->rx_skb_list);
+ spin_unlock_irq(&port->rx_wq.lock);
+
+ read_len = min_t(size_t, count, skb->len);
+ memcpy(buf, skb->data, read_len);
+
+ if (read_len < skb->len) {
+ skb_pull(skb, read_len);
+ skb_queue_head(&port->rx_skb_list, skb);
+ } else {
+ consume_skb(skb);
+ }
+
+ return read_len;
+}
+
+static int t7xx_devlink_port_write(struct t7xx_port *port, const char *buf, size_t count)
+{
+ const struct t7xx_port_conf *port_conf = port->port_conf;
+ size_t actual = count, offset = 0;
+ int txq_mtu;
+
+ txq_mtu = t7xx_get_port_mtu(port);
+ if (txq_mtu < 0)
+ return -EINVAL;
+
+ while (actual) {
+ int len = min_t(size_t, actual, txq_mtu);
+ struct sk_buff *skb;
+ int ret;
+
+ skb = __dev_alloc_skb(len, GFP_KERNEL);
+ if (!skb)
+ return -ENOMEM;
+
+ skb_put_data(skb, buf + offset, len);
+ ret = t7xx_port_send_raw_skb(port, skb);
+ if (ret) {
+ dev_err(port->dev, "write error on %s, size: %d, ret: %d\n",
+ port_conf->name, len, ret);
+ dev_kfree_skb(skb);
+ return ret;
+ }
+
+ offset += len;
+ actual -= len;
+ }
+
+ return count;
+}
+
+static int t7xx_devlink_fb_handle_response(struct t7xx_port *port, char *data)
+{
+ char status[T7XX_FB_RESPONSE_SIZE + 1];
+ int ret = 0, index;
+
+ for (index = 0; index < T7XX_FB_RESP_COUNT; index++) {
+ int read_bytes = t7xx_devlink_port_read(port, status, T7XX_FB_RESPONSE_SIZE);
+
+ if (read_bytes < 0) {
+ dev_err(port->dev, "status read interrupted\n");
+ ret = read_bytes;
+ break;
+ }
+
+ status[read_bytes] = '\0';
+ dev_dbg(port->dev, "raw response from device: %s\n", status);
+ if (!strncmp(status, T7XX_FB_RESP_INFO, strlen(T7XX_FB_RESP_INFO))) {
+ break;
+ } else if (!strncmp(status, T7XX_FB_RESP_OKAY, strlen(T7XX_FB_RESP_OKAY))) {
+ break;
+ } else if (!strncmp(status, T7XX_FB_RESP_FAIL, strlen(T7XX_FB_RESP_FAIL))) {
+ ret = -EPROTO;
+ break;
+ } else if (!strncmp(status, T7XX_FB_RESP_DATA, strlen(T7XX_FB_RESP_DATA))) {
+ if (data)
+ snprintf(data, T7XX_FB_RESPONSE_SIZE, "%s",
+ status + strlen(T7XX_FB_RESP_DATA));
+ break;
+ }
+ }
+
+ return ret;
+}
+
+static int t7xx_devlink_fb_raw_command(char *cmd, struct t7xx_port *port, char *data)
+{
+ int ret, cmd_size = strlen(cmd);
+
+ if (cmd_size > T7XX_FB_COMMAND_SIZE) {
+ dev_err(port->dev, "command length %d is long\n", cmd_size);
+ return -EINVAL;
+ }
+
+ if (cmd_size != t7xx_devlink_port_write(port, cmd, cmd_size)) {
+ dev_err(port->dev, "raw command = %s write failed\n", cmd);
+ return -EIO;
+ }
+
+ dev_dbg(port->dev, "raw command = %s written to the device\n", cmd);
+ ret = t7xx_devlink_fb_handle_response(port, data);
+ if (ret)
+ dev_err(port->dev, "raw command = %s response FAILURE:%d\n", cmd, ret);
+
+ return ret;
+}
+
+static int t7xx_devlink_fb_download_command(struct t7xx_port *port, size_t size)
+{
+ char download_command[T7XX_FB_COMMAND_SIZE];
+
+ snprintf(download_command, sizeof(download_command), "%s:%08zx",
+ T7XX_FB_CMD_DOWNLOAD, size);
+ return t7xx_devlink_fb_raw_command(download_command, port, NULL);
+}
+
+static int t7xx_devlink_fb_download(struct t7xx_port *port, const u8 *buf, size_t size)
+{
+ int ret;
+
+ if (!size)
+ return -EINVAL;
+
+ ret = t7xx_devlink_fb_download_command(port, size);
+ if (ret)
+ return ret;
+
+ ret = t7xx_devlink_port_write(port, buf, size);
+ if (ret < 0)
+ return ret;
+
+ return t7xx_devlink_fb_handle_response(port, NULL);
+}
+
+static int t7xx_devlink_fb_flash(struct t7xx_port *port, const char *cmd)
+{
+ char flash_command[T7XX_FB_COMMAND_SIZE];
+
+ snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd);
+ return t7xx_devlink_fb_raw_command(flash_command, port, NULL);
+}
+
+static int t7xx_devlink_fb_flash_partition(struct t7xx_port *port, const char *partition,
+ const u8 *buf, size_t size)
+{
+ int ret;
+
+ ret = t7xx_devlink_fb_download(port, buf, size);
+ if (ret < 0)
+ return ret;
+
+ return t7xx_devlink_fb_flash(port, partition);
+}
+
static int t7xx_devlink_flash_update(struct devlink *devlink,
struct devlink_flash_update_params *params,
struct netlink_ext_ack *extack)
{
+ struct t7xx_devlink *dl = devlink_priv(devlink);
+ const char *component = params->component;
+ const struct firmware *fw = params->fw;
+ struct t7xx_port *port;
+ char flash_status[32];
+ int ret;
+
+ if (dl->mode != T7XX_FB_DL_MODE) {
+ dev_err(&dl->t7xx_dev->pdev->dev, "Modem is not in fastboot download mode!\n");
+ ret = -EPERM;
+ goto err_out;
+ }
+
+ if (dl->status != T7XX_DEVLINK_IDLE) {
+ dev_err(&dl->t7xx_dev->pdev->dev, "Modem is busy!\n");
+ ret = -EBUSY;
+ goto err_out;
+ }
+
+ if (!component || !fw->data) {
+ ret = -EINVAL;
+ goto err_out;
+ }
+
+ set_bit(T7XX_FLASH_STATUS, &dl->status);
+ port = dl->port;
+ dev_dbg(port->dev, "flash partition name:%s binary size:%zu\n", component, fw->size);
+ ret = t7xx_devlink_fb_flash_partition(port, component, fw->data, fw->size);
+
+ sprintf(flash_status, "%s %s", "flashing", !ret ? "success" : "failure");
+ devlink_flash_update_status_notify(devlink, flash_status, params->component, 0, 0);
+ clear_bit(T7XX_FLASH_STATUS, &dl->status);
+
+err_out:
+ return ret;
return 0;
}
@@ -41,7 +245,19 @@ static int t7xx_devlink_reload_down(struct devlink *devlink, bool netns_change,
enum devlink_reload_limit limit,
struct netlink_ext_ack *extack)
{
- return 0;
+ struct t7xx_devlink *dl = devlink_priv(devlink);
+
+ switch (action) {
+ case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
+ return 0;
+ case DEVLINK_RELOAD_ACTION_FW_ACTIVATE:
+ if (!dl->mode)
+ return -EPERM;
+ return t7xx_devlink_fb_raw_command(T7XX_FB_CMD_REBOOT, dl->port, NULL);
+ default:
+ /* Unsupported action should not get to this function */
+ return -EOPNOTSUPP;
+ }
}
static int t7xx_devlink_reload_up(struct devlink *devlink,
@@ -50,13 +266,114 @@ static int t7xx_devlink_reload_up(struct devlink *devlink,
u32 *actions_performed,
struct netlink_ext_ack *extack)
{
- return 0;
+ *actions_performed = BIT(action);
+ switch (action) {
+ case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
+ case DEVLINK_RELOAD_ACTION_FW_ACTIVATE:
+ return 0;
+ default:
+ /* Unsupported action should not get to this function */
+ return -EOPNOTSUPP;
+ }
+}
+
+static int t7xx_devlink_get_part_ver_fb_mode(struct t7xx_port *port, const char *cmd, char *data)
+{
+ char req_command[T7XX_FB_COMMAND_SIZE];
+
+ snprintf(req_command, sizeof(req_command), "%s:%s", T7XX_FB_CMD_GET_VER, cmd);
+ return t7xx_devlink_fb_raw_command(req_command, port, data);
+}
+
+static int t7xx_devlink_get_part_ver_norm_mode(struct t7xx_port *port, const char *cmd, char *data)
+{
+ char req_command[T7XX_FB_COMMAND_SIZE];
+ int len;
+
+ len = snprintf(req_command, sizeof(req_command), "%s:%s", T7XX_FB_CMD_GET_VER, cmd);
+ t7xx_port_ap_msg_tx(port, req_command, len);
+
+ return t7xx_devlink_fb_handle_response(port, data);
}
static int t7xx_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
struct netlink_ext_ack *extack)
{
- return 0;
+ struct t7xx_devlink *dl = devlink_priv(devlink);
+ char *part_name, *ver, *part_no, *data;
+ int ret, total_part, i, ver_len;
+ struct t7xx_port *port;
+
+ port = dl->port;
+ port->port_conf->ops->enable_chl(port);
+
+ if (dl->status != T7XX_DEVLINK_IDLE) {
+ dev_err(&dl->t7xx_dev->pdev->dev, "Modem is busy!\n");
+ return -EBUSY;
+ }
+
+ data = kzalloc(T7XX_FB_RESPONSE_SIZE, GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ set_bit(T7XX_GET_INFO, &dl->status);
+ if (dl->mode == T7XX_FB_DL_MODE)
+ ret = t7xx_devlink_get_part_ver_fb_mode(port, "", data);
+ else
+ ret = t7xx_devlink_get_part_ver_norm_mode(port, "", data);
+
+ if (ret < 0)
+ goto err_clear_bit;
+
+ part_no = strsep(&data, ",");
+ if (kstrtoint(part_no, 16, &total_part)) {
+ dev_err(&dl->t7xx_dev->pdev->dev, "kstrtoint error!\n");
+ ret = -EINVAL;
+ goto err_clear_bit;
+ }
+
+ for (i = 0; i < total_part; i++) {
+ part_name = strsep(&data, ",");
+ ver = strsep(&data, ",");
+ ver_len = strlen(ver);
+ if (ver[ver_len - 2] == 0x5C && ver[ver_len - 1] == 0x6E)
+ ver[ver_len - 4] = '\0';
+ ret = devlink_info_version_running_put_ext(req, part_name, ver,
+ DEVLINK_INFO_VERSION_TYPE_COMPONENT);
+ }
+
+err_clear_bit:
+ clear_bit(T7XX_GET_INFO, &dl->status);
+ kfree(data);
+ return ret;
+}
+
+struct devlink_info_req {
+ struct sk_buff *msg;
+ void (*version_cb)(const char *version_name,
+ enum devlink_info_version_type version_type,
+ void *version_cb_priv);
+ void *version_cb_priv;
+};
+
+struct devlink_flash_component_lookup_ctx {
+ const char *lookup_name;
+ bool lookup_name_found;
+};
+
+static int t7xx_devlink_info_get_loopback(struct devlink *devlink, struct devlink_info_req *req,
+ struct netlink_ext_ack *extack)
+{
+ struct devlink_flash_component_lookup_ctx *lookup_ctx = req->version_cb_priv;
+ int ret;
+
+ if (!req)
+ return t7xx_devlink_info_get(devlink, req, extack);
+
+ ret = devlink_info_version_running_put_ext(req, lookup_ctx->lookup_name,
+ "1.0", DEVLINK_INFO_VERSION_TYPE_COMPONENT);
+
+ return ret;
}
/* Call back function for devlink ops */
@@ -65,7 +382,7 @@ static const struct devlink_ops devlink_flash_ops = {
.flash_update = t7xx_devlink_flash_update,
.reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT) |
BIT(DEVLINK_RELOAD_ACTION_FW_ACTIVATE),
- .info_get = t7xx_devlink_info_get,
+ .info_get = t7xx_devlink_info_get_loopback,
.reload_down = t7xx_devlink_reload_down,
.reload_up = t7xx_devlink_reload_up,
};
diff --git a/drivers/net/wwan/t7xx/t7xx_port_devlink.h b/drivers/net/wwan/t7xx/t7xx_port_devlink.h
index 12e5a63203af..92f0993e7205 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_devlink.h
+++ b/drivers/net/wwan/t7xx/t7xx_port_devlink.h
@@ -10,9 +10,25 @@
#include <linux/string.h>
#define T7XX_MAX_QUEUE_LENGTH 32
+#define T7XX_FB_COMMAND_SIZE 64
+#define T7XX_FB_RESPONSE_SIZE 512
+#define T7XX_FB_RESP_COUNT 30
+
+#define T7XX_FLASH_STATUS 0
+#define T7XX_GET_INFO 3
#define T7XX_DEVLINK_IDLE 0
#define T7XX_NORMAL_MODE 0
+#define T7XX_FB_DL_MODE 1
+
+#define T7XX_FB_CMD_DOWNLOAD "download"
+#define T7XX_FB_CMD_FLASH "flash"
+#define T7XX_FB_CMD_REBOOT "reboot"
+#define T7XX_FB_RESP_OKAY "OKAY"
+#define T7XX_FB_RESP_FAIL "FAIL"
+#define T7XX_FB_RESP_DATA "DATA"
+#define T7XX_FB_RESP_INFO "INFO"
+#define T7XX_FB_CMD_GET_VER "get_version"
struct t7xx_devlink {
struct t7xx_pci_dev *t7xx_dev;
diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
index f185a7fb0265..9e22f751bb2e 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
@@ -40,6 +40,7 @@
#define Q_IDX_CTRL 0
#define Q_IDX_MBIM 2
#define Q_IDX_AT_CMD 5
+#define Q_IDX_AP_MSG 2
#define INVALID_SEQ_NUM GENMASK(15, 0)
@@ -97,7 +98,18 @@ static const struct t7xx_port_conf t7xx_port_conf[] = {
.path_id = CLDMA_ID_AP,
.ops = &ctl_port_ops,
.name = "t7xx_ap_ctrl",
+ }, {
+ .tx_ch = PORT_CH_AP_MSG_TX,
+ .rx_ch = PORT_CH_AP_MSG_RX,
+ .txq_index = Q_IDX_AP_MSG,
+ .rxq_index = Q_IDX_AP_MSG,
+ .txq_exp_index = Q_IDX_AP_MSG,
+ .rxq_exp_index = Q_IDX_AP_MSG,
+ .path_id = CLDMA_ID_AP,
+ .ops = &ap_msg_port_ops,
+ .name = "ap_msg",
},
+
};
static struct t7xx_port_conf t7xx_early_port_conf[] = {
diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.h b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
index c4cd1078ee92..030576a55623 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.h
+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
@@ -94,6 +94,7 @@ struct ctrl_msg_header {
extern struct port_ops wwan_sub_port_ops;
extern struct port_ops ctl_port_ops;
extern struct port_ops devlink_port_ops;
+extern struct port_ops ap_msg_port_ops;
#ifdef CONFIG_WWAN_DEBUGFS
extern struct port_ops t7xx_trace_port_ops;
diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
index ddc20ddfa734..b4e2926f33f6 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_wwan.c
+++ b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
@@ -131,24 +131,6 @@ static int t7xx_port_wwan_recv_skb(struct t7xx_port *port, struct sk_buff *skb)
return 0;
}
-static int t7xx_port_wwan_enable_chl(struct t7xx_port *port)
-{
- spin_lock(&port->port_update_lock);
- port->chan_enable = true;
- spin_unlock(&port->port_update_lock);
-
- return 0;
-}
-
-static int t7xx_port_wwan_disable_chl(struct t7xx_port *port)
-{
- spin_lock(&port->port_update_lock);
- port->chan_enable = false;
- spin_unlock(&port->port_update_lock);
-
- return 0;
-}
-
static void t7xx_port_wwan_md_state_notify(struct t7xx_port *port, unsigned int state)
{
const struct t7xx_port_conf *port_conf = port->port_conf;
@@ -173,7 +155,7 @@ struct port_ops wwan_sub_port_ops = {
.init = t7xx_port_wwan_init,
.recv_skb = t7xx_port_wwan_recv_skb,
.uninit = t7xx_port_wwan_uninit,
- .enable_chl = t7xx_port_wwan_enable_chl,
- .disable_chl = t7xx_port_wwan_disable_chl,
+ .enable_chl = t7xx_port_enable_chl,
+ .disable_chl = t7xx_port_disable_chl,
.md_state_notify = t7xx_port_wwan_md_state_notify,
};
diff --git a/drivers/net/wwan/t7xx/t7xx_reg.h b/drivers/net/wwan/t7xx/t7xx_reg.h
index 3b665c6116fe..b106c988321a 100644
--- a/drivers/net/wwan/t7xx/t7xx_reg.h
+++ b/drivers/net/wwan/t7xx/t7xx_reg.h
@@ -101,10 +101,16 @@ enum t7xx_pm_resume_state {
PM_RESUME_REG_STATE_L2_EXP,
};
+enum host_event_e {
+ HOST_EVENT_INIT = 0,
+ FASTBOOT_DL_NOTIFY = 0x3,
+};
+
#define T7XX_PCIE_MISC_DEV_STATUS 0x0d1c
#define MISC_RESET_TYPE_FLDR BIT(27)
#define MISC_RESET_TYPE_PLDR BIT(26)
#define MISC_LK_EVENT_MASK GENMASK(11, 8)
+#define HOST_EVENT_MASK GENMASK(31, 28)
enum lk_event_id {
LK_EVENT_NORMAL = 0,
diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.c b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
index 9c51e332e7c5..a6147f2324a6 100644
--- a/drivers/net/wwan/t7xx/t7xx_state_monitor.c
+++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
@@ -37,6 +37,7 @@
#include "t7xx_modem_ops.h"
#include "t7xx_pci.h"
#include "t7xx_pcie_mac.h"
+#include "t7xx_port_devlink.h"
#include "t7xx_port_proxy.h"
#include "t7xx_reg.h"
#include "t7xx_state_monitor.h"
@@ -206,11 +207,22 @@ static void fsm_routine_exception(struct t7xx_fsm_ctl *ctl, struct t7xx_fsm_comm
fsm_finish_command(ctl, cmd, 0);
}
+static void t7xx_host_event_notify(struct t7xx_modem *md, unsigned int event_id)
+{
+ u32 value;
+
+ value = ioread32(IREG_BASE(md->t7xx_dev) + T7XX_PCIE_MISC_DEV_STATUS);
+ value &= ~HOST_EVENT_MASK;
+ value |= FIELD_PREP(HOST_EVENT_MASK, event_id);
+ iowrite32(value, IREG_BASE(md->t7xx_dev) + T7XX_PCIE_MISC_DEV_STATUS);
+}
+
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 t7xx_port *port;
struct device *dev;
dev = &md->t7xx_dev->pdev->dev;
@@ -221,10 +233,19 @@ static void t7xx_lk_stage_event_handling(struct t7xx_fsm_ctl *ctl, unsigned int
break;
case LK_EVENT_CREATE_PD_PORT:
+ case LK_EVENT_CREATE_POST_DL_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);
+ port = ctl->md->t7xx_dev->dl->port;
+ if (WARN_ON(!port))
+ return;
+
+ if (lk_event == LK_EVENT_CREATE_POST_DL_PORT)
+ md->t7xx_dev->dl->mode = T7XX_FB_DL_MODE;
+
+ port->port_conf->ops->enable_chl(port);
t7xx_cldma_start(md_ctrl);
break;
@@ -258,7 +279,9 @@ static void fsm_routine_stopping(struct t7xx_fsm_ctl *ctl, struct t7xx_fsm_comma
struct cldma_ctrl *md_ctrl;
int err;
- if (ctl->curr_state == FSM_STATE_STOPPED || ctl->curr_state == FSM_STATE_STOPPING) {
+ if (ctl->curr_state == FSM_STATE_STOPPED ||
+ ctl->curr_state == FSM_STATE_STOPPING ||
+ ctl->md->rgu_irq_asserted) {
fsm_finish_command(ctl, cmd, -EINVAL);
return;
}
@@ -270,11 +293,18 @@ static void fsm_routine_stopping(struct t7xx_fsm_ctl *ctl, struct t7xx_fsm_comma
t7xx_fsm_broadcast_state(ctl, MD_STATE_WAITING_TO_STOP);
t7xx_cldma_stop(md_ctrl);
- if (!ctl->md->rgu_irq_asserted) {
- t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_DRM_DISABLE_AP);
- /* Wait for the DRM disable to take effect */
- msleep(FSM_DRM_DISABLE_DELAY_MS);
-
+ if (t7xx_devlink_param_get_fastboot(t7xx_dev->dl->ctx))
+ t7xx_host_event_notify(ctl->md, FASTBOOT_DL_NOTIFY);
+
+ t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_DRM_DISABLE_AP);
+ /* Wait for the DRM disable to take effect */
+ msleep(FSM_DRM_DISABLE_DELAY_MS);
+ if (t7xx_devlink_param_get_fastboot(t7xx_dev->dl->ctx)) {
+ /* Do not try fldr because device will always wait for
+ * MHCCIF bit 13 in fastboot download flow.
+ */
+ t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_DEVICE_RESET);
+ } else {
err = t7xx_acpi_fldr_func(t7xx_dev);
if (err)
t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_DEVICE_RESET);
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework
2023-08-03 2:18 ` [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework Jinjian Song
@ 2023-08-03 9:31 ` Jiri Pirko
2023-08-03 9:59 ` Jiri Pirko
` (2 subsequent siblings)
3 siblings, 0 replies; 17+ messages in thread
From: Jiri Pirko @ 2023-08-03 9:31 UTC (permalink / raw)
To: Jinjian Song
Cc: netdev, kuba, davem, johannes, ryazanov.s.a, loic.poulain,
ilpo.jarvinen, ricardo.martinez, chiranjeevi.rapolu, haijun.liu,
edumazet, pabeni, chandrashekar.devegowda, m.chetan.kumar,
linuxwwan, linuxwwan_5g, soumya.prakash.mishra, jesse.brandeburg,
danielwinkler, Jinjian Song
ehu, Aug 03, 2023 at 04:18:08AM CEST, songjinjian@hotmail.com wrote:
>From: Jinjian Song <jinjian.song@fibocom.com>
>
>Adds support for t7xx wwan device firmware flashing 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.
>
>Base on the v5 patch version of follow series:
>'net: wwan: t7xx: fw flashing & coredump support'
>(https://patchwork.kernel.org/project/netdevbpf/patch/fc8bbb0b66a5ff3a489ea9857d79b374508090ef.1674307425.git.m.chetan.kumar@linux.intel.com/)
>
>Signed-off-by: Jinjian Song <jinjian.song@fibocom.com>
>---
> drivers/net/wwan/Kconfig | 1 +
> drivers/net/wwan/t7xx/Makefile | 3 +-
> drivers/net/wwan/t7xx/t7xx_pci.c | 15 ++-
> drivers/net/wwan/t7xx/t7xx_pci.h | 2 +
> drivers/net/wwan/t7xx/t7xx_port_devlink.c | 149 ++++++++++++++++++++++
> drivers/net/wwan/t7xx/t7xx_port_devlink.h | 29 +++++
> drivers/net/wwan/t7xx/t7xx_port_proxy.c | 20 +++
> drivers/net/wwan/t7xx/t7xx_port_proxy.h | 3 +
> 8 files changed, 218 insertions(+), 4 deletions(-)
> create mode 100644 drivers/net/wwan/t7xx/t7xx_port_devlink.c
> create mode 100644 drivers/net/wwan/t7xx/t7xx_port_devlink.h
>
>diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
>index 410b0245114e..dd7a9883c1ff 100644
>--- a/drivers/net/wwan/Kconfig
>+++ b/drivers/net/wwan/Kconfig
>@@ -108,6 +108,7 @@ config IOSM
> config MTK_T7XX
> tristate "MediaTek PCIe 5G WWAN modem T7xx device"
> depends on PCI
>+ select NET_DEVLINK
> select RELAY if WWAN_DEBUGFS
> help
> Enables MediaTek PCIe based 5G WWAN modem (T7xx series) device.
>diff --git a/drivers/net/wwan/t7xx/Makefile b/drivers/net/wwan/t7xx/Makefile
>index 2652cd00504e..bb28e03eea68 100644
>--- a/drivers/net/wwan/t7xx/Makefile
>+++ b/drivers/net/wwan/t7xx/Makefile
>@@ -15,7 +15,8 @@ mtk_t7xx-y:= t7xx_pci.o \
> t7xx_hif_dpmaif_tx.o \
> t7xx_hif_dpmaif_rx.o \
> t7xx_dpmaif.o \
>- t7xx_netdev.o
>+ t7xx_netdev.o \
>+ t7xx_port_devlink.o
>
> mtk_t7xx-$(CONFIG_WWAN_DEBUGFS) += \
> t7xx_port_trace.o \
>diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
>index 91256e005b84..831819267989 100644
>--- a/drivers/net/wwan/t7xx/t7xx_pci.c
>+++ b/drivers/net/wwan/t7xx/t7xx_pci.c
>@@ -39,6 +39,7 @@
> #include "t7xx_modem_ops.h"
> #include "t7xx_pci.h"
> #include "t7xx_pcie_mac.h"
>+#include "t7xx_port_devlink.h"
> #include "t7xx_reg.h"
> #include "t7xx_state_monitor.h"
>
>@@ -108,7 +109,7 @@ static int t7xx_pci_pm_init(struct t7xx_pci_dev *t7xx_dev)
> pm_runtime_set_autosuspend_delay(&pdev->dev, PM_AUTOSUSPEND_MS);
> pm_runtime_use_autosuspend(&pdev->dev);
>
>- return t7xx_wait_pm_config(t7xx_dev);
>+ return 0;
> }
>
> void t7xx_pci_pm_init_late(struct t7xx_pci_dev *t7xx_dev)
>@@ -723,22 +724,30 @@ static int t7xx_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> t7xx_pci_infracfg_ao_calc(t7xx_dev);
> t7xx_mhccif_init(t7xx_dev);
>
>- ret = t7xx_md_init(t7xx_dev);
>+ ret = t7xx_devlink_register(t7xx_dev);
> if (ret)
> return ret;
>
>+ ret = t7xx_md_init(t7xx_dev);
>+ if (ret)
>+ goto err_devlink_unregister;
>+
> t7xx_pcie_mac_interrupts_dis(t7xx_dev);
>
> ret = t7xx_interrupt_init(t7xx_dev);
> if (ret) {
> t7xx_md_exit(t7xx_dev);
>- return ret;
>+ goto err_devlink_unregister;
> }
>
> t7xx_pcie_mac_set_int(t7xx_dev, MHCCIF_INT);
> t7xx_pcie_mac_interrupts_en(t7xx_dev);
>
> return 0;
>+
>+err_devlink_unregister:
>+ t7xx_devlink_unregister(t7xx_dev);
>+ return ret;
> }
>
> static void t7xx_pci_remove(struct pci_dev *pdev)
>diff --git a/drivers/net/wwan/t7xx/t7xx_pci.h b/drivers/net/wwan/t7xx/t7xx_pci.h
>index f08f1ab74469..6777581cd540 100644
>--- a/drivers/net/wwan/t7xx/t7xx_pci.h
>+++ b/drivers/net/wwan/t7xx/t7xx_pci.h
>@@ -59,6 +59,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
>+ * @dl: devlink struct
> */
> struct t7xx_pci_dev {
> t7xx_intr_callback intr_handler[EXT_INT_NUM];
>@@ -82,6 +83,7 @@ struct t7xx_pci_dev {
> #ifdef CONFIG_WWAN_DEBUGFS
> struct dentry *debugfs_dir;
> #endif
>+ struct t7xx_devlink *dl;
> };
>
> enum t7xx_pm_id {
>diff --git a/drivers/net/wwan/t7xx/t7xx_port_devlink.c b/drivers/net/wwan/t7xx/t7xx_port_devlink.c
>new file mode 100644
>index 000000000000..9c09464b28ee
>--- /dev/null
>+++ b/drivers/net/wwan/t7xx/t7xx_port_devlink.c
>@@ -0,0 +1,149 @@
>+// SPDX-License-Identifier: GPL-2.0-only
>+/*
>+ * Copyright (c) 2022-2023, Intel Corporation.
>+ */
>+
>+#include <linux/vmalloc.h>
>+
>+#include "t7xx_port_proxy.h"
>+#include "t7xx_port_devlink.h"
>+
>+static int t7xx_devlink_flash_update(struct devlink *devlink,
>+ struct devlink_flash_update_params *params,
>+ struct netlink_ext_ack *extack)
>+{
>+ return 0;
>+}
>+
>+enum t7xx_devlink_param_id {
>+ T7XX_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
>+ T7XX_DEVLINK_PARAM_ID_FASTBOOT,
>+};
>+
>+static const struct devlink_param t7xx_devlink_params[] = {
>+ DEVLINK_PARAM_DRIVER(T7XX_DEVLINK_PARAM_ID_FASTBOOT,
>+ "fastboot", DEVLINK_PARAM_TYPE_BOOL,
>+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT),
>+ NULL, NULL, NULL),
>+};
Please have the param introduction in a separate file.
Just to mention it right here, this param smells very oddly to me.
>+
>+bool t7xx_devlink_param_get_fastboot(struct devlink *devlink)
>+{
>+ union devlink_param_value saved_value;
>+
>+ devl_param_driverinit_value_get(devlink, T7XX_DEVLINK_PARAM_ID_FASTBOOT,
>+ &saved_value);
>+ return saved_value.vbool;
>+}
>+
>+static int t7xx_devlink_reload_down(struct devlink *devlink, bool netns_change,
>+ enum devlink_reload_action action,
>+ enum devlink_reload_limit limit,
>+ struct netlink_ext_ack *extack)
>+{
>+ return 0;
>+}
>+
>+static int t7xx_devlink_reload_up(struct devlink *devlink,
>+ enum devlink_reload_action action,
>+ enum devlink_reload_limit limit,
>+ u32 *actions_performed,
>+ struct netlink_ext_ack *extack)
>+{
>+ return 0;
>+}
>+
>+static int t7xx_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
>+ struct netlink_ext_ack *extack)
>+{
>+ return 0;
>+}
Don't put the stub functions here. Introduce them when you need them.
>+
>+/* Call back function for devlink ops */
>+static const struct devlink_ops devlink_flash_ops = {
>+ .supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK,
>+ .flash_update = t7xx_devlink_flash_update,
>+ .reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT) |
>+ BIT(DEVLINK_RELOAD_ACTION_FW_ACTIVATE),
>+ .info_get = t7xx_devlink_info_get,
>+ .reload_down = t7xx_devlink_reload_down,
>+ .reload_up = t7xx_devlink_reload_up,
>+};
>+
>+int t7xx_devlink_register(struct t7xx_pci_dev *t7xx_dev)
>+{
>+ union devlink_param_value value;
>+ struct devlink *dl_ctx;
>+
>+ dl_ctx = devlink_alloc(&devlink_flash_ops, sizeof(struct t7xx_devlink),
>+ &t7xx_dev->pdev->dev);
>+ if (!dl_ctx)
>+ return -ENOMEM;
>+
>+ t7xx_dev->dl = devlink_priv(dl_ctx);
>+ t7xx_dev->dl->ctx = dl_ctx;
>+ t7xx_dev->dl->t7xx_dev = t7xx_dev;
>+
>+ devl_lock(dl_ctx);
>+ devl_params_register(dl_ctx, t7xx_devlink_params, ARRAY_SIZE(t7xx_devlink_params));
>+ value.vbool = false;
>+ devl_param_driverinit_value_set(dl_ctx, T7XX_DEVLINK_PARAM_ID_FASTBOOT, value);
>+ devl_register(dl_ctx);
>+ devl_unlock(dl_ctx);
>+
>+ return 0;
>+}
>+
>+void t7xx_devlink_unregister(struct t7xx_pci_dev *t7xx_dev)
>+{
>+ struct devlink *dl_ctx = t7xx_dev->dl->ctx;
>+
>+ devl_lock(dl_ctx);
>+ devl_unregister(dl_ctx);
>+ devl_params_unregister(dl_ctx, t7xx_devlink_params, ARRAY_SIZE(t7xx_devlink_params));
>+ devl_unlock(dl_ctx);
>+ devlink_free(dl_ctx);
>+}
>+
>+/**
>+ * t7xx_devlink_init - Initialize devlink to t7xx driver
>+ * @port: Pointer to port structure
>+ *
>+ * Returns: 0 on success and error values on failure
>+ */
>+static int t7xx_devlink_init(struct t7xx_port *port)
>+{
>+ struct t7xx_devlink *dl = port->t7xx_dev->dl;
>+
>+ port->rx_length_th = T7XX_MAX_QUEUE_LENGTH;
>+
>+ dl->mode = T7XX_NORMAL_MODE;
>+ dl->status = T7XX_DEVLINK_IDLE;
? What is this supposed to mean? Looks quite wrong.
>+ dl->port = port;
>+
>+ return 0;
>+}
>+
>+static void t7xx_devlink_uninit(struct t7xx_port *port)
>+{
>+ struct t7xx_devlink *dl = port->t7xx_dev->dl;
>+
>+ dl->mode = T7XX_NORMAL_MODE;
>+
>+ skb_queue_purge(&port->rx_skb_list);
>+}
>+
>+static int t7xx_devlink_enable_chl(struct t7xx_port *port)
>+{
>+ t7xx_port_enable_chl(port);
>+
>+ return 0;
>+}
>+
>+struct port_ops devlink_port_ops = {
Could you reneame this to me less confusing? Looks like this should be
related to devlink_port, but actually it is not.
Could you please describe what exatly is the "port" entity for your
driver? What it represents?
>+ .init = &t7xx_devlink_init,
>+ .recv_skb = &t7xx_port_enqueue_skb,
>+ .uninit = &t7xx_devlink_uninit,
>+ .enable_chl = &t7xx_devlink_enable_chl,
>+ .disable_chl = &t7xx_port_disable_chl,
>+};
>diff --git a/drivers/net/wwan/t7xx/t7xx_port_devlink.h b/drivers/net/wwan/t7xx/t7xx_port_devlink.h
>new file mode 100644
>index 000000000000..12e5a63203af
>--- /dev/null
>+++ b/drivers/net/wwan/t7xx/t7xx_port_devlink.h
>@@ -0,0 +1,29 @@
>+/* SPDX-License-Identifier: GPL-2.0-only
>+ *
>+ * Copyright (c) 2022-2023, Intel Corporation.
>+ */
>+
>+#ifndef __T7XX_PORT_DEVLINK_H__
>+#define __T7XX_PORT_DEVLINK_H__
>+
>+#include <net/devlink.h>
>+#include <linux/string.h>
>+
>+#define T7XX_MAX_QUEUE_LENGTH 32
>+
>+#define T7XX_DEVLINK_IDLE 0
>+#define T7XX_NORMAL_MODE 0
>+
>+struct t7xx_devlink {
>+ struct t7xx_pci_dev *t7xx_dev;
>+ struct t7xx_port *port;
>+ struct devlink *ctx;
>+ unsigned long status;
>+ u8 mode;
>+};
>+
>+bool t7xx_devlink_param_get_fastboot(struct devlink *devlink);
>+int t7xx_devlink_register(struct t7xx_pci_dev *t7xx_dev);
>+void t7xx_devlink_unregister(struct t7xx_pci_dev *t7xx_dev);
>+
>+#endif /*__T7XX_PORT_DEVLINK_H__*/
>diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>index bdfeb10e0c51..f185a7fb0265 100644
>--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>@@ -109,6 +109,8 @@ static struct t7xx_port_conf t7xx_early_port_conf[] = {
> .txq_exp_index = CLDMA_Q_IDX_DUMP,
> .rxq_exp_index = CLDMA_Q_IDX_DUMP,
> .path_id = CLDMA_ID_AP,
>+ .ops = &devlink_port_ops,
>+ .name = "devlink",
> },
> };
>
>@@ -325,6 +327,24 @@ int t7xx_port_send_skb(struct t7xx_port *port, struct sk_buff *skb, unsigned int
> return t7xx_port_send_ccci_skb(port, skb, pkt_header, ex_msg);
> }
>
>+int t7xx_port_enable_chl(struct t7xx_port *port)
>+{
>+ spin_lock(&port->port_update_lock);
>+ port->chan_enable = true;
>+ spin_unlock(&port->port_update_lock);
>+
>+ return 0;
>+}
>+
>+int t7xx_port_disable_chl(struct t7xx_port *port)
>+{
>+ spin_lock(&port->port_update_lock);
>+ port->chan_enable = false;
>+ spin_unlock(&port->port_update_lock);
>+
>+ return 0;
>+}
>+
> static void t7xx_proxy_setup_ch_mapping(struct port_proxy *port_prox)
> {
> struct t7xx_port *port;
>diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.h b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
>index 7f5706811445..c4cd1078ee92 100644
>--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.h
>+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
>@@ -93,6 +93,7 @@ struct ctrl_msg_header {
> /* Port operations mapping */
> extern struct port_ops wwan_sub_port_ops;
> extern struct port_ops ctl_port_ops;
>+extern struct port_ops devlink_port_ops;
>
> #ifdef CONFIG_WWAN_DEBUGFS
> extern struct port_ops t7xx_trace_port_ops;
>@@ -108,5 +109,7 @@ int t7xx_port_proxy_chl_enable_disable(struct port_proxy *port_prox, unsigned in
> void t7xx_port_proxy_set_cfg(struct t7xx_modem *md, enum port_cfg_id cfg_id);
> int t7xx_port_proxy_recv_skb(struct cldma_queue *queue, struct sk_buff *skb);
> int t7xx_port_proxy_recv_skb_from_dedicated_queue(struct cldma_queue *queue, struct sk_buff *skb);
>+int t7xx_port_enable_chl(struct t7xx_port *port);
>+int t7xx_port_disable_chl(struct t7xx_port *port);
>
> #endif /* __T7XX_PORT_PROXY_H__ */
>--
>2.34.1
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next 3/6] net: wwan: t7xx: Implements devlink ops of firmware flashing
2023-08-03 2:18 ` [net-next 3/6] net: wwan: t7xx: Implements devlink ops of firmware flashing Jinjian Song
@ 2023-08-03 9:52 ` Jiri Pirko
2023-08-05 12:05 ` Jinjian Song
1 sibling, 0 replies; 17+ messages in thread
From: Jiri Pirko @ 2023-08-03 9:52 UTC (permalink / raw)
To: Jinjian Song
Cc: netdev, kuba, davem, johannes, ryazanov.s.a, loic.poulain,
ilpo.jarvinen, ricardo.martinez, chiranjeevi.rapolu, haijun.liu,
edumazet, pabeni, chandrashekar.devegowda, m.chetan.kumar,
linuxwwan, linuxwwan_5g, soumya.prakash.mishra, jesse.brandeburg,
danielwinkler, Jinjian Song
Thu, Aug 03, 2023 at 04:18:09AM CEST, songjinjian@hotmail.com wrote:
>From: Jinjian Song <jinjian.song@fibocom.com>
>
>Adds support for t7xx wwan device firmware flashing using devlink.
>
>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.
>
>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.
>
>Base on the v5 patch version of follow series:
>'net: wwan: t7xx: fw flashing & coredump support'
>(https://patchwork.kernel.org/project/netdevbpf/patch/fc8bbb0b66a5ff3a489ea9857d79b374508090ef.1674307425.git.m.chetan.kumar@linux.intel.com/)
>
>Signed-off-by: Jinjian Song <jinjian.song@fibocom.com>
Overall, this patch/patchset is very ugly and does some wrong or
questionable things that make my head hurt. Ugh.
>---
> drivers/net/wwan/t7xx/Makefile | 3 +-
> drivers/net/wwan/t7xx/t7xx_pci.c | 2 +
> drivers/net/wwan/t7xx/t7xx_port.h | 2 +
> drivers/net/wwan/t7xx/t7xx_port_devlink.c | 325 ++++++++++++++++++++-
> drivers/net/wwan/t7xx/t7xx_port_devlink.h | 16 +
> drivers/net/wwan/t7xx/t7xx_port_proxy.c | 12 +
> drivers/net/wwan/t7xx/t7xx_port_proxy.h | 1 +
> drivers/net/wwan/t7xx/t7xx_port_wwan.c | 22 +-
> drivers/net/wwan/t7xx/t7xx_reg.h | 6 +
> drivers/net/wwan/t7xx/t7xx_state_monitor.c | 42 ++-
> 10 files changed, 400 insertions(+), 31 deletions(-)
>
>diff --git a/drivers/net/wwan/t7xx/Makefile b/drivers/net/wwan/t7xx/Makefile
>index bb28e03eea68..1f98e28011fd 100644
>--- a/drivers/net/wwan/t7xx/Makefile
>+++ b/drivers/net/wwan/t7xx/Makefile
>@@ -16,7 +16,8 @@ mtk_t7xx-y:= t7xx_pci.o \
> t7xx_hif_dpmaif_rx.o \
> t7xx_dpmaif.o \
> t7xx_netdev.o \
>- t7xx_port_devlink.o
>+ t7xx_port_devlink.o \
>+ t7xx_port_ap_msg.o
>
> mtk_t7xx-$(CONFIG_WWAN_DEBUGFS) += \
> t7xx_port_trace.o \
>diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
>index 831819267989..5e9b954f39ce 100644
>--- a/drivers/net/wwan/t7xx/t7xx_pci.c
>+++ b/drivers/net/wwan/t7xx/t7xx_pci.c
>@@ -766,6 +766,8 @@ static void t7xx_pci_remove(struct pci_dev *pdev)
> }
>
> pci_free_irq_vectors(t7xx_dev->pdev);
>+
>+ t7xx_devlink_unregister(t7xx_dev);
> }
>
> static const struct pci_device_id t7xx_pci_table[] = {
>diff --git a/drivers/net/wwan/t7xx/t7xx_port.h b/drivers/net/wwan/t7xx/t7xx_port.h
>index 09acb1ef144d..dfa7ad2a9796 100644
>--- a/drivers/net/wwan/t7xx/t7xx_port.h
>+++ b/drivers/net/wwan/t7xx/t7xx_port.h
>@@ -42,6 +42,8 @@ enum port_ch {
> /* to AP */
> PORT_CH_AP_CONTROL_RX = 0x1000,
> PORT_CH_AP_CONTROL_TX = 0x1001,
>+ PORT_CH_AP_MSG_RX = 0x101E,
>+ PORT_CH_AP_MSG_TX = 0x101F,
>
> /* to MD */
> PORT_CH_CONTROL_RX = 0x2000,
>diff --git a/drivers/net/wwan/t7xx/t7xx_port_devlink.c b/drivers/net/wwan/t7xx/t7xx_port_devlink.c
>index 9c09464b28ee..f10804a2c0d7 100644
>--- a/drivers/net/wwan/t7xx/t7xx_port_devlink.c
>+++ b/drivers/net/wwan/t7xx/t7xx_port_devlink.c
>@@ -6,12 +6,216 @@
> #include <linux/vmalloc.h>
>
> #include "t7xx_port_proxy.h"
>+#include "t7xx_port_ap_msg.h"
> #include "t7xx_port_devlink.h"
>
>+static int t7xx_devlink_port_read(struct t7xx_port *port, char *buf, size_t count)
You have "devlink" in lot of the function and struct field names. Does
not make sense to me as for example this function does not have anything
to do with devlink. Could you please rename them all?
>+{
>+ struct sk_buff *skb;
>+ int read_len;
>+
>+ spin_lock_irq(&port->rx_wq.lock);
>+ if (skb_queue_empty(&port->rx_skb_list)) {
>+ int ret = wait_event_interruptible_locked_irq(port->rx_wq,
>+ !skb_queue_empty(&port->rx_skb_list));
>+ if (ret == -ERESTARTSYS) {
>+ spin_unlock_irq(&port->rx_wq.lock);
>+ return ret;
>+ }
>+ }
>+ skb = skb_dequeue(&port->rx_skb_list);
>+ spin_unlock_irq(&port->rx_wq.lock);
>+
>+ read_len = min_t(size_t, count, skb->len);
>+ memcpy(buf, skb->data, read_len);
>+
>+ if (read_len < skb->len) {
>+ skb_pull(skb, read_len);
>+ skb_queue_head(&port->rx_skb_list, skb);
>+ } else {
>+ consume_skb(skb);
>+ }
>+
>+ return read_len;
>+}
>+
>+static int t7xx_devlink_port_write(struct t7xx_port *port, const char *buf, size_t count)
>+{
>+ const struct t7xx_port_conf *port_conf = port->port_conf;
>+ size_t actual = count, offset = 0;
>+ int txq_mtu;
>+
>+ txq_mtu = t7xx_get_port_mtu(port);
>+ if (txq_mtu < 0)
>+ return -EINVAL;
>+
>+ while (actual) {
>+ int len = min_t(size_t, actual, txq_mtu);
>+ struct sk_buff *skb;
>+ int ret;
>+
>+ skb = __dev_alloc_skb(len, GFP_KERNEL);
>+ if (!skb)
>+ return -ENOMEM;
>+
>+ skb_put_data(skb, buf + offset, len);
>+ ret = t7xx_port_send_raw_skb(port, skb);
>+ if (ret) {
>+ dev_err(port->dev, "write error on %s, size: %d, ret: %d\n",
>+ port_conf->name, len, ret);
>+ dev_kfree_skb(skb);
>+ return ret;
>+ }
>+
>+ offset += len;
>+ actual -= len;
>+ }
>+
>+ return count;
>+}
>+
>+static int t7xx_devlink_fb_handle_response(struct t7xx_port *port, char *data)
>+{
>+ char status[T7XX_FB_RESPONSE_SIZE + 1];
>+ int ret = 0, index;
>+
>+ for (index = 0; index < T7XX_FB_RESP_COUNT; index++) {
>+ int read_bytes = t7xx_devlink_port_read(port, status, T7XX_FB_RESPONSE_SIZE);
>+
>+ if (read_bytes < 0) {
>+ dev_err(port->dev, "status read interrupted\n");
>+ ret = read_bytes;
>+ break;
>+ }
>+
>+ status[read_bytes] = '\0';
>+ dev_dbg(port->dev, "raw response from device: %s\n", status);
>+ if (!strncmp(status, T7XX_FB_RESP_INFO, strlen(T7XX_FB_RESP_INFO))) {
>+ break;
>+ } else if (!strncmp(status, T7XX_FB_RESP_OKAY, strlen(T7XX_FB_RESP_OKAY))) {
>+ break;
>+ } else if (!strncmp(status, T7XX_FB_RESP_FAIL, strlen(T7XX_FB_RESP_FAIL))) {
>+ ret = -EPROTO;
>+ break;
>+ } else if (!strncmp(status, T7XX_FB_RESP_DATA, strlen(T7XX_FB_RESP_DATA))) {
>+ if (data)
>+ snprintf(data, T7XX_FB_RESPONSE_SIZE, "%s",
>+ status + strlen(T7XX_FB_RESP_DATA));
>+ break;
>+ }
>+ }
>+
>+ return ret;
>+}
>+
>+static int t7xx_devlink_fb_raw_command(char *cmd, struct t7xx_port *port, char *data)
>+{
>+ int ret, cmd_size = strlen(cmd);
>+
>+ if (cmd_size > T7XX_FB_COMMAND_SIZE) {
>+ dev_err(port->dev, "command length %d is long\n", cmd_size);
>+ return -EINVAL;
>+ }
>+
>+ if (cmd_size != t7xx_devlink_port_write(port, cmd, cmd_size)) {
>+ dev_err(port->dev, "raw command = %s write failed\n", cmd);
>+ return -EIO;
>+ }
>+
>+ dev_dbg(port->dev, "raw command = %s written to the device\n", cmd);
>+ ret = t7xx_devlink_fb_handle_response(port, data);
>+ if (ret)
>+ dev_err(port->dev, "raw command = %s response FAILURE:%d\n", cmd, ret);
>+
>+ return ret;
>+}
>+
>+static int t7xx_devlink_fb_download_command(struct t7xx_port *port, size_t size)
>+{
>+ char download_command[T7XX_FB_COMMAND_SIZE];
>+
>+ snprintf(download_command, sizeof(download_command), "%s:%08zx",
>+ T7XX_FB_CMD_DOWNLOAD, size);
>+ return t7xx_devlink_fb_raw_command(download_command, port, NULL);
>+}
>+
>+static int t7xx_devlink_fb_download(struct t7xx_port *port, const u8 *buf, size_t size)
>+{
>+ int ret;
>+
>+ if (!size)
>+ return -EINVAL;
>+
>+ ret = t7xx_devlink_fb_download_command(port, size);
>+ if (ret)
>+ return ret;
>+
>+ ret = t7xx_devlink_port_write(port, buf, size);
>+ if (ret < 0)
>+ return ret;
>+
>+ return t7xx_devlink_fb_handle_response(port, NULL);
>+}
>+
>+static int t7xx_devlink_fb_flash(struct t7xx_port *port, const char *cmd)
>+{
>+ char flash_command[T7XX_FB_COMMAND_SIZE];
>+
>+ snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd);
>+ return t7xx_devlink_fb_raw_command(flash_command, port, NULL);
>+}
>+
>+static int t7xx_devlink_fb_flash_partition(struct t7xx_port *port, const char *partition,
>+ const u8 *buf, size_t size)
>+{
>+ int ret;
>+
>+ ret = t7xx_devlink_fb_download(port, buf, size);
>+ if (ret < 0)
>+ return ret;
>+
>+ return t7xx_devlink_fb_flash(port, partition);
>+}
>+
> static int t7xx_devlink_flash_update(struct devlink *devlink,
> struct devlink_flash_update_params *params,
> struct netlink_ext_ack *extack)
> {
>+ struct t7xx_devlink *dl = devlink_priv(devlink);
>+ const char *component = params->component;
>+ const struct firmware *fw = params->fw;
>+ struct t7xx_port *port;
>+ char flash_status[32];
>+ int ret;
>+
>+ if (dl->mode != T7XX_FB_DL_MODE) {
>+ dev_err(&dl->t7xx_dev->pdev->dev, "Modem is not in fastboot download mode!\n");
>+ ret = -EPERM;
>+ goto err_out;
>+ }
>+
>+ if (dl->status != T7XX_DEVLINK_IDLE) {
>+ dev_err(&dl->t7xx_dev->pdev->dev, "Modem is busy!\n");
>+ ret = -EBUSY;
>+ goto err_out;
>+ }
>+
>+ if (!component || !fw->data) {
>+ ret = -EINVAL;
>+ goto err_out;
>+ }
>+
>+ set_bit(T7XX_FLASH_STATUS, &dl->status);
>+ port = dl->port;
>+ dev_dbg(port->dev, "flash partition name:%s binary size:%zu\n", component, fw->size);
>+ ret = t7xx_devlink_fb_flash_partition(port, component, fw->data, fw->size);
>+
>+ sprintf(flash_status, "%s %s", "flashing", !ret ? "success" : "failure");
Don't put return status in to the flash_status. Function returns error
value which is propagated to the user.
In fact, in your case, usage of devlink_flash_update_status_notify()
does not make much sense as you don't have multiple flash stages.
>+ devlink_flash_update_status_notify(devlink, flash_status, params->component, 0, 0);
>+ clear_bit(T7XX_FLASH_STATUS, &dl->status);
>+
>+err_out:
>+ return ret;
> return 0;
> }
>
>@@ -41,7 +245,19 @@ static int t7xx_devlink_reload_down(struct devlink *devlink, bool netns_change,
> enum devlink_reload_limit limit,
> struct netlink_ext_ack *extack)
> {
>- return 0;
>+ struct t7xx_devlink *dl = devlink_priv(devlink);
>+
>+ switch (action) {
>+ case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
>+ return 0;
>+ case DEVLINK_RELOAD_ACTION_FW_ACTIVATE:
>+ if (!dl->mode)
>+ return -EPERM;
>+ return t7xx_devlink_fb_raw_command(T7XX_FB_CMD_REBOOT, dl->port, NULL);
>+ default:
>+ /* Unsupported action should not get to this function */
>+ return -EOPNOTSUPP;
>+ }
> }
>
> static int t7xx_devlink_reload_up(struct devlink *devlink,
>@@ -50,13 +266,114 @@ static int t7xx_devlink_reload_up(struct devlink *devlink,
> u32 *actions_performed,
> struct netlink_ext_ack *extack)
> {
>- return 0;
>+ *actions_performed = BIT(action);
>+ switch (action) {
>+ case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
Your driver reinit does not do anything. Please remove it from supported
actions.
>+ case DEVLINK_RELOAD_ACTION_FW_ACTIVATE:
>+ return 0;
>+ default:
>+ /* Unsupported action should not get to this function */
>+ return -EOPNOTSUPP;
>+ }
>+}
>+
>+static int t7xx_devlink_get_part_ver_fb_mode(struct t7xx_port *port, const char *cmd, char *data)
>+{
>+ char req_command[T7XX_FB_COMMAND_SIZE];
>+
>+ snprintf(req_command, sizeof(req_command), "%s:%s", T7XX_FB_CMD_GET_VER, cmd);
>+ return t7xx_devlink_fb_raw_command(req_command, port, data);
>+}
>+
>+static int t7xx_devlink_get_part_ver_norm_mode(struct t7xx_port *port, const char *cmd, char *data)
>+{
>+ char req_command[T7XX_FB_COMMAND_SIZE];
>+ int len;
>+
>+ len = snprintf(req_command, sizeof(req_command), "%s:%s", T7XX_FB_CMD_GET_VER, cmd);
>+ t7xx_port_ap_msg_tx(port, req_command, len);
>+
>+ return t7xx_devlink_fb_handle_response(port, data);
> }
>
> static int t7xx_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
> struct netlink_ext_ack *extack)
> {
>- return 0;
>+ struct t7xx_devlink *dl = devlink_priv(devlink);
>+ char *part_name, *ver, *part_no, *data;
>+ int ret, total_part, i, ver_len;
>+ struct t7xx_port *port;
>+
>+ port = dl->port;
>+ port->port_conf->ops->enable_chl(port);
>+
>+ if (dl->status != T7XX_DEVLINK_IDLE) {
>+ dev_err(&dl->t7xx_dev->pdev->dev, "Modem is busy!\n");
>+ return -EBUSY;
>+ }
>+
>+ data = kzalloc(T7XX_FB_RESPONSE_SIZE, GFP_KERNEL);
>+ if (!data)
>+ return -ENOMEM;
>+
>+ set_bit(T7XX_GET_INFO, &dl->status);
>+ if (dl->mode == T7XX_FB_DL_MODE)
>+ ret = t7xx_devlink_get_part_ver_fb_mode(port, "", data);
>+ else
>+ ret = t7xx_devlink_get_part_ver_norm_mode(port, "", data);
>+
>+ if (ret < 0)
>+ goto err_clear_bit;
>+
>+ part_no = strsep(&data, ",");
>+ if (kstrtoint(part_no, 16, &total_part)) {
>+ dev_err(&dl->t7xx_dev->pdev->dev, "kstrtoint error!\n");
>+ ret = -EINVAL;
>+ goto err_clear_bit;
>+ }
>+
>+ for (i = 0; i < total_part; i++) {
>+ part_name = strsep(&data, ",");
>+ ver = strsep(&data, ",");
>+ ver_len = strlen(ver);
>+ if (ver[ver_len - 2] == 0x5C && ver[ver_len - 1] == 0x6E)
>+ ver[ver_len - 4] = '\0';
>+ ret = devlink_info_version_running_put_ext(req, part_name, ver,
>+ DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>+ }
>+
>+err_clear_bit:
>+ clear_bit(T7XX_GET_INFO, &dl->status);
>+ kfree(data);
>+ return ret;
>+}
>+
>+struct devlink_info_req {
>+ struct sk_buff *msg;
>+ void (*version_cb)(const char *version_name,
>+ enum devlink_info_version_type version_type,
>+ void *version_cb_priv);
>+ void *version_cb_priv;
>+};
Ah! No. Remove this. If you need to touch internal of the struct, this
is definitelly not the way to do it.
>+
>+struct devlink_flash_component_lookup_ctx {
>+ const char *lookup_name;
>+ bool lookup_name_found;
>+};
Same here.
>+
>+static int t7xx_devlink_info_get_loopback(struct devlink *devlink, struct devlink_info_req *req,
>+ struct netlink_ext_ack *extack)
>+{
>+ struct devlink_flash_component_lookup_ctx *lookup_ctx = req->version_cb_priv;
>+ int ret;
>+
>+ if (!req)
>+ return t7xx_devlink_info_get(devlink, req, extack);
>+
>+ ret = devlink_info_version_running_put_ext(req, lookup_ctx->lookup_name,
It actually took me a while why you are doing this. You try to overcome
the limitation for drivers to expose version for all components that are
valid for flashing. That is not nice
Please don't do things like this!
Expose the versions for all valid components, or don't flash them.
>+ "1.0", DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>+
>+ return ret;
> }
>
> /* Call back function for devlink ops */
>@@ -65,7 +382,7 @@ static const struct devlink_ops devlink_flash_ops = {
> .flash_update = t7xx_devlink_flash_update,
> .reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT) |
> BIT(DEVLINK_RELOAD_ACTION_FW_ACTIVATE),
>- .info_get = t7xx_devlink_info_get,
>+ .info_get = t7xx_devlink_info_get_loopback,
> .reload_down = t7xx_devlink_reload_down,
> .reload_up = t7xx_devlink_reload_up,
> };
>diff --git a/drivers/net/wwan/t7xx/t7xx_port_devlink.h b/drivers/net/wwan/t7xx/t7xx_port_devlink.h
>index 12e5a63203af..92f0993e7205 100644
>--- a/drivers/net/wwan/t7xx/t7xx_port_devlink.h
>+++ b/drivers/net/wwan/t7xx/t7xx_port_devlink.h
>@@ -10,9 +10,25 @@
> #include <linux/string.h>
>
> #define T7XX_MAX_QUEUE_LENGTH 32
>+#define T7XX_FB_COMMAND_SIZE 64
>+#define T7XX_FB_RESPONSE_SIZE 512
>+#define T7XX_FB_RESP_COUNT 30
>+
>+#define T7XX_FLASH_STATUS 0
>+#define T7XX_GET_INFO 3
>
> #define T7XX_DEVLINK_IDLE 0
> #define T7XX_NORMAL_MODE 0
>+#define T7XX_FB_DL_MODE 1
>+
>+#define T7XX_FB_CMD_DOWNLOAD "download"
>+#define T7XX_FB_CMD_FLASH "flash"
>+#define T7XX_FB_CMD_REBOOT "reboot"
>+#define T7XX_FB_RESP_OKAY "OKAY"
>+#define T7XX_FB_RESP_FAIL "FAIL"
>+#define T7XX_FB_RESP_DATA "DATA"
>+#define T7XX_FB_RESP_INFO "INFO"
>+#define T7XX_FB_CMD_GET_VER "get_version"
>
> struct t7xx_devlink {
> struct t7xx_pci_dev *t7xx_dev;
>diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>index f185a7fb0265..9e22f751bb2e 100644
>--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>@@ -40,6 +40,7 @@
> #define Q_IDX_CTRL 0
> #define Q_IDX_MBIM 2
> #define Q_IDX_AT_CMD 5
>+#define Q_IDX_AP_MSG 2
>
> #define INVALID_SEQ_NUM GENMASK(15, 0)
>
>@@ -97,7 +98,18 @@ static const struct t7xx_port_conf t7xx_port_conf[] = {
> .path_id = CLDMA_ID_AP,
> .ops = &ctl_port_ops,
> .name = "t7xx_ap_ctrl",
>+ }, {
>+ .tx_ch = PORT_CH_AP_MSG_TX,
>+ .rx_ch = PORT_CH_AP_MSG_RX,
>+ .txq_index = Q_IDX_AP_MSG,
>+ .rxq_index = Q_IDX_AP_MSG,
>+ .txq_exp_index = Q_IDX_AP_MSG,
>+ .rxq_exp_index = Q_IDX_AP_MSG,
>+ .path_id = CLDMA_ID_AP,
>+ .ops = &ap_msg_port_ops,
>+ .name = "ap_msg",
> },
>+
> };
>
> static struct t7xx_port_conf t7xx_early_port_conf[] = {
>diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.h b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
>index c4cd1078ee92..030576a55623 100644
>--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.h
>+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
>@@ -94,6 +94,7 @@ struct ctrl_msg_header {
> extern struct port_ops wwan_sub_port_ops;
> extern struct port_ops ctl_port_ops;
> extern struct port_ops devlink_port_ops;
>+extern struct port_ops ap_msg_port_ops;
>
> #ifdef CONFIG_WWAN_DEBUGFS
> extern struct port_ops t7xx_trace_port_ops;
>diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
>index ddc20ddfa734..b4e2926f33f6 100644
>--- a/drivers/net/wwan/t7xx/t7xx_port_wwan.c
>+++ b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
>@@ -131,24 +131,6 @@ static int t7xx_port_wwan_recv_skb(struct t7xx_port *port, struct sk_buff *skb)
> return 0;
> }
>
>-static int t7xx_port_wwan_enable_chl(struct t7xx_port *port)
>-{
>- spin_lock(&port->port_update_lock);
>- port->chan_enable = true;
>- spin_unlock(&port->port_update_lock);
>-
>- return 0;
>-}
>-
>-static int t7xx_port_wwan_disable_chl(struct t7xx_port *port)
>-{
>- spin_lock(&port->port_update_lock);
>- port->chan_enable = false;
>- spin_unlock(&port->port_update_lock);
>-
>- return 0;
>-}
>-
> static void t7xx_port_wwan_md_state_notify(struct t7xx_port *port, unsigned int state)
> {
> const struct t7xx_port_conf *port_conf = port->port_conf;
>@@ -173,7 +155,7 @@ struct port_ops wwan_sub_port_ops = {
> .init = t7xx_port_wwan_init,
> .recv_skb = t7xx_port_wwan_recv_skb,
> .uninit = t7xx_port_wwan_uninit,
>- .enable_chl = t7xx_port_wwan_enable_chl,
>- .disable_chl = t7xx_port_wwan_disable_chl,
>+ .enable_chl = t7xx_port_enable_chl,
>+ .disable_chl = t7xx_port_disable_chl,
> .md_state_notify = t7xx_port_wwan_md_state_notify,
> };
>diff --git a/drivers/net/wwan/t7xx/t7xx_reg.h b/drivers/net/wwan/t7xx/t7xx_reg.h
>index 3b665c6116fe..b106c988321a 100644
>--- a/drivers/net/wwan/t7xx/t7xx_reg.h
>+++ b/drivers/net/wwan/t7xx/t7xx_reg.h
>@@ -101,10 +101,16 @@ enum t7xx_pm_resume_state {
> PM_RESUME_REG_STATE_L2_EXP,
> };
>
>+enum host_event_e {
>+ HOST_EVENT_INIT = 0,
>+ FASTBOOT_DL_NOTIFY = 0x3,
>+};
>+
> #define T7XX_PCIE_MISC_DEV_STATUS 0x0d1c
> #define MISC_RESET_TYPE_FLDR BIT(27)
> #define MISC_RESET_TYPE_PLDR BIT(26)
> #define MISC_LK_EVENT_MASK GENMASK(11, 8)
>+#define HOST_EVENT_MASK GENMASK(31, 28)
>
> enum lk_event_id {
> LK_EVENT_NORMAL = 0,
>diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.c b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>index 9c51e332e7c5..a6147f2324a6 100644
>--- a/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>+++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>@@ -37,6 +37,7 @@
> #include "t7xx_modem_ops.h"
> #include "t7xx_pci.h"
> #include "t7xx_pcie_mac.h"
>+#include "t7xx_port_devlink.h"
> #include "t7xx_port_proxy.h"
> #include "t7xx_reg.h"
> #include "t7xx_state_monitor.h"
>@@ -206,11 +207,22 @@ static void fsm_routine_exception(struct t7xx_fsm_ctl *ctl, struct t7xx_fsm_comm
> fsm_finish_command(ctl, cmd, 0);
> }
>
>+static void t7xx_host_event_notify(struct t7xx_modem *md, unsigned int event_id)
>+{
>+ u32 value;
>+
>+ value = ioread32(IREG_BASE(md->t7xx_dev) + T7XX_PCIE_MISC_DEV_STATUS);
>+ value &= ~HOST_EVENT_MASK;
>+ value |= FIELD_PREP(HOST_EVENT_MASK, event_id);
>+ iowrite32(value, IREG_BASE(md->t7xx_dev) + T7XX_PCIE_MISC_DEV_STATUS);
>+}
>+
> 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 t7xx_port *port;
> struct device *dev;
>
> dev = &md->t7xx_dev->pdev->dev;
>@@ -221,10 +233,19 @@ static void t7xx_lk_stage_event_handling(struct t7xx_fsm_ctl *ctl, unsigned int
> break;
>
> case LK_EVENT_CREATE_PD_PORT:
>+ case LK_EVENT_CREATE_POST_DL_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);
>+ port = ctl->md->t7xx_dev->dl->port;
>+ if (WARN_ON(!port))
>+ return;
>+
>+ if (lk_event == LK_EVENT_CREATE_POST_DL_PORT)
>+ md->t7xx_dev->dl->mode = T7XX_FB_DL_MODE;
>+
>+ port->port_conf->ops->enable_chl(port);
> t7xx_cldma_start(md_ctrl);
> break;
>
>@@ -258,7 +279,9 @@ static void fsm_routine_stopping(struct t7xx_fsm_ctl *ctl, struct t7xx_fsm_comma
> struct cldma_ctrl *md_ctrl;
> int err;
>
>- if (ctl->curr_state == FSM_STATE_STOPPED || ctl->curr_state == FSM_STATE_STOPPING) {
>+ if (ctl->curr_state == FSM_STATE_STOPPED ||
>+ ctl->curr_state == FSM_STATE_STOPPING ||
>+ ctl->md->rgu_irq_asserted) {
> fsm_finish_command(ctl, cmd, -EINVAL);
> return;
> }
>@@ -270,11 +293,18 @@ static void fsm_routine_stopping(struct t7xx_fsm_ctl *ctl, struct t7xx_fsm_comma
> t7xx_fsm_broadcast_state(ctl, MD_STATE_WAITING_TO_STOP);
> t7xx_cldma_stop(md_ctrl);
>
>- if (!ctl->md->rgu_irq_asserted) {
>- t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_DRM_DISABLE_AP);
>- /* Wait for the DRM disable to take effect */
>- msleep(FSM_DRM_DISABLE_DELAY_MS);
>-
>+ if (t7xx_devlink_param_get_fastboot(t7xx_dev->dl->ctx))
>+ t7xx_host_event_notify(ctl->md, FASTBOOT_DL_NOTIFY);
>+
>+ t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_DRM_DISABLE_AP);
>+ /* Wait for the DRM disable to take effect */
>+ msleep(FSM_DRM_DISABLE_DELAY_MS);
>+ if (t7xx_devlink_param_get_fastboot(t7xx_dev->dl->ctx)) {
>+ /* Do not try fldr because device will always wait for
>+ * MHCCIF bit 13 in fastboot download flow.
>+ */
>+ t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_DEVICE_RESET);
>+ } else {
> err = t7xx_acpi_fldr_func(t7xx_dev);
> if (err)
> t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_DEVICE_RESET);
>--
>2.34.1
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework
2023-08-03 2:18 ` [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework Jinjian Song
2023-08-03 9:31 ` Jiri Pirko
@ 2023-08-03 9:59 ` Jiri Pirko
2023-08-05 10:25 ` Jinjian Song
2023-08-05 12:12 ` Jinjian Song
3 siblings, 0 replies; 17+ messages in thread
From: Jiri Pirko @ 2023-08-03 9:59 UTC (permalink / raw)
To: Jinjian Song
Cc: netdev, kuba, davem, johannes, ryazanov.s.a, loic.poulain,
ilpo.jarvinen, ricardo.martinez, chiranjeevi.rapolu, haijun.liu,
edumazet, pabeni, chandrashekar.devegowda, m.chetan.kumar,
linuxwwan, linuxwwan_5g, soumya.prakash.mishra, jesse.brandeburg,
danielwinkler, Jinjian Song
Thu, Aug 03, 2023 at 04:18:08AM CEST, songjinjian@hotmail.com wrote:
[...]
>+static const struct devlink_param t7xx_devlink_params[] = {
>+ DEVLINK_PARAM_DRIVER(T7XX_DEVLINK_PARAM_ID_FASTBOOT,
>+ "fastboot", DEVLINK_PARAM_TYPE_BOOL,
>+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT),
>+ NULL, NULL, NULL),
driver init params is there so the user could configure driver instance
and then hit devlink reload in order to reinitialize with the new param
values. In your case, it is a device command. Does not make any sense to
have it as param.
NAK
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework
2023-08-03 2:18 ` [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework Jinjian Song
2023-08-03 9:31 ` Jiri Pirko
2023-08-03 9:59 ` Jiri Pirko
@ 2023-08-05 10:25 ` Jinjian Song
2023-08-05 12:12 ` Jinjian Song
3 siblings, 0 replies; 17+ messages in thread
From: Jinjian Song @ 2023-08-05 10:25 UTC (permalink / raw)
To: jiri
Cc: chandrashekar.devegowda, chiranjeevi.rapolu, danielwinkler, davem,
edumazet, haijun.liu, ilpo.jarvinen, jesse.brandeburg,
jinjian.song, johannes, kuba, linuxwwan, linuxwwan_5g,
loic.poulain, m.chetan.kumar, netdev, pabeni, ricardo.martinez,
ryazanov.s.a, songjinjian, soumya.prakash.mishra
>>+static const struct devlink_param t7xx_devlink_params[] = {
>>+ DEVLINK_PARAM_DRIVER(T7XX_DEVLINK_PARAM_ID_FASTBOOT,
>>+ "fastboot", DEVLINK_PARAM_TYPE_BOOL,
>>+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT),
>>+ NULL, NULL, NULL),
>>+};
>
>Please have the param introduction in a separate file.
>
>Just to mention it right here, this param smells very oddly to me.
>
Thanks for your review, I will add introduction.
>>+static int t7xx_devlink_reload_down(struct devlink *devlink, bool netns_change,
>>+ enum devlink_reload_action action,
>>+ enum devlink_reload_limit limit,
>>+ struct netlink_ext_ack *extack)
>>+{
>>+ return 0;
>>+}
>>+
>>+static int t7xx_devlink_reload_up(struct devlink *devlink,
>>+ enum devlink_reload_action action,
>>+ enum devlink_reload_limit limit,
>>+ u32 *actions_performed,
>>+ struct netlink_ext_ack *extack)
>>+{
>>+ return 0;
>>+}
>>+
>>+static int t7xx_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
>>+ struct netlink_ext_ack *extack)
>>+{
>>+ return 0;
>>+}
>
>Don't put the stub functions here. Introduce them when you need them.
Thanks for your review, I have split devlink falsh to devlink register and devlink flash
as suggestion on v5 before, I want to move this functions to
'device firmware flashing using devlink' patch, is that ok?
>>+static int t7xx_devlink_init(struct t7xx_port *port)
>>+{
>>+ struct t7xx_devlink *dl = port->t7xx_dev->dl;
>>+
>>+ port->rx_length_th = T7XX_MAX_QUEUE_LENGTH;
>>+
>>+ dl->mode = T7XX_NORMAL_MODE;
>>+ dl->status = T7XX_DEVLINK_IDLE;
>
>? What is this supposed to mean? Looks quite wrong.
Thanks for your review, modem register devlink framework to do firmware flashing and core
dump collection, we define mode to identify modem mode T7XX_NORMAL_MODE, T7XX_FB_DL_MODE
(modem fastboot download mode), T7XX_FB_DUMP_MODE(modem fastboot coredump modem)
status used to define the current devlink progress status T7XX_NORMAL_MODE<->T7XX_DEVLINK_IDLE,
T7XX_FB_DL_MODE<->T7XX_FLASH_STATUS, devlink ops info_get<->T7XX_GET_INFO, T7XX_FB_DUMP_MODE<->
(T7XX_LKDUMP_STATUS,T7XX_MRDUMP_STATUS)
>>+ dl->port = port;
>>+
>>+ return 0;
>>+}
>>+
>>+static int t7xx_devlink_enable_chl(struct t7xx_port *port)
>>+{
>>+ t7xx_port_enable_chl(port);
>>+
>>+ return 0;
>>+}
>>+
>>+struct port_ops devlink_port_ops = {
>
>Could you reneame this to me less confusing? Looks like this should be
>related to devlink_port, but actually it is not.
>
>Could you please describe what exatly is the "port" entity for your
>driver? What it represents?
Thanks for your review, this port ops used to define modem port witch used to
flash firmware or collect core dump,devlink command will use this port config to send firmware data
to modem or recieve coredump data from modem.
The "port" mapping to the modem data channel, which used for different functions to comunicate with
modem, all the port configs defined in t7xx_port_proxy.c t7xx_port_config, devlink flash/regions will send
firmware to modem and get core dump info use this "port" config, so we need to define here.
Maybe I can define this port name to flash_dump_port?
>>+ .init = &t7xx_devlink_init,
>>+ .recv_skb = &t7xx_port_enqueue_skb,
>>+ .uninit = &t7xx_devlink_uninit,
>>+ .enable_chl = &t7xx_devlink_enable_chl,
>>+ .disable_chl = &t7xx_port_disable_chl,
>>+};
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next 3/6] net: wwan: t7xx: Implements devlink ops of firmware flashing
2023-08-03 2:18 ` [net-next 3/6] net: wwan: t7xx: Implements devlink ops of firmware flashing Jinjian Song
2023-08-03 9:52 ` Jiri Pirko
@ 2023-08-05 12:05 ` Jinjian Song
2023-08-07 7:35 ` Jiri Pirko
2023-08-07 12:26 ` Jinjian Song
1 sibling, 2 replies; 17+ messages in thread
From: Jinjian Song @ 2023-08-05 12:05 UTC (permalink / raw)
To: jiri
Cc: chandrashekar.devegowda, chiranjeevi.rapolu, danielwinkler, davem,
edumazet, haijun.liu, ilpo.jarvinen, jesse.brandeburg,
jinjian.song, johannes, kuba, linuxwwan, linuxwwan_5g,
loic.poulain, m.chetan.kumar, netdev, pabeni, ricardo.martinez,
ryazanov.s.a, songjinjian, soumya.prakash.mishra
Thu, Aug 03, 2023 at 04:18:09AM CEST, songjinjian@hotmail.com wrote:
>>From: Jinjian Song <jinjian.song@fibocom.com>
>>
>>Adds support for t7xx wwan device firmware flashing using devlink.
>>
>>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.
>>
>>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.
>>
>>Base on the v5 patch version of follow series:
>>'net: wwan: t7xx: fw flashing & coredump support'
>>(https://patchwork.kernel.org/project/netdevbpf/patch/fc8bbb0b66a5ff3a489ea9857d79b374508090ef.1674307425.git.m.chetan.kumar@linux.intel.com/)
>>
>>Signed-off-by: Jinjian Song <jinjian.song@fibocom.com>
>
>Overall, this patch/patchset is very ugly and does some wrong or
>questionable things that make my head hurt. Ugh.
Thanks for your review, this is my first time to do this and when I git send-email,
my email account locked so the patchset break at 3/6 and I resend the remaining 3 patches again.
I will reorganize and prepare v2 version, sorry for that.
>> #include "t7xx_port_devlink.h"
>>
>>+static int t7xx_devlink_port_read(struct t7xx_port *port, char *buf, size_t count)
>
>You have "devlink" in lot of the function and struct field names. Does
>not make sense to me as for example this function does not have anything
>to do with devlink. Could you please rename them all?
Thanks for your review, I think I can rename them all to flash_dump port read, this functions used
to send or recieve data(firmware/coredump/command) with modem
>>+ set_bit(T7XX_FLASH_STATUS, &dl->status);
>>+ port = dl->port;
>>+ dev_dbg(port->dev, "flash partition name:%s binary size:%zu\n", component, fw->size);
>>+ ret = t7xx_devlink_fb_flash_partition(port, component, fw->data, fw->size);
>>+
>>+ sprintf(flash_status, "%s %s", "flashing", !ret ? "success" : "failure");
>
>Don't put return status in to the flash_status. Function returns error
>value which is propagated to the user.
>
>In fact, in your case, usage of devlink_flash_update_status_notify()
>does not make much sense as you don't have multiple flash stages.
Thanks for your review, yes 'success' and 'failure', Function can returns error I will remove
status notify and flash_status
>>+ devlink_flash_update_status_notify(devlink, flash_status, params->component, 0, 0);
>>+ clear_bit(T7XX_FLASH_STATUS, &dl->status);
>>+
>>+err_out:
>>+ return ret;
>> return 0;
>> }
>> static int t7xx_devlink_reload_up(struct devlink *devlink,
>>@@ -50,13 +266,114 @@ static int t7xx_devlink_reload_up(struct devlink *devlink,
>> u32 *actions_performed,
>> struct netlink_ext_ack *extack)
>> {
>>- return 0;
>>+ *actions_performed = BIT(action);
>>+ switch (action) {
>>+ case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
>
>Your driver reinit does not do anything. Please remove it from supported
>actions.
I want to register reinit action and fastboot param with it, so it work like follow:
1.devlink param set fastboot 1 driver_reinit
2.devlink driver_reinit
3.use space command remove driver, then driver remove function get the fastboot param
true, then send message to modem to let modem reboot to fastboot download mode.
4.use space command rescan device, driver probe and export flash port.
5.devlink flash firmware image.
if don't suggest use devlink param fastboot driver_reinit, I think set
fastboot flag during this action, but Intel colleague Kumar have drop that way in the old
v2 patch version.
https://patchwork.kernel.org/project/netdevbpf/patch/20230105154300.198873-1-m.chetan.kumar@linux.intel.com/
>>+ case DEVLINK_RELOAD_ACTION_FW_ACTIVATE:
>>+ return 0;
>>+ default:
>>+ /* Unsupported action should not get to this function */
>>+ return -EOPNOTSUPP;
>>+ }
>>+}
>>+struct devlink_info_req {
>>+ struct sk_buff *msg;
>>+ void (*version_cb)(const char *version_name,
>>+ enum devlink_info_version_type version_type,
>>+ void *version_cb_priv);
>>+ void *version_cb_priv;
>>+};
>
>Ah! No. Remove this. If you need to touch internal of the struct, this
>is definitelly not the way to do it.
Thanks for your review, got it.
>>+
>>+struct devlink_flash_component_lookup_ctx {
>>+ const char *lookup_name;
>>+ bool lookup_name_found;
>>+};
>
>Same here.
Thanks for your review, got it.
>>+
>>+static int t7xx_devlink_info_get_loopback(struct devlink *devlink, struct devlink_info_req *req,
>>+ struct netlink_ext_ack *extack)
>>+{
>>+ struct devlink_flash_component_lookup_ctx *lookup_ctx = req->version_cb_priv;
>>+ int ret;
>>+
>>+ if (!req)
>>+ return t7xx_devlink_info_get(devlink, req, extack);
>>+
>>+ ret = devlink_info_version_running_put_ext(req, lookup_ctx->lookup_name,
>
>It actually took me a while why you are doing this. You try to overcome
>the limitation for drivers to expose version for all components that are
>valid for flashing. That is not nice
>
>Please don't do things like this!
>
>Expose the versions for all valid components, or don't flash them.
For the old modem firmware, it don't support the info_get function, so add the logic here to
compatible with old modem firmware update during devlink flash.
>>+ "1.0", DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>>+
>>+ return ret;
>> }
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework
2023-08-03 2:18 ` [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework Jinjian Song
` (2 preceding siblings ...)
2023-08-05 10:25 ` Jinjian Song
@ 2023-08-05 12:12 ` Jinjian Song
2023-08-07 7:22 ` Jiri Pirko
2023-08-07 12:44 ` Jinjian Song
3 siblings, 2 replies; 17+ messages in thread
From: Jinjian Song @ 2023-08-05 12:12 UTC (permalink / raw)
To: jiri
Cc: chandrashekar.devegowda, chiranjeevi.rapolu, danielwinkler, davem,
edumazet, haijun.liu, ilpo.jarvinen, jesse.brandeburg,
jinjian.song, johannes, kuba, linuxwwan, linuxwwan_5g,
loic.poulain, m.chetan.kumar, netdev, pabeni, ricardo.martinez,
ryazanov.s.a, songjinjian, soumya.prakash.mishra
Thu, Aug 03, 2023 at 04:18:08AM CEST, songjinjian@hotmail.com wrote:
[...]
>>+static const struct devlink_param t7xx_devlink_params[] = {
>>+ DEVLINK_PARAM_DRIVER(T7XX_DEVLINK_PARAM_ID_FASTBOOT,
>>+ "fastboot", DEVLINK_PARAM_TYPE_BOOL,
>>+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT),
>>+ NULL, NULL, NULL),
>
>driver init params is there so the user could configure driver instance
>and then hit devlink reload in order to reinitialize with the new param
>values. In your case, it is a device command. Does not make any sense to
>have it as param.
>
>NAK
Thanks for your review, so drop this param and set this param insider driver when driver_reinit,
is that fine?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework
2023-08-05 12:12 ` Jinjian Song
@ 2023-08-07 7:22 ` Jiri Pirko
2023-08-07 12:44 ` Jinjian Song
1 sibling, 0 replies; 17+ messages in thread
From: Jiri Pirko @ 2023-08-07 7:22 UTC (permalink / raw)
To: Jinjian Song
Cc: chandrashekar.devegowda, chiranjeevi.rapolu, danielwinkler, davem,
edumazet, haijun.liu, ilpo.jarvinen, jesse.brandeburg,
jinjian.song, johannes, kuba, linuxwwan, linuxwwan_5g,
loic.poulain, m.chetan.kumar, netdev, pabeni, ricardo.martinez,
ryazanov.s.a, soumya.prakash.mishra
Sat, Aug 05, 2023 at 02:12:13PM CEST, songjinjian@hotmail.com wrote:
>Thu, Aug 03, 2023 at 04:18:08AM CEST, songjinjian@hotmail.com wrote:
>
>[...]
>
>>>+static const struct devlink_param t7xx_devlink_params[] = {
>>>+ DEVLINK_PARAM_DRIVER(T7XX_DEVLINK_PARAM_ID_FASTBOOT,
>>>+ "fastboot", DEVLINK_PARAM_TYPE_BOOL,
>>>+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT),
>>>+ NULL, NULL, NULL),
>>
>>driver init params is there so the user could configure driver instance
>>and then hit devlink reload in order to reinitialize with the new param
>>values. In your case, it is a device command. Does not make any sense to
>>have it as param.
>>
>>NAK
>
>Thanks for your review, so drop this param and set this param insider driver when driver_reinit,
>is that fine?
I don't understand the question, sorry.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next 3/6] net: wwan: t7xx: Implements devlink ops of firmware flashing
2023-08-05 12:05 ` Jinjian Song
@ 2023-08-07 7:35 ` Jiri Pirko
2023-08-07 12:26 ` Jinjian Song
1 sibling, 0 replies; 17+ messages in thread
From: Jiri Pirko @ 2023-08-07 7:35 UTC (permalink / raw)
To: Jinjian Song
Cc: chandrashekar.devegowda, chiranjeevi.rapolu, danielwinkler, davem,
edumazet, haijun.liu, ilpo.jarvinen, jesse.brandeburg,
jinjian.song, johannes, kuba, linuxwwan, linuxwwan_5g,
loic.poulain, m.chetan.kumar, netdev, pabeni, ricardo.martinez,
ryazanov.s.a, soumya.prakash.mishra
Sat, Aug 05, 2023 at 02:05:35PM CEST, songjinjian@hotmail.com wrote:
>Thu, Aug 03, 2023 at 04:18:09AM CEST, songjinjian@hotmail.com wrote:
>>>From: Jinjian Song <jinjian.song@fibocom.com>
>>>
>>>Adds support for t7xx wwan device firmware flashing using devlink.
>>>
>>>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.
>>>
>>>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.
>>>
>>>Base on the v5 patch version of follow series:
>>>'net: wwan: t7xx: fw flashing & coredump support'
>>>(https://patchwork.kernel.org/project/netdevbpf/patch/fc8bbb0b66a5ff3a489ea9857d79b374508090ef.1674307425.git.m.chetan.kumar@linux.intel.com/)
>>>
>>>Signed-off-by: Jinjian Song <jinjian.song@fibocom.com>
>>
>>Overall, this patch/patchset is very ugly and does some wrong or
>>questionable things that make my head hurt. Ugh.
>
>Thanks for your review, this is my first time to do this and when I git send-email,
>my email account locked so the patchset break at 3/6 and I resend the remaining 3 patches again.
>
>I will reorganize and prepare v2 version, sorry for that.
>
>>> #include "t7xx_port_devlink.h"
>>>
>>>+static int t7xx_devlink_port_read(struct t7xx_port *port, char *buf, size_t count)
>>
>>You have "devlink" in lot of the function and struct field names. Does
>>not make sense to me as for example this function does not have anything
>>to do with devlink. Could you please rename them all?
>
>Thanks for your review, I think I can rename them all to flash_dump port read, this functions used
>to send or recieve data(firmware/coredump/command) with modem
>
>>>+ set_bit(T7XX_FLASH_STATUS, &dl->status);
>>>+ port = dl->port;
>>>+ dev_dbg(port->dev, "flash partition name:%s binary size:%zu\n", component, fw->size);
>>>+ ret = t7xx_devlink_fb_flash_partition(port, component, fw->data, fw->size);
>>>+
>>>+ sprintf(flash_status, "%s %s", "flashing", !ret ? "success" : "failure");
>>
>>Don't put return status in to the flash_status. Function returns error
>>value which is propagated to the user.
>>
>>In fact, in your case, usage of devlink_flash_update_status_notify()
>>does not make much sense as you don't have multiple flash stages.
>
>Thanks for your review, yes 'success' and 'failure', Function can returns error I will remove
>status notify and flash_status
>
>>>+ devlink_flash_update_status_notify(devlink, flash_status, params->component, 0, 0);
>>>+ clear_bit(T7XX_FLASH_STATUS, &dl->status);
>>>+
>>>+err_out:
>>>+ return ret;
>>> return 0;
>>> }
>
>>> static int t7xx_devlink_reload_up(struct devlink *devlink,
>>>@@ -50,13 +266,114 @@ static int t7xx_devlink_reload_up(struct devlink *devlink,
>>> u32 *actions_performed,
>>> struct netlink_ext_ack *extack)
>>> {
>>>- return 0;
>>>+ *actions_performed = BIT(action);
>>>+ switch (action) {
>>>+ case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
>>
>>Your driver reinit does not do anything. Please remove it from supported
>>actions.
>
>I want to register reinit action and fastboot param with it, so it work like follow:
>1.devlink param set fastboot 1 driver_reinit
>2.devlink driver_reinit
>3.use space command remove driver, then driver remove function get the fastboot param
>true, then send message to modem to let modem reboot to fastboot download mode.
What do you mean by this? I don't follow. What's "command remove
driver"?
>4.use space command rescan device, driver probe and export flash port.
Again, what's "command rescan device" ?
Could you perhaps rather use command line examples?
>5.devlink flash firmware image.
>
>if don't suggest use devlink param fastboot driver_reinit, I think set
>fastboot flag during this action, but Intel colleague Kumar have drop that way in the old
>v2 patch version.
>https://patchwork.kernel.org/project/netdevbpf/patch/20230105154300.198873-1-m.chetan.kumar@linux.intel.com/
>
>>>+ case DEVLINK_RELOAD_ACTION_FW_ACTIVATE:
>>>+ return 0;
>>>+ default:
>>>+ /* Unsupported action should not get to this function */
>>>+ return -EOPNOTSUPP;
>>>+ }
>>>+}
>
>>>+struct devlink_info_req {
>>>+ struct sk_buff *msg;
>>>+ void (*version_cb)(const char *version_name,
>>>+ enum devlink_info_version_type version_type,
>>>+ void *version_cb_priv);
>>>+ void *version_cb_priv;
>>>+};
>>
>>Ah! No. Remove this. If you need to touch internal of the struct, this
>>is definitelly not the way to do it.
>
>Thanks for your review, got it.
>
>>>+
>>>+struct devlink_flash_component_lookup_ctx {
>>>+ const char *lookup_name;
>>>+ bool lookup_name_found;
>>>+};
>>
>>Same here.
>
>Thanks for your review, got it.
>
>>>+
>>>+static int t7xx_devlink_info_get_loopback(struct devlink *devlink, struct devlink_info_req *req,
>>>+ struct netlink_ext_ack *extack)
>>>+{
>>>+ struct devlink_flash_component_lookup_ctx *lookup_ctx = req->version_cb_priv;
>>>+ int ret;
>>>+
>>>+ if (!req)
>>>+ return t7xx_devlink_info_get(devlink, req, extack);
>>>+
>>>+ ret = devlink_info_version_running_put_ext(req, lookup_ctx->lookup_name,
>>
>>It actually took me a while why you are doing this. You try to overcome
>>the limitation for drivers to expose version for all components that are
>>valid for flashing. That is not nice
>>
>>Please don't do things like this!
>>
>>Expose the versions for all valid components, or don't flash them.
>
>For the old modem firmware, it don't support the info_get function, so add the logic here to
>compatible with old modem firmware update during devlink flash.
No! Don't do this. I don't care about your firmware. We enforce info_get
and flash component parity, obey it. Either provide the version info for
all components you want to flash with proper versions, or don't
implement the flash.
>
>>>+ "1.0", DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>>>+
>>>+ return ret;
>>> }
>>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next 3/6] net: wwan: t7xx: Implements devlink ops of firmware flashing
2023-08-05 12:05 ` Jinjian Song
2023-08-07 7:35 ` Jiri Pirko
@ 2023-08-07 12:26 ` Jinjian Song
2023-08-07 13:01 ` Jiri Pirko
2023-08-10 15:32 ` Jinjian Song
1 sibling, 2 replies; 17+ messages in thread
From: Jinjian Song @ 2023-08-07 12:26 UTC (permalink / raw)
To: jiri
Cc: chandrashekar.devegowda, chiranjeevi.rapolu, danielwinkler, davem,
edumazet, haijun.liu, ilpo.jarvinen, jesse.brandeburg,
jinjian.song, johannes, kuba, linuxwwan, linuxwwan_5g,
loic.poulain, m.chetan.kumar, netdev, pabeni, ricardo.martinez,
ryazanov.s.a, songjinjian, soumya.prakash.mishra
Thansk for your review.
Witch command: echo 1 > /sys/bus/pci/devices/${bdf}/remove, then driver will run the
.remove ops, during this steps, driver get the fastboot param then send command to
device let device reset to fastboot download mode.
>
>
>>4.use space command rescan device, driver probe and export flash port.
>
>Again, what's "command rescan device" ?
>
>Could you perhaps rather use command line examples?
Thansk for your review.
With the command: echo 1 > /sys/bus/pci/rescan, then driver will run the .probe options
then driver will follow the fastboot download process to export the download ports.
>
>>5.devlink flash firmware image.
>>
>>if don't suggest use devlink param fastboot driver_reinit, I think set
>>fastboot flag during this action, but Intel colleague Kumar have drop that way in the old
>>v2 patch version.
>>https://patchwork.kernel.org/project/netdevbpf/patch/20230105154300.198873-1-m.chetan.kumar@linux.intel.com/
>>
>>>+ case DEVLINK_RELOAD_ACTION_FW_ACTIVATE:
>>>+ return 0;
>>>+ default:
>>>+ /* Unsupported action should not get to this function */
>>>+ return -EOPNOTSUPP;
>>>+ }
>>>+}
>
>>>>+static int t7xx_devlink_info_get_loopback(struct devlink *devlink, struct devlink_info_req *req,
>>>>+ struct netlink_ext_ack *extack)
>>>>+{
>>>>+ struct devlink_flash_component_lookup_ctx *lookup_ctx = req->version_cb_priv;
>>>>+ int ret;
>>>>+
>>>>+ if (!req)
>>>>+ return t7xx_devlink_info_get(devlink, req, extack);
>>>>+
>>>>+ ret = devlink_info_version_running_put_ext(req, lookup_ctx->lookup_name,
>>>
>>>It actually took me a while why you are doing this. You try to overcome
>>>the limitation for drivers to expose version for all components that are
>>>valid for flashing. That is not nice
>>>
>>>Please don't do things like this!
>>>
>>>Expose the versions for all valid components, or don't flash them.
>>
>>For the old modem firmware, it don't support the info_get function, so add the logic here to
>>compatible with old modem firmware update during devlink flash.
>
>No! Don't do this. I don't care about your firmware. We enforce info_get
>and flash component parity, obey it. Either provide the version info for
>all components you want to flash with proper versions, or don't
>implement the flash.
Thanks for your review, I will delete the info_get_loopback function.
>
>
>>
>>>>+ "1.0", DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>>>>+
>>>>+ return ret;
>>>> }
>>>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework
2023-08-05 12:12 ` Jinjian Song
2023-08-07 7:22 ` Jiri Pirko
@ 2023-08-07 12:44 ` Jinjian Song
2023-08-07 12:58 ` Jiri Pirko
2023-08-10 15:19 ` Jinjian Song
1 sibling, 2 replies; 17+ messages in thread
From: Jinjian Song @ 2023-08-07 12:44 UTC (permalink / raw)
To: jiri
Cc: chandrashekar.devegowda, chiranjeevi.rapolu, danielwinkler, davem,
edumazet, haijun.liu, ilpo.jarvinen, jesse.brandeburg,
jinjian.song, johannes, kuba, linuxwwan, linuxwwan_5g,
loic.poulain, m.chetan.kumar, netdev, pabeni, ricardo.martinez,
ryazanov.s.a, songjinjian, soumya.prakash.mishra
Sat, Aug 05, 2023 at 02:12:13PM CEST, songjinjian@hotmail.com wrote:
>>Thu, Aug 03, 2023 at 04:18:08AM CEST, songjinjian@hotmail.com wrote:
>>
>>[...]
>>
>>>>+static const struct devlink_param t7xx_devlink_params[] = {
>>>>+ DEVLINK_PARAM_DRIVER(T7XX_DEVLINK_PARAM_ID_FASTBOOT,
>>>>+ "fastboot", DEVLINK_PARAM_TYPE_BOOL,
>>>>+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT),
>>>>+ NULL, NULL, NULL),
>>>
>>>driver init params is there so the user could configure driver instance
>>>and then hit devlink reload in order to reinitialize with the new param
>>>values. In your case, it is a device command. Does not make any sense to
>>>have it as param.
>>>
>>>NAK
>>
>>Thanks for your review, so drop this param and set this param insider driver when driver_reinit,
>>is that fine?
>
>I don't understand the question, sorry.
Thanks for your review, I mean if I don't define fastboot param like devlink_param above, I will
define a global bool variable in driver, then when devlink ... driver_reinit, set this variable to true.
like:
t7xx_devlink {
....
bool reset_to_fastboot;
}
t7xx_devlink_reload_down () {
...
case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
t7xx_devlink.reset_to_fastboot = true;
...
}
other functions use this variable:
if (t7xx_devlink.reset_to_fastboot) {
iowrite(reg, "reset to fastboot");
}
Intel colleague has change to the way of devlink_param, so I hope to keep this.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework
2023-08-07 12:44 ` Jinjian Song
@ 2023-08-07 12:58 ` Jiri Pirko
2023-08-10 15:19 ` Jinjian Song
1 sibling, 0 replies; 17+ messages in thread
From: Jiri Pirko @ 2023-08-07 12:58 UTC (permalink / raw)
To: Jinjian Song
Cc: chandrashekar.devegowda, chiranjeevi.rapolu, danielwinkler, davem,
edumazet, haijun.liu, ilpo.jarvinen, jesse.brandeburg,
jinjian.song, johannes, kuba, linuxwwan, linuxwwan_5g,
loic.poulain, m.chetan.kumar, netdev, pabeni, ricardo.martinez,
ryazanov.s.a, soumya.prakash.mishra
Mon, Aug 07, 2023 at 02:44:00PM CEST, songjinjian@hotmail.com wrote:
>Sat, Aug 05, 2023 at 02:12:13PM CEST, songjinjian@hotmail.com wrote:
>>>Thu, Aug 03, 2023 at 04:18:08AM CEST, songjinjian@hotmail.com wrote:
>>>
>>>[...]
>>>
>>>>>+static const struct devlink_param t7xx_devlink_params[] = {
>>>>>+ DEVLINK_PARAM_DRIVER(T7XX_DEVLINK_PARAM_ID_FASTBOOT,
>>>>>+ "fastboot", DEVLINK_PARAM_TYPE_BOOL,
>>>>>+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT),
>>>>>+ NULL, NULL, NULL),
>>>>
>>>>driver init params is there so the user could configure driver instance
>>>>and then hit devlink reload in order to reinitialize with the new param
>>>>values. In your case, it is a device command. Does not make any sense to
>>>>have it as param.
>>>>
>>>>NAK
>>>
>>>Thanks for your review, so drop this param and set this param insider driver when driver_reinit,
>>>is that fine?
>>
>>I don't understand the question, sorry.
>
>Thanks for your review, I mean if I don't define fastboot param like devlink_param above, I will
Could you not thank me in every reply, please?
>define a global bool variable in driver, then when devlink ... driver_reinit, set this variable to true.
That would be very wrong. Driver reinit should just reload the driver,
recreate the entities created. should not be trigger to change
behaviour.
>
>like:
> t7xx_devlink {
> ....
> bool reset_to_fastboot;
> }
>
>
> t7xx_devlink_reload_down () {
> ...
> case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
> t7xx_devlink.reset_to_fastboot = true;
> ...
> }
>
> other functions use this variable:
>
> if (t7xx_devlink.reset_to_fastboot) {
> iowrite(reg, "reset to fastboot");
> }
>
>Intel colleague has change to the way of devlink_param, so I hope to keep this.
>
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next 3/6] net: wwan: t7xx: Implements devlink ops of firmware flashing
2023-08-07 12:26 ` Jinjian Song
@ 2023-08-07 13:01 ` Jiri Pirko
2023-08-10 15:32 ` Jinjian Song
1 sibling, 0 replies; 17+ messages in thread
From: Jiri Pirko @ 2023-08-07 13:01 UTC (permalink / raw)
To: Jinjian Song
Cc: chandrashekar.devegowda, chiranjeevi.rapolu, danielwinkler, davem,
edumazet, haijun.liu, ilpo.jarvinen, jesse.brandeburg,
jinjian.song, johannes, kuba, linuxwwan, linuxwwan_5g,
loic.poulain, m.chetan.kumar, netdev, pabeni, ricardo.martinez,
ryazanov.s.a, soumya.prakash.mishra
Mon, Aug 07, 2023 at 02:26:53PM CEST, songjinjian@hotmail.com wrote:
>Thansk for your review.
>Witch command: echo 1 > /sys/bus/pci/devices/${bdf}/remove, then driver will run the
>.remove ops, during this steps, driver get the fastboot param then send command to
>device let device reset to fastboot download mode.
Ugh.
>
>>
>>
>>>4.use space command rescan device, driver probe and export flash port.
>>
>>Again, what's "command rescan device" ?
>>
>>Could you perhaps rather use command line examples?
>
>Thansk for your review.
>With the command: echo 1 > /sys/bus/pci/rescan, then driver will run the .probe options
>then driver will follow the fastboot download process to export the download ports.
That is certainly incorrect. No configuration or operation with the
device instance should require to unbind&bind the device on the bus.
>
>>
>>>5.devlink flash firmware image.
>>>
>>>if don't suggest use devlink param fastboot driver_reinit, I think set
>>>fastboot flag during this action, but Intel colleague Kumar have drop that way in the old
>>>v2 patch version.
>>>https://patchwork.kernel.org/project/netdevbpf/patch/20230105154300.198873-1-m.chetan.kumar@linux.intel.com/
>>>
>>>>+ case DEVLINK_RELOAD_ACTION_FW_ACTIVATE:
>>>>+ return 0;
>>>>+ default:
>>>>+ /* Unsupported action should not get to this function */
>>>>+ return -EOPNOTSUPP;
>>>>+ }
>>>>+}
>>
>>>>>+static int t7xx_devlink_info_get_loopback(struct devlink *devlink, struct devlink_info_req *req,
>>>>>+ struct netlink_ext_ack *extack)
>>>>>+{
>>>>>+ struct devlink_flash_component_lookup_ctx *lookup_ctx = req->version_cb_priv;
>>>>>+ int ret;
>>>>>+
>>>>>+ if (!req)
>>>>>+ return t7xx_devlink_info_get(devlink, req, extack);
>>>>>+
>>>>>+ ret = devlink_info_version_running_put_ext(req, lookup_ctx->lookup_name,
>>>>
>>>>It actually took me a while why you are doing this. You try to overcome
>>>>the limitation for drivers to expose version for all components that are
>>>>valid for flashing. That is not nice
>>>>
>>>>Please don't do things like this!
>>>>
>>>>Expose the versions for all valid components, or don't flash them.
>>>
>>>For the old modem firmware, it don't support the info_get function, so add the logic here to
>>>compatible with old modem firmware update during devlink flash.
>>
>>No! Don't do this. I don't care about your firmware. We enforce info_get
>>and flash component parity, obey it. Either provide the version info for
>>all components you want to flash with proper versions, or don't
>>implement the flash.
>
>Thanks for your review, I will delete the info_get_loopback function.
>
>>
>>
>>>
>>>>>+ "1.0", DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>>>>>+
>>>>>+ return ret;
>>>>> }
>>>>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework
2023-08-07 12:44 ` Jinjian Song
2023-08-07 12:58 ` Jiri Pirko
@ 2023-08-10 15:19 ` Jinjian Song
1 sibling, 0 replies; 17+ messages in thread
From: Jinjian Song @ 2023-08-10 15:19 UTC (permalink / raw)
To: jiri
Cc: chandrashekar.devegowda, chiranjeevi.rapolu, danielwinkler, davem,
edumazet, haijun.liu, ilpo.jarvinen, jesse.brandeburg,
jinjian.song, johannes, kuba, linuxwwan, linuxwwan_5g,
loic.poulain, m.chetan.kumar, netdev, pabeni, ricardo.martinez,
ryazanov.s.a, songjinjian, soumya.prakash.mishra, vsankar
>Mon, Aug 07, 2023 at 02:44:00PM CEST, songjinjian@hotmail.com wrote:
>>Sat, Aug 05, 2023 at 02:12:13PM CEST, songjinjian@hotmail.com wrote:
>>>>Thu, Aug 03, 2023 at 04:18:08AM CEST, songjinjian@hotmail.com wrote:
>>>>
>>>>[...]
>>>>
>>>>>>+static const struct devlink_param t7xx_devlink_params[] = {
>>>>>>+ DEVLINK_PARAM_DRIVER(T7XX_DEVLINK_PARAM_ID_FASTBOOT,
>>>>>>+ "fastboot", DEVLINK_PARAM_TYPE_BOOL,
>>>>>>+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT),
>>>>>>+ NULL, NULL, NULL),
>>>>>
>>>>>driver init params is there so the user could configure driver instance
>>>>>and then hit devlink reload in order to reinitialize with the new param
>>>>>values. In your case, it is a device command. Does not make any sense to
>>>>>have it as param.
>>>>>
>>>>>NAK
>>>>
>>>>Thanks for your review, so drop this param and set this param insider driver when driver_reinit,
>>>>is that fine?
>>>
>>>I don't understand the question, sorry.
>>
>>Thanks for your review, I mean if I don't define fastboot param like devlink_param above, I will
>
>Could you not thank me in every reply, please?
>
>
>>define a global bool variable in driver, then when devlink ... driver_reinit, set this variable to true.
>
>That would be very wrong. Driver reinit should just reload the driver,
>recreate the entities created. should not be trigger to change
>behaviour.
Thanks for your review, if so, I hope it is possible to keep the devlink param as above.
>
>>
>>like:
>> t7xx_devlink {
>> ....
>> bool reset_to_fastboot;
>> }
>>
>>
>> t7xx_devlink_reload_down () {
>> ...
>> case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
>> t7xx_devlink.reset_to_fastboot = true;
>> ...
>> }
>>
>> other functions use this variable:
>>
>> if (t7xx_devlink.reset_to_fastboot) {
>> iowrite(reg, "reset to fastboot");
>> }
>>
>>Intel colleague has change to the way of devlink_param, so I hope to keep this.
>>
>>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next 3/6] net: wwan: t7xx: Implements devlink ops of firmware flashing
2023-08-07 12:26 ` Jinjian Song
2023-08-07 13:01 ` Jiri Pirko
@ 2023-08-10 15:32 ` Jinjian Song
1 sibling, 0 replies; 17+ messages in thread
From: Jinjian Song @ 2023-08-10 15:32 UTC (permalink / raw)
To: jiri
Cc: chandrashekar.devegowda, chiranjeevi.rapolu, danielwinkler, davem,
edumazet, haijun.liu, ilpo.jarvinen, jesse.brandeburg,
jinjian.song, johannes, kuba, linuxwwan, linuxwwan_5g,
loic.poulain, m.chetan.kumar, netdev, pabeni, ricardo.martinez,
ryazanov.s.a, songjinjian, soumya.prakash.mishra, vsankar
>Mon, Aug 07, 2023 at 02:26:53PM CEST, songjinjian@hotmail.com wrote:
>>Thansk for your review.
>>Witch command: echo 1 > /sys/bus/pci/devices/${bdf}/remove, then driver will run the
>>.remove ops, during this steps, driver get the fastboot param then send command to
>>device let device reset to fastboot download mode.
>
>Ugh.
>
>
>>
>>>
>>>
>>>>4.use space command rescan device, driver probe and export flash port.
>>>
>>>Again, what's "command rescan device" ?
>>>
>>>Could you perhaps rather use command line examples?
>>
>>Thansk for your review.
>>With the command: echo 1 > /sys/bus/pci/rescan, then driver will run the .probe options
>>then driver will follow the fastboot download process to export the download ports.
>
>That is certainly incorrect. No configuration or operation with the
>device instance should require to unbind&bind the device on the bus.
Thanks for your review, this remove rescan logic impliment in V5 patch driver version, as it can't be allowned
in driver so I move it out driver patch, just now it seemed no other suitable way to reprobe device and identify
device in download mode after device reset.
By the way,this remove rescan logic works with iosm driver also, so if there is no idea of other way, I hope it
can be allowned without effect the kernel.
>>
>>>
>>>>5.devlink flash firmware image.
>>>>
>>>>if don't suggest use devlink param fastboot driver_reinit, I think set
>>>>fastboot flag during this action, but Intel colleague Kumar have drop that way in the old
>>>>v2 patch version.
>>>>https://patchwork.kernel.org/project/netdevbpf/patch/20230105154300.198873-1-m.chetan.kumar@linux.intel.com/
>>>>
>>>>>+ case DEVLINK_RELOAD_ACTION_FW_ACTIVATE:
>>>>>+ return 0;
>>>>>+ default:
>>>>>+ /* Unsupported action should not get to this function */
>>>>>+ return -EOPNOTSUPP;
>>>>>+ }
>>>>>+}
>>>
>>>>>>+static int t7xx_devlink_info_get_loopback(struct devlink *devlink, struct devlink_info_req *req,
>>>>>>+ struct netlink_ext_ack *extack)
>>>>>>+{
>>>>>>+ struct devlink_flash_component_lookup_ctx *lookup_ctx = req->version_cb_priv;
>>>>>>+ int ret;
>>>>>>+
>>>>>>+ if (!req)
>>>>>>+ return t7xx_devlink_info_get(devlink, req, extack);
>>>>>>+
>>>>>>+ ret = devlink_info_version_running_put_ext(req, lookup_ctx->lookup_name,
>>>>>
>>>>>It actually took me a while why you are doing this. You try to overcome
>>>>>the limitation for drivers to expose version for all components that are
>>>>>valid for flashing. That is not nice
>>>>>
>>>>>Please don't do things like this!
>>>>>
>>>>>Expose the versions for all valid components, or don't flash them.
>>>>
>>>>For the old modem firmware, it don't support the info_get function, so add the logic here to
>>>>compatible with old modem firmware update during devlink flash.
>>>
>>>No! Don't do this. I don't care about your firmware. We enforce info_get
>>>and flash component parity, obey it. Either provide the version info for
>>>all components you want to flash with proper versions, or don't
>>>implement the flash.
>>
>>Thanks for your review, I will delete the info_get_loopback function.
>>
>>>
>>>
>>>>
>>>>>>+ "1.0", DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>>>>>>+
>>>>>>+ return ret;
>>>>>> }
>>>>>>
>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-08-10 15:33 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230803021812.6126-1-songjinjian@hotmail.com>
2023-08-03 2:18 ` [net-next 1/6] net: wwan: t7xx: Infrastructure for early port configuration Jinjian Song
2023-08-03 2:18 ` [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework Jinjian Song
2023-08-03 9:31 ` Jiri Pirko
2023-08-03 9:59 ` Jiri Pirko
2023-08-05 10:25 ` Jinjian Song
2023-08-05 12:12 ` Jinjian Song
2023-08-07 7:22 ` Jiri Pirko
2023-08-07 12:44 ` Jinjian Song
2023-08-07 12:58 ` Jiri Pirko
2023-08-10 15:19 ` Jinjian Song
2023-08-03 2:18 ` [net-next 3/6] net: wwan: t7xx: Implements devlink ops of firmware flashing Jinjian Song
2023-08-03 9:52 ` Jiri Pirko
2023-08-05 12:05 ` Jinjian Song
2023-08-07 7:35 ` Jiri Pirko
2023-08-07 12:26 ` Jinjian Song
2023-08-07 13:01 ` Jiri Pirko
2023-08-10 15:32 ` Jinjian Song
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox