Message-ID: <roxfse6rf7ngnopn42f6la2ewzsaonjbrfokqjlumrpkobfvgh@7v7vblqi3mak>
Date: Tue, 19 Mar 2024 18:53:25 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Yanteng Si <siyanteng@...ngson.cn>
Cc: andrew@...n.ch, hkallweit1@...il.com, peppe.cavallaro@...com, 
	alexandre.torgue@...s.st.com, joabreu@...opsys.com, Jose.Abreu@...opsys.com, 
	chenhuacai@...ngson.cn, linux@...linux.org.uk, guyinggang@...ngson.cn, 
	netdev@...r.kernel.org, chris.chenfeiyang@...il.com
Subject: Re: [PATCH net-next v8 07/11] net: stmmac: dwmac-loongson: Add
 multi-channel supports for loongson

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@...ngson.cn>
> > >> Signed-off-by: Feiyang Chen<chenfeiyang@...ngson.cn>
> > >> Signed-off-by: Yinggang Gu<guyinggang@...ngson.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
> > >>
> 

Powered by blists - more mailing lists