Message-ID: <utswwqjgfy3iybt54ilyqnfss77vzit7kegctjp3tef636hc3p@724xe3dzlpip>
Date: Wed, 2 Apr 2025 11:32:16 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: 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 Sat, Mar 29, 2025 at 12:42:02PM +0100, Konrad Dybcio wrote:
> 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) {

s/PCIE_SPEED_16_0GT/PCIE_SPEED_32_0GT

> 	...

So this I read as: Oh, your controller supports 32 GT/s and you firmware also
wanted to apply the custom preset offsets, but sorry we didn't do it because we
don't know if it would work or not. So please let us know so that we can work
with you test it and then finally we can apply the presets.

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

Honestly, I don't know why you are having this opinion. This piece of code is
not in Qcom driver and the registers are the same for 8 GT/s, 16 GT/s as per the
PCIe spec. So the hardware programming sequence and other arguments doesn't
apply here (atleast to me).

- Mani

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

Powered by blists - more mailing lists