Re: [PATCH 02/11] PCI: rcar-gen4: Drop the APP-based link_up check

[Koichiro Den]

On Sun, Sep 27, 2026 at 09:59:50PM +0200, Marek Vasut wrote:
> On 9/23/26 4:56 PM, Koichiro Den wrote:
> 
> Hello Den-san,
> 
> > > > @@ -298,7 +285,6 @@ static int rcar_gen4_pcie_get_resources(struct rcar_gen4_pcie *rcar)
> > > >    static const struct dw_pcie_ops dw_pcie_ops = {
> > > >    	.start_link = rcar_gen4_pcie_start_link,
> > > >    	.stop_link = rcar_gen4_pcie_stop_link,
> > > > -	.link_up = rcar_gen4_pcie_link_up,
> > > >    };
> > > >    static struct rcar_gen4_pcie *rcar_gen4_pcie_alloc(struct platform_device *pdev)
> > > 
> > > Can we include some form of the draft patch below, so the S4 Reference
> > > Manual rev.1.40 , page 1564 , Figure 104.5 Initial Setting of PCIEC ,
> > > bottommost diamond in the figure (smlh_link_up and rdlh_link_up = 1 test),
> > > would still be fulfilled, and the initialization code in the driver would
> > > not diverge from the initialization sequence listed in the reference manual
> > > ? What do you think ?
> > 
> > If always relying on the PORT_DEBUG1 check instead of the SMLH/RDLH check does
> > not introduce any regressions, I'd personally prefer to keep patch 2 as-is
> > because it keeps the code simpler. We could just add a comment noting that this
> > link-up check diverges from Figure 104.5.
> 
> I think the SMLH/RDLH does behave slightly differently, because those
> SMLH/RDLH bits are set and latched in, and they have to be explicitly
> cleared. The DEBUG1 bits report the current state of the link, which might
> (?) be susceptible to bouncing (link going out and down in a short window,
> but ultimately being up) ? I am not entirely sure whether that might pose a
> problem or not.

Hm, I have no idea whether the DEBUG1 bits are succesptible to bouncing.

> 
> > That said, I agree that in general we should follow the R-Car reference manual
> > where possible, and your draft makes sense for that purpose. If we go that way,
> > I have one question: would we need a polling loop with a timeout
> > (PCIE_LINK_WAIT_MAX_RETRIES * PCIE_LINK_WAIT_SLEEP_MS) in
> > rcar_gen4_pcie_start_link(), similar to dw_pcie_wait_for_link()?
> 
> This is a good point, and I think we probably shouldn't do it this way,
> because we can reuse the DWC core code for that purpose.
> 
> How about extending the .link_up callback, and check both the SMLH/RDLH bits
> there (to fulfill the datasheet compliance, dw_pcie_start_link() is always
> followed by dw_pcie_wait_for_link() which calls the .link_up() callback) and
> the DEBUG1 bits (to make sure we check the current state of the link) ?
> 
> That should cover all our concerns (datasheet compliance, DEBUG1 current
> state of link check, polling), shouldn't it ?

If you mean AND-ing the SMLH/RDLH check with the DEBUG1 check, that sounds
reasonable. We could clear the APP latches in .start_link(), before enabling
LTSSM, and leave them latched across .link_up() calls.
Thanks for the suggestion!

> 
> > P.S. I'll rebase v2 onto the latest pci/controller/dwc-rcar-gen4.
> 
> Thank you, and I apologize for the inconvenience.

No worries at all. I just hadn't caught up with your X5H series.
I've now rebased v2 onto next-20260925, since the series also needs
b43aa6a6ebe8 ("arm64: dts: renesas: r8a779f0: Add GICv3 ITS and update PCIe nodes"),
as noted in the cover letter.

Best regards,
Koichiro Den

> 
> -- 
> Best regards,
> Marek Vasut