[PATCH] Add GPIO, I2C and SPI support for the RPi BSP

[Gedare Bloom]

On Tue, Nov 4, 2014 at 6:01 PM, Andre Marques
<andre.lousa.marques at gmail.com> wrote:
> Hi gedare,
>
> will send a new version of this patch in the next few days.
>
> Replies to your comments below.
>
> --André Marques.
>
> On 10/31/14 14:25, Gedare Bloom wrote:
>>
>> Thanks, I'll leave detailed comments to the experts but noticed a few
>> things.
>>
>> On Fri, Oct 31, 2014 at 7:56 AM, Andre Marques
>> <andre.lousa.marques at gmail.com> wrote:
>>>
[...]
>>> +    return -1;
>>
>> Consider using more informative error codes.
>
>
> Should I print something before the return?
>
No, but you could return something that maps to an rtems_status_code.
I don't have an example off-hand, but the idea is to give a better
clue to users about what went wrong.

[,,,]
>>> +  /* Call the user's ISR. */
>>> +  (handler_args->handler) ();
>>
>> Probably should pass the handler_args to the handler.
>
>
> The handler args are processed in this generic handler, as it avoids calling
> the handler when the interrupt was generated by a switch bounce. The called
> handler (given by an application) receives no arguments.
>
I think Pavel made some comments regarding passing arguments. It is
usually a good idea to provide some way to pass at least the "context"
of an ISR to the handler, so it can make informed decisions about what
to do. This is especially important when one handler de-multiplexes
several interrupt sources.

[...]
>>> +/**
>>> + * @brief Reads/writes to/from the I2C bus.
>>> + *
>>> + * @param[in] bushdl Pointer to the libi2c API bus driver data
>>> structure.
>>> + * @param[in] rd_buf Read buffer. If not NULL the function will read
>>> from
>>> + *                   the bus and store the read on this buffer.
>>> + * @param[in] wr_buf Write buffer. If not NULL the function will write
>>> the
>>> + *                   contents of this buffer to the bus.
>>> + * @param[in] buffer_size Size of the non-NULL buffer.
>>> + *
>>> + * @retval -1 Could not send/receive data to/from the bus.
>>> + * @retval >=0 The number of bytes read/written.
>>> + */
>>> +static int bcm2835_i2c_read_write(rtems_libi2c_bus_t * bushdl, unsigned
>>> char *rd_buf, const unsigned char *wr_buf, int buffer_size)
>>> +{
>>> +  bcm2835_i2c_softc_t *softc_ptr = &(((bcm2835_i2c_desc_t
>>> *)(bushdl))->softc);
>>> +
>>> +  uint32_t bytes_sent = buffer_size;
>>> +
>>> +  /* Since there is a maximum of 0xFFFF packets per transfer
>>> +   * (size of the DLEN register), count how many transfers will be
>>> +   * needed and adjust each transfer size accordingly. */
>>> +  int transfer_count = buffer_size / 0xFFFF;
>>> +  uint16_t dlen_buffer_size;
>>> +
>>> +  /* If the buffer size is a multiple of the max size per transfer,
>>> +   * round up the transfer count. */
>>> +  if ( buffer_size % 0xFFFF != 0 )
>>> +    transfer_count++;
>>> +
>>
>> You can calculate transfer_count more efficiently with
>> transfer_count = (buffer_size + 0xFFFE) / 0xFFFF;
>>
>> In general, ceil(x/n) = (x + (n-1))/n
>>
>>> +  do {
>>> +    if (transfer_count > 1)
>>> +      dlen_buffer_size = 0xFFFF;
>>> +    else
>>> +      dlen_buffer_size = (buffer_size & 0xFFFF);
>>> +
>>> +    /* Set the DLEN register, which specifies how many data packets will
>>> be transferred. */
>>> +    BCM2835_REG(BCM2835_I2C_DLEN) = dlen_buffer_size;
>>> +
>>> +    /* Clear the acknowledgment and clock stretching error status. */
>>> +    BCM2835_REG(BCM2835_I2C_S) |= (3 << 8);
>>> +
>>> +    /* While there is data to transfer. */
>>> +    while ( dlen_buffer_size >= 1 ) {
>>> +
>>> +      /* If writing. */
>>> +      if ( rd_buf == NULL ) {
>>> +
>>> +        /* If transfer is not active, send start bit. */
>>> +        if( (BCM2835_REG(BCM2835_I2C_S) & (1 << 0)) == 0)
>>> +          BCM2835_REG(BCM2835_I2C_C) |= (1 << 7);
>>> +
>>> +        /* If using the I2C bus in interrupt-driven mode. */
>>> +        if ( I2C_IO_MODE == 1 ) {
>>> +
>>> +          /* Generate interrupts on the TXW bit condition. */
>>> +          BCM2835_REG(BCM2835_I2C_C) |= (1 << 9);
>>> +
>>> +          if ( rtems_semaphore_obtain(softc_ptr->irq_sema_id,
>>> RTEMS_WAIT, 50) != RTEMS_SUCCESSFUL )
>>
>> Is the semaphore accounted for in confdefs?
>
>
> I account for it in my test cases but I am not sure if I mentioned this in
> the documentation. Will check that.
>
>>> +            return -1;
>>> +        }
>>> +
>>> +        /* If using the bus in polling mode. */
>>> +        else
>>> +          /* Poll TXW bit until there is space available to write. */
>>> +          while ( (BCM2835_REG(BCM2835_I2C_S) & (1 << 2)) == 0 )
>>> +            ;
>>> +
>>> +        /* Write data to the TX FIFO. */
>>> +        BCM2835_REG(BCM2835_I2C_FIFO) = (*(uint8_t *)wr_buf);
>>> +
>>> +        wr_buf++;
>>> +
>>> +        /* Check for acknowledgment or clock stretching errors. */
>>> +        if ( (BCM2835_REG(BCM2835_I2C_S) & (1 << 8)) ||
>>> (BCM2835_REG(BCM2835_I2C_S) & (1 << 9)) )
>>> +          return -1;
>>> +      }
>>> +
>>> +      /* If reading. */
>>> +      else {
>>> +        /* Send start bit. Before any read a libi2c_send_addr call
>>> should be made signaling a read operation. */
>>> +        BCM2835_REG(BCM2835_I2C_C) |= (1 << 7);
>>> +
>>> +        /* Check for an acknowledgment error. */
>>> +        if ( (BCM2835_REG(BCM2835_I2C_S) & (1 << 8)) != 0)
>>> +          return -1;
>>> +
>>> +        /* Poll RXD bit until there is data on the RX FIFO to read. */
>>> +        while ( (BCM2835_REG(BCM2835_I2C_S) & (1 << 5)) == 0 )
>>> +          ;
>>> +
>>> +        /* Read data from the RX FIFO. */
>>> +        (*(uint8_t *)rd_buf) = BCM2835_REG(BCM2835_I2C_FIFO) & 0xFF;
>>> +
>>> +        rd_buf++;
>>> +
>>> +        /* Check for acknowledgment or clock stretching errors. */
>>> +        if ( (BCM2835_REG(BCM2835_I2C_S) & (1 << 8)) ||
>>> (BCM2835_REG(BCM2835_I2C_S) & (1 << 9)) )
>>> +          return -1;
>>> +      }
>>> +
>>> +      dlen_buffer_size--;
>>> +      transfer_count--;
>>> +      buffer_size--;
>>
>> Is this right? It seems like transfer_count--; should go after the next }.
>
>
> The transfer_count is the total number of byte transfers -1, because each of
> these transfers use the full 32 bits of the DLEN register. The -1 part is
> because the last transfer may not need the full register to hold the
> remaining data. A full while loop will send 4 bytes of information, so if
> the transfer_count-- goes after the next } it must become transfer_count
> -=4.
>
I still didn't quite follow. The transfer_count is a scaled value of
buffer_size. It does not make sense to me that both get decremented by
1 at the same point in time. Thus my confusion, and my intuition tells
me there is a bug here. Perhaps there is a problem with buffer_size?
Should it be buffer_size -= 4?

>>
>>> +    }
>>> +  } while ( transfer_count > 0 );
>>> +
>>> +  /* If using the I2C bus in interrupt-driven mode. */
>>> +  if ( I2C_IO_MODE == 1 ) {
>>> +    /* Generate interrupts on the DONE bit condition. */
>>> +    BCM2835_REG(BCM2835_I2C_C) |= (1 << 8);
>>> +
>>> +    if ( rtems_semaphore_obtain(softc_ptr->irq_sema_id, RTEMS_WAIT, 50)
>>> != RTEMS_SUCCESSFUL )
>>> +      return -1;
>>> +  }
>>> +
>>> +  /* If using the bus in polling mode. */
>>> +  else
>>> +    /* Poll DONE bit until data has been sent. */
>>> +    while ( (BCM2835_REG(BCM2835_I2C_S) & (1 << 1)) == 0 )
>>> +      ;
>>> +
>>> +  bytes_sent -= buffer_size;
>>> +
>>> +  return bytes_sent;
Did you verify this is correct?

>>> +}