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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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_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
> +#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);
> +			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;
> +		}
> +	}

Same for these: it would be better to split up the probe function here
and move all of the above into the common code.

> +	pp->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);

I assume the reset logic would need to remain exynos specific

> +	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;
> +
> +	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.

> +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;
> +	}
> +
> +	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
--
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




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux