* [U-Boot] [PATCH v1 0/3] Add Cadence PCIe endpoint driver with new uclass
@ 2019-04-06 2:11 Ramon Fried
2019-04-06 2:12 ` [U-Boot] [PATCH v1 1/3] drivers: pci_ep: Introduce UCLASS_PCI_EP uclass Ramon Fried
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Ramon Fried @ 2019-04-06 2:11 UTC (permalink / raw)
To: u-boot
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
--
2.21.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v1 1/3] drivers: pci_ep: Introduce UCLASS_PCI_EP uclass
2019-04-06 2:11 [U-Boot] [PATCH v1 0/3] Add Cadence PCIe endpoint driver with new uclass Ramon Fried
@ 2019-04-06 2:12 ` Ramon Fried
2019-04-22 2:56 ` Simon Glass
2019-04-06 2:12 ` [U-Boot] [PATCH v1 2/3] pci: pci.h: add missing maskbit Ramon Fried
2019-04-06 2:12 ` [U-Boot] [PATCH v1 3/3] pci_ep: add Cadence PCIe endpoint driver Ramon Fried
2 siblings, 1 reply; 11+ messages in thread
From: Ramon Fried @ 2019-04-06 2:12 UTC (permalink / raw)
To: u-boot
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
--
2.21.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v1 2/3] pci: pci.h: add missing maskbit
2019-04-06 2:11 [U-Boot] [PATCH v1 0/3] Add Cadence PCIe endpoint driver with new uclass Ramon Fried
2019-04-06 2:12 ` [U-Boot] [PATCH v1 1/3] drivers: pci_ep: Introduce UCLASS_PCI_EP uclass Ramon Fried
@ 2019-04-06 2:12 ` Ramon Fried
2019-04-22 2:57 ` Simon Glass
2019-04-06 2:12 ` [U-Boot] [PATCH v1 3/3] pci_ep: add Cadence PCIe endpoint driver Ramon Fried
2 siblings, 1 reply; 11+ messages in thread
From: Ramon Fried @ 2019-04-06 2:12 UTC (permalink / raw)
To: u-boot
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) */
--
2.21.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v1 3/3] pci_ep: add Cadence PCIe endpoint driver
2019-04-06 2:11 [U-Boot] [PATCH v1 0/3] Add Cadence PCIe endpoint driver with new uclass Ramon Fried
2019-04-06 2:12 ` [U-Boot] [PATCH v1 1/3] drivers: pci_ep: Introduce UCLASS_PCI_EP uclass Ramon Fried
2019-04-06 2:12 ` [U-Boot] [PATCH v1 2/3] pci: pci.h: add missing maskbit Ramon Fried
@ 2019-04-06 2:12 ` Ramon Fried
2019-04-22 2:56 ` Simon Glass
2 siblings, 1 reply; 11+ messages in thread
From: Ramon Fried @ 2019-04-06 2:12 UTC (permalink / raw)
To: u-boot
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 at 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@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 */
--
2.21.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v1 1/3] drivers: pci_ep: Introduce UCLASS_PCI_EP uclass
2019-04-06 2:12 ` [U-Boot] [PATCH v1 1/3] drivers: pci_ep: Introduce UCLASS_PCI_EP uclass Ramon Fried
@ 2019-04-22 2:56 ` Simon Glass
2019-04-22 16:32 ` Ramon Fried
0 siblings, 1 reply; 11+ messages in thread
From: Simon Glass @ 2019-04-22 2:56 UTC (permalink / raw)
To: u-boot
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@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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v1 3/3] pci_ep: add Cadence PCIe endpoint driver
2019-04-06 2:12 ` [U-Boot] [PATCH v1 3/3] pci_ep: add Cadence PCIe endpoint driver Ramon Fried
@ 2019-04-22 2:56 ` Simon Glass
0 siblings, 0 replies; 11+ messages in thread
From: Simon Glass @ 2019-04-22 2:56 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v1 2/3] pci: pci.h: add missing maskbit
2019-04-06 2:12 ` [U-Boot] [PATCH v1 2/3] pci: pci.h: add missing maskbit Ramon Fried
@ 2019-04-22 2:57 ` Simon Glass
2019-04-24 15:17 ` Simon Glass
0 siblings, 1 reply; 11+ messages in thread
From: Simon Glass @ 2019-04-22 2:57 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v1 1/3] drivers: pci_ep: Introduce UCLASS_PCI_EP uclass
2019-04-22 2:56 ` Simon Glass
@ 2019-04-22 16:32 ` Ramon Fried
2019-04-24 23:44 ` Simon Glass
0 siblings, 1 reply; 11+ messages in thread
From: Ramon Fried @ 2019-04-22 16:32 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v1 2/3] pci: pci.h: add missing maskbit
2019-04-22 2:57 ` Simon Glass
@ 2019-04-24 15:17 ` Simon Glass
0 siblings, 0 replies; 11+ messages in thread
From: Simon Glass @ 2019-04-24 15:17 UTC (permalink / raw)
To: u-boot
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!
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v1 1/3] drivers: pci_ep: Introduce UCLASS_PCI_EP uclass
2019-04-22 16:32 ` Ramon Fried
@ 2019-04-24 23:44 ` Simon Glass
2019-04-25 2:55 ` Ramon Fried
0 siblings, 1 reply; 11+ messages in thread
From: Simon Glass @ 2019-04-24 23:44 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v1 1/3] drivers: pci_ep: Introduce UCLASS_PCI_EP uclass
2019-04-24 23:44 ` Simon Glass
@ 2019-04-25 2:55 ` Ramon Fried
0 siblings, 0 replies; 11+ messages in thread
From: Ramon Fried @ 2019-04-25 2:55 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-04-25 2:55 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-06 2:11 [U-Boot] [PATCH v1 0/3] Add Cadence PCIe endpoint driver with new uclass Ramon Fried
2019-04-06 2:12 ` [U-Boot] [PATCH v1 1/3] drivers: pci_ep: Introduce UCLASS_PCI_EP uclass Ramon Fried
2019-04-22 2:56 ` Simon Glass
2019-04-22 16:32 ` Ramon Fried
2019-04-24 23:44 ` Simon Glass
2019-04-25 2:55 ` Ramon Fried
2019-04-06 2:12 ` [U-Boot] [PATCH v1 2/3] pci: pci.h: add missing maskbit Ramon Fried
2019-04-22 2:57 ` Simon Glass
2019-04-24 15:17 ` Simon Glass
2019-04-06 2:12 ` [U-Boot] [PATCH v1 3/3] pci_ep: add Cadence PCIe endpoint driver Ramon Fried
2019-04-22 2:56 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.