| 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
| ||
|
Message-ID: <20100816141540.GQ27749@pengutronix.de>
Date: Mon, 16 Aug 2010 16:15:40 +0200
From: Sascha Hauer <s.hauer@...gutronix.de>
To: Linus Walleij <linus.ml.walleij@...il.com>
Cc: linux-kernel@...r.kernel.org,
Dan Williams <dan.j.williams@...el.com>,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 3/3] dmaengine: Add Freescale i.MX SDMA support
Hi Linus,
Thank you for the review. Sorry for so many trival mistakes in the code,
but I'm staring at this code in many different variants for some time
now and I'm getting blind on it.
On Mon, Aug 16, 2010 at 02:21:06PM +0200, Linus Walleij wrote:
> 2010/8/16 Sascha Hauer <s.hauer@...gutronix.de>:
>
> > This patch adds support for the Freescale i.MX SDMA engine.
>
> I like it!
>
> > The SDMA engine is a scatter/gather DMA engine which is implemented
> > as a seperate coprocessor. SDMA needs its own firmware which is
> > requested using the standard request_firmware mechanism. The firmware
> > has different entry points for each peripheral type, so drivers
> > have to pass the peripheral type to the DMA engine which in turn
> > picks the correct firmware entry point from a table contained in
> > the firmware image itself.
>
> Quite fun, if the spec for the microcode is open this opens up
> for dynamic firmware generation for specific DMA jobs does it
> not?
Unfortunately the specs are not open, so we are sticked to the binary
microcode from Freescale. I'm pretty sure though that the SDMA engine
could do at least a device_prep_dma_xor operation.
>
> > I took a very simple approach to implement dmaengine support. Only
> > a single descriptor is statically assigned to a each channel. This
> > means that transfers can't be queued up but only a single transfer
> > is in progress. This simplifies implementation a lot and is sufficient
> > for the usual device/memory transfers.
>
> If you want to add memcpy() capability later you're gonna need
> this I think, but you can take that when that need arise.
Yes, I left this as an exercise for those who want to have this feature ;)
I think it's better to have tested code in the Kernel than having a
complicated list handling which is completely untested for anything
other than a single entry in the list.
>
> >(...)
> > +++ b/arch/arm/plat-mxc/include/mach/dma.h
> > @@ -0,0 +1,64 @@
> > +/*
> > + * Copyright 2004-2009 Freescale Semiconductor, Inc. All Rights Reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#ifndef __ASM_ARCH_MXC_DMA_H__
> > +#define __ASM_ARCH_MXC_DMA_H__
> > +
> > +#include <linux/scatterlist.h>
> > +
> > +/*
> > + * This enumerates peripheral types. Used for SDMA.
> > + */
> > +typedef enum {
>
> The kernel is not really happy about typedefs, can't this be a
> regular enum?
>
> > + IMX_DMATYPE_SSI, /* MCU domain SSI */
> > + IMX_DMATYPE_SSI_SP, /* Shared SSI */
> > + IMX_DMATYPE_MMC, /* MMC */
> > + IMX_DMATYPE_SDHC, /* SDHC */
> > + IMX_DMATYPE_UART, /* MCU domain UART */
> > + IMX_DMATYPE_UART_SP, /* Shared UART */
> > + IMX_DMATYPE_FIRI, /* FIRI */
> > + IMX_DMATYPE_CSPI, /* MCU domain CSPI */
> > + IMX_DMATYPE_CSPI_SP, /* Shared CSPI */
> > + IMX_DMATYPE_SIM, /* SIM */
> > + IMX_DMATYPE_ATA, /* ATA */
> > + IMX_DMATYPE_CCM, /* CCM */
> > + IMX_DMATYPE_EXT, /* External peripheral */
> > + IMX_DMATYPE_MSHC, /* Memory Stick Host Controller */
> > + IMX_DMATYPE_MSHC_SP, /* Shared Memory Stick Host Controller */
> > + IMX_DMATYPE_DSP, /* DSP */
> > + IMX_DMATYPE_MEMORY, /* Memory */
> > + IMX_DMATYPE_FIFO_MEMORY,/* FIFO type Memory */
> > + IMX_DMATYPE_SPDIF, /* SPDIF */
> > + IMX_DMATYPE_IPU_MEMORY, /* IPU Memory */
> > + IMX_DMATYPE_ASRC, /* ASRC */
> > + IMX_DMATYPE_ESAI, /* ESAI */
> > +} sdma_peripheral_type;
> > +
> > +enum imx_dma_prio {
> > + DMA_PRIO_HIGH = 0,
> > + DMA_PRIO_MEDIUM = 1,
> > + DMA_PRIO_LOW = 2
> > +};
> > +
> > +struct imx_dma_data {
> > + int dma_request; /* DMA request line */
>
> Can this be negative and what is the range? I would
> suspect something like u8 or u16 would surely be more
> apropriate...
>
> > + sdma_peripheral_type peripheral_type;
> > + int priority;
>
> Isn't this an enum imx_dma_prio?
>
> > +};
> > +
> > +static inline int imx_dma_is_ipu(struct dma_chan *chan)
> > +{
> > + return !strcmp(dev_name(chan->device->dev), "ipu-core");
> > +}
> > +
> > +static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
> > +{
> > + return !strcmp(dev_name(chan->device->dev), "imx-sdma");
> > +}
> > +
> > +#endif
> > diff --git a/arch/arm/plat-mxc/include/mach/sdma.h b/arch/arm/plat-mxc/include/mach/sdma.h
> > new file mode 100644
> > index 0000000..5d542b8
> > --- /dev/null
> > +++ b/arch/arm/plat-mxc/include/mach/sdma.h
> > @@ -0,0 +1,8 @@
> > +#ifndef __MACH_MXC_SDMA_H__
> > +#define __MACH_MXC_SDMA_H__
> > +
> > +struct sdma_platform_data {
> > + int sdma_version;
>
> Do you have negative versions or can it be unsigned?
nope, will change this to an unsigned type.
>
> > +};
> > +
> > +#endif /* __MACH_MXC_SDMA_H__ */
> > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> > index 9520cf0..f76bda9 100644
> > --- a/drivers/dma/Kconfig
> > +++ b/drivers/dma/Kconfig
> > @@ -195,6 +195,14 @@ config PCH_DMA
> > help
> > Enable support for the Topcliff PCH DMA engine.
> >
> > +config IMX_SDMA
> > + tristate "Atmel AHB DMA support"
> > + depends on ARCH_MXC
> > + select DMA_ENGINE
> > + help
> > + Support the i.MX SDMA engine. This engine is integrated into
> > + Freescale i.MX25/31/35/51 chips.
> > +
> > config DMA_ENGINE
> > bool
> >
> > diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> > index 72bd703..14d7a1b 100644
> > --- a/drivers/dma/Makefile
> > +++ b/drivers/dma/Makefile
> > @@ -25,3 +25,4 @@ obj-$(CONFIG_TIMB_DMA) += timb_dma.o
> > obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
> > obj-$(CONFIG_PL330_DMA) += pl330.o
> > obj-$(CONFIG_PCH_DMA) += pch_dma.o
> > +obj-$(CONFIG_IMX_SDMA) += imx-sdma.o
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > new file mode 100644
> > index 0000000..3ba7905
> > --- /dev/null
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -0,0 +1,1383 @@
> > +/*
> > + * drivers/dma/imx-sdma.c
> > + *
> > + * This file contains a driver for the Freescale Smart DMA engine
> > + *
> > + * Copyright 2010 Sascha Hauer, Pengutronix <s.hauer@...gutronix.de>
> > + *
> > + * Based on code from Freescale:
> > + *
> > + * Copyright 2004-2009 Freescale Semiconductor, Inc. All Rights Reserved.
> > + *
> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> > +#include <linux/init.h>
> > +#include <linux/types.h>
> > +#include <linux/mm.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/clk.h>
> > +#include <linux/semaphore.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/device.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/firmware.h>
> > +#include <linux/slab.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/dmaengine.h>
> > +
> > +#include <asm/irq.h>
> > +#include <mach/sdma.h>
> > +#include <mach/dma.h>
> > +#include <mach/hardware.h>
> > +
> > +/* SDMA registers */
> > +#define SDMA_H_C0PTR (sdma_base + 0x000)
> > +#define SDMA_H_INTR (sdma_base + 0x004)
> > +#define SDMA_H_STATSTOP (sdma_base + 0x008)
> > +#define SDMA_H_START (sdma_base + 0x00c)
> > +#define SDMA_H_EVTOVR (sdma_base + 0x010)
> > +#define SDMA_H_DSPOVR (sdma_base + 0x014)
> > +#define SDMA_H_HOSTOVR (sdma_base + 0x018)
> > +#define SDMA_H_EVTPEND (sdma_base + 0x01c)
> > +#define SDMA_H_DSPENBL (sdma_base + 0x020)
> > +#define SDMA_H_RESET (sdma_base + 0x024)
> > +#define SDMA_H_EVTERR (sdma_base + 0x028)
> > +#define SDMA_H_INTRMSK (sdma_base + 0x02c)
> > +#define SDMA_H_PSW (sdma_base + 0x030)
> > +#define SDMA_H_EVTERRDBG (sdma_base + 0x034)
> > +#define SDMA_H_CONFIG (sdma_base + 0x038)
> > +#define SDMA_ONCE_ENB (sdma_base + 0x040)
> > +#define SDMA_ONCE_DATA (sdma_base + 0x044)
> > +#define SDMA_ONCE_INSTR (sdma_base + 0x048)
> > +#define SDMA_ONCE_STAT (sdma_base + 0x04c)
> > +#define SDMA_ONCE_CMD (sdma_base + 0x050)
> > +#define SDMA_EVT_MIRROR (sdma_base + 0x054)
> > +#define SDMA_ILLINSTADDR (sdma_base + 0x058)
> > +#define SDMA_CHN0ADDR (sdma_base + 0x05c)
> > +#define SDMA_ONCE_RTB (sdma_base + 0x060)
> > +#define SDMA_XTRIG_CONF1 (sdma_base + 0x070)
> > +#define SDMA_XTRIG_CONF2 (sdma_base + 0x074)
> > +#define SDMA_CHNENBL_0 (sdma_base + (sdma_version == 2 ? 0x200 : 0x80))
> > +#define SDMA_CHNPRI_0 (sdma_base + 0x100)
>
> All these rely on a fixed sdma_base which makes the driver
> a singleton. This is not so good if you imagine the situation with a
> platform with two SDMA engines on different addresses.
>
> Can't you create a runtime allocated stateholder to hold
> the base and access relative to the offset?
This could be done since a dma channel is a pointer now. Originally
a channel was referenced by its number only. Doing this would only
be for the beauty of the code though since I don't think there will
be ever more than one SDMA engine in one SoC. Famous last words...
>
> > +
> > +/*
> > + * Buffer descriptor status values.
> > + */
> > +#define BD_DONE 0x01
> > +#define BD_WRAP 0x02
> > +#define BD_CONT 0x04
> > +#define BD_INTR 0x08
> > +#define BD_RROR 0x10
> > +#define BD_LAST 0x20
> > +#define BD_EXTD 0x80
> > +
> > +/*
> > + * Data Node descriptor status values.
> > + */
> > +#define DND_END_OF_FRAME 0x80
> > +#define DND_END_OF_XFER 0x40
> > +#define DND_DONE 0x20
> > +#define DND_UNUSED 0x01
> > +
> > +/*
> > + * IPCV2 descriptor status values.
> > + */
> > +#define BD_IPCV2_END_OF_FRAME 0x40
> > +
> > +#define IPCV2_MAX_NODES 50
> > +/*
> > + * Error bit set in the CCB status field by the SDMA,
> > + * in setbd routine, in case of a transfer error
> > + */
> > +#define DATA_ERROR 0x10000000
> > +
> > +/*
> > + * Buffer descriptor commands.
> > + */
> > +#define C0_ADDR 0x01
> > +#define C0_LOAD 0x02
> > +#define C0_DUMP 0x03
> > +#define C0_SETCTX 0x07
> > +#define C0_GETCTX 0x03
> > +#define C0_SETDM 0x01
> > +#define C0_SETPM 0x04
> > +#define C0_GETDM 0x02
> > +#define C0_GETPM 0x08
> > +/*
> > + * Change endianness indicator in the BD command field
> > + */
> > +#define CHANGE_ENDIANNESS 0x80
> > +
> > +/*
> > + * Mode/Count of data node descriptors - IPCv2
> > + */
> > +#ifdef __BIG_ENDIAN
> > +struct sdma_mode_count {
> > + u32 command : 8; /* command mostlky used for channel 0 */
>
> There are a lot of inline commented struct members, please
> use kerneldoc, that's simple. (Applies all over the patch...)
> Documentation/kernel-doc-nano-HOWTO
Ok.
>
> > + u32 status : 8; /* E,R,I,C,W,D status bits stored here */
> > + u32 count : 16; /* size of the buffer pointed by this BD */
> > +};
> > +#else
> > +struct sdma_mode_count {
> > + u32 count : 16; /* size of the buffer pointed by this BD */
> > + u32 status : 8; /* E,R,I,C,W,D status bits stored here */
> > + u32 command : 8; /* command mostlky used for channel 0 */
> > +};
> > +#endif
>
> This use of #ifdef is odd to me but others are probably more
> experienced. Anyway, the way it is used with different
> :n suffixes makes me believe that you need a packed
> compiler directive for this layout to be explicitly coherent.
>
> Atleast add some comment on what this #ifdef construction
> does so guys like me can understand what's going on.
This is a direct copy from the Freescale code. Since Linux does not
support i.MX SoCs in big endian modes I think we can remove the ifdef
completely. Adding this again will be the smallest problem when we want
to add big endian mode in the future.
>
> > +
> > +/*
> > + * Buffer descriptor
> > + */
> > +struct sdma_buffer_descriptor {
> > + struct sdma_mode_count mode;
> > + u32 buffer_addr; /* address of the buffer described */
> > + u32 ext_buffer_addr; /* extended buffer address */
>
> Shouldn't these be dma_addr_t? OK that's probably u32
> anyway but just to make a marker...
>
> > +};
> > +
> > +/*
> > + * Channel control Block
> > + */
> > +struct sdma_channel_control {
> > + u32 current_bd_ptr; /* current buffer descriptor processed */
> > + u32 base_bd_ptr; /* first element of buffer descriptor array */
> > + void *unused;
> > + void *unused1;
>
> Hm, can you comment on what these unused things are for...?
The SDMA engine expects an array of these structures (one for each
channel). The unused fields are just to make the structure the correct
size. They should be of type u32 though.
>
> > +};
> > +
> > +/**
> > + * Context structure.
> > + */
> > +#ifdef __BIG_ENDIAN
> > +struct sdma_state_registers {
> > + u32 sf : 1; /* source fault while loading data */
> > + u32 unused0: 1;
> > + u32 rpc :14; /* return program counter */
> > + u32 t : 1; /* test bit:status of arithmetic & test instruction*/
> > + u32 unused1: 1;
> > + u32 pc :14; /* program counter */
> > + u32 lm : 2; /* loop mode */
> > + u32 epc :14; /* loop end program counter */
> > + u32 df : 1; /* destination fault while storing data */
> > + u32 unused2: 1;
> > + u32 spc :14; /* loop start program counter */
> > +};
> > +#else
> > +struct sdma_state_registers {
> > + u32 pc :14; /* program counter */
> > + u32 unused1: 1;
> > + u32 t : 1; /* test bit: status of arithmetic & test instruction*/
> > + u32 rpc :14; /* return program counter */
> > + u32 unused0: 1;
> > + u32 sf : 1; /* source fault while loading data */
> > + u32 spc :14; /* loop start program counter */
> > + u32 unused2: 1;
> > + u32 df : 1; /* destination fault while storing data */
> > + u32 epc :14; /* loop end program counter */
> > + u32 lm : 2; /* loop mode */
> > +};
> > +#endif
>
> Again this is odd to me...
>
> > +
> > +struct sdma_context_data {
> > + struct sdma_state_registers channel_state; /* channel state bits */
> > + u32 gReg[8]; /* general registers */
> > + u32 mda; /* burst dma destination address register */
> > + u32 msa; /* burst dma source address register */
> > + u32 ms; /* burst dma status register */
> > + u32 md; /* burst dma data register */
> > + u32 pda; /* peripheral dma destination address register */
> > + u32 psa; /* peripheral dma source address register */
> > + u32 ps; /* peripheral dma status register */
> > + u32 pd; /* peripheral dma data register */
> > + u32 ca; /* CRC polynomial register */
> > + u32 cs; /* CRC accumulator register */
> > + u32 dda; /* dedicated core destination address register */
> > + u32 dsa; /* dedicated core source address register */
> > + u32 ds; /* dedicated core status register */
> > + u32 dd; /* dedicated core data register */
> > + u32 scratch0;
> > + u32 scratch1;
> > + u32 scratch2;
> > + u32 scratch3;
> > + u32 scratch4;
> > + u32 scratch5;
> > + u32 scratch6;
> > + u32 scratch7;
> > +};
> > +
> > +struct sdma_channel {
> > + /* Channel number */
> > + int channel;
>
> Unsigned?
>
> > + /* Transfer type. Needed for setting SDMA script */
> > + enum dma_data_direction direction;
> > + /* Peripheral type. Needed for setting SDMA script */
> > + sdma_peripheral_type peripheral_type;
> > + /* Peripheral event id */
> > + int event_id;
>
> Unsigned?
>
> > + /* Peripheral event id2 (for channels that use 2 events) */
> > + int event_id2;
>
> Unsigned?
Ok for all the 'unsigned'. Will change.
>
> > + /* SDMA data access word size */
> > + unsigned long word_size;
>
> Is this in bits, bytes etc? Isn't e.g. an u8 enough to hold this,
> and further, isn't it possible to recycle enum dma_slave_buswidth
> from dmaengine.h instead?
Yes, will change.
>
> > +
> > + /* ID of the buffer that was processed */
> > + unsigned int buf_tail;
> > +
> > + wait_queue_head_t waitq; /* channel completion waitqeue */
> > +
> > + int num_bd;
>
> Unsigned? Range?
>
> > +
> > + struct sdma_buffer_descriptor *bd;
> > + dma_addr_t bd_phys;
> > +
> > + int pc_from_device, pc_to_device;
>
> Unsigned?
>
> > +
> > + unsigned long flags;
>
> Is this an u32?
There is no need to tie this to a particular size.
>
> > + dma_addr_t per_address;
> > +
> > + uint32_t event_mask1, event_mask2;
> > + uint32_t watermark_level;
> > + uint32_t shp_addr, per_addr;
> > +
> > + /* DMA-Engine Channel */
> > + struct dma_chan chan;
> > +
> > + spinlock_t lock;
> > + struct dma_async_tx_descriptor desc;
> > + dma_cookie_t last_completed;
> > + int busy;
>
> Shouldn't this be a bool?
ok
>
> > +};
> > +
> > +#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
> > + */
> > +typedef enum {
>
> Again a typedef, please plain enum is fine.
>
> > + 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;
> > +
> > +/*
> > + * Structure containing sdma request parameters.
> > + */
> > +struct sdma_script_start_addrs {
> > + int ap_2_ap_addr;
> > + int ap_2_bp_addr;
> > + int ap_2_ap_fixed_addr;
> > + int bp_2_ap_addr;
> > + int loopback_on_dsp_side_addr;
> > + int mcu_interrupt_only_addr;
> > +
> > + int firi_2_per_addr;
> > + int firi_2_mcu_addr;
> > + int per_2_firi_addr;
> > + int mcu_2_firi_addr;
> > +
> > + int uart_2_per_addr;
> > + int uart_2_mcu_addr;
> > + int per_2_app_addr;
> > + int mcu_2_app_addr;
> > + int per_2_per_addr;
> > +
> > + int uartsh_2_per_addr;
> > + int uartsh_2_mcu_addr;
> > + int per_2_shp_addr;
> > + int mcu_2_shp_addr;
> > +
> > + int ata_2_mcu_addr;
> > + int mcu_2_ata_addr;
> > +
> > + int app_2_per_addr;
> > + int app_2_mcu_addr;
> > + int shp_2_per_addr;
> > + int shp_2_mcu_addr;
> > +
> > + int mshc_2_mcu_addr;
> > + int mcu_2_mshc_addr;
> > +
> > + int spdif_2_mcu_addr;
> > + int mcu_2_spdif_addr;
> > +
> > + int asrc_2_mcu_addr;
> > +
> > + int ext_mem_2_ipu_addr;
> > +
> > + int descrambler_addr;
> > +
> > + int dptc_dvfs_addr;
> > +
> > + int utra_addr;
> > +
> > + int ram_code_start_addr;
>
> All these addresses, are they really integers with
> valid negative values... Aren't they dma_addr_t or
> atleast u32?
Since this struct must match the layout of the firmware, they should be
u32, yes. They are no dma_addr_t since it's the sdma controller address
space described here.
>
> > +};
> > +
> > +#define SDMA_FIRMWARE_MAGIC 0x414d4453
> > +
> > +struct sdma_firmware_header {
> > + uint32_t magic; /* "SDMA" */
> > + uint32_t version_major; /* increased whenever layout of struct sdma_script_start_addrs changes */
> > + uint32_t version_minor; /* firmware version */
> > + uint32_t script_addrs_start; /* offset of struct sdma_script_start_addrs in this image */
> > + uint32_t num_script_addrs; /* Number of script addresses in this image */
> > + uint32_t ram_code_start; /* offset of SDMA ram image in this firmware image */
> > + uint32_t ram_code_size; /* size of SDMA ram image */
>
> Please use u32. uint32_t is not the preferred kernel type.
> (Still I've seen people use it in some cases so I might be wrong,
> feel welcome to bit back on this.)
At least one type should be consequently used in a driver. Changed them
all to u32.
>
> > +};
> > +
> > +static struct sdma_channel sdma_data[MAX_DMA_CHANNELS];
> > +static struct sdma_channel_control *channel_control;
> > +static void __iomem *sdma_base;
> > +static int sdma_version;
>
> Unsigned?
>
> > +static int sdma_num_events;
>
> Unsigned?
>
> > +static struct sdma_context_data *sdma_context;
> > +dma_addr_t sdma_context_phys;
> > +static struct dma_device __sdma_dma_device;
> > +static struct dma_device *sdma_dma_device = &__sdma_dma_device;
>
> This is what I suspected: local variables making the entire driver
> a singleton, which means you can never have more than one
> SDMA. Atleast collect all of these in a struct, call it
> "struct sdma" simply (if you ask me) and use as a stateholder.
> This makes it easier to kzalloc() that struct later if you
> want to support non-singletons.
>
> I know this require some work but I've done it to several drivers
> (always asked on mailinglists to do this) and I don't regret a single
> rewrite. Last time was for the PL18x DMAengine driver actually.
I've done it myself often enough. Ok, will change.
>
> > +
> > +#define SDMA_H_CONFIG_DSPDMA (1 << 12) /* indicates if the DSPDMA is used */
> > +#define SDMA_H_CONFIG_RTD_PINS (1 << 11) /* indicates if Real-Time Debug pins are enabled */
> > +#define SDMA_H_CONFIG_ACR (1 << 4) /* indicates if AHB freq /core freq = 2 or 1 */
> > +#define SDMA_H_CONFIG_CSM (3) /* indicates which context switch mode is selected*/
> > +
> > +static int sdma_config_ownership(int channel, int event_override,
> > + int mcu_verride, int dsp_override)
> > +{
> > + u32 evt, mcu, dsp;
> > +
> > + if (event_override && mcu_verride && dsp_override)
> > + return -EINVAL;
> > +
> > + evt = readl(SDMA_H_EVTOVR);
> > + mcu = readl(SDMA_H_HOSTOVR);
> > + dsp = readl(SDMA_H_DSPOVR);
> > +
> > + if (dsp_override)
> > + dsp &= ~(1 << channel);
> > + else
> > + dsp |= (1 << channel);
> > +
> > + if (event_override)
> > + evt &= ~(1 << channel);
> > + else
> > + evt |= (1 << channel);
> > +
> > + if (mcu_verride)
> > + mcu &= ~(1 << channel);
> > + else
> > + mcu |= (1 << channel);
> > +
> > + writel(evt, SDMA_H_EVTOVR);
> > + writel(mcu, SDMA_H_HOSTOVR);
> > + writel(dsp, SDMA_H_DSPOVR);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * sdma_run_channel - run a channel and wait till it's done
> > + */
> > +static int sdma_run_channel(int channel)
> > +{
> > + struct sdma_channel *sdma = &sdma_data[channel];
> > + int ret;
> > +
> > + writel(1 << channel, SDMA_H_START);
> > +
> > + ret = wait_event_interruptible(sdma->waitq,
> > + !(readl(SDMA_H_STATSTOP) & (1 << channel)));
>
> OK not the biggest thing in the world, but can't you use a
> completion for this? (I'm not so clever with waitqueues so
> forgive me if this is malinformed.)
Ok.
>
> > + return ret;
> > +}
> > +
> > +static int sdma_load_script(void *buf, int size, u32 address)
> > +{
> > + struct sdma_buffer_descriptor *bd0 = sdma_data[0].bd;
> > + void *buf_virt;
> > + dma_addr_t buf_phys;
> > + int ret;
> > +
> > + buf_virt = dma_alloc_coherent(NULL,
> > + size,
> > + &buf_phys, GFP_KERNEL);
> > + if (!buf_virt)
> > + return -ENOMEM;
> > +
> > + bd0->mode.command = C0_SETPM;
> > + bd0->mode.status = BD_DONE | BD_INTR | BD_WRAP | BD_EXTD;
> > + bd0->mode.count = size / 2;
> > + bd0->buffer_addr = buf_phys;
> > + bd0->ext_buffer_addr = address;
> > +
> > + memcpy(buf_virt, buf, size);
> > +
> > + ret = sdma_run_channel(0);
> > +
> > + dma_free_coherent(NULL, size, buf_virt, buf_phys);
> > +
> > + return ret;
> > +}
> > +
> > +static void sdma_event_enable(int channel, int event)
> > +{
> > + u32 val;
> > +
> > + val = readl(SDMA_CHNENBL_0 + event * 4);
>
> This use indicates that event should probably be
> unsigned, and probably not greater than u16 atleast.
> I suspect it is never more than an u8 really.
>
> > + val |= (1 << channel);
> > + writel(val, SDMA_CHNENBL_0 + event * 4);
> > +}
> > +
> > +static void sdma_event_disable(int channel, int event)
> > +{
> > + u32 val;
> > +
> > + val = readl(SDMA_CHNENBL_0 + event * 4);
> > + val &= ~(1 << channel);
> > + writel(val, SDMA_CHNENBL_0 + event * 4);
>
> Same comment here.
Ok, changed to unsigned and added a check for valid values in
sdma_config_channel.
>
> > +}
> > +
> > +static void mxc_sdma_handle_channel_loop(int channel)
> > +{
> > + struct sdma_channel *sdma = &sdma_data[channel];
>
> This indicates that channel should be unsigned.
>
> > + struct sdma_buffer_descriptor *bd;
> > + int error = 0;
>
> Unused variable?
Originally the callback function had a status parameter where errors
were signalled. I assume device_tx_status is the function to which this
error should be passed, right?
>
> > +
> > + /*
> > + * loop mode. Iterate over descriptors, re-setup them and
> > + * call callback function.
> > + */
> > + while (1) {
> > + bd = &sdma->bd[sdma->buf_tail];
> > +
> > + if (bd->mode.status & BD_DONE)
> > + break;
> > +
> > + if (bd->mode.status & BD_RROR)
> > + error = -EIO;
> > +
> > + bd->mode.status |= BD_DONE;
> > + sdma->buf_tail++;
> > + sdma->buf_tail %= sdma->num_bd;
> > +
> > + if (sdma->desc.callback)
> > + sdma->desc.callback(sdma->desc.callback_param);
> > + }
> > +}
> > +
> > +static void mxc_sdma_handle_channel_normal(int channel)
> > +{
> > + struct sdma_channel *sdma = &sdma_data[channel];
> > + struct sdma_buffer_descriptor *bd;
> > + int i, error = 0;
> > +
> > + /*
> > + * non loop mode. Iterate over all descriptors, collect
> > + * errors and call callback function
> > + */
> > + for (i = 0; i < sdma->num_bd; i++) {
> > + bd = &sdma->bd[i];
> > +
> > + if (bd->mode.status & (BD_DONE | BD_RROR))
> > + error = -EIO;
> > + }
> > +
> > + if (sdma->desc.callback)
> > + sdma->desc.callback(sdma->desc.callback_param);
> > + sdma->last_completed = sdma->desc.cookie;
> > +
> > + sdma->busy = 0;
>
> = true if you switch this to bool..
>
> > +}
> > +
> > +static void mxc_sdma_handle_channel(int channel)
> > +{
> > + struct sdma_channel *sdma = &sdma_data[channel];
> > +
> > + wake_up_interruptible(&sdma->waitq);
> > +
> > + /* not interested in channel 0 interrupts */
> > + if (!channel)
> > + return;
> > +
> > + if (sdma->flags & IMX_DMA_SG_LOOP)
> > + mxc_sdma_handle_channel_loop(channel);
> > + else
> > + mxc_sdma_handle_channel_normal(channel);
> > +}
> > +
> > +static irqreturn_t sdma_int_handler(int irq, void *dev_id)
> > +{
> > + u32 stat;
> > +
> > + stat = readl(SDMA_H_INTR);
> > + writel(stat, SDMA_H_INTR);
> > +
> > + while (stat) {
> > + int channel = fls(stat) - 1;
> > +
> > + mxc_sdma_handle_channel(channel);
> > +
> > + stat &= ~(1 << channel);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static struct clk *sdma_clk;
> > +
> > +/*
> > + * 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;
> > +
> > +/*
> > + * sets the pc of SDMA script according to the peripheral type
> > + */
> > +static void sdma_get_pc(struct sdma_channel *sdma,
> > + sdma_peripheral_type peripheral_type)
> > +{
> > + int res = 0;
> > + int per_2_emi = 0, emi_2_per = 0;
> > + int per_2_int = 0, int_2_per = 0;
> > + int per_2_per = 0, emi_2_emi = 0;
> > +
> > + sdma->pc_from_device = 0;
> > + sdma->pc_to_device = 0;
>
> There are a *lot* of local variables here, and only two of them
> are used eventually, at the end of the function. I cannot quite
> follow this, what is going on?
'per' is for peripheral (like sdhc, ssi unit or similar)
'int' is for internal SRAM
'emi' is for SDRAM
currently we only support transfers from per to emi and emi to per and
so the other variables are unused. We could make this function simpler
by removing the unused variables, but I suggest to keep them for making
it easier to support other transfer types later. I can add a comment
what these variables are for and why they are unused.
>
> Some like emi_2_emi seem to be totally unused.
>
> The types here look like some kind of enum or other
> similar construction is really what's being asked for
> here.
>
> > +
> > + switch (peripheral_type) {
> > + case IMX_DMATYPE_MEMORY:
> > + emi_2_emi = sdma_script_addrs->ap_2_ap_addr;
> > + break;
> > + case IMX_DMATYPE_DSP:
> > + emi_2_per = sdma_script_addrs->bp_2_ap_addr;
> > + per_2_emi = sdma_script_addrs->ap_2_bp_addr;
> > + break;
> > + case IMX_DMATYPE_FIRI:
> > + per_2_int = sdma_script_addrs->firi_2_per_addr;
> > + per_2_emi = sdma_script_addrs->firi_2_mcu_addr;
> > + int_2_per = sdma_script_addrs->per_2_firi_addr;
> > + emi_2_per = sdma_script_addrs->mcu_2_firi_addr;
> > + break;
> > + case IMX_DMATYPE_UART:
> > + per_2_int = sdma_script_addrs->uart_2_per_addr;
> > + per_2_emi = sdma_script_addrs->uart_2_mcu_addr;
> > + int_2_per = sdma_script_addrs->per_2_app_addr;
> > + emi_2_per = sdma_script_addrs->mcu_2_app_addr;
> > + break;
> > + case IMX_DMATYPE_UART_SP:
> > + per_2_int = sdma_script_addrs->uartsh_2_per_addr;
> > + per_2_emi = sdma_script_addrs->uartsh_2_mcu_addr;
> > + int_2_per = sdma_script_addrs->per_2_shp_addr;
> > + emi_2_per = sdma_script_addrs->mcu_2_shp_addr;
> > + break;
> > + case IMX_DMATYPE_ATA:
> > + per_2_emi = sdma_script_addrs->ata_2_mcu_addr;
> > + emi_2_per = sdma_script_addrs->mcu_2_ata_addr;
> > + break;
> > + case IMX_DMATYPE_CSPI:
> > + case IMX_DMATYPE_EXT:
> > + case IMX_DMATYPE_SSI:
> > + per_2_int = sdma_script_addrs->app_2_per_addr;
> > + per_2_emi = sdma_script_addrs->app_2_mcu_addr;
> > + int_2_per = sdma_script_addrs->per_2_app_addr;
> > + emi_2_per = sdma_script_addrs->mcu_2_app_addr;
> > + break;
> > + case IMX_DMATYPE_SSI_SP:
> > + case IMX_DMATYPE_MMC:
> > + case IMX_DMATYPE_SDHC:
> > + case IMX_DMATYPE_CSPI_SP:
> > + case IMX_DMATYPE_ESAI:
> > + case IMX_DMATYPE_MSHC_SP:
> > + per_2_int = sdma_script_addrs->shp_2_per_addr;
> > + per_2_emi = sdma_script_addrs->shp_2_mcu_addr;
> > + int_2_per = sdma_script_addrs->per_2_shp_addr;
> > + emi_2_per = sdma_script_addrs->mcu_2_shp_addr;
> > + break;
> > + case IMX_DMATYPE_ASRC:
> > + per_2_emi = sdma_script_addrs->asrc_2_mcu_addr;
> > + emi_2_per = sdma_script_addrs->asrc_2_mcu_addr;
> > + per_2_per = sdma_script_addrs->per_2_per_addr;
> > + break;
> > + case IMX_DMATYPE_MSHC:
> > + per_2_emi = sdma_script_addrs->mshc_2_mcu_addr;
> > + emi_2_per = sdma_script_addrs->mcu_2_mshc_addr;
> > + break;
> > + case IMX_DMATYPE_CCM:
> > + per_2_emi = sdma_script_addrs->dptc_dvfs_addr;
> > + break;
> > + case IMX_DMATYPE_FIFO_MEMORY:
> > + res = sdma_script_addrs->ap_2_ap_fixed_addr;
>
> res? This thing is never used.
I have no idea what DMATYPE_FIFO_MEMORY is. Will remove this.
>
> > + break;
> > + case IMX_DMATYPE_SPDIF:
> > + per_2_emi = sdma_script_addrs->spdif_2_mcu_addr;
> > + emi_2_per = sdma_script_addrs->mcu_2_spdif_addr;
> > + break;
> > + case IMX_DMATYPE_IPU_MEMORY:
> > + emi_2_per = sdma_script_addrs->ext_mem_2_ipu_addr;
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + sdma->pc_from_device = per_2_emi;
> > + sdma->pc_to_device = emi_2_per;
>
> Return res? You're assigning it a value in some cases.
>
> > +}
> > +
> > +static int sdma_load_context(int channel)
> > +{
> > + struct sdma_channel *sdma = &sdma_data[channel];
> > + int load_address;
> > + struct sdma_buffer_descriptor *bd0 = sdma_data[0].bd;
> > + int ret;
> > +
> > + if (sdma->direction == DMA_FROM_DEVICE) {
> > + load_address = sdma->pc_from_device;
> > + } else {
> > + load_address = sdma->pc_to_device;
> > + }
> > +
> > + if (load_address < 0)
> > + return load_address;
> > +
> > + pr_debug("%s: load_address = %d\n", __func__, load_address);
> > + pr_debug("%s: wml = 0x%08x\n", __func__, sdma->watermark_level);
> > + pr_debug("%s: shp_addr = 0x%08x\n", __func__, sdma->shp_addr);
> > + pr_debug("%s: per_addr = 0x%08x\n", __func__, sdma->per_addr);
> > + pr_debug("%s: event_mask1 = 0x%08x\n", __func__, sdma->event_mask1);
> > + pr_debug("%s: event_mask2 = 0x%08x\n", __func__, sdma->event_mask2);
>
> Surely it must be possible to get the struct device * pointer for the
> channels host and use dev_dbg() instead?
Ok, will do
>
> > +
> > + memset(sdma_context, 0, sizeof(*sdma_context));
> > + sdma_context->channel_state.pc = load_address;
> > +
> > + /* Send by context the event mask,base address for peripheral
> > + * and watermark level
> > + */
> > + sdma_context->gReg[0] = sdma->event_mask2;
> > + sdma_context->gReg[1] = sdma->event_mask1;
> > + sdma_context->gReg[2] = sdma->per_addr;
> > + sdma_context->gReg[6] = sdma->shp_addr;
> > + sdma_context->gReg[7] = sdma->watermark_level;
> > +
> > + bd0->mode.command = C0_SETDM;
> > + bd0->mode.status = BD_DONE | BD_INTR | BD_WRAP | BD_EXTD;
> > + bd0->mode.count = sizeof(*sdma_context) / 4;
> > + bd0->buffer_addr = sdma_context_phys;
> > + bd0->ext_buffer_addr = 2048 + (sizeof(*sdma_context) / 4) * channel;
> > +
> > + ret = sdma_run_channel(0);
> > +
> > + return ret;
> > +}
> > +
> > +static void sdma_disable_channel(int channel)
> > +{
> > + struct sdma_channel *sdma = &sdma_data[channel];
> > +
> > + writel(1 << channel, SDMA_H_STATSTOP);
> > + sdma->busy = 0;
> > +}
> > +
> > +static int sdma_config_channel(int channel)
> > +{
> > + struct sdma_channel *sdma = &sdma_data[channel];
> > + int ret;
> > +
> > + sdma_disable_channel(channel);
> > +
> > + sdma->event_mask1 = 0;
> > + sdma->event_mask2 = 0;
> > + sdma->shp_addr = 0;
> > + sdma->per_addr = 0;
> > +
> > + if (sdma->event_id)
> > + sdma_event_enable(channel, sdma->event_id);
> > +
> > + switch (sdma->peripheral_type) {
> > + case IMX_DMATYPE_DSP:
> > + sdma_config_ownership(channel, 0, 1, 1);
>
> The parameters here makes yoy believe that the types should
> be bool rather than int...
ok.
>
> > + break;
> > + case IMX_DMATYPE_MEMORY:
> > + sdma_config_ownership(channel, 0, 1, 0);
> > + break;
> > + default:
> > + sdma_config_ownership(channel, 1, 1, 0);
> > + break;
> > + }
> > +
> > + sdma_get_pc(sdma, sdma->peripheral_type);
> > +
> > + if ((sdma->peripheral_type != IMX_DMATYPE_MEMORY) &&
> > + (sdma->peripheral_type != IMX_DMATYPE_DSP)) {
> > + /* Handle multiple event channels differently */
> > + if (sdma->event_id2) {
> > + sdma->event_mask2 = 1 << (sdma->event_id2 % 32);
> > + if (sdma->event_id2 > 31)
> > + sdma->watermark_level |= 1 << 31;
> > + sdma->event_mask1 = 1 << (sdma->event_id % 32);
> > + if (sdma->event_id > 31)
> > + sdma->watermark_level |= 1 << 30;
> > + } else {
> > + sdma->event_mask1 = 1 << sdma->event_id;
> > + sdma->event_mask2 = 1 << (sdma->event_id - 32);
> > + }
> > + /* Watermark Level */
> > + sdma->watermark_level |= sdma->watermark_level;
> > + /* Address */
> > + sdma->shp_addr = sdma->per_address;
> > + } else {
> > + sdma->watermark_level = 0; /* FIXME: M3_BASE_ADDRESS */
> > + }
> > +
> > + ret = sdma_load_context(channel);
> > +
> > + return ret;
> > +}
> > +
> > +static int sdma_set_channel_priority(unsigned int channel, unsigned int priority)
> > +{
> > + if (priority < MXC_SDMA_MIN_PRIORITY
> > + || priority > MXC_SDMA_MAX_PRIORITY) {
> > + return -EINVAL;
> > + }
> > +
> > + writel(priority, SDMA_CHNPRI_0 + 4 * channel);
> > +
> > + return 0;
> > +}
> > +
> > +static int sdma_request_channel(int channel)
> > +{
> > + struct sdma_channel *sdma = &sdma_data[channel];
> > + int ret = -EBUSY;
> > +
> > + sdma->bd = dma_alloc_coherent(NULL, PAGE_SIZE, &sdma->bd_phys, GFP_KERNEL);
> > + if (!sdma->bd) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + memset(sdma->bd, 0, PAGE_SIZE);
> > +
> > + channel_control[channel].base_bd_ptr = sdma->bd_phys;
> > + channel_control[channel].current_bd_ptr = sdma->bd_phys;
> > +
> > + clk_enable(sdma_clk);
>
> Aha you're enabling it once for every channel and rely on
> clk reference counting that's clever!
>
> > +
> > + sdma_set_channel_priority(channel, MXC_SDMA_DEFAULT_PRIORITY);
> > +
> > + init_waitqueue_head(&sdma->waitq);
> > +
> > + sdma->buf_tail = 0;
> > +
> > + return 0;
> > +out:
> > +
> > + return ret;
> > +}
> > +
> > +static void sdma_enable_channel(int channel)
> > +{
> > + writel(1 << channel, SDMA_H_START);
> > +}
> > +
> > +static int __init sdma_init(unsigned long phys_base, int irq, int version,
> > + void *ram_code,
> > + int ram_code_size)
> > +{
> > + int i, ret;
> > + int channel;
> > + dma_addr_t ccb_phys;
> > +
> > + sdma_version = version;
> > + switch (sdma_version) {
> > + case 1:
> > + sdma_num_events = 32;
> > + break;
> > + case 2:
> > + sdma_num_events = 48;
> > + break;
> > + default:
> > + pr_err("SDMA: Unknown version %d. aborting\n", sdma_version);
> > + return -ENODEV;
> > + }
> > +
> > + clk_enable(sdma_clk);
> > +
> > + sdma_base = ioremap(phys_base, 4096);
>
> Use SZ_4K instead of 4096.
Or even better resouce_size(iores)
>
> > + if (!sdma_base) {
> > + ret = -ENOMEM;
> > + goto err_ioremap;
> > + }
> > +
> > + /* Initialize SDMA private data */
> > + memset(sdma_data, 0, sizeof(struct sdma_channel) * MAX_DMA_CHANNELS);
> > +
> > + for (channel = 0; channel < MAX_DMA_CHANNELS; channel++)
> > + sdma_data[channel].channel = channel;
> > +
> > + ret = request_irq(irq, sdma_int_handler, 0, "sdma", NULL);
> > + if (ret)
> > + goto err_request_irq;
> > +
> > + /* Be sure SDMA has not started yet */
> > + writel(0, SDMA_H_C0PTR);
> > +
> > + channel_control = dma_alloc_coherent(NULL,
> > + MAX_DMA_CHANNELS * sizeof (struct sdma_channel_control) +
> > + sizeof(struct sdma_context_data),
> > + &ccb_phys, GFP_KERNEL);
> > +
> > + if (!channel_control) {
> > + ret = -ENOMEM;
> > + goto err_dma_alloc;
> > + }
> > +
> > + sdma_context = (void *)channel_control +
> > + MAX_DMA_CHANNELS * sizeof (struct sdma_channel_control);
> > + sdma_context_phys = ccb_phys +
> > + MAX_DMA_CHANNELS * sizeof (struct sdma_channel_control);
> > +
> > + /* Zero-out the CCB structures array just allocated */
> > + memset(channel_control, 0,
> > + MAX_DMA_CHANNELS * sizeof (struct sdma_channel_control));
> > +
> > + /* disable all channels */
> > + for (i = 0; i < sdma_num_events; i++)
> > + writel(0, SDMA_CHNENBL_0 + i * 4);
> > +
> > + /* All channels have priority 0 */
> > + for (i = 0; i < MAX_DMA_CHANNELS; i++)
> > + writel(0, SDMA_CHNPRI_0 + i * 4);
> > +
> > + ret = sdma_request_channel(0);
> > + if (ret)
> > + goto err_dma_alloc;
> > +
> > + sdma_config_ownership(0, 0, 1, 0);
> > +
> > + /* Set Command Channel (Channel Zero) */
> > + writel(0x4050, SDMA_CHN0ADDR);
> > +
> > + /* Set bits of CONFIG register but with static context switching */
> > + /* FIXME: Check whether to set ACR bit depending on clock ratios */
> > + writel(0, SDMA_H_CONFIG);
> > +
> > + writel(ccb_phys, SDMA_H_C0PTR);
> > +
> > + /* download the RAM image for SDMA */
> > + sdma_load_script(ram_code,
> > + ram_code_size,
> > + sdma_script_addrs->ram_code_start_addr);
> > +
> > + /* Set bits of CONFIG register with given context switching mode */
> > + writel(SDMA_H_CONFIG_CSM, SDMA_H_CONFIG);
> > +
> > + /* Initializes channel's priorities */
> > + sdma_set_channel_priority(0, 7);
> > +
> > + clk_disable(sdma_clk);
> > +
> > + return 0;
> > +
> > +err_dma_alloc:
> > + free_irq(irq, NULL);
> > +err_request_irq:
> > + iounmap(sdma_base);
> > +err_ioremap:
> > + clk_disable(sdma_clk);
> > + pr_err("%s failed with %d\n", __func__, ret);
> > + return ret;
> > +}
> > +
> > +static dma_cookie_t sdma_assign_cookie(struct sdma_channel *sdma)
> > +{
> > + dma_cookie_t cookie = sdma->chan.cookie;
> > +
> > + if (++cookie < 0)
> > + cookie = 1;
> > +
> > + sdma->chan.cookie = cookie;
> > + sdma->desc.cookie = cookie;
> > +
> > + return cookie;
> > +}
> > +
> > +static struct sdma_channel *to_sdma_chan(struct dma_chan *chan)
> > +{
> > + return container_of(chan, struct sdma_channel, chan);
> > +}
> > +
> > +static dma_cookie_t sdma_tx_submit(struct dma_async_tx_descriptor *tx)
> > +{
> > + struct sdma_channel *sdma = to_sdma_chan(tx->chan);
> > + dma_cookie_t cookie;
> > +
> > + spin_lock_irq(&sdma->lock);
> > +
> > + cookie = sdma_assign_cookie(sdma);
> > +
> > + sdma_enable_channel(tx->chan->chan_id);
> > +
> > + spin_unlock_irq(&sdma->lock);
> > +
> > + return cookie;
> > +}
> > +
> > +static int sdma_alloc_chan_resources(struct dma_chan *chan)
> > +{
> > + struct sdma_channel *sdma = to_sdma_chan(chan);
> > + struct imx_dma_data *data = chan->private;
> > + int prio, ret;
> > +
> > + /* No need to execute this for internal channel 0 */
> > + if (!chan->chan_id)
> > + return 0;
> > +
> > + if (!data)
> > + return -EINVAL;
> > +
> > + switch (data->priority) {
> > + case DMA_PRIO_HIGH:
> > + prio = 3;
>
> Wait, aren't these enumerated?
> Add some enum sdma_channel_prio {}..
Hm, The SDMA engine has priorities from 1 to 7 from which we happen to use
the lowest priorities only. I think this should not be an enum.
(The DMA_PRIO_* are only used in an attempt to provide the same API for
the i.MX2 SoCs which have a different DMA engine which is not so
flexible)
>
>
> > + break;
> > + case DMA_PRIO_MEDIUM:
> > + prio = 2;
> > + break;
> > + case DMA_PRIO_LOW:
> > + default:
> > + prio = 1;
> > + break;
> > + }
> > +
> > + sdma->peripheral_type = data->peripheral_type;
> > + sdma->event_id = data->dma_request;
> > + ret = sdma_set_channel_priority(chan->chan_id, prio);
> > + if (ret)
> > + return ret;
> > +
> > + if (chan->chan_id) {
> > + ret = sdma_request_channel(chan->chan_id);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + dma_async_tx_descriptor_init(&sdma->desc, chan);
> > + sdma->desc.tx_submit = sdma_tx_submit;
> > + /* txd.flags will be overwritten in prep funcs */
> > + sdma->desc.flags = DMA_CTRL_ACK;
> > +
> > + return 0;
> > +}
> > +
> > +static void sdma_free_chan_resources(struct dma_chan *chan)
> > +{
> > + struct sdma_channel *sdma = to_sdma_chan(chan);
> > + int channel = chan->chan_id;
> > +
> > + sdma_disable_channel(channel);
> > +
> > + if (sdma->event_id)
> > + sdma_event_disable(channel, sdma->event_id);
> > + if (sdma->event_id2)
> > + sdma_event_disable(channel, sdma->event_id2);
> > +
> > + sdma->event_id = 0;
> > + sdma->event_id2 = 0;
> > +
> > + sdma_set_channel_priority(channel, 0);
> > +
> > + dma_free_coherent(NULL, PAGE_SIZE, sdma->bd, sdma->bd_phys);
> > +
> > + clk_disable(sdma_clk);
> > +}
> > +
> > +#define NUM_BD (int)(PAGE_SIZE / sizeof(struct sdma_buffer_descriptor))
> > +
> > +static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
> > + struct dma_chan *chan, struct scatterlist *sgl,
> > + unsigned int sg_len, enum dma_data_direction direction,
> > + unsigned long flags)
> > +{
> > + struct sdma_channel *sdma = to_sdma_chan(chan);
> > + int ret, i, count;
> > + int channel = chan->chan_id;
> > + struct scatterlist *sg;
> > +
> > + if (sdma->busy)
> > + return NULL;
> > + sdma->busy = 1;
> > +
> > + sdma->flags = 0;
>
> What are those flags anyway? I think you will need some
> #define:s for them.
There's only one currently: IMX_DMA_SG_LOOP indicating that we are
doing cyclic transfers.
>
> > +
> > + pr_debug("SDMA: setting up %d entries for channel %d.\n",
> > + sg_len, channel);
> > +
> > + sdma->direction = direction;
> > + ret = sdma_load_context(channel);
> > + if (ret)
> > + goto err_out;
> > +
> > + if (sg_len > NUM_BD) {
> > + pr_err("SDMA channel %d: maximum number of sg exceeded: %d > %d\n",
> > + channel, sg_len, NUM_BD);
> > + ret = -EINVAL;
> > + goto err_out;
> > + }
> > +
> > + for_each_sg(sgl, sg, sg_len, i) {
> > + struct sdma_buffer_descriptor *bd = &sdma->bd[i];
> > + int param;
> > +
> > + bd->buffer_addr = sgl->dma_address;
> > +
> > + count = sg->length;
> > +
> > + if (count > 0xffff) {
> > + pr_err("SDMA channel %d: maximum bytes for sg entry exceeded: %d > %d\n",
> > + channel, count, 0xffff);
> > + ret = -EINVAL;
> > + goto err_out;
> > + }
> > +
> > + bd->mode.count = count;
> > +
> > + if (sdma->word_size > 4) {
> > + ret = -EINVAL;
> > + goto err_out;
> > + }
> > + if (sdma->word_size == 4)
> > + bd->mode.command = 0;
> > + else
> > + bd->mode.command = sdma->word_size;
> > +
> > + param = BD_DONE | BD_EXTD | BD_CONT;
> > +
> > + if (sdma->flags & IMX_DMA_SG_LOOP) {
> > + param |= BD_INTR;
> > + if (i + 1 == sg_len)
> > + param |= BD_WRAP;
> > + }
> > +
> > + if (i + 1 == sg_len)
> > + param |= BD_INTR;
> > +
> > + pr_debug("entry %d: count: %d dma: 0x%08x %s%s\n",
> > + i, count, sg->dma_address,
> > + param & BD_WRAP ? "wrap" : "",
> > + param & BD_INTR ? " intr" : "");
> > +
> > + bd->mode.status = param;
> > + }
> > +
> > + sdma->num_bd = sg_len;
> > + channel_control[channel].current_bd_ptr = sdma->bd_phys;
> > +
> > + return &sdma->desc;
> > +err_out:
> > + return NULL;
> > +}
> > +
> > +static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
> > + struct dma_chan *chan, dma_addr_t dma_addr, size_t buf_len,
> > + size_t period_len, enum dma_data_direction direction)
> > +{
> > + int num_periods = buf_len / period_len;
> > + struct sdma_channel *sdma = to_sdma_chan(chan);
> > + int channel = chan->chan_id;
> > + int ret, i = 0, buf = 0;
> > +
> > + pr_debug("%s channel: %d\n", __func__, channel);
>
> Must be possible to find struct device * and use dev_dbg()
>
> > +
> > + if (sdma->busy)
> > + return NULL;
> > +
> > + sdma->busy = 1;
> > +
> > + sdma->flags |= IMX_DMA_SG_LOOP;
> > + sdma->direction = direction;
> > + ret = sdma_load_context(channel);
> > + if (ret)
> > + goto err_out;
> > +
> > + if (num_periods > NUM_BD) {
> > + pr_err("SDMA channel %d: maximum number of sg exceeded: %d > %d\n",
> > + channel, num_periods, NUM_BD);
> > + goto err_out;
> > + }
> > +
> > + if (period_len > 0xffff) {
> > + pr_err("SDMA channel %d: maximum period size exceeded: %d > %d\n",
> > + channel, period_len, 0xffff);
> > + goto err_out;
> > + }
> > +
> > + while (buf < buf_len) {
> > + struct sdma_buffer_descriptor *bd = &sdma->bd[i];
> > + int param;
> > +
> > + bd->buffer_addr = dma_addr;
> > +
> > + bd->mode.count = period_len;
> > +
> > + if (sdma->word_size > 4)
> > + goto err_out;
> > + if (sdma->word_size == 4)
> > + bd->mode.command = 0;
> > + else
> > + bd->mode.command = sdma->word_size;
> > +
> > + param = BD_DONE | BD_EXTD | BD_CONT | BD_INTR;
> > + if (i + 1 == num_periods)
> > + param |= BD_WRAP;
> > +
> > + pr_debug("entry %d: count: %d dma: 0x%08x %s%s\n",
> > + i, period_len, dma_addr,
> > + param & BD_WRAP ? "wrap" : "",
> > + param & BD_INTR ? " intr" : "");
> > +
> > + bd->mode.status = param;
> > +
> > + dma_addr += period_len;
> > + buf += period_len;
> > +
> > + i++;
> > + }
> > +
> > + sdma->num_bd = num_periods;
> > + channel_control[channel].current_bd_ptr = sdma->bd_phys;
> > +
> > + return &sdma->desc;
> > +err_out:
> > + sdma->busy = 0;
> > + return NULL;
> > +}
> > +
> > +static int sdma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> > + unsigned long arg)
> > +{
> > + struct sdma_channel *sdma = to_sdma_chan(chan);
> > + struct dma_slave_config *dmaengine_cfg = (void *)arg;
> > +
> > + switch (cmd) {
> > + case DMA_TERMINATE_ALL:
> > + sdma_disable_channel(chan->chan_id);
> > + return 0;
> > + case DMA_SLAVE_CONFIG:
> > + if (dmaengine_cfg->direction == DMA_FROM_DEVICE) {
> > + sdma->per_address = dmaengine_cfg->src_addr;
> > + sdma->watermark_level = dmaengine_cfg->src_maxburst;
> > + sdma->word_size = dmaengine_cfg->src_addr_width;
> > + } else {
> > + sdma->per_address = dmaengine_cfg->dst_addr;
> > + sdma->watermark_level = dmaengine_cfg->dst_maxburst;
> > + sdma->word_size = dmaengine_cfg->dst_addr_width;
> > + }
> > + return sdma_config_channel(chan->chan_id);
> > + default:
> > + return -ENOSYS;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static enum dma_status sdma_tx_status(struct dma_chan *chan,
> > + dma_cookie_t cookie,
> > + struct dma_tx_state *txstate)
> > +{
> > + struct sdma_channel *sdma = to_sdma_chan(chan);
> > + dma_cookie_t last_used;
> > + enum dma_status ret;
> > +
> > + last_used = chan->cookie;
> > +
> > + ret = dma_async_is_complete(cookie, sdma->last_completed, last_used);
> > + dma_set_tx_state(txstate, sdma->last_completed, last_used, 0);
> > +
> > + return ret;
> > +}
> > +
> > +static void sdma_issue_pending(struct dma_chan *chan)
> > +{
> > + /*
> > + * Nothing to do. We only have a single descriptor
> > + */
> > +}
> > +
> > +static int __devinit sdma_probe(struct platform_device *pdev)
> > +{
> > + int ret;
> > + const struct firmware *fw;
> > + const struct sdma_firmware_header *header;
> > + const struct sdma_script_start_addrs *addr;
> > + int irq;
> > + unsigned short *ram_code;
> > + struct resource *iores;
> > + struct sdma_platform_data *pdata = pdev->dev.platform_data;
> > + int version;
> > + char *cpustr, *fwname;
> > + int i;
> > + dma_cap_mask_t mask;
> > +
> > + /* there can be only one */
> > + BUG_ON(sdma_base);
> > +
> > + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + irq = platform_get_irq(pdev, 0);
> > + if (!iores || irq < 0 || !pdata)
> > + return -EINVAL;
> > +
> > + sdma_clk = clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(sdma_clk)) {
> > + ret = PTR_ERR(sdma_clk);
> > + goto err_clk;
> > + }
> > +
> > + if (cpu_is_mx31()) {
> > + cpustr = "imx31";
> > + version = mx31_revision() >> 4;
> > + } else if (cpu_is_mx35()) {
> > + cpustr = "imx35";
> > +/* FIXME: version = mx35_revision(); */
> > + version = 2;
> > + } else {
> > + ret = -EINVAL;
> > + goto err_cputype;
> > + }
> > +
> > + fwname = kasprintf(GFP_KERNEL, "sdma-%s-to%d.bin", cpustr, version);
> > + if (!fwname) {
> > + ret = -ENOMEM;
> > + goto err_cputype;
> > + }
> > +
> > + ret = request_firmware(&fw, fwname, &pdev->dev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "request firmware \"%s\" failed with %d\n",
> > + fwname, ret);
> > + kfree(fwname);
> > + goto err_cputype;
> > + }
> > + kfree(fwname);
> > +
> > + if (fw->size < sizeof(*header))
> > + goto err_firmware;
> > +
> > + header = (struct sdma_firmware_header *)fw->data;
> > +
> > + if (header->magic != SDMA_FIRMWARE_MAGIC)
> > + goto err_firmware;
> > + if (header->ram_code_start + header->ram_code_size > fw->size)
> > + goto err_firmware;
> > +
> > + addr = (void *)header + header->script_addrs_start;
> > + ram_code = (void *)header + header->ram_code_start;
> > + memcpy(&__sdma_script_addrs, addr, sizeof(*addr));
> > +
> > + ret = sdma_init(iores->start, irq, pdata->sdma_version,
> > + ram_code, header->ram_code_size);
> > + if (ret)
> > + goto err_firmware;
> > +
> > + INIT_LIST_HEAD(&sdma_dma_device->channels);
> > +
> > + /* Initialize channel parameters */
> > + for (i = 0; i < MAX_DMA_CHANNELS; i++) {
> > + struct sdma_channel *sdma = &sdma_data[i];
> > +
> > + spin_lock_init(&sdma->lock);
> > +
> > + dma_cap_set(DMA_SLAVE, sdma_dma_device->cap_mask);
> > + dma_cap_set(DMA_CYCLIC, sdma_dma_device->cap_mask);
> > +
> > + sdma->chan.device = sdma_dma_device;
> > + sdma->chan.chan_id = i;
> > +
> > + /* Add the channel to the DMAC list */
> > + list_add_tail(&sdma->chan.device_node, &sdma_dma_device->channels);
> > + }
> > +
> > + sdma_dma_device->dev = &pdev->dev;
> > +
> > + sdma_dma_device->device_alloc_chan_resources = sdma_alloc_chan_resources;
> > + sdma_dma_device->device_free_chan_resources = sdma_free_chan_resources;
> > + sdma_dma_device->device_tx_status = sdma_tx_status;
> > + sdma_dma_device->device_prep_slave_sg = sdma_prep_slave_sg;
> > + sdma_dma_device->device_prep_dma_cyclic = sdma_prep_dma_cyclic;
> > + sdma_dma_device->device_control = sdma_control;
> > + sdma_dma_device->device_issue_pending = sdma_issue_pending;
> > +
> > + ret = dma_async_device_register(sdma_dma_device);
> > + if (ret) {
> > + dev_err(&pdev->dev, "unable to register DMAC\n");
>
> SDMAC even?
Better just "unable to register". The name of the device will give
enough information.
>
> > + goto err_firmware;
> > + }
> > +
> > + dev_info(&pdev->dev, "initialized (firmware %d.%d)\n",
> > + header->version_major,
> > + header->version_minor);
> > +
> > + /* request channel 0. This is an internal control channel
> > + * to the SDMA engine and not available to clients.
> > + */
> > + dma_cap_zero(mask);
> > + dma_cap_set(DMA_SLAVE, mask);
> > + dma_request_channel(mask, NULL, NULL);
> > +
> > + release_firmware(fw);
> > +
> > + return 0;
> > +
> > +err_firmware:
> > + release_firmware(fw);
> > +err_cputype:
> > + clk_put(sdma_clk);
> > +err_clk:
> > + return 0;
> > +}
> > +
> > +static int __devexit sdma_remove(struct platform_device *pdev)
> > +{
> > + return -EBUSY;
> > +}
> > +
> > +static struct platform_driver sdma_driver = {
> > + .driver = {
> > + .name = "imx-sdma",
> > + },
> > + .probe = sdma_probe,
> > + .remove = __devexit_p(sdma_remove),
> > +};
> > +
> > +static int __init sdma_module_init(void)
> > +{
> > + return platform_driver_register(&sdma_driver);
> > +}
> > +subsys_initcall(sdma_module_init);
> > +
> > +MODULE_AUTHOR("Sascha Hauer, Pengutronix <s.hauer@...gutronix.de>");
> > +MODULE_DESCRIPTION("i.MX SDMA driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 1.7.1
>
> Thanks for using this API
> Sascha!
>
> Yours,
> Linus Walleij
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
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