On Wed, Apr 7, 2021 at 4:04 AM Z.q. Hou <[email protected]> wrote: > > Hi Rob, > > Thanks a lot for the comments! > > > -----Original Message----- > > From: Rob Herring <[email protected]> > > Sent: 2021年4月7日 6:00 > > To: Z.q. Hou <[email protected]> > > Cc: PCI <[email protected]>; [email protected]; Lorenzo > > Pieralisi <[email protected]>; Bjorn Helgaas > > <[email protected]>; Kishon Vijay Abraham I <[email protected]>; Jingoo > > Han <[email protected]>; Richard Zhu <[email protected]>; > > Lucas Stach <[email protected]>; Murali Karicheri > > <[email protected]>; M.h. Lian <[email protected]>; Mingkai Hu > > <[email protected]>; Roy Zang <[email protected]>; Yue Wang > > <[email protected]>; Jonathan Chocron <[email protected]>; > > Thomas Petazzoni <[email protected]>; Jesper Nilsson > > <[email protected]>; Gustavo Pimentel > > <[email protected]>; Xiaowei Song > > <[email protected]>; Binghui Wang <[email protected]>; > > Stanimir Varbanov <[email protected]>; Pratyush Anand > > <[email protected]>; Kunihiko Hayashi > > <[email protected]>; Jason Yan <[email protected]>; > > Thierry Reding <[email protected]> > > Subject: Re: [PATCH] PCI: dwc: Change the inheritance between the > > abstracted structures > > > > On Fri, Jan 29, 2021 at 3:32 AM Zhiqiang Hou <[email protected]> > > wrote: > > > > > > From: Hou Zhiqiang <[email protected]> > > > > > > Currently the core struct dw_pcie includes both struct pcie_port > > > and dw_pcie_ep and the RC and EP platform drivers directly > > > includes the dw_pcie. So it results in a RC or EP platform driver > > > has 2 indirect parents pcie_port and dw_pcie_ep, but it doesn't > > > make sense let RC platform driver includes the dw_pcie_ep and > > > so does the EP platform driver. > > > > A less invasive change would be just doing a union: > > > > union { > > struct pcie_port pp; > > struct dw_pcie_ep ep; > > }; > > > > Though I agree reversing how the structs are embedded is more logical. > [Z.q. Hou] > Yes, this change involved all the platform drivers, but I think it's worth, > this change makes the drivers more easy to understand and maintain. > > > > > Ideally, I'd like to see all drivers move to a single alloc using > > devm_pci_alloc_host_bridge() which takes extra size for a private > > struct. Currently, every driver has either 2 or 3 allocs. The first > > step I think is getting rid of the 3rd alloc by embedding the DWC > > struct into the platform specific struct rather than having a pointer > > to the DWC struct. > > Yes, agree it's the direction we are stepping to and it doesn't conflict > with this change.
I'm not convinced of that. If anything we're changing the same code twice. > > > This patch makes the struct pcie_port and dw_pcie_ep includes > > > the core struct dw_pcie and the RC and EP platform drivers > > > include struct pcie_port and dw_pcie_ep respectively. > > > > > > Signed-off-by: Hou Zhiqiang <[email protected]> > > > Cc: Kishon Vijay Abraham I <[email protected]> > > > Cc: Lorenzo Pieralisi <[email protected]> > > > Cc: Rob Herring <[email protected]> > > > Cc: Bjorn Helgaas <[email protected]> > > > Cc: Jingoo Han <[email protected]> > > > Cc: Richard Zhu <[email protected]> > > > Cc: Lucas Stach <[email protected]> > > > Cc: Murali Karicheri <[email protected]> > > > Cc: Minghuan Lian <[email protected]> > > > Cc: Mingkai Hu <[email protected]> > > > Cc: Roy Zang <[email protected]> > > > Cc: Yue Wang <[email protected]> > > > Cc: Jonathan Chocron <[email protected]> > > > Cc: Thomas Petazzoni <[email protected]> > > > Cc: Jesper Nilsson <[email protected]> > > > Cc: Gustavo Pimentel <[email protected]> > > > Cc: Xiaowei Song <[email protected]> > > > Cc: Binghui Wang <[email protected]> > > > Cc: Stanimir Varbanov <[email protected]> > > > Cc: Pratyush Anand <[email protected]> > > > Cc: Kunihiko Hayashi <[email protected]> > > > Cc: Jason Yan <[email protected]> > > > Cc: Thierry Reding <[email protected]> > > > --- > > > drivers/pci/controller/dwc/pci-dra7xx.c | 74 +++++--- > > > drivers/pci/controller/dwc/pci-exynos.c | 26 +-- > > > drivers/pci/controller/dwc/pci-imx6.c | 46 +++-- > > > drivers/pci/controller/dwc/pci-keystone.c | 79 +++++--- > > > .../pci/controller/dwc/pci-layerscape-ep.c | 18 +- > > > drivers/pci/controller/dwc/pci-layerscape.c | 51 +++--- > > > drivers/pci/controller/dwc/pci-meson.c | 25 +-- > > > drivers/pci/controller/dwc/pcie-al.c | 21 ++- > > > drivers/pci/controller/dwc/pcie-armada8k.c | 17 +- > > > drivers/pci/controller/dwc/pcie-artpec6.c | 74 +++++--- > > > .../pci/controller/dwc/pcie-designware-host.c | 2 +- > > > .../pci/controller/dwc/pcie-designware-plat.c | 38 ++-- > > > drivers/pci/controller/dwc/pcie-designware.h | 72 ++++---- > > > drivers/pci/controller/dwc/pcie-histb.c | 27 +-- > > > drivers/pci/controller/dwc/pcie-intel-gw.c | 42 +++-- > > > drivers/pci/controller/dwc/pcie-kirin.c | 42 +++-- > > > drivers/pci/controller/dwc/pcie-qcom.c | 40 ++--- > > > drivers/pci/controller/dwc/pcie-spear13xx.c | 16 +- > > > drivers/pci/controller/dwc/pcie-tegra194.c | 169 > > +++++++++++------- > > > drivers/pci/controller/dwc/pcie-uniphier-ep.c | 14 +- > > > drivers/pci/controller/dwc/pcie-uniphier.c | 17 +- > > > 21 files changed, 557 insertions(+), 353 deletions(-) > > > > What exactly have we improved with 200 more lines? > > No performance improvement, but corrected the cluttered inheritance logic. I prefer the 200 fewer lines and the current structure. So you had better make the diffstat more appealing. > > > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c > > b/drivers/pci/controller/dwc/pci-dra7xx.c > > > index 12726c63366f..0e914df6eaba 100644 > > > --- a/drivers/pci/controller/dwc/pci-dra7xx.c > > > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c > > > @@ -85,7 +85,8 @@ > > > #define PCIE_B0_B1_TSYNCEN BIT(0) > > > > > > struct dra7xx_pcie { > > > - struct dw_pcie *pci; > > > + struct pcie_port *pp; > > > + struct dw_pcie_ep *ep; > > > > It's better to keep struct dw_pcie ptr here because we can easily get > > pp or ep from it. > > With this patch, I want to change all the RC and EP platform drivers use the > 'struct pcie_port' > and 'struct dw_pcie_ep' respectively instead of embedded the core 'struct > dw_pcie'. > As this driver is for both RC and EP mode, and they are using the same private > 'struct dra7xx_pcie', so the 'struct pcie_port *pp' and 'struct dw_pcie_ep > *ep' are both > put into the private struct and they are used by the corresponding driver. > > > > > > > void __iomem *base; /* DT ti_conf */ > > > int phy_count; /* DT phy-names > > count */ > > > struct phy **phy; > > > @@ -290,11 +291,19 @@ static void dra7xx_pcie_msi_irq_handler(struct > > irq_desc *desc) > > > static irqreturn_t dra7xx_pcie_irq_handler(int irq, void *arg) > > > { > > > struct dra7xx_pcie *dra7xx = arg; > > > - struct dw_pcie *pci = dra7xx->pci; > > > - struct device *dev = pci->dev; > > > - struct dw_pcie_ep *ep = &pci->ep; > > > + struct dw_pcie_ep *ep; > > > + struct dw_pcie *pci; > > > + struct device *dev; > > > u32 reg; > > > > > > + if (dra7xx->mode == DW_PCIE_RC_TYPE) { > > > + pci = to_dw_pcie_from_pp(dra7xx->pp); > > > + } else { > > > + ep = dra7xx->ep; > > > + pci = to_dw_pcie_from_ep(ep); > > > + } > > > > This is not a good pattern... > > Will change to the 'switch (mode) {...}' in next version. No! Even worse. Given the function needs struct dw_pcie, it should be able to get it without having to check the mode. We could before, so this is a step backwards. Also, dra7xx->mode is something I plan to move into struct dw_pcie because it's duplicated across drivers. And this change just makes doing that harder. > > > @@ -105,11 +105,12 @@ static void histb_pcie_dbi_r_mode(struct > > pcie_port *pp, bool enable) > > > static u32 histb_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base, > > > u32 reg, size_t size) > > > { > > > + struct histb_pcie *hipcie = to_histb_pcie(pci); > > > u32 val; > > > > > > - histb_pcie_dbi_r_mode(&pci->pp, true); > > > + histb_pcie_dbi_r_mode(hipcie->pp, true); > > > > DBI access really has nothing to do with RC or EP mode, so this > > function should probably take a struct dw_pcie *. > > Agree, it can be improved but this patch is not intend to change the platform > driver > internal operations. IMO, you should before doing this change. That's how you'll make the diffstat more appealing. > > > dw_pcie_read(base + reg, size, &val); > > > - histb_pcie_dbi_r_mode(&pci->pp, false); > > > + histb_pcie_dbi_r_mode(hipcie->pp, false); > > > > > > return val; > > > }
