[RESEND] [PATCH] convert sticore.c to PCI ROM API

[RESEND] [PATCH] convert sticore.c to PCI ROM API

[Krzysztof Helt]

From: Krzysztof Helt <krzysztof.h1@wp.pl>

Convert console/sticore.c file to use PCI ROM API.

Addresses http://bugzilla.kernel.org/show_bug.cgi?id=9425

Signed-off-by: Krzysztof Helt <krzysztof.h1@wp.pl>
Signed-off-by: Helge Deller <deller@gmx.de>

---
This patch was fixed and tested by Helge Deller on his
PARISC machine.


diff --git a/drivers/video/console/sticore.c b/drivers/video/console/sticore.c
index e9ab657..df30499 100644
--- a/drivers/video/console/sticore.c
+++ b/drivers/video/console/sticore.c
@@ -780,11 +780,13 @@ out_err:
 }
 
 static struct sti_struct * __devinit
-sti_try_rom_generic(unsigned long address, unsigned long hpa, struct pci_dev *pd)
+sti_try_rom_generic(unsigned long address, unsigned long hpa,
+		    struct pci_dev *pd)
 {
+	char __iomem *rom_base = (char __iomem *) address;
 	struct sti_struct *sti;
 	int ok;
-	u32 sig;
+	__le32 sig;
 
 	if (num_sti_roms >= MAX_STI_ROMS) {
 		printk(KERN_WARNING "maximum number of STI ROMS reached !\n");
@@ -808,7 +810,7 @@ test_rom:
 	sig = gsc_readl(address);
 
 	/* check for a PCI ROM structure */
-	if ((le32_to_cpu(sig)==0xaa55)) {
+	if (sig == cpu_to_le32(0xaa55)) {
 		unsigned int i, rm_offset;
 		u32 *rm;
 		i = gsc_readl(address+0x04);
@@ -868,10 +870,8 @@ test_rom:
 	/* disable STI PCI ROM. ROM and card RAM overlap and
 	 * leaving it enabled would force HPMCs
 	 */
-	if (sti->pd) {
-		unsigned long rom_base;
-		rom_base = pci_resource_start(sti->pd, PCI_ROM_RESOURCE);	
-		pci_write_config_dword(sti->pd, PCI_ROM_ADDRESS, rom_base & ~PCI_ROM_ADDRESS_ENABLE);
+	if (sti->pd && rom_base) {
+		pci_unmap_rom(sti->pd, rom_base);
 		DPRINTK((KERN_DEBUG "STI PCI ROM disabled\n"));
 	}
 
@@ -930,28 +930,25 @@ static int __devinit sticore_pci_init(struct pci_dev *pd,
 		const struct pci_device_id *ent)
 {
 #ifdef CONFIG_PCI
-	unsigned long fb_base, rom_base;
-	unsigned int fb_len, rom_len;
+	unsigned long fb_base;
+	unsigned int fb_len;
+	char __iomem *rom_base;
+	size_t rom_len;
 	struct sti_struct *sti;
 	
 	pci_enable_device(pd);
 
 	fb_base = pci_resource_start(pd, 0);
 	fb_len = pci_resource_len(pd, 0);
-	rom_base = pci_resource_start(pd, PCI_ROM_RESOURCE);
-	rom_len = pci_resource_len(pd, PCI_ROM_RESOURCE);
-	if (rom_base) {
-		pci_write_config_dword(pd, PCI_ROM_ADDRESS, rom_base | PCI_ROM_ADDRESS_ENABLE);
-		DPRINTK((KERN_DEBUG "STI PCI ROM enabled at 0x%08lx\n", rom_base));
-	}
+	rom_base = pci_map_rom(pd, &rom_len);
 
-	printk(KERN_INFO "STI PCI graphic ROM found at %08lx (%u kB), fb at %08lx (%u MB)\n",
+	printk(KERN_INFO "STI PCI graphic ROM found at %p (%u kB), fb at %08lx (%u MB)\n",
 		rom_base, rom_len/1024, fb_base, fb_len/1024/1024);
 
-	DPRINTK((KERN_DEBUG "Trying PCI STI ROM at %08lx, PCI hpa at %08lx\n",
+	DPRINTK((KERN_DEBUG "Trying PCI STI ROM at %p, PCI hpa at %08lx\n",
 		    rom_base, fb_base));
 
-	sti = sti_try_rom_generic(rom_base, fb_base, pd);
+	sti = sti_try_rom_generic((unsigned long)rom_base, fb_base, pd);
 	if (sti) {
 		char pa_path[30];
 		print_pci_hwpath(pd, pa_path);
@@ -975,7 +972,7 @@ static void __devexit sticore_pci_remove(struct pci_dev *pd)
 }
 
 
-static struct pci_device_id sti_pci_tbl[] = {
+DEFINE_PCI_DEVICE_TABLE(sti_pci_tbl) = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_HP, PCI_DEVICE_ID_HP_VISUALIZE_EG) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_HP, PCI_DEVICE_ID_HP_VISUALIZE_FX6) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_HP, PCI_DEVICE_ID_HP_VISUALIZE_FX4) },




----------------------------------------------------------------------
Tanie rozmowy!
Sprawdz >>> http://link.interia.pl/f1e20 



-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

Re: [RESEND] [PATCH] convert sticore.c to PCI ROM API

[Andrew Morton]

On Thu, 5 Jun 2008 23:13:27 +0200
Krzysztof Helt <krzysztof.h1@poczta.fm> wrote:

> From: Krzysztof Helt <krzysztof.h1@wp.pl>
> 
> Convert console/sticore.c file to use PCI ROM API.
> 
> Addresses http://bugzilla.kernel.org/show_bug.cgi?id=9425

I'm unable to understand that bug report.  Is this just a cleanup
or does it fix some runtime error, or...?

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

Re: [RESEND] [PATCH] convert sticore.c to PCI ROM API

[Krzysztof Helt]

On Thu, 5 Jun 2008 15:11:22 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 5 Jun 2008 23:13:27 +0200
> Krzysztof Helt <krzysztof.h1@poczta.fm> wrote:
> 
> > From: Krzysztof Helt <krzysztof.h1@wp.pl>
> > 
> > Convert console/sticore.c file to use PCI ROM API.
> > 
> > Addresses http://bugzilla.kernel.org/show_bug.cgi?id=9425
> 
> I'm unable to understand that bug report.  Is this just a cleanup
> or does it fix some runtime error, or...?
> 

It is conversion to a new API after the API was introduced (code change/cleanup).

AFAIK, it does not fix any known error.

Regards,
Krzysztof

----------------------------------------------------------------------
Tanie rozmowy!
Sprawdz >>>  http://link.interia.pl/f1e22 


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

Re: [RESEND] [PATCH] convert sticore.c to PCI ROM API

[James Bottomley]

On Thu, 2008-06-05 at 23:13 +0200, Krzysztof Helt wrote:
> From: Krzysztof Helt <krzysztof.h1@wp.pl>
> 
> Convert console/sticore.c file to use PCI ROM API.
> 
> Addresses http://bugzilla.kernel.org/show_bug.cgi?id=9425
> 
> Signed-off-by: Krzysztof Helt <krzysztof.h1@wp.pl>
> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> ---
> This patch was fixed and tested by Helge Deller on his
> PARISC machine.
> 
> 
> diff --git a/drivers/video/console/sticore.c b/drivers/video/console/sticore.c
> index e9ab657..df30499 100644
> --- a/drivers/video/console/sticore.c
> +++ b/drivers/video/console/sticore.c
> @@ -780,11 +780,13 @@ out_err:
>  }
>  
>  static struct sti_struct * __devinit
> -sti_try_rom_generic(unsigned long address, unsigned long hpa, struct pci_dev *pd)
> +sti_try_rom_generic(unsigned long address, unsigned long hpa,
> +		    struct pci_dev *pd)
>  {
> +	char __iomem *rom_base = (char __iomem *) address;
>  	struct sti_struct *sti;
>  	int ok;
> -	u32 sig;
> +	__le32 sig;
>  
>  	if (num_sti_roms >= MAX_STI_ROMS) {
>  		printk(KERN_WARNING "maximum number of STI ROMS reached !\n");
> @@ -808,7 +810,7 @@ test_rom:
>  	sig = gsc_readl(address);

Since gsc_readl() has (designedly) no endianness type, doesn't this give
a sparse warning?
 
>  	/* check for a PCI ROM structure */
> -	if ((le32_to_cpu(sig)==0xaa55)) {
> +	if (sig == cpu_to_le32(0xaa55)) {
>  		unsigned int i, rm_offset;
>  		u32 *rm;
>  		i = gsc_readl(address+0x04);
> @@ -868,10 +870,8 @@ test_rom:
>  	/* disable STI PCI ROM. ROM and card RAM overlap and
>  	 * leaving it enabled would force HPMCs
>  	 */
> -	if (sti->pd) {
> -		unsigned long rom_base;
> -		rom_base = pci_resource_start(sti->pd, PCI_ROM_RESOURCE);	
> -		pci_write_config_dword(sti->pd, PCI_ROM_ADDRESS, rom_base & ~PCI_ROM_ADDRESS_ENABLE);
> +	if (sti->pd && rom_base) {
> +		pci_unmap_rom(sti->pd, rom_base);
>  		DPRINTK((KERN_DEBUG "STI PCI ROM disabled\n"));
>  	}
>  
> @@ -930,28 +930,25 @@ static int __devinit sticore_pci_init(struct pci_dev *pd,
>  		const struct pci_device_id *ent)
>  {
>  #ifdef CONFIG_PCI
> -	unsigned long fb_base, rom_base;
> -	unsigned int fb_len, rom_len;
> +	unsigned long fb_base;
> +	unsigned int fb_len;
> +	char __iomem *rom_base;
> +	size_t rom_len;
>  	struct sti_struct *sti;
>  	
>  	pci_enable_device(pd);
>  
>  	fb_base = pci_resource_start(pd, 0);
>  	fb_len = pci_resource_len(pd, 0);
> -	rom_base = pci_resource_start(pd, PCI_ROM_RESOURCE);
> -	rom_len = pci_resource_len(pd, PCI_ROM_RESOURCE);
> -	if (rom_base) {
> -		pci_write_config_dword(pd, PCI_ROM_ADDRESS, rom_base | PCI_ROM_ADDRESS_ENABLE);
> -		DPRINTK((KERN_DEBUG "STI PCI ROM enabled at 0x%08lx\n", rom_base));
> -	}
> +	rom_base = pci_map_rom(pd, &rom_len);

I'm really not sure this is such a good idea.  pci_map_rom() can do an
ioremap() on the region.  In that case, gsc_readl which punches through
our virtual memory into physical I/O space will fail.  What assurance do
we have that all STI roms are correctly set up so pci_map_rom() isn't
inclined to ioremap them?

James

Re: [RESEND] [PATCH] convert sticore.c to PCI ROM API

[Krzysztof Helt]

On Thu, 05 Jun 2008 17:26:54 -0500
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> 
> I'm really not sure this is such a good idea.  pci_map_rom() can do an
> ioremap() on the region.  In that case, gsc_readl which punches through
> our virtual memory into physical I/O space will fail.  What assurance do
> we have that all STI roms are correctly set up so pci_map_rom() isn't
> inclined to ioremap them?
> 

I haven't thought about this. The ROM the Helge tested was obviously
set up correctly, otherwise reading the ROM signature would be impossible.

What is you advise for this patch James?

Regards,
Krzysztof

---------------------------------------------------------------
Sprawdz jak zdobyc zdrowy usmiech!
Kliknij >> http://link.interia.pl/f1e26

Re: [RESEND] [PATCH] convert sticore.c to PCI ROM API

[James Bottomley]

On Fri, 2008-06-06 at 22:35 +0200, Krzysztof Helt wrote:
> On Thu, 05 Jun 2008 17:26:54 -0500
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > 
> > I'm really not sure this is such a good idea.  pci_map_rom() can do an
> > ioremap() on the region.  In that case, gsc_readl which punches through
> > our virtual memory into physical I/O space will fail.  What assurance do
> > we have that all STI roms are correctly set up so pci_map_rom() isn't
> > inclined to ioremap them?
> > 
> 
> I haven't thought about this. The ROM the Helge tested was obviously
> set up correctly, otherwise reading the ROM signature would be impossible.

I suspect Helge only tested GSC based cards, which will work just fine
because they use the old logic.

> What is you advise for this patch James?

The more I look at this the more I doubt that it's a good thing.  STI is
really a platform driver; it has to function for both GSC and PCI
busses.  pci_map_rom() seems to be a bit of a minefield designed for x86
centric video cards, which STI definitely isn't.  The sticore driver is
also designed to operate on unmapped ranges ... at least this is the way
it's designed for the GSC, so the PCI part has to follow suit.

Our simple base get and enabling transparently the driver currently has
says what we're doing; I don't really see why it needs to switch to
pci_map_rom.  Additionally, I fear that switching to pci_map_rom will
cause us to break later on down the road.

James

Re: [RESEND] [PATCH] convert sticore.c to PCI ROM API

[Krzysztof Helt]

On Fri, 06 Jun 2008 18:27:41 -0500
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Fri, 2008-06-06 at 22:35 +0200, Krzysztof Helt wrote:

> > What is you advise for this patch James?
> 
> The more I look at this the more I doubt that it's a good thing.  STI is
> really a platform driver; it has to function for both GSC and PCI
> busses.  pci_map_rom() seems to be a bit of a minefield designed for x86
> centric video cards, which STI definitely isn't.  The sticore driver is
> also designed to operate on unmapped ranges ... at least this is the way
> it's designed for the GSC, so the PCI part has to follow suit.
> 
> Our simple base get and enabling transparently the driver currently has
> says what we're doing; I don't really see why it needs to switch to
> pci_map_rom.  Additionally, I fear that switching to pci_map_rom will
> cause us to break later on down the road.
> 

I am forwarding it to the reporter of the bug 9425 as this bug should be 
closed without changing the code.

A very similar case is for the bug 9424. I analyzed code for the Matrox
framebuffers and it is not worth changing. The idea behind the  pci_map_rom()
is that it enables and maps the ROM area. The Matrox framebuffer has
these two separated as the ROM may appear in the already mapped area.
The ROM is always enabled but not always mapped.

The only unification I see is to export pci_rom_enable/pci_rom_disable()
and use them inside the Matrox and sticore drivers (so no ioremap() 
is done but the code is shorter).

Regards,
Krzysztof

----------------------------------------------------------------------
Tanie rozmowy!
Sprawdz >>>  http://link.interia.pl/f1e22 

Re: [RESEND] [PATCH] convert sticore.c to PCI ROM API

[Grant Grundler]

On Sat, Jun 07, 2008 at 11:08:36AM +0200, Krzysztof Helt wrote:
...
> I am forwarding it to the reporter of the bug 9425 as this bug should be 
> closed without changing the code.

Actually, we should change the code: add a comment that summarizes jejb's
feedback (and the rest of the conversation) so we don't repeat this
exercise again in 2 years.

thanks,
grant

> 
> A very similar case is for the bug 9424. I analyzed code for the Matrox
> framebuffers and it is not worth changing. The idea behind the  pci_map_rom()
> is that it enables and maps the ROM area. The Matrox framebuffer has
> these two separated as the ROM may appear in the already mapped area.
> The ROM is always enabled but not always mapped.
> 
> The only unification I see is to export pci_rom_enable/pci_rom_disable()
> and use them inside the Matrox and sticore drivers (so no ioremap() 
> is done but the code is shorter).
> 
> Regards,
> Krzysztof
> 
> ----------------------------------------------------------------------
> Tanie rozmowy!
> Sprawdz >>>  http://link.interia.pl/f1e22 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND] [PATCH] convert sticore.c to PCI ROM API

[Helge Deller]

James Bottomley wrote:
>>  static struct sti_struct * __devinit
>> -sti_try_rom_generic(unsigned long address, unsigned long hpa, struct pci_dev *pd)
>> +sti_try_rom_generic(unsigned long address, unsigned long hpa,
>> +		    struct pci_dev *pd)
>>  {
>> +	char __iomem *rom_base = (char __iomem *) address;
>>  	struct sti_struct *sti;
>>  	int ok;
>> -	u32 sig;
>> +	__le32 sig;
>>  
>>  	if (num_sti_roms >= MAX_STI_ROMS) {
>>  		printk(KERN_WARNING "maximum number of STI ROMS reached !\n");
>> @@ -808,7 +810,7 @@ test_rom:
>>  	sig = gsc_readl(address);
> 
> Since gsc_readl() has (designedly) no endianness type, doesn't this give
> a sparse warning?

No, it doesn't.
But maybe I tested it wrongly?

[hd@p100 linus-linux-2.6]# REAL_CC=hppa-linux-gcc cgcc -Wbitwise 
-Wp,-MD,drivers/video/console/.sticore.o.d  -nostdinc -isystem 
/opt/palinux33/lib/gcc-lib/hppa-linux/3.3.4/include -D__KERNEL__ 
-Iinclude  -include include/linux/autoconf.h -Wall -Wundef 
-Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common 
-Werror-implicit-function-declaration -Os   -pipe -mno-space-regs 
-mfast-indirect-calls -mdisable-fpregs -ffunction-sections -march=1.1 
-mschedule=7100LC -fomit-frame-pointer       -D"KBUILD_STR(s)=#s" 
-D"KBUILD_BASENAME=KBUILD_STR(sticore)" 
-D"KBUILD_MODNAME=KBUILD_STR(sticore)" -c -o 
drivers/video/console/sticore.o drivers/video/console/sticore.c
drivers/video/console/sticore.c:32:19: warning: symbol 'default_sti' was 
not declared. Should it be static?
drivers/video/console/sticore.c:241:1: warning: symbol 'sti_rom_copy' 
was not declared. Should it be static?
drivers/video/console/sticore.c:481:24: warning: symbol 
'sti_select_fbfont' was not declared. Should it be static?
drivers/video/console/sticore.c:544:24: warning: symbol 
'sti_select_font' was not declared. Should it be static?
drivers/video/console/sticore.c:710:16: warning: symbol 
'sti_get_wmode_rom' was not declared. Should it be static?
drivers/video/console/sticore.c:727:1: warning: symbol 'sti_read_rom' 
was not declared. Should it be static?
drivers/video/console/sticore.c:975:1: warning: symbol 'sti_pci_tbl' was 
not declared. Should it be static?

Helge

Re: [RESEND] [PATCH] convert sticore.c to PCI ROM API

[Jon Smirl]

----- Original Message ----
From: Grant Grundler <grundler@parisc-linux.org>
To: Krzysztof Helt <krzysztof.h1@poczta.fm>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>; Jon Smirl <jonsmirl@yahoo.com>; Linux-fbdev-devel <linux-fbdev-devel@lists.sourceforge.net>; Andrew Morton <akpm@linux-foundation.org>; Helge Deller <deller@gmx.de>; linux-parisc@vger.kernel.org
Sent: Saturday, June 7, 2008 4:34:28 PM
Subject: Re: [RESEND] [PATCH] convert sticore.c to PCI ROM API

On Sat, Jun 07, 2008 at 11:08:36AM +0200, Krzysztof Helt wrote:
...
> I am forwarding it to the reporter of the bug 9425 as this bug should be 
> closed without changing the code.

Actually, we should change the code: add a comment that summarizes jejb's
feedback (and the rest of the conversation) so we don't repeat this
exercise again in 2 years.
----------------------------------------------------

The PCI ROM API was made for three main reasons:

1) Drivers,
including X, were mapping the ROMs without declaring the resource. If
you map a ROM without declaring the resource the kernel doesn't know it
is there and may map something else into the same addresses. It's been
a while but I believe the virtualization people were the ones
complaining. 

2) Error checking was not robust in the code that
was directly mapping. This was causing drivers to hang when they used
ROMs they thought the ROM was there and really wasn't. This can definitely
happen and it has happened on my own boxes while doing video driver
work. Occasionally hardware doesn't initialize right and the ROMs won't
enable.

3) It is the ground work for supporting multiple video
cards in a box. When there are multiple cards the API keeps them from
mapping on top of each other. If the location of the ROM is not flexible one has to be turned off before the other one can be turned on.

This code was not intended to be x86 specific. The PCI ROM format is cross platform and this code is used on the PowerPC. 

> 
> A very similar case is for the bug 9424. I analyzed code for the Matrox
> framebuffers and it is not worth changing. The idea behind the  pci_map_rom()
> is that it enables and maps the ROM area. The Matrox framebuffer has
> these two separated as the ROM may appear in the already mapped area.
> The ROM is always enabled but not always mapped.

This is one of the ways to get into trouble. If the ROM is enabled the kernel needs to have a resource marking it to make sure that nothing else gets enabled in the same place. When these ROM were accessed directly by the drivers, the driver code was often missing a few of the error checks needed or the resource allocation, etc. The centralize code made sure that every necessary was done to access the ROMs.

> 
> The only unification I see is to export pci_rom_enable/pci_rom_disable()
> and use them inside the Matrox and sticore drivers (so no ioremap() 
> is done but the code is shorter).
> 
> Regards,
> Krzysztof
> 
> ----------------------------------------------------------------------
> Tanie rozmowy!
> Sprawdz >>>  http://link.interia.pl/f1e22 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html