[PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c
Pali Rohár
pali at kernel.org
Tue Nov 23 16:59:53 CET 2021
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 at 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 at kernel.org>
> > > > Signed-off-by: Marek Behún <marek.behun at 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 at 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 at 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 at 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 at denx.de
More information about the U-Boot
mailing list