Re: [PATCH] Marvell 6440 SAS/SATA driver
On Fri, 2008-02-22 at 11:26 -0500, Jeff Garzik wrote: > 4) [major] Your email was encoded in base64, which makes it difficult > for automated tools to handle, and difficult for some mail clients to > view and reply-to. Yes, I echo this, there's also another problem it causes: The email didn't actually get through the scsi reflector in part, I suspect, because of the base64 content (anybody who wasn't on the direct to or cc list is only seeing Jeff's reply). 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
Ke Wei wrote: Added support for Expander. Based on version 0.1 for mvsas. Signed-off-by: Ke Wei <[EMAIL PROTECTED]> --- diff --git a/drivers/scsi/mvsas.c b/drivers/scsi/mvsas.c old mode 100644 new mode 100755 index 03638b9..3c7a154 --- a/drivers/scsi/mvsas.c +++ b/drivers/scsi/mvsas.c @@ -2,6 +2,7 @@ mvsas.c - Marvell 88SE6440 SAS/SATA support Copyright 2007 Red Hat, Inc. + Copyright 2008 Marvell. <[EMAIL PROTECTED]> This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as @@ -25,6 +26,13 @@ structures. this permits elimination of all the le32_to_cpu() and cpu_to_le32() conversions. + Changelog: + 2008-02-22 0.5 Added support for Expander. + 2008-02-05 0.4 Added support for hotplug and wide port. + 2008-01-22 0.3 Added support for SAS HD and SATA Devices. + 2008-01-09 0.2 detect SAS disk. + 2007-09-25 0.1 rough draft, Initial version. + */ #include Technical content: looks good, ACK Patch content: looks diff'd against correct version, ACK But we still have one major process problem, and a couple minor problems to fix: 1) [minor] please do not include a changelog in the source code. That's what the git repository history is for. 2) [minor] Your patch description (email body) is incorrect. It should describe all changes since version 0.1, the version you diff'd against: Convert rough draft Marvell 6440 driver to a working driver. Added support for SAS and SATA devices, hotplug, wide port, and expanders. 3) [minor] Your email subject should reflect that you are updating version 0.1, the version you diff'd against: [PATCH] mvsas: convert from rough draft to working driver 4) [major] Your email was encoded in base64, which makes it difficult for automated tools to handle, and difficult for some mail clients to view and reply-to. It will require some email configuration on your part to disable this, and send the email as a text/plain message. I've copied Saeed Bishara @ Marvell on this email. Saeed has been successfully sending patch for the sata_mv driver (5040, 6080, 6042, etc.) Maybe Saeed can advise you on his email setup? In any case, once we fix this last problem -- base64 -- we can finally apply your patch and get things moving. You are very close to having a working Linux kernel development setup, thanks for your patience! 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
Ke Wei wrote: Added support for hotplug and wide port. Signed-off-by: Ke Wei <[EMAIL PROTECTED]> --- drivers/scsi/mvsas.c | 445 ++ 1 files changed, 339 insertions(+), 106 deletions(-) Technically speaking, everything is looking great so far. We need to correct one process problem, and we should be able to get your work into the 'mvsas' branch of scsi-misc-2.6.git... and soon thereafter hopefully into the official upstream kernel. The process problem is... when making revisions, we need to get the full patch each time. In this case, each patch should be diff'd against the current version in the 'mvsas' branch: version 0.1. The succession of emails would look like this: Email #1: Subject: [PATCH] mvsas: make it work Convert the skeleton mvsas driver into a real, working driver. Currently, the following works: SAS, SAS expanders, SAS wide ports SATA devices, SATAPI Signed-off-by: Ke Wei <[EMAIL PROTECTED]> And then James, myself, other reviewers reply. Or, you add some new features like hotplugging. Each time, you must regenerate a full patch against the most git repository revision: If a previous patch of yours, version "0.2", has been applied to git, then you would create your patch against version 0.2. If a previous patch of yours has NOT yet been applied to git, then you would create your patch against version 0.1. Thus, in this case, version 0.1 is in 'mvsas' branch, so your second email would then be Email #2: Subject: [PATCH v2] mvsas: make it work Convert the skeleton mvsas driver into a real, working driver. Currently, the following works: SAS, SAS expanders, SAS wide ports SATA devices, SATAPI Signed-off-by: Ke Wei <[EMAIL PROTECTED]> --- Changes since last posting (version 0.2): - fix coding problem And then you continue your work, and add another revision while everyone else is sleeping, the third email would look like: Email #3: Subject: [PATCH v3] mvsas: make it work Convert the skeleton mvsas driver into a real, working driver. Currently, the following works: SAS, SAS expanders, SAS wide ports SATA devices, SATAPI Signed-off-by: Ke Wei <[EMAIL PROTECTED]> --- Changes since last posting (version 0.3): - add hotplug support - add wide port support Changes since last posting (version 0.2): - fix coding problem Thus, you always create a patch against the most recent source code in the git repository. It is common for patches to go through a few revisions on the mailing list, before it is applied to the git repository. So anyway... send a patch against the latest #mvsas (version 0.1), and that patch should go in rapidly. Thanks! And keep up the good work, 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
--- On Tue, 2/5/08, Ke Wei <[EMAIL PROTECTED]> wrote: > + for_each_phy(port->wide_port_phymap, no, j, mvi->chip->n_phy) { > + mvs_write_port_cfg_addr(mvi, no, PHYR_WIDE_PORT); > + mvs_write_port_cfg_data(mvi, no , port->wide_port_phymap); > + } else { > + mvs_write_port_cfg_addr(mvi, no, PHYR_WIDE_PORT); > + mvs_write_port_cfg_data(mvi, no , 0); > + } > +} Don't do this. Make the "if" explicit. Since I can see you've taken this verbatim from the SAS code, if "no" means number, then it is "j". "no" is just a temporary register which gets shifted right each iteration and not of much use outside the macro. Also if "__rest" (which you added to the macro) is 0, then nether statement would execute, which is probably not what you want. If "n_phy" means "number of phys", then its usage that you added into the macro is inconsistent. Furthermore it shouldn't be necessary since wide_port_phymap & ~((2^n_phy)-1) must never be true. Luben - 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
Added support for hotplug and wide port. Signed-off-by: Ke Wei <[EMAIL PROTECTED]> --- drivers/scsi/mvsas.c | 445 ++ 1 files changed, 339 insertions(+), 106 deletions(-) diff --git a/drivers/scsi/mvsas.c b/drivers/scsi/mvsas.c index 3bf009b..e5cf3ad 100755 --- a/drivers/scsi/mvsas.c +++ b/drivers/scsi/mvsas.c @@ -26,6 +26,12 @@ structures. this permits elimination of all the le32_to_cpu() and cpu_to_le32() conversions. + Changelog: + 2008-02-05 0.4 Added support for hotplug and wide port. + 2008-01-22 0.3 Added support for SAS HD and SATA Devices. + 2008-01-09 0.2 detect SAS disk. + 2007-09-95 0.1 rough draft, Initial version. + */ #include @@ -39,13 +45,13 @@ #include #define DRV_NAME "mvsas" -#define DRV_VERSION"0.3" +#define DRV_VERSION"0.4" #define _MV_DUMP 0 #define MVS_DISABLE_NVRAM #define mr32(reg) readl(regs + MVS_##reg) #define mw32(reg,val) writel((val), regs + MVS_##reg) -#define mw32_f(reg,val)do {\ +#define mw32_f(reg,val)do {\ writel((val), regs + MVS_##reg);\ readl(regs + MVS_##reg);\ } while (0) @@ -54,13 +60,19 @@ #define MVS_CHIP_SLOT_SZ (1U << mvi->chip->slot_width) /* offset for D2H FIS in the Received FIS List Structure */ -#define SATA_RECEIVED_D2H_FIS(reg_set) \ +#define SATA_RECEIVED_D2H_FIS(reg_set) \ ((void *) mvi->rx_fis + 0x400 + 0x100 * reg_set + 0x40) -#define SATA_RECEIVED_PIO_FIS(reg_set) \ +#define SATA_RECEIVED_PIO_FIS(reg_set) \ ((void *) mvi->rx_fis + 0x400 + 0x100 * reg_set + 0x20) -#define UNASSOC_D2H_FIS(id) \ +#define UNASSOC_D2H_FIS(id)\ ((void *) mvi->rx_fis + 0x100 * id) +#define for_each_phy(__lseq_mask, __mc, __lseq, __rest) \ + for ((__mc) = (__lseq_mask), (__lseq) = 0; \ + (__mc) != 0 && __rest; \ + (++__lseq), (__mc) >>= 1) \ + if (((__mc) & 1)) + /* driver compile-time configuration */ enum driver_configuration { MVS_TX_RING_SZ = 1024, /* TX ring size (12-bit) */ @@ -130,6 +142,7 @@ enum hw_registers { MVS_INT_STAT= 0x150, /* Central int status */ MVS_INT_MASK= 0x154, /* Central int enable */ MVS_INT_STAT_SRS= 0x158, /* SATA register set status */ + MVS_INT_MASK_SRS= 0x15C, /* ports 1-3 follow after this */ MVS_P0_INT_STAT = 0x160, /* port0 interrupt status */ @@ -223,7 +236,7 @@ enum hw_register_bits { /* shl for ports 1-3 */ CINT_PORT_STOPPED = (1U << 16), /* port0 stopped */ - CINT_PORT = (1U << 8),/* port0 event */ + CINT_PORT = (1U << 8),/* port0 event */ CINT_PORT_MASK_OFFSET = 8, CINT_PORT_MASK = (0xFF << CINT_PORT_MASK_OFFSET), @@ -300,6 +313,7 @@ enum hw_register_bits { PHY_READY_MASK = (1U << 20), /* MVS_Px_INT_STAT, MVS_Px_INT_MASK (per-phy events) */ + PHYEV_DEC_ERR = (1U << 24), /* Phy Decoding Error */ PHYEV_UNASSOC_FIS = (1U << 19), /* unassociated FIS rx'd */ PHYEV_AN= (1U << 18), /* SATA async notification */ PHYEV_BIST_ACT = (1U << 17), /* BIST activate FIS */ @@ -501,6 +515,9 @@ enum status_buffer { SB_RFB_MAX = 0x400, /* RFB size*/ }; +enum error_info_rec { + CMD_ISS_STPD= (1U << 31), /* Cmd Issue Stopped */ +}; struct mvs_chip_info { u32 n_phy; @@ -534,6 +551,7 @@ struct mvs_cmd_hdr { struct mvs_slot_info { struct sas_task *task; u32 n_elem; + u32 tx; /* DMA buffer for storing cmd tbl, open addr frame, status buffer, * and PRD table @@ -546,23 +564,28 @@ struct mvs_slot_info { struct mvs_port { struct asd_sas_port sas_port; - u8 taskfileset; + u8 port_attached; + union { + u8 taskfileset; + u8 wide_port_phymap; + }; }; struct mvs_phy { struct mvs_port *port; struct asd_sas_phy sas_phy; - struct sas_identify identify; + struct sas_identify identify; + struct scsi_device *sdev; u64 dev_sas_addr; u64 att_dev_sas_addr; u32 att_dev_info; u32 dev_info; - u32 type; + u32 phy_type; u32 phy_status;
Re: [PATCH] Marvell 6440 SAS/SATA driver
On Sun, 2008-01-27 at 23:27 +0800, Ke Wei wrote: > > I really don't think you should be doing this. That single ring governs > > all the potential tag slots for everything in this device. If you do a > > simple head tail allocation, what can happen is that you get a slow tag > > (attached to a format command, or a tape command) and then the ring head > > will hit the slow tag and the entire device will halt. I think you need > > a bitmap based allocation algorithm to ensure that if you have a free > > tag anywhere, you'll use it. > > > > If you look at the aic94xx index functions in aic94xx_hwi.h you'll see > > asd_tc_index_get() and asd_tc_index_release() doing exactly what you > > want with the native linux bitmap functions (the aic also uses a single > > issue queue with indexes into it). > > I don't think we need to make use of a bitmap based allocation > algorithm. > My algorithm is a simple non-blocking scheduling. Some faster tag may > be free in advance, so ring will alloc a tag number which has been > stored in mvi->tags array instead of waiting to hit the slow tag. Ah, sorry I didn't see you were actually shuffling entries in the tag array ... > I think that the bitmap based allocation algorithm try to poll and > find > the first zero bit. So process may be slow. > > pls point out anything wrong. I think you'll find a bitmap based algorithm can be much faster. The linux bitmap routines are optimally coded on most architectures (large numbers actually have a find bit in word/long word instruction). Even for a nearly full bitmap, a 512bit array can find the free tag in at most sixteen machine instructions; if the array is sparse, it can do it in about two. Plus a bitmap based scheme has the advantage of tending to allocate hot tags, thus usually keeping reasonable hot cache reuse. A ring based algorithm will effectively do levelling to ensure that every tag is used just about equally, which actually promotes pessimal hot cache reuse. Whether cache locality actually matters to the driver is something I can't actually determine, so I'll leave it up to you to decide. 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
mvsas : Fixed coding problem Signed-off-by: Ke Wei <[EMAIL PROTECTED]> --- drivers/scsi/mvsas.c | 61 +++-- 1 files changed, 34 insertions(+), 27 deletions(-) diff --git a/drivers/scsi/mvsas.c b/drivers/scsi/mvsas.c index 321be21..3bf009b 100755 --- a/drivers/scsi/mvsas.c +++ b/drivers/scsi/mvsas.c @@ -55,11 +55,11 @@ /* offset for D2H FIS in the Received FIS List Structure */ #define SATA_RECEIVED_D2H_FIS(reg_set) \ - (mvi->rx_fis + 0x400 + 0x100 * reg_set + 0x40) + ((void *) mvi->rx_fis + 0x400 + 0x100 * reg_set + 0x40) #define SATA_RECEIVED_PIO_FIS(reg_set) \ - (mvi->rx_fis + 0x400 + 0x100 * reg_set + 0x20) + ((void *) mvi->rx_fis + 0x400 + 0x100 * reg_set + 0x20) #define UNASSOC_D2H_FIS(id) \ - (mvi->rx_fis + 0x100 * id) + ((void *) mvi->rx_fis + 0x100 * id) /* driver compile-time configuration */ enum driver_configuration { @@ -553,17 +553,16 @@ struct mvs_phy { struct mvs_port *port; struct asd_sas_phy sas_phy; struct sas_identify identify; - __le32 dev_info; - __le64 dev_sas_addr; - __le32 att_dev_info; - __le64 att_dev_sas_addr; + u64 dev_sas_addr; + u64 att_dev_sas_addr; + u32 att_dev_info; + u32 dev_info; u32 type; - __le32 phy_status; - __le32 irq_status; - u8 wide_port_phymap; + u32 phy_status; + u32 irq_status; u32 frame_rcvd_size; u8 frame_rcvd[32]; - + u8 wide_port_phymap; }; struct mvs_info { @@ -586,7 +585,7 @@ struct mvs_info { dma_addr_t rx_dma; u32 rx_cons;/* RX consumer idx */ - void*rx_fis;/* RX'd FIS area */ + __le32 *rx_fis;/* RX'd FIS area */ dma_addr_t rx_fis_dma; struct mvs_cmd_hdr *slot; /* DMA command header slots */ @@ -624,7 +623,6 @@ static void mvs_write_port_vsr_data(struct mvs_info *mvi, u32 port, u32 val); static void mvs_write_port_vsr_addr(struct mvs_info *mvi, u32 port, u32 addr); static u32 mvs_read_port_irq_stat(struct mvs_info *mvi, u32 port); static void mvs_write_port_irq_stat(struct mvs_info *mvi, u32 port, u32 val); -static u32 mvs_read_port_irq_mask(struct mvs_info *mvi, u32 port); static void mvs_write_port_irq_mask(struct mvs_info *mvi, u32 port, u32 val); static int mvs_scan_finished(struct Scsi_Host *, unsigned long); @@ -705,8 +703,7 @@ static void mvs_hba_sb_dump(struct mvs_info *mvi, u32 tag, else len_ct = MVS_SSP_CMD_SZ; - offset = - len_ct + MVS_OAF_SZ + + offset = len_ct + MVS_OAF_SZ + sizeof(struct mvs_prd) * mvi->slot_info[tag].n_elem; dev_printk(KERN_DEBUG, &pdev->dev, "+>Status buffer :\n"); mvs_hexdump(32, (u8 *) mvi->slot_info[tag].response, @@ -792,21 +789,27 @@ static void mvs_hba_cq_dump(struct mvs_info *mvi) #endif } -static void mvs_hba_interrupt_enable(struct mvs_info *mvi, int enable) +#if 0 +static void mvs_hba_interrupt_enable(struct mvs_info *mvi) { void __iomem *regs = mvi->regs; u32 tmp; - unsigned long flags; - spin_lock_irqsave(&mvi->lock, flags); tmp = mr32(GBL_CTL); - if (enable) - mw32(GBL_CTL, tmp | INT_EN); - else - mw32(GBL_CTL, tmp & ~INT_EN); - spin_unlock_irqrestore(&mvi->lock, flags); + mw32(GBL_CTL, tmp | INT_EN); +} + +static void mvs_hba_interrupt_disable(struct mvs_info *mvi) +{ + void __iomem *regs = mvi->regs; + u32 tmp; + + tmp = mr32(GBL_CTL); + + mw32(GBL_CTL, tmp & ~INT_EN); } +#endif static int mvs_int_rx(struct mvs_info *mvi, bool self_clear); @@ -1345,6 +1348,7 @@ err_out: return rc; } +#if 0 static void mvs_free_reg_set(struct mvs_info *mvi, struct mvs_port *port) { void __iomem *regs = mvi->regs; @@ -1364,6 +1368,7 @@ static void mvs_free_reg_set(struct mvs_info *mvi, struct mvs_port *port) port->taskfileset = MVS_ID_NOT_MAPPED; } +#endif static u8 mvs_assign_reg_set(struct mvs_info *mvi, struct mvs_port *port) { @@ -2122,10 +2127,12 @@ static void mvs_write_port_irq_stat(struct mvs_info *mvi, u32 port, u32 val) mvs_write_port(mvi, MVS_P0_INT_STAT, MVS_P4_INT_STAT, port, val); } +#if 0 static u32 mvs_read_port_irq_mask(struct mvs_info *mvi, u32 port) { return mvs_read_port(mvi, MVS_P0_INT_MASK, MVS_P4_INT_MASK, port); } +#endif static void mvs_write_port_irq_mask(struct mvs_info *mvi, u32 port, u32 val) { @@ -2258,17 +2265,17 @@ static void __devinit mvs_update_phyinfo(struct mvs_info *mvi, int i) { struct mvs_phy *phy = &mvi->phy[i];
Re: [PATCH] Marvell 6440 SAS/SATA driver
James Bottomley wrote: The lack of interrupt enable looks potentially fatal... See my comments on this specific issue, in this thread, for the reason why that function isn't used... 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
On Sat, 2008-01-26 at 00:43 +0800, Ke Wei wrote: > The attached is Marvell 6440 SAS/SATA driver. I will try to send email > by git-send-email. > > Changelog : > Merged from Jeff's branch to James's branch. > > Signed-off-by: Ke Wei <[EMAIL PROTECTED]> A compile test of this seems to show some problems: drivers/scsi/mvsas.c:2126: warning: 'mvs_read_port_irq_mask' defined but not used drivers/scsi/mvsas.c:796: warning: 'mvs_hba_interrupt_enable' defined but not used drivers/scsi/mvsas.c:1349: warning: 'mvs_free_reg_set' defined but not used The lack of interrupt enable looks potentially fatal...except that you have an open coded interrupt enable in mvs_hw_init(). 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
On Sat, 2008-01-26 at 00:43 +0800, Ke Wei wrote: > struct mvs_phy { > struct mvs_port *port; > struct asd_sas_phy sas_phy; > + struct sas_identify identify; > + __le32 dev_info; > + __le64 dev_sas_addr; > + __le32 att_dev_info; > + __le64 att_dev_sas_addr; > + u32 type; > + __le32 phy_status; > + __le32 irq_status; > + u8 wide_port_phymap; > + u32 frame_rcvd_size; > + u8 frame_rcvd[32]; > > - u8 frame_rcvd[24 + 1024]; > }; These __le quantites don't look right ... they're all read in by readl, which will convert little endian to CPU anyway. > @@ -437,27 +586,55 @@ struct mvs_info { > dma_addr_t rx_dma; > u32 rx_cons;/* RX consumer idx */ > > - __le32 *rx_fis;/* RX'd FIS area */ > + void*rx_fis;/* RX'd FIS area */ Now the received FIS, on the other hand, provided you're storing it in wire format (which you look to be) *is* little endian data by definition in the ATA spec. > -static void mvs_tag_clear(struct mvs_info *mvi, unsigned int tag) > +static void mvs_tag_clear(struct mvs_info *mvi, u32 tag) > { > - mvi->tags[tag / sizeof(unsigned long)] &= > - ~(1UL << (tag % sizeof(unsigned long))); > + mvi->tag_in = (mvi->tag_in + 1) & (MVS_SLOTS - 1); > + mvi->tags[mvi->tag_in] = tag; > } > > -static void mvs_tag_set(struct mvs_info *mvi, unsigned int tag) > +static void mvs_tag_free(struct mvs_info *mvi, u32 tag) > { > - mvi->tags[tag / sizeof(unsigned long)] |= > - (1UL << (tag % sizeof(unsigned long))); > + mvi->tag_out = (mvi->tag_out - 1) & (MVS_SLOTS - 1); > } > > -static bool mvs_tag_test(struct mvs_info *mvi, unsigned int tag) > +static int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out) > { > - return mvi->tags[tag / sizeof(unsigned long)] & > - (1UL << (tag % sizeof(unsigned long))); > + if (mvi->tag_out != mvi->tag_in) { > + *tag_out = mvi->tags[mvi->tag_out]; > + mvi->tag_out = (mvi->tag_out + 1) & (MVS_SLOTS - 1); > + return 0; > + } > + return -EBUSY; I really don't think you should be doing this. That single ring governs all the potential tag slots for everything in this device. If you do a simple head tail allocation, what can happen is that you get a slow tag (attached to a format command, or a tape command) and then the ring head will hit the slow tag and the entire device will halt. I think you need a bitmap based allocation algorithm to ensure that if you have a free tag anywhere, you'll use it. If you look at the aic94xx index functions in aic94xx_hwi.h you'll see asd_tc_index_get() and asd_tc_index_release() doing exactly what you want with the native linux bitmap functions (the aic also uses a single issue queue with indexes into it). 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
Grant Grundler wrote: On Jan 25, 2008 8:43 AM, Ke Wei <[EMAIL PROTECTED]> wrote: The attached is Marvell 6440 SAS/SATA driver. I will try to send email by git-send-email. I know this isn't part of this patch: #define mr32(reg) readl(regs + MVS_##reg) #define mw32(reg,val) writel((val), regs + MVS_##reg) But can "regs" be declared a parameter to the macro? This is a common technique in drivers (especially net drivers), eliminating the redundant-across-the-entire-driver argument passed to each register read or write. The result is infinitely more readable and compact. val = mr32(IRQ_STAT); immediately communicates all the necessary information you need. + MODE_AUTO_DET_PORT7 = (1U << 15), /* port0 SAS/SATA autodetect */ + MODE_AUTO_DET_PORT6 = (1U << 14), + MODE_AUTO_DET_PORT5 = (1U << 13), + MODE_AUTO_DET_PORT4 = (1U << 12), + MODE_AUTO_DET_PORT3 = (1U << 11), + MODE_AUTO_DET_PORT2 = (1U << 10), + MODE_AUTO_DET_PORT1 = (1U << 9), + MODE_AUTO_DET_PORT0 = (1U << 8), These really aren't needed. Like James noted, without public docs, we don't want to be removing any hardware definitions. Have to stop for now...but I'm wonder how this driver ever worked given the number of HW register bits that were changed (assuming they were wrong before this patch). And this driver looks _alot_ better than the previous Marvell drivers I've looked at. Please keep up the good work! :) Before this patch, the driver did not work. As I do with all other drivers I write, I write the entire driver from the datasheet before testing anything. 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
On Wed, 2008-01-23 at 18:54 +0800, Ke Wei wrote: > Attachment is a patch file for 6440 driver. I will have to spend more > time on setting my mail client. Yesterday I used mutt tool. But Look > like the problem still exists. By default, unfortunately, I think no email tool nowadays inserts plain text ... they all assume you want it nicely formatted with proper line breaks. There's a guide to inserting plain text patches inline with some email tools in the kernel Documentation directory: Documentation/email-clients.txt The current attachment is fine for a patch application, 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
On Fri, 2008-01-25 at 09:24 -0800, Grant Grundler wrote: > On Jan 25, 2008 8:43 AM, Ke Wei <[EMAIL PROTECTED]> wrote: > > The attached is Marvell 6440 SAS/SATA driver. I will try to send email > > by git-send-email. > > I know this isn't part of this patch: > #define mr32(reg)readl(regs + MVS_##reg) > #define mw32(reg,val)writel((val), regs + MVS_##reg) > > But can "regs" be declared a parameter to the macro? > And the one letter difference in the name is just asking for trouble. > Better to call it "mmio_base" or something a bit longer that won't > likely collide with other stuff. > > +/* offset for D2H FIS in the Received FIS List Structure */ > +#define SATA_RECEIVED_D2H_FIS(reg_set) \ > + (mvi->rx_fis + 0x400 + 0x100 * reg_set + 0x40) > > Ditto. > > + MODE_AUTO_DET_PORT7 = (1U << 15), /* port0 SAS/SATA autodetect */ > + MODE_AUTO_DET_PORT6 = (1U << 14), > + MODE_AUTO_DET_PORT5 = (1U << 13), > + MODE_AUTO_DET_PORT4 = (1U << 12), > + MODE_AUTO_DET_PORT3 = (1U << 11), > + MODE_AUTO_DET_PORT2 = (1U << 10), > + MODE_AUTO_DET_PORT1 = (1U << 9), > + MODE_AUTO_DET_PORT0 = (1U << 8), > > These really aren't needed. > > #define MODE_AUTO_DET_EN (0xff << 8)/* enable auto detect on all > 8 ports */ > > > Ditto for MODE_SAS_SATA. Given that we don't have public docs for this driver, having register bit definitions in the driver, even if they aren't used anywhere can be incredibly helpful, so please don't remove them. Even more useful would be additional comments saying what they actually do ... 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
On Jan 25, 2008 8:43 AM, Ke Wei <[EMAIL PROTECTED]> wrote: > The attached is Marvell 6440 SAS/SATA driver. I will try to send email > by git-send-email. > Sorry, just saw some more issues in +static void mvs_hba_interrupt_enable(struct mvs_info *mvi, int enable) Three comments here: 1) This routine is slightly misnamed since it can both enable and disable the interrupt. I _suggest_ to not pass in "enable" parameter and add a mvs_hba_interrupt_disable() to deal with (2) and (3) below. 2) On interrupt disable, MMIO write posting _could_ be a problem here. ie interrupts could be generated after calling this routine since it doesn't force the MMIO write to the PCI device with an MMIO read. (For more info on this see chapter 4 of http://iou.parisc-linux.org/ols_2002/ ) 3) Even after fixing (2), interrupts can already be in-flight and not yet delivered. Higher level code needs to make sure it can tolerate a "late arriving" IRQ. One MMIO read should mostly solve this for MSI but not for Line-based IRQ. (MMIO Read-return will flush inflight MSI). hth, grant - 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
On Jan 25, 2008 8:43 AM, Ke Wei <[EMAIL PROTECTED]> wrote: > The attached is Marvell 6440 SAS/SATA driver. I will try to send email > by git-send-email. I know this isn't part of this patch: #define mr32(reg) readl(regs + MVS_##reg) #define mw32(reg,val) writel((val), regs + MVS_##reg) But can "regs" be declared a parameter to the macro? And the one letter difference in the name is just asking for trouble. Better to call it "mmio_base" or something a bit longer that won't likely collide with other stuff. +/* offset for D2H FIS in the Received FIS List Structure */ +#define SATA_RECEIVED_D2H_FIS(reg_set) \ + (mvi->rx_fis + 0x400 + 0x100 * reg_set + 0x40) Ditto. + MODE_AUTO_DET_PORT7 = (1U << 15), /* port0 SAS/SATA autodetect */ + MODE_AUTO_DET_PORT6 = (1U << 14), + MODE_AUTO_DET_PORT5 = (1U << 13), + MODE_AUTO_DET_PORT4 = (1U << 12), + MODE_AUTO_DET_PORT3 = (1U << 11), + MODE_AUTO_DET_PORT2 = (1U << 10), + MODE_AUTO_DET_PORT1 = (1U << 9), + MODE_AUTO_DET_PORT0 = (1U << 8), These really aren't needed. #define MODE_AUTO_DET_EN (0xff << 8)/* enable auto detect on all 8 ports */ Ditto for MODE_SAS_SATA. + /* Port n Attached Device Info */ Groups bits defined have a comment preceeding a group of bits (which is a good thing). If multiple registers are being defined in one enum declaration, can you please add either the register offset to the comment or include the constant that defines the offset (e.g. MVS_P0_CFG_ADDR)? Have to stop for now...but I'm wonder how this driver ever worked given the number of HW register bits that were changed (assuming they were wrong before this patch). And this driver looks _alot_ better than the previous Marvell drivers I've looked at. Please keep up the good work! :) hth, grant > Changelog : > Merged from Jeff's branch to James's branch. > > Signed-off-by: Ke Wei <[EMAIL PROTECTED]> > > > > On Jan 23, 2008 7:41 PM, Jeff Garzik <[EMAIL PROTECTED]> wrote: > > Ke Wei wrote: > > > Attachment is a patch file for 6440 driver. I will have to spend more > > > time on setting my mail client. Yesterday I used mutt tool. But Look > > > like the problem still exists. > > > I fixed all issues which you mentioned , but > > > > The changes look good, thanks! > > > > In terms of engineering process, I should have been more clear: when > > revising your submission, you should make the revisions and then > > regenerate your original patch. > > > > To use a silly example, if you needed to fix a 1-character typo in a > > 1,000-line patch, you would need to make your revision, re-generate the > > 1,000-line patch, and email that new patch. > > > > So, to move forward, please create one single patch with the latest > > mvsas, diff'd against the 'mvsas' branch of > > git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi-misc-2.6 > > > > I will "ack" that patch (indicate my approval), and unless other > > objections surface, James will merge your patch into the 'mvsas' branch. > > That will get the driver on the road to be included in 2.6.25. > > > > > > >>> @@ -666,11 +970,53 @@ static int mvs_nvram_read(struct mvs_info *mvi, > > >>> unsigned > > >>> int addr, > > >>> err_out: > > >>>dev_printk(KERN_ERR, &mvi->pdev->dev, "%s", msg); > > >>>return rc; > > >>> +#else > > >>> + memcpy(buf, "\x50\x05\x04\x30\x11\xab\x00\x00", 8); > > >>> + return 0; > > >>> +#endif > > >> > > >> what happens if two adapters are used, with the same SAS address? That > > >> causes problems... > > >> > > >> > > > Our bios can write SAS Address per port when system boot , so I think > > > we don't need read or configure address. > > > > It is standard Linux policy to reduce or eliminate BIOS dependencies as > > much as possible. > > > > This is because there are several common scenarios where BIOS may not > > run, or otherwise not be available: > > > > * suspend/resume > > * controller hotplug > > * non-x86 PCI platforms such as POWER > > > > Thus, we need the driver to obtain these addresses -- hopefully reading > > them from a BIOS data table somewhere, if NVRAM is not available. > > > > This is a highly common scenario -- obtaining unique identifying > > information in the absence of BIOS. > > > > > > > > Similarly, we cannot rely on BIOS to perform any global reset or errata > > workaround duties for us. That must be coded into the driver also. > > > > > > > And I reserved hexdump funciton if you don't care. Only debugging. > > > > It is not an important matter, but it would be nice to clean that up > > eventually. > > > > Also, FWIW, we have a standard method of debug output: > > > >struct pci_dev *pdev; > > > >dev_dbg(&pdev->dev, KERN_INFO, > >"this is debug message number %d", 2); > > > > which will only be compiled into the driver when DEBUG is defined. > > > > > > >>> +static int mvs_abort_task(struct sas_task *task) > > >>> +{ > > >>> + /*FIXME*/ > >
Re: [PATCH] Marvell 6440 SAS/SATA driver
On Jan 22, 2008 7:58 PM, Jeff Garzik <[EMAIL PROTECTED]> wrote: > > +#define READ_PORT_CONFIG_DATA(i) \ > > + ((i > 3)?mr32(P4_CFG_DATA + (i - 4) * 8):mr32(P0_CFG_DATA + i * 8)) > > +#define WRITE_PORT_CONFIG_DATA(i,tmp) \ > > + {if (i > 3)mw32(P4_CFG_DATA + (i - 4) * 8, tmp); \ > > + else \ > > + mw32(P0_CFG_DATA + i * 8, tmp); } > > +#define WRITE_PORT_CONFIG_ADDR(i,tmp) \ > > + {if (i > 3)mw32(P4_CFG_ADDR + (i - 4) * 8, tmp); \ > > + else \ > > + mw32(P0_CFG_ADDR + i * 8, tmp); } > > + > > +#define READ_PORT_PHY_CONTROL(i) \ > > + ((i > 3)?mr32(P4_SER_CTLSTAT + (i - 4) * 4):mr32(P0_SER_CTLSTAT+i * > > 4)) > > +#define WRITE_PORT_PHY_CONTROL(i,tmp) \ > > + {if (i > 3)mw32(P4_SER_CTLSTAT + (i - 4) * 4, tmp); \ > > + else \ > > + mw32(P0_SER_CTLSTAT + i * 4, tmp); } > > + > > +#define READ_PORT_VSR_DATA(i) \ > > + ((i > 3)?mr32(P4_VSR_DATA + (i - 4) * 8):mr32(P0_VSR_DATA+i*8)) > > +#define WRITE_PORT_VSR_DATA(i,tmp) \ > > + {if (i > 3)mw32(P4_VSR_DATA + (i - 4) * 8, tmp); \ > > + else \ > > + mw32(P0_VSR_DATA + i*8, tmp); } > > +#define WRITE_PORT_VSR_ADDR(i,tmp) \ > > + {if (i > 3)mw32(P4_VSR_ADDR + (i - 4) * 8, tmp); \ > > + else \ > > + mw32(P0_VSR_ADDR + i * 8, tmp); } > > + > > +#define READ_PORT_IRQ_STAT(i) \ > > + ((i > 3)?mr32(P4_INT_STAT + (i - 4) * 8):mr32(P0_INT_STAT + i * 8)) > > +#define WRITE_PORT_IRQ_STAT(i,tmp) \ > > + {if (i > 3)mw32(P4_INT_STAT + (i-4) * 8, tmp); \ > > + else \ > > + mw32(P0_INT_STAT + i * 8, tmp); } > > +#define READ_PORT_IRQ_MASK(i) \ > > + ((i > 3)?mr32(P4_INT_MASK + (i-4) * 8):mr32(P0_INT_MASK+i*8)) > > +#define WRITE_PORT_IRQ_MASK(i,tmp) \ > > + {if (i > 3)mw32(P4_INT_MASK + (i-4) * 8, tmp); \ > > + else \ > > + mw32(P0_INT_MASK + i * 8, tmp); } > > > make these macros readable, by breaking each C statement into a separate > line I'd prefer these all be written as static functions. Let the compiler do the type checking instead of assuming we've got it right. There is no performance difference AFAIK. > > > > > > @@ -260,13 +368,33 @@ enum hw_register_bits { > >PHYEV_RDY_CH= (1U << 0),/* phy ready changed state */ > > > >/* MVS_PCS */ > > + PCS_EN_SATA_REG = (16), /* Enable SATA Register > > Set*/ > > + PCS_EN_PORT_XMT_START = (12), /* Enable Port Transmit*/ > > + PCS_EN_PORT_XMT_START2 = (8), /* For 6480*/ > >PCS_SATA_RETRY = (1U << 8),/* retry ctl FIS on R_ERR */ > >PCS_RSP_RX_EN = (1U << 7),/* raw response rx */ > >PCS_SELF_CLEAR = (1U << 5),/* self-clearing int mode */ > >PCS_FIS_RX_EN = (1U << 4),/* FIS rx enable */ What's the difference between PCS_FIS_RX_EN and PCS_EN_SATA_REG? Grouping them together implies they define bits/commands for the same register. The three new entries collide with the existing definitions and it's not obvious what the intended usage is. Later, jeff pointed out drivers should be using dev_dbg() instead of splattering "MVS_PRINTK" alll over the place. I'd like to strongly encourage that. hth, grant - 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
Ke Wei wrote: Attachment is a patch file for 6440 driver. I will have to spend more time on setting my mail client. Yesterday I used mutt tool. But Look like the problem still exists. I fixed all issues which you mentioned , but The changes look good, thanks! In terms of engineering process, I should have been more clear: when revising your submission, you should make the revisions and then regenerate your original patch. To use a silly example, if you needed to fix a 1-character typo in a 1,000-line patch, you would need to make your revision, re-generate the 1,000-line patch, and email that new patch. So, to move forward, please create one single patch with the latest mvsas, diff'd against the 'mvsas' branch of git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi-misc-2.6 I will "ack" that patch (indicate my approval), and unless other objections surface, James will merge your patch into the 'mvsas' branch. That will get the driver on the road to be included in 2.6.25. @@ -666,11 +970,53 @@ static int mvs_nvram_read(struct mvs_info *mvi, unsigned int addr, err_out: dev_printk(KERN_ERR, &mvi->pdev->dev, "%s", msg); return rc; +#else + memcpy(buf, "\x50\x05\x04\x30\x11\xab\x00\x00", 8); + return 0; +#endif what happens if two adapters are used, with the same SAS address? That causes problems... Our bios can write SAS Address per port when system boot , so I think we don't need read or configure address. It is standard Linux policy to reduce or eliminate BIOS dependencies as much as possible. This is because there are several common scenarios where BIOS may not run, or otherwise not be available: * suspend/resume * controller hotplug * non-x86 PCI platforms such as POWER Thus, we need the driver to obtain these addresses -- hopefully reading them from a BIOS data table somewhere, if NVRAM is not available. This is a highly common scenario -- obtaining unique identifying information in the absence of BIOS. Similarly, we cannot rely on BIOS to perform any global reset or errata workaround duties for us. That must be coded into the driver also. And I reserved hexdump funciton if you don't care. Only debugging. It is not an important matter, but it would be nice to clean that up eventually. Also, FWIW, we have a standard method of debug output: struct pci_dev *pdev; dev_dbg(&pdev->dev, KERN_INFO, "this is debug message number %d", 2); which will only be compiled into the driver when DEBUG is defined. +static int mvs_abort_task(struct sas_task *task) +{ + /*FIXME*/ + MVS_PRINTK("mvs abort task\n"); + return TMF_RESP_FUNC_COMPLETE; +} should make an attempt to do something sane here if entering this abort function , I think I must fix the unknown issues instead of here. But I also will implement next. Ok, 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
Comments inline, mostly minor stuff cleaning up the source. Major problem though: your mailer converted tabs to spaces, so our automated patch tools won't work on your submission. It usually takes a few attempts to get your email setup working, such that all the automated tools used in the Linux community work. Ke Wei wrote: +#define MVS_QUEUE_SIZE (30) to be consistent with the rest of the driver, make this an enum +#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,6 +53,65 @@ readl(regs + MVS_##reg);\ } while (0) +#define MVS_BIT(x) (1L << (x)) + +#define PORT_TYPE_SATA MVS_BIT(0) +#define PORT_TYPE_SAS MVS_BIT(1) to be consistent with the rest of the driver, just open-code "1 << n". This also makes it easier to get the C type correct. +#define MVS_ID_NOT_MAPPED 0xff +#define MVS_CHIP_SLOT_SZ (1U << mvi->chip->slot_width) + +/* offset for D2H FIS in the Received FIS List Structure */ +#define SATA_RECEIVED_D2H_FIS(reg_set) \ + (mvi->rx_fis + 0x400 + 0x100 * reg_set + 0x40) +#define SATA_RECEIVED_PIO_FIS(reg_set) \ + (mvi->rx_fis + 0x400 + 0x100 * reg_set + 0x20) +#define UNASSOC_D2H_FIS(id) \ + (mvi->rx_fis + 0x100 * id) + + +#define READ_PORT_CONFIG_DATA(i) \ + ((i > 3)?mr32(P4_CFG_DATA + (i - 4) * 8):mr32(P0_CFG_DATA + i * 8)) +#define WRITE_PORT_CONFIG_DATA(i,tmp) \ + {if (i > 3)mw32(P4_CFG_DATA + (i - 4) * 8, tmp); \ + else \ + mw32(P0_CFG_DATA + i * 8, tmp); } +#define WRITE_PORT_CONFIG_ADDR(i,tmp) \ + {if (i > 3)mw32(P4_CFG_ADDR + (i - 4) * 8, tmp); \ + else \ + mw32(P0_CFG_ADDR + i * 8, tmp); } + +#define READ_PORT_PHY_CONTROL(i) \ + ((i > 3)?mr32(P4_SER_CTLSTAT + (i - 4) * 4):mr32(P0_SER_CTLSTAT+i * 4)) +#define WRITE_PORT_PHY_CONTROL(i,tmp) \ + {if (i > 3)mw32(P4_SER_CTLSTAT + (i - 4) * 4, tmp); \ + else \ + mw32(P0_SER_CTLSTAT + i * 4, tmp); } + +#define READ_PORT_VSR_DATA(i) \ + ((i > 3)?mr32(P4_VSR_DATA + (i - 4) * 8):mr32(P0_VSR_DATA+i*8)) +#define WRITE_PORT_VSR_DATA(i,tmp) \ + {if (i > 3)mw32(P4_VSR_DATA + (i - 4) * 8, tmp); \ + else \ + mw32(P0_VSR_DATA + i*8, tmp); } +#define WRITE_PORT_VSR_ADDR(i,tmp) \ + {if (i > 3)mw32(P4_VSR_ADDR + (i - 4) * 8, tmp); \ + else \ + mw32(P0_VSR_ADDR + i * 8, tmp); } + +#define READ_PORT_IRQ_STAT(i) \ + ((i > 3)?mr32(P4_INT_STAT + (i - 4) * 8):mr32(P0_INT_STAT + i * 8)) +#define WRITE_PORT_IRQ_STAT(i,tmp) \ + {if (i > 3)mw32(P4_INT_STAT + (i-4) * 8, tmp); \ + else \ + mw32(P0_INT_STAT + i * 8, tmp); } +#define READ_PORT_IRQ_MASK(i) \ + ((i > 3)?mr32(P4_INT_MASK + (i-4) * 8):mr32(P0_INT_MASK+i*8)) +#define WRITE_PORT_IRQ_MASK(i,tmp) \ + {if (i > 3)mw32(P4_INT_MASK + (i-4) * 8, tmp); \ + else \ + mw32(P0_INT_MASK + i * 8, tmp); } make these macros readable, by breaking each C statement into a separate line @@ -260,13 +368,33 @@ enum hw_register_bits { PHYEV_RDY_CH= (1U << 0),/* phy ready changed state */ /* MVS_PCS */ + PCS_EN_SATA_REG = (16), /* Enable SATA Register Set*/ + PCS_EN_PORT_XMT_START = (12), /* Enable Port Transmit*/ + PCS_EN_PORT_XMT_START2 = (8), /* For 6480*/ PCS_SATA_RETRY = (1U << 8),/* retry ctl FIS on R_ERR */ PCS_RSP_RX_EN = (1U << 7),/* raw response rx */ PCS_SELF_CLEAR = (1U << 5),/* self-clearing int mode */ PCS_FIS_RX_EN = (1U << 4),/* FIS rx enable */ PCS_CMD_STOP_ERR= (1U << 3),/* cmd stop-on-err enable */ - PCS_CMD_RST = (1U << 2),/* reset cmd issue */ + PCS_CMD_RST = (1U << 1),/* reset cmd issue */ PCS_CMD_EN = (1U << 0),/* enable cmd issue */ + + /*Port n Attached Device Info*/ + PORT_DEV_SSP_TRGT = (1U << 19), + PORT_DEV_SMP_TRGT = (1U << 18), + PORT_DEV_STP_TRGT = (1U << 17), + PORT_DEV_SSP_INIT = (1U << 11), + PORT_DEV_SMP_INIT = (1U << 10), + PORT_DEV_STP_INIT = (1U << 9), + PORT_PHY_ID_MASK= (0xFFU << 24), + PORT_DEV_TRGT_MASK = (0x7U << 17), + PORT_DEV_INIT_MASK = (0x7U << 9), + PORT_DEV_TYPE_MASK = (0x7U << 0), + + /*Port n PHY Status*/ + PHY_RDY = (1U << 2), + PHY_DW_SYNC = (1U << 1), + PHY_OOB_DTCTD = (1U << 0), to be consistent, add spaces after /* and before */ struct mvs_port { struct asd_sas_port sas_port; + u8 taskfileset; }; struct mvs_phy { str
Re: [PATCH] Marvell 6440 SAS/SATA driver (draft)
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) \ > > + ((i>3)?mr32(P4_CFG_DATA+(i-4)*8):mr32(P0_CFG_DATA+i*8)) > > +#define WRITE_PORT_CONFIG_DATA(i,tmp) \ > > + {if(i>3)mw32(P4_CFG_DATA+(i-4)*8,tmp);else mw32(P0_CFG_DATA+i*8,tmp);} > > +#define WRITE_PORT_CONFIG_ADDR(i,tmp) \ > > + {if(i>3)mw32(P4_CFG_ADDR+(i-4)*8,tmp);else mw32(P0_CFG_ADDR+i*8,tmp);} > > + > > +#define READ_PORT_PHY_CONTROL(i) \ > > + ((i>3)?mr32(P4_SER_CTLSTAT+(i-4)*4):mr32(P0_SER_CTLSTAT+i*4)) > > +#define WRITE_PORT_PHY_CONTROL(i,tmp) \ > > + {if(i>3)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) \ > > + ((i>3)?mr32(P4_VSR_DATA+(i-4)*8):mr32(P0_VSR_DATA+i*8)) > > +#define WRITE_PORT_VSR_DATA(i,tmp) \ > > + {if(i>3)mw32(P4_VSR_DATA+(i-4)*8,tmp);else mw32(P0_VSR_DATA+i*8,tmp);} > > +#define WRITE_PORT_VSR_ADDR(i,tmp) \ > > + {if(i>3)mw32(P4_VSR_ADDR+(i-4)*8,tmp);else mw32(P0_VSR_ADDR+i*8,tmp);} > > + > > +#define READ_PORT_IRQ_STAT(i) \ > > + ((i>3)?mr32(P4_INT_STAT+(i-4)*8):mr32(P0_INT_STAT+i*8)) > > +#define WRITE_PORT_IRQ_STAT(i,tmp) \ > > + {if(i>3)mw32(P4_INT_STAT+(i-4)*8,tmp);else mw32(P0_INT_STAT+i*8,tmp);} > > +#define READ_PORT_IRQ_MASK(i) \ > > + ((i>3)?mr32(P4_INT_MASK+(i-4)*8):mr32(P0_INT_MASK+i*8)) > > +#define WRITE_PORT_IRQ_MASK(i,tmp) \ > > + {if(i>3)mw32(P4_INT_MASK+(i-4)*8,tmp);else mw32(P0_INT_MASK+i*8,tmp);} > > + > > /* driver compile-time configuration */ > > enum driver_configuration { > > - MVS_
Re: [PATCH] Marvell 6440 SAS/SATA driver (draft)
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) \ > + ((i>3)?mr32(P4_CFG_DATA+(i-4)*8):mr32(P0_CFG_DATA+i*8)) > +#define WRITE_PORT_CONFIG_DATA(i,tmp) \ > + {if(i>3)mw32(P4_CFG_DATA+(i-4)*8,tmp);else mw32(P0_CFG_DATA+i*8,tmp);} > +#define WRITE_PORT_CONFIG_ADDR(i,tmp) \ > + {if(i>3)mw32(P4_CFG_ADDR+(i-4)*8,tmp);else mw32(P0_CFG_ADDR+i*8,tmp);} > + > +#define READ_PORT_PHY_CONTROL(i) \ > + ((i>3)?mr32(P4_SER_CTLSTAT+(i-4)*4):mr32(P0_SER_CTLSTAT+i*4)) > +#define WRITE_PORT_PHY_CONTROL(i,tmp) \ > + {if(i>3)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) \ > + ((i>3)?mr32(P4_VSR_DATA+(i-4)*8):mr32(P0_VSR_DATA+i*8)) > +#define WRITE_PORT_VSR_DATA(i,tmp) \ > + {if(i>3)mw32(P4_VSR_DATA+(i-4)*8,tmp);else mw32(P0_VSR_DATA+i*8,tmp);} > +#define WRITE_PORT_VSR_ADDR(i,tmp) \ > + {if(i>3)mw32(P4_VSR_ADDR+(i-4)*8,tmp);else mw32(P0_VSR_ADDR+i*8,tmp);} > + > +#define READ_PORT_IRQ_STAT(i) \ > + ((i>3)?mr32(P4_INT_STAT+(i-4)*8):mr32(P0_INT_STAT+i*8)) > +#define WRITE_PORT_IRQ_STAT(i,tmp) \ > + {if(i>3)mw32(P4_INT_STAT+(i-4)*8,tmp);else mw32(P0_INT_STAT+i*8,tmp);} > +#define READ_PORT_IRQ_MASK(i) \ > + ((i>3)?mr32(P4_INT_MASK+(i-4)*8):mr32(P0_INT_MASK+i*8)) > +#define WRITE_PORT_IRQ_MASK(i,tmp) \ > + {if(i>3)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 conf
Re: [PATCH] Marvell 6440 SAS/SATA driver (draft)
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)
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
Re: [PATCH] Marvell 6440 SAS/SATA driver (draft)
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