Hi Ramon,
On Thu, 25 Apr 2019 at 05:35, Ramon Fried ramon.fried@gmail.com wrote:
On Thu, Apr 25, 2019 at 2:29 AM Simon Glass sjg@chromium.org wrote:
Hi Ramon,
On Wed, 24 Apr 2019 at 12:54, Ramon Fried ramon.fried@gmail.com wrote:
Please add a commit message.
Signed-off-by: Ramon Fried ramon.fried@gmail.com
arch/sandbox/dts/test.dts | 4 + configs/sandbox64_defconfig | 2 + configs/sandbox_defconfig | 2 + drivers/pci_endpoint/Kconfig | 7 + drivers/pci_endpoint/Makefile | 1 + drivers/pci_endpoint/sandbox-pci_ep.c | 176 ++++++++++++++++++++++++++ 6 files changed, 192 insertions(+) create mode 100644 drivers/pci_endpoint/sandbox-pci_ep.c
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index 8b2d6451c6..001dc302ed 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -475,6 +475,10 @@ }; };
pci_ep: pci_ep {compatible = "sandbox,pci_ep";};probing { compatible = "simple-bus"; test1 {diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig index c04ecd915a..7137ea481c 100644 --- a/configs/sandbox64_defconfig +++ b/configs/sandbox64_defconfig @@ -127,9 +127,11 @@ CONFIG_SPI_FLASH_WINBOND=y CONFIG_DM_ETH=y CONFIG_NVME=y CONFIG_PCI=y +CONFIG_PCI_ENDPOINT=y
It might be better to 'imply' this in the sandbox Kconfig file.
arch/sandbox/Kconfig doesn't include any implies or selects for drivers, am I looking at the wrong file ?
See arch/Kconfig
CONFIG_DM_PCI=y CONFIG_DM_PCI_COMPAT=y CONFIG_PCI_SANDBOX=y +CONFIG_PCI_SANDBOX_EP=y CONFIG_PHY=y CONFIG_PHY_SANDBOX=y CONFIG_PINCTRL=y diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index bb508a8d02..04ba9a3ba1 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -142,9 +142,11 @@ CONFIG_SPI_FLASH_WINBOND=y CONFIG_DM_ETH=y CONFIG_NVME=y CONFIG_PCI=y +CONFIG_PCI_ENDPOINT=y CONFIG_DM_PCI=y CONFIG_DM_PCI_COMPAT=y CONFIG_PCI_SANDBOX=y +CONFIG_PCI_SANDBOX_EP=y CONFIG_PHY=y CONFIG_PHY_SANDBOX=y CONFIG_PINCTRL=y diff --git a/drivers/pci_endpoint/Kconfig b/drivers/pci_endpoint/Kconfig index c54bd2a9ac..e529e560fc 100644 --- a/drivers/pci_endpoint/Kconfig +++ b/drivers/pci_endpoint/Kconfig @@ -22,4 +22,11 @@ config PCIE_CADENCE_EP endpoint mode. This PCIe controller may be embedded into many different vendors SoCs.
+config PCI_SANDBOX_EP
bool "Sandbox PCIe endpoint controller"depends on PCI_ENDPOINThelpSay Y here if you want to support the Sandbox PCIe controller inUse single space after PCIe
endpoint mode.How about another few sentences saying what the driver does?
endmenu diff --git a/drivers/pci_endpoint/Makefile b/drivers/pci_endpoint/Makefile index 0a849deb19..3cd987259d 100644 --- a/drivers/pci_endpoint/Makefile +++ b/drivers/pci_endpoint/Makefile @@ -5,3 +5,4 @@
obj-y += pci_ep-uclass.o obj-$(CONFIG_PCIE_CADENCE_EP) += pcie-cadence-ep.o +obj-$(CONFIG_PCI_SANDBOX_EP) += sandbox-pci_ep.o diff --git a/drivers/pci_endpoint/sandbox-pci_ep.c b/drivers/pci_endpoint/sandbox-pci_ep.c new file mode 100644 index 0000000000..eb19c6da81 --- /dev/null +++ b/drivers/pci_endpoint/sandbox-pci_ep.c @@ -0,0 +1,176 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Copyright (c) 2019 Ramon Fried ramon.fried@gmail.com
- */
+#include <common.h> +#include <dm.h> +#include <errno.h>
I think you can drop this
+#include <pci.h> +#include <pci_ep.h> +#include <asm/test.h>
struct comment, explaining each member.
+struct sandbox_pci_ep_priv {
struct pci_ep_header hdr;int msix;int msi;+};
+static const struct udevice_id sandbox_pci_ep_ids[] = {
{ .compatible = "sandbox,pci_ep" },{ }+};
+static int sandbox_write_header(struct udevice *dev, uint fn,
struct pci_ep_header *hdr)+{
struct sandbox_pci_ep_priv *priv = dev_get_priv(dev);if (fn > 0)return -ENODEV;memcpy(&priv->hdr, hdr, sizeof(*hdr));return 0;+}
+static int sandbox_read_header(struct udevice *dev, uint fn,
struct pci_ep_header *hdr)+{
struct sandbox_pci_ep_priv *priv = dev_get_priv(dev);if (fn > 0)return -ENODEV;memcpy(hdr, &priv->hdr, sizeof(*hdr));return 0;+}
+static int sandbox_set_bar(struct udevice *dev, uint fn,
struct pci_bar *ep_bar)+{
if (fn > 0)return -ENODEV;blank line before return. Please fix globally.
return 0;+}
+int sandbox_clear_bar(struct udevice *dev, uint fn,
static int?
enum pci_barno bar)+{
if (fn > 0)return -ENODEV;return 0;+}
+static int sandbox_map_addr(struct udevice *dev, uint fn, phys_addr_t addr,
u64 pci_addr, size_t size)+{
if (fn > 0)return -ENODEV;return 0;+}
+static void sandbox_unmap_addr(struct udevice *dev, uint fn, phys_addr_t addr)
This should have a return value
this practically behaves like free(), why should it have a return value ?
Well you have the answer two lines below. It should return -EINVAL, not void:
+{
if (fn > 0)return;+}
+static int sandbox_set_msi(struct udevice *dev, uint fn, uint interrupts) +{
struct sandbox_pci_ep_priv *priv = dev_get_priv(dev);if (fn > 0)return -ENODEV;priv->msi = interrupts;return 0;+}
+static int sandbox_get_msi(struct udevice *dev, uint fn) +{
struct sandbox_pci_ep_priv *priv = dev_get_priv(dev);if (fn > 0)return -ENODEV;return priv->msi;+}
+static int sandbox_set_msix(struct udevice *dev, uint fn, uint interrupts) +{
struct sandbox_pci_ep_priv *priv = dev_get_priv(dev);if (fn > 0)return -ENODEV;priv->msix = interrupts;return 0;+}
+static int sandbox_get_msix(struct udevice *dev, uint fn) +{
struct sandbox_pci_ep_priv *priv = dev_get_priv(dev);if (fn > 0)return -ENODEV;return priv->msix;+}
+static int sandbox_raise_irq(struct udevice *dev, uint fn,
enum pci_ep_irq_type type, uint interrupt_num)+{
if (fn > 0)return -ENODEV;return 0;+}
+static int sandbox_start(struct udevice *dev) +{
return 0;+}
+static int sandbox_stop(struct udevice *dev) +{
return 0;+}
+static int sandbox_pci_ep_probe(struct udevice *dev) +{
struct sandbox_pci_ep_priv *priv = dev_get_priv(dev);memset(priv, 0, sizeof(*priv));return 0;+}
+static struct pci_ep_ops sandbox_pci_ep_ops = {
.write_header = sandbox_write_header,.read_header = sandbox_read_header,.set_bar = sandbox_set_bar,.clear_bar = sandbox_clear_bar,.map_addr = sandbox_map_addr,.unmap_addr = sandbox_unmap_addr,.set_msi = sandbox_set_msi,.get_msi = sandbox_get_msi,.set_msix = sandbox_set_msix,.get_msix = sandbox_get_msix,.raise_irq = sandbox_raise_irq,.start = sandbox_start,.stop = sandbox_stop,+};
+U_BOOT_DRIVER(pci_ep_sandbox) = {
.name = "pci_ep_sandbox",.id = UCLASS_PCI_EP,.of_match = sandbox_pci_ep_ids,.probe = sandbox_pci_ep_probe,.ops = &sandbox_pci_ep_ops,.priv_auto_alloc_size = sizeof(struct sandbox_pci_ep_priv),+};
You also need an actual test here, perhaps in test/dm/pci.c, which calls each of these methods and checks (perhaps via a back-channel direct call into the driver) that they work. You don't have to be exhaustive, just a basic test for each method.
Not sure I understand, how do you back-channel a driver (removing static ?).
Yes that's right. Because it is sandbox this is acceptable, and you can add a prototype to arch/sandbox/include/asm/test.h
See test/dm/sound.c for an example.
It is also possible to add the platdata struct to that header, include it in the test and access the platdata (or priv data) from the test to check that things are ending up their correctly. Another way that I'm trying to get away from is to put things in struct sandbox_state.
Regards, Simon