[PATCH net-next v8 0/9] TXGBE PHYLINK support

[PATCH net-next v8 0/9] TXGBE PHYLINK support

[Jiawen Wu]

Implement I2C, SFP, GPIO and PHYLINK to setup TXGBE link.

Because our I2C and PCS are based on Synopsys Designware IP-core, extend
the i2c-designware and pcs-xpcs driver to realize our functions.

v7 -> v8:
- use macro defined I2C FIFO depth instead of magic number
- fix return code of clock create failure
- add spinlock for writing GPIO registers
- implement triggering GPIO interrupts for both-edge type
- remove the condition that enables interrupts
- add mii bus check for PCS device
- other minor style changes

v6 -> v7:
- change swnode property of I2C platform to be boolean
- use device_property_present() to match I2C device data

v5 -> v6:
- fix to set error code if pointer of txgbe is NULL
- change "if" to "switch" for *_i2c_dw_xfer_quirk()
- rename property for I2C device flag
- use regmap to access I2C mem region
- use DEFINE_RES_IRQ()
- use phylink_mii_c45_pcs_get_state() for DW_XPCS_10GBASER

v4 -> v5:
- add clock register
- delete i2c-dw.h with platform data
- introduce property "i2c-dw-flags" to match device flags
- get resource from platform info to do ioremap
- rename quirk functions in i2c-designware-*.c
- fix calling txgbe_phylink_init()

v3 -> v4:
- modify I2C transfer to be generic implementation
- avoid to read DW_IC_COMP_PARAM_1
- remove redundant "if" statement
- add specific labels to handle error in txgbe_init_phy(), and remove
  "if" conditions in txgbe_remove_phy()

v2 -> v3:
- delete own I2C bus master driver, support it in i2c-designware
- delete own PCS functions, remove pma configuration and 1000BASE-X mode
- add basic function for 10GBASE-R interface in pcs-xpcs
- add helper to get txgbe pointer from netdev

v1 -> v2:
- add comments to indicate GPIO lines
- add I2C write operation support
- modify GPIO direction functions
- rename functions related to PHY interface
- add condition on interface changing to re-config PCS
- add to set advertise and fix to get status for 1000BASE-X mode
- other redundant codes remove

Jiawen Wu (9):
  net: txgbe: Add software nodes to support phylink
  i2c: designware: Add driver support for Wangxun 10Gb NIC
  net: txgbe: Register fixed rate clock
  net: txgbe: Register I2C platform device
  net: txgbe: Add SFP module identify
  net: txgbe: Support GPIO to SFP socket
  net: pcs: Add 10GBASE-R mode for Synopsys Designware XPCS
  net: txgbe: Implement phylink pcs
  net: txgbe: Support phylink MAC layer

 drivers/i2c/busses/i2c-designware-common.c    |   8 +
 drivers/i2c/busses/i2c-designware-core.h      |   4 +
 drivers/i2c/busses/i2c-designware-master.c    |  89 ++-
 drivers/i2c/busses/i2c-designware-platdrv.c   |  15 +
 drivers/net/ethernet/wangxun/Kconfig          |   8 +
 drivers/net/ethernet/wangxun/libwx/wx_lib.c   |   3 +-
 drivers/net/ethernet/wangxun/libwx/wx_type.h  |   4 +
 drivers/net/ethernet/wangxun/txgbe/Makefile   |   1 +
 .../ethernet/wangxun/txgbe/txgbe_ethtool.c    |  28 +
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   |  65 +-
 .../net/ethernet/wangxun/txgbe/txgbe_phy.c    | 672 ++++++++++++++++++
 .../net/ethernet/wangxun/txgbe/txgbe_phy.h    |  10 +
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   |  90 +++
 drivers/net/pcs/pcs-xpcs.c                    |  30 +
 include/linux/pcs/pcs-xpcs.h                  |   1 +
 15 files changed, 990 insertions(+), 38 deletions(-)
 create mode 100644 drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
 create mode 100644 drivers/net/ethernet/wangxun/txgbe/txgbe_phy.h

-- 
2.27.0

[PATCH net-next v8 1/9] net: txgbe: Add software nodes to support phylink

[Jiawen Wu]

Register software nodes for GPIO, I2C, SFP and PHYLINK. Define the
device properties.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Piotr Raczynski <piotr.raczynski@intel.com>
---
 drivers/net/ethernet/wangxun/libwx/wx_type.h  |  1 +
 drivers/net/ethernet/wangxun/txgbe/Makefile   |  1 +
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   | 22 ++++-
 .../net/ethernet/wangxun/txgbe/txgbe_phy.c    | 89 +++++++++++++++++++
 .../net/ethernet/wangxun/txgbe/txgbe_phy.h    | 10 +++
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   | 49 ++++++++++
 6 files changed, 171 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
 create mode 100644 drivers/net/ethernet/wangxun/txgbe/txgbe_phy.h

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
index 32f952d93009..97bce855bc60 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
@@ -611,6 +611,7 @@ enum wx_isb_idx {
 
 struct wx {
 	u8 __iomem *hw_addr;
+	void *priv;
 	struct pci_dev *pdev;
 	struct net_device *netdev;
 	struct wx_bus_info bus;
diff --git a/drivers/net/ethernet/wangxun/txgbe/Makefile b/drivers/net/ethernet/wangxun/txgbe/Makefile
index 6db14a2cb2d0..7507f762edfe 100644
--- a/drivers/net/ethernet/wangxun/txgbe/Makefile
+++ b/drivers/net/ethernet/wangxun/txgbe/Makefile
@@ -8,4 +8,5 @@ obj-$(CONFIG_TXGBE) += txgbe.o
 
 txgbe-objs := txgbe_main.o \
               txgbe_hw.o \
+              txgbe_phy.o \
               txgbe_ethtool.o
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index 5b8a121fb496..e10296abf5b4 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -15,6 +15,7 @@
 #include "../libwx/wx_hw.h"
 #include "txgbe_type.h"
 #include "txgbe_hw.h"
+#include "txgbe_phy.h"
 #include "txgbe_ethtool.h"
 
 char txgbe_driver_name[] = "txgbe";
@@ -513,6 +514,7 @@ static int txgbe_probe(struct pci_dev *pdev,
 	struct net_device *netdev;
 	int err, expected_gts;
 	struct wx *wx = NULL;
+	struct txgbe *txgbe;
 
 	u16 eeprom_verh = 0, eeprom_verl = 0, offset = 0;
 	u16 eeprom_cfg_blkh = 0, eeprom_cfg_blkl = 0;
@@ -663,10 +665,23 @@ static int txgbe_probe(struct pci_dev *pdev,
 			 "0x%08x", etrack_id);
 	}
 
-	err = register_netdev(netdev);
+	txgbe = devm_kzalloc(&pdev->dev, sizeof(*txgbe), GFP_KERNEL);
+	if (!txgbe) {
+		err = -ENOMEM;
+		goto err_release_hw;
+	}
+
+	txgbe->wx = wx;
+	wx->priv = txgbe;
+
+	err = txgbe_init_phy(txgbe);
 	if (err)
 		goto err_release_hw;
 
+	err = register_netdev(netdev);
+	if (err)
+		goto err_remove_phy;
+
 	pci_set_drvdata(pdev, wx);
 
 	netif_tx_stop_all_queues(netdev);
@@ -694,6 +709,8 @@ static int txgbe_probe(struct pci_dev *pdev,
 
 	return 0;
 
+err_remove_phy:
+	txgbe_remove_phy(txgbe);
 err_release_hw:
 	wx_clear_interrupt_scheme(wx);
 	wx_control_hw(wx, false);
@@ -719,11 +736,14 @@ static int txgbe_probe(struct pci_dev *pdev,
 static void txgbe_remove(struct pci_dev *pdev)
 {
 	struct wx *wx = pci_get_drvdata(pdev);
+	struct txgbe *txgbe = wx->priv;
 	struct net_device *netdev;
 
 	netdev = wx->netdev;
 	unregister_netdev(netdev);
 
+	txgbe_remove_phy(txgbe);
+
 	pci_release_selected_regions(pdev,
 				     pci_select_bars(pdev, IORESOURCE_MEM));
 
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
new file mode 100644
index 000000000000..3476074869cb
--- /dev/null
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2015 - 2023 Beijing WangXun Technology Co., Ltd. */
+
+#include <linux/gpio/property.h>
+#include <linux/i2c.h>
+#include <linux/pci.h>
+
+#include "../libwx/wx_type.h"
+#include "txgbe_type.h"
+#include "txgbe_phy.h"
+
+static int txgbe_swnodes_register(struct txgbe *txgbe)
+{
+	struct txgbe_nodes *nodes = &txgbe->nodes;
+	struct pci_dev *pdev = txgbe->wx->pdev;
+	struct software_node *swnodes;
+	u32 id;
+
+	id = (pdev->bus->number << 8) | pdev->devfn;
+
+	snprintf(nodes->gpio_name, sizeof(nodes->gpio_name), "txgbe_gpio-%x", id);
+	snprintf(nodes->i2c_name, sizeof(nodes->i2c_name), "txgbe_i2c-%x", id);
+	snprintf(nodes->sfp_name, sizeof(nodes->sfp_name), "txgbe_sfp-%x", id);
+	snprintf(nodes->phylink_name, sizeof(nodes->phylink_name), "txgbe_phylink-%x", id);
+
+	swnodes = nodes->swnodes;
+
+	/* GPIO 0: tx fault
+	 * GPIO 1: tx disable
+	 * GPIO 2: sfp module absent
+	 * GPIO 3: rx signal lost
+	 * GPIO 4: rate select, 1G(0) 10G(1)
+	 * GPIO 5: rate select, 1G(0) 10G(1)
+	 */
+	nodes->gpio_props[0] = PROPERTY_ENTRY_STRING("pinctrl-names", "default");
+	swnodes[SWNODE_GPIO] = NODE_PROP(nodes->gpio_name, nodes->gpio_props);
+	nodes->gpio0_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO], 0, GPIO_ACTIVE_HIGH);
+	nodes->gpio1_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO], 1, GPIO_ACTIVE_HIGH);
+	nodes->gpio2_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO], 2, GPIO_ACTIVE_LOW);
+	nodes->gpio3_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO], 3, GPIO_ACTIVE_HIGH);
+	nodes->gpio4_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO], 4, GPIO_ACTIVE_HIGH);
+	nodes->gpio5_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO], 5, GPIO_ACTIVE_HIGH);
+
+	nodes->i2c_props[0] = PROPERTY_ENTRY_STRING("compatible", "snps,designware-i2c");
+	nodes->i2c_props[1] = PROPERTY_ENTRY_BOOL("snps,i2c-platform");
+	nodes->i2c_props[2] = PROPERTY_ENTRY_U32("clock-frequency", I2C_MAX_STANDARD_MODE_FREQ);
+	swnodes[SWNODE_I2C] = NODE_PROP(nodes->i2c_name, nodes->i2c_props);
+	nodes->i2c_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_I2C]);
+
+	nodes->sfp_props[0] = PROPERTY_ENTRY_STRING("compatible", "sff,sfp");
+	nodes->sfp_props[1] = PROPERTY_ENTRY_REF_ARRAY("i2c-bus", nodes->i2c_ref);
+	nodes->sfp_props[2] = PROPERTY_ENTRY_REF_ARRAY("tx-fault-gpios", nodes->gpio0_ref);
+	nodes->sfp_props[3] = PROPERTY_ENTRY_REF_ARRAY("tx-disable-gpios", nodes->gpio1_ref);
+	nodes->sfp_props[4] = PROPERTY_ENTRY_REF_ARRAY("mod-def0-gpios", nodes->gpio2_ref);
+	nodes->sfp_props[5] = PROPERTY_ENTRY_REF_ARRAY("los-gpios", nodes->gpio3_ref);
+	nodes->sfp_props[6] = PROPERTY_ENTRY_REF_ARRAY("rate-select1-gpios", nodes->gpio4_ref);
+	nodes->sfp_props[7] = PROPERTY_ENTRY_REF_ARRAY("rate-select0-gpios", nodes->gpio5_ref);
+	swnodes[SWNODE_SFP] = NODE_PROP(nodes->sfp_name, nodes->sfp_props);
+	nodes->sfp_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_SFP]);
+
+	nodes->phylink_props[0] = PROPERTY_ENTRY_STRING("managed", "in-band-status");
+	nodes->phylink_props[1] = PROPERTY_ENTRY_REF_ARRAY("sfp", nodes->sfp_ref);
+	swnodes[SWNODE_PHYLINK] = NODE_PROP(nodes->phylink_name, nodes->phylink_props);
+
+	nodes->group[SWNODE_GPIO] = &swnodes[SWNODE_GPIO];
+	nodes->group[SWNODE_I2C] = &swnodes[SWNODE_I2C];
+	nodes->group[SWNODE_SFP] = &swnodes[SWNODE_SFP];
+	nodes->group[SWNODE_PHYLINK] = &swnodes[SWNODE_PHYLINK];
+
+	return software_node_register_node_group(nodes->group);
+}
+
+int txgbe_init_phy(struct txgbe *txgbe)
+{
+	int ret;
+
+	ret = txgbe_swnodes_register(txgbe);
+	if (ret) {
+		wx_err(txgbe->wx, "failed to register software nodes\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+void txgbe_remove_phy(struct txgbe *txgbe)
+{
+	software_node_unregister_node_group(txgbe->nodes.group);
+}
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.h
new file mode 100644
index 000000000000..1ab592124986
--- /dev/null
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2015 - 2023 Beijing WangXun Technology Co., Ltd. */
+
+#ifndef _TXGBE_PHY_H_
+#define _TXGBE_PHY_H_
+
+int txgbe_init_phy(struct txgbe *txgbe);
+void txgbe_remove_phy(struct txgbe *txgbe);
+
+#endif /* _TXGBE_NODE_H_ */
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index 63a1c733718d..5bef0f9df523 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -4,6 +4,8 @@
 #ifndef _TXGBE_TYPE_H_
 #define _TXGBE_TYPE_H_
 
+#include <linux/property.h>
+
 /* Device IDs */
 #define TXGBE_DEV_ID_SP1000                     0x1001
 #define TXGBE_DEV_ID_WX1820                     0x2001
@@ -99,4 +101,51 @@
 
 extern char txgbe_driver_name[];
 
+static inline struct txgbe *netdev_to_txgbe(struct net_device *netdev)
+{
+	struct wx *wx = netdev_priv(netdev);
+
+	return wx->priv;
+}
+
+#define NODE_PROP(_NAME, _PROP)			\
+	(const struct software_node) {		\
+		.name = _NAME,			\
+		.properties = _PROP,		\
+	}
+
+enum txgbe_swnodes {
+	SWNODE_GPIO = 0,
+	SWNODE_I2C,
+	SWNODE_SFP,
+	SWNODE_PHYLINK,
+	SWNODE_MAX
+};
+
+struct txgbe_nodes {
+	char gpio_name[32];
+	char i2c_name[32];
+	char sfp_name[32];
+	char phylink_name[32];
+	struct property_entry gpio_props[1];
+	struct property_entry i2c_props[3];
+	struct property_entry sfp_props[8];
+	struct property_entry phylink_props[2];
+	struct software_node_ref_args i2c_ref[1];
+	struct software_node_ref_args gpio0_ref[1];
+	struct software_node_ref_args gpio1_ref[1];
+	struct software_node_ref_args gpio2_ref[1];
+	struct software_node_ref_args gpio3_ref[1];
+	struct software_node_ref_args gpio4_ref[1];
+	struct software_node_ref_args gpio5_ref[1];
+	struct software_node_ref_args sfp_ref[1];
+	struct software_node swnodes[SWNODE_MAX];
+	const struct software_node *group[SWNODE_MAX + 1];
+};
+
+struct txgbe {
+	struct wx *wx;
+	struct txgbe_nodes nodes;
+};
+
 #endif /* _TXGBE_TYPE_H_ */
-- 
2.27.0

[PATCH net-next v8 2/9] i2c: designware: Add driver support for Wangxun 10Gb NIC

[Jiawen Wu]

Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate
with SFP.

Introduce the property "snps,i2c-platform" to match device data for Wangxun
in software node case. Since IO resource was mapped on the ethernet driver,
add a model quirk to get regmap from parent device.

The exists IP limitations are dealt as workarounds:
- IP does not support interrupt mode, it works on polling mode.
- Additionally set FIFO depth address the chip issue.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
Reviewed-by: Piotr Raczynski <piotr.raczynski@intel.com>
---
 drivers/i2c/busses/i2c-designware-common.c  |  8 ++
 drivers/i2c/busses/i2c-designware-core.h    |  4 +
 drivers/i2c/busses/i2c-designware-master.c  | 89 +++++++++++++++++++--
 drivers/i2c/busses/i2c-designware-platdrv.c | 15 ++++
 4 files changed, 111 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 0dc6b1ce663f..cdd8c67d9129 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -575,6 +575,14 @@ int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev)
 	unsigned int param;
 	int ret;
 
+	/* DW_IC_COMP_PARAM_1 not implement for IP issue */
+	if ((dev->flags & MODEL_MASK) == MODEL_WANGXUN_SP) {
+		dev->tx_fifo_depth = TXGBE_TX_FIFO_DEPTH;
+		dev->rx_fifo_depth = TXGBE_RX_FIFO_DEPTH;
+
+		return 0;
+	}
+
 	/*
 	 * Try to detect the FIFO depth if not set by interface driver,
 	 * the depth could be from 2 to 256 from HW spec.
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index c5d87aae39c6..782532c20bd1 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -303,6 +303,7 @@ struct dw_i2c_dev {
 #define MODEL_MSCC_OCELOT			BIT(8)
 #define MODEL_BAIKAL_BT1			BIT(9)
 #define MODEL_AMD_NAVI_GPU			BIT(10)
+#define MODEL_WANGXUN_SP			BIT(11)
 #define MODEL_MASK				GENMASK(11, 8)
 
 /*
@@ -312,6 +313,9 @@ struct dw_i2c_dev {
 #define AMD_UCSI_INTR_REG			0x474
 #define AMD_UCSI_INTR_EN			0xd
 
+#define TXGBE_TX_FIFO_DEPTH			4
+#define TXGBE_RX_FIFO_DEPTH			0
+
 struct i2c_dw_semaphore_callbacks {
 	int	(*probe)(struct dw_i2c_dev *dev);
 	void	(*remove)(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index 55ea91a63382..3bfd7a2232db 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -354,6 +354,68 @@ static int amd_i2c_dw_xfer_quirk(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	return 0;
 }
 
+static int i2c_dw_poll_tx_empty(struct dw_i2c_dev *dev)
+{
+	u32 val;
+
+	return regmap_read_poll_timeout(dev->map, DW_IC_RAW_INTR_STAT, val,
+					val & DW_IC_INTR_TX_EMPTY,
+					100, 1000);
+}
+
+static int i2c_dw_poll_rx_full(struct dw_i2c_dev *dev)
+{
+	u32 val;
+
+	return regmap_read_poll_timeout(dev->map, DW_IC_RAW_INTR_STAT, val,
+					val & DW_IC_INTR_RX_FULL,
+					100, 1000);
+}
+
+static int txgbe_i2c_dw_xfer_quirk(struct i2c_adapter *adap, struct i2c_msg *msgs,
+				   int num_msgs)
+{
+	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+	int msg_idx, buf_len, data_idx, ret;
+	unsigned int val, stop = 0;
+	u8 *buf;
+
+	dev->msgs = msgs;
+	dev->msgs_num = num_msgs;
+	i2c_dw_xfer_init(dev);
+	regmap_write(dev->map, DW_IC_INTR_MASK, 0);
+
+	for (msg_idx = 0; msg_idx < num_msgs; msg_idx++) {
+		buf = msgs[msg_idx].buf;
+		buf_len = msgs[msg_idx].len;
+
+		for (data_idx = 0; data_idx < buf_len; data_idx++) {
+			if (msg_idx == num_msgs - 1 && data_idx == buf_len - 1)
+				stop |= BIT(9);
+
+			if (msgs[msg_idx].flags & I2C_M_RD) {
+				regmap_write(dev->map, DW_IC_DATA_CMD, 0x100 | stop);
+
+				ret = i2c_dw_poll_rx_full(dev);
+				if (ret)
+					return ret;
+
+				regmap_read(dev->map, DW_IC_DATA_CMD, &val);
+				buf[data_idx] = val;
+			} else {
+				ret = i2c_dw_poll_tx_empty(dev);
+				if (ret)
+					return ret;
+
+				regmap_write(dev->map, DW_IC_DATA_CMD,
+					     buf[data_idx] | stop);
+			}
+		}
+	}
+
+	return num_msgs;
+}
+
 /*
  * Initiate (and continue) low level master read/write transaction.
  * This function is only called from i2c_dw_isr, and pumping i2c_msg
@@ -559,13 +621,19 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	pm_runtime_get_sync(dev->dev);
 
 	/*
-	 * Initiate I2C message transfer when AMD NAVI GPU card is enabled,
+	 * Initiate I2C message transfer when polling mode is enabled,
 	 * As it is polling based transfer mechanism, which does not support
 	 * interrupt based functionalities of existing DesignWare driver.
 	 */
-	if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU) {
+	switch (dev->flags & MODEL_MASK) {
+	case MODEL_AMD_NAVI_GPU:
 		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
 		goto done_nolock;
+	case MODEL_WANGXUN_SP:
+		ret = txgbe_i2c_dw_xfer_quirk(adap, msgs, num);
+		goto done_nolock;
+	default:
+		break;
 	}
 
 	reinit_completion(&dev->cmd_complete);
@@ -848,7 +916,7 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
 	return 0;
 }
 
-static int amd_i2c_adap_quirk(struct dw_i2c_dev *dev)
+static int i2c_dw_poll_adap_quirk(struct dw_i2c_dev *dev)
 {
 	struct i2c_adapter *adap = &dev->adapter;
 	int ret;
@@ -862,6 +930,17 @@ static int amd_i2c_adap_quirk(struct dw_i2c_dev *dev)
 	return ret;
 }
 
+static bool i2c_dw_is_model_poll(struct dw_i2c_dev *dev)
+{
+	switch (dev->flags & MODEL_MASK) {
+	case MODEL_AMD_NAVI_GPU:
+	case MODEL_WANGXUN_SP:
+		return true;
+	default:
+		return false;
+	}
+}
+
 int i2c_dw_probe_master(struct dw_i2c_dev *dev)
 {
 	struct i2c_adapter *adap = &dev->adapter;
@@ -917,8 +996,8 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev)
 	adap->dev.parent = dev->dev;
 	i2c_set_adapdata(adap, dev);
 
-	if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU)
-		return amd_i2c_adap_quirk(dev);
+	if (i2c_dw_is_model_poll(dev))
+		return i2c_dw_poll_adap_quirk(dev);
 
 	if (dev->flags & ACCESS_NO_IRQ_SUSPEND) {
 		irq_flags = IRQF_NO_SUSPEND;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 89ad88c54754..1bf50150386d 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -168,6 +168,15 @@ static inline int dw_i2c_of_configure(struct platform_device *pdev)
 }
 #endif
 
+static int txgbe_i2c_request_regs(struct dw_i2c_dev *dev)
+{
+	dev->map = dev_get_regmap(dev->dev->parent, NULL);
+	if (!dev->map)
+		return -ENODEV;
+
+	return 0;
+}
+
 static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
 {
 	pm_runtime_disable(dev->dev);
@@ -185,6 +194,9 @@ static int dw_i2c_plat_request_regs(struct dw_i2c_dev *dev)
 	case MODEL_BAIKAL_BT1:
 		ret = bt1_i2c_request_regs(dev);
 		break;
+	case MODEL_WANGXUN_SP:
+		ret = txgbe_i2c_request_regs(dev);
+		break;
 	default:
 		dev->base = devm_platform_ioremap_resource(pdev, 0);
 		ret = PTR_ERR_OR_ZERO(dev->base);
@@ -277,6 +289,9 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	dev->flags = (uintptr_t)device_get_match_data(&pdev->dev);
+	if (device_property_present(&pdev->dev, "snps,i2c-platform"))
+		dev->flags |= MODEL_WANGXUN_SP;
+
 	dev->dev = &pdev->dev;
 	dev->irq = irq;
 	platform_set_drvdata(pdev, dev);
-- 
2.27.0

[PATCH net-next v8 3/9] net: txgbe: Register fixed rate clock

[Jiawen Wu]

In order for I2C to be able to work in standard mode, register a fixed
rate clock for each I2C device.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/Kconfig          |  1 +
 .../net/ethernet/wangxun/txgbe/txgbe_phy.c    | 41 +++++++++++++++++++
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   |  2 +
 3 files changed, 44 insertions(+)

diff --git a/drivers/net/ethernet/wangxun/Kconfig b/drivers/net/ethernet/wangxun/Kconfig
index c9d88673d306..a62614eeed2e 100644
--- a/drivers/net/ethernet/wangxun/Kconfig
+++ b/drivers/net/ethernet/wangxun/Kconfig
@@ -40,6 +40,7 @@ config NGBE
 config TXGBE
 	tristate "Wangxun(R) 10GbE PCI Express adapters support"
 	depends on PCI
+	select COMMON_CLK
 	select LIBWX
 	help
 	  This driver supports Wangxun(R) 10GbE PCI Express family of
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
index 3476074869cb..f1c455d799f3 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
@@ -2,6 +2,8 @@
 /* Copyright (c) 2015 - 2023 Beijing WangXun Technology Co., Ltd. */
 
 #include <linux/gpio/property.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
 #include <linux/i2c.h>
 #include <linux/pci.h>
 
@@ -70,6 +72,32 @@ static int txgbe_swnodes_register(struct txgbe *txgbe)
 	return software_node_register_node_group(nodes->group);
 }
 
+static int txgbe_clock_register(struct txgbe *txgbe)
+{
+	struct pci_dev *pdev = txgbe->wx->pdev;
+	struct clk_lookup *clock;
+	char clk_name[32];
+	struct clk *clk;
+
+	snprintf(clk_name, sizeof(clk_name), "i2c_designware.%d",
+		 (pdev->bus->number << 8) | pdev->devfn);
+
+	clk = clk_register_fixed_rate(NULL, clk_name, NULL, 0, 156250000);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	clock = clkdev_create(clk, NULL, clk_name);
+	if (!clock) {
+		clk_unregister(clk);
+		return -ENOMEM;
+	}
+
+	txgbe->clk = clk;
+	txgbe->clock = clock;
+
+	return 0;
+}
+
 int txgbe_init_phy(struct txgbe *txgbe)
 {
 	int ret;
@@ -80,10 +108,23 @@ int txgbe_init_phy(struct txgbe *txgbe)
 		return ret;
 	}
 
+	ret = txgbe_clock_register(txgbe);
+	if (ret) {
+		wx_err(txgbe->wx, "failed to register clock: %d\n", ret);
+		goto err_unregister_swnode;
+	}
+
 	return 0;
+
+err_unregister_swnode:
+	software_node_unregister_node_group(txgbe->nodes.group);
+
+	return ret;
 }
 
 void txgbe_remove_phy(struct txgbe *txgbe)
 {
+	clkdev_drop(txgbe->clock);
+	clk_unregister(txgbe->clk);
 	software_node_unregister_node_group(txgbe->nodes.group);
 }
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index 5bef0f9df523..cdbc4b37f832 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -146,6 +146,8 @@ struct txgbe_nodes {
 struct txgbe {
 	struct wx *wx;
 	struct txgbe_nodes nodes;
+	struct clk_lookup *clock;
+	struct clk *clk;
 };
 
 #endif /* _TXGBE_TYPE_H_ */
-- 
2.27.0

[PATCH net-next v8 4/9] net: txgbe: Register I2C platform device

[Jiawen Wu]

Register the platform device to use Designware I2C bus master driver.
Use regmap to read/write I2C device region from given base offset.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Piotr Raczynski <piotr.raczynski@intel.com>
---
 drivers/net/ethernet/wangxun/Kconfig          |  2 +
 .../net/ethernet/wangxun/txgbe/txgbe_phy.c    | 71 +++++++++++++++++++
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   |  4 ++
 3 files changed, 77 insertions(+)

diff --git a/drivers/net/ethernet/wangxun/Kconfig b/drivers/net/ethernet/wangxun/Kconfig
index a62614eeed2e..ec058a72afb6 100644
--- a/drivers/net/ethernet/wangxun/Kconfig
+++ b/drivers/net/ethernet/wangxun/Kconfig
@@ -40,6 +40,8 @@ config NGBE
 config TXGBE
 	tristate "Wangxun(R) 10GbE PCI Express adapters support"
 	depends on PCI
+	select I2C_DESIGNWARE_PLATFORM
+	select REGMAP
 	select COMMON_CLK
 	select LIBWX
 	help
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
index f1c455d799f3..a76c9fd74864 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
@@ -1,7 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2015 - 2023 Beijing WangXun Technology Co., Ltd. */
 
+#include <linux/platform_device.h>
 #include <linux/gpio/property.h>
+#include <linux/regmap.h>
 #include <linux/clkdev.h>
 #include <linux/clk-provider.h>
 #include <linux/i2c.h>
@@ -98,6 +100,65 @@ static int txgbe_clock_register(struct txgbe *txgbe)
 	return 0;
 }
 
+static int txgbe_i2c_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct wx *wx = context;
+
+	*val = rd32(wx, reg + TXGBE_I2C_BASE);
+
+	return 0;
+}
+
+static int txgbe_i2c_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct wx *wx = context;
+
+	wr32(wx, reg + TXGBE_I2C_BASE, val);
+
+	return 0;
+}
+
+static const struct regmap_config i2c_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_read = txgbe_i2c_read,
+	.reg_write = txgbe_i2c_write,
+};
+
+static int txgbe_i2c_register(struct txgbe *txgbe)
+{
+	struct platform_device_info info = {};
+	struct platform_device *i2c_dev;
+	struct wx *wx = txgbe->wx;
+	struct regmap *i2c_regmap;
+	struct resource res = {};
+	struct pci_dev *pdev;
+
+	pdev = wx->pdev;
+	i2c_regmap = devm_regmap_init(&pdev->dev, NULL, wx,
+				      &i2c_regmap_config);
+	if (IS_ERR(i2c_regmap)) {
+		wx_err(wx, "failed to init regmap\n");
+		return PTR_ERR(i2c_regmap);
+	}
+
+	info.parent = &pdev->dev;
+	info.fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_I2C]);
+	info.name = "i2c_designware";
+	info.id = (pdev->bus->number << 8) | pdev->devfn;
+
+	res = DEFINE_RES_IRQ(pdev->irq);
+	info.res = &res;
+	info.num_res = 1;
+	i2c_dev = platform_device_register_full(&info);
+	if (IS_ERR(i2c_dev))
+		return PTR_ERR(i2c_dev);
+
+	txgbe->i2c_dev = i2c_dev;
+
+	return 0;
+}
+
 int txgbe_init_phy(struct txgbe *txgbe)
 {
 	int ret;
@@ -114,8 +175,17 @@ int txgbe_init_phy(struct txgbe *txgbe)
 		goto err_unregister_swnode;
 	}
 
+	ret = txgbe_i2c_register(txgbe);
+	if (ret) {
+		wx_err(txgbe->wx, "failed to init i2c interface: %d\n", ret);
+		goto err_unregister_clk;
+	}
+
 	return 0;
 
+err_unregister_clk:
+	clkdev_drop(txgbe->clock);
+	clk_unregister(txgbe->clk);
 err_unregister_swnode:
 	software_node_unregister_node_group(txgbe->nodes.group);
 
@@ -124,6 +194,7 @@ int txgbe_init_phy(struct txgbe *txgbe)
 
 void txgbe_remove_phy(struct txgbe *txgbe)
 {
+	platform_device_unregister(txgbe->i2c_dev);
 	clkdev_drop(txgbe->clock);
 	clk_unregister(txgbe->clk);
 	software_node_unregister_node_group(txgbe->nodes.group);
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index cdbc4b37f832..55979abf01f2 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -55,6 +55,9 @@
 #define TXGBE_TS_CTL                            0x10300
 #define TXGBE_TS_CTL_EVAL_MD                    BIT(31)
 
+/* I2C registers */
+#define TXGBE_I2C_BASE                          0x14900
+
 /* Part Number String Length */
 #define TXGBE_PBANUM_LENGTH                     32
 
@@ -146,6 +149,7 @@ struct txgbe_nodes {
 struct txgbe {
 	struct wx *wx;
 	struct txgbe_nodes nodes;
+	struct platform_device *i2c_dev;
 	struct clk_lookup *clock;
 	struct clk *clk;
 };
-- 
2.27.0

[PATCH net-next v8 5/9] net: txgbe: Add SFP module identify

[Jiawen Wu]

Register SFP platform device to get modules information.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Piotr Raczynski <piotr.raczynski@intel.com>
---
 drivers/net/ethernet/wangxun/Kconfig          |  1 +
 .../net/ethernet/wangxun/txgbe/txgbe_phy.c    | 28 +++++++++++++++++++
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   |  1 +
 3 files changed, 30 insertions(+)

diff --git a/drivers/net/ethernet/wangxun/Kconfig b/drivers/net/ethernet/wangxun/Kconfig
index ec058a72afb6..90812d76181d 100644
--- a/drivers/net/ethernet/wangxun/Kconfig
+++ b/drivers/net/ethernet/wangxun/Kconfig
@@ -44,6 +44,7 @@ config TXGBE
 	select REGMAP
 	select COMMON_CLK
 	select LIBWX
+	select SFP
 	help
 	  This driver supports Wangxun(R) 10GbE PCI Express family of
 	  adapters.
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
index a76c9fd74864..38830e280031 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
@@ -159,6 +159,25 @@ static int txgbe_i2c_register(struct txgbe *txgbe)
 	return 0;
 }
 
+static int txgbe_sfp_register(struct txgbe *txgbe)
+{
+	struct pci_dev *pdev = txgbe->wx->pdev;
+	struct platform_device_info info = {};
+	struct platform_device *sfp_dev;
+
+	info.parent = &pdev->dev;
+	info.fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_SFP]);
+	info.name = "sfp";
+	info.id = (pdev->bus->number << 8) | pdev->devfn;
+	sfp_dev = platform_device_register_full(&info);
+	if (IS_ERR(sfp_dev))
+		return PTR_ERR(sfp_dev);
+
+	txgbe->sfp_dev = sfp_dev;
+
+	return 0;
+}
+
 int txgbe_init_phy(struct txgbe *txgbe)
 {
 	int ret;
@@ -181,8 +200,16 @@ int txgbe_init_phy(struct txgbe *txgbe)
 		goto err_unregister_clk;
 	}
 
+	ret = txgbe_sfp_register(txgbe);
+	if (ret) {
+		wx_err(txgbe->wx, "failed to register sfp\n");
+		goto err_unregister_i2c;
+	}
+
 	return 0;
 
+err_unregister_i2c:
+	platform_device_unregister(txgbe->i2c_dev);
 err_unregister_clk:
 	clkdev_drop(txgbe->clock);
 	clk_unregister(txgbe->clk);
@@ -194,6 +221,7 @@ int txgbe_init_phy(struct txgbe *txgbe)
 
 void txgbe_remove_phy(struct txgbe *txgbe)
 {
+	platform_device_unregister(txgbe->sfp_dev);
 	platform_device_unregister(txgbe->i2c_dev);
 	clkdev_drop(txgbe->clock);
 	clk_unregister(txgbe->clk);
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index 55979abf01f2..fc91e0fc37a6 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -149,6 +149,7 @@ struct txgbe_nodes {
 struct txgbe {
 	struct wx *wx;
 	struct txgbe_nodes nodes;
+	struct platform_device *sfp_dev;
 	struct platform_device *i2c_dev;
 	struct clk_lookup *clock;
 	struct clk *clk;
-- 
2.27.0

[PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Jiawen Wu]

Register GPIO chip and handle GPIO IRQ for SFP socket.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/Kconfig          |   2 +
 drivers/net/ethernet/wangxun/libwx/wx_lib.c   |   3 +-
 drivers/net/ethernet/wangxun/libwx/wx_type.h  |   3 +
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   |  20 +-
 .../net/ethernet/wangxun/txgbe/txgbe_phy.c    | 241 ++++++++++++++++++
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   |  23 ++
 6 files changed, 273 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/Kconfig b/drivers/net/ethernet/wangxun/Kconfig
index 90812d76181d..73f4492928c0 100644
--- a/drivers/net/ethernet/wangxun/Kconfig
+++ b/drivers/net/ethernet/wangxun/Kconfig
@@ -41,6 +41,8 @@ config TXGBE
 	tristate "Wangxun(R) 10GbE PCI Express adapters support"
 	depends on PCI
 	select I2C_DESIGNWARE_PLATFORM
+	select GPIOLIB_IRQCHIP
+	select GPIOLIB
 	select REGMAP
 	select COMMON_CLK
 	select LIBWX
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
index 1e8d8b7b0c62..590215303d45 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
@@ -1348,7 +1348,8 @@ void wx_free_irq(struct wx *wx)
 		free_irq(entry->vector, q_vector);
 	}
 
-	free_irq(wx->msix_entries[vector].vector, wx);
+	if (wx->mac.type == wx_mac_em)
+		free_irq(wx->msix_entries[vector].vector, wx);
 }
 EXPORT_SYMBOL(wx_free_irq);
 
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
index 97bce855bc60..bb8c63bdd5d4 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
@@ -79,7 +79,9 @@
 #define WX_GPIO_INTMASK              0x14834
 #define WX_GPIO_INTTYPE_LEVEL        0x14838
 #define WX_GPIO_POLARITY             0x1483C
+#define WX_GPIO_INTSTATUS            0x14844
 #define WX_GPIO_EOI                  0x1484C
+#define WX_GPIO_EXT                  0x14850
 
 /*********************** Transmit DMA registers **************************/
 /* transmit global control */
@@ -643,6 +645,7 @@ struct wx {
 	bool wol_enabled;
 	bool ncsi_enabled;
 	bool gpio_ctrl;
+	spinlock_t gpio_lock;
 
 	/* Tx fast path data */
 	int num_tx_queues;
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index e10296abf5b4..ded04e9e136f 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -82,6 +82,8 @@ static int txgbe_enumerate_functions(struct wx *wx)
  **/
 static void txgbe_irq_enable(struct wx *wx, bool queues)
 {
+	wr32(wx, WX_PX_MISC_IEN, TXGBE_PX_MISC_IEN_MASK);
+
 	/* unmask interrupt */
 	wx_intr_enable(wx, TXGBE_INTR_MISC(wx));
 	if (queues)
@@ -129,17 +131,6 @@ static irqreturn_t txgbe_intr(int __always_unused irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t txgbe_msix_other(int __always_unused irq, void *data)
-{
-	struct wx *wx = data;
-
-	/* re-enable the original interrupt state */
-	if (netif_running(wx->netdev))
-		txgbe_irq_enable(wx, false);
-
-	return IRQ_HANDLED;
-}
-
 /**
  * txgbe_request_msix_irqs - Initialize MSI-X interrupts
  * @wx: board private structure
@@ -171,13 +162,6 @@ static int txgbe_request_msix_irqs(struct wx *wx)
 		}
 	}
 
-	err = request_irq(wx->msix_entries[vector].vector,
-			  txgbe_msix_other, 0, netdev->name, wx);
-	if (err) {
-		wx_err(wx, "request_irq for msix_other failed: %d\n", err);
-		goto free_queue_irqs;
-	}
-
 	return 0;
 
 free_queue_irqs:
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
index 38830e280031..aa8b4444e77a 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
@@ -2,6 +2,8 @@
 /* Copyright (c) 2015 - 2023 Beijing WangXun Technology Co., Ltd. */
 
 #include <linux/platform_device.h>
+#include <linux/gpio/machine.h>
+#include <linux/gpio/driver.h>
 #include <linux/gpio/property.h>
 #include <linux/regmap.h>
 #include <linux/clkdev.h>
@@ -10,6 +12,7 @@
 #include <linux/pci.h>
 
 #include "../libwx/wx_type.h"
+#include "../libwx/wx_hw.h"
 #include "txgbe_type.h"
 #include "txgbe_phy.h"
 
@@ -74,6 +77,238 @@ static int txgbe_swnodes_register(struct txgbe *txgbe)
 	return software_node_register_node_group(nodes->group);
 }
 
+static int txgbe_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct wx *wx = gpiochip_get_data(chip);
+	int val;
+
+	val = rd32m(wx, WX_GPIO_EXT, BIT(offset));
+
+	return !!(val & BIT(offset));
+}
+
+static int txgbe_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct wx *wx = gpiochip_get_data(chip);
+	u32 val;
+
+	val = rd32(wx, WX_GPIO_DDR);
+	if (BIT(offset) & val)
+		return GPIO_LINE_DIRECTION_OUT;
+
+	return GPIO_LINE_DIRECTION_IN;
+}
+
+static int txgbe_gpio_direction_in(struct gpio_chip *chip, unsigned int offset)
+{
+	struct wx *wx = gpiochip_get_data(chip);
+	unsigned long flags;
+
+	spin_lock_irqsave(&wx->gpio_lock, flags);
+	wr32m(wx, WX_GPIO_DDR, BIT(offset), 0);
+	spin_unlock_irqrestore(&wx->gpio_lock, flags);
+
+	return 0;
+}
+
+static int txgbe_gpio_direction_out(struct gpio_chip *chip, unsigned int offset,
+				    int val)
+{
+	struct wx *wx = gpiochip_get_data(chip);
+	unsigned long flags;
+	u32 set;
+
+	spin_lock_irqsave(&wx->gpio_lock, flags);
+	set = val ? BIT(offset) : 0;
+	wr32m(wx, WX_GPIO_DR, BIT(offset), set);
+	wr32m(wx, WX_GPIO_DDR, BIT(offset), BIT(offset));
+	spin_unlock_irqrestore(&wx->gpio_lock, flags);
+
+	return 0;
+}
+
+static void txgbe_gpio_irq_ack(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	struct wx *wx = gpiochip_get_data(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&wx->gpio_lock, flags);
+	wr32(wx, WX_GPIO_EOI, BIT(hwirq));
+	spin_unlock_irqrestore(&wx->gpio_lock, flags);
+}
+
+static void txgbe_gpio_irq_mask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	struct wx *wx = gpiochip_get_data(gc);
+	unsigned long flags;
+
+	gpiochip_disable_irq(gc, hwirq);
+
+	spin_lock_irqsave(&wx->gpio_lock, flags);
+	wr32m(wx, WX_GPIO_INTMASK, BIT(hwirq), BIT(hwirq));
+	spin_unlock_irqrestore(&wx->gpio_lock, flags);
+}
+
+static void txgbe_gpio_irq_unmask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	struct wx *wx = gpiochip_get_data(gc);
+	unsigned long flags;
+
+	gpiochip_enable_irq(gc, hwirq);
+
+	spin_lock_irqsave(&wx->gpio_lock, flags);
+	wr32m(wx, WX_GPIO_INTMASK, BIT(hwirq), 0);
+	spin_unlock_irqrestore(&wx->gpio_lock, flags);
+}
+
+static void txgbe_toggle_trigger(struct gpio_chip *gc, unsigned int offset)
+{
+	struct wx *wx = gpiochip_get_data(gc);
+	u32 pol;
+	int val;
+
+	pol = rd32(wx, WX_GPIO_POLARITY);
+	val = gc->get(gc, offset);
+	if (val)
+		pol &= ~BIT(offset);
+	else
+		pol |= BIT(offset);
+
+	wr32(wx, WX_GPIO_POLARITY, pol);
+}
+
+static int txgbe_gpio_set_type(struct irq_data *d, unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	struct wx *wx = gpiochip_get_data(gc);
+	u32 level, polarity, mask;
+	unsigned long flags;
+
+	spin_lock_irqsave(&wx->gpio_lock, flags);
+
+	mask = BIT(hwirq);
+
+	if (type & IRQ_TYPE_LEVEL_MASK) {
+		level = 0;
+		irq_set_handler_locked(d, handle_level_irq);
+	} else {
+		level = mask;
+		irq_set_handler_locked(d, handle_edge_irq);
+	}
+
+	if (type == IRQ_TYPE_EDGE_RISING || type == IRQ_TYPE_LEVEL_HIGH)
+		polarity = mask;
+	else
+		polarity = 0;
+
+	wr32m(wx, WX_GPIO_INTEN, mask, mask);
+	wr32m(wx, WX_GPIO_INTTYPE_LEVEL, mask, level);
+	if (type == IRQ_TYPE_EDGE_BOTH)
+		txgbe_toggle_trigger(gc, hwirq);
+	else
+		wr32m(wx, WX_GPIO_POLARITY, mask, polarity);
+
+	spin_unlock_irqrestore(&wx->gpio_lock, flags);
+
+	return 0;
+}
+
+static const struct irq_chip txgbe_gpio_irq_chip = {
+	.name = "txgbe_gpio_irq",
+	.irq_ack = txgbe_gpio_irq_ack,
+	.irq_mask = txgbe_gpio_irq_mask,
+	.irq_unmask = txgbe_gpio_irq_unmask,
+	.irq_set_type = txgbe_gpio_set_type,
+	.flags = IRQCHIP_IMMUTABLE,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
+static void txgbe_irq_handler(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct wx *wx = irq_desc_get_handler_data(desc);
+	struct txgbe *txgbe = wx->priv;
+	irq_hw_number_t hwirq;
+	unsigned long gpioirq;
+	struct gpio_chip *gc;
+
+	chained_irq_enter(chip, desc);
+
+	gpioirq = rd32(wx, WX_GPIO_INTSTATUS);
+
+	gc = txgbe->gpio;
+	for_each_set_bit(hwirq, &gpioirq, gc->ngpio) {
+		int gpio = irq_find_mapping(gc->irq.domain, hwirq);
+		u32 irq_type = irq_get_trigger_type(gpio);
+
+		generic_handle_irq(gpio);
+
+		if ((irq_type & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_EDGE_BOTH)
+			txgbe_toggle_trigger(gc, hwirq);
+	}
+
+	chained_irq_exit(chip, desc);
+
+	/* unmask interrupt */
+	wx_intr_enable(wx, TXGBE_INTR_MISC(wx));
+}
+
+static int txgbe_gpio_init(struct txgbe *txgbe)
+{
+	struct gpio_irq_chip *girq;
+	struct wx *wx = txgbe->wx;
+	struct gpio_chip *gc;
+	struct device *dev;
+	int ret;
+
+	dev = &wx->pdev->dev;
+
+	gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
+	if (!gc)
+		return -ENOMEM;
+
+	gc->label = devm_kasprintf(dev, GFP_KERNEL, "txgbe_gpio-%x",
+				   (wx->pdev->bus->number << 8) | wx->pdev->devfn);
+	gc->base = -1;
+	gc->ngpio = 6;
+	gc->owner = THIS_MODULE;
+	gc->parent = dev;
+	gc->fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_GPIO]);
+	gc->get = txgbe_gpio_get;
+	gc->get_direction = txgbe_gpio_get_direction;
+	gc->direction_input = txgbe_gpio_direction_in;
+	gc->direction_output = txgbe_gpio_direction_out;
+
+	girq = &gc->irq;
+	gpio_irq_chip_set_chip(girq, &txgbe_gpio_irq_chip);
+	girq->parent_handler = txgbe_irq_handler;
+	girq->parent_handler_data = wx;
+	girq->num_parents = 1;
+	girq->parents = devm_kcalloc(dev, girq->num_parents,
+				     sizeof(*girq->parents), GFP_KERNEL);
+	if (!girq->parents)
+		return -ENOMEM;
+	girq->parents[0] = wx->msix_entries[wx->num_q_vectors].vector;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_bad_irq;
+
+	ret = devm_gpiochip_add_data(dev, gc, wx);
+	if (ret)
+		return ret;
+
+	spin_lock_init(&wx->gpio_lock);
+	txgbe->gpio = gc;
+
+	return 0;
+}
+
 static int txgbe_clock_register(struct txgbe *txgbe)
 {
 	struct pci_dev *pdev = txgbe->wx->pdev;
@@ -188,6 +423,12 @@ int txgbe_init_phy(struct txgbe *txgbe)
 		return ret;
 	}
 
+	ret = txgbe_gpio_init(txgbe);
+	if (ret) {
+		wx_err(txgbe->wx, "failed to init gpio\n");
+		goto err_unregister_swnode;
+	}
+
 	ret = txgbe_clock_register(txgbe);
 	if (ret) {
 		wx_err(txgbe->wx, "failed to register clock: %d\n", ret);
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index fc91e0fc37a6..6c903e4517c7 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -55,6 +55,28 @@
 #define TXGBE_TS_CTL                            0x10300
 #define TXGBE_TS_CTL_EVAL_MD                    BIT(31)
 
+/* GPIO register bit */
+#define TXGBE_GPIOBIT_0                         BIT(0) /* I:tx fault */
+#define TXGBE_GPIOBIT_1                         BIT(1) /* O:tx disabled */
+#define TXGBE_GPIOBIT_2                         BIT(2) /* I:sfp module absent */
+#define TXGBE_GPIOBIT_3                         BIT(3) /* I:rx signal lost */
+#define TXGBE_GPIOBIT_4                         BIT(4) /* O:rate select, 1G(0) 10G(1) */
+#define TXGBE_GPIOBIT_5                         BIT(5) /* O:rate select, 1G(0) 10G(1) */
+
+/* Extended Interrupt Enable Set */
+#define TXGBE_PX_MISC_ETH_LKDN                  BIT(8)
+#define TXGBE_PX_MISC_DEV_RST                   BIT(10)
+#define TXGBE_PX_MISC_ETH_EVENT                 BIT(17)
+#define TXGBE_PX_MISC_ETH_LK                    BIT(18)
+#define TXGBE_PX_MISC_ETH_AN                    BIT(19)
+#define TXGBE_PX_MISC_INT_ERR                   BIT(20)
+#define TXGBE_PX_MISC_GPIO                      BIT(26)
+#define TXGBE_PX_MISC_IEN_MASK                            \
+	(TXGBE_PX_MISC_ETH_LKDN | TXGBE_PX_MISC_DEV_RST | \
+	 TXGBE_PX_MISC_ETH_EVENT | TXGBE_PX_MISC_ETH_LK | \
+	 TXGBE_PX_MISC_ETH_AN | TXGBE_PX_MISC_INT_ERR |   \
+	 TXGBE_PX_MISC_GPIO)
+
 /* I2C registers */
 #define TXGBE_I2C_BASE                          0x14900
 
@@ -153,6 +175,7 @@ struct txgbe {
 	struct platform_device *i2c_dev;
 	struct clk_lookup *clock;
 	struct clk *clk;
+	struct gpio_chip *gpio;
 };
 
 #endif /* _TXGBE_TYPE_H_ */
-- 
2.27.0

[PATCH net-next v8 7/9] net: pcs: Add 10GBASE-R mode for Synopsys Designware XPCS

[Jiawen Wu]

Add basic support for XPCS using 10GBASE-R interface. This mode will
be extended to use interrupt, so set pcs.poll false. And avoid soft
reset so that the device using this mode is in the default configuration.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/pcs/pcs-xpcs.c   | 30 ++++++++++++++++++++++++++++++
 include/linux/pcs/pcs-xpcs.h |  1 +
 2 files changed, 31 insertions(+)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 539cd43eae8d..c7c8b8d1311f 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -64,6 +64,16 @@ static const int xpcs_xlgmii_features[] = {
 	__ETHTOOL_LINK_MODE_MASK_NBITS,
 };
 
+static const int xpcs_10gbaser_features[] = {
+	ETHTOOL_LINK_MODE_Pause_BIT,
+	ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+	ETHTOOL_LINK_MODE_10000baseSR_Full_BIT,
+	ETHTOOL_LINK_MODE_10000baseLR_Full_BIT,
+	ETHTOOL_LINK_MODE_10000baseLRM_Full_BIT,
+	ETHTOOL_LINK_MODE_10000baseER_Full_BIT,
+	__ETHTOOL_LINK_MODE_MASK_NBITS,
+};
+
 static const int xpcs_sgmii_features[] = {
 	ETHTOOL_LINK_MODE_Pause_BIT,
 	ETHTOOL_LINK_MODE_Asym_Pause_BIT,
@@ -106,6 +116,10 @@ static const phy_interface_t xpcs_xlgmii_interfaces[] = {
 	PHY_INTERFACE_MODE_XLGMII,
 };
 
+static const phy_interface_t xpcs_10gbaser_interfaces[] = {
+	PHY_INTERFACE_MODE_10GBASER,
+};
+
 static const phy_interface_t xpcs_sgmii_interfaces[] = {
 	PHY_INTERFACE_MODE_SGMII,
 };
@@ -123,6 +137,7 @@ enum {
 	DW_XPCS_USXGMII,
 	DW_XPCS_10GKR,
 	DW_XPCS_XLGMII,
+	DW_XPCS_10GBASER,
 	DW_XPCS_SGMII,
 	DW_XPCS_1000BASEX,
 	DW_XPCS_2500BASEX,
@@ -246,6 +261,7 @@ static int xpcs_soft_reset(struct dw_xpcs *xpcs,
 
 	switch (compat->an_mode) {
 	case DW_AN_C73:
+	case DW_10GBASER:
 		dev = MDIO_MMD_PCS;
 		break;
 	case DW_AN_C37_SGMII:
@@ -872,6 +888,8 @@ int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
 		return -ENODEV;
 
 	switch (compat->an_mode) {
+	case DW_10GBASER:
+		break;
 	case DW_AN_C73:
 		if (phylink_autoneg_inband(mode)) {
 			ret = xpcs_config_aneg_c73(xpcs, compat);
@@ -1033,6 +1051,9 @@ static void xpcs_get_state(struct phylink_pcs *pcs,
 		return;
 
 	switch (compat->an_mode) {
+	case DW_10GBASER:
+		phylink_mii_c45_pcs_get_state(xpcs->mdiodev, state);
+		break;
 	case DW_AN_C73:
 		ret = xpcs_get_state_c73(xpcs, state, compat);
 		if (ret) {
@@ -1188,6 +1209,12 @@ static const struct xpcs_compat synopsys_xpcs_compat[DW_XPCS_INTERFACE_MAX] = {
 		.num_interfaces = ARRAY_SIZE(xpcs_xlgmii_interfaces),
 		.an_mode = DW_AN_C73,
 	},
+	[DW_XPCS_10GBASER] = {
+		.supported = xpcs_10gbaser_features,
+		.interface = xpcs_10gbaser_interfaces,
+		.num_interfaces = ARRAY_SIZE(xpcs_10gbaser_interfaces),
+		.an_mode = DW_10GBASER,
+	},
 	[DW_XPCS_SGMII] = {
 		.supported = xpcs_sgmii_features,
 		.interface = xpcs_sgmii_interfaces,
@@ -1290,6 +1317,9 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
 		}
 
 		xpcs->pcs.ops = &xpcs_phylink_ops;
+		if (compat->an_mode == DW_10GBASER)
+			return xpcs;
+
 		xpcs->pcs.poll = true;
 
 		ret = xpcs_soft_reset(xpcs, compat);
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index d2da1e0b4a92..61df0c717a0e 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -18,6 +18,7 @@
 #define DW_AN_C37_SGMII			2
 #define DW_2500BASEX			3
 #define DW_AN_C37_1000BASEX		4
+#define DW_10GBASER			5
 
 struct xpcs_id;
 
-- 
2.27.0

[PATCH net-next v8 8/9] net: txgbe: Implement phylink pcs

[Jiawen Wu]

Register MDIO bus for PCS layer to use Synopsys designware XPCS, support
10GBASE-R interface to the controller.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/Kconfig          |  1 +
 .../net/ethernet/wangxun/txgbe/txgbe_phy.c    | 97 ++++++++++++++++++-
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   |  6 ++
 3 files changed, 102 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/Kconfig b/drivers/net/ethernet/wangxun/Kconfig
index 73f4492928c0..f3fb273e6fd0 100644
--- a/drivers/net/ethernet/wangxun/Kconfig
+++ b/drivers/net/ethernet/wangxun/Kconfig
@@ -45,6 +45,7 @@ config TXGBE
 	select GPIOLIB
 	select REGMAP
 	select COMMON_CLK
+	select PCS_XPCS
 	select LIBWX
 	select SFP
 	help
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
index aa8b4444e77a..36c6517c7266 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
@@ -8,6 +8,8 @@
 #include <linux/regmap.h>
 #include <linux/clkdev.h>
 #include <linux/clk-provider.h>
+#include <linux/pcs/pcs-xpcs.h>
+#include <linux/mdio.h>
 #include <linux/i2c.h>
 #include <linux/pci.h>
 
@@ -77,6 +79,88 @@ static int txgbe_swnodes_register(struct txgbe *txgbe)
 	return software_node_register_node_group(nodes->group);
 }
 
+static int txgbe_pcs_read(struct mii_bus *bus, int addr, int devnum, int regnum)
+{
+	struct wx *wx  = bus->priv;
+	u32 offset, val;
+
+	if (addr)
+		return -EOPNOTSUPP;
+
+	offset = devnum << 16 | regnum;
+
+	/* Set the LAN port indicator to IDA_ADDR */
+	wr32(wx, TXGBE_XPCS_IDA_ADDR, offset);
+
+	/* Read the data from IDA_DATA register */
+	val = rd32(wx, TXGBE_XPCS_IDA_DATA);
+
+	return (u16)val;
+}
+
+static int txgbe_pcs_write(struct mii_bus *bus, int addr, int devnum, int regnum, u16 val)
+{
+	struct wx *wx = bus->priv;
+	u32 offset;
+
+	if (addr)
+		return -EOPNOTSUPP;
+
+	offset = devnum << 16 | regnum;
+
+	/* Set the LAN port indicator to IDA_ADDR */
+	wr32(wx, TXGBE_XPCS_IDA_ADDR, offset);
+
+	/* Write the data to IDA_DATA register */
+	wr32(wx, TXGBE_XPCS_IDA_DATA, val);
+
+	return 0;
+}
+
+static int txgbe_mdio_pcs_init(struct txgbe *txgbe)
+{
+	struct mdio_device *mdiodev;
+	struct wx *wx = txgbe->wx;
+	struct mii_bus *mii_bus;
+	struct dw_xpcs *xpcs;
+	struct pci_dev *pdev;
+	int ret = 0;
+
+	pdev = wx->pdev;
+
+	mii_bus = devm_mdiobus_alloc(&pdev->dev);
+	if (!mii_bus)
+		return -ENOMEM;
+
+	mii_bus->name = "txgbe_pcs_mdio_bus";
+	mii_bus->read_c45 = &txgbe_pcs_read;
+	mii_bus->write_c45 = &txgbe_pcs_write;
+	mii_bus->parent = &pdev->dev;
+	mii_bus->phy_mask = ~0;
+	mii_bus->priv = wx;
+	snprintf(mii_bus->id, MII_BUS_ID_SIZE, "txgbe_pcs-%x",
+		 (pdev->bus->number << 8) | pdev->devfn);
+
+	ret = devm_mdiobus_register(&pdev->dev, mii_bus);
+	if (ret)
+		return ret;
+
+	mdiodev = mdio_device_create(mii_bus, 0);
+	if (IS_ERR(mdiodev))
+		return PTR_ERR(mdiodev);
+
+	xpcs = xpcs_create(mdiodev, PHY_INTERFACE_MODE_10GBASER);
+	if (IS_ERR(xpcs)) {
+		mdio_device_free(mdiodev);
+		return PTR_ERR(xpcs);
+	}
+
+	txgbe->mdiodev = mdiodev;
+	txgbe->xpcs = xpcs;
+
+	return 0;
+}
+
 static int txgbe_gpio_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct wx *wx = gpiochip_get_data(chip);
@@ -423,16 +507,22 @@ int txgbe_init_phy(struct txgbe *txgbe)
 		return ret;
 	}
 
+	ret = txgbe_mdio_pcs_init(txgbe);
+	if (ret) {
+		wx_err(txgbe->wx, "failed to init mdio pcs: %d\n", ret);
+		goto err_unregister_swnode;
+	}
+
 	ret = txgbe_gpio_init(txgbe);
 	if (ret) {
 		wx_err(txgbe->wx, "failed to init gpio\n");
-		goto err_unregister_swnode;
+		goto err_destroy_xpcs;
 	}
 
 	ret = txgbe_clock_register(txgbe);
 	if (ret) {
 		wx_err(txgbe->wx, "failed to register clock: %d\n", ret);
-		goto err_unregister_swnode;
+		goto err_destroy_xpcs;
 	}
 
 	ret = txgbe_i2c_register(txgbe);
@@ -454,6 +544,8 @@ int txgbe_init_phy(struct txgbe *txgbe)
 err_unregister_clk:
 	clkdev_drop(txgbe->clock);
 	clk_unregister(txgbe->clk);
+err_destroy_xpcs:
+	xpcs_destroy(txgbe->xpcs);
 err_unregister_swnode:
 	software_node_unregister_node_group(txgbe->nodes.group);
 
@@ -466,5 +558,6 @@ void txgbe_remove_phy(struct txgbe *txgbe)
 	platform_device_unregister(txgbe->i2c_dev);
 	clkdev_drop(txgbe->clock);
 	clk_unregister(txgbe->clk);
+	xpcs_destroy(txgbe->xpcs);
 	software_node_unregister_node_group(txgbe->nodes.group);
 }
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index 6c903e4517c7..6c0393c19b83 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -80,6 +80,10 @@
 /* I2C registers */
 #define TXGBE_I2C_BASE                          0x14900
 
+/************************************** ETH PHY ******************************/
+#define TXGBE_XPCS_IDA_ADDR                     0x13000
+#define TXGBE_XPCS_IDA_DATA                     0x13004
+
 /* Part Number String Length */
 #define TXGBE_PBANUM_LENGTH                     32
 
@@ -171,6 +175,8 @@ struct txgbe_nodes {
 struct txgbe {
 	struct wx *wx;
 	struct txgbe_nodes nodes;
+	struct mdio_device *mdiodev;
+	struct dw_xpcs *xpcs;
 	struct platform_device *sfp_dev;
 	struct platform_device *i2c_dev;
 	struct clk_lookup *clock;
-- 
2.27.0

[PATCH net-next v8 9/9] net: txgbe: Support phylink MAC layer

[Jiawen Wu]

Add phylink support to Wangxun 10Gb Ethernet controller for the 10GBASE-R
interface.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/Kconfig          |   1 +
 .../ethernet/wangxun/txgbe/txgbe_ethtool.c    |  28 +++++
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   |  23 ++--
 .../net/ethernet/wangxun/txgbe/txgbe_phy.c    | 113 +++++++++++++++++-
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   |   5 +
 5 files changed, 155 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/Kconfig b/drivers/net/ethernet/wangxun/Kconfig
index f3fb273e6fd0..2ca163f07359 100644
--- a/drivers/net/ethernet/wangxun/Kconfig
+++ b/drivers/net/ethernet/wangxun/Kconfig
@@ -46,6 +46,7 @@ config TXGBE
 	select REGMAP
 	select COMMON_CLK
 	select PCS_XPCS
+	select PHYLINK
 	select LIBWX
 	select SFP
 	help
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
index d914e9a05404..859da112586a 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
@@ -6,11 +6,39 @@
 #include <linux/netdevice.h>
 
 #include "../libwx/wx_ethtool.h"
+#include "../libwx/wx_type.h"
+#include "txgbe_type.h"
 #include "txgbe_ethtool.h"
 
+static int txgbe_nway_reset(struct net_device *netdev)
+{
+	struct txgbe *txgbe = netdev_to_txgbe(netdev);
+
+	return phylink_ethtool_nway_reset(txgbe->phylink);
+}
+
+static int txgbe_get_link_ksettings(struct net_device *netdev,
+				    struct ethtool_link_ksettings *cmd)
+{
+	struct txgbe *txgbe = netdev_to_txgbe(netdev);
+
+	return phylink_ethtool_ksettings_get(txgbe->phylink, cmd);
+}
+
+static int txgbe_set_link_ksettings(struct net_device *netdev,
+				    const struct ethtool_link_ksettings *cmd)
+{
+	struct txgbe *txgbe = netdev_to_txgbe(netdev);
+
+	return phylink_ethtool_ksettings_set(txgbe->phylink, cmd);
+}
+
 static const struct ethtool_ops txgbe_ethtool_ops = {
 	.get_drvinfo		= wx_get_drvinfo,
+	.nway_reset		= txgbe_nway_reset,
 	.get_link		= ethtool_op_get_link,
+	.get_link_ksettings	= txgbe_get_link_ksettings,
+	.set_link_ksettings	= txgbe_set_link_ksettings,
 };
 
 void txgbe_set_ethtool_ops(struct net_device *netdev)
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index ded04e9e136f..bdf735e863eb 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -7,6 +7,7 @@
 #include <linux/netdevice.h>
 #include <linux/string.h>
 #include <linux/etherdevice.h>
+#include <linux/phylink.h>
 #include <net/ip.h>
 #include <linux/if_vlan.h>
 
@@ -204,7 +205,8 @@ static int txgbe_request_irq(struct wx *wx)
 
 static void txgbe_up_complete(struct wx *wx)
 {
-	u32 reg;
+	struct net_device *netdev = wx->netdev;
+	struct txgbe *txgbe = netdev_to_txgbe(netdev);
 
 	wx_control_hw(wx, true);
 	wx_configure_vectors(wx);
@@ -213,24 +215,16 @@ static void txgbe_up_complete(struct wx *wx)
 	smp_mb__before_atomic();
 	wx_napi_enable_all(wx);
 
+	phylink_start(txgbe->phylink);
+
 	/* clear any pending interrupts, may auto mask */
 	rd32(wx, WX_PX_IC(0));
 	rd32(wx, WX_PX_IC(1));
 	rd32(wx, WX_PX_MISC_IC);
 	txgbe_irq_enable(wx, true);
 
-	/* Configure MAC Rx and Tx when link is up */
-	reg = rd32(wx, WX_MAC_RX_CFG);
-	wr32(wx, WX_MAC_RX_CFG, reg);
-	wr32(wx, WX_MAC_PKT_FLT, WX_MAC_PKT_FLT_PR);
-	reg = rd32(wx, WX_MAC_WDG_TIMEOUT);
-	wr32(wx, WX_MAC_WDG_TIMEOUT, reg);
-	reg = rd32(wx, WX_MAC_TX_CFG);
-	wr32(wx, WX_MAC_TX_CFG, (reg & ~WX_MAC_TX_CFG_SPEED_MASK) | WX_MAC_TX_CFG_SPEED_10G);
-
 	/* enable transmits */
-	netif_tx_start_all_queues(wx->netdev);
-	netif_carrier_on(wx->netdev);
+	netif_tx_start_all_queues(netdev);
 }
 
 static void txgbe_reset(struct wx *wx)
@@ -264,7 +258,6 @@ static void txgbe_disable_device(struct wx *wx)
 		wx_disable_rx_queue(wx, wx->rx_ring[i]);
 
 	netif_tx_stop_all_queues(netdev);
-	netif_carrier_off(netdev);
 	netif_tx_disable(netdev);
 
 	wx_irq_disable(wx);
@@ -295,8 +288,12 @@ static void txgbe_disable_device(struct wx *wx)
 
 static void txgbe_down(struct wx *wx)
 {
+	struct net_device *netdev = wx->netdev;
+	struct txgbe *txgbe = netdev_to_txgbe(netdev);
+
 	txgbe_disable_device(wx);
 	txgbe_reset(wx);
+	phylink_stop(txgbe->phylink);
 
 	wx_clean_all_tx_rings(wx);
 	wx_clean_all_rx_rings(wx);
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
index 36c6517c7266..8500b9a57dcd 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
@@ -9,11 +9,13 @@
 #include <linux/clkdev.h>
 #include <linux/clk-provider.h>
 #include <linux/pcs/pcs-xpcs.h>
+#include <linux/phylink.h>
 #include <linux/mdio.h>
 #include <linux/i2c.h>
 #include <linux/pci.h>
 
 #include "../libwx/wx_type.h"
+#include "../libwx/wx_lib.h"
 #include "../libwx/wx_hw.h"
 #include "txgbe_type.h"
 #include "txgbe_phy.h"
@@ -161,6 +163,95 @@ static int txgbe_mdio_pcs_init(struct txgbe *txgbe)
 	return 0;
 }
 
+static struct phylink_pcs *txgbe_phylink_mac_select(struct phylink_config *config,
+						    phy_interface_t interface)
+{
+	struct txgbe *txgbe = netdev_to_txgbe(to_net_dev(config->dev));
+
+	return &txgbe->xpcs->pcs;
+}
+
+static void txgbe_mac_config(struct phylink_config *config, unsigned int mode,
+			     const struct phylink_link_state *state)
+{
+}
+
+static void txgbe_mac_link_down(struct phylink_config *config,
+				unsigned int mode, phy_interface_t interface)
+{
+	struct wx *wx = netdev_priv(to_net_dev(config->dev));
+
+	wr32m(wx, WX_MAC_TX_CFG, WX_MAC_TX_CFG_TE, 0);
+}
+
+static void txgbe_mac_link_up(struct phylink_config *config,
+			      struct phy_device *phy,
+			      unsigned int mode, phy_interface_t interface,
+			      int speed, int duplex,
+			      bool tx_pause, bool rx_pause)
+{
+	struct wx *wx = netdev_priv(to_net_dev(config->dev));
+	u32 txcfg, wdg;
+
+	txcfg = rd32(wx, WX_MAC_TX_CFG);
+	txcfg &= ~WX_MAC_TX_CFG_SPEED_MASK;
+
+	switch (speed) {
+	case SPEED_10000:
+		txcfg |= WX_MAC_TX_CFG_SPEED_10G;
+		break;
+	case SPEED_1000:
+	case SPEED_100:
+	case SPEED_10:
+		txcfg |= WX_MAC_TX_CFG_SPEED_1G;
+		break;
+	default:
+		break;
+	}
+
+	wr32(wx, WX_MAC_TX_CFG, txcfg | WX_MAC_TX_CFG_TE);
+
+	/* Re configure MAC Rx */
+	wr32m(wx, WX_MAC_RX_CFG, WX_MAC_RX_CFG_RE, WX_MAC_RX_CFG_RE);
+	wr32(wx, WX_MAC_PKT_FLT, WX_MAC_PKT_FLT_PR);
+	wdg = rd32(wx, WX_MAC_WDG_TIMEOUT);
+	wr32(wx, WX_MAC_WDG_TIMEOUT, wdg);
+}
+
+static const struct phylink_mac_ops txgbe_mac_ops = {
+	.mac_select_pcs = txgbe_phylink_mac_select,
+	.mac_config = txgbe_mac_config,
+	.mac_link_down = txgbe_mac_link_down,
+	.mac_link_up = txgbe_mac_link_up,
+};
+
+static int txgbe_phylink_init(struct txgbe *txgbe)
+{
+	struct phylink_config *config;
+	struct fwnode_handle *fwnode;
+	struct wx *wx = txgbe->wx;
+	phy_interface_t phy_mode;
+	struct phylink *phylink;
+
+	config = devm_kzalloc(&wx->pdev->dev, sizeof(*config), GFP_KERNEL);
+	if (!config)
+		return -ENOMEM;
+
+	config->dev = &wx->netdev->dev;
+	config->type = PHYLINK_NETDEV;
+	config->mac_capabilities = MAC_10000FD | MAC_1000FD | MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
+	phy_mode = PHY_INTERFACE_MODE_10GBASER;
+	__set_bit(PHY_INTERFACE_MODE_10GBASER, config->supported_interfaces);
+	fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_PHYLINK]);
+	phylink = phylink_create(config, fwnode, phy_mode, &txgbe_mac_ops);
+	if (IS_ERR(phylink))
+		return PTR_ERR(phylink);
+
+	txgbe->phylink = phylink;
+
+	return 0;
+}
+
 static int txgbe_gpio_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct wx *wx = gpiochip_get_data(chip);
@@ -322,6 +413,9 @@ static void txgbe_irq_handler(struct irq_desc *desc)
 	irq_hw_number_t hwirq;
 	unsigned long gpioirq;
 	struct gpio_chip *gc;
+	u32 eicr;
+
+	eicr = wx_misc_isb(wx, WX_ISB_MISC);
 
 	chained_irq_enter(chip, desc);
 
@@ -340,6 +434,12 @@ static void txgbe_irq_handler(struct irq_desc *desc)
 
 	chained_irq_exit(chip, desc);
 
+	if (eicr & (TXGBE_PX_MISC_ETH_LK | TXGBE_PX_MISC_ETH_LKDN)) {
+		u32 reg = rd32(wx, TXGBE_CFG_PORT_ST);
+
+		phylink_mac_change(txgbe->phylink, !!(reg & TXGBE_CFG_PORT_ST_LINK_UP));
+	}
+
 	/* unmask interrupt */
 	wx_intr_enable(wx, TXGBE_INTR_MISC(wx));
 }
@@ -513,16 +613,22 @@ int txgbe_init_phy(struct txgbe *txgbe)
 		goto err_unregister_swnode;
 	}
 
+	ret = txgbe_phylink_init(txgbe);
+	if (ret) {
+		wx_err(txgbe->wx, "failed to init phylink\n");
+		goto err_destroy_xpcs;
+	}
+
 	ret = txgbe_gpio_init(txgbe);
 	if (ret) {
 		wx_err(txgbe->wx, "failed to init gpio\n");
-		goto err_destroy_xpcs;
+		goto err_destroy_phylink;
 	}
 
 	ret = txgbe_clock_register(txgbe);
 	if (ret) {
 		wx_err(txgbe->wx, "failed to register clock: %d\n", ret);
-		goto err_destroy_xpcs;
+		goto err_destroy_phylink;
 	}
 
 	ret = txgbe_i2c_register(txgbe);
@@ -544,6 +650,8 @@ int txgbe_init_phy(struct txgbe *txgbe)
 err_unregister_clk:
 	clkdev_drop(txgbe->clock);
 	clk_unregister(txgbe->clk);
+err_destroy_phylink:
+	phylink_destroy(txgbe->phylink);
 err_destroy_xpcs:
 	xpcs_destroy(txgbe->xpcs);
 err_unregister_swnode:
@@ -558,6 +666,7 @@ void txgbe_remove_phy(struct txgbe *txgbe)
 	platform_device_unregister(txgbe->i2c_dev);
 	clkdev_drop(txgbe->clock);
 	clk_unregister(txgbe->clk);
+	phylink_destroy(txgbe->phylink);
 	xpcs_destroy(txgbe->xpcs);
 	software_node_unregister_node_group(txgbe->nodes.group);
 }
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index 6c0393c19b83..7e46ebf704c8 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -77,6 +77,10 @@
 	 TXGBE_PX_MISC_ETH_AN | TXGBE_PX_MISC_INT_ERR |   \
 	 TXGBE_PX_MISC_GPIO)
 
+/* Port cfg registers */
+#define TXGBE_CFG_PORT_ST                       0x14404
+#define TXGBE_CFG_PORT_ST_LINK_UP               BIT(0)
+
 /* I2C registers */
 #define TXGBE_I2C_BASE                          0x14900
 
@@ -177,6 +181,7 @@ struct txgbe {
 	struct txgbe_nodes nodes;
 	struct mdio_device *mdiodev;
 	struct dw_xpcs *xpcs;
+	struct phylink *phylink;
 	struct platform_device *sfp_dev;
 	struct platform_device *i2c_dev;
 	struct clk_lookup *clock;
-- 
2.27.0

Re: [PATCH net-next v8 2/9] i2c: designware: Add driver support for Wangxun 10Gb NIC

[andy.shevchenko]

Mon, May 15, 2023 at 02:31:53PM +0800, Jiawen Wu kirjoitti:
> Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate
> with SFP.
> 
> Introduce the property "snps,i2c-platform" to match device data for Wangxun
> in software node case. Since IO resource was mapped on the ethernet driver,
> add a model quirk to get regmap from parent device.
> 
> The exists IP limitations are dealt as workarounds:
> - IP does not support interrupt mode, it works on polling mode.
> - Additionally set FIFO depth address the chip issue.

...

>  	dev->flags = (uintptr_t)device_get_match_data(&pdev->dev);
> +	if (device_property_present(&pdev->dev, "snps,i2c-platform"))
> +		dev->flags |= MODEL_WANGXUN_SP;

What I meant here is to use device_property_present() _iff_ you have decided to
go with the _vendor-specific_ property name.

Otherwise it should be handled differently, i.e. with reading the actual value
of that property. Hence it should correspond the model enum, which you need to
declare in the Device Tree bindings before use.

So, either

	if (device_property_present(&pdev->dev, "wx,..."))
		dev->flags |= MODEL_WANGXUN_SP;

or

	if ((dev->flags & MODEL_MASK) == MODEL_NONE) {
	// you now have to distinguish that there is no model set in driver data
		u32 model;

		ret = device_property_read_u32(dev, "snps,i2c-platform");
		if (ret) {
			...handle error...
		}
		dev->flags |= model

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net-next v8 4/9] net: txgbe: Register I2C platform device

[andy.shevchenko]

Mon, May 15, 2023 at 02:31:55PM +0800, Jiawen Wu kirjoitti:
> Register the platform device to use Designware I2C bus master driver.
> Use regmap to read/write I2C device region from given base offset.

...

> +#include <linux/platform_device.h>
>  #include <linux/gpio/property.h>
> +#include <linux/regmap.h>

Perhaps keeping this ordered?

>  #include <linux/clkdev.h>
>  #include <linux/clk-provider.h>
>  #include <linux/i2c.h>

...

> +static const struct regmap_config i2c_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_read = txgbe_i2c_read,
> +	.reg_write = txgbe_i2c_write,

	fast_io = true;

? (Note, I haven't checked if IO accessors are really fast)

> +};

...

> +	i2c_regmap = devm_regmap_init(&pdev->dev, NULL, wx,
> +				      &i2c_regmap_config);

This is exactly a single line (80 characters), why to have two?

> +	if (IS_ERR(i2c_regmap)) {
> +		wx_err(wx, "failed to init regmap\n");
> +		return PTR_ERR(i2c_regmap);
> +	}

...

> +	res = DEFINE_RES_IRQ(pdev->irq);
> +	info.res = &res;

You can do

	info.res = &DEFINE_RES_IRQ(pdev->irq);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[andy.shevchenko]

Mon, May 15, 2023 at 02:31:57PM +0800, Jiawen Wu kirjoitti:
> Register GPIO chip and handle GPIO IRQ for SFP socket.

...

> +static int txgbe_gpio_direction_out(struct gpio_chip *chip, unsigned int offset,
> +				    int val)
> +{
> +	struct wx *wx = gpiochip_get_data(chip);
> +	unsigned long flags;
> +	u32 set;
> +
> +	spin_lock_irqsave(&wx->gpio_lock, flags);

> +	set = val ? BIT(offset) : 0;

Why is this under the lock?

> +	wr32m(wx, WX_GPIO_DR, BIT(offset), set);
> +	wr32m(wx, WX_GPIO_DDR, BIT(offset), BIT(offset));
> +	spin_unlock_irqrestore(&wx->gpio_lock, flags);
> +
> +	return 0;
> +}

...

> +static void txgbe_toggle_trigger(struct gpio_chip *gc, unsigned int offset)

> +{
> +	struct wx *wx = gpiochip_get_data(gc);
> +	u32 pol;
> +	int val;
> +
> +	pol = rd32(wx, WX_GPIO_POLARITY);

> +	val = gc->get(gc, offset);

Can't you use directly the respective rd32()?

> +	if (val)
> +		pol &= ~BIT(offset);
> +	else
> +		pol |= BIT(offset);
> +
> +	wr32(wx, WX_GPIO_POLARITY, pol);
> +}

...

> +static void txgbe_irq_handler(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct wx *wx = irq_desc_get_handler_data(desc);
> +	struct txgbe *txgbe = wx->priv;
> +	irq_hw_number_t hwirq;
> +	unsigned long gpioirq;
> +	struct gpio_chip *gc;
> +
> +	chained_irq_enter(chip, desc);

Seems spin_lock() call is missing in this function.

> +	gpioirq = rd32(wx, WX_GPIO_INTSTATUS);
> +
> +	gc = txgbe->gpio;
> +	for_each_set_bit(hwirq, &gpioirq, gc->ngpio) {
> +		int gpio = irq_find_mapping(gc->irq.domain, hwirq);
> +		u32 irq_type = irq_get_trigger_type(gpio);
> +
> +		generic_handle_irq(gpio);

Can generic_handle_domain_irq() be used here?

> +		if ((irq_type & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_EDGE_BOTH)
> +			txgbe_toggle_trigger(gc, hwirq);
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +
> +	/* unmask interrupt */
> +	wx_intr_enable(wx, TXGBE_INTR_MISC(wx));
> +}

...

> +static int txgbe_gpio_init(struct txgbe *txgbe)
> +{
> +	struct gpio_irq_chip *girq;
> +	struct wx *wx = txgbe->wx;
> +	struct gpio_chip *gc;
> +	struct device *dev;
> +	int ret;

> +	dev = &wx->pdev->dev;

This can be united with the defintion above.

	struct device *dev = &wx->pdev->dev;

> +	gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
> +	if (!gc)
> +		return -ENOMEM;
> +
> +	gc->label = devm_kasprintf(dev, GFP_KERNEL, "txgbe_gpio-%x",
> +				   (wx->pdev->bus->number << 8) | wx->pdev->devfn);
> +	gc->base = -1;
> +	gc->ngpio = 6;
> +	gc->owner = THIS_MODULE;
> +	gc->parent = dev;
> +	gc->fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_GPIO]);

Looking at the I²C case, I'm wondering if gpio-regmap can be used for this piece.

> +	gc->get = txgbe_gpio_get;
> +	gc->get_direction = txgbe_gpio_get_direction;
> +	gc->direction_input = txgbe_gpio_direction_in;
> +	gc->direction_output = txgbe_gpio_direction_out;
> +
> +	girq = &gc->irq;
> +	gpio_irq_chip_set_chip(girq, &txgbe_gpio_irq_chip);
> +	girq->parent_handler = txgbe_irq_handler;
> +	girq->parent_handler_data = wx;
> +	girq->num_parents = 1;
> +	girq->parents = devm_kcalloc(dev, girq->num_parents,
> +				     sizeof(*girq->parents), GFP_KERNEL);
> +	if (!girq->parents)
> +		return -ENOMEM;
> +	girq->parents[0] = wx->msix_entries[wx->num_q_vectors].vector;
> +	girq->default_type = IRQ_TYPE_NONE;
> +	girq->handler = handle_bad_irq;
> +
> +	ret = devm_gpiochip_add_data(dev, gc, wx);
> +	if (ret)
> +		return ret;

> +	spin_lock_init(&wx->gpio_lock);

Isn't it too late?

> +	txgbe->gpio = gc;
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Andy Shevchenko]

On Mon, May 15, 2023 at 02:31:57PM +0800, Jiawen Wu wrote:
> Register GPIO chip and handle GPIO IRQ for SFP socket.

...

> +	spin_lock_init(&wx->gpio_lock);

Almost forgot to ask, are you planning to use this GPIO part on PREEMPT_RT
kernels? Currently you will get a splat in case IRQ is fired.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Jiawen Wu]

On Tuesday, May 16, 2023 5:36 AM, Andy Shevchenko wrote:
> On Mon, May 15, 2023 at 02:31:57PM +0800, Jiawen Wu wrote:
> > Register GPIO chip and handle GPIO IRQ for SFP socket.
> 
> ...
> 
> > +	spin_lock_init(&wx->gpio_lock);
> 
> Almost forgot to ask, are you planning to use this GPIO part on PREEMPT_RT
> kernels? Currently you will get a splat in case IRQ is fired.

Hmmm, I don't know much about this. Should I use raw_spinlock_t instead of
spinlock_t?

RE: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Jiawen Wu]

> > +static int txgbe_gpio_init(struct txgbe *txgbe)
> > +{
> > +	struct gpio_irq_chip *girq;
> > +	struct wx *wx = txgbe->wx;
> > +	struct gpio_chip *gc;
> > +	struct device *dev;
> > +	int ret;
> 
> > +	dev = &wx->pdev->dev;
> 
> This can be united with the defintion above.
> 
> 	struct device *dev = &wx->pdev->dev;
> 

This is a question that I often run into, when I want to keep this order,
i.e. lines longest to shortest, but the line of the pointer which get later
is longer. For this example:

	struct wx *wx = txgbe->wx;
	struct device *dev = &wx->pdev->dev;

should I split the line, or put the long line abruptly there?

> > +	gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
> > +	if (!gc)
> > +		return -ENOMEM;
> > +
> > +	gc->label = devm_kasprintf(dev, GFP_KERNEL, "txgbe_gpio-%x",
> > +				   (wx->pdev->bus->number << 8) | wx->pdev->devfn);
> > +	gc->base = -1;
> > +	gc->ngpio = 6;
> > +	gc->owner = THIS_MODULE;
> > +	gc->parent = dev;
> > +	gc->fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_GPIO]);
> 
> Looking at the I²C case, I'm wondering if gpio-regmap can be used for this piece.

I can access this GPIO region directly, do I really need to use regmap?
 

Re: [PATCH net-next v8 8/9] net: txgbe: Implement phylink pcs

[Lars-Peter Clausen]

On 5/14/23 23:31, Jiawen Wu wrote:
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> index 6c903e4517c7..6c0393c19b83 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> @@ -80,6 +80,10 @@
>   /* I2C registers */
>   #define TXGBE_I2C_BASE                          0x14900
>   
> +/************************************** ETH PHY ******************************/
> +#define TXGBE_XPCS_IDA_ADDR                     0x13000
> +#define TXGBE_XPCS_IDA_DATA                     0x13004
> +
>   /* Part Number String Length */
>   #define TXGBE_PBANUM_LENGTH                     32
>   
> @@ -171,6 +175,8 @@ struct txgbe_nodes {
>   struct txgbe {
>   	struct wx *wx;
>   	struct txgbe_nodes nodes;
> +	struct mdio_device *mdiodev;

While assigned mdiodev is never read.

> +	struct dw_xpcs *xpcs;
>   	struct platform_device *sfp_dev;
>   	struct platform_device *i2c_dev;
>   	struct clk_lookup *clock;

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Andy Shevchenko]

On Tue, May 16, 2023 at 5:39 AM Jiawen Wu <jiawenwu@trustnetic.com> wrote:

...

> > > +   struct gpio_irq_chip *girq;
> > > +   struct wx *wx = txgbe->wx;
> > > +   struct gpio_chip *gc;
> > > +   struct device *dev;
> > > +   int ret;
> >
> > > +   dev = &wx->pdev->dev;
> >
> > This can be united with the defintion above.
> >
> >       struct device *dev = &wx->pdev->dev;
> >
>
> This is a question that I often run into, when I want to keep this order,
> i.e. lines longest to shortest, but the line of the pointer which get later
> is longer. For this example:
>
>         struct wx *wx = txgbe->wx;
>         struct device *dev = &wx->pdev->dev;

So, we locate assignments according to the flow. I do not see an issue here.

> should I split the line, or put the long line abruptly there?

The latter is fine.

...

> > > +   gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
> > > +   if (!gc)
> > > +           return -ENOMEM;
> > > +
> > > +   gc->label = devm_kasprintf(dev, GFP_KERNEL, "txgbe_gpio-%x",
> > > +                              (wx->pdev->bus->number << 8) | wx->pdev->devfn);
> > > +   gc->base = -1;
> > > +   gc->ngpio = 6;
> > > +   gc->owner = THIS_MODULE;
> > > +   gc->parent = dev;
> > > +   gc->fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_GPIO]);
> >
> > Looking at the I²C case, I'm wondering if gpio-regmap can be used for this piece.
>
> I can access this GPIO region directly, do I really need to use regmap?

It's not a matter of access, it's a matter of using an existing
wrapper that will give you already a lot of code done there, i.o.w.
you don't need to reinvent a wheel.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Paolo Abeni]

On Tue, 2023-05-16 at 10:12 +0300, Andy Shevchenko wrote:
> On Tue, May 16, 2023 at 5:39 AM Jiawen Wu <jiawenwu@trustnetic.com> wrote:
> 
> ...
> 
> > > > +   struct gpio_irq_chip *girq;
> > > > +   struct wx *wx = txgbe->wx;
> > > > +   struct gpio_chip *gc;
> > > > +   struct device *dev;
> > > > +   int ret;
> > > 
> > > > +   dev = &wx->pdev->dev;
> > > 
> > > This can be united with the defintion above.
> > > 
> > >       struct device *dev = &wx->pdev->dev;
> > > 
> > 
> > This is a question that I often run into, when I want to keep this order,
> > i.e. lines longest to shortest, but the line of the pointer which get later
> > is longer. For this example:
> > 
> >         struct wx *wx = txgbe->wx;
> >         struct device *dev = &wx->pdev->dev;
> 
> So, we locate assignments according to the flow. I do not see an issue here.

That would break the reverse x-mass tree order.

> > should I split the line, or put the long line abruptly there?
> 
> The latter is fine.

This is minor, but I have to disagree. My understanding is that
respecting the reversed x-mass tree is preferred. In case of dependent
initialization as the above, the preferred style it the one used by
this patch.

Cheers,

Paolo

Re: [PATCH net-next v8 9/9] net: txgbe: Support phylink MAC layer

[Paolo Abeni]

On Mon, 2023-05-15 at 14:32 +0800, Jiawen Wu wrote:
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index ded04e9e136f..bdf735e863eb 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> @@ -7,6 +7,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/string.h>
>  #include <linux/etherdevice.h>
> +#include <linux/phylink.h>
>  #include <net/ip.h>
>  #include <linux/if_vlan.h>
>  
> @@ -204,7 +205,8 @@ static int txgbe_request_irq(struct wx *wx)
>  
>  static void txgbe_up_complete(struct wx *wx)
>  {
> -	u32 reg;
> +	struct net_device *netdev = wx->netdev;
> +	struct txgbe *txgbe = netdev_to_txgbe(netdev);

Please respect the reverse x-mass tree order. Either avoid 'netdev'
declaration or split txgbe declaration and assignment.

>  
>  	wx_control_hw(wx, true);
>  	wx_configure_vectors(wx);
> @@ -213,24 +215,16 @@ static void txgbe_up_complete(struct wx *wx)
>  	smp_mb__before_atomic();
>  	wx_napi_enable_all(wx);
>  
> +	phylink_start(txgbe->phylink);
> +
>  	/* clear any pending interrupts, may auto mask */
>  	rd32(wx, WX_PX_IC(0));
>  	rd32(wx, WX_PX_IC(1));
>  	rd32(wx, WX_PX_MISC_IC);
>  	txgbe_irq_enable(wx, true);
>  
> -	/* Configure MAC Rx and Tx when link is up */
> -	reg = rd32(wx, WX_MAC_RX_CFG);
> -	wr32(wx, WX_MAC_RX_CFG, reg);
> -	wr32(wx, WX_MAC_PKT_FLT, WX_MAC_PKT_FLT_PR);
> -	reg = rd32(wx, WX_MAC_WDG_TIMEOUT);
> -	wr32(wx, WX_MAC_WDG_TIMEOUT, reg);
> -	reg = rd32(wx, WX_MAC_TX_CFG);
> -	wr32(wx, WX_MAC_TX_CFG, (reg & ~WX_MAC_TX_CFG_SPEED_MASK) | WX_MAC_TX_CFG_SPEED_10G);
> -
>  	/* enable transmits */
> -	netif_tx_start_all_queues(wx->netdev);
> -	netif_carrier_on(wx->netdev);
> +	netif_tx_start_all_queues(netdev);
>  }
>  
>  static void txgbe_reset(struct wx *wx)
> @@ -264,7 +258,6 @@ static void txgbe_disable_device(struct wx *wx)
>  		wx_disable_rx_queue(wx, wx->rx_ring[i]);
>  
>  	netif_tx_stop_all_queues(netdev);
> -	netif_carrier_off(netdev);
>  	netif_tx_disable(netdev);
>  
>  	wx_irq_disable(wx);
> @@ -295,8 +288,12 @@ static void txgbe_disable_device(struct wx *wx)
>  
>  static void txgbe_down(struct wx *wx)
>  {
> +	struct net_device *netdev = wx->netdev;
> +	struct txgbe *txgbe = netdev_to_txgbe(netdev);

You can replace the above 2 lines with:

	struct txgbe *txgbe = netdev_to_txgbe(wx->netdev);


Cheers,

Paolo

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Russell King (Oracle)]

On Tue, May 16, 2023 at 11:39:08AM +0200, Paolo Abeni wrote:
> On Tue, 2023-05-16 at 10:12 +0300, Andy Shevchenko wrote:
> > On Tue, May 16, 2023 at 5:39 AM Jiawen Wu <jiawenwu@trustnetic.com> wrote:
> > 
> > ...
> > 
> > > > > +   struct gpio_irq_chip *girq;
> > > > > +   struct wx *wx = txgbe->wx;
> > > > > +   struct gpio_chip *gc;
> > > > > +   struct device *dev;
> > > > > +   int ret;
> > > > 
> > > > > +   dev = &wx->pdev->dev;
> > > > 
> > > > This can be united with the defintion above.
> > > > 
> > > >       struct device *dev = &wx->pdev->dev;
> > > > 
> > > 
> > > This is a question that I often run into, when I want to keep this order,
> > > i.e. lines longest to shortest, but the line of the pointer which get later
> > > is longer. For this example:
> > > 
> > >         struct wx *wx = txgbe->wx;
> > >         struct device *dev = &wx->pdev->dev;
> > 
> > So, we locate assignments according to the flow. I do not see an issue here.
> 
> That would break the reverse x-mass tree order.
> 
> > > should I split the line, or put the long line abruptly there?
> > 
> > The latter is fine.
> 
> This is minor, but I have to disagree. My understanding is that
> respecting the reversed x-mass tree is preferred. In case of dependent
> initialization as the above, the preferred style it the one used by
> this patch.

Meanwhile, I've been told something completely different, and therefore
I do something else, namely:


	struct device *dev;
	struct wx *wx;

	wx = txgbe->wx;
	dev = &wx->pdev->dev;

	...

I've been lead to believe that this is preferred in netdev to breaking
the reverse christmas-tree due to dependent initialisations.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Jiawen Wu]

> > > > +   gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
> > > > +   if (!gc)
> > > > +           return -ENOMEM;
> > > > +
> > > > +   gc->label = devm_kasprintf(dev, GFP_KERNEL, "txgbe_gpio-%x",
> > > > +                              (wx->pdev->bus->number << 8) | wx->pdev->devfn);
> > > > +   gc->base = -1;
> > > > +   gc->ngpio = 6;
> > > > +   gc->owner = THIS_MODULE;
> > > > +   gc->parent = dev;
> > > > +   gc->fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_GPIO]);
> > >
> > > Looking at the I²C case, I'm wondering if gpio-regmap can be used for this piece.
> >
> > I can access this GPIO region directly, do I really need to use regmap?
> 
> It's not a matter of access, it's a matter of using an existing
> wrapper that will give you already a lot of code done there, i.o.w.
> you don't need to reinvent a wheel.

I took a look at the gpio-regmap code, when I call devm_gpio_regmap_register(),
I should provide gpio_regmap_config.irq_domain if I want to add the gpio_irq_chip.
But in this use, GPIO IRQs are requested by SFP driver. How can I get irq_domain
before SFP probe? And where do I add IRQ parent handler?

Re: [PATCH net-next v8 2/9] i2c: designware: Add driver support for Wangxun 10Gb NIC

[Jarkko Nikula]

On 5/15/23 12:24, andy.shevchenko@gmail.com wrote:
> Mon, May 15, 2023 at 02:31:53PM +0800, Jiawen Wu kirjoitti:
>> Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate
>> with SFP.
>>
>> Introduce the property "snps,i2c-platform" to match device data for Wangxun
>> in software node case. Since IO resource was mapped on the ethernet driver,
>> add a model quirk to get regmap from parent device.
>>
>> The exists IP limitations are dealt as workarounds:
>> - IP does not support interrupt mode, it works on polling mode.
>> - Additionally set FIFO depth address the chip issue.
> 
> ...
> 
>>   	dev->flags = (uintptr_t)device_get_match_data(&pdev->dev);
>> +	if (device_property_present(&pdev->dev, "snps,i2c-platform"))
>> +		dev->flags |= MODEL_WANGXUN_SP;
> 
> What I meant here is to use device_property_present() _iff_ you have decided to
> go with the _vendor-specific_ property name.
> 
> Otherwise it should be handled differently, i.e. with reading the actual value
> of that property. Hence it should correspond the model enum, which you need to
> declare in the Device Tree bindings before use.
> 
> So, either
> 
> 	if (device_property_present(&pdev->dev, "wx,..."))
> 		dev->flags |= MODEL_WANGXUN_SP;
> 
> or
> 
> 	if ((dev->flags & MODEL_MASK) == MODEL_NONE) {
> 	// you now have to distinguish that there is no model set in driver data
> 		u32 model;
> 
> 		ret = device_property_read_u32(dev, "snps,i2c-platform");
> 		if (ret) {
> 			...handle error...
> 		}
> 		dev->flags |= model
> 
I'm not a device tree expert but I wonder would it be possible somehow 
combine this and compatible properties in dw_i2c_of_match[]? They set 
model flag for MODEL_MSCC_OCELOT and MODEL_BAIKAL_BT1.

Then I'm thinking is "snps,i2c-platform" descriptive enough name for a 
model and does it confuse with "snps,designware-i2c" compatible property?

RE: [PATCH net-next v8 2/9] i2c: designware: Add driver support for Wangxun 10Gb NIC

[Jiawen Wu]

On Wednesday, May 17, 2023 4:49 PM, Jarkko Nikula wrote:
> On 5/15/23 12:24, andy.shevchenko@gmail.com wrote:
> > Mon, May 15, 2023 at 02:31:53PM +0800, Jiawen Wu kirjoitti:
> >> Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate
> >> with SFP.
> >>
> >> Introduce the property "snps,i2c-platform" to match device data for Wangxun
> >> in software node case. Since IO resource was mapped on the ethernet driver,
> >> add a model quirk to get regmap from parent device.
> >>
> >> The exists IP limitations are dealt as workarounds:
> >> - IP does not support interrupt mode, it works on polling mode.
> >> - Additionally set FIFO depth address the chip issue.
> >
> > ...
> >
> >>   	dev->flags = (uintptr_t)device_get_match_data(&pdev->dev);
> >> +	if (device_property_present(&pdev->dev, "snps,i2c-platform"))
> >> +		dev->flags |= MODEL_WANGXUN_SP;
> >
> > What I meant here is to use device_property_present() _iff_ you have decided to
> > go with the _vendor-specific_ property name.
> >
> > Otherwise it should be handled differently, i.e. with reading the actual value
> > of that property. Hence it should correspond the model enum, which you need to
> > declare in the Device Tree bindings before use.
> >
> > So, either
> >
> > 	if (device_property_present(&pdev->dev, "wx,..."))
> > 		dev->flags |= MODEL_WANGXUN_SP;
> >
> > or
> >
> > 	if ((dev->flags & MODEL_MASK) == MODEL_NONE) {
> > 	// you now have to distinguish that there is no model set in driver data
> > 		u32 model;
> >
> > 		ret = device_property_read_u32(dev, "snps,i2c-platform");
> > 		if (ret) {
> > 			...handle error...
> > 		}
> > 		dev->flags |= model
> >
> I'm not a device tree expert but I wonder would it be possible somehow
> combine this and compatible properties in dw_i2c_of_match[]? They set
> model flag for MODEL_MSCC_OCELOT and MODEL_BAIKAL_BT1.

Maybe the table could be changed to match device property, instead of relying
on DT only. Or device_get_match_data() could be also implemented in
software node case?

> 
> Then I'm thinking is "snps,i2c-platform" descriptive enough name for a
> model and does it confuse with "snps,designware-i2c" compatible property?

I'd like to change the name back to "wx,i2c-snps-model" for the specific one.

Re: [PATCH net-next v8 2/9] i2c: designware: Add driver support for Wangxun 10Gb NIC

[Andy Shevchenko]

On Wed, May 17, 2023 at 11:49 AM Jarkko Nikula
<jarkko.nikula@linux.intel.com> wrote:
> On 5/15/23 12:24, andy.shevchenko@gmail.com wrote:
> > Mon, May 15, 2023 at 02:31:53PM +0800, Jiawen Wu kirjoitti:
> >> Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate
> >> with SFP.
> >>
> >> Introduce the property "snps,i2c-platform" to match device data for Wangxun
> >> in software node case. Since IO resource was mapped on the ethernet driver,
> >> add a model quirk to get regmap from parent device.
> >>
> >> The exists IP limitations are dealt as workarounds:
> >> - IP does not support interrupt mode, it works on polling mode.
> >> - Additionally set FIFO depth address the chip issue.
> >
> > ...
> >
> >>      dev->flags = (uintptr_t)device_get_match_data(&pdev->dev);
> >> +    if (device_property_present(&pdev->dev, "snps,i2c-platform"))
> >> +            dev->flags |= MODEL_WANGXUN_SP;
> >
> > What I meant here is to use device_property_present() _iff_ you have decided to
> > go with the _vendor-specific_ property name.
> >
> > Otherwise it should be handled differently, i.e. with reading the actual value
> > of that property. Hence it should correspond the model enum, which you need to
> > declare in the Device Tree bindings before use.
> >
> > So, either
> >
> >       if (device_property_present(&pdev->dev, "wx,..."))
> >               dev->flags |= MODEL_WANGXUN_SP;
> >
> > or
> >
> >       if ((dev->flags & MODEL_MASK) == MODEL_NONE) {
> >       // you now have to distinguish that there is no model set in driver data
> >               u32 model;
> >
> >               ret = device_property_read_u32(dev, "snps,i2c-platform");
> >               if (ret) {
> >                       ...handle error...
> >               }
> >               dev->flags |= model
> >
> I'm not a device tree expert

Me neither, that's why I replied earlier that this needs to be
reviewed by DT people.

> but I wonder would it be possible somehow
> combine this and compatible properties in dw_i2c_of_match[]? They set
> model flag for MODEL_MSCC_OCELOT and MODEL_BAIKAL_BT1.
>
> Then I'm thinking is "snps,i2c-platform" descriptive enough name for a
> model and does it confuse with "snps,designware-i2c" compatible property?



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net-next v8 2/9] i2c: designware: Add driver support for Wangxun 10Gb NIC

[Andy Shevchenko]

On Wed, May 17, 2023 at 12:26 PM Jiawen Wu <jiawenwu@trustnetic.com> wrote:
> On Wednesday, May 17, 2023 4:49 PM, Jarkko Nikula wrote:
> > On 5/15/23 12:24, andy.shevchenko@gmail.com wrote:
> > > Mon, May 15, 2023 at 02:31:53PM +0800, Jiawen Wu kirjoitti:

...

> > >>    dev->flags = (uintptr_t)device_get_match_data(&pdev->dev);
> > >> +  if (device_property_present(&pdev->dev, "snps,i2c-platform"))
> > >> +          dev->flags |= MODEL_WANGXUN_SP;
> > >
> > > What I meant here is to use device_property_present() _iff_ you have decided to
> > > go with the _vendor-specific_ property name.
> > >
> > > Otherwise it should be handled differently, i.e. with reading the actual value
> > > of that property. Hence it should correspond the model enum, which you need to
> > > declare in the Device Tree bindings before use.
> > >
> > > So, either
> > >
> > >     if (device_property_present(&pdev->dev, "wx,..."))
> > >             dev->flags |= MODEL_WANGXUN_SP;
> > >
> > > or
> > >
> > >     if ((dev->flags & MODEL_MASK) == MODEL_NONE) {
> > >     // you now have to distinguish that there is no model set in driver data
> > >             u32 model;
> > >
> > >             ret = device_property_read_u32(dev, "snps,i2c-platform");
> > >             if (ret) {
> > >                     ...handle error...
> > >             }
> > >             dev->flags |= model
> > >
> > I'm not a device tree expert but I wonder would it be possible somehow
> > combine this and compatible properties in dw_i2c_of_match[]? They set
> > model flag for MODEL_MSCC_OCELOT and MODEL_BAIKAL_BT1.
>
> Maybe the table could be changed to match device property, instead of relying
> on DT only. Or device_get_match_data() could be also implemented in
> software node case?

This has been discussed [1] and still no visible prototype. Perhaps
you can collaborate with Vladimir on the matter.

[1]: https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Andy Shevchenko]

On Wed, May 17, 2023 at 5:56 AM Jiawen Wu <jiawenwu@trustnetic.com> wrote:
>
> > > > > +   gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
> > > > > +   if (!gc)
> > > > > +           return -ENOMEM;
> > > > > +
> > > > > +   gc->label = devm_kasprintf(dev, GFP_KERNEL, "txgbe_gpio-%x",
> > > > > +                              (wx->pdev->bus->number << 8) | wx->pdev->devfn);
> > > > > +   gc->base = -1;
> > > > > +   gc->ngpio = 6;
> > > > > +   gc->owner = THIS_MODULE;
> > > > > +   gc->parent = dev;
> > > > > +   gc->fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_GPIO]);
> > > >
> > > > Looking at the I²C case, I'm wondering if gpio-regmap can be used for this piece.
> > >
> > > I can access this GPIO region directly, do I really need to use regmap?
> >
> > It's not a matter of access, it's a matter of using an existing
> > wrapper that will give you already a lot of code done there, i.o.w.
> > you don't need to reinvent a wheel.
>
> I took a look at the gpio-regmap code, when I call devm_gpio_regmap_register(),
> I should provide gpio_regmap_config.irq_domain if I want to add the gpio_irq_chip.
> But in this use, GPIO IRQs are requested by SFP driver. How can I get irq_domain
> before SFP probe? And where do I add IRQ parent handler?

Summon Michael who can probably answer this better than me.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

['Andy Shevchenko']

On Tue, May 16, 2023 at 10:05:41AM +0800, Jiawen Wu wrote:
> On Tuesday, May 16, 2023 5:36 AM, Andy Shevchenko wrote:
> > On Mon, May 15, 2023 at 02:31:57PM +0800, Jiawen Wu wrote:
> > > Register GPIO chip and handle GPIO IRQ for SFP socket.

...

> > > +	spin_lock_init(&wx->gpio_lock);
> > 
> > Almost forgot to ask, are you planning to use this GPIO part on PREEMPT_RT
> > kernels? Currently you will get a splat in case IRQ is fired.
> 
> Hmmm, I don't know much about this. Should I use raw_spinlock_t instead of
> spinlock_t?

If you need support PREEMPT_RT.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Andrew Lunn]

On Wed, May 17, 2023 at 10:55:01AM +0800, Jiawen Wu wrote:
> > > > > +   gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
> > > > > +   if (!gc)
> > > > > +           return -ENOMEM;
> > > > > +
> > > > > +   gc->label = devm_kasprintf(dev, GFP_KERNEL, "txgbe_gpio-%x",
> > > > > +                              (wx->pdev->bus->number << 8) | wx->pdev->devfn);
> > > > > +   gc->base = -1;
> > > > > +   gc->ngpio = 6;
> > > > > +   gc->owner = THIS_MODULE;
> > > > > +   gc->parent = dev;
> > > > > +   gc->fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_GPIO]);
> > > >
> > > > Looking at the I²C case, I'm wondering if gpio-regmap can be used for this piece.
> > >
> > > I can access this GPIO region directly, do I really need to use regmap?
> > 
> > It's not a matter of access, it's a matter of using an existing
> > wrapper that will give you already a lot of code done there, i.o.w.
> > you don't need to reinvent a wheel.
> 
> I took a look at the gpio-regmap code, when I call devm_gpio_regmap_register(),
> I should provide gpio_regmap_config.irq_domain if I want to add the gpio_irq_chip.
> But in this use, GPIO IRQs are requested by SFP driver. How can I get irq_domain
> before SFP probe? And where do I add IRQ parent handler?
 
I _think_ you are mixing upstream IRQs and downstream IRQs.

Interrupts are arranged in trees. The CPU itself only has one or two
interrupts. e.g. for ARM you have FIQ and IRQ. When the CPU gets an
interrupt, you look in the interrupt controller to see what external
or internal interrupt triggered the CPU interrupt. And that interrupt
controller might indicate the interrupt came from another interrupt
controller. Hence the tree structure. And each node in the tree is
considered an interrupt domain.

A GPIO controller can also be an interrupt controller. It has an
upstream interrupt, going to the controller above it. And it has
downstream interrupts, the GPIO lines coming into it which can cause
an interrupt. And the GPIO interrupt controller is a domain.

So what exactly does gpio_regmap_config.irq_domain mean? Is it the
domain of the upstream interrupt controller? Is it an empty domain
structure to be used by the GPIO interrupt controller? It is very
unlikely to have anything to do with the SFP devices below it.

	 Andrew

RE: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Jiawen Wu]

On Wednesday, May 17, 2023 11:01 PM, Andrew Lunn wrote:
> On Wed, May 17, 2023 at 10:55:01AM +0800, Jiawen Wu wrote:
> > > > > > +   gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
> > > > > > +   if (!gc)
> > > > > > +           return -ENOMEM;
> > > > > > +
> > > > > > +   gc->label = devm_kasprintf(dev, GFP_KERNEL, "txgbe_gpio-%x",
> > > > > > +                              (wx->pdev->bus->number << 8) | wx->pdev->devfn);
> > > > > > +   gc->base = -1;
> > > > > > +   gc->ngpio = 6;
> > > > > > +   gc->owner = THIS_MODULE;
> > > > > > +   gc->parent = dev;
> > > > > > +   gc->fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_GPIO]);
> > > > >
> > > > > Looking at the I²C case, I'm wondering if gpio-regmap can be used for this piece.
> > > >
> > > > I can access this GPIO region directly, do I really need to use regmap?
> > >
> > > It's not a matter of access, it's a matter of using an existing
> > > wrapper that will give you already a lot of code done there, i.o.w.
> > > you don't need to reinvent a wheel.
> >
> > I took a look at the gpio-regmap code, when I call devm_gpio_regmap_register(),
> > I should provide gpio_regmap_config.irq_domain if I want to add the gpio_irq_chip.
> > But in this use, GPIO IRQs are requested by SFP driver. How can I get irq_domain
> > before SFP probe? And where do I add IRQ parent handler?
> 
> I _think_ you are mixing upstream IRQs and downstream IRQs.
> 
> Interrupts are arranged in trees. The CPU itself only has one or two
> interrupts. e.g. for ARM you have FIQ and IRQ. When the CPU gets an
> interrupt, you look in the interrupt controller to see what external
> or internal interrupt triggered the CPU interrupt. And that interrupt
> controller might indicate the interrupt came from another interrupt
> controller. Hence the tree structure. And each node in the tree is
> considered an interrupt domain.
> 
> A GPIO controller can also be an interrupt controller. It has an
> upstream interrupt, going to the controller above it. And it has
> downstream interrupts, the GPIO lines coming into it which can cause
> an interrupt. And the GPIO interrupt controller is a domain.
> 
> So what exactly does gpio_regmap_config.irq_domain mean? Is it the
> domain of the upstream interrupt controller? Is it an empty domain
> structure to be used by the GPIO interrupt controller? It is very
> unlikely to have anything to do with the SFP devices below it.

Sorry, since I don't know much about interrupt,  it is difficult to understand
regmap-irq in a short time. There are many questions about regmap-irq.

When I want to add an IRQ chip for regmap, for the further irq_domain,
I need to pass a parameter of IRQ, and this IRQ will be requested with handler:
regmap_irq_thread(). Which IRQ does it mean? In the previous code of using
devm_gpiochip_add_data(), I set the MSI-X interrupt as gpio-irq's parent, but
it was used to set chained handler only. Should the parent be this IRQ? I found
the error with irq_free_descs and irq_domain_remove when I remove txgbe.ko.

As you said, the interrupt of each tree node has its domain. Can I understand
that there are two layer in the interrupt tree for MSI-X and GPIOs, and requesting
them separately is not conflicting? Although I thought so, but after I implement
gpio-regmap, SFP driver even could not find gpio_desc. Maybe I missed something
on registering gpio-regmap...

Anyway it is a bit complicated, could I use this version of GPIO implementation if
it's really tough? Thanks.

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Andy Shevchenko]

On Thu, May 18, 2023 at 2:50 PM Jiawen Wu <jiawenwu@trustnetic.com> wrote:
> On Wednesday, May 17, 2023 11:01 PM, Andrew Lunn wrote:
> > On Wed, May 17, 2023 at 10:55:01AM +0800, Jiawen Wu wrote:

...

> > > I should provide gpio_regmap_config.irq_domain if I want to add the gpio_irq_chip.
> > > But in this use, GPIO IRQs are requested by SFP driver. How can I get irq_domain
> > > before SFP probe? And where do I add IRQ parent handler?
> >
> > I _think_ you are mixing upstream IRQs and downstream IRQs.
> >
> > Interrupts are arranged in trees. The CPU itself only has one or two
> > interrupts. e.g. for ARM you have FIQ and IRQ. When the CPU gets an
> > interrupt, you look in the interrupt controller to see what external
> > or internal interrupt triggered the CPU interrupt. And that interrupt
> > controller might indicate the interrupt came from another interrupt
> > controller. Hence the tree structure. And each node in the tree is
> > considered an interrupt domain.
> >
> > A GPIO controller can also be an interrupt controller. It has an
> > upstream interrupt, going to the controller above it. And it has
> > downstream interrupts, the GPIO lines coming into it which can cause
> > an interrupt. And the GPIO interrupt controller is a domain.
> >
> > So what exactly does gpio_regmap_config.irq_domain mean? Is it the
> > domain of the upstream interrupt controller? Is it an empty domain
> > structure to be used by the GPIO interrupt controller? It is very
> > unlikely to have anything to do with the SFP devices below it.
>
> Sorry, since I don't know much about interrupt,  it is difficult to understand
> regmap-irq in a short time. There are many questions about regmap-irq.

That's why I Cc'ed to Michael who is the author of gpio-regmap to
probably get advice from.

> When I want to add an IRQ chip for regmap, for the further irq_domain,
> I need to pass a parameter of IRQ, and this IRQ will be requested with handler:
> regmap_irq_thread(). Which IRQ does it mean? In the previous code of using
> devm_gpiochip_add_data(), I set the MSI-X interrupt as gpio-irq's parent, but
> it was used to set chained handler only. Should the parent be this IRQ? I found
> the error with irq_free_descs and irq_domain_remove when I remove txgbe.ko.
>
> As you said, the interrupt of each tree node has its domain. Can I understand
> that there are two layer in the interrupt tree for MSI-X and GPIOs, and requesting
> them separately is not conflicting? Although I thought so, but after I implement
> gpio-regmap, SFP driver even could not find gpio_desc. Maybe I missed something
> on registering gpio-regmap...
>
> Anyway it is a bit complicated, could I use this version of GPIO implementation if
> it's really tough?

It's possible but from GPIO subsystem point of view it's discouraged
as long as there is no technical impediment to go the regmap way.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Andrew Lunn]

> > I _think_ you are mixing upstream IRQs and downstream IRQs.
> > 
> > Interrupts are arranged in trees. The CPU itself only has one or two
> > interrupts. e.g. for ARM you have FIQ and IRQ. When the CPU gets an
> > interrupt, you look in the interrupt controller to see what external
> > or internal interrupt triggered the CPU interrupt. And that interrupt
> > controller might indicate the interrupt came from another interrupt
> > controller. Hence the tree structure. And each node in the tree is
> > considered an interrupt domain.
> > 
> > A GPIO controller can also be an interrupt controller. It has an
> > upstream interrupt, going to the controller above it. And it has
> > downstream interrupts, the GPIO lines coming into it which can cause
> > an interrupt. And the GPIO interrupt controller is a domain.
> > 
> > So what exactly does gpio_regmap_config.irq_domain mean? Is it the
> > domain of the upstream interrupt controller? Is it an empty domain
> > structure to be used by the GPIO interrupt controller? It is very
> > unlikely to have anything to do with the SFP devices below it.
> 
> Sorry, since I don't know much about interrupt,  it is difficult to understand
> regmap-irq in a short time. There are many questions about regmap-irq.
> 
> When I want to add an IRQ chip for regmap, for the further irq_domain,
> I need to pass a parameter of IRQ, and this IRQ will be requested with handler:
> regmap_irq_thread(). Which IRQ does it mean?

That is your upstream IRQ, the interrupt indicating one of your GPIO
lines has changed state.

> In the previous code of using
> devm_gpiochip_add_data(), I set the MSI-X interrupt as gpio-irq's parent, but
> it was used to set chained handler only. Should the parent be this IRQ? I found
> the error with irq_free_descs and irq_domain_remove when I remove txgbe.ko.

Do you have one MSI-X dedicated for GPIOs. Or is it your general MAC
interrupt, and you need to read an interrupt controller register to
determine it was GPIOs which triggered the interrupt?

If you are getting errors when removing the driver it means you are
missing some level of undoing what us done in probe. Are you sure
regmap_del_irq_chip() is being called on unload?

> As you said, the interrupt of each tree node has its domain. Can I understand
> that there are two layer in the interrupt tree for MSI-X and GPIOs, and requesting
> them separately is not conflicting? Although I thought so, but after I implement
> gpio-regmap, SFP driver even could not find gpio_desc. Maybe I missed something
> on registering gpio-regmap...

That is probably some sort of naming issue. You might want to add some
prints in swnode_find_gpio() and gpiochip_find() to see what it is
looking for vs what the name actually is.

	Andrew

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Michael Walle]

Hi, 

I'm currently (and for the next three weeks) on vacation. Sorry in advanced if the format of the mail is wrong or similar. I just have access to my mobile. 

Am 18. Mai 2023 14:27:00 MESZ schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
>On Thu, May 18, 2023 at 2:50 PM Jiawen Wu <jiawenwu@trustnetic.com> wrote:
>> On Wednesday, May 17, 2023 11:01 PM, Andrew Lunn wrote:
>> > On Wed, May 17, 2023 at 10:55:01AM +0800, Jiawen Wu wrote:
>
>...
>
>> > > I should provide gpio_regmap_config.irq_domain if I want to add the gpio_irq_chip.
>> > > But in this use, GPIO IRQs are requested by SFP driver. How can I get irq_domain
>> > > before SFP probe? And where do I add IRQ parent handler?
>> >
>> > I _think_ you are mixing upstream IRQs and downstream IRQs.
>> >
>> > Interrupts are arranged in trees. The CPU itself only has one or two
>> > interrupts. e.g. for ARM you have FIQ and IRQ. When the CPU gets an
>> > interrupt, you look in the interrupt controller to see what external
>> > or internal interrupt triggered the CPU interrupt. And that interrupt
>> > controller might indicate the interrupt came from another interrupt
>> > controller. Hence the tree structure. And each node in the tree is
>> > considered an interrupt domain.
>> >
>> > A GPIO controller can also be an interrupt controller. It has an
>> > upstream interrupt, going to the controller above it. And it has
>> > downstream interrupts, the GPIO lines coming into it which can cause
>> > an interrupt. And the GPIO interrupt controller is a domain.
>> >
>> > So what exactly does gpio_regmap_config.irq_domain mean? Is it the
>> > domain of the upstream interrupt controller? Is it an empty domain
>> > structure to be used by the GPIO interrupt controller? It is very
>> > unlikely to have anything to do with the SFP devices below it.
>>
>> Sorry, since I don't know much about interrupt,  it is difficult to understand
>> regmap-irq in a short time. There are many questions about regmap-irq.
>
>That's why I Cc'ed to Michael who is the author of gpio-regmap to
>probably get advice from.

All gpio remap is doing is forwarding the IRQ domain from regmap-irq to the gpio subsystem. It's opaque to gpio-regmap and outside the scope of gpio-regmap. 

-michael

>> When I want to add an IRQ chip for regmap, for the further irq_domain,
>> I need to pass a parameter of IRQ, and this IRQ will be requested with handler:
>> regmap_irq_thread(). Which IRQ does it mean? In the previous code of using
>> devm_gpiochip_add_data(), I set the MSI-X interrupt as gpio-irq's parent, but
>> it was used to set chained handler only. Should the parent be this IRQ? I found
>> the error with irq_free_descs and irq_domain_remove when I remove txgbe.ko.
>>
>> As you said, the interrupt of each tree node has its domain. Can I understand
>> that there are two layer in the interrupt tree for MSI-X and GPIOs, and requesting
>> them separately is not conflicting? Although I thought so, but after I implement
>> gpio-regmap, SFP driver even could not find gpio_desc. Maybe I missed something
>> on registering gpio-regmap...
>>
>> Anyway it is a bit complicated, could I use this version of GPIO implementation if
>> it's really tough?
>
>It's possible but from GPIO subsystem point of view it's discouraged
>as long as there is no technical impediment to go the regmap way.

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Andy Shevchenko]

On Thu, May 18, 2023 at 7:03 PM Michael Walle <michael@walle.cc> wrote:

> I'm currently (and for the next three weeks) on vacation. Sorry in advanced if the format of the mail is wrong or similar. I just have access to my mobile.

Have a good one and thank you for chiming in!

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Jiawen Wu]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, May 18, 2023 8:49 PM
> To: Jiawen Wu <jiawenwu@trustnetic.com>
> Cc: 'Andy Shevchenko' <andy.shevchenko@gmail.com>; netdev@vger.kernel.org; jarkko.nikula@linux.intel.com;
> andriy.shevchenko@linux.intel.com; mika.westerberg@linux.intel.com; jsd@semihalf.com; Jose.Abreu@synopsys.com;
> hkallweit1@gmail.com; linux@armlinux.org.uk; linux-i2c@vger.kernel.org; linux-gpio@vger.kernel.org; mengyuanlou@net-swift.com
> Subject: Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket
> 
> > > I _think_ you are mixing upstream IRQs and downstream IRQs.
> > >
> > > Interrupts are arranged in trees. The CPU itself only has one or two
> > > interrupts. e.g. for ARM you have FIQ and IRQ. When the CPU gets an
> > > interrupt, you look in the interrupt controller to see what external
> > > or internal interrupt triggered the CPU interrupt. And that interrupt
> > > controller might indicate the interrupt came from another interrupt
> > > controller. Hence the tree structure. And each node in the tree is
> > > considered an interrupt domain.
> > >
> > > A GPIO controller can also be an interrupt controller. It has an
> > > upstream interrupt, going to the controller above it. And it has
> > > downstream interrupts, the GPIO lines coming into it which can cause
> > > an interrupt. And the GPIO interrupt controller is a domain.
> > >
> > > So what exactly does gpio_regmap_config.irq_domain mean? Is it the
> > > domain of the upstream interrupt controller? Is it an empty domain
> > > structure to be used by the GPIO interrupt controller? It is very
> > > unlikely to have anything to do with the SFP devices below it.
> >
> > Sorry, since I don't know much about interrupt,  it is difficult to understand
> > regmap-irq in a short time. There are many questions about regmap-irq.
> >
> > When I want to add an IRQ chip for regmap, for the further irq_domain,
> > I need to pass a parameter of IRQ, and this IRQ will be requested with handler:
> > regmap_irq_thread(). Which IRQ does it mean?
> 
> That is your upstream IRQ, the interrupt indicating one of your GPIO
> lines has changed state.
> 
> > In the previous code of using
> > devm_gpiochip_add_data(), I set the MSI-X interrupt as gpio-irq's parent, but
> > it was used to set chained handler only. Should the parent be this IRQ? I found
> > the error with irq_free_descs and irq_domain_remove when I remove txgbe.ko.
> 
> Do you have one MSI-X dedicated for GPIOs. Or is it your general MAC
> interrupt, and you need to read an interrupt controller register to
> determine it was GPIOs which triggered the interrupt?

I have one MSI-X interrupt for all general MAC interrupt (see TXGBE_PX_MISC_IEN_MASK).
It has 32 bits to indicate various interrupts, GPIOs are the one of them. When GPIO
interrupt is determined, GPIO_INT_STATUS register should be read to determine
which GPIO line has changed state.

> If you are getting errors when removing the driver it means you are
> missing some level of undoing what us done in probe. Are you sure
> regmap_del_irq_chip() is being called on unload?

I used devm_* all when I registered them.

> > As you said, the interrupt of each tree node has its domain. Can I understand
> > that there are two layer in the interrupt tree for MSI-X and GPIOs, and requesting
> > them separately is not conflicting? Although I thought so, but after I implement
> > gpio-regmap, SFP driver even could not find gpio_desc. Maybe I missed something
> > on registering gpio-regmap...
> 
> That is probably some sort of naming issue. You might want to add some
> prints in swnode_find_gpio() and gpiochip_find() to see what it is
> looking for vs what the name actually is.

Thanks for the advice, I'll try again today.

RE: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Jiawen Wu]

On Thursday, May 18, 2023 8:49 PM, Andrew Lunn wrote:
> > > I _think_ you are mixing upstream IRQs and downstream IRQs.
> > >
> > > Interrupts are arranged in trees. The CPU itself only has one or two
> > > interrupts. e.g. for ARM you have FIQ and IRQ. When the CPU gets an
> > > interrupt, you look in the interrupt controller to see what external
> > > or internal interrupt triggered the CPU interrupt. And that interrupt
> > > controller might indicate the interrupt came from another interrupt
> > > controller. Hence the tree structure. And each node in the tree is
> > > considered an interrupt domain.
> > >
> > > A GPIO controller can also be an interrupt controller. It has an
> > > upstream interrupt, going to the controller above it. And it has
> > > downstream interrupts, the GPIO lines coming into it which can cause
> > > an interrupt. And the GPIO interrupt controller is a domain.
> > >
> > > So what exactly does gpio_regmap_config.irq_domain mean? Is it the
> > > domain of the upstream interrupt controller? Is it an empty domain
> > > structure to be used by the GPIO interrupt controller? It is very
> > > unlikely to have anything to do with the SFP devices below it.
> >
> > Sorry, since I don't know much about interrupt,  it is difficult to understand
> > regmap-irq in a short time. There are many questions about regmap-irq.
> >
> > When I want to add an IRQ chip for regmap, for the further irq_domain,
> > I need to pass a parameter of IRQ, and this IRQ will be requested with handler:
> > regmap_irq_thread(). Which IRQ does it mean?
> 
> That is your upstream IRQ, the interrupt indicating one of your GPIO
> lines has changed state.
> 
> > In the previous code of using
> > devm_gpiochip_add_data(), I set the MSI-X interrupt as gpio-irq's parent, but
> > it was used to set chained handler only. Should the parent be this IRQ? I found
> > the error with irq_free_descs and irq_domain_remove when I remove txgbe.ko.
> 
> Do you have one MSI-X dedicated for GPIOs. Or is it your general MAC
> interrupt, and you need to read an interrupt controller register to
> determine it was GPIOs which triggered the interrupt?
> 
> If you are getting errors when removing the driver it means you are
> missing some level of undoing what us done in probe. Are you sure
> regmap_del_irq_chip() is being called on unload?
> 
> > As you said, the interrupt of each tree node has its domain. Can I understand
> > that there are two layer in the interrupt tree for MSI-X and GPIOs, and requesting
> > them separately is not conflicting? Although I thought so, but after I implement
> > gpio-regmap, SFP driver even could not find gpio_desc. Maybe I missed something
> > on registering gpio-regmap...
> 
> That is probably some sort of naming issue. You might want to add some
> prints in swnode_find_gpio() and gpiochip_find() to see what it is
> looking for vs what the name actually is.

It's true for the problem of name, but there is another problem. SFP driver has
successfully got gpio_desc, then it failed to get gpio_irq from gpio_desc (with error
return -517). I traced the function gpiod_to_irq():

	gc = desc->gdev->chip;
	offset = gpio_chip_hwgpio(desc);
	if (gc->to_irq) {
		int retirq = gc->to_irq(gc, offset);

		/* Zero means NO_IRQ */
		if (!retirq)
			return -ENXIO;

		return retirq;
	}

'gc->to_irq = gpiochip_to_irq' was set in [4]gpiochip_irqchip_add_domain().
So:

	static int gpiochip_to_irq(struct gpio_chip *gc, unsigned int offset)
	{
		struct irq_domain *domain = gc->irq.domain;

	#ifdef CONFIG_GPIOLIB_IRQCHIP
		/*
		 * Avoid race condition with other code, which tries to lookup
		 * an IRQ before the irqchip has been properly registered,
		 * i.e. while gpiochip is still being brought up.
		 */
		if (!gc->irq.initialized)
			return -EPROBE_DEFER;
	#endif

gc->irq.initialized is set to true at the end of [3]gpiochip_add_irqchip() only.
Firstly, it checks if irqchip is NULL:

	static int gpiochip_add_irqchip(struct gpio_chip *gc,
					struct lock_class_key *lock_key,
					struct lock_class_key *request_key)
	{
		struct fwnode_handle *fwnode = dev_fwnode(&gc->gpiodev->dev);
		struct irq_chip *irqchip = gc->irq.chip;
		unsigned int type;
		unsigned int i;

		if (!irqchip)
			return 0;

The result shows that it was NULL, so gc->irq.initialized = false.
Above all, return irq = -EPROBE_DEFER.

So let's sort the function calls. In chronological order, [1] calls [2], [2] calls
[3], then [1] calls [4]. The irq_chip was added to irq_domain->host_data->irq_chip
before [1]. But I don't find where to convert gpio_chip->irq.domain->host_data->irq_chip
to gpio_chip->irq.chip, it seems like it should happen after [4] ? But if it wants to use
'gc->to_irq' successfully, it should happen before [3]?

[1] gpio_regmap_register()
[2] gpiochip_add_data()
[3] gpiochip_add_irqchip()
[4] gpiochip_irqchip_add_domain()

I'm sorry that I described the problem in a confusing way, apologize if I missed
some code that caused this confusion.

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Andrew Lunn]

> I have one MSI-X interrupt for all general MAC interrupt (see TXGBE_PX_MISC_IEN_MASK).
> It has 32 bits to indicate various interrupts, GPIOs are the one of them. When GPIO
> interrupt is determined, GPIO_INT_STATUS register should be read to determine
> which GPIO line has changed state.

So you have another interrupt controller above the GPIO interrupt
controller. regmap-gpio is pushing you towards describing this
interrupt controller as a Linux interrupt controller.

When you look at drivers handling interrupts, most leaf interrupt
controllers are not described as Linux interrupt controllers. The
driver interrupt handler reads the interrupt status register and
internally dispatches to the needed handler. This works well when
everything is internal to one driver.

However, here, you have two drivers involved, your MAC driver and a
GPIO driver instantiated by the MAC driver. So i think you are going
to need to described the MAC interrupt controller as a Linux interrupt
controller.

Take a look at the mv88e6xxx driver, which does this. It has two
interrupt controller embedded within it, and they are chained.

> > If you are getting errors when removing the driver it means you are
> > missing some level of undoing what us done in probe. Are you sure
> > regmap_del_irq_chip() is being called on unload?
> 
> I used devm_* all when I registered them.

Look at the ordering. Is regmap_del_irq_chip() being called too late?
I've had problems like this with the mv88e6xxx driver and its
interrupt controllers. I ended up not using devm_ so i had full
control over the order things got undone. In that case, the external
devices was PHYs, with the PHY interrupt being inside the Ethernet
switch, which i exposed using a Linux interrupt controller.

	Andrew

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Andrew Lunn]

> It's true for the problem of name, but there is another problem. SFP driver has
> successfully got gpio_desc, then it failed to get gpio_irq from gpio_desc (with error
> return -517). I traced the function gpiod_to_irq():

-517 is a number you learn after a while. -EPROBE_DEFFER. So the GPIO
controller is not fully ready when the SFP driver tries to use it.

I guess this is the missing upstream interrupt. You need to get the
order correct:

Register the MAC interrupt controller
Instantiate the regmap-gpio controller
Instantiate the I2C bus master
Instantiate the SPF devices
Instantiate PHYLINK.

	Andrew

Re: [PATCH net-next v8 2/9] i2c: designware: Add driver support for Wangxun 10Gb NIC

[Jarkko Nikula]

On 5/17/23 12:44, Andy Shevchenko wrote:
> On Wed, May 17, 2023 at 12:26 PM Jiawen Wu <jiawenwu@trustnetic.com> wrote:
>> On Wednesday, May 17, 2023 4:49 PM, Jarkko Nikula wrote:
>>> On 5/15/23 12:24, andy.shevchenko@gmail.com wrote:
>>>> Mon, May 15, 2023 at 02:31:53PM +0800, Jiawen Wu kirjoitti:
>>>>>     dev->flags = (uintptr_t)device_get_match_data(&pdev->dev);
>>>>> +  if (device_property_present(&pdev->dev, "snps,i2c-platform"))
>>>>> +          dev->flags |= MODEL_WANGXUN_SP;
>>>>
>>>> What I meant here is to use device_property_present() _iff_ you have decided to
>>>> go with the _vendor-specific_ property name.
>>>>
>>>> Otherwise it should be handled differently, i.e. with reading the actual value
>>>> of that property. Hence it should correspond the model enum, which you need to
>>>> declare in the Device Tree bindings before use.
>>>>
>>>> So, either
>>>>
>>>>      if (device_property_present(&pdev->dev, "wx,..."))
>>>>              dev->flags |= MODEL_WANGXUN_SP;
>>>>
>>>> or
>>>>
>>>>      if ((dev->flags & MODEL_MASK) == MODEL_NONE) {
>>>>      // you now have to distinguish that there is no model set in driver data
>>>>              u32 model;
>>>>
>>>>              ret = device_property_read_u32(dev, "snps,i2c-platform");
>>>>              if (ret) {
>>>>                      ...handle error...
>>>>              }
>>>>              dev->flags |= model
>>>>
>>> I'm not a device tree expert but I wonder would it be possible somehow
>>> combine this and compatible properties in dw_i2c_of_match[]? They set
>>> model flag for MODEL_MSCC_OCELOT and MODEL_BAIKAL_BT1.
>>
>> Maybe the table could be changed to match device property, instead of relying
>> on DT only. Or device_get_match_data() could be also implemented in
>> software node case?
> 
> This has been discussed [1] and still no visible prototype. Perhaps
> you can collaborate with Vladimir on the matter.
> 
> [1]: https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/
> 
Ok, not possible at the moment. Perhaps for now setting model using 
device_property_read_u32() is good enough? I asked above out of 
curiosity rather than demanding perfection. They say "Perfect is the 
enemy of good" :-)

RE: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Jiawen Wu]

On Friday, May 19, 2023 9:13 PM, Andrew Lunn wrote:
> > I have one MSI-X interrupt for all general MAC interrupt (see TXGBE_PX_MISC_IEN_MASK).
> > It has 32 bits to indicate various interrupts, GPIOs are the one of them. When GPIO
> > interrupt is determined, GPIO_INT_STATUS register should be read to determine
> > which GPIO line has changed state.
> 
> So you have another interrupt controller above the GPIO interrupt
> controller. regmap-gpio is pushing you towards describing this
> interrupt controller as a Linux interrupt controller.
> 
> When you look at drivers handling interrupts, most leaf interrupt
> controllers are not described as Linux interrupt controllers. The
> driver interrupt handler reads the interrupt status register and
> internally dispatches to the needed handler. This works well when
> everything is internal to one driver.
> 
> However, here, you have two drivers involved, your MAC driver and a
> GPIO driver instantiated by the MAC driver. So i think you are going
> to need to described the MAC interrupt controller as a Linux interrupt
> controller.
> 
> Take a look at the mv88e6xxx driver, which does this. It has two
> interrupt controller embedded within it, and they are chained.

Now I add two interrupt controllers, the first one for the MAC interrupt,
and the second one for regmap-gpio. In the second adding flow,

	irq = irq_find_mapping(txgbe->misc.domain, TXGBE_PX_MISC_GPIO_OFFSET);
	err = regmap_add_irq_chip_fwnode(fwnode, regmap, irq, 0, 0,
					 chip, &chip_data);

and then,

	config.irq_domain = regmap_irq_get_domain(chip_data);
	gpio_regmap = gpio_regmap_register(&config);

"txgbe->misc.domain" is the MAC interrupt domain. I think this flow should
be correct, but still failed to get gpio_irq from gpio_desc with err -517.

And I still have doubts about what I said earlier:
https://lore.kernel.org/netdev/20230515063200.301026-1-jiawenwu@trustnetic.com/T/#me1be68e1a1e44426ecc0dd8edf0f6b224e50630d

There really is nothing wrong with gpiochip_to_irq()??

> > > If you are getting errors when removing the driver it means you are
> > > missing some level of undoing what us done in probe. Are you sure
> > > regmap_del_irq_chip() is being called on unload?
> >
> > I used devm_* all when I registered them.
> 
> Look at the ordering. Is regmap_del_irq_chip() being called too late?
> I've had problems like this with the mv88e6xxx driver and its
> interrupt controllers. I ended up not using devm_ so i had full
> control over the order things got undone. In that case, the external
> devices was PHYs, with the PHY interrupt being inside the Ethernet
> switch, which i exposed using a Linux interrupt controller.

I use no devm_ functions to add regmap irq chip, register gpio regmap,
and call their del/unregister functions at the position corresponding to
release. irq_domain_remove() call trace still exist.

[  104.553182] Call Trace:
[  104.553184]  <TASK>
[  104.553185]  irq_domain_remove+0x2b/0xe0
[  104.553190]  regmap_del_irq_chip.part.0+0x8a/0x160
[  104.553196]  txgbe_remove_phy+0x57/0x80 [txgbe]
[  104.553201]  txgbe_remove+0x2a/0x90 [txgbe]
[  104.553205]  pci_device_remove+0x36/0xa0
[  104.553208]  device_release_driver_internal+0xaa/0x140
[  104.553213]  driver_detach+0x44/0x90
[  104.553215]  bus_remove_driver+0x69/0xf0
[  104.553217]  pci_unregister_driver+0x29/0xb0
[  104.553220]  __x64_sys_delete_module+0x145/0x240
[  104.553223]  ? exit_to_user_mode_prepare+0x3c/0x1a0
[  104.553226]  do_syscall_64+0x3b/0x90
[  104.553230]  entry_SYSCALL_64_after_hwframe+0x72/0xdc

RE: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Jiawen Wu]

> And I still have doubts about what I said earlier:
> https://lore.kernel.org/netdev/20230515063200.301026-1-
> jiawenwu@trustnetic.com/T/#me1be68e1a1e44426ecc0dd8edf0f6b224e50630d

Sorry for paste error, the true link is:
https://lore.kernel.org/netdev/02ad01d98a2b$4cd080e0$e67182a0$@trustnetic.com/

RE: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Jiawen Wu]

On Monday, May 22, 2023 5:01 PM, Jiawen Wu wrote:
> On Friday, May 19, 2023 9:13 PM, Andrew Lunn wrote:
> > > I have one MSI-X interrupt for all general MAC interrupt (see TXGBE_PX_MISC_IEN_MASK).
> > > It has 32 bits to indicate various interrupts, GPIOs are the one of them. When GPIO
> > > interrupt is determined, GPIO_INT_STATUS register should be read to determine
> > > which GPIO line has changed state.
> >
> > So you have another interrupt controller above the GPIO interrupt
> > controller. regmap-gpio is pushing you towards describing this
> > interrupt controller as a Linux interrupt controller.
> >
> > When you look at drivers handling interrupts, most leaf interrupt
> > controllers are not described as Linux interrupt controllers. The
> > driver interrupt handler reads the interrupt status register and
> > internally dispatches to the needed handler. This works well when
> > everything is internal to one driver.
> >
> > However, here, you have two drivers involved, your MAC driver and a
> > GPIO driver instantiated by the MAC driver. So i think you are going
> > to need to described the MAC interrupt controller as a Linux interrupt
> > controller.
> >
> > Take a look at the mv88e6xxx driver, which does this. It has two
> > interrupt controller embedded within it, and they are chained.
> 
> Now I add two interrupt controllers, the first one for the MAC interrupt,
> and the second one for regmap-gpio. In the second adding flow,
> 
> 	irq = irq_find_mapping(txgbe->misc.domain, TXGBE_PX_MISC_GPIO_OFFSET);
> 	err = regmap_add_irq_chip_fwnode(fwnode, regmap, irq, 0, 0,
> 					 chip, &chip_data);
> 
> and then,
> 
> 	config.irq_domain = regmap_irq_get_domain(chip_data);
> 	gpio_regmap = gpio_regmap_register(&config);
> 
> "txgbe->misc.domain" is the MAC interrupt domain. I think this flow should
> be correct, but still failed to get gpio_irq from gpio_desc with err -517.
> 
> And I still have doubts about what I said earlier:
> https://lore.kernel.org/netdev/20230515063200.301026-1-
> jiawenwu@trustnetic.com/T/#me1be68e1a1e44426ecc0dd8edf0f6b224e50630d
> 
> There really is nothing wrong with gpiochip_to_irq()??

There is indeed something wrong in gpiochip_to_irq(), since commit 5467801 ("gpio:
Restrict usage of GPIO chip irq members before initialization"):
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit?id=5467801f1fcbdc46bc7298a84dbf3ca1ff2a7320

When I use gpio_regmap_register() to add gpiochip, gpiochip_add_irqchip() will just
return 0 since irqchip = NULL, then gc->irq.initialized = false.

 Cc the committer: Shreeya Patel.

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

['Andy Shevchenko']

On Mon, May 22, 2023 at 06:58:19PM +0800, Jiawen Wu wrote:
> On Monday, May 22, 2023 5:01 PM, Jiawen Wu wrote:
> > On Friday, May 19, 2023 9:13 PM, Andrew Lunn wrote:
> > > > I have one MSI-X interrupt for all general MAC interrupt (see TXGBE_PX_MISC_IEN_MASK).
> > > > It has 32 bits to indicate various interrupts, GPIOs are the one of them. When GPIO
> > > > interrupt is determined, GPIO_INT_STATUS register should be read to determine
> > > > which GPIO line has changed state.
> > >
> > > So you have another interrupt controller above the GPIO interrupt
> > > controller. regmap-gpio is pushing you towards describing this
> > > interrupt controller as a Linux interrupt controller.
> > >
> > > When you look at drivers handling interrupts, most leaf interrupt
> > > controllers are not described as Linux interrupt controllers. The
> > > driver interrupt handler reads the interrupt status register and
> > > internally dispatches to the needed handler. This works well when
> > > everything is internal to one driver.
> > >
> > > However, here, you have two drivers involved, your MAC driver and a
> > > GPIO driver instantiated by the MAC driver. So i think you are going
> > > to need to described the MAC interrupt controller as a Linux interrupt
> > > controller.
> > >
> > > Take a look at the mv88e6xxx driver, which does this. It has two
> > > interrupt controller embedded within it, and they are chained.
> > 
> > Now I add two interrupt controllers, the first one for the MAC interrupt,
> > and the second one for regmap-gpio. In the second adding flow,
> > 
> > 	irq = irq_find_mapping(txgbe->misc.domain, TXGBE_PX_MISC_GPIO_OFFSET);
> > 	err = regmap_add_irq_chip_fwnode(fwnode, regmap, irq, 0, 0,
> > 					 chip, &chip_data);
> > 
> > and then,
> > 
> > 	config.irq_domain = regmap_irq_get_domain(chip_data);
> > 	gpio_regmap = gpio_regmap_register(&config);
> > 
> > "txgbe->misc.domain" is the MAC interrupt domain. I think this flow should
> > be correct, but still failed to get gpio_irq from gpio_desc with err -517.
> > 
> > And I still have doubts about what I said earlier:
> > https://lore.kernel.org/netdev/20230515063200.301026-1-
> > jiawenwu@trustnetic.com/T/#me1be68e1a1e44426ecc0dd8edf0f6b224e50630d
> > 
> > There really is nothing wrong with gpiochip_to_irq()??
> 
> There is indeed something wrong in gpiochip_to_irq(), since commit 5467801 ("gpio:
> Restrict usage of GPIO chip irq members before initialization"):
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit?id=5467801f1fcbdc46bc7298a84dbf3ca1ff2a7320
> 
> When I use gpio_regmap_register() to add gpiochip, gpiochip_add_irqchip() will just
> return 0 since irqchip = NULL, then gc->irq.initialized = false.

As far as I understood your hardware, you need to provide an IRQ chip for your
GPIOs. The driver that provides an IRQ chip for GPIO and uses GPIO regmap is
drivers/gpio/gpio-sl28cpld.c.

So, you need to create a proper IRQ domain tree before calling for GPIO
registration.

>  Cc the committer: Shreeya Patel.

You meant "author", right?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Jiawen Wu]

On Tuesday, May 23, 2023 5:37 AM, Andy Shevchenko wrote:
> On Mon, May 22, 2023 at 06:58:19PM +0800, Jiawen Wu wrote:
> > On Monday, May 22, 2023 5:01 PM, Jiawen Wu wrote:
> > > On Friday, May 19, 2023 9:13 PM, Andrew Lunn wrote:
> > > > > I have one MSI-X interrupt for all general MAC interrupt (see TXGBE_PX_MISC_IEN_MASK).
> > > > > It has 32 bits to indicate various interrupts, GPIOs are the one of them. When GPIO
> > > > > interrupt is determined, GPIO_INT_STATUS register should be read to determine
> > > > > which GPIO line has changed state.
> > > >
> > > > So you have another interrupt controller above the GPIO interrupt
> > > > controller. regmap-gpio is pushing you towards describing this
> > > > interrupt controller as a Linux interrupt controller.
> > > >
> > > > When you look at drivers handling interrupts, most leaf interrupt
> > > > controllers are not described as Linux interrupt controllers. The
> > > > driver interrupt handler reads the interrupt status register and
> > > > internally dispatches to the needed handler. This works well when
> > > > everything is internal to one driver.
> > > >
> > > > However, here, you have two drivers involved, your MAC driver and a
> > > > GPIO driver instantiated by the MAC driver. So i think you are going
> > > > to need to described the MAC interrupt controller as a Linux interrupt
> > > > controller.
> > > >
> > > > Take a look at the mv88e6xxx driver, which does this. It has two
> > > > interrupt controller embedded within it, and they are chained.
> > >
> > > Now I add two interrupt controllers, the first one for the MAC interrupt,
> > > and the second one for regmap-gpio. In the second adding flow,
> > >
> > > 	irq = irq_find_mapping(txgbe->misc.domain, TXGBE_PX_MISC_GPIO_OFFSET);
> > > 	err = regmap_add_irq_chip_fwnode(fwnode, regmap, irq, 0, 0,
> > > 					 chip, &chip_data);
> > >
> > > and then,
> > >
> > > 	config.irq_domain = regmap_irq_get_domain(chip_data);
> > > 	gpio_regmap = gpio_regmap_register(&config);
> > >
> > > "txgbe->misc.domain" is the MAC interrupt domain. I think this flow should
> > > be correct, but still failed to get gpio_irq from gpio_desc with err -517.
> > >
> > > And I still have doubts about what I said earlier:
> > > https://lore.kernel.org/netdev/20230515063200.301026-1-
> > > jiawenwu@trustnetic.com/T/#me1be68e1a1e44426ecc0dd8edf0f6b224e50630d
> > >
> > > There really is nothing wrong with gpiochip_to_irq()??
> >
> > There is indeed something wrong in gpiochip_to_irq(), since commit 5467801 ("gpio:
> > Restrict usage of GPIO chip irq members before initialization"):
> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit?id=5467801f1fcbdc46bc7298a84dbf3ca1ff2a7320
> >
> > When I use gpio_regmap_register() to add gpiochip, gpiochip_add_irqchip() will just
> > return 0 since irqchip = NULL, then gc->irq.initialized = false.
> 
> As far as I understood your hardware, you need to provide an IRQ chip for your
> GPIOs. The driver that provides an IRQ chip for GPIO and uses GPIO regmap is
> drivers/gpio/gpio-sl28cpld.c.
> 
> So, you need to create a proper IRQ domain tree before calling for GPIO
> registration.

I've already created it. There is the full code snippet:

+static int txgbe_gpio_init(struct txgbe *txgbe)
+{
+       struct regmap_irq_chip_data *chip_data;
+       struct gpio_regmap_config config = {};
+       struct gpio_regmap *gpio_regmap;
+       struct fwnode_handle *fwnode;
+       struct regmap_irq_chip *chip;
+       struct regmap *regmap;
+       struct pci_dev *pdev;
+       struct device *dev;
+       unsigned int irq;
+       struct wx *wx;
+       int err;
+
+       wx = txgbe->wx;
+       pdev = wx->pdev;
+       dev = &pdev->dev;
+       fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_GPIO]);
+
+       regmap = devm_regmap_init(dev, NULL, wx, &gpio_regmap_config);
+       if (IS_ERR(regmap)) {
+               wx_err(wx, "failed to init GPIO regmap\n");
+               return PTR_ERR(regmap);
+       }
+
+       chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+       if (!chip)
+               return -ENOMEM;
+
+       chip->name = "txgbe-gpio-irq";
+       chip->irq_drv_data = wx;
+       chip->num_regs = 1;
+       chip->irqs = txgbe_gpio_irqs;
+       chip->num_irqs = ARRAY_SIZE(txgbe_gpio_irqs);
+       chip->status_base = WX_GPIO_INTSTATUS;
+       chip->ack_base = WX_GPIO_EOI;
+       chip->mask_base = WX_GPIO_INTMASK;
+       chip->get_irq_reg = txgbe_get_irq_reg;
+       chip->handle_post_irq = txgbe_handle_post_irq;
+
+       irq = irq_find_mapping(txgbe->misc.domain, TXGBE_PX_MISC_GPIO_OFFSET);
+       err = regmap_add_irq_chip_fwnode(fwnode, regmap, irq, 0, 0,
+                                        chip, &chip_data);
+       if (err) {
+               wx_err(wx, "GPIO IRQ register failed\n");
+               return err;
+       }
+
+       txgbe->gpio_irq = irq;
+       txgbe->gpio_data = chip_data;
+
+       config.label = devm_kasprintf(dev, GFP_KERNEL, "txgbe_gpio-%x",
+                                     (pdev->bus->number << 8) | pdev->devfn);
+       config.parent = dev;
+       config.regmap = regmap;
+       config.fwnode = fwnode;
+       config.drvdata = txgbe;
+       config.ngpio = 6;
+       config.reg_mask_xlate = txgbe_reg_mask_xlate;
+       config.reg_dat_base = WX_GPIO_EXT;
+       config.reg_set_base = WX_GPIO_DR;
+       config.reg_dir_out_base = WX_GPIO_DDR;
+       config.irq_domain = regmap_irq_get_domain(chip_data);
+
+       gpio_regmap = gpio_regmap_register(&config);
+       if (IS_ERR(gpio_regmap)) {
+               wx_err(wx, "GPIO regmap register failed\n");
+               regmap_del_irq_chip(irq, chip_data);
+               return PTR_ERR(gpio_regmap);
+       }
+
+       txgbe->gpio_regmap = gpio_regmap;
+
+       return 0;
+}

> 
> >  Cc the committer: Shreeya Patel.
> 
> You meant "author", right?

Yes, author.
I think "gpiochip_add_data" does not take gpio-regmap case into account.

RE: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Jiawen Wu]

> > > > If you are getting errors when removing the driver it means you are
> > > > missing some level of undoing what us done in probe. Are you sure
> > > > regmap_del_irq_chip() is being called on unload?
> > >
> > > I used devm_* all when I registered them.
> >
> > Look at the ordering. Is regmap_del_irq_chip() being called too late?
> > I've had problems like this with the mv88e6xxx driver and its
> > interrupt controllers. I ended up not using devm_ so i had full
> > control over the order things got undone. In that case, the external
> > devices was PHYs, with the PHY interrupt being inside the Ethernet
> > switch, which i exposed using a Linux interrupt controller.
> 
> I use no devm_ functions to add regmap irq chip, register gpio regmap,
> and call their del/unregister functions at the position corresponding to
> release. irq_domain_remove() call trace still exist.
> 
> [  104.553182] Call Trace:
> [  104.553184]  <TASK>
> [  104.553185]  irq_domain_remove+0x2b/0xe0
> [  104.553190]  regmap_del_irq_chip.part.0+0x8a/0x160
> [  104.553196]  txgbe_remove_phy+0x57/0x80 [txgbe]
> [  104.553201]  txgbe_remove+0x2a/0x90 [txgbe]
> [  104.553205]  pci_device_remove+0x36/0xa0
> [  104.553208]  device_release_driver_internal+0xaa/0x140
> [  104.553213]  driver_detach+0x44/0x90
> [  104.553215]  bus_remove_driver+0x69/0xf0
> [  104.553217]  pci_unregister_driver+0x29/0xb0
> [  104.553220]  __x64_sys_delete_module+0x145/0x240
> [  104.553223]  ? exit_to_user_mode_prepare+0x3c/0x1a0
> [  104.553226]  do_syscall_64+0x3b/0x90
> [  104.553230]  entry_SYSCALL_64_after_hwframe+0x72/0xdc

I think this problem is caused by a conflict calling of irq_domain_remove()
between the two functions gpiochip_irqchip_remove() and regmap_del_irq_chip().
The front one is called by gpio_regmap_unregister().

I adjusted the order of release functions, regmap_del_irq_chip() first, then
gpio_regmap_unregister(). Log became:

[  383.261168] Call Trace:
[  383.261169]  <TASK>
[  383.261170]  irq_domain_remove+0x2b/0xe0
[  383.261174]  gpiochip_irqchip_remove+0xf0/0x210
[  383.261177]  gpiochip_remove+0x4a/0x110
[  383.261181]  gpio_regmap_unregister+0x12/0x20 [gpio_regmap]
[  383.261186]  txgbe_remove_phy+0x57/0x80 [txgbe]
[  383.261190]  txgbe_remove+0x2a/0x90 [txgbe]

irq_domain_remove() just free the memory of irq_domain, but its pointer address
still exists. So it will be called twice.

RE: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Jiawen Wu]

> > Anyway it is a bit complicated, could I use this version of GPIO implementation if
> > it's really tough?
> 
> It's possible but from GPIO subsystem point of view it's discouraged
> as long as there is no technical impediment to go the regmap way.

After these days of trying, I guess there are still some bugs on gpio - regmap - irq.
It looks like an compatibility issue with gpio_irq_chip and regmap_irq_chip (My rough
fixes seems to work).

Other than that, it seems to be no way to add interrupt trigger in regmap_irq_thread(),
to solve the both-edge problem for my hardware.

I'd be willing to use gpio-regmap if above issues worked out, I learned IRQ controller,
IRQ domain, etc. , after all. But if not, I'd like to implement GPIO in the original way,
it was tested to work. May I? Thanks for all your suggestions.

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Andy Shevchenko]

On Tue, May 23, 2023 at 12:57 PM Jiawen Wu <jiawenwu@trustnetic.com> wrote:
> > > Anyway it is a bit complicated, could I use this version of GPIO implementation if
> > > it's really tough?
> >
> > It's possible but from GPIO subsystem point of view it's discouraged
> > as long as there is no technical impediment to go the regmap way.
>
> After these days of trying, I guess there are still some bugs on gpio - regmap - irq.
> It looks like an compatibility issue with gpio_irq_chip and regmap_irq_chip (My rough
> fixes seems to work).
>
> Other than that, it seems to be no way to add interrupt trigger in regmap_irq_thread(),
> to solve the both-edge problem for my hardware.
>
> I'd be willing to use gpio-regmap if above issues worked out, I learned IRQ controller,
> IRQ domain, etc. , after all.

And thank you for all this!
Now you may suggest the fixes to the GPIO regmap with all your
knowledge of the area.

> But if not, I'd like to implement GPIO in the original way,
> it was tested to work. May I? Thanks for all your suggestions.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net-next v8 6/9] net: txgbe: Support GPIO to SFP socket

[Andy Shevchenko]

I just realized that we are discussing all this without GPIO
maintainers to be involved!
Cc'ed to Linus and Bart for their valuable opinions / comments.

(TL;DR: GPIO regmap seems need some fixes)

On Tue, May 23, 2023 at 2:07 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, May 23, 2023 at 12:57 PM Jiawen Wu <jiawenwu@trustnetic.com> wrote:
> > > > Anyway it is a bit complicated, could I use this version of GPIO implementation if
> > > > it's really tough?
> > >
> > > It's possible but from GPIO subsystem point of view it's discouraged
> > > as long as there is no technical impediment to go the regmap way.
> >
> > After these days of trying, I guess there are still some bugs on gpio - regmap - irq.
> > It looks like an compatibility issue with gpio_irq_chip and regmap_irq_chip (My rough
> > fixes seems to work).
> >
> > Other than that, it seems to be no way to add interrupt trigger in regmap_irq_thread(),
> > to solve the both-edge problem for my hardware.
> >
> > I'd be willing to use gpio-regmap if above issues worked out, I learned IRQ controller,
> > IRQ domain, etc. , after all.
>
> And thank you for all this!
> Now you may suggest the fixes to the GPIO regmap with all your
> knowledge of the area.
>
> > But if not, I'd like to implement GPIO in the original way,
> > it was tested to work. May I? Thanks for all your suggestions.
>
>
> --
> With Best Regards,
> Andy Shevchenko



--
With Best Regards,
Andy Shevchenko