[U-Boot] IDE_BUS unconditionally expects 2 devices per bus
Hi folks
Since include/ide.h defines IDE_BUS(dev) as (dev >> 1), it ignores the values of CONFIG_SYS_IDE_MAXBUS and CONFIG_SYS_MAXDEVICE, and unconditionally expects an IDE bus to have two devices.
This expectation falls down with the Orion5x SATA support, which uses that SATA controller in IDE compatability mode, but can obviously only have one device per bus.
I'm not 100% sure whether the problem is with the IDE code, or with the Orion5x code which identifies a drive at both "positions" on the IDE bus. i.e. if I define CONFIG_SYS_IDE_MAXDEVICE as (CONFIG_SYS_IDE_MAXBUS*2), I get the first drive shown twice, and the second drive shown twice.
The following RFC patch works for me, but may well break other boards that do not define CONFIG_SYS_IDE_MAXDEVICE and CONFIG_SYS_IDE_MAXBUS properly:
diff --git a/include/ide.h b/include/ide.h index 6a1b7ae..3f81fb1 100644 --- a/include/ide.h +++ b/include/ide.h @@ -24,7 +24,7 @@ #ifndef _IDE_H #define _IDE_H
-#define IDE_BUS(dev) (dev >> 1) +#define IDE_BUS(dev) (dev >> (CONFIG_SYS_IDE_MAXDEVICE / CONFIG_SYS_IDE_MAXBUS - 1))
#define ATA_CURR_BASE(dev) (CONFIG_SYS_ATA_BASE_ADDR+ide_bus_offset[IDE_BUS(dev)])
Ok, I'm sure it is line wrapped, tab-damaged, etc, and the line itself is too long, but conceptually, would that be acceptable?
Thanks
Rogan
On 2010/08/14 12:41 PM, Rogan Dawes wrote:
-#define IDE_BUS(dev) (dev>> 1) +#define IDE_BUS(dev) (dev>> (CONFIG_SYS_IDE_MAXDEVICE / CONFIG_SYS_IDE_MAXBUS - 1))
#define ATA_CURR_BASE(dev) (CONFIG_SYS_ATA_BASE_ADDR+ide_bus_offset[IDE_BUS(dev)])
Ok, I'm an idiot! The reason was staring me in the face! ATA_CURR_BASE(dev) relies on IDE_BUS, which is why the same disk was being enumerated twice.
It seems that the above patch is indeed correct.
Rogan
Le 14/08/2010 12:46, Rogan Dawes a écrit :
On 2010/08/14 12:41 PM, Rogan Dawes wrote:
-#define IDE_BUS(dev) (dev>> 1) +#define IDE_BUS(dev) (dev>> (CONFIG_SYS_IDE_MAXDEVICE / CONFIG_SYS_IDE_MAXBUS - 1))
#define ATA_CURR_BASE(dev)(CONFIG_SYS_ATA_BASE_ADDR+ide_bus_offset[IDE_BUS(dev)])
Ok, I'm an idiot! The reason was staring me in the face! ATA_CURR_BASE(dev) relies on IDE_BUS, which is why the same disk was being enumerated twice.
It seems that the above patch is indeed correct.
Good findings, Rogan. :)
However, you should submit patches using git format-patch and git send-email, both properly configured -- make sure format-patch has the -s option and send-email has the proper e-mail address settings for sender and recipient(s).
And before submitting, think of checking the patch with linux's script/checkpatch.pl. --no-tree. At least it'll let you know about the long line. Mind you, ide.h itself has several long lines, that checkpatch won't tell you about since this is outside your patch. :)
I applied the change manually, With 2 busse and 2 devices on the ED Mini (which has only one disk), I don't get the duplicate drive. With 1 bus and 2 devices, I do see the disk twice. :(
Amicalement,
Some SATA controllers can operate in an IDE compatible mode (e.g. mvsata) but will only ever have a single device per bus.
This allows the upcoming DNS323 port to properly identify and use a drive on both SATA interfaces. --- include/ide.h | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/include/ide.h b/include/ide.h index 6a1b7ae..85a48f8 100644 --- a/include/ide.h +++ b/include/ide.h @@ -24,7 +24,8 @@ #ifndef _IDE_H #define _IDE_H
-#define IDE_BUS(dev) (dev >> 1) +#define IDE_BUS(dev) (dev >> (CONFIG_SYS_IDE_MAXDEVICE / \ + CONFIG_SYS_IDE_MAXBUS - 1))
#define ATA_CURR_BASE(dev) (CONFIG_SYS_ATA_BASE_ADDR+ide_bus_offset[IDE_BUS(dev)])
Dear Rogan Dawes,
In message 1281904542-11694-1-git-send-email-rogan@dawes.za.net you wrote:
-#define IDE_BUS(dev) (dev >> 1) +#define IDE_BUS(dev) (dev >> (CONFIG_SYS_IDE_MAXDEVICE / \
CONFIG_SYS_IDE_MAXBUS - 1))
Please add parens to make clear you really mean what you write. Especially with the line wrapping this could easily be misread.
Best regards,
Wolfgang Denk
This addresses Wolfgang's suggestion to use additional parens
From: Rogan Dawes rogan@dawes.za.net
Some SATA controllers can operate in an IDE compatible mode (e.g. mvsata) but will only ever have a single device per bus.
This allows the upcoming DNS323 port to properly identify and use a drive on both SATA interfaces. --- include/ide.h | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/include/ide.h b/include/ide.h index 6a1b7ae..c812b28 100644 --- a/include/ide.h +++ b/include/ide.h @@ -24,7 +24,8 @@ #ifndef _IDE_H #define _IDE_H
-#define IDE_BUS(dev) (dev >> 1) +#define IDE_BUS(dev) (dev >> ((CONFIG_SYS_IDE_MAXDEVICE / \ + CONFIG_SYS_IDE_MAXBUS) - 1))
#define ATA_CURR_BASE(dev) (CONFIG_SYS_ATA_BASE_ADDR+ide_bus_offset[IDE_BUS(dev)])
On 2010/08/16 7:47 AM, Rogan Dawes wrote:
From: Rogan Dawes rogan@dawes.za.net
Some SATA controllers can operate in an IDE compatible mode (e.g. mvsata) but will only ever have a single device per bus.
This allows the upcoming DNS323 port to properly identify and use a drive on both SATA interfaces.
include/ide.h | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/include/ide.h b/include/ide.h index 6a1b7ae..c812b28 100644 --- a/include/ide.h +++ b/include/ide.h @@ -24,7 +24,8 @@ #ifndef _IDE_H #define _IDE_H
-#define IDE_BUS(dev) (dev >> 1) +#define IDE_BUS(dev) (dev >> ((CONFIG_SYS_IDE_MAXDEVICE / \
CONFIG_SYS_IDE_MAXBUS) - 1))#define ATA_CURR_BASE(dev) (CONFIG_SYS_ATA_BASE_ADDR+ide_bus_offset[IDE_BUS(dev)])
Anything wrong with this patch?
Regards,
Rogan
Le 26/08/2010 15:16, Rogan Dawes a écrit :
Anything wrong with this patch?
I think I finally found what was bugging me with it.
Granted, there are cases where we don't want two devices per bus, but this is a requirement unrelated to the maximum number of busses and devices: this is simply due to the fact that we're on a SATA, not PATA, controller.
I think that, rather than modifying IDE_BUS(dev), you should introduce a CONFIG_SYS_IDE_MAXDEVICE_PER_BUS config option that will limit how many devices will be probed for on a given bus.
Without this config option, for each bus B there can be up to two devices, numbered (B*2) and (B*2+1); with the config option, there can be only one device numbered (B*2). In all cases, IDE_BUS(dev) can remain defined as (dev >> 1) which will always amount to B.
Amicalement,
Le 04/09/2010 10:22, Albert ARIBAUD a écrit :
Le 26/08/2010 15:16, Rogan Dawes a écrit :
Anything wrong with this patch?
I think I finally found what was bugging me with it.
Granted, there are cases where we don't want two devices per bus, but this is a requirement unrelated to the maximum number of busses and devices: this is simply due to the fact that we're on a SATA, not PATA, controller.
I think that, rather than modifying IDE_BUS(dev), you should introduce a CONFIG_SYS_IDE_MAXDEVICE_PER_BUS config option that will limit how many devices will be probed for on a given bus.
Without this config option, for each bus B there can be up to two devices, numbered (B*2) and (B*2+1); with the config option, there can be only one device numbered (B*2). In all cases, IDE_BUS(dev) can remain defined as (dev>> 1) which will always amount to B.
Amicalement,
Rogan,
Actually, I am looking into refactoring the cmd_ide.c code right now, because I'll need it for supporting the net5big's eight ports and devices. Do you mind if I give a try at my own suggestion?
Amicalement,
On 2010/09/04 11:07 AM, Albert ARIBAUD wrote:
Le 04/09/2010 10:22, Albert ARIBAUD a écrit :
Le 26/08/2010 15:16, Rogan Dawes a écrit :
Anything wrong with this patch?
I think I finally found what was bugging me with it.
Granted, there are cases where we don't want two devices per bus, but this is a requirement unrelated to the maximum number of busses and devices: this is simply due to the fact that we're on a SATA, not PATA, controller.
I think that, rather than modifying IDE_BUS(dev), you should introduce a CONFIG_SYS_IDE_MAXDEVICE_PER_BUS config option that will limit how many devices will be probed for on a given bus.
Without this config option, for each bus B there can be up to two devices, numbered (B*2) and (B*2+1); with the config option, there can be only one device numbered (B*2). In all cases, IDE_BUS(dev) can remain defined as (dev>> 1) which will always amount to B.
Amicalement,
Rogan,
Actually, I am looking into refactoring the cmd_ide.c code right now, because I'll need it for supporting the net5big's eight ports and devices. Do you mind if I give a try at my own suggestion?
Amicalement,
By all means.
Have at it! :-)
Rogan
Dear Albert ARIBAUD,
In message 4C8201CB.9080606@free.fr you wrote:
I think that, rather than modifying IDE_BUS(dev), you should introduce a CONFIG_SYS_IDE_MAXDEVICE_PER_BUS config option that will limit how many devices will be probed for on a given bus. Without this config option, for each bus B there can be up to two devices, numbered (B*2) and (B*2+1); with the config option, there can be only one device numbered (B*2). In all cases, IDE_BUS(dev) can remain defined as (dev >> 1) which will always amount to B.
I'm not happy at all about this.
First, CONFIG_SYS_IDE_* is for "IDE" (aka "Integrated Drive Electronics"), now usually references as "Parallel ATA", as defined by the underlying AT Attachment (ATA) and AT Attachment Packet Interface (ATAPI) standards. It is my understanding that these standards allow for one or two devices on the bus. So if there was any CONFIG_SYS_IDE_MAXDEVICE_PER_BUS, it would have to be set to 2, which renders it useless.
Second, who says that one setting fits all busses in the system? What happens if you want (or need) to setb the limit to 1, and I insert a PCI PATA controller card with 2 devices attached to a bus?
Sorry, this does not seem to fit IMO.
Best regards,
Wolfgang Denk
Hi Wolfgang,
Le 06/09/2010 00:19, Wolfgang Denk a écrit :
Dear Albert ARIBAUD,
In message4C8201CB.9080606@free.fr you wrote:
I think that, rather than modifying IDE_BUS(dev), you should introduce a CONFIG_SYS_IDE_MAXDEVICE_PER_BUS config option that will limit how many devices will be probed for on a given bus. Without this config option, for each bus B there can be up to two devices, numbered (B*2) and (B*2+1); with the config option, there can be only one device numbered (B*2). In all cases, IDE_BUS(dev) can remain defined as (dev>> 1) which will always amount to B.
I'm not happy at all about this.
First, CONFIG_SYS_IDE_* is for "IDE" (aka "Integrated Drive Electronics"), now usually references as "Parallel ATA", as defined by the underlying AT Attachment (ATA) and AT Attachment Packet Interface (ATAPI) standards. It is my understanding that these standards allow for one or two devices on the bus. So if there was any CONFIG_SYS_IDE_MAXDEVICE_PER_BUS, it would have to be set to 2, which renders it useless.
You're right, and actually this is an overlook on my part, as the current set of port configs has _ATA_ in in addition to _IDE. Would the name CONFIG_SYS_ATA_IDE_MAXDEVICEPERBUS make you be happier?
Second, who says that one setting fits all busses in the system? What happens if you want (or need) to setb the limit to 1, and I insert a PCI PATA controller card with 2 devices attached to a bus?
Indeed; and additionally, who says you can only have two IDE busses? I am facing the case right now with the MV88SX6081, which is an 8-ports controller.
I have thus started a patch where the CONFIG_SYS_ATA_IDE{0,1}_OFFSET are replaced with a single one, CONFIG_SYS_ATA_IDE_OFFSETS, defined as an open array of values, which allows as many ports as required. For e.g. openrd_base, the config for ATA ports would change from:
/* ATA bus 0 is Kirkwood port 0 on openrd */ #define CONFIG_SYS_ATA_IDE0_OFFSET KW_SATA_PORT0_OFFSET /* ATA bus 1 is Kirkwood port 1 on openrd */ #define CONFIG_SYS_ATA_IDE1_OFFSET KW_SATA_PORT1_OFFSET
to:
/* OpenRD has two ATA busses, provided by kirkwood */ #define CONFIG_SYS_ATA_IDE_OFFSETS { \ KW_SATA_PORT0_OFFSET, \ KW_SATA_PORT1_OFFSET \ }
I could easily extend my work to account for a maximum number of devices per bus by replacing CONFIG_SYS_ATA_IDE_OFFSETS with a more general CONFIG_SYS_ATA_IDE_CONFIG array of structs which would provide the port offset and maximum devices for each bus:
/* OpenRD's two kirkwood busses are SATA: 1 device per bux max) */ #define CONFIG_SYS_ATA_IDE_CONFIG { \ { KW_SATA_PORT0_OFFSET, 1}, \ { KW_SATA_PORT1_OFFSET, 1} \ }
Sorry, this does not seem to fit IMO.
How about the two suggestions above?
Amicalement,
On 2010/09/06 7:54 AM, Albert ARIBAUD wrote:
/* OpenRD's two kirkwood busses are SATA: 1 device per bux max) */ #define CONFIG_SYS_ATA_IDE_CONFIG { \ { KW_SATA_PORT0_OFFSET, 1}, \ { KW_SATA_PORT1_OFFSET, 1} \ }
I like this, as it removes assumptions from the code.
FWIW.
Rogan
Dear Albert ARIBAUD,
In message 4C848200.7080401@free.fr you wrote:
First, CONFIG_SYS_IDE_* is for "IDE" (aka "Integrated Drive Electronics"), now usually references as "Parallel ATA", as defined by the underlying AT Attachment (ATA) and AT Attachment Packet Interface (ATAPI) standards. It is my understanding that these standards allow for one or two devices on the bus. So if there was any CONFIG_SYS_IDE_MAXDEVICE_PER_BUS, it would have to be set to 2, which renders it useless.
You're right, and actually this is an overlook on my part, as the current set of port configs has _ATA_ in in addition to _IDE. Would the name CONFIG_SYS_ATA_IDE_MAXDEVICEPERBUS make you be happier?
No. See the AT Attachment (ATA) and AT Attachment Packe Interface (ATAPI) standards.
happens if you want (or need) to setb the limit to 1, and I insert a PCI PATA controller card with 2 devices attached to a bus?
Indeed; and additionally, who says you can only have two IDE busses? I am facing the case right now with the MV88SX6081, which is an 8-ports controller.
But this is NOT an PATA controller, right?
I have thus started a patch where the CONFIG_SYS_ATA_IDE{0,1}_OFFSET are replaced with a single one, CONFIG_SYS_ATA_IDE_OFFSETS, defined as an open array of values, which allows as many ports as required. For e.g. openrd_base, the config for ATA ports would change from:
We should not use "IDE" code on non-IDE devices. If there are common code parts, these should be split off and generalized as needed (without reference to "ATA" or "IDE").
/* OpenRD's two kirkwood busses are SATA: 1 device per bux max) */ #define CONFIG_SYS_ATA_IDE_CONFIG { \ { KW_SATA_PORT0_OFFSET, 1}, \ { KW_SATA_PORT1_OFFSET, 1} \
No. It is inherently wrong to use "*IDE*" in combination with a SATA device.
How about the two suggestions above?
No.
Best regards,
Wolfgang Denk
Hi again Wolfgang,
Le 06/09/2010 08:05, Wolfgang Denk a écrit :
Dear Albert ARIBAUD,
In message4C848200.7080401@free.fr you wrote:
First, CONFIG_SYS_IDE_* is for "IDE" (aka "Integrated Drive Electronics"), now usually references as "Parallel ATA", as defined by the underlying AT Attachment (ATA) and AT Attachment Packet Interface (ATAPI) standards. It is my understanding that these standards allow for one or two devices on the bus. So if there was any CONFIG_SYS_IDE_MAXDEVICE_PER_BUS, it would have to be set to 2, which renders it useless.
You're right, and actually this is an overlook on my part, as the current set of port configs has _ATA_ in in addition to _IDE. Would the name CONFIG_SYS_ATA_IDE_MAXDEVICEPERBUS make you be happier?
No. See the AT Attachment (ATA) and AT Attachment Packe Interface (ATAPI) standards.
happens if you want (or need) to setb the limit to 1, and I insert a PCI PATA controller card with 2 devices attached to a bus?
Indeed; and additionally, who says you can only have two IDE busses? I am facing the case right now with the MV88SX6081, which is an 8-ports controller.
But this is NOT an PATA controller, right?
Correct. It is a SATA controller with ATA, not PATA, compatibility.
I have thus started a patch where the CONFIG_SYS_ATA_IDE{0,1}_OFFSET are replaced with a single one, CONFIG_SYS_ATA_IDE_OFFSETS, defined as an open array of values, which allows as many ports as required. For e.g. openrd_base, the config for ATA ports would change from:
We should not use "IDE" code on non-IDE devices. If there are common code parts, these should be split off and generalized as needed (without reference to "ATA" or "IDE").
For IDE as such, I agree. However:
1) Some systems simply do not allow more that one IDE drive for mechanical reasons. I suspect this is the reason behind the CONFIG_SYS_IDE_MAXDEVICE option, which, as such, contradicts IDE since it prevents cmd_ide to handle all 2*MAXBUS devices it could.
2) More generally, the "cmd_ide.c" actually uses ATA to communicate with the devices it manages; thus I feel cmd_ide.c is rightly used when on non-IDE-but-ATA-compatible devices, and that the issue is of naming only.
Besides, since cmd_ide's maximum bus feature is purely a design choice not dictated by standards, I assume your rebuttal addresses maximum devices per bus but not maximum busses, right? Would you agree at least to the minimal idea of having more than two ATA-compatible busses managed by cmd_ide?
/* OpenRD's two kirkwood busses are SATA: 1 device per bux max) */ #define CONFIG_SYS_ATA_IDE_CONFIG { \ { KW_SATA_PORT0_OFFSET, 1}, \ { KW_SATA_PORT1_OFFSET, 1} \
No. It is inherently wrong to use "*IDE*" in combination with a SATA device.
How about this: we could remove "IDE" altogether from config options which are not actually IDE related. This includes CONFIG_SYS_IDE_MAXBUS which is neither IDE- or ATA-related but system-defined
We would then have:
#define CONFIG_SYS_ATA_CONFIG { \
{ KW_SATA_PORT0_OFFSET, 1}, \ { KW_SATA_PORT1_OFFSET, 1} \
We would also remove ATA from non-ATA related symbols used in cmd_ide, but so far the only one I see is CONFIG_SYS_IDE_MAXDEVICE, which is purely a hack to accommodate system limitations; and in my proposal, it would disappear from config files because it equals the sum of the "max device" fields in the struct.
Is this proposal better?
Amicalement,
Dear Albert ARIBAUD,
In message 4C848E1A.1070003@free.fr you wrote:
Indeed; and additionally, who says you can only have two IDE busses? I am facing the case right now with the MV88SX6081, which is an 8-ports controller.
But this is NOT an PATA controller, right?
Correct. It is a SATA controller with ATA, not PATA, compatibility.
OK. Then we must not talk about "IDE busses".
For IDE as such, I agree. However:
- Some systems simply do not allow more that one IDE drive for
mechanical reasons. I suspect this is the reason behind the CONFIG_SYS_IDE_MAXDEVICE option, which, as such, contradicts IDE since it prevents cmd_ide to handle all 2*MAXBUS devices it could.
I don't understand what you want to tell me. As far as PATA is concerned (and this is all what "*_IDE_*" config options refer to), we can have zero, one or two devices per bus, and the code supports this.
For systems, where only one device can be physically attached, CONFIG_SYS_IDE_MAXDEVICE allows to save memory footprint because in common/cmd_ide.c we allocate only as many device descriptors as we need:
block_dev_desc_t ide_dev_desc[CONFIG_SYS_IDE_MAXDEVICE];
This is just an implementation detail to avoid wasting memory; it has nothing to do with restrictions in implementing the PATA specs.
- More generally, the "cmd_ide.c" actually uses ATA to communicate with
the devices it manages; thus I feel cmd_ide.c is rightly used when on non-IDE-but-ATA-compatible devices, and that the issue is of naming only.
Maybe. But if you are using it on such devices, please make sure not to apply terms that make sense on IDE / PATA devices only.
Besides, since cmd_ide's maximum bus feature is purely a design choice not dictated by standards, I assume your rebuttal addresses maximum
This "design choice" is merely the coinsequence of providing a static allocation of data structures with minimal memory footprint. This is OK for typical embedded systems where the configuration is inherently static and known in advance.
devices per bus but not maximum busses, right? Would you agree at least to the minimal idea of having more than two ATA-compatible busses managed by cmd_ide?
Yes, I agree. But this is not a new feature, but already supported out of the box, isn't it?
How about this: we could remove "IDE" altogether from config options which are not actually IDE related. This includes CONFIG_SYS_IDE_MAXBUS which is neither IDE- or ATA-related but system-defined
It may be "system-defined" (what exactly do you mean by that term?), but it is ATA-related, isn't it?
We would also remove ATA from non-ATA related symbols used in cmd_ide, but so far the only one I see is CONFIG_SYS_IDE_MAXDEVICE, which is purely a hack to accommodate system limitations; and in my proposal, it
What makes you think this is a "hack"?
would disappear from config files because it equals the sum of the "max device" fields in the struct.
This would just waste memory for data structures which will never be used.
Best regards,
Wolfgang Denk
Le 06/09/2010 10:18, Wolfgang Denk a écrit :
I don't understand what you want to tell me. As far as PATA is concerned (and this is all what "*_IDE_*" config options refer to), we can have zero, one or two devices per bus, and the code supports this.
For systems, where only one device can be physically attached, CONFIG_SYS_IDE_MAXDEVICE allows to save memory footprint because in common/cmd_ide.c we allocate only as many device descriptors as we need:
block_dev_desc_t ide_dev_desc[CONFIG_SYS_IDE_MAXDEVICE];
This is just an implementation detail to avoid wasting memory; it has nothing to do with restrictions in implementing the PATA specs.
Agreed -- more comments on CONFIG_SYS_IDE_MAXDEVICE below.
- More generally, the "cmd_ide.c" actually uses ATA to communicate with
the devices it manages; thus I feel cmd_ide.c is rightly used when on non-IDE-but-ATA-compatible devices, and that the issue is of naming only.
Maybe. But if you are using it on such devices, please make sure not to apply terms that make sense on IDE / PATA devices only.
Besides, since cmd_ide's maximum bus feature is purely a design choice not dictated by standards, I assume your rebuttal addresses maximum
This "design choice" is merely the coinsequence of providing a static allocation of data structures with minimal memory footprint. This is OK for typical embedded systems where the configuration is inherently static and known in advance.
devices per bus but not maximum busses, right? Would you agree at least to the minimal idea of having more than two ATA-compatible busses managed by cmd_ide?
Yes, I agree. But this is not a new feature, but already supported out of the box, isn't it?
Er... no, it isn't. cmd_ide can support no more than two busses, the offsets of which are defined by CONFIG_SYS_ATA_IDE0_OFFSET, and optional CONFIG_SYS_ATA_IDE1_OFFSET if CONFIG_SYS_IDE_MAXBUS > 0 (note that these have _IDE in their names although they are not IDE per se).
So you cannot have more than two busses.
Moreover, the way it is done, if you want a third bus you have to modify cmd_ide.c to introduce CONFIG_SYS_ATA_IDE2_OFFSET, and once again later if you need a fourth one -- this does not scale well.
OTOH, my proposal to group offsets in CONFIG_SYS_ATA_IDE_OFFSETS ("IDE" kept in name to match the original CONFIG_SYS_ATA_IDEx_OFFSET names, but I can live without this "IDE") makes it scalable: the config option provides both the number of busses and their offsets, and the cmd_ide code would need no change to accomodate any number of busses.
How about this: we could remove "IDE" altogether from config options which are not actually IDE related. This includes CONFIG_SYS_IDE_MAXBUS which is neither IDE- or ATA-related but system-defined
It may be "system-defined" (what exactly do you mean by that term?), but it is ATA-related, isn't it?
I meant that neither IDE or ATA standards restrict the maximum number of busses that can coexist in a given system; only the system designer can introduce such a limit.
We would also remove ATA from non-ATA related symbols used in cmd_ide, but so far the only one I see is CONFIG_SYS_IDE_MAXDEVICE, which is purely a hack to accommodate system limitations; and in my proposal, it
What makes you think this is a "hack"?
I don't mean 'hack' in any negative way; what I mean is that restricting the number of devices to less than twice the number of busses is not mandated by any standard, and aims not at providing functionality but at reducing footprint.
would disappear from config files because it equals the sum of the "max device" fields in the struct.
This would just waste memory for data structures which will never be used.
I don't think there would be a waste of memory:
1) The new CONFIG_SYS_ATA_[IDE_]OFFSETS will not consome global memory, as it is only needed in ide_init() where it can initialize a local variable that ide_init() will run through to fill the existing ide_bus_offset[] and ide_dev_desc[].
2) as for ide_bus_offset[] and ide_dev_desc[], and any other existing array based on CONFIG_SYS_IDE_MAXBUS or CONFIG_SYS_IDE_MAXDEVICE, they are not going to grow any bigger with my proposal since neither config option will increase.
Amicalement,
Dear Albert ARIBAUD,
In message 4C84D13C.8040702@free.fr you wrote:
Er... no, it isn't. cmd_ide can support no more than two busses, the offsets of which are defined by CONFIG_SYS_ATA_IDE0_OFFSET, and optional CONFIG_SYS_ATA_IDE1_OFFSET if CONFIG_SYS_IDE_MAXBUS > 0 (note that these have _IDE in their names although they are not IDE per se).
So you cannot have more than two busses.
I see.
Moreover, the way it is done, if you want a third bus you have to modify cmd_ide.c to introduce CONFIG_SYS_ATA_IDE2_OFFSET, and once again later if you need a fourth one -- this does not scale well.
Agreed.
OTOH, my proposal to group offsets in CONFIG_SYS_ATA_IDE_OFFSETS ("IDE" kept in name to match the original CONFIG_SYS_ATA_IDEx_OFFSET names, but I can live without this "IDE") makes it scalable: the config option provides both the number of busses and their offsets, and the cmd_ide code would need no change to accomodate any number of busses.
I'm not sure about the IDE in the name (it's offset of IDE register structures, isn't it?), but that's not really important. I agree about the rest.
I meant that neither IDE or ATA standards restrict the maximum number of busses that can coexist in a given system; only the system designer can introduce such a limit.
OK.
I don't mean 'hack' in any negative way; what I mean is that restricting the number of devices to less than twice the number of busses is not mandated by any standard, and aims not at providing functionality but at reducing footprint.
Agreed.
- as for ide_bus_offset[] and ide_dev_desc[], and any other existing
array based on CONFIG_SYS_IDE_MAXBUS or CONFIG_SYS_IDE_MAXDEVICE, they are not going to grow any bigger with my proposal since neither config option will increase.
We might chnage this to dynamically allocated structures. maybe that would make more sense then?
Best regards,
Wolfgang Denk
Le 06/09/2010 14:50, Wolfgang Denk a écrit :
- as for ide_bus_offset[] and ide_dev_desc[], and any other existing
array based on CONFIG_SYS_IDE_MAXBUS or CONFIG_SYS_IDE_MAXDEVICE, they are not going to grow any bigger with my proposal since neither config option will increase.
We might chnage this to dynamically allocated structures. maybe that would make more sense then?
I don't see much benefit in allocating dynamically rather than statically unless you are really tight on RAM, which seems to not fit with a board which has many busses and disks.
Plus, IDE may not know about hotplugging, but SATA does, which means I can do an ide_init(), plug or unplug disks, and do an ide_init() again with a different number of devices. Dynamic allocation requires resizing the allocated arrays; static is just less trouble.
Anyway, static vs dynamic can be done in a second step after adding N busses (and thereby fixing the issue of ghost devices that prompted Rogan's patch proposal).
Amicalement,
Dear Albert ARIBAUD,
In message 4C8521A8.6050107@free.fr you wrote:
I don't see much benefit in allocating dynamically rather than statically unless you are really tight on RAM, which seems to not fit with a board which has many busses and disks.
Plus, IDE may not know about hotplugging, but SATA does, which means I can do an ide_init(), plug or unplug disks, and do an ide_init() again with a different number of devices. Dynamic allocation requires resizing the allocated arrays; static is just less trouble.
Anyway, static vs dynamic can be done in a second step after adding N busses (and thereby fixing the issue of ghost devices that prompted Rogan's patch proposal).
OK, I agree.
Best regards,
Wolfgang Denk
participants (3)
-
Albert ARIBAUD -
Rogan Dawes -
Wolfgang Denk