How similar are the OnCE implementations for various FreeScale chips?
I get the impression that the answer is "not very". But if that's not
true, then I wonder how much OnCE stuff could be shared between drivers
for different chips. Once a few more OnCE-equipped chips merge into
the tree, that could matter ... assuming it happens!
Also, how does this stack up feature-wise to the FreeScale code?
We've recently found that FreeScale is shipping a modified version
of OpenOCD ... which would be good, if they weren't violating the
GPL to do so (booo!) by refusing to distribute their modifications.
On Saturday 05 December 2009, Mathias K. wrote:
> +int dsp563xx_read_memory_p(struct target *target, uint32_t address, uint32_t
> size, uint32_t count,
> uint8_t *buffer);
> +int dsp563xx_write_memory_p(struct target *target, uint32_t address,
> uint32_t size, uint32_t count,
> uint8_t *buffer);
> +
Please resubmit this one with lines sanely wrapped (before
column 80) in the original patch ... and thus, not in
your email! :)
And for that matter, you shouldn't need most of these
forward declarations; just move the target_type struct
to the end of the file.
Plus, don't export those functions needlessly ... those
should evidently be static, or they should have been
in one of the headers.
> +struct dsp563xx_core_reg
> dsp563xx_core_reg_list_arch_info[DSP563XX_NUMCOREREGS] =
Wouldn't it be more robust to use an unsized array here, and
then use ARRAY_SIZE(dsp563xx_core_reg_list_arch_info) later?
And this should be "static const" ... no need to export the
table elsewhere, or make it writable.
> + {0, 24, ASM_REG_R_R0, NULL, NULL},
And rather than have parallel arrays for the register names
and this initializer stuff ... it *would* be better to have
a single array of register template data, ensuring the two
arrays can't get out of sync. And not having those NULL
pointers would save space.
> + /* get pointers to arch-specific information */
> + struct dsp563xx_common *dsp563xx = target->arch_info;
Please wrap this in a target_to_dsp563xx() macro ... and
I'd like to see the "_common" suffix die too.
> +/*
> + .get_gdb_reg_list = dsp563xx_get_gdb_reg_list,
What's the story on GCC and GDB support for these processors?
I'm suspecting that if there is no GDB support, we may be missing
a clean way to prevent opening a GDB connection for this CPU...
> +
> + .write_memory = dsp563xx_write_memory,
> + .bulk_write_memory = dsp563xx_bulk_write_memory,
> + .checksum_memory = dsp563xx_checksum_memory,
> + .blank_check_memory = dsp563xx_blank_check_memory,
> +
> + .run_algorithm = dsp563xx_run_algorithm,
> +
> + .add_breakpoint = dsp563xx_add_breakpoint,
> + .remove_breakpoint = dsp563xx_remove_breakpoint,
> + .add_watchpoint = dsp563xx_add_watchpoint,
> + .remove_watchpoint = dsp563xx_remove_watchpoint,
> +*/
Don't add commented-out code like that.
... that's all the patch-review time I have just now. Glad
to see support for another processor! Even though, as with
almost all initial patches, it still needs some work. ;)
- Dave
_______________________________________________
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
