[parisc-linux-cvs] 64-bit cleanup for the sim700 driver.

[Ryan Bradetich]

On Tue, Mar 20, 2001 at 05:49:32PM -0700, Grant Grundler wrote:
> 
> Ryan Bradetich wrote:
> ...
> >  #if 0	/* test1_src/dst need to be in targdata if we ever use them */
> >  	patch_add_32 (targdata->script, 0, test1_src, 
> > -    		(u32)&(hostbus->test1_src));
> > +    		low32(&(hostbus->test1_src));)
> >  	patch_add_32 (targdata->script, 0, test1_dst, 
> > -    		(u32)&(hostbus->test1_dst));
> > +    		low32(&(hostbus->test1_dst)));
> >  #endif
> 
> I was wondering how the ';)' above compiled and then noticed it wasn't.
> It would be nice if it were correct though in case it gets copied to
> another location and used.

Thanks! Good eye.  Fixed in my local tree for the next patch update :)
 
> ...
> >  	targdata->din_script[MAX_SG*2+1] =
> > -		cpu_to_ncr32((u32)(targdata->ba->script + Ent_end_data_trans/4)
> >   );
> > +		cpu_to_ncr32(low32(targdata->ba->script + Ent_end_data_trans/4)
> >   );
> 
> Some code uses sizeof(u32) and other '4'.
> I can't say "sizeof(u32)" is more "correct" than hardcoding '4'.
> My preference is just one of those forms be used though unless there
> is a sematic difference (which doesn't seem to be the case).

Richard, does it make sense to replace the '4's with sizeof(32)'s?  If it
makes sense, I would be glad to make the changes and submit the patch.
If it does not make sense, can we use a define to replace the magic number
for better readability?
 
> ...
> > @@ -888,7 +887,7 @@ handle_phase_mismatch (struct Scsi_Host 
> >      unsigned char sbcl;
> >  
> >      sbcl = NCR_read8(SBCL_REG) & SBCL_PHASE_MASK;
> > -    index = (u32)((u32 *)(NCR_read32(DSP_REG)) - targdata->ba->script);
> > +    index = low32((u32 *)(unsigned long)NCR_read32(DSP_REG) - targdata->ba->
> >   script);
> 
> I'm abit confused by the casts. Is this meant?
> 	index = NCR_read32(DSP_REG) - low32(targdata->ba->script);

I thought the same thing at first .... cost me several hours of debugging time too :(
Here is the error message I was seeing under the 32-bit kernel when I reduced the
casting to your (and my previous) suggestion.

sim700: WARNING IRQ probe failed, (returned 0)
scsi1: Good, target data areas are dma coherent
scsi1: test 1 completed ok.
scsi1 : LASI/Simple 53c7xx
scsi1: Unexpected phase change to STATUSIN at index 0x8b4  <----
  Vendor: TOSHIBA   Model: CD-ROM XM-5701TA  Rev: 1557
  Type:   CD-ROM                             ANSI SCSI revision: 02

[snip]

Detected scsi CD-ROM sr0 at scsi1, channel 0, id 2, lun 0
scsi1: Unexpected phase change to STATUSIN at index 0x8b4  <----
scsi1: Unexpected phase change to STATUSIN at index 0x8b4  <----
Uniform CD-ROM driver Revision: 3.12

I left in this nasty casting stuff to avoid this error under 32-bit
kernels :(

> ...
> > @@ -1243,8 +1242,8 @@ sim700_intr_handle(int irq, void *dev_id
> >  	    printk("scsi%d: Int %d, istat %02x, sstat0 %02x "
> >  		"dstat %02x, dsp [%04x], target %x, dsps %08x\n",
> >  		host->host_no, sim700_intrs, istat, sstat0, dstat,
> > -		(u32 *)(NCR_read32(DSP_REG)) - targdata->ba->script,
> > -		target, dsps);
> > +		low32((u32 *)(unsigned long)NCR_read32(DSP_REG) - 
> > +	  	(unsigned long)targdata->ba->script), target, dsps);
> 
> Again, similar construct with casts.
> 
> ...
> > @@ -1255,7 +1254,7 @@ sim700_intr_handle(int irq, void *dev_id
> >  	    resume_offset = Ent_reselect;
> >  	}
> >  	else if (hostdata->chip == 700 && (dstat & DSTAT_ABRT)) {
> > -	    u32 offset = (dsp - (u32)targdata->ba->script)/4;
> > +	    u32 offset = (dsp - low32(targdata->ba->script)/4);
> 
> This looks wrong to me. Is it? (parenthesis moved)

it looks correct to me ... but let me macro-expand it to make sure we are not
missing anything:

32-bit kernel
low32(x) --> ((u32)(x))
	u32 offset = (dsp - ((u32)(targdata->ba->script))/4);

64-bit kernel
low32(x) --> ((u32)(unsigned long)(x))
	 u32 offset = (dsp - ((u32)(unsigned long)(targdata->ba->script))/4);

Still looks correct to me.  If it still seems wrong, lets discuss it further,
becuase I'm missing it :(

> Ok...had enough for now...

Thanks for the review Grant!  Hopefully this email will hit the list ... playing
with mutt now instead of my normal mail client :)

- Ryan
 
> grant
> 
> Grant Grundler
> parisc-linux {PCI|IOMMU|SMP} hacker
> +1.408.447.7253
>