linux-kernel - Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related 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:   Fri, 7 May 2021 13:06:31 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Prasad Malisetty <pmaliset@...eaurora.org>,
        Rob Herring <robh+dt@...nel.org>
Cc:     linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        mgautam@...eaurora.org, dianders@...omium.org, mka@...omium.org
Subject: Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes

Quoting Prasad Malisetty (2021-05-07 03:17:27)
> Add PCIe controller and PHY nodes for sc7280 SOC.
>
> Signed-off-by: Prasad Malisetty <pmaliset@...eaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 138 +++++++++++++++++++++++++++++++++++
>  1 file changed, 138 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 2cc4785..a9f25fc1 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -12,6 +12,7 @@
>  #include <dt-bindings/power/qcom-aoss-qmp.h>
>  #include <dt-bindings/power/qcom-rpmpd.h>
>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>
> +#include <dt-bindings/gpio/gpio.h>
>
>  / {
>         interrupt-parent = <&intc>;
> @@ -316,6 +317,118 @@
>                         };
>                 };
>
[...]
> +
> +               pcie1_phy: phy@...e000 {
> +                       compatible = "qcom,sm8250-qmp-gen3x2-pcie-phy";
> +                       reg = <0 0x01c0e000 0 0x1c0>;
> +                       #address-cells = <2>;
> +                       #size-cells = <2>;
> +                       ranges;
> +                       clocks = <&gcc GCC_PCIE_1_AUX_CLK>,
> +                                <&gcc GCC_PCIE_1_CFG_AHB_CLK>,
> +                                <&gcc GCC_PCIE_CLKREF_EN>,
> +                                <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
> +                       clock-names = "aux", "cfg_ahb", "ref", "refgen";
> +
> +                       resets = <&gcc GCC_PCIE_1_PHY_BCR>;
> +                       reset-names = "phy";
> +
> +                       assigned-clocks = <&gcc GCC_PCIE1_PHY_RCHNG_CLK>;
> +                       assigned-clock-rates = <100000000>;
> +
> +                       status = "disabled";

I think the style is to put status disabled close to the compatible?

> +
> +                       pcie1_lane: lanes@...e200 {
> +                               reg = <0 0x1c0e200 0 0x170>, /* tx0 */

Please pad reg addresses to 8 characters.

> +                                     <0 0x1c0e400 0 0x200>, /* rx0 */
> +                                     <0 0x1c0ea00 0 0x1f0>, /* pcs */
> +                                     <0 0x1c0e600 0 0x170>, /* tx1 */
> +                                     <0 0x1c0e800 0 0x200>, /* rx1 */
> +                                     <0 0x1c0ee00 0 0xf4>; /* "pcs_com" same as pcs_misc? */

Is this a TODO? I'd prefer all the comments on the reg properties to be
removed.

> +                               clocks = <&rpmhcc RPMH_CXO_CLK>;
> +                               clock-names = "pipe0";
> +
> +                               #phy-cells = <0>;
> +                               #clock-cells = <1>;
> +                               clock-output-names = "pcie_1_pipe_clk";
> +                       };
> +               };
> +
>                 stm@...2000 {
>                         compatible = "arm,coresight-stm", "arm,primecell";
>                         reg = <0 0x06002000 0 0x1000>,
> @@ -871,6 +984,31 @@
>                                 pins = "gpio46", "gpio47";
>                                 function = "qup13";
>                         };
> +
> +                       pcie1_default_state: pcie1-default {
> +                               clkreq {
> +                                       pins = "gpio79";
> +                                       function = "pcie1_clkreqn";
> +                                       bias-pull-up;

Move this bias-pull-up to the idp file?

> +                               };
> +
> +                               reset-n {
> +                                       pins = "gpio2";
> +                                       function = "gpio";
> +
> +                                       drive-strength = <16>;
> +                                       output-low;
> +                                       bias-disable;
> +                               };
> +
> +                               wake-n {
> +                                       pins = "gpio3";
> +                                       function = "gpio";
> +
> +                                       drive-strength = <2>;
> +                                       bias-pull-up;
> +                               };

These last two nodes with the pull-up and drive-strength settings should
be in the board files, like the idp one, instead of here in the SoC
file. That way board designers can take the SoC and connect the pcie to
an external device using these pins and set the configuration they want
on these pins, or choose not to connect them to the SoC at all and use
those pins for something else.

In addition, it looks like the reset could be a reset-gpios property
instead of an output-low config.

> +                       };
>                 };
>