| Message ID | 1664435628-4011-1-git-send-email-quic_kriskura@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280 | expand |
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
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 >
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 >> > >
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,
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 > >> > > > >
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
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,
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 {
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(+)