Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-07 Thread Øyvind Harboe
On Sun, Dec 6, 2009 at 2:51 PM, Øyvind Harboe oyvind.har...@zylin.com wrote:
     1. Move the $(srcdir) from defining _DIR vars to their use. i.e.
        set just the directory component.

 I'm not quite sure what you mean by this. I have a makefile rule
 that is only used with the minidriver, would you like that to be
 defined always? I'm not even sure that would work when there
 are definitions missing...

 The oharboe/fixminidriver code is shaping up, but I still need to
 run tests.

oharboe/fixminidriver now builds with:

Linux:

a) --enable-dummy
b) --enable-minidriver-dummy
c) zy1000

Cygwin:

a) --enable-dummy
b) --enable-minidriver-dummy


Some problems:

make clean does not remove jtag_minidriver.h and minidriver_impl.h.


-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-07 Thread Zach Welch
On Mon, 2009-12-07 at 12:26 +0100, Øyvind Harboe wrote:
 On Sun, Dec 6, 2009 at 2:51 PM, Øyvind Harboe oyvind.har...@zylin.com wrote:
  1. Move the $(srcdir) from defining _DIR vars to their use. i.e.
 set just the directory component.
 
  I'm not quite sure what you mean by this. I have a makefile rule
  that is only used with the minidriver, would you like that to be
  defined always? I'm not even sure that would work when there
  are definitions missing...
 
  The oharboe/fixminidriver code is shaping up, but I still need to
  run tests.
 
 oharboe/fixminidriver now builds with:
 
 Linux:
 
 a) --enable-dummy
 b) --enable-minidriver-dummy
 c) zy1000
 
 Cygwin:
 
 a) --enable-dummy
 b) --enable-minidriver-dummy
 
 
 Some problems:
 
 make clean does not remove jtag_minidriver.h and minidriver_impl.h.

And if you read _all_ of the instructions I gave you, this would not be
a problem.

When you think they are done, post these patches in a new thread.

--Z
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-06 Thread Øyvind Harboe
     1. Move the $(srcdir) from defining _DIR vars to their use. i.e.
        set just the directory component.

I'm not quite sure what you mean by this. I have a makefile rule
that is only used with the minidriver, would you like that to be
defined always? I'm not even sure that would work when there
are definitions missing...

The oharboe/fixminidriver code is shaping up, but I still need to
run tests.

-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-05 Thread Zach Welch
On Sat, 2009-12-05 at 08:25 +0100, Øyvind Harboe wrote:
 This worked pretty well, but there are lots of (to me) inexplicable
 -I's on the command line still and none of them point to the right
 place in the build tree. I'm testing minidummy build != src dir.
 
 oharboe/fixminidriver2
 
 ../openocd/configure --enable-minidriver-dummy --enable-maintainer-mode
 
 make
 
 libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I.
 -I../../../zy1000/openocd/src/jtag -I../..
 -I../../../zy1000/openocd/src
 -I../../../zy1000/openocd/src/jtag/minidriver -g -O2 -Wall
 -Wstrict-prototypes -Wformat-security -Wextra -Wno-unused-parameter
 -Wbad-function-cast -Wcast-align -Wredundant-decls -Werror -MT core.lo
 -MD -MP -MF .deps/core.Tpo -c ../../../zy1000/openocd/src/jtag/core.c
 -o core.o
 In file included from ../../../zy1000/openocd/src/jtag/jtag.h:729,
  from ../../../zy1000/openocd/src/jtag/core.c:34:
 ../../../zy1000/openocd/src/jtag/minidriver.h:50:33: error:
 jtag/minidriver_imp.h: No such file or directory
 cc1: warnings being treated as errors

At the least, the -I$(srcdir)/jtag/minidriver should be gone, and I'd
guess you didn't add the two header names to BUILT_SOURCES so their
rules get called before everything else.

At this point, I need to see a new series, so please mirror or post
them.  I don't know what you've done to tell you how to fix it. :)

--Z

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-05 Thread Øyvind Harboe
On Sat, Dec 5, 2009 at 8:45 AM, Zach Welch z...@superlucidity.net wrote:
 On Sat, 2009-12-05 at 08:35 +0100, Øyvind Harboe wrote:
 The approach seems to work fine, but requires that some other
 issues are sorted out first with include dirs.

 From error message above, there are two strange -I additions:

 NB! build != src dir

 -I. = current build directory
 -I../.. = this is the root build dir, so I would have to include
 include src/jtag/minidriver_imp.h. Was this supposed to be
 the build/src dir instead?

 I don't know where the two above are defined, but at the top of each
 Makefile.am there is a

 AM_CPPFLAGS = \
       -I$(top_srcdir)/src

 Should this include the build/src dir as well?



 You got it.  We need to add -I$(top_builddir)/src to all directories
 now that we are talking about generating them on the fly.

 Do that in a separate patch please.  :)

Seems to build now (oharboe/fixminidriver3) I need to clean up
and run tests, but it should be plain sailing from here on.

-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-05 Thread Zach Welch
On Sat, 2009-12-05 at 11:14 +0100, Øyvind Harboe wrote:
 On Sat, Dec 5, 2009 at 8:45 AM, Zach Welch z...@superlucidity.net wrote:
  On Sat, 2009-12-05 at 08:35 +0100, Øyvind Harboe wrote:
  The approach seems to work fine, but requires that some other
  issues are sorted out first with include dirs.
 
  From error message above, there are two strange -I additions:
 
  NB! build != src dir
 
  -I. = current build directory
  -I../.. = this is the root build dir, so I would have to include
  include src/jtag/minidriver_imp.h. Was this supposed to be
  the build/src dir instead?
 
  I don't know where the two above are defined, but at the top of each
  Makefile.am there is a
 
  AM_CPPFLAGS = \
-I$(top_srcdir)/src
 
  Should this include the build/src dir as well?
 
 
 
  You got it.  We need to add -I$(top_builddir)/src to all directories
  now that we are talking about generating them on the fly.
 
  Do that in a separate patch please.  :)
 
 Seems to build now (oharboe/fixminidriver3) I need to clean up
 and run tests, but it should be plain sailing from here on.


Patch #1:
 1. use continuations '\' to make the files more patch-friendly. 

Patch #3:
 1. Move the $(srcdir) from defining _DIR vars to their use. i.e.
set just the directory component.
 2. Set BUILD_SOURCES as empty and use += in both branches for
symmetry.
 3. ZY1000_POKE does not need to be here (and #if 0?!? never!!!)

Cheers,

--Z
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-04 Thread Øyvind Harboe
It's getting there, see oharboe/fixminidriver.

I had to mess with lots of Makefile.am to pollute the include path. If I
can get that out of the way, then I should be able to clean up the
rest too.




-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-04 Thread Zach Welch
On Fri, 2009-12-04 at 16:03 +0100, Øyvind Harboe wrote:
 It's getting there, see oharboe/fixminidriver.
 
 I had to mess with lots of Makefile.am to pollute the include path. If I
 can get that out of the way, then I should be able to clean up the
 rest too

Yeah, those changes should not go into the tree as-is.  They make a
total mess of the build system.

In your second patch, the drivers/minidriver_imp.h patch is close, but a
little buggy.  You #define and declare the same function; the declared
version should be the _imp version.

Seeing your new patch gives me a new idea for how to fix things:
1) In jtag/Makefile, use BUILT_SOURCES to copy the proper
minidriver_imp.h (and jtag_minidriver.h, if needed) into the jtag
directory.
2) #include jtag/minidriver_imp.h and jtag/jtag_minidriver_h.

Will that work?  It should, allowing to kill your first patch too.

--Z
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-04 Thread Øyvind Harboe
On Fri, Dec 4, 2009 at 10:36 PM, Zach Welch z...@superlucidity.net wrote:
 On Fri, 2009-12-04 at 16:03 +0100, Øyvind Harboe wrote:
 It's getting there, see oharboe/fixminidriver.

 I had to mess with lots of Makefile.am to pollute the include path. If I
 can get that out of the way, then I should be able to clean up the
 rest too

 Yeah, those changes should not go into the tree as-is.  They make a
 total mess of the build system.

Yeah, I had to put in a kludge in the whole 15 minutes I had between
routing FPGAs to test the rest :-)

 In your second patch, the drivers/minidriver_imp.h patch is close, but a
 little buggy.  You #define and declare the same function; the declared
 version should be the _imp version.

I think I can simply do away with the _imp and the #define altogether.
More straightforward.

 Seeing your new patch gives me a new idea for how to fix things:
 1) In jtag/Makefile, use BUILT_SOURCES to copy the proper
 minidriver_imp.h (and jtag_minidriver.h, if needed) into the jtag
 directory.

Sounds good to me. I don't know the autotools speak to do that.
I'll give it a go.

 2) #include jtag/minidriver_imp.h and jtag/jtag_minidriver_h.

minidriver_imp.h includes jtag_minidriver.h.

 Will that work?  It should, allowing to kill your first patch too.

Should do. Some trivial Makefile.am is the biggest hurdle
for me, my autotools skills are pretty limited.



-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-04 Thread Zach Welch
On Fri, 2009-12-04 at 22:51 +0100, Øyvind Harboe wrote:
 On Fri, Dec 4, 2009 at 10:36 PM, Zach Welch z...@superlucidity.net wrote:
  On Fri, 2009-12-04 at 16:03 +0100, Øyvind Harboe wrote:
  It's getting there, see oharboe/fixminidriver.
 
  I had to mess with lots of Makefile.am to pollute the include path. If I
  can get that out of the way, then I should be able to clean up the
  rest too
 
  Yeah, those changes should not go into the tree as-is.  They make a
  total mess of the build system.
 
 Yeah, I had to put in a kludge in the whole 15 minutes I had between
 routing FPGAs to test the rest :-)
 
  In your second patch, the drivers/minidriver_imp.h patch is close, but a
  little buggy.  You #define and declare the same function; the declared
  version should be the _imp version.
 
 I think I can simply do away with the _imp and the #define altogether.
 More straightforward.

Minimal patches.  Whatever that turns out to be. :)

  Seeing your new patch gives me a new idea for how to fix things:
  1) In jtag/Makefile, use BUILT_SOURCES to copy the proper
  minidriver_imp.h (and jtag_minidriver.h, if needed) into the jtag
  directory.
 
 Sounds good to me. I don't know the autotools speak to do that.
 I'll give it a go.

I would have to stab at it several times myself, I'm sure.
At the core, I would expect the rules to look something like:

BUILT_SOURCES += minidriver_imp.h jtag_minidriver.h

minidriver_imp.h: $(MINIDRIVER_IMP_DIR)/minidriver_imp.h
cp $ $@

jtag_minidriver.h: $(JTAG_MINIDRIVER_DIR)/jtag_minidriver.h
cp $ $@

The two _DIR variables get set inside the automake logic.  The values
will be 'drivers' and 'minidriver' for the first, and 'minidummy' and
'zy1000' for the second.   So, you're just copying the two files from
the source directories (rather than -I'ing those directories).
You'll also need to add those files to the CLEANFILES rule.

  2) #include jtag/minidriver_imp.h and jtag/jtag_minidriver_h.
 
 minidriver_imp.h includes jtag_minidriver.h.

Right, but both should use bracketed paths... not foo.h... This should
obviate the rules added by your first patch.

--Z
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-04 Thread Øyvind Harboe
This worked pretty well, but there are lots of (to me) inexplicable
-I's on the command line still and none of them point to the right
place in the build tree. I'm testing minidummy build != src dir.

oharboe/fixminidriver2

../openocd/configure --enable-minidriver-dummy --enable-maintainer-mode

make

libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I.
-I../../../zy1000/openocd/src/jtag -I../..
-I../../../zy1000/openocd/src
-I../../../zy1000/openocd/src/jtag/minidriver -g -O2 -Wall
-Wstrict-prototypes -Wformat-security -Wextra -Wno-unused-parameter
-Wbad-function-cast -Wcast-align -Wredundant-decls -Werror -MT core.lo
-MD -MP -MF .deps/core.Tpo -c ../../../zy1000/openocd/src/jtag/core.c
-o core.o
In file included from ../../../zy1000/openocd/src/jtag/jtag.h:729,
 from ../../../zy1000/openocd/src/jtag/core.c:34:
../../../zy1000/openocd/src/jtag/minidriver.h:50:33: error:
jtag/minidriver_imp.h: No such file or directory
cc1: warnings being treated as errors


-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-04 Thread Zach Welch
On Sat, 2009-12-05 at 08:35 +0100, Øyvind Harboe wrote:
 The approach seems to work fine, but requires that some other
 issues are sorted out first with include dirs.
 
 From error message above, there are two strange -I additions:
 
 NB! build != src dir

 -I. = current build directory
 -I../.. = this is the root build dir, so I would have to include
 include src/jtag/minidriver_imp.h. Was this supposed to be
 the build/src dir instead?
 
 I don't know where the two above are defined, but at the top of each
 Makefile.am there is a
 
 AM_CPPFLAGS = \
   -I$(top_srcdir)/src
 
 Should this include the build/src dir as well?
 
 

You got it.  We need to add -I$(top_builddir)/src to all directories
now that we are talking about generating them on the fly.

Do that in a separate patch please.  :)

--Z



___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-03 Thread Øyvind Harboe
So the minidriver (including classic API) must have control over whether or
not the following fn's gets inlined or not. These fn's really are in the inner
loops and boil away to *nothing* on a low performance low latency
system.

Note that I'm not keen on the idea of an ABI for OpenOCD, we need
more flexibility and less commitment than that, but I think the goal
can be a good design influence.

static inline void jtag_add_dr_out(struct jtag_tap* tap,
int num_fields, const int* num_bits, const uint32_t* value,
tap_state_t end_state)
{
assert(end_state != TAP_RESET);
assert(end_state != TAP_INVALID);

cmd_queue_cur_state = end_state;

interface_jtag_add_dr_out(tap,
num_fields, num_bits, value,
end_state);
}

/* The minidriver must be able to inline this fn */
static inline void jtag_add_callback(jtag_callback1_t f,
jtag_callback_data_t data0)
{
interface_jtag_add_callback(f, data0);
}

/* The minidriver must be able to inline this fn */
static inline void jtag_add_callback4(jtag_callback_t f,
jtag_callback_data_t data0,
jtag_callback_data_t data1, jtag_callback_data_t data2,
jtag_callback_data_t data3)
{
interface_jtag_add_callback4(f, data0, data1, data2, data3);
}




-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-03 Thread Zach Welch
On Thu, 2009-12-03 at 09:20 +0100, Øyvind Harboe wrote:
   The public API _must_ be a non-inline version of the API that calls the
  inline version.  Period.
 
 You don't explain why this is necessary and I don't agree. I believe
 you assume that it is impossible to allow the minidriver to inline
 *and* support an ABI. Actually you can, but the choice is made
 compile time on the particular platform.
 
 The minidriver should be able to take over and *inline* jtag_add_xxx()
 fn's if that is the right thing to do on a particular platform.

This is an important point: if you want your ZY1000 to provide an inline
version, then you should be able to do that.

 Similarly the classic minidriver must be able to implement jtag_add_xxx()
 as non-inline fn's to support an ABI.
 
  Inline functions may not be used in public APIs, because they prevent
  upgrading shared libraries.
 
 The statement above is wrong. For a platform with an ABI for shared
 libraries you use a minidriver(the classic one) that *does* allow
 shared library ABI, on an embedded platform you use the minidriver
 that removes this unecessary layer.

Right, for situations like yours, it does makes sense to allow inline
functions.  For shared libraries, however, my statements stand.
And those are public APIs.  Your platform has no public APIs and never
will, because you're statically linking the everything into one
binary... right?  So my statements are fully correct, and your reading
of them was too incorrect. ;)

Anyway, I think that I might have fixed everything that I was annoyed
with in the patches I just pushed.  Take another look at things and tell
me what you think needs to be done now :)

--Z
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-03 Thread Zach Welch
On Thu, 2009-12-03 at 09:53 +0100, Øyvind Harboe wrote:
 So the minidriver (including classic API) must have control over whether or
 not the following fn's gets inlined or not. These fn's really are in the inner
 loops and boil away to *nothing* on a low performance low latency
 system.

And they should now, after my patches.  I want to see yours again,
updated against the current master.  :)

 Note that I'm not keen on the idea of an ABI for OpenOCD, we need
 more flexibility and less commitment than that, but I think the goal
 can be a good design influence.

For your platform, you can forget about the ABI.  For shared libraries,
it is important  However, I agree that commitment to the ABI
should not be something to consider until 1.0 (or even 2.0...).

Striving to create a stable ABI, however, will improve the code, and
that is the real goal here.  It's about architectural correctness, and
inline functions are _not_ necessary for good performance.  I feel very
confident that your requirement for them could be eliminated by using
better algorithms in the JTAG layer, and you would still see better
performance than you get today.

Sure, that entails far more work, but the price to get correctness in
design is higher than that required to reach correctness in function.

--Z

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-03 Thread Zach Welch
On Thu, 2009-12-03 at 10:22 +0100, Øyvind Harboe wrote:
 (Ref. discussion I think we've settled that the minidriver can either
 implement an ABI or use inlining for performance.)
 
 On Thu, Dec 3, 2009 at 10:11 AM, Zach Welch z...@superlucidity.net wrote:
  On Thu, 2009-12-03 at 09:53 +0100, Øyvind Harboe wrote:
  So the minidriver (including classic API) must have control over whether or
  not the following fn's gets inlined or not. These fn's really are in the 
  inner
  loops and boil away to *nothing* on a low performance low latency
  system.
 
  And they should now, after my patches.  I want to see yours again,
  updated against the current master.  :)
 
 I have an automake problem / include file problem I don't know how
 to solve in a pretty fashion:
 
 jtag.h needs to include a minidriver_inline.h for the active minidriver.
 How to do that?

You should not need to add any new files at this point:

jtag/minidriver.h includes minidriver_imp.h:
jtag/drivers/minidriver_imp.h   -- the driver's implementation
jtag/minidriver/minidriver_imp.h -- the minidriver's implementation

The minidriver's implementaiton includes jtag_minidriver.h, which is
defined by the implementation:

jtag/minidummy/jtag_minidriver.h 
jtag/zy1000/jtag_minidriver.h 

Put stuff in these files...  Do not put new inline functions in
jtag/jtag.h. jtag/minidriver.h, or jtag/drivers/minidriver_imp.h.

 - I don't want to add -I to *all* places that #include jtag.h to the
 active minidriver
 
 - #include ACTIVE_MINIDRIVER, using a #define?
 
 I don't particularly like the #include jtag.h form. Why quotes
 rather than angle brackets BTW? Quotes allow including from
 the current directory, why should we allow the official jtag.h to
 be replaced by a variant local to a source dir? Can't think of
 why really.

Look at my mirror.  Go there right now. :)  I've mentioned my 'hdr' and
'tests' branches a few times.  You haven't looked at them, or you
wouldn't be asking this question.  That work is now on my master branch,
queued to be pushed.

Indeed, I was just about to reply to my post on this subject (see my
recent post with the subject libopenocd... again?) to warn that I want
to push this series in the next couple of hours.

So, you've gone and read it now, right... did it answer your question?
Unfortunately, it needs a little more love before it's ready to push,
but the above minidriver_imp.h and jtag_minidriver.h will _not_ be
changed to use angle brackets...  Otherwise, these tricks don't work.

--Z
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-03 Thread Øyvind Harboe
On Thu, Dec 3, 2009 at 10:36 AM, Zach Welch z...@superlucidity.net wrote:
 On Thu, 2009-12-03 at 10:22 +0100, Øyvind Harboe wrote:
 (Ref. discussion I think we've settled that the minidriver can either
 implement an ABI or use inlining for performance.)

 On Thu, Dec 3, 2009 at 10:11 AM, Zach Welch z...@superlucidity.net wrote:
  On Thu, 2009-12-03 at 09:53 +0100, Øyvind Harboe wrote:
  So the minidriver (including classic API) must have control over whether 
  or
  not the following fn's gets inlined or not. These fn's really are in the 
  inner
  loops and boil away to *nothing* on a low performance low latency
  system.
 
  And they should now, after my patches.  I want to see yours again,
  updated against the current master.  :)

 I have an automake problem / include file problem I don't know how
 to solve in a pretty fashion:

 jtag.h needs to include a minidriver_inline.h for the active minidriver.
 How to do that?

 You should not need to add any new files at this point:

 jtag/minidriver.h includes minidriver_imp.h:
 jtag/drivers/minidriver_imp.h   -- the driver's implementation
 jtag/minidriver/minidriver_imp.h -- the minidriver's implementation

 The minidriver's implementaiton includes jtag_minidriver.h, which is
 defined by the implementation:

 jtag/minidummy/jtag_minidriver.h
 jtag/zy1000/jtag_minidriver.h

 Put stuff in these files...  Do not put new inline functions in
 jtag/jtag.h. jtag/minidriver.h, or jtag/drivers/minidriver_imp.h.

To get inlining to work, I need to make a few fn's in core.c
inline fn's too. Take jtag_add_dr_out() as an example.

jtag_add_dr_out() must be defined after including jtag.h. This means
jtag.h must include the inline version of interface_jtag_add_dr_out().

So yes... I need to figure out a way for minidrivers to expose, via jtag.h,
the inline implementations.

The driver minidriver should be able to implement jtag_add_dr_out() as a
non-inlined function in order to be able to support an ABI at that level.

 Look at my mirror.  Go there right now. :)  I've mentioned my 'hdr' and
 'tests' branches a few times.  You haven't looked at them, or you
 wouldn't be asking this question.  That work is now on my master branch,
 queued to be pushed.

Rights... I recall those changes. Yes that's a good cleanup. I
protest when you are doing cleanup work that causes problems,
but not comment when it looks all good.

I think I want to wait with the minidriver work until you have the hdr branch
pushed.


-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-03 Thread Øyvind Harboe
(Ref. discussion I think we've settled that the minidriver can either
implement an ABI or use inlining for performance.)

On Thu, Dec 3, 2009 at 10:11 AM, Zach Welch z...@superlucidity.net wrote:
 On Thu, 2009-12-03 at 09:53 +0100, Øyvind Harboe wrote:
 So the minidriver (including classic API) must have control over whether or
 not the following fn's gets inlined or not. These fn's really are in the 
 inner
 loops and boil away to *nothing* on a low performance low latency
 system.

 And they should now, after my patches.  I want to see yours again,
 updated against the current master.  :)

I have an automake problem / include file problem I don't know how
to solve in a pretty fashion:

jtag.h needs to include a minidriver_inline.h for the active minidriver.
How to do that?

- I don't want to add -I to *all* places that #include jtag.h to the
active minidriver

- #include ACTIVE_MINIDRIVER, using a #define?

I don't particularly like the #include jtag.h form. Why quotes
rather than angle brackets BTW? Quotes allow including from
the current directory, why should we allow the official jtag.h to
be replaced by a variant local to a source dir? Can't think of
why really.


-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-03 Thread Zach Welch
On Thu, 2009-12-03 at 10:51 +0100, Øyvind Harboe wrote:
 On Thu, Dec 3, 2009 at 10:36 AM, Zach Welch z...@superlucidity.net wrote:
  On Thu, 2009-12-03 at 10:22 +0100, Øyvind Harboe wrote:
  (Ref. discussion I think we've settled that the minidriver can either
  implement an ABI or use inlining for performance.)
 
  On Thu, Dec 3, 2009 at 10:11 AM, Zach Welch z...@superlucidity.net wrote:
   On Thu, 2009-12-03 at 09:53 +0100, Øyvind Harboe wrote:
   So the minidriver (including classic API) must have control over 
   whether or
   not the following fn's gets inlined or not. These fn's really are in 
   the inner
   loops and boil away to *nothing* on a low performance low latency
   system.
  
   And they should now, after my patches.  I want to see yours again,
   updated against the current master.  :)
 
  I have an automake problem / include file problem I don't know how
  to solve in a pretty fashion:
 
  jtag.h needs to include a minidriver_inline.h for the active minidriver.
  How to do that?
 
  You should not need to add any new files at this point:
 
  jtag/minidriver.h includes minidriver_imp.h:
  jtag/drivers/minidriver_imp.h   -- the driver's implementation
  jtag/minidriver/minidriver_imp.h -- the minidriver's implementation
 
  The minidriver's implementaiton includes jtag_minidriver.h, which is
  defined by the implementation:
 
  jtag/minidummy/jtag_minidriver.h
  jtag/zy1000/jtag_minidriver.h
 
  Put stuff in these files...  Do not put new inline functions in
  jtag/jtag.h. jtag/minidriver.h, or jtag/drivers/minidriver_imp.h.
 
 To get inlining to work, I need to make a few fn's in core.c
 inline fn's too. Take jtag_add_dr_out() as an example.

Well, you can't.  The rules prohibit this. ;)

 jtag_add_dr_out() must be defined after including jtag.h. This means
 jtag.h must include the inline version of interface_jtag_add_dr_out().

That's fine -- for ZY1000. 

 So yes... I need to figure out a way for minidrivers to expose, via 
 jtag.h,
 the inline implementations.

...while also providing an out-of-line version for the drivers.  :/

Define a macro called jtag_add_dr_out in drivers/minidriver_imp.h that
redirects the call to an out-of-line function (with the _imp suffix).
The driver.c file can define that function and #include a new header
that is also shared with minidriver/minidriver_imp.h.  This new header
provides the inline version, which is called from the driver's _imp.
There you have it.

Or, improve the algorithms such that such inlining is moot. ;)

 The driver minidriver should be able to implement jtag_add_dr_out() as a
 non-inlined function in order to be able to support an ABI at that level.

As I said elsewhere, it's now safe to #include minidriver.h in
jtag.h, so long as you do not add in-lines to the files I noted.

  Look at my mirror.  Go there right now. :)  I've mentioned my 'hdr' and
  'tests' branches a few times.  You haven't looked at them, or you
  wouldn't be asking this question.  That work is now on my master branch,
  queued to be pushed.
 
 Rights... I recall those changes. Yes that's a good cleanup. I
 protest when you are doing cleanup work that causes problems,
 but not comment when it looks all good.

The problem is that your definition of good puts performance too high at
the list of priorities.  At this stage, that is like polishing a turd.

You would be better off investigating long-term fixes, rather than
chasing around implementation issues that patch over the problem
temporary.  The architectural improvements that are binding your hands
presently reflect other design improvements need to be made.

That said, I still want to see you find a suitable workaround, and there
will undoubtedly be a compromise involved. ;)

 I think I want to wait with the minidriver work until you have the hdr branch
 pushed.

It's nearly ready.

--Z
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-02 Thread Zach Welch
Do not apply this until you fix the problem noted below.

Thanks,

Zach

On Wed, 2009-12-02 at 14:38 +0100, Øyvind Harboe wrote:
 Fix 'regression' where jtag_add_dr_out() is no longer inlined
 after refactoring.
 
 The minidrivers strip away all the overhead and allow inner loops
 to communicate directly with a hardware FIFO.
 
 Signed-off-by: Øyvind Harboe oyvind.har...@zylin.com
 ---
  src/jtag/Makefile.am |2 -
  src/jtag/core.c  |   14 -
  src/jtag/driver.c|   10 +++
  src/jtag/jtag.h  |   37 +++--
  src/jtag/minidriver.h|   67 ++---
  src/jtag/zy1000/zy1000.c |3 +-
  6 files changed, 73 insertions(+), 60 deletions(-)
 
 diff --git a/src/jtag/Makefile.am b/src/jtag/Makefile.am
 index 5254a2b..c762300 100644
 --- a/src/jtag/Makefile.am
 +++ b/src/jtag/Makefile.am
 @@ -11,11 +11,9 @@ if MINIDRIVER
  
  if ZY1000
  DRIVERFILES += zy1000/zy1000.c
 -AM_CPPFLAGS += -I$(srcdir)/zy1000
  endif
  if MINIDRIVER_DUMMY
  DRIVERFILES += minidummy/minidummy.c commands.c
 -AM_CPPFLAGS += -I$(srcdir)/minidummy
  endif
  
  else
 diff --git a/src/jtag/core.c b/src/jtag/core.c
 index 9230cc2..cf14394 100644
 --- a/src/jtag/core.c
 +++ b/src/jtag/core.c
 @@ -502,20 +502,6 @@ void jtag_add_plain_dr_scan(int in_num_fields, const 
 struct scan_field *in_field
   jtag_set_error(retval);
  }
  
 -void jtag_add_dr_out(struct jtag_tap* tap,
 - int num_fields, const int* num_bits, const uint32_t* value,
 - tap_state_t end_state)
 -{
 - assert(end_state != TAP_RESET);
 - assert(end_state != TAP_INVALID);
 -
 - cmd_queue_cur_state = end_state;
 -
 - interface_jtag_add_dr_out(tap,
 - num_fields, num_bits, value,
 - end_state);
 -}
 -
  void jtag_add_tlr(void)
  {
   jtag_prelude(TAP_RESET);
 diff --git a/src/jtag/driver.c b/src/jtag/driver.c
 index cadd88e..fe56369 100644
 --- a/src/jtag/driver.c
 +++ b/src/jtag/driver.c
 @@ -525,3 +525,13 @@ void interface_jtag_add_callback(jtag_callback1_t 
 callback, jtag_callback_data_t
   jtag_add_callback4(jtag_convert_to_callback4, data0, 
 (jtag_callback_data_t)callback, 0, 0);
  }
  
 +void interface_jtag_alloc_in_value32(struct scan_field *field)
 +{
 + field-in_value = (uint8_t *)cmd_queue_alloc(4);
 +}
 +
 +void interface_jtag_add_scan_check_alloc(struct scan_field *field)
 +{
 + unsigned num_bytes = DIV_ROUND_UP(field-num_bits, 8);
 + field-in_value = (uint8_t *)cmd_queue_alloc(num_bytes);
 +}
 diff --git a/src/jtag/jtag.h b/src/jtag/jtag.h
 index ee96775..4b0e8b2 100644
 --- a/src/jtag/jtag.h
 +++ b/src/jtag/jtag.h
 @@ -2,7 +2,7 @@
  *   Copyright (C) 2005 by Dominic Rath*
  *   dominic.r...@gmx.de   *
  * *
 -*   Copyright (C) 2007,2008 Øyvind Harboe *
 +*   Copyright (C) 2007-2009 Øyvind Harboe *
  *   oyvind.har...@zylin.com   *
  * *
  *   This program is free software; you can redistribute it and/or modify  *
 @@ -663,37 +663,6 @@ void jtag_sleep(uint32_t us);
  #define ERROR_JTAG_INIT_SOFT_FAIL(-110)
  
  /**
 - * jtag_add_dr_out() is a version of jtag_add_dr_scan() which
 - * only scans data out. It operates on 32 bit integers instead
 - * of 8 bit, which makes it a better impedance match with
 - * the calling code which often operate on 32 bit integers.
 - *
 - * Current or end_state can not be TAP_RESET. end_state can be TAP_INVALID
 - *
 - * num_bits[i] is the number of bits to clock out from value[i] LSB first.
 - *
 - * If the device is in bypass, then that is an error condition in
 - * the caller code that is not detected by this fn, whereas
 - * jtag_add_dr_scan() does detect it. Similarly if the device is not in
 - * bypass, data must be passed to it.
 - *
 - * If anything fails, then jtag_error will be set and jtag_execute() will
 - * return an error. There is no way to determine if there was a failure
 - * during this function call.
 - *
 - * This is an inline fn to speed up embedded hosts. Also note that
 - * interface_jtag_add_dr_out() can be a *small* inline function for
 - * embedded hosts.
 - *
 - * There is no jtag_add_dr_outin() version of this fn that also allows
 - * clocking data back in. Patches gladly accepted!
 - */
 -void jtag_add_dr_out(struct jtag_tap* tap,
 - int num_fields, const int* num_bits, const uint32_t* value,
 - tap_state_t end_state);
 -
 -
 -/**
   * Set the current JTAG core execution error, unless one was set
   * by a previous call previously.  Driver or application code must
   * use jtag_error_clear to reset jtag_error once this routine has been
 @@ -725,4 +694,8 @@ 

Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-02 Thread Øyvind Harboe
 +
 +/* The minidriver contains inline versions of JTAG fn's */
 +#include minidriver.h
 +

 This is bad, as you are creating a new layering violation that will need
 to be removed.  You should move this #include to somewhere other than
 what should be our public API, probably inside the C files that need it.
 Presently, this change exposes _more_ internals, not less.

I'm not quite sure how you define a layer violation here. How can I tell
that there is a violation happening?

A minidriver must be able to inline jtag_add_dr_out() into inner loops,
this is what broke in the refactoring. All the rest of the refactoring is
great stuff!

jtag_add_dr_out() is also at the same time an API, so what was done in
minidrivers was a carefully controlled layer violation that took
advantage of the
tight coupling between the CPU and the hardware = two pokes
for some jtag_add_dr_out() cases.

The jtag_add_dr_out() API was engineered with this specifically in mind.

Do you have any thoughts on how to get back the functionality of
letting the minidriver provide an inline version of jtag_add_dr_out()?

Inlining is important because, not because of the fn call overhead,
but because the arguments to jtag_add_dr_out() are often constants,
so there is *significant* constant propagation going on that will
reduce jtag_add_dr_out() to two pokes in many cases. Important
on low latency low processing power(embedded) environments.

-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-02 Thread Zach Welch
I just noticed that there may be other problems with this; I question
the need for both -I and #include dirname/  Seems like one or the
other is enough, so there appears to be a new redundancy here.

On Wed, 2009-12-02 at 13:39 -0800, Zach Welch wrote:
 Do not apply this until you fix the problem noted below.
 
 Thanks,
 
 Zach
 
 On Wed, 2009-12-02 at 14:38 +0100, Øyvind Harboe wrote:
  Fix 'regression' where jtag_add_dr_out() is no longer inlined
  after refactoring.
  
  The minidrivers strip away all the overhead and allow inner loops
  to communicate directly with a hardware FIFO.
  
  Signed-off-by: Øyvind Harboe oyvind.har...@zylin.com
  ---
   src/jtag/Makefile.am |2 -
   src/jtag/core.c  |   14 -
   src/jtag/driver.c|   10 +++
   src/jtag/jtag.h  |   37 +++--
   src/jtag/minidriver.h|   67 
  ++---
   src/jtag/zy1000/zy1000.c |3 +-
   6 files changed, 73 insertions(+), 60 deletions(-)
  
  diff --git a/src/jtag/Makefile.am b/src/jtag/Makefile.am
  index 5254a2b..c762300 100644
  --- a/src/jtag/Makefile.am
  +++ b/src/jtag/Makefile.am
  @@ -11,11 +11,9 @@ if MINIDRIVER
   
   if ZY1000
   DRIVERFILES += zy1000/zy1000.c
  -AM_CPPFLAGS += -I$(srcdir)/zy1000
   endif
   if MINIDRIVER_DUMMY
   DRIVERFILES += minidummy/minidummy.c commands.c
  -AM_CPPFLAGS += -I$(srcdir)/minidummy
   endif
   
   else
  diff --git a/src/jtag/core.c b/src/jtag/core.c
  index 9230cc2..cf14394 100644
  --- a/src/jtag/core.c
  +++ b/src/jtag/core.c
  @@ -502,20 +502,6 @@ void jtag_add_plain_dr_scan(int in_num_fields, const 
  struct scan_field *in_field
  jtag_set_error(retval);
   }
   
  -void jtag_add_dr_out(struct jtag_tap* tap,
  -   int num_fields, const int* num_bits, const uint32_t* value,
  -   tap_state_t end_state)
  -{
  -   assert(end_state != TAP_RESET);
  -   assert(end_state != TAP_INVALID);
  -
  -   cmd_queue_cur_state = end_state;
  -
  -   interface_jtag_add_dr_out(tap,
  -   num_fields, num_bits, value,
  -   end_state);
  -}
  -
   void jtag_add_tlr(void)
   {
  jtag_prelude(TAP_RESET);
  diff --git a/src/jtag/driver.c b/src/jtag/driver.c
  index cadd88e..fe56369 100644
  --- a/src/jtag/driver.c
  +++ b/src/jtag/driver.c
  @@ -525,3 +525,13 @@ void interface_jtag_add_callback(jtag_callback1_t 
  callback, jtag_callback_data_t
  jtag_add_callback4(jtag_convert_to_callback4, data0, 
  (jtag_callback_data_t)callback, 0, 0);
   }
   
  +void interface_jtag_alloc_in_value32(struct scan_field *field)
  +{
  +   field-in_value = (uint8_t *)cmd_queue_alloc(4);
  +}
  +
  +void interface_jtag_add_scan_check_alloc(struct scan_field *field)
  +{
  +   unsigned num_bytes = DIV_ROUND_UP(field-num_bits, 8);
  +   field-in_value = (uint8_t *)cmd_queue_alloc(num_bytes);
  +}
  diff --git a/src/jtag/jtag.h b/src/jtag/jtag.h
  index ee96775..4b0e8b2 100644
  --- a/src/jtag/jtag.h
  +++ b/src/jtag/jtag.h
  @@ -2,7 +2,7 @@
   *   Copyright (C) 2005 by Dominic Rath*
   *   dominic.r...@gmx.de   *
   * *
  -*   Copyright (C) 2007,2008 Øyvind Harboe *
  +*   Copyright (C) 2007-2009 Øyvind Harboe *
   *   oyvind.har...@zylin.com   *
   * *
   *   This program is free software; you can redistribute it and/or modify  *
  @@ -663,37 +663,6 @@ void jtag_sleep(uint32_t us);
   #define ERROR_JTAG_INIT_SOFT_FAIL(-110)
   
   /**
  - * jtag_add_dr_out() is a version of jtag_add_dr_scan() which
  - * only scans data out. It operates on 32 bit integers instead
  - * of 8 bit, which makes it a better impedance match with
  - * the calling code which often operate on 32 bit integers.
  - *
  - * Current or end_state can not be TAP_RESET. end_state can be TAP_INVALID
  - *
  - * num_bits[i] is the number of bits to clock out from value[i] LSB first.
  - *
  - * If the device is in bypass, then that is an error condition in
  - * the caller code that is not detected by this fn, whereas
  - * jtag_add_dr_scan() does detect it. Similarly if the device is not in
  - * bypass, data must be passed to it.
  - *
  - * If anything fails, then jtag_error will be set and jtag_execute() will
  - * return an error. There is no way to determine if there was a failure
  - * during this function call.
  - *
  - * This is an inline fn to speed up embedded hosts. Also note that
  - * interface_jtag_add_dr_out() can be a *small* inline function for
  - * embedded hosts.
  - *
  - * There is no jtag_add_dr_outin() version of this fn that also allows
  - * clocking data back in. Patches gladly accepted!
  - */
  -void jtag_add_dr_out(struct jtag_tap* 

Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-02 Thread Øyvind Harboe
On Wed, Dec 2, 2009 at 10:55 PM, Zach Welch z...@superlucidity.net wrote:
 I just noticed that there may be other problems with this; I question
 the need for both -I and #include dirname/  Seems like one or the
 other is enough, so there appears to be a new redundancy here.

I removed the -I and used only dirname/.

I find the addition of numerous -I's confusing and error prone
+ makes it harder to read  check build logs.

-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-02 Thread Zach Welch
On Wed, 2009-12-02 at 23:00 +0100, Øyvind Harboe wrote:
 On Wed, Dec 2, 2009 at 10:55 PM, Zach Welch z...@superlucidity.net wrote:
  I just noticed that there may be other problems with this; I question
  the need for both -I and #include dirname/  Seems like one or the
  other is enough, so there appears to be a new redundancy here.
 
 I removed the -I and used only dirname/.

Thanks!

 I find the addition of numerous -I's confusing and error prone
 + makes it harder to read  check build logs.

Try my 'hdr' or 'tests' branches on my mirror.  They remove almost all
of the internal -I's from the tree!!! :)

--Z
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-02 Thread Zach Welch
On Wed, 2009-12-02 at 22:54 +0100, Øyvind Harboe wrote:
  +
  +/* The minidriver contains inline versions of JTAG fn's */
  +#include minidriver.h
  +
 
  This is bad, as you are creating a new layering violation that will need
  to be removed.  You should move this #include to somewhere other than
  what should be our public API, probably inside the C files that need it.
  Presently, this change exposes _more_ internals, not less.

Let me emphasize my point above: remove the #include from jtag.h.
Do not expose it outside the JTAG layer.  If it is a public API too,
then you must rename the inline version and call it from a new wrapper
function in jtag/core.c.  You are exposing all of the minidriver to the
entire system, and that's unacceptable.

If this hurts performance, then please show us the numbers here first.
Even then, this needs to be done in a way that maintains encapsulation.

 I'm not quite sure how you define a layer violation here. How can I tell
 that there is a violation happening?

jtag.h  public APIs for use by external applications

minidriver.h  internal APIs.  These drivers will _always_ be built-in
to the tree (for performance reasons). Thus, their interfaces should
__never__ be exposed to users through the public JTAG API.

minidummy/jtag_interface.h  implementation APIs are even more
internal, yet your patch will make these available in the public API!!

No one should be adding new internals into public headers.  If this
means we need to create a slew of module_imp.h files to share bits
of the internal implementation, then so be it.  It's worth it, if it
means that we can stop expose our internals.  Nevertheless, you have to
stop and really consider the consequences of stick stuff in headers!
Who will see it?  Should they?! ;)   Create a new function to wrap the
inline call if needed, but stop sticking stuff in the wrong place!!

I can make our libraries usable for 0.4.0, though the APIs will not be
stable for some time yet.  Still, it opens the door to writing external
applications; that option is closed today, and we are hurting the code
by delaying.  We'll never find out the problems until we start writing
applications that use the code -- such as a test suite.

Given these hard constraints, I recommend taking another stab at the
patch and finding a way around this.  To be honest, this might reflect
fundamental design flaws with the minidriver, such that it can only be
fixed after some rather drastic re-writing of the code.  If the code
relies this heavily on these optimizations, it's broken by design.

Cheers,

--Z
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-02 Thread Zach Welch
On Wed, 2009-12-02 at 14:32 -0800, Zach Welch wrote:
 On Wed, 2009-12-02 at 22:54 +0100, Øyvind Harboe wrote:
   +
   +/* The minidriver contains inline versions of JTAG fn's */
   +#include minidriver.h
   +
  
   This is bad, as you are creating a new layering violation that will need
   to be removed.  You should move this #include to somewhere other than
   what should be our public API, probably inside the C files that need it.
   Presently, this change exposes _more_ internals, not less.
 
 Let me emphasize my point above: remove the #include from jtag.h.
 Do not expose it outside the JTAG layer.  If it is a public API too,
 then you must rename the inline version and call it from a new wrapper
 function in jtag/core.c.  You are exposing all of the minidriver to the
 entire system, and that's unacceptable.
[snip]
 Given these hard constraints, I recommend taking another stab at the
 patch and finding a way around this.  To be honest, this might reflect
 fundamental design flaws with the minidriver, such that it can only be
 fixed after some rather drastic re-writing of the code.  If the code
 relies this heavily on these optimizations, it's broken by design.

Here's one possible solution: we could #include minidriver.h -- as long
as it is an empty file when installed.  This means two versions of
minidriver.h, one that includes the proper jtag_minidriver.h and one
that is empty.  In fact, I think you can rename the later to the former
name, then use the -I trick.  Then add a mininone/minidriver.h that is
the empty file, which is included when the others aren't.  Grok?

--
Z
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-02 Thread Øyvind Harboe
I was thinking about splitting this patch into at least a few:

The first would be to get rid of some -I's.

The second would clarify that the classic JTAG driver
is just another minidriver.

The third would then allow a minidriver to grab the
JTAG stack from jtag_xxx() and down to the metal.

The performance gain potential here is, over time with
a few more fixes like it, ca. 10-25%. I currently get 550kBytes/s
@ 16MHz w/arm7/9 for gdb load, so say up to 600-700kBytes/s
somewhere eventually due to this. Performance is a long
term  fun project for me :-)

I actually saw that jtag_add_dr_out() was no longer inlined
because it is creeping up on the gprof list for gdb load...

-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-02 Thread Zach Welch
On Wed, 2009-12-02 at 14:44 -0800, Zach Welch wrote:
 On Wed, 2009-12-02 at 14:32 -0800, Zach Welch wrote:
  On Wed, 2009-12-02 at 22:54 +0100, Øyvind Harboe wrote:
+
+/* The minidriver contains inline versions of JTAG fn's */
+#include minidriver.h
+
   
This is bad, as you are creating a new layering violation that will need
to be removed.  You should move this #include to somewhere other than
what should be our public API, probably inside the C files that need it.
Presently, this change exposes _more_ internals, not less.
  
  Let me emphasize my point above: remove the #include from jtag.h.
  Do not expose it outside the JTAG layer.  If it is a public API too,
  then you must rename the inline version and call it from a new wrapper
  function in jtag/core.c.  You are exposing all of the minidriver to the
  entire system, and that's unacceptable.
 [snip]
  Given these hard constraints, I recommend taking another stab at the
  patch and finding a way around this.  To be honest, this might reflect
  fundamental design flaws with the minidriver, such that it can only be
  fixed after some rather drastic re-writing of the code.  If the code
  relies this heavily on these optimizations, it's broken by design.
 
 Here's one possible solution: we could #include minidriver.h -- as long
 as it is an empty file when installed.  This means two versions of
 minidriver.h, one that includes the proper jtag_minidriver.h and one
 that is empty.  In fact, I think you can rename the later to the former
 name, then use the -I trick.  Then add a mininone/minidriver.h that is
 the empty file, which is included when the others aren't.  Grok?

I am probably wrong about some details (e.g. renaming), but the goal of
this is to eliminate the #if logic from the header files too.  That is
not acceptable for installed libraries either, so we should not be
adding more of that to headers  The tricks above are similar to
those that I used to clean openocd.c of the same logic, so those efforts
might help provide you with the required inspiration to solve this.

--Z

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-02 Thread Øyvind Harboe
 I am probably wrong about some details (e.g. renaming), but the goal of
 this is to eliminate the #if logic from the header files too.  That is
 not acceptable for installed libraries either, so we should not be
 adding more of that to headers  The tricks above are similar to
 those that I used to clean openocd.c of the same logic, so those efforts
 might help provide you with the required inspiration to solve this.

The minidrivers and inlining of jtag_add_dr_out() is pulling off
something that's inheritely incestuous.

The active minidriver must be included conditionally by some trick or
another into all users of the jtag_add_xxx() API.

-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] minidriver: allow jtag_add_dr_out() to be inlined into inner loops

2009-12-02 Thread Øyvind Harboe
 Okay.  The solution that I have tried to propose will get this back,
 without exposing anything during a normal non-minidriver installation.

The point I'm trying to make is that there is no normal non-minidriver.
The normal/classic stack is *also* a minidriver.

The *first* step would be to split out the normal non-minidriver as
a minidriver just like the rest. That should make things more clear.

Essentially the minidriver supports *everything* from jtag_add_xxx()
API and down to the metal. This goes for the classic case as well.


I won't be pushing this issue with any haste, especially as there
is lots of stuff in flight.


-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development