[U-Boot] [PATCH v2 3/4] pci_ep: add pci_ep sandbox driver
Simon Glass
sjg at chromium.org
Fri Apr 26 01:11:40 UTC 2019
Hi Ramon,
On Thu, 25 Apr 2019 at 05:35, Ramon Fried <ramon.fried at gmail.com> wrote:
>
> 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 ?
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_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 ?
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
More information about the U-Boot
mailing list