On 10/31/2014 08:21 AM, Finn Thain wrote:
>
> On Thu, 30 Oct 2014, Hannes Reinecke wrote:
>
>> On 10/27/2014 06:26 AM, Finn Thain wrote:
>>> There is very little difference between the sun3_NCR5380.c core driver
>>> and atari_NCR5380.c. The former is a fork of the latter.
>>>
>>> Merge the sun3_NCR5380.c core driver into atari_NCR5380.c so that
>>> sun3_scsi.c can adopt the latter and the former can be deleted.
>>>
>>> Signed-off-by: Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx>
>>>
>>> ---
>>>
>>> This patch brings some undesirable #ifdef's to atari_NCR5380. It's a
>>> small price to pay for the elimination of so much duplication.
>>> Eventually, with some testing, it should be possible to remove many of
>>> these.
>>>
>>> Elimination of the SUN3_SCSI_VME macro is necessary to reach the goal
>>> of a single platform driver to replace the
>>> {atari,mac,sun3,sun3_vme}_scsi modules.
>>>
>>> Elimination of the #ifdef CONFIG_SUN3 tests is not necessary. But some
>>> of these would go away if the sun3 driver followed the atari logic
>>> more closely, such as use of PIO vs DMA for certain phases and
>>> transfer sizes.
>>>
>>> After applying this patch it is easy to compare the new
>>> atari_NCR5380.c with sun3_NCR5380.c, to check that they are
>>> equivalent. E.g.
>>>
>>> $ cp drivers/scsi/{sun3,atari}_NCR5380.c /tmp
>>> $ sed -i -e 's/ *$//' /tmp/{sun3,atari}_NCR5380.c
>>> $ scripts/Lindent -l96 -hnl /tmp/{sun3,atari}_NCR5380.c
>>> $ meld /tmp/{sun3,atari}_NCR5380.c
>>>
>>> One can see that the two implementations of tagged command queueing
>>> have diverged since the fork. The atari version has been updated and
>>> the sun3 implementation is unused as sun3_scsi.c does not define
>>> SUPPORT_TAGS.
>>>
>>> The remaining differences are merge_contiguous_buffers() and the
>>> falcon locking code. The falcon code disappears when suitable macro
>>> definitions are provided in sun3_scsi.c. merge_contiguous_buffers()
>>> could be a problem for sun3 (due to a DMA setup in an odd place) so it
>>> is #ifdef'd for sun3 until the DMA setup discrepancies can be
>>> reconciled or moved out of the core driver.
>>>
>>> ---
>>> drivers/scsi/atari_NCR5380.c | 210 +++++++++++++++++++++++++++++++++++++++----
>>> drivers/scsi/atari_scsi.c | 1
>>> 2 files changed, 195 insertions(+), 16 deletions(-)
>>>
>>> Index: linux/drivers/scsi/atari_NCR5380.c
>>> ===================================================================
>>> --- linux.orig/drivers/scsi/atari_NCR5380.c 2014-10-27 16:25:55.000000000 +1100
>>> +++ linux/drivers/scsi/atari_NCR5380.c 2014-10-27 16:25:56.000000000 +1100
>>> @@ -71,6 +71,9 @@
>>> * 1. Test linked command handling code after Eric is ready with
>>> * the high level code.
>>> */
>>> +
>>> +/* Adapted for the sun3 by Sam Creasey. */
>>> +
>>> #include <scsi/scsi_dbg.h>
>>> #include <scsi/scsi_transport_spi.h>
>>>
>>> @@ -458,6 +461,7 @@ static void free_all_tags(void)
>>>
>>> static void merge_contiguous_buffers(struct scsi_cmnd *cmd)
>>> {
>>> +#if !defined(CONFIG_SUN3)
>>> unsigned long endaddr;
>>> #if (NDEBUG & NDEBUG_MERGING)
>>> unsigned long oldlen = cmd->SCp.this_residual;
>>> @@ -482,6 +486,7 @@ static void merge_contiguous_buffers(str
>>> dprintk(NDEBUG_MERGING, "merged %d buffers from %p, new length %08x\n",
>>> cnt, cmd->SCp.ptr, cmd->SCp.this_residual);
>>> #endif
>>> +#endif /* !defined(CONFIG_SUN3) */
>>> }
>>>
>>> /*
>>> @@ -1043,12 +1048,10 @@ static void NCR5380_main(struct work_str
>>> prev = NULL; tmp; prev = tmp, tmp = NEXT(tmp)) {
>>> u8 lun = tmp->device->lun;
>>>
>>> -#if (NDEBUG & NDEBUG_LISTS)
>>> - if (prev != tmp)
>>> - printk("MAIN tmp=%p target=%d busy=%d lun=%llu\n",
>>> - tmp, tmp->device->id, hostdata->busy[tmp->device->id],
>>> - lun);
>>> -#endif
>>> + dprintk(NDEBUG_LISTS,
>>> + "MAIN tmp=%p target=%d busy=%d lun=%d\n",
>>> + tmp, scmd_id(tmp), hostdata->busy[scmd_id(tmp)],
>>> + lun);
>>> /* When we find one, remove it from the issue queue. */
>>> /* ++guenther: possible race with Falcon locking */
>>> if (
>>> @@ -1157,9 +1160,11 @@ static void NCR5380_main(struct work_str
>>> static void NCR5380_dma_complete(struct Scsi_Host *instance)
>>> {
>>> SETUP_HOSTDATA(instance);
>>> - int transfered, saved_data = 0, overrun = 0, cnt, toPIO;
>>> - unsigned char **data, p;
>>> + int transfered;
>>> + unsigned char **data;
>>> volatile int *count;
>>> + int saved_data = 0, overrun = 0;
>>> + unsigned char p;
>>>
>>> if (!hostdata->connected) {
>>> printk(KERN_WARNING "scsi%d: received end of DMA interrupt with "
>>
>> nitpick: shouldn't that be 'transferred' ?
>
>
> The intention of this patch was to merge two files into one file, in such
> a way that the result could be easily compared with the originals.
>
> With that in mind, fixing this typo could be done pre-merger by patching
> both files, or post-merger by patching just one.
>
> If this patch series ends up iterating again as v3, I'll add a new patch
> to the end to fix this (long standing) typo.
>
> If there's no need for a re-spin, I'll send a separate patch with Cc to
> trivial@xxxxxxxxxx.
>
Okay, fair enough.
No need to clean it up now.
>
>>
>>> @@ -1185,6 +1190,26 @@ static void NCR5380_dma_complete(struct
>>> HOSTNO, NCR5380_read(BUS_AND_STATUS_REG),
>>> NCR5380_read(STATUS_REG));
>>>
>>> +#if defined(CONFIG_SUN3)
>>> + if ((sun3scsi_dma_finish(rq_data_dir(hostdata->connected->request)))) {
>>> + pr_err("scsi%d: overrun in UDC counter -- not prepared to deal with this!\n",
>>> + instance->host_no);
>>> + pr_err("please e-mail sammy@xxxxxxxxx with a description of how this error was produced.\n");
>>> + BUG();
>>> + }
>>> +
>>> + /* make sure we're not stuck in a data phase */
>>> + if ((NCR5380_read(BUS_AND_STATUS_REG) & (BASR_PHASE_MATCH | BASR_ACK)) ==
>>> + (BASR_PHASE_MATCH | BASR_ACK)) {
>>> + pr_err("scsi%d: BASR %02x\n", instance->host_no,
>>> + NCR5380_read(BUS_AND_STATUS_REG));
>>> + pr_err("scsi%d: bus stuck in data phase -- probably a single byte overrun!\n",
>>> + instance->host_no);
>>> + pr_err("not prepared for this error! please e-mail sammy@xxxxxxxxx with a description of how this error was produced.\n");
>>> + BUG();
>>> + }
>>> +#endif
>>> +
>>> (void)NCR5380_read(RESET_PARITY_INTERRUPT_REG);
>>> NCR5380_write(MODE_REG, MR_BASE);
>>> NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
>>> @@ -1198,6 +1223,8 @@ static void NCR5380_dma_complete(struct
>>> *count -= transfered;
>>>
>>> if (hostdata->read_overruns) {
>>> + int cnt, toPIO;
>>> +
>>> if ((NCR5380_read(STATUS_REG) & PHASE_MASK) == p && (p & SR_IO)) {
>>> cnt = toPIO = hostdata->read_overruns;
>>> if (overrun) {
>>> @@ -1277,11 +1304,14 @@ static irqreturn_t NCR5380_intr(int irq,
>>> {
>>> /* MS: Ignore unknown phase mismatch interrupts (caused by EOP interrupt) */
>>> if (basr & BASR_PHASE_MATCH)
>>> - printk(KERN_NOTICE "scsi%d: unknown interrupt, "
>>> + dprintk(NDEBUG_INTR, "scsi%d: unknown interrupt, "
>>> "BASR 0x%x, MR 0x%x, SR 0x%x\n",
>>> HOSTNO, basr, NCR5380_read(MODE_REG),
>>> NCR5380_read(STATUS_REG));
>>> (void)NCR5380_read(RESET_PARITY_INTERRUPT_REG);
>>> +#ifdef SUN3_SCSI_VME
>>> + dregs->csr |= CSR_DMA_ENABLE;
>>> +#endif
>>> }
>>> } /* if !(SELECTION || PARITY) */
>>> handled = 1;
>>> @@ -1290,6 +1320,9 @@ static irqreturn_t NCR5380_intr(int irq,
>>> "BASR 0x%X, MR 0x%X, SR 0x%x\n", HOSTNO, basr,
>>> NCR5380_read(MODE_REG), NCR5380_read(STATUS_REG));
>>> (void)NCR5380_read(RESET_PARITY_INTERRUPT_REG);
>>> +#ifdef SUN3_SCSI_VME
>>> + dregs->csr |= CSR_DMA_ENABLE;
>>> +#endif
>>> }
>>>
>>> if (!done) {
>>> @@ -1622,6 +1655,9 @@ static int NCR5380_select(struct Scsi_Ho
>>> #ifndef SUPPORT_TAGS
>>> hostdata->busy[cmd->device->id] |= (1 << cmd->device->lun);
>>> #endif
>>> +#ifdef SUN3_SCSI_VME
>>> + dregs->csr |= CSR_INTR;
>>> +#endif
>>>
>>> initialize_SCp(cmd);
>>>
>>> @@ -1845,9 +1881,54 @@ static int NCR5380_transfer_dma(struct S
>>> SETUP_HOSTDATA(instance);
>>> register int c = *count;
>>> register unsigned char p = *phase;
>>> + unsigned long flags;
>>> +
>>> +#if defined(CONFIG_SUN3)
>>> + /* sanity check */
>>> + if (!sun3_dma_setup_done) {
>>> + pr_err("scsi%d: transfer_dma without setup!\n",
>>> + instance->host_no);
>>> + BUG();
>>> + }
>>> + hostdata->dma_len = c;
>>> +
>>> + dprintk(NDEBUG_DMA, "scsi%d: initializing DMA for %s, %d bytes %s %p\n",
>>> + instance->host_no, (p & SR_IO) ? "reading" : "writing",
>>> + c, (p & SR_IO) ? "to" : "from", *data);
>>> +
>>> + /* netbsd turns off ints here, why not be safe and do it too */
>>> + local_irq_save(flags);
>>> +
>>> + /* send start chain */
>>> + sun3scsi_dma_start(c, *data);
>>> +
>>> + if (p & SR_IO) {
>>> + NCR5380_write(TARGET_COMMAND_REG, 1);
>>> + NCR5380_read(RESET_PARITY_INTERRUPT_REG);
>>> + NCR5380_write(INITIATOR_COMMAND_REG, 0);
>>> + NCR5380_write(MODE_REG,
>>> + (NCR5380_read(MODE_REG) | MR_DMA_MODE | MR_ENABLE_EOP_INTR));
>>> + NCR5380_write(START_DMA_INITIATOR_RECEIVE_REG, 0);
>>> + } else {
>>> + NCR5380_write(TARGET_COMMAND_REG, 0);
>>> + NCR5380_read(RESET_PARITY_INTERRUPT_REG);
>>> + NCR5380_write(INITIATOR_COMMAND_REG, ICR_ASSERT_DATA);
>>> + NCR5380_write(MODE_REG,
>>> + (NCR5380_read(MODE_REG) | MR_DMA_MODE | MR_ENABLE_EOP_INTR));
>>> + NCR5380_write(START_DMA_SEND_REG, 0);
>>> + }
>>> +
>>> +#ifdef SUN3_SCSI_VME
>>> + dregs->csr |= CSR_DMA_ENABLE;
>>> +#endif
>>> +
>>> + local_irq_restore(flags);
>>> +
>>> + sun3_dma_active = 1;
>>> +
>>> +#else /* !defined(CONFIG_SUN3) */
>>> register unsigned char *d = *data;
>>> unsigned char tmp;
>>> - unsigned long flags;
>>>
>>> if ((tmp = (NCR5380_read(STATUS_REG) & PHASE_MASK)) != p) {
>>> *phase = tmp;
>>> @@ -1895,6 +1976,8 @@ static int NCR5380_transfer_dma(struct S
>>> NCR5380_dma_write_setup(instance, d, c);
>>> local_irq_restore(flags);
>>> }
>>> +#endif /* !defined(CONFIG_SUN3) */
>>> +
>>> return 0;
>>> }
>>> #endif /* defined(REAL_DMA) */
>>> @@ -1930,6 +2013,10 @@ static void NCR5380_information_transfer
>>> unsigned char phase, tmp, extended_msg[10], old_phase = 0xff;
>>> struct scsi_cmnd *cmd = (struct scsi_cmnd *) hostdata->connected;
>>>
>>> +#ifdef SUN3_SCSI_VME
>>> + dregs->csr |= CSR_INTR;
>>> +#endif
>>> +
>>> while (1) {
>>> tmp = NCR5380_read(STATUS_REG);
>>> /* We only have a valid SCSI phase when REQ is asserted */
>>> @@ -1939,6 +2026,33 @@ static void NCR5380_information_transfer
>>> old_phase = phase;
>>> NCR5380_dprint_phase(NDEBUG_INFORMATION, instance);
>>> }
>>> +#if defined(CONFIG_SUN3)
>>> + if (phase == PHASE_CMDOUT) {
>>> +#if defined(REAL_DMA)
>>> + void *d;
>>> + unsigned long count;
>>> +
>>> + if (!cmd->SCp.this_residual && cmd->SCp.buffers_residual) {
>>> + count = cmd->SCp.buffer->length;
>>> + d = sg_virt(cmd->SCp.buffer);
>>> + } else {
>>> + count = cmd->SCp.this_residual;
>>> + d = cmd->SCp.ptr;
>>> + }
>>> + /* this command setup for dma yet? */
>>> + if ((count >= DMA_MIN_SIZE) && (sun3_dma_setup_done != cmd)) {
>>> + if (cmd->request->cmd_type == REQ_TYPE_FS) {
>>> + sun3scsi_dma_setup(d, count,
>>> + rq_data_dir(cmd->request));
>>> + sun3_dma_setup_done = cmd;
>>> + }
>>> + }
>>> +#endif
>>> +#ifdef SUN3_SCSI_VME
>>> + dregs->csr |= CSR_INTR;
>>> +#endif
>>> + }
>>> +#endif /* CONFIG_SUN3 */
>>>
>>> if (sink && (phase != PHASE_MSGOUT)) {
>>> NCR5380_write(TARGET_COMMAND_REG, PHASE_SR_TO_TCR(tmp));
>>> @@ -2000,8 +2114,11 @@ static void NCR5380_information_transfer
>>> */
>>>
>>> #if defined(REAL_DMA)
>>> - if (!cmd->device->borken &&
>>> - (transfersize = NCR5380_dma_xfer_len(instance,cmd,phase)) > 31) {
>>> + if (
>>> +#if !defined(CONFIG_SUN3)
>>> + !cmd->device->borken &&
>>> +#endif
>>> + (transfersize = NCR5380_dma_xfer_len(instance, cmd, phase)) >= DMA_MIN_SIZE) {
>>> len = transfersize;
>>> cmd->SCp.phase = phase;
>>> if (NCR5380_transfer_dma(instance, &phase,
>>> @@ -2038,6 +2155,11 @@ static void NCR5380_information_transfer
>>> NCR5380_transfer_pio(instance, &phase,
>>> (int *)&cmd->SCp.this_residual,
>>> (unsigned char **)&cmd->SCp.ptr);
>>> +#if defined(CONFIG_SUN3) && defined(REAL_DMA)
>>> + /* if we had intended to dma that command clear it */
>>> + if (sun3_dma_setup_done == cmd)
>>> + sun3_dma_setup_done = NULL;
>>> +#endif
>>> break;
>>> case PHASE_MSGIN:
>>> len = 1;
>>> @@ -2243,6 +2365,9 @@ static void NCR5380_information_transfer
>>> /* Wait for bus free to avoid nasty timeouts */
>>> while ((NCR5380_read(STATUS_REG) & SR_BSY) && !hostdata->connected)
>>> barrier();
>>> +#ifdef SUN3_SCSI_VME
>>> + dregs->csr |= CSR_DMA_ENABLE;
>>> +#endif
>>> return;
>>> /*
>>> * The SCSI data pointer is *IMPLICITLY* saved on a disconnect
>>> @@ -2402,17 +2527,20 @@ static void NCR5380_information_transfer
>>> */
>>>
>>>
>>> +/* it might eventually prove necessary to do a dma setup on
>>> + reselection, but it doesn't seem to be needed now -- sam */
>>> +
>>> static void NCR5380_reselect(struct Scsi_Host *instance)
>>> {
>>> SETUP_HOSTDATA(instance);
>>> unsigned char target_mask;
>>> - unsigned char lun, phase;
>>> - int len;
>>> + unsigned char lun;
>>> #ifdef SUPPORT_TAGS
>>> unsigned char tag;
>>> #endif
>>> unsigned char msg[3];
>>> - unsigned char *data;
>>> + int __maybe_unused len;
>>> + unsigned char __maybe_unused *data, __maybe_unused phase;
>>> struct scsi_cmnd *tmp = NULL, *prev;
>>>
>>> /*
>>> @@ -2449,10 +2577,18 @@ static void NCR5380_reselect(struct Scsi
>>> while (!(NCR5380_read(STATUS_REG) & SR_REQ))
>>> ;
>>>
>>> +#if defined(CONFIG_SUN3) && defined(REAL_DMA)
>>> + /* acknowledge toggle to MSGIN */
>>> + NCR5380_write(TARGET_COMMAND_REG, PHASE_SR_TO_TCR(PHASE_MSGIN));
>>> +
>>> + /* peek at the byte without really hitting the bus */
>>> + msg[0] = NCR5380_read(CURRENT_SCSI_DATA_REG);
>>> +#else
>>> len = 1;
>>> data = msg;
>>> phase = PHASE_MSGIN;
>>> NCR5380_transfer_pio(instance, &phase, &len, &data);
>>> +#endif
>>>
>>> if (!(msg[0] & 0x80)) {
>>> printk(KERN_DEBUG "scsi%d: expecting IDENTIFY message, got ", HOSTNO);
>>> @@ -2462,7 +2598,7 @@ static void NCR5380_reselect(struct Scsi
>>> }
>>> lun = (msg[0] & 0x07);
>>>
>>> -#ifdef SUPPORT_TAGS
>>> +#if defined(SUPPORT_TAGS) && !defined(CONFIG_SUN3)
>>> /* If the phase is still MSGIN, the target wants to send some more
>>> * messages. In case it supports tagged queuing, this is probably a
>>> * SIMPLE_QUEUE_TAG for the I_T_L_Q nexus.
>>> @@ -2524,9 +2660,51 @@ static void NCR5380_reselect(struct Scsi
>>> return;
>>> }
>>>
>>> +#if defined(CONFIG_SUN3) && defined(REAL_DMA)
>>> + /* engage dma setup for the command we just saw */
>>> + {
>>> + void *d;
>>> + unsigned long count;
>>> +
>>> + if (!tmp->SCp.this_residual && tmp->SCp.buffers_residual) {
>>> + count = tmp->SCp.buffer->length;
>>> + d = sg_virt(tmp->SCp.buffer);
>>> + } else {
>>> + count = tmp->SCp.this_residual;
>>> + d = tmp->SCp.ptr;
>>> + }
>>> + /* setup this command for dma if not already */
>>> + if ((count >= DMA_MIN_SIZE) && (sun3_dma_setup_done != tmp)) {
>>> + sun3scsi_dma_setup(d, count, rq_data_dir(tmp->request));
>>> + sun3_dma_setup_done = tmp;
>>> + }
>>> + }
>>> +
>>> + NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE | ICR_ASSERT_ACK);
>>> +#endif
>>> +
>>> /* Accept message by clearing ACK */
>>> NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
>>>
>>> +#if defined(SUPPORT_TAGS) && defined(CONFIG_SUN3)
>>> + /* If the phase is still MSGIN, the target wants to send some more
>>> + * messages. In case it supports tagged queuing, this is probably a
>>> + * SIMPLE_QUEUE_TAG for the I_T_L_Q nexus.
>>> + */
>>> + tag = TAG_NONE;
>>> + if (phase == PHASE_MSGIN && setup_use_tagged_queuing) {
>>> + /* Accept previous IDENTIFY message by clearing ACK */
>>> + NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
>>> + len = 2;
>>> + data = msg + 1;
>>> + if (!NCR5380_transfer_pio(instance, &phase, &len, &data) &&
>>> + msg[1] == SIMPLE_QUEUE_TAG)
>>> + tag = msg[2];
>>> + dprintk(NDEBUG_TAGS, "scsi%d: target mask %02x, lun %d sent tag %d at reselection\n"
>>> + HOSTNO, target_mask, lun, tag);
>>> + }
>>> +#endif
>>> +
>>> hostdata->connected = tmp;
>>> dprintk(NDEBUG_RESELECTION, "scsi%d: nexus established, target = %d, lun = %llu, tag = %d\n",
>>> HOSTNO, tmp->device->id, tmp->device->lun, tmp->tag);
>>> Index: linux/drivers/scsi/atari_scsi.c
>>> ===================================================================
>>> --- linux.orig/drivers/scsi/atari_scsi.c 2014-10-27 16:25:55.000000000 +1100
>>> +++ linux/drivers/scsi/atari_scsi.c 2014-10-27 16:25:56.000000000 +1100
>>> @@ -89,6 +89,7 @@
>>> #define REAL_DMA
>>> #define SUPPORT_TAGS
>>> #define MAX_TAGS 32
>>> +#define DMA_MIN_SIZE 32
>>>
>>> #define NCR5380_implementation_fields /* none */
>>>
>>>
>>>
>> You reference a certain Sam Creasey in the code (and there is even
>> his email in it). Has he contributed to this patch?
>
> No, the patch is my work.
>
> As I understand it, sun3_NCR5380.c was originally Sam's (derived) work.
>
>
>> If so, shouldn't he be added somewhere in the patch description,
>> either with 'From;' (if the patch was originally by him)
>> or with a 'Signed-off-by' tag?
>
> I did Cc these patches to Sam, for comment/testing; perhaps a Cc tag might
> have been appropriate but I gather they are optional.
>
Hmm. I always feel a bit awkward by using personal names in the code.
It's not good practice nowadays. Plus one can never be sure if that
address is still valid. Or remains so for the foreseeable future.
But hey, these are just my personal opinions.
Reviewed-by: Hannes Reinecke <hare@xxxxxxx>
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@xxxxxxx +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
