[PATCH u-boot-marvell 00/10] PCI mvebu and aardvark changes
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(-)
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; }
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
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
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
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;
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 */
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
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
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
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
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
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.
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
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/d...
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
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/d...
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
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/d...
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
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/d...
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:
- pci-mvebu.c configures all controller registers to correct values
- PCIe port is enabled via SoC-specific register
- 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
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/d...
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:
- pci-mvebu.c configures all controller registers to correct values
- PCIe port is enabled via SoC-specific register
- 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
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
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.
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/d...
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:
- pci-mvebu.c configures all controller registers to correct values
- PCIe port is enabled via SoC-specific register
- 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
Hi Pali,
On 12/10/21 15:23, Pali Rohár wrote:
<snip>
So I think the correct behavior should be:
- pci-mvebu.c configures all controller registers to correct values
- PCIe port is enabled via SoC-specific register
- 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
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:
- pci-mvebu.c configures all controller registers to correct values
- PCIe port is enabled via SoC-specific register
- 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
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);
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
*/ static void mvebu_pcie_setup_wins(struct mvebu_pcie *pcie)
- BAR[2] -> disabled
- WIN[0-3] -> DRAM bank[0-3]
@@ -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
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
*/ static void mvebu_pcie_setup_wins(struct mvebu_pcie *pcie)
- BAR[2] -> disabled
- WIN[0-3] -> DRAM bank[0-3]
@@ -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
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 */
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))) {
(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))) {
(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);
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
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))) {
(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))) {
(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);
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
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]; };
/*
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
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
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;
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))) {
} 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))) {
(offset & ~3) == PCI_BASE_ADDRESS_1)
(offset & ~3) == PCI_BASE_ADDRESS_1 ||(offset & ~3) == PCI_ROM_ADDRESS1) data = 0x0;@@ -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.
Viele Grüße, 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))) {
} 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))) {
(offset & ~3) == PCI_BASE_ADDRESS_1)
(offset & ~3) == PCI_BASE_ADDRESS_1 ||(offset & ~3) == PCI_ROM_ADDRESS1) data = 0x0;@@ -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.
Viele Grüße, Stefan Roese
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);
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))) {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
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))) {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))) {pcie->io.start = 0;pcie->io.end = -1;@@ -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
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;@@ -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
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)
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
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
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;
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 (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);@@ -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 (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);@@ -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.
Viele Grüße, Stefan Roese
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; }
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
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
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
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
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
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
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
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
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
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
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.
participants (3)
-
Marek Behún -
Pali Rohár -
Stefan Roese