imu-scsi for iop13xx
Subsystems (3)
cover imu-scsi for iop13xx
The imu (inter-processor messaging unit) on the iop13xx enables mailbox style communication between the two cores on iop13xx. Greg Tucker has has wrapped this capability in a scsi driver which allows Linux to interface with the other core as if it were a scsi disk target. I would like to submit it for 2.6.21 consideration. As it is an ARM-platform-specific driver does it fall under Russell's purview, or James' since it is a scsi driver.
Regards,
Dan
Greg Tucker (2):
iop13xx: add base support for the imu
iop13xx: imu scsi driver
[note: [email protected] is subscriber-only ]
RFC PATCH 1/2 iop13xx: add base support for the imu
From: Greg Tucker <[email protected]>
The interprocessor messaging unit supports mailbox style communication between the two Xscale cores on iop342.
---
arch/arm/mach-iop13xx/Kconfig | 2 arch/arm/mach-iop13xx/Makefile | 1 arch/arm/mach-iop13xx/imu/Kconfig | 19 + arch/arm/mach-iop13xx/imu/Makefile | 3 arch/arm/mach-iop13xx/imu/common.c | 486 ++++++++++++++++++++++++++++ arch/arm/mach-iop13xx/imu/dev.c | 438 +++++++++++++++++++++++++ arch/arm/mach-iop13xx/imu/imu.c | 151 +++++++++ include/asm-arm/arch-iop13xx/iop13xx-imu.h | 207 ++++++++++++ 8 files changed, 1307 insertions(+), 0 deletions(-)
arch/arm/mach-iop13xx/Kconfig +2 -0
@@ -16,5 +16,7 @@ config MACH_IQ81340MC Say Y here if you want to support running on the Intel IQ81340MC evaluation kit. +source "arch/arm/mach-iop13xx/imu/Kconfig" + endmenu endif
arch/arm/mach-iop13xx/Makefile +1 -0
@@ -9,3 +9,4 @@ obj-$(CONFIG_ARCH_IOP13XX) += pci.o obj-$(CONFIG_ARCH_IOP13XX) += io.o obj-$(CONFIG_MACH_IQ81340SC) += iq81340sc.o obj-$(CONFIG_MACH_IQ81340MC) += iq81340mc.o +obj-$(CONFIG_IOP_IMU) += imu/
arch/arm/mach-iop13xx/imu/Kconfig new file +19 -0
@@ -0,0 +1,19 @@ +# +# IOP13xx IMU Support +# + +menu "IOP13XX IMU Support" + +config IOP_IMU + tristate "IOP IMU support" + depends on EXPERIMENTAL + ---help--- + This includes support functions for the IMU. + +config IOP_IMU_DEV + tristate "IOP IMU char driver" + depends on IOP_IMU + ---help--- + This is a char driver that passes messages throught the IMU. + +endmenu
arch/arm/mach-iop13xx/imu/Makefile new file +3 -0
@@ -0,0 +1,3 @@ +obj-$(CONFIG_IOP_IMU) += common.o imu.o +obj-$(CONFIG_IOP_IMU_DEV) += dev.o +
arch/arm/mach-iop13xx/imu/common.c new file +486 -0
486 lines not on this page · raw message
arch/arm/mach-iop13xx/imu/dev.c new file +438 -0
438 lines not on this page · raw message
arch/arm/mach-iop13xx/imu/imu.c new file +151 -0
151 lines not on this page · raw message
include/asm-arm/arch-iop13xx/iop13xx-imu.h new file +207 -0
207 lines not on this page · raw message
RFC PATCH 2/2 iop13xx: imu scsi driver
In reply to [email protected]From: Greg Tucker <[email protected]>
Enable Linux to access the other core as if it were a scsi target.
---
arch/arm/mach-iop13xx/imu/Kconfig | 7 drivers/scsi/Makefile | 1 drivers/scsi/iop13xx-imu-scsi.c | 623 +++++++++++++++++++++++++++++++++++++ 3 files changed, 631 insertions(+), 0 deletions(-)
arch/arm/mach-iop13xx/imu/Kconfig +7 -0
@@ -16,4 +16,11 @@ config IOP_IMU_DEV ---help--- This is a char driver that passes messages throught the IMU. +config IOP_IMU_SCSI + tristate "IOP IMU scsi driver" + depends on IOP_IMU + ---help--- + This is a low-level SCSI driver that passes SCSI commands + to core 2. + endmenu
drivers/scsi/Makefile +1 -0
@@ -132,6 +132,7 @@ obj-$(CONFIG_SCSI_IBMVSCSIS) += ibmvscsi/ obj-$(CONFIG_SCSI_HPTIOP) += hptiop.o obj-$(CONFIG_SCSI_STEX) += stex.o +obj-$(CONFIG_IOP_IMU_SCSI) += iop13xx-imu-scsi.o obj-$(CONFIG_ARM) += arm/ obj-$(CONFIG_CHR_DEV_ST) += st.o
drivers/scsi/iop13xx-imu-scsi.c new file +623 -0
623 lines not on this page · raw message
Re: [PATCH RFC 2/2] iop13xx: imu scsi driver
There are quite a few commented out debugging statements and deprecated C++ style comments dotted throughout this driver ... could these be corrected? (move to C style comments and either implement a debugging strategy or pull out the statements).
Another thing that strikes me is on the ARM side, it looks like this is effectively vectoring up to a user space target ... is that true? And if so, would the existing in-kernel target infrastructure be a better way of doing this?
Quoted 1 line · Dan Williams
+static int imu_queuecommand(struct
scsi_cmnd*scp,
[...]
drivers/scsi/iop13xx-imu-scsi.c line 237
Quoted 5 lines
drivers/scsi/iop13xx-imu-scsi.c new file +5
-0+if (!imu_scsi->connection_handle) { + scp->result = (DID_RESET <<16 ) | (SUGGEST_RETRY <<24 ); +done (scp); +return SCSI_MLQUEUE_HOST_BUSY ; + }
Absolutely wrong. Either you execute done() on the command and return zero, or you *don't* execute done on the command and return a queueing condition. Doing both will lead to double completions and really horrible things.
drivers/scsi/iop13xx-imu-scsi.c line 263
Quoted 9 lines
drivers/scsi/iop13xx-imu-scsi.c new file +9
-0+switch (scp->cmnd[0 ]) { +case TEST_UNIT_READY : +break ; +case READ_10 : +case READ_6 : +case WRITE_10 : +case WRITE_6 : +case READ_CAPACITY : +default :
The predicate for this switch looks wrong. You're not really interpreting the commands, you appear to be interested in whether they have data buffers attached. Might I suggest this might be better done by
if (scp->sc_data_direction == DMA_NONE)
break; else if (scp->use_sg == 0) /* request buffer case; hopefully disappearing soon */ else /* sg case */
Quoted 15 lines
drivers/scsi/iop13xx-imu-scsi.c new file +15
-0+if (scp->use_sg ==0 ) { +dma_addr_t buff =0 ; +if (NULL == scp->request_buffer) +imu_warn ("got cmd with NULL request_buffer\n" ); +else + buff = +dma_map_single (NULL , scp->request_buffer, + scp->request_bufflen, +DMA_BIDIRECTIONAL ); +if (buff ==0 ) { +imu_warn ("null return from dma_map_single\n" ); + msg->flags =IMU_FCODE_STATUS ; + msg->status =0 ;//todo: what is error status? +iop_queue_postmsg (Q_NUM , msg); +return 1 ;
drivers/scsi/iop13xx-imu-scsi.c line 288
Quoted 10 lines
drivers/scsi/iop13xx-imu-scsi.c new file +10
-0+ } + sge = &(msg->cdb_cmd.sge); + sge->addr_l = buff; + sge->addr_h =0 ; + sge->flen = scp->request_bufflen |FLAG_LAST_ELEMENT ; + +// This shouldn't be necessary +//dma_sync_single(NULL, buff, scp->request_bufflen, DMA_BIDIRECTIONAL); +imu_debug ("cmd msg with 1 sgl entry, addr=0x%x\n" , + buff);
Why BIDIRECTIONAL? This can be expensive on some architectures (although I don't know about ARM). However, for these commands, the scp->sc_data_direction should contain the exact direction you need to program into the DMA engines. (Consider this the same comment for the use_sg > 0 portion as well).
Review
Quoted 1 line
(quoted diff) +1
-0+ iop_queue_postmsg(Q_NUM, msg);
This has error returns ... should you be handling them?
Quoted 15 lines
drivers/scsi/iop13xx-imu-scsi.c new file +15
-0+static int imu_eh_abort (struct scsi_cmnd *cmd) +{ +imu_warn ("Received eh command abort\n" ); +//dbg_print_cmd(cmd); +return SUCCESS ; +} + +static int imu_eh_device_reset (struct scsi_cmnd *cmd) +{ +int target = cmd->device->id; +imu_warn ("Received eh device reset for target %d\n" , target); +return SUCCESS ; +} + +static int imu_eh_host_reset (struct scsi_cmnd *cmd)
drivers/scsi/iop13xx-imu-scsi.c line 359
Quoted 10 lines
drivers/scsi/iop13xx-imu-scsi.c new file +10
-0+{ +imu_warn ("Received eh host reset\n" ); +return SUCCESS ; +} + +static int imu_eh_bus_reset (struct scsi_cmnd *cmd) +{ +imu_warn ("Received eh bus reset\n" ); +return SUCCESS ; +}
This lot all look really wrong. All you're doing is printing a message and then telling the mid-layer the device is recovered, when in fact you've done nothing to alleviate the error condition. The mid-layer's next action will be to send down a TUR to check the device condition, which will likely fail and continue on to offline the device.
James
RE: [PATCH RFC 2/2] iop13xx: imu scsi driver
James, thank you for the corrections. I think I can fix all of these as suggested and resubmit a new patch shortly.
Quoted 4 lines
Another thing that strikes me is on the ARM side, it looks like this is effectively vectoring up to a user space target ... is that true? And if so, would the existing in-kernel target infrastructure be a better way of doing this?
No, there's no user space target. The target is special hardware. Or more precisely, special communication hardware to send messages to off-core hardware.
Quoted 12 lines
Quoted 1 line
+static int imu_queuecommand(struct
scsi_cmnd*scp,[...]
drivers/scsi/iop13xx-imu-scsi.c line 237
Quoted 5 lines
drivers/scsi/iop13xx-imu-scsi.c new file +5
-0+if (!imu_scsi->connection_handle) { + scp->result = (DID_RESET <<16 ) | (SUGGEST_RETRY <<24 ); +done (scp); +return SCSI_MLQUEUE_HOST_BUSY ; + }Absolutely wrong. Either you execute
done()on the command and return zero, or you *don't* execute done on the command and return a queueing condition. Doing both will lead to double completions and really horrible things.
I'll return zero like the previous error condition, thanks.
Quoted 1 line
...wrong...
Other corrections greatly appreciated and I'll roll into next patch.
--Greg
Quoted 7 lines · On Wed, 2007-01-24 at 11:22 -0700, [email protected] wrote:
The imu (inter-processor messaging unit) on the iop13xx enables mailbox style communication between the two cores on iop13xx. Greg Tucker has has wrapped this capability in a scsi driver which allows Linux to interface with the other core as if it were a scsi disk target. I would like to submit it for 2.6.21 consideration. As it is an ARM-platform-specific driver does it fall under Russell's purview, or James' since it is a scsi driver.
Both of ours, I'm afraid ... as in Russell is the sign off for the arm pieces and I'll do the SCSI bits.
As to logistics, we could do it split tree, or we could do cross signoffs for either the SCSI or the ARM tree.
James