[PATCH 3/4] PCI: Add driver for i.MX6 PCI Express

On Tuesday 02 July 2013 10:11:55 Pratyush Anand wrote:

On 7/2/2013 9:16 AM, Sean Cross wrote:

quoted

quoted

quoted

I may be wrong, but from these offset it seems to me that it is SNPS
controller. If yes, then please go through comments of
"[PATCH V1-10 0/4] PCIe support for Samsung Exynos5440 SoC"

Exynos5440 appears to use the same port logic controller.  However, the PHYs are different.  I'm not exactly certain which comments you want me to notice in that series of patchsets, but I see references to splitting Exynos-specific code into its own project.  Based on that, it seems like the best approach would be to:

     1) Move Exynos code into its own file, say, pcie-exynos.c.  This would leave only Synopsys-specific ATC code in pcie-designware.c
     2) Rename generic exynos functions to reflect the fact that they're designware-generic functions.

Ouch, I missed the fact that Jingoo Han had only renamed the file, but not
also all the function identifiers. This change needs to be done anyway.

quoted

     3) Have pcie-imx.c reference this generic designware ATC code.

I'll rework the patch and re-submit it following these three changes.

Correct, Exactly these steps has to be done. But, Mohit might be doing 
similar work, so it would be better to get consensus on what has to be done.

If the platform specific parts are small enough, you can actually just leave
everything in one file, and use the of_device_id data field to point
to a structure describing the differences, e.g. using function pointers.
 

IMO, there should be three categories of functions. May be arnd can 
comment if we can do even something better.

As a general comment in case you are wondering: In a situation like this,
the common code should always be structured as a "library" of functions
and the hardware specific drivers become the actual driver that register
to a particular device 'compatible' string in DT.

Group I: Initially, These functions should remain in pcie-designware.c 
(offcourse by changing exynos tag to dw). Latter on, we can see if some 
of them can even be moved to common pci layer.

sys_to_pcie
cfg_read
cfg_write
dw_pcie_prog_viewport_cfg0
dw_pcie_prog_viewport_cfg1
dw_pcie_prog_viewport_mem_outbound
dw_pcie_prog_viewport_io_outbound
dw_pcie_rd_other_conf
dw_pcie_wr_other_conf
dw_pcie_setup
dw_pcie_valid_config
dw_pcie_rd_conf
dw_pcie_wd_conf
dw_pcie_scan_bus
dw_pcie_map_irq
dw_pcie_setup_rc
add_pcie_port (after a bit of generalization)
dw_pcie_probe
dw_pcie_remove

The probe and remove functions might need be split up into a generic
part that gets called by the hardware specific part.

Group II: These functions should still remain as dummy in 
pcie-designware.c , and should be classified as __week. So, each 
implementer of designware controller say Exynos/SPEAr/imx will have to 
define their own function to super-seed default dummy definitions.

dw_readl_rc
dw_writel_rc
dw_pcie_rd_own_conf
dw_pcie_wr_own_conf
dw_pcie_link_up
dw_pcie_host_init (will contain all platform specific and phy 
initialization)

I don't like __weak functions really, and they don't work well with loadable
modules the way I suggested above. Instead, you probably need a structure
with function pointers that gets initialized by platform specific
driver.

	struct dw_pcie_host_ops {
		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);
{
	if (pp->ops->readl_rc)
		pp->ops->readl_rc(pp, dbi_base, val);
	else
		*val = readl(dbi_base);
}

dw_pcie_host_init would become the probe function that calls the generic
dw_pcie_probe.

Group III: Functions specific to Exynos, which should move to pcie-exynos.c

exynos_pcie_sideband_dbi_w_mode
exynos_pcie_sideband_dbi_r_mode
exynos_pcie_assert_core_reset
exynos_pcie_deassert_core_reset
exynos_pcie_assert_phy_reset
exynos_pcie_deassert_phy_reset
exynos_pcie_init_phy
exynos_pcie_assert_reset
exynos_pcie_establish_link


@Mohit, See if you have BW then please take it further.

arnd, are exynos patches applied to any branch of arm-soc git tree?

Yes, they are currently in the next/soc branch.

	Arnd