Using bitfields for DMA configuration

Hi

First of all, bitfields are well-defined though implementation dependent, this means that both the architecture and the compiler affects how a bitfield behaves. This is especially true when writing portable code shared between little and big endian architectures. The key thing is that they are well defined for each compiler/architecture.

Anyway, I find it convenient and less error prone to use bitfields, like for the DMA configuration. Also, if using named constants, the amount of comments required are also decreased. Less comments result in less risk of divergences between code and comments

A few suggestions, what does the community think?

Change the DMA_CONFIG type to a bitfield, found last in
github.com/pololu/wixel-sdk/blo … 2511_map.h
Suggestion is

// Endianess conversion for 8051 <-> DMA controller
#define DMA_ADDR(a) (((uint16_t) (a) << 8) | ((uint16_t) (a) >> 8))

// Note the reverse order of the bitfields in each byte!
struct DmaConfig {
    unsigned SRCADDR  : 16;
    unsigned DESTADDR : 16;

    unsigned LENH      : 5;
    unsigned VLEN      : 3;
    unsigned LENL      : 8;
    
    unsigned TRIG      : 5;
    unsigned TMODE     : 2;
    unsigned WORDSIZE  : 1;

    unsigned PRIORITY  : 2;
    unsigned M8        : 1;
    unsigned IRQMASK   : 1;
    unsigned DESTINC   : 2;
    unsigned SRCINC    : 2;
};[/code]

example code would look like this, configuring the DMA channel 3
[code]    // DMA Rx
    dmaConfig[3].SRCADDR  = DMA_ADDR(XDATA_SFR_ADDRESS(U0DBUF));
    dmaConfig[3].DESTADDR = DMA_ADDR(dmaRxPacket);

    // we simplify things by having a fixed SPI buffer size
    dmaConfig[3].LENL      = ((sizeof(dmaRxPacket) >> 0) & 0xff) + 0;

    dmaConfig[3].TMODE     = DMA_TransSingle;
    dmaConfig[3].TRIG      = DMA_TrigURX0; 

    dmaConfig[3].SRCINC    = DMA_Inc0;
    dmaConfig[3].DESTINC   = DMA_Inc1;
    dmaConfig[3].PRIORITY  = DMA_PrioHigh;

Change the dmaConfig variable to vector
Code found last in
github.com/pololu/wixel-sdk/blo … lude/dma.h
Suggestion

This means that initialization (done in library) would look like this
github.com/pololu/wixel-sdk/blo … /dma/dma.c

[code]DmaConfig XDATA dmaConfig[5];

void dmaInit()
{
DMA0CFG = (uint16)(dmaConfig + 0);
DMA1CFG = (uint16)(dmaConfig + 1);
}[/code]

Finally, using named constants for the different bit values for the DMA configuratino fields, Suggestion:

[code]enum {
DMA_TrigNONE = 0,
DMA_TrigPREV = 1,

DMA_TrigT1_CH0 = 2,
DMA_TrigT1_CH1,
DMA_TrigT1_CH2,

DMA_TrigT2_OVFL = 6,

DMA_TrigT3_CH0  = 7,
DMA_TrigT3_CH1,
DMA_TrigT4_CH0,
DMA_TrigT4_CH1,

DMA_TrigIOC_0 = 12,
DMA_TrigIOC_1,

DMA_TrigURX0 = 14,
DMA_TrigUTX0,
DMA_TrigURX1,
DMA_TrigUTX1,

DMA_TrigFLASH = 18,
DMA_TrigRADIO = 19,

DMA_TrigADC_CHALL = 20,
DMA_TrigADC_CH0,
DMA_TrigADC_CH1,
DMA_TrigADC_CH2,
DMA_TrigADC_CH3,
DMA_TrigADC_CH4,
DMA_TrigADC_CH5,
DMA_TrigADC_CH6,
DMA_TrigADC_CH7,

DMA_TrigENC_DW  = 29,
DMA_TrigENC_UP  = 30,

};

enum {
DMA_Inc0 = 0b00, // Incremented by 0
DMA_Inc1 = 0b01, // Incremented by 1
DMA_Inc2 = 0b10, // Incremented by 2
DMA_Dec1 = 0b11, // Decremented by 1
};

enum {
DMA_LengthVLEN = 0b000, // Length is LENL and LENH
DMA_LengthN0 = 0b010, // First byte + 0
DMA_LengthN1 = 0b001, // First byte + 1
DMA_LengthN2 = 0b011, // First byte + 2
DMA_LengthN3 = 0b100, // First byte + 3
};

enum {
DMA_PrioLOW = 0,
DMA_PrioNormal = 1,
DMA_PrioHigh = 2,
};

enum {
DMA_TransSingle = 0b00,
DMA_TransBlock = 0b01,
DMA_TransRepeatSingle = 0b10,
DMA_TransRepeatBlock = 0b11,
};[/code]

br
Mattias

Hello, Mattias.

dmaConfig[3].SRCINC    = DMA_Inc0;
dmaConfig[3].DESTINC   = DMA_Inc1;
dmaConfig[3].PRIORITY  = DMA_PrioHigh;

The last time I checked, the code above is not very well optimized by the compiler (SDCC), and will take a lot more time than writing an entire byte at once because it does three read-modify-write operations. I don’t have a specific example of an application where that difference was big enough to matter, but I think that DMA is often used in applications where speed matters so we shouldn’t make it slower than it needs to be.

The main reason that the Wixel DMA library exists is because the configurations for DMA1 through DMA4 have to be contiguous in memory, so we needed some library to coordinate that. There is no reason for the DMA library to touch DMA0CFG. Maybe DMA0 should be reserved for functions that run in the main loop and need to use DMA to do a fast copy operation, and those functions will always complete their DMA transfers before returning. Each different function that uses DMA0 in that way could define their own configuration struct, and simply change DMA0CFG to point to it when needed. We have not really written that many libraries or apps that use DMA yet, so we have not needed to do this yet.

–David

hi,

thats true, bitfields might be slower, as in this case with the SDCC suit.
Most of my uses configures DMA once and those part are not speed sensitive… but I agree that it is a good thing to be able to set whole bytes at a time for speed reasons.

in my code at the moment I’m using,

XDATA DmaConfig    dma0Config;
XDATA DmaConfig*   dma14Config = (DmaConfig XDATA *) ((uint8_t*)&dmaConfig - sizeof(DmaConfig));

in that way I can use bitfields for readability reasons and still access the existing dmaConfig.

having all five dma variables consecutive in memory would be nice though, as you still have the option of changing the DMA0CFG register to point different structure at any one time.

btw, as speed is of concern in radio_mac, why not have two pre-filled DMA_CONFIG variables,one for rx and one for tx?
Then just do a (though, it wastes two cycles I think), dmaTx could be initialized at compile time.

void radioMacTx(uint8 XDATA * packet)
{
    dmaConfig.radio = dmaTx;  
    dmaConfig.radio.SRCADDR = DMA_ADDR(packet); 

    radioMacState = RADIO_MAC_STATE_TX;
}

This makes the radioMacTx easier to read and the part that are “dynamic” during run time is “obvious” when doing code reviews, less static code that is in the way.

br