From: Stefan Roese <sr@denx.de>
To: "Marek Behún" <marek.behun@nic.cz>
Cc: u-boot@lists.denx.de, pali@kernel.org
Subject: Re: [PATCH u-boot-marvell 7/9] arm: mvebu: a38x: serdes: Don't configure PCIe cards in SerDes init code
Date: Fri, 8 Oct 2021 08:29:34 +0200 [thread overview]
Message-ID: <73734348-c131-037a-1d1b-00fbdd9cc267@denx.de> (raw)
In-Reply-To: <20210924205922.25432-8-marek.behun@nic.cz>
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;
> 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,
> - 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
--
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
next prev parent reply other threads:[~2021-10-08 6:29 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-24 20:59 [PATCH u-boot-marvell 0/9] a38x serdes cleanup Marek Behún
2021-09-24 20:59 ` [PATCH u-boot-marvell 1/9] arm: mvebu: a38x: serdes: Add comments and use macros in PCIe code Marek Behún
2021-10-08 6:24 ` Stefan Roese
2021-09-24 20:59 ` [PATCH u-boot-marvell 2/9] arm: mvebu: a38x: serdes: Remove duplicate macro SOC_CTRL_REG Marek Behún
2021-10-08 6:26 ` Stefan Roese
2021-09-24 20:59 ` [PATCH u-boot-marvell 3/9] arm: mvebu: a38x: serdes: Add comments for hws_pex_config() code Marek Behún
2021-10-08 6:27 ` Stefan Roese
2021-09-24 20:59 ` [PATCH u-boot-marvell 4/9] arm: mvebu: a38x: serdes: Don't overwrite read-only SAR PCIe registers Marek Behún
2021-10-08 6:27 ` Stefan Roese
2021-09-24 20:59 ` [PATCH u-boot-marvell 5/9] arm: mvebu: a38x: serdes: Don't set PCIe Common Clock Configuration Marek Behún
2021-10-08 6:28 ` Stefan Roese
2021-09-24 20:59 ` [PATCH u-boot-marvell 6/9] arm: mvebu: a38x: serdes: Don't overwrite PCI device ID Marek Behún
2021-10-08 6:28 ` Stefan Roese
2021-09-24 20:59 ` [PATCH u-boot-marvell 7/9] arm: mvebu: a38x: serdes: Don't configure PCIe cards in SerDes init code Marek Behún
2021-10-08 6:29 ` Stefan Roese [this message]
2021-09-24 20:59 ` [PATCH u-boot-marvell 8/9] arm: mvebu: a38x: serdes: Remove unused PCIe macros and functions Marek Behún
2021-10-08 6:29 ` Stefan Roese
2021-09-24 20:59 ` [PATCH u-boot-marvell 9/9] arm: mvebu: a38x: serdes: Update comment about PCIE*_ENABLE_* defines Marek Behún
2021-10-08 6:30 ` Stefan Roese
2021-10-08 6:31 ` [PATCH u-boot-marvell 0/9] a38x serdes cleanup Stefan Roese
2021-10-08 9:18 ` Stefan Roese
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=73734348-c131-037a-1d1b-00fbdd9cc267@denx.de \
--to=sr@denx.de \
--cc=marek.behun@nic.cz \
--cc=pali@kernel.org \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).