Message-ID: <xa2ewgfe3qjljsraet5d77qk3dygcvexnqk5atm5fm5oro3ogp@xctegdmx2srt>
Date: Wed, 15 May 2024 11:40:16 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Huacai Chen <chenhuacai@...nel.org>
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 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.

-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