On Wed, Apr 09, 2025 at 02:40:33PM +0800, Shawn Lin wrote:
> Two mistakes here:
> 1. 0x11 is L0 not L0S, so the naming is wrong from the very beginning.
> 2. It's totally broken if enabling ASPM as rockchip_pcie_link_up() treat other
> states, for instance, L0S or L1 as link down which is obvioult wrong.
>
> Remove the check.
>
> Signed-off-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
> ---
>
> drivers/pci/controller/dwc/pcie-dw-rockchip.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index c624b7e..21dc99c 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -44,7 +44,6 @@
> #define PCIE_LINKUP (PCIE_SMLH_LINKUP | PCIE_RDLH_LINKUP)
> #define PCIE_RDLH_LINK_UP_CHGED BIT(1)
> #define PCIE_LINK_REQ_RST_NOT_INT BIT(2)
> -#define PCIE_L0S_ENTRY 0x11
> #define PCIE_CLIENT_GENERAL_CONTROL 0x0
> #define PCIE_CLIENT_INTR_STATUS_LEGACY 0x8
> #define PCIE_CLIENT_INTR_MASK_LEGACY 0x1c
> @@ -177,8 +176,7 @@ static int rockchip_pcie_link_up(struct dw_pcie *pci)
> struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
> u32 val = rockchip_pcie_get_ltssm(rockchip);
>
> - if ((val & PCIE_LINKUP) == PCIE_LINKUP &&
> - (val & PCIE_LTSSM_STATUS_MASK) == PCIE_L0S_ENTRY)
> + if ((val & PCIE_LINKUP) == PCIE_LINKUP)
> return 1;
>
> return 0;
> --
> 2.7.4
>
You should probably also add:
Fixes: 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
Considering that dw_pcie_link_up() looks like this:
https://github.com/torvalds/linux/blob/v6.15-rc1/drivers/pci/controller/dwc/pcie-designware.c#L714-L725
Why not simply remove the rockchip_pcie_link_up() callback completely?
Is there any advantage of using a rockchip specific way to read link up,
rather than just reading link up via the DWC PCIE_PORT_DEBUG1 register?
Kind regards,
Niklas
