openOCD.fseu wrote: > please find attached the updated Fujistu CORTEX M3 NOR Flash driver. > It includes: > - Improvement of error handling of "retval = target_read/write_xxx();"
Great to get better error handling! All the repeated retval=..if() return look absolutely horrible but that's indeed not your fault. We should maybe have some handy macros for this in shared code, to make drivers prettier and simpler. > - No #if statements anymore > - Added "Flash type 2" support Great. Content-Description: 0001-Updated-fm3.c-added-Flash-type-2-support-error-handl.patch > --- a/src/flash/nor/fm3.c > +++ b/src/flash/nor/fm3.c > @@ -1,23 +1,28 @@ > -/*************************************************************************** > - * Copyright (C) 2011 by Marc Willam, Holger Wech * > - * [email protected] * > - * Copyright (C) 2011 Ronny Strutz * > - * * > - * This program is free software; you can redistribute it and/or modify * > - * it under the terms of the GNU General Public License as published by * > - * the Free Software Foundation; either version 2 of the License, or * > - * (at your option) any later version. * > - * * > - * This program is distributed in the hope that it will be useful, * > - * but WITHOUT ANY WARRANTY; without even the implied warranty of * > - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * > - * GNU General Public License for more details. * > - * * > - * You should have received a copy of the GNU General Public License * > - * along with this program; if not, write to the * > - * Free Software Foundation, Inc., * > - * 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. * > - ***************************************************************************/ > +/***************************************************************************** > + * Copyright (C) 2011 by Marc Willam, Holger Wech > * > + * openOCD.fseu(AT)de.fujitsu.com > * > + * Copyright (C) 2011 Ronny Strutz > * > + * > * > + * This program is free software; you can redistribute it and/or modify > * > + * it under the terms of the GNU General Public License as published by > * > + * the Free Software Foundation; either version 2 of the License, or > * > + * (at your option) any later version. > * > + * > * > + * This program is distributed in the hope that it will be useful, > * > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > * > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > * > + * GNU General Public License for more details. > * > + * > * > + * You should have received a copy of the GNU General Public License > * > + * along with this program; if not, write to the > * > + * Free Software Foundation, Inc., > * > + * 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. > * > + > *****************************************************************************/ Please avoid all whitespace-only changes. Perhaps especially so in the license text for a file. > + > +// History: > +// 2011-07-13 MWi First version > +// 2011-09-27 MWi Flash type 2 added, algorithm start address now > relocateable > + As Øyvind pointed out this is redundant, since git tracks history already, and with good detail. > enum fm3_variant > { > - mb9bfxx1, > + mb9bfxx1, /* Flash Type '1' */ > mb9bfxx2, > mb9bfxx3, > mb9bfxx4, > mb9bfxx5, > - mb9bfxx6 > + mb9bfxx6, > + mb9afxx1, /* Flash Type '2' */ > + mb9afxx2, > + mb9afxx3, > + mb9afxx4, > + mb9afxx5, > + mb9afxx6 > +}; It would help to have a separate patch that adds the comma to the last entry in the enum, followed by another patch that adds the new variants, making sure to include a comma after the last new entry, and putting the two comments on separate lines instead of together with actual values, so that in the future no lines will need to change unless there is an actual code change. Changing existing lines creates unneccessary noise in the source code history and is important to avoid. > static int fm3_erase(struct flash_bank *bank, int first, int last) > { > + struct fm3_flash_bank *fm3_info = bank->driver_priv; > struct target *target = bank->target; > int retval = ERROR_OK; > uint32_t u32DummyRead; > int sector, odd; > - > + uint32_t u32FlashType; > + uint32_t u32FlashSeqAddress1; > + uint32_t u32FlashSeqAddress2; > + > + u32FlashType = (uint32_t) fm3_info->flashtype; > + > + if (u32FlashType == fm3_flash_type1) > + { > + u32FlashSeqAddress1 = 0x00001550; > + u32FlashSeqAddress2 = 0x00000AA8; > + } > + else if (u32FlashType == fm3_flash_type2) > + { > + u32FlashSeqAddress1 = 0x00000AA8; > + u32FlashSeqAddress2 = 0x00000554; > + } > + else > + { > + LOG_ERROR("Flash/Device type unknown!"); > + return ERROR_FLASH_OPERATION_FAILED; > + } The above could benefit from using switch() rather than if/else if. > - for (sector = first ; sector <= last ; sector++) { > + for (sector = first ; sector <= last ; sector++) > + { > uint32_t offset = bank->sectors[sector].offset; > > - for (odd = 0; odd < 2 ; odd++) { > - > + for (odd = 0; odd < 2 ; odd++) > + { .. > -static int fm3_write_block(struct flash_bank *bank, uint8_t *buffer, > uint32_t offset, uint32_t count) > +static int fm3_write_block(struct flash_bank *bank, uint8_t *buffer, > + uint32_t offset, uint32_t count) Please make code style changes in a separate patch, since it does not have any impact on actual code. > { > struct fm3_flash_bank *fm3_info = bank->driver_priv; > struct target *target = bank->target; > - uint32_t buffer_size = 8192; > + uint32_t buffer_size = 2048; // 8192 for MB9Bxx6! I don't see the patch setting 8192, but maybe I overlooked it. Anyway it might be better to not set a default value here and assign the correct values in a switch(), so that the compiler can catch any cases in the future where the driver is not assigning a value to this variable. > + u32FlashType = (uint32_t) fm3_info->flashtype; > + > + if (u32FlashType == fm3_flash_type1) > + { > + u32FlashSeqAddress1 = 0x00001550; > + u32FlashSeqAddress2 = 0x00000AA8; > + } > + else if (u32FlashType == fm3_flash_type2) > + { > + u32FlashSeqAddress1 = 0x00000AA8; > + u32FlashSeqAddress2 = 0x00000554; > + } > + else > + { > + LOG_ERROR("Flash/Device type unknown!"); > + return ERROR_FLASH_OPERATION_FAILED; > + } Why the extra variable, and please use a switch() here too. > - const uint8_t fm3_flash_write_code[] = { > - /* > fm3_FLASH_IF->FASZ &= 0xFFFD; */ > - 0x00, 0xBF, /* NOP > */ > - 0x5F, 0xF0, 0x80, 0x43, /* MOVS.W R3, > #(fm3_FLASH_IF->FASZ) */ > - 0x1B, 0x68, /* LDR R3, [R3] > */ > - 0x4F, 0xF6, 0xFD, 0x74, /* MOVW R4, #0xFFFD > */ .. > + const uint8_t fm3_flash_write_code[] = { > + /* > fm3_FLASH_IF->FASZ &= 0xFFFD; */ > + 0x5F, 0xF0, 0x80, 0x45, /* MOVS.W R5, > #(fm3_FLASH_IF->FASZ) */ > + 0x2D, 0x68, /* LDR R5, [R5] > */ > + 0x4F, 0xF6, 0xFD, 0x76, /* MOVW R6, #0xFFFD > */ You are changing the complete flash_write_code[] array even though parts of it are not actually changing. This creates a lot of noise in the file history, which is important and helpful to avoid. Could you please undo all whitespace changes, so that only the lines will be changed that actually have different code? > - LOG_WARNING("offset 0x%" PRIx32 " breaks required 2-byte > alignment", offset); > + LOG_WARNING("offset 0x%" PRIx32 " breaks required 2-byte > alignment", > + offset); ..and avoid whitespace changes mixed with actual code changes. > @@ -348,6 +530,8 @@ static int fm3_write_block(struct flash_bank *bank, > uint8_t *buffer, uint32_t of > if (retval != ERROR_OK) > return retval; > > + > + > /* memory buffer */ Are the added blank lines intentional? Please keep whitespace changes separate. > @@ -360,7 +544,8 @@ static int fm3_write_block(struct flash_bank *bank, > uint8_t *buffer, uint32_t of > target_free_working_area(target, > fm3_info->write_algorithm); > } > > - LOG_WARNING("no large enough working area available, > can't do block memory writes"); > + LOG_WARNING("No large enough working area available, > can't do \ > + block memory writes"); Nice catch! I personally have nothing against longer lines, but maybe there is another policy in effect. > @@ -371,57 +556,75 @@ static int fm3_write_block(struct flash_bank *bank, > uint8_t *buffer, uint32_t of .. > while (count > 0) > { > - uint32_t thisrun_count = (count > (buffer_size / 2)) ? > (buffer_size / 2) : count; > - > - /* for some reason the first 8 byte of code are corrupt when > target_run_algorithm() returns */ > - /* need some more investigation on this > */ > - retval = target_write_buffer(target, > - fm3_info->write_algorithm->address, 8, > fm3_flash_write_code); > + uint32_t thisrun_count = (count > (buffer_size / 2)) > + ? (buffer_size / 2) : count; Another case of no actual code change but lines changing anyway. Please always avoid this. > - retval = target_run_algorithm(target, 0, NULL, 4, reg_params, > + retval = target_run_algorithm(target, 0, NULL, 6, reg_params, Perhaps this could be ARRAY_SIZE(reg_params) + 1, to avoid unneccessary future changes and human errors. > @@ -471,7 +676,9 @@ static int fm3_probe(struct flash_bank *bank) > bank->sectors[1].is_erased = -1; > bank->sectors[1].is_protected = -1; > > - if (fm3_info->variant == mb9bfxx1) > + if ( (fm3_info->variant == mb9bfxx1) > + || (fm3_info->variant == mb9afxx1) > + ) > { > num_pages = 3; > bank->size = 64 * 1024; /* bytes */ > @@ -483,10 +690,15 @@ static int fm3_probe(struct flash_bank *bank) > bank->sectors[2].is_protected = -1; > } > > - if ( (fm3_info->variant == mb9bfxx2) > + if ( (fm3_info->variant == mb9bfxx2) > || (fm3_info->variant == mb9bfxx4) > || (fm3_info->variant == mb9bfxx5) > - || (fm3_info->variant == mb9bfxx6)) > + || (fm3_info->variant == mb9bfxx6) > + || (fm3_info->variant == mb9afxx2) > + || (fm3_info->variant == mb9afxx4) > + || (fm3_info->variant == mb9afxx5) > + || (fm3_info->variant == mb9afxx6) > + ) > { > num_pages = 3; > bank->size = 128 * 1024; // bytes > @@ -498,9 +710,13 @@ static int fm3_probe(struct flash_bank *bank) > bank->sectors[2].is_protected = -1; > } > > - if ( (fm3_info->variant == mb9bfxx4) > + if ( (fm3_info->variant == mb9bfxx4) > || (fm3_info->variant == mb9bfxx5) > - || (fm3_info->variant == mb9bfxx6)) > + || (fm3_info->variant == mb9bfxx6) > + || (fm3_info->variant == mb9afxx4) > + || (fm3_info->variant == mb9afxx5) > + || (fm3_info->variant == mb9afxx6) > + ) I'd totally make this into a switch(). > +/* Chip erase */ > static int fm3_chip_erase(struct flash_bank *bank) That comment is really redundant. > + u32FlashType = (uint32_t) fm3_info->flashtype; > + > + if (u32FlashType == fm3_flash_type1) > + { > + LOG_INFO("*** Erasing mb9bfxxx type"); > + u32FlashSeqAddress1 = 0x00001550; > + u32FlashSeqAddress2 = 0x00000AA8; > + } > + else if (u32FlashType == fm3_flash_type2) > + { > + LOG_INFO("*** Erasing mb9afxxx type"); > + u32FlashSeqAddress1 = 0x00000AA8; > + u32FlashSeqAddress2 = 0x00000554; > + } > + else > + { > + LOG_ERROR("Flash/Device type unknown!"); > + return ERROR_FLASH_OPERATION_FAILED; > + } Another switch() I think.. The patch obviously adds good functionality, please do also add the simple style and quality improvements. Thanks! //Peter _______________________________________________ Openocd-development mailing list [email protected] https://lists.berlios.de/mailman/listinfo/openocd-development
