Re: [PATCH] Marvell 6440 SAS/SATA driver (draft)

2008-01-20 Thread Ke Wei
Hello James:


   I read your comments.

.target_alloc   = sas_target_alloc,
.slave_configure= sas_slave_configure,
.slave_destroy  = sas_slave_destroy,
.change_queue_depth = sas_change_queue_depth,
.change_queue_type  = sas_change_queue_type,
.bios_param = sas_bios_param,
  - .can_queue  = 1,
  + .can_queue  = 30,

 I don't think you want to do this.  It starts sending multiple commands
 at once.  The design use of libsas is to start off at 1 and then up the
 limit in slave configure once we know what type of device we're dealing
 with.  The current sas_slave_configure has a limitation in that the
 depth is hard coded to 32, so if you need it smaller, we'll have to find
 a way of allowing this?  Is 30 your max queue depth?

Yes , I may not do this. But I need to set
sas_ha_struct.lldd_queue_size to suitable value.
Because sas_queue sends multiple commands according to lldd_queue_size
before calling lldd_execute_task function which supports queue depth ,
30 is suitable.


.cmd_per_lun= 1,
.this_id= -1,
  - .sg_tablesize   = SG_ALL,
  - .max_sectors= SCSI_DEFAULT_MAX_SECTORS,
  - .use_clustering = ENABLE_CLUSTERING,
  + .sg_tablesize   = 32,

 32 looks a little small.  My reading of the code is that the PRD table
 has to fit with the command header, OAF area and status area into an 8k
 segment of memory, so at 16 bytes per PRD entry, it looks like a page
 worth at least (256) isn't unreasonable.  You should probably have some
 type of macro for this though.

  + .max_sectors= (128*1024)9,

Yes , It's demo code . And I need to test device to find a good value
according to performance and reliability.


  + .use_clustering = DISABLE_CLUSTERING,

 I think this is wrong ... there should be no modern device that disables
 clustering (otherwise they fall over badly on iommu platforms).

.eh_device_reset_handler= sas_eh_device_reset_handler,
.eh_bus_reset_handler   = sas_eh_bus_reset_handler,
  - .slave_alloc= sas_slave_alloc,

 Please don't remove this ... it's a standard callback, and it's required
 for the day you support SATA.

You are right. I forgot to recover these codes when I debuged.
And Driver has support for SATA devices . I will commit the patches in
this weeks.

Thank you for your help.

Ke Wei.

On Jan 19, 2008 2:53 AM, James Bottomley
[EMAIL PROTECTED] wrote:
 On Thu, 2008-01-10 at 14:53 +0800, Ke Wei wrote:
  The 88SE6440 driver :
 
  The driver is based on bare code from Jeff Garzik. And it can work
  under linux kernel 2.6.23.
  By far, Can discover and find SAS HDD, but SATA is currently
  unsupported. Command queue depth can be above 1.
  Most error handling, and some phy handling code is notably missing.
 
 
  contains the following updates:
 
  --- mvsas_orig.c  2007-12-06 19:21:32.0 -0500
  +++ mvsas.c   2008-01-09 04:53:14.0 -0500
 [...]
  +#define MVS_BIT(x) (1L  (x))
  +
  +#define PORT_TYPE_SATAMVS_BIT(0)
  +#define PORT_TYPE_SAS MVS_BIT(1)
  +
  +#define READ_PORT_CONFIG_DATA(i) \
  + ((i3)?mr32(P4_CFG_DATA+(i-4)*8):mr32(P0_CFG_DATA+i*8))
  +#define WRITE_PORT_CONFIG_DATA(i,tmp) \
  + {if(i3)mw32(P4_CFG_DATA+(i-4)*8,tmp);else mw32(P0_CFG_DATA+i*8,tmp);}
  +#define WRITE_PORT_CONFIG_ADDR(i,tmp) \
  + {if(i3)mw32(P4_CFG_ADDR+(i-4)*8,tmp);else mw32(P0_CFG_ADDR+i*8,tmp);}
  +
  +#define READ_PORT_PHY_CONTROL(i) \
  +   ((i3)?mr32(P4_SER_CTLSTAT+(i-4)*4):mr32(P0_SER_CTLSTAT+i*4))
  +#define WRITE_PORT_PHY_CONTROL(i,tmp) \
  +   {if(i3)mw32(P4_SER_CTLSTAT+(i-4)*4,tmp);else
  mw32(P0_SER_CTLSTAT+i*4,tmp);}

 This is an example of where you mailer has broken a line (which causes
 the patch not to apply).


  +
  +#define READ_PORT_VSR_DATA(i) \
  +   ((i3)?mr32(P4_VSR_DATA+(i-4)*8):mr32(P0_VSR_DATA+i*8))
  +#define WRITE_PORT_VSR_DATA(i,tmp) \
  +   {if(i3)mw32(P4_VSR_DATA+(i-4)*8,tmp);else mw32(P0_VSR_DATA+i*8,tmp);}
  +#define WRITE_PORT_VSR_ADDR(i,tmp) \
  +   {if(i3)mw32(P4_VSR_ADDR+(i-4)*8,tmp);else mw32(P0_VSR_ADDR+i*8,tmp);}
  +
  +#define READ_PORT_IRQ_STAT(i) \
  +   ((i3)?mr32(P4_INT_STAT+(i-4)*8):mr32(P0_INT_STAT+i*8))
  +#define WRITE_PORT_IRQ_STAT(i,tmp) \
  +   {if(i3)mw32(P4_INT_STAT+(i-4)*8,tmp);else mw32(P0_INT_STAT+i*8,tmp);}
  +#define READ_PORT_IRQ_MASK(i) \
  +   ((i3)?mr32(P4_INT_MASK+(i-4)*8):mr32(P0_INT_MASK+i*8))
  +#define WRITE_PORT_IRQ_MASK(i,tmp) \
  +   {if(i3)mw32(P4_INT_MASK+(i-4)*8,tmp);else mw32(P0_INT_MASK+i*8,tmp);}
  +
   /* driver compile-time configuration */
   enum driver_configuration {
  - MVS_TX_RING_SZ  = 1024, /* TX ring size (12-bit) */
  + MVS_TX_RING_SZ  = 512,  /* TX ring size (12-bit) */
MVS_RX_RING_SZ  = 1024, /* RX ring size 

Re: [PATCH] Marvell 6440 SAS/SATA driver (draft)

2008-01-18 Thread James Bottomley

On Thu, 2008-01-10 at 02:21 -0500, Jeff Garzik wrote:
 Jeff Garzik wrote:
  1) To make it easier for people to review and test the driver, I would 
  suggest posting a diff against 2.6.24-rc7 (or 2.6.23), ignoring my 
  original code.  Thus, it would result in a patch
 
 Er, that sentence was incomplete.  Continuing...
 
 
 Thus it would result in a patch that adds a new file 
 drivers/scsi/mvsas.c to the 2.6.24-rc7 kernel, and modifies 
 drivers/scsi/Makefile and drivers/scsi/Kconfig to enable this new driver.
 
 That is the format that developers and users are most familiar with, 
 when reviewing (and testing) a new driver.
 
 But of course this is a draft, so these guidelines are certainly loose...

OK, in order to try to expedite this, I've created a mvsas branch in

git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi-misc-2.6.git

I think the first patch (the infrastructure change) is safe to go in
immediately.  Unfortunately, I can't put the marvell update in because
the emailed patch is corrupt (it looks like the mailer has added line
breaks).

Ke, If you can't get your email tool to insert plain text (as a lot of
microsoft based one's can't), you can add the patch as an attachment; I
can apply it from that (Although in line plain text is by far the
preferred method for review if you can do it, we have a bunch of other
driver writers who have problematic email tools, so we're reasonably
used to this).

Thanks,

James


-
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


Re: [PATCH] Marvell 6440 SAS/SATA driver (draft)

2008-01-18 Thread James Bottomley
On Thu, 2008-01-10 at 14:53 +0800, Ke Wei wrote:
 The 88SE6440 driver :
 
 The driver is based on bare code from Jeff Garzik. And it can work
 under linux kernel 2.6.23.
 By far, Can discover and find SAS HDD, but SATA is currently
 unsupported. Command queue depth can be above 1.
 Most error handling, and some phy handling code is notably missing.
 
 
 contains the following updates:
 
 --- mvsas_orig.c  2007-12-06 19:21:32.0 -0500
 +++ mvsas.c   2008-01-09 04:53:14.0 -0500
[...]
 +#define MVS_BIT(x) (1L  (x))
 +
 +#define PORT_TYPE_SATAMVS_BIT(0)
 +#define PORT_TYPE_SAS MVS_BIT(1)
 +
 +#define READ_PORT_CONFIG_DATA(i) \
 + ((i3)?mr32(P4_CFG_DATA+(i-4)*8):mr32(P0_CFG_DATA+i*8))
 +#define WRITE_PORT_CONFIG_DATA(i,tmp) \
 + {if(i3)mw32(P4_CFG_DATA+(i-4)*8,tmp);else mw32(P0_CFG_DATA+i*8,tmp);}
 +#define WRITE_PORT_CONFIG_ADDR(i,tmp) \
 + {if(i3)mw32(P4_CFG_ADDR+(i-4)*8,tmp);else mw32(P0_CFG_ADDR+i*8,tmp);}
 +
 +#define READ_PORT_PHY_CONTROL(i) \
 +   ((i3)?mr32(P4_SER_CTLSTAT+(i-4)*4):mr32(P0_SER_CTLSTAT+i*4))
 +#define WRITE_PORT_PHY_CONTROL(i,tmp) \
 +   {if(i3)mw32(P4_SER_CTLSTAT+(i-4)*4,tmp);else
 mw32(P0_SER_CTLSTAT+i*4,tmp);}

This is an example of where you mailer has broken a line (which causes
the patch not to apply).

 +
 +#define READ_PORT_VSR_DATA(i) \
 +   ((i3)?mr32(P4_VSR_DATA+(i-4)*8):mr32(P0_VSR_DATA+i*8))
 +#define WRITE_PORT_VSR_DATA(i,tmp) \
 +   {if(i3)mw32(P4_VSR_DATA+(i-4)*8,tmp);else mw32(P0_VSR_DATA+i*8,tmp);}
 +#define WRITE_PORT_VSR_ADDR(i,tmp) \
 +   {if(i3)mw32(P4_VSR_ADDR+(i-4)*8,tmp);else mw32(P0_VSR_ADDR+i*8,tmp);}
 +
 +#define READ_PORT_IRQ_STAT(i) \
 +   ((i3)?mr32(P4_INT_STAT+(i-4)*8):mr32(P0_INT_STAT+i*8))
 +#define WRITE_PORT_IRQ_STAT(i,tmp) \
 +   {if(i3)mw32(P4_INT_STAT+(i-4)*8,tmp);else mw32(P0_INT_STAT+i*8,tmp);}
 +#define READ_PORT_IRQ_MASK(i) \
 +   ((i3)?mr32(P4_INT_MASK+(i-4)*8):mr32(P0_INT_MASK+i*8))
 +#define WRITE_PORT_IRQ_MASK(i,tmp) \
 +   {if(i3)mw32(P4_INT_MASK+(i-4)*8,tmp);else mw32(P0_INT_MASK+i*8,tmp);}
 +
  /* driver compile-time configuration */
  enum driver_configuration {
 - MVS_TX_RING_SZ  = 1024, /* TX ring size (12-bit) */
 + MVS_TX_RING_SZ  = 512,  /* TX ring size (12-bit) */
   MVS_RX_RING_SZ  = 1024, /* RX ring size (12-bit) */
   /* software requires power-of-2
  ring size */
 @@ -89,7 +125,7 @@
   MVS_GBL_CTL = 0x04,  /* global control */
   MVS_GBL_INT_STAT= 0x08,  /* global irq status */
   MVS_GBL_PI  = 0x0C,  /* ports implemented bitmask */
 - MVS_GBL_PORT_TYPE   = 0x00,  /* port type */
 + MVS_GBL_PORT_TYPE   = 0xa0,  /* port type */
 
   MVS_CTL = 0x100, /* SAS/SATA port configuration */
   MVS_PCS = 0x104, /* SAS/SATA port control/status */
 @@ -102,11 +138,12 @@
   MVS_TX_LO   = 0x124, /* TX (delivery) ring addr */
   MVS_TX_HI   = 0x128,
 
 - MVS_RX_PROD_IDX = 0x12C, /* RX producer pointer */
 - MVS_RX_CONS_IDX = 0x130, /* RX consumer pointer (RO) */
 + MVS_TX_PROD_IDX = 0x12C, /* TX producer pointer */
 + MVS_TX_CONS_IDX = 0x130, /* TX consumer pointer (RO) */
   MVS_RX_CFG  = 0x134, /* RX configuration */
   MVS_RX_LO   = 0x138, /* RX (completion) ring addr */
   MVS_RX_HI   = 0x13C,
 + MVS_RX_CONS_IDX = 0x140, /* RX consumer pointer (RO) */ 
 
   MVS_INT_COAL= 0x148, /* Int coalescing config */
   MVS_INT_COAL_TMOUT  = 0x14C, /* Int coalescing timeout */
 @@ -117,9 +154,12 @@
/* ports 1-3 follow after this */
   MVS_P0_INT_STAT = 0x160, /* port0 interrupt status */
   MVS_P0_INT_MASK = 0x164, /* port0 interrupt mask */
 + MVS_P4_INT_STAT = 0x200, /* Port 4 interrupt status */
 + MVS_P4_INT_MASK = 0x204, /* Port 4 interrupt enable/disable 
 mask */
 
/* ports 1-3 follow after this */
   MVS_P0_SER_CTLSTAT  = 0x180, /* port0 serial control/status */
 + MVS_P4_SER_CTLSTAT  = 0x220, /* port4 serial control/status */
 
   MVS_CMD_ADDR= 0x1B8, /* Command register port (addr) */
   MVS_CMD_DATA= 0x1BC, /* Command register port (data) */
 @@ -127,6 +167,14 @@
/* ports 1-3 follow after this */
   MVS_P0_CFG_ADDR = 0x1C0, /* port0 phy register address */
   MVS_P0_CFG_DATA = 0x1C4, /* port0 phy register data */
 + MVS_P4_CFG_ADDR = 0x230, /* Port 4 config address */
 + MVS_P4_CFG_DATA = 0x234, /* Port 4 config data */   
 + 
 +  /* ports 1-3 follow after this */
 + 

[PATCH] Marvell 6440 SAS/SATA driver (draft)

2008-01-09 Thread Ke Wei
The 88SE6440 driver :

The driver is based on bare code from Jeff Garzik. And it can work
under linux kernel 2.6.23.
By far, Can discover and find SAS HDD, but SATA is currently
unsupported. Command queue depth can be above 1.
Most error handling, and some phy handling code is notably missing.


contains the following updates:

--- mvsas_orig.c2007-12-06 19:21:32.0 -0500
+++ mvsas.c 2008-01-09 04:53:14.0 -0500
@@ -2,6 +2,7 @@
mvsas.c - Marvell 88SE6440 SAS/SATA support

Copyright 2007 Red Hat, Inc.
+   Copyright 2008 Marvell.

This program is free software; you can redistribute it and/or
modify it under the terms of the GNU General Public License as
@@ -37,8 +38,10 @@
 #include scsi/libsas.h
 #include asm/io.h

-#define DRV_NAME mvsas
-#define DRV_VERSION 0.1
+#define DRV_NAME   mvsas
+#define DRV_VERSION0.2
+#define _MV_DUMP 0
+#define MVS_PRINTK(_x_,...)printk(KERN_NOTICE DRV_NAME :  _x_ , ##
__VA_ARGS__)

 #define mr32(reg)  readl(regs + MVS_##reg)
 #define mw32(reg,val)  writel((val), regs + MVS_##reg)
@@ -47,9 +50,42 @@
readl(regs + MVS_##reg);\
} while (0)

+#define MVS_BIT(x) (1L  (x))
+
+#define PORT_TYPE_SATAMVS_BIT(0)
+#define PORT_TYPE_SAS MVS_BIT(1)
+
+#define READ_PORT_CONFIG_DATA(i) \
+   ((i3)?mr32(P4_CFG_DATA+(i-4)*8):mr32(P0_CFG_DATA+i*8))
+#define WRITE_PORT_CONFIG_DATA(i,tmp) \
+   {if(i3)mw32(P4_CFG_DATA+(i-4)*8,tmp);else mw32(P0_CFG_DATA+i*8,tmp);}
+#define WRITE_PORT_CONFIG_ADDR(i,tmp) \
+   {if(i3)mw32(P4_CFG_ADDR+(i-4)*8,tmp);else mw32(P0_CFG_ADDR+i*8,tmp);}
+
+#define READ_PORT_PHY_CONTROL(i) \
+   ((i3)?mr32(P4_SER_CTLSTAT+(i-4)*4):mr32(P0_SER_CTLSTAT+i*4))
+#define WRITE_PORT_PHY_CONTROL(i,tmp) \
+   {if(i3)mw32(P4_SER_CTLSTAT+(i-4)*4,tmp);else
mw32(P0_SER_CTLSTAT+i*4,tmp);}
+
+#define READ_PORT_VSR_DATA(i) \
+   ((i3)?mr32(P4_VSR_DATA+(i-4)*8):mr32(P0_VSR_DATA+i*8))
+#define WRITE_PORT_VSR_DATA(i,tmp) \
+   {if(i3)mw32(P4_VSR_DATA+(i-4)*8,tmp);else mw32(P0_VSR_DATA+i*8,tmp);}
+#define WRITE_PORT_VSR_ADDR(i,tmp) \
+   {if(i3)mw32(P4_VSR_ADDR+(i-4)*8,tmp);else mw32(P0_VSR_ADDR+i*8,tmp);}
+
+#define READ_PORT_IRQ_STAT(i) \
+   ((i3)?mr32(P4_INT_STAT+(i-4)*8):mr32(P0_INT_STAT+i*8))
+#define WRITE_PORT_IRQ_STAT(i,tmp) \
+   {if(i3)mw32(P4_INT_STAT+(i-4)*8,tmp);else mw32(P0_INT_STAT+i*8,tmp);}
+#define READ_PORT_IRQ_MASK(i) \
+   ((i3)?mr32(P4_INT_MASK+(i-4)*8):mr32(P0_INT_MASK+i*8))
+#define WRITE_PORT_IRQ_MASK(i,tmp) \
+   {if(i3)mw32(P4_INT_MASK+(i-4)*8,tmp);else mw32(P0_INT_MASK+i*8,tmp);}
+
 /* driver compile-time configuration */
 enum driver_configuration {
-   MVS_TX_RING_SZ  = 1024, /* TX ring size (12-bit) */
+   MVS_TX_RING_SZ  = 512,  /* TX ring size (12-bit) */
MVS_RX_RING_SZ  = 1024, /* RX ring size (12-bit) */
/* software requires power-of-2
   ring size */
@@ -89,7 +125,7 @@
MVS_GBL_CTL = 0x04,  /* global control */
MVS_GBL_INT_STAT= 0x08,  /* global irq status */
MVS_GBL_PI  = 0x0C,  /* ports implemented bitmask */
-   MVS_GBL_PORT_TYPE   = 0x00,  /* port type */
+   MVS_GBL_PORT_TYPE   = 0xa0,  /* port type */

MVS_CTL = 0x100, /* SAS/SATA port configuration */
MVS_PCS = 0x104, /* SAS/SATA port control/status */
@@ -102,11 +138,12 @@
MVS_TX_LO   = 0x124, /* TX (delivery) ring addr */
MVS_TX_HI   = 0x128,

-   MVS_RX_PROD_IDX = 0x12C, /* RX producer pointer */
-   MVS_RX_CONS_IDX = 0x130, /* RX consumer pointer (RO) */
+   MVS_TX_PROD_IDX = 0x12C, /* TX producer pointer */
+   MVS_TX_CONS_IDX = 0x130, /* TX consumer pointer (RO) */
MVS_RX_CFG  = 0x134, /* RX configuration */
MVS_RX_LO   = 0x138, /* RX (completion) ring addr */
MVS_RX_HI   = 0x13C,
+   MVS_RX_CONS_IDX = 0x140, /* RX consumer pointer (RO) */ 

MVS_INT_COAL= 0x148, /* Int coalescing config */
MVS_INT_COAL_TMOUT  = 0x14C, /* Int coalescing timeout */
@@ -117,9 +154,12 @@
 /* ports 1-3 follow after this */
MVS_P0_INT_STAT = 0x160, /* port0 interrupt status */
MVS_P0_INT_MASK = 0x164, /* port0 interrupt mask */
+   MVS_P4_INT_STAT = 0x200, /* Port 4 interrupt status */
+   MVS_P4_INT_MASK = 0x204, /* Port 4 interrupt enable/disable 
mask */

 /* ports 1-3 follow after this */
MVS_P0_SER_CTLSTAT  = 0x180, /* port0 serial control/status */
+   MVS_P4_SER_CTLSTAT  = 0x220, /* port4 serial control/status */

MVS_CMD_ADDR= 0x1B8, 

Re: [PATCH] Marvell 6440 SAS/SATA driver (draft)

2008-01-09 Thread Jeff Garzik

Ke Wei wrote:

The 88SE6440 driver :

The driver is based on bare code from Jeff Garzik. And it can work
under linux kernel 2.6.23.
By far, Can discover and find SAS HDD, but SATA is currently
unsupported. Command queue depth can be above 1.
Most error handling, and some phy handling code is notably missing.


contains the following updates:

--- mvsas_orig.c2007-12-06 19:21:32.0 -0500
+++ mvsas.c 2008-01-09 04:53:14.0 -0500


Fantastic!  I'm delighted to see this is moving, and thanks much for 
getting the driver to that critical it works milestone.


We should note for the linux-scsi audience and potential testers that 
the base driver upon which this is based is available on the sas branch of


git://git.kernel.org:/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git

I will give some substantive comments tomorrow (just got back from long 
trip).  Two quick suggestions:


1) To make it easier for people to review and test the driver, I would 
suggest posting a diff against 2.6.24-rc7 (or 2.6.23), ignoring my 
original code.  Thus, it would result in a patch


2) In future patches, include a Signed-off-by: line when you are ready 
for your patch to be included in the kernel.


Thanks!

Jeff


-
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


Re: [PATCH] Marvell 6440 SAS/SATA driver (draft)

2008-01-09 Thread Jeff Garzik

Jeff Garzik wrote:
1) To make it easier for people to review and test the driver, I would 
suggest posting a diff against 2.6.24-rc7 (or 2.6.23), ignoring my 
original code.  Thus, it would result in a patch


Er, that sentence was incomplete.  Continuing...


Thus it would result in a patch that adds a new file 
drivers/scsi/mvsas.c to the 2.6.24-rc7 kernel, and modifies 
drivers/scsi/Makefile and drivers/scsi/Kconfig to enable this new driver.


That is the format that developers and users are most familiar with, 
when reviewing (and testing) a new driver.


But of course this is a draft, so these guidelines are certainly loose...

Jeff



-
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