Add Marvell UMI driver
PATCH 1/1 Add Marvell UMI driver
The Marvell Universal Message Interface (UMI) defines a messaging
interface between host and Marvell products. It considers situations of limited system resource and optimized system performance.
UMI driver translates host request to message and sends message to
FW via UMI, FW receives message and processes it, then sends response to UMI driver.
FW generates an interrupt when it needs to send information or
responseto UMI driver.
--- drivers/scsi/Kconfig | 9 + drivers/scsi/Makefile | 1 + drivers/scsi/mvumi.c | 2245 +++++++++++++++++++++++++++++++++++++++++++++++ drivers/scsi/mvumi.h | 610 +++++++++++++ include/linux/pci_ids.h | 2 + 5 files changed, 2867 insertions(+), 0 deletions(-) create mode 100644 drivers/scsi/mvumi.c create mode 100644 drivers/scsi/mvumi.h
drivers/scsi/Kconfig +9 -0
@@ -559,6 +559,15 @@ source "drivers/scsi/aic7xxx/Kconfig.aic79xx" source "drivers/scsi/aic94xx/Kconfig" source "drivers/scsi/mvsas/Kconfig" +config SCSI_MVUMI + tristate "Marvell UMI driver" + depends on SCSI && PCI + help + Module for Marvell Universal Message Interface(UMI) driver + + To compile this driver as a module, choose M here: the + module will be called mvumi. + config SCSI_DPT_I2O tristate "Adaptec I2O RAID support " depends on SCSI && PCI && VIRT_TO_BUS
drivers/scsi/Makefile +1 -0
@@ -133,6 +133,7 @@ obj-$(CONFIG_SCSI_IBMVFC) += ibmvscsi/ obj-$(CONFIG_SCSI_HPTIOP) += hptiop.o obj-$(CONFIG_SCSI_STEX) += stex.o obj-$(CONFIG_SCSI_MVSAS) += mvsas/ +obj-$(CONFIG_SCSI_MVUMI) += mvumi.o obj-$(CONFIG_PS3_ROM) += ps3rom.o obj-$(CONFIG_SCSI_CXGB3_ISCSI) += libiscsi.o libiscsi_tcp.o cxgbi/ obj-$(CONFIG_SCSI_CXGB4_ISCSI) += libiscsi.o libiscsi_tcp.o cxgbi/
drivers/scsi/mvumi.c new file +2245 -0
2,245 lines not on this page · raw message
drivers/scsi/mvumi.h new file +610 -0
610 lines not on this page · raw message
include/linux/pci_ids.h +2 -0
@@ -1569,10 +1569,12 @@#define PCI_SUBDEVICE_ID_KEYSPAN_SX2 0x5334#define PCI_VENDOR_ID_MARVELL 0x11ab +#define PCI_VENDOR_ID_MARVELL_2 0x1b4b#define PCI_DEVICE_ID_MARVELL_GT64111 0x4146#define PCI_DEVICE_ID_MARVELL_GT64260 0x6430#define PCI_DEVICE_ID_MARVELL_MV64360 0x6460#define PCI_DEVICE_ID_MARVELL_MV64460 0x6480 +#define PCI_DEVICE_ID_MARVELL_MV9143 0x9143#define PCI_DEVICE_ID_MARVELL_88ALP01_NAND 0x4100#define PCI_DEVICE_ID_MARVELL_88ALP01_SD 0x4101#define PCI_DEVICE_ID_MARVELL_88ALP01_CCIC 0x4102
--
1.7.1
Quoted 11 lines · Li Jianyun
The Marvell Universal Message Interface (UMI) defines a messaging
interface between host and Marvell products. It considers situations of limited system resource and optimized system performance.
UMI driver translates host request to message and sends message to
FW via UMI, FW receives message and processes it, then sends response to UMI driver.
FW generates an interrupt when it needs to send information or
responseto UMI driver.Signed-off-by Jianyun Li <[email protected]>
Why not sign off using your marvell.com email address?
Quoted 13 lines
+++
b/drivers/scsi/mvumi.cdrivers/scsi/mvumi.c new file +11
-0@@ -0,0 +1,2245 @@ +/* +* Marvell UMI driver +* +* Copyright 2011 Marvell. <[email protected]> +* +* This file is licensed under GPLv2. +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License as +* published by the Free Software Foundation; version 2 of the +* License.
drivers/scsi/mvumi.c line 21
Quoted 10 lines
drivers/scsi/mvumi.c new file +10
-0+* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +* General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 +* USA
Unless you are willing to track the office location of the FSF for the next 40 years, and send patches updating it here, just drop this paragraph, it's pointless, as the person _did_ get a copy with the kernel.
thanks,
greg k-h
Hi Grep KH
thanks for your remind,I am a beginner，and don't know how to
send patch to kernel.
I will update the Signed-off-by. Do I need update the subject to "[PATCH][RESEND] Add Marvell UMI driver" when I resend this patch?
thanks for your patient.
2011/4/20 Greg KH <[email protected]>:
Quoted 44 lines · Li Jianyun
Quoted 11 lines
The Marvell Universal Message Interface (UMI) defines a messaging
interface between host and Marvell products. It considers situations of limited system resource and optimized system performance.
UMI driver translates host request to message and sends message to
FW via UMI, FW receives message and processes it, then sends response to UMI driver.
FW generates an interrupt when it needs to send information or
responseto UMI driver.Signed-off-by Jianyun Li <[email protected]>Why not sign off using your marvell.com email address?
Quoted 13 lines
+++
b/drivers/scsi/mvumi.cdrivers/scsi/mvumi.c new file +11
-0@@ -0,0 +1,2245 @@ +/* +* Marvell UMI driver +* +* Copyright 2011 Marvell. <[email protected]> +* +* This file is licensed under GPLv2. +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License as +* published by the Free Software Foundation; version 2 of the +* License. drivers/scsi/mvumi.c line 21
Quoted 10 lines
drivers/scsi/mvumi.c new file +10
-0+* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +* General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 +* USA Unless you are willing to track the office location of the FSF for the next 40 years, and send patches updating it here, just drop this paragraph, it's pointless, as the person _did_ get a copy with the kernel.
Quoted 4 lines
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 6 lines · li jianyun
Hi Grep KH
thanks for your remind,I am a beginner，and don't know how to
send patch to kernel.
I will update the Signed-off-by. Do I need update the subject to "[PATCH][RESEND] Add Marvell UMI driver" when I resend this patch?
thanks for your patient.
No, just do:
[patch v2]
as git will strip out the first set of [] markings, but not the second when applying the patch.
thanks,
greg k-h
Quoted 1 line
+#include <
asm/uaccess.h>
linux/uaccess.h
[scripts/checkpatch.pl --file whatever is your friend]
drivers/scsi/mvumi.c line 128
Quoted 13 lines
drivers/scsi/mvumi.c new file +13
-0+static int mvumi_map_pci_addr (struct pci_dev *dev,void **addr_array) +{ +int i; +unsigned long addr, range; + +for (i =0 ; i <MAX_BASE_ADDRESS ; i++) { + addr =pci_resource_start (dev, i); + range =pci_resource_len (dev, i); + +if (pci_resource_flags (dev, i) &IORESOURCE_MEM ) { + addr_array[i] = (void *)ioremap_nocache (addr, range); +if (!addr_array[i]) { + dev_printk(KERN_ERR, &dev->dev,
dev_err is a bit easier
drivers/scsi/mvumi.c line 139
Quoted 11 lines
drivers/scsi/mvumi.c new file +11
-0+ "Failed to map IO mem\n"); +return -1 ; + } + }else + addr_array[i] = (void *) addr; + +dev_printk (KERN_INFO , &dev->dev, +"BAR %d : %p.\n" , i, addr_array[i]); + } +return 0 ; +}
See pci_iomap/ioread32/iowrite32/etc you appear to be reinventing the wheel.
drivers/scsi/mvumi.c line 154
Quoted 5 lines
drivers/scsi/mvumi.c new file +5
-0+static struct mvumi_res_mgnt *mvumi_alloc_mem_resource (struct mvumi_hba *mhba, +enum resource_type type,unsigned int size) +{ +struct mvumi_res_mgnt *res_mgnt = +vmalloc (sizeof (struct mvumi_res_mgnt ));
Unless it's a very large object (> 2 pages) use kmalloc - on 32bit machines kmalloc resources are much more freely available
This whole abstraction appears to be overkill though. You know what sort of resource you allocated anyway so this seems to be excessive and confusing
drivers/scsi/mvumi.c line 334
Quoted 12 lines
drivers/scsi/mvumi.c new file +12
-0+static struct mvumi_cmd *mvumi_create_internal_cmd (struct mvumi_hba *mhba, +unsigned int buf_size) +{ +struct mvumi_cmd *cmd; + + cmd =kmalloc (sizeof (struct mvumi_cmd ),GFP_KERNEL ); +if (cmd ==NULL ) { +dev_printk (KERN_ERR , &mhba->pdev->dev, +"failed to create a internal cmd\n" ); +return NULL ; + } +memset (cmd,0 ,sizeof (struct mvumi_cmd ));
kzalloc
drivers/scsi/mvumi.c line 560
Quoted 4 lines
drivers/scsi/mvumi.c new file +4
-0+static unsigned char mvumi_send_ib_list_entry (struct mvumi_hba *mhba) +{ +void *regs = mhba->mmio; +writel ((unsigned int )0xfff , mhba->ib_shadow);
Don't think you need some of these casts either
drivers/scsi/mvumi.c line 1100
Quoted 1 line
drivers/scsi/mvumi.c new file +1
-0+dev_printk (KERN_INFO , &mhba->pdev->dev,"start firmware handshake.\n" );
dev_info etc as well as dev_err
drivers/scsi/mvumi.c line 1139
Quoted 9 lines
drivers/scsi/mvumi.c new file +9
-0+dev_printk (KERN_INFO , &mhba->pdev->dev,"CLA_INB_LIST_BASEL=0x%x.\n" , +mvumi_mr32 (CLA_INB_LIST_BASEL )); +dev_printk (KERN_INFO , &mhba->pdev->dev,"CLA_INB_LIST_BASEH=0x%x.\n" , +mvumi_mr32 (CLA_INB_LIST_BASEH )); +dev_printk (KERN_INFO , &mhba->pdev->dev,"CLA_OUTB_LIST_BASEL=0x%x.\n" , +mvumi_mr32 (CLA_OUTB_LIST_BASEL )); +dev_printk (KERN_INFO , &mhba->pdev->dev,"CLA_OUTB_LIST_BASEH=0x%x.\n" , +mvumi_mr32 (CLA_OUTB_LIST_BASEH )); +dev_printk (KERN_INFO , &mhba->pdev->dev,"firmware handshake ok.\n" );
A lot of this looks like it should be dev_dbg()
drivers/scsi/mvumi.c line 1552
Quoted 10 lines
drivers/scsi/mvumi.c new file +10
-0+static void dwordcpy (void *to,const void *from,size_t n) +{ +unsigned int *p_dst = (unsigned int *) to; +unsigned int *p_src = (unsigned int *) from; + +WARN_ON (n &0x3 ); + n >>=2 ; +while (n--) + *p_dst++ = *p_src++; +}
Why not use memcpy ?
drivers/scsi/mvumi.c line 1554
Quoted 2 lines
drivers/scsi/mvumi.c new file +2
-0+static enum MV_QUEUE_COMMAND_RESULT mvumi_send_command (struct mvumi_hba *mhba, +struct mvumi_cmd *cmd)
Please dont use CAPITAL_LETTER_SIZED_GIANT_TYPE_NAMING, Linux reserves shouting for defines in general
drivers/scsi/mvumi.c line 2015
Quoted 1 line
drivers/scsi/mvumi.c new file +1
-0+pci_read_config_word (pdev,0x0A , &class_code);
The class is available in the pci_dev already is it not ?
drivers/scsi/mvumi.c line 2023
Quoted 8 lines
drivers/scsi/mvumi.c new file +8
-0+dev_printk (KERN_INFO , &pdev->dev, +" %#4.04x:%#4.04x:%#4.04x:%#4.04x: " , + pdev->vendor, pdev->device, pdev->subsystem_vendor, + pdev->subsystem_device); +dev_printk (KERN_INFO , &pdev->dev, +"bus %d:slot %d:func %d:class_code:%#4.04x\n" , + pdev->bus->number,PCI_SLOT (pdev->devfn), +PCI_FUNC (pdev->devfn), class_code);
Like most of the printk stuff this should be debug level
drivers/scsi/mvumi.h line 214
Quoted 6 lines
drivers/scsi/mvumi.h new file +6
-0+struct mv_sgl { +unsigned int baseaddr_l; +unsigned int baseaddr_h; +unsigned int flags; +unsigned int size; +};
In general if you want these to be a given size (ie they are shared) use u32/u64/u8/u16 etc and make it explicit.
drivers/scsi/mvumi.h line 227
Quoted 13 lines
drivers/scsi/mvumi.h new file +13
-0+ +#define sgd_getsz (sgd, sz) do { \ + (sz) = (sgd)->size; \ +} while (0) + +#define sgd_setsz (sgd, sz) do { \ + (sgd)->size = (sz); \ +} while (0) + + +#define sgd_inc (sgd) do { \ + sgd = (struct mv_sgl *) (((unsigned char *) (sgd)) + 16);\ +} while (0)
These really shouldn't be magic macros, and all the casting actually makes them messier than just using C pointer increment rules like normal drivers.
If struct foo is 16 bytes then ++ moves a struct foo * on to the next struct so you don't need all the gunge anyway
Quoted 1 line
+static inline struct
list_head*list_get_first(structlist_head*head)
I'm not sure hiding your only subtly different list handling in a header is going to help everyone understand the code either !
drivers/scsi/mvumi.h
Quoted 8 lines
drivers/scsi/mvumi.h new file +7
-0+#define list_get_first_entry (head, type, member) \ + list_entry(list_get_first(head), type, member) + +#define list_get_last_entry (head, type, member) \ + list_entry(list_get_last(head), type, member) + +extern struct timezone sys_tz;
Umm why ???
Quoted 20 lines · Li Jianyun
drivers/scsi/mvumi.c new file +20
-0+/** +* mvumi_init_data - Initialize requested date for FW +* @mhba: Adapter soft state +*/ +static int mvumi_init_data (struct mvumi_hba *mhba) +{ +struct mv_ob_data_pool *ob_pool; +struct mvumi_res_mgnt *res_mgnt; +unsigned int tmp_size, offset, i; +void *virmem, *v; +dma_addr_t p; + +if (mhba->fw_flag &MVUMI_FW_ALLOC ) +return 0 ; + tmp_size =128 + mhba->ib_max_entry_size_bytes * mhba->max_io +16 + +sizeof (unsigned int ); + tmp_size +=8 + mhba->ob_max_entry_size_bytes * mhba->max_io +4 ; + res_mgnt =mvumi_alloc_mem_resource (mhba, +RESOURCE_UNCACHED_MEMORY , tmp_size); +if (!res_mgnt) {
drivers/scsi/mvumi.c line 1459
Quoted 10 lines
drivers/scsi/mvumi.c new file +10
-0+dev_printk (KERN_ERR , &mhba->pdev->dev, +"failed to allocate memory for inbound list\n" ); + goto fail_alloc_dma_buf; + } + + p = res_mgnt->bus_addr; + v = res_mgnt->virt_addr; + offset = (unsigned int )(round_up (res_mgnt->bus_addr,128 ) - + res_mgnt->bus_addr); + p += offset;
p now has offset again, no? Is this intentional?
Review
Quoted 1 line
(quoted diff) +1
-0+ v = (unsigned char *) v + offset;
I don't think you need the cast. The kernel relies on the gcc extension that "void* += n" moves up n bytes.
Quoted 13 lines
drivers/scsi/mvumi.c new file +13
-0+ mhba->ib_list = v; + mhba->ib_list_phys = p; + + v = (unsigned char *) v + mhba->ib_max_entry_size_bytes * mhba->max_io; + p += mhba->ib_max_entry_size_bytes * mhba->max_io; + + offset = (unsigned int )(round_up (p,8 ) - p); + p += offset; + v = (unsigned char *) v + offset; + mhba->ib_shadow = v; + mhba->ib_shadow_phys = p; + + p +=sizeof (unsigned int );
drivers/scsi/mvumi.c line 1483
Quoted 10 lines
drivers/scsi/mvumi.c new file +10
-0+ v = (unsigned char *) v +sizeof (unsigned int ); + + offset =round_up (p,8 ) - p; + p += offset; + v = (unsigned char *) v + offset; + mhba->ob_shadow = v; + mhba->ob_shadow_phys = p; + p +=8 ; + v = (unsigned char *) v +8 ; + offset = (unsigned int ) (round_up (p,128 ) - p);
Sometimes you cast the result of round_up() to unsigned int and sometimes not. So I guess you just need to never cast. Since the result will be in range 8..128 anyway I think the casting can just go away completely.
drivers/scsi/mvumi.c line 384
Quoted 4 lines
drivers/scsi/mvumi.c new file +4
-0+ +return 0 ; +} +/**
Newline missing here.
Quoted 13 lines
+ *
mvumi_probe_one- PCI hotplug entry point
+ * @pdev: PCI device structure
+ * @id: PCI ids of supported hotplugged adapter
+ */drivers/scsi/mvumi.c new file +9
-0+static int __devinit mvumi_probe_one (struct pci_dev *pdev, +const struct pci_device_id *id) +{ +int ret; +struct Scsi_Host *host; +struct mvumi_hba *mhba; +unsigned short class_code =0xFFFF ; + +pci_read_config_word (pdev,0x0A , &class_code);
drivers/scsi/mvumi.c line 2025
Quoted 10 lines
drivers/scsi/mvumi.c new file +10
-0+dev_printk (KERN_INFO , &pdev->dev, +" %#4.04x:%#4.04x:%#4.04x:%#4.04x: " , + pdev->vendor, pdev->device, pdev->subsystem_vendor, + pdev->subsystem_device); +dev_printk (KERN_INFO , &pdev->dev, +"bus %d:slot %d:func %d:class_code:%#4.04x\n" , + pdev->bus->number,PCI_SLOT (pdev->devfn), +PCI_FUNC (pdev->devfn), class_code); + + ret =pci_enable_device (pdev);
You may want to use pcim_enable_device(). Look at Documentation/driver- model/devres.txt. This would take care of some of your error handling for free.
Quoted 4 lines
drivers/scsi/mvumi.c new file +4
-0+if (ret) +return ret; + +pci_set_master (pdev);
drivers/scsi/mvumi.c line 2039
Quoted 10 lines
drivers/scsi/mvumi.c new file +10
-0+ +if (IS_DMA64 ) { +if (pci_set_dma_mask (pdev,DMA_BIT_MASK (64 )) !=0 ) { +if (pci_set_dma_mask (pdev,DMA_BIT_MASK (32 )) !=0 ) + goto fail_set_dma_mask; + } + }else { +if (pci_set_dma_mask (pdev,DMA_BIT_MASK (32 )) !=0 ) + goto fail_set_dma_mask; + }
I don't like that you lose the error code of pci_set_dma_mask() here.
drivers/scsi/mvumi.c line 2046
Quoted 6 lines
drivers/scsi/mvumi.c new file +6
-0+ host =scsi_host_alloc (&mvumi_template,sizeof (struct mvumi_hba )); +if (!host) { +dev_printk (KERN_ERR , &pdev->dev,"scsi_host_alloc failed\n" ); + goto fail_alloc_instance; + } + mhba = (struct mvumi_hba *) host->hostdata;
mhba = shost_priv(host);
Quoted 10 lines
drivers/scsi/mvumi.c new file +10
-0+memset (mhba,0 ,sizeof (*mhba)); + +INIT_LIST_HEAD (&mhba->cmd_pool); +INIT_LIST_HEAD (&mhba->ob_data_pool_list); +INIT_LIST_HEAD (&mhba->free_ob_list); +INIT_LIST_HEAD (&mhba->res_list); +INIT_LIST_HEAD (&mhba->waiting_req_list); +atomic_set (&mhba->fw_outstanding,0 ); + +init_waitqueue_head (&mhba->int_cmd_wait_q);
drivers/scsi/mvumi.c line 2066
Quoted 10 lines
drivers/scsi/mvumi.c new file +10
-0+ +spin_lock_init (&mhba->cmd_pool_lock); +spin_lock_init (&mhba->tag_lock); + + mhba->pdev = pdev; + mhba->shost = host; + mhba->unique_id = pdev->bus->number <<8 | pdev->devfn; + +if (mvumi_init_fw (mhba)) + goto fail_init_fw;
You are once again loosing an error code here.
Quoted 4 lines
drivers/scsi/mvumi.c new file +4
-0+ ret =request_irq (mhba->pdev->irq, mvumi_isr_handler,IRQF_SHARED , +"mvumi" , mhba); +if (ret) { +dev_printk (KERN_ERR , &pdev->dev,"failed to register IRQ\n" );
drivers/scsi/mvumi.c line 2080
Quoted 10 lines
drivers/scsi/mvumi.c new file +10
-0+ goto fail_init_irq; + } + + mhba->instancet->enable_intr (mhba->mmio); +pci_set_drvdata (pdev, mhba); + +if (mvumi_io_attach (mhba)) + goto fail_io_attach; +dev_printk (KERN_INFO , &pdev->dev,"probe mvumi driver successfully.\n" ); +return 0 ;
I don't know if the ordering is correct here. Once you have registered the IRQ handler you must be prepared to get interrupts. In fact with IRQ debugging enabled your IRQ handler will be called immediately in request_irq(). So when you set up other stuff here that is needed in the IRQ handler this can do all sort of things wrong.
Quoted 5 lines
drivers/scsi/mvumi.c new file +5
-0+fail_io_attach: +pci_set_drvdata (pdev,NULL ); + mhba->instancet->disable_intr (mhba->mmio); +free_irq (mhba->pdev->irq, mhba); +fail_init_irq:
drivers/scsi/mvumi.c line 2096
Quoted 10 lines
drivers/scsi/mvumi.c new file +10
-0+mvumi_release_fw (mhba); +fail_init_fw: +scsi_host_put (host); + +fail_alloc_instance: +fail_set_dma_mask: +pci_disable_device (pdev); + +return -ENODEV ; +}
You always lose the error codes of anything in your probe function. I don't think this is a good idea.
drivers/scsi/mvumi.c line 2109
Quoted 13 lines
drivers/scsi/mvumi.c new file +13
-0+ +static void mvumi_detach_one (struct pci_dev *pdev) +{ +struct Scsi_Host *host; +struct mvumi_hba *mhba; + + mhba =pci_get_drvdata (pdev); + host = mhba->shost; +scsi_remove_host (mhba->shost); +mvumi_flush_cache (mhba); + + mhba->instancet->disable_intr (mhba->mmio); +free_irq (mhba->pdev->irq, mhba);
Same as above here: you may still get an IRQ until this point. So be careful with what you free first.
drivers/scsi/mvumi.c line 2115
Quoted 6 lines
drivers/scsi/mvumi.c new file +6
-0+mvumi_release_fw (mhba); +scsi_host_put (host); +pci_set_drvdata (pdev,NULL ); +pci_disable_device (pdev); +dev_printk (KERN_INFO , &pdev->dev,"driver is removed!\n" ); +}
Greetings,
Eike