[PATCH] arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280

[PATCH] arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280

[Krishna Kurapati]

Override the SNPS Phy tuning parameters for SC7280 devices. These
values are common for both trogdor and herobrine variants.

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 2125803..ae2c23e 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -3310,6 +3310,13 @@
 			clock-names = "ref";
 
 			resets = <&gcc GCC_QUSB2PHY_SEC_BCR>;
+
+			qcom,hs-rise-fall-time-bp = <0>;
+			qcom,squelch-detector-bp = <(-2090)>;
+			qcom,hs-disconnect-bp = <1743>;
+			qcom,hs-amplitude-bp = <1780>;
+			qcom,hs-crossover-voltage-microvolt = <(-31000)>;
+			qcom,hs-output-impedance-micro-ohms = <2600000>;
 		};
 
 		usb_1_qmpphy: phy-wrapper@88e9000 {
-- 
2.7.4

Re: [PATCH] arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280

[Krzysztof Kozlowski]

On 29/09/2022 09:13, Krishna Kurapati wrote:
> Override the SNPS Phy tuning parameters for SC7280 devices. These
> values are common for both trogdor and herobrine variants.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH] arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280

[Krishna Kurapati PSSNV]

On 9/29/2022 1:20 PM, Krzysztof Kozlowski wrote:
> On 29/09/2022 09:13, Krishna Kurapati wrote:
>> Override the SNPS Phy tuning parameters for SC7280 devices. These
>> values are common for both trogdor and herobrine variants.
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
> 
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Best regards,
> Krzysztof
> 

Hi Krzysztof,

  Thanks for the review.

I pushed patch [1] previously for the overriding params for idp 
platform. The first 2 patches of that series are merged. Just wanted to 
request you to ignore the third patch in that series [1] and consider 
this current patch for merging.

[1] : 
https://lore.kernel.org/linux-devicetree/1664435628-4011-1-git-send-email-quic_kriskura@quicinc.com/T/#u

Regards,
Krishna,

Re: [PATCH] arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280

[Dmitry Baryshkov]

On Thu, 29 Sept 2022 at 10:14, Krishna Kurapati
<quic_kriskura@quicinc.com> wrote:
>
> Override the SNPS Phy tuning parameters for SC7280 devices. These
> values are common for both trogdor and herobrine variants.

They are common for trogdor and herobrine, but should these parameters
be a default? In other words, a random new device based on sc7280
would more likely use these overrides or the hardware defaults?

>
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 2125803..ae2c23e 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -3310,6 +3310,13 @@
>                         clock-names = "ref";
>
>                         resets = <&gcc GCC_QUSB2PHY_SEC_BCR>;
> +
> +                       qcom,hs-rise-fall-time-bp = <0>;
> +                       qcom,squelch-detector-bp = <(-2090)>;
> +                       qcom,hs-disconnect-bp = <1743>;
> +                       qcom,hs-amplitude-bp = <1780>;
> +                       qcom,hs-crossover-voltage-microvolt = <(-31000)>;
> +                       qcom,hs-output-impedance-micro-ohms = <2600000>;
>                 };
>
>                 usb_1_qmpphy: phy-wrapper@88e9000 {
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

Re: [PATCH] arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280

[Krishna Kurapati PSSNV]

On 9/29/2022 1:29 PM, Dmitry Baryshkov wrote:
> On Thu, 29 Sept 2022 at 10:14, Krishna Kurapati
> <quic_kriskura@quicinc.com> wrote:
>>
>> Override the SNPS Phy tuning parameters for SC7280 devices. These
>> values are common for both trogdor and herobrine variants.
> 
> They are common for trogdor and herobrine, but should these parameters
> be a default? In other words, a random new device based on sc7280
> would more likely use these overrides or the hardware defaults?
> 
Hi Dmitry,

   Currently there are only two platforms, so I made these changes on 
common dtsi. If a new platform comes (mostly it won't) we can override 
them in platform specific file is what I thought.

Regards,
Krishna,
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sc7280.dtsi | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index 2125803..ae2c23e 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -3310,6 +3310,13 @@
>>                          clock-names = "ref";
>>
>>                          resets = <&gcc GCC_QUSB2PHY_SEC_BCR>;
>> +
>> +                       qcom,hs-rise-fall-time-bp = <0>;
>> +                       qcom,squelch-detector-bp = <(-2090)>;
>> +                       qcom,hs-disconnect-bp = <1743>;
>> +                       qcom,hs-amplitude-bp = <1780>;
>> +                       qcom,hs-crossover-voltage-microvolt = <(-31000)>;
>> +                       qcom,hs-output-impedance-micro-ohms = <2600000>;
>>                  };
>>
>>                  usb_1_qmpphy: phy-wrapper@88e9000 {
>> --
>> 2.7.4
>>
> 
> 

Re: [PATCH] arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280

[Dmitry Baryshkov]

On Thu, 29 Sept 2022 at 11:54, Krishna Kurapati PSSNV
<quic_kriskura@quicinc.com> wrote:
>
>
> On 9/29/2022 1:29 PM, Dmitry Baryshkov wrote:
> > On Thu, 29 Sept 2022 at 10:14, Krishna Kurapati
> > <quic_kriskura@quicinc.com> wrote:
> >>
> >> Override the SNPS Phy tuning parameters for SC7280 devices. These
> >> values are common for both trogdor and herobrine variants.
> >
> > They are common for trogdor and herobrine, but should these parameters
> > be a default? In other words, a random new device based on sc7280
> > would more likely use these overrides or the hardware defaults?
> >
> Hi Dmitry,
>
>    Currently there are only two platforms, so I made these changes on
> common dtsi. If a new platform comes (mostly it won't) we can override
> them in platform specific file is what I thought.

This is not how it usually works. The 'sc7280.dtsi' is not a 'common
dtsi' for trogdor and herobrine. It describes the SoC.
Thus in my opinion if these overrides should be a default to all
sc7280 platforms, this patch is fine. If these overrides are
applicable only to the two mentioned platforms, they should go to
respective platform-specific DT files.

>
> Regards,
> Krishna,
> >>
> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> >> ---
> >>   arch/arm64/boot/dts/qcom/sc7280.dtsi | 7 +++++++
> >>   1 file changed, 7 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >> index 2125803..ae2c23e 100644
> >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >> @@ -3310,6 +3310,13 @@
> >>                          clock-names = "ref";
> >>
> >>                          resets = <&gcc GCC_QUSB2PHY_SEC_BCR>;
> >> +
> >> +                       qcom,hs-rise-fall-time-bp = <0>;
> >> +                       qcom,squelch-detector-bp = <(-2090)>;
> >> +                       qcom,hs-disconnect-bp = <1743>;
> >> +                       qcom,hs-amplitude-bp = <1780>;
> >> +                       qcom,hs-crossover-voltage-microvolt = <(-31000)>;
> >> +                       qcom,hs-output-impedance-micro-ohms = <2600000>;
> >>                  };
> >>
> >>                  usb_1_qmpphy: phy-wrapper@88e9000 {
> >> --
> >> 2.7.4
> >>
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH] arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280

[Krzysztof Kozlowski]

On 29/09/2022 11:02, Dmitry Baryshkov wrote:
> On Thu, 29 Sept 2022 at 11:54, Krishna Kurapati PSSNV
> <quic_kriskura@quicinc.com> wrote:
>>
>>
>> On 9/29/2022 1:29 PM, Dmitry Baryshkov wrote:
>>> On Thu, 29 Sept 2022 at 10:14, Krishna Kurapati
>>> <quic_kriskura@quicinc.com> wrote:
>>>>
>>>> Override the SNPS Phy tuning parameters for SC7280 devices. These
>>>> values are common for both trogdor and herobrine variants.
>>>
>>> They are common for trogdor and herobrine, but should these parameters
>>> be a default? In other words, a random new device based on sc7280
>>> would more likely use these overrides or the hardware defaults?
>>>
>> Hi Dmitry,
>>
>>    Currently there are only two platforms, so I made these changes on
>> common dtsi. If a new platform comes (mostly it won't) we can override
>> them in platform specific file is what I thought.
> 
> This is not how it usually works. The 'sc7280.dtsi' is not a 'common
> dtsi' for trogdor and herobrine. It describes the SoC.
> Thus in my opinion if these overrides should be a default to all
> sc7280 platforms, this patch is fine. If these overrides are
> applicable only to the two mentioned platforms, they should go to
> respective platform-specific DT files.

Dmitry's conclusion is correct here. The true question is whether these
are properties of the SoC itself (so do not depend on the board or board
layout) or these depend on design of board.

Best regards,
Krzysztof

Re: [PATCH] arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280

[Krishna Kurapati PSSNV]

On 9/29/2022 2:44 PM, Krzysztof Kozlowski wrote:
> On 29/09/2022 11:02, Dmitry Baryshkov wrote:
>> On Thu, 29 Sept 2022 at 11:54, Krishna Kurapati PSSNV
>> <quic_kriskura@quicinc.com> wrote:
>>>
>>>
>>> On 9/29/2022 1:29 PM, Dmitry Baryshkov wrote:
>>>> On Thu, 29 Sept 2022 at 10:14, Krishna Kurapati
>>>> <quic_kriskura@quicinc.com> wrote:
>>>>>
>>>>> Override the SNPS Phy tuning parameters for SC7280 devices. These
>>>>> values are common for both trogdor and herobrine variants.
>>>>
>>>> They are common for trogdor and herobrine, but should these parameters
>>>> be a default? In other words, a random new device based on sc7280
>>>> would more likely use these overrides or the hardware defaults?
>>>>
>>> Hi Dmitry,
>>>
>>>     Currently there are only two platforms, so I made these changes on
>>> common dtsi. If a new platform comes (mostly it won't) we can override
>>> them in platform specific file is what I thought.
>>
Hi Dmitry, Krzysztof,

>> This is not how it usually works. The 'sc7280.dtsi' is not a 'common
>> dtsi' for trogdor and herobrine. It describes the SoC.
>> Thus in my opinion if these overrides should be a default to all
>> sc7280 platforms, this patch is fine. If these overrides are
>> applicable only to the two mentioned platforms, they should go to
>> respective platform-specific DT files.
> 
> Dmitry's conclusion is correct here. The true question is whether these
> are properties of the SoC itself (so do not depend on the board or board
> layout) or these depend on design of board.
> 
> Best regards,
> Krzysztof
> 
Thanks for the info. These parameters are per-board and not common to 
SoC. I sent one patch previously for IDP platform [1]. I will let that 
get merged and I will send a v2 of this patch adding parameters only for 
herobrine platform.

[1] : 
https://patchwork.kernel.org/project/linux-usb/patch/1662480933-12326-4-git-send-email-quic_kriskura@quicinc.com/

Thanks,
Krishna,