[U-Boot] [PATCH v2 3/4] pci_ep: add pci_ep sandbox driver
Ramon Fried
ramon.fried at gmail.com
Thu Apr 25 11:35:31 UTC 2019
On Thu, Apr 25, 2019 at 2:29 AM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Ramon,
>
> On Wed, 24 Apr 2019 at 12:54, Ramon Fried <ramon.fried at gmail.com> wrote:
>
> Please add a commit message.
>
> >
> > Signed-off-by: Ramon Fried <ramon.fried at 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.fried at 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
More information about the U-Boot
mailing list