| Message ID | 20230616103534.4031331-9-quic_mohs@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add SC7280 audioreach device tree nodes | expand |
On 16.06.2023 12:35, Mohammad Rafi Shaik wrote: > From: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com> > > Add "qcom,adsp-pil-mode" property in clock nodes for herobrine > crd revision 3 board specific device tree. > This is to register clocks conditionally by differentiating ADSP > based platforms and legacy path platforms. > Also disable lpass_core clock, as it is creating conflict > with ADSP clocks and it is not required for ADSP based platforms. > > Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com> > Signed-off-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com> > --- > .../qcom/sc7280-herobrine-audioreach-wcd9385.dtsi | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi > index c02ca393378f..876a29178d46 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi > @@ -197,6 +197,14 @@ q6prmcc: clock-controller { > }; > }; > > +&lpass_aon { > + qcom,adsp-pil-mode; That's a whole bunch of hacky bindings that makes no sense.. What should have been done from the beginning is: - all clocks should be registered inside the clock driver, unconditionally as far as .c code is concerned - the regmap properties within should reflect the actual max register range within the hardware block - device-tree should contain protected-clocks, which omits registering specified clks (I guess in the ADSP-less case you could probably even register all of them and it wouldn't hurt) > +}; > + > +&lpass_core { > + status = "disabled"; status = "reserved"; Konrad > +}; > + > &lpass_rx_macro { > /delete-property/ power-domains; > /delete-property/ power-domain-names; > @@ -239,3 +247,7 @@ &lpass_va_macro { > > status = "okay"; > }; > + > +&lpasscc { > + qcom,adsp-pil-mode; > +};
On 6/16/2023 5:06 PM, Konrad Dybcio wrote: > On 16.06.2023 12:35, Mohammad Rafi Shaik wrote: >> From: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com> >> >> Add "qcom,adsp-pil-mode" property in clock nodes for herobrine >> crd revision 3 board specific device tree. >> This is to register clocks conditionally by differentiating ADSP >> based platforms and legacy path platforms. >> Also disable lpass_core clock, as it is creating conflict >> with ADSP clocks and it is not required for ADSP based platforms. >> >> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com> >> Signed-off-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com> >> --- >> .../qcom/sc7280-herobrine-audioreach-wcd9385.dtsi | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi >> index c02ca393378f..876a29178d46 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi >> @@ -197,6 +197,14 @@ q6prmcc: clock-controller { >> }; >> }; >> >> +&lpass_aon { >> + qcom,adsp-pil-mode; > That's a whole bunch of hacky bindings that makes no sense.. > > What should have been done from the beginning is: > > - all clocks should be registered inside the clock driver, unconditionally > as far as .c code is concerned > > - the regmap properties within should reflect the actual max register > range within the hardware block > > - device-tree should contain protected-clocks, which omits registering > specified clks (I guess in the ADSP-less case you could probably even > register all of them and it wouldn't hurt) > For AR solution, it is required to add "qcom,adsp-pil-mode" flag to enable ahbm and ahbs clocks. Please refer: https://elixir.bootlin.com/linux/v6.4-rc7/source/drivers/clk/qcom/lpassaudiocc-sc7280.c >> +}; >> + >> +&lpass_core { >> + status = "disabled"; > status = "reserved"; > > Konrad Okay, will change status flag. Rafi >> +}; >> + >> &lpass_rx_macro { >> /delete-property/ power-domains; >> /delete-property/ power-domain-names; >> @@ -239,3 +247,7 @@ &lpass_va_macro { >> >> status = "okay"; >> }; >> + >> +&lpasscc { >> + qcom,adsp-pil-mode; >> +};
On 26.06.2023 13:17, Mohammad Rafi Shaik wrote: > > On 6/16/2023 5:06 PM, Konrad Dybcio wrote: >> On 16.06.2023 12:35, Mohammad Rafi Shaik wrote: >>> From: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com> >>> >>> Add "qcom,adsp-pil-mode" property in clock nodes for herobrine >>> crd revision 3 board specific device tree. >>> This is to register clocks conditionally by differentiating ADSP >>> based platforms and legacy path platforms. >>> Also disable lpass_core clock, as it is creating conflict >>> with ADSP clocks and it is not required for ADSP based platforms. >>> >>> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com> >>> Signed-off-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com> >>> --- >>> .../qcom/sc7280-herobrine-audioreach-wcd9385.dtsi | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi >>> index c02ca393378f..876a29178d46 100644 >>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi >>> @@ -197,6 +197,14 @@ q6prmcc: clock-controller { >>> }; >>> }; >>> +&lpass_aon { >>> + qcom,adsp-pil-mode; >> That's a whole bunch of hacky bindings that makes no sense.. >> >> What should have been done from the beginning is: >> >> - all clocks should be registered inside the clock driver, unconditionally >> as far as .c code is concerned >> >> - the regmap properties within should reflect the actual max register >> range within the hardware block >> >> - device-tree should contain protected-clocks, which omits registering >> specified clks (I guess in the ADSP-less case you could probably even >> register all of them and it wouldn't hurt) >> > For AR solution, it is required to add "qcom,adsp-pil-mode" flag to enable ahbm and ahbs clocks. > Please refer: https://elixir.bootlin.com/linux/v6.4-rc7/source/drivers/clk/qcom/lpassaudiocc-sc7280.c It does what I've just said, except in an implementation-specific way, instead of using an existing, common DT property. Konrad >>> +}; >>> + >>> +&lpass_core { >>> + status = "disabled"; >> status = "reserved"; >> >> Konrad > Okay, will change status flag. > > Rafi >>> +}; >>> + >>> &lpass_rx_macro { >>> /delete-property/ power-domains; >>> /delete-property/ power-domain-names; >>> @@ -239,3 +247,7 @@ &lpass_va_macro { >>> status = "okay"; >>> }; >>> + >>> +&lpasscc { >>> + qcom,adsp-pil-mode; >>> +};
diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi index c02ca393378f..876a29178d46 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi @@ -197,6 +197,14 @@ q6prmcc: clock-controller { }; }; +&lpass_aon { + qcom,adsp-pil-mode; +}; + +&lpass_core { + status = "disabled"; +}; + &lpass_rx_macro { /delete-property/ power-domains; /delete-property/ power-domain-names; @@ -239,3 +247,7 @@ &lpass_va_macro { status = "okay"; }; + +&lpasscc { + qcom,adsp-pil-mode; +};