Hi Rob,

On Fri, Jun 16, 2023 at 11:57:43AM -0600, Rob Herring wrote:

On Sun, Jun 4, 2023 at 2:55 AM Vladimir Oltean [off-list ref] wrote:

quoted

Sorry, just now seeing this as I've been out the last month.

quoted

On Sat, Jun 03, 2023 at 10:35:50AM +0800, Jianmin Lv wrote:

quoted

quoted

How about 3. handle of_device_is_available() in the probe function of
the "loongson, pci-gmac" driver? Would that not work?

This way does work only for the specified device. There are other devices,
such as HDA, I2S, etc, which have shared pins. Then we have to add
of_device_is_available() checking to those drivers one by one. And we are
not sure if there are other devices in new generation chips in future. So
I'm afraid that the way you mentioned is not suitable for us.

If we decided that disabled devices should probe, then that is exactly
what will have to be done. The restriction (of shared pins) is in the
devices and is potentially per device, so it makes more sense for the
device's drivers to handle than the host bridge IMO. (Assuming the
core doesn't handle a per device property.)

quoted

Got it, so you have more on-chip PCIe devices than the ones listed in
loongson64-2k1000.dtsi, and you don't want to describe them in the
device tree just to put status = "disabled" for those devices/functions
that you don't want Linux to use - although you could, and it wouldn't
be that hard or have unintended side effects.

Though you need to admit, in case you had an on-chip multi-function PCIe
device like the NXP ENETC, and you wanted Linux to not use function 0,
the strategy you're suggesting here that is acceptable for Loongson
would not have worked.

I believe we need a bit of coordination from PCIe and device tree
maintainers, to suggest what would be the encouraged best practices and
ways to solve this regression for the ENETC.

I think we need to define what behavior is correct for 'status =
"disabled"'. For almost everywhere in DT, it is equivalent to the
device is not present. A not present device doesn't probe. There are
unfortunately cases where status got ignored/forgotten and PCI was one
of those. PCI is a bit different since there are 2 sources of
information about a device being present. The intent with PCI is DT
overrides what's discovered. For example, 'vendor-id' overrides what's
read from the h/w.

I think we can fix making the status per function simply by making
'match_driver' be set based on the status. This would move the check
later to just before probing. That would not work for a case where
accessing the config registers is a problem. It doesn't sound like
that's a problem for Loongson based on the above response, but their
original solution did prevent that. This change would also mean the
PCI quirks would run. Perhaps the func0 memory clearing you need could
be run as a quirk instead?

Rob

Sorry to return to this thread very late. I had lots of other stuff to
take care of, and somehow *this* breakage had less priority :)

So, first off, there's a confusion regarding the "func0 memory clearing"
that could be run as a quirk instead. It's not memory clearing for fn 0,
but memory clearing for all ENETC functions, regardless or not whether
they have status = "disabled" or not in the device tree.

That being said, I've implemented the workaround below in a quirk as
you've said, and the quirks only get applied for those PCI functions
which don't have status = "disabled" in the device tree. So, as things
stand, it won't work.

Also, the original patch on which we're commenting ("PCI: don't skip
probing entire device if first fn OF node has status = "disabled"") is
needed in any case, because of the other issue: the PCI core thinks that
when fn 0 has status = "disabled", fn 1 .. 6 are also unavailable. False.

From 9c3b88196a7c7e2b010d051c6d48faf36791e220 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Tue, 20 Jun 2023 16:31:07 +0300
Subject: [PATCH] net: enetc: reimplement RFS/RSS memory clearing as PCI quirk

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 .../net/ethernet/freescale/enetc/enetc_pf.c   | 57 ++++++++++++++-----
 1 file changed, 43 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index 1416262d4296..b8f6f0799170 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c

@@ -1242,18 +1242,6 @@ static int enetc_pf_probe(struct pci_dev *pdev,
 	if (err)
 		goto err_setup_cbdr;

-	err = enetc_init_port_rfs_memory(si);
-	if (err) {
-		dev_err(&pdev->dev, "Failed to initialize RFS memory\n");
-		goto err_init_port_rfs;
-	}
-
-	err = enetc_init_port_rss_memory(si);
-	if (err) {
-		dev_err(&pdev->dev, "Failed to initialize RSS memory\n");
-		goto err_init_port_rss;
-	}
-
 	if (node && !of_device_is_available(node)) {
 		dev_info(&pdev->dev, "device is disabled, skipping\n");
 		err = -ENODEV;

@@ -1339,8 +1327,6 @@ static int enetc_pf_probe(struct pci_dev *pdev,
 	si->ndev = NULL;
 	free_netdev(ndev);
 err_alloc_netdev:
-err_init_port_rss:
-err_init_port_rfs:
 err_device_disabled:
 err_setup_mac_addresses:
 	enetc_teardown_cbdr(&si->cbd_ring);

@@ -1377,6 +1363,49 @@ static void enetc_pf_remove(struct pci_dev *pdev)
 	enetc_pci_remove(pdev);
 }

+static void enetc_fixup_clear_rss_rfs(struct pci_dev *pdev)
+{
+	struct enetc_si *si;
+	struct enetc_pf *pf;
+	int err;
+
+	err = enetc_pci_probe(pdev, KBUILD_MODNAME, sizeof(*pf));
+	if (err)
+		goto out;
+
+	si = pci_get_drvdata(pdev);
+	if (!si->hw.port || !si->hw.global) {
+		err = -ENODEV;
+		goto out_pci_remove;
+	}
+
+	err = enetc_setup_cbdr(&pdev->dev, &si->hw, ENETC_CBDR_DEFAULT_SIZE,
+			       &si->cbd_ring);
+	if (err)
+		goto out_pci_remove;
+
+	err = enetc_init_port_rfs_memory(si);
+	if (err)
+		goto out_teardown_cbdr;
+
+	err = enetc_init_port_rss_memory(si);
+	if (err)
+		goto out_teardown_cbdr;
+
+out_teardown_cbdr:
+	enetc_teardown_cbdr(&si->cbd_ring);
+out_pci_remove:
+	enetc_pci_remove(pdev);
+out:
+	if (err) {
+		dev_err(&pdev->dev,
+			"Failed to apply PCI fixup for clearing RFS/RSS memories: %pe\n",
+			ERR_PTR(err));
+	}
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_FREESCALE, ENETC_DEV_ID_PF,
+			enetc_fixup_clear_rss_rfs);
+
 static const struct pci_device_id enetc_pf_id_table[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, ENETC_DEV_ID_PF) },
 	{ 0, } /* End of table. */
--

2.34.1