Re: [Qemu-devel] [PATCH pic32 v3 13/16] pic32: add file pic32_spi.c

[Peter Crosthwaite]

On Sun, Jul 5, 2015 at 11:15 PM, Serge Vakulenko
<address@hidden> wrote:
> Implement pic32 SPI peripheral interface.
>
> Signed-off-by: Serge Vakulenko <address@hidden>
> ---
>  hw/mips/pic32_spi.c | 121 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++

SPI controllers go in hw/ssi.

>  1 file changed, 121 insertions(+)
>  create mode 100644 hw/mips/pic32_spi.c
>
> diff --git a/hw/mips/pic32_spi.c b/hw/mips/pic32_spi.c
> new file mode 100644
> index 0000000..52b1579
> --- /dev/null
> +++ b/hw/mips/pic32_spi.c
> @@ -0,0 +1,121 @@
> +/*
> + * SPI ports.
> + *
> + * Copyright (C) 2014-2015 Serge Vakulenko
> + *
> + * Permission to use, copy, modify, and distribute this software
> + * and its documentation for any purpose and without fee is hereby
> + * granted, provided that the above copyright notice appear in all
> + * copies and that both that the copyright notice and this
> + * permission notice and warranty disclaimer appear in supporting
> + * documentation, and that the name of the author not be used in
> + * advertising or publicity pertaining to distribution of the
> + * software without specific, written prior permission.
> + *
> + * The author disclaim all warranties with regard to this
> + * software, including all implied warranties of merchantability
> + * and fitness.  In no event shall the author be liable for any
> + * special, indirect or consequential damages or any damages
> + * whatsoever resulting from loss of use, data or profits, whether
> + * in an action of contract, negligence or other tortious action,
> + * arising out of or in connection with the use or performance of
> + * this software.
> + */
> +#include "hw/hw.h"
> +#include "pic32_peripherals.h"
> +

Devices should have their own header rather than share with other devs
of the same family.

> +#include "pic32mz.h"
> +
> +#define SPI_IRQ_FAULT   0               /* error irq offset */
> +#define SPI_IRQ_TX      1               /* transmitter irq offset */
> +#define SPI_IRQ_RX      2               /* receiver irq offset */
> +
> +unsigned pic32_spi_readbuf(pic32_t *s, int unit)
> +{
> +    spi_t *p = &s->spi[unit];

This looks like it is retrieving its own state from the chip
container. Rather the device should be self container and the higher
layers (chip level?) should pass is the single SPI controller to these
APIs.

> +    unsigned result = p->buf[p->rfifo];
> +
> +    if (VALUE(p->con) & PIC32_SPICON_ENHBUF) {
> +        p->rfifo++;
> +        p->rfifo &= 3;
> +    }
> +    if (VALUE(p->stat) & PIC32_SPISTAT_SPIRBF) {
> +        VALUE(p->stat) &= ~PIC32_SPISTAT_SPIRBF;
> +        /*s->irq_clear(s, p->irq + SPI_IRQ_RX);*/
> +    }
> +    return result;
> +}
> +
> +void pic32_spi_writebuf(pic32_t *s, int unit, unsigned val)
> +{
> +    spi_t *p = &s->spi[unit];
> +
> +    /* Perform SD card i/o on configured SPI port. */
> +    if (unit == s->sdcard_spi_port) {

The SPI controller should be self-contained, and not have SD
awareness. Connection of SPI to SD needs to happen on chip, board or
machine level.

I think MIPS may be a little out of date with device modelling style.
I would look further afield for template code. For a simple SPI
controller that is (more) up to date style wise I suggest
xilinx_spi.c.

Regards,
Peter

> +        unsigned result = 0;
> +
> +        if (VALUE(p->con) & PIC32_SPICON_MODE32) {
> +            /* 32-bit data width */
> +            result  = (unsigned char) pic32_sdcard_io(s, val >> 24) << 24;
> +            result |= (unsigned char) pic32_sdcard_io(s, val >> 16) << 16;
> +            result |= (unsigned char) pic32_sdcard_io(s, val >> 8) << 8;
> +            result |= (unsigned char) pic32_sdcard_io(s, val);
> +
> +        } else if (VALUE(p->con) & PIC32_SPICON_MODE16) {
> +            /* 16-bit data width */
> +            result = (unsigned char) pic32_sdcard_io(s, val >> 8) << 8;
> +            result |= (unsigned char) pic32_sdcard_io(s, val);
> +
> +        } else {
> +            /* 8-bit data width */
> +            result = (unsigned char) pic32_sdcard_io(s, val);
> +        }
> +        p->buf[p->wfifo] = result;
> +    } else {
> +        /* No device */
> +        p->buf[p->wfifo] = ~0;
> +    }
> +    if (VALUE(p->stat) & PIC32_SPISTAT_SPIRBF) {
> +        VALUE(p->stat) |= PIC32_SPISTAT_SPIROV;
> +        /*s->irq_raise(s, p->irq + SPI_IRQ_FAULT);*/
> +    } else if (VALUE(p->con) & PIC32_SPICON_ENHBUF) {
> +        p->wfifo++;
> +        p->wfifo &= 3;
> +        if (p->wfifo == p->rfifo) {
> +            VALUE(p->stat) |= PIC32_SPISTAT_SPIRBF;
> +            /*s->irq_raise(s, p->irq + SPI_IRQ_RX);*/
> +        }
> +    } else {
> +        VALUE(p->stat) |= PIC32_SPISTAT_SPIRBF;
> +        /*s->irq_raise(s, p->irq + SPI_IRQ_RX);*/
> +    }
> +}
> +
> +void pic32_spi_control(pic32_t *s, int unit)
> +{
> +    spi_t *p = &s->spi[unit];
> +
> +    if (!(VALUE(p->con) & PIC32_SPICON_ON)) {
> +        s->irq_clear(s, p->irq + SPI_IRQ_FAULT);
> +        s->irq_clear(s, p->irq + SPI_IRQ_RX);
> +        s->irq_clear(s, p->irq + SPI_IRQ_TX);
> +        VALUE(p->stat) = PIC32_SPISTAT_SPITBE;
> +    } else if (!(VALUE(p->con) & PIC32_SPICON_ENHBUF)) {
> +        p->rfifo = 0;
> +        p->wfifo = 0;
> +    }
> +}
> +
> +/*
> + * Initialize the SPI data structure.
> + */
> +void pic32_spi_init(pic32_t *s, int unit, int irq, int con, int stat)
> +{
> +    spi_t *p = &s->spi[unit];
> +
> +    p->irq = irq;
> +    p->con = con;
> +    p->stat = stat;
> +    p->rfifo = 0;
> +    p->wfifo = 0;
> +}
> --
> 2.2.2
>
>