Re: [U-Boot] [PATCH 1/2] PCI: Add driver for a 'pci-host-ecam-generic' host controller

Hi,

On 08/31/2017 09:51 AM, Bin Meng wrote:

Hi Tuomas,

On Wed, Aug 30, 2017 at 4:31 PM, Tuomas Tynkkynen tuomas.tynkkynen@iki.fi wrote:

...

QEMU emulates such a device with '-machine virt,highmem=off' on ARM. The 'highmem=off' part is required for things to work as the PCI code in U-Boot doesn't seem to support 64-bit BARs.

This driver is basically a copy-paste of the Xilinx PCIE driver with the Xilinx-specific bits removed and compatible string changed... The generic code should probably be extracted into some sort of library functions instead of duplicating them before committing this driver.

Signed-off-by: Tuomas Tynkkynen tuomas.tynkkynen@iki.fi

drivers/pci/Kconfig | 8 ++ drivers/pci/Makefile | 1 + drivers/pci/pcie_ecam_generic.c | 193 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 202 insertions(+) create mode 100644 drivers/pci/pcie_ecam_generic.c

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index e2a1c0a409..745161fb9f 100644 --- a/drivers/pci/Kconfig +++ b/drivers/pci/Kconfig @@ -33,6 +33,14 @@ config PCI_PNP help Enable PCI memory and I/O space resource allocation and assignment.

+config PCIE_ECAM_GENERIC

•   bool "Generic PCI-E ECAM support"
  

•   default n
  

nits: default n is not needed as it is the default value

Seems I have copied from PCIE_DW_MVEBU below. Removed.

...

•   depends on DM_PCI
  

•   help
  

•     Say Y here if you want to enable support for generic ECAM-based
  

•     PCIe controllers, such as the one emulated by QEMU.
  

• config PCIE_DW_MVEBU bool "Enable Armada-8K PCIe driver (DesignWare core)" default n

diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile index ad44e83996..5eb12efbf5 100644 --- a/drivers/pci/Makefile +++ b/drivers/pci/Makefile @@ -17,6 +17,7 @@ obj-$(CONFIG_PCI) += pci.o pci_auto_old.o endif obj-$(CONFIG_PCI) += pci_auto_common.o pci_common.o

+obj-$(CONFIG_PCIE_ECAM_GENERIC) += pcie_ecam_generic.o obj-$(CONFIG_FSL_PCI_INIT) += fsl_pci_init.o obj-$(CONFIG_PCI_INDIRECT_BRIDGE) += pci_indirect.o obj-$(CONFIG_PCI_GT64120) += pci_gt64120.o diff --git a/drivers/pci/pcie_ecam_generic.c b/drivers/pci/pcie_ecam_generic.c new file mode 100644 index 0000000000..039e378cb0 --- /dev/null +++ b/drivers/pci/pcie_ecam_generic.c @@ -0,0 +1,193 @@ +/*

• • Generic PCIE host provided by e.g. QEMU
• • Heavily based on drivers/pci/pcie_xilinx.c
• • 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 generic_ecam_pcie - generic_ecam PCIe controller state
• • @hose: The parent classes PCI controller state
• • @cfg_base: The base address of memory mapped configuration space
• */

+struct generic_ecam_pcie {

•   struct pci_controller hose;
  

This sounds like a non-DM PCI driver stuff. I don't see it is referenced in this driver.

Indeed, it appears to be leftover code also in the pcie_xilinx that I copied from. Also a bunch of other drivers that have had a DM conversion have this as leftovers. I will clean them up also.

...

•   void *cfg_base;
  

+};

+/**

• • pcie_generic_ecam_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 -ENODEV

I see this driver always return 0.

Will fix the comment. I kept the same signature for config_address since I'm planning to have common parts of .write_config and .read_config abstracted (similar to pci_generic_config_{read,write} in Linux) instead of copy pasting the same code the 3rd time in U-Boot.

...

• */

+static int pcie_generic_ecam_config_address(struct generic_ecam_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;
  

•   addr = pcie->cfg_base;
  

•   addr += bus << 20;
  

•   addr += dev << 15;
  

•   addr += func << 12;
  

•   addr += offset;
  

•   *paddress = addr;
  

•   return 0;
  

+}

+/**

• • pcie_generic_ecam_read_config() - Read from configuration space
• • @pcie: Pointer to the PCI controller state

There is no pcie parameter, instead it's bus.

Again a problem inherited from pcie_xilinx... will fix there as well.

...

• • @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 -ENODEV or -EINVAL
• */

+static int pcie_generic_ecam_read_config(struct udevice *bus, pci_dev_t bdf,

•                              uint offset, ulong *valuep,
  

•                              enum pci_size_t size)
  

+{

•   struct generic_ecam_pcie *pcie = dev_get_priv(bus);
  

•   void *address;
  

•   int err;
  

•   err = pcie_generic_ecam_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_generic_ecam_write_config() - Write to configuration space
• • @pcie: Pointer to the PCI controller state

There is no pcie parameter, instead it's bus.

Ditto.