Date: Fri, 17 May 2024 18:37:50 +0800
From: Yanteng Si <siyanteng@...ngson.cn>
To: Serge Semin <fancer.lancer@...il.com>, Huacai Chen <chenhuacai@...nel.org>
Cc: andrew@...n.ch, hkallweit1@...il.com, peppe.cavallaro@...com,
 alexandre.torgue@...s.st.com, joabreu@...opsys.com, Jose.Abreu@...opsys.com,
 linux@...linux.org.uk, guyinggang@...ngson.cn, netdev@...r.kernel.org,
 chris.chenfeiyang@...il.com, siyanteng01@...il.com
Subject: Re: [PATCH net-next v12 13/15] net: stmmac: dwmac-loongson: Add
 Loongson GNET support

Hi Serge,

在 2024/5/17 17:07, Serge Semin 写道:
> On Fri, May 17, 2024 at 04:42:51PM +0800, Yanteng Si wrote:
>> Hi Huacai, Serge,
>>
>> 在 2024/5/15 21:55, Huacai Chen 写道:
>>>>>> Once again about the naming. From the retrospective point of view the
>>>>>> so called legacy PCI IRQs (in fact PCI INTx) and the platform IRQs
>>>>>> look similar because these are just the level-type signals connected
>>>>>> to the system IRQ controller. But when it comes to the PCI_Express_,
>>>>>> the implementation is completely different. The PCIe INTx is just the
>>>>>> PCIe TLPs of special type, like MSI. Upon receiving these special
>>>>>> messages the PCIe host controller delivers the IRQ up to the
>>>>>> respective system IRQ controller. So in order to avoid the confusion
>>>>>> between the actual legacy PCI INTx, PCI Express INTx and the just
>>>>>> platform IRQs, it's better to emphasize the actual way of the IRQs
>>>>>> delivery. In this case it's the later method.
>>>>> You are absolutely right, and I think I found a method to use your
>>>>> framework to solve our problems:
>>>>>
>>>>>      static int loongson_dwmac_config_irqs(struct pci_dev *pdev,
>>>>>                                             struct plat_stmmacenet_data *plat,
>>>>>                                             struct stmmac_resources *res)
>>>>>      {
>>>>>          int i, ret, vecs;
>>>>>
>>>>>          /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx |
>>>>>           * --------- ----- -------- --------  ...  -------- --------
>>>>>           * IRQ NUM  |  0  |   1    |   2    | ... |   15   |   16   |
>>>>>           */
>>>>>          vecs = plat->rx_queues_to_use + plat->tx_queues_to_use + 1;
>>>>>          ret = pci_alloc_irq_vectors(pdev, 1, vecs, PCI_IRQ_MSI | PCI_IRQ_INTX);
>>>>>          if (ret < 0) {
>>>>>                  dev_err(&pdev->dev, "Failed to allocate PCI IRQs\n");
>>>>>                  return ret;
>>>>>          }
>>>>>         if (ret >= vecs) {
>>>>>                  for (i = 0; i < plat->rx_queues_to_use; i++) {
>>>>>                          res->rx_irq[CHANNELS_NUM - 1 - i] =
>>>>>                                  pci_irq_vector(pdev, 1 + i * 2);
>>>>>                  }
>>>>>                  for (i = 0; i < plat->tx_queues_to_use; i++) {
>>>>>                          res->tx_irq[CHANNELS_NUM - 1 - i] =
>>>>>                                  pci_irq_vector(pdev, 2 + i * 2);
>>>>>                  }
>>>>>
>>>>>                  plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
>>>>>          }
>>>>>
>>>>>          res->irq = pci_irq_vector(pdev, 0);
>>>>>
>>>>>        if (np) {
>>>>>            res->irq = of_irq_get_byname(np, "macirq");
>>>>>            if (res->irq < 0) {
>>>>>               dev_err(&pdev->dev, "IRQ macirq not found\n");
>>>>>               return -ENODEV;
>>>>>            }
>>>>>
>>>>>            res->wol_irq = of_irq_get_byname(np, "eth_wake_irq");
>>>>>            if (res->wol_irq < 0) {
>>>>>               dev_info(&pdev->dev,
>>>>>                    "IRQ eth_wake_irq not found, using macirq\n");
>>>>>               res->wol_irq = res->irq;
>>>>>            }
>>>>>
>>>>>            res->lpi_irq = of_irq_get_byname(np, "eth_lpi");
>>>>>            if (res->lpi_irq < 0) {
>>>>>               dev_err(&pdev->dev, "IRQ eth_lpi not found\n");
>>>>>               return -ENODEV;
>>>>>            }
>>>>>        }
>>>>>          return 0;
>>>>>      }
>>>>>
>>>>> If your agree, Yanteng can use this method in V13, then avoid furthur changes.
>>>> Since yesterday I have been too relaxed sitting back to explain in
>>>> detail the problems with the code above. Shortly speaking, no to the
>>>> method designed as above.
>>> This function is copy-paste from your version which you suggest to
>>> Yanteng, and plus the fallback parts for DT. If you don't want to
>>> discuss it any more, we can discuss after V13.
> My conclusion is the same. no to _your_ (Huacai) version of the code.
> I suggest to Huacai dig dipper in the function semantic and find out
> the problems it has. Meanwhile I'll keep relaxing...
>
>>> BTW, we cannot remove "res->wol_irq = res->irq", because Loongson
>>> GMAC/GNET indeed supports WoL.
>> Okay, I will not drop it in v13.
> Apparently Huacai isn't well familiar with what he is reviewing. Once
> again the initialization is useless. Drop it.

Hmm, to be honest, I'm still a little confused about this.

When we first designed the driver, we looked at intel,See:

$: vim drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c +953

static int stmmac_config_single_msi(struct pci_dev *pdev,
                     struct plat_stmmacenet_data *plat,
                     struct stmmac_resources *res)
{
     int ret;

     ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
     if (ret < 0) {
         dev_info(&pdev->dev, "%s: Single IRQ enablement failed\n",
              __func__);
         return ret;
     }

     res->irq = pci_irq_vector(pdev, 0);
     res->wol_irq = res->irq;

Why can't we do this?

Intel Patch thread link 
<https://lore.kernel.org/netdev/20210316121823.18659-5-weifeng.voon@intel.com/>


>
>> All right. I have been preparing v13 and will send it as soon as possible.
>>
>> Let's continue the discussion in v13. Of course, I will copy the part that
>> has
>>
>> not received a clear reply to v13.
>>
> Note the merge window has been opened and the 'net-next' tree is now
> closed. So either you submit your series as RFC or wait for the window
> being closed.
>
Ok, if I'm fast enough, I'll send an RFC to talk about msi and legacy.


Thanks,

Yanteng

Powered by blists - more mailing lists