From: "Russell King (Oracle)" <linux@armlinux.org.uk> To: Jitendra Vegiraju <jitendra.vegiraju@broadcom.com> 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 Subject: Re: [PATCH v2, net-next, 2/2] net: stmmac: PCI driver for BCM8958X SoC Date: Sat, 11 May 2024 20:34:22 +0100 [thread overview] Message-ID: <Zj/IPpub11OL3jBo@shell.armlinux.org.uk> (raw) In-Reply-To: <20240511015924.41457-1-jitendra.vegiraju@broadcom.com> 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!
WARNING: multiple messages have this Message-ID (diff)
From: "Russell King (Oracle)" <linux@armlinux.org.uk> To: Jitendra Vegiraju <jitendra.vegiraju@broadcom.com> 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 Subject: Re: [PATCH v2, net-next, 2/2] net: stmmac: PCI driver for BCM8958X SoC Date: Sat, 11 May 2024 20:34:22 +0100 [thread overview] Message-ID: <Zj/IPpub11OL3jBo@shell.armlinux.org.uk> (raw) In-Reply-To: <20240511015924.41457-1-jitendra.vegiraju@broadcom.com> 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! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2024-05-11 19:34 UTC|newest] Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-05-10 0:03 [PATCH, net-next, 0/2] net: stmmac: support for BCM8958x SoC Jitendra Vegiraju 2024-05-10 0:03 ` [PATCH, net-next, 1/2] net: stmmac: Export dma_ops for reuse in glue drivers Jitendra Vegiraju 2024-05-10 0:03 ` [PATCH, net-next, 2/2] net: stmmac: PCI driver for BCM8958X SoC Jitendra Vegiraju 2024-05-11 1:35 ` Jakub Kicinski 2024-05-11 1:59 ` [PATCH v2, " Jitendra Vegiraju 2024-05-11 1:59 ` Jitendra Vegiraju 2024-05-11 2:08 ` Jakub Kicinski 2024-05-11 2:08 ` Jakub Kicinski 2024-05-13 16:47 ` Jitendra Vegiraju 2024-05-13 16:47 ` Jitendra Vegiraju 2024-05-11 16:16 ` Andrew Lunn 2024-05-11 16:16 ` Andrew Lunn 2024-05-11 17:12 ` Russell King (Oracle) 2024-05-11 17:12 ` Russell King (Oracle) 2024-05-11 17:19 ` Andrew Lunn 2024-05-11 17:19 ` Andrew Lunn 2024-05-11 19:35 ` Russell King (Oracle) 2024-05-11 19:35 ` Russell King (Oracle) 2024-05-11 17:50 ` Andrew Lunn 2024-05-11 17:50 ` Andrew Lunn 2024-05-11 19:36 ` Russell King (Oracle) 2024-05-11 19:36 ` Russell King (Oracle) 2024-05-13 17:32 ` Jitendra Vegiraju 2024-05-13 17:32 ` Jitendra Vegiraju 2024-05-13 18:07 ` Andrew Lunn 2024-05-13 18:07 ` Andrew Lunn 2024-05-14 8:19 ` Russell King (Oracle) 2024-05-14 8:19 ` Russell King (Oracle) 2024-05-14 16:18 ` Florian Fainelli 2024-05-14 16:18 ` Florian Fainelli 2024-05-11 19:34 ` Russell King (Oracle) [this message] 2024-05-11 19:34 ` Russell King (Oracle) 2024-05-13 17:38 ` Jitendra Vegiraju 2024-05-13 17:38 ` Jitendra Vegiraju 2024-05-13 17:41 ` Russell King (Oracle) 2024-05-13 17:41 ` Russell King (Oracle) 2024-05-13 19:52 ` Andrew Lunn 2024-05-13 19:52 ` Andrew Lunn 2024-05-12 8:35 ` Simon Horman 2024-05-12 8:35 ` Simon Horman 2024-05-17 23:50 ` [PATCH, " kernel test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=Zj/IPpub11OL3jBo@shell.armlinux.org.uk \ --to=linux@armlinux.org.uk \ --cc=alexandre.torgue@foss.st.com \ --cc=bcm-kernel-feedback-list@broadcom.com \ --cc=davem@davemloft.net \ --cc=edumazet@google.com \ --cc=jitendra.vegiraju@broadcom.com \ --cc=joabreu@synopsys.com \ --cc=kuba@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-stm32@st-md-mailman.stormreply.com \ --cc=mcoquelin.stm32@gmail.com \ --cc=netdev@vger.kernel.org \ --cc=pabeni@redhat.com \ --cc=richardcochran@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.