[ offtopic threads #2, #3, ... ]
> The reason why I did not plug arm11 into the existing arm
> infrastructure and why we implemented etm as a tcl script
"We" didn't see any such Tcl scripts posted though.
"We" want to see ETM support that doesn't need to be
rewritten from scratch for each new core, too...
> instead of
> using etm.c is because, beside being mostly undocumented, most of the
> openocd code in these areas is a programming hazard full of endless
> repetitive code that obscures the underlying algorithms. Reverse
> engineering what these functions do just costs too much time.
On the other hand, from a project-wide perspective having
one part ("arm11") not even *trying* to reuse code is bad.
When there are code quality issues, the solution is to fix
them ... not to create something off on the side, which is
primarily just different and not compellingly better.
Plus, someone used to an ARM9 should be able to switch to an
ARM11 platform without needing to learn a whole new approach
or scratching their head about why something doesn't work
as fast any more (the memory things) or isn't even present
(disassembly, show "all" registers, etc).
One of the benefits of having OpenOCD handle multiple
JTAG targets is supposed to be that generalizing tends to
improve quality. Going off in a different direction makes
such shared improvements impossible.
> Take for example:
>
> fields[0].tap = etm_reg->jtag_info->tap;
> fields[0].num_bits = 32;
> fields[0].out_value = reg->value;
> fields[0].in_value = NULL;
> fields[0].check_value = NULL;
> fields[0].check_mask = NULL;
Note that if fields[] were zero-initialized, the size of
these would be halved: no NULL assignments.
> fields[1].tap = etm_reg->jtag_info->tap;
> fields[1].num_bits = 7;
> fields[1].out_value = malloc(1);
> buf_set_u32(fields[1].out_value, 0, 7, reg_addr);
> fields[1].in_value = NULL;
> fields[1].check_value = NULL;
> fields[1].check_mask = NULL;
>
> fields[2].tap = etm_reg->jtag_info->tap;
> fields[2].num_bits = 1;
> fields[2].out_value = malloc(1);
> buf_set_u32(fields[2].out_value, 0, 0, 0);
> fields[2].in_value = NULL;
> fields[2].check_value = NULL;
> fields[2].check_mask = NULL;
>
> This piece of digital diarrhea
The entire JTAG scan field interface suffers from the same
malady!!! So I don't think you have grounds to specifically
criticize *one* of the many modules that suffer from it.
Example, it'd have made sense to have routines packaged which
read and write registers using that 32 + 7 + 1 bit DR scan
model, since it's shared by EmbeddedICE and older ETMs. (Plus
maybe a bit more, but I'm sure of those.) That should be a
simple bit of re-factoring, yes?
> is no doubt "standard and universally
> understood" C code. Unfortunately it is also an obstacle when someone
> tries to read the surrounding algorithm. If that block would say:
>
> OUT_FIELD(fields[0], 32, reg->value);
> OUT_FIELD(fields[1], 7, reg_addr);
> OUT_FIELD(fields[2], 1, 0);
>
> it would be macros you might have to look up once when you deal with
> openocd for the first time, but that is a tiny price to pay for
> immediately seeing the relevant parts of the action.
Fair enough. I'd have made it a function, possibly an inlined one.
Note that there's an issue with host vs target bit ordering...
When you have an idea like that, the best approach is to discuss
it and then provide patches phasing it in over the *WHOLE* tree.
Not solve it for one little part of the tree, and ignore the rest.
> Btw. I added a small typo to the long version above that would not be
> detected by the compiler and is difficult to make with the macro
> version. Do you think someone would spot that when checking this in as
> a patch?
Depends on the level of checking. But on the other hand, when you
submit a buggy patch, that's basically your problem. One that's
addressable by testing. And fixable by a followup patch. (Fixed
before the bug gets released, one hopes...)
> And if it is not a typo but an obscure special case for a
> given CPU, would you, when inheriting that code for another CPU,
> notice a small difference that is buried under that pile of repetitive
> code that you skim over at best?
This is why I tell developers to provide comments. Specifically,
when something works around hardware bugs, reference those docs
(where available, else at least comment) to reduce such problems.
I'm sure we've all had problems with flakey code someone else
wrote. There's not a lot we can do about it, other than try
to minimize such problems in code we hand to someone else.
_______________________________________________
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
