linux-kernel - Re: [PATCH v1 2/3] PCI: dwc: add equalization settings for gen4

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] [day] [month] [year] [list]

Message-ID: <6eafdda1-76e1-49af-ae4b-ffe7b26097ca@quicinc.com>
Date: Tue, 19 Mar 2024 04:07:13 -0700
From: Shashank Babu Chinta Venkata <quic_schintav@...cinc.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "agross@...nel.org" <agross@...nel.org>,
        "andersson@...nel.org"
	<andersson@...nel.org>,
        "konrad.dybcio@...aro.org"
	<konrad.dybcio@...aro.org>,
        "mani@...nel.org" <mani@...nel.org>,
        "Mrinmay
 Sarkar (QUIC)" <quic_msarkar@...cinc.com>,
        "Aravind Ramakrishnaiah (QUIC)"
	<quic_kraravin@...cinc.com>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Rob Herring
	<robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
        "manivannan.sadhasivam@...aro.org" <manivannan.sadhasivam@...aro.org>,
        "Serge
 Semin" <fancer.lancer@...il.com>,
        Yoshihiro Shimoda
	<yoshihiro.shimoda.uh@...esas.com>,
        Josh Triplett <josh@...htriplett.org>,
        Conor Dooley <conor.dooley@...rochip.com>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "linux-pci@...r.kernel.org"
	<linux-pci@...r.kernel.org>,
        "linux-arm-msm@...r.kernel.org"
	<linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH v1 2/3] PCI: dwc: add equalization settings for gen4


On 3/1/24 12:00, Bjorn Helgaas wrote:
> On Thu, Feb 29, 2024 at 09:11:35PM -0800, Shashank Babu Chinta Venkata wrote:
>> GEN3_RELATED_OFFSET is being used as shadow register for generation4 and
>> generation5 data rates based on rate select mask settings on this register.
>> Select relevant mask and equalization settings for generation4 operation.
> 
> Please capitalize subject lines to match history ("PCI: qcom: Add ...")
> 
> s/GEN3_RELATED_OFFSET/GEN3_RELATED_OFF/ (I think?)
> 
> I wish these "GEN3_RELATED" things were named with the data rate
> instead of "GEN3".  The PCIe spec defines these things based on the
> data rate (8GT/s, 16GT/s, etc), not the revision of the spec they
> appeared in (gen3/gen4/etc).
I have kept it consistent with nomenclature followed in pcie designware
documentation for these registers. For function names and constraint to 
apply these, I will fall back to data rates rather than generation.

> Using "GEN3" means we have to first look up the "gen -> rate" mapping
> before finding the relevant spec info.
> 
> Applies to the subject line, commit log, #defines, function names,
> etc.
> 
>> +void qcom_pcie_cmn_set_gen4_eq_settings(struct dw_pcie *pci)
>> +{
>> +	u32 reg;
>> +
>> +	reg = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
> 
> Warrants a one-line comment about using "GEN3_..." in a function named
> "..._gen4_..."  (But ideally both would be renamed based on the data
> rate instead.)
> 
>> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
>> @@ -9,10 +9,29 @@
>>   #include "../../pci.h"
>>   #include "pcie-designware.h"
>>   
>> +#define GEN3_EQ_CONTROL_OFF			0x8a8
>> +#define GEN3_EQ_CONTROL_OFF_FB_MODE_MASK        GENMASK(3, 0)
>> +#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE   BIT(4)
>> +#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC_MASK	GENMASK(23, 8)
>> +#define GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL	BIT(24)
> 
> Are these qcom-specific registers, or should they be added alongside
> GEN3_RELATED_OFF in pcie-designware.h?
yes, these are designware register offsets. Will move them to designware 
header file. However, the settings are vendor specific.I will park
settings for these in QCOM specific files.
> 
>> +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF          0x8ac
>> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_VAL   0x5
>> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_VAL  0x5
>> +#define GEN3_EQ_FMDC_N_EVALS_VAL          0xD
>> +#define GEN3_EQ_FMDC_T_MIN_PHASE23_MASK         GENMASK(4, 0)
>> +#define GEN3_EQ_FMDC_N_EVALS_MASK               GENMASK(9, 5)
>> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_MASK  GENMASK(13, 10)
>> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_MASK	GENMASK(17, 14)
>> +#define GEN3_EQ_FMDC_N_EVALS_SHIFT			5
>> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_SHIFT		10
>> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_SHIFT	14
> 
>> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> @@ -438,6 +438,10 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
>>   		goto err_disable_resources;
>>   	}
>>   
>> +	/* set Gen4 equalization settings */
> 
> Pointless comment.
> 
>> +	if (pci->link_gen == 4)
>> +		qcom_pcie_cmn_set_gen4_eq_settings(pci);
> 
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -263,6 +263,10 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
>>   {
>>   	struct qcom_pcie *pcie = to_qcom_pcie(pci);
>>   
>> +	/* set Gen4 equalization settings */
> 
> Pointless comment.
> 
>> +	if (pci->link_gen == 4)
>> +		qcom_pcie_cmn_set_gen4_eq_settings(pci);