linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stephen Boyd <swboyd@chromium.org>
To: Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Prasad Malisetty <pmaliset@codeaurora.org>,
	Rob Herring <robh+dt@kernel.org>
Cc: linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
	mgautam@codeaurora.org, dianders@chromium.org, mka@chromium.org
Subject: Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
Date: Fri, 7 May 2021 13:06:31 -0700	[thread overview]
Message-ID: <CAE-0n530bSPupOHVDzwpd_JVVN0tOfrAOm9dAt1ZGj7zaXOZ6A@mail.gmail.com> (raw)
In-Reply-To: <1620382648-17395-3-git-send-email-pmaliset@codeaurora.org>

Quoting Prasad Malisetty (2021-05-07 03:17:27)
> Add PCIe controller and PHY nodes for sc7280 SOC.
>
> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.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@1c0e000 {
> +                       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@1c0e200 {
> +                               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@6002000 {
>                         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.

> +                       };
>                 };
>

  reply	other threads:[~2021-05-07 20:06 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-07 10:17 [PATCH 0/3] Add DT bindings and DT nodes for PCIe and PHY in SC7280 Prasad Malisetty
2021-05-07 10:17 ` [PATCH 1/3] dt-bindings: pci: qcom: Document PCIe bindings for SC720 Prasad Malisetty
2021-05-07 19:59   ` Stephen Boyd
2021-06-04 11:26     ` Prasad Malisetty
2021-06-04 21:44       ` Stephen Boyd
2021-05-10 15:48   ` Rob Herring
2021-05-07 10:17 ` [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes Prasad Malisetty
2021-05-07 20:06   ` Stephen Boyd [this message]
2021-05-21  9:57     ` Prasad Malisetty
2021-06-04 21:43       ` Stephen Boyd
2021-06-06  4:02         ` Bjorn Andersson
2021-06-15  5:26           ` Prasad Malisetty
2021-05-07 10:17 ` [PATCH 3/3] arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board Prasad Malisetty

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAE-0n530bSPupOHVDzwpd_JVVN0tOfrAOm9dAt1ZGj7zaXOZ6A@mail.gmail.com \
    --to=swboyd@chromium.org \
    --cc=agross@kernel.org \
    --cc=bhelgaas@google.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=mgautam@codeaurora.org \
    --cc=mka@chromium.org \
    --cc=pmaliset@codeaurora.org \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).