Re: [PATCH] pci: exynos: split into two parts such as Synopsys part and Exynos part
On Wednesday, July 10, 2013 11:02 PM, Kishon Vijay Abraham I: > On Friday 05 July 2013 01: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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pci: exynos: split into two parts such as Synopsys part and Exynos part
Hi, On Friday 05 July 2013 01: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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pci: exynos: split into two parts such as Synopsys part and Exynos part
Hi, On Friday 05 July 2013 01: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. 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pci: exynos: split into two parts such as Synopsys part and Exynos part
On Wednesday, July 10, 2013 11:02 PM, Kishon Vijay Abraham I: On Friday 05 July 2013 01: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. 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pci: exynos: split into two parts such as Synopsys part and Exynos part
On Friday, July 05, 2013 7:46 PM, Arnd Bergmann wrote:
> On Friday 05 July 2013, Jingoo Han wrote:
>
> > --- /dev/null
> > +++ b/drivers/pci/host/pcie-exynos.c
>
> > +
> > +/* 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_SPECIAL0x014
> > +#define PCIE_PWR_RESET 0x018
> > +#define PCIE_CORE_RESET0x01c
> > +#define PCIE_CORE_RESET_ENABLE (0x1 << 0)
> > +#define PCIE_STICKY_RESET 0x020
> > +#define PCIE_NONSTICKY_RESET 0x024
> > +#define PCIE_APP_INIT_RESET0x028
> > +#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_LOCKED0x010
> > +#define PCIE_PHY_TRSVREG_RESET 0x020
> > +#define PCIE_PHY_TRSV_RESET0x024
> > +
> > +/* 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_LVCC0x0dc
> > +#define PCIE_PHY_TRSV1_EMP_LVL 0x144
> > +#define PCIE_PHY_TRSV1_RXCDR 0x16c
> > +#define PCIE_PHY_TRSV1_LVCC0x19c
> > +#define PCIE_PHY_TRSV2_EMP_LVL 0x204
> > +#define PCIE_PHY_TRSV2_RXCDR 0x22c
> > +#define PCIE_PHY_TRSV2_LVCC0x25c
> > +#define PCIE_PHY_TRSV3_EMP_LVL 0x2c4
> > +#define PCIE_PHY_TRSV3_RXCDR 0x2ec
> > +#define PCIE_PHY_TRSV3_LVCC0x31c
>
> Are 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.
>
> > +
> > + pp->dbi_base = devm_ioremap(>dev, pp->cfg.start,
> > + resource_size(>cfg));
> > + if (!pp->dbi_base) {
> > + dev_err(>dev, "error with ioremap\n");
> > + return -ENOMEM;
> > + }
> > +
> > + pp->root_bus_nr = -1;
> > + pp->ops = _pcie_host_ops;
> > +
> > + spin_lock_init(>conf_lock);
> > + dw_pcie_host_init(pp);
> > + pp->va_cfg0_base = devm_ioremap(>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(>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.
OK.
I will move it to dw_pcie_host_init().
>
> > +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(>dev, sizeof(*pp), GFP_KERNEL);
> > + if (!pp) {
> > + dev_err(>dev, "no memory for pcie port\n");
> > + return -ENOMEM;
> > + }
> > +
> > + pp->dev = >dev;
> > +
> > + if (of_pci_range_parser_init(, np)) {
> > + dev_err(>dev, "missing ranges property\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* Get the I/O and memory ranges from DT */
> > + for_each_of_pci_range(, ) {
> > + unsigned long restype = range.flags & IORESOURCE_TYPE_BITS;
> > + if (restype == IORESOURCE_IO) {
> > + of_pci_range_to_resource(, np, >io);
> > + pp->io.name = "I/O";
> > +
Re: [PATCH] pci: exynos: split into two parts such as Synopsys part and Exynos part
On Friday, July 05, 2013 7:44 PM, Pratyush Anand wrote:
> 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
> > Cc: Pratyush Anand
> > Cc: Mohit KUMAR
> > ---
> > 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
> >
[...]
> > -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?
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, );
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
>
> > {
> > - 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pci: exynos: split into two parts such as Synopsys part and Exynos part
On Friday, July 05, 2013 7:44 PM, Pratyush Anand wrote:
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@samsung.com
Cc: Pratyush Anand pratyush.an...@st.com
Cc: Mohit KUMAR mohit.ku...@st.com
---
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
[...]
-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?
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
{
- 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pci: exynos: split into two parts such as Synopsys part and Exynos part
On Friday, July 05, 2013 7:46 PM, Arnd Bergmann wrote:
On Friday 05 July 2013, Jingoo Han wrote:
--- /dev/null
+++ b/drivers/pci/host/pcie-exynos.c
+
+/* 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_SPECIAL0x014
+#define PCIE_PWR_RESET 0x018
+#define PCIE_CORE_RESET0x01c
+#define PCIE_CORE_RESET_ENABLE (0x1 0)
+#define PCIE_STICKY_RESET 0x020
+#define PCIE_NONSTICKY_RESET 0x024
+#define PCIE_APP_INIT_RESET0x028
+#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_LOCKED0x010
+#define PCIE_PHY_TRSVREG_RESET 0x020
+#define PCIE_PHY_TRSV_RESET0x024
+
+/* 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_LVCC0x0dc
+#define PCIE_PHY_TRSV1_EMP_LVL 0x144
+#define PCIE_PHY_TRSV1_RXCDR 0x16c
+#define PCIE_PHY_TRSV1_LVCC0x19c
+#define PCIE_PHY_TRSV2_EMP_LVL 0x204
+#define PCIE_PHY_TRSV2_RXCDR 0x22c
+#define PCIE_PHY_TRSV2_LVCC0x25c
+#define PCIE_PHY_TRSV3_EMP_LVL 0x2c4
+#define PCIE_PHY_TRSV3_RXCDR 0x2ec
+#define PCIE_PHY_TRSV3_LVCC0x31c
Are 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.
+
+ 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,
+ 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.
OK.
I will move it to dw_pcie_host_init().
+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);
+
Re: [PATCH] pci: exynos: split into two parts such as Synopsys part and Exynos part
On Monday, July 08, 2013 2:06 PM, Jingoo Han wrote: > On Friday, July 05, 2013 7:44 PM, Pratyush Anand wrote: > > 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pci: exynos: split into two parts such as Synopsys part and Exynos part
On Friday, July 05, 2013 7:44 PM, Pratyush Anand wrote:
> 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.
Thank you.
Without your comment, it could be done. :)
>
> > Signed-off-by: Jingoo Han
> > Cc: Pratyush Anand
> > Cc: Mohit KUMAR
> > ---
> > 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?
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'?
>
> > - 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?
OK, I see.
I will use 2 args.
Best regards,
Jingoo Han
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pci: exynos: split into two parts such as Synopsys part and Exynos part
On Friday, July 05, 2013 7:44 PM, Pratyush Anand wrote:
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.
Thank you.
Without your comment, it could be done. :)
Signed-off-by: Jingoo Han jg1@samsung.com
Cc: Pratyush Anand pratyush.an...@st.com
Cc: Mohit KUMAR mohit.ku...@st.com
---
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?
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'?
- 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?
OK, I see.
I will use 2 args.
Best regards,
Jingoo Han
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pci: exynos: split into two parts such as Synopsys part and Exynos part
On Monday, July 08, 2013 2:06 PM, Jingoo Han wrote: On Friday, July 05, 2013 7:44 PM, Pratyush Anand wrote: 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. 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pci: exynos: split into two parts such as Synopsys part and Exynos part
On Friday 05 July 2013, Jingoo Han wrote:
> --- /dev/null
> +++ b/drivers/pci/host/pcie-exynos.c
> +
> +/* 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_PULSE0x00c
> +#define PCIE_IRQ_EN_LEVEL0x010
> +#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_RESET0x020
> +#define PCIE_NONSTICKY_RESET 0x024
> +#define PCIE_APP_INIT_RESET 0x028
> +#define PCIE_APP_LTSSM_ENABLE0x02c
> +#define PCIE_ELBI_RDLH_LINKUP0x064
> +#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_RESET0x000
> +#define PCIE_PHY_COMMON_RESET0x004
> +#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_BIAS0x00c
> +#define PCIE_PHY_DCC_FEEDBACK0x014
> +#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
> +#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?
> +
> + pp->dbi_base = devm_ioremap(>dev, pp->cfg.start,
> + resource_size(>cfg));
> + if (!pp->dbi_base) {
> + dev_err(>dev, "error with ioremap\n");
> + return -ENOMEM;
> + }
> +
> + pp->root_bus_nr = -1;
> + pp->ops = _pcie_host_ops;
> +
> + spin_lock_init(>conf_lock);
> + dw_pcie_host_init(pp);
> + pp->va_cfg0_base = devm_ioremap(>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(>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.
> +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(>dev, sizeof(*pp), GFP_KERNEL);
> + if (!pp) {
> + dev_err(>dev, "no memory for pcie port\n");
> + return -ENOMEM;
> + }
> +
> + pp->dev = >dev;
> +
> + if (of_pci_range_parser_init(, np)) {
> + dev_err(>dev, "missing ranges property\n");
> + return -EINVAL;
> + }
> +
> + /* Get the I/O and memory ranges from DT */
> + for_each_of_pci_range(, ) {
> + unsigned long restype = range.flags & IORESOURCE_TYPE_BITS;
> + if (restype == IORESOURCE_IO) {
> + of_pci_range_to_resource(, np, >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,
> +
Re: [PATCH] pci: exynos: split into two parts such as Synopsys part and Exynos part
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
Cc: Pratyush Anand
Cc: Mohit KUMAR
---
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pci: exynos: split into two parts such as Synopsys part and Exynos part
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@samsung.com
Cc: Pratyush Anand pratyush.an...@st.com
Cc: Mohit KUMAR mohit.ku...@st.com
---
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pci: exynos: split into two parts such as Synopsys part and Exynos part
On Friday 05 July 2013, Jingoo Han wrote:
--- /dev/null
+++ b/drivers/pci/host/pcie-exynos.c
+
+/* 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_PULSE0x00c
+#define PCIE_IRQ_EN_LEVEL0x010
+#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_RESET0x020
+#define PCIE_NONSTICKY_RESET 0x024
+#define PCIE_APP_INIT_RESET 0x028
+#define PCIE_APP_LTSSM_ENABLE0x02c
+#define PCIE_ELBI_RDLH_LINKUP0x064
+#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_RESET0x000
+#define PCIE_PHY_COMMON_RESET0x004
+#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_BIAS0x00c
+#define PCIE_PHY_DCC_FEEDBACK0x014
+#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
+#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?
+
+ 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,
+ 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.
+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);
+
