Received-SPF: pass (google.com: domain of linux-kernel+bounces-177926-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240510000331.154486-3-jitendra.vegiraju@broadcom.com>
 <20240511015924.41457-1-jitendra.vegiraju@broadcom.com> <Zj/IPpub11OL3jBo@shell.armlinux.org.uk>
In-Reply-To: <Zj/IPpub11OL3jBo@shell.armlinux.org.uk>
From: Jitendra Vegiraju <jitendra.vegiraju@broadcom.com>
Date: Mon, 13 May 2024 10:38:46 -0700
Message-ID: <CAMdnO-KCC0qXEsE1iDGNZwdd0PAcsRinmxe8_-5Anp=e1c5WFA@mail.gmail.com>
Subject: Re: [PATCH v2, net-next, 2/2] net: stmmac: PCI driver for BCM8958X SoC
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc: netdev@vger.kernel.org, davem@davemloft.net, edumazet@google.com, 
	kuba@kernel.org, pabeni@redhat.com, bcm-kernel-feedback-list@broadcom.com, 
	alexandre.torgue@foss.st.com, joabreu@synopsys.com, mcoquelin.stm32@gmail.com, 
	richardcochran@gmail.com, linux-kernel@vger.kernel.org, 
	linux-stm32@st-md-mailman.stormreply.com, 
	linux-arm-kernel@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Thanks for reviewing the patch.
On Sat, May 11, 2024 at 12:34=E2=80=AFPM Russell King (Oracle)
<linux@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 =3D readl(ioaddr + XGMAC_DMA_CH_TX_CONTROL(chan));
> > +     value &=3D ~XGMAC_TxPBL;
> > +     value &=3D ~GENMASK(6, 4);
> > +     writel(value, ioaddr + XGMAC_DMA_CH_TX_CONTROL(chan));
> > +
> > +     writel(upper_32_bits(phy), ioaddr + XGMAC_DMA_CH_TxDESC_HADDR(cha=
n));
> > +     writel(lower_32_bits(phy), ioaddr + XGMAC_DMA_CH_TxDESC_LADDR(cha=
n));
>
> 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 =3D readl(ioaddr + XGMAC_DMA_CH_RX_CONTROL(chan));
> > +     value &=3D ~XGMAC_RxPBL;
> > +     writel(value, ioaddr + XGMAC_DMA_CH_RX_CONTROL(chan));
> > +
> > +     writel(upper_32_bits(phy), ioaddr + XGMAC_DMA_CH_RxDESC_HADDR(cha=
n));
> > +     writel(lower_32_bits(phy), ioaddr + XGMAC_DMA_CH_RxDESC_LADDR(cha=
n));
>
> 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 =3D
> > +              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", __fun=
c__);
> > +             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 =3D 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 =3D dev_get_drvdata(&pdev->dev);
> > +     priv =3D netdev_priv(ndev);
> > +     priv->mii =3D mdio_find_bus("fixed-0");
> > +
> > +     ndev->hw_features &=3D ~NETIF_F_HW_VLAN_CTAG_RX;
> > +     priv->hw->hw_vlan_en =3D 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!