PCI: aardvark: Fix support for Turris MOX and Compex wifi cards

OF API function of_property_read_u32() returns -EINVAL if property is not found. Therefore this also happens with of_pci_get_max_link_speed(), which also returns -EINVAL if the 'max-link-speed' property has invalid value.

Change the behaviour of of_pci_get_max_link_speed() to return -ENOENT in case when the property does not exist and -EINVAL if it has invalid value.

Also interpret zero max-link-speed value of this property as invalid, as the device tree bindings documentation specifies.

Update pcie-tegra194 code to handle errors from this function like other drivers - they do not distinguish between no value and invalid value.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/pci/controller/dwc/pcie-tegra194.c |  6 +++---
 drivers/pci/of.c                           | 15 +++++++++++----
 2 files changed, 14 insertions(+), 7 deletions(-)

drivers/pci/controller/dwc/pcie-tegra194.c +3 -3

index ae30a2fd3716..027bb41809f9 100644
@@ -296,7 +296,7 @@ struct tegra_pcie_dw {
        u8 init_link_width;
        u32 msi_ctrl_int;
        u32 num_lanes;
-       u32 max_speed;
+       int max_speed;
        u32 cid;
        u32 cfg_link_cap_l1sub;
        u32 pcie_cap_base;

@@ -911,7 +911,7 @@ static void tegra_pcie_prepare_host(struct pcie_port *pp)
        dw_pcie_writel_dbi(pci, PORT_LOGIC_AMBA_ERROR_RESPONSE_DEFAULT, val);
 
        /* Configure Max Speed from DT */
-       if (pcie->max_speed && pcie->max_speed != -EINVAL) {
+       if (pcie->max_speed > 0) {
                val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base +
                                        PCI_EXP_LNKCAP);
                val &= ~PCI_EXP_LNKCAP_SLS;

@@ -1830,7 +1830,7 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
        dw_pcie_writel_dbi(pci, PORT_LOGIC_GEN2_CTRL, val);
 
        /* Configure Max Speed from DT */
-       if (pcie->max_speed && pcie->max_speed != -EINVAL) {
+       if (pcie->max_speed > 0) {
                val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base +
                                        PCI_EXP_LNKCAP);
                val &= ~PCI_EXP_LNKCAP_SLS;

drivers/pci/of.c +2 -2

index 81ceeaa6f1d5..19bf652256d8 100644
@@ -584,15 +584,22 @@ EXPORT_SYMBOL_GPL(pci_parse_request_of_pci_ranges);
  *
  * @node: device tree node with the max link speed information
  *
- * Returns the associated max link speed from DT, or a negative value if the
- * required property is not found or is invalid.
+ * Returns the associated max link speed from DT, -ENOENT if the required
+ * property is not found or -EINVAL if the required property is invalid.

Quoted 73 lines · Pali Rohár

OF API function of_property_read_u32() returns -EINVAL if property is not found. Therefore this also happens with of_pci_get_max_link_speed(), which also returns -EINVAL if the 'max-link-speed' property has invalid value.

Change the behaviour of of_pci_get_max_link_speed() to return -ENOENT in case when the property does not exist and -EINVAL if it has invalid value.

Also interpret zero max-link-speed value of this property as invalid, as the device tree bindings documentation specifies.

Update pcie-tegra194 code to handle errors from this function like other drivers - they do not distinguish between no value and invalid value.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/pci/controller/dwc/pcie-tegra194.c |  6 +++---
 drivers/pci/of.c                           | 15 +++++++++++----
 2 files changed, 14 insertions(+), 7 deletions(-)

drivers/pci/controller/dwc/pcie-tegra194.c +3 -3

index ae30a2fd3716..027bb41809f9 100644
@@ -296,7 +296,7 @@ struct tegra_pcie_dw {
        u8 init_link_width;
        u32 msi_ctrl_int;
        u32 num_lanes;
-       u32 max_speed;
+       int max_speed;
        u32 cid;
        u32 cfg_link_cap_l1sub;
        u32 pcie_cap_base;

@@ -911,7 +911,7 @@ static void tegra_pcie_prepare_host(struct pcie_port *pp)
        dw_pcie_writel_dbi(pci, PORT_LOGIC_AMBA_ERROR_RESPONSE_DEFAULT, val);
 
        /* Configure Max Speed from DT */
-       if (pcie->max_speed && pcie->max_speed != -EINVAL) {
+       if (pcie->max_speed > 0) {
                val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base +
                                        PCI_EXP_LNKCAP);
                val &= ~PCI_EXP_LNKCAP_SLS;

@@ -1830,7 +1830,7 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
        dw_pcie_writel_dbi(pci, PORT_LOGIC_GEN2_CTRL, val);
 
        /* Configure Max Speed from DT */
-       if (pcie->max_speed && pcie->max_speed != -EINVAL) {
+       if (pcie->max_speed > 0) {
                val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base +
                                        PCI_EXP_LNKCAP);
                val &= ~PCI_EXP_LNKCAP_SLS;

drivers/pci/of.c +6 -2

index 81ceeaa6f1d5..19bf652256d8 100644
@@ -584,15 +584,22 @@ EXPORT_SYMBOL_GPL(pci_parse_request_of_pci_ranges);
  *
  * @node: device tree node with the max link speed information
  *
- * Returns the associated max link speed from DT, or a negative value if the
- * required property is not found or is invalid.
+ * Returns the associated max link speed from DT, -ENOENT if the required
+ * property is not found or -EINVAL if the required property is invalid.
  */
 int of_pci_get_max_link_speed(struct device_node *node)
 {
        u32 max_link_speed;
+       int ret;
+
+       /* of_property_read_u32 returns -EINVAL if property does not exist */
+       ret = of_property_read_u32(node, "max-link-speed", &max_link_speed);

drivers/pci/of.c EXPORT_SYMBOL_GPL(pci_parse_request_of_pci_ranges); · line 598

Quoted 2 lines

drivers/pci/of.c +2 -0

+       if (ret == -EINVAL)
+               return -ENOENT;

Generally, it's considered bad to change return values (though I guess this was happening. In hindsight, not present probably should have been -ENOENT. But it shouldn't really matter. The kernel should treat malformed as not present. It's not the kernel's job to validate the DT (the schema should and does now).

Plus you are adding capability to distinguish not present and out of bounds, but I don't see you using that?

If there's any error with max-link-speed, then just use the max speed for the block which should be implied by the compatible string if there's more than one.

Rob

Quoted 74 lines · On Friday 24 April 2020 11:47:26 Rob Herring wrote:

Quoted 73 lines · Pali Rohár

OF API function of_property_read_u32() returns -EINVAL if property is not found. Therefore this also happens with of_pci_get_max_link_speed(), which also returns -EINVAL if the 'max-link-speed' property has invalid value.

Change the behaviour of of_pci_get_max_link_speed() to return -ENOENT in case when the property does not exist and -EINVAL if it has invalid value.

Also interpret zero max-link-speed value of this property as invalid, as the device tree bindings documentation specifies.

Update pcie-tegra194 code to handle errors from this function like other drivers - they do not distinguish between no value and invalid value.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/pci/controller/dwc/pcie-tegra194.c |  6 +++---
 drivers/pci/of.c                           | 15 +++++++++++----
 2 files changed, 14 insertions(+), 7 deletions(-)

drivers/pci/controller/dwc/pcie-tegra194.c +3 -3

index ae30a2fd3716..027bb41809f9 100644
@@ -296,7 +296,7 @@ struct tegra_pcie_dw {
        u8 init_link_width;
        u32 msi_ctrl_int;
        u32 num_lanes;
-       u32 max_speed;
+       int max_speed;
        u32 cid;
        u32 cfg_link_cap_l1sub;
        u32 pcie_cap_base;

@@ -911,7 +911,7 @@ static void tegra_pcie_prepare_host(struct pcie_port *pp)
        dw_pcie_writel_dbi(pci, PORT_LOGIC_AMBA_ERROR_RESPONSE_DEFAULT, val);
 
        /* Configure Max Speed from DT */
-       if (pcie->max_speed && pcie->max_speed != -EINVAL) {
+       if (pcie->max_speed > 0) {
                val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base +
                                        PCI_EXP_LNKCAP);
                val &= ~PCI_EXP_LNKCAP_SLS;

@@ -1830,7 +1830,7 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
        dw_pcie_writel_dbi(pci, PORT_LOGIC_GEN2_CTRL, val);
 
        /* Configure Max Speed from DT */
-       if (pcie->max_speed && pcie->max_speed != -EINVAL) {
+       if (pcie->max_speed > 0) {
                val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base +
                                        PCI_EXP_LNKCAP);
                val &= ~PCI_EXP_LNKCAP_SLS;

drivers/pci/of.c +6 -2

index 81ceeaa6f1d5..19bf652256d8 100644
@@ -584,15 +584,22 @@ EXPORT_SYMBOL_GPL(pci_parse_request_of_pci_ranges);
  *
  * @node: device tree node with the max link speed information
  *
- * Returns the associated max link speed from DT, or a negative value if the
- * required property is not found or is invalid.
+ * Returns the associated max link speed from DT, -ENOENT if the required
+ * property is not found or -EINVAL if the required property is invalid.
  */
 int of_pci_get_max_link_speed(struct device_node *node)
 {
        u32 max_link_speed;
+       int ret;
+
+       /* of_property_read_u32 returns -EINVAL if property does not exist */
+       ret = of_property_read_u32(node, "max-link-speed", &max_link_speed);

Quoted 8 lines

drivers/pci/of.c EXPORT_SYMBOL_GPL(pci_parse_request_of_pci_ranges); · line 598

Quoted 2 lines

drivers/pci/of.c +2 -0

+       if (ret == -EINVAL)
+               return -ENOENT;

Generally, it's considered bad to change return values (though I guess this was happening. In hindsight, not present probably should have been -ENOENT. But it shouldn't really matter. The kernel should treat malformed as not present. It's not the kernel's job to validate the DT (the schema should and does now).

Bjorn in review of V1 patch wrote that aardavark driver should at least warn on DT error.

And because max-link-speed is optional property, it is perfectly valid when it is absent.

So without ability to distinguish between "property is not present in DT" and "property is malformed" it is not possible to properly detect this DT error.

Quoted 85 lines · Pali Rohár

Quoted 82 lines · On Friday 24 April 2020 11:47:26 Rob Herring wrote:

Quoted 65 lines · Pali Rohár

OF API function of_property_read_u32() returns -EINVAL if property is not found. Therefore this also happens with of_pci_get_max_link_speed(), which also returns -EINVAL if the 'max-link-speed' property has invalid value.

Change the behaviour of of_pci_get_max_link_speed() to return -ENOENT in case when the property does not exist and -EINVAL if it has invalid value.

Also interpret zero max-link-speed value of this property as invalid, as the device tree bindings documentation specifies.

Update pcie-tegra194 code to handle errors from this function like other drivers - they do not distinguish between no value and invalid value.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/pci/controller/dwc/pcie-tegra194.c |  6 +++---
 drivers/pci/of.c                           | 15 +++++++++++----
 2 files changed, 14 insertions(+), 7 deletions(-)

drivers/pci/controller/dwc/pcie-tegra194.c +3 -3

index ae30a2fd3716..027bb41809f9 100644
@@ -296,7 +296,7 @@ struct tegra_pcie_dw {
        u8 init_link_width;
        u32 msi_ctrl_int;
        u32 num_lanes;
-       u32 max_speed;
+       int max_speed;
        u32 cid;
        u32 cfg_link_cap_l1sub;
        u32 pcie_cap_base;

@@ -911,7 +911,7 @@ static void tegra_pcie_prepare_host(struct pcie_port *pp)
        dw_pcie_writel_dbi(pci, PORT_LOGIC_AMBA_ERROR_RESPONSE_DEFAULT, val);
 
        /* Configure Max Speed from DT */
-       if (pcie->max_speed && pcie->max_speed != -EINVAL) {
+       if (pcie->max_speed > 0) {
                val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base +
                                        PCI_EXP_LNKCAP);
                val &= ~PCI_EXP_LNKCAP_SLS;

@@ -1830,7 +1830,7 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
        dw_pcie_writel_dbi(pci, PORT_LOGIC_GEN2_CTRL, val);
 
        /* Configure Max Speed from DT */
-       if (pcie->max_speed && pcie->max_speed != -EINVAL) {
+       if (pcie->max_speed > 0) {
                val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base +
                                        PCI_EXP_LNKCAP);
                val &= ~PCI_EXP_LNKCAP_SLS;

drivers/pci/of.c +2 -2

index 81ceeaa6f1d5..19bf652256d8 100644
@@ -584,15 +584,22 @@ EXPORT_SYMBOL_GPL(pci_parse_request_of_pci_ranges);
  *
  * @node: device tree node with the max link speed information
  *
- * Returns the associated max link speed from DT, or a negative value if the
- * required property is not found or is invalid.
+ * Returns the associated max link speed from DT, -ENOENT if the required
+ * property is not found or -EINVAL if the required property is invalid.

drivers/pci/of.c EXPORT_SYMBOL_GPL(pci_parse_request_of_pci_ranges); · line 598

Quoted 10 lines

drivers/pci/of.c +6 -0

  */
 int of_pci_get_max_link_speed(struct device_node *node)
 {
        u32 max_link_speed;
+       int ret;
+
+       /* of_property_read_u32 returns -EINVAL if property does not exist */
+       ret = of_property_read_u32(node, "max-link-speed", &max_link_speed);
+       if (ret == -EINVAL)
+               return -ENOENT;

Generally, it's considered bad to change return values (though I guess this was happening. In hindsight, not present probably should have been -ENOENT. But it shouldn't really matter. The kernel should treat malformed as not present. It's not the kernel's job to validate the DT (the schema should and does now).

Quoted 2 lines

Bjorn in review of V1 patch wrote that aardavark driver should at least warn on DT error.

Yes, but I disagree. Just treat an error as not present as long as the driver can make progress. If something critical required is missing, then yes we should print an error and bail out.

Quoted 6 lines

And because max-link-speed is optional property, it is perfectly valid when it is absent.

So without ability to distinguish between "property is not present in DT" and "property is malformed" it is not possible to properly detect this DT error.

How would you have an error? Your DT has a schema to check it, right? (Hint: convert your binding)

Think about it this way. If every driver has an error string for every property, that's a lot of bloat to the kernel. If we really want to print errors, we should define of_property_.*_optional() variants and print errors in the core code.

Quoted 2 lines

How would you have an error? Your DT has a schema to check it, right? (Hint: convert your binding)

Schema is currently defined as human readable text file. So checking is possible but only manually.

Quoted 84 lines · Pali Rohár

Quoted 82 lines · On Friday 24 April 2020 11:47:26 Rob Herring wrote:

Quoted 65 lines · Pali Rohár

OF API function of_property_read_u32() returns -EINVAL if property is not found. Therefore this also happens with of_pci_get_max_link_speed(), which also returns -EINVAL if the 'max-link-speed' property has invalid value.

Change the behaviour of of_pci_get_max_link_speed() to return -ENOENT in case when the property does not exist and -EINVAL if it has invalid value.

Also interpret zero max-link-speed value of this property as invalid, as the device tree bindings documentation specifies.

Update pcie-tegra194 code to handle errors from this function like other drivers - they do not distinguish between no value and invalid value.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/pci/controller/dwc/pcie-tegra194.c |  6 +++---
 drivers/pci/of.c                           | 15 +++++++++++----
 2 files changed, 14 insertions(+), 7 deletions(-)

drivers/pci/controller/dwc/pcie-tegra194.c +3 -3

index ae30a2fd3716..027bb41809f9 100644
@@ -296,7 +296,7 @@ struct tegra_pcie_dw {
        u8 init_link_width;
        u32 msi_ctrl_int;
        u32 num_lanes;
-       u32 max_speed;
+       int max_speed;
        u32 cid;
        u32 cfg_link_cap_l1sub;
        u32 pcie_cap_base;

@@ -911,7 +911,7 @@ static void tegra_pcie_prepare_host(struct pcie_port *pp)
        dw_pcie_writel_dbi(pci, PORT_LOGIC_AMBA_ERROR_RESPONSE_DEFAULT, val);
 
        /* Configure Max Speed from DT */
-       if (pcie->max_speed && pcie->max_speed != -EINVAL) {
+       if (pcie->max_speed > 0) {
                val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base +
                                        PCI_EXP_LNKCAP);
                val &= ~PCI_EXP_LNKCAP_SLS;

@@ -1830,7 +1830,7 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
        dw_pcie_writel_dbi(pci, PORT_LOGIC_GEN2_CTRL, val);
 
        /* Configure Max Speed from DT */
-       if (pcie->max_speed && pcie->max_speed != -EINVAL) {
+       if (pcie->max_speed > 0) {
                val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base +
                                        PCI_EXP_LNKCAP);
                val &= ~PCI_EXP_LNKCAP_SLS;

drivers/pci/of.c +2 -2

index 81ceeaa6f1d5..19bf652256d8 100644
@@ -584,15 +584,22 @@ EXPORT_SYMBOL_GPL(pci_parse_request_of_pci_ranges);
  *
  * @node: device tree node with the max link speed information
  *
- * Returns the associated max link speed from DT, or a negative value if the
- * required property is not found or is invalid.
+ * Returns the associated max link speed from DT, -ENOENT if the required
+ * property is not found or -EINVAL if the required property is invalid.

drivers/pci/of.c EXPORT_SYMBOL_GPL(pci_parse_request_of_pci_ranges); · line 598

Quoted 10 lines

drivers/pci/of.c +6 -0

  */
 int of_pci_get_max_link_speed(struct device_node *node)
 {
        u32 max_link_speed;
+       int ret;
+
+       /* of_property_read_u32 returns -EINVAL if property does not exist */
+       ret = of_property_read_u32(node, "max-link-speed", &max_link_speed);
+       if (ret == -EINVAL)
+               return -ENOENT;

Generally, it's considered bad to change return values (though I guess this was happening. In hindsight, not present probably should have been -ENOENT. But it shouldn't really matter. The kernel should treat malformed as not present. It's not the kernel's job to validate the DT (the schema should and does now).

Quoted 2 lines

Bjorn in review of V1 patch wrote that aardavark driver should at least warn on DT error.

Yes, but I disagree. Just treat an error as not present as long as the driver can make progress. If something critical required is missing, then yes we should print an error and bail out.

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 | 113 ++++++++++++++++++++------
 1 file changed, 88 insertions(+), 25 deletions(-)

drivers/pci/controller/pci-aardvark.c +83 -25

index 74b90940a9d4..a6c4d4d52631 100644
@@ -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 |

drivers/pci/controller/pci-aardvark.c +2 -1

+       pcie->link_gen = (ret > 3) ? 3 : ret;
+
        advk_pcie_setup_hw(pcie);
 
        advk_sw_pci_bridge_init(pcie);
--

2.20.1

Quoted 191 lines · Pali Rohár

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 | 113 ++++++++++++++++++++------
 1 file changed, 88 insertions(+), 25 deletions(-)

drivers/pci/controller/pci-aardvark.c +83 -25

index 74b90940a9d4..a6c4d4d52631 100644
@@ -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,11 @@ static int advk_pcie_probe(struct platform_device *pdev)
        }
        pcie->root_bus_nr = bus->start;
 

drivers/pci/controller/pci-aardvark.c static int advk_pcie_probe(struct platform_device *pdev) · line 1051

Quoted 3 lines

drivers/pci/controller/pci-aardvark.c +3 -0

+       ret = of_pci_get_max_link_speed(dev->of_node);
+       if (ret < 0)
+               return ret;

Why just give up simply on DT error? Just start at gen 3 since you now retry at lower speeds.

Quoted 8 lines

drivers/pci/controller/pci-aardvark.c +2 -1

+       pcie->link_gen = (ret > 3) ? 3 : ret;
+
        advk_pcie_setup_hw(pcie);
 
        advk_sw_pci_bridge_init(pcie);
--

2.20.1

Quoted 5 lines · On Friday 24 April 2020 12:00:54 Rob Herring wrote:

Quoted 4 lines · Pali Rohár

drivers/pci/controller/pci-aardvark.c +0 -0

@@ -988,6 +1046,11 @@ static int advk_pcie_probe(struct platform_device *pdev)
        }
        pcie->root_bus_nr = bus->start;
 

Quoted 6 lines

drivers/pci/controller/pci-aardvark.c static int advk_pcie_probe(struct platform_device *pdev) · line 1051

Quoted 3 lines

drivers/pci/controller/pci-aardvark.c +3 -0

+       ret = of_pci_get_max_link_speed(dev->of_node);
+       if (ret < 0)
+               return ret;

Why just give up simply on DT error? Just start at gen 3 since you now retry at lower speeds.

Ou, I forgot there a special check for ret == -ENOENT.

Quoted 2 lines

Quoted 2 lines

drivers/pci/controller/pci-aardvark.c +2 -0

+       pcie->link_gen = (ret > 3) ? 3 : ret;
+

Add support for issuing PERST via GPIO specified in 'reset-gpios' property (as described in PCI device tree bindings).

Some buggy cards (e.g. Compex WLE900VX or WLE1216) are not detected after reboot when PERST is not issued during driver initialization.

If bootloader already enabled link training then issuing PERST has no effect for some buggy cards (e.g. Compex WLE900VX) and these cards are not detected. We therefore clear the LINK_TRAINING_EN register before.

It was observed that Compex WLE900VX card needs to be in PERST reset for at least 10ms if bootloader enabled link training.

Tested on Turris MOX.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/pci/controller/pci-aardvark.c | 43 ++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

drivers/pci/controller/pci-aardvark.c +31 -1

index a6c4d4d52631..9e2f44213b5e 100644
@@ -9,6 +9,7 @@
  */
 
 #include <linux/delay.h>
+#include <linux/gpio.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>

@@ -18,6 +19,7 @@
 #include <linux/platform_device.h>
 #include <linux/msi.h>
 #include <linux/of_address.h>
+#include <linux/of_gpio.h>
 #include <linux/of_pci.h>
 
 #include "../pci.h"

@@ -204,6 +206,7 @@ struct advk_pcie {
        int root_bus_nr;
        int link_gen;
        struct pci_bridge_emul bridge;
+       struct gpio_desc *reset_gpio;
 };
 
 static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)

@@ -330,10 +333,31 @@ static void advk_pcie_train_link(struct advk_pcie *pcie)
        dev_err(dev, "link never came up\n");
 }
 
+static void advk_pcie_issue_perst(struct advk_pcie *pcie)
+{
+       u32 reg;
+
+       if (!pcie->reset_gpio)
+               return;
+
+       /* PERST does not work for some cards when link training is enabled */
+       reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
+       reg &= ~LINK_TRAINING_EN;
+       advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
+
+       /* 10ms delay is needed for some cards */
+       dev_info(&pcie->pdev->dev, "issuing PERST via reset GPIO for 10ms\n");
+       gpiod_set_value_cansleep(pcie->reset_gpio, 1);
+       usleep_range(10000, 11000);
+       gpiod_set_value_cansleep(pcie->reset_gpio, 0);
+}
+
 static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 {
        u32 reg;
 
+       advk_pcie_issue_perst(pcie);
+
        /* Set to Direct mode */
        reg = advk_readl(pcie, CTRL_CONFIG_REG);
        reg &= ~(CTRL_MODE_MASK << CTRL_MODE_SHIFT);

@@ -406,7 +430,8 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 
        /*
         * PERST# signal could have been asserted by pinctrl subsystem before
-        * probe() callback has been called, making the endpoint going into
+        * probe() callback has been called or issued explicitly by reset gpio
+        * function advk_pcie_issue_perst(), making the endpoint going into
         * fundamental reset. As required by PCI Express spec a delay for at
         * least 100ms after such a reset before link training is needed.
         */

@@ -1046,6 +1071,22 @@ static int advk_pcie_probe(struct platform_device *pdev)
        }
        pcie->root_bus_nr = bus->start;
 
+       pcie->reset_gpio = devm_gpiod_get_from_of_node(dev, dev->of_node,
+                                                      "reset-gpios", 0,
+                                                      GPIOD_OUT_LOW,
+                                                      "pcie1-reset");
+       ret = PTR_ERR_OR_ZERO(pcie->reset_gpio);

Quoted 13 lines · Pali Rohár

drivers/pci/controller/pci-aardvark.c +9 -0

@@ -1046,6 +1071,22 @@ static int advk_pcie_probe(struct platform_device *pdev)
        }
        pcie->root_bus_nr = bus->start;
 
+       pcie->reset_gpio = devm_gpiod_get_from_of_node(dev, dev->of_node,
+                                                      "reset-gpios", 0,
+                                                      GPIOD_OUT_LOW,
+                                                      "pcie1-reset");
+       ret = PTR_ERR_OR_ZERO(pcie->reset_gpio);
+       if (ret) {
+               if (ret == -ENOENT) {
+                       pcie->reset_gpio = NULL;
+               } else {

drivers/pci/controller/pci-aardvark.c static int advk_pcie_probe(struct platform_device *pdev) · line 1088

Quoted 6 lines

drivers/pci/controller/pci-aardvark.c +6 -0

+                       if (ret != -EPROBE_DEFER)
+                               dev_err(dev, "Failed to get reset-gpio: %i\n",
+                                       ret);
+                       return ret;
+               }
+       }

I believe all this can be replaced with devm_gpiod_get_optional.

PCI-E capability macros are already defined in linux/pci_regs.h. Remove their reimplementation in pcie-aardvark.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/pci/controller/pci-aardvark.c | 41 ++++++++++++---------------
 1 file changed, 18 insertions(+), 23 deletions(-)

drivers/pci/controller/pci-aardvark.c +18 -23

index 7a4f395c5812..948e61e76053 100644
@@ -34,17 +34,6 @@
 #define     PCIE_CORE_CMD_MEM_IO_REQ_EN				BIT(2)
 #define PCIE_CORE_DEV_REV_REG					0x8
 #define PCIE_CORE_PCIEXP_CAP					0xc0
-#define PCIE_CORE_DEV_CTRL_STATS_REG				0xc8
-#define     PCIE_CORE_DEV_CTRL_STATS_RELAX_ORDER_DISABLE	(0 << 4)
-#define     PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ_SHIFT	5
-#define     PCIE_CORE_DEV_CTRL_STATS_SNOOP_DISABLE		(0 << 11)
-#define     PCIE_CORE_DEV_CTRL_STATS_MAX_RD_REQ_SIZE_SHIFT	12
-#define     PCIE_CORE_DEV_CTRL_STATS_MAX_RD_REQ_SZ		0x2
-#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)
 #define     PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX_EN			BIT(6)

@@ -223,6 +212,11 @@ static inline u32 advk_readl(struct advk_pcie *pcie, u64 reg)
 	return readl(pcie->base + reg);
 }
 
+static inline u16 advk_read16(struct advk_pcie *pcie, u64 reg)
+{
+	return advk_readl(pcie, (reg & ~0x3)) >> ((reg & 0x3) * 8);
+}
+
 static int advk_pcie_link_up(struct advk_pcie *pcie)
 {
 	u32 val, ltssm_state;

@@ -286,16 +280,16 @@ static int advk_pcie_train_at_gen(struct advk_pcie *pcie, int gen)
 	 * 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);
+	reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_LNKCTL);
+	reg |= PCI_EXP_LNKCTL_RL;
+	advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_LNKCTL);
 
 	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;
+	reg = advk_read16(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_LNKSTA);
+	neg_gen = reg & PCI_EXP_LNKSTA_CLS;
 
 	return neg_gen;
 }

@@ -385,13 +379,14 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 		PCIE_CORE_ERR_CAPCTL_ECRC_CHCK_RCV;
 	advk_writel(pcie, reg, PCIE_CORE_ERR_CAPCTL_REG);
 
-	/* Set PCIe Device Control and Status 1 PF0 register */
-	reg = PCIE_CORE_DEV_CTRL_STATS_RELAX_ORDER_DISABLE |
-		(7 << PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ_SHIFT) |
-		PCIE_CORE_DEV_CTRL_STATS_SNOOP_DISABLE |
-		(PCIE_CORE_DEV_CTRL_STATS_MAX_RD_REQ_SZ <<
-		 PCIE_CORE_DEV_CTRL_STATS_MAX_RD_REQ_SIZE_SHIFT);
-	advk_writel(pcie, reg, PCIE_CORE_DEV_CTRL_STATS_REG);
+	/* Set PCIe Device Control register */
+	reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
+	reg &= ~PCI_EXP_DEVCTL_RELAX_EN;
+	reg &= ~PCI_EXP_DEVCTL_NOSNOOP_EN;
+	reg &= ~PCI_EXP_DEVCTL_READRQ;
+	reg |= PCI_EXP_DEVCTL_PAYLOAD; /* Set max payload size */
+	reg |= PCI_EXP_DEVCTL_READRQ_512B;
+	advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
 
 	/* Program PCIe Control 2 to disable strict ordering */
 	reg = PCIE_CORE_CTRL2_RESERVED |

Signature · 1 line

2.20.1