[U-Boot] [PATCH 3/7] pci: xilinx: Add a driver for Xilinx AXI to PCIe bridge
Paul Burton
paul.burton at imgtec.com
Wed Jul 27 16:18:11 CEST 2016
On 27/07/16 00:43, Simon Glass wrote:
> Hi Paul,
>
> On 26 July 2016 at 16:24, Paul Burton <paul.burton at imgtec.com> wrote:
>> This patch adds a driver for the Xilinx AXI bridge for PCI express, an
>> IP block which can be used on some generations of Xilinx FPGAs. This is
>> mostly a case of implementing PCIe ECAM specification, but with some
>> quirks about what devices are valid to access.
>>
>> Signed-off-by: Paul Burton <paul.burton at imgtec.com>
>> ---
>>
>> drivers/pci/Kconfig | 7 ++
>> drivers/pci/Makefile | 1 +
>> drivers/pci/pcie_xilinx.c | 219 ++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 227 insertions(+)
>> create mode 100644 drivers/pci/pcie_xilinx.c
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
Hi Simon,
Thanks :)
> Nits below.
>
>>
>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>> index 26aa2b0..1f9ea66 100644
>> --- a/drivers/pci/Kconfig
>> +++ b/drivers/pci/Kconfig
>> @@ -38,4 +38,11 @@ config PCI_TEGRA
>> with a total of 5 lanes. Some boards require this for Ethernet
>> support to work (e.g. beaver, jetson-tk1).
>>
>> +config PCI_XILINX
>> + bool "Xilinx AXI Bridge for PCI Express"
>> + depends on DM_PCI
>> + help
>> + Enable support for the Xilinx AXI bridge for PCI express, an IP block
>> + which can be used on some generations of Xilinx FPGAs.
>> +
>> endmenu
>> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
>> index f8be9bf..9583e91 100644
>> --- a/drivers/pci/Makefile
>> +++ b/drivers/pci/Makefile
>> @@ -31,3 +31,4 @@ obj-$(CONFIG_PCI_TEGRA) += pci_tegra.o
>> obj-$(CONFIG_TSI108_PCI) += tsi108_pci.o
>> obj-$(CONFIG_WINBOND_83C553) += w83c553f.o
>> obj-$(CONFIG_PCIE_LAYERSCAPE) += pcie_layerscape.o
>> +obj-$(CONFIG_PCI_XILINX) += pcie_xilinx.o
>> diff --git a/drivers/pci/pcie_xilinx.c b/drivers/pci/pcie_xilinx.c
>> new file mode 100644
>> index 0000000..55c3454
>> --- /dev/null
>> +++ b/drivers/pci/pcie_xilinx.c
>> @@ -0,0 +1,219 @@
>> +/*
>> + * Xilinx AXI Bridge for PCI Express Driver
>> + *
>> + * Copyright (C) 2016 Imagination Technologies
>> + *
>> + * SPDX-License-Identifier: GPL-2.0
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <pci.h>
>> +
>> +#include <asm/io.h>
>> +
>> +/**
>> + * struct xilinx_pcie - Xilinx PCIe controller state
>> + * @hose: The parent classes PCI controller state
>> + * @cfg_base: The base address of memory mapped configuration space
>> + */
>> +struct xilinx_pcie {
>> + struct pci_controller hose;
>> + void *cfg_base;
>> +};
>> +
>> +/* Register definitions */
>> +#define XILINX_PCIE_REG_PSCR 0x144
>> +#define XILINX_PCIE_REG_PSCR_LNKUP BIT(11)
>> +
>> +/**
>> + * pcie_xilinx_link_up() - Check whether the PCIe link is up
>> + * @pcie: Pointer to the PCI controller state
>> + *
>> + * Checks whether the PCIe link for the given device is up or down.
>> + *
>> + * Return: true if the link is up, else false
>> + */
>> +static bool pcie_xilinx_link_up(struct xilinx_pcie *pcie)
>> +{
>> + uint32_t pscr = __raw_readl(pcie->cfg_base + XILINX_PCIE_REG_PSCR);
>> +
>> + return pscr & XILINX_PCIE_REG_PSCR_LNKUP;
>> +}
>> +
>> +/**
>> + * pcie_xilinx_config_address() - Calculate the address of a config access
>> + * @pcie: Pointer to the PCI controller state
>> + * @bdf: Identifies the PCIe device to access
>> + * @offset: The offset into the device's configuration space
>> + * @paddress: Pointer to the pointer to write the calculates address to
>> + *
>> + * Calculates the address that should be accessed to perform a PCIe
>> + * configuration space access for a given device identified by the PCIe
>> + * controller device @pcie and the bus, device & function numbers in @bdf. If
>> + * access to the device is not valid then the function will return an error
>> + * code. Otherwise the address to access will be written to the pointer pointed
>> + * to by @paddress.
>> + *
>> + * Return: 0 on success, else -ERRNO
>
> -EINVAL or something
I'll make it -ENODEV as that's the only error returned.
>> + */
>> +static int pcie_xilinx_config_address(struct xilinx_pcie *pcie, pci_dev_t bdf,
>> + uint offset, void **paddress)
>> +{
>> + unsigned int bus = PCI_BUS(bdf);
>> + unsigned int dev = PCI_DEV(bdf);
>> + unsigned int func = PCI_FUNC(bdf);
>> + void *addr;
>> +
>> + if ((bus > 0) && !pcie_xilinx_link_up(pcie))
>> + return -ENODEV;
>
> There is a device, so perhaps -EINVAL would be better?
It depends what device we're talking about. There's a PCIE controller,
but there's no device accessible at the address encoded by bdf, and
that's what I'm trying to communicate back to the caller here.
>> +
>> + /*
>> + * Busses 0 (host-PCIe bridge) & 1 (its immediate child) are
>> + * limited to a single device each.
>> + */
>> + if ((bus < 2) && (dev > 0))
>> + return -ENODEV;
>
> Same here.
Likewise.
>> +
>> + addr = pcie->cfg_base;
>> + addr += bus << 20;
>> + addr += dev << 15;
>> + addr += func << 12;
>> + addr += offset;
>> + *paddress = addr;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * pcie_xilinx_read_config() - Read from configuration space
>> + * @pcie: Pointer to the PCI controller state
>> + * @bdf: Identifies the PCIe device to access
>> + * @offset: The offset into the device's configuration space
>> + * @valuep: A pointer at which to store the read value
>> + * @size: Indicates the size of access to perform
>> + *
>> + * Read a value of size @size from offset @offset within the configuration
>> + * space of the device identified by the bus, device & function numbers in @bdf
>> + * on the PCI bus @bus.
>> + *
>> + * Return: 0 on success, else -ERRNO
>> + */
>> +static int pcie_xilinx_read_config(struct udevice *bus, pci_dev_t bdf,
>> + uint offset, ulong *valuep,
>> + enum pci_size_t size)
>> +{
>> + struct xilinx_pcie *pcie = dev_get_priv(bus);
>> + void *address;
>> + int err;
>> +
>> + err = pcie_xilinx_config_address(pcie, bdf, offset, &address);
>> + if (err < 0) {
>> + *valuep = pci_get_ff(size);
>> + return 0;
>> + }
>> +
>> + switch (size) {
>> + case PCI_SIZE_8:
>> + *valuep = __raw_readb(address);
>> + return 0;
>> + case PCI_SIZE_16:
>> + *valuep = __raw_readw(address);
>> + return 0;
>> + case PCI_SIZE_32:
>> + *valuep = __raw_readl(address);
>> + return 0;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +/**
>> + * pcie_xilinx_write_config() - Write to configuration space
>> + * @pcie: Pointer to the PCI controller state
>> + * @bdf: Identifies the PCIe device to access
>> + * @offset: The offset into the device's configuration space
>> + * @value: The value to write
>> + * @size: Indicates the size of access to perform
>> + *
>> + * Write the value @value of size @size from offset @offset within the
>> + * configuration space of the device identified by the bus, device & function
>> + * numbers in @bdf on the PCI bus @bus.
>> + *
>> + * Return: 0 on success, else -ERRNO
>> + */
>> +static int pcie_xilinx_write_config(struct udevice *bus, pci_dev_t bdf,
>> + uint offset, ulong value,
>> + enum pci_size_t size)
>> +{
>> + struct xilinx_pcie *pcie = dev_get_priv(bus);
>> + void *address;
>> + int err;
>> +
>> + err = pcie_xilinx_config_address(pcie, bdf, offset, &address);
>> + if (err < 0)
>> + return 0;
>
> Why squash the error?
The pci_bind_bus_devices function in pci-uclass.c expects to be able to
loop through each possible PCI device & attempt a read from it,
regardless of whether that device actually exists.
pcie_xilinx_config_address return -ENODEV where it knows that a device
is not allowed to exist or be accessed due to limitations of the PCIE
controller, and if we propogate that up to pci_bind_bus_devices then it
gets unhappy & fails with a "Cannot read bus configuration: -19"
message. As far as I can tell U-Boot expects accesses to non-existant
devices to "succeed" and for reads to just return all-Fs, which is what
happens here.
>> +
>> + switch (size) {
>> + case PCI_SIZE_8:
>> + __raw_writeb(value, address);
>> + return 0;
>> + case PCI_SIZE_16:
>> + __raw_writew(value, address);
>> + return 0;
>> + case PCI_SIZE_32:
>> + __raw_writel(value, address);
>> + return 0;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +/**
>> + * pcie_xilinx_ofdata_to_platdata() - Translate from DT to device state
>> + * @dev: A pointer to the device being operated on
>> + *
>> + * Translate relevant data from the device tree pertaining to device @dev into
>> + * state that the driver will later make use of. This state is stored in the
>> + * device's private data structure.
>> + *
>> + * Return: 0 on success, else -ERRNO
>> + */
>> +static int pcie_xilinx_ofdata_to_platdata(struct udevice *dev)
>> +{
>> + struct xilinx_pcie *pcie = dev_get_priv(dev);
>> + struct fdt_resource reg_res;
>> + DECLARE_GLOBAL_DATA_PTR;
>> + int err;
>> +
>> + err = fdt_get_resource(gd->fdt_blob, dev->of_offset, "reg",
>> + 0, ®_res);
>> + if (err < 0) {
>> + error("\"reg\" resource not found\n");
>> + return -EINVAL;
>
> return ret?
Yup, thanks.
Paul
>> + }
>> +
>> + pcie->cfg_base = ioremap_nocache(reg_res.start,
>> + fdt_resource_size(®_res));
>> +
>> + return 0;
>> +}
>> +
>> +static const struct dm_pci_ops pcie_xilinx_ops = {
>> + .read_config = pcie_xilinx_read_config,
>> + .write_config = pcie_xilinx_write_config,
>> +};
>> +
>> +static const struct udevice_id pcie_xilinx_ids[] = {
>> + { .compatible = "xlnx,axi-pcie-host-1.00.a" },
>> + { }
>> +};
>> +
>> +U_BOOT_DRIVER(pcie_xilinx) = {
>> + .name = "pcie_xilinx",
>> + .id = UCLASS_PCI,
>> + .of_match = pcie_xilinx_ids,
>> + .ops = &pcie_xilinx_ops,
>> + .ofdata_to_platdata = pcie_xilinx_ofdata_to_platdata,
>> + .priv_auto_alloc_size = sizeof(struct xilinx_pcie),
>> +};
>> --
>> 2.9.0
>>
>
> Regards,
> Simon
>
More information about the U-Boot
mailing list