This is the mail archive of the
ecos-patches@sources.redhat.com
mailing list for the eCos project.
Re: [flash_v2] Handle devices which don't initialise
- From: Bart Veer <bartv at ecoscentric dot com>
- To: andrew at lunn dot ch
- Cc: ecos-patches at ecos dot sourceware dot org
- Date: Sun, 28 Nov 2004 19:08:50 +0000 (GMT)
- Subject: Re: [flash_v2] Handle devices which don't initialise
- References: <20041125135405.GI31200@lunn.ch>
>>>>> "Andrew" == Andrew Lunn <andrew@lunn.ch> writes:
Andrew> I found a bug that stops my board from working.
Andrew> Manufacturing can either put a Strata or an SST device on
Andrew> the board and we want one image that runs both. So the
Andrew> image has both drivers and we simple let one fail to
Andrew> initialise. With my old code i only put devices that
Andrew> initialised onto the head of the linked list. You changed
Andrew> this so all devices in the table get put onto the list and
Andrew> find_dev() checked the device was initialised before
Andrew> returning it.
Yes, I see where I have changed the behaviour and broken things.
Andrew> This however leads to one problem. After sorting the
Andrew> linked list into order there is a check to see if any
Andrew> devices overlap. This check failed because the device
Andrew> which failed to initialise uses the same addresses as the
Andrew> device which did initialise.
Andrew> So i've changed the code to only add devices which
Andrew> initialise onto the list and then optimised the code a
Andrew> little.
Agree in principle, but:
Andrew> Index: io/flash/current/src/flash.c
Andrew> ===================================================================
Andrew> RCS file: /cvs/ecos/ecos/packages/io/flash/current/src/flash.c,v
Andrew> retrieving revision 1.26.2.14
Andrew> diff -u -r1.26.2.14 flash.c
Andrew> --- io/flash/current/src/flash.c 22 Nov 2004 13:25:23 -0000 1.26.2.14
Andrew> +++ io/flash/current/src/flash.c 25 Nov 2004 13:39:43 -0000
Andrew> @@ -154,15 +154,17 @@
Andrew> bool moved;
Andrew> struct cyg_flash_dev *dev, **previous_next;
Andrew> - // Place all devices on the list, unsorted for now.
Andrew> + // Place all devices that initialised on the list, unsorted for now.
Andrew> for (dev = &cyg_flashdevtab[0]; dev != &cyg_flashdevtab_end; dev++) {
Andrew> - dev->next = flash_head;
Andrew> - flash_head = dev;
Andrew> + if (dev->init) {
Andrew> + dev->next = flash_head;
Andrew> + flash_head = dev;
Andrew> + }
Andrew> }
Andrew> - // If there are no devices, abort. This should not happen because of
Andrew> - // the constraints on CYGHWR_IO_FLASH_DEVICE.
Andrew> - if (flash_head == NULL) {
Andrew> + // If there are no devices, abort. This could happen because none
Andrew> + // of the devices initialised. Also short cut for just one device.
Andrew> + if (flash_head == NULL || flash_head->next == NULL) {
Andrew> return false;
Andrew> }
This flash_head->next test looks wrong. If there are no valid flash
devices then initialization should fail. However if there is a single
flash device then initialization should succeed. In your scenario
flash_sort_and_check() will now return false, even though there is a
valid flash device, and hence the flash subsystem initialization as a
whole will fail and the init flag never gets set.
The following patch incorporates part of yours, so only initialized
devices get put on the list. I have also fixed cyg_flash_get_info() so
it will do the right thing for initialized/uninitialized flash
devices. I have removed the test for a single-entry list from
flash_sort_and_check(). In theory a shortcut is possible, returning
true if there is just one valid device. However the loops will still
work and finish immediately if there is just one device, so the
shortcut would only save a couple of cycles in a once-off init routine
yet add permanently to the code size.
Bart
Index: io/flash/current/ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/io/flash/current/ChangeLog,v
retrieving revision 1.38.2.21
diff -u -r1.38.2.21 ChangeLog
--- io/flash/current/ChangeLog 22 Nov 2004 21:27:43 -0000 1.38.2.21
+++ io/flash/current/ChangeLog 28 Nov 2004 19:02:12 -0000
@@ -1,3 +1,17 @@
+2004-11-28 Bart Veer <bartv@ecoscentric.com>
+
+ * src/flash.c (flash_sort_and_check): previous patch would have
+ resulted in init failure if only one device initialized.
+ (cyg_flash_get_info): handle per-device init flag
+
+2004-11-25 Andrew Lunn <andrew.lunn@ascom.ch>
+
+ * src/flash.c (flash_sort_and_check): Don't add devices which
+ failed to initialise onto the list. Don't bother sorting the
+ list if its empty or only has one entry.
+ * src/flash.c (find_dev): All devices on the list are initialised so
+ don't both checking the init flag.
+
2004-11-22 Bart Veer <bartv@ecoscentric.com>
* src/legacy_dev.c: remove .2ram attributes. These functions do
Index: io/flash/current/src/flash.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/io/flash/current/src/flash.c,v
retrieving revision 1.26.2.14
diff -u -r1.26.2.14 flash.c
--- io/flash/current/src/flash.c 22 Nov 2004 13:25:23 -0000 1.26.2.14
+++ io/flash/current/src/flash.c 28 Nov 2004 19:02:15 -0000
@@ -154,21 +154,24 @@
bool moved;
struct cyg_flash_dev *dev, **previous_next;
- // Place all devices on the list, unsorted for now.
+ // Place all devices that initialised on the list, unsorted for now.
for (dev = &cyg_flashdevtab[0]; dev != &cyg_flashdevtab_end; dev++) {
- dev->next = flash_head;
- flash_head = dev;
+ if (dev->init) {
+ dev->next = flash_head;
+ flash_head = dev;
+ }
}
- // If there are no devices, abort. This should not happen because of
- // the constraints on CYGHWR_IO_FLASH_DEVICE.
+ // If there are no devices, abort. This could happen because none
+ // of the devices initialised.
if (flash_head == NULL) {
return false;
}
// Sort the linked list into ascending order of flash address. Use a
// primitive ripple sort, but since we don't expect to have many
- // devices this should be OK.
+ // devices this should be OK. This loop may run safely with just one
+ // entry on the list.
do {
moved=false;
for (dev=flash_head, previous_next=&flash_head;
@@ -204,10 +207,6 @@
}
for (dev = flash_head; dev; dev = dev->next) {
if ((dev->start <= addr) && (addr <= dev->end)) {
- if (! dev->init) {
- *stat = CYG_FLASH_ERR_NOT_INIT;
- return NULL;
- }
return dev;
}
}
@@ -299,15 +298,16 @@
if (!init) return CYG_FLASH_ERR_NOT_INIT;
#if (1 == CYGHWR_IO_FLASH_DEVICE)
- if (0 == Nth) {
+ if ((0 == Nth) && cyg_flashdevtab[0].init) {
dev = &(cyg_flashdevtab[0]);
} else {
return CYG_FLASH_ERR_INVALID;
}
#else
+ // Only initialized devices are on the list.
for (dev = flash_head; dev && Nth; dev=dev->next, Nth--)
;
- if (!dev || !dev->init) {
+ if (!dev) {
return CYG_FLASH_ERR_INVALID;
}
#endif
--
Bart Veer eCos Configuration Architect
http://www.ecoscentric.com/ The eCos and RedBoot experts