Message-ID: <648058f6-7e0f-4e6e-9e27-cecf48ef1e2c@loongson.cn>
Date: Thu, 4 Jul 2024 16:56:41 +0800
From: Yanteng Si <siyanteng@...ngson.cn>
To: Serge Semin <fancer.lancer@...il.com>
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


在 2024/7/4 00:19, Serge Semin 写道:
> 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.
Oh, I see!
>
>>> 	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

Powered by blists - more mailing lists