| Message ID | 20221208183837.505454-1-elder@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: qcom: sc7280: enable IPA in sc7280-herobrine-lte-sku.dtsi | expand |
Alex, Thanks for the patch. On 12/9/22 00:08, Alex Elder wrote: > IPA is only needed on a platform if it includes a modem, and not all > SC7280 SoC variants do. The file "sc7280-herobrine-lte-sku.dtsi" is > used to encapsulate definitions related to Chrome OS SC7280 devices > where a modem is present, and that's the proper place for the IPA > node to be enabled. > > Currently IPA is enabled in "sc7280-idp.dtsi", which is included by > DTS files for Qualcomm reference platforms (all of which include the > modem). That also includes "sc7280-herobrine-lte-sku.dtsi", so > enabling IPA there would make it unnecessary for "sc7280-idp.dtsi" > to enable it. > > The only other place IPA is enabled is "sc7280-qcard.dtsi". > That file is included only by "sc7280-herobrine.dtsi", which > is (eventually) included only by these top-level DTS files: > sc7280-herobrine-crd.dts > sc7280-herobrine-herobrine-r1.dts > sc7280-herobrine-evoker.dts > sc7280-herobrine-evoker-lte.dts > sc7280-herobrine-villager-r0.dts > sc7280-herobrine-villager-r1.dts > sc7280-herobrine-villager-r1-lte.dts > All of but two of these include "sc7280-herobrine-lte-sku.dtsi", and > for those cases, enabling IPA there means there is no need for it to > be enabled in "sc7280-qcard.dtsi". > > The two remaining cases will no longer enable IPA as a result of > this change: > sc7280-herobrine-evoker.dts > sc7280-herobrine-villager-r1.dts > Both of these have "lte" counterparts, and are meant to represent > board variants that do *not* have a modem. > > This is exactly the desired configuration. > > Signed-off-by: Alex Elder <elder@linaro.org> > --- > arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi | 5 +++++ > arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 5 ----- > arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi | 5 ----- > 3 files changed, 5 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi > index ad66e5e9db4ed..956708397f035 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi > @@ -34,3 +34,8 @@ &remoteproc_mpss { > &rmtfs_mem { > reg = <0x0 0x9c900000 0x0 0x800000>; > }; > + > +&ipa { > + status = "okay"; > + modem-init; > +}; nit; You'll probably want to move the ipa node above remoteproc node to maintain sorting. Reviewed-by: Sibi Sankar <quic_sibis@quicinc.com> Tested-by: Sibi Sankar <quic_sibis@quicinc.com> > diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > index f7efb9966afd1..0ddbe7f732bd9 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > @@ -376,11 +376,6 @@ &gpi_dma1 { > status = "okay"; > }; > > -&ipa { > - status = "okay"; > - modem-init; > -}; > - > &lpass_cpu { > status = "okay"; > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi b/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi > index df49564ae6dc1..cd6ee84b36fd4 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi > @@ -336,11 +336,6 @@ vreg_bob: bob { > > /* ADDITIONS TO NODES DEFINED IN PARENT DEVICE TREE FILES */ > > -&ipa { > - status = "okay"; > - modem-init; > -}; > - > &lpass_va_macro { > vdd-micb-supply = <&vreg_bob>; > };
On 15.12.2022 18:38, Sibi Sankar wrote: > Alex, > > Thanks for the patch. > > On 12/9/22 00:08, Alex Elder wrote: >> IPA is only needed on a platform if it includes a modem, and not all >> SC7280 SoC variants do. The file "sc7280-herobrine-lte-sku.dtsi" is >> used to encapsulate definitions related to Chrome OS SC7280 devices >> where a modem is present, and that's the proper place for the IPA >> node to be enabled. >> >> Currently IPA is enabled in "sc7280-idp.dtsi", which is included by >> DTS files for Qualcomm reference platforms (all of which include the >> modem). That also includes "sc7280-herobrine-lte-sku.dtsi", so >> enabling IPA there would make it unnecessary for "sc7280-idp.dtsi" >> to enable it. >> >> The only other place IPA is enabled is "sc7280-qcard.dtsi". >> That file is included only by "sc7280-herobrine.dtsi", which >> is (eventually) included only by these top-level DTS files: >> sc7280-herobrine-crd.dts >> sc7280-herobrine-herobrine-r1.dts >> sc7280-herobrine-evoker.dts >> sc7280-herobrine-evoker-lte.dts >> sc7280-herobrine-villager-r0.dts >> sc7280-herobrine-villager-r1.dts >> sc7280-herobrine-villager-r1-lte.dts >> All of but two of these include "sc7280-herobrine-lte-sku.dtsi", and >> for those cases, enabling IPA there means there is no need for it to >> be enabled in "sc7280-qcard.dtsi". >> >> The two remaining cases will no longer enable IPA as a result of >> this change: >> sc7280-herobrine-evoker.dts >> sc7280-herobrine-villager-r1.dts >> Both of these have "lte" counterparts, and are meant to represent >> board variants that do *not* have a modem. >> >> This is exactly the desired configuration. >> >> Signed-off-by: Alex Elder <elder@linaro.org> >> --- >> arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi | 5 +++++ >> arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 5 ----- >> arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi | 5 ----- >> 3 files changed, 5 insertions(+), 10 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi >> index ad66e5e9db4ed..956708397f035 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi >> @@ -34,3 +34,8 @@ &remoteproc_mpss { >> &rmtfs_mem { >> reg = <0x0 0x9c900000 0x0 0x800000>; >> }; >> + >> +&ipa { >> + status = "okay"; >> + modem-init; >> +}; > > nit; You'll probably want to move the ipa node above remoteproc node to > maintain sorting. > > Reviewed-by: Sibi Sankar <quic_sibis@quicinc.com> > Tested-by: Sibi Sankar <quic_sibis@quicinc.com> That + please flip the property order, as we're trying to keep status last wherever possible. Konrad > >> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >> index f7efb9966afd1..0ddbe7f732bd9 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >> @@ -376,11 +376,6 @@ &gpi_dma1 { >> status = "okay"; >> }; >> -&ipa { >> - status = "okay"; >> - modem-init; >> -}; >> - >> &lpass_cpu { >> status = "okay"; >> diff --git a/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi b/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi >> index df49564ae6dc1..cd6ee84b36fd4 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi >> @@ -336,11 +336,6 @@ vreg_bob: bob { >> /* ADDITIONS TO NODES DEFINED IN PARENT DEVICE TREE FILES */ >> -&ipa { >> - status = "okay"; >> - modem-init; >> -}; >> - >> &lpass_va_macro { >> vdd-micb-supply = <&vreg_bob>; >> };
On 12/15/22 12:01 PM, Konrad Dybcio wrote: > > > On 15.12.2022 18:38, Sibi Sankar wrote: >> Alex, >> >> Thanks for the patch. >> >> On 12/9/22 00:08, Alex Elder wrote: >>> IPA is only needed on a platform if it includes a modem, and not all >>> SC7280 SoC variants do. The file "sc7280-herobrine-lte-sku.dtsi" is >>> used to encapsulate definitions related to Chrome OS SC7280 devices >>> where a modem is present, and that's the proper place for the IPA >>> node to be enabled. >>> >>> Currently IPA is enabled in "sc7280-idp.dtsi", which is included by >>> DTS files for Qualcomm reference platforms (all of which include the >>> modem). That also includes "sc7280-herobrine-lte-sku.dtsi", so >>> enabling IPA there would make it unnecessary for "sc7280-idp.dtsi" >>> to enable it. >>> >>> The only other place IPA is enabled is "sc7280-qcard.dtsi". >>> That file is included only by "sc7280-herobrine.dtsi", which >>> is (eventually) included only by these top-level DTS files: >>> sc7280-herobrine-crd.dts >>> sc7280-herobrine-herobrine-r1.dts >>> sc7280-herobrine-evoker.dts >>> sc7280-herobrine-evoker-lte.dts >>> sc7280-herobrine-villager-r0.dts >>> sc7280-herobrine-villager-r1.dts >>> sc7280-herobrine-villager-r1-lte.dts >>> All of but two of these include "sc7280-herobrine-lte-sku.dtsi", and >>> for those cases, enabling IPA there means there is no need for it to >>> be enabled in "sc7280-qcard.dtsi". >>> >>> The two remaining cases will no longer enable IPA as a result of >>> this change: >>> sc7280-herobrine-evoker.dts >>> sc7280-herobrine-villager-r1.dts >>> Both of these have "lte" counterparts, and are meant to represent >>> board variants that do *not* have a modem. >>> >>> This is exactly the desired configuration. >>> >>> Signed-off-by: Alex Elder <elder@linaro.org> >>> --- >>> arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi | 5 +++++ >>> arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 5 ----- >>> arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi | 5 ----- >>> 3 files changed, 5 insertions(+), 10 deletions(-) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi >>> index ad66e5e9db4ed..956708397f035 100644 >>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi >>> @@ -34,3 +34,8 @@ &remoteproc_mpss { >>> &rmtfs_mem { >>> reg = <0x0 0x9c900000 0x0 0x800000>; >>> }; >>> + >>> +&ipa { >>> + status = "okay"; >>> + modem-init; >>> +}; >> >> nit; You'll probably want to move the ipa node above remoteproc node to >> maintain sorting. >> >> Reviewed-by: Sibi Sankar <quic_sibis@quicinc.com> >> Tested-by: Sibi Sankar <quic_sibis@quicinc.com> > That + please flip the property order, as we're trying to > keep status last wherever possible. Thank you, I'll post version 2 this afternoon. -Alex > > Konrad >> >>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >>> index f7efb9966afd1..0ddbe7f732bd9 100644 >>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >>> @@ -376,11 +376,6 @@ &gpi_dma1 { >>> status = "okay"; >>> }; >>> -&ipa { >>> - status = "okay"; >>> - modem-init; >>> -}; >>> - >>> &lpass_cpu { >>> status = "okay"; >>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi b/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi >>> index df49564ae6dc1..cd6ee84b36fd4 100644 >>> --- a/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi >>> @@ -336,11 +336,6 @@ vreg_bob: bob { >>> /* ADDITIONS TO NODES DEFINED IN PARENT DEVICE TREE FILES */ >>> -&ipa { >>> - status = "okay"; >>> - modem-init; >>> -}; >>> - >>> &lpass_va_macro { >>> vdd-micb-supply = <&vreg_bob>; >>> };
diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi index ad66e5e9db4ed..956708397f035 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi @@ -34,3 +34,8 @@ &remoteproc_mpss { &rmtfs_mem { reg = <0x0 0x9c900000 0x0 0x800000>; }; + +&ipa { + status = "okay"; + modem-init; +}; diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi index f7efb9966afd1..0ddbe7f732bd9 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi @@ -376,11 +376,6 @@ &gpi_dma1 { status = "okay"; }; -&ipa { - status = "okay"; - modem-init; -}; - &lpass_cpu { status = "okay"; diff --git a/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi b/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi index df49564ae6dc1..cd6ee84b36fd4 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi @@ -336,11 +336,6 @@ vreg_bob: bob { /* ADDITIONS TO NODES DEFINED IN PARENT DEVICE TREE FILES */ -&ipa { - status = "okay"; - modem-init; -}; - &lpass_va_macro { vdd-micb-supply = <&vreg_bob>; };
IPA is only needed on a platform if it includes a modem, and not all SC7280 SoC variants do. The file "sc7280-herobrine-lte-sku.dtsi" is used to encapsulate definitions related to Chrome OS SC7280 devices where a modem is present, and that's the proper place for the IPA node to be enabled. Currently IPA is enabled in "sc7280-idp.dtsi", which is included by DTS files for Qualcomm reference platforms (all of which include the modem). That also includes "sc7280-herobrine-lte-sku.dtsi", so enabling IPA there would make it unnecessary for "sc7280-idp.dtsi" to enable it. The only other place IPA is enabled is "sc7280-qcard.dtsi". That file is included only by "sc7280-herobrine.dtsi", which is (eventually) included only by these top-level DTS files: sc7280-herobrine-crd.dts sc7280-herobrine-herobrine-r1.dts sc7280-herobrine-evoker.dts sc7280-herobrine-evoker-lte.dts sc7280-herobrine-villager-r0.dts sc7280-herobrine-villager-r1.dts sc7280-herobrine-villager-r1-lte.dts All of but two of these include "sc7280-herobrine-lte-sku.dtsi", and for those cases, enabling IPA there means there is no need for it to be enabled in "sc7280-qcard.dtsi". The two remaining cases will no longer enable IPA as a result of this change: sc7280-herobrine-evoker.dts sc7280-herobrine-villager-r1.dts Both of these have "lte" counterparts, and are meant to represent board variants that do *not* have a modem. This is exactly the desired configuration. Signed-off-by: Alex Elder <elder@linaro.org> --- arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi | 5 +++++ arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 5 ----- arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi | 5 ----- 3 files changed, 5 insertions(+), 10 deletions(-)