Message-ID: <CAAhV-H5JT+QfZgHX7K3HYLFSxuZeer4PdUPjehtyXKcfi=L2oQ@mail.gmail.com>
Date: Wed, 15 May 2024 21:55:09 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: Serge Semin <fancer.lancer@...il.com>
Cc: Yanteng Si <siyanteng@...ngson.cn>, 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

On Wed, May 15, 2024 at 4:40 PM Serge Semin <fancer.lancer@...il.com> wrote:
>
> On Tue, May 14, 2024 at 08:53:46PM +0800, Huacai Chen wrote:
> > Hi, Serge,
> >
> > On Tue, May 14, 2024 at 7:33 PM Serge Semin <fancer.lancer@...il.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@...il.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@...il.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. :)
>
> The smiley enclosing your epigram doesn't make it appropriate. Please
> refrain from familiarity in our future discussions.
>
> > 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. :)
>
> Yes, my mistake. It should be replaced with PCI_IRQ_INTX.
>
> >
> > >
> > > 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.

BTW, we cannot remove "res->wol_irq = res->irq", because Loongson
GMAC/GNET indeed supports WoL.

Huacai

>
> -Serge(y)
>
> >
> > 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
> > > > > > > >

Powered by blists - more mailing lists