Message ID | 20200320101448.10714-1-rasmus.villemoes@prevas.dk |
---|---|
State | New |
Headers | show |
Series | [v2,1/2] spi: call WATCHDOG_RESET() in spi_nor_wait_till_ready_with_timeout() | expand |
Dear Vignesh, In message <20200320101448.10714-1-rasmus.villemoes at prevas.dk> Rasmus Villemoes wrote: > I have a board for which doing "sf erase 0x100000 0x80000" > consistently causes the external watchdog circuit to reset the > board. Make sure to pet the watchdog during slow operations such as > erasing or writing large areas of a spi nor flash. ... > drivers/mtd/spi/spi-nor-core.c | 2 ++ > drivers/mtd/spi/spi-nor-tiny.c | 2 ++ Rasmus' patch triggers a few interesting questions about the SPI NOR code: Is there any specific reason why spi-nor-core.c and spi-nor-tiny.c contain large parts of absolutely identical code? All the functions spi_nor_read_write_reg() spi_nor_read_reg() spi_nor_write_reg() spi_nor_read_data() mtd_to_spi_nor() spi_nor_convert_opcode() spi_nor_ready() spi_nor_wait_till_ready_with_timeout() spi_nor_wait_till_ready() macronix_quad_enable() spansion_read_cr_quad_enable() spi_nor_set_read_settings() are absolutely identical; functions read_cr() write_sr() write_disable() set_4byte() spi_nor_read() write_sr_cr() are mostly identical, but I wonder if the differences (like nor->write_reg() versus spi_nor_write_reg()) is indeed intended to save memory footprint or lack an update to later code? Function spi_nor_convert_3to4_read() spi_nor_set_4byte_opcodes() looks like it has not been updated/synced for some time. Function spi_nor_sr_ready() spi_nor_fsr_ready() is lacking error handling in spi-nor-tiny.c; and the rror handling code in spi-nor-core.c is also mostly duplicated a couple or times. Function spi_nor_read_id() differs in "interesting" ways, i. e. we have 370 info = spi_nor_ids; 371 for (; info->sector_size != 0; info++) { 372 if (info->id_len) { here, and 894 info = spi_nor_ids; 895 for (; info->name; info++) { 896 if (info->id_len) { there, while all the rest is idential. Lack of synchronization? Also the differences in spi_nor_select_read() make we wonder... This extensive code duplication looks really painful and error prone to me. Do you have any intentions to clean this up any time soon? Best regards, Wolfgang Denk
Dear Wolfgang, On 20/03/20 4:48 pm, Wolfgang Denk wrote: > Dear Vignesh, > > In message <20200320101448.10714-1-rasmus.villemoes at prevas.dk> Rasmus Villemoes wrote: >> I have a board for which doing "sf erase 0x100000 0x80000" >> consistently causes the external watchdog circuit to reset the >> board. Make sure to pet the watchdog during slow operations such as >> erasing or writing large areas of a spi nor flash. > ... > >> drivers/mtd/spi/spi-nor-core.c | 2 ++ >> drivers/mtd/spi/spi-nor-tiny.c | 2 ++ > > Rasmus' patch triggers a few interesting questions about the SPI NOR > code: > > > Is there any specific reason why spi-nor-core.c and spi-nor-tiny.c > contain large parts of absolutely identical code? > Aim of spi-nor-tiny.c is to have a tiny stack that can be used in SPL/TPL or on resource constraint boards to only support _reading_ from the flash. So tiny stack would be subset of spi-nor-core. There are few options here: One is to have single driver and hide things that are not required for tiny stack under #ifdef. But this makes code harder to read and modify Second option, is to factor out common functions into a separate file as a library. This would avoid ifdef'ry. But whenever a new feature is added that would add to the size of these common functions, it would be probably mean moving it out of common library and into individual stacks. This may need to unnecessary code churn whenever a new feature is added. So, suggestion was to add a parallel tiny stack (which was supposed to plug into tiny read only MTD stack) that only supports reading from flash. This would mean that new features can be freely added to spi-nor-core.c without worrying about bloating SPL for older devices. If the opinion is to switch to second option now, then I can rework the framework. But note that this would make adding new features bit harder due to need to maintain size of spi-nor-tiny.c. Please, let me know? > All the functions > > spi_nor_read_write_reg() > spi_nor_read_reg() > spi_nor_write_reg() > spi_nor_read_data() > mtd_to_spi_nor() > spi_nor_convert_opcode() > spi_nor_ready() > spi_nor_wait_till_ready_with_timeout() > spi_nor_wait_till_ready() > macronix_quad_enable() > spansion_read_cr_quad_enable() > spi_nor_set_read_settings() > > > are absolutely identical; > > functions > > read_cr() > write_sr() > write_disable() > set_4byte() > spi_nor_read() > write_sr_cr() > > are mostly identical, but I wonder if the differences (like > nor->write_reg() versus spi_nor_write_reg()) is indeed intended to > save memory footprint or lack an update to later code? > I am in the process of dropping nor->*() functions altogether as I don't see any users outside of spi-nor-core.c Note that some of these will no longer be same with 8D-8D-8D support[1] thus further reducing the similarities. [1] https://patchwork.ozlabs.org/project/uboot/list/?series=166175 > Function > > spi_nor_convert_3to4_read() > spi_nor_set_4byte_opcodes() > > looks like it has not been updated/synced for some time. Thats intentional... Adding Octal mode support to tiny stack would add to code size and possibly break few boards. Any addition to spi-nor-tiny.c should be debated whether or not that change is absolutely needed for low footprint boards. > > Function > > spi_nor_sr_ready() > spi_nor_fsr_ready() > > is lacking error handling in spi-nor-tiny.c; and the rror handling > code in spi-nor-core.c is also mostly duplicated a couple or times. > Error handling is not required, as tiny stack does not support writing to flash and these errors are raised when writing or erasing flash. > > Function > > spi_nor_read_id() > > differs in "interesting" ways, i. e. we have > > 370 info = spi_nor_ids; > 371 for (; info->sector_size != 0; info++) { > 372 if (info->id_len) { > > here, and In case of tiny stack, we save space by not storing flash names in spi_nor_ids[] table (its a significant saving) and hence have to rely on another field to detect EOL. > > 894 info = spi_nor_ids; > 895 for (; info->name; info++) { > 896 if (info->id_len) { > > there, while all the rest is idential. Lack of synchronization? > > > Also the differences in > > spi_nor_select_read() > > make we wonder... > > > > This extensive code duplication looks really painful and error prone > to me. Duplication is to avoid feature creep leading to increase in code size. But I can factor out common code if there is a wider agreement. > > Do you have any intentions to clean this up any time soon? > > Best regards, > > Wolfgang Denk >
Dear Vignesh, In message <05694b0e-50a1-de5d-25d8-0444a2caeff3 at ti.com> you wrote: > > Aim of spi-nor-tiny.c is to have a tiny stack that can be used in > SPL/TPL or on resource constraint boards to only support _reading_ from > the flash. So tiny stack would be subset of spi-nor-core. I fully understand this goal. > There are few options here: > One is to have single driver and hide things that are not required for > tiny stack under #ifdef. But this makes code harder to read and modify #ifdef is one way to implement conditioan code, plain C code like if (IS_ENABLED(CONFIG_<something)) { ... } is another, usually much cleaner. > Second option, is to factor out common functions into a separate file as > a library. This would avoid ifdef'ry. But whenever a new feature is > added that would add to the size of these common functions, it would be > probably mean moving it out of common library and into individual > stacks. This may need to unnecessary code churn whenever a new feature > is added. This is all speculation, and experience says that this risks and disadvantages of duplicated code are much higher. > So, suggestion was to add a parallel tiny stack (which was supposed to > plug into tiny read only MTD stack) that only supports reading from > flash. This would mean that new features can be freely added to > spi-nor-core.c without worrying about bloating SPL for older devices. Yes, and the result is that you have two different implementations that are out of sync from day one after you created them, bugs get fixed here but no there, support for new chips same, etc. > If the opinion is to switch to second option now, then I can rework the > framework. But note that this would make adding new features bit harder > due to need to maintain size of spi-nor-tiny.c. I agree that the reorganization will take additional efforts, but in the long term, maintenance efforts will be much smaller, as you have to maintain one common code base only. And if you add new features and see that they have negative impact on the SPL configurations, you can always encapsulate these parts in IS_ENABLED() code. > Please, let me know? Well, for large parts things are pretty easy: > > All the functions > > > > spi_nor_read_write_reg() > > spi_nor_read_reg() > > spi_nor_write_reg() > > spi_nor_read_data() > > mtd_to_spi_nor() > > spi_nor_convert_opcode() > > spi_nor_ready() > > spi_nor_wait_till_ready_with_timeout() > > spi_nor_wait_till_ready() > > macronix_quad_enable() > > spansion_read_cr_quad_enable() > > spi_nor_set_read_settings() > > > > > > are absolutely identical; For these functions there is absolutely no justification to have them duplicated. > > functions > > > > read_cr() > > write_sr() > > write_disable() > > set_4byte() > > spi_nor_read() > > write_sr_cr() > > > > are mostly identical, but I wonder if the differences (like > > nor->write_reg() versus spi_nor_write_reg()) is indeed intended to > > save memory footprint or lack an update to later code? > > I am in the process of dropping nor->*() functions altogether as I don't > see any users outside of spi-nor-core.c > > Note that some of these will no longer be same with 8D-8D-8D support[1] > thus further reducing the similarities. Well, maybe this rework should consider the idea of having common code both for normal and size-limited use cases? In the current form, the differences are so small they could easily be handled by a few macro definitions so the code would be indentical again. Maybe this is also possible in your rework? > > Function > > > > spi_nor_convert_3to4_read() > > spi_nor_set_4byte_opcodes() > > > > looks like it has not been updated/synced for some time. > > Thats intentional... Adding Octal mode support to tiny stack would add > to code size and possibly break few boards. OK - I did not look deeply enough into the code if it was just new features that will never be needed in the SPL, of if they might actually be needed, or if they were actually bug fixes. You are the expert here, so I trust your assessment. > Any addition to spi-nor-tiny.c should be debated whether or not that > change is absolutely needed for low footprint boards. Agreed. > > Function > > > > spi_nor_sr_ready() > > spi_nor_fsr_ready() > > > > is lacking error handling in spi-nor-tiny.c; and the rror handling > > code in spi-nor-core.c is also mostly duplicated a couple or times. > > Error handling is not required, as tiny stack does not support writing > to flash and these errors are raised when writing or erasing flash. These differences are tricial to handle using IS_ENABLED() for code parts that are needed only when erase/write support is configured. > > Function > > > > spi_nor_read_id() > > > > differs in "interesting" ways, i. e. we have > > > > 370 info = spi_nor_ids; > > 371 for (; info->sector_size != 0; info++) { > > 372 if (info->id_len) { > > > > here, and > > In case of tiny stack, we save space by not storing flash names in > spi_nor_ids[] table (its a significant saving) and hence have to rely on > another field to detect EOL. You could still use the same method in both implementations, right? > Duplication is to avoid feature creep leading to increase in code size. > But I can factor out common code if there is a wider agreement. Code duplication never evermakes sense to me. It is just a cause of errors and mental pain. I would really appreciate if youc ould clean this up. Thanks! Best regards, Wolfgang Denk
On 24/03/20 8:11 pm, Wolfgang Denk wrote: > Dear Vignesh, > > In message <05694b0e-50a1-de5d-25d8-0444a2caeff3 at ti.com> you wrote: >> >> Aim of spi-nor-tiny.c is to have a tiny stack that can be used in >> SPL/TPL or on resource constraint boards to only support _reading_ from >> the flash. So tiny stack would be subset of spi-nor-core. > > I fully understand this goal. > >> There are few options here: >> One is to have single driver and hide things that are not required for >> tiny stack under #ifdef. But this makes code harder to read and modify > > #ifdef is one way to implement conditioan code, plain C code like > > if (IS_ENABLED(CONFIG_<something)) { > ... > } > > is another, usually much cleaner. > >> Second option, is to factor out common functions into a separate file as >> a library. This would avoid ifdef'ry. But whenever a new feature is >> added that would add to the size of these common functions, it would be >> probably mean moving it out of common library and into individual >> stacks. This may need to unnecessary code churn whenever a new feature >> is added. > > This is all speculation, and experience says that this risks and > disadvantages of duplicated code are much higher. > >> So, suggestion was to add a parallel tiny stack (which was supposed to >> plug into tiny read only MTD stack) that only supports reading from >> flash. This would mean that new features can be freely added to >> spi-nor-core.c without worrying about bloating SPL for older devices. > > Yes, and the result is that you have two different implementations > that are out of sync from day one after you created them, bugs get > fixed here but no there, support for new chips same, etc. > I fully understand your concerns and will work on unifying the two stacks with IS_ENABLED() macro so that there will still be a tiny stack with same memory footprint. But I want to state that the differences currently in spi-nor-tiny.c vs spi-nor-core.c are intentional. I don't think there have been any fixes in the main code missing from tiny stack. Features such as Octal mode and other stuff have been intentionally kept out of spi-nor-tiny to avoid code size increase. Appreciate all the suggestions! Regards Vignesh [...]
Dear Vignesh, In message <9a1e75ac-135a-26aa-2ded-784fbe14bc14 at ti.com> you wrote: > > I fully understand your concerns and will work on unifying the two > stacks with IS_ENABLED() macro so that there will still be a tiny stack > with same memory footprint. Thanks!! > But I want to state that the differences currently in spi-nor-tiny.c vs > spi-nor-core.c are intentional. I don't think there have been any fixes > in the main code missing from tiny stack. > Features such as Octal mode and other stuff have been intentionally kept > out of spi-nor-tiny to avoid code size increase. Understood. Best regards, Wolfgang Denk
On 20/03/2020 11.14, Rasmus Villemoes wrote: > I have a board for which doing "sf erase 0x100000 0x80000" > consistently causes the external watchdog circuit to reset the > board. Make sure to pet the watchdog during slow operations such as > erasing or writing large areas of a spi nor flash. Ping.
On 20/05/2020 00.12, Rasmus Villemoes wrote: > On 20/03/2020 11.14, Rasmus Villemoes wrote: >> I have a board for which doing "sf erase 0x100000 0x80000" >> consistently causes the external watchdog circuit to reset the >> board. Make sure to pet the watchdog during slow operations such as >> erasing or writing large areas of a spi nor flash. > > Ping. > Ping^2.
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 7b6ad495ac..c5d98debf0 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -22,6 +22,7 @@ #include <linux/mtd/spi-nor.h> #include <spi-mem.h> #include <spi.h> +#include <watchdog.h> #include "sf_internal.h" @@ -424,6 +425,7 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor, unsigned long timebase; int ret; + WATCHDOG_RESET(); timebase = get_timer(0); while (get_timer(timebase) < timeout) { diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c index ccc0ab07af..d91989567d 100644 --- a/drivers/mtd/spi/spi-nor-tiny.c +++ b/drivers/mtd/spi/spi-nor-tiny.c @@ -21,6 +21,7 @@ #include <linux/mtd/spi-nor.h> #include <spi-mem.h> #include <spi.h> +#include <watchdog.h> #include "sf_internal.h" @@ -324,6 +325,7 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor, unsigned long timebase; int ret; + WATCHDOG_RESET(); timebase = get_timer(0); while (get_timer(timebase) < timeout) {
I have a board for which doing "sf erase 0x100000 0x80000" consistently causes the external watchdog circuit to reset the board. Make sure to pet the watchdog during slow operations such as erasing or writing large areas of a spi nor flash. Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk> --- drivers/mtd/spi/spi-nor-core.c | 2 ++ drivers/mtd/spi/spi-nor-tiny.c | 2 ++ 2 files changed, 4 insertions(+)