On Thu, Apr 25, 2019 at 2:29 AM Simon Glass <s...@chromium.org> wrote: > > Hi Ramon, > > On Wed, 24 Apr 2019 at 12:54, Ramon Fried <ramon.fr...@gmail.com> wrote: > > Please add a commit message. > > > > > Signed-off-by: Ramon Fried <ramon.fr...@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 ? > > > > 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_ENDPOINT > > + help > > + Say Y here if you want to support the Sandbox PCIe controller in > > Use 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.fr...@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 ? > > > +{ > > + 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 ?). > > Regards, > Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot