Re: [PATCH 3/3 v2] dmaengine: Add Freescale i.MX SDMA support

From: Linus Walleij
Date: Mon Aug 23 2010 - 13:30:43 EST


2010/8/23 Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>:

> This patch adds support for the Freescale i.MX SDMA engine.

Great progress!

> (...)
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> (...)
> +/* SDMA registers */
> +#define SDMA_H_C0PTR           (sdma->regs + 0x000)
> +#define SDMA_H_INTR            (sdma->regs + 0x004)
> +#define SDMA_H_STATSTOP                (sdma->regs + 0x008)
> +#define SDMA_H_START           (sdma->regs + 0x00c)
> +#define SDMA_H_EVTOVR          (sdma->regs + 0x010)
> +#define SDMA_H_DSPOVR          (sdma->regs + 0x014)
> +#define SDMA_H_HOSTOVR         (sdma->regs + 0x018)
> +#define SDMA_H_EVTPEND         (sdma->regs + 0x01c)
> +#define SDMA_H_DSPENBL         (sdma->regs + 0x020)
> +#define SDMA_H_RESET           (sdma->regs + 0x024)
> +#define SDMA_H_EVTERR          (sdma->regs + 0x028)
> +#define SDMA_H_INTRMSK         (sdma->regs + 0x02c)
> +#define SDMA_H_PSW             (sdma->regs + 0x030)
> +#define SDMA_H_EVTERRDBG       (sdma->regs + 0x034)
> +#define SDMA_H_CONFIG          (sdma->regs + 0x038)
> +#define SDMA_ONCE_ENB          (sdma->regs + 0x040)
> +#define SDMA_ONCE_DATA         (sdma->regs + 0x044)
> +#define SDMA_ONCE_INSTR                (sdma->regs + 0x048)
> +#define SDMA_ONCE_STAT         (sdma->regs + 0x04c)
> +#define SDMA_ONCE_CMD          (sdma->regs + 0x050)
> +#define SDMA_EVT_MIRROR                (sdma->regs + 0x054)
> +#define SDMA_ILLINSTADDR       (sdma->regs + 0x058)
> +#define SDMA_CHN0ADDR          (sdma->regs + 0x05c)
> +#define SDMA_ONCE_RTB          (sdma->regs + 0x060)
> +#define SDMA_XTRIG_CONF1       (sdma->regs + 0x070)
> +#define SDMA_XTRIG_CONF2       (sdma->regs + 0x074)
> +#define SDMA_CHNENBL_0         (sdma->regs + (sdma->version == 2 ? 0x200 : 0x80))
> +#define SDMA_CHNPRI_0          (sdma->regs + 0x100)

These macros expand to the local variable "sdma" which must
be present in all functions using them. I don't know what is
considered moste readable, but I would certainly just

#define SDMA_FOO (0x0123)
(...)
u32 foo = readl(sdma->regs + SDMA_FOO);

That is more common I think.

> (...)
> +/*
> + * Channel control Block

Some kerneldoc here describing especially
@unused: padding for register cast
@usused1: padding for register cast

Hm "unused" and "unused1" wrinkles my binary brain,
can you call the first one "unused0" for my perceptions
sake?

> + */
> +struct sdma_channel_control {
> +       dma_addr_t current_bd_ptr; /* current buffer descriptor processed */
> +       dma_addr_t base_bd_ptr;    /* first element of buffer descriptor array */
> +       u32 unused;
> +       u32 unused1;
> +} __attribute__ ((packed));

> (...)
> +/**
> + * struct sdma_channel - housekeeping for a SDMA channel
> + *
> + * @sdma               pointer to the SDMA engine for this channel
> + * @channel            the channel number, matches dmaengine chan_id
> + * @direction          transfer type. Needed for setting SDMA script
> + * @peripheral_type    Peripheral type. Needed for setting SDMA script
> + * @event_id           aka dma request line
> + * @event_id2          for channels that use 2 events
> + * @word_size          peripheral access size
> + * @buf_tail           ID of the buffer that was processed
> + * @done               channel completion
> + * @num_bd             max NUM_BD. number of descriptors currently handling
> + */
> +struct sdma_channel {
> +       struct sdma_engine              *sdma;
> +       unsigned int                    channel;
> +       enum dma_data_direction         direction;
> +       enum sdma_peripheral_type       peripheral_type;
> +       unsigned int                    event_id;
> +       unsigned int                    event_id2;

id1 und id2 oder
id0 und id1 fierleicht?

> +       enum dma_slave_buswidth         word_size;
> +       unsigned int                    buf_tail;
> +       struct completion               done;
> +       unsigned int                    num_bd;
> +       struct sdma_buffer_descriptor   *bd;
> +       dma_addr_t                      bd_phys;
> +       unsigned int                    pc_from_device, pc_to_device;
> +       unsigned long                   flags;
> +       dma_addr_t                      per_address;
> +       u32                             event_mask1, event_mask2;

1 and 2, else unnumbered and 1, else unnumbered and 2 X-)

> +       u32                             watermark_level;
> +       u32                             shp_addr, per_addr;
> +       struct dma_chan                 chan;
> +       spinlock_t                      lock;
> +       struct dma_async_tx_descriptor  desc;
> +       dma_cookie_t                    last_completed;
> +       enum dma_status                 status;
> +};
> +
> +#define IMX_DMA_SG_LOOP                (1 << 0)
> +
> +#define MAX_DMA_CHANNELS 32
> +#define MXC_SDMA_DEFAULT_PRIORITY 1
> +#define MXC_SDMA_MIN_PRIORITY 1
> +#define MXC_SDMA_MAX_PRIORITY 7
> +
> +/*
> + * This enumerates transfer types
> + */
> +enum {
> +       emi_2_per = 0,          /* EMI memory to peripheral */
> +       emi_2_int,              /* EMI memory to internal RAM */
> +       emi_2_emi,              /* EMI memory to EMI memory */
> +       emi_2_dsp,              /* EMI memory to DSP memory */
> +       per_2_int,              /* Peripheral to internal RAM */
> +       per_2_emi,              /* Peripheral to internal EMI memory */
> +       per_2_dsp,              /* Peripheral to DSP memory */
> +       per_2_per,              /* Peripheral to Peripheral */
> +       int_2_per,              /* Internal RAM to peripheral */
> +       int_2_int,              /* Internal RAM to Internal RAM */
> +       int_2_emi,              /* Internal RAM to EMI memory */
> +       int_2_dsp,              /* Internal RAM to DSP memory */
> +       dsp_2_per,              /* DSP memory to peripheral */
> +       dsp_2_int,              /* DSP memory to internal RAM */
> +       dsp_2_emi,              /* DSP memory to EMI memory */
> +       dsp_2_dsp,              /* DSP memory to DSP memory */
> +       emi_2_dsp_loop,         /* EMI memory to DSP memory loopback */
> +       dsp_2_emi_loop,         /* DSP memory to EMI memory loopback */
> +       dvfs_pll,               /* DVFS script with PLL change       */
> +       dvfs_pdr                /* DVFS script without PLL change    */
> +} sdma_transfer_type;

Picky me, but it's no type, its an enum. I understand that it is
a technical term...

What about just calling is sdma_transfer? Short and nice.
Or sdma_transfer_line?

> (...)
> +/*
> + * Stores the start address of the SDMA scripts
> + */
> +static struct sdma_script_start_addrs __sdma_script_addrs;
> +static struct sdma_script_start_addrs *sdma_script_addrs = &__sdma_script_addrs;

What's the rationale behind prefixing that variable with __?

The same name for struct and variable is perfectly viable.

Apart from these smallies (and it's all minor stuff) it's nice and clean so:

Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxxxxxx>

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/