Re: [PATCH v2 02/15] PCI: rcar-gen4: Check live link status in link_up()

[Koichiro Den]

On Sat, Oct 03, 2026 at 08:51:58PM +0200, Marek Vasut wrote:
> On 10/3/26 8:29 PM, Marek Vasut wrote:
> > On 9/28/26 6:52 PM, Koichiro Den wrote:
> > > rcar_gen4_pcie_link_up() checks link state using SMLH_LINK_UP and
> > > RDLH_LINK_UP in PCIEINTSTS0. However, these bits do not reflect the live
> > > link state. On an R-Car S4, after taking down the endpoint, a link-down
> > > interrupt saw PCIEINTSTS0 = 0x20a000c5 with both bits still set. Even
> > > after resetting the controller with the LTSSM back in Polling, they read
> > > 0xa000c5, still set.
> > > 
> > > As a result, dw_pcie_link_up() keeps reporting the link as up after it
> > > has gone down. That defeats the check in dw_pcie_other_conf_map_bus(),
> > > which is supposed to stop config accesses to downstream devices while
> > > the link is down, so such accesses go out on the dead link and stall the
> > > host. It also makes the callback useless for the link-down recovery
> > > added later, which has to wait for the link to actually come back after
> > > resetting the controller.
> > > 
> > > Keep the APP link-up event check from Figure 104.5 of the R-Car S4
> > > reference manual, but also require PORT_DEBUG1 to report the link up and
> > > not in training. The callback then rejects a downed link even if the APP
> > > link-up events remain latched.
> > > 
> > > Clear the APP latches before enabling LTSSM to discard events from a
> > > previous start, and only read them in .link_up(). RC startup uses
> > > dw_pcie_wait_for_link() to poll the combined condition.
> > > 
> > > Fixes: 0d0c551011df ("PCI: rcar-gen4: Add R-Car Gen4 PCIe controller
> > > support for host mode")
> > > Suggested-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxxxx>
> > > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > 

Hi Marek,

> > I apologize for the late reply.
> > 
> > Reviewed-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxxxx>
> > Tested-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxxxx> # R-Car V4H

Thanks for the review and for testing on V4H.

> > 
> > Since next 20261002 now contains commit
> > 
> > f29719b5064c ("PCI: dwc: Align register macros with Synopsys
> > documentation")
> > 
> > This will need the following slight adjustment:
> > 
> > "s@PCIE_PORT_DEBUG@PORT_LINK_DEBUG@g"
> > 
> > "
> > -       val = dw_pcie_readl_dbi(dw, PCIE_PORT_DEBUG1);
> > -       return (val & PCIE_PORT_DEBUG1_LINK_UP) &&
> > -              !(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING);
> > +       val = dw_pcie_readl_dbi(dw, PORT_LINK_DEBUG1);
> > +       return (val & PORT_LINK_DEBUG1_LINK_UP) &&
> > +              !(val & PORT_LINK_DEBUG1_LINK_IN_TRAINING);
> > "

Thanks for the heads-up. v3 will be rebased onto the latest -next and use the
new names.

> 
> A small nitpick, would the following change make sense to reduce duplication
> a bit ?

Yes. In v3 I'm planning to add it as a separate patch right before this one,
with your Suggested-by.

Best regards,
Koichiro

> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> b/drivers/pci/controller/dwc/pcie-designware.c
> index c726aa71c830c..52bdb1fcc2db2 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -817,17 +817,23 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_wait_for_link);
> 
> -bool dw_pcie_link_up(struct dw_pcie *pci)
> +bool dw_pcie_link_up_debug_check(struct dw_pcie *pci)
>  {
>         u32 val;
> 
> -       if (pci->ops && pci->ops->link_up)
> -               return pci->ops->link_up(pci);
> -
>         val = dw_pcie_readl_dbi(pci, PORT_LINK_DEBUG1);
>         return ((val & PORT_LINK_DEBUG1_LINK_UP) &&
>                 (!(val & PORT_LINK_DEBUG1_LINK_IN_TRAINING)));
>  }
> +EXPORT_SYMBOL_GPL(dw_pcie_link_up_debug_check);
> +
> +bool dw_pcie_link_up(struct dw_pcie *pci)
> +{
> +       if (pci->ops && pci->ops->link_up)
> +               return pci->ops->link_up(pci);
> +
> +       return dw_pcie_link_up_debug_check(pci);
> +}
>  EXPORT_SYMBOL_GPL(dw_pcie_link_up);
> 
>  void dw_pcie_upconfig_setup(struct dw_pcie *pci)
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> b/drivers/pci/controller/dwc/pcie-designware.h
> index 4199324882800..2ce61709b58c2 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -618,6 +618,7 @@ int dw_pcie_write(void __iomem *addr, int size, u32
> val);
>  u32 dw_pcie_read_dbi(struct dw_pcie *pci, u32 reg, size_t size);
>  void dw_pcie_write_dbi(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
>  void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32
> val);
> +bool dw_pcie_link_up_debug_check(struct dw_pcie *pci);
>  bool dw_pcie_link_up(struct dw_pcie *pci);
>  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
>  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index dbc0115885afc..b2ed1a329c418 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> @@ -127,9 +127,7 @@ static bool rcar_gen4_pcie_link_up(struct dw_pcie *dw)
>                 return false;
> 
>         /* The APP link-up events remain latched after the link goes down.
> */
> -       val = dw_pcie_readl_dbi(dw, PORT_LINK_DEBUG1);
> -       return (val & PORT_LINK_DEBUG1_LINK_UP) &&
> -              !(val & PORT_LINK_DEBUG1_LINK_IN_TRAINING);
> +       return dw_pcie_link_up_debug_check(dw);
>  }
> 
>  /*