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

[Jitendra Vegiraju]

Thanks for reviewing the patch.
On Sat, May 11, 2024 at 12:34 PM Russell King (Oracle)
<linux at armlinux.org.uk> wrote:
>
> 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.
>
Yes, we will address it.
> > +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.
>
Noted.
> ...
>
> > +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.
>
Thank you for the pointers or software node support.
Since the driver is initially targetted for X86/_64, we were not sure
how to deal with lack of OF support.
We will try out the software node facility.
> > +     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.
>
This will be tied to the above topic.
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!