Re: [PATCH] PCI: dwc: Change the inheritance between the abstracted structures
On Wed, Apr 7, 2021 at 4:04 AM Z.q. Hou wrote:
>
> Hi Rob,
>
> Thanks a lot for the comments!
>
> > -Original Message-
> > From: Rob Herring
> > Sent: 2021年4月7日 6:00
> > To: Z.q. Hou
> > Cc: PCI ; linux-kernel@vger.kernel.org; Lorenzo
> > Pieralisi ; Bjorn Helgaas
> > ; Kishon Vijay Abraham I ; Jingoo
> > Han ; Richard Zhu ;
> > Lucas Stach ; Murali Karicheri
> > ; M.h. Lian ; Mingkai Hu
> > ; Roy Zang ; Yue Wang
> > ; Jonathan Chocron ;
> > Thomas Petazzoni ; Jesper Nilsson
> > ; Gustavo Pimentel
> > ; Xiaowei Song
> > ; Binghui Wang ;
> > Stanimir Varbanov ; Pratyush Anand
> > ; Kunihiko Hayashi
> > ; Jason Yan ;
> > Thierry Reding
> > Subject: Re: [PATCH] PCI: dwc: Change the inheritance between the
> > abstracted structures
> >
> > On Fri, Jan 29, 2021 at 3:32 AM Zhiqiang Hou
> > wrote:
> > >
> > > From: Hou Zhiqiang
> > >
> > > 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_portpp;
> >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
> > > Cc: Kishon Vijay Abraham I
> > > Cc: Lorenzo Pieralisi
> > > Cc: Rob Herring
> > > Cc: Bjorn Helgaas
> > > Cc: Jingoo Han
> > > Cc: Richard Zhu
> > > Cc: Lucas Stach
> > > Cc: Murali Karicheri
> > > Cc: Minghuan Lian
> > > Cc: Mingkai Hu
> > > Cc: Roy Zang
> > > Cc: Yue Wang
> > > Cc: Jonathan Chocron
> > > Cc: Thomas Petazzoni
> > > Cc: Jesper Nilsson
> > > Cc: Gustavo Pimentel
> > > Cc: Xiaowei Song
> > > Cc: Binghui Wang
> > > Cc: Stanimir Varbanov
> > > Cc: Pratyush Anand
> > > Cc: Kunihiko Hayashi
> > > Cc: Jason Yan
> > > Cc: Thierry Reding
> > > ---
> > > 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 +-
>
RE: [PATCH] PCI: dwc: Change the inheritance between the abstracted structures
Hi Bjorn,
Thanks a lot for the comments!
> -Original Message-
> From: Bjorn Helgaas
> Sent: 2021年4月7日 1:09
> To: Z.q. Hou
> Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org;
> lorenzo.pieral...@arm.com; r...@kernel.org; bhelg...@google.com;
> Kishon Vijay Abraham I ; Jingoo Han
> ; Richard Zhu ; Lucas
> Stach ; Murali Karicheri ;
> M.h. Lian ; Mingkai Hu ;
> Roy Zang ; Yue Wang ;
> Jonathan Chocron ; Thomas Petazzoni
> ; Jesper Nilsson
> ; Gustavo Pimentel
> ; Xiaowei Song
> ; Binghui Wang ;
> Stanimir Varbanov ; Pratyush Anand
> ; Kunihiko Hayashi
> ; Jason Yan ;
> Thierry Reding
> Subject: Re: [PATCH] PCI: dwc: Change the inheritance between the
> abstracted structures
>
> On Tue, Apr 06, 2021 at 05:28:25PM +0800, Zhiqiang Hou wrote:
> > From: Hou Zhiqiang
> >
> > 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.
> >
> > 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.
>
> I really like the way this patch is heading. There's a lot of historical
> cruft in
> these drivers and this is a good step to cleaning it up. Thanks a lot for
> working on this!
>
> What does this patch apply to? It doesn't apply cleanly to either my "main"
> branch or the "next" branch. Try to send things that apply to "main" and if
> it needs to apply on top of something else, mention what that is.
Sorry, this patch was workout in several months ago, but seems my mailbox failed
to send it out that time, I can rebase it in these days if necessary.
>
> > 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;
>
> 1) This is not related to your patch, but I think "pcie_port" used to
>make more sense before we had endpoint drivers, but now it's the
>wrong name. Root Ports and Endpoints both have "PCIe Ports", but
>IIUC "struct pcie_port" only applies to Root Ports, and "struct
>dw_pcie_ep" is the analogue for Endpoints.
>
>It would be nice to coordinate these names with a separate patch,
>e.g., maybe "dw_pcie_rc" (or "dw_pcie_rp") and "dw_pcie_ep".
Cannot agree more!
>
> 2) We allocate struct dra7xx_pcie for both RPs and EPs. But IIUC, RPs
>only use "struct pcie_port", and EPs only use "struct dw_pcie_ep".
>It doesn't seem right to keep both pointers when only one is ever
>used.
The reason is this driver is for both RC mode and EP mode, and they are
using the same private struct dra7xx_pcie.
Do you have some suggestion on this case? Add a union of RC struct
in the driver private struct and uses the corresponding union member
according to the PCIe controller's mode, is it better?
Struct dra7xxx_pcie {
...
union {
struct pcie_port pp;
struct dw_pcie_ep ep;
} type;
...
}
>
> 3) I'm not sure why these should be pointers at all. Why can't they
>be directly embedded, e.g., "struct pcie_port pp" instead of
>"struct pcie_port *pp"? Obviously this would have to be done in a
>way that we allocate an RC-specific structure or an EP-specific
>one.
Agree, and this change can be done in a separate patch.
>
> > void __iomem*base; /* DT ti_conf */
> > int phy_count; /* DT phy-names count */
> > struct phy **phy;
>
> > @@ -796,6 +798,17 @@ static int __init dra7xx_pcie_probe(struct
> > platform_device *pdev)
> >
> > switch (mode) {
> > case DW_PCIE_RC_TYPE:
> > + pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL);
>
> We know "mode" right after the of_match_device() at the top of this
> function. I think we should
Re: [PATCH] PCI: dwc: Change the inheritance between the abstracted structures
On Fri, Jan 29, 2021 at 3:32 AM Zhiqiang Hou wrote:
>
> From: Hou Zhiqiang
>
> 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_portpp;
struct dw_pcie_ep ep;
};
Though I agree reversing how the structs are embedded is more logical.
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.
> 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
> Cc: Kishon Vijay Abraham I
> Cc: Lorenzo Pieralisi
> Cc: Rob Herring
> Cc: Bjorn Helgaas
> Cc: Jingoo Han
> Cc: Richard Zhu
> Cc: Lucas Stach
> Cc: Murali Karicheri
> Cc: Minghuan Lian
> Cc: Mingkai Hu
> Cc: Roy Zang
> Cc: Yue Wang
> Cc: Jonathan Chocron
> Cc: Thomas Petazzoni
> Cc: Jesper Nilsson
> Cc: Gustavo Pimentel
> Cc: Xiaowei Song
> Cc: Binghui Wang
> Cc: Stanimir Varbanov
> Cc: Pratyush Anand
> Cc: Kunihiko Hayashi
> Cc: Jason Yan
> Cc: Thierry Reding
> ---
> 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?
> 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.
> 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 = >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...
> +
> + dev = pci->dev;
> reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN);
>
> if (reg & ERR_SYS)
> @@ -447,11 +456,10 @@ static int __init dra7xx_add_pcie_ep(struct dra7xx_pcie
> *dra7xx,
> struct platform_device *pdev)
> {
> int ret;
> - struct dw_pcie_ep *ep;
> + struct dw_pcie_ep *ep = dra7xx->ep;
> + struct dw_pcie *pci =
Re: [PATCH] PCI: dwc: Change the inheritance between the abstracted structures
On Tue, Apr 06, 2021 at 05:28:25PM +0800, Zhiqiang Hou wrote:
> From: Hou Zhiqiang
>
> 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.
>
> 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.
I really like the way this patch is heading. There's a lot of
historical cruft in these drivers and this is a good step to cleaning
it up. Thanks a lot for working on this!
What does this patch apply to? It doesn't apply cleanly to either my
"main" branch or the "next" branch. Try to send things that apply to
"main" and if it needs to apply on top of something else, mention what
that is.
> 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;
1) This is not related to your patch, but I think "pcie_port" used to
make more sense before we had endpoint drivers, but now it's the
wrong name. Root Ports and Endpoints both have "PCIe Ports", but
IIUC "struct pcie_port" only applies to Root Ports, and "struct
dw_pcie_ep" is the analogue for Endpoints.
It would be nice to coordinate these names with a separate patch,
e.g., maybe "dw_pcie_rc" (or "dw_pcie_rp") and "dw_pcie_ep".
2) We allocate struct dra7xx_pcie for both RPs and EPs. But IIUC, RPs
only use "struct pcie_port", and EPs only use "struct dw_pcie_ep".
It doesn't seem right to keep both pointers when only one is ever
used.
3) I'm not sure why these should be pointers at all. Why can't they
be directly embedded, e.g., "struct pcie_port pp" instead of
"struct pcie_port *pp"? Obviously this would have to be done in a
way that we allocate an RC-specific structure or an EP-specific
one.
> void __iomem*base; /* DT ti_conf */
> int phy_count; /* DT phy-names count */
> struct phy **phy;
> @@ -796,6 +798,17 @@ static int __init dra7xx_pcie_probe(struct
> platform_device *pdev)
>
> switch (mode) {
> case DW_PCIE_RC_TYPE:
> + pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL);
We know "mode" right after the of_match_device() at the top of this
function. I think we should allocate the RC or EP structure way up
there, ideally with a single alloc for everything we need
(dra7xx_pcie, pcie_port, dw_pcie_ep, etc). That would be fewer allocs
and would simplify error handling because if the alloc fails we
wouldn't have to undo anything.
> + if (!pp) {
> + ret = -ENOMEM;
> + goto err_gpio;
> + }
> +
> + pci = >pcie;
> + pci->dev = dev;
> + pci->ops = _pcie_ops;
> + dra7xx->pp = pp;
> +
> if (!IS_ENABLED(CONFIG_PCI_DRA7XX_HOST)) {
> ret = -ENODEV;
> goto err_gpio;
> @@ -813,6 +826,17 @@ static int __init dra7xx_pcie_probe(struct
> platform_device *pdev)
> goto err_gpio;
> break;
> case DW_PCIE_EP_TYPE:
> + ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
> + if (!ep) {
> + ret = -ENOMEM;
> + goto err_gpio;
> + }
> +
> + pci = >pcie;
> + pci->dev = dev;
> + pci->ops = _pcie_ops;
> + dra7xx->ep = ep;
> +
> if (!IS_ENABLED(CONFIG_PCI_DRA7XX_EP)) {
> ret = -ENODEV;
> goto err_gpio;
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -171,12 +171,44 @@ enum dw_pcie_device_mode {
> DW_PCIE_RC_TYPE,
> };
>
> +struct dw_pcie_ops {
> + u64 (*cpu_addr_fixup)(struct dw_pcie *pcie, u64 cpu_addr);
> + u32 (*read_dbi)(struct dw_pcie *pcie, void __iomem *base, u32 reg,
> + size_t size);
> + void(*write_dbi)(struct dw_pcie *pcie, void __iomem *base, u32 reg,
> + size_t size, u32 val);
> + void(*write_dbi2)(struct dw_pcie *pcie, void __iomem *base, u32 reg,
> + size_t size, u32 val);
> + int (*link_up)(struct dw_pcie *pcie);
> + int
[PATCH] PCI: dwc: Change the inheritance between the abstracted structures
From: Hou Zhiqiang
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.
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
Cc: Kishon Vijay Abraham I
Cc: Lorenzo Pieralisi
Cc: Rob Herring
Cc: Bjorn Helgaas
Cc: Jingoo Han
Cc: Richard Zhu
Cc: Lucas Stach
Cc: Murali Karicheri
Cc: Minghuan Lian
Cc: Mingkai Hu
Cc: Roy Zang
Cc: Yue Wang
Cc: Jonathan Chocron
Cc: Thomas Petazzoni
Cc: Jesper Nilsson
Cc: Gustavo Pimentel
Cc: Xiaowei Song
Cc: Binghui Wang
Cc: Stanimir Varbanov
Cc: Pratyush Anand
Cc: Kunihiko Hayashi
Cc: Jason Yan
Cc: Thierry Reding
---
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(-)
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;
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 = >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);
+ }
+
+ dev = pci->dev;
reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN);
if (reg & ERR_SYS)
@@ -447,11 +456,10 @@ static int __init dra7xx_add_pcie_ep(struct dra7xx_pcie
*dra7xx,
struct platform_device *pdev)
{
int ret;
- struct dw_pcie_ep *ep;
+ struct dw_pcie_ep *ep = dra7xx->ep;
+ struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
struct device *dev = >dev;
- struct dw_pcie *pci = dra7xx->pci;
- ep = >ep;
ep->ops = _ep_ops;
pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "ep_dbics");
@@ -476,8 +484,8 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie
*dra7xx,
struct platform_device *pdev)
{
int ret;
- struct dw_pcie *pci = dra7xx->pci;
- struct pcie_port *pp = >pp;
+ struct pcie_port *pp = dra7xx->pp;
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
struct device *dev = pci->dev;
pp->irq = platform_get_irq(pdev, 1);
@@ -693,6 +701,8 @@ static int __init dra7xx_pcie_probe(struct platform_device
*pdev)
struct device_link **link;
void __iomem *base;
struct dw_pcie *pci;
+ struct pcie_port *pp;
+ struct dw_pcie_ep *ep;
struct dra7xx_pcie *dra7xx;
struct device *dev = >dev;
struct device_node *np =
RE: [PATCH] PCI: dwc: Change the inheritance between the abstracted structures
Hi Lorenzo and All,
Any comments on this patch?
Thanks,
Zhiqiang
> -Original Message-
> From: Z.q. Hou
> Sent: 2021年1月29日 17:40
> To: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org;
> lorenzo.pieral...@arm.com; r...@kernel.org; bhelg...@google.com
> Cc: Z.q. Hou ; Kishon Vijay Abraham I
> ; Jingoo Han ; Richard Zhu
> ; Lucas Stach ; Murali
> Karicheri ; M.h. Lian ;
> Mingkai Hu ; Roy Zang ; Yue
> Wang ; Jonathan Chocron
> ; Thomas Petazzoni
> ; Jesper Nilsson
> ; Gustavo Pimentel
> ; Xiaowei Song
> ; Binghui Wang ;
> Stanimir Varbanov ; Pratyush Anand
> ; Kunihiko Hayashi
> ; Jason Yan ;
> Thierry Reding
> Subject: [PATCH] PCI: dwc: Change the inheritance between the abstracted
> structures
>
> From: Hou Zhiqiang
>
> 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.
>
> 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
> Cc: Kishon Vijay Abraham I
> Cc: Lorenzo Pieralisi
> Cc: Rob Herring
> Cc: Bjorn Helgaas
> Cc: Jingoo Han
> Cc: Richard Zhu
> Cc: Lucas Stach
> Cc: Murali Karicheri
> Cc: Minghuan Lian
> Cc: Mingkai Hu
> Cc: Roy Zang
> Cc: Yue Wang
> Cc: Jonathan Chocron
> Cc: Thomas Petazzoni
> Cc: Jesper Nilsson
> Cc: Gustavo Pimentel
> Cc: Xiaowei Song
> Cc: Binghui Wang
> Cc: Stanimir Varbanov
> Cc: Pratyush Anand
> Cc: Kunihiko Hayashi
> Cc: Jason Yan
> Cc: Thierry Reding
> ---
> 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(-)
>
> 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;
> 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 = >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);
> + }
> +
> + dev = pci->dev;
> reg = dra7xx_pcie_readl(dra7xx,
> PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN);
>
> if (reg & ERR_SYS)
> @@ -447,11 +456,10 @@
