| Message ID | c97cb15ab77fb9dfdd281640f48dcfc08c6988c0.1714046812.git.siyanteng@loongson.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | stmmac: Add Loongson platform support | expand |
Copy here a comment from v11 that didn't get a clear response. In v11: 在 2024/4/25 21:11, Yanteng Si 写道: > +static int loongson_dwmac_config_msi(struct pci_dev *pdev, > + struct plat_stmmacenet_data *plat, > + struct stmmac_resources *res, > + struct device_node *np) > +{ > + int i, ret, vecs; > + > + vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1); > + ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI); > + if (ret < 0) { > + dev_info(&pdev->dev, > + "MSI enable failed, Fallback to legacy interrupt\n"); > + return loongson_dwmac_config_legacy(pdev, plat, res, np); > + } > + > + res->irq = pci_irq_vector(pdev, 0); > + res->wol_irq = 0; > + > + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > + * --------- ----- -------- -------- ... -------- -------- > + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > + */ > + for (i = 0; i < CHANNEL_NUM; i++) { > + res->rx_irq[CHANNEL_NUM - 1 - i] = > + pci_irq_vector(pdev, 1 + i * 2); > + res->tx_irq[CHANNEL_NUM - 1 - i] = > + pci_irq_vector(pdev, 2 + i * 2); > + } > + > + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > + > + return 0; > +} * First,Serge wrote: Once again. Please replace this with simpler solution: static int loongson_dwmac_config_multi_msi(struct pci_dev *pdev, + struct plat_stmmacenet_data *plat, + struct stmmac_resources *res) +{ + int i, ret, vecs; + + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | + * --------- ----- -------- -------- ... -------- -------- + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | + */ + vecs = plat->rx_queues_to_use + plat->tx_queues_to_use + 1; + ret = pci_alloc_irq_vectors(pdev, 1, vecs, PCI_IRQ_MSI | PCI_IRQ_LEGACY); + if (ret < 0) { + dev_err(&pdev->dev, "Failed to allocate PCI IRQs\n"); + return ret; + } else if (ret >= vecs) { + for (i = 0; i < plat->rx_queues_to_use; i++) { + res->rx_irq[CHANNELS_NUM - 1 - i] = + pci_irq_vector(pdev, 1 + i * 2); + } + for (i = 0; i < plat->tx_queues_to_use; i++) { + res->tx_irq[CHANNELS_NUM - 1 - i] = + pci_irq_vector(pdev, 2 + i * 2); + } + + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; + } + + res->irq = pci_irq_vector(pdev, 0); + + return 0; +} * Then, Yanteng wrote: Well, I'll try again. * And then,Hiacai wrote: In full PCI system the below function works fine, because alloc irq vectors with PCI_IRQ_LEGACY do the same thing as fallback to call loongson_dwmac_config_legacy(). But for a DT-based system it doesn't work. * Last, Yanteng wrote: Hi Serge, How about we stay the same in v12? Thanks, Yanteng
On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote: > There are two types of Loongson DWGMAC. The first type shares the same s/Loongson DWGMAC/Loongson GNET controllers > register definitions and has similar logic as dwmac1000. The second type > uses several different register definitions, we think it is necessary to > distinguish rx and tx, so we split these bits into two. s/rx/Rx s/tx/Tx > > Simply put, we split some single bit fields into double bits fileds: > > Name Tx Rx > > DMA_INTR_ENA_NIE = 0x00040000 | 0x00020000; > DMA_INTR_ENA_AIE = 0x00010000 | 0x00008000; > DMA_STATUS_NIS = 0x00040000 | 0x00020000; > DMA_STATUS_AIS = 0x00010000 | 0x00008000; > DMA_STATUS_FBI = 0x00002000 | 0x00001000; > > Therefore, when using, TX and RX must be set at the same time. > > How to use them: > 1. Create the Loongson GNET-specific > stmmac_dma_ops.dma_interrupt() > stmmac_dma_ops.init_chan() > methods in the dwmac-loongson.c driver. Adding all the > Loongson-specific macros > > 2. Create a Loongson GNET-specific platform setup method with the next > semantics: > + allocate stmmac_dma_ops instance and initialize it with > dwmac1000_dma_ops. > + override the stmmac_dma_ops.{dma_interrupt, init_chan} with > the pointers to the methods defined in 2. > + allocate mac_device_info instance and initialize the > mac_device_info.dma field with a pointer to the new > stmmac_dma_ops instance. > + initialize mac_device_info in a way it's done in > dwmac1000_setup(). > > 3. Initialize plat_stmmacenet_data.setup() with the pointer to the > method created in 2. > > GNET features: > > Speeds: 10/100/1000Mbps > DMA-descriptors type: enhanced > L3/L4 filters availability: support > VLAN hash table filter: support > PHY-interface: GMII > Remote Wake-up support: support > Mac Management Counters (MMC): support > Number of additional MAC addresses: 5 > MAC Hash-based filter: support > Number of ash table size: 256 > DMA chennel number: 0x10 device is 8 and 0x37 device is 1 > > Others: > > GNET integrates both MAC and PHY chips inside. > GNET device: LS2K2000, LS7A2000, the chip connection between the mac and > phy of these devices is not normal and requires two rounds of > negotiation; LS7A2000 does not support half-duplex and > multi-channel; > > To enable multi-channel on LS2K2000, you need to turn off > hardware checksum. > > **Note**: Currently, only the LS2K2000's synopsys_id is 0x10, while the > synopsys_id of other devices are 0x37. The entire commit log looks as a set of information and doesn't explicitly explain what is going on in the patch body. Let's make it a bit more coherent: "Aside with the Loongson GMAC controllers which can be normally found on the LS2K1000 SoC and LS7A1000 chipset, Loongson released a new version of the network controllers called Loongson GNET. It has been synthesized into the new generation LS2K2000 SoC and LS7A2000 chipset with the next DW GMAC features enabled: DW GMAC IP-core: v3.73a Speeds: 10/100/1000Mbps Duplex: Full (both versions), Half (LS2K2000 SoC only) DMA-descriptors type: enhanced L3/L4 filters availability: Y VLAN hash table filter: Y PHY-interface: GMII (PHY is integrated into the chips) Remote Wake-up support: Y Mac Management Counters (MMC): Y Number of additional MAC addresses: 5 MAC Hash-based filter: Y Hash Table Size: 256 AV feature: Y (LS2K2000 SoC only) DMA channels: 8 (LS2K2000 SoC), 1 (LS7A2000 chipset) The integrated PHY has a weird problem with switching from the low speeds to 1000Mbps mode. The speedup procedure requires the PHY-link re-negotiation. Besides the LS2K2000 GNET controller the next peculiarities: 1. Split up Tx and Rx DMA IRQ status/mask bits: Name Tx Rx DMA_INTR_ENA_NIE = 0x00040000 | 0x00020000; DMA_INTR_ENA_AIE = 0x00010000 | 0x00008000; DMA_STATUS_NIS = 0x00040000 | 0x00020000; DMA_STATUS_AIS = 0x00010000 | 0x00008000; DMA_STATUS_FBI = 0x00002000 | 0x00001000; 2. Custom Synopsys ID hardwired into the GMAC_VERSION.SNPSVER field. It's 0x10 while it should have been 0x37 in accordance with the actual DW GMAC IP-core version. Thus in order to have the Loongson GNET controllers supported let's modify the Loongson DWMAC driver in accordance with all the peculiarities described above: 1. Create the Loongson GNET-specific stmmac_dma_ops::dma_interrupt() stmmac_dma_ops::init_chan() callbacks due to the non-standard DMA IRQ CSR flags layout. 2. Create the Loongson GNET-specific platform setup() method which gets to initialize the DMA-ops with the dwmac1000_dma_ops instance and overrides the callbacks described in 1, and overrides the custom Synopsys ID with the real one in order to have the rest of the HW-specific callbacks correctly detected by the driver core. 3. Make sure the Loongson GNET-specific platform setup() method enables the duplex modes supported by the controller. 4. Provide the plat_stmmacenet_data::fix_mac_speed() callback which will restart the link Auto-negotiation in case of the speed change." See, you don't need to mention the 0x10 ID all the time. Just once and in the place where it's actually relevant. > > Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> > Signed-off-by: Yanteng Si <siyanteng@loongson.cn> > --- > drivers/net/ethernet/stmicro/stmmac/common.h | 1 + > .../ethernet/stmicro/stmmac/dwmac-loongson.c | 381 +++++++++++++++++- > 2 files changed, 371 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h > index 9cd62b2110a1..aed6ae80cc7c 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/common.h > +++ b/drivers/net/ethernet/stmicro/stmmac/common.h > @@ -29,6 +29,7 @@ > /* Synopsys Core versions */ > #define DWMAC_CORE_3_40 0x34 > #define DWMAC_CORE_3_50 0x35 > +#define DWMAC_CORE_3_70 0x37 > #define DWMAC_CORE_4_00 0x40 > #define DWMAC_CORE_4_10 0x41 > #define DWMAC_CORE_5_00 0x50 > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > index a16bba389417..68de90c44feb 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > @@ -8,9 +8,71 @@ > #include <linux/device.h> > #include <linux/of_irq.h> > #include "stmmac.h" > +#include "dwmac_dma.h" > +#include "dwmac1000.h" > + > +/* Normal Loongson Tx Summary */ > +#define DMA_INTR_ENA_NIE_TX_LOONGSON 0x00040000 > +/* Normal Loongson Rx Summary */ > +#define DMA_INTR_ENA_NIE_RX_LOONGSON 0x00020000 > + > +#define DMA_INTR_NORMAL_LOONGSON (DMA_INTR_ENA_NIE_TX_LOONGSON | \ > + DMA_INTR_ENA_NIE_RX_LOONGSON | \ > + DMA_INTR_ENA_RIE | DMA_INTR_ENA_TIE) > + > +/* Abnormal Loongson Tx Summary */ > +#define DMA_INTR_ENA_AIE_TX_LOONGSON 0x00010000 > +/* Abnormal Loongson Rx Summary */ > +#define DMA_INTR_ENA_AIE_RX_LOONGSON 0x00008000 > + > +#define DMA_INTR_ABNORMAL_LOONGSON (DMA_INTR_ENA_AIE_TX_LOONGSON | \ > + DMA_INTR_ENA_AIE_RX_LOONGSON | \ > + DMA_INTR_ENA_FBE | DMA_INTR_ENA_UNE) > + > +#define DMA_INTR_DEFAULT_MASK_LOONGSON (DMA_INTR_NORMAL_LOONGSON | \ > + DMA_INTR_ABNORMAL_LOONGSON) > + > +/* Normal Loongson Tx Interrupt Summary */ > +#define DMA_STATUS_NIS_TX_LOONGSON 0x00040000 > +/* Normal Loongson Rx Interrupt Summary */ > +#define DMA_STATUS_NIS_RX_LOONGSON 0x00020000 > + > +/* Abnormal Loongson Tx Interrupt Summary */ > +#define DMA_STATUS_AIS_TX_LOONGSON 0x00010000 > +/* Abnormal Loongson Rx Interrupt Summary */ > +#define DMA_STATUS_AIS_RX_LOONGSON 0x00008000 > + > +/* Fatal Loongson Tx Bus Error Interrupt */ > +#define DMA_STATUS_FBI_TX_LOONGSON 0x00002000 > +/* Fatal Loongson Rx Bus Error Interrupt */ > +#define DMA_STATUS_FBI_RX_LOONGSON 0x00001000 > + > +#define DMA_STATUS_MSK_COMMON_LOONGSON (DMA_STATUS_NIS_TX_LOONGSON | \ > + DMA_STATUS_NIS_RX_LOONGSON | \ > + DMA_STATUS_AIS_TX_LOONGSON | \ > + DMA_STATUS_AIS_RX_LOONGSON | \ > + DMA_STATUS_FBI_TX_LOONGSON | \ > + DMA_STATUS_FBI_RX_LOONGSON) > + > +#define DMA_STATUS_MSK_RX_LOONGSON (DMA_STATUS_ERI | DMA_STATUS_RWT | \ > + DMA_STATUS_RPS | DMA_STATUS_RU | \ > + DMA_STATUS_RI | DMA_STATUS_OVF | \ > + DMA_STATUS_MSK_COMMON_LOONGSON) > + > +#define DMA_STATUS_MSK_TX_LOONGSON (DMA_STATUS_ETI | DMA_STATUS_UNF | \ > + DMA_STATUS_TJT | DMA_STATUS_TU | \ > + DMA_STATUS_TPS | DMA_STATUS_TI | \ > + DMA_STATUS_MSK_COMMON_LOONGSON) > > #define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03 > #define PCI_DEVICE_ID_LOONGSON_GNET 0x7a13 > +#define LOONGSON_DWMAC_CORE_1_00 0x10 /* Loongson custom IP */ What about using the name like calling as: +#define DWMAC_CORE_LS2K2000 0x10 Thus you'll have the name similar to the rest of the DWMAC_CORE_* macros and which would emphasize what the device for which the custom ID is specific. > +#define CHANNEL_NUM 8 > + > +struct loongson_data { > + u32 gmac_verion; Let's call it loongson_id thus referring to the stmmac_priv::synopsys_id field. > + struct device *dev; > +}; > > struct stmmac_pci_info { > int (*setup)(struct pci_dev *pdev, struct plat_stmmacenet_data *plat); > @@ -69,6 +131,168 @@ static struct stmmac_pci_info loongson_gmac_pci_info = { > .setup = loongson_gmac_data, > }; > > +static void loongson_gnet_dma_init_channel(struct stmmac_priv *priv, > + void __iomem *ioaddr, > + struct stmmac_dma_cfg *dma_cfg, > + u32 chan) > +{ > + int txpbl = dma_cfg->txpbl ?: dma_cfg->pbl; > + int rxpbl = dma_cfg->rxpbl ?: dma_cfg->pbl; > + u32 value; > + > + value = readl(ioaddr + DMA_CHAN_BUS_MODE(chan)); > + > + if (dma_cfg->pblx8) > + value |= DMA_BUS_MODE_MAXPBL; > + > + value |= DMA_BUS_MODE_USP; > + value &= ~(DMA_BUS_MODE_PBL_MASK | DMA_BUS_MODE_RPBL_MASK); > + value |= (txpbl << DMA_BUS_MODE_PBL_SHIFT); > + value |= (rxpbl << DMA_BUS_MODE_RPBL_SHIFT); > + > + /* Set the Fixed burst mode */ > + if (dma_cfg->fixed_burst) > + value |= DMA_BUS_MODE_FB; > + > + /* Mixed Burst has no effect when fb is set */ > + if (dma_cfg->mixed_burst) > + value |= DMA_BUS_MODE_MB; > + > + if (dma_cfg->atds) > + value |= DMA_BUS_MODE_ATDS; > + > + if (dma_cfg->aal) > + value |= DMA_BUS_MODE_AAL; > + > + writel(value, ioaddr + DMA_CHAN_BUS_MODE(chan)); > + > + /* Mask interrupts by writing to CSR7 */ > + writel(DMA_INTR_DEFAULT_MASK_LOONGSON, ioaddr + > + DMA_CHAN_INTR_ENA(chan)); > +} > + > +static int loongson_gnet_dma_interrupt(struct stmmac_priv *priv, > + void __iomem *ioaddr, > + struct stmmac_extra_stats *x, > + u32 chan, u32 dir) > +{ > + struct stmmac_pcpu_stats *stats = this_cpu_ptr(priv->xstats.pcpu_stats); > + u32 abnor_intr_status; > + u32 nor_intr_status; > + u32 fb_intr_status; > + u32 intr_status; > + int ret = 0; > + > + /* read the status register (CSR5) */ > + intr_status = readl(ioaddr + DMA_CHAN_STATUS(chan)); > + > + if (dir == DMA_DIR_RX) > + intr_status &= DMA_STATUS_MSK_RX_LOONGSON; > + else if (dir == DMA_DIR_TX) > + intr_status &= DMA_STATUS_MSK_TX_LOONGSON; > + > + nor_intr_status = intr_status & (DMA_STATUS_NIS_TX_LOONGSON | > + DMA_STATUS_NIS_RX_LOONGSON); > + abnor_intr_status = intr_status & (DMA_STATUS_AIS_TX_LOONGSON | > + DMA_STATUS_AIS_RX_LOONGSON); > + fb_intr_status = intr_status & (DMA_STATUS_FBI_TX_LOONGSON | > + DMA_STATUS_FBI_RX_LOONGSON); > + > + /* ABNORMAL interrupts */ > + if (unlikely(abnor_intr_status)) { > + if (unlikely(intr_status & DMA_STATUS_UNF)) { > + ret = tx_hard_error_bump_tc; > + x->tx_undeflow_irq++; > + } > + if (unlikely(intr_status & DMA_STATUS_TJT)) > + x->tx_jabber_irq++; > + if (unlikely(intr_status & DMA_STATUS_OVF)) > + x->rx_overflow_irq++; > + if (unlikely(intr_status & DMA_STATUS_RU)) > + x->rx_buf_unav_irq++; > + if (unlikely(intr_status & DMA_STATUS_RPS)) > + x->rx_process_stopped_irq++; > + if (unlikely(intr_status & DMA_STATUS_RWT)) > + x->rx_watchdog_irq++; > + if (unlikely(intr_status & DMA_STATUS_ETI)) > + x->tx_early_irq++; > + if (unlikely(intr_status & DMA_STATUS_TPS)) { > + x->tx_process_stopped_irq++; > + ret = tx_hard_error; > + } > + if (unlikely(fb_intr_status)) { > + x->fatal_bus_error_irq++; > + ret = tx_hard_error; > + } > + } > + /* TX/RX NORMAL interrupts */ > + if (likely(nor_intr_status)) { > + if (likely(intr_status & DMA_STATUS_RI)) { > + u32 value = readl(ioaddr + DMA_INTR_ENA); > + /* to schedule NAPI on real RIE event. */ > + if (likely(value & DMA_INTR_ENA_RIE)) { > + u64_stats_update_begin(&stats->syncp); > + u64_stats_inc(&stats->rx_normal_irq_n[chan]); > + u64_stats_update_end(&stats->syncp); > + ret |= handle_rx; > + } > + } > + if (likely(intr_status & DMA_STATUS_TI)) { > + u64_stats_update_begin(&stats->syncp); > + u64_stats_inc(&stats->tx_normal_irq_n[chan]); > + u64_stats_update_end(&stats->syncp); > + ret |= handle_tx; > + } > + if (unlikely(intr_status & DMA_STATUS_ERI)) > + x->rx_early_irq++; > + } > + /* Optional hardware blocks, interrupts should be disabled */ > + if (unlikely(intr_status & > + (DMA_STATUS_GPI | DMA_STATUS_GMI | DMA_STATUS_GLI))) > + pr_warn("%s: unexpected status %08x\n", __func__, intr_status); > + > + /* Clear the interrupt by writing a logic 1 to the CSR5[15-0] */ 0x7ffff != CSR5[15-0] > + writel((intr_status & 0x7ffff), ioaddr + DMA_CHAN_STATUS(chan)); > + > + return ret; > +} > + > +static void loongson_gnet_fix_speed(void *priv, unsigned int speed, > + unsigned int mode) > +{ > + struct loongson_data *ld = (struct loongson_data *)priv; > + struct net_device *ndev = dev_get_drvdata(ld->dev); > + struct stmmac_priv *ptr = netdev_priv(ndev); > + > + /* The controller and PHY don't work well together. > + * We need to use the PS bit to check if the controller's status > + * is correct and reset PHY if necessary. This doesn't correspond to what you're actually doing. Please align the comment with what is done below (if what I provided in the commit log regarding this problem is correct, use the description here). > + * MAC_CTRL_REG.15 is defined by the GMAC_CONTROL_PS macro. useless. please drop > + */ > + if (speed == SPEED_1000) { > + if (readl(ptr->ioaddr + MAC_CTRL_REG) & > + GMAC_CONTROL_PS) > + /* Word around hardware bug, restart autoneg */ > + phy_restart_aneg(ndev->phydev); > + } > +} > + > +static int loongson_gnet_data(struct pci_dev *pdev, > + struct plat_stmmacenet_data *plat) > +{ > + loongson_default_data(pdev, plat); > + > + plat->phy_interface = PHY_INTERFACE_MODE_GMII; > + plat->mdio_bus_data->phy_mask = ~(u32)BIT(2); > + plat->fix_mac_speed = loongson_gnet_fix_speed; > + > + return 0; > +} > + > +static struct stmmac_pci_info loongson_gnet_pci_info = { > + .setup = loongson_gnet_data, > +}; > + > static int loongson_dwmac_config_legacy(struct pci_dev *pdev, > struct plat_stmmacenet_data *plat, > struct stmmac_resources *res, > @@ -101,12 +325,126 @@ static int loongson_dwmac_config_legacy(struct pci_dev *pdev, > return 0; > } > > +static int loongson_dwmac_config_msi(struct pci_dev *pdev, > + struct plat_stmmacenet_data *plat, > + struct stmmac_resources *res, > + struct device_node *np) > +{ > + int i, ret, vecs; > + > + vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1); > + ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI); > + if (ret < 0) { > + dev_info(&pdev->dev, > + "MSI enable failed, Fallback to legacy interrupt\n"); > + return loongson_dwmac_config_legacy(pdev, plat, res, np); > + } > + > + res->irq = pci_irq_vector(pdev, 0); > + res->wol_irq = 0; > + > + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > + * --------- ----- -------- -------- ... -------- -------- > + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > + */ > + for (i = 0; i < CHANNEL_NUM; i++) { > + res->rx_irq[CHANNEL_NUM - 1 - i] = > + pci_irq_vector(pdev, 1 + i * 2); > + res->tx_irq[CHANNEL_NUM - 1 - i] = > + pci_irq_vector(pdev, 2 + i * 2); > + } > + > + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > + > + return 0; > +} > + > +static struct mac_device_info *loongson_dwmac_setup(void *apriv) > +{ > + struct stmmac_priv *priv = apriv; > + struct mac_device_info *mac; seems unused. See my next comment. > + struct stmmac_dma_ops *dma; > + struct loongson_data *ld; > + struct pci_dev *pdev; > + > + ld = priv->plat->bsp_priv; > + pdev = to_pci_dev(priv->device); > + > + mac = devm_kzalloc(priv->device, sizeof(*mac), GFP_KERNEL); > + if (!mac) > + return NULL; I see you no longer override the ops in dwmac1000_ops. If so this can be dropped. > + > + dma = devm_kzalloc(priv->device, sizeof(*dma), GFP_KERNEL); > + if (!dma) > + return NULL; > + > + /* The original IP-core version is 0x37 in all Loongson GNET s/0x37/v3.73a > + * (ls2k2000 and ls7a2000), but the GNET HW designers have changed the > + * GMAC_VERSION.SNPSVER field to the custom 0x10 value on the Loongson > + * ls2k2000 MAC to emphasize the differences: multiple DMA-channels, s/ls2k2000/LS2K2000 s/ls7a2000/LS7A2000 > + * AV feature and GMAC_INT_STATUS CSR flags layout. Get back the > + * original value so the correct HW-interface would be selected. > + */ > + if (ld->gmac_verion == LOONGSON_DWMAC_CORE_1_00) { > + priv->synopsys_id = DWMAC_CORE_3_70; > + *dma = dwmac1000_dma_ops; > + dma->init_chan = loongson_gnet_dma_init_channel; > + dma->dma_interrupt = loongson_gnet_dma_interrupt; > + mac->dma = dma; > + } > + > + mac->mac = &dwmac1000_ops; Unused? > + priv->dev->priv_flags |= IFF_UNICAST_FLT; > + > + /* Pre-initialize the respective "mac" fields as it's done in > + * dwmac1000_setup() > + */ > + mac->pcsr = priv->ioaddr; > + mac->multicast_filter_bins = priv->plat->multicast_filter_bins; > + mac->unicast_filter_entries = priv->plat->unicast_filter_entries; > + mac->mcast_bits_log2 = 0; > + > + if (mac->multicast_filter_bins) > + mac->mcast_bits_log2 = ilog2(mac->multicast_filter_bins); > + > + /* The GMAC devices with PCI ID 0x7a03 does not support any pause mode. > + * The GNET devices without CORE ID 0x10 does not support half-duplex. > + */ No need to mention the IDs but just the actual devices: /* Loongson GMAC doesn't support the flow control. LS2K2000 * GNET doesn't support the half-duplex link mode. */ > + if (pdev->device == PCI_DEVICE_ID_LOONGSON_GMAC) { > + mac->link.caps = MAC_10 | MAC_100 | MAC_1000; > + } else { > + if (ld->gmac_verion == LOONGSON_DWMAC_CORE_1_00) > + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | > + MAC_10 | MAC_100 | MAC_1000; > + else > + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | > + MAC_10FD | MAC_100FD | MAC_1000FD; > + } > + > + mac->link.duplex = GMAC_CONTROL_DM; > + mac->link.speed10 = GMAC_CONTROL_PS; > + mac->link.speed100 = GMAC_CONTROL_PS | GMAC_CONTROL_FES; > + mac->link.speed1000 = 0; > + mac->link.speed_mask = GMAC_CONTROL_PS | GMAC_CONTROL_FES; > + mac->mii.addr = GMAC_MII_ADDR; > + mac->mii.data = GMAC_MII_DATA; > + mac->mii.addr_shift = 11; > + mac->mii.addr_mask = 0x0000F800; > + mac->mii.reg_shift = 6; > + mac->mii.reg_mask = 0x000007C0; > + mac->mii.clk_csr_shift = 2; > + mac->mii.clk_csr_mask = GENMASK(5, 2); > + > + return mac; > +} > + > static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > struct plat_stmmacenet_data *plat; > int ret, i, bus_id, phy_mode; > struct stmmac_pci_info *info; > struct stmmac_resources res; > + struct loongson_data *ld; > struct device_node *np; > > np = dev_of_node(&pdev->dev); > @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > return -ENOMEM; > > plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL); > - if (!plat->dma_cfg) { > - ret = -ENOMEM; > - goto err_put_node; > - } > + if (!plat->dma_cfg) > + return -ENOMEM; This change must have been introduced in the patch [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support which moves the mdio_node pointer initialization to under the if-clause. > + > + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); > + if (!ld) > + return -ENOMEM; > > /* Enable pci device */ > ret = pci_enable_device(pdev); > @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > plat->phy_interface = phy_mode; > } > > - pci_enable_msi(pdev); Hm, this must be justified and better being done in a separate patch. > + plat->bsp_priv = ld; > + plat->setup = loongson_dwmac_setup; > + ld->dev = &pdev->dev; > + > memset(&res, 0, sizeof(res)); > res.addr = pcim_iomap_table(pdev)[0]; > + ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; > + > + switch (ld->gmac_verion) { > + case LOONGSON_DWMAC_CORE_1_00: > + plat->rx_queues_to_use = CHANNEL_NUM; > + plat->tx_queues_to_use = CHANNEL_NUM; > + > + /* Only channel 0 supports checksum, > + * so turn off checksum to enable multiple channels. > + */ > + for (i = 1; i < CHANNEL_NUM; i++) > + plat->tx_queues_cfg[i].coe_unsupported = 1; > > - plat->tx_queues_to_use = 1; > - plat->rx_queues_to_use = 1; > + ret = loongson_dwmac_config_msi(pdev, plat, &res, np); > + break; > + default: /* 0x35 device and 0x37 device. */ > + plat->tx_queues_to_use = 1; > + plat->rx_queues_to_use = 1; > Move the NoF queues (and coe flag) initializations to the respective loongson_*_data() methods. Besides I don't see you freeing the IRQ vectors allocated in the loongson_dwmac_config_msi() method neither in probe(), nor in remove() functions. That's definitely wrong. What you need is to have a method antagonistic to loongson_dwmac_config_msi() (like loongson_dwmac_clear_msi()) which would execute the cleanup procedure. > - ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > + break; > + } > > /* GNET devices with dev revision 0x00 do not support manually > * setting the speed to 1000. > @@ -189,12 +549,10 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > ret = stmmac_dvr_probe(&pdev->dev, plat, &res); > if (ret) > - goto err_disable_msi; > + goto err_disable_device; > > return ret; > > -err_disable_msi: > - pci_disable_msi(pdev); Once again. Justify the change. Moreover I don't see you dropping the pci_disable_msi() from the remove() method. -Serge(y) > err_disable_device: > pci_disable_device(pdev); > err_put_node: > @@ -262,6 +620,7 @@ static SIMPLE_DEV_PM_OPS(loongson_dwmac_pm_ops, loongson_dwmac_suspend, > > static const struct pci_device_id loongson_dwmac_id_table[] = { > { PCI_DEVICE_DATA(LOONGSON, GMAC, &loongson_gmac_pci_info) }, > + { PCI_DEVICE_DATA(LOONGSON, GNET, &loongson_gnet_pci_info) }, > {} > }; > MODULE_DEVICE_TABLE(pci, loongson_dwmac_id_table); > -- > 2.31.4 >
On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote: > ... > > +static int loongson_dwmac_config_msi(struct pci_dev *pdev, > + struct plat_stmmacenet_data *plat, > + struct stmmac_resources *res, > + struct device_node *np) > +{ > + int i, ret, vecs; > + > + vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1); > + ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI); > + if (ret < 0) { > + dev_info(&pdev->dev, > + "MSI enable failed, Fallback to legacy interrupt\n"); > + return loongson_dwmac_config_legacy(pdev, plat, res, np); > + } > + > + res->irq = pci_irq_vector(pdev, 0); > + res->wol_irq = 0; > + > + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > + * --------- ----- -------- -------- ... -------- -------- > + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > + */ > + for (i = 0; i < CHANNEL_NUM; i++) { > + res->rx_irq[CHANNEL_NUM - 1 - i] = > + pci_irq_vector(pdev, 1 + i * 2); > + res->tx_irq[CHANNEL_NUM - 1 - i] = > + pci_irq_vector(pdev, 2 + i * 2); > + } > + > + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > + > + return 0; > +} > + > ... > static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > struct plat_stmmacenet_data *plat; > int ret, i, bus_id, phy_mode; > struct stmmac_pci_info *info; > struct stmmac_resources res; > + struct loongson_data *ld; > struct device_node *np; > > np = dev_of_node(&pdev->dev); > @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > return -ENOMEM; > > plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL); > - if (!plat->dma_cfg) { > - ret = -ENOMEM; > - goto err_put_node; > - } > + if (!plat->dma_cfg) > + return -ENOMEM; > + > + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); > + if (!ld) > + return -ENOMEM; > > /* Enable pci device */ > ret = pci_enable_device(pdev); > @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > plat->phy_interface = phy_mode; > } > > - pci_enable_msi(pdev); > + plat->bsp_priv = ld; > + plat->setup = loongson_dwmac_setup; > + ld->dev = &pdev->dev; > + > memset(&res, 0, sizeof(res)); > res.addr = pcim_iomap_table(pdev)[0]; > + ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; > + > + switch (ld->gmac_verion) { > + case LOONGSON_DWMAC_CORE_1_00: > + plat->rx_queues_to_use = CHANNEL_NUM; > + plat->tx_queues_to_use = CHANNEL_NUM; > + > + /* Only channel 0 supports checksum, > + * so turn off checksum to enable multiple channels. > + */ > + for (i = 1; i < CHANNEL_NUM; i++) > + plat->tx_queues_cfg[i].coe_unsupported = 1; > > - plat->tx_queues_to_use = 1; > - plat->rx_queues_to_use = 1; > + ret = loongson_dwmac_config_msi(pdev, plat, &res, np); > + break; > + default: /* 0x35 device and 0x37 device. */ > + plat->tx_queues_to_use = 1; > + plat->rx_queues_to_use = 1; > > - ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > + break; > + } > Let's now talk about this change. First of all, one more time. You can't miss the return value check because if any of the IRQ config method fails then the driver won't work! The first change that introduces the problem is in the patch [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy Second, as I already mentioned in another message sent to this patch you are missing the PCI MSI IRQs freeing in the cleanup-on-error path and in the device/driver remove() function. It's definitely wrong. Thirdly, you said that the node-pointer is now optional and introduced the patch [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support If so and the DT-based setting up isn't mandatory then I would suggest to proceed with the entire so called legacy setups only if the node-pointer has been found, otherwise the pure PCI-based setup would be performed. So the patches 10-13 (in your v12 order) would look like this: 1. Patch 10 introduces the two types of the configs - DT and PCI plus the bus_id initialized based on the PCI domain and device ID. [PATCH net-next v13 10/15] net: stmmac: dwmac-loongson: Add DT-less GMAC PCI-device support The DT and PCI config functions can look like this: static int loongson_dwmac_config_dt(struct pci_dev *pdev, struct plat_stmmacenet_data *plat, struct stmmac_resources *res) { struct device_node *np = dev_of_node(&pdev->dev); int ret; plat->mdio_node = of_get_child_by_name(np, "mdio"); if (plat->mdio_node) { dev_info(&pdev->dev, "Found MDIO subnode\n"); plat->mdio_bus_data->needs_reset = true; } ret = of_alias_get_id(np, "ethernet"); if (ret >= 0) plat->bus_id = ret; res->irq = of_irq_get_byname(np, "macirq"); if (res->irq < 0) { dev_err(&pdev->dev, "IRQ macirq not found\n"); return -ENODEV; } res->wol_irq = of_irq_get_byname(np, "eth_wake_irq"); if (res->wol_irq < 0) { dev_info(&pdev->dev, "IRQ eth_wake_irq not found, using macirq\n"); res->wol_irq = res->irq; } res->lpi_irq = of_irq_get_byname(np, "eth_lpi"); if (res->lpi_irq < 0) { dev_err(&pdev->dev, "IRQ eth_lpi not found\n"); return -ENODEV; } return 0; } static int loongson_dwmac_config_pci(struct pci_dev *pdev, struct plat_stmmacenet_data *plat, struct stmmac_resources *res) { res.irq = pdev->irq; return 0; } ... static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) { ... if (dev_of_node(&pdev->dev)) ret = loongson_dwmac_dt_config(pdev, plat, res); else ret = loongson_dwmac_pci_config(pdev, plat, res); if (ret) goto err_disable_msi; ... } 2. Patch 11 introduces the stmmac_pci_info structure, makes the stmmac_pci_info::setup() callback called in the probe() function and assigns the loongson_gmac_data() method pointer to the GMAC info data. [PATCH net-next v13 11/15] net: stmmac: dwmac-loongson: Introduce PCI device info data 3. Patch 12 can be preserved as is (but see my notes regarding moving a part of it to the patch 13). [PATCH net-next v13 12/15] net: stmmac: dwmac-loongson: Add flag disabling AN-less 1Gbps setup 4. Patch 13 introduces the GNET support as it's mainly done in your patch (see my notes in there though) [PATCH net-next v13 13/15] net: stmmac: dwmac-loongson: Add Loongson GNET support but the loongson_dwmac_config_pci() method would now look as follows: static int loongson_dwmac_config_pci(struct pci_dev *pdev, struct plat_stmmacenet_data *plat, struct stmmac_resources *res) { int i, ret, vecs; /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | * --------- ----- -------- -------- ... -------- -------- * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | */ vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1); ret = pci_alloc_irq_vectors(pdev, 1, vecs, PCI_IRQ_MSI | PCI_IRQ_LEGACY); if (ret < 0) { dev_err(&pdev->dev, "Failed to allocate PCI IRQs\n"); return ret; } else if (ret >= vecs) { for (i = 0; i < CHANNEL_NUM; i++) { res->rx_irq[CHANNEL_NUM - 1 - i] = pci_irq_vector(pdev, 1 + i * 2); res->tx_irq[CHANNEL_NUM - 1 - i] = pci_irq_vector(pdev, 2 + i * 2); } plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; } else { dev_warn(&pdev->dev, "Fall back to PCIe INTx IRQs\n"); } res->irq = pci_irq_vector(pdev, 0); return 0; } What do you think? -Serge(y)
Hi Serge, 在 2024/5/6 18:39, Serge Semin 写道: > On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote: >> ... >> >> +static int loongson_dwmac_config_msi(struct pci_dev *pdev, >> + struct plat_stmmacenet_data *plat, >> + struct stmmac_resources *res, >> + struct device_node *np) >> +{ >> + int i, ret, vecs; >> + >> + vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1); >> + ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI); >> + if (ret < 0) { >> + dev_info(&pdev->dev, >> + "MSI enable failed, Fallback to legacy interrupt\n"); >> + return loongson_dwmac_config_legacy(pdev, plat, res, np); >> + } >> + >> + res->irq = pci_irq_vector(pdev, 0); >> + res->wol_irq = 0; >> + >> + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | >> + * --------- ----- -------- -------- ... -------- -------- >> + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | >> + */ >> + for (i = 0; i < CHANNEL_NUM; i++) { >> + res->rx_irq[CHANNEL_NUM - 1 - i] = >> + pci_irq_vector(pdev, 1 + i * 2); >> + res->tx_irq[CHANNEL_NUM - 1 - i] = >> + pci_irq_vector(pdev, 2 + i * 2); >> + } >> + >> + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; >> + >> + return 0; >> +} >> + >> ... >> static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> { >> struct plat_stmmacenet_data *plat; >> int ret, i, bus_id, phy_mode; >> struct stmmac_pci_info *info; >> struct stmmac_resources res; >> + struct loongson_data *ld; >> struct device_node *np; >> >> np = dev_of_node(&pdev->dev); >> @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id >> return -ENOMEM; >> >> plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL); >> - if (!plat->dma_cfg) { >> - ret = -ENOMEM; >> - goto err_put_node; >> - } >> + if (!plat->dma_cfg) >> + return -ENOMEM; >> + >> + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); >> + if (!ld) >> + return -ENOMEM; >> >> /* Enable pci device */ >> ret = pci_enable_device(pdev); >> @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id >> plat->phy_interface = phy_mode; >> } >> >> - pci_enable_msi(pdev); >> + plat->bsp_priv = ld; >> + plat->setup = loongson_dwmac_setup; >> + ld->dev = &pdev->dev; >> + >> memset(&res, 0, sizeof(res)); >> res.addr = pcim_iomap_table(pdev)[0]; >> + ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; >> + >> + switch (ld->gmac_verion) { >> + case LOONGSON_DWMAC_CORE_1_00: >> + plat->rx_queues_to_use = CHANNEL_NUM; >> + plat->tx_queues_to_use = CHANNEL_NUM; >> + >> + /* Only channel 0 supports checksum, >> + * so turn off checksum to enable multiple channels. >> + */ >> + for (i = 1; i < CHANNEL_NUM; i++) >> + plat->tx_queues_cfg[i].coe_unsupported = 1; >> >> - plat->tx_queues_to_use = 1; >> - plat->rx_queues_to_use = 1; >> + ret = loongson_dwmac_config_msi(pdev, plat, &res, np); >> + break; >> + default: /* 0x35 device and 0x37 device. */ >> + plat->tx_queues_to_use = 1; >> + plat->rx_queues_to_use = 1; >> >> - ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); >> + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); >> + break; >> + } >> > Let's now talk about this change. > > First of all, one more time. You can't miss the return value check > because if any of the IRQ config method fails then the driver won't > work! The first change that introduces the problem is in the patch > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy OK! > > Second, as I already mentioned in another message sent to this patch > you are missing the PCI MSI IRQs freeing in the cleanup-on-error path > and in the device/driver remove() function. It's definitely wrong. You are right! I will do it. > Thirdly, you said that the node-pointer is now optional and introduced > the patch > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > If so and the DT-based setting up isn't mandatory then I would > suggest to proceed with the entire so called legacy setups only if the > node-pointer has been found, otherwise the pure PCI-based setup would > be performed. So the patches 10-13 (in your v12 order) would look In this case, MSI will not be enabled when the node-pointer is found. . In fact, a large fraction of 2k devices are DT-based, of course, many are PCI-based. Thanks, Yanteng
On Tue, May 07, 2024 at 09:35:24PM +0800, Yanteng Si wrote: > Hi Serge, > > 在 2024/5/6 18:39, Serge Semin 写道: > > On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote: > > > ... > > > +static int loongson_dwmac_config_msi(struct pci_dev *pdev, > > > + struct plat_stmmacenet_data *plat, > > > + struct stmmac_resources *res, > > > + struct device_node *np) > > > +{ > > > + int i, ret, vecs; > > > + > > > + vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1); > > > + ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI); > > > + if (ret < 0) { > > > + dev_info(&pdev->dev, > > > + "MSI enable failed, Fallback to legacy interrupt\n"); > > > + return loongson_dwmac_config_legacy(pdev, plat, res, np); > > > + } > > > + > > > + res->irq = pci_irq_vector(pdev, 0); > > > + res->wol_irq = 0; > > > + > > > + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > > > + * --------- ----- -------- -------- ... -------- -------- > > > + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > > > + */ > > > + for (i = 0; i < CHANNEL_NUM; i++) { > > > + res->rx_irq[CHANNEL_NUM - 1 - i] = > > > + pci_irq_vector(pdev, 1 + i * 2); > > > + res->tx_irq[CHANNEL_NUM - 1 - i] = > > > + pci_irq_vector(pdev, 2 + i * 2); > > > + } > > > + > > > + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > > > + > > > + return 0; > > > +} > > > + > > > ... > > > static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > { > > > struct plat_stmmacenet_data *plat; > > > int ret, i, bus_id, phy_mode; > > > struct stmmac_pci_info *info; > > > struct stmmac_resources res; > > > + struct loongson_data *ld; > > > struct device_node *np; > > > np = dev_of_node(&pdev->dev); > > > @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > return -ENOMEM; > > > plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL); > > > - if (!plat->dma_cfg) { > > > - ret = -ENOMEM; > > > - goto err_put_node; > > > - } > > > + if (!plat->dma_cfg) > > > + return -ENOMEM; > > > + > > > + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); > > > + if (!ld) > > > + return -ENOMEM; > > > /* Enable pci device */ > > > ret = pci_enable_device(pdev); > > > @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > plat->phy_interface = phy_mode; > > > } > > > - pci_enable_msi(pdev); > > > + plat->bsp_priv = ld; > > > + plat->setup = loongson_dwmac_setup; > > > + ld->dev = &pdev->dev; > > > + > > > memset(&res, 0, sizeof(res)); > > > res.addr = pcim_iomap_table(pdev)[0]; > > > + ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; > > > + > > > + switch (ld->gmac_verion) { > > > + case LOONGSON_DWMAC_CORE_1_00: > > > + plat->rx_queues_to_use = CHANNEL_NUM; > > > + plat->tx_queues_to_use = CHANNEL_NUM; > > > + > > > + /* Only channel 0 supports checksum, > > > + * so turn off checksum to enable multiple channels. > > > + */ > > > + for (i = 1; i < CHANNEL_NUM; i++) > > > + plat->tx_queues_cfg[i].coe_unsupported = 1; > > > - plat->tx_queues_to_use = 1; > > > - plat->rx_queues_to_use = 1; > > > + ret = loongson_dwmac_config_msi(pdev, plat, &res, np); > > > + break; > > > + default: /* 0x35 device and 0x37 device. */ > > > + plat->tx_queues_to_use = 1; > > > + plat->rx_queues_to_use = 1; > > > - ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > + break; > > > + } > > Let's now talk about this change. > > > > First of all, one more time. You can't miss the return value check > > because if any of the IRQ config method fails then the driver won't > > work! The first change that introduces the problem is in the patch > > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > OK! > > > > Second, as I already mentioned in another message sent to this patch > > you are missing the PCI MSI IRQs freeing in the cleanup-on-error path > > and in the device/driver remove() function. It's definitely wrong. > You are right! I will do it. > > Thirdly, you said that the node-pointer is now optional and introduced > > the patch > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > If so and the DT-based setting up isn't mandatory then I would > > suggest to proceed with the entire so called legacy setups only if the > > node-pointer has been found, otherwise the pure PCI-based setup would > > be performed. So the patches 10-13 (in your v12 order) would look > > In this case, MSI will not be enabled when the node-pointer is found. > > . > > > In fact, a large fraction of 2k devices are DT-based, of course, many are > PCI-based. Then please summarise which devices need the DT-node pointer which don't? And most importantly if they do why do they need the DT-node? AFAICS currently both LS2K1000 and LS7A1000 GMACs require the DT-node to get the MAC and LPI IRQ signals. AFAICS from your series LS7A2000 GNET is also DT-based for the same reason. But the LS2K2000 GNET case is different. You say that some of the platforms have the respective DT-node some don't, but at the same time you submitting this patch which permits the MSI IRQs only for the LS7A2000 GNET. It looks contradicting. Does it mean that the GNET devices may generate the IRQs via both legacy (an IRQ signal directly connected to the system GIC) and the PCI MSI ways? Let's get the question to the more generic level. Are the Loongson GNET and GMAC controllers able to generate the IRQs via both ways: physical IRQ signal and PCI MSI? Please don't consider this as a vastly meticulous review. I am just trying to figure out how to make things less complicated and fix the driver to permitting only the cases which are actually possible. -Serge(y) > > > Thanks, > > Yanteng > >
Hi, Serge, On Wed, May 8, 2024 at 10:38 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Tue, May 07, 2024 at 09:35:24PM +0800, Yanteng Si wrote: > > Hi Serge, > > > > 在 2024/5/6 18:39, Serge Semin 写道: > > > On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote: > > > > ... > > > > +static int loongson_dwmac_config_msi(struct pci_dev *pdev, > > > > + struct plat_stmmacenet_data *plat, > > > > + struct stmmac_resources *res, > > > > + struct device_node *np) > > > > +{ > > > > + int i, ret, vecs; > > > > + > > > > + vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1); > > > > + ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI); > > > > + if (ret < 0) { > > > > + dev_info(&pdev->dev, > > > > + "MSI enable failed, Fallback to legacy interrupt\n"); > > > > + return loongson_dwmac_config_legacy(pdev, plat, res, np); > > > > + } > > > > + > > > > + res->irq = pci_irq_vector(pdev, 0); > > > > + res->wol_irq = 0; > > > > + > > > > + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > > > > + * --------- ----- -------- -------- ... -------- -------- > > > > + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > > > > + */ > > > > + for (i = 0; i < CHANNEL_NUM; i++) { > > > > + res->rx_irq[CHANNEL_NUM - 1 - i] = > > > > + pci_irq_vector(pdev, 1 + i * 2); > > > > + res->tx_irq[CHANNEL_NUM - 1 - i] = > > > > + pci_irq_vector(pdev, 2 + i * 2); > > > > + } > > > > + > > > > + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > ... > > > > static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > > { > > > > struct plat_stmmacenet_data *plat; > > > > int ret, i, bus_id, phy_mode; > > > > struct stmmac_pci_info *info; > > > > struct stmmac_resources res; > > > > + struct loongson_data *ld; > > > > struct device_node *np; > > > > np = dev_of_node(&pdev->dev); > > > > @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > > return -ENOMEM; > > > > plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL); > > > > - if (!plat->dma_cfg) { > > > > - ret = -ENOMEM; > > > > - goto err_put_node; > > > > - } > > > > + if (!plat->dma_cfg) > > > > + return -ENOMEM; > > > > + > > > > + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); > > > > + if (!ld) > > > > + return -ENOMEM; > > > > /* Enable pci device */ > > > > ret = pci_enable_device(pdev); > > > > @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > > plat->phy_interface = phy_mode; > > > > } > > > > - pci_enable_msi(pdev); > > > > + plat->bsp_priv = ld; > > > > + plat->setup = loongson_dwmac_setup; > > > > + ld->dev = &pdev->dev; > > > > + > > > > memset(&res, 0, sizeof(res)); > > > > res.addr = pcim_iomap_table(pdev)[0]; > > > > + ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; > > > > + > > > > + switch (ld->gmac_verion) { > > > > + case LOONGSON_DWMAC_CORE_1_00: > > > > + plat->rx_queues_to_use = CHANNEL_NUM; > > > > + plat->tx_queues_to_use = CHANNEL_NUM; > > > > + > > > > + /* Only channel 0 supports checksum, > > > > + * so turn off checksum to enable multiple channels. > > > > + */ > > > > + for (i = 1; i < CHANNEL_NUM; i++) > > > > + plat->tx_queues_cfg[i].coe_unsupported = 1; > > > > - plat->tx_queues_to_use = 1; > > > > - plat->rx_queues_to_use = 1; > > > > + ret = loongson_dwmac_config_msi(pdev, plat, &res, np); > > > > + break; > > > > + default: /* 0x35 device and 0x37 device. */ > > > > + plat->tx_queues_to_use = 1; > > > > + plat->rx_queues_to_use = 1; > > > > - ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > > + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > > + break; > > > > + } > > > Let's now talk about this change. > > > > > > First of all, one more time. You can't miss the return value check > > > because if any of the IRQ config method fails then the driver won't > > > work! The first change that introduces the problem is in the patch > > > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > OK! > > > > > > Second, as I already mentioned in another message sent to this patch > > > you are missing the PCI MSI IRQs freeing in the cleanup-on-error path > > > and in the device/driver remove() function. It's definitely wrong. > > You are right! I will do it. > > > Thirdly, you said that the node-pointer is now optional and introduced > > > the patch > > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > > If so and the DT-based setting up isn't mandatory then I would > > > suggest to proceed with the entire so called legacy setups only if the > > > node-pointer has been found, otherwise the pure PCI-based setup would > > > be performed. So the patches 10-13 (in your v12 order) would look > > > > In this case, MSI will not be enabled when the node-pointer is found. > > > > . > > > > > > In fact, a large fraction of 2k devices are DT-based, of course, many are > > PCI-based. > > Then please summarise which devices need the DT-node pointer which > don't? And most importantly if they do why do they need the DT-node? Whether we need DT-nodes doesn't depend on device type, but depends on the BIOS type. When we boot with UEFI+ACPI, we don't need DT-node, when we boot with PMON+FDT, we need DT-node. Loongson machines may have either BIOS types. Huacai > > AFAICS currently both LS2K1000 and LS7A1000 GMACs require the DT-node > to get the MAC and LPI IRQ signals. AFAICS from your series LS7A2000 > GNET is also DT-based for the same reason. But the LS2K2000 GNET case > is different. You say that some of the platforms have the respective > DT-node some don't, but at the same time you submitting this patch > which permits the MSI IRQs only for the LS7A2000 GNET. It looks > contradicting. Does it mean that the GNET devices may generate the > IRQs via both legacy (an IRQ signal directly connected to the system > GIC) and the PCI MSI ways? > > Let's get the question to the more generic level. Are the Loongson > GNET and GMAC controllers able to generate the IRQs via both ways: > physical IRQ signal and PCI MSI? > > Please don't consider this as a vastly meticulous review. I am just > trying to figure out how to make things less complicated and fix the > driver to permitting only the cases which are actually possible. > > -Serge(y) > > > > > > > Thanks, > > > > Yanteng > > > >
On Wed, May 08, 2024 at 10:58:16PM +0800, Huacai Chen wrote: > Hi, Serge, > > On Wed, May 8, 2024 at 10:38 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > On Tue, May 07, 2024 at 09:35:24PM +0800, Yanteng Si wrote: > > > Hi Serge, > > > > > > 在 2024/5/6 18:39, Serge Semin 写道: > > > > On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote: > > > > > ... > > > > > +static int loongson_dwmac_config_msi(struct pci_dev *pdev, > > > > > + struct plat_stmmacenet_data *plat, > > > > > + struct stmmac_resources *res, > > > > > + struct device_node *np) > > > > > +{ > > > > > + int i, ret, vecs; > > > > > + > > > > > + vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1); > > > > > + ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI); > > > > > + if (ret < 0) { > > > > > + dev_info(&pdev->dev, > > > > > + "MSI enable failed, Fallback to legacy interrupt\n"); > > > > > + return loongson_dwmac_config_legacy(pdev, plat, res, np); > > > > > + } > > > > > + > > > > > + res->irq = pci_irq_vector(pdev, 0); > > > > > + res->wol_irq = 0; > > > > > + > > > > > + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > > > > > + * --------- ----- -------- -------- ... -------- -------- > > > > > + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > > > > > + */ > > > > > + for (i = 0; i < CHANNEL_NUM; i++) { > > > > > + res->rx_irq[CHANNEL_NUM - 1 - i] = > > > > > + pci_irq_vector(pdev, 1 + i * 2); > > > > > + res->tx_irq[CHANNEL_NUM - 1 - i] = > > > > > + pci_irq_vector(pdev, 2 + i * 2); > > > > > + } > > > > > + > > > > > + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > ... > > > > > static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > > > { > > > > > struct plat_stmmacenet_data *plat; > > > > > int ret, i, bus_id, phy_mode; > > > > > struct stmmac_pci_info *info; > > > > > struct stmmac_resources res; > > > > > + struct loongson_data *ld; > > > > > struct device_node *np; > > > > > np = dev_of_node(&pdev->dev); > > > > > @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > > > return -ENOMEM; > > > > > plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL); > > > > > - if (!plat->dma_cfg) { > > > > > - ret = -ENOMEM; > > > > > - goto err_put_node; > > > > > - } > > > > > + if (!plat->dma_cfg) > > > > > + return -ENOMEM; > > > > > + > > > > > + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); > > > > > + if (!ld) > > > > > + return -ENOMEM; > > > > > /* Enable pci device */ > > > > > ret = pci_enable_device(pdev); > > > > > @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > > > plat->phy_interface = phy_mode; > > > > > } > > > > > - pci_enable_msi(pdev); > > > > > + plat->bsp_priv = ld; > > > > > + plat->setup = loongson_dwmac_setup; > > > > > + ld->dev = &pdev->dev; > > > > > + > > > > > memset(&res, 0, sizeof(res)); > > > > > res.addr = pcim_iomap_table(pdev)[0]; > > > > > + ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; > > > > > + > > > > > + switch (ld->gmac_verion) { > > > > > + case LOONGSON_DWMAC_CORE_1_00: > > > > > + plat->rx_queues_to_use = CHANNEL_NUM; > > > > > + plat->tx_queues_to_use = CHANNEL_NUM; > > > > > + > > > > > + /* Only channel 0 supports checksum, > > > > > + * so turn off checksum to enable multiple channels. > > > > > + */ > > > > > + for (i = 1; i < CHANNEL_NUM; i++) > > > > > + plat->tx_queues_cfg[i].coe_unsupported = 1; > > > > > - plat->tx_queues_to_use = 1; > > > > > - plat->rx_queues_to_use = 1; > > > > > + ret = loongson_dwmac_config_msi(pdev, plat, &res, np); > > > > > + break; > > > > > + default: /* 0x35 device and 0x37 device. */ > > > > > + plat->tx_queues_to_use = 1; > > > > > + plat->rx_queues_to_use = 1; > > > > > - ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > > > + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > > > + break; > > > > > + } > > > > Let's now talk about this change. > > > > > > > > First of all, one more time. You can't miss the return value check > > > > because if any of the IRQ config method fails then the driver won't > > > > work! The first change that introduces the problem is in the patch > > > > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > > OK! > > > > > > > > Second, as I already mentioned in another message sent to this patch > > > > you are missing the PCI MSI IRQs freeing in the cleanup-on-error path > > > > and in the device/driver remove() function. It's definitely wrong. > > > You are right! I will do it. > > > > Thirdly, you said that the node-pointer is now optional and introduced > > > > the patch > > > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > > > If so and the DT-based setting up isn't mandatory then I would > > > > suggest to proceed with the entire so called legacy setups only if the > > > > node-pointer has been found, otherwise the pure PCI-based setup would > > > > be performed. So the patches 10-13 (in your v12 order) would look > > > > > > In this case, MSI will not be enabled when the node-pointer is found. > > > > > > . > > > > > > > > > In fact, a large fraction of 2k devices are DT-based, of course, many are > > > PCI-based. > > > > Then please summarise which devices need the DT-node pointer which > > don't? And most importantly if they do why do they need the DT-node? > Whether we need DT-nodes doesn't depend on device type, but depends on > the BIOS type. When we boot with UEFI+ACPI, we don't need DT-node, > when we boot with PMON+FDT, we need DT-node. Loongson machines may > have either BIOS types. Thanks for the answer. Just to fully clarify. Does it mean that all Loongson Ethernet controllers (Loongson GNET and GMAC) are able to deliver both PCI MSI IRQs and direct GIC IRQs (so called legacy)? -Serge(y) > > Huacai > > > > > AFAICS currently both LS2K1000 and LS7A1000 GMACs require the DT-node > > to get the MAC and LPI IRQ signals. AFAICS from your series LS7A2000 > > GNET is also DT-based for the same reason. But the LS2K2000 GNET case > > is different. You say that some of the platforms have the respective > > DT-node some don't, but at the same time you submitting this patch > > which permits the MSI IRQs only for the LS7A2000 GNET. It looks > > contradicting. Does it mean that the GNET devices may generate the > > IRQs via both legacy (an IRQ signal directly connected to the system > > GIC) and the PCI MSI ways? > > > > Let's get the question to the more generic level. Are the Loongson > > GNET and GMAC controllers able to generate the IRQs via both ways: > > physical IRQ signal and PCI MSI? > > > > Please don't consider this as a vastly meticulous review. I am just > > trying to figure out how to make things less complicated and fix the > > driver to permitting only the cases which are actually possible. > > > > -Serge(y) > > > > > > > > > > > Thanks, > > > > > > Yanteng > > > > > >
Hi Serge 在 2024/5/8 23:10, Serge Semin 写道: > On Wed, May 08, 2024 at 10:58:16PM +0800, Huacai Chen wrote: >> Hi, Serge, >> >> On Wed, May 8, 2024 at 10:38 PM Serge Semin<fancer.lancer@gmail.com> wrote: >>> On Tue, May 07, 2024 at 09:35:24PM +0800, Yanteng Si wrote: >>>> Hi Serge, >>>> >>>> 在 2024/5/6 18:39, Serge Semin 写道: >>>>> On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote: >>>>>> ... >>>>>> +static int loongson_dwmac_config_msi(struct pci_dev *pdev, >>>>>> + struct plat_stmmacenet_data *plat, >>>>>> + struct stmmac_resources *res, >>>>>> + struct device_node *np) >>>>>> +{ >>>>>> + int i, ret, vecs; >>>>>> + >>>>>> + vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1); >>>>>> + ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI); >>>>>> + if (ret < 0) { >>>>>> + dev_info(&pdev->dev, >>>>>> + "MSI enable failed, Fallback to legacy interrupt\n"); >>>>>> + return loongson_dwmac_config_legacy(pdev, plat, res, np); >>>>>> + } >>>>>> + >>>>>> + res->irq = pci_irq_vector(pdev, 0); >>>>>> + res->wol_irq = 0; >>>>>> + >>>>>> + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | >>>>>> + * --------- ----- -------- -------- ... -------- -------- >>>>>> + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | >>>>>> + */ >>>>>> + for (i = 0; i < CHANNEL_NUM; i++) { >>>>>> + res->rx_irq[CHANNEL_NUM - 1 - i] = >>>>>> + pci_irq_vector(pdev, 1 + i * 2); >>>>>> + res->tx_irq[CHANNEL_NUM - 1 - i] = >>>>>> + pci_irq_vector(pdev, 2 + i * 2); >>>>>> + } >>>>>> + >>>>>> + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> ... >>>>>> static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) >>>>>> { >>>>>> struct plat_stmmacenet_data *plat; >>>>>> int ret, i, bus_id, phy_mode; >>>>>> struct stmmac_pci_info *info; >>>>>> struct stmmac_resources res; >>>>>> + struct loongson_data *ld; >>>>>> struct device_node *np; >>>>>> np = dev_of_node(&pdev->dev); >>>>>> @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id >>>>>> return -ENOMEM; >>>>>> plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL); >>>>>> - if (!plat->dma_cfg) { >>>>>> - ret = -ENOMEM; >>>>>> - goto err_put_node; >>>>>> - } >>>>>> + if (!plat->dma_cfg) >>>>>> + return -ENOMEM; >>>>>> + >>>>>> + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); >>>>>> + if (!ld) >>>>>> + return -ENOMEM; >>>>>> /* Enable pci device */ >>>>>> ret = pci_enable_device(pdev); >>>>>> @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id >>>>>> plat->phy_interface = phy_mode; >>>>>> } >>>>>> - pci_enable_msi(pdev); >>>>>> + plat->bsp_priv = ld; >>>>>> + plat->setup = loongson_dwmac_setup; >>>>>> + ld->dev = &pdev->dev; >>>>>> + >>>>>> memset(&res, 0, sizeof(res)); >>>>>> res.addr = pcim_iomap_table(pdev)[0]; >>>>>> + ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; >>>>>> + >>>>>> + switch (ld->gmac_verion) { >>>>>> + case LOONGSON_DWMAC_CORE_1_00: >>>>>> + plat->rx_queues_to_use = CHANNEL_NUM; >>>>>> + plat->tx_queues_to_use = CHANNEL_NUM; >>>>>> + >>>>>> + /* Only channel 0 supports checksum, >>>>>> + * so turn off checksum to enable multiple channels. >>>>>> + */ >>>>>> + for (i = 1; i < CHANNEL_NUM; i++) >>>>>> + plat->tx_queues_cfg[i].coe_unsupported = 1; >>>>>> - plat->tx_queues_to_use = 1; >>>>>> - plat->rx_queues_to_use = 1; >>>>>> + ret = loongson_dwmac_config_msi(pdev, plat, &res, np); >>>>>> + break; >>>>>> + default: /* 0x35 device and 0x37 device. */ >>>>>> + plat->tx_queues_to_use = 1; >>>>>> + plat->rx_queues_to_use = 1; >>>>>> - ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); >>>>>> + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); >>>>>> + break; >>>>>> + } >>>>> Let's now talk about this change. >>>>> >>>>> First of all, one more time. You can't miss the return value check >>>>> because if any of the IRQ config method fails then the driver won't >>>>> work! The first change that introduces the problem is in the patch >>>>> [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy >>>> OK! >>>>> Second, as I already mentioned in another message sent to this patch >>>>> you are missing the PCI MSI IRQs freeing in the cleanup-on-error path >>>>> and in the device/driver remove() function. It's definitely wrong. >>>> You are right! I will do it. >>>>> Thirdly, you said that the node-pointer is now optional and introduced >>>>> the patch >>>>> [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support >>>>> If so and the DT-based setting up isn't mandatory then I would >>>>> suggest to proceed with the entire so called legacy setups only if the >>>>> node-pointer has been found, otherwise the pure PCI-based setup would >>>>> be performed. So the patches 10-13 (in your v12 order) would look >>>> In this case, MSI will not be enabled when the node-pointer is found. >>>> >>>> . >>>> >>>> >>>> In fact, a large fraction of 2k devices are DT-based, of course, many are >>>> PCI-based. >>> Then please summarise which devices need the DT-node pointer which >>> don't? And most importantly if they do why do they need the DT-node? >> Whether we need DT-nodes doesn't depend on device type, but depends on >> the BIOS type. When we boot with UEFI+ACPI, we don't need DT-node, >> when we boot with PMON+FDT, we need DT-node. Loongson machines may >> have either BIOS types. > Thanks for the answer. Just to fully clarify. Does it mean that all > Loongson Ethernet controllers (Loongson GNET and GMAC) are able to > deliver both PCI MSI IRQs and direct GIC IRQs (so called legacy)? No, only devices that support multiple channels can deliver both PCI MSI IRQs and direct GIC IRQs, other devices can only deliver GIC IRQs. Furthermore, multiple channel features are bundled with MSI. If we want to enable multiple channels, we must enable MSI. Thanks, Yanteng
On Thu, May 09, 2024 at 04:57:44PM +0800, Yanteng Si wrote: > Hi Serge > > 在 2024/5/8 23:10, Serge Semin 写道: > > On Wed, May 08, 2024 at 10:58:16PM +0800, Huacai Chen wrote: > > > Hi, Serge, > > > > > > On Wed, May 8, 2024 at 10:38 PM Serge Semin<fancer.lancer@gmail.com> wrote: > > > > On Tue, May 07, 2024 at 09:35:24PM +0800, Yanteng Si wrote: > > > > > Hi Serge, > > > > > > > > > > 在 2024/5/6 18:39, Serge Semin 写道: > > > > > > On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote: > > > > > > > ... > > > > > > > +static int loongson_dwmac_config_msi(struct pci_dev *pdev, > > > > > > > + struct plat_stmmacenet_data *plat, > > > > > > > + struct stmmac_resources *res, > > > > > > > + struct device_node *np) > > > > > > > +{ > > > > > > > + int i, ret, vecs; > > > > > > > + > > > > > > > + vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1); > > > > > > > + ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI); > > > > > > > + if (ret < 0) { > > > > > > > + dev_info(&pdev->dev, > > > > > > > + "MSI enable failed, Fallback to legacy interrupt\n"); > > > > > > > + return loongson_dwmac_config_legacy(pdev, plat, res, np); > > > > > > > + } > > > > > > > + > > > > > > > + res->irq = pci_irq_vector(pdev, 0); > > > > > > > + res->wol_irq = 0; > > > > > > > + > > > > > > > + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > > > > > > > + * --------- ----- -------- -------- ... -------- -------- > > > > > > > + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > > > > > > > + */ > > > > > > > + for (i = 0; i < CHANNEL_NUM; i++) { > > > > > > > + res->rx_irq[CHANNEL_NUM - 1 - i] = > > > > > > > + pci_irq_vector(pdev, 1 + i * 2); > > > > > > > + res->tx_irq[CHANNEL_NUM - 1 - i] = > > > > > > > + pci_irq_vector(pdev, 2 + i * 2); > > > > > > > + } > > > > > > > + > > > > > > > + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > > > > > > > + > > > > > > > + return 0; > > > > > > > +} > > > > > > > + > > > > > > > ... > > > > > > > static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > > > > > { > > > > > > > struct plat_stmmacenet_data *plat; > > > > > > > int ret, i, bus_id, phy_mode; > > > > > > > struct stmmac_pci_info *info; > > > > > > > struct stmmac_resources res; > > > > > > > + struct loongson_data *ld; > > > > > > > struct device_node *np; > > > > > > > np = dev_of_node(&pdev->dev); > > > > > > > @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > > > > > return -ENOMEM; > > > > > > > plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL); > > > > > > > - if (!plat->dma_cfg) { > > > > > > > - ret = -ENOMEM; > > > > > > > - goto err_put_node; > > > > > > > - } > > > > > > > + if (!plat->dma_cfg) > > > > > > > + return -ENOMEM; > > > > > > > + > > > > > > > + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); > > > > > > > + if (!ld) > > > > > > > + return -ENOMEM; > > > > > > > /* Enable pci device */ > > > > > > > ret = pci_enable_device(pdev); > > > > > > > @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > > > > > plat->phy_interface = phy_mode; > > > > > > > } > > > > > > > - pci_enable_msi(pdev); > > > > > > > + plat->bsp_priv = ld; > > > > > > > + plat->setup = loongson_dwmac_setup; > > > > > > > + ld->dev = &pdev->dev; > > > > > > > + > > > > > > > memset(&res, 0, sizeof(res)); > > > > > > > res.addr = pcim_iomap_table(pdev)[0]; > > > > > > > + ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; > > > > > > > + > > > > > > > + switch (ld->gmac_verion) { > > > > > > > + case LOONGSON_DWMAC_CORE_1_00: > > > > > > > + plat->rx_queues_to_use = CHANNEL_NUM; > > > > > > > + plat->tx_queues_to_use = CHANNEL_NUM; > > > > > > > + > > > > > > > + /* Only channel 0 supports checksum, > > > > > > > + * so turn off checksum to enable multiple channels. > > > > > > > + */ > > > > > > > + for (i = 1; i < CHANNEL_NUM; i++) > > > > > > > + plat->tx_queues_cfg[i].coe_unsupported = 1; > > > > > > > - plat->tx_queues_to_use = 1; > > > > > > > - plat->rx_queues_to_use = 1; > > > > > > > + ret = loongson_dwmac_config_msi(pdev, plat, &res, np); > > > > > > > + break; > > > > > > > + default: /* 0x35 device and 0x37 device. */ > > > > > > > + plat->tx_queues_to_use = 1; > > > > > > > + plat->rx_queues_to_use = 1; > > > > > > > - ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > > > > > + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > > > > > + break; > > > > > > > + } > > > > > > Let's now talk about this change. > > > > > > > > > > > > First of all, one more time. You can't miss the return value check > > > > > > because if any of the IRQ config method fails then the driver won't > > > > > > work! The first change that introduces the problem is in the patch > > > > > > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > > > > OK! > > > > > > Second, as I already mentioned in another message sent to this patch > > > > > > you are missing the PCI MSI IRQs freeing in the cleanup-on-error path > > > > > > and in the device/driver remove() function. It's definitely wrong. > > > > > You are right! I will do it. > > > > > > Thirdly, you said that the node-pointer is now optional and introduced > > > > > > the patch > > > > > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > > > > > If so and the DT-based setting up isn't mandatory then I would > > > > > > suggest to proceed with the entire so called legacy setups only if the > > > > > > node-pointer has been found, otherwise the pure PCI-based setup would > > > > > > be performed. So the patches 10-13 (in your v12 order) would look > > > > > In this case, MSI will not be enabled when the node-pointer is found. > > > > > > > > > > . > > > > > > > > > > > > > > > In fact, a large fraction of 2k devices are DT-based, of course, many are > > > > > PCI-based. > > > > Then please summarise which devices need the DT-node pointer which > > > > don't? And most importantly if they do why do they need the DT-node? > > > Whether we need DT-nodes doesn't depend on device type, but depends on > > > the BIOS type. When we boot with UEFI+ACPI, we don't need DT-node, > > > when we boot with PMON+FDT, we need DT-node. Loongson machines may > > > have either BIOS types. > > Thanks for the answer. Just to fully clarify. Does it mean that all > > Loongson Ethernet controllers (Loongson GNET and GMAC) are able to > > deliver both PCI MSI IRQs and direct GIC IRQs (so called legacy)? > > No, only devices that support multiple channels can deliver both PCI MSI > IRQs > > and direct GIC IRQs, other devices can only deliver GIC IRQs. > > Furthermore, multiple channel features are bundled with MSI. If we want to > > enable multiple channels, we must enable MSI. Sadly to say but this information changes a lot. Based on that the only platform with optional DT-node is the LS2K2000 GNET device. The rest of the devices (GMACs and LS7A2000 GNET) must be equipped with a node-pointer otherwise they won't work. Due to that the logic of the patches [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy is incorrect. 1. [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support So this patch doesn't add a pure PCI-based probe procedure after all because the Loongson GMACs are required to have a DT-node. AFAICS pdev->irq is actually the IRQ retrieved from the DT-node. So the "if (np) {} else {}" clause doesn't really make sense. 2. [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy First of all the function name is incorrect. The IRQ signal isn't legacy (INTx-based), but is retrieved from the DT-node. Secondly the "if (np) {} else {}" statement is very much redundant because if no DT-node found the pdev->irq won't be initialized at all, and the driver won't work with no error printed. All of that also affects the patch/commit logs. Glad we figured that out at this stage. Seeing there have been tons of another comments let's postpone the discussion around this problem for v13 then. I'll keep in mind the info you shared in this thread and think of the way to fix the patches after v13 is submitted for review. Thanks -Serge(y) > > Thanks, > > Yanteng >
Hi, Serge, On Mon, May 13, 2024 at 6:57 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Thu, May 09, 2024 at 04:57:44PM +0800, Yanteng Si wrote: > > Hi Serge > > > > 在 2024/5/8 23:10, Serge Semin 写道: > > > On Wed, May 08, 2024 at 10:58:16PM +0800, Huacai Chen wrote: > > > > Hi, Serge, > > > > > > > > On Wed, May 8, 2024 at 10:38 PM Serge Semin<fancer.lancer@gmail.com> wrote: > > > > > On Tue, May 07, 2024 at 09:35:24PM +0800, Yanteng Si wrote: > > > > > > Hi Serge, > > > > > > > > > > > > 在 2024/5/6 18:39, Serge Semin 写道: > > > > > > > On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote: > > > > > > > > ... > > > > > > > > +static int loongson_dwmac_config_msi(struct pci_dev *pdev, > > > > > > > > + struct plat_stmmacenet_data *plat, > > > > > > > > + struct stmmac_resources *res, > > > > > > > > + struct device_node *np) > > > > > > > > +{ > > > > > > > > + int i, ret, vecs; > > > > > > > > + > > > > > > > > + vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1); > > > > > > > > + ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI); > > > > > > > > + if (ret < 0) { > > > > > > > > + dev_info(&pdev->dev, > > > > > > > > + "MSI enable failed, Fallback to legacy interrupt\n"); > > > > > > > > + return loongson_dwmac_config_legacy(pdev, plat, res, np); > > > > > > > > + } > > > > > > > > + > > > > > > > > + res->irq = pci_irq_vector(pdev, 0); > > > > > > > > + res->wol_irq = 0; > > > > > > > > + > > > > > > > > + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > > > > > > > > + * --------- ----- -------- -------- ... -------- -------- > > > > > > > > + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > > > > > > > > + */ > > > > > > > > + for (i = 0; i < CHANNEL_NUM; i++) { > > > > > > > > + res->rx_irq[CHANNEL_NUM - 1 - i] = > > > > > > > > + pci_irq_vector(pdev, 1 + i * 2); > > > > > > > > + res->tx_irq[CHANNEL_NUM - 1 - i] = > > > > > > > > + pci_irq_vector(pdev, 2 + i * 2); > > > > > > > > + } > > > > > > > > + > > > > > > > > + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > > > > > > > > + > > > > > > > > + return 0; > > > > > > > > +} > > > > > > > > + > > > > > > > > ... > > > > > > > > static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > > > > > > { > > > > > > > > struct plat_stmmacenet_data *plat; > > > > > > > > int ret, i, bus_id, phy_mode; > > > > > > > > struct stmmac_pci_info *info; > > > > > > > > struct stmmac_resources res; > > > > > > > > + struct loongson_data *ld; > > > > > > > > struct device_node *np; > > > > > > > > np = dev_of_node(&pdev->dev); > > > > > > > > @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > > > > > > return -ENOMEM; > > > > > > > > plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL); > > > > > > > > - if (!plat->dma_cfg) { > > > > > > > > - ret = -ENOMEM; > > > > > > > > - goto err_put_node; > > > > > > > > - } > > > > > > > > + if (!plat->dma_cfg) > > > > > > > > + return -ENOMEM; > > > > > > > > + > > > > > > > > + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); > > > > > > > > + if (!ld) > > > > > > > > + return -ENOMEM; > > > > > > > > /* Enable pci device */ > > > > > > > > ret = pci_enable_device(pdev); > > > > > > > > @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > > > > > > plat->phy_interface = phy_mode; > > > > > > > > } > > > > > > > > - pci_enable_msi(pdev); > > > > > > > > + plat->bsp_priv = ld; > > > > > > > > + plat->setup = loongson_dwmac_setup; > > > > > > > > + ld->dev = &pdev->dev; > > > > > > > > + > > > > > > > > memset(&res, 0, sizeof(res)); > > > > > > > > res.addr = pcim_iomap_table(pdev)[0]; > > > > > > > > + ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; > > > > > > > > + > > > > > > > > + switch (ld->gmac_verion) { > > > > > > > > + case LOONGSON_DWMAC_CORE_1_00: > > > > > > > > + plat->rx_queues_to_use = CHANNEL_NUM; > > > > > > > > + plat->tx_queues_to_use = CHANNEL_NUM; > > > > > > > > + > > > > > > > > + /* Only channel 0 supports checksum, > > > > > > > > + * so turn off checksum to enable multiple channels. > > > > > > > > + */ > > > > > > > > + for (i = 1; i < CHANNEL_NUM; i++) > > > > > > > > + plat->tx_queues_cfg[i].coe_unsupported = 1; > > > > > > > > - plat->tx_queues_to_use = 1; > > > > > > > > - plat->rx_queues_to_use = 1; > > > > > > > > + ret = loongson_dwmac_config_msi(pdev, plat, &res, np); > > > > > > > > + break; > > > > > > > > + default: /* 0x35 device and 0x37 device. */ > > > > > > > > + plat->tx_queues_to_use = 1; > > > > > > > > + plat->rx_queues_to_use = 1; > > > > > > > > - ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > > > > > > + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > > > > > > + break; > > > > > > > > + } > > > > > > > Let's now talk about this change. > > > > > > > > > > > > > > First of all, one more time. You can't miss the return value check > > > > > > > because if any of the IRQ config method fails then the driver won't > > > > > > > work! The first change that introduces the problem is in the patch > > > > > > > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > > > > > OK! > > > > > > > Second, as I already mentioned in another message sent to this patch > > > > > > > you are missing the PCI MSI IRQs freeing in the cleanup-on-error path > > > > > > > and in the device/driver remove() function. It's definitely wrong. > > > > > > You are right! I will do it. > > > > > > > Thirdly, you said that the node-pointer is now optional and introduced > > > > > > > the patch > > > > > > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > > > > > > If so and the DT-based setting up isn't mandatory then I would > > > > > > > suggest to proceed with the entire so called legacy setups only if the > > > > > > > node-pointer has been found, otherwise the pure PCI-based setup would > > > > > > > be performed. So the patches 10-13 (in your v12 order) would look > > > > > > In this case, MSI will not be enabled when the node-pointer is found. > > > > > > > > > > > > . > > > > > > > > > > > > > > > > > > In fact, a large fraction of 2k devices are DT-based, of course, many are > > > > > > PCI-based. > > > > > Then please summarise which devices need the DT-node pointer which > > > > > don't? And most importantly if they do why do they need the DT-node? > > > > Whether we need DT-nodes doesn't depend on device type, but depends on > > > > the BIOS type. When we boot with UEFI+ACPI, we don't need DT-node, > > > > when we boot with PMON+FDT, we need DT-node. Loongson machines may > > > > have either BIOS types. > > > Thanks for the answer. Just to fully clarify. Does it mean that all > > > Loongson Ethernet controllers (Loongson GNET and GMAC) are able to > > > deliver both PCI MSI IRQs and direct GIC IRQs (so called legacy)? > > > > > No, only devices that support multiple channels can deliver both PCI MSI > > IRQs > > > > and direct GIC IRQs, other devices can only deliver GIC IRQs. > > > > Furthermore, multiple channel features are bundled with MSI. If we want to > > > > enable multiple channels, we must enable MSI. > > Sadly to say but this information changes a lot. Based on that the > only platform with optional DT-node is the LS2K2000 GNET device. The > rest of the devices (GMACs and LS7A2000 GNET) must be equipped with a > node-pointer otherwise they won't work. Due to that the logic of the > patches > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > is incorrect. > > 1. [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > So this patch doesn't add a pure PCI-based probe procedure after all > because the Loongson GMACs are required to have a DT-node. AFAICS > pdev->irq is actually the IRQ retrieved from the DT-node. So the "if > (np) {} else {}" clause doesn't really make sense. > > 2. [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > First of all the function name is incorrect. The IRQ signal isn't legacy > (INTx-based), but is retrieved from the DT-node. Secondly the > "if (np) {} else {}" statement is very much redundant because if no > DT-node found the pdev->irq won't be initialized at all, and the > driver won't work with no error printed. > > All of that also affects the patch/commit logs. Glad we figured that > out at this stage. Seeing there have been tons of another comments > let's postpone the discussion around this problem for v13 then. I'll > keep in mind the info you shared in this thread and think of the way > to fix the patches after v13 is submitted for review. Let me clarify the interrupt information, hope that can help you to understand better: 1, Loongson machines may use UEFI (implies ACPI) or PMON/UBOOT (implies FDT) as the BIOS. 2, The BIOS type has no relationship with device types, which means: machines with GMAC can be either ACPI-based or FDT-based, machines with GNET can also be either ACPI-based or FDT-based. 3, The existing Loongson driver can only support FDT, which means the device should be PCI-probed and DT-configured. Though the existing driver only supports GMAC, it doesn't mean that GMAC is bound to FDT. GMAC can also work with ACPI, in that case we say it is "full PCI", which means we don't need "np". 4, At present, multi-channel devices support MSI, currently only GNET support MSI, but in future there may also GMAC support MSI. 5, So, in Yanteng's patches, a device firstly request MSI, and since MSI is dynamically allocated, it doesn't care about the BIOS type (ACPI or FDT). However, if MSI fails (either because MSI is exhausted or the device doesn't support it), it fallback to "legacy" interrupt, which means irq lines mapped to INT-A/B/C/D of PCI. 6. In the legacy case, the irq is get from DT-node (FDT case), or already in pdev->irq (ACPI case). So Yanteng use a "if (np) { } else { }", which is reasonable from my point of view. So Yanteng's interrupt code is good for me, but I also agree to improve that after v13, if needed. Huacai > > Thanks > -Serge(y) > > > > > Thanks, > > > > Yanteng > >
On Mon, May 13, 2024 at 09:26:11PM +0800, Huacai Chen wrote: > Hi, Serge, > > On Mon, May 13, 2024 at 6:57 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > On Thu, May 09, 2024 at 04:57:44PM +0800, Yanteng Si wrote: > > > Hi Serge > > > > > > 在 2024/5/8 23:10, Serge Semin 写道: > > > > On Wed, May 08, 2024 at 10:58:16PM +0800, Huacai Chen wrote: > > > > > Hi, Serge, > > > > > > > > > > On Wed, May 8, 2024 at 10:38 PM Serge Semin<fancer.lancer@gmail.com> wrote: > > > > > > On Tue, May 07, 2024 at 09:35:24PM +0800, Yanteng Si wrote: > > > > > > > Hi Serge, > > > > > > > > > > > > > > 在 2024/5/6 18:39, Serge Semin 写道: > > > > > > > > On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote: > > > > > > > > > ... > > > > > > > > > +static int loongson_dwmac_config_msi(struct pci_dev *pdev, > > > > > > > > > + struct plat_stmmacenet_data *plat, > > > > > > > > > + struct stmmac_resources *res, > > > > > > > > > + struct device_node *np) > > > > > > > > > +{ > > > > > > > > > + int i, ret, vecs; > > > > > > > > > + > > > > > > > > > + vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1); > > > > > > > > > + ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI); > > > > > > > > > + if (ret < 0) { > > > > > > > > > + dev_info(&pdev->dev, > > > > > > > > > + "MSI enable failed, Fallback to legacy interrupt\n"); > > > > > > > > > + return loongson_dwmac_config_legacy(pdev, plat, res, np); > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + res->irq = pci_irq_vector(pdev, 0); > > > > > > > > > + res->wol_irq = 0; > > > > > > > > > + > > > > > > > > > + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > > > > > > > > > + * --------- ----- -------- -------- ... -------- -------- > > > > > > > > > + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > > > > > > > > > + */ > > > > > > > > > + for (i = 0; i < CHANNEL_NUM; i++) { > > > > > > > > > + res->rx_irq[CHANNEL_NUM - 1 - i] = > > > > > > > > > + pci_irq_vector(pdev, 1 + i * 2); > > > > > > > > > + res->tx_irq[CHANNEL_NUM - 1 - i] = > > > > > > > > > + pci_irq_vector(pdev, 2 + i * 2); > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > > > > > > > > > + > > > > > > > > > + return 0; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > ... > > > > > > > > > static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > > > > > > > { > > > > > > > > > struct plat_stmmacenet_data *plat; > > > > > > > > > int ret, i, bus_id, phy_mode; > > > > > > > > > struct stmmac_pci_info *info; > > > > > > > > > struct stmmac_resources res; > > > > > > > > > + struct loongson_data *ld; > > > > > > > > > struct device_node *np; > > > > > > > > > np = dev_of_node(&pdev->dev); > > > > > > > > > @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > > > > > > > return -ENOMEM; > > > > > > > > > plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL); > > > > > > > > > - if (!plat->dma_cfg) { > > > > > > > > > - ret = -ENOMEM; > > > > > > > > > - goto err_put_node; > > > > > > > > > - } > > > > > > > > > + if (!plat->dma_cfg) > > > > > > > > > + return -ENOMEM; > > > > > > > > > + > > > > > > > > > + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); > > > > > > > > > + if (!ld) > > > > > > > > > + return -ENOMEM; > > > > > > > > > /* Enable pci device */ > > > > > > > > > ret = pci_enable_device(pdev); > > > > > > > > > @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > > > > > > > plat->phy_interface = phy_mode; > > > > > > > > > } > > > > > > > > > - pci_enable_msi(pdev); > > > > > > > > > + plat->bsp_priv = ld; > > > > > > > > > + plat->setup = loongson_dwmac_setup; > > > > > > > > > + ld->dev = &pdev->dev; > > > > > > > > > + > > > > > > > > > memset(&res, 0, sizeof(res)); > > > > > > > > > res.addr = pcim_iomap_table(pdev)[0]; > > > > > > > > > + ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; > > > > > > > > > + > > > > > > > > > + switch (ld->gmac_verion) { > > > > > > > > > + case LOONGSON_DWMAC_CORE_1_00: > > > > > > > > > + plat->rx_queues_to_use = CHANNEL_NUM; > > > > > > > > > + plat->tx_queues_to_use = CHANNEL_NUM; > > > > > > > > > + > > > > > > > > > + /* Only channel 0 supports checksum, > > > > > > > > > + * so turn off checksum to enable multiple channels. > > > > > > > > > + */ > > > > > > > > > + for (i = 1; i < CHANNEL_NUM; i++) > > > > > > > > > + plat->tx_queues_cfg[i].coe_unsupported = 1; > > > > > > > > > - plat->tx_queues_to_use = 1; > > > > > > > > > - plat->rx_queues_to_use = 1; > > > > > > > > > + ret = loongson_dwmac_config_msi(pdev, plat, &res, np); > > > > > > > > > + break; > > > > > > > > > + default: /* 0x35 device and 0x37 device. */ > > > > > > > > > + plat->tx_queues_to_use = 1; > > > > > > > > > + plat->rx_queues_to_use = 1; > > > > > > > > > - ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > > > > > > > + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > > > > > > > + break; > > > > > > > > > + } > > > > > > > > Let's now talk about this change. > > > > > > > > > > > > > > > > First of all, one more time. You can't miss the return value check > > > > > > > > because if any of the IRQ config method fails then the driver won't > > > > > > > > work! The first change that introduces the problem is in the patch > > > > > > > > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > > > > > > OK! > > > > > > > > Second, as I already mentioned in another message sent to this patch > > > > > > > > you are missing the PCI MSI IRQs freeing in the cleanup-on-error path > > > > > > > > and in the device/driver remove() function. It's definitely wrong. > > > > > > > You are right! I will do it. > > > > > > > > Thirdly, you said that the node-pointer is now optional and introduced > > > > > > > > the patch > > > > > > > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > > > > > > > If so and the DT-based setting up isn't mandatory then I would > > > > > > > > suggest to proceed with the entire so called legacy setups only if the > > > > > > > > node-pointer has been found, otherwise the pure PCI-based setup would > > > > > > > > be performed. So the patches 10-13 (in your v12 order) would look > > > > > > > In this case, MSI will not be enabled when the node-pointer is found. > > > > > > > > > > > > > > . > > > > > > > > > > > > > > > > > > > > > In fact, a large fraction of 2k devices are DT-based, of course, many are > > > > > > > PCI-based. > > > > > > Then please summarise which devices need the DT-node pointer which > > > > > > don't? And most importantly if they do why do they need the DT-node? > > > > > Whether we need DT-nodes doesn't depend on device type, but depends on > > > > > the BIOS type. When we boot with UEFI+ACPI, we don't need DT-node, > > > > > when we boot with PMON+FDT, we need DT-node. Loongson machines may > > > > > have either BIOS types. > > > > Thanks for the answer. Just to fully clarify. Does it mean that all > > > > Loongson Ethernet controllers (Loongson GNET and GMAC) are able to > > > > deliver both PCI MSI IRQs and direct GIC IRQs (so called legacy)? > > > > > > > > No, only devices that support multiple channels can deliver both PCI MSI > > > IRQs > > > > > > and direct GIC IRQs, other devices can only deliver GIC IRQs. > > > > > > Furthermore, multiple channel features are bundled with MSI. If we want to > > > > > > enable multiple channels, we must enable MSI. > > > > Sadly to say but this information changes a lot. Based on that the > > only platform with optional DT-node is the LS2K2000 GNET device. The > > rest of the devices (GMACs and LS7A2000 GNET) must be equipped with a > > node-pointer otherwise they won't work. Due to that the logic of the > > patches > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > is incorrect. > > > > 1. [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > So this patch doesn't add a pure PCI-based probe procedure after all > > because the Loongson GMACs are required to have a DT-node. AFAICS > > pdev->irq is actually the IRQ retrieved from the DT-node. So the "if > > (np) {} else {}" clause doesn't really make sense. > > > > 2. [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > First of all the function name is incorrect. The IRQ signal isn't legacy > > (INTx-based), but is retrieved from the DT-node. Secondly the > > "if (np) {} else {}" statement is very much redundant because if no > > DT-node found the pdev->irq won't be initialized at all, and the > > driver won't work with no error printed. > > > > All of that also affects the patch/commit logs. Glad we figured that > > out at this stage. Seeing there have been tons of another comments > > let's postpone the discussion around this problem for v13 then. I'll > > keep in mind the info you shared in this thread and think of the way > > to fix the patches after v13 is submitted for review. > Let me clarify the interrupt information, hope that can help you to > understand better: > 1, Loongson machines may use UEFI (implies ACPI) or PMON/UBOOT > (implies FDT) as the BIOS. Ok. Aside with the OF-based platform there is an ACPI case. > 2, The BIOS type has no relationship with device types, which means: > machines with GMAC can be either ACPI-based or FDT-based, machines > with GNET can also be either ACPI-based or FDT-based. Ok. It's either-or. Got it. > 3, The existing Loongson driver can only support FDT, which means the > device should be PCI-probed and DT-configured. Though the existing > driver only supports GMAC, it doesn't mean that GMAC is bound to FDT. > GMAC can also work with ACPI, in that case we say it is "full PCI", > which means we don't need "np". "full PCI" statement can't be utilized for the case of the ACPI-based IRQ assignment. "full PCI" is the way the GNET probe procedure works - everything required for the device handling is detected in runtime with no ACPI/DT stuff. So the patch 10 with the "full PCI"-related subject doesn't actually adds the PCIe-only-based device probe support, but actually converts the driver to supporting the ACPI-case.) > 4, At present, multi-channel devices support MSI, currently only GNET > support MSI, but in future there may also GMAC support MSI. It's better to avoid adding a support for hypothetical devices and prohibit all the currently unreal cases. It will simplify the code, ease it' maintenance, reduce the bugs probability. > 5, So, in Yanteng's patches, a device firstly request MSI, and since > MSI is dynamically allocated, it doesn't care about the BIOS type > (ACPI or FDT). However, if MSI fails (either because MSI is exhausted > or the device doesn't support it), it fallback to "legacy" interrupt, > which means irq lines mapped to INT-A/B/C/D of PCI. Unless we are talking about the actual PCI devices (not PCI express) or the cases where the INT-x is emulated by means the specific PCIe TLPs, I wouldn't mentioned the INTx or "legacy" names in the current context. It's just a platform ACPI/DT IRQs. > 6. In the legacy case, the irq is get from DT-node (FDT case), or > already in pdev->irq (ACPI case). It will be in the pdev->irq in any case whether it's DT or ACPI. See: ACPI: pci_device_probe(): +-> arch/loongarch/pci/pci.c:pcibios_alloc_irq() DT: pci_device_probe(): +-> pci_assign_irq(); +-> pci_host_bridge::map_irq() +-> of_irq_parse_and_map_pci() or in case of Loongson PCIe host controller: +-> drivers/pci/controller/pci-loongson.c::loongson_map_irq() Moreover unless the MSI IRQs are enabled, the platform IRQ (and the legacy IRQ) can be retrieved by means of the pci_irq_vector() method. The only reason of having the direct OF-based IRQs getting in the Loongson DWMAC driver I see is that the LPI IRQ will be missing in case of the pci_irq_vector() method utilization. In the rest of the cases the pci_irq_vector() function could be freely used. > So Yanteng use a "if (np) { } else { > }", which is reasonable from my point of view. > At least one problem is there. What if pdev->irq isn't initialized (initialized with zero)?.. > So Yanteng's interrupt code is good for me, but I also agree to > improve that after v13, if needed. Ok. I've got much better picture about what is going on under the hood. Thanks. In anyway I'll get back to this topic in details in v13. -Serge(y) > > Huacai > > > > > Thanks > > -Serge(y) > > > > > > > > Thanks, > > > > > > Yanteng > > >
On Tue, May 14, 2024 at 12:11 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Mon, May 13, 2024 at 09:26:11PM +0800, Huacai Chen wrote: > > Hi, Serge, > > > > On Mon, May 13, 2024 at 6:57 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > On Thu, May 09, 2024 at 04:57:44PM +0800, Yanteng Si wrote: > > > > Hi Serge > > > > > > > > 在 2024/5/8 23:10, Serge Semin 写道: > > > > > On Wed, May 08, 2024 at 10:58:16PM +0800, Huacai Chen wrote: > > > > > > Hi, Serge, > > > > > > > > > > > > On Wed, May 8, 2024 at 10:38 PM Serge Semin<fancer.lancer@gmail.com> wrote: > > > > > > > On Tue, May 07, 2024 at 09:35:24PM +0800, Yanteng Si wrote: > > > > > > > > Hi Serge, > > > > > > > > > > > > > > > > 在 2024/5/6 18:39, Serge Semin 写道: > > > > > > > > > On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote: > > > > > > > > > > ... > > > > > > > > > > +static int loongson_dwmac_config_msi(struct pci_dev *pdev, > > > > > > > > > > + struct plat_stmmacenet_data *plat, > > > > > > > > > > + struct stmmac_resources *res, > > > > > > > > > > + struct device_node *np) > > > > > > > > > > +{ > > > > > > > > > > + int i, ret, vecs; > > > > > > > > > > + > > > > > > > > > > + vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1); > > > > > > > > > > + ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI); > > > > > > > > > > + if (ret < 0) { > > > > > > > > > > + dev_info(&pdev->dev, > > > > > > > > > > + "MSI enable failed, Fallback to legacy interrupt\n"); > > > > > > > > > > + return loongson_dwmac_config_legacy(pdev, plat, res, np); > > > > > > > > > > + } > > > > > > > > > > + > > > > > > > > > > + res->irq = pci_irq_vector(pdev, 0); > > > > > > > > > > + res->wol_irq = 0; > > > > > > > > > > + > > > > > > > > > > + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > > > > > > > > > > + * --------- ----- -------- -------- ... -------- -------- > > > > > > > > > > + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > > > > > > > > > > + */ > > > > > > > > > > + for (i = 0; i < CHANNEL_NUM; i++) { > > > > > > > > > > + res->rx_irq[CHANNEL_NUM - 1 - i] = > > > > > > > > > > + pci_irq_vector(pdev, 1 + i * 2); > > > > > > > > > > + res->tx_irq[CHANNEL_NUM - 1 - i] = > > > > > > > > > > + pci_irq_vector(pdev, 2 + i * 2); > > > > > > > > > > + } > > > > > > > > > > + > > > > > > > > > > + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > > > > > > > > > > + > > > > > > > > > > + return 0; > > > > > > > > > > +} > > > > > > > > > > + > > > > > > > > > > ... > > > > > > > > > > static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > > > > > > > > { > > > > > > > > > > struct plat_stmmacenet_data *plat; > > > > > > > > > > int ret, i, bus_id, phy_mode; > > > > > > > > > > struct stmmac_pci_info *info; > > > > > > > > > > struct stmmac_resources res; > > > > > > > > > > + struct loongson_data *ld; > > > > > > > > > > struct device_node *np; > > > > > > > > > > np = dev_of_node(&pdev->dev); > > > > > > > > > > @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > > > > > > > > return -ENOMEM; > > > > > > > > > > plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL); > > > > > > > > > > - if (!plat->dma_cfg) { > > > > > > > > > > - ret = -ENOMEM; > > > > > > > > > > - goto err_put_node; > > > > > > > > > > - } > > > > > > > > > > + if (!plat->dma_cfg) > > > > > > > > > > + return -ENOMEM; > > > > > > > > > > + > > > > > > > > > > + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); > > > > > > > > > > + if (!ld) > > > > > > > > > > + return -ENOMEM; > > > > > > > > > > /* Enable pci device */ > > > > > > > > > > ret = pci_enable_device(pdev); > > > > > > > > > > @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > > > > > > > > plat->phy_interface = phy_mode; > > > > > > > > > > } > > > > > > > > > > - pci_enable_msi(pdev); > > > > > > > > > > + plat->bsp_priv = ld; > > > > > > > > > > + plat->setup = loongson_dwmac_setup; > > > > > > > > > > + ld->dev = &pdev->dev; > > > > > > > > > > + > > > > > > > > > > memset(&res, 0, sizeof(res)); > > > > > > > > > > res.addr = pcim_iomap_table(pdev)[0]; > > > > > > > > > > + ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; > > > > > > > > > > + > > > > > > > > > > + switch (ld->gmac_verion) { > > > > > > > > > > + case LOONGSON_DWMAC_CORE_1_00: > > > > > > > > > > + plat->rx_queues_to_use = CHANNEL_NUM; > > > > > > > > > > + plat->tx_queues_to_use = CHANNEL_NUM; > > > > > > > > > > + > > > > > > > > > > + /* Only channel 0 supports checksum, > > > > > > > > > > + * so turn off checksum to enable multiple channels. > > > > > > > > > > + */ > > > > > > > > > > + for (i = 1; i < CHANNEL_NUM; i++) > > > > > > > > > > + plat->tx_queues_cfg[i].coe_unsupported = 1; > > > > > > > > > > - plat->tx_queues_to_use = 1; > > > > > > > > > > - plat->rx_queues_to_use = 1; > > > > > > > > > > + ret = loongson_dwmac_config_msi(pdev, plat, &res, np); > > > > > > > > > > + break; > > > > > > > > > > + default: /* 0x35 device and 0x37 device. */ > > > > > > > > > > + plat->tx_queues_to_use = 1; > > > > > > > > > > + plat->rx_queues_to_use = 1; > > > > > > > > > > - ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > > > > > > > > + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > > > > > > > > + break; > > > > > > > > > > + } > > > > > > > > > Let's now talk about this change. > > > > > > > > > > > > > > > > > > First of all, one more time. You can't miss the return value check > > > > > > > > > because if any of the IRQ config method fails then the driver won't > > > > > > > > > work! The first change that introduces the problem is in the patch > > > > > > > > > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > > > > > > > OK! > > > > > > > > > Second, as I already mentioned in another message sent to this patch > > > > > > > > > you are missing the PCI MSI IRQs freeing in the cleanup-on-error path > > > > > > > > > and in the device/driver remove() function. It's definitely wrong. > > > > > > > > You are right! I will do it. > > > > > > > > > Thirdly, you said that the node-pointer is now optional and introduced > > > > > > > > > the patch > > > > > > > > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > > > > > > > > If so and the DT-based setting up isn't mandatory then I would > > > > > > > > > suggest to proceed with the entire so called legacy setups only if the > > > > > > > > > node-pointer has been found, otherwise the pure PCI-based setup would > > > > > > > > > be performed. So the patches 10-13 (in your v12 order) would look > > > > > > > > In this case, MSI will not be enabled when the node-pointer is found. > > > > > > > > > > > > > > > > . > > > > > > > > > > > > > > > > > > > > > > > > In fact, a large fraction of 2k devices are DT-based, of course, many are > > > > > > > > PCI-based. > > > > > > > Then please summarise which devices need the DT-node pointer which > > > > > > > don't? And most importantly if they do why do they need the DT-node? > > > > > > Whether we need DT-nodes doesn't depend on device type, but depends on > > > > > > the BIOS type. When we boot with UEFI+ACPI, we don't need DT-node, > > > > > > when we boot with PMON+FDT, we need DT-node. Loongson machines may > > > > > > have either BIOS types. > > > > > Thanks for the answer. Just to fully clarify. Does it mean that all > > > > > Loongson Ethernet controllers (Loongson GNET and GMAC) are able to > > > > > deliver both PCI MSI IRQs and direct GIC IRQs (so called legacy)? > > > > > > > > > > > No, only devices that support multiple channels can deliver both PCI MSI > > > > IRQs > > > > > > > > and direct GIC IRQs, other devices can only deliver GIC IRQs. > > > > > > > > Furthermore, multiple channel features are bundled with MSI. If we want to > > > > > > > > enable multiple channels, we must enable MSI. > > > > > > Sadly to say but this information changes a lot. Based on that the > > > only platform with optional DT-node is the LS2K2000 GNET device. The > > > rest of the devices (GMACs and LS7A2000 GNET) must be equipped with a > > > node-pointer otherwise they won't work. Due to that the logic of the > > > patches > > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > > is incorrect. > > > > > > 1. [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > > So this patch doesn't add a pure PCI-based probe procedure after all > > > because the Loongson GMACs are required to have a DT-node. AFAICS > > > pdev->irq is actually the IRQ retrieved from the DT-node. So the "if > > > (np) {} else {}" clause doesn't really make sense. > > > > > > 2. [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > > First of all the function name is incorrect. The IRQ signal isn't legacy > > > (INTx-based), but is retrieved from the DT-node. Secondly the > > > "if (np) {} else {}" statement is very much redundant because if no > > > DT-node found the pdev->irq won't be initialized at all, and the > > > driver won't work with no error printed. > > > > > > All of that also affects the patch/commit logs. Glad we figured that > > > out at this stage. Seeing there have been tons of another comments > > > let's postpone the discussion around this problem for v13 then. I'll > > > keep in mind the info you shared in this thread and think of the way > > > to fix the patches after v13 is submitted for review. > > Let me clarify the interrupt information, hope that can help you to > > understand better: > > > 1, Loongson machines may use UEFI (implies ACPI) or PMON/UBOOT > > (implies FDT) as the BIOS. > > Ok. Aside with the OF-based platform there is an ACPI case. > > > 2, The BIOS type has no relationship with device types, which means: > > machines with GMAC can be either ACPI-based or FDT-based, machines > > with GNET can also be either ACPI-based or FDT-based. > > Ok. It's either-or. Got it. > > > 3, The existing Loongson driver can only support FDT, which means the > > device should be PCI-probed and DT-configured. Though the existing > > driver only supports GMAC, it doesn't mean that GMAC is bound to FDT. > > GMAC can also work with ACPI, in that case we say it is "full PCI", > > which means we don't need "np". > > "full PCI" statement can't be utilized for the case of the ACPI-based > IRQ assignment. "full PCI" is the way the GNET probe procedure works - > everything required for the device handling is detected in runtime > with no ACPI/DT stuff. > > So the patch 10 with the "full PCI"-related subject doesn't actually > adds the PCIe-only-based device probe support, but actually converts > the driver to supporting the ACPI-case.) Yes, the commit message can be improved. > > > 4, At present, multi-channel devices support MSI, currently only GNET > > support MSI, but in future there may also GMAC support MSI. > > It's better to avoid adding a support for hypothetical devices and > prohibit all the currently unreal cases. It will simplify the code, > ease it' maintenance, reduce the bugs probability. > > > 5, So, in Yanteng's patches, a device firstly request MSI, and since > > MSI is dynamically allocated, it doesn't care about the BIOS type > > (ACPI or FDT). However, if MSI fails (either because MSI is exhausted > > or the device doesn't support it), it fallback to "legacy" interrupt, > > which means irq lines mapped to INT-A/B/C/D of PCI. > > Unless we are talking about the actual PCI devices (not PCI express) > or the cases where the INT-x is emulated by means the specific PCIe > TLPs, I wouldn't mentioned the INTx or "legacy" names in the current > context. It's just a platform ACPI/DT IRQs. Yes, it is probably a platform ACPI/DT IRQ, but I think the "legacy" name is still reasonable in this case. Otherwise, what does "legacy" stand for in "PCI_IRQ_LEGACY/PCI_IRQ_MSI/PCI_IRQ_MSIX"? > > > 6. In the legacy case, the irq is get from DT-node (FDT case), or > > already in pdev->irq (ACPI case). > > It will be in the pdev->irq in any case whether it's DT or ACPI. See: > > ACPI: > pci_device_probe(): > +-> arch/loongarch/pci/pci.c:pcibios_alloc_irq() > > DT: > pci_device_probe(): > +-> pci_assign_irq(); > +-> pci_host_bridge::map_irq() > +-> of_irq_parse_and_map_pci() > or in case of Loongson PCIe host controller: > +-> drivers/pci/controller/pci-loongson.c::loongson_map_irq() > > Moreover unless the MSI IRQs are enabled, the platform IRQ (and the > legacy IRQ) can be retrieved by means of the pci_irq_vector() method. > The only reason of having the direct OF-based IRQs getting in the > Loongson DWMAC driver I see is that the LPI IRQ will be missing in > case of the pci_irq_vector() method utilization. In the rest of the > cases the pci_irq_vector() function could be freely used. Yes, in the DT case, they may be macirq, eth_wake_irq and eth_lpi, rather than a single irq, so we need an if-else here. > > > So Yanteng use a "if (np) { } else { > > }", which is reasonable from my point of view. > > > > At least one problem is there. What if pdev->irq isn't initialized > (initialized with zero)?.. As you said above, both ACPI and DT initialized pdev->irq, unless there is a bug in BIOS. Huacai > > > So Yanteng's interrupt code is good for me, but I also agree to > > improve that after v13, if needed. > > Ok. I've got much better picture about what is going on under the > hood. Thanks. In anyway I'll get back to this topic in details in v13. > > -Serge(y) > > > > > Huacai > > > > > > > > Thanks > > > -Serge(y) > > > > > > > > > > > Thanks, > > > > > > > > Yanteng > > > >
On Tue, May 14, 2024 at 12:58:33PM +0800, Huacai Chen wrote: > On Tue, May 14, 2024 at 12:11 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > On Mon, May 13, 2024 at 09:26:11PM +0800, Huacai Chen wrote: > > > Hi, Serge, > > > > > > On Mon, May 13, 2024 at 6:57 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > > > On Thu, May 09, 2024 at 04:57:44PM +0800, Yanteng Si wrote: > > > > > Hi Serge > > > > > ... > > > > > > > > > No, only devices that support multiple channels can deliver both PCI MSI > > > > > IRQs > > > > > > > > > > and direct GIC IRQs, other devices can only deliver GIC IRQs. > > > > > > > > > > Furthermore, multiple channel features are bundled with MSI. If we want to > > > > > > > > > > enable multiple channels, we must enable MSI. > > > > > > > > Sadly to say but this information changes a lot. Based on that the > > > > only platform with optional DT-node is the LS2K2000 GNET device. The > > > > rest of the devices (GMACs and LS7A2000 GNET) must be equipped with a > > > > node-pointer otherwise they won't work. Due to that the logic of the > > > > patches > > > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > > > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > > > is incorrect. > > > > > > > > 1. [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > > > So this patch doesn't add a pure PCI-based probe procedure after all > > > > because the Loongson GMACs are required to have a DT-node. AFAICS > > > > pdev->irq is actually the IRQ retrieved from the DT-node. So the "if > > > > (np) {} else {}" clause doesn't really make sense. > > > > > > > > 2. [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > > > First of all the function name is incorrect. The IRQ signal isn't legacy > > > > (INTx-based), but is retrieved from the DT-node. Secondly the > > > > "if (np) {} else {}" statement is very much redundant because if no > > > > DT-node found the pdev->irq won't be initialized at all, and the > > > > driver won't work with no error printed. > > > > > > > > All of that also affects the patch/commit logs. Glad we figured that > > > > out at this stage. Seeing there have been tons of another comments > > > > let's postpone the discussion around this problem for v13 then. I'll > > > > keep in mind the info you shared in this thread and think of the way > > > > to fix the patches after v13 is submitted for review. > > > Let me clarify the interrupt information, hope that can help you to > > > understand better: > > > > > 1, Loongson machines may use UEFI (implies ACPI) or PMON/UBOOT > > > (implies FDT) as the BIOS. > > > > Ok. Aside with the OF-based platform there is an ACPI case. > > > > > 2, The BIOS type has no relationship with device types, which means: > > > machines with GMAC can be either ACPI-based or FDT-based, machines > > > with GNET can also be either ACPI-based or FDT-based. > > > > Ok. It's either-or. Got it. > > > > > 3, The existing Loongson driver can only support FDT, which means the > > > device should be PCI-probed and DT-configured. Though the existing > > > driver only supports GMAC, it doesn't mean that GMAC is bound to FDT. > > > GMAC can also work with ACPI, in that case we say it is "full PCI", > > > which means we don't need "np". > > > > "full PCI" statement can't be utilized for the case of the ACPI-based > > IRQ assignment. "full PCI" is the way the GNET probe procedure works - > > everything required for the device handling is detected in runtime > > with no ACPI/DT stuff. > > > > So the patch 10 with the "full PCI"-related subject doesn't actually > > adds the PCIe-only-based device probe support, but actually converts > > the driver to supporting the ACPI-case.) > Yes, the commit message can be improved. Can be? It must be changed, because at the very least it's misleading, but frankly speaking it now sounds just wrong. > > > > > > 4, At present, multi-channel devices support MSI, currently only GNET > > > support MSI, but in future there may also GMAC support MSI. > > > > It's better to avoid adding a support for hypothetical devices and > > prohibit all the currently unreal cases. It will simplify the code, > > ease it' maintenance, reduce the bugs probability. > > > > > 5, So, in Yanteng's patches, a device firstly request MSI, and since > > > MSI is dynamically allocated, it doesn't care about the BIOS type > > > (ACPI or FDT). However, if MSI fails (either because MSI is exhausted > > > or the device doesn't support it), it fallback to "legacy" interrupt, > > > which means irq lines mapped to INT-A/B/C/D of PCI. > > > > Unless we are talking about the actual PCI devices (not PCI express) > > or the cases where the INT-x is emulated by means the specific PCIe > > TLPs, I wouldn't mentioned the INTx or "legacy" names in the current > > context. It's just a platform ACPI/DT IRQs. > Yes, it is probably a platform ACPI/DT IRQ, but I think the "legacy" > name is still reasonable in this case. Probably? These _are_ pure platform IRQs. > Otherwise, what does "legacy" > stand for in "PCI_IRQ_LEGACY/PCI_IRQ_MSI/PCI_IRQ_MSIX"? It means that the platform IRQs has just been implemented via the already available old-school API, which has been in the kernel since the plain PCI devices. The platform IRQs and the traditional PCI INTx are normally mutually exclusive, so I guess that's why they have been implemented in framework of the same interface. Another reason could be to have less troubles with adopting the PCI drivers for both type of the IRQs delivery. Moreover just recently the so called _legacy_ flag name has been deprecated in favor of the more generic INTx one: https://lore.kernel.org/linux-pci/20231122060406.14695-1-dlemoal@kernel.org/ Once again about the naming. From the retrospective point of view the so called legacy PCI IRQs (in fact PCI INTx) and the platform IRQs look similar because these are just the level-type signals connected to the system IRQ controller. But when it comes to the PCI _Express_, the implementation is completely different. The PCIe INTx is just the PCIe TLPs of special type, like MSI. Upon receiving these special messages the PCIe host controller delivers the IRQ up to the respective system IRQ controller. So in order to avoid the confusion between the actual legacy PCI INTx, PCI Express INTx and the just platform IRQs, it's better to emphasize the actual way of the IRQs delivery. In this case it's the later method. > > > > > > 6. In the legacy case, the irq is get from DT-node (FDT case), or > > > already in pdev->irq (ACPI case). > > > > It will be in the pdev->irq in any case whether it's DT or ACPI. See: > > > > ACPI: > > pci_device_probe(): > > +-> arch/loongarch/pci/pci.c:pcibios_alloc_irq() > > > > DT: > > pci_device_probe(): > > +-> pci_assign_irq(); > > +-> pci_host_bridge::map_irq() > > +-> of_irq_parse_and_map_pci() > > or in case of Loongson PCIe host controller: > > +-> drivers/pci/controller/pci-loongson.c::loongson_map_irq() > > > > Moreover unless the MSI IRQs are enabled, the platform IRQ (and the > > legacy IRQ) can be retrieved by means of the pci_irq_vector() method. > > The only reason of having the direct OF-based IRQs getting in the > > Loongson DWMAC driver I see is that the LPI IRQ will be missing in > > case of the pci_irq_vector() method utilization. In the rest of the > > cases the pci_irq_vector() function could be freely used. > Yes, in the DT case, they may be macirq, eth_wake_irq and eth_lpi, > rather than a single irq, so we need an if-else here. > > > > > > So Yanteng use a "if (np) { } else { > > > }", which is reasonable from my point of view. > > > > > > > At least one problem is there. What if pdev->irq isn't initialized > > (initialized with zero)?.. > As you said above, both ACPI and DT initialized pdev->irq, unless > there is a bug in BIOS. I meant that based on the platform firmware nature the pdev->irq field shall be initialized with an IRQ number in accordance with the DT or ACPI logic. I never said it was impossible to have the field uninitialized (that is being left zero). It's absolutely possible. There are much more reasons to have that than just a firmware bug. On the top of my mind: MSI being enabled, kernel misconfiguration, kernel bug, DT/ACPI lacking the IRQ property, ... -Serge(y) > > > Huacai > > > > > > So Yanteng's interrupt code is good for me, but I also agree to > > > improve that after v13, if needed. > > > > Ok. I've got much better picture about what is going on under the > > hood. Thanks. In anyway I'll get back to this topic in details in v13. > > > > -Serge(y) > > > > > > > > Huacai > > > > > > > > > > > Thanks > > > > -Serge(y) > > > > > > > > > > > > > > Thanks, > > > > > > > > > > Yanteng > > > > >
Hi, Serge, On Tue, May 14, 2024 at 7:33 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Tue, May 14, 2024 at 12:58:33PM +0800, Huacai Chen wrote: > > On Tue, May 14, 2024 at 12:11 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > On Mon, May 13, 2024 at 09:26:11PM +0800, Huacai Chen wrote: > > > > Hi, Serge, > > > > > > > > On Mon, May 13, 2024 at 6:57 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > > > > > On Thu, May 09, 2024 at 04:57:44PM +0800, Yanteng Si wrote: > > > > > > Hi Serge > > > > > > > ... > > > > > > > > > > > No, only devices that support multiple channels can deliver both PCI MSI > > > > > > IRQs > > > > > > > > > > > > and direct GIC IRQs, other devices can only deliver GIC IRQs. > > > > > > > > > > > > Furthermore, multiple channel features are bundled with MSI. If we want to > > > > > > > > > > > > enable multiple channels, we must enable MSI. > > > > > > > > > > Sadly to say but this information changes a lot. Based on that the > > > > > only platform with optional DT-node is the LS2K2000 GNET device. The > > > > > rest of the devices (GMACs and LS7A2000 GNET) must be equipped with a > > > > > node-pointer otherwise they won't work. Due to that the logic of the > > > > > patches > > > > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > > > > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > > > > is incorrect. > > > > > > > > > > 1. [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > > > > So this patch doesn't add a pure PCI-based probe procedure after all > > > > > because the Loongson GMACs are required to have a DT-node. AFAICS > > > > > pdev->irq is actually the IRQ retrieved from the DT-node. So the "if > > > > > (np) {} else {}" clause doesn't really make sense. > > > > > > > > > > 2. [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > > > > First of all the function name is incorrect. The IRQ signal isn't legacy > > > > > (INTx-based), but is retrieved from the DT-node. Secondly the > > > > > "if (np) {} else {}" statement is very much redundant because if no > > > > > DT-node found the pdev->irq won't be initialized at all, and the > > > > > driver won't work with no error printed. > > > > > > > > > > All of that also affects the patch/commit logs. Glad we figured that > > > > > out at this stage. Seeing there have been tons of another comments > > > > > let's postpone the discussion around this problem for v13 then. I'll > > > > > keep in mind the info you shared in this thread and think of the way > > > > > to fix the patches after v13 is submitted for review. > > > > Let me clarify the interrupt information, hope that can help you to > > > > understand better: > > > > > > > 1, Loongson machines may use UEFI (implies ACPI) or PMON/UBOOT > > > > (implies FDT) as the BIOS. > > > > > > Ok. Aside with the OF-based platform there is an ACPI case. > > > > > > > 2, The BIOS type has no relationship with device types, which means: > > > > machines with GMAC can be either ACPI-based or FDT-based, machines > > > > with GNET can also be either ACPI-based or FDT-based. > > > > > > Ok. It's either-or. Got it. > > > > > > > 3, The existing Loongson driver can only support FDT, which means the > > > > device should be PCI-probed and DT-configured. Though the existing > > > > driver only supports GMAC, it doesn't mean that GMAC is bound to FDT. > > > > GMAC can also work with ACPI, in that case we say it is "full PCI", > > > > which means we don't need "np". > > > > > > "full PCI" statement can't be utilized for the case of the ACPI-based > > > IRQ assignment. "full PCI" is the way the GNET probe procedure works - > > > everything required for the device handling is detected in runtime > > > with no ACPI/DT stuff. > > > > > > So the patch 10 with the "full PCI"-related subject doesn't actually > > > adds the PCIe-only-based device probe support, but actually converts > > > the driver to supporting the ACPI-case.) > > > Yes, the commit message can be improved. > > Can be? It must be changed, because at the very least it's misleading, > but frankly speaking it now sounds just wrong. Sit back and relax. :) I agree with your opinion, but we don't need to so abolute, yes? > > > > > > > > > > 4, At present, multi-channel devices support MSI, currently only GNET > > > > support MSI, but in future there may also GMAC support MSI. > > > > > > It's better to avoid adding a support for hypothetical devices and > > > prohibit all the currently unreal cases. It will simplify the code, > > > ease it' maintenance, reduce the bugs probability. > > > > > > > 5, So, in Yanteng's patches, a device firstly request MSI, and since > > > > MSI is dynamically allocated, it doesn't care about the BIOS type > > > > (ACPI or FDT). However, if MSI fails (either because MSI is exhausted > > > > or the device doesn't support it), it fallback to "legacy" interrupt, > > > > which means irq lines mapped to INT-A/B/C/D of PCI. > > > > > > Unless we are talking about the actual PCI devices (not PCI express) > > > or the cases where the INT-x is emulated by means the specific PCIe > > > TLPs, I wouldn't mentioned the INTx or "legacy" names in the current > > > context. It's just a platform ACPI/DT IRQs. > > > Yes, it is probably a platform ACPI/DT IRQ, but I think the "legacy" > > name is still reasonable in this case. > > Probably? These _are_ pure platform IRQs. > > > Otherwise, what does "legacy" > > stand for in "PCI_IRQ_LEGACY/PCI_IRQ_MSI/PCI_IRQ_MSIX"? > > It means that the platform IRQs has just been implemented via the > already available old-school API, which has been in the kernel since > the plain PCI devices. The platform IRQs and the traditional PCI INTx > are normally mutually exclusive, so I guess that's why they have been > implemented in framework of the same interface. Another reason could > be to have less troubles with adopting the PCI drivers for both type > of the IRQs delivery. > > Moreover just recently the so called _legacy_ flag name has been > deprecated in favor of the more generic INTx one: > https://lore.kernel.org/linux-pci/20231122060406.14695-1-dlemoal@kernel.org/ This info is important, but your last suggestion for Yanteng still use PCI_IRQ_LEGACY. :) > > Once again about the naming. From the retrospective point of view the > so called legacy PCI IRQs (in fact PCI INTx) and the platform IRQs > look similar because these are just the level-type signals connected > to the system IRQ controller. But when it comes to the PCI _Express_, > the implementation is completely different. The PCIe INTx is just the > PCIe TLPs of special type, like MSI. Upon receiving these special > messages the PCIe host controller delivers the IRQ up to the > respective system IRQ controller. So in order to avoid the confusion > between the actual legacy PCI INTx, PCI Express INTx and the just > platform IRQs, it's better to emphasize the actual way of the IRQs > delivery. In this case it's the later method. You are absolutely right, and I think I found a method to use your framework to solve our problems: static int loongson_dwmac_config_irqs(struct pci_dev *pdev, struct plat_stmmacenet_data *plat, struct stmmac_resources *res) { int i, ret, vecs; /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | * --------- ----- -------- -------- ... -------- -------- * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | */ vecs = plat->rx_queues_to_use + plat->tx_queues_to_use + 1; ret = pci_alloc_irq_vectors(pdev, 1, vecs, PCI_IRQ_MSI | PCI_IRQ_INTX); if (ret < 0) { dev_err(&pdev->dev, "Failed to allocate PCI IRQs\n"); return ret; } if (ret >= vecs) { for (i = 0; i < plat->rx_queues_to_use; i++) { res->rx_irq[CHANNELS_NUM - 1 - i] = pci_irq_vector(pdev, 1 + i * 2); } for (i = 0; i < plat->tx_queues_to_use; i++) { res->tx_irq[CHANNELS_NUM - 1 - i] = pci_irq_vector(pdev, 2 + i * 2); } plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; } res->irq = pci_irq_vector(pdev, 0); if (np) { res->irq = of_irq_get_byname(np, "macirq"); if (res->irq < 0) { dev_err(&pdev->dev, "IRQ macirq not found\n"); return -ENODEV; } res->wol_irq = of_irq_get_byname(np, "eth_wake_irq"); if (res->wol_irq < 0) { dev_info(&pdev->dev, "IRQ eth_wake_irq not found, using macirq\n"); res->wol_irq = res->irq; } res->lpi_irq = of_irq_get_byname(np, "eth_lpi"); if (res->lpi_irq < 0) { dev_err(&pdev->dev, "IRQ eth_lpi not found\n"); return -ENODEV; } } return 0; } If your agree, Yanteng can use this method in V13, then avoid furthur changes. Huacai > > > > > > > > > > 6. In the legacy case, the irq is get from DT-node (FDT case), or > > > > already in pdev->irq (ACPI case). > > > > > > It will be in the pdev->irq in any case whether it's DT or ACPI. See: > > > > > > ACPI: > > > pci_device_probe(): > > > +-> arch/loongarch/pci/pci.c:pcibios_alloc_irq() > > > > > > DT: > > > pci_device_probe(): > > > +-> pci_assign_irq(); > > > +-> pci_host_bridge::map_irq() > > > +-> of_irq_parse_and_map_pci() > > > or in case of Loongson PCIe host controller: > > > +-> drivers/pci/controller/pci-loongson.c::loongson_map_irq() > > > > > > Moreover unless the MSI IRQs are enabled, the platform IRQ (and the > > > legacy IRQ) can be retrieved by means of the pci_irq_vector() method. > > > The only reason of having the direct OF-based IRQs getting in the > > > Loongson DWMAC driver I see is that the LPI IRQ will be missing in > > > case of the pci_irq_vector() method utilization. In the rest of the > > > cases the pci_irq_vector() function could be freely used. > > Yes, in the DT case, they may be macirq, eth_wake_irq and eth_lpi, > > rather than a single irq, so we need an if-else here. > > > > > > > > > So Yanteng use a "if (np) { } else { > > > > }", which is reasonable from my point of view. > > > > > > > > > > At least one problem is there. What if pdev->irq isn't initialized > > > (initialized with zero)?.. > > > As you said above, both ACPI and DT initialized pdev->irq, unless > > there is a bug in BIOS. > > I meant that based on the platform firmware nature the pdev->irq field > shall be initialized with an IRQ number in accordance with the DT or > ACPI logic. I never said it was impossible to have the field > uninitialized (that is being left zero). It's absolutely possible. > There are much more reasons to have that than just a firmware bug. On > the top of my mind: MSI being enabled, kernel misconfiguration, kernel > bug, DT/ACPI lacking the IRQ property, ... > > -Serge(y) > > > > > > > Huacai > > > > > > > > > So Yanteng's interrupt code is good for me, but I also agree to > > > > improve that after v13, if needed. > > > > > > Ok. I've got much better picture about what is going on under the > > > hood. Thanks. In anyway I'll get back to this topic in details in v13. > > > > > > -Serge(y) > > > > > > > > > > > Huacai > > > > > > > > > > > > > > Thanks > > > > > -Serge(y) > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > Yanteng > > > > > >
On Tue, May 14, 2024 at 08:53:46PM +0800, Huacai Chen wrote: > Hi, Serge, > > On Tue, May 14, 2024 at 7:33 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > On Tue, May 14, 2024 at 12:58:33PM +0800, Huacai Chen wrote: > > > On Tue, May 14, 2024 at 12:11 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > > > On Mon, May 13, 2024 at 09:26:11PM +0800, Huacai Chen wrote: > > > > > Hi, Serge, > > > > > > > > > > On Mon, May 13, 2024 at 6:57 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > > > > > > > On Thu, May 09, 2024 at 04:57:44PM +0800, Yanteng Si wrote: > > > > > > > Hi Serge > > > > > > > > > ... > > > > > > > > > > > > > No, only devices that support multiple channels can deliver both PCI MSI > > > > > > > IRQs > > > > > > > > > > > > > > and direct GIC IRQs, other devices can only deliver GIC IRQs. > > > > > > > > > > > > > > Furthermore, multiple channel features are bundled with MSI. If we want to > > > > > > > > > > > > > > enable multiple channels, we must enable MSI. > > > > > > > > > > > > Sadly to say but this information changes a lot. Based on that the > > > > > > only platform with optional DT-node is the LS2K2000 GNET device. The > > > > > > rest of the devices (GMACs and LS7A2000 GNET) must be equipped with a > > > > > > node-pointer otherwise they won't work. Due to that the logic of the > > > > > > patches > > > > > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > > > > > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > > > > > is incorrect. > > > > > > > > > > > > 1. [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > > > > > So this patch doesn't add a pure PCI-based probe procedure after all > > > > > > because the Loongson GMACs are required to have a DT-node. AFAICS > > > > > > pdev->irq is actually the IRQ retrieved from the DT-node. So the "if > > > > > > (np) {} else {}" clause doesn't really make sense. > > > > > > > > > > > > 2. [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > > > > > First of all the function name is incorrect. The IRQ signal isn't legacy > > > > > > (INTx-based), but is retrieved from the DT-node. Secondly the > > > > > > "if (np) {} else {}" statement is very much redundant because if no > > > > > > DT-node found the pdev->irq won't be initialized at all, and the > > > > > > driver won't work with no error printed. > > > > > > > > > > > > All of that also affects the patch/commit logs. Glad we figured that > > > > > > out at this stage. Seeing there have been tons of another comments > > > > > > let's postpone the discussion around this problem for v13 then. I'll > > > > > > keep in mind the info you shared in this thread and think of the way > > > > > > to fix the patches after v13 is submitted for review. > > > > > Let me clarify the interrupt information, hope that can help you to > > > > > understand better: > > > > > > > > > 1, Loongson machines may use UEFI (implies ACPI) or PMON/UBOOT > > > > > (implies FDT) as the BIOS. > > > > > > > > Ok. Aside with the OF-based platform there is an ACPI case. > > > > > > > > > 2, The BIOS type has no relationship with device types, which means: > > > > > machines with GMAC can be either ACPI-based or FDT-based, machines > > > > > with GNET can also be either ACPI-based or FDT-based. > > > > > > > > Ok. It's either-or. Got it. > > > > > > > > > 3, The existing Loongson driver can only support FDT, which means the > > > > > device should be PCI-probed and DT-configured. Though the existing > > > > > driver only supports GMAC, it doesn't mean that GMAC is bound to FDT. > > > > > GMAC can also work with ACPI, in that case we say it is "full PCI", > > > > > which means we don't need "np". > > > > > > > > "full PCI" statement can't be utilized for the case of the ACPI-based > > > > IRQ assignment. "full PCI" is the way the GNET probe procedure works - > > > > everything required for the device handling is detected in runtime > > > > with no ACPI/DT stuff. > > > > > > > > So the patch 10 with the "full PCI"-related subject doesn't actually > > > > adds the PCIe-only-based device probe support, but actually converts > > > > the driver to supporting the ACPI-case.) > > > > > Yes, the commit message can be improved. > > > > Can be? It must be changed, because at the very least it's misleading, > > but frankly speaking it now sounds just wrong. > Sit back and relax. :) The smiley enclosing your epigram doesn't make it appropriate. Please refrain from familiarity in our future discussions. > I agree with your opinion, but we don't need to so abolute, yes? > > > > > > > > > > > > > > > 4, At present, multi-channel devices support MSI, currently only GNET > > > > > support MSI, but in future there may also GMAC support MSI. > > > > > > > > It's better to avoid adding a support for hypothetical devices and > > > > prohibit all the currently unreal cases. It will simplify the code, > > > > ease it' maintenance, reduce the bugs probability. > > > > > > > > > 5, So, in Yanteng's patches, a device firstly request MSI, and since > > > > > MSI is dynamically allocated, it doesn't care about the BIOS type > > > > > (ACPI or FDT). However, if MSI fails (either because MSI is exhausted > > > > > or the device doesn't support it), it fallback to "legacy" interrupt, > > > > > which means irq lines mapped to INT-A/B/C/D of PCI. > > > > > > > > Unless we are talking about the actual PCI devices (not PCI express) > > > > or the cases where the INT-x is emulated by means the specific PCIe > > > > TLPs, I wouldn't mentioned the INTx or "legacy" names in the current > > > > context. It's just a platform ACPI/DT IRQs. > > > > > Yes, it is probably a platform ACPI/DT IRQ, but I think the "legacy" > > > name is still reasonable in this case. > > > > Probably? These _are_ pure platform IRQs. > > > > > Otherwise, what does "legacy" > > > stand for in "PCI_IRQ_LEGACY/PCI_IRQ_MSI/PCI_IRQ_MSIX"? > > > > It means that the platform IRQs has just been implemented via the > > already available old-school API, which has been in the kernel since > > the plain PCI devices. The platform IRQs and the traditional PCI INTx > > are normally mutually exclusive, so I guess that's why they have been > > implemented in framework of the same interface. Another reason could > > be to have less troubles with adopting the PCI drivers for both type > > of the IRQs delivery. > > > > Moreover just recently the so called _legacy_ flag name has been > > deprecated in favor of the more generic INTx one: > > https://lore.kernel.org/linux-pci/20231122060406.14695-1-dlemoal@kernel.org/ > This info is important, but your last suggestion for Yanteng still use > PCI_IRQ_LEGACY. :) Yes, my mistake. It should be replaced with PCI_IRQ_INTX. > > > > > Once again about the naming. From the retrospective point of view the > > so called legacy PCI IRQs (in fact PCI INTx) and the platform IRQs > > look similar because these are just the level-type signals connected > > to the system IRQ controller. But when it comes to the PCI _Express_, > > the implementation is completely different. The PCIe INTx is just the > > PCIe TLPs of special type, like MSI. Upon receiving these special > > messages the PCIe host controller delivers the IRQ up to the > > respective system IRQ controller. So in order to avoid the confusion > > between the actual legacy PCI INTx, PCI Express INTx and the just > > platform IRQs, it's better to emphasize the actual way of the IRQs > > delivery. In this case it's the later method. > You are absolutely right, and I think I found a method to use your > framework to solve our problems: > > static int loongson_dwmac_config_irqs(struct pci_dev *pdev, > struct plat_stmmacenet_data *plat, > struct stmmac_resources *res) > { > int i, ret, vecs; > > /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > * --------- ----- -------- -------- ... -------- -------- > * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > */ > vecs = plat->rx_queues_to_use + plat->tx_queues_to_use + 1; > ret = pci_alloc_irq_vectors(pdev, 1, vecs, PCI_IRQ_MSI | PCI_IRQ_INTX); > if (ret < 0) { > dev_err(&pdev->dev, "Failed to allocate PCI IRQs\n"); > return ret; > } > if (ret >= vecs) { > for (i = 0; i < plat->rx_queues_to_use; i++) { > res->rx_irq[CHANNELS_NUM - 1 - i] = > pci_irq_vector(pdev, 1 + i * 2); > } > for (i = 0; i < plat->tx_queues_to_use; i++) { > res->tx_irq[CHANNELS_NUM - 1 - i] = > pci_irq_vector(pdev, 2 + i * 2); > } > > plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > } > > res->irq = pci_irq_vector(pdev, 0); > > if (np) { > res->irq = of_irq_get_byname(np, "macirq"); > if (res->irq < 0) { > dev_err(&pdev->dev, "IRQ macirq not found\n"); > return -ENODEV; > } > > res->wol_irq = of_irq_get_byname(np, "eth_wake_irq"); > if (res->wol_irq < 0) { > dev_info(&pdev->dev, > "IRQ eth_wake_irq not found, using macirq\n"); > res->wol_irq = res->irq; > } > > res->lpi_irq = of_irq_get_byname(np, "eth_lpi"); > if (res->lpi_irq < 0) { > dev_err(&pdev->dev, "IRQ eth_lpi not found\n"); > return -ENODEV; > } > } > return 0; > } > > If your agree, Yanteng can use this method in V13, then avoid furthur changes. Since yesterday I have been too relaxed sitting back to explain in detail the problems with the code above. Shortly speaking, no to the method designed as above. -Serge(y) > > Huacai > > > > > > > > > > > > > > > 6. In the legacy case, the irq is get from DT-node (FDT case), or > > > > > already in pdev->irq (ACPI case). > > > > > > > > It will be in the pdev->irq in any case whether it's DT or ACPI. See: > > > > > > > > ACPI: > > > > pci_device_probe(): > > > > +-> arch/loongarch/pci/pci.c:pcibios_alloc_irq() > > > > > > > > DT: > > > > pci_device_probe(): > > > > +-> pci_assign_irq(); > > > > +-> pci_host_bridge::map_irq() > > > > +-> of_irq_parse_and_map_pci() > > > > or in case of Loongson PCIe host controller: > > > > +-> drivers/pci/controller/pci-loongson.c::loongson_map_irq() > > > > > > > > Moreover unless the MSI IRQs are enabled, the platform IRQ (and the > > > > legacy IRQ) can be retrieved by means of the pci_irq_vector() method. > > > > The only reason of having the direct OF-based IRQs getting in the > > > > Loongson DWMAC driver I see is that the LPI IRQ will be missing in > > > > case of the pci_irq_vector() method utilization. In the rest of the > > > > cases the pci_irq_vector() function could be freely used. > > > Yes, in the DT case, they may be macirq, eth_wake_irq and eth_lpi, > > > rather than a single irq, so we need an if-else here. > > > > > > > > > > > > So Yanteng use a "if (np) { } else { > > > > > }", which is reasonable from my point of view. > > > > > > > > > > > > > At least one problem is there. What if pdev->irq isn't initialized > > > > (initialized with zero)?.. > > > > > As you said above, both ACPI and DT initialized pdev->irq, unless > > > there is a bug in BIOS. > > > > I meant that based on the platform firmware nature the pdev->irq field > > shall be initialized with an IRQ number in accordance with the DT or > > ACPI logic. I never said it was impossible to have the field > > uninitialized (that is being left zero). It's absolutely possible. > > There are much more reasons to have that than just a firmware bug. On > > the top of my mind: MSI being enabled, kernel misconfiguration, kernel > > bug, DT/ACPI lacking the IRQ property, ... > > > > -Serge(y) > > > > > > > > > > > Huacai > > > > > > > > > > > > So Yanteng's interrupt code is good for me, but I also agree to > > > > > improve that after v13, if needed. > > > > > > > > Ok. I've got much better picture about what is going on under the > > > > hood. Thanks. In anyway I'll get back to this topic in details in v13. > > > > > > > > -Serge(y) > > > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > Thanks > > > > > > -Serge(y) > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > Yanteng > > > > > > >
On Wed, May 15, 2024 at 4:40 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Tue, May 14, 2024 at 08:53:46PM +0800, Huacai Chen wrote: > > Hi, Serge, > > > > On Tue, May 14, 2024 at 7:33 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > On Tue, May 14, 2024 at 12:58:33PM +0800, Huacai Chen wrote: > > > > On Tue, May 14, 2024 at 12:11 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > > > > > On Mon, May 13, 2024 at 09:26:11PM +0800, Huacai Chen wrote: > > > > > > Hi, Serge, > > > > > > > > > > > > On Mon, May 13, 2024 at 6:57 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > > > > > > > > > On Thu, May 09, 2024 at 04:57:44PM +0800, Yanteng Si wrote: > > > > > > > > Hi Serge > > > > > > > > > > > ... > > > > > > > > > > > > > > > No, only devices that support multiple channels can deliver both PCI MSI > > > > > > > > IRQs > > > > > > > > > > > > > > > > and direct GIC IRQs, other devices can only deliver GIC IRQs. > > > > > > > > > > > > > > > > Furthermore, multiple channel features are bundled with MSI. If we want to > > > > > > > > > > > > > > > > enable multiple channels, we must enable MSI. > > > > > > > > > > > > > > Sadly to say but this information changes a lot. Based on that the > > > > > > > only platform with optional DT-node is the LS2K2000 GNET device. The > > > > > > > rest of the devices (GMACs and LS7A2000 GNET) must be equipped with a > > > > > > > node-pointer otherwise they won't work. Due to that the logic of the > > > > > > > patches > > > > > > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > > > > > > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > > > > > > is incorrect. > > > > > > > > > > > > > > 1. [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > > > > > > So this patch doesn't add a pure PCI-based probe procedure after all > > > > > > > because the Loongson GMACs are required to have a DT-node. AFAICS > > > > > > > pdev->irq is actually the IRQ retrieved from the DT-node. So the "if > > > > > > > (np) {} else {}" clause doesn't really make sense. > > > > > > > > > > > > > > 2. [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy > > > > > > > First of all the function name is incorrect. The IRQ signal isn't legacy > > > > > > > (INTx-based), but is retrieved from the DT-node. Secondly the > > > > > > > "if (np) {} else {}" statement is very much redundant because if no > > > > > > > DT-node found the pdev->irq won't be initialized at all, and the > > > > > > > driver won't work with no error printed. > > > > > > > > > > > > > > All of that also affects the patch/commit logs. Glad we figured that > > > > > > > out at this stage. Seeing there have been tons of another comments > > > > > > > let's postpone the discussion around this problem for v13 then. I'll > > > > > > > keep in mind the info you shared in this thread and think of the way > > > > > > > to fix the patches after v13 is submitted for review. > > > > > > Let me clarify the interrupt information, hope that can help you to > > > > > > understand better: > > > > > > > > > > > 1, Loongson machines may use UEFI (implies ACPI) or PMON/UBOOT > > > > > > (implies FDT) as the BIOS. > > > > > > > > > > Ok. Aside with the OF-based platform there is an ACPI case. > > > > > > > > > > > 2, The BIOS type has no relationship with device types, which means: > > > > > > machines with GMAC can be either ACPI-based or FDT-based, machines > > > > > > with GNET can also be either ACPI-based or FDT-based. > > > > > > > > > > Ok. It's either-or. Got it. > > > > > > > > > > > 3, The existing Loongson driver can only support FDT, which means the > > > > > > device should be PCI-probed and DT-configured. Though the existing > > > > > > driver only supports GMAC, it doesn't mean that GMAC is bound to FDT. > > > > > > GMAC can also work with ACPI, in that case we say it is "full PCI", > > > > > > which means we don't need "np". > > > > > > > > > > "full PCI" statement can't be utilized for the case of the ACPI-based > > > > > IRQ assignment. "full PCI" is the way the GNET probe procedure works - > > > > > everything required for the device handling is detected in runtime > > > > > with no ACPI/DT stuff. > > > > > > > > > > So the patch 10 with the "full PCI"-related subject doesn't actually > > > > > adds the PCIe-only-based device probe support, but actually converts > > > > > the driver to supporting the ACPI-case.) > > > > > > > Yes, the commit message can be improved. > > > > > > Can be? It must be changed, because at the very least it's misleading, > > > but frankly speaking it now sounds just wrong. > > > Sit back and relax. :) > > The smiley enclosing your epigram doesn't make it appropriate. Please > refrain from familiarity in our future discussions. > > > I agree with your opinion, but we don't need to so abolute, yes? > > > > > > > > > > > > > > > > > > > > 4, At present, multi-channel devices support MSI, currently only GNET > > > > > > support MSI, but in future there may also GMAC support MSI. > > > > > > > > > > It's better to avoid adding a support for hypothetical devices and > > > > > prohibit all the currently unreal cases. It will simplify the code, > > > > > ease it' maintenance, reduce the bugs probability. > > > > > > > > > > > 5, So, in Yanteng's patches, a device firstly request MSI, and since > > > > > > MSI is dynamically allocated, it doesn't care about the BIOS type > > > > > > (ACPI or FDT). However, if MSI fails (either because MSI is exhausted > > > > > > or the device doesn't support it), it fallback to "legacy" interrupt, > > > > > > which means irq lines mapped to INT-A/B/C/D of PCI. > > > > > > > > > > Unless we are talking about the actual PCI devices (not PCI express) > > > > > or the cases where the INT-x is emulated by means the specific PCIe > > > > > TLPs, I wouldn't mentioned the INTx or "legacy" names in the current > > > > > context. It's just a platform ACPI/DT IRQs. > > > > > > > Yes, it is probably a platform ACPI/DT IRQ, but I think the "legacy" > > > > name is still reasonable in this case. > > > > > > Probably? These _are_ pure platform IRQs. > > > > > > > Otherwise, what does "legacy" > > > > stand for in "PCI_IRQ_LEGACY/PCI_IRQ_MSI/PCI_IRQ_MSIX"? > > > > > > It means that the platform IRQs has just been implemented via the > > > already available old-school API, which has been in the kernel since > > > the plain PCI devices. The platform IRQs and the traditional PCI INTx > > > are normally mutually exclusive, so I guess that's why they have been > > > implemented in framework of the same interface. Another reason could > > > be to have less troubles with adopting the PCI drivers for both type > > > of the IRQs delivery. > > > > > > Moreover just recently the so called _legacy_ flag name has been > > > deprecated in favor of the more generic INTx one: > > > https://lore.kernel.org/linux-pci/20231122060406.14695-1-dlemoal@kernel.org/ > > > This info is important, but your last suggestion for Yanteng still use > > PCI_IRQ_LEGACY. :) > > Yes, my mistake. It should be replaced with PCI_IRQ_INTX. > > > > > > > > > Once again about the naming. From the retrospective point of view the > > > so called legacy PCI IRQs (in fact PCI INTx) and the platform IRQs > > > look similar because these are just the level-type signals connected > > > to the system IRQ controller. But when it comes to the PCI _Express_, > > > the implementation is completely different. The PCIe INTx is just the > > > PCIe TLPs of special type, like MSI. Upon receiving these special > > > messages the PCIe host controller delivers the IRQ up to the > > > respective system IRQ controller. So in order to avoid the confusion > > > between the actual legacy PCI INTx, PCI Express INTx and the just > > > platform IRQs, it's better to emphasize the actual way of the IRQs > > > delivery. In this case it's the later method. > > You are absolutely right, and I think I found a method to use your > > framework to solve our problems: > > > > static int loongson_dwmac_config_irqs(struct pci_dev *pdev, > > struct plat_stmmacenet_data *plat, > > struct stmmac_resources *res) > > { > > int i, ret, vecs; > > > > /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > > * --------- ----- -------- -------- ... -------- -------- > > * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > > */ > > vecs = plat->rx_queues_to_use + plat->tx_queues_to_use + 1; > > ret = pci_alloc_irq_vectors(pdev, 1, vecs, PCI_IRQ_MSI | PCI_IRQ_INTX); > > if (ret < 0) { > > dev_err(&pdev->dev, "Failed to allocate PCI IRQs\n"); > > return ret; > > } > > if (ret >= vecs) { > > for (i = 0; i < plat->rx_queues_to_use; i++) { > > res->rx_irq[CHANNELS_NUM - 1 - i] = > > pci_irq_vector(pdev, 1 + i * 2); > > } > > for (i = 0; i < plat->tx_queues_to_use; i++) { > > res->tx_irq[CHANNELS_NUM - 1 - i] = > > pci_irq_vector(pdev, 2 + i * 2); > > } > > > > plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > > } > > > > res->irq = pci_irq_vector(pdev, 0); > > > > if (np) { > > res->irq = of_irq_get_byname(np, "macirq"); > > if (res->irq < 0) { > > dev_err(&pdev->dev, "IRQ macirq not found\n"); > > return -ENODEV; > > } > > > > res->wol_irq = of_irq_get_byname(np, "eth_wake_irq"); > > if (res->wol_irq < 0) { > > dev_info(&pdev->dev, > > "IRQ eth_wake_irq not found, using macirq\n"); > > res->wol_irq = res->irq; > > } > > > > res->lpi_irq = of_irq_get_byname(np, "eth_lpi"); > > if (res->lpi_irq < 0) { > > dev_err(&pdev->dev, "IRQ eth_lpi not found\n"); > > return -ENODEV; > > } > > } > > return 0; > > } > > > > If your agree, Yanteng can use this method in V13, then avoid furthur changes. > > Since yesterday I have been too relaxed sitting back to explain in > detail the problems with the code above. Shortly speaking, no to the > method designed as above. This function is copy-paste from your version which you suggest to Yanteng, and plus the fallback parts for DT. If you don't want to discuss it any more, we can discuss after V13. BTW, we cannot remove "res->wol_irq = res->irq", because Loongson GMAC/GNET indeed supports WoL. Huacai > > -Serge(y) > > > > > Huacai > > > > > > > > > > > > > > > > > > > > 6. In the legacy case, the irq is get from DT-node (FDT case), or > > > > > > already in pdev->irq (ACPI case). > > > > > > > > > > It will be in the pdev->irq in any case whether it's DT or ACPI. See: > > > > > > > > > > ACPI: > > > > > pci_device_probe(): > > > > > +-> arch/loongarch/pci/pci.c:pcibios_alloc_irq() > > > > > > > > > > DT: > > > > > pci_device_probe(): > > > > > +-> pci_assign_irq(); > > > > > +-> pci_host_bridge::map_irq() > > > > > +-> of_irq_parse_and_map_pci() > > > > > or in case of Loongson PCIe host controller: > > > > > +-> drivers/pci/controller/pci-loongson.c::loongson_map_irq() > > > > > > > > > > Moreover unless the MSI IRQs are enabled, the platform IRQ (and the > > > > > legacy IRQ) can be retrieved by means of the pci_irq_vector() method. > > > > > The only reason of having the direct OF-based IRQs getting in the > > > > > Loongson DWMAC driver I see is that the LPI IRQ will be missing in > > > > > case of the pci_irq_vector() method utilization. In the rest of the > > > > > cases the pci_irq_vector() function could be freely used. > > > > Yes, in the DT case, they may be macirq, eth_wake_irq and eth_lpi, > > > > rather than a single irq, so we need an if-else here. > > > > > > > > > > > > > > > So Yanteng use a "if (np) { } else { > > > > > > }", which is reasonable from my point of view. > > > > > > > > > > > > > > > > At least one problem is there. What if pdev->irq isn't initialized > > > > > (initialized with zero)?.. > > > > > > > As you said above, both ACPI and DT initialized pdev->irq, unless > > > > there is a bug in BIOS. > > > > > > I meant that based on the platform firmware nature the pdev->irq field > > > shall be initialized with an IRQ number in accordance with the DT or > > > ACPI logic. I never said it was impossible to have the field > > > uninitialized (that is being left zero). It's absolutely possible. > > > There are much more reasons to have that than just a firmware bug. On > > > the top of my mind: MSI being enabled, kernel misconfiguration, kernel > > > bug, DT/ACPI lacking the IRQ property, ... > > > > > > -Serge(y) > > > > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > So Yanteng's interrupt code is good for me, but I also agree to > > > > > > improve that after v13, if needed. > > > > > > > > > > Ok. I've got much better picture about what is going on under the > > > > > hood. Thanks. In anyway I'll get back to this topic in details in v13. > > > > > > > > > > -Serge(y) > > > > > > > > > > > > > > > > > Huacai > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > -Serge(y) > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > Yanteng > > > > > > > >
Well, let's modify the biggest patch。 在 2024/5/6 05:50, Serge Semin 写道: > On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote: >> There are two types of Loongson DWGMAC. The first type shares the same > s/Loongson DWGMAC/Loongson GNET controllers OK, > >> register definitions and has similar logic as dwmac1000. The second type >> uses several different register definitions, we think it is necessary to >> distinguish rx and tx, so we split these bits into two. > s/rx/Rx > s/tx/Tx OK. >> Simply put, we split some single bit fields into double bits fileds: >> >> Name Tx Rx >> >> DMA_INTR_ENA_NIE = 0x00040000 | 0x00020000; >> DMA_INTR_ENA_AIE = 0x00010000 | 0x00008000; >> DMA_STATUS_NIS = 0x00040000 | 0x00020000; >> DMA_STATUS_AIS = 0x00010000 | 0x00008000; >> DMA_STATUS_FBI = 0x00002000 | 0x00001000; >> >> Therefore, when using, TX and RX must be set at the same time. >> >> How to use them: >> 1. Create the Loongson GNET-specific >> stmmac_dma_ops.dma_interrupt() >> stmmac_dma_ops.init_chan() >> methods in the dwmac-loongson.c driver. Adding all the >> Loongson-specific macros >> >> 2. Create a Loongson GNET-specific platform setup method with the next >> semantics: >> + allocate stmmac_dma_ops instance and initialize it with >> dwmac1000_dma_ops. >> + override the stmmac_dma_ops.{dma_interrupt, init_chan} with >> the pointers to the methods defined in 2. >> + allocate mac_device_info instance and initialize the >> mac_device_info.dma field with a pointer to the new >> stmmac_dma_ops instance. >> + initialize mac_device_info in a way it's done in >> dwmac1000_setup(). >> >> 3. Initialize plat_stmmacenet_data.setup() with the pointer to the >> method created in 2. >> >> GNET features: >> >> Speeds: 10/100/1000Mbps >> DMA-descriptors type: enhanced >> L3/L4 filters availability: support >> VLAN hash table filter: support >> PHY-interface: GMII >> Remote Wake-up support: support >> Mac Management Counters (MMC): support >> Number of additional MAC addresses: 5 >> MAC Hash-based filter: support >> Number of ash table size: 256 >> DMA chennel number: 0x10 device is 8 and 0x37 device is 1 >> >> Others: >> >> GNET integrates both MAC and PHY chips inside. >> GNET device: LS2K2000, LS7A2000, the chip connection between the mac and >> phy of these devices is not normal and requires two rounds of >> negotiation; LS7A2000 does not support half-duplex and >> multi-channel; >> >> To enable multi-channel on LS2K2000, you need to turn off >> hardware checksum. >> >> **Note**: Currently, only the LS2K2000's synopsys_id is 0x10, while the >> synopsys_id of other devices are 0x37. > The entire commit log looks as a set of information and doesn't > explicitly explain what is going on in the patch body. Let's make it a > bit more coherent: > > "Aside with the Loongson GMAC controllers which can be normally found > on the LS2K1000 SoC and LS7A1000 chipset, Loongson released a new > version of the network controllers called Loongson GNET. It has > been synthesized into the new generation LS2K2000 SoC and LS7A2000 > chipset with the next DW GMAC features enabled: > > DW GMAC IP-core: v3.73a > Speeds: 10/100/1000Mbps > Duplex: Full (both versions), Half (LS2K2000 SoC only) > DMA-descriptors type: enhanced > L3/L4 filters availability: Y > VLAN hash table filter: Y > PHY-interface: GMII (PHY is integrated into the chips) > Remote Wake-up support: Y > Mac Management Counters (MMC): Y > Number of additional MAC addresses: 5 > MAC Hash-based filter: Y > Hash Table Size: 256 > AV feature: Y (LS2K2000 SoC only) > DMA channels: 8 (LS2K2000 SoC), 1 (LS7A2000 chipset) > > The integrated PHY has a weird problem with switching from the low > speeds to 1000Mbps mode. The speedup procedure requires the PHY-link > re-negotiation. Besides the LS2K2000 GNET controller the next > peculiarities: > 1. Split up Tx and Rx DMA IRQ status/mask bits: > Name Tx Rx > DMA_INTR_ENA_NIE = 0x00040000 | 0x00020000; > DMA_INTR_ENA_AIE = 0x00010000 | 0x00008000; > DMA_STATUS_NIS = 0x00040000 | 0x00020000; > DMA_STATUS_AIS = 0x00010000 | 0x00008000; > DMA_STATUS_FBI = 0x00002000 | 0x00001000; > 2. Custom Synopsys ID hardwired into the GMAC_VERSION.SNPSVER field. > It's 0x10 while it should have been 0x37 in accordance with the actual > DW GMAC IP-core version. > > Thus in order to have the Loongson GNET controllers supported let's > modify the Loongson DWMAC driver in accordance with all the > peculiarities described above: > > 1. Create the Loongson GNET-specific > stmmac_dma_ops::dma_interrupt() > stmmac_dma_ops::init_chan() > callbacks due to the non-standard DMA IRQ CSR flags layout. > 2. Create the Loongson GNET-specific platform setup() method which > gets to initialize the DMA-ops with the dwmac1000_dma_ops instance > and overrides the callbacks described in 1, and overrides the custom > Synopsys ID with the real one in order to have the rest of the > HW-specific callbacks correctly detected by the driver core. > 3. Make sure the Loongson GNET-specific platform setup() method > enables the duplex modes supported by the controller. > 4. Provide the plat_stmmacenet_data::fix_mac_speed() callback which > will restart the link Auto-negotiation in case of the speed change." > > > See, you don't need to mention the 0x10 ID all the time. Just once and > in the place where it's actually relevant. OK, Thanks a lot! > >> Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> >> Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> >> Signed-off-by: Yanteng Si <siyanteng@loongson.cn> >> --- >> drivers/net/ethernet/stmicro/stmmac/common.h | 1 + >> .../ethernet/stmicro/stmmac/dwmac-loongson.c | 381 +++++++++++++++++- >> 2 files changed, 371 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h >> index 9cd62b2110a1..aed6ae80cc7c 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/common.h >> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h >> @@ -29,6 +29,7 @@ >> /* Synopsys Core versions */ >> #define DWMAC_CORE_3_40 0x34 >> #define DWMAC_CORE_3_50 0x35 >> +#define DWMAC_CORE_3_70 0x37 >> #define DWMAC_CORE_4_00 0x40 >> #define DWMAC_CORE_4_10 0x41 >> #define DWMAC_CORE_5_00 0x50 >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> index a16bba389417..68de90c44feb 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c >> @@ -8,9 +8,71 @@ >> #include <linux/device.h> >> #include <linux/of_irq.h> >> #include "stmmac.h" >> +#include "dwmac_dma.h" >> +#include "dwmac1000.h" >> + >> >> +#define LOONGSON_DWMAC_CORE_1_00 0x10 /* Loongson custom IP */ > What about using the name like calling as: > +#define DWMAC_CORE_LS2K2000 0x10 > Thus you'll have the name similar to the rest of the DWMAC_CORE_* > macros and which would emphasize what the device for which the custom > ID is specific. OK. > >> +#define CHANNEL_NUM 8 >> + >> +struct loongson_data { >> + u32 gmac_verion; > Let's call it loongson_id thus referring to the > stmmac_priv::synopsys_id field. OK. > >> + >> +static int loongson_gnet_dma_interrupt(struct stmmac_priv *priv, >> + void __iomem *ioaddr, >> + struct stmmac_extra_stats *x, >> + u32 chan, u32 dir) >> +{ >> + struct stmmac_pcpu_stats *stats = > ... >> + /* Clear the interrupt by writing a logic 1 to the CSR5[15-0] */ > 0x7ffff != CSR5[15-0] Hmmm, It should be CSR5[19-0]? BTW, 0x1ffff != CSR5[15-0], too. It should be CSR5[16-0], right? > >> + writel((intr_status & 0x7ffff), ioaddr + DMA_CHAN_STATUS(chan)); >> + >> + return ret; >> +} >> + >> +static void loongson_gnet_fix_speed(void *priv, unsigned int speed, >> + unsigned int mode) >> +{ >> + struct loongson_data *ld = (struct loongson_data *)priv; >> + struct net_device *ndev = dev_get_drvdata(ld->dev); >> + struct stmmac_priv *ptr = netdev_priv(ndev); >> + >> + /* The controller and PHY don't work well together. >> + * We need to use the PS bit to check if the controller's status >> + * is correct and reset PHY if necessary. > This doesn't correspond to what you're actually doing. Please align > the comment with what is done below (if what I provided in the commit > log regarding this problem is correct, use the description here). OK, you are right. >> + * MAC_CTRL_REG.15 is defined by the GMAC_CONTROL_PS macro. > useless. please drop OK. > >> + */ >> + if (speed == SPEED_1000) { >> >> + >> +static struct mac_device_info *loongson_dwmac_setup(void *apriv) >> +{ >> + struct stmmac_priv *priv = apriv; >> + struct mac_device_info *mac; > seems unused. See my next comment. No, We're using it. See my next reply. > >> + struct stmmac_dma_ops *dma; >> + struct loongson_data *ld; >> + struct pci_dev *pdev; >> + >> + ld = priv->plat->bsp_priv; >> + pdev = to_pci_dev(priv->device); >> + >> + mac = devm_kzalloc(priv->device, sizeof(*mac), GFP_KERNEL); >> + if (!mac) >> + return NULL; > I see you no longer override the ops in dwmac1000_ops. If so this can > be dropped. No, Because I pre-initialize the respective "mac" fields as it's done in dwmac1000_setup(). > >> + >> + dma = devm_kzalloc(priv->device, sizeof(*dma), GFP_KERNEL); >> + if (!dma) >> + return NULL; >> + >> + /* The original IP-core version is 0x37 in all Loongson GNET > s/0x37/v3.73a Yeah! > >> + * (ls2k2000 and ls7a2000), but the GNET HW designers have changed the >> + * GMAC_VERSION.SNPSVER field to the custom 0x10 value on the Loongson >> + * ls2k2000 MAC to emphasize the differences: multiple DMA-channels, > s/ls2k2000/LS2K2000 > s/ls7a2000/LS7A2000 OK. > >> + * AV feature and GMAC_INT_STATUS CSR flags layout. Get back the >> + * original value so the correct HW-interface would be selected. >> + */ >> + if (ld->gmac_verion == LOONGSON_DWMAC_CORE_1_00) { >> + priv->synopsys_id = DWMAC_CORE_3_70; >> + *dma = dwmac1000_dma_ops; >> + dma->init_chan = loongson_gnet_dma_init_channel; >> + dma->dma_interrupt = loongson_gnet_dma_interrupt; >> + mac->dma = dma; >> + } >> + >> + mac->mac = &dwmac1000_ops; > Unused? Yeah, will be droped! > >> + priv->dev->priv_flags |= IFF_UNICAST_FLT; >> + >> + /* Pre-initialize the respective "mac" fields as it's done in >> + * dwmac1000_setup() >> + */ >> + mac->pcsr = priv->ioaddr; >> + mac->multicast_filter_bins = priv->plat->multicast_filter_bins; >> + mac->unicast_filter_entries = priv->plat->unicast_filter_entries; >> + mac->mcast_bits_log2 = 0; >> + >> + if (mac->multicast_filter_bins) >> + mac->mcast_bits_log2 = ilog2(mac->multicast_filter_bins); >> + >> + /* The GMAC devices with PCI ID 0x7a03 does not support any pause mode. >> + * The GNET devices without CORE ID 0x10 does not support half-duplex. >> + */ > No need to mention the IDs but just the actual devices: > /* Loongson GMAC doesn't support the flow control. LS2K2000 > * GNET doesn't support the half-duplex link mode. > */ > OK, Thanks. >> + if (pdev->device == PCI_DEVICE_ID_LOONGSON_GMAC) { >> + mac->link.caps = MAC_10 | MAC_100 | MAC_1000; >> + } else { >> + if (ld->gmac_verion == LOONGSON_DWMAC_CORE_1_00) >> + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | >> + MAC_10 | MAC_100 | MAC_1000; >> + else >> + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | >> + MAC_10FD | MAC_100FD | MAC_1000FD; >> + } >> + >> + mac->link.duplex = GMAC_CONTROL_DM; >> + mac->link.speed10 = GMAC_CONTROL_PS; >> + mac->link.speed100 = GMAC_CONTROL_PS | GMAC_CONTROL_FES; >> + mac->link.speed1000 = 0; >> + mac->link.speed_mask = GMAC_CONTROL_PS | GMAC_CONTROL_FES; >> + mac->mii.addr = GMAC_MII_ADDR; >> + mac->mii.data = GMAC_MII_DATA; >> + mac->mii.addr_shift = 11; >> + mac->mii.addr_mask = 0x0000F800; >> + mac->mii.reg_shift = 6; >> + mac->mii.reg_mask = 0x000007C0; >> + mac->mii.clk_csr_shift = 2; >> + mac->mii.clk_csr_mask = GENMASK(5, 2); >> + >> + return mac; >> +} >> + >> static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> { >> struct plat_stmmacenet_data *plat; >> int ret, i, bus_id, phy_mode; >> struct stmmac_pci_info *info; >> struct stmmac_resources res; >> + struct loongson_data *ld; >> struct device_node *np; >> >> np = dev_of_node(&pdev->dev); >> @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id >> return -ENOMEM; >> >> plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL); >> - if (!plat->dma_cfg) { >> - ret = -ENOMEM; >> - goto err_put_node; >> - } >> + if (!plat->dma_cfg) >> + return -ENOMEM; > This change must have been introduced in the patch > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > which moves the mdio_node pointer initialization to under the if-clause. OK. > >> + >> + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); >> + if (!ld) >> + return -ENOMEM; >> >> /* Enable pci device */ >> ret = pci_enable_device(pdev); >> @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id >> plat->phy_interface = phy_mode; >> } >> >> - pci_enable_msi(pdev); > Hm, this must be justified and better being done in a separate patch. OK. > >> + plat->bsp_priv = ld; >> + plat->setup = loongson_dwmac_setup; >> + ld->dev = &pdev->dev; >> + >> memset(&res, 0, sizeof(res)); >> res.addr = pcim_iomap_table(pdev)[0]; >> + ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; >> + >> + switch (ld->gmac_verion) { >> + case LOONGSON_DWMAC_CORE_1_00: >> + plat->rx_queues_to_use = CHANNEL_NUM; >> + plat->tx_queues_to_use = CHANNEL_NUM; >> + >> + /* Only channel 0 supports checksum, >> + * so turn off checksum to enable multiple channels. >> + */ >> + for (i = 1; i < CHANNEL_NUM; i++) >> + plat->tx_queues_cfg[i].coe_unsupported = 1; >> >> - plat->tx_queues_to_use = 1; >> - plat->rx_queues_to_use = 1; >> + ret = loongson_dwmac_config_msi(pdev, plat, &res, np); >> + break; >> + default: /* 0x35 device and 0x37 device. */ >> + plat->tx_queues_to_use = 1; >> + plat->rx_queues_to_use = 1; >> > Move the NoF queues (and coe flag) initializations to the respective > loongson_*_data() methods. OK. > > Besides I don't see you freeing the IRQ vectors allocated in the > loongson_dwmac_config_msi() method neither in probe(), nor in remove() > functions. That's definitely wrong. What you need is to have a > method antagonistic to loongson_dwmac_config_msi() (like > loongson_dwmac_clear_msi()) which would execute the cleanup procedure. Hmmm, We can free it in struct pci_driver ->remove method. Just in loongson_dwmac_remove() call pci_free_irq_vectors(pdev); > >> - ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); >> + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); >> + break; >> + } >> >> /* GNET devices with dev revision 0x00 do not support manually >> * setting the speed to 1000. >> @@ -189,12 +549,10 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id >> >> ret = stmmac_dvr_probe(&pdev->dev, plat, &res); >> if (ret) >> - goto err_disable_msi; >> + goto err_disable_device; >> >> return ret; >> >> -err_disable_msi: >> - pci_disable_msi(pdev); > Once again. Justify the change. Moreover I don't see you dropping the > pci_disable_msi() from the remove() method. Since we need to check the return value of the allocated msi, we will restore this change in v13. Thanks, Yanteng
Hi Huacai, Serge, 在 2024/5/15 21:55, Huacai Chen 写道: >>>> Once again about the naming. From the retrospective point of view the >>>> so called legacy PCI IRQs (in fact PCI INTx) and the platform IRQs >>>> look similar because these are just the level-type signals connected >>>> to the system IRQ controller. But when it comes to the PCI_Express_, >>>> the implementation is completely different. The PCIe INTx is just the >>>> PCIe TLPs of special type, like MSI. Upon receiving these special >>>> messages the PCIe host controller delivers the IRQ up to the >>>> respective system IRQ controller. So in order to avoid the confusion >>>> between the actual legacy PCI INTx, PCI Express INTx and the just >>>> platform IRQs, it's better to emphasize the actual way of the IRQs >>>> delivery. In this case it's the later method. >>> You are absolutely right, and I think I found a method to use your >>> framework to solve our problems: >>> >>> static int loongson_dwmac_config_irqs(struct pci_dev *pdev, >>> struct plat_stmmacenet_data *plat, >>> struct stmmac_resources *res) >>> { >>> int i, ret, vecs; >>> >>> /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | >>> * --------- ----- -------- -------- ... -------- -------- >>> * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | >>> */ >>> vecs = plat->rx_queues_to_use + plat->tx_queues_to_use + 1; >>> ret = pci_alloc_irq_vectors(pdev, 1, vecs, PCI_IRQ_MSI | PCI_IRQ_INTX); >>> if (ret < 0) { >>> dev_err(&pdev->dev, "Failed to allocate PCI IRQs\n"); >>> return ret; >>> } >>> if (ret >= vecs) { >>> for (i = 0; i < plat->rx_queues_to_use; i++) { >>> res->rx_irq[CHANNELS_NUM - 1 - i] = >>> pci_irq_vector(pdev, 1 + i * 2); >>> } >>> for (i = 0; i < plat->tx_queues_to_use; i++) { >>> res->tx_irq[CHANNELS_NUM - 1 - i] = >>> pci_irq_vector(pdev, 2 + i * 2); >>> } >>> >>> plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; >>> } >>> >>> res->irq = pci_irq_vector(pdev, 0); >>> >>> if (np) { >>> res->irq = of_irq_get_byname(np, "macirq"); >>> if (res->irq < 0) { >>> dev_err(&pdev->dev, "IRQ macirq not found\n"); >>> return -ENODEV; >>> } >>> >>> res->wol_irq = of_irq_get_byname(np, "eth_wake_irq"); >>> if (res->wol_irq < 0) { >>> dev_info(&pdev->dev, >>> "IRQ eth_wake_irq not found, using macirq\n"); >>> res->wol_irq = res->irq; >>> } >>> >>> res->lpi_irq = of_irq_get_byname(np, "eth_lpi"); >>> if (res->lpi_irq < 0) { >>> dev_err(&pdev->dev, "IRQ eth_lpi not found\n"); >>> return -ENODEV; >>> } >>> } >>> return 0; >>> } >>> >>> If your agree, Yanteng can use this method in V13, then avoid furthur changes. >> Since yesterday I have been too relaxed sitting back to explain in >> detail the problems with the code above. Shortly speaking, no to the >> method designed as above. > This function is copy-paste from your version which you suggest to > Yanteng, and plus the fallback parts for DT. If you don't want to > discuss it any more, we can discuss after V13. All right. I have been preparing v13 and will send it as soon as possible. Let's continue the discussion in v13. Of course, I will copy the part that has not received a clear reply to v13. > > BTW, we cannot remove "res->wol_irq = res->irq", because Loongson > GMAC/GNET indeed supports WoL. Okay, I will not drop it in v13. Thanks, Yanteng
On Fri, May 17, 2024 at 04:42:51PM +0800, Yanteng Si wrote: > Hi Huacai, Serge, > > 在 2024/5/15 21:55, Huacai Chen 写道: > > > > > Once again about the naming. From the retrospective point of view the > > > > > so called legacy PCI IRQs (in fact PCI INTx) and the platform IRQs > > > > > look similar because these are just the level-type signals connected > > > > > to the system IRQ controller. But when it comes to the PCI_Express_, > > > > > the implementation is completely different. The PCIe INTx is just the > > > > > PCIe TLPs of special type, like MSI. Upon receiving these special > > > > > messages the PCIe host controller delivers the IRQ up to the > > > > > respective system IRQ controller. So in order to avoid the confusion > > > > > between the actual legacy PCI INTx, PCI Express INTx and the just > > > > > platform IRQs, it's better to emphasize the actual way of the IRQs > > > > > delivery. In this case it's the later method. > > > > You are absolutely right, and I think I found a method to use your > > > > framework to solve our problems: > > > > > > > > static int loongson_dwmac_config_irqs(struct pci_dev *pdev, > > > > struct plat_stmmacenet_data *plat, > > > > struct stmmac_resources *res) > > > > { > > > > int i, ret, vecs; > > > > > > > > /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > > > > * --------- ----- -------- -------- ... -------- -------- > > > > * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > > > > */ > > > > vecs = plat->rx_queues_to_use + plat->tx_queues_to_use + 1; > > > > ret = pci_alloc_irq_vectors(pdev, 1, vecs, PCI_IRQ_MSI | PCI_IRQ_INTX); > > > > if (ret < 0) { > > > > dev_err(&pdev->dev, "Failed to allocate PCI IRQs\n"); > > > > return ret; > > > > } > > > > if (ret >= vecs) { > > > > for (i = 0; i < plat->rx_queues_to_use; i++) { > > > > res->rx_irq[CHANNELS_NUM - 1 - i] = > > > > pci_irq_vector(pdev, 1 + i * 2); > > > > } > > > > for (i = 0; i < plat->tx_queues_to_use; i++) { > > > > res->tx_irq[CHANNELS_NUM - 1 - i] = > > > > pci_irq_vector(pdev, 2 + i * 2); > > > > } > > > > > > > > plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > > > > } > > > > > > > > res->irq = pci_irq_vector(pdev, 0); > > > > > > > > if (np) { > > > > res->irq = of_irq_get_byname(np, "macirq"); > > > > if (res->irq < 0) { > > > > dev_err(&pdev->dev, "IRQ macirq not found\n"); > > > > return -ENODEV; > > > > } > > > > > > > > res->wol_irq = of_irq_get_byname(np, "eth_wake_irq"); > > > > if (res->wol_irq < 0) { > > > > dev_info(&pdev->dev, > > > > "IRQ eth_wake_irq not found, using macirq\n"); > > > > res->wol_irq = res->irq; > > > > } > > > > > > > > res->lpi_irq = of_irq_get_byname(np, "eth_lpi"); > > > > if (res->lpi_irq < 0) { > > > > dev_err(&pdev->dev, "IRQ eth_lpi not found\n"); > > > > return -ENODEV; > > > > } > > > > } > > > > return 0; > > > > } > > > > > > > > If your agree, Yanteng can use this method in V13, then avoid furthur changes. > > > Since yesterday I have been too relaxed sitting back to explain in > > > detail the problems with the code above. Shortly speaking, no to the > > > method designed as above. > > This function is copy-paste from your version which you suggest to > > Yanteng, and plus the fallback parts for DT. If you don't want to > > discuss it any more, we can discuss after V13. My conclusion is the same. no to _your_ (Huacai) version of the code. I suggest to Huacai dig dipper in the function semantic and find out the problems it has. Meanwhile I'll keep relaxing... > > > > BTW, we cannot remove "res->wol_irq = res->irq", because Loongson > > GMAC/GNET indeed supports WoL. > > Okay, I will not drop it in v13. Apparently Huacai isn't well familiar with what he is reviewing. Once again the initialization is useless. Drop it. > > All right. I have been preparing v13 and will send it as soon as possible. > > Let's continue the discussion in v13. Of course, I will copy the part that > has > > not received a clear reply to v13. > Note the merge window has been opened and the 'net-next' tree is now closed. So either you submit your series as RFC or wait for the window being closed. -Serge(y) > > > Thanks, > > Yanteng >
On Fri, May 17, 2024 at 04:12:20PM +0800, Yanteng Si wrote: > Well, let's modify the biggest patch。 > > 在 2024/5/6 05:50, Serge Semin 写道: > > On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote: > > > There are two types of Loongson DWGMAC. The first type shares the same > > s/Loongson DWGMAC/Loongson GNET controllers > OK, > > > > > register definitions and has similar logic as dwmac1000. The second type > > > uses several different register definitions, we think it is necessary to > > > distinguish rx and tx, so we split these bits into two. > > s/rx/Rx > > s/tx/Tx > > OK. > > > > Simply put, we split some single bit fields into double bits fileds: > > > > > > Name Tx Rx > > > > > > DMA_INTR_ENA_NIE = 0x00040000 | 0x00020000; > > > DMA_INTR_ENA_AIE = 0x00010000 | 0x00008000; > > > DMA_STATUS_NIS = 0x00040000 | 0x00020000; > > > DMA_STATUS_AIS = 0x00010000 | 0x00008000; > > > DMA_STATUS_FBI = 0x00002000 | 0x00001000; > > > > > > Therefore, when using, TX and RX must be set at the same time. > > > > > > How to use them: > > > 1. Create the Loongson GNET-specific > > > stmmac_dma_ops.dma_interrupt() > > > stmmac_dma_ops.init_chan() > > > methods in the dwmac-loongson.c driver. Adding all the > > > Loongson-specific macros > > > > > > 2. Create a Loongson GNET-specific platform setup method with the next > > > semantics: > > > + allocate stmmac_dma_ops instance and initialize it with > > > dwmac1000_dma_ops. > > > + override the stmmac_dma_ops.{dma_interrupt, init_chan} with > > > the pointers to the methods defined in 2. > > > + allocate mac_device_info instance and initialize the > > > mac_device_info.dma field with a pointer to the new > > > stmmac_dma_ops instance. > > > + initialize mac_device_info in a way it's done in > > > dwmac1000_setup(). > > > > > > 3. Initialize plat_stmmacenet_data.setup() with the pointer to the > > > method created in 2. > > > > > > GNET features: > > > > > > Speeds: 10/100/1000Mbps > > > DMA-descriptors type: enhanced > > > L3/L4 filters availability: support > > > VLAN hash table filter: support > > > PHY-interface: GMII > > > Remote Wake-up support: support > > > Mac Management Counters (MMC): support > > > Number of additional MAC addresses: 5 > > > MAC Hash-based filter: support > > > Number of ash table size: 256 > > > DMA chennel number: 0x10 device is 8 and 0x37 device is 1 > > > > > > Others: > > > > > > GNET integrates both MAC and PHY chips inside. > > > GNET device: LS2K2000, LS7A2000, the chip connection between the mac and > > > phy of these devices is not normal and requires two rounds of > > > negotiation; LS7A2000 does not support half-duplex and > > > multi-channel; > > > > > > To enable multi-channel on LS2K2000, you need to turn off > > > hardware checksum. > > > > > > **Note**: Currently, only the LS2K2000's synopsys_id is 0x10, while the > > > synopsys_id of other devices are 0x37. > > The entire commit log looks as a set of information and doesn't > > explicitly explain what is going on in the patch body. Let's make it a > > bit more coherent: > > > > "Aside with the Loongson GMAC controllers which can be normally found > > on the LS2K1000 SoC and LS7A1000 chipset, Loongson released a new > > version of the network controllers called Loongson GNET. It has > > been synthesized into the new generation LS2K2000 SoC and LS7A2000 > > chipset with the next DW GMAC features enabled: > > > > DW GMAC IP-core: v3.73a > > Speeds: 10/100/1000Mbps > > Duplex: Full (both versions), Half (LS2K2000 SoC only) > > DMA-descriptors type: enhanced > > L3/L4 filters availability: Y > > VLAN hash table filter: Y > > PHY-interface: GMII (PHY is integrated into the chips) > > Remote Wake-up support: Y > > Mac Management Counters (MMC): Y > > Number of additional MAC addresses: 5 > > MAC Hash-based filter: Y > > Hash Table Size: 256 > > AV feature: Y (LS2K2000 SoC only) > > DMA channels: 8 (LS2K2000 SoC), 1 (LS7A2000 chipset) > > > > The integrated PHY has a weird problem with switching from the low > > speeds to 1000Mbps mode. The speedup procedure requires the PHY-link > > re-negotiation. Besides the LS2K2000 GNET controller the next > > peculiarities: > > 1. Split up Tx and Rx DMA IRQ status/mask bits: > > Name Tx Rx > > DMA_INTR_ENA_NIE = 0x00040000 | 0x00020000; > > DMA_INTR_ENA_AIE = 0x00010000 | 0x00008000; > > DMA_STATUS_NIS = 0x00040000 | 0x00020000; > > DMA_STATUS_AIS = 0x00010000 | 0x00008000; > > DMA_STATUS_FBI = 0x00002000 | 0x00001000; > > 2. Custom Synopsys ID hardwired into the GMAC_VERSION.SNPSVER field. > > It's 0x10 while it should have been 0x37 in accordance with the actual > > DW GMAC IP-core version. > > > > Thus in order to have the Loongson GNET controllers supported let's > > modify the Loongson DWMAC driver in accordance with all the > > peculiarities described above: > > > > 1. Create the Loongson GNET-specific > > stmmac_dma_ops::dma_interrupt() > > stmmac_dma_ops::init_chan() > > callbacks due to the non-standard DMA IRQ CSR flags layout. > > 2. Create the Loongson GNET-specific platform setup() method which > > gets to initialize the DMA-ops with the dwmac1000_dma_ops instance > > and overrides the callbacks described in 1, and overrides the custom > > Synopsys ID with the real one in order to have the rest of the > > HW-specific callbacks correctly detected by the driver core. > > 3. Make sure the Loongson GNET-specific platform setup() method > > enables the duplex modes supported by the controller. > > 4. Provide the plat_stmmacenet_data::fix_mac_speed() callback which > > will restart the link Auto-negotiation in case of the speed change." > > > > > > See, you don't need to mention the 0x10 ID all the time. Just once and > > in the place where it's actually relevant. > OK, Thanks a lot! > > > > > Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> > > > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> > > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn> > > > --- > > > drivers/net/ethernet/stmicro/stmmac/common.h | 1 + > > > .../ethernet/stmicro/stmmac/dwmac-loongson.c | 381 +++++++++++++++++- > > > 2 files changed, 371 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h > > > index 9cd62b2110a1..aed6ae80cc7c 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/common.h > > > +++ b/drivers/net/ethernet/stmicro/stmmac/common.h > > > @@ -29,6 +29,7 @@ > > > /* Synopsys Core versions */ > > > #define DWMAC_CORE_3_40 0x34 > > > #define DWMAC_CORE_3_50 0x35 > > > +#define DWMAC_CORE_3_70 0x37 > > > #define DWMAC_CORE_4_00 0x40 > > > #define DWMAC_CORE_4_10 0x41 > > > #define DWMAC_CORE_5_00 0x50 > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > index a16bba389417..68de90c44feb 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > > @@ -8,9 +8,71 @@ > > > #include <linux/device.h> > > > #include <linux/of_irq.h> > > > #include "stmmac.h" > > > +#include "dwmac_dma.h" > > > +#include "dwmac1000.h" > > > + > > > > > > +#define LOONGSON_DWMAC_CORE_1_00 0x10 /* Loongson custom IP */ > > What about using the name like calling as: > > +#define DWMAC_CORE_LS2K2000 0x10 > > Thus you'll have the name similar to the rest of the DWMAC_CORE_* > > macros and which would emphasize what the device for which the custom > > ID is specific. > OK. > > > > > +#define CHANNEL_NUM 8 > > > + > > > +struct loongson_data { > > > + u32 gmac_verion; > > Let's call it loongson_id thus referring to the > > stmmac_priv::synopsys_id field. > OK. > > > > > + > > > +static int loongson_gnet_dma_interrupt(struct stmmac_priv *priv, > > > + void __iomem *ioaddr, > > > + struct stmmac_extra_stats *x, > > > + u32 chan, u32 dir) > > > +{ > > > + struct stmmac_pcpu_stats *stats = > > ... > > > + /* Clear the interrupt by writing a logic 1 to the CSR5[15-0] */ > > 0x7ffff != CSR5[15-0] > > Hmmm, It should be CSR5[19-0]? 0x7ffff = [18-0] 0xfffff = [19-0] > > BTW, 0x1ffff != CSR5[15-0], too. > > It should be CSR5[16-0], right? Right. If you wish to fix that in the original code, that has to be done in a dedicated patch. > > > > > > > + writel((intr_status & 0x7ffff), ioaddr + DMA_CHAN_STATUS(chan)); > > > + > > > + return ret; > > > +} > > > + > > > +static void loongson_gnet_fix_speed(void *priv, unsigned int speed, > > > + unsigned int mode) > > > +{ > > > + struct loongson_data *ld = (struct loongson_data *)priv; > > > + struct net_device *ndev = dev_get_drvdata(ld->dev); > > > + struct stmmac_priv *ptr = netdev_priv(ndev); > > > + > > > + /* The controller and PHY don't work well together. > > > + * We need to use the PS bit to check if the controller's status > > > + * is correct and reset PHY if necessary. > > This doesn't correspond to what you're actually doing. Please align > > the comment with what is done below (if what I provided in the commit > > log regarding this problem is correct, use the description here). > OK, you are right. > > > + * MAC_CTRL_REG.15 is defined by the GMAC_CONTROL_PS macro. > > useless. please drop > OK. > > > > > + */ > > > + if (speed == SPEED_1000) { > > > > > > + > > > +static struct mac_device_info *loongson_dwmac_setup(void *apriv) > > > +{ > > > + struct stmmac_priv *priv = apriv; > > > + struct mac_device_info *mac; > > seems unused. See my next comment. > No, We're using it. See my next reply. Right. Sorry for the misleading comment. Got confused the mac and the mac->mac parts. Of course I meant the usage and initialization of the "mac_device_info::mac" field. > > > > > + struct stmmac_dma_ops *dma; > > > + struct loongson_data *ld; > > > + struct pci_dev *pdev; > > > + > > > + ld = priv->plat->bsp_priv; > > > + pdev = to_pci_dev(priv->device); > > > + > > > + mac = devm_kzalloc(priv->device, sizeof(*mac), GFP_KERNEL); > > > + if (!mac) > > > + return NULL; > > I see you no longer override the ops in dwmac1000_ops. If so this can > > be dropped. > No, > > Because I pre-initialize the respective "mac" fields as it's done > in dwmac1000_setup(). You are right. > > > > > > + > > > + dma = devm_kzalloc(priv->device, sizeof(*dma), GFP_KERNEL); > > > + if (!dma) > > > + return NULL; > > > + > > > + /* The original IP-core version is 0x37 in all Loongson GNET > > s/0x37/v3.73a > Yeah! > > > > > + * (ls2k2000 and ls7a2000), but the GNET HW designers have changed the > > > + * GMAC_VERSION.SNPSVER field to the custom 0x10 value on the Loongson > > > + * ls2k2000 MAC to emphasize the differences: multiple DMA-channels, > > s/ls2k2000/LS2K2000 > > s/ls7a2000/LS7A2000 > OK. > > > > > + * AV feature and GMAC_INT_STATUS CSR flags layout. Get back the > > > + * original value so the correct HW-interface would be selected. > > > + */ > > > + if (ld->gmac_verion == LOONGSON_DWMAC_CORE_1_00) { > > > + priv->synopsys_id = DWMAC_CORE_3_70; > > > + *dma = dwmac1000_dma_ops; > > > + dma->init_chan = loongson_gnet_dma_init_channel; > > > + dma->dma_interrupt = loongson_gnet_dma_interrupt; > > > + mac->dma = dma; > > > + } > > > + > > > + mac->mac = &dwmac1000_ops; > > Unused? > Yeah, will be droped! That's what I meant. > > > > > + priv->dev->priv_flags |= IFF_UNICAST_FLT; > > > + > > > + /* Pre-initialize the respective "mac" fields as it's done in > > > + * dwmac1000_setup() > > > + */ > > > + mac->pcsr = priv->ioaddr; > > > + mac->multicast_filter_bins = priv->plat->multicast_filter_bins; > > > + mac->unicast_filter_entries = priv->plat->unicast_filter_entries; > > > + mac->mcast_bits_log2 = 0; > > > + > > > + if (mac->multicast_filter_bins) > > > + mac->mcast_bits_log2 = ilog2(mac->multicast_filter_bins); > > > + > > > + /* The GMAC devices with PCI ID 0x7a03 does not support any pause mode. > > > + * The GNET devices without CORE ID 0x10 does not support half-duplex. > > > + */ > > No need to mention the IDs but just the actual devices: > > /* Loongson GMAC doesn't support the flow control. LS2K2000 > > * GNET doesn't support the half-duplex link mode. > > */ > > > OK, Thanks. > > > + if (pdev->device == PCI_DEVICE_ID_LOONGSON_GMAC) { > > > + mac->link.caps = MAC_10 | MAC_100 | MAC_1000; > > > + } else { > > > + if (ld->gmac_verion == LOONGSON_DWMAC_CORE_1_00) > > > + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | > > > + MAC_10 | MAC_100 | MAC_1000; > > > + else > > > + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | > > > + MAC_10FD | MAC_100FD | MAC_1000FD; > > > + } > > > + > > > + mac->link.duplex = GMAC_CONTROL_DM; > > > + mac->link.speed10 = GMAC_CONTROL_PS; > > > + mac->link.speed100 = GMAC_CONTROL_PS | GMAC_CONTROL_FES; > > > + mac->link.speed1000 = 0; > > > + mac->link.speed_mask = GMAC_CONTROL_PS | GMAC_CONTROL_FES; > > > + mac->mii.addr = GMAC_MII_ADDR; > > > + mac->mii.data = GMAC_MII_DATA; > > > + mac->mii.addr_shift = 11; > > > + mac->mii.addr_mask = 0x0000F800; > > > + mac->mii.reg_shift = 6; > > > + mac->mii.reg_mask = 0x000007C0; > > > + mac->mii.clk_csr_shift = 2; > > > + mac->mii.clk_csr_mask = GENMASK(5, 2); > > > + > > > + return mac; > > > +} > > > + > > > static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > { > > > struct plat_stmmacenet_data *plat; > > > int ret, i, bus_id, phy_mode; > > > struct stmmac_pci_info *info; > > > struct stmmac_resources res; > > > + struct loongson_data *ld; > > > struct device_node *np; > > > np = dev_of_node(&pdev->dev); > > > @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > return -ENOMEM; > > > plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL); > > > - if (!plat->dma_cfg) { > > > - ret = -ENOMEM; > > > - goto err_put_node; > > > - } > > > + if (!plat->dma_cfg) > > > + return -ENOMEM; > > This change must have been introduced in the patch > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support > > which moves the mdio_node pointer initialization to under the if-clause. > OK. > > > > > + > > > + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); > > > + if (!ld) > > > + return -ENOMEM; > > > /* Enable pci device */ > > > ret = pci_enable_device(pdev); > > > @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > plat->phy_interface = phy_mode; > > > } > > > - pci_enable_msi(pdev); > > Hm, this must be justified and better being done in a separate patch. > OK. AFAICS the pci_enable_msi()/pci_disable_msi() calls can be dropped due to the Loongson GMAC not having the MSI IRQs delivered (at least that's what I got from the discussion and from the original driver, please correct my if I am wrong). Thus no need in the MSI capability being enabled. Meanwhile the multi-channels Loongson GNET will use the pci_alloc_irq_vectors()/pci_free_irq_vectors() functions for the IRQ vectors allocation and freeing which already perform the MSIs enable/disable by design. * But once again, please drop the functions call in a separate patch submitted with the proper commit log justifying the removal. > > > > > + plat->bsp_priv = ld; > > > + plat->setup = loongson_dwmac_setup; > > > + ld->dev = &pdev->dev; > > > + > > > memset(&res, 0, sizeof(res)); > > > res.addr = pcim_iomap_table(pdev)[0]; > > > + ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; > > > + > > > + switch (ld->gmac_verion) { > > > + case LOONGSON_DWMAC_CORE_1_00: > > > + plat->rx_queues_to_use = CHANNEL_NUM; > > > + plat->tx_queues_to_use = CHANNEL_NUM; > > > + > > > + /* Only channel 0 supports checksum, > > > + * so turn off checksum to enable multiple channels. > > > + */ > > > + for (i = 1; i < CHANNEL_NUM; i++) > > > + plat->tx_queues_cfg[i].coe_unsupported = 1; > > > - plat->tx_queues_to_use = 1; > > > - plat->rx_queues_to_use = 1; > > > + ret = loongson_dwmac_config_msi(pdev, plat, &res, np); > > > + break; > > > + default: /* 0x35 device and 0x37 device. */ > > > + plat->tx_queues_to_use = 1; > > > + plat->rx_queues_to_use = 1; > > Move the NoF queues (and coe flag) initializations to the respective > > loongson_*_data() methods. > OK. > > > > Besides I don't see you freeing the IRQ vectors allocated in the > > loongson_dwmac_config_msi() method neither in probe(), nor in remove() > > functions. That's definitely wrong. What you need is to have a > > method antagonistic to loongson_dwmac_config_msi() (like > > loongson_dwmac_clear_msi()) which would execute the cleanup procedure. > > Hmmm, We can free it in struct pci_driver ->remove method. > > Just in loongson_dwmac_remove() call > > pci_free_irq_vectors(pdev); Sounds good. Although I would have implemented that in a more maintainable way: loongson_dwmac_config_msi() { ... } loongson_dwmac_clear_msi() { pci_free_irq_vectors(pdev) } ... loongson_dwmac_remove() { ... if (ld->loongson_id == DWMAC_CORE_LS2K2000) loongson_dwmac_clear_msi(); ... } -Serge(y) > > > > > > > - ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > > + break; > > > + } > > > /* GNET devices with dev revision 0x00 do not support manually > > > * setting the speed to 1000. > > > @@ -189,12 +549,10 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > ret = stmmac_dvr_probe(&pdev->dev, plat, &res); > > > if (ret) > > > - goto err_disable_msi; > > > + goto err_disable_device; > > > return ret; > > > -err_disable_msi: > > > - pci_disable_msi(pdev); > > Once again. Justify the change. Moreover I don't see you dropping the > > pci_disable_msi() from the remove() method. > > Since we need to check the return value of the allocated msi, we will > > restore this change in v13. > > > Thanks, > > Yanteng >
Hi Serge, 在 2024/5/17 17:07, Serge Semin 写道: > On Fri, May 17, 2024 at 04:42:51PM +0800, Yanteng Si wrote: >> Hi Huacai, Serge, >> >> 在 2024/5/15 21:55, Huacai Chen 写道: >>>>>> Once again about the naming. From the retrospective point of view the >>>>>> so called legacy PCI IRQs (in fact PCI INTx) and the platform IRQs >>>>>> look similar because these are just the level-type signals connected >>>>>> to the system IRQ controller. But when it comes to the PCI_Express_, >>>>>> the implementation is completely different. The PCIe INTx is just the >>>>>> PCIe TLPs of special type, like MSI. Upon receiving these special >>>>>> messages the PCIe host controller delivers the IRQ up to the >>>>>> respective system IRQ controller. So in order to avoid the confusion >>>>>> between the actual legacy PCI INTx, PCI Express INTx and the just >>>>>> platform IRQs, it's better to emphasize the actual way of the IRQs >>>>>> delivery. In this case it's the later method. >>>>> You are absolutely right, and I think I found a method to use your >>>>> framework to solve our problems: >>>>> >>>>> static int loongson_dwmac_config_irqs(struct pci_dev *pdev, >>>>> struct plat_stmmacenet_data *plat, >>>>> struct stmmac_resources *res) >>>>> { >>>>> int i, ret, vecs; >>>>> >>>>> /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | >>>>> * --------- ----- -------- -------- ... -------- -------- >>>>> * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | >>>>> */ >>>>> vecs = plat->rx_queues_to_use + plat->tx_queues_to_use + 1; >>>>> ret = pci_alloc_irq_vectors(pdev, 1, vecs, PCI_IRQ_MSI | PCI_IRQ_INTX); >>>>> if (ret < 0) { >>>>> dev_err(&pdev->dev, "Failed to allocate PCI IRQs\n"); >>>>> return ret; >>>>> } >>>>> if (ret >= vecs) { >>>>> for (i = 0; i < plat->rx_queues_to_use; i++) { >>>>> res->rx_irq[CHANNELS_NUM - 1 - i] = >>>>> pci_irq_vector(pdev, 1 + i * 2); >>>>> } >>>>> for (i = 0; i < plat->tx_queues_to_use; i++) { >>>>> res->tx_irq[CHANNELS_NUM - 1 - i] = >>>>> pci_irq_vector(pdev, 2 + i * 2); >>>>> } >>>>> >>>>> plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; >>>>> } >>>>> >>>>> res->irq = pci_irq_vector(pdev, 0); >>>>> >>>>> if (np) { >>>>> res->irq = of_irq_get_byname(np, "macirq"); >>>>> if (res->irq < 0) { >>>>> dev_err(&pdev->dev, "IRQ macirq not found\n"); >>>>> return -ENODEV; >>>>> } >>>>> >>>>> res->wol_irq = of_irq_get_byname(np, "eth_wake_irq"); >>>>> if (res->wol_irq < 0) { >>>>> dev_info(&pdev->dev, >>>>> "IRQ eth_wake_irq not found, using macirq\n"); >>>>> res->wol_irq = res->irq; >>>>> } >>>>> >>>>> res->lpi_irq = of_irq_get_byname(np, "eth_lpi"); >>>>> if (res->lpi_irq < 0) { >>>>> dev_err(&pdev->dev, "IRQ eth_lpi not found\n"); >>>>> return -ENODEV; >>>>> } >>>>> } >>>>> return 0; >>>>> } >>>>> >>>>> If your agree, Yanteng can use this method in V13, then avoid furthur changes. >>>> Since yesterday I have been too relaxed sitting back to explain in >>>> detail the problems with the code above. Shortly speaking, no to the >>>> method designed as above. >>> This function is copy-paste from your version which you suggest to >>> Yanteng, and plus the fallback parts for DT. If you don't want to >>> discuss it any more, we can discuss after V13. > My conclusion is the same. no to _your_ (Huacai) version of the code. > I suggest to Huacai dig dipper in the function semantic and find out > the problems it has. Meanwhile I'll keep relaxing... > >>> BTW, we cannot remove "res->wol_irq = res->irq", because Loongson >>> GMAC/GNET indeed supports WoL. >> Okay, I will not drop it in v13. > Apparently Huacai isn't well familiar with what he is reviewing. Once > again the initialization is useless. Drop it. Hmm, to be honest, I'm still a little confused about this. When we first designed the driver, we looked at intel,See: $: vim drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c +953 static int stmmac_config_single_msi(struct pci_dev *pdev, struct plat_stmmacenet_data *plat, struct stmmac_resources *res) { int ret; ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES); if (ret < 0) { dev_info(&pdev->dev, "%s: Single IRQ enablement failed\n", __func__); return ret; } res->irq = pci_irq_vector(pdev, 0); res->wol_irq = res->irq; Why can't we do this? Intel Patch thread link <https://lore.kernel.org/netdev/20210316121823.18659-5-weifeng.voon@intel.com/> > >> All right. I have been preparing v13 and will send it as soon as possible. >> >> Let's continue the discussion in v13. Of course, I will copy the part that >> has >> >> not received a clear reply to v13. >> > Note the merge window has been opened and the 'net-next' tree is now > closed. So either you submit your series as RFC or wait for the window > being closed. > Ok, if I'm fast enough, I'll send an RFC to talk about msi and legacy. Thanks, Yanteng
Hi Serge, I can't access siyanteng@loongson.cn right now for some reason, so let's switch to siyanteng01@gmail.com. Serge Semin <fancer.lancer@gmail.com> 于2024年5月17日周五 17:48写道： > > > > + > > > > +static int loongson_gnet_dma_interrupt(struct stmmac_priv *priv, > > > > + void __iomem *ioaddr, > > > > + struct stmmac_extra_stats *x, > > > > + u32 chan, u32 dir) > > > > +{ > > > > + struct stmmac_pcpu_stats *stats = > > > ... > > > > + /* Clear the interrupt by writing a logic 1 to the CSR5[15-0] */ > > > 0x7ffff != CSR5[15-0] > > > > > Hmmm, It should be CSR5[19-0]? > > 0x7ffff = [18-0] > 0xfffff = [19-0] > > > > > BTW, 0x1ffff != CSR5[15-0], too. > > > > It should be CSR5[16-0], right? > > Right. If you wish to fix that in the original code, that has to be > done in a dedicated patch. OK. > > > > > + > > > > + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); > > > > + if (!ld) > > > > + return -ENOMEM; > > > > /* Enable pci device */ > > > > ret = pci_enable_device(pdev); > > > > @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id > > > > plat->phy_interface = phy_mode; > > > > } > > > > - pci_enable_msi(pdev); > > > > Hm, this must be justified and better being done in a separate patch. > > OK. > > AFAICS the pci_enable_msi()/pci_disable_msi() calls can be dropped due > to the Loongson GMAC not having the MSI IRQs delivered (at least > that's what I got from the discussion and from the original driver, > please correct my if I am wrong). Thus no need in the MSI capability > being enabled. Meanwhile the multi-channels Loongson GNET will use the > pci_alloc_irq_vectors()/pci_free_irq_vectors() functions for the IRQ > vectors allocation and freeing which already perform the MSIs > enable/disable by design. Ok, I will try. > * But once again, please drop the functions call in a separate patch > submitted with the proper commit log justifying the removal. Ok. > > > > > > > Besides I don't see you freeing the IRQ vectors allocated in the > > > loongson_dwmac_config_msi() method neither in probe(), nor in remove() > > > functions. That's definitely wrong. What you need is to have a > > > method antagonistic to loongson_dwmac_config_msi() (like > > > loongson_dwmac_clear_msi()) which would execute the cleanup procedure. > > > > > Hmmm, We can free it in struct pci_driver ->remove method. > > > > Just in loongson_dwmac_remove() call > > > > pci_free_irq_vectors(pdev); > > Sounds good. Although I would have implemented that in a more > maintainable way: > > loongson_dwmac_config_msi() > { > ... > } > > loongson_dwmac_clear_msi() > { > pci_free_irq_vectors(pdev) > } > > ... > > loongson_dwmac_remove() > { > ... > if (ld->loongson_id == DWMAC_CORE_LS2K2000) > loongson_dwmac_clear_msi(); > ... > } > loongson_dwmac_clear_msi() looks better, and you'll see it in RFC/v13. Thanks, Yanteng
On Fri, May 17, 2024 at 06:37:50PM +0800, Yanteng Si wrote: > Hi Serge, > > 在 2024/5/17 17:07, Serge Semin 写道: > > On Fri, May 17, 2024 at 04:42:51PM +0800, Yanteng Si wrote: > > > Hi Huacai, Serge, > > > > > > 在 2024/5/15 21:55, Huacai Chen 写道: > > > > > > > Once again about the naming. From the retrospective point of view the > > > > > > > so called legacy PCI IRQs (in fact PCI INTx) and the platform IRQs > > > > > > > look similar because these are just the level-type signals connected > > > > > > > to the system IRQ controller. But when it comes to the PCI_Express_, > > > > > > > the implementation is completely different. The PCIe INTx is just the > > > > > > > PCIe TLPs of special type, like MSI. Upon receiving these special > > > > > > > messages the PCIe host controller delivers the IRQ up to the > > > > > > > respective system IRQ controller. So in order to avoid the confusion > > > > > > > between the actual legacy PCI INTx, PCI Express INTx and the just > > > > > > > platform IRQs, it's better to emphasize the actual way of the IRQs > > > > > > > delivery. In this case it's the later method. > > > > > > You are absolutely right, and I think I found a method to use your > > > > > > framework to solve our problems: > > > > > > > > > > > > static int loongson_dwmac_config_irqs(struct pci_dev *pdev, > > > > > > struct plat_stmmacenet_data *plat, > > > > > > struct stmmac_resources *res) > > > > > > { > > > > > > int i, ret, vecs; > > > > > > > > > > > > /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > > > > > > * --------- ----- -------- -------- ... -------- -------- > > > > > > * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > > > > > > */ > > > > > > vecs = plat->rx_queues_to_use + plat->tx_queues_to_use + 1; > > > > > > ret = pci_alloc_irq_vectors(pdev, 1, vecs, PCI_IRQ_MSI | PCI_IRQ_INTX); > > > > > > if (ret < 0) { > > > > > > dev_err(&pdev->dev, "Failed to allocate PCI IRQs\n"); > > > > > > return ret; > > > > > > } > > > > > > if (ret >= vecs) { > > > > > > for (i = 0; i < plat->rx_queues_to_use; i++) { > > > > > > res->rx_irq[CHANNELS_NUM - 1 - i] = > > > > > > pci_irq_vector(pdev, 1 + i * 2); > > > > > > } > > > > > > for (i = 0; i < plat->tx_queues_to_use; i++) { > > > > > > res->tx_irq[CHANNELS_NUM - 1 - i] = > > > > > > pci_irq_vector(pdev, 2 + i * 2); > > > > > > } > > > > > > > > > > > > plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > > > > > > } > > > > > > > > > > > > res->irq = pci_irq_vector(pdev, 0); > > > > > > > > > > > > if (np) { > > > > > > res->irq = of_irq_get_byname(np, "macirq"); > > > > > > if (res->irq < 0) { > > > > > > dev_err(&pdev->dev, "IRQ macirq not found\n"); > > > > > > return -ENODEV; > > > > > > } > > > > > > > > > > > > res->wol_irq = of_irq_get_byname(np, "eth_wake_irq"); > > > > > > if (res->wol_irq < 0) { > > > > > > dev_info(&pdev->dev, > > > > > > "IRQ eth_wake_irq not found, using macirq\n"); > > > > > > res->wol_irq = res->irq; > > > > > > } > > > > > > > > > > > > res->lpi_irq = of_irq_get_byname(np, "eth_lpi"); > > > > > > if (res->lpi_irq < 0) { > > > > > > dev_err(&pdev->dev, "IRQ eth_lpi not found\n"); > > > > > > return -ENODEV; > > > > > > } > > > > > > } > > > > > > return 0; > > > > > > } > > > > > > > > > > > > If your agree, Yanteng can use this method in V13, then avoid furthur changes. > > > > > Since yesterday I have been too relaxed sitting back to explain in > > > > > detail the problems with the code above. Shortly speaking, no to the > > > > > method designed as above. > > > > This function is copy-paste from your version which you suggest to > > > > Yanteng, and plus the fallback parts for DT. If you don't want to > > > > discuss it any more, we can discuss after V13. > > My conclusion is the same. no to _your_ (Huacai) version of the code. > > I suggest to Huacai dig dipper in the function semantic and find out > > the problems it has. Meanwhile I'll keep relaxing... > > > > > > BTW, we cannot remove "res->wol_irq = res->irq", because Loongson > > > > GMAC/GNET indeed supports WoL. > > > Okay, I will not drop it in v13. > > Apparently Huacai isn't well familiar with what he is reviewing. Once > > again the initialization is useless. Drop it. > > Hmm, to be honest, I'm still a little confused about this. > > When we first designed the driver, we looked at intel,See: > > $: vim drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c +953 > > static int stmmac_config_single_msi(struct pci_dev *pdev, > struct plat_stmmacenet_data *plat, > struct stmmac_resources *res) > { > int ret; > > ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES); > if (ret < 0) { > dev_info(&pdev->dev, "%s: Single IRQ enablement failed\n", > __func__); > return ret; > } > > res->irq = pci_irq_vector(pdev, 0); > res->wol_irq = res->irq; > > Why can't we do this? > > Intel Patch thread link <https://lore.kernel.org/netdev/20210316121823.18659-5-weifeng.voon@intel.com/> First of all the Intel' STMMAC patches isn't something what can be referred to as a good-practice example. A significant part of the mess in the plat_stmmacenet_data structure is their doing. Secondly as I already said several times initializing res->wol_irq with res->irq is _useless_. It's because of the way the WoL IRQ line is requested: stmmac_request_irq_single(struct net_device *dev) { ... if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) { ret = request_irq(priv->wol_irq, stmmac_interrupt, IRQF_SHARED, dev->name, dev); ... } ... } stmmac_request_irq_multi_msi(struct net_device *dev) { ... if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) { int_name = priv->int_name_wol; sprintf(int_name, "%s:%s", dev->name, "wol"); ret = request_irq(priv->wol_irq, stmmac_mac_interrupt, 0, int_name, dev); ... } ... } See, even if you initialize priv->wol_irq with dev->irq (res->irq) it will have the same effect as if you had it left uninitialized (pre-initialized with zero). So from both maintainability and readability points of view it's better to avoid a redundant code especially if it causes an ill coding practice reproduction. Interestingly to note that having res->wol_irq initialized with res->irq had been required before another Intel' commit: 8532f613bc78 ("net: stmmac: introduce MSI Interrupt routines for mac, safety, RX & TX") (submitted sometime around the commit you are referring to). In that commit Intel' developers themself fixed the semantics in the STMMAC core driver, but didn't bother with fixing the platform drivers and even the Intel' DWMAC PCI driver has been left with that redundant line of the code. Sigh... > > > > > > > All right. I have been preparing v13 and will send it as soon as possible. > > > > > > Let's continue the discussion in v13. Of course, I will copy the part that > > > has > > > > > > not received a clear reply to v13. > > > > > Note the merge window has been opened and the 'net-next' tree is now > > closed. So either you submit your series as RFC or wait for the window > > being closed. > > > Ok, if I'm fast enough, I'll send an RFC to talk about msi and legacy. It's up to you. But please be aware, I'll be busy next week with my own patches cooking up. So I won't be able to actively participate in your patches review. -Serge(y) > > > Thanks, > > Yanteng >
Serge Semin <fancer.lancer@gmail.com> 于2024年5月18日周六 00:37写道： > > On Fri, May 17, 2024 at 06:37:50PM +0800, Yanteng Si wrote: > > Hi Serge, > > > > 在 2024/5/17 17:07, Serge Semin 写道: > > > On Fri, May 17, 2024 at 04:42:51PM +0800, Yanteng Si wrote: > > > > Hi Huacai, Serge, > > > > > > > > 在 2024/5/15 21:55, Huacai Chen 写道: > > > > > > > > Once again about the naming. From the retrospective point of view the > > > > > > > > so called legacy PCI IRQs (in fact PCI INTx) and the platform IRQs > > > > > > > > look similar because these are just the level-type signals connected > > > > > > > > to the system IRQ controller. But when it comes to the PCI_Express_, > > > > > > > > the implementation is completely different. The PCIe INTx is just the > > > > > > > > PCIe TLPs of special type, like MSI. Upon receiving these special > > > > > > > > messages the PCIe host controller delivers the IRQ up to the > > > > > > > > respective system IRQ controller. So in order to avoid the confusion > > > > > > > > between the actual legacy PCI INTx, PCI Express INTx and the just > > > > > > > > platform IRQs, it's better to emphasize the actual way of the IRQs > > > > > > > > delivery. In this case it's the later method. > > > > > > > You are absolutely right, and I think I found a method to use your > > > > > > > framework to solve our problems: > > > > > > > > > > > > > > static int loongson_dwmac_config_irqs(struct pci_dev *pdev, > > > > > > > struct plat_stmmacenet_data *plat, > > > > > > > struct stmmac_resources *res) > > > > > > > { > > > > > > > int i, ret, vecs; > > > > > > > > > > > > > > /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | > > > > > > > * --------- ----- -------- -------- ... -------- -------- > > > > > > > * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | > > > > > > > */ > > > > > > > vecs = plat->rx_queues_to_use + plat->tx_queues_to_use + 1; > > > > > > > ret = pci_alloc_irq_vectors(pdev, 1, vecs, PCI_IRQ_MSI | PCI_IRQ_INTX); > > > > > > > if (ret < 0) { > > > > > > > dev_err(&pdev->dev, "Failed to allocate PCI IRQs\n"); > > > > > > > return ret; > > > > > > > } > > > > > > > if (ret >= vecs) { > > > > > > > for (i = 0; i < plat->rx_queues_to_use; i++) { > > > > > > > res->rx_irq[CHANNELS_NUM - 1 - i] = > > > > > > > pci_irq_vector(pdev, 1 + i * 2); > > > > > > > } > > > > > > > for (i = 0; i < plat->tx_queues_to_use; i++) { > > > > > > > res->tx_irq[CHANNELS_NUM - 1 - i] = > > > > > > > pci_irq_vector(pdev, 2 + i * 2); > > > > > > > } > > > > > > > > > > > > > > plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; > > > > > > > } > > > > > > > > > > > > > > res->irq = pci_irq_vector(pdev, 0); > > > > > > > > > > > > > > if (np) { > > > > > > > res->irq = of_irq_get_byname(np, "macirq"); > > > > > > > if (res->irq < 0) { > > > > > > > dev_err(&pdev->dev, "IRQ macirq not found\n"); > > > > > > > return -ENODEV; > > > > > > > } > > > > > > > > > > > > > > res->wol_irq = of_irq_get_byname(np, "eth_wake_irq"); > > > > > > > if (res->wol_irq < 0) { > > > > > > > dev_info(&pdev->dev, > > > > > > > "IRQ eth_wake_irq not found, using macirq\n"); > > > > > > > res->wol_irq = res->irq; > > > > > > > } > > > > > > > > > > > > > > res->lpi_irq = of_irq_get_byname(np, "eth_lpi"); > > > > > > > if (res->lpi_irq < 0) { > > > > > > > dev_err(&pdev->dev, "IRQ eth_lpi not found\n"); > > > > > > > return -ENODEV; > > > > > > > } > > > > > > > } > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > If your agree, Yanteng can use this method in V13, then avoid furthur changes. > > > > > > Since yesterday I have been too relaxed sitting back to explain in > > > > > > detail the problems with the code above. Shortly speaking, no to the > > > > > > method designed as above. > > > > > This function is copy-paste from your version which you suggest to > > > > > Yanteng, and plus the fallback parts for DT. If you don't want to > > > > > discuss it any more, we can discuss after V13. > > > My conclusion is the same. no to _your_ (Huacai) version of the code. > > > I suggest to Huacai dig dipper in the function semantic and find out > > > the problems it has. Meanwhile I'll keep relaxing... > > > > > > > > BTW, we cannot remove "res->wol_irq = res->irq", because Loongson > > > > > GMAC/GNET indeed supports WoL. > > > > Okay, I will not drop it in v13. > > > Apparently Huacai isn't well familiar with what he is reviewing. Once > > > again the initialization is useless. Drop it. > > > > > Hmm, to be honest, I'm still a little confused about this. > > > > When we first designed the driver, we looked at intel,See: > > > > $: vim drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c +953 > > > > static int stmmac_config_single_msi(struct pci_dev *pdev, > > struct plat_stmmacenet_data *plat, > > struct stmmac_resources *res) > > { > > int ret; > > > > ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES); > > if (ret < 0) { > > dev_info(&pdev->dev, "%s: Single IRQ enablement failed\n", > > __func__); > > return ret; > > } > > > > res->irq = pci_irq_vector(pdev, 0); > > res->wol_irq = res->irq; > > > > Why can't we do this? > > > > Intel Patch thread link <https://lore.kernel.org/netdev/20210316121823.18659-5-weifeng.voon@intel.com/> > > First of all the Intel' STMMAC patches isn't something what can be > referred to as a good-practice example. A significant part of the > mess in the plat_stmmacenet_data structure is their doing. > > Secondly as I already said several times initializing res->wol_irq > with res->irq is _useless_. It's because of the way the WoL IRQ line > is requested: I see, res->irq will be droped. Thanks. > > stmmac_request_irq_single(struct net_device *dev) > { > ... > if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) { > ret = request_irq(priv->wol_irq, stmmac_interrupt, > IRQF_SHARED, dev->name, dev); > ... > } > ... > } > > stmmac_request_irq_multi_msi(struct net_device *dev) > { > ... > if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) { > int_name = priv->int_name_wol; > sprintf(int_name, "%s:%s", dev->name, "wol"); > ret = request_irq(priv->wol_irq, > stmmac_mac_interrupt, > 0, int_name, dev); > ... > } > ... > } > > See, even if you initialize priv->wol_irq with dev->irq (res->irq) it > will have the same effect as if you had it left uninitialized > (pre-initialized with zero). So from both maintainability and > readability points of view it's better to avoid a redundant code > especially if it causes an ill coding practice reproduction. Oh, I see. Thank you！ > > > Interestingly to note that having res->wol_irq initialized with > res->irq had been required before another Intel' commit: > 8532f613bc78 ("net: stmmac: introduce MSI Interrupt routines for mac, safety, RX & TX") > (submitted sometime around the commit you are referring to). > In that commit Intel' developers themself fixed the semantics in the > STMMAC core driver, but didn't bother with fixing the platform drivers > and even the Intel' DWMAC PCI driver has been left with that redundant > line of the code. Sigh... > > > Ok, if I'm fast enough, I'll send an RFC to talk about msi and legacy. > > It's up to you. But please be aware, I'll be busy next week with my > own patches cooking up. So I won't be able to actively participate in > your patches review. Okay, maybe it would be better to send v13 after the window closes. Thanks, Yanteng
diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h index 9cd62b2110a1..aed6ae80cc7c 100644 --- a/drivers/net/ethernet/stmicro/stmmac/common.h +++ b/drivers/net/ethernet/stmicro/stmmac/common.h @@ -29,6 +29,7 @@ /* Synopsys Core versions */ #define DWMAC_CORE_3_40 0x34 #define DWMAC_CORE_3_50 0x35 +#define DWMAC_CORE_3_70 0x37 #define DWMAC_CORE_4_00 0x40 #define DWMAC_CORE_4_10 0x41 #define DWMAC_CORE_5_00 0x50 diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c index a16bba389417..68de90c44feb 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c @@ -8,9 +8,71 @@ #include <linux/device.h> #include <linux/of_irq.h> #include "stmmac.h" +#include "dwmac_dma.h" +#include "dwmac1000.h" + +/* Normal Loongson Tx Summary */ +#define DMA_INTR_ENA_NIE_TX_LOONGSON 0x00040000 +/* Normal Loongson Rx Summary */ +#define DMA_INTR_ENA_NIE_RX_LOONGSON 0x00020000 + +#define DMA_INTR_NORMAL_LOONGSON (DMA_INTR_ENA_NIE_TX_LOONGSON | \ + DMA_INTR_ENA_NIE_RX_LOONGSON | \ + DMA_INTR_ENA_RIE | DMA_INTR_ENA_TIE) + +/* Abnormal Loongson Tx Summary */ +#define DMA_INTR_ENA_AIE_TX_LOONGSON 0x00010000 +/* Abnormal Loongson Rx Summary */ +#define DMA_INTR_ENA_AIE_RX_LOONGSON 0x00008000 + +#define DMA_INTR_ABNORMAL_LOONGSON (DMA_INTR_ENA_AIE_TX_LOONGSON | \ + DMA_INTR_ENA_AIE_RX_LOONGSON | \ + DMA_INTR_ENA_FBE | DMA_INTR_ENA_UNE) + +#define DMA_INTR_DEFAULT_MASK_LOONGSON (DMA_INTR_NORMAL_LOONGSON | \ + DMA_INTR_ABNORMAL_LOONGSON) + +/* Normal Loongson Tx Interrupt Summary */ +#define DMA_STATUS_NIS_TX_LOONGSON 0x00040000 +/* Normal Loongson Rx Interrupt Summary */ +#define DMA_STATUS_NIS_RX_LOONGSON 0x00020000 + +/* Abnormal Loongson Tx Interrupt Summary */ +#define DMA_STATUS_AIS_TX_LOONGSON 0x00010000 +/* Abnormal Loongson Rx Interrupt Summary */ +#define DMA_STATUS_AIS_RX_LOONGSON 0x00008000 + +/* Fatal Loongson Tx Bus Error Interrupt */ +#define DMA_STATUS_FBI_TX_LOONGSON 0x00002000 +/* Fatal Loongson Rx Bus Error Interrupt */ +#define DMA_STATUS_FBI_RX_LOONGSON 0x00001000 + +#define DMA_STATUS_MSK_COMMON_LOONGSON (DMA_STATUS_NIS_TX_LOONGSON | \ + DMA_STATUS_NIS_RX_LOONGSON | \ + DMA_STATUS_AIS_TX_LOONGSON | \ + DMA_STATUS_AIS_RX_LOONGSON | \ + DMA_STATUS_FBI_TX_LOONGSON | \ + DMA_STATUS_FBI_RX_LOONGSON) + +#define DMA_STATUS_MSK_RX_LOONGSON (DMA_STATUS_ERI | DMA_STATUS_RWT | \ + DMA_STATUS_RPS | DMA_STATUS_RU | \ + DMA_STATUS_RI | DMA_STATUS_OVF | \ + DMA_STATUS_MSK_COMMON_LOONGSON) + +#define DMA_STATUS_MSK_TX_LOONGSON (DMA_STATUS_ETI | DMA_STATUS_UNF | \ + DMA_STATUS_TJT | DMA_STATUS_TU | \ + DMA_STATUS_TPS | DMA_STATUS_TI | \ + DMA_STATUS_MSK_COMMON_LOONGSON) #define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03 #define PCI_DEVICE_ID_LOONGSON_GNET 0x7a13 +#define LOONGSON_DWMAC_CORE_1_00 0x10 /* Loongson custom IP */ +#define CHANNEL_NUM 8 + +struct loongson_data { + u32 gmac_verion; + struct device *dev; +}; struct stmmac_pci_info { int (*setup)(struct pci_dev *pdev, struct plat_stmmacenet_data *plat); @@ -69,6 +131,168 @@ static struct stmmac_pci_info loongson_gmac_pci_info = { .setup = loongson_gmac_data, }; +static void loongson_gnet_dma_init_channel(struct stmmac_priv *priv, + void __iomem *ioaddr, + struct stmmac_dma_cfg *dma_cfg, + u32 chan) +{ + int txpbl = dma_cfg->txpbl ?: dma_cfg->pbl; + int rxpbl = dma_cfg->rxpbl ?: dma_cfg->pbl; + u32 value; + + value = readl(ioaddr + DMA_CHAN_BUS_MODE(chan)); + + if (dma_cfg->pblx8) + value |= DMA_BUS_MODE_MAXPBL; + + value |= DMA_BUS_MODE_USP; + value &= ~(DMA_BUS_MODE_PBL_MASK | DMA_BUS_MODE_RPBL_MASK); + value |= (txpbl << DMA_BUS_MODE_PBL_SHIFT); + value |= (rxpbl << DMA_BUS_MODE_RPBL_SHIFT); + + /* Set the Fixed burst mode */ + if (dma_cfg->fixed_burst) + value |= DMA_BUS_MODE_FB; + + /* Mixed Burst has no effect when fb is set */ + if (dma_cfg->mixed_burst) + value |= DMA_BUS_MODE_MB; + + if (dma_cfg->atds) + value |= DMA_BUS_MODE_ATDS; + + if (dma_cfg->aal) + value |= DMA_BUS_MODE_AAL; + + writel(value, ioaddr + DMA_CHAN_BUS_MODE(chan)); + + /* Mask interrupts by writing to CSR7 */ + writel(DMA_INTR_DEFAULT_MASK_LOONGSON, ioaddr + + DMA_CHAN_INTR_ENA(chan)); +} + +static int loongson_gnet_dma_interrupt(struct stmmac_priv *priv, + void __iomem *ioaddr, + struct stmmac_extra_stats *x, + u32 chan, u32 dir) +{ + struct stmmac_pcpu_stats *stats = this_cpu_ptr(priv->xstats.pcpu_stats); + u32 abnor_intr_status; + u32 nor_intr_status; + u32 fb_intr_status; + u32 intr_status; + int ret = 0; + + /* read the status register (CSR5) */ + intr_status = readl(ioaddr + DMA_CHAN_STATUS(chan)); + + if (dir == DMA_DIR_RX) + intr_status &= DMA_STATUS_MSK_RX_LOONGSON; + else if (dir == DMA_DIR_TX) + intr_status &= DMA_STATUS_MSK_TX_LOONGSON; + + nor_intr_status = intr_status & (DMA_STATUS_NIS_TX_LOONGSON | + DMA_STATUS_NIS_RX_LOONGSON); + abnor_intr_status = intr_status & (DMA_STATUS_AIS_TX_LOONGSON | + DMA_STATUS_AIS_RX_LOONGSON); + fb_intr_status = intr_status & (DMA_STATUS_FBI_TX_LOONGSON | + DMA_STATUS_FBI_RX_LOONGSON); + + /* ABNORMAL interrupts */ + if (unlikely(abnor_intr_status)) { + if (unlikely(intr_status & DMA_STATUS_UNF)) { + ret = tx_hard_error_bump_tc; + x->tx_undeflow_irq++; + } + if (unlikely(intr_status & DMA_STATUS_TJT)) + x->tx_jabber_irq++; + if (unlikely(intr_status & DMA_STATUS_OVF)) + x->rx_overflow_irq++; + if (unlikely(intr_status & DMA_STATUS_RU)) + x->rx_buf_unav_irq++; + if (unlikely(intr_status & DMA_STATUS_RPS)) + x->rx_process_stopped_irq++; + if (unlikely(intr_status & DMA_STATUS_RWT)) + x->rx_watchdog_irq++; + if (unlikely(intr_status & DMA_STATUS_ETI)) + x->tx_early_irq++; + if (unlikely(intr_status & DMA_STATUS_TPS)) { + x->tx_process_stopped_irq++; + ret = tx_hard_error; + } + if (unlikely(fb_intr_status)) { + x->fatal_bus_error_irq++; + ret = tx_hard_error; + } + } + /* TX/RX NORMAL interrupts */ + if (likely(nor_intr_status)) { + if (likely(intr_status & DMA_STATUS_RI)) { + u32 value = readl(ioaddr + DMA_INTR_ENA); + /* to schedule NAPI on real RIE event. */ + if (likely(value & DMA_INTR_ENA_RIE)) { + u64_stats_update_begin(&stats->syncp); + u64_stats_inc(&stats->rx_normal_irq_n[chan]); + u64_stats_update_end(&stats->syncp); + ret |= handle_rx; + } + } + if (likely(intr_status & DMA_STATUS_TI)) { + u64_stats_update_begin(&stats->syncp); + u64_stats_inc(&stats->tx_normal_irq_n[chan]); + u64_stats_update_end(&stats->syncp); + ret |= handle_tx; + } + if (unlikely(intr_status & DMA_STATUS_ERI)) + x->rx_early_irq++; + } + /* Optional hardware blocks, interrupts should be disabled */ + if (unlikely(intr_status & + (DMA_STATUS_GPI | DMA_STATUS_GMI | DMA_STATUS_GLI))) + pr_warn("%s: unexpected status %08x\n", __func__, intr_status); + + /* Clear the interrupt by writing a logic 1 to the CSR5[15-0] */ + writel((intr_status & 0x7ffff), ioaddr + DMA_CHAN_STATUS(chan)); + + return ret; +} + +static void loongson_gnet_fix_speed(void *priv, unsigned int speed, + unsigned int mode) +{ + struct loongson_data *ld = (struct loongson_data *)priv; + struct net_device *ndev = dev_get_drvdata(ld->dev); + struct stmmac_priv *ptr = netdev_priv(ndev); + + /* The controller and PHY don't work well together. + * We need to use the PS bit to check if the controller's status + * is correct and reset PHY if necessary. + * MAC_CTRL_REG.15 is defined by the GMAC_CONTROL_PS macro. + */ + if (speed == SPEED_1000) { + if (readl(ptr->ioaddr + MAC_CTRL_REG) & + GMAC_CONTROL_PS) + /* Word around hardware bug, restart autoneg */ + phy_restart_aneg(ndev->phydev); + } +} + +static int loongson_gnet_data(struct pci_dev *pdev, + struct plat_stmmacenet_data *plat) +{ + loongson_default_data(pdev, plat); + + plat->phy_interface = PHY_INTERFACE_MODE_GMII; + plat->mdio_bus_data->phy_mask = ~(u32)BIT(2); + plat->fix_mac_speed = loongson_gnet_fix_speed; + + return 0; +} + +static struct stmmac_pci_info loongson_gnet_pci_info = { + .setup = loongson_gnet_data, +}; + static int loongson_dwmac_config_legacy(struct pci_dev *pdev, struct plat_stmmacenet_data *plat, struct stmmac_resources *res, @@ -101,12 +325,126 @@ static int loongson_dwmac_config_legacy(struct pci_dev *pdev, return 0; } +static int loongson_dwmac_config_msi(struct pci_dev *pdev, + struct plat_stmmacenet_data *plat, + struct stmmac_resources *res, + struct device_node *np) +{ + int i, ret, vecs; + + vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1); + ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI); + if (ret < 0) { + dev_info(&pdev->dev, + "MSI enable failed, Fallback to legacy interrupt\n"); + return loongson_dwmac_config_legacy(pdev, plat, res, np); + } + + res->irq = pci_irq_vector(pdev, 0); + res->wol_irq = 0; + + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx | + * --------- ----- -------- -------- ... -------- -------- + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 | + */ + for (i = 0; i < CHANNEL_NUM; i++) { + res->rx_irq[CHANNEL_NUM - 1 - i] = + pci_irq_vector(pdev, 1 + i * 2); + res->tx_irq[CHANNEL_NUM - 1 - i] = + pci_irq_vector(pdev, 2 + i * 2); + } + + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN; + + return 0; +} + +static struct mac_device_info *loongson_dwmac_setup(void *apriv) +{ + struct stmmac_priv *priv = apriv; + struct mac_device_info *mac; + struct stmmac_dma_ops *dma; + struct loongson_data *ld; + struct pci_dev *pdev; + + ld = priv->plat->bsp_priv; + pdev = to_pci_dev(priv->device); + + mac = devm_kzalloc(priv->device, sizeof(*mac), GFP_KERNEL); + if (!mac) + return NULL; + + dma = devm_kzalloc(priv->device, sizeof(*dma), GFP_KERNEL); + if (!dma) + return NULL; + + /* The original IP-core version is 0x37 in all Loongson GNET + * (ls2k2000 and ls7a2000), but the GNET HW designers have changed the + * GMAC_VERSION.SNPSVER field to the custom 0x10 value on the Loongson + * ls2k2000 MAC to emphasize the differences: multiple DMA-channels, + * AV feature and GMAC_INT_STATUS CSR flags layout. Get back the + * original value so the correct HW-interface would be selected. + */ + if (ld->gmac_verion == LOONGSON_DWMAC_CORE_1_00) { + priv->synopsys_id = DWMAC_CORE_3_70; + *dma = dwmac1000_dma_ops; + dma->init_chan = loongson_gnet_dma_init_channel; + dma->dma_interrupt = loongson_gnet_dma_interrupt; + mac->dma = dma; + } + + mac->mac = &dwmac1000_ops; + priv->dev->priv_flags |= IFF_UNICAST_FLT; + + /* Pre-initialize the respective "mac" fields as it's done in + * dwmac1000_setup() + */ + mac->pcsr = priv->ioaddr; + mac->multicast_filter_bins = priv->plat->multicast_filter_bins; + mac->unicast_filter_entries = priv->plat->unicast_filter_entries; + mac->mcast_bits_log2 = 0; + + if (mac->multicast_filter_bins) + mac->mcast_bits_log2 = ilog2(mac->multicast_filter_bins); + + /* The GMAC devices with PCI ID 0x7a03 does not support any pause mode. + * The GNET devices without CORE ID 0x10 does not support half-duplex. + */ + if (pdev->device == PCI_DEVICE_ID_LOONGSON_GMAC) { + mac->link.caps = MAC_10 | MAC_100 | MAC_1000; + } else { + if (ld->gmac_verion == LOONGSON_DWMAC_CORE_1_00) + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | + MAC_10 | MAC_100 | MAC_1000; + else + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | + MAC_10FD | MAC_100FD | MAC_1000FD; + } + + mac->link.duplex = GMAC_CONTROL_DM; + mac->link.speed10 = GMAC_CONTROL_PS; + mac->link.speed100 = GMAC_CONTROL_PS | GMAC_CONTROL_FES; + mac->link.speed1000 = 0; + mac->link.speed_mask = GMAC_CONTROL_PS | GMAC_CONTROL_FES; + mac->mii.addr = GMAC_MII_ADDR; + mac->mii.data = GMAC_MII_DATA; + mac->mii.addr_shift = 11; + mac->mii.addr_mask = 0x0000F800; + mac->mii.reg_shift = 6; + mac->mii.reg_mask = 0x000007C0; + mac->mii.clk_csr_shift = 2; + mac->mii.clk_csr_mask = GENMASK(5, 2); + + return mac; +} + static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id) { struct plat_stmmacenet_data *plat; int ret, i, bus_id, phy_mode; struct stmmac_pci_info *info; struct stmmac_resources res; + struct loongson_data *ld; struct device_node *np; np = dev_of_node(&pdev->dev); @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id return -ENOMEM; plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL); - if (!plat->dma_cfg) { - ret = -ENOMEM; - goto err_put_node; - } + if (!plat->dma_cfg) + return -ENOMEM; + + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL); + if (!ld) + return -ENOMEM; /* Enable pci device */ ret = pci_enable_device(pdev); @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id plat->phy_interface = phy_mode; } - pci_enable_msi(pdev); + plat->bsp_priv = ld; + plat->setup = loongson_dwmac_setup; + ld->dev = &pdev->dev; + memset(&res, 0, sizeof(res)); res.addr = pcim_iomap_table(pdev)[0]; + ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff; + + switch (ld->gmac_verion) { + case LOONGSON_DWMAC_CORE_1_00: + plat->rx_queues_to_use = CHANNEL_NUM; + plat->tx_queues_to_use = CHANNEL_NUM; + + /* Only channel 0 supports checksum, + * so turn off checksum to enable multiple channels. + */ + for (i = 1; i < CHANNEL_NUM; i++) + plat->tx_queues_cfg[i].coe_unsupported = 1; - plat->tx_queues_to_use = 1; - plat->rx_queues_to_use = 1; + ret = loongson_dwmac_config_msi(pdev, plat, &res, np); + break; + default: /* 0x35 device and 0x37 device. */ + plat->tx_queues_to_use = 1; + plat->rx_queues_to_use = 1; - ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); + break; + } /* GNET devices with dev revision 0x00 do not support manually * setting the speed to 1000. @@ -189,12 +549,10 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id ret = stmmac_dvr_probe(&pdev->dev, plat, &res); if (ret) - goto err_disable_msi; + goto err_disable_device; return ret; -err_disable_msi: - pci_disable_msi(pdev); err_disable_device: pci_disable_device(pdev); err_put_node: @@ -262,6 +620,7 @@ static SIMPLE_DEV_PM_OPS(loongson_dwmac_pm_ops, loongson_dwmac_suspend, static const struct pci_device_id loongson_dwmac_id_table[] = { { PCI_DEVICE_DATA(LOONGSON, GMAC, &loongson_gmac_pci_info) }, + { PCI_DEVICE_DATA(LOONGSON, GNET, &loongson_gnet_pci_info) }, {} }; MODULE_DEVICE_TABLE(pci, loongson_dwmac_id_table);
There are two types of Loongson DWGMAC. The first type shares the same register definitions and has similar logic as dwmac1000. The second type uses several different register definitions, we think it is necessary to distinguish rx and tx, so we split these bits into two. Simply put, we split some single bit fields into double bits fileds: Name Tx Rx DMA_INTR_ENA_NIE = 0x00040000 | 0x00020000; DMA_INTR_ENA_AIE = 0x00010000 | 0x00008000; DMA_STATUS_NIS = 0x00040000 | 0x00020000; DMA_STATUS_AIS = 0x00010000 | 0x00008000; DMA_STATUS_FBI = 0x00002000 | 0x00001000; Therefore, when using, TX and RX must be set at the same time. How to use them: 1. Create the Loongson GNET-specific stmmac_dma_ops.dma_interrupt() stmmac_dma_ops.init_chan() methods in the dwmac-loongson.c driver. Adding all the Loongson-specific macros 2. Create a Loongson GNET-specific platform setup method with the next semantics: + allocate stmmac_dma_ops instance and initialize it with dwmac1000_dma_ops. + override the stmmac_dma_ops.{dma_interrupt, init_chan} with the pointers to the methods defined in 2. + allocate mac_device_info instance and initialize the mac_device_info.dma field with a pointer to the new stmmac_dma_ops instance. + initialize mac_device_info in a way it's done in dwmac1000_setup(). 3. Initialize plat_stmmacenet_data.setup() with the pointer to the method created in 2. GNET features: Speeds: 10/100/1000Mbps DMA-descriptors type: enhanced L3/L4 filters availability: support VLAN hash table filter: support PHY-interface: GMII Remote Wake-up support: support Mac Management Counters (MMC): support Number of additional MAC addresses: 5 MAC Hash-based filter: support Number of ash table size: 256 DMA chennel number: 0x10 device is 8 and 0x37 device is 1 Others: GNET integrates both MAC and PHY chips inside. GNET device: LS2K2000, LS7A2000, the chip connection between the mac and phy of these devices is not normal and requires two rounds of negotiation; LS7A2000 does not support half-duplex and multi-channel; To enable multi-channel on LS2K2000, you need to turn off hardware checksum. **Note**: Currently, only the LS2K2000's synopsys_id is 0x10, while the synopsys_id of other devices are 0x37. Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> Signed-off-by: Yanteng Si <siyanteng@loongson.cn> --- drivers/net/ethernet/stmicro/stmmac/common.h | 1 + .../ethernet/stmicro/stmmac/dwmac-loongson.c | 381 +++++++++++++++++- 2 files changed, 371 insertions(+), 11 deletions(-)