Re: [m5-dev] Review Request: ARM: Add support for a dumb IDE controller
As feared, this change does break x86 boot. I'll see if I can fix it. Ironically I found this out while adding an X86 FS regression. We really do need to figure out what's going on with those values and how that maps to other ISAs. Gabe Gabe Black wrote: On 2010-11-08 17:56:14, Nathan Binkert wrote: src/dev/ide_ctrl.cc, line 454 http://reviews.m5sim.org/r/292/diff/1/?file=5058#file5058line454 Is this something that we should deal with on a per device basis, or is this a more generic thing? Also, is this something that should be configured by the user, or is this something that's either fixed or gleaned from the OS? Ali Saidi wrote: It seems rather arbitrary, but the world of function pointers that sets this value in the OS is pretty deep. I think having the user configure it is fine, there isn't a really good way we could grab it from the OS since there isn't once place where there is a device struct that describes a ide controller. These values only seem to apply to the IDE device. I'm also really curious what's going on with these values. We should probably figure out what they're for before we go hacking them in. Also, please make sure an X86 kernel still boots with this change. I remember getting the IDE controller to work was a little finicky at least partially because of its legacy fixed IO port locations, and it might break even though other ISAs are ok. - Gabe --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/292/#review436 --- On 2010-11-08 15:34:45, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/292/ --- (Updated 2010-11-08 15:34:45) Review request for Default. Summary --- ARM: Add support for a dumb IDE controller Diffs - configs/common/FSConfig.py f61e079ad05e src/dev/Ide.py f61e079ad05e src/dev/arm/realview.cc f61e079ad05e src/dev/ide_ctrl.hh f61e079ad05e src/dev/ide_ctrl.cc f61e079ad05e src/dev/pcidev.cc f61e079ad05e Diff: http://reviews.m5sim.org/r/292/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: ARM: Add support for a dumb IDE controller
It's not an isa thing, it's hard-coded in the struct that is passed to the ide driver. Either way, it-s not those values that is breaking it. They do nothing when they're 0. Ali On Nov 21, 2010, at 11:58 PM, Gabe Black wrote: As feared, this change does break x86 boot. I'll see if I can fix it. Ironically I found this out while adding an X86 FS regression. We really do need to figure out what's going on with those values and how that maps to other ISAs. Gabe Gabe Black wrote: On 2010-11-08 17:56:14, Nathan Binkert wrote: src/dev/ide_ctrl.cc, line 454 http://reviews.m5sim.org/r/292/diff/1/?file=5058#file5058line454 Is this something that we should deal with on a per device basis, or is this a more generic thing? Also, is this something that should be configured by the user, or is this something that's either fixed or gleaned from the OS? Ali Saidi wrote: It seems rather arbitrary, but the world of function pointers that sets this value in the OS is pretty deep. I think having the user configure it is fine, there isn't a really good way we could grab it from the OS since there isn't once place where there is a device struct that describes a ide controller. These values only seem to apply to the IDE device. I'm also really curious what's going on with these values. We should probably figure out what they're for before we go hacking them in. Also, please make sure an X86 kernel still boots with this change. I remember getting the IDE controller to work was a little finicky at least partially because of its legacy fixed IO port locations, and it might break even though other ISAs are ok. - Gabe --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/292/#review436 --- On 2010-11-08 15:34:45, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/292/ --- (Updated 2010-11-08 15:34:45) Review request for Default. Summary --- ARM: Add support for a dumb IDE controller Diffs - configs/common/FSConfig.py f61e079ad05e src/dev/Ide.py f61e079ad05e src/dev/arm/realview.cc f61e079ad05e src/dev/ide_ctrl.hh f61e079ad05e src/dev/ide_ctrl.cc f61e079ad05e src/dev/pcidev.cc f61e079ad05e Diff: http://reviews.m5sim.org/r/292/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: ARM: Add support for a dumb IDE controller
Yeah, I figured it out, and a fix will be coming soon. If we're keeping score I'm sure I've accidentally broken more things than you, so no sweat. There will be a regression soon so this will be less likely to happen again. Gabe Ali Saidi wrote: It's not an isa thing, it's hard-coded in the struct that is passed to the ide driver. Either way, it-s not those values that is breaking it. They do nothing when they're 0. Ali On Nov 21, 2010, at 11:58 PM, Gabe Black wrote: As feared, this change does break x86 boot. I'll see if I can fix it. Ironically I found this out while adding an X86 FS regression. We really do need to figure out what's going on with those values and how that maps to other ISAs. Gabe Gabe Black wrote: On 2010-11-08 17:56:14, Nathan Binkert wrote: src/dev/ide_ctrl.cc, line 454 http://reviews.m5sim.org/r/292/diff/1/?file=5058#file5058line454 Is this something that we should deal with on a per device basis, or is this a more generic thing? Also, is this something that should be configured by the user, or is this something that's either fixed or gleaned from the OS? Ali Saidi wrote: It seems rather arbitrary, but the world of function pointers that sets this value in the OS is pretty deep. I think having the user configure it is fine, there isn't a really good way we could grab it from the OS since there isn't once place where there is a device struct that describes a ide controller. These values only seem to apply to the IDE device. I'm also really curious what's going on with these values. We should probably figure out what they're for before we go hacking them in. Also, please make sure an X86 kernel still boots with this change. I remember getting the IDE controller to work was a little finicky at least partially because of its legacy fixed IO port locations, and it might break even though other ISAs are ok. - Gabe --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/292/#review436 --- On 2010-11-08 15:34:45, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/292/ --- (Updated 2010-11-08 15:34:45) Review request for Default. Summary --- ARM: Add support for a dumb IDE controller Diffs - configs/common/FSConfig.py f61e079ad05e src/dev/Ide.py f61e079ad05e src/dev/arm/realview.cc f61e079ad05e src/dev/ide_ctrl.hh f61e079ad05e src/dev/ide_ctrl.cc f61e079ad05e src/dev/pcidev.cc f61e079ad05e Diff: http://reviews.m5sim.org/r/292/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: ARM: Add support for a dumb IDE controller
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/292/#review441 --- Ship it! - Nathan On 2010-11-08 15:34:45, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/292/ --- (Updated 2010-11-08 15:34:45) Review request for Default. Summary --- ARM: Add support for a dumb IDE controller Diffs - configs/common/FSConfig.py f61e079ad05e src/dev/Ide.py f61e079ad05e src/dev/arm/realview.cc f61e079ad05e src/dev/ide_ctrl.hh f61e079ad05e src/dev/ide_ctrl.cc f61e079ad05e src/dev/pcidev.cc f61e079ad05e Diff: http://reviews.m5sim.org/r/292/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
[m5-dev] Review Request: ARM: Add support for a dumb IDE controller
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/292/ --- Review request for Default. Summary --- ARM: Add support for a dumb IDE controller Diffs - configs/common/FSConfig.py f61e079ad05e src/dev/Ide.py f61e079ad05e src/dev/arm/realview.cc f61e079ad05e src/dev/ide_ctrl.hh f61e079ad05e src/dev/ide_ctrl.cc f61e079ad05e src/dev/pcidev.cc f61e079ad05e Diff: http://reviews.m5sim.org/r/292/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: ARM: Add support for a dumb IDE controller
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/292/#review436 --- src/dev/ide_ctrl.cc http://reviews.m5sim.org/r/292/#comment646 Is this something that we should deal with on a per device basis, or is this a more generic thing? Also, is this something that should be configured by the user, or is this something that's either fixed or gleaned from the OS? - Nathan On 2010-11-08 15:34:45, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/292/ --- (Updated 2010-11-08 15:34:45) Review request for Default. Summary --- ARM: Add support for a dumb IDE controller Diffs - configs/common/FSConfig.py f61e079ad05e src/dev/Ide.py f61e079ad05e src/dev/arm/realview.cc f61e079ad05e src/dev/ide_ctrl.hh f61e079ad05e src/dev/ide_ctrl.cc f61e079ad05e src/dev/pcidev.cc f61e079ad05e Diff: http://reviews.m5sim.org/r/292/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: ARM: Add support for a dumb IDE controller
On 2010-11-08 17:56:14, Nathan Binkert wrote: src/dev/ide_ctrl.cc, line 454 http://reviews.m5sim.org/r/292/diff/1/?file=5058#file5058line454 Is this something that we should deal with on a per device basis, or is this a more generic thing? Also, is this something that should be configured by the user, or is this something that's either fixed or gleaned from the OS? It seems rather arbitrary, but the world of function pointers that sets this value in the OS is pretty deep. I think having the user configure it is fine, there isn't a really good way we could grab it from the OS since there isn't once place where there is a device struct that describes a ide controller. These values only seem to apply to the IDE device. - Ali --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/292/#review436 --- On 2010-11-08 15:34:45, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/292/ --- (Updated 2010-11-08 15:34:45) Review request for Default. Summary --- ARM: Add support for a dumb IDE controller Diffs - configs/common/FSConfig.py f61e079ad05e src/dev/Ide.py f61e079ad05e src/dev/arm/realview.cc f61e079ad05e src/dev/ide_ctrl.hh f61e079ad05e src/dev/ide_ctrl.cc f61e079ad05e src/dev/pcidev.cc f61e079ad05e Diff: http://reviews.m5sim.org/r/292/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
