diff mbox series

[V5,net-next,05/11] net: hibmcge: Implement some .ndo functions

Message ID 20240827131455.2919051-6-shaojijie@huawei.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Add support of HIBMCGE Ethernet 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: 7 this patch: 7
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: yisen.zhuang@huawei.com
netdev/build_clang success Errors and warnings before: 7 this patch: 7
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 success Errors and warnings before: 7 this patch: 7
netdev/checkpatch warning WARNING: line length of 87 exceeds 80 columns
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
netdev/contest success net-next-2024-08-27--21-00 (tests: 714)

Commit Message

Jijie Shao Aug. 27, 2024, 1:14 p.m. UTC
Implement the .ndo_open .ndo_stop .ndo_set_mac_address
and .ndo_change_mtu functions.
And .ndo_validate_addr calls the eth_validate_addr function directly

Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 .../ethernet/hisilicon/hibmcge/hbg_common.h   |  3 +
 .../net/ethernet/hisilicon/hibmcge/hbg_hw.c   | 40 ++++++++
 .../net/ethernet/hisilicon/hibmcge/hbg_hw.h   |  3 +
 .../net/ethernet/hisilicon/hibmcge/hbg_main.c | 99 +++++++++++++++++++
 .../net/ethernet/hisilicon/hibmcge/hbg_reg.h  | 11 ++-
 5 files changed, 155 insertions(+), 1 deletion(-)

Comments

Jakub Kicinski Aug. 29, 2024, 1:39 a.m. UTC | #1
On Tue, 27 Aug 2024 21:14:49 +0800 Jijie Shao wrote:
> +static int hbg_net_open(struct net_device *dev)
> +{
> +	struct hbg_priv *priv = netdev_priv(dev);
> +
> +	if (test_and_set_bit(HBG_NIC_STATE_OPEN, &priv->state))
> +		return 0;
> +
> +	netif_carrier_off(dev);

Why clear the carrier during open? You should probably clear it once on
the probe path and then on stop.

> +	hbg_all_irq_enable(priv, true);
> +	hbg_hw_mac_enable(priv, HBG_STATUS_ENABLE);
> +	netif_start_queue(dev);
> +	hbg_phy_start(priv);
> +
> +	return 0;
> +}
Jijie Shao Aug. 29, 2024, 2:40 a.m. UTC | #2
on 2024/8/29 9:39, Jakub Kicinski wrote:
> On Tue, 27 Aug 2024 21:14:49 +0800 Jijie Shao wrote:
>> +static int hbg_net_open(struct net_device *dev)
>> +{
>> +	struct hbg_priv *priv = netdev_priv(dev);
>> +
>> +	if (test_and_set_bit(HBG_NIC_STATE_OPEN, &priv->state))
>> +		return 0;
>> +
>> +	netif_carrier_off(dev);
> Why clear the carrier during open? You should probably clear it once on
> the probe path and then on stop.

In net_open(), the GMAC is not ready to receive or transmit packets.
Therefore, netif_carrier_off() is called.

Packets can be received or transmitted only after the PHY is linked.
Therefore, netif_carrier_on() should be called in adjust_link.

In net_stop() we also call netif_carrier_off()
	
	Thanks,
	Jijie Shao
Jakub Kicinski Aug. 29, 2024, 2:43 p.m. UTC | #3
On Thu, 29 Aug 2024 10:40:07 +0800 Jijie Shao wrote:
> on 2024/8/29 9:39, Jakub Kicinski wrote:
> > On Tue, 27 Aug 2024 21:14:49 +0800 Jijie Shao wrote:  
> >> +static int hbg_net_open(struct net_device *dev)
> >> +{
> >> +	struct hbg_priv *priv = netdev_priv(dev);
> >> +
> >> +	if (test_and_set_bit(HBG_NIC_STATE_OPEN, &priv->state))
> >> +		return 0;
> >> +
> >> +	netif_carrier_off(dev);  
> > Why clear the carrier during open? You should probably clear it once on
> > the probe path and then on stop.  
> 
> In net_open(), the GMAC is not ready to receive or transmit packets.
> Therefore, netif_carrier_off() is called.
> 
> Packets can be received or transmitted only after the PHY is linked.
> Therefore, netif_carrier_on() should be called in adjust_link.

But why are you calling _off() during .ndo_open() ?
Surely the link is also off before ndo_open is called?

> In net_stop() we also call netif_carrier_off()

Exactly, so it should already be off.
Andrew Lunn Aug. 29, 2024, 2:59 p.m. UTC | #4
On Thu, Aug 29, 2024 at 07:43:39AM -0700, Jakub Kicinski wrote:
> On Thu, 29 Aug 2024 10:40:07 +0800 Jijie Shao wrote:
> > on 2024/8/29 9:39, Jakub Kicinski wrote:
> > > On Tue, 27 Aug 2024 21:14:49 +0800 Jijie Shao wrote:  
> > >> +static int hbg_net_open(struct net_device *dev)
> > >> +{
> > >> +	struct hbg_priv *priv = netdev_priv(dev);
> > >> +
> > >> +	if (test_and_set_bit(HBG_NIC_STATE_OPEN, &priv->state))
> > >> +		return 0;
> > >> +
> > >> +	netif_carrier_off(dev);  
> > > Why clear the carrier during open? You should probably clear it once on
> > > the probe path and then on stop.  
> > 
> > In net_open(), the GMAC is not ready to receive or transmit packets.
> > Therefore, netif_carrier_off() is called.
> > 
> > Packets can be received or transmitted only after the PHY is linked.
> > Therefore, netif_carrier_on() should be called in adjust_link.
> 
> But why are you calling _off() during .ndo_open() ?
> Surely the link is also off before ndo_open is called?

I wounder what driver they copied?

The general trend is .probe() calls netif_carrier_off(). After than,
phylib/phylink is in control of the carrier and the MAC driver does
not touch it. in fact, when using phylink, if you try to change the
carrier, you will get SHOUTED at from Russell.

	Andrew
Jijie Shao Aug. 30, 2024, 2:27 a.m. UTC | #5
on 2024/8/29 22:59, Andrew Lunn wrote:
> On Thu, Aug 29, 2024 at 07:43:39AM -0700, Jakub Kicinski wrote:
>> On Thu, 29 Aug 2024 10:40:07 +0800 Jijie Shao wrote:
>>> on 2024/8/29 9:39, Jakub Kicinski wrote:
>>>> On Tue, 27 Aug 2024 21:14:49 +0800 Jijie Shao wrote:
>>>>> +static int hbg_net_open(struct net_device *dev)
>>>>> +{
>>>>> +	struct hbg_priv *priv = netdev_priv(dev);
>>>>> +
>>>>> +	if (test_and_set_bit(HBG_NIC_STATE_OPEN, &priv->state))
>>>>> +		return 0;
>>>>> +
>>>>> +	netif_carrier_off(dev);
>>>> Why clear the carrier during open? You should probably clear it once on
>>>> the probe path and then on stop.
>>> In net_open(), the GMAC is not ready to receive or transmit packets.
>>> Therefore, netif_carrier_off() is called.
>>>
>>> Packets can be received or transmitted only after the PHY is linked.
>>> Therefore, netif_carrier_on() should be called in adjust_link.
>> But why are you calling _off() during .ndo_open() ?
>> Surely the link is also off before ndo_open is called?
> I wounder what driver they copied?
>
> The general trend is .probe() calls netif_carrier_off(). After than,
> phylib/phylink is in control of the carrier and the MAC driver does
> not touch it. in fact, when using phylink, if you try to change the
> carrier, you will get SHOUTED at from Russell.
>
> 	Andrew

Read the PHY driver code:
netif_carrier_on() or netif_carrier_off()
has been called in phy_link_change() based on the link status.
Therefore, the driver does not need to process it.

static void phy_link_change(struct phy_device *phydev, bool up)
{
	struct net_device *netdev = phydev->attached_dev;

	if (up)
		netif_carrier_on(netdev);
	else
		netif_carrier_off(netdev);
	phydev->adjust_link(netdev);
	if (phydev->mii_ts && phydev->mii_ts->link_state)
		phydev->mii_ts->link_state(phydev->mii_ts, phydev);
}

Thank you. I'll delete it in v6.
	Jijie Shao
diff mbox series

Patch

diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h b/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h
index e94ae2be5c4c..d11ef081f4da 100644
--- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h
+++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h
@@ -17,8 +17,11 @@ 
 
 enum hbg_nic_state {
 	HBG_NIC_STATE_EVENT_HANDLING = 0,
+	HBG_NIC_STATE_OPEN,
 };
 
+#define hbg_nic_is_open(priv) test_bit(HBG_NIC_STATE_OPEN, &(priv)->state)
+
 enum hbg_hw_event_type {
 	HBG_HW_EVENT_NONE = 0,
 	HBG_HW_EVENT_INIT, /* driver is loading */
diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c
index f7a7c8524546..04085a68c317 100644
--- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c
+++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c
@@ -125,6 +125,46 @@  void hbg_hw_irq_enable(struct hbg_priv *priv, u32 mask, bool enable)
 	hbg_reg_write(priv, HBG_REG_CF_INTRPT_MSK_ADDR, value);
 }
 
+void hbg_hw_set_uc_addr(struct hbg_priv *priv, u64 mac_addr)
+{
+	hbg_reg_write64(priv, HBG_REG_STATION_ADDR_LOW_2_ADDR, mac_addr);
+}
+
+static void hbg_hw_set_pcu_max_frame_len(struct hbg_priv *priv,
+					 u16 max_frame_len)
+{
+#define HBG_PCU_FRAME_LEN_PLUS 4
+
+	max_frame_len = max_t(u32, max_frame_len, HBG_DEFAULT_MTU_SIZE);
+
+	/* lower two bits of value must be set to 0. Otherwise, the value is ignored */
+	max_frame_len = round_up(max_frame_len, HBG_PCU_FRAME_LEN_PLUS);
+
+	hbg_reg_write_field(priv, HBG_REG_MAX_FRAME_LEN_ADDR,
+			    HBG_REG_MAX_FRAME_LEN_M, max_frame_len);
+}
+
+static void hbg_hw_set_mac_max_frame_len(struct hbg_priv *priv,
+					 u16 max_frame_size)
+{
+	hbg_reg_write_field(priv, HBG_REG_MAX_FRAME_SIZE_ADDR,
+			    HBG_REG_MAX_FRAME_LEN_M, max_frame_size);
+}
+
+void hbg_hw_set_mtu(struct hbg_priv *priv, u16 mtu)
+{
+	hbg_hw_set_pcu_max_frame_len(priv, mtu);
+	hbg_hw_set_mac_max_frame_len(priv, mtu);
+}
+
+void hbg_hw_mac_enable(struct hbg_priv *priv, u32 enable)
+{
+	hbg_reg_write_field(priv, HBG_REG_PORT_ENABLE_ADDR,
+			    HBG_REG_PORT_ENABLE_TX_B, enable);
+	hbg_reg_write_field(priv, HBG_REG_PORT_ENABLE_ADDR,
+			    HBG_REG_PORT_ENABLE_RX_B, enable);
+}
+
 void hbg_hw_adjust_link(struct hbg_priv *priv, u32 speed, u32 duplex)
 {
 	hbg_reg_write_field(priv, HBG_REG_PORT_MODE_ADDR,
diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.h b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.h
index 09946c3966ff..ed72e1192b79 100644
--- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.h
+++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.h
@@ -47,5 +47,8 @@  u32 hbg_hw_get_irq_status(struct hbg_priv *priv);
 void hbg_hw_irq_clear(struct hbg_priv *priv, u32 mask);
 bool hbg_hw_irq_is_enabled(struct hbg_priv *priv, u32 mask);
 void hbg_hw_irq_enable(struct hbg_priv *priv, u32 mask, bool enable);
+void hbg_hw_set_mtu(struct hbg_priv *priv, u16 mtu);
+void hbg_hw_mac_enable(struct hbg_priv *priv, u32 enable);
+void hbg_hw_set_uc_addr(struct hbg_priv *priv, u64 mac_addr);
 
 #endif
diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c
index fb0e87c8fef7..b7248c830434 100644
--- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c
+++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c
@@ -2,6 +2,7 @@ 
 // Copyright (c) 2024 Hisilicon Limited.
 
 #include <linux/etherdevice.h>
+#include <linux/if_vlan.h>
 #include <linux/netdevice.h>
 #include <linux/pci.h>
 #include "hbg_common.h"
@@ -9,6 +10,99 @@ 
 #include "hbg_irq.h"
 #include "hbg_mdio.h"
 
+static void hbg_all_irq_enable(struct hbg_priv *priv, bool enabled)
+{
+	struct hbg_irq_info *info;
+	u32 i;
+
+	for (i = 0; i < priv->vectors.info_array_len; i++) {
+		info = &priv->vectors.info_array[i];
+		hbg_hw_irq_enable(priv, info->mask, enabled);
+	}
+}
+
+static int hbg_net_open(struct net_device *dev)
+{
+	struct hbg_priv *priv = netdev_priv(dev);
+
+	if (test_and_set_bit(HBG_NIC_STATE_OPEN, &priv->state))
+		return 0;
+
+	netif_carrier_off(dev);
+	hbg_all_irq_enable(priv, true);
+	hbg_hw_mac_enable(priv, HBG_STATUS_ENABLE);
+	netif_start_queue(dev);
+	hbg_phy_start(priv);
+
+	return 0;
+}
+
+static int hbg_net_stop(struct net_device *dev)
+{
+	struct hbg_priv *priv = netdev_priv(dev);
+
+	if (!hbg_nic_is_open(priv))
+		return 0;
+
+	clear_bit(HBG_NIC_STATE_OPEN, &priv->state);
+	netif_carrier_off(dev);
+	hbg_phy_stop(priv);
+	netif_stop_queue(dev);
+	hbg_hw_mac_enable(priv, HBG_STATUS_DISABLE);
+	hbg_all_irq_enable(priv, false);
+
+	return 0;
+}
+
+static int hbg_net_set_mac_address(struct net_device *dev, void *addr)
+{
+	struct hbg_priv *priv = netdev_priv(dev);
+	u8 *mac_addr;
+
+	mac_addr = ((struct sockaddr *)addr)->sa_data;
+	if (!is_valid_ether_addr(mac_addr))
+		return -EADDRNOTAVAIL;
+
+	hbg_hw_set_uc_addr(priv, ether_addr_to_u64(mac_addr));
+	dev_addr_set(dev, mac_addr);
+
+	return 0;
+}
+
+static void hbg_change_mtu(struct hbg_priv *priv, int new_mtu)
+{
+	u32 frame_len;
+
+	frame_len = new_mtu + VLAN_HLEN * priv->dev_specs.vlan_layers +
+		    ETH_HLEN + ETH_FCS_LEN;
+	hbg_hw_set_mtu(priv, frame_len);
+}
+
+static int hbg_net_change_mtu(struct net_device *dev, int new_mtu)
+{
+	struct hbg_priv *priv = netdev_priv(dev);
+	bool is_opened = hbg_nic_is_open(priv);
+
+	hbg_net_stop(dev);
+
+	hbg_change_mtu(priv, new_mtu);
+	WRITE_ONCE(dev->mtu, new_mtu);
+
+	dev_dbg(&priv->pdev->dev,
+		"change mtu from %u to %u\n", dev->mtu, new_mtu);
+	if (is_opened)
+		hbg_net_open(dev);
+	return 0;
+}
+
+static const struct net_device_ops hbg_netdev_ops = {
+	.ndo_open		= hbg_net_open,
+	.ndo_stop		= hbg_net_stop,
+	.ndo_validate_addr	= eth_validate_addr,
+	.ndo_set_mac_address	= hbg_net_set_mac_address,
+	.ndo_change_mtu		= hbg_net_change_mtu,
+};
+
 static int hbg_init(struct hbg_priv *priv)
 {
 	int ret;
@@ -69,6 +163,7 @@  static int hbg_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	priv = netdev_priv(netdev);
 	priv->netdev = netdev;
 	priv->pdev = pdev;
+	netdev->netdev_ops = &hbg_netdev_ops;
 
 	ret = hbg_pci_init(pdev);
 	if (ret)
@@ -78,6 +173,10 @@  static int hbg_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret)
 		return ret;
 
+	netdev->max_mtu = priv->dev_specs.max_mtu;
+	netdev->min_mtu = priv->dev_specs.min_mtu;
+	hbg_change_mtu(priv, HBG_DEFAULT_MTU_SIZE);
+	hbg_net_set_mac_address(priv->netdev, &priv->dev_specs.mac_addr);
 	ret = devm_register_netdev(dev, netdev);
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to register netdev\n");
diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_reg.h b/drivers/net/ethernet/hisilicon/hibmcge/hbg_reg.h
index b0991063ccba..63bb1bead8c0 100644
--- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_reg.h
+++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_reg.h
@@ -37,18 +37,24 @@ 
 #define HBG_REG_SGMII_BASE			0x10000
 #define HBG_REG_DUPLEX_TYPE_ADDR		(HBG_REG_SGMII_BASE + 0x0008)
 #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)
 #define HBG_REG_PORT_MODE_M			GENMASK(3, 0)
+#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_TRANSMIT_CONTROL_ADDR		(HBG_REG_SGMII_BASE + 0x0060)
 #define HBG_REG_TRANSMIT_CONTROL_PAD_EN_B	BIT(7)
 #define HBG_REG_TRANSMIT_CONTROL_CRC_ADD_B	BIT(6)
 #define HBG_REG_TRANSMIT_CONTROL_AN_EN_B	BIT(5)
 #define HBG_REG_CF_CRC_STRIP_ADDR		(HBG_REG_SGMII_BASE + 0x01B0)
-#define HBG_REG_CF_CRC_STRIP_B			BIT(0)
+#define HBG_REG_CF_CRC_STRIP_B			BIT(1)
 #define HBG_REG_MODE_CHANGE_EN_ADDR		(HBG_REG_SGMII_BASE + 0x01B4)
 #define HBG_REG_MODE_CHANGE_EN_B		BIT(0)
 #define HBG_REG_RECV_CONTROL_ADDR		(HBG_REG_SGMII_BASE + 0x01E0)
 #define HBG_REG_RECV_CONTROL_STRIP_PAD_EN_B	BIT(3)
+#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)
 
 /* PCU */
 #define HBG_REG_CF_INTRPT_MSK_ADDR		(HBG_REG_SGMII_BASE + 0x042C)
@@ -72,6 +78,8 @@ 
 #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_MAX_FRAME_LEN_ADDR		(HBG_REG_SGMII_BASE + 0x0444)
+#define HBG_REG_MAX_FRAME_LEN_M			GENMASK(15, 0)
 #define HBG_REG_RX_BUF_SIZE_ADDR		(HBG_REG_SGMII_BASE + 0x04E4)
 #define HBG_REG_RX_BUF_SIZE_M			GENMASK(15, 0)
 #define HBG_REG_BUS_CTRL_ADDR			(HBG_REG_SGMII_BASE + 0x04E8)
@@ -86,6 +94,7 @@ 
 #define HBG_REG_RX_PKT_MODE_ADDR		(HBG_REG_SGMII_BASE + 0x04F4)
 #define HBG_REG_RX_PKT_MODE_PARSE_MODE_M	GENMASK(22, 21)
 #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)
 #define HBG_REG_CF_IND_TXINT_CLR_ADDR		(HBG_REG_SGMII_BASE + 0x069C)
 #define HBG_REG_CF_IND_RXINT_MSK_ADDR		(HBG_REG_SGMII_BASE + 0x06a0)