| Message ID | 20170830083135.9183-2-tuomas.tynkkynen@iki.fi |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Board for QEMU's '-machine virt' on ARM | expand |
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 > + 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. > + 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. > + */ > +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. > + * @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. > + * @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 -ENODEV or -EINVAL > + */ > +static int pcie_generic_ecam_write_config(struct udevice *bus, pci_dev_t bdf, > + uint offset, ulong value, > + 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) > + return 0; > + > + 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_generic_ecam_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 -EINVAL > + */ > +static int pcie_generic_ecam_ofdata_to_platdata(struct udevice *dev) > +{ > + struct generic_ecam_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(dev), "reg", > + 0, ®_res); > + if (err < 0) { > + error("\"reg\" resource not found\n"); > + return err; > + } > + > + pcie->cfg_base = map_physmem(reg_res.start, > + fdt_resource_size(®_res), > + MAP_NOCACHE); > + > + return 0; > +} > + > +static const struct dm_pci_ops pcie_generic_ecam_ops = { > + .read_config = pcie_generic_ecam_read_config, > + .write_config = pcie_generic_ecam_write_config, > +}; > + > +static const struct udevice_id pcie_generic_ecam_ids[] = { > + { .compatible = "pci-host-ecam-generic" }, > + { } > +}; > + > +U_BOOT_DRIVER(pcie_generic_ecam) = { > + .name = "pcie_generic_ecam", > + .id = UCLASS_PCI, > + .of_match = pcie_generic_ecam_ids, > + .ops = &pcie_generic_ecam_ops, > + .ofdata_to_platdata = pcie_generic_ecam_ofdata_to_platdata, > + .priv_auto_alloc_size = sizeof(struct generic_ecam_pcie), > +}; > -- Regards, Bin
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.
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 + 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; + 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 + */ +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 + * @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 + * @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 -ENODEV or -EINVAL + */ +static int pcie_generic_ecam_write_config(struct udevice *bus, pci_dev_t bdf, + uint offset, ulong value, + 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) + return 0; + + 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_generic_ecam_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 -EINVAL + */ +static int pcie_generic_ecam_ofdata_to_platdata(struct udevice *dev) +{ + struct generic_ecam_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(dev), "reg", + 0, ®_res); + if (err < 0) { + error("\"reg\" resource not found\n"); + return err; + } + + pcie->cfg_base = map_physmem(reg_res.start, + fdt_resource_size(®_res), + MAP_NOCACHE); + + return 0; +} + +static const struct dm_pci_ops pcie_generic_ecam_ops = { + .read_config = pcie_generic_ecam_read_config, + .write_config = pcie_generic_ecam_write_config, +}; + +static const struct udevice_id pcie_generic_ecam_ids[] = { + { .compatible = "pci-host-ecam-generic" }, + { } +}; + +U_BOOT_DRIVER(pcie_generic_ecam) = { + .name = "pcie_generic_ecam", + .id = UCLASS_PCI, + .of_match = pcie_generic_ecam_ids, + .ops = &pcie_generic_ecam_ops, + .ofdata_to_platdata = pcie_generic_ecam_ofdata_to_platdata, + .priv_auto_alloc_size = sizeof(struct generic_ecam_pcie), +};
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