Myles,
Should the domain functions run before the device functions? It looks like the device functions run before the domain but I would expect it to be the other way. This is a problem for Geode LX. It has the VSA and graphics memory init in the domain phase2_fixup but it is running after the 5536 devices run phase2_fixup and is a problem.
statictree: http://pastebin.ca/1301081 .next = &dev_domain_0 - is the link from the last pci device in the domain to the domain device.
serial output: http://pastebin.ca/1301072
Marc
Myles,
Should the domain functions run before the device functions?
Yes.
It looks like the device functions run before the domain but I would expect it to be the other way.
Me too.
This is a problem for Geode LX. It has the VSA and graphics memory init in the domain phase2_fixup but it is running after the 5536 devices run phase2_fixup and is a problem.
Sorry it broke. Since I don't have the hardware I can't test very well. I had a patch that moved those functions, but I broke something else and re-simplified. I'll send you a patch, and maybe together we can do it right.
statictree: http://pastebin.ca/1301081 .next = &dev_domain_0 - is the link from the last pci device in the domain to the domain device.
Yeah. That seems like the "root" of the problem. Because of the way the statictree is made, anything done in device order ends up being depth-first. You have to specify to make things breadth first, which seems like the correct way.
serial output: http://pastebin.ca/1301072
Thanks, Myles
Marc,
Hopefully this makes everything right again. I still think some of the geode functions should be moved, but that's really a separate issue.
You have to specify to make things breadth first, which seems like the correct way.
I guess it's not really breadth first. It's just parents before siblings.
I think this should be done for all the phases unless there's some compelling reason not to.
Signed-off-by: Myles Watson mylesgw@gmail.com
Thanks, Myles
On Tue, Jan 6, 2009 at 3:37 PM, Myles Watson mylesgw@gmail.com wrote:
Marc,
Hopefully this makes everything right again. I still think some of the geode functions should be moved, but that's really a separate issue.
You have to specify to make things breadth first, which seems like the correct way.
I guess it's not really breadth first. It's just parents before siblings.
I think this should be done for all the phases unless there's some compelling reason not to.
Signed-off-by: Myles Watson mylesgw@gmail.com
I agree it is parents before siblings but I thought it looked more like a problem with the statictree. I must not understand .next.
from http://pastebin.ca/1301081
dev_apic_0 has a .sibling = &dev_domain_0 and .next = &dev_domain_0_pci_1_0
Then later in the last device domain dev_domain_0_pci_f_2 has a .next = &dev_domain_0.
I would expect .sibling and .next to point to the &dev_domain_0 and then dev_domain_0 to have .next = &dev_domain_0_pci_1_0
dev_cpus looks like I would expect .sibling = &dev_apic_0 and .next = &dev_apic_0. dev_domain_0_pci_f_0 also has the same .sibling and .next.
Can you explain the last pci dev .next pointing back to the domain?
I am not setup to test this either. I was talking with Mart on NAND device problems. Maybe he can test it tomorrow.
Marc
On Tue, Jan 6, 2009 at 4:13 PM, Marc Jones marcj303@gmail.com wrote:
I agree it is parents before siblings but I thought it looked more like a problem with the statictree. I must not understand .next.
from http://pastebin.ca/1301081
dev_apic_0 has a .sibling = &dev_domain_0 and .next = &dev_domain_0_pci_1_0
Then later in the last device domain dev_domain_0_pci_f_2 has a .next = &dev_domain_0.
I would expect .sibling and .next to point to the &dev_domain_0 and then dev_domain_0 to have .next = &dev_domain_0_pci_1_0
Yes. I would too. It's that way because parents get defined after children. When the tree is being read, the full parent is not read (we don't get to the closing brace) until after we read all the children. It seems confusing to me, but it is the way dts works.
dev_cpus looks like I would expect .sibling = &dev_apic_0 and .next = &dev_apic_0. dev_domain_0_pci_f_0 also has the same .sibling and .next.
Yes. Children are complete before their sibling is read in, so all devices with childless siblings have the same .sibling and .next.
I hope that doesn't sound like blather :)
Thanks, Myles
Here's the rest of the geodelx fixup patch too. I think this is more intuitive, but again I don't have the hardware.
Signed-off-by: Myles Watson mylesgw@gmail.com
Thanks, Myles
On Tue, Jan 6, 2009 at 4:29 PM, Myles Watson mylesgw@gmail.com wrote:
Here's the rest of the geodelx fixup patch too. I think this is more intuitive, but again I don't have the hardware.
Signed-off-by: Myles Watson mylesgw@gmail.com
This matches the resource allocation work you did much better.
Can you make the dts changes to the other Geode mainboards when you check it in? amd/norwich amd/db800 artec/dbe61 artec/dbe62 pcengines/alix1c pcengines/alix2c3
I think that is all of them.
Acked-by: Marc Jones marcj303@gmail.com
Thanks, Marc
On Wed, Jan 7, 2009 at 10:23 AM, Myles Watson mylesgw@gmail.com wrote:
This matches the resource allocation work you did much better.
Thanks.
Can you make the dts changes to the other Geode mainboards when you check it in?
Sure.
pcengines/alix2c3
This one doesn't have any graphics memory reserved. Is that on purpose?
Yes, It is headless.
Marc
On Wed, Jan 7, 2009 at 10:05 AM, Marc Jones marcj303@gmail.com wrote:
On Tue, Jan 6, 2009 at 4:29 PM, Myles Watson mylesgw@gmail.com wrote:
Here's the rest of the geodelx fixup patch too. I think this is more intuitive, but again I don't have the hardware.
Signed-off-by: Myles Watson mylesgw@gmail.com
This matches the resource allocation work you did much better.
Can you make the dts changes to the other Geode mainboards when you check it in? amd/norwich amd/db800 artec/dbe61 artec/dbe62 pcengines/alix1c pcengines/alix2c3
Done.
I think that is all of them.
Acked-by: Marc Jones marcj303@gmail.com
Rev 1103.
Thanks, Myles
On Wed, Jan 07, 2009 at 11:27:01AM -0700, Myles Watson wrote:
On Wed, Jan 7, 2009 at 10:05 AM, Marc Jones marcj303@gmail.com wrote:
On Tue, Jan 6, 2009 at 4:29 PM, Myles Watson mylesgw@gmail.com wrote:
Here's the rest of the geodelx fixup patch too. I think this is more intuitive, but again I don't have the hardware.
Signed-off-by: Myles Watson mylesgw@gmail.com
This matches the resource allocation work you did much better.
Can you make the dts changes to the other Geode mainboards when you check it in? amd/norwich amd/db800 artec/dbe61 artec/dbe62 pcengines/alix1c pcengines/alix2c3
Done.
I think that is all of them.
Acked-by: Marc Jones marcj303@gmail.com
Rev 1103.
Marc asked me to test on geode - I tried on an alix2c3 just now with v1104. Looks like something broke (but perhaps not because of this particular checkin - the last known ok v3 version for alix2c3 is 682). Basically, the alix resets itself right after (during?)
cs5536: southbridge_init
Bootlog attached.
Help?
Thanks, Ward.
On Wed, Jan 07, 2009 at 09:01:37PM -0500, Ward Vandewege wrote:
Acked-by: Marc Jones marcj303@gmail.com
Rev 1103.
Marc asked me to test on geode - I tried on an alix2c3 just now with v1104. Looks like something broke (but perhaps not because of this particular checkin - the last known ok v3 version for alix2c3 is 682). Basically, the alix resets itself right after (during?)
cs5536: southbridge_init
Bootlog attached.
Help?
The rev that broke the boot was 1089 or 1090. 1089 does not build, so I can't tell for sure. 1088 boots fine. 1090 is broken.
Btw: buildrom + artec dongle + alix2c3 -> 40 second turnaround time between boots (and that includes 10 seconds for me). This is *nice*.
Thanks, Ward.
On Wed, Jan 7, 2009 at 7:31 PM, Ward Vandewege ward@gnu.org wrote:
On Wed, Jan 07, 2009 at 09:01:37PM -0500, Ward Vandewege wrote:
Acked-by: Marc Jones marcj303@gmail.com
Rev 1103.
Marc asked me to test on geode - I tried on an alix2c3 just now with v1104. Looks like something broke (but perhaps not because of this particular checkin - the last known ok v3 version for alix2c3 is 682). Basically, the alix resets itself right after (during?)
cs5536: southbridge_init
Bootlog attached.
Help?
The rev that broke the boot was 1089 or 1090. 1089 does not build, so I can't tell for sure. 1088 boots fine. 1090 is broken.
They might have well been the same commit because they were interdependent. It was just an easier way of looking at the changes for me.
Thanks for tracking it down. Could you send me a log from 1088? Hopefully we can track it down quickly.
Btw: buildrom + artec dongle + alix2c3 -> 40 second turnaround time between boots (and that includes 10 seconds for me). This is *nice*.
That is nice. I wish my Tyan boards had < 1min turnaround.
Thanks, Myles
On Wed, Jan 07, 2009 at 09:39:39PM -0700, Myles Watson wrote:
The rev that broke the boot was 1089 or 1090. 1089 does not build, so I can't tell for sure. 1088 boots fine. 1090 is broken.
They might have well been the same commit because they were interdependent. It was just an easier way of looking at the changes for me.
Thanks for tracking it down. Could you send me a log from 1088? Hopefully we can track it down quickly.
Sure, see attached.
Btw: buildrom + artec dongle + alix2c3 -> 40 second turnaround time between boots (and that includes 10 seconds for me). This is *nice*.
That is nice. I wish my Tyan boards had < 1min turnaround.
Well, with the dongle + plcc adapter, you might get pretty close to that... I have not tried with a tyan board and plcc adapter yet though.
Thanks, Ward.
On Thu, Jan 8, 2009 at 6:58 AM, Ward Vandewege ward@gnu.org wrote:
On Wed, Jan 07, 2009 at 09:39:39PM -0700, Myles Watson wrote:
The rev that broke the boot was 1089 or 1090. 1089 does not build, so I can't tell for sure. 1088 boots fine. 1090 is broken.
They might have well been the same commit because they were interdependent. It was just an easier way of looking at the changes for me.
Thanks for tracking it down. Could you send me a log from 1088? Hopefully we can track it down quickly.
Sure, see attached.
The biggest difference that I see is that there is no graphics device (PCI 1.1) in the broken one. In 1088 it was found dynamically (not in the tree), but now it isn't found at all.
Since it was supposed to be disabled by: /* this board does not really have vga; disable it (pci device 00:01.1) */ unwanted_vpci = < 80000900 0 >; in the dts, I'm not sure why it disappeared now.
Possible solution: - Try it again without the unwanted_vpci line in the dts.
I guess I don't understand why the device is gone now. I didn't think the changes I've made would make the device disappear.
Btw: buildrom + artec dongle + alix2c3 -> 40 second turnaround time between boots (and that includes 10 seconds for me). This is *nice*.
That is nice. I wish my Tyan boards had < 1min turnaround.
Well, with the dongle + plcc adapter, you might get pretty close to that... I have not tried with a tyan board and plcc adapter yet though.
Good to know.
Thanks, Myles
On Thu, Jan 08, 2009 at 08:52:44AM -0700, Myles Watson wrote:
On Thu, Jan 8, 2009 at 6:58 AM, Ward Vandewege ward@gnu.org wrote:
On Wed, Jan 07, 2009 at 09:39:39PM -0700, Myles Watson wrote:
The rev that broke the boot was 1089 or 1090. 1089 does not build, so I can't tell for sure. 1088 boots fine. 1090 is broken.
They might have well been the same commit because they were interdependent. It was just an easier way of looking at the changes for me.
Thanks for tracking it down. Could you send me a log from 1088? Hopefully we can track it down quickly.
Sure, see attached.
The biggest difference that I see is that there is no graphics device (PCI 1.1) in the broken one. In 1088 it was found dynamically (not in the tree), but now it isn't found at all.
Since it was supposed to be disabled by: /* this board does not really have vga; disable it (pci device 00:01.1) */ unwanted_vpci = < 80000900 0 >; in the dts, I'm not sure why it disappeared now.
Possible solution:
- Try it again without the unwanted_vpci line in the dts.
I guess I don't understand why the device is gone now. I didn't think the changes I've made would make the device disappear.
I have not tried that yet, but here's a boot log from v1108, which still does not boot but it stops in a different place now, it seems.
Thanks, Ward.
On Thu, Jan 8, 2009 at 11:00 AM, Ward Vandewege ward@gnu.org wrote:
On Thu, Jan 08, 2009 at 08:52:44AM -0700, Myles Watson wrote:
On Thu, Jan 8, 2009 at 6:58 AM, Ward Vandewege ward@gnu.org wrote:
On Wed, Jan 07, 2009 at 09:39:39PM -0700, Myles Watson wrote:
The rev that broke the boot was 1089 or 1090. 1089 does not build, so I can't tell for sure. 1088 boots fine. 1090 is broken.
They might have well been the same commit because they were interdependent. It was just an easier way of looking at the changes for me.
Thanks for tracking it down. Could you send me a log from 1088? Hopefully we can track it down quickly.
Sure, see attached.
The biggest difference that I see is that there is no graphics device (PCI 1.1) in the broken one. In 1088 it was found dynamically (not in the tree), but now it isn't found at all.
Since it was supposed to be disabled by: /* this board does not really have vga; disable it (pci device 00:01.1) */ unwanted_vpci = < 80000900 0 >; in the dts, I'm not sure why it disappeared now.
Possible solution:
- Try it again without the unwanted_vpci line in the dts.
I guess I don't understand why the device is gone now. I didn't think the changes I've made would make the device disappear.
I have not tried that yet, but here's a boot log from v1108, which still does not boot but it stops in a different place now, it seems.
Thanks for your patience. One problem is that there is no area reserved for the APIC and the ROM. I think we should add a reserved resource in the domain that goes from 0xfc000000-0xffffffff. That will stop allocations from going there.
I copied the reserved areas from i440bx_emulation. Like the comments say, I know these need to be reserved areas, but I'm not exactly sure where they belong.
Compile tested.
Signed-off-by: Myles Watson mylesgw@gmail.com
Thanks, Myles
On Thu, Jan 8, 2009 at 11:27 AM, Myles Watson mylesgw@gmail.com wrote:
On Thu, Jan 8, 2009 at 11:00 AM, Ward Vandewege ward@gnu.org wrote:
On Thu, Jan 08, 2009 at 08:52:44AM -0700, Myles Watson wrote:
On Thu, Jan 8, 2009 at 6:58 AM, Ward Vandewege ward@gnu.org wrote:
On Wed, Jan 07, 2009 at 09:39:39PM -0700, Myles Watson wrote:
The rev that broke the boot was 1089 or 1090. 1089 does not build, so I can't tell for sure. 1088 boots fine. 1090 is broken.
They might have well been the same commit because they were interdependent. It was just an easier way of looking at the changes for me.
Thanks for tracking it down. Could you send me a log from 1088? Hopefully we can track it down quickly.
Sure, see attached.
The biggest difference that I see is that there is no graphics device (PCI 1.1) in the broken one. In 1088 it was found dynamically (not in the tree), but now it isn't found at all.
Since it was supposed to be disabled by: /* this board does not really have vga; disable it (pci device 00:01.1) */ unwanted_vpci = < 80000900 0 >; in the dts, I'm not sure why it disappeared now.
Possible solution:
- Try it again without the unwanted_vpci line in the dts.
I guess I don't understand why the device is gone now. I didn't think the changes I've made would make the device disappear.
I have not tried that yet, but here's a boot log from v1108, which still does not boot but it stops in a different place now, it seems.
Thanks for your patience. One problem is that there is no area reserved for the APIC and the ROM. I think we should add a reserved resource in the domain that goes from 0xfc000000-0xffffffff. That will stop allocations from going there.
I copied the reserved areas from i440bx_emulation. Like the comments say, I know these need to be reserved areas, but I'm not exactly sure where they belong.
Compile tested.
Signed-off-by: Myles Watson mylesgw@gmail.com
There is just a PIC in the Geode so you don't need to reserve the LAPIC range. You do still need to reserve the ROM range. Just fix up the comment.
Acked-by: Marc Jones marcj303@gmail.com
Marc
I copied the reserved areas from i440bx_emulation. Like the comments say, I know these need to be reserved areas, but I'm not exactly sure where they belong.
Compile tested.
Signed-off-by: Myles Watson mylesgw@gmail.com
There is just a PIC in the Geode so you don't need to reserve the LAPIC range. You do still need to reserve the ROM range. Just fix up the comment.
Acked-by: Marc Jones marcj303@gmail.com
Rev 1111.
Thanks, Myles
On Thu, Jan 08, 2009 at 01:07:55PM -0700, Myles Watson wrote:
I copied the reserved areas from i440bx_emulation. Like the comments say, I know these need to be reserved areas, but I'm not exactly sure where they belong.
Compile tested.
Signed-off-by: Myles Watson mylesgw@gmail.com
There is just a PIC in the Geode so you don't need to reserve the LAPIC range. You do still need to reserve the ROM range. Just fix up the comment.
Acked-by: Marc Jones marcj303@gmail.com
Rev 1111.
Still the same, sadly, cf attached.
Thanks, Ward.
On Thu, Jan 8, 2009 at 1:17 PM, Ward Vandewege ward@gnu.org wrote:
On Thu, Jan 08, 2009 at 01:07:55PM -0700, Myles Watson wrote:
I copied the reserved areas from i440bx_emulation. Like the comments say, I know these need to be reserved areas, but I'm not exactly sure where they belong.
Compile tested.
Signed-off-by: Myles Watson mylesgw@gmail.com
There is just a PIC in the Geode so you don't need to reserve the LAPIC range. You do still need to reserve the ROM range. Just fix up the comment.
Acked-by: Marc Jones marcj303@gmail.com
Rev 1111.
Still the same, sadly, cf attached.
I wasn't traversing the tree completely when constraining resources. This patch adds the domain's resources and all links (not just link 0) to the tree traversal. Build and boot tested on qemu and serengeti.
Signed-off-by: Myles Watson mylesgw@gmail.com
Thanks, Myles
On Thu, Jan 08, 2009 at 01:57:39PM -0700, Myles Watson wrote:
And the patch :)
Yay, that fixes booting (log attached).
Signed-off-by: Myles Watson mylesgw@gmail.com
Acked-by: Ward Vandewege ward@gnu.org
Thanks, Ward.
On Thu, Jan 08, 2009 at 01:57:39PM -0700, Myles Watson wrote:
And the patch :)
Yay, that fixes booting (log attached).
Whew!
Signed-off-by: Myles Watson mylesgw@gmail.com
Acked-by: Ward Vandewege ward@gnu.org
Rev 1112.
Thanks, Myles
On Thu, Jan 8, 2009 at 1:17 PM, Ward Vandewege ward@gnu.org wrote:
On Thu, Jan 08, 2009 at 01:07:55PM -0700, Myles Watson wrote:
I copied the reserved areas from i440bx_emulation. Like the comments say, I know these need to be reserved areas, but I'm not exactly sure where they belong.
Compile tested.
Signed-off-by: Myles Watson mylesgw@gmail.com
There is just a PIC in the Geode so you don't need to reserve the LAPIC range. You do still need to reserve the ROM range. Just fix up the comment.
Acked-by: Marc Jones marcj303@gmail.com
Rev 1111.
Still the same, sadly, cf attached.
I see the graphics device still getting setup even though it has no memory. I think that is might be a problem. This patch hides the header before resource allocation and should avoid the problem and is completely untested.....
Marc
-----Original Message----- From: Marc Jones [mailto:marcj303@gmail.com] Sent: Thursday, January 08, 2009 2:16 PM To: Myles Watson; Marc Jones; ron minnich; Coreboot Subject: Re: [coreboot] domain vs device statictree order
On Thu, Jan 8, 2009 at 1:17 PM, Ward Vandewege ward@gnu.org wrote:
On Thu, Jan 08, 2009 at 01:07:55PM -0700, Myles Watson wrote:
I copied the reserved areas from i440bx_emulation. Like the
comments
say, I know these need to be reserved areas, but I'm not exactly
sure
where they belong.
Compile tested.
Signed-off-by: Myles Watson mylesgw@gmail.com
There is just a PIC in the Geode so you don't need to reserve the LAPIC range. You do still need to reserve the ROM range. Just fix up the comment.
Acked-by: Marc Jones marcj303@gmail.com
Rev 1111.
Still the same, sadly, cf attached.
I see the graphics device still getting setup even though it has no memory. I think that is might be a problem. This patch hides the header before resource allocation and should avoid the problem and is completely untested.....
It seems cleaner to hide the graphics when there's no memory allocated, but is the geodelx always paired with the cs5536? It's surprising to have them be that tightly coupled.
I think I'd prefer to hide the device in the cs5536 stage2_fixup. That way it can stay in cs5536, but get done in the right order. I would submit a patch, but while I was there I saw that there are other things (ide vs nand) that probably should be moved at the same time, and Mart's already working on that.
Thanks, Myles
On Thu, Jan 8, 2009 at 3:45 PM, Myles Watson mylesgw@gmail.com wrote:
-----Original Message----- From: Marc Jones [mailto:marcj303@gmail.com] Sent: Thursday, January 08, 2009 2:16 PM To: Myles Watson; Marc Jones; ron minnich; Coreboot Subject: Re: [coreboot] domain vs device statictree order
On Thu, Jan 8, 2009 at 1:17 PM, Ward Vandewege ward@gnu.org wrote:
On Thu, Jan 08, 2009 at 01:07:55PM -0700, Myles Watson wrote:
I copied the reserved areas from i440bx_emulation. Like the
comments
say, I know these need to be reserved areas, but I'm not exactly
sure
where they belong.
Compile tested.
Signed-off-by: Myles Watson mylesgw@gmail.com
There is just a PIC in the Geode so you don't need to reserve the LAPIC range. You do still need to reserve the ROM range. Just fix up the comment.
Acked-by: Marc Jones marcj303@gmail.com
Rev 1111.
Still the same, sadly, cf attached.
I see the graphics device still getting setup even though it has no memory. I think that is might be a problem. This patch hides the header before resource allocation and should avoid the problem and is completely untested.....
It seems cleaner to hide the graphics when there's no memory allocated, but is the geodelx always paired with the cs5536? It's surprising to have them be that tightly coupled.
I think I'd prefer to hide the device in the cs5536 stage2_fixup. That way it can stay in cs5536, but get done in the right order. I would submit a patch, but while I was there I saw that there are other things (ide vs nand) that probably should be moved at the same time, and Mart's already working on that.
Yes they are that closely attached and use the same SMM code which is how the device is hidden. I think that hiding the graphics device during graphics init is the right thing to do.
Marc
Ühel kenal päeval, N, 2009-01-08 kell 18:12, kirjutas Marc Jones:
On Thu, Jan 8, 2009 at 3:45 PM, Myles Watson mylesgw@gmail.com wrote:
-----Original Message----- From: Marc Jones [mailto:marcj303@gmail.com] Sent: Thursday, January 08, 2009 2:16 PM To: Myles Watson; Marc Jones; ron minnich; Coreboot Subject: Re: [coreboot] domain vs device statictree order
On Thu, Jan 8, 2009 at 1:17 PM, Ward Vandewege ward@gnu.org wrote:
On Thu, Jan 08, 2009 at 01:07:55PM -0700, Myles Watson wrote:
> I copied the reserved areas from i440bx_emulation. Like the
comments
> say, I know these need to be reserved areas, but I'm not exactly
sure
> where they belong. > > Compile tested. > > Signed-off-by: Myles Watson mylesgw@gmail.com
There is just a PIC in the Geode so you don't need to reserve the LAPIC range. You do still need to reserve the ROM range. Just fix up the comment.
Acked-by: Marc Jones marcj303@gmail.com
Rev 1111.
Still the same, sadly, cf attached.
I see the graphics device still getting setup even though it has no memory. I think that is might be a problem. This patch hides the header before resource allocation and should avoid the problem and is completely untested.....
It seems cleaner to hide the graphics when there's no memory allocated, but is the geodelx always paired with the cs5536? It's surprising to have them be that tightly coupled.
I think I'd prefer to hide the device in the cs5536 stage2_fixup. That way it can stay in cs5536, but get done in the right order. I would submit a patch, but while I was there I saw that there are other things (ide vs nand) that probably should be moved at the same time, and Mart's already working on that.
Yes they are that closely attached and use the same SMM code which is how the device is hidden. I think that hiding the graphics device during graphics init is the right thing to do.
We now have a graphics device in the device tree of mainboards. How about having a graphics device in northbridge/amd/geodelx or southbridge/amd/cs5536 too, and doing graphics init from there if the device exists in mainboard device tree? Or does that need to be run even on headless? Also, what if we had the possibility to run code for disabled devices - then the vpci hiding could cleanly happen when the device is disabled instead of checks from northbridge code. Like a .disable functor in device_ops?
Take it as a possible thought on how to make things more v3-ish if you will. The hiding on zero graphics memory patch is still good in the current state and should be committed for the time being either way imho.
Regards, Mart Raudsepp Artec Design LLC
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Mart Raudsepp
Also, what if we had the possibility to run code for disabled devices - then the vpci hiding could cleanly happen when the device is disabled instead of checks from northbridge code. Like a .disable functor in device_ops?
We do. If you run it in phase2 it runs independent of the enabled flag, and could use that to do any disabling that was needed. It's a little confusing to put the device in the tree then, though, because it will never show up in lspci.
Thanks, Myles
Ühel kenal päeval, R, 2009-01-09 kell 07:22, kirjutas Myles Watson:
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Mart Raudsepp
Also, what if we had the possibility to run code for disabled devices - then the vpci hiding could cleanly happen when the device is disabled instead of checks from northbridge code. Like a .disable functor in device_ops?
We do. If you run it in phase2 it runs independent of the enabled flag, and could use that to do any disabling that was needed. It's a little confusing to put the device in the tree then, though, because it will never show up in lspci.
I guess I mean a way to run disabling code if a device doesn't appear in the mainboard dts at all. E.g, there is a northbridge/amd/geodelx/graphics.dts and if the mainboard dts doesn't source it or it's disabled, a disabling function would be ran. This is just something that optionally wired built-in PCI devices might be interested in, not the typical case of PCI slots.
I can imagine it would get tricky to implement and quite likely it's just a bad idea.
Mart Raudsepp
On Thu, Jan 08, 2009 at 02:16:18PM -0700, Marc Jones wrote:
On Thu, Jan 8, 2009 at 1:17 PM, Ward Vandewege ward@gnu.org wrote:
On Thu, Jan 08, 2009 at 01:07:55PM -0700, Myles Watson wrote:
I copied the reserved areas from i440bx_emulation. Like the comments say, I know these need to be reserved areas, but I'm not exactly sure where they belong.
Compile tested.
Signed-off-by: Myles Watson mylesgw@gmail.com
There is just a PIC in the Geode so you don't need to reserve the LAPIC range. You do still need to reserve the ROM range. Just fix up the comment.
Acked-by: Marc Jones marcj303@gmail.com
Rev 1111.
Still the same, sadly, cf attached.
I see the graphics device still getting setup even though it has no memory. I think that is might be a problem. This patch hides the header before resource allocation and should avoid the problem and is completely untested.....
I tested with a few small (typo) changes, but it does not fix the boot. Log attached. Myles' patch does fix it though, and for what it's worth, it also boots with both patches applied.
Thanks, Ward.
On T, 2009-01-06 at 16:19 -0700, Myles Watson wrote:
On Tue, Jan 6, 2009 at 4:13 PM, Marc Jones marcj303@gmail.com wrote:
I agree it is parents before siblings but I thought it looked more like a problem with the statictree. I must not understand .next.
from http://pastebin.ca/1301081
dev_apic_0 has a .sibling = &dev_domain_0 and .next = &dev_domain_0_pci_1_0
Then later in the last device domain dev_domain_0_pci_f_2 has a .next = &dev_domain_0.
I would expect .sibling and .next to point to the &dev_domain_0 and then dev_domain_0 to have .next = &dev_domain_0_pci_1_0
Yes. I would too. It's that way because parents get defined after children. When the tree is being read, the full parent is not read (we don't get to the closing brace) until after we read all the children.
It seems the order code-wise is the expected one. Domain structure definitions appear before its devices, and so on, just the next pointers jump to the end for one of them in the pastebin example. Couldn't the .next pointers simply be pointing to the entry that is going to be printed out next as far as domain/device go?
"The rest of the geodelx fixup patch" looks quite invasive to me. Not regarding quality, but to my own local changes I had done :) Will take some digesting from the start to get things understood, merged with other changes and working order for me. But tomorrow is clean-up and submission time anyways. I will be adding a Flash device and juggle around some code to be done in appropriate phases for the interaction with VSA to be working as far as NAND Flash goes.
Mart Raudsepp
Ühel kenal päeval, T, 2009-01-06 kell 15:37, kirjutas Myles Watson:
Marc,
Hopefully this makes everything right again. I still think some of the geode functions should be moved, but that's really a separate issue.
You have to specify to make things breadth first, which seems like the correct way.
I guess it's not really breadth first. It's just parents before siblings.
I think this should be done for all the phases unless there's some compelling reason not to.
I don't think having different phases run in different order is a good idea, as it can lead to unexpected code execution orders all over again.
Hopefully we can finalize this soon, so I can finalize my month long on-and-off NAND flash battles :)
Signed-off-by: Myles Watson mylesgw@gmail.com
Code-wise this looks good if all phases act the same way. I'm not qualified right now to coment about the correctness of the change for the order and how this affects existing mainboard code and their code execution order expectations.
Mart Raudsepp
I guess it's not really breadth first. It's just parents before siblings.
I think this should be done for all the phases unless there's some compelling reason not to.
I don't think having different phases run in different order is a good idea, as it can lead to unexpected code execution orders all over again.
I agree.
Code-wise this looks good if all phases act the same way.
Without this patch Phase1, Phase2, and Phase6 are done in statictree order, and Phase3, Phase4, and Phase5 are done Parents before children.
I don't see Phase1 being used anywhere. Phase2 is handled with the patch. I think Phase6 should be updated, and I can send a patch.
Ron?
Thanks, Myles
On Wed, Jan 7, 2009 at 9:35 AM, Myles Watson mylesgw@gmail.com wrote:
I guess it's not really breadth first. It's just parents before siblings.
I think this should be done for all the phases unless there's some compelling reason not to.
I don't think having different phases run in different order is a good idea, as it can lead to unexpected code execution orders all over again.
I agree.
Code-wise this looks good if all phases act the same way.
Without this patch Phase1, Phase2, and Phase6 are done in statictree order, and Phase3, Phase4, and Phase5 are done Parents before children.
I don't see Phase1 being used anywhere. Phase2 is handled with the patch. I think Phase6 should be updated, and I can send a patch.
Ron?
I think all the phases should be in parents before children order, ie the same order that the devices would get scan. That seems to be the most intuitive and should be consistent.
Marc
Myles Watson wrote:
I guess it's not really breadth first. It's just parents before siblings.
Will our scheme with 6 phases all processing devices in one and the same way fit all hardware?
Maybe the bus scan order needs to be settable, maybe by Kconfig, maybe even at runtime?
I'll try to clarify:
Geode and Mart needs parents before siblings for phase2 to have VSA init before the NAND enable, VSA is in domain.p2 and NAND in ide.p2.
I can imagine that one algorithm for deciding the order of processing all devices in the system will not be enough however.
We'll see more dynamic hardware, which will require us to be better at running code in the right order.
HT speed reconfiguration may be another example, or does it run already before any of the phases? Does that also require parents before siblings, or is something else (breadth first?) more appropriate there?
If we can actually get by with just one simple algorithm that's great of course. Can we?
I don't think having different phases run in different order is a good idea, as it can lead to unexpected code execution orders all over again.
I agree.
Yes, me too, in the sense that whoever is filling out phases will need code for different devices to run in some particular order, and instead of trying to guess and document what we think is sufficient, it would be nice to have infrastructure that allows each phase filler to specify the order they need in a very easy way.
Without this patch Phase1, Phase2, and Phase6 are done in statictree order, and Phase3, Phase4, and Phase5 are done Parents before children.
The merit of statictree order is that it's the only thing available before the dynamic bus scans in phase3, or?
I don't see Phase1 being used anywhere. Phase2 is handled with the patch. I think Phase6 should be updated, and I can send a patch.
--8<-- doc/design/newboot.lyx Phase 1 -- very early setup This phase existed to make printk work and has been obsoleted. The simple traversal (forall devices) is used for this phase.
Phase 2 -- fixup. Fix broken devices if needed. Typically used by mainboard device.
These are functions that are required before any PCI operations of any kind are run. The simple traversal (forall devices) is used for this phase. -->8--
I suggest moving VSA init to domain.phase1. It's certainly very early setup because it _creates_ the PCI devices that are to be fixed up in phase2.
Another option might be to put the NAND init in phase3.
//Peter
On Wed, Jan 7, 2009 at 10:29 AM, Peter Stuge peter@stuge.se wrote:
Myles Watson wrote:
I guess it's not really breadth first. It's just parents before siblings.
Will our scheme with 6 phases all processing devices in one and the same way fit all hardware?
I can't think of a time when a child expects to be initialized (whatever phase) before its parent.
Maybe the bus scan order needs to be settable, maybe by Kconfig, maybe even at runtime?
I'll try to clarify:
Geode and Mart needs parents before siblings for phase2 to have VSA init before the NAND enable, VSA is in domain.p2 and NAND in ide.p2.
I can imagine that one algorithm for deciding the order of processing all devices in the system will not be enough however.
We'll see more dynamic hardware, which will require us to be better at running code in the right order.
Those porting the code can choose what phase things need to go in if it is clear what order things are called. If that's not clear it seems very hard to write the code.
HT speed reconfiguration may be another example, or does it run already before any of the phases?
It runs in stage 1.
Does that also require parents before siblings, or is something else (breadth first?) more appropriate there?
If we can actually get by with just one simple algorithm that's great of course. Can we?
I think so. It seems like the only downside is that for_all_devices was simpler than the tree traversal. I guess it also reaches devices which weren't found in phase3, but it seems like if phase3 can't find them you shouldn't do phase4-6.
I don't think having different phases run in different order is a good idea, as it can lead to unexpected code execution orders all over again.
I agree.
Yes, me too, in the sense that whoever is filling out phases will need code for different devices to run in some particular order, and instead of trying to guess and document what we think is sufficient, it would be nice to have infrastructure that allows each phase filler to specify the order they need in a very easy way.
Without this patch Phase1, Phase2, and Phase6 are done in statictree order, and Phase3, Phase4, and Phase5 are done Parents before children.
The merit of statictree order is that it's the only thing available before the dynamic bus scans in phase3, or?
There are always two orders available. When stage2 starts we have the device tree, which contains children, sibling, and next pointers. I'm not sure we ever care to use the next pointers. Dynamic devices are not available until after phase3, no matter which way you traverse the devices.
I don't see Phase1 being used anywhere. Phase2 is handled with the patch. I think Phase6 should be updated, and I can send a patch.
--8<-- doc/design/newboot.lyx Phase 1 -- very early setup This phase existed to make printk work and has been obsoleted. The simple traversal (forall devices) is used for this phase.
Phase 2 -- fixup. Fix broken devices if needed. Typically used by mainboard device.
These are functions that are required before any PCI operations of any kind are run. The simple traversal (forall devices) is used for this phase. -->8--
I suggest moving VSA init to domain.phase1. It's certainly very early setup because it _creates_ the PCI devices that are to be fixed up in phase2.
If phase1 is really obsolete, then phase2 is as early as you get. Do we need to reinstate phase1?
Thanks, Myles
On Wed, Jan 7, 2009 at 10:42 AM, Myles Watson mylesgw@gmail.com wrote:
On Wed, Jan 7, 2009 at 10:29 AM, Peter Stuge peter@stuge.se wrote:
I suggest moving VSA init to domain.phase1. It's certainly very early setup because it _creates_ the PCI devices that are to be fixed up in phase2.
If phase1 is really obsolete, then phase2 is as early as you get. Do we need to reinstate phase1?
Being the top level of the Geode PCI domain, Phase2_fixup for the domain in parent to child order is correct place to do VSA init. Phase2_fixup for the cs5536 southbridge should make the NAND/IDE switch before resource allocation in phase 3-5.
I think that you want phase6 to walk the tree in case there is something that needs fixup after allocation especially if it was hidden from allocation.
Marc
On Wed, Jan 7, 2009 at 8:35 AM, Myles Watson mylesgw@gmail.com wrote:
I guess it's not really breadth first. It's just parents before siblings.
I think this should be done for all the phases unless there's some compelling reason not to.
I don't think having different phases run in different order is a good idea, as it can lead to unexpected code execution orders all over again.
I agree.
Code-wise this looks good if all phases act the same way.
Without this patch Phase1, Phase2, and Phase6 are done in statictree order, and Phase3, Phase4, and Phase5 are done Parents before children.
I don't see Phase1 being used anywhere. Phase2 is handled with the patch. I think Phase6 should be updated, and I can send a patch.
Ron?
I am still sort of out but you guys are on the right approach, do what you think best. I trust you ...
ron
On Tue, Jan 6, 2009 at 3:37 PM, Myles Watson mylesgw@gmail.com wrote:
Marc,
Hopefully this makes everything right again. I still think some of the geode functions should be moved, but that's really a separate issue.
You have to specify to make things breadth first, which seems like the correct way.
I guess it's not really breadth first. It's just parents before siblings.
I guess the more correct terminology is preorder. Take care of the root before siblings or children.
I think this should be done for all the phases unless there's some compelling reason not to.
This patch implements preorder traversal for Phase 2 and Phase 6, and prints a warning that Phase 1 is obsolete if anyone implements one.
The only difference between the Phase 2 and Phase 6 implementations is that phase2_fixup is always called if it's defined, but phase6_init is only called if the device is enabled.
Note that any devices not found in the tree will not have their init functions called with this patch. I think that's a good thing, but it will require some dts fixes.
Boot tested on Serengeti and qemu.
Signed-off-by: Myles Watson mylesgw@gmail.com
Thanks, Myles
Myles Watson wrote:
This patch implements preorder traversal for Phase 2 and Phase 6, and prints a warning that Phase 1 is obsolete if anyone implements one.
The only difference between the Phase 2 and Phase 6 implementations is that phase2_fixup is always called if it's defined, but phase6_init is only called if the device is enabled.
Note that any devices not found in the tree will not have their init functions called with this patch. I think that's a good thing, but it will require some dts fixes.
Boot tested on Serengeti and qemu.
Signed-off-by: Myles Watson mylesgw@gmail.com
Acked-by: Peter Stuge peter@stuge.se
Ühel kenal päeval, N, 2009-01-08 kell 02:11, kirjutas Peter Stuge:
Myles Watson wrote:
This patch implements preorder traversal for Phase 2 and Phase 6, and prints a warning that Phase 1 is obsolete if anyone implements one.
The only difference between the Phase 2 and Phase 6 implementations is that phase2_fixup is always called if it's defined, but phase6_init is only called if the device is enabled.
Note that any devices not found in the tree will not have their init functions called with this patch. I think that's a good thing, but it will require some dts fixes.
What's the easy way to find out which devices could be missing from the tree?
Boot tested on Serengeti and qemu.
Signed-off-by: Myles Watson mylesgw@gmail.com
Acked-by: Peter Stuge peter@stuge.se
This patch is necessary to get GeodeLX to boot again (DBE61). rev1103 had broken it with the memory controller device separation being committed before, as stuff appears to not be set up right and it resets in the end parts of stage2. Peter suggested the memory controller isn't set up right, and something like that appears to be the case then, as this patch here fixed it up again with the execution order being changed.
Mart Raudsepp
On Thu, Jan 8, 2009 at 8:29 AM, Mart Raudsepp mart.raudsepp@artecdesign.ee wrote:
Ühel kenal päeval, N, 2009-01-08 kell 02:11, kirjutas Peter Stuge:
Myles Watson wrote:
This patch implements preorder traversal for Phase 2 and Phase 6, and prints a warning that Phase 1 is obsolete if anyone implements one.
The only difference between the Phase 2 and Phase 6 implementations is that phase2_fixup is always called if it's defined, but phase6_init is only called if the device is enabled.
Note that any devices not found in the tree will not have their init functions called with this patch. I think that's a good thing, but it will require some dts fixes.
What's the easy way to find out which devices could be missing from the tree?
If you look at the device tree that's printed "after phase 3", any device that is dynamic wasn't found in the device tree. PCI cards that you plug in will never be in the dts, so they will always be dynamic.
Boot tested on Serengeti and qemu.
Signed-off-by: Myles Watson mylesgw@gmail.com
Acked-by: Peter Stuge peter@stuge.se
This patch is necessary to get GeodeLX to boot again (DBE61). rev1103 had broken it with the memory controller device separation being committed before, as stuff appears to not be set up right and it resets in the end parts of stage2. Peter suggested the memory controller isn't set up right, and something like that appears to be the case then, as this patch here fixed it up again with the execution order being changed.
I'm glad it fixes it. Thanks for testing.
Myles
On Wed, Jan 7, 2009 at 6:11 PM, Peter Stuge peter@stuge.se wrote:
Myles Watson wrote:
This patch implements preorder traversal for Phase 2 and Phase 6, and prints a warning that Phase 1 is obsolete if anyone implements one.
The only difference between the Phase 2 and Phase 6 implementations is that phase2_fixup is always called if it's defined, but phase6_init is only called if the device is enabled.
Note that any devices not found in the tree will not have their init functions called with this patch. I think that's a good thing, but it will require some dts fixes.
Boot tested on Serengeti and qemu.
Signed-off-by: Myles Watson mylesgw@gmail.com
Acked-by: Peter Stuge peter@stuge.se
Rev 1108.
Thanks, Myles
-
Marc Jones -
Mart Raudsepp -
Myles Watson -
Peter Stuge -
ron minnich -
Ward Vandewege