linux-kernel - Re: [PATCH V2] pci: exynos: split into two parts such as Synopsys part and Exynos part

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <000601ce8134$d5277b20$7f767160$@samsung.com>
Date:	Mon, 15 Jul 2013 17:25:15 +0900
From:	Jingoo Han <jg1.han@...sung.com>
To:	'Kishon Vijay Abraham I' <kishon@...com>
Cc:	'Bjorn Helgaas' <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
	linux-samsung-soc@...r.kernel.org,
	'Kukjin Kim' <kgene.kim@...sung.com>,
	'Pratyush Anand' <pratyush.anand@...com>,
	'Mohit KUMAR' <Mohit.KUMAR@...com>,
	'Arnd Bergmann' <arnd@...db.de>,
	'Sean Cross' <xobs@...agi.com>,
	'SRIKANTH TUMKUR SHIVANAND' <ts.srikanth@...sung.com>,
	linux-kernel@...r.kernel.org, Jingoo Han <jg1.han@...sung.com>
Subject: Re: [PATCH V2] pci: exynos: split into two parts such as Synopsys part
 and Exynos part

On Friday, July 12, 2013 7:01 PM, Kishon Vijay Abraham I wrote:
> On Thursday 11 July 2013 11:19 AM, 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.
> >
> > Signed-off-by: Jingoo Han <jg1.han@...sung.com>
> > Cc: Pratyush Anand <pratyush.anand@...com>
> > Cc: Mohit KUMAR <Mohit.KUMAR@...com>
> > ---
> > Changes since v1:
> > - moved the configuration, I/O, memory space handling to dw_pcie_host_init()
> > - removed exynos_pcie_abort()
> > - replaced 'purple_base' with 'block_base'
> > - replaced 'dbi_base' with 'dbi_addr'
> >
> >  drivers/pci/host/Makefile          |    1 +
> >  drivers/pci/host/pcie-designware.c |  963 +++++++++---------------------------
> >  drivers/pci/host/pcie-designware.h |   71 +++
> >  drivers/pci/host/pcie-exynos.c     |  523 ++++++++++++++++++++
> >  4 files changed, 822 insertions(+), 736 deletions(-)
> >  create mode 100644 drivers/pci/host/pcie-designware.h
> >  create mode 100644 drivers/pci/host/pcie-exynos.c
> >

[...]

> > -static void exynos_pcie_setup_rc(struct pcie_port *pp)
> > +void dw_pcie_setup_rc(struct pcie_port *pp)
> >  {
> >  	struct pcie_port_info *config = &pp->config;
> >  	void __iomem *dbi_base = pp->dbi_base;
> > @@ -549,509 +502,47 @@ static void exynos_pcie_setup_rc(struct pcie_port *pp)
> >  	u32 memlimit;
> >
> >  	/* set the number of lines as 4 */
> > -	readl_rc(pp, dbi_base + PCIE_PORT_LINK_CONTROL, &val);
> > +	dw_pcie_readl_rc(pp, dbi_base + PCIE_PORT_LINK_CONTROL, &val);
> >  	val &= ~PORT_LINK_MODE_MASK;
> >  	val |= PORT_LINK_MODE_4_LANES;
> > -	writel_rc(pp, val, dbi_base + PCIE_PORT_LINK_CONTROL);
> > +	dw_pcie_writel_rc(pp, val, dbi_base + PCIE_PORT_LINK_CONTROL);
> 
> I guess here we need to make this configurable. In Jacinto6 this can be either
> single lane or double lane. Maybe we should have a dt property to specify the
> number of lanes?

OK, I will make it configurable.

[...]

> > +struct pcie_port_info {
> > +	u32		cfg0_size;
> > +	u32		cfg1_size;
> > +	u32		io_size;
> > +	u32		mem_size;
> > +	phys_addr_t	io_bus_addr;
> > +	phys_addr_t	mem_bus_addr;
> > +};
> > +
> > +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		*block_base;
> > +	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;
> > +	struct dw_pcie_host_ops	*ops;
> > +};
> 
> I think this structure should be split. This has too many of platform specific
> fields. Maybe we should make pcie_port have only fields necessary for core part
> and have some other structure for platform specific part?
> 
> Something like
> struct exynos_pcie {
> 	[...]
> 	void __iomem		*dbi_base;
> 	void __iomem		*elbi_base;
> 	void __iomem		*phy_base;
> 	int			reset_gpio;
> 	struct clk		*clk;
> 	struct clk		*bus_clk;
> 	[...]
> 	struct pcie_port	pp;
> }
> 
> struct pcie_port {
> 	struct device		*dev;
> 	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;
> 	int			irq;
> 	struct dw_pcie_host_ops	*ops;
> };
> 
> And in ops, you can use container_of to get reference to exynos_pcie (if we
> want to write to exynos pcie registers)

OK, I see.
It looks good. I will use it as you guided.
Thank you for your suggestion. :)

Best regards,
Jingoo Han


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