[PATCH] quiet non-x86 option ROM warnings

[PATCH] quiet non-x86 option ROM warnings

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 458 bytes --]

Both the r128 and radeon drivers complain if they don't find an x86 option ROM 
on the device they're talking to.  This would be fine, except that the 
message is incorrect--not all option ROMs are required to be x86 based.  This 
small patch just removes the messages altogether, causing the drivers to 
*silently* fall back to the non-x86 option ROM behavior (it works fine and 
there's no cause for alarm).

Signed-off-by: Jesse Barnes <jbarnes@sgi.com>


[-- Attachment #2: non-x86-rom-ok.patch --]
[-- Type: text/x-diff, Size: 1226 bytes --]

===== drivers/video/aty/radeon_base.c 1.39 vs edited =====
--- 1.39/drivers/video/aty/radeon_base.c	2005-02-10 22:57:44 -08:00
+++ edited/drivers/video/aty/radeon_base.c	2005-02-15 15:51:19 -08:00
@@ -329,11 +329,9 @@
 	rinfo->bios_seg = rom;
 
 	/* Very simple test to make sure it appeared */
-	if (BIOS_IN16(0) != 0xaa55) {
-		printk(KERN_ERR "radeonfb (%s): Invalid ROM signature %x should be"
-		       "0xaa55\n", pci_name(rinfo->pdev), BIOS_IN16(0));
-		goto failed;
-	}
+	if (BIOS_IN16(0) != 0xaa55)
+		goto failed; /* not an x86 option ROM, bail out */
+
 	/* Look for the PCI data to check the ROM type */
 	dptr = BIOS_IN16(0x18);
 
===== drivers/video/aty/aty128fb.c 1.56 vs edited =====
--- 1.56/drivers/video/aty/aty128fb.c	2005-02-10 22:57:44 -08:00
+++ edited/drivers/video/aty/aty128fb.c	2005-02-15 15:51:40 -08:00
@@ -812,11 +812,8 @@
 	}
 
 	/* Very simple test to make sure it appeared */
-	if (BIOS_IN16(0) != 0xaa55) {
-		printk(KERN_ERR "aty128fb: Invalid ROM signature %x should be 0xaa55\n",
-		       BIOS_IN16(0));
-		goto failed;
-	}
+	if (BIOS_IN16(0) != 0xaa55)
+		goto failed; /* not an x86 option ROM, bail out */
 
 	/* Look for the PCI data to check the ROM type */
 	dptr = BIOS_IN16(0x18);

Re: [PATCH] quiet non-x86 option ROM warnings

[Jon Smirl]

You're removing the check for 55AA at the start of the ROM. I though
the PCI standard was that all ROMs had to start with the no matter
what object code they contain. Then if you look for PCIR there is a
field in the stucture that says what language the ROM is in. Maybe the
problem is in the BIOS_IN16() function and things are getting byte
swapped wrong.

                void __iomem *pds;
                /* Standard PCI ROMs start out with these bytes 55 AA */
                if (readb(image) != 0x55)
                        break;
                if (readb(image + 1) != 0xAA)
                        break;
                /* get the PCI data structure and check its signature */
                pds = image + readw(image + 24);
                if (readb(pds) != 'P')
                        break;
                if (readb(pds + 1) != 'C')
                        break;
                if (readb(pds + 2) != 'I')
                        break;
                if (readb(pds + 3) != 'R')
                        break;
                last_image = readb(pds + 21) & 0x80;
                /* this length is reliable */
                image += readw(pds + 16) * 512;



On Tue, 15 Feb 2005 15:57:05 -0800, Jesse Barnes <jbarnes@sgi.com> wrote:
> Both the r128 and radeon drivers complain if they don't find an x86 option ROM
> on the device they're talking to.  This would be fine, except that the
> message is incorrect--not all option ROMs are required to be x86 based.  This
> small patch just removes the messages altogether, causing the drivers to
> *silently* fall back to the non-x86 option ROM behavior (it works fine and
> there's no cause for alarm).
> 
> Signed-off-by: Jesse Barnes <jbarnes@sgi.com>
> 
> 
> 


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] quiet non-x86 option ROM warnings

[Jesse Barnes]

On Tuesday, February 15, 2005 4:36 pm, Jon Smirl wrote:
> You're removing the check for 55AA at the start of the ROM.

No, the check is still there, I just removed the printk if 0xaa55 isn't found 
(my box returns 0x303 instead).

> I though 
> the PCI standard was that all ROMs had to start with the no matter
> what object code they contain. Then if you look for PCIR there is a
> field in the stucture that says what language the ROM is in. Maybe the
> problem is in the BIOS_IN16() function and things are getting byte
> swapped wrong.

I thought the signature described what type of ROM was there?  E.g. 0xaa55 
means x86 ROM, x0303 means OF ROM, etc.?

At any rate, not having a ROM at all (which my case may be) isn't an error 
either, so I think removing the printk is appropriate regardless.

Thanks,
Jesse

Re: [PATCH] quiet non-x86 option ROM warnings

[Benjamin Herrenschmidt]

On Tue, 2005-02-15 at 16:45 -0800, Jesse Barnes wrote:
> On Tuesday, February 15, 2005 4:36 pm, Jon Smirl wrote:
> > You're removing the check for 55AA at the start of the ROM.
> 
> No, the check is still there, I just removed the printk if 0xaa55 isn't found 
> (my box returns 0x303 instead).
> 
> > I though 
> > the PCI standard was that all ROMs had to start with the no matter
> > what object code they contain. Then if you look for PCIR there is a
> > field in the stucture that says what language the ROM is in. Maybe the
> > problem is in the BIOS_IN16() function and things are getting byte
> > swapped wrong.
> 
> I thought the signature described what type of ROM was there?  E.g. 0xaa55 
> means x86 ROM, x0303 means OF ROM, etc.?
> 
> At any rate, not having a ROM at all (which my case may be) isn't an error 
> either, so I think removing the printk is appropriate regardless.

No, they all haev 0xaa55, then a header that says the type of ROM...

Ben.

Re: [PATCH] quiet non-x86 option ROM warnings

[Benjamin Herrenschmidt]

> I thought the signature described what type of ROM was there?  E.g. 0xaa55 
> means x86 ROM, x0303 means OF ROM, etc.?
> 
> At any rate, not having a ROM at all (which my case may be) isn't an error 
> either, so I think removing the printk is appropriate regardless.

Oh, and if this is the PowerBook, then you don't have a ROM attached to
the video chip, the OF driver is part of the main system ROM.

Ben.

Re: [PATCH] quiet non-x86 option ROM warnings

[Benjamin Herrenschmidt]

On Tue, 2005-02-15 at 15:57 -0800, Jesse Barnes wrote:
> Both the r128 and radeon drivers complain if they don't find an x86 option ROM 
> on the device they're talking to.  This would be fine, except that the 
> message is incorrect--not all option ROMs are required to be x86 based.  This 
> small patch just removes the messages altogether, causing the drivers to 
> *silently* fall back to the non-x86 option ROM behavior (it works fine and 
> there's no cause for alarm).

What about printing "No PCI ROM detected" ? I like having that info when
getting user reports, but I agree that a less worrying message would
be good.

Ben.

Re: [PATCH] quiet non-x86 option ROM warnings

[Jon Smirl]

There is a new io resource flag as part of the pci rom code,
IORESOURCE_ROM_SHADOW, that is used on x86. If IORESOURCE_ROM_SHADOW
is set, you should ignore the physical ROM and use the copy at C000:0.
Can we build an equivalent flag for PPC? On x86 arch specific code
determines the boot video device and sets the flag.

Acutally, if radeon and rage fb drivers were using the PCI ROM support
(drivers/pci/rom.c) would they be having this problem? The 55AA check
is in the PCI ROM support too.

On Wed, 16 Feb 2005 12:00:31 +1100, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> 
> > I thought the signature described what type of ROM was there?  E.g. 0xaa55
> > means x86 ROM, x0303 means OF ROM, etc.?
> >
> > At any rate, not having a ROM at all (which my case may be) isn't an error
> > either, so I think removing the printk is appropriate regardless.
> 
> Oh, and if this is the PowerBook, then you don't have a ROM attached to
> the video chip, the OF driver is part of the main system ROM.
> 
> Ben.
> 
> 


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] quiet non-x86 option ROM warnings

[Jesse Barnes]

On Tuesday, February 15, 2005 5:08 pm, Jon Smirl wrote:
> There is a new io resource flag as part of the pci rom code,
> IORESOURCE_ROM_SHADOW, that is used on x86. If IORESOURCE_ROM_SHADOW
> is set, you should ignore the physical ROM and use the copy at C000:0.
> Can we build an equivalent flag for PPC? On x86 arch specific code
> determines the boot video device and sets the flag.
>
> Acutally, if radeon and rage fb drivers were using the PCI ROM support
> (drivers/pci/rom.c) would they be having this problem? The 55AA check
> is in the PCI ROM support too.

They're using it, they just do additional checks.

Jesse

Re: [PATCH] quiet non-x86 option ROM warnings

[Benjamin Herrenschmidt]

On Tue, 2005-02-15 at 20:08 -0500, Jon Smirl wrote:
> There is a new io resource flag as part of the pci rom code,
> IORESOURCE_ROM_SHADOW, that is used on x86. If IORESOURCE_ROM_SHADOW
> is set, you should ignore the physical ROM and use the copy at C000:0.
> Can we build an equivalent flag for PPC? On x86 arch specific code
> determines the boot video device and sets the flag.
> 
> Acutally, if radeon and rage fb drivers were using the PCI ROM support
> (drivers/pci/rom.c) would they be having this problem? The 55AA check
> is in the PCI ROM support too.

No, such a flag wouldn't make sense as it's not really a ROM shadow. In
fact, the driver is just part of the main open firmware, there are no
tables we can get to like x86 etc... however, we can access properties
in the open firmware device-tree that have been set by the OF driver. So
it's a completely different mecanism. A ROM shadow bit wouldn't make
sense.

Ben.

Re: [PATCH] quiet non-x86 option ROM warnings

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

On Tuesday, February 15, 2005 5:03 pm, Benjamin Herrenschmidt wrote:
> What about printing "No PCI ROM detected" ? I like having that info when
> getting user reports, but I agree that a less worrying message would
> be good.

Ok, how about this then?  It changes the printks in both drivers to KERN_INFO 
and describes the situation a bit more accurately.

Signed-off-by: Jesse Barnes <jbarnes@sgi.com>

Thanks,
Jesse

P.S. Jon, I think the pci_map_rom code is buggy--if the option ROM signature 
is missing or indicates that there's no ROM, the routine still returns a 
valid pointer making the caller thing it succeeded.  If we fix that up we can 
fix up the callers.

[-- Attachment #2: aty-no-rom-present-cleanup.patch --]
[-- Type: text/x-diff, Size: 1089 bytes --]

===== drivers/video/aty/radeon_base.c 1.39 vs edited =====
--- 1.39/drivers/video/aty/radeon_base.c	2005-02-10 22:57:44 -08:00
+++ edited/drivers/video/aty/radeon_base.c	2005-02-16 15:48:48 -08:00
@@ -330,8 +330,8 @@
 
 	/* Very simple test to make sure it appeared */
 	if (BIOS_IN16(0) != 0xaa55) {
-		printk(KERN_ERR "radeonfb (%s): Invalid ROM signature %x should be"
-		       "0xaa55\n", pci_name(rinfo->pdev), BIOS_IN16(0));
+		printk(KERN_INFO "radeonfb (%s): no ROM present\n",
+		       pci_name(rinfo->pdev));
 		goto failed;
 	}
 	/* Look for the PCI data to check the ROM type */
===== drivers/video/aty/aty128fb.c 1.56 vs edited =====
--- 1.56/drivers/video/aty/aty128fb.c	2005-02-10 22:57:44 -08:00
+++ edited/drivers/video/aty/aty128fb.c	2005-02-16 15:50:12 -08:00
@@ -813,8 +813,8 @@
 
 	/* Very simple test to make sure it appeared */
 	if (BIOS_IN16(0) != 0xaa55) {
-		printk(KERN_ERR "aty128fb: Invalid ROM signature %x should be 0xaa55\n",
-		       BIOS_IN16(0));
+		printk(KERN_INFO "aty128fb (%s): no ROM present\n",
+		       pci_name(dev));
 		goto failed;
 	}
 

Re: [PATCH] quiet non-x86 option ROM warnings

[Benjamin Herrenschmidt]

On Wed, 2005-02-16 at 15:54 -0800, Jesse Barnes wrote:
> On Tuesday, February 15, 2005 5:03 pm, Benjamin Herrenschmidt wrote:
> > What about printing "No PCI ROM detected" ? I like having that info when
> > getting user reports, but I agree that a less worrying message would
> > be good.
> 
> Ok, how about this then?  It changes the printks in both drivers to KERN_INFO 
> and describes the situation a bit more accurately.
> 
> Signed-off-by: Jesse Barnes <jbarnes@sgi.com>
> 
> Thanks,
> Jesse
> 
> P.S. Jon, I think the pci_map_rom code is buggy--if the option ROM signature 
> is missing or indicates that there's no ROM, the routine still returns a 
> valid pointer making the caller thing it succeeded.  If we fix that up we can 
> fix up the callers.

No, pci_map_rom shouldn't test the signature IMHO. While PCI ROMs should
have the signature to be recognized as containing valid firmware images
on x86 BIOSes an OF, it's just a convention on these platforms, and I
would rather let people put whatever they want in those ROMs and still
let them map it...

Ben.

Re: [PATCH] quiet non-x86 option ROM warnings

[Jon Smirl]

On Thu, 17 Feb 2005 11:48:14 +1100, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2005-02-16 at 15:54 -0800, Jesse Barnes wrote:
> > On Tuesday, February 15, 2005 5:03 pm, Benjamin Herrenschmidt wrote:
> > > What about printing "No PCI ROM detected" ? I like having that info when
> > > getting user reports, but I agree that a less worrying message would
> > > be good.
> >
> > Ok, how about this then?  It changes the printks in both drivers to KERN_INFO
> > and describes the situation a bit more accurately.
> >
> > Signed-off-by: Jesse Barnes <jbarnes@sgi.com>
> >
> > Thanks,
> > Jesse
> >
> > P.S. Jon, I think the pci_map_rom code is buggy--if the option ROM signature
> > is missing or indicates that there's no ROM, the routine still returns a
> > valid pointer making the caller thing it succeeded.  If we fix that up we can
> > fix up the callers.
> 
> No, pci_map_rom shouldn't test the signature IMHO. While PCI ROMs should
> have the signature to be recognized as containing valid firmware images
> on x86 BIOSes an OF, it's just a convention on these platforms, and I
> would rather let people put whatever they want in those ROMs and still
> let them map it...
> 

pci_map_rom will return a pointer to any ROM it finds. It the
signature is invalid the size returned will be zero. Is this ok or do
we want it to do something different?

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] quiet non-x86 option ROM warnings

[Jesse Barnes]

On Thursday, February 17, 2005 8:33 am, Jon Smirl wrote:
> > No, pci_map_rom shouldn't test the signature IMHO. While PCI ROMs should
> > have the signature to be recognized as containing valid firmware images
> > on x86 BIOSes an OF, it's just a convention on these platforms, and I
> > would rather let people put whatever they want in those ROMs and still
> > let them map it...
>
> pci_map_rom will return a pointer to any ROM it finds. It the
> signature is invalid the size returned will be zero. Is this ok or do
> we want it to do something different?

Shouldn't it return NULL if the signature is invalid?

Jesse

Re: [PATCH] quiet non-x86 option ROM warnings

[Jon Smirl]

On Thu, 17 Feb 2005 09:29:53 -0800, Jesse Barnes <jbarnes@sgi.com> wrote:
> On Thursday, February 17, 2005 8:33 am, Jon Smirl wrote:
> > > No, pci_map_rom shouldn't test the signature IMHO. While PCI ROMs should
> > > have the signature to be recognized as containing valid firmware images
> > > on x86 BIOSes an OF, it's just a convention on these platforms, and I
> > > would rather let people put whatever they want in those ROMs and still
> > > let them map it...
> >
> > pci_map_rom will return a pointer to any ROM it finds. It the
> > signature is invalid the size returned will be zero. Is this ok or do
> > we want it to do something different?
> 
> Shouldn't it return NULL if the signature is invalid?
> 

But then you couldn't get to your non-standard ROMs

> Jesse
> 


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] quiet non-x86 option ROM warnings

[Jesse Barnes]

On Thursday, February 17, 2005 9:32 am, Jon Smirl wrote:
> > Shouldn't it return NULL if the signature is invalid?
>
> But then you couldn't get to your non-standard ROMs

Ok, I'll fix up the callers then.

Jesse

Re: [PATCH] quiet non-x86 option ROM warnings

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

On Thursday, February 17, 2005 9:32 am, Jon Smirl wrote:
> On Thu, 17 Feb 2005 09:29:53 -0800, Jesse Barnes <jbarnes@sgi.com> wrote:
> > On Thursday, February 17, 2005 8:33 am, Jon Smirl wrote:
> > > > No, pci_map_rom shouldn't test the signature IMHO. While PCI ROMs
> > > > should have the signature to be recognized as containing valid
> > > > firmware images on x86 BIOSes an OF, it's just a convention on these
> > > > platforms, and I would rather let people put whatever they want in
> > > > those ROMs and still let them map it...
> > >
> > > pci_map_rom will return a pointer to any ROM it finds. It the
> > > signature is invalid the size returned will be zero. Is this ok or do
> > > we want it to do something different?
> >
> > Shouldn't it return NULL if the signature is invalid?
>
> But then you couldn't get to your non-standard ROMs

Ok, how does this one look to you guys?  The r128 driver would need similar 
fixes.

Thanks,
Jesse

[-- Attachment #2: radeonfb-pci-map-rom-fix.patch --]
[-- Type: text/x-diff, Size: 2467 bytes --]

===== drivers/video/aty/radeon_base.c 1.39 vs edited =====
--- 1.39/drivers/video/aty/radeon_base.c	2005-02-10 22:57:44 -08:00
+++ edited/drivers/video/aty/radeon_base.c	2005-02-17 09:43:13 -08:00
@@ -318,54 +318,20 @@
 	temp |= 0x04 << 24;
 	OUTREG(MPP_TB_CONFIG, temp);
 	temp = INREG(MPP_TB_CONFIG);
-                                                                                                          
+
 	rom = pci_map_rom(dev, &rom_size);
 	if (!rom) {
-		printk(KERN_ERR "radeonfb (%s): ROM failed to map\n",
+		printk(KERN_INFO "radeonfb (%s): ROM failed to map\n",
 		       pci_name(rinfo->pdev));
 		return -ENOMEM;
 	}
-	
-	rinfo->bios_seg = rom;
 
-	/* Very simple test to make sure it appeared */
-	if (BIOS_IN16(0) != 0xaa55) {
-		printk(KERN_ERR "radeonfb (%s): Invalid ROM signature %x should be"
-		       "0xaa55\n", pci_name(rinfo->pdev), BIOS_IN16(0));
+	if (!rom_size) /* we mapped something, but it wasn't valid */
 		goto failed;
-	}
-	/* Look for the PCI data to check the ROM type */
+	
+	rinfo->bios_seg = rom;
 	dptr = BIOS_IN16(0x18);
 
-	/* Check the PCI data signature. If it's wrong, we still assume a normal x86 ROM
-	 * for now, until I've verified this works everywhere. The goal here is more
-	 * to phase out Open Firmware images.
-	 *
-	 * Currently, we only look at the first PCI data, we could iteratre and deal with
-	 * them all, and we should use fb_bios_start relative to start of image and not
-	 * relative start of ROM, but so far, I never found a dual-image ATI card
-	 *
-	 * typedef struct {
-	 * 	u32	signature;	+ 0x00
-	 * 	u16	vendor;		+ 0x04
-	 * 	u16	device;		+ 0x06
-	 * 	u16	reserved_1;	+ 0x08
-	 * 	u16	dlen;		+ 0x0a
-	 * 	u8	drevision;	+ 0x0c
-	 * 	u8	class_hi;	+ 0x0d
-	 * 	u16	class_lo;	+ 0x0e
-	 * 	u16	ilen;		+ 0x10
-	 * 	u16	irevision;	+ 0x12
-	 * 	u8	type;		+ 0x14
-	 * 	u8	indicator;	+ 0x15
-	 * 	u16	reserved_2;	+ 0x16
-	 * } pci_data_t;
-	 */
-	if (BIOS_IN32(dptr) !=  (('R' << 24) | ('I' << 16) | ('C' << 8) | 'P')) {
-		printk(KERN_WARNING "radeonfb (%s): PCI DATA signature in ROM"
-		       "incorrect: %08x\n", pci_name(rinfo->pdev), BIOS_IN32(dptr));
-		goto anyway;
-	}
 	rom_type = BIOS_IN8(dptr + 0x14);
 	switch(rom_type) {
 	case 0:
@@ -381,7 +347,7 @@
 		printk(KERN_INFO "radeonfb: Found unknown type %d ROM Image\n", rom_type);
 		goto failed;
 	}
- anyway:
+	
 	/* Locate the flat panel infos, do some sanity checking !!! */
 	rinfo->fp_bios_start = BIOS_IN16(0x48);
 	return 0;

Re: [PATCH] quiet non-x86 option ROM warnings

[Jon Smirl]

On Thu, 17 Feb 2005 09:45:30 -0800, Jesse Barnes <jbarnes@sgi.com> wrote:
> Ok, how does this one look to you guys?  The r128 driver would need similar
> fixes.

Do any of the radeon ROMs store multiple images in different formats?
Should the radeon driver loop throught the ROM images looking for one
that it can understand, or is there alway only a single image? If ATI
wanted to they could make ROMs with both x86 and OpenFirmware images
on them.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] quiet non-x86 option ROM warnings

[Benjamin Herrenschmidt]

On Thu, 2005-02-17 at 11:33 -0500, Jon Smirl wrote:
> On Thu, 17 Feb 2005 11:48:14 +1100, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Wed, 2005-02-16 at 15:54 -0800, Jesse Barnes wrote:
> > > On Tuesday, February 15, 2005 5:03 pm, Benjamin Herrenschmidt wrote:
> > > > What about printing "No PCI ROM detected" ? I like having that info when
> > > > getting user reports, but I agree that a less worrying message would
> > > > be good.
> > >
> > > Ok, how about this then?  It changes the printks in both drivers to KERN_INFO
> > > and describes the situation a bit more accurately.
> > >
> > > Signed-off-by: Jesse Barnes <jbarnes@sgi.com>
> > >
> > > Thanks,
> > > Jesse
> > >
> > > P.S. Jon, I think the pci_map_rom code is buggy--if the option ROM signature
> > > is missing or indicates that there's no ROM, the routine still returns a
> > > valid pointer making the caller thing it succeeded.  If we fix that up we can
> > > fix up the callers.
> > 
> > No, pci_map_rom shouldn't test the signature IMHO. While PCI ROMs should
> > have the signature to be recognized as containing valid firmware images
> > on x86 BIOSes an OF, it's just a convention on these platforms, and I
> > would rather let people put whatever they want in those ROMs and still
> > let them map it...
> > 
> 
> pci_map_rom will return a pointer to any ROM it finds. It the
> signature is invalid the size returned will be zero. Is this ok or do
> we want it to do something different?

Can't the size be obtained like any other BAR ?

Ben.

Re: [PATCH] quiet non-x86 option ROM warnings

[Benjamin Herrenschmidt]

On Thu, 2005-02-17 at 09:45 -0800, Jesse Barnes wrote:
> On Thursday, February 17, 2005 9:32 am, Jon Smirl wrote:
> > On Thu, 17 Feb 2005 09:29:53 -0800, Jesse Barnes <jbarnes@sgi.com> wrote:
> > > On Thursday, February 17, 2005 8:33 am, Jon Smirl wrote:
> > > > > No, pci_map_rom shouldn't test the signature IMHO. While PCI ROMs
> > > > > should have the signature to be recognized as containing valid
> > > > > firmware images on x86 BIOSes an OF, it's just a convention on these
> > > > > platforms, and I would rather let people put whatever they want in
> > > > > those ROMs and still let them map it...
> > > >
> > > > pci_map_rom will return a pointer to any ROM it finds. It the
> > > > signature is invalid the size returned will be zero. Is this ok or do
> > > > we want it to do something different?
> > >
> > > Shouldn't it return NULL if the signature is invalid?
> >
> > But then you couldn't get to your non-standard ROMs
> 
> Ok, how does this one look to you guys?  The r128 driver would need similar 
> fixes.

We could provide additional helpers, like pci_find_rom_partition(),
which takes the architecture code as an argument. It would check the
signature, and iterate all "partitions" til it finds the proper
architecture (or none).

Sorry, I'm still in the middle of breakfast, so no patch attached :)

Ben.

Re: [PATCH] quiet non-x86 option ROM warnings

[Benjamin Herrenschmidt]

On Thu, 2005-02-17 at 12:56 -0500, Jon Smirl wrote:
> On Thu, 17 Feb 2005 09:45:30 -0800, Jesse Barnes <jbarnes@sgi.com> wrote:
> > Ok, how does this one look to you guys?  The r128 driver would need similar
> > fixes.
> 
> Do any of the radeon ROMs store multiple images in different formats?
> Should the radeon driver loop throught the ROM images looking for one
> that it can understand, or is there alway only a single image? If ATI
> wanted to they could make ROMs with both x86 and OpenFirmware images
> on them.

While it's possible, I don't think it's actually done for radeon's (but
may for other video cards). Anyway as I wrote earlier, what about a
helper that deal with all these things ?

Ben.

Re: [PATCH] quiet non-x86 option ROM warnings

[Jon Smirl]

On Fri, 18 Feb 2005 09:45:50 +1100, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:

> Can't the size be obtained like any other BAR ?

yes, but cards that don't fully decode their ROM address space can
waste memory in copy_rom. For example I have a card around here that
reports a BAR address space of 128K and has a 2K ROM in it. You only
want to copy the 2K, not the 128K.


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] quiet non-x86 option ROM warnings

[Jon Smirl]

On Fri, 18 Feb 2005 09:47:15 +1100, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> We could provide additional helpers, like pci_find_rom_partition(),
> which takes the architecture code as an argument. It would check the
> signature, and iterate all "partitions" til it finds the proper
> architecture (or none).

The spec allows for it but has anyone actually seen a ROM with
multiple images in it? I haven't but I only work on x86.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] quiet non-x86 option ROM warnings

[Jesse Barnes]

On Thursday, February 17, 2005 2:59 pm, Jon Smirl wrote:
> On Fri, 18 Feb 2005 09:47:15 +1100, Benjamin Herrenschmidt
>
> <benh@kernel.crashing.org> wrote:
> > We could provide additional helpers, like pci_find_rom_partition(),
> > which takes the architecture code as an argument. It would check the
> > signature, and iterate all "partitions" til it finds the proper
> > architecture (or none).
>
> The spec allows for it but has anyone actually seen a ROM with
> multiple images in it? I haven't but I only work on x86.

I haven't seen any either, but maybe the parisc folks have?

Jesse

Re: [PATCH] quiet non-x86 option ROM warnings

[Benjamin Herrenschmidt]

On Thu, 2005-02-17 at 17:59 -0500, Jon Smirl wrote:
> On Fri, 18 Feb 2005 09:47:15 +1100, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > We could provide additional helpers, like pci_find_rom_partition(),
> > which takes the architecture code as an argument. It would check the
> > signature, and iterate all "partitions" til it finds the proper
> > architecture (or none).
> 
> The spec allows for it but has anyone actually seen a ROM with
> multiple images in it? I haven't but I only work on x86.

Yes, I pretty sure some video cards did that in the past at least, and
maybe some scsi cards. It was a while ago, I don't know if this is still
true, but it's relatively easy to do, let's just hide all of this logic,
along with size & signature checking in a single place, that way, we
don't have to duplicate all that logic in drivers...

Ben.

Re: [PATCH] quiet non-x86 option ROM warnings

[Andrew Vasquez]

On Thu, 17 Feb 2005, Jon Smirl wrote:
> On Fri, 18 Feb 2005 09:47:15 +1100, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > We could provide additional helpers, like pci_find_rom_partition(),
> > which takes the architecture code as an argument. It would check the
> > signature, and iterate all "partitions" til it finds the proper
> > architecture (or none).
> 
> The spec allows for it but has anyone actually seen a ROM with
> multiple images in it? I haven't but I only work on x86.
> 

Yes.  Many SCSI and fibre-channel cards carry multiple images.

--
Andrew Vasquez

Re: [PATCH] quiet non-x86 option ROM warnings

[Gabriel Paubert]

On Thu, Feb 17, 2005 at 05:56:03PM -0500, Jon Smirl wrote:
> On Fri, 18 Feb 2005 09:45:50 +1100, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> 
> > Can't the size be obtained like any other BAR ?
> 
> yes, but cards that don't fully decode their ROM address space can
> waste memory in copy_rom. For example I have a card around here that
> reports a BAR address space of 128K and has a 2K ROM in it. You only
> want to copy the 2K, not the 128K.

Indeed, but they normally repeat by powers of 2, ignoring
high order address bits. Is it that hard to detect?

For example if it declares 128k, compare the two halves, reduce
to 64k if equal. Lather, rinse, repeat.

It's equivalent to reading the BAR declared size twice in 
the worst case, so it's not that bad performance-wise.

That would only be in the case of an unknown signature
in the first bytes, otherwise the third byte gives you
the size IIUC.

	Gabriel

Re: [PATCH] quiet non-x86 option ROM warnings

[Jon Smirl]

On Fri, 18 Feb 2005 13:09:14 +0100, Gabriel Paubert <paubert@iram.es> wrote:
> For example if it declares 128k, compare the two halves, reduce
> to 64k if equal. Lather, rinse, repeat.
> 
> It's equivalent to reading the BAR declared size twice in
> the worst case, so it's not that bad performance-wise.
> 
> That would only be in the case of an unknown signature
> in the first bytes, otherwise the third byte gives you
> the size IIUC.

The third byte size is wrong in too many cards to be useful. I have
always found the size in the PCIR header to be accurate.

> 
>         Gabriel
> 


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] quiet non-x86 option ROM warnings

[Olaf Hering]

 On Tue, Feb 15, Jesse Barnes wrote:

> Both the r128 and radeon drivers complain if they don't find an x86 option ROM 
> on the device they're talking to.  This would be fine, except that the 
> message is incorrect--not all option ROMs are required to be x86 based.  This 
> small patch just removes the messages altogether, causing the drivers to 
> *silently* fall back to the non-x86 option ROM behavior (it works fine and 
> there's no cause for alarm).

This patch wasnt applied, back in February this year. Please do so now.



Quiet an incorrect warning in aty128fb and radeonfb about the PCI ROM
content. Macs work just find without that signature.


Signed-off-by: Olaf Hering <olh@suse.de>

 drivers/video/aty/aty128fb.c    |    2 +-
 drivers/video/aty/radeon_base.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6.13-fb-rom/drivers/video/aty/aty128fb.c
===================================================================
--- linux-2.6.13-fb-rom.orig/drivers/video/aty/aty128fb.c
+++ linux-2.6.13-fb-rom/drivers/video/aty/aty128fb.c
@@ -806,7 +806,7 @@ static void __iomem * __init aty128_map_
 
 	/* Very simple test to make sure it appeared */
 	if (BIOS_IN16(0) != 0xaa55) {
-		printk(KERN_ERR "aty128fb: Invalid ROM signature %x should be 0xaa55\n",
+		printk(KERN_DEBUG "aty128fb: Invalid ROM signature %x should be 0xaa55\n",
 		       BIOS_IN16(0));
 		goto failed;
 	}
Index: linux-2.6.13-fb-rom/drivers/video/aty/radeon_base.c
===================================================================
--- linux-2.6.13-fb-rom.orig/drivers/video/aty/radeon_base.c
+++ linux-2.6.13-fb-rom/drivers/video/aty/radeon_base.c
@@ -329,7 +329,7 @@ static int __devinit radeon_map_ROM(stru
 
 	/* Very simple test to make sure it appeared */
 	if (BIOS_IN16(0) != 0xaa55) {
-		printk(KERN_ERR "radeonfb (%s): Invalid ROM signature %x should be"
+		printk(KERN_DEBUG "radeonfb (%s): Invalid ROM signature %x should be"
 		       "0xaa55\n", pci_name(rinfo->pdev), BIOS_IN16(0));
 		goto failed;
 	}

Re: [PATCH] quiet non-x86 option ROM warnings

[Andreas Schwab]

Olaf Hering <olh@suse.de> writes:

> -		printk(KERN_ERR "radeonfb (%s): Invalid ROM signature %x should be"
> +		printk(KERN_DEBUG "radeonfb (%s): Invalid ROM signature %x should be"
>  		       "0xaa55\n", pci_name(rinfo->pdev), BIOS_IN16(0));

While you are at it you could also add the missing space.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."