| Message ID | 20240724022719.2868490-2-quic_pyarlaga@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: qcom: Avoid DBI and ATU register space mirroring | expand |
On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote: Subject could be modified as below: PCI: dwc: Cache DBI and iATU physical addresses in 'struct dw_pcie_ops' > Both DBI and ATU physical base addresses are needed by pcie_qcom.c > driver to program the location of DBI and ATU blocks in Qualcomm > PCIe Controller specific PARF hardware block. > > Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com> Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > Reviewed-by: Mayank Rana <quic_mrana@quicinc.com> > --- > drivers/pci/controller/dwc/pcie-designware.c | 2 ++ > drivers/pci/controller/dwc/pcie-designware.h | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 1b5aba1f0c92..bc3a5d6b0177 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) > pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res); > if (IS_ERR(pci->dbi_base)) > return PTR_ERR(pci->dbi_base); > + pci->dbi_phys_addr = res->start; > } > > /* DBI2 is mainly useful for the endpoint controller */ > @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) > pci->atu_base = devm_ioremap_resource(pci->dev, res); > if (IS_ERR(pci->atu_base)) > return PTR_ERR(pci->atu_base); > + pci->atu_phys_addr = res->start; > } else { > pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET; > } > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 53c4c8f399c8..efc72989330c 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -407,8 +407,10 @@ struct dw_pcie_ops { > struct dw_pcie { > struct device *dev; > void __iomem *dbi_base; > + phys_addr_t dbi_phys_addr; > void __iomem *dbi_base2; > void __iomem *atu_base; > + phys_addr_t atu_phys_addr; > size_t atu_size; > u32 num_ib_windows; > u32 num_ob_windows; > -- > 2.25.1 >
Hi Manivannan, Thanks for the review comments. On 7/24/2024 6:53 AM, Manivannan Sadhasivam wrote: > On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote: > > Subject could be modified as below: > > PCI: dwc: Cache DBI and iATU physical addresses in 'struct dw_pcie_ops' > ACK. I will update it in the next patch. >> Both DBI and ATU physical base addresses are needed by pcie_qcom.c >> driver to program the location of DBI and ATU blocks in Qualcomm >> PCIe Controller specific PARF hardware block. >> >> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com> > > Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > - Mani > ACK. I will add it in the next patch. Thanks, Prudhvi >> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com> >> --- >> drivers/pci/controller/dwc/pcie-designware.c | 2 ++ >> drivers/pci/controller/dwc/pcie-designware.h | 2 ++ >> 2 files changed, 4 insertions(+) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c >> index 1b5aba1f0c92..bc3a5d6b0177 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.c >> +++ b/drivers/pci/controller/dwc/pcie-designware.c >> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) >> pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res); >> if (IS_ERR(pci->dbi_base)) >> return PTR_ERR(pci->dbi_base); >> + pci->dbi_phys_addr = res->start; >> } >> >> /* DBI2 is mainly useful for the endpoint controller */ >> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) >> pci->atu_base = devm_ioremap_resource(pci->dev, res); >> if (IS_ERR(pci->atu_base)) >> return PTR_ERR(pci->atu_base); >> + pci->atu_phys_addr = res->start; >> } else { >> pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET; >> } >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h >> index 53c4c8f399c8..efc72989330c 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.h >> +++ b/drivers/pci/controller/dwc/pcie-designware.h >> @@ -407,8 +407,10 @@ struct dw_pcie_ops { >> struct dw_pcie { >> struct device *dev; >> void __iomem *dbi_base; >> + phys_addr_t dbi_phys_addr; >> void __iomem *dbi_base2; >> void __iomem *atu_base; >> + phys_addr_t atu_phys_addr; >> size_t atu_size; >> u32 num_ib_windows; >> u32 num_ob_windows; >> -- >> 2.25.1 >> >
On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote: > Both DBI and ATU physical base addresses are needed by pcie_qcom.c > driver to program the location of DBI and ATU blocks in Qualcomm > PCIe Controller specific PARF hardware block. > > Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com> > Reviewed-by: Mayank Rana <quic_mrana@quicinc.com> > --- > drivers/pci/controller/dwc/pcie-designware.c | 2 ++ > drivers/pci/controller/dwc/pcie-designware.h | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 1b5aba1f0c92..bc3a5d6b0177 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) > pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res); > if (IS_ERR(pci->dbi_base)) > return PTR_ERR(pci->dbi_base); > + pci->dbi_phys_addr = res->start; > } > > /* DBI2 is mainly useful for the endpoint controller */ > @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) > pci->atu_base = devm_ioremap_resource(pci->dev, res); > if (IS_ERR(pci->atu_base)) > return PTR_ERR(pci->atu_base); > + pci->atu_phys_addr = res->start; > } else { > pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET; > } > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 53c4c8f399c8..efc72989330c 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -407,8 +407,10 @@ struct dw_pcie_ops { > struct dw_pcie { > struct device *dev; > void __iomem *dbi_base; > + phys_addr_t dbi_phys_addr; > void __iomem *dbi_base2; > void __iomem *atu_base; > + phys_addr_t atu_phys_addr; > size_t atu_size; > u32 num_ib_windows; > u32 num_ob_windows; This patch is pretty trivial and it doesn't show anything to justify the need to keep these addresses. I think this should be squashed with the next patch that actually *uses* them.
Hi Bjorn, Thanks for the review comments. On 7/24/2024 11:39 AM, Bjorn Helgaas wrote: > On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote: >> Both DBI and ATU physical base addresses are needed by pcie_qcom.c >> driver to program the location of DBI and ATU blocks in Qualcomm >> PCIe Controller specific PARF hardware block. >> >> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com> >> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com> >> --- >> drivers/pci/controller/dwc/pcie-designware.c | 2 ++ >> drivers/pci/controller/dwc/pcie-designware.h | 2 ++ >> 2 files changed, 4 insertions(+) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c >> index 1b5aba1f0c92..bc3a5d6b0177 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.c >> +++ b/drivers/pci/controller/dwc/pcie-designware.c >> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) >> pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res); >> if (IS_ERR(pci->dbi_base)) >> return PTR_ERR(pci->dbi_base); >> + pci->dbi_phys_addr = res->start; >> } >> >> /* DBI2 is mainly useful for the endpoint controller */ >> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) >> pci->atu_base = devm_ioremap_resource(pci->dev, res); >> if (IS_ERR(pci->atu_base)) >> return PTR_ERR(pci->atu_base); >> + pci->atu_phys_addr = res->start; >> } else { >> pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET; >> } >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h >> index 53c4c8f399c8..efc72989330c 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.h >> +++ b/drivers/pci/controller/dwc/pcie-designware.h >> @@ -407,8 +407,10 @@ struct dw_pcie_ops { >> struct dw_pcie { >> struct device *dev; >> void __iomem *dbi_base; >> + phys_addr_t dbi_phys_addr; >> void __iomem *dbi_base2; >> void __iomem *atu_base; >> + phys_addr_t atu_phys_addr; >> size_t atu_size; >> u32 num_ib_windows; >> u32 num_ob_windows; > > This patch is pretty trivial and it doesn't show anything to justify > the need to keep these addresses. I think this should be squashed > with the next patch that actually *uses* them. > ACK. I will update it in the next patch. Thanks, Prudhvi
On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote: > Both DBI and ATU physical base addresses are needed by pcie_qcom.c > driver to program the location of DBI and ATU blocks in Qualcomm > PCIe Controller specific PARF hardware block. > > Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com> > Reviewed-by: Mayank Rana <quic_mrana@quicinc.com> > --- > drivers/pci/controller/dwc/pcie-designware.c | 2 ++ > drivers/pci/controller/dwc/pcie-designware.h | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 1b5aba1f0c92..bc3a5d6b0177 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) > pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res); > if (IS_ERR(pci->dbi_base)) > return PTR_ERR(pci->dbi_base); > + pci->dbi_phys_addr = res->start; > } > > /* DBI2 is mainly useful for the endpoint controller */ > @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) > pci->atu_base = devm_ioremap_resource(pci->dev, res); > if (IS_ERR(pci->atu_base)) > return PTR_ERR(pci->atu_base); > + pci->atu_phys_addr = res->start; > } else { > pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET; > } > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 53c4c8f399c8..efc72989330c 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -407,8 +407,10 @@ struct dw_pcie_ops { > struct dw_pcie { > struct device *dev; > void __iomem *dbi_base; > + phys_addr_t dbi_phys_addr; > void __iomem *dbi_base2; > void __iomem *atu_base; > + phys_addr_t atu_phys_addr; What's the point in adding these fields to the generic DW PCIe private data if they are going to be used in the Qcom glue driver only? What about moving them to the qcom_pcie structure and initializing the fields in some place of the pcie-qcom.c driver? -Serge(y) > size_t atu_size; > u32 num_ib_windows; > u32 num_ob_windows; > -- > 2.25.1 > >
Hi Serge, Thanks for the review comment. On 8/1/2024 12:25 PM, Serge Semin wrote: > On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote: >> Both DBI and ATU physical base addresses are needed by pcie_qcom.c >> driver to program the location of DBI and ATU blocks in Qualcomm >> PCIe Controller specific PARF hardware block. >> >> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com> >> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com> >> --- >> drivers/pci/controller/dwc/pcie-designware.c | 2 ++ >> drivers/pci/controller/dwc/pcie-designware.h | 2 ++ >> 2 files changed, 4 insertions(+) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c >> index 1b5aba1f0c92..bc3a5d6b0177 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.c >> +++ b/drivers/pci/controller/dwc/pcie-designware.c >> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) >> pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res); >> if (IS_ERR(pci->dbi_base)) >> return PTR_ERR(pci->dbi_base); >> + pci->dbi_phys_addr = res->start; >> } >> >> /* DBI2 is mainly useful for the endpoint controller */ >> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) >> pci->atu_base = devm_ioremap_resource(pci->dev, res); >> if (IS_ERR(pci->atu_base)) >> return PTR_ERR(pci->atu_base); >> + pci->atu_phys_addr = res->start; >> } else { >> pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET; >> } >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h >> index 53c4c8f399c8..efc72989330c 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.h >> +++ b/drivers/pci/controller/dwc/pcie-designware.h >> @@ -407,8 +407,10 @@ struct dw_pcie_ops { >> struct dw_pcie { >> struct device *dev; >> void __iomem *dbi_base; > >> + phys_addr_t dbi_phys_addr; >> void __iomem *dbi_base2; >> void __iomem *atu_base; >> + phys_addr_t atu_phys_addr; > > What's the point in adding these fields to the generic DW PCIe private > data if they are going to be used in the Qcom glue driver only? > > What about moving them to the qcom_pcie structure and initializing the > fields in some place of the pcie-qcom.c driver? > > -Serge(y) > These fields were in pcie-qcom.c driver in the v1 patch[1] and Manivannan suggested to move these fields to 'struct dw_pcie' so that duplication of resource fetching code 'platform_get_resource_byname()' can be avoided. [1] https://lore.kernel.org/linux-pci/a01404d2-2f4d-4fb8-af9d-3db66d39acf7@quicinc.com/T/#mf9843386d57e9003de983e24e17de4d54314ff73 Thanks, Prudhvi >> size_t atu_size; >> u32 num_ib_windows; >> u32 num_ob_windows; >> -- >> 2.25.1 >> >>
On Thu, Aug 01, 2024 at 02:29:49PM -0700, Prudhvi Yarlagadda wrote: > Hi Serge, > > Thanks for the review comment. > > On 8/1/2024 12:25 PM, Serge Semin wrote: > > On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote: > >> Both DBI and ATU physical base addresses are needed by pcie_qcom.c > >> driver to program the location of DBI and ATU blocks in Qualcomm > >> PCIe Controller specific PARF hardware block. > >> > >> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com> > >> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com> > >> --- > >> drivers/pci/controller/dwc/pcie-designware.c | 2 ++ > >> drivers/pci/controller/dwc/pcie-designware.h | 2 ++ > >> 2 files changed, 4 insertions(+) > >> > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > >> index 1b5aba1f0c92..bc3a5d6b0177 100644 > >> --- a/drivers/pci/controller/dwc/pcie-designware.c > >> +++ b/drivers/pci/controller/dwc/pcie-designware.c > >> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) > >> pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res); > >> if (IS_ERR(pci->dbi_base)) > >> return PTR_ERR(pci->dbi_base); > >> + pci->dbi_phys_addr = res->start; > >> } > >> > >> /* DBI2 is mainly useful for the endpoint controller */ > >> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) > >> pci->atu_base = devm_ioremap_resource(pci->dev, res); > >> if (IS_ERR(pci->atu_base)) > >> return PTR_ERR(pci->atu_base); > >> + pci->atu_phys_addr = res->start; > >> } else { > >> pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET; > >> } > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > >> index 53c4c8f399c8..efc72989330c 100644 > >> --- a/drivers/pci/controller/dwc/pcie-designware.h > >> +++ b/drivers/pci/controller/dwc/pcie-designware.h > >> @@ -407,8 +407,10 @@ struct dw_pcie_ops { > >> struct dw_pcie { > >> struct device *dev; > >> void __iomem *dbi_base; > > > >> + phys_addr_t dbi_phys_addr; > >> void __iomem *dbi_base2; > >> void __iomem *atu_base; > >> + phys_addr_t atu_phys_addr; > > > > What's the point in adding these fields to the generic DW PCIe private > > data if they are going to be used in the Qcom glue driver only? > > > > What about moving them to the qcom_pcie structure and initializing the > > fields in some place of the pcie-qcom.c driver? > > > > -Serge(y) > > > > These fields were in pcie-qcom.c driver in the v1 patch[1] and > Manivannan suggested to move these fields to 'struct dw_pcie' so that duplication > of resource fetching code 'platform_get_resource_byname()' can be avoided. > > [1] https://lore.kernel.org/linux-pci/a01404d2-2f4d-4fb8-af9d-3db66d39acf7@quicinc.com/T/#mf9843386d57e9003de983e24e17de4d54314ff73 Em, polluting the core driver structure with data not being used by the core driver but by the glue-code doesn't seem like a better alternative to additional platform_get_resource_byname() call in the glue-driver. I would have got back v1 version so to keep the core driver simpler. Bjorn? -Serge(y) > > Thanks, > Prudhvi > >> size_t atu_size; > >> u32 num_ib_windows; > >> u32 num_ob_windows; > >> -- > >> 2.25.1 > >> > >>
On Fri, Aug 02, 2024 at 12:59:57AM +0300, Serge Semin wrote: > On Thu, Aug 01, 2024 at 02:29:49PM -0700, Prudhvi Yarlagadda wrote: > > Hi Serge, > > > > Thanks for the review comment. > > > > On 8/1/2024 12:25 PM, Serge Semin wrote: > > > On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote: > > >> Both DBI and ATU physical base addresses are needed by pcie_qcom.c > > >> driver to program the location of DBI and ATU blocks in Qualcomm > > >> PCIe Controller specific PARF hardware block. > > >> > > >> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com> > > >> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com> > > >> --- > > >> drivers/pci/controller/dwc/pcie-designware.c | 2 ++ > > >> drivers/pci/controller/dwc/pcie-designware.h | 2 ++ > > >> 2 files changed, 4 insertions(+) > > >> > > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > >> index 1b5aba1f0c92..bc3a5d6b0177 100644 > > >> --- a/drivers/pci/controller/dwc/pcie-designware.c > > >> +++ b/drivers/pci/controller/dwc/pcie-designware.c > > >> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) > > >> pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res); > > >> if (IS_ERR(pci->dbi_base)) > > >> return PTR_ERR(pci->dbi_base); > > >> + pci->dbi_phys_addr = res->start; > > >> } > > >> > > >> /* DBI2 is mainly useful for the endpoint controller */ > > >> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) > > >> pci->atu_base = devm_ioremap_resource(pci->dev, res); > > >> if (IS_ERR(pci->atu_base)) > > >> return PTR_ERR(pci->atu_base); > > >> + pci->atu_phys_addr = res->start; > > >> } else { > > >> pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET; > > >> } > > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > > >> index 53c4c8f399c8..efc72989330c 100644 > > >> --- a/drivers/pci/controller/dwc/pcie-designware.h > > >> +++ b/drivers/pci/controller/dwc/pcie-designware.h > > >> @@ -407,8 +407,10 @@ struct dw_pcie_ops { > > >> struct dw_pcie { > > >> struct device *dev; > > >> void __iomem *dbi_base; > > > > > >> + phys_addr_t dbi_phys_addr; > > >> void __iomem *dbi_base2; > > >> void __iomem *atu_base; > > >> + phys_addr_t atu_phys_addr; > > > > > > What's the point in adding these fields to the generic DW PCIe private > > > data if they are going to be used in the Qcom glue driver only? > > > > > > What about moving them to the qcom_pcie structure and initializing the > > > fields in some place of the pcie-qcom.c driver? > > > > > > -Serge(y) > > > > > > > > These fields were in pcie-qcom.c driver in the v1 patch[1] and > > Manivannan suggested to move these fields to 'struct dw_pcie' so that duplication > > of resource fetching code 'platform_get_resource_byname()' can be avoided. > > > > [1] https://lore.kernel.org/linux-pci/a01404d2-2f4d-4fb8-af9d-3db66d39acf7@quicinc.com/T/#mf9843386d57e9003de983e24e17de4d54314ff73 > > Em, polluting the core driver structure with data not being used by > the core driver but by the glue-code doesn't seem like a better > alternative to additional platform_get_resource_byname() call in the > glue-driver. I would have got back v1 version so to keep the core > driver simpler. Bjorn? > IDK how adding two fields which is very related to DWC code *pollutes* it. Since there is already 'dbi_base', adding 'dbi_phys_addr' made sense to me even though only glue drivers are using it. Otherwise, glue drivers have to duplicate the platform_get_resource_byname() code which I find annoying. - Mani
On Fri, Aug 02, 2024 at 10:52:06AM +0530, Manivannan Sadhasivam wrote: > On Fri, Aug 02, 2024 at 12:59:57AM +0300, Serge Semin wrote: > > On Thu, Aug 01, 2024 at 02:29:49PM -0700, Prudhvi Yarlagadda wrote: > > > Hi Serge, > > > > > > Thanks for the review comment. > > > > > > On 8/1/2024 12:25 PM, Serge Semin wrote: > > > > On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote: > > > >> Both DBI and ATU physical base addresses are needed by pcie_qcom.c > > > >> driver to program the location of DBI and ATU blocks in Qualcomm > > > >> PCIe Controller specific PARF hardware block. > > > >> > > > >> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com> > > > >> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com> > > > >> --- > > > >> drivers/pci/controller/dwc/pcie-designware.c | 2 ++ > > > >> drivers/pci/controller/dwc/pcie-designware.h | 2 ++ > > > >> 2 files changed, 4 insertions(+) > > > >> > > > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > >> index 1b5aba1f0c92..bc3a5d6b0177 100644 > > > >> --- a/drivers/pci/controller/dwc/pcie-designware.c > > > >> +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > >> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) > > > >> pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res); > > > >> if (IS_ERR(pci->dbi_base)) > > > >> return PTR_ERR(pci->dbi_base); > > > >> + pci->dbi_phys_addr = res->start; > > > >> } > > > >> > > > >> /* DBI2 is mainly useful for the endpoint controller */ > > > >> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) > > > >> pci->atu_base = devm_ioremap_resource(pci->dev, res); > > > >> if (IS_ERR(pci->atu_base)) > > > >> return PTR_ERR(pci->atu_base); > > > >> + pci->atu_phys_addr = res->start; > > > >> } else { > > > >> pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET; > > > >> } > > > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > > > >> index 53c4c8f399c8..efc72989330c 100644 > > > >> --- a/drivers/pci/controller/dwc/pcie-designware.h > > > >> +++ b/drivers/pci/controller/dwc/pcie-designware.h > > > >> @@ -407,8 +407,10 @@ struct dw_pcie_ops { > > > >> struct dw_pcie { > > > >> struct device *dev; > > > >> void __iomem *dbi_base; > > > > > > > >> + phys_addr_t dbi_phys_addr; > > > >> void __iomem *dbi_base2; > > > >> void __iomem *atu_base; > > > >> + phys_addr_t atu_phys_addr; > > > > > > > > What's the point in adding these fields to the generic DW PCIe private > > > > data if they are going to be used in the Qcom glue driver only? > > > > > > > > What about moving them to the qcom_pcie structure and initializing the > > > > fields in some place of the pcie-qcom.c driver? > > > > > > > > -Serge(y) > > > > > > > > > > > > These fields were in pcie-qcom.c driver in the v1 patch[1] and > > > Manivannan suggested to move these fields to 'struct dw_pcie' so that duplication > > > of resource fetching code 'platform_get_resource_byname()' can be avoided. > > > > > > [1] https://lore.kernel.org/linux-pci/a01404d2-2f4d-4fb8-af9d-3db66d39acf7@quicinc.com/T/#mf9843386d57e9003de983e24e17de4d54314ff73 > > > > Em, polluting the core driver structure with data not being used by > > the core driver but by the glue-code doesn't seem like a better > > alternative to additional platform_get_resource_byname() call in the > > glue-driver. I would have got back v1 version so to keep the core > > driver simpler. Bjorn? > > > > IDK how adding two fields which is very related to DWC code *pollutes* it. Since > there is already 'dbi_base', adding 'dbi_phys_addr' made sense to me even though > only glue drivers are using it. Otherwise, glue drivers have to duplicate the > platform_get_resource_byname() code which I find annoying. I just explained why it was redundant: 1. adding the fields expands the core private data size for _all_ platforms for no reason. (a few bytes but still) 2. the new fields aren't utilized by the core driver, but still defined in the core private data which is first confusing and second implicitly encourages the kernel developers to add another unused or even weakly-related fields in there. 3. the new fields utilized in a single glue-driver and there is a small chance they will be used in another ones. Another story would have been if we had them used in more than one glue-driver... So from that perspective I find adding these fields to the driver core data less appropriate than duplicating the platform_get_resource_byname() call in a _single_ glue driver. It seems more reasonable to have them defined and utilized in the code that actually needs them, but not in the place that doesn't annoy you.) Anyway I read your v1 command and did understand your point in the first place. That's why my question was addressed to Bjorn. Please also note the resource::start field is of the resource_size_t type. So wherever the fields are added, it's better to have them defined of that type instead. -Serge(y) > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
On 8/2/2024 2:22 AM, Serge Semin wrote: > On Fri, Aug 02, 2024 at 10:52:06AM +0530, Manivannan Sadhasivam wrote: >> On Fri, Aug 02, 2024 at 12:59:57AM +0300, Serge Semin wrote: >>> On Thu, Aug 01, 2024 at 02:29:49PM -0700, Prudhvi Yarlagadda wrote: >>>> Hi Serge, >>>> >>>> Thanks for the review comment. >>>> >>>> On 8/1/2024 12:25 PM, Serge Semin wrote: >>>>> On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote: >>>>>> Both DBI and ATU physical base addresses are needed by pcie_qcom.c >>>>>> driver to program the location of DBI and ATU blocks in Qualcomm >>>>>> PCIe Controller specific PARF hardware block. >>>>>> >>>>>> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com> >>>>>> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com> >>>>>> --- >>>>>> drivers/pci/controller/dwc/pcie-designware.c | 2 ++ >>>>>> drivers/pci/controller/dwc/pcie-designware.h | 2 ++ >>>>>> 2 files changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c >>>>>> index 1b5aba1f0c92..bc3a5d6b0177 100644 >>>>>> --- a/drivers/pci/controller/dwc/pcie-designware.c >>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware.c >>>>>> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) >>>>>> pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res); >>>>>> if (IS_ERR(pci->dbi_base)) >>>>>> return PTR_ERR(pci->dbi_base); >>>>>> + pci->dbi_phys_addr = res->start; >>>>>> } >>>>>> >>>>>> /* DBI2 is mainly useful for the endpoint controller */ >>>>>> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) >>>>>> pci->atu_base = devm_ioremap_resource(pci->dev, res); >>>>>> if (IS_ERR(pci->atu_base)) >>>>>> return PTR_ERR(pci->atu_base); >>>>>> + pci->atu_phys_addr = res->start; >>>>>> } else { >>>>>> pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET; >>>>>> } >>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h >>>>>> index 53c4c8f399c8..efc72989330c 100644 >>>>>> --- a/drivers/pci/controller/dwc/pcie-designware.h >>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware.h >>>>>> @@ -407,8 +407,10 @@ struct dw_pcie_ops { >>>>>> struct dw_pcie { >>>>>> struct device *dev; >>>>>> void __iomem *dbi_base; >>>>> >>>>>> + phys_addr_t dbi_phys_addr; >>>>>> void __iomem *dbi_base2; >>>>>> void __iomem *atu_base; >>>>>> + phys_addr_t atu_phys_addr; >>>>> >>>>> What's the point in adding these fields to the generic DW PCIe private >>>>> data if they are going to be used in the Qcom glue driver only? >>>>> >>>>> What about moving them to the qcom_pcie structure and initializing the >>>>> fields in some place of the pcie-qcom.c driver? >>>>> >>>>> -Serge(y) >>>>> >>>> >>> >>>> These fields were in pcie-qcom.c driver in the v1 patch[1] and >>>> Manivannan suggested to move these fields to 'struct dw_pcie' so that duplication >>>> of resource fetching code 'platform_get_resource_byname()' can be avoided. >>>> >>>> [1] https://lore.kernel.org/linux-pci/a01404d2-2f4d-4fb8-af9d-3db66d39acf7@quicinc.com/T/#mf9843386d57e9003de983e24e17de4d54314ff73 >>> >>> Em, polluting the core driver structure with data not being used by >>> the core driver but by the glue-code doesn't seem like a better >>> alternative to additional platform_get_resource_byname() call in the >>> glue-driver. I would have got back v1 version so to keep the core >>> driver simpler. Bjorn? >>> >> >> IDK how adding two fields which is very related to DWC code *pollutes* it. Since >> there is already 'dbi_base', adding 'dbi_phys_addr' made sense to me even though >> only glue drivers are using it. Otherwise, glue drivers have to duplicate the >> platform_get_resource_byname() code which I find annoying. > > I just explained why it was redundant: > 1. adding the fields expands the core private data size for _all_ > platforms for no reason. (a few bytes but still) > 2. the new fields aren't utilized by the core driver, but still > defined in the core private data which is first confusing and > second implicitly encourages the kernel developers to add another > unused or even weakly-related fields in there. > 3. the new fields utilized in a single glue-driver and there is a small > chance they will be used in another ones. Another story would have > been if we had them used in more than one glue-driver... > > So from that perspective I find adding these fields to the driver core > data less appropriate than duplicating the > platform_get_resource_byname() call in a _single_ glue driver. It > seems more reasonable to have them defined and utilized in the code > that actually needs them, but not in the place that doesn't annoy you.) > > Anyway I read your v1 command and did understand your point in the > first place. That's why my question was addressed to Bjorn. > > Please also note the resource::start field is of the resource_size_t > type. So wherever the fields are added, it's better to have them > defined of that type instead. > > -Serge(y) > Hi Bjorn, Gentle ping for your feedback on the above discussed two approaches. Thanks, Prudhvi >> >> - Mani >> >> -- >> மணிவண்ணன் சதாசிவம் >
On Thu, Aug 08, 2024 at 11:30:52AM -0700, Prudhvi Yarlagadda wrote: > On 8/2/2024 2:22 AM, Serge Semin wrote: > > On Fri, Aug 02, 2024 at 10:52:06AM +0530, Manivannan Sadhasivam wrote: > >> On Fri, Aug 02, 2024 at 12:59:57AM +0300, Serge Semin wrote: > >>> On Thu, Aug 01, 2024 at 02:29:49PM -0700, Prudhvi Yarlagadda wrote: > >>>> Hi Serge, > >>>> > >>>> Thanks for the review comment. > >>>> > >>>> On 8/1/2024 12:25 PM, Serge Semin wrote: > >>>>> On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote: > >>>>>> Both DBI and ATU physical base addresses are needed by pcie_qcom.c > >>>>>> driver to program the location of DBI and ATU blocks in Qualcomm > >>>>>> PCIe Controller specific PARF hardware block. > >>>>>> > >>>>>> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com> > >>>>>> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com> > >>>>>> --- > >>>>>> drivers/pci/controller/dwc/pcie-designware.c | 2 ++ > >>>>>> drivers/pci/controller/dwc/pcie-designware.h | 2 ++ > >>>>>> 2 files changed, 4 insertions(+) > >>>>>> > >>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > >>>>>> index 1b5aba1f0c92..bc3a5d6b0177 100644 > >>>>>> --- a/drivers/pci/controller/dwc/pcie-designware.c > >>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware.c > >>>>>> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) > >>>>>> pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res); > >>>>>> if (IS_ERR(pci->dbi_base)) > >>>>>> return PTR_ERR(pci->dbi_base); > >>>>>> + pci->dbi_phys_addr = res->start; > >>>>>> } > >>>>>> > >>>>>> /* DBI2 is mainly useful for the endpoint controller */ > >>>>>> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) > >>>>>> pci->atu_base = devm_ioremap_resource(pci->dev, res); > >>>>>> if (IS_ERR(pci->atu_base)) > >>>>>> return PTR_ERR(pci->atu_base); > >>>>>> + pci->atu_phys_addr = res->start; > >>>>>> } else { > >>>>>> pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET; > >>>>>> } > >>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > >>>>>> index 53c4c8f399c8..efc72989330c 100644 > >>>>>> --- a/drivers/pci/controller/dwc/pcie-designware.h > >>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware.h > >>>>>> @@ -407,8 +407,10 @@ struct dw_pcie_ops { > >>>>>> struct dw_pcie { > >>>>>> struct device *dev; > >>>>>> void __iomem *dbi_base; > >>>>> > >>>>>> + phys_addr_t dbi_phys_addr; > >>>>>> void __iomem *dbi_base2; > >>>>>> void __iomem *atu_base; > >>>>>> + phys_addr_t atu_phys_addr; > >>>>> > >>>>> What's the point in adding these fields to the generic DW PCIe private > >>>>> data if they are going to be used in the Qcom glue driver only? > >>>>> > >>>>> What about moving them to the qcom_pcie structure and initializing the > >>>>> fields in some place of the pcie-qcom.c driver? > >>>>> > >>>>> -Serge(y) > >>>>> > >>>> > >>> > >>>> These fields were in pcie-qcom.c driver in the v1 patch[1] and > >>>> Manivannan suggested to move these fields to 'struct dw_pcie' so that duplication > >>>> of resource fetching code 'platform_get_resource_byname()' can be avoided. > >>>> > >>>> [1] https://lore.kernel.org/linux-pci/a01404d2-2f4d-4fb8-af9d-3db66d39acf7@quicinc.com/T/#mf9843386d57e9003de983e24e17de4d54314ff73 > >>> > >>> Em, polluting the core driver structure with data not being used by > >>> the core driver but by the glue-code doesn't seem like a better > >>> alternative to additional platform_get_resource_byname() call in the > >>> glue-driver. I would have got back v1 version so to keep the core > >>> driver simpler. Bjorn? > >>> > >> > >> IDK how adding two fields which is very related to DWC code *pollutes* it. Since > >> there is already 'dbi_base', adding 'dbi_phys_addr' made sense to me even though > >> only glue drivers are using it. Otherwise, glue drivers have to duplicate the > >> platform_get_resource_byname() code which I find annoying. > > > > I just explained why it was redundant: > > 1. adding the fields expands the core private data size for _all_ > > platforms for no reason. (a few bytes but still) > > 2. the new fields aren't utilized by the core driver, but still > > defined in the core private data which is first confusing and > > second implicitly encourages the kernel developers to add another > > unused or even weakly-related fields in there. > > 3. the new fields utilized in a single glue-driver and there is a small > > chance they will be used in another ones. Another story would have > > been if we had them used in more than one glue-driver... > > > > So from that perspective I find adding these fields to the driver core > > data less appropriate than duplicating the > > platform_get_resource_byname() call in a _single_ glue driver. It > > seems more reasonable to have them defined and utilized in the code > > that actually needs them, but not in the place that doesn't annoy you.) > > > > Anyway I read your v1 command and did understand your point in the > > first place. That's why my question was addressed to Bjorn. > > > > Please also note the resource::start field is of the resource_size_t > > type. So wherever the fields are added, it's better to have them > > defined of that type instead. > > Gentle ping for your feedback on the above discussed two approaches. I don't really care one way or the other. Jingoo and Mani can sort it out.
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index 1b5aba1f0c92..bc3a5d6b0177 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res); if (IS_ERR(pci->dbi_base)) return PTR_ERR(pci->dbi_base); + pci->dbi_phys_addr = res->start; } /* DBI2 is mainly useful for the endpoint controller */ @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci) pci->atu_base = devm_ioremap_resource(pci->dev, res); if (IS_ERR(pci->atu_base)) return PTR_ERR(pci->atu_base); + pci->atu_phys_addr = res->start; } else { pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET; } diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 53c4c8f399c8..efc72989330c 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -407,8 +407,10 @@ struct dw_pcie_ops { struct dw_pcie { struct device *dev; void __iomem *dbi_base; + phys_addr_t dbi_phys_addr; void __iomem *dbi_base2; void __iomem *atu_base; + phys_addr_t atu_phys_addr; size_t atu_size; u32 num_ib_windows; u32 num_ob_windows;