aux-spi, aux-uart interrupt issues

[franzflasch]

Hi! I am using the rpi-compute module with kernel version 4.4 and got some issues when using aux spi1 and aux spi2 simultaneously. On the aux spi1 I've connected an spi lcd device and on aux spi2 I've connected an sd card, which I am using with the mmc_spi driver. When using both devices simultaneously I get errors when reading from the mmc card. I also backported the latest spi-bcm2835aux driver from kernel 4.6 which introduced some fixes.

I think the problem is, that the interrupt handlers are not implemented properly:

static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
{
    struct spi_master *master = dev_id;
    struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
    irqreturn_t ret = IRQ_NONE;

    /* check if we have data to read */
    while (bs->rx_len &&
           (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
          BCM2835_AUX_SPI_STAT_RX_EMPTY))) {
        bcm2835aux_rd_fifo(bs);
        ret = IRQ_HANDLED;
    }

    /* check if we have data to write */
    while (bs->tx_len &&
           (bs->pending < 12) &&
           (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
          BCM2835_AUX_SPI_STAT_TX_FULL))) {
        bcm2835aux_wr_fifo(bs);
        ret = IRQ_HANDLED;
    }

    /* and check if we have reached "done" */
    while (bs->rx_len &&
           (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
          BCM2835_AUX_SPI_STAT_BUSY))) {
        bcm2835aux_rd_fifo(bs);
        ret = IRQ_HANDLED;
    }

    if (!bs->tx_len) {
        /* disable tx fifo empty interrupt */
        bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1] |
            BCM2835_AUX_SPI_CNTL1_IDLE);
    }

    /* and if rx_len is 0 then disable interrupts and wake up completion */
    if (!bs->rx_len) {
        bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
        complete(&master->xfer_completion);
    }

    /* and return */
    return ret;
}

I can imagine that this routine will fail if there are massive interrupt requests simultaneously on aux spi1 and aux spi2.

Some notes regarding these issues:

1. The bcm2835 has an interrupt status register on 0x7E21 5000 "AUX_IRQ" - however the aux spi drivers seem to ignore this status register completely. Shouldn't it be checked if the interrupt belongs to the corresponding device?
2. I've already begun to fix these issues, but realized that there is another issue: According to the device tree of the compute module the above mentioned register is already mapped in the clk-bcm2835-aux.c driver:

        aux: aux@0x7e215004 {
            compatible = "brcm,bcm2835-aux";
            #clock-cells = <1>;
            reg = <0x7e215000 0x8>;
            clocks = <&clk_core>;
            status = "disabled";
        };

However the aux irq register is not used anywhrere in the driver.

I think it should be corrected to this:

        aux: aux@0x7e215004 {
            compatible = "brcm,bcm2835-aux";
            #clock-cells = <1>;
            reg = <0x7e215004 0x4>;
            clocks = <&clk_core>;
            status = "disabled";
        };

The main issue is, that the IRQ status register is not checked within the aux spi driver. It would be great if someone can take a look at this and fixes this.

[franzflasch]

OK. I've fixed it myself. I really can't understand why the auxirq register was not taken into account in the spi drivers. If someone is interested: I've changed the interrupt routine to this:

static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
{
    struct spi_master *master = dev_id;
    struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
    u32 irq_flags = readl(bcm2835_aux_irq_reg);

    if(!(irq_flags & (1<<master->bus_num)))
    {
        return IRQ_NONE;
    }

    /* check if we have data to read */
    while (bs->rx_len &&
           (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
          BCM2835_AUX_SPI_STAT_RX_EMPTY))) {
        bcm2835aux_rd_fifo(bs);
    }

    /* check if we have data to write */
    while (bs->tx_len &&
           (bs->pending < 12) &&
           (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
          BCM2835_AUX_SPI_STAT_TX_FULL))) {
        bcm2835aux_wr_fifo(bs);
    }

    /* and check if we have reached "done" */
    while (bs->rx_len &&
           (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
          BCM2835_AUX_SPI_STAT_BUSY))) {
        bcm2835aux_rd_fifo(bs);
    }

    if (!bs->tx_len) {
        /* disable tx fifo empty interrupt */
        bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1] |
            BCM2835_AUX_SPI_CNTL1_IDLE);
    }

    /* and if rx_len is 0 then disable interrupts and wake up completion */
    if (!bs->rx_len) {
        bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
        complete(&master->xfer_completion);
    }

    /* and return */
    return IRQ_HANDLED;;
}

The variable bcm2835_aux_irq_reg is an io mapped pointer which i've exported from the clk-bcm2835-aux.c driver to get access to the auxirq register.

It is just a quick and dirty hack and should be implemented in a clean way by someone who has the time for it. But I hope you guys get an idea whats wrong in the current driver. Now it is working like a charm ;)

[lategoodbye]

@franzflasch Thanks for providing a fix. Could you please create a patch based on linux-next (mainline) with your changes and send it to linux-rpi-kernel@lists.infradead.org ?

[franzflasch]

@lategoodbye Yes I can send a patch, however at first I need to reimplement this. My "fix" works, but it is just an ugly hack.

[lategoodbye]

Wonderful, but don't spend too much time in reimplementation. It's imporant that this issue is reported to the mailing list, the decision how this should be fixed finally lies to the maintainer. It's possible that we need to implement an additional instance which delegate the IRQs.

[pelwell]

The current trees now include a modified bcm2835-aux driver that also acts as an interrupt controller, and DT modifications to activate it. I've tested it with the console over ttyS0 running an MFRC reader on SPI1.

[franzflasch]

This is great! Thanks!