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@xxxxxxxxxxx>
> > > >> Reviewed-by: Mayank Rana <quic_mrana@xxxxxxxxxxx>
> > > >> ---
> > > >> 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@xxxxxxxxxxx/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
>
> --
> மணிவண்ணன் சதாசிவம்
