[U-Boot] [PATCH v2 3/7] axi: Add AXI sandbox driver and simple emulator
Mario Six
mario.six at gdsys.cc
Mon Jun 25 09:51:05 UTC 2018
Hi Simon,
On Fri, May 25, 2018 at 4:41 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Mario,
>
> On 23 May 2018 at 06:10, Mario Six <mario.six at gdsys.cc> wrote:
>> Add test infrastructure and tests for the AXI uclass.
>>
>> Signed-off-by: Mario Six <mario.six at gdsys.cc>
>>
>> ---
>>
>> v1 -> v2:
>> * Spelled out abbreviations in Kconfig help
>> * Expanded emulation documentation
>> * Renamed storage pointer to storep
>> * Expanded AXI emulator usage documentation
>> * Switched AXI emulator to aligned access
>>
>> ---
>> drivers/axi/Kconfig | 7 +++
>> drivers/axi/Makefile | 3 ++
>> drivers/axi/axi-emul-uclass.c | 68 ++++++++++++++++++++++++++
>> drivers/axi/axi_sandbox.c | 77 +++++++++++++++++++++++++++++
>> drivers/axi/sandbox_store.c | 110 ++++++++++++++++++++++++++++++++++++++++++
>> include/axi.h | 84 ++++++++++++++++++++++++++++++++
>> include/dm/uclass-id.h | 1 +
>> 7 files changed, 350 insertions(+)
>> create mode 100644 drivers/axi/axi-emul-uclass.c
>> create mode 100644 drivers/axi/axi_sandbox.c
>> create mode 100644 drivers/axi/sandbox_store.c
>>
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> Some nits below.
>
>> diff --git a/drivers/axi/axi_sandbox.c b/drivers/axi/axi_sandbox.c
>> new file mode 100644
>> index 00000000000..7a485b114bf
>> --- /dev/null
>> +++ b/drivers/axi/axi_sandbox.c
>> @@ -0,0 +1,77 @@
>> +/*
>> + * (C) Copyright 2018
>> + * Mario Six, Guntermann & Drunck GmbH, mario.six at gdsys.cc
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <axi.h>
>> +#include <dm.h>
>> +
>> +/*
>> + * This driver implements a AXI bus for the sandbox architecture for testing
>> + * purposes.
>> + *
>> + * The bus forwards every access to it to a special AXI emulation device (which
>> + * it gets via the axi_emul_get_ops function) that implements a simple
>> + * read/write storage.
>> + *
>> + * The emulator device must still be contained in the device tree in the usual
>> + * way, since configuration data for the storage is read from the DT.
>> + */
>> +
>> +int axi_sandbox_read(struct udevice *bus, ulong address, void *data,
>> + enum axi_size_t size)
>
> Can this and the write() function below be static?
>
Yes, sure, will be fixed in v3.
>> +{
>> + struct axi_emul_ops *ops;
>> + struct udevice *emul;
>> + int ret;
>> +
>> + /* Get emulator device */
>> + ret = axi_sandbox_get_emul(bus, address, size, &emul);
>> + if (ret)
>> + return ret == -ENODEV ? 0 : ret;
>> + /* Forward all reads to the AXI emulator */
>> + ops = axi_emul_get_ops(emul);
>> + if (!ops || !ops->read)
>> + return -ENOSYS;
>> +
>> + return ops->read(emul, address, data, size);
>> +}
>> +
>> +int axi_sandbox_write(struct udevice *bus, ulong address, void *data,
>> + enum axi_size_t size)
>> +{
>> + struct axi_emul_ops *ops;
>> + struct udevice *emul;
>> + int ret;
>> +
>> + /* Get emulator device */
>> + ret = axi_sandbox_get_emul(bus, address, size, &emul);
>> + if (ret)
>> + return ret == -ENODEV ? 0 : ret;
>> + /* Forward all writes to the AXI emulator */
>> + ops = axi_emul_get_ops(emul);
>> + if (!ops || !ops->write)
>> + return -ENOSYS;
>> +
>> + return ops->write(emul, address, data, size);
>> +}
>> +
>> +static const struct udevice_id axi_sandbox_ids[] = {
>> + { .compatible = "sandbox,axi" },
>> + { /* sentinel */ }
>> +};
>> +
>> +static const struct axi_ops axi_sandbox_ops = {
>> + .read = axi_sandbox_read,
>> + .write = axi_sandbox_write,
>> +};
>> +
>> +U_BOOT_DRIVER(axi_sandbox_bus) = {
>> + .name = "axi_sandbox_bus",
>> + .id = UCLASS_AXI,
>> + .of_match = axi_sandbox_ids,
>> + .ops = &axi_sandbox_ops,
>> +};
>> diff --git a/drivers/axi/sandbox_store.c b/drivers/axi/sandbox_store.c
>> new file mode 100644
>> index 00000000000..61c7aea22c8
>> --- /dev/null
>> +++ b/drivers/axi/sandbox_store.c
>> @@ -0,0 +1,110 @@
>> +/*
>> + * (C) Copyright 2018
>> + * Mario Six, Guntermann & Drunck GmbH, mario.six at gdsys.cc
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <axi.h>
>> +#include <dm.h>
>> +
>> +struct sandbox_store_priv {
>> + u8 *store;
>> +};
>> +
>> +void copy_axi_data(void *src, void *dst, enum axi_size_t size)
>> +{
>> + u8 *dst8 = dst;
>> + u16 *dst16 = dst;
>> + u32 *dst32 = dst;
>
> Can you put these declarations in the case statements instead? I'm not
> sure what coverity with think of this aliasing.
>
Yeah, coverity probably won't like that. I'll put the casts in the case
statements for v3.
>> +
>> + switch (size) {
>> + case AXI_SIZE_8:
>> + *dst8 = *((u8 *)src);
>> + break;
>> + case AXI_SIZE_16:
>> + *dst16 = be16_to_cpu(*((u16 *)src));
>> + break;
>> + case AXI_SIZE_32:
>> + *dst32 = be32_to_cpu(*((u32 *)src));
>> + break;
>> + }
>> +}
>> +
>> +int sandbox_store_read(struct udevice *dev, ulong address, void *data,
>> + enum axi_size_t size)
>> +{
>> + struct sandbox_store_priv *priv = dev_get_priv(dev);
>> +
>> + copy_axi_data(priv->store + address, data, size);
>> +
>> + return 0;
>> +}
>> +
>> +int sandbox_store_write(struct udevice *dev, ulong address, void *data,
>> + enum axi_size_t size)
>> +{
>> + struct sandbox_store_priv *priv = dev_get_priv(dev);
>> +
>> + copy_axi_data(data, priv->store + address, size);
>> +
>> + return 0;
>> +}
>> +
>> +int sandbox_store_get_store(struct udevice *dev, u8 **store)
>> +{
>> + struct sandbox_store_priv *priv = dev_get_priv(dev);
>> +
>> + *store = priv->store;
>> +
>> + return 0;
>> +}
>> +
>> +static const struct udevice_id sandbox_store_ids[] = {
>> + { .compatible = "sandbox,sandbox_store" },
>> + { /* sentinel */ }
>> +};
>> +
>> +static const struct axi_emul_ops sandbox_store_ops = {
>> + .read = sandbox_store_read,
>> + .write = sandbox_store_write,
>> + .get_store = sandbox_store_get_store,
>> +};
>> +
>> +int sandbox_store_probe(struct udevice *dev)
>> +{
>> + struct sandbox_store_priv *priv = dev_get_priv(dev);
>> + u32 reg[2];
>> + int ret;
>> +
>> + ret = dev_read_u32_array(dev, "reg", reg, ARRAY_SIZE(reg));
>> + if (ret)
>> + return -EINVAL;
>> +
>> + priv->store = calloc(reg[1], 1);
>> +
>
> drop blank line
>
Will be done for v3.
>> + if (!priv->store)
>> + return -ENOMEM;
>> +
>> + return 0;
>> +}
>> +
>> +int sandbox_store_remove(struct udevice *dev)
>> +{
>> + struct sandbox_store_priv *priv = dev_get_priv(dev);
>> +
>> + free(priv->store);
>> +
>> + return 0;
>> +}
>> +
>> +U_BOOT_DRIVER(sandbox_axi_store) = {
>> + .name = "sandbox_axi_store",
>> + .id = UCLASS_AXI_EMUL,
>> + .of_match = sandbox_store_ids,
>> + .ops = &sandbox_store_ops,
>> + .priv_auto_alloc_size = sizeof(struct sandbox_store_priv),
>> + .probe = sandbox_store_probe,
>> + .remove = sandbox_store_remove,
>> +};
>> diff --git a/include/axi.h b/include/axi.h
>> index 317e931a6cf..5b428127f8c 100644
>> --- a/include/axi.h
>> +++ b/include/axi.h
>> @@ -72,4 +72,88 @@ int axi_read(struct udevice *dev, ulong address, void *data, enum axi_size_t siz
>> * @return 0 if OK, -ve on error.
>> */
>> int axi_write(struct udevice *dev, ulong address, void *data, enum axi_size_t size);
>> +
>> +struct axi_emul_ops {
>> + /**
>> + * read() - Read a single value from a specified address on a AXI bus
>> + *
>> + * @dev: AXI bus to read from.
>> + * @address: The address to read from.
>> + * @data: Pointer to a variable that takes the data value read
>> + * from the address on the AXI bus.
>> + * @size: The size of the data to be read.
>> + * @return 0 if OK, -ve on error.
>> + */
>> + int (*read)(struct udevice *dev, ulong address, void *data, enum axi_size_t size);
>> +
>> + /**
>> + * write() - Write a single value to a specified address on a AXI bus
>> + *
>> + * @dev: AXI bus to write to.
>> + * @address: The address to write to.
>> + * @data: Pointer to the data value to be written to the address
>> + * on the AXI bus.
>> + * @size: The size of the data to write.
>> + * @return 0 if OK, -ve on error.
>> + */
>> + int (*write)(struct udevice *dev, ulong address, void *data, enum axi_size_t size);
>> +
>> + /**
>> + * get_store() - Get address of internal storage of a emulated AXI
>> + * device
>> + *
>> + * @dev: Emulated AXI device to get the pointer of the internal
>> + * storage for.
>> + * @storep: Pointer to the internal storage of the emulated AXI
>> + * device.
>> + * @return 0 if OK, -ve on error.
>> + */
>> + int (*get_store)(struct udevice *dev, u8 **storep);
>> +};
>> +
>> +#define axi_emul_get_ops(dev) ((struct axi_emul_ops *)(dev)->driver->ops)
>> +
>> +/**
>> + * axi_sandbox_get_emul() - Retrieve a pointer to a AXI emulation device
>> + *
>> + * To test the AXI uclass, we implement a simple AXI emulation device, which is
>> + * a virtual device on a AXI bus that exposes a simple storage interface: When
>> + * reading and writing from the device, the addresses are translated to offsets
>> + * within the device's storage. For write accesses the data is written to the
>> + * specified storage offset, and for read accesses the data is read from the
>> + * specified storage offset.
>> + *
>> + * A DTS entry might look like this:
>> + *
>> + * axi: axi at 0 {
>> + * compatible = "sandbox,axi";
>> + * #address-cells = <0x1>;
>> + * #size-cells = <0x1>;
>> + * store at 0 {
>> + * compatible = "sandbox,sandbox_store";
>> + * reg = <0x0 0x400>;
>> + * };
>> + * };
>> + *
>> + * This function may then be used to retrieve the pointer to the sandbox_store
>> + * emulation device given the AXI bus device.
>> + */
>> +int axi_sandbox_get_emul(struct udevice *bus, ulong address, uint length,
>> + struct udevice **emulp);
>
> Can these two function declarations go in arch/sandbox/include/asm
> perhaps? I don't think they are needed except for sandbox?
>
Yes, no problem. Will be fixed in v3.
>> +/**
>> + * axi_get_store() - Get address of internal storage of a emulated AXI device
>> + *
>> + * To preset or read back the contents internal storage of the emulated AXI
>> + * device, this function returns the pointer to the storage. Changes to the
>> + * contents of the storage are reflected when using the AXI read/write API
>> + * methods, and vice versa, so by using this method expected read data can be
>> + * set up in advance, and written data can be checked in unit tests.
>> + *
>> + * @dev: Emulated AXI device to get the pointer of the internal storage
>> + * for.
>> + * @storep: Pointer to the internal storage of the emulated AXI device.
>> + * @return 0 if OK, -ve on error.
>> + */
>> +int axi_get_store(struct udevice *dev, u8 **storep);
>> +
>> #endif
>
> Regards,
> Simon
>
Best regards,
Mario
More information about the U-Boot
mailing list