diff mbox series

[V3,6/6] PCI: qcom: Add support for IPQ9574

Message ID	20230421124938.21974-7-quic_devipriy@quicinc.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Devi Priya <quic_devipriy@quicinc.com> To: <agross@kernel.org>, <andersson@kernel.org>, <konrad.dybcio@linaro.org>, <lpieralisi@kernel.org>, <kw@linux.com>, <robh@kernel.org>, <bhelgaas@google.com>, <krzysztof.kozlowski+dt@linaro.org>, <mturquette@baylibre.com>, <sboyd@kernel.org>, <mani@kernel.org>, <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-pci@vger.kernel.org>, <linux-clk@vger.kernel.org> CC: <quic_srichara@quicinc.com>, <quic_sjaganat@quicinc.com>, <quic_kathirav@quicinc.com>, <quic_arajkuma@quicinc.com>, <quic_anusha@quicinc.com>, <quic_ipkumar@quicinc.com> Subject: [PATCH V3 6/6] PCI: qcom: Add support for IPQ9574 Date: Fri, 21 Apr 2023 18:19:38 +0530 Message-ID: <20230421124938.21974-7-quic_devipriy@quicinc.com> In-Reply-To: <20230421124938.21974-1-quic_devipriy@quicinc.com> References: <20230421124938.21974-1-quic_devipriy@quicinc.com> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk
Series	Add PCIe support for IPQ9574 \| [V3,0/6] Add PCIe support for IPQ9574 [V3,1/6] dt-bindings: clock: Add PCIe pipe clock definitions [V3,2/6] clk: qcom: gcc-ipq9574: Add PCIe pipe clocks [V3,3/6] dt-bindings: PCI: qcom: Add IPQ9574 [V3,4/6] arm64: dts: qcom: ipq9574: Add PCIe PHYs and controller nodes [V3,5/6] arm64: dts: qcom: ipq9574: Enable PCIe PHYs and controllers [V3,6/6] PCI: qcom: Add support for IPQ9574

Commit Message

Devi Priya April 21, 2023, 12:49 p.m. UTC

  The IPQ9574 platform has 4 Gen3 PCIe controllers: two single-lane
and two dual-lane based on SNPS core 5.70a
The Qcom IP rev is 1.27.0 and Synopsys IP rev is 5.80a
Added a new compatible 'qcom,pcie-ipq9574' and 'ops_1_27_0'
which reuses all the members of 'ops_2_9_0' except for the post_init
as the SLV_ADDR_SPACE_SIZE configuration differs between 2_9_0
and 1_27_0.
Also, modified get_resources of 'ops 2_9_0' to get the clocks
from the device tree and modelled the post init sequence as
a common function to avoid code redundancy.

Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
---
 Changes in V3:
	- Rebased on top of linux-next/master

 drivers/pci/controller/dwc/pcie-qcom.c | 61 ++++++++++++++++++--------
 1 file changed, 43 insertions(+), 18 deletions(-)

Comments

Dmitry Baryshkov April 22, 2023, 12:05 a.m. UTC | #1

On Fri, 21 Apr 2023 at 15:51, Devi Priya <quic_devipriy@quicinc.com> wrote:
>
> The IPQ9574 platform has 4 Gen3 PCIe controllers: two single-lane
> and two dual-lane based on SNPS core 5.70a
> The Qcom IP rev is 1.27.0 and Synopsys IP rev is 5.80a
> Added a new compatible 'qcom,pcie-ipq9574' and 'ops_1_27_0'
> which reuses all the members of 'ops_2_9_0' except for the post_init
> as the SLV_ADDR_SPACE_SIZE configuration differs between 2_9_0
> and 1_27_0.
> Also, modified get_resources of 'ops 2_9_0' to get the clocks
> from the device tree and modelled the post init sequence as
> a common function to avoid code redundancy.
>
> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> ---
>  Changes in V3:
>         - Rebased on top of linux-next/master
>
>  drivers/pci/controller/dwc/pcie-qcom.c | 61 ++++++++++++++++++--------
>  1 file changed, 43 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 4ab30892f6ef..3682ecdead1f 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -107,6 +107,7 @@
>
>  /* PARF_SLV_ADDR_SPACE_SIZE register value */
>  #define SLV_ADDR_SPACE_SZ                      0x10000000
> +#define SLV_ADDR_SPACE_SZ_1_27_0               0x08000000
>
>  /* PARF_MHI_CLOCK_RESET_CTRL register fields */
>  #define AHB_CLK_EN                             BIT(0)
> @@ -202,10 +203,10 @@ struct qcom_pcie_resources_2_7_0 {
>         struct reset_control *rst;
>  };
>
> -#define QCOM_PCIE_2_9_0_MAX_CLOCKS             5
>  struct qcom_pcie_resources_2_9_0 {
> -       struct clk_bulk_data clks[QCOM_PCIE_2_9_0_MAX_CLOCKS];
> +       struct clk_bulk_data *clks;
>         struct reset_control *rst;
> +       int num_clks;
>  };
>
>  union qcom_pcie_resources {
> @@ -1050,17 +1051,10 @@ static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
>         struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>         struct dw_pcie *pci = pcie->pci;
>         struct device *dev = pci->dev;
> -       int ret;
>
> -       res->clks[0].id = "iface";
> -       res->clks[1].id = "axi_m";
> -       res->clks[2].id = "axi_s";
> -       res->clks[3].id = "axi_bridge";
> -       res->clks[4].id = "rchng";
> -
> -       ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> -       if (ret < 0)
> -               return ret;
> +       res->num_clks = devm_clk_bulk_get_all(dev, &res->clks);
> +       if (res->clks < 0)
> +               return res->num_clks;
>
>         res->rst = devm_reset_control_array_get_exclusive(dev);
>         if (IS_ERR(res->rst))
> @@ -1073,7 +1067,7 @@ static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
>  {
>         struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>
> -       clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
> +       clk_bulk_disable_unprepare(res->num_clks, res->clks);
>  }
>
>  static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
> @@ -1102,19 +1096,16 @@ static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
>
>         usleep_range(2000, 2500);
>
> -       return clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
> +       return clk_bulk_prepare_enable(res->num_clks, res->clks);
>  }
>
> -static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> +static int qcom_pcie_post_init(struct qcom_pcie *pcie)
>  {
>         struct dw_pcie *pci = pcie->pci;
>         u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>         u32 val;
>         int i;
>
> -       writel(SLV_ADDR_SPACE_SZ,
> -               pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> -
>         val = readl(pcie->parf + PARF_PHY_CTRL);
>         val &= ~PHY_TEST_PWR_DOWN;
>         writel(val, pcie->parf + PARF_PHY_CTRL);
> @@ -1151,6 +1142,26 @@ static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
>         return 0;
>  }
>
> +static int qcom_pcie_post_init_1_27_0(struct qcom_pcie *pcie)
> +{
> +       writel(SLV_ADDR_SPACE_SZ_1_27_0,
> +              pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> +
> +       qcom_pcie_post_init(pcie);
> +
> +       return 0;
> +}
> +
> +static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> +{
> +       writel(SLV_ADDR_SPACE_SZ,
> +              pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> +
> +       qcom_pcie_post_init(pcie);
> +
> +       return 0;
> +}

I'm not sure about moving the SLV_ADDR_SPACE_SIZE initialization from
init() to post_init(). Probably a better solution might be to have two
init() callbacks and to call the common function from both of them.

> +
>  static int qcom_pcie_link_up(struct dw_pcie *pci)
>  {
>         u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> @@ -1291,6 +1302,15 @@ static const struct qcom_pcie_ops ops_2_9_0 = {
>         .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>  };
>
> +/* Qcom IP rev.: 1.27.0  Synopsys IP rev.: 5.80a */
> +static const struct qcom_pcie_ops ops_1_27_0 = {
> +       .get_resources = qcom_pcie_get_resources_2_9_0,
> +       .init = qcom_pcie_init_2_9_0,
> +       .post_init = qcom_pcie_post_init_1_27_0,
> +       .deinit = qcom_pcie_deinit_2_9_0,
> +       .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> +};
> +
>  static const struct qcom_pcie_cfg cfg_1_0_0 = {
>         .ops = &ops_1_0_0,
>  };
> @@ -1323,6 +1343,10 @@ static const struct qcom_pcie_cfg cfg_2_9_0 = {
>         .ops = &ops_2_9_0,
>  };
>
> +static const struct qcom_pcie_cfg cfg_1_27_0 = {
> +       .ops = &ops_1_27_0,
> +};
> +
>  static const struct dw_pcie_ops dw_pcie_ops = {
>         .link_up = qcom_pcie_link_up,
>         .start_link = qcom_pcie_start_link,
> @@ -1607,6 +1631,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>         { .compatible = "qcom,pcie-ipq8064-v2", .data = &cfg_2_1_0 },
>         { .compatible = "qcom,pcie-ipq8074", .data = &cfg_2_3_3 },
>         { .compatible = "qcom,pcie-ipq8074-gen3", .data = &cfg_2_9_0 },
> +       { .compatible = "qcom,pcie-ipq9574", .data = &cfg_1_27_0 },
>         { .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
>         { .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
>         { .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 },
> --
> 2.17.1
>

Devi Priya May 2, 2023, 6:36 a.m. UTC | #2

On 4/22/2023 5:35 AM, Dmitry Baryshkov wrote:
> On Fri, 21 Apr 2023 at 15:51, Devi Priya <quic_devipriy@quicinc.com> wrote:
>>
>> The IPQ9574 platform has 4 Gen3 PCIe controllers: two single-lane
>> and two dual-lane based on SNPS core 5.70a
>> The Qcom IP rev is 1.27.0 and Synopsys IP rev is 5.80a
>> Added a new compatible 'qcom,pcie-ipq9574' and 'ops_1_27_0'
>> which reuses all the members of 'ops_2_9_0' except for the post_init
>> as the SLV_ADDR_SPACE_SIZE configuration differs between 2_9_0
>> and 1_27_0.
>> Also, modified get_resources of 'ops 2_9_0' to get the clocks
>> from the device tree and modelled the post init sequence as
>> a common function to avoid code redundancy.
>>
>> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
>> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
>> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
>> ---
>>   Changes in V3:
>>          - Rebased on top of linux-next/master
>>
>>   drivers/pci/controller/dwc/pcie-qcom.c | 61 ++++++++++++++++++--------
>>   1 file changed, 43 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 4ab30892f6ef..3682ecdead1f 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -107,6 +107,7 @@
>>
>>   /* PARF_SLV_ADDR_SPACE_SIZE register value */
>>   #define SLV_ADDR_SPACE_SZ                      0x10000000
>> +#define SLV_ADDR_SPACE_SZ_1_27_0               0x08000000
>>
>>   /* PARF_MHI_CLOCK_RESET_CTRL register fields */
>>   #define AHB_CLK_EN                             BIT(0)
>> @@ -202,10 +203,10 @@ struct qcom_pcie_resources_2_7_0 {
>>          struct reset_control *rst;
>>   };
>>
>> -#define QCOM_PCIE_2_9_0_MAX_CLOCKS             5
>>   struct qcom_pcie_resources_2_9_0 {
>> -       struct clk_bulk_data clks[QCOM_PCIE_2_9_0_MAX_CLOCKS];
>> +       struct clk_bulk_data *clks;
>>          struct reset_control *rst;
>> +       int num_clks;
>>   };
>>
>>   union qcom_pcie_resources {
>> @@ -1050,17 +1051,10 @@ static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
>>          struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>>          struct dw_pcie *pci = pcie->pci;
>>          struct device *dev = pci->dev;
>> -       int ret;
>>
>> -       res->clks[0].id = "iface";
>> -       res->clks[1].id = "axi_m";
>> -       res->clks[2].id = "axi_s";
>> -       res->clks[3].id = "axi_bridge";
>> -       res->clks[4].id = "rchng";
>> -
>> -       ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
>> -       if (ret < 0)
>> -               return ret;
>> +       res->num_clks = devm_clk_bulk_get_all(dev, &res->clks);
>> +       if (res->clks < 0)
>> +               return res->num_clks;
>>
>>          res->rst = devm_reset_control_array_get_exclusive(dev);
>>          if (IS_ERR(res->rst))
>> @@ -1073,7 +1067,7 @@ static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
>>   {
>>          struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>>
>> -       clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
>> +       clk_bulk_disable_unprepare(res->num_clks, res->clks);
>>   }
>>
>>   static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
>> @@ -1102,19 +1096,16 @@ static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
>>
>>          usleep_range(2000, 2500);
>>
>> -       return clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
>> +       return clk_bulk_prepare_enable(res->num_clks, res->clks);
>>   }
>>
>> -static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
>> +static int qcom_pcie_post_init(struct qcom_pcie *pcie)
>>   {
>>          struct dw_pcie *pci = pcie->pci;
>>          u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>>          u32 val;
>>          int i;
>>
>> -       writel(SLV_ADDR_SPACE_SZ,
>> -               pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
>> -
>>          val = readl(pcie->parf + PARF_PHY_CTRL);
>>          val &= ~PHY_TEST_PWR_DOWN;
>>          writel(val, pcie->parf + PARF_PHY_CTRL);
>> @@ -1151,6 +1142,26 @@ static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
>>          return 0;
>>   }
>>
>> +static int qcom_pcie_post_init_1_27_0(struct qcom_pcie *pcie)
>> +{
>> +       writel(SLV_ADDR_SPACE_SZ_1_27_0,
>> +              pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
>> +
>> +       qcom_pcie_post_init(pcie);
>> +
>> +       return 0;
>> +}
>> +
>> +static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
>> +{
>> +       writel(SLV_ADDR_SPACE_SZ,
>> +              pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
>> +
>> +       qcom_pcie_post_init(pcie);
>> +
>> +       return 0;
>> +}
> 
> I'm not sure about moving the SLV_ADDR_SPACE_SIZE initialization from
> init() to post_init(). Probably a better solution might be to have two
> init() callbacks and to call the common function from both of them.
> 
Hi Dmitry, Originally, the SLV_ADDR_SPACE_SIZE initialization was done
part of post_init() callback only and we haven't moved it from init() to
post_init().We have just added two post_init() callbacks to
handle the SLV_ADDR_SPACE_SIZE initialization accordingly for 1_27_0 and
2_9_0.

Thanks,
Devi Priya
>> +
>>   static int qcom_pcie_link_up(struct dw_pcie *pci)
>>   {
>>          u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>> @@ -1291,6 +1302,15 @@ static const struct qcom_pcie_ops ops_2_9_0 = {
>>          .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>>   };
>>
>> +/* Qcom IP rev.: 1.27.0  Synopsys IP rev.: 5.80a */
>> +static const struct qcom_pcie_ops ops_1_27_0 = {
>> +       .get_resources = qcom_pcie_get_resources_2_9_0,
>> +       .init = qcom_pcie_init_2_9_0,
>> +       .post_init = qcom_pcie_post_init_1_27_0,
>> +       .deinit = qcom_pcie_deinit_2_9_0,
>> +       .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>> +};
>> +
>>   static const struct qcom_pcie_cfg cfg_1_0_0 = {
>>          .ops = &ops_1_0_0,
>>   };
>> @@ -1323,6 +1343,10 @@ static const struct qcom_pcie_cfg cfg_2_9_0 = {
>>          .ops = &ops_2_9_0,
>>   };
>>
>> +static const struct qcom_pcie_cfg cfg_1_27_0 = {
>> +       .ops = &ops_1_27_0,
>> +};
>> +
>>   static const struct dw_pcie_ops dw_pcie_ops = {
>>          .link_up = qcom_pcie_link_up,
>>          .start_link = qcom_pcie_start_link,
>> @@ -1607,6 +1631,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>>          { .compatible = "qcom,pcie-ipq8064-v2", .data = &cfg_2_1_0 },
>>          { .compatible = "qcom,pcie-ipq8074", .data = &cfg_2_3_3 },
>>          { .compatible = "qcom,pcie-ipq8074-gen3", .data = &cfg_2_9_0 },
>> +       { .compatible = "qcom,pcie-ipq9574", .data = &cfg_1_27_0 },
>>          { .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
>>          { .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
>>          { .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 },
>> --
>> 2.17.1
>>
> 
>

Dmitry Baryshkov May 2, 2023, 8:34 a.m. UTC | #3

On Tue, 2 May 2023 at 09:36, Devi Priya <quic_devipriy@quicinc.com> wrote:
>
>
>
> On 4/22/2023 5:35 AM, Dmitry Baryshkov wrote:
> > On Fri, 21 Apr 2023 at 15:51, Devi Priya <quic_devipriy@quicinc.com> wrote:
> >>
> >> The IPQ9574 platform has 4 Gen3 PCIe controllers: two single-lane
> >> and two dual-lane based on SNPS core 5.70a
> >> The Qcom IP rev is 1.27.0 and Synopsys IP rev is 5.80a
> >> Added a new compatible 'qcom,pcie-ipq9574' and 'ops_1_27_0'
> >> which reuses all the members of 'ops_2_9_0' except for the post_init
> >> as the SLV_ADDR_SPACE_SIZE configuration differs between 2_9_0
> >> and 1_27_0.
> >> Also, modified get_resources of 'ops 2_9_0' to get the clocks
> >> from the device tree and modelled the post init sequence as
> >> a common function to avoid code redundancy.
> >>
> >> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
> >> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
> >> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> >> ---
> >>   Changes in V3:
> >>          - Rebased on top of linux-next/master
> >>
> >>   drivers/pci/controller/dwc/pcie-qcom.c | 61 ++++++++++++++++++--------
> >>   1 file changed, 43 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> >> index 4ab30892f6ef..3682ecdead1f 100644
> >> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> >> @@ -107,6 +107,7 @@
> >>
> >>   /* PARF_SLV_ADDR_SPACE_SIZE register value */
> >>   #define SLV_ADDR_SPACE_SZ                      0x10000000
> >> +#define SLV_ADDR_SPACE_SZ_1_27_0               0x08000000
> >>
> >>   /* PARF_MHI_CLOCK_RESET_CTRL register fields */
> >>   #define AHB_CLK_EN                             BIT(0)
> >> @@ -202,10 +203,10 @@ struct qcom_pcie_resources_2_7_0 {
> >>          struct reset_control *rst;
> >>   };
> >>
> >> -#define QCOM_PCIE_2_9_0_MAX_CLOCKS             5
> >>   struct qcom_pcie_resources_2_9_0 {
> >> -       struct clk_bulk_data clks[QCOM_PCIE_2_9_0_MAX_CLOCKS];
> >> +       struct clk_bulk_data *clks;
> >>          struct reset_control *rst;
> >> +       int num_clks;
> >>   };
> >>
> >>   union qcom_pcie_resources {
> >> @@ -1050,17 +1051,10 @@ static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
> >>          struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> >>          struct dw_pcie *pci = pcie->pci;
> >>          struct device *dev = pci->dev;
> >> -       int ret;
> >>
> >> -       res->clks[0].id = "iface";
> >> -       res->clks[1].id = "axi_m";
> >> -       res->clks[2].id = "axi_s";
> >> -       res->clks[3].id = "axi_bridge";
> >> -       res->clks[4].id = "rchng";
> >> -
> >> -       ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> >> -       if (ret < 0)
> >> -               return ret;
> >> +       res->num_clks = devm_clk_bulk_get_all(dev, &res->clks);
> >> +       if (res->clks < 0)
> >> +               return res->num_clks;
> >>
> >>          res->rst = devm_reset_control_array_get_exclusive(dev);
> >>          if (IS_ERR(res->rst))
> >> @@ -1073,7 +1067,7 @@ static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
> >>   {
> >>          struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> >>
> >> -       clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
> >> +       clk_bulk_disable_unprepare(res->num_clks, res->clks);
> >>   }
> >>
> >>   static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
> >> @@ -1102,19 +1096,16 @@ static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
> >>
> >>          usleep_range(2000, 2500);
> >>
> >> -       return clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
> >> +       return clk_bulk_prepare_enable(res->num_clks, res->clks);
> >>   }
> >>
> >> -static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> >> +static int qcom_pcie_post_init(struct qcom_pcie *pcie)
> >>   {
> >>          struct dw_pcie *pci = pcie->pci;
> >>          u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> >>          u32 val;
> >>          int i;
> >>
> >> -       writel(SLV_ADDR_SPACE_SZ,
> >> -               pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> >> -
> >>          val = readl(pcie->parf + PARF_PHY_CTRL);
> >>          val &= ~PHY_TEST_PWR_DOWN;
> >>          writel(val, pcie->parf + PARF_PHY_CTRL);
> >> @@ -1151,6 +1142,26 @@ static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> >>          return 0;
> >>   }
> >>
> >> +static int qcom_pcie_post_init_1_27_0(struct qcom_pcie *pcie)
> >> +{
> >> +       writel(SLV_ADDR_SPACE_SZ_1_27_0,
> >> +              pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> >> +
> >> +       qcom_pcie_post_init(pcie);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> >> +{
> >> +       writel(SLV_ADDR_SPACE_SZ,
> >> +              pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> >> +
> >> +       qcom_pcie_post_init(pcie);
> >> +
> >> +       return 0;
> >> +}
> >
> > I'm not sure about moving the SLV_ADDR_SPACE_SIZE initialization from
> > init() to post_init(). Probably a better solution might be to have two
> > init() callbacks and to call the common function from both of them.
> >
> Hi Dmitry, Originally, the SLV_ADDR_SPACE_SIZE initialization was done
> part of post_init() callback only and we haven't moved it from init() to
> post_init().We have just added two post_init() callbacks to
> handle the SLV_ADDR_SPACE_SIZE initialization accordingly for 1_27_0 and
> 2_9_0.

Ack, I see then.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Manivannan Sadhasivam May 8, 2023, 12:21 p.m. UTC | #4

On Fri, Apr 21, 2023 at 06:19:38PM +0530, Devi Priya wrote:
> The IPQ9574 platform has 4 Gen3 PCIe controllers: two single-lane
> and two dual-lane based on SNPS core 5.70a
> The Qcom IP rev is 1.27.0 and Synopsys IP rev is 5.80a
> Added a new compatible 'qcom,pcie-ipq9574' and 'ops_1_27_0'
> which reuses all the members of 'ops_2_9_0' except for the post_init
> as the SLV_ADDR_SPACE_SIZE configuration differs between 2_9_0
> and 1_27_0.
> Also, modified get_resources of 'ops 2_9_0' to get the clocks
> from the device tree and modelled the post init sequence as
> a common function to avoid code redundancy.
> 
> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>

One comment below. With that fixed,

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

- Mani

> ---
>  Changes in V3:
> 	- Rebased on top of linux-next/master
> 
>  drivers/pci/controller/dwc/pcie-qcom.c | 61 ++++++++++++++++++--------
>  1 file changed, 43 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 4ab30892f6ef..3682ecdead1f 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -107,6 +107,7 @@
>  
>  /* PARF_SLV_ADDR_SPACE_SIZE register value */
>  #define SLV_ADDR_SPACE_SZ			0x10000000
> +#define SLV_ADDR_SPACE_SZ_1_27_0		0x08000000
>  
>  /* PARF_MHI_CLOCK_RESET_CTRL register fields */
>  #define AHB_CLK_EN				BIT(0)
> @@ -202,10 +203,10 @@ struct qcom_pcie_resources_2_7_0 {
>  	struct reset_control *rst;
>  };
>  
> -#define QCOM_PCIE_2_9_0_MAX_CLOCKS		5
>  struct qcom_pcie_resources_2_9_0 {
> -	struct clk_bulk_data clks[QCOM_PCIE_2_9_0_MAX_CLOCKS];
> +	struct clk_bulk_data *clks;
>  	struct reset_control *rst;
> +	int num_clks;
>  };
>  
>  union qcom_pcie_resources {
> @@ -1050,17 +1051,10 @@ static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
>  	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>  	struct dw_pcie *pci = pcie->pci;
>  	struct device *dev = pci->dev;
> -	int ret;
>  
> -	res->clks[0].id = "iface";
> -	res->clks[1].id = "axi_m";
> -	res->clks[2].id = "axi_s";
> -	res->clks[3].id = "axi_bridge";
> -	res->clks[4].id = "rchng";
> -
> -	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> -	if (ret < 0)
> -		return ret;
> +	res->num_clks = devm_clk_bulk_get_all(dev, &res->clks);
> +	if (res->clks < 0)
> +		return res->num_clks;

Why not return proper error no?

>  
>  	res->rst = devm_reset_control_array_get_exclusive(dev);
>  	if (IS_ERR(res->rst))
> @@ -1073,7 +1067,7 @@ static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>  
> -	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
> +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
>  }
>  
>  static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
> @@ -1102,19 +1096,16 @@ static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
>  
>  	usleep_range(2000, 2500);
>  
> -	return clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
> +	return clk_bulk_prepare_enable(res->num_clks, res->clks);
>  }
>  
> -static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> +static int qcom_pcie_post_init(struct qcom_pcie *pcie)
>  {
>  	struct dw_pcie *pci = pcie->pci;
>  	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>  	u32 val;
>  	int i;
>  
> -	writel(SLV_ADDR_SPACE_SZ,
> -		pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> -
>  	val = readl(pcie->parf + PARF_PHY_CTRL);
>  	val &= ~PHY_TEST_PWR_DOWN;
>  	writel(val, pcie->parf + PARF_PHY_CTRL);
> @@ -1151,6 +1142,26 @@ static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
>  	return 0;
>  }
>  
> +static int qcom_pcie_post_init_1_27_0(struct qcom_pcie *pcie)
> +{
> +	writel(SLV_ADDR_SPACE_SZ_1_27_0,
> +	       pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> +
> +	qcom_pcie_post_init(pcie);
> +
> +	return 0;
> +}
> +
> +static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> +{
> +	writel(SLV_ADDR_SPACE_SZ,
> +	       pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> +
> +	qcom_pcie_post_init(pcie);
> +
> +	return 0;
> +}
> +
>  static int qcom_pcie_link_up(struct dw_pcie *pci)
>  {
>  	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> @@ -1291,6 +1302,15 @@ static const struct qcom_pcie_ops ops_2_9_0 = {
>  	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>  };
>  
> +/* Qcom IP rev.: 1.27.0  Synopsys IP rev.: 5.80a */
> +static const struct qcom_pcie_ops ops_1_27_0 = {
> +	.get_resources = qcom_pcie_get_resources_2_9_0,
> +	.init = qcom_pcie_init_2_9_0,
> +	.post_init = qcom_pcie_post_init_1_27_0,
> +	.deinit = qcom_pcie_deinit_2_9_0,
> +	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> +};
> +
>  static const struct qcom_pcie_cfg cfg_1_0_0 = {
>  	.ops = &ops_1_0_0,
>  };
> @@ -1323,6 +1343,10 @@ static const struct qcom_pcie_cfg cfg_2_9_0 = {
>  	.ops = &ops_2_9_0,
>  };
>  
> +static const struct qcom_pcie_cfg cfg_1_27_0 = {
> +	.ops = &ops_1_27_0,
> +};
> +
>  static const struct dw_pcie_ops dw_pcie_ops = {
>  	.link_up = qcom_pcie_link_up,
>  	.start_link = qcom_pcie_start_link,
> @@ -1607,6 +1631,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &cfg_2_1_0 },
>  	{ .compatible = "qcom,pcie-ipq8074", .data = &cfg_2_3_3 },
>  	{ .compatible = "qcom,pcie-ipq8074-gen3", .data = &cfg_2_9_0 },
> +	{ .compatible = "qcom,pcie-ipq9574", .data = &cfg_1_27_0 },
>  	{ .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
>  	{ .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
>  	{ .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 },
> -- 
> 2.17.1
>

Dmitry Baryshkov May 8, 2023, 12:46 p.m. UTC | #5

On Mon, 8 May 2023 at 15:21, Manivannan Sadhasivam <mani@kernel.org> wrote:
>
> On Fri, Apr 21, 2023 at 06:19:38PM +0530, Devi Priya wrote:
> > The IPQ9574 platform has 4 Gen3 PCIe controllers: two single-lane
> > and two dual-lane based on SNPS core 5.70a
> > The Qcom IP rev is 1.27.0 and Synopsys IP rev is 5.80a
> > Added a new compatible 'qcom,pcie-ipq9574' and 'ops_1_27_0'
> > which reuses all the members of 'ops_2_9_0' except for the post_init
> > as the SLV_ADDR_SPACE_SIZE configuration differs between 2_9_0
> > and 1_27_0.
> > Also, modified get_resources of 'ops 2_9_0' to get the clocks
> > from the device tree and modelled the post init sequence as
> > a common function to avoid code redundancy.
> >
> > Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
> > Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
> > Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
>
> One comment below. With that fixed,
>
> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
>
> - Mani
>
> > ---
> >  Changes in V3:
> >       - Rebased on top of linux-next/master
> >
> >  drivers/pci/controller/dwc/pcie-qcom.c | 61 ++++++++++++++++++--------
> >  1 file changed, 43 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 4ab30892f6ef..3682ecdead1f 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -107,6 +107,7 @@
> >
> >  /* PARF_SLV_ADDR_SPACE_SIZE register value */
> >  #define SLV_ADDR_SPACE_SZ                    0x10000000
> > +#define SLV_ADDR_SPACE_SZ_1_27_0             0x08000000
> >
> >  /* PARF_MHI_CLOCK_RESET_CTRL register fields */
> >  #define AHB_CLK_EN                           BIT(0)
> > @@ -202,10 +203,10 @@ struct qcom_pcie_resources_2_7_0 {
> >       struct reset_control *rst;
> >  };
> >
> > -#define QCOM_PCIE_2_9_0_MAX_CLOCKS           5
> >  struct qcom_pcie_resources_2_9_0 {
> > -     struct clk_bulk_data clks[QCOM_PCIE_2_9_0_MAX_CLOCKS];
> > +     struct clk_bulk_data *clks;
> >       struct reset_control *rst;
> > +     int num_clks;
> >  };
> >
> >  union qcom_pcie_resources {
> > @@ -1050,17 +1051,10 @@ static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
> >       struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> >       struct dw_pcie *pci = pcie->pci;
> >       struct device *dev = pci->dev;
> > -     int ret;
> >
> > -     res->clks[0].id = "iface";
> > -     res->clks[1].id = "axi_m";
> > -     res->clks[2].id = "axi_s";
> > -     res->clks[3].id = "axi_bridge";
> > -     res->clks[4].id = "rchng";
> > -
> > -     ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> > -     if (ret < 0)
> > -             return ret;
> > +     res->num_clks = devm_clk_bulk_get_all(dev, &res->clks);
> > +     if (res->clks < 0)
> > +             return res->num_clks;
>
> Why not return proper error no?

Instead the question should be, why not the proper condition: it tells
`if (res->clks < 0)', while it should be `if (res->num_clks < 0)'.

>
> >
> >       res->rst = devm_reset_control_array_get_exclusive(dev);
> >       if (IS_ERR(res->rst))
> > @@ -1073,7 +1067,7 @@ static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
> >  {
> >       struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> >
> > -     clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
> > +     clk_bulk_disable_unprepare(res->num_clks, res->clks);
> >  }
> >
> >  static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
> > @@ -1102,19 +1096,16 @@ static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
> >
> >       usleep_range(2000, 2500);
> >
> > -     return clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
> > +     return clk_bulk_prepare_enable(res->num_clks, res->clks);
> >  }
> >
> > -static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> > +static int qcom_pcie_post_init(struct qcom_pcie *pcie)
> >  {
> >       struct dw_pcie *pci = pcie->pci;
> >       u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> >       u32 val;
> >       int i;
> >
> > -     writel(SLV_ADDR_SPACE_SZ,
> > -             pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> > -
> >       val = readl(pcie->parf + PARF_PHY_CTRL);
> >       val &= ~PHY_TEST_PWR_DOWN;
> >       writel(val, pcie->parf + PARF_PHY_CTRL);
> > @@ -1151,6 +1142,26 @@ static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> >       return 0;
> >  }
> >
> > +static int qcom_pcie_post_init_1_27_0(struct qcom_pcie *pcie)
> > +{
> > +     writel(SLV_ADDR_SPACE_SZ_1_27_0,
> > +            pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> > +
> > +     qcom_pcie_post_init(pcie);
> > +
> > +     return 0;
> > +}
> > +
> > +static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> > +{
> > +     writel(SLV_ADDR_SPACE_SZ,
> > +            pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> > +
> > +     qcom_pcie_post_init(pcie);
> > +
> > +     return 0;
> > +}
> > +
> >  static int qcom_pcie_link_up(struct dw_pcie *pci)
> >  {
> >       u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > @@ -1291,6 +1302,15 @@ static const struct qcom_pcie_ops ops_2_9_0 = {
> >       .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> >  };
> >
> > +/* Qcom IP rev.: 1.27.0  Synopsys IP rev.: 5.80a */
> > +static const struct qcom_pcie_ops ops_1_27_0 = {
> > +     .get_resources = qcom_pcie_get_resources_2_9_0,
> > +     .init = qcom_pcie_init_2_9_0,
> > +     .post_init = qcom_pcie_post_init_1_27_0,
> > +     .deinit = qcom_pcie_deinit_2_9_0,
> > +     .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> > +};
> > +
> >  static const struct qcom_pcie_cfg cfg_1_0_0 = {
> >       .ops = &ops_1_0_0,
> >  };
> > @@ -1323,6 +1343,10 @@ static const struct qcom_pcie_cfg cfg_2_9_0 = {
> >       .ops = &ops_2_9_0,
> >  };
> >
> > +static const struct qcom_pcie_cfg cfg_1_27_0 = {
> > +     .ops = &ops_1_27_0,
> > +};
> > +
> >  static const struct dw_pcie_ops dw_pcie_ops = {
> >       .link_up = qcom_pcie_link_up,
> >       .start_link = qcom_pcie_start_link,
> > @@ -1607,6 +1631,7 @@ static const struct of_device_id qcom_pcie_match[] = {
> >       { .compatible = "qcom,pcie-ipq8064-v2", .data = &cfg_2_1_0 },
> >       { .compatible = "qcom,pcie-ipq8074", .data = &cfg_2_3_3 },
> >       { .compatible = "qcom,pcie-ipq8074-gen3", .data = &cfg_2_9_0 },
> > +     { .compatible = "qcom,pcie-ipq9574", .data = &cfg_1_27_0 },
> >       { .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
> >       { .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
> >       { .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 },
> > --
> > 2.17.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்

Manivannan Sadhasivam May 8, 2023, 3:37 p.m. UTC | #6

On Mon, May 08, 2023 at 03:46:53PM +0300, Dmitry Baryshkov wrote:
> On Mon, 8 May 2023 at 15:21, Manivannan Sadhasivam <mani@kernel.org> wrote:
> >
> > On Fri, Apr 21, 2023 at 06:19:38PM +0530, Devi Priya wrote:
> > > The IPQ9574 platform has 4 Gen3 PCIe controllers: two single-lane
> > > and two dual-lane based on SNPS core 5.70a
> > > The Qcom IP rev is 1.27.0 and Synopsys IP rev is 5.80a
> > > Added a new compatible 'qcom,pcie-ipq9574' and 'ops_1_27_0'
> > > which reuses all the members of 'ops_2_9_0' except for the post_init
> > > as the SLV_ADDR_SPACE_SIZE configuration differs between 2_9_0
> > > and 1_27_0.
> > > Also, modified get_resources of 'ops 2_9_0' to get the clocks
> > > from the device tree and modelled the post init sequence as
> > > a common function to avoid code redundancy.
> > >
> > > Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
> > > Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
> > > Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> >
> > One comment below. With that fixed,
> >
> > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> >
> > - Mani
> >
> > > ---
> > >  Changes in V3:
> > >       - Rebased on top of linux-next/master
> > >
> > >  drivers/pci/controller/dwc/pcie-qcom.c | 61 ++++++++++++++++++--------
> > >  1 file changed, 43 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 4ab30892f6ef..3682ecdead1f 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -107,6 +107,7 @@
> > >
> > >  /* PARF_SLV_ADDR_SPACE_SIZE register value */
> > >  #define SLV_ADDR_SPACE_SZ                    0x10000000
> > > +#define SLV_ADDR_SPACE_SZ_1_27_0             0x08000000
> > >
> > >  /* PARF_MHI_CLOCK_RESET_CTRL register fields */
> > >  #define AHB_CLK_EN                           BIT(0)
> > > @@ -202,10 +203,10 @@ struct qcom_pcie_resources_2_7_0 {
> > >       struct reset_control *rst;
> > >  };
> > >
> > > -#define QCOM_PCIE_2_9_0_MAX_CLOCKS           5
> > >  struct qcom_pcie_resources_2_9_0 {
> > > -     struct clk_bulk_data clks[QCOM_PCIE_2_9_0_MAX_CLOCKS];
> > > +     struct clk_bulk_data *clks;
> > >       struct reset_control *rst;
> > > +     int num_clks;
> > >  };
> > >
> > >  union qcom_pcie_resources {
> > > @@ -1050,17 +1051,10 @@ static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
> > >       struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> > >       struct dw_pcie *pci = pcie->pci;
> > >       struct device *dev = pci->dev;
> > > -     int ret;
> > >
> > > -     res->clks[0].id = "iface";
> > > -     res->clks[1].id = "axi_m";
> > > -     res->clks[2].id = "axi_s";
> > > -     res->clks[3].id = "axi_bridge";
> > > -     res->clks[4].id = "rchng";
> > > -
> > > -     ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> > > -     if (ret < 0)
> > > -             return ret;
> > > +     res->num_clks = devm_clk_bulk_get_all(dev, &res->clks);
> > > +     if (res->clks < 0)
> > > +             return res->num_clks;
> >
> > Why not return proper error no?
> 
> Instead the question should be, why not the proper condition: it tells
> `if (res->clks < 0)', while it should be `if (res->num_clks < 0)'.
> 

Heh. I completely overlooked that part. Yes, the if condition itself should be
fixed.

- Mani

> >
> > >
> > >       res->rst = devm_reset_control_array_get_exclusive(dev);
> > >       if (IS_ERR(res->rst))
> > > @@ -1073,7 +1067,7 @@ static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
> > >  {
> > >       struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> > >
> > > -     clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
> > > +     clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > >  }
> > >
> > >  static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
> > > @@ -1102,19 +1096,16 @@ static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
> > >
> > >       usleep_range(2000, 2500);
> > >
> > > -     return clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
> > > +     return clk_bulk_prepare_enable(res->num_clks, res->clks);
> > >  }
> > >
> > > -static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> > > +static int qcom_pcie_post_init(struct qcom_pcie *pcie)
> > >  {
> > >       struct dw_pcie *pci = pcie->pci;
> > >       u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > >       u32 val;
> > >       int i;
> > >
> > > -     writel(SLV_ADDR_SPACE_SZ,
> > > -             pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> > > -
> > >       val = readl(pcie->parf + PARF_PHY_CTRL);
> > >       val &= ~PHY_TEST_PWR_DOWN;
> > >       writel(val, pcie->parf + PARF_PHY_CTRL);
> > > @@ -1151,6 +1142,26 @@ static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> > >       return 0;
> > >  }
> > >
> > > +static int qcom_pcie_post_init_1_27_0(struct qcom_pcie *pcie)
> > > +{
> > > +     writel(SLV_ADDR_SPACE_SZ_1_27_0,
> > > +            pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> > > +
> > > +     qcom_pcie_post_init(pcie);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> > > +{
> > > +     writel(SLV_ADDR_SPACE_SZ,
> > > +            pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> > > +
> > > +     qcom_pcie_post_init(pcie);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  static int qcom_pcie_link_up(struct dw_pcie *pci)
> > >  {
> > >       u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > > @@ -1291,6 +1302,15 @@ static const struct qcom_pcie_ops ops_2_9_0 = {
> > >       .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> > >  };
> > >
> > > +/* Qcom IP rev.: 1.27.0  Synopsys IP rev.: 5.80a */
> > > +static const struct qcom_pcie_ops ops_1_27_0 = {
> > > +     .get_resources = qcom_pcie_get_resources_2_9_0,
> > > +     .init = qcom_pcie_init_2_9_0,
> > > +     .post_init = qcom_pcie_post_init_1_27_0,
> > > +     .deinit = qcom_pcie_deinit_2_9_0,
> > > +     .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> > > +};
> > > +
> > >  static const struct qcom_pcie_cfg cfg_1_0_0 = {
> > >       .ops = &ops_1_0_0,
> > >  };
> > > @@ -1323,6 +1343,10 @@ static const struct qcom_pcie_cfg cfg_2_9_0 = {
> > >       .ops = &ops_2_9_0,
> > >  };
> > >
> > > +static const struct qcom_pcie_cfg cfg_1_27_0 = {
> > > +     .ops = &ops_1_27_0,
> > > +};
> > > +
> > >  static const struct dw_pcie_ops dw_pcie_ops = {
> > >       .link_up = qcom_pcie_link_up,
> > >       .start_link = qcom_pcie_start_link,
> > > @@ -1607,6 +1631,7 @@ static const struct of_device_id qcom_pcie_match[] = {
> > >       { .compatible = "qcom,pcie-ipq8064-v2", .data = &cfg_2_1_0 },
> > >       { .compatible = "qcom,pcie-ipq8074", .data = &cfg_2_3_3 },
> > >       { .compatible = "qcom,pcie-ipq8074-gen3", .data = &cfg_2_9_0 },
> > > +     { .compatible = "qcom,pcie-ipq9574", .data = &cfg_1_27_0 },
> > >       { .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
> > >       { .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
> > >       { .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 },
> > > --
> > > 2.17.1
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்
> 
> 
> 
> -- 
> With best wishes
> Dmitry

Devi Priya May 9, 2023, 8:49 a.m. UTC | #7

On 5/8/2023 9:07 PM, Manivannan Sadhasivam wrote:
> On Mon, May 08, 2023 at 03:46:53PM +0300, Dmitry Baryshkov wrote:
>> On Mon, 8 May 2023 at 15:21, Manivannan Sadhasivam <mani@kernel.org> wrote:
>>>
>>> On Fri, Apr 21, 2023 at 06:19:38PM +0530, Devi Priya wrote:
>>>> The IPQ9574 platform has 4 Gen3 PCIe controllers: two single-lane
>>>> and two dual-lane based on SNPS core 5.70a
>>>> The Qcom IP rev is 1.27.0 and Synopsys IP rev is 5.80a
>>>> Added a new compatible 'qcom,pcie-ipq9574' and 'ops_1_27_0'
>>>> which reuses all the members of 'ops_2_9_0' except for the post_init
>>>> as the SLV_ADDR_SPACE_SIZE configuration differs between 2_9_0
>>>> and 1_27_0.
>>>> Also, modified get_resources of 'ops 2_9_0' to get the clocks
>>>> from the device tree and modelled the post init sequence as
>>>> a common function to avoid code redundancy.
>>>>
>>>> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
>>>> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
>>>> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
>>>
>>> One comment below. With that fixed,
>>>
>>> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
>>>
>>> - Mani
>>>
>>>> ---
>>>>   Changes in V3:
>>>>        - Rebased on top of linux-next/master
>>>>
>>>>   drivers/pci/controller/dwc/pcie-qcom.c | 61 ++++++++++++++++++--------
>>>>   1 file changed, 43 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>>> index 4ab30892f6ef..3682ecdead1f 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>>> @@ -107,6 +107,7 @@
>>>>
>>>>   /* PARF_SLV_ADDR_SPACE_SIZE register value */
>>>>   #define SLV_ADDR_SPACE_SZ                    0x10000000
>>>> +#define SLV_ADDR_SPACE_SZ_1_27_0             0x08000000
>>>>
>>>>   /* PARF_MHI_CLOCK_RESET_CTRL register fields */
>>>>   #define AHB_CLK_EN                           BIT(0)
>>>> @@ -202,10 +203,10 @@ struct qcom_pcie_resources_2_7_0 {
>>>>        struct reset_control *rst;
>>>>   };
>>>>
>>>> -#define QCOM_PCIE_2_9_0_MAX_CLOCKS           5
>>>>   struct qcom_pcie_resources_2_9_0 {
>>>> -     struct clk_bulk_data clks[QCOM_PCIE_2_9_0_MAX_CLOCKS];
>>>> +     struct clk_bulk_data *clks;
>>>>        struct reset_control *rst;
>>>> +     int num_clks;
>>>>   };
>>>>
>>>>   union qcom_pcie_resources {
>>>> @@ -1050,17 +1051,10 @@ static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
>>>>        struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>>>>        struct dw_pcie *pci = pcie->pci;
>>>>        struct device *dev = pci->dev;
>>>> -     int ret;
>>>>
>>>> -     res->clks[0].id = "iface";
>>>> -     res->clks[1].id = "axi_m";
>>>> -     res->clks[2].id = "axi_s";
>>>> -     res->clks[3].id = "axi_bridge";
>>>> -     res->clks[4].id = "rchng";
>>>> -
>>>> -     ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
>>>> -     if (ret < 0)
>>>> -             return ret;
>>>> +     res->num_clks = devm_clk_bulk_get_all(dev, &res->clks);
>>>> +     if (res->clks < 0)
>>>> +             return res->num_clks;
>>>
>>> Why not return proper error no?
>>
>> Instead the question should be, why not the proper condition: it tells
>> `if (res->clks < 0)', while it should be `if (res->num_clks < 0)'.
>>
> 
> Heh. I completely overlooked that part. Yes, the if condition itself should be
> fixed.
> 
> - Mani

Sure, will fix it up!

Regards,
Devi Priya
> 
>>>
>>>>
>>>>        res->rst = devm_reset_control_array_get_exclusive(dev);
>>>>        if (IS_ERR(res->rst))
>>>> @@ -1073,7 +1067,7 @@ static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
>>>>   {
>>>>        struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>>>>
>>>> -     clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
>>>> +     clk_bulk_disable_unprepare(res->num_clks, res->clks);
>>>>   }
>>>>
>>>>   static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
>>>> @@ -1102,19 +1096,16 @@ static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
>>>>
>>>>        usleep_range(2000, 2500);
>>>>
>>>> -     return clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
>>>> +     return clk_bulk_prepare_enable(res->num_clks, res->clks);
>>>>   }
>>>>
>>>> -static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
>>>> +static int qcom_pcie_post_init(struct qcom_pcie *pcie)
>>>>   {
>>>>        struct dw_pcie *pci = pcie->pci;
>>>>        u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>>>>        u32 val;
>>>>        int i;
>>>>
>>>> -     writel(SLV_ADDR_SPACE_SZ,
>>>> -             pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
>>>> -
>>>>        val = readl(pcie->parf + PARF_PHY_CTRL);
>>>>        val &= ~PHY_TEST_PWR_DOWN;
>>>>        writel(val, pcie->parf + PARF_PHY_CTRL);
>>>> @@ -1151,6 +1142,26 @@ static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
>>>>        return 0;
>>>>   }
>>>>
>>>> +static int qcom_pcie_post_init_1_27_0(struct qcom_pcie *pcie)
>>>> +{
>>>> +     writel(SLV_ADDR_SPACE_SZ_1_27_0,
>>>> +            pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
>>>> +
>>>> +     qcom_pcie_post_init(pcie);
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
>>>> +{
>>>> +     writel(SLV_ADDR_SPACE_SZ,
>>>> +            pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
>>>> +
>>>> +     qcom_pcie_post_init(pcie);
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>>   static int qcom_pcie_link_up(struct dw_pcie *pci)
>>>>   {
>>>>        u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>>>> @@ -1291,6 +1302,15 @@ static const struct qcom_pcie_ops ops_2_9_0 = {
>>>>        .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>>>>   };
>>>>
>>>> +/* Qcom IP rev.: 1.27.0  Synopsys IP rev.: 5.80a */
>>>> +static const struct qcom_pcie_ops ops_1_27_0 = {
>>>> +     .get_resources = qcom_pcie_get_resources_2_9_0,
>>>> +     .init = qcom_pcie_init_2_9_0,
>>>> +     .post_init = qcom_pcie_post_init_1_27_0,
>>>> +     .deinit = qcom_pcie_deinit_2_9_0,
>>>> +     .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>>>> +};
>>>> +
>>>>   static const struct qcom_pcie_cfg cfg_1_0_0 = {
>>>>        .ops = &ops_1_0_0,
>>>>   };
>>>> @@ -1323,6 +1343,10 @@ static const struct qcom_pcie_cfg cfg_2_9_0 = {
>>>>        .ops = &ops_2_9_0,
>>>>   };
>>>>
>>>> +static const struct qcom_pcie_cfg cfg_1_27_0 = {
>>>> +     .ops = &ops_1_27_0,
>>>> +};
>>>> +
>>>>   static const struct dw_pcie_ops dw_pcie_ops = {
>>>>        .link_up = qcom_pcie_link_up,
>>>>        .start_link = qcom_pcie_start_link,
>>>> @@ -1607,6 +1631,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>>>>        { .compatible = "qcom,pcie-ipq8064-v2", .data = &cfg_2_1_0 },
>>>>        { .compatible = "qcom,pcie-ipq8074", .data = &cfg_2_3_3 },
>>>>        { .compatible = "qcom,pcie-ipq8074-gen3", .data = &cfg_2_9_0 },
>>>> +     { .compatible = "qcom,pcie-ipq9574", .data = &cfg_1_27_0 },
>>>>        { .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
>>>>        { .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
>>>>        { .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 },
>>>> --
>>>> 2.17.1
>>>>
>>>
>>> --
>>> மணிவண்ணன் சதாசிவம்
>>
>>
>>
>> -- 
>> With best wishes
>> Dmitry
>

diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 4ab30892f6ef..3682ecdead1f 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -107,6 +107,7 @@ 
 
 /* PARF_SLV_ADDR_SPACE_SIZE register value */
 #define SLV_ADDR_SPACE_SZ			0x10000000
+#define SLV_ADDR_SPACE_SZ_1_27_0		0x08000000
 
 /* PARF_MHI_CLOCK_RESET_CTRL register fields */
 #define AHB_CLK_EN				BIT(0)
@@ -202,10 +203,10 @@  struct qcom_pcie_resources_2_7_0 {
 	struct reset_control *rst;
 };
 
-#define QCOM_PCIE_2_9_0_MAX_CLOCKS		5
 struct qcom_pcie_resources_2_9_0 {
-	struct clk_bulk_data clks[QCOM_PCIE_2_9_0_MAX_CLOCKS];
+	struct clk_bulk_data *clks;
 	struct reset_control *rst;
+	int num_clks;
 };
 
 union qcom_pcie_resources {
@@ -1050,17 +1051,10 @@  static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
 	struct dw_pcie *pci = pcie->pci;
 	struct device *dev = pci->dev;
-	int ret;
 
-	res->clks[0].id = "iface";
-	res->clks[1].id = "axi_m";
-	res->clks[2].id = "axi_s";
-	res->clks[3].id = "axi_bridge";
-	res->clks[4].id = "rchng";
-
-	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
-	if (ret < 0)
-		return ret;
+	res->num_clks = devm_clk_bulk_get_all(dev, &res->clks);
+	if (res->clks < 0)
+		return res->num_clks;
 
 	res->rst = devm_reset_control_array_get_exclusive(dev);
 	if (IS_ERR(res->rst))
@@ -1073,7 +1067,7 @@  static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
 
-	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
+	clk_bulk_disable_unprepare(res->num_clks, res->clks);
 }
 
 static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
@@ -1102,19 +1096,16 @@  static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
 
 	usleep_range(2000, 2500);
 
-	return clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
+	return clk_bulk_prepare_enable(res->num_clks, res->clks);
 }
 
-static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
+static int qcom_pcie_post_init(struct qcom_pcie *pcie)
 {
 	struct dw_pcie *pci = pcie->pci;
 	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
 	u32 val;
 	int i;
 
-	writel(SLV_ADDR_SPACE_SZ,
-		pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
-
 	val = readl(pcie->parf + PARF_PHY_CTRL);
 	val &= ~PHY_TEST_PWR_DOWN;
 	writel(val, pcie->parf + PARF_PHY_CTRL);
@@ -1151,6 +1142,26 @@  static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
 	return 0;
 }
 
+static int qcom_pcie_post_init_1_27_0(struct qcom_pcie *pcie)
+{
+	writel(SLV_ADDR_SPACE_SZ_1_27_0,
+	       pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
+
+	qcom_pcie_post_init(pcie);
+
+	return 0;
+}
+
+static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
+{
+	writel(SLV_ADDR_SPACE_SZ,
+	       pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
+
+	qcom_pcie_post_init(pcie);
+
+	return 0;
+}
+
 static int qcom_pcie_link_up(struct dw_pcie *pci)
 {
 	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
@@ -1291,6 +1302,15 @@  static const struct qcom_pcie_ops ops_2_9_0 = {
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 };
 
+/* Qcom IP rev.: 1.27.0  Synopsys IP rev.: 5.80a */
+static const struct qcom_pcie_ops ops_1_27_0 = {
+	.get_resources = qcom_pcie_get_resources_2_9_0,
+	.init = qcom_pcie_init_2_9_0,
+	.post_init = qcom_pcie_post_init_1_27_0,
+	.deinit = qcom_pcie_deinit_2_9_0,
+	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
+};
+
 static const struct qcom_pcie_cfg cfg_1_0_0 = {
 	.ops = &ops_1_0_0,
 };
@@ -1323,6 +1343,10 @@  static const struct qcom_pcie_cfg cfg_2_9_0 = {
 	.ops = &ops_2_9_0,
 };
 
+static const struct qcom_pcie_cfg cfg_1_27_0 = {
+	.ops = &ops_1_27_0,
+};
+
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = qcom_pcie_link_up,
 	.start_link = qcom_pcie_start_link,
@@ -1607,6 +1631,7 @@  static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &cfg_2_1_0 },
 	{ .compatible = "qcom,pcie-ipq8074", .data = &cfg_2_3_3 },
 	{ .compatible = "qcom,pcie-ipq8074-gen3", .data = &cfg_2_9_0 },
+	{ .compatible = "qcom,pcie-ipq9574", .data = &cfg_1_27_0 },
 	{ .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
 	{ .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
 	{ .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 },