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

Fri Sep 1 09:10:52 UTC 2017

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 at 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 at 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.