Message-ID: <vss2wuq5ivfnpdfgkjsp23yaed5ve2c73loeybddhbwdx2ynt2@yfk2nmj5lmod>
Date: Wed, 3 Jul 2024 19:19:10 +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@...nel.org, linux@...linux.org.uk, guyinggang@...ngson.cn, 
	netdev@...r.kernel.org, chris.chenfeiyang@...il.com, si.yanteng@...ux.dev
Subject: Re: [PATCH net-next v13 06/15] net: stmmac: dwmac-loongson: Detach
 GMAC-specific platform data init

On Wed, Jul 03, 2024 at 05:41:55PM +0800, Yanteng Si wrote:
> > > > > -	plat->mac_interface = PHY_INTERFACE_MODE_GMII;
> > > > >    	pci_set_master(pdev);
> > > > > -	loongson_default_data(plat);
> > > > > +	loongson_gmac_data(plat);
> > > > >    	pci_enable_msi(pdev);
> > > > >    	memset(&res, 0, sizeof(res));
> > > > >    	res.addr = pcim_iomap_table(pdev)[0];
> > > > > @@ -140,6 +142,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
> > > > >    		goto err_disable_msi;
> > > > >    	}
> > > > > +	plat->tx_queues_to_use = 1;
> > > > > +	plat->rx_queues_to_use = 1;
> > > > > +
> > > > Please move this to the loongson_gmac_data(). Thus all the
> > > > platform-data initializations would be collected in two coherent
> > > > methods: loongson_default_data() and loongson_gmac_data(). It will be
> > > > positive from the readability and maintainability points of view.
> > > OK, I will move this to the  loongson_default_data(),
> > > 
> > > Because loongson_gmac/gnet_data() call it.
> > It shall also work. But the fields will be overwritten in the
> > loongson_gmac_data()/loongson_gnet_data() methods for the
> > multi-channels case. I don't have a strong opinion against that. But
> > some maintainers may have.
> I see. I will move this to the loongson_gmac_data()/loongson_gnet_data().
> > 
> > > 
> > > > In the patch adding the Loongson multi-channel GMAC support make sure
> > > > the loongson_data::loongson_id field is initialized before the
> > > > stmmac_pci_info::setup() method is called.
> > > I've tried. It's almost impossible.
> > Emm, why is it impossible? I don't see any significant problem with
> > implementing that.
> Sorry, I've to take back my words.
> > 
> > > 
> > > The only way to do this is to initialize loongson_id again in
> > > loongson_default_data().
> > > 
> > > But that will add a lot of code.
> > Not sure, why? What about the next probe() code snippet:
> Full marks!
> > 
> > 	plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
> > 	if (!plat)
> > 		return -ENOMEM;
> > 
> > 	plat->mdio_bus_data = devm_kzalloc(&pdev->dev,
> > 					   sizeof(*plat->mdio_bus_data),
> > 					   GFP_KERNEL);
> >          if (!plat->mdio_bus_data)
> >                  return -ENOMEM;
> > 
> > 	plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL);
> > 	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);
> >   	if (ret)
> > 		return ret;
> > 
> > 	// AFAIR the bus-mastering isn't required for the normal PCIe
> > 	// IOs. So pci_set_master() can be called some place
> > 	// afterwards.
> > 	pci_set_master(pdev);
> > 
> > 	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> > 		if (pci_resource_len(pdev, i) == 0)
> > 			continue;
> > 		ret = pcim_iomap_regions(pdev, BIT(0), pci_name(pdev));
> > 		if (ret)
> > 			goto err_disable_device;
> > 		break;
> > 	}
> > 
> > 	memset(&res, 0, sizeof(res));
> > 	res.addr = pcim_iomap_table(pdev)[0];
> > 
> > 	plat->bsp_priv = ld;
> > 	plat->setup = loongson_dwmac_setup;
> > 	ld->dev = &pdev->dev;
> > 	ld->loongson_id = readl(res.addr + GMAC_VERSION) & 0xff;
> > 
> > 	info = (struct stmmac_pci_info *)id->driver_data;
> > 	ret = info->setup(plat);
> > 	if (ret)
> > 		goto err_disable_device;
> > 
> > 	if (dev_of_node(&pdev->dev))
> >   		ret = loongson_dwmac_dt_config(pdev, plat, &res);
> > 	else
> 

> > 		ret = loongson_dwmac_acpi_config(pdev, plat, &res);
> 
> I don't know how to write this function, it seems that there
> 
> is no obvious acpi code in the probe method.

I've provided the method code here:
https://lore.kernel.org/netdev/glm3jfqf36t5vnkmk4gsdqfx53ga7ohs3pxnsizqlogkbim7gg@a3dxav5siczn/

It just gets the IRQ from the pci_device::irq field:

static int loongson_dwmac_acpi_config(struct pci_dev *pdev,
				      struct plat_stmmacenet_data *plat,
				      struct stmmac_resources *res)
{
	if (!pdev->irq)
		return -EINVAL;

	res->irq = pdev->irq;

	return 0;
}

It implies that if there is no DT found on the platform or no DT-node
assigned to the device, the IRQ line was supposed to be detected via
the ACPI interface by the PCIe subsystem core. Just recently you said
that the Loongson platforms are either UEFI or U-boot based. So at
least the loongson_dwmac_acpi_config() method would imply that the IRQ
number was supposed to be retrieved by means of the ACPI interface.

> 
> > 
> > 	if (ret)
> > 		goto err_disable_device;
> > 
> > 	if (ld->loongson_id == DWMAC_CORE_LS2K2000) {
> > 		ret = loongson_dwmac_msi_config(pdev, plat, &res);
> > 		if (ret)
> > 			goto err_disable_device;
> > 	}
> 

> It seems that we don't need if else.
> 
> If failed to allocate msi,  it will fallback to intx.
> 
> so loongson_dwmac_msi_config also needs a new name. How about:
> 
> ...
> 
>     ret = loongson_dwmac_irq_config(pdev, plat, &res);
>     if (ret)
>         goto err_disable_device;

Well, I've been thinking about that for quite some time. The problem
with your approach is that you _always_ override the res->irq field
with data retrieved from pci_irq_vector(pdev, 0). What's the point in
the res->irq initialization in the loongson_dwmac_dt_config() method
then?

Originally I suggested to use the PCI_IRQ_INTX flag in the
loongson_dwmac_msi_config() method because implied that eventually we
could come up to some generic IRQs initialization method with no
IRQ-related code performed in the rest of the driver. But after some
brainstorming I gave up that topic for now. Sending comments connected
with that would mean to cause a one more discussion. Seeing we already
at v13 it would have extended the review process for even longer than
it has already got to.

So since the MSI IRQs are required for the multi-channels device it
would be better to allocate MSIs for that case only. Thus I'd preserve
the conditional loongson_dwmac_msi_config() execution and would drop
the PCI_IRQ_INTX flag seeing we aren't going to implement the generic
IRQ setup method anymore. Like this:

+static int loongson_dwmac_msi_config(struct pci_dev *pdev,
+				     struct plat_stmmacenet_data *plat,
+				     struct stmmac_resources *res)
+{
+	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_err(&pdev->dev, "Failed to allocate per-channels IRQs\n");
+		return ret;
+	}
+
+	res->irq = pci_irq_vector(pdev, 0);
+
+	for (i = 0; i < plat->rx_queues_to_use; i++) {
+		res->rx_irq[CHANNEL_NUM - 1 - i] =
+			pci_irq_vector(pdev, 1 + i * 2);
+	}
+	for (i = 0; i < plat->tx_queues_to_use; i++) {
+		res->tx_irq[CHANNEL_NUM - 1 - i] =
+			pci_irq_vector(pdev, 2 + i * 2);
+	}
+
+	plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
+
+	return 0;
+}

* Please note the pci_alloc_irq_vectors(..., vecs, vecs, PCI_IRQ_MSI) arguments.

Seeing the discussion has started anyway, could you please find out
whether the multi-channel controller will still work if the MSI IRQs
allocation failed? Will the multi-channel-ness still work in that
case?

If the IRQ events from _all_ DMA-channels would still be delivered via
the main MAC IRQ (AFAICS from the DW GMAC v3.73a HW databook it works
like that by default), then we could ignore the errors returned from
the loongson_dwmac_msi_config() method, and simplify the probe()
method like this:

	/* Fallback to the common MAC IRQ if MSIs allocation failed */
	if (ld->loongson_id == DWMAC_CORE_LOONGSON_MULTI_CH)
		loongson_dwmac_msi_config(pdev, plat, &res);

-Serge(y)

> 
>     ret = stmmac_dvr_probe(&pdev->dev, plat, &res);
> 
> ...
> 
> 
> 
> > 
> > 	ret = stmmac_dvr_probe(&pdev->dev, plat, &res);
> >   	if (ret)
> > 		goto err_clear_msi;
> > 
> > 	return 0;
> > 
> > 	...
> > 
> > The code allocates all the data, then enables the PCIe device
> > and maps the PCIe device resources. After that it calls all the setup
> > and config methods. Do I miss something?
> 
> You are right!
> 
> 
> 
> Thanks,
> 
> Yanteng
> 
> > 
> > -Serge(y)
> > 
> > > > > 2.31.4
> > > > > 
> 

Powered by blists - more mailing lists