Re: [PATCH net-next v16 12/14] net: stmmac: dwmac-loongson: Add Loongson Multi-channels GMAC support

[YanTeng Si <si.yanteng@linux.dev>]

在 2024/8/7 3:17, Serge Semin 写道:
> On Tue, Aug 06, 2024 at 07:00:22PM +0800, Yanteng Si wrote:
>> The Loongson DWMAC driver currently supports the Loongson GMAC
>> devices (based on the DW GMAC v3.50a/v3.73a IP-core) installed to the
>> LS2K1000 SoC and LS7A1000 chipset. But recently a new generation
>> LS2K2000 SoC was released with the new version of the Loongson GMAC
>> synthesized in. The new controller is based on the DW GMAC v3.73a
>> IP-core with the AV-feature enabled, which implies the multi
>> DMA-channels support. The multi DMA-channels feature has the next
>> vendor-specific peculiarities:
>>
>> 1. Split up Tx and Rx DMA IRQ status/mask bits:
>>         Name              Tx          Rx
>>    DMA_INTR_ENA_NIE = 0x00040000 | 0x00020000;
>>    DMA_INTR_ENA_AIE = 0x00010000 | 0x00008000;
>>    DMA_STATUS_NIS   = 0x00040000 | 0x00020000;
>>    DMA_STATUS_AIS   = 0x00010000 | 0x00008000;
>>    DMA_STATUS_FBI   = 0x00002000 | 0x00001000;
>> 2. Custom Synopsys ID hardwired into the GMAC_VERSION.SNPSVER register
>> field. It's 0x10 while it should have been 0x37 in accordance with
>> the actual DW GMAC IP-core version.
>> 3. There are eight DMA-channels available meanwhile the Synopsys DW
>> GMAC IP-core supports up to three DMA-channels.
>> 4. It's possible to have each DMA-channel IRQ independently delivered.
>> The MSI IRQs must be utilized for that.
>>
>> Thus in order to have the multi-channels Loongson GMAC controllers
>> supported let's modify the Loongson DWMAC driver in accordance with
>> all the peculiarities described above:
>>
>> 1. Create the multi-channels Loongson GMAC-specific
>>     stmmac_dma_ops::dma_interrupt()
>>     stmmac_dma_ops::init_chan()
>>     callbacks due to the non-standard DMA IRQ CSR flags layout.
>> 2. Create the Loongson DWMAC-specific platform setup() method
>> which gets to initialize the DMA-ops with the dwmac1000_dma_ops
>> instance and overrides the callbacks described in 1. The method also
>> overrides the custom Synopsys ID with the real one in order to have
>> the rest of the HW-specific callbacks correctly detected by the driver
>> core.
>> 3. Make sure the platform setup() method enables the flow control and
>> duplex modes supported by the controller.
>>
>> Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn>
>> Signed-off-by: Yinggang Gu <guyinggang@loongson.cn>
>> Acked-by: Huacai Chen <chenhuacai@loongson.cn>
>> Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
>> ---
>>
>> ...
>>
>> +
>> +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_warn(&pdev->dev, "Failed to allocate MSI 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;
>> +}
>> +
>> +static void loongson_dwmac_msi_clear(struct pci_dev *pdev)
>> +{
>> +	pci_free_irq_vectors(pdev);
>> +}
>> +
>>   static int loongson_dwmac_dt_config(struct pci_dev *pdev,
>>   				    struct plat_stmmacenet_data *plat,
>>   				    struct stmmac_resources *res)
>> @@ -146,6 +450,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>>   	struct plat_stmmacenet_data *plat;
>>   	struct stmmac_pci_info *info;
>>   	struct stmmac_resources res;
>> +	struct loongson_data *ld;
>>   	int ret, i;
>>   
>>   	plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
>> @@ -162,6 +467,10 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>>   	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) {
>> @@ -184,6 +493,10 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>>   	memset(&res, 0, sizeof(res));
>>   	res.addr = pcim_iomap_table(pdev)[0];
>>   
>> +	plat->bsp_priv = ld;
>> +	plat->setup = loongson_dwmac_setup;
>> +	ld->loongson_id = readl(res.addr + GMAC_VERSION) & 0xff;
>> +
>>   	info = (struct stmmac_pci_info *)id->driver_data;
>>   	ret = info->setup(pdev, plat);
>>   	if (ret)
>> @@ -196,6 +509,10 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>>   	if (ret)
>>   		goto err_disable_device;
>>   
>> +	/* Use the common MAC IRQ if per-channel MSIs allocation failed */
>> +	if (ld->loongson_id == DWMAC_CORE_LS_MULTICHAN)
>> +		loongson_dwmac_msi_config(pdev, plat, &res);
>> +
>>   	ret = stmmac_dvr_probe(&pdev->dev, plat, &res);
>>   	if (ret)
>>   		goto err_plat_clear;
>> @@ -205,6 +522,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>>   err_plat_clear:
>>   	if (dev_of_node(&pdev->dev))
>>   		loongson_dwmac_dt_clear(pdev, plat);
>> +	loongson_dwmac_msi_clear(pdev);
> Em, why have you dropped the if-statement here? That has caused the
> Simon note to be posted. Please get it back:
> +	if (ld->loongson_id == DWMAC_CORE_LS_MULTICHAN)
> +		loongson_dwmac_msi_clear(pdev);
OK.
>
>>   err_disable_device:
>>   	pci_disable_device(pdev);
>>   	return ret;
>> @@ -214,12 +532,15 @@ static void loongson_dwmac_remove(struct pci_dev *pdev)
>>   {
>>   	struct net_device *ndev = dev_get_drvdata(&pdev->dev);
>>   	struct stmmac_priv *priv = netdev_priv(ndev);
>> +	struct loongson_data *ld;
>>   	int i;
>>   
>> +	ld = priv->plat->bsp_priv;
>>   	stmmac_dvr_remove(&pdev->dev);
>>   
>>   	if (dev_of_node(&pdev->dev))
>>   		loongson_dwmac_dt_clear(pdev, priv->plat);
>> +	loongson_dwmac_msi_clear(pdev);
> Ditto. Please get back the conditional MSI-clear method execution:
> +
> +	if (ld->loongson_id == DWMAC_CORE_LS_MULTICHAN)
> +		loongson_dwmac_msi_clear(pdev);
>
> * Note the empty line above the if-statement.

OK, Thanks!


Thanks,

Yanteng

>
> -Serge(y)
>
>>   
>>   	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>>   		if (pci_resource_len(pdev, i) == 0)
>> -- 
>> 2.31.4
>>