在 2024/7/4 00:19, Serge Semin 写道:

On Wed, Jul 03, 2024 at 05:41:55PM +0800, Yanteng Si wrote:

quoted

quoted

quoted

quoted

quoted

-	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().

quoted

quoted

quoted

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.

quoted

quoted

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!

quoted

	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/ (local)

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.

Oh, I see!

quoted

quoted

	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.

OK. I got you!

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?

Based on my test results:

In this case, multi-channel controller don't work. If the MSI IRQs 
allocation

failed, NIC will work in single channel.


I will prepare v14 according to your comments,


Over the past year, with everyone's help, the drive has become better 
and better.

Thank you everyone. Thank you very much!


Thanks,

Yanteng