public inbox for linux-m68k@lists.linux-m68k.org
 help / color / mirror / Atom feed
From: Michael Schmitz <schmitzmic@gmail.com>
To: Finn Thain <fthain@telegraphics.com.au>
Cc: Christoph Hellwig <hch@infradead.org>,
	Michael Schmitz <schmitz@debian.org>,
	Sam Creasey <sammy@sammy.net>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Linux/m68k <linux-m68k@vger.kernel.org>
Subject: Re: converting the NCR5380 drivers away from scsi_register
Date: Tue, 05 Aug 2014 21:06:54 +1200	[thread overview]
Message-ID: <53E09EAE.70308@gmail.com> (raw)
In-Reply-To: <alpine.LNX.2.00.1408041146400.21367@nippy.intranet>

Hi Finn,
> On Sun, 3 Aug 2014, Michael Schmitz wrote:
>
>   
>>> NCR5380.c lacks support for tagged queueing, while sun3_NCR5380.c and 
>>> atari_NCR5380.c have divergent implementations of this.
>>>   
>>>       
>> Might be equivalent for all I've seen - just array vs. bitmap to store 
>> tags..
>>     
>
> It is messier than that. setup_use_tagged_queuing is disabled by default 
> on atari, and enabled by default on sun3. If we don't disable it by 
>   

It's configured but disabled by default on Atari because command_per_lun 
is set to 1 by default. Can't remember why that was exactly - my best 
guess would be that queueing up more commands in the lowlevel driver did 
use to exacerbate timeout problems. But that was over a decade ago. On 
Sun3, the code is not even configured from what I've seen (SUPPORT_TAGS 
not defined).

> default everywhere then we need to resolve a change to the algorithm: the 
> following code appears at the beginning of NCR5380_reselect() in 
> atari_NCR5380.c but appears at the end of that routine in sun3_NCR5380.c. 
>   

That's what I meant to say before - sun3_NCR5380.c does peek at the 
message byte without invoking NCR5380_transfer_pio() (needs to manually 
set/clear ACK for that resason, finds the disconnected command at issue, 
does some DMA setup stuff for that command, and then goes ahead to fetch 
the tag message bytes if the phase is still at message in. No idea where 
the variable 'phase' gets set in that routine so it might never get to 
read tag messages.

atari_NCR5390.c does things a bit different - instead of peeking at the 
message byte, NCR5380_transfer_pio() is used for the first message byte 
(ACK set there, cleared manually later). NCR5380_transfer_pio() sets the 
current phase, so the subsequent transfer of the tag message may work 
there (again ACK raised in NCR5380_transfer_pio(), cleared manually later).
> I still have to try to figure out the implications of this change.
>   

 From what I've checked both should work, but the sun3 code never sets 
'phase' so it should never actually have worked. It should not even 
compile.
> #ifdef SUPPORT_TAGS
>     /* 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
>
>   
>> Changes to NCR5380. are rather major though. I need to carefully go 
>> through the diff again. ISTR trying to run the coroutine as delayed 
>> workqueue instead of immediate but dropped that again for some reason.
>>     
>
> I don't think it makes sense to unify NCR5380.c and atari_NCR5380.c at 
> this stage. The ISA cards' requirements together are more peculiar than, 
> say, ST-DMA, and are mostly irrelevant to the M68k, ARM and PCI devices.
>   

OK, not for today then.

> As an aside, forking NCR5380.c into atari_NCR5380.c, sun3_NCR5380.c and 
> mac_NCR5380.c (deleted in v2.6.3) was clearly misguided as it never 
> actually produced a good alternative to NCR5380.c.
>   

atari_NCR5380.c was forked at a time when the m68k port was developed 
entirely independent from the main Linux development (which was i386 
only at that stage). Linus did not want to deal with other 
architectures, and merging m68k code into Linus' code was not something 
the m68k authors envisaged at that stage. The fork was not meant to 
produce a good alternative to the main driver, just something that 
worked. At least that is as I recall it - I had little to do with Linux 
kernel development at that stage.
Merging ongoing development on NCR5380.c into the Atari driver was 
always something of an afterthought (Geert probably took care of most of 
that). The sun3 and Mac drivers were then forked off the Atari one. I'm 
guilty in part of the initial Mac one, but that was done mainly in order 
to stop Alan Cox from whinging about the lack of a disk driver. (Don't 
think he liked SCSI in PIO mode much.)

What I mean to say - in hindsight, it was misguided, but at the time we 
did not have the benefit of a crystal ball (or git, or even fast cross 
compilers).

> Forking just multiplied the maintenance demands, and diminishing man-power 
> of course lead to neglect and breakage. So I can sympathise with 
> Christoph's comment in NCR5380.c:
>
>  * (Note from hch:  unfortunately it was not enough for the different
>  * m68k folks and instead of improving this driver they copied it
>  * and hacked it up for their needs.  As a consequence they lost
>  * most updates to this driver.  Maybe someone will fix all these
>  * drivers to use a common core one day..)
>   

Yep, I can now sympathise with that. When m68k began to merge with 
Linus, we had already lost much of the initial man-power and expertise. 
I'm sure we'd do things different now, but these lessons had to be 
learned along the way.

Cheers,

    Michael

  reply	other threads:[~2014-08-05  9:07 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-06-16 14:18 converting the NCR5380 drivers away from scsi_register Christoph Hellwig
2014-06-17  8:20 ` Finn Thain
2014-06-17  8:38   ` Geert Uytterhoeven
2014-07-30  8:32     ` Finn Thain
2014-07-31  5:31       ` Michael Schmitz
2014-08-01  3:13         ` Finn Thain
2014-08-01  8:15           ` Michael Schmitz
2014-08-02  1:27             ` Finn Thain
2014-08-02  8:51               ` Michael Schmitz
2014-08-03  3:43                 ` Finn Thain
2014-08-03  9:07                   ` Michael Schmitz
2014-08-04  3:28                     ` Finn Thain
2014-08-05  9:06                       ` Michael Schmitz [this message]
2014-08-06  1:25                         ` Sun3 SCSI DMA, was " Finn Thain
2014-08-06 14:42                           ` Sam Creasey
2014-08-08  8:46                             ` Michael Schmitz
2014-08-11 15:10                               ` Sam Creasey
2014-08-13  5:29                                 ` Finn Thain
2014-08-13  9:14                                   ` Michael Schmitz
2014-08-14  1:43                                     ` Finn Thain
2014-08-14  8:57                                       ` Michael Schmitz
2014-08-15  1:46                                         ` Finn Thain
2014-08-15  1:51                                           ` Michael Schmitz
2014-08-15  2:09                                         ` Finn Thain
2014-08-15  3:03                                           ` Michael Schmitz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=53E09EAE.70308@gmail.com \
    --to=schmitzmic@gmail.com \
    --cc=fthain@telegraphics.com.au \
    --cc=geert@linux-m68k.org \
    --cc=hch@infradead.org \
    --cc=linux-m68k@vger.kernel.org \
    --cc=sammy@sammy.net \
    --cc=schmitz@debian.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox