pci: exynos: split into two parts such as Synopsys part and Exynos part
Subsystems (2)
PATCH 1/1 pci: exynos: split into two parts such as Synopsys part and Exynos part
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.
--- 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
drivers/pci/host/Makefile +1 -0
@@ -1,2 +1,3 @@ obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o obj-$(CONFIG_PCIE_DW) += pcie-designware.o +obj-$(CONFIG_PCI_EXYNOS) += pcie-exynos.o
drivers/pci/host/pcie-designware.c +170 -737
1,141 lines not on this page · raw message
drivers/pci/host/pcie-designware.h new file +72 -0
72 lines not on this page · raw message
drivers/pci/host/pcie-exynos.c new file +619 -0
619 lines not on this page · raw message
Signature · 1 line
1.7.10.4
Quoted 7 lines · Jingoo Han
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.
Quoted 12 lines
Signed-off-by Jingoo Han <[email protected]>Cc Pratyush Anand <[email protected]>Cc Mohit KUMAR <[email protected]>--- 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.hcreate mode 100644drivers/pci/host/pcie-exynos.c
[...]
drivers/pci/host/pcie-designware.c line 49
Quoted 9 lines
drivers/pci/host/pcie-designware.c +0
-9- -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?
Quoted 17 lines
drivers/pci/host/pcie-designware.c +0
-17-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; -}; -
[...]
drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 96
Quoted 4 lines
drivers/pci/host/pcie-designware.c +2
-2- -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?
drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 99
Quoted 7 lines
{drivers/pci/host/pcie-designware.c +2
-4-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
drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 105
Quoted 7 lines
drivers/pci/host/pcie-designware.c +4
-1+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
drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 108
Quoted 7 lines
{drivers/pci/host/pcie-designware.c +2
-4-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
drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 111
Quoted 3 lines
drivers/pci/host/pcie-designware.c +2
-0+else +writel (val, dbi_base); }
Regards
Pratyush
Quoted 11 lines · Pratyush Anand
Quoted 7 lines · Jingoo Han
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.
Thank you.
Without your comment, it could be done. :)
Quoted 17 lines
Quoted 13 lines
Signed-off-by Jingoo Han <[email protected]>Cc Pratyush Anand <[email protected]>Cc Mohit KUMAR <[email protected]>--- 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.hcreate mode 100644drivers/pci/host/pcie-exynos.c[...]
Quoted 13 lines
drivers/pci/host/pcie-designware.c line 49
Quoted 9 lines
drivers/pci/host/pcie-designware.c +0
-9- -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?
purple_base is a register to control PCIe block.
For example, controlling various reset control signals, PLL lock indication, etc.
Generic name would be 'block_base' or 'misc_base'?
Quoted 21 lines
Quoted 18 lines
drivers/pci/host/pcie-designware.c +0
-17-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; -}; -[...]
Quoted 6 lines
drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 96
Quoted 4 lines
drivers/pci/host/pcie-designware.c +2
-2- -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_baseis part of pp. So why to pass 3 args?
OK, I see.
I will use 2 args.
Best regards,
Jingoo Han
Quoted 10 lines · Jingoo Han
Quoted 9 lines · Pratyush Anand
Quoted 7 lines · Jingoo Han
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.
Quoted 5 lines
Quoted 2 lines
A quick and nice job :)
Just few minor comments.Thank you.
Without your comment, it could be done. :)
Oops, it is a typo.
Without your comment, it could 'NOT' be done. :)
Thank you for your comment.
It is very helpful.
Best regards,
Jingoo Han
Quoted 24 lines · Pratyush Anand
Quoted 7 lines · Jingoo Han
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.Quoted 12 lines
Signed-off-by Jingoo Han <[email protected]>Cc Pratyush Anand <[email protected]>Cc Mohit KUMAR <[email protected]>--- 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.hcreate mode 100644drivers/pci/host/pcie-exynos.c
[...]
Quoted 5 lines
drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 96
Quoted 3 lines
drivers/pci/host/pcie-designware.c +2
-1-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_baseis part of pp. So why to pass 3 args?
Sorry, this variable does not mean dbi_base;
it means 'dbi_base + offset'.
dw_pcie_{readl/writel}_rc() are called as below:
dw_pcie_readl_rc(pp, dbi_base + PCIE_PORT_LINK_CONTROL, &val); dw_pcie_writel_rc(pp, val, dbi_base + PCIE_PORT_LINK_CONTROL);
Thus, 'dbi_base' can be replaced with 'dbi_addr'.
Best regards,
Jingoo Han
Quoted 38 lines
drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 99
Quoted 8 lines
{drivers/pci/host/pcie-designware.c +2
-4-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
drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 105
Quoted 7 lines
drivers/pci/host/pcie-designware.c +4
-1+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
drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 108
Quoted 7 lines
{drivers/pci/host/pcie-designware.c +2
-4-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
drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 111
Quoted 3 lines
drivers/pci/host/pcie-designware.c +2
-0+else +writel (val, dbi_base); }Regards
Pratyush
Quoted 46 lines · Jingoo Han
drivers/pci/host/pcie-exynos.c new file +43
-0+ +/* PCIe ELBI registers */ +#define PCIE_IRQ_PULSE 0x000 +#define IRQ_INTA_ASSERT (0x1 << 0) +#define IRQ_INTB_ASSERT (0x1 << 2) +#define IRQ_INTC_ASSERT (0x1 << 4) +#define IRQ_INTD_ASSERT (0x1 << 6) +#define PCIE_IRQ_LEVEL 0x004 +#define PCIE_IRQ_SPECIAL 0x008 +#define PCIE_IRQ_EN_PULSE 0x00c +#define PCIE_IRQ_EN_LEVEL 0x010 +#define PCIE_IRQ_EN_SPECIAL 0x014 +#define PCIE_PWR_RESET 0x018 +#define PCIE_CORE_RESET 0x01c +#define PCIE_CORE_RESET_ENABLE (0x1 << 0) +#define PCIE_STICKY_RESET 0x020 +#define PCIE_NONSTICKY_RESET 0x024 +#define PCIE_APP_INIT_RESET 0x028 +#define PCIE_APP_LTSSM_ENABLE 0x02c +#define PCIE_ELBI_RDLH_LINKUP 0x064 +#define PCIE_ELBI_LTSSM_ENABLE 0x1 +#define PCIE_ELBI_SLV_AWMISC 0x11c +#define PCIE_ELBI_SLV_ARMISC 0x120 +#define PCIE_ELBI_SLV_DBI_ENABLE (0x1 << 21) + +/* PCIe Purple registers */ +#define PCIE_PHY_GLOBAL_RESET 0x000 +#define PCIE_PHY_COMMON_RESET 0x004 +#define PCIE_PHY_CMN_REG 0x008 +#define PCIE_PHY_MAC_RESET 0x00c +#define PCIE_PHY_PLL_LOCKED 0x010 +#define PCIE_PHY_TRSVREG_RESET 0x020 +#define PCIE_PHY_TRSV_RESET 0x024 + +/* PCIe PHY registers */ +#define PCIE_PHY_IMPEDANCE 0x004 +#define PCIE_PHY_PLL_DIV_0 0x008 +#define PCIE_PHY_PLL_BIAS 0x00c +#define PCIE_PHY_DCC_FEEDBACK 0x014 +#define PCIE_PHY_PLL_DIV_1 0x05c +#define PCIE_PHY_TRSV0_EMP_LVL 0x084 +#define PCIE_PHY_TRSV0_DRV_LVL 0x088 +#define PCIE_PHY_TRSV0_RXCDR 0x0ac
drivers/pci/host/pcie-exynos.c line 83
Quoted 10 lines
drivers/pci/host/pcie-exynos.c new file +10
-0+#define PCIE_PHY_TRSV0_LVCC 0x0dc +#define PCIE_PHY_TRSV1_EMP_LVL 0x144 +#define PCIE_PHY_TRSV1_RXCDR 0x16c +#define PCIE_PHY_TRSV1_LVCC 0x19c +#define PCIE_PHY_TRSV2_EMP_LVL 0x204 +#define PCIE_PHY_TRSV2_RXCDR 0x22c +#define PCIE_PHY_TRSV2_LVCC 0x25c +#define PCIE_PHY_TRSV3_EMP_LVL 0x2c4 +#define PCIE_PHY_TRSV3_RXCDR 0x2ec +#define PCIE_PHY_TRSV3_LVCC 0x31c
Are you sure these are all exynos specific? Maybe they are licensed from someone else?
Quoted 15 lines
drivers/pci/host/pcie-exynos.c new file +15
-0+ + pp->dbi_base =devm_ioremap (&pdev->dev, pp->cfg.start, +resource_size (&pp->cfg)); +if (!pp->dbi_base) { +dev_err (&pdev->dev,"error with ioremap\n" ); +return -ENOMEM ; + } + + pp->root_bus_nr =-1 ; + pp->ops = &exynos_pcie_host_ops; + +spin_lock_init (&pp->conf_lock); +dw_pcie_host_init (pp); + pp->va_cfg0_base =devm_ioremap (&pdev->dev, pp->cfg0_base, + pp->config.cfg0_size);
drivers/pci/host/pcie-exynos.c line 442
Quoted 10 lines
drivers/pci/host/pcie-exynos.c new file +10
-0+if (!pp->va_cfg0_base) { +dev_err (pp->dev,"error with ioremap in function\n" ); +return -ENOMEM ; + } + pp->va_cfg1_base =devm_ioremap (&pdev->dev, pp->cfg1_base, + pp->config.cfg1_size); +if (!pp->va_cfg1_base) { +dev_err (pp->dev,"error with ioremap\n" ); +return -ENOMEM ; + }
I think the configuration space handling should go into the dw_pcie_host_init function, as that part would be needed by all drivers.
Quoted 39 lines
drivers/pci/host/pcie-exynos.c new file +39
-0+static int __init exynos_pcie_probe (struct platform_device *pdev) +{ +struct pcie_port *pp; +struct device_node *np = pdev->dev.of_node; +struct of_pci_range range; +struct of_pci_range_parser parser; +int ret; + + pp =devm_kzalloc (&pdev->dev,sizeof (*pp),GFP_KERNEL ); +if (!pp) { +dev_err (&pdev->dev,"no memory for pcie port\n" ); +return -ENOMEM ; + } + + pp->dev = &pdev->dev; + +if (of_pci_range_parser_init (&parser, np)) { +dev_err (&pdev->dev,"missing ranges property\n" ); +return -EINVAL ; + } + +/* Get the I/O and memory ranges from DT */ +for_each_of_pci_range (&parser, &range) { +unsigned long restype = range.flags &IORESOURCE_TYPE_BITS ; +if (restype ==IORESOURCE_IO ) { +of_pci_range_to_resource (&range, np, &pp->io); + pp->io.name ="I/O" ; + pp->io.start =max_t (resource_size_t, +PCIBIOS_MIN_IO , + range.pci_addr + global_io_offset); + pp->io.end =min_t (resource_size_t, +IO_SPACE_LIMIT , + range.pci_addr + range.size + + global_io_offset); + pp->config.io_size =resource_size (&pp->io); + pp->config.io_bus_addr = range.pci_addr; + } +if (restype ==IORESOURCE_MEM ) { +of_pci_range_to_resource (&range, np, &pp->mem);
drivers/pci/host/pcie-exynos.c line 495
Quoted 10 lines
drivers/pci/host/pcie-exynos.c new file +10
-0+ pp->mem.name ="MEM" ; + pp->config.mem_size =resource_size (&pp->mem); + pp->config.mem_bus_addr = range.pci_addr; + } +if (restype ==0 ) { +of_pci_range_to_resource (&range, np, &pp->cfg); + pp->config.cfg0_size =resource_size (&pp->cfg)/2 ; + pp->config.cfg1_size =resource_size (&pp->cfg)/2 ; + } + }
Same for these: it would be better to split up the probe function here and move all of the above into the common code.
drivers/pci/host/pcie-exynos.c line 497
Quoted 1 line
drivers/pci/host/pcie-exynos.c new file +1
-0+ pp->reset_gpio =of_get_named_gpio (np,"reset-gpio" ,0 );
I assume the reset logic would need to remain exynos specific
Quoted 8 lines
drivers/pci/host/pcie-exynos.c new file +8
-0+ pp->clk =devm_clk_get (&pdev->dev,"pcie" ); +if (IS_ERR (pp->clk)) { +dev_err (&pdev->dev,"Failed to get pcie rc clock\n" ); +return PTR_ERR (pp->clk); + } + ret =clk_prepare_enable (pp->clk); +if (ret) +return ret;
drivers/pci/host/pcie-exynos.c line 516
Quoted 10 lines
drivers/pci/host/pcie-exynos.c new file +10
-0+ + pp->bus_clk =devm_clk_get (&pdev->dev,"pcie_bus" ); +if (IS_ERR (pp->bus_clk)) { +dev_err (&pdev->dev,"Failed to get pcie bus clock\n" ); + ret =PTR_ERR (pp->bus_clk); + goto fail_clk; + } + ret =clk_prepare_enable (pp->bus_clk); +if (ret) + goto fail_clk;
For the clocks, I think we can keep them in the common code, but make them optional, i.e. do not fail the probe function if the clocks don't exist, i.e. you get -ENOENT from devm_clk_get. We should however still fail here if devm_clk_get returns a different error.
I also noticed an existing bug in your code here: if the clock controller is not yet initialized, devm_clk_get() will return -EPROBE_DEFER, which is broken in combination with platform_driver_probe. In order to fix that, just change the code to use platform_driver_register so it has the option to retry the probe after the clock controller is initialized.
That should be a separate patch.
Quoted 26 lines
drivers/pci/host/pcie-exynos.c new file +26
-0+static int exynos_pcie_abort (unsigned long addr,unsigned int fsr, +struct pt_regs *regs) +{ +unsigned long pc =instruction_pointer (regs); +unsigned long instr = *(unsigned long *)pc; + +WARN_ONCE (1 ,"pcie abort\n" ); + +/* +* If the instruction being executed was a read, +* make it look like it read all-ones. +*/ +if ((instr &0x0c100000 ) ==0x04100000 ) { +int reg = (instr >>12 ) &15 ; +unsigned long val; + +if (instr &0x00400000 ) + val =255 ; +else + val =-1 ; + + regs->uregs[reg] = val; + regs->ARM_pc +=4 ; +return 0 ; + } +
drivers/pci/host/pcie-exynos.c line 602
Quoted 10 lines
drivers/pci/host/pcie-exynos.c new file +10
-0+if ((instr &0x0e100090 ) ==0x00100090 ) { +int reg = (instr >>12 ) &15 ; + + regs->uregs[reg] =-1 ; + regs->ARM_pc +=4 ; +return 0 ; + } + +return 1 ; +}
Another observation that is only partially related: the abort handler logic is not specific to either exynos nor synopsys. However it is specific to ARM and should not be part of a driver that is potentially architecture independent. I would recommend moving this to a separate file "arm-pci-abort.c" or something like that. I notice that a lot of the other ARM PCI implementations have the exact same code but also need to reset the abort in the bridge. Can you confirm that a) you don't need to tell the root port about the abort and b) the abort handler is actually required for exynos? How often do you run into this?
Arnd
Quoted 53 lines · Arnd Bergmann
Quoted 51 lines · Jingoo Han
drivers/pci/host/pcie-exynos.c new file +48
-0+ +/* PCIe ELBI registers */ +#define PCIE_IRQ_PULSE 0x000 +#define IRQ_INTA_ASSERT (0x1 << 0) +#define IRQ_INTB_ASSERT (0x1 << 2) +#define IRQ_INTC_ASSERT (0x1 << 4) +#define IRQ_INTD_ASSERT (0x1 << 6) +#define PCIE_IRQ_LEVEL 0x004 +#define PCIE_IRQ_SPECIAL 0x008 +#define PCIE_IRQ_EN_PULSE 0x00c +#define PCIE_IRQ_EN_LEVEL 0x010 +#define PCIE_IRQ_EN_SPECIAL 0x014 +#define PCIE_PWR_RESET 0x018 +#define PCIE_CORE_RESET 0x01c +#define PCIE_CORE_RESET_ENABLE (0x1 << 0) +#define PCIE_STICKY_RESET 0x020 +#define PCIE_NONSTICKY_RESET 0x024 +#define PCIE_APP_INIT_RESET 0x028 +#define PCIE_APP_LTSSM_ENABLE 0x02c +#define PCIE_ELBI_RDLH_LINKUP 0x064 +#define PCIE_ELBI_LTSSM_ENABLE 0x1 +#define PCIE_ELBI_SLV_AWMISC 0x11c +#define PCIE_ELBI_SLV_ARMISC 0x120 +#define PCIE_ELBI_SLV_DBI_ENABLE (0x1 << 21) + +/* PCIe Purple registers */ +#define PCIE_PHY_GLOBAL_RESET 0x000 +#define PCIE_PHY_COMMON_RESET 0x004 +#define PCIE_PHY_CMN_REG 0x008 +#define PCIE_PHY_MAC_RESET 0x00c +#define PCIE_PHY_PLL_LOCKED 0x010 +#define PCIE_PHY_TRSVREG_RESET 0x020 +#define PCIE_PHY_TRSV_RESET 0x024 + +/* PCIe PHY registers */ +#define PCIE_PHY_IMPEDANCE 0x004 +#define PCIE_PHY_PLL_DIV_0 0x008 +#define PCIE_PHY_PLL_BIAS 0x00c +#define PCIE_PHY_DCC_FEEDBACK 0x014 +#define PCIE_PHY_PLL_DIV_1 0x05c +#define PCIE_PHY_TRSV0_EMP_LVL 0x084 +#define PCIE_PHY_TRSV0_DRV_LVL 0x088 +#define PCIE_PHY_TRSV0_RXCDR 0x0ac +#define PCIE_PHY_TRSV0_LVCC 0x0dc +#define PCIE_PHY_TRSV1_EMP_LVL 0x144 +#define PCIE_PHY_TRSV1_RXCDR 0x16c +#define PCIE_PHY_TRSV1_LVCC 0x19c +#define PCIE_PHY_TRSV2_EMP_LVL 0x204
Quoted 8 lines
drivers/pci/host/pcie-exynos.c line 83
Quoted 5 lines
drivers/pci/host/pcie-exynos.c new file +5
-0+#define PCIE_PHY_TRSV2_RXCDR 0x22c +#define PCIE_PHY_TRSV2_LVCC 0x25c +#define PCIE_PHY_TRSV3_EMP_LVL 0x2c4 +#define PCIE_PHY_TRSV3_RXCDR 0x2ec +#define PCIE_PHY_TRSV3_LVCC 0x31cAre you sure these are all exynos specific? Maybe they are licensed from someone else?
Samsung specific.
I asked our hardware engineer, and he confirmed it.
Quoted 21 lines
Quoted 21 lines
drivers/pci/host/pcie-exynos.c new file +20
-0+ + pp->dbi_base =devm_ioremap (&pdev->dev, pp->cfg.start, +resource_size (&pp->cfg)); +if (!pp->dbi_base) { +dev_err (&pdev->dev,"error with ioremap\n" ); +return -ENOMEM ; + } + + pp->root_bus_nr =-1 ; + pp->ops = &exynos_pcie_host_ops; + +spin_lock_init (&pp->conf_lock); +dw_pcie_host_init (pp); + pp->va_cfg0_base =devm_ioremap (&pdev->dev, pp->cfg0_base, + pp->config.cfg0_size); +if (!pp->va_cfg0_base) { +dev_err (pp->dev,"error with ioremap in function\n" ); +return -ENOMEM ; + } + pp->va_cfg1_base = devm_ioremap(&pdev->dev, pp->cfg1_base,
Quoted 8 lines
drivers/pci/host/pcie-exynos.c line 442
Quoted 5 lines
drivers/pci/host/pcie-exynos.c new file +5
-0+ pp->config.cfg1_size); +if (!pp->va_cfg1_base) { +dev_err (pp->dev,"error with ioremap\n" ); +return -ENOMEM ; + }I think the configuration space handling should go into the
dw_pcie_host_initfunction, as that part would be needed by all drivers.
OK.
I will move it to dw_pcie_host_init().
Quoted 45 lines
Quoted 45 lines
drivers/pci/host/pcie-exynos.c new file +44
-0+static int __init exynos_pcie_probe (struct platform_device *pdev) +{ +struct pcie_port *pp; +struct device_node *np = pdev->dev.of_node; +struct of_pci_range range; +struct of_pci_range_parser parser; +int ret; + + pp =devm_kzalloc (&pdev->dev,sizeof (*pp),GFP_KERNEL ); +if (!pp) { +dev_err (&pdev->dev,"no memory for pcie port\n" ); +return -ENOMEM ; + } + + pp->dev = &pdev->dev; + +if (of_pci_range_parser_init (&parser, np)) { +dev_err (&pdev->dev,"missing ranges property\n" ); +return -EINVAL ; + } + +/* Get the I/O and memory ranges from DT */ +for_each_of_pci_range (&parser, &range) { +unsigned long restype = range.flags &IORESOURCE_TYPE_BITS ; +if (restype ==IORESOURCE_IO ) { +of_pci_range_to_resource (&range, np, &pp->io); + pp->io.name ="I/O" ; + pp->io.start =max_t (resource_size_t, +PCIBIOS_MIN_IO , + range.pci_addr + global_io_offset); + pp->io.end =min_t (resource_size_t, +IO_SPACE_LIMIT , + range.pci_addr + range.size + + global_io_offset); + pp->config.io_size =resource_size (&pp->io); + pp->config.io_bus_addr = range.pci_addr; + } +if (restype ==IORESOURCE_MEM ) { +of_pci_range_to_resource (&range, np, &pp->mem); + pp->mem.name ="MEM" ; + pp->config.mem_size =resource_size (&pp->mem); + pp->config.mem_bus_addr = range.pci_addr; + } +if (restype ==0 ) {
Quoted 8 lines
drivers/pci/host/pcie-exynos.c line 495
Quoted 5 lines
drivers/pci/host/pcie-exynos.c new file +5
-0+of_pci_range_to_resource (&range, np, &pp->cfg); + pp->config.cfg0_size =resource_size (&pp->cfg)/2 ; + pp->config.cfg1_size =resource_size (&pp->cfg)/2 ; + } + }Same for these: it would be better to split up the probe function here and move all of the above into the common code.
OK.
I will move it to dw_pcie_host_init().
Quoted 29 lines
drivers/pci/host/pcie-exynos.c line 497
Quoted 2 lines
drivers/pci/host/pcie-exynos.c new file +1
-0+ pp->reset_gpio =of_get_named_gpio (np,"reset-gpio" ,0 );I assume the reset logic would need to remain exynos specific
Quoted 8 lines
drivers/pci/host/pcie-exynos.c new file +8
-0+ pp->clk =devm_clk_get (&pdev->dev,"pcie" ); +if (IS_ERR (pp->clk)) { +dev_err (&pdev->dev,"Failed to get pcie rc clock\n" ); +return PTR_ERR (pp->clk); + } + ret =clk_prepare_enable (pp->clk); +if (ret) +return ret;drivers/pci/host/pcie-exynos.c line 516
Quoted 10 lines
drivers/pci/host/pcie-exynos.c new file +10
-0+ + pp->bus_clk =devm_clk_get (&pdev->dev,"pcie_bus" ); +if (IS_ERR (pp->bus_clk)) { +dev_err (&pdev->dev,"Failed to get pcie bus clock\n" ); + ret =PTR_ERR (pp->bus_clk); + goto fail_clk; + } + ret =clk_prepare_enable (pp->bus_clk); +if (ret) + goto fail_clk;For the clocks, I think we can keep them in the common code, but make them optional, i.e. do not fail the probe function if the clocks don't exist, i.e. you get
-ENOENTfromdevm_clk_get. We should however still fail here ifdevm_clk_getreturns a different error.
Quoted 7 lines
I also noticed an existing bug in your code here: if the clock controller is not yet initialized,
devm_clk_get()will return -EPROBE_DEFER, which is broken in combination withplatform_driver_probe. In order to fix that, just change the code to useplatform_driver_registerso it has the option to retry the probe after the clock controller is initialized.
That should be a separate patch.
If I should do it, I will send a separate patch.
Quoted 39 lines
Quoted 27 lines
drivers/pci/host/pcie-exynos.c new file +26
-0+static int exynos_pcie_abort (unsigned long addr,unsigned int fsr, +struct pt_regs *regs) +{ +unsigned long pc =instruction_pointer (regs); +unsigned long instr = *(unsigned long *)pc; + +WARN_ONCE (1 ,"pcie abort\n" ); + +/* +* If the instruction being executed was a read, +* make it look like it read all-ones. +*/ +if ((instr &0x0c100000 ) ==0x04100000 ) { +int reg = (instr >>12 ) &15 ; +unsigned long val; + +if (instr &0x00400000 ) + val =255 ; +else + val =-1 ; + + regs->uregs[reg] = val; + regs->ARM_pc +=4 ; +return 0 ; + } +drivers/pci/host/pcie-exynos.c line 602
Quoted 10 lines
drivers/pci/host/pcie-exynos.c new file +10
-0+if ((instr &0x0e100090 ) ==0x00100090 ) { +int reg = (instr >>12 ) &15 ; + + regs->uregs[reg] =-1 ; + regs->ARM_pc +=4 ; +return 0 ; + } + +return 1 ; +}Another observation that is only partially related: the abort handler logic
Quoted 8 lines
is not specific to either exynos nor synopsys. However it is specific to ARM and should not be part of a driver that is potentially architecture independent. I would recommend moving this to a separate file "
arm-pci-abort.c" or something like that. I notice that a lot of the other ARM PCI implementations have the exact same code but also need to reset the abort in the bridge. Can you confirm that a) you don't need to tell the root port about the abort and b) the abort handler is actually required for exynos? How often do you run into this?
There is no special reason to add it.
I just referenced other ARM PCI drivers. Also, I have never seen calling this abort function.
Then, I will remove it.
Thank you for your opinion. :) Your comment is very helpful.
Best regards,
Jingoo Han
Hi,
Quoted 6 lines · Jingoo Han
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.
Thanks for doing that :-)
I'll be using the synopsys specific part as Jacinto6 also uses the same pcie core. Once I start implementing, I'll have some queries and comments ;-)
Cheers
Kishon
On Wednesday, July 10, 2013 11:02 PM, Kishon Vijay Abraham I:
Quoted 8 lines · Jingoo Han
Quoted 6 lines
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.
Quoted 4 lines
Thanks for doing that :-)
I'll be using the synopsys specific part as Jacinto6 also uses the same pcie core. Once I start implementing, I'll have some queries and comments ;-)
Hi Kishon,
OK, I see.
I will send v2 patch.
Also, I will be CC'ing you. :)
Best regards,
Jingoo Han