[Cbe-oss-dev] [patch 03/16] Common header files of the Axon driver
Arnd Bergmann
arnd at arndb.de
Fri May 25 09:07:16 EST 2007
On Thursday 24 May 2007, jdubois at mc.com wrote:
> Various common header files.
It would be better to add the header files together with the
implementation that belongs to each of them.
> +
> +typedef struct addr_xltr_t {
> +
> + void *context;
> +
> +
> +
> + dma_addr_t(*from_plb) (struct addr_xltr_t * self,
> + plb_addr_t plb_bus_addr);
> +
> + plb_addr_t(*to_plb) (struct addr_xltr_t * self,
> + dma_addr_t cpu_bus_addr);
> +
> + int (*is_local) (struct addr_xltr_t * self, plb_addr_t plb_bus_addr);
> +
> + void (*destroy) (struct addr_xltr_t * self);
> +} addr_xltr_t;
Please don't use typedef on data structures that are meant to
be passed by reference, i.e. anything larger than 8 bytes, and
drop the _t postfix on the struct name.
I can only guess what the documentation looks like, but it's
probably not following the kerneldoc style. I know that most of
the headers are wrong, but for new code it's always good to
do it right. See include/linux/usb.h for a good example on kerneldoc.
> +static inline dma_addr_t axon_addr_xltr_from_plb(addr_xltr_t * self,
> + plb_addr_t plb_bus_addr)
> +{
> + return self->from_plb(self, plb_bus_addr);
> +
> +}
> +
> +static inline plb_addr_t axon_addr_xltr_to_plb(addr_xltr_t * self,
> + dma_addr_t cpu_bus_addr)
> +{
> + return self->to_plb(self, cpu_bus_addr);
> +
> +}
Why do you need these? All handling of DMA addresses should normally
done through the DMA mapping API. arch/powerpc/kernel/iommu.c should
do the right thing here if the drivers just use the common dma_map_*
functions.
> +
> +#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))
> +# define GET_BIT_MASK_FROM_IBM_DOC(nr,type) (1ULL << ( (sizeof(type)*8 - 1) - nr ))
> +
> +#elif defined(__i386__) || defined(__x86_64__)
> +
> +# define LE_HI32_FROM_LE64(x) ((__u32)( (x) >> 32))
> +# define LE_LO32_FROM_LE64(x) ((__u32)( (x) & ((1ULL<<32)-1) ))
> +# define BE_HI32_FROM_BE64(x) ((__u32)( (x) & ((1ULL<<32)-1) ))
> +# define BE_LO32_FROM_BE64(x) ((__u32)( (x) >> 32))
> +# define GET_BIT_MASK_FROM_IBM_DOC(nr,type) (1ULL << ( (~((nr) & 0x7) & 0x7) + (nr & ~0x7)) )
> +
> +#else
> +# error "Unsupported platform"
> +#endif
Please use the common cpu_to_{be,le}XX macros provided by all architectures.
> +#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; \
> +}
and use the generic bitops implementation here. If you always convert
to native endianess at the HW access, you shouldn't need to make a difference
here.
> +
> +#ifndef AXON_DBG_H
> +#define AXON_DBG_H
> +
> +#include <linux/kernel.h>
> +
> +#define AXON_DRIVER_NAME "Axon"
> +
> +
> +#if defined(AXON_DEBUG_MODULE) || defined(AXON_DEBUG)
> +#define dbg_log(...) { printk(KERN_DEBUG AXON_DRIVER_NAME ":%s=>", __FUNCTION__); printk(__VA_ARGS__ ); }
> +#else
> +#define dbg_log(...) do { } while(0)
> +#endif
> +
> +#define dbg_inf(...) { printk(KERN_INFO AXON_DRIVER_NAME ":%s=>", __FUNCTION__); printk(__VA_ARGS__ ); }
> +
> +#define dbg_err(...) { printk(KERN_ERR AXON_DRIVER_NAME ":%s=>", __FUNCTION__); printk(__VA_ARGS__ ); }
> +
> +#endif
Just use dev_dbg/dev_info/dev_... in your driver instead of defining
your own.
> +
> +#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_MPIC_INTR_COUNT (128+4+4)
> +
Kill these definitions here, if a driver needs the values, it should
look in the device tree.
> +#define AXON_BEI_INTR_COUNT (AXON_MPIC_INTR_COUNT + 2)
> +#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
same here
> +#define AXON_C3PO_REGS_OFFSET 0x0000
> +#define AXON_C3PO_SCRATCHPAD_OFFSET 0xC00
> +#define AXON_DMAX_REGS_OFFSET 0x1000
> +#define AXON_MBOX_REGS_OFFSET 0x2000
> +#define AXON_SCRATCHPAD_REGS_OFFSET 0xC00
these should probably be local to the respective drivers,
if you can't have them in the device tree. I guess you need
them here, because they are part of a single register
are in the PCIe device when seen from the host.
> +#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
> +
> +#define AXON_DCR_STEP 0x10
We already have an abstraction for DCR. Use it ;-)
It probably involves adding the dcr interfaces for the
case of the PCIe bus, but the result will be a lot cleaner.
> +#define AXON_PLB_MBOX_MESSAGE_ADDRESS 0x4000004400002000ULL
> +
> +#define AXON_MBOX_ACCESS_REGION_ADDR_HI (0x4E * AXON_DCR_STEP)
> +#define AXON_MBOX_ACCESS_REGION_ADDR_LO (0x4F * AXON_DCR_STEP)
> +#define AXON_MBOX_ACCESS_REGION_MASK_HI (0x50 * AXON_DCR_STEP)
> +#define AXON_MBOX_ACCESS_REGION_MASK_LO (0x51 * AXON_DCR_STEP)
> +
> +#define AXON_MBOX_DEFAULT_ACCESS_REGION_ADDR_HI 0x40000044
> +#define AXON_MBOX_DEFAULT_ACCESS_REGION_ADDR_LO 0x00002000
> +#define AXON_MBOX_DEFAULT_ACCESS_REGION_MASK_HI 0x7FFFFFFF
> +#define AXON_MBOX_DEFAULT_ACCESS_REGION_MASK_LO 0xFFFFF005
> +#define AXON_MBOX_DISABLE_ACCESS_REGION_MASK_LO 0xFFFFF004
> +
> +#define AXON_MBOX_PARK_ACCESS_REGION_ADDR_HI 0x80000080
> +
> +#define AXON_MBOX_CR (0x68 * AXON_DCR_STEP)
> +#define AXON_MBOX_MESSAGE_ADDR_HI (0x69 * AXON_DCR_STEP)
> +#define AXON_MBOX_MESSAGE_ADDR_LO (0x6A * AXON_DCR_STEP)
> +#define AXON_MBOX_BASE_ADDR_HI (0x6B * AXON_DCR_STEP)
> +#define AXON_MBOX_BASE_ADDR_LO (0x6C * AXON_DCR_STEP)
> +#define AXON_MBOX_READ_OFFSET (0x6D * AXON_DCR_STEP)
> +#define AXON_MBOX_WRITE_OFFSET (0x6E * AXON_DCR_STEP)
> +
> +#define AXON_MBOX_CR_SIZE_32K (0)
> +#define AXON_MBOX_CR_SIZE_64K (1)
> +#define AXON_MBOX_CR_SIZE_128K (2)
> +#define AXON_MBOX_CR_SIZE_256K (3)
> +
> +#define AXON_MBOX_CR_SIZE_hi (22)
> +#define AXON_MBOX_CR_SIZE_lo (23)
> +#define AXON_MBOX_CR_STOP_INC (27)
> +#define AXON_MBOX_CR_INTR_INDIRECT (28)
> +#define AXON_MBOX_CR_INTR_DIRECT (29)
> +#define AXON_MBOX_CR_INTR_FULL (30)
> +#define AXON_MBOX_CR_EN (31)
Should go into the mbox driver, I guess.
> +#define AXON_UTL_INTR_COUNT 8
> +
> +
> +#define AXON_BAR2_PCIE_OFFSET 0x4000
> +#define AXON_PCIE_UTL_REGS_OFFSET (0x0000 + AXON_BAR2_PCIE_OFFSET)
> +#define AXON_PCIE_CFG_REGS_OFFSET (0x1000 + AXON_BAR2_PCIE_OFFSET)
UTL should really be a cascaded interrupt controller,
not something driver specific.
> +#define PCIe_PIM0_LOWER 0x340
> +#define PCIe_PIM0_UPPER 0x344
> +
> +#define PCIe_PIM1_LOWER 0x348
> +#define PCIe_PIM1_UPPER 0x34C
> +#define PCIe_PIM1_SIZE_LOWER 0x350
> +#define PCIe_PIM1_SIZE_UPPER 0x354
> +
> +#define PCIe_PIM2_LOWER 0x358
> +#define PCIe_PIM2_UPPER 0x35C
> +
> +#define PCIe_PIM3_LOWER 0x360
> +#define PCIe_PIM3_UPPER 0x364
> +
> +#define PCIe_PIM4_LOWER 0x368
> +#define PCIe_PIM4_UPPER 0x36C
> +#define PCIe_PIM34_SIZE_LOWER 0x370
> +#define PCIe_PIM34_SIZE_UPPER 0x374
> +
> +#define PCIe_PIM6_LOWER 0x3C0
> +#define PCIe_PIM6_UPPER 0x3C4
> +#define PCIe_PIM26_SIZE 0x3C8
> +
> +#define PCIe_PIM5_LOWER 0x378
> +#define PCIe_PIM5_UPPER 0x37C
> +
> +
> +#define PCIe_POM0_LOWER 0x380
> +#define PCIe_POM0_HIGHER 0x384
> +
> +#define PCIe_POM1_LOWER 0x388
> +#define PCIe_POM1_HIGHER 0x38C
> +
> +#define PCIe_POM2_LOWER 0x390
> +#define PCIe_POM2_HIGHER 0x394
These should not be touched by the operating system, but
instead be set up by the firmware.
> +#define AuxDMA_DESC_STEP 0x4
> +
> +#define AuxDMA_Base (AXON_DCR_STEP * 0x100)
> +#define AuxDMA_Len ((AXON_DCR_STEP * 0x140) - AuxDMA_Base)
> +
> +#define AuxDMA_CRn (AuxDMA_DESC_STEP * 0x00)
> +#define AuxDMA_CTCn (AuxDMA_DESC_STEP * 0x01)
> +#define AuxDMA_SAHn (AuxDMA_DESC_STEP * 0x02)
> +#define AuxDMA_SALn (AuxDMA_DESC_STEP * 0x03)
>
> ...
Move to the file implementing the code for the DMA controller?
> +typedef unsigned long axon_dma_target_t;
shouldn't that be u64? Otherwise it will break for 32 bit hosts.
> +
> +static inline void dma_aux_write(axon_dma_ops_dma_aux_t *
> + p_axon_dma_ops_dma_aux, u32 reg, u32 value)
> +{
> + out_be32(p_axon_dma_ops_dma_aux->p_regs + reg, value);
> + wmb();
> +}
out_be32 implies wmb(), no need to do it again.
> +typedef enum {
> + AXON_DMA_SOFT_DIRECTION_INVALID,
> + AXON_DMA_SOFT_DIRECTION_FROM_CPU,
> + AXON_DMA_SOFT_DIRECTION_TO_CPU,
> +} axon_dma_soft_direction_t;
like structs, you don't need a typedef for an enum.
> +typedef struct axon_driver_t {
> +
> + struct list_head list;
> +
> + char name[32];
> +
> +
> + int (*probe) (struct axon_t * p_axon);
> +
> +
> + int (*start) (struct axon_t * p_axon);
> + int (*stop) (struct axon_t * p_axon);
> +
> +
> + int (*remove) (struct axon_t * p_axon);
> +
> +} axon_driver_t;
A driver structure normally needs to embed a struct device_driver.
Why doesn't this one?
> +typedef irqreturn_t(*axon_irq_handler_t) (void *axon_data, int irq,
> + void *dev_id);
strange abstraction, why do you need _two_ void pointers?
Normally, you should only need
typedef irqreturn_t(*axon_irq_handler_t) (struct device *dev);
and then do container_of(dev) to get to your real device.
> +typedef struct axon_t {
> +
> + struct list_head list;
> +
> + void *context;
> +
> + char name[32];
> +
> + u8 id;
> +
> + struct proc_dir_entry *dir;
> +
> +
> + int (*prepare_soft_reset) (struct axon_t * p_axon);
> + int (*finish_soft_reset) (struct axon_t * p_axon, u32 timeout);
> +
> +
> + int (*register_irq_handler) (struct axon_t * p_axon, int irq_nr,
> + axon_irq_handler_t handler,
> + char *name, void *data);
> + int (*unregister_irq_handler) (struct axon_t * p_axon, int irq_nr,
> + axon_irq_handler_t handler);
> +
> +
> + struct device *(*get_device) (struct axon_t * p_axon);
> +
> +
> + struct axon_sms_t *(*sms_get) (struct axon_t * p_axon);
> +
> +
> + struct axon_mbox_t *(*mbox_get) (struct axon_t * p_axon);
> +
> +
> + struct axon_dmax_t *(*dmax_get) (struct axon_t * p_axon);
> +
> + struct axon_pio_t *(*pio_get) (struct axon_t * p_axon);
> +
> + struct addr_xltr_t *(*addr_xltr_get) (struct axon_t * p_axon);
> +
> +
> + void __iomem *(*regs_mbox_get) (struct axon_t * p_axon);
> + void __iomem *(*regs_utl_get) (struct axon_t * p_axon);
> + void __iomem *(*regs_dmax_get) (struct axon_t * p_axon);
> + void __iomem *(*regs_dma_aux_get) (struct axon_t * p_axon);
> + void __iomem *(*regs_c3po_get) (struct axon_t * p_axon);
> + void __iomem *(*regs_dcr_get) (struct axon_t * p_axon);
> +
> + int (*pim_map) (struct axon_pim_t * p_pim, plb_addr_t plb_addr,
> + size_t size);
> + void (*pim_unmap) (struct axon_pim_t * p_pim, plb_addr_t plb_addr);
> +
> + struct axon_mbox_t *(*peer_mbox_get) (struct axon_t * p_axon);
> +
> + void (*enable_irq) (struct axon_t * p_axon, int irq_nr);
> + void (*disable_irq) (struct axon_t * p_axon, int irq_nr);
> +} axon_t;
This abstraction looks backwards. We already have one device structure
for each of the parts (mbox, dmax, dma, ...), so they should each have
a device driver that exports a high-level interface, instead of
allowing arbitrary parts of the code get register access.
> +#ifndef AXON_IO_H
> +#define AXON_IO_H
> +
> +#include <asm/io.h>
> +
> +
> +#if defined(__x86_64__) || defined(__powerpc64__)
> +static inline void axon_writeq(u64 q, void __iomem * addr)
> +{
> + __raw_writeq(q, addr);
> +
> +}
> +
> +#define AXON_HAS_WRITEQ 1
> +#else
> +#undef AXON_HAS_WRITEQ
> +#endif
> +
> +static inline void axon_writel(u32 l, void __iomem * addr)
> +{
> + __raw_writel(l, addr);
> +
> +}
> +static inline void axon_writew(u16 w, void __iomem * addr)
> +{
> + __raw_writew(w, addr);
> +
> +}
> +static inline void axon_writeb(u8 b, void __iomem * addr)
> +{
> + __raw_writeb(b, addr);
> +
> +}
> +
> +#if defined(__x86_64__) || defined(__powerpc64__)
> +static inline u64 axon_readq(const void __iomem * addr)
> +{
> + return __raw_readq(addr);
> +
> +}
> +
> +#define AXON_HAS_READQ 1
> +#else
> +#undef AXON_HAS_READQ
> +#endif
> +
> +static inline u32 axon_readl(const void __iomem * addr)
> +{
> + return __raw_readl(addr);
> +
> +}
> +static inline u16 axon_readw(const void __iomem * addr)
> +{
> + return __raw_readw(addr);
> +
> +}
> +static inline u8 axon_readb(const void __iomem * addr)
> +{
> + return __raw_readb(addr);
> +
> +}
You shouldn't use __raw_{read,write} functions in generic code.
For the PCIe access, use {read,write}{b,w,l,q}, and on the cell
side, use {in,out}_be{8,16,32,64}.
> +#ifndef __powerpc__
> +#define axon_virt_to_bus(x) virt_to_bus( (x) )
> +#else
> +#define axon_virt_to_bus(x) virt_to_phys( (x) )
> +#endif
You're kidding, right? virt_to_bus is so deprecated, it's not
even funny any more.
Use the dma-mapping API if you have drivers that depend on these
accessors.
> +#if LINUX_VERSION_CODE > KERNEL_VERSION(2,6,19)
> + struct delayed_work irq_poller;
> +#else
> + struct work_struct irq_poller;
> +#endif
For you that you submit upstream for integration, please remove the
version checks, as they are pointless once the code is merged.
> +typedef struct axon_pim_reg_t {
> +
> + u64 mask;
> + u64 base;
> + int users;
> + u32 reg;
> +
> + u32 bar_id;
> + u32 bar_len;
> +} axon_pim_reg_t;
What do you need these for? Doesn't the firmware handle pim setup?
> +#ifdef __KERNEL__
> +
> +#include <linux/types.h>
> +
> +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
What are these used for? Can't you use any of the already present types
like dma_addr_t?
> +
> +#else
> +
> +#include <stdint.h>
> +
> +typedef uint64_t plb_addr_t;
> +typedef uint64_t axon_size_bus_t;
> +
> +#if defined(__x86_64__) || defined(__powerpc64__)
> +
> +typedef uint64_t axon_addr_ptr_t;
> +typedef uint64_t axon_size_t;
> +
> +#elif defined(__i386__) || ( defined(__powerpc__) && !defined(__powerpc64__) )
> +
> +typedef uint32_t axon_addr_ptr_t;
> +typedef uint32_t axon_size_t;
> +
> +#else
> +
> +#error "Unsupported platform. Please add the right type mapping"
> +
> +#endif
Why do you care in user space? Just make the kernel interface use
64 bit data structures in any case, it will make you emulation code
a lot easier.
> +#ifndef AXON_UTILS_H
> +#define AXON_UTILS_H
> +
> +
> +#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
> +
> +#endif
If you define them all as 'long long', you have the added
bonus of not needing these macros ;-).
The more common convention in the kernel is to just
cast to a known type in the printk output, instead of
defining format string macros.
Arnd <><
More information about the cbe-oss-dev
mailing list