On 24.09.21 22:59, Marek Behún wrote:
From: Pali Rohár pali@kernel.org
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 pali@kernel.org Reviewed-by: Marek Behún marek.behun@nic.cz
Reviewed-by: Stefan Roese sr@denx.de
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;- 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,PEX_LINK_CTRL_STAT_REG)) &0xf0000) >> 16;/* Check if the link established is GEN1 */DEBUG_INIT_FULL_S("Checking if the link established is gen1\n");if (temp_reg != 0x1)continue;pex_local_bus_num_set(pex_idx, first_busno);pex_local_dev_num_set(pex_idx, 1);DEBUG_INIT_FULL_S("PCIe, Idx ");DEBUG_INIT_FULL_D(pex_idx, 1);DEBUG_INIT_S(":** Link is Gen1, check the EP capability\n");/* link is Gen1, check the EP capability */addr = pex_config_read(pex_idx, first_busno, 0, 0, 0x34) & 0xff;DEBUG_INIT_FULL_C("pex_config_read: return addr=0x%x", addr, 4);if (addr == 0xff) {DEBUG_INIT_FULL_C("pex_config_read: return 0xff -->PCIe (%d): Detected No Link.",pex_idx, 1);continue;}/* Find start of the PCI Express Capability registers */while ((pex_config_read(pex_idx, first_busno, 0, 0, addr)& 0xff) != 0x10) {addr = (pex_config_read(pex_idx, first_busno, 0,0, addr) & 0xff00) >> 8;}/* Check for Gen2 and above */if ((pex_config_read(pex_idx, first_busno, 0, 0,addr + 0xc) & 0xf) < 0x2) {DEBUG_INIT_S("PCIe, Idx ");DEBUG_INIT_D(pex_idx, 1);DEBUG_INIT_S(": remains Gen1\n");continue;}tmp = reg_read(PEX_LINK_CTRL_STATUS2_REG(pex_idx));DEBUG_RD_REG(PEX_LINK_CTRL_STATUS2_REG(pex_idx), tmp);tmp &= ~(BIT(0) | BIT(1));tmp |= BIT(1); /* Force Target Link Speed to 5.0 GT/s */tmp |= BIT(6); /* Select Deemphasize (-3.5d_b) */reg_write(PEX_LINK_CTRL_STATUS2_REG(pex_idx), tmp);DEBUG_WR_REG(PEX_LINK_CTRL_STATUS2_REG(pex_idx), tmp);/** Enable Auto Speed change. When set, link will issue link* speed change to max possible link speed.*/tmp = reg_read(PEX_CTRL_REG(pex_idx));DEBUG_RD_REG(PEX_CTRL_REG(pex_idx), tmp);tmp |= BIT(10);reg_write(PEX_CTRL_REG(pex_idx), tmp);DEBUG_WR_REG(PEX_CTRL_REG(pex_idx), tmp);/** We need to wait 10ms before reading the PEX_DBG_STATUS_REG* in order not to read the status of the former state*/mdelay(10);DEBUG_INIT_S("PCIe, Idx ");DEBUG_INIT_D(pex_idx, 1);DEBUG_INIT_S(": Link upgraded to Gen2 based on client capabilities\n");}
return MV_OK; }
Viele Grüße, Stefan