| Message ID | bec0d6bf78c0dcf4797a148e3509058e46ccdb13.1706601050.git.siyanteng@loongson.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | stmmac: Add Loongson platform support | expand |
On Tue, Jan 30, 2024 at 04:48:19PM +0800, Yanteng Si wrote: > Request allocation for MSI for specific versions. > Please elaborate what is actually done in the patch. What device version it is dedicated for (Loongson GNET?), what IRQs the patch adds, etc. BTW will GNET device work without this patch? If no you need to either merge it into the patch introducing the GNET-device support or place it before that patch (6/11). > Some features of Loongson platforms are bound to the GMAC_VERSION > register. We have to read its value in order to get the correct channel > number. This message seems misleading. I don't see you doing that in the patch below... > > Signed-off-by: Yanteng Si <siyanteng@loongson.cn> > Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> > --- > .../ethernet/stmicro/stmmac/dwmac-loongson.c | 57 +++++++++++++++---- > 1 file changed, 46 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > index 584f7322bd3e..60d0a122d7c9 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > @@ -98,10 +98,10 @@ static void dwlgmac_dma_init_channel(struct stmmac_priv *priv, > if (dma_cfg->aal) > value |= DMA_BUS_MODE_AAL; > > - writel(value, ioaddr + DMA_BUS_MODE); > + writel(value, ioaddr + DMA_CHAN_BUS_MODE(chan)); > > /* Mask interrupts by writing to CSR7 */ > - writel(DMA_INTR_DEFAULT_MASK_LOONGSON, ioaddr + DMA_INTR_ENA); > + writel(DMA_INTR_DEFAULT_MASK_LOONGSON, ioaddr + DMA_CHAN_INTR_ENA(chan)); Em, why is this here? There is a patch [PATCH net-next v8 05/11] net: stmmac: dwmac-loongson: Add Loongson-specific register definitions in this series which was supposed to introduce the fully functional GNET-specific callbacks. Move this change in there. > } > > static int dwlgmac_dma_interrupt(struct stmmac_priv *priv, void __iomem *ioaddr, > @@ -238,6 +238,45 @@ static int loongson_dwmac_config_legacy(struct pci_dev *pdev, > return 0; > } > > +static int loongson_dwmac_config_multi_msi(struct pci_dev *pdev, This method seems like the GNET-specific one. What about using the appropriate prefix then? > + struct plat_stmmacenet_data *plat, > + struct stmmac_resources *res, > + struct device_node *np, > + int channel_num) Why do you need this parametrization? Since this method is GNET-specific what about defining a macro with the channels amount and using it here? > +{ > + 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); In what conditions is this possible? Will the loongson_dwmac_config_legacy() method work in that case? Did you test it out? > + } > + > + plat->rx_queues_to_use = channel_num; > + plat->tx_queues_to_use = channel_num; This is supposed to be initialized in the setup() methods. Please move it to the dedicated patch. > + > + res->irq = pci_irq_vector(pdev, 0); > + res->wol_irq = res->irq; Once again. wol_irq is optional. If there is no dedicated WoL IRQ leave the field as zero. > + > + /* 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; > + dev_info(&pdev->dev, "%s: multi MSI enablement successful\n", __func__); What's the point in printing this message especially with the __func__ prefix? You'll always be able to figure out the allocated IRQs by means of procfs. I suggest to drop it. > + > + return 0; > +} > + > static void loongson_default_data(struct pci_dev *pdev, > struct plat_stmmacenet_data *plat) > { > @@ -296,11 +335,8 @@ static int loongson_gmac_config(struct pci_dev *pdev, > struct stmmac_resources *res, > struct device_node *np) > { > - int ret; > - > - ret = loongson_dwmac_config_legacy(pdev, plat, res, np); > > - return ret; > + return 0; > } > > static struct stmmac_pci_info loongson_gmac_pci_info = { > @@ -380,11 +416,7 @@ static int loongson_gnet_config(struct pci_dev *pdev, > struct stmmac_resources *res, > struct device_node *np) > { > - int ret; > - > - ret = loongson_dwmac_config_legacy(pdev, plat, res, np); > - > - return ret; > + return 0; > } Here you are dropping the changes you just introduced leaving the config() methods empty... Why not to place the loongson_dwmac_config_legacy() invocation in the probe() method right at the patches introducing the config() functions and not to add the config() callback in the first place? -Serge(y) > > static struct stmmac_pci_info loongson_gnet_pci_info = { > @@ -483,6 +515,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, > ld->dwlgmac_dma_ops.dma_interrupt = dwlgmac_dma_interrupt; > > plat->setup = loongson_setup; > + ret = loongson_dwmac_config_multi_msi(pdev, plat, &res, np, 8); > + } else { > + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > } > > plat->bsp_priv = ld; > -- > 2.31.4 >
在 2024/3/14 17:33, Yanteng Si 写道: > 在 2024/2/6 05:28, Serge Semin 写道: > > On Tue, Jan 30, 2024 at 04:48:19PM +0800, Yanteng Si wrote: > >> Request allocation for MSI for specific versions. > >> > > Please elaborate what is actually done in the patch. What device > > version it is dedicated for (Loongson GNET?), what IRQs the patch > > adds, etc. > gnet_device core IP IRQ > 7a2000 0x37 legacy > 2k2000 0x10 multi_msi > > > > BTW will GNET device work without this patch? If no you need to either > > merge it into the patch introducing the GNET-device support or place > > it before that patch (6/11). > OK, GNET device work need this patch. > > > >> Some features of Loongson platforms are bound to the GMAC_VERSION > >> register. We have to read its value in order to get the correct channel > >> number. > > This message seems misleading. I don't see you doing that in the patch below... > Because part of our gnet hardware (7a2000) core IP is 0x37 > > > >> Signed-off-by: Yanteng Si<siyanteng@loongson.cn> > >> Signed-off-by: Feiyang Chen<chenfeiyang@loongson.cn> > >> Signed-off-by: Yinggang Gu<guyinggang@loongson.cn> > >> --- > >> .../ethernet/stmicro/stmmac/dwmac-loongson.c | 57 +++++++++++++++---- > >> 1 file changed, 46 insertions(+), 11 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > >> index 584f7322bd3e..60d0a122d7c9 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > >> @@ -98,10 +98,10 @@ static void dwlgmac_dma_init_channel(struct stmmac_priv *priv, > >> if (dma_cfg->aal) > >> value |= DMA_BUS_MODE_AAL; > >> > > > >> - writel(value, ioaddr + DMA_BUS_MODE); > >> + writel(value, ioaddr + DMA_CHAN_BUS_MODE(chan)); > >> > >> /* Mask interrupts by writing to CSR7 */ > >> - writel(DMA_INTR_DEFAULT_MASK_LOONGSON, ioaddr + DMA_INTR_ENA); > >> + writel(DMA_INTR_DEFAULT_MASK_LOONGSON, ioaddr + DMA_CHAN_INTR_ENA(chan)); > > Em, why is this here? There is a patch > > [PATCH net-next v8 05/11] net: stmmac: dwmac-loongson: Add Loongson-specific register definitions > > in this series which was supposed to introduce the fully functional > > GNET-specific callbacks. Move this change in there. > OK. > > > >> } > >> > >> static int dwlgmac_dma_interrupt(struct stmmac_priv *priv, void __iomem *ioaddr, > >> @@ -238,6 +238,45 @@ static int loongson_dwmac_config_legacy(struct pci_dev *pdev, > >> return 0; > >> } > >> > >> +static int loongson_dwmac_config_multi_msi(struct pci_dev *pdev, > > This method seems like the GNET-specific one. What about using the > > appropriate prefix then? > OK. loongson_gnet_config_multi_msi() > > > > >> + struct plat_stmmacenet_data *plat, > >> + struct stmmac_resources *res, > >> + struct device_node *np, > >> + int channel_num) > > Why do you need this parametrization? Since this method is > > GNET-specific what about defining a macro with the channels amount and > > using it here? > > OK. > > #define CHANNEL_NUM 8 > > > > >> +{ > >> + 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); > > In what conditions is this possible? Will the > > loongson_dwmac_config_legacy() method work in that case? Did you test > > it out? > Failed to apply for msi interrupt when the interrupt number is not enough，For > example, there are a large number of devices。 > >> + } > >> + > >> + plat->rx_queues_to_use = channel_num; > >> + plat->tx_queues_to_use = channel_num; > > This is supposed to be initialized in the setup() methods. Please move > > it to the dedicated patch. > No, referring to my previous reply, only the 0x10 gnet device has 8 channels, > and the 0x37 device has a single channel. > > > >> + > >> + res->irq = pci_irq_vector(pdev, 0); > >> + res->wol_irq = res->irq; > > Once again. wol_irq is optional. If there is no dedicated WoL IRQ > > leave the field as zero. > OK. > > 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; > >> + dev_info(&pdev->dev, "%s: multi MSI enablement successful\n", __func__); > > What's the point in printing this message especially with the __func__ > > prefix? You'll always be able to figure out the allocated IRQs by > > means of procfs. I suggest to drop it. > > OK. > > > > >> + > >> + return 0; > >> +} > >> + > >> static void loongson_default_data(struct pci_dev *pdev, > >> struct plat_stmmacenet_data *plat) > >> { > >> @@ -296,11 +335,8 @@ static int loongson_gmac_config(struct pci_dev *pdev, > >> struct stmmac_resources *res, > >> struct device_node *np) > >> { > >> - int ret; > >> - > >> - ret = loongson_dwmac_config_legacy(pdev, plat, res, np); > >> > >> - return ret; > >> + return 0; > >> } > >> > >> static struct stmmac_pci_info loongson_gmac_pci_info = { > >> @@ -380,11 +416,7 @@ static int loongson_gnet_config(struct pci_dev *pdev, > >> struct stmmac_resources *res, > >> struct device_node *np) > >> { > >> - int ret; > >> - > >> - ret = loongson_dwmac_config_legacy(pdev, plat, res, np); > >> - > >> - return ret; > >> + return 0; > >> } > > Here you are dropping the changes you just introduced leaving the > > config() methods empty... Why not to place the > > loongson_dwmac_config_legacy() invocation in the probe() method right > > at the patches introducing the config() functions and not to add the > > config() callback in the first place? > OK, I will try. > > Thanks, > > Yanteng > > > > > -Serge(y) > > > >> > >> static struct stmmac_pci_info loongson_gnet_pci_info = { > >> @@ -483,6 +515,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, > >> ld->dwlgmac_dma_ops.dma_interrupt = dwlgmac_dma_interrupt; > >> > >> plat->setup = loongson_setup; > >> + ret = loongson_dwmac_config_multi_msi(pdev, plat, &res, np, 8); > >> + } else { > >> + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > >> } > >> > >> plat->bsp_priv = ld; > >> -- > >> 2.31.4 > >>
On Thu, Mar 14, 2024 at 09:13:58PM +0800, Yanteng Si wrote: > > 在 2024/3/14 17:33, Yanteng Si 写道: > > 在 2024/2/6 05:28, Serge Semin 写道: > > > On Tue, Jan 30, 2024 at 04:48:19PM +0800, Yanteng Si wrote: > > >> Request allocation for MSI for specific versions. > > >> > > > Please elaborate what is actually done in the patch. What device > > > version it is dedicated for (Loongson GNET?), what IRQs the patch > > > adds, etc. > > gnet_device core IP IRQ > > 7a2000 0x37 legacy > > 2k2000 0x10 multi_msi > > > > > > BTW will GNET device work without this patch? If no you need to either > > > merge it into the patch introducing the GNET-device support or place > > > it before that patch (6/11). > > OK, GNET device work need this patch. > > > >> Some features of Loongson platforms are bound to the GMAC_VERSION > > >> register. We have to read its value in order to get the correct channel > > >> number. > > > This message seems misleading. I don't see you doing that in the patch below... > > Because part of our gnet hardware (7a2000) core IP is 0x37 > > > > > >> Signed-off-by: Yanteng Si<siyanteng@loongson.cn> > > >> Signed-off-by: Feiyang Chen<chenfeiyang@loongson.cn> > > >> Signed-off-by: Yinggang Gu<guyinggang@loongson.cn> > > >> --- > > >> .../ethernet/stmicro/stmmac/dwmac-loongson.c | 57 +++++++++++++++---- > > >> 1 file changed, 46 insertions(+), 11 deletions(-) > > >> > > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > >> index 584f7322bd3e..60d0a122d7c9 100644 > > >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c > > >> @@ -98,10 +98,10 @@ static void dwlgmac_dma_init_channel(struct stmmac_priv *priv, > > >> if (dma_cfg->aal) > > >> value |= DMA_BUS_MODE_AAL; > > >> > > > >> - writel(value, ioaddr + DMA_BUS_MODE); > > >> + writel(value, ioaddr + DMA_CHAN_BUS_MODE(chan)); > > >> >> /* Mask interrupts by writing to CSR7 */ > > >> - writel(DMA_INTR_DEFAULT_MASK_LOONGSON, ioaddr + DMA_INTR_ENA); > > >> + writel(DMA_INTR_DEFAULT_MASK_LOONGSON, ioaddr + DMA_CHAN_INTR_ENA(chan)); > > > Em, why is this here? There is a patch > > > [PATCH net-next v8 05/11] net: stmmac: dwmac-loongson: Add Loongson-specific register definitions > > > in this series which was supposed to introduce the fully functional > > > GNET-specific callbacks. Move this change in there. > > OK. > > > > > >> } > > >> >> static int dwlgmac_dma_interrupt(struct stmmac_priv *priv, > > void __iomem *ioaddr, > > >> @@ -238,6 +238,45 @@ static int loongson_dwmac_config_legacy(struct pci_dev *pdev, > > >> return 0; > > >> } > > >> >> +static int loongson_dwmac_config_multi_msi(struct pci_dev > > *pdev, > > > This method seems like the GNET-specific one. What about using the > > > appropriate prefix then? > > OK. loongson_gnet_config_multi_msi() > > > > > > > >> + struct plat_stmmacenet_data *plat, > > >> + struct stmmac_resources *res, > > >> + struct device_node *np, > > >> + int channel_num) > > > Why do you need this parametrization? Since this method is > > > GNET-specific what about defining a macro with the channels amount and > > > using it here? > > > > OK. > > > > #define CHANNEL_NUM 8 > > > > > > > >> +{ > > >> + 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); > > > In what conditions is this possible? Will the > > > loongson_dwmac_config_legacy() method work in that case? Did you test > > > it out? > > Failed to apply for msi interrupt when the interrupt number is not enough，For > > example, there are a large number of devices。 Then those platforms will _require_ to have the DT-node specified. This will define the DT-bindings which I doubt you imply here. Am I wrong? Once again have you tested the loongson_dwmac_config_legacy() method working in the case of the pci_alloc_irq_vectors() failure? > > >> + } > > >> + > > >> + plat->rx_queues_to_use = channel_num; > > >> + plat->tx_queues_to_use = channel_num; > > > This is supposed to be initialized in the setup() methods. Please move > > > it to the dedicated patch. > > No, referring to my previous reply, only the 0x10 gnet device has 8 channels, > > and the 0x37 device has a single channel. Yes. You have a perfectly suitable method for it. It's loongson_gnet_data(). Init the number of channels there based on the value read from the GMAC_VERSION.SNPSVER field. Thus the loongson_gnet_config_multi_msi() will get to be more coherent setting up the MSI IRQs only. Am I missing something in the last two notes chunks regard? If yes, let's submit v9 as you see it. We'll get back to this part there then. -Serge(y) > > > > > >> + > > >> + res->irq = pci_irq_vector(pdev, 0); > > >> + res->wol_irq = res->irq; > > > Once again. wol_irq is optional. If there is no dedicated WoL IRQ > > > leave the field as zero. > > OK. > > > > 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; > > >> + dev_info(&pdev->dev, "%s: multi MSI enablement successful\n", __func__); > > > What's the point in printing this message especially with the __func__ > > > prefix? You'll always be able to figure out the allocated IRQs by > > > means of procfs. I suggest to drop it. > > > > OK. > > > > > > > >> + > > >> + return 0; > > >> +} > > >> + > > >> static void loongson_default_data(struct pci_dev *pdev, > > >> struct plat_stmmacenet_data *plat) > > >> { > > >> @@ -296,11 +335,8 @@ static int loongson_gmac_config(struct pci_dev *pdev, > > >> struct stmmac_resources *res, > > >> struct device_node *np) > > >> { > > >> - int ret; > > >> - > > >> - ret = loongson_dwmac_config_legacy(pdev, plat, res, np); > > >> >> - return ret; > > >> + return 0; > > >> } > > >> >> static struct stmmac_pci_info loongson_gmac_pci_info = { > > >> @@ -380,11 +416,7 @@ static int loongson_gnet_config(struct pci_dev *pdev, > > >> struct stmmac_resources *res, > > >> struct device_node *np) > > >> { > > >> - int ret; > > >> - > > >> - ret = loongson_dwmac_config_legacy(pdev, plat, res, np); > > >> - > > >> - return ret; > > >> + return 0; > > >> } > > > Here you are dropping the changes you just introduced leaving the > > > config() methods empty... Why not to place the > > > loongson_dwmac_config_legacy() invocation in the probe() method right > > > at the patches introducing the config() functions and not to add the > > > config() callback in the first place? > > OK, I will try. > > > > Thanks, > > > > Yanteng > > > > > > > > -Serge(y) > > > > > >> >> static struct stmmac_pci_info loongson_gnet_pci_info = { > > >> @@ -483,6 +515,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, > > >> ld->dwlgmac_dma_ops.dma_interrupt = dwlgmac_dma_interrupt; > > >> >> plat->setup = loongson_setup; > > >> + ret = loongson_dwmac_config_multi_msi(pdev, plat, &res, np, 8); > > >> + } else { > > >> + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); > > >> } > > >> >> plat->bsp_priv = ld; > > >> -- >> 2.31.4 > > >> >
>>>>> +{ >>>>> + 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); >>>> In what conditions is this possible? Will the >>>> loongson_dwmac_config_legacy() method work in that case? Did you test >>>> it out? I need to wait for special hardware and PMON for this. Please give me some time. > Then those platforms will _require_ to have the DT-node specified. This > will define the DT-bindings which I doubt you imply here. Am I wrong? > > Once again have you tested the loongson_dwmac_config_legacy() method > working in the case of the pci_alloc_irq_vectors() failure? Yes! I have tested it, it works in single channel mode. dmesg: [ 3.935203] mdio_bus stmmac-18:02: attached PHY driver [unbound] (mii_bus:phy_addr=stmmac-18:02, irq=POLL) [ 3.945625] dwmac-loongson-pci 0000:00:03.1: MSI enable failed, Fallback to legacy interrupt [ 3.954175] dwmac-loongson-pci 0000:00:03.1: User ID: 0xd1, Synopsys ID: 0x10 [ 3.973676] dwmac-loongson-pci 0000:00:03.1: DMA HW capability register supported [ 3.981135] dwmac-loongson-pci 0000:00:03.1: RX Checksum Offload Engine supported cat /proc/interrupt: 43: 0 0 PCH PIC 16 ahci[0000:00:08.0] 44: 0 0 PCH PIC 12 enp0s3f0 45: 0 0 PCH PIC 14 enp0s3f1 46: 16233 0 PCH PIC 17 enp0s3f2 47: 12698 0 PCH PIC 48 xhci-hcd:usb1 the irq number 46 is the falkback legacy irq. > > >>>>> + } >>>>> + >>>>> + plat->rx_queues_to_use = channel_num; >>>>> + plat->tx_queues_to_use = channel_num; >>>> This is supposed to be initialized in the setup() methods. Please move >>>> it to the dedicated patch. >>> No, referring to my previous reply, only the 0x10 gnet device has 8 channels, >>> and the 0x37 device has a single channel. > Yes. You have a perfectly suitable method for it. It's > loongson_gnet_data(). Init the number of channels there based on the > value read from the GMAC_VERSION.SNPSVER field. Thus the > loongson_gnet_config_multi_msi() will get to be more coherent setting > up the MSI IRQs only. You are right! it works well. Thanks, Yanteng
On Fri, Mar 22, 2024 at 06:36:20PM +0800, Yanteng Si wrote: > > > > > > +{ > > > > > > + 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); > > > > > In what conditions is this possible? Will the > > > > > loongson_dwmac_config_legacy() method work in that case? Did you test > > > > > it out? > I need to wait for special hardware and PMON for this. Please give me some > time. > > > Then those platforms will _require_ to have the DT-node specified. This > > will define the DT-bindings which I doubt you imply here. Am I wrong? > > > > Once again have you tested the loongson_dwmac_config_legacy() method > > working in the case of the pci_alloc_irq_vectors() failure? > > Yes! I have tested it, it works in single channel mode. > > dmesg: > > [ 3.935203] mdio_bus stmmac-18:02: attached PHY driver [unbound] > (mii_bus:phy_addr=stmmac-18:02, irq=POLL) > [ 3.945625] dwmac-loongson-pci 0000:00:03.1: MSI enable failed, Fallback to > legacy interrupt > [ 3.954175] dwmac-loongson-pci 0000:00:03.1: User ID: 0xd1, Synopsys ID: 0x10 > [ 3.973676] dwmac-loongson-pci 0000:00:03.1: DMA HW capability register supported > [ 3.981135] dwmac-loongson-pci 0000:00:03.1: RX Checksum Offload Engine supported > > cat /proc/interrupt: > > 43: 0 0 PCH PIC 16 ahci[0000:00:08.0] > 44: 0 0 PCH PIC 12 enp0s3f0 > 45: 0 0 PCH PIC 14 enp0s3f1 > 46: 16233 0 PCH PIC 17 enp0s3f2 > 47: 12698 0 PCH PIC 48 xhci-hcd:usb1 > > > the irq number 46 is the falkback legacy irq. Ok. Thanks. You can do that in a bit more clever manner. Like this: +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; +} Thus in case if for some reason you were able to allocate less MSI IRQs than required you'll still be able to use them. The legacy IRQ will be also available in case if MSI failed to be allocated. -Serge(y) > > > > > > > > > > > + } > > > > > > + > > > > > > + plat->rx_queues_to_use = channel_num; > > > > > > + plat->tx_queues_to_use = channel_num; > > > > > This is supposed to be initialized in the setup() methods. Please move > > > > > it to the dedicated patch. > > > > No, referring to my previous reply, only the 0x10 gnet device has 8 channels, > > > > and the 0x37 device has a single channel. > > Yes. You have a perfectly suitable method for it. It's > > loongson_gnet_data(). Init the number of channels there based on the > > value read from the GMAC_VERSION.SNPSVER field. Thus the > > loongson_gnet_config_multi_msi() will get to be more coherent setting > > up the MSI IRQs only. > You are right! it works well. > > > Thanks, > > Yanteng > >
在 2024/3/23 02:47, Serge Semin 写道: > +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; > +} > > Thus in case if for some reason you were able to allocate less MSI > IRQs than required you'll still be able to use them. The legacy IRQ > will be also available in case if MSI failed to be allocated. Great, we will consider doing this in the future, but at this stage, we don't want to add too much complexity. Thanks, Yanteng
在 2024/3/19 23:53, Serge Semin 写道: > Then those platforms will_require_ to have the DT-node specified. This > will define the DT-bindings which I doubt you imply here. Am I wrong? yes，dt-node has specified irq information，and this driver also work with ACPI configuration. Thanks, Yanteng
On Wed, Apr 03, 2024 at 04:09:21PM +0800, Yanteng Si wrote: > > 在 2024/3/23 02:47, Serge Semin 写道: > > +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; > > +} > > > > Thus in case if for some reason you were able to allocate less MSI > > IRQs than required you'll still be able to use them. The legacy IRQ > > will be also available in case if MSI failed to be allocated. > > Great, we will consider doing this in the future, but at this stage, we > don't want to add too much complexity. This comment isn't about complexity. Moreover the code in my comment is simpler since the function is more coherent and doesn't have the redundant dependencies from the node-pointer and the loongson_dwmac_config_legacy() function. In addition it provides more flexible solution in case if there were less MSI vectors allocated then required. -Serge(y) > > > Thanks, > > Yanteng >
在 2024/4/3 20:03, Serge Semin 写道: > On Wed, Apr 03, 2024 at 04:09:21PM +0800, Yanteng Si wrote: >> 在 2024/3/23 02:47, Serge Semin 写道: >>> +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; >>> +} >>> >>> Thus in case if for some reason you were able to allocate less MSI >>> IRQs than required you'll still be able to use them. The legacy IRQ >>> will be also available in case if MSI failed to be allocated. >> Great, we will consider doing this in the future, but at this stage, we >> don't want to add too much complexity. > This comment isn't about complexity. Moreover the code in my comment > is simpler since the function is more coherent and doesn't have the > redundant dependencies from the node-pointer and the > loongson_dwmac_config_legacy() function. In addition it provides more > flexible solution in case if there were less MSI vectors allocated > then required. I just tried it, the network card doesn't work, the reason is not clear. Considering that the overall change is a little big, I want to send the current working state as the patch v9. However, I will continue to analyze the reasons for the failure and submit them as a separate patch in the future. Is that OK? Thanks, Yanteng
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c index 584f7322bd3e..60d0a122d7c9 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c @@ -98,10 +98,10 @@ static void dwlgmac_dma_init_channel(struct stmmac_priv *priv, if (dma_cfg->aal) value |= DMA_BUS_MODE_AAL; - writel(value, ioaddr + DMA_BUS_MODE); + writel(value, ioaddr + DMA_CHAN_BUS_MODE(chan)); /* Mask interrupts by writing to CSR7 */ - writel(DMA_INTR_DEFAULT_MASK_LOONGSON, ioaddr + DMA_INTR_ENA); + writel(DMA_INTR_DEFAULT_MASK_LOONGSON, ioaddr + DMA_CHAN_INTR_ENA(chan)); } static int dwlgmac_dma_interrupt(struct stmmac_priv *priv, void __iomem *ioaddr, @@ -238,6 +238,45 @@ static int loongson_dwmac_config_legacy(struct pci_dev *pdev, return 0; } +static int loongson_dwmac_config_multi_msi(struct pci_dev *pdev, + struct plat_stmmacenet_data *plat, + struct stmmac_resources *res, + struct device_node *np, + int channel_num) +{ + 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); + } + + plat->rx_queues_to_use = channel_num; + plat->tx_queues_to_use = channel_num; + + res->irq = pci_irq_vector(pdev, 0); + res->wol_irq = res->irq; + + /* 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; + dev_info(&pdev->dev, "%s: multi MSI enablement successful\n", __func__); + + return 0; +} + static void loongson_default_data(struct pci_dev *pdev, struct plat_stmmacenet_data *plat) { @@ -296,11 +335,8 @@ static int loongson_gmac_config(struct pci_dev *pdev, struct stmmac_resources *res, struct device_node *np) { - int ret; - - ret = loongson_dwmac_config_legacy(pdev, plat, res, np); - return ret; + return 0; } static struct stmmac_pci_info loongson_gmac_pci_info = { @@ -380,11 +416,7 @@ static int loongson_gnet_config(struct pci_dev *pdev, struct stmmac_resources *res, struct device_node *np) { - int ret; - - ret = loongson_dwmac_config_legacy(pdev, plat, res, np); - - return ret; + return 0; } static struct stmmac_pci_info loongson_gnet_pci_info = { @@ -483,6 +515,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, ld->dwlgmac_dma_ops.dma_interrupt = dwlgmac_dma_interrupt; plat->setup = loongson_setup; + ret = loongson_dwmac_config_multi_msi(pdev, plat, &res, np, 8); + } else { + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np); } plat->bsp_priv = ld;