[Cbe-oss-dev] [RFC 1/9] AXON - common driver core
Arnd Bergmann
arnd at arndb.de
Fri Dec 22 08:13:18 EST 2006
On Wednesday 20 December 2006 12:13, jdubois at mc.com wrote:
> @@ -0,0 +1,111 @@
> +/******************************************************************
> + * Copyright (C) 2006 Mercury Computer Systems, Inc.
> + * 199 Riverneck Road
> + * Chelmsford, MA 01824-2820
> + * (978) 256-1300
> + * webinfo at mc.com
It's common to list the name and address of the actual person
responsible for the code instead of a info at example.com style address.
Unless that would violate your company rules, please change this.
Generally, you should follow the rules from Documentation/CodingStyle
religiously when submitting kernel code. There's nothing wrong with
your style, we just like to keep everything looking the same in the
kernel. I'll list my comments about this marked with [style].
> +static struct class *axon_class = NULL;
[style] don't initialize static or global variables to NULL/0,
the compiler will do this automatically.
> +int
> +axon_create_class()
[style] write this as
int axon_create_class(void)
if possible, make the function static.
> + if (axon_class == NULL) {
> + axon_class = class_simple_create(THIS_MODULE, "axon");
> +
> + if (axon_class != NULL)
> + return 0;
> + else
> + return -1;
> + } else {
> + return 1;
> + }
> +
> +}
I prefer to keep the if() statements like 'if (axon_class)'
or 'if (!axon_class)' when testing for a NULL pointer. Benh
disagrees with me, so do as you like ;-)
Regarding return values, errors should always be reported as
negative errno.h numbers, not '-1'.
> +EXPORT_SYMBOL(axon_create_class);
> +EXPORT_SYMBOL(axon_destroy_class);
> +EXPORT_SYMBOL(axon_get_class);
The EXPORT_SYMBOL should follow the symbol definition, not
come as a separate block. When adding new exports, please
use EXPORT_SYMBOL_GPL.
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/vermagic.h>
> +#include <linux/version.h>
> +
> +#include <linux/device.h>
> +#include <linux/dmapool.h>
[style] by convention, order includes alphabetically.
> +#include <axon_utils.h>
> +#include <axon_io.h>
> +#include <axon_dbg.h>
> +#include <axon_defs.h>
> +#include <axon_bitops.h>
> +#include <axon_dma_ops_dma_aux.h>
[style] Your own headers should move to a more appropriate place.
Ideally, this is the local directory, with '#include "axon_foo.h"'
lines. If that doesn't work, put them into include/asm.
> +static int axon_use_aux_dma = 1;
> +
> +module_param(axon_use_aux_dma, int, 0644);
> +MODULE_PARM_DESC(axon_use_aux_dma,
> + "This allows to define if the driver should try to use the Aux DMA if present. By default it will try to discover and use it but you can request to skip it by setting this parameter to 0");
You should make this separate lines, like
MODULE_PARM_DESC(axon_use_aux_dma,
"This allows to define if the driver should try to use\n"
"the Aux DMA if present. By default it will try to\n"
"discover and use it but you can request to skip it\n"
"by setting this parameter to 0");
> +static int axon_aux_dma_is_broken = 1;
> +
> +module_param(axon_aux_dma_is_broken, int, 0644);
> +MODULE_PARM_DESC(axon_aux_dma_is_broken,
> + "On FCAB the AuxDMA is broken and can't cross 256 bytes bondaries without corrupting data. If you know your AuxDMA is not broken you can set this parameter to 0");
This should be detected from a device tree property of the dma
device.
> +static int axon_aux_dma_use_ben_bit = 1;
> +
> +module_param(axon_aux_dma_use_ben_bit, int, 0644);
> +MODULE_PARM_DESC(axon_aux_dma_use_ben_bit,
> + "Should we use the BEN bit on the DMA engine (required on FCAB)");
same here.
> +static void
> +link(struct axon_dma_ops_t *dma_ops,
> + struct axon_dma_pkt_t *p_dma_pkt_prev,
> + struct axon_dma_pkt_t *p_dma_pkt)
It would be better to name the function uniquely, like 'axon_dma_link',
even if it is static, to help interpret stack traces.
> + *pp_dma_ops = p_dma_ops;
> +
> + goto no_err;
> +
> +
> + err_dma_ops_dma_soft:
> + kfree(p_dma_ops);
> +
> + err_dma_ops:
> +
> +
> + no_err:
> +
> + return ret;
> +
> +}
[style] Labels should not be indented at all, they go to the first column.
> +static int
> +axon_dma_request_pkt_alloc_P(struct axon_dma_req_t *p_dma_req,
> + struct axon_dma_pkt_t **pp_dma_pkt,
> + int *p_index)
> +{
> + int ret = 0;
It would be more common to make this return the axon_dma_pkt
or NULL instead of 0 or error and pass the result in an argument.
It's not clear what 'P' means (pointer, packet, ... ?). Maybe
write the full word, and don't use capital letters.
> +static int
> +axon_dma_request_push_mbox_by_write(struct axon_dma_req_t *p_dma_req, struct axon_dma_req_mbox_t
> + *p_dma_req_mbox)
> +{
> + int ret = 0;
> +
> + struct axon_dma_pkt_t *p_dma_pkt;
> + struct axon_mbox_t *p_mbox = p_dma_req_mbox->p_dst_mbox;
> +
> + ret = axon_dma_request_pkt_dat_alloc(p_dma_req, &p_dma_pkt);
> +
> + if (ret >= 0) {
> +
> + void *p_mbox_msg = p_dma_pkt->addr;
> + plb_addr_t mbox_plb_bus_dst;
> + dma_addr_t mbox_cpu_bus_dst;
> + volatile void *mbox_cpu_dst;
Using 'volatile' in a variable declaration is practically always
a bug. Why do you think you need it here?
> +
> +struct axon_dma_req_t;
> +
> +struct axon_dmax_t {
> +
What does the _t stand for? By convention, this is often used
for typedefs, which this isn't, so it can probably better be
left out.
> + struct addr_xltr_t *p_addr_xltr;
> +
> +
> + struct axon_t *p_axon;
> +
> +
> + struct axon_mbox_t *p_axon_mbox_self;
> +
> +
> + struct axon_mbox_t *p_axon_mbox_peer;
> +
> +
> + struct axon_sms_t *p_axon_sms;
Same for the p_ prefix.
> + volatile u8 *p_dma_channel;
volatile seems wrong here.
> +static inline void
> +mbox_write_reg(struct axon_mbox_hw_t *p_mbox, u32 value, u32 dcr_addr)
> +{
> + __raw_writel(value, (p_mbox->p_regs) + dcr_addr);
> +
> +}
> +
> +static inline u32
> +mbox_read_reg(struct axon_mbox_hw_t *p_mbox, u32 dcr_addr)
> +{
> + return __raw_readl((p_mbox->p_regs) + dcr_addr);
> +
> +}
__raw_writel and __raw_readl are somewhat underspecified, so I'd
rather avoid them. out_be32 and in_be32 are probably what you
want.
> +static int
> +map_on_local_bus(struct axon_pio_pci_t *p_axon_pio_pci, plb_addr_t dst,
> + dma_addr_t * p_bus_addr, size_t size)
> +{
> + int ret = 0;
> +
> + dma_addr_t dst_bus_addr;
> +
> +
> + if (p_axon_pio_pci->p_pim != NULL) {
> +
> + axon_pim_map(p_axon_pio_pci->p_pim, dst, size);
> + }
> +
> +
> +
> + dst_bus_addr =
> + p_axon_pio_pci->p_addr_xltr->from_plb(p_axon_pio_pci->
> + p_addr_xltr, dst);
> +
> +
> + if (dst_bus_addr != AXON_INVALID_BUS_ADDR) {
> +
> + *p_bus_addr = dst_bus_addr;
> +
> + } else {
> +
> + ret = -EIO;
> + dbg_log("Unable to find an existing PCI address for 0x%016"
> + AXON_PLB_ADDR_FMT_T " \n", dst);
> + *p_bus_addr = 0x0;
> + }
> +
> + return ret;
> +
> +}
This is probably part of the comments you removed, but why do you need
to map anything physically? This should all be taken care of by
the firmware.
> +#define AXON_READ(size,type) \
> +static int \
> +axon_pio_pci_read##type(struct axon_pio_t* p_axon_pio, plb_addr_t src, u##size * p_data) \
> +{ \
> + int ret = 0; \
> + struct axon_pio_pci_t* p_axon_pio_pci = p_axon_pio -> context; \
> + void* src_cpu_addr; \
> + \
> + \
> + src_cpu_addr = do_ioremap( p_axon_pio_pci, src, size / 8); \
> + if ( src_cpu_addr != NULL ) { \
> +\
> + *p_data = read##type( src_cpu_addr ); \
> +\
> + do_iounmap( p_axon_pio_pci, src, src_cpu_addr ); \
> +\
> + } else {\
> + dbg_err("Unable to map IO space located on the bus 0x%p(%lld bytes) \n", src_cpu_addr, (unsigned long long)size / 8);\
> + ret = -EIO;\
> + }\
> +\
> + return ret;\
> +}
This seems like a _huge_ overhead for accessing a single register.
If this is on the PCI bus, you should really be able to use
the normal PCI subsystem.
> +inline void
> +axon_ring_msg_alloc(struct axon_ring_t *p_axon_ring,
> + dma_addr_t * p_bus_addr, void **pp_cpu_addr)
Inline functions should practically always be 'static inline'
with gcc. As with other 'alloc' type functions, you should
return the resulting pointer.
> +static irqreturn_t
> +sys_int_handler(void *data, int irq, void *dev_id, struct pt_regs *regs)
A function name should only start with 'sys_' if it is a system call
handler. Better make this 'axon_sys_int_handler'.
> +#if defined( __powerpc__)
> +
> +# define BE_HI32_FROM_BE64(x) ((__u32)( (x) >> 32))
> +# define BE_LO32_FROM_BE64(x) ((__u32)( (x) & ((1ULL<<32)-1) ))
> +# define LE_HI32_FROM_LE64(x) ((__u32)( (x) & ((1ULL<<32)-1) ))
> +# define LE_LO32_FROM_LE64(x) ((__u32)( (x) >> 32))
I guess you could define these in an architecture neutral
way as
static inline u32 be_hi32_from_be64(u64 val)
{
return *((u32 *)&val);
}
and so on.
I'm also not sure why you need this in the first place. The low-level
I/O accessors should already care about endianess.
> +# define GET_BIT_MASK_FROM_IBM_DOC(nr,type) (1ULL << ( (sizeof(type)*8 - 1) - nr ))
Nice one ;-)
I usually avoid confusing the numbers from the docs by always specifying
bit masks instead of bit numbers, e.g.
#define FOO_BIT 10
I write
#define FOO 0x0040
> +#define BExx_clr_bit(size) \
> +static inline u##size \
> +be##size##_clr_bit(u##size reg##size,int nr) \
> +{ \
> + reg##size = reg##size & ~(GET_BIT_MASK_FROM_IBM_DOC(nr,u##size)); \
> + return reg##size; \
> +}
If you really need to work with bit numbers, it's better to
use the functions from asm/bitops.h instead of defining your own.
> +
> +#if defined(AXON_DEBUG_MODULE) || defined(AXON_DEBUG)
> +#define dbg_log(...) printk(KERN_INFO AXON_DRIVER_NAME "=>" __VA_ARGS__ )
> +#else
> +#define dbg_log(...) do { } while(0)
> +#endif
> +
> +#define dbg_inf(...) printk(KERN_INFO AXON_DRIVER_NAME "=>" __VA_ARGS__ )
> +
> +#define dbg_err(...) printk(KERN_EMERG AXON_DRIVER_NAME "=>" __VA_ARGS__ )
> +
> +#endif
[style] Don't define these yourself, just use pr_debug, pr_info or
(better) dev_dbg, dev_info, dev_err, which are provided by the kernel.
> +
> +#define AXON_PLB_XDR_CHAN0_OFFSET 0x6000006000000000ULL
> +#define AXON_PLB_XDR_CHAN0_MASK 0xFFFFFFF000000000ULL
> +
> +
> +#define AXON_PLB_MASK_FROM_BE 0x000000FFFFFFFFFFULL
> +#define AXON_PLB_OFFSET_FROM_BE 0x0000010000000000ULL
> +
> +#define AXON_PLB_NIBBLE_MASK 0x000000F000000000ULL
> +#define AXON_PLB_NIBBLE_SHIFT 24
> +
> +#define AXON_MPCI_INTR_COUNT 128
> +
> +
> +#define AXON_BEI_INTR_COUNT (AXON_MPCI_INTR_COUNT + 128)
> +#define AXON_BEI_INTR_MBOX_DIRECT 136
> +#define AXON_BEI_INTR_MBOX_INDIRECT 116
> +#define AXON_BEI_INTR_MBOX_OVERFLOW 117
> +#define AXON_BEI_INTR_AUX_DMA_CH0_ERR 12
> +#define AXON_BEI_INTR_AUX_DMA_CH1_ERR 13
> +#define AXON_BEI_INTR_AUX_DMA_CH2_ERR 14
> +#define AXON_BEI_INTR_AUX_DMA_CH3_ERR 15
> +#define AXON_BEI_INTR_DMAX_CH0 119
> +#define AXON_BEI_INTR_DMAX_CH1 120
> +#define AXON_BEI_INTR_DMAX_CH2 121
> +#define AXON_BEI_INTR_DMAX_CH3 122
> +#define AXON_BEI_INTR_DMAX_CH0_ERR 123
> +#define AXON_BEI_INTR_DMAX_CH1_ERR 124
> +#define AXON_BEI_INTR_DMAX_CH2_ERR 125
> +#define AXON_BEI_INTR_DMAX_CH3_ERR 126
> +#define AXON_BEI_INTR_DMAX_SLAVE_ERR 127
> +
You can't hardcode these numbers, that immediately breaks with
newer kernels. Instead, read the numbers from the device tree.
> +#define AXON_PLB_DDR_OFFSET 0x0000000000000000ULL
> +#define AXON_PLB_DDR_SIZE 0x0000001000000000ULL
> +
> +#define AXON_PLB_UTL_REGS_OFFSET 0xA00000A000004000ULL
> +#define AXON_PLB_UTL_REGS_SIZE 0x1000
> +
> +#define AXON_PLB_CFG_REGS_OFFSET 0xA00000A000005000ULL
> +#define AXON_PLB_CFG_REGS_SIZE 0x1000
> +
> +#define AXON_PLB_DCR_REGS_OFFSET 0x8000008000000000ULL
> +#define AXON_PLB_DCR_REGS_SIZE (0x240 * 4)
> +#define AXON_PLB_PCIE_OFFSET 0xC00000C000000000ULL
> +#define AXON_PLB_PCIE_MASK 0x00000007FFFFFFFFULL
> +
> +#define AXON_PLB_REGS_SLV_OFFSET 0x4000004400000000ULL
> +#define AXON_PLB_REGS_SLV_SIZE 0x5000
> +
> +#define AXON_PLB_PCIE_REGS_OFFSET 0xA00000A000000000ULL
Same for the offsets. The kernel must not care about them.
> +
> +typedef enum {
> + AXON_DMA_CHANNEL_INVALID,
> + AXON_DMA_CHANNEL_0,
> + AXON_DMA_CHANNEL_1,
> + AXON_DMA_CHANNEL_2,
> + AXON_DMA_CHANNEL_3,
> +
> +} axon_dma_channel_t;
[style] No need for a typedef here, just make this 'enum axon_dma_channel'
> +typedef enum {
> + AXON_WIDTH_BYTE = 0,
> + AXON_WIDTH_HALFWORD,
> + AXON_WIDTH_WORD,
> + AXON_WIDTH_DOUBLEWORD,
> + AXON_WIDTH_QUADWORD
> +} axon_dma_xfer_width;
> +
> +
> +typedef enum {
> +
> + DMA_NO_INTR,
> +
> +
> + DMA_INTR_WAIT,
> +
> +
> + DMA_INTR_NOWAIT
> +} axon_dma_intr_t;
> +
same for these
> +static inline void
> +dma_aux_write(struct axon_dma_ops_dma_aux_t *p_axon_dma_ops_dma_aux,
> + u32 reg, u32 value)
> +{
> + axon_writel(value, p_axon_dma_ops_dma_aux->p_regs + reg);
> + wmb();
> +}
better use out_be32
> +static inline u32
> +dma_aux_read(struct axon_dma_ops_dma_aux_t *p_axon_dma_ops_dma_aux,
> + u32 reg)
> +{
> + return axon_readl(p_axon_dma_ops_dma_aux->p_regs + reg);
> +}
and in_be32
> +#ifndef __powerpc__
> +#define axon_virt_to_bus(x) virt_to_bus( (x) )
> +#else
> +#define axon_virt_to_bus(x) virt_to_phys( (x) )
> +#endif
virt_to_bus is inherently broken, you can't use that. Instead, you
need to use the dma mapping functions.
> +#define AXON_MAX_PIM_REG 4
> +
> +struct axon_pim_reg_t {
> +
> + u64 mask;
> + u64 base;
> + int users;
> + u32 reg;
> +
> + u32 bar_id;
> + u32 bar_len;
> +};
What do you need these for? Shouldn't the firmware set them all up
correctly?
> +typedef u64 plb_addr_t;
> +typedef u64 axon_size_bus_t;
> +
> +#if defined(__x86_64__) || defined(__powerpc64__)
> +typedef u64 axon_addr_ptr_t;
> +typedef u64 axon_size_t;
> +#elif defined(__i386__) || ( defined(__powerpc__) && !defined(__powerpc64__) )
> +
> +typedef u32 axon_addr_ptr_t;
> +typedef u32 axon_size_t;
> +
> +#else
> +
> +#error "Unsupported platform. Please add the right type mapping"
> +
> +#endif
Can't you just check BITS_PER_LONG instead of the arch specific
defines?
> +
> +#define MIN(i,j) ((i) < (j) ? (i) : (j))
> +
This one is broken when the arguments have side effects.
Please use the min() and max() macros provided by the kernel
instead.
> +#if defined(__i386__) && !defined(__x86_64__)
> +# define AXON_PLB_ADDR_FMT_T "llx"
> +# define AXON_PADDR_FMT_T "x"
> +# define AXON_VADDR_FMT_T "x"
> +# define AXON_DMA_ADDR_FMT_T "x"
> +# define AXON_U64_FMT_T "llx"
> +#elif defined(__powerpc64__)
> +# define AXON_PLB_ADDR_FMT_T "lx"
> +# define AXON_PADDR_FMT_T "lx"
> +# define AXON_VADDR_FMT_T "lx"
> +# define AXON_DMA_ADDR_FMT_T "lx"
> +# define AXON_U64_FMT_T "lx"
> +#elif defined(__x86_64__)
> +# define AXON_PLB_ADDR_FMT_T "llx"
> +# define AXON_PADDR_FMT_T "llx"
> +# define AXON_VADDR_FMT_T "llx"
> +# define AXON_DMA_ADDR_FMT_T "llx"
> +# define AXON_U64_FMT_T "llx"
> +#else
> +# error "Unsupported platform"
> +#endif
I think there was a better way to do this, but don't remember
right now.
Arnd <><
More information about the cbe-oss-dev
mailing list