| Message ID | 20240801054803.3015572-5-quic_srichara@quicinc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | Add PCIe support for IPQ9574 | expand |
On Thu, Aug 01, 2024 at 11:18:03AM +0530, Sricharan R wrote: > From: devi priya <quic_devipriy@quicinc.com> > > The IPQ9574 platform has four Gen3 PCIe controllers: > two single-lane and two dual-lane based on SNPS core 5.70a. > > QCOM IP rev is 1.27.0 and Synopsys IP rev is 5.80a. > Reuse all the members of 'ops_2_9_0'. Wow, this is confusing. "Based on SNPS core 5.70a", but "Synopsys IP rev is 5.80a." Are those supposed to match? Or is it 5.70a of one thing but 5.80a of a different thing? And where does ops_2_9_0 come in? The code comment says: /* Qcom IP rev.: 2.9.0 Synopsys IP rev.: 5.00a */ static const struct qcom_pcie_ops ops_2_9_0 = { which doesn't match 1.27.0 or 5.70a or 5.80a. In fact there's nothing in the file that matches 1.*27.*0 Honestly, I don't really care if you have all the versions here in the commit log. But if the versions *are* here, can we make them make sense? > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> > 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> > Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> > --- > [V7] Rebased on top of [1] to avoid DBI/ATU mirroring. With that dropped > the need for separate ops. > [1] https://lore.kernel.org/linux-arm-msm/a01404d2-2f4d-4fb8-af9d-3db66d39acf7@quicinc.com/ > > drivers/pci/controller/dwc/pcie-qcom.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 6976efb8e2f0..e9371f945900 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -1752,6 +1752,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_2_9_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_sc8280xp }, > -- > 2.34.1 >
On Tue, Nov 05, 2024 at 07:40:24PM -0600, Bjorn Helgaas wrote: > On Thu, Aug 01, 2024 at 11:18:03AM +0530, Sricharan R wrote: > > From: devi priya <quic_devipriy@quicinc.com> > > > > The IPQ9574 platform has four Gen3 PCIe controllers: > > two single-lane and two dual-lane based on SNPS core 5.70a. > > > > QCOM IP rev is 1.27.0 and Synopsys IP rev is 5.80a. > > Reuse all the members of 'ops_2_9_0'. > > Wow, this is confusing. > > "Based on SNPS core 5.70a", but "Synopsys IP rev is 5.80a." > Are those supposed to match? Or is it 5.70a of one thing but 5.80a of > a different thing? > Hmm, I'm not sure why 5.70a is mentioned here. It seems irrelevant (even if it is the base). > And where does ops_2_9_0 come in? The code comment says: > > /* Qcom IP rev.: 2.9.0 Synopsys IP rev.: 5.00a */ > static const struct qcom_pcie_ops ops_2_9_0 = { > > which doesn't match 1.27.0 or 5.70a or 5.80a. In fact there's nothing > in the file that matches 1.*27.*0 > > Honestly, I don't really care if you have all the versions here in the > commit log. But if the versions *are* here, can we make them make > sense? > We name the 'ops' structure based on Qcom IP revision. And we reuse it across the SoCs which are compatible. That's why ops_2_9_0 is used for this SoC which has Qcom IP rev 1.27.0. - Mani > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> > > 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> > > Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> > > --- > > [V7] Rebased on top of [1] to avoid DBI/ATU mirroring. With that dropped > > the need for separate ops. > > [1] https://lore.kernel.org/linux-arm-msm/a01404d2-2f4d-4fb8-af9d-3db66d39acf7@quicinc.com/ > > > > drivers/pci/controller/dwc/pcie-qcom.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > > index 6976efb8e2f0..e9371f945900 100644 > > --- a/drivers/pci/controller/dwc/pcie-qcom.c > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > > @@ -1752,6 +1752,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_2_9_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_sc8280xp }, > > -- > > 2.34.1 > >
On Wed, Nov 06, 2024 at 02:56:15PM +0000, Manivannan Sadhasivam wrote: > On Tue, Nov 05, 2024 at 07:40:24PM -0600, Bjorn Helgaas wrote: > > On Thu, Aug 01, 2024 at 11:18:03AM +0530, Sricharan R wrote: > > > From: devi priya <quic_devipriy@quicinc.com> > > > > > > The IPQ9574 platform has four Gen3 PCIe controllers: > > > two single-lane and two dual-lane based on SNPS core 5.70a. > > > > > > QCOM IP rev is 1.27.0 and Synopsys IP rev is 5.80a. > > > Reuse all the members of 'ops_2_9_0'. > > > > Wow, this is confusing. > > > > "Based on SNPS core 5.70a", but "Synopsys IP rev is 5.80a." > > Are those supposed to match? Or is it 5.70a of one thing but 5.80a of > > a different thing? > > Hmm, I'm not sure why 5.70a is mentioned here. It seems irrelevant > (even if it is the base). > > > And where does ops_2_9_0 come in? The code comment says: > > > > /* Qcom IP rev.: 2.9.0 Synopsys IP rev.: 5.00a */ > > static const struct qcom_pcie_ops ops_2_9_0 = { > > > > which doesn't match 1.27.0 or 5.70a or 5.80a. In fact there's nothing > > in the file that matches 1.*27.*0 > > > > Honestly, I don't really care if you have all the versions here in the > > commit log. But if the versions *are* here, can we make them make > > sense? > > We name the 'ops' structure based on Qcom IP revision. And we reuse > it across the SoCs which are compatible. That's why ops_2_9_0 is > used for this SoC which has Qcom IP rev 1.27.0. Got it. So a family of compatible Qcom IP starts with 2.9.0 and newer members are 1.27.0 etc. With no hint in the source about what the members of the family are. Perfect sEnSe. Bjorn
On Wed, Nov 06, 2024 at 10:53:12AM -0600, Bjorn Helgaas wrote: > On Wed, Nov 06, 2024 at 02:56:15PM +0000, Manivannan Sadhasivam wrote: > > On Tue, Nov 05, 2024 at 07:40:24PM -0600, Bjorn Helgaas wrote: > > > On Thu, Aug 01, 2024 at 11:18:03AM +0530, Sricharan R wrote: > > > > From: devi priya <quic_devipriy@quicinc.com> > > > > > > > > The IPQ9574 platform has four Gen3 PCIe controllers: > > > > two single-lane and two dual-lane based on SNPS core 5.70a. > > > > > > > > QCOM IP rev is 1.27.0 and Synopsys IP rev is 5.80a. > > > > Reuse all the members of 'ops_2_9_0'. > > > > > > Wow, this is confusing. > > > > > > "Based on SNPS core 5.70a", but "Synopsys IP rev is 5.80a." > > > Are those supposed to match? Or is it 5.70a of one thing but 5.80a of > > > a different thing? > > > > Hmm, I'm not sure why 5.70a is mentioned here. It seems irrelevant > > (even if it is the base). > > > > > And where does ops_2_9_0 come in? The code comment says: > > > > > > /* Qcom IP rev.: 2.9.0 Synopsys IP rev.: 5.00a */ > > > static const struct qcom_pcie_ops ops_2_9_0 = { > > > > > > which doesn't match 1.27.0 or 5.70a or 5.80a. In fact there's nothing > > > in the file that matches 1.*27.*0 > > > > > > Honestly, I don't really care if you have all the versions here in the > > > commit log. But if the versions *are* here, can we make them make > > > sense? > > > > We name the 'ops' structure based on Qcom IP revision. And we reuse > > it across the SoCs which are compatible. That's why ops_2_9_0 is > > used for this SoC which has Qcom IP rev 1.27.0. > > Got it. So a family of compatible Qcom IP starts with 2.9.0 and newer > members are 1.27.0 etc. With no hint in the source about what the > members of the family are. Perfect sEnSe. > You can get that information by mapping the ops structure with compatibles. I feel less incentive in documenting the SoC names deriving the same IP. And the reason why 2.9.0 ops is used by 1.27 is because, we ended up adding support for 2.9.0 IP first and the lower version followed later. - Mani
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index 6976efb8e2f0..e9371f945900 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -1752,6 +1752,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_2_9_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_sc8280xp },