Hi, On Thu, Jul 16, 2020 at 10:34 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > Hi, > > On Tue, Jul 14, 2020 at 11:56 PM Taniya Das <tdas@xxxxxxxxxxxxxx> wrote: > > > > Update the clock controller nodes for Low power audio subsystem > > functionality. > > > > Signed-off-by: Taniya Das <tdas@xxxxxxxxxxxxxx> > > --- > > Somewhere here you should be pointing to the unlanded bindings patch, AKA: > > https://lore.kernel.org/r/1594795010-9074-3-git-send-email-tdas@xxxxxxxxxxxxxx > > As per usual the fact that are using a new bindings #include file > means Qualcomm maintainers and clock maintainers will need to > coordinate landing and this needs to be pointed out. > > > > arch/arm64/boot/dts/qcom/sc7180.dtsi | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi > > index 2be81a2..8c30a17 100644 > > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi > > @@ -8,6 +8,7 @@ > > #include <dt-bindings/clock/qcom,dispcc-sc7180.h> > > #include <dt-bindings/clock/qcom,gcc-sc7180.h> > > #include <dt-bindings/clock/qcom,gpucc-sc7180.h> > > +#include <dt-bindings/clock/qcom,lpasscorecc-sc7180.h> > > #include <dt-bindings/clock/qcom,rpmh.h> > > #include <dt-bindings/clock/qcom,videocc-sc7180.h> > > #include <dt-bindings/interconnect/qcom,osm-l3.h> > > @@ -2136,6 +2137,27 @@ > > }; > > }; > > > > + lpasscc: clock-controller@62d00000 { > > + compatible = "qcom,sc7180-lpasscorecc"; > > + reg = <0 0x62d00000 0 0x50000>, > > + <0 0x62780000 0 0x30000>; > > + reg-names = "lpass_core_cc", "lpass_audio_cc"; > > + clocks = <&gcc GCC_LPASS_CFG_NOC_SWAY_CLK>; > > + clock-names = "iface"; > > + power-domains = <&lpass_hm LPASS_CORE_HM_GDSCR>; > > + #clock-cells = <1>; > > + #power-domain-cells = <1>; > > + }; > > + > > + lpass_hm: clock-controller@63000000 { > > + compatible = "qcom,sc7180-lpasshm"; > > + reg = <0 0x63000000 0 0x28>; > > + clocks = <&gcc GCC_LPASS_CFG_NOC_SWAY_CLK>; > > + clock-names = "iface"; > > + #clock-cells = <1>; > > + #power-domain-cells = <1>; > > + }; > > Question: would it ever make sense for a board not to need this clock > controller? I ask because the sdm845 "lpass" clock controller is > "disabled" by default but yours here isn't. I know sc7180 and sdm845 > work pretty differently and perhaps the sdm845's default of "disabled" > was just overkill, but I thought I'd ask. > > > > + > > etm@7040000 { > > compatible = "arm,coresight-etm4x", "arm,primecell"; > > reg = <0 0x07040000 0 0x1000>; > > Your sort order is off. You should be sorting by unit address. Note > that the "ETM" has an extra 0 before its 7, so you're comparing 63 to > 07 and you should be after. > > Other than those small things above this patch looks like it matches > the example in the bindings, so as long as Rob / the clock guys are > fine with the bindings then this seems good to go. So it looks like the bindings landed and one would think we'd be good to go. Except that when I mixed your device tree patch with the driver that landed things went boom. Can you please re-post addressing my concerns above and also adding "bi_tcxo" as I have done in the bindings example here: https://lore.kernel.org/r/20200731133006.1.Iee81b115f5be50d6d69500fe1bda11bba6e16143@changeid Thanks! -Doug
