Message-ID: <nt5bjlmul5jchxvx6zzgvbmdsegpwwz7quzt57vfejnxng7smz@abqdfipuclzh>
Date: Mon, 13 May 2024 19:11: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 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
> > >
> > > 在 2024/5/8 23:10, Serge Semin 写道:
> > > > On Wed, May 08, 2024 at 10:58:16PM +0800, Huacai Chen wrote:
> > > > > Hi, Serge,
> > > > >
> > > > > On Wed, May 8, 2024 at 10:38 PM Serge Semin<fancer.lancer@...il.com>  wrote:
> > > > > > On Tue, May 07, 2024 at 09:35:24PM +0800, Yanteng Si wrote:
> > > > > > > Hi Serge,
> > > > > > >
> > > > > > > 在 2024/5/6 18:39, Serge Semin 写道:
> > > > > > > > On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote:
> > > > > > > > > ...
> > > > > > > > > +static int loongson_dwmac_config_msi(struct pci_dev *pdev,
> > > > > > > > > +                              struct plat_stmmacenet_data *plat,
> > > > > > > > > +                              struct stmmac_resources *res,
> > > > > > > > > +                              struct device_node *np)
> > > > > > > > > +{
> > > > > > > > > + 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_info(&pdev->dev,
> > > > > > > > > +                  "MSI enable failed, Fallback to legacy interrupt\n");
> > > > > > > > > +         return loongson_dwmac_config_legacy(pdev, plat, res, np);
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + res->irq = pci_irq_vector(pdev, 0);
> > > > > > > > > + res->wol_irq = 0;
> > > > > > > > > +
> > > > > > > > > + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx |
> > > > > > > > > +  * --------- ----- -------- --------  ...  -------- --------
> > > > > > > > > +  * IRQ NUM  |  0  |   1    |   2    | ... |   15   |   16   |
> > > > > > > > > +  */
> > > > > > > > > + for (i = 0; i < CHANNEL_NUM; i++) {
> > > > > > > > > +         res->rx_irq[CHANNEL_NUM - 1 - i] =
> > > > > > > > > +                 pci_irq_vector(pdev, 1 + i * 2);
> > > > > > > > > +         res->tx_irq[CHANNEL_NUM - 1 - i] =
> > > > > > > > > +                 pci_irq_vector(pdev, 2 + i * 2);
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
> > > > > > > > > +
> > > > > > > > > + return 0;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > ...
> > > > > > > > >    static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > > > > > > >    {
> > > > > > > > >            struct plat_stmmacenet_data *plat;
> > > > > > > > >            int ret, i, bus_id, phy_mode;
> > > > > > > > >            struct stmmac_pci_info *info;
> > > > > > > > >            struct stmmac_resources res;
> > > > > > > > > + struct loongson_data *ld;
> > > > > > > > >            struct device_node *np;
> > > > > > > > >            np = dev_of_node(&pdev->dev);
> > > > > > > > > @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
> > > > > > > > >                    return -ENOMEM;
> > > > > > > > >            plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL);
> > > > > > > > > - if (!plat->dma_cfg) {
> > > > > > > > > -         ret = -ENOMEM;
> > > > > > > > > -         goto err_put_node;
> > > > > > > > > - }
> > > > > > > > > + 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);
> > > > > > > > > @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
> > > > > > > > >                    plat->phy_interface = phy_mode;
> > > > > > > > >            }
> > > > > > > > > - pci_enable_msi(pdev);
> > > > > > > > > + plat->bsp_priv = ld;
> > > > > > > > > + plat->setup = loongson_dwmac_setup;
> > > > > > > > > + ld->dev = &pdev->dev;
> > > > > > > > > +
> > > > > > > > >            memset(&res, 0, sizeof(res));
> > > > > > > > >            res.addr = pcim_iomap_table(pdev)[0];
> > > > > > > > > + ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff;
> > > > > > > > > +
> > > > > > > > > + switch (ld->gmac_verion) {
> > > > > > > > > + case LOONGSON_DWMAC_CORE_1_00:
> > > > > > > > > +         plat->rx_queues_to_use = CHANNEL_NUM;
> > > > > > > > > +         plat->tx_queues_to_use = CHANNEL_NUM;
> > > > > > > > > +
> > > > > > > > > +         /* Only channel 0 supports checksum,
> > > > > > > > > +          * so turn off checksum to enable multiple channels.
> > > > > > > > > +          */
> > > > > > > > > +         for (i = 1; i < CHANNEL_NUM; i++)
> > > > > > > > > +                 plat->tx_queues_cfg[i].coe_unsupported = 1;
> > > > > > > > > - plat->tx_queues_to_use = 1;
> > > > > > > > > - plat->rx_queues_to_use = 1;
> > > > > > > > > +         ret = loongson_dwmac_config_msi(pdev, plat, &res, np);
> > > > > > > > > +         break;
> > > > > > > > > + default:        /* 0x35 device and 0x37 device. */
> > > > > > > > > +         plat->tx_queues_to_use = 1;
> > > > > > > > > +         plat->rx_queues_to_use = 1;
> > > > > > > > > - ret = loongson_dwmac_config_legacy(pdev, plat, &res, np);
> > > > > > > > > +         ret = loongson_dwmac_config_legacy(pdev, plat, &res, np);
> > > > > > > > > +         break;
> > > > > > > > > + }
> > > > > > > > Let's now talk about this change.
> > > > > > > >
> > > > > > > > First of all, one more time. You can't miss the return value check
> > > > > > > > because if any of the IRQ config method fails then the driver won't
> > > > > > > > work! The first change that introduces the problem is in the patch
> > > > > > > > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy
> > > > > > > OK!
> > > > > > > > Second, as I already mentioned in another message sent to this patch
> > > > > > > > you are missing the PCI MSI IRQs freeing in the cleanup-on-error path
> > > > > > > > and in the device/driver remove() function. It's definitely wrong.
> > > > > > > You are right! I will do it.
> > > > > > > > Thirdly, you said that the node-pointer is now optional and introduced
> > > > > > > > the patch
> > > > > > > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support
> > > > > > > > If so and the DT-based setting up isn't mandatory then I would
> > > > > > > > suggest to proceed with the entire so called legacy setups only if the
> > > > > > > > node-pointer has been found, otherwise the pure PCI-based setup would
> > > > > > > > be performed. So the patches 10-13 (in your v12 order) would look
> > > > > > > In this case, MSI will not be enabled when the node-pointer is found.
> > > > > > >
> > > > > > > .
> > > > > > >
> > > > > > >
> > > > > > > In fact, a large fraction of 2k devices are DT-based, of course, many are
> > > > > > > PCI-based.
> > > > > > Then please summarise which devices need the DT-node pointer which
> > > > > > don't? And most importantly if they do why do they need the DT-node?
> > > > > Whether we need DT-nodes doesn't depend on device type, but depends on
> > > > > the BIOS type. When we boot with UEFI+ACPI, we don't need DT-node,
> > > > > when we boot with PMON+FDT, we need DT-node. Loongson machines may
> > > > > have either BIOS types.
> > > > Thanks for the answer. Just to fully clarify. Does it mean that all
> > > > Loongson Ethernet controllers (Loongson GNET and GMAC) are able to
> > > > deliver both PCI MSI IRQs and direct GIC IRQs (so called legacy)?
> > >
> >
> > > 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.)

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

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

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

> 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