[PATCH v6 0/9] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[PATCH v6 0/9] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

This patchset includes misc patchs:

The patch 1 fixup the mtk_pcie_find_port logic which will cause system
could not touch the EP's configuration space that connected to PCIe slot 1.

The patch 2 fixup the class type for MT7622.
The patch 6 fixup the enable msi logic, the operation to enable msi
should be after system clock is enabled. Call mtk_pcie_enable_msi in
mtk_pcie_startup_port_v2 since the clock was all enabled at that time.

The patch 7 was rebased and refactor of the v4 patch[1], changes are:
 -Add PM support for MT7622.
 -Using mtk_pcie_enable_port to re-establish the link when resumed.
 -Rebased on this patchset.

The patch 9 add loadable kernel module support.

[1] https://patchwork.kernel.org/patch/10479079

Change since v5:
 - A bit improvement of mtk_pcie_find_port suggest by Lorenzo.
 - Add fixup tags of fix enable MSI logic in patch 6.
 - Add Acked-by tags from Ryder.

Change since v4:
 - Add patch 2 to fixup class type for MT7622.
 - Add patch 3 to remove the redundant dev->pm_domain check
 - Add patch 4 to covert to use pci_host_probe
 - Add patch 5 to re-arrange the function define, this is a prepare patch for
   fixup the enable_msi logic, no functional changed have been made by this one.
 - Add patch 8 to save the GIC IRQ in mtk_pcie_port as a prepare patch for tear
   down the irq when remove the kernel module.
 - Re-arrange the find_port flow suggest by Lorenzo to make the code parse easier
   for the patch 1.
 - Remove the .pm_support in mtk_pcie_soc in patch 7 since if system pm was not
   supported, then those pm callbacks will never be executed for MT7622. So the
   .pm_support is not needed.

Change since v3:
 - Remove pm_runtime_XXX ops in suspend and resume callbacks in the third patch.
 - Rebase to 4.19-rc1.

Change since v2:
 - Fix the list_for_each_entry_safe parameter error.
 - Add Ryder's Acked-by flag.

Change since v1:
 - A bit of code refact of the first patch suggested by Andy Shevchenko, and
   commit message updated.
 - Using __maybe_unused.
 - Remove the redundant list_empty check of the fourth patch.


Honghui Zhang (9):
  PCI: mediatek: Using slot's devfn for compare to fix
    mtk_pcie_find_port logic
  PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI
  PCI: mediatek: Remove the redundant dev->pm_domain check
  PCI: mediatek: Convert to use pci_host_probe()
  PCI: mediatek: Move the mtk_pcie_startup_port_v2 function's define
    after mtk_pcie_setup_irq
  PCI: mediatek: Fixup enable msi logic by enable msi after clock
    enabled
  PCI: mediatek: Add system pm support for MT2712 and MT7622
  PCI: mediatek: Save the GIC IRQ in mtk_pcie_port
  PCI: mediatek: Add loadable kernel module support

 drivers/pci/controller/Kconfig         |   2 +-
 drivers/pci/controller/pcie-mediatek.c | 320 +++++++++++++++++++++------------
 2 files changed, 205 insertions(+), 117 deletions(-)

-- 
2.6.4

[PATCH v6 0/9] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

This patchset includes misc patchs:

The patch 1 fixup the mtk_pcie_find_port logic which will cause system
could not touch the EP's configuration space that connected to PCIe slot 1.

The patch 2 fixup the class type for MT7622.
The patch 6 fixup the enable msi logic, the operation to enable msi
should be after system clock is enabled. Call mtk_pcie_enable_msi in
mtk_pcie_startup_port_v2 since the clock was all enabled at that time.

The patch 7 was rebased and refactor of the v4 patch[1], changes are:
 -Add PM support for MT7622.
 -Using mtk_pcie_enable_port to re-establish the link when resumed.
 -Rebased on this patchset.

The patch 9 add loadable kernel module support.

[1] https://patchwork.kernel.org/patch/10479079

Change since v5:
 - A bit improvement of mtk_pcie_find_port suggest by Lorenzo.
 - Add fixup tags of fix enable MSI logic in patch 6.
 - Add Acked-by tags from Ryder.

Change since v4:
 - Add patch 2 to fixup class type for MT7622.
 - Add patch 3 to remove the redundant dev->pm_domain check
 - Add patch 4 to covert to use pci_host_probe
 - Add patch 5 to re-arrange the function define, this is a prepare patch for
   fixup the enable_msi logic, no functional changed have been made by this one.
 - Add patch 8 to save the GIC IRQ in mtk_pcie_port as a prepare patch for tear
   down the irq when remove the kernel module.
 - Re-arrange the find_port flow suggest by Lorenzo to make the code parse easier
   for the patch 1.
 - Remove the .pm_support in mtk_pcie_soc in patch 7 since if system pm was not
   supported, then those pm callbacks will never be executed for MT7622. So the
   .pm_support is not needed.

Change since v3:
 - Remove pm_runtime_XXX ops in suspend and resume callbacks in the third patch.
 - Rebase to 4.19-rc1.

Change since v2:
 - Fix the list_for_each_entry_safe parameter error.
 - Add Ryder's Acked-by flag.

Change since v1:
 - A bit of code refact of the first patch suggested by Andy Shevchenko, and
   commit message updated.
 - Using __maybe_unused.
 - Remove the redundant list_empty check of the fourth patch.


Honghui Zhang (9):
  PCI: mediatek: Using slot's devfn for compare to fix
    mtk_pcie_find_port logic
  PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI
  PCI: mediatek: Remove the redundant dev->pm_domain check
  PCI: mediatek: Convert to use pci_host_probe()
  PCI: mediatek: Move the mtk_pcie_startup_port_v2 function's define
    after mtk_pcie_setup_irq
  PCI: mediatek: Fixup enable msi logic by enable msi after clock
    enabled
  PCI: mediatek: Add system pm support for MT2712 and MT7622
  PCI: mediatek: Save the GIC IRQ in mtk_pcie_port
  PCI: mediatek: Add loadable kernel module support

 drivers/pci/controller/Kconfig         |   2 +-
 drivers/pci/controller/pcie-mediatek.c | 320 +++++++++++++++++++++------------
 2 files changed, 205 insertions(+), 117 deletions(-)

-- 
2.6.4

[PATCH v6 0/9] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[honghui.zhang at mediatek.com]

From: Honghui Zhang <honghui.zhang@mediatek.com>

This patchset includes misc patchs:

The patch 1 fixup the mtk_pcie_find_port logic which will cause system
could not touch the EP's configuration space that connected to PCIe slot 1.

The patch 2 fixup the class type for MT7622.
The patch 6 fixup the enable msi logic, the operation to enable msi
should be after system clock is enabled. Call mtk_pcie_enable_msi in
mtk_pcie_startup_port_v2 since the clock was all enabled at that time.

The patch 7 was rebased and refactor of the v4 patch[1], changes are:
 -Add PM support for MT7622.
 -Using mtk_pcie_enable_port to re-establish the link when resumed.
 -Rebased on this patchset.

The patch 9 add loadable kernel module support.

[1] https://patchwork.kernel.org/patch/10479079

Change since v5:
 - A bit improvement of mtk_pcie_find_port suggest by Lorenzo.
 - Add fixup tags of fix enable MSI logic in patch 6.
 - Add Acked-by tags from Ryder.

Change since v4:
 - Add patch 2 to fixup class type for MT7622.
 - Add patch 3 to remove the redundant dev->pm_domain check
 - Add patch 4 to covert to use pci_host_probe
 - Add patch 5 to re-arrange the function define, this is a prepare patch for
   fixup the enable_msi logic, no functional changed have been made by this one.
 - Add patch 8 to save the GIC IRQ in mtk_pcie_port as a prepare patch for tear
   down the irq when remove the kernel module.
 - Re-arrange the find_port flow suggest by Lorenzo to make the code parse easier
   for the patch 1.
 - Remove the .pm_support in mtk_pcie_soc in patch 7 since if system pm was not
   supported, then those pm callbacks will never be executed for MT7622. So the
   .pm_support is not needed.

Change since v3:
 - Remove pm_runtime_XXX ops in suspend and resume callbacks in the third patch.
 - Rebase to 4.19-rc1.

Change since v2:
 - Fix the list_for_each_entry_safe parameter error.
 - Add Ryder's Acked-by flag.

Change since v1:
 - A bit of code refact of the first patch suggested by Andy Shevchenko, and
   commit message updated.
 - Using __maybe_unused.
 - Remove the redundant list_empty check of the fourth patch.


Honghui Zhang (9):
  PCI: mediatek: Using slot's devfn for compare to fix
    mtk_pcie_find_port logic
  PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI
  PCI: mediatek: Remove the redundant dev->pm_domain check
  PCI: mediatek: Convert to use pci_host_probe()
  PCI: mediatek: Move the mtk_pcie_startup_port_v2 function's define
    after mtk_pcie_setup_irq
  PCI: mediatek: Fixup enable msi logic by enable msi after clock
    enabled
  PCI: mediatek: Add system pm support for MT2712 and MT7622
  PCI: mediatek: Save the GIC IRQ in mtk_pcie_port
  PCI: mediatek: Add loadable kernel module support

 drivers/pci/controller/Kconfig         |   2 +-
 drivers/pci/controller/pcie-mediatek.c | 320 +++++++++++++++++++++------------
 2 files changed, 205 insertions(+), 117 deletions(-)

-- 
2.6.4

[PATCH v6 1/9] PCI: mediatek: Using slot's devfn for compare to fix mtk_pcie_find_port logic

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The Mediatek's host controller has two slots, each with it's own control
registers. The host driver need to identify which slot was connected
in order to access the device's configuration space. There's problem
for current host driver to find out which slot was connected to for
a given EP device.

Assuming each slot have connect with one EP device as below:

                host bridge
  bus 0 --> __________|_______
           |                  |
           |                  |
         slot 0             slot 1
  bus 1 -->|        bus 2 --> |
           |                  |
         EP 0               EP 1

During PCI enumeration, system software will scan all the PCI device
starting from devfn 0. So it will get the proper port for slot0 and
slot1 device when using PCI_SLOT(devfn) for match. But it will get
the wrong slot for EP1: The devfn will be start from 0 when scanning
EP1 behind slot1, it will get port0 since the PCI_SLOT(EP1) is match
for port0's slot value. So the host driver should not using EP's devfn
but the slot's devfn(the slot which EP was connected to) for match.

This patch fix the mtk_pcie_find_port's logic by using the slot's
devfn for match if finding device connected to the subordinate bus.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 9999dae..288b8e2 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -337,6 +337,17 @@ static struct mtk_pcie_port *mtk_pcie_find_port(struct pci_bus *bus,
 {
 	struct mtk_pcie *pcie = bus->sysdata;
 	struct mtk_pcie_port *port;
+	struct pci_dev *dev = NULL;
+
+	/*
+	 * Walk the bus hierarchy to get the devfn value
+	 * of the port in the root bus.
+	 */
+	while (bus && bus->number) {
+		dev = bus->self;
+		bus = dev->bus;
+		devfn = dev->devfn;
+	}
 
 	list_for_each_entry(port, &pcie->ports, list)
 		if (port->slot == PCI_SLOT(devfn))
-- 
2.6.4

[PATCH v6 1/9] PCI: mediatek: Using slot's devfn for compare to fix mtk_pcie_find_port logic

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The Mediatek's host controller has two slots, each with it's own control
registers. The host driver need to identify which slot was connected
in order to access the device's configuration space. There's problem
for current host driver to find out which slot was connected to for
a given EP device.

Assuming each slot have connect with one EP device as below:

                host bridge
  bus 0 --> __________|_______
           |                  |
           |                  |
         slot 0             slot 1
  bus 1 -->|        bus 2 --> |
           |                  |
         EP 0               EP 1

During PCI enumeration, system software will scan all the PCI device
starting from devfn 0. So it will get the proper port for slot0 and
slot1 device when using PCI_SLOT(devfn) for match. But it will get
the wrong slot for EP1: The devfn will be start from 0 when scanning
EP1 behind slot1, it will get port0 since the PCI_SLOT(EP1) is match
for port0's slot value. So the host driver should not using EP's devfn
but the slot's devfn(the slot which EP was connected to) for match.

This patch fix the mtk_pcie_find_port's logic by using the slot's
devfn for match if finding device connected to the subordinate bus.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 9999dae..288b8e2 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -337,6 +337,17 @@ static struct mtk_pcie_port *mtk_pcie_find_port(struct pci_bus *bus,
 {
 	struct mtk_pcie *pcie = bus->sysdata;
 	struct mtk_pcie_port *port;
+	struct pci_dev *dev = NULL;
+
+	/*
+	 * Walk the bus hierarchy to get the devfn value
+	 * of the port in the root bus.
+	 */
+	while (bus && bus->number) {
+		dev = bus->self;
+		bus = dev->bus;
+		devfn = dev->devfn;
+	}
 
 	list_for_each_entry(port, &pcie->ports, list)
 		if (port->slot == PCI_SLOT(devfn))
-- 
2.6.4

[PATCH v6 1/9] PCI: mediatek: Using slot's devfn for compare to fix mtk_pcie_find_port logic

[honghui.zhang at mediatek.com]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The Mediatek's host controller has two slots, each with it's own control
registers. The host driver need to identify which slot was connected
in order to access the device's configuration space. There's problem
for current host driver to find out which slot was connected to for
a given EP device.

Assuming each slot have connect with one EP device as below:

                host bridge
  bus 0 --> __________|_______
           |                  |
           |                  |
         slot 0             slot 1
  bus 1 -->|        bus 2 --> |
           |                  |
         EP 0               EP 1

During PCI enumeration, system software will scan all the PCI device
starting from devfn 0. So it will get the proper port for slot0 and
slot1 device when using PCI_SLOT(devfn) for match. But it will get
the wrong slot for EP1: The devfn will be start from 0 when scanning
EP1 behind slot1, it will get port0 since the PCI_SLOT(EP1) is match
for port0's slot value. So the host driver should not using EP's devfn
but the slot's devfn(the slot which EP was connected to) for match.

This patch fix the mtk_pcie_find_port's logic by using the slot's
devfn for match if finding device connected to the subordinate bus.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 9999dae..288b8e2 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -337,6 +337,17 @@ static struct mtk_pcie_port *mtk_pcie_find_port(struct pci_bus *bus,
 {
 	struct mtk_pcie *pcie = bus->sysdata;
 	struct mtk_pcie_port *port;
+	struct pci_dev *dev = NULL;
+
+	/*
+	 * Walk the bus hierarchy to get the devfn value
+	 * of the port in the root bus.
+	 */
+	while (bus && bus->number) {
+		dev = bus->self;
+		bus = dev->bus;
+		devfn = dev->devfn;
+	}
 
 	list_for_each_entry(port, &pcie->ports, list)
 		if (port->slot == PCI_SLOT(devfn))
-- 
2.6.4

[PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The PCIe controller of MT7622 has TYPE 1 configuration space type, but
the HW default class type values is invalid.

The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
type for MT7622") have set the class ID for MT7622 as
PCI_CLASS_BRIDGE_HOST, but it's not workable for MT7622:

In __pci_bus_assign_resources, the framework only setup bridge's
resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
will leave the subordinary PCIe device's MMIO window un-touched.

Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
driver do.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 288b8e2..bcdac9b 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -432,7 +432,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
 		val = PCI_VENDOR_ID_MEDIATEK;
 		writew(val, port->base + PCIE_CONF_VEND_ID);
 
-		val = PCI_CLASS_BRIDGE_HOST;
+		val = PCI_CLASS_BRIDGE_PCI;
 		writew(val, port->base + PCIE_CONF_CLASS_ID);
 	}
 
-- 
2.6.4

[PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[honghui.zhang-NuS5LvNUpcJWk0Htik3J/w]

From: Honghui Zhang <honghui.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

The PCIe controller of MT7622 has TYPE 1 configuration space type, but
the HW default class type values is invalid.

The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
type for MT7622") have set the class ID for MT7622 as
PCI_CLASS_BRIDGE_HOST, but it's not workable for MT7622:

In __pci_bus_assign_resources, the framework only setup bridge's
resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
will leave the subordinary PCIe device's MMIO window un-touched.

Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
driver do.

Signed-off-by: Honghui Zhang <honghui.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Acked-by: Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 drivers/pci/controller/pcie-mediatek.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 288b8e2..bcdac9b 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -432,7 +432,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
 		val = PCI_VENDOR_ID_MEDIATEK;
 		writew(val, port->base + PCIE_CONF_VEND_ID);
 
-		val = PCI_CLASS_BRIDGE_HOST;
+		val = PCI_CLASS_BRIDGE_PCI;
 		writew(val, port->base + PCIE_CONF_CLASS_ID);
 	}
 
-- 
2.6.4

[PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[honghui.zhang at mediatek.com]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The PCIe controller of MT7622 has TYPE 1 configuration space type, but
the HW default class type values is invalid.

The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
type for MT7622") have set the class ID for MT7622 as
PCI_CLASS_BRIDGE_HOST, but it's not workable for MT7622:

In __pci_bus_assign_resources, the framework only setup bridge's
resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
will leave the subordinary PCIe device's MMIO window un-touched.

Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
driver do.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 288b8e2..bcdac9b 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -432,7 +432,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
 		val = PCI_VENDOR_ID_MEDIATEK;
 		writew(val, port->base + PCIE_CONF_VEND_ID);
 
-		val = PCI_CLASS_BRIDGE_HOST;
+		val = PCI_CLASS_BRIDGE_PCI;
 		writew(val, port->base + PCIE_CONF_CLASS_ID);
 	}
 
-- 
2.6.4

[PATCH v6 3/9] PCI: mediatek: Remove the redundant dev->pm_domain check

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

It's no needed to check whether device have pm_domain attached before
calling the pm_runtime_XXX interface, remove it.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index bcdac9b..59fdb60 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -225,10 +225,8 @@ static void mtk_pcie_subsys_powerdown(struct mtk_pcie *pcie)
 
 	clk_disable_unprepare(pcie->free_ck);
 
-	if (dev->pm_domain) {
-		pm_runtime_put_sync(dev);
-		pm_runtime_disable(dev);
-	}
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
 }
 
 static void mtk_pcie_port_free(struct mtk_pcie_port *port)
@@ -998,10 +996,8 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)
 		pcie->free_ck = NULL;
 	}
 
-	if (dev->pm_domain) {
-		pm_runtime_enable(dev);
-		pm_runtime_get_sync(dev);
-	}
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
 
 	/* enable top level clock */
 	err = clk_prepare_enable(pcie->free_ck);
@@ -1013,10 +1009,8 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)
 	return 0;
 
 err_free_ck:
-	if (dev->pm_domain) {
-		pm_runtime_put_sync(dev);
-		pm_runtime_disable(dev);
-	}
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
 
 	return err;
 }
-- 
2.6.4

[PATCH v6 3/9] PCI: mediatek: Remove the redundant dev->pm_domain check

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

It's no needed to check whether device have pm_domain attached before
calling the pm_runtime_XXX interface, remove it.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index bcdac9b..59fdb60 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -225,10 +225,8 @@ static void mtk_pcie_subsys_powerdown(struct mtk_pcie *pcie)
 
 	clk_disable_unprepare(pcie->free_ck);
 
-	if (dev->pm_domain) {
-		pm_runtime_put_sync(dev);
-		pm_runtime_disable(dev);
-	}
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
 }
 
 static void mtk_pcie_port_free(struct mtk_pcie_port *port)
@@ -998,10 +996,8 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)
 		pcie->free_ck = NULL;
 	}
 
-	if (dev->pm_domain) {
-		pm_runtime_enable(dev);
-		pm_runtime_get_sync(dev);
-	}
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
 
 	/* enable top level clock */
 	err = clk_prepare_enable(pcie->free_ck);
@@ -1013,10 +1009,8 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)
 	return 0;
 
 err_free_ck:
-	if (dev->pm_domain) {
-		pm_runtime_put_sync(dev);
-		pm_runtime_disable(dev);
-	}
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
 
 	return err;
 }
-- 
2.6.4

[PATCH v6 3/9] PCI: mediatek: Remove the redundant dev->pm_domain check

[honghui.zhang at mediatek.com]

From: Honghui Zhang <honghui.zhang@mediatek.com>

It's no needed to check whether device have pm_domain attached before
calling the pm_runtime_XXX interface, remove it.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index bcdac9b..59fdb60 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -225,10 +225,8 @@ static void mtk_pcie_subsys_powerdown(struct mtk_pcie *pcie)
 
 	clk_disable_unprepare(pcie->free_ck);
 
-	if (dev->pm_domain) {
-		pm_runtime_put_sync(dev);
-		pm_runtime_disable(dev);
-	}
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
 }
 
 static void mtk_pcie_port_free(struct mtk_pcie_port *port)
@@ -998,10 +996,8 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)
 		pcie->free_ck = NULL;
 	}
 
-	if (dev->pm_domain) {
-		pm_runtime_enable(dev);
-		pm_runtime_get_sync(dev);
-	}
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
 
 	/* enable top level clock */
 	err = clk_prepare_enable(pcie->free_ck);
@@ -1013,10 +1009,8 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)
 	return 0;
 
 err_free_ck:
-	if (dev->pm_domain) {
-		pm_runtime_put_sync(dev);
-		pm_runtime_disable(dev);
-	}
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
 
 	return err;
 }
-- 
2.6.4

[PATCH v6 4/9] PCI: mediatek: Convert to use pci_host_probe()

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

Part of mtk_pcie_register_host is an open-coded version of
pci_host_probe(). So instead of duplicating this code, use
pci_host_probe() directly and remove mtk_pcie_register_host.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 37 ++++++++--------------------------
 1 file changed, 8 insertions(+), 29 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 59fdb60..ead6005 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -1121,34 +1121,6 @@ static int mtk_pcie_request_resources(struct mtk_pcie *pcie)
 	return 0;
 }
 
-static int mtk_pcie_register_host(struct pci_host_bridge *host)
-{
-	struct mtk_pcie *pcie = pci_host_bridge_priv(host);
-	struct pci_bus *child;
-	int err;
-
-	host->busnr = pcie->busn.start;
-	host->dev.parent = pcie->dev;
-	host->ops = pcie->soc->ops;
-	host->map_irq = of_irq_parse_and_map_pci;
-	host->swizzle_irq = pci_common_swizzle;
-	host->sysdata = pcie;
-
-	err = pci_scan_root_bus_bridge(host);
-	if (err < 0)
-		return err;
-
-	pci_bus_size_bridges(host->bus);
-	pci_bus_assign_resources(host->bus);
-
-	list_for_each_entry(child, &host->bus->children, node)
-		pcie_bus_configure_settings(child);
-
-	pci_bus_add_devices(host->bus);
-
-	return 0;
-}
-
 static int mtk_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1175,7 +1147,14 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	if (err)
 		goto put_resources;
 
-	err = mtk_pcie_register_host(host);
+	host->busnr = pcie->busn.start;
+	host->dev.parent = pcie->dev;
+	host->ops = pcie->soc->ops;
+	host->map_irq = of_irq_parse_and_map_pci;
+	host->swizzle_irq = pci_common_swizzle;
+	host->sysdata = pcie;
+
+	err = pci_host_probe(host);
 	if (err)
 		goto put_resources;
 
-- 
2.6.4

[PATCH v6 4/9] PCI: mediatek: Convert to use pci_host_probe()

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

Part of mtk_pcie_register_host is an open-coded version of
pci_host_probe(). So instead of duplicating this code, use
pci_host_probe() directly and remove mtk_pcie_register_host.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 37 ++++++++--------------------------
 1 file changed, 8 insertions(+), 29 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 59fdb60..ead6005 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -1121,34 +1121,6 @@ static int mtk_pcie_request_resources(struct mtk_pcie *pcie)
 	return 0;
 }
 
-static int mtk_pcie_register_host(struct pci_host_bridge *host)
-{
-	struct mtk_pcie *pcie = pci_host_bridge_priv(host);
-	struct pci_bus *child;
-	int err;
-
-	host->busnr = pcie->busn.start;
-	host->dev.parent = pcie->dev;
-	host->ops = pcie->soc->ops;
-	host->map_irq = of_irq_parse_and_map_pci;
-	host->swizzle_irq = pci_common_swizzle;
-	host->sysdata = pcie;
-
-	err = pci_scan_root_bus_bridge(host);
-	if (err < 0)
-		return err;
-
-	pci_bus_size_bridges(host->bus);
-	pci_bus_assign_resources(host->bus);
-
-	list_for_each_entry(child, &host->bus->children, node)
-		pcie_bus_configure_settings(child);
-
-	pci_bus_add_devices(host->bus);
-
-	return 0;
-}
-
 static int mtk_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1175,7 +1147,14 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	if (err)
 		goto put_resources;
 
-	err = mtk_pcie_register_host(host);
+	host->busnr = pcie->busn.start;
+	host->dev.parent = pcie->dev;
+	host->ops = pcie->soc->ops;
+	host->map_irq = of_irq_parse_and_map_pci;
+	host->swizzle_irq = pci_common_swizzle;
+	host->sysdata = pcie;
+
+	err = pci_host_probe(host);
 	if (err)
 		goto put_resources;
 
-- 
2.6.4

[PATCH v6 4/9] PCI: mediatek: Convert to use pci_host_probe()

[honghui.zhang at mediatek.com]

From: Honghui Zhang <honghui.zhang@mediatek.com>

Part of mtk_pcie_register_host is an open-coded version of
pci_host_probe(). So instead of duplicating this code, use
pci_host_probe() directly and remove mtk_pcie_register_host.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 37 ++++++++--------------------------
 1 file changed, 8 insertions(+), 29 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 59fdb60..ead6005 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -1121,34 +1121,6 @@ static int mtk_pcie_request_resources(struct mtk_pcie *pcie)
 	return 0;
 }
 
-static int mtk_pcie_register_host(struct pci_host_bridge *host)
-{
-	struct mtk_pcie *pcie = pci_host_bridge_priv(host);
-	struct pci_bus *child;
-	int err;
-
-	host->busnr = pcie->busn.start;
-	host->dev.parent = pcie->dev;
-	host->ops = pcie->soc->ops;
-	host->map_irq = of_irq_parse_and_map_pci;
-	host->swizzle_irq = pci_common_swizzle;
-	host->sysdata = pcie;
-
-	err = pci_scan_root_bus_bridge(host);
-	if (err < 0)
-		return err;
-
-	pci_bus_size_bridges(host->bus);
-	pci_bus_assign_resources(host->bus);
-
-	list_for_each_entry(child, &host->bus->children, node)
-		pcie_bus_configure_settings(child);
-
-	pci_bus_add_devices(host->bus);
-
-	return 0;
-}
-
 static int mtk_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1175,7 +1147,14 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	if (err)
 		goto put_resources;
 
-	err = mtk_pcie_register_host(host);
+	host->busnr = pcie->busn.start;
+	host->dev.parent = pcie->dev;
+	host->ops = pcie->soc->ops;
+	host->map_irq = of_irq_parse_and_map_pci;
+	host->swizzle_irq = pci_common_swizzle;
+	host->sysdata = pcie;
+
+	err = pci_host_probe(host);
 	if (err)
 		goto put_resources;
 
-- 
2.6.4

[PATCH v6 5/9] PCI: mediatek: Move the mtk_pcie_startup_port_v2 function's define after mtk_pcie_setup_irq

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

This is a prepare patch to fix enable MSI logic, move the function's
define later to avoid forward declaration of mtk_pcie_enable_msi in
the future. No functional changed.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 138 ++++++++++++++++-----------------
 1 file changed, 69 insertions(+), 69 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index ead6005..654a63e 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -392,75 +392,6 @@ static struct pci_ops mtk_pcie_ops_v2 = {
 	.write = mtk_pcie_config_write,
 };
 
-static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
-{
-	struct mtk_pcie *pcie = port->pcie;
-	struct resource *mem = &pcie->mem;
-	const struct mtk_pcie_soc *soc = port->pcie->soc;
-	u32 val;
-	size_t size;
-	int err;
-
-	/* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
-	if (pcie->base) {
-		val = readl(pcie->base + PCIE_SYS_CFG_V2);
-		val |= PCIE_CSR_LTSSM_EN(port->slot) |
-		       PCIE_CSR_ASPM_L1_EN(port->slot);
-		writel(val, pcie->base + PCIE_SYS_CFG_V2);
-	}
-
-	/* Assert all reset signals */
-	writel(0, port->base + PCIE_RST_CTRL);
-
-	/*
-	 * Enable PCIe link down reset, if link status changed from link up to
-	 * link down, this will reset MAC control registers and configuration
-	 * space.
-	 */
-	writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
-
-	/* De-assert PHY, PE, PIPE, MAC and configuration reset	*/
-	val = readl(port->base + PCIE_RST_CTRL);
-	val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
-	       PCIE_MAC_SRSTB | PCIE_CRSTB;
-	writel(val, port->base + PCIE_RST_CTRL);
-
-	/* Set up vendor ID and class code */
-	if (soc->need_fix_class_id) {
-		val = PCI_VENDOR_ID_MEDIATEK;
-		writew(val, port->base + PCIE_CONF_VEND_ID);
-
-		val = PCI_CLASS_BRIDGE_PCI;
-		writew(val, port->base + PCIE_CONF_CLASS_ID);
-	}
-
-	/* 100ms timeout value should be enough for Gen1/2 training */
-	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val,
-				 !!(val & PCIE_PORT_LINKUP_V2), 20,
-				 100 * USEC_PER_MSEC);
-	if (err)
-		return -ETIMEDOUT;
-
-	/* Set INTx mask */
-	val = readl(port->base + PCIE_INT_MASK);
-	val &= ~INTX_MASK;
-	writel(val, port->base + PCIE_INT_MASK);
-
-	/* Set AHB to PCIe translation windows */
-	size = mem->end - mem->start;
-	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
-	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
-
-	val = upper_32_bits(mem->start);
-	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
-
-	/* Set PCIe to AXI translation memory space.*/
-	val = fls(0xffffffff) | WIN_ENABLE;
-	writel(val, port->base + PCIE_AXI_WINDOW0);
-
-	return 0;
-}
-
 static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 {
 	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
@@ -705,6 +636,75 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 	return 0;
 }
 
+static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
+{
+	struct mtk_pcie *pcie = port->pcie;
+	struct resource *mem = &pcie->mem;
+	const struct mtk_pcie_soc *soc = port->pcie->soc;
+	u32 val;
+	size_t size;
+	int err;
+
+	/* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
+	if (pcie->base) {
+		val = readl(pcie->base + PCIE_SYS_CFG_V2);
+		val |= PCIE_CSR_LTSSM_EN(port->slot) |
+		       PCIE_CSR_ASPM_L1_EN(port->slot);
+		writel(val, pcie->base + PCIE_SYS_CFG_V2);
+	}
+
+	/* Assert all reset signals */
+	writel(0, port->base + PCIE_RST_CTRL);
+
+	/*
+	 * Enable PCIe link down reset, if link status changed from link up to
+	 * link down, this will reset MAC control registers and configuration
+	 * space.
+	 */
+	writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
+
+	/* De-assert PHY, PE, PIPE, MAC and configuration reset	*/
+	val = readl(port->base + PCIE_RST_CTRL);
+	val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
+	       PCIE_MAC_SRSTB | PCIE_CRSTB;
+	writel(val, port->base + PCIE_RST_CTRL);
+
+	/* Set up vendor ID and class code */
+	if (soc->need_fix_class_id) {
+		val = PCI_VENDOR_ID_MEDIATEK;
+		writew(val, port->base + PCIE_CONF_VEND_ID);
+
+		val = PCI_CLASS_BRIDGE_PCI;
+		writew(val, port->base + PCIE_CONF_CLASS_ID);
+	}
+
+	/* 100ms timeout value should be enough for Gen1/2 training */
+	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val,
+				 !!(val & PCIE_PORT_LINKUP_V2), 20,
+				 100 * USEC_PER_MSEC);
+	if (err)
+		return -ETIMEDOUT;
+
+	/* Set INTx mask */
+	val = readl(port->base + PCIE_INT_MASK);
+	val &= ~INTX_MASK;
+	writel(val, port->base + PCIE_INT_MASK);
+
+	/* Set AHB to PCIe translation windows */
+	size = mem->end - mem->start;
+	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
+	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
+
+	val = upper_32_bits(mem->start);
+	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
+
+	/* Set PCIe to AXI translation memory space.*/
+	val = fls(0xffffffff) | WIN_ENABLE;
+	writel(val, port->base + PCIE_AXI_WINDOW0);
+
+	return 0;
+}
+
 static void __iomem *mtk_pcie_map_bus(struct pci_bus *bus,
 				      unsigned int devfn, int where)
 {
-- 
2.6.4

[PATCH v6 5/9] PCI: mediatek: Move the mtk_pcie_startup_port_v2 function's define after mtk_pcie_setup_irq

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

This is a prepare patch to fix enable MSI logic, move the function's
define later to avoid forward declaration of mtk_pcie_enable_msi in
the future. No functional changed.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 138 ++++++++++++++++-----------------
 1 file changed, 69 insertions(+), 69 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index ead6005..654a63e 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -392,75 +392,6 @@ static struct pci_ops mtk_pcie_ops_v2 = {
 	.write = mtk_pcie_config_write,
 };
 
-static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
-{
-	struct mtk_pcie *pcie = port->pcie;
-	struct resource *mem = &pcie->mem;
-	const struct mtk_pcie_soc *soc = port->pcie->soc;
-	u32 val;
-	size_t size;
-	int err;
-
-	/* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
-	if (pcie->base) {
-		val = readl(pcie->base + PCIE_SYS_CFG_V2);
-		val |= PCIE_CSR_LTSSM_EN(port->slot) |
-		       PCIE_CSR_ASPM_L1_EN(port->slot);
-		writel(val, pcie->base + PCIE_SYS_CFG_V2);
-	}
-
-	/* Assert all reset signals */
-	writel(0, port->base + PCIE_RST_CTRL);
-
-	/*
-	 * Enable PCIe link down reset, if link status changed from link up to
-	 * link down, this will reset MAC control registers and configuration
-	 * space.
-	 */
-	writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
-
-	/* De-assert PHY, PE, PIPE, MAC and configuration reset	*/
-	val = readl(port->base + PCIE_RST_CTRL);
-	val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
-	       PCIE_MAC_SRSTB | PCIE_CRSTB;
-	writel(val, port->base + PCIE_RST_CTRL);
-
-	/* Set up vendor ID and class code */
-	if (soc->need_fix_class_id) {
-		val = PCI_VENDOR_ID_MEDIATEK;
-		writew(val, port->base + PCIE_CONF_VEND_ID);
-
-		val = PCI_CLASS_BRIDGE_PCI;
-		writew(val, port->base + PCIE_CONF_CLASS_ID);
-	}
-
-	/* 100ms timeout value should be enough for Gen1/2 training */
-	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val,
-				 !!(val & PCIE_PORT_LINKUP_V2), 20,
-				 100 * USEC_PER_MSEC);
-	if (err)
-		return -ETIMEDOUT;
-
-	/* Set INTx mask */
-	val = readl(port->base + PCIE_INT_MASK);
-	val &= ~INTX_MASK;
-	writel(val, port->base + PCIE_INT_MASK);
-
-	/* Set AHB to PCIe translation windows */
-	size = mem->end - mem->start;
-	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
-	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
-
-	val = upper_32_bits(mem->start);
-	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
-
-	/* Set PCIe to AXI translation memory space.*/
-	val = fls(0xffffffff) | WIN_ENABLE;
-	writel(val, port->base + PCIE_AXI_WINDOW0);
-
-	return 0;
-}
-
 static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 {
 	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
@@ -705,6 +636,75 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 	return 0;
 }
 
+static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
+{
+	struct mtk_pcie *pcie = port->pcie;
+	struct resource *mem = &pcie->mem;
+	const struct mtk_pcie_soc *soc = port->pcie->soc;
+	u32 val;
+	size_t size;
+	int err;
+
+	/* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
+	if (pcie->base) {
+		val = readl(pcie->base + PCIE_SYS_CFG_V2);
+		val |= PCIE_CSR_LTSSM_EN(port->slot) |
+		       PCIE_CSR_ASPM_L1_EN(port->slot);
+		writel(val, pcie->base + PCIE_SYS_CFG_V2);
+	}
+
+	/* Assert all reset signals */
+	writel(0, port->base + PCIE_RST_CTRL);
+
+	/*
+	 * Enable PCIe link down reset, if link status changed from link up to
+	 * link down, this will reset MAC control registers and configuration
+	 * space.
+	 */
+	writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
+
+	/* De-assert PHY, PE, PIPE, MAC and configuration reset	*/
+	val = readl(port->base + PCIE_RST_CTRL);
+	val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
+	       PCIE_MAC_SRSTB | PCIE_CRSTB;
+	writel(val, port->base + PCIE_RST_CTRL);
+
+	/* Set up vendor ID and class code */
+	if (soc->need_fix_class_id) {
+		val = PCI_VENDOR_ID_MEDIATEK;
+		writew(val, port->base + PCIE_CONF_VEND_ID);
+
+		val = PCI_CLASS_BRIDGE_PCI;
+		writew(val, port->base + PCIE_CONF_CLASS_ID);
+	}
+
+	/* 100ms timeout value should be enough for Gen1/2 training */
+	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val,
+				 !!(val & PCIE_PORT_LINKUP_V2), 20,
+				 100 * USEC_PER_MSEC);
+	if (err)
+		return -ETIMEDOUT;
+
+	/* Set INTx mask */
+	val = readl(port->base + PCIE_INT_MASK);
+	val &= ~INTX_MASK;
+	writel(val, port->base + PCIE_INT_MASK);
+
+	/* Set AHB to PCIe translation windows */
+	size = mem->end - mem->start;
+	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
+	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
+
+	val = upper_32_bits(mem->start);
+	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
+
+	/* Set PCIe to AXI translation memory space.*/
+	val = fls(0xffffffff) | WIN_ENABLE;
+	writel(val, port->base + PCIE_AXI_WINDOW0);
+
+	return 0;
+}
+
 static void __iomem *mtk_pcie_map_bus(struct pci_bus *bus,
 				      unsigned int devfn, int where)
 {
-- 
2.6.4

[PATCH v6 5/9] PCI: mediatek: Move the mtk_pcie_startup_port_v2 function's define after mtk_pcie_setup_irq

[honghui.zhang at mediatek.com]

From: Honghui Zhang <honghui.zhang@mediatek.com>

This is a prepare patch to fix enable MSI logic, move the function's
define later to avoid forward declaration of mtk_pcie_enable_msi in
the future. No functional changed.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 138 ++++++++++++++++-----------------
 1 file changed, 69 insertions(+), 69 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index ead6005..654a63e 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -392,75 +392,6 @@ static struct pci_ops mtk_pcie_ops_v2 = {
 	.write = mtk_pcie_config_write,
 };
 
-static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
-{
-	struct mtk_pcie *pcie = port->pcie;
-	struct resource *mem = &pcie->mem;
-	const struct mtk_pcie_soc *soc = port->pcie->soc;
-	u32 val;
-	size_t size;
-	int err;
-
-	/* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
-	if (pcie->base) {
-		val = readl(pcie->base + PCIE_SYS_CFG_V2);
-		val |= PCIE_CSR_LTSSM_EN(port->slot) |
-		       PCIE_CSR_ASPM_L1_EN(port->slot);
-		writel(val, pcie->base + PCIE_SYS_CFG_V2);
-	}
-
-	/* Assert all reset signals */
-	writel(0, port->base + PCIE_RST_CTRL);
-
-	/*
-	 * Enable PCIe link down reset, if link status changed from link up to
-	 * link down, this will reset MAC control registers and configuration
-	 * space.
-	 */
-	writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
-
-	/* De-assert PHY, PE, PIPE, MAC and configuration reset	*/
-	val = readl(port->base + PCIE_RST_CTRL);
-	val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
-	       PCIE_MAC_SRSTB | PCIE_CRSTB;
-	writel(val, port->base + PCIE_RST_CTRL);
-
-	/* Set up vendor ID and class code */
-	if (soc->need_fix_class_id) {
-		val = PCI_VENDOR_ID_MEDIATEK;
-		writew(val, port->base + PCIE_CONF_VEND_ID);
-
-		val = PCI_CLASS_BRIDGE_PCI;
-		writew(val, port->base + PCIE_CONF_CLASS_ID);
-	}
-
-	/* 100ms timeout value should be enough for Gen1/2 training */
-	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val,
-				 !!(val & PCIE_PORT_LINKUP_V2), 20,
-				 100 * USEC_PER_MSEC);
-	if (err)
-		return -ETIMEDOUT;
-
-	/* Set INTx mask */
-	val = readl(port->base + PCIE_INT_MASK);
-	val &= ~INTX_MASK;
-	writel(val, port->base + PCIE_INT_MASK);
-
-	/* Set AHB to PCIe translation windows */
-	size = mem->end - mem->start;
-	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
-	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
-
-	val = upper_32_bits(mem->start);
-	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
-
-	/* Set PCIe to AXI translation memory space.*/
-	val = fls(0xffffffff) | WIN_ENABLE;
-	writel(val, port->base + PCIE_AXI_WINDOW0);
-
-	return 0;
-}
-
 static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 {
 	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
@@ -705,6 +636,75 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 	return 0;
 }
 
+static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
+{
+	struct mtk_pcie *pcie = port->pcie;
+	struct resource *mem = &pcie->mem;
+	const struct mtk_pcie_soc *soc = port->pcie->soc;
+	u32 val;
+	size_t size;
+	int err;
+
+	/* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
+	if (pcie->base) {
+		val = readl(pcie->base + PCIE_SYS_CFG_V2);
+		val |= PCIE_CSR_LTSSM_EN(port->slot) |
+		       PCIE_CSR_ASPM_L1_EN(port->slot);
+		writel(val, pcie->base + PCIE_SYS_CFG_V2);
+	}
+
+	/* Assert all reset signals */
+	writel(0, port->base + PCIE_RST_CTRL);
+
+	/*
+	 * Enable PCIe link down reset, if link status changed from link up to
+	 * link down, this will reset MAC control registers and configuration
+	 * space.
+	 */
+	writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
+
+	/* De-assert PHY, PE, PIPE, MAC and configuration reset	*/
+	val = readl(port->base + PCIE_RST_CTRL);
+	val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
+	       PCIE_MAC_SRSTB | PCIE_CRSTB;
+	writel(val, port->base + PCIE_RST_CTRL);
+
+	/* Set up vendor ID and class code */
+	if (soc->need_fix_class_id) {
+		val = PCI_VENDOR_ID_MEDIATEK;
+		writew(val, port->base + PCIE_CONF_VEND_ID);
+
+		val = PCI_CLASS_BRIDGE_PCI;
+		writew(val, port->base + PCIE_CONF_CLASS_ID);
+	}
+
+	/* 100ms timeout value should be enough for Gen1/2 training */
+	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val,
+				 !!(val & PCIE_PORT_LINKUP_V2), 20,
+				 100 * USEC_PER_MSEC);
+	if (err)
+		return -ETIMEDOUT;
+
+	/* Set INTx mask */
+	val = readl(port->base + PCIE_INT_MASK);
+	val &= ~INTX_MASK;
+	writel(val, port->base + PCIE_INT_MASK);
+
+	/* Set AHB to PCIe translation windows */
+	size = mem->end - mem->start;
+	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
+	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
+
+	val = upper_32_bits(mem->start);
+	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
+
+	/* Set PCIe to AXI translation memory space.*/
+	val = fls(0xffffffff) | WIN_ENABLE;
+	writel(val, port->base + PCIE_AXI_WINDOW0);
+
+	return 0;
+}
+
 static void __iomem *mtk_pcie_map_bus(struct pci_bus *bus,
 				      unsigned int devfn, int where)
 {
-- 
2.6.4

[PATCH v6 6/9] PCI: mediatek: Fixup enable msi logic by enable msi after clock enabled

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The commit 43e6409db64d ("PCI: mediatek: Add MSI support for MT2712 and
MT7622") added MSI support but enable MSI in wrong place, clocks was not
enabled when enable MSI. This patch fix this issue by calling
mtk_pcie_enable_msi in mtk_pcie_startup_port_v2 since the clock was all
enabled at that time.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 654a63e..d3f4694 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -568,8 +568,6 @@ static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
 		ret = mtk_pcie_allocate_msi_domains(port);
 		if (ret)
 			return ret;
-
-		mtk_pcie_enable_msi(port);
 	}
 
 	return 0;
@@ -690,6 +688,9 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
 	val &= ~INTX_MASK;
 	writel(val, port->base + PCIE_INT_MASK);
 
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		mtk_pcie_enable_msi(port);
+
 	/* Set AHB to PCIe translation windows */
 	size = mem->end - mem->start;
 	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
-- 
2.6.4

[PATCH v6 6/9] PCI: mediatek: Fixup enable msi logic by enable msi after clock enabled

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The commit 43e6409db64d ("PCI: mediatek: Add MSI support for MT2712 and
MT7622") added MSI support but enable MSI in wrong place, clocks was not
enabled when enable MSI. This patch fix this issue by calling
mtk_pcie_enable_msi in mtk_pcie_startup_port_v2 since the clock was all
enabled at that time.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 654a63e..d3f4694 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -568,8 +568,6 @@ static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
 		ret = mtk_pcie_allocate_msi_domains(port);
 		if (ret)
 			return ret;
-
-		mtk_pcie_enable_msi(port);
 	}
 
 	return 0;
@@ -690,6 +688,9 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
 	val &= ~INTX_MASK;
 	writel(val, port->base + PCIE_INT_MASK);
 
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		mtk_pcie_enable_msi(port);
+
 	/* Set AHB to PCIe translation windows */
 	size = mem->end - mem->start;
 	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
-- 
2.6.4

[PATCH v6 6/9] PCI: mediatek: Fixup enable msi logic by enable msi after clock enabled

[honghui.zhang at mediatek.com]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The commit 43e6409db64d ("PCI: mediatek: Add MSI support for MT2712 and
MT7622") added MSI support but enable MSI in wrong place, clocks was not
enabled when enable MSI. This patch fix this issue by calling
mtk_pcie_enable_msi in mtk_pcie_startup_port_v2 since the clock was all
enabled at that time.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 654a63e..d3f4694 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -568,8 +568,6 @@ static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
 		ret = mtk_pcie_allocate_msi_domains(port);
 		if (ret)
 			return ret;
-
-		mtk_pcie_enable_msi(port);
 	}
 
 	return 0;
@@ -690,6 +688,9 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
 	val &= ~INTX_MASK;
 	writel(val, port->base + PCIE_INT_MASK);
 
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		mtk_pcie_enable_msi(port);
+
 	/* Set AHB to PCIe translation windows */
 	size = mem->end - mem->start;
 	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
-- 
2.6.4

[PATCH v6 7/9] PCI: mediatek: Add system pm support for MT2712 and MT7622

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

In order to reduce the PCIe power consuming while system suspend,
the physical layer should be gated. And the PCIe link should be
re-established and the related control register values should be
re-initialized after system resume.

Register suspend_noirq & resume_noirq callback functions to allow
PCIe to come up after resume from RAM.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 50 ++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index d3f4694..42cf2a4 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -1168,6 +1168,55 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
+{
+	struct mtk_pcie *pcie = dev_get_drvdata(dev);
+	struct mtk_pcie_port *port;
+
+	if (list_empty(&pcie->ports))
+		return 0;
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		clk_disable_unprepare(port->pipe_ck);
+		clk_disable_unprepare(port->obff_ck);
+		clk_disable_unprepare(port->axi_ck);
+		clk_disable_unprepare(port->aux_ck);
+		clk_disable_unprepare(port->ahb_ck);
+		clk_disable_unprepare(port->sys_ck);
+		phy_power_off(port->phy);
+		phy_exit(port->phy);
+	}
+
+	clk_disable_unprepare(pcie->free_ck);
+
+	return 0;
+}
+
+static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
+{
+	struct mtk_pcie *pcie = dev_get_drvdata(dev);
+	struct mtk_pcie_port *port, *tmp;
+
+	if (list_empty(&pcie->ports))
+		return 0;
+
+	clk_prepare_enable(pcie->free_ck);
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+		mtk_pcie_enable_port(port);
+
+	/* In case of EP was removed while system suspend. */
+	if (list_empty(&pcie->ports))
+		clk_disable_unprepare(pcie->free_ck);
+
+	return 0;
+}
+
+static const struct dev_pm_ops mtk_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
+				      mtk_pcie_resume_noirq)
+};
+
 static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
 	.ops = &mtk_pcie_ops,
 	.startup = mtk_pcie_startup_port,
@@ -1200,6 +1249,7 @@ static struct platform_driver mtk_pcie_driver = {
 		.name = "mtk-pcie",
 		.of_match_table = mtk_pcie_ids,
 		.suppress_bind_attrs = true,
+		.pm = &mtk_pcie_pm_ops,
 	},
 };
 builtin_platform_driver(mtk_pcie_driver);
-- 
2.6.4

[PATCH v6 7/9] PCI: mediatek: Add system pm support for MT2712 and MT7622

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

In order to reduce the PCIe power consuming while system suspend,
the physical layer should be gated. And the PCIe link should be
re-established and the related control register values should be
re-initialized after system resume.

Register suspend_noirq & resume_noirq callback functions to allow
PCIe to come up after resume from RAM.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 50 ++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index d3f4694..42cf2a4 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -1168,6 +1168,55 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
+{
+	struct mtk_pcie *pcie = dev_get_drvdata(dev);
+	struct mtk_pcie_port *port;
+
+	if (list_empty(&pcie->ports))
+		return 0;
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		clk_disable_unprepare(port->pipe_ck);
+		clk_disable_unprepare(port->obff_ck);
+		clk_disable_unprepare(port->axi_ck);
+		clk_disable_unprepare(port->aux_ck);
+		clk_disable_unprepare(port->ahb_ck);
+		clk_disable_unprepare(port->sys_ck);
+		phy_power_off(port->phy);
+		phy_exit(port->phy);
+	}
+
+	clk_disable_unprepare(pcie->free_ck);
+
+	return 0;
+}
+
+static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
+{
+	struct mtk_pcie *pcie = dev_get_drvdata(dev);
+	struct mtk_pcie_port *port, *tmp;
+
+	if (list_empty(&pcie->ports))
+		return 0;
+
+	clk_prepare_enable(pcie->free_ck);
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+		mtk_pcie_enable_port(port);
+
+	/* In case of EP was removed while system suspend. */
+	if (list_empty(&pcie->ports))
+		clk_disable_unprepare(pcie->free_ck);
+
+	return 0;
+}
+
+static const struct dev_pm_ops mtk_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
+				      mtk_pcie_resume_noirq)
+};
+
 static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
 	.ops = &mtk_pcie_ops,
 	.startup = mtk_pcie_startup_port,
@@ -1200,6 +1249,7 @@ static struct platform_driver mtk_pcie_driver = {
 		.name = "mtk-pcie",
 		.of_match_table = mtk_pcie_ids,
 		.suppress_bind_attrs = true,
+		.pm = &mtk_pcie_pm_ops,
 	},
 };
 builtin_platform_driver(mtk_pcie_driver);
-- 
2.6.4

[PATCH v6 7/9] PCI: mediatek: Add system pm support for MT2712 and MT7622

[honghui.zhang at mediatek.com]

From: Honghui Zhang <honghui.zhang@mediatek.com>

In order to reduce the PCIe power consuming while system suspend,
the physical layer should be gated. And the PCIe link should be
re-established and the related control register values should be
re-initialized after system resume.

Register suspend_noirq & resume_noirq callback functions to allow
PCIe to come up after resume from RAM.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 50 ++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index d3f4694..42cf2a4 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -1168,6 +1168,55 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
+{
+	struct mtk_pcie *pcie = dev_get_drvdata(dev);
+	struct mtk_pcie_port *port;
+
+	if (list_empty(&pcie->ports))
+		return 0;
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		clk_disable_unprepare(port->pipe_ck);
+		clk_disable_unprepare(port->obff_ck);
+		clk_disable_unprepare(port->axi_ck);
+		clk_disable_unprepare(port->aux_ck);
+		clk_disable_unprepare(port->ahb_ck);
+		clk_disable_unprepare(port->sys_ck);
+		phy_power_off(port->phy);
+		phy_exit(port->phy);
+	}
+
+	clk_disable_unprepare(pcie->free_ck);
+
+	return 0;
+}
+
+static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
+{
+	struct mtk_pcie *pcie = dev_get_drvdata(dev);
+	struct mtk_pcie_port *port, *tmp;
+
+	if (list_empty(&pcie->ports))
+		return 0;
+
+	clk_prepare_enable(pcie->free_ck);
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+		mtk_pcie_enable_port(port);
+
+	/* In case of EP was removed while system suspend. */
+	if (list_empty(&pcie->ports))
+		clk_disable_unprepare(pcie->free_ck);
+
+	return 0;
+}
+
+static const struct dev_pm_ops mtk_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
+				      mtk_pcie_resume_noirq)
+};
+
 static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
 	.ops = &mtk_pcie_ops,
 	.startup = mtk_pcie_startup_port,
@@ -1200,6 +1249,7 @@ static struct platform_driver mtk_pcie_driver = {
 		.name = "mtk-pcie",
 		.of_match_table = mtk_pcie_ids,
 		.suppress_bind_attrs = true,
+		.pm = &mtk_pcie_pm_ops,
 	},
 };
 builtin_platform_driver(mtk_pcie_driver);
-- 
2.6.4

[PATCH v6 8/9] PCI: mediatek: Save the GIC IRQ in mtk_pcie_port

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

Need to save the PCIe's GIC IRQ for dispose_irq, this is a prepare
patch for add mediatek PCIe module support to tear down the IRQ, no
functional changed.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 42cf2a4..daba78f 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -162,6 +162,7 @@ struct mtk_pcie_soc {
  * @phy: pointer to PHY control block
  * @lane: lane count
  * @slot: port slot
+ * @irq: GIC irq
  * @irq_domain: legacy INTx IRQ domain
  * @inner_domain: inner IRQ domain
  * @msi_domain: MSI IRQ domain
@@ -182,6 +183,7 @@ struct mtk_pcie_port {
 	struct phy *phy;
 	u32 lane;
 	u32 slot;
+	int irq;
 	struct irq_domain *irq_domain;
 	struct irq_domain *inner_domain;
 	struct irq_domain *msi_domain;
@@ -620,7 +622,7 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 	struct mtk_pcie *pcie = port->pcie;
 	struct device *dev = pcie->dev;
 	struct platform_device *pdev = to_platform_device(dev);
-	int err, irq;
+	int err;
 
 	err = mtk_pcie_init_irq_domain(port, node);
 	if (err) {
@@ -628,8 +630,9 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 		return err;
 	}
 
-	irq = platform_get_irq(pdev, port->slot);
-	irq_set_chained_handler_and_data(irq, mtk_pcie_intr_handler, port);
+	port->irq = platform_get_irq(pdev, port->slot);
+	irq_set_chained_handler_and_data(port->irq,
+					 mtk_pcie_intr_handler, port);
 
 	return 0;
 }
-- 
2.6.4

[PATCH v6 8/9] PCI: mediatek: Save the GIC IRQ in mtk_pcie_port

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

Need to save the PCIe's GIC IRQ for dispose_irq, this is a prepare
patch for add mediatek PCIe module support to tear down the IRQ, no
functional changed.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 42cf2a4..daba78f 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -162,6 +162,7 @@ struct mtk_pcie_soc {
  * @phy: pointer to PHY control block
  * @lane: lane count
  * @slot: port slot
+ * @irq: GIC irq
  * @irq_domain: legacy INTx IRQ domain
  * @inner_domain: inner IRQ domain
  * @msi_domain: MSI IRQ domain
@@ -182,6 +183,7 @@ struct mtk_pcie_port {
 	struct phy *phy;
 	u32 lane;
 	u32 slot;
+	int irq;
 	struct irq_domain *irq_domain;
 	struct irq_domain *inner_domain;
 	struct irq_domain *msi_domain;
@@ -620,7 +622,7 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 	struct mtk_pcie *pcie = port->pcie;
 	struct device *dev = pcie->dev;
 	struct platform_device *pdev = to_platform_device(dev);
-	int err, irq;
+	int err;
 
 	err = mtk_pcie_init_irq_domain(port, node);
 	if (err) {
@@ -628,8 +630,9 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 		return err;
 	}
 
-	irq = platform_get_irq(pdev, port->slot);
-	irq_set_chained_handler_and_data(irq, mtk_pcie_intr_handler, port);
+	port->irq = platform_get_irq(pdev, port->slot);
+	irq_set_chained_handler_and_data(port->irq,
+					 mtk_pcie_intr_handler, port);
 
 	return 0;
 }
-- 
2.6.4

[PATCH v6 8/9] PCI: mediatek: Save the GIC IRQ in mtk_pcie_port

[honghui.zhang at mediatek.com]

From: Honghui Zhang <honghui.zhang@mediatek.com>

Need to save the PCIe's GIC IRQ for dispose_irq, this is a prepare
patch for add mediatek PCIe module support to tear down the IRQ, no
functional changed.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 42cf2a4..daba78f 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -162,6 +162,7 @@ struct mtk_pcie_soc {
  * @phy: pointer to PHY control block
  * @lane: lane count
  * @slot: port slot
+ * @irq: GIC irq
  * @irq_domain: legacy INTx IRQ domain
  * @inner_domain: inner IRQ domain
  * @msi_domain: MSI IRQ domain
@@ -182,6 +183,7 @@ struct mtk_pcie_port {
 	struct phy *phy;
 	u32 lane;
 	u32 slot;
+	int irq;
 	struct irq_domain *irq_domain;
 	struct irq_domain *inner_domain;
 	struct irq_domain *msi_domain;
@@ -620,7 +622,7 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 	struct mtk_pcie *pcie = port->pcie;
 	struct device *dev = pcie->dev;
 	struct platform_device *pdev = to_platform_device(dev);
-	int err, irq;
+	int err;
 
 	err = mtk_pcie_init_irq_domain(port, node);
 	if (err) {
@@ -628,8 +630,9 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 		return err;
 	}
 
-	irq = platform_get_irq(pdev, port->slot);
-	irq_set_chained_handler_and_data(irq, mtk_pcie_intr_handler, port);
+	port->irq = platform_get_irq(pdev, port->slot);
+	irq_set_chained_handler_and_data(port->irq,
+					 mtk_pcie_intr_handler, port);
 
 	return 0;
 }
-- 
2.6.4

[PATCH v6 9/9] PCI: mediatek: Add loadable kernel module support

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

Implement remove callback function for Mediatek PCIe driver to add
loadable kernel module support.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Reviewed-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/Kconfig         |  2 +-
 drivers/pci/controller/pcie-mediatek.c | 52 +++++++++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 028b287..465790f 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -231,7 +231,7 @@ config PCIE_ROCKCHIP_EP
 	  available to support GEN2 with 4 slots.
 
 config PCIE_MEDIATEK
-	bool "MediaTek PCIe controller"
+	tristate "MediaTek PCIe controller"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
 	depends on OF
 	depends on PCI_MSI_IRQ_DOMAIN
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index daba78f..3327c75 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -15,6 +15,7 @@
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/msi.h>
+#include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
@@ -532,6 +533,27 @@ static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
 	writel(val, port->base + PCIE_INT_MASK);
 }
 
+static void mtk_pcie_irq_teardown(struct mtk_pcie *pcie)
+{
+	struct mtk_pcie_port *port, *tmp;
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		irq_set_chained_handler_and_data(port->irq, NULL, NULL);
+
+		if (port->irq_domain)
+			irq_domain_remove(port->irq_domain);
+
+		if (IS_ENABLED(CONFIG_PCI_MSI)) {
+			if (port->msi_domain)
+				irq_domain_remove(port->msi_domain);
+			if (port->inner_domain)
+				irq_domain_remove(port->inner_domain);
+		}
+
+		irq_dispose_mapping(port->irq);
+	}
+}
+
 static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
 			     irq_hw_number_t hwirq)
 {
@@ -1171,6 +1193,32 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+
+static void mtk_pcie_free_resources(struct mtk_pcie *pcie)
+{
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+	struct list_head *windows = &host->windows;
+
+	pci_unmap_iospace(&pcie->pio);
+	pci_free_resource_list(windows);
+}
+
+static int mtk_pcie_remove(struct platform_device *pdev)
+{
+	struct mtk_pcie *pcie = platform_get_drvdata(pdev);
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+
+	pci_stop_root_bus(host->bus);
+	pci_remove_root_bus(host->bus);
+	mtk_pcie_free_resources(pcie);
+
+	mtk_pcie_irq_teardown(pcie);
+
+	mtk_pcie_put_resources(pcie);
+
+	return 0;
+}
+
 static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
 {
 	struct mtk_pcie *pcie = dev_get_drvdata(dev);
@@ -1248,6 +1296,7 @@ static const struct of_device_id mtk_pcie_ids[] = {
 
 static struct platform_driver mtk_pcie_driver = {
 	.probe = mtk_pcie_probe,
+	.remove = mtk_pcie_remove,
 	.driver = {
 		.name = "mtk-pcie",
 		.of_match_table = mtk_pcie_ids,
@@ -1255,4 +1304,5 @@ static struct platform_driver mtk_pcie_driver = {
 		.pm = &mtk_pcie_pm_ops,
 	},
 };
-builtin_platform_driver(mtk_pcie_driver);
+module_platform_driver(mtk_pcie_driver);
+MODULE_LICENSE("GPL v2");
-- 
2.6.4

[PATCH v6 9/9] PCI: mediatek: Add loadable kernel module support

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

Implement remove callback function for Mediatek PCIe driver to add
loadable kernel module support.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Reviewed-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/Kconfig         |  2 +-
 drivers/pci/controller/pcie-mediatek.c | 52 +++++++++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 028b287..465790f 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -231,7 +231,7 @@ config PCIE_ROCKCHIP_EP
 	  available to support GEN2 with 4 slots.
 
 config PCIE_MEDIATEK
-	bool "MediaTek PCIe controller"
+	tristate "MediaTek PCIe controller"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
 	depends on OF
 	depends on PCI_MSI_IRQ_DOMAIN
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index daba78f..3327c75 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -15,6 +15,7 @@
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/msi.h>
+#include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
@@ -532,6 +533,27 @@ static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
 	writel(val, port->base + PCIE_INT_MASK);
 }
 
+static void mtk_pcie_irq_teardown(struct mtk_pcie *pcie)
+{
+	struct mtk_pcie_port *port, *tmp;
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		irq_set_chained_handler_and_data(port->irq, NULL, NULL);
+
+		if (port->irq_domain)
+			irq_domain_remove(port->irq_domain);
+
+		if (IS_ENABLED(CONFIG_PCI_MSI)) {
+			if (port->msi_domain)
+				irq_domain_remove(port->msi_domain);
+			if (port->inner_domain)
+				irq_domain_remove(port->inner_domain);
+		}
+
+		irq_dispose_mapping(port->irq);
+	}
+}
+
 static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
 			     irq_hw_number_t hwirq)
 {
@@ -1171,6 +1193,32 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+
+static void mtk_pcie_free_resources(struct mtk_pcie *pcie)
+{
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+	struct list_head *windows = &host->windows;
+
+	pci_unmap_iospace(&pcie->pio);
+	pci_free_resource_list(windows);
+}
+
+static int mtk_pcie_remove(struct platform_device *pdev)
+{
+	struct mtk_pcie *pcie = platform_get_drvdata(pdev);
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+
+	pci_stop_root_bus(host->bus);
+	pci_remove_root_bus(host->bus);
+	mtk_pcie_free_resources(pcie);
+
+	mtk_pcie_irq_teardown(pcie);
+
+	mtk_pcie_put_resources(pcie);
+
+	return 0;
+}
+
 static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
 {
 	struct mtk_pcie *pcie = dev_get_drvdata(dev);
@@ -1248,6 +1296,7 @@ static const struct of_device_id mtk_pcie_ids[] = {
 
 static struct platform_driver mtk_pcie_driver = {
 	.probe = mtk_pcie_probe,
+	.remove = mtk_pcie_remove,
 	.driver = {
 		.name = "mtk-pcie",
 		.of_match_table = mtk_pcie_ids,
@@ -1255,4 +1304,5 @@ static struct platform_driver mtk_pcie_driver = {
 		.pm = &mtk_pcie_pm_ops,
 	},
 };
-builtin_platform_driver(mtk_pcie_driver);
+module_platform_driver(mtk_pcie_driver);
+MODULE_LICENSE("GPL v2");
-- 
2.6.4

[PATCH v6 9/9] PCI: mediatek: Add loadable kernel module support

[honghui.zhang at mediatek.com]

From: Honghui Zhang <honghui.zhang@mediatek.com>

Implement remove callback function for Mediatek PCIe driver to add
loadable kernel module support.

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
Reviewed-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/Kconfig         |  2 +-
 drivers/pci/controller/pcie-mediatek.c | 52 +++++++++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 028b287..465790f 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -231,7 +231,7 @@ config PCIE_ROCKCHIP_EP
 	  available to support GEN2 with 4 slots.
 
 config PCIE_MEDIATEK
-	bool "MediaTek PCIe controller"
+	tristate "MediaTek PCIe controller"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
 	depends on OF
 	depends on PCI_MSI_IRQ_DOMAIN
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index daba78f..3327c75 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -15,6 +15,7 @@
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/msi.h>
+#include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
@@ -532,6 +533,27 @@ static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
 	writel(val, port->base + PCIE_INT_MASK);
 }
 
+static void mtk_pcie_irq_teardown(struct mtk_pcie *pcie)
+{
+	struct mtk_pcie_port *port, *tmp;
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		irq_set_chained_handler_and_data(port->irq, NULL, NULL);
+
+		if (port->irq_domain)
+			irq_domain_remove(port->irq_domain);
+
+		if (IS_ENABLED(CONFIG_PCI_MSI)) {
+			if (port->msi_domain)
+				irq_domain_remove(port->msi_domain);
+			if (port->inner_domain)
+				irq_domain_remove(port->inner_domain);
+		}
+
+		irq_dispose_mapping(port->irq);
+	}
+}
+
 static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
 			     irq_hw_number_t hwirq)
 {
@@ -1171,6 +1193,32 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+
+static void mtk_pcie_free_resources(struct mtk_pcie *pcie)
+{
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+	struct list_head *windows = &host->windows;
+
+	pci_unmap_iospace(&pcie->pio);
+	pci_free_resource_list(windows);
+}
+
+static int mtk_pcie_remove(struct platform_device *pdev)
+{
+	struct mtk_pcie *pcie = platform_get_drvdata(pdev);
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+
+	pci_stop_root_bus(host->bus);
+	pci_remove_root_bus(host->bus);
+	mtk_pcie_free_resources(pcie);
+
+	mtk_pcie_irq_teardown(pcie);
+
+	mtk_pcie_put_resources(pcie);
+
+	return 0;
+}
+
 static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
 {
 	struct mtk_pcie *pcie = dev_get_drvdata(dev);
@@ -1248,6 +1296,7 @@ static const struct of_device_id mtk_pcie_ids[] = {
 
 static struct platform_driver mtk_pcie_driver = {
 	.probe = mtk_pcie_probe,
+	.remove = mtk_pcie_remove,
 	.driver = {
 		.name = "mtk-pcie",
 		.of_match_table = mtk_pcie_ids,
@@ -1255,4 +1304,5 @@ static struct platform_driver mtk_pcie_driver = {
 		.pm = &mtk_pcie_pm_ops,
 	},
 };
-builtin_platform_driver(mtk_pcie_driver);
+module_platform_driver(mtk_pcie_driver);
+MODULE_LICENSE("GPL v2");
-- 
2.6.4

Re: [PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Lorenzo Pieralisi]

On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang@mediatek.com wrote:
> From: Honghui Zhang <honghui.zhang@mediatek.com>
> 
> The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> the HW default class type values is invalid.
> 
> The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> type for MT7622") have set the class ID for MT7622 as
> PCI_CLASS_BRIDGE_HOST, but it's not workable for MT7622:
> 
> In __pci_bus_assign_resources, the framework only setup bridge's
> resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> will leave the subordinary PCIe device's MMIO window un-touched.
> 
> Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> driver do.

I think that this patch is correct but the commit log fails to pin point
the problem. The IP you are programming is a root port, that's why you
have to have the proper class code, the patch looks fine but I would
like to peek Bjorn's brain on this since it is a fundamental concept.

If the kernel does not assign resources unless it detects a
PCI_CLASS_BRIDGE_PCI this means that for components that are
actually PCI_CLASS_BRIDGE_HOST their register set must come
preprogrammed unless I am missing something.

I would like to get to the bottom of this since it is a fundamental
enumeration concept.

Thanks,
Lorenzo

> 
> Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 288b8e2..bcdac9b 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -432,7 +432,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
>  		val = PCI_VENDOR_ID_MEDIATEK;
>  		writew(val, port->base + PCIE_CONF_VEND_ID);
>  
> -		val = PCI_CLASS_BRIDGE_HOST;
> +		val = PCI_CLASS_BRIDGE_PCI;
>  		writew(val, port->base + PCIE_CONF_CLASS_ID);
>  	}
>  
> -- 
> 2.6.4
> 

[PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Lorenzo Pieralisi]

On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang at mediatek.com wrote:
> From: Honghui Zhang <honghui.zhang@mediatek.com>
> 
> The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> the HW default class type values is invalid.
> 
> The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> type for MT7622") have set the class ID for MT7622 as
> PCI_CLASS_BRIDGE_HOST, but it's not workable for MT7622:
> 
> In __pci_bus_assign_resources, the framework only setup bridge's
> resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> will leave the subordinary PCIe device's MMIO window un-touched.
> 
> Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> driver do.

I think that this patch is correct but the commit log fails to pin point
the problem. The IP you are programming is a root port, that's why you
have to have the proper class code, the patch looks fine but I would
like to peek Bjorn's brain on this since it is a fundamental concept.

If the kernel does not assign resources unless it detects a
PCI_CLASS_BRIDGE_PCI this means that for components that are
actually PCI_CLASS_BRIDGE_HOST their register set must come
preprogrammed unless I am missing something.

I would like to get to the bottom of this since it is a fundamental
enumeration concept.

Thanks,
Lorenzo

> 
> Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 288b8e2..bcdac9b 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -432,7 +432,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
>  		val = PCI_VENDOR_ID_MEDIATEK;
>  		writew(val, port->base + PCIE_CONF_VEND_ID);
>  
> -		val = PCI_CLASS_BRIDGE_HOST;
> +		val = PCI_CLASS_BRIDGE_PCI;
>  		writew(val, port->base + PCIE_CONF_CLASS_ID);
>  	}
>  
> -- 
> 2.6.4
> 

Re: [PATCH v6 0/9] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[Bjorn Helgaas]

On Mon, Oct 08, 2018 at 11:24:39AM +0800, honghui.zhang@mediatek.com wrote:

> Honghui Zhang (9):
>   PCI: mediatek: Using slot's devfn for compare to fix
>     mtk_pcie_find_port logic
>   PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI
>   PCI: mediatek: Remove the redundant dev->pm_domain check
>   PCI: mediatek: Convert to use pci_host_probe()
>   PCI: mediatek: Move the mtk_pcie_startup_port_v2 function's define
>     after mtk_pcie_setup_irq
>   PCI: mediatek: Fixup enable msi logic by enable msi after clock
>     enabled

s/msi/MSI/ (twice)

>   PCI: mediatek: Add system pm support for MT2712 and MT7622

s/pm/PM/

"msi" and "pm" are not English words, and capitalizing them tells the
reader that they are acronyms or initialisms (like GIC and IRQ below).

>   PCI: mediatek: Save the GIC IRQ in mtk_pcie_port
>   PCI: mediatek: Add loadable kernel module support

[PATCH v6 0/9] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[Bjorn Helgaas]

On Mon, Oct 08, 2018 at 11:24:39AM +0800, honghui.zhang at mediatek.com wrote:

> Honghui Zhang (9):
>   PCI: mediatek: Using slot's devfn for compare to fix
>     mtk_pcie_find_port logic
>   PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI
>   PCI: mediatek: Remove the redundant dev->pm_domain check
>   PCI: mediatek: Convert to use pci_host_probe()
>   PCI: mediatek: Move the mtk_pcie_startup_port_v2 function's define
>     after mtk_pcie_setup_irq
>   PCI: mediatek: Fixup enable msi logic by enable msi after clock
>     enabled

s/msi/MSI/ (twice)

>   PCI: mediatek: Add system pm support for MT2712 and MT7622

s/pm/PM/

"msi" and "pm" are not English words, and capitalizing them tells the
reader that they are acronyms or initialisms (like GIC and IRQ below).

>   PCI: mediatek: Save the GIC IRQ in mtk_pcie_port
>   PCI: mediatek: Add loadable kernel module support

Re: [PATCH v6 0/9] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[Honghui Zhang]

On Mon, 2018-10-08 at 12:31 -0500, Bjorn Helgaas wrote:
> On Mon, Oct 08, 2018 at 11:24:39AM +0800, honghui.zhang@mediatek.com wrote:
> 
> > Honghui Zhang (9):
> >   PCI: mediatek: Using slot's devfn for compare to fix
> >     mtk_pcie_find_port logic
> >   PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI
> >   PCI: mediatek: Remove the redundant dev->pm_domain check
> >   PCI: mediatek: Convert to use pci_host_probe()
> >   PCI: mediatek: Move the mtk_pcie_startup_port_v2 function's define
> >     after mtk_pcie_setup_irq
> >   PCI: mediatek: Fixup enable msi logic by enable msi after clock
> >     enabled
> 
> s/msi/MSI/ (twice)
> 
> >   PCI: mediatek: Add system pm support for MT2712 and MT7622
> 
> s/pm/PM/
> 
> "msi" and "pm" are not English words, and capitalizing them tells the
> reader that they are acronyms or initialisms (like GIC and IRQ below).
> 

Thanks for the comments. Will fix those in the next version.

thanks.

> >   PCI: mediatek: Save the GIC IRQ in mtk_pcie_port
> >   PCI: mediatek: Add loadable kernel module support

Re: [PATCH v6 0/9] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[Honghui Zhang]

On Mon, 2018-10-08 at 12:31 -0500, Bjorn Helgaas wrote:
> On Mon, Oct 08, 2018 at 11:24:39AM +0800, honghui.zhang@mediatek.com wrote:
> 
> > Honghui Zhang (9):
> >   PCI: mediatek: Using slot's devfn for compare to fix
> >     mtk_pcie_find_port logic
> >   PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI
> >   PCI: mediatek: Remove the redundant dev->pm_domain check
> >   PCI: mediatek: Convert to use pci_host_probe()
> >   PCI: mediatek: Move the mtk_pcie_startup_port_v2 function's define
> >     after mtk_pcie_setup_irq
> >   PCI: mediatek: Fixup enable msi logic by enable msi after clock
> >     enabled
> 
> s/msi/MSI/ (twice)
> 
> >   PCI: mediatek: Add system pm support for MT2712 and MT7622
> 
> s/pm/PM/
> 
> "msi" and "pm" are not English words, and capitalizing them tells the
> reader that they are acronyms or initialisms (like GIC and IRQ below).
> 

Thanks for the comments. Will fix those in the next version.

thanks.

> >   PCI: mediatek: Save the GIC IRQ in mtk_pcie_port
> >   PCI: mediatek: Add loadable kernel module support

[PATCH v6 0/9] PCI: mediatek: fixup find_port, enable_msi and add pm, module support

[Honghui Zhang]

On Mon, 2018-10-08 at 12:31 -0500, Bjorn Helgaas wrote:
> On Mon, Oct 08, 2018 at 11:24:39AM +0800, honghui.zhang at mediatek.com wrote:
> 
> > Honghui Zhang (9):
> >   PCI: mediatek: Using slot's devfn for compare to fix
> >     mtk_pcie_find_port logic
> >   PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI
> >   PCI: mediatek: Remove the redundant dev->pm_domain check
> >   PCI: mediatek: Convert to use pci_host_probe()
> >   PCI: mediatek: Move the mtk_pcie_startup_port_v2 function's define
> >     after mtk_pcie_setup_irq
> >   PCI: mediatek: Fixup enable msi logic by enable msi after clock
> >     enabled
> 
> s/msi/MSI/ (twice)
> 
> >   PCI: mediatek: Add system pm support for MT2712 and MT7622
> 
> s/pm/PM/
> 
> "msi" and "pm" are not English words, and capitalizing them tells the
> reader that they are acronyms or initialisms (like GIC and IRQ below).
> 

Thanks for the comments. Will fix those in the next version.

thanks.

> >   PCI: mediatek: Save the GIC IRQ in mtk_pcie_port
> >   PCI: mediatek: Add loadable kernel module support

Re: [PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Honghui Zhang]

On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang@mediatek.com wrote:
> > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > 
> > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > the HW default class type values is invalid.
> > 
> > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > type for MT7622") have set the class ID for MT7622 as
> > PCI_CLASS_BRIDGE_HOST, but it's not workable for MT7622:
> > 
> > In __pci_bus_assign_resources, the framework only setup bridge's
> > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > will leave the subordinary PCIe device's MMIO window un-touched.
> > 
> > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > driver do.
> 
> I think that this patch is correct but the commit log fails to pin point
> the problem. The IP you are programming is a root port, that's why you
> have to have the proper class code, the patch looks fine but I would
> like to peek Bjorn's brain on this since it is a fundamental concept.
> 

I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
PCI_CLASS_BRIDGE_PCI, from PCI express spec,
4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
"part of a Root Complex that connects a host CPU or CPUs to a
Hierarchy". While Root Complex defines as "A defined System Element that
includes at least one Host Bridge, Root port, or Root complex Integrated
Endpoint".

But according to my understanding, most of the root port IPs integrated
with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
could be saw as a pci device when using lspci.

And for MT7622, it integrated with block of internal control registers,
type 1 configuration space, and is considered as a root complex.

I'm not sure which CLASS type it should have:
From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
literally suitable for MT7622(which is a root complex)(In my personal
opinion). But it is the only workable CLASS type for MT7622 in current
kernel.

> If the kernel does not assign resources unless it detects a
> PCI_CLASS_BRIDGE_PCI this means that for components that are
> actually PCI_CLASS_BRIDGE_HOST their register set must come
> preprogrammed unless I am missing something.
> 

In the function pci_request_resource_alignment, it will by pass the
resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
out why.

> I would like to get to the bottom of this since it is a fundamental
> enumeration concept.
> 

Do you like me to re-write the commit message for this patch and put the
above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
assign resource routine?

Thanks

> Thanks,
> Lorenzo
> 
> > 
> > Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  drivers/pci/controller/pcie-mediatek.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > index 288b8e2..bcdac9b 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -432,7 +432,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> >  		val = PCI_VENDOR_ID_MEDIATEK;
> >  		writew(val, port->base + PCIE_CONF_VEND_ID);
> >  
> > -		val = PCI_CLASS_BRIDGE_HOST;
> > +		val = PCI_CLASS_BRIDGE_PCI;
> >  		writew(val, port->base + PCIE_CONF_CLASS_ID);
> >  	}
> >  
> > -- 
> > 2.6.4
> > 

Re: [PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Honghui Zhang]

On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang@mediatek.com wrote:
> > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > 
> > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > the HW default class type values is invalid.
> > 
> > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > type for MT7622") have set the class ID for MT7622 as
> > PCI_CLASS_BRIDGE_HOST, but it's not workable for MT7622:
> > 
> > In __pci_bus_assign_resources, the framework only setup bridge's
> > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > will leave the subordinary PCIe device's MMIO window un-touched.
> > 
> > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > driver do.
> 
> I think that this patch is correct but the commit log fails to pin point
> the problem. The IP you are programming is a root port, that's why you
> have to have the proper class code, the patch looks fine but I would
> like to peek Bjorn's brain on this since it is a fundamental concept.
> 

I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
PCI_CLASS_BRIDGE_PCI, from PCI express spec,
4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
"part of a Root Complex that connects a host CPU or CPUs to a
Hierarchy". While Root Complex defines as "A defined System Element that
includes at least one Host Bridge, Root port, or Root complex Integrated
Endpoint".

But according to my understanding, most of the root port IPs integrated
with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
could be saw as a pci device when using lspci.

And for MT7622, it integrated with block of internal control registers,
type 1 configuration space, and is considered as a root complex.

I'm not sure which CLASS type it should have:
>From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
literally suitable for MT7622(which is a root complex)(In my personal
opinion). But it is the only workable CLASS type for MT7622 in current
kernel.

> If the kernel does not assign resources unless it detects a
> PCI_CLASS_BRIDGE_PCI this means that for components that are
> actually PCI_CLASS_BRIDGE_HOST their register set must come
> preprogrammed unless I am missing something.
> 

In the function pci_request_resource_alignment, it will by pass the
resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
out why.

> I would like to get to the bottom of this since it is a fundamental
> enumeration concept.
> 

Do you like me to re-write the commit message for this patch and put the
above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
assign resource routine?

Thanks

> Thanks,
> Lorenzo
> 
> > 
> > Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  drivers/pci/controller/pcie-mediatek.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > index 288b8e2..bcdac9b 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -432,7 +432,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> >  		val = PCI_VENDOR_ID_MEDIATEK;
> >  		writew(val, port->base + PCIE_CONF_VEND_ID);
> >  
> > -		val = PCI_CLASS_BRIDGE_HOST;
> > +		val = PCI_CLASS_BRIDGE_PCI;
> >  		writew(val, port->base + PCIE_CONF_CLASS_ID);
> >  	}
> >  
> > -- 
> > 2.6.4
> > 

[PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Honghui Zhang]

On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang at mediatek.com wrote:
> > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > 
> > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > the HW default class type values is invalid.
> > 
> > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > type for MT7622") have set the class ID for MT7622 as
> > PCI_CLASS_BRIDGE_HOST, but it's not workable for MT7622:
> > 
> > In __pci_bus_assign_resources, the framework only setup bridge's
> > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > will leave the subordinary PCIe device's MMIO window un-touched.
> > 
> > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > driver do.
> 
> I think that this patch is correct but the commit log fails to pin point
> the problem. The IP you are programming is a root port, that's why you
> have to have the proper class code, the patch looks fine but I would
> like to peek Bjorn's brain on this since it is a fundamental concept.
> 

I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
PCI_CLASS_BRIDGE_PCI, from PCI express spec,
4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
"part of a Root Complex that connects a host CPU or CPUs to a
Hierarchy". While Root Complex defines as "A defined System Element that
includes at least one Host Bridge, Root port, or Root complex Integrated
Endpoint".

But according to my understanding, most of the root port IPs integrated
with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
could be saw as a pci device when using lspci.

And for MT7622, it integrated with block of internal control registers,
type 1 configuration space, and is considered as a root complex.

I'm not sure which CLASS type it should have:
>From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
literally suitable for MT7622(which is a root complex)(In my personal
opinion). But it is the only workable CLASS type for MT7622 in current
kernel.

> If the kernel does not assign resources unless it detects a
> PCI_CLASS_BRIDGE_PCI this means that for components that are
> actually PCI_CLASS_BRIDGE_HOST their register set must come
> preprogrammed unless I am missing something.
> 

In the function pci_request_resource_alignment, it will by pass the
resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
out why.

> I would like to get to the bottom of this since it is a fundamental
> enumeration concept.
> 

Do you like me to re-write the commit message for this patch and put the
above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
assign resource routine?

Thanks

> Thanks,
> Lorenzo
> 
> > 
> > Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  drivers/pci/controller/pcie-mediatek.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > index 288b8e2..bcdac9b 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -432,7 +432,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> >  		val = PCI_VENDOR_ID_MEDIATEK;
> >  		writew(val, port->base + PCIE_CONF_VEND_ID);
> >  
> > -		val = PCI_CLASS_BRIDGE_HOST;
> > +		val = PCI_CLASS_BRIDGE_PCI;
> >  		writew(val, port->base + PCIE_CONF_CLASS_ID);
> >  	}
> >  
> > -- 
> > 2.6.4
> > 

Re: [PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Lorenzo Pieralisi]

On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang@mediatek.com wrote:
> > > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > > 
> > > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > > the HW default class type values is invalid.
> > > 
> > > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > > type for MT7622") have set the class ID for MT7622 as
> > > PCI_CLASS_BRIDGE_HOSTe, but it's not workable for MT7622:
> > > 
> > > In __pci_bus_assign_resources, the framework only setup bridge's
> > > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > > will leave the subordinary PCIe device's MMIO window un-touched.

Can you please provide me with a full log of the issue ?

What is the bug this patch is actually fixing ?

> > > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > > driver do.
> > 
> > I think that this patch is correct but the commit log fails to pin point
> > the problem. The IP you are programming is a root port, that's why you
> > have to have the proper class code, the patch looks fine but I would
> > like to peek Bjorn's brain on this since it is a fundamental concept.
> > 
> 
> I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
> PCI_CLASS_BRIDGE_PCI, from PCI express spec,
> 4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
> "part of a Root Complex that connects a host CPU or CPUs to a
> Hierarchy". While Root Complex defines as "A defined System Element that
> includes at least one Host Bridge, Root port, or Root complex Integrated
> Endpoint".
> 
> But according to my understanding, most of the root port IPs integrated
> with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
> could be saw as a pci device when using lspci.
> 
> And for MT7622, it integrated with block of internal control registers,
> type 1 configuration space, and is considered as a root complex.

I assume you mean a type 1 config header here. I do not think it
is mandatory for a host bridge to have a type 1 config header
(and related bridge windows + primary/secondary/subordinate bus
numbers) but I do not know how the IP you are programming is
designed.

If the host bridge needs its memory window to be configured you can
easily do that in the driver (with driver specific code) without
requiring the generic PCI resource core to do that for you, the host
bridge is the root of the bus I do not see any reason why it should
be up to core PCI code to assign it, it is preprogrammed.

> I'm not sure which CLASS type it should have:
> From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
> 0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
> literally suitable for MT7622(which is a root complex)(In my personal
> opinion). But it is the only workable CLASS type for MT7622 in current
> kernel.
> 
> > If the kernel does not assign resources unless it detects a
> > PCI_CLASS_BRIDGE_PCI this means that for components that are
> > actually PCI_CLASS_BRIDGE_HOST their register set must come
> > preprogrammed unless I am missing something.
> > 
> 
> In the function pci_request_resource_alignment, it will by pass the
> resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
> out why.
> 
> > I would like to get to the bottom of this since it is a fundamental
> > enumeration concept.
> > 
> 
> Do you like me to re-write the commit message for this patch and put the
> above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
> assign resource routine?

I want to understand where the problem is and whether it is right to
define the component as a PCI bridge rather than a host bridge.

Lorenzo

[PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Lorenzo Pieralisi]

On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang at mediatek.com wrote:
> > > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > > 
> > > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > > the HW default class type values is invalid.
> > > 
> > > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > > type for MT7622") have set the class ID for MT7622 as
> > > PCI_CLASS_BRIDGE_HOSTe, but it's not workable for MT7622:
> > > 
> > > In __pci_bus_assign_resources, the framework only setup bridge's
> > > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > > will leave the subordinary PCIe device's MMIO window un-touched.

Can you please provide me with a full log of the issue ?

What is the bug this patch is actually fixing ?

> > > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > > driver do.
> > 
> > I think that this patch is correct but the commit log fails to pin point
> > the problem. The IP you are programming is a root port, that's why you
> > have to have the proper class code, the patch looks fine but I would
> > like to peek Bjorn's brain on this since it is a fundamental concept.
> > 
> 
> I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
> PCI_CLASS_BRIDGE_PCI, from PCI express spec,
> 4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
> "part of a Root Complex that connects a host CPU or CPUs to a
> Hierarchy". While Root Complex defines as "A defined System Element that
> includes at least one Host Bridge, Root port, or Root complex Integrated
> Endpoint".
> 
> But according to my understanding, most of the root port IPs integrated
> with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
> could be saw as a pci device when using lspci.
> 
> And for MT7622, it integrated with block of internal control registers,
> type 1 configuration space, and is considered as a root complex.

I assume you mean a type 1 config header here. I do not think it
is mandatory for a host bridge to have a type 1 config header
(and related bridge windows + primary/secondary/subordinate bus
numbers) but I do not know how the IP you are programming is
designed.

If the host bridge needs its memory window to be configured you can
easily do that in the driver (with driver specific code) without
requiring the generic PCI resource core to do that for you, the host
bridge is the root of the bus I do not see any reason why it should
be up to core PCI code to assign it, it is preprogrammed.

> I'm not sure which CLASS type it should have:
> From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
> 0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
> literally suitable for MT7622(which is a root complex)(In my personal
> opinion). But it is the only workable CLASS type for MT7622 in current
> kernel.
> 
> > If the kernel does not assign resources unless it detects a
> > PCI_CLASS_BRIDGE_PCI this means that for components that are
> > actually PCI_CLASS_BRIDGE_HOST their register set must come
> > preprogrammed unless I am missing something.
> > 
> 
> In the function pci_request_resource_alignment, it will by pass the
> resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
> out why.
> 
> > I would like to get to the bottom of this since it is a fundamental
> > enumeration concept.
> > 
> 
> Do you like me to re-write the commit message for this patch and put the
> above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
> assign resource routine?

I want to understand where the problem is and whether it is right to
define the component as a PCI bridge rather than a host bridge.

Lorenzo

Re: [PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Honghui Zhang]

On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> > On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > > On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang@mediatek.com wrote:
> > > > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > > > 
> > > > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > > > the HW default class type values is invalid.
> > > > 
> > > > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > > > type for MT7622") have set the class ID for MT7622 as
> > > > PCI_CLASS_BRIDGE_HOSTe, but it's not workable for MT7622:
> > > > 
> > > > In __pci_bus_assign_resources, the framework only setup bridge's
> > > > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > > > will leave the subordinary PCIe device's MMIO window un-touched.
> 
> Can you please provide me with a full log of the issue ?
> 
> What is the bug this patch is actually fixing ?
> 
> > > > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > > > driver do.
> > > 
> > > I think that this patch is correct but the commit log fails to pin point
> > > the problem. The IP you are programming is a root port, that's why you
> > > have to have the proper class code, the patch looks fine but I would
> > > like to peek Bjorn's brain on this since it is a fundamental concept.
> > > 
> > 
> > I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
> > PCI_CLASS_BRIDGE_PCI, from PCI express spec,
> > 4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
> > "part of a Root Complex that connects a host CPU or CPUs to a
> > Hierarchy". While Root Complex defines as "A defined System Element that
> > includes at least one Host Bridge, Root port, or Root complex Integrated
> > Endpoint".
> > 
> > But according to my understanding, most of the root port IPs integrated
> > with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
> > could be saw as a pci device when using lspci.
> > 
> > And for MT7622, it integrated with block of internal control registers,
> > type 1 configuration space, and is considered as a root complex.
> 
> I assume you mean a type 1 config header here. I do not think it
> is mandatory for a host bridge to have a type 1 config header
> (and related bridge windows + primary/secondary/subordinate bus
> numbers) but I do not know how the IP you are programming is
> designed.

Yes, MT7622 has the type 1 config header and related bridge windows and
primary/secondary/subordinate bus numbers.

> 
> If the host bridge needs its memory window to be configured you can
> easily do that in the driver (with driver specific code) without
> requiring the generic PCI resource core to do that for you, the host
> bridge is the root of the bus I do not see any reason why it should
> be up to core PCI code to assign it, it is preprogrammed.
> 
Thanks for explain this.
Yes, the MT7622 bridge needs its memory window to be configured but I
prefer the PCI resource core to do this for the following reasons:
1. MTK have MT7622 and MT2712, they pretty much using the same IP, but
have different memory window base. Currently we using device tree to
pass the memory window base. Take using of PCI resource core code to
parse and assign those resource will release the burden of driver.
2. I do not want to re-implement the resource parse code to get the
memory base from device node. And hard code the memory base in driver is
not quite elegant.
3. Most of the host driver which I referenced are using the PCI resource
core to help with it's memory window configure, I guess following the
majority way maybe better even they may have slit different of the IP
design from MTK.
4. Passing the memory base in device node make it more flexible to
change the memory window base (in the HW acceptable range) in case of
some special EP device required some special PCI address range.
5. MT7622 and MT2712 have the pretty much same IP only MT7622's HW has
set the class type to an un-proper class type. Set it to the same values
as MT2712 is an easy way to fix.

thanks.
> > I'm not sure which CLASS type it should have:
> > From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
> > 0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
> > literally suitable for MT7622(which is a root complex)(In my personal
> > opinion). But it is the only workable CLASS type for MT7622 in current
> > kernel.
> > 
> > > If the kernel does not assign resources unless it detects a
> > > PCI_CLASS_BRIDGE_PCI this means that for components that are
> > > actually PCI_CLASS_BRIDGE_HOST their register set must come
> > > preprogrammed unless I am missing something.
> > > 
> > 
> > In the function pci_request_resource_alignment, it will by pass the
> > resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
> > out why.
> > 
> > > I would like to get to the bottom of this since it is a fundamental
> > > enumeration concept.
> > > 
> > 
> > Do you like me to re-write the commit message for this patch and put the
> > above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
> > assign resource routine?
> 
> I want to understand where the problem is and whether it is right to
> define the component as a PCI bridge rather than a host bridge.
> 

I will update the commit message (put some of the above reasons in the
commit message) and send a new version of this patch if it's acceptable
for you.

Thanks
> Lorenzo

Re: [PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Honghui Zhang]

On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> > On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > > On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang@mediatek.com wrote:
> > > > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > > > 
> > > > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > > > the HW default class type values is invalid.
> > > > 
> > > > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > > > type for MT7622") have set the class ID for MT7622 as
> > > > PCI_CLASS_BRIDGE_HOSTe, but it's not workable for MT7622:
> > > > 
> > > > In __pci_bus_assign_resources, the framework only setup bridge's
> > > > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > > > will leave the subordinary PCIe device's MMIO window un-touched.
> 
> Can you please provide me with a full log of the issue ?
> 
> What is the bug this patch is actually fixing ?
> 
> > > > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > > > driver do.
> > > 
> > > I think that this patch is correct but the commit log fails to pin point
> > > the problem. The IP you are programming is a root port, that's why you
> > > have to have the proper class code, the patch looks fine but I would
> > > like to peek Bjorn's brain on this since it is a fundamental concept.
> > > 
> > 
> > I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
> > PCI_CLASS_BRIDGE_PCI, from PCI express spec,
> > 4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
> > "part of a Root Complex that connects a host CPU or CPUs to a
> > Hierarchy". While Root Complex defines as "A defined System Element that
> > includes at least one Host Bridge, Root port, or Root complex Integrated
> > Endpoint".
> > 
> > But according to my understanding, most of the root port IPs integrated
> > with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
> > could be saw as a pci device when using lspci.
> > 
> > And for MT7622, it integrated with block of internal control registers,
> > type 1 configuration space, and is considered as a root complex.
> 
> I assume you mean a type 1 config header here. I do not think it
> is mandatory for a host bridge to have a type 1 config header
> (and related bridge windows + primary/secondary/subordinate bus
> numbers) but I do not know how the IP you are programming is
> designed.

Yes, MT7622 has the type 1 config header and related bridge windows and
primary/secondary/subordinate bus numbers.

> 
> If the host bridge needs its memory window to be configured you can
> easily do that in the driver (with driver specific code) without
> requiring the generic PCI resource core to do that for you, the host
> bridge is the root of the bus I do not see any reason why it should
> be up to core PCI code to assign it, it is preprogrammed.
> 
Thanks for explain this.
Yes, the MT7622 bridge needs its memory window to be configured but I
prefer the PCI resource core to do this for the following reasons:
1. MTK have MT7622 and MT2712, they pretty much using the same IP, but
have different memory window base. Currently we using device tree to
pass the memory window base. Take using of PCI resource core code to
parse and assign those resource will release the burden of driver.
2. I do not want to re-implement the resource parse code to get the
memory base from device node. And hard code the memory base in driver is
not quite elegant.
3. Most of the host driver which I referenced are using the PCI resource
core to help with it's memory window configure, I guess following the
majority way maybe better even they may have slit different of the IP
design from MTK.
4. Passing the memory base in device node make it more flexible to
change the memory window base (in the HW acceptable range) in case of
some special EP device required some special PCI address range.
5. MT7622 and MT2712 have the pretty much same IP only MT7622's HW has
set the class type to an un-proper class type. Set it to the same values
as MT2712 is an easy way to fix.

thanks.
> > I'm not sure which CLASS type it should have:
> > From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
> > 0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
> > literally suitable for MT7622(which is a root complex)(In my personal
> > opinion). But it is the only workable CLASS type for MT7622 in current
> > kernel.
> > 
> > > If the kernel does not assign resources unless it detects a
> > > PCI_CLASS_BRIDGE_PCI this means that for components that are
> > > actually PCI_CLASS_BRIDGE_HOST their register set must come
> > > preprogrammed unless I am missing something.
> > > 
> > 
> > In the function pci_request_resource_alignment, it will by pass the
> > resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
> > out why.
> > 
> > > I would like to get to the bottom of this since it is a fundamental
> > > enumeration concept.
> > > 
> > 
> > Do you like me to re-write the commit message for this patch and put the
> > above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
> > assign resource routine?
> 
> I want to understand where the problem is and whether it is right to
> define the component as a PCI bridge rather than a host bridge.
> 

I will update the commit message (put some of the above reasons in the
commit message) and send a new version of this patch if it's acceptable
for you.

Thanks
> Lorenzo

[PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Honghui Zhang]

On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> > On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > > On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang at mediatek.com wrote:
> > > > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > > > 
> > > > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > > > the HW default class type values is invalid.
> > > > 
> > > > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > > > type for MT7622") have set the class ID for MT7622 as
> > > > PCI_CLASS_BRIDGE_HOSTe, but it's not workable for MT7622:
> > > > 
> > > > In __pci_bus_assign_resources, the framework only setup bridge's
> > > > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > > > will leave the subordinary PCIe device's MMIO window un-touched.
> 
> Can you please provide me with a full log of the issue ?
> 
> What is the bug this patch is actually fixing ?
> 
> > > > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > > > driver do.
> > > 
> > > I think that this patch is correct but the commit log fails to pin point
> > > the problem. The IP you are programming is a root port, that's why you
> > > have to have the proper class code, the patch looks fine but I would
> > > like to peek Bjorn's brain on this since it is a fundamental concept.
> > > 
> > 
> > I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
> > PCI_CLASS_BRIDGE_PCI, from PCI express spec,
> > 4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
> > "part of a Root Complex that connects a host CPU or CPUs to a
> > Hierarchy". While Root Complex defines as "A defined System Element that
> > includes at least one Host Bridge, Root port, or Root complex Integrated
> > Endpoint".
> > 
> > But according to my understanding, most of the root port IPs integrated
> > with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
> > could be saw as a pci device when using lspci.
> > 
> > And for MT7622, it integrated with block of internal control registers,
> > type 1 configuration space, and is considered as a root complex.
> 
> I assume you mean a type 1 config header here. I do not think it
> is mandatory for a host bridge to have a type 1 config header
> (and related bridge windows + primary/secondary/subordinate bus
> numbers) but I do not know how the IP you are programming is
> designed.

Yes, MT7622 has the type 1 config header and related bridge windows and
primary/secondary/subordinate bus numbers.

> 
> If the host bridge needs its memory window to be configured you can
> easily do that in the driver (with driver specific code) without
> requiring the generic PCI resource core to do that for you, the host
> bridge is the root of the bus I do not see any reason why it should
> be up to core PCI code to assign it, it is preprogrammed.
> 
Thanks for explain this.
Yes, the MT7622 bridge needs its memory window to be configured but I
prefer the PCI resource core to do this for the following reasons:
1. MTK have MT7622 and MT2712, they pretty much using the same IP, but
have different memory window base. Currently we using device tree to
pass the memory window base. Take using of PCI resource core code to
parse and assign those resource will release the burden of driver.
2. I do not want to re-implement the resource parse code to get the
memory base from device node. And hard code the memory base in driver is
not quite elegant.
3. Most of the host driver which I referenced are using the PCI resource
core to help with it's memory window configure, I guess following the
majority way maybe better even they may have slit different of the IP
design from MTK.
4. Passing the memory base in device node make it more flexible to
change the memory window base (in the HW acceptable range) in case of
some special EP device required some special PCI address range.
5. MT7622 and MT2712 have the pretty much same IP only MT7622's HW has
set the class type to an un-proper class type. Set it to the same values
as MT2712 is an easy way to fix.

thanks.
> > I'm not sure which CLASS type it should have:
> > From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
> > 0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
> > literally suitable for MT7622(which is a root complex)(In my personal
> > opinion). But it is the only workable CLASS type for MT7622 in current
> > kernel.
> > 
> > > If the kernel does not assign resources unless it detects a
> > > PCI_CLASS_BRIDGE_PCI this means that for components that are
> > > actually PCI_CLASS_BRIDGE_HOST their register set must come
> > > preprogrammed unless I am missing something.
> > > 
> > 
> > In the function pci_request_resource_alignment, it will by pass the
> > resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
> > out why.
> > 
> > > I would like to get to the bottom of this since it is a fundamental
> > > enumeration concept.
> > > 
> > 
> > Do you like me to re-write the commit message for this patch and put the
> > above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
> > assign resource routine?
> 
> I want to understand where the problem is and whether it is right to
> define the component as a PCI bridge rather than a host bridge.
> 

I will update the commit message (put some of the above reasons in the
commit message) and send a new version of this patch if it's acceptable
for you.

Thanks
> Lorenzo

Re: [PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Lorenzo Pieralisi]

On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
> On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> > On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> > > On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > > > On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang@mediatek.com wrote:
> > > > > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > > > > 
> > > > > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > > > > the HW default class type values is invalid.
> > > > > 
> > > > > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > > > > type for MT7622") have set the class ID for MT7622 as
> > > > > PCI_CLASS_BRIDGE_HOSTe, but it's not workable for MT7622:
> > > > > 
> > > > > In __pci_bus_assign_resources, the framework only setup bridge's
> > > > > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > > > > will leave the subordinary PCIe device's MMIO window un-touched.
> > 
> > Can you please provide me with a full log of the issue ?
> > 
> > What is the bug this patch is actually fixing ?
> > 
> > > > > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > > > > driver do.
> > > > 
> > > > I think that this patch is correct but the commit log fails to pin point
> > > > the problem. The IP you are programming is a root port, that's why you
> > > > have to have the proper class code, the patch looks fine but I would
> > > > like to peek Bjorn's brain on this since it is a fundamental concept.
> > > > 
> > > 
> > > I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
> > > PCI_CLASS_BRIDGE_PCI, from PCI express spec,
> > > 4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
> > > "part of a Root Complex that connects a host CPU or CPUs to a
> > > Hierarchy". While Root Complex defines as "A defined System Element that
> > > includes at least one Host Bridge, Root port, or Root complex Integrated
> > > Endpoint".
> > > 
> > > But according to my understanding, most of the root port IPs integrated
> > > with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
> > > could be saw as a pci device when using lspci.
> > > 
> > > And for MT7622, it integrated with block of internal control registers,
> > > type 1 configuration space, and is considered as a root complex.
> > 
> > I assume you mean a type 1 config header here. I do not think it
> > is mandatory for a host bridge to have a type 1 config header
> > (and related bridge windows + primary/secondary/subordinate bus
> > numbers) but I do not know how the IP you are programming is
> > designed.
> 
> Yes, MT7622 has the type 1 config header and related bridge windows and
> primary/secondary/subordinate bus numbers.
> 
> > 
> > If the host bridge needs its memory window to be configured you can
> > easily do that in the driver (with driver specific code) without
> > requiring the generic PCI resource core to do that for you, the host
> > bridge is the root of the bus I do not see any reason why it should
> > be up to core PCI code to assign it, it is preprogrammed.
> > 
> Thanks for explain this.
> Yes, the MT7622 bridge needs its memory window to be configured but I
> prefer the PCI resource core to do this for the following reasons:
> 1. MTK have MT7622 and MT2712, they pretty much using the same IP, but
> have different memory window base. Currently we using device tree to
> pass the memory window base. Take using of PCI resource core code to
> parse and assign those resource will release the burden of driver.
> 2. I do not want to re-implement the resource parse code to get the
> memory base from device node. And hard code the memory base in driver is
> not quite elegant.
> 3. Most of the host driver which I referenced are using the PCI resource
> core to help with it's memory window configure, I guess following the
> majority way maybe better even they may have slit different of the IP
> design from MTK.
> 4. Passing the memory base in device node make it more flexible to
> change the memory window base (in the HW acceptable range) in case of
> some special EP device required some special PCI address range.
> 5. MT7622 and MT2712 have the pretty much same IP only MT7622's HW has
> set the class type to an un-proper class type. Set it to the same values
> as MT2712 is an easy way to fix.

We do what needs to be done. From what you are saying, your IP
implements a config 1 type header and that defines it as a PCI-to-PCI
bridge (eg a root port, of sorts).

The points you are making above are software, I understand them but
that's not what we are discussing here.

I want you to define the class of your IP according to what your IP
is and it seems _reasonable_ to me to define the IP as a PCI-to-PCI
bridge class from the information you are providing.

I would like you to:

1) Rewrite the commit log and explain why your IP needs class
   reprogramming. I do not care about driver software complexity,
   I want you to describe how HW works. I do not want you to define
   a class for your IP because that makes the driver simpler, I want
   you to define a class for your IP to describe to the kernel what
   that IP really is, which is different things.
2) Define and report the bug you are fixing in the commit log
3) Provide a Fixes: tag pointing at the faulty commit

Thanks for your time providing information.

Lorenzo

> thanks.
> > > I'm not sure which CLASS type it should have:
> > > From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
> > > 0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
> > > literally suitable for MT7622(which is a root complex)(In my personal
> > > opinion). But it is the only workable CLASS type for MT7622 in current
> > > kernel.
> > > 
> > > > If the kernel does not assign resources unless it detects a
> > > > PCI_CLASS_BRIDGE_PCI this means that for components that are
> > > > actually PCI_CLASS_BRIDGE_HOST their register set must come
> > > > preprogrammed unless I am missing something.
> > > > 
> > > 
> > > In the function pci_request_resource_alignment, it will by pass the
> > > resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
> > > out why.
> > > 
> > > > I would like to get to the bottom of this since it is a fundamental
> > > > enumeration concept.
> > > > 
> > > 
> > > Do you like me to re-write the commit message for this patch and put the
> > > above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
> > > assign resource routine?
> > 
> > I want to understand where the problem is and whether it is right to
> > define the component as a PCI bridge rather than a host bridge.
> > 
> 
> I will update the commit message (put some of the above reasons in the
> commit message) and send a new version of this patch if it's acceptable
> for you.
> 
> Thanks
> > Lorenzo
> 
> 
> 

[PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Lorenzo Pieralisi]

On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
> On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> > On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> > > On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > > > On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang at mediatek.com wrote:
> > > > > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > > > > 
> > > > > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > > > > the HW default class type values is invalid.
> > > > > 
> > > > > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > > > > type for MT7622") have set the class ID for MT7622 as
> > > > > PCI_CLASS_BRIDGE_HOSTe, but it's not workable for MT7622:
> > > > > 
> > > > > In __pci_bus_assign_resources, the framework only setup bridge's
> > > > > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > > > > will leave the subordinary PCIe device's MMIO window un-touched.
> > 
> > Can you please provide me with a full log of the issue ?
> > 
> > What is the bug this patch is actually fixing ?
> > 
> > > > > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > > > > driver do.
> > > > 
> > > > I think that this patch is correct but the commit log fails to pin point
> > > > the problem. The IP you are programming is a root port, that's why you
> > > > have to have the proper class code, the patch looks fine but I would
> > > > like to peek Bjorn's brain on this since it is a fundamental concept.
> > > > 
> > > 
> > > I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
> > > PCI_CLASS_BRIDGE_PCI, from PCI express spec,
> > > 4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
> > > "part of a Root Complex that connects a host CPU or CPUs to a
> > > Hierarchy". While Root Complex defines as "A defined System Element that
> > > includes at least one Host Bridge, Root port, or Root complex Integrated
> > > Endpoint".
> > > 
> > > But according to my understanding, most of the root port IPs integrated
> > > with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
> > > could be saw as a pci device when using lspci.
> > > 
> > > And for MT7622, it integrated with block of internal control registers,
> > > type 1 configuration space, and is considered as a root complex.
> > 
> > I assume you mean a type 1 config header here. I do not think it
> > is mandatory for a host bridge to have a type 1 config header
> > (and related bridge windows + primary/secondary/subordinate bus
> > numbers) but I do not know how the IP you are programming is
> > designed.
> 
> Yes, MT7622 has the type 1 config header and related bridge windows and
> primary/secondary/subordinate bus numbers.
> 
> > 
> > If the host bridge needs its memory window to be configured you can
> > easily do that in the driver (with driver specific code) without
> > requiring the generic PCI resource core to do that for you, the host
> > bridge is the root of the bus I do not see any reason why it should
> > be up to core PCI code to assign it, it is preprogrammed.
> > 
> Thanks for explain this.
> Yes, the MT7622 bridge needs its memory window to be configured but I
> prefer the PCI resource core to do this for the following reasons:
> 1. MTK have MT7622 and MT2712, they pretty much using the same IP, but
> have different memory window base. Currently we using device tree to
> pass the memory window base. Take using of PCI resource core code to
> parse and assign those resource will release the burden of driver.
> 2. I do not want to re-implement the resource parse code to get the
> memory base from device node. And hard code the memory base in driver is
> not quite elegant.
> 3. Most of the host driver which I referenced are using the PCI resource
> core to help with it's memory window configure, I guess following the
> majority way maybe better even they may have slit different of the IP
> design from MTK.
> 4. Passing the memory base in device node make it more flexible to
> change the memory window base (in the HW acceptable range) in case of
> some special EP device required some special PCI address range.
> 5. MT7622 and MT2712 have the pretty much same IP only MT7622's HW has
> set the class type to an un-proper class type. Set it to the same values
> as MT2712 is an easy way to fix.

We do what needs to be done. From what you are saying, your IP
implements a config 1 type header and that defines it as a PCI-to-PCI
bridge (eg a root port, of sorts).

The points you are making above are software, I understand them but
that's not what we are discussing here.

I want you to define the class of your IP according to what your IP
is and it seems _reasonable_ to me to define the IP as a PCI-to-PCI
bridge class from the information you are providing.

I would like you to:

1) Rewrite the commit log and explain why your IP needs class
   reprogramming. I do not care about driver software complexity,
   I want you to describe how HW works. I do not want you to define
   a class for your IP because that makes the driver simpler, I want
   you to define a class for your IP to describe to the kernel what
   that IP really is, which is different things.
2) Define and report the bug you are fixing in the commit log
3) Provide a Fixes: tag pointing at the faulty commit

Thanks for your time providing information.

Lorenzo

> thanks.
> > > I'm not sure which CLASS type it should have:
> > > From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
> > > 0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
> > > literally suitable for MT7622(which is a root complex)(In my personal
> > > opinion). But it is the only workable CLASS type for MT7622 in current
> > > kernel.
> > > 
> > > > If the kernel does not assign resources unless it detects a
> > > > PCI_CLASS_BRIDGE_PCI this means that for components that are
> > > > actually PCI_CLASS_BRIDGE_HOST their register set must come
> > > > preprogrammed unless I am missing something.
> > > > 
> > > 
> > > In the function pci_request_resource_alignment, it will by pass the
> > > resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
> > > out why.
> > > 
> > > > I would like to get to the bottom of this since it is a fundamental
> > > > enumeration concept.
> > > > 
> > > 
> > > Do you like me to re-write the commit message for this patch and put the
> > > above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
> > > assign resource routine?
> > 
> > I want to understand where the problem is and whether it is right to
> > define the component as a PCI bridge rather than a host bridge.
> > 
> 
> I will update the commit message (put some of the above reasons in the
> commit message) and send a new version of this patch if it's acceptable
> for you.
> 
> Thanks
> > Lorenzo
> 
> 
> 

Re: [PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Bjorn Helgaas]

On Fri, Oct 12, 2018 at 11:22:30AM +0100, Lorenzo Pieralisi wrote:
> On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
>> On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
>>> On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
>>>> On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
>>>>> On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang@mediatek.com wrote:
>>>>>> From: Honghui Zhang <honghui.zhang@mediatek.com>
>>>>>> 
>>>>>> The PCIe controller of MT7622 has TYPE 1 configuration
>>>>>> space type, but the HW default class type values is
>>>>>> invalid.
>>>>>> 
>>>>>> The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID
>>>>>> and class type for MT7622") have set the class ID for
>>>>>> MT7622 as PCI_CLASS_BRIDGE_HOSTe, but it's not workable
>>>>>> for MT7622:
>>>>>> 
>>>>>> In __pci_bus_assign_resources, the framework only setup
>>>>>> bridge's resource window only if class type is
>>>>>> PCI_CLASS_BRIDGE_PCI. Or it will leave the subordinary PCIe
>>>>>> device's MMIO window un-touched.

I think __pci_bus_assign_resources() should be testing dev->hdr_type
instead of dev->class.  The connection between "Header Type" and the
layout of the rest of the header is very explicit (PCI r3.0 sec 6.1,
PCIe r4.0 sec 7.5.1.1.9), and the reason for the switch statement in
__pci_bus_assign_resources() is precisely to determine which layout to
use.

There are several other uses of dev->class in setup-bus.c that I think
should also be changed to use dev->hdr_type.

If we make these changes in setup-bus.c, I suspect the class code you
assign won't matter too much.  There are a few other tests of the
class code to figure out whether to leave certain things untouched.
These seem a little hacky to me, but we're probably stuck with them
for now, so you should look and see whether they apply to your
situation.

>>>> And for MT7622, it integrated with block of internal control
>>>> registers, type 1 configuration space, and is considered as a
>>>> root complex.
>>> 
>>> I assume you mean a type 1 config header here. I do not think it
>>> is mandatory for a host bridge to have a type 1 config header (and
>>> related bridge windows + primary/secondary/subordinate bus
>>> numbers) but I do not know how the IP you are programming is
>>> designed.

It is definitely not mandatory for a host bridge to have a type 1
header.  I'm not even sure that would make sense: the "Primary Bus
Number" would not apply to a host bridge (since a host bridge's
primary bus is some sort of CPU bus, not a PCI bus), and a type 1
device cannot perform address translation between its primary and
secondary buses, while a host bridge can.

A Root Port is a type 1 device where the primary bus is logically
internal to the Root Complex.  A host bridge bridges from the CPU bus
to that internal bus and might perform address translation.  The Root
Port must be a PCI device.  A host bridge, being a bridge *to* the PCI
domain, is not itself generally programmed via PCI config space and
might not even be visible as a device in PCI config space.

Bjorn

[PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Bjorn Helgaas]

On Fri, Oct 12, 2018 at 11:22:30AM +0100, Lorenzo Pieralisi wrote:
> On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
>> On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
>>> On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
>>>> On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
>>>>> On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang at mediatek.com wrote:
>>>>>> From: Honghui Zhang <honghui.zhang@mediatek.com>
>>>>>> 
>>>>>> The PCIe controller of MT7622 has TYPE 1 configuration
>>>>>> space type, but the HW default class type values is
>>>>>> invalid.
>>>>>> 
>>>>>> The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID
>>>>>> and class type for MT7622") have set the class ID for
>>>>>> MT7622 as PCI_CLASS_BRIDGE_HOSTe, but it's not workable
>>>>>> for MT7622:
>>>>>> 
>>>>>> In __pci_bus_assign_resources, the framework only setup
>>>>>> bridge's resource window only if class type is
>>>>>> PCI_CLASS_BRIDGE_PCI. Or it will leave the subordinary PCIe
>>>>>> device's MMIO window un-touched.

I think __pci_bus_assign_resources() should be testing dev->hdr_type
instead of dev->class.  The connection between "Header Type" and the
layout of the rest of the header is very explicit (PCI r3.0 sec 6.1,
PCIe r4.0 sec 7.5.1.1.9), and the reason for the switch statement in
__pci_bus_assign_resources() is precisely to determine which layout to
use.

There are several other uses of dev->class in setup-bus.c that I think
should also be changed to use dev->hdr_type.

If we make these changes in setup-bus.c, I suspect the class code you
assign won't matter too much.  There are a few other tests of the
class code to figure out whether to leave certain things untouched.
These seem a little hacky to me, but we're probably stuck with them
for now, so you should look and see whether they apply to your
situation.

>>>> And for MT7622, it integrated with block of internal control
>>>> registers, type 1 configuration space, and is considered as a
>>>> root complex.
>>> 
>>> I assume you mean a type 1 config header here. I do not think it
>>> is mandatory for a host bridge to have a type 1 config header (and
>>> related bridge windows + primary/secondary/subordinate bus
>>> numbers) but I do not know how the IP you are programming is
>>> designed.

It is definitely not mandatory for a host bridge to have a type 1
header.  I'm not even sure that would make sense: the "Primary Bus
Number" would not apply to a host bridge (since a host bridge's
primary bus is some sort of CPU bus, not a PCI bus), and a type 1
device cannot perform address translation between its primary and
secondary buses, while a host bridge can.

A Root Port is a type 1 device where the primary bus is logically
internal to the Root Complex.  A host bridge bridges from the CPU bus
to that internal bus and might perform address translation.  The Root
Port must be a PCI device.  A host bridge, being a bridge *to* the PCI
domain, is not itself generally programmed via PCI config space and
might not even be visible as a device in PCI config space.

Bjorn

Re: [PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Honghui Zhang]

On Fri, 2018-10-12 at 11:22 +0100, Lorenzo Pieralisi wrote:
> On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
> > On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> > > On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> > > > On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > > > > On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang@mediatek.com wrote:
> > > > > > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > > > > > 
> > > > > > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > > > > > the HW default class type values is invalid.
> > > > > > 
> > > > > > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > > > > > type for MT7622") have set the class ID for MT7622 as
> > > > > > PCI_CLASS_BRIDGE_HOSTe, but it's not workable for MT7622:
> > > > > > 
> > > > > > In __pci_bus_assign_resources, the framework only setup bridge's
> > > > > > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > > > > > will leave the subordinary PCIe device's MMIO window un-touched.
> > > 
> > > Can you please provide me with a full log of the issue ?
> > > 
> > > What is the bug this patch is actually fixing ?
> > > 
> > > > > > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > > > > > driver do.
> > > > > 
> > > > > I think that this patch is correct but the commit log fails to pin point
> > > > > the problem. The IP you are programming is a root port, that's why you
> > > > > have to have the proper class code, the patch looks fine but I would
> > > > > like to peek Bjorn's brain on this since it is a fundamental concept.
> > > > > 
> > > > 
> > > > I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
> > > > PCI_CLASS_BRIDGE_PCI, from PCI express spec,
> > > > 4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
> > > > "part of a Root Complex that connects a host CPU or CPUs to a
> > > > Hierarchy". While Root Complex defines as "A defined System Element that
> > > > includes at least one Host Bridge, Root port, or Root complex Integrated
> > > > Endpoint".
> > > > 
> > > > But according to my understanding, most of the root port IPs integrated
> > > > with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
> > > > could be saw as a pci device when using lspci.
> > > > 
> > > > And for MT7622, it integrated with block of internal control registers,
> > > > type 1 configuration space, and is considered as a root complex.
> > > 
> > > I assume you mean a type 1 config header here. I do not think it
> > > is mandatory for a host bridge to have a type 1 config header
> > > (and related bridge windows + primary/secondary/subordinate bus
> > > numbers) but I do not know how the IP you are programming is
> > > designed.
> > 
> > Yes, MT7622 has the type 1 config header and related bridge windows and
> > primary/secondary/subordinate bus numbers.
> > 
> > > 
> > > If the host bridge needs its memory window to be configured you can
> > > easily do that in the driver (with driver specific code) without
> > > requiring the generic PCI resource core to do that for you, the host
> > > bridge is the root of the bus I do not see any reason why it should
> > > be up to core PCI code to assign it, it is preprogrammed.
> > > 
> > Thanks for explain this.
> > Yes, the MT7622 bridge needs its memory window to be configured but I
> > prefer the PCI resource core to do this for the following reasons:
> > 1. MTK have MT7622 and MT2712, they pretty much using the same IP, but
> > have different memory window base. Currently we using device tree to
> > pass the memory window base. Take using of PCI resource core code to
> > parse and assign those resource will release the burden of driver.
> > 2. I do not want to re-implement the resource parse code to get the
> > memory base from device node. And hard code the memory base in driver is
> > not quite elegant.
> > 3. Most of the host driver which I referenced are using the PCI resource
> > core to help with it's memory window configure, I guess following the
> > majority way maybe better even they may have slit different of the IP
> > design from MTK.
> > 4. Passing the memory base in device node make it more flexible to
> > change the memory window base (in the HW acceptable range) in case of
> > some special EP device required some special PCI address range.
> > 5. MT7622 and MT2712 have the pretty much same IP only MT7622's HW has
> > set the class type to an un-proper class type. Set it to the same values
> > as MT2712 is an easy way to fix.
> 
> We do what needs to be done. From what you are saying, your IP
> implements a config 1 type header and that defines it as a PCI-to-PCI
> bridge (eg a root port, of sorts).
> 
> The points you are making above are software, I understand them but
> that's not what we are discussing here.
> 
> I want you to define the class of your IP according to what your IP
> is and it seems _reasonable_ to me to define the IP as a PCI-to-PCI
> bridge class from the information you are providing.
> 
> I would like you to:
> 
> 1) Rewrite the commit log and explain why your IP needs class
>    reprogramming. I do not care about driver software complexity,
>    I want you to describe how HW works. I do not want you to define
>    a class for your IP because that makes the driver simpler, I want
>    you to define a class for your IP to describe to the kernel what
>    that IP really is, which is different things.
> 2) Define and report the bug you are fixing in the commit log
> 3) Provide a Fixes: tag pointing at the faulty commit

Thanks for your explain, I will follow your suggest in the next version.

Thanks.

> Thanks for your time providing information.
> 
> Lorenzo
> 
> > thanks.
> > > > I'm not sure which CLASS type it should have:
> > > > From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
> > > > 0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
> > > > literally suitable for MT7622(which is a root complex)(In my personal
> > > > opinion). But it is the only workable CLASS type for MT7622 in current
> > > > kernel.
> > > > 
> > > > > If the kernel does not assign resources unless it detects a
> > > > > PCI_CLASS_BRIDGE_PCI this means that for components that are
> > > > > actually PCI_CLASS_BRIDGE_HOST their register set must come
> > > > > preprogrammed unless I am missing something.
> > > > > 
> > > > 
> > > > In the function pci_request_resource_alignment, it will by pass the
> > > > resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
> > > > out why.
> > > > 
> > > > > I would like to get to the bottom of this since it is a fundamental
> > > > > enumeration concept.
> > > > > 
> > > > 
> > > > Do you like me to re-write the commit message for this patch and put the
> > > > above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
> > > > assign resource routine?
> > > 
> > > I want to understand where the problem is and whether it is right to
> > > define the component as a PCI bridge rather than a host bridge.
> > > 
> > 
> > I will update the commit message (put some of the above reasons in the
> > commit message) and send a new version of this patch if it's acceptable
> > for you.
> > 
> > Thanks
> > > Lorenzo
> > 
> > 
> > 

Re: [PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Honghui Zhang]

On Fri, 2018-10-12 at 11:22 +0100, Lorenzo Pieralisi wrote:
> On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
> > On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> > > On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> > > > On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > > > > On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org wrote:
> > > > > > From: Honghui Zhang <honghui.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > > > > > 
> > > > > > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > > > > > the HW default class type values is invalid.
> > > > > > 
> > > > > > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > > > > > type for MT7622") have set the class ID for MT7622 as
> > > > > > PCI_CLASS_BRIDGE_HOSTe, but it's not workable for MT7622:
> > > > > > 
> > > > > > In __pci_bus_assign_resources, the framework only setup bridge's
> > > > > > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > > > > > will leave the subordinary PCIe device's MMIO window un-touched.
> > > 
> > > Can you please provide me with a full log of the issue ?
> > > 
> > > What is the bug this patch is actually fixing ?
> > > 
> > > > > > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > > > > > driver do.
> > > > > 
> > > > > I think that this patch is correct but the commit log fails to pin point
> > > > > the problem. The IP you are programming is a root port, that's why you
> > > > > have to have the proper class code, the patch looks fine but I would
> > > > > like to peek Bjorn's brain on this since it is a fundamental concept.
> > > > > 
> > > > 
> > > > I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
> > > > PCI_CLASS_BRIDGE_PCI, from PCI express spec,
> > > > 4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
> > > > "part of a Root Complex that connects a host CPU or CPUs to a
> > > > Hierarchy". While Root Complex defines as "A defined System Element that
> > > > includes at least one Host Bridge, Root port, or Root complex Integrated
> > > > Endpoint".
> > > > 
> > > > But according to my understanding, most of the root port IPs integrated
> > > > with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
> > > > could be saw as a pci device when using lspci.
> > > > 
> > > > And for MT7622, it integrated with block of internal control registers,
> > > > type 1 configuration space, and is considered as a root complex.
> > > 
> > > I assume you mean a type 1 config header here. I do not think it
> > > is mandatory for a host bridge to have a type 1 config header
> > > (and related bridge windows + primary/secondary/subordinate bus
> > > numbers) but I do not know how the IP you are programming is
> > > designed.
> > 
> > Yes, MT7622 has the type 1 config header and related bridge windows and
> > primary/secondary/subordinate bus numbers.
> > 
> > > 
> > > If the host bridge needs its memory window to be configured you can
> > > easily do that in the driver (with driver specific code) without
> > > requiring the generic PCI resource core to do that for you, the host
> > > bridge is the root of the bus I do not see any reason why it should
> > > be up to core PCI code to assign it, it is preprogrammed.
> > > 
> > Thanks for explain this.
> > Yes, the MT7622 bridge needs its memory window to be configured but I
> > prefer the PCI resource core to do this for the following reasons:
> > 1. MTK have MT7622 and MT2712, they pretty much using the same IP, but
> > have different memory window base. Currently we using device tree to
> > pass the memory window base. Take using of PCI resource core code to
> > parse and assign those resource will release the burden of driver.
> > 2. I do not want to re-implement the resource parse code to get the
> > memory base from device node. And hard code the memory base in driver is
> > not quite elegant.
> > 3. Most of the host driver which I referenced are using the PCI resource
> > core to help with it's memory window configure, I guess following the
> > majority way maybe better even they may have slit different of the IP
> > design from MTK.
> > 4. Passing the memory base in device node make it more flexible to
> > change the memory window base (in the HW acceptable range) in case of
> > some special EP device required some special PCI address range.
> > 5. MT7622 and MT2712 have the pretty much same IP only MT7622's HW has
> > set the class type to an un-proper class type. Set it to the same values
> > as MT2712 is an easy way to fix.
> 
> We do what needs to be done. From what you are saying, your IP
> implements a config 1 type header and that defines it as a PCI-to-PCI
> bridge (eg a root port, of sorts).
> 
> The points you are making above are software, I understand them but
> that's not what we are discussing here.
> 
> I want you to define the class of your IP according to what your IP
> is and it seems _reasonable_ to me to define the IP as a PCI-to-PCI
> bridge class from the information you are providing.
> 
> I would like you to:
> 
> 1) Rewrite the commit log and explain why your IP needs class
>    reprogramming. I do not care about driver software complexity,
>    I want you to describe how HW works. I do not want you to define
>    a class for your IP because that makes the driver simpler, I want
>    you to define a class for your IP to describe to the kernel what
>    that IP really is, which is different things.
> 2) Define and report the bug you are fixing in the commit log
> 3) Provide a Fixes: tag pointing at the faulty commit

Thanks for your explain, I will follow your suggest in the next version.

Thanks.

> Thanks for your time providing information.
> 
> Lorenzo
> 
> > thanks.
> > > > I'm not sure which CLASS type it should have:
> > > > From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
> > > > 0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
> > > > literally suitable for MT7622(which is a root complex)(In my personal
> > > > opinion). But it is the only workable CLASS type for MT7622 in current
> > > > kernel.
> > > > 
> > > > > If the kernel does not assign resources unless it detects a
> > > > > PCI_CLASS_BRIDGE_PCI this means that for components that are
> > > > > actually PCI_CLASS_BRIDGE_HOST their register set must come
> > > > > preprogrammed unless I am missing something.
> > > > > 
> > > > 
> > > > In the function pci_request_resource_alignment, it will by pass the
> > > > resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
> > > > out why.
> > > > 
> > > > > I would like to get to the bottom of this since it is a fundamental
> > > > > enumeration concept.
> > > > > 
> > > > 
> > > > Do you like me to re-write the commit message for this patch and put the
> > > > above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
> > > > assign resource routine?
> > > 
> > > I want to understand where the problem is and whether it is right to
> > > define the component as a PCI bridge rather than a host bridge.
> > > 
> > 
> > I will update the commit message (put some of the above reasons in the
> > commit message) and send a new version of this patch if it's acceptable
> > for you.
> > 
> > Thanks
> > > Lorenzo
> > 
> > 
> > 

[PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Honghui Zhang]

On Fri, 2018-10-12 at 11:22 +0100, Lorenzo Pieralisi wrote:
> On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
> > On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> > > On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> > > > On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > > > > On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang at mediatek.com wrote:
> > > > > > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > > > > > 
> > > > > > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > > > > > the HW default class type values is invalid.
> > > > > > 
> > > > > > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > > > > > type for MT7622") have set the class ID for MT7622 as
> > > > > > PCI_CLASS_BRIDGE_HOSTe, but it's not workable for MT7622:
> > > > > > 
> > > > > > In __pci_bus_assign_resources, the framework only setup bridge's
> > > > > > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > > > > > will leave the subordinary PCIe device's MMIO window un-touched.
> > > 
> > > Can you please provide me with a full log of the issue ?
> > > 
> > > What is the bug this patch is actually fixing ?
> > > 
> > > > > > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > > > > > driver do.
> > > > > 
> > > > > I think that this patch is correct but the commit log fails to pin point
> > > > > the problem. The IP you are programming is a root port, that's why you
> > > > > have to have the proper class code, the patch looks fine but I would
> > > > > like to peek Bjorn's brain on this since it is a fundamental concept.
> > > > > 
> > > > 
> > > > I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
> > > > PCI_CLASS_BRIDGE_PCI, from PCI express spec,
> > > > 4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
> > > > "part of a Root Complex that connects a host CPU or CPUs to a
> > > > Hierarchy". While Root Complex defines as "A defined System Element that
> > > > includes at least one Host Bridge, Root port, or Root complex Integrated
> > > > Endpoint".
> > > > 
> > > > But according to my understanding, most of the root port IPs integrated
> > > > with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
> > > > could be saw as a pci device when using lspci.
> > > > 
> > > > And for MT7622, it integrated with block of internal control registers,
> > > > type 1 configuration space, and is considered as a root complex.
> > > 
> > > I assume you mean a type 1 config header here. I do not think it
> > > is mandatory for a host bridge to have a type 1 config header
> > > (and related bridge windows + primary/secondary/subordinate bus
> > > numbers) but I do not know how the IP you are programming is
> > > designed.
> > 
> > Yes, MT7622 has the type 1 config header and related bridge windows and
> > primary/secondary/subordinate bus numbers.
> > 
> > > 
> > > If the host bridge needs its memory window to be configured you can
> > > easily do that in the driver (with driver specific code) without
> > > requiring the generic PCI resource core to do that for you, the host
> > > bridge is the root of the bus I do not see any reason why it should
> > > be up to core PCI code to assign it, it is preprogrammed.
> > > 
> > Thanks for explain this.
> > Yes, the MT7622 bridge needs its memory window to be configured but I
> > prefer the PCI resource core to do this for the following reasons:
> > 1. MTK have MT7622 and MT2712, they pretty much using the same IP, but
> > have different memory window base. Currently we using device tree to
> > pass the memory window base. Take using of PCI resource core code to
> > parse and assign those resource will release the burden of driver.
> > 2. I do not want to re-implement the resource parse code to get the
> > memory base from device node. And hard code the memory base in driver is
> > not quite elegant.
> > 3. Most of the host driver which I referenced are using the PCI resource
> > core to help with it's memory window configure, I guess following the
> > majority way maybe better even they may have slit different of the IP
> > design from MTK.
> > 4. Passing the memory base in device node make it more flexible to
> > change the memory window base (in the HW acceptable range) in case of
> > some special EP device required some special PCI address range.
> > 5. MT7622 and MT2712 have the pretty much same IP only MT7622's HW has
> > set the class type to an un-proper class type. Set it to the same values
> > as MT2712 is an easy way to fix.
> 
> We do what needs to be done. From what you are saying, your IP
> implements a config 1 type header and that defines it as a PCI-to-PCI
> bridge (eg a root port, of sorts).
> 
> The points you are making above are software, I understand them but
> that's not what we are discussing here.
> 
> I want you to define the class of your IP according to what your IP
> is and it seems _reasonable_ to me to define the IP as a PCI-to-PCI
> bridge class from the information you are providing.
> 
> I would like you to:
> 
> 1) Rewrite the commit log and explain why your IP needs class
>    reprogramming. I do not care about driver software complexity,
>    I want you to describe how HW works. I do not want you to define
>    a class for your IP because that makes the driver simpler, I want
>    you to define a class for your IP to describe to the kernel what
>    that IP really is, which is different things.
> 2) Define and report the bug you are fixing in the commit log
> 3) Provide a Fixes: tag pointing at the faulty commit

Thanks for your explain, I will follow your suggest in the next version.

Thanks.

> Thanks for your time providing information.
> 
> Lorenzo
> 
> > thanks.
> > > > I'm not sure which CLASS type it should have:
> > > > From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
> > > > 0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
> > > > literally suitable for MT7622(which is a root complex)(In my personal
> > > > opinion). But it is the only workable CLASS type for MT7622 in current
> > > > kernel.
> > > > 
> > > > > If the kernel does not assign resources unless it detects a
> > > > > PCI_CLASS_BRIDGE_PCI this means that for components that are
> > > > > actually PCI_CLASS_BRIDGE_HOST their register set must come
> > > > > preprogrammed unless I am missing something.
> > > > > 
> > > > 
> > > > In the function pci_request_resource_alignment, it will by pass the
> > > > resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
> > > > out why.
> > > > 
> > > > > I would like to get to the bottom of this since it is a fundamental
> > > > > enumeration concept.
> > > > > 
> > > > 
> > > > Do you like me to re-write the commit message for this patch and put the
> > > > above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
> > > > assign resource routine?
> > > 
> > > I want to understand where the problem is and whether it is right to
> > > define the component as a PCI bridge rather than a host bridge.
> > > 
> > 
> > I will update the commit message (put some of the above reasons in the
> > commit message) and send a new version of this patch if it's acceptable
> > for you.
> > 
> > Thanks
> > > Lorenzo
> > 
> > 
> > 

Re: [PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Honghui Zhang]

On Fri, 2018-10-12 at 09:12 -0500, Bjorn Helgaas wrote:
> On Fri, Oct 12, 2018 at 11:22:30AM +0100, Lorenzo Pieralisi wrote:
> > On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
> >> On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> >>> On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> >>>> On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> >>>>> On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang@mediatek.com wrote:
> >>>>>> From: Honghui Zhang <honghui.zhang@mediatek.com>
> >>>>>> 
> >>>>>> The PCIe controller of MT7622 has TYPE 1 configuration
> >>>>>> space type, but the HW default class type values is
> >>>>>> invalid.
> >>>>>> 
> >>>>>> The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID
> >>>>>> and class type for MT7622") have set the class ID for
> >>>>>> MT7622 as PCI_CLASS_BRIDGE_HOSTe, but it's not workable
> >>>>>> for MT7622:
> >>>>>> 
> >>>>>> In __pci_bus_assign_resources, the framework only setup
> >>>>>> bridge's resource window only if class type is
> >>>>>> PCI_CLASS_BRIDGE_PCI. Or it will leave the subordinary PCIe
> >>>>>> device's MMIO window un-touched.
> 
> I think __pci_bus_assign_resources() should be testing dev->hdr_type
> instead of dev->class.  The connection between "Header Type" and the
> layout of the rest of the header is very explicit (PCI r3.0 sec 6.1,
> PCIe r4.0 sec 7.5.1.1.9), and the reason for the switch statement in
> __pci_bus_assign_resources() is precisely to determine which layout to
> use.
> 
> There are several other uses of dev->class in setup-bus.c that I think
> should also be changed to use dev->hdr_type.
> 
> If we make these changes in setup-bus.c, I suspect the class code you
> assign won't matter too much.  There are a few other tests of the
> class code to figure out whether to leave certain things untouched.
> These seem a little hacky to me, but we're probably stuck with them
> for now, so you should look and see whether they apply to your
> situation.

If these change could be made in the PCI core, then the class code is no
matter what will be workable for MT7622.

As Lorenzo point it out, it's more reasonable for MT7622 to defined as a
PCI-to-PCI class code since the IP is defined as that. I intend to
following Lorenzo's suggest to update the commit message and re-send
this patch set for current solution.

> 
> >>>> And for MT7622, it integrated with block of internal control
> >>>> registers, type 1 configuration space, and is considered as a
> >>>> root complex.
> >>> 
> >>> I assume you mean a type 1 config header here. I do not think it
> >>> is mandatory for a host bridge to have a type 1 config header (and
> >>> related bridge windows + primary/secondary/subordinate bus
> >>> numbers) but I do not know how the IP you are programming is
> >>> designed.
> 
> It is definitely not mandatory for a host bridge to have a type 1
> header.  I'm not even sure that would make sense: the "Primary Bus
> Number" would not apply to a host bridge (since a host bridge's
> primary bus is some sort of CPU bus, not a PCI bus), and a type 1
> device cannot perform address translation between its primary and
> secondary buses, while a host bridge can.
> 
> A Root Port is a type 1 device where the primary bus is logically
> internal to the Root Complex.  A host bridge bridges from the CPU bus
> to that internal bus and might perform address translation.  The Root
> Port must be a PCI device.  A host bridge, being a bridge *to* the PCI
> domain, is not itself generally programmed via PCI config space and
> might not even be visible as a device in PCI config space.
> 
Thanks for the explain. Per my understanding, MT7622 is more like a
complex of Root Port and PCI-to-PCI bridge. It has type 1 header and has
the ability to translate address between its primary and secondary
buses. I guess apply the class type as PCI_CLASS_BRIDGE_PCI is
reasonable way to make its integrated internal bridge workable.

Thanks.
> Bjorn

Re: [PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Honghui Zhang]

On Fri, 2018-10-12 at 09:12 -0500, Bjorn Helgaas wrote:
> On Fri, Oct 12, 2018 at 11:22:30AM +0100, Lorenzo Pieralisi wrote:
> > On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
> >> On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> >>> On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> >>>> On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> >>>>> On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang@mediatek.com wrote:
> >>>>>> From: Honghui Zhang <honghui.zhang@mediatek.com>
> >>>>>> 
> >>>>>> The PCIe controller of MT7622 has TYPE 1 configuration
> >>>>>> space type, but the HW default class type values is
> >>>>>> invalid.
> >>>>>> 
> >>>>>> The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID
> >>>>>> and class type for MT7622") have set the class ID for
> >>>>>> MT7622 as PCI_CLASS_BRIDGE_HOSTe, but it's not workable
> >>>>>> for MT7622:
> >>>>>> 
> >>>>>> In __pci_bus_assign_resources, the framework only setup
> >>>>>> bridge's resource window only if class type is
> >>>>>> PCI_CLASS_BRIDGE_PCI. Or it will leave the subordinary PCIe
> >>>>>> device's MMIO window un-touched.
> 
> I think __pci_bus_assign_resources() should be testing dev->hdr_type
> instead of dev->class.  The connection between "Header Type" and the
> layout of the rest of the header is very explicit (PCI r3.0 sec 6.1,
> PCIe r4.0 sec 7.5.1.1.9), and the reason for the switch statement in
> __pci_bus_assign_resources() is precisely to determine which layout to
> use.
> 
> There are several other uses of dev->class in setup-bus.c that I think
> should also be changed to use dev->hdr_type.
> 
> If we make these changes in setup-bus.c, I suspect the class code you
> assign won't matter too much.  There are a few other tests of the
> class code to figure out whether to leave certain things untouched.
> These seem a little hacky to me, but we're probably stuck with them
> for now, so you should look and see whether they apply to your
> situation.

If these change could be made in the PCI core, then the class code is no
matter what will be workable for MT7622.

As Lorenzo point it out, it's more reasonable for MT7622 to defined as a
PCI-to-PCI class code since the IP is defined as that. I intend to
following Lorenzo's suggest to update the commit message and re-send
this patch set for current solution.

> 
> >>>> And for MT7622, it integrated with block of internal control
> >>>> registers, type 1 configuration space, and is considered as a
> >>>> root complex.
> >>> 
> >>> I assume you mean a type 1 config header here. I do not think it
> >>> is mandatory for a host bridge to have a type 1 config header (and
> >>> related bridge windows + primary/secondary/subordinate bus
> >>> numbers) but I do not know how the IP you are programming is
> >>> designed.
> 
> It is definitely not mandatory for a host bridge to have a type 1
> header.  I'm not even sure that would make sense: the "Primary Bus
> Number" would not apply to a host bridge (since a host bridge's
> primary bus is some sort of CPU bus, not a PCI bus), and a type 1
> device cannot perform address translation between its primary and
> secondary buses, while a host bridge can.
> 
> A Root Port is a type 1 device where the primary bus is logically
> internal to the Root Complex.  A host bridge bridges from the CPU bus
> to that internal bus and might perform address translation.  The Root
> Port must be a PCI device.  A host bridge, being a bridge *to* the PCI
> domain, is not itself generally programmed via PCI config space and
> might not even be visible as a device in PCI config space.
> 
Thanks for the explain. Per my understanding, MT7622 is more like a
complex of Root Port and PCI-to-PCI bridge. It has type 1 header and has
the ability to translate address between its primary and secondary
buses. I guess apply the class type as PCI_CLASS_BRIDGE_PCI is
reasonable way to make its integrated internal bridge workable.

Thanks.
> Bjorn

[PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Honghui Zhang]

On Fri, 2018-10-12 at 09:12 -0500, Bjorn Helgaas wrote:
> On Fri, Oct 12, 2018 at 11:22:30AM +0100, Lorenzo Pieralisi wrote:
> > On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
> >> On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> >>> On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> >>>> On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> >>>>> On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang at mediatek.com wrote:
> >>>>>> From: Honghui Zhang <honghui.zhang@mediatek.com>
> >>>>>> 
> >>>>>> The PCIe controller of MT7622 has TYPE 1 configuration
> >>>>>> space type, but the HW default class type values is
> >>>>>> invalid.
> >>>>>> 
> >>>>>> The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID
> >>>>>> and class type for MT7622") have set the class ID for
> >>>>>> MT7622 as PCI_CLASS_BRIDGE_HOSTe, but it's not workable
> >>>>>> for MT7622:
> >>>>>> 
> >>>>>> In __pci_bus_assign_resources, the framework only setup
> >>>>>> bridge's resource window only if class type is
> >>>>>> PCI_CLASS_BRIDGE_PCI. Or it will leave the subordinary PCIe
> >>>>>> device's MMIO window un-touched.
> 
> I think __pci_bus_assign_resources() should be testing dev->hdr_type
> instead of dev->class.  The connection between "Header Type" and the
> layout of the rest of the header is very explicit (PCI r3.0 sec 6.1,
> PCIe r4.0 sec 7.5.1.1.9), and the reason for the switch statement in
> __pci_bus_assign_resources() is precisely to determine which layout to
> use.
> 
> There are several other uses of dev->class in setup-bus.c that I think
> should also be changed to use dev->hdr_type.
> 
> If we make these changes in setup-bus.c, I suspect the class code you
> assign won't matter too much.  There are a few other tests of the
> class code to figure out whether to leave certain things untouched.
> These seem a little hacky to me, but we're probably stuck with them
> for now, so you should look and see whether they apply to your
> situation.

If these change could be made in the PCI core, then the class code is no
matter what will be workable for MT7622.

As Lorenzo point it out, it's more reasonable for MT7622 to defined as a
PCI-to-PCI class code since the IP is defined as that. I intend to
following Lorenzo's suggest to update the commit message and re-send
this patch set for current solution.

> 
> >>>> And for MT7622, it integrated with block of internal control
> >>>> registers, type 1 configuration space, and is considered as a
> >>>> root complex.
> >>> 
> >>> I assume you mean a type 1 config header here. I do not think it
> >>> is mandatory for a host bridge to have a type 1 config header (and
> >>> related bridge windows + primary/secondary/subordinate bus
> >>> numbers) but I do not know how the IP you are programming is
> >>> designed.
> 
> It is definitely not mandatory for a host bridge to have a type 1
> header.  I'm not even sure that would make sense: the "Primary Bus
> Number" would not apply to a host bridge (since a host bridge's
> primary bus is some sort of CPU bus, not a PCI bus), and a type 1
> device cannot perform address translation between its primary and
> secondary buses, while a host bridge can.
> 
> A Root Port is a type 1 device where the primary bus is logically
> internal to the Root Complex.  A host bridge bridges from the CPU bus
> to that internal bus and might perform address translation.  The Root
> Port must be a PCI device.  A host bridge, being a bridge *to* the PCI
> domain, is not itself generally programmed via PCI config space and
> might not even be visible as a device in PCI config space.
> 
Thanks for the explain. Per my understanding, MT7622 is more like a
complex of Root Port and PCI-to-PCI bridge. It has type 1 header and has
the ability to translate address between its primary and secondary
buses. I guess apply the class type as PCI_CLASS_BRIDGE_PCI is
reasonable way to make its integrated internal bridge workable.

Thanks.
> Bjorn

Re: [PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Bjorn Helgaas]

On Mon, Oct 15, 2018 at 10:42:23AM +0800, Honghui Zhang wrote:
> On Fri, 2018-10-12 at 09:12 -0500, Bjorn Helgaas wrote:
> > On Fri, Oct 12, 2018 at 11:22:30AM +0100, Lorenzo Pieralisi wrote:
> > > On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
> > >> On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> > >>> On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> > >>>> On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > >>>>> On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang@mediatek.com wrote:
> > >>>>>> From: Honghui Zhang <honghui.zhang@mediatek.com>
> > >>>>>> 
> > >>>>>> The PCIe controller of MT7622 has TYPE 1 configuration
> > >>>>>> space type, but the HW default class type values is
> > >>>>>> invalid.
> > >>>>>> 
> > >>>>>> The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID
> > >>>>>> and class type for MT7622") have set the class ID for
> > >>>>>> MT7622 as PCI_CLASS_BRIDGE_HOSTe, but it's not workable
> > >>>>>> for MT7622:
> > >>>>>> 
> > >>>>>> In __pci_bus_assign_resources, the framework only setup
> > >>>>>> bridge's resource window only if class type is
> > >>>>>> PCI_CLASS_BRIDGE_PCI. Or it will leave the subordinary PCIe
> > >>>>>> device's MMIO window un-touched.
> > 
> > I think __pci_bus_assign_resources() should be testing dev->hdr_type
> > instead of dev->class.  The connection between "Header Type" and the
> > layout of the rest of the header is very explicit (PCI r3.0 sec 6.1,
> > PCIe r4.0 sec 7.5.1.1.9), and the reason for the switch statement in
> > __pci_bus_assign_resources() is precisely to determine which layout to
> > use.
> > 
> > There are several other uses of dev->class in setup-bus.c that I think
> > should also be changed to use dev->hdr_type.
> > 
> > If we make these changes in setup-bus.c, I suspect the class code you
> > assign won't matter too much.  There are a few other tests of the
> > class code to figure out whether to leave certain things untouched.
> > These seem a little hacky to me, but we're probably stuck with them
> > for now, so you should look and see whether they apply to your
> > situation.
> 
> If these change could be made in the PCI core, then the class code is no
> matter what will be workable for MT7622.
> 
> As Lorenzo point it out, it's more reasonable for MT7622 to defined as a
> PCI-to-PCI class code since the IP is defined as that. I intend to
> following Lorenzo's suggest to update the commit message and re-send
> this patch set for current solution.
> 
> > >>>> And for MT7622, it integrated with block of internal control
> > >>>> registers, type 1 configuration space, and is considered as a
> > >>>> root complex.
> > >>> 
> > >>> I assume you mean a type 1 config header here. I do not think it
> > >>> is mandatory for a host bridge to have a type 1 config header (and
> > >>> related bridge windows + primary/secondary/subordinate bus
> > >>> numbers) but I do not know how the IP you are programming is
> > >>> designed.
> > 
> > It is definitely not mandatory for a host bridge to have a type 1
> > header.  I'm not even sure that would make sense: the "Primary Bus
> > Number" would not apply to a host bridge (since a host bridge's
> > primary bus is some sort of CPU bus, not a PCI bus), and a type 1
> > device cannot perform address translation between its primary and
> > secondary buses, while a host bridge can.
> > 
> > A Root Port is a type 1 device where the primary bus is logically
> > internal to the Root Complex.  A host bridge bridges from the CPU bus
> > to that internal bus and might perform address translation.  The Root
> > Port must be a PCI device.  A host bridge, being a bridge *to* the PCI
> > domain, is not itself generally programmed via PCI config space and
> > might not even be visible as a device in PCI config space.
> > 
> Thanks for the explain. Per my understanding, MT7622 is more like a
> complex of Root Port and PCI-to-PCI bridge. It has type 1 header and has
> the ability to translate address between its primary and secondary
> buses.

Nope.  Logically speaking, the PCI device in question is a Root Port,
which is a bridge between a primary PCI bus (probably bus 0) that is
internal to the Root Complex, and a secondary PCI bus.  This bridge,
like all other PCI-to-PCI bridges, does no address translation.

The Root Complex *also* contains a host bridge from whatever the
upstream CPU bus is and the logical PCI bus 0 internal to the Root
Complex.  This host bridge can perform address translation.

The Root Port PCI device might also have device-specific PCI config
registers, and those might offer some control over the host bridge
part of the Root Complex.  There is probably an MMIO (non-PCI config
space) way to configure the Root Complex, since you may not be able to
access the PCI config space before the Root Complex is configured.

> I guess apply the class type as PCI_CLASS_BRIDGE_PCI is
> reasonable way to make its integrated internal bridge workable.

Nope, that's not the way this process works.  You proposed setting
the class code as "a way to make this work".  I did a bunch of
research to figure out the best way of solving this and pointed out
a defect in the PCI core.  Fixing that defect will solve your problem
and possibly others.

The correct next step is for you to make that PCI core change, test it
and make sure it works, and post that as part of your series.  If, in
addition, you want to change the class code of your device, you can
also do that, but that's a secondary, optional step as far as I'm
concerned.

Bjorn

[PATCH v6 2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

[Bjorn Helgaas]

On Mon, Oct 15, 2018 at 10:42:23AM +0800, Honghui Zhang wrote:
> On Fri, 2018-10-12 at 09:12 -0500, Bjorn Helgaas wrote:
> > On Fri, Oct 12, 2018 at 11:22:30AM +0100, Lorenzo Pieralisi wrote:
> > > On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
> > >> On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> > >>> On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> > >>>> On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > >>>>> On Mon, Oct 08, 2018 at 11:24:41AM +0800, honghui.zhang at mediatek.com wrote:
> > >>>>>> From: Honghui Zhang <honghui.zhang@mediatek.com>
> > >>>>>> 
> > >>>>>> The PCIe controller of MT7622 has TYPE 1 configuration
> > >>>>>> space type, but the HW default class type values is
> > >>>>>> invalid.
> > >>>>>> 
> > >>>>>> The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID
> > >>>>>> and class type for MT7622") have set the class ID for
> > >>>>>> MT7622 as PCI_CLASS_BRIDGE_HOSTe, but it's not workable
> > >>>>>> for MT7622:
> > >>>>>> 
> > >>>>>> In __pci_bus_assign_resources, the framework only setup
> > >>>>>> bridge's resource window only if class type is
> > >>>>>> PCI_CLASS_BRIDGE_PCI. Or it will leave the subordinary PCIe
> > >>>>>> device's MMIO window un-touched.
> > 
> > I think __pci_bus_assign_resources() should be testing dev->hdr_type
> > instead of dev->class.  The connection between "Header Type" and the
> > layout of the rest of the header is very explicit (PCI r3.0 sec 6.1,
> > PCIe r4.0 sec 7.5.1.1.9), and the reason for the switch statement in
> > __pci_bus_assign_resources() is precisely to determine which layout to
> > use.
> > 
> > There are several other uses of dev->class in setup-bus.c that I think
> > should also be changed to use dev->hdr_type.
> > 
> > If we make these changes in setup-bus.c, I suspect the class code you
> > assign won't matter too much.  There are a few other tests of the
> > class code to figure out whether to leave certain things untouched.
> > These seem a little hacky to me, but we're probably stuck with them
> > for now, so you should look and see whether they apply to your
> > situation.
> 
> If these change could be made in the PCI core, then the class code is no
> matter what will be workable for MT7622.
> 
> As Lorenzo point it out, it's more reasonable for MT7622 to defined as a
> PCI-to-PCI class code since the IP is defined as that. I intend to
> following Lorenzo's suggest to update the commit message and re-send
> this patch set for current solution.
> 
> > >>>> And for MT7622, it integrated with block of internal control
> > >>>> registers, type 1 configuration space, and is considered as a
> > >>>> root complex.
> > >>> 
> > >>> I assume you mean a type 1 config header here. I do not think it
> > >>> is mandatory for a host bridge to have a type 1 config header (and
> > >>> related bridge windows + primary/secondary/subordinate bus
> > >>> numbers) but I do not know how the IP you are programming is
> > >>> designed.
> > 
> > It is definitely not mandatory for a host bridge to have a type 1
> > header.  I'm not even sure that would make sense: the "Primary Bus
> > Number" would not apply to a host bridge (since a host bridge's
> > primary bus is some sort of CPU bus, not a PCI bus), and a type 1
> > device cannot perform address translation between its primary and
> > secondary buses, while a host bridge can.
> > 
> > A Root Port is a type 1 device where the primary bus is logically
> > internal to the Root Complex.  A host bridge bridges from the CPU bus
> > to that internal bus and might perform address translation.  The Root
> > Port must be a PCI device.  A host bridge, being a bridge *to* the PCI
> > domain, is not itself generally programmed via PCI config space and
> > might not even be visible as a device in PCI config space.
> > 
> Thanks for the explain. Per my understanding, MT7622 is more like a
> complex of Root Port and PCI-to-PCI bridge. It has type 1 header and has
> the ability to translate address between its primary and secondary
> buses.

Nope.  Logically speaking, the PCI device in question is a Root Port,
which is a bridge between a primary PCI bus (probably bus 0) that is
internal to the Root Complex, and a secondary PCI bus.  This bridge,
like all other PCI-to-PCI bridges, does no address translation.

The Root Complex *also* contains a host bridge from whatever the
upstream CPU bus is and the logical PCI bus 0 internal to the Root
Complex.  This host bridge can perform address translation.

The Root Port PCI device might also have device-specific PCI config
registers, and those might offer some control over the host bridge
part of the Root Complex.  There is probably an MMIO (non-PCI config
space) way to configure the Root Complex, since you may not be able to
access the PCI config space before the Root Complex is configured.

> I guess apply the class type as PCI_CLASS_BRIDGE_PCI is
> reasonable way to make its integrated internal bridge workable.

Nope, that's not the way this process works.  You proposed setting
the class code as "a way to make this work".  I did a bunch of
research to figure out the best way of solving this and pointed out
a defect in the PCI core.  Fixing that defect will solve your problem
and possibly others.

The correct next step is for you to make that PCI core change, test it
and make sure it works, and post that as part of your series.  If, in
addition, you want to change the class code of your device, you can
also do that, but that's a secondary, optional step as far as I'm
concerned.

Bjorn