Re: [PATCH 3/5] PCI: st: Provide support for the sti PCIe controller

[Arnd Bergmann]

On Wednesday 17 December 2014 11:34:44 Gabriel FERNANDEZ wrote:
> sti pcie is built around a Synopsis Designware PCIe IP.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxx>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@xxxxxxxxxx>
> ---
>  drivers/pci/host/Kconfig  |   5 +
>  drivers/pci/host/Makefile |   1 +
>  drivers/pci/host/pci-st.c | 713 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 719 insertions(+)
>  create mode 100644 drivers/pci/host/pci-st.c
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index c4b6568..999d2b9 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -102,4 +102,9 @@ config PCI_LAYERSCAPE
>  	help
>  	  Say Y here if you want PCIe controller support on Layerscape SoCs.
>  
> +config PCI_ST
> +	bool "ST STiH41x PCIe controller"
> +	depends on ARCH_STI
> +	select PCIE_DW

I'd use 'depends on ARCH_STI || (ARM && COMPILE_TEST)' to enable
building this on other platforms for test purposes.

> +
> +#define to_st_pcie(x)	container_of(x, struct st_pcie, pp)
> +
> +/**
> + * struct st_pcie_ops - SOC dependent data
> + * @init: reference to controller power & reset init routine
> + * @enable_ltssm: reference to controller link enable routine
> + * @disable_ltssm:  reference to controller link disable routine
> + * @phy_auto: flag when phy automatically configured
> + */
> +struct st_pcie_ops {
> +	int (*init)(struct pcie_port *pp);
> +	int (*enable_ltssm)(struct pcie_port *pp);
> +	int (*disable_ltssm)(struct pcie_port *pp);
> +	bool phy_auto;
> +};

It would be better not to invent another level of indirection. Try
turning this around so you have a driver that binds to the specific
SoC compatible string for the PCIe port while calling into a common
library module for things that are shared.

> +/*
> + * On ARM platforms, we actually get a bus error returned when the PCIe IP
> + * returns a UR or CRS instead of an OK.
> + */
> +static int st_pcie_abort_handler(unsigned long addr, unsigned int fsr,
> +				 struct pt_regs *regs)
> +{
> +	return 0;
> +}

You should check that it's actually PCI that caused the abort. Don't
just ignore a hard error condition.

Usually there are registers in the PCI core that let you identify what
happened.

> +static int st_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> +				 unsigned int devfn, int where, int size,
> +				 u32 *val)
> +{
> +	u32 data;
> +	u32 bdf;
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +	int is_root_bus = pci_is_root_bus(bus);
> +	int retry_count = 0;
> +	int ret;
> +	void __iomem *addr;
> +
> +	/*
> +	 * Prerequisite
> +	 * PCI express devices will respond to all config type 0 cycles, since
> +	 * they are point to point links. Thus to avoid probing for multiple
> +	 * devices on the root, dw-pcie already check for us if it is on the
> +	 * root bus / other slots. Also, dw-pcie checks for the link being up
> +	 * as we will hang if we issue a config request and the link is down.
> +	 * A switch will reject requests for slots it knows do not exist.
> +	 */
> +	bdf = bdf_num(bus->number, devfn, is_root_bus);
> +	addr = pcie->config_area + config_addr(where,
> +			bus->parent->number == pp->root_bus_nr);
> +retry:
> +	/* Set the config packet devfn */
> +	writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
> +	readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
> +
> +	ret = dw_pcie_cfg_read(addr, where, size, &data);
> +
> +	/*
> +	 * This is intended to help with when we are probing the bus. The
> +	 * problem is that the wrapper logic doesn't have any way to
> +	 * interrogate if the configuration request failed or not.
> +	 * On the ARM we actually get a real bus error.
> +	 *
> +	 * Unfortunately this means it is impossible to tell the difference
> +	 * between when a device doesn't exist (the switch will return a UR
> +	 * completion) or the device does exist but isn't yet ready to accept
> +	 * configuration requests (the device will return a CRS completion)
> +	 *
> +	 * The result of this is that we will miss devices when probing.
> +	 *
> +	 * So if we are trying to read the dev/vendor id on devfn 0 and we
> +	 * appear to get zero back, then we retry the request.  We know that
> +	 * zero can never be a valid device/vendor id. The specification says
> +	 * we must retry for up to a second before we decide the device is
> +	 * dead. If we are still dead then we assume there is nothing there and
> +	 * return ~0
> +	 *
> +	 * The downside of this is that we incur a delay of 1s for every pci
> +	 * express link that doesn't have a device connected.
> +	 */
> +	if (((where & ~3) == 0) && devfn == 0 && (data == 0 || data == ~0)) {
> +		if (retry_count++ < 1000) {
> +			mdelay(1);
> +			goto retry;
> +		} else {
> +			*val = ~0;
> +			return PCIBIOS_DEVICE_NOT_FOUND;
> +		}
> +	}
> +
> +	*val = data;
> +	return ret;
> +}

A busy-loop is extremely nasty. If this is only during the initial bus
scan, could you use an msleep instead?

Also, it sounds like the error you get is actually the fault that you
are catching above. If this is correct, then use the fault handler to
communicate this to the probe function.

> +
> +static void st_pcie_board_reset(struct pcie_port *pp)
> +{
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +
> +	if (!gpio_is_valid(pcie->reset_gpio))
> +		return;
> +
> +	if (gpio_direction_output(pcie->reset_gpio, 0)) {
> +		dev_err(pp->dev, "Cannot set PERST# (gpio %u) to output\n",
> +			pcie->reset_gpio);
> +		return;
> +	}
> +
> +	/* From PCIe spec */
> +	usleep_range(1000, 2000);
> +	gpio_direction_output(pcie->reset_gpio, 1);
> +
> +	/*
> +	 * PCIe specification states that you should not issue any config
> +	 * requests until 100ms after asserting reset, so we enforce that here
> +	 */
> +	usleep_range(100000, 150000);
> +}

This seems hardly performance critical, just use msleep(2) and
msleep(100) instead of the usleep_range().

> +static void st_msi_init_one(struct pcie_port *pp)
> +{
> +	struct st_pcie *pcie = to_st_pcie(pp);
> +
> +	/*
> +	 * Set the magic address the hardware responds to. This has to be in
> +	 * the range the PCI controller can write to.
> +	 */
> +	dw_pcie_msi_init(pp);
> +
> +	if ((virt_to_phys((void *)pp->msi_data) < pcie->lmi->start) ||
> +	    (virt_to_phys((void *)pp->msi_data) > pcie->lmi->end))
> +		dev_err(pp->dev, "MSI addr miss-configured\n");
> +}

Why do you call virt_to_phys() here? Isn't msi_data a physical address?

> +static int __init st_pcie_probe(struct platform_device *pdev)

I'd suggest removing the __init here, as discussed in the review for
the qualcomm driver.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/