[PATCH u-boot-marvell 00/10] PCI mvebu and aardvark changes

[PATCH u-boot-marvell 00/10] PCI mvebu and aardvark changes

[Marek Behún]

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

Hello Stefan,

we have some more fixes for PCI (mvebu and aardvark), and one patch
for Turris MOX board code.

Marek

Marek Behún (2):
  pci: pci_mvebu, pci_aardvark: Fix size of configuration cache
  arm: mvebu: turris_mox: Remove extra newline after module topology

Pali Rohár (8):
  pci: pci_mvebu: Wait 100ms for Link Up in mvebu_pcie_probe()
  arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c
  pci: pci_mvebu: Move setup for BAR[0] where other BARs are setup
  pci: pci_mvebu: Replace MBUS_PCI_*_SIZE by resource_size()
  pci: pci_mvebu: Do not allow setting ROM BAR on PCI Bridge
  pci: pci_mvebu: Fix PCIe MEM and IO resources assignment and mbus
    mapping
  pci: pci_mvebu: Remove unused DECLARE_GLOBAL_DATA_PTR
  arm: a37xx: pci: Do not allow setting ROM BAR on PCI Bridge

 arch/arm/mach-mvebu/include/mach/cpu.h        |   4 +-
 arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h    |   4 -
 .../serdes/a38x/high_speed_env_spec.c         |  15 --
 board/CZ.NIC/turris_mox/turris_mox.c          |   3 -
 drivers/pci/pci-aardvark.c                    |  54 +++--
 drivers/pci/pci_mvebu.c                       | 205 ++++++++++++------
 6 files changed, 175 insertions(+), 110 deletions(-)

-- 
2.32.0

[PATCH u-boot-marvell 01/10] pci: pci_mvebu: Wait 100ms for Link Up in mvebu_pcie_probe()

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Function mvebu_pcie_probe() configures PCIe controller and sometimes it
takes some time for PCIe card to bring link up. Delay mvebu_pcie_probe()
for 100ms to ensure that PCIe link is up after function finish. In the
case when no card is connected to the PCIe slot, this will delay probe
time by 100ms, which should not be problematic.

This change fixes detection and initialization of some QCA98xx cards on
the first serdes when configured in x1 mode. Default configuration of
the first serdes on A385 is x4 mode, so it looks as if some delay is
required when x4 is changed to x1 and card correctly links with A385.
Other PCIe serdes ports on A385 are x1-only, and so they don't have this
problem.

(We need this patch because in the following patch we are moving the
 configuration change from x4 to x1 from serdes driver to PCIe mvebu
 driver, because the corresponding register lives in the address space
 of the PCIe controller. Without that this explicit timeout is not
 needed, because there is an implicit timeout between change from x4 to
 x1 and probe of PCIe mvebu driver, due to the first being run in SPL
 and the second in U-Boot proper.)

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/pci_mvebu.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
index 14cd82db6f..a3364d5a59 100644
--- a/drivers/pci/pci_mvebu.c
+++ b/drivers/pci/pci_mvebu.c
@@ -22,6 +22,7 @@
 #include <asm/arch/cpu.h>
 #include <asm/arch/soc.h>
 #include <linux/bitops.h>
+#include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/ioport.h>
 #include <linux/mbus.h>
@@ -70,6 +71,9 @@ DECLARE_GLOBAL_DATA_PTR;
 #define PCIE_DEBUG_CTRL			0x1a60
 #define  PCIE_DEBUG_SOFT_RESET		BIT(20)
 
+#define LINK_WAIT_RETRIES	100
+#define LINK_WAIT_TIMEOUT	1000
+
 struct mvebu_pcie {
 	struct pci_controller hose;
 	void __iomem *base;
@@ -106,6 +110,23 @@ static inline bool mvebu_pcie_link_up(struct mvebu_pcie *pcie)
 	return !(val & PCIE_STAT_LINK_DOWN);
 }
 
+static void mvebu_pcie_wait_for_link(struct mvebu_pcie *pcie)
+{
+	int retries;
+
+	/* check if the link is up or not */
+	for (retries = 0; retries < LINK_WAIT_RETRIES; retries++) {
+		if (mvebu_pcie_link_up(pcie)) {
+			printf("%s: Link up\n", pcie->name);
+			return;
+		}
+
+		udelay(LINK_WAIT_TIMEOUT);
+	}
+
+	printf("%s: Link down\n", pcie->name);
+}
+
 static void mvebu_pcie_set_local_bus_nr(struct mvebu_pcie *pcie, int busno)
 {
 	u32 stat;
@@ -477,6 +498,8 @@ static int mvebu_pcie_probe(struct udevice *dev)
 	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
 		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
 
+	mvebu_pcie_wait_for_link(pcie);
+
 	return 0;
 }
 
-- 
2.32.0

Re: [PATCH u-boot-marvell 01/10] pci: pci_mvebu: Wait 100ms for Link Up in mvebu_pcie_probe()

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Function mvebu_pcie_probe() configures PCIe controller and sometimes it
> takes some time for PCIe card to bring link up. Delay mvebu_pcie_probe()
> for 100ms to ensure that PCIe link is up after function finish. In the
> case when no card is connected to the PCIe slot, this will delay probe
> time by 100ms, which should not be problematic.

Where does this 100ms come from? From tests with your PCIe devices /
card?

Please see another comment below...

> This change fixes detection and initialization of some QCA98xx cards on
> the first serdes when configured in x1 mode. Default configuration of
> the first serdes on A385 is x4 mode, so it looks as if some delay is
> required when x4 is changed to x1 and card correctly links with A385.
> Other PCIe serdes ports on A385 are x1-only, and so they don't have this
> problem.
> 
> (We need this patch because in the following patch we are moving the
>   configuration change from x4 to x1 from serdes driver to PCIe mvebu
>   driver, because the corresponding register lives in the address space
>   of the PCIe controller. Without that this explicit timeout is not
>   needed, because there is an implicit timeout between change from x4 to
>   x1 and probe of PCIe mvebu driver, due to the first being run in SPL
>   and the second in U-Boot proper.)
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>   drivers/pci/pci_mvebu.c | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index 14cd82db6f..a3364d5a59 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -22,6 +22,7 @@
>   #include <asm/arch/cpu.h>
>   #include <asm/arch/soc.h>
>   #include <linux/bitops.h>
> +#include <linux/delay.h>
>   #include <linux/errno.h>
>   #include <linux/ioport.h>
>   #include <linux/mbus.h>
> @@ -70,6 +71,9 @@ DECLARE_GLOBAL_DATA_PTR;
>   #define PCIE_DEBUG_CTRL			0x1a60
>   #define  PCIE_DEBUG_SOFT_RESET		BIT(20)
>   
> +#define LINK_WAIT_RETRIES	100
> +#define LINK_WAIT_TIMEOUT	1000

Wouldn't it be easier read, if this was defines like this:

#define LINK_TIMEOUT_MS		100
#define LINK_WAIT_TIMEOUT_US	1000
#define LINK_WAIT_RETRIES	((LINK_TIMEOUT_MS * 1000) / LINK_WAIT_TIMEOUT_US)

Other than this:

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> +
>   struct mvebu_pcie {
>   	struct pci_controller hose;
>   	void __iomem *base;
> @@ -106,6 +110,23 @@ static inline bool mvebu_pcie_link_up(struct mvebu_pcie *pcie)
>   	return !(val & PCIE_STAT_LINK_DOWN);
>   }
>   
> +static void mvebu_pcie_wait_for_link(struct mvebu_pcie *pcie)
> +{
> +	int retries;
> +
> +	/* check if the link is up or not */
> +	for (retries = 0; retries < LINK_WAIT_RETRIES; retries++) {
> +		if (mvebu_pcie_link_up(pcie)) {
> +			printf("%s: Link up\n", pcie->name);
> +			return;
> +		}
> +
> +		udelay(LINK_WAIT_TIMEOUT);
> +	}
> +
> +	printf("%s: Link down\n", pcie->name);
> +}
> +
>   static void mvebu_pcie_set_local_bus_nr(struct mvebu_pcie *pcie, int busno)
>   {
>   	u32 stat;
> @@ -477,6 +498,8 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
>   		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
>   
> +	mvebu_pcie_wait_for_link(pcie);
> +
>   	return 0;
>   }
>   
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 01/10] pci: pci_mvebu: Wait 100ms for Link Up in mvebu_pcie_probe()

[Pali Rohár]

On Friday 12 November 2021 14:59:31 Stefan Roese wrote:
> On 11/11/21 16:35, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > Function mvebu_pcie_probe() configures PCIe controller and sometimes it
> > takes some time for PCIe card to bring link up. Delay mvebu_pcie_probe()
> > for 100ms to ensure that PCIe link is up after function finish. In the
> > case when no card is connected to the PCIe slot, this will delay probe
> > time by 100ms, which should not be problematic.
> 
> Where does this 100ms come from? From tests with your PCIe devices /
> card?

pci-aardvark.c has similar wait timeout, but up to the 1s.

In PCIe base spec 4.0 in section 6.6.1 Conventional Reset is written:
With a Downstream Port that does not support Link speeds greater than
5.0 GT/s, software must wait a minimum of 100 ms before sending a
Configuration Request to the device immediately below that Port.

So I think that waiting 100ms is reasonable... But I do not know what
should be correct here as proper initialization needs more steps...
For more details see my email sent to linux-pci:
https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/

I saw that more drivers in kernel are using different timeouts at
different levels and they are between 1ms-150ms. It is just mess.

> Please see another comment below...
> 
> > This change fixes detection and initialization of some QCA98xx cards on
> > the first serdes when configured in x1 mode. Default configuration of
> > the first serdes on A385 is x4 mode, so it looks as if some delay is
> > required when x4 is changed to x1 and card correctly links with A385.
> > Other PCIe serdes ports on A385 are x1-only, and so they don't have this
> > problem.
> > 
> > (We need this patch because in the following patch we are moving the
> >   configuration change from x4 to x1 from serdes driver to PCIe mvebu
> >   driver, because the corresponding register lives in the address space
> >   of the PCIe controller. Without that this explicit timeout is not
> >   needed, because there is an implicit timeout between change from x4 to
> >   x1 and probe of PCIe mvebu driver, due to the first being run in SPL
> >   and the second in U-Boot proper.)
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >   drivers/pci/pci_mvebu.c | 23 +++++++++++++++++++++++
> >   1 file changed, 23 insertions(+)
> > 
> > diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> > index 14cd82db6f..a3364d5a59 100644
> > --- a/drivers/pci/pci_mvebu.c
> > +++ b/drivers/pci/pci_mvebu.c
> > @@ -22,6 +22,7 @@
> >   #include <asm/arch/cpu.h>
> >   #include <asm/arch/soc.h>
> >   #include <linux/bitops.h>
> > +#include <linux/delay.h>
> >   #include <linux/errno.h>
> >   #include <linux/ioport.h>
> >   #include <linux/mbus.h>
> > @@ -70,6 +71,9 @@ DECLARE_GLOBAL_DATA_PTR;
> >   #define PCIE_DEBUG_CTRL			0x1a60
> >   #define  PCIE_DEBUG_SOFT_RESET		BIT(20)
> > +#define LINK_WAIT_RETRIES	100
> > +#define LINK_WAIT_TIMEOUT	1000
> 
> Wouldn't it be easier read, if this was defines like this:
> 
> #define LINK_TIMEOUT_MS		100
> #define LINK_WAIT_TIMEOUT_US	1000
> #define LINK_WAIT_RETRIES	((LINK_TIMEOUT_MS * 1000) / LINK_WAIT_TIMEOUT_US)
> 
> Other than this:
> 
> Reviewed-by: Stefan Roese <sr@denx.de>
> 
> Thanks,
> Stefan
> 
> > +
> >   struct mvebu_pcie {
> >   	struct pci_controller hose;
> >   	void __iomem *base;
> > @@ -106,6 +110,23 @@ static inline bool mvebu_pcie_link_up(struct mvebu_pcie *pcie)
> >   	return !(val & PCIE_STAT_LINK_DOWN);
> >   }
> > +static void mvebu_pcie_wait_for_link(struct mvebu_pcie *pcie)
> > +{
> > +	int retries;
> > +
> > +	/* check if the link is up or not */
> > +	for (retries = 0; retries < LINK_WAIT_RETRIES; retries++) {
> > +		if (mvebu_pcie_link_up(pcie)) {
> > +			printf("%s: Link up\n", pcie->name);
> > +			return;
> > +		}
> > +
> > +		udelay(LINK_WAIT_TIMEOUT);
> > +	}
> > +
> > +	printf("%s: Link down\n", pcie->name);
> > +}
> > +
> >   static void mvebu_pcie_set_local_bus_nr(struct mvebu_pcie *pcie, int busno)
> >   {
> >   	u32 stat;
> > @@ -477,6 +498,8 @@ static int mvebu_pcie_probe(struct udevice *dev)
> >   	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
> >   		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
> > +	mvebu_pcie_wait_for_link(pcie);
> > +
> >   	return 0;
> >   }
> > 
> 
> Viele Grüße,
> Stefan Roese
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 01/10] pci: pci_mvebu: Wait 100ms for Link Up in mvebu_pcie_probe()

[Stefan Roese]

On 11/12/21 16:44, Pali Rohár wrote:
> On Friday 12 November 2021 14:59:31 Stefan Roese wrote:
>> On 11/11/21 16:35, Marek Behún wrote:
>>> From: Pali Rohár <pali@kernel.org>
>>>
>>> Function mvebu_pcie_probe() configures PCIe controller and sometimes it
>>> takes some time for PCIe card to bring link up. Delay mvebu_pcie_probe()
>>> for 100ms to ensure that PCIe link is up after function finish. In the
>>> case when no card is connected to the PCIe slot, this will delay probe
>>> time by 100ms, which should not be problematic.
>>
>> Where does this 100ms come from? From tests with your PCIe devices /
>> card?
> 
> pci-aardvark.c has similar wait timeout, but up to the 1s.
> 
> In PCIe base spec 4.0 in section 6.6.1 Conventional Reset is written:
> With a Downstream Port that does not support Link speeds greater than
> 5.0 GT/s, software must wait a minimum of 100 ms before sending a
> Configuration Request to the device immediately below that Port.
> 
> So I think that waiting 100ms is reasonable... But I do not know what
> should be correct here as proper initialization needs more steps...
> For more details see my email sent to linux-pci:
> https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/
> 
> I saw that more drivers in kernel are using different timeouts at
> different levels and they are between 1ms-150ms. It is just mess.

Agreed. So let's tick with this 100ms for now.

Thanks,
Stefan

>> Please see another comment below...
>>
>>> This change fixes detection and initialization of some QCA98xx cards on
>>> the first serdes when configured in x1 mode. Default configuration of
>>> the first serdes on A385 is x4 mode, so it looks as if some delay is
>>> required when x4 is changed to x1 and card correctly links with A385.
>>> Other PCIe serdes ports on A385 are x1-only, and so they don't have this
>>> problem.
>>>
>>> (We need this patch because in the following patch we are moving the
>>>    configuration change from x4 to x1 from serdes driver to PCIe mvebu
>>>    driver, because the corresponding register lives in the address space
>>>    of the PCIe controller. Without that this explicit timeout is not
>>>    needed, because there is an implicit timeout between change from x4 to
>>>    x1 and probe of PCIe mvebu driver, due to the first being run in SPL
>>>    and the second in U-Boot proper.)
>>>
>>> Signed-off-by: Pali Rohár <pali@kernel.org>
>>> Signed-off-by: Marek Behún <marek.behun@nic.cz>
>>> ---
>>>    drivers/pci/pci_mvebu.c | 23 +++++++++++++++++++++++
>>>    1 file changed, 23 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
>>> index 14cd82db6f..a3364d5a59 100644
>>> --- a/drivers/pci/pci_mvebu.c
>>> +++ b/drivers/pci/pci_mvebu.c
>>> @@ -22,6 +22,7 @@
>>>    #include <asm/arch/cpu.h>
>>>    #include <asm/arch/soc.h>
>>>    #include <linux/bitops.h>
>>> +#include <linux/delay.h>
>>>    #include <linux/errno.h>
>>>    #include <linux/ioport.h>
>>>    #include <linux/mbus.h>
>>> @@ -70,6 +71,9 @@ DECLARE_GLOBAL_DATA_PTR;
>>>    #define PCIE_DEBUG_CTRL			0x1a60
>>>    #define  PCIE_DEBUG_SOFT_RESET		BIT(20)
>>> +#define LINK_WAIT_RETRIES	100
>>> +#define LINK_WAIT_TIMEOUT	1000
>>
>> Wouldn't it be easier read, if this was defines like this:
>>
>> #define LINK_TIMEOUT_MS		100
>> #define LINK_WAIT_TIMEOUT_US	1000
>> #define LINK_WAIT_RETRIES	((LINK_TIMEOUT_MS * 1000) / LINK_WAIT_TIMEOUT_US)
>>
>> Other than this:
>>
>> Reviewed-by: Stefan Roese <sr@denx.de>
>>
>> Thanks,
>> Stefan
>>
>>> +
>>>    struct mvebu_pcie {
>>>    	struct pci_controller hose;
>>>    	void __iomem *base;
>>> @@ -106,6 +110,23 @@ static inline bool mvebu_pcie_link_up(struct mvebu_pcie *pcie)
>>>    	return !(val & PCIE_STAT_LINK_DOWN);
>>>    }
>>> +static void mvebu_pcie_wait_for_link(struct mvebu_pcie *pcie)
>>> +{
>>> +	int retries;
>>> +
>>> +	/* check if the link is up or not */
>>> +	for (retries = 0; retries < LINK_WAIT_RETRIES; retries++) {
>>> +		if (mvebu_pcie_link_up(pcie)) {
>>> +			printf("%s: Link up\n", pcie->name);
>>> +			return;
>>> +		}
>>> +
>>> +		udelay(LINK_WAIT_TIMEOUT);
>>> +	}
>>> +
>>> +	printf("%s: Link down\n", pcie->name);
>>> +}
>>> +
>>>    static void mvebu_pcie_set_local_bus_nr(struct mvebu_pcie *pcie, int busno)
>>>    {
>>>    	u32 stat;
>>> @@ -477,6 +498,8 @@ static int mvebu_pcie_probe(struct udevice *dev)
>>>    	pcie->cfgcache[(PCI_PREF_MEMORY_BASE - 0x10) / 4] =
>>>    		PCI_PREF_RANGE_TYPE_64 | (PCI_PREF_RANGE_TYPE_64 << 16);
>>> +	mvebu_pcie_wait_for_link(pcie);
>>> +
>>>    	return 0;
>>>    }
>>>
>>
>> Viele Grüße,
>> Stefan Roese
>>
>> -- 
>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 01/10] pci: pci_mvebu: Wait 100ms for Link Up in mvebu_pcie_probe()

[Pali Rohár]

On Friday 12 November 2021 14:59:31 Stefan Roese wrote:
> > diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> > index 14cd82db6f..a3364d5a59 100644
> > --- a/drivers/pci/pci_mvebu.c
> > +++ b/drivers/pci/pci_mvebu.c
> > @@ -22,6 +22,7 @@
> >   #include <asm/arch/cpu.h>
> >   #include <asm/arch/soc.h>
> >   #include <linux/bitops.h>
> > +#include <linux/delay.h>
> >   #include <linux/errno.h>
> >   #include <linux/ioport.h>
> >   #include <linux/mbus.h>
> > @@ -70,6 +71,9 @@ DECLARE_GLOBAL_DATA_PTR;
> >   #define PCIE_DEBUG_CTRL			0x1a60
> >   #define  PCIE_DEBUG_SOFT_RESET		BIT(20)
> > +#define LINK_WAIT_RETRIES	100
> > +#define LINK_WAIT_TIMEOUT	1000
> 
> Wouldn't it be easier read, if this was defines like this:
> 
> #define LINK_TIMEOUT_MS		100
> #define LINK_WAIT_TIMEOUT_US	1000
> #define LINK_WAIT_RETRIES	((LINK_TIMEOUT_MS * 1000) / LINK_WAIT_TIMEOUT_US)

It looks like a good idea!

> Other than this:
> 
> Reviewed-by: Stefan Roese <sr@denx.de>
> 
> Thanks,
> Stefan
> 
> > +
> >   struct mvebu_pcie {
> >   	struct pci_controller hose;
> >   	void __iomem *base;
> > @@ -106,6 +110,23 @@ static inline bool mvebu_pcie_link_up(struct mvebu_pcie *pcie)
> >   	return !(val & PCIE_STAT_LINK_DOWN);
> >   }
> > +static void mvebu_pcie_wait_for_link(struct mvebu_pcie *pcie)
> > +{
> > +	int retries;
> > +
> > +	/* check if the link is up or not */
> > +	for (retries = 0; retries < LINK_WAIT_RETRIES; retries++) {
> > +		if (mvebu_pcie_link_up(pcie)) {
> > +			printf("%s: Link up\n", pcie->name);
> > +			return;
> > +		}
> > +
> > +		udelay(LINK_WAIT_TIMEOUT);
> > +	}
> > +
> > +	printf("%s: Link down\n", pcie->name);
> > +}
> > +
> >   static void mvebu_pcie_set_local_bus_nr(struct mvebu_pcie *pcie, int busno)
> >   {
> >   	u32 stat;

[PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

As explained in commit 3bedbcc3aa18 ("arm: mvebu: a38x: serdes: Don't
overwrite read-only SAR PCIe registers") it is required to set Maximum Link
Width bits of PCIe Root Port Link Capabilities Register depending of number
of used serdes lanes. As this register is part of PCIe address space and
not serdes address space, move it into pci_mvebu.c driver.

Read number of PCIe lanes from DT propery "num-lanes" which is used also by
other PCIe controller drivers in Linux kernel. If this property is absent.
default to 1. This property needs to be set to 4 for every mvebu board
which use PEX_ROOT_COMPLEX_X4. Currently in U-Boot there is no such board.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h     |  4 ----
 .../serdes/a38x/high_speed_env_spec.c          | 15 ---------------
 drivers/pci/pci_mvebu.c                        | 18 ++++++++++++++++++
 3 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
index 64193d5288..0df898c625 100644
--- a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
+++ b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
@@ -6,12 +6,8 @@
 #ifndef _CTRL_PEX_H
 #define _CTRL_PEX_H
 
-#include <pci.h>
 #include "high_speed_env_spec.h"
 
-/* Direct access to PEX0 Root Port's PCIe Capability structure */
-#define PEX0_RP_PCIE_CFG_OFFSET		(0x00080000 + 0x60)
-
 /* SOC_CONTROL_REG1 fields */
 #define PCIE0_ENABLE_OFFS		0
 #define PCIE0_ENABLE_MASK		(0x1 << PCIE0_ENABLE_OFFS)
diff --git a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
index d2bc3ab25c..ef4b89c96a 100644
--- a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
+++ b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
@@ -1720,21 +1720,6 @@ int serdes_power_up_ctrl(u32 serdes_num, int serdes_power_up,
 				else
 					reg_data &= ~0x4000;
 				reg_write(SOC_CONTROL_REG1, reg_data);
-
-				/*
-				 * Set Maximum Link Width to X1 or X4 in Root
-				 * Port's PCIe Link Capability register.
-				 * This register is read-only but if is not set
-				 * correctly then access to PCI config space of
-				 * endpoint card behind this Root Port does not
-				 * work.
-				 */
-				reg_data = reg_read(PEX0_RP_PCIE_CFG_OFFSET +
-						    PCI_EXP_LNKCAP);
-				reg_data &= ~PCI_EXP_LNKCAP_MLW;
-				reg_data |= (is_pex_by1 ? 1 : 4) << 4;
-				reg_write(PEX0_RP_PCIE_CFG_OFFSET +
-					  PCI_EXP_LNKCAP, reg_data);
 			}
 
 			CHECK_STATUS(mv_seq_exec(serdes_num, PEX_POWER_UP_SEQ));
diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
index a3364d5a59..278dc2756f 100644
--- a/drivers/pci/pci_mvebu.c
+++ b/drivers/pci/pci_mvebu.c
@@ -83,6 +83,7 @@ struct mvebu_pcie {
 	struct resource io;
 	u32 port;
 	u32 lane;
+	bool is_x4;
 	int devfn;
 	u32 lane_mask;
 	int first_busno;
@@ -399,6 +400,18 @@ static int mvebu_pcie_probe(struct udevice *dev)
 	reg |= PCIE_CTRL_RC_MODE;
 	writel(reg, pcie->base + PCIE_CTRL_OFF);
 
+	/*
+	 * Set Maximum Link Width to X1 or X4 in Root Port's PCIe Link
+	 * Capability register. This register is defined by PCIe specification
+	 * as read-only but this mvebu controller has it as read-write and must
+	 * be set to number of SerDes PCIe lanes (1 or 4). If this register is
+	 * not set correctly then link with endpoint card is not established.
+	 */
+	reg = readl(pcie->base + PCIE_CAPAB_OFF + PCI_EXP_LNKCAP);
+	reg &= ~PCI_EXP_LNKCAP_MLW;
+	reg |= (pcie->is_x4 ? 4 : 1) << 4;
+	writel(reg, pcie->base + PCIE_CAPAB_OFF + PCI_EXP_LNKCAP);
+
 	/*
 	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
 	 * because default value is Memory controller (0x508000) which
@@ -582,6 +595,7 @@ static int mvebu_get_tgt_attr(ofnode node, int devfn,
 static int mvebu_pcie_of_to_plat(struct udevice *dev)
 {
 	struct mvebu_pcie *pcie = dev_get_plat(dev);
+	u32 num_lanes = 1;
 	int ret = 0;
 
 	/* Get port number, lane number and memory target / attr */
@@ -594,6 +608,10 @@ static int mvebu_pcie_of_to_plat(struct udevice *dev)
 	if (ofnode_read_u32(dev_ofnode(dev), "marvell,pcie-lane", &pcie->lane))
 		pcie->lane = 0;
 
+	if (!ofnode_read_u32(dev_ofnode(dev), "num-lanes", &num_lanes) &&
+	    num_lanes == 4)
+		pcie->is_x4 = true;
+
 	sprintf(pcie->name, "pcie%d.%d", pcie->port, pcie->lane);
 
 	/* pci_get_devfn() returns devfn in bits 15..8, see PCI_DEV usage */
-- 
2.32.0

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> As explained in commit 3bedbcc3aa18 ("arm: mvebu: a38x: serdes: Don't
> overwrite read-only SAR PCIe registers") it is required to set Maximum Link
> Width bits of PCIe Root Port Link Capabilities Register depending of number
> of used serdes lanes. As this register is part of PCIe address space and
> not serdes address space, move it into pci_mvebu.c driver.
> 
> Read number of PCIe lanes from DT propery "num-lanes" which is used also by
> other PCIe controller drivers in Linux kernel. If this property is absent.
> default to 1. This property needs to be set to 4 for every mvebu board
> which use PEX_ROOT_COMPLEX_X4. Currently in U-Boot there is no such board.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>   arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h     |  4 ----
>   .../serdes/a38x/high_speed_env_spec.c          | 15 ---------------
>   drivers/pci/pci_mvebu.c                        | 18 ++++++++++++++++++
>   3 files changed, 18 insertions(+), 19 deletions(-)

I'm wondering now, if and how this works on Armada XP, which uses the
same PCIe driver but a different serdes/axp/*. Did you take this into
account?

Thanks,
Stefan

> diff --git a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
> index 64193d5288..0df898c625 100644
> --- a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
> +++ b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
> @@ -6,12 +6,8 @@
>   #ifndef _CTRL_PEX_H
>   #define _CTRL_PEX_H
>   
> -#include <pci.h>
>   #include "high_speed_env_spec.h"
>   
> -/* Direct access to PEX0 Root Port's PCIe Capability structure */
> -#define PEX0_RP_PCIE_CFG_OFFSET		(0x00080000 + 0x60)
> -
>   /* SOC_CONTROL_REG1 fields */
>   #define PCIE0_ENABLE_OFFS		0
>   #define PCIE0_ENABLE_MASK		(0x1 << PCIE0_ENABLE_OFFS)
> diff --git a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
> index d2bc3ab25c..ef4b89c96a 100644
> --- a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
> +++ b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
> @@ -1720,21 +1720,6 @@ int serdes_power_up_ctrl(u32 serdes_num, int serdes_power_up,
>   				else
>   					reg_data &= ~0x4000;
>   				reg_write(SOC_CONTROL_REG1, reg_data);
> -
> -				/*
> -				 * Set Maximum Link Width to X1 or X4 in Root
> -				 * Port's PCIe Link Capability register.
> -				 * This register is read-only but if is not set
> -				 * correctly then access to PCI config space of
> -				 * endpoint card behind this Root Port does not
> -				 * work.
> -				 */
> -				reg_data = reg_read(PEX0_RP_PCIE_CFG_OFFSET +
> -						    PCI_EXP_LNKCAP);
> -				reg_data &= ~PCI_EXP_LNKCAP_MLW;
> -				reg_data |= (is_pex_by1 ? 1 : 4) << 4;
> -				reg_write(PEX0_RP_PCIE_CFG_OFFSET +
> -					  PCI_EXP_LNKCAP, reg_data);
>   			}
>   
>   			CHECK_STATUS(mv_seq_exec(serdes_num, PEX_POWER_UP_SEQ));
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index a3364d5a59..278dc2756f 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -83,6 +83,7 @@ struct mvebu_pcie {
>   	struct resource io;
>   	u32 port;
>   	u32 lane;
> +	bool is_x4;
>   	int devfn;
>   	u32 lane_mask;
>   	int first_busno;
> @@ -399,6 +400,18 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   	reg |= PCIE_CTRL_RC_MODE;
>   	writel(reg, pcie->base + PCIE_CTRL_OFF);
>   
> +	/*
> +	 * Set Maximum Link Width to X1 or X4 in Root Port's PCIe Link
> +	 * Capability register. This register is defined by PCIe specification
> +	 * as read-only but this mvebu controller has it as read-write and must
> +	 * be set to number of SerDes PCIe lanes (1 or 4). If this register is
> +	 * not set correctly then link with endpoint card is not established.
> +	 */
> +	reg = readl(pcie->base + PCIE_CAPAB_OFF + PCI_EXP_LNKCAP);
> +	reg &= ~PCI_EXP_LNKCAP_MLW;
> +	reg |= (pcie->is_x4 ? 4 : 1) << 4;
> +	writel(reg, pcie->base + PCIE_CAPAB_OFF + PCI_EXP_LNKCAP);
> +
>   	/*
>   	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
>   	 * because default value is Memory controller (0x508000) which
> @@ -582,6 +595,7 @@ static int mvebu_get_tgt_attr(ofnode node, int devfn,
>   static int mvebu_pcie_of_to_plat(struct udevice *dev)
>   {
>   	struct mvebu_pcie *pcie = dev_get_plat(dev);
> +	u32 num_lanes = 1;
>   	int ret = 0;
>   
>   	/* Get port number, lane number and memory target / attr */
> @@ -594,6 +608,10 @@ static int mvebu_pcie_of_to_plat(struct udevice *dev)
>   	if (ofnode_read_u32(dev_ofnode(dev), "marvell,pcie-lane", &pcie->lane))
>   		pcie->lane = 0;
>   
> +	if (!ofnode_read_u32(dev_ofnode(dev), "num-lanes", &num_lanes) &&
> +	    num_lanes == 4)
> +		pcie->is_x4 = true;
> +
>   	sprintf(pcie->name, "pcie%d.%d", pcie->port, pcie->lane);
>   
>   	/* pci_get_devfn() returns devfn in bits 15..8, see PCI_DEV usage */
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Pali Rohár]

On Friday 12 November 2021 15:01:57 Stefan Roese wrote:
> On 11/11/21 16:35, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > As explained in commit 3bedbcc3aa18 ("arm: mvebu: a38x: serdes: Don't
> > overwrite read-only SAR PCIe registers") it is required to set Maximum Link
> > Width bits of PCIe Root Port Link Capabilities Register depending of number
> > of used serdes lanes. As this register is part of PCIe address space and
> > not serdes address space, move it into pci_mvebu.c driver.
> > 
> > Read number of PCIe lanes from DT propery "num-lanes" which is used also by
> > other PCIe controller drivers in Linux kernel. If this property is absent.
> > default to 1. This property needs to be set to 4 for every mvebu board
> > which use PEX_ROOT_COMPLEX_X4. Currently in U-Boot there is no such board.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >   arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h     |  4 ----
> >   .../serdes/a38x/high_speed_env_spec.c          | 15 ---------------
> >   drivers/pci/pci_mvebu.c                        | 18 ++++++++++++++++++
> >   3 files changed, 18 insertions(+), 19 deletions(-)
> 
> I'm wondering now, if and how this works on Armada XP, which uses the
> same PCIe driver but a different serdes/axp/*. Did you take this into
> account?

It looks like that axp serdes code also touches register of PCIe Root
Ports, e.g. in section /* 6.2 PCI Express Link Capabilities */. So it is
something which could be improved and cleaned. But it should not cause
any issue if registers are configures two times, once in serdes code and
once in pci-mvebu.c code. Moreover registers in pci-mvebu space,
including config space of PCIe Root Ports are reconfigured by kernel, so
I think that it should not cause any issue.

But what could cause issues is X1 vs X4 configuration in pci-mvebu.c if
"num-lanes" property is not properly set. As is mentioned in commit
message there is no A38x board in U-Boot which uses X4.

Now with your comment for Armada XP I checked that serdes code and find
out that AXP uses macro 'PEX_BUS_MODE_X4' for X4 mode (A38x serdes uses
PEX_ROOT_COMPLEX_X4). And in U-Boot there are two boards which sets this
macro: board/Synology/ds414/ds414.c and board/theadorable/theadorable.c

So it would be needed to adjust this patch for these two boards. Please
hold this one patch for now. I will try to prepare new fixed version.
Other patches should be OK and independent of this one.

> Thanks,
> Stefan
> 
> > diff --git a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
> > index 64193d5288..0df898c625 100644
> > --- a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
> > +++ b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
> > @@ -6,12 +6,8 @@
> >   #ifndef _CTRL_PEX_H
> >   #define _CTRL_PEX_H
> > -#include <pci.h>
> >   #include "high_speed_env_spec.h"
> > -/* Direct access to PEX0 Root Port's PCIe Capability structure */
> > -#define PEX0_RP_PCIE_CFG_OFFSET		(0x00080000 + 0x60)
> > -
> >   /* SOC_CONTROL_REG1 fields */
> >   #define PCIE0_ENABLE_OFFS		0
> >   #define PCIE0_ENABLE_MASK		(0x1 << PCIE0_ENABLE_OFFS)
> > diff --git a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
> > index d2bc3ab25c..ef4b89c96a 100644
> > --- a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
> > +++ b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
> > @@ -1720,21 +1720,6 @@ int serdes_power_up_ctrl(u32 serdes_num, int serdes_power_up,
> >   				else
> >   					reg_data &= ~0x4000;
> >   				reg_write(SOC_CONTROL_REG1, reg_data);
> > -
> > -				/*
> > -				 * Set Maximum Link Width to X1 or X4 in Root
> > -				 * Port's PCIe Link Capability register.
> > -				 * This register is read-only but if is not set
> > -				 * correctly then access to PCI config space of
> > -				 * endpoint card behind this Root Port does not
> > -				 * work.
> > -				 */
> > -				reg_data = reg_read(PEX0_RP_PCIE_CFG_OFFSET +
> > -						    PCI_EXP_LNKCAP);
> > -				reg_data &= ~PCI_EXP_LNKCAP_MLW;
> > -				reg_data |= (is_pex_by1 ? 1 : 4) << 4;
> > -				reg_write(PEX0_RP_PCIE_CFG_OFFSET +
> > -					  PCI_EXP_LNKCAP, reg_data);
> >   			}
> >   			CHECK_STATUS(mv_seq_exec(serdes_num, PEX_POWER_UP_SEQ));
> > diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> > index a3364d5a59..278dc2756f 100644
> > --- a/drivers/pci/pci_mvebu.c
> > +++ b/drivers/pci/pci_mvebu.c
> > @@ -83,6 +83,7 @@ struct mvebu_pcie {
> >   	struct resource io;
> >   	u32 port;
> >   	u32 lane;
> > +	bool is_x4;
> >   	int devfn;
> >   	u32 lane_mask;
> >   	int first_busno;
> > @@ -399,6 +400,18 @@ static int mvebu_pcie_probe(struct udevice *dev)
> >   	reg |= PCIE_CTRL_RC_MODE;
> >   	writel(reg, pcie->base + PCIE_CTRL_OFF);
> > +	/*
> > +	 * Set Maximum Link Width to X1 or X4 in Root Port's PCIe Link
> > +	 * Capability register. This register is defined by PCIe specification
> > +	 * as read-only but this mvebu controller has it as read-write and must
> > +	 * be set to number of SerDes PCIe lanes (1 or 4). If this register is
> > +	 * not set correctly then link with endpoint card is not established.
> > +	 */
> > +	reg = readl(pcie->base + PCIE_CAPAB_OFF + PCI_EXP_LNKCAP);
> > +	reg &= ~PCI_EXP_LNKCAP_MLW;
> > +	reg |= (pcie->is_x4 ? 4 : 1) << 4;
> > +	writel(reg, pcie->base + PCIE_CAPAB_OFF + PCI_EXP_LNKCAP);
> > +
> >   	/*
> >   	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
> >   	 * because default value is Memory controller (0x508000) which
> > @@ -582,6 +595,7 @@ static int mvebu_get_tgt_attr(ofnode node, int devfn,
> >   static int mvebu_pcie_of_to_plat(struct udevice *dev)
> >   {
> >   	struct mvebu_pcie *pcie = dev_get_plat(dev);
> > +	u32 num_lanes = 1;
> >   	int ret = 0;
> >   	/* Get port number, lane number and memory target / attr */
> > @@ -594,6 +608,10 @@ static int mvebu_pcie_of_to_plat(struct udevice *dev)
> >   	if (ofnode_read_u32(dev_ofnode(dev), "marvell,pcie-lane", &pcie->lane))
> >   		pcie->lane = 0;
> > +	if (!ofnode_read_u32(dev_ofnode(dev), "num-lanes", &num_lanes) &&
> > +	    num_lanes == 4)
> > +		pcie->is_x4 = true;
> > +
> >   	sprintf(pcie->name, "pcie%d.%d", pcie->port, pcie->lane);
> >   	/* pci_get_devfn() returns devfn in bits 15..8, see PCI_DEV usage */
> > 
> 
> Viele Grüße,
> Stefan Roese
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Stefan Roese]

On 11/18/21 19:01, Pali Rohár wrote:
> On Friday 12 November 2021 15:01:57 Stefan Roese wrote:
>> On 11/11/21 16:35, Marek Behún wrote:
>>> From: Pali Rohár <pali@kernel.org>
>>>
>>> As explained in commit 3bedbcc3aa18 ("arm: mvebu: a38x: serdes: Don't
>>> overwrite read-only SAR PCIe registers") it is required to set Maximum Link
>>> Width bits of PCIe Root Port Link Capabilities Register depending of number
>>> of used serdes lanes. As this register is part of PCIe address space and
>>> not serdes address space, move it into pci_mvebu.c driver.
>>>
>>> Read number of PCIe lanes from DT propery "num-lanes" which is used also by
>>> other PCIe controller drivers in Linux kernel. If this property is absent.
>>> default to 1. This property needs to be set to 4 for every mvebu board
>>> which use PEX_ROOT_COMPLEX_X4. Currently in U-Boot there is no such board.
>>>
>>> Signed-off-by: Pali Rohár <pali@kernel.org>
>>> Signed-off-by: Marek Behún <marek.behun@nic.cz>
>>> ---
>>>    arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h     |  4 ----
>>>    .../serdes/a38x/high_speed_env_spec.c          | 15 ---------------
>>>    drivers/pci/pci_mvebu.c                        | 18 ++++++++++++++++++
>>>    3 files changed, 18 insertions(+), 19 deletions(-)
>>
>> I'm wondering now, if and how this works on Armada XP, which uses the
>> same PCIe driver but a different serdes/axp/*. Did you take this into
>> account?
> 
> It looks like that axp serdes code also touches register of PCIe Root
> Ports, e.g. in section /* 6.2 PCI Express Link Capabilities */. So it is
> something which could be improved and cleaned. But it should not cause
> any issue if registers are configures two times, once in serdes code and
> once in pci-mvebu.c code. Moreover registers in pci-mvebu space,
> including config space of PCIe Root Ports are reconfigured by kernel, so
> I think that it should not cause any issue.

I assume that the AXP serdes code is very similar to the A38x code that
you recently overhauled very thoroughly. For the AXP serdes code, I know
that the PCIe link is already established in the serdes code right now.
And since we had some link-up issues on the theadorable AXP board, we
have been trying to optimize / tune this in this ugly serdes code at few
years ago. From what I understand, you've removed all this PCIe specific
code in the A38x serdes part, so that the link establishment now happens
in the PCIe driver itself (pci_mvebu.c). Which makes perfect sense IMHO.

Since A38x and AXP share the same PCIe driver in U-Boot, I would very
much like to see some similar changes being made to the AXP serdes code
as well, so that the link establishment solely will happen for all
these platforms in the PCIe driver.

> But what could cause issues is X1 vs X4 configuration in pci-mvebu.c if
> "num-lanes" property is not properly set. As is mentioned in commit
> message there is no A38x board in U-Boot which uses X4.
> 
> Now with your comment for Armada XP I checked that serdes code and find
> out that AXP uses macro 'PEX_BUS_MODE_X4' for X4 mode (A38x serdes uses
> PEX_ROOT_COMPLEX_X4). And in U-Boot there are two boards which sets this
> macro: board/Synology/ds414/ds414.c and board/theadorable/theadorable.c
> 
> So it would be needed to adjust this patch for these two boards. Please
> hold this one patch for now. I will try to prepare new fixed version.
> Other patches should be OK and independent of this one.

Thanks. And yes, theadorable has one x4 and one x1.

Thanks,
Stefan

>> Thanks,
>> Stefan
>>
>>> diff --git a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
>>> index 64193d5288..0df898c625 100644
>>> --- a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
>>> +++ b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
>>> @@ -6,12 +6,8 @@
>>>    #ifndef _CTRL_PEX_H
>>>    #define _CTRL_PEX_H
>>> -#include <pci.h>
>>>    #include "high_speed_env_spec.h"
>>> -/* Direct access to PEX0 Root Port's PCIe Capability structure */
>>> -#define PEX0_RP_PCIE_CFG_OFFSET		(0x00080000 + 0x60)
>>> -
>>>    /* SOC_CONTROL_REG1 fields */
>>>    #define PCIE0_ENABLE_OFFS		0
>>>    #define PCIE0_ENABLE_MASK		(0x1 << PCIE0_ENABLE_OFFS)
>>> diff --git a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
>>> index d2bc3ab25c..ef4b89c96a 100644
>>> --- a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
>>> +++ b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
>>> @@ -1720,21 +1720,6 @@ int serdes_power_up_ctrl(u32 serdes_num, int serdes_power_up,
>>>    				else
>>>    					reg_data &= ~0x4000;
>>>    				reg_write(SOC_CONTROL_REG1, reg_data);
>>> -
>>> -				/*
>>> -				 * Set Maximum Link Width to X1 or X4 in Root
>>> -				 * Port's PCIe Link Capability register.
>>> -				 * This register is read-only but if is not set
>>> -				 * correctly then access to PCI config space of
>>> -				 * endpoint card behind this Root Port does not
>>> -				 * work.
>>> -				 */
>>> -				reg_data = reg_read(PEX0_RP_PCIE_CFG_OFFSET +
>>> -						    PCI_EXP_LNKCAP);
>>> -				reg_data &= ~PCI_EXP_LNKCAP_MLW;
>>> -				reg_data |= (is_pex_by1 ? 1 : 4) << 4;
>>> -				reg_write(PEX0_RP_PCIE_CFG_OFFSET +
>>> -					  PCI_EXP_LNKCAP, reg_data);
>>>    			}
>>>    			CHECK_STATUS(mv_seq_exec(serdes_num, PEX_POWER_UP_SEQ));
>>> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
>>> index a3364d5a59..278dc2756f 100644
>>> --- a/drivers/pci/pci_mvebu.c
>>> +++ b/drivers/pci/pci_mvebu.c
>>> @@ -83,6 +83,7 @@ struct mvebu_pcie {
>>>    	struct resource io;
>>>    	u32 port;
>>>    	u32 lane;
>>> +	bool is_x4;
>>>    	int devfn;
>>>    	u32 lane_mask;
>>>    	int first_busno;
>>> @@ -399,6 +400,18 @@ static int mvebu_pcie_probe(struct udevice *dev)
>>>    	reg |= PCIE_CTRL_RC_MODE;
>>>    	writel(reg, pcie->base + PCIE_CTRL_OFF);
>>> +	/*
>>> +	 * Set Maximum Link Width to X1 or X4 in Root Port's PCIe Link
>>> +	 * Capability register. This register is defined by PCIe specification
>>> +	 * as read-only but this mvebu controller has it as read-write and must
>>> +	 * be set to number of SerDes PCIe lanes (1 or 4). If this register is
>>> +	 * not set correctly then link with endpoint card is not established.
>>> +	 */
>>> +	reg = readl(pcie->base + PCIE_CAPAB_OFF + PCI_EXP_LNKCAP);
>>> +	reg &= ~PCI_EXP_LNKCAP_MLW;
>>> +	reg |= (pcie->is_x4 ? 4 : 1) << 4;
>>> +	writel(reg, pcie->base + PCIE_CAPAB_OFF + PCI_EXP_LNKCAP);
>>> +
>>>    	/*
>>>    	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
>>>    	 * because default value is Memory controller (0x508000) which
>>> @@ -582,6 +595,7 @@ static int mvebu_get_tgt_attr(ofnode node, int devfn,
>>>    static int mvebu_pcie_of_to_plat(struct udevice *dev)
>>>    {
>>>    	struct mvebu_pcie *pcie = dev_get_plat(dev);
>>> +	u32 num_lanes = 1;
>>>    	int ret = 0;
>>>    	/* Get port number, lane number and memory target / attr */
>>> @@ -594,6 +608,10 @@ static int mvebu_pcie_of_to_plat(struct udevice *dev)
>>>    	if (ofnode_read_u32(dev_ofnode(dev), "marvell,pcie-lane", &pcie->lane))
>>>    		pcie->lane = 0;
>>> +	if (!ofnode_read_u32(dev_ofnode(dev), "num-lanes", &num_lanes) &&
>>> +	    num_lanes == 4)
>>> +		pcie->is_x4 = true;
>>> +
>>>    	sprintf(pcie->name, "pcie%d.%d", pcie->port, pcie->lane);
>>>    	/* pci_get_devfn() returns devfn in bits 15..8, see PCI_DEV usage */
>>>
>>
>> Viele Grüße,
>> Stefan Roese
>>
>> -- 
>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Pali Rohár]

On Friday 19 November 2021 07:55:00 Stefan Roese wrote:
> On 11/18/21 19:01, Pali Rohár wrote:
> > On Friday 12 November 2021 15:01:57 Stefan Roese wrote:
> > > On 11/11/21 16:35, Marek Behún wrote:
> > > > From: Pali Rohár <pali@kernel.org>
> > > > 
> > > > As explained in commit 3bedbcc3aa18 ("arm: mvebu: a38x: serdes: Don't
> > > > overwrite read-only SAR PCIe registers") it is required to set Maximum Link
> > > > Width bits of PCIe Root Port Link Capabilities Register depending of number
> > > > of used serdes lanes. As this register is part of PCIe address space and
> > > > not serdes address space, move it into pci_mvebu.c driver.
> > > > 
> > > > Read number of PCIe lanes from DT propery "num-lanes" which is used also by
> > > > other PCIe controller drivers in Linux kernel. If this property is absent.
> > > > default to 1. This property needs to be set to 4 for every mvebu board
> > > > which use PEX_ROOT_COMPLEX_X4. Currently in U-Boot there is no such board.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > > > ---
> > > >    arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h     |  4 ----
> > > >    .../serdes/a38x/high_speed_env_spec.c          | 15 ---------------
> > > >    drivers/pci/pci_mvebu.c                        | 18 ++++++++++++++++++
> > > >    3 files changed, 18 insertions(+), 19 deletions(-)
> > > 
> > > I'm wondering now, if and how this works on Armada XP, which uses the
> > > same PCIe driver but a different serdes/axp/*. Did you take this into
> > > account?
> > 
> > It looks like that axp serdes code also touches register of PCIe Root
> > Ports, e.g. in section /* 6.2 PCI Express Link Capabilities */. So it is
> > something which could be improved and cleaned. But it should not cause
> > any issue if registers are configures two times, once in serdes code and
> > once in pci-mvebu.c code. Moreover registers in pci-mvebu space,
> > including config space of PCIe Root Ports are reconfigured by kernel, so
> > I think that it should not cause any issue.
> 
> I assume that the AXP serdes code is very similar to the A38x code that
> you recently overhauled very thoroughly. For the AXP serdes code, I know
> that the PCIe link is already established in the serdes code right now.
> And since we had some link-up issues on the theadorable AXP board, we
> have been trying to optimize / tune this in this ugly serdes code at few
> years ago. From what I understand, you've removed all this PCIe specific
> code in the A38x serdes part, so that the link establishment now happens
> in the PCIe driver itself (pci_mvebu.c). Which makes perfect sense IMHO.
> 
> Since A38x and AXP share the same PCIe driver in U-Boot, I would very
> much like to see some similar changes being made to the AXP serdes code
> as well, so that the link establishment solely will happen for all
> these platforms in the PCIe driver.

I have looked into AXP serdes code and seems to be similar mess like it
was in A38x serdes code. Unfortunately I do not want to touch this code
and do movement without heavy hardware testing as it can be easy to
break. And I do not have Theadorable board for testing.

Anyway, all changes done in pci_mvebu.c driver just configures registers
to correct or expected values, so they should have low probability of
breaking existing hardware...

> > But what could cause issues is X1 vs X4 configuration in pci-mvebu.c if
> > "num-lanes" property is not properly set. As is mentioned in commit
> > message there is no A38x board in U-Boot which uses X4.
> > 
> > Now with your comment for Armada XP I checked that serdes code and find
> > out that AXP uses macro 'PEX_BUS_MODE_X4' for X4 mode (A38x serdes uses
> > PEX_ROOT_COMPLEX_X4). And in U-Boot there are two boards which sets this
> > macro: board/Synology/ds414/ds414.c and board/theadorable/theadorable.c
> > 
> > So it would be needed to adjust this patch for these two boards. Please
> > hold this one patch for now. I will try to prepare new fixed version.
> > Other patches should be OK and independent of this one.
> 
> Thanks. And yes, theadorable has one x4 and one x1.

I think that this change should be enough:

diff --git a/arch/arm/dts/armada-xp-synology-ds414.dts b/arch/arm/dts/armada-xp-synology-ds414.dts
index 861967cd7e87..35909e3c69c6 100644
--- a/arch/arm/dts/armada-xp-synology-ds414.dts
+++ b/arch/arm/dts/armada-xp-synology-ds414.dts
@@ -187,6 +187,7 @@
 	pcie@1,0 {
 		/* Port 0, Lane 0 */
 		status = "okay";
+		num-lanes = <4>;
 	};
 
 	/*
diff --git a/arch/arm/dts/armada-xp-theadorable.dts b/arch/arm/dts/armada-xp-theadorable.dts
index 6a1df870ab56..726676b3e1d5 100644
--- a/arch/arm/dts/armada-xp-theadorable.dts
+++ b/arch/arm/dts/armada-xp-theadorable.dts
@@ -202,5 +202,6 @@
 	pcie@9,0 {
 		/* Port 2, Lane 0 */
 		status = "okay";
+		num-lanes = <4>;
 	};
 };

After this DTS change, pci-mvebu.c will just replace value of current
number of lanes (which is set to 4 by serdes code) to value from DTS,
which is 4. Therefore there should be no change.

Could you test whole patch series with above DTS change if it works
properly on Theadorable board?

> Thanks,
> Stefan
> 
> > > Thanks,
> > > Stefan
> > > 
> > > > diff --git a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
> > > > index 64193d5288..0df898c625 100644
> > > > --- a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
> > > > +++ b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
> > > > @@ -6,12 +6,8 @@
> > > >    #ifndef _CTRL_PEX_H
> > > >    #define _CTRL_PEX_H
> > > > -#include <pci.h>
> > > >    #include "high_speed_env_spec.h"
> > > > -/* Direct access to PEX0 Root Port's PCIe Capability structure */
> > > > -#define PEX0_RP_PCIE_CFG_OFFSET		(0x00080000 + 0x60)
> > > > -
> > > >    /* SOC_CONTROL_REG1 fields */
> > > >    #define PCIE0_ENABLE_OFFS		0
> > > >    #define PCIE0_ENABLE_MASK		(0x1 << PCIE0_ENABLE_OFFS)
> > > > diff --git a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
> > > > index d2bc3ab25c..ef4b89c96a 100644
> > > > --- a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
> > > > +++ b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
> > > > @@ -1720,21 +1720,6 @@ int serdes_power_up_ctrl(u32 serdes_num, int serdes_power_up,
> > > >    				else
> > > >    					reg_data &= ~0x4000;
> > > >    				reg_write(SOC_CONTROL_REG1, reg_data);
> > > > -
> > > > -				/*
> > > > -				 * Set Maximum Link Width to X1 or X4 in Root
> > > > -				 * Port's PCIe Link Capability register.
> > > > -				 * This register is read-only but if is not set
> > > > -				 * correctly then access to PCI config space of
> > > > -				 * endpoint card behind this Root Port does not
> > > > -				 * work.
> > > > -				 */
> > > > -				reg_data = reg_read(PEX0_RP_PCIE_CFG_OFFSET +
> > > > -						    PCI_EXP_LNKCAP);
> > > > -				reg_data &= ~PCI_EXP_LNKCAP_MLW;
> > > > -				reg_data |= (is_pex_by1 ? 1 : 4) << 4;
> > > > -				reg_write(PEX0_RP_PCIE_CFG_OFFSET +
> > > > -					  PCI_EXP_LNKCAP, reg_data);
> > > >    			}
> > > >    			CHECK_STATUS(mv_seq_exec(serdes_num, PEX_POWER_UP_SEQ));
> > > > diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> > > > index a3364d5a59..278dc2756f 100644
> > > > --- a/drivers/pci/pci_mvebu.c
> > > > +++ b/drivers/pci/pci_mvebu.c
> > > > @@ -83,6 +83,7 @@ struct mvebu_pcie {
> > > >    	struct resource io;
> > > >    	u32 port;
> > > >    	u32 lane;
> > > > +	bool is_x4;
> > > >    	int devfn;
> > > >    	u32 lane_mask;
> > > >    	int first_busno;
> > > > @@ -399,6 +400,18 @@ static int mvebu_pcie_probe(struct udevice *dev)
> > > >    	reg |= PCIE_CTRL_RC_MODE;
> > > >    	writel(reg, pcie->base + PCIE_CTRL_OFF);
> > > > +	/*
> > > > +	 * Set Maximum Link Width to X1 or X4 in Root Port's PCIe Link
> > > > +	 * Capability register. This register is defined by PCIe specification
> > > > +	 * as read-only but this mvebu controller has it as read-write and must
> > > > +	 * be set to number of SerDes PCIe lanes (1 or 4). If this register is
> > > > +	 * not set correctly then link with endpoint card is not established.
> > > > +	 */
> > > > +	reg = readl(pcie->base + PCIE_CAPAB_OFF + PCI_EXP_LNKCAP);
> > > > +	reg &= ~PCI_EXP_LNKCAP_MLW;
> > > > +	reg |= (pcie->is_x4 ? 4 : 1) << 4;
> > > > +	writel(reg, pcie->base + PCIE_CAPAB_OFF + PCI_EXP_LNKCAP);
> > > > +
> > > >    	/*
> > > >    	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400)
> > > >    	 * because default value is Memory controller (0x508000) which
> > > > @@ -582,6 +595,7 @@ static int mvebu_get_tgt_attr(ofnode node, int devfn,
> > > >    static int mvebu_pcie_of_to_plat(struct udevice *dev)
> > > >    {
> > > >    	struct mvebu_pcie *pcie = dev_get_plat(dev);
> > > > +	u32 num_lanes = 1;
> > > >    	int ret = 0;
> > > >    	/* Get port number, lane number and memory target / attr */
> > > > @@ -594,6 +608,10 @@ static int mvebu_pcie_of_to_plat(struct udevice *dev)
> > > >    	if (ofnode_read_u32(dev_ofnode(dev), "marvell,pcie-lane", &pcie->lane))
> > > >    		pcie->lane = 0;
> > > > +	if (!ofnode_read_u32(dev_ofnode(dev), "num-lanes", &num_lanes) &&
> > > > +	    num_lanes == 4)
> > > > +		pcie->is_x4 = true;
> > > > +
> > > >    	sprintf(pcie->name, "pcie%d.%d", pcie->port, pcie->lane);
> > > >    	/* pci_get_devfn() returns devfn in bits 15..8, see PCI_DEV usage */
> > > > 
> > > 
> > > Viele Grüße,
> > > Stefan Roese
> > > 
> > > -- 
> > > DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> > > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> > > Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
> 
> Viele Grüße,
> Stefan Roese
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Stefan Roese]

Hi Pali,

On 11/23/21 16:59, Pali Rohár wrote:
> On Friday 19 November 2021 07:55:00 Stefan Roese wrote:
>> On 11/18/21 19:01, Pali Rohár wrote:
>>> On Friday 12 November 2021 15:01:57 Stefan Roese wrote:
>>>> On 11/11/21 16:35, Marek Behún wrote:
>>>>> From: Pali Rohár <pali@kernel.org>
>>>>>
>>>>> As explained in commit 3bedbcc3aa18 ("arm: mvebu: a38x: serdes: Don't
>>>>> overwrite read-only SAR PCIe registers") it is required to set Maximum Link
>>>>> Width bits of PCIe Root Port Link Capabilities Register depending of number
>>>>> of used serdes lanes. As this register is part of PCIe address space and
>>>>> not serdes address space, move it into pci_mvebu.c driver.
>>>>>
>>>>> Read number of PCIe lanes from DT propery "num-lanes" which is used also by
>>>>> other PCIe controller drivers in Linux kernel. If this property is absent.
>>>>> default to 1. This property needs to be set to 4 for every mvebu board
>>>>> which use PEX_ROOT_COMPLEX_X4. Currently in U-Boot there is no such board.
>>>>>
>>>>> Signed-off-by: Pali Rohár <pali@kernel.org>
>>>>> Signed-off-by: Marek Behún <marek.behun@nic.cz>
>>>>> ---
>>>>>     arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h     |  4 ----
>>>>>     .../serdes/a38x/high_speed_env_spec.c          | 15 ---------------
>>>>>     drivers/pci/pci_mvebu.c                        | 18 ++++++++++++++++++
>>>>>     3 files changed, 18 insertions(+), 19 deletions(-)
>>>>
>>>> I'm wondering now, if and how this works on Armada XP, which uses the
>>>> same PCIe driver but a different serdes/axp/*. Did you take this into
>>>> account?
>>>
>>> It looks like that axp serdes code also touches register of PCIe Root
>>> Ports, e.g. in section /* 6.2 PCI Express Link Capabilities */. So it is
>>> something which could be improved and cleaned. But it should not cause
>>> any issue if registers are configures two times, once in serdes code and
>>> once in pci-mvebu.c code. Moreover registers in pci-mvebu space,
>>> including config space of PCIe Root Ports are reconfigured by kernel, so
>>> I think that it should not cause any issue.
>>
>> I assume that the AXP serdes code is very similar to the A38x code that
>> you recently overhauled very thoroughly. For the AXP serdes code, I know
>> that the PCIe link is already established in the serdes code right now.
>> And since we had some link-up issues on the theadorable AXP board, we
>> have been trying to optimize / tune this in this ugly serdes code at few
>> years ago. From what I understand, you've removed all this PCIe specific
>> code in the A38x serdes part, so that the link establishment now happens
>> in the PCIe driver itself (pci_mvebu.c). Which makes perfect sense IMHO.
>>
>> Since A38x and AXP share the same PCIe driver in U-Boot, I would very
>> much like to see some similar changes being made to the AXP serdes code
>> as well, so that the link establishment solely will happen for all
>> these platforms in the PCIe driver.
> 
> I have looked into AXP serdes code and seems to be similar mess like it
> was in A38x serdes code. Unfortunately I do not want to touch this code
> and do movement without heavy hardware testing as it can be easy to
> break. And I do not have Theadorable board for testing.

I fully agree, that such a rework / cleanup, as you have done for a38x,
can only be done with intensive testing. I might try to find some time
in the future to work on this, as theadorable is still actively used and
PCIe is always a potential basket of trouble here.

> Anyway, all changes done in pci_mvebu.c driver just configures registers
> to correct or expected values, so they should have low probability of
> breaking existing hardware...

Okay. Please see below...

>>> But what could cause issues is X1 vs X4 configuration in pci-mvebu.c if
>>> "num-lanes" property is not properly set. As is mentioned in commit
>>> message there is no A38x board in U-Boot which uses X4.
>>>
>>> Now with your comment for Armada XP I checked that serdes code and find
>>> out that AXP uses macro 'PEX_BUS_MODE_X4' for X4 mode (A38x serdes uses
>>> PEX_ROOT_COMPLEX_X4). And in U-Boot there are two boards which sets this
>>> macro: board/Synology/ds414/ds414.c and board/theadorable/theadorable.c
>>>
>>> So it would be needed to adjust this patch for these two boards. Please
>>> hold this one patch for now. I will try to prepare new fixed version.
>>> Other patches should be OK and independent of this one.
>>
>> Thanks. And yes, theadorable has one x4 and one x1.
> 
> I think that this change should be enough:
> 
> diff --git a/arch/arm/dts/armada-xp-synology-ds414.dts b/arch/arm/dts/armada-xp-synology-ds414.dts
> index 861967cd7e87..35909e3c69c6 100644
> --- a/arch/arm/dts/armada-xp-synology-ds414.dts
> +++ b/arch/arm/dts/armada-xp-synology-ds414.dts
> @@ -187,6 +187,7 @@
>   	pcie@1,0 {
>   		/* Port 0, Lane 0 */
>   		status = "okay";
> +		num-lanes = <4>;
>   	};
>   
>   	/*
> diff --git a/arch/arm/dts/armada-xp-theadorable.dts b/arch/arm/dts/armada-xp-theadorable.dts
> index 6a1df870ab56..726676b3e1d5 100644
> --- a/arch/arm/dts/armada-xp-theadorable.dts
> +++ b/arch/arm/dts/armada-xp-theadorable.dts
> @@ -202,5 +202,6 @@
>   	pcie@9,0 {
>   		/* Port 2, Lane 0 */
>   		status = "okay";
> +		num-lanes = <4>;
>   	};
>   };
> 
> After this DTS change, pci-mvebu.c will just replace value of current
> number of lanes (which is set to 4 by serdes code) to value from DTS,
> which is 4. Therefore there should be no change.
> 
> Could you test whole patch series with above DTS change if it works
> properly on Theadorable board?

Yes, I don't see any issues with this patchset applied plus this DT
patch on theadorable. The PCIe links are up and with the correct width.

What I'm wondering is, when exactly does the PCIe RP start the link
establishment. In my case with AXP this is still in the AXP serdes code
of course. But in the A38x code, it should be in the PCIe controller
driver now AFAIU. I see that you configure the link width in the
controller and do some other configuration (address windows etc), but
at the end you "simply" wait for the link to come up via
mvebu_pcie_wait_for_link(). I would have expected here some special
command (config bit?) to the PCIe controller to start the link
establishment. So when exactly does the A38x start this action?

Thanks,
Stefan

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Pali Rohár]

On Monday 29 November 2021 08:46:47 Stefan Roese wrote:
> Hi Pali,
> 
> On 11/23/21 16:59, Pali Rohár wrote:
> > On Friday 19 November 2021 07:55:00 Stefan Roese wrote:
> > > On 11/18/21 19:01, Pali Rohár wrote:
> > > > On Friday 12 November 2021 15:01:57 Stefan Roese wrote:
> > > > > On 11/11/21 16:35, Marek Behún wrote:
> > > > > > From: Pali Rohár <pali@kernel.org>
> > > > > > 
> > > > > > As explained in commit 3bedbcc3aa18 ("arm: mvebu: a38x: serdes: Don't
> > > > > > overwrite read-only SAR PCIe registers") it is required to set Maximum Link
> > > > > > Width bits of PCIe Root Port Link Capabilities Register depending of number
> > > > > > of used serdes lanes. As this register is part of PCIe address space and
> > > > > > not serdes address space, move it into pci_mvebu.c driver.
> > > > > > 
> > > > > > Read number of PCIe lanes from DT propery "num-lanes" which is used also by
> > > > > > other PCIe controller drivers in Linux kernel. If this property is absent.
> > > > > > default to 1. This property needs to be set to 4 for every mvebu board
> > > > > > which use PEX_ROOT_COMPLEX_X4. Currently in U-Boot there is no such board.
> > > > > > 
> > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > > > > > ---
> > > > > >     arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h     |  4 ----
> > > > > >     .../serdes/a38x/high_speed_env_spec.c          | 15 ---------------
> > > > > >     drivers/pci/pci_mvebu.c                        | 18 ++++++++++++++++++
> > > > > >     3 files changed, 18 insertions(+), 19 deletions(-)
> > > > > 
> > > > > I'm wondering now, if and how this works on Armada XP, which uses the
> > > > > same PCIe driver but a different serdes/axp/*. Did you take this into
> > > > > account?
> > > > 
> > > > It looks like that axp serdes code also touches register of PCIe Root
> > > > Ports, e.g. in section /* 6.2 PCI Express Link Capabilities */. So it is
> > > > something which could be improved and cleaned. But it should not cause
> > > > any issue if registers are configures two times, once in serdes code and
> > > > once in pci-mvebu.c code. Moreover registers in pci-mvebu space,
> > > > including config space of PCIe Root Ports are reconfigured by kernel, so
> > > > I think that it should not cause any issue.
> > > 
> > > I assume that the AXP serdes code is very similar to the A38x code that
> > > you recently overhauled very thoroughly. For the AXP serdes code, I know
> > > that the PCIe link is already established in the serdes code right now.
> > > And since we had some link-up issues on the theadorable AXP board, we
> > > have been trying to optimize / tune this in this ugly serdes code at few
> > > years ago. From what I understand, you've removed all this PCIe specific
> > > code in the A38x serdes part, so that the link establishment now happens
> > > in the PCIe driver itself (pci_mvebu.c). Which makes perfect sense IMHO.
> > > 
> > > Since A38x and AXP share the same PCIe driver in U-Boot, I would very
> > > much like to see some similar changes being made to the AXP serdes code
> > > as well, so that the link establishment solely will happen for all
> > > these platforms in the PCIe driver.
> > 
> > I have looked into AXP serdes code and seems to be similar mess like it
> > was in A38x serdes code. Unfortunately I do not want to touch this code
> > and do movement without heavy hardware testing as it can be easy to
> > break. And I do not have Theadorable board for testing.
> 
> I fully agree, that such a rework / cleanup, as you have done for a38x,
> can only be done with intensive testing. I might try to find some time
> in the future to work on this, as theadorable is still actively used and
> PCIe is always a potential basket of trouble here.
> 
> > Anyway, all changes done in pci_mvebu.c driver just configures registers
> > to correct or expected values, so they should have low probability of
> > breaking existing hardware...
> 
> Okay. Please see below...
> 
> > > > But what could cause issues is X1 vs X4 configuration in pci-mvebu.c if
> > > > "num-lanes" property is not properly set. As is mentioned in commit
> > > > message there is no A38x board in U-Boot which uses X4.
> > > > 
> > > > Now with your comment for Armada XP I checked that serdes code and find
> > > > out that AXP uses macro 'PEX_BUS_MODE_X4' for X4 mode (A38x serdes uses
> > > > PEX_ROOT_COMPLEX_X4). And in U-Boot there are two boards which sets this
> > > > macro: board/Synology/ds414/ds414.c and board/theadorable/theadorable.c
> > > > 
> > > > So it would be needed to adjust this patch for these two boards. Please
> > > > hold this one patch for now. I will try to prepare new fixed version.
> > > > Other patches should be OK and independent of this one.
> > > 
> > > Thanks. And yes, theadorable has one x4 and one x1.
> > 
> > I think that this change should be enough:
> > 
> > diff --git a/arch/arm/dts/armada-xp-synology-ds414.dts b/arch/arm/dts/armada-xp-synology-ds414.dts
> > index 861967cd7e87..35909e3c69c6 100644
> > --- a/arch/arm/dts/armada-xp-synology-ds414.dts
> > +++ b/arch/arm/dts/armada-xp-synology-ds414.dts
> > @@ -187,6 +187,7 @@
> >   	pcie@1,0 {
> >   		/* Port 0, Lane 0 */
> >   		status = "okay";
> > +		num-lanes = <4>;
> >   	};
> >   	/*
> > diff --git a/arch/arm/dts/armada-xp-theadorable.dts b/arch/arm/dts/armada-xp-theadorable.dts
> > index 6a1df870ab56..726676b3e1d5 100644
> > --- a/arch/arm/dts/armada-xp-theadorable.dts
> > +++ b/arch/arm/dts/armada-xp-theadorable.dts
> > @@ -202,5 +202,6 @@
> >   	pcie@9,0 {
> >   		/* Port 2, Lane 0 */
> >   		status = "okay";
> > +		num-lanes = <4>;
> >   	};
> >   };
> > 
> > After this DTS change, pci-mvebu.c will just replace value of current
> > number of lanes (which is set to 4 by serdes code) to value from DTS,
> > which is 4. Therefore there should be no change.
> > 
> > Could you test whole patch series with above DTS change if it works
> > properly on Theadorable board?
> 
> Yes, I don't see any issues with this patchset applied plus this DT
> patch on theadorable. The PCIe links are up and with the correct width.
> 
> What I'm wondering is, when exactly does the PCIe RP start the link
> establishment. In my case with AXP this is still in the AXP serdes code
> of course. But in the A38x code, it should be in the PCIe controller
> driver now AFAIU. I see that you configure the link width in the
> controller and do some other configuration (address windows etc), but
> at the end you "simply" wait for the link to come up via
> mvebu_pcie_wait_for_link(). I would have expected here some special
> command (config bit?) to the PCIe controller to start the link
> establishment. So when exactly does the A38x start this action?

That is interesting question... While I'm reading it again, I really do
not know. Because you are right that mvebu_pcie_wait_for_link() is just
waiting for a link and it "magically" comes up. I have tested it on A385
and it is stable with different Compex Atheros cards which caused issues
in past also on A3720.

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Stefan Roese]

On 11/29/21 10:06, Pali Rohár wrote:

<snip>

>>> After this DTS change, pci-mvebu.c will just replace value of current
>>> number of lanes (which is set to 4 by serdes code) to value from DTS,
>>> which is 4. Therefore there should be no change.
>>>
>>> Could you test whole patch series with above DTS change if it works
>>> properly on Theadorable board?
>>
>> Yes, I don't see any issues with this patchset applied plus this DT
>> patch on theadorable. The PCIe links are up and with the correct width.
>>
>> What I'm wondering is, when exactly does the PCIe RP start the link
>> establishment. In my case with AXP this is still in the AXP serdes code
>> of course. But in the A38x code, it should be in the PCIe controller
>> driver now AFAIU. I see that you configure the link width in the
>> controller and do some other configuration (address windows etc), but
>> at the end you "simply" wait for the link to come up via
>> mvebu_pcie_wait_for_link(). I would have expected here some special
>> command (config bit?) to the PCIe controller to start the link
>> establishment. So when exactly does the A38x start this action?
> 
> That is interesting question... While I'm reading it again, I really do
> not know. Because you are right that mvebu_pcie_wait_for_link() is just
> waiting for a link and it "magically" comes up. I have tested it on A385
> and it is stable with different Compex Atheros cards which caused issues
> in past also on A3720.

I would prefer, to fully understand when exactly the link establishment
is started. Since this is crucial for the setup of the controller that
needs to be done *before* the link starts to come up.

Could you perhaps try to remove some of the register configurations in
the MVEBU PCIe driver to see, if the link establishment relies on this
register to be written to (e.g. PCI_EXP_LNKCAP)?

Thanks,
Stefan

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Pali Rohár]

Hello!

On Monday 29 November 2021 10:22:58 Stefan Roese wrote:
> On 11/29/21 10:06, Pali Rohár wrote:
> 
> <snip>
> 
> > > > After this DTS change, pci-mvebu.c will just replace value of current
> > > > number of lanes (which is set to 4 by serdes code) to value from DTS,
> > > > which is 4. Therefore there should be no change.
> > > > 
> > > > Could you test whole patch series with above DTS change if it works
> > > > properly on Theadorable board?
> > > 
> > > Yes, I don't see any issues with this patchset applied plus this DT
> > > patch on theadorable. The PCIe links are up and with the correct width.
> > > 
> > > What I'm wondering is, when exactly does the PCIe RP start the link
> > > establishment. In my case with AXP this is still in the AXP serdes code
> > > of course. But in the A38x code, it should be in the PCIe controller
> > > driver now AFAIU. I see that you configure the link width in the
> > > controller and do some other configuration (address windows etc), but
> > > at the end you "simply" wait for the link to come up via
> > > mvebu_pcie_wait_for_link(). I would have expected here some special
> > > command (config bit?) to the PCIe controller to start the link
> > > establishment. So when exactly does the A38x start this action?
> > 
> > That is interesting question... While I'm reading it again, I really do
> > not know. Because you are right that mvebu_pcie_wait_for_link() is just
> > waiting for a link and it "magically" comes up. I have tested it on A385
> > and it is stable with different Compex Atheros cards which caused issues
> > in past also on A3720.
> 
> I would prefer, to fully understand when exactly the link establishment
> is started. Since this is crucial for the setup of the controller that
> needs to be done *before* the link starts to come up.

I try to dig as more information as possible and finally I find out that
important information is available also in now removed, but originally
public A38x documentation. Thankfully web archive has copy of it:

https://web.archive.org/web/20200420191927/https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-embedded-processors-armada-38x-functional-specifications-2015-11.pdf

  17.3 Link Initialization

  In case the initialization fails and no link is established, the PHY
  will keep on trying to initiate a link forever unless the port is
  disabled. As long as the port is enabled, the PHY will continue trying
  to establish a link; once the PHY identifies that a device is
  connected to it, a link will be established.

PCIe port is enabled by bits in SoC Control 1 Register, which is done in
U-Boot SerDes initialization code. This is IIRC SoC specific, and reason
why every Armada SoC has own SerDes init code.

And looks like that due to "the PHY will keep on trying to initiate a
link forever", the PCIe link comes up when pci-mvebu.c sets all required
registers to correct values. E.g. set correct mode (RC vs endpoint),
link width (x1 vs x4), etc...

> Could you perhaps try to remove some of the register configurations in
> the MVEBU PCIe driver to see, if the link establishment relies on this
> register to be written to (e.g. PCI_EXP_LNKCAP)?

First port on A385 is by default set to X4 width, other ports to X1
width. Without updating LNKCAP to correct width, card in first PCIe port
never initialize. And cards in other ports are initialized even before
pci-mvebu.c starts configuration.

So seems that this matches above behavior. SerDes init code enabled all
PCIe ports. Ports which are using default configuration (second, third)
are immediately initialized and link is established. Port which requires
additional configuration (first port, for switching from X4 to X1) just
stay in that "keep on trying to initiate a link forever" state until
pci-mvebu starts and set PCI_EXP_LNKCAP register, after which PHY try X1
width and success. And seems that this is the reason why 100ms timeout
is needed... As at this stage when pci-mvebu.c switches X4 to X1, init
timeout as defined in PCIe spec (that 100ms) starts ticking. For other
ports it starts ticking when serdes init code enables ports.


I have looked into all PCIe registers which are present in functional
spec, but it looks like that there is no pci-mvebu register which can
turn of LTSSM and link training, like it is in other PCIe controllers.
It looks like that only SoC-specific port enable bits are there.

It is starting to be bigger mess as before... Any suggestion how to
continue with it?

We cannot (easily) move that code which flips PCIe bits in SoC Control 1
Register from SerDes init code to pci-mvebu.c as this is outside of
pci-mvebu.c address space and also it is different on every SoC.
pci-mvebu.c registers are same on all Marvell SoCs, starting from Orion
up to the A39x.

> Thanks,
> Stefan

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Stefan Roese]

Hi Pali,

On 11/29/21 12:47, Pali Rohár wrote:
> Hello!
> 
> On Monday 29 November 2021 10:22:58 Stefan Roese wrote:
>> On 11/29/21 10:06, Pali Rohár wrote:
>>
>> <snip>
>>
>>>>> After this DTS change, pci-mvebu.c will just replace value of current
>>>>> number of lanes (which is set to 4 by serdes code) to value from DTS,
>>>>> which is 4. Therefore there should be no change.
>>>>>
>>>>> Could you test whole patch series with above DTS change if it works
>>>>> properly on Theadorable board?
>>>>
>>>> Yes, I don't see any issues with this patchset applied plus this DT
>>>> patch on theadorable. The PCIe links are up and with the correct width.
>>>>
>>>> What I'm wondering is, when exactly does the PCIe RP start the link
>>>> establishment. In my case with AXP this is still in the AXP serdes code
>>>> of course. But in the A38x code, it should be in the PCIe controller
>>>> driver now AFAIU. I see that you configure the link width in the
>>>> controller and do some other configuration (address windows etc), but
>>>> at the end you "simply" wait for the link to come up via
>>>> mvebu_pcie_wait_for_link(). I would have expected here some special
>>>> command (config bit?) to the PCIe controller to start the link
>>>> establishment. So when exactly does the A38x start this action?
>>>
>>> That is interesting question... While I'm reading it again, I really do
>>> not know. Because you are right that mvebu_pcie_wait_for_link() is just
>>> waiting for a link and it "magically" comes up. I have tested it on A385
>>> and it is stable with different Compex Atheros cards which caused issues
>>> in past also on A3720.
>>
>> I would prefer, to fully understand when exactly the link establishment
>> is started. Since this is crucial for the setup of the controller that
>> needs to be done *before* the link starts to come up.
> 
> I try to dig as more information as possible and finally I find out that
> important information is available also in now removed, but originally
> public A38x documentation. Thankfully web archive has copy of it:
> 
> https://web.archive.org/web/20200420191927/https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-embedded-processors-armada-38x-functional-specifications-2015-11.pdf
> 
>    17.3 Link Initialization
> 
>    In case the initialization fails and no link is established, the PHY
>    will keep on trying to initiate a link forever unless the port is
>    disabled. As long as the port is enabled, the PHY will continue trying
>    to establish a link; once the PHY identifies that a device is
>    connected to it, a link will be established.
> 
> PCIe port is enabled by bits in SoC Control 1 Register, which is done in
> U-Boot SerDes initialization code. This is IIRC SoC specific, and reason
> why every Armada SoC has own SerDes init code.
> 
> And looks like that due to "the PHY will keep on trying to initiate a
> link forever", the PCIe link comes up when pci-mvebu.c sets all required
> registers to correct values. E.g. set correct mode (RC vs endpoint),
> link width (x1 vs x4), etc...
> 
>> Could you perhaps try to remove some of the register configurations in
>> the MVEBU PCIe driver to see, if the link establishment relies on this
>> register to be written to (e.g. PCI_EXP_LNKCAP)?
> 
> First port on A385 is by default set to X4 width, other ports to X1
> width. Without updating LNKCAP to correct width, card in first PCIe port
> never initialize. And cards in other ports are initialized even before
> pci-mvebu.c starts configuration.

So the PCIe ports are now trying to establish the links, even when the
correct configuration is not yet done. This might work but sound far
from perfect to me IMHO.

> So seems that this matches above behavior. SerDes init code enabled all
> PCIe ports. Ports which are using default configuration (second, third)
> are immediately initialized and link is established. Port which requires
> additional configuration (first port, for switching from X4 to X1) just
> stay in that "keep on trying to initiate a link forever" state until
> pci-mvebu starts and set PCI_EXP_LNKCAP register, after which PHY try X1
> width and success. And seems that this is the reason why 100ms timeout
> is needed... As at this stage when pci-mvebu.c switches X4 to X1, init
> timeout as defined in PCIe spec (that 100ms) starts ticking. For other
> ports it starts ticking when serdes init code enables ports.
> 
> 
> I have looked into all PCIe registers which are present in functional
> spec, but it looks like that there is no pci-mvebu register which can
> turn of LTSSM and link training, like it is in other PCIe controllers.
> It looks like that only SoC-specific port enable bits are there.
> 
> It is starting to be bigger mess as before... Any suggestion how to
> continue with it?
> 
> We cannot (easily) move that code which flips PCIe bits in SoC Control 1
> Register from SerDes init code to pci-mvebu.c as this is outside of
> pci-mvebu.c address space and also it is different on every SoC.
> pci-mvebu.c registers are same on all Marvell SoCs, starting from Orion
> up to the A39x.

One idea would be, to use a "reset-controller" driver on the Armada
platforms, that is capable to at least reset and release the PCIe ports.
Via the SoC Control 1 reg on A38x and via the SoC Control Register
on AXP.

I just looked into some Linux PCIe DT bindings and found e.g. this in
the mediatek spec:

Documentation/devicetree/bindings/pci/mediatek-pcie.txt
...
Required properties for MT7623/MT2701:
...
- resets: Must contain an entry for each entry in reset-names.
   See ../reset/reset.txt for details.
- reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the
   number of root ports.
...
		resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>,
			 <&hifsys MT2701_HIFSYS_PCIE1_RST>,
			 <&hifsys MT2701_HIFSYS_PCIE2_RST>;
		reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2";
		phys = <&pcie0_phy PHY_TYPE_PCIE>, <&pcie1_phy PHY_TYPE_PCIE>,
		       <&pcie2_phy PHY_TYPE_PCIE>;
		phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2";


And make sure in the serdes code keeps (or actively sets?) these PCIe
ports into the reset state. The PCIe driver would then release the ports
out of reset after their configuration.

Or is some other serdes code missing in between "get PCIe port out of
reset" and the MVEBU PCIe driver taking over the control?

What do you think? I might be missing something here.

Thanks,
Stefan

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Pali Rohár]

On Monday 29 November 2021 13:30:45 Stefan Roese wrote:
> Hi Pali,
> 
> On 11/29/21 12:47, Pali Rohár wrote:
> > Hello!
> > 
> > On Monday 29 November 2021 10:22:58 Stefan Roese wrote:
> > > On 11/29/21 10:06, Pali Rohár wrote:
> > > 
> > > <snip>
> > > 
> > > > > > After this DTS change, pci-mvebu.c will just replace value of current
> > > > > > number of lanes (which is set to 4 by serdes code) to value from DTS,
> > > > > > which is 4. Therefore there should be no change.
> > > > > > 
> > > > > > Could you test whole patch series with above DTS change if it works
> > > > > > properly on Theadorable board?
> > > > > 
> > > > > Yes, I don't see any issues with this patchset applied plus this DT
> > > > > patch on theadorable. The PCIe links are up and with the correct width.
> > > > > 
> > > > > What I'm wondering is, when exactly does the PCIe RP start the link
> > > > > establishment. In my case with AXP this is still in the AXP serdes code
> > > > > of course. But in the A38x code, it should be in the PCIe controller
> > > > > driver now AFAIU. I see that you configure the link width in the
> > > > > controller and do some other configuration (address windows etc), but
> > > > > at the end you "simply" wait for the link to come up via
> > > > > mvebu_pcie_wait_for_link(). I would have expected here some special
> > > > > command (config bit?) to the PCIe controller to start the link
> > > > > establishment. So when exactly does the A38x start this action?
> > > > 
> > > > That is interesting question... While I'm reading it again, I really do
> > > > not know. Because you are right that mvebu_pcie_wait_for_link() is just
> > > > waiting for a link and it "magically" comes up. I have tested it on A385
> > > > and it is stable with different Compex Atheros cards which caused issues
> > > > in past also on A3720.
> > > 
> > > I would prefer, to fully understand when exactly the link establishment
> > > is started. Since this is crucial for the setup of the controller that
> > > needs to be done *before* the link starts to come up.
> > 
> > I try to dig as more information as possible and finally I find out that
> > important information is available also in now removed, but originally
> > public A38x documentation. Thankfully web archive has copy of it:
> > 
> > https://web.archive.org/web/20200420191927/https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-embedded-processors-armada-38x-functional-specifications-2015-11.pdf
> > 
> >    17.3 Link Initialization
> > 
> >    In case the initialization fails and no link is established, the PHY
> >    will keep on trying to initiate a link forever unless the port is
> >    disabled. As long as the port is enabled, the PHY will continue trying
> >    to establish a link; once the PHY identifies that a device is
> >    connected to it, a link will be established.
> > 
> > PCIe port is enabled by bits in SoC Control 1 Register, which is done in
> > U-Boot SerDes initialization code. This is IIRC SoC specific, and reason
> > why every Armada SoC has own SerDes init code.
> > 
> > And looks like that due to "the PHY will keep on trying to initiate a
> > link forever", the PCIe link comes up when pci-mvebu.c sets all required
> > registers to correct values. E.g. set correct mode (RC vs endpoint),
> > link width (x1 vs x4), etc...
> > 
> > > Could you perhaps try to remove some of the register configurations in
> > > the MVEBU PCIe driver to see, if the link establishment relies on this
> > > register to be written to (e.g. PCI_EXP_LNKCAP)?
> > 
> > First port on A385 is by default set to X4 width, other ports to X1
> > width. Without updating LNKCAP to correct width, card in first PCIe port
> > never initialize. And cards in other ports are initialized even before
> > pci-mvebu.c starts configuration.
> 
> So the PCIe ports are now trying to establish the links, even when the
> correct configuration is not yet done. This might work but sound far
> from perfect to me IMHO.

Yes, it looks like (based on behavior of the first port). And it is not
perfect, just another mess :-(

> > So seems that this matches above behavior. SerDes init code enabled all
> > PCIe ports. Ports which are using default configuration (second, third)
> > are immediately initialized and link is established. Port which requires
> > additional configuration (first port, for switching from X4 to X1) just
> > stay in that "keep on trying to initiate a link forever" state until
> > pci-mvebu starts and set PCI_EXP_LNKCAP register, after which PHY try X1
> > width and success. And seems that this is the reason why 100ms timeout
> > is needed... As at this stage when pci-mvebu.c switches X4 to X1, init
> > timeout as defined in PCIe spec (that 100ms) starts ticking. For other
> > ports it starts ticking when serdes init code enables ports.
> > 
> > 
> > I have looked into all PCIe registers which are present in functional
> > spec, but it looks like that there is no pci-mvebu register which can
> > turn of LTSSM and link training, like it is in other PCIe controllers.
> > It looks like that only SoC-specific port enable bits are there.
> > 
> > It is starting to be bigger mess as before... Any suggestion how to
> > continue with it?
> > 
> > We cannot (easily) move that code which flips PCIe bits in SoC Control 1
> > Register from SerDes init code to pci-mvebu.c as this is outside of
> > pci-mvebu.c address space and also it is different on every SoC.
> > pci-mvebu.c registers are same on all Marvell SoCs, starting from Orion
> > up to the A39x.
> 
> One idea would be, to use a "reset-controller" driver on the Armada
> platforms, that is capable to at least reset and release the PCIe ports.
> Via the SoC Control 1 reg on A38x and via the SoC Control Register
> on AXP.

In that specification is also written:

  Enable the PCI Express interface by setting the <PCIe0En>, <PCIe1En>,
  <PCIe2En>, or <PCIe3En> field in the SoC Control 1 Register (Table
  1888 p. 1395). This allows programming of link parameters before the
  start of link initialization. The highest common link width is
  established according to the following order: x4 to x1.

So I think the correct behavior should be:

1. pci-mvebu.c configures all controller registers to correct values
2. PCIe port is enabled via SoC-specific register
3. pci-mvebu.c waits for link up

I guess that reset-controller does not help, as core release this reset
prior starting driver initialization.

Anyway, this A385 SoC Control 1 Register is at address 0x18204 which is
part of following device defined in kernel DTS file:

			systemc: system-controller@18200 {
				compatible = "marvell,armada-380-system-controller",
					     "marvell,armada-370-xp-system-controller";
				reg = <0x18200 0x100>;
			};

Linux kernel has driver for this DTS device is file:
arch/arm/mach-mvebu/system-controller.c

U-Boot does not have any driver for this compatible string.

So PCIe port enable/disable should be in this driver. I can write simple
driver also for U-Boot which can control this register. But I really do
not know which interface should it use.

Has somebody else any idea?

> I just looked into some Linux PCIe DT bindings and found e.g. this in
> the mediatek spec:
> 
> Documentation/devicetree/bindings/pci/mediatek-pcie.txt
> ...
> Required properties for MT7623/MT2701:
> ...
> - resets: Must contain an entry for each entry in reset-names.
>   See ../reset/reset.txt for details.
> - reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the
>   number of root ports.
> ...
> 		resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>,
> 			 <&hifsys MT2701_HIFSYS_PCIE1_RST>,
> 			 <&hifsys MT2701_HIFSYS_PCIE2_RST>;
> 		reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2";
> 		phys = <&pcie0_phy PHY_TYPE_PCIE>, <&pcie1_phy PHY_TYPE_PCIE>,
> 		       <&pcie2_phy PHY_TYPE_PCIE>;
> 		phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2";
> 
> 
> And make sure in the serdes code keeps (or actively sets?) these PCIe
> ports into the reset state. The PCIe driver would then release the ports
> out of reset after their configuration.
> 
> Or is some other serdes code missing in between "get PCIe port out of
> reset" and the MVEBU PCIe driver taking over the control?
> 
> What do you think? I might be missing something here.
> 
> Thanks,
> Stefan

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Pali Rohár]

On Monday 29 November 2021 14:27:48 Pali Rohár wrote:
> On Monday 29 November 2021 13:30:45 Stefan Roese wrote:
> > Hi Pali,
> > 
> > On 11/29/21 12:47, Pali Rohár wrote:
> > > Hello!
> > > 
> > > On Monday 29 November 2021 10:22:58 Stefan Roese wrote:
> > > > On 11/29/21 10:06, Pali Rohár wrote:
> > > > 
> > > > <snip>
> > > > 
> > > > > > > After this DTS change, pci-mvebu.c will just replace value of current
> > > > > > > number of lanes (which is set to 4 by serdes code) to value from DTS,
> > > > > > > which is 4. Therefore there should be no change.
> > > > > > > 
> > > > > > > Could you test whole patch series with above DTS change if it works
> > > > > > > properly on Theadorable board?
> > > > > > 
> > > > > > Yes, I don't see any issues with this patchset applied plus this DT
> > > > > > patch on theadorable. The PCIe links are up and with the correct width.
> > > > > > 
> > > > > > What I'm wondering is, when exactly does the PCIe RP start the link
> > > > > > establishment. In my case with AXP this is still in the AXP serdes code
> > > > > > of course. But in the A38x code, it should be in the PCIe controller
> > > > > > driver now AFAIU. I see that you configure the link width in the
> > > > > > controller and do some other configuration (address windows etc), but
> > > > > > at the end you "simply" wait for the link to come up via
> > > > > > mvebu_pcie_wait_for_link(). I would have expected here some special
> > > > > > command (config bit?) to the PCIe controller to start the link
> > > > > > establishment. So when exactly does the A38x start this action?
> > > > > 
> > > > > That is interesting question... While I'm reading it again, I really do
> > > > > not know. Because you are right that mvebu_pcie_wait_for_link() is just
> > > > > waiting for a link and it "magically" comes up. I have tested it on A385
> > > > > and it is stable with different Compex Atheros cards which caused issues
> > > > > in past also on A3720.
> > > > 
> > > > I would prefer, to fully understand when exactly the link establishment
> > > > is started. Since this is crucial for the setup of the controller that
> > > > needs to be done *before* the link starts to come up.
> > > 
> > > I try to dig as more information as possible and finally I find out that
> > > important information is available also in now removed, but originally
> > > public A38x documentation. Thankfully web archive has copy of it:
> > > 
> > > https://web.archive.org/web/20200420191927/https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-embedded-processors-armada-38x-functional-specifications-2015-11.pdf
> > > 
> > >    17.3 Link Initialization
> > > 
> > >    In case the initialization fails and no link is established, the PHY
> > >    will keep on trying to initiate a link forever unless the port is
> > >    disabled. As long as the port is enabled, the PHY will continue trying
> > >    to establish a link; once the PHY identifies that a device is
> > >    connected to it, a link will be established.
> > > 
> > > PCIe port is enabled by bits in SoC Control 1 Register, which is done in
> > > U-Boot SerDes initialization code. This is IIRC SoC specific, and reason
> > > why every Armada SoC has own SerDes init code.
> > > 
> > > And looks like that due to "the PHY will keep on trying to initiate a
> > > link forever", the PCIe link comes up when pci-mvebu.c sets all required
> > > registers to correct values. E.g. set correct mode (RC vs endpoint),
> > > link width (x1 vs x4), etc...
> > > 
> > > > Could you perhaps try to remove some of the register configurations in
> > > > the MVEBU PCIe driver to see, if the link establishment relies on this
> > > > register to be written to (e.g. PCI_EXP_LNKCAP)?
> > > 
> > > First port on A385 is by default set to X4 width, other ports to X1
> > > width. Without updating LNKCAP to correct width, card in first PCIe port
> > > never initialize. And cards in other ports are initialized even before
> > > pci-mvebu.c starts configuration.
> > 
> > So the PCIe ports are now trying to establish the links, even when the
> > correct configuration is not yet done. This might work but sound far
> > from perfect to me IMHO.
> 
> Yes, it looks like (based on behavior of the first port). And it is not
> perfect, just another mess :-(
> 
> > > So seems that this matches above behavior. SerDes init code enabled all
> > > PCIe ports. Ports which are using default configuration (second, third)
> > > are immediately initialized and link is established. Port which requires
> > > additional configuration (first port, for switching from X4 to X1) just
> > > stay in that "keep on trying to initiate a link forever" state until
> > > pci-mvebu starts and set PCI_EXP_LNKCAP register, after which PHY try X1
> > > width and success. And seems that this is the reason why 100ms timeout
> > > is needed... As at this stage when pci-mvebu.c switches X4 to X1, init
> > > timeout as defined in PCIe spec (that 100ms) starts ticking. For other
> > > ports it starts ticking when serdes init code enables ports.
> > > 
> > > 
> > > I have looked into all PCIe registers which are present in functional
> > > spec, but it looks like that there is no pci-mvebu register which can
> > > turn of LTSSM and link training, like it is in other PCIe controllers.
> > > It looks like that only SoC-specific port enable bits are there.
> > > 
> > > It is starting to be bigger mess as before... Any suggestion how to
> > > continue with it?
> > > 
> > > We cannot (easily) move that code which flips PCIe bits in SoC Control 1
> > > Register from SerDes init code to pci-mvebu.c as this is outside of
> > > pci-mvebu.c address space and also it is different on every SoC.
> > > pci-mvebu.c registers are same on all Marvell SoCs, starting from Orion
> > > up to the A39x.
> > 
> > One idea would be, to use a "reset-controller" driver on the Armada
> > platforms, that is capable to at least reset and release the PCIe ports.
> > Via the SoC Control 1 reg on A38x and via the SoC Control Register
> > on AXP.
> 
> In that specification is also written:
> 
>   Enable the PCI Express interface by setting the <PCIe0En>, <PCIe1En>,
>   <PCIe2En>, or <PCIe3En> field in the SoC Control 1 Register (Table
>   1888 p. 1395). This allows programming of link parameters before the
>   start of link initialization. The highest common link width is
>   established according to the following order: x4 to x1.
> 
> So I think the correct behavior should be:
> 
> 1. pci-mvebu.c configures all controller registers to correct values
> 2. PCIe port is enabled via SoC-specific register
> 3. pci-mvebu.c waits for link up
> 
> I guess that reset-controller does not help, as core release this reset
> prior starting driver initialization.

Ok, it looks like that reset controller API allows to do this. It would
mean to define that "system-controller@18200" as reset controller,
exports from it for each PCIe port reset functionality and implements
assert and deassert functions which disable and enable port.

And because DTS for pci-mvebu.c driver is defined as multi-root-port,
"resets" property would need to be defined for each port separately.

Just I'm not sure if this "enable port functionality" should be
implemented via Reset Controller API...

> Anyway, this A385 SoC Control 1 Register is at address 0x18204 which is
> part of following device defined in kernel DTS file:
> 
> 			systemc: system-controller@18200 {
> 				compatible = "marvell,armada-380-system-controller",
> 					     "marvell,armada-370-xp-system-controller";
> 				reg = <0x18200 0x100>;
> 			};
> 
> Linux kernel has driver for this DTS device is file:
> arch/arm/mach-mvebu/system-controller.c
> 
> U-Boot does not have any driver for this compatible string.
> 
> So PCIe port enable/disable should be in this driver. I can write simple
> driver also for U-Boot which can control this register. But I really do
> not know which interface should it use.
> 
> Has somebody else any idea?
> 
> > I just looked into some Linux PCIe DT bindings and found e.g. this in
> > the mediatek spec:
> > 
> > Documentation/devicetree/bindings/pci/mediatek-pcie.txt
> > ...
> > Required properties for MT7623/MT2701:
> > ...
> > - resets: Must contain an entry for each entry in reset-names.
> >   See ../reset/reset.txt for details.
> > - reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the
> >   number of root ports.
> > ...
> > 		resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>,
> > 			 <&hifsys MT2701_HIFSYS_PCIE1_RST>,
> > 			 <&hifsys MT2701_HIFSYS_PCIE2_RST>;
> > 		reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2";
> > 		phys = <&pcie0_phy PHY_TYPE_PCIE>, <&pcie1_phy PHY_TYPE_PCIE>,
> > 		       <&pcie2_phy PHY_TYPE_PCIE>;
> > 		phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2";
> > 
> > 
> > And make sure in the serdes code keeps (or actively sets?) these PCIe
> > ports into the reset state. The PCIe driver would then release the ports
> > out of reset after their configuration.
> > 
> > Or is some other serdes code missing in between "get PCIe port out of
> > reset" and the MVEBU PCIe driver taking over the control?
> > 
> > What do you think? I might be missing something here.
> > 
> > Thanks,
> > Stefan

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Stefan Roese]

On 11/29/21 15:28, Pali Rohár wrote:
> On Monday 29 November 2021 14:27:48 Pali Rohár wrote:
>> On Monday 29 November 2021 13:30:45 Stefan Roese wrote:
>>> Hi Pali,
>>>
>>> On 11/29/21 12:47, Pali Rohár wrote:
>>>> Hello!
>>>>
>>>> On Monday 29 November 2021 10:22:58 Stefan Roese wrote:
>>>>> On 11/29/21 10:06, Pali Rohár wrote:
>>>>>
>>>>> <snip>
>>>>>
>>>>>>>> After this DTS change, pci-mvebu.c will just replace value of current
>>>>>>>> number of lanes (which is set to 4 by serdes code) to value from DTS,
>>>>>>>> which is 4. Therefore there should be no change.
>>>>>>>>
>>>>>>>> Could you test whole patch series with above DTS change if it works
>>>>>>>> properly on Theadorable board?
>>>>>>>
>>>>>>> Yes, I don't see any issues with this patchset applied plus this DT
>>>>>>> patch on theadorable. The PCIe links are up and with the correct width.
>>>>>>>
>>>>>>> What I'm wondering is, when exactly does the PCIe RP start the link
>>>>>>> establishment. In my case with AXP this is still in the AXP serdes code
>>>>>>> of course. But in the A38x code, it should be in the PCIe controller
>>>>>>> driver now AFAIU. I see that you configure the link width in the
>>>>>>> controller and do some other configuration (address windows etc), but
>>>>>>> at the end you "simply" wait for the link to come up via
>>>>>>> mvebu_pcie_wait_for_link(). I would have expected here some special
>>>>>>> command (config bit?) to the PCIe controller to start the link
>>>>>>> establishment. So when exactly does the A38x start this action?
>>>>>>
>>>>>> That is interesting question... While I'm reading it again, I really do
>>>>>> not know. Because you are right that mvebu_pcie_wait_for_link() is just
>>>>>> waiting for a link and it "magically" comes up. I have tested it on A385
>>>>>> and it is stable with different Compex Atheros cards which caused issues
>>>>>> in past also on A3720.
>>>>>
>>>>> I would prefer, to fully understand when exactly the link establishment
>>>>> is started. Since this is crucial for the setup of the controller that
>>>>> needs to be done *before* the link starts to come up.
>>>>
>>>> I try to dig as more information as possible and finally I find out that
>>>> important information is available also in now removed, but originally
>>>> public A38x documentation. Thankfully web archive has copy of it:
>>>>
>>>> https://web.archive.org/web/20200420191927/https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-embedded-processors-armada-38x-functional-specifications-2015-11.pdf
>>>>
>>>>     17.3 Link Initialization
>>>>
>>>>     In case the initialization fails and no link is established, the PHY
>>>>     will keep on trying to initiate a link forever unless the port is
>>>>     disabled. As long as the port is enabled, the PHY will continue trying
>>>>     to establish a link; once the PHY identifies that a device is
>>>>     connected to it, a link will be established.
>>>>
>>>> PCIe port is enabled by bits in SoC Control 1 Register, which is done in
>>>> U-Boot SerDes initialization code. This is IIRC SoC specific, and reason
>>>> why every Armada SoC has own SerDes init code.
>>>>
>>>> And looks like that due to "the PHY will keep on trying to initiate a
>>>> link forever", the PCIe link comes up when pci-mvebu.c sets all required
>>>> registers to correct values. E.g. set correct mode (RC vs endpoint),
>>>> link width (x1 vs x4), etc...
>>>>
>>>>> Could you perhaps try to remove some of the register configurations in
>>>>> the MVEBU PCIe driver to see, if the link establishment relies on this
>>>>> register to be written to (e.g. PCI_EXP_LNKCAP)?
>>>>
>>>> First port on A385 is by default set to X4 width, other ports to X1
>>>> width. Without updating LNKCAP to correct width, card in first PCIe port
>>>> never initialize. And cards in other ports are initialized even before
>>>> pci-mvebu.c starts configuration.
>>>
>>> So the PCIe ports are now trying to establish the links, even when the
>>> correct configuration is not yet done. This might work but sound far
>>> from perfect to me IMHO.
>>
>> Yes, it looks like (based on behavior of the first port). And it is not
>> perfect, just another mess :-(
>>
>>>> So seems that this matches above behavior. SerDes init code enabled all
>>>> PCIe ports. Ports which are using default configuration (second, third)
>>>> are immediately initialized and link is established. Port which requires
>>>> additional configuration (first port, for switching from X4 to X1) just
>>>> stay in that "keep on trying to initiate a link forever" state until
>>>> pci-mvebu starts and set PCI_EXP_LNKCAP register, after which PHY try X1
>>>> width and success. And seems that this is the reason why 100ms timeout
>>>> is needed... As at this stage when pci-mvebu.c switches X4 to X1, init
>>>> timeout as defined in PCIe spec (that 100ms) starts ticking. For other
>>>> ports it starts ticking when serdes init code enables ports.
>>>>
>>>>
>>>> I have looked into all PCIe registers which are present in functional
>>>> spec, but it looks like that there is no pci-mvebu register which can
>>>> turn of LTSSM and link training, like it is in other PCIe controllers.
>>>> It looks like that only SoC-specific port enable bits are there.
>>>>
>>>> It is starting to be bigger mess as before... Any suggestion how to
>>>> continue with it?
>>>>
>>>> We cannot (easily) move that code which flips PCIe bits in SoC Control 1
>>>> Register from SerDes init code to pci-mvebu.c as this is outside of
>>>> pci-mvebu.c address space and also it is different on every SoC.
>>>> pci-mvebu.c registers are same on all Marvell SoCs, starting from Orion
>>>> up to the A39x.
>>>
>>> One idea would be, to use a "reset-controller" driver on the Armada
>>> platforms, that is capable to at least reset and release the PCIe ports.
>>> Via the SoC Control 1 reg on A38x and via the SoC Control Register
>>> on AXP.
>>
>> In that specification is also written:
>>
>>    Enable the PCI Express interface by setting the <PCIe0En>, <PCIe1En>,
>>    <PCIe2En>, or <PCIe3En> field in the SoC Control 1 Register (Table
>>    1888 p. 1395). This allows programming of link parameters before the
>>    start of link initialization. The highest common link width is
>>    established according to the following order: x4 to x1.
>>
>> So I think the correct behavior should be:
>>
>> 1. pci-mvebu.c configures all controller registers to correct values
>> 2. PCIe port is enabled via SoC-specific register
>> 3. pci-mvebu.c waits for link up
>>
>> I guess that reset-controller does not help, as core release this reset
>> prior starting driver initialization.
> 
> Ok, it looks like that reset controller API allows to do this. It would
> mean to define that "system-controller@18200" as reset controller,
> exports from it for each PCIe port reset functionality and implements
> assert and deassert functions which disable and enable port.
> 
> And because DTS for pci-mvebu.c driver is defined as multi-root-port,
> "resets" property would need to be defined for each port separately.

Okay. Sounds like a plan to me.

> Just I'm not sure if this "enable port functionality" should be
> implemented via Reset Controller API...

How else should / could this be done then? Do you have alterative ideas?

Thanks,
Stefan

>> Anyway, this A385 SoC Control 1 Register is at address 0x18204 which is
>> part of following device defined in kernel DTS file:
>>
>> 			systemc: system-controller@18200 {
>> 				compatible = "marvell,armada-380-system-controller",
>> 					     "marvell,armada-370-xp-system-controller";
>> 				reg = <0x18200 0x100>;
>> 			};
>>
>> Linux kernel has driver for this DTS device is file:
>> arch/arm/mach-mvebu/system-controller.c
>>
>> U-Boot does not have any driver for this compatible string.
>>
>> So PCIe port enable/disable should be in this driver. I can write simple
>> driver also for U-Boot which can control this register. But I really do
>> not know which interface should it use.
>>
>> Has somebody else any idea?
>>
>>> I just looked into some Linux PCIe DT bindings and found e.g. this in
>>> the mediatek spec:
>>>
>>> Documentation/devicetree/bindings/pci/mediatek-pcie.txt
>>> ...
>>> Required properties for MT7623/MT2701:
>>> ...
>>> - resets: Must contain an entry for each entry in reset-names.
>>>    See ../reset/reset.txt for details.
>>> - reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the
>>>    number of root ports.
>>> ...
>>> 		resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>,
>>> 			 <&hifsys MT2701_HIFSYS_PCIE1_RST>,
>>> 			 <&hifsys MT2701_HIFSYS_PCIE2_RST>;
>>> 		reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2";
>>> 		phys = <&pcie0_phy PHY_TYPE_PCIE>, <&pcie1_phy PHY_TYPE_PCIE>,
>>> 		       <&pcie2_phy PHY_TYPE_PCIE>;
>>> 		phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2";
>>>
>>>
>>> And make sure in the serdes code keeps (or actively sets?) these PCIe
>>> ports into the reset state. The PCIe driver would then release the ports
>>> out of reset after their configuration.
>>>
>>> Or is some other serdes code missing in between "get PCIe port out of
>>> reset" and the MVEBU PCIe driver taking over the control?
>>>
>>> What do you think? I might be missing something here.
>>>
>>> Thanks,
>>> Stefan

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Marek Behún]

On Mon, 29 Nov 2021 17:07:54 +0100
Stefan Roese <sr@denx.de> wrote:

> > Just I'm not sure if this "enable port functionality" should be
> > implemented via Reset Controller API...  
> 
> How else should / could this be done then? Do you have alterative ideas?

syscon regmap

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Pali Rohár]

On Monday 29 November 2021 18:09:22 Marek Behún wrote:
> On Mon, 29 Nov 2021 17:07:54 +0100
> Stefan Roese <sr@denx.de> wrote:
> 
> > > Just I'm not sure if this "enable port functionality" should be
> > > implemented via Reset Controller API...  
> > 
> > How else should / could this be done then? Do you have alterative ideas?
> 
> syscon regmap

regmap does not differ from classic usage of MVEBU_REG() macro and
moreover every SoC have its own way how to enable / disable PCIe port.

So regmap is not usable for this functionality.

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Pali Rohár]

On Monday 29 November 2021 17:07:54 Stefan Roese wrote:
> On 11/29/21 15:28, Pali Rohár wrote:
> > On Monday 29 November 2021 14:27:48 Pali Rohár wrote:
> > > On Monday 29 November 2021 13:30:45 Stefan Roese wrote:
> > > > Hi Pali,
> > > > 
> > > > On 11/29/21 12:47, Pali Rohár wrote:
> > > > > Hello!
> > > > > 
> > > > > On Monday 29 November 2021 10:22:58 Stefan Roese wrote:
> > > > > > On 11/29/21 10:06, Pali Rohár wrote:
> > > > > > 
> > > > > > <snip>
> > > > > > 
> > > > > > > > > After this DTS change, pci-mvebu.c will just replace value of current
> > > > > > > > > number of lanes (which is set to 4 by serdes code) to value from DTS,
> > > > > > > > > which is 4. Therefore there should be no change.
> > > > > > > > > 
> > > > > > > > > Could you test whole patch series with above DTS change if it works
> > > > > > > > > properly on Theadorable board?
> > > > > > > > 
> > > > > > > > Yes, I don't see any issues with this patchset applied plus this DT
> > > > > > > > patch on theadorable. The PCIe links are up and with the correct width.
> > > > > > > > 
> > > > > > > > What I'm wondering is, when exactly does the PCIe RP start the link
> > > > > > > > establishment. In my case with AXP this is still in the AXP serdes code
> > > > > > > > of course. But in the A38x code, it should be in the PCIe controller
> > > > > > > > driver now AFAIU. I see that you configure the link width in the
> > > > > > > > controller and do some other configuration (address windows etc), but
> > > > > > > > at the end you "simply" wait for the link to come up via
> > > > > > > > mvebu_pcie_wait_for_link(). I would have expected here some special
> > > > > > > > command (config bit?) to the PCIe controller to start the link
> > > > > > > > establishment. So when exactly does the A38x start this action?
> > > > > > > 
> > > > > > > That is interesting question... While I'm reading it again, I really do
> > > > > > > not know. Because you are right that mvebu_pcie_wait_for_link() is just
> > > > > > > waiting for a link and it "magically" comes up. I have tested it on A385
> > > > > > > and it is stable with different Compex Atheros cards which caused issues
> > > > > > > in past also on A3720.
> > > > > > 
> > > > > > I would prefer, to fully understand when exactly the link establishment
> > > > > > is started. Since this is crucial for the setup of the controller that
> > > > > > needs to be done *before* the link starts to come up.
> > > > > 
> > > > > I try to dig as more information as possible and finally I find out that
> > > > > important information is available also in now removed, but originally
> > > > > public A38x documentation. Thankfully web archive has copy of it:
> > > > > 
> > > > > https://web.archive.org/web/20200420191927/https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-embedded-processors-armada-38x-functional-specifications-2015-11.pdf
> > > > > 
> > > > >     17.3 Link Initialization
> > > > > 
> > > > >     In case the initialization fails and no link is established, the PHY
> > > > >     will keep on trying to initiate a link forever unless the port is
> > > > >     disabled. As long as the port is enabled, the PHY will continue trying
> > > > >     to establish a link; once the PHY identifies that a device is
> > > > >     connected to it, a link will be established.
> > > > > 
> > > > > PCIe port is enabled by bits in SoC Control 1 Register, which is done in
> > > > > U-Boot SerDes initialization code. This is IIRC SoC specific, and reason
> > > > > why every Armada SoC has own SerDes init code.
> > > > > 
> > > > > And looks like that due to "the PHY will keep on trying to initiate a
> > > > > link forever", the PCIe link comes up when pci-mvebu.c sets all required
> > > > > registers to correct values. E.g. set correct mode (RC vs endpoint),
> > > > > link width (x1 vs x4), etc...
> > > > > 
> > > > > > Could you perhaps try to remove some of the register configurations in
> > > > > > the MVEBU PCIe driver to see, if the link establishment relies on this
> > > > > > register to be written to (e.g. PCI_EXP_LNKCAP)?
> > > > > 
> > > > > First port on A385 is by default set to X4 width, other ports to X1
> > > > > width. Without updating LNKCAP to correct width, card in first PCIe port
> > > > > never initialize. And cards in other ports are initialized even before
> > > > > pci-mvebu.c starts configuration.
> > > > 
> > > > So the PCIe ports are now trying to establish the links, even when the
> > > > correct configuration is not yet done. This might work but sound far
> > > > from perfect to me IMHO.
> > > 
> > > Yes, it looks like (based on behavior of the first port). And it is not
> > > perfect, just another mess :-(
> > > 
> > > > > So seems that this matches above behavior. SerDes init code enabled all
> > > > > PCIe ports. Ports which are using default configuration (second, third)
> > > > > are immediately initialized and link is established. Port which requires
> > > > > additional configuration (first port, for switching from X4 to X1) just
> > > > > stay in that "keep on trying to initiate a link forever" state until
> > > > > pci-mvebu starts and set PCI_EXP_LNKCAP register, after which PHY try X1
> > > > > width and success. And seems that this is the reason why 100ms timeout
> > > > > is needed... As at this stage when pci-mvebu.c switches X4 to X1, init
> > > > > timeout as defined in PCIe spec (that 100ms) starts ticking. For other
> > > > > ports it starts ticking when serdes init code enables ports.
> > > > > 
> > > > > 
> > > > > I have looked into all PCIe registers which are present in functional
> > > > > spec, but it looks like that there is no pci-mvebu register which can
> > > > > turn of LTSSM and link training, like it is in other PCIe controllers.
> > > > > It looks like that only SoC-specific port enable bits are there.
> > > > > 
> > > > > It is starting to be bigger mess as before... Any suggestion how to
> > > > > continue with it?
> > > > > 
> > > > > We cannot (easily) move that code which flips PCIe bits in SoC Control 1
> > > > > Register from SerDes init code to pci-mvebu.c as this is outside of
> > > > > pci-mvebu.c address space and also it is different on every SoC.
> > > > > pci-mvebu.c registers are same on all Marvell SoCs, starting from Orion
> > > > > up to the A39x.
> > > > 
> > > > One idea would be, to use a "reset-controller" driver on the Armada
> > > > platforms, that is capable to at least reset and release the PCIe ports.
> > > > Via the SoC Control 1 reg on A38x and via the SoC Control Register
> > > > on AXP.
> > > 
> > > In that specification is also written:
> > > 
> > >    Enable the PCI Express interface by setting the <PCIe0En>, <PCIe1En>,
> > >    <PCIe2En>, or <PCIe3En> field in the SoC Control 1 Register (Table
> > >    1888 p. 1395). This allows programming of link parameters before the
> > >    start of link initialization. The highest common link width is
> > >    established according to the following order: x4 to x1.
> > > 
> > > So I think the correct behavior should be:
> > > 
> > > 1. pci-mvebu.c configures all controller registers to correct values
> > > 2. PCIe port is enabled via SoC-specific register
> > > 3. pci-mvebu.c waits for link up
> > > 
> > > I guess that reset-controller does not help, as core release this reset
> > > prior starting driver initialization.
> > 
> > Ok, it looks like that reset controller API allows to do this. It would
> > mean to define that "system-controller@18200" as reset controller,
> > exports from it for each PCIe port reset functionality and implements
> > assert and deassert functions which disable and enable port.
> > 
> > And because DTS for pci-mvebu.c driver is defined as multi-root-port,
> > "resets" property would need to be defined for each port separately.
> 
> Okay. Sounds like a plan to me.

I'm looking at this right now and it is even more complicated. For
example Armada XP has for some ports dedicated enable bits and for some
ports has just one shared enable bit.

And this HW design does not fit into current U-Boot PCI DM model where
for each PCIe port there is dedicated mvebu_pcie_probe() call which
should setup PCIe port, enable PCIe port and returns once PCIe port is
ready. In U-Boot PCI DM model is every PCIe port as separate DM device.

Any idea how to solve this issue?

> > Just I'm not sure if this "enable port functionality" should be
> > implemented via Reset Controller API...
> 
> How else should / could this be done then? Do you have alterative ideas?
> 
> Thanks,
> Stefan
> 
> > > Anyway, this A385 SoC Control 1 Register is at address 0x18204 which is
> > > part of following device defined in kernel DTS file:
> > > 
> > > 			systemc: system-controller@18200 {
> > > 				compatible = "marvell,armada-380-system-controller",
> > > 					     "marvell,armada-370-xp-system-controller";
> > > 				reg = <0x18200 0x100>;
> > > 			};
> > > 
> > > Linux kernel has driver for this DTS device is file:
> > > arch/arm/mach-mvebu/system-controller.c
> > > 
> > > U-Boot does not have any driver for this compatible string.
> > > 
> > > So PCIe port enable/disable should be in this driver. I can write simple
> > > driver also for U-Boot which can control this register. But I really do
> > > not know which interface should it use.
> > > 
> > > Has somebody else any idea?
> > > 
> > > > I just looked into some Linux PCIe DT bindings and found e.g. this in
> > > > the mediatek spec:
> > > > 
> > > > Documentation/devicetree/bindings/pci/mediatek-pcie.txt
> > > > ...
> > > > Required properties for MT7623/MT2701:
> > > > ...
> > > > - resets: Must contain an entry for each entry in reset-names.
> > > >    See ../reset/reset.txt for details.
> > > > - reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the
> > > >    number of root ports.
> > > > ...
> > > > 		resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>,
> > > > 			 <&hifsys MT2701_HIFSYS_PCIE1_RST>,
> > > > 			 <&hifsys MT2701_HIFSYS_PCIE2_RST>;
> > > > 		reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2";
> > > > 		phys = <&pcie0_phy PHY_TYPE_PCIE>, <&pcie1_phy PHY_TYPE_PCIE>,
> > > > 		       <&pcie2_phy PHY_TYPE_PCIE>;
> > > > 		phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2";
> > > > 
> > > > 
> > > > And make sure in the serdes code keeps (or actively sets?) these PCIe
> > > > ports into the reset state. The PCIe driver would then release the ports
> > > > out of reset after their configuration.
> > > > 
> > > > Or is some other serdes code missing in between "get PCIe port out of
> > > > reset" and the MVEBU PCIe driver taking over the control?
> > > > 
> > > > What do you think? I might be missing something here.
> > > > 
> > > > Thanks,
> > > > Stefan

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Stefan Roese]

Hi Pali,

On 12/10/21 15:23, Pali Rohár wrote:

<snip>

>>>> So I think the correct behavior should be:
>>>>
>>>> 1. pci-mvebu.c configures all controller registers to correct values
>>>> 2. PCIe port is enabled via SoC-specific register
>>>> 3. pci-mvebu.c waits for link up
>>>>
>>>> I guess that reset-controller does not help, as core release this reset
>>>> prior starting driver initialization.
>>>
>>> Ok, it looks like that reset controller API allows to do this. It would
>>> mean to define that "system-controller@18200" as reset controller,
>>> exports from it for each PCIe port reset functionality and implements
>>> assert and deassert functions which disable and enable port.
>>>
>>> And because DTS for pci-mvebu.c driver is defined as multi-root-port,
>>> "resets" property would need to be defined for each port separately.
>>
>> Okay. Sounds like a plan to me.
> 
> I'm looking at this right now and it is even more complicated. For
> example Armada XP has for some ports dedicated enable bits and for some
> ports has just one shared enable bit.
> 
> And this HW design does not fit into current U-Boot PCI DM model where
> for each PCIe port there is dedicated mvebu_pcie_probe() call which
> should setup PCIe port, enable PCIe port and returns once PCIe port is
> ready. In U-Boot PCI DM model is every PCIe port as separate DM device.
> 
> Any idea how to solve this issue?

Sorry, but I don't have clear "solution" for this in my mind right now.

Thanks,
Stefan

>>> Just I'm not sure if this "enable port functionality" should be
>>> implemented via Reset Controller API...
>>
>> How else should / could this be done then? Do you have alterative ideas?
>>
>> Thanks,
>> Stefan
>>
>>>> Anyway, this A385 SoC Control 1 Register is at address 0x18204 which is
>>>> part of following device defined in kernel DTS file:
>>>>
>>>> 			systemc: system-controller@18200 {
>>>> 				compatible = "marvell,armada-380-system-controller",
>>>> 					     "marvell,armada-370-xp-system-controller";
>>>> 				reg = <0x18200 0x100>;
>>>> 			};
>>>>
>>>> Linux kernel has driver for this DTS device is file:
>>>> arch/arm/mach-mvebu/system-controller.c
>>>>
>>>> U-Boot does not have any driver for this compatible string.
>>>>
>>>> So PCIe port enable/disable should be in this driver. I can write simple
>>>> driver also for U-Boot which can control this register. But I really do
>>>> not know which interface should it use.
>>>>
>>>> Has somebody else any idea?
>>>>
>>>>> I just looked into some Linux PCIe DT bindings and found e.g. this in
>>>>> the mediatek spec:
>>>>>
>>>>> Documentation/devicetree/bindings/pci/mediatek-pcie.txt
>>>>> ...
>>>>> Required properties for MT7623/MT2701:
>>>>> ...
>>>>> - resets: Must contain an entry for each entry in reset-names.
>>>>>     See ../reset/reset.txt for details.
>>>>> - reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the
>>>>>     number of root ports.
>>>>> ...
>>>>> 		resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>,
>>>>> 			 <&hifsys MT2701_HIFSYS_PCIE1_RST>,
>>>>> 			 <&hifsys MT2701_HIFSYS_PCIE2_RST>;
>>>>> 		reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2";
>>>>> 		phys = <&pcie0_phy PHY_TYPE_PCIE>, <&pcie1_phy PHY_TYPE_PCIE>,
>>>>> 		       <&pcie2_phy PHY_TYPE_PCIE>;
>>>>> 		phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2";
>>>>>
>>>>>
>>>>> And make sure in the serdes code keeps (or actively sets?) these PCIe
>>>>> ports into the reset state. The PCIe driver would then release the ports
>>>>> out of reset after their configuration.
>>>>>
>>>>> Or is some other serdes code missing in between "get PCIe port out of
>>>>> reset" and the MVEBU PCIe driver taking over the control?
>>>>>
>>>>> What do you think? I might be missing something here.
>>>>>
>>>>> Thanks,
>>>>> Stefan

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Pali Rohár]

On Monday 13 December 2021 08:36:00 Stefan Roese wrote:
> Hi Pali,
> 
> On 12/10/21 15:23, Pali Rohár wrote:
> 
> <snip>
> 
> > > > > So I think the correct behavior should be:
> > > > > 
> > > > > 1. pci-mvebu.c configures all controller registers to correct values
> > > > > 2. PCIe port is enabled via SoC-specific register
> > > > > 3. pci-mvebu.c waits for link up
> > > > > 
> > > > > I guess that reset-controller does not help, as core release this reset
> > > > > prior starting driver initialization.
> > > > 
> > > > Ok, it looks like that reset controller API allows to do this. It would
> > > > mean to define that "system-controller@18200" as reset controller,
> > > > exports from it for each PCIe port reset functionality and implements
> > > > assert and deassert functions which disable and enable port.
> > > > 
> > > > And because DTS for pci-mvebu.c driver is defined as multi-root-port,
> > > > "resets" property would need to be defined for each port separately.
> > > 
> > > Okay. Sounds like a plan to me.
> > 
> > I'm looking at this right now and it is even more complicated. For
> > example Armada XP has for some ports dedicated enable bits and for some
> > ports has just one shared enable bit.
> > 
> > And this HW design does not fit into current U-Boot PCI DM model where
> > for each PCIe port there is dedicated mvebu_pcie_probe() call which
> > should setup PCIe port, enable PCIe port and returns once PCIe port is
> > ready. In U-Boot PCI DM model is every PCIe port as separate DM device.
> > 
> > Any idea how to solve this issue?
> 
> Sorry, but I don't have clear "solution" for this in my mind right now.

Ok, I will try to invent something...

> Thanks,
> Stefan
> 
> > > > Just I'm not sure if this "enable port functionality" should be
> > > > implemented via Reset Controller API...
> > > 
> > > How else should / could this be done then? Do you have alterative ideas?
> > > 
> > > Thanks,
> > > Stefan
> > > 
> > > > > Anyway, this A385 SoC Control 1 Register is at address 0x18204 which is
> > > > > part of following device defined in kernel DTS file:
> > > > > 
> > > > > 			systemc: system-controller@18200 {
> > > > > 				compatible = "marvell,armada-380-system-controller",
> > > > > 					     "marvell,armada-370-xp-system-controller";
> > > > > 				reg = <0x18200 0x100>;
> > > > > 			};
> > > > > 
> > > > > Linux kernel has driver for this DTS device is file:
> > > > > arch/arm/mach-mvebu/system-controller.c
> > > > > 
> > > > > U-Boot does not have any driver for this compatible string.
> > > > > 
> > > > > So PCIe port enable/disable should be in this driver. I can write simple
> > > > > driver also for U-Boot which can control this register. But I really do
> > > > > not know which interface should it use.
> > > > > 
> > > > > Has somebody else any idea?
> > > > > 
> > > > > > I just looked into some Linux PCIe DT bindings and found e.g. this in
> > > > > > the mediatek spec:
> > > > > > 
> > > > > > Documentation/devicetree/bindings/pci/mediatek-pcie.txt
> > > > > > ...
> > > > > > Required properties for MT7623/MT2701:
> > > > > > ...
> > > > > > - resets: Must contain an entry for each entry in reset-names.
> > > > > >     See ../reset/reset.txt for details.
> > > > > > - reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the
> > > > > >     number of root ports.
> > > > > > ...
> > > > > > 		resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>,
> > > > > > 			 <&hifsys MT2701_HIFSYS_PCIE1_RST>,
> > > > > > 			 <&hifsys MT2701_HIFSYS_PCIE2_RST>;
> > > > > > 		reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2";
> > > > > > 		phys = <&pcie0_phy PHY_TYPE_PCIE>, <&pcie1_phy PHY_TYPE_PCIE>,
> > > > > > 		       <&pcie2_phy PHY_TYPE_PCIE>;
> > > > > > 		phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2";
> > > > > > 
> > > > > > 
> > > > > > And make sure in the serdes code keeps (or actively sets?) these PCIe
> > > > > > ports into the reset state. The PCIe driver would then release the ports
> > > > > > out of reset after their configuration.
> > > > > > 
> > > > > > Or is some other serdes code missing in between "get PCIe port out of
> > > > > > reset" and the MVEBU PCIe driver taking over the control?
> > > > > > 
> > > > > > What do you think? I might be missing something here.
> > > > > > 
> > > > > > Thanks,
> > > > > > Stefan
> 
> Viele Grüße,
> Stefan Roese
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell 03/10] pci: pci_mvebu: Move setup for BAR[0] where other BARs are setup

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Function mvebu_pcie_setup_wins() sets up all other BARs, so move setup of
BAR[0] to this function to have common code at one place.

In the past, commit 193a1e9f196b ("pci: pci_mvebu: set BAR0 after memory
space is set") moved setup of BAR[0] to another location, due to ath10k
not working in kernel, but the reason why was unknown, but it seems to
work now, and we think the issue then was cause by the PCIe Root Port
presenting itself as a Memory Controller and therefore U-Boot's code
have overwritten the BAR. Since the driver now ignores any write
operations to PCIe Root Port BARs, this should not be an issue anymore.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/pci_mvebu.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
index 278dc2756f..97c7b5604f 100644
--- a/drivers/pci/pci_mvebu.c
+++ b/drivers/pci/pci_mvebu.c
@@ -335,7 +335,9 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf,
 
 /*
  * Setup PCIE BARs and Address Decode Wins:
- * BAR[0,2] -> disabled, BAR[1] -> covers all DRAM banks
+ * BAR[0] -> internal registers
+ * BAR[1] -> covers all DRAM banks
+ * BAR[2] -> disabled
  * WIN[0-3] -> DRAM bank[0-3]
  */
 static void mvebu_pcie_setup_wins(struct mvebu_pcie *pcie)
@@ -386,6 +388,10 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie *pcie)
 	writel(0, pcie->base + PCIE_BAR_HI_OFF(1));
 	writel(((size - 1) & 0xffff0000) | 0x1,
 	       pcie->base + PCIE_BAR_CTRL_OFF(1));
+
+	/* Setup BAR[0] to internal registers. */
+	writel(SOC_REGS_PHY_BASE, pcie->base + PCIE_BAR_LO_OFF(0));
+	writel(0, pcie->base + PCIE_BAR_HI_OFF(0));
 }
 
 static int mvebu_pcie_probe(struct udevice *dev)
@@ -501,10 +507,6 @@ static int mvebu_pcie_probe(struct udevice *dev)
 		       pcie->io.start, MBUS_PCI_IO_SIZE, PCI_REGION_IO);
 	hose->region_count = 3;
 
-	/* Set BAR0 to internal registers */
-	writel(SOC_REGS_PHY_BASE, pcie->base + PCIE_BAR_LO_OFF(0));
-	writel(0, pcie->base + PCIE_BAR_HI_OFF(0));
-
 	/* PCI Bridge support 32-bit I/O and 64-bit prefetch mem addressing */
 	pcie->cfgcache[(PCI_IO_BASE - 0x10) / 4] =
 		PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8);
-- 
2.32.0

Re: [PATCH u-boot-marvell 03/10] pci: pci_mvebu: Move setup for BAR[0] where other BARs are setup

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Function mvebu_pcie_setup_wins() sets up all other BARs, so move setup of
> BAR[0] to this function to have common code at one place.
> 
> In the past, commit 193a1e9f196b ("pci: pci_mvebu: set BAR0 after memory
> space is set") moved setup of BAR[0] to another location, due to ath10k
> not working in kernel, but the reason why was unknown, but it seems to
> work now, and we think the issue then was cause by the PCIe Root Port
> presenting itself as a Memory Controller and therefore U-Boot's code
> have overwritten the BAR. Since the driver now ignores any write
> operations to PCIe Root Port BARs, this should not be an issue anymore.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/pci/pci_mvebu.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index 278dc2756f..97c7b5604f 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -335,7 +335,9 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf,
>   
>   /*
>    * Setup PCIE BARs and Address Decode Wins:
> - * BAR[0,2] -> disabled, BAR[1] -> covers all DRAM banks
> + * BAR[0] -> internal registers
> + * BAR[1] -> covers all DRAM banks
> + * BAR[2] -> disabled
>    * WIN[0-3] -> DRAM bank[0-3]
>    */
>   static void mvebu_pcie_setup_wins(struct mvebu_pcie *pcie)
> @@ -386,6 +388,10 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie *pcie)
>   	writel(0, pcie->base + PCIE_BAR_HI_OFF(1));
>   	writel(((size - 1) & 0xffff0000) | 0x1,
>   	       pcie->base + PCIE_BAR_CTRL_OFF(1));
> +
> +	/* Setup BAR[0] to internal registers. */
> +	writel(SOC_REGS_PHY_BASE, pcie->base + PCIE_BAR_LO_OFF(0));
> +	writel(0, pcie->base + PCIE_BAR_HI_OFF(0));
>   }
>   
>   static int mvebu_pcie_probe(struct udevice *dev)
> @@ -501,10 +507,6 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   		       pcie->io.start, MBUS_PCI_IO_SIZE, PCI_REGION_IO);
>   	hose->region_count = 3;
>   
> -	/* Set BAR0 to internal registers */
> -	writel(SOC_REGS_PHY_BASE, pcie->base + PCIE_BAR_LO_OFF(0));
> -	writel(0, pcie->base + PCIE_BAR_HI_OFF(0));
> -
>   	/* PCI Bridge support 32-bit I/O and 64-bit prefetch mem addressing */
>   	pcie->cfgcache[(PCI_IO_BASE - 0x10) / 4] =
>   		PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8);
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 03/10] pci: pci_mvebu: Move setup for BAR[0] where other BARs are setup

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Function mvebu_pcie_setup_wins() sets up all other BARs, so move setup of
> BAR[0] to this function to have common code at one place.
> 
> In the past, commit 193a1e9f196b ("pci: pci_mvebu: set BAR0 after memory
> space is set") moved setup of BAR[0] to another location, due to ath10k
> not working in kernel, but the reason why was unknown, but it seems to
> work now, and we think the issue then was cause by the PCIe Root Port
> presenting itself as a Memory Controller and therefore U-Boot's code
> have overwritten the BAR. Since the driver now ignores any write
> operations to PCIe Root Port BARs, this should not be an issue anymore.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Applied to u-boot-marvell/next

Thanks,
Stefan

> ---
>   drivers/pci/pci_mvebu.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index 278dc2756f..97c7b5604f 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -335,7 +335,9 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf,
>   
>   /*
>    * Setup PCIE BARs and Address Decode Wins:
> - * BAR[0,2] -> disabled, BAR[1] -> covers all DRAM banks
> + * BAR[0] -> internal registers
> + * BAR[1] -> covers all DRAM banks
> + * BAR[2] -> disabled
>    * WIN[0-3] -> DRAM bank[0-3]
>    */
>   static void mvebu_pcie_setup_wins(struct mvebu_pcie *pcie)
> @@ -386,6 +388,10 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie *pcie)
>   	writel(0, pcie->base + PCIE_BAR_HI_OFF(1));
>   	writel(((size - 1) & 0xffff0000) | 0x1,
>   	       pcie->base + PCIE_BAR_CTRL_OFF(1));
> +
> +	/* Setup BAR[0] to internal registers. */
> +	writel(SOC_REGS_PHY_BASE, pcie->base + PCIE_BAR_LO_OFF(0));
> +	writel(0, pcie->base + PCIE_BAR_HI_OFF(0));
>   }
>   
>   static int mvebu_pcie_probe(struct udevice *dev)
> @@ -501,10 +507,6 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   		       pcie->io.start, MBUS_PCI_IO_SIZE, PCI_REGION_IO);
>   	hose->region_count = 3;
>   
> -	/* Set BAR0 to internal registers */
> -	writel(SOC_REGS_PHY_BASE, pcie->base + PCIE_BAR_LO_OFF(0));
> -	writel(0, pcie->base + PCIE_BAR_HI_OFF(0));
> -
>   	/* PCI Bridge support 32-bit I/O and 64-bit prefetch mem addressing */
>   	pcie->cfgcache[(PCI_IO_BASE - 0x10) / 4] =
>   		PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8);
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell 04/10] pci: pci_mvebu: Replace MBUS_PCI_*_SIZE by resource_size()

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Use more appropriate resource_size() function when working with data in
struct resource.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/pci_mvebu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
index 97c7b5604f..fde52ec99d 100644
--- a/drivers/pci/pci_mvebu.c
+++ b/drivers/pci/pci_mvebu.c
@@ -477,9 +477,9 @@ static int mvebu_pcie_probe(struct udevice *dev)
 
 	if (mvebu_mbus_add_window_by_id(pcie->mem_target, pcie->mem_attr,
 					(phys_addr_t)pcie->mem.start,
-					MBUS_PCI_MEM_SIZE)) {
+					resource_size(&pcie->mem))) {
 		printf("PCIe unable to add mbus window for mem at %08x+%08x\n",
-		       (u32)pcie->mem.start, MBUS_PCI_MEM_SIZE);
+		       (u32)pcie->mem.start, (unsigned)resource_size(&pcie->mem));
 	}
 
 	pcie->io.start = (u32)mvebu_pcie_iobase;
@@ -488,9 +488,9 @@ static int mvebu_pcie_probe(struct udevice *dev)
 
 	if (mvebu_mbus_add_window_by_id(pcie->io_target, pcie->io_attr,
 					(phys_addr_t)pcie->io.start,
-					MBUS_PCI_IO_SIZE)) {
+					resource_size(&pcie->io))) {
 		printf("PCIe unable to add mbus window for IO at %08x+%08x\n",
-		       (u32)pcie->io.start, MBUS_PCI_IO_SIZE);
+		       (u32)pcie->io.start, (unsigned)resource_size(&pcie->io));
 	}
 
 	/* Setup windows and configure host bridge */
@@ -498,13 +498,13 @@ static int mvebu_pcie_probe(struct udevice *dev)
 
 	/* PCI memory space */
 	pci_set_region(hose->regions + 0, pcie->mem.start,
-		       pcie->mem.start, MBUS_PCI_MEM_SIZE, PCI_REGION_MEM);
+		       pcie->mem.start, resource_size(&pcie->mem), PCI_REGION_MEM);
 	pci_set_region(hose->regions + 1,
 		       0, 0,
 		       gd->ram_size,
 		       PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
 	pci_set_region(hose->regions + 2, pcie->io.start,
-		       pcie->io.start, MBUS_PCI_IO_SIZE, PCI_REGION_IO);
+		       pcie->io.start, resource_size(&pcie->io), PCI_REGION_IO);
 	hose->region_count = 3;
 
 	/* PCI Bridge support 32-bit I/O and 64-bit prefetch mem addressing */
-- 
2.32.0

Re: [PATCH u-boot-marvell 04/10] pci: pci_mvebu: Replace MBUS_PCI_*_SIZE by resource_size()

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Use more appropriate resource_size() function when working with data in
> struct resource.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/pci/pci_mvebu.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index 97c7b5604f..fde52ec99d 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -477,9 +477,9 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   
>   	if (mvebu_mbus_add_window_by_id(pcie->mem_target, pcie->mem_attr,
>   					(phys_addr_t)pcie->mem.start,
> -					MBUS_PCI_MEM_SIZE)) {
> +					resource_size(&pcie->mem))) {
>   		printf("PCIe unable to add mbus window for mem at %08x+%08x\n",
> -		       (u32)pcie->mem.start, MBUS_PCI_MEM_SIZE);
> +		       (u32)pcie->mem.start, (unsigned)resource_size(&pcie->mem));
>   	}
>   
>   	pcie->io.start = (u32)mvebu_pcie_iobase;
> @@ -488,9 +488,9 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   
>   	if (mvebu_mbus_add_window_by_id(pcie->io_target, pcie->io_attr,
>   					(phys_addr_t)pcie->io.start,
> -					MBUS_PCI_IO_SIZE)) {
> +					resource_size(&pcie->io))) {
>   		printf("PCIe unable to add mbus window for IO at %08x+%08x\n",
> -		       (u32)pcie->io.start, MBUS_PCI_IO_SIZE);
> +		       (u32)pcie->io.start, (unsigned)resource_size(&pcie->io));
>   	}
>   
>   	/* Setup windows and configure host bridge */
> @@ -498,13 +498,13 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   
>   	/* PCI memory space */
>   	pci_set_region(hose->regions + 0, pcie->mem.start,
> -		       pcie->mem.start, MBUS_PCI_MEM_SIZE, PCI_REGION_MEM);
> +		       pcie->mem.start, resource_size(&pcie->mem), PCI_REGION_MEM);
>   	pci_set_region(hose->regions + 1,
>   		       0, 0,
>   		       gd->ram_size,
>   		       PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
>   	pci_set_region(hose->regions + 2, pcie->io.start,
> -		       pcie->io.start, MBUS_PCI_IO_SIZE, PCI_REGION_IO);
> +		       pcie->io.start, resource_size(&pcie->io), PCI_REGION_IO);
>   	hose->region_count = 3;
>   
>   	/* PCI Bridge support 32-bit I/O and 64-bit prefetch mem addressing */
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 04/10] pci: pci_mvebu: Replace MBUS_PCI_*_SIZE by resource_size()

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Use more appropriate resource_size() function when working with data in
> struct resource.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Applied to u-boot-marvell/next

Thanks,
Stefan

> ---
>   drivers/pci/pci_mvebu.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index 97c7b5604f..fde52ec99d 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -477,9 +477,9 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   
>   	if (mvebu_mbus_add_window_by_id(pcie->mem_target, pcie->mem_attr,
>   					(phys_addr_t)pcie->mem.start,
> -					MBUS_PCI_MEM_SIZE)) {
> +					resource_size(&pcie->mem))) {
>   		printf("PCIe unable to add mbus window for mem at %08x+%08x\n",
> -		       (u32)pcie->mem.start, MBUS_PCI_MEM_SIZE);
> +		       (u32)pcie->mem.start, (unsigned)resource_size(&pcie->mem));
>   	}
>   
>   	pcie->io.start = (u32)mvebu_pcie_iobase;
> @@ -488,9 +488,9 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   
>   	if (mvebu_mbus_add_window_by_id(pcie->io_target, pcie->io_attr,
>   					(phys_addr_t)pcie->io.start,
> -					MBUS_PCI_IO_SIZE)) {
> +					resource_size(&pcie->io))) {
>   		printf("PCIe unable to add mbus window for IO at %08x+%08x\n",
> -		       (u32)pcie->io.start, MBUS_PCI_IO_SIZE);
> +		       (u32)pcie->io.start, (unsigned)resource_size(&pcie->io));
>   	}
>   
>   	/* Setup windows and configure host bridge */
> @@ -498,13 +498,13 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   
>   	/* PCI memory space */
>   	pci_set_region(hose->regions + 0, pcie->mem.start,
> -		       pcie->mem.start, MBUS_PCI_MEM_SIZE, PCI_REGION_MEM);
> +		       pcie->mem.start, resource_size(&pcie->mem), PCI_REGION_MEM);
>   	pci_set_region(hose->regions + 1,
>   		       0, 0,
>   		       gd->ram_size,
>   		       PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
>   	pci_set_region(hose->regions + 2, pcie->io.start,
> -		       pcie->io.start, MBUS_PCI_IO_SIZE, PCI_REGION_IO);
> +		       pcie->io.start, resource_size(&pcie->io), PCI_REGION_IO);
>   	hose->region_count = 3;
>   
>   	/* PCI Bridge support 32-bit I/O and 64-bit prefetch mem addressing */
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell 05/10] pci: pci_mvebu, pci_aardvark: Fix size of configuration cache

[Marek Behún]

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

Since u32 takes up 4 bytes, we need to divide the number of u32s by 4
for cfgcache.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/pci-aardvark.c | 2 +-
 drivers/pci/pci_mvebu.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
index 4e94b776c5..8abbc3ffe8 100644
--- a/drivers/pci/pci-aardvark.c
+++ b/drivers/pci/pci-aardvark.c
@@ -202,7 +202,7 @@ struct pcie_advk {
 	int			sec_busno;
 	struct udevice		*dev;
 	struct gpio_desc	reset_gpio;
-	u32			cfgcache[0x34 - 0x10];
+	u32			cfgcache[(0x34 - 0x10) / 4];
 	bool			cfgcrssve;
 };
 
diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
index fde52ec99d..b545e62689 100644
--- a/drivers/pci/pci_mvebu.c
+++ b/drivers/pci/pci_mvebu.c
@@ -93,7 +93,7 @@ struct mvebu_pcie {
 	unsigned int mem_attr;
 	unsigned int io_target;
 	unsigned int io_attr;
-	u32 cfgcache[0x34 - 0x10];
+	u32 cfgcache[(0x34 - 0x10) / 4];
 };
 
 /*
-- 
2.32.0

Re: [PATCH u-boot-marvell 05/10] pci: pci_mvebu, pci_aardvark: Fix size of configuration cache

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Marek Behún <marek.behun@nic.cz>
> 
> Since u32 takes up 4 bytes, we need to divide the number of u32s by 4
> for cfgcache.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/pci/pci-aardvark.c | 2 +-
>   drivers/pci/pci_mvebu.c    | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
> index 4e94b776c5..8abbc3ffe8 100644
> --- a/drivers/pci/pci-aardvark.c
> +++ b/drivers/pci/pci-aardvark.c
> @@ -202,7 +202,7 @@ struct pcie_advk {
>   	int			sec_busno;
>   	struct udevice		*dev;
>   	struct gpio_desc	reset_gpio;
> -	u32			cfgcache[0x34 - 0x10];
> +	u32			cfgcache[(0x34 - 0x10) / 4];
>   	bool			cfgcrssve;
>   };
>   
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index fde52ec99d..b545e62689 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -93,7 +93,7 @@ struct mvebu_pcie {
>   	unsigned int mem_attr;
>   	unsigned int io_target;
>   	unsigned int io_attr;
> -	u32 cfgcache[0x34 - 0x10];
> +	u32 cfgcache[(0x34 - 0x10) / 4];
>   };
>   
>   /*
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 05/10] pci: pci_mvebu, pci_aardvark: Fix size of configuration cache

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Marek Behún <marek.behun@nic.cz>
> 
> Since u32 takes up 4 bytes, we need to divide the number of u32s by 4
> for cfgcache.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Applied to u-boot-marvell/master

Thanks,
Stefan

> ---
>   drivers/pci/pci-aardvark.c | 2 +-
>   drivers/pci/pci_mvebu.c    | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
> index 4e94b776c5..8abbc3ffe8 100644
> --- a/drivers/pci/pci-aardvark.c
> +++ b/drivers/pci/pci-aardvark.c
> @@ -202,7 +202,7 @@ struct pcie_advk {
>   	int			sec_busno;
>   	struct udevice		*dev;
>   	struct gpio_desc	reset_gpio;
> -	u32			cfgcache[0x34 - 0x10];
> +	u32			cfgcache[(0x34 - 0x10) / 4];
>   	bool			cfgcrssve;
>   };
>   
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index fde52ec99d..b545e62689 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -93,7 +93,7 @@ struct mvebu_pcie {
>   	unsigned int mem_attr;
>   	unsigned int io_target;
>   	unsigned int io_attr;
> -	u32 cfgcache[0x34 - 0x10];
> +	u32 cfgcache[(0x34 - 0x10) / 4];
>   };
>   
>   /*
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell 06/10] pci: pci_mvebu: Do not allow setting ROM BAR on PCI Bridge

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

The PCI Bridge which represents mvebu PCIe Root Port has Expansion ROM
Base Address register at offset 0x30 but its meaning is different that
of PCI's Expansion ROM BAR register, although the address format of
the register is the same.

In reality, this device does not have any configurable PCI BARs. So
ensure that write operation into BARs (including Expansion ROM BAR) is a
noop and registers always contain zero address which indicates that BARs
are unsupported.

Fixes: a7b61ab58d5d ("pci: pci_mvebu: Properly configure and use PCI Bridge (PCIe Root Port)")
Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/pci_mvebu.c | 55 +++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
index b545e62689..701a17dfb7 100644
--- a/drivers/pci/pci_mvebu.c
+++ b/drivers/pci/pci_mvebu.c
@@ -93,7 +93,7 @@ struct mvebu_pcie {
 	unsigned int mem_attr;
 	unsigned int io_target;
 	unsigned int io_attr;
-	u32 cfgcache[(0x34 - 0x10) / 4];
+	u32 cfgcache[(0x3c - 0x10) / 4];
 };
 
 /*
@@ -189,20 +189,20 @@ static int mvebu_pcie_read_config(const struct udevice *bus, pci_dev_t bdf,
 	}
 
 	/*
-	 * mvebu has different internal registers mapped into PCI config space
-	 * in range 0x10-0x34 for PCI bridge, so do not access PCI config space
-	 * for this range and instead read content from driver virtual cfgcache
+	 * The configuration space of the PCI Bridge on primary (first) bus is
+	 * of Type 0 but the BAR registers (including ROM BAR) don't have the
+	 * same meaning as in the PCIe specification. Therefore do not access
+	 * BAR registers and non-common registers (those which have different
+	 * meaning for Type 0 and Type 1 config space) of the PCI Bridge and
+	 * instead read their content from driver virtual cfgcache[].
 	 */
-	if (busno == pcie->first_busno && offset >= 0x10 && offset < 0x34) {
+	if (busno == pcie->first_busno && ((offset >= 0x10 && offset < 0x34) ||
+					   (offset >= 0x38 && offset < 0x3c))) {
 		data = pcie->cfgcache[(offset - 0x10) / 4];
 		debug("(addr,size,val)=(0x%04x, %d, 0x%08x) from cfgcache\n",
 		      offset, size, data);
 		*valuep = pci_conv_32_to_size(data, offset, size);
 		return 0;
-	} else if (busno == pcie->first_busno &&
-		   (offset & ~3) == PCI_ROM_ADDRESS1) {
-		/* mvebu has Expansion ROM Base Address (0x38) at offset 0x30 */
-		offset -= PCI_ROM_ADDRESS1 - PCIE_EXP_ROM_BAR_OFF;
 	}
 
 	/*
@@ -269,17 +269,21 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf,
 	}
 
 	/*
-	 * mvebu has different internal registers mapped into PCI config space
-	 * in range 0x10-0x34 for PCI bridge, so do not access PCI config space
-	 * for this range and instead write content to driver virtual cfgcache
+	 * As explained in mvebu_pcie_read_config(), PCI Bridge Type 1 specific
+	 * config registers are not available, so we write their content only
+	 * into driver virtual cfgcache[].
+	 * And as explained in mvebu_pcie_probe(), mvebu has its own specific
+	 * way for configuring primary and secondary bus numbers.
 	 */
-	if (busno == pcie->first_busno && offset >= 0x10 && offset < 0x34) {
+	if (busno == pcie->first_busno && ((offset >= 0x10 && offset < 0x34) ||
+					   (offset >= 0x38 && offset < 0x3c))) {
 		debug("Writing to cfgcache only\n");
 		data = pcie->cfgcache[(offset - 0x10) / 4];
 		data = pci_conv_size_to_32(data, value, offset, size);
 		/* mvebu PCI bridge does not have configurable bars */
 		if ((offset & ~3) == PCI_BASE_ADDRESS_0 ||
-		    (offset & ~3) == PCI_BASE_ADDRESS_1)
+		    (offset & ~3) == PCI_BASE_ADDRESS_1 ||
+		    (offset & ~3) == PCI_ROM_ADDRESS1)
 			data = 0x0;
 		pcie->cfgcache[(offset - 0x10) / 4] = data;
 		/* mvebu has its own way how to set PCI primary bus number */
@@ -297,10 +301,6 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf,
 			      pcie->sec_busno);
 		}
 		return 0;
-	} else if (busno == pcie->first_busno &&
-		   (offset & ~3) == PCI_ROM_ADDRESS1) {
-		/* mvebu has Expansion ROM Base Address (0x38) at offset 0x30 */
-		offset -= PCI_ROM_ADDRESS1 - PCIE_EXP_ROM_BAR_OFF;
 	}
 
 	/*
@@ -424,13 +424,20 @@ static int mvebu_pcie_probe(struct udevice *dev)
 	 * U-Boot cannot recognize as P2P Bridge.
 	 *
 	 * Note that this mvebu PCI Bridge does not have compliant Type 1
-	 * Configuration Space. Header Type is reported as Type 0 and in
-	 * range 0x10-0x34 it has aliased internal mvebu registers 0x10-0x34
-	 * (e.g. PCIE_BAR_LO_OFF) and register 0x38 is reserved.
+	 * Configuration Space. Header Type is reported as Type 0 and it
+	 * has format of Type 0 config space.
 	 *
-	 * Driver for this range redirects access to virtual cfgcache[] buffer
-	 * which avoids changing internal mvebu registers. And changes Header
-	 * Type response value to Type 1.
+	 * Moreover Type 0 BAR registers (ranges 0x10 - 0x28 and 0x30 - 0x34)
+	 * have the same format in Marvell's specification as in PCIe
+	 * specification, but their meaning is totally different and they do
+	 * different things: they are aliased into internal mvebu registers
+	 * (e.g. PCIE_BAR_LO_OFF) and these should not be changed or
+	 * reconfigured by pci device drivers.
+	 *
+	 * So our driver converts Type 0 config space to Type 1 and reports
+	 * Header Type as Type 1. Access to BAR registers and to non-existent
+	 * Type 1 registers is redirected to the virtual cfgcache[] buffer,
+	 * which avoids changing unrelated registers.
 	 */
 	reg = readl(pcie->base + PCIE_DEV_REV_OFF);
 	reg &= ~0xffffff00;
-- 
2.32.0

Re: [PATCH u-boot-marvell 06/10] pci: pci_mvebu: Do not allow setting ROM BAR on PCI Bridge

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> The PCI Bridge which represents mvebu PCIe Root Port has Expansion ROM
> Base Address register at offset 0x30 but its meaning is different that
> of PCI's Expansion ROM BAR register, although the address format of
> the register is the same.
> 
> In reality, this device does not have any configurable PCI BARs. So
> ensure that write operation into BARs (including Expansion ROM BAR) is a
> noop and registers always contain zero address which indicates that BARs
> are unsupported.
> 
> Fixes: a7b61ab58d5d ("pci: pci_mvebu: Properly configure and use PCI Bridge (PCIe Root Port)")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/pci/pci_mvebu.c | 55 +++++++++++++++++++++++------------------
>   1 file changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index b545e62689..701a17dfb7 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -93,7 +93,7 @@ struct mvebu_pcie {
>   	unsigned int mem_attr;
>   	unsigned int io_target;
>   	unsigned int io_attr;
> -	u32 cfgcache[(0x34 - 0x10) / 4];
> +	u32 cfgcache[(0x3c - 0x10) / 4];
>   };
>   
>   /*
> @@ -189,20 +189,20 @@ static int mvebu_pcie_read_config(const struct udevice *bus, pci_dev_t bdf,
>   	}
>   
>   	/*
> -	 * mvebu has different internal registers mapped into PCI config space
> -	 * in range 0x10-0x34 for PCI bridge, so do not access PCI config space
> -	 * for this range and instead read content from driver virtual cfgcache
> +	 * The configuration space of the PCI Bridge on primary (first) bus is
> +	 * of Type 0 but the BAR registers (including ROM BAR) don't have the
> +	 * same meaning as in the PCIe specification. Therefore do not access
> +	 * BAR registers and non-common registers (those which have different
> +	 * meaning for Type 0 and Type 1 config space) of the PCI Bridge and
> +	 * instead read their content from driver virtual cfgcache[].
>   	 */
> -	if (busno == pcie->first_busno && offset >= 0x10 && offset < 0x34) {
> +	if (busno == pcie->first_busno && ((offset >= 0x10 && offset < 0x34) ||
> +					   (offset >= 0x38 && offset < 0x3c))) {
>   		data = pcie->cfgcache[(offset - 0x10) / 4];
>   		debug("(addr,size,val)=(0x%04x, %d, 0x%08x) from cfgcache\n",
>   		      offset, size, data);
>   		*valuep = pci_conv_32_to_size(data, offset, size);
>   		return 0;
> -	} else if (busno == pcie->first_busno &&
> -		   (offset & ~3) == PCI_ROM_ADDRESS1) {
> -		/* mvebu has Expansion ROM Base Address (0x38) at offset 0x30 */
> -		offset -= PCI_ROM_ADDRESS1 - PCIE_EXP_ROM_BAR_OFF;
>   	}
>   
>   	/*
> @@ -269,17 +269,21 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf,
>   	}
>   
>   	/*
> -	 * mvebu has different internal registers mapped into PCI config space
> -	 * in range 0x10-0x34 for PCI bridge, so do not access PCI config space
> -	 * for this range and instead write content to driver virtual cfgcache
> +	 * As explained in mvebu_pcie_read_config(), PCI Bridge Type 1 specific
> +	 * config registers are not available, so we write their content only
> +	 * into driver virtual cfgcache[].
> +	 * And as explained in mvebu_pcie_probe(), mvebu has its own specific
> +	 * way for configuring primary and secondary bus numbers.
>   	 */
> -	if (busno == pcie->first_busno && offset >= 0x10 && offset < 0x34) {
> +	if (busno == pcie->first_busno && ((offset >= 0x10 && offset < 0x34) ||
> +					   (offset >= 0x38 && offset < 0x3c))) {
>   		debug("Writing to cfgcache only\n");
>   		data = pcie->cfgcache[(offset - 0x10) / 4];
>   		data = pci_conv_size_to_32(data, value, offset, size);
>   		/* mvebu PCI bridge does not have configurable bars */
>   		if ((offset & ~3) == PCI_BASE_ADDRESS_0 ||
> -		    (offset & ~3) == PCI_BASE_ADDRESS_1)
> +		    (offset & ~3) == PCI_BASE_ADDRESS_1 ||
> +		    (offset & ~3) == PCI_ROM_ADDRESS1)
>   			data = 0x0;
>   		pcie->cfgcache[(offset - 0x10) / 4] = data;
>   		/* mvebu has its own way how to set PCI primary bus number */
> @@ -297,10 +301,6 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf,
>   			      pcie->sec_busno);
>   		}
>   		return 0;
> -	} else if (busno == pcie->first_busno &&
> -		   (offset & ~3) == PCI_ROM_ADDRESS1) {
> -		/* mvebu has Expansion ROM Base Address (0x38) at offset 0x30 */
> -		offset -= PCI_ROM_ADDRESS1 - PCIE_EXP_ROM_BAR_OFF;
>   	}
>   
>   	/*
> @@ -424,13 +424,20 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   	 * U-Boot cannot recognize as P2P Bridge.
>   	 *
>   	 * Note that this mvebu PCI Bridge does not have compliant Type 1
> -	 * Configuration Space. Header Type is reported as Type 0 and in
> -	 * range 0x10-0x34 it has aliased internal mvebu registers 0x10-0x34
> -	 * (e.g. PCIE_BAR_LO_OFF) and register 0x38 is reserved.
> +	 * Configuration Space. Header Type is reported as Type 0 and it
> +	 * has format of Type 0 config space.
>   	 *
> -	 * Driver for this range redirects access to virtual cfgcache[] buffer
> -	 * which avoids changing internal mvebu registers. And changes Header
> -	 * Type response value to Type 1.
> +	 * Moreover Type 0 BAR registers (ranges 0x10 - 0x28 and 0x30 - 0x34)
> +	 * have the same format in Marvell's specification as in PCIe
> +	 * specification, but their meaning is totally different and they do
> +	 * different things: they are aliased into internal mvebu registers
> +	 * (e.g. PCIE_BAR_LO_OFF) and these should not be changed or
> +	 * reconfigured by pci device drivers.
> +	 *
> +	 * So our driver converts Type 0 config space to Type 1 and reports
> +	 * Header Type as Type 1. Access to BAR registers and to non-existent
> +	 * Type 1 registers is redirected to the virtual cfgcache[] buffer,
> +	 * which avoids changing unrelated registers.
>   	 */
>   	reg = readl(pcie->base + PCIE_DEV_REV_OFF);
>   	reg &= ~0xffffff00;
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 06/10] pci: pci_mvebu: Do not allow setting ROM BAR on PCI Bridge

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> The PCI Bridge which represents mvebu PCIe Root Port has Expansion ROM
> Base Address register at offset 0x30 but its meaning is different that
> of PCI's Expansion ROM BAR register, although the address format of
> the register is the same.
> 
> In reality, this device does not have any configurable PCI BARs. So
> ensure that write operation into BARs (including Expansion ROM BAR) is a
> noop and registers always contain zero address which indicates that BARs
> are unsupported.
> 
> Fixes: a7b61ab58d5d ("pci: pci_mvebu: Properly configure and use PCI Bridge (PCIe Root Port)")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Applied to u-boot-marvell/master

Thanks,
Stefan

> ---
>   drivers/pci/pci_mvebu.c | 55 +++++++++++++++++++++++------------------
>   1 file changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index b545e62689..701a17dfb7 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -93,7 +93,7 @@ struct mvebu_pcie {
>   	unsigned int mem_attr;
>   	unsigned int io_target;
>   	unsigned int io_attr;
> -	u32 cfgcache[(0x34 - 0x10) / 4];
> +	u32 cfgcache[(0x3c - 0x10) / 4];
>   };
>   
>   /*
> @@ -189,20 +189,20 @@ static int mvebu_pcie_read_config(const struct udevice *bus, pci_dev_t bdf,
>   	}
>   
>   	/*
> -	 * mvebu has different internal registers mapped into PCI config space
> -	 * in range 0x10-0x34 for PCI bridge, so do not access PCI config space
> -	 * for this range and instead read content from driver virtual cfgcache
> +	 * The configuration space of the PCI Bridge on primary (first) bus is
> +	 * of Type 0 but the BAR registers (including ROM BAR) don't have the
> +	 * same meaning as in the PCIe specification. Therefore do not access
> +	 * BAR registers and non-common registers (those which have different
> +	 * meaning for Type 0 and Type 1 config space) of the PCI Bridge and
> +	 * instead read their content from driver virtual cfgcache[].
>   	 */
> -	if (busno == pcie->first_busno && offset >= 0x10 && offset < 0x34) {
> +	if (busno == pcie->first_busno && ((offset >= 0x10 && offset < 0x34) ||
> +					   (offset >= 0x38 && offset < 0x3c))) {
>   		data = pcie->cfgcache[(offset - 0x10) / 4];
>   		debug("(addr,size,val)=(0x%04x, %d, 0x%08x) from cfgcache\n",
>   		      offset, size, data);
>   		*valuep = pci_conv_32_to_size(data, offset, size);
>   		return 0;
> -	} else if (busno == pcie->first_busno &&
> -		   (offset & ~3) == PCI_ROM_ADDRESS1) {
> -		/* mvebu has Expansion ROM Base Address (0x38) at offset 0x30 */
> -		offset -= PCI_ROM_ADDRESS1 - PCIE_EXP_ROM_BAR_OFF;
>   	}
>   
>   	/*
> @@ -269,17 +269,21 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf,
>   	}
>   
>   	/*
> -	 * mvebu has different internal registers mapped into PCI config space
> -	 * in range 0x10-0x34 for PCI bridge, so do not access PCI config space
> -	 * for this range and instead write content to driver virtual cfgcache
> +	 * As explained in mvebu_pcie_read_config(), PCI Bridge Type 1 specific
> +	 * config registers are not available, so we write their content only
> +	 * into driver virtual cfgcache[].
> +	 * And as explained in mvebu_pcie_probe(), mvebu has its own specific
> +	 * way for configuring primary and secondary bus numbers.
>   	 */
> -	if (busno == pcie->first_busno && offset >= 0x10 && offset < 0x34) {
> +	if (busno == pcie->first_busno && ((offset >= 0x10 && offset < 0x34) ||
> +					   (offset >= 0x38 && offset < 0x3c))) {
>   		debug("Writing to cfgcache only\n");
>   		data = pcie->cfgcache[(offset - 0x10) / 4];
>   		data = pci_conv_size_to_32(data, value, offset, size);
>   		/* mvebu PCI bridge does not have configurable bars */
>   		if ((offset & ~3) == PCI_BASE_ADDRESS_0 ||
> -		    (offset & ~3) == PCI_BASE_ADDRESS_1)
> +		    (offset & ~3) == PCI_BASE_ADDRESS_1 ||
> +		    (offset & ~3) == PCI_ROM_ADDRESS1)
>   			data = 0x0;
>   		pcie->cfgcache[(offset - 0x10) / 4] = data;
>   		/* mvebu has its own way how to set PCI primary bus number */
> @@ -297,10 +301,6 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf,
>   			      pcie->sec_busno);
>   		}
>   		return 0;
> -	} else if (busno == pcie->first_busno &&
> -		   (offset & ~3) == PCI_ROM_ADDRESS1) {
> -		/* mvebu has Expansion ROM Base Address (0x38) at offset 0x30 */
> -		offset -= PCI_ROM_ADDRESS1 - PCIE_EXP_ROM_BAR_OFF;
>   	}
>   
>   	/*
> @@ -424,13 +424,20 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   	 * U-Boot cannot recognize as P2P Bridge.
>   	 *
>   	 * Note that this mvebu PCI Bridge does not have compliant Type 1
> -	 * Configuration Space. Header Type is reported as Type 0 and in
> -	 * range 0x10-0x34 it has aliased internal mvebu registers 0x10-0x34
> -	 * (e.g. PCIE_BAR_LO_OFF) and register 0x38 is reserved.
> +	 * Configuration Space. Header Type is reported as Type 0 and it
> +	 * has format of Type 0 config space.
>   	 *
> -	 * Driver for this range redirects access to virtual cfgcache[] buffer
> -	 * which avoids changing internal mvebu registers. And changes Header
> -	 * Type response value to Type 1.
> +	 * Moreover Type 0 BAR registers (ranges 0x10 - 0x28 and 0x30 - 0x34)
> +	 * have the same format in Marvell's specification as in PCIe
> +	 * specification, but their meaning is totally different and they do
> +	 * different things: they are aliased into internal mvebu registers
> +	 * (e.g. PCIE_BAR_LO_OFF) and these should not be changed or
> +	 * reconfigured by pci device drivers.
> +	 *
> +	 * So our driver converts Type 0 config space to Type 1 and reports
> +	 * Header Type as Type 1. Access to BAR registers and to non-existent
> +	 * Type 1 registers is redirected to the virtual cfgcache[] buffer,
> +	 * which avoids changing unrelated registers.
>   	 */
>   	reg = readl(pcie->base + PCIE_DEV_REV_OFF);
>   	reg &= ~0xffffff00;
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell 07/10] pci: pci_mvebu: Fix PCIe MEM and IO resources assignment and mbus mapping

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Do not call pci_set_region() for resources which were not properly mapped.
This prevents U-Boot to access unmapped memory space.

Update MBUS_PCI_MEM_SIZE and MBUS_PCI_IO_SIZE macros to cover all PCIe MEM
and IO ranges. Previously these macros covered only address ranges for the
first PCIe port. Between MBUS_PCI_IO_BASE and MBUS_PCI_MEM_BASE there is
space for six 128 MB long address ranges. So set MBUS_PCI_MEM_SIZE to value
of 6*128 MB. Similarly set MBUS_PCI_IO_SIZE to 6*64 KB.

Function resource_size() returns zero when start address is 0 and end
address is -1. So set invalid resources to these values to indicate that
resource has no mapping.

Split global PCIe MEM and IO resources (defined by MBUS_PCI_*_* macros)
into PCIe ports in mvebu_pcie_bind() function which allocates per-port
based struct mvebu_pcie, instead of using global state variables
mvebu_pcie_membase and mvebu_pcie_iobase. This makes pci_mvebu.c driver
independent of global static variables (which store the state of
allocation) and allows to bind and unbind the driver more times.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 arch/arm/mach-mvebu/include/mach/cpu.h |  4 +-
 drivers/pci/pci_mvebu.c                | 84 ++++++++++++++++++--------
 2 files changed, 61 insertions(+), 27 deletions(-)

diff --git a/arch/arm/mach-mvebu/include/mach/cpu.h b/arch/arm/mach-mvebu/include/mach/cpu.h
index a7a62c7e7d..4c52a330d9 100644
--- a/arch/arm/mach-mvebu/include/mach/cpu.h
+++ b/arch/arm/mach-mvebu/include/mach/cpu.h
@@ -75,9 +75,9 @@ enum {
  * Default Device Address MAP BAR values
  */
 #define MBUS_PCI_MEM_BASE	MVEBU_SDRAM_SIZE_MAX
-#define MBUS_PCI_MEM_SIZE	(128 << 20)
+#define MBUS_PCI_MEM_SIZE	((6*128) << 20)
 #define MBUS_PCI_IO_BASE	0xF1100000
-#define MBUS_PCI_IO_SIZE	(64 << 10)
+#define MBUS_PCI_IO_SIZE	((6*64) << 10)
 #define MBUS_SPI_BASE		0xF4000000
 #define MBUS_SPI_SIZE		(8 << 20)
 #define MBUS_DFX_BASE		0xF6000000
diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
index 701a17dfb7..fea32414bf 100644
--- a/drivers/pci/pci_mvebu.c
+++ b/drivers/pci/pci_mvebu.c
@@ -26,6 +26,7 @@
 #include <linux/errno.h>
 #include <linux/ioport.h>
 #include <linux/mbus.h>
+#include <linux/sizes.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -96,14 +97,6 @@ struct mvebu_pcie {
 	u32 cfgcache[(0x3c - 0x10) / 4];
 };
 
-/*
- * MVEBU PCIe controller needs MEMORY and I/O BARs to be mapped
- * into SoCs address space. Each controller will map 128M of MEM
- * and 64K of I/O space when registered.
- */
-static void __iomem *mvebu_pcie_membase = (void __iomem *)MBUS_PCI_MEM_BASE;
-static void __iomem *mvebu_pcie_iobase = (void __iomem *)MBUS_PCI_IO_BASE;
-
 static inline bool mvebu_pcie_link_up(struct mvebu_pcie *pcie)
 {
 	u32 val;
@@ -478,26 +471,24 @@ static int mvebu_pcie_probe(struct udevice *dev)
 	mvebu_pcie_set_local_bus_nr(pcie, 0);
 	mvebu_pcie_set_local_dev_nr(pcie, 1);
 
-	pcie->mem.start = (u32)mvebu_pcie_membase;
-	pcie->mem.end = pcie->mem.start + MBUS_PCI_MEM_SIZE - 1;
-	mvebu_pcie_membase += MBUS_PCI_MEM_SIZE;
-
-	if (mvebu_mbus_add_window_by_id(pcie->mem_target, pcie->mem_attr,
+	if (resource_size(&pcie->mem) &&
+	    mvebu_mbus_add_window_by_id(pcie->mem_target, pcie->mem_attr,
 					(phys_addr_t)pcie->mem.start,
 					resource_size(&pcie->mem))) {
 		printf("PCIe unable to add mbus window for mem at %08x+%08x\n",
 		       (u32)pcie->mem.start, (unsigned)resource_size(&pcie->mem));
+		pcie->mem.start = 0;
+		pcie->mem.end = -1;
 	}
 
-	pcie->io.start = (u32)mvebu_pcie_iobase;
-	pcie->io.end = pcie->io.start + MBUS_PCI_IO_SIZE - 1;
-	mvebu_pcie_iobase += MBUS_PCI_IO_SIZE;
-
-	if (mvebu_mbus_add_window_by_id(pcie->io_target, pcie->io_attr,
+	if (resource_size(&pcie->io) &&
+	    mvebu_mbus_add_window_by_id(pcie->io_target, pcie->io_attr,
 					(phys_addr_t)pcie->io.start,
 					resource_size(&pcie->io))) {
 		printf("PCIe unable to add mbus window for IO at %08x+%08x\n",
 		       (u32)pcie->io.start, (unsigned)resource_size(&pcie->io));
+		pcie->io.start = 0;
+		pcie->io.end = -1;
 	}
 
 	/* Setup windows and configure host bridge */
@@ -506,13 +497,23 @@ static int mvebu_pcie_probe(struct udevice *dev)
 	/* PCI memory space */
 	pci_set_region(hose->regions + 0, pcie->mem.start,
 		       pcie->mem.start, resource_size(&pcie->mem), PCI_REGION_MEM);
-	pci_set_region(hose->regions + 1,
-		       0, 0,
-		       gd->ram_size,
-		       PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
-	pci_set_region(hose->regions + 2, pcie->io.start,
-		       pcie->io.start, resource_size(&pcie->io), PCI_REGION_IO);
-	hose->region_count = 3;
+	hose->region_count = 1;
+
+	if (resource_size(&pcie->mem)) {
+		pci_set_region(hose->regions + hose->region_count,
+			       pcie->mem.start, pcie->mem.start,
+			       resource_size(&pcie->mem),
+			       PCI_REGION_MEM);
+		hose->region_count++;
+	}
+
+	if (resource_size(&pcie->io)) {
+		pci_set_region(hose->regions + hose->region_count,
+			       pcie->io.start, pcie->io.start,
+			       resource_size(&pcie->io),
+			       PCI_REGION_IO);
+		hose->region_count++;
+	}
 
 	/* PCI Bridge support 32-bit I/O and 64-bit prefetch mem addressing */
 	pcie->cfgcache[(PCI_IO_BASE - 0x10) / 4] =
@@ -680,6 +681,8 @@ static int mvebu_pcie_bind(struct udevice *parent)
 	struct mvebu_pcie *pcie;
 	struct uclass_driver *drv;
 	struct udevice *dev;
+	struct resource mem;
+	struct resource io;
 	ofnode subnode;
 
 	/* Lookup pci driver */
@@ -689,6 +692,11 @@ static int mvebu_pcie_bind(struct udevice *parent)
 		return -ENOENT;
 	}
 
+	mem.start = MBUS_PCI_MEM_BASE;
+	mem.end = MBUS_PCI_MEM_BASE + MBUS_PCI_MEM_SIZE - 1;
+	io.start = MBUS_PCI_IO_BASE;
+	io.end = MBUS_PCI_IO_BASE + MBUS_PCI_IO_SIZE - 1;
+
 	ofnode_for_each_subnode(subnode, dev_ofnode(parent)) {
 		if (!ofnode_is_available(subnode))
 			continue;
@@ -697,6 +705,32 @@ static int mvebu_pcie_bind(struct udevice *parent)
 		if (!pcie)
 			return -ENOMEM;
 
+		/*
+		 * MVEBU PCIe controller needs MEMORY and I/O BARs to be mapped
+		 * into SoCs address space. Each controller will map 128M of MEM
+		 * and 64K of I/O space when registered.
+		 */
+
+		if (resource_size(&mem) >= SZ_128M) {
+			pcie->mem.start = mem.start;
+			pcie->mem.end = mem.start + SZ_128M - 1;
+			mem.start += SZ_128M;
+		} else {
+			printf("PCIe unable to assign mbus window for mem\n");
+			pcie->mem.start = 0;
+			pcie->mem.end = -1;
+		}
+
+		if (resource_size(&io) >= SZ_64K) {
+			pcie->io.start = io.start;
+			pcie->io.end = io.start + SZ_64K - 1;
+			io.start += SZ_64K;
+		} else {
+			printf("PCIe unable to assign mbus window for io\n");
+			pcie->io.start = 0;
+			pcie->io.end = -1;
+		}
+
 		/* Create child device UCLASS_PCI and bind it */
 		device_bind(parent, &pcie_mvebu_drv, pcie->name, pcie, subnode,
 			    &dev);
-- 
2.32.0

Re: [PATCH u-boot-marvell 07/10] pci: pci_mvebu: Fix PCIe MEM and IO resources assignment and mbus mapping

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Do not call pci_set_region() for resources which were not properly mapped.
> This prevents U-Boot to access unmapped memory space.
> 
> Update MBUS_PCI_MEM_SIZE and MBUS_PCI_IO_SIZE macros to cover all PCIe MEM
> and IO ranges. Previously these macros covered only address ranges for the
> first PCIe port. Between MBUS_PCI_IO_BASE and MBUS_PCI_MEM_BASE there is
> space for six 128 MB long address ranges. So set MBUS_PCI_MEM_SIZE to value
> of 6*128 MB. Similarly set MBUS_PCI_IO_SIZE to 6*64 KB.

Perhaps it makes sense to calculate this '6' in some macro? This makes
it easier to understand this value at a later time. Something like this:

#define MEM_SIZE_TOTAL	(0xf0000000 - MBUS_PCI_MEM_BASE) // or some 
better macro for 0xf000000 here?
#define MEM_REGION_COUNT (MEM_SIZE_TOTAL / (128 << 20))

?

> Function resource_size() returns zero when start address is 0 and end
> address is -1. So set invalid resources to these values to indicate that
> resource has no mapping.
> 
> Split global PCIe MEM and IO resources (defined by MBUS_PCI_*_* macros)
> into PCIe ports in mvebu_pcie_bind() function which allocates per-port
> based struct mvebu_pcie, instead of using global state variables
> mvebu_pcie_membase and mvebu_pcie_iobase. This makes pci_mvebu.c driver
> independent of global static variables (which store the state of
> allocation) and allows to bind and unbind the driver more times.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>   arch/arm/mach-mvebu/include/mach/cpu.h |  4 +-
>   drivers/pci/pci_mvebu.c                | 84 ++++++++++++++++++--------
>   2 files changed, 61 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm/mach-mvebu/include/mach/cpu.h b/arch/arm/mach-mvebu/include/mach/cpu.h
> index a7a62c7e7d..4c52a330d9 100644
> --- a/arch/arm/mach-mvebu/include/mach/cpu.h
> +++ b/arch/arm/mach-mvebu/include/mach/cpu.h
> @@ -75,9 +75,9 @@ enum {
>    * Default Device Address MAP BAR values
>    */
>   #define MBUS_PCI_MEM_BASE	MVEBU_SDRAM_SIZE_MAX
> -#define MBUS_PCI_MEM_SIZE	(128 << 20)
> +#define MBUS_PCI_MEM_SIZE	((6*128) << 20)

Nitpicking: Space before and after the '*' please.

>   #define MBUS_PCI_IO_BASE	0xF1100000
> -#define MBUS_PCI_IO_SIZE	(64 << 10)
> +#define MBUS_PCI_IO_SIZE	((6*64) << 10)

Same here.

>   #define MBUS_SPI_BASE		0xF4000000
>   #define MBUS_SPI_SIZE		(8 << 20)
>   #define MBUS_DFX_BASE		0xF6000000
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index 701a17dfb7..fea32414bf 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -26,6 +26,7 @@
>   #include <linux/errno.h>
>   #include <linux/ioport.h>
>   #include <linux/mbus.h>
> +#include <linux/sizes.h>
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> @@ -96,14 +97,6 @@ struct mvebu_pcie {
>   	u32 cfgcache[(0x3c - 0x10) / 4];
>   };
>   
> -/*
> - * MVEBU PCIe controller needs MEMORY and I/O BARs to be mapped
> - * into SoCs address space. Each controller will map 128M of MEM
> - * and 64K of I/O space when registered.
> - */
> -static void __iomem *mvebu_pcie_membase = (void __iomem *)MBUS_PCI_MEM_BASE;
> -static void __iomem *mvebu_pcie_iobase = (void __iomem *)MBUS_PCI_IO_BASE;
> -
>   static inline bool mvebu_pcie_link_up(struct mvebu_pcie *pcie)
>   {
>   	u32 val;
> @@ -478,26 +471,24 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   	mvebu_pcie_set_local_bus_nr(pcie, 0);
>   	mvebu_pcie_set_local_dev_nr(pcie, 1);
>   
> -	pcie->mem.start = (u32)mvebu_pcie_membase;
> -	pcie->mem.end = pcie->mem.start + MBUS_PCI_MEM_SIZE - 1;
> -	mvebu_pcie_membase += MBUS_PCI_MEM_SIZE;
> -
> -	if (mvebu_mbus_add_window_by_id(pcie->mem_target, pcie->mem_attr,
> +	if (resource_size(&pcie->mem) &&
> +	    mvebu_mbus_add_window_by_id(pcie->mem_target, pcie->mem_attr,
>   					(phys_addr_t)pcie->mem.start,
>   					resource_size(&pcie->mem))) {
>   		printf("PCIe unable to add mbus window for mem at %08x+%08x\n",
>   		       (u32)pcie->mem.start, (unsigned)resource_size(&pcie->mem));
> +		pcie->mem.start = 0;
> +		pcie->mem.end = -1;
>   	}
>   
> -	pcie->io.start = (u32)mvebu_pcie_iobase;
> -	pcie->io.end = pcie->io.start + MBUS_PCI_IO_SIZE - 1;
> -	mvebu_pcie_iobase += MBUS_PCI_IO_SIZE;
> -
> -	if (mvebu_mbus_add_window_by_id(pcie->io_target, pcie->io_attr,
> +	if (resource_size(&pcie->io) &&
> +	    mvebu_mbus_add_window_by_id(pcie->io_target, pcie->io_attr,
>   					(phys_addr_t)pcie->io.start,
>   					resource_size(&pcie->io))) {
>   		printf("PCIe unable to add mbus window for IO at %08x+%08x\n",
>   		       (u32)pcie->io.start, (unsigned)resource_size(&pcie->io));
> +		pcie->io.start = 0;
> +		pcie->io.end = -1;
>   	}
>   
>   	/* Setup windows and configure host bridge */
> @@ -506,13 +497,23 @@ static int mvebu_pcie_probe(struct udevice *dev)
>   	/* PCI memory space */
>   	pci_set_region(hose->regions + 0, pcie->mem.start,
>   		       pcie->mem.start, resource_size(&pcie->mem), PCI_REGION_MEM);
> -	pci_set_region(hose->regions + 1,
> -		       0, 0,
> -		       gd->ram_size,
> -		       PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
> -	pci_set_region(hose->regions + 2, pcie->io.start,
> -		       pcie->io.start, resource_size(&pcie->io), PCI_REGION_IO);
> -	hose->region_count = 3;
> +	hose->region_count = 1;
> +
> +	if (resource_size(&pcie->mem)) {
> +		pci_set_region(hose->regions + hose->region_count,
> +			       pcie->mem.start, pcie->mem.start,
> +			       resource_size(&pcie->mem),
> +			       PCI_REGION_MEM);
> +		hose->region_count++;
> +	}
> +
> +	if (resource_size(&pcie->io)) {
> +		pci_set_region(hose->regions + hose->region_count,
> +			       pcie->io.start, pcie->io.start,
> +			       resource_size(&pcie->io),
> +			       PCI_REGION_IO);
> +		hose->region_count++;
> +	}
>   
>   	/* PCI Bridge support 32-bit I/O and 64-bit prefetch mem addressing */
>   	pcie->cfgcache[(PCI_IO_BASE - 0x10) / 4] =
> @@ -680,6 +681,8 @@ static int mvebu_pcie_bind(struct udevice *parent)
>   	struct mvebu_pcie *pcie;
>   	struct uclass_driver *drv;
>   	struct udevice *dev;
> +	struct resource mem;
> +	struct resource io;
>   	ofnode subnode;
>   
>   	/* Lookup pci driver */
> @@ -689,6 +692,11 @@ static int mvebu_pcie_bind(struct udevice *parent)
>   		return -ENOENT;
>   	}
>   
> +	mem.start = MBUS_PCI_MEM_BASE;
> +	mem.end = MBUS_PCI_MEM_BASE + MBUS_PCI_MEM_SIZE - 1;
> +	io.start = MBUS_PCI_IO_BASE;
> +	io.end = MBUS_PCI_IO_BASE + MBUS_PCI_IO_SIZE - 1;
> +
>   	ofnode_for_each_subnode(subnode, dev_ofnode(parent)) {
>   		if (!ofnode_is_available(subnode))
>   			continue;
> @@ -697,6 +705,32 @@ static int mvebu_pcie_bind(struct udevice *parent)
>   		if (!pcie)
>   			return -ENOMEM;
>   
> +		/*
> +		 * MVEBU PCIe controller needs MEMORY and I/O BARs to be mapped
> +		 * into SoCs address space. Each controller will map 128M of MEM
> +		 * and 64K of I/O space when registered.
> +		 */
> +
> +		if (resource_size(&mem) >= SZ_128M) {
> +			pcie->mem.start = mem.start;
> +			pcie->mem.end = mem.start + SZ_128M - 1;
> +			mem.start += SZ_128M;
> +		} else {
> +			printf("PCIe unable to assign mbus window for mem\n");
> +			pcie->mem.start = 0;
> +			pcie->mem.end = -1;
> +		}
> +
> +		if (resource_size(&io) >= SZ_64K) {
> +			pcie->io.start = io.start;
> +			pcie->io.end = io.start + SZ_64K - 1;
> +			io.start += SZ_64K;
> +		} else {
> +			printf("PCIe unable to assign mbus window for io\n");
> +			pcie->io.start = 0;
> +			pcie->io.end = -1;
> +		}
> +
>   		/* Create child device UCLASS_PCI and bind it */
>   		device_bind(parent, &pcie_mvebu_drv, pcie->name, pcie, subnode,
>   			    &dev);
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 07/10] pci: pci_mvebu: Fix PCIe MEM and IO resources assignment and mbus mapping

[Pali Rohár]

On Friday 12 November 2021 15:18:48 Stefan Roese wrote:
> On 11/11/21 16:35, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > Do not call pci_set_region() for resources which were not properly mapped.
> > This prevents U-Boot to access unmapped memory space.
> > 
> > Update MBUS_PCI_MEM_SIZE and MBUS_PCI_IO_SIZE macros to cover all PCIe MEM
> > and IO ranges. Previously these macros covered only address ranges for the
> > first PCIe port. Between MBUS_PCI_IO_BASE and MBUS_PCI_MEM_BASE there is
> > space for six 128 MB long address ranges. So set MBUS_PCI_MEM_SIZE to value
> > of 6*128 MB. Similarly set MBUS_PCI_IO_SIZE to 6*64 KB.
> 
> Perhaps it makes sense to calculate this '6' in some macro?

Hm... maybe.

Or maybe better to add "#define maximal_number_of_pcie_ports 6" (but
with better name!) as 6 is the maximal number of PCIe ports on 88f78xx0.
Other SoCs can have maximally only 4 PCIe ports. So even if we change
this default mapping in future and there would be bigger gap between
MBUS_PCI_IO_BASE and MBUS_PCI_MEM_BASE, it does not make sense to change
that '6' to any higher value.

What do you think?

> This makes
> it easier to understand this value at a later time. Something like this:
> 
> #define MEM_SIZE_TOTAL	(0xf0000000 - MBUS_PCI_MEM_BASE) // or some better
> macro for 0xf000000 here?
> #define MEM_REGION_COUNT (MEM_SIZE_TOTAL / (128 << 20))
> 
> ?
> 
> > Function resource_size() returns zero when start address is 0 and end
> > address is -1. So set invalid resources to these values to indicate that
> > resource has no mapping.
> > 
> > Split global PCIe MEM and IO resources (defined by MBUS_PCI_*_* macros)
> > into PCIe ports in mvebu_pcie_bind() function which allocates per-port
> > based struct mvebu_pcie, instead of using global state variables
> > mvebu_pcie_membase and mvebu_pcie_iobase. This makes pci_mvebu.c driver
> > independent of global static variables (which store the state of
> > allocation) and allows to bind and unbind the driver more times.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >   arch/arm/mach-mvebu/include/mach/cpu.h |  4 +-
> >   drivers/pci/pci_mvebu.c                | 84 ++++++++++++++++++--------
> >   2 files changed, 61 insertions(+), 27 deletions(-)
> > 
> > diff --git a/arch/arm/mach-mvebu/include/mach/cpu.h b/arch/arm/mach-mvebu/include/mach/cpu.h
> > index a7a62c7e7d..4c52a330d9 100644
> > --- a/arch/arm/mach-mvebu/include/mach/cpu.h
> > +++ b/arch/arm/mach-mvebu/include/mach/cpu.h
> > @@ -75,9 +75,9 @@ enum {
> >    * Default Device Address MAP BAR values
> >    */
> >   #define MBUS_PCI_MEM_BASE	MVEBU_SDRAM_SIZE_MAX
> > -#define MBUS_PCI_MEM_SIZE	(128 << 20)
> > +#define MBUS_PCI_MEM_SIZE	((6*128) << 20)
> 
> Nitpicking: Space before and after the '*' please.
> 
> >   #define MBUS_PCI_IO_BASE	0xF1100000
> > -#define MBUS_PCI_IO_SIZE	(64 << 10)
> > +#define MBUS_PCI_IO_SIZE	((6*64) << 10)
> 
> Same here.
> 
> >   #define MBUS_SPI_BASE		0xF4000000
> >   #define MBUS_SPI_SIZE		(8 << 20)
> >   #define MBUS_DFX_BASE		0xF6000000
> > diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> > index 701a17dfb7..fea32414bf 100644
> > --- a/drivers/pci/pci_mvebu.c
> > +++ b/drivers/pci/pci_mvebu.c
> > @@ -26,6 +26,7 @@
> >   #include <linux/errno.h>
> >   #include <linux/ioport.h>
> >   #include <linux/mbus.h>
> > +#include <linux/sizes.h>
> >   DECLARE_GLOBAL_DATA_PTR;
> > @@ -96,14 +97,6 @@ struct mvebu_pcie {
> >   	u32 cfgcache[(0x3c - 0x10) / 4];
> >   };
> > -/*
> > - * MVEBU PCIe controller needs MEMORY and I/O BARs to be mapped
> > - * into SoCs address space. Each controller will map 128M of MEM
> > - * and 64K of I/O space when registered.
> > - */
> > -static void __iomem *mvebu_pcie_membase = (void __iomem *)MBUS_PCI_MEM_BASE;
> > -static void __iomem *mvebu_pcie_iobase = (void __iomem *)MBUS_PCI_IO_BASE;
> > -
> >   static inline bool mvebu_pcie_link_up(struct mvebu_pcie *pcie)
> >   {
> >   	u32 val;
> > @@ -478,26 +471,24 @@ static int mvebu_pcie_probe(struct udevice *dev)
> >   	mvebu_pcie_set_local_bus_nr(pcie, 0);
> >   	mvebu_pcie_set_local_dev_nr(pcie, 1);
> > -	pcie->mem.start = (u32)mvebu_pcie_membase;
> > -	pcie->mem.end = pcie->mem.start + MBUS_PCI_MEM_SIZE - 1;
> > -	mvebu_pcie_membase += MBUS_PCI_MEM_SIZE;
> > -
> > -	if (mvebu_mbus_add_window_by_id(pcie->mem_target, pcie->mem_attr,
> > +	if (resource_size(&pcie->mem) &&
> > +	    mvebu_mbus_add_window_by_id(pcie->mem_target, pcie->mem_attr,
> >   					(phys_addr_t)pcie->mem.start,
> >   					resource_size(&pcie->mem))) {
> >   		printf("PCIe unable to add mbus window for mem at %08x+%08x\n",
> >   		       (u32)pcie->mem.start, (unsigned)resource_size(&pcie->mem));
> > +		pcie->mem.start = 0;
> > +		pcie->mem.end = -1;
> >   	}
> > -	pcie->io.start = (u32)mvebu_pcie_iobase;
> > -	pcie->io.end = pcie->io.start + MBUS_PCI_IO_SIZE - 1;
> > -	mvebu_pcie_iobase += MBUS_PCI_IO_SIZE;
> > -
> > -	if (mvebu_mbus_add_window_by_id(pcie->io_target, pcie->io_attr,
> > +	if (resource_size(&pcie->io) &&
> > +	    mvebu_mbus_add_window_by_id(pcie->io_target, pcie->io_attr,
> >   					(phys_addr_t)pcie->io.start,
> >   					resource_size(&pcie->io))) {
> >   		printf("PCIe unable to add mbus window for IO at %08x+%08x\n",
> >   		       (u32)pcie->io.start, (unsigned)resource_size(&pcie->io));
> > +		pcie->io.start = 0;
> > +		pcie->io.end = -1;
> >   	}
> >   	/* Setup windows and configure host bridge */
> > @@ -506,13 +497,23 @@ static int mvebu_pcie_probe(struct udevice *dev)
> >   	/* PCI memory space */
> >   	pci_set_region(hose->regions + 0, pcie->mem.start,
> >   		       pcie->mem.start, resource_size(&pcie->mem), PCI_REGION_MEM);
> > -	pci_set_region(hose->regions + 1,
> > -		       0, 0,
> > -		       gd->ram_size,
> > -		       PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
> > -	pci_set_region(hose->regions + 2, pcie->io.start,
> > -		       pcie->io.start, resource_size(&pcie->io), PCI_REGION_IO);
> > -	hose->region_count = 3;
> > +	hose->region_count = 1;
> > +
> > +	if (resource_size(&pcie->mem)) {
> > +		pci_set_region(hose->regions + hose->region_count,
> > +			       pcie->mem.start, pcie->mem.start,
> > +			       resource_size(&pcie->mem),
> > +			       PCI_REGION_MEM);
> > +		hose->region_count++;
> > +	}
> > +
> > +	if (resource_size(&pcie->io)) {
> > +		pci_set_region(hose->regions + hose->region_count,
> > +			       pcie->io.start, pcie->io.start,
> > +			       resource_size(&pcie->io),
> > +			       PCI_REGION_IO);
> > +		hose->region_count++;
> > +	}
> >   	/* PCI Bridge support 32-bit I/O and 64-bit prefetch mem addressing */
> >   	pcie->cfgcache[(PCI_IO_BASE - 0x10) / 4] =
> > @@ -680,6 +681,8 @@ static int mvebu_pcie_bind(struct udevice *parent)
> >   	struct mvebu_pcie *pcie;
> >   	struct uclass_driver *drv;
> >   	struct udevice *dev;
> > +	struct resource mem;
> > +	struct resource io;
> >   	ofnode subnode;
> >   	/* Lookup pci driver */
> > @@ -689,6 +692,11 @@ static int mvebu_pcie_bind(struct udevice *parent)
> >   		return -ENOENT;
> >   	}
> > +	mem.start = MBUS_PCI_MEM_BASE;
> > +	mem.end = MBUS_PCI_MEM_BASE + MBUS_PCI_MEM_SIZE - 1;
> > +	io.start = MBUS_PCI_IO_BASE;
> > +	io.end = MBUS_PCI_IO_BASE + MBUS_PCI_IO_SIZE - 1;
> > +
> >   	ofnode_for_each_subnode(subnode, dev_ofnode(parent)) {
> >   		if (!ofnode_is_available(subnode))
> >   			continue;
> > @@ -697,6 +705,32 @@ static int mvebu_pcie_bind(struct udevice *parent)
> >   		if (!pcie)
> >   			return -ENOMEM;
> > +		/*
> > +		 * MVEBU PCIe controller needs MEMORY and I/O BARs to be mapped
> > +		 * into SoCs address space. Each controller will map 128M of MEM
> > +		 * and 64K of I/O space when registered.
> > +		 */
> > +
> > +		if (resource_size(&mem) >= SZ_128M) {
> > +			pcie->mem.start = mem.start;
> > +			pcie->mem.end = mem.start + SZ_128M - 1;
> > +			mem.start += SZ_128M;
> > +		} else {
> > +			printf("PCIe unable to assign mbus window for mem\n");
> > +			pcie->mem.start = 0;
> > +			pcie->mem.end = -1;
> > +		}
> > +
> > +		if (resource_size(&io) >= SZ_64K) {
> > +			pcie->io.start = io.start;
> > +			pcie->io.end = io.start + SZ_64K - 1;
> > +			io.start += SZ_64K;
> > +		} else {
> > +			printf("PCIe unable to assign mbus window for io\n");
> > +			pcie->io.start = 0;
> > +			pcie->io.end = -1;
> > +		}
> > +
> >   		/* Create child device UCLASS_PCI and bind it */
> >   		device_bind(parent, &pcie_mvebu_drv, pcie->name, pcie, subnode,
> >   			    &dev);
> > 
> 
> Viele Grüße,
> Stefan Roese
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 07/10] pci: pci_mvebu: Fix PCIe MEM and IO resources assignment and mbus mapping

[Stefan Roese]

On 11/18/21 18:46, Pali Rohár wrote:
> On Friday 12 November 2021 15:18:48 Stefan Roese wrote:
>> On 11/11/21 16:35, Marek Behún wrote:
>>> From: Pali Rohár <pali@kernel.org>
>>>
>>> Do not call pci_set_region() for resources which were not properly mapped.
>>> This prevents U-Boot to access unmapped memory space.
>>>
>>> Update MBUS_PCI_MEM_SIZE and MBUS_PCI_IO_SIZE macros to cover all PCIe MEM
>>> and IO ranges. Previously these macros covered only address ranges for the
>>> first PCIe port. Between MBUS_PCI_IO_BASE and MBUS_PCI_MEM_BASE there is
>>> space for six 128 MB long address ranges. So set MBUS_PCI_MEM_SIZE to value
>>> of 6*128 MB. Similarly set MBUS_PCI_IO_SIZE to 6*64 KB.
>>
>> Perhaps it makes sense to calculate this '6' in some macro?
> 
> Hm... maybe.
> 
> Or maybe better to add "#define maximal_number_of_pcie_ports 6" (but
> with better name!) as 6 is the maximal number of PCIe ports on 88f78xx0.
> Other SoCs can have maximally only 4 PCIe ports. So even if we change
> this default mapping in future and there would be bigger gap between
> MBUS_PCI_IO_BASE and MBUS_PCI_MEM_BASE, it does not make sense to change
> that '6' to any higher value.
> 
> What do you think?

Sounds good to me.

Thanks,
Stefan

>> This makes
>> it easier to understand this value at a later time. Something like this:
>>
>> #define MEM_SIZE_TOTAL	(0xf0000000 - MBUS_PCI_MEM_BASE) // or some better
>> macro for 0xf000000 here?
>> #define MEM_REGION_COUNT (MEM_SIZE_TOTAL / (128 << 20))
>>
>> ?
>>
>>> Function resource_size() returns zero when start address is 0 and end
>>> address is -1. So set invalid resources to these values to indicate that
>>> resource has no mapping.
>>>
>>> Split global PCIe MEM and IO resources (defined by MBUS_PCI_*_* macros)
>>> into PCIe ports in mvebu_pcie_bind() function which allocates per-port
>>> based struct mvebu_pcie, instead of using global state variables
>>> mvebu_pcie_membase and mvebu_pcie_iobase. This makes pci_mvebu.c driver
>>> independent of global static variables (which store the state of
>>> allocation) and allows to bind and unbind the driver more times.
>>>
>>> Signed-off-by: Pali Rohár <pali@kernel.org>
>>> Signed-off-by: Marek Behún <marek.behun@nic.cz>
>>> ---
>>>    arch/arm/mach-mvebu/include/mach/cpu.h |  4 +-
>>>    drivers/pci/pci_mvebu.c                | 84 ++++++++++++++++++--------
>>>    2 files changed, 61 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-mvebu/include/mach/cpu.h b/arch/arm/mach-mvebu/include/mach/cpu.h
>>> index a7a62c7e7d..4c52a330d9 100644
>>> --- a/arch/arm/mach-mvebu/include/mach/cpu.h
>>> +++ b/arch/arm/mach-mvebu/include/mach/cpu.h
>>> @@ -75,9 +75,9 @@ enum {
>>>     * Default Device Address MAP BAR values
>>>     */
>>>    #define MBUS_PCI_MEM_BASE	MVEBU_SDRAM_SIZE_MAX
>>> -#define MBUS_PCI_MEM_SIZE	(128 << 20)
>>> +#define MBUS_PCI_MEM_SIZE	((6*128) << 20)
>>
>> Nitpicking: Space before and after the '*' please.
>>
>>>    #define MBUS_PCI_IO_BASE	0xF1100000
>>> -#define MBUS_PCI_IO_SIZE	(64 << 10)
>>> +#define MBUS_PCI_IO_SIZE	((6*64) << 10)
>>
>> Same here.
>>
>>>    #define MBUS_SPI_BASE		0xF4000000
>>>    #define MBUS_SPI_SIZE		(8 << 20)
>>>    #define MBUS_DFX_BASE		0xF6000000
>>> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
>>> index 701a17dfb7..fea32414bf 100644
>>> --- a/drivers/pci/pci_mvebu.c
>>> +++ b/drivers/pci/pci_mvebu.c
>>> @@ -26,6 +26,7 @@
>>>    #include <linux/errno.h>
>>>    #include <linux/ioport.h>
>>>    #include <linux/mbus.h>
>>> +#include <linux/sizes.h>
>>>    DECLARE_GLOBAL_DATA_PTR;
>>> @@ -96,14 +97,6 @@ struct mvebu_pcie {
>>>    	u32 cfgcache[(0x3c - 0x10) / 4];
>>>    };
>>> -/*
>>> - * MVEBU PCIe controller needs MEMORY and I/O BARs to be mapped
>>> - * into SoCs address space. Each controller will map 128M of MEM
>>> - * and 64K of I/O space when registered.
>>> - */
>>> -static void __iomem *mvebu_pcie_membase = (void __iomem *)MBUS_PCI_MEM_BASE;
>>> -static void __iomem *mvebu_pcie_iobase = (void __iomem *)MBUS_PCI_IO_BASE;
>>> -
>>>    static inline bool mvebu_pcie_link_up(struct mvebu_pcie *pcie)
>>>    {
>>>    	u32 val;
>>> @@ -478,26 +471,24 @@ static int mvebu_pcie_probe(struct udevice *dev)
>>>    	mvebu_pcie_set_local_bus_nr(pcie, 0);
>>>    	mvebu_pcie_set_local_dev_nr(pcie, 1);
>>> -	pcie->mem.start = (u32)mvebu_pcie_membase;
>>> -	pcie->mem.end = pcie->mem.start + MBUS_PCI_MEM_SIZE - 1;
>>> -	mvebu_pcie_membase += MBUS_PCI_MEM_SIZE;
>>> -
>>> -	if (mvebu_mbus_add_window_by_id(pcie->mem_target, pcie->mem_attr,
>>> +	if (resource_size(&pcie->mem) &&
>>> +	    mvebu_mbus_add_window_by_id(pcie->mem_target, pcie->mem_attr,
>>>    					(phys_addr_t)pcie->mem.start,
>>>    					resource_size(&pcie->mem))) {
>>>    		printf("PCIe unable to add mbus window for mem at %08x+%08x\n",
>>>    		       (u32)pcie->mem.start, (unsigned)resource_size(&pcie->mem));
>>> +		pcie->mem.start = 0;
>>> +		pcie->mem.end = -1;
>>>    	}
>>> -	pcie->io.start = (u32)mvebu_pcie_iobase;
>>> -	pcie->io.end = pcie->io.start + MBUS_PCI_IO_SIZE - 1;
>>> -	mvebu_pcie_iobase += MBUS_PCI_IO_SIZE;
>>> -
>>> -	if (mvebu_mbus_add_window_by_id(pcie->io_target, pcie->io_attr,
>>> +	if (resource_size(&pcie->io) &&
>>> +	    mvebu_mbus_add_window_by_id(pcie->io_target, pcie->io_attr,
>>>    					(phys_addr_t)pcie->io.start,
>>>    					resource_size(&pcie->io))) {
>>>    		printf("PCIe unable to add mbus window for IO at %08x+%08x\n",
>>>    		       (u32)pcie->io.start, (unsigned)resource_size(&pcie->io));
>>> +		pcie->io.start = 0;
>>> +		pcie->io.end = -1;
>>>    	}
>>>    	/* Setup windows and configure host bridge */
>>> @@ -506,13 +497,23 @@ static int mvebu_pcie_probe(struct udevice *dev)
>>>    	/* PCI memory space */
>>>    	pci_set_region(hose->regions + 0, pcie->mem.start,
>>>    		       pcie->mem.start, resource_size(&pcie->mem), PCI_REGION_MEM);
>>> -	pci_set_region(hose->regions + 1,
>>> -		       0, 0,
>>> -		       gd->ram_size,
>>> -		       PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
>>> -	pci_set_region(hose->regions + 2, pcie->io.start,
>>> -		       pcie->io.start, resource_size(&pcie->io), PCI_REGION_IO);
>>> -	hose->region_count = 3;
>>> +	hose->region_count = 1;
>>> +
>>> +	if (resource_size(&pcie->mem)) {
>>> +		pci_set_region(hose->regions + hose->region_count,
>>> +			       pcie->mem.start, pcie->mem.start,
>>> +			       resource_size(&pcie->mem),
>>> +			       PCI_REGION_MEM);
>>> +		hose->region_count++;
>>> +	}
>>> +
>>> +	if (resource_size(&pcie->io)) {
>>> +		pci_set_region(hose->regions + hose->region_count,
>>> +			       pcie->io.start, pcie->io.start,
>>> +			       resource_size(&pcie->io),
>>> +			       PCI_REGION_IO);
>>> +		hose->region_count++;
>>> +	}
>>>    	/* PCI Bridge support 32-bit I/O and 64-bit prefetch mem addressing */
>>>    	pcie->cfgcache[(PCI_IO_BASE - 0x10) / 4] =
>>> @@ -680,6 +681,8 @@ static int mvebu_pcie_bind(struct udevice *parent)
>>>    	struct mvebu_pcie *pcie;
>>>    	struct uclass_driver *drv;
>>>    	struct udevice *dev;
>>> +	struct resource mem;
>>> +	struct resource io;
>>>    	ofnode subnode;
>>>    	/* Lookup pci driver */
>>> @@ -689,6 +692,11 @@ static int mvebu_pcie_bind(struct udevice *parent)
>>>    		return -ENOENT;
>>>    	}
>>> +	mem.start = MBUS_PCI_MEM_BASE;
>>> +	mem.end = MBUS_PCI_MEM_BASE + MBUS_PCI_MEM_SIZE - 1;
>>> +	io.start = MBUS_PCI_IO_BASE;
>>> +	io.end = MBUS_PCI_IO_BASE + MBUS_PCI_IO_SIZE - 1;
>>> +
>>>    	ofnode_for_each_subnode(subnode, dev_ofnode(parent)) {
>>>    		if (!ofnode_is_available(subnode))
>>>    			continue;
>>> @@ -697,6 +705,32 @@ static int mvebu_pcie_bind(struct udevice *parent)
>>>    		if (!pcie)
>>>    			return -ENOMEM;
>>> +		/*
>>> +		 * MVEBU PCIe controller needs MEMORY and I/O BARs to be mapped
>>> +		 * into SoCs address space. Each controller will map 128M of MEM
>>> +		 * and 64K of I/O space when registered.
>>> +		 */
>>> +
>>> +		if (resource_size(&mem) >= SZ_128M) {
>>> +			pcie->mem.start = mem.start;
>>> +			pcie->mem.end = mem.start + SZ_128M - 1;
>>> +			mem.start += SZ_128M;
>>> +		} else {
>>> +			printf("PCIe unable to assign mbus window for mem\n");
>>> +			pcie->mem.start = 0;
>>> +			pcie->mem.end = -1;
>>> +		}
>>> +
>>> +		if (resource_size(&io) >= SZ_64K) {
>>> +			pcie->io.start = io.start;
>>> +			pcie->io.end = io.start + SZ_64K - 1;
>>> +			io.start += SZ_64K;
>>> +		} else {
>>> +			printf("PCIe unable to assign mbus window for io\n");
>>> +			pcie->io.start = 0;
>>> +			pcie->io.end = -1;
>>> +		}
>>> +
>>>    		/* Create child device UCLASS_PCI and bind it */
>>>    		device_bind(parent, &pcie_mvebu_drv, pcie->name, pcie, subnode,
>>>    			    &dev);
>>>
>>
>> Viele Grüße,
>> Stefan Roese
>>
>> -- 
>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell 08/10] pci: pci_mvebu: Remove unused DECLARE_GLOBAL_DATA_PTR

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

The global data pointer is not used in this driver, remove it's
declaration.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/pci_mvebu.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
index fea32414bf..f3830f67aa 100644
--- a/drivers/pci/pci_mvebu.c
+++ b/drivers/pci/pci_mvebu.c
@@ -13,7 +13,6 @@
 #include <dm.h>
 #include <log.h>
 #include <malloc.h>
-#include <asm/global_data.h>
 #include <dm/device-internal.h>
 #include <dm/lists.h>
 #include <dm/of_access.h>
@@ -28,8 +27,6 @@
 #include <linux/mbus.h>
 #include <linux/sizes.h>
 
-DECLARE_GLOBAL_DATA_PTR;
-
 /* PCIe unit register offsets */
 #define SELECT(x, n)			((x >> n) & 1UL)
 
-- 
2.32.0

Re: [PATCH u-boot-marvell 08/10] pci: pci_mvebu: Remove unused DECLARE_GLOBAL_DATA_PTR

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> The global data pointer is not used in this driver, remove it's
> declaration.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/pci/pci_mvebu.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index fea32414bf..f3830f67aa 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -13,7 +13,6 @@
>   #include <dm.h>
>   #include <log.h>
>   #include <malloc.h>
> -#include <asm/global_data.h>
>   #include <dm/device-internal.h>
>   #include <dm/lists.h>
>   #include <dm/of_access.h>
> @@ -28,8 +27,6 @@
>   #include <linux/mbus.h>
>   #include <linux/sizes.h>
>   
> -DECLARE_GLOBAL_DATA_PTR;
> -
>   /* PCIe unit register offsets */
>   #define SELECT(x, n)			((x >> n) & 1UL)
>   
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 08/10] pci: pci_mvebu: Remove unused DECLARE_GLOBAL_DATA_PTR

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> The global data pointer is not used in this driver, remove it's
> declaration.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Applied to u-boot-marvell/next

Thanks,
Stefan

> ---
>   drivers/pci/pci_mvebu.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
> index fea32414bf..f3830f67aa 100644
> --- a/drivers/pci/pci_mvebu.c
> +++ b/drivers/pci/pci_mvebu.c
> @@ -13,7 +13,6 @@
>   #include <dm.h>
>   #include <log.h>
>   #include <malloc.h>
> -#include <asm/global_data.h>
>   #include <dm/device-internal.h>
>   #include <dm/lists.h>
>   #include <dm/of_access.h>
> @@ -28,8 +27,6 @@
>   #include <linux/mbus.h>
>   #include <linux/sizes.h>
>   
> -DECLARE_GLOBAL_DATA_PTR;
> -
>   /* PCIe unit register offsets */
>   #define SELECT(x, n)			((x >> n) & 1UL)
>   
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell 09/10] arm: a37xx: pci: Do not allow setting ROM BAR on PCI Bridge

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

PCI Bridge which represents aardvark PCIe Root Port has Expansion ROM Base
Address register at offset 0x30 but its meaning is different than PCI's
Expansion ROM BAR register. Only address format of register is same.

In reality, this device does not have any configurable PCI BARs. So ensure
that write operation into BARs (including Expansion ROM BAR) is noop and
registers always contain zero address which indicates that bars are
unsupported.

Fixes: cb056005dc67 ("arm: a37xx: pci: Add support for accessing PCI Bridge on root bus")
Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/pci-aardvark.c | 54 +++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
index 8abbc3ffe8..a92f00d0af 100644
--- a/drivers/pci/pci-aardvark.c
+++ b/drivers/pci/pci-aardvark.c
@@ -202,7 +202,7 @@ struct pcie_advk {
 	int			sec_busno;
 	struct udevice		*dev;
 	struct gpio_desc	reset_gpio;
-	u32			cfgcache[(0x34 - 0x10) / 4];
+	u32			cfgcache[(0x3c - 0x10) / 4];
 	bool			cfgcrssve;
 };
 
@@ -389,20 +389,19 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
 	}
 
 	/*
-	 * The configuration space of the PCI Bridge on primary (local) bus is
+	 * The configuration space of the PCI Bridge on primary (first) bus is
 	 * not accessible via PIO transfers like all other PCIe devices. PCI
 	 * Bridge config registers are available directly in Aardvark memory
-	 * space starting at offset zero. Moreover PCI Bridge registers in the
-	 * range 0x10 - 0x34 are not available and register 0x38 (Expansion ROM
-	 * Base Address) is at offset 0x30.
-	 * We therefore read configuration space content of the primary PCI
-	 * Bridge from our virtual cache.
+	 * space starting at offset zero. The PCI Bridge config space is of
+	 * Type 0, but the BAR registers (including ROM BAR) don't have the same
+	 * meaning as in the PCIe specification. Therefore do not access BAR
+	 * registers and non-common registers (those which have different
+	 * meaning for Type 0 and Type 1 config space) of the primary PCI Bridge
+	 * and instead read their content from driver virtual cfgcache[].
 	 */
 	if (busno == pcie->first_busno) {
-		if (offset >= 0x10 && offset < 0x34)
+		if ((offset >= 0x10 && offset < 0x34) || (offset >= 0x38 && offset < 0x3c))
 			data = pcie->cfgcache[(offset - 0x10) / 4];
-		else if ((offset & ~3) == PCI_ROM_ADDRESS1)
-			data = advk_readl(pcie, PCIE_CORE_EXP_ROM_BAR_REG);
 		else
 			data = advk_readl(pcie, offset & ~3);
 
@@ -576,23 +575,22 @@ static int pcie_advk_write_config(struct udevice *bus, pci_dev_t bdf,
 	}
 
 	/*
-	 * As explained in pcie_advk_read_config(), for the configuration
-	 * space of the primary PCI Bridge, we write the content into virtual
-	 * cache.
+	 * As explained in pcie_advk_read_config(), PCI Bridge config registers
+	 * are available directly in Aardvark memory space starting at offset
+	 * zero. Type 1 specific registers are not available, so we write their
+	 * content only into driver virtual cfgcache[].
 	 */
 	if (busno == pcie->first_busno) {
-		if (offset >= 0x10 && offset < 0x34) {
+		if ((offset >= 0x10 && offset < 0x34) ||
+		    (offset >= 0x38 && offset < 0x3c)) {
 			data = pcie->cfgcache[(offset - 0x10) / 4];
 			data = pci_conv_size_to_32(data, value, offset, size);
 			/* This PCI bridge does not have configurable bars */
 			if ((offset & ~3) == PCI_BASE_ADDRESS_0 ||
-			    (offset & ~3) == PCI_BASE_ADDRESS_1)
+			    (offset & ~3) == PCI_BASE_ADDRESS_1 ||
+			    (offset & ~3) == PCI_ROM_ADDRESS1)
 				data = 0x0;
 			pcie->cfgcache[(offset - 0x10) / 4] = data;
-		} else if ((offset & ~3) == PCI_ROM_ADDRESS1) {
-			data = advk_readl(pcie, PCIE_CORE_EXP_ROM_BAR_REG);
-			data = pci_conv_size_to_32(data, value, offset, size);
-			advk_writel(pcie, data, PCIE_CORE_EXP_ROM_BAR_REG);
 		} else {
 			data = advk_readl(pcie, offset & ~3);
 			data = pci_conv_size_to_32(data, value, offset, size);
@@ -830,12 +828,20 @@ static int pcie_advk_setup_hw(struct pcie_advk *pcie)
 	 *
 	 * Note that this Aardvark PCI Bridge does not have a compliant Type 1
 	 * Configuration Space and it even cannot be accessed via Aardvark's
-	 * PCI config space access method. Something like config space is
+	 * PCI config space access method. Aardvark PCI Bridge Config space is
 	 * available in internal Aardvark registers starting at offset 0x0
-	 * and is reported as Type 0. In range 0x10 - 0x34 it has totally
-	 * different registers. So our driver reports Header Type as Type 1 and
-	 * for the above mentioned range redirects access to the virtual
-	 * cfgcache[] buffer, which avoids changing internal Aardvark registers.
+	 * and has format of Type 0 config space.
+	 *
+	 * Moreover Type 0 BAR registers (ranges 0x10 - 0x28 and 0x30 - 0x34)
+	 * have the same format in Marvell's specification as in PCIe
+	 * specification, but their meaning is totally different (and not even
+	 * the same meaning as explained in the corresponding comment in the
+	 * pci_mvebu driver; aardvark is still different).
+	 *
+	 * So our driver converts Type 0 config space to Type 1 and reports
+	 * Header Type as Type 1. Access to BAR registers and to non-existent
+	 * Type 1 registers is redirected to the virtual cfgcache[] buffer,
+	 * which avoids changing unrelated registers.
 	 */
 	reg = advk_readl(pcie, PCIE_CORE_DEV_REV_REG);
 	reg &= ~0xffffff00;
-- 
2.32.0

Re: [PATCH u-boot-marvell 09/10] arm: a37xx: pci: Do not allow setting ROM BAR on PCI Bridge

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> PCI Bridge which represents aardvark PCIe Root Port has Expansion ROM Base
> Address register at offset 0x30 but its meaning is different than PCI's
> Expansion ROM BAR register. Only address format of register is same.
> 
> In reality, this device does not have any configurable PCI BARs. So ensure
> that write operation into BARs (including Expansion ROM BAR) is noop and
> registers always contain zero address which indicates that bars are
> unsupported.
> 
> Fixes: cb056005dc67 ("arm: a37xx: pci: Add support for accessing PCI Bridge on root bus")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/pci/pci-aardvark.c | 54 +++++++++++++++++++++-----------------
>   1 file changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
> index 8abbc3ffe8..a92f00d0af 100644
> --- a/drivers/pci/pci-aardvark.c
> +++ b/drivers/pci/pci-aardvark.c
> @@ -202,7 +202,7 @@ struct pcie_advk {
>   	int			sec_busno;
>   	struct udevice		*dev;
>   	struct gpio_desc	reset_gpio;
> -	u32			cfgcache[(0x34 - 0x10) / 4];
> +	u32			cfgcache[(0x3c - 0x10) / 4];
>   	bool			cfgcrssve;
>   };
>   
> @@ -389,20 +389,19 @@ static int pcie_advk_read_config(const struct udevice *bus, pci_dev_t bdf,
>   	}
>   
>   	/*
> -	 * The configuration space of the PCI Bridge on primary (local) bus is
> +	 * The configuration space of the PCI Bridge on primary (first) bus is
>   	 * not accessible via PIO transfers like all other PCIe devices. PCI
>   	 * Bridge config registers are available directly in Aardvark memory
> -	 * space starting at offset zero. Moreover PCI Bridge registers in the
> -	 * range 0x10 - 0x34 are not available and register 0x38 (Expansion ROM
> -	 * Base Address) is at offset 0x30.
> -	 * We therefore read configuration space content of the primary PCI
> -	 * Bridge from our virtual cache.
> +	 * space starting at offset zero. The PCI Bridge config space is of
> +	 * Type 0, but the BAR registers (including ROM BAR) don't have the same
> +	 * meaning as in the PCIe specification. Therefore do not access BAR
> +	 * registers and non-common registers (those which have different
> +	 * meaning for Type 0 and Type 1 config space) of the primary PCI Bridge
> +	 * and instead read their content from driver virtual cfgcache[].
>   	 */
>   	if (busno == pcie->first_busno) {
> -		if (offset >= 0x10 && offset < 0x34)
> +		if ((offset >= 0x10 && offset < 0x34) || (offset >= 0x38 && offset < 0x3c))
>   			data = pcie->cfgcache[(offset - 0x10) / 4];
> -		else if ((offset & ~3) == PCI_ROM_ADDRESS1)
> -			data = advk_readl(pcie, PCIE_CORE_EXP_ROM_BAR_REG);
>   		else
>   			data = advk_readl(pcie, offset & ~3);
>   
> @@ -576,23 +575,22 @@ static int pcie_advk_write_config(struct udevice *bus, pci_dev_t bdf,
>   	}
>   
>   	/*
> -	 * As explained in pcie_advk_read_config(), for the configuration
> -	 * space of the primary PCI Bridge, we write the content into virtual
> -	 * cache.
> +	 * As explained in pcie_advk_read_config(), PCI Bridge config registers
> +	 * are available directly in Aardvark memory space starting at offset
> +	 * zero. Type 1 specific registers are not available, so we write their
> +	 * content only into driver virtual cfgcache[].
>   	 */
>   	if (busno == pcie->first_busno) {
> -		if (offset >= 0x10 && offset < 0x34) {
> +		if ((offset >= 0x10 && offset < 0x34) ||
> +		    (offset >= 0x38 && offset < 0x3c)) {
>   			data = pcie->cfgcache[(offset - 0x10) / 4];
>   			data = pci_conv_size_to_32(data, value, offset, size);
>   			/* This PCI bridge does not have configurable bars */
>   			if ((offset & ~3) == PCI_BASE_ADDRESS_0 ||
> -			    (offset & ~3) == PCI_BASE_ADDRESS_1)
> +			    (offset & ~3) == PCI_BASE_ADDRESS_1 ||
> +			    (offset & ~3) == PCI_ROM_ADDRESS1)
>   				data = 0x0;
>   			pcie->cfgcache[(offset - 0x10) / 4] = data;
> -		} else if ((offset & ~3) == PCI_ROM_ADDRESS1) {
> -			data = advk_readl(pcie, PCIE_CORE_EXP_ROM_BAR_REG);
> -			data = pci_conv_size_to_32(data, value, offset, size);
> -			advk_writel(pcie, data, PCIE_CORE_EXP_ROM_BAR_REG);
>   		} else {
>   			data = advk_readl(pcie, offset & ~3);
>   			data = pci_conv_size_to_32(data, value, offset, size);
> @@ -830,12 +828,20 @@ static int pcie_advk_setup_hw(struct pcie_advk *pcie)
>   	 *
>   	 * Note that this Aardvark PCI Bridge does not have a compliant Type 1
>   	 * Configuration Space and it even cannot be accessed via Aardvark's
> -	 * PCI config space access method. Something like config space is
> +	 * PCI config space access method. Aardvark PCI Bridge Config space is
>   	 * available in internal Aardvark registers starting at offset 0x0
> -	 * and is reported as Type 0. In range 0x10 - 0x34 it has totally
> -	 * different registers. So our driver reports Header Type as Type 1 and
> -	 * for the above mentioned range redirects access to the virtual
> -	 * cfgcache[] buffer, which avoids changing internal Aardvark registers.
> +	 * and has format of Type 0 config space.
> +	 *
> +	 * Moreover Type 0 BAR registers (ranges 0x10 - 0x28 and 0x30 - 0x34)
> +	 * have the same format in Marvell's specification as in PCIe
> +	 * specification, but their meaning is totally different (and not even
> +	 * the same meaning as explained in the corresponding comment in the
> +	 * pci_mvebu driver; aardvark is still different).
> +	 *
> +	 * So our driver converts Type 0 config space to Type 1 and reports
> +	 * Header Type as Type 1. Access to BAR registers and to non-existent
> +	 * Type 1 registers is redirected to the virtual cfgcache[] buffer,
> +	 * which avoids changing unrelated registers.
>   	 */
>   	reg = advk_readl(pcie, PCIE_CORE_DEV_REV_REG);
>   	reg &= ~0xffffff00;
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell 10/10] arm: mvebu: turris_mox: Remove extra newline after module topology

[Marek Behún]

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

Remove extra newline after module topology is printed.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 board/CZ.NIC/turris_mox/turris_mox.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/board/CZ.NIC/turris_mox/turris_mox.c b/board/CZ.NIC/turris_mox/turris_mox.c
index 2202eb8cfb..52fa77d68c 100644
--- a/board/CZ.NIC/turris_mox/turris_mox.c
+++ b/board/CZ.NIC/turris_mox/turris_mox.c
@@ -607,9 +607,6 @@ int show_board_info(void)
 		}
 	}
 
-	if (module_count)
-		printf("\n");
-
 	return 0;
 }
 
-- 
2.32.0

Re: [PATCH u-boot-marvell 10/10] arm: mvebu: turris_mox: Remove extra newline after module topology

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Marek Behún <marek.behun@nic.cz>
> 
> Remove extra newline after module topology is printed.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Not really related to this series, but still:

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan


> ---
>   board/CZ.NIC/turris_mox/turris_mox.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/board/CZ.NIC/turris_mox/turris_mox.c b/board/CZ.NIC/turris_mox/turris_mox.c
> index 2202eb8cfb..52fa77d68c 100644
> --- a/board/CZ.NIC/turris_mox/turris_mox.c
> +++ b/board/CZ.NIC/turris_mox/turris_mox.c
> @@ -607,9 +607,6 @@ int show_board_info(void)
>   		}
>   	}
>   
> -	if (module_count)
> -		printf("\n");
> -
>   	return 0;
>   }
>   
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 10/10] arm: mvebu: turris_mox: Remove extra newline after module topology

[Stefan Roese]

On 11/11/21 16:35, Marek Behún wrote:
> From: Marek Behún <marek.behun@nic.cz>
> 
> Remove extra newline after module topology is printed.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Applied to u-boot-marvell/next

Thanks,
Stefan

> ---
>   board/CZ.NIC/turris_mox/turris_mox.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/board/CZ.NIC/turris_mox/turris_mox.c b/board/CZ.NIC/turris_mox/turris_mox.c
> index 2202eb8cfb..52fa77d68c 100644
> --- a/board/CZ.NIC/turris_mox/turris_mox.c
> +++ b/board/CZ.NIC/turris_mox/turris_mox.c
> @@ -607,9 +607,6 @@ int show_board_info(void)
>   		}
>   	}
>   
> -	if (module_count)
> -		printf("\n");
> -
>   	return 0;
>   }
>   
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 00/10] PCI mvebu and aardvark changes

[Pali Rohár]

On Thursday 11 November 2021 16:35:39 Marek Behún wrote:
> From: Marek Behún <marek.behun@nic.cz>
> 
> Hello Stefan,
> 
> we have some more fixes for PCI (mvebu and aardvark), and one patch
> for Turris MOX board code.
> 
> Marek

Hello Stefan, patches 1, 2 and 7 still needs some modifications. But
other remaining patches should be OK. Would you merge remaining patches?
And then we can focus on issues with link initialization.

> Marek Behún (2):
>   pci: pci_mvebu, pci_aardvark: Fix size of configuration cache
>   arm: mvebu: turris_mox: Remove extra newline after module topology
> 
> Pali Rohár (8):
>   pci: pci_mvebu: Wait 100ms for Link Up in mvebu_pcie_probe()
>   arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c
>   pci: pci_mvebu: Move setup for BAR[0] where other BARs are setup
>   pci: pci_mvebu: Replace MBUS_PCI_*_SIZE by resource_size()
>   pci: pci_mvebu: Do not allow setting ROM BAR on PCI Bridge
>   pci: pci_mvebu: Fix PCIe MEM and IO resources assignment and mbus
>     mapping
>   pci: pci_mvebu: Remove unused DECLARE_GLOBAL_DATA_PTR
>   arm: a37xx: pci: Do not allow setting ROM BAR on PCI Bridge
> 
>  arch/arm/mach-mvebu/include/mach/cpu.h        |   4 +-
>  arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h    |   4 -
>  .../serdes/a38x/high_speed_env_spec.c         |  15 --
>  board/CZ.NIC/turris_mox/turris_mox.c          |   3 -
>  drivers/pci/pci-aardvark.c                    |  54 +++--
>  drivers/pci/pci_mvebu.c                       | 205 ++++++++++++------
>  6 files changed, 175 insertions(+), 110 deletions(-)
> 
> -- 
> 2.32.0
> 

Re: [PATCH u-boot-marvell 00/10] PCI mvebu and aardvark changes

[Stefan Roese]

Hi Pali,

On 12/12/21 12:23, Pali Rohár wrote:
> On Thursday 11 November 2021 16:35:39 Marek Behún wrote:
>> From: Marek Behún <marek.behun@nic.cz>
>>
>> Hello Stefan,
>>
>> we have some more fixes for PCI (mvebu and aardvark), and one patch
>> for Turris MOX board code.
>>
>> Marek
> 
> Hello Stefan, patches 1, 2 and 7 still needs some modifications. But
> other remaining patches should be OK. Would you merge remaining patches?
> And then we can focus on issues with link initialization.

Sure, I can pull the "remaining patches" into next (for master it's too
late in the release cycle) if this helps. Just let me know and I'll go
through the patch queue in the next days to push more Marvell related
patches into next.

Thanks,
Stefan

>> Marek Behún (2):
>>    pci: pci_mvebu, pci_aardvark: Fix size of configuration cache
>>    arm: mvebu: turris_mox: Remove extra newline after module topology
>>
>> Pali Rohár (8):
>>    pci: pci_mvebu: Wait 100ms for Link Up in mvebu_pcie_probe()
>>    arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c
>>    pci: pci_mvebu: Move setup for BAR[0] where other BARs are setup
>>    pci: pci_mvebu: Replace MBUS_PCI_*_SIZE by resource_size()
>>    pci: pci_mvebu: Do not allow setting ROM BAR on PCI Bridge
>>    pci: pci_mvebu: Fix PCIe MEM and IO resources assignment and mbus
>>      mapping
>>    pci: pci_mvebu: Remove unused DECLARE_GLOBAL_DATA_PTR
>>    arm: a37xx: pci: Do not allow setting ROM BAR on PCI Bridge
>>
>>   arch/arm/mach-mvebu/include/mach/cpu.h        |   4 +-
>>   arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h    |   4 -
>>   .../serdes/a38x/high_speed_env_spec.c         |  15 --
>>   board/CZ.NIC/turris_mox/turris_mox.c          |   3 -
>>   drivers/pci/pci-aardvark.c                    |  54 +++--
>>   drivers/pci/pci_mvebu.c                       | 205 ++++++++++++------
>>   6 files changed, 175 insertions(+), 110 deletions(-)
>>
>> -- 
>> 2.32.0
>>

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 00/10] PCI mvebu and aardvark changes

[Pali Rohár]

On Monday 13 December 2021 08:41:30 Stefan Roese wrote:
> Hi Pali,
> 
> On 12/12/21 12:23, Pali Rohár wrote:
> > On Thursday 11 November 2021 16:35:39 Marek Behún wrote:
> > > From: Marek Behún <marek.behun@nic.cz>
> > > 
> > > Hello Stefan,
> > > 
> > > we have some more fixes for PCI (mvebu and aardvark), and one patch
> > > for Turris MOX board code.
> > > 
> > > Marek
> > 
> > Hello Stefan, patches 1, 2 and 7 still needs some modifications. But
> > other remaining patches should be OK. Would you merge remaining patches?
> > And then we can focus on issues with link initialization.
> 
> Sure, I can pull the "remaining patches" into next (for master it's too
> late in the release cycle) if this helps. Just let me know and I'll go
> through the patch queue in the next days to push more Marvell related
> patches into next.

Ok. But please consider applying patches 5, 6 and 9 to master as they
have Fixes tag for commits which are already in master branch.

> Thanks,
> Stefan
> 
> > > Marek Behún (2):
> > >    pci: pci_mvebu, pci_aardvark: Fix size of configuration cache
> > >    arm: mvebu: turris_mox: Remove extra newline after module topology
> > > 
> > > Pali Rohár (8):
> > >    pci: pci_mvebu: Wait 100ms for Link Up in mvebu_pcie_probe()
> > >    arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c
> > >    pci: pci_mvebu: Move setup for BAR[0] where other BARs are setup
> > >    pci: pci_mvebu: Replace MBUS_PCI_*_SIZE by resource_size()
> > >    pci: pci_mvebu: Do not allow setting ROM BAR on PCI Bridge
> > >    pci: pci_mvebu: Fix PCIe MEM and IO resources assignment and mbus
> > >      mapping
> > >    pci: pci_mvebu: Remove unused DECLARE_GLOBAL_DATA_PTR
> > >    arm: a37xx: pci: Do not allow setting ROM BAR on PCI Bridge
> > > 
> > >   arch/arm/mach-mvebu/include/mach/cpu.h        |   4 +-
> > >   arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h    |   4 -
> > >   .../serdes/a38x/high_speed_env_spec.c         |  15 --
> > >   board/CZ.NIC/turris_mox/turris_mox.c          |   3 -
> > >   drivers/pci/pci-aardvark.c                    |  54 +++--
> > >   drivers/pci/pci_mvebu.c                       | 205 ++++++++++++------
> > >   6 files changed, 175 insertions(+), 110 deletions(-)
> > > 
> > > -- 
> > > 2.32.0
> > > 
> 
> Viele Grüße,
> Stefan Roese
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 00/10] PCI mvebu and aardvark changes

[Stefan Roese]

Hi Pali,

On 12/13/21 11:27, Pali Rohár wrote:
> On Monday 13 December 2021 08:41:30 Stefan Roese wrote:
>> Hi Pali,
>>
>> On 12/12/21 12:23, Pali Rohár wrote:
>>> On Thursday 11 November 2021 16:35:39 Marek Behún wrote:
>>>> From: Marek Behún <marek.behun@nic.cz>
>>>>
>>>> Hello Stefan,
>>>>
>>>> we have some more fixes for PCI (mvebu and aardvark), and one patch
>>>> for Turris MOX board code.
>>>>
>>>> Marek
>>>
>>> Hello Stefan, patches 1, 2 and 7 still needs some modifications. But
>>> other remaining patches should be OK. Would you merge remaining patches?
>>> And then we can focus on issues with link initialization.
>>
>> Sure, I can pull the "remaining patches" into next (for master it's too
>> late in the release cycle) if this helps. Just let me know and I'll go
>> through the patch queue in the next days to push more Marvell related
>> patches into next.
> 
> Ok. But please consider applying patches 5, 6 and 9 to master as they
> have Fixes tag for commits which are already in master branch.

Okay, thanks for the reminder. The Azure world build is running. So I'll
send the pull request for these 3 patches most likely later today.

Thanks,
Stefan


>> Thanks,
>> Stefan
>>
>>>> Marek Behún (2):
>>>>     pci: pci_mvebu, pci_aardvark: Fix size of configuration cache
>>>>     arm: mvebu: turris_mox: Remove extra newline after module topology
>>>>
>>>> Pali Rohár (8):
>>>>     pci: pci_mvebu: Wait 100ms for Link Up in mvebu_pcie_probe()
>>>>     arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c
>>>>     pci: pci_mvebu: Move setup for BAR[0] where other BARs are setup
>>>>     pci: pci_mvebu: Replace MBUS_PCI_*_SIZE by resource_size()
>>>>     pci: pci_mvebu: Do not allow setting ROM BAR on PCI Bridge
>>>>     pci: pci_mvebu: Fix PCIe MEM and IO resources assignment and mbus
>>>>       mapping
>>>>     pci: pci_mvebu: Remove unused DECLARE_GLOBAL_DATA_PTR
>>>>     arm: a37xx: pci: Do not allow setting ROM BAR on PCI Bridge
>>>>
>>>>    arch/arm/mach-mvebu/include/mach/cpu.h        |   4 +-
>>>>    arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h    |   4 -
>>>>    .../serdes/a38x/high_speed_env_spec.c         |  15 --
>>>>    board/CZ.NIC/turris_mox/turris_mox.c          |   3 -
>>>>    drivers/pci/pci-aardvark.c                    |  54 +++--
>>>>    drivers/pci/pci_mvebu.c                       | 205 ++++++++++++------
>>>>    6 files changed, 175 insertions(+), 110 deletions(-)
>>>>
>>>> -- 
>>>> 2.32.0
>>>>
>>
>> Viele Grüße,
>> Stefan Roese
>>
>> -- 
>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 00/10] PCI mvebu and aardvark changes

[Pali Rohár]

On Wednesday 15 December 2021 09:10:50 Stefan Roese wrote:
> Hi Pali,
> 
> On 12/13/21 11:27, Pali Rohár wrote:
> > On Monday 13 December 2021 08:41:30 Stefan Roese wrote:
> > > Hi Pali,
> > > 
> > > On 12/12/21 12:23, Pali Rohár wrote:
> > > > On Thursday 11 November 2021 16:35:39 Marek Behún wrote:
> > > > > From: Marek Behún <marek.behun@nic.cz>
> > > > > 
> > > > > Hello Stefan,
> > > > > 
> > > > > we have some more fixes for PCI (mvebu and aardvark), and one patch
> > > > > for Turris MOX board code.
> > > > > 
> > > > > Marek
> > > > 
> > > > Hello Stefan, patches 1, 2 and 7 still needs some modifications. But
> > > > other remaining patches should be OK. Would you merge remaining patches?
> > > > And then we can focus on issues with link initialization.
> > > 
> > > Sure, I can pull the "remaining patches" into next (for master it's too
> > > late in the release cycle) if this helps. Just let me know and I'll go
> > > through the patch queue in the next days to push more Marvell related
> > > patches into next.
> > 
> > Ok. But please consider applying patches 5, 6 and 9 to master as they
> > have Fixes tag for commits which are already in master branch.
> 
> Okay, thanks for the reminder. The Azure world build is running. So I'll
> send the pull request for these 3 patches most likely later today.

Hello and thank you! Could you include patches prepared for next into
some branch, so I can prepare new patches on top of that branch to so
patches would be correctly rebased?

> Thanks,
> Stefan
> 
> 
> > > Thanks,
> > > Stefan
> > > 
> > > > > Marek Behún (2):
> > > > >     pci: pci_mvebu, pci_aardvark: Fix size of configuration cache
> > > > >     arm: mvebu: turris_mox: Remove extra newline after module topology
> > > > > 
> > > > > Pali Rohár (8):
> > > > >     pci: pci_mvebu: Wait 100ms for Link Up in mvebu_pcie_probe()
> > > > >     arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c
> > > > >     pci: pci_mvebu: Move setup for BAR[0] where other BARs are setup
> > > > >     pci: pci_mvebu: Replace MBUS_PCI_*_SIZE by resource_size()
> > > > >     pci: pci_mvebu: Do not allow setting ROM BAR on PCI Bridge
> > > > >     pci: pci_mvebu: Fix PCIe MEM and IO resources assignment and mbus
> > > > >       mapping
> > > > >     pci: pci_mvebu: Remove unused DECLARE_GLOBAL_DATA_PTR
> > > > >     arm: a37xx: pci: Do not allow setting ROM BAR on PCI Bridge
> > > > > 
> > > > >    arch/arm/mach-mvebu/include/mach/cpu.h        |   4 +-
> > > > >    arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h    |   4 -
> > > > >    .../serdes/a38x/high_speed_env_spec.c         |  15 --
> > > > >    board/CZ.NIC/turris_mox/turris_mox.c          |   3 -
> > > > >    drivers/pci/pci-aardvark.c                    |  54 +++--
> > > > >    drivers/pci/pci_mvebu.c                       | 205 ++++++++++++------
> > > > >    6 files changed, 175 insertions(+), 110 deletions(-)
> > > > > 
> > > > > -- 
> > > > > 2.32.0
> > > > > 
> > > 
> > > Viele Grüße,
> > > Stefan Roese
> > > 
> > > -- 
> > > DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> > > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> > > Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
> 
> Viele Grüße,
> Stefan Roese
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 00/10] PCI mvebu and aardvark changes

[Stefan Roese]

Hi Pali,

On 12/16/21 11:28, Pali Rohár wrote:
> On Wednesday 15 December 2021 09:10:50 Stefan Roese wrote:
>> Hi Pali,
>>
>> On 12/13/21 11:27, Pali Rohár wrote:
>>> On Monday 13 December 2021 08:41:30 Stefan Roese wrote:
>>>> Hi Pali,
>>>>
>>>> On 12/12/21 12:23, Pali Rohár wrote:
>>>>> On Thursday 11 November 2021 16:35:39 Marek Behún wrote:
>>>>>> From: Marek Behún <marek.behun@nic.cz>
>>>>>>
>>>>>> Hello Stefan,
>>>>>>
>>>>>> we have some more fixes for PCI (mvebu and aardvark), and one patch
>>>>>> for Turris MOX board code.
>>>>>>
>>>>>> Marek
>>>>>
>>>>> Hello Stefan, patches 1, 2 and 7 still needs some modifications. But
>>>>> other remaining patches should be OK. Would you merge remaining patches?
>>>>> And then we can focus on issues with link initialization.
>>>>
>>>> Sure, I can pull the "remaining patches" into next (for master it's too
>>>> late in the release cycle) if this helps. Just let me know and I'll go
>>>> through the patch queue in the next days to push more Marvell related
>>>> patches into next.
>>>
>>> Ok. But please consider applying patches 5, 6 and 9 to master as they
>>> have Fixes tag for commits which are already in master branch.
>>
>> Okay, thanks for the reminder. The Azure world build is running. So I'll
>> send the pull request for these 3 patches most likely later today.
> 
> Hello and thank you! Could you include patches prepared for next into
> some branch, so I can prepare new patches on top of that branch to so
> patches would be correctly rebased?

Done. As you will have noticed, I've pulled a bunch of patches into next
and did send a pull request just a few minutes ago.

Please let me know if I forgot something. Once all this lands in Tom's
next branch, this should be a good base for your rebase.

Thanks,
Stefan

>> Thanks,
>> Stefan
>>
>>
>>>> Thanks,
>>>> Stefan
>>>>
>>>>>> Marek Behún (2):
>>>>>>      pci: pci_mvebu, pci_aardvark: Fix size of configuration cache
>>>>>>      arm: mvebu: turris_mox: Remove extra newline after module topology
>>>>>>
>>>>>> Pali Rohár (8):
>>>>>>      pci: pci_mvebu: Wait 100ms for Link Up in mvebu_pcie_probe()
>>>>>>      arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c
>>>>>>      pci: pci_mvebu: Move setup for BAR[0] where other BARs are setup
>>>>>>      pci: pci_mvebu: Replace MBUS_PCI_*_SIZE by resource_size()
>>>>>>      pci: pci_mvebu: Do not allow setting ROM BAR on PCI Bridge
>>>>>>      pci: pci_mvebu: Fix PCIe MEM and IO resources assignment and mbus
>>>>>>        mapping
>>>>>>      pci: pci_mvebu: Remove unused DECLARE_GLOBAL_DATA_PTR
>>>>>>      arm: a37xx: pci: Do not allow setting ROM BAR on PCI Bridge
>>>>>>
>>>>>>     arch/arm/mach-mvebu/include/mach/cpu.h        |   4 +-
>>>>>>     arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h    |   4 -
>>>>>>     .../serdes/a38x/high_speed_env_spec.c         |  15 --
>>>>>>     board/CZ.NIC/turris_mox/turris_mox.c          |   3 -
>>>>>>     drivers/pci/pci-aardvark.c                    |  54 +++--
>>>>>>     drivers/pci/pci_mvebu.c                       | 205 ++++++++++++------
>>>>>>     6 files changed, 175 insertions(+), 110 deletions(-)
>>>>>>
>>>>>> -- 
>>>>>> 2.32.0
>>>>>>
>>>>
>>>> Viele Grüße,
>>>> Stefan Roese
>>>>
>>>> -- 
>>>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>>>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>>>> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
>>
>> Viele Grüße,
>> Stefan Roese
>>
>> -- 
>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 00/10] PCI mvebu and aardvark changes

[Pali Rohár]

On Saturday 18 December 2021 14:53:52 Stefan Roese wrote:
> Hi Pali,
> 
> On 12/16/21 11:28, Pali Rohár wrote:
> > On Wednesday 15 December 2021 09:10:50 Stefan Roese wrote:
> > > Hi Pali,
> > > 
> > > On 12/13/21 11:27, Pali Rohár wrote:
> > > > On Monday 13 December 2021 08:41:30 Stefan Roese wrote:
> > > > > Hi Pali,
> > > > > 
> > > > > On 12/12/21 12:23, Pali Rohár wrote:
> > > > > > On Thursday 11 November 2021 16:35:39 Marek Behún wrote:
> > > > > > > From: Marek Behún <marek.behun@nic.cz>
> > > > > > > 
> > > > > > > Hello Stefan,
> > > > > > > 
> > > > > > > we have some more fixes for PCI (mvebu and aardvark), and one patch
> > > > > > > for Turris MOX board code.
> > > > > > > 
> > > > > > > Marek
> > > > > > 
> > > > > > Hello Stefan, patches 1, 2 and 7 still needs some modifications. But
> > > > > > other remaining patches should be OK. Would you merge remaining patches?
> > > > > > And then we can focus on issues with link initialization.
> > > > > 
> > > > > Sure, I can pull the "remaining patches" into next (for master it's too
> > > > > late in the release cycle) if this helps. Just let me know and I'll go
> > > > > through the patch queue in the next days to push more Marvell related
> > > > > patches into next.
> > > > 
> > > > Ok. But please consider applying patches 5, 6 and 9 to master as they
> > > > have Fixes tag for commits which are already in master branch.
> > > 
> > > Okay, thanks for the reminder. The Azure world build is running. So I'll
> > > send the pull request for these 3 patches most likely later today.
> > 
> > Hello and thank you! Could you include patches prepared for next into
> > some branch, so I can prepare new patches on top of that branch to so
> > patches would be correctly rebased?
> 
> Done. As you will have noticed, I've pulled a bunch of patches into next
> and did send a pull request just a few minutes ago.

Perfect!

> Please let me know if I forgot something. Once all this lands in Tom's
> next branch, this should be a good base for your rebase.

Could you include into next branch also other patches from this patch
series which you have already reviewed, which are OK?

Patches 1, 2 and 7 are those which need modifications and are not ready
yet.

> Thanks,
> Stefan
> 
> > > Thanks,
> > > Stefan
> > > 
> > > 
> > > > > Thanks,
> > > > > Stefan
> > > > > 
> > > > > > > Marek Behún (2):
> > > > > > >      pci: pci_mvebu, pci_aardvark: Fix size of configuration cache
> > > > > > >      arm: mvebu: turris_mox: Remove extra newline after module topology
> > > > > > > 
> > > > > > > Pali Rohár (8):
> > > > > > >      pci: pci_mvebu: Wait 100ms for Link Up in mvebu_pcie_probe()
> > > > > > >      arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c
> > > > > > >      pci: pci_mvebu: Move setup for BAR[0] where other BARs are setup
> > > > > > >      pci: pci_mvebu: Replace MBUS_PCI_*_SIZE by resource_size()
> > > > > > >      pci: pci_mvebu: Do not allow setting ROM BAR on PCI Bridge
> > > > > > >      pci: pci_mvebu: Fix PCIe MEM and IO resources assignment and mbus
> > > > > > >        mapping
> > > > > > >      pci: pci_mvebu: Remove unused DECLARE_GLOBAL_DATA_PTR
> > > > > > >      arm: a37xx: pci: Do not allow setting ROM BAR on PCI Bridge
> > > > > > > 
> > > > > > >     arch/arm/mach-mvebu/include/mach/cpu.h        |   4 +-
> > > > > > >     arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h    |   4 -
> > > > > > >     .../serdes/a38x/high_speed_env_spec.c         |  15 --
> > > > > > >     board/CZ.NIC/turris_mox/turris_mox.c          |   3 -
> > > > > > >     drivers/pci/pci-aardvark.c                    |  54 +++--
> > > > > > >     drivers/pci/pci_mvebu.c                       | 205 ++++++++++++------
> > > > > > >     6 files changed, 175 insertions(+), 110 deletions(-)
> > > > > > > 
> > > > > > > -- 
> > > > > > > 2.32.0
> > > > > > > 
> > > > > 
> > > > > Viele Grüße,
> > > > > Stefan Roese
> > > > > 
> > > > > -- 
> > > > > DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> > > > > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> > > > > Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
> > > 
> > > Viele Grüße,
> > > Stefan Roese
> > > 
> > > -- 
> > > DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> > > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> > > Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
> 
> Viele Grüße,
> Stefan Roese
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 00/10] PCI mvebu and aardvark changes

[Stefan Roese]

Hi Pali,

On 12/20/21 14:30, Pali Rohár wrote:
> On Saturday 18 December 2021 14:53:52 Stefan Roese wrote:
>> Hi Pali,
>>
>> On 12/16/21 11:28, Pali Rohár wrote:
>>> On Wednesday 15 December 2021 09:10:50 Stefan Roese wrote:
>>>> Hi Pali,
>>>>
>>>> On 12/13/21 11:27, Pali Rohár wrote:
>>>>> On Monday 13 December 2021 08:41:30 Stefan Roese wrote:
>>>>>> Hi Pali,
>>>>>>
>>>>>> On 12/12/21 12:23, Pali Rohár wrote:
>>>>>>> On Thursday 11 November 2021 16:35:39 Marek Behún wrote:
>>>>>>>> From: Marek Behún <marek.behun@nic.cz>
>>>>>>>>
>>>>>>>> Hello Stefan,
>>>>>>>>
>>>>>>>> we have some more fixes for PCI (mvebu and aardvark), and one patch
>>>>>>>> for Turris MOX board code.
>>>>>>>>
>>>>>>>> Marek
>>>>>>>
>>>>>>> Hello Stefan, patches 1, 2 and 7 still needs some modifications. But
>>>>>>> other remaining patches should be OK. Would you merge remaining patches?
>>>>>>> And then we can focus on issues with link initialization.
>>>>>>
>>>>>> Sure, I can pull the "remaining patches" into next (for master it's too
>>>>>> late in the release cycle) if this helps. Just let me know and I'll go
>>>>>> through the patch queue in the next days to push more Marvell related
>>>>>> patches into next.
>>>>>
>>>>> Ok. But please consider applying patches 5, 6 and 9 to master as they
>>>>> have Fixes tag for commits which are already in master branch.
>>>>
>>>> Okay, thanks for the reminder. The Azure world build is running. So I'll
>>>> send the pull request for these 3 patches most likely later today.
>>>
>>> Hello and thank you! Could you include patches prepared for next into
>>> some branch, so I can prepare new patches on top of that branch to so
>>> patches would be correctly rebased?
>>
>> Done. As you will have noticed, I've pulled a bunch of patches into next
>> and did send a pull request just a few minutes ago.
> 
> Perfect!
> 
>> Please let me know if I forgot something. Once all this lands in Tom's
>> next branch, this should be a good base for your rebase.
> 
> Could you include into next branch also other patches from this patch
> series which you have already reviewed, which are OK?

Sure, if this helps.

> Patches 1, 2 and 7 are those which need modifications and are not ready
> yet.

Thanks. The other patches are "on the way" to next.

Thanks,
Stefan

>> Thanks,
>> Stefan
>>
>>>> Thanks,
>>>> Stefan
>>>>
>>>>
>>>>>> Thanks,
>>>>>> Stefan
>>>>>>
>>>>>>>> Marek Behún (2):
>>>>>>>>       pci: pci_mvebu, pci_aardvark: Fix size of configuration cache
>>>>>>>>       arm: mvebu: turris_mox: Remove extra newline after module topology
>>>>>>>>
>>>>>>>> Pali Rohár (8):
>>>>>>>>       pci: pci_mvebu: Wait 100ms for Link Up in mvebu_pcie_probe()
>>>>>>>>       arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c
>>>>>>>>       pci: pci_mvebu: Move setup for BAR[0] where other BARs are setup
>>>>>>>>       pci: pci_mvebu: Replace MBUS_PCI_*_SIZE by resource_size()
>>>>>>>>       pci: pci_mvebu: Do not allow setting ROM BAR on PCI Bridge
>>>>>>>>       pci: pci_mvebu: Fix PCIe MEM and IO resources assignment and mbus
>>>>>>>>         mapping
>>>>>>>>       pci: pci_mvebu: Remove unused DECLARE_GLOBAL_DATA_PTR
>>>>>>>>       arm: a37xx: pci: Do not allow setting ROM BAR on PCI Bridge
>>>>>>>>
>>>>>>>>      arch/arm/mach-mvebu/include/mach/cpu.h        |   4 +-
>>>>>>>>      arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h    |   4 -
>>>>>>>>      .../serdes/a38x/high_speed_env_spec.c         |  15 --
>>>>>>>>      board/CZ.NIC/turris_mox/turris_mox.c          |   3 -
>>>>>>>>      drivers/pci/pci-aardvark.c                    |  54 +++--
>>>>>>>>      drivers/pci/pci_mvebu.c                       | 205 ++++++++++++------
>>>>>>>>      6 files changed, 175 insertions(+), 110 deletions(-)
>>>>>>>>
>>>>>>>> -- 
>>>>>>>> 2.32.0
>>>>>>>>
>>>>>>
>>>>>> Viele Grüße,
>>>>>> Stefan Roese
>>>>>>
>>>>>> -- 
>>>>>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>>>>>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>>>>>> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
>>>>
>>>> Viele Grüße,
>>>> Stefan Roese
>>>>
>>>> -- 
>>>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>>>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>>>> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
>>
>> Viele Grüße,
>> Stefan Roese
>>
>> -- 
>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 00/10] PCI mvebu and aardvark changes

[Pali Rohár]

On Tuesday 21 December 2021 09:19:12 Stefan Roese wrote:
> Hi Pali,
> 
> On 12/20/21 14:30, Pali Rohár wrote:
> > On Saturday 18 December 2021 14:53:52 Stefan Roese wrote:
> > > Hi Pali,
> > > 
> > > On 12/16/21 11:28, Pali Rohár wrote:
> > > > On Wednesday 15 December 2021 09:10:50 Stefan Roese wrote:
> > > > > Hi Pali,
> > > > > 
> > > > > On 12/13/21 11:27, Pali Rohár wrote:
> > > > > > On Monday 13 December 2021 08:41:30 Stefan Roese wrote:
> > > > > > > Hi Pali,
> > > > > > > 
> > > > > > > On 12/12/21 12:23, Pali Rohár wrote:
> > > > > > > > On Thursday 11 November 2021 16:35:39 Marek Behún wrote:
> > > > > > > > > From: Marek Behún <marek.behun@nic.cz>
> > > > > > > > > 
> > > > > > > > > Hello Stefan,
> > > > > > > > > 
> > > > > > > > > we have some more fixes for PCI (mvebu and aardvark), and one patch
> > > > > > > > > for Turris MOX board code.
> > > > > > > > > 
> > > > > > > > > Marek
> > > > > > > > 
> > > > > > > > Hello Stefan, patches 1, 2 and 7 still needs some modifications. But
> > > > > > > > other remaining patches should be OK. Would you merge remaining patches?
> > > > > > > > And then we can focus on issues with link initialization.
> > > > > > > 
> > > > > > > Sure, I can pull the "remaining patches" into next (for master it's too
> > > > > > > late in the release cycle) if this helps. Just let me know and I'll go
> > > > > > > through the patch queue in the next days to push more Marvell related
> > > > > > > patches into next.
> > > > > > 
> > > > > > Ok. But please consider applying patches 5, 6 and 9 to master as they
> > > > > > have Fixes tag for commits which are already in master branch.
> > > > > 
> > > > > Okay, thanks for the reminder. The Azure world build is running. So I'll
> > > > > send the pull request for these 3 patches most likely later today.
> > > > 
> > > > Hello and thank you! Could you include patches prepared for next into
> > > > some branch, so I can prepare new patches on top of that branch to so
> > > > patches would be correctly rebased?
> > > 
> > > Done. As you will have noticed, I've pulled a bunch of patches into next
> > > and did send a pull request just a few minutes ago.
> > 
> > Perfect!
> > 
> > > Please let me know if I forgot something. Once all this lands in Tom's
> > > next branch, this should be a good base for your rebase.
> > 
> > Could you include into next branch also other patches from this patch
> > series which you have already reviewed, which are OK?
> 
> Sure, if this helps.
> 
> > Patches 1, 2 and 7 are those which need modifications and are not ready
> > yet.
> 
> Thanks. The other patches are "on the way" to next.

Ok! I will send new pci mvebu patches rebased on top of the
u-boot-marvell/next branch.