[Cbe-oss-dev] [PATCH] axonram: 1st version
Arnd Bergmann
arnd at arndb.de
Wed Feb 14 11:09:43 EST 2007
On Wednesday 14 February 2007 00:06, Maxim Shchetynin wrote:
>
> Hello,
>
> This patch implements access to Axon's DDR2 memory via character and block
> devices.
>
>
> Subject: cell: driver for DDR2 memory on AXON
>
> From: Maxim Shchetynin <maxim.shchetynin at de.ibm.com>
>
The patch is missing any description whatsoever. You should really
at least describe what the hardware does that you are driving and
what the user interface is.
You should also mention how it relates to the driver posted by
JC for the same hardware.
> Signed-off-by: Arnd Bergmann <arnd.bergmann at de.ibm.com>
When you send out a patch, you need to put your _own_ Signed-off-by
into the description, not mine.
> Index: linux-2.6.20-axonram/arch/powerpc/Kconfig
> ===================================================================
> diff -Nuar linux-2.6.20/arch/powerpc/Kconfig
> linux-2.6.20-axonram/arch/powerpc/Kconfig
> --- linux-2.6.20/arch/powerpc/Kconfig 2007-02-13 20:42:42.000000000
> +0100
> +++ linux-2.6.20-axonram/arch/powerpc/Kconfig 2007-02-13
> 20:54:06.000000000 +0100
Your mail program broke the line wraps, this patch will not
apply like this.
> @@ -727,6 +727,13 @@
>
> If in doubt, say N here.
>
> +config AXON_RAM
> + tristate "Axon DDR2 memory device driver"
> + depends on PPC_IBM_CELL_BLADE
> + default y
> + help
> + Block and character device driver for Axon DDR2 RAM support.
> +
> endmenu
This also could use some more text.
> @@ -0,0 +1,501 @@
> +/*
> + * (C) Copyright IBM Corp. 2006
This line is formatted in a strange way. You should put your
name in here as well.
> +
> +#include <asm/mmu.h>
> +#include <asm/of_device.h>
> +#include <asm/of_platform.h>
> +#include <asm/page.h>
> +#include <asm/pgtable.h>
> +#include <asm/prom.h>
> +#include <asm/string.h>
> +#include <asm/uaccess.h>
> +#include <linux/bio.h>
> +#include <linux/blkdev.h>
> +#include <linux/compiler.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/fs.h>
> +#include <linux/genhd.h>
> +#include <linux/kernel.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mm.h>
> +#include <linux/mm_types.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/stat.h>
> +#include <linux/types.h>
Normally, linux/ includes come before asm/
> +static int
> +axon_ram_open(struct inode *, struct file *);
> +static int
> +axon_ram_release(struct inode *, struct file *);
> +static loff_t
> +axon_ram_llseek(struct file *, loff_t, int);
> +static ssize_t
> +axon_ram_read(struct file *, char __user *, size_t, loff_t *);
> +static ssize_t
> +axon_ram_write(struct file *, const char __user *, size_t, loff_t *);
> +static int
> +axon_ram_mmap(struct file *, struct vm_area_struct *);
> +static int
> +axon_ram_direct_access(struct block_device *, sector_t, unsigned long *);
> +static int
> +axon_ram_make_request(struct request_queue *, struct bio *);
> +static int
> +axon_ram_probe(struct of_device *, const struct of_device_id *);
> +static int
> +axon_ram_remove(struct of_device *);
try to avoid forward declarations by defining the functions themselves
in the right order.
> +struct axon_ram_bank {
> + char chrdev_name[DEVICE_NAME_SIZE];
> + char blkdev_name[DEVICE_NAME_SIZE];
> + rwlock_t lock;
> + phys_addr_t phys_addr;
> + void __iomem *addr;
> + size_t size;
> + struct gendisk *disk;
> + struct miscdevice misc;
> + struct file_operations fops;
> +};
File operations should be const nowadays, so just make them a
global 'const struct file_operations' which you initialize
completely.
> +/**
> + * axon_ram_open - open() method for device
> + * @inode, @file: see file_operations method
> + */
> +static int
> +axon_ram_open(struct inode *inode, struct file *file)
> +{
> + struct axon_ram_bank *bank;
> +
> + /* Prevent access to memory bank using simultaneously both */
> + /* block and character device methods. */
> + if (S_ISCHR(inode->i_mode)) {
> + file->private_data = (void*) file->f_op +
> + sizeof(struct file_operations) -
> + sizeof(struct axon_ram_bank);
> + bank = (struct axon_ram_bank*) file->private_data;
> +
> + /* Allow only a single open of character device. */
> + /* No access to device if it is already read-locked */
> + /* (opened at least one time as block device). */
> + if (!write_trylock(&bank->lock))
> + return -EBUSY;
> + } else if (S_ISBLK(inode->i_mode)) {
> + bank = (struct axon_ram_bank*)
> + inode->i_bdev->bd_disk->private_data;
> +
> + /* Block device maybe opened multiple times therefore */
> + /* read-lock. No access to device if it is already */
> + /* write-locked (opened as character device). */
> + if (!read_trylock(&bank->lock))
> + return -EBUSY;
> + inode->i_bdev->bd_block_size = 1 << AXON_RAM_BLOCK_SHIFT;
> + } else {
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
Why not just make this two functions? The block device and the
character device are very different entitities.
> +/**
> + * axon_ram_release - release() method for device
> + * @inode, @file: see file_operations method
> + */
> +static int
> +axon_ram_release(struct inode *inode, struct file *file)
> +{
> + struct axon_ram_bank *bank;
> +
> + if (S_ISCHR(inode->i_mode)) {
> + bank = (struct axon_ram_bank*)
> + file->private_data;
> + write_unlock(&bank->lock);
> + } else if (S_ISBLK(inode->i_mode)) {
> + bank = (struct axon_ram_bank*)
> + inode->i_bdev->bd_disk->private_data;
> + read_unlock(&bank->lock);
> + } else {
> + /* Paranoja */
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
same here.
> +/**
> + * axon_ram_llseek - llseek() method for character device
> + * @file, @offset, @origin: see file_operations method
> + */
> +static loff_t
> +axon_ram_llseek(struct file *file, loff_t offset, int origin)
> +{
> + struct axon_ram_bank *bank;
> +
> + bank = (struct axon_ram_bank*) file->private_data;
file->private_data is a void pointer, so you don't need the
explicit cast.
> + switch (origin) {
> + case SEEK_CUR:
> + offset += file->f_pos;
> + break;
> +
> + case SEEK_END:
> + offset += bank->size;
> + break;
> + }
> +
> + /* No access outside a memory DIMM. */
> + if (offset < 0 || offset > bank->size)
> + return -EINVAL;
> +
> + return file->f_pos = offset;
> +}
> +
> +/**
> + * axon_ram_read - read() method for character device
> + * @file, @buffer, @size, @offset: see file_operations method
> + */
> +static ssize_t
> +axon_ram_read(struct file *file,
> + char __user *buffer, size_t size, loff_t *offset)
> +{
> + struct axon_ram_bank *bank;
> +
> + bank = (struct axon_ram_bank*) file->private_data;
> + /* No access outside a memory DIMM but still do as much as possible.
> */
> + if (*offset + size > bank->size)
> + size = bank->size - *offset;
> + copy_to_user(__user buffer, __iomem bank->addr + *offset, size);
This is not how __iomem annotations work. If you try to run this code
through sparse, you will get an error. It's also a good idea to fix
all the warnings that you will find using sparse.
> + *offset += size;
> +
> + return size;
> +}
You can simplify this whole function significantly using
simple_copy_from_buffer().
> +/**
> + * axon_ram_write - write() method for character device
> + * @file, @buffer, @size, @offset: see file_operations method
> + */
> +static ssize_t
> +axon_ram_write(struct file *file,
> + const char __user *buffer, size_t size, loff_t *offset)
> +{
> + struct axon_ram_bank *bank;
> +
> + bank = (struct axon_ram_bank*) file->private_data;
> + /* No access outside a memory DIMM but still do as much as possible.
> */
> + if (*offset + size > bank->size)
> + size = bank->size - *offset;
> + copy_from_user(__iomem bank->addr + *offset, __user buffer, size);
> + *offset += size;
> +
> + return size;
> +}
same comments apply here.
> +/**
> + * axon_ram_mmap - mmap() method for character device
> + * @file, @vm: see file_operations method
> + */
> +static int
> +axon_ram_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> + struct axon_ram_bank *bank;
> + unsigned long page;
> + unsigned long size;
> + pgprot_t page_prot;
> + int retval = 0;
> +
> + bank = (struct axon_ram_bank*) file->private_data;
> +
> + size = vma->vm_end - vma->vm_start;
> +
> + /* No access outside a memory DIMM. */
> + if ((vma->vm_pgoff << PAGE_SHIFT) + size > bank->size) {
> + retval = -ERANGE;
> + goto out;
> + }
> +
> + /* Transfer real address to page number. */
> + page = bank->phys_addr;
> + page >>= PAGE_SHIFT;
> + page += vma->vm_pgoff;
> +
> + /* Switch off page-guard. */
> + page_prot = pgprot_val(vma->vm_page_prot);
> + page_prot &= ~(_PAGE_NO_CACHE | _PAGE_GUARDED);
> + vma->vm_page_prot = __pgprot(page_prot);
> + if (remap_pfn_range(vma, vma->vm_start, page, size, vma->vm_page_prot))
> + retval = -EAGAIN;
> +
> +out:
> + return retval;
> +}
Do you really mean to map the memory in cacheable mode? That sounds
very wrong, since it is outside of the coherency domain.
> +/**
> + * axon_ram_direct_access - direct_access() method for block device
> + * @device, @sector, @data: see block_device_operations method
> + */
> +static int
> +axon_ram_direct_access(struct block_device *device, sector_t sector,
> + unsigned long *data)
> +{
> + struct axon_ram_bank *bank;
> + loff_t offset;
> +
> + bank = (struct axon_ram_bank*) device->bd_disk->private_data;
> + /* Paranoja in it's best traditions. */
> + if (bank == NULL)
> + return -ENODEV;
For paranoia checks, you can better use
BUG_ON(!bank);
However, since you dereference bank a few lines below, you will
get a nice Oops message when you hit that, even without the BUG_ON().
However, with the check you have in here, you don't get any
message that there is something wrong, but only a silent error
message.
> + /* No access outside a memory DIMM. */
> + offset = sector << AXON_RAM_SECTOR_SHIFT;
> + if (offset > bank->size)
> + return -ERANGE;
> +
> + *data = (unsigned long) (bank->phys_addr + offset);
> +
> + return 0;
> +}
> +
> +/**
> + * axon_ram_make_request - make_request() method for block device
> + * @queue, @bio: see blk_queue_make_request()
> + */
> +static int
> +axon_ram_make_request(struct request_queue *queue, struct bio *bio)
> +{
> + struct axon_ram_bank *bank;
> + unsigned long phys_mem, phys_end, user_mem;
> + unsigned long transfered;
> + struct bio_vec *vec;
> + int i;
> +
> + bank = (struct axon_ram_bank*) bio->bi_bdev->bd_disk->private_data;
> + phys_mem = (unsigned long)
> + bank->addr + (bio->bi_sector << AXON_RAM_SECTOR_SHIFT);
> + phys_end = (unsigned long)
> + bank->addr + bank->size;
If you always need to cast bank->addr to unsigned long, maybe it should
have been an unsigned long in the first place.
> + transfered = 0;
> + bio_for_each_segment(vec, bio, i) {
> + /* No access outside a memory DIMM. */
> + if (phys_mem + vec->bv_len > phys_end) {
> + bio_io_error(bio, bio->bi_size);
> + return -ERANGE;
> + }
> +
> + user_mem = (unsigned long)
> + page_address(vec->bv_page) + vec->bv_offset;
> + if (bio_data_dir(bio) == READ)
> + memcpy((void*) user_mem, (void*) phys_mem, vec->bv_len);
> + else
> + memcpy((void*) phys_mem, (void*) user_mem, vec->bv_len);
This should probably be memcpy_toio and memcpy_fromio, if only to avoid
the warning message from sparse.
> + phys_mem += vec->bv_len;
> + transfered += vec->bv_len;
> + }
> + bio_endio(bio, transfered, 0);
> +
> + return 0;
> +}
> +
> +static struct block_device_operations axon_ram_devops = {
> + .owner = THIS_MODULE,
> + .open = axon_ram_open,
> + .release = axon_ram_release,
> + .direct_access = axon_ram_direct_access
> +};
> +
> +/**
> + * axon_ram_probe - probe() method for platform driver
> + * @device, @device_id: see of_platform_driver method
> + */
> +static int
> +axon_ram_probe(struct of_device *device, const struct of_device_id
> *device_id)
> +{
> + static int axon_ram_bank_id = -1;
> + struct axon_ram_bank *bank;
> + const u32 *reg0;
> + const u64 *reg1;
> + phys_addr_t phys_addr;
> + size_t size;
> +
> + /* There should be a better solution... */
> + axon_ram_bank_id++;
One thing you could use for this is 'idr' (see include/linux/idr.h).
> + printk("Found DDR2 memory DIMM %s%d on %s\n",
> + AXON_RAM_DEVICE_NAME, axon_ram_bank_id,
> + device->node->full_name);
This needs a console log level, like KERN_DEBUG. You should probably
just use dev_dbg or dev_info for printing messages, that will also
output the name of the driver and the device for you.
> + reg0 = get_property(device->node, "reg", NULL);
> + if (reg0 == NULL) {
> + axon_ram_error("Cannot access device tree!\n");
> + return -EFAULT;
> + }
> + reg1 = (void*) reg0 + sizeof(u64);
> +
> + phys_addr = of_translate_address(device->node, reg0);
> + if (phys_addr == OF_BAD_ADDR) {
> + axon_ram_error("Cannot access register in device tree!\n");
> + return -EFAULT;
> + }
> +
> + size = *reg1;
> + if (size == 0) {
> + axon_ram_error("Cannot access register in device tree!\n");
> + return -EFAULT;
> + }
This could simply use of_address_to_resource().
> + bank = kzalloc(sizeof(struct axon_ram_bank), GFP_KERNEL);
> + if (bank == NULL) {
> + axon_ram_error("Out of memory!\n");
> + return -ENOMEM;
> + }
> + rwlock_init(&bank->lock);
> + device->dev.platform_data = (void*) bank;
> +
> + snprintf(bank->chrdev_name, DEVICE_NAME_SIZE, "%s%d",
> + AXON_RAM_CHRDEV_NAME, axon_ram_bank_id);
> + snprintf(bank->blkdev_name, DEVICE_NAME_SIZE, "%s%d",
> + AXON_RAM_BLKDEV_NAME, axon_ram_bank_id);
> + bank->phys_addr = phys_addr;
> + bank->addr = __iomem ioremap_flags(phys_addr, size, 0);
> + bank->size = size;
> +
> + bank->misc.minor = MISC_DYNAMIC_MINOR;
> + bank->misc.name = bank->chrdev_name;
> + bank->misc.fops = &bank->fops;
> + bank->misc.parent = &device->dev;
> +
> + bank->fops.owner = THIS_MODULE;
> + bank->fops.open = axon_ram_open;
> + bank->fops.release = axon_ram_release;
> + bank->fops.llseek = axon_ram_llseek;
> + bank->fops.read = axon_ram_read;
> + bank->fops.write = axon_ram_write;
> + bank->fops.mmap = axon_ram_mmap;
as mentioned, misc and fops should just be global variables.
> + /* Don't quit if it fails - maybe at least block device would work.
> */
> + if (misc_register(&bank->misc) != 0)
> + axon_ram_error("Cannot register character device!\n");
> +
> + bank->disk = alloc_disk(AXON_RAM_MINORS_PER_DISK);
> + if (bank->disk == NULL) {
> + axon_ram_error("Cannot register disk!\n");
> + if (bank->misc.minor != MISC_DYNAMIC_MINOR)
> + misc_deregister(&bank->misc);
> + kfree(bank);
> + return -EFAULT;
> + }
> +
> + strcpy(bank->disk->disk_name, bank->blkdev_name);
> +
> + bank->disk->major = register_blkdev(0, bank->disk->disk_name);
> + if (bank->disk->major < 0) {
> + axon_ram_error("Cannot register block device!\n");
> + if (bank->misc.minor != MISC_DYNAMIC_MINOR)
> + misc_deregister(&bank->misc);
> + del_gendisk(bank->disk);
> + kfree(bank);
> + return -EFAULT;
> + }
> +
> + bank->disk->first_minor = 0;
> + bank->disk->fops = &axon_ram_devops;
> + bank->disk->queue = blk_alloc_queue(GFP_KERNEL);
> + bank->disk->private_data = (void*) bank;
> + bank->disk->driverfs_dev = &device->dev;
> +
> + set_capacity(bank->disk, bank->size >> AXON_RAM_SECTOR_SHIFT);
> + blk_queue_make_request(bank->disk->queue, axon_ram_make_request);
> + blk_queue_hardsect_size(bank->disk->queue, 1 <<
> AXON_RAM_BLOCK_SHIFT);
> + add_disk(bank->disk);
> +
> + return 0;
> +}
> +
> +/**
> + * axon_ram_remove - remove() method for platform driver
> + * @device: see of_platform_driver method
> + */
> +static int
> +axon_ram_remove(struct of_device *device)
> +{
> + struct axon_ram_bank *bank;
> + int rc = 0;
> +
> + bank = (struct axon_ram_bank*) device->dev.platform_data;
> +
> + if (bank->misc.minor != MISC_DYNAMIC_MINOR)
> + rc |= misc_deregister(&bank->misc);
> +
> + if (bank->disk != NULL) {
> + if (bank->disk->major > 0)
> + rc |= unregister_blkdev(bank->disk->major,
> + bank->disk->disk_name);
> + del_gendisk(bank->disk);
> + }
> +
> + if (rc == 0)
> + kfree(bank);
> +
> + return rc == 0 ? 0 : -EFAULT;
> +}
EFAULT is the wrong return code here. You should try to use
one of the values returned by the failing function.
> +static struct of_device_id axon_ram_device_id[] = {
> + {
> + .type = "dma-memory"
> + },
> + {}
> +};
This looks somewhat underspecified. I think you should at
least match a 'compatible' property, or the firmware should
be more specific with the device type.
Arnd <><
More information about the cbe-oss-dev
mailing list