I know some people will get upset by the find_pci_device, but: This patch fixes a long-standing problem in the cs5536 driver, that is probably also in v2.
The ide_init is called with the sb device but needs the IDE device, which is different. It also did a write_config8,not write_config32; this bug has been in there since the code was written for v2. I don't know how or why it ever worked, since PWB is 1<<14.
Works fine with 2.6.24. 2.6.15 hangs on boot, but I don't think it was ever IDE. POST on the 2.6.25 boot hang is 00.
Signed-off-by: Ronald G. Minnich rminnich@gmail.com
On 06.05.2008 07:54, ron minnich wrote:
I know some people will get upset by the find_pci_device, but:
Indeed. Since we don't use the struct device passed in to ide_init, why don't we give Geode IDE its own struct device_operations and store the IDE config there?
This patch fixes a long-standing problem in the cs5536 driver, that is probably also in v2.
The ide_init is called with the sb device but needs the IDE device, which is different. It also did a write_config8,not write_config32; this bug has been in there since the code was written for v2. I don't know how or why it ever worked, since PWB is 1<<14.
Works fine with 2.6.24. 2.6.15 hangs on boot, but I don't think it was
^^^^^^ 2.6.25?
ever IDE. POST on the 2.6.25 boot hang is 00.
I have problems parsing the sentence above: "... I don't think it was ever IDE."
Signed-off-by: Ronald G. Minnich rminnich@gmail.com
Index: southbridge/amd/cs5536/cs5536.c
--- southbridge/amd/cs5536/cs5536.c (revision 676) +++ southbridge/amd/cs5536/cs5536.c (working copy) @@ -589,15 +589,18 @@ static void ide_init(struct device *dev) { u32 ide_cfg;
struct device *ide_dev;
printk(BIOS_DEBUG, "cs5536_ide: %s\n", __func__); /* GPIO and IRQ setup are handled in the main chipset code. */
ide_dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
PCI_DEVICE_ID_AMD_CS5536_B0_IDE, 0);// Enable the channel and Post Write Buffer // NOTE: Only 32-bit writes to the data buffer are allowed when PWB is set
- ide_cfg = pci_read_config32(dev, IDE_CFG);
- ide_cfg = pci_read_config32(ide_dev, IDE_CFG); ide_cfg |= CHANEN | PWB;
- pci_write_config8(dev, IDE_CFG, ide_cfg);
- pci_write_config32(ide_dev, IDE_CFG, ide_cfg);
}
You know my preference for killing dev_find_pci_device, but that can come later if it is too difficult.
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Regards, Carl-Daniel
On Tue, May 6, 2008 at 5:15 AM, Carl-Daniel Hailfinger
Indeed. Since we don't use the struct device passed in to ide_init, why don't we give Geode IDE its own struct device_operations and store the IDE config there?
That's a great question. The reason is that the cs5536 is one chip. On v2, we treated it as more than one. For my goal of easing non-experts into the code, I decided to try making it one chip and one dts. This is an experiment that may, in the end, prove wrong.
I still think one dts per chip is the right way to go, but do we need more than one level of dts? I really think this all should be in one file, it got much simpler that way.
I have problems parsing the sentence above: "... I don't think it was ever IDE."
I should not write when that tired.
Let's try again.
Works on 2.6.24. hangs on 2.6.25, I thought for a second it was NOT ide, but it is clear that 2.6.25 is hanging on probing ide1 -- or it sure looks that way. And, again, the post code of 00 is weird; I keep thinking there are vsa issues here. It just doesn't act like Linux hangs I see on other boards.
You know my preference for killing dev_find_pci_device, but that can come later if it is too difficult.
I have no disagreement as long as, in so doing, we reduce code overall and increase comprehensability.
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
many thanks!
ron
On 06.05.2008 17:37, ron minnich wrote:
On Tue, May 6, 2008 at 5:15 AM, Carl-Daniel Hailfinger
Indeed. Since we don't use the struct device passed in to ide_init, why don't we give Geode IDE its own struct device_operations and store the IDE config there?
That's a great question. The reason is that the cs5536 is one chip. On v2, we treated it as more than one. For my goal of easing non-experts into the code, I decided to try making it one chip and one dts. This is an experiment that may, in the end, prove wrong.
I still think one dts per chip is the right way to go, but do we need more than one level of dts? I really think this all should be in one file, it got much simpler that way.
My dream is to have templates for such chips so we can refer to the template unless we need some very special handling. That would mean multi-level includes in the dts AFAICS, but I'm not totally happy about that.
I have problems parsing the sentence above: "... I don't think it was ever IDE."
I should not write when that tired.
Let's try again.
Works on 2.6.24. hangs on 2.6.25, I thought for a second it was NOT ide, but it is clear that 2.6.25 is hanging on probing ide1 -- or it sure looks that way. And, again, the post code of 00 is weird; I keep thinking there are vsa issues here. It just doesn't act like Linux hangs I see on other boards.
The phenomenon is weird, the text is well-written.
You know my preference for killing dev_find_pci_device, but that can come later if it is too difficult.
I have no disagreement as long as, in so doing, we reduce code overall and increase comprehensability.
See below for my take at this.
Move CS5536 IDE configuration into a separate dts and its own PCI device.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Build-tested on db800, norwich, dbe62, alix.1c, alix.2c3. Breaks build for dbe61.
Index: LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/ide =================================================================== --- LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/ide (revision 0) +++ LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/ide (revision 0) @@ -0,0 +1,26 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2007 Ronald G. Minnich rminnich@gmail.com + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ + +{ + device_operations = "cs5536_ide"; + + /* IDE: enable CS5536 IDE. There may be a different IDE controller on board */ + enable_ide = "0"; +}; Index: LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/cs5536.c =================================================================== --- LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/cs5536.c (revision 676) +++ LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/cs5536.c (working copy) @@ -590,6 +590,11 @@ { u32 ide_cfg;
+ struct southbridge_amd_cs5536_ide_config *ide = + (struct southbridge_amd_cs5536_ide_config *)dev->device_configuration; + if (!ide->enable_ide) + return; + printk(BIOS_DEBUG, "cs5536_ide: %s\n", __func__); /* GPIO and IRQ setup are handled in the main chipset code. */
@@ -654,9 +659,6 @@ hide_vpci(sb->unwanted_vpci[i]); }
- if (sb->enable_ide) - ide_init(dev); - cs5536_setup_power_button(sb);
printk(BIOS_SPEW, "cs5536: %s() Exit\n", __FUNCTION__); @@ -688,3 +690,17 @@ .phase6_init = southbridge_init, };
+struct device_operations cs5536_ide = { + .id = {.type = DEVICE_ID_PCI, + .u = {.pci = {.vendor = PCI_VENDOR_ID_AMD, + .device = PCI_DEVICE_ID_AMD_CS5536_B0_IDE}}}, + .constructor = default_device_constructor, +#warning FIXME: what has to go in phase3_scan? + .phase3_scan = 0, + .phase4_read_resources = pci_dev_read_resources, + .phase4_set_resources = pci_dev_set_resources, + .phase5_enable_resources = pci_dev_enable_resources, + .phase6_init = ide_init, + .ops_pci = &pci_dev_ops_pci, +}; + Index: LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/dts =================================================================== --- LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/dts (revision 676) +++ LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/dts (working copy) @@ -36,9 +36,6 @@ /* 0:IDE 1:FLASH, if you are using NAND flash instead of IDE drive. */ enable_ide_nand_flash = "0";
- /* IDE: enable CS5536 IDE. There may be a different IDE controller on board */ - enable_ide = "0"; - /* Enable USB Port 4 (0:host 1:device). */ enable_USBP4_device = "0";
Index: LinuxBIOSv3-hierarchical_dts/mainboard/amd/norwich/dts =================================================================== --- LinuxBIOSv3-hierarchical_dts/mainboard/amd/norwich/dts (revision 676) +++ LinuxBIOSv3-hierarchical_dts/mainboard/amd/norwich/dts (working copy) @@ -34,7 +34,6 @@ }; pci@15,0 { /config/("southbridge/amd/cs5536/dts"); - enable_ide = "1"; /* Interrupt enables for LPC bus. * Each bit is an IRQ 0-15. */ lpc_serirq_enable = "0x00001002"; @@ -50,5 +49,9 @@ com1_address = "0x3f8"; com1_irq = "4"; }; + pci@15,2 { + /config/("southbridge/amd/cs5536/ide"); + enable_ide = "1"; + }; }; }; Index: LinuxBIOSv3-hierarchical_dts/mainboard/amd/db800/dts =================================================================== --- LinuxBIOSv3-hierarchical_dts/mainboard/amd/db800/dts (revision 676) +++ LinuxBIOSv3-hierarchical_dts/mainboard/amd/db800/dts (working copy) @@ -34,7 +34,6 @@ }; pci@15,0 { /config/("southbridge/amd/cs5536/dts"); - enable_ide = "1"; /* Interrupt enables for LPC bus. * Each bit is an IRQ 0-15. */ lpc_serirq_enable = "0x000010da"; @@ -47,6 +46,10 @@ enable_gpio_int_route = "0x0D0C0700"; enable_USBP4_device = "1"; }; + pci@15,2 { + /config/("southbridge/amd/cs5536/ide"); + enable_ide = "1"; + }; ioport@46 { /config/("superio/winbond/w83627hf/dts"); com1enable = "1"; Index: LinuxBIOSv3-hierarchical_dts/mainboard/artecgroup/dbe62/dts =================================================================== --- LinuxBIOSv3-hierarchical_dts/mainboard/artecgroup/dbe62/dts (revision 676) +++ LinuxBIOSv3-hierarchical_dts/mainboard/artecgroup/dbe62/dts (working copy) @@ -34,7 +34,6 @@ }; pci@15,0 { /config/("southbridge/amd/cs5536/dts"); - enable_ide = "1"; /* Interrupt enables for LPC bus. * Each bit is an IRQ 0-15. */ lpc_serirq_enable = "0x00001002"; @@ -54,5 +53,9 @@ /* Set com2 IRQ to be what is usually COM1 */ com2_irq = "4"; }; + pci@15,2 { + /config/("southbridge/amd/cs5536/ide"); + enable_ide = "1"; + }; }; }; Index: LinuxBIOSv3-hierarchical_dts/mainboard/pcengines/alix1c/dts =================================================================== --- LinuxBIOSv3-hierarchical_dts/mainboard/pcengines/alix1c/dts (revision 676) +++ LinuxBIOSv3-hierarchical_dts/mainboard/pcengines/alix1c/dts (working copy) @@ -34,7 +34,6 @@ }; pci@15,0 { /config/("southbridge/amd/cs5536/dts"); - enable_ide = "1"; /* Interrupt enables for LPC bus. * Each bit is an IRQ 0-15. */ lpc_serirq_enable = "0x000010da"; @@ -46,6 +45,10 @@ * See virtual PIC spec. */ enable_gpio_int_route = "0x0D0C0700"; }; + pci@15,2 { + /config/("southbridge/amd/cs5536/ide"); + enable_ide = "1"; + }; ioport@46 { /config/("superio/winbond/w83627hf/dts"); com1enable = "1"; Index: LinuxBIOSv3-hierarchical_dts/mainboard/pcengines/alix2c3/dts =================================================================== --- LinuxBIOSv3-hierarchical_dts/mainboard/pcengines/alix2c3/dts (revision 676) +++ LinuxBIOSv3-hierarchical_dts/mainboard/pcengines/alix2c3/dts (working copy) @@ -32,7 +32,6 @@ }; pci@15,0 { /config/("southbridge/amd/cs5536/dts"); - enable_ide = "1"; /* Interrupt enables for LPC bus. * Each bit is an IRQ 0-15. */ lpc_serirq_enable = "0x000010da"; @@ -50,5 +49,9 @@ /* this board does not really have vga; disable it (pci device 00:01.1) */ unwanted_vpci = < 80000900 0 >; }; + pci@15,2 { + /config/("southbridge/amd/cs5536/ide"); + enable_ide = "1"; + }; }; };
On Wed, May 07, 2008 at 02:42:24AM +0200, Carl-Daniel Hailfinger wrote:
+++ LinuxBIOSv3-hierarchical_dts/mainboard/amd/norwich/dts (working copy) @@ -34,7 +34,6 @@ }; pci@15,0 { /config/("southbridge/amd/cs5536/dts");
enable_ide = "1"; /* Interrupt enables for LPC bus. * Each bit is an IRQ 0-15. */ lpc_serirq_enable = "0x00001002";@@ -50,5 +49,9 @@ com1_address = "0x3f8"; com1_irq = "4"; };
pci@15,2 {/config/("southbridge/amd/cs5536/ide");enable_ide = "1";};};
This makes more sense, but..
What was the reason for introducing multiple dts files for a single chip again?
Only the C struct name limitations?
*scratches forehead* I kind of want the single dts per chip back..
//Peter
On Tue, May 6, 2008 at 6:55 PM, Peter Stuge peter@stuge.se wrote:
This makes more sense, but..
What was the reason for introducing multiple dts files for a single chip again?
Only the C struct name limitations?
*scratches forehead* I kind of want the single dts per chip back..
yep, me too. that's a concern. Also, I worry about people having to deal with the nasty little thicket of tons of little file, all (mostly) alike.
And we've got a vendor/devid. At what point are we asking people to describe the bus/dev/fn too much?
ron
On Tue, May 06, 2008 at 08:50:46PM -0700, ron minnich wrote:
On Tue, May 6, 2008 at 6:55 PM, Peter Stuge peter@stuge.se wrote:
This makes more sense, but..
What was the reason for introducing multiple dts files for a single chip again?
Only the C struct name limitations?
*scratches forehead* I kind of want the single dts per chip back..
yep, me too. that's a concern. Also, I worry about people having to deal with the nasty little thicket of tons of little file, all (mostly) alike.
Full ACK. One dts per chip, one per board. Many small files is not the way to go, we worked hard in the past to reduce this cluttering, and rightly so.
Uwe.
one last comment: Stepan once received a complaint that linuxbios was "too greppy". Is this patch pushing us back that direction? This is a question. I don't know.
ron
On Tue, May 06, 2008 at 08:50:46PM -0700, ron minnich wrote:
What was the reason for introducing multiple dts files for a single chip again?
Only the C struct name limitations?
*scratches forehead* I kind of want the single dts per chip back..
yep, me too. that's a concern.
What needs to change so we can just combine the split out dts files into one?
Also, I worry about people having to deal with the nasty little thicket of tons of little file, all (mostly) alike.
Yes, there is no point to that.
And we've got a vendor/devid. At what point are we asking people to describe the bus/dev/fn too much?
I don't think this should be a concern.
The device tree is based on physical components and I think that is the only clean design since that's how the hardware we try to abstract is constructed.
I understand your worry, but keep in mind that "people" should only have to describe the hardware once, when the chip dts is created.
On Tue, May 06, 2008 at 08:53:51PM -0700, ron minnich wrote:
"too greppy". Is this patch pushing us back that direction?
In the sense that more and more files now have less and less data bits, yes definately.
I would love to solve this issue by moving (back?) to composite devices in dts files.
Yes, this means a little more "magic" in dtc, but hopefully we can still keep a simple (1:1?) device:struct relationship.
//Peter
Ühel kenal päeval, K, 2008-05-07 kell 02:42, kirjutas Carl-Daniel Hailfinger:
See below for my take at this.
Move CS5536 IDE configuration into a separate dts and its own PCI device.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Build-tested on db800, norwich, dbe62, alix.1c, alix.2c3. Breaks build for dbe61.
I think it's fine that it breaks build for dbe61 and I think that dbe61 code should be brought up to mostly the state that dbe62 is in anyway.
Index: LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/dts
--- LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/dts (revision 676) +++ LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/dts (working copy) @@ -36,9 +36,6 @@ /* 0:IDE 1:FLASH, if you are using NAND flash instead of IDE drive. */ enable_ide_nand_flash = "0";
- /* IDE: enable CS5536 IDE. There may be a different IDE controller on board */
- enable_ide = "0";
- /* Enable USB Port 4 (0:host 1:device). */ enable_USBP4_device = "0";
...
Index: LinuxBIOSv3-hierarchical_dts/mainboard/artecgroup/dbe62/dts
--- LinuxBIOSv3-hierarchical_dts/mainboard/artecgroup/dbe62/dts (revision 676) +++ LinuxBIOSv3-hierarchical_dts/mainboard/artecgroup/dbe62/dts (working copy) @@ -34,7 +34,6 @@ }; pci@15,0 { /config/("southbridge/amd/cs5536/dts");
enable_ide = "1"; /* Interrupt enables for LPC bus. * Each bit is an IRQ 0-15. */ lpc_serirq_enable = "0x00001002";@@ -54,5 +53,9 @@ /* Set com2 IRQ to be what is usually COM1 */ com2_irq = "4"; };
pci@15,2 {/config/("southbridge/amd/cs5536/ide");enable_ide = "1";};};
Just removing enable_ide as in southbridge/cs5536 would be better in my opinion. DBE62 uses NAND not any IDE, so I was actually intending to send a patch that sets enable_ide to 0, but now I saw this.
Those things said, not qualified for acking
Mart Raudsepp Artec Design LLC
On Wed, May 7, 2008 at 1:04 AM, Mart Raudsepp
Just removing enable_ide as in southbridge/cs5536 would be better in my opinion. DBE62 uses NAND not any IDE, so I was actually intending to send a patch that sets enable_ide to 0, but now I saw this.
why is that an issue? The default value is 0.
ron
OK, if stepan Acks this patch of Carl-Daniel, I think I will ack it too.
So, conditional on stepan, Acked-by: Ronald G. Minnich rminnich@gmail.com
overall, it's the best idea I've seen.
ron
On Wed, May 07, 2008 at 07:22:07AM -0700, ron minnich wrote:
OK, if stepan Acks this patch of Carl-Daniel, I think I will ack it too.
So, conditional on stepan, Acked-by: Ronald G. Minnich rminnich@gmail.com
overall, it's the best idea I've seen.
Short-term, yes, but I think we need to figure this out.
//Peter
On Wed, May 7, 2008 at 7:26 AM, Peter Stuge peter@stuge.se wrote:
Short-term, yes, but I think we need to figure this out.
Agree. But I'm a big believer in breadboards. See PIke's comments on this sort of thing -- I agree with him.
We can design all we want, but, until we write code, it's not clear we got it right.
Speaking of which -- what did we do w.r.t. SELF? ron
On 07/05/08 08:17 -0700, ron minnich wrote:
On Wed, May 7, 2008 at 7:26 AM, Peter Stuge peter@stuge.se wrote:
Short-term, yes, but I think we need to figure this out.
Agree. But I'm a big believer in breadboards. See PIke's comments on this sort of thing -- I agree with him.
We can design all we want, but, until we write code, it's not clear we got it right.
Speaking of which -- what did we do w.r.t. SELF?
Still open for debate. I'm going to start coding it up for libpayload so I can get moving on the chooser, but I dare not touch coreboot until we get a consensus.
Jordan
On 07.05.2008 22:25, Jordan Crouse wrote:
On 07/05/08 08:17 -0700, ron minnich wrote:
On Wed, May 7, 2008 at 7:26 AM, Peter Stuge peter@stuge.se wrote:
Short-term, yes, but I think we need to figure this out.
Agree. But I'm a big believer in breadboards. See PIke's comments on this sort of thing -- I agree with him.
We can design all we want, but, until we write code, it's not clear we got it right.
Speaking of which -- what did we do w.r.t. SELF?
Still open for debate. I'm going to start coding it up for libpayload so I can get moving on the chooser, but I dare not touch coreboot until we get a consensus.
I have some 60-odd mails about SELF in my "ToDo" folder, but at least this week I will be swamped with organizing LinuxTag and writing more text for my thesis. I hope to answer most of these mails in a batch sometime next week.
Regards, Carl-Daniel
Stefan? Can you take a look at my variant of the patch?
Regards, Carl-Daniel
On 07.05.2008 16:22, ron minnich wrote:
OK, if stepan Acks this patch of Carl-Daniel, I think I will ack it too.
So, conditional on stepan, Acked-by: Ronald G. Minnich rminnich@gmail.com
overall, it's the best idea I've seen.
ron
Carl-Daniel Hailfinger wrote:
See below for my take at this.
Move CS5536 IDE configuration into a separate dts and its own PCI device.
Carl-Daniel, is this the patch you mentioned?
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Build-tested on db800, norwich, dbe62, alix.1c, alix.2c3. Breaks build for dbe61.
Why is that? Please fix this before committing.
Without breaks, this patch is Acked-by: Stefan Reinauer stepan@coresystems.de
Index: LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/cs5536.c
--- LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/cs5536.c (revision 676) +++ LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/cs5536.c (working copy) @@ -590,6 +590,11 @@ { u32 ide_cfg;
- struct southbridge_amd_cs5536_ide_config *ide =
(struct southbridge_amd_cs5536_ide_config *)dev->device_configuration;- if (!ide->enable_ide)
return;- printk(BIOS_DEBUG, "cs5536_ide: %s\n", __func__); /* GPIO and IRQ setup are handled in the main chipset code. */
@@ -654,9 +659,6 @@ hide_vpci(sb->unwanted_vpci[i]); }
if (sb->enable_ide)
ide_init(dev);cs5536_setup_power_button(sb);
printk(BIOS_SPEW, "cs5536: %s() Exit\n", __FUNCTION__);
@@ -688,3 +690,17 @@ .phase6_init = southbridge_init, };
+struct device_operations cs5536_ide = {
- .id = {.type = DEVICE_ID_PCI,
.u = {.pci = {.vendor = PCI_VENDOR_ID_AMD,.device = PCI_DEVICE_ID_AMD_CS5536_B0_IDE}}},- .constructor = default_device_constructor,
+#warning FIXME: what has to go in phase3_scan?
- .phase3_scan = 0,
- .phase4_read_resources = pci_dev_read_resources,
- .phase4_set_resources = pci_dev_set_resources,
- .phase5_enable_resources = pci_dev_enable_resources,
- .phase6_init = ide_init,
- .ops_pci = &pci_dev_ops_pci,
+};
In v2 I liked that every hardware driver for every pci device was in a single, small file. Packing all of it in one large cs5536.c is a bit ugly. But that is not within the scope of this patch I guess.
On 08.05.2008 00:38, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
See below for my take at this.
Move CS5536 IDE configuration into a separate dts and its own PCI device.
Carl-Daniel, is this the patch you mentioned?
Yes.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Build-tested on db800, norwich, dbe62, alix.1c, alix.2c3. Breaks build for dbe61.
Why is that? Please fix this before committing.
Will do. I'll also integrate the fixes Mart Raudsepp suggested for dbe61 and dbe62.
Without breaks, this patch is Acked-by: Stefan Reinauer stepan@coresystems.de
Thanks!
Regards, Carl-Daniel
Index: LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/cs5536.c
--- LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/cs5536.c (revision 676) +++ LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/cs5536.c (working copy) @@ -590,6 +590,11 @@ { u32 ide_cfg;
- struct southbridge_amd_cs5536_ide_config *ide =
(struct southbridge_amd_cs5536_ide_config*)dev->device_configuration;
- if (!ide->enable_ide)
return;- printk(BIOS_DEBUG, "cs5536_ide: %s\n", __func__); /* GPIO and IRQ setup are handled in the main chipset code. */
@@ -654,9 +659,6 @@ hide_vpci(sb->unwanted_vpci[i]); }
if (sb->enable_ide)
ide_init(dev);cs5536_setup_power_button(sb);
printk(BIOS_SPEW, "cs5536: %s() Exit\n", __FUNCTION__);
@@ -688,3 +690,17 @@ .phase6_init = southbridge_init, };
+struct device_operations cs5536_ide = {
- .id = {.type = DEVICE_ID_PCI,
.u = {.pci = {.vendor = PCI_VENDOR_ID_AMD,.device = PCI_DEVICE_ID_AMD_CS5536_B0_IDE}}},- .constructor = default_device_constructor,
+#warning FIXME: what has to go in phase3_scan?
- .phase3_scan = 0,
- .phase4_read_resources = pci_dev_read_resources,
- .phase4_set_resources = pci_dev_set_resources,
- .phase5_enable_resources = pci_dev_enable_resources,
- .phase6_init = ide_init,
- .ops_pci = &pci_dev_ops_pci,
+};
In v2 I liked that every hardware driver for every pci device was in a single, small file. Packing all of it in one large cs5536.c is a bit ugly. But that is not within the scope of this patch I guess.
On 08.05.2008 01:03, Carl-Daniel Hailfinger wrote:
On 08.05.2008 00:38, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
See below for my take at this.
Move CS5536 IDE configuration into a separate dts and its own PCI device.
Carl-Daniel, is this the patch you mentioned?
Yes.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Build-tested on db800, norwich, dbe62, alix.1c, alix.2c3. Breaks build for dbe61.
Why is that? Please fix this before committing.
Will do. I'll also integrate the fixes Mart Raudsepp suggested for dbe61 and dbe62.
Done. Please note that the dbe61 is now back to the preexisting age-old breakage, but the new breakage has been fixed. The dbe61 code has too many v2 leftovers to compile.
Without breaks, this patch is Acked-by: Stefan Reinauer stepan@coresystems.de
Thanks!
Committed in r677.
Regards, Carl-Daniel
Regards, Carl-Daniel
Index: LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/cs5536.c
--- LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/cs5536.c (revision 676) +++ LinuxBIOSv3-hierarchical_dts/southbridge/amd/cs5536/cs5536.c (working copy) @@ -590,6 +590,11 @@ { u32 ide_cfg;
- struct southbridge_amd_cs5536_ide_config *ide =
(struct southbridge_amd_cs5536_ide_config*)dev->device_configuration;
- if (!ide->enable_ide)
return;- printk(BIOS_DEBUG, "cs5536_ide: %s\n", __func__); /* GPIO and IRQ setup are handled in the main chipset code. */
@@ -654,9 +659,6 @@ hide_vpci(sb->unwanted_vpci[i]); }
if (sb->enable_ide)
ide_init(dev);cs5536_setup_power_button(sb);
printk(BIOS_SPEW, "cs5536: %s() Exit\n", __FUNCTION__);
@@ -688,3 +690,17 @@ .phase6_init = southbridge_init, };
+struct device_operations cs5536_ide = {
- .id = {.type = DEVICE_ID_PCI,
.u = {.pci = {.vendor = PCI_VENDOR_ID_AMD,.device = PCI_DEVICE_ID_AMD_CS5536_B0_IDE}}},- .constructor = default_device_constructor,
+#warning FIXME: what has to go in phase3_scan?
- .phase3_scan = 0,
- .phase4_read_resources = pci_dev_read_resources,
- .phase4_set_resources = pci_dev_set_resources,
- .phase5_enable_resources = pci_dev_enable_resources,
- .phase6_init = ide_init,
- .ops_pci = &pci_dev_ops_pci,
+};
In v2 I liked that every hardware driver for every pci device was in a single, small file. Packing all of it in one large cs5536.c is a bit ugly. But that is not within the scope of this patch I guess.
That will come as a separate cleanup, but probably not from me in the next 3 weeks.
On Tue, May 06, 2008 at 08:37:34AM -0700, ron minnich wrote:
the cs5536 is one chip. On v2, we treated it as more than one. For my goal of easing non-experts into the code, I decided to try making it one chip and one dts.
..
I still think one dts per chip is the right way to go, but do we need more than one level of dts?
One level as in a single file - yes I think so.
One level as in one level of {} within that file - no I don't think so.
Whatever we do in dtc that depends on the file name right now should instead depend on something else. Maybe device path+id? Or we create a new namespace for chip "friendlynames" that are used to build code?
Case in point, looking at alix1c/dts:
/{ mainboard-vendor = "PC Engines"; mainboard-name = "ALIX1.C"; cpus { }; apic@0 { /config/("northbridge/amd/geodelx/apic"); }; domain@0 { /config/("northbridge/amd/geodelx/domain");
I'd like everything up to this point to be "inherited" from one geodelx dts.
/* Video RAM has to be in 2MB chunks. */ geode_video_mb = "8"; pci@1,0 { /config/("northbridge/amd/geodelx/pci"); };
Oh, and the above 1,0 device as well.
pci@15,0 { /config/("southbridge/amd/cs5536/dts"); enable_ide = "1";
This would stay pretty much as is, except maybe the enable_ide should be a generic enable variable instead, and be in pci@15,4 or whatever.
Is 15 really determined by the mainboard by the way - it should _not_ be in the mainboard dts otherwise.
All the above may be a little premature. I was hoping we would have "feedback" on the dts scheme from a larger K8 system available before we made any more changes, but it seems the time is now.
Maybe I should just not be afraid to change it back and forth a few times?
//Peter
On Wed, May 7, 2008 at 5:11 AM, Peter Stuge peter@stuge.se wrote:
Is 15 really determined by the mainboard by the way - it should _not_ be in the mainboard dts otherwise.
on most boards it is. But, at the time time, the 15.2 for IDE is determined entirely by the chip. I should not have to say that ide is f.2 -- it's not really a seperate part. So how do we do this?
ron
ron minnich wrote:
I know some people will get upset by the find_pci_device, but: This patch fixes a long-standing problem in the cs5536 driver, that is probably also in v2.
The ide_init is called with the sb device but needs the IDE device, which is different.
You are going to run into this problem on almost every chipset. Integrated PCI devices is normal (IDE, SMB, SATA, USB, LPC, audio, etc).
It also did a write_config8,not write_config32; this bug has been in there since the code was written for v2. I don't know how or why it ever worked, since PWB is 1<<14.
Yup, bug. I will post a v2 patch.
PWB should be set for better performance so it shouldn't cause complete breakage.
Marc
Marc Jones wrote:
ron minnich wrote:
I know some people will get upset by the find_pci_device, but: This patch fixes a long-standing problem in the cs5536 driver, that is probably also in v2.
..
It also did a write_config8,not write_config32; this bug has been in there since the code was written for v2. I don't know how or why it ever worked, since PWB is 1<<14.
Yup, bug. I will post a v2 patch.
PWB should be set for better performance so it shouldn't cause complete breakage.
On Tue, May 6, 2008 at 9:40 AM, Marc Jones Marc.Jones@amd.com wrote:
PWB was not getting set since it is 1<<14 and it was only doing a pci_write_config8.
Signed-off-by: Marc Jones marc.jones@amd.com
Acked-by: Ronald G. Minnich rminnich@gmail.com
ron minnich wrote:
I know some people will get upset by the find_pci_device, but: This patch fixes a long-standing problem in the cs5536 driver, that is probably also in v2.
The ide_init is called with the sb device but needs the IDE device, which is different.
Then that is the error that needs to be fixed. ide_init needs to be called with the IDE device.
Signed-off-by: Ronald G. Minnich rminnich@gmail.com
I veto. The above is against the concept of constructors we have in v3 or drivers we have in v2. If we start searching for device X in code attached to device Y, we could as well just make a long linear piece of code searching for all devices and stop walking over the device tree.
Stefan
On 07.05.2008 12:29, Stefan Reinauer wrote:
ron minnich wrote:
I know some people will get upset by the find_pci_device, but: This patch fixes a long-standing problem in the cs5536 driver, that is probably also in v2.
The ide_init is called with the sb device but needs the IDE device, which is different.
Then that is the error that needs to be fixed. ide_init needs to be called with the IDE device.
Signed-off-by: Ronald G. Minnich rminnich@gmail.com
I veto. The above is against the concept of constructors we have in v3 or drivers we have in v2. If we start searching for device X in code attached to device Y, we could as well just make a long linear piece of code searching for all devices and stop walking over the device tree.
See my counter-proposal patch in this thread from a few hours ago. It should address your worries.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Jordan Crouse -
Marc Jones -
Mart Raudsepp -
Peter Stuge -
ron minnich -
Stefan Reinauer -
Uwe Hermann