[PATCH v3 00/12] PCI: aardvark: Fix support for Turris MOX and Compex wifi cards

[PATCH v3 00/12] PCI: aardvark: Fix support for Turris MOX and Compex wifi cards

[Pali Rohár]

Hello,

this is the third 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.

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: Return -ENOENT if max-link-speed property is not found
  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/dwc/pcie-tegra194.c    |   6 +-
 drivers/pci/controller/pci-aardvark.c         | 262 +++++++++++++++---
 drivers/pci/of.c                              |  15 +-
 8 files changed, 243 insertions(+), 59 deletions(-)

-- 
2.20.1

[PATCH v3 01/12] PCI: aardvark: Train link immediately after enabling training

[Pali Rohár]

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 <pali@kernel.org>
---
 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

[PATCH v3 02/12] PCI: aardvark: Don't blindly enable ASPM L0s and don't write to read-only register

[Pali Rohár]

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 <pali@kernel.org>
---
 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

[PATCH v3 03/12] PCI: of: Return -ENOENT if max-link-speed property is not found

[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 <pali@kernel.org>
---
 drivers/pci/controller/dwc/pcie-tegra194.c |  6 +++---
 drivers/pci/of.c                           | 15 +++++++++++----
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index ae30a2fd3716..027bb41809f9 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -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;
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 81ceeaa6f1d5..19bf652256d8 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -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);
+	if (ret == -EINVAL)
+		return -ENOENT;
+	else if (ret)
+		return -EINVAL;
 
-	if (of_property_read_u32(node, "max-link-speed", &max_link_speed) ||
-	    max_link_speed > 4)
+	if (max_link_speed == 0 || max_link_speed > 4)
 		return -EINVAL;
 
 	return max_link_speed;
-- 
2.20.1

Re: [PATCH v3 03/12] PCI: of: Return -ENOENT if max-link-speed property is not found

[Rob Herring]

On Fri, Apr 24, 2020 at 10:39 AM Pali Rohár <pali@kernel.org> wrote:
>
> 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 <pali@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-tegra194.c |  6 +++---
>  drivers/pci/of.c                           | 15 +++++++++++----
>  2 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index ae30a2fd3716..027bb41809f9 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -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;
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 81ceeaa6f1d5..19bf652256d8 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -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);
> +       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

Re: [PATCH v3 03/12] PCI: of: Return -ENOENT if max-link-speed property is not found

[Pali Rohár]

On Friday 24 April 2020 11:47:26 Rob Herring wrote:
> On Fri, Apr 24, 2020 at 10:39 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > 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 <pali@kernel.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-tegra194.c |  6 +++---
> >  drivers/pci/of.c                           | 15 +++++++++++----
> >  2 files changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > index ae30a2fd3716..027bb41809f9 100644
> > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > @@ -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;
> > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > index 81ceeaa6f1d5..19bf652256d8 100644
> > --- a/drivers/pci/of.c
> > +++ b/drivers/pci/of.c
> > @@ -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);
> > +       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.

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

It should have been used in next aardvark patch, but I forgot to include
needed code. My mistake.

> 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

Re: [PATCH v3 03/12] PCI: of: Return -ENOENT if max-link-speed property is not found

[Rob Herring]

On Mon, Apr 27, 2020 at 4:00 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Friday 24 April 2020 11:47:26 Rob Herring wrote:
> > On Fri, Apr 24, 2020 at 10:39 AM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > 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 <pali@kernel.org>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-tegra194.c |  6 +++---
> > >  drivers/pci/of.c                           | 15 +++++++++++----
> > >  2 files changed, 14 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > index ae30a2fd3716..027bb41809f9 100644
> > > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > @@ -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;
> > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > index 81ceeaa6f1d5..19bf652256d8 100644
> > > --- a/drivers/pci/of.c
> > > +++ b/drivers/pci/of.c
> > > @@ -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);
> > > +       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.

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.

> 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.

Rob

Re: [PATCH v3 03/12] PCI: of: Return -ENOENT if max-link-speed property is not found

[Pali Rohár]

On Tuesday 28 April 2020 10:52:34 Rob Herring wrote:
> On Mon, Apr 27, 2020 at 4:00 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Friday 24 April 2020 11:47:26 Rob Herring wrote:
> > > On Fri, Apr 24, 2020 at 10:39 AM Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > 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 <pali@kernel.org>
> > > > ---
> > > >  drivers/pci/controller/dwc/pcie-tegra194.c |  6 +++---
> > > >  drivers/pci/of.c                           | 15 +++++++++++----
> > > >  2 files changed, 14 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > index ae30a2fd3716..027bb41809f9 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > @@ -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;
> > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > > index 81ceeaa6f1d5..19bf652256d8 100644
> > > > --- a/drivers/pci/of.c
> > > > +++ b/drivers/pci/of.c
> > > > @@ -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);
> > > > +       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.
> 
> 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.

Ok Rob, thank you for your input!

I understand your arguments about bloating and also Bjorn's concern
about DT error.

I have no problem with reverting these changes back and silently use
some default value. Just I need to know which option should I choose.

> > 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)

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

> 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.

Re: [PATCH v3 03/12] PCI: of: Return -ENOENT if max-link-speed property is not found

[Marek Behun]

On Tue, 28 Apr 2020 18:01:28 +0200
Pali Rohár <pali@kernel.org> wrote:

> > 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.

I think that's what Rob meant by "Hint: convert your binding", that we
should convert the pci-aardvardk dt binding to yaml.

Re: [PATCH v3 03/12] PCI: of: Return -ENOENT if max-link-speed property is not found

[Bjorn Helgaas]

On Tue, Apr 28, 2020 at 10:52:34AM -0500, Rob Herring wrote:
> On Mon, Apr 27, 2020 at 4:00 AM Pali Rohár <pali@kernel.org> wrote:
> > On Friday 24 April 2020 11:47:26 Rob Herring wrote:
> > > On Fri, Apr 24, 2020 at 10:39 AM Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > 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 <pali@kernel.org>
> > > > ---
> > > >  drivers/pci/controller/dwc/pcie-tegra194.c |  6 +++---
> > > >  drivers/pci/of.c                           | 15 +++++++++++----
> > > >  2 files changed, 14 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > index ae30a2fd3716..027bb41809f9 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > @@ -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;
> > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > > index 81ceeaa6f1d5..19bf652256d8 100644
> > > > --- a/drivers/pci/of.c
> > > > +++ b/drivers/pci/of.c
> > > > @@ -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);
> > > > +       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.
> 
> 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.

That sounds good to me.

Bjorn

[PATCH v3 04/12] PCI: aardvark: Improve link training

[Pali Rohár]

From: Marek Behún <marek.behun@nic.cz>

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 <pali@kernel.org>
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/controller/pci-aardvark.c | 113 ++++++++++++++++++++------
 1 file changed, 88 insertions(+), 25 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 74b90940a9d4..a6c4d4d52631 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,11 @@ 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)
+		return ret;
+	pcie->link_gen = (ret > 3) ? 3 : ret;
+
 	advk_pcie_setup_hw(pcie);
 
 	advk_sw_pci_bridge_init(pcie);
-- 
2.20.1

Re: [PATCH v3 04/12] PCI: aardvark: Improve link training

[Rob Herring]

On Fri, Apr 24, 2020 at 10:39 AM Pali Rohár <pali@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> 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 <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  drivers/pci/controller/pci-aardvark.c | 113 ++++++++++++++++++++------
>  1 file changed, 88 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 74b90940a9d4..a6c4d4d52631 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,11 @@ 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)
> +               return ret;

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

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

Re: [PATCH v3 04/12] PCI: aardvark: Improve link training

[Pali Rohár]

On Friday 24 April 2020 12:00:54 Rob Herring wrote:
> On Fri, Apr 24, 2020 at 10:39 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > From: Marek Behún <marek.behun@nic.cz>
> >
> > 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 <pali@kernel.org>
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 113 ++++++++++++++++++++------
> >  1 file changed, 88 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index 74b90940a9d4..a6c4d4d52631 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,11 @@ 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)
> > +               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.

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

Re: [PATCH v3 04/12] PCI: aardvark: Improve link training

[Pali Rohár]

On Friday 24 April 2020 20:55:23 Pali Rohár wrote:
> On Friday 24 April 2020 12:00:54 Rob Herring wrote:
> > On Fri, Apr 24, 2020 at 10:39 AM Pali Rohár <pali@kernel.org> wrote:
> > > @@ -988,6 +1046,11 @@ 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)
> > > +               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.
> 
> > > +       pcie->link_gen = (ret > 3) ? 3 : ret;
> > > +

This code should have been something like this:

+	ret = of_pci_get_max_link_speed(dev->of_node);
+	if (ret == -ENOENT || ret > 3)
+		pcie->link_gen = 3;
+	else if (ret >= 0)
+		pcie->link_gen = ret;
+	else {
+		dev_err(dev, "Failed to parse max-link-speed\n");
+		return ret;
+	}

[PATCH v3 05/12] PCI: aardvark: Issue PERST via GPIO

[Pali Rohár]

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 <pali@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 43 ++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index a6c4d4d52631..9e2f44213b5e 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>
@@ -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);
+	if (ret) {
+		if (ret == -ENOENT) {
+			pcie->reset_gpio = NULL;
+		} else {
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "Failed to get reset-gpio: %i\n",
+					ret);
+			return ret;
+		}
+	}
+
 	ret = of_pci_get_max_link_speed(dev->of_node);
 	if (ret < 0)
 		return ret;
-- 
2.20.1

Re: [PATCH v3 05/12] PCI: aardvark: Issue PERST via GPIO

[Rob Herring]

On Fri, Apr 24, 2020 at 10:39 AM Pali Rohár <pali@kernel.org> wrote:
>
> 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 <pali@kernel.org>
> ---
>  drivers/pci/controller/pci-aardvark.c | 43 ++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index a6c4d4d52631..9e2f44213b5e 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>
> @@ -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);
> +       if (ret) {
> +               if (ret == -ENOENT) {
> +                       pcie->reset_gpio = NULL;
> +               } else {
> +                       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.

Rob

Re: [PATCH v3 05/12] PCI: aardvark: Issue PERST via GPIO

[Pali Rohár]

On Friday 24 April 2020 12:05:29 Rob Herring wrote:
> On Fri, Apr 24, 2020 at 10:39 AM Pali Rohár <pali@kernel.org> wrote:
> > @@ -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 {
> > +                       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.

I'm looking at the devm_gpiod_get_optional() code and it calls
fwnode_gpiod_get_index() which mangle passed gpio name by appending
suffix from array gpio_suffixes[]. And then mangled name is passed to
fwnode_get_named_gpiod(). At the end devm_gpiod_get_optional() handles
-ENOENT and transform it to NULL.

So via devm_gpiod_get_optional() I cannot request for "reset-gpios" DT
property directly, but I need to specify implicit name "reset" due to
appending suffix from gpio_suffixes[] array. And the only benefit is
that I do not need to handle ret == -ENOENT state.

Handling of -EPROBE_DEFER is still needed in aardvark driver.

For me it makes usage of devm_gpiod_get_optional() in this case harder
to debug code in future. As I would not be able to find a place which
reads 'reset-gpios' DT property via 'git grep reset-gpios'.

So it is really useful for this particular case to use
devm_gpiod_get_optional() instead of devm_gpiod_get_from_of_node().

[PATCH v3 06/12] PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access

[Pali Rohár]

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 <pali@kernel.org>
---
 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 9e2f44213b5e..8e1f61d82c21 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

[PATCH v3 07/12] PCI: aardvark: Add PHY support

[Pali Rohár]

From: Marek Behún <marek.behun@nic.cz>

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 <marek.behun@nic.cz>
Cc: Miquèl Raynal <miquel.raynal@bootlin.com>
---
 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 8e1f61d82c21..7a4f395c5812 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;
@@ -1099,6 +1164,10 @@ static int advk_pcie_probe(struct platform_device *pdev)
 		return ret;
 	pcie->link_gen = (ret > 3) ? 3 : 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

[PATCH v3 08/12] PCI: aardvark: Replace custom macros by standard linux/pci_regs.h macros

[Pali Rohár]

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

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 41 ++++++++++++---------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 7a4f395c5812..948e61e76053 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -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 |
-- 
2.20.1

Re: [PATCH v3 08/12] PCI: aardvark: Replace custom macros by standard linux/pci_regs.h macros

[Bjorn Helgaas]

On Fri, Apr 24, 2020 at 05:38:54PM +0200, Pali Rohár wrote:
> PCI-E capability macros are already defined in linux/pci_regs.h.
> Remove their reimplementation in pcie-aardvark.

s/PCI-E/PCIe/ but only if you need to repost this for some other reason.

Thanks a lot for doing this!  I noticed this while reading through the
driver earlier.  Using the normal names definitely makes this a lot
more readable and greppable.

> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  drivers/pci/controller/pci-aardvark.c | 41 ++++++++++++---------------
>  1 file changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 7a4f395c5812..948e61e76053 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -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 |
> -- 
> 2.20.1
> 

[PATCH v3 09/12] dt-bindings: PCI: aardvark: Describe new properties

[Pali Rohár]

From: Marek Behún <marek.behun@nic.cz>

Document the possibility to reference a PHY and reset-gpios and to set
max-link-speed property.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
 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

[PATCH v3 10/12] arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function

[Pali Rohár]

From: Marek Behún <marek.behun@nic.cz>

We found out that we are unable to control the PERST# signal via the
default pin dedicated to be PERST# pin (GPIO2[3] pin) on A3700 SOC when
this pin is in EP_PCIE1_Resetn mode. There is a register in the PCIe
register space called PERSTN_GPIO_EN (D0088004[3]), but changing the
value of this register does not change the pin output when measuring
with voltmeter.

We do not know if this is a bug in the SOC, or if it works only when
PCIe controller is in a certain state.

Commit f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready
before training link") says that when this pin changes pinctrl mode
from EP_PCIE1_Resetn to GPIO, the PERST# signal is asserted for a brief
moment.

So currently the situation is that on A3700 boards the PERST# signal is
asserted in U-Boot (because the code in U-Boot issues reset via this pin
via GPIO mode), and then in Linux by the obscure and undocumented
mechanism described by the above mentioned commit.

We want to issue PERST# signal in a known way, therefore this patch
changes the pcie_reset_pin function from "pcie" to "gpio" and adds the
reset-gpios property to the PCIe node in device tree files of
EspressoBin and Armada 3720 Dev Board (Turris Mox device tree already
has this property and uDPU does not have a PCIe port).

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Remi Pommarel <repk@triplefau.lt>
---
 arch/arm64/boot/dts/marvell/armada-3720-db.dts           | 3 +++
 arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi | 1 +
 arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts   | 4 ----
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi             | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-db.dts b/arch/arm64/boot/dts/marvell/armada-3720-db.dts
index f2cc00594d64..3e5789f37206 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-db.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-db.dts
@@ -128,6 +128,9 @@
 
 /* CON15(V2.0)/CON17(V1.4) : PCIe / CON15(V2.0)/CON12(V1.4) :mini-PCIe */
 &pcie0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie_reset_pins &pcie_clkreq_pins>;
+	reset-gpios = <&gpiosb 3 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };
 
diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
index 42e992f9c8a5..c92ad664cb0e 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
@@ -47,6 +47,7 @@
 	phys = <&comphy1 0>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie_reset_pins &pcie_clkreq_pins>;
+	reset-gpios = <&gpiosb 3 GPIO_ACTIVE_LOW>;
 };
 
 /* J6 */
diff --git a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
index bb42d1e6a4e9..e496bd9d4737 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
@@ -128,10 +128,6 @@
 	};
 };
 
-&pcie_reset_pins {
-	function = "gpio";
-};
-
 &pcie0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie_reset_pins &pcie_clkreq_pins>;
diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 000c135e39b7..7909c146eabf 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -317,7 +317,7 @@
 
 				pcie_reset_pins: pcie-reset-pins {
 					groups = "pcie1";
-					function = "pcie";
+					function = "gpio";
 				};
 
 				pcie_clkreq_pins: pcie-clkreq-pins {
-- 
2.20.1

[PATCH v3 11/12] arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property

[Pali Rohár]

From: Marek Behún <marek.behun@nic.cz>

Move the comphy handle property of the PCIe node from board specific
device tree files (EspressoBin and Turris Mox) to the generic
armada-37xx.dtsi.

This is correct since this is the only possible PCIe PHY configuration
on Armada 37xx, so when PCIe is enabled on any board, this handle is
correct.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi | 1 -
 arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts   | 1 -
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi             | 1 +
 3 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
index c92ad664cb0e..b97218c72727 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
@@ -44,7 +44,6 @@
 /* J9 */
 &pcie0 {
 	status = "okay";
-	phys = <&comphy1 0>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie_reset_pins &pcie_clkreq_pins>;
 	reset-gpios = <&gpiosb 3 GPIO_ACTIVE_LOW>;
diff --git a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
index e496bd9d4737..15c1cf5c5b69 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
@@ -134,7 +134,6 @@
 	status = "okay";
 	max-link-speed = <2>;
 	reset-gpios = <&gpiosb 3 GPIO_ACTIVE_LOW>;
-	phys = <&comphy1 0>;
 
 	/* enabled by U-Boot if PCIe module is present */
 	status = "disabled";
diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 7909c146eabf..5aaad64a793d 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -493,6 +493,7 @@
 					<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

[PATCH v3 12/12] arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property

[Pali Rohár]

Move the max-link-speed property of the PCIe node from board specific
device tree files to the generic armada-37xx.dtsi.

Armada 37xx supports only PCIe gen2 speed so max-link-speed property
should be in the genetic armada-37xx.dtsi file.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts | 1 -
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi           | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
index 15c1cf5c5b69..4cc735899c5d 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
@@ -132,7 +132,6 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie_reset_pins &pcie_clkreq_pins>;
 	status = "okay";
-	max-link-speed = <2>;
 	reset-gpios = <&gpiosb 3 GPIO_ACTIVE_LOW>;
 
 	/* enabled by U-Boot if PCIe module is present */
diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 5aaad64a793d..2bbc69b4dc99 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -493,6 +493,7 @@
 					<0 0 0 2 &pcie_intc 1>,
 					<0 0 0 3 &pcie_intc 2>,
 					<0 0 0 4 &pcie_intc 3>;
+			max-link-speed = <2>;
 			phys = <&comphy1 0>;
 			pcie_intc: interrupt-controller {
 				interrupt-controller;
-- 
2.20.1