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 DMhelpEnable this configuration option to support configurable PCIendpoint. This should be enabled if the platform has a PCIs/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 fillIs 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, u8interrupts);
/*** get_msi() - get the number of MSI interrupts allocated by thehost.
** 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 athost.
*/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 countThis 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, u16interrupts);
/*** get_msix() - get the number of MSIx interrupts allocated bythe 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 athost.
*/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, u16interrupt_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 linkNeeds 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