Hello,
As part of an effort to bring suspend to RAM support to Armada 3700
SoCs (main target: ESPRESSObin), this series handles the work around
the PCIe IP.
First, more configuration is done in the 'setup' helper as inspired
from the U-Boot driver. This is needed to entirely initialize the IP
during future resume operation (patch 1).
Then, reset GPIO, PHY and clock support are introduced (patch 2-4). As
current device trees do not provide the corresponding properties, not
finding one of these properties is not an error and just produces a
warning. However, if the property is present, an error during PHY
initialization will fail the probe of the driver.
Note: To be sure the clock will be resumed before this driver, a first
series adding links between clocks and consumers has been submitted,
see [1]. Anyway, having the clock series applied first is not needed.
Patch 5 adds suspend/resume hooks, re-using all the above.
Finally, bindings and device trees are updated to reflect the hardware
(patch 6-12). While the clock depends on the SoC, the reset GPIO and
the PHY depends on the board so the clock is added in the
armada-37xx.dtsi file while the two other properties are added in
armada-3720-espressobin.dts.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-November/614527.html
Thanks,
Miquèl
Changes since v1:
=================
* Change the capitalization in commit titles to follow the PCI
subsystem rules.
* Added Suggested-by tag to the patch adding PHY support and to the
patch adding the PHY property in the DT.
* Added Rob's Reviewed-by tags on bindings.
* I am following the discussion about calling functions that might
sleep in a NOIRQ context. As there is no real problem yet (as per my
understanding), I did not change anything on this regard.
Miquel Raynal (12):
PCI: aardvark: Configure more registers in the configuration helper
PCI: aardvark: Add reset GPIO support
PCI: aardvark: Add PHY support
PCI: aardvark: Add clock support
PCI: aardvark: Add suspend to RAM support
dt-bindings: PCI: aardvark: Describe the reset-gpios property
dt-bindings: PCI: aardvark: Describe the clocks property
dt-bindings: PCI: aardvark: Describe the PHY property
ARM64: dts: marvell: armada-37xx: declare PCIe reset pin
ARM64: dts: marvell: armada-3720-espressobin: declare PCIe reset GPIO
ARM64: dts: marvell: armada-37xx: declare PCIe clock
ARM64: dts: marvell: armada-3720-espressobin: declare PCIe PHY
.../devicetree/bindings/pci/aardvark-pci.txt | 9 +
.../dts/marvell/armada-3720-espressobin.dts | 4 +
arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 5 +
drivers/pci/controller/pci-aardvark.c | 214 ++++++++++++++++++
4 files changed, 232 insertions(+)
--
2.19.1
Mimic U-Boot configuration to be sure all hardware registers are set
properly. This will be needed for future S2RAM operation.
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/pci/controller/pci-aardvark.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 750081c1cb48..b95eb2aa00bb 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -100,6 +100,8 @@
#define PCIE_CORE_CTRL2_STRICT_ORDER_ENABLE BIT(5)
#define PCIE_CORE_CTRL2_OB_WIN_ENABLE BIT(6)
#define PCIE_CORE_CTRL2_MSI_ENABLE BIT(10)
+#define PCIE_PHY_REFCLK (CONTROL_BASE_ADDR + 0x14)
+#define PCIE_PHY_REFCLK_BUF_CTRL 0x1342
#define PCIE_MSG_LOG_REG (CONTROL_BASE_ADDR + 0x30)
#define PCIE_ISR0_REG (CONTROL_BASE_ADDR + 0x40)
#define PCIE_MSG_PM_PME_MASK BIT(7)
@@ -243,6 +245,9 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
{
u32 reg;
+ /* Set HW Reference Clock Buffer Control */
+ advk_writel(pcie, PCIE_PHY_REFCLK_BUF_CTRL, PCIE_PHY_REFCLK);
+
/* Set to Direct mode */
reg = advk_readl(pcie, CTRL_CONFIG_REG);
reg &= ~(CTRL_MODE_MASK << CTRL_MODE_SHIFT);
@@ -274,6 +279,15 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
PCIE_CORE_CTRL2_TD_ENABLE;
advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
+ /* Set PCIe Device Control and Status 1 PF0 register */
+ reg = PCIE_CORE_DEV_CTRL_STATS_RELAX_ORDER_DISABLE |
+ PCIE_CORE_DEV_CTRL_STATS_SNOOP_DISABLE;
+ advk_writel(pcie, reg, PCIE_CORE_DEV_CTRL_STATS_REG);
+
+ /* Program PCIe Control 2 to disable strict ordering */
+ reg = PCIE_CORE_CTRL2_RESERVED | PCIE_CORE_CTRL2_TD_ENABLE;
+ advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
+
/* Set GEN2 */
reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
reg &= ~PCIE_GEN_SEL_MSK;
--
2.19.1
Document the possibility to reference a PHY.
Signed-off-by: Miquel Raynal <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
Documentation/devicetree/bindings/pci/aardvark-pci.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/aardvark-pci.txt b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
index c275c3e39cde..b41c69968e38 100644
--- a/Documentation/devicetree/bindings/pci/aardvark-pci.txt
+++ b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
@@ -24,6 +24,7 @@ contain the following properties:
The following are optional properties:
- reset-gpios: GPIO to reset the device
+ - phys: the PCIe PHY handle
In addition, the Device Tree describing an Aardvark PCIe controller
must include a sub-node that describes the legacy interrupt controller
@@ -55,6 +56,7 @@ Example:
<0 0 0 3 &pcie_intc 2>,
<0 0 0 4 &pcie_intc 3>;
reset-gpios = <&gpiosb 3 GPIO_ACTIVE_HIGH>;
+ phys = <&comphy1 0>;
pcie_intc: interrupt-controller {
interrupt-controller;
#interrupt-cells = <1>;
--
2.19.1
One GPIO can be muxed as PCIe reset.
Signed-off-by: Miquel Raynal <[email protected]>
---
arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 65bf774516ec..9f7e932c8144 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -295,6 +295,10 @@
function = "mii";
};
+ pcie_pins: pcie-pins {
+ groups = "pcie1";
+ function = "gpio";
+ };
};
eth0: ethernet@30000 {
--
2.19.1
Add a reset-gpios property to the PCIe node.
Signed-off-by: Miquel Raynal <[email protected]>
---
arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
index 094994a9c68e..76a508da80b9 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
@@ -46,6 +46,9 @@
/* J9 */
&pcie0 {
status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pcie_pins>;
+ reset-gpios = <&gpiosb 3 GPIO_ACTIVE_HIGH>;
};
/* J6 */
--
2.19.1
The PCIe IP is fed by a gated clock.
Signed-off-by: Miquel Raynal <[email protected]>
---
arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 9f7e932c8144..854b1d59b2ca 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -418,6 +418,7 @@
#address-cells = <3>;
#size-cells = <2>;
bus-range = <0x00 0xff>;
+ clocks = <&sb_periph_clk 13>;
interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
#interrupt-cells = <1>;
msi-parent = <&pcie0>;
--
2.19.1
Add suspend and resume callbacks. The priority of these are
"_noirq()", to workaround early access to the registers done by the
PCI core through the ->read()/->write() callbacks at resume time.
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/pci/controller/pci-aardvark.c | 52 +++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 108b3f15c410..7ecf1ac4036b 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1108,6 +1108,55 @@ static int advk_pcie_setup_clk(struct advk_pcie *pcie)
return ret;
}
+static int __maybe_unused advk_pcie_suspend(struct device *dev)
+{
+ struct advk_pcie *pcie = dev_get_drvdata(dev);
+
+ advk_pcie_disable_phy(pcie);
+
+ clk_disable_unprepare(pcie->clk);
+
+ return 0;
+}
+
+static int __maybe_unused advk_pcie_resume(struct device *dev)
+{
+ struct advk_pcie *pcie = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_prepare_enable(pcie->clk);
+ if (ret)
+ return ret;
+
+ /*
+ * Empirical delay needed after enabling the clock and before
+ * accessing any register.
+ */
+ msleep(10);
+
+ ret = advk_pcie_enable_phy(pcie);
+ if (ret)
+ return ret;
+
+ advk_pcie_hard_reset(pcie);
+
+ advk_pcie_setup_hw(pcie);
+
+ advk_sw_pci_bridge_init(pcie);
+
+ return 0;
+}
+
+/*
+ * The PCI core will try to reconfigure the bus quite early in the resume path.
+ * We must use the _noirq() alternatives to ensure the controller is ready when
+ * the core uses the ->read()/->write() callbacks.
+ */
+static const struct dev_pm_ops advk_pcie_dev_pm_ops = {
+ SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(advk_pcie_suspend,
+ advk_pcie_resume)
+};
+
static int advk_pcie_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1188,6 +1237,8 @@ static int advk_pcie_probe(struct platform_device *pdev)
return ret;
}
+ dev_set_drvdata(dev, pcie);
+
return 0;
}
@@ -1200,6 +1251,7 @@ static struct platform_driver advk_pcie_driver = {
.driver = {
.name = "advk-pcie",
.of_match_table = advk_pcie_of_match_table,
+ .pm = &advk_pcie_dev_pm_ops,
/* Driver unloading/unbinding currently not supported */
.suppress_bind_attrs = true,
},
--
2.19.1
The PCIe node is wired to the second PHY of the COMPHY IP.
Suggested-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Miquel Raynal <[email protected]>
---
arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
index 76a508da80b9..43e8e1edc467 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
@@ -49,6 +49,7 @@
pinctrl-names = "default";
pinctrl-0 = <&pcie_pins>;
reset-gpios = <&gpiosb 3 GPIO_ACTIVE_HIGH>;
+ phys = <&comphy1 0>;
};
/* J6 */
--
2.19.1
The IP relies on a gated clock. When we will add S2RAM support, this
clock will need to be resumed before any PCIe registers are
accessed. Add support for this clock.
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/pci/controller/pci-aardvark.c | 29 +++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index da695572a2ed..108b3f15c410 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -8,6 +8,7 @@
* Author: Hezi Shahmoon <[email protected]>
*/
+#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/gpio.h>
#include <linux/interrupt.h>
@@ -190,6 +191,7 @@
struct advk_pcie {
struct platform_device *pdev;
+ struct clk *clk;
void __iomem *base;
struct list_head resources;
struct irq_domain *irq_domain;
@@ -1083,6 +1085,29 @@ static int advk_pcie_setup_phy(struct advk_pcie *pcie)
return ret;
}
+static int advk_pcie_setup_clk(struct advk_pcie *pcie)
+{
+ struct device *dev = &pcie->pdev->dev;
+ int ret;
+
+ pcie->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(pcie->clk) && (PTR_ERR(pcie->clk) == -EPROBE_DEFER))
+ return PTR_ERR(pcie->clk);
+
+ /* Old bindings miss the clock handle */
+ if (IS_ERR(pcie->clk)) {
+ dev_warn(dev, "Clock unavailable (%ld)\n", PTR_ERR(pcie->clk));
+ pcie->clk = NULL;
+ return 0;
+ }
+
+ ret = clk_prepare_enable(pcie->clk);
+ if (ret)
+ dev_err(dev, "Clock initialization failed (%d)\n", ret);
+
+ return ret;
+}
+
static int advk_pcie_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1118,6 +1143,10 @@ static int advk_pcie_probe(struct platform_device *pdev)
return ret;
}
+ ret = advk_pcie_setup_clk(pcie);
+ if (ret)
+ return ret;
+
ret = advk_pcie_setup_phy(pcie);
if (ret)
return ret;
--
2.19.1
A GPIO might be used to reset the PCI IP. Describe the property needed
in this case.
Signed-off-by: Miquel Raynal <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
Documentation/devicetree/bindings/pci/aardvark-pci.txt | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/aardvark-pci.txt b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
index 310ef7145c47..252934237138 100644
--- a/Documentation/devicetree/bindings/pci/aardvark-pci.txt
+++ b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
@@ -20,6 +20,10 @@ contain the following properties:
define the mapping of the PCIe interface to interrupt numbers.
- bus-range: PCI bus numbers covered
+The following are optional properties:
+
+ - reset-gpios: GPIO to reset the device
+
In addition, the Device Tree describing an Aardvark PCIe controller
must include a sub-node that describes the legacy interrupt controller
built into the PCIe controller. This sub-node must have the following
@@ -48,6 +52,7 @@ Example:
<0 0 0 2 &pcie_intc 1>,
<0 0 0 3 &pcie_intc 2>,
<0 0 0 4 &pcie_intc 3>;
+ reset-gpios = <&gpiosb 3 GPIO_ACTIVE_HIGH>;
pcie_intc: interrupt-controller {
interrupt-controller;
#interrupt-cells = <1>;
--
2.19.1
The IP supports a reset GPIO. When S2RAM will be added, we must ensure
the reset line (if any) is deasserted when resuming. Add support for
it.
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/pci/controller/pci-aardvark.c | 57 +++++++++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index b95eb2aa00bb..1d31d74ddab7 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -9,6 +9,7 @@
*/
#include <linux/delay.h>
+#include <linux/gpio.h>
#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/irqdomain.h>
@@ -17,6 +18,7 @@
#include <linux/init.h>
#include <linux/platform_device.h>
#include <linux/of_address.h>
+#include <linux/of_gpio.h>
#include <linux/of_pci.h>
#include "../pci.h"
@@ -201,6 +203,7 @@ struct advk_pcie {
u16 msi_msg;
int root_bus_nr;
struct pci_bridge_emul bridge;
+ struct gpio_desc *reset_gpio;
};
static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)
@@ -973,6 +976,55 @@ static int advk_pcie_parse_request_of_pci_ranges(struct advk_pcie *pcie)
return err;
}
+static int advk_pcie_hard_reset(struct advk_pcie *pcie)
+{
+ if (!pcie->reset_gpio)
+ return -EINVAL;
+
+ gpiod_set_value_cansleep(pcie->reset_gpio, 0);
+ msleep(1);
+ gpiod_set_value_cansleep(pcie->reset_gpio, 1);
+
+ return 0;
+}
+
+static int advk_pcie_setup_reset_gpio(struct advk_pcie *pcie)
+{
+ struct device *dev = &pcie->pdev->dev;
+ enum of_gpio_flags of_flags;
+ unsigned long gpio_flags;
+ int gpio_nb;
+ int ret;
+
+ gpio_nb = of_get_named_gpio_flags(dev->of_node, "reset-gpios", 0,
+ &of_flags);
+ if (gpio_nb == -EPROBE_DEFER)
+ return gpio_nb;
+
+ /* Old bindings miss the reset GPIO handle */
+ if (!gpio_is_valid(gpio_nb)) {
+ dev_warn(dev, "Reset GPIO unavailable\n");
+ return 0;
+ }
+
+ if (of_flags & OF_GPIO_ACTIVE_LOW)
+ gpio_flags = GPIOF_ACTIVE_LOW |
+ GPIOF_OUT_INIT_LOW;
+ else
+ gpio_flags = GPIOF_OUT_INIT_HIGH;
+
+ ret = devm_gpio_request_one(dev, gpio_nb, gpio_flags,
+ "pcie-aardvark-reset");
+ if (ret) {
+ dev_err(dev, "Failed to retrieve reset GPIO (%d)\n", ret);
+ return ret;
+ }
+
+ pcie->reset_gpio = gpio_to_desc(gpio_nb);
+
+ return 0;
+}
+
static int advk_pcie_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1008,6 +1060,11 @@ static int advk_pcie_probe(struct platform_device *pdev)
return ret;
}
+ ret = advk_pcie_setup_reset_gpio(pcie);
+ if (ret)
+ return ret;
+
+ advk_pcie_hard_reset(pcie);
advk_pcie_setup_hw(pcie);
advk_sw_pci_bridge_init(pcie);
--
2.19.1
Describe the missing gated clock feeding the PCIe IP.
Signed-off-by: Miquel Raynal <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
Documentation/devicetree/bindings/pci/aardvark-pci.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/aardvark-pci.txt b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
index 252934237138..c275c3e39cde 100644
--- a/Documentation/devicetree/bindings/pci/aardvark-pci.txt
+++ b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
@@ -12,6 +12,7 @@ contain the following properties:
- #size-cells: set to <2>
- device_type: set to "pci"
- ranges: ranges for the PCI memory and I/O regions
+ - clocks: the clock feeding the IP
- #interrupt-cells: set to <1>
- msi-controller: indicates that the PCIe controller can itself
handle MSI interrupts
@@ -41,6 +42,7 @@ Example:
#address-cells = <3>;
#size-cells = <2>;
bus-range = <0x00 0xff>;
+ clocks = <&sb_periph_clk 13>;
interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
#interrupt-cells = <1>;
msi-controller;
--
2.19.1
The IP needs its PHY to be properly configured to work. While the PHY
is usually already configured by the bootloader, we will need this
feature when adding S2RAM support. Take care of registering and
configuring the PHY from the driver itself.
Suggested-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/pci/controller/pci-aardvark.c | 62 +++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 1d31d74ddab7..da695572a2ed 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -17,6 +17,7 @@
#include <linux/pci.h>
#include <linux/init.h>
#include <linux/platform_device.h>
+#include <linux/phy/phy.h>
#include <linux/of_address.h>
#include <linux/of_gpio.h>
#include <linux/of_pci.h>
@@ -204,6 +205,7 @@ struct advk_pcie {
int root_bus_nr;
struct pci_bridge_emul bridge;
struct gpio_desc *reset_gpio;
+ struct phy *phy;
};
static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)
@@ -1025,6 +1027,62 @@ static int advk_pcie_setup_reset_gpio(struct advk_pcie *pcie)
return 0;
}
+static void advk_pcie_disable_phy(struct advk_pcie *pcie)
+{
+ phy_power_off(pcie->phy);
+ phy_exit(pcie->phy);
+}
+
+static int advk_pcie_enable_phy(struct advk_pcie *pcie)
+{
+ int ret;
+
+ if (!pcie->phy)
+ return 0;
+
+ ret = phy_init(pcie->phy);
+ if (ret)
+ return ret;
+
+ ret = phy_set_mode(pcie->phy, PHY_MODE_PCIE);
+ if (ret) {
+ phy_exit(pcie->phy);
+ return ret;
+ }
+
+ ret = phy_power_on(pcie->phy);
+ if (ret) {
+ phy_exit(pcie->phy);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int advk_pcie_setup_phy(struct advk_pcie *pcie)
+{
+ struct device *dev = &pcie->pdev->dev;
+ struct device_node *node = dev->of_node;
+ int ret = 0;
+
+ pcie->phy = devm_of_phy_get(dev, node, NULL);
+ if (IS_ERR(pcie->phy) && (PTR_ERR(pcie->phy) == -EPROBE_DEFER))
+ return PTR_ERR(pcie->phy);
+
+ /* Old bindings miss the PHY handle */
+ if (IS_ERR(pcie->phy)) {
+ dev_warn(dev, "PHY unavailable (%ld)\n", PTR_ERR(pcie->phy));
+ pcie->phy = NULL;
+ return 0;
+ }
+
+ ret = advk_pcie_enable_phy(pcie);
+ if (ret)
+ dev_err(dev, "Failed to initialize PHY (%d)\n", ret);
+
+ return ret;
+}
+
static int advk_pcie_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1060,6 +1118,10 @@ static int advk_pcie_probe(struct platform_device *pdev)
return ret;
}
+ ret = advk_pcie_setup_phy(pcie);
+ if (ret)
+ return ret;
+
ret = advk_pcie_setup_reset_gpio(pcie);
if (ret)
return ret;
--
2.19.1
Hello,
Miquel Raynal <[email protected]> wrote on Wed, 12 Dec 2018
11:21:40 +0100:
> Add a reset-gpios property to the PCIe node.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> index 094994a9c68e..76a508da80b9 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> @@ -46,6 +46,9 @@
> /* J9 */
> &pcie0 {
> status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pcie_pins>;
> + reset-gpios = <&gpiosb 3 GPIO_ACTIVE_HIGH>;
> };
>
> /* J6 */
While this change may be fine for platforms based on Armada 3700 SoC,
it is not for the EspressoBin that has no reset GPIO for PCIe and
instead uses this pin to control the Ethenet switch.
I will re-send a series without this patch. I think it does not hurt to
keep the previous patch adding the pinmux setting in the
Armada-37xx.dtsi file even without using it, so I will drop only this
patch.
Thanks,
Miquèl
Hello,
On Thu, 13 Dec 2018 15:33:06 +0100, Miquel Raynal wrote:
> I will re-send a series without this patch. I think it does not hurt to
> keep the previous patch adding the pinmux setting in the
> Armada-37xx.dtsi file even without using it, so I will drop only this
> patch.
I tend to disagree here (but perhaps you'll have other arguments to
convince me otherwise): the GPIO used for PCIe reset is a completely
board-specific thing. You can chose whatever GPIO you want, and each
board can be different. Therefore, there is no reason to have such a
pinmux configuration at the SoC level (.dtsi), it should be within the
particular board that uses that pinmux configuration.
This is a rule that we have applied to mvebu platforms in general, and
which I believe is fairly common in many DTs.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Miquel,
are there already patches for the A37xx comphy driver?
On Wed, 12 Dec 2018 11:21:33 +0100
Miquel Raynal <[email protected]> wrote:
> The IP needs its PHY to be properly configured to work. While the PHY
> is usually already configured by the bootloader, we will need this
> feature when adding S2RAM support. Take care of registering and
> configuring the PHY from the driver itself.
>
> Suggested-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/pci/controller/pci-aardvark.c | 62
> +++++++++++++++++++++++++++ 1 file changed, 62 insertions(+)
>
> diff --git a/drivers/pci/controller/pci-aardvark.c
> b/drivers/pci/controller/pci-aardvark.c index
> 1d31d74ddab7..da695572a2ed 100644 ---
> a/drivers/pci/controller/pci-aardvark.c +++
> b/drivers/pci/controller/pci-aardvark.c @@ -17,6 +17,7 @@
> #include <linux/pci.h>
> #include <linux/init.h>
> #include <linux/platform_device.h>
> +#include <linux/phy/phy.h>
> #include <linux/of_address.h>
> #include <linux/of_gpio.h>
> #include <linux/of_pci.h>
> @@ -204,6 +205,7 @@ struct advk_pcie {
> int root_bus_nr;
> struct pci_bridge_emul bridge;
> struct gpio_desc *reset_gpio;
> + struct phy *phy;
> };
>
> static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64
> reg) @@ -1025,6 +1027,62 @@ static int
> advk_pcie_setup_reset_gpio(struct advk_pcie *pcie) return 0;
> }
>
> +static void advk_pcie_disable_phy(struct advk_pcie *pcie)
> +{
> + phy_power_off(pcie->phy);
> + phy_exit(pcie->phy);
> +}
> +
> +static int advk_pcie_enable_phy(struct advk_pcie *pcie)
> +{
> + int ret;
> +
> + if (!pcie->phy)
> + return 0;
> +
> + ret = phy_init(pcie->phy);
> + if (ret)
> + return ret;
> +
> + ret = phy_set_mode(pcie->phy, PHY_MODE_PCIE);
> + if (ret) {
> + phy_exit(pcie->phy);
> + return ret;
> + }
> +
> + ret = phy_power_on(pcie->phy);
> + if (ret) {
> + phy_exit(pcie->phy);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int advk_pcie_setup_phy(struct advk_pcie *pcie)
> +{
> + struct device *dev = &pcie->pdev->dev;
> + struct device_node *node = dev->of_node;
> + int ret = 0;
> +
> + pcie->phy = devm_of_phy_get(dev, node, NULL);
> + if (IS_ERR(pcie->phy) && (PTR_ERR(pcie->phy) ==
> -EPROBE_DEFER))
> + return PTR_ERR(pcie->phy);
> +
> + /* Old bindings miss the PHY handle */
> + if (IS_ERR(pcie->phy)) {
> + dev_warn(dev, "PHY unavailable (%ld)\n",
> PTR_ERR(pcie->phy));
> + pcie->phy = NULL;
> + return 0;
> + }
> +
> + ret = advk_pcie_enable_phy(pcie);
> + if (ret)
> + dev_err(dev, "Failed to initialize PHY (%d)\n", ret);
> +
> + return ret;
> +}
> +
> static int advk_pcie_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -1060,6 +1118,10 @@ static int advk_pcie_probe(struct
> platform_device *pdev) return ret;
> }
>
> + ret = advk_pcie_setup_phy(pcie);
> + if (ret)
> + return ret;
> +
> ret = advk_pcie_setup_reset_gpio(pcie);
> if (ret)
> return ret;
On Fri, 14 Dec 2018 01:47:01 +0100
Marek Behun <[email protected]> wrote:
> Hi Miquel,
> are there already patches for the A37xx comphy driver?
Never mind, I found them. I shall test this on Turris Mox.
Is the comphy driver supposed to be able to change eth mode from
1000BASE-X to 2500BASE-X and back without reboot?
This would be cool for our SFP cage module of Turris Mox, just to add
support for comphy into mvneta.
Marek
Hi Marek,
Marek Behun <[email protected]> wrote on Fri, 14 Dec 2018 01:57:12
+0100:
> On Fri, 14 Dec 2018 01:47:01 +0100
> Marek Behun <[email protected]> wrote:
>
> > Hi Miquel,
> > are there already patches for the A37xx comphy driver?
Please try the latest patches on top of phy -next (with PHY interface
mode), available there [1]. You can pick only the COMPHY patches, but
keep them over the phy -next branch.
Please note that for the SMC calls to succeed you must use a recent
ATF. Personally I used to work with the 'atf-v1.5-devel' branch of
Marvell's misl-atf repository but it is not available anymore. If you
already have a clone, then you are good to go, otherwise it might be
great if someone from Marvell could share a public Github link?
> Never mind, I found them. I shall test this on Turris Mox.
> Is the comphy driver supposed to be able to change eth mode from
> 1000BASE-X to 2500BASE-X and back without reboot?
In theory yes, unfortunately I am working on an EspressoBin which uses
the SATA, PCIe and SATA configurations of the COMPHY, but not the
Ethernet ones (SGMII/1000BASE-X and HS_SGMII/2500BASE-X). So the
support is ready to be tested!
>
> This would be cool for our SFP cage module of Turris Mox, just to add
> support for comphy into mvneta.
>
> Marek
[1] https://github.com/miquelraynal/linux.git branch marvell/phy-next/pm
Thanks,
Miquèl
Hi Thomas,
Thomas Petazzoni <[email protected]> wrote on Thu, 13 Dec
2018 15:36:19 +0100:
> Hello,
>
> On Thu, 13 Dec 2018 15:33:06 +0100, Miquel Raynal wrote:
>
> > I will re-send a series without this patch. I think it does not hurt to
> > keep the previous patch adding the pinmux setting in the
> > Armada-37xx.dtsi file even without using it, so I will drop only this
> > patch.
>
> I tend to disagree here (but perhaps you'll have other arguments to
> convince me otherwise): the GPIO used for PCIe reset is a completely
> board-specific thing. You can chose whatever GPIO you want, and each
> board can be different. Therefore, there is no reason to have such a
> pinmux configuration at the SoC level (.dtsi), it should be within the
> particular board that uses that pinmux configuration.
>
> This is a rule that we have applied to mvebu platforms in general, and
> which I believe is fairly common in many DTs.
Actually this is a pin that may be driven directly by the PCI IP and is
not board-specific (note that the patch is wrong as the functions
should be "pcie" instead of "gpio"). What is board specific is if this
pin is actually wired to the endpoint PCIe card or not.
Anyway, as seen by Gregory, the pinctrl driver must be fixed as when
selecting the "pcie1" group, the driver was poking another area making
the EspressoBin switch unstable. With a quick fix on my side I realized
the reset was not behaving at all as expected. As it is not actually
needed for suspend/resume operation (at least on my setup) I will drop
the 'reset pin' related patches in the next iteration of the series.
Thanks,
Miquèl
Miquel,
I tried the patches and they are working, with the exception of Compex
WLE900X card, but we know that this card is problematic.
I am interesting if there is a known way to turn of the comphy on
A3720, or at least change the SGMII mode from 1Gbps to 2.5Gbps and
back. Marvell documentation does not provide this information and the
code in ATF providing the SMC calls does not do this either.
Do you know how this can be achieved?
Thanks,
Marek
On Mon, 17 Dec 2018 17:07:24 +0100
Miquel Raynal <[email protected]> wrote:
> Hi Marek,
>
> Marek Behun <[email protected]> wrote on Fri, 14 Dec 2018 01:57:12
> +0100:
>
> > On Fri, 14 Dec 2018 01:47:01 +0100
> > Marek Behun <[email protected]> wrote:
> >
> > > Hi Miquel,
> > > are there already patches for the A37xx comphy driver?
>
> Please try the latest patches on top of phy -next (with PHY interface
> mode), available there [1]. You can pick only the COMPHY patches, but
> keep them over the phy -next branch.
>
> Please note that for the SMC calls to succeed you must use a recent
> ATF. Personally I used to work with the 'atf-v1.5-devel' branch of
> Marvell's misl-atf repository but it is not available anymore. If you
> already have a clone, then you are good to go, otherwise it might be
> great if someone from Marvell could share a public Github link?
>
> > Never mind, I found them. I shall test this on Turris Mox.
> > Is the comphy driver supposed to be able to change eth mode from
> > 1000BASE-X to 2500BASE-X and back without reboot?
>
> In theory yes, unfortunately I am working on an EspressoBin which uses
> the SATA, PCIe and SATA configurations of the COMPHY, but not the
> Ethernet ones (SGMII/1000BASE-X and HS_SGMII/2500BASE-X). So the
> support is ready to be tested!
>
> >
> > This would be cool for our SFP cage module of Turris Mox, just to
> > add support for comphy into mvneta.
> >
> > Marek
>
> [1] https://github.com/miquelraynal/linux.git branch
> marvell/phy-next/pm
>
>
> Thanks,
> Miquèl
Hi Miquel,
Miquel Raynal writes:
> Marek Behun <[email protected]> wrote on Fri, 14 Dec 2018 01:57:12
> +0100:
>
>> On Fri, 14 Dec 2018 01:47:01 +0100
>> Marek Behun <[email protected]> wrote:
>>
>> > are there already patches for the A37xx comphy driver?
>
> Please try the latest patches on top of phy -next (with PHY interface
> mode), available there [1]. You can pick only the COMPHY patches, but
> keep them over the phy -next branch.
>
> Please note that for the SMC calls to succeed you must use a recent
> ATF. Personally I used to work with the 'atf-v1.5-devel' branch of
> Marvell's misl-atf repository but it is not available anymore. If you
> already have a clone, then you are good to go, otherwise it might be
> great if someone from Marvell could share a public Github link?
The latest ATF from Marvell is at the 'atf-v1.5-armada-18.09' branch at
https://github.com/MarvellEmbeddedProcessors/atf-marvell
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.52.368.4656, http://www.tkos.co.il -
Hi Baruch,
Baruch Siach <[email protected]> wrote on Mon, 17 Dec 2018 20:27:42
+0200:
> Hi Miquel,
>
> Miquel Raynal writes:
> > Marek Behun <[email protected]> wrote on Fri, 14 Dec 2018 01:57:12
> > +0100:
> >
> >> On Fri, 14 Dec 2018 01:47:01 +0100
> >> Marek Behun <[email protected]> wrote:
> >>
> >> > are there already patches for the A37xx comphy driver?
> >
> > Please try the latest patches on top of phy -next (with PHY interface
> > mode), available there [1]. You can pick only the COMPHY patches, but
> > keep them over the phy -next branch.
> >
> > Please note that for the SMC calls to succeed you must use a recent
> > ATF. Personally I used to work with the 'atf-v1.5-devel' branch of
> > Marvell's misl-atf repository but it is not available anymore. If you
> > already have a clone, then you are good to go, otherwise it might be
> > great if someone from Marvell could share a public Github link?
>
> The latest ATF from Marvell is at the 'atf-v1.5-armada-18.09' branch at
>
> https://github.com/MarvellEmbeddedProcessors/atf-marvell
Indeed, but I don't see A3700 COMPHY support there (only CP110's), I
think it will be merged into 18.12 release (will happen this month).
Thanks,
Miquèl
Hi Marek,
Marek Behun <[email protected]> wrote on Mon, 17 Dec 2018 22:34:30
+0100:
> Miquel,
>
> I tried the patches and they are working, with the exception of Compex
> WLE900X card, but we know that this card is problematic.
How did you test them? I am surprised that COMPHY calls worked for you
out of the box. I am not sure you have A3700 COMPHY support in your ATF
but it probably work because U-Boot is doing the initial configuration.
> I am interesting if there is a known way to turn of the comphy on
> A3720, or at least change the SGMII mode from 1Gbps to 2.5Gbps and
> back. Marvell documentation does not provide this information and the
> code in ATF providing the SMC calls does not do this either.
> Do you know how this can be achieved?
The driver already handles these calls (see [1]), making use of
them is just a matter of hacking into drivers (in your case, I
suppose it is mvneta) and add phy_set_mode()/phy_power_on() calls.
[1] https://github.com/miquelraynal/linux/blob/marvell/phy-next/pm/drivers/phy/marvell/phy-mvebu-a3700-comphy.c#L189
Thanks,
Miquèl
Hello,
Miquel Raynal <[email protected]> wrote on Tue, 18 Dec 2018
09:18:17 +0100:
> Hi Marek,
>
> Marek Behun <[email protected]> wrote on Mon, 17 Dec 2018 22:34:30
> +0100:
>
> > Miquel,
> >
> > I tried the patches and they are working, with the exception of Compex
> > WLE900X card, but we know that this card is problematic.
>
> How did you test them? I am surprised that COMPHY calls worked for you
> out of the box. I am not sure you have A3700 COMPHY support in your ATF
> but it probably work because U-Boot is doing the initial configuration.
Let me correct myself: actually the feature is already mainline! See
[2]. It will be added to Marvell's BSP by the end of the year. So if
you are running a recent mainline ATF you are good to go!
>
> > I am interesting if there is a known way to turn of the comphy on
> > A3720, or at least change the SGMII mode from 1Gbps to 2.5Gbps and
> > back. Marvell documentation does not provide this information and the
> > code in ATF providing the SMC calls does not do this either.
> > Do you know how this can be achieved?
>
> The driver already handles these calls (see [1]), making use of
> them is just a matter of hacking into drivers (in your case, I
> suppose it is mvneta) and add phy_set_mode()/phy_power_on() calls.
>
> [1] https://github.com/miquelraynal/linux/blob/marvell/phy-next/pm/drivers/phy/marvell/phy-mvebu-a3700-comphy.c#L189
>
>
> Thanks,
> Miquèl
[2] https://github.com/ARM-software/arm-trusted-firmware/blob/master/drivers/marvell/comphy/phy-comphy-3700.c
Thanks,
Miquèl
> [2]
> https://github.com/ARM-software/arm-trusted-firmware/blob/master/drivers/marvell/comphy/phy-comphy-3700.c
Yes, I used mainline atf (it did not work out of the box with 18.09
atf-marvell of course). But there is no _power_off function for SGMII,
nor a digital_reset function like in cp110 implementation.
Marek
Hi Marek,
Marek Behun <[email protected]> wrote on Tue, 18 Dec 2018 14:09:20
+0100:
> > [2]
> > https://github.com/ARM-software/arm-trusted-firmware/blob/master/drivers/marvell/comphy/phy-comphy-3700.c
>
> Yes, I used mainline atf (it did not work out of the box with 18.09
> atf-marvell of course). But there is no _power_off function for SGMII,
> nor a digital_reset function like in cp110 implementation.
Indeed, but why would you need one? Just use the helpers from the core
and if there is no implementation, nothing should happen and the helper
should exit without error. Just call phy_set_mode()/phy_power_on() an
you should be good.
Thanks,
Miquèl
Hi,
One thing for which I would like to be able to disable comphy is that
each consumes about 100mW of power. On Turris Mox we configure the
comphys to SGMII1, PCIe and USB3 modes. If there is no USB device
plugged, the USB3 phy can be disabled, and save 100mW of power. If the
PCIe extension module is not present, the PCIe can too be disabled, and
if there is no switch nor SFP module present, so can SGMII1.
The other reason is this: if the SGMII phy is set to 1G mode, and then
powered on second time in 2.5G mode, will it work? I would like to
patch mvneta driver to power on/off the comphy, if the device node is
present in device tree. But then the system can request such a change
(SGMII to 2500BASE-X or back).
Marek
On Tue, 18 Dec 2018 14:41:30 +0100
Miquel Raynal <[email protected]> wrote:
> Hi Marek,
>
> Marek Behun <[email protected]> wrote on Tue, 18 Dec 2018 14:09:20
> +0100:
>
> > > [2]
> > > https://github.com/ARM-software/arm-trusted-firmware/blob/master/drivers/marvell/comphy/phy-comphy-3700.c
> >
> > Yes, I used mainline atf (it did not work out of the box with 18.09
> > atf-marvell of course). But there is no _power_off function for
> > SGMII, nor a digital_reset function like in cp110 implementation.
>
> Indeed, but why would you need one? Just use the helpers from the core
> and if there is no implementation, nothing should happen and the
> helper should exit without error. Just call
> phy_set_mode()/phy_power_on() an you should be good.
>
>
> Thanks,
> Miquèl
Hi Marek,
Marek Behún <[email protected]> wrote on Wed, 19 Dec 2018 16:28:35
+0100:
> Hi,
>
> One thing for which I would like to be able to disable comphy is that
> each consumes about 100mW of power. On Turris Mox we configure the
> comphys to SGMII1, PCIe and USB3 modes. If there is no USB device
> plugged, the USB3 phy can be disabled, and save 100mW of power. If the
> PCIe extension module is not present, the PCIe can too be disabled, and
> if there is no switch nor SFP module present, so can SGMII1.
Indeed not all PHY types implement ->power_off() (see in ATF code).
Better ask Marvell directly for that.
>
> The other reason is this: if the SGMII phy is set to 1G mode, and then
> powered on second time in 2.5G mode, will it work? I would like to
This should work, yes.
> patch mvneta driver to power on/off the comphy, if the device node is
> present in device tree. But then the system can request such a change
> (SGMII to 2500BASE-X or back).
>
> Marek
>
> On Tue, 18 Dec 2018 14:41:30 +0100
> Miquel Raynal <[email protected]> wrote:
>
> > Hi Marek,
> >
> > Marek Behun <[email protected]> wrote on Tue, 18 Dec 2018 14:09:20
> > +0100:
> >
> > > > [2]
> > > > https://github.com/ARM-software/arm-trusted-firmware/blob/master/drivers/marvell/comphy/phy-comphy-3700.c
> > >
> > > Yes, I used mainline atf (it did not work out of the box with 18.09
> > > atf-marvell of course). But there is no _power_off function for
> > > SGMII, nor a digital_reset function like in cp110 implementation.
> >
> > Indeed, but why would you need one? Just use the helpers from the core
> > and if there is no implementation, nothing should happen and the
> > helper should exit without error. Just call
> > phy_set_mode()/phy_power_on() an you should be good.
> >
> >
> > Thanks,
> > Miquèl
>
For the record, I found out what was wrong in my code, toggling the
reset lines do not produce random effects anymore.
Thanks,
Miquèl