Re: [PATCH] pci: exynos: split into two parts such as Synopsys partand Exynos part

From: Pratyush Anand
Date: Fri Jul 05 2013 - 06:44:26 EST


On 7/5/2013 1:59 PM, Jingoo Han wrote:
Exynos PCIe IP consists of Synopsys specific part and Exynos
specific part. Only core block is a Synopsys designware part;
other parts are Exynos specific.
Also, the Synopsys designware part can be shared with other
platforms; thus, it can be split two parts such as Synopsys
designware part and Exynos specific part.


A quick and nice job :)
Just few minor comments.

Signed-off-by: Jingoo Han <jg1.han@xxxxxxxxxxx>
Cc: Pratyush Anand <pratyush.anand@xxxxxx>
Cc: Mohit KUMAR <Mohit.KUMAR@xxxxxx>
---
drivers/pci/host/Makefile | 1 +
drivers/pci/host/pcie-designware.c | 907 +++++++-----------------------------
drivers/pci/host/pcie-designware.h | 72 +++
drivers/pci/host/pcie-exynos.c | 619 ++++++++++++++++++++++++
4 files changed, 862 insertions(+), 737 deletions(-)
create mode 100644 drivers/pci/host/pcie-designware.h
create mode 100644 drivers/pci/host/pcie-exynos.c


[...]


-
-struct pcie_port {
- struct device *dev;
- u8 controller;
- u8 root_bus_nr;
- void __iomem *dbi_base;
- void __iomem *elbi_base;
- void __iomem *phy_base;
- void __iomem *purple_base;

Just for knowledge, what is the purple_base. It might not be needed by all vendors. Can we explain the name in comment or can give some generic name?

- u64 cfg0_base;
- void __iomem *va_cfg0_base;
- u64 cfg1_base;
- void __iomem *va_cfg1_base;
- u64 io_base;
- u64 mem_base;
- spinlock_t conf_lock;
- struct resource cfg;
- struct resource io;
- struct resource mem;
- struct pcie_port_info config;
- struct clk *clk;
- struct clk *bus_clk;
- int irq;
- int reset_gpio;
-};
-

[...]

-
-static inline void readl_rc(struct pcie_port *pp, void *dbi_base, u32 *val)
+static inline void dw_pcie_readl_rc(struct pcie_port *pp, void *dbi_base,
+ u32 *val)

dbi_base is part of pp. So why to pass 3 args?

{
- exynos_pcie_sideband_dbi_r_mode(pp, true);
- *val = readl(dbi_base);
- exynos_pcie_sideband_dbi_r_mode(pp, false);
- return;
+ if (pp->ops->readl_rc)
+ pp->ops->readl_rc(pp, dbi_base, val);

ditto

+ else
+ *val = readl(dbi_base);
}

-static inline void writel_rc(struct pcie_port *pp, u32 val, void *dbi_base)
+static inline void dw_pcie_writel_rc(struct pcie_port *pp, u32 val,
+ void *dbi_base)

ditto

{
- exynos_pcie_sideband_dbi_w_mode(pp, true);
- writel(val, dbi_base);
- exynos_pcie_sideband_dbi_w_mode(pp, false);
- return;
+ if (pp->ops->writel_rc)
+ pp->ops->writel_rc(pp, val, dbi_base);

ditto

+ else
+ writel(val, dbi_base);
}


Regards
Pratyush
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/