[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/4] i.MX: Add the Freescale SPI Controller
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/4] i.MX: Add the Freescale SPI Controller |
Date: |
Thu, 25 Feb 2016 14:52:43 +0000 |
On 15 February 2016 at 11:17, Jean-Christophe Dubois
<address@hidden> wrote:
> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> ---
>
> Changes since v1:
> * Access SPI slave only at a byte level.
> * rework the CS activation to avoid to reset access to SPI slaves.
>
> hw/ssi/Makefile.objs | 1 +
> hw/ssi/imx_spi.c | 459
> +++++++++++++++++++++++++++++++++++++++++++++++
> include/hw/ssi/imx_spi.h | 104 +++++++++++
> 3 files changed, 564 insertions(+)
> create mode 100644 hw/ssi/imx_spi.c
> create mode 100644 include/hw/ssi/imx_spi.h
>
> diff --git a/hw/ssi/Makefile.objs b/hw/ssi/Makefile.objs
> index 9555825..fcbb79e 100644
> --- a/hw/ssi/Makefile.objs
> +++ b/hw/ssi/Makefile.objs
> @@ -4,3 +4,4 @@ common-obj-$(CONFIG_XILINX_SPI) += xilinx_spi.o
> common-obj-$(CONFIG_XILINX_SPIPS) += xilinx_spips.o
>
> obj-$(CONFIG_OMAP) += omap_spi.o
> +obj-$(CONFIG_IMX) += imx_spi.o
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> new file mode 100644
> index 0000000..cab102c
> --- /dev/null
> +++ b/hw/ssi/imx_spi.c
> @@ -0,0 +1,459 @@
> +/*
> + * IMX SPI Controller
> + *
> + * Copyright (c) 2016 Jean-Christophe Dubois <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 "hw/ssi/imx_spi.h"
> +#include "sysemu/sysemu.h"
Source files need to #include "qemu/osdep.h" as the first #include.
(You will find this doesn't compile otherwise if you rebase on master.)
> +
> +#ifndef DEBUG_IMX_SPI
> +#define DEBUG_IMX_SPI 0
> +#endif
> +
> +#define DPRINTF(fmt, args...) \
> + do { \
> + if (DEBUG_IMX_SPI) { \
> + fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX_SPI, \
> + __func__, ##args); \
> + } \
> + } while (0)
> +
> +static char const *imx_spi_reg_name(uint32_t reg)
> +{
> + static char unknown[20];
> +
> + switch (reg) {
> + case ECSPI_RXDATA:
> + return "ECSPI_RXDATA";
> + case ECSPI_TXDATA:
> + return "ECSPI_TXDATA";
> + case ECSPI_CONREG:
> + return "ECSPI_CONREG";
> + case ECSPI_CONFIGREG:
> + return "ECSPI_CONFIGREG";
> + case ECSPI_INTREG:
> + return "ECSPI_INTREG";
> + case ECSPI_DMAREG:
> + return "ECSPI_DMAREG";
> + case ECSPI_STATREG:
> + return "ECSPI_STATREG";
> + case ECSPI_PERIODREG:
> + return "ECSPI_PERIODREG";
> + case ECSPI_TESTREG:
> + return "ECSPI_TESTREG";
> + case ECSPI_MSGDATA:
> + return "ECSPI_MSGDATA";
> + default:
> + sprintf(unknown, "%d ?", reg);
> + return unknown;
> + }
> +}
> +
> +static const VMStateDescription vmstate_imx_spi = {
> + .name = TYPE_IMX_SPI,
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_FIFO32(tx_fifo, IMXSPIState),
> + VMSTATE_FIFO32(rx_fifo, IMXSPIState),
> + VMSTATE_UINT32_ARRAY(regs, IMXSPIState, ECSPI_MAX),
> + VMSTATE_END_OF_LIST()
> + },
> +};
> +
> +static void imx_spi_txfifo_reset(IMXSPIState *s)
> +{
> + fifo32_reset(&s->tx_fifo);
> + s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TE;
> + s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_TF;
> +}
> +
> +static void imx_spi_rxfifo_reset(IMXSPIState *s)
> +{
> + fifo32_reset(&s->rx_fifo);
> + s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RR;
> + s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RF;
> + s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RO;
> +}
> +
> +static void imx_spi_update_irq(IMXSPIState *s)
> +{
> + int level;
> +
> + if (fifo32_is_empty(&s->rx_fifo)) {
> + s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RR;
> + } else {
> + s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RR;
> + }
> +
> + if (fifo32_is_full(&s->rx_fifo)) {
> + s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RF;
> + } else {
> + s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RF;
> + }
> +
> + if (fifo32_is_empty(&s->tx_fifo)) {
> + s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TE;
> + } else {
> + s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_TE;
> + }
> +
> + if (fifo32_is_full(&s->tx_fifo)) {
> + s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TF;
> + } else {
> + s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_TF;
> + }
> +
> + level = s->regs[ECSPI_STATREG] & s->regs[ECSPI_INTREG] ? 1 : 0;
> +
> + if (s->previous_level != level) {
> + DPRINTF("setting IRQ a level %d\n", level);
> + s->previous_level = level;
> + qemu_set_irq(s->irq, level);
> + }
s->previous_level isn't real hardware device state, and it isn't
in your migration struct either. You can either:
(a) just go ahead and always call qemu_set_irq() even if maybe
the level didn't change
(b) calculate previous_level before you change any of the
STATREG or INTREG bits
(a) is simplest.
> + DPRINTF("IRQ level is %d\n", level);
> +}
> +
> +static uint8_t imx_spi_selected_channel(IMXSPIState *s)
> +{
> + return EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_CHANNEL_SELECT);
> +}
> +
> +static uint32_t imx_spi_burst_length(IMXSPIState *s)
> +{
> + return EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_BURST_LENGTH) + 1;
> +}
> +
> +static bool imx_spi_is_enabled(IMXSPIState *s)
> +{
> + return (s->regs[ECSPI_CONREG] & ECSPI_CONREG_EN) ? true : false;
"X ? true : false" as a bool is just "X".
> +}
> +
> +static bool imx_spi_channel_is_master(IMXSPIState *s)
> +{
> + uint8_t mode = EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_CHANNEL_MODE);
> +
> + return (mode & (1 << imx_spi_selected_channel(s))) ? true : false;
> +}
> +
> +static bool imx_spi_is_multiple_master_burst(IMXSPIState *s)
> +{
> + uint8_t wave = EXTRACT(s->regs[ECSPI_CONFIGREG], ECSPI_CONFIGREG_SS_CTL);
> +
> + return imx_spi_channel_is_master(s) &&
> + !(s->regs[ECSPI_CONREG] & ECSPI_CONREG_SMC) &&
> + ((wave & (1 << imx_spi_selected_channel(s))) ? true : false);
> +}
> +
> +static void imx_spi_flush_txfifo(IMXSPIState *s)
> +{
> + uint32_t tx;
> + uint32_t rx;
> +
> + DPRINTF("Begin: TX Fifo Size = %d, RX Fifo Size = %d\n",
> + fifo32_num_used(&s->tx_fifo), fifo32_num_used(&s->rx_fifo));
> +
> + while (!fifo32_is_empty(&s->tx_fifo)) {
> + int tx_burst = 0;
> +
> + if (s->burst_length <= 0) {
> + s->burst_length = imx_spi_burst_length(s);
Why is s->burst_length in your state struct? It isn't in your
migration state structure. Does it really correspond to
hardware state that isn't the same as the register field?
> +
> + DPRINTF("Burst length = %d\n", s->burst_length);
> +
> + if (imx_spi_is_multiple_master_burst(s)) {
> + s->regs[ECSPI_CONREG] |= ECSPI_CONREG_XCH;
> + }
> + }
> +
> + tx = fifo32_pop(&s->tx_fifo);
> +
> + DPRINTF("data tx:0x%08x\n", tx);
> +
> + tx_burst = MIN(s->burst_length, 32);
> +
> + rx = 0;
> +
> + while (tx_burst) {
> + rx = rx << 8;
> +
> + DPRINTF("writing 0x%02x\n", tx & 0xff);
> +
> + /* We need to write one byte at a time */
> + rx |= ssi_transfer(s->bus, tx & 0xFF) & 0xFF;
> +
> + DPRINTF("0x%02x read\n", rx & 0xff);
Consistency about whether you write hex as 0xFF or 0xff would be nice.
> +
> + tx = tx >> 8;
> +
> + /* Remove 8 bits from the actual burst */
> + tx_burst -= 8;
> +
> + s->burst_length -= 8;
> + }
> +
> + DPRINTF("data rx:0x%08x\n", rx);
> +
> + if (fifo32_is_full(&s->rx_fifo)) {
> + s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RO;
> + } else {
> + fifo32_push(&s->rx_fifo, (uint8_t)rx);
> + }
> +
> + if (s->burst_length <= 0) {
> + s->regs[ECSPI_CONREG] &= ~ECSPI_CONREG_XCH;
> +
> + if (!imx_spi_is_multiple_master_burst(s)) {
> + s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TC;
> + break;
> + }
> + }
> + }
> +
> + if (fifo32_is_empty(&s->tx_fifo)) {
> + s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TC;
> + }
> +
> + /* TODO: We should also use TDR and RDR bits */
> +
> + DPRINTF("End: TX Fifo Size = %d, RX Fifo Size = %d\n",
> + fifo32_num_used(&s->tx_fifo), fifo32_num_used(&s->rx_fifo));
> +}
> +
> +static void imx_spi_reset(DeviceState *dev)
> +{
> + IMXSPIState *s = IMX_SPI(dev);
> + int i;
> +
> + DPRINTF("\n");
> +
> + memset(s->regs, 0, ECSPI_MAX * sizeof(uint32_t));
You could just write sizeof(s->regs).
> +
> + s->regs[ECSPI_STATREG] = 0x00000003;
> +
> + imx_spi_rxfifo_reset(s);
> + imx_spi_txfifo_reset(s);
> +
> + imx_spi_update_irq(s);
Updating outbound IRQs in a reset function is better avoided
(though this area of QEMU is a mess).
> +
> + s->burst_length = 0;
> +
> + for (i=0; i<4; i++) {
> + qemu_set_irq(s->cs_lines[i], 0);
> + }
> +}
> +
> +static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
> +{
> + uint32 value = 0;
> + IMXSPIState *s = (IMXSPIState *)opaque;
You don't need to explicitly cast void pointers.
> + uint32_t index = offset >> 2;
> +
> + if (index >= ECSPI_MAX) {
> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
> + HWADDR_PRIx "\n", TYPE_IMX_SPI, __func__, offset);
> + return 0;
> + }
> +
> + switch (index) {
> + case ECSPI_RXDATA:
> + if (!imx_spi_is_enabled(s)) {
> + value = 0;
> + } else if (fifo32_is_empty(&s->rx_fifo)) {
> + value = 0xdeadbeef;
Is this really what the hardware does? If the h/w spec says "undefined value"
or something then it's worth a comment to that effect.
> + } else {
> + /* read from the RX FIFO */
> + value = fifo32_pop(&s->rx_fifo);
> + }
> +
> + break;
> + case ECSPI_TXDATA:
> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to read from TX
> FIFO\n",
> + TYPE_IMX_SPI, __func__);
> +
> + /* Reading from TXDATA gives 0 */
> +
> + break;
> + case ECSPI_MSGDATA:
> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to read from MSG
> FIFO\n",
> + TYPE_IMX_SPI, __func__);
> +
> + /* Reading from MSGDATA gives 0 */
> +
> + break;
> + default:
> + value = s->regs[index];
> + break;
> + }
> +
> + DPRINTF("reg[%s] => 0x%" PRIx32 "\n", imx_spi_reg_name(index), value);
> +
> + imx_spi_update_irq(s);
> +
> + return (uint64_t)value;
> +}
> +
> +static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
> + unsigned size)
> +{
> + IMXSPIState *s = (IMXSPIState *)opaque;
> + uint32_t index = offset >> 2;
> + uint32_t change_mask;
> +
> + if (index >= ECSPI_MAX) {
> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
> + HWADDR_PRIx "\n", TYPE_IMX_SPI, __func__, offset);
> + return;
> + }
> +
> + DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx_spi_reg_name(index),
> + (uint32_t)value);
> +
> + change_mask = s->regs[index] ^ value;
> +
> + switch (index) {
> + case ECSPI_RXDATA:
> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to write to RX
> FIFO\n",
> + TYPE_IMX_SPI, __func__);
> + break;
> + case ECSPI_TXDATA:
> + case ECSPI_MSGDATA:
> + /* Is there any difference between TXDATA and MSGDATA ? */
> + /* I'll have to look in the linux driver */
That sounds like it would be a good plan :-)
> + if (!imx_spi_is_enabled(s)) {
> + /* Ignore writes if device is disabled */
> + break;
> + } else if (fifo32_is_full(&s->tx_fifo)) {
> + /* Ignore writes if queue is full */
> + break;
> + }
> +
> + fifo32_push(&s->tx_fifo, (uint32_t)value);
> +
> + if (imx_spi_channel_is_master(s) &&
> + (s->regs[ECSPI_CONREG] & ECSPI_CONREG_SMC)) {
> + /*
> + * Start emiting if current channel is master and SMC bit is
"emitting"
> + * set.
> + */
> + imx_spi_flush_txfifo(s);
> + }
> +
> + break;
> + case ECSPI_STATREG:
> + /* Clear RO and TC bits on write */
Do you mean "the RO and TC bits are write-one-to-clear" ?
> + value &= ECSPI_STATREG_RO | ECSPI_STATREG_TC;
> + s->regs[ECSPI_STATREG] &= ~value;
> +
> + break;
> + case ECSPI_CONREG:
> + s->regs[ECSPI_CONREG] = value;
> +
> + if (!imx_spi_is_enabled(s)) {
> + /* device is diabled, so this is a reset */
"disabled"
> + imx_spi_reset(DEVICE(s));
> + return;
> + }
> +
> + if (imx_spi_channel_is_master(s)) {
> + int i;
> +
> + /* We are in master mode */
> +
> + for (i=0; i<4; i++) {
> + qemu_set_irq(s->cs_lines[i],
> + i == imx_spi_selected_channel(s) ? 0 : 1);
> + }
> +
> + if ((value & change_mask & ECSPI_CONREG_SMC) &&
> + !fifo32_is_empty(&s->tx_fifo)) {
> + /* SMC bit is set and TX FIFO has some slots filled in */
> + imx_spi_flush_txfifo(s);
> + } else if ((value & change_mask & ECSPI_CONREG_XCH) &&
> + !(value & ECSPI_CONREG_SMC)) {
> + /* This is a request to start emiting */
"emitting"
> + imx_spi_flush_txfifo(s);
> + }
> + }
> +
> + break;
> + default:
> + s->regs[index] = value;
> +
> + break;
> + }
> +
> + imx_spi_update_irq(s);
> +}
thanks
-- PMM