linux-kernel - Re: [PATCH v8 4/4] PCI: dwc: Add support for configuring lane equalization presets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ed8a59ce-0527-4514-91f8-c27972d799d4@oss.qualcomm.com>
Date: Sat, 29 Mar 2025 12:42:02 +0100
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Cc: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
 <conor+dt@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>, Jingoo Han <jingoohan1@...il.com>,
        Lorenzo Pieralisi
 <lpieralisi@...nel.org>,
        Krzysztof Wilczyński
 <kw@...ux.com>,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        quic_mrana@...cinc.com, quic_vbadigan@...cinc.com
Subject: Re: [PATCH v8 4/4] PCI: dwc: Add support for configuring lane
 equalization presets

On 3/29/25 10:39 AM, Manivannan Sadhasivam wrote:
> On Sat, Mar 29, 2025 at 09:59:46AM +0100, Konrad Dybcio wrote:
>> On 3/29/25 7:30 AM, Manivannan Sadhasivam wrote:
>>> On Fri, Mar 28, 2025 at 10:53:19PM +0100, Konrad Dybcio wrote:
>>>> On 3/28/25 7:45 AM, Manivannan Sadhasivam wrote:
>>>>> On Fri, Mar 28, 2025 at 11:04:11AM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>
>>>>>>
>>>>>> On 3/28/2025 10:23 AM, Manivannan Sadhasivam wrote:
>>>>>>> On Sun, Mar 16, 2025 at 09:39:04AM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>> PCIe equalization presets are predefined settings used to optimize
>>>>>>>> signal integrity by compensating for signal loss and distortion in
>>>>>>>> high-speed data transmission.
>>>>>>>>
>>>>>>>> Based upon the number of lanes and the data rate supported, write
>>>>>>>> the preset data read from the device tree in to the lane equalization
>>>>>>>> control registers.
>>>>>>>>
>>>>>>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
>>>>>>>> ---
>>>>>>>>   drivers/pci/controller/dwc/pcie-designware-host.c | 60 +++++++++++++++++++++++
>>>>>>>>   drivers/pci/controller/dwc/pcie-designware.h      |  3 ++
>>>>>>>>   include/uapi/linux/pci_regs.h                     |  3 ++
>>>>>>>>   3 files changed, 66 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>>> index dd56cc02f4ef..7c6e6a74383b 100644
>>>>>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>>> @@ -507,6 +507,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>>>>>>>   	if (pci->num_lanes < 1)
>>>>>>>>   		pci->num_lanes = dw_pcie_link_get_max_link_width(pci);
>>>>>>>> +	ret = of_pci_get_equalization_presets(dev, &pp->presets, pci->num_lanes);
>>>>>>>> +	if (ret)
>>>>>>>> +		goto err_free_msi;
>>>>>>>> +
>>>>>>>>   	/*
>>>>>>>>   	 * Allocate the resource for MSG TLP before programming the iATU
>>>>>>>>   	 * outbound window in dw_pcie_setup_rc(). Since the allocation depends
>>>>>>>> @@ -808,6 +812,61 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>>>>>>>>   	return 0;
>>>>>>>>   }
>>>>>>>> +static void dw_pcie_program_presets(struct dw_pcie_rp *pp, enum pci_bus_speed speed)
>>>>>>>> +{
>>>>>>>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>>>>>> +	u8 lane_eq_offset, lane_reg_size, cap_id;
>>>>>>>> +	u8 *presets;
>>>>>>>> +	u32 cap;
>>>>>>>> +	int i;
>>>>>>>> +
>>>>>>>> +	if (speed == PCIE_SPEED_8_0GT) {
>>>>>>>> +		presets = (u8 *)pp->presets.eq_presets_8gts;
>>>>>>>> +		lane_eq_offset =  PCI_SECPCI_LE_CTRL;
>>>>>>>> +		cap_id = PCI_EXT_CAP_ID_SECPCI;
>>>>>>>> +		/* For data rate of 8 GT/S each lane equalization control is 16bits wide*/
>>>>>>>> +		lane_reg_size = 0x2;
>>>>>>>> +	} else if (speed == PCIE_SPEED_16_0GT) {
>>>>>>>> +		presets = pp->presets.eq_presets_Ngts[EQ_PRESET_TYPE_16GTS - 1];
>>>>>>>> +		lane_eq_offset = PCI_PL_16GT_LE_CTRL;
>>>>>>>> +		cap_id = PCI_EXT_CAP_ID_PL_16GT;
>>>>>>>> +		lane_reg_size = 0x1;
>>>>>>>> +	} else {
>>>>>>>
>>>>>>> Can you add conditions for other data rates also? Like 32, 64 GT/s. If
>>>>>>> controller supports them and if the presets property is defined in DT, then you
>>>>>>> should apply the preset values.
>>>>>>>
>>>>>>> If the presets property is not present in DT, then below 'PCI_EQ_RESV' will
>>>>>>> safely return.
>>>>>>>
>>>>>> I am fine to add it, but there is no GEN5 or GEN6 controller support
>>>>>> added in dwc, isn't it best to add when that support is added and
>>>>>> tested.
>>>>>>
>>>>>
>>>>> What is the guarantee that this part of the code will be updated once the
>>>>> capable controllers start showing up? I don't think there will be any issue in
>>>>> writing to these registers.
>>>>
>>>> Let's not make assumptions about the spec of a cross-vendor mass-deployed IP
>>>>
>>>
>>> I have seen the worse... The problem is, if those controllers start to show up
>>> and define preset properties in DT, there will be no errors whatsoever to
>>> indicate that the preset values were not applied, resulting in hard to debug
>>> errors.
>>
>> else {
>> 	dev_warn(pci->dev, "Missing equalization presets programming sequence\n");
>> }
>>
> 
> Then we'd warn for controllers supporting GEN5 or more if they do not pass the
> presets property (which is optional).

Ohh, I didn't think about that - and I can only think about solutions that are
rather janky.. with perhaps the least janky one being changing the else case I
proposed above into:

else if (speed >= PCIE_SPEED_32_0GT && eq_presets_Ngts[speed - PCIE_SPEED_16_0GT][0] != PCI_EQ_RESV) {
	...
}> 
>>>
>>> I'm not forseeing any issue in this part of the code to support higher GEN
>>> speeds though.
>>
>> I would hope so as well, but both not programming and misprogramming are
>> equally hard to detect
>>
> 
> I don't disagree. I wanted to have it since there is no sensible way of warning
> users that this part of the code needs to be updated in the future.

I understand, however I'm worried that the programming sequence or register
may change for higher speeds in a way that would be incompatible with what
we assume here

Konrad