Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

gart: factor out common code

 

 

Linux kernel RSS feed   Index | Next | Previous | View Threaded


pavel at ucw

Apr 15, 2008, 3:43 AM

Post #1 of 9 (1401 views)
Permalink
gart: factor out common code

Cleanup gart handling on amd64 a bit: move common code into
enable_gart_translation , and use symbolic register names where
appropriate.

Signed-off-by: Pavel Machek <pavel [at] suse>

---
commit 43cfc0427c14f482e36adac447409c82b5cbbe09
tree fc873f1b75d5f1be05978124e7d9a9a31efc2d23
parent 5c9a05115efe1572a05bb1ba155b1e7870374a37
author Pavel <pavel [at] amd> Tue, 15 Apr 2008 12:42:29 +0200
committer Pavel <pavel [at] amd> Tue, 15 Apr 2008 12:42:29 +0200

arch/x86/kernel/pci-gart_64.c | 23 ++++++-----------------
drivers/char/agp/amd64-agp.c | 30 ++++++++++--------------------
include/asm-x86/gart.h | 17 +++++++++++++++++
3 files changed, 33 insertions(+), 37 deletions(-)

diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
index fd3c475..035dc61 100644
--- a/arch/x86/kernel/pci-gart_64.c
+++ b/arch/x86/kernel/pci-gart_64.c
@@ -534,8 +534,8 @@ static __init unsigned read_aperture(str
unsigned aper_size = 0, aper_base_32, aper_order;
u64 aper_base;

- pci_read_config_dword(dev, 0x94, &aper_base_32);
- pci_read_config_dword(dev, 0x90, &aper_order);
+ pci_read_config_dword(dev, AMD64_GARTAPERTUREBASE, &aper_base_32);
+ pci_read_config_dword(dev, AMD64_GARTAPERTURECTL, &aper_order);
aper_order = (aper_order >> 1) & 7;

aper_base = aper_base_32 & 0x7fff;
@@ -593,19 +593,8 @@ static __init int init_k8_gatt(struct ag
agp_gatt_table = gatt;

for (i = 0; i < num_k8_northbridges; i++) {
- u32 gatt_reg;
- u32 ctl;
-
dev = k8_northbridges[i];
- gatt_reg = __pa(gatt) >> 12;
- gatt_reg <<= 4;
- pci_write_config_dword(dev, AMD64_GARTTABLEBASE, gatt_reg);
- pci_read_config_dword(dev, AMD64_GARTAPERTURECTL, &ctl);
-
- ctl |= GARTEN;
- ctl &= ~(DISGARTCPU | DISGARTIO);
-
- pci_write_config_dword(dev, AMD64_GARTAPERTURECTL, ctl);
+ enable_gart_translation(dev, __pa(gatt));
}
flush_gart();

@@ -646,11 +635,11 @@ void gart_iommu_shutdown(void)
u32 ctl;

dev = k8_northbridges[i];
- pci_read_config_dword(dev, 0x90, &ctl);
+ pci_read_config_dword(dev, AMD64_GARTAPERTURECTL, &ctl);

- ctl &= ~1;
+ ctl &= ~GARTEN;

- pci_write_config_dword(dev, 0x90, ctl);
+ pci_write_config_dword(dev, AMD64_GARTAPERTURECTL, ctl);
}
}

diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
index 25d6422..7e64c3c 100644
--- a/drivers/char/agp/amd64-agp.c
+++ b/drivers/char/agp/amd64-agp.c
@@ -150,25 +150,14 @@ static u64 amd64_configure(struct pci_de
{
u64 aperturebase;
u32 tmp;
- u64 addr, aper_base;
+ u64 aper_base;

/* Address to map to */
- pci_read_config_dword (hammer, AMD64_GARTAPERTUREBASE, &tmp);
+ pci_read_config_dword(hammer, AMD64_GARTAPERTUREBASE, &tmp);
aperturebase = tmp << 25;
aper_base = (aperturebase & PCI_BASE_ADDRESS_MEM_MASK);

- /* address of the mappings table */
- addr = (u64) gatt_table;
- addr >>= 12;
- tmp = (u32) addr<<4;
- tmp &= ~0xf;
- pci_write_config_dword(hammer, AMD64_GARTTABLEBASE, tmp);
-
- /* Enable GART translation for this hammer. */
- pci_read_config_dword(hammer, AMD64_GARTAPERTURECTL, &tmp);
- tmp |= GARTEN;
- tmp &= ~(DISGARTCPU | DISGARTIO);
- pci_write_config_dword(hammer, AMD64_GARTAPERTURECTL, tmp);
+ enable_gart_translation(hammer, gatt_table);

return aper_base;
}
@@ -207,9 +196,9 @@ static void amd64_cleanup(void)
for (i = 0; i < num_k8_northbridges; i++) {
struct pci_dev *dev = k8_northbridges[i];
/* disable gart translation */
- pci_read_config_dword (dev, AMD64_GARTAPERTURECTL, &tmp);
+ pci_read_config_dword(dev, AMD64_GARTAPERTURECTL, &tmp);
tmp &= ~AMD64_GARTEN;
- pci_write_config_dword (dev, AMD64_GARTAPERTURECTL, tmp);
+ pci_write_config_dword(dev, AMD64_GARTAPERTURECTL, tmp);
}
}

@@ -289,9 +278,9 @@ static __devinit int fix_northbridge(str
u32 nb_order, nb_base;
u16 apsize;

- pci_read_config_dword(nb, 0x90, &nb_order);
+ pci_read_config_dword(nb, AMD64_GARTAPERTURECTL, &nb_order);
nb_order = (nb_order >> 1) & 7;
- pci_read_config_dword(nb, 0x94, &nb_base);
+ pci_read_config_dword(nb, AMD64_GARTAPERTUREBASE, &nb_base);
nb_aper = nb_base << 25;
if (aperture_valid(nb_aper, (32*1024*1024)<<nb_order)) {
return 0;
@@ -316,8 +306,8 @@ static __devinit int fix_northbridge(str
if (order < 0 || !aperture_valid(aper, (32*1024*1024)<<order))
return -1;

- pci_write_config_dword(nb, 0x90, order << 1);
- pci_write_config_dword(nb, 0x94, aper >> 25);
+ pci_write_config_dword(nb, AMD64_GARTAPERTURECTL, order << 1);
+ pci_write_config_dword(nb, AMD64_GARTAPERTUREBASE, aper >> 25);

return 0;
}
diff --git a/include/asm-x86/gart.h b/include/asm-x86/gart.h
index 248e577..6f22786 100644
--- a/include/asm-x86/gart.h
+++ b/include/asm-x86/gart.h
@@ -52,4 +52,21 @@ #define AMD64_GARTTABLEBASE 0x98
#define AMD64_GARTCACHECTL 0x9c
#define AMD64_GARTEN (1<<0)

+static inline void enable_gart_translation(struct pci_dev *dev, u64 addr)
+{
+ u32 tmp, ctl;
+
+ /* address of the mappings table */
+ addr >>= 12;
+ tmp = (u32) addr<<4;
+ tmp &= ~0xf;
+ pci_write_config_dword(dev, AMD64_GARTTABLEBASE, tmp);
+
+ /* Enable GART translation for this hammer. */
+ pci_read_config_dword(dev, AMD64_GARTAPERTURECTL, &ctl);
+ ctl |= GARTEN;
+ ctl &= ~(DISGARTCPU | DISGARTIO);
+ pci_write_config_dword(dev, AMD64_GARTAPERTURECTL, ctl);
+}
+
#endif

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


steve at xemacs

Apr 15, 2008, 4:25 PM

Post #2 of 9 (1334 views)
Permalink
Re: gart: factor out common code [In reply to]

On 4/15/08, Pavel Machek <pavel [at] ucw> wrote:
> Cleanup gart handling on amd64 a bit: move common code into
> enable_gart_translation , and use symbolic register names where
> appropriate.
>
> Signed-off-by: Pavel Machek <pavel [at] suse>

Which source tree is this patch against? I'm looking at Linus
latest and I don't see the AMD64_* constants defined in the
same place.

Other reviewer comments:
The casting between struct pci_dev * and u64 is kind of ugly,
I presume the reason for the narrowing casts in the block of
code you moved is documented somewhere.

It looks OK as far as I can see, but I'm not looking at the same
source code, so I don't think I should add a Reviewed-by:.

-sb
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


pavel at ucw

Apr 16, 2008, 1:14 AM

Post #3 of 9 (1325 views)
Permalink
Re: gart: factor out common code [In reply to]

On Tue 2008-04-15 16:25:29, SL Baur wrote:
> On 4/15/08, Pavel Machek <pavel [at] ucw> wrote:
> > Cleanup gart handling on amd64 a bit: move common code into
> > enable_gart_translation , and use symbolic register names where
> > appropriate.
> >
> > Signed-off-by: Pavel Machek <pavel [at] suse>
>
> Which source tree is this patch against? I'm looking at Linus
> latest and I don't see the AMD64_* constants defined in the
> same place.

x86 tree, hopefully.

> Other reviewer comments:
> The casting between struct pci_dev * and u64 is kind of ugly,
> I presume the reason for the narrowing casts in the block of
> code you moved is documented somewhere.

Where am I doing that?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at elte

Apr 16, 2008, 6:08 AM

Post #4 of 9 (1323 views)
Permalink
Re: gart: factor out common code [In reply to]

* Pavel Machek <pavel [at] ucw> wrote:

> Cleanup gart handling on amd64 a bit: move common code into
> enable_gart_translation , and use symbolic register names where
> appropriate.

thanks Pavel, applied.

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


yhlu.kernel at gmail

Apr 16, 2008, 3:50 PM

Post #5 of 9 (1318 views)
Permalink
Re: gart: factor out common code [In reply to]

On Wed, Apr 16, 2008 at 6:08 AM, Ingo Molnar <mingo [at] elte> wrote:
>
> * Pavel Machek <pavel [at] ucw> wrote:
>
>
> > Cleanup gart handling on amd64 a bit: move common code into
> > enable_gart_translation , and use symbolic register names where
> > appropriate.
>
> thanks Pavel, applied.

good, pavel, can you do that on aperture_64.c?
or I will do it.

YH
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


pavel at ucw

Apr 16, 2008, 3:56 PM

Post #6 of 9 (1314 views)
Permalink
Re: gart: factor out common code [In reply to]

Hi!

> > > Cleanup gart handling on amd64 a bit: move common code into
> > > enable_gart_translation , and use symbolic register names where
> > > appropriate.
> >
> > thanks Pavel, applied.
>
> good, pavel, can you do that on aperture_64.c?
> or I will do it.

I'd like you to help. I'm no iommu/agpgart/aperture expert...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


steve at xemacs

Apr 16, 2008, 7:55 PM

Post #7 of 9 (1303 views)
Permalink
Re: gart: factor out common code [In reply to]

On 4/16/08, Pavel Machek <pavel [at] ucw> wrote:
> On Tue 2008-04-15 16:25:29, SL Baur wrote:
> > On 4/15/08, Pavel Machek <pavel [at] ucw> wrote:

> > Other reviewer comments:
> > The casting between struct pci_dev * and u64 is kind of ugly,
> > I presume the reason for the narrowing casts in the block of
> > code you moved is documented somewhere.
>
>
> Where am I doing that?

The struct pci_dev */u64 casting is in init_k8_gatt. If Ingo is happy, then
forget my comment. I don't have this kind of system to test the patch on, it
was just that as I was looking it over, all the casts kind of made my head
swim.

The narrowing cast is here:
+static inline void enable_gart_translation(struct pci_dev *dev, u64 addr)
+{
+ u32 tmp, ctl;
+
+ /* address of the mappings table */
+ addr >>= 12;
+ tmp = (u32) addr<<4;

Also, putting on my Andrew Morton cap, which I should have done before,
that function is too big to be inlined so you should probably drop the explicit
`inline' and let the compiler decide.

Add a
Reviewed-by: SL Baur <steve [at] xemacs>
if that's appropriate. I'm satisified that you did a one-for-one code move.

-sb
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


andi at firstfloor

Apr 16, 2008, 11:29 PM

Post #8 of 9 (1315 views)
Permalink
Re: gart: factor out common code [In reply to]

SL Baur wrote:
> On 4/16/08, Pavel Machek <pavel [at] ucw> wrote:
>> On Tue 2008-04-15 16:25:29, SL Baur wrote:
>> > On 4/15/08, Pavel Machek <pavel [at] ucw> wrote:
>
>> > Other reviewer comments:
>> > The casting between struct pci_dev * and u64 is kind of ugly,
>> > I presume the reason for the narrowing casts in the block of
>> > code you moved is documented somewhere.
>>
>>
>> Where am I doing that?
>
> The struct pci_dev */u64 casting is in init_k8_gatt. If Ingo is happy, then
> forget my comment. I don't have this kind of system to test the patch on, it
> was just that as I was looking it over, all the casts kind of made my head
> swim.

I think the casts make it more clear because the register is 32bit.
The only truly ugly bit is naming a variable "tmp".

> The narrowing cast is here:
> +static inline void enable_gart_translation(struct pci_dev *dev, u64 addr)
> +{
> + u32 tmp, ctl;
> +
> + /* address of the mappings table */
> + addr >>= 12;
> + tmp = (u32) addr<<4;
>
> Also, putting on my Andrew Morton cap, which I should have done before,
> that function is too big to be inlined so you should probably drop the explicit
> `inline' and let the compiler decide.

In a header static inline is needed, otherwise the compiler will warn
about an unused static if it's not used which is unfortunate because
not every includer wants to use all functions.

That said the function should probably not be in a header.

-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


steve at xemacs

Apr 17, 2008, 12:34 AM

Post #9 of 9 (1308 views)
Permalink
Re: gart: factor out common code [In reply to]

On 4/16/08, Andi Kleen <andi [at] firstfloor> wrote:
> SL Baur wrote:

> I think the casts make it more clear because the register is 32bit.
> The only truly ugly bit is naming a variable "tmp".

Good point.

> > that function is too big to be inlined so you should probably drop the explicit
> > `inline' and let the compiler decide.

> In a header static inline is needed, otherwise the compiler will warn
> about an unused static if it's not used which is unfortunate because
> not every includer wants to use all functions.
>
> That said the function should probably not be in a header.

Yes. Agreed. I'm happy enough having common duplicated
code removed, but I know Ingo is picky about saving space
whenever he can, so I thought I would point it out now before
it becomes a maintenance issue.

Thanks for the feedback. I'll focus harder on future reviews.

-sb
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.