sis.patch.20020123_1

sis.patch.20020123_1

[Martin Garton]

Lionel,

Thanks for releasing your patch. I have tested it with my SiS737 chipset 
and noticed the following:

(please note - I may well not know what I am talking about)

My chipset is UDMA100 capable, but with your patch, no DMA was available 
by default (yes, I have CONFIG_BLK_DEV_IDEDMA=y)

I think I know whats wrong, but I'm not certain. I was expecting 
pci_init_sis5513() to find the device SiS735, but it finds SiS5513 
instead. (I understand that SiS735 is the host bridge, whereas SiS5513 is 
the ide interface) Is my thinking correct?

I did a nasty hack to get the device recognised as SiS735, and all is 
fine. I haven't posted my patch for this since I don't know the Right fix.

The second problem I had was an OOPS when doing "cat /proc/ide/sis" . I
think I have tracked this down, and I believe the attached patch fixes it.  
(applied on top of your patch, obviously)  (works for me, but please let
me know if it is nonsense.)

Regards,

Martin Garton.

--- linux-2.4.17-sispatches/drivers/ide/sis5513.c	Sun Jan 27 14:01:18 2002
+++ linux-2.4.17-martin/drivers/ide/sis5513.c	Sun Jan 27 14:06:25 2002
@@ -314,7 +314,8 @@
 		case ATA_16: /* confirmed */
 		case ATA_33:
 		case ATA_66: p += sprintf(p, active_time[(reg01 & 0x07) >> 4]); break;
-		case ATA_100: p += sprintf(p, active_time[(reg00 & 0x70)]); break;
+		case ATA_100: p += sprintf(p, active_time[(reg00 & 0x07)]); break;
+				      
 		case ATA_133:
 		default: p += sprintf(p, "133+ ?"); break;
 	}
@@ -324,7 +325,7 @@
 		case ATA_16:
 		case ATA_33:
 		case ATA_66: p += sprintf(p, active_time[(reg11 & 0x07) >> 4]); break;
-		case ATA_100: p += sprintf(p, active_time[(reg10 & 0x70)]); break;
+		case ATA_100: p += sprintf(p, active_time[(reg10 & 0x07)]); break;
 		case ATA_133:
 		default: p += sprintf(p, "133+ ?"); break;
 	}

Re: sis.patch.20020123_1

[Daniela Engert]

On Sun, 27 Jan 2002 14:31:28 +0000 (GMT), Martin Garton wrote:

>I think I know whats wrong, but I'm not certain. I was expecting 
>pci_init_sis5513() to find the device SiS735, but it finds SiS5513 
>instead. (I understand that SiS735 is the host bridge, whereas SiS5513 is 
>the ide interface) Is my thinking correct?

Your chipset cannot be detected by the surrent SiS IDE patch because it
takes a list based approach to find supported chips and their
capabilities rather than a more intelligent detection scheme (I've sent
Lionel code which shows how to do that). Over time (often a very short
one) a device list is missing valid members. In your case, the SiS
driver doesn't know about your SiS737 chip and it defaults to the least
common denominator - the ancient SiS5513 IDE chip.

>I did a nasty hack to get the device recognised as SiS735, and all is 
>fine. I haven't posted my patch for this since I don't know the Right fix.

You just thave to add it to the device list. There are other chips
missing as well.

>The second problem I had was an OOPS when doing "cat /proc/ide/sis" . I
>think I have tracked this down, and I believe the attached patch fixes it.  

Well, it does but at the expense of correctness ;-)

> 		case ATA_66: p += sprintf(p, active_time[(reg01 & 0x07) >> 4]); break;
>-		case ATA_100: p += sprintf(p, active_time[(reg00 & 0x70)]); break;
>+		case ATA_100: p += sprintf(p, active_time[(reg00 & 0x07)]); break;

The problem is that the calculation of the index into the active time
table is incorrect in *all* three lines above! In the ATA66 case the
shift is wrong and causes an zero value regardless of the register
setting. In the "old" ATA100 case the index is calculated from the
correct bits but is missing the shift by four from the line above;
because of the too large index you see the OOPS. The "new" ATA100 case
is wrong because it takes the wrong bits into calculation.

Ciao,
  Dani

Re: sis.patch.20020123_1

[Lionel Bouton]

On Sun, Jan 27, 2002 at 04:59:43PM +0100, Daniela Engert wrote:
> Your chipset cannot be detected by the surrent SiS IDE patch because it
> takes a list based approach to find supported chips and their
> capabilities rather than a more intelligent detection scheme (I've sent
> Lionel code which shows how to do that).

I didn't even know about SiS737 before! I guess I won't rely on SiSHostChipInfo in
v0.14.
I'll check your code next week (skying starting tomorrow).

> >I did a nasty hack to get the device recognised as SiS735, and all is 
> >fine. I haven't posted my patch for this since I don't know the Right fix.
> 
> You just thave to add it to the device list. There are other chips
> missing as well.
> 
> > 		case ATA_66: p += sprintf(p, active_time[(reg01 & 0x07) >> 4]); break;
> >-		case ATA_100: p += sprintf(p, active_time[(reg00 & 0x70)]); break;
> >+		case ATA_100: p += sprintf(p, active_time[(reg00 & 0x07)]); break;
> 
> The problem is that the calculation of the index into the active time
> table is incorrect in *all* three lines above! In the ATA66 case the
> shift is wrong and causes an zero value regardless of the register
> setting. In the "old" ATA100 case the index is calculated from the
> correct bits but is missing the shift by four from the line above;
> because of the too large index you see the OOPS. The "new" ATA100 case
> is wrong because it takes the wrong bits into calculation.

Ooops. Corrected in the last patch.

http://inet6.dyn.dhs.org/sponsoring/sis5513/sis.patch.20020128_1

LB.