[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 :)

> ...
> >  	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).

I'm not sure on this one ... Richard?  I would be glad to change them all
to sizeof(u32) if that is the correct thing to do.

> ...
> > @@ -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);

This is what I first thought also :( (It took me hours to figure out this was
my problem on 32 bit kernels.)

When I change it to your (and mine) suggested cast ... I get the following
errors in the boot sequence.

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]

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

Maybe Richard can shed some light on this, but I left it this way
because it eliminated the errors under 32-bit kernels and I 
really don't understand what it is doing anyways :)

> ...
> > @@ -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)

I think it is correct....

32-bit kernel:
the low32(x) is really ((u32)(x))
so the line would be: 
	u32 offset = (dsp - ((u32)(targdata->ba->script))/4);

64-bit kernel:
the low32(x) is really ((u32)(unsigned long)(x))
so the line would be:
	u32 offset = (dsp - ((u32)(unsigned long)(targdata->ba->script))/4);

does it still look odd?  or am I just having a major brain fart and not seeing
the problem? :^)
 
> Ok...had enough for now...

Thanks alot for the review!

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