Re: [PATCH v1 1/5] hw/ppc: SPI responder model

[Stefan Berger]

On 2/7/24 11:08, Chalapathi V wrote:
> Serial pheripheral interface provides full-duplex synchronous serial
> communication between single controller and multiple responder devices.
> One SPI Controller is implemented and supported on a SPI Bus, there is
> no support for multiple controllers on the SPI bus.
>
> The current implemetation assumes that single responder is connected to
> bus, hence chip_select is not modelled.
>
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
>   include/hw/ppc/pnv_spi_responder.h | 109 +++++++++++++++++++
>   hw/ppc/pnv_spi_responder.c         | 166 +++++++++++++++++++++++++++++
>   hw/ppc/meson.build                 |   1 +
>   3 files changed, 276 insertions(+)
>   create mode 100644 include/hw/ppc/pnv_spi_responder.h
>   create mode 100644 hw/ppc/pnv_spi_responder.c
>
> diff --git a/include/hw/ppc/pnv_spi_responder.h 
> b/include/hw/ppc/pnv_spi_responder.h
> new file mode 100644
> index 0000000000..1cf7279aad
> --- /dev/null
> +++ b/include/hw/ppc/pnv_spi_responder.h
> @@ -0,0 +1,109 @@
> +/*
> + * QEMU SPI Responder.
> + *
> + * Copyright (c) 2024, IBM Corporation.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * SPI provides full-duplex synchronous serial communication between single
> + * controller and multiple responder devices. One SPI Controller is
> + * implemented and supported on a SPI Bus, there is no support for multiple

an SPI Bus

> + * controllers on the SPI bus.
> + *
> + * The current implementation assumes that single responder is connected to > 
> + * bus, hence chip_select is not modelled.

to the bus, hence chip_select is not modeled.

> + */
> +
> +#ifndef PPC_PNV_SPI_RESPONDER_H
> +#define PPC_PNV_SPI_RESPONDER_H
> +
> +#include "hw/qdev-core.h"
> +#include "qom/object.h"
> +#include "qemu/log.h"
> +
> +#define TYPE_PNV_SPI_RESPONDER "spi-responder"
> +OBJECT_DECLARE_TYPE(PnvSpiResponder, PnvSpiResponderClass,
> +                    PNV_SPI_RESPONDER)
> +
> +typedef struct xfer_buffer xfer_buffer;
> +
> +struct PnvSpiResponderClass {
> +    DeviceClass parent_class;
> +
> +    /*
> +     * These methods are from controller to responder and implemented
> +     * by all responders.
> +     * Connect_controller/disconnect_controller methods are called by
> +     * controller to initiate/stop the SPI transfer.
> +     */
> +    void (*connect_controller)(PnvSpiResponder *responder, const char *port);
> +    void (*disconnect_controller)(PnvSpiResponder *responder);
> +    /*
> +     * SPI transfer consists of a number of consecutive calls to the request
> +     * method.
> +     * The parameter first is true on first call of the transfer and last is on
> +     * the final call of the transfer. Parameter bits and payload defines the
> +     * number of bits and data payload sent by controller.
> +     * Responder returns the response payload.
> +     */
> +    xfer_buffer *(*request)(PnvSpiResponder *responder, int first, int last,
> +                    int bits, xfer_buffer *payload);
> +};
> +
> +struct PnvSpiResponder {
> +    DeviceState parent_obj;
> +};
> +
> +#define TYPE_SPI_BUS "spi-bus"
> +OBJECT_DECLARE_SIMPLE_TYPE(SpiBus, SPI_BUS)
> +
> +struct SpiBus {
> +    BusState parent_obj;
> +};
> +
> +/*
> + * spi_realize_and_unref: realize and unref an SPI responder
> + * @dev: SPI responder to realize
> + * @bus: SPI bus to put it on
> + * @errp: error pointer
> + */
> +bool spi_realize_and_unref(DeviceState *dev, SpiBus *bus, Error **errp);
> +
> +/*
> + * spi_create_responder: create a SPI responder.

a -> an

> + * @bus: SPI bus to put it on
> + * @name: name of the responder object.
> + * call spi_realize_and_unref() after creating the responder.
> + */
> +
> +PnvSpiResponder *spi_create_responder(SpiBus *bus, const char *name);
> +
> +/* xfer_buffer */
> +typedef struct xfer_buffer {
> +
> +    uint32_t    len;
> +    uint8_t    *data;
> +
> +} xfer_buffer;
> +
> +/*
> + * xfer_buffer_read_ptr: Increment the payload length and return the pointer
> + * to the data at offset
> + */
> +uint8_t *xfer_buffer_write_ptr(xfer_buffer *payload, uint32_t offset,
> +                uint32_t length);
> +/* xfer_buffer_read_ptr: Return the pointer to the data at offset */
> +void xfer_buffer_read_ptr(xfer_buffer *payload, uint8_t **read_buf,
> +                uint32_t offset, uint32_t length);
> +/* xfer_buffer_new: Allocate memory and return the pointer */
> +xfer_buffer *xfer_buffer_new(void);
> +/* xfer_buffer_free: free the payload */
> +void xfer_buffer_free(xfer_buffer *payload);
> +
> +/* Controller interface */
> +SpiBus *spi_create_bus(DeviceState *parent, const char *name);
> +xfer_buffer *spi_request(SpiBus *bus, int first, int last, int bits,
> +                xfer_buffer *payload);
> +bool spi_connect_controller(SpiBus *bus, const char *port);
> +bool spi_disconnect_controller(SpiBus *bus);
> +#endif /* PPC_PNV_SPI_SEEPROM_H */
> diff --git a/hw/ppc/pnv_spi_responder.c b/hw/ppc/pnv_spi_responder.c
> new file mode 100644
> index 0000000000..c3bc659b1b
> --- /dev/null
> +++ b/hw/ppc/pnv_spi_responder.c
> @@ -0,0 +1,166 @@
> +/*
> + * QEMU PowerPC SPI Responder
> + *
> + * Copyright (c) 2024, IBM Corporation.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/ppc/pnv_spi_responder.h"
> +#include "qapi/error.h"
> +
> +static const TypeInfo spi_bus_info = {
> +    .name = TYPE_SPI_BUS,
> +    .parent = TYPE_BUS,
> +    .instance_size = sizeof(SpiBus),
> +};
> +
> +SpiBus *spi_create_bus(DeviceState *parent, const char *name)
> +{
> +    BusState *bus;

empty line after var decl

> +    bus = qbus_new(TYPE_SPI_BUS, parent, name);
> +    return SPI_BUS(bus);
> +}
> +
> +/* xfer_buffer_methods */
> +xfer_buffer *xfer_buffer_new(void)
> +{
> +    xfer_buffer *payload = g_malloc0(sizeof(*payload));

empty line after var decl

> +    payload->data = g_malloc0(payload->len * sizeof(uint8_t));

sizeof(uint8_t) doesn't seem necessary


> +    return payload;
> +}
> +
> +void xfer_buffer_free(xfer_buffer *payload)
> +{
> +    free(payload->data);
> +    payload->data = NULL;

not necessary to do this

> +    free(payload);
> +}
> +
> +uint8_t *xfer_buffer_write_ptr(xfer_buffer *payload, uint32_t offset,
> +                            uint32_t length)
> +{
> +    if (payload->len < (offset + length)) {
> +        payload->len = offset + length;
> +        payload->data = g_realloc(payload->data,
> +                        payload->len * sizeof(uint8_t));

sizeof(uint8_t)  does not seem necessary

> +    }
> +    return &payload->data[offset];
> +}
> +
> +void xfer_buffer_read_ptr(xfer_buffer *payload, uint8_t **read_buf,
> +                uint32_t offset, uint32_t length)
> +{
> +    static uint32_t prev_len;

Why do you keep prev_len around?

> +    uint32_t offset_org = offset;

empty line after var decl

> +    if (offset > payload->len) {
if (offset + length > payload->len) ?

> +        if (length < payload->len) {
> +            offset = payload->len - length;
> +        } else {
> +            offset = 0;
> +            length = payload->len;
> +        }

If the user passes in length (1000) and now you artificially lower it 
(10) he may assume he got 1000 bytes to read and end up reading over the 
end of the buffer in the end becasue there are only 10 bytes. I don't 
think you should neither adjust offset nor length but realloc (if at all 
necessary to do this here) to accomodate these values.

> +        qemu_log_mask(LOG_GUEST_ERROR, "Read offset(%d) exceeds buffer "
> +                        "length(%d), altered offset to %d and length to %d to "
> +                        "read within buffer\n", offset_org, payload->len,
> +                        offset, length);
> +    } else if ((offset + length) > payload->len) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "Read length(%d) bytes from offset (%d)"
> +                        ", exceeds buffer length(%d)\n", length, offset,
> +                        payload->len);
> +        length = payload->len - offset;
> +    }
> +
> +    if ((prev_len != length) || (*read_buf == NULL)) {
> +        *read_buf = g_realloc(*read_buf, length * sizeof(uint8_t));
> +        prev_len = length;
> +    }
> +    *read_buf = &payload->data[offset];
> +}
> +
> +/* Controller interface methods */
> +bool spi_connect_controller(SpiBus *bus, const char *port)
> +{
> +    BusState *b = BUS(bus);
> +    BusChild *kid;

empty line

> +    QTAILQ_FOREACH(kid, &b->children, sibling) {
> +        PnvSpiResponder *r = PNV_SPI_RESPONDER(kid->child);
> +        PnvSpiResponderClass *rc = PNV_SPI_RESPONDER_GET_CLASS(r);

empty line

> +        rc->connect_controller(r, port);
> +        return true;

you didn't process the whole list but returned on the first element?

> +    }
> +    return false;
> +}
> +bool spi_disconnect_controller(SpiBus *bus)
> +{
> +    BusState *b = BUS(bus);
> +    BusChild *kid;

empty line

> +    QTAILQ_FOREACH(kid, &b->children, sibling) {
> +        PnvSpiResponder *r = PNV_SPI_RESPONDER(kid->child);
> +        PnvSpiResponderClass *rc = PNV_SPI_RESPONDER_GET_CLASS(r);
> +        rc->disconnect_controller(r);
> +        return true;

same comments here

> +    }
> +    return false;
> +}
> +
> +xfer_buffer *spi_request(SpiBus *bus,
> +                int first, int last, int bits, xfer_buffer *payload)
> +{
> +    BusState *b = BUS(bus);
> +    BusChild *kid;
> +    xfer_buffer *rsp_payload = NULL;
> +    uint8_t *buf = NULL;
> +
> +    QTAILQ_FOREACH(kid, &b->children, sibling) {
> +        PnvSpiResponder *r = PNV_SPI_RESPONDER(kid->child);
> +        PnvSpiResponderClass *rc = PNV_SPI_RESPONDER_GET_CLASS(r);
> +        rsp_payload = rc->request(r, first, last, bits, payload);
> +        return rsp_payload;

Also here you seem to stop processing in the first element.

> +    }
> +    if (rsp_payload == NULL) {
> +        rsp_payload = xfer_buffer_new();
> +    }
> +    buf = xfer_buffer_write_ptr(rsp_payload, 0, payload->len);
> +    memset(buf, 0, payload->len);
> +    return rsp_payload;
> +}
> +
> +/* create and realise spi responder device */
> +bool spi_realize_and_unref(DeviceState *dev, SpiBus *bus, Error **errp)
> +{
> +    return qdev_realize_and_unref(dev, &bus->parent_obj, errp);
> +}
> +
> +PnvSpiResponder *spi_create_responder(SpiBus *bus, const char *name)
> +{
> +    DeviceState *dev = qdev_new(name);
> +
> +    spi_realize_and_unref(dev, bus, &error_fatal);
> +    return PNV_SPI_RESPONDER(dev);
> +}
> +
> +static void pnv_spi_responder_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->desc = "PowerNV SPI RESPONDER";
> +}
> +
> +static const TypeInfo pnv_spi_responder_info = {
> +    .name          = TYPE_PNV_SPI_RESPONDER,
> +    .parent        = TYPE_DEVICE,
> +    .instance_size = sizeof(PnvSpiResponder),
> +    .class_init    = pnv_spi_responder_class_init,
> +    .abstract      = true,
> +    .class_size    = sizeof(PnvSpiResponderClass),
> +};
> +
> +static void pnv_spi_responder_register_types(void)
> +{
> +    type_register_static(&pnv_spi_responder_info);
> +    type_register_static(&spi_bus_info);
> +}
> +
> +type_init(pnv_spi_responder_register_types);
> diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
> index eba3406e7f..9bfd5a5613 100644
> --- a/hw/ppc/meson.build
> +++ b/hw/ppc/meson.build
> @@ -53,6 +53,7 @@ ppc_ss.add(when: 'CONFIG_POWERNV', if_true: files(
>     'pnv_bmc.c',
>     'pnv_homer.c',
>     'pnv_pnor.c',
> +  'pnv_spi_responder.c',
>   ))
>   # PowerPC 4xx boards
>   ppc_ss.add(when: 'CONFIG_PPC405', if_true: files(