Re: [PATCH v7 3/3] PCI: rockchip: drive at 2.5 GT/s, error other speeds

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Spurious warning emitted when the optional 'max-link-speed' device tree property is absent.
- [Low] The parsed `max-link-speed` value stored in `rockchip->link_gen` is dead code and entirely ignored during link initialization.
--

commit 3b268bac2a052935f203b8d3ae0eea857d63f07b
Author: Geraldo Nascimento <geraldogabriel@gmail.com>
PCI: rockchip: drive at 2.5 GT/s, error other speeds

This commit configures the Rockchip PCIe core to drive at a maximum link
speed of 2.5 GT/s to prevent potential data loss or hardware issues. It
ignores other configured speeds with a warning and removes 5.0 GT/s defines.

Link: https://lore.kernel.org/all/ffd05070-9879-4468-94e3-b88968b4c21b@rock-chips.com/

> diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
> index 0f88da3788054..456dcfd676ed7 100644
> --- a/drivers/pci/controller/pcie-rockchip.c
> +++ b/drivers/pci/controller/pcie-rockchip.c
> @@ -66,8 +66,10 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
>  	}
>  
>  	rockchip->link_gen = of_pci_get_max_link_speed(node);
> -	if (rockchip->link_gen < 0 || rockchip->link_gen > 2)
> -		rockchip->link_gen = 2;
> +	if (rockchip->link_gen < 0 || rockchip->link_gen >= 2) {
> +		rockchip->link_gen = 1;
> +		dev_warn(dev, "invalid max-link-speed, limited to 2.5 GT/s\n");
> +	}

[Severity: Medium]
Since the max-link-speed device tree property is optional,
of_pci_get_max_link_speed() will return a negative error code such as -EINVAL
when it is absent. Does this mean the rockchip->link_gen < 0 check will cause
a spurious warning on every boot for perfectly valid device trees that
omit this property?

>  
>  	for (i = 0; i < ROCKCHIP_NUM_PM_RSTS; i++)
>  		rockchip->pm_rsts[i].id = rockchip_pci_pm_rsts[i];

[ ... ]

> @@ -147,12 +149,8 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>  		goto err_exit_phy;
>  	}
>  
> -	if (rockchip->link_gen == 2)
> -		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_2,
> -				    PCIE_CLIENT_CONFIG);
> -	else
> -		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_1,
> -				    PCIE_CLIENT_CONFIG);
> +	rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_1,
> +			    PCIE_CLIENT_CONFIG);

[Severity: Low]
Does this unconditional write make the rockchip->link_gen value parsed in
rockchip_pcie_parse_dt() obsolete? If link_gen is no longer used during link
initialization, could the device tree parsing logic and state be removed
entirely to simplify the code?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1781622998.git.geraldogabriel@gmail.com?part=3