On Tuesday, July 23, 2013 12:04 AM, Kishon Vijay Abraham I wrote:
> On Thursday 18 July 2013 10:51 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.
>
> some more queries and comments..
Hi Kishon,
Thank you for your comments. :)
Hi Pratyush Anand,
Please, answer one question mentioned below. :)
> >
> > Signed-off-by: Jingoo Han <jg1.han@xxxxxxxxxxx>
> > Cc: Pratyush Anand <pratyush.anand@xxxxxx>
> > Cc: Mohit KUMAR <Mohit.KUMAR@xxxxxx>
> > ---
> > Changes since v2:
> .
> .
> <snip>
> .
> .
> > +
> > +static struct pcie_host_ops exynos_pcie_host_ops = {
> > + .readl_rc = exynos_pcie_readl_rc,
> > + .writel_rc = exynos_pcie_writel_rc,
> > + .rd_own_conf = exynos_pcie_rd_own_conf,
> > + .wr_own_conf = exynos_pcie_wr_own_conf,
> > + .link_up = exynos_pcie_link_up,
> > + .host_init = exynos_pcie_host_init,
> > +};
> > +
> > +static int add_pcie_port(struct pcie_port *pp, struct platform_device *pdev)
> > +{
> > + struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp);
>
> We can move the exynos_pcie specific initialization to probe and leave only
> pcie_port initialization here.
OK, I will move the exynos_pcie specific initialization to probe
as you mentioned.
> > + struct resource *elbi_base;
> > + struct resource *phy_base;
> > + struct resource *block_base;
> > + int ret;
> > +
> > + elbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!elbi_base) {
> > + dev_err(&pdev->dev, "couldn't get elbi base resource\n");
> > + return -EINVAL;
> > + }
> > + exynos_pcie->elbi_base = devm_ioremap_resource(&pdev->dev, elbi_base);
> > + if (IS_ERR(exynos_pcie->elbi_base))
> > + return PTR_ERR(exynos_pcie->elbi_base);
> > +
> > + phy_base = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + if (!phy_base) {
> > + dev_err(&pdev->dev, "couldn't get phy base resource\n");
> > + return -EINVAL;
> > + }
> > + exynos_pcie->phy_base = devm_ioremap_resource(&pdev->dev, phy_base);
> > + if (IS_ERR(exynos_pcie->phy_base))
> > + return PTR_ERR(exynos_pcie->phy_base);
> > +
> > + block_base = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> > + if (!block_base) {
> > + dev_err(&pdev->dev, "couldn't get block base resource\n");
> > + return -EINVAL;
> > + }
> > + exynos_pcie->block_base = devm_ioremap_resource(&pdev->dev, block_base);
> > + if (IS_ERR(exynos_pcie->block_base))
> > + return PTR_ERR(exynos_pcie->block_base);
>
> So all this till here can be moved to probe.
As above mentioned, I will move it to probe.
> > +
> > + pp->irq = platform_get_irq(pdev, 1);
> > + if (!pp->irq) {
> > + dev_err(&pdev->dev, "failed to get irq\n");
> > + return -ENODEV;
> > + }
> > + ret = devm_request_irq(&pdev->dev, pp->irq, exynos_pcie_irq_handler,
> > + IRQF_SHARED, "exynos-pcie", pp);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to request irq\n");
> > + return ret;
> > + }
> > +
> > + pp->root_bus_nr = -1;
> > + pp->ops = &exynos_pcie_host_ops;
> > +
> > + spin_lock_init(&pp->conf_lock);
> > + ret = dw_pcie_host_init(pp);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to initialize host\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int __init exynos_pcie_probe(struct platform_device *pdev)
> > +{
> > + struct exynos_pcie *exynos_pcie;
> > + struct pcie_port *pp;
> > + struct device_node *np = pdev->dev.of_node;
> > + int ret;
> > +
> > + exynos_pcie = devm_kzalloc(&pdev->dev, sizeof(*exynos_pcie),
> > + GFP_KERNEL);
> > + if (!exynos_pcie) {
> > + dev_err(&pdev->dev, "no memory for exynos pcie\n");
> > + return -ENOMEM;
> > + }
> > +
> > + pp = &exynos_pcie->pp;
> > +
> > + pp->dev = &pdev->dev;
> > +
> > + exynos_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> > +
> > + exynos_pcie->clk = devm_clk_get(&pdev->dev, "pcie");
> > + if (IS_ERR(exynos_pcie->clk)) {
> > + dev_err(&pdev->dev, "Failed to get pcie rc clock\n");
> > + return PTR_ERR(exynos_pcie->clk);
> > + }
> > + ret = clk_prepare_enable(exynos_pcie->clk);
> > + if (ret)
> > + return ret;
> > +
> > + exynos_pcie->bus_clk = devm_clk_get(&pdev->dev, "pcie_bus");
> > + if (IS_ERR(exynos_pcie->bus_clk)) {
> > + dev_err(&pdev->dev, "Failed to get pcie bus clock\n");
> > + ret = PTR_ERR(exynos_pcie->bus_clk);
> > + goto fail_clk;
> > + }
> > + ret = clk_prepare_enable(exynos_pcie->bus_clk);
> > + if (ret)
> > + goto fail_clk;
> > +
> > + ret = add_pcie_port(pp, pdev);
> > + if (ret < 0)
> > + goto fail_bus_clk;
>
> I think we should move all the below code to designware core file. IMO it
> should be common everyone who use designware core.
OK, I will move the below code to dw_pcie_host_init() in pcie-designware.c
> > +
> > + dw_pci.nr_controllers = 1;
> > + dw_pci.private_data = (void **)&pp;
> > +
> > + pci_common_init(&dw_pci);
> > + pci_assign_unassigned_resources();
> > +#ifdef CONFIG_PCI_DOMAINS
> > + dw_pci.domain++;
> > +#endif
> > +
> > + platform_set_drvdata(pdev, exynos_pcie);
> > + return 0;
> > +
> > +fail_bus_clk:
> > + clk_disable_unprepare(exynos_pcie->bus_clk);
> > +fail_clk:
> > + clk_disable_unprepare(exynos_pcie->clk);
> > + return ret;
> > +}
> > +
[.....]
> > +int dw_pcie_host_init(struct pcie_port *pp)
> > +{
> > + struct device_node *np = pp->dev->of_node;
> > + struct of_pci_range range;
> > + struct of_pci_range_parser parser;
> > + u32 val;
> > +
> > + if (of_pci_range_parser_init(&parser, np)) {
> > + dev_err(pp->dev, "missing ranges property\n");
> > + return -EINVAL;
> > + }
>
> I have some confusion here w.r.t address space :-s
This scheme has been confirmed for last 6 months.
Previous mail threads on Marvell PCIe, Tegra PCIe will be helpful
to catch up this.
> > +
> > + /* 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) {
> > + 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;
> > + }
> > + }
> > +
> > + pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
> > + resource_size(&pp->cfg));
>
> Why is configuraion space divided into two?
Sorry, I don't know the exact reason. :(
Pratyush Anand may know about this.
Pratyush Anand, could you answer the question?
Also, if you find some problems, please let me know.
> Why should it be same as dbi_base?
> AFAIK, jacinto6 has a dedicated configuration/io/memory space that is entirely
> different from dbi_base.
According to the Synopsys designware PCIe datasheet,
in chapter 5.1.1 Register Space Layout,
'Port Logic Registers' are placed between (config space 0x0 + 0x700)
and (config space 0x0 + 0xFFF).
'dbi_base' is used for reading/writing 'Port Logic Registers'.
Exynos are using 'dbi_base' like this. Thus, the base addresses of
both 'dbi_base' and configuration/io/memory space are same.
Just now, I looked at Spear PCIe driver.
However, in the case of Spear, the base address of configuration/io/memory
space is defined as 0x80000000. The base address of 'Port Logic Registers'
is defined as 0xb1000000.
I think that the situation of 'jacinto6' is similar with Spear, right?
Then, I will move pp->dbi_base mapping code from pcie-designware.c
to pci-exynos.c.
Best regards,
Jingoo Han
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
