On Wed, Apr 7, 2021 at 4:04 AM Z.q. Hou <zhiqiang.hou@xxxxxxx> wrote:
>
> Hi Rob,
>
> Thanks a lot for the comments!
>
> > -----Original Message-----
> > From: Rob Herring <robh@xxxxxxxxxx>
> > Sent: 2021年4月7日 6:00
> > To: Z.q. Hou <zhiqiang.hou@xxxxxxx>
> > Cc: PCI <linux-pci@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; Lorenzo
> > Pieralisi <lorenzo.pieralisi@xxxxxxx>; Bjorn Helgaas
> > <bhelgaas@xxxxxxxxxx>; Kishon Vijay Abraham I <kishon@xxxxxx>; Jingoo
> > Han <jingoohan1@xxxxxxxxx>; Richard Zhu <hongxing.zhu@xxxxxxx>;
> > Lucas Stach <l.stach@xxxxxxxxxxxxxx>; Murali Karicheri
> > <m-karicheri2@xxxxxx>; M.h. Lian <minghuan.lian@xxxxxxx>; Mingkai Hu
> > <mingkai.hu@xxxxxxx>; Roy Zang <roy.zang@xxxxxxx>; Yue Wang
> > <yue.wang@xxxxxxxxxxx>; Jonathan Chocron <jonnyc@xxxxxxxxxx>;
> > Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxx>; Jesper Nilsson
> > <jesper.nilsson@xxxxxxxx>; Gustavo Pimentel
> > <gustavo.pimentel@xxxxxxxxxxxx>; Xiaowei Song
> > <songxiaowei@xxxxxxxxxxxxx>; Binghui Wang <wangbinghui@xxxxxxxxxxxxx>;
> > Stanimir Varbanov <svarbanov@xxxxxxxxxx>; Pratyush Anand
> > <pratyush.anand@xxxxxxxxx>; Kunihiko Hayashi
> > <hayashi.kunihiko@xxxxxxxxxxxxx>; Jason Yan <yanaijie@xxxxxxxxxx>;
> > Thierry Reding <thierry.reding@xxxxxxxxx>
> > Subject: Re: [PATCH] PCI: dwc: Change the inheritance between the
> > abstracted structures
> >
> > On Fri, Jan 29, 2021 at 3:32 AM Zhiqiang Hou <Zhiqiang.Hou@xxxxxxx>
> > wrote:
> > >
> > > From: Hou Zhiqiang <Zhiqiang.Hou@xxxxxxx>
> > >
> > > 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 <Zhiqiang.Hou@xxxxxxx>
> > > Cc: Kishon Vijay Abraham I <kishon@xxxxxx>
> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> > > Cc: Rob Herring <robh@xxxxxxxxxx>
> > > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > > Cc: Jingoo Han <jingoohan1@xxxxxxxxx>
> > > Cc: Richard Zhu <hongxing.zhu@xxxxxxx>
> > > Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> > > Cc: Murali Karicheri <m-karicheri2@xxxxxx>
> > > Cc: Minghuan Lian <minghuan.Lian@xxxxxxx>
> > > Cc: Mingkai Hu <mingkai.hu@xxxxxxx>
> > > Cc: Roy Zang <roy.zang@xxxxxxx>
> > > Cc: Yue Wang <yue.wang@xxxxxxxxxxx>
> > > Cc: Jonathan Chocron <jonnyc@xxxxxxxxxx>
> > > Cc: Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxx>
> > > Cc: Jesper Nilsson <jesper.nilsson@xxxxxxxx>
> > > Cc: Gustavo Pimentel <gustavo.pimentel@xxxxxxxxxxxx>
> > > Cc: Xiaowei Song <songxiaowei@xxxxxxxxxxxxx>
> > > Cc: Binghui Wang <wangbinghui@xxxxxxxxxxxxx>
> > > Cc: Stanimir Varbanov <svarbanov@xxxxxxxxxx>
> > > Cc: Pratyush Anand <pratyush.anand@xxxxxxxxx>
> > > Cc: Kunihiko Hayashi <hayashi.kunihiko@xxxxxxxxxxxxx>
> > > Cc: Jason Yan <yanaijie@xxxxxxxxxx>
> > > Cc: Thierry Reding <thierry.reding@xxxxxxxxx>
> > > ---
> > > 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;
> > > }
