Re: [PATCH v1 5/5] pci: keystone: add pcie driver based on designware core driver

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 15 May 2014 12:01:32 Murali Karicheri wrote:

> +static int
> +keystone_pcie_fault(unsigned long addr, unsigned int fsr,
> +		struct pt_regs *regs)
> +{
> +	unsigned long instr = *(unsigned long *) instruction_pointer(regs);
> +
> +	if ((instr & 0x0e100090) == 0x00100090) {
> +		int reg = (instr >> 12) & 15;
> +
> +		regs->uregs[reg] = -1;
> +		regs->ARM_pc += 4;
> +	}
> +
> +	return 0;
> +}

This needs to check in the PCI host registers what happened and do
some appropriate action. If the fault was not caused by PCIe, it
should not be ignored here but get passed on to the next handler.

> +static int __init ks_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct keystone_pcie *ks_pcie;
> +	void __iomem *devstat;
> +	struct pcie_port *pp;
> +	struct resource *res;
> +	struct phy *phy;
> +	int ret = 0;
> +	u32 val;
> +
> +	ks_pcie = devm_kzalloc(&pdev->dev, sizeof(*ks_pcie),
> +				GFP_KERNEL);
> +	if (!ks_pcie) {
> +		dev_err(dev, "no memory for keystone pcie\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* check if serdes phy needs to be enabled */
> +	if (of_get_property(np, "ti,init-phy", NULL) != NULL) {
> +		phy = devm_phy_get(dev, "pcie-phy");
> +			if (IS_ERR(phy))
> +				return PTR_ERR(phy);
> +
> +		ret = phy_init(phy);
> +		if (ret < 0)
> +			return ret;
> +	}

I think you can just call devm_phy_get() unconditionally here and
get rid of the ti,init-phy property. If there is no phy, don't
initialize it.

> +
> +	ret = add_pcie_port(ks_pcie, pdev);
> +	if (ret < 0)
> +		goto fail_clk;
> +
> +	platform_set_drvdata(pdev, ks_pcie);

Set the platform data first, then add the port.

> +	dev_info(dev, "pcie rc probe success\n");

Remove this.

> +#ifdef CONFIG_PCI_KEYSTONE
> +/*
> + * The KeyStone PCIe controller has maximum read request size of 256 bytes.
> + */
> +static void quirk_limit_readrequest(struct pci_dev *dev)
> +{
> +	int readrq = pcie_get_readrq(dev);
> +
> +	if (readrq > 256)
> +		pcie_set_readrq(dev, 256);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest);
> +#endif /* CONFIG_PCI_KEYSTONE */

This doesn't work: you can't just limit do this for all devices just based
on PCI_KEYSTONE being enabled, you have to check if you are actually using
this controller.

	Arnd