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

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

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.

> 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 ?

> P.S. I'll rebase v2 onto the latest pci/controller/dwc-rcar-gen4.

Thank you, and I apologize for the inconvenience.

-- 
Best regards,
Marek Vasut