Currently we are handling pcie wake in mrvl wifi driver. But Brian
suggests to move it into rockchip pcie driver.
Tested on my chromebook bob(with cros 4.4 kernel and mrvl wifi).
Changes in v4:
Rebase on newest for-next branch, also fix error handling by:
1e7f570a1b86 PCI: rockchip: Idle inactive PHY(s)
Changes in v3:
Fix error handling
Changes in v2:
Use dev_pm_set_dedicated_wake_irq
-- Suggested by Brian Norris <[email protected]>
Jeffy Chen (4):
PCI: rockchip: Fix error handlings
PCI: rockchip: Add support for pcie wake irq
dt-bindings: PCI: rockchip: Add support for pcie wake irq
arm64: dts: rockchip: Handle pcie wake in pcie driver for Gru
.../devicetree/bindings/pci/rockchip-pcie.txt | 20 ++-
arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 15 +-
drivers/pci/host/pcie-rockchip.c | 179 ++++++++++++---------
3 files changed, 126 insertions(+), 88 deletions(-)
--
2.11.0
Fix error handlings in probe & resume.
Signed-off-by: Jeffy Chen <[email protected]>
---
Changes in v4:
Rebase on newest for-next branch, also fix error handling by:
1e7f570a1b86 PCI: rockchip: Idle inactive PHY(s)
Changes in v3: None
Changes in v2: None
drivers/pci/host/pcie-rockchip.c | 160 +++++++++++++++++++++------------------
1 file changed, 87 insertions(+), 73 deletions(-)
diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 2eccd532c256..5d85ec2e2fb0 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -561,32 +561,32 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
err = phy_init(rockchip->phys[i]);
if (err) {
dev_err(dev, "init phy%d err %d\n", i, err);
- return err;
+ goto err_exit_phy;
}
}
err = reset_control_assert(rockchip->core_rst);
if (err) {
dev_err(dev, "assert core_rst err %d\n", err);
- return err;
+ goto err_exit_phy;
}
err = reset_control_assert(rockchip->mgmt_rst);
if (err) {
dev_err(dev, "assert mgmt_rst err %d\n", err);
- return err;
+ goto err_exit_phy;
}
err = reset_control_assert(rockchip->mgmt_sticky_rst);
if (err) {
dev_err(dev, "assert mgmt_sticky_rst err %d\n", err);
- return err;
+ goto err_exit_phy;
}
err = reset_control_assert(rockchip->pipe_rst);
if (err) {
dev_err(dev, "assert pipe_rst err %d\n", err);
- return err;
+ goto err_exit_phy;
}
udelay(10);
@@ -594,19 +594,19 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
err = reset_control_deassert(rockchip->pm_rst);
if (err) {
dev_err(dev, "deassert pm_rst err %d\n", err);
- return err;
+ goto err_exit_phy;
}
err = reset_control_deassert(rockchip->aclk_rst);
if (err) {
dev_err(dev, "deassert aclk_rst err %d\n", err);
- return err;
+ goto err_exit_phy;
}
err = reset_control_deassert(rockchip->pclk_rst);
if (err) {
dev_err(dev, "deassert pclk_rst err %d\n", err);
- return err;
+ goto err_exit_phy;
}
if (rockchip->link_gen == 2)
@@ -628,7 +628,7 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
err = phy_power_on(rockchip->phys[i]);
if (err) {
dev_err(dev, "power on phy%d err %d\n", i, err);
- return err;
+ goto err_power_off_phy;
}
}
@@ -639,25 +639,25 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
err = reset_control_deassert(rockchip->mgmt_sticky_rst);
if (err) {
dev_err(dev, "deassert mgmt_sticky_rst err %d\n", err);
- return err;
+ goto err_power_off_phy;
}
err = reset_control_deassert(rockchip->core_rst);
if (err) {
dev_err(dev, "deassert core_rst err %d\n", err);
- return err;
+ goto err_power_off_phy;
}
err = reset_control_deassert(rockchip->mgmt_rst);
if (err) {
dev_err(dev, "deassert mgmt_rst err %d\n", err);
- return err;
+ goto err_power_off_phy;
}
err = reset_control_deassert(rockchip->pipe_rst);
if (err) {
dev_err(dev, "deassert pipe_rst err %d\n", err);
- return err;
+ goto err_power_off_phy;
}
/* Fix the transmitted FTS count desired to exit from L0s. */
@@ -690,7 +690,7 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
500 * USEC_PER_MSEC);
if (err) {
dev_err(dev, "PCIe link training gen1 timeout!\n");
- return -ETIMEDOUT;
+ goto err_power_off_phy;
}
if (rockchip->link_gen == 2) {
@@ -751,6 +751,26 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DCSR);
return 0;
+err_power_off_phy:
+ while (i--)
+ phy_power_off(rockchip->phys[i]);
+ i = MAX_LANE_NUM;
+err_exit_phy:
+ while (i--)
+ phy_exit(rockchip->phys[i]);
+ return err;
+}
+
+static void rockchip_pcie_deinit_phys(struct rockchip_pcie *rockchip)
+{
+ int i;
+
+ for (i = 0; i < MAX_LANE_NUM; i++) {
+ /* inactive lane is already powered off */
+ if (rockchip->legacy_phy || rockchip->lanes_map & BIT(i))
+ phy_power_off(rockchip->phys[i]);
+ phy_exit(rockchip->phys[i]);
+ }
}
static irqreturn_t rockchip_pcie_subsys_irq_handler(int irq, void *arg)
@@ -1131,7 +1151,7 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
err = regulator_enable(rockchip->vpcie12v);
if (err) {
dev_err(dev, "fail to enable vpcie12v regulator\n");
- goto err_out;
+ return err;
}
}
@@ -1160,7 +1180,6 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
}
return 0;
-
err_disable_1v8:
if (!IS_ERR(rockchip->vpcie1v8))
regulator_disable(rockchip->vpcie1v8);
@@ -1170,7 +1189,6 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
err_disable_12v:
if (!IS_ERR(rockchip->vpcie12v))
regulator_disable(rockchip->vpcie12v);
-err_out:
return err;
}
@@ -1364,7 +1382,7 @@ static int rockchip_pcie_wait_l2(struct rockchip_pcie *rockchip)
static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
{
struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
- int ret, i;
+ int ret;
/* disable core and cli int since we don't need to ack PME_ACK */
rockchip_pcie_write(rockchip, (PCIE_CLIENT_INT_CLI << 16) |
@@ -1377,12 +1395,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
return ret;
}
- for (i = 0; i < MAX_LANE_NUM; i++) {
- /* inactive lane is already powered off */
- if (rockchip->lanes_map & BIT(i))
- phy_power_off(rockchip->phys[i]);
- phy_exit(rockchip->phys[i]);
- }
+ rockchip_pcie_deinit_phys(rockchip);
clk_disable_unprepare(rockchip->clk_pcie_pm);
clk_disable_unprepare(rockchip->hclk_pcie);
@@ -1391,7 +1404,6 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
if (!IS_ERR(rockchip->vpcie0v9))
regulator_disable(rockchip->vpcie0v9);
-
return ret;
}
@@ -1410,43 +1422,46 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
err = clk_prepare_enable(rockchip->clk_pcie_pm);
if (err)
- goto err_pcie_pm;
+ goto err_disable_0v9;
err = clk_prepare_enable(rockchip->hclk_pcie);
if (err)
- goto err_hclk_pcie;
+ goto err_disable_clk_pcie_pm;
err = clk_prepare_enable(rockchip->aclk_perf_pcie);
if (err)
- goto err_aclk_perf_pcie;
+ goto err_disable_hclk_pcie;
err = clk_prepare_enable(rockchip->aclk_pcie);
if (err)
- goto err_aclk_pcie;
+ goto err_disable_aclk_perf_pcie;
err = rockchip_pcie_init_port(rockchip);
if (err)
- goto err_pcie_resume;
+ goto err_disable_aclk_pcie;
err = rockchip_pcie_cfg_atu(rockchip);
if (err)
- goto err_pcie_resume;
+ goto err_deinit_port;
/* Need this to enter L1 again */
rockchip_pcie_update_txcredit_mui(rockchip);
rockchip_pcie_enable_interrupts(rockchip);
return 0;
-
-err_pcie_resume:
+err_deinit_port:
+ rockchip_pcie_deinit_phys(rockchip);
+err_disable_aclk_pcie:
clk_disable_unprepare(rockchip->aclk_pcie);
-err_aclk_pcie:
+err_disable_aclk_perf_pcie:
clk_disable_unprepare(rockchip->aclk_perf_pcie);
-err_aclk_perf_pcie:
+err_disable_hclk_pcie:
clk_disable_unprepare(rockchip->hclk_pcie);
-err_hclk_pcie:
+err_disable_clk_pcie_pm:
clk_disable_unprepare(rockchip->clk_pcie_pm);
-err_pcie_pm:
+err_disable_0v9:
+ if (!IS_ERR(rockchip->vpcie0v9))
+ regulator_disable(rockchip->vpcie0v9);
return err;
}
@@ -1483,47 +1498,47 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
err = clk_prepare_enable(rockchip->aclk_pcie);
if (err) {
dev_err(dev, "unable to enable aclk_pcie clock\n");
- goto err_aclk_pcie;
+ return err;
}
err = clk_prepare_enable(rockchip->aclk_perf_pcie);
if (err) {
dev_err(dev, "unable to enable aclk_perf_pcie clock\n");
- goto err_aclk_perf_pcie;
+ goto err_disable_aclk_pcie;
}
err = clk_prepare_enable(rockchip->hclk_pcie);
if (err) {
dev_err(dev, "unable to enable hclk_pcie clock\n");
- goto err_hclk_pcie;
+ goto err_disable_aclk_perf_pcie;
}
err = clk_prepare_enable(rockchip->clk_pcie_pm);
if (err) {
dev_err(dev, "unable to enable hclk_pcie clock\n");
- goto err_pcie_pm;
+ goto err_disable_hclk_pcie;
}
err = rockchip_pcie_set_vpcie(rockchip);
if (err) {
dev_err(dev, "failed to set vpcie regulator\n");
- goto err_set_vpcie;
+ goto err_disable_clk_pcie_pm;
}
err = rockchip_pcie_init_port(rockchip);
if (err)
- goto err_vpcie;
+ goto err_disable_vpcie;
rockchip_pcie_enable_interrupts(rockchip);
err = rockchip_pcie_init_irq_domain(rockchip);
if (err < 0)
- goto err_vpcie;
+ goto err_deinit_port;
err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff,
&res, &io_base);
if (err)
- goto err_vpcie;
+ goto err_remove_irq_domain;
err = devm_request_pci_bus_resources(dev, &res);
if (err)
@@ -1561,12 +1576,12 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
err = rockchip_pcie_cfg_atu(rockchip);
if (err)
- goto err_free_res;
+ goto err_unmap_iospace;
rockchip->msg_region = devm_ioremap(dev, rockchip->msg_bus_addr, SZ_1M);
if (!rockchip->msg_region) {
err = -ENOMEM;
- goto err_free_res;
+ goto err_unmap_iospace;
}
list_splice_init(&res, &bridge->windows);
@@ -1591,28 +1606,33 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
pcie_bus_configure_settings(child);
pci_bus_add_devices(bus);
- return 0;
+ return 0;
+err_unmap_iospace:
+ pci_unmap_iospace(rockchip->io);
err_free_res:
pci_free_resource_list(&res);
-err_vpcie:
- if (!IS_ERR(rockchip->vpcie12v))
- regulator_disable(rockchip->vpcie12v);
- if (!IS_ERR(rockchip->vpcie3v3))
- regulator_disable(rockchip->vpcie3v3);
- if (!IS_ERR(rockchip->vpcie1v8))
- regulator_disable(rockchip->vpcie1v8);
+err_remove_irq_domain:
+ irq_domain_remove(rockchip->irq_domain);
+err_deinit_port:
+ rockchip_pcie_deinit_phys(rockchip);
+err_disable_vpcie:
if (!IS_ERR(rockchip->vpcie0v9))
regulator_disable(rockchip->vpcie0v9);
-err_set_vpcie:
+ if (!IS_ERR(rockchip->vpcie1v8))
+ regulator_disable(rockchip->vpcie1v8);
+ if (!IS_ERR(rockchip->vpcie3v3))
+ regulator_disable(rockchip->vpcie3v3);
+ if (!IS_ERR(rockchip->vpcie12v))
+ regulator_disable(rockchip->vpcie12v);
+err_disable_clk_pcie_pm:
clk_disable_unprepare(rockchip->clk_pcie_pm);
-err_pcie_pm:
+err_disable_hclk_pcie:
clk_disable_unprepare(rockchip->hclk_pcie);
-err_hclk_pcie:
+err_disable_aclk_perf_pcie:
clk_disable_unprepare(rockchip->aclk_perf_pcie);
-err_aclk_perf_pcie:
+err_disable_aclk_pcie:
clk_disable_unprepare(rockchip->aclk_pcie);
-err_aclk_pcie:
return err;
}
@@ -1620,33 +1640,27 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
- int i;
pci_stop_root_bus(rockchip->root_bus);
pci_remove_root_bus(rockchip->root_bus);
pci_unmap_iospace(rockchip->io);
irq_domain_remove(rockchip->irq_domain);
- for (i = 0; i < MAX_LANE_NUM; i++) {
- /* inactive lane is already powered off */
- if (rockchip->lanes_map & BIT(i))
- phy_power_off(rockchip->phys[i]);
- phy_exit(rockchip->phys[i]);
- }
+ rockchip_pcie_deinit_phys(rockchip);
clk_disable_unprepare(rockchip->clk_pcie_pm);
clk_disable_unprepare(rockchip->hclk_pcie);
clk_disable_unprepare(rockchip->aclk_perf_pcie);
clk_disable_unprepare(rockchip->aclk_pcie);
- if (!IS_ERR(rockchip->vpcie12v))
- regulator_disable(rockchip->vpcie12v);
- if (!IS_ERR(rockchip->vpcie3v3))
- regulator_disable(rockchip->vpcie3v3);
- if (!IS_ERR(rockchip->vpcie1v8))
- regulator_disable(rockchip->vpcie1v8);
if (!IS_ERR(rockchip->vpcie0v9))
regulator_disable(rockchip->vpcie0v9);
+ if (!IS_ERR(rockchip->vpcie1v8))
+ regulator_disable(rockchip->vpcie1v8);
+ if (!IS_ERR(rockchip->vpcie3v3))
+ regulator_disable(rockchip->vpcie3v3);
+ if (!IS_ERR(rockchip->vpcie12v))
+ regulator_disable(rockchip->vpcie12v);
return 0;
}
--
2.11.0
Add support for PCIE_WAKE pin in rockchip pcie driver.
Signed-off-by: Jeffy Chen <[email protected]>
---
Changes in v4: None
Changes in v3:
Fix error handling
Changes in v2:
Use dev_pm_set_dedicated_wake_irq
-- Suggested by Brian Norris <[email protected]>
drivers/pci/host/pcie-rockchip.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 5d85ec2e2fb0..a0f7267984da 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -37,6 +37,7 @@
#include <linux/pci_ids.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
#include <linux/reset.h>
#include <linux/regmap.h>
@@ -1111,6 +1112,15 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
return err;
}
+ /* Must init wakeup before setting dedicated wake irq. */
+ device_init_wakeup(dev, true);
+ irq = platform_get_irq_byname(pdev, "wake");
+ if (irq >= 0) {
+ err = dev_pm_set_dedicated_wake_irq(dev, irq);
+ if (err)
+ dev_err(dev, "failed to setup PCIe wake IRQ\n");
+ }
+
rockchip->vpcie12v = devm_regulator_get_optional(dev, "vpcie12v");
if (IS_ERR(rockchip->vpcie12v)) {
if (PTR_ERR(rockchip->vpcie12v) == -EPROBE_DEFER)
@@ -1493,12 +1503,13 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
err = rockchip_pcie_parse_dt(rockchip);
if (err)
- return err;
+ /* It's safe to disable wake even not enabled */
+ goto err_disable_wake;
err = clk_prepare_enable(rockchip->aclk_pcie);
if (err) {
dev_err(dev, "unable to enable aclk_pcie clock\n");
- return err;
+ goto err_disable_wake;
}
err = clk_prepare_enable(rockchip->aclk_perf_pcie);
@@ -1633,6 +1644,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
clk_disable_unprepare(rockchip->aclk_perf_pcie);
err_disable_aclk_pcie:
clk_disable_unprepare(rockchip->aclk_pcie);
+err_disable_wake:
+ dev_pm_clear_wake_irq(dev);
+ device_init_wakeup(dev, false);
return err;
}
@@ -1662,6 +1676,9 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
if (!IS_ERR(rockchip->vpcie12v))
regulator_disable(rockchip->vpcie12v);
+ dev_pm_clear_wake_irq(dev);
+ device_init_wakeup(dev, false);
+
return 0;
}
--
2.11.0
Add an optional interrupt for PCIE_WAKE pin.
Signed-off-by: Jeffy Chen <[email protected]>
---
Changes in v4: None
Changes in v3: None
Changes in v2: None
.../devicetree/bindings/pci/rockchip-pcie.txt | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
index 5678be82530d..9f6504129e80 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
@@ -20,10 +20,13 @@ Required properties:
- msi-map: Maps a Requester ID to an MSI controller and associated
msi-specifier data. See ./pci-msi.txt
- interrupts: Three interrupt entries must be specified.
-- interrupt-names: Must include the following names
- - "sys"
- - "legacy"
- - "client"
+- interrupt-names: Include the following names
+ Required:
+ - "sys"
+ - "legacy"
+ - "client"
+ Optional:
+ - "wake"
- resets: Must contain seven entries for each entry in reset-names.
See ../reset/reset.txt for details.
- reset-names: Must include the following names
@@ -87,10 +90,11 @@ pcie0: pcie@f8000000 {
clock-names = "aclk", "aclk-perf",
"hclk", "pm";
bus-range = <0x0 0x1>;
- interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH 0>,
- <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH 0>,
- <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH 0>;
- interrupt-names = "sys", "legacy", "client";
+ interrupts-extended = <&gic GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH 0>,
+ <&gic GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH 0>,
+ <&gic GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH 0>,
+ <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-names = "sys", "legacy", "client", "wake";
assigned-clocks = <&cru SCLK_PCIEPHY_REF>;
assigned-clock-parents = <&cru SCLK_PCIEPHY_REF100M>;
assigned-clock-rates = <100000000>;
--
2.11.0
Currently we are handling pcie wake irq in mrvl wifi driver.
Move it to rockchip pcie driver for Gru boards.
Signed-off-by: Jeffy Chen <[email protected]>
---
Changes in v4: None
Changes in v3: None
Changes in v2: None
arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
index d48e98b62d09..42158512e979 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
@@ -712,7 +712,15 @@ ap_i2c_audio: &i2c8 {
ep-gpios = <&gpio2 27 GPIO_ACTIVE_HIGH>;
pinctrl-names = "default";
- pinctrl-0 = <&pcie_clkreqn_cpm>, <&wifi_perst_l>;
+ pinctrl-0 = <&pcie_clkreqn_cpm>, <&wlan_host_wake_l>, <&wifi_perst_l>;
+
+ interrupts-extended = <&gic GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH 0>,
+ <&gic GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH 0>,
+ <&gic GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH 0>,
+ <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-names = "sys", "legacy", "client", "wake";
+ /delete-property/ interrupts;
+
vpcie3v3-supply = <&pp3300_wifi_bt>;
vpcie1v8-supply = <&wlan_pd_n>; /* HACK: see &wlan_pd_n */
vpcie0v9-supply = <&pp900_pcie>;
@@ -727,11 +735,6 @@ ap_i2c_audio: &i2c8 {
compatible = "pci1b4b,2b42";
reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
0x83010000 0x0 0x00100000 0x0 0x00100000>;
- interrupt-parent = <&gpio0>;
- interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
- pinctrl-names = "default";
- pinctrl-0 = <&wlan_host_wake_l>;
- wakeup-source;
};
};
};
--
2.11.0
On Tue, Aug 22, 2017 at 11:19:31AM +0800, Jeffy Chen wrote:
> Fix error handlings in probe & resume.
Changelog should have a little detail about *what* you're fixing. For
example, it looks like you are powering off PHYs if the init fails.
> Signed-off-by: Jeffy Chen <[email protected]>
> ---
>
> Changes in v4:
> Rebase on newest for-next branch, also fix error handling by:
> 1e7f570a1b86 PCI: rockchip: Idle inactive PHY(s)
>
> Changes in v3: None
> Changes in v2: None
>
> drivers/pci/host/pcie-rockchip.c | 160 +++++++++++++++++++++------------------
> 1 file changed, 87 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 2eccd532c256..5d85ec2e2fb0 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -561,32 +561,32 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
> err = phy_init(rockchip->phys[i]);
> if (err) {
> dev_err(dev, "init phy%d err %d\n", i, err);
> - return err;
> + goto err_exit_phy;
> }
> }
>
> err = reset_control_assert(rockchip->core_rst);
> if (err) {
> dev_err(dev, "assert core_rst err %d\n", err);
> - return err;
> + goto err_exit_phy;
> }
>
> err = reset_control_assert(rockchip->mgmt_rst);
> if (err) {
> dev_err(dev, "assert mgmt_rst err %d\n", err);
> - return err;
> + goto err_exit_phy;
> }
>
> err = reset_control_assert(rockchip->mgmt_sticky_rst);
> if (err) {
> dev_err(dev, "assert mgmt_sticky_rst err %d\n", err);
> - return err;
> + goto err_exit_phy;
> }
>
> err = reset_control_assert(rockchip->pipe_rst);
> if (err) {
> dev_err(dev, "assert pipe_rst err %d\n", err);
> - return err;
> + goto err_exit_phy;
> }
>
> udelay(10);
> @@ -594,19 +594,19 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
> err = reset_control_deassert(rockchip->pm_rst);
> if (err) {
> dev_err(dev, "deassert pm_rst err %d\n", err);
> - return err;
> + goto err_exit_phy;
> }
>
> err = reset_control_deassert(rockchip->aclk_rst);
> if (err) {
> dev_err(dev, "deassert aclk_rst err %d\n", err);
> - return err;
> + goto err_exit_phy;
> }
>
> err = reset_control_deassert(rockchip->pclk_rst);
> if (err) {
> dev_err(dev, "deassert pclk_rst err %d\n", err);
> - return err;
> + goto err_exit_phy;
> }
>
> if (rockchip->link_gen == 2)
> @@ -628,7 +628,7 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
> err = phy_power_on(rockchip->phys[i]);
> if (err) {
> dev_err(dev, "power on phy%d err %d\n", i, err);
> - return err;
> + goto err_power_off_phy;
> }
> }
>
> @@ -639,25 +639,25 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
> err = reset_control_deassert(rockchip->mgmt_sticky_rst);
> if (err) {
> dev_err(dev, "deassert mgmt_sticky_rst err %d\n", err);
> - return err;
> + goto err_power_off_phy;
> }
>
> err = reset_control_deassert(rockchip->core_rst);
> if (err) {
> dev_err(dev, "deassert core_rst err %d\n", err);
> - return err;
> + goto err_power_off_phy;
> }
>
> err = reset_control_deassert(rockchip->mgmt_rst);
> if (err) {
> dev_err(dev, "deassert mgmt_rst err %d\n", err);
> - return err;
> + goto err_power_off_phy;
> }
>
> err = reset_control_deassert(rockchip->pipe_rst);
> if (err) {
> dev_err(dev, "deassert pipe_rst err %d\n", err);
> - return err;
> + goto err_power_off_phy;
> }
>
> /* Fix the transmitted FTS count desired to exit from L0s. */
> @@ -690,7 +690,7 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
> 500 * USEC_PER_MSEC);
> if (err) {
> dev_err(dev, "PCIe link training gen1 timeout!\n");
> - return -ETIMEDOUT;
> + goto err_power_off_phy;
> }
>
> if (rockchip->link_gen == 2) {
> @@ -751,6 +751,26 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
> rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DCSR);
>
> return 0;
> +err_power_off_phy:
> + while (i--)
> + phy_power_off(rockchip->phys[i]);
> + i = MAX_LANE_NUM;
> +err_exit_phy:
> + while (i--)
> + phy_exit(rockchip->phys[i]);
> + return err;
> +}
> +
> +static void rockchip_pcie_deinit_phys(struct rockchip_pcie *rockchip)
> +{
> + int i;
> +
> + for (i = 0; i < MAX_LANE_NUM; i++) {
> + /* inactive lane is already powered off */
> + if (rockchip->legacy_phy || rockchip->lanes_map & BIT(i))
> + phy_power_off(rockchip->phys[i]);
> + phy_exit(rockchip->phys[i]);
> + }
> }
>
> static irqreturn_t rockchip_pcie_subsys_irq_handler(int irq, void *arg)
> @@ -1131,7 +1151,7 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
> err = regulator_enable(rockchip->vpcie12v);
> if (err) {
> dev_err(dev, "fail to enable vpcie12v regulator\n");
> - goto err_out;
> + return err;
> }
> }
>
> @@ -1160,7 +1180,6 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
> }
>
> return 0;
> -
> err_disable_1v8:
> if (!IS_ERR(rockchip->vpcie1v8))
> regulator_disable(rockchip->vpcie1v8);
> @@ -1170,7 +1189,6 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
> err_disable_12v:
> if (!IS_ERR(rockchip->vpcie12v))
> regulator_disable(rockchip->vpcie12v);
> -err_out:
> return err;
> }
>
> @@ -1364,7 +1382,7 @@ static int rockchip_pcie_wait_l2(struct rockchip_pcie *rockchip)
> static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
> {
> struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> - int ret, i;
> + int ret;
>
> /* disable core and cli int since we don't need to ack PME_ACK */
> rockchip_pcie_write(rockchip, (PCIE_CLIENT_INT_CLI << 16) |
> @@ -1377,12 +1395,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
> return ret;
> }
>
> - for (i = 0; i < MAX_LANE_NUM; i++) {
> - /* inactive lane is already powered off */
> - if (rockchip->lanes_map & BIT(i))
> - phy_power_off(rockchip->phys[i]);
> - phy_exit(rockchip->phys[i]);
> - }
> + rockchip_pcie_deinit_phys(rockchip);
>
> clk_disable_unprepare(rockchip->clk_pcie_pm);
> clk_disable_unprepare(rockchip->hclk_pcie);
> @@ -1391,7 +1404,6 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>
> if (!IS_ERR(rockchip->vpcie0v9))
> regulator_disable(rockchip->vpcie0v9);
> -
> return ret;
> }
>
> @@ -1410,43 +1422,46 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>
> err = clk_prepare_enable(rockchip->clk_pcie_pm);
> if (err)
> - goto err_pcie_pm;
> + goto err_disable_0v9;
>
> err = clk_prepare_enable(rockchip->hclk_pcie);
> if (err)
> - goto err_hclk_pcie;
> + goto err_disable_clk_pcie_pm;
>
> err = clk_prepare_enable(rockchip->aclk_perf_pcie);
> if (err)
> - goto err_aclk_perf_pcie;
> + goto err_disable_hclk_pcie;
>
> err = clk_prepare_enable(rockchip->aclk_pcie);
> if (err)
> - goto err_aclk_pcie;
> + goto err_disable_aclk_perf_pcie;
>
> err = rockchip_pcie_init_port(rockchip);
> if (err)
> - goto err_pcie_resume;
> + goto err_disable_aclk_pcie;
>
> err = rockchip_pcie_cfg_atu(rockchip);
> if (err)
> - goto err_pcie_resume;
> + goto err_deinit_port;
>
> /* Need this to enter L1 again */
> rockchip_pcie_update_txcredit_mui(rockchip);
> rockchip_pcie_enable_interrupts(rockchip);
>
> return 0;
> -
> -err_pcie_resume:
> +err_deinit_port:
> + rockchip_pcie_deinit_phys(rockchip);
> +err_disable_aclk_pcie:
> clk_disable_unprepare(rockchip->aclk_pcie);
> -err_aclk_pcie:
> +err_disable_aclk_perf_pcie:
> clk_disable_unprepare(rockchip->aclk_perf_pcie);
> -err_aclk_perf_pcie:
> +err_disable_hclk_pcie:
> clk_disable_unprepare(rockchip->hclk_pcie);
> -err_hclk_pcie:
> +err_disable_clk_pcie_pm:
> clk_disable_unprepare(rockchip->clk_pcie_pm);
> -err_pcie_pm:
> +err_disable_0v9:
> + if (!IS_ERR(rockchip->vpcie0v9))
> + regulator_disable(rockchip->vpcie0v9);
> return err;
> }
>
> @@ -1483,47 +1498,47 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> err = clk_prepare_enable(rockchip->aclk_pcie);
> if (err) {
> dev_err(dev, "unable to enable aclk_pcie clock\n");
> - goto err_aclk_pcie;
> + return err;
> }
>
> err = clk_prepare_enable(rockchip->aclk_perf_pcie);
> if (err) {
> dev_err(dev, "unable to enable aclk_perf_pcie clock\n");
> - goto err_aclk_perf_pcie;
> + goto err_disable_aclk_pcie;
> }
>
> err = clk_prepare_enable(rockchip->hclk_pcie);
> if (err) {
> dev_err(dev, "unable to enable hclk_pcie clock\n");
> - goto err_hclk_pcie;
> + goto err_disable_aclk_perf_pcie;
> }
>
> err = clk_prepare_enable(rockchip->clk_pcie_pm);
> if (err) {
> dev_err(dev, "unable to enable hclk_pcie clock\n");
> - goto err_pcie_pm;
> + goto err_disable_hclk_pcie;
> }
>
> err = rockchip_pcie_set_vpcie(rockchip);
> if (err) {
> dev_err(dev, "failed to set vpcie regulator\n");
> - goto err_set_vpcie;
> + goto err_disable_clk_pcie_pm;
> }
>
> err = rockchip_pcie_init_port(rockchip);
> if (err)
> - goto err_vpcie;
> + goto err_disable_vpcie;
>
> rockchip_pcie_enable_interrupts(rockchip);
>
> err = rockchip_pcie_init_irq_domain(rockchip);
> if (err < 0)
> - goto err_vpcie;
> + goto err_deinit_port;
>
> err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff,
> &res, &io_base);
> if (err)
> - goto err_vpcie;
> + goto err_remove_irq_domain;
>
> err = devm_request_pci_bus_resources(dev, &res);
> if (err)
> @@ -1561,12 +1576,12 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>
> err = rockchip_pcie_cfg_atu(rockchip);
> if (err)
> - goto err_free_res;
> + goto err_unmap_iospace;
>
> rockchip->msg_region = devm_ioremap(dev, rockchip->msg_bus_addr, SZ_1M);
> if (!rockchip->msg_region) {
> err = -ENOMEM;
> - goto err_free_res;
> + goto err_unmap_iospace;
> }
>
> list_splice_init(&res, &bridge->windows);
> @@ -1591,28 +1606,33 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> pcie_bus_configure_settings(child);
>
> pci_bus_add_devices(bus);
> - return 0;
>
> + return 0;
> +err_unmap_iospace:
> + pci_unmap_iospace(rockchip->io);
> err_free_res:
> pci_free_resource_list(&res);
> -err_vpcie:
> - if (!IS_ERR(rockchip->vpcie12v))
> - regulator_disable(rockchip->vpcie12v);
> - if (!IS_ERR(rockchip->vpcie3v3))
> - regulator_disable(rockchip->vpcie3v3);
> - if (!IS_ERR(rockchip->vpcie1v8))
> - regulator_disable(rockchip->vpcie1v8);
> +err_remove_irq_domain:
> + irq_domain_remove(rockchip->irq_domain);
> +err_deinit_port:
> + rockchip_pcie_deinit_phys(rockchip);
> +err_disable_vpcie:
> if (!IS_ERR(rockchip->vpcie0v9))
> regulator_disable(rockchip->vpcie0v9);
> -err_set_vpcie:
> + if (!IS_ERR(rockchip->vpcie1v8))
> + regulator_disable(rockchip->vpcie1v8);
> + if (!IS_ERR(rockchip->vpcie3v3))
> + regulator_disable(rockchip->vpcie3v3);
> + if (!IS_ERR(rockchip->vpcie12v))
> + regulator_disable(rockchip->vpcie12v);
> +err_disable_clk_pcie_pm:
> clk_disable_unprepare(rockchip->clk_pcie_pm);
> -err_pcie_pm:
> +err_disable_hclk_pcie:
> clk_disable_unprepare(rockchip->hclk_pcie);
> -err_hclk_pcie:
> +err_disable_aclk_perf_pcie:
> clk_disable_unprepare(rockchip->aclk_perf_pcie);
> -err_aclk_perf_pcie:
> +err_disable_aclk_pcie:
> clk_disable_unprepare(rockchip->aclk_pcie);
> -err_aclk_pcie:
> return err;
> }
>
> @@ -1620,33 +1640,27 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> - int i;
>
> pci_stop_root_bus(rockchip->root_bus);
> pci_remove_root_bus(rockchip->root_bus);
> pci_unmap_iospace(rockchip->io);
> irq_domain_remove(rockchip->irq_domain);
>
> - for (i = 0; i < MAX_LANE_NUM; i++) {
> - /* inactive lane is already powered off */
> - if (rockchip->lanes_map & BIT(i))
> - phy_power_off(rockchip->phys[i]);
> - phy_exit(rockchip->phys[i]);
> - }
> + rockchip_pcie_deinit_phys(rockchip);
>
> clk_disable_unprepare(rockchip->clk_pcie_pm);
> clk_disable_unprepare(rockchip->hclk_pcie);
> clk_disable_unprepare(rockchip->aclk_perf_pcie);
> clk_disable_unprepare(rockchip->aclk_pcie);
>
> - if (!IS_ERR(rockchip->vpcie12v))
> - regulator_disable(rockchip->vpcie12v);
> - if (!IS_ERR(rockchip->vpcie3v3))
> - regulator_disable(rockchip->vpcie3v3);
> - if (!IS_ERR(rockchip->vpcie1v8))
> - regulator_disable(rockchip->vpcie1v8);
> if (!IS_ERR(rockchip->vpcie0v9))
> regulator_disable(rockchip->vpcie0v9);
> + if (!IS_ERR(rockchip->vpcie1v8))
> + regulator_disable(rockchip->vpcie1v8);
> + if (!IS_ERR(rockchip->vpcie3v3))
> + regulator_disable(rockchip->vpcie3v3);
> + if (!IS_ERR(rockchip->vpcie12v))
> + regulator_disable(rockchip->vpcie12v);
>
> return 0;
> }
> --
> 2.11.0
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Aug 22, 2017 at 11:19:32AM +0800, Jeffy Chen wrote:
> Add support for PCIE_WAKE pin in rockchip pcie driver.
Changelog should include a hint about what DT property this PCIE_WAKE
support depends on.
> Signed-off-by: Jeffy Chen <[email protected]>
> ---
>
> Changes in v4: None
> Changes in v3:
> Fix error handling
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
> -- Suggested by Brian Norris <[email protected]>
>
> drivers/pci/host/pcie-rockchip.c | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 5d85ec2e2fb0..a0f7267984da 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -37,6 +37,7 @@
> #include <linux/pci_ids.h>
> #include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_wakeirq.h>
> #include <linux/reset.h>
> #include <linux/regmap.h>
>
> @@ -1111,6 +1112,15 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> return err;
> }
>
> + /* Must init wakeup before setting dedicated wake irq. */
> + device_init_wakeup(dev, true);
> + irq = platform_get_irq_byname(pdev, "wake");
> + if (irq >= 0) {
> + err = dev_pm_set_dedicated_wake_irq(dev, irq);
> + if (err)
> + dev_err(dev, "failed to setup PCIe wake IRQ\n");
Error message could include the IRQ #.
> + }
> +
> rockchip->vpcie12v = devm_regulator_get_optional(dev, "vpcie12v");
> if (IS_ERR(rockchip->vpcie12v)) {
> if (PTR_ERR(rockchip->vpcie12v) == -EPROBE_DEFER)
> @@ -1493,12 +1503,13 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>
> err = rockchip_pcie_parse_dt(rockchip);
> if (err)
> - return err;
> + /* It's safe to disable wake even not enabled */
> + goto err_disable_wake;
>
> err = clk_prepare_enable(rockchip->aclk_pcie);
> if (err) {
> dev_err(dev, "unable to enable aclk_pcie clock\n");
> - return err;
> + goto err_disable_wake;
> }
>
> err = clk_prepare_enable(rockchip->aclk_perf_pcie);
> @@ -1633,6 +1644,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> clk_disable_unprepare(rockchip->aclk_perf_pcie);
> err_disable_aclk_pcie:
> clk_disable_unprepare(rockchip->aclk_pcie);
> +err_disable_wake:
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> return err;
> }
>
> @@ -1662,6 +1676,9 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> if (!IS_ERR(rockchip->vpcie12v))
> regulator_disable(rockchip->vpcie12v);
>
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> +
> return 0;
> }
>
> --
> 2.11.0
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Aug 22, 2017 at 11:19:33AM +0800, Jeffy Chen wrote:
> Add an optional interrupt for PCIE_WAKE pin.
Rob?
> Signed-off-by: Jeffy Chen <[email protected]>
> ---
>
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
> .../devicetree/bindings/pci/rockchip-pcie.txt | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> index 5678be82530d..9f6504129e80 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> @@ -20,10 +20,13 @@ Required properties:
> - msi-map: Maps a Requester ID to an MSI controller and associated
> msi-specifier data. See ./pci-msi.txt
> - interrupts: Three interrupt entries must be specified.
> -- interrupt-names: Must include the following names
> - - "sys"
> - - "legacy"
> - - "client"
> +- interrupt-names: Include the following names
> + Required:
> + - "sys"
> + - "legacy"
> + - "client"
> + Optional:
> + - "wake"
Why is there no other PCI binding that includes "wake" as an
interrupt-name? This feels like something that should be fairly
common across host controllers. I don't want a Rockport-specific
DT description if it could be made more general.
> - resets: Must contain seven entries for each entry in reset-names.
> See ../reset/reset.txt for details.
> - reset-names: Must include the following names
> @@ -87,10 +90,11 @@ pcie0: pcie@f8000000 {
> clock-names = "aclk", "aclk-perf",
> "hclk", "pm";
> bus-range = <0x0 0x1>;
> - interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH 0>,
> - <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH 0>,
> - <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH 0>;
> - interrupt-names = "sys", "legacy", "client";
> + interrupts-extended = <&gic GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH 0>,
> + <&gic GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH 0>,
> + <&gic GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH 0>,
> + <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-names = "sys", "legacy", "client", "wake";
> assigned-clocks = <&cru SCLK_PCIEPHY_REF>;
> assigned-clock-parents = <&cru SCLK_PCIEPHY_REF100M>;
> assigned-clock-rates = <100000000>;
> --
> 2.11.0
>
>
On Tue, Aug 22, 2017 at 11:19:30AM +0800, Jeffy Chen wrote:
>
> Currently we are handling pcie wake in mrvl wifi driver. But Brian
> suggests to move it into rockchip pcie driver.
>
> Tested on my chromebook bob(with cros 4.4 kernel and mrvl wifi).
>
>
> Changes in v4:
> Rebase on newest for-next branch, also fix error handling by:
> 1e7f570a1b86 PCI: rockchip: Idle inactive PHY(s)
>
> Changes in v3:
> Fix error handling
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
> -- Suggested by Brian Norris <[email protected]>
>
> Jeffy Chen (4):
> PCI: rockchip: Fix error handlings
> PCI: rockchip: Add support for pcie wake irq
> dt-bindings: PCI: rockchip: Add support for pcie wake irq
> arm64: dts: rockchip: Handle pcie wake in pcie driver for Gru
>
> .../devicetree/bindings/pci/rockchip-pcie.txt | 20 ++-
> arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 15 +-
> drivers/pci/host/pcie-rockchip.c | 179 ++++++++++++---------
> 3 files changed, 126 insertions(+), 88 deletions(-)
Looking for acks from Shawn and Rob...
And I'm not sure about the DT wake IRQ description. That seems like it
could potentially be generic than this Rockchip-specific proposal.
Hi Bjorn,
On 08/25/2017 12:55 AM, Bjorn Helgaas wrote:
> Looking for acks from Shawn and Rob...
>
> And I'm not sure about the DT wake IRQ description. That seems like it
> could potentially be generic than this Rockchip-specific proposal.
>
>
it looks like shawn already take the error handling patch into his
series, so i'll remove that one in my next version.
i'll wait for those error handling patches to land, then rebase and
resend this wake irq patches, thanks:)
>
On Thu, Aug 24, 2017 at 11:53:54AM -0500, Bjorn Helgaas wrote:
> On Tue, Aug 22, 2017 at 11:19:33AM +0800, Jeffy Chen wrote:
> > Signed-off-by: Jeffy Chen <[email protected]>
> > diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > index 5678be82530d..9f6504129e80 100644
> > --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > @@ -20,10 +20,13 @@ Required properties:
> > - msi-map: Maps a Requester ID to an MSI controller and associated
> > msi-specifier data. See ./pci-msi.txt
> > - interrupts: Three interrupt entries must be specified.
> > -- interrupt-names: Must include the following names
> > - - "sys"
> > - - "legacy"
> > - - "client"
> > +- interrupt-names: Include the following names
> > + Required:
> > + - "sys"
> > + - "legacy"
> > + - "client"
> > + Optional:
> > + - "wake"
>
> Why is there no other PCI binding that includes "wake" as an
> interrupt-name? This feels like something that should be fairly
> common across host controllers. I don't want a Rockport-specific
s/port/chip/ :)
> DT description if it could be made more general.
I'm not sure we can really answer that question ("why do no other PCI
bindings have this?"). But one guess would be that every other
controller uses only beacon wake.
It would be OK with me if we made a blanket statement that a controller
with a "wake" interrupt means PCI WAKE# (per the specification). It's
possible this could even be stuck into some generic PCI/DT code
eventually. (I don't think we have a really good place for this today.)
Brian
> > - resets: Must contain seven entries for each entry in reset-names.
> > See ../reset/reset.txt for details.
> > - reset-names: Must include the following names
> > @@ -87,10 +90,11 @@ pcie0: pcie@f8000000 {
> > clock-names = "aclk", "aclk-perf",
> > "hclk", "pm";
> > bus-range = <0x0 0x1>;
> > - interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH 0>,
> > - <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH 0>,
> > - <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH 0>;
> > - interrupt-names = "sys", "legacy", "client";
> > + interrupts-extended = <&gic GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH 0>,
> > + <&gic GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH 0>,
> > + <&gic GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH 0>,
> > + <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
> > + interrupt-names = "sys", "legacy", "client", "wake";
> > assigned-clocks = <&cru SCLK_PCIEPHY_REF>;
> > assigned-clock-parents = <&cru SCLK_PCIEPHY_REF100M>;
> > assigned-clock-rates = <100000000>;
> > --
> > 2.11.0
> >
> >
On 2017/8/25 10:11, Brian Norris wrote:
> On Thu, Aug 24, 2017 at 11:53:54AM -0500, Bjorn Helgaas wrote:
>> On Tue, Aug 22, 2017 at 11:19:33AM +0800, Jeffy Chen wrote:
>>> Signed-off-by: Jeffy Chen <[email protected]>
>
>>> diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>>> index 5678be82530d..9f6504129e80 100644
>>> --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>>> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>>> @@ -20,10 +20,13 @@ Required properties:
>>> - msi-map: Maps a Requester ID to an MSI controller and associated
>>> msi-specifier data. See ./pci-msi.txt
>>> - interrupts: Three interrupt entries must be specified.
>>> -- interrupt-names: Must include the following names
>>> - - "sys"
>>> - - "legacy"
>>> - - "client"
>>> +- interrupt-names: Include the following names
>>> + Required:
>>> + - "sys"
>>> + - "legacy"
>>> + - "client"
>>> + Optional:
>>> + - "wake"
>>
>> Why is there no other PCI binding that includes "wake" as an
>> interrupt-name? This feels like something that should be fairly
>> common across host controllers. I don't want a Rockport-specific
>
> s/port/chip/ :)
>
>> DT description if it could be made more general.
>
> I'm not sure we can really answer that question ("why do no other PCI
> bindings have this?"). But one guess would be that every other
> controller uses only beacon wake.
>
> It would be OK with me if we made a blanket statement that a controller
> with a "wake" interrupt means PCI WAKE# (per the specification). It's
> possible this could even be stuck into some generic PCI/DT code
> eventually. (I don't think we have a really good place for this today.)
I guess we could register a pcie port service for dedicated WAKE# as it
seems fairly parallel to pme code there, if we need a common place for
that?
>
> Brian
>
>>> - resets: Must contain seven entries for each entry in reset-names.
>>> See ../reset/reset.txt for details.
>>> - reset-names: Must include the following names
>>> @@ -87,10 +90,11 @@ pcie0: pcie@f8000000 {
>>> clock-names = "aclk", "aclk-perf",
>>> "hclk", "pm";
>>> bus-range = <0x0 0x1>;
>>> - interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH 0>,
>>> - <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH 0>,
>>> - <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH 0>;
>>> - interrupt-names = "sys", "legacy", "client";
>>> + interrupts-extended = <&gic GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH 0>,
>>> + <&gic GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH 0>,
>>> + <&gic GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH 0>,
>>> + <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
>>> + interrupt-names = "sys", "legacy", "client", "wake";
>>> assigned-clocks = <&cru SCLK_PCIEPHY_REF>;
>>> assigned-clock-parents = <&cru SCLK_PCIEPHY_REF100M>;
>>> assigned-clock-rates = <100000000>;
>>> --
>>> 2.11.0
>>>
>>>
>
>
>
On Thu, Aug 24, 2017 at 07:11:32PM -0700, Brian Norris wrote:
> On Thu, Aug 24, 2017 at 11:53:54AM -0500, Bjorn Helgaas wrote:
> > On Tue, Aug 22, 2017 at 11:19:33AM +0800, Jeffy Chen wrote:
> > > Signed-off-by: Jeffy Chen <[email protected]>
>
> > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > > index 5678be82530d..9f6504129e80 100644
> > > --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > > +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > > @@ -20,10 +20,13 @@ Required properties:
> > > - msi-map: Maps a Requester ID to an MSI controller and associated
> > > msi-specifier data. See ./pci-msi.txt
> > > - interrupts: Three interrupt entries must be specified.
> > > -- interrupt-names: Must include the following names
> > > - - "sys"
> > > - - "legacy"
> > > - - "client"
> > > +- interrupt-names: Include the following names
> > > + Required:
> > > + - "sys"
> > > + - "legacy"
> > > + - "client"
> > > + Optional:
> > > + - "wake"
> >
> > Why is there no other PCI binding that includes "wake" as an
> > interrupt-name? This feels like something that should be fairly
> > common across host controllers. I don't want a Rockport-specific
>
> s/port/chip/ :)
I visited Rockport this summer, guess I had it on the brain :)
> > DT description if it could be made more general.
>
> I'm not sure we can really answer that question ("why do no other PCI
> bindings have this?"). But one guess would be that every other
> controller uses only beacon wake.
>
> It would be OK with me if we made a blanket statement that a controller
> with a "wake" interrupt means PCI WAKE# (per the specification). It's
> possible this could even be stuck into some generic PCI/DT code
> eventually. (I don't think we have a really good place for this today.)
I'd just like every controller that uses WAKE# to use the same name in
DT. Maybe all that means for now is mentioning it in
Documentation/devicetree/bindings/pci/pci.txt instead of (or in
addition to) Documentation/devicetree/bindings/pci/rockchip-pcie.txt
On Tue, Aug 22, 2017 at 11:19:33AM +0800, Jeffy Chen wrote:
> Add an optional interrupt for PCIE_WAKE pin.
>
> Signed-off-by: Jeffy Chen <[email protected]>
> ---
>
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
> .../devicetree/bindings/pci/rockchip-pcie.txt | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> index 5678be82530d..9f6504129e80 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> @@ -20,10 +20,13 @@ Required properties:
> - msi-map: Maps a Requester ID to an MSI controller and associated
> msi-specifier data. See ./pci-msi.txt
> - interrupts: Three interrupt entries must be specified.
> -- interrupt-names: Must include the following names
> - - "sys"
> - - "legacy"
> - - "client"
> +- interrupt-names: Include the following names
> + Required:
> + - "sys"
> + - "legacy"
> + - "client"
> + Optional:
> + - "wake"
Use the wakeup source binding:
Documentation/devicetree/bindings/power/wakeup-source.txt
Rob
On Fri, Aug 25, 2017 at 01:14:39PM -0500, Rob Herring wrote:
> On Tue, Aug 22, 2017 at 11:19:33AM +0800, Jeffy Chen wrote:
> > Add an optional interrupt for PCIE_WAKE pin.
> >
> > Signed-off-by: Jeffy Chen <[email protected]>
> > ---
> >
> > Changes in v4: None
> > Changes in v3: None
> > Changes in v2: None
> >
> > .../devicetree/bindings/pci/rockchip-pcie.txt | 20 ++++++++++++--------
> > 1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > index 5678be82530d..9f6504129e80 100644
> > --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > @@ -20,10 +20,13 @@ Required properties:
> > - msi-map: Maps a Requester ID to an MSI controller and associated
> > msi-specifier data. See ./pci-msi.txt
> > - interrupts: Three interrupt entries must be specified.
> > -- interrupt-names: Must include the following names
> > - - "sys"
> > - - "legacy"
> > - - "client"
> > +- interrupt-names: Include the following names
> > + Required:
> > + - "sys"
> > + - "legacy"
> > + - "client"
> > + Optional:
> > + - "wake"
>
> Use the wakeup source binding:
> Documentation/devicetree/bindings/power/wakeup-source.txt
And I suppose this means we'd fall under this paragraph?
"However if the devices have dedicated interrupt as the wakeup source
then they need to specify/identify the same using device specific
interrupt name. In such cases only that interrupt can be used as wakeup
interrupt."
We don't expect *any* interrupt to qualify as PCI WAKE#; so we should
still also document the interrupt name ("wake"?) in
Documentation/devicetree/bindings/pci/pci.txt as Bjorn suggested, in
addition to using the 'wakeup-source' property documented there.
Brian
On Fri, Aug 25, 2017 at 1:20 PM, Brian Norris <[email protected]> wrote:
> On Fri, Aug 25, 2017 at 01:14:39PM -0500, Rob Herring wrote:
>> On Tue, Aug 22, 2017 at 11:19:33AM +0800, Jeffy Chen wrote:
>> > Add an optional interrupt for PCIE_WAKE pin.
>> >
>> > Signed-off-by: Jeffy Chen <[email protected]>
>> > ---
>> >
>> > Changes in v4: None
>> > Changes in v3: None
>> > Changes in v2: None
>> >
>> > .../devicetree/bindings/pci/rockchip-pcie.txt | 20 ++++++++++++--------
>> > 1 file changed, 12 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> > index 5678be82530d..9f6504129e80 100644
>> > --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> > +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> > @@ -20,10 +20,13 @@ Required properties:
>> > - msi-map: Maps a Requester ID to an MSI controller and associated
>> > msi-specifier data. See ./pci-msi.txt
>> > - interrupts: Three interrupt entries must be specified.
>> > -- interrupt-names: Must include the following names
>> > - - "sys"
>> > - - "legacy"
>> > - - "client"
>> > +- interrupt-names: Include the following names
>> > + Required:
>> > + - "sys"
>> > + - "legacy"
>> > + - "client"
>> > + Optional:
>> > + - "wake"
>>
>> Use the wakeup source binding:
>> Documentation/devicetree/bindings/power/wakeup-source.txt
>
> And I suppose this means we'd fall under this paragraph?
>
> "However if the devices have dedicated interrupt as the wakeup source
> then they need to specify/identify the same using device specific
> interrupt name. In such cases only that interrupt can be used as wakeup
> interrupt."
>
> We don't expect *any* interrupt to qualify as PCI WAKE#; so we should
> still also document the interrupt name ("wake"?) in
> Documentation/devicetree/bindings/pci/pci.txt as Bjorn suggested, in
> addition to using the 'wakeup-source' property documented there.
I believe the defined interrupt name is "wakeup" as example 1 shows.
Rob
On Mon, Aug 28, 2017 at 04:32:55PM -0500, Rob Herring wrote:
> On Fri, Aug 25, 2017 at 1:20 PM, Brian Norris <[email protected]> wrote:
> > On Fri, Aug 25, 2017 at 01:14:39PM -0500, Rob Herring wrote:
> >> Use the wakeup source binding:
> >> Documentation/devicetree/bindings/power/wakeup-source.txt
> >
> > And I suppose this means we'd fall under this paragraph?
> >
> > "However if the devices have dedicated interrupt as the wakeup source
> > then they need to specify/identify the same using device specific
> > interrupt name. In such cases only that interrupt can be used as wakeup
> > interrupt."
> >
> > We don't expect *any* interrupt to qualify as PCI WAKE#; so we should
> > still also document the interrupt name ("wake"?) in
> > Documentation/devicetree/bindings/pci/pci.txt as Bjorn suggested, in
> > addition to using the 'wakeup-source' property documented there.
>
> I believe the defined interrupt name is "wakeup" as example 1 shows.
That's an example, not a definition. And the definition I quoted
literally says "device specific interrupt name". The PCIe specification
calls it "WAKE#" all over the place, so I figured that's a good name to
use.
"wakeup" is also fine I suppose, as long as we document that it must be
PCIe WAKE# signal, as per the PCIe specfication.
Brian