Hi Ramon, On Fri, 5 Apr 2019 at 19:12, Ramon Fried <ramon.fr...@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.fr...@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.fr...@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 <kis...@ti.com> > + * > + * Copyright (c) 2019 > + * Written by Ramon Fried <ramon.fr...@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 <kis...@ti.com> > + * > + * (C) Copyright 2019 > + * Ramon Fried <ramon.fr...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot