linux-kernel - Re: [PATCH 3/8] phy: qcom: qmp: Add phy register and clk setting for x1e80100 PCIe3

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: <2c23f7e8-c407-4c5c-a8e2-65be98f9c92b@quicinc.com>
Date: Wed, 28 Aug 2024 17:47:04 +0800
From: Qiang Yu <quic_qianyu@...cinc.com>
To: Konrad Dybcio <konradybcio@...nel.org>, <manivannan.sadhasivam@...aro.org>,
        <vkoul@...nel.org>, <kishon@...nel.org>, <robh@...nel.org>,
        <andersson@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
        <mturquette@...libre.com>, <sboyd@...nel.org>, <abel.vesa@...aro.org>,
        <quic_msarkar@...cinc.com>, <quic_devipriy@...cinc.com>
CC: <dmitry.baryshkov@...aro.org>, <kw@...ux.com>, <lpieralisi@...nel.org>,
        <neil.armstrong@...aro.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-phy@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <linux-pci@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-clk@...r.kernel.org>
Subject: Re: [PATCH 3/8] phy: qcom: qmp: Add phy register and clk setting for
 x1e80100 PCIe3


On 8/27/2024 6:33 PM, Konrad Dybcio wrote:
> On 27.08.2024 8:36 AM, Qiang Yu wrote:
>> Currently driver supports only x4 lane based functionality using tx/rx and
>> tx2/rx2 pair of register sets. To support 8 lane functionality with PCIe3,
>> PCIe3 related QMP PHY provides additional programming which are available
>> as txz and rxz based register set. Hence adds txz and rxz based registers
>> usage and programming sequences. Phy register setting for txz and rxz will
>> be applied to all 8 lanes. Some lanes may have different settings on
>> several registers than txz/rxz, these registers should be programmed after
>> txz/rxz programming sequences completing.
>>
>> Besides, PCIe3 related QMP PHY also requires addtional clk, which is named
>> as clkref_en. Hence, add this clk into qmp_pciephy_clk_l so that it can be
>> easily parsed from devicetree during init.
>>
>> Signed-off-by: Qiang Yu <quic_qianyu@...cinc.com>
>> ---
> [...]
>
>> +static const struct qmp_phy_init_tbl x1e80100_qmp_gen4x8_pcie_rx_tbl[] = {
>> +	QMP_PHY_INIT_CFG_LANE(QSERDES_V6_20_RX_DFE_CTLE_POST_CAL_OFFSET, 0x3a, 1),
> 1 -> BIT(0)
>
> [...]
>
>> +	/* Set to true for programming all 8 lanes using txz/rxz registers */
>> +	bool lane_broadcasting;
> This is unnecessary because you call qmp_configure_lane conditionally,
> but that function has a nullcheck built in
Yes, there is null pointer check in qmp_configure_lane, will remove
lane_broadcating check.
>> +
>>   	/* resets to be requested */
>>   	const char * const *reset_list;
>>   	int num_resets;
>> @@ -2655,6 +2815,8 @@ struct qmp_pcie {
>>   	void __iomem *rx;
>>   	void __iomem *tx2;
>>   	void __iomem *rx2;
>> +	void __iomem *txz;
>> +	void __iomem *rxz;
>>   	void __iomem *ln_shrd;
>>   
>>   	void __iomem *port_b;
>> @@ -2700,7 +2862,7 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
>>   
>>   /* list of clocks required by phy */
>>   static const char * const qmp_pciephy_clk_l[] = {
>> -	"aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux",
>> +	"aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", "clkref_en",
> Why not just put in TCSR_PCIE_8L_CLKREF_EN as "ref"? It's downstream
> of the XO anyway.
Yes, TCSR_PCIE_8L_CLKREF_EN is source from XO, will update patch as
your comments.

Thanks,
Qiang
>
> [...]
>
>>   	const struct qmp_phy_cfg *cfg = qmp->cfg;
>> @@ -3700,6 +3907,11 @@ static void qmp_pcie_init_registers(struct qmp_pcie *qmp, const struct qmp_phy_c
>>   
>>   	qmp_configure(qmp->dev, serdes, tbls->serdes, tbls->serdes_num);
>>   
>> +	if (cfg->lane_broadcasting) {
> All these ifs can be unconditional
>
> Konrad