Hi Mike,
Thank you again.
On Wed, Sep 26, 2012 at 10:03 PM, Mike Frysinger <vap...@gentoo.org> wrote:
> On Monday 24 September 2012 16:22:26 Jie Zhang wrote:
>> On Mon, Sep 24, 2012 at 12:38 AM, Mike Frysinger wrote:
>> > On Sunday 23 September 2012 22:43:16 Jie Zhang wrote:
>> >> This patch adds Analog Devices BF609 support to UrJTAG. BF609 has an
>> >> SDU (System Debug Unit), which does not exist in previous Blackfin
>> >> processors. This patch treats SDU as a new part type.
>> >>
>> >> Because the special design of TAPs in BF609 processors, new functions
>> >> are added to manually add/remove Blackfin core TAPs in/from scan
>> >> chain, like urj_part_parts_add_part_at and
>> >> urj_part_parts_remove_part_at in src/part/part.c.
>> >>
>> >> This patch also add a bus driver for BF609 EZ-BOARD.
>> >
>> > the sdu.h header needs a comment block at the top like other headers. it
>> > probably should also be named "bfin-sdu.h" since it's entirely
>> > ADI-specific. similarly, i'd move src/sdu/ to src/bfin/sdu/ and rename
>> > "sdu" in user visible strings to "bfin-sdu" or something.
>>
>> Although SDU is a ADI-specific thing, it's not a Blackfin specific
>> thing. Putting sdu/ under bfin/ is not correct. There is still some
>> code mixed between sdu and bfin, but I'm going to separate them
>> further in future.
>
> ok, but they still don't belong directly under src/ as "sdu" is too generic a
> name. maybe we should start a src/cpu/ dir and put both bfin & sdu inside of
> there ? similarly, start a cpu/ subdir in the include tree.
>
I agree, although cpu is not a good name since sdu is not a cpu. I'd
like call it emu or emulation. But I don't want to do it with this
patch. I would like to do it after this patch but before adding
another one.
>> > the signals in data/analog/sdu/sdu (and lack there of in
>> > data/analog/bf609/bf609) make me suspicious. shouldn't the pins be wired
>> > up to the bf609 cores ? or is everything routed through the sdu ? if
>> > the latter, then there should be a "sdu-bf609" file with the common sdu
>> > stuff remaining in the "sdu" file. also, how come bf609/bf609 isn't
>> > including bfin/bfin like all the other blackfin parts ?
>>
>> Everything is routed through the sdu. But I'd rather do that change
>> when there is a second processor using sdu. If there is no more such
>> processors, I can save a little time.
>> ...
>> Since I'm not sure if there
>> will be more processors using sdu in future, I don't bother to put
>> more effort into it.
>
> well, then separating out the sdu/ and bfin/ dirs doesn't make much sense when
> the current sdu data files are so Blackfin-centric :). at least on the data
> side, you could call it SDU609 as that should be a quick change.
>
It makes sense since sdu and bfin are separate logically. Each one can
exist without the other. When there is another processor using sdu, we
can find a naming solution at that time. For now, it should be OK to
just call it sdu.
>> Because BF609 part does not have IDCODE instruction or DIR register,
>> including common bfin file is not correct.
>
> well, i think every part on that jtag bus has to have an IDCODE. that it
> returns all 1's means it doesn't have an idcode assigned to it. as for the
> DIR register, i don't think having it defined is a problem is it ? it might
> be
> a little confusing ...
>
IDCODE is optional. I think being correct is more important. If it
does not have IDCODE, just don't define it. If you really want to
share things, we can create a bfin-no-idcode file which does not
contain the IDCODE, then let bfin include it + IDCODE. But I wonder it
will be worth to do it.
>> +static struct URJ_PART_DATA_COMMON urj_unknown_part_data_initializer =
>
> i think you can make this const
>
Actually all initializers can be const. Will do it in a follow-up patch.
>> +#define URJ_UNKNOWN_PART_DATA(part) ((struct URJ_PART_DATA_COMMON *)((part)-
>>params->data))
>
> i would just call this URJ_PART_DATA()
>
Will do.
>> +static void
>> +urj_unknown_part_init (urj_part_t *part)
>> +{
>> + if (!part || !part->params)
>> + {
>> + urj_error_set (URJ_ERROR_INVALID, "initialization failed for
> unknown part");
>
> needs _(...)
>
Will do.
>> + return;
>> +}
>
> pointless return here
>
Will remove it.
>> +void
>> +urj_part_init (urj_part_t *part)
>> +{
>> + urj_part_init_func_t part_init_func;
>> +
>> + part->params = malloc (sizeof (urj_part_params_t));
>> + part_init_func = urj_part_find_init (part->part);
>> + if (part_init_func)
>> + (*part_init_func) (part);
>> + else
>> + urj_unknown_part_init (part);
>> +}
>
> what if urj_part_find_init just return urj_unknown_part_init when it couldn't
> find one ?
>
Will do.
>> +#define PART_MAGIC(part) (((struct URJ_PART_DATA_COMMON *)((part)-
>>params->data))->magic)
>> +#define PART_BYPASS(part) (((struct URJ_PART_DATA_COMMON *)((part)-
>>params->data))->bypass)
>> +#define PART_SCAN(part) (((struct URJ_PART_DATA_COMMON *)((part)-
>>params->data))->scan)
>
> you can make one macro to rule them all:
> #define PART_FIELD(part, field) \
> (((struct URJ_PART_DATA_COMMON *)((part)->params->data))->field)
>
Will do.
>> +int
>> +urj_part_is_bypassed (urj_part_t *part)
>
> bool urj_part_get_bypass (...)
>
> that keeps the form of the other get/set accessors API
>
I think urj_part_is_bypassed is more readable than urj_part_get_bypass. Compare
if (urj_part_is_bypassed (part))
with
if (urj_part_get_bypassed (part))
.
>> +uint32_t
>> +urj_part_get_magic (urj_part_t *part)
>> +{
>> + return PART_MAGIC(part);
>
> needs spaces before the paren
>
>> +int
>> +urj_part_get_scan (urj_part_t *part)
>> +{
>> + return PART_SCAN(part);
>
> here too
>
>> +void
>> +urj_part_set_scan (urj_part_t *part, int scan)
>> +{
>> + PART_SCAN(part) = scan;
>
> here too
Will fix all of three.
Jie
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://ad.doubleclick.net/clk;258768047;13503038;j?
http://info.appdynamics.com/FreeJavaPerformanceDownload.html
_______________________________________________
UrJTAG-development mailing list
UrJTAG-development@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/urjtag-development
