From: Honghui Zhang <[email protected]>
This patchset includes misc patchs:
The first patch fixup the mtk_pcie_find_port logical which will cause system
could not touch the EP's configuration space which was connected to PCIe slot 1.
The second patch fixup the enable msi logical, the operation to enable msi
should be after system clock is enabled. The function of mtk_pcie_startup_port_v2's
define location is re-arranged to avoid mtk_pcie_enable_msi's forward declaration.
And call mtk_pcie_enable_msi in mtk_pcie_startup_port_v2 since the clock was all
enabled at that time.
The third patch 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.
-Rebase on the previous two patches.
The fourth patch add loadable kernel module support.
Those patches was already reviewed-by Ryder Lee <[email protected]> so
I just add the Reviewed-by tags in those patches.
[1] https://patchwork.kernel.org/patch/10479079
Honghui Zhang (4):
PCI: mediatek: fixup mtk_pcie_find_port logical
PCI: mediatek: enable msi after clock enabled
PCI: mediatek: Add system pm support for MT2712 and MT7622
PCI: mediatek: Add loadable kernel module support
drivers/pci/controller/Kconfig | 2 +-
drivers/pci/controller/pcie-mediatek.c | 291 ++++++++++++++++++++++++---------
2 files changed, 215 insertions(+), 78 deletions(-)
--
2.6.4
From: Honghui Zhang <[email protected]>
Implement remove callback function for Mediatek PCIe driver to add
loadable kernel module support.
Signed-off-by: Honghui Zhang <[email protected]>
Reviewed-by: Ryder Lee <[email protected]>
---
drivers/pci/controller/Kconfig | 2 +-
drivers/pci/controller/pcie-mediatek.c | 63 +++++++++++++++++++++++++++++++---
2 files changed, 60 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 18fa09b..6c61ac65 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -234,7 +234,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 cac01ab..5830cea 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>
@@ -184,6 +185,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;
@@ -536,6 +538,30 @@ 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;
+
+ if (list_empty(&pcie->ports))
+ return;
+
+ 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)
{
@@ -626,7 +652,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) {
@@ -634,8 +660,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;
}
@@ -1197,6 +1224,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);
+
+ if (!list_empty(&pcie->ports))
+ mtk_pcie_put_resources(pcie);
+
+ return 0;
+}
+
#ifdef CONFIG_PM_SLEEP
static int mtk_pcie_suspend_noirq(struct device *dev)
{
@@ -1290,6 +1343,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,
@@ -1297,4 +1351,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
From: Honghui Zhang <[email protected]>
Mediatek's host controller have two slots, each have 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
While PCI emulation, 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 logical by using the slot's
devfn for match.
Signed-off-by: Honghui Zhang <[email protected]>
Reviewed-by: Ryder Lee <[email protected]>
---
drivers/pci/controller/pcie-mediatek.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 0baabe3..9cf7ecf 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -337,10 +337,23 @@ 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;
+ struct pci_bus *pbus;
- list_for_each_entry(port, &pcie->ports, list)
- if (port->slot == PCI_SLOT(devfn))
+ list_for_each_entry(port, &pcie->ports, list) {
+ if (bus->number == 0 && port->slot == PCI_SLOT(devfn)) {
return port;
+ } else if (bus->number != 0) {
+ pbus = bus;
+ do {
+ dev = pbus->self;
+ if (port->slot == PCI_SLOT(dev->devfn))
+ return port;
+
+ pbus = dev->bus;
+ } while (dev->bus->number != 0);
+ }
+ }
return NULL;
}
--
2.6.4
From: Honghui Zhang <[email protected]>
The 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.
The function of mtk_pcie_startup_port_v2's define location is
re-arranged to avoid mtk_pcie_enable_msi's forward declaration.
Signed-off-by: Honghui Zhang <[email protected]>
Reviewed-by: Ryder Lee <[email protected]>
---
drivers/pci/controller/pcie-mediatek.c | 143 +++++++++++++++++----------------
1 file changed, 72 insertions(+), 71 deletions(-)
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 9cf7ecf..17bf99b 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -396,75 +396,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_HOST;
- 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);
@@ -641,8 +572,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;
@@ -709,6 +638,78 @@ 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_HOST;
+ 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);
+
+ 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));
+ 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
From: Honghui Zhang <[email protected]>
The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
suspend, and all the internal control register will be reset after system
resume. The PCIe link should be re-established and the related control
register values should be re-set after system resume.
Signed-off-by: Honghui Zhang <[email protected]>
Reviewed-by: Ryder Lee <[email protected]>
---
drivers/pci/controller/pcie-mediatek.c | 68 ++++++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 17bf99b..cac01ab 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -134,12 +134,14 @@ struct mtk_pcie_port;
/**
* struct mtk_pcie_soc - differentiate between host generations
* @need_fix_class_id: whether this host's class ID needed to be fixed or not
+ * @pm_support: whether the host's MTCMOS will be off when suspend
* @ops: pointer to configuration access functions
* @startup: pointer to controller setting functions
* @setup_irq: pointer to initialize IRQ functions
*/
struct mtk_pcie_soc {
bool need_fix_class_id;
+ bool pm_support;
struct pci_ops *ops;
int (*startup)(struct mtk_pcie_port *port);
int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
@@ -1195,12 +1197,76 @@ static int mtk_pcie_probe(struct platform_device *pdev)
return err;
}
+#ifdef CONFIG_PM_SLEEP
+static int mtk_pcie_suspend_noirq(struct device *dev)
+{
+ struct mtk_pcie *pcie = dev_get_drvdata(dev);
+ const struct mtk_pcie_soc *soc = pcie->soc;
+ struct mtk_pcie_port *port;
+
+ if (!soc->pm_support)
+ return 0;
+
+ 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);
+ }
+
+ mtk_pcie_subsys_powerdown(pcie);
+
+ return 0;
+}
+
+static int mtk_pcie_resume_noirq(struct device *dev)
+{
+ struct mtk_pcie *pcie = dev_get_drvdata(dev);
+ const struct mtk_pcie_soc *soc = pcie->soc;
+ struct mtk_pcie_port *port;
+
+ if (!soc->pm_support)
+ return 0;
+
+ if (list_empty(&pcie->ports))
+ return 0;
+
+ if (dev->pm_domain) {
+ pm_runtime_enable(dev);
+ pm_runtime_get_sync(dev);
+ }
+
+ clk_prepare_enable(pcie->free_ck);
+
+ list_for_each_entry(port, &pcie->ports, list)
+ mtk_pcie_enable_port(port);
+
+ if (list_empty(&pcie->ports))
+ mtk_pcie_subsys_powerdown(pcie);
+
+ return 0;
+}
+#endif
+
+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,
};
static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
+ .pm_support = true,
.ops = &mtk_pcie_ops_v2,
.startup = mtk_pcie_startup_port_v2,
.setup_irq = mtk_pcie_setup_irq,
@@ -1208,6 +1274,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
.need_fix_class_id = true,
+ .pm_support = true,
.ops = &mtk_pcie_ops_v2,
.startup = mtk_pcie_startup_port_v2,
.setup_irq = mtk_pcie_setup_irq,
@@ -1227,6 +1294,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
On Wed, Jun 27, 2018 at 12:21 PM, <[email protected]> wrote:
> From: Honghui Zhang <[email protected]>
>
> Mediatek's host controller have two slots, each have 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
>
> While PCI emulation, 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 logical by using the slot's
> devfn for match.
> + list_for_each_entry(port, &pcie->ports, list) {
> + if (bus->number == 0 && port->slot == PCI_SLOT(devfn)) {
> return port;
> + } else if (bus->number != 0) {
You can do it like (no need 'else')
if (...)
return ...;
if (bus->number) {
...
}
> + pbus = bus;
> + do {
> + dev = pbus->self;
> + if (port->slot == PCI_SLOT(dev->devfn))
> + return port;
> +
> + pbus = dev->bus;
> + } while (dev->bus->number != 0);
This can be rewritten like
pbus = bus;
while (pbus->number) {
dev = ...;
...
pbus = dev->bus;
}
and no need for if (bus->number) anymore.
> + }
> + }
>
> return NULL;
> }
> --
> 2.6.4
>
--
With Best Regards,
Andy Shevchenko
On Wed, Jun 27, 2018 at 12:21 PM, <[email protected]> wrote:
> From: Honghui Zhang <[email protected]>
>
> Implement remove callback function for Mediatek PCIe driver to add
> loadable kernel module support.
> +static void mtk_pcie_irq_teardown(struct mtk_pcie *pcie)
> +{
> + struct mtk_pcie_port *port, *tmp;
> +
> + if (list_empty(&pcie->ports))
> + return;
This is redundant.
> +
> + 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 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);
> +
> + if (!list_empty(&pcie->ports))
I think this is also not needed. Do you put runtime PM by the way?
> + mtk_pcie_put_resources(pcie);
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
On Wed, Jun 27, 2018 at 12:21 PM, <[email protected]> wrote:
> From: Honghui Zhang <[email protected]>
>
> The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
> suspend, and all the internal control register will be reset after system
> resume. The PCIe link should be re-established and the related control
> register values should be re-set after system resume.
> struct mtk_pcie_soc {
> bool need_fix_class_id;
> + bool pm_support;
Hmm... Do you really need this flag? Can't runtime PM API tell you this?
> struct pci_ops *ops;
> int (*startup)(struct mtk_pcie_port *port);
> int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> @@ -1195,12 +1197,76 @@ static int mtk_pcie_probe(struct platform_device *pdev)
> return err;
> }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int mtk_pcie_suspend_noirq(struct device *dev)
Perhaps __maybe_unused?
> +{
> + struct mtk_pcie *pcie = dev_get_drvdata(dev);
> + const struct mtk_pcie_soc *soc = pcie->soc;
> + struct mtk_pcie_port *port;
> +
> + if (!soc->pm_support)
> + return 0;
> +
> + if (list_empty(&pcie->ports))
> + return 0;
So, if the list is empty you are not suspending for the host?
> +
> + 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);
> + }
> +
> + mtk_pcie_subsys_powerdown(pcie);
> +
> + return 0;
> +}
> +
> +static int mtk_pcie_resume_noirq(struct device *dev)
> +{
> + struct mtk_pcie *pcie = dev_get_drvdata(dev);
> + const struct mtk_pcie_soc *soc = pcie->soc;
> + struct mtk_pcie_port *port;
> +
> + if (!soc->pm_support)
> + return 0;
> +
> + if (list_empty(&pcie->ports))
> + return 0;
No runtime PM for this case?
(It seems now I understand why in previous patch you have a similar check)
> +
> + if (dev->pm_domain) {
> + pm_runtime_enable(dev);
> + pm_runtime_get_sync(dev);
> + }
> +
> + clk_prepare_enable(pcie->free_ck);
> +
> + list_for_each_entry(port, &pcie->ports, list)
> + mtk_pcie_enable_port(port);
> +
> + if (list_empty(&pcie->ports))
Hmm... How it would be true if you already bailed out above on the
same condition?
> + mtk_pcie_subsys_powerdown(pcie);
> +
> + return 0;
> +}
> +#endif
> +
> +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,
> };
>
> static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> + .pm_support = true,
> .ops = &mtk_pcie_ops_v2,
> .startup = mtk_pcie_startup_port_v2,
> .setup_irq = mtk_pcie_setup_irq,
No update for .pm ?
> @@ -1208,6 +1274,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
>
> static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
> .need_fix_class_id = true,
> + .pm_support = true,
> .ops = &mtk_pcie_ops_v2,
> .startup = mtk_pcie_startup_port_v2,
> .setup_irq = mtk_pcie_setup_irq,
> @@ -1227,6 +1294,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
>
--
With Best Regards,
Andy Shevchenko
On Wed, 2018-06-27 at 19:35 +0300, Andy Shevchenko wrote:
> On Wed, Jun 27, 2018 at 12:21 PM, <[email protected]> wrote:
> > From: Honghui Zhang <[email protected]>
> >
> > Mediatek's host controller have two slots, each have 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
> >
> > While PCI emulation, 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 logical by using the slot's
> > devfn for match.
>
>
> > + list_for_each_entry(port, &pcie->ports, list) {
> > + if (bus->number == 0 && port->slot == PCI_SLOT(devfn)) {
> > return port;
> > + } else if (bus->number != 0) {
>
> You can do it like (no need 'else')
>
> if (...)
> return ...;
> if (bus->number) {
> ...
> }
>
> > + pbus = bus;
> > + do {
> > + dev = pbus->self;
> > + if (port->slot == PCI_SLOT(dev->devfn))
> > + return port;
> > +
> > + pbus = dev->bus;
> > + } while (dev->bus->number != 0);
>
> This can be rewritten like
>
> pbus = bus;
> while (pbus->number) {
> dev = ...;
> ...
> pbus = dev->bus;
> }
>
> and no need for if (bus->number) anymore.
>
Hi, Andy, thanks very much for your advise, I will change it in the next
version.
thanks.
> > + }
> > + }
> >
> > return NULL;
> > }
> > --
> > 2.6.4
> >
>
>
>
On Wed, 2018-06-27 at 19:39 +0300, Andy Shevchenko wrote:
> On Wed, Jun 27, 2018 at 12:21 PM, <[email protected]> wrote:
> > From: Honghui Zhang <[email protected]>
> >
> > Implement remove callback function for Mediatek PCIe driver to add
> > loadable kernel module support.
>
> > +static void mtk_pcie_irq_teardown(struct mtk_pcie *pcie)
> > +{
> > + struct mtk_pcie_port *port, *tmp;
> > +
>
> > + if (list_empty(&pcie->ports))
> > + return;
>
> This is redundant.
Yeah, you are right, I will remove it.
>
> > +
> > + 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 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);
> > +
>
> > + if (!list_empty(&pcie->ports))
>
> I think this is also not needed. Do you put runtime PM by the way?
Yes, it's could be removed.
the pm was putted in mtk_pcie_put_resources which will call
mtk_pcie_subsys_powerdown, and the pm was putted in that function.
thanks.
>
> > + mtk_pcie_put_resources(pcie);
> > +
> > + return 0;
> > +}
>
On Wed, 2018-06-27 at 19:45 +0300, Andy Shevchenko wrote:
> On Wed, Jun 27, 2018 at 12:21 PM, <[email protected]> wrote:
> > From: Honghui Zhang <[email protected]>
> >
> > The MTCMOS of PCIe Host for MT2712 and MT7622 will be off when system
> > suspend, and all the internal control register will be reset after system
> > resume. The PCIe link should be re-established and the related control
> > register values should be re-set after system resume.
>
> > struct mtk_pcie_soc {
> > bool need_fix_class_id;
>
> > + bool pm_support;
>
> Hmm... Do you really need this flag? Can't runtime PM API tell you this?
This host driver is both for MT2712, MT7622 and MT7623, but MT7623 do
not this this patch.
I only add this flag to identify whether we need to do the PM operation
for this SoC since I do not want to touch anything for MT7623.
>
> > struct pci_ops *ops;
> > int (*startup)(struct mtk_pcie_port *port);
> > int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node);
> > @@ -1195,12 +1197,76 @@ static int mtk_pcie_probe(struct platform_device *pdev)
> > return err;
> > }
> >
> > +#ifdef CONFIG_PM_SLEEP
> > +static int mtk_pcie_suspend_noirq(struct device *dev)
>
> Perhaps __maybe_unused?
Ok, I'm a bit confused about this, if there's some discussion about
this, do you have the link of those discussion and kindly point put it
in this thread? Or I guess I can change this back to __maybe_unused if
there's no other comments about this.
>
> > +{
> > + struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > + const struct mtk_pcie_soc *soc = pcie->soc;
> > + struct mtk_pcie_port *port;
> > +
> > + if (!soc->pm_support)
> > + return 0;
> > +
>
> > + if (list_empty(&pcie->ports))
> > + return 0;
>
> So, if the list is empty you are not suspending for the host?
>
if the list was empty then it indicate that all the PCIe slot was not
connected with any EP device, and all the resource have been release in
probe time. So the host driver does not need to save or restore
anything.
>
> > +
> > + 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);
> > + }
> > +
> > + mtk_pcie_subsys_powerdown(pcie);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_pcie_resume_noirq(struct device *dev)
> > +{
> > + struct mtk_pcie *pcie = dev_get_drvdata(dev);
> > + const struct mtk_pcie_soc *soc = pcie->soc;
> > + struct mtk_pcie_port *port;
> > +
> > + if (!soc->pm_support)
> > + return 0;
> > +
>
> > + if (list_empty(&pcie->ports))
> > + return 0;
>
> No runtime PM for this case?
> (It seems now I understand why in previous patch you have a similar check)
>
I guess host driver does not need to resume anything if there's no EP
device was connected.
> > +
> > + if (dev->pm_domain) {
> > + pm_runtime_enable(dev);
> > + pm_runtime_get_sync(dev);
> > + }
> > +
> > + clk_prepare_enable(pcie->free_ck);
> > +
> > + list_for_each_entry(port, &pcie->ports, list)
> > + mtk_pcie_enable_port(port);
> > +
>
> > + if (list_empty(&pcie->ports))
>
> Hmm... How it would be true if you already bailed out above on the
> same condition?
Assuming that there's EP device connected before suspend, and the EP was
removed from the link while system suspend. It means that when enter
this function the list is not empty, but after the function of
mtk_pcie_enable_port was called, host driver found there's no EP device
was connected anymore, it will free this list and some other resources
in mtk_pcie_enable_port,(after mtk_pcie_enable_port was called, the list
is empty in this scenario) but leave the subsys power along, I guess we
need to take care of this scenario.
>
> > + mtk_pcie_subsys_powerdown(pcie);
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +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,
> > };
> >
> > static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> > + .pm_support = true,
> > .ops = &mtk_pcie_ops_v2,
> > .startup = mtk_pcie_startup_port_v2,
> > .setup_irq = mtk_pcie_setup_irq,
>
> No update for .pm ?
I'm not get your point, I update the .pm callbacks in the
platform_driver struct, this SoC data just store the different
information for SoCs. Did I missed something?
>
> > @@ -1208,6 +1274,7 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt2712 = {
> >
> > static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = {
> > .need_fix_class_id = true,
> > + .pm_support = true,
> > .ops = &mtk_pcie_ops_v2,
> > .startup = mtk_pcie_startup_port_v2,
> > .setup_irq = mtk_pcie_setup_irq,
> > @@ -1227,6 +1294,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
> >
>
>
>
On Wed, Jun 27, 2018 at 05:21:35PM +0800, [email protected] wrote:
> From: Honghui Zhang <[email protected]>
>
> Mediatek's host controller have two slots, each have 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.
The Mediatek host controller has two slots, each with its own control
registers.
> 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
>
> While PCI emulation, system software will scan all the PCI device
s/While PCI emulation/During PCI enumeration/
> 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 logical by using the slot's
> devfn for match.
>
> Signed-off-by: Honghui Zhang <[email protected]>
> Reviewed-by: Ryder Lee <[email protected]>
> ---
> drivers/pci/controller/pcie-mediatek.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 0baabe3..9cf7ecf 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -337,10 +337,23 @@ 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;
> + struct pci_bus *pbus;
>
> - list_for_each_entry(port, &pcie->ports, list)
> - if (port->slot == PCI_SLOT(devfn))
> + list_for_each_entry(port, &pcie->ports, list) {
mvebu has the identical hardware structure but uses an array instead
of a list:
num = of_get_available_child_count(np);
pcie->ports = devm_kcalloc(dev, num, sizeof(*pcie->ports), GFP_KERNEL);
for_each_available_child_of_node(np, child) {
struct mvebu_pcie_port *port = &pcie->ports[i];
mvebu_pcie_parse_port(pcie, port, child);
}
It would be nice if mvebu and mtk used the same strategy so the code
looks the same.
> + if (bus->number == 0 && port->slot == PCI_SLOT(devfn)) {
Is the root bus number fixed at 0 or is it programmable? Many drivers
do something like this:
if (bus->number == pcie->root_bus_nr)
to handle the case of the root bus number being programmable.
> return port;
> + } else if (bus->number != 0) {
> + pbus = bus;
> + do {
> + dev = pbus->self;
> + if (port->slot == PCI_SLOT(dev->devfn))
> + return port;
> +
> + pbus = dev->bus;
> + } while (dev->bus->number != 0);
You should not need to search up the tree of dev->bus->self.
mvebu_pcie_find_port() checks the root port secondary and subordinate
bus numbers, which should work here, too.
> + }
> + }
>
> return NULL;
> }
> --
> 2.6.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, 2018-06-28 at 08:07 -0500, Bjorn Helgaas wrote:
> On Wed, Jun 27, 2018 at 05:21:35PM +0800, [email protected] wrote:
> > From: Honghui Zhang <[email protected]>
> >
> > Mediatek's host controller have two slots, each have 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.
>
> The Mediatek host controller has two slots, each with its own control
> registers.
Thanks, I will update it in the next version.
>
> > 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
> >
> > While PCI emulation, system software will scan all the PCI device
>
> s/While PCI emulation/During PCI enumeration/
Thanks, will update this.
>
> > 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 logical by using the slot's
> > devfn for match.
> >
> > Signed-off-by: Honghui Zhang <[email protected]>
> > Reviewed-by: Ryder Lee <[email protected]>
> > ---
> > drivers/pci/controller/pcie-mediatek.c | 17 +++++++++++++++--
> > 1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > index 0baabe3..9cf7ecf 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -337,10 +337,23 @@ 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;
> > + struct pci_bus *pbus;
> >
> > - list_for_each_entry(port, &pcie->ports, list)
> > - if (port->slot == PCI_SLOT(devfn))
> > + list_for_each_entry(port, &pcie->ports, list) {
>
> mvebu has the identical hardware structure but uses an array instead
> of a list:
>
> num = of_get_available_child_count(np);
> pcie->ports = devm_kcalloc(dev, num, sizeof(*pcie->ports), GFP_KERNEL);
> for_each_available_child_of_node(np, child) {
> struct mvebu_pcie_port *port = &pcie->ports[i];
> mvebu_pcie_parse_port(pcie, port, child);
> }
>
> It would be nice if mvebu and mtk used the same strategy so the code
> looks the same.
Seems there's different approach existing in current host controller
driver for this, tegra PCIe is using list too.
I guess I can change it to using array after this problem is fixed if
you have strong opinion here.
>
> > + if (bus->number == 0 && port->slot == PCI_SLOT(devfn)) {
>
> Is the root bus number fixed at 0 or is it programmable? Many drivers
> do something like this:
The primary bus number is fixed at 0.(technically speaking, hardware
will capture the very first value of primary bus number, once it's been
captured, change the value in configuration space will not take effort.
But the hardware default value is 0, the hardware will not response to
other values except 0 at the first time. And the only way to touch the
value in it's configuration space is using primary bus number 0. So it
could be taken as fixed at 0.)
>
> if (bus->number == pcie->root_bus_nr)
>
> to handle the case of the root bus number being programmable.
>
> > return port;
> > + } else if (bus->number != 0) {
> > + pbus = bus;
> > + do {
> > + dev = pbus->self;
> > + if (port->slot == PCI_SLOT(dev->devfn))
> > + return port;
> > +
> > + pbus = dev->bus;
> > + } while (dev->bus->number != 0);
>
> You should not need to search up the tree of dev->bus->self.
>
> mvebu_pcie_find_port() checks the root port secondary and subordinate
> bus numbers, which should work here, too.
>
I checked mvebu's host driver, it using software bridge to abstract a
pseudo bridge. And access to this pseudo bridge's configuration space is
all software behavior.
If I following mvebu's way, I need to touch the hardware to read back
the secondary and subordinary bus number, I think current software
solution is more efficient.
Or I could add per port parameter to store the bus_number, and update
the value as mvebu's driver is doing. Do you think it's better than
current solution?
> > + }
> > + }
> >
> > return NULL;
> > }
> > --
> > 2.6.4
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel