Re: [PATCH v4 2/2] PCI: cadence: Add debugfs property to provide LTSSM status of the PCIe link

[Hans Zhang <18255117159@163.com>]

On 5/14/26 09:39, Manikandan Karunakaran Pillai wrote:
>>>> On 5/13/26 16:34, Aksh Garg wrote:
>>>>
>>>>>>>
>>>>>>
>>>>>> The above LTSSM states are internal LTSSM encoding states and may
>>>>>> not be available for software to use.
>>>>>> The LTSSM states in the document pointed by Aksh (TI Soc) are the
>>>>>> states  available in all cadence controllers.
>>>>>
>>>>> Is this true for HPA IPs as well? The test performed by Hans:
>>>>> root@orangepi6plus:~# cat /sys/kernel/debug/cdns_pcie_a0*/ltss*
>>>>> L0_STATE (0x29)
>>>>> L0_STATE (0x29)
>>>>> L0_STATE (0x29)
>>>>>
>>>>> This implies that the L0_STATE LTSSM state is mapped to 0x29 (41) there,
>>>>> which according to your response is internal LTSSM encoding, and hence
>>>>> the register read should have resulted in 0x10 instead of 0x29.
>>>>>
>>>>
>>>> Hi Aksh,
>>>>
>>>>
>>>> For HPA, my view is similar to that of DWC - it requires a common
>>>> internal LTSSM state. For each of its own Root Port drivers, a
>>>> callback can be used to implement the reading of LTSSM. This part can
>>>> be referred to as the implementation of the function in DWC.
>>>>
>>>>
>>>> static inline enum dw_pcie_ltssm dw_pcie_get_ltssm(struct dw_pcie *pci)
>>>> {
>>>>       u32 val;
>>>>
>>>>       if (pci->ops && pci->ops->get_ltssm)
>>>>           return pci->ops->get_ltssm(pci);
>>>>
>>>>       val = dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0);
>>>>
>>>>       return (enum
>> dw_pcie_ltssm)FIELD_GET(PORT_LOGIC_LTSSM_STATE_MASK,
>>>> val);
>>>> }
>>>>
>>>>
>>>> static int ltssm_status_show(struct seq_file *s, void *v)
>>>> {
>>>>       struct dw_pcie *pci = s->private;
>>>>       enum dw_pcie_ltssm val;
>>>>
>>>>       val = dw_pcie_get_ltssm(pci);
>>>>       seq_printf(s, "%s (0x%02x)\n", dw_pcie_ltssm_status_string(val),
>>>> val);
>>>>
>>>>       return 0;
>>>> }
>>>>
>>>>
>>>>
>>>> For example, it can be modified as follows. Of course, the function
>>>> name will start with "cdns".
>>>>
>>>> For LGA IP, currently we will allow each Root Port driver to implement
>>>> the corresponding ops::get_ltssm() by itself.
>>>>
>>>> static int ltssm_status_show(struct seq_file *s, void *v)
>>>> {
>>>>       struct dw_pcie *pci = s->private;
>>>>       enum dw_pcie_ltssm val;
>>>>
>>>>       if (pci->ops && pci->ops->get_ltssm)
>>>>           val = pci->ops->get_ltssm(pci);
>>>>       else
>>>>           val = dw_pcie_get_ltssm(pci);
>>>>       seq_printf(s, "%s (0x%02x)\n", dw_pcie_ltssm_status_string(val),
>>>> val);
>>>>
>>>>       return 0;
>>>> }
>>>>
>>>
>>> The process above tells how to read the register and get the LTSSM
>>> state values. However, my concern is whether we require different LTSSM
>>> state encoding in your debugfs patch, one for LGA, and other for HPA.
>>> This is because the L0_state seems to have different values in the LTSSM
>>> fields of different IPs. On LGA, the L0_state seems to have value as
>>> 0x10 in the register (as can be seen in the J7200 TRM). On HPA, the
>>> L0_state seems to have value of 0x29 in the register (as can be seen in
>>> your test logs in the cover letter). Hence, if we want to print the
>>> LTSSM state string in the debugfs, then for LGA IPs, 0x10 value should
>>> print L0_STATE, and for HPA IPs, 0x41 value should print L0_state.
>>>
>>
>> Hi Aksh,
>>
>> Yes, different codes will be used. The reason for this was because of
>> the result I obtained from consulting Manikandan. This time, it was a
>> problem identified by Sashiko's review. So, we need to ask you and
>> Manikandan to confirm this LTSSM code again.
>>
>>
>> Best regards,
>> Hans
> 
> The LTSSM codes are the same across HPA and LGA. You have to refer to "External LTSSM state encoding" table in
> the specification given by cadence.
> 

Hi Manikandan,


Are there 128 different states? If they are the same, for the LGA IP, 
which register should I read?

Previously, Sashiko reported a problem with the review. Please click on 
the following link:

https://lore.kernel.org/linux-pci/20260508041956.C8F10C2BCB0@smtp.kernel.org/


"""
 > +#define CDNS_PCIE_LGA_LTSSM_STATUS_MASK	GENMASK(29, 24)

Does this mask need to be extended to cover all possible states?

This mask covers only 6 bits, allowing it to extract values up to 63.
However, the cdns_pcie_ltssm enum defines states up to 127. If the
hardware enters a state 64 or higher, FIELD_GET will silently truncate
the value, making state 65 appear as state 1.

"""


Best regards,
Hans