Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] arm64: dts: qcom: sc7180: Add LPASS clock controller nodes
@ 2020-07-15  6:55 Taniya Das
  2020-07-16 17:34 ` Doug Anderson
  0 siblings, 1 reply; 3+ messages in thread
From: Taniya Das @ 2020-07-15  6:55 UTC (permalink / raw)
  To: Stephen Boyd, Michael Turquette  
  Cc: David Brown, Rajendra Nayak, linux-arm-msm, linux-soc, linux-clk,
	linux-kernel, Andy Gross, devicetree, robh, robh+dt, cychiang,
	Taniya Das

Update the clock controller nodes for Low power audio subsystem
functionality.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 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>;
+		};
+
 		etm@7040000 {
 			compatible = "arm,coresight-etm4x", "arm,primecell";
 			reg = <0 0x07040000 0 0x1000>;
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] arm64: dts: qcom: sc7180: Add LPASS clock controller nodes
  2020-07-15  6:55 [PATCH] arm64: dts: qcom: sc7180: Add LPASS clock controller nodes Taniya Das
@ 2020-07-16 17:34 ` Doug Anderson
  2020-07-31 20:38   ` Doug Anderson
  0 siblings, 1 reply; 3+ messages in thread
From: Doug Anderson @ 2020-07-16 17:34 UTC (permalink / raw)
  To: Taniya Das
  Cc: Stephen Boyd, Michael Turquette, David Brown, Rajendra Nayak,
	linux-arm-msm, open list:ARM/QUALCOMM SUPPORT, linux-clk, LKML,
	Andy Gross,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Rob Herring, Jimmy Cheng-Yi Chiang

Hi,

On Tue, Jul 14, 2020 at 11:56 PM Taniya Das <tdas@codeaurora.org> wrote:
>
> Update the clock controller nodes for Low power audio subsystem
> functionality.
>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---

Somewhere here you should be pointing to the unlanded bindings patch, AKA:

https://lore.kernel.org/r/1594795010-9074-3-git-send-email-tdas@codeaurora.org

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.

-Doug

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] arm64: dts: qcom: sc7180: Add LPASS clock controller nodes
  2020-07-16 17:34 ` Doug Anderson
@ 2020-07-31 20:38   ` Doug Anderson
  0 siblings, 0 replies; 3+ messages in thread
From: Doug Anderson @ 2020-07-31 20:38 UTC (permalink / raw)
  To: Taniya Das
  Cc: Stephen Boyd, Michael Turquette, David Brown, Rajendra Nayak,
	linux-arm-msm, open list:ARM/QUALCOMM SUPPORT, linux-clk, LKML,
	Andy Gross,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, Rob Herring, Jimmy Cheng-Yi Chiang

Hi,

On Thu, Jul 16, 2020 at 10:34 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Jul 14, 2020 at 11:56 PM Taniya Das <tdas@codeaurora.org> wrote:
> >
> > Update the clock controller nodes for Low power audio subsystem
> > functionality.
> >
> > Signed-off-by: Taniya Das <tdas@codeaurora.org>
> > ---
>
> Somewhere here you should be pointing to the unlanded bindings patch, AKA:
>
> https://lore.kernel.org/r/1594795010-9074-3-git-send-email-tdas@codeaurora.org
>
> 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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-15  6:55 [PATCH] arm64: dts: qcom: sc7180: Add LPASS clock controller nodes Taniya Das
2020-07-16 17:34 ` Doug Anderson
2020-07-31 20:38   ` Doug Anderson

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org
	public-inbox-index linux-clk

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git