Re: [PATCH v2, net-next, 2/2] net: stmmac: PCI driver for BCM8958X SoC

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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!