[U-Boot] [PATCH] IDE: Don't assume there are always two devices per bus

[Albert ARIBAUD]

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.

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