This is a possible mod to change the way resource maps are done.
Unify the big mess into a little mess. This code has all the functionality of the 4 or 5 rmap functions from v2 ..
This includes my proposed pci config patch.
Signed-off-by: Ronald G. Minnich rminnich@gmail.com
On Mon, Aug 04, 2008 at 09:01:20AM -0700, ron minnich wrote:
This is a possible mod to change the way resource maps are done.
Unify the big mess into a little mess. This code has all the functionality of the 4 or 5 rmap functions from v2 ..
Awesome. But..
+void setup_resource_map_x_offset(const rmap *rm, u32 max,
u32 offset_dev, u32 offset_pciio,u32 offset_io)+{
- u32 i;
- printk(BIOS_DEBUG, "setting up resource map ex offset....\n");
- for(i = 0; i < max; i++, rm++) {
switch (rm->type){case PCIRM:{u32 dev;unsigned where;unsigned long reg;printk(BIOS_DEBUG, "(%x,%x+%x,%x,%x) & %08x | %08x+%08x\n", r->pcm.bus,r->pcm.dev+offset_dev,r->pcm.fn,r->pcm.reg,r->pcm.and,r->pcm.or, offset_pciio);
This will not build. r is undeclared. :) When fixed,
Acked-by: Peter Stuge peter@stuge.se
//Peter
On 04.08.2008 18:01, ron minnich wrote:
This is a possible mod to change the way resource maps are done.
Unify the big mess into a little mess. This code has all the functionality of the 4 or 5 rmap functions from v2 ..
This includes my proposed pci config patch.
Signed-off-by: Ronald G. Minnich rminnich@gmail.com
Can you hold off committing a bit? I think I can come up with a cleaner alternative to the pci config patch (hopefully without any code changes).
Regards, Carl-Daniel
On Mon, Aug 04, 2008 at 07:27:05PM +0200, Carl-Daniel Hailfinger wrote:
This includes my proposed pci config patch.
Can you hold off committing a bit? I think I can come up with a cleaner alternative to the pci config patch (hopefully without any code changes).
Please stop creating wait states.
Apply Ron's patch to your local tree, work there, then send a patch to apply on top of Ron's commit.
//Peter
On 04.08.2008 19:32, Peter Stuge wrote:
On Mon, Aug 04, 2008 at 07:27:05PM +0200, Carl-Daniel Hailfinger wrote:
This includes my proposed pci config patch.
Can you hold off committing a bit? I think I can come up with a cleaner alternative to the pci config patch (hopefully without any code changes).
Please stop creating wait states.
Apply Ron's patch to your local tree, work there, then send a patch to apply on top of Ron's commit.
If I'm successful, I'll be undoing 50% of Ron's patch.
- pci_write_config32(a, b, c); + pci_cf8_conf1.write32(NULL, 0, a, b, c);
Can't we just use a macro for that stuff? #define PCI_WRITE_CONFIG32_EARLY(a,b,c) pci_cf8_conf1.write32(NULL, 0, a, b, c);
Do this, and 50% of Ron's changes are obsolete.
Regards, Carl-Daniel
On Mon, Aug 04, 2008 at 07:50:32PM +0200, Carl-Daniel Hailfinger wrote:
a patch to apply on top of Ron's commit.
If I'm successful, I'll be undoing 50% of Ron's patch.
That doesn't matter. This is early work in progress. Key word: progress
- pci_write_config32(a, b, c);
- pci_cf8_conf1.write32(NULL, 0, a, b, c);
Can't we just use a macro for that stuff? #define PCI_WRITE_CONFIG32_EARLY(a,b,c) pci_cf8_conf1.write32(NULL, 0, a, b, c);
Sorry, I don't see the point. I expect pci_cf8_* to change at least once anyway. I think time is better spent elsewhere.
//Peter
On 04.08.2008 19:56, Peter Stuge wrote:
On Mon, Aug 04, 2008 at 07:50:32PM +0200, Carl-Daniel Hailfinger wrote:
a patch to apply on top of Ron's commit.
If I'm successful, I'll be undoing 50% of Ron's patch.
That doesn't matter. This is early work in progress. Key word: progress
If something has to be rolled back, it's the opposite of progress.
- pci_write_config32(a, b, c);
- pci_cf8_conf1.write32(NULL, 0, a, b, c);
Can't we just use a macro for that stuff? #define PCI_WRITE_CONFIG32_EARLY(a,b,c) pci_cf8_conf1.write32(NULL, 0, a, b, c);
Sorry, I don't see the point. I expect pci_cf8_* to change at least once anyway. I think time is better spent elsewhere.
That's why I don't want it committed in the first place. The least intrusive change would be the following #define at the beginning of each affected C file:
#define pci_write_config32(a,b,c) pci_cf8_conf1.write32(NULL, 0, a, b, c);
No other code changes needed. It reduces code churn and keeps the familiar interface. With that change, I see no reason to hold the commit back.
Regards, Carl-Daniel
On Mon, Aug 4, 2008 at 11:11 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
#define pci_write_config32(a,b,c) pci_cf8_conf1.write32(NULL, 0, a, b, c);
Yeah I can do that. I think it is more confusing but I'm not picky. I'd rather see the raw form as a reminder.
But if the other form is preferred it takes me about 2 seconds to change :-)
ron
On 04.08.2008 20:27, ron minnich wrote:
On Mon, Aug 4, 2008 at 11:11 AM, Carl-Daniel Hailfinger wrote:
#define pci_write_config32(a,b,c) pci_cf8_conf1.write32(NULL, 0, a, b, c);
Yeah I can do that. I think it is more confusing but I'm not picky. I'd rather see the raw form as a reminder.
I can't argue with that one.
But if the other form is preferred it takes me about 2 seconds to change :-)
Peter already gave you an Ack for the patch (see his comment about compilation, though). If you remove the pci_write_config part of the patch and/or replace it with a macro, the patch is also Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
I do have some minor comments, but feel free to commit without addressing them. Cut-and-paste reduced to the areas I want to comment on.
+/**
- setup a resource map.
- for PCRM entries, add a pci device offset, and a pci "OR value offset"
"for PCIRM entries,..." "OR value offset is not entirely clear.
- for IO8 and IO32 entries, add an io base offset.
- This function combines a bunch of seperate functions that were scattered
- throughout v2. It may be awkward but it does allow for one rmap for
- all settings, which is handy. See include/arch/x86/cpu.h for usage on
- how to set up a resource map.
- @param rm The resource map
- @param max The map size
- @param offset_dev pci device offset. This can be useful on e.g. k8
we have a number of similar devices which need the same setups
we can use one map for more than one device. NOTE:
offset_dev IS NOT ASSUMED TO BE OFFSET BY FN (i.e. it is not << 3)
* offset_dev IS NOT ASSUMED TO BE SHIFTED BY FN (i.e. it is not << 3)
- @param offset_pciio added to the OR value for setting up PCI IO
- @param offset_io offset from the io base in the resource map
- */
+void setup_resource_map_x_offset(const rmap *rm, u32 max,
u32 offset_dev, u32 offset_pciio,u32 offset_io)
I had some trouble understanding the comments, but after rereading them they look pretty ok.
/* pci config map */ struct pcm
struct pcicm would be a bit more readable.
struct io8
Maybe struct io8_modify or struct io8_change? Not urgent.
/* convenience initializer */ #define PCM(abus,adev,afn,areg,aand,aor) {.type = PCIRM, {.pcm ={.bus=abus,.dev=adev,.fn=afn,.reg=areg,.and=aand,.or=aor}}} #define EIO8(aport, aand, aor) {.type=IO8, {.io8 = {.port = aport, .and = aand, .or = aor}}} #define EIO32(aport, aand, aaor) {.type = IO32, {.io32 = {.port = aport, .and = aand, .or = aor}}}
Consistent naming, please. If "E" stands for initializer, please use EPCIRM instead of PCM.
Overall, I like the patch and want to see it in the tree. Your work is very valuable. I hope you didn't get the impression that I want to stall you.
Regards, Carl-Daniel
On Mon, Aug 4, 2008 at 1:50 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Overall, I like the patch and want to see it in the tree. Your work is very valuable. I hope you didn't get the impression that I want to stall you.
not at all!
thanks
ron
With a bug fix from peter and a suggest change by Carl-Daniel
Resource map and a way to use the pci config stuff from stage 1, done in a way that will work in stage 2 (but only for systems that use type 1 config cycles; will fail for type MEM config cycles).
We need to rethink the PCI config stuff per Stepan's comment, in part because the device tree now includes things that are NOT PCI devices. Stepan's suggestion, to make the functions take busdevfn as the parameter, makes a lot of sense.
Committed revision 720.
On Mon, Aug 04, 2008 at 06:20:49PM -0700, ron minnich wrote:
Committed revision 720.
Cool.
We need to rethink the PCI config stuff per Stepan's comment, in part because the device tree now includes things that are NOT PCI devices. Stepan's suggestion, to make the functions take busdevfn as the parameter, makes a lot of sense.
Didn't we want to use a flat tree in early code? Was that just me?
//Peter
Carl-Daniel Hailfinger wrote:
On 04.08.2008 19:56, Peter Stuge wrote:
Please stop creating wait states. Apply Ron's patch to your local tree, work there, then send a patch to apply on top of Ron's commit.
On Mon, Aug 04, 2008 at 07:50:32PM +0200, Carl-Daniel Hailfinger wrote:
a patch to apply on top of Ron's commit.
If I'm successful, I'll be undoing 50% of Ron's patch.
That doesn't matter. This is early work in progress. Key word: progress
If something has to be rolled back, it's the opposite of progress.
More so are endless discussions of what could happen if one had more time. In fact, they're the reason we do not have a reasonably working version of v3 yet.
Stefan
On Mon, Aug 04, 2008 at 07:56:03PM +0200, Peter Stuge wrote:
On Mon, Aug 04, 2008 at 07:50:32PM +0200, Carl-Daniel Hailfinger wrote:
a patch to apply on top of Ron's commit.
If I'm successful, I'll be undoing 50% of Ron's patch.
That doesn't matter. This is early work in progress. Key word: progress
Doesn't mean we have to commit everything right away though, too. A certain amount of time for reviews and discussions before committing anything is needed and a good idea (doesn't have to be weeks, but at least a few hours or a day or so shouldn't be a problem).
(I'm not commenting on this specific patch or technical issue, but I cannot agree with the general idea of "no delays, commit everything ASAP no matter what").
Uwe.
On 04/08/08 21:36 +0200, Uwe Hermann wrote:
On Mon, Aug 04, 2008 at 07:56:03PM +0200, Peter Stuge wrote:
On Mon, Aug 04, 2008 at 07:50:32PM +0200, Carl-Daniel Hailfinger wrote:
a patch to apply on top of Ron's commit.
If I'm successful, I'll be undoing 50% of Ron's patch.
That doesn't matter. This is early work in progress. Key word: progress
Doesn't mean we have to commit everything right away though, too. A certain amount of time for reviews and discussions before committing anything is needed and a good idea (doesn't have to be weeks, but at least a few hours or a day or so shouldn't be a problem).
(I'm not commenting on this specific patch or technical issue, but I cannot agree with the general idea of "no delays, commit everything ASAP no matter what").
We need to find the middle ground. Development doesn't have to stop for everybody to give the patch a checkmark. We do _not_ need to be on the same page for the project to proceed. Discussion is good, but not mandatory.
Jordan
ron minnich wrote:
This is a possible mod to change the way resource maps are done.
Unify the big mess into a little mess. This code has all the functionality of the 4 or 5 rmap functions from v2 ..
This includes my proposed pci config patch.
Signed-off-by: Ronald G. Minnich rminnich@gmail.com
I don't understand the patch. Why does having a single function for "resource maps" require pci_read_configXY to be renamed? And how does that reduce the mess?
- nbcap = pci_read_config32(ctrl->f3, NORTHBRIDGE_CAP); - dcl = pci_read_config32(ctrl->f2, DRAM_CONFIG_LOW); + nbcap = pci_cf8_conf1.read32(NULL, 0, ctrl->f3, NORTHBRIDGE_CAP); + dcl = pci_cf8_conf1.read32(NULL, 0, ctrl->f2, DRAM_CONFIG_LOW);
I never saw a conf2 system since 1994, so putting this in the name is an anachronism. No conf2 system could ever work anyways without signigicantly changing the code.
On Mon, Aug 04, 2008 at 08:44:28PM +0200, Stefan Reinauer wrote:
I don't understand the patch. Why does having a single function for "resource maps" require pci_read_configXY to be renamed?
These are two unrelated changes. I don't think that matters so much right now.
//Peter
let's take it in two pieces.
Everyone ok with the resource map patch? It's pretty simple.
second piece.
On Mon, Aug 4, 2008 at 11:44 AM, Stefan Reinauer stepan@coresystems.de wrote:
I don't understand the patch. Why does having a single function for "resource maps" require pci_read_configXY to be renamed? And how does that reduce the mess?
The resource map and the configXY are totally unrelated.
romcc-based code used to have: nbcap = pci_read_config32(ctrl->f3, NORTHBRIDGE_CAP);
This worked in v2 due to abuse of cpp and a few other bits. I was never comfortable with it -- more magic than I like. in romcc and CAR code, a device_t was a u32; in other code, it was a struct device *. icky.
In v3 pci_read_config32 takes a struct device *. We don't have them in stage 1. So to make this work, I do this:
nbcap = pci_cf8_conf1.read32(NULL, 0, ctrl->f3, NORTHBRIDGE_CAP);
Which will work fine.
I'm not that concerned with syntactic sugar to cover this up, such as the define at front. I'm actually *more* comfortable with this, since it makes it very clear what's going on. Nobody will come along later and get confused.
I never saw a conf2 system since 1994, so putting this in the name is an anachronism. No conf2 system could ever work anyways without signigicantly changing the code.
I did not pick the name, it's the name of the struct with the function pointers.
I've got an ack from peter and I'd like to commit. Yes these are two patches but --key point -- they'll work and we can keep going.
Thanks
ron
ron minnich wrote:
romcc-based code used to have: nbcap = pci_read_config32(ctrl->f3, NORTHBRIDGE_CAP);
This worked in v2 due to abuse of cpp and a few other bits.
I think that was very elegant actually, given the assumption that we need a device_t (I still like that better than struct device) to do a simple pci config read/write,... but...
I was never comfortable with it -- more magic than I like. in romcc and CAR code, a device_t was a u32; in other code, it was a struct device *. icky.
In v3 pci_read_config32 takes a struct device *. We don't have them in stage 1. So to make this work, I do this:
nbcap = pci_cf8_conf1.read32(NULL, 0, ctrl->f3, NORTHBRIDGE_CAP);
Which will work fine.
The obvious question: Do we need to use device_t as an input to that function at all? Why don't we instead use simple pci functions taking a u32 as an input? We're a bios, after all, not an OS. Are we?
I did not pick the name, it's the name of the struct with the function pointers.
I've got an ack from peter and I'd like to commit. Yes these are two patches but --key point -- they'll work and we can keep going.
Sure, keep going. I'll wait and see what comes out of this in the end.
Stefan
On Mon, Aug 4, 2008 at 1:40 PM, Stefan Reinauer stepan@coresystems.de wrote:
The obvious question: Do we need to use device_t as an input to that function at all? Why don't we instead use simple pci functions taking a u32 as an input? We're a bios, after all, not an OS. Are we?
That's a fine question. The convention could be that we call those functions as follows: blahblah(device->bus, device->devfn, etc. etc. )
works for me. But let's revisit it later.
I am fine with overloading but only in languages that really allow it ;-)
ron
ron minnich wrote:
On Mon, Aug 4, 2008 at 1:40 PM, Stefan Reinauer stepan@coresystems.de wrote:
The obvious question: Do we need to use device_t as an input to that function at all? Why don't we instead use simple pci functions taking a u32 as an input? We're a bios, after all, not an OS. Are we?
That's a fine question. The convention could be that we call those functions as follows: blahblah(device->bus, device->devfn, etc. etc. )
pci_read_config32(PCI_DEV(0,0x1f,0), reg); ?
Maps to about 5 assembler instructions.
works for me. But let's revisit it later.
I am fine with overloading but only in languages that really allow it ;-)
Overloading? I'm talking about simplifying, not inventing an OS ;-)
On 04.08.2008 20:44, Stefan Reinauer wrote:
I never saw a conf2 system since 1994, so putting this in the name is an anachronism. No conf2 system could ever work anyways without signigicantly changing the code.
By the way, I found a system manufactured in 2003 which apparently needs conf2: The Gericom Masterpiece 2440 XL laptop. Not that I would want to revert the conf2 removal, but I think it's important to keep the record about conf2 complete. I'd like to keep conf2 code in the v2 tree, though.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
I'd like to keep conf2 code in the v2 tree, though.
There is no working version of conf2 in v2, there never was.
-
Carl-Daniel Hailfinger -
Jordan Crouse -
Peter Stuge -
ron minnich -
Stefan Reinauer -
Uwe Hermann