This patch supports querying the detailed status of the port
through debugfs, including the TX/RX ring, specifications,
interrupt and port status.

This driver supports four interrupts. the abnormal interrupt has
multiple interrupt sources. To locate the exception cause in detail,
the debugfs displays the status and count of each interrupt source.

Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 .../net/ethernet/hisilicon/hibmcge/Makefile   |   3 +-
 .../ethernet/hisilicon/hibmcge/hbg_debugfs.c  | 150 ++++++++++++++++++
 .../ethernet/hisilicon/hibmcge/hbg_debugfs.h  |  12 ++
 .../net/ethernet/hisilicon/hibmcge/hbg_main.c |  26 ++-
 4 files changed, 189 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/ethernet/hisilicon/hibmcge/hbg_debugfs.c
 create mode 100644 drivers/net/ethernet/hisilicon/hibmcge/hbg_debugfs.h

diff --git a/drivers/net/ethernet/hisilicon/hibmcge/Makefile b/drivers/net/ethernet/hisilicon/hibmcge/Makefile
index ae58ac38c206..1a0ec2fb8c24 100644
--- a/drivers/net/ethernet/hisilicon/hibmcge/Makefile
+++ b/drivers/net/ethernet/hisilicon/hibmcge/Makefile
@@ -5,4 +5,5 @@
 
 obj-$(CONFIG_HIBMCGE) += hibmcge.o
 
-hibmcge-objs = hbg_main.o hbg_hw.o hbg_mdio.o hbg_irq.o hbg_txrx.o hbg_ethtool.o
+hibmcge-objs = hbg_main.o hbg_hw.o hbg_mdio.o hbg_irq.o hbg_txrx.o hbg_ethtool.o \
+		hbg_debugfs.o
diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_debugfs.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_debugfs.c
new file mode 100644
index 000000000000..e65e1d498d2b
--- /dev/null
+++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_debugfs.c
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Copyright (c) 2024 Hisilicon Limited.
+
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/etherdevice.h>
+#include <linux/seq_file.h>
+#include "hbg_common.h"
+#include "hbg_debugfs.h"
+#include "hbg_hw.h"
+#include "hbg_irq.h"
+#include "hbg_txrx.h"
+
+static struct dentry *hbg_dbgfs_root;
+
+struct hbg_dbg_info {
+	const char *name;
+	int (*read)(struct seq_file *seq, void *data);
+};
+
+#define hbg_get_bool_str(state) ((state) ? "true" : "false")
+
+static int hbg_dbg_dev_spec(struct seq_file *s, void *unused)
+{
+	struct net_device *netdev = dev_get_drvdata(s->private);
+	struct hbg_priv *priv = netdev_priv(netdev);
+	struct hbg_dev_specs *specs;
+
+	specs = &priv->dev_specs;
+	seq_printf(s, "mac id: %u\n", specs->mac_id);
+	seq_printf(s, "phy addr: %u\n", specs->phy_addr);
+	seq_printf(s, "mac addr: %pM\n", specs->mac_addr.sa_data);
+	seq_printf(s, "vlan layers: %u\n", specs->vlan_layers);
+	seq_printf(s, "max frame len: %u\n", specs->max_frame_len);
+	seq_printf(s, "min mtu: %u, max mtu: %u\n",
+		   specs->min_mtu, specs->max_mtu);
+	seq_printf(s, "mdio frequency: %u\n", specs->mdio_frequency);
+
+	return 0;
+}
+
+static void hbg_dbg_ring(struct hbg_priv *priv, struct hbg_ring *ring,
+			 struct seq_file *s)
+{
+	u32 irq_mask = ring->dir == HBG_DIR_TX ? HBG_INT_MSK_TX_B :
+						 HBG_INT_MSK_RX_B;
+
+	seq_printf(s, "ring used num: %u\n",
+		   hbg_get_queue_used_num(ring));
+	seq_printf(s, "ring max num: %u\n", ring->len);
+	seq_printf(s, "ring head: %u, tail: %u\n", ring->head, ring->tail);
+	seq_printf(s, "fifo used num: %u\n",
+		   hbg_hw_get_fifo_used_num(priv, ring->dir));
+	seq_printf(s, "fifo max num: %u\n",
+		   hbg_get_spec_fifo_max_num(priv, ring->dir));
+	seq_printf(s, "irq enabled: %s\n",
+		   hbg_get_bool_str(hbg_hw_irq_is_enabled(priv, irq_mask)));
+}
+
+static int hbg_dbg_tx_ring(struct seq_file *s, void *unused)
+{
+	struct net_device *netdev = dev_get_drvdata(s->private);
+	struct hbg_priv *priv = netdev_priv(netdev);
+
+	hbg_dbg_ring(priv, &priv->tx_ring, s);
+	return 0;
+}
+
+static int hbg_dbg_rx_ring(struct seq_file *s, void *unused)
+{
+	struct net_device *netdev = dev_get_drvdata(s->private);
+	struct hbg_priv *priv = netdev_priv(netdev);
+
+	hbg_dbg_ring(priv, &priv->rx_ring, s);
+	return 0;
+}
+
+static int hbg_dbg_irq_info(struct seq_file *s, void *unused)
+{
+	struct net_device *netdev = dev_get_drvdata(s->private);
+	struct hbg_priv *priv = netdev_priv(netdev);
+	struct hbg_irq_info *info;
+	u32 i;
+
+	for (i = 0; i < priv->vectors.info_array_len; i++) {
+		info = &priv->vectors.info_array[i];
+		seq_printf(s,
+			   "%-20s: is enabled: %s, print: %s, count: %llu\n",
+			   info->name,
+			   hbg_get_bool_str(hbg_hw_irq_is_enabled(priv,
+								  info->mask)),
+			   hbg_get_bool_str(info->need_print),
+			   info->count);
+	}
+
+	return 0;
+}
+
+static int hbg_dbg_nic_state(struct seq_file *s, void *unused)
+{
+	struct net_device *netdev = dev_get_drvdata(s->private);
+	struct hbg_priv *priv = netdev_priv(netdev);
+
+	seq_printf(s, "event handling state: %s\n",
+		   hbg_get_bool_str(test_bit(HBG_NIC_STATE_EVENT_HANDLING,
+					     &priv->state)));
+
+	seq_printf(s, "tx timeout cnt: %llu\n", priv->stats.tx_timeout_cnt);
+	return 0;
+}
+
+static const struct hbg_dbg_info hbg_dbg_infos[] = {
+	{ "dev_spec", hbg_dbg_dev_spec },
+	{ "tx_ring", hbg_dbg_tx_ring },
+	{ "rx_ring", hbg_dbg_rx_ring },
+	{ "irq_info", hbg_dbg_irq_info },
+	{ "nic_state", hbg_dbg_nic_state },
+};
+
+static void hbg_debugfs_uninit(void *data)
+{
+	debugfs_remove_recursive((struct dentry *)data);
+}
+
+int hbg_debugfs_init(struct hbg_priv *priv)
+{
+	const char *name = pci_name(priv->pdev);
+	struct device *dev = &priv->pdev->dev;
+	struct dentry *root;
+	u32 i;
+
+	root = debugfs_create_dir(name, hbg_dbgfs_root);
+
+	for (i = 0; i < ARRAY_SIZE(hbg_dbg_infos); i++)
+		debugfs_create_devm_seqfile(dev, hbg_dbg_infos[i].name,
+					    root, hbg_dbg_infos[i].read);
+
+	return devm_add_action_or_reset(dev, hbg_debugfs_uninit, root);
+}
+
+void hbg_debugfs_register(void)
+{
+	hbg_dbgfs_root = debugfs_create_dir("hibmcge", NULL);
+}
+
+void hbg_debugfs_unregister(void)
+{
+	debugfs_remove_recursive(hbg_dbgfs_root);
+	hbg_dbgfs_root = NULL;
+}
diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_debugfs.h b/drivers/net/ethernet/hisilicon/hibmcge/hbg_debugfs.h
new file mode 100644
index 000000000000..678651ec710b
--- /dev/null
+++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_debugfs.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/* Copyright (c) 2024 Hisilicon Limited. */
+
+#ifndef __HBG_DEBUGFS_H
+#define __HBG_DEBUGFS_H
+
+void hbg_debugfs_register(void);
+void hbg_debugfs_unregister(void);
+
+int hbg_debugfs_init(struct hbg_priv *priv);
+
+#endif
diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c
index 33fe92104e90..30576483a938 100644
--- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c
+++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c
@@ -11,6 +11,7 @@
 #include "hbg_irq.h"
 #include "hbg_mdio.h"
 #include "hbg_txrx.h"
+#include "hbg_debugfs.h"
 
 static void hbg_change_mtu(struct hbg_priv *priv, int new_mtu);
 
@@ -209,6 +210,10 @@ static int hbg_init(struct hbg_priv *priv)
 	if (ret)
 		return ret;
 
+	ret = hbg_debugfs_init(priv);
+	if (ret)
+		return ret;
+
 	hbg_delaywork_init(priv);
 	return devm_add_action_or_reset(&priv->pdev->dev, hbg_delaywork_uninit,
 					&priv->service_task);
@@ -296,7 +301,26 @@ static struct pci_driver hbg_driver = {
 	.id_table	= hbg_pci_tbl,
 	.probe		= hbg_probe,
 };
-module_pci_driver(hbg_driver);
+
+static int __init hbg_module_init(void)
+{
+	int ret;
+
+	hbg_debugfs_register();
+	ret = pci_register_driver(&hbg_driver);
+	if (ret)
+		hbg_debugfs_unregister();
+
+	return ret;
+}
+module_init(hbg_module_init);
+
+static void __exit hbg_module_exit(void)
+{
+	pci_unregister_driver(&hbg_driver);
+	hbg_debugfs_unregister();
+}
+module_exit(hbg_module_exit);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Huawei Tech. Co., Ltd.");
-- 
2.33.0

> +static int hbg_dbg_dev_spec(struct seq_file *s, void *unused)
> +{
> +	struct net_device *netdev = dev_get_drvdata(s->private);
> +	struct hbg_priv *priv = netdev_priv(netdev);
> +	struct hbg_dev_specs *specs;
> +
> +	specs = &priv->dev_specs;
> +	seq_printf(s, "mac id: %u\n", specs->mac_id);
> +	seq_printf(s, "phy addr: %u\n", specs->phy_addr);
> +	seq_printf(s, "mac addr: %pM\n", specs->mac_addr.sa_data);
> +	seq_printf(s, "vlan layers: %u\n", specs->vlan_layers);
> +	seq_printf(s, "max frame len: %u\n", specs->max_frame_len);
> +	seq_printf(s, "min mtu: %u, max mtu: %u\n",
> +		   specs->min_mtu, specs->max_mtu);

I think these are all available via standard APIs. There is no need to
have them in debugfs as well.

> +	seq_printf(s, "mdio frequency: %u\n", specs->mdio_frequency);

Is this interesting? Are you clocking it greater than 2.5MHz?

> +static int hbg_dbg_irq_info(struct seq_file *s, void *unused)
> +{
> +	struct net_device *netdev = dev_get_drvdata(s->private);
> +	struct hbg_priv *priv = netdev_priv(netdev);
> +	struct hbg_irq_info *info;
> +	u32 i;
> +
> +	for (i = 0; i < priv->vectors.info_array_len; i++) {
> +		info = &priv->vectors.info_array[i];
> +		seq_printf(s,
> +			   "%-20s: is enabled: %s, print: %s, count: %llu\n",
> +			   info->name,
> +			   hbg_get_bool_str(hbg_hw_irq_is_enabled(priv,
> +								  info->mask)),
> +			   hbg_get_bool_str(info->need_print),
> +			   info->count);
> +	}

How does this differ from what is available already from the IRQ
subsystem?

> +static int hbg_dbg_nic_state(struct seq_file *s, void *unused)
> +{
> +	struct net_device *netdev = dev_get_drvdata(s->private);
> +	struct hbg_priv *priv = netdev_priv(netdev);
> +
> +	seq_printf(s, "event handling state: %s\n",
> +		   hbg_get_bool_str(test_bit(HBG_NIC_STATE_EVENT_HANDLING,
> +					     &priv->state)));
> +
> +	seq_printf(s, "tx timeout cnt: %llu\n", priv->stats.tx_timeout_cnt);

Don't you have this via ethtool -S ?

> @@ -209,6 +210,10 @@ static int hbg_init(struct hbg_priv *priv)
>  	if (ret)
>  		return ret;
>  
> +	ret = hbg_debugfs_init(priv);
> +	if (ret)
> +		return ret;
> +

There is no need to test the results from debugfs calls.

> +static int __init hbg_module_init(void)
> +{
> +	int ret;
> +
> +	hbg_debugfs_register();
> +	ret = pci_register_driver(&hbg_driver);
> +	if (ret)
> +		hbg_debugfs_unregister();

This seems odd. I would expect that each device has its own debugfs,
there is nothing global.

	Andrew

on 2024/10/23 22:00, Andrew Lunn wrote:
>> +static int hbg_dbg_dev_spec(struct seq_file *s, void *unused)
>> +{
>> +	struct net_device *netdev = dev_get_drvdata(s->private);
>> +	struct hbg_priv *priv = netdev_priv(netdev);
>> +	struct hbg_dev_specs *specs;
>> +
>> +	specs = &priv->dev_specs;
>> +	seq_printf(s, "mac id: %u\n", specs->mac_id);
>> +	seq_printf(s, "phy addr: %u\n", specs->phy_addr);
>> +	seq_printf(s, "mac addr: %pM\n", specs->mac_addr.sa_data);
>> +	seq_printf(s, "vlan layers: %u\n", specs->vlan_layers);
>> +	seq_printf(s, "max frame len: %u\n", specs->max_frame_len);
>> +	seq_printf(s, "min mtu: %u, max mtu: %u\n",
>> +		   specs->min_mtu, specs->max_mtu);
> I think these are all available via standard APIs. There is no need to
> have them in debugfs as well.

Yes, and these specifications are displayed by running the ethtool -d command. You can delete these specifications,
We will discuss internally, there is a high probability that this debugfs file will be deleted in v2.

>
>> +	seq_printf(s, "mdio frequency: %u\n", specs->mdio_frequency);
> Is this interesting? Are you clocking it greater than 2.5MHz?

MDIO controller supports 1MHz, 2.5MHz, 12.5MHz, and 25MHz
Of course, we chose and tested 2.5M in actual work, but this can be modified.

>
>> +static int hbg_dbg_irq_info(struct seq_file *s, void *unused)
>> +{
>> +	struct net_device *netdev = dev_get_drvdata(s->private);
>> +	struct hbg_priv *priv = netdev_priv(netdev);
>> +	struct hbg_irq_info *info;
>> +	u32 i;
>> +
>> +	for (i = 0; i < priv->vectors.info_array_len; i++) {
>> +		info = &priv->vectors.info_array[i];
>> +		seq_printf(s,
>> +			   "%-20s: is enabled: %s, print: %s, count: %llu\n",
>> +			   info->name,
>> +			   hbg_get_bool_str(hbg_hw_irq_is_enabled(priv,
>> +								  info->mask)),
>> +			   hbg_get_bool_str(info->need_print),
>> +			   info->count);
>> +	}
> How does this differ from what is available already from the IRQ
> subsystem?

We requested three interrupts: "tx", "rx", "err"
The err interrupt is a summary interrupt. We distinguish different errors
based on the status register and mask.

With "cat /proc/interrupts | grep hibmcge",
we can't distinguish the detailed cause of the error,
so we added this file to debugfs.

the following effects are achieved:
[root@localhost sjj]# cat /sys/kernel/debug/hibmcge/0000\:83\:00.1/irq_info
RX                  : is enabled: true, print: false, count: 2
TX                  : is enabled: true, print: false, count: 0
MAC_MII_FIFO_ERR    : is enabled: false, print: true, count: 0
MAC_PCS_RX_FIFO_ERR : is enabled: false, print: true, count: 0
MAC_PCS_TX_FIFO_ERR : is enabled: false, print: true, count: 0
MAC_APP_RX_FIFO_ERR : is enabled: false, print: true, count: 0
MAC_APP_TX_FIFO_ERR : is enabled: false, print: true, count: 0
SRAM_PARITY_ERR     : is enabled: true, print: true, count: 0
TX_AHB_ERR          : is enabled: true, print: true, count: 0
RX_BUF_AVL          : is enabled: true, print: false, count: 0
REL_BUF_ERR         : is enabled: true, print: true, count: 0
TXCFG_AVL           : is enabled: true, print: false, count: 0
TX_DROP             : is enabled: true, print: false, count: 0
RX_DROP             : is enabled: true, print: false, count: 0
RX_AHB_ERR          : is enabled: true, print: true, count: 0
MAC_FIFO_ERR        : is enabled: true, print: false, count: 0
RBREQ_ERR           : is enabled: true, print: false, count: 0
WE_ERR              : is enabled: true, print: false, count: 0


The irq framework of hibmcge driver also includes tx/rx interrupts.
Therefore, these interrupts are not distinguished separately in debugfs.

>
>> +static int hbg_dbg_nic_state(struct seq_file *s, void *unused)
>> +{
>> +	struct net_device *netdev = dev_get_drvdata(s->private);
>> +	struct hbg_priv *priv = netdev_priv(netdev);
>> +
>> +	seq_printf(s, "event handling state: %s\n",
>> +		   hbg_get_bool_str(test_bit(HBG_NIC_STATE_EVENT_HANDLING,
>> +					     &priv->state)));
>> +
>> +	seq_printf(s, "tx timeout cnt: %llu\n", priv->stats.tx_timeout_cnt);
> Don't you have this via ethtool -S ?

Although tx_timeout_cnt is a statistical item, it is not displayed in the ethtool -S.

>
>> @@ -209,6 +210,10 @@ static int hbg_init(struct hbg_priv *priv)
>>   	if (ret)
>>   		return ret;
>>   
>> +	ret = hbg_debugfs_init(priv);
>> +	if (ret)
>> +		return ret;
>> +
> There is no need to test the results from debugfs calls.

ok

>
>> +static int __init hbg_module_init(void)
>> +{
>> +	int ret;
>> +
>> +	hbg_debugfs_register();
>> +	ret = pci_register_driver(&hbg_driver);
>> +	if (ret)
>> +		hbg_debugfs_unregister();
> This seems odd. I would expect that each device has its own debugfs,
> there is nothing global.
>
> 	Andrew

Yes, that's how we designed it.
In this, We register and create the root dir of hibmcge,
And in each probe(), device create their own dir using bdf:

/sys/kernel/debug/hibmcge/0000\:83\:00.1/
/sys/kernel/debug/hibmcge/0000\:83\:00.2/

for each device:
[root@localhost sjj]# ls -n /sys/kernel/debug/hibmcge/0000\:83\:00.1/
-r--r--r--. 1 0 0 0 10月 24 09:42 dev_spec
-r--r--r--. 1 0 0 0 10月 24 09:42 irq_info
-r--r--r--. 1 0 0 0 10月 24 09:42 mac_talbe
-r--r--r--. 1 0 0 0 10月 24 09:42 nic_state
-r--r--r--. 1 0 0 0 10月 24 09:42 rx_ring
-r--r--r--. 1 0 0 0 10月 24 09:42 tx_ring


Thanks a lot!
Jijie Shao

> > > +	seq_printf(s, "mdio frequency: %u\n", specs->mdio_frequency);
> > Is this interesting? Are you clocking it greater than 2.5MHz?
> 
> MDIO controller supports 1MHz, 2.5MHz, 12.5MHz, and 25MHz
> Of course, we chose and tested 2.5M in actual work, but this can be modified.

How? What API are you using it allow it to be modified? Why cannot you
get the value using the same API?

> We requested three interrupts: "tx", "rx", "err"
> The err interrupt is a summary interrupt. We distinguish different errors
> based on the status register and mask.
> 
> With "cat /proc/interrupts | grep hibmcge",
> we can't distinguish the detailed cause of the error,
> so we added this file to debugfs.
> 
> the following effects are achieved:
> [root@localhost sjj]# cat /sys/kernel/debug/hibmcge/0000\:83\:00.1/irq_info
> RX                  : is enabled: true, print: false, count: 2
> TX                  : is enabled: true, print: false, count: 0
> MAC_MII_FIFO_ERR    : is enabled: false, print: true, count: 0
> MAC_PCS_RX_FIFO_ERR : is enabled: false, print: true, count: 0
> MAC_PCS_TX_FIFO_ERR : is enabled: false, print: true, count: 0
> MAC_APP_RX_FIFO_ERR : is enabled: false, print: true, count: 0
> MAC_APP_TX_FIFO_ERR : is enabled: false, print: true, count: 0
> SRAM_PARITY_ERR     : is enabled: true, print: true, count: 0
> TX_AHB_ERR          : is enabled: true, print: true, count: 0
> RX_BUF_AVL          : is enabled: true, print: false, count: 0
> REL_BUF_ERR         : is enabled: true, print: true, count: 0
> TXCFG_AVL           : is enabled: true, print: false, count: 0
> TX_DROP             : is enabled: true, print: false, count: 0
> RX_DROP             : is enabled: true, print: false, count: 0
> RX_AHB_ERR          : is enabled: true, print: true, count: 0
> MAC_FIFO_ERR        : is enabled: true, print: false, count: 0
> RBREQ_ERR           : is enabled: true, print: false, count: 0
> WE_ERR              : is enabled: true, print: false, count: 0
> 
> 
> The irq framework of hibmcge driver also includes tx/rx interrupts.
> Therefore, these interrupts are not distinguished separately in debugfs.

Please make this a patch of its own, and include this in the commit
message.

Ideally you need to show there is no standard API for what you want to
put into debugfs, because if there is a standard API, you don't need
debugfs...

> 
> > 
> > > +static int hbg_dbg_nic_state(struct seq_file *s, void *unused)
> > > +{
> > > +	struct net_device *netdev = dev_get_drvdata(s->private);
> > > +	struct hbg_priv *priv = netdev_priv(netdev);
> > > +
> > > +	seq_printf(s, "event handling state: %s\n",
> > > +		   hbg_get_bool_str(test_bit(HBG_NIC_STATE_EVENT_HANDLING,
> > > +					     &priv->state)));
> > > +
> > > +	seq_printf(s, "tx timeout cnt: %llu\n", priv->stats.tx_timeout_cnt);
> > Don't you have this via ethtool -S ?
> 
> Although tx_timeout_cnt is a statistical item, it is not displayed in the ethtool -S.

Why?

	Andrew

on 2024/10/24 20:05, Andrew Lunn wrote:
>>>> +	seq_printf(s, "mdio frequency: %u\n", specs->mdio_frequency);
>>> Is this interesting? Are you clocking it greater than 2.5MHz?
>> MDIO controller supports 1MHz, 2.5MHz, 12.5MHz, and 25MHz
>> Of course, we chose and tested 2.5M in actual work, but this can be modified.
> How? What API are you using it allow it to be modified? Why cannot you
> get the value using the same API?

This frequency cannot be modified dynamically.
There are some specification registers that store some initialization configuration parameters
written by the BMC, such as the default MAC address and hardware FIFO size and mdio frequency.

When the device is in prob, the driver reads the related configuration information and
initializes the device based on the configuration.

>
>> We requested three interrupts: "tx", "rx", "err"
>> The err interrupt is a summary interrupt. We distinguish different errors
>> based on the status register and mask.
>>
>> With "cat /proc/interrupts | grep hibmcge",
>> we can't distinguish the detailed cause of the error,
>> so we added this file to debugfs.
>>
>> the following effects are achieved:
>> [root@localhost sjj]# cat /sys/kernel/debug/hibmcge/0000\:83\:00.1/irq_info
>> RX                  : is enabled: true, print: false, count: 2
>> TX                  : is enabled: true, print: false, count: 0
>> MAC_MII_FIFO_ERR    : is enabled: false, print: true, count: 0
>> MAC_PCS_RX_FIFO_ERR : is enabled: false, print: true, count: 0
>> MAC_PCS_TX_FIFO_ERR : is enabled: false, print: true, count: 0
>> MAC_APP_RX_FIFO_ERR : is enabled: false, print: true, count: 0
>> MAC_APP_TX_FIFO_ERR : is enabled: false, print: true, count: 0
>> SRAM_PARITY_ERR     : is enabled: true, print: true, count: 0
>> TX_AHB_ERR          : is enabled: true, print: true, count: 0
>> RX_BUF_AVL          : is enabled: true, print: false, count: 0
>> REL_BUF_ERR         : is enabled: true, print: true, count: 0
>> TXCFG_AVL           : is enabled: true, print: false, count: 0
>> TX_DROP             : is enabled: true, print: false, count: 0
>> RX_DROP             : is enabled: true, print: false, count: 0
>> RX_AHB_ERR          : is enabled: true, print: true, count: 0
>> MAC_FIFO_ERR        : is enabled: true, print: false, count: 0
>> RBREQ_ERR           : is enabled: true, print: false, count: 0
>> WE_ERR              : is enabled: true, print: false, count: 0
>>
>>
>> The irq framework of hibmcge driver also includes tx/rx interrupts.
>> Therefore, these interrupts are not distinguished separately in debugfs.
> Please make this a patch of its own, and include this in the commit
> message.
>
> Ideally you need to show there is no standard API for what you want to
> put into debugfs, because if there is a standard API, you don't need
> debugfs...

Because standard API don't meet my needs, I added detailed interrupt information to debugfs.
I'll add a detailed description to the commit message of v2.

>
>>>> +static int hbg_dbg_nic_state(struct seq_file *s, void *unused)
>>>> +{
>>>> +	struct net_device *netdev = dev_get_drvdata(s->private);
>>>> +	struct hbg_priv *priv = netdev_priv(netdev);
>>>> +
>>>> +	seq_printf(s, "event handling state: %s\n",
>>>> +		   hbg_get_bool_str(test_bit(HBG_NIC_STATE_EVENT_HANDLING,
>>>> +					     &priv->state)));
>>>> +
>>>> +	seq_printf(s, "tx timeout cnt: %llu\n", priv->stats.tx_timeout_cnt);
>>> Don't you have this via ethtool -S ?
>> Although tx_timeout_cnt is a statistical item, it is not displayed in the ethtool -S.
> Why?
>
> 	Andrew

This was decided by our internal discussion before,
and we'll revisit it, and move it to ethtool -S in the next version if it's okay with us.

Thanks,
Jijie Shao

On Thu, Oct 24, 2024 at 10:06:14PM +0800, Jijie Shao wrote:
> 
> on 2024/10/24 20:05, Andrew Lunn wrote:
> > > > > +	seq_printf(s, "mdio frequency: %u\n", specs->mdio_frequency);
> > > > Is this interesting? Are you clocking it greater than 2.5MHz?
> > > MDIO controller supports 1MHz, 2.5MHz, 12.5MHz, and 25MHz
> > > Of course, we chose and tested 2.5M in actual work, but this can be modified.
> > How? What API are you using it allow it to be modified? Why cannot you
> > get the value using the same API?
> 
> This frequency cannot be modified dynamically.
> There are some specification registers that store some initialization configuration parameters
> written by the BMC, such as the default MAC address and hardware FIFO size and mdio frequency.
> 
> When the device is in prob, the driver reads the related configuration information and
> initializes the device based on the configuration.

Does the BMC have an API to set these values? And show these values?

	Andrew

on 2024/10/24 22:21, Andrew Lunn wrote:
> On Thu, Oct 24, 2024 at 10:06:14PM +0800, Jijie Shao wrote:
>> on 2024/10/24 20:05, Andrew Lunn wrote:
>>>>>> +	seq_printf(s, "mdio frequency: %u\n", specs->mdio_frequency);
>>>>> Is this interesting? Are you clocking it greater than 2.5MHz?
>>>> MDIO controller supports 1MHz, 2.5MHz, 12.5MHz, and 25MHz
>>>> Of course, we chose and tested 2.5M in actual work, but this can be modified.
>>> How? What API are you using it allow it to be modified? Why cannot you
>>> get the value using the same API?
>> This frequency cannot be modified dynamically.
>> There are some specification registers that store some initialization configuration parameters
>> written by the BMC, such as the default MAC address and hardware FIFO size and mdio frequency.
>>
>> When the device is in prob, the driver reads the related configuration information and
>> initializes the device based on the configuration.
> Does the BMC have an API to set these values? And show these values?
>
> 	Andrew

Currently, there are no other API except devmem.

But this is not important.
According to the discussion in patch "[PATCH net-next 4/7] net: hibmcge: Add register dump supported in this module",
this debugfs file will be deleted. I will put these informations in register dump by ethtool -d.

Thanks