mbox series

[v2,00/10] qcom: add support for PCIe0 on SM8450 platform

Message ID 20211208171442.1327689-1-dmitry.baryshkov@linaro.org
Headers show
Series qcom: add support for PCIe0 on SM8450 platform | expand

Message

Dmitry Baryshkov Dec. 8, 2021, 5:14 p.m. UTC
There are two different PCIe controllers and PHYs on SM8450, one having
one lane and another with two lanes. This set of patches adds support
for the first PCIe phy and controller only, support for the second PCIe
part will come later.

Changes since v1:
 - Fix capitalization/wording of PCI patch subjects
 - Add missing gen3x1 specification to PHY table names

----------------------------------------------------------------
Dmitry Baryshkov (10):
      dt-bindings: pci: qcom: Document PCIe bindings for SM8450
      dt-bindings: phy: qcom,qmp: Add SM8450 PCIe PHY bindings
      phy: qcom-qmp: Add SM8450 PCIe0 PHY support
      PCI: qcom: Remove redundancy between qcom_pcie and qcom_pcie_cfg
      PCI: qcom: Add ddrss_sf_tbu flag
      PCI: qcom: Add SM8450 PCIe support
      arm64: dts: qcom: sm8450: add PCIe0 PHY node
      arm64: dts: qcom: sm8450: add PCIe0 RC device
      arm64: dts: qcom: sm8450-qrd: enable PCIe0 PHY device
      arm64: dts: qcom: sm8450-qrd: enable PCIe0 host

 .../devicetree/bindings/pci/qcom,pcie.txt          |  21 ++-
 .../devicetree/bindings/phy/qcom,qmp-phy.yaml      |   2 +
 arch/arm64/boot/dts/qcom/sm8450-qrd.dts            |  14 ++
 arch/arm64/boot/dts/qcom/sm8450.dtsi               | 143 ++++++++++++++++++++-
 drivers/pci/controller/dwc/pcie-qcom.c             |  89 ++++++++-----
 drivers/phy/qualcomm/phy-qcom-qmp.c                | 125 ++++++++++++++++++
 drivers/phy/qualcomm/phy-qcom-qmp.h                |  33 +++++
 7 files changed, 389 insertions(+), 38 deletions(-)

Comments

Manivannan Sadhasivam Dec. 10, 2021, 8:42 a.m. UTC | #1
On Wed, Dec 08, 2021 at 08:14:33PM +0300, Dmitry Baryshkov wrote:
> Document the PCIe DT bindings for SM8450 SoC.The PCIe IP is similar
> to the one used on SM8250. Add the compatible for SM8450.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Note to self: This binding should be converted to YAML very soon.

Thanks,
Mani

> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt     | 21 ++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index a0ae024c2d0c..73bc763c5009 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -15,6 +15,7 @@
>  			- "qcom,pcie-sc8180x" for sc8180x
>  			- "qcom,pcie-sdm845" for sdm845
>  			- "qcom,pcie-sm8250" for sm8250
> +			- "qcom,pcie-sm8450" for sm8450
>  			- "qcom,pcie-ipq6018" for ipq6018
>  
>  - reg:
> @@ -169,6 +170,24 @@
>  			- "ddrss_sf_tbu" PCIe SF TBU clock
>  			- "pipe"	PIPE clock
>  
> +- clock-names:
> +	Usage: required for sm8450
> +	Value type: <stringlist>
> +	Definition: Should contain the following entries
> +			- "aux"         Auxiliary clock
> +			- "cfg"         Configuration clock
> +			- "bus_master"  Master AXI clock
> +			- "bus_slave"   Slave AXI clock
> +			- "slave_q2a"   Slave Q2A clock
> +			- "tbu"         PCIe TBU clock
> +			- "ddrss_sf_tbu" PCIe SF TBU clock
> +			- "pipe"        PIPE clock
> +			- "pipe_mux"    PIPE MUX
> +			- "phy_pipe"    PIPE output clock
> +			- "ref"         REFERENCE clock
> +			- "aggre0"	Aggre NoC PCIe0 AXI clock
> +			- "aggre1"	Aggre NoC PCIe1 AXI clock
> +
>  - resets:
>  	Usage: required
>  	Value type: <prop-encoded-array>
> @@ -246,7 +265,7 @@
>  			- "ahb"			AHB reset
>  
>  - reset-names:
> -	Usage: required for sc8180x, sdm845 and sm8250
> +	Usage: required for sc8180x, sdm845, sm8250 and sm8450
>  	Value type: <stringlist>
>  	Definition: Should contain the following entries
>  			- "pci"			PCIe core reset
> -- 
> 2.33.0
>
Manivannan Sadhasivam Dec. 10, 2021, 11:15 a.m. UTC | #2
On Wed, Dec 08, 2021 at 08:14:36PM +0300, Dmitry Baryshkov wrote:
> In preparation to adding more flags to configuration data, use struct
> qcom_pcie_cfg directly inside struct qcom_pcie, rather than duplicating
> all its fields. This would save us from the boilerplate code that just
> copies flags values from one sruct to another one.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 39 +++++++++++---------------
>  1 file changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 1c3d1116bb60..51a0475173fb 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -204,8 +204,7 @@ struct qcom_pcie {
>  	union qcom_pcie_resources res;
>  	struct phy *phy;
>  	struct gpio_desc *reset;
> -	const struct qcom_pcie_ops *ops;
> -	unsigned int pipe_clk_need_muxing:1;
> +	const struct qcom_pcie_cfg *cfg;

There is no change in this patch that adds "pipe_clk_need_muxing" to
qcom_pcie_cfg.

Thanks,
Mani

>  };
>  
>  #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
> @@ -229,8 +228,8 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
>  	struct qcom_pcie *pcie = to_qcom_pcie(pci);
>  
>  	/* Enable Link Training state machine */
> -	if (pcie->ops->ltssm_enable)
> -		pcie->ops->ltssm_enable(pcie);
> +	if (pcie->cfg->ops->ltssm_enable)
> +		pcie->cfg->ops->ltssm_enable(pcie);
>  
>  	return 0;
>  }
> @@ -1176,7 +1175,7 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (pcie->pipe_clk_need_muxing) {
> +	if (pcie->cfg->pipe_clk_need_muxing) {
>  		res->pipe_clk_src = devm_clk_get(dev, "pipe_mux");
>  		if (IS_ERR(res->pipe_clk_src))
>  			return PTR_ERR(res->pipe_clk_src);
> @@ -1209,7 +1208,7 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
>  	}
>  
>  	/* Set TCXO as clock source for pcie_pipe_clk_src */
> -	if (pcie->pipe_clk_need_muxing)
> +	if (pcie->cfg->pipe_clk_need_muxing)
>  		clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
>  
>  	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> @@ -1284,7 +1283,7 @@ 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;
>  
>  	/* Set pipe clock as clock source for pcie_pipe_clk_src */
> -	if (pcie->pipe_clk_need_muxing)
> +	if (pcie->cfg->pipe_clk_need_muxing)
>  		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
>  
>  	return clk_prepare_enable(res->pipe_clk);
> @@ -1384,7 +1383,7 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
>  
>  	qcom_ep_reset_assert(pcie);
>  
> -	ret = pcie->ops->init(pcie);
> +	ret = pcie->cfg->ops->init(pcie);
>  	if (ret)
>  		return ret;
>  
> @@ -1392,16 +1391,16 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
>  	if (ret)
>  		goto err_deinit;
>  
> -	if (pcie->ops->post_init) {
> -		ret = pcie->ops->post_init(pcie);
> +	if (pcie->cfg->ops->post_init) {
> +		ret = pcie->cfg->ops->post_init(pcie);
>  		if (ret)
>  			goto err_disable_phy;
>  	}
>  
>  	qcom_ep_reset_deassert(pcie);
>  
> -	if (pcie->ops->config_sid) {
> -		ret = pcie->ops->config_sid(pcie);
> +	if (pcie->cfg->ops->config_sid) {
> +		ret = pcie->cfg->ops->config_sid(pcie);
>  		if (ret)
>  			goto err;
>  	}
> @@ -1410,12 +1409,12 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
>  
>  err:
>  	qcom_ep_reset_assert(pcie);
> -	if (pcie->ops->post_deinit)
> -		pcie->ops->post_deinit(pcie);
> +	if (pcie->cfg->ops->post_deinit)
> +		pcie->cfg->ops->post_deinit(pcie);
>  err_disable_phy:
>  	phy_power_off(pcie->phy);
>  err_deinit:
> -	pcie->ops->deinit(pcie);
> +	pcie->cfg->ops->deinit(pcie);
>  
>  	return ret;
>  }
> @@ -1531,7 +1530,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	struct pcie_port *pp;
>  	struct dw_pcie *pci;
>  	struct qcom_pcie *pcie;
> -	const struct qcom_pcie_cfg *pcie_cfg;
>  	int ret;
>  
>  	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> @@ -1553,15 +1551,12 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  
>  	pcie->pci = pci;
>  
> -	pcie_cfg = of_device_get_match_data(dev);
> -	if (!pcie_cfg || !pcie_cfg->ops) {
> +	pcie->cfg = of_device_get_match_data(dev);
> +	if (!pcie->cfg || !pcie->cfg->ops) {
>  		dev_err(dev, "Invalid platform data\n");
>  		return -EINVAL;
>  	}
>  
> -	pcie->ops = pcie_cfg->ops;
> -	pcie->pipe_clk_need_muxing = pcie_cfg->pipe_clk_need_muxing;
> -
>  	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
>  	if (IS_ERR(pcie->reset)) {
>  		ret = PTR_ERR(pcie->reset);
> @@ -1586,7 +1581,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  		goto err_pm_runtime_put;
>  	}
>  
> -	ret = pcie->ops->get_resources(pcie);
> +	ret = pcie->cfg->ops->get_resources(pcie);
>  	if (ret)
>  		goto err_pm_runtime_put;
>  
> -- 
> 2.33.0
>
Manivannan Sadhasivam Dec. 10, 2021, 11:22 a.m. UTC | #3
On Wed, Dec 08, 2021 at 08:14:37PM +0300, Dmitry Baryshkov wrote:
> Qualcomm PCIe driver uses compatible string to check if the ddrss_sf_tbu
> clock should be used. Since sc7280 support has added flags, switch to
> the new mechanism to check if this clock should be used.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 51a0475173fb..803d3ac18c56 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -194,7 +194,9 @@ struct qcom_pcie_ops {
>  
>  struct qcom_pcie_cfg {
>  	const struct qcom_pcie_ops *ops;
> +	/* flags for ops 2.7.0 and 1.9.0 */

No need of this comment.

>  	unsigned int pipe_clk_need_muxing:1;

This should be added in the previous patch.

> +	unsigned int has_ddrss_sf_tbu_clk:1;

Wondering if we could make both the flags "bool" as the values passed to it
are of boolean type. I don't think we could save a significant amount of
memory using bitfields.

Thanks,
Mani
>  };
>  
>  struct qcom_pcie {
> @@ -1164,7 +1166,7 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>  	res->clks[3].id = "bus_slave";
>  	res->clks[4].id = "slave_q2a";
>  	res->clks[5].id = "tbu";
> -	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sm8250")) {
> +	if (pcie->cfg->has_ddrss_sf_tbu_clk) {
>  		res->clks[6].id = "ddrss_sf_tbu";
>  		res->num_clks = 7;
>  	} else {
> @@ -1512,6 +1514,7 @@ static const struct qcom_pcie_cfg sdm845_cfg = {
>  
>  static const struct qcom_pcie_cfg sm8250_cfg = {
>  	.ops = &ops_1_9_0,
> +	.has_ddrss_sf_tbu_clk = true,
>  };
>  
>  static const struct qcom_pcie_cfg sc7280_cfg = {
> -- 
> 2.33.0
>
Manivannan Sadhasivam Dec. 10, 2021, 11:37 a.m. UTC | #4
On Wed, Dec 08, 2021 at 08:14:39PM +0300, Dmitry Baryshkov wrote:
> Add device tree node for the first PCIe PHY device found on the Qualcomm
> SM8450 platform.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8450.dtsi | 42 ++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> index 16a789cacb65..a047d8a22897 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -558,8 +558,12 @@ gcc: clock-controller@100000 {
>  			#clock-cells = <1>;
>  			#reset-cells = <1>;
>  			#power-domain-cells = <1>;
> -			clock-names = "bi_tcxo", "sleep_clk";
> -			clocks = <&rpmhcc RPMH_CXO_CLK>, <&sleep_clk>;
> +			clocks = <&rpmhcc RPMH_CXO_CLK>,
> +				 <&pcie0_lane>,
> +				 <&sleep_clk>;
> +			clock-names = "bi_tcxo",
> +				      "pcie_0_pipe_clk",
> +				      "sleep_clk";
>  		};
>  
>  		qupv3_id_0: geniqup@9c0000 {
> @@ -625,6 +629,40 @@ i2c14: i2c@a98000 {
>  			};
>  		};
>  
> +		pcie0_phy: phy@1c06000 {
> +			compatible = "qcom,sm8450-qmp-gen3x1-pcie-phy";
> +			reg = <0 0x01c06000 0 0x200>;
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			ranges;
> +			clocks = <&gcc GCC_PCIE_0_AUX_CLK>,
> +				 <&gcc GCC_PCIE_0_CFG_AHB_CLK>,
> +				 <&gcc GCC_PCIE_0_CLKREF_EN>,
> +				 <&gcc GCC_PCIE_0_PHY_RCHNG_CLK>;
> +			clock-names = "aux", "cfg_ahb", "ref", "refgen";
> +
> +			resets = <&gcc GCC_PCIE_0_PHY_BCR>;
> +			reset-names = "phy";
> +
> +			assigned-clocks = <&gcc GCC_PCIE_0_PHY_RCHNG_CLK>;
> +			assigned-clock-rates = <100000000>;
> +
> +			status = "disabled";
> +
> +			pcie0_lane: lanes@1c06200 {
> +				reg = <0 0x1c06e00 0 0x200>, /* tx */
> +				      <0 0x1c07000 0 0x200>, /* rx */
> +				      <0 0x1c06200 0 0x200>, /* pcs */

Oh, so this platform has "PCS" at the starting offset? This is different
compared to other platforms as "TX" always comes first.

And the size is "0x200" for all?

Thanks,
Mani

> +				      <0 0x1c06600 0 0x200>; /* pcs_pcie */
> +				clocks = <&gcc GCC_PCIE_0_PIPE_CLK>;
> +				clock-names = "pipe0";
> +
> +				#clock-cells = <0>;
> +				#phy-cells = <0>;
> +				clock-output-names = "pcie_0_pipe_clk";
> +			};
> +		};
> +
>  		config_noc: interconnect@1500000 {
>  			compatible = "qcom,sm8450-config-noc";
>  			reg = <0 0x01500000 0 0x1c000>;
> -- 
> 2.33.0
>
Dmitry Baryshkov Dec. 11, 2021, 1:59 a.m. UTC | #5
On 10/12/2021 14:22, Manivannan Sadhasivam wrote:
> On Wed, Dec 08, 2021 at 08:14:37PM +0300, Dmitry Baryshkov wrote:
>> Qualcomm PCIe driver uses compatible string to check if the ddrss_sf_tbu
>> clock should be used. Since sc7280 support has added flags, switch to
>> the new mechanism to check if this clock should be used.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 51a0475173fb..803d3ac18c56 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -194,7 +194,9 @@ struct qcom_pcie_ops {
>>   
>>   struct qcom_pcie_cfg {
>>   	const struct qcom_pcie_ops *ops;
>> +	/* flags for ops 2.7.0 and 1.9.0 */
> 
> No need of this comment.

Dropping it

> 
>>   	unsigned int pipe_clk_need_muxing:1;
> 
> This should be added in the previous patch.

It exists already

> 
>> +	unsigned int has_ddrss_sf_tbu_clk:1;
> 
> Wondering if we could make both the flags "bool" as the values passed to it
> are of boolean type. I don't think we could save a significant amount of
> memory using bitfields.

I followed the existing pipe_clk_need_muxing. I have no strong 
preference here, so let's see what Bjorn will prefer.

> 
> Thanks,
> Mani
>>   };
>>   
>>   struct qcom_pcie {
>> @@ -1164,7 +1166,7 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>>   	res->clks[3].id = "bus_slave";
>>   	res->clks[4].id = "slave_q2a";
>>   	res->clks[5].id = "tbu";
>> -	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sm8250")) {
>> +	if (pcie->cfg->has_ddrss_sf_tbu_clk) {
>>   		res->clks[6].id = "ddrss_sf_tbu";
>>   		res->num_clks = 7;
>>   	} else {
>> @@ -1512,6 +1514,7 @@ static const struct qcom_pcie_cfg sdm845_cfg = {
>>   
>>   static const struct qcom_pcie_cfg sm8250_cfg = {
>>   	.ops = &ops_1_9_0,
>> +	.has_ddrss_sf_tbu_clk = true,
>>   };
>>   
>>   static const struct qcom_pcie_cfg sc7280_cfg = {
>> -- 
>> 2.33.0
>>
Dmitry Baryshkov Dec. 11, 2021, 2:10 a.m. UTC | #6
On 10/12/2021 14:37, Manivannan Sadhasivam wrote:
> On Wed, Dec 08, 2021 at 08:14:39PM +0300, Dmitry Baryshkov wrote:
>> Add device tree node for the first PCIe PHY device found on the Qualcomm
>> SM8450 platform.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   arch/arm64/boot/dts/qcom/sm8450.dtsi | 42 ++++++++++++++++++++++++++--
>>   1 file changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> index 16a789cacb65..a047d8a22897 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> @@ -558,8 +558,12 @@ gcc: clock-controller@100000 {
>>   			#clock-cells = <1>;
>>   			#reset-cells = <1>;
>>   			#power-domain-cells = <1>;
>> -			clock-names = "bi_tcxo", "sleep_clk";
>> -			clocks = <&rpmhcc RPMH_CXO_CLK>, <&sleep_clk>;
>> +			clocks = <&rpmhcc RPMH_CXO_CLK>,
>> +				 <&pcie0_lane>,
>> +				 <&sleep_clk>;
>> +			clock-names = "bi_tcxo",
>> +				      "pcie_0_pipe_clk",
>> +				      "sleep_clk";
>>   		};
>>   
>>   		qupv3_id_0: geniqup@9c0000 {
>> @@ -625,6 +629,40 @@ i2c14: i2c@a98000 {
>>   			};
>>   		};
>>   
>> +		pcie0_phy: phy@1c06000 {
>> +			compatible = "qcom,sm8450-qmp-gen3x1-pcie-phy";
>> +			reg = <0 0x01c06000 0 0x200>;
>> +			#address-cells = <2>;
>> +			#size-cells = <2>;
>> +			ranges;
>> +			clocks = <&gcc GCC_PCIE_0_AUX_CLK>,
>> +				 <&gcc GCC_PCIE_0_CFG_AHB_CLK>,
>> +				 <&gcc GCC_PCIE_0_CLKREF_EN>,
>> +				 <&gcc GCC_PCIE_0_PHY_RCHNG_CLK>;
>> +			clock-names = "aux", "cfg_ahb", "ref", "refgen";
>> +
>> +			resets = <&gcc GCC_PCIE_0_PHY_BCR>;
>> +			reset-names = "phy";
>> +
>> +			assigned-clocks = <&gcc GCC_PCIE_0_PHY_RCHNG_CLK>;
>> +			assigned-clock-rates = <100000000>;
>> +
>> +			status = "disabled";
>> +
>> +			pcie0_lane: lanes@1c06200 {
>> +				reg = <0 0x1c06e00 0 0x200>, /* tx */
>> +				      <0 0x1c07000 0 0x200>, /* rx */
>> +				      <0 0x1c06200 0 0x200>, /* pcs */
> 
> Oh, so this platform has "PCS" at the starting offset? This is different
> compared to other platforms as "TX" always comes first.
> 

Yes. this is correct.


> And the size is "0x200" for all?

It is for the PCS block.

As you see below, PCS_PCIE starts at 0x600. Initially I thought about 
extend it further, making it cover few other regions (up to the tx 
region). However as we do not touch other regions, I decided to keep it 
as this way.

> 
> Thanks,
> Mani
> 
>> +				      <0 0x1c06600 0 0x200>; /* pcs_pcie */
>> +				clocks = <&gcc GCC_PCIE_0_PIPE_CLK>;
>> +				clock-names = "pipe0";
>> +
>> +				#clock-cells = <0>;
>> +				#phy-cells = <0>;
>> +				clock-output-names = "pcie_0_pipe_clk";
>> +			};
>> +		};
>> +
>>   		config_noc: interconnect@1500000 {
>>   			compatible = "qcom,sm8450-config-noc";
>>   			reg = <0 0x01500000 0 0x1c000>;
>> -- 
>> 2.33.0
>>
Manivannan Sadhasivam Dec. 11, 2021, 3:11 a.m. UTC | #7
On Sat, Dec 11, 2021 at 04:59:05AM +0300, Dmitry Baryshkov wrote:
> On 10/12/2021 14:22, Manivannan Sadhasivam wrote:
> > On Wed, Dec 08, 2021 at 08:14:37PM +0300, Dmitry Baryshkov wrote:
> > > Qualcomm PCIe driver uses compatible string to check if the ddrss_sf_tbu
> > > clock should be used. Since sc7280 support has added flags, switch to
> > > the new mechanism to check if this clock should be used.
> > > 
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >   drivers/pci/controller/dwc/pcie-qcom.c | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 51a0475173fb..803d3ac18c56 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -194,7 +194,9 @@ struct qcom_pcie_ops {
> > >   struct qcom_pcie_cfg {
> > >   	const struct qcom_pcie_ops *ops;
> > > +	/* flags for ops 2.7.0 and 1.9.0 */
> > 
> > No need of this comment.
> 
> Dropping it
> 
> > 
> > >   	unsigned int pipe_clk_need_muxing:1;
> > 
> > This should be added in the previous patch.
> 
> It exists already
> 

Ah, my tree was outdated. I do see it in -rc4.

> > 
> > > +	unsigned int has_ddrss_sf_tbu_clk:1;
> > 
> > Wondering if we could make both the flags "bool" as the values passed to it
> > are of boolean type. I don't think we could save a significant amount of
> > memory using bitfields.
> 
> I followed the existing pipe_clk_need_muxing. I have no strong preference
> here, so let's see what Bjorn will prefer.
> 

Okay.

Thanks,
Mani

> > 
> > Thanks,
> > Mani
> > >   };
> > >   struct qcom_pcie {
> > > @@ -1164,7 +1166,7 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
> > >   	res->clks[3].id = "bus_slave";
> > >   	res->clks[4].id = "slave_q2a";
> > >   	res->clks[5].id = "tbu";
> > > -	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sm8250")) {
> > > +	if (pcie->cfg->has_ddrss_sf_tbu_clk) {
> > >   		res->clks[6].id = "ddrss_sf_tbu";
> > >   		res->num_clks = 7;
> > >   	} else {
> > > @@ -1512,6 +1514,7 @@ static const struct qcom_pcie_cfg sdm845_cfg = {
> > >   static const struct qcom_pcie_cfg sm8250_cfg = {
> > >   	.ops = &ops_1_9_0,
> > > +	.has_ddrss_sf_tbu_clk = true,
> > >   };
> > >   static const struct qcom_pcie_cfg sc7280_cfg = {
> > > -- 
> > > 2.33.0
> > > 
> 
> 
> -- 
> With best wishes
> Dmitry