[U-Boot] [PATCH v1 0/3] Add Cadence PCIe endpoint driver with new uclass

This patchset adds support for new uclass, UCLASS_PCI_EP allowing new set of PCI endpoint drivers. Included in the patchset is also a driver for Cadence PCIe endpoint.
Ramon Fried (3): drivers: pci_ep: Introduce UCLASS_PCI_EP uclass pci: pci.h: add missing maskbit pci_ep: add Cadence PCIe endpoint driver
.../pci_endpoint/cdns,cdns-pcie-ep.txt | 18 + drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/pci_endpoint/Kconfig | 25 ++ drivers/pci_endpoint/Makefile | 7 + drivers/pci_endpoint/pci_ep-uclass.c | 192 +++++++++ drivers/pci_endpoint/pcie-cadence-ep.c | 177 +++++++++ drivers/pci_endpoint/pcie-cadence.h | 309 +++++++++++++++ include/dm/uclass-id.h | 1 + include/pci.h | 1 + include/pci_ep.h | 375 ++++++++++++++++++ 11 files changed, 1108 insertions(+) create mode 100644 doc/device-tree-bindings/pci_endpoint/cdns,cdns-pcie-ep.txt create mode 100644 drivers/pci_endpoint/Kconfig create mode 100644 drivers/pci_endpoint/Makefile create mode 100644 drivers/pci_endpoint/pci_ep-uclass.c create mode 100644 drivers/pci_endpoint/pcie-cadence-ep.c create mode 100644 drivers/pci_endpoint/pcie-cadence.h create mode 100644 include/pci_ep.h

Introduce new UCLASS_PCI_EP class for handling PCI endpoint devices, allowing to set various attributes of the PCI endpoint device, such as: * configuration space header * BAR definitions * outband memory mapping * start/stop PCI link
Signed-off-by: Ramon Fried ramon.fried@gmail.com
---
drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/pci_endpoint/Kconfig | 16 ++ drivers/pci_endpoint/Makefile | 6 + drivers/pci_endpoint/pci_ep-uclass.c | 192 ++++++++++++++ include/dm/uclass-id.h | 1 + include/pci_ep.h | 375 +++++++++++++++++++++++++++ 7 files changed, 593 insertions(+) create mode 100644 drivers/pci_endpoint/Kconfig create mode 100644 drivers/pci_endpoint/Makefile create mode 100644 drivers/pci_endpoint/pci_ep-uclass.c create mode 100644 include/pci_ep.h
diff --git a/drivers/Kconfig b/drivers/Kconfig index f24351ac4f..59e2c22cc6 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -64,6 +64,8 @@ source "drivers/nvme/Kconfig"
source "drivers/pci/Kconfig"
+source "drivers/pci_endpoint/Kconfig" + source "drivers/pch/Kconfig"
source "drivers/pcmcia/Kconfig" diff --git a/drivers/Makefile b/drivers/Makefile index a7bba3ed56..480b97ef58 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -85,6 +85,7 @@ obj-$(CONFIG_FPGA) += fpga/ obj-y += misc/ obj-$(CONFIG_MMC) += mmc/ obj-$(CONFIG_NVME) += nvme/ +obj-$(CONFIG_PCI_ENDPOINT) += pci_endpoint/ obj-y += pcmcia/ obj-y += dfu/ obj-$(CONFIG_PCH) += pch/ diff --git a/drivers/pci_endpoint/Kconfig b/drivers/pci_endpoint/Kconfig new file mode 100644 index 0000000000..2c0a399a08 --- /dev/null +++ b/drivers/pci_endpoint/Kconfig @@ -0,0 +1,16 @@ +# SPDX-License-Identifier: GPL-2.0 +# +# PCI Endpoint Support +# + +menu "PCI Endpoint" + +config PCI_ENDPOINT + bool "PCI Endpoint Support" + depends on DM + help + Enable this configuration option to support configurable PCI + endpoint. This should be enabled if the platform has a PCI + controller that can operate in endpoint mode. + +endmenu diff --git a/drivers/pci_endpoint/Makefile b/drivers/pci_endpoint/Makefile new file mode 100644 index 0000000000..80a1066925 --- /dev/null +++ b/drivers/pci_endpoint/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0+ +# +# (C) Copyright 2019 +# Ramon Fried ramon.fried@gmail.com + +obj-y += pci_ep-uclass.o diff --git a/drivers/pci_endpoint/pci_ep-uclass.c b/drivers/pci_endpoint/pci_ep-uclass.c new file mode 100644 index 0000000000..06fcfc5d14 --- /dev/null +++ b/drivers/pci_endpoint/pci_ep-uclass.c @@ -0,0 +1,192 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * PCI Endpoint uclass + * + * Based on Linux PCI-EP driver written by + * Kishon Vijay Abraham I kishon@ti.com + * + * Copyright (c) 2019 + * Written by Ramon Fried ramon.fried@gmail.com + */ + +#include <common.h> +#include <dm.h> +#include <errno.h> +#include <linux/log2.h> +#include <pci_ep.h> + +DECLARE_GLOBAL_DATA_PTR; + +int dm_pci_ep_write_header(struct udevice *dev, u8 fn, + struct pci_ep_header *hdr) +{ + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev); + + if (!ops->write_header) + return -ENODEV; + + return ops->write_header(dev, fn, hdr); +} + +int dm_pci_ep_read_header(struct udevice *dev, u8 fn, + struct pci_ep_header *hdr) +{ + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev); + + if (!ops->read_header) + return -ENODEV; + + return ops->read_header(dev, fn, hdr); +} + +int dm_pci_ep_set_bar(struct udevice *dev, u8 func_no, + struct pci_bar *ep_bar) +{ + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev); + int flags = ep_bar->flags; + + /* Some basic bar validity checks */ + if ((ep_bar->barno == BAR_5 && flags & + PCI_BASE_ADDRESS_MEM_TYPE_64) || + (flags & PCI_BASE_ADDRESS_SPACE_IO && + flags & PCI_BASE_ADDRESS_IO_MASK) || + (upper_32_bits(ep_bar->size) && + !(flags & PCI_BASE_ADDRESS_MEM_TYPE_64))) + return -EINVAL; + + if (!ops->set_bar) + return -ENODEV; + + return ops->set_bar(dev, func_no, ep_bar); +} + +void pci_ep_clear_bar(struct udevice *dev, u8 func_num, enum pci_barno bar) +{ + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev); + + if (!ops->clear_bar) + return; + + ops->clear_bar(dev, func_num, bar); +} + +int dm_pci_ep_map_addr(struct udevice *dev, u8 func_no, phys_addr_t addr, + u64 pci_addr, size_t size) +{ + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev); + + if (!ops->map_addr) + return -ENODEV; + + return ops->map_addr(dev, func_no, addr, pci_addr, size); +} + +void dm_pci_ep_unmap_addr(struct udevice *dev, u8 func_no, phys_addr_t addr) +{ + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev); + + if (!ops->unmap_addr) + return; + + ops->unmap_addr(dev, func_no, addr); +} + +int dm_pci_ep_set_msi(struct udevice *dev, u8 func_no, u8 interrupts) +{ + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev); + u8 encode_int; + + if (interrupts > 32) + return -EINVAL; + + if (!ops->set_msi) + return -ENODEV; + + encode_int = order_base_2(interrupts); + + return ops->set_msi(dev, func_no, encode_int); +} + +int dm_pci_ep_get_msi(struct udevice *dev, u8 func_no) +{ + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev); + int interrupt; + + if (!ops->get_msi) + return -ENODEV; + + interrupt = ops->get_msi(dev, func_no); + + if (interrupt < 0) + return 0; + + interrupt = 1 << interrupt; + + return interrupt; +} + +int dm_pci_ep_set_msix(struct udevice *dev, u8 func_no, u16 interrupts) +{ + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev); + + if (interrupts < 1 || interrupts > 2048) + return -EINVAL; + + if (!ops->set_msix) + return -ENODEV; + + return ops->set_msix(dev, func_no, interrupts); +} + +int dm_pci_ep_get_msix(struct udevice *dev, u8 func_no) +{ + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev); + int interrupt; + + if (!ops->get_msix) + return -ENODEV; + + interrupt = ops->get_msix(dev, func_no); + + if (interrupt < 0) + return 0; + + return interrupt + 1; +} + +int dm_pci_ep_raise_irq(struct udevice *dev, u8 func_no, + enum pci_ep_irq_type type, u16 interrupt_num) +{ + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev); + + if (!ops->raise_irq) + return -ENODEV; + + return ops->raise_irq(dev, func_no, type, interrupt_num); +} + +int dm_pci_ep_start(struct udevice *dev) +{ + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev); + + if (!ops->start) + return -ENODEV; + + return ops->start(dev); +} + +void dm_pci_ep_stop(struct udevice *dev) +{ + struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev); + + if (!ops->stop) + return; + + ops->stop(dev); +} + +UCLASS_DRIVER(pci_ep) = { + .id = UCLASS_PCI_EP, + .name = "pci_ep", + .flags = DM_UC_FLAG_SEQ_ALIAS, +}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 86e59781b0..d29f7d5a34 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -67,6 +67,7 @@ enum uclass_id { UCLASS_PANEL_BACKLIGHT, /* Backlight controller for panel */ UCLASS_PCH, /* x86 platform controller hub */ UCLASS_PCI, /* PCI bus */ + UCLASS_PCI_EP, /* PCI endpoint device */ UCLASS_PCI_GENERIC, /* Generic PCI bus device */ UCLASS_PHY, /* Physical Layer (PHY) device */ UCLASS_PINCONFIG, /* Pin configuration node device */ diff --git a/include/pci_ep.h b/include/pci_ep.h new file mode 100644 index 0000000000..c98453ccfa --- /dev/null +++ b/include/pci_ep.h @@ -0,0 +1,375 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Adapted from Linux kernel driver + * Copyright (C) 2017 Texas Instruments + * Author: Kishon Vijay Abraham I kishon@ti.com + * + * (C) Copyright 2019 + * Ramon Fried ramon.fried@gmail.com + */ + +#ifndef _PCI_EP_H +#define _PCI_EP_H + +#include <pci.h> + +/** + * enum pci_interrupt_pin - PCI INTx interrupt values + * @PCI_INTERRUPT_UNKNOWN: Unknown or unassigned interrupt + * @PCI_INTERRUPT_INTA: PCI INTA pin + * @PCI_INTERRUPT_INTB: PCI INTB pin + * @PCI_INTERRUPT_INTC: PCI INTC pin + * @PCI_INTERRUPT_INTD: PCI INTD pin + * + * Corresponds to values for legacy PCI INTx interrupts, as can be found in the + * PCI_INTERRUPT_PIN register. + */ +enum pci_interrupt_pin { + PCI_INTERRUPT_UNKNOWN, + PCI_INTERRUPT_INTA, + PCI_INTERRUPT_INTB, + PCI_INTERRUPT_INTC, + PCI_INTERRUPT_INTD, +}; + +enum pci_barno { + BAR_0, + BAR_1, + BAR_2, + BAR_3, + BAR_4, + BAR_5, +}; + +enum pci_ep_irq_type { + PCI_EP_IRQ_UNKNOWN, + PCI_EP_IRQ_LEGACY, + PCI_EP_IRQ_MSI, + PCI_EP_IRQ_MSIX, +}; + +/** + * struct pci_bar - represents the BAR of EP device + * @phys_addr: physical address that should be mapped to the BAR + * @size: the size of the address space present in BAR + */ +struct pci_bar { + dma_addr_t phys_addr; + size_t size; + enum pci_barno barno; + int flags; +}; + +/** + * struct pci_ep_header - represents standard configuration header + * @vendorid: identifies device manufacturer + * @deviceid: identifies a particular device + * @revid: specifies a device-specific revision identifier + * @progif_code: identifies a specific register-level programming interface + * @subclass_code: identifies more specifically the function of the device + * @baseclass_code: broadly classifies the type of function the device performs + * @cache_line_size: specifies the system cacheline size in units of DWORDs + * @subsys_vendor_id: vendor of the add-in card or subsystem + * @subsys_id: id specific to vendor + * @interrupt_pin: interrupt pin the device (or device function) uses + */ +struct pci_ep_header { + u16 vendorid; + u16 deviceid; + u8 revid; + u8 progif_code; + u8 subclass_code; + u8 baseclass_code; + u8 cache_line_size; + u16 subsys_vendor_id; + u16 subsys_id; + enum pci_interrupt_pin interrupt_pin; +}; + +/* PCI endpoint operations */ +struct dm_pci_ep_ops { + /** + * write_header() - Write a PCI configuration space header + * + * @dev: device to write to + * @func_num: Function to fill + * @hdr: header to write + * @return 0 if OK, -ve on error + */ + int (*write_header)(struct udevice *dev, u8 func_num, + struct pci_ep_header *hdr); + /** + * read_header() - Read a PCI configuration space header + * + * @dev: device to write to + * @func_num: Function to fill + * @hdr: header to read to + * @return 0 if OK, -ve on error + */ + int (*read_header)(struct udevice *dev, u8 func_num, + struct pci_ep_header *hdr); + /** + * set_bar() - set BAR (Base Address Register) + * + * @dev: device to set + * @func_num: Function to set + * @bar: bar data + * @return 0 if OK, -ve on error + */ + int (*set_bar)(struct udevice *dev, u8 func_num, + struct pci_bar *bar); + /** + * clear_bar() - clear BAR (Base Address Register) + * + * @dev: device to clear + * @func_num: Function to clear + * @bar: bar number + */ + void (*clear_bar)(struct udevice *dev, u8 func_num, + enum pci_barno bar); + /** + * map_addr() - map CPU address to PCI address + * + * outband region is used in order to generate PCI read/write + * transaction from local memory/write. + * + * @dev: device to set + * @func_num: Function to set + * @addr: local physical address base + * @pci_addr: pci address to translate to + * @size: region size + * @return 0 if OK, -ve on error + */ + int (*map_addr)(struct udevice *dev, u8 func_num, + phys_addr_t addr, u64 pci_addr, size_t size); + /** + * unmap_addr() - unmap CPU address to PCI address + * + * unmap previously mapped region. + * + * @dev: device to set + * @func_num: Function to set + * @addr: local physical address base + */ + void (*unmap_addr)(struct udevice *dev, u8 func_num, + phys_addr_t addr); + /** + * set_msi() - set msi capability property + * + * set the number of required MSI vectors the device + * needs for operation. + * + * @dev: device to set + * @func_num: Function to set + * @interrupts: required interrupts count + * @return 0 if OK, -ve on error + */ + int (*set_msi)(struct udevice *dev, u8 func_num, u8 interrupts); + + /** + * get_msi() - get the number of MSI interrupts allocated by the host. + * + * Read the Multiple Message Enable bitfield from + * Message control register. + * + * @dev: device to use + * @func_num: Function to use + * @return msi count if OK, -EINVAL if msi were not enabled at host. + */ + int (*get_msi)(struct udevice *dev, u8 func_num); + + /** + * set_msix() - set msix capability property + * + * set the number of required MSIx vectors the device + * needs for operation. + * + * @dev: device to set + * @func_num: Function to set + * @interrupts: required interrupts count + * @return 0 if OK, -ve on error + */ + int (*set_msix)(struct udevice *dev, u8 func_num, u16 interrupts); + + /** + * get_msix() - get the number of MSIx interrupts allocated by the host. + * + * Read the Multiple Message Enable bitfield from + * Message control register. + * + * @dev: device to use + * @func_num: Function to use + * @return msi count if OK, -EINVAL if msi were not enabled at host. + */ + int (*get_msix)(struct udevice *dev, u8 func_num); + + /** + * raise_irq() - raise a legacy, MSI or MSI-X interrupt + * + * @dev: device to set + * @func_num: Function to set + * @type: type of irq to send + * @interrupt_num: interrupt vector to use + * @return 0 if OK, -ve on error + */ + int (*raise_irq)(struct udevice *dev, u8 func_num, + enum pci_ep_irq_type type, u16 interrupt_num); + /** + * start() - start the PCI link + * + * @dev: device to set + * @return 0 if OK, -ve on error + */ + int (*start)(struct udevice *dev); + + /** + * stop() - stop the PCI link + * + * @dev: device to set + */ + void (*stop)(struct udevice *dev); +}; + +#define pci_ep_get_ops(dev) ((struct dm_pci_ep_ops *)(dev)->driver->ops) + +/** + * pci_ep_write_header() - Write a PCI configuration space header + * + * @dev: device to write to + * @func_num: Function to fill + * @hdr: header to write + * @return 0 if OK, -ve on error + */ +int pci_ep_write_header(struct udevice *dev, u8 func_num, + struct pci_ep_header *hdr); + +/** + * dm_pci_ep_read_header() - Read a PCI configuration space header + * + * @dev: device to write to + * @func_num: Function to fill + * @hdr: header to read to + * @return 0 if OK, -ve on error + */ +int pci_ep_read_header(struct udevice *dev, u8 func_num, + struct pci_ep_header *hdr); +/** + * pci_ep_set_bar() - set BAR (Base Address Register) + * + * @dev: device to set + * @func_num: Function to set + * @bar: bar data + * @return 0 if OK, -ve on error + */ +int pci_ep_set_bar(struct udevice *dev, u8 func_num, struct pci_bar *bar); + +/** + * pci_ep_clear_bar() - clear BAR (Base Address Register) + * + * @dev: device to clear + * @func_num: Function to clear + * @bar: bar number + */ +void pci_ep_clear_bar(struct udevice *dev, u8 func_num, enum pci_barno bar); +/** + * pci_ep_map_addr() - map CPU address to PCI address + * + * outband region is used in order to generate PCI read/write + * transaction from local memory/write. + * + * @dev: device to set + * @func_num: Function to set + * @addr: local physical address base + * @pci_addr: pci address to translate to + * @size: region size + * @return 0 if OK, -ve on error + */ +int pci_ep_map_addr(struct udevice *dev, u8 func_num, phys_addr_t addr, + u64 pci_addr, size_t size); +/** + * pci_ep_unmap_addr() - unmap CPU address to PCI address + * + * unmap previously mapped region. + * + * @dev: device to set + * @func_num: Function to set + * @addr: local physical address base + */ +void pci_ep_unmap_addr(struct udevice *dev, u8 func_num, phys_addr_t addr); +/** + * pci_ep_set_msi() - set msi capability property + * + * set the number of required MSI vectors the device + * needs for operation. + * + * @dev: device to set + * @func_num: Function to set + * @interrupts: required interrupts count + * @return 0 if OK, -ve on error + */ +int pci_ep_set_msi(struct udevice *dev, u8 func_num, u8 interrupts); + +/** + * pci_ep_get_msi() - get the number of MSI interrupts allocated by the host. + * + * Read the Multiple Message Enable bitfield from + * Message control register. + * + * @dev: device to use + * @func_num: Function to use + * @return msi count if OK, -EINVAL if msi were not enabled at host. + */ +int pci_ep_get_msi(struct udevice *dev, u8 func_num); + +/** + * pci_ep_set_msix() - set msi capability property + * + * set the number of required MSIx vectors the device + * needs for operation. + * + * @dev: device to set + * @func_num: Function to set + * @interrupts: required interrupts count + * @return 0 if OK, -ve on error + */ +int pci_ep_set_msix(struct udevice *dev, u8 func_num, u16 interrupts); + +/** + * pci_ep_get_msix() - get the number of MSIx interrupts allocated by the host. + * + * Read the Multiple Message Enable bitfield from + * Message control register. + * + * @dev: device to use + * @func_num: Function to use + * @return msi count if OK, -EINVAL if msi were not enabled at host. + */ +int pci_ep_get_msix(struct udevice *dev, u8 func_num); + +/** + * pci_ep_raise_irq() - raise a legacy, MSI or MSI-X interrupt + * + * @dev: device to set + * @func_num: Function to set + * @type: type of irq to send + * @interrupt_num: interrupt vector to use + * @return 0 if OK, -ve on error + */ +int pci_ep_raise_irq(struct udevice *dev, u8 func_num, + enum pci_ep_irq_type type, u16 interrupt_num); +/** + * pci_ep_start() - start the PCI link + * + * @dev: device to set + * @return 0 if OK, -ve on error + */ +int pci_ep_start(struct udevice *dev); + +/** + * pci_ep_stop() - stop the PCI link + * + * @dev: device to set + */ +void pci_ep_stop(struct udevice *dev); + +#endif

Hi Ramon,
On Fri, 5 Apr 2019 at 19:12, Ramon Fried ramon.fried@gmail.com wrote:
Introduce new UCLASS_PCI_EP class for handling PCI endpoint devices, allowing to set various attributes of the PCI endpoint device, such as:
- configuration space header
- BAR definitions
- outband memory mapping
- start/stop PCI link
Signed-off-by: Ramon Fried ramon.fried@gmail.com
drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/pci_endpoint/Kconfig | 16 ++ drivers/pci_endpoint/Makefile | 6 + drivers/pci_endpoint/pci_ep-uclass.c | 192 ++++++++++++++ include/dm/uclass-id.h | 1 + include/pci_ep.h | 375 +++++++++++++++++++++++++++ 7 files changed, 593 insertions(+) create mode 100644 drivers/pci_endpoint/Kconfig create mode 100644 drivers/pci_endpoint/Makefile create mode 100644 drivers/pci_endpoint/pci_ep-uclass.c create mode 100644 include/pci_ep.h
This looks pretty good but I have a lot of nits, sorry.
Please can you add a sandbox driver and a test for this (can be in a separate patch).
diff --git a/drivers/Kconfig b/drivers/Kconfig index f24351ac4f..59e2c22cc6 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -64,6 +64,8 @@ source "drivers/nvme/Kconfig"
source "drivers/pci/Kconfig"
+source "drivers/pci_endpoint/Kconfig"
source "drivers/pch/Kconfig"
source "drivers/pcmcia/Kconfig" diff --git a/drivers/Makefile b/drivers/Makefile index a7bba3ed56..480b97ef58 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -85,6 +85,7 @@ obj-$(CONFIG_FPGA) += fpga/ obj-y += misc/ obj-$(CONFIG_MMC) += mmc/ obj-$(CONFIG_NVME) += nvme/ +obj-$(CONFIG_PCI_ENDPOINT) += pci_endpoint/ obj-y += pcmcia/ obj-y += dfu/ obj-$(CONFIG_PCH) += pch/ diff --git a/drivers/pci_endpoint/Kconfig b/drivers/pci_endpoint/Kconfig new file mode 100644 index 0000000000..2c0a399a08 --- /dev/null +++ b/drivers/pci_endpoint/Kconfig @@ -0,0 +1,16 @@ +# SPDX-License-Identifier: GPL-2.0 +# +# PCI Endpoint Support +#
+menu "PCI Endpoint"
+config PCI_ENDPOINT
bool "PCI Endpoint Support"
depends on DM
help
Enable this configuration option to support configurable PCI
endpoint. This should be enabled if the platform has a PCI
s/endpoint/endpoints/ I think
controller that can operate in endpoint mode.
Can you explain a bit more about what this means. I understand that people can look at the spec, but what is the purpose of 'endpoint mode' and what does it enable?
+endmenu diff --git a/drivers/pci_endpoint/Makefile b/drivers/pci_endpoint/Makefile new file mode 100644 index 0000000000..80a1066925 --- /dev/null +++ b/drivers/pci_endpoint/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0+ +# +# (C) Copyright 2019 +# Ramon Fried ramon.fried@gmail.com
+obj-y += pci_ep-uclass.o diff --git a/drivers/pci_endpoint/pci_ep-uclass.c b/drivers/pci_endpoint/pci_ep-uclass.c new file mode 100644 index 0000000000..06fcfc5d14 --- /dev/null +++ b/drivers/pci_endpoint/pci_ep-uclass.c @@ -0,0 +1,192 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- PCI Endpoint uclass
- Based on Linux PCI-EP driver written by
- Kishon Vijay Abraham I kishon@ti.com
- Copyright (c) 2019
- Written by Ramon Fried ramon.fried@gmail.com
- */
+#include <common.h> +#include <dm.h> +#include <errno.h> +#include <linux/log2.h> +#include <pci_ep.h>
+DECLARE_GLOBAL_DATA_PTR;
+int dm_pci_ep_write_header(struct udevice *dev, u8 fn,
struct pci_ep_header *hdr)
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (!ops->write_header)
return -ENODEV;
-ENOSYS (please fix globally)
There is a device. The problem here is that the driver does not implement the method.
return ops->write_header(dev, fn, hdr);
+}
+int dm_pci_ep_read_header(struct udevice *dev, u8 fn,
struct pci_ep_header *hdr)
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (!ops->read_header)
return -ENODEV;
return ops->read_header(dev, fn, hdr);
+}
+int dm_pci_ep_set_bar(struct udevice *dev, u8 func_no,
Please use uint for func_no. There is no need to limit it to 8 bits, and this may not be efficient for non-8-bit machines. Please fix globally.
struct pci_bar *ep_bar)
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
int flags = ep_bar->flags;
/* Some basic bar validity checks */
if ((ep_bar->barno == BAR_5 && flags &
PCI_BASE_ADDRESS_MEM_TYPE_64) ||
(flags & PCI_BASE_ADDRESS_SPACE_IO &&
flags & PCI_BASE_ADDRESS_IO_MASK) ||
Can you add another set of () in those two lines?
(upper_32_bits(ep_bar->size) &&
!(flags & PCI_BASE_ADDRESS_MEM_TYPE_64)))
return -EINVAL;
if (!ops->set_bar)
return -ENODEV;
return ops->set_bar(dev, func_no, ep_bar);
+}
+void pci_ep_clear_bar(struct udevice *dev, u8 func_num, enum pci_barno bar)
This should return an error code.
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (!ops->clear_bar)
return;
ops->clear_bar(dev, func_num, bar);
+}
+int dm_pci_ep_map_addr(struct udevice *dev, u8 func_no, phys_addr_t addr,
u64 pci_addr, size_t size)
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (!ops->map_addr)
return -ENODEV;
return ops->map_addr(dev, func_no, addr, pci_addr, size);
+}
+void dm_pci_ep_unmap_addr(struct udevice *dev, u8 func_no, phys_addr_t addr) +{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (!ops->unmap_addr)
return;
ops->unmap_addr(dev, func_no, addr);
+}
+int dm_pci_ep_set_msi(struct udevice *dev, u8 func_no, u8 interrupts)
Similar with interrupts (use uint).
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
u8 encode_int;
uint
if (interrupts > 32)
return -EINVAL;
if (!ops->set_msi)
return -ENODEV;
encode_int = order_base_2(interrupts);
return ops->set_msi(dev, func_no, encode_int);
+}
+int dm_pci_ep_get_msi(struct udevice *dev, u8 func_no) +{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
int interrupt;
if (!ops->get_msi)
return -ENODEV;
interrupt = ops->get_msi(dev, func_no);
if (interrupt < 0)
return 0;
interrupt = 1 << interrupt;
Perhaps you should declare a 'mask' variable? In any case, it is confusing for the same variable to have two different meanings, and it does not save code at run-time.
return interrupt;
+}
+int dm_pci_ep_set_msix(struct udevice *dev, u8 func_no, u16 interrupts)
s/u16/uint/
There is no saving by using u16, only potential cost. You range-check it below anyway. Please fix globally.
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (interrupts < 1 || interrupts > 2048)
return -EINVAL;
if (!ops->set_msix)
return -ENODEV;
return ops->set_msix(dev, func_no, interrupts);
+}
+int dm_pci_ep_get_msix(struct udevice *dev, u8 func_no) +{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
int interrupt;
if (!ops->get_msix)
return -ENODEV;
interrupt = ops->get_msix(dev, func_no);
if (interrupt < 0)
return 0;
return interrupt + 1;
It's odd that your uclass function returns a different value from your driver function. I think that will be confusing. Is it necessary?
+}
+int dm_pci_ep_raise_irq(struct udevice *dev, u8 func_no,
enum pci_ep_irq_type type, u16 interrupt_num)
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (!ops->raise_irq)
return -ENODEV;
return ops->raise_irq(dev, func_no, type, interrupt_num);
+}
+int dm_pci_ep_start(struct udevice *dev) +{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (!ops->start)
return -ENODEV;
return ops->start(dev);
+}
+void dm_pci_ep_stop(struct udevice *dev)
Needs a return value.
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (!ops->stop)
return;
ops->stop(dev);
+}
+UCLASS_DRIVER(pci_ep) = {
.id = UCLASS_PCI_EP,
.name = "pci_ep",
.flags = DM_UC_FLAG_SEQ_ALIAS,
+}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 86e59781b0..d29f7d5a34 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -67,6 +67,7 @@ enum uclass_id { UCLASS_PANEL_BACKLIGHT, /* Backlight controller for panel */ UCLASS_PCH, /* x86 platform controller hub */ UCLASS_PCI, /* PCI bus */
UCLASS_PCI_EP, /* PCI endpoint device */ UCLASS_PCI_GENERIC, /* Generic PCI bus device */ UCLASS_PHY, /* Physical Layer (PHY) device */ UCLASS_PINCONFIG, /* Pin configuration node device */
diff --git a/include/pci_ep.h b/include/pci_ep.h new file mode 100644 index 0000000000..c98453ccfa --- /dev/null +++ b/include/pci_ep.h @@ -0,0 +1,375 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- Adapted from Linux kernel driver
- Copyright (C) 2017 Texas Instruments
- Author: Kishon Vijay Abraham I kishon@ti.com
- (C) Copyright 2019
- Ramon Fried ramon.fried@gmail.com
- */
+#ifndef _PCI_EP_H +#define _PCI_EP_H
+#include <pci.h>
+/**
- enum pci_interrupt_pin - PCI INTx interrupt values
- @PCI_INTERRUPT_UNKNOWN: Unknown or unassigned interrupt
- @PCI_INTERRUPT_INTA: PCI INTA pin
- @PCI_INTERRUPT_INTB: PCI INTB pin
- @PCI_INTERRUPT_INTC: PCI INTC pin
- @PCI_INTERRUPT_INTD: PCI INTD pin
- Corresponds to values for legacy PCI INTx interrupts, as can be found in the
- PCI_INTERRUPT_PIN register.
- */
+enum pci_interrupt_pin {
PCI_INTERRUPT_UNKNOWN,
PCI_INTERRUPT_INTA,
PCI_INTERRUPT_INTB,
PCI_INTERRUPT_INTC,
PCI_INTERRUPT_INTD,
+};
+enum pci_barno {
BAR_0,
BAR_1,
BAR_2,
BAR_3,
BAR_4,
BAR_5,
+};
+enum pci_ep_irq_type {
PCI_EP_IRQ_UNKNOWN,
PCI_EP_IRQ_LEGACY,
PCI_EP_IRQ_MSI,
PCI_EP_IRQ_MSIX,
+};
+/**
- struct pci_bar - represents the BAR of EP device
What is a BAR and what is an EP? Please spell things out in comments, at least ones per file:
base-address register (BAR)
- @phys_addr: physical address that should be mapped to the BAR
- @size: the size of the address space present in BAR
Please add the other two also
- */
+struct pci_bar {
dma_addr_t phys_addr;
size_t size;
enum pci_barno barno;
int flags;
+};
+/**
- struct pci_ep_header - represents standard configuration header
- @vendorid: identifies device manufacturer
- @deviceid: identifies a particular device
- @revid: specifies a device-specific revision identifier
- @progif_code: identifies a specific register-level programming interface
- @subclass_code: identifies more specifically the function of the device
- @baseclass_code: broadly classifies the type of function the device performs
- @cache_line_size: specifies the system cacheline size in units of DWORDs
What is a DWORD? I think i is 32-bits, so just say that, since WORD is a confusing term on a non-16-bit machine.
- @subsys_vendor_id: vendor of the add-in card or subsystem
- @subsys_id: id specific to vendor
- @interrupt_pin: interrupt pin the device (or device function) uses
- */
+struct pci_ep_header {
u16 vendorid;
u16 deviceid;
u8 revid;
u8 progif_code;
u8 subclass_code;
u8 baseclass_code;
u8 cache_line_size;
u16 subsys_vendor_id;
u16 subsys_id;
enum pci_interrupt_pin interrupt_pin;
Shouldn't that be a u16 or something? The enum does not have a defined size, I think.
+};
+/* PCI endpoint operations */ +struct dm_pci_ep_ops {
/**
* write_header() - Write a PCI configuration space header
*
* @dev: device to write to
* @func_num: Function to fill
Is this the function number within the device? If so, how about 'Function number (within device) to write to'? Similarly below.
* @hdr: header to write
* @return 0 if OK, -ve on error
*/
int (*write_header)(struct udevice *dev, u8 func_num,
struct pci_ep_header *hdr);
/**
* read_header() - Read a PCI configuration space header
*
* @dev: device to write to
* @func_num: Function to fill
* @hdr: header to read to
* @return 0 if OK, -ve on error
*/
int (*read_header)(struct udevice *dev, u8 func_num,
struct pci_ep_header *hdr);
/**
* set_bar() - set BAR (Base Address Register)
Good to expand BAR as you have here
*
* @dev: device to set
* @func_num: Function to set
* @bar: bar data
* @return 0 if OK, -ve on error
*/
int (*set_bar)(struct udevice *dev, u8 func_num,
struct pci_bar *bar);
/**
* clear_bar() - clear BAR (Base Address Register)
*
* @dev: device to clear
* @func_num: Function to clear
* @bar: bar number
*/
void (*clear_bar)(struct udevice *dev, u8 func_num,
enum pci_barno bar);
/**
* map_addr() - map CPU address to PCI address
*
* outband region is used in order to generate PCI read/write
* transaction from local memory/write.
*
* @dev: device to set
* @func_num: Function to set
* @addr: local physical address base
* @pci_addr: pci address to translate to
* @size: region size
* @return 0 if OK, -ve on error
*/
int (*map_addr)(struct udevice *dev, u8 func_num,
phys_addr_t addr, u64 pci_addr, size_t size);
/**
* unmap_addr() - unmap CPU address to PCI address
*
* unmap previously mapped region.
*
* @dev: device to set
* @func_num: Function to set
* @addr: local physical address base
*/
void (*unmap_addr)(struct udevice *dev, u8 func_num,
phys_addr_t addr);
/**
* set_msi() - set msi capability property
*
* set the number of required MSI vectors the device
* needs for operation.
*
* @dev: device to set
* @func_num: Function to set
* @interrupts: required interrupts count
* @return 0 if OK, -ve on error
*/
int (*set_msi)(struct udevice *dev, u8 func_num, u8 interrupts);
/**
* get_msi() - get the number of MSI interrupts allocated by the host.
*
* Read the Multiple Message Enable bitfield from
* Message control register.
*
* @dev: device to use
* @func_num: Function to use
* @return msi count if OK, -EINVAL if msi were not enabled at host.
*/
int (*get_msi)(struct udevice *dev, u8 func_num);
/**
* set_msix() - set msix capability property
*
* set the number of required MSIx vectors the device
* needs for operation.
*
* @dev: device to set
* @func_num: Function to set
* @interrupts: required interrupts count
This is too vague, please expand it.
* @return 0 if OK, -ve on error
*/
int (*set_msix)(struct udevice *dev, u8 func_num, u16 interrupts);
/**
* get_msix() - get the number of MSIx interrupts allocated by the host.
*
* Read the Multiple Message Enable bitfield from
* Message control register.
*
* @dev: device to use
* @func_num: Function to use
* @return msi count if OK, -EINVAL if msi were not enabled at host.
*/
int (*get_msix)(struct udevice *dev, u8 func_num);
/**
* raise_irq() - raise a legacy, MSI or MSI-X interrupt
*
* @dev: device to set
* @func_num: Function to set
* @type: type of irq to send
* @interrupt_num: interrupt vector to use
* @return 0 if OK, -ve on error
*/
int (*raise_irq)(struct udevice *dev, u8 func_num,
enum pci_ep_irq_type type, u16 interrupt_num);
/**
* start() - start the PCI link
*
Needs more docs. What is the effect of this?
* @dev: device to set
* @return 0 if OK, -ve on error
*/
int (*start)(struct udevice *dev);
/**
* stop() - stop the PCI link
Needs more docs. What is the effect of this?
*
* @dev: device to set
*/
void (*stop)(struct udevice *dev);
+};
+#define pci_ep_get_ops(dev) ((struct dm_pci_ep_ops *)(dev)->driver->ops)
+/**
- pci_ep_write_header() - Write a PCI configuration space header
- @dev: device to write to
- @func_num: Function to fill
- @hdr: header to write
- @return 0 if OK, -ve on error
- */
+int pci_ep_write_header(struct udevice *dev, u8 func_num,
struct pci_ep_header *hdr);
+/**
- dm_pci_ep_read_header() - Read a PCI configuration space header
- @dev: device to write to
- @func_num: Function to fill
- @hdr: header to read to
- @return 0 if OK, -ve on error
- */
+int pci_ep_read_header(struct udevice *dev, u8 func_num,
struct pci_ep_header *hdr);
+/**
- pci_ep_set_bar() - set BAR (Base Address Register)
- @dev: device to set
- @func_num: Function to set
- @bar: bar data
- @return 0 if OK, -ve on error
- */
+int pci_ep_set_bar(struct udevice *dev, u8 func_num, struct pci_bar *bar);
+/**
- pci_ep_clear_bar() - clear BAR (Base Address Register)
- @dev: device to clear
- @func_num: Function to clear
- @bar: bar number
- */
+void pci_ep_clear_bar(struct udevice *dev, u8 func_num, enum pci_barno bar); +/**
- pci_ep_map_addr() - map CPU address to PCI address
- outband region is used in order to generate PCI read/write
- transaction from local memory/write.
- @dev: device to set
- @func_num: Function to set
- @addr: local physical address base
- @pci_addr: pci address to translate to
- @size: region size
- @return 0 if OK, -ve on error
- */
+int pci_ep_map_addr(struct udevice *dev, u8 func_num, phys_addr_t addr,
u64 pci_addr, size_t size);
+/**
- pci_ep_unmap_addr() - unmap CPU address to PCI address
- unmap previously mapped region.
- @dev: device to set
- @func_num: Function to set
- @addr: local physical address base
- */
+void pci_ep_unmap_addr(struct udevice *dev, u8 func_num, phys_addr_t addr); +/**
- pci_ep_set_msi() - set msi capability property
- set the number of required MSI vectors the device
- needs for operation.
- @dev: device to set
- @func_num: Function to set
- @interrupts: required interrupts count
- @return 0 if OK, -ve on error
- */
+int pci_ep_set_msi(struct udevice *dev, u8 func_num, u8 interrupts);
+/**
- pci_ep_get_msi() - get the number of MSI interrupts allocated by the host.
- Read the Multiple Message Enable bitfield from
- Message control register.
- @dev: device to use
- @func_num: Function to use
- @return msi count if OK, -EINVAL if msi were not enabled at host.
- */
+int pci_ep_get_msi(struct udevice *dev, u8 func_num);
+/**
- pci_ep_set_msix() - set msi capability property
- set the number of required MSIx vectors the device
- needs for operation.
- @dev: device to set
- @func_num: Function to set
- @interrupts: required interrupts count
- @return 0 if OK, -ve on error
- */
+int pci_ep_set_msix(struct udevice *dev, u8 func_num, u16 interrupts);
+/**
- pci_ep_get_msix() - get the number of MSIx interrupts allocated by the host.
- Read the Multiple Message Enable bitfield from
- Message control register.
- @dev: device to use
- @func_num: Function to use
- @return msi count if OK, -EINVAL if msi were not enabled at host.
- */
+int pci_ep_get_msix(struct udevice *dev, u8 func_num);
+/**
- pci_ep_raise_irq() - raise a legacy, MSI or MSI-X interrupt
- @dev: device to set
- @func_num: Function to set
- @type: type of irq to send
- @interrupt_num: interrupt vector to use
- @return 0 if OK, -ve on error
- */
+int pci_ep_raise_irq(struct udevice *dev, u8 func_num,
enum pci_ep_irq_type type, u16 interrupt_num);
+/**
- pci_ep_start() - start the PCI link
- @dev: device to set
- @return 0 if OK, -ve on error
- */
+int pci_ep_start(struct udevice *dev);
+/**
- pci_ep_stop() - stop the PCI link
- @dev: device to set
- */
+void pci_ep_stop(struct udevice *dev);
+#endif
2.21.0
Regards, Simon

Hi Simon, Thanks for the review. please see inline, I have few questions/suggestions regarding your notes.
Thanks, Ramon. On Mon, Apr 22, 2019 at 5:56 AM Simon Glass sjg@chromium.org wrote:
Hi Ramon,
On Fri, 5 Apr 2019 at 19:12, Ramon Fried ramon.fried@gmail.com wrote:
Introduce new UCLASS_PCI_EP class for handling PCI endpoint devices, allowing to set various attributes of the PCI endpoint device, such as:
- configuration space header
- BAR definitions
- outband memory mapping
- start/stop PCI link
Signed-off-by: Ramon Fried ramon.fried@gmail.com
drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/pci_endpoint/Kconfig | 16 ++ drivers/pci_endpoint/Makefile | 6 + drivers/pci_endpoint/pci_ep-uclass.c | 192 ++++++++++++++ include/dm/uclass-id.h | 1 + include/pci_ep.h | 375 +++++++++++++++++++++++++++ 7 files changed, 593 insertions(+) create mode 100644 drivers/pci_endpoint/Kconfig create mode 100644 drivers/pci_endpoint/Makefile create mode 100644 drivers/pci_endpoint/pci_ep-uclass.c create mode 100644 include/pci_ep.h
This looks pretty good but I have a lot of nits, sorry.
no worries.
Please can you add a sandbox driver and a test for this (can be in a separate patch).
sure.
diff --git a/drivers/Kconfig b/drivers/Kconfig index f24351ac4f..59e2c22cc6 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -64,6 +64,8 @@ source "drivers/nvme/Kconfig"
source "drivers/pci/Kconfig"
+source "drivers/pci_endpoint/Kconfig"
source "drivers/pch/Kconfig"
source "drivers/pcmcia/Kconfig" diff --git a/drivers/Makefile b/drivers/Makefile index a7bba3ed56..480b97ef58 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -85,6 +85,7 @@ obj-$(CONFIG_FPGA) += fpga/ obj-y += misc/ obj-$(CONFIG_MMC) += mmc/ obj-$(CONFIG_NVME) += nvme/ +obj-$(CONFIG_PCI_ENDPOINT) += pci_endpoint/ obj-y += pcmcia/ obj-y += dfu/ obj-$(CONFIG_PCH) += pch/ diff --git a/drivers/pci_endpoint/Kconfig b/drivers/pci_endpoint/Kconfig new file mode 100644 index 0000000000..2c0a399a08 --- /dev/null +++ b/drivers/pci_endpoint/Kconfig @@ -0,0 +1,16 @@ +# SPDX-License-Identifier: GPL-2.0 +# +# PCI Endpoint Support +#
+menu "PCI Endpoint"
+config PCI_ENDPOINT
bool "PCI Endpoint Support"
depends on DM
help
Enable this configuration option to support configurable PCI
endpoint. This should be enabled if the platform has a PCI
s/endpoint/endpoints/ I think
Right.
controller that can operate in endpoint mode.
Can you explain a bit more about what this means. I understand that
people can look at the spec, but what is the purpose of 'endpoint mode' and what does it enable?
+endmenu diff --git a/drivers/pci_endpoint/Makefile
b/drivers/pci_endpoint/Makefile
new file mode 100644 index 0000000000..80a1066925 --- /dev/null +++ b/drivers/pci_endpoint/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0+ +# +# (C) Copyright 2019 +# Ramon Fried ramon.fried@gmail.com
+obj-y += pci_ep-uclass.o diff --git a/drivers/pci_endpoint/pci_ep-uclass.c
b/drivers/pci_endpoint/pci_ep-uclass.c
new file mode 100644 index 0000000000..06fcfc5d14 --- /dev/null +++ b/drivers/pci_endpoint/pci_ep-uclass.c @@ -0,0 +1,192 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- PCI Endpoint uclass
- Based on Linux PCI-EP driver written by
- Kishon Vijay Abraham I kishon@ti.com
- Copyright (c) 2019
- Written by Ramon Fried ramon.fried@gmail.com
- */
+#include <common.h> +#include <dm.h> +#include <errno.h> +#include <linux/log2.h> +#include <pci_ep.h>
+DECLARE_GLOBAL_DATA_PTR;
+int dm_pci_ep_write_header(struct udevice *dev, u8 fn,
struct pci_ep_header *hdr)
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (!ops->write_header)
return -ENODEV;
-ENOSYS (please fix globally)
OK.
There is a device. The problem here is that the driver does not implement the method.
return ops->write_header(dev, fn, hdr);
+}
+int dm_pci_ep_read_header(struct udevice *dev, u8 fn,
struct pci_ep_header *hdr)
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (!ops->read_header)
return -ENODEV;
return ops->read_header(dev, fn, hdr);
+}
+int dm_pci_ep_set_bar(struct udevice *dev, u8 func_no,
Please use uint for func_no. There is no need to limit it to 8 bits, and this may not be efficient for non-8-bit machines. Please fix globally.
I tried to keep the API as similar as it can to the Linux API. I can change it, no problem, but IMHO I think it's better to keep it similar.
struct pci_bar *ep_bar)
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
int flags = ep_bar->flags;
/* Some basic bar validity checks */
if ((ep_bar->barno == BAR_5 && flags &
PCI_BASE_ADDRESS_MEM_TYPE_64) ||
(flags & PCI_BASE_ADDRESS_SPACE_IO &&
flags & PCI_BASE_ADDRESS_IO_MASK) ||
Can you add another set of () in those two lines?
yep
(upper_32_bits(ep_bar->size) &&
!(flags & PCI_BASE_ADDRESS_MEM_TYPE_64)))
return -EINVAL;
if (!ops->set_bar)
return -ENODEV;
return ops->set_bar(dev, func_no, ep_bar);
+}
+void pci_ep_clear_bar(struct udevice *dev, u8 func_num, enum pci_barno
bar)
This should return an error code.
ok
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (!ops->clear_bar)
return;
ops->clear_bar(dev, func_num, bar);
+}
+int dm_pci_ep_map_addr(struct udevice *dev, u8 func_no, phys_addr_t
addr,
u64 pci_addr, size_t size)
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (!ops->map_addr)
return -ENODEV;
return ops->map_addr(dev, func_no, addr, pci_addr, size);
+}
+void dm_pci_ep_unmap_addr(struct udevice *dev, u8 func_no, phys_addr_t
addr)
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (!ops->unmap_addr)
return;
ops->unmap_addr(dev, func_no, addr);
+}
+int dm_pci_ep_set_msi(struct udevice *dev, u8 func_no, u8 interrupts)
Similar with interrupts (use uint).
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
u8 encode_int;
uint
if (interrupts > 32)
return -EINVAL;
if (!ops->set_msi)
return -ENODEV;
encode_int = order_base_2(interrupts);
return ops->set_msi(dev, func_no, encode_int);
+}
+int dm_pci_ep_get_msi(struct udevice *dev, u8 func_no) +{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
int interrupt;
if (!ops->get_msi)
return -ENODEV;
interrupt = ops->get_msi(dev, func_no);
if (interrupt < 0)
return 0;
interrupt = 1 << interrupt;
Perhaps you should declare a 'mask' variable? In any case, it is confusing for the same variable to have two different meanings, and it does not save code at run-time.
as before, this is practically a copy-paste from Linux, I can change it, but I think it's clearer if the code looks the same as in Linux, might be easier the port patches like that.
return interrupt;
+}
+int dm_pci_ep_set_msix(struct udevice *dev, u8 func_no, u16 interrupts)
s/u16/uint/
There is no saving by using u16, only potential cost. You range-check it below anyway. Please fix globally.
ok
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (interrupts < 1 || interrupts > 2048)
return -EINVAL;
if (!ops->set_msix)
return -ENODEV;
return ops->set_msix(dev, func_no, interrupts);
+}
+int dm_pci_ep_get_msix(struct udevice *dev, u8 func_no) +{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
int interrupt;
if (!ops->get_msix)
return -ENODEV;
interrupt = ops->get_msix(dev, func_no);
if (interrupt < 0)
return 0;
return interrupt + 1;
It's odd that your uclass function returns a different value from your driver function. I think that will be confusing. Is it necessary?
As you can understand from the code, 0 means 1 interrupt. basically the driver functions just return the msix field in the PCI registers, The translation to real number is done by the uclass wrapper.
+}
+int dm_pci_ep_raise_irq(struct udevice *dev, u8 func_no,
enum pci_ep_irq_type type, u16 interrupt_num)
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (!ops->raise_irq)
return -ENODEV;
return ops->raise_irq(dev, func_no, type, interrupt_num);
+}
+int dm_pci_ep_start(struct udevice *dev) +{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (!ops->start)
return -ENODEV;
return ops->start(dev);
+}
+void dm_pci_ep_stop(struct udevice *dev)
Needs a return value.
ok
+{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
if (!ops->stop)
return;
ops->stop(dev);
+}
+UCLASS_DRIVER(pci_ep) = {
.id = UCLASS_PCI_EP,
.name = "pci_ep",
.flags = DM_UC_FLAG_SEQ_ALIAS,
+}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 86e59781b0..d29f7d5a34 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -67,6 +67,7 @@ enum uclass_id { UCLASS_PANEL_BACKLIGHT, /* Backlight controller for panel */ UCLASS_PCH, /* x86 platform controller hub */ UCLASS_PCI, /* PCI bus */
UCLASS_PCI_EP, /* PCI endpoint device */ UCLASS_PCI_GENERIC, /* Generic PCI bus device */ UCLASS_PHY, /* Physical Layer (PHY) device */ UCLASS_PINCONFIG, /* Pin configuration node device */
diff --git a/include/pci_ep.h b/include/pci_ep.h new file mode 100644 index 0000000000..c98453ccfa --- /dev/null +++ b/include/pci_ep.h @@ -0,0 +1,375 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- Adapted from Linux kernel driver
- Copyright (C) 2017 Texas Instruments
- Author: Kishon Vijay Abraham I kishon@ti.com
- (C) Copyright 2019
- Ramon Fried ramon.fried@gmail.com
- */
+#ifndef _PCI_EP_H +#define _PCI_EP_H
+#include <pci.h>
+/**
- enum pci_interrupt_pin - PCI INTx interrupt values
- @PCI_INTERRUPT_UNKNOWN: Unknown or unassigned interrupt
- @PCI_INTERRUPT_INTA: PCI INTA pin
- @PCI_INTERRUPT_INTB: PCI INTB pin
- @PCI_INTERRUPT_INTC: PCI INTC pin
- @PCI_INTERRUPT_INTD: PCI INTD pin
- Corresponds to values for legacy PCI INTx interrupts, as can be
found in the
- PCI_INTERRUPT_PIN register.
- */
+enum pci_interrupt_pin {
PCI_INTERRUPT_UNKNOWN,
PCI_INTERRUPT_INTA,
PCI_INTERRUPT_INTB,
PCI_INTERRUPT_INTC,
PCI_INTERRUPT_INTD,
+};
+enum pci_barno {
BAR_0,
BAR_1,
BAR_2,
BAR_3,
BAR_4,
BAR_5,
+};
+enum pci_ep_irq_type {
PCI_EP_IRQ_UNKNOWN,
PCI_EP_IRQ_LEGACY,
PCI_EP_IRQ_MSI,
PCI_EP_IRQ_MSIX,
+};
+/**
- struct pci_bar - represents the BAR of EP device
What is a BAR and what is an EP? Please spell things out in comments, at least ones per file:
base-address register (BAR)
- @phys_addr: physical address that should be mapped to the BAR
- @size: the size of the address space present in BAR
Please add the other two also
- */
+struct pci_bar {
dma_addr_t phys_addr;
size_t size;
enum pci_barno barno;
int flags;
+};
+/**
- struct pci_ep_header - represents standard configuration header
- @vendorid: identifies device manufacturer
- @deviceid: identifies a particular device
- @revid: specifies a device-specific revision identifier
- @progif_code: identifies a specific register-level programming
interface
- @subclass_code: identifies more specifically the function of the
device
- @baseclass_code: broadly classifies the type of function the device
performs
- @cache_line_size: specifies the system cacheline size in units of
DWORDs
What is a DWORD? I think i is 32-bits, so just say that, since WORD is a confusing term on a non-16-bit machine.
hehe, I presume it's just a copy-paste from the PCI spec. I'll check, if so, I'll prefer to keep the original naming.
- @subsys_vendor_id: vendor of the add-in card or subsystem
- @subsys_id: id specific to vendor
- @interrupt_pin: interrupt pin the device (or device function) uses
- */
+struct pci_ep_header {
u16 vendorid;
u16 deviceid;
u8 revid;
u8 progif_code;
u8 subclass_code;
u8 baseclass_code;
u8 cache_line_size;
u16 subsys_vendor_id;
u16 subsys_id;
enum pci_interrupt_pin interrupt_pin;
Shouldn't that be a u16 or something? The enum does not have a defined size, I think.
well, there can be only 4 different interrupt pins, so it doesn't matter as long as the rage is checked.
+};
+/* PCI endpoint operations */ +struct dm_pci_ep_ops {
/**
* write_header() - Write a PCI configuration space header
*
* @dev: device to write to
* @func_num: Function to fill
Is this the function number within the device? If so, how about 'Function number (within device) to write to'? Similarly below.
OK, I concur.
* @hdr: header to write
* @return 0 if OK, -ve on error
*/
int (*write_header)(struct udevice *dev, u8 func_num,
struct pci_ep_header *hdr);
/**
* read_header() - Read a PCI configuration space header
*
* @dev: device to write to
* @func_num: Function to fill
* @hdr: header to read to
* @return 0 if OK, -ve on error
*/
int (*read_header)(struct udevice *dev, u8 func_num,
struct pci_ep_header *hdr);
/**
* set_bar() - set BAR (Base Address Register)
Good to expand BAR as you have here
*
* @dev: device to set
* @func_num: Function to set
* @bar: bar data
* @return 0 if OK, -ve on error
*/
int (*set_bar)(struct udevice *dev, u8 func_num,
struct pci_bar *bar);
/**
* clear_bar() - clear BAR (Base Address Register)
*
* @dev: device to clear
* @func_num: Function to clear
* @bar: bar number
*/
void (*clear_bar)(struct udevice *dev, u8 func_num,
enum pci_barno bar);
/**
* map_addr() - map CPU address to PCI address
*
* outband region is used in order to generate PCI read/write
* transaction from local memory/write.
*
* @dev: device to set
* @func_num: Function to set
* @addr: local physical address base
* @pci_addr: pci address to translate to
* @size: region size
* @return 0 if OK, -ve on error
*/
int (*map_addr)(struct udevice *dev, u8 func_num,
phys_addr_t addr, u64 pci_addr, size_t size);
/**
* unmap_addr() - unmap CPU address to PCI address
*
* unmap previously mapped region.
*
* @dev: device to set
* @func_num: Function to set
* @addr: local physical address base
*/
void (*unmap_addr)(struct udevice *dev, u8 func_num,
phys_addr_t addr);
/**
* set_msi() - set msi capability property
*
* set the number of required MSI vectors the device
* needs for operation.
*
* @dev: device to set
* @func_num: Function to set
* @interrupts: required interrupts count
* @return 0 if OK, -ve on error
*/
int (*set_msi)(struct udevice *dev, u8 func_num, u8
interrupts);
/**
* get_msi() - get the number of MSI interrupts allocated by the
host.
*
* Read the Multiple Message Enable bitfield from
* Message control register.
*
* @dev: device to use
* @func_num: Function to use
* @return msi count if OK, -EINVAL if msi were not enabled at
host.
*/
int (*get_msi)(struct udevice *dev, u8 func_num);
/**
* set_msix() - set msix capability property
*
* set the number of required MSIx vectors the device
* needs for operation.
*
* @dev: device to set
* @func_num: Function to set
* @interrupts: required interrupts count
This is too vague, please expand it.
You're referring to the set_msix() or the whole section, can you elaborate ?
* @return 0 if OK, -ve on error
*/
int (*set_msix)(struct udevice *dev, u8 func_num, u16
interrupts);
/**
* get_msix() - get the number of MSIx interrupts allocated by
the host.
*
* Read the Multiple Message Enable bitfield from
* Message control register.
*
* @dev: device to use
* @func_num: Function to use
* @return msi count if OK, -EINVAL if msi were not enabled at
host.
*/
int (*get_msix)(struct udevice *dev, u8 func_num);
/**
* raise_irq() - raise a legacy, MSI or MSI-X interrupt
*
* @dev: device to set
* @func_num: Function to set
* @type: type of irq to send
* @interrupt_num: interrupt vector to use
* @return 0 if OK, -ve on error
*/
int (*raise_irq)(struct udevice *dev, u8 func_num,
enum pci_ep_irq_type type, u16
interrupt_num);
/**
* start() - start the PCI link
*
Needs more docs. What is the effect of this?
OK, I'll elaborate more.
* @dev: device to set
* @return 0 if OK, -ve on error
*/
int (*start)(struct udevice *dev);
/**
* stop() - stop the PCI link
Needs more docs. What is the effect of this?
*
* @dev: device to set
*/
void (*stop)(struct udevice *dev);
+};
+#define pci_ep_get_ops(dev) ((struct dm_pci_ep_ops
*)(dev)->driver->ops)
+/**
- pci_ep_write_header() - Write a PCI configuration space header
- @dev: device to write to
- @func_num: Function to fill
- @hdr: header to write
- @return 0 if OK, -ve on error
- */
+int pci_ep_write_header(struct udevice *dev, u8 func_num,
struct pci_ep_header *hdr);
+/**
- dm_pci_ep_read_header() - Read a PCI configuration space header
- @dev: device to write to
- @func_num: Function to fill
- @hdr: header to read to
- @return 0 if OK, -ve on error
- */
+int pci_ep_read_header(struct udevice *dev, u8 func_num,
struct pci_ep_header *hdr);
+/**
- pci_ep_set_bar() - set BAR (Base Address Register)
- @dev: device to set
- @func_num: Function to set
- @bar: bar data
- @return 0 if OK, -ve on error
- */
+int pci_ep_set_bar(struct udevice *dev, u8 func_num, struct pci_bar
*bar);
+/**
- pci_ep_clear_bar() - clear BAR (Base Address Register)
- @dev: device to clear
- @func_num: Function to clear
- @bar: bar number
- */
+void pci_ep_clear_bar(struct udevice *dev, u8 func_num, enum pci_barno
bar);
+/**
- pci_ep_map_addr() - map CPU address to PCI address
- outband region is used in order to generate PCI read/write
- transaction from local memory/write.
- @dev: device to set
- @func_num: Function to set
- @addr: local physical address base
- @pci_addr: pci address to translate to
- @size: region size
- @return 0 if OK, -ve on error
- */
+int pci_ep_map_addr(struct udevice *dev, u8 func_num, phys_addr_t addr,
u64 pci_addr, size_t size);
+/**
- pci_ep_unmap_addr() - unmap CPU address to PCI address
- unmap previously mapped region.
- @dev: device to set
- @func_num: Function to set
- @addr: local physical address base
- */
+void pci_ep_unmap_addr(struct udevice *dev, u8 func_num, phys_addr_t
addr);
+/**
- pci_ep_set_msi() - set msi capability property
- set the number of required MSI vectors the device
- needs for operation.
- @dev: device to set
- @func_num: Function to set
- @interrupts: required interrupts count
- @return 0 if OK, -ve on error
- */
+int pci_ep_set_msi(struct udevice *dev, u8 func_num, u8 interrupts);
+/**
- pci_ep_get_msi() - get the number of MSI interrupts allocated by the
host.
- Read the Multiple Message Enable bitfield from
- Message control register.
- @dev: device to use
- @func_num: Function to use
- @return msi count if OK, -EINVAL if msi were not enabled at host.
- */
+int pci_ep_get_msi(struct udevice *dev, u8 func_num);
+/**
- pci_ep_set_msix() - set msi capability property
- set the number of required MSIx vectors the device
- needs for operation.
- @dev: device to set
- @func_num: Function to set
- @interrupts: required interrupts count
- @return 0 if OK, -ve on error
- */
+int pci_ep_set_msix(struct udevice *dev, u8 func_num, u16 interrupts);
+/**
- pci_ep_get_msix() - get the number of MSIx interrupts allocated by
the host.
- Read the Multiple Message Enable bitfield from
- Message control register.
- @dev: device to use
- @func_num: Function to use
- @return msi count if OK, -EINVAL if msi were not enabled at host.
- */
+int pci_ep_get_msix(struct udevice *dev, u8 func_num);
+/**
- pci_ep_raise_irq() - raise a legacy, MSI or MSI-X interrupt
- @dev: device to set
- @func_num: Function to set
- @type: type of irq to send
- @interrupt_num: interrupt vector to use
- @return 0 if OK, -ve on error
- */
+int pci_ep_raise_irq(struct udevice *dev, u8 func_num,
enum pci_ep_irq_type type, u16 interrupt_num);
+/**
- pci_ep_start() - start the PCI link
- @dev: device to set
- @return 0 if OK, -ve on error
- */
+int pci_ep_start(struct udevice *dev);
+/**
- pci_ep_stop() - stop the PCI link
- @dev: device to set
- */
+void pci_ep_stop(struct udevice *dev);
+#endif
2.21.0
Regards, Simon

HI Ramon,
On Mon, 22 Apr 2019 at 10:33, Ramon Fried ramon.fried@gmail.com wrote:
Hi Simon, Thanks for the review. please see inline, I have few questions/suggestions regarding your notes.
Thanks, Ramon. On Mon, Apr 22, 2019 at 5:56 AM Simon Glass sjg@chromium.org wrote:
Hi Ramon,
On Fri, 5 Apr 2019 at 19:12, Ramon Fried ramon.fried@gmail.com wrote:
Introduce new UCLASS_PCI_EP class for handling PCI endpoint devices, allowing to set various attributes of the PCI endpoint device, such as:
- configuration space header
- BAR definitions
- outband memory mapping
- start/stop PCI link
Signed-off-by: Ramon Fried ramon.fried@gmail.com
drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/pci_endpoint/Kconfig | 16 ++ drivers/pci_endpoint/Makefile | 6 + drivers/pci_endpoint/pci_ep-uclass.c | 192 ++++++++++++++ include/dm/uclass-id.h | 1 + include/pci_ep.h | 375 +++++++++++++++++++++++++++ 7 files changed, 593 insertions(+) create mode 100644 drivers/pci_endpoint/Kconfig create mode 100644 drivers/pci_endpoint/Makefile create mode 100644 drivers/pci_endpoint/pci_ep-uclass.c create mode 100644 include/pci_ep.h
[..]
+int dm_pci_ep_set_bar(struct udevice *dev, u8 func_no,
Please use uint for func_no. There is no need to limit it to 8 bits, and this may not be efficient for non-8-bit machines. Please fix globally.
I tried to keep the API as similar as it can to the Linux API. I can change it, no problem, but IMHO I think it's better to keep it similar.
Hmm, why?
Perhaps you should declare a 'mask' variable? In any case, it is confusing for the same variable to have two different meanings, and it does not save code at run-time.
as before, this is practically a copy-paste from Linux, I can change it, but I think it's clearer if the code looks the same as in Linux, might be easier the port patches like that.
It's a minor thing so you can keep the linux code if you like.
[..]
+int dm_pci_ep_get_msix(struct udevice *dev, u8 func_no) +{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
int interrupt;
if (!ops->get_msix)
return -ENODEV;
interrupt = ops->get_msix(dev, func_no);
if (interrupt < 0)
return 0;
return interrupt + 1;
It's odd that your uclass function returns a different value from your driver function. I think that will be confusing. Is it necessary?
As you can understand from the code, 0 means 1 interrupt. basically the driver functions just return the msix field in the PCI registers, The translation to real number is done by the uclass wrapper.
OK, but why? Why not use the same return value for the drive methods and the uclass? If you are using a different API, then please rename one of the functions.
[..]
What is a DWORD? I think i is 32-bits, so just say that, since WORD is a confusing term on a non-16-bit machine.
hehe, I presume it's just a copy-paste from the PCI spec. I'll check, if so, I'll prefer to keep the original naming.
OK, then how about adding the length as well, since DWORD is pretty obscure these days.
- @subsys_vendor_id: vendor of the add-in card or subsystem
- @subsys_id: id specific to vendor
- @interrupt_pin: interrupt pin the device (or device function) uses
- */
+struct pci_ep_header {
u16 vendorid;
u16 deviceid;
u8 revid;
u8 progif_code;
u8 subclass_code;
u8 baseclass_code;
u8 cache_line_size;
u16 subsys_vendor_id;
u16 subsys_id;
enum pci_interrupt_pin interrupt_pin;
Shouldn't that be a u16 or something? The enum does not have a defined size, I think.
well, there can be only 4 different interrupt pins, so it doesn't matter as long as the rage is checked.
My concern is that if this is a hardware-mapped register, then the enum could be any size, and may not map over the correct bits.
If this is not a hardware-mapped register, then OK.
[..]
/**
* set_msix() - set msix capability property
*
* set the number of required MSIx vectors the device
* needs for operation.
*
* @dev: device to set
* @func_num: Function to set
* @interrupts: required interrupts count
This is too vague, please expand it.
You're referring to the set_msix() or the whole section, can you elaborate ?
I mean the interrupts line. Can you given example values perhaps? Does it mean 'number of interrupts required to be alllocated' or something like that?
[..]
Regards, Simon

On Thu, Apr 25, 2019 at 2:44 AM Simon Glass sjg@chromium.org wrote:
HI Ramon,
On Mon, 22 Apr 2019 at 10:33, Ramon Fried ramon.fried@gmail.com wrote:
Hi Simon, Thanks for the review. please see inline, I have few questions/suggestions regarding your notes.
Thanks, Ramon. On Mon, Apr 22, 2019 at 5:56 AM Simon Glass sjg@chromium.org wrote:
Hi Ramon,
On Fri, 5 Apr 2019 at 19:12, Ramon Fried ramon.fried@gmail.com wrote:
Introduce new UCLASS_PCI_EP class for handling PCI endpoint devices, allowing to set various attributes of the PCI endpoint device, such as:
- configuration space header
- BAR definitions
- outband memory mapping
- start/stop PCI link
Signed-off-by: Ramon Fried ramon.fried@gmail.com
drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/pci_endpoint/Kconfig | 16 ++ drivers/pci_endpoint/Makefile | 6 + drivers/pci_endpoint/pci_ep-uclass.c | 192 ++++++++++++++ include/dm/uclass-id.h | 1 + include/pci_ep.h | 375
+++++++++++++++++++++++++++
7 files changed, 593 insertions(+) create mode 100644 drivers/pci_endpoint/Kconfig create mode 100644 drivers/pci_endpoint/Makefile create mode 100644 drivers/pci_endpoint/pci_ep-uclass.c create mode 100644 include/pci_ep.h
[..]
+int dm_pci_ep_set_bar(struct udevice *dev, u8 func_no,
Please use uint for func_no. There is no need to limit it to 8 bits, and this may not be efficient for non-8-bit machines. Please fix globally.
I tried to keep the API as similar as it can to the Linux API. I can change it, no problem, but IMHO I think it's better to keep it
similar.
Hmm, why?
Nevermind, I already changed my mind :)
Perhaps you should declare a 'mask' variable? In any case, it is confusing for the same variable to have two different meanings, and it does not save code at run-time.
as before, this is practically a copy-paste from Linux, I can change it,
but I think it's clearer if the code looks the same as in Linux,
might be easier the port patches like that.
It's a minor thing so you can keep the linux code if you like.
[..]
+int dm_pci_ep_get_msix(struct udevice *dev, u8 func_no) +{
struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
int interrupt;
if (!ops->get_msix)
return -ENODEV;
interrupt = ops->get_msix(dev, func_no);
if (interrupt < 0)
return 0;
return interrupt + 1;
It's odd that your uclass function returns a different value from your driver function. I think that will be confusing. Is it necessary?
As you can understand from the code, 0 means 1 interrupt. basically the
driver functions just return the msix field in the PCI registers,
The translation to real number is done by the uclass wrapper.
OK, but why? Why not use the same return value for the drive methods and the uclass? If you are using a different API, then please rename one of the functions.
I'm here advocating for the Linux implementation, you're right, I'll change it.
[..]
What is a DWORD? I think i is 32-bits, so just say that, since WORD is a confusing term on a non-16-bit machine.
hehe, I presume it's just a copy-paste from the PCI spec. I'll check, if
so, I'll prefer to keep the original naming.
OK, then how about adding the length as well, since DWORD is pretty obscure these days.
OK, will do.
- @subsys_vendor_id: vendor of the add-in card or subsystem
- @subsys_id: id specific to vendor
- @interrupt_pin: interrupt pin the device (or device function) uses
- */
+struct pci_ep_header {
u16 vendorid;
u16 deviceid;
u8 revid;
u8 progif_code;
u8 subclass_code;
u8 baseclass_code;
u8 cache_line_size;
u16 subsys_vendor_id;
u16 subsys_id;
enum pci_interrupt_pin interrupt_pin;
Shouldn't that be a u16 or something? The enum does not have a defined size, I think.
well, there can be only 4 different interrupt pins, so it doesn't matter
as long as the rage
is checked.
My concern is that if this is a hardware-mapped register, then the enum could be any size, and may not map over the correct bits.
If this is not a hardware-mapped register, then OK.
Not mapped, phew.
[..]
/**
* set_msix() - set msix capability property
*
* set the number of required MSIx vectors the device
* needs for operation.
*
* @dev: device to set
* @func_num: Function to set
* @interrupts: required interrupts count
This is too vague, please expand it.
You're referring to the set_msix() or the whole section, can you elaborate ?
I mean the interrupts line. Can you given example values perhaps? Does it mean 'number of interrupts required to be alllocated' or something like that?
It's basically a request from the endpoint, it might not be fully fulfilled by the host, the endpoint needs to check later using get_msix() how many interrupts the host allocated for him. I'll add documentation for that.
[..]
Regards, Simon

PCI_MSI_FLAGS_MASKBIT was missing from include file, add it.
Signed-off-by: Ramon Fried ramon.fried@gmail.com ---
include/pci.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/pci.h b/include/pci.h index 5fb212cab1..508f7bca81 100644 --- a/include/pci.h +++ b/include/pci.h @@ -405,6 +405,7 @@ #define PCI_MSI_FLAGS_QSIZE 0x70 /* Message queue size configured */ #define PCI_MSI_FLAGS_QMASK 0x0e /* Maximum queue size available */ #define PCI_MSI_FLAGS_ENABLE 0x01 /* MSI feature enabled */ +#define PCI_MSI_FLAGS_MASKBIT 0x0100 /* Per-vector masking capable */ #define PCI_MSI_RFU 3 /* Rest of capability flags */ #define PCI_MSI_ADDRESS_LO 4 /* Lower 32 bits */ #define PCI_MSI_ADDRESS_HI 8 /* Upper 32 bits (if PCI_MSI_FLAGS_64BIT set) */

On Fri, 5 Apr 2019 at 19:12, Ramon Fried ramon.fried@gmail.com wrote:
PCI_MSI_FLAGS_MASKBIT was missing from include file, add it.
Signed-off-by: Ramon Fried ramon.fried@gmail.com
include/pci.h | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Simon Glass sjg@chromium.org

On Sun, 21 Apr 2019 at 20:57, Simon Glass sjg@chromium.org wrote:
On Fri, 5 Apr 2019 at 19:12, Ramon Fried ramon.fried@gmail.com wrote:
PCI_MSI_FLAGS_MASKBIT was missing from include file, add it.
Signed-off-by: Ramon Fried ramon.fried@gmail.com
include/pci.h | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot-dm, thanks!

Add Cadence PCIe endpoint driver supporting configuration of header, bars and MSI for device.
Signed-off-by: Ramon Fried ramon.fried@gmail.com ---
.../pci_endpoint/cdns,cdns-pcie-ep.txt | 18 + drivers/pci_endpoint/Kconfig | 9 + drivers/pci_endpoint/Makefile | 1 + drivers/pci_endpoint/pcie-cadence-ep.c | 177 ++++++++++ drivers/pci_endpoint/pcie-cadence.h | 309 ++++++++++++++++++ 5 files changed, 514 insertions(+) create mode 100644 doc/device-tree-bindings/pci_endpoint/cdns,cdns-pcie-ep.txt create mode 100644 drivers/pci_endpoint/pcie-cadence-ep.c create mode 100644 drivers/pci_endpoint/pcie-cadence.h
diff --git a/doc/device-tree-bindings/pci_endpoint/cdns,cdns-pcie-ep.txt b/doc/device-tree-bindings/pci_endpoint/cdns,cdns-pcie-ep.txt new file mode 100644 index 0000000000..7705430559 --- /dev/null +++ b/doc/device-tree-bindings/pci_endpoint/cdns,cdns-pcie-ep.txt @@ -0,0 +1,18 @@ +* Cadence PCIe endpoint controller + +Required properties: +- compatible: Should contain "cdns,cdns-pcie-ep" to identify the IP used. +- reg: Should contain the controller register base address. + +Optional properties: +- max-functions: Maximum number of functions that can be configured (default 1). +- cdns,max-outbound-regions: Set to maximum number of outbound regions (default 8) + +Example: + +pcie_ep@fc000000 { + compatible = "cdns,cdns-pcie-ep"; + reg = <0x0 0xfc000000 0x0 0x01000000>; + cdns,max-outbound-regions = <16>; + max-functions = /bits/ 8 <8>; +}; diff --git a/drivers/pci_endpoint/Kconfig b/drivers/pci_endpoint/Kconfig index 2c0a399a08..d4cf4227df 100644 --- a/drivers/pci_endpoint/Kconfig +++ b/drivers/pci_endpoint/Kconfig @@ -13,4 +13,13 @@ config PCI_ENDPOINT endpoint. This should be enabled if the platform has a PCI controller that can operate in endpoint mode.
+config PCIE_CADENCE_EP + bool "Cadence PCIe endpoint controller" + depends on OF + depends on PCI_ENDPOINT + help + Say Y here if you want to support the Cadence PCIe controller in + endpoint mode. This PCIe controller may be embedded into many + different vendors SoCs. + endmenu diff --git a/drivers/pci_endpoint/Makefile b/drivers/pci_endpoint/Makefile index 80a1066925..0a849deb19 100644 --- a/drivers/pci_endpoint/Makefile +++ b/drivers/pci_endpoint/Makefile @@ -4,3 +4,4 @@ # Ramon Fried ramon.fried@gmail.com
obj-y += pci_ep-uclass.o +obj-$(CONFIG_PCIE_CADENCE_EP) += pcie-cadence-ep.o diff --git a/drivers/pci_endpoint/pcie-cadence-ep.c b/drivers/pci_endpoint/pcie-cadence-ep.c new file mode 100644 index 0000000000..2b669c3c7a --- /dev/null +++ b/drivers/pci_endpoint/pcie-cadence-ep.c @@ -0,0 +1,177 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2019 + * Written by Ramon Fried ramon.fried@gmail.com + */ + +#include <common.h> +#include <dm.h> +#include <errno.h> +#include <pci_ep.h> +#include <linux/sizes.h> +#include <linux/log2.h> +#include "pcie-cadence.h" + +DECLARE_GLOBAL_DATA_PTR; + +static int cdns_write_header(struct udevice *dev, u8 fn, + struct pci_ep_header *hdr) +{ + struct cdns_pcie *pcie = dev_get_priv(dev); + + cdns_pcie_ep_fn_writew(pcie, fn, PCI_DEVICE_ID, hdr->deviceid); + cdns_pcie_ep_fn_writeb(pcie, fn, PCI_REVISION_ID, hdr->revid); + cdns_pcie_ep_fn_writeb(pcie, fn, PCI_CLASS_PROG, + hdr->progif_code); + cdns_pcie_ep_fn_writew(pcie, fn, PCI_CLASS_DEVICE, + hdr->subclass_code | + hdr->baseclass_code << 8); + cdns_pcie_ep_fn_writeb(pcie, fn, PCI_CACHE_LINE_SIZE, + hdr->cache_line_size); + cdns_pcie_ep_fn_writew(pcie, fn, PCI_SUBSYSTEM_ID, + hdr->subsys_id); + cdns_pcie_ep_fn_writeb(pcie, fn, PCI_INTERRUPT_PIN, + hdr->interrupt_pin); + + /* + * Vendor ID can only be modified from function 0, all other functions + * use the same vendor ID as function 0. + */ + if (fn == 0) { + /* Update the vendor IDs. */ + u32 id = CDNS_PCIE_LM_ID_VENDOR(hdr->vendorid) | + CDNS_PCIE_LM_ID_SUBSYS(hdr->subsys_vendor_id); + + cdns_pcie_writel(pcie, CDNS_PCIE_LM_ID, id); + } + + return 0; +} + +static int cdns_set_bar(struct udevice *dev, u8 fn, struct pci_bar *ep_bar) +{ + struct cdns_pcie *pcie = dev_get_priv(dev); + dma_addr_t bar_phys = ep_bar->phys_addr; + enum pci_barno bar = ep_bar->barno; + int flags = ep_bar->flags; + u32 addr0, addr1, reg, cfg, b, aperture, ctrl; + u64 sz; + + /* BAR size is 2^(aperture + 7) */ + sz = max_t(size_t, ep_bar->size, CDNS_PCIE_EP_MIN_APERTURE); + /* + * roundup_pow_of_two() returns an unsigned long, which is not suited + * for 64bit values. + */ + sz = 1ULL << fls64(sz - 1); + aperture = ilog2(sz) - 7; /* 128B -> 0, 256B -> 1, 512B -> 2, ... */ + + if ((flags & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) { + ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_IO_32BITS; + } else { + bool is_prefetch = !!(flags & PCI_BASE_ADDRESS_MEM_PREFETCH); + bool is_64bits = (sz > SZ_2G) | + !!(ep_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64); + + if (is_64bits && (bar & 1)) + return -EINVAL; + + if (is_64bits && !(flags & PCI_BASE_ADDRESS_MEM_TYPE_64)) + ep_bar->flags |= PCI_BASE_ADDRESS_MEM_TYPE_64; + + if (is_64bits && is_prefetch) + ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS; + else if (is_prefetch) + ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_32BITS; + else if (is_64bits) + ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_64BITS; + else + ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_32BITS; + } + + addr0 = lower_32_bits(bar_phys); + addr1 = upper_32_bits(bar_phys); + cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_EP_FUNC_BAR_ADDR0(fn, bar), + addr0); + cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_EP_FUNC_BAR_ADDR1(fn, bar), + addr1); + + if (bar < BAR_4) { + reg = CDNS_PCIE_LM_EP_FUNC_BAR_CFG0(fn); + b = bar; + } else { + reg = CDNS_PCIE_LM_EP_FUNC_BAR_CFG1(fn); + b = bar - BAR_4; + } + + cfg = cdns_pcie_readl(pcie, reg); + cfg &= ~(CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_APERTURE_MASK(b) | + CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_CTRL_MASK(b)); + cfg |= (CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_APERTURE(b, aperture) | + CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_CTRL(b, ctrl)); + cdns_pcie_writel(pcie, reg, cfg); + + return 0; +} + +static int cdns_set_msi(struct udevice *dev, u8 fn, u8 mmc) +{ + struct cdns_pcie *pcie = dev_get_priv(dev); + u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET; + + /* + * Set the Multiple Message Capable bitfield into the Message Control + * register. + */ + u16 flags; + + flags = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_MSI_FLAGS); + flags = (flags & ~PCI_MSI_FLAGS_QMASK) | (mmc << 1); + flags |= PCI_MSI_FLAGS_64BIT; + flags &= ~PCI_MSI_FLAGS_MASKBIT; + cdns_pcie_ep_fn_writew(pcie, fn, cap + PCI_MSI_FLAGS, flags); + + return 0; +} + +static struct dm_pci_ep_ops cdns_pci_ep_ops = { + .write_header = cdns_write_header, + .set_bar = cdns_set_bar, + .set_msi = cdns_set_msi, +}; + +static int cdns_pci_ep_probe(struct udevice *dev) +{ + struct cdns_pcie *pdata = dev_get_priv(dev); + + pdata->reg_base = (void *)devfdt_get_addr(dev); + if (!pdata->reg_base) + return -ENOMEM; + + pdata->max_functions = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev), + "max-functions", 1); + pdata->max_regions = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev), + "cdns,max-outbound-regions", 8); + + return 0; +} + +static int cdns_pci_ep_remove(struct udevice *dev) +{ + return 0; +} + +const struct udevice_id cadence_pci_ep_of_match[] = { + { .compatible = "cdns,cdns-pcie-ep" }, + { } +}; + +U_BOOT_DRIVER(cdns_pcie) = { + .name = "cdns,pcie-ep", + .id = UCLASS_PCI_EP, + .of_match = cadence_pci_ep_of_match, + .ops = &cdns_pci_ep_ops, + .probe = cdns_pci_ep_probe, + .remove = cdns_pci_ep_remove, + .priv_auto_alloc_size = sizeof(struct cdns_pcie), +}; diff --git a/drivers/pci_endpoint/pcie-cadence.h b/drivers/pci_endpoint/pcie-cadence.h new file mode 100644 index 0000000000..91630d35c3 --- /dev/null +++ b/drivers/pci_endpoint/pcie-cadence.h @@ -0,0 +1,309 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Cadence PCIe controlloer definitions + * Adapted from linux kernel driver. + * Copyright (c) 2017 Cadence + * + * Copyright (c) 2019 + * Written by Ramon Fried ramon.fried@gmail.com + */ + +#ifndef PCIE_CADENCE_H +#define PCIE_CADENCE_H + +#include <common.h> +#include <pci_ep.h> +#include <asm/io.h> + +/* + * Local Management Registers + */ +#define CDNS_PCIE_LM_BASE 0x00100000 + +/* Vendor ID Register */ +#define CDNS_PCIE_LM_ID (CDNS_PCIE_LM_BASE + 0x0044) +#define CDNS_PCIE_LM_ID_VENDOR_MASK GENMASK(15, 0) +#define CDNS_PCIE_LM_ID_VENDOR_SHIFT 0 +#define CDNS_PCIE_LM_ID_VENDOR(vid) \ + (((vid) << CDNS_PCIE_LM_ID_VENDOR_SHIFT) & CDNS_PCIE_LM_ID_VENDOR_MASK) +#define CDNS_PCIE_LM_ID_SUBSYS_MASK GENMASK(31, 16) +#define CDNS_PCIE_LM_ID_SUBSYS_SHIFT 16 +#define CDNS_PCIE_LM_ID_SUBSYS(sub) \ + (((sub) << CDNS_PCIE_LM_ID_SUBSYS_SHIFT) & CDNS_PCIE_LM_ID_SUBSYS_MASK) + +/* Root Port Requestor ID Register */ +#define CDNS_PCIE_LM_RP_RID (CDNS_PCIE_LM_BASE + 0x0228) +#define CDNS_PCIE_LM_RP_RID_MASK GENMASK(15, 0) +#define CDNS_PCIE_LM_RP_RID_SHIFT 0 +#define CDNS_PCIE_LM_RP_RID_(rid) \ + (((rid) << CDNS_PCIE_LM_RP_RID_SHIFT) & CDNS_PCIE_LM_RP_RID_MASK) + +/* Endpoint Bus and Device Number Register */ +#define CDNS_PCIE_LM_EP_ID (CDNS_PCIE_LM_BASE + 0x022c) +#define CDNS_PCIE_LM_EP_ID_DEV_MASK GENMASK(4, 0) +#define CDNS_PCIE_LM_EP_ID_DEV_SHIFT 0 +#define CDNS_PCIE_LM_EP_ID_BUS_MASK GENMASK(15, 8) +#define CDNS_PCIE_LM_EP_ID_BUS_SHIFT 8 + +/* Endpoint Function f BAR b Configuration Registers */ +#define CDNS_PCIE_LM_EP_FUNC_BAR_CFG0(fn) \ + (CDNS_PCIE_LM_BASE + 0x0240 + (fn) * 0x0008) +#define CDNS_PCIE_LM_EP_FUNC_BAR_CFG1(fn) \ + (CDNS_PCIE_LM_BASE + 0x0244 + (fn) * 0x0008) +#define CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_APERTURE_MASK(b) \ + (GENMASK(4, 0) << ((b) * 8)) +#define CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_APERTURE(b, a) \ + (((a) << ((b) * 8)) & CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_APERTURE_MASK(b)) +#define CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_CTRL_MASK(b) \ + (GENMASK(7, 5) << ((b) * 8)) +#define CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_CTRL(b, c) \ + (((c) << ((b) * 8 + 5)) & CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_CTRL_MASK(b)) + +/* Endpoint Function Configuration Register */ +#define CDNS_PCIE_LM_EP_FUNC_CFG (CDNS_PCIE_LM_BASE + 0x02c0) + +/* Root Complex BAR Configuration Register */ +#define CDNS_PCIE_LM_RC_BAR_CFG (CDNS_PCIE_LM_BASE + 0x0300) +#define CDNS_PCIE_LM_RC_BAR_CFG_BAR0_APERTURE_MASK GENMASK(5, 0) +#define CDNS_PCIE_LM_RC_BAR_CFG_BAR0_APERTURE(a) \ + (((a) << 0) & CDNS_PCIE_LM_RC_BAR_CFG_BAR0_APERTURE_MASK) +#define CDNS_PCIE_LM_RC_BAR_CFG_BAR0_CTRL_MASK GENMASK(8, 6) +#define CDNS_PCIE_LM_RC_BAR_CFG_BAR0_CTRL(c) \ + (((c) << 6) & CDNS_PCIE_LM_RC_BAR_CFG_BAR0_CTRL_MASK) +#define CDNS_PCIE_LM_RC_BAR_CFG_BAR1_APERTURE_MASK GENMASK(13, 9) +#define CDNS_PCIE_LM_RC_BAR_CFG_BAR1_APERTURE(a) \ + (((a) << 9) & CDNS_PCIE_LM_RC_BAR_CFG_BAR1_APERTURE_MASK) +#define CDNS_PCIE_LM_RC_BAR_CFG_BAR1_CTRL_MASK GENMASK(16, 14) +#define CDNS_PCIE_LM_RC_BAR_CFG_BAR1_CTRL(c) \ + (((c) << 14) & CDNS_PCIE_LM_RC_BAR_CFG_BAR1_CTRL_MASK) +#define CDNS_PCIE_LM_RC_BAR_CFG_PREFETCH_MEM_ENABLE BIT(17) +#define CDNS_PCIE_LM_RC_BAR_CFG_PREFETCH_MEM_32BITS 0 +#define CDNS_PCIE_LM_RC_BAR_CFG_PREFETCH_MEM_64BITS BIT(18) +#define CDNS_PCIE_LM_RC_BAR_CFG_IO_ENABLE BIT(19) +#define CDNS_PCIE_LM_RC_BAR_CFG_IO_16BITS 0 +#define CDNS_PCIE_LM_RC_BAR_CFG_IO_32BITS BIT(20) +#define CDNS_PCIE_LM_RC_BAR_CFG_CHECK_ENABLE BIT(31) + +/* BAR control values applicable to both Endpoint Function and Root Complex */ +#define CDNS_PCIE_LM_BAR_CFG_CTRL_DISABLED 0x0 +#define CDNS_PCIE_LM_BAR_CFG_CTRL_IO_32BITS 0x1 +#define CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_32BITS 0x4 +#define CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_32BITS 0x5 +#define CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_64BITS 0x6 +#define CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS 0x7 + +/* + * Endpoint Function Registers (PCI configuration space for endpoint functions) + */ +#define CDNS_PCIE_EP_FUNC_BASE(fn) (((fn) << 12) & GENMASK(19, 12)) + +#define CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET 0x90 + +/* + * Root Port Registers (PCI configuration space for the root port function) + */ +#define CDNS_PCIE_RP_BASE 0x00200000 + +/* + * Address Translation Registers + */ +#define CDNS_PCIE_AT_BASE 0x00400000 + +/* Region r Outbound AXI to PCIe Address Translation Register 0 */ +#define CDNS_PCIE_AT_OB_REGION_PCI_ADDR0(r) \ + (CDNS_PCIE_AT_BASE + 0x0000 + ((r) & 0x1f) * 0x0020) +#define CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_NBITS_MASK GENMASK(5, 0) +#define CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_NBITS(nbits) \ + (((nbits) - 1) & CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_NBITS_MASK) +#define CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_DEVFN_MASK GENMASK(19, 12) +#define CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_DEVFN(devfn) \ + (((devfn) << 12) & CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_DEVFN_MASK) +#define CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_BUS_MASK GENMASK(27, 20) +#define CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_BUS(bus) \ + (((bus) << 20) & CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_BUS_MASK) + +/* Region r Outbound AXI to PCIe Address Translation Register 1 */ +#define CDNS_PCIE_AT_OB_REGION_PCI_ADDR1(r) \ + (CDNS_PCIE_AT_BASE + 0x0004 + ((r) & 0x1f) * 0x0020) + +/* Region r Outbound PCIe Descriptor Register 0 */ +#define CDNS_PCIE_AT_OB_REGION_DESC0(r) \ + (CDNS_PCIE_AT_BASE + 0x0008 + ((r) & 0x1f) * 0x0020) +#define CDNS_PCIE_AT_OB_REGION_DESC0_TYPE_MASK GENMASK(3, 0) +#define CDNS_PCIE_AT_OB_REGION_DESC0_TYPE_MEM 0x2 +#define CDNS_PCIE_AT_OB_REGION_DESC0_TYPE_IO 0x6 +#define CDNS_PCIE_AT_OB_REGION_DESC0_TYPE_CONF_TYPE0 0xa +#define CDNS_PCIE_AT_OB_REGION_DESC0_TYPE_CONF_TYPE1 0xb +#define CDNS_PCIE_AT_OB_REGION_DESC0_TYPE_NORMAL_MSG 0xc +#define CDNS_PCIE_AT_OB_REGION_DESC0_TYPE_VENDOR_MSG 0xd +/* Bit 23 MUST be set in RC mode. */ +#define CDNS_PCIE_AT_OB_REGION_DESC0_HARDCODED_RID BIT(23) +#define CDNS_PCIE_AT_OB_REGION_DESC0_DEVFN_MASK GENMASK(31, 24) +#define CDNS_PCIE_AT_OB_REGION_DESC0_DEVFN(devfn) \ + (((devfn) << 24) & CDNS_PCIE_AT_OB_REGION_DESC0_DEVFN_MASK) + +/* Region r Outbound PCIe Descriptor Register 1 */ +#define CDNS_PCIE_AT_OB_REGION_DESC1(r) \ + (CDNS_PCIE_AT_BASE + 0x000c + ((r) & 0x1f) * 0x0020) +#define CDNS_PCIE_AT_OB_REGION_DESC1_BUS_MASK GENMASK(7, 0) +#define CDNS_PCIE_AT_OB_REGION_DESC1_BUS(bus) \ + ((bus) & CDNS_PCIE_AT_OB_REGION_DESC1_BUS_MASK) + +/* Region r AXI Region Base Address Register 0 */ +#define CDNS_PCIE_AT_OB_REGION_CPU_ADDR0(r) \ + (CDNS_PCIE_AT_BASE + 0x0018 + ((r) & 0x1f) * 0x0020) +#define CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS_MASK GENMASK(5, 0) +#define CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS(nbits) \ + (((nbits) - 1) & CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS_MASK) + +/* Region r AXI Region Base Address Register 1 */ +#define CDNS_PCIE_AT_OB_REGION_CPU_ADDR1(r) \ + (CDNS_PCIE_AT_BASE + 0x001c + ((r) & 0x1f) * 0x0020) + +/* Root Port BAR Inbound PCIe to AXI Address Translation Register */ +#define CDNS_PCIE_AT_IB_RP_BAR_ADDR0(bar) \ + (CDNS_PCIE_AT_BASE + 0x0800 + (bar) * 0x0008) +#define CDNS_PCIE_AT_IB_RP_BAR_ADDR0_NBITS_MASK GENMASK(5, 0) +#define CDNS_PCIE_AT_IB_RP_BAR_ADDR0_NBITS(nbits) \ + (((nbits) - 1) & CDNS_PCIE_AT_IB_RP_BAR_ADDR0_NBITS_MASK) +#define CDNS_PCIE_AT_IB_RP_BAR_ADDR1(bar) \ + (CDNS_PCIE_AT_BASE + 0x0804 + (bar) * 0x0008) + +/* AXI link down register */ +#define CDNS_PCIE_AT_LINKDOWN (CDNS_PCIE_AT_BASE + 0x0824) + +enum cdns_pcie_rp_bar { + RP_BAR0, + RP_BAR1, + RP_NO_BAR +}; + +/* Endpoint Function BAR Inbound PCIe to AXI Address Translation Register */ +#define CDNS_PCIE_AT_IB_EP_FUNC_BAR_ADDR0(fn, bar) \ + (CDNS_PCIE_AT_BASE + 0x0840 + (fn) * 0x0040 + (bar) * 0x0008) +#define CDNS_PCIE_AT_IB_EP_FUNC_BAR_ADDR1(fn, bar) \ + (CDNS_PCIE_AT_BASE + 0x0844 + (fn) * 0x0040 + (bar) * 0x0008) + +/* Normal/Vendor specific message access: offset inside some outbound region */ +#define CDNS_PCIE_NORMAL_MSG_ROUTING_MASK GENMASK(7, 5) +#define CDNS_PCIE_NORMAL_MSG_ROUTING(route) \ + (((route) << 5) & CDNS_PCIE_NORMAL_MSG_ROUTING_MASK) +#define CDNS_PCIE_NORMAL_MSG_CODE_MASK GENMASK(15, 8) +#define CDNS_PCIE_NORMAL_MSG_CODE(code) \ + (((code) << 8) & CDNS_PCIE_NORMAL_MSG_CODE_MASK) +#define CDNS_PCIE_MSG_NO_DATA BIT(16) + +#define CDNS_PCIE_EP_MIN_APERTURE 128 /* 128 bytes */ + +enum cdns_pcie_msg_code { + MSG_CODE_ASSERT_INTA = 0x20, + MSG_CODE_ASSERT_INTB = 0x21, + MSG_CODE_ASSERT_INTC = 0x22, + MSG_CODE_ASSERT_INTD = 0x23, + MSG_CODE_DEASSERT_INTA = 0x24, + MSG_CODE_DEASSERT_INTB = 0x25, + MSG_CODE_DEASSERT_INTC = 0x26, + MSG_CODE_DEASSERT_INTD = 0x27, +}; + +enum cdns_pcie_msg_routing { + /* Route to Root Complex */ + MSG_ROUTING_TO_RC, + + /* Use Address Routing */ + MSG_ROUTING_BY_ADDR, + + /* Use ID Routing */ + MSG_ROUTING_BY_ID, + + /* Route as Broadcast Message from Root Complex */ + MSG_ROUTING_BCAST, + + /* Local message; terminate at receiver (INTx messages) */ + MSG_ROUTING_LOCAL, + + /* Gather & route to Root Complex (PME_TO_Ack message) */ + MSG_ROUTING_GATHER, +}; + +struct cdns_pcie { + void __iomem *reg_base; + u32 max_functions; + u32 max_regions; +}; + +/* Register access */ +static inline void cdns_pcie_writeb(struct cdns_pcie *pcie, u32 reg, u8 value) +{ + writeb(value, pcie->reg_base + reg); +} + +static inline void cdns_pcie_writew(struct cdns_pcie *pcie, u32 reg, u16 value) +{ + writew(value, pcie->reg_base + reg); +} + +static inline void cdns_pcie_writel(struct cdns_pcie *pcie, u32 reg, u32 value) +{ + writel(value, pcie->reg_base + reg); +} + +static inline u32 cdns_pcie_readl(struct cdns_pcie *pcie, u32 reg) +{ + return readl(pcie->reg_base + reg); +} + +/* Root Port register access */ +static inline void cdns_pcie_rp_writeb(struct cdns_pcie *pcie, + u32 reg, u8 value) +{ + writeb(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg); +} + +static inline void cdns_pcie_rp_writew(struct cdns_pcie *pcie, + u32 reg, u16 value) +{ + writew(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg); +} + +static inline void cdns_pcie_rp_writel(struct cdns_pcie *pcie, + u32 reg, u32 value) +{ + writel(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg); +} + +/* Endpoint Function register access */ +static inline void cdns_pcie_ep_fn_writeb(struct cdns_pcie *pcie, u8 fn, + u32 reg, u8 value) +{ + writeb(value, pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg); +} + +static inline void cdns_pcie_ep_fn_writew(struct cdns_pcie *pcie, u8 fn, + u32 reg, u16 value) +{ + writew(value, pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg); +} + +static inline void cdns_pcie_ep_fn_writel(struct cdns_pcie *pcie, u8 fn, + u32 reg, u32 value) +{ + writel(value, pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg); +} + +static inline u8 cdns_pcie_ep_fn_readb(struct cdns_pcie *pcie, u8 fn, u32 reg) +{ + return readb(pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg); +} + +static inline u16 cdns_pcie_ep_fn_readw(struct cdns_pcie *pcie, u8 fn, u32 reg) +{ + return readw(pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg); +} + +static inline u32 cdns_pcie_ep_fn_readl(struct cdns_pcie *pcie, u8 fn, u32 reg) +{ + return readl(pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg); +} + +#endif /* end of include guard: PCIE_CADENCE_H */

On Fri, 5 Apr 2019 at 19:12, Ramon Fried ramon.fried@gmail.com wrote:
Add Cadence PCIe endpoint driver supporting configuration of header, bars and MSI for device.
Signed-off-by: Ramon Fried ramon.fried@gmail.com
.../pci_endpoint/cdns,cdns-pcie-ep.txt | 18 + drivers/pci_endpoint/Kconfig | 9 + drivers/pci_endpoint/Makefile | 1 + drivers/pci_endpoint/pcie-cadence-ep.c | 177 ++++++++++ drivers/pci_endpoint/pcie-cadence.h | 309 ++++++++++++++++++ 5 files changed, 514 insertions(+) create mode 100644 doc/device-tree-bindings/pci_endpoint/cdns,cdns-pcie-ep.txt create mode 100644 drivers/pci_endpoint/pcie-cadence-ep.c create mode 100644 drivers/pci_endpoint/pcie-cadence.h
Looks like tidy code. I'll await the updated uclass patch.
- Simon
participants (2)
-
Ramon Fried
-
Simon Glass