| Message ID | 20211111153549.29111-3-kabel@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | PCI mvebu and aardvark changes | expand |
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/dam/marvell/en/public-collateral/embedded-processors/marvell-embedded-processors-armada-38x-functional-specifications-2015-11.pdf 17.3 Link Initialization In case the initialization fails and no link is established, the PHY will keep on trying to initiate a link forever unless the port is disabled. As long as the port is enabled, the PHY will continue trying to establish a link; once the PHY identifies that a device is connected to it, a link will be established. PCIe port is enabled by bits in SoC Control 1 Register, which is done in U-Boot SerDes initialization code. This is IIRC SoC specific, and reason why every Armada SoC has own SerDes init code. And looks like that due to "the PHY will keep on trying to initiate a link forever", the PCIe link comes up when pci-mvebu.c sets all required registers to correct values. E.g. set correct mode (RC vs endpoint), link width (x1 vs x4), etc... > Could you perhaps try to remove some of the register configurations in > the MVEBU PCIe driver to see, if the link establishment relies on this > register to be written to (e.g. PCI_EXP_LNKCAP)? First port on A385 is by default set to X4 width, other ports to X1 width. Without updating LNKCAP to correct width, card in first PCIe port never initialize. And cards in other ports are initialized even before pci-mvebu.c starts configuration. So seems that this matches above behavior. SerDes init code enabled all PCIe ports. Ports which are using default configuration (second, third) are immediately initialized and link is established. Port which requires additional configuration (first port, for switching from X4 to X1) just stay in that "keep on trying to initiate a link forever" state until pci-mvebu starts and set PCI_EXP_LNKCAP register, after which PHY try X1 width and success. And seems that this is the reason why 100ms timeout is needed... As at this stage when pci-mvebu.c switches X4 to X1, init timeout as defined in PCIe spec (that 100ms) starts ticking. For other ports it starts ticking when serdes init code enables ports. I have looked into all PCIe registers which are present in functional spec, but it looks like that there is no pci-mvebu register which can turn of LTSSM and link training, like it is in other PCIe controllers. It looks like that only SoC-specific port enable bits are there. It is starting to be bigger mess as before... Any suggestion how to continue with it? We cannot (easily) move that code which flips PCIe bits in SoC Control 1 Register from SerDes init code to pci-mvebu.c as this is outside of pci-mvebu.c address space and also it is different on every SoC. pci-mvebu.c registers are same on all Marvell SoCs, starting from Orion up to the A39x. > Thanks, > Stefan
Hi Pali, On 11/29/21 12:47, Pali Rohár wrote: > Hello! > > On Monday 29 November 2021 10:22:58 Stefan Roese wrote: >> On 11/29/21 10:06, Pali Rohár wrote: >> >> <snip> >> >>>>> After this DTS change, pci-mvebu.c will just replace value of current >>>>> number of lanes (which is set to 4 by serdes code) to value from DTS, >>>>> which is 4. Therefore there should be no change. >>>>> >>>>> Could you test whole patch series with above DTS change if it works >>>>> properly on Theadorable board? >>>> >>>> Yes, I don't see any issues with this patchset applied plus this DT >>>> patch on theadorable. The PCIe links are up and with the correct width. >>>> >>>> What I'm wondering is, when exactly does the PCIe RP start the link >>>> establishment. In my case with AXP this is still in the AXP serdes code >>>> of course. But in the A38x code, it should be in the PCIe controller >>>> driver now AFAIU. I see that you configure the link width in the >>>> controller and do some other configuration (address windows etc), but >>>> at the end you "simply" wait for the link to come up via >>>> mvebu_pcie_wait_for_link(). I would have expected here some special >>>> command (config bit?) to the PCIe controller to start the link >>>> establishment. So when exactly does the A38x start this action? >>> >>> That is interesting question... While I'm reading it again, I really do >>> not know. Because you are right that mvebu_pcie_wait_for_link() is just >>> waiting for a link and it "magically" comes up. I have tested it on A385 >>> and it is stable with different Compex Atheros cards which caused issues >>> in past also on A3720. >> >> I would prefer, to fully understand when exactly the link establishment >> is started. Since this is crucial for the setup of the controller that >> needs to be done *before* the link starts to come up. > > I try to dig as more information as possible and finally I find out that > important information is available also in now removed, but originally > public A38x documentation. Thankfully web archive has copy of it: > > https://web.archive.org/web/20200420191927/https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-embedded-processors-armada-38x-functional-specifications-2015-11.pdf > > 17.3 Link Initialization > > In case the initialization fails and no link is established, the PHY > will keep on trying to initiate a link forever unless the port is > disabled. As long as the port is enabled, the PHY will continue trying > to establish a link; once the PHY identifies that a device is > connected to it, a link will be established. > > PCIe port is enabled by bits in SoC Control 1 Register, which is done in > U-Boot SerDes initialization code. This is IIRC SoC specific, and reason > why every Armada SoC has own SerDes init code. > > And looks like that due to "the PHY will keep on trying to initiate a > link forever", the PCIe link comes up when pci-mvebu.c sets all required > registers to correct values. E.g. set correct mode (RC vs endpoint), > link width (x1 vs x4), etc... > >> Could you perhaps try to remove some of the register configurations in >> the MVEBU PCIe driver to see, if the link establishment relies on this >> register to be written to (e.g. PCI_EXP_LNKCAP)? > > First port on A385 is by default set to X4 width, other ports to X1 > width. Without updating LNKCAP to correct width, card in first PCIe port > never initialize. And cards in other ports are initialized even before > pci-mvebu.c starts configuration. So the PCIe ports are now trying to establish the links, even when the correct configuration is not yet done. This might work but sound far from perfect to me IMHO. > So seems that this matches above behavior. SerDes init code enabled all > PCIe ports. Ports which are using default configuration (second, third) > are immediately initialized and link is established. Port which requires > additional configuration (first port, for switching from X4 to X1) just > stay in that "keep on trying to initiate a link forever" state until > pci-mvebu starts and set PCI_EXP_LNKCAP register, after which PHY try X1 > width and success. And seems that this is the reason why 100ms timeout > is needed... As at this stage when pci-mvebu.c switches X4 to X1, init > timeout as defined in PCIe spec (that 100ms) starts ticking. For other > ports it starts ticking when serdes init code enables ports. > > > I have looked into all PCIe registers which are present in functional > spec, but it looks like that there is no pci-mvebu register which can > turn of LTSSM and link training, like it is in other PCIe controllers. > It looks like that only SoC-specific port enable bits are there. > > It is starting to be bigger mess as before... Any suggestion how to > continue with it? > > We cannot (easily) move that code which flips PCIe bits in SoC Control 1 > Register from SerDes init code to pci-mvebu.c as this is outside of > pci-mvebu.c address space and also it is different on every SoC. > pci-mvebu.c registers are same on all Marvell SoCs, starting from Orion > up to the A39x. One idea would be, to use a "reset-controller" driver on the Armada platforms, that is capable to at least reset and release the PCIe ports. Via the SoC Control 1 reg on A38x and via the SoC Control Register on AXP. I just looked into some Linux PCIe DT bindings and found e.g. this in the mediatek spec: Documentation/devicetree/bindings/pci/mediatek-pcie.txt ... Required properties for MT7623/MT2701: ... - resets: Must contain an entry for each entry in reset-names. See ../reset/reset.txt for details. - reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the number of root ports. ... resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>, <&hifsys MT2701_HIFSYS_PCIE1_RST>, <&hifsys MT2701_HIFSYS_PCIE2_RST>; reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2"; phys = <&pcie0_phy PHY_TYPE_PCIE>, <&pcie1_phy PHY_TYPE_PCIE>, <&pcie2_phy PHY_TYPE_PCIE>; phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2"; And make sure in the serdes code keeps (or actively sets?) these PCIe ports into the reset state. The PCIe driver would then release the ports out of reset after their configuration. Or is some other serdes code missing in between "get PCIe port out of reset" and the MVEBU PCIe driver taking over the control? What do you think? I might be missing something here. Thanks, Stefan
On Monday 29 November 2021 13:30:45 Stefan Roese wrote: > Hi Pali, > > On 11/29/21 12:47, Pali Rohár wrote: > > Hello! > > > > On Monday 29 November 2021 10:22:58 Stefan Roese wrote: > > > On 11/29/21 10:06, Pali Rohár wrote: > > > > > > <snip> > > > > > > > > > After this DTS change, pci-mvebu.c will just replace value of current > > > > > > number of lanes (which is set to 4 by serdes code) to value from DTS, > > > > > > which is 4. Therefore there should be no change. > > > > > > > > > > > > Could you test whole patch series with above DTS change if it works > > > > > > properly on Theadorable board? > > > > > > > > > > Yes, I don't see any issues with this patchset applied plus this DT > > > > > patch on theadorable. The PCIe links are up and with the correct width. > > > > > > > > > > What I'm wondering is, when exactly does the PCIe RP start the link > > > > > establishment. In my case with AXP this is still in the AXP serdes code > > > > > of course. But in the A38x code, it should be in the PCIe controller > > > > > driver now AFAIU. I see that you configure the link width in the > > > > > controller and do some other configuration (address windows etc), but > > > > > at the end you "simply" wait for the link to come up via > > > > > mvebu_pcie_wait_for_link(). I would have expected here some special > > > > > command (config bit?) to the PCIe controller to start the link > > > > > establishment. So when exactly does the A38x start this action? > > > > > > > > That is interesting question... While I'm reading it again, I really do > > > > not know. Because you are right that mvebu_pcie_wait_for_link() is just > > > > waiting for a link and it "magically" comes up. I have tested it on A385 > > > > and it is stable with different Compex Atheros cards which caused issues > > > > in past also on A3720. > > > > > > I would prefer, to fully understand when exactly the link establishment > > > is started. Since this is crucial for the setup of the controller that > > > needs to be done *before* the link starts to come up. > > > > I try to dig as more information as possible and finally I find out that > > important information is available also in now removed, but originally > > public A38x documentation. Thankfully web archive has copy of it: > > > > https://web.archive.org/web/20200420191927/https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-embedded-processors-armada-38x-functional-specifications-2015-11.pdf > > > > 17.3 Link Initialization > > > > In case the initialization fails and no link is established, the PHY > > will keep on trying to initiate a link forever unless the port is > > disabled. As long as the port is enabled, the PHY will continue trying > > to establish a link; once the PHY identifies that a device is > > connected to it, a link will be established. > > > > PCIe port is enabled by bits in SoC Control 1 Register, which is done in > > U-Boot SerDes initialization code. This is IIRC SoC specific, and reason > > why every Armada SoC has own SerDes init code. > > > > And looks like that due to "the PHY will keep on trying to initiate a > > link forever", the PCIe link comes up when pci-mvebu.c sets all required > > registers to correct values. E.g. set correct mode (RC vs endpoint), > > link width (x1 vs x4), etc... > > > > > Could you perhaps try to remove some of the register configurations in > > > the MVEBU PCIe driver to see, if the link establishment relies on this > > > register to be written to (e.g. PCI_EXP_LNKCAP)? > > > > First port on A385 is by default set to X4 width, other ports to X1 > > width. Without updating LNKCAP to correct width, card in first PCIe port > > never initialize. And cards in other ports are initialized even before > > pci-mvebu.c starts configuration. > > So the PCIe ports are now trying to establish the links, even when the > correct configuration is not yet done. This might work but sound far > from perfect to me IMHO. Yes, it looks like (based on behavior of the first port). And it is not perfect, just another mess :-( > > So seems that this matches above behavior. SerDes init code enabled all > > PCIe ports. Ports which are using default configuration (second, third) > > are immediately initialized and link is established. Port which requires > > additional configuration (first port, for switching from X4 to X1) just > > stay in that "keep on trying to initiate a link forever" state until > > pci-mvebu starts and set PCI_EXP_LNKCAP register, after which PHY try X1 > > width and success. And seems that this is the reason why 100ms timeout > > is needed... As at this stage when pci-mvebu.c switches X4 to X1, init > > timeout as defined in PCIe spec (that 100ms) starts ticking. For other > > ports it starts ticking when serdes init code enables ports. > > > > > > I have looked into all PCIe registers which are present in functional > > spec, but it looks like that there is no pci-mvebu register which can > > turn of LTSSM and link training, like it is in other PCIe controllers. > > It looks like that only SoC-specific port enable bits are there. > > > > It is starting to be bigger mess as before... Any suggestion how to > > continue with it? > > > > We cannot (easily) move that code which flips PCIe bits in SoC Control 1 > > Register from SerDes init code to pci-mvebu.c as this is outside of > > pci-mvebu.c address space and also it is different on every SoC. > > pci-mvebu.c registers are same on all Marvell SoCs, starting from Orion > > up to the A39x. > > One idea would be, to use a "reset-controller" driver on the Armada > platforms, that is capable to at least reset and release the PCIe ports. > Via the SoC Control 1 reg on A38x and via the SoC Control Register > on AXP. In that specification is also written: Enable the PCI Express interface by setting the <PCIe0En>, <PCIe1En>, <PCIe2En>, or <PCIe3En> field in the SoC Control 1 Register (Table 1888 p. 1395). This allows programming of link parameters before the start of link initialization. The highest common link width is established according to the following order: x4 to x1. So I think the correct behavior should be: 1. pci-mvebu.c configures all controller registers to correct values 2. PCIe port is enabled via SoC-specific register 3. pci-mvebu.c waits for link up I guess that reset-controller does not help, as core release this reset prior starting driver initialization. Anyway, this A385 SoC Control 1 Register is at address 0x18204 which is part of following device defined in kernel DTS file: systemc: system-controller@18200 { compatible = "marvell,armada-380-system-controller", "marvell,armada-370-xp-system-controller"; reg = <0x18200 0x100>; }; Linux kernel has driver for this DTS device is file: arch/arm/mach-mvebu/system-controller.c U-Boot does not have any driver for this compatible string. So PCIe port enable/disable should be in this driver. I can write simple driver also for U-Boot which can control this register. But I really do not know which interface should it use. Has somebody else any idea? > I just looked into some Linux PCIe DT bindings and found e.g. this in > the mediatek spec: > > Documentation/devicetree/bindings/pci/mediatek-pcie.txt > ... > Required properties for MT7623/MT2701: > ... > - resets: Must contain an entry for each entry in reset-names. > See ../reset/reset.txt for details. > - reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the > number of root ports. > ... > resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>, > <&hifsys MT2701_HIFSYS_PCIE1_RST>, > <&hifsys MT2701_HIFSYS_PCIE2_RST>; > reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2"; > phys = <&pcie0_phy PHY_TYPE_PCIE>, <&pcie1_phy PHY_TYPE_PCIE>, > <&pcie2_phy PHY_TYPE_PCIE>; > phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2"; > > > And make sure in the serdes code keeps (or actively sets?) these PCIe > ports into the reset state. The PCIe driver would then release the ports > out of reset after their configuration. > > Or is some other serdes code missing in between "get PCIe port out of > reset" and the MVEBU PCIe driver taking over the control? > > What do you think? I might be missing something here. > > Thanks, > Stefan
On Monday 29 November 2021 14:27:48 Pali Rohár wrote: > On Monday 29 November 2021 13:30:45 Stefan Roese wrote: > > Hi Pali, > > > > On 11/29/21 12:47, Pali Rohár wrote: > > > Hello! > > > > > > On Monday 29 November 2021 10:22:58 Stefan Roese wrote: > > > > On 11/29/21 10:06, Pali Rohár wrote: > > > > > > > > <snip> > > > > > > > > > > > After this DTS change, pci-mvebu.c will just replace value of current > > > > > > > number of lanes (which is set to 4 by serdes code) to value from DTS, > > > > > > > which is 4. Therefore there should be no change. > > > > > > > > > > > > > > Could you test whole patch series with above DTS change if it works > > > > > > > properly on Theadorable board? > > > > > > > > > > > > Yes, I don't see any issues with this patchset applied plus this DT > > > > > > patch on theadorable. The PCIe links are up and with the correct width. > > > > > > > > > > > > What I'm wondering is, when exactly does the PCIe RP start the link > > > > > > establishment. In my case with AXP this is still in the AXP serdes code > > > > > > of course. But in the A38x code, it should be in the PCIe controller > > > > > > driver now AFAIU. I see that you configure the link width in the > > > > > > controller and do some other configuration (address windows etc), but > > > > > > at the end you "simply" wait for the link to come up via > > > > > > mvebu_pcie_wait_for_link(). I would have expected here some special > > > > > > command (config bit?) to the PCIe controller to start the link > > > > > > establishment. So when exactly does the A38x start this action? > > > > > > > > > > That is interesting question... While I'm reading it again, I really do > > > > > not know. Because you are right that mvebu_pcie_wait_for_link() is just > > > > > waiting for a link and it "magically" comes up. I have tested it on A385 > > > > > and it is stable with different Compex Atheros cards which caused issues > > > > > in past also on A3720. > > > > > > > > I would prefer, to fully understand when exactly the link establishment > > > > is started. Since this is crucial for the setup of the controller that > > > > needs to be done *before* the link starts to come up. > > > > > > I try to dig as more information as possible and finally I find out that > > > important information is available also in now removed, but originally > > > public A38x documentation. Thankfully web archive has copy of it: > > > > > > https://web.archive.org/web/20200420191927/https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-embedded-processors-armada-38x-functional-specifications-2015-11.pdf > > > > > > 17.3 Link Initialization > > > > > > In case the initialization fails and no link is established, the PHY > > > will keep on trying to initiate a link forever unless the port is > > > disabled. As long as the port is enabled, the PHY will continue trying > > > to establish a link; once the PHY identifies that a device is > > > connected to it, a link will be established. > > > > > > PCIe port is enabled by bits in SoC Control 1 Register, which is done in > > > U-Boot SerDes initialization code. This is IIRC SoC specific, and reason > > > why every Armada SoC has own SerDes init code. > > > > > > And looks like that due to "the PHY will keep on trying to initiate a > > > link forever", the PCIe link comes up when pci-mvebu.c sets all required > > > registers to correct values. E.g. set correct mode (RC vs endpoint), > > > link width (x1 vs x4), etc... > > > > > > > Could you perhaps try to remove some of the register configurations in > > > > the MVEBU PCIe driver to see, if the link establishment relies on this > > > > register to be written to (e.g. PCI_EXP_LNKCAP)? > > > > > > First port on A385 is by default set to X4 width, other ports to X1 > > > width. Without updating LNKCAP to correct width, card in first PCIe port > > > never initialize. And cards in other ports are initialized even before > > > pci-mvebu.c starts configuration. > > > > So the PCIe ports are now trying to establish the links, even when the > > correct configuration is not yet done. This might work but sound far > > from perfect to me IMHO. > > Yes, it looks like (based on behavior of the first port). And it is not > perfect, just another mess :-( > > > > So seems that this matches above behavior. SerDes init code enabled all > > > PCIe ports. Ports which are using default configuration (second, third) > > > are immediately initialized and link is established. Port which requires > > > additional configuration (first port, for switching from X4 to X1) just > > > stay in that "keep on trying to initiate a link forever" state until > > > pci-mvebu starts and set PCI_EXP_LNKCAP register, after which PHY try X1 > > > width and success. And seems that this is the reason why 100ms timeout > > > is needed... As at this stage when pci-mvebu.c switches X4 to X1, init > > > timeout as defined in PCIe spec (that 100ms) starts ticking. For other > > > ports it starts ticking when serdes init code enables ports. > > > > > > > > > I have looked into all PCIe registers which are present in functional > > > spec, but it looks like that there is no pci-mvebu register which can > > > turn of LTSSM and link training, like it is in other PCIe controllers. > > > It looks like that only SoC-specific port enable bits are there. > > > > > > It is starting to be bigger mess as before... Any suggestion how to > > > continue with it? > > > > > > We cannot (easily) move that code which flips PCIe bits in SoC Control 1 > > > Register from SerDes init code to pci-mvebu.c as this is outside of > > > pci-mvebu.c address space and also it is different on every SoC. > > > pci-mvebu.c registers are same on all Marvell SoCs, starting from Orion > > > up to the A39x. > > > > One idea would be, to use a "reset-controller" driver on the Armada > > platforms, that is capable to at least reset and release the PCIe ports. > > Via the SoC Control 1 reg on A38x and via the SoC Control Register > > on AXP. > > In that specification is also written: > > Enable the PCI Express interface by setting the <PCIe0En>, <PCIe1En>, > <PCIe2En>, or <PCIe3En> field in the SoC Control 1 Register (Table > 1888 p. 1395). This allows programming of link parameters before the > start of link initialization. The highest common link width is > established according to the following order: x4 to x1. > > So I think the correct behavior should be: > > 1. pci-mvebu.c configures all controller registers to correct values > 2. PCIe port is enabled via SoC-specific register > 3. pci-mvebu.c waits for link up > > I guess that reset-controller does not help, as core release this reset > prior starting driver initialization. Ok, it looks like that reset controller API allows to do this. It would mean to define that "system-controller@18200" as reset controller, exports from it for each PCIe port reset functionality and implements assert and deassert functions which disable and enable port. And because DTS for pci-mvebu.c driver is defined as multi-root-port, "resets" property would need to be defined for each port separately. Just I'm not sure if this "enable port functionality" should be implemented via Reset Controller API... > Anyway, this A385 SoC Control 1 Register is at address 0x18204 which is > part of following device defined in kernel DTS file: > > systemc: system-controller@18200 { > compatible = "marvell,armada-380-system-controller", > "marvell,armada-370-xp-system-controller"; > reg = <0x18200 0x100>; > }; > > Linux kernel has driver for this DTS device is file: > arch/arm/mach-mvebu/system-controller.c > > U-Boot does not have any driver for this compatible string. > > So PCIe port enable/disable should be in this driver. I can write simple > driver also for U-Boot which can control this register. But I really do > not know which interface should it use. > > Has somebody else any idea? > > > I just looked into some Linux PCIe DT bindings and found e.g. this in > > the mediatek spec: > > > > Documentation/devicetree/bindings/pci/mediatek-pcie.txt > > ... > > Required properties for MT7623/MT2701: > > ... > > - resets: Must contain an entry for each entry in reset-names. > > See ../reset/reset.txt for details. > > - reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the > > number of root ports. > > ... > > resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>, > > <&hifsys MT2701_HIFSYS_PCIE1_RST>, > > <&hifsys MT2701_HIFSYS_PCIE2_RST>; > > reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2"; > > phys = <&pcie0_phy PHY_TYPE_PCIE>, <&pcie1_phy PHY_TYPE_PCIE>, > > <&pcie2_phy PHY_TYPE_PCIE>; > > phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2"; > > > > > > And make sure in the serdes code keeps (or actively sets?) these PCIe > > ports into the reset state. The PCIe driver would then release the ports > > out of reset after their configuration. > > > > Or is some other serdes code missing in between "get PCIe port out of > > reset" and the MVEBU PCIe driver taking over the control? > > > > What do you think? I might be missing something here. > > > > Thanks, > > Stefan
On 11/29/21 15:28, Pali Rohár wrote: > On Monday 29 November 2021 14:27:48 Pali Rohár wrote: >> On Monday 29 November 2021 13:30:45 Stefan Roese wrote: >>> Hi Pali, >>> >>> On 11/29/21 12:47, Pali Rohár wrote: >>>> Hello! >>>> >>>> On Monday 29 November 2021 10:22:58 Stefan Roese wrote: >>>>> On 11/29/21 10:06, Pali Rohár wrote: >>>>> >>>>> <snip> >>>>> >>>>>>>> After this DTS change, pci-mvebu.c will just replace value of current >>>>>>>> number of lanes (which is set to 4 by serdes code) to value from DTS, >>>>>>>> which is 4. Therefore there should be no change. >>>>>>>> >>>>>>>> Could you test whole patch series with above DTS change if it works >>>>>>>> properly on Theadorable board? >>>>>>> >>>>>>> Yes, I don't see any issues with this patchset applied plus this DT >>>>>>> patch on theadorable. The PCIe links are up and with the correct width. >>>>>>> >>>>>>> What I'm wondering is, when exactly does the PCIe RP start the link >>>>>>> establishment. In my case with AXP this is still in the AXP serdes code >>>>>>> of course. But in the A38x code, it should be in the PCIe controller >>>>>>> driver now AFAIU. I see that you configure the link width in the >>>>>>> controller and do some other configuration (address windows etc), but >>>>>>> at the end you "simply" wait for the link to come up via >>>>>>> mvebu_pcie_wait_for_link(). I would have expected here some special >>>>>>> command (config bit?) to the PCIe controller to start the link >>>>>>> establishment. So when exactly does the A38x start this action? >>>>>> >>>>>> That is interesting question... While I'm reading it again, I really do >>>>>> not know. Because you are right that mvebu_pcie_wait_for_link() is just >>>>>> waiting for a link and it "magically" comes up. I have tested it on A385 >>>>>> and it is stable with different Compex Atheros cards which caused issues >>>>>> in past also on A3720. >>>>> >>>>> I would prefer, to fully understand when exactly the link establishment >>>>> is started. Since this is crucial for the setup of the controller that >>>>> needs to be done *before* the link starts to come up. >>>> >>>> I try to dig as more information as possible and finally I find out that >>>> important information is available also in now removed, but originally >>>> public A38x documentation. Thankfully web archive has copy of it: >>>> >>>> https://web.archive.org/web/20200420191927/https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-embedded-processors-armada-38x-functional-specifications-2015-11.pdf >>>> >>>> 17.3 Link Initialization >>>> >>>> In case the initialization fails and no link is established, the PHY >>>> will keep on trying to initiate a link forever unless the port is >>>> disabled. As long as the port is enabled, the PHY will continue trying >>>> to establish a link; once the PHY identifies that a device is >>>> connected to it, a link will be established. >>>> >>>> PCIe port is enabled by bits in SoC Control 1 Register, which is done in >>>> U-Boot SerDes initialization code. This is IIRC SoC specific, and reason >>>> why every Armada SoC has own SerDes init code. >>>> >>>> And looks like that due to "the PHY will keep on trying to initiate a >>>> link forever", the PCIe link comes up when pci-mvebu.c sets all required >>>> registers to correct values. E.g. set correct mode (RC vs endpoint), >>>> link width (x1 vs x4), etc... >>>> >>>>> Could you perhaps try to remove some of the register configurations in >>>>> the MVEBU PCIe driver to see, if the link establishment relies on this >>>>> register to be written to (e.g. PCI_EXP_LNKCAP)? >>>> >>>> First port on A385 is by default set to X4 width, other ports to X1 >>>> width. Without updating LNKCAP to correct width, card in first PCIe port >>>> never initialize. And cards in other ports are initialized even before >>>> pci-mvebu.c starts configuration. >>> >>> So the PCIe ports are now trying to establish the links, even when the >>> correct configuration is not yet done. This might work but sound far >>> from perfect to me IMHO. >> >> Yes, it looks like (based on behavior of the first port). And it is not >> perfect, just another mess :-( >> >>>> So seems that this matches above behavior. SerDes init code enabled all >>>> PCIe ports. Ports which are using default configuration (second, third) >>>> are immediately initialized and link is established. Port which requires >>>> additional configuration (first port, for switching from X4 to X1) just >>>> stay in that "keep on trying to initiate a link forever" state until >>>> pci-mvebu starts and set PCI_EXP_LNKCAP register, after which PHY try X1 >>>> width and success. And seems that this is the reason why 100ms timeout >>>> is needed... As at this stage when pci-mvebu.c switches X4 to X1, init >>>> timeout as defined in PCIe spec (that 100ms) starts ticking. For other >>>> ports it starts ticking when serdes init code enables ports. >>>> >>>> >>>> I have looked into all PCIe registers which are present in functional >>>> spec, but it looks like that there is no pci-mvebu register which can >>>> turn of LTSSM and link training, like it is in other PCIe controllers. >>>> It looks like that only SoC-specific port enable bits are there. >>>> >>>> It is starting to be bigger mess as before... Any suggestion how to >>>> continue with it? >>>> >>>> We cannot (easily) move that code which flips PCIe bits in SoC Control 1 >>>> Register from SerDes init code to pci-mvebu.c as this is outside of >>>> pci-mvebu.c address space and also it is different on every SoC. >>>> pci-mvebu.c registers are same on all Marvell SoCs, starting from Orion >>>> up to the A39x. >>> >>> One idea would be, to use a "reset-controller" driver on the Armada >>> platforms, that is capable to at least reset and release the PCIe ports. >>> Via the SoC Control 1 reg on A38x and via the SoC Control Register >>> on AXP. >> >> In that specification is also written: >> >> Enable the PCI Express interface by setting the <PCIe0En>, <PCIe1En>, >> <PCIe2En>, or <PCIe3En> field in the SoC Control 1 Register (Table >> 1888 p. 1395). This allows programming of link parameters before the >> start of link initialization. The highest common link width is >> established according to the following order: x4 to x1. >> >> So I think the correct behavior should be: >> >> 1. pci-mvebu.c configures all controller registers to correct values >> 2. PCIe port is enabled via SoC-specific register >> 3. pci-mvebu.c waits for link up >> >> I guess that reset-controller does not help, as core release this reset >> prior starting driver initialization. > > Ok, it looks like that reset controller API allows to do this. It would > mean to define that "system-controller@18200" as reset controller, > exports from it for each PCIe port reset functionality and implements > assert and deassert functions which disable and enable port. > > And because DTS for pci-mvebu.c driver is defined as multi-root-port, > "resets" property would need to be defined for each port separately. Okay. Sounds like a plan to me. > Just I'm not sure if this "enable port functionality" should be > implemented via Reset Controller API... How else should / could this be done then? Do you have alterative ideas? Thanks, Stefan >> Anyway, this A385 SoC Control 1 Register is at address 0x18204 which is >> part of following device defined in kernel DTS file: >> >> systemc: system-controller@18200 { >> compatible = "marvell,armada-380-system-controller", >> "marvell,armada-370-xp-system-controller"; >> reg = <0x18200 0x100>; >> }; >> >> Linux kernel has driver for this DTS device is file: >> arch/arm/mach-mvebu/system-controller.c >> >> U-Boot does not have any driver for this compatible string. >> >> So PCIe port enable/disable should be in this driver. I can write simple >> driver also for U-Boot which can control this register. But I really do >> not know which interface should it use. >> >> Has somebody else any idea? >> >>> I just looked into some Linux PCIe DT bindings and found e.g. this in >>> the mediatek spec: >>> >>> Documentation/devicetree/bindings/pci/mediatek-pcie.txt >>> ... >>> Required properties for MT7623/MT2701: >>> ... >>> - resets: Must contain an entry for each entry in reset-names. >>> See ../reset/reset.txt for details. >>> - reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the >>> number of root ports. >>> ... >>> resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>, >>> <&hifsys MT2701_HIFSYS_PCIE1_RST>, >>> <&hifsys MT2701_HIFSYS_PCIE2_RST>; >>> reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2"; >>> phys = <&pcie0_phy PHY_TYPE_PCIE>, <&pcie1_phy PHY_TYPE_PCIE>, >>> <&pcie2_phy PHY_TYPE_PCIE>; >>> phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2"; >>> >>> >>> And make sure in the serdes code keeps (or actively sets?) these PCIe >>> ports into the reset state. The PCIe driver would then release the ports >>> out of reset after their configuration. >>> >>> Or is some other serdes code missing in between "get PCIe port out of >>> reset" and the MVEBU PCIe driver taking over the control? >>> >>> What do you think? I might be missing something here. >>> >>> Thanks, >>> Stefan
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/dam/marvell/en/public-collateral/embedded-processors/marvell-embedded-processors-armada-38x-functional-specifications-2015-11.pdf > > > > > > > > > > 17.3 Link Initialization > > > > > > > > > > In case the initialization fails and no link is established, the PHY > > > > > will keep on trying to initiate a link forever unless the port is > > > > > disabled. As long as the port is enabled, the PHY will continue trying > > > > > to establish a link; once the PHY identifies that a device is > > > > > connected to it, a link will be established. > > > > > > > > > > PCIe port is enabled by bits in SoC Control 1 Register, which is done in > > > > > U-Boot SerDes initialization code. This is IIRC SoC specific, and reason > > > > > why every Armada SoC has own SerDes init code. > > > > > > > > > > And looks like that due to "the PHY will keep on trying to initiate a > > > > > link forever", the PCIe link comes up when pci-mvebu.c sets all required > > > > > registers to correct values. E.g. set correct mode (RC vs endpoint), > > > > > link width (x1 vs x4), etc... > > > > > > > > > > > Could you perhaps try to remove some of the register configurations in > > > > > > the MVEBU PCIe driver to see, if the link establishment relies on this > > > > > > register to be written to (e.g. PCI_EXP_LNKCAP)? > > > > > > > > > > First port on A385 is by default set to X4 width, other ports to X1 > > > > > width. Without updating LNKCAP to correct width, card in first PCIe port > > > > > never initialize. And cards in other ports are initialized even before > > > > > pci-mvebu.c starts configuration. > > > > > > > > So the PCIe ports are now trying to establish the links, even when the > > > > correct configuration is not yet done. This might work but sound far > > > > from perfect to me IMHO. > > > > > > Yes, it looks like (based on behavior of the first port). And it is not > > > perfect, just another mess :-( > > > > > > > > So seems that this matches above behavior. SerDes init code enabled all > > > > > PCIe ports. Ports which are using default configuration (second, third) > > > > > are immediately initialized and link is established. Port which requires > > > > > additional configuration (first port, for switching from X4 to X1) just > > > > > stay in that "keep on trying to initiate a link forever" state until > > > > > pci-mvebu starts and set PCI_EXP_LNKCAP register, after which PHY try X1 > > > > > width and success. And seems that this is the reason why 100ms timeout > > > > > is needed... As at this stage when pci-mvebu.c switches X4 to X1, init > > > > > timeout as defined in PCIe spec (that 100ms) starts ticking. For other > > > > > ports it starts ticking when serdes init code enables ports. > > > > > > > > > > > > > > > I have looked into all PCIe registers which are present in functional > > > > > spec, but it looks like that there is no pci-mvebu register which can > > > > > turn of LTSSM and link training, like it is in other PCIe controllers. > > > > > It looks like that only SoC-specific port enable bits are there. > > > > > > > > > > It is starting to be bigger mess as before... Any suggestion how to > > > > > continue with it? > > > > > > > > > > We cannot (easily) move that code which flips PCIe bits in SoC Control 1 > > > > > Register from SerDes init code to pci-mvebu.c as this is outside of > > > > > pci-mvebu.c address space and also it is different on every SoC. > > > > > pci-mvebu.c registers are same on all Marvell SoCs, starting from Orion > > > > > up to the A39x. > > > > > > > > One idea would be, to use a "reset-controller" driver on the Armada > > > > platforms, that is capable to at least reset and release the PCIe ports. > > > > Via the SoC Control 1 reg on A38x and via the SoC Control Register > > > > on AXP. > > > > > > In that specification is also written: > > > > > > Enable the PCI Express interface by setting the <PCIe0En>, <PCIe1En>, > > > <PCIe2En>, or <PCIe3En> field in the SoC Control 1 Register (Table > > > 1888 p. 1395). This allows programming of link parameters before the > > > start of link initialization. The highest common link width is > > > established according to the following order: x4 to x1. > > > > > > So I think the correct behavior should be: > > > > > > 1. pci-mvebu.c configures all controller registers to correct values > > > 2. PCIe port is enabled via SoC-specific register > > > 3. pci-mvebu.c waits for link up > > > > > > I guess that reset-controller does not help, as core release this reset > > > prior starting driver initialization. > > > > Ok, it looks like that reset controller API allows to do this. It would > > mean to define that "system-controller@18200" as reset controller, > > exports from it for each PCIe port reset functionality and implements > > assert and deassert functions which disable and enable port. > > > > And because DTS for pci-mvebu.c driver is defined as multi-root-port, > > "resets" property would need to be defined for each port separately. > > Okay. Sounds like a plan to me. I'm looking at this right now and it is even more complicated. For example Armada XP has for some ports dedicated enable bits and for some ports has just one shared enable bit. And this HW design does not fit into current U-Boot PCI DM model where for each PCIe port there is dedicated mvebu_pcie_probe() call which should setup PCIe port, enable PCIe port and returns once PCIe port is ready. In U-Boot PCI DM model is every PCIe port as separate DM device. Any idea how to solve this issue? > > Just I'm not sure if this "enable port functionality" should be > > implemented via Reset Controller API... > > How else should / could this be done then? Do you have alterative ideas? > > Thanks, > Stefan > > > > Anyway, this A385 SoC Control 1 Register is at address 0x18204 which is > > > part of following device defined in kernel DTS file: > > > > > > systemc: system-controller@18200 { > > > compatible = "marvell,armada-380-system-controller", > > > "marvell,armada-370-xp-system-controller"; > > > reg = <0x18200 0x100>; > > > }; > > > > > > Linux kernel has driver for this DTS device is file: > > > arch/arm/mach-mvebu/system-controller.c > > > > > > U-Boot does not have any driver for this compatible string. > > > > > > So PCIe port enable/disable should be in this driver. I can write simple > > > driver also for U-Boot which can control this register. But I really do > > > not know which interface should it use. > > > > > > Has somebody else any idea? > > > > > > > I just looked into some Linux PCIe DT bindings and found e.g. this in > > > > the mediatek spec: > > > > > > > > Documentation/devicetree/bindings/pci/mediatek-pcie.txt > > > > ... > > > > Required properties for MT7623/MT2701: > > > > ... > > > > - resets: Must contain an entry for each entry in reset-names. > > > > See ../reset/reset.txt for details. > > > > - reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the > > > > number of root ports. > > > > ... > > > > resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>, > > > > <&hifsys MT2701_HIFSYS_PCIE1_RST>, > > > > <&hifsys MT2701_HIFSYS_PCIE2_RST>; > > > > reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2"; > > > > phys = <&pcie0_phy PHY_TYPE_PCIE>, <&pcie1_phy PHY_TYPE_PCIE>, > > > > <&pcie2_phy PHY_TYPE_PCIE>; > > > > phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2"; > > > > > > > > > > > > And make sure in the serdes code keeps (or actively sets?) these PCIe > > > > ports into the reset state. The PCIe driver would then release the ports > > > > out of reset after their configuration. > > > > > > > > Or is some other serdes code missing in between "get PCIe port out of > > > > reset" and the MVEBU PCIe driver taking over the control? > > > > > > > > What do you think? I might be missing something here. > > > > > > > > Thanks, > > > > Stefan
Hi Pali, On 12/10/21 15:23, Pali Rohár wrote: <snip> >>>> So I think the correct behavior should be: >>>> >>>> 1. pci-mvebu.c configures all controller registers to correct values >>>> 2. PCIe port is enabled via SoC-specific register >>>> 3. pci-mvebu.c waits for link up >>>> >>>> I guess that reset-controller does not help, as core release this reset >>>> prior starting driver initialization. >>> >>> Ok, it looks like that reset controller API allows to do this. It would >>> mean to define that "system-controller@18200" as reset controller, >>> exports from it for each PCIe port reset functionality and implements >>> assert and deassert functions which disable and enable port. >>> >>> And because DTS for pci-mvebu.c driver is defined as multi-root-port, >>> "resets" property would need to be defined for each port separately. >> >> Okay. Sounds like a plan to me. > > I'm looking at this right now and it is even more complicated. For > example Armada XP has for some ports dedicated enable bits and for some > ports has just one shared enable bit. > > And this HW design does not fit into current U-Boot PCI DM model where > for each PCIe port there is dedicated mvebu_pcie_probe() call which > should setup PCIe port, enable PCIe port and returns once PCIe port is > ready. In U-Boot PCI DM model is every PCIe port as separate DM device. > > Any idea how to solve this issue? Sorry, but I don't have clear "solution" for this in my mind right now. Thanks, Stefan >>> Just I'm not sure if this "enable port functionality" should be >>> implemented via Reset Controller API... >> >> How else should / could this be done then? Do you have alterative ideas? >> >> Thanks, >> Stefan >> >>>> Anyway, this A385 SoC Control 1 Register is at address 0x18204 which is >>>> part of following device defined in kernel DTS file: >>>> >>>> systemc: system-controller@18200 { >>>> compatible = "marvell,armada-380-system-controller", >>>> "marvell,armada-370-xp-system-controller"; >>>> reg = <0x18200 0x100>; >>>> }; >>>> >>>> Linux kernel has driver for this DTS device is file: >>>> arch/arm/mach-mvebu/system-controller.c >>>> >>>> U-Boot does not have any driver for this compatible string. >>>> >>>> So PCIe port enable/disable should be in this driver. I can write simple >>>> driver also for U-Boot which can control this register. But I really do >>>> not know which interface should it use. >>>> >>>> Has somebody else any idea? >>>> >>>>> I just looked into some Linux PCIe DT bindings and found e.g. this in >>>>> the mediatek spec: >>>>> >>>>> Documentation/devicetree/bindings/pci/mediatek-pcie.txt >>>>> ... >>>>> Required properties for MT7623/MT2701: >>>>> ... >>>>> - resets: Must contain an entry for each entry in reset-names. >>>>> See ../reset/reset.txt for details. >>>>> - reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the >>>>> number of root ports. >>>>> ... >>>>> resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>, >>>>> <&hifsys MT2701_HIFSYS_PCIE1_RST>, >>>>> <&hifsys MT2701_HIFSYS_PCIE2_RST>; >>>>> reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2"; >>>>> phys = <&pcie0_phy PHY_TYPE_PCIE>, <&pcie1_phy PHY_TYPE_PCIE>, >>>>> <&pcie2_phy PHY_TYPE_PCIE>; >>>>> phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2"; >>>>> >>>>> >>>>> And make sure in the serdes code keeps (or actively sets?) these PCIe >>>>> ports into the reset state. The PCIe driver would then release the ports >>>>> out of reset after their configuration. >>>>> >>>>> Or is some other serdes code missing in between "get PCIe port out of >>>>> reset" and the MVEBU PCIe driver taking over the control? >>>>> >>>>> What do you think? I might be missing something here. >>>>> >>>>> Thanks, >>>>> Stefan Viele Grüße, Stefan Roese
On Monday 13 December 2021 08:36:00 Stefan Roese wrote: > Hi Pali, > > On 12/10/21 15:23, Pali Rohár wrote: > > <snip> > > > > > > So I think the correct behavior should be: > > > > > > > > > > 1. pci-mvebu.c configures all controller registers to correct values > > > > > 2. PCIe port is enabled via SoC-specific register > > > > > 3. pci-mvebu.c waits for link up > > > > > > > > > > I guess that reset-controller does not help, as core release this reset > > > > > prior starting driver initialization. > > > > > > > > Ok, it looks like that reset controller API allows to do this. It would > > > > mean to define that "system-controller@18200" as reset controller, > > > > exports from it for each PCIe port reset functionality and implements > > > > assert and deassert functions which disable and enable port. > > > > > > > > And because DTS for pci-mvebu.c driver is defined as multi-root-port, > > > > "resets" property would need to be defined for each port separately. > > > > > > Okay. Sounds like a plan to me. > > > > I'm looking at this right now and it is even more complicated. For > > example Armada XP has for some ports dedicated enable bits and for some > > ports has just one shared enable bit. > > > > And this HW design does not fit into current U-Boot PCI DM model where > > for each PCIe port there is dedicated mvebu_pcie_probe() call which > > should setup PCIe port, enable PCIe port and returns once PCIe port is > > ready. In U-Boot PCI DM model is every PCIe port as separate DM device. > > > > Any idea how to solve this issue? > > Sorry, but I don't have clear "solution" for this in my mind right now. Ok, I will try to invent something... > Thanks, > Stefan > > > > > Just I'm not sure if this "enable port functionality" should be > > > > implemented via Reset Controller API... > > > > > > How else should / could this be done then? Do you have alterative ideas? > > > > > > Thanks, > > > Stefan > > > > > > > > Anyway, this A385 SoC Control 1 Register is at address 0x18204 which is > > > > > part of following device defined in kernel DTS file: > > > > > > > > > > systemc: system-controller@18200 { > > > > > compatible = "marvell,armada-380-system-controller", > > > > > "marvell,armada-370-xp-system-controller"; > > > > > reg = <0x18200 0x100>; > > > > > }; > > > > > > > > > > Linux kernel has driver for this DTS device is file: > > > > > arch/arm/mach-mvebu/system-controller.c > > > > > > > > > > U-Boot does not have any driver for this compatible string. > > > > > > > > > > So PCIe port enable/disable should be in this driver. I can write simple > > > > > driver also for U-Boot which can control this register. But I really do > > > > > not know which interface should it use. > > > > > > > > > > Has somebody else any idea? > > > > > > > > > > > I just looked into some Linux PCIe DT bindings and found e.g. this in > > > > > > the mediatek spec: > > > > > > > > > > > > Documentation/devicetree/bindings/pci/mediatek-pcie.txt > > > > > > ... > > > > > > Required properties for MT7623/MT2701: > > > > > > ... > > > > > > - resets: Must contain an entry for each entry in reset-names. > > > > > > See ../reset/reset.txt for details. > > > > > > - reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the > > > > > > number of root ports. > > > > > > ... > > > > > > resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>, > > > > > > <&hifsys MT2701_HIFSYS_PCIE1_RST>, > > > > > > <&hifsys MT2701_HIFSYS_PCIE2_RST>; > > > > > > reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2"; > > > > > > phys = <&pcie0_phy PHY_TYPE_PCIE>, <&pcie1_phy PHY_TYPE_PCIE>, > > > > > > <&pcie2_phy PHY_TYPE_PCIE>; > > > > > > phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2"; > > > > > > > > > > > > > > > > > > And make sure in the serdes code keeps (or actively sets?) these PCIe > > > > > > ports into the reset state. The PCIe driver would then release the ports > > > > > > out of reset after their configuration. > > > > > > > > > > > > Or is some other serdes code missing in between "get PCIe port out of > > > > > > reset" and the MVEBU PCIe driver taking over the control? > > > > > > > > > > > > What do you think? I might be missing something here. > > > > > > > > > > > > Thanks, > > > > > > Stefan > > Viele Grüße, > Stefan Roese > > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
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 */