Re: [PATCH 1/2] phy: qcom: qmp-pcie: Update PHY settings for SA8775P

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
To: Mrinmay Sarkar <mrinmay.sarkar@oss.qualcomm.com>,
	Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Cc: Vinod Koul <vkoul@kernel.org>,
	Kishon Vijay Abraham I <kishon@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konradybcio@kernel.org>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-phy@lists.infradead.org, devicetree@vger.kernel.org,
	krishna.chundru@oss.qualcomm.com, quic_vbadigan@quicinc.com,
	quic_nayiluri@quicinc.com, quic_ramkri@quicinc.com,
	quic_nitegupt@quicinc.com
Subject: Re: [PATCH 1/2] phy: qcom: qmp-pcie: Update PHY settings for SA8775P
Date: Fri, 2 May 2025 15:41:19 +0200	[thread overview]
Message-ID: <f6d9b03d-3870-494d-b97f-b0487927c9d3@oss.qualcomm.com> (raw)
In-Reply-To: <CAMyL0qMQCGE-JNwTjqJk8TrGLBDr_1W7Sv_nECkW9Xp1fXgeuw@mail.gmail.com>

On 5/2/25 2:25 PM, Mrinmay Sarkar wrote:
> On Wed, Apr 23, 2025 at 7:07 PM Dmitry Baryshkov <
> dmitry.baryshkov@oss.qualcomm.com> wrote:
> 
>> On Wed, Apr 23, 2025 at 04:45:43PM +0530, Mrinmay Sarkar wrote:
>>> This change updates the PHY settings to align with the latest
>>> PCIe PHY Hardware Programming Guide for both PCIe controllers
>>> on the SA8775P platform.
>>
>> Please read Documentation/process/submitting-patches.rst, look for
>> '[This patch] makes xyzzy'.
>>
>>>
>>> Signed-off-by: Mrinmay Sarkar <mrinmay.sarkar@oss.qualcomm.com>
>>> ---
>>>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c           | 89
>> ++++++++++++----------
>>>  drivers/phy/qualcomm/phy-qcom-qmp-pcs-pcie-v5_20.h |  2 +
>>>  drivers/phy/qualcomm/phy-qcom-qmp-pcs-v5_20.h      |  4 +
>>>  .../phy/qualcomm/phy-qcom-qmp-qserdes-ln-shrd-v5.h | 11 +++
>>>  drivers/phy/qualcomm/phy-qcom-qmp.h                |  1 +
>>>  5 files changed, 66 insertions(+), 41 deletions(-)
>>>
>>> @@ -3191,6 +3194,7 @@ static const struct qmp_pcie_offsets
>> qmp_pcie_offsets_v5_20 = {
>>>       .rx             = 0x0200,
>>>       .tx2            = 0x0800,
>>>       .rx2            = 0x0a00,
>>> +     .ln_shrd        = 0x0e00,
>>>  };
>>
>> This does more than just updating PHY sequences. ln_shrd-related changes
>> should go into a separate commit.
>>
>>     Hi Dmitry,
>     thanks for the review.
>     Actually in the previous phy version there was no ln_shrd related
> register write.
>     and only one ln_shrd related register write introduced to the latest
> phy version.
>     so introduce added ln_shrd with phy update.
> 
>     I will add a separate change for ln_shrd.

I think it's fine to change them both in a single commit, but you should
explain in the commit message that previously no writes have been made
to that region simply because none were deemed necessary.

Splitting that into two commits will give us an unpredictable programming
of the PHY, where not all parameters are in sync.

Unless that separate commit would be just adding the offset data, but I
think that's form over function really

Konrad

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy