diff mbox series

[net-next,4/7] net: hibmcge: Add register dump supported in this module

Message ID 20241023134213.3359092-5-shaojijie@huawei.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Support some features for the HIBMCGE driver | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5 this patch: 5
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 4 this patch: 10
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 280 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jijie Shao Oct. 23, 2024, 1:42 p.m. UTC 
With the ethtool of a specific version,
the following effects are achieved:

[root@localhost sjj]# ./ethtool -d enp131s0f1
[SPEC] VALID                    [0x0000]: 0x00000001
[SPEC] EVENT_REQ                [0x0004]: 0x00000000
[SPEC] MAC_ID                   [0x0008]: 0x00000002
[SPEC] PHY_ADDR                 [0x000c]: 0x00000002
[SPEC] MAC_ADDR_L               [0x0010]: 0x00000808
[SPEC] MAC_ADDR_H               [0x0014]: 0x08080802
[SPEC] UC_MAX_NUM               [0x0018]: 0x00000004
[SPEC] MAX_MTU                  [0x0028]: 0x00000fc2
[SPEC] MIN_MTU                  [0x002c]: 0x00000100
[SPEC] TX_FIFO_NUM              [0x0030]: 0x00000040
[SPEC] RX_FIFO_NUM              [0x0034]: 0x0000007f
[SPEC] VLAN_LAYERS              [0x0038]: 0x00000002
[MDIO] COMMAND_REG              [0x0000]: 0x0000185f
[MDIO] ADDR_REG                 [0x0004]: 0x00000000
[MDIO] WDATA_REG                [0x0008]: 0x0000a000
[MDIO] RDATA_REG                [0x000c]: 0x00000000
[MDIO] STA_REG                  [0x0010]: 0x00000000
[GMAC] DUPLEX_TYPE              [0x0008]: 0x00000001
[GMAC] FD_FC_TYPE               [0x000c]: 0x00008808
[GMAC] FC_TX_TIMER              [0x001c]: 0x000000ff
[GMAC] FD_FC_ADDR_LOW           [0x0020]: 0xc2000001
[GMAC] FD_FC_ADDR_HIGH          [0x0024]: 0x00000180
[GMAC] MAX_FRM_SIZE             [0x003c]: 0x000005f6
[GMAC] PORT_MODE                [0x0040]: 0x00000002
[GMAC] PORT_EN                  [0x0044]: 0x00000006
...

Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 .../ethernet/hisilicon/hibmcge/hbg_ethtool.c  | 163 ++++++++++++++++++
 .../net/ethernet/hisilicon/hibmcge/hbg_reg.h  |  34 ++++
 2 files changed, 197 insertions(+)

Comments

Andrew Lunn Oct. 23, 2024, 2:13 p.m. UTC | #1 
On Wed, Oct 23, 2024 at 09:42:10PM +0800, Jijie Shao wrote:
> With the ethtool of a specific version,
> the following effects are achieved:
> 
> [root@localhost sjj]# ./ethtool -d enp131s0f1
> [SPEC] VALID                    [0x0000]: 0x00000001
> [SPEC] EVENT_REQ                [0x0004]: 0x00000000
> [SPEC] MAC_ID                   [0x0008]: 0x00000002
> [SPEC] PHY_ADDR                 [0x000c]: 0x00000002
> [SPEC] MAC_ADDR_L               [0x0010]: 0x00000808
> [SPEC] MAC_ADDR_H               [0x0014]: 0x08080802
> [SPEC] UC_MAX_NUM               [0x0018]: 0x00000004
> [SPEC] MAX_MTU                  [0x0028]: 0x00000fc2
> [SPEC] MIN_MTU                  [0x002c]: 0x00000100

Seems like this makes your debugfs patches redundant?

> +static u32 hbg_get_reg_info(struct hbg_priv *priv,
> +			    const struct hbg_reg_type_info *type_info,
> +			    const struct hbg_reg_offset_name_map *reg_map,
> +			    struct hbg_reg_info *info)
> +{
> +	info->val = hbg_reg_read(priv, reg_map->reg_offset);
> +	info->offset = reg_map->reg_offset - type_info->offset_base;
> +	snprintf(info->name, sizeof(info->name),
> +		 "[%s] %s", type_info->name, reg_map->name);
> +
> +	return sizeof(*info);
> +}
> +
> +static void hbg_ethtool_get_regs(struct net_device *netdev,
> +				 struct ethtool_regs *regs, void *data)
> +{
> +	struct hbg_priv *priv = netdev_priv(netdev);
> +	const struct hbg_reg_type_info *info;
> +	u32 i, j, offset = 0;
> +
> +	regs->version = 0;
> +	for (i = 0; i < ARRAY_SIZE(hbg_type_infos); i++) {
> +		info = &hbg_type_infos[i];
> +		for (j = 0; j < info->reg_num; j++)
> +			offset += hbg_get_reg_info(priv, info,
> +						   &info->reg_maps[j],
> +						   data + offset);
> +	}
> +}

data is supposed to be just raw values, dumped from registers in the
device. You appear to be passing back ASCII text. It is supposed to be
ethtool which does the pretty print, not the kernel driver.

    Andrew

---
pw-bot: cr
Jijie Shao Oct. 24, 2024, 3:43 a.m. UTC | #2 
on 2024/10/23 22:13, Andrew Lunn wrote:
> On Wed, Oct 23, 2024 at 09:42:10PM +0800, Jijie Shao wrote:
>> With the ethtool of a specific version,
>> the following effects are achieved:
>>
>> [root@localhost sjj]# ./ethtool -d enp131s0f1
>> [SPEC] VALID                    [0x0000]: 0x00000001
>> [SPEC] EVENT_REQ                [0x0004]: 0x00000000
>> [SPEC] MAC_ID                   [0x0008]: 0x00000002
>> [SPEC] PHY_ADDR                 [0x000c]: 0x00000002
>> [SPEC] MAC_ADDR_L               [0x0010]: 0x00000808
>> [SPEC] MAC_ADDR_H               [0x0014]: 0x08080802
>> [SPEC] UC_MAX_NUM               [0x0018]: 0x00000004
>> [SPEC] MAX_MTU                  [0x0028]: 0x00000fc2
>> [SPEC] MIN_MTU                  [0x002c]: 0x00000100
> Seems like this makes your debugfs patches redundant?

Yes, the debugfs will be removed.

>
>> +static u32 hbg_get_reg_info(struct hbg_priv *priv,
>> +			    const struct hbg_reg_type_info *type_info,
>> +			    const struct hbg_reg_offset_name_map *reg_map,
>> +			    struct hbg_reg_info *info)
>> +{
>> +	info->val = hbg_reg_read(priv, reg_map->reg_offset);
>> +	info->offset = reg_map->reg_offset - type_info->offset_base;
>> +	snprintf(info->name, sizeof(info->name),
>> +		 "[%s] %s", type_info->name, reg_map->name);
>> +
>> +	return sizeof(*info);
>> +}
>> +
>> +static void hbg_ethtool_get_regs(struct net_device *netdev,
>> +				 struct ethtool_regs *regs, void *data)
>> +{
>> +	struct hbg_priv *priv = netdev_priv(netdev);
>> +	const struct hbg_reg_type_info *info;
>> +	u32 i, j, offset = 0;
>> +
>> +	regs->version = 0;
>> +	for (i = 0; i < ARRAY_SIZE(hbg_type_infos); i++) {
>> +		info = &hbg_type_infos[i];
>> +		for (j = 0; j < info->reg_num; j++)
>> +			offset += hbg_get_reg_info(priv, info,
>> +						   &info->reg_maps[j],
>> +						   data + offset);
>> +	}
>> +}
> data is supposed to be just raw values, dumped from registers in the
> device. You appear to be passing back ASCII text. It is supposed to be
> ethtool which does the pretty print, not the kernel driver.
>
>      Andrew

We have other considerations:

If the dump register changes in the future, we hope that
only the kernel needs to be modified, and the ethtool does not need to be modified.
In this case, We do not need to consider the mapping between the ethtool and driver versions.

So in ethtool, we only need to consider basic formatted printing.
like this(not send yet):
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
#define HBG_REG_NAEM_MAX_LEN 32
  
struct hbg_reg_info {
	char name[HBG_REG_NAEM_MAX_LEN];
	u32 offset;
	u32 val;
};
  
static void hibmcge_dump_reg_info(struct hbg_reg_info *info)
{
	fprintf(stdout, "%-*s[0x%04x]: 0x%08x\n",
		HBG_REG_NAEM_MAX_LEN, info->name, info->offset, info->val);
}
  
int hibmcge_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
		      struct ethtool_regs *regs)
{
	struct hbg_reg_info *reg_info;
	u32 name_max_len;
	u32 offset = 0;
  
	if (regs->len % sizeof(*reg_info) != 0)
		return -EINVAL;
  
	while (offset < regs->len) {
		reg_info = (struct hbg_reg_info *)(regs->data + offset);
		hibmcge_dump_reg_info(reg_info);
		offset += sizeof(*reg_info);
	}
  
	return 0;
}
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

So, In this patch, pass back hbg_reg_info(name, offset, value)

Thanks,
Jijie Shao
Jijie Shao Oct. 24, 2024, 4:02 a.m. UTC | #3 
on 2024/10/23 21:42, Jijie Shao wrote:
>   
> +#define HBG_REG_NAEM_MAX_LEN	24
> +#define HBG_REG_TYPE_MAX_LEN	8

......

> +
> +struct hbg_reg_offset_name_map {
> +	u32 reg_offset;
> +	char name[HBG_REG_NAEM_MAX_LEN];
> +};
> +
> +struct hbg_reg_type_info {
> +	char name[HBG_REG_TYPE_MAX_LEN];
> +	u32 offset_base;
> +	const struct hbg_reg_offset_name_map *reg_maps;
> +	u32 reg_num;
> +};
> +
> +struct hbg_reg_info {
> +	char name[HBG_REG_NAEM_MAX_LEN + HBG_REG_TYPE_MAX_LEN];
> +	u32 offset;
> +	u32 val;

......

> +
> +static u32 hbg_get_reg_info(struct hbg_priv *priv,
> +			    const struct hbg_reg_type_info *type_info,
> +			    const struct hbg_reg_offset_name_map *reg_map,
> +			    struct hbg_reg_info *info)
> +{
> +	info->val = hbg_reg_read(priv, reg_map->reg_offset);
> +	info->offset = reg_map->reg_offset - type_info->offset_base;
> +	snprintf(info->name, sizeof(info->name),
> +		 "[%s] %s", type_info->name, reg_map->name);
> +

In addition, there are compilation warning here:
../drivers/net/ethernet/hisilicon/hibmcge/hbg_ethtool.c: In function ‘hbg_ethtool_get_regs’:
../drivers/net/ethernet/hisilicon/hibmcge/hbg_ethtool.c:322:20: warning: ‘%s’ directive output may be truncated writing up to 127 bytes into a region of size 31 [-Wformat-truncation=]
   322 |                  "[%s] %s", type_info->name, reg_map->name);
       |                    ^~
In function ‘hbg_get_reg_info’,
     inlined from ‘hbg_ethtool_get_regs’ at ../drivers/net/ethernet/hisilicon/hibmcge/hbg_ethtool.c:338:14:
../drivers/net/ethernet/hisilicon/hibmcge/hbg_ethtool.c:321:9: note: ‘snprintf’ output between 4 and 154 bytes into a destination of size 32
   321 |         snprintf(info->name, sizeof(info->name),
       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   322 |                  "[%s] %s", type_info->name, reg_map->name);


But in fact, sizeof(info->name) is (24+8), type_info->name length is 8, and reg_map->name length is 24.
I understand that it should be fine to use here.

Thanks,
Jijie Shao
Andrew Lunn Oct. 24, 2024, 12:22 p.m. UTC | #4 
> We have other considerations:
> 
> If the dump register changes in the future, we hope that
> only the kernel needs to be modified, and the ethtool does not need to be modified.
> In this case, We do not need to consider the mapping between the ethtool and driver versions.
> 
> So in ethtool, we only need to consider basic formatted printing.
> like this(not send yet):
> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 
> #define HBG_REG_NAEM_MAX_LEN 32
> struct hbg_reg_info {
> 	char name[HBG_REG_NAEM_MAX_LEN];
> 	u32 offset;
> 	u32 val;
> };
> static void hibmcge_dump_reg_info(struct hbg_reg_info *info)
> {
> 	fprintf(stdout, "%-*s[0x%04x]: 0x%08x\n",
> 		HBG_REG_NAEM_MAX_LEN, info->name, info->offset, info->val);
> }
> int hibmcge_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
> 		      struct ethtool_regs *regs)
> {
> 	struct hbg_reg_info *reg_info;
> 	u32 name_max_len;
> 	u32 offset = 0;
> 	if (regs->len % sizeof(*reg_info) != 0)
> 		return -EINVAL;
> 	while (offset < regs->len) {
> 		reg_info = (struct hbg_reg_info *)(regs->data + offset);
> 		hibmcge_dump_reg_info(reg_info);
> 		offset += sizeof(*reg_info);
> 	}
> 	return 0;
> }
> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 
> 
> So, In this patch, pass back hbg_reg_info(name, offset, value)

So this is different to all other drivers doing registers dumps.

1) Please explain this in the commit message, with a justification why
your driver is different.

2) What is actually specific to your driver here? Why not make this
available to all drivers? Maybe check if ethtool_regs.version ==
MAX_U32 is used by any of the other drivers, and if not, make that a
magic value to indicate your special format.

3) Maybe consider that there does not appear to be a netlink version
of this ethtool ioctl. Could this be nicely integrated into a netlink
version, where you have more flexibility with attributes?


	Andrew
Jijie Shao Oct. 24, 2024, 2:44 p.m. UTC | #5 
on 2024/10/24 20:22, Andrew Lunn wrote:
>> We have other considerations:
>>
>> If the dump register changes in the future, we hope that
>> only the kernel needs to be modified, and the ethtool does not need to be modified.
>> In this case, We do not need to consider the mapping between the ethtool and driver versions.
>>
>> So in ethtool, we only need to consider basic formatted printing.
>> like this(not send yet):
>>
>> #define HBG_REG_NAEM_MAX_LEN 32
>> struct hbg_reg_info {
>> 	char name[HBG_REG_NAEM_MAX_LEN];
>> 	u32 offset;
>> 	u32 val;
>> };
>> static void hibmcge_dump_reg_info(struct hbg_reg_info *info)
>> {
>> 	fprintf(stdout, "%-*s[0x%04x]: 0x%08x\n",
>> 		HBG_REG_NAEM_MAX_LEN, info->name, info->offset, info->val);
>> }
>> int hibmcge_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
>> 		      struct ethtool_regs *regs)
>> {
>> 	struct hbg_reg_info *reg_info;
>> 	u32 name_max_len;
>> 	u32 offset = 0;
>> 	if (regs->len % sizeof(*reg_info) != 0)
>> 		return -EINVAL;
>> 	while (offset < regs->len) {
>> 		reg_info = (struct hbg_reg_info *)(regs->data + offset);
>> 		hibmcge_dump_reg_info(reg_info);
>> 		offset += sizeof(*reg_info);
>> 	}
>> 	return 0;
>> }
>>
>> So, In this patch, pass back hbg_reg_info(name, offset, value)
> So this is different to all other drivers doing registers dumps.
>
> 1) Please explain this in the commit message, with a justification why
> your driver is different.

In fact, we don't have anything different with other drivers.

In the customer environment, the ethtool version may not be the latest.

If the driver adds a register to the register dump, the register is unknown
when the ethtool is used to query the register.
Therefore, we want to separate the ethtool from the driver.
No matter how the driver is modified, the ethtool can display all information perfectly.

>
> 2) What is actually specific to your driver here? Why not make this
> available to all drivers? Maybe check if ethtool_regs.version ==
> MAX_U32 is used by any of the other drivers, and if not, make that a
> magic value to indicate your special format.

In fact, it would be best if ethtool could provide a unified framework to
elegantly display all register information.
The existing framework prints the information in hexadecimal format,
which is not intuitive enough.

>
> 3) Maybe consider that there does not appear to be a netlink version
> of this ethtool ioctl. Could this be nicely integrated into a netlink
> version, where you have more flexibility with attributes?

Okay, I'll analyze it.

Thanks a lot.
Jijie Shao
diff mbox series

Patch

diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_ethtool.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_ethtool.c
index 59f8c84d43fa..a630c7d8ef5c 100644
--- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_ethtool.c
@@ -133,6 +133,126 @@  static const struct hbg_ethtool_stats hbg_ethtool_stats_map[] = {
 	HBG_STATS_I(tx_dma_err_cnt),
 };
 
+#define HBG_REG_NAEM_MAX_LEN	24
+#define HBG_REG_TYPE_MAX_LEN	8
+
+struct hbg_reg_offset_name_map {
+	u32 reg_offset;
+	char name[HBG_REG_NAEM_MAX_LEN];
+};
+
+struct hbg_reg_type_info {
+	char name[HBG_REG_TYPE_MAX_LEN];
+	u32 offset_base;
+	const struct hbg_reg_offset_name_map *reg_maps;
+	u32 reg_num;
+};
+
+struct hbg_reg_info {
+	char name[HBG_REG_NAEM_MAX_LEN + HBG_REG_TYPE_MAX_LEN];
+	u32 offset;
+	u32 val;
+};
+
+const struct hbg_reg_offset_name_map hbg_dev_spec_reg_map[] = {
+	{HBG_REG_SPEC_VALID_ADDR, "VALID"},
+	{HBG_REG_EVENT_REQ_ADDR, "EVENT_REQ"},
+	{HBG_REG_MAC_ID_ADDR, "MAC_ID"},
+	{HBG_REG_PHY_ID_ADDR, "PHY_ADDR"},
+	{HBG_REG_MAC_ADDR_ADDR, "MAC_ADDR_L"},
+	{HBG_REG_MAC_ADDR_HIGH_ADDR, "MAC_ADDR_H"},
+	{HBG_REG_UC_MAC_NUM_ADDR, "UC_MAX_NUM"},
+	{HBG_REG_MAX_MTU_ADDR, "MAX_MTU"},
+	{HBG_REG_MIN_MTU_ADDR, "MIN_MTU"},
+	{HBG_REG_TX_FIFO_NUM_ADDR, "TX_FIFO_NUM"},
+	{HBG_REG_RX_FIFO_NUM_ADDR, "RX_FIFO_NUM"},
+	{HBG_REG_VLAN_LAYERS_ADDR, "VLAN_LAYERS"},
+};
+
+const struct hbg_reg_offset_name_map hbg_mdio_reg_map[] = {
+	{HBG_REG_MDIO_COMMAND_ADDR, "COMMAND_REG"},
+	{HBG_REG_MDIO_ADDR_ADDR, "ADDR_REG"},
+	{HBG_REG_MDIO_WDATA_ADDR, "WDATA_REG"},
+	{HBG_REG_MDIO_RDATA_ADDR, "RDATA_REG"},
+	{HBG_REG_MDIO_STA_ADDR, "STA_REG"},
+};
+
+const struct hbg_reg_offset_name_map hbg_gmac_reg_map[] = {
+	{HBG_REG_DUPLEX_TYPE_ADDR, "DUPLEX_TYPE"},
+	{HBG_REG_FD_FC_TYPE_ADDR, "FD_FC_TYPE"},
+	{HBG_REG_FC_TX_TIMER_ADDR, "FC_TX_TIMER"},
+	{HBG_REG_FD_FC_ADDR_LOW_ADDR, "FD_FC_ADDR_LOW"},
+	{HBG_REG_FD_FC_ADDR_HIGH_ADDR, "FD_FC_ADDR_HIGH"},
+	{HBG_REG_MAX_FRAME_SIZE_ADDR, "MAX_FRM_SIZE"},
+	{HBG_REG_PORT_MODE_ADDR, "PORT_MODE"},
+	{HBG_REG_PORT_ENABLE_ADDR, "PORT_EN"},
+	{HBG_REG_PAUSE_ENABLE_ADDR, "PAUSE_EN"},
+	{HBG_REG_AN_NEG_STATE_ADDR, "AN_NEG_STATE"},
+	{HBG_REG_LINE_LOOP_BACK_ADDR, "LINE_LOOP_BACK"},
+	{HBG_REG_CF_CRC_STRIP_ADDR, "CF_CRC_STRIP"},
+	{HBG_REG_MODE_CHANGE_EN_ADDR, "MODE_CHANGE_EN"},
+	{HBG_REG_LOOP_REG_ADDR, "LOOP_REG"},
+	{HBG_REG_RECV_CTRL_ADDR, "RECV_CONTROL"},
+	{HBG_REG_VLAN_CODE_ADDR, "VLAN_CODE"},
+	{HBG_REG_STATION_ADDR_LOW_0_ADDR, "STATION_ADDR_LOW_0"},
+	{HBG_REG_STATION_ADDR_HIGH_0_ADDR, "STATION_ADDR_HIGH_0"},
+	{HBG_REG_STATION_ADDR_LOW_1_ADDR, "STATION_ADDR_LOW_1"},
+	{HBG_REG_STATION_ADDR_HIGH_1_ADDR, "STATION_ADDR_HIGH_1"},
+	{HBG_REG_STATION_ADDR_LOW_2_ADDR, "STATION_ADDR_LOW_2"},
+	{HBG_REG_STATION_ADDR_HIGH_2_ADDR, "STATION_ADDR_HIGH_2"},
+	{HBG_REG_STATION_ADDR_LOW_3_ADDR, "STATION_ADDR_LOW_3"},
+	{HBG_REG_STATION_ADDR_HIGH_3_ADDR, "STATION_ADDR_HIGH_3"},
+	{HBG_REG_STATION_ADDR_LOW_4_ADDR, "STATION_ADDR_LOW_4"},
+	{HBG_REG_STATION_ADDR_HIGH_4_ADDR, "STATION_ADDR_HIGH_4"},
+	{HBG_REG_STATION_ADDR_LOW_5_ADDR, "STATION_ADDR_LOW_5"},
+	{HBG_REG_STATION_ADDR_HIGH_5_ADDR, "STATION_ADDR_HIGH_5"},
+};
+
+const struct hbg_reg_offset_name_map hbg_pcu_reg_map[] = {
+	{HBG_REG_TX_FIFO_THRSLD_ADDR, "CF_TX_FIFO_THRSLD"},
+	{HBG_REG_RX_FIFO_THRSLD_ADDR, "CF_RX_FIFO_THRSLD"},
+	{HBG_REG_CFG_FIFO_THRSLD_ADDR, "CF_CFG_FIFO_THRSLD"},
+	{HBG_REG_CF_INTRPT_MSK_ADDR, "CF_INTRPT_MSK"},
+	{HBG_REG_CF_INTRPT_STAT_ADDR, "CF_INTRPT_STAT"},
+	{HBG_REG_CF_INTRPT_CLR_ADDR, "CF_INTRPT_CLR"},
+	{HBG_REG_TX_BUS_ERR_ADDR_ADDR, "TX_BUS_ERR_ADDR"},
+	{HBG_REG_RX_BUS_ERR_ADDR_ADDR, "RX_BUS_ERR_ADDR"},
+	{HBG_REG_MAX_FRAME_LEN_ADDR, "MAX_FRAME_LEN"},
+	{HBG_REG_DEBUG_ST_MCH_ADDR, "DEBUG_ST_MCH"},
+	{HBG_REG_FIFO_CURR_STATUS_ADDR, "FIFO_CURR_STATUS"},
+	{HBG_REG_FIFO_HIST_STATUS_ADDR, "FIFO_HIS_STATUS"},
+	{HBG_REG_CF_CFF_DATA_NUM_ADDR, "CF_CFF_DATA_NUM"},
+	{HBG_REG_CF_TX_PAUSE_ADDR, "CF_TX_PAUSE"},
+	{HBG_REG_TX_CFF_ADDR_0_ADDR, "TX_CFF_ADDR_0"},
+	{HBG_REG_TX_CFF_ADDR_1_ADDR, "TX_CFF_ADDR_1"},
+	{HBG_REG_TX_CFF_ADDR_2_ADDR, "TX_CFF_ADDR_2"},
+	{HBG_REG_TX_CFF_ADDR_3_ADDR, "TX_CFF_ADDR_3"},
+	{HBG_REG_RX_CFF_ADDR_ADDR, "RX_CFF_ADDR"},
+	{HBG_REG_RX_BUF_SIZE_ADDR, "RX_BUF_SIZE"},
+	{HBG_REG_BUS_CTRL_ADDR, "BUS_CTRL"},
+	{HBG_REG_RX_CTRL_ADDR, "RX_CTRL"},
+	{HBG_REG_RX_PKT_MODE_ADDR, "RX_PKT_MODE"},
+	{HBG_REG_DBG_ST0_ADDR, "DBG_ST0"},
+	{HBG_REG_DBG_ST1_ADDR, "DBG_ST1"},
+	{HBG_REG_DBG_ST2_ADDR, "DBG_ST2"},
+	{HBG_REG_BUS_RST_EN_ADDR, "BUS_RST_EN"},
+	{HBG_REG_CF_IND_TXINT_MSK_ADDR, "CF_IND_TXINT_MSK"},
+	{HBG_REG_CF_IND_TXINT_STAT_ADDR, "CF_IND_TXINT_STAT"},
+	{HBG_REG_CF_IND_TXINT_CLR_ADDR, "CF_IND_TXINT_CLR"},
+	{HBG_REG_CF_IND_RXINT_MSK_ADDR, "CF_IND_RXINT_MSK"},
+	{HBG_REG_CF_IND_RXINT_STAT_ADDR, "CF_IND_RXINT_STAT"},
+	{HBG_REG_CF_IND_RXINT_CLR_ADDR, "CF_IND_RXINT_CLR"},
+};
+
+#define HBG_REG_TYPE_INFO_I(name, base, map) {name, base, map, ARRAY_SIZE(map)}
+
+const struct hbg_reg_type_info hbg_type_infos[] = {
+	HBG_REG_TYPE_INFO_I("SPEC", 0, hbg_dev_spec_reg_map),
+	HBG_REG_TYPE_INFO_I("MDIO", HBG_REG_MDIO_BASE, hbg_mdio_reg_map),
+	HBG_REG_TYPE_INFO_I("GMAC", HBG_REG_SGMII_BASE, hbg_gmac_reg_map),
+	HBG_REG_TYPE_INFO_I("PCU", HBG_REG_SGMII_BASE, hbg_pcu_reg_map),
+};
+
 static int hbg_ethtool_get_sset_count(struct net_device *netdev, int stringset)
 {
 	if (stringset != ETH_SS_STATS)
@@ -180,6 +300,47 @@  static void hbg_ethtool_get_stats(struct net_device *netdev,
 					 hbg_ethtool_stats_map[i].offset);
 }
 
+static int hbg_ethtool_get_regs_len(struct net_device *netdev)
+{
+	u32 len = 0;
+	u32 i;
+
+	for (i = 0; i < ARRAY_SIZE(hbg_type_infos); i++)
+		len += hbg_type_infos[i].reg_num * sizeof(struct hbg_reg_info);
+
+	return len;
+}
+
+static u32 hbg_get_reg_info(struct hbg_priv *priv,
+			    const struct hbg_reg_type_info *type_info,
+			    const struct hbg_reg_offset_name_map *reg_map,
+			    struct hbg_reg_info *info)
+{
+	info->val = hbg_reg_read(priv, reg_map->reg_offset);
+	info->offset = reg_map->reg_offset - type_info->offset_base;
+	snprintf(info->name, sizeof(info->name),
+		 "[%s] %s", type_info->name, reg_map->name);
+
+	return sizeof(*info);
+}
+
+static void hbg_ethtool_get_regs(struct net_device *netdev,
+				 struct ethtool_regs *regs, void *data)
+{
+	struct hbg_priv *priv = netdev_priv(netdev);
+	const struct hbg_reg_type_info *info;
+	u32 i, j, offset = 0;
+
+	regs->version = 0;
+	for (i = 0; i < ARRAY_SIZE(hbg_type_infos); i++) {
+		info = &hbg_type_infos[i];
+		for (j = 0; j < info->reg_num; j++)
+			offset += hbg_get_reg_info(priv, info,
+						   &info->reg_maps[j],
+						   data + offset);
+	}
+}
+
 static const struct ethtool_ops hbg_ethtool_ops = {
 	.get_link		= ethtool_op_get_link,
 	.get_link_ksettings	= phy_ethtool_get_link_ksettings,
@@ -187,6 +348,8 @@  static const struct ethtool_ops hbg_ethtool_ops = {
 	.get_sset_count		= hbg_ethtool_get_sset_count,
 	.get_strings		= hbg_ethtool_get_strings,
 	.get_ethtool_stats	= hbg_ethtool_get_stats,
+	.get_regs_len		= hbg_ethtool_get_regs_len,
+	.get_regs		= hbg_ethtool_get_regs,
 };
 
 void hbg_ethtool_set_ops(struct net_device *netdev)
diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_reg.h b/drivers/net/ethernet/hisilicon/hibmcge/hbg_reg.h
index 59bda7a8ce5f..bbfefe9c1e61 100644
--- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_reg.h
+++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_reg.h
@@ -10,6 +10,7 @@ 
 #define HBG_REG_MAC_ID_ADDR			0x0008
 #define HBG_REG_PHY_ID_ADDR			0x000C
 #define HBG_REG_MAC_ADDR_ADDR			0x0010
+#define HBG_REG_MAC_ADDR_HIGH_ADDR		0x0014
 #define HBG_REG_UC_MAC_NUM_ADDR			0x0018
 #define HBG_REG_MDIO_FREQ_ADDR			0x0024
 #define HBG_REG_MAX_MTU_ADDR			0x0028
@@ -29,6 +30,7 @@ 
 #define HBG_REG_MDIO_COMMAND_OP_M		GENMASK(11, 10)
 #define HBG_REG_MDIO_COMMAND_PRTAD_M		GENMASK(9, 5)
 #define HBG_REG_MDIO_COMMAND_DEVAD_M		GENMASK(4, 0)
+#define HBG_REG_MDIO_ADDR_ADDR			(HBG_REG_MDIO_BASE + 0x0004)
 #define HBG_REG_MDIO_WDATA_ADDR			(HBG_REG_MDIO_BASE + 0x0008)
 #define HBG_REG_MDIO_WDATA_M			GENMASK(15, 0)
 #define HBG_REG_MDIO_RDATA_ADDR			(HBG_REG_MDIO_BASE + 0x000C)
@@ -37,6 +39,10 @@ 
 /* GMAC */
 #define HBG_REG_SGMII_BASE			0x10000
 #define HBG_REG_DUPLEX_TYPE_ADDR		(HBG_REG_SGMII_BASE + 0x0008)
+#define HBG_REG_FD_FC_TYPE_ADDR			(HBG_REG_SGMII_BASE + 0x000C)
+#define HBG_REG_FC_TX_TIMER_ADDR		(HBG_REG_SGMII_BASE + 0x001C)
+#define HBG_REG_FD_FC_ADDR_LOW_ADDR		(HBG_REG_SGMII_BASE + 0x0020)
+#define HBG_REG_FD_FC_ADDR_HIGH_ADDR		(HBG_REG_SGMII_BASE + 0x0024)
 #define HBG_REG_DUPLEX_B			BIT(0)
 #define HBG_REG_MAX_FRAME_SIZE_ADDR		(HBG_REG_SGMII_BASE + 0x003C)
 #define HBG_REG_PORT_MODE_ADDR			(HBG_REG_SGMII_BASE + 0x0040)
@@ -44,6 +50,8 @@ 
 #define HBG_REG_PORT_ENABLE_ADDR		(HBG_REG_SGMII_BASE + 0x0044)
 #define HBG_REG_PORT_ENABLE_RX_B		BIT(1)
 #define HBG_REG_PORT_ENABLE_TX_B		BIT(2)
+#define HBG_REG_PAUSE_ENABLE_ADDR		(HBG_REG_SGMII_BASE + 0x0048)
+#define HBG_REG_AN_NEG_STATE_ADDR		(HBG_REG_SGMII_BASE + 0x0058)
 #define HBG_REG_TRANSMIT_CTRL_ADDR		(HBG_REG_SGMII_BASE + 0x0060)
 #define HBG_REG_TRANSMIT_CTRL_PAD_EN_B		BIT(7)
 #define HBG_REG_TRANSMIT_CTRL_CRC_ADD_B		BIT(6)
@@ -92,19 +100,35 @@ 
 #define HBG_REG_TX_TAGGED_ADDR			(HBG_REG_SGMII_BASE + 0x0154)
 #define HBG_REG_TX_CRC_ERROR_ADDR		(HBG_REG_SGMII_BASE + 0x0158)
 #define HBG_REG_TX_PAUSE_FRAMES_ADDR		(HBG_REG_SGMII_BASE + 0x015C)
+#define HBG_REG_LINE_LOOP_BACK_ADDR		(HBG_REG_SGMII_BASE + 0x01A8)
 #define HBG_REG_CF_CRC_STRIP_ADDR		(HBG_REG_SGMII_BASE + 0x01B0)
 #define HBG_REG_CF_CRC_STRIP_B			BIT(0)
 #define HBG_REG_MODE_CHANGE_EN_ADDR		(HBG_REG_SGMII_BASE + 0x01B4)
 #define HBG_REG_MODE_CHANGE_EN_B		BIT(0)
+#define HBG_REG_LOOP_REG_ADDR			(HBG_REG_SGMII_BASE + 0x01DC)
 #define HBG_REG_RECV_CTRL_ADDR			(HBG_REG_SGMII_BASE + 0x01E0)
+#define HBG_REG_VLAN_CODE_ADDR			(HBG_REG_SGMII_BASE + 0x01E8)
 #define HBG_REG_RECV_CTRL_STRIP_PAD_EN_B	BIT(3)
 #define HBG_REG_RX_OVERRUN_CNT_ADDR		(HBG_REG_SGMII_BASE + 0x01EC)
 #define HBG_REG_RX_LENGTHFIELD_ERR_CNT_ADDR	(HBG_REG_SGMII_BASE + 0x01F4)
 #define HBG_REG_RX_FAIL_COMMA_CNT_ADDR		(HBG_REG_SGMII_BASE + 0x01F8)
+#define HBG_REG_STATION_ADDR_LOW_0_ADDR		(HBG_REG_SGMII_BASE + 0x0200)
+#define HBG_REG_STATION_ADDR_HIGH_0_ADDR	(HBG_REG_SGMII_BASE + 0x0204)
+#define HBG_REG_STATION_ADDR_LOW_1_ADDR		(HBG_REG_SGMII_BASE + 0x0208)
+#define HBG_REG_STATION_ADDR_HIGH_1_ADDR	(HBG_REG_SGMII_BASE + 0x020C)
 #define HBG_REG_STATION_ADDR_LOW_2_ADDR		(HBG_REG_SGMII_BASE + 0x0210)
 #define HBG_REG_STATION_ADDR_HIGH_2_ADDR	(HBG_REG_SGMII_BASE + 0x0214)
+#define HBG_REG_STATION_ADDR_LOW_3_ADDR		(HBG_REG_SGMII_BASE + 0x0218)
+#define HBG_REG_STATION_ADDR_HIGH_3_ADDR	(HBG_REG_SGMII_BASE + 0x021C)
+#define HBG_REG_STATION_ADDR_LOW_4_ADDR		(HBG_REG_SGMII_BASE + 0x0220)
+#define HBG_REG_STATION_ADDR_HIGH_4_ADDR	(HBG_REG_SGMII_BASE + 0x0224)
+#define HBG_REG_STATION_ADDR_LOW_5_ADDR		(HBG_REG_SGMII_BASE + 0x0228)
+#define HBG_REG_STATION_ADDR_HIGH_5_ADDR	(HBG_REG_SGMII_BASE + 0x022C)
 
 /* PCU */
+#define HBG_REG_TX_FIFO_THRSLD_ADDR		(HBG_REG_SGMII_BASE + 0x0420)
+#define HBG_REG_RX_FIFO_THRSLD_ADDR		(HBG_REG_SGMII_BASE + 0x0424)
+#define HBG_REG_CFG_FIFO_THRSLD_ADDR		(HBG_REG_SGMII_BASE + 0x0428)
 #define HBG_REG_CF_INTRPT_MSK_ADDR		(HBG_REG_SGMII_BASE + 0x042C)
 #define HBG_INT_MSK_WE_ERR_B			BIT(31)
 #define HBG_INT_MSK_RBREQ_ERR_B			BIT(30)
@@ -126,10 +150,15 @@ 
 #define HBG_INT_MSK_RX_B			BIT(0) /* just used in driver */
 #define HBG_REG_CF_INTRPT_STAT_ADDR		(HBG_REG_SGMII_BASE + 0x0434)
 #define HBG_REG_CF_INTRPT_CLR_ADDR		(HBG_REG_SGMII_BASE + 0x0438)
+#define HBG_REG_TX_BUS_ERR_ADDR_ADDR		(HBG_REG_SGMII_BASE + 0x043C)
+#define HBG_REG_RX_BUS_ERR_ADDR_ADDR		(HBG_REG_SGMII_BASE + 0x0440)
 #define HBG_REG_MAX_FRAME_LEN_ADDR		(HBG_REG_SGMII_BASE + 0x0444)
 #define HBG_REG_TX_DROP_CNT_ADDR		(HBG_REG_SGMII_BASE + 0x0448)
 #define HBG_REG_RX_OVER_FLOW_CNT_ADDR		(HBG_REG_SGMII_BASE + 0x044C)
 #define HBG_REG_MAX_FRAME_LEN_M			GENMASK(15, 0)
+#define HBG_REG_DEBUG_ST_MCH_ADDR		(HBG_REG_SGMII_BASE + 0x0450)
+#define HBG_REG_FIFO_CURR_STATUS_ADDR		(HBG_REG_SGMII_BASE + 0x0454)
+#define HBG_REG_FIFO_HIST_STATUS_ADDR		(HBG_REG_SGMII_BASE + 0x0458)
 #define HBG_REG_CF_CFF_DATA_NUM_ADDR		(HBG_REG_SGMII_BASE + 0x045C)
 #define HBG_REG_CF_CFF_DATA_NUM_ADDR_TX_M	GENMASK(8, 0)
 #define HBG_REG_CF_CFF_DATA_NUM_ADDR_RX_M	GENMASK(24, 16)
@@ -137,6 +166,7 @@ 
 #define HBG_REG_RX_TRANS_PKG_CNT_ADDR		(HBG_REG_SGMII_BASE + 0x0464)
 #define HBG_REG_TX_TRANS_PKG_CNT_ADDR		(HBG_REG_SGMII_BASE + 0x0468)
 #define HBG_REG_RX_ADDR_OVERFLOW_ADDR		(HBG_REG_SGMII_BASE + 0x046C)
+#define HBG_REG_CF_TX_PAUSE_ADDR		(HBG_REG_SGMII_BASE + 0x0470)
 #define HBG_REG_TX_CFF_ADDR_0_ADDR		(HBG_REG_SGMII_BASE + 0x0488)
 #define HBG_REG_TX_CFF_ADDR_1_ADDR		(HBG_REG_SGMII_BASE + 0x048C)
 #define HBG_REG_TX_CFF_ADDR_2_ADDR		(HBG_REG_SGMII_BASE + 0x0490)
@@ -158,6 +188,10 @@ 
 #define HBG_REG_RX_BUFRQ_ERR_CNT_ADDR		(HBG_REG_SGMII_BASE + 0x058C)
 #define HBG_REG_TX_BUFRL_ERR_CNT_ADDR		(HBG_REG_SGMII_BASE + 0x0590)
 #define HBG_REG_RX_WE_ERR_CNT_ADDR		(HBG_REG_SGMII_BASE + 0x0594)
+#define HBG_REG_DBG_ST0_ADDR			(HBG_REG_SGMII_BASE + 0x05E4)
+#define HBG_REG_DBG_ST1_ADDR			(HBG_REG_SGMII_BASE + 0x05E8)
+#define HBG_REG_DBG_ST2_ADDR			(HBG_REG_SGMII_BASE + 0x05EC)
+#define HBG_REG_BUS_RST_EN_ADDR			(HBG_REG_SGMII_BASE + 0x0688)
 #define HBG_REG_CF_IND_TXINT_MSK_ADDR		(HBG_REG_SGMII_BASE + 0x0694)
 #define HBG_REG_IND_INTR_MASK_B			BIT(0)
 #define HBG_REG_CF_IND_TXINT_STAT_ADDR		(HBG_REG_SGMII_BASE + 0x0698)