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

[Manivannan Sadhasivam]

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@xxxxxxxxxxxxxxxx>
> >>>>>> ---
> >>>>>>   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).

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

- Mani

-- 
மணிவண்ணன் சதாசிவம்