Hello,
this is the fourth version of the patch series for Armada 3720 PCIe
controller (aardvark). It's main purpose is to fix some bugs regarding
buggy ath10k cards, but we also found out some suspicious stuff about
the driver and the SOC itself, which we try to address.
Patches are available also in my git branch pci-aardvark:
https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=pci-aardvark
Changes since v3:
- do not change return value of of_pci_get_max_link_speed() function
- mark zero 'max-link-speed' as invalid
- silently use gen3 speed when 'max-link-speed' as invalid
Changes since v2:
- move PCIe max-link-speed property to armada-37xx.dtsi
- replace custom macros by standard linux/pci_regs.h macros
- increase PERST delay to 10ms (needed for initialized Compex WLE900VX)
- disable link training before PERST (needed for Compex WLE900VX)
- change of_pci_get_max_link_speed() function to signal -ENOENT
- handle errors from of_pci_get_max_link_speed() function
- updated comments, commit titles and messages
Changes since v1:
- commit titles and messages were reviewed and some of them were rewritten
- patches 1 and 5 from v1 which touch PCIe speed configuration were
reworked into one patch
- patch 2 from v1 was removed, it is not needed anymore
- patch 7 from v1 now touches the device tree of armada-3720-db
- a patch was added that tries to enable PCIe PHY via generic-phy API
(if a phandle to the PHY is found in the device tree)
- a patch describing the new PCIe node DT properties was added
- a patch was added that moves the PHY phandle from board device trees
to armada-37xx.dtsi
Marek and Pali
Marek Behún (5):
PCI: aardvark: Improve link training
PCI: aardvark: Add PHY support
dt-bindings: PCI: aardvark: Describe new properties
arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property
Pali Rohár (7):
PCI: aardvark: Train link immediately after enabling training
PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
read-only register
PCI: of: Zero max-link-speed value is invalid
PCI: aardvark: Issue PERST via GPIO
PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
macros
arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property
.../devicetree/bindings/pci/aardvark-pci.txt | 4 +
.../arm64/boot/dts/marvell/armada-3720-db.dts | 3 +
.../dts/marvell/armada-3720-espressobin.dtsi | 2 +-
.../dts/marvell/armada-3720-turris-mox.dts | 6 -
arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 4 +-
drivers/pci/controller/pci-aardvark.c | 263 +++++++++++++++---
drivers/pci/of.c | 2 +-
7 files changed, 231 insertions(+), 53 deletions(-)
--
2.20.1
Adding even 100ms (PCI_PM_D3COLD_WAIT) delay between enabling link
training and starting link training causes detection issues with some
buggy cards (such as Compex WLE900VX).
Move the code which enables link training immediately before the one
which starts link traning.
This fixes detection issues of Compex WLE900VX card on Turris MOX after
cold boot.
Fixes: f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready...")
Signed-off-by: Pali Rohár <[email protected]>
---
drivers/pci/controller/pci-aardvark.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 2a20b649f40c..f9955b494267 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -300,11 +300,6 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
reg |= LANE_COUNT_1;
advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
- /* Enable link training */
- reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
- reg |= LINK_TRAINING_EN;
- advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
-
/* Enable MSI */
reg = advk_readl(pcie, PCIE_CORE_CTRL2_REG);
reg |= PCIE_CORE_CTRL2_MSI_ENABLE;
@@ -346,7 +341,15 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
*/
msleep(PCI_PM_D3COLD_WAIT);
- /* Start link training */
+ /* Enable link training */
+ reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
+ reg |= LINK_TRAINING_EN;
+ advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
+
+ /*
+ * Start link training immediately after enabling it.
+ * This solves problems for some buggy cards.
+ */
reg = advk_readl(pcie, PCIE_CORE_LINK_CTRL_STAT_REG);
reg |= PCIE_CORE_LINK_TRAINING;
advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
--
2.20.1
Interpret zero value of max-link-speed property as invalid,
as the device tree bindings documentation specifies.
Signed-off-by: Pali Rohár <[email protected]>
---
drivers/pci/of.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 81ceeaa6f1d5..27839cd2459f 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -592,7 +592,7 @@ int of_pci_get_max_link_speed(struct device_node *node)
u32 max_link_speed;
if (of_property_read_u32(node, "max-link-speed", &max_link_speed) ||
- max_link_speed > 4)
+ max_link_speed == 0 || max_link_speed > 4)
return -EINVAL;
return max_link_speed;
--
2.20.1
From: Marek Behún <[email protected]>
Currently the aardvark driver trains link in PCIe gen2 mode. This may
cause some buggy gen1 cards (such as Compex WLE900VX) to be unstable or
even not detected. Moreover when ASPM code tries to retrain link second
time, these cards may stop responding and link goes down. If gen1 is
used this does not happen.
Unconditionally forcing gen1 is not a good solution since it may have
performance impact on gen2 cards.
To overcome this, read 'max-link-speed' property (as defined in PCI
device tree bindings) and use this as max gen mode. Then iteratively try
link training at this mode or lower until successful. After successful
link training choose final controller gen based on Negotiated Link Speed
from Link Status register, which should match card speed.
Signed-off-by: Pali Rohár <[email protected]>
Signed-off-by: Marek Behún <[email protected]>
---
drivers/pci/controller/pci-aardvark.c | 114 ++++++++++++++++++++------
1 file changed, 89 insertions(+), 25 deletions(-)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 74b90940a9d4..e202f954eb84 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -40,6 +40,7 @@
#define PCIE_CORE_LINK_CTRL_STAT_REG 0xd0
#define PCIE_CORE_LINK_L0S_ENTRY BIT(0)
#define PCIE_CORE_LINK_TRAINING BIT(5)
+#define PCIE_CORE_LINK_SPEED_SHIFT 16
#define PCIE_CORE_LINK_WIDTH_SHIFT 20
#define PCIE_CORE_ERR_CAPCTL_REG 0x118
#define PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX BIT(5)
@@ -201,6 +202,7 @@ struct advk_pcie {
struct mutex msi_used_lock;
u16 msi_msg;
int root_bus_nr;
+ int link_gen;
struct pci_bridge_emul bridge;
};
@@ -225,20 +227,16 @@ static int advk_pcie_link_up(struct advk_pcie *pcie)
static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
{
- struct device *dev = &pcie->pdev->dev;
int retries;
/* check if the link is up or not */
for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
- if (advk_pcie_link_up(pcie)) {
- dev_info(dev, "link up\n");
+ if (advk_pcie_link_up(pcie))
return 0;
- }
usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
}
- dev_err(dev, "link never came up\n");
return -ETIMEDOUT;
}
@@ -253,6 +251,85 @@ static void advk_pcie_wait_for_retrain(struct advk_pcie *pcie)
}
}
+static int advk_pcie_train_at_gen(struct advk_pcie *pcie, int gen)
+{
+ int ret, neg_gen;
+ u32 reg;
+
+ /* Setup link speed */
+ reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
+ reg &= ~PCIE_GEN_SEL_MSK;
+ if (gen == 3)
+ reg |= SPEED_GEN_3;
+ else if (gen == 2)
+ reg |= SPEED_GEN_2;
+ else
+ reg |= SPEED_GEN_1;
+ advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
+
+ /*
+ * Enable link training. This is not needed in every call to this
+ * function, just once suffices, but it does not break anything either.
+ */
+ reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
+ reg |= LINK_TRAINING_EN;
+ advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
+
+ /*
+ * Start link training immediately after enabling it.
+ * This solves problems for some buggy cards.
+ */
+ reg = advk_readl(pcie, PCIE_CORE_LINK_CTRL_STAT_REG);
+ reg |= PCIE_CORE_LINK_TRAINING;
+ advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
+
+ ret = advk_pcie_wait_for_link(pcie);
+ if (ret)
+ return ret;
+
+ reg = advk_readl(pcie, PCIE_CORE_LINK_CTRL_STAT_REG);
+ neg_gen = (reg >> PCIE_CORE_LINK_SPEED_SHIFT) & 0xf;
+
+ return neg_gen;
+}
+
+static void advk_pcie_train_link(struct advk_pcie *pcie)
+{
+ struct device *dev = &pcie->pdev->dev;
+ int neg_gen = -1, gen;
+
+ /*
+ * Try link training at link gen specified by device tree property
+ * 'max-link-speed'. If this fails, iteratively train at lower gen.
+ */
+ for (gen = pcie->link_gen; gen > 0; --gen) {
+ neg_gen = advk_pcie_train_at_gen(pcie, gen);
+ if (neg_gen > 0)
+ break;
+ }
+
+ if (neg_gen < 0)
+ goto err;
+
+ /*
+ * After successful training if negotiated gen is lower than requested,
+ * train again on negotiated gen. This solves some stability issues for
+ * some buggy gen1 cards.
+ */
+ if (neg_gen < gen) {
+ gen = neg_gen;
+ neg_gen = advk_pcie_train_at_gen(pcie, gen);
+ }
+
+ if (neg_gen == gen) {
+ dev_info(dev, "link up at gen %i\n", gen);
+ return;
+ }
+
+err:
+ dev_err(dev, "link never came up\n");
+}
+
static void advk_pcie_setup_hw(struct advk_pcie *pcie)
{
u32 reg;
@@ -288,12 +365,6 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
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;
- reg |= SPEED_GEN_2;
- advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
-
/* Set lane X1 */
reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
reg &= ~LANE_CNT_MSK;
@@ -341,20 +412,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
*/
msleep(PCI_PM_D3COLD_WAIT);
- /* Enable link training */
- reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
- reg |= LINK_TRAINING_EN;
- advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
-
- /*
- * Start link training immediately after enabling it.
- * This solves problems for some buggy cards.
- */
- reg = advk_readl(pcie, PCIE_CORE_LINK_CTRL_STAT_REG);
- reg |= PCIE_CORE_LINK_TRAINING;
- advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
-
- advk_pcie_wait_for_link(pcie);
+ advk_pcie_train_link(pcie);
reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
reg |= PCIE_CORE_CMD_MEM_ACCESS_EN |
@@ -988,6 +1046,12 @@ static int advk_pcie_probe(struct platform_device *pdev)
}
pcie->root_bus_nr = bus->start;
+ ret = of_pci_get_max_link_speed(dev->of_node);
+ if (ret <= 0 || ret > 3)
+ pcie->link_gen = 3;
+ else
+ pcie->link_gen = ret;
+
advk_pcie_setup_hw(pcie);
advk_sw_pci_bridge_init(pcie);
--
2.20.1
This register is applicable only when the controller is configured for
Endpoint mode, which is not the case for the current version of this
driver.
Attempting to remove this code though caused some ath10k cards to stop
working, so for some unknown reason it is needed here.
This should be investigated and a comment explaining this should be put
before the code, so we add a FIXME comment for now.
Signed-off-by: Pali Rohár <[email protected]>
---
drivers/pci/controller/pci-aardvark.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 2ecc79c03ade..8332c71d69fa 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -439,6 +439,13 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
advk_pcie_train_link(pcie);
+ /*
+ * FIXME: The following register update is suspicious. This register is
+ * applicable only when the PCI controller is configured for Endpoint
+ * mode, not as a Root Complex. But apparently when this code is
+ * removed, some cards stop working. This should be investigated and
+ * a comment explaining this should be put here.
+ */
reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
reg |= PCIE_CORE_CMD_MEM_ACCESS_EN |
PCIE_CORE_CMD_IO_ACCESS_EN |
--
2.20.1
From: Marek Behún <[email protected]>
Document the possibility to reference a PHY and reset-gpios and to set
max-link-speed property.
Signed-off-by: Marek Behún <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: [email protected]
---
Documentation/devicetree/bindings/pci/aardvark-pci.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/aardvark-pci.txt b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
index 310ef7145c47..2b8ca920a7fa 100644
--- a/Documentation/devicetree/bindings/pci/aardvark-pci.txt
+++ b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
@@ -19,6 +19,9 @@ contain the following properties:
- interrupt-map-mask and interrupt-map: standard PCI properties to
define the mapping of the PCIe interface to interrupt numbers.
- bus-range: PCI bus numbers covered
+ - phys: the PCIe PHY handle
+ - max-link-speed: see pci.txt
+ - reset-gpios: see pci.txt
In addition, the Device Tree describing an Aardvark PCIe controller
must include a sub-node that describes the legacy interrupt controller
@@ -48,6 +51,7 @@ Example:
<0 0 0 2 &pcie_intc 1>,
<0 0 0 3 &pcie_intc 2>,
<0 0 0 4 &pcie_intc 3>;
+ phys = <&comphy1 0>;
pcie_intc: interrupt-controller {
interrupt-controller;
#interrupt-cells = <1>;
--
2.20.1
Trying to change Link Status register does not have any effect as this
is a read-only register. Trying to overwrite bits for Negotiated Link
Width does not make sense.
In future proper change of link width can be done via Lane Count Select
bits in PCIe Control 0 register.
Trying to unconditionally enable ASPM L0s via ASPM Control bits in Link
Control register is wrong. There should be at least some detection if
endpoint supports L0s as isn't mandatory.
Moreover ASPM Control bits in Link Control register are controlled by
pcie/aspm.c code which sets it according to system ASPM settings,
immediately after aardvark driver probes. So setting these bits by
aardvark driver has no long running effect.
Remove code which touches ASPM L0s bits from this driver and let
kernel's ASPM implementation to set ASPM state properly.
Some users are reporting issues that this code is problematic for some
Intel wifi cards and removing it fixes them, see e.g.:
https://bugzilla.kernel.org/show_bug.cgi?id=196339
If problems with Intel wifi cards occur even after this commit, then
pcie/aspm.c code could be modified / hooked to not enable ASPM L0s state
for affected problematic cards.
Signed-off-by: Pali Rohár <[email protected]>
---
drivers/pci/controller/pci-aardvark.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index f9955b494267..74b90940a9d4 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -356,10 +356,6 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
advk_pcie_wait_for_link(pcie);
- reg = PCIE_CORE_LINK_L0S_ENTRY |
- (1 << PCIE_CORE_LINK_WIDTH_SHIFT);
- advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
-
reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
reg |= PCIE_CORE_CMD_MEM_ACCESS_EN |
PCIE_CORE_CMD_IO_ACCESS_EN |
--
2.20.1
From: Marek Behún <[email protected]>
With recent proposed changes for U-Boot it is possible that bootloader
won't initialize the PHY for this controller (currently the PHY is
initialized regardless whether PCI is used in U-Boot, but with these
proposed changes the PHY is initialized only on request).
Since the mvebu-a3700-comphy driver by Miquèl Raynal supports enabling
PCIe PHY, and since Linux' functionality should be independent on what
bootloader did, add code for enabling generic PHY if found in device OF
node.
The mvebu-a3700-comphy driver does PHY powering via SMC calls to ARM
Trusted Firmware. The corresponding code in ARM Trusted Firmware skips
one register write which U-Boot does not: step 7 ("Enable TX"), see [1].
Instead ARM Trusted Firmware expects PCIe driver to do this step,
probably because the register is in PCIe controller address space,
instead of PHY address space. We therefore add this step into the
advk_pcie_setup_hw function.
[1] https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/drivers/marvell/comphy/phy-comphy-3700.c?h=v2.3-rc2#n836
Signed-off-by: Marek Behún <[email protected]>
Cc: Miquèl Raynal <[email protected]>
---
drivers/pci/controller/pci-aardvark.c | 69 +++++++++++++++++++++++++++
1 file changed, 69 insertions(+)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 8332c71d69fa..053ae6c19a3d 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -16,6 +16,7 @@
#include <linux/kernel.h>
#include <linux/pci.h>
#include <linux/init.h>
+#include <linux/phy/phy.h>
#include <linux/platform_device.h>
#include <linux/msi.h>
#include <linux/of_address.h>
@@ -104,6 +105,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_CORE_REF_CLK_REG (CONTROL_BASE_ADDR + 0x14)
+#define PCIE_CORE_REF_CLK_TX_ENABLE BIT(1)
#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)
@@ -207,6 +210,7 @@ struct advk_pcie {
int link_gen;
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)
@@ -358,6 +362,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
advk_pcie_issue_perst(pcie);
+ /* Enable TX */
+ reg = advk_readl(pcie, PCIE_CORE_REF_CLK_REG);
+ reg |= PCIE_CORE_REF_CLK_TX_ENABLE;
+ advk_writel(pcie, reg, PCIE_CORE_REF_CLK_REG);
+
/* Set to Direct mode */
reg = advk_readl(pcie, CTRL_CONFIG_REG);
reg &= ~(CTRL_MODE_MASK << CTRL_MODE_SHIFT);
@@ -1041,6 +1050,62 @@ static irqreturn_t advk_pcie_irq_handler(int irq, void *arg)
return IRQ_HANDLED;
}
+static void __maybe_unused 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;
@@ -1100,6 +1165,10 @@ static int advk_pcie_probe(struct platform_device *pdev)
else
pcie->link_gen = ret;
+ ret = advk_pcie_setup_phy(pcie);
+ if (ret)
+ return ret;
+
advk_pcie_setup_hw(pcie);
advk_sw_pci_bridge_init(pcie);
--
2.20.1
On Thu, Apr 30, 2020 at 10:06:14AM +0200, Pali Roh?r wrote:
> Adding even 100ms (PCI_PM_D3COLD_WAIT) delay between enabling link
> training and starting link training causes detection issues with some
> buggy cards (such as Compex WLE900VX).
>
> Move the code which enables link training immediately before the one
> which starts link traning.
s/traning/training/
> This fixes detection issues of Compex WLE900VX card on Turris MOX after
> cold boot.
>
> Fixes: f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready...")
Don't repost just for these, but if you do repost, include the
complete subject here, e.g.,
f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready before training link")
On Thu, 30 Apr 2020 10:06:14 +0200, =?UTF-8?q?Pali=20Roh=C3=A1r?= wrote:
> Adding even 100ms (PCI_PM_D3COLD_WAIT) delay between enabling link
> training and starting link training causes detection issues with some
> buggy cards (such as Compex WLE900VX).
>
> Move the code which enables link training immediately before the one
> which starts link traning.
>
> This fixes detection issues of Compex WLE900VX card on Turris MOX after
> cold boot.
>
> Fixes: f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready...")
> Signed-off-by: Pali Roh?r <[email protected]>
> ---
> drivers/pci/controller/pci-aardvark.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
Acked-by: Rob Herring <[email protected]>
On Thu, 30 Apr 2020 10:06:15 +0200, =?UTF-8?q?Pali=20Roh=C3=A1r?= wrote:
> Trying to change Link Status register does not have any effect as this
> is a read-only register. Trying to overwrite bits for Negotiated Link
> Width does not make sense.
>
> In future proper change of link width can be done via Lane Count Select
> bits in PCIe Control 0 register.
>
> Trying to unconditionally enable ASPM L0s via ASPM Control bits in Link
> Control register is wrong. There should be at least some detection if
> endpoint supports L0s as isn't mandatory.
>
> Moreover ASPM Control bits in Link Control register are controlled by
> pcie/aspm.c code which sets it according to system ASPM settings,
> immediately after aardvark driver probes. So setting these bits by
> aardvark driver has no long running effect.
>
> Remove code which touches ASPM L0s bits from this driver and let
> kernel's ASPM implementation to set ASPM state properly.
>
> Some users are reporting issues that this code is problematic for some
> Intel wifi cards and removing it fixes them, see e.g.:
> https://bugzilla.kernel.org/show_bug.cgi?id=196339
>
> If problems with Intel wifi cards occur even after this commit, then
> pcie/aspm.c code could be modified / hooked to not enable ASPM L0s state
> for affected problematic cards.
>
> Signed-off-by: Pali Roh?r <[email protected]>
> ---
> drivers/pci/controller/pci-aardvark.c | 4 ----
> 1 file changed, 4 deletions(-)
>
Acked-by: Rob Herring <[email protected]>
On Thu, 30 Apr 2020 10:06:16 +0200, =?UTF-8?q?Pali=20Roh=C3=A1r?= wrote:
> Interpret zero value of max-link-speed property as invalid,
> as the device tree bindings documentation specifies.
>
> Signed-off-by: Pali Roh?r <[email protected]>
> ---
> drivers/pci/of.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Acked-by: Rob Herring <[email protected]>
On Thu, 30 Apr 2020 10:06:17 +0200, =?UTF-8?q?Pali=20Roh=C3=A1r?= wrote:
> From: Marek Beh?n <[email protected]>
>
> Currently the aardvark driver trains link in PCIe gen2 mode. This may
> cause some buggy gen1 cards (such as Compex WLE900VX) to be unstable or
> even not detected. Moreover when ASPM code tries to retrain link second
> time, these cards may stop responding and link goes down. If gen1 is
> used this does not happen.
>
> Unconditionally forcing gen1 is not a good solution since it may have
> performance impact on gen2 cards.
>
> To overcome this, read 'max-link-speed' property (as defined in PCI
> device tree bindings) and use this as max gen mode. Then iteratively try
> link training at this mode or lower until successful. After successful
> link training choose final controller gen based on Negotiated Link Speed
> from Link Status register, which should match card speed.
>
> Signed-off-by: Pali Roh?r <[email protected]>
> Signed-off-by: Marek Beh?n <[email protected]>
> ---
> drivers/pci/controller/pci-aardvark.c | 114 ++++++++++++++++++++------
> 1 file changed, 89 insertions(+), 25 deletions(-)
>
Reviewed-by: Rob Herring <[email protected]>
On Thu, 30 Apr 2020 10:06:19 +0200, =?UTF-8?q?Pali=20Roh=C3=A1r?= wrote:
> This register is applicable only when the controller is configured for
> Endpoint mode, which is not the case for the current version of this
> driver.
>
> Attempting to remove this code though caused some ath10k cards to stop
> working, so for some unknown reason it is needed here.
>
> This should be investigated and a comment explaining this should be put
> before the code, so we add a FIXME comment for now.
>
> Signed-off-by: Pali Roh?r <[email protected]>
> ---
> drivers/pci/controller/pci-aardvark.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
Acked-by: Rob Herring <[email protected]>
On Thu, 30 Apr 2020 10:06:20 +0200, =?UTF-8?q?Pali=20Roh=C3=A1r?= wrote:
> From: Marek Beh?n <[email protected]>
>
> With recent proposed changes for U-Boot it is possible that bootloader
> won't initialize the PHY for this controller (currently the PHY is
> initialized regardless whether PCI is used in U-Boot, but with these
> proposed changes the PHY is initialized only on request).
>
> Since the mvebu-a3700-comphy driver by Miqu?l Raynal supports enabling
> PCIe PHY, and since Linux' functionality should be independent on what
> bootloader did, add code for enabling generic PHY if found in device OF
> node.
>
> The mvebu-a3700-comphy driver does PHY powering via SMC calls to ARM
> Trusted Firmware. The corresponding code in ARM Trusted Firmware skips
> one register write which U-Boot does not: step 7 ("Enable TX"), see [1].
> Instead ARM Trusted Firmware expects PCIe driver to do this step,
> probably because the register is in PCIe controller address space,
> instead of PHY address space. We therefore add this step into the
> advk_pcie_setup_hw function.
>
> [1] https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/drivers/marvell/comphy/phy-comphy-3700.c?h=v2.3-rc2#n836
>
> Signed-off-by: Marek Beh?n <[email protected]>
> Cc: Miqu?l Raynal <[email protected]>
> ---
> drivers/pci/controller/pci-aardvark.c | 69 +++++++++++++++++++++++++++
> 1 file changed, 69 insertions(+)
>
Reviewed-by: Rob Herring <[email protected]>
On Thu, 30 Apr 2020 10:06:22 +0200, =?UTF-8?q?Pali=20Roh=C3=A1r?= wrote:
> From: Marek Beh?n <[email protected]>
>
> Document the possibility to reference a PHY and reset-gpios and to set
> max-link-speed property.
>
> Signed-off-by: Marek Beh?n <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: [email protected]
> ---
> Documentation/devicetree/bindings/pci/aardvark-pci.txt | 4 ++++
> 1 file changed, 4 insertions(+)
>
Reviewed-by: Rob Herring <[email protected]>
W dniu 30.04.2020 o 10:06, Pali Rohár pisze:
> Hello,
>
> this is the fourth version of the patch series for Armada 3720 PCIe
> controller (aardvark). It's main purpose is to fix some bugs regarding
> buggy ath10k cards, but we also found out some suspicious stuff about
> the driver and the SOC itself, which we try to address.
>
> Patches are available also in my git branch pci-aardvark:
> https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=pci-aardvark
>
> Changes since v3:
> - do not change return value of of_pci_get_max_link_speed() function
> - mark zero 'max-link-speed' as invalid
> - silently use gen3 speed when 'max-link-speed' as invalid
>
> Changes since v2:
> - move PCIe max-link-speed property to armada-37xx.dtsi
> - replace custom macros by standard linux/pci_regs.h macros
> - increase PERST delay to 10ms (needed for initialized Compex WLE900VX)
> - disable link training before PERST (needed for Compex WLE900VX)
> - change of_pci_get_max_link_speed() function to signal -ENOENT
> - handle errors from of_pci_get_max_link_speed() function
> - updated comments, commit titles and messages
>
> Changes since v1:
> - commit titles and messages were reviewed and some of them were rewritten
> - patches 1 and 5 from v1 which touch PCIe speed configuration were
> reworked into one patch
> - patch 2 from v1 was removed, it is not needed anymore
> - patch 7 from v1 now touches the device tree of armada-3720-db
> - a patch was added that tries to enable PCIe PHY via generic-phy API
> (if a phandle to the PHY is found in the device tree)
> - a patch describing the new PCIe node DT properties was added
> - a patch was added that moves the PHY phandle from board device trees
> to armada-37xx.dtsi
>
> Marek and Pali
>
> Marek Behún (5):
> PCI: aardvark: Improve link training
> PCI: aardvark: Add PHY support
> dt-bindings: PCI: aardvark: Describe new properties
> arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
> arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property
>
> Pali Rohár (7):
> PCI: aardvark: Train link immediately after enabling training
> PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
> read-only register
> PCI: of: Zero max-link-speed value is invalid
> PCI: aardvark: Issue PERST via GPIO
> PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
> PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
> macros
> arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property
Hi.
The PCI interface seems to work fine as in the first series, so
Tested-by: Tomasz Maciej Nowak <[email protected]>
--
TMN
On Thursday 30 April 2020 10:06:13 Pali Rohár wrote:
> Hello,
>
> this is the fourth version of the patch series for Armada 3720 PCIe
> controller (aardvark). It's main purpose is to fix some bugs regarding
> buggy ath10k cards, but we also found out some suspicious stuff about
> the driver and the SOC itself, which we try to address.
>
> Patches are available also in my git branch pci-aardvark:
> https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=pci-aardvark
Hello! Thanks everybody for review and testing of this patch series.
I would like to ask, is there something needed to fix / modify in this
patch series? If everything is OK, would you Bjorn or Lorenzo take this
patch series into your tree?
Hello,
On Thu, 30 Apr 2020 10:06:13 +0200
Pali Rohár <[email protected]> wrote:
> Marek Behún (5):
> PCI: aardvark: Improve link training
> PCI: aardvark: Add PHY support
> dt-bindings: PCI: aardvark: Describe new properties
> arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
> arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property
>
> Pali Rohár (7):
> PCI: aardvark: Train link immediately after enabling training
> PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
> read-only register
> PCI: of: Zero max-link-speed value is invalid
> PCI: aardvark: Issue PERST via GPIO
> PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
> PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
> macros
> arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property
Thanks a lot for this work. For a number of reasons, I'm less involved
in Marvell platform support in Linux, but I reviewed your series and
followed the discussions around it, and I'm happy to give my:
Acked-by: Thomas Petazzoni <[email protected]>
for the whole series. The changes all seem sensible, and have been
tested by several folks.
Thanks!
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Wednesday 13 May 2020 12:33:14 Lorenzo Pieralisi wrote:
> On Wed, May 13, 2020 at 01:16:51PM +0200, Pali Rohár wrote:
> > On Thursday 30 April 2020 10:06:13 Pali Rohár wrote:
> > > Hello,
> > >
> > > this is the fourth version of the patch series for Armada 3720 PCIe
> > > controller (aardvark). It's main purpose is to fix some bugs regarding
> > > buggy ath10k cards, but we also found out some suspicious stuff about
> > > the driver and the SOC itself, which we try to address.
> > >
> > > Patches are available also in my git branch pci-aardvark:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=pci-aardvark
> >
> > Hello! Thanks everybody for review and testing of this patch series.
> >
> > I would like to ask, is there something needed to fix / modify in this
> > patch series? If everything is OK, would you Bjorn or Lorenzo take this
> > patch series into your tree?
>
> We need Thomas' ACK on the series. We don't have this HW and
> we comment on the generic code, Thomas owns it and must check that
> what you are changing is sound.
Ok, we will wait for Thomas ACK/review.
> On patch 5 I share Rob's concerns - it does not make much sense
> to have something driver specific there, need to look further.
I fully understand yours concerns. I wanted to solve it. Problem is that
I really do not know which timeout is there applicable. I read
information about PERST# more times but I was not able to clearly deduce
that minimal timeout/delay needed for this reset scenario.
So what I was able to do are just experiments. I found out what is the
minimal needed time to correctly initialize wifi cars which I used for
testing.
You can look into my previous email [1] where I wrote which timeouts are
used by which drivers. Basically every driver is using its own custom
timeout and this is something which should be fixed / improved. In my
opinion authors tested their own (wifi) cards and measured minimal
timeout needed for initializing them.
So somebody with deeper PCI knowledge should look at this PERST# problem
and try to address it.
After it happens I see there two scenarios:
1) Timeout according to specification/authority is lower than what we
currently use. In this case it would mean that we have buggy wifi cards
(and we already know that people reported issues with Compex cards) and
we would have to stay with higher timeout. Probably we can define common
macro with timeout value and use it.
2) Timeout according to specification/authority is bigger then what we
currently use. In this case there is no problem to increase it, card
would be just longer in reset state. What could be problematic for
somebody is that this increase boot / initialization time.
[1] - https://lore.kernel.org/linux-pci/20200424092546.25p3hdtkehohe3xw@pali/
On Wed, May 13, 2020 at 01:16:51PM +0200, Pali Roh?r wrote:
> On Thursday 30 April 2020 10:06:13 Pali Roh?r wrote:
> > Hello,
> >
> > this is the fourth version of the patch series for Armada 3720 PCIe
> > controller (aardvark). It's main purpose is to fix some bugs regarding
> > buggy ath10k cards, but we also found out some suspicious stuff about
> > the driver and the SOC itself, which we try to address.
> >
> > Patches are available also in my git branch pci-aardvark:
> > https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=pci-aardvark
>
> Hello! Thanks everybody for review and testing of this patch series.
>
> I would like to ask, is there something needed to fix / modify in this
> patch series? If everything is OK, would you Bjorn or Lorenzo take this
> patch series into your tree?
We need Thomas' ACK on the series. We don't have this HW and
we comment on the generic code, Thomas owns it and must check that
what you are changing is sound.
On patch 5 I share Rob's concerns - it does not make much sense
to have something driver specific there, need to look further.
Lorenzo
Hello,
> Hello,
>
> On Thu, 30 Apr 2020 10:06:13 +0200
> Pali Rohár <[email protected]> wrote:
>
>> Marek Behún (5):
>> PCI: aardvark: Improve link training
>> PCI: aardvark: Add PHY support
>> dt-bindings: PCI: aardvark: Describe new properties
>> arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
>> arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property
>>
>> Pali Rohár (7):
>> PCI: aardvark: Train link immediately after enabling training
>> PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
>> read-only register
>> PCI: of: Zero max-link-speed value is invalid
>> PCI: aardvark: Issue PERST via GPIO
>> PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
>> PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
>> macros
>> arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property
>
> Thanks a lot for this work. For a number of reasons, I'm less involved
> in Marvell platform support in Linux, but I reviewed your series and
> followed the discussions around it, and I'm happy to give my:
>
> Acked-by: Thomas Petazzoni <[email protected]>
With this acked-by for the series, the reviewed-by from Rob on the
binding and the tested-by, I am pretty confident so I applied the
patches 10, 11 and 12 on mvebu/dt64.
Thanks,
Gregory
>
> for the whole series. The changes all seem sensible, and have been
> tested by several folks.
>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
--
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com
On Sunday 17 May 2020 17:57:02 Gregory CLEMENT wrote:
> Hello,
>
> > Hello,
> >
> > On Thu, 30 Apr 2020 10:06:13 +0200
> > Pali Rohár <[email protected]> wrote:
> >
> >> Marek Behún (5):
> >> PCI: aardvark: Improve link training
> >> PCI: aardvark: Add PHY support
> >> dt-bindings: PCI: aardvark: Describe new properties
> >> arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
> >> arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property
> >>
> >> Pali Rohár (7):
> >> PCI: aardvark: Train link immediately after enabling training
> >> PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
> >> read-only register
> >> PCI: of: Zero max-link-speed value is invalid
> >> PCI: aardvark: Issue PERST via GPIO
> >> PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
> >> PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
> >> macros
> >> arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property
> >
> > Thanks a lot for this work. For a number of reasons, I'm less involved
> > in Marvell platform support in Linux, but I reviewed your series and
> > followed the discussions around it, and I'm happy to give my:
> >
> > Acked-by: Thomas Petazzoni <[email protected]>
>
> With this acked-by for the series, the reviewed-by from Rob on the
> binding and the tested-by, I am pretty confident so I applied the
> patches 10, 11 and 12 on mvebu/dt64.
>
> Thanks,
>
> Gregory
Thank you!
Lorenzo, would you now take remaining patches?
>
> >
> > for the whole series. The changes all seem sensible, and have been
> > tested by several folks.
> >
> > Thanks!
> >
> > Thomas
> > --
> > Thomas Petazzoni, CTO, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
>
> --
> Gregory Clement, Bootlin
> Embedded Linux and Kernel engineering
> http://bootlin.com
On Mon, May 18, 2020 at 12:30:04PM +0200, Pali Roh?r wrote:
> On Sunday 17 May 2020 17:57:02 Gregory CLEMENT wrote:
> > Hello,
> >
> > > Hello,
> > >
> > > On Thu, 30 Apr 2020 10:06:13 +0200
> > > Pali Roh?r <[email protected]> wrote:
> > >
> > >> Marek Beh?n (5):
> > >> PCI: aardvark: Improve link training
> > >> PCI: aardvark: Add PHY support
> > >> dt-bindings: PCI: aardvark: Describe new properties
> > >> arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
> > >> arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property
> > >>
> > >> Pali Roh?r (7):
> > >> PCI: aardvark: Train link immediately after enabling training
> > >> PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
> > >> read-only register
> > >> PCI: of: Zero max-link-speed value is invalid
> > >> PCI: aardvark: Issue PERST via GPIO
> > >> PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
> > >> PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
> > >> macros
> > >> arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property
> > >
> > > Thanks a lot for this work. For a number of reasons, I'm less involved
> > > in Marvell platform support in Linux, but I reviewed your series and
> > > followed the discussions around it, and I'm happy to give my:
> > >
> > > Acked-by: Thomas Petazzoni <[email protected]>
> >
> > With this acked-by for the series, the reviewed-by from Rob on the
> > binding and the tested-by, I am pretty confident so I applied the
> > patches 10, 11 and 12 on mvebu/dt64.
> >
> > Thanks,
> >
> > Gregory
>
> Thank you!
>
> Lorenzo, would you now take remaining patches?
Yes - even though I have reservations about patch (5) and the
problem is related to a complete lack of programming model for
these host controllers and a clear separation between what's
done in the OS vs bootloader, PERST handling in this host
bridge is *really* a mess.
I applied 1-9 to pci/aardvark.
Lorenzo
On Mon, 18 May 2020 14:46:14 +0100
Lorenzo Pieralisi <[email protected]> wrote:
> On Mon, May 18, 2020 at 12:30:04PM +0200, Pali Rohár wrote:
> > On Sunday 17 May 2020 17:57:02 Gregory CLEMENT wrote:
> > > Hello,
> > >
> > > > Hello,
> > > >
> > > > On Thu, 30 Apr 2020 10:06:13 +0200
> > > > Pali Rohár <[email protected]> wrote:
> > > >
> > > >> Marek Behún (5):
> > > >> PCI: aardvark: Improve link training
> > > >> PCI: aardvark: Add PHY support
> > > >> dt-bindings: PCI: aardvark: Describe new properties
> > > >> arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
> > > >> arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property
> > > >>
> > > >> Pali Rohár (7):
> > > >> PCI: aardvark: Train link immediately after enabling training
> > > >> PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
> > > >> read-only register
> > > >> PCI: of: Zero max-link-speed value is invalid
> > > >> PCI: aardvark: Issue PERST via GPIO
> > > >> PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
> > > >> PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
> > > >> macros
> > > >> arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property
> > > >
> > > > Thanks a lot for this work. For a number of reasons, I'm less involved
> > > > in Marvell platform support in Linux, but I reviewed your series and
> > > > followed the discussions around it, and I'm happy to give my:
> > > >
> > > > Acked-by: Thomas Petazzoni <[email protected]>
> > >
> > > With this acked-by for the series, the reviewed-by from Rob on the
> > > binding and the tested-by, I am pretty confident so I applied the
> > > patches 10, 11 and 12 on mvebu/dt64.
> > >
> > > Thanks,
> > >
> > > Gregory
> >
> > Thank you!
> >
> > Lorenzo, would you now take remaining patches?
>
> Yes - even though I have reservations about patch (5) and the
> problem is related to a complete lack of programming model for
> these host controllers and a clear separation between what's
> done in the OS vs bootloader, PERST handling in this host
> bridge is *really* a mess.
>
> I applied 1-9 to pci/aardvark.
>
> Lorenzo
Hooray, thanks, Lorenzo (and everyone else).