mbox series

[v2,0/4] Add DT bindings and DT nodes for PCIe and PHY in SC7280

Message ID 1622904059-21244-1-git-send-email-pmaliset@codeaurora.org
Headers show
Series Add DT bindings and DT nodes for PCIe and PHY in SC7280 | expand

Message

Prasad Malisetty June 5, 2021, 2:40 p.m. UTC
This series includes PCIe support for qualcomm sc7280
which includes PCIe controller and PHY DT bindings.
The PCIe controller and PHYs are mostly comaptible with SM8250 SoC,
hence the existing pcie drivers are modified to add the support.

Changes in v2:
	* Moved pcie pin control settings into IDP file.
	* Replaced pipe_clk_src with pipe_clk_mux in pcie driver 
	* Included pipe clk mux setting change set in this series

Prasad Malisetty (4):
  arm64: dts: qcom: sc7280: Add PCIe nodes for IDP board
  arm64: dts: qcom: sc7280: Add PCIe and PHY related nodes
  PCIe: qcom: Add support to control pipe clk mux
  dt-bindings: pci: qcom: Document PCIe bindings for SC720

 .../devicetree/bindings/pci/qcom,pcie.txt          |  17 +++
 arch/arm64/boot/dts/qcom/sc7280-idp.dts            |  28 +++++
 arch/arm64/boot/dts/qcom/sc7280.dtsi               | 138 +++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom.c             |  22 ++++
 4 files changed, 205 insertions(+)

Comments

Stephen Boyd June 5, 2021, 9:26 p.m. UTC | #1
Quoting Prasad Malisetty (2021-06-05 07:40:58)
> In PCIe driver pipe-clk mux needs to switch between pipe_clk
> and XO for GDSC enable. This is done by setting pipe_clk mux
> as parent of pipe_clk after phy init.

Just to confirm, we can't set this parent via assigned-clock-parents
property in DT?

>
> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 8a7a300..5cbbea4 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -166,6 +166,9 @@ struct qcom_pcie_resources_2_7_0 {
>         struct regulator_bulk_data supplies[2];
>         struct reset_control *pci_reset;
>         struct clk *pipe_clk;
> +       struct clk *pipe_clk_mux;
> +       struct clk *pipe_ext_src;
> +       struct clk *ref_clk_src;
>  };
>
>  union qcom_pcie_resources {
> @@ -1167,6 +1170,20 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>         if (ret < 0)
>                 return ret;
>
> +       if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280")) {
> +               res->pipe_clk_mux = devm_clk_get(dev, "pipe_src");
> +               if (IS_ERR(res->pipe_clk_mux))
> +                       return PTR_ERR(res->pipe_clk_mux);
> +
> +               res->pipe_ext_src = devm_clk_get(dev, "pipe_ext");
> +               if (IS_ERR(res->pipe_ext_src))
> +                       return PTR_ERR(res->pipe_ext_src);
> +
> +               res->ref_clk_src = devm_clk_get(dev, "ref");

Is this going to be used by any code?

> +               if (IS_ERR(res->ref_clk_src))
> +                       return PTR_ERR(res->ref_clk_src);
> +       }
> +
>         res->pipe_clk = devm_clk_get(dev, "pipe");
>         return PTR_ERR_OR_ZERO(res->pipe_clk);
>  }
> @@ -1255,6 +1272,11 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
>  static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
>  {
>         struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> +       struct dw_pcie *pci = pcie->pci;
> +       struct device *dev = pci->dev;
> +
> +       if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280"))
> +               clk_set_parent(res->pipe_clk_mux, res->pipe_ext_src);
>
>         return clk_prepare_enable(res->pipe_clk);
>  }
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
Bjorn Andersson June 6, 2021, 2:15 a.m. UTC | #2
On Sat 05 Jun 09:40 CDT 2021, Prasad Malisetty wrote:

> In PCIe driver pipe-clk mux needs to switch between pipe_clk
> and XO for GDSC enable. This is done by setting pipe_clk mux
> as parent of pipe_clk after phy init.

But you're not switching between pipe_clk and XO, you're only making
sure that the pipe_clk is parented by the PHY's pipe clock.

Also, can you please elaborate on how this relates to the GDSC?

> 
> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 8a7a300..5cbbea4 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -166,6 +166,9 @@ struct qcom_pcie_resources_2_7_0 {
>  	struct regulator_bulk_data supplies[2];
>  	struct reset_control *pci_reset;
>  	struct clk *pipe_clk;
> +	struct clk *pipe_clk_mux;
> +	struct clk *pipe_ext_src;
> +	struct clk *ref_clk_src;
>  };
>  
>  union qcom_pcie_resources {
> @@ -1167,6 +1170,20 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>  	if (ret < 0)
>  		return ret;
>  
> +	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280")) {
> +		res->pipe_clk_mux = devm_clk_get(dev, "pipe_src");
> +		if (IS_ERR(res->pipe_clk_mux))
> +			return PTR_ERR(res->pipe_clk_mux);
> +
> +		res->pipe_ext_src = devm_clk_get(dev, "pipe_ext");
> +		if (IS_ERR(res->pipe_ext_src))
> +			return PTR_ERR(res->pipe_ext_src);
> +
> +		res->ref_clk_src = devm_clk_get(dev, "ref");
> +		if (IS_ERR(res->ref_clk_src))
> +			return PTR_ERR(res->ref_clk_src);
> +	}
> +
>  	res->pipe_clk = devm_clk_get(dev, "pipe");
>  	return PTR_ERR_OR_ZERO(res->pipe_clk);
>  }
> @@ -1255,6 +1272,11 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
>  static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> +	struct dw_pcie *pci = pcie->pci;
> +	struct device *dev = pci->dev;
> +
> +	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280"))

If this is something only found on 7280, you need to document (in the
commit message at least) why this does not apply to other platforms with
this controller.

Thanks,
Bjorn

> +		clk_set_parent(res->pipe_clk_mux, res->pipe_ext_src);
>  
>  	return clk_prepare_enable(res->pipe_clk);
>  }
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
Bjorn Andersson June 6, 2021, 2:26 a.m. UTC | #3
On Sat 05 Jun 09:40 CDT 2021, Prasad Malisetty wrote:

> 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 0b6f119..9e8414d 100644

> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi

> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi

> @@ -15,6 +15,7 @@

>  #include <dt-bindings/reset/qcom,sdm845-pdc.h>

>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>

>  #include <dt-bindings/thermal/thermal.h>

> +#include <dt-bindings/gpio/gpio.h>

>  

>  / {

>  	interrupt-parent = <&intc>;

> @@ -484,6 +485,117 @@

>  			#power-domain-cells = <1>;

>  		};

>  

> +		pcie1: pci@1c08000 {


Does this name imply that you have a pcie0 as well? Please introduce it
while you're at it.

> +			compatible = "qcom,pcie-sc7280", "qcom,pcie-sm8250", "snps,dw-pcie";

> +			reg = <0 0x01c08000 0 0x3000>,

> +			      <0 0x40000000 0 0xf1d>,

> +			      <0 0x40000f20 0 0xa8>,

> +			      <0 0x40001000 0 0x1000>,

> +			      <0 0x40100000 0 0x100000>;

> +

> +			reg-names = "parf", "dbi", "elbi", "atu", "config";

> +			device_type = "pci";

> +			linux,pci-domain = <1>;

> +			bus-range = <0x00 0xff>;

> +			num-lanes = <2>;

> +

> +			#address-cells = <3>;

> +			#size-cells = <2>;

> +

> +			ranges = <0x01000000 0x0 0x40200000 0x0 0x40200000 0x0 0x100000>,

> +				 <0x02000000 0x0 0x40300000 0x0 0x40300000 0x0 0x1fd00000>;

> +

> +			interrupts = <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>;

> +			interrupt-names = "msi";

> +			#interrupt-cells = <1>;

> +			interrupt-map-mask = <0 0 0 0x7>;

> +			interrupt-map = <0 0 0 1 &intc 0 434 IRQ_TYPE_LEVEL_HIGH>, /* int_a */

> +					<0 0 0 2 &intc 0 435 IRQ_TYPE_LEVEL_HIGH>, /* int_b */

> +					<0 0 0 3 &intc 0 438 IRQ_TYPE_LEVEL_HIGH>, /* int_c */

> +					<0 0 0 4 &intc 0 439 IRQ_TYPE_LEVEL_HIGH>; /* int_d */

> +

> +			clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,

> +				 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,

> +				 <&pcie1_lane 0>,

> +				 <&rpmhcc RPMH_CXO_CLK>,

> +				 <&gcc GCC_PCIE_1_AUX_CLK>,

> +				 <&gcc GCC_PCIE_1_CFG_AHB_CLK>,

> +				 <&gcc GCC_PCIE_1_MSTR_AXI_CLK>,

> +				 <&gcc GCC_PCIE_1_SLV_AXI_CLK>,

> +				 <&gcc GCC_PCIE_1_SLV_Q2A_AXI_CLK>,

> +				 <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>,

> +				 <&gcc GCC_DDRSS_PCIE_SF_CLK>;

> +

> +			clock-names = "pipe",

> +				      "pipe_src",

> +				      "pipe_ext",

> +				      "ref",

> +				      "aux",

> +				      "cfg",

> +				      "bus_master",

> +				      "bus_slave",

> +				      "slave_q2a",

> +				      "tbu",

> +				      "ddrss_sf_tbu";

> +

> +			assigned-clocks = <&gcc GCC_PCIE_1_AUX_CLK>;

> +			assigned-clock-rates = <19200000>;

> +

> +			resets = <&gcc GCC_PCIE_1_BCR>;

> +			reset-names = "pci";

> +

> +			power-domains = <&gcc GCC_PCIE_1_GDSC>;

> +

> +			phys = <&pcie1_lane>;

> +			phy-names = "pciephy";

> +

> +			perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;

> +			pinctrl-names = "default";

> +			pinctrl-0 = <&pcie1_default_state>;

> +

> +			iommus = <&apps_smmu 0x1c80 0x1>;

> +

> +			iommu-map = <0x0 &apps_smmu 0x1c80 0x1>,

> +				    <0x100 &apps_smmu 0x1c81 0x1>;

> +

> +			status = "disabled";

> +		};

> +

> +		pcie1_phy: phy@1c0e000 {

> +			compatible = "qcom,sm8250-qmp-gen3x2-pcie-phy";


No, you don't have a sm8250-qmp-gen3x2-pcie-phy in your sc7280.

> +			status = "disabled";

> +			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>;

> +

> +			pcie1_lane: lanes@1c0e200 {

> +				reg = <0 0x01c0e200 0 0x170>,

> +				      <0 0x01c0e400 0 0x200>,

> +				      <0 0x01c0ea00 0 0x1f0>,

> +				      <0 0x01c0e600 0 0x170>,

> +				      <0 0x01c0e800 0 0x200>,

> +				      <0 0x01c0ee00 0 0xf4>;

> +				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>,

> @@ -1102,6 +1214,32 @@

>  				pins = "gpio46", "gpio47";

>  				function = "qup13";

>  			};

> +

> +			pcie1_default_state: pcie1-default {


Per the binding the name has to end with "-pins", although I would like
us to change that to "-state". Either way, this is not correct.

> +				clkreq {

> +					pins = "gpio79";

> +					function = "pcie1_clkreqn";

> +				};

> +

> +				reset-n {

> +					pins = "gpio2";

> +					function = "gpio";

> +

> +					drive-strength = <16>;

> +					output-low;

> +					bias-disable;

> +				};

> +

> +				wake-n {

> +					pins = "gpio3";

> +					function = "gpio";

> +				};

> +

> +				nvme-n {


This doesn't look like a standard PCIe pin, is it perhaps the enable pin
for the regulator powering your NVME, or something along those lines?

If so you should describe it as a fixed-regulator...and either way I
suspect it should be moved to the device specific file.

Regards,
Bjorn

> +					pins = "gpio19";

> +					function = "gpio";

> +				};

> +			};

>  		};

>  

>  		apps_smmu: iommu@15000000 {

> -- 

> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,

> a Linux Foundation Collaborative Project

>
Prasad Malisetty June 18, 2021, 5:52 a.m. UTC | #4
On 2021-06-06 07:56, Bjorn Andersson wrote:
> On Sat 05 Jun 09:40 CDT 2021, Prasad Malisetty wrote:

> 

>> 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 0b6f119..9e8414d 100644

>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi

>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi

>> @@ -15,6 +15,7 @@

>>  #include <dt-bindings/reset/qcom,sdm845-pdc.h>

>>  #include <dt-bindings/soc/qcom,rpmh-rsc.h>

>>  #include <dt-bindings/thermal/thermal.h>

>> +#include <dt-bindings/gpio/gpio.h>

>> 

>>  / {

>>  	interrupt-parent = <&intc>;

>> @@ -484,6 +485,117 @@

>>  			#power-domain-cells = <1>;

>>  		};

>> 

>> +		pcie1: pci@1c08000 {

> 

> Does this name imply that you have a pcie0 as well? Please introduce it

> while you're at it.

> 

>> We are not using pcie0 for HLOS.


>> +			compatible = "qcom,pcie-sc7280", "qcom,pcie-sm8250", 

>> "snps,dw-pcie";

>> +			reg = <0 0x01c08000 0 0x3000>,

>> +			      <0 0x40000000 0 0xf1d>,

>> +			      <0 0x40000f20 0 0xa8>,

>> +			      <0 0x40001000 0 0x1000>,

>> +			      <0 0x40100000 0 0x100000>;

>> +

>> +			reg-names = "parf", "dbi", "elbi", "atu", "config";

>> +			device_type = "pci";

>> +			linux,pci-domain = <1>;

>> +			bus-range = <0x00 0xff>;

>> +			num-lanes = <2>;

>> +

>> +			#address-cells = <3>;

>> +			#size-cells = <2>;

>> +

>> +			ranges = <0x01000000 0x0 0x40200000 0x0 0x40200000 0x0 0x100000>,

>> +				 <0x02000000 0x0 0x40300000 0x0 0x40300000 0x0 0x1fd00000>;

>> +

>> +			interrupts = <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>;

>> +			interrupt-names = "msi";

>> +			#interrupt-cells = <1>;

>> +			interrupt-map-mask = <0 0 0 0x7>;

>> +			interrupt-map = <0 0 0 1 &intc 0 434 IRQ_TYPE_LEVEL_HIGH>, /* 

>> int_a */

>> +					<0 0 0 2 &intc 0 435 IRQ_TYPE_LEVEL_HIGH>, /* int_b */

>> +					<0 0 0 3 &intc 0 438 IRQ_TYPE_LEVEL_HIGH>, /* int_c */

>> +					<0 0 0 4 &intc 0 439 IRQ_TYPE_LEVEL_HIGH>; /* int_d */

>> +

>> +			clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,

>> +				 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,

>> +				 <&pcie1_lane 0>,

>> +				 <&rpmhcc RPMH_CXO_CLK>,

>> +				 <&gcc GCC_PCIE_1_AUX_CLK>,

>> +				 <&gcc GCC_PCIE_1_CFG_AHB_CLK>,

>> +				 <&gcc GCC_PCIE_1_MSTR_AXI_CLK>,

>> +				 <&gcc GCC_PCIE_1_SLV_AXI_CLK>,

>> +				 <&gcc GCC_PCIE_1_SLV_Q2A_AXI_CLK>,

>> +				 <&gcc GCC_AGGRE_NOC_PCIE_TBU_CLK>,

>> +				 <&gcc GCC_DDRSS_PCIE_SF_CLK>;

>> +

>> +			clock-names = "pipe",

>> +				      "pipe_src",

>> +				      "pipe_ext",

>> +				      "ref",

>> +				      "aux",

>> +				      "cfg",

>> +				      "bus_master",

>> +				      "bus_slave",

>> +				      "slave_q2a",

>> +				      "tbu",

>> +				      "ddrss_sf_tbu";

>> +

>> +			assigned-clocks = <&gcc GCC_PCIE_1_AUX_CLK>;

>> +			assigned-clock-rates = <19200000>;

>> +

>> +			resets = <&gcc GCC_PCIE_1_BCR>;

>> +			reset-names = "pci";

>> +

>> +			power-domains = <&gcc GCC_PCIE_1_GDSC>;

>> +

>> +			phys = <&pcie1_lane>;

>> +			phy-names = "pciephy";

>> +

>> +			perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;

>> +			pinctrl-names = "default";

>> +			pinctrl-0 = <&pcie1_default_state>;

>> +

>> +			iommus = <&apps_smmu 0x1c80 0x1>;

>> +

>> +			iommu-map = <0x0 &apps_smmu 0x1c80 0x1>,

>> +				    <0x100 &apps_smmu 0x1c81 0x1>;

>> +

>> +			status = "disabled";

>> +		};

>> +

>> +		pcie1_phy: phy@1c0e000 {

>> +			compatible = "qcom,sm8250-qmp-gen3x2-pcie-phy";

> 

> No, you don't have a sm8250-qmp-gen3x2-pcie-phy in your sc7280.

> 

>> Both are having same PHY sequence.


>> +			status = "disabled";

>> +			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>;

>> +

>> +			pcie1_lane: lanes@1c0e200 {

>> +				reg = <0 0x01c0e200 0 0x170>,

>> +				      <0 0x01c0e400 0 0x200>,

>> +				      <0 0x01c0ea00 0 0x1f0>,

>> +				      <0 0x01c0e600 0 0x170>,

>> +				      <0 0x01c0e800 0 0x200>,

>> +				      <0 0x01c0ee00 0 0xf4>;

>> +				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>,

>> @@ -1102,6 +1214,32 @@

>>  				pins = "gpio46", "gpio47";

>>  				function = "qup13";

>>  			};

>> +

>> +			pcie1_default_state: pcie1-default {

> 

> Per the binding the name has to end with "-pins", although I would like

> us to change that to "-state". Either way, this is not correct.

> 

>> +				clkreq {

>> +					pins = "gpio79";

>> +					function = "pcie1_clkreqn";

>> +				};

>> +

>> +				reset-n {

>> +					pins = "gpio2";

>> +					function = "gpio";

>> +

>> +					drive-strength = <16>;

>> +					output-low;

>> +					bias-disable;

>> +				};

>> +

>> +				wake-n {

>> +					pins = "gpio3";

>> +					function = "gpio";

>> +				};

>> +

>> +				nvme-n {

> 

> This doesn't look like a standard PCIe pin, is it perhaps the enable 

> pin

> for the regulator powering your NVME, or something along those lines?

> 

> If so you should describe it as a fixed-regulator...and either way I

> suspect it should be moved to the device specific file.

> 

> Regards,

> Bjorn

> 

> Agree, will move into board specific file.


>> +					pins = "gpio19";

>> +					function = "gpio";

>> +				};

>> +			};

>>  		};

>> 

>>  		apps_smmu: iommu@15000000 {

>> --

>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 

>> Forum,

>> a Linux Foundation Collaborative Project

>>
Prasad Malisetty June 18, 2021, 1 p.m. UTC | #5
On 2021-06-06 02:56, Stephen Boyd wrote:
> Quoting Prasad Malisetty (2021-06-05 07:40:58)
>> In PCIe driver pipe-clk mux needs to switch between pipe_clk
>> and XO for GDSC enable. This is done by setting pipe_clk mux
>> as parent of pipe_clk after phy init.
> 
> Just to confirm, we can't set this parent via assigned-clock-parents
> property in DT?
> 
>> 
This clock setting need be done after phy init.

>> Signed-off-by: Prasad Malisetty <pmaliset@codeaurora.org>
>> ---
>>  drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>> 
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c 
>> b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 8a7a300..5cbbea4 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -166,6 +166,9 @@ struct qcom_pcie_resources_2_7_0 {
>>         struct regulator_bulk_data supplies[2];
>>         struct reset_control *pci_reset;
>>         struct clk *pipe_clk;
>> +       struct clk *pipe_clk_mux;
>> +       struct clk *pipe_ext_src;
>> +       struct clk *ref_clk_src;
>>  };
>> 
>>  union qcom_pcie_resources {
>> @@ -1167,6 +1170,20 @@ static int qcom_pcie_get_resources_2_7_0(struct 
>> qcom_pcie *pcie)
>>         if (ret < 0)
>>                 return ret;
>> 
>> +       if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280")) 
>> {
>> +               res->pipe_clk_mux = devm_clk_get(dev, "pipe_src");
>> +               if (IS_ERR(res->pipe_clk_mux))
>> +                       return PTR_ERR(res->pipe_clk_mux);
>> +
>> +               res->pipe_ext_src = devm_clk_get(dev, "pipe_ext");
>> +               if (IS_ERR(res->pipe_ext_src))
>> +                       return PTR_ERR(res->pipe_ext_src);
>> +
>> +               res->ref_clk_src = devm_clk_get(dev, "ref");
> 
> Is this going to be used by any code?
> 
Yes, ref clock will be used in system suspend case. currently system 
suspend changes are in under validation.

>> +               if (IS_ERR(res->ref_clk_src))
>> +                       return PTR_ERR(res->ref_clk_src);
>> +       }
>> +
>>         res->pipe_clk = devm_clk_get(dev, "pipe");
>>         return PTR_ERR_OR_ZERO(res->pipe_clk);
>>  }
>> @@ -1255,6 +1272,11 @@ static void qcom_pcie_deinit_2_7_0(struct 
>> qcom_pcie *pcie)
>>  static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
>>  {
>>         struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
>> +       struct dw_pcie *pci = pcie->pci;
>> +       struct device *dev = pci->dev;
>> +
>> +       if (of_device_is_compatible(dev->of_node, "qcom,pcie-sc7280"))
>> +               clk_set_parent(res->pipe_clk_mux, res->pipe_ext_src);
>> 
>>         return clk_prepare_enable(res->pipe_clk);
>>  }
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>>