lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <zv2yxt4aw2wscvmcaadll5gmiswhlgdiesuny2ge7ufhs3xyjn@d7oxhj63qiey>
Date: Tue, 10 Sep 2024 22:51:59 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: jitendra.vegiraju@...adcom.com
Cc: netdev@...r.kernel.org, alexandre.torgue@...s.st.com, 
	joabreu@...opsys.com, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, mcoquelin.stm32@...il.com, bcm-kernel-feedback-list@...adcom.com, 
	richardcochran@...il.com, ast@...nel.org, daniel@...earbox.net, hawk@...nel.org, 
	john.fastabend@...il.com, rmk+kernel@...linux.org.uk, ahalaney@...hat.com, 
	xiaolei.wang@...driver.com, rohan.g.thomas@...el.com, Jianheng.Zhang@...opsys.com, 
	linux-kernel@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com, 
	linux-arm-kernel@...ts.infradead.org, bpf@...r.kernel.org, andrew@...n.ch, linux@...linux.org.uk, 
	horms@...nel.org, florian.fainelli@...adcom.com
Subject: Re: [PATCH net-next v5 4/5] net: stmmac: Add PCI driver support for
 BCM8958x

On Tue, Sep 03, 2024 at 10:48:14PM -0700, jitendra.vegiraju@...adcom.com wrote:
> From: Jitendra Vegiraju <jitendra.vegiraju@...adcom.com>
> 
> Add PCI ethernet driver support for Broadcom BCM8958x SoC devices used
> in automotive applications.
> 
> This SoC device has PCIe ethernet MAC attached to an integrated ethernet
> switch using XGMII interface. The PCIe ethernet controller is presented to
> the Linux host as PCI network device.
> 
> The following block diagram gives an overview of the application.
>              +=================================+
>              |       Host CPU/Linux            |
>              +=================================+
>                         || PCIe
>                         ||
>         +==========================================+
>         |           +--------------+               |
>         |           | PCIE Endpoint|               |
>         |           | Ethernet     |               |
>         |           | Controller   |               |
>         |           |   DMA        |               |
>         |           +--------------+               |
>         |           |   MAC        |   BCM8958X    |
>         |           +--------------+   SoC         |
>         |               || XGMII                   |
>         |               ||                         |
>         |           +--------------+               |
>         |           | Ethernet     |               |
>         |           | switch       |               |
>         |           +--------------+               |
>         |             || || || ||                  |
>         +==========================================+
>                       || || || || More external interfaces
> 
> The MAC IP block on BCM8958x is based on Synopsis XGMAC 4.00a core. This
> driver uses common dwxgmac2 code where applicable.
> Driver functionality specific to this MAC is implemented in dw25gmac.c.
> The glue driver is responsible for setting up hdma mappings.
> 
> Management of integrated ethernet switch on this SoC is not handled by
> the PCIe interface.
> Since BCM8958x is an early adopter device, override the hardware reported
> synopsis versions with actual DW25MAC versions that support hdma.
> 
> This SoC device has PCIe ethernet MAC directly attached to an integrated
> ethernet switch using XGMII interface. Since device tree support is not
> available on this platform, a software node is created to enable
> fixed-link support using phylink driver.
> 
> Signed-off-by: Jitendra Vegiraju <jitendra.vegiraju@...adcom.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-brcm.c  | 507 ++++++++++++++++++
>  1 file changed, 507 insertions(+)
>  create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-brcm.c
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-brcm.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-brcm.c
> new file mode 100644
> index 000000000000..b5dd97d9b938
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-brcm.c
> @@ -0,0 +1,507 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2024 Broadcom Corporation
> + *
> + * PCI driver for ethernet interface of BCM8958X automotive switch chip.
> + *
> + * High level block diagram of the device.
> + *              +=================================+
> + *              |       Host CPU/Linux            |
> + *              +=================================+
> + *                         || PCIe
> + *                         ||
> + *         +==========================================+
> + *         |           +--------------+               |
> + *         |           | PCIE Endpoint|               |
> + *         |           | Ethernet     |               |
> + *         |           | Controller   |               |
> + *         |           |   DMA        |               |
> + *         |           +--------------+               |
> + *         |           |   MAC        |   BCM8958X    |
> + *         |           +--------------+   SoC         |
> + *         |               || XGMII                   |
> + *         |               ||                         |
> + *         |           +--------------+               |
> + *         |           | Ethernet     |               |
> + *         |           | switch       |               |
> + *         |           +--------------+               |
> + *         |             || || || ||                  |
> + *         +==========================================+
> + *                       || || || || More external interfaces
> + *
> + * This SoC device has PCIe ethernet MAC directly attached to an integrated
> + * ethernet switch using XGMII interface. Since devicetree support is not
> + * available on this platform, a software node is created to enable
> + * fixed-link support using phylink driver.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/dmi.h>
> +#include <linux/pci.h>
> +#include <linux/phy.h>
> +
> +#include "stmmac.h"
> +#include "dwxgmac2.h"
> +#include "dw25gmac.h"
> +
> +#define PCI_DEVICE_ID_BROADCOM_BCM8958X		0xa00d
> +#define BRCM_MAX_MTU				1500

> +#define READ_POLL_DELAY_US			100
> +#define READ_POLL_TIMEOUT_US			10000

These macros are unused. Why do you need them here?

> +#define DWMAC_125MHZ				125000000
> +#define DWMAC_250MHZ				250000000

Drop these and use the literals directly.

> +#define BRCM_XGMAC_NUM_VLAN_FILTERS		32
> +
> +/* TX and RX Queue counts */
> +#define BRCM_TX_Q_COUNT				4
> +#define BRCM_RX_Q_COUNT				4
> +

> +#define BRCM_XGMAC_DMA_TX_SIZE			1024
> +#define BRCM_XGMAC_DMA_RX_SIZE			1024

Unused.

> +#define BRCM_XGMAC_BAR0_MASK			BIT(0)
> +
> +#define BRCM_XGMAC_IOMEM_MISC_REG_OFFSET	0x0
> +#define BRCM_XGMAC_IOMEM_MBOX_REG_OFFSET	0x1000
> +#define BRCM_XGMAC_IOMEM_CFG_REG_OFFSET		0x3000
> +
> +#define XGMAC_MMC_CTRL_RCHM_DISABLE		BIT(31)
> +#define XGMAC_PCIE_CFG_MSIX_ADDR_MATCH_LOW	0x940
> +#define XGMAC_PCIE_CFG_MSIX_ADDR_MATCH_LO_VALUE	0x00000001
> +#define XGMAC_PCIE_CFG_MSIX_ADDR_MATCH_HIGH	0x944
> +#define XGMAC_PCIE_CFG_MSIX_ADDR_MATCH_HI_VALUE	0x88000000
> +
> +#define XGMAC_PCIE_MISC_MII_CTRL_OFFSET			0x4
> +#define XGMAC_PCIE_MISC_MII_CTRL_PAUSE_RX		BIT(0)
> +#define XGMAC_PCIE_MISC_MII_CTRL_PAUSE_TX		BIT(1)
> +#define XGMAC_PCIE_MISC_MII_CTRL_LINK_UP		BIT(2)
> +#define XGMAC_PCIE_MISC_PCIESS_CTRL_OFFSET		0x8
> +#define XGMAC_PCIE_MISC_PCIESS_CTRL_EN_MSI_MSIX		BIT(9)
> +#define XGMAC_PCIE_MISC_MSIX_ADDR_MATCH_LO_OFFSET	0x90
> +#define XGMAC_PCIE_MISC_MSIX_ADDR_MATCH_LO_VALUE	0x00000001
> +#define XGMAC_PCIE_MISC_MSIX_ADDR_MATCH_HI_OFFSET	0x94
> +#define XGMAC_PCIE_MISC_MSIX_ADDR_MATCH_HI_VALUE	0x88000000
> +#define XGMAC_PCIE_MISC_MSIX_VECTOR_MAP_EP2HOST0_OFFSET	0x700
> +#define XGMAC_PCIE_MISC_MSIX_VECTOR_MAP_EP2HOST0_VALUE	1
> +#define XGMAC_PCIE_MISC_MSIX_VECTOR_MAP_EP2HOST1_OFFSET	0x704
> +#define XGMAC_PCIE_MISC_MSIX_VECTOR_MAP_EP2HOST1_VALUE	1
> +#define XGMAC_PCIE_MISC_MSIX_VECTOR_MAP_EP2HOST_DBELL_OFFSET	0x728
> +#define XGMAC_PCIE_MISC_MSIX_VECTOR_MAP_EP2HOST_DBELL_VALUE	1
> +#define XGMAC_PCIE_MISC_MSIX_VECTOR_MAP_SBD_ALL_OFFSET	0x740
> +#define XGMAC_PCIE_MISC_MSIX_VECTOR_MAP_SBD_ALL_VALUE	0
> +
> +#define XGMAC_PCIE_MISC_FUNC_RESOURCES_PF0_OFFSET	0x804
> +
> +/* MSIX Vector map register starting offsets */
> +#define XGMAC_PCIE_MISC_MSIX_VECTOR_MAP_RX0_PF0_OFFSET	0x840
> +#define XGMAC_PCIE_MISC_MSIX_VECTOR_MAP_TX0_PF0_OFFSET	0x890
> +#define BRCM_MAX_DMA_CHANNEL_PAIRS		4
> +
> +#define BRCM_XGMAC_MSI_MAC_VECTOR		0
> +#define BRCM_XGMAC_MSI_RX_VECTOR_START		9
> +#define BRCM_XGMAC_MSI_TX_VECTOR_START		10
> +
> +static char *fixed_link_node_name = "fixed-link";
> +
> +static const struct property_entry fixed_link_properties[] = {
> +	PROPERTY_ENTRY_U32("speed", 10000),
> +	PROPERTY_ENTRY_BOOL("full-duplex"),
> +	PROPERTY_ENTRY_BOOL("pause"),
> +	{ }
> +};
> +
> +struct brcm_priv_data {
> +	void __iomem *mbox_regs;    /* MBOX  Registers*/
> +	void __iomem *misc_regs;    /* MISC  Registers*/
> +	void __iomem *xgmac_regs;   /* XGMAC Registers*/
> +	struct software_node fixed_link_node;
> +};
> +
> +struct dwxgmac_brcm_pci_info {
> +	int (*setup)(struct pci_dev *pdev, struct plat_stmmacenet_data *plat);
> +};
> +
> +static void misc_iowrite(struct brcm_priv_data *brcm_priv,
> +			 u32 reg, u32 val)
> +{
> +	iowrite32(val, brcm_priv->misc_regs + reg);
> +}
> +
> +static void dwxgmac_brcm_common_default_data(struct plat_stmmacenet_data *plat)
> +{
> +	int i;
> +
> +	plat->has_xgmac = 1;
> +	plat->force_sf_dma_mode = 1;
> +	plat->mac_port_sel_speed = SPEED_10000;

> +	plat->clk_ptp_rate = DWMAC_125MHZ;
> +	plat->clk_ref_rate = DWMAC_250MHZ;

Just 125000000 and 250000000. There is no need in defining the macro
with the names matching the numerical literals.

> +	plat->tx_coe = 1;
> +	plat->rx_coe = 1;
> +	plat->max_speed = SPEED_10000;
> +
> +	/* Set default value for multicast hash bins */
> +	plat->multicast_filter_bins = HASH_TABLE_SIZE;
> +
> +	/* Set default value for unicast filter entries */
> +	plat->unicast_filter_entries = 1;
> +
> +	/* Set the maxmtu to device's default */
> +	plat->maxmtu = BRCM_MAX_MTU;
> +
> +	/* Set default number of RX and TX queues to use */
> +	plat->tx_queues_to_use = BRCM_TX_Q_COUNT;
> +	plat->rx_queues_to_use = BRCM_RX_Q_COUNT;
> +
> +	plat->tx_sched_algorithm = MTL_TX_ALGORITHM_SP;
> +	for (i = 0; i < plat->tx_queues_to_use; i++) {
> +		plat->tx_queues_cfg[i].use_prio = false;
> +		plat->tx_queues_cfg[i].prio = 0;
> +		plat->tx_queues_cfg[i].mode_to_use = MTL_QUEUE_AVB;
> +	}
> +
> +	plat->rx_sched_algorithm = MTL_RX_ALGORITHM_SP;
> +	for (i = 0; i < plat->rx_queues_to_use; i++) {
> +		plat->rx_queues_cfg[i].use_prio = false;
> +		plat->rx_queues_cfg[i].mode_to_use = MTL_QUEUE_AVB;
> +		plat->rx_queues_cfg[i].pkt_route = 0x0;
> +		plat->rx_queues_cfg[i].chan = i;
> +	}
> +}
> +
> +static int dwxgmac_brcm_default_data(struct pci_dev *pdev,
> +				     struct plat_stmmacenet_data *plat)
> +{
> +	/* Set common default data first */
> +	dwxgmac_brcm_common_default_data(plat);
> +
> +	plat->snps_id = DW25GMAC_CORE_4_00;
> +	plat->snps_dev_id = DW25GMAC_ID;
> +	plat->bus_id = 0;
> +	plat->phy_addr = 0;

> +	plat->phy_interface = PHY_INTERFACE_MODE_USXGMII;

Really, USXGMII? Universal Serial XGMII? Synopsys call it just XGMII:
https://www.synopsys.com/dw/ipdir.php?ds=dwc_25g_ethernet_mac_ip

> +
> +	plat->dma_cfg->pbl = 32;
> +	plat->dma_cfg->pblx8 = 0;
> +	plat->dma_cfg->aal = 0;
> +	plat->dma_cfg->eame = 1;
> +
> +	plat->axi->axi_wr_osr_lmt = 31;
> +	plat->axi->axi_rd_osr_lmt = 31;
> +	plat->axi->axi_fb = 0;
> +	plat->axi->axi_blen[0] = 4;
> +	plat->axi->axi_blen[1] = 8;
> +	plat->axi->axi_blen[2] = 16;
> +	plat->axi->axi_blen[3] = 32;
> +	plat->axi->axi_blen[4] = 64;
> +	plat->axi->axi_blen[5] = 128;
> +	plat->axi->axi_blen[6] = 256;
> +

> +	plat->msi_mac_vec = BRCM_XGMAC_MSI_MAC_VECTOR;
> +	plat->msi_rx_base_vec = BRCM_XGMAC_MSI_RX_VECTOR_START;
> +	plat->msi_tx_base_vec = BRCM_XGMAC_MSI_TX_VECTOR_START;

Please see my next comments about these fields utilization.

> +
> +	return 0;
> +}
> +
> +static struct dwxgmac_brcm_pci_info dwxgmac_brcm_pci_info = {
> +	.setup = dwxgmac_brcm_default_data,
> +};
> +
> +static void brcm_config_misc_regs(struct pci_dev *pdev,
> +				  struct brcm_priv_data *brcm_priv)
> +{
> +	pci_write_config_dword(pdev, XGMAC_PCIE_CFG_MSIX_ADDR_MATCH_LOW,
> +			       XGMAC_PCIE_CFG_MSIX_ADDR_MATCH_LO_VALUE);
> +	pci_write_config_dword(pdev, XGMAC_PCIE_CFG_MSIX_ADDR_MATCH_HIGH,
> +			       XGMAC_PCIE_CFG_MSIX_ADDR_MATCH_HI_VALUE);
> +
> +	misc_iowrite(brcm_priv, XGMAC_PCIE_MISC_MSIX_ADDR_MATCH_LO_OFFSET,
> +		     XGMAC_PCIE_MISC_MSIX_ADDR_MATCH_LO_VALUE);
> +	misc_iowrite(brcm_priv, XGMAC_PCIE_MISC_MSIX_ADDR_MATCH_HI_OFFSET,
> +		     XGMAC_PCIE_MISC_MSIX_ADDR_MATCH_HI_VALUE);
> +
> +	/* Enable Switch Link */
> +	misc_iowrite(brcm_priv, XGMAC_PCIE_MISC_MII_CTRL_OFFSET,
> +		     XGMAC_PCIE_MISC_MII_CTRL_PAUSE_RX |
> +		     XGMAC_PCIE_MISC_MII_CTRL_PAUSE_TX |
> +		     XGMAC_PCIE_MISC_MII_CTRL_LINK_UP);
> +}
> +
> +static int brcm_config_multi_msi(struct pci_dev *pdev,
> +				 struct plat_stmmacenet_data *plat,
> +				 struct stmmac_resources *res)
> +{
> +	int ret, i;
> +

> +	if (plat->msi_rx_base_vec >= STMMAC_MSI_VEC_MAX ||
> +	    plat->msi_tx_base_vec >= STMMAC_MSI_VEC_MAX) {

Please see my next comment about these fields and STMMAC_MSI_VEC_MAX
utilization.

> +		dev_err(&pdev->dev, "%s: Invalid RX & TX vector defined\n",
> +			__func__);
> +		return -EINVAL;
> +	}
> +
> +	ret = pci_alloc_irq_vectors(pdev, 2, STMMAC_MSI_VEC_MAX,
> +				    PCI_IRQ_MSI | PCI_IRQ_MSIX);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "%s: multi MSI enablement failed\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	/* For RX MSI */
> +	for (i = 0; i < plat->rx_queues_to_use; i++)
> +		res->rx_irq[i] = pci_irq_vector(pdev,
> +						plat->msi_rx_base_vec + i * 2);
> +
> +	/* For TX MSI */
> +	for (i = 0; i < plat->tx_queues_to_use; i++)
> +		res->tx_irq[i] = pci_irq_vector(pdev,
> +						plat->msi_tx_base_vec + i * 2);
> +

> +	if (plat->msi_mac_vec < STMMAC_MSI_VEC_MAX)
> +		res->irq = pci_irq_vector(pdev, plat->msi_mac_vec);

What if msi_mac_vec is greater than STMMAC_MSI_VEC_MAX? Will your
device work without delivering the MAC IRQs? I doubt so.

In anyway see my next comment.

> +
> +	plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
> +	plat->flags |= STMMAC_FLAG_TSO_EN;
> +
> +	return 0;
> +}
> +
> +static int dwxgmac_brcm_pci_probe(struct pci_dev *pdev,
> +				  const struct pci_device_id *id)
> +{
> +	struct dwxgmac_brcm_pci_info *info =
> +		(struct dwxgmac_brcm_pci_info *)id->driver_data;
> +	struct plat_stmmacenet_data *plat;
> +	struct brcm_priv_data *brcm_priv;
> +	struct stmmac_resources res;
> +	struct device *dev;
> +	int rx_offset;
> +	int tx_offset;
> +	int vector;
> +	int ret;
> +
> +	dev = &pdev->dev;
> +
> +	brcm_priv = devm_kzalloc(&pdev->dev, sizeof(*brcm_priv), GFP_KERNEL);
> +	if (!brcm_priv)
> +		return -ENOMEM;
> +
> +	plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
> +	if (!plat)
> +		return -ENOMEM;
> +
> +	plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg),
> +				     GFP_KERNEL);
> +	if (!plat->dma_cfg)
> +		return -ENOMEM;
> +
> +	plat->axi = devm_kzalloc(&pdev->dev, sizeof(*plat->axi), GFP_KERNEL);
> +	if (!plat->axi)
> +		return -ENOMEM;
> +
> +	/* This device is directly attached to the switch chip internal to the
> +	 * SoC using XGMII interface. Since no MDIO is present, register
> +	 * fixed-link software_node to create phylink.
> +	 */
> +	plat->port_node = fwnode_create_software_node(NULL, NULL);
> +	brcm_priv->fixed_link_node.name = fixed_link_node_name;
> +	brcm_priv->fixed_link_node.properties = fixed_link_properties;
> +	brcm_priv->fixed_link_node.parent = to_software_node(plat->port_node);
> +	device_add_software_node(dev, &brcm_priv->fixed_link_node);
> +
> +	/* Disable D3COLD as our device does not support it */
> +	pci_d3cold_disable(pdev);
> +
> +	/* Enable PCI device */
> +	ret = pcim_enable_device(pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: ERROR: failed to enable device\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	/* Get the base address of device */
> +	ret = pcim_iomap_regions(pdev, BRCM_XGMAC_BAR0_MASK, pci_name(pdev));
> +	if (ret)
> +		return ret;
> +	pci_set_master(pdev);
> +
> +	memset(&res, 0, sizeof(res));
> +	res.addr = pcim_iomap_table(pdev)[0];
> +	/* MISC Regs */
> +	brcm_priv->misc_regs = res.addr + BRCM_XGMAC_IOMEM_MISC_REG_OFFSET;
> +	/* MBOX Regs */
> +	brcm_priv->mbox_regs = res.addr + BRCM_XGMAC_IOMEM_MBOX_REG_OFFSET;
> +	/* XGMAC config Regs */
> +	res.addr += BRCM_XGMAC_IOMEM_CFG_REG_OFFSET;
> +	brcm_priv->xgmac_regs = res.addr;
> +
> +	plat->bsp_priv = brcm_priv;
> +
> +	/* Initialize all MSI vectors to invalid so that it can be set
> +	 * according to platform data settings below.
> +	 * Note: MSI vector takes value from 0 up to 31 (STMMAC_MSI_VEC_MAX)
> +	 */

> +	plat->msi_mac_vec = STMMAC_MSI_VEC_MAX;
> +	plat->msi_wol_vec = STMMAC_MSI_VEC_MAX;
> +	plat->msi_lpi_vec = STMMAC_MSI_VEC_MAX;
> +	plat->msi_sfty_ce_vec = STMMAC_MSI_VEC_MAX;
> +	plat->msi_sfty_ue_vec = STMMAC_MSI_VEC_MAX;
> +	plat->msi_rx_base_vec = STMMAC_MSI_VEC_MAX;
> +	plat->msi_tx_base_vec = STMMAC_MSI_VEC_MAX;

Please don't use these fields and the STMMAC_MSI_VEC_MAX macro. Either
have the BRCM_XGMAC_MSI* macros utilized directly or define the
device-specific data in the glue driver (in brcm_priv_data if you
wish). Really the MSI vectors aren't related to any DW *MAC IP-core
these are the pure vendor platform-specific settings.

The fields originally have been introduced by the Intel developers,
who AFAICS just found it easier to extend the generic platform-data
structure instead of introducing the new Intel MAC-specific data.

I am going to drop these fields in a future cleanup patchset so to
reduce the plat_stmmacenet_data structure complexity.

-Serge(y)

> [...]

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ