Hi Serge
Thanks for reviewing the patches.
On Tue, Sep 10, 2024 at 12:52 PM Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
>
> On Tue, Sep 03, 2024 at 10:48:14PM -0700, jitendra.vegiraju@xxxxxxxxxxxx wrote:
> > From: Jitendra Vegiraju <jitendra.vegiraju@xxxxxxxxxxxx>
> >
>
> > +#define READ_POLL_DELAY_US 100
> > +#define READ_POLL_TIMEOUT_US 10000
>
> These macros are unused. Why do you need them here?
>
Thanks, missed the cleaning these up.
> > +#define DWMAC_125MHZ 125000000
> > +#define DWMAC_250MHZ 250000000
>
> Drop these and use the literals directly.
>
Ack
> > +#define BRCM_XGMAC_NUM_VLAN_FILTERS 32
> > +
> > +/* TX and RX Queue counts */
> > +#define BRCM_TX_Q_COUNT 4
> > +#define BRCM_RX_Q_COUNT 4
> > +
>
> > +#define BRCM_XGMAC_DMA_TX_SIZE 1024
> > +#define BRCM_XGMAC_DMA_RX_SIZE 1024
>
> Unused.
>
Ack
> > +static void dwxgmac_brcm_common_default_data(struct plat_stmmacenet_data *plat)
> > +{
> > + int i;
> > +
> > + plat->has_xgmac = 1;
> > + plat->force_sf_dma_mode = 1;
> > + plat->mac_port_sel_speed = SPEED_10000;
>
> > + plat->clk_ptp_rate = DWMAC_125MHZ;
> > + plat->clk_ref_rate = DWMAC_250MHZ;
>
> Just 125000000 and 250000000. There is no need in defining the macro
> with the names matching the numerical literals.
>
Ack
> > +static int dwxgmac_brcm_default_data(struct pci_dev *pdev,
> > + struct plat_stmmacenet_data *plat)
> > +{
> > + /* Set common default data first */
> > + dwxgmac_brcm_common_default_data(plat);
> > +
> > + plat->snps_id = DW25GMAC_CORE_4_00;
> > + plat->snps_dev_id = DW25GMAC_ID;
> > + plat->bus_id = 0;
> > + plat->phy_addr = 0;
>
> > + plat->phy_interface = PHY_INTERFACE_MODE_USXGMII;
>
> Really, USXGMII? Universal Serial XGMII? Synopsys call it just XGMII:
> https://www.synopsys.com/dw/ipdir.php?ds=dwc_25g_ethernet_mac_ip
>
Thanks for pointing out. It was a misunderstanding on our part.
I will change it to XGMII and add corresponding handling for XGMII in
stmmac_mac_link_up().
> > +
>
> > + plat->msi_mac_vec = BRCM_XGMAC_MSI_MAC_VECTOR;
> > + plat->msi_rx_base_vec = BRCM_XGMAC_MSI_RX_VECTOR_START;
> > + plat->msi_tx_base_vec = BRCM_XGMAC_MSI_TX_VECTOR_START;
>
> Please see my next comments about these fields utilization.
>
Ack
> > +
> > + return 0;
> > +}
> > +
> > +static struct dwxgmac_brcm_pci_info dwxgmac_brcm_pci_info = {
> > + .setup = dwxgmac_brcm_default_data,
> > +};
> > +
> > +static void brcm_config_misc_regs(struct pci_dev *pdev,
> > + struct brcm_priv_data *brcm_priv)
> > +{
> > + pci_write_config_dword(pdev, XGMAC_PCIE_CFG_MSIX_ADDR_MATCH_LOW,
> > + XGMAC_PCIE_CFG_MSIX_ADDR_MATCH_LO_VALUE);
> > + pci_write_config_dword(pdev, XGMAC_PCIE_CFG_MSIX_ADDR_MATCH_HIGH,
> > + XGMAC_PCIE_CFG_MSIX_ADDR_MATCH_HI_VALUE);
> > +
> > + misc_iowrite(brcm_priv, XGMAC_PCIE_MISC_MSIX_ADDR_MATCH_LO_OFFSET,
> > + XGMAC_PCIE_MISC_MSIX_ADDR_MATCH_LO_VALUE);
> > + misc_iowrite(brcm_priv, XGMAC_PCIE_MISC_MSIX_ADDR_MATCH_HI_OFFSET,
> > + XGMAC_PCIE_MISC_MSIX_ADDR_MATCH_HI_VALUE);
> > +
> > + /* Enable Switch Link */
> > + misc_iowrite(brcm_priv, XGMAC_PCIE_MISC_MII_CTRL_OFFSET,
> > + XGMAC_PCIE_MISC_MII_CTRL_PAUSE_RX |
> > + XGMAC_PCIE_MISC_MII_CTRL_PAUSE_TX |
> > + XGMAC_PCIE_MISC_MII_CTRL_LINK_UP);
> > +}
> > +
> > +static int brcm_config_multi_msi(struct pci_dev *pdev,
> > + struct plat_stmmacenet_data *plat,
> > + struct stmmac_resources *res)
> > +{
> > + int ret, i;
> > +
>
> > + if (plat->msi_rx_base_vec >= STMMAC_MSI_VEC_MAX ||
> > + plat->msi_tx_base_vec >= STMMAC_MSI_VEC_MAX) {
>
> Please see my next comment about these fields and STMMAC_MSI_VEC_MAX
> utilization.
>
Ack
> > + dev_err(&pdev->dev, "%s: Invalid RX & TX vector defined\n",
> > + __func__);
> > + return -EINVAL;
> > + }
> > +
> > + ret = pci_alloc_irq_vectors(pdev, 2, STMMAC_MSI_VEC_MAX,
> > + PCI_IRQ_MSI | PCI_IRQ_MSIX);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "%s: multi MSI enablement failed\n",
> > + __func__);
> > + return ret;
> > + }
> > +
> > + /* For RX MSI */
> > + for (i = 0; i < plat->rx_queues_to_use; i++)
> > + res->rx_irq[i] = pci_irq_vector(pdev,
> > + plat->msi_rx_base_vec + i * 2);
> > +
> > + /* For TX MSI */
> > + for (i = 0; i < plat->tx_queues_to_use; i++)
> > + res->tx_irq[i] = pci_irq_vector(pdev,
> > + plat->msi_tx_base_vec + i * 2);
> > +
>
> > + if (plat->msi_mac_vec < STMMAC_MSI_VEC_MAX)
> > + res->irq = pci_irq_vector(pdev, plat->msi_mac_vec);
>
> What if msi_mac_vec is greater than STMMAC_MSI_VEC_MAX? Will your
> device work without delivering the MAC IRQs? I doubt so.
>
> In anyway see my next comment.
>
Ack
> > +
> > +static int dwxgmac_brcm_pci_probe(struct pci_dev *pdev,
> > + const struct pci_device_id *id)
>
> > + plat->msi_mac_vec = STMMAC_MSI_VEC_MAX;
> > + plat->msi_wol_vec = STMMAC_MSI_VEC_MAX;
> > + plat->msi_lpi_vec = STMMAC_MSI_VEC_MAX;
> > + plat->msi_sfty_ce_vec = STMMAC_MSI_VEC_MAX;
> > + plat->msi_sfty_ue_vec = STMMAC_MSI_VEC_MAX;
> > + plat->msi_rx_base_vec = STMMAC_MSI_VEC_MAX;
> > + plat->msi_tx_base_vec = STMMAC_MSI_VEC_MAX;
>
> Please don't use these fields and the STMMAC_MSI_VEC_MAX macro. Either
> have the BRCM_XGMAC_MSI* macros utilized directly or define the
> device-specific data in the glue driver (in brcm_priv_data if you
> wish). Really the MSI vectors aren't related to any DW *MAC IP-core
> these are the pure vendor platform-specific settings.
>
> The fields originally have been introduced by the Intel developers,
> who AFAICS just found it easier to extend the generic platform-data
> structure instead of introducing the new Intel MAC-specific data.
>
> I am going to drop these fields in a future cleanup patchset so to
> reduce the plat_stmmacenet_data structure complexity.
>
Thanks for the explanation. Will follow your guidelines in next patch.
> -Serge(y)
>
> > [...]
