| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 23 Aug 2010 19:30:34 +0200
From: Linus Walleij <linus.ml.walleij@...il.com>
To: Sascha Hauer <s.hauer@...gutronix.de>
Cc: linux-kernel@...r.kernel.org,
Dan Williams <dan.j.williams@...el.com>,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 3/3 v2] dmaengine: Add Freescale i.MX SDMA support
2010/8/23 Sascha Hauer <s.hauer@...gutronix.de>:
> 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@...ricsson.com>
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists