[U-Boot] IDE_BUS unconditionally expects 2 devices per bus

[U-Boot] IDE_BUS unconditionally expects 2 devices per bus

[Rogan Dawes]

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

[U-Boot] IDE_BUS unconditionally expects 2 devices per bus

[Rogan Dawes]

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

[U-Boot] IDE_BUS unconditionally expects 2 devices per bus

[Albert ARIBAUD]

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

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

[Rogan Dawes]

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

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"In Christianity neither morality nor religion come into contact with
reality at any point."                          - Friedrich Nietzsche

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

[Rogan Dawes]

This addresses Wolfgang's suggestion to use additional parens

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

[Rogan Dawes]

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

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

[Rogan Dawes]

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

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

[Albert ARIBAUD]

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

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

[Albert ARIBAUD]

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

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

[Rogan Dawes]

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

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The good Christian should beware of mathematicians and all those who
make empty prophecies. The danger already exists that  mathematicians
have  made a covenant with the devil to darken the spirit and confine
man in the bonds of Hell."                          - Saint Augustine

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

[Albert ARIBAUD]

Hi Wolfgang,

Le 06/09/2010 00:19, Wolfgang Denk a ?crit :
> 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.

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

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

[Rogan Dawes]

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

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
When all else fails, read the instructions.

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

[Albert ARIBAUD]

Hi again Wolfgang,

Le 06/09/2010 08:05, Wolfgang Denk a ?crit :
> 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?

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

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

[Wolfgang Denk]

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

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.

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

> 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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Crash programs fail because they are based on the theory  that,  with
nine women pregnant, you can get a baby a month.  - Wernher von Braun

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

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

[Wolfgang Denk]

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.

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

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"One planet is all you get."

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

[Albert ARIBAUD]

Le 06/09/2010 14:50, Wolfgang Denk a ?crit :

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

Amicalement,
-- 
Albert.

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
In theory, there is no difference between  theory  and  practice.  In
practice, however, there is.