diff mbox series

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

Message ID 20240910075942.1270054-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

Commit Message

Jijie Shao Sept. 10, 2024, 7:59 a.m. UTC 
Implement the .ndo_open() .ndo_stop() .ndo_set_mac_address()
.ndo_change_mtu functions() and ndo.get_stats64()
And .ndo_validate_addr calls the eth_validate_addr function directly

Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
ChangeLog:
v8 -> v9:
  - Remove HBG_NIC_STATE_OPEN in ndo.open() and ndo.stop(),
    suggested by Kalesh and Andrew.
  - Use netif_running() instead of hbg_nic_is_open() in ndo.change_mtu(),
    suggested by Kalesh and Andrew
  v8: https://lore.kernel.org/all/20240909023141.3234567-1-shaojijie@huawei.com/
v6 -> v7:
  - Add implement ndo.get_stats64(), suggested by Paolo.
  v6: https://lore.kernel.org/all/20240830121604.2250904-6-shaojijie@huawei.com/
v5 -> v6:
  - Delete netif_carrier_off() in .ndo_open() and .ndo_stop(),
    suggested by Jakub and Andrew.
 v5: https://lore.kernel.org/all/20240827131455.2919051-1-shaojijie@huawei.com/
v3 -> v4:
  - Delete INITED_STATE in priv, suggested by Andrew.
  - Delete unnecessary defensive code in hbg_phy_start()
    and hbg_phy_stop(), suggested by Andrew.
  v3: https://lore.kernel.org/all/20240822093334.1687011-1-shaojijie@huawei.com/
RFC v1 -> RFC v2:
  - Delete validation for mtu in hbg_net_change_mtu(), suggested by Andrew.
  - Delete validation for mac address in hbg_net_set_mac_address(),
    suggested by Andrew.
  - Add a patch to add is_valid_ether_addr check in dev_set_mac_address,
    suggested by Andrew.
  RFC v1: https://lore.kernel.org/all/20240731094245.1967834-1-shaojijie@huawei.com/
---
 .../net/ethernet/hisilicon/hibmcge/hbg_hw.c   | 39 ++++++++
 .../net/ethernet/hisilicon/hibmcge/hbg_hw.h   |  3 +
 .../net/ethernet/hisilicon/hibmcge/hbg_main.c | 97 +++++++++++++++++++
 .../net/ethernet/hisilicon/hibmcge/hbg_reg.h  | 11 ++-
 4 files changed, 149 insertions(+), 1 deletion(-)

Comments

Kalesh Anakkur Purayil Sept. 10, 2024, 8:20 a.m. UTC | #1 
Thank you Jijie for taking care of the comments. One comment in line
though it is not directly related to your changes.

On Tue, Sep 10, 2024 at 1:36 PM Jijie Shao <shaojijie@huawei.com> wrote:
>
> Implement the .ndo_open() .ndo_stop() .ndo_set_mac_address()
> .ndo_change_mtu functions() and ndo.get_stats64()
> And .ndo_validate_addr calls the eth_validate_addr function directly
>
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> ---
> ChangeLog:
> v8 -> v9:
>   - Remove HBG_NIC_STATE_OPEN in ndo.open() and ndo.stop(),
>     suggested by Kalesh and Andrew.
>   - Use netif_running() instead of hbg_nic_is_open() in ndo.change_mtu(),
>     suggested by Kalesh and Andrew
>   v8: https://lore.kernel.org/all/20240909023141.3234567-1-shaojijie@huawei.com/
> v6 -> v7:
>   - Add implement ndo.get_stats64(), suggested by Paolo.
>   v6: https://lore.kernel.org/all/20240830121604.2250904-6-shaojijie@huawei.com/
> v5 -> v6:
>   - Delete netif_carrier_off() in .ndo_open() and .ndo_stop(),
>     suggested by Jakub and Andrew.
>  v5: https://lore.kernel.org/all/20240827131455.2919051-1-shaojijie@huawei.com/
> v3 -> v4:
>   - Delete INITED_STATE in priv, suggested by Andrew.
>   - Delete unnecessary defensive code in hbg_phy_start()
>     and hbg_phy_stop(), suggested by Andrew.
>   v3: https://lore.kernel.org/all/20240822093334.1687011-1-shaojijie@huawei.com/
> RFC v1 -> RFC v2:
>   - Delete validation for mtu in hbg_net_change_mtu(), suggested by Andrew.
>   - Delete validation for mac address in hbg_net_set_mac_address(),
>     suggested by Andrew.
>   - Add a patch to add is_valid_ether_addr check in dev_set_mac_address,
>     suggested by Andrew.
>   RFC v1: https://lore.kernel.org/all/20240731094245.1967834-1-shaojijie@huawei.com/
> ---
>  .../net/ethernet/hisilicon/hibmcge/hbg_hw.c   | 39 ++++++++
>  .../net/ethernet/hisilicon/hibmcge/hbg_hw.h   |  3 +
>  .../net/ethernet/hisilicon/hibmcge/hbg_main.c | 97 +++++++++++++++++++
>  .../net/ethernet/hisilicon/hibmcge/hbg_reg.h  | 11 ++-
>  4 files changed, 149 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c
> index 8e971e9f62a0..97fee714155a 100644
> --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c
> +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c
> @@ -15,6 +15,7 @@
>   * ctrl means packet description, data means skb packet data
>   */
>  #define HBG_ENDIAN_CTRL_LE_DATA_BE     0x0
> +#define HBG_PCU_FRAME_LEN_PLUS 4
>
>  static bool hbg_hw_spec_is_valid(struct hbg_priv *priv)
>  {
> @@ -129,6 +130,44 @@ 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)
> +{
> +       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 4d09bdd41c76..0ce500e907b3 100644
> --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.h
> +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.h
> @@ -49,5 +49,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 29e0513fa836..d882a7822299 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,97 @@
>  #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 *netdev)
> +{
> +       struct hbg_priv *priv = netdev_priv(netdev);
> +
> +       hbg_all_irq_enable(priv, true);
> +       hbg_hw_mac_enable(priv, HBG_STATUS_ENABLE);
> +       netif_start_queue(netdev);
> +       hbg_phy_start(priv);
> +
> +       return 0;
> +}
> +
> +static int hbg_net_stop(struct net_device *netdev)
> +{
> +       struct hbg_priv *priv = netdev_priv(netdev);
> +
> +       hbg_phy_stop(priv);
> +       netif_stop_queue(netdev);
> +       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 *netdev, void *addr)
> +{
> +       struct hbg_priv *priv = netdev_priv(netdev);
> +       u8 *mac_addr;
> +
> +       mac_addr = ((struct sockaddr *)addr)->sa_data;
> +
> +       hbg_hw_set_uc_addr(priv, ether_addr_to_u64(mac_addr));
> +       dev_addr_set(netdev, 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 *netdev, int new_mtu)
> +{
> +       struct hbg_priv *priv = netdev_priv(netdev);
> +       bool is_running = netif_running(netdev);
> +
> +       if (is_running)
> +               hbg_net_stop(netdev);
> +
> +       hbg_change_mtu(priv, new_mtu);
> +       WRITE_ONCE(netdev->mtu, new_mtu);
[Kalesh] IMO the setting of "netdev->mtu" should be moved to the core
layer so that not all drivers have to do this.
__dev_set_mtu() can be modified to incorporate this. Just a thought.
> +
> +       dev_dbg(&priv->pdev->dev,
> +               "change mtu from %u to %u\n", netdev->mtu, new_mtu);
> +       if (is_running)
> +               hbg_net_open(netdev);
> +       return 0;
> +}
> +
> +static void hbg_net_get_stats64(struct net_device *netdev,
> +                               struct rtnl_link_stats64 *stats)
> +{
> +       netdev_stats_to_stats64(stats, &netdev->stats);
> +       dev_fetch_sw_netstats(stats, netdev->tstats);
> +}
> +
> +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,
> +       .ndo_get_stats64        = hbg_net_get_stats64,
> +};
> +
>  static int hbg_init(struct hbg_priv *priv)
>  {
>         int ret;
> @@ -73,6 +165,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;
>
>         netdev->tstats = devm_netdev_alloc_pcpu_stats(&pdev->dev,
>                                                       struct pcpu_sw_netstats);
> @@ -88,6 +181,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)
> --
> 2.33.0
>
Andrew Lunn Sept. 10, 2024, 12:14 p.m. UTC | #2 
> > +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 *netdev, int new_mtu)
> > +{
> > +       struct hbg_priv *priv = netdev_priv(netdev);
> > +       bool is_running = netif_running(netdev);
> > +
> > +       if (is_running)
> > +               hbg_net_stop(netdev);
> > +
> > +       hbg_change_mtu(priv, new_mtu);
> > +       WRITE_ONCE(netdev->mtu, new_mtu);
> [Kalesh] IMO the setting of "netdev->mtu" should be moved to the core
> layer so that not all drivers have to do this.
> __dev_set_mtu() can be modified to incorporate this. Just a thought.

Hi Kalesh

If you look at git history, the core has left the driver to set
dev->mtu since the beginning of the code being in git, and probably
longer. It seems a bit unfair to ask a developer to go modify over 200
drivers. Please feel free to submit 200 patches yourself.

	 Andrew
Kalesh Anakkur Purayil Sept. 10, 2024, 12:20 p.m. UTC | #3 
On Tue, Sep 10, 2024 at 5:44 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > +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 *netdev, int new_mtu)
> > > +{
> > > +       struct hbg_priv *priv = netdev_priv(netdev);
> > > +       bool is_running = netif_running(netdev);
> > > +
> > > +       if (is_running)
> > > +               hbg_net_stop(netdev);
> > > +
> > > +       hbg_change_mtu(priv, new_mtu);
> > > +       WRITE_ONCE(netdev->mtu, new_mtu);
> > [Kalesh] IMO the setting of "netdev->mtu" should be moved to the core
> > layer so that not all drivers have to do this.
> > __dev_set_mtu() can be modified to incorporate this. Just a thought.
>
> Hi Kalesh
>
> If you look at git history, the core has left the driver to set
> dev->mtu since the beginning of the code being in git, and probably
> longer. It seems a bit unfair to ask a developer to go modify over 200
> drivers. Please feel free to submit 200 patches yourself.

Hi Andrew,

I did not ask Jijie to make this change. In fact, I had added my RB
tag and added this as a comment saying this comment has nothing to do
with his changes.
Sorry for the confusion.
>
>          Andrew
Andrew Lunn Sept. 10, 2024, 12:34 p.m. UTC | #4 
On Tue, Sep 10, 2024 at 03:59:36PM +0800, Jijie Shao wrote:
> Implement the .ndo_open() .ndo_stop() .ndo_set_mac_address()
> .ndo_change_mtu functions() and ndo.get_stats64()
> And .ndo_validate_addr calls the eth_validate_addr function directly
> 
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> ---
> ChangeLog:
> v8 -> v9:
>   - Remove HBG_NIC_STATE_OPEN in ndo.open() and ndo.stop(),
>     suggested by Kalesh and Andrew.
>   - Use netif_running() instead of hbg_nic_is_open() in ndo.change_mtu(),
>     suggested by Kalesh and Andrew
>   v8: https://lore.kernel.org/all/20240909023141.3234567-1-shaojijie@huawei.com/
> v6 -> v7:
>   - Add implement ndo.get_stats64(), suggested by Paolo.
>   v6: https://lore.kernel.org/all/20240830121604.2250904-6-shaojijie@huawei.com/
> v5 -> v6:
>   - Delete netif_carrier_off() in .ndo_open() and .ndo_stop(),
>     suggested by Jakub and Andrew.
>  v5: https://lore.kernel.org/all/20240827131455.2919051-1-shaojijie@huawei.com/
> v3 -> v4:
>   - Delete INITED_STATE in priv, suggested by Andrew.
>   - Delete unnecessary defensive code in hbg_phy_start()
>     and hbg_phy_stop(), suggested by Andrew.
>   v3: https://lore.kernel.org/all/20240822093334.1687011-1-shaojijie@huawei.com/
> RFC v1 -> RFC v2:
>   - Delete validation for mtu in hbg_net_change_mtu(), suggested by Andrew.
>   - Delete validation for mac address in hbg_net_set_mac_address(),
>     suggested by Andrew.
>   - Add a patch to add is_valid_ether_addr check in dev_set_mac_address,
>     suggested by Andrew.
>   RFC v1: https://lore.kernel.org/all/20240731094245.1967834-1-shaojijie@huawei.com/
> ---
>  .../net/ethernet/hisilicon/hibmcge/hbg_hw.c   | 39 ++++++++
>  .../net/ethernet/hisilicon/hibmcge/hbg_hw.h   |  3 +
>  .../net/ethernet/hisilicon/hibmcge/hbg_main.c | 97 +++++++++++++++++++
>  .../net/ethernet/hisilicon/hibmcge/hbg_reg.h  | 11 ++-
>  4 files changed, 149 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c
> index 8e971e9f62a0..97fee714155a 100644
> --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c
> +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c
> @@ -15,6 +15,7 @@
>   * ctrl means packet description, data means skb packet data
>   */
>  #define HBG_ENDIAN_CTRL_LE_DATA_BE	0x0
> +#define HBG_PCU_FRAME_LEN_PLUS 4
>  
>  static bool hbg_hw_spec_is_valid(struct hbg_priv *priv)
>  {
> @@ -129,6 +130,44 @@ 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);
> +}
> +
> @@ -88,6 +181,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);

It does not help that you added added HBG_DEFAULT_MTU_SIZE in a
previous patch, but as far as i see, it is just ETH_DATA_LEN.

Please use the standard defines, rather than adding your own. It makes
the code a lot easier to understand, it is not using some special
jumbo size by default, it is just the plain, boring, normal 1500
bytes.

    Andrew

---
pw-bot: cr
Jijie Shao Sept. 10, 2024, 1:58 p.m. UTC | #5 
on 2024/9/10 20:34, Andrew Lunn wrote:
> On Tue, Sep 10, 2024 at 03:59:36PM +0800, Jijie Shao wrote:
>> +
>> @@ -88,6 +181,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);
> It does not help that you added added HBG_DEFAULT_MTU_SIZE in a
> previous patch, but as far as i see, it is just ETH_DATA_LEN.
>
> Please use the standard defines, rather than adding your own. It makes
> the code a lot easier to understand, it is not using some special
> jumbo size by default, it is just the plain, boring, normal 1500
> bytes.
>
>      Andrew
>
The value of HBG_DEFAULT_MTU_SIZE is also 1500.
So，ETH_DATA_LEN can also be used.

Sorry I didn't notice ETH_DATA_LEN  before, I'll use it in V10.

Thanks,
	Jijie Shao
diff mbox series

Patch

diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c
index 8e971e9f62a0..97fee714155a 100644
--- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c
+++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c
@@ -15,6 +15,7 @@ 
  * ctrl means packet description, data means skb packet data
  */
 #define HBG_ENDIAN_CTRL_LE_DATA_BE	0x0
+#define HBG_PCU_FRAME_LEN_PLUS 4
 
 static bool hbg_hw_spec_is_valid(struct hbg_priv *priv)
 {
@@ -129,6 +130,44 @@  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)
+{
+	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 4d09bdd41c76..0ce500e907b3 100644
--- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.h
+++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.h
@@ -49,5 +49,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 29e0513fa836..d882a7822299 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,97 @@ 
 #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 *netdev)
+{
+	struct hbg_priv *priv = netdev_priv(netdev);
+
+	hbg_all_irq_enable(priv, true);
+	hbg_hw_mac_enable(priv, HBG_STATUS_ENABLE);
+	netif_start_queue(netdev);
+	hbg_phy_start(priv);
+
+	return 0;
+}
+
+static int hbg_net_stop(struct net_device *netdev)
+{
+	struct hbg_priv *priv = netdev_priv(netdev);
+
+	hbg_phy_stop(priv);
+	netif_stop_queue(netdev);
+	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 *netdev, void *addr)
+{
+	struct hbg_priv *priv = netdev_priv(netdev);
+	u8 *mac_addr;
+
+	mac_addr = ((struct sockaddr *)addr)->sa_data;
+
+	hbg_hw_set_uc_addr(priv, ether_addr_to_u64(mac_addr));
+	dev_addr_set(netdev, 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 *netdev, int new_mtu)
+{
+	struct hbg_priv *priv = netdev_priv(netdev);
+	bool is_running = netif_running(netdev);
+
+	if (is_running)
+		hbg_net_stop(netdev);
+
+	hbg_change_mtu(priv, new_mtu);
+	WRITE_ONCE(netdev->mtu, new_mtu);
+
+	dev_dbg(&priv->pdev->dev,
+		"change mtu from %u to %u\n", netdev->mtu, new_mtu);
+	if (is_running)
+		hbg_net_open(netdev);
+	return 0;
+}
+
+static void hbg_net_get_stats64(struct net_device *netdev,
+				struct rtnl_link_stats64 *stats)
+{
+	netdev_stats_to_stats64(stats, &netdev->stats);
+	dev_fetch_sw_netstats(stats, netdev->tstats);
+}
+
+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,
+	.ndo_get_stats64	= hbg_net_get_stats64,
+};
+
 static int hbg_init(struct hbg_priv *priv)
 {
 	int ret;
@@ -73,6 +165,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;
 
 	netdev->tstats = devm_netdev_alloc_pcpu_stats(&pdev->dev,
 						      struct pcpu_sw_netstats);
@@ -88,6 +181,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)