Re: [Qemu-devel] [PATCH] hw/misc: Add code to emulate Xilinx Slave Serial port

[Alistair Francis]

On Thu, Dec 14, 2017 at 7:19 AM, Andrey Smirnov
<address@hidden> wrote:
> Add code to emulate Xilinx Slave Serial FPGA configuration port.
>
> Cc: "Edgar E. Iglesias" <address@hidden>
> Cc: Alistair Francis <address@hidden>
> Cc: address@hidden
> Cc: address@hidden
> Cc: address@hidden
> Signed-off-by: Andrey Smirnov <address@hidden>

Hey,

Thanks for the patch!

I have some comments inline, if anything is unclear just email me back
and I can provide more information or help.

> ---
>
> Integrating this into a build system via "obj-y" might not be the best
> way. Does this code need a dedicated CONFIG_ symbol?

You probably don't need a specific one, there are already some Xilinx
ones in there you can use.

Maybe CONFIG_XILINX or CONFIG_XILINX_AXI

>
> Thanks,
> Andrey Smirnov
>
>
>  hw/misc/Makefile.objs                 |   1 +
>  hw/misc/xilinx_slave_serial.c         | 105 
> ++++++++++++++++++++++++++++++++++
>  include/hw/misc/xilinx_slave_serial.h |  21 +++++++
>  3 files changed, 127 insertions(+)
>  create mode 100644 hw/misc/xilinx_slave_serial.c
>  create mode 100644 include/hw/misc/xilinx_slave_serial.h

You will need to connect this to a machine as well.

>
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index a68a201083..4599288e55 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -38,6 +38,7 @@ obj-$(CONFIG_IMX) += imx7_ccm.o
>  obj-$(CONFIG_IMX) += imx2_wdt.o
>  obj-$(CONFIG_IMX) += imx7_snvs.o
>  obj-$(CONFIG_IMX) += imx7_gpr.o
> +obj-y += xilinx_slave_serial.o
>  obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
>  obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
>  obj-$(CONFIG_MAINSTONE) += mst_fpga.o
> diff --git a/hw/misc/xilinx_slave_serial.c b/hw/misc/xilinx_slave_serial.c
> new file mode 100644
> index 0000000000..607674fb60
> --- /dev/null
> +++ b/hw/misc/xilinx_slave_serial.c
> @@ -0,0 +1,105 @@
> +/*
> + * Copyright (c) 2017, Impinj, Inc.
> + *
> + * Code to emulate programming "port" of Xilinx FPGA in Slave Serial
> + * configuration connected via SPI, for more deatils see (p. 27):
> + *
> + * See https://www.xilinx.com/support/documentation/user_guides/ug380.pdf

Ah, so this is for a Spartan-6 device. We don't have any QEMU support
for Spartan-6. What are you trying to use this for?

> + *
> + * Author: Andrey Smirnov <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/misc/xilinx_slave_serial.h"
> +#include "qemu/log.h"
> +
> +enum {
> +    XILINX_SLAVE_SERIAL_STATE_RESET,
> +    XILINX_SLAVE_SERIAL_STATE_RECONFIGURATION,
> +    XILINX_SLAVE_SERIAL_STATE_DONE,
> +};
> +
> +static void xilinx_slave_serial_update_outputs(XilinxSlaveSerialState 
> *xlnxss)

For function names try to use xlnx instead of xilinx, it just saves line length.

> +{
> +    qemu_set_irq(xlnxss->done,
> +                 xlnxss->state == XILINX_SLAVE_SERIAL_STATE_DONE);
> +}
> +
> +static void xilinx_slave_serial_reset(DeviceState *dev)
> +{
> +    XilinxSlaveSerialState *xlnxss = XILINX_SLAVE_SERIAL(dev);

This is generally just called 's'.

> +
> +    xlnxss->state = XILINX_SLAVE_SERIAL_STATE_RESET;
> +
> +    xilinx_slave_serial_update_outputs(xlnxss);
> +}
> +
> +static void xilinx_slave_serial_prog_b(void *opaque, int n, int level)
> +{
> +    XilinxSlaveSerialState *xlnxss = XILINX_SLAVE_SERIAL(opaque);
> +    assert(n == 0);
> +
> +    if (level) {
> +        xlnxss->state = XILINX_SLAVE_SERIAL_STATE_RECONFIGURATION;
> +    }
> +
> +    xilinx_slave_serial_update_outputs(xlnxss);
> +}
> +
> +static void xilinx_slave_serial_realize(SSISlave *ss, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(ss);
> +    XilinxSlaveSerialState *xlnxss = XILINX_SLAVE_SERIAL(ss);
> +
> +    qdev_init_gpio_in_named(dev,
> +                            xilinx_slave_serial_prog_b,
> +                            XILINX_SLAVE_SERIAL_GPIO_PROG_B,
> +                            1);
> +    qdev_init_gpio_out_named(dev, &xlnxss->done,
> +                             XILINX_SLAVE_SERIAL_GPIO_DONE, 1);
> +}
> +
> +static uint32_t xilinx_slave_serial_transfer(SSISlave *ss, uint32_t tx)
> +{
> +    XilinxSlaveSerialState *xlnxss = XILINX_SLAVE_SERIAL(ss);
> +
> +    if (xlnxss->state == XILINX_SLAVE_SERIAL_STATE_RECONFIGURATION) {
> +        xlnxss->state = XILINX_SLAVE_SERIAL_STATE_DONE;
> +    }
> +
> +    xilinx_slave_serial_update_outputs(xlnxss);
> +    return 0;
> +}
> +
> +static void xilinx_slave_serial_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass   *dc = DEVICE_CLASS(klass);
> +    SSISlaveClass *k  = SSI_SLAVE_CLASS(klass);
> +
> +    dc->reset      = xilinx_slave_serial_reset;
> +    dc->desc       = "Xilinx Slave Serial";
> +    k->realize     = xilinx_slave_serial_realize;
> +    k->transfer    = xilinx_slave_serial_transfer;
> +    /*
> +     * Slave Serial configuration is not technically SPI and there's
> +     * no CS signal
> +     */
> +    k->set_cs      = NULL;
> +    k->cs_polarity = SSI_CS_NONE;
> +}
> +
> +static const TypeInfo xilinx_slave_serial_info = {
> +    .name          = TYPE_XILINX_SLAVE_SERIAL,
> +    .parent        = TYPE_SSI_SLAVE,
> +    .instance_size = sizeof(XilinxSlaveSerialState),
> +    .class_init    = xilinx_slave_serial_class_init,
> +};
> +
> +static void xilinx_slave_serial_register_type(void)
> +{
> +    type_register_static(&xilinx_slave_serial_info);
> +}
> +type_init(xilinx_slave_serial_register_type)
> diff --git a/include/hw/misc/xilinx_slave_serial.h 
> b/include/hw/misc/xilinx_slave_serial.h
> new file mode 100644
> index 0000000000..f7b2e22be3
> --- /dev/null
> +++ b/include/hw/misc/xilinx_slave_serial.h
> @@ -0,0 +1,21 @@
> +#ifndef XILINX_SLAVE_SERIAL_H
> +#define XILINX_SLAVE_SERIAL_H
> +
> +#include "hw/ssi/ssi.h"
> +
> +typedef struct XilinxSlaveSerialState {
> +    /*< private >*/
> +    SSISlave parent_obj;
> +
> +    qemu_irq done;
> +    int state;
> +} XilinxSlaveSerialState;
> +
> +#define TYPE_XILINX_SLAVE_SERIAL "xilinx:slave-serial"

Full stop instead of colon.

Overall the model looks fine, I'm just not sure what you are using it
for as it isn't connected to anything.

Thanks,
Alistair

> +#define XILINX_SLAVE_SERIAL(obj) \
> +    OBJECT_CHECK(XilinxSlaveSerialState, (obj), TYPE_XILINX_SLAVE_SERIAL)
> +
> +#define XILINX_SLAVE_SERIAL_GPIO_DONE   "xilinx:slave-serial:done"
> +#define XILINX_SLAVE_SERIAL_GPIO_PROG_B "xilinx:slave-serial:prog-b"
> +
> +#endif /* XILINX_SLAVE_SERIAL_H */
> --
> 2.14.3
>
>