Re: [U-Boot] [PATCH 3/7] pci: xilinx: Add a driver for Xilinx AXI to PCIe bridge

On 27/07/16 00:43, Simon Glass wrote:

Hi Paul,

On 26 July 2016 at 16:24, Paul Burton paul.burton@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@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@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, &reg_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(&reg_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