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.
> > 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.
> 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 ...
> +static struct URJ_PART_DATA_COMMON urj_unknown_part_data_initializer =
i think you can make this const
> +#define URJ_UNKNOWN_PART_DATA(part) ((struct URJ_PART_DATA_COMMON *)((part)-
>params->data))
i would just call this URJ_PART_DATA()
> +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 _(...)
> + return;
> +}
pointless return here
> +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 ?
> +#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)
> +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
> +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
-mike
signature.asc
Description: This is a digitally signed message part.
------------------------------------------------------------------------------ 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
