Re: [PATCH v6 3/4] PCI: qcom: Add equalization settings for 16.0 GT/s

[Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>]

On 9/4/24 08:52, Manivannan Sadhasivam wrote:
> On Wed, Sep 04, 2024 at 11:39:09AM +0200, Johan Hovold wrote:
>> On Wed, Sep 04, 2024 at 12:41:59PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
>>> From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
>>>
>>> During high data transmission rates such as 16.0 GT/s, there is an
>>> increased risk of signal loss due to poor channel quality and interference.
>>> This can impact receiver's ability to capture signals accurately. Hence,
>>> signal compensation is achieved through appropriate lane equalization
>>> settings at both transmitter and receiver. This will result in increased
>>> PCIe signal strength.
>>>
>>> Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
>>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> [mani: dropped the code refactoring and minor changes]
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>  
>>> +#define GEN3_EQ_CONTROL_OFF			0x8a8
>>
>> Nit: uppercase hex since that's what is used for the other offsets
>>
>>> +#define GEN3_EQ_CONTROL_OFF_FB_MODE		GENMASK(3, 0)
>>> +#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE	BIT(4)
>>> +#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC	GENMASK(23, 8)
>>> +#define GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL	BIT(24)
>>> +
>>> +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF          0x8ac
>>
>> Nit: odd indentation uses spaces, uppercase
>>
>>> +#define GEN3_EQ_FMDC_T_MIN_PHASE23		GENMASK(4, 0)
>>> +#define GEN3_EQ_FMDC_N_EVALS			GENMASK(9, 5)
>>> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA	GENMASK(13, 10)
>>> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA	GENMASK(17, 14)
>>> +
>>>  #define PCIE_PORT_MULTI_LANE_CTRL	0x8C0
>>>  #define PORT_MLTI_UPCFG_SUPPORT		BIT(7)
>>>  
>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c
>>> new file mode 100644
>>> index 000000000000..dc7d93db9dc5
>>> --- /dev/null
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
>>> @@ -0,0 +1,45 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>> + */
>>> +
>>> +#include <linux/pci.h>
>>> +
>>> +#include "pcie-designware.h"
>>> +#include "pcie-qcom-common.h"
>>> +
>>> +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci)
>>> +{
>>> +	u32 reg;
>>> +
>>> +	/*
>>> +	 * GEN3_RELATED_OFF register is repurposed to apply equalization
>>> +	 * settings at various data transmission rates through registers namely
>>> +	 * GEN3_EQ_*. RATE_SHADOW_SEL bit field of GEN3_RELATED_OFF determines
>>> +	 * data rate for which this equalization settings are applied.
>>
>> *The* RATE_SHADOW_SEL bit field
>>
>> *the* data rate
>>
>> s/this/these/
>>
>>> +	 */
>>> +	reg = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
>>> +	reg &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
>>> +	reg &= ~GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK;
>>> +	reg |= FIELD_PREP(GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK, 0x1);
>>
>> How does 0x1 map to gen4/16 GT?
>>
> 
> I need inputs from Shashank here as I don't know the answer.
> 
> - Mani

GEN3_RELATED_OFF has been repurposed to use with multiple data rates. RATE_SHADOW_SEL_MASK on GEN3_RELATED_OFF value decides the data rate of shadow registers namely GEN3_EQ_* registers. Per documentation 0x0 maps to 8 GT/s, 0x1 maps to 16 GT/s and 0x2 maps to 32 GT/s. 
> 
>>> +	dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, reg);
>>> +
>>> +	reg = dw_pcie_readl_dbi(pci, GEN3_EQ_FB_MODE_DIR_CHANGE_OFF);
>>> +	reg &= ~(GEN3_EQ_FMDC_T_MIN_PHASE23 |
>>> +		GEN3_EQ_FMDC_N_EVALS |
>>> +		GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA |
>>> +		GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA);
>>> +	reg |= FIELD_PREP(GEN3_EQ_FMDC_T_MIN_PHASE23, 0x1) |
>>> +		FIELD_PREP(GEN3_EQ_FMDC_N_EVALS, 0xd) |
>>> +		FIELD_PREP(GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA, 0x5) |
>>> +		FIELD_PREP(GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA, 0x5);
>>> +	dw_pcie_writel_dbi(pci, GEN3_EQ_FB_MODE_DIR_CHANGE_OFF, reg);
>>> +
>>> +	reg = dw_pcie_readl_dbi(pci, GEN3_EQ_CONTROL_OFF);
>>> +	reg &= ~(GEN3_EQ_CONTROL_OFF_FB_MODE |
>>> +		GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE |
>>> +		GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL |
>>> +		GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC);
>>> +	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, reg);
>>> +}
>>> +EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_eq_settings);
>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h
>>> new file mode 100644
>>> index 000000000000..259e04b7bdf9
>>> --- /dev/null
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
>>> @@ -0,0 +1,8 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>> + */
>>> +
>>> +#include "pcie-designware.h"
>>
>> You only need a forward declaration:
>>
>> 	struct dw_pcie;
>>
>>> +
>>> +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci);
>>
>> Compile guard still missing.
>>
>> Johan
>