Patch generalizing IDE Reset function for MPC5200

U-Boot-Users Reinhard Meyer 2 participants 3 messages Mar 1, 2004

Share & tools

Reinhard Meyer

author

2004-03-01 11:27 UTC #0 lore raw

The following patch generalises the use of PSC1.4 for IDE Reset. The idea behind is that most probably many 5200 designs will be similiar to IceCube, _if_ they use the IDE interface. Using the otherwise quite useless PSC1.4 for UDE-Reset seems an obvious choice then.

Instead of continuosly adding more and more boards into the #if defined () chain, one define
CONFIG_IDE_RESET_USE_PSC1_4 can be added to the board's configuration file.

If someone else uses another I/O port, that can be easyly added in a similiar way.

This patch patches cpu/mpc5xxx/ide.c AND include/config/IceCube.h.

Thanks, Pierre, for the good work, the driver works fine on our board. -------------- next part --------------
A non-text attachment was scrubbed...
Name: u-boot.ide5200.patch
Type: application/octet-stream
Size: 1885 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20040301/ebade1a2/attachment.obj

Wolfgang Denk

2004-03-01 12:00 UTC #1 lore raw

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

The following patch generalises the use of PSC1.4 for IDE Reset. The idea behind is that most probably many 5200 designs will be similiar to IceCube,

Such assumptions are know to fail on the very next board that will be added.

Quoted 3 lines

Instead of continuosly adding more and more boards into the #if defined () chain, one define
CONFIG_IDE_RESET_USE_PSC1_4 can be added to the board's configuration file.

Urggh. What an ugly name.

Quoted 2 lines

If someone else uses another I/O port, that can be easyly added in a similiar way.

Yes, and we end up "adding more and more boards into the "#define CONFIG_IDE_RESET_USE_this" and "#define CONFIG_IDE_RESET_USE_that" chain.

Sorry, this code is ugly and I reject it.

Review

Quoted 8 lines

(quoted diff) +2 -2

-#if defined (CONFIG_ICECUBE) && defined (CONFIG_IDE_RESET)
+#if defined (CONFIG_IDE_RESET_USE_PSC1_4)
 	/* Configure PSC1_4 as GPIO output for ATA reset */
 	*(vu_long *) MPC5XXX_WU_GPIO_DATA |= GPIO_PSC1_4;
 	*(vu_long *) MPC5XXX_WU_GPIO_ENABLE |= GPIO_PSC1_4;
 	*(vu_long *) MPC5XXX_WU_GPIO_DIR |= GPIO_PSC1_4;
-#endif /* defined (CONFIG_ICECUBE) && defined (CONFIG_IDE_RESET) */
+#endif /* defined (CONFIG_IDE_RESET_USE_PSC1_4) */

How about changing this into something like??

include/configs/???.h:
...
#define CONFIG_IDE_RESET_PIN GPIO_PSC1_4

...

#ifdef CONFIG_IDE_RESET_PIN
	/* Configure IDE Reset pin as GPIO output for ATA reset */
	*(vu_long *) MPC5XXX_WU_GPIO_DATA |= CONFIG_IDE_RESET_PIN;
	...

#endif
...

Best regards,

Wolfgang Denk

Signature · 4 lines

Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd at denx.de
It's hard to think of you as the end result of millions of  years  of
evolution.

Reinhard Meyer

author

2004-03-01 13:24 UTC #2 lore raw

Hello,

if a real genuine generalization is wanted, the ide_set_reset () function must be moved to the board specific files.

"Generally" it cannot be assumed that a GPIO Port is used in a non-inverting way to perform a IDE_Reset.

Therefore I will now move
"void ide_set_reset (int idereset)"
to our top5200.c and leave the mpc5xxx/ide.c untouched.

best regards
Reinhard

----- Original Message -----

Quoted 1 line

Quoted 1 line
The following patch generalises the use of PSC1.4 for IDE Reset. The

idea

Quoted 1 line

Quoted 1 line
behind is that most probably many 5200 designs will be similiar to

IceCube,

Quoted 5 lines

Such assumptions are know to fail on the very next board that will be added.
Quoted 1 line
Instead of continuosly adding more and more boards into the #if defined

()

Quoted 2 lines

Quoted 2 lines
chain, one define
CONFIG_IDE_RESET_USE_PSC1_4 can be added to the board's configuration

file.

Quoted 12 lines

Urggh. What an ugly name.
Quoted 2 lines
If someone else uses another I/O port, that can be easyly added in a similiar way.
Yes, and we end up "adding more and more boards into the "#define CONFIG_IDE_RESET_USE_this" and "#define CONFIG_IDE_RESET_USE_that" chain.
Sorry, this code is ugly and I reject it.

Quoted 7 lines

.....
How about changing this into something like??
include/configs/???.h:
...
#define CONFIG_IDE_RESET_PIN GPIO_PSC1_4

THAT assumption will fail on the first board that does not use a GPIO pin for reset, or uses one that is inverted.