Re: [PATCH net-next v12 13/15] net: stmmac: dwmac-loongson: Add Loongson GNET support

[Huacai Chen <chenhuacai@kernel.org>]

Hi, Serge,

On Tue, May 14, 2024 at 7:33 PM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> On Tue, May 14, 2024 at 12:58:33PM +0800, Huacai Chen wrote:
> > On Tue, May 14, 2024 at 12:11 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> > >
> > > On Mon, May 13, 2024 at 09:26:11PM +0800, Huacai Chen wrote:
> > > > Hi, Serge,
> > > >
> > > > On Mon, May 13, 2024 at 6:57 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > > >
> > > > > On Thu, May 09, 2024 at 04:57:44PM +0800, Yanteng Si wrote:
> > > > > > Hi Serge
> > > > > >
> ...
> > > > >
> > > > > > No, only devices that support multiple channels can deliver both PCI MSI
> > > > > > IRQs
> > > > > >
> > > > > > and direct GIC IRQs, other devices can only deliver GIC IRQs.
> > > > > >
> > > > > > Furthermore, multiple channel features are bundled with MSI. If we want to
> > > > > >
> > > > > > enable multiple channels, we must enable MSI.
> > > > >
> > > > > Sadly to say but this information changes a lot. Based on that the
> > > > > only platform with optional DT-node is the LS2K2000 GNET device. The
> > > > > rest of the devices (GMACs and LS7A2000 GNET) must be equipped with a
> > > > > node-pointer otherwise they won't work. Due to that the logic of the
> > > > > patches
> > > > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support
> > > > > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy
> > > > > is incorrect.
> > > > >
> > > > > 1. [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support
> > > > > So this patch doesn't add a pure PCI-based probe procedure after all
> > > > > because the Loongson GMACs are required to have a DT-node. AFAICS
> > > > > pdev->irq is actually the IRQ retrieved from the DT-node. So the "if
> > > > > (np) {} else {}" clause doesn't really make sense.
> > > > >
> > > > > 2. [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy
> > > > > First of all the function name is incorrect. The IRQ signal isn't legacy
> > > > > (INTx-based), but is retrieved from the DT-node. Secondly the
> > > > > "if (np) {} else {}" statement is very much redundant because if no
> > > > > DT-node found the pdev->irq won't be initialized at all, and the
> > > > > driver won't work with no error printed.
> > > > >
> > > > > All of that also affects the patch/commit logs. Glad we figured that
> > > > > out at this stage. Seeing there have been tons of another comments
> > > > > let's postpone the discussion around this problem for v13 then. I'll
> > > > > keep in mind the info you shared in this thread and think of the way
> > > > > to fix the patches after v13 is submitted for review.
> > > > Let me clarify the interrupt information, hope that can help you to
> > > > understand better:
> > >
> > > > 1, Loongson machines may use UEFI (implies ACPI) or PMON/UBOOT
> > > > (implies FDT) as the BIOS.
> > >
> > > Ok. Aside with the OF-based platform there is an ACPI case.
> > >
> > > > 2, The BIOS type has no relationship with device types, which means:
> > > > machines with GMAC can be either ACPI-based or FDT-based, machines
> > > > with GNET can also be either ACPI-based or FDT-based.
> > >
> > > Ok. It's either-or. Got it.
> > >
> > > > 3, The existing Loongson driver can only support FDT, which means the
> > > > device should be PCI-probed and DT-configured. Though the existing
> > > > driver only supports GMAC, it doesn't mean that GMAC is bound to FDT.
> > > > GMAC can also work with ACPI, in that case we say it is "full PCI",
> > > > which means we don't need "np".
> > >
> > > "full PCI" statement can't be utilized for the case of the ACPI-based
> > > IRQ assignment. "full PCI" is the way the GNET probe procedure works -
> > > everything required for the device handling is detected in runtime
> > > with no ACPI/DT stuff.
> > >
> > > So the patch 10 with the "full PCI"-related subject doesn't actually
> > > adds the PCIe-only-based device probe support, but actually converts
> > > the driver to supporting the ACPI-case.)
>
> > Yes, the commit message can be improved.
>
> Can be? It must be changed, because at the very least it's misleading,
> but frankly speaking it now sounds just wrong.
Sit back and relax. :)
I agree with your opinion, but we don't need to so abolute, yes?

>
> >
> > >
> > > > 4, At present, multi-channel devices support MSI, currently only GNET
> > > > support MSI, but in future there may also GMAC support MSI.
> > >
> > > It's better to avoid adding a support for hypothetical devices and
> > > prohibit all the currently unreal cases. It will simplify the code,
> > > ease it' maintenance, reduce the bugs probability.
> > >
> > > > 5, So, in Yanteng's patches, a device firstly request MSI, and since
> > > > MSI is dynamically allocated, it doesn't care about the BIOS type
> > > > (ACPI or FDT). However, if MSI fails (either because MSI is exhausted
> > > > or the device doesn't support it), it fallback to "legacy" interrupt,
> > > > which means irq lines mapped to INT-A/B/C/D of PCI.
> > >
> > > Unless we are talking about the actual PCI devices (not PCI express)
> > > or the cases where the INT-x is emulated by means the specific PCIe
> > > TLPs, I wouldn't mentioned the INTx or "legacy" names in the current
> > > context. It's just a platform ACPI/DT IRQs.
>
> > Yes, it is probably a platform ACPI/DT IRQ, but I think the "legacy"
> > name is still reasonable in this case.
>
> Probably? These _are_ pure platform IRQs.
>
> > Otherwise, what does "legacy"
> > stand for in "PCI_IRQ_LEGACY/PCI_IRQ_MSI/PCI_IRQ_MSIX"?
>
> It means that the platform IRQs has just been implemented via the
> already available old-school API, which has been in the kernel since
> the plain PCI devices. The platform IRQs and the traditional PCI INTx
> are normally mutually exclusive, so I guess that's why they have been
> implemented in framework of the same interface. Another reason could
> be to have less troubles with adopting the PCI drivers for both type
> of the IRQs delivery.
>
> Moreover just recently the so called _legacy_ flag name has been
> deprecated in favor of the more generic INTx one:
> https://lore.kernel.org/linux-pci/20231122060406.14695-1-dlemoal@kernel.org/
This info is important, but your last suggestion for Yanteng still use
PCI_IRQ_LEGACY. :)

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

Huacai

>
> >
> > >
> > > > 6. In the legacy case, the irq is get from DT-node (FDT case), or
> > > > already in pdev->irq (ACPI case).
> > >
> > > It will be in the pdev->irq in any case whether it's DT or ACPI. See:
> > >
> > > ACPI:
> > > pci_device_probe():
> > > +-> arch/loongarch/pci/pci.c:pcibios_alloc_irq()
> > >
> > > DT:
> > > pci_device_probe():
> > > +-> pci_assign_irq();
> > >     +-> pci_host_bridge::map_irq()
> > >         +-> of_irq_parse_and_map_pci()
> > >         or in case of Loongson PCIe host controller:
> > >         +-> drivers/pci/controller/pci-loongson.c::loongson_map_irq()
> > >
> > > Moreover unless the MSI IRQs are enabled, the platform IRQ (and the
> > > legacy IRQ) can be retrieved by means of the pci_irq_vector() method.
> > > The only reason of having the direct OF-based IRQs getting in the
> > > Loongson DWMAC driver I see is that the LPI IRQ will be missing in
> > > case of the pci_irq_vector() method utilization. In the rest of the
> > > cases the pci_irq_vector() function could be freely used.
> > Yes, in the DT case, they may be macirq, eth_wake_irq and eth_lpi,
> > rather than a single irq, so we need an if-else here.
> >
> > >
> > > >  So Yanteng use a "if (np) { } else {
> > > > }", which is reasonable from my point of view.
> > > >
> > >
> > > At least one problem is there. What if pdev->irq isn't initialized
> > > (initialized with zero)?..
>
> > As you said above, both ACPI and DT initialized pdev->irq, unless
> > there is a bug in BIOS.
>
> I meant that based on the platform firmware nature the pdev->irq field
> shall be initialized with an IRQ number in accordance with the DT or
> ACPI logic. I never said it was impossible to have the field
> uninitialized (that is being left zero). It's absolutely possible.
> There are much more reasons to have that than just a firmware bug. On
> the top of my mind: MSI being enabled, kernel misconfiguration, kernel
> bug, DT/ACPI lacking the IRQ property, ...
>
> -Serge(y)
>
> >
> >
> > Huacai
> >
> > >
> > > > So Yanteng's interrupt code is good for me, but I also agree to
> > > > improve that after v13, if needed.
> > >
> > > Ok. I've got much better picture about what is going on under the
> > > hood. Thanks. In anyway I'll get back to this topic in details in v13.
> > >
> > > -Serge(y)
> > >
> > > >
> > > > Huacai
> > > >
> > > > >
> > > > > Thanks
> > > > > -Serge(y)
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > Yanteng
> > > > > >