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

[Marek Vasut <marek.vasut@mailbox.org>]

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@mailbox.org>
>> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> 
> I apologize for the late reply.
> 
> Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Tested-by: Marek Vasut <marek.vasut+renesas@mailbox.org> # R-Car 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);
> "

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

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);
  }

  /*