linux-kernel - Re: [PATCH] arm64: dts: qcom: sc7180: Add LPASS clock controller nodes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Jul 2020 10:34:36 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Taniya Das <tdas@...eaurora.org>
Cc:     Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        David Brown <david.brown@...aro.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-soc@...r.kernel.org>,
        linux-clk <linux-clk@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Andy Gross <agross@...nel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Rob Herring <robh@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Jimmy Cheng-Yi Chiang <cychiang@...gle.com>
Subject: Re: [PATCH] arm64: dts: qcom: sc7180: Add LPASS clock controller nodes

Hi,

On Tue, Jul 14, 2020 at 11:56 PM Taniya Das <tdas@...eaurora.org> wrote:
>
> Update the clock controller nodes for Low power audio subsystem
> functionality.
>
> Signed-off-by: Taniya Das <tdas@...eaurora.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@...00000 {
> +                       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@...00000 {
> +                       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@...0000 {
>                         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