Re: [PATCH net-next v3 1/2] net/ethernet/realtek: Add Realtek automotive PCIe driver code
From: Andrew Lunn
Date: Tue Aug 15 2023 - 11:35:04 EST
On Tue, Aug 15, 2023 at 10:37:55PM +0800, Justin Lai wrote:
> This patch is to add the ethernet device driver for the PCIe interface of Realtek Automotive Ethernet Switch,
> applicable to RTL9054, RTL9068, RTL9072, RTL9075, RTL9068, RTL9071.
>
> Below is a simplified block diagram of the chip and its relevant interfaces.
>
> *************************
> * *
> * CPU network device *
> * ____________ *
> * | | *
> * | PCIE Host | *
> *************************
> ||
> PCIE
> ||
> ****************************************
> * | PCIE Endpoint | *
> * |---------------| *
> * | GMAC | *
> * |------| Realtek *
> * || RTL90xx Series *
> * || *
> * _____________||______________ *
> * | |MAC| | *
> * | |---| | *
> * | | *
> * | Ethernet Switch Core | *
> * | | *
> * | ----- ----- | *
> * | |MAC| ............|MAC| | *
> * |__|___|_____________|___|____| *
> * |PHY| ............|PHY| *
> * ----- ----- *
> *********||****************||***********
> This driver is mainly used to control GMAC, but does not control the switch core, so it is not the same as DSA.
> In addition, the GMAC is not directly connected to the PHY, but directly connected to the mac of the switch core,
> so there is no need for PHY handing.
So this describes your board? Is the MAC and the swtich two separate
chips? Is it however possible to connect the GMAC to a PHY? Could some
OEM purchase the chipset and build a board with a PHY? We write MAC
drivers around what the MAC can do, not what one specific board
allows.
What MAC drivers do to support being connected to a switch like this
is use a fixed-link PHY, via phylink. This gives a virtual PHY, which
supports one speed. The MAC driver then just uses the phylink API as
normal.
On your board, how is the switch controlled? Is there an MDIO bus as
part of the MAC? If so, you should add an MDIO bus master driver.
> +/******************************************************************************
> + * This product is covered by one or more of the following patents:
> + * US6,570,884, US6,115,776, and US6,327,625.
> + ******************************************************************************/
How many other drivers mentions patents? I'm not a lawyer, but doesn't
the GPL say something about this?
> +/* 0x05F3 = 1522bye + 1 */
> +#define RX_BUF_SIZE 0x05F3u
Why not just use:
#define RX_BUF_SIZE 1522 + 1
You don't need the comment then. Better still if you can express this
in terms of ETH_FRAME_LEN, ETH_FCS_LEN etc.
> +/* write/read MMIO register */
> +#define RTL_W8(reg, val8) writeb((val8), ioaddr + (reg))
> +#define RTL_W16(reg, val16) writew((val16), ioaddr + (reg))
> +#define RTL_W32(reg, val32) writel((val32), ioaddr + (reg))
macros should only access what is passed to them. ioaddr is not
passed...
Using small functions would be better, you can then do type checking,
ensure that val8 is a u8, val16 is a u16 etc.
> +/*****************************************************************************/
> +#define RTL_NETIF_RX_SCHEDULE_PREP(dev, napi) napi_schedule_prep(napi)
> +#define __RTL_NETIF_RX_SCHEDULE(dev, napi) __napi_schedule(napi)
No wrappers. Also, it seems odd you are using a __foo
function. Anything with a __ prefix is mean to be hidden.
> +#include <linux/module.h>
> +#include <linux/version.h>
> +#include <linux/cdev.h>
> +#include <linux/pci.h>
> +#include <linux/netdevice.h>
> +#include <linux/etherdevice.h>
> +#include <linux/delay.h>
> +#include <linux/if_vlan.h>
> +#include <linux/crc32.h>
> +#include <linux/interrupt.h>
> +#include <linux/in.h>
> +#include <linux/ip.h>
> +#include <linux/ipv6.h>
> +#include <linux/tcp.h>
> +#include <linux/init.h>
> +#include <linux/rtnetlink.h>
> +#include <linux/prefetch.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/moduleparam.h>
> +#include <linux/mdio.h>
> +#include <net/ip6_checksum.h>
> +#include <net/pkt_cls.h>
> +#include <linux/io.h>
> +#include <asm/irq.h>
> +
> +#include "rtase.h"
> +
> +/* Maximum number of multicast addresses to filter (vs. Rx-all-multicast).
> + * The RTL chips use a 64 element hash table based on the Ethernet CRC.
> + */
> +#define _R(NAME, MAC, JUM_FRAME_SZ) \
> + { \
> + .name = NAME, .mcfg = MAC, .jumbo_frame_sz = JUM_FRAME_SZ \
> + }
> +
> +static const struct {
> + const char *name;
> + u8 mcfg;
> + u16 jumbo_frame_sz;
> +} rtl_chip_info[] = {_R("RTL9072", CFG_METHOD_1, JUMBO_FRAME_9K),
> +
> + _R("Unknown", CFG_METHOD_DEFAULT, JUMBO_FRAME_1K)};
> +#undef _R
The R macro just seems like obfuscation.
> +/******************************************************************************
> + * Module Parameters
> + ******************************************************************************/
> +static unsigned int txq_ctrl = 1;
> +static unsigned int func_txq_num = 1;
> +static unsigned int func_rxq_num = 1;
> +static unsigned int interrupt_num = 1;
> +static int rx_copybreak;
> +
> +module_param(txq_ctrl, uint, 0);
> +MODULE_PARM_DESC(txq_ctrl, "The maximum number of TX queues for PF.");
> +
> +module_param(func_txq_num, uint, 0);
> +MODULE_PARM_DESC(func_txq_num, "TX queue number for this function.");
> +
> +module_param(func_rxq_num, uint, 0);
> +MODULE_PARM_DESC(func_rxq_num, "RX queue number for this function.");
> +
> +module_param(interrupt_num, uint, 0);
> +MODULE_PARM_DESC(interrupt_num, "Interrupt Vector number for this function.");
> +
> +module_param(rx_copybreak, int, 0);
> +MODULE_PARM_DESC(rx_copybreak, "Copy breakpoint for copy-only-tiny-frames");
No module parameters. Please delete them.
> +static int rtase_open(struct net_device *dev);
> +static netdev_tx_t rtase_start_xmit(struct sk_buff *skb, struct net_device *dev);
> +static void rtase_set_rx_mode(struct net_device *dev);
> +static int rtase_set_mac_address(struct net_device *dev, void *p);
> +static int rtase_change_mtu(struct net_device *dev, int new_mtu);
> +static void rtase_tx_timeout(struct net_device *dev, unsigned int txqueue);
> +static void rtase_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats);
> +static int rtase_vlan_rx_add_vid(struct net_device *dev, __be16 protocol, u16 vid);
> +static int rtase_vlan_rx_kill_vid(struct net_device *dev, __be16 protocol, u16 vid);
> +static int rtase_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data);
> +
> +static irqreturn_t rtase_interrupt(int irq, void *dev_instance);
> +static irqreturn_t rtase_q_interrupt(int irq, void *dev_instance);
> +static void rtase_hw_config(struct net_device *dev);
> +static int rtase_close(struct net_device *dev);
> +static void rtase_down(struct net_device *dev);
> +static s32 rtase_init_ring(const struct net_device *dev);
> +static void rtase_init_netdev_ops(struct net_device *dev);
> +static void rtase_rar_set(const struct rtase_private *tp, const uint8_t *addr);
> +static void rtase_desc_addr_fill(const struct rtase_private *tp);
> +static void rtase_tx_desc_init(struct rtase_private *tp, u16 idx);
> +static void rtase_rx_desc_init(struct rtase_private *tp, u16 idx);
> +static void rtase_set_mar(const struct rtase_private *tp);
> +static s32 tx_handler(struct rtase_ring *ring, s32 budget);
> +static s32 rx_handler(struct rtase_ring *ring, s32 budget);
> +static int rtase_poll(struct napi_struct *napi, int budget);
> +static void rtase_sw_reset(struct net_device *dev);
> +static void rtase_hw_start(const struct net_device *dev);
> +static void rtase_dump_tally_counter(const struct rtase_private *tp, dma_addr_t paddr);
> +static void mac_ocp_write(const struct rtase_private *tp, u16 reg_addr, u16 value);
Remove all these put the code in the right order so that they are not
needed. The only time you need forward definitions is for recursion.
Andrew
---
pw-bot: cr