Hi Tomek,
First, thank you for giving objective comments on the patch. Much
appreciated from you.
(Much better than the "Student code galore" from Peter)
Hello Laurent,
Sorry if my comment seems not meritoric, I am not mentioning about
coding style because this leaves a lot to tell in case of all program
source tree. I am not advanced OpenOCD developer, I am learning code
internals for some time and I have some grasp on it already (enough to
recognize the function we are talking about without looking at the
code because I have already worked on it). Please take a look at my
explanation why I think this patch is not that good ant try to
convince me (maybe others?) that I am wrong :-)
I reply on your text, maybe I can convince you.
1. The reason to propose this patch was improper error handling.
OpenOCD works in a manner where functions return ERROR_OK or
ERROR_FAIL, all additional/verbose information are provided by LOG_*
messages. This is also the case in ft2232_init(), so no need to
change. If you change this all other interfaces will behave different,
so the code gets dirty and unpredictable. This is a standard behavior
for all of the code.
Yes, right. But the patch do not touch the error mechanism, which one is
good enough.
2. The malloc() problem can be solved in a simple way as demonstrated
by my patch. No need to complicate things, no need to add other
functions that makes code less readable.
The patch resolve it.
3. All interfaces have init() and quit() functions. If you change this
for one type of driver the code will get dirty as one drivers will
behave other than another interfaces. This should be avoided.
Yes, the patch do not touch the API. We still have the init and quit for
the high layer acceding the ft2232 interface.
4. What is the real benefit of implementing/splitting open() and
init() for interface? Interface is a platform for further program
operation. Without interface program cannot work properly, so what
would you like to do with interface that is opened but
unusable/uninitialized? The current behavior is simple - interface
initialization failed, so will other things, retry or quit, don't go
futher. Interface init() checks if interface is connected and sets up
its state, this is enough for one function, why you want to split it?
Again the goal is to have INSIDE the ft2232.c something like :
open(open the ft2232 handle)
init(init the ft2232 device)
deinit(deinit the ft2232 device)
close(close the ft2232 handle)
From the JTAG INTERFACE API (higher layer point of view) we still have
the only two functions
init (init the interface)
quit (quit the interface)
Globally we have:
init (init the interface)
open(open the ft2232 handle)
init(init the ft2232 device)
deinit(deinit the ft2232 device)
close(close the ft2232 handle)
quit (quit the interface)
The objective is to provide at middle term a better code and more
comprehensible code for the FT2232.c.
From the higher level, you still init and quit the interface. But in
the ft2232 interface, we separate open / close the handle and the init
/ deinit the device.
In the open and close we should only process the ft2232 handle
In the init and deinit, we initialize and deinitialize the ft2232 device
( the MPSSE mode and the specific layout with specific function (as
jtagkey_init) )
Actually the deinit is not implemented and it should be done quickly.
The deinit should first deinitialize the specific layout (as a
jtagkey_deinit), then disable the MPSSE / bitbang mode for any USB JTAG
based ft2232. Objective is to let see the ft2232 device as if it was
just plugged (after a power-off -> power-on). After the deinit of the
ft2232 device is done, we can close the ft2232 handle ...
Without doing something within deinit, we still have strange openocd
inter-session phenomena (as TRST still driven instead to be TRISTATE ...
really important for debugging TI OMAP ! )
If you have a Amontec JTAGkey-2 you can see the yellow LED stays active
ON after a shutdown of openocd. The yellow LED on Amontec JTAGkey-2 let
know the users that one of the TRST_OE_N SRST_OE_N JTAG_OE_N are/is
still asserted. This mean, actually, the openOCD did not correctly
deinit the jtagkey layout nor the MPSSE nor the bitbang mode, before
closing the handle ... in other term :
We do not deinit the device before closing the handle ... That's make
troubles in inter-session of openocd ...
So the deinit is really important, but was never implemented. The patch
will help to quickly integrate the deinit of a specific layout and
deinit of the MPSSE mode.
(Sebastien is ready to send patch concerning the deinit)
Also, you can see the importance of decoupling the :
open / close of the FT2232 handle
init / deinit of the FT2232 device
5. There is a transport layer above interface, transport has select()
and init() - maybe this is the place you should place your code?
Transport select() function sets up interface and memory/registers for
transport internals, while transport init() can interrogate connected
targets (as the interface and transport are ready). Transport is
initialized automatically just after interface is initialized.
Again the patch do not touch the interface see reply on 4.)
The transport layer is at a higher level than the interface layer !
I do not want to talk about how the higher layer access the lower layer
. That's not the subject of the patch.
Long story short - there is already presented open/init mechanism at
transport layer, interface is only a platform for this to happen.
There is not much use of interface that is opened but unusable - this
is common for all interfaces, not only ft2232 - it is simply ready for
use or not.
Yes right.
But when we will come with new SWD transport support, the interface
layer ( ft2232.c ) will have to be modified to accept new hardware as
the Amontec JTAGkey-3 coming with the SWD support.
There are more things to be fixed and changes in the OpenOCD internal
organization (i.e. program flow, global variables rewrite into common
context passed as function parameter, etc), but this should be done in
non-invasive, clean and rational manner. If we want to change
interface behavior, this change should apply to all interfaces. We
cannot allow one interface behave in other way than the others because
this will only bring confusion. The program flow is unclear enough to
complicate it even more right now. This is why I would freeze the
changes for a moment where general interface architecture will change
to become non-jtag-specific. Again please specify the benefits of
open() and init() for interface, and why it cannot happen in transport
layer as it is done right now.
NO NO NO Again, we do not change the init / quit for the higher level
access. But we split the how-to open close the FT2232 handle and how-to
init deinit the FT2232 device + specific layout ...
Sure, there are a lot to do in the openocd structure itself.. But the
patch is for ft2232.c only. Nothing to do yet with transport layer.
About the (c) in the header fields I think that those should be
reserved for people that bring more than patching/rewrite but this is
a minor issue :-)
Right, but the patch corrects an important memory trouble in the
FT2232.c, and prepare the FT2232.c for a better open -> init -> deinit
-> close ! The patch is not only a rewrite, it is a correction.
Best regards,
Tomek
THE PATCH DO NOT TOUCH THE JTAG INTERFACE API !
convinced ?
Best regards,
Laurent
http://www.amontec.com
http://www.amontec.com/jtagkey.shtml
_______________________________________________
Openocd-development mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/openocd-development
