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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
>
>
>
--
Jon Smirl
[email protected]
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
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.
> 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.
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.
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
<[email protected]> 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
[email protected]
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
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.
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 <[email protected]>
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.
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 <[email protected]>
>
> 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.
On Thu, 17 Feb 2005 11:48:14 +1100, Benjamin Herrenschmidt
<[email protected]> 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 <[email protected]>
> >
> > 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
[email protected]
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
On Thu, 17 Feb 2005 09:29:53 -0800, Jesse Barnes <[email protected]> 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
[email protected]
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
On Thursday, February 17, 2005 9:32 am, Jon Smirl wrote:
> On Thu, 17 Feb 2005 09:29:53 -0800, Jesse Barnes <[email protected]> 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
On Thu, 17 Feb 2005 09:45:30 -0800, Jesse Barnes <[email protected]> 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
[email protected]
On Thu, 2005-02-17 at 11:33 -0500, Jon Smirl wrote:
> On Thu, 17 Feb 2005 11:48:14 +1100, Benjamin Herrenschmidt
> <[email protected]> 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 <[email protected]>
> > >
> > > 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.
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 <[email protected]> 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.
On Thu, 2005-02-17 at 12:56 -0500, Jon Smirl wrote:
> On Thu, 17 Feb 2005 09:45:30 -0800, Jesse Barnes <[email protected]> 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.
On Fri, 18 Feb 2005 09:45:50 +1100, Benjamin Herrenschmidt
<[email protected]> 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
[email protected]
On Fri, 18 Feb 2005 09:47:15 +1100, Benjamin Herrenschmidt
<[email protected]> 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
[email protected]
On Thursday, February 17, 2005 2:59 pm, Jon Smirl wrote:
> On Fri, 18 Feb 2005 09:47:15 +1100, Benjamin Herrenschmidt
>
> <[email protected]> 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
On Thu, 2005-02-17 at 17:59 -0500, Jon Smirl wrote:
> On Fri, 18 Feb 2005 09:47:15 +1100, Benjamin Herrenschmidt
> <[email protected]> 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.
On Thu, 17 Feb 2005, Jon Smirl wrote:
> On Fri, 18 Feb 2005 09:47:15 +1100, Benjamin Herrenschmidt
> <[email protected]> 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
On Thu, Feb 17, 2005 at 05:56:03PM -0500, Jon Smirl wrote:
> On Fri, 18 Feb 2005 09:45:50 +1100, Benjamin Herrenschmidt
> <[email protected]> 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
On Fri, 18 Feb 2005 13:09:14 +0100, Gabriel Paubert <[email protected]> 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
[email protected]
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 <[email protected]>
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;
}
Olaf Hering <[email protected]> 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, [email protected]
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."