IDE_BUS unconditionally expects 2 devices per bus

(quoted diff) +1 -1

-#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)])

Quoted 4 lines

(quoted diff) +1 -1

-#define        IDE_BUS(dev)    (dev>>   1)
+#define        IDE_BUS(dev)    (dev>>   (CONFIG_SYS_IDE_MAXDEVICE /

CONFIG_SYS_IDE_MAXBUS - 1))

Quoted 2 lines

    #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.

From: Rogan Dawes <[email protected]>

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(-)

include/ide.h +0 -0

index 6a1b7ae..c812b28 100644

Anything wrong with this patch?

Le 26/08/2010 15:16, Rogan Dawes a ?crit :

Quoted 1 line

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 :

Quoted 15 lines

Le 26/08/2010 15:16, Rogan Dawes a ?crit :

Quoted 1 line

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.

Quoted 6 lines

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,

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.

Dear Albert ARIBAUD,

Quoted 8 lines · In message<[email protected]> 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.

/* 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} \
}

Quoted 8 lines

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?

Quoted 2 lines

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:

/* 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} \

How about the two suggestions above?

Dear Albert ARIBAUD,

Quoted 9 lines · In message<[email protected]> you wrote:

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

Quoted 2 lines

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?

Quoted 4 lines

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").

Quoted 4 lines

/* 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.

Quoted 2 lines

	{ KW_SATA_PORT0_OFFSET, 1}, \
	{ KW_SATA_PORT1_OFFSET, 1} \

Quoted 6 lines

Quoted 4 lines

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.

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?

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 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.

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.

Quoted 3 lines

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.

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.

Quoted 2 lines

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.

Quoted 3 lines

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?

Quoted 3 lines

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?

Quoted 3 lines

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"?

Quoted 2 lines

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.

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.

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.

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.

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.

Quoted 4 lines

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.

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).