[PATCH] Remove warnings for s2892

[Myles Watson]

Remove warnings from compilation of the s2892 with and without CBFS.

I didn't try to remove "defined but not used" warnings because there are too
many ifdefs to be sure I wouldn't break something.

For shadowed variable declarations I renamed the inner-most variable.

The one in src/pc80/keyboard.c might need help.  I didn't change the
functionality but it looks like a bug.

I boot tested it on s2892.

Abuild is about half way done.

Signed-off-by: Myles Watson

Thanks,
Myles

[ron minnich]

Thanks for this fine work.

That pc80 thing is incredible.

Assuming it passes abuild,
Acked-by: Ronald G. Minnich

[Myles Watson]

On Thu, Apr 30, 2009 at 4:38 PM, ron minnich  wrote:
> Thanks for this fine work.
>
> That pc80 thing is incredible.

Should we take it out or enable it?  It's dead code right now.
> Assuming it passes abuild,

Yes.
> Acked-by: Ronald G. Minnich

Rev 4240.

Thanks,
Myles

[Peter Stuge]

Myles Watson wrote:
> +++ cbv2/src/pc80/keyboard.c	2009-04-30 15:47:16.000000000 -0600
> @@ -112,11 +112,11 @@
>    	outb(0x60, 0x64);
>    	if (!kbc_input_buffer_empty()) return;
>    	outb(0x20, 0x60);	/* send cmd: enable keyboard and IRQ 1 */
> u8 resend = 10;
> u8 broken_resend = 10;
> if ((inb(0x64) & 0x01)) {
> 	regval = inb(0x60);
> }
> --resend;
> --broken_resend;
> } while (regval == 0xFE && resend > 0);

This does not look right to me. I think the good fix is to not rename
things but only remove the u8 (re)declaration on line 115. Very good
find!

//Peter

[Myles Watson]

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> On Behalf Of Peter Stuge
> Sent: Thursday, April 30, 2009 5:47 PM
> To: [e-mail]
> Subject: Re: [coreboot] [PATCH] Remove warnings for s2892
>
> Myles Watson wrote:
> > +++ cbv2/src/pc80/keyboard.c	2009-04-30 15:47:16.000000000 -0600
> > @@ -112,11 +112,11 @@
> >    	outb(0x60, 0x64);
> >    	if (!kbc_input_buffer_empty()) return;
> >    	outb(0x20, 0x60);	/* send cmd: enable keyboard and IRQ

1 */
> > u8 resend = 10;
> > u8 broken_resend = 10;
> > if ((inb(0x64) & 0x01)) {
> > 	regval = inb(0x60);
> > }
> > --resend;
> > --broken_resend;
> > } while (regval == 0xFE && resend > 0);
>
> This does not look right to me. I think the good fix is to not rename
> things but only remove the u8 (re)declaration on line 115.

I didn't do that because it would change the behavior and I wouldn't be able
to tell breakage by building.  I don't know enough about keyboard
controllers to test this part of the code.
> Very good
> find!

Luckily the compiler did most of the work :)

Thanks,
Myles

[Peter Stuge]

Myles Watson wrote:
> > This does not look right to me. I think the good fix is to not rename
> > things but only remove the u8 (re)declaration on line 115.
>
> I didn't do that because it would change the behavior and I wouldn't
> be able to tell breakage by building.  I don't know enough about
> keyboard controllers to test this part of the code.

The semantics of the changed code are pretty simple and looking at
other parts of the same file shows that it is a common construct.

It's used to retry keyboard communication a few times in case the
keyboard is slow or just generally crappy.

If you make a patch it is

Acked-by: Peter Stuge

:)

//Peter

[Myles Watson]

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> On Behalf Of Peter Stuge
> Sent: Thursday, April 30, 2009 6:29 PM
> To: [e-mail]
> Subject: Re: [coreboot] [PATCH] Remove warnings for s2892
>
> Myles Watson wrote:
> > > This does not look right to me. I think the good fix is to not rename
> > > things but only remove the u8 (re)declaration on line 115.
> >
> > I didn't do that because it would change the behavior and I wouldn't
> > be able to tell breakage by building.  I don't know enough about
> > keyboard controllers to test this part of the code.
>
> The semantics of the changed code are pretty simple and looking at
> other parts of the same file shows that it is a common construct.
>
> It's used to retry keyboard communication a few times in case the
> keyboard is slow or just generally crappy.
>
> If you make a patch it is
>
> Acked-by: Peter Stuge

Rev 4241.

Thanks,
Myles

[Stefan Reinauer]

Awesome patch!

On 01.05.2009 0:22 Uhr, Myles Watson wrote:
> if (!eeprom_valid) {
>    	unsigned long mac_pos;
>    	mac_pos = 0xffffffd0; /* See romstrap.inc and romstrap.lds. */
> mac_l = readl(mac_pos) + nic_index;
> mac_h = readl(mac_pos + 4);
> mac_l = readl((uint8_t*)mac_pos) + nic_index;
> mac_h = readl((uint8_t*)mac_pos + 4);
> }

One could put that slightly simpler as

if (!eeprom_valid) {
    	u8 *mac_pos;
    	mac_pos = 0xffffffd0; /* See romstrap.inc and romstrap.lds. */
    	mac_l = readl(mac_pos) + nic_index;
    	mac_h = readl(mac_pos + 4);
    }

since mac_pos seems never used as unsigned long.

[Myles Watson]

> -----Original Message-----
> From: Stefan Reinauer [mailto:[e-mail]]
> Sent: Friday, May 01, 2009 3:06 AM
> To: Myles Watson
> Cc: coreboot
> Subject: Re: [coreboot] [PATCH] Remove warnings for s2892
>
> Awesome patch!

Thanks.
> On 01.05.2009 0:22 Uhr, Myles Watson wrote:
> > if (!eeprom_valid) {
> >    	unsigned long mac_pos;
> >    	mac_pos = 0xffffffd0; /* See romstrap.inc and romstrap.lds.

*/
> > mac_l = readl(mac_pos) + nic_index;
> > mac_h = readl(mac_pos + 4);
> > mac_l = readl((uint8_t*)mac_pos) + nic_index;
> > mac_h = readl((uint8_t*)mac_pos + 4);
> > }
>
> One could put that slightly simpler as
>
> if (!eeprom_valid) {
>    	u8 *mac_pos;
>    	mac_pos = 0xffffffd0; /* See romstrap.inc and romstrap.lds.

*/
I think you still need a cast, but it is nicer than two casts.
    	mac_pos = (u8*) 0xffffffd0; /* See romstrap.inc and
romstrap.lds. */
> mac_l = readl(mac_pos) + nic_index;
> mac_h = readl(mac_pos + 4);
>
> }

I wasn't sure if we wanted to convert types from uint8_t to u8 in old files,
so I didn't do any of that.

Acked-by: Myles Watson

Thanks,
Myles