Re: [PATCH u-boot-marvell 7/9] arm: mvebu: a38x: serdes: Don't configure PCIe cards in SerDes init code
On 24.09.21 22:59, Marek Behún wrote:
From: Pali Rohár
This code is trying to parse PCIe config space of PCIe card connected on
the other end of link and then is trying to force 5.0 GT/s speed via Target
Link Speed bits in PCIe Root Port Link Control 2 Register on the local part
of link if it sees that card supports 5.0 GT/s via Max Link Speed bits in
Link Capabilities Register.
The code is incorrect for more reasons:
- Accessing config space of an endpoint card cannot be done immediately.
If the PCIe link is not up, reading vendor/device ID registers will
return all ones.
- Parsing is incomplete, so it can cause issues even for working cards.
Moreover there is no need to force speed to 5.0 GT/s via Target Link Speed
bits on PCIe Root Port Link Control 2 Register. Hardware changes speed from
2.5 GT/s to 5.0 GT/s autonomously when it is supported.
Most importantly, this code does not change link speed at all, since
because after updating Target Link Speed bits on PCIe Root Port Link
Control 2 Register, it is required to retrain the link, and the code for
that is completely missing.
The code was probably needed for making buggy endpoint cards work. Such a
workaround, though, should be implemented via PCIe subsystem (via quirks,
for example), as buggy cards could also affect other PCIe controllers.
Note that this code is fully unrelated to a38x SerDes code and really
should not have been included in SerDes initialization. Usage of magic
constants without names and comments made this SerDes code hard to read and
understand.
Remove this PCIe application code from low level SerDes code. As this code
is configuring only 5.0 GT/s part, in the worst case, it could leave buggy
cards at the initial speed of 2.5 GT/s (if somehow before this change they
could have been "upgraded" to 5.0 GT/s speed even with missing link
retraining). Compliant cards which just need longer initialization should
work better after this change.
Signed-off-by: Pali Rohár
Reviewed-by: Marek Behún
Reviewed-by: Stefan Roese
Thanks,
Stefan
---
arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.c | 128 +
1 file changed, 1 insertion(+), 127 deletions(-)
diff --git a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.c
b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.c
index a7e45a5550..0445b43def 100644
--- a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.c
+++ b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.c
@@ -21,10 +21,8 @@ __weak void board_pex_config(void)
int hws_pex_config(const struct serdes_map *serdes_map, u8 count)
{
- u32 pex_idx, tmp, next_busno, first_busno, temp_pex_reg,
- temp_reg, addr, dev_id, ctrl_mode;
enum serdes_type serdes_type;
- u32 idx;
+ u32 idx, tmp;
DEBUG_INIT_FULL_S("\n### hws_pex_config ###\n");
@@ -60,132 +58,8 @@ int hws_pex_config(const struct serdes_map *serdes_map, u8 count)
reg_write(SOC_CONTROL_REG1, tmp);
- /* Support gen1/gen2 */
- DEBUG_INIT_FULL_S("Support gen1/gen2\n");
-
board_pex_config();
- next_busno = 0;
- mdelay(150);
-
- for (idx = 0; idx < count; idx++) {
- serdes_type = serdes_map[idx].serdes_type;
- DEBUG_INIT_FULL_S(" serdes_type=0x");
- DEBUG_INIT_FULL_D(serdes_type, 8);
- DEBUG_INIT_FULL_S("\n");
- DEBUG_INIT_FULL_S(" idx=0x");
- DEBUG_INIT_FULL_D(idx, 8);
- DEBUG_INIT_FULL_S("\n");
-
- /* Configuration for PEX only */
- if ((serdes_type != PEX0) && (serdes_type != PEX1) &&
- (serdes_type != PEX2) && (serdes_type != PEX3))
- continue;
-
- if ((serdes_type != PEX0) &&
- ((serdes_map[idx].serdes_mode == PEX_ROOT_COMPLEX_X4) ||
-(serdes_map[idx].serdes_mode == PEX_END_POINT_X4))) {
- /* for PEX by4 - relevant for the first port only */
- continue;
- }
-
- pex_idx = serdes_type - PEX0;
- tmp = reg_read(PEX_DBG_STATUS_REG(pex_idx));
-
- first_busno = next_busno;
- if ((tmp & 0x7f) != 0x7e) {
- DEBUG_INIT_S("PCIe, Idx ");
- DEBUG_INIT_D(pex_idx, 1);
- DEBUG_INIT_S(": detected no link\n");
- continue;
- }
-
- next_busno++;
-
- /*
-* Read maximum link speed. It must be 0x2 (5.0 GT/s) as this
-* value was set in serdes_power_up_ctrl() function.
-*/
- temp_pex_reg = reg_read((PEX_CFG_DIRECT_ACCESS
-(pex_idx, PEX_LINK_CAPABILITY_REG)));
- temp_pex_reg &= 0xf;
- if (temp_pex_reg != 0x2)
- continue;
-
- /* Read negotiated link speed */
- temp_reg = (reg_read(PEX_CFG_DI
[PATCH u-boot-marvell 7/9] arm: mvebu: a38x: serdes: Don't configure PCIe cards in SerDes init code
From: Pali Rohár
This code is trying to parse PCIe config space of PCIe card connected on
the other end of link and then is trying to force 5.0 GT/s speed via Target
Link Speed bits in PCIe Root Port Link Control 2 Register on the local part
of link if it sees that card supports 5.0 GT/s via Max Link Speed bits in
Link Capabilities Register.
The code is incorrect for more reasons:
- Accessing config space of an endpoint card cannot be done immediately.
If the PCIe link is not up, reading vendor/device ID registers will
return all ones.
- Parsing is incomplete, so it can cause issues even for working cards.
Moreover there is no need to force speed to 5.0 GT/s via Target Link Speed
bits on PCIe Root Port Link Control 2 Register. Hardware changes speed from
2.5 GT/s to 5.0 GT/s autonomously when it is supported.
Most importantly, this code does not change link speed at all, since
because after updating Target Link Speed bits on PCIe Root Port Link
Control 2 Register, it is required to retrain the link, and the code for
that is completely missing.
The code was probably needed for making buggy endpoint cards work. Such a
workaround, though, should be implemented via PCIe subsystem (via quirks,
for example), as buggy cards could also affect other PCIe controllers.
Note that this code is fully unrelated to a38x SerDes code and really
should not have been included in SerDes initialization. Usage of magic
constants without names and comments made this SerDes code hard to read and
understand.
Remove this PCIe application code from low level SerDes code. As this code
is configuring only 5.0 GT/s part, in the worst case, it could leave buggy
cards at the initial speed of 2.5 GT/s (if somehow before this change they
could have been "upgraded" to 5.0 GT/s speed even with missing link
retraining). Compliant cards which just need longer initialization should
work better after this change.
Signed-off-by: Pali Rohár
Reviewed-by: Marek Behún
---
arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.c | 128 +
1 file changed, 1 insertion(+), 127 deletions(-)
diff --git a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.c
b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.c
index a7e45a5550..0445b43def 100644
--- a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.c
+++ b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.c
@@ -21,10 +21,8 @@ __weak void board_pex_config(void)
int hws_pex_config(const struct serdes_map *serdes_map, u8 count)
{
- u32 pex_idx, tmp, next_busno, first_busno, temp_pex_reg,
- temp_reg, addr, dev_id, ctrl_mode;
enum serdes_type serdes_type;
- u32 idx;
+ u32 idx, tmp;
DEBUG_INIT_FULL_S("\n### hws_pex_config ###\n");
@@ -60,132 +58,8 @@ int hws_pex_config(const struct serdes_map *serdes_map, u8
count)
reg_write(SOC_CONTROL_REG1, tmp);
- /* Support gen1/gen2 */
- DEBUG_INIT_FULL_S("Support gen1/gen2\n");
-
board_pex_config();
- next_busno = 0;
- mdelay(150);
-
- for (idx = 0; idx < count; idx++) {
- serdes_type = serdes_map[idx].serdes_type;
- DEBUG_INIT_FULL_S(" serdes_type=0x");
- DEBUG_INIT_FULL_D(serdes_type, 8);
- DEBUG_INIT_FULL_S("\n");
- DEBUG_INIT_FULL_S(" idx=0x");
- DEBUG_INIT_FULL_D(idx, 8);
- DEBUG_INIT_FULL_S("\n");
-
- /* Configuration for PEX only */
- if ((serdes_type != PEX0) && (serdes_type != PEX1) &&
- (serdes_type != PEX2) && (serdes_type != PEX3))
- continue;
-
- if ((serdes_type != PEX0) &&
- ((serdes_map[idx].serdes_mode == PEX_ROOT_COMPLEX_X4) ||
-(serdes_map[idx].serdes_mode == PEX_END_POINT_X4))) {
- /* for PEX by4 - relevant for the first port only */
- continue;
- }
-
- pex_idx = serdes_type - PEX0;
- tmp = reg_read(PEX_DBG_STATUS_REG(pex_idx));
-
- first_busno = next_busno;
- if ((tmp & 0x7f) != 0x7e) {
- DEBUG_INIT_S("PCIe, Idx ");
- DEBUG_INIT_D(pex_idx, 1);
- DEBUG_INIT_S(": detected no link\n");
- continue;
- }
-
- next_busno++;
-
- /*
-* Read maximum link speed. It must be 0x2 (5.0 GT/s) as this
-* value was set in serdes_power_up_ctrl() function.
-*/
- temp_pex_reg = reg_read((PEX_CFG_DIRECT_ACCESS
-(pex_idx, PEX_LINK_CAPABILITY_REG)));
- temp_pex_reg &= 0xf;
- if (temp_pex_reg != 0x2)
- continue;
-
- /* Read negotiated link speed */
- temp_reg = (reg_read(PEX_CFG_DIRECT_ACCESS(
-pex_idx,
-
