[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zj/IPpub11OL3jBo@shell.armlinux.org.uk>
Date: Sat, 11 May 2024 20:34:22 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Jitendra Vegiraju <jitendra.vegiraju@...adcom.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com,
bcm-kernel-feedback-list@...adcom.com, alexandre.torgue@...s.st.com,
joabreu@...opsys.com, mcoquelin.stm32@...il.com,
richardcochran@...il.com, linux-kernel@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2, net-next, 2/2] net: stmmac: PCI driver for BCM8958X
SoC
Hi,
Thanks for the patch,. but there are things that need some improvement.
On Fri, May 10, 2024 at 06:59:24PM -0700, Jitendra Vegiraju wrote:
> +static void dwxgmac_brcm_dma_init_tx_chan(struct stmmac_priv *priv,
> + void __iomem *ioaddr,
> + struct stmmac_dma_cfg *dma_cfg,
> + dma_addr_t phy, u32 chan)
> +{
> + u32 value;
> +
> + value = readl(ioaddr + XGMAC_DMA_CH_TX_CONTROL(chan));
> + value &= ~XGMAC_TxPBL;
> + value &= ~GENMASK(6, 4);
> + writel(value, ioaddr + XGMAC_DMA_CH_TX_CONTROL(chan));
> +
> + writel(upper_32_bits(phy), ioaddr + XGMAC_DMA_CH_TxDESC_HADDR(chan));
> + writel(lower_32_bits(phy), ioaddr + XGMAC_DMA_CH_TxDESC_LADDR(chan));
Please use "dma_addr" not "phy" here. "phy" could mean ethernet phy.
I personally dislike "physical address" for DMA stuff because if
there's an IOMMU or other translation layer present, what you have
here is *not* a physical address.
> +static void dwxgmac_brcm_dma_init_rx_chan(struct stmmac_priv *priv,
> + void __iomem *ioaddr,
> + struct stmmac_dma_cfg *dma_cfg,
> + dma_addr_t phy, u32 chan)
> +{
> + u32 value;
> +
> + value = readl(ioaddr + XGMAC_DMA_CH_RX_CONTROL(chan));
> + value &= ~XGMAC_RxPBL;
> + writel(value, ioaddr + XGMAC_DMA_CH_RX_CONTROL(chan));
> +
> + writel(upper_32_bits(phy), ioaddr + XGMAC_DMA_CH_RxDESC_HADDR(chan));
> + writel(lower_32_bits(phy), ioaddr + XGMAC_DMA_CH_RxDESC_LADDR(chan));
Ditto.
..
> +static void dwxgmac_brcm_fix_speed(void *priv, unsigned int speed,
> + unsigned int mode)
> +{
> +}
If this is empty, do you really need it? The method is optional.
..
> +static int dwxgmac_brcm_pci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> +{
..
> + /* This device interface is directly attached to the switch chip on
> + * the SoC. Since no MDIO is present, register fixed_phy.
> + */
> + brcm_priv->phy_dev =
> + fixed_phy_register(PHY_POLL,
> + &dwxgmac_brcm_fixed_phy_status, NULL);
> + if (IS_ERR(brcm_priv->phy_dev)) {
> + dev_err(&pdev->dev, "%s\tNo PHY/fixed_PHY found\n", __func__);
> + return -ENODEV;
> + }
> + phy_attached_info(brcm_priv->phy_dev);
As pointed out in the other sub-thread, you don't need this. If you need
a fixed-link and you don't have a firmware description of it, you can
provide a swnode based description through plat->port_node that will be
passed to phylink. Through that, you can tell phylink to create a
fixed link.
> + ret = stmmac_dvr_probe(&pdev->dev, plat, &res);
> + if (ret)
> + goto err_disable_msi;
> +
> + /* The stmmac core driver doesn't have the infrastructure to
> + * support fixed-phy mdio bus for non-platform bus drivers.
> + * Until a better solution is implemented, initialize the
> + * following entries after priv structure is populated.
> + */
> + ndev = dev_get_drvdata(&pdev->dev);
> + priv = netdev_priv(ndev);
> + priv->mii = mdio_find_bus("fixed-0");
> +
> + ndev->hw_features &= ~NETIF_F_HW_VLAN_CTAG_RX;
> + priv->hw->hw_vlan_en = false;
Basically... no. Do not do any setup after stmmac_dvr_probe(), because
the network device has already been registered and published to
userspace, and userspace may have already opened the network device.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists