Hi Andreas / Tomek, You're right that the current adapter driver architecture sucks. I like your suggested architecture, so I've made a diagram of my interpretation of it:
https://docs.google.com/drawings/d/1n0ron1Xb08bA9lMscgt6cp1LfidkHWACsuMQmGmX18I/edit Please feel free to hack it if you think it's not quite right. I also like Andreas' idea of a wrapper for legacy drivers. I think that the drivers should be dynamic with registration/deregistration of: * transports they support as they are enabled/disabled in hardware * extra TCL commands supported Regards, Evan -----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of CeDeROM Sent: Tuesday, 6 November 2012 9:20 PM To: Andreas Fritiofson Cc: Evan Hunter; [email protected] Subject: Re: [OpenOCD-devel] Interface driver change I agree with Andreas that we need to define API and organization first, then code in the last place... Transport is very very good idea and I have used it with my own implementation of the mpsse functions in ft2232 interface, but new mpsse is even better. Transport works, its great idea - see http://repo.or.cz/w/openocd/libswd.git/shortlog/refs/heads/openocd-201106-development-libswd-0.4-release I am still working on the transport part and the SWD on top of the fresh openocd head. In the previous one I just deleted ugly adiv5_swd.c and this solved all problems. But if I did right now it could break existing code/solutions... From the Transport perspective it is essential to have two generic transport functions on the Interface port, and some additional operations that may help solving problems with the Target: 1. "transfter" that would transfter in/out bytestream on mosi/miso/tdo/tdi lines of the interface from/to char array. This will allow to perform packed transfer operations and it will be faster than sending single bits. This should be "must have" for an interface. 2. "bitbang" that would allow to set/reset/read state of a gpio pin of an interface. If will allow driving port states from TCL but more important switch buffers for half-duplex transports like SWD. This should be "must have" for a interface. 3. Additional list of "Features" registered at runtime that may help transport or target operations. Why 3 is very important: +because it will give transport independence and leave generic interface driver very clean simple and elegant. +because some drivers (like ft2232) cannot do transport on their own, so they need dedicated transport infrastructure (like libswd) to generate and analyze bistream for them, while some other drivers (like versaloon) can do transport on their own far better. I can try out this approach on my fork. I will define "Features" list on the current jtag_driver driver and those features will contain pointers to functions that perform operations on a target (i.e. dp_read) by an Interface. If some transport is selected (i.e. SWD) it will check if Interface have necessary Features and mark to use them if so. These Features will be the interface between Interface - Transport - Target. For Versaloon or other intelligent devices such DP_READ from Target layer will simply pass the call by transport to Feature if an Interface and return the result. In case of FT2232 Tranport layer is essential because it will convert Target DP_READ call intro bistream that will be transported by a "Transfer" and "Bitbang" functions at its base. In short words Interface->Features will be the bridge between Transport and the Interface. In case of intelligent interfaces it will only get information from Target layer and the Features will be defined by the Interface driver itself. In case of dummy drivers Features list will contain functions defined by Tansport to perform desired operations using only two generic Interface's functions "transfer" and "bitbang"... this should solve the current problem between intelligent and dummy drivers that are supposed to run different transports and the same transports in different manner. Best regards :-) Tomek -- CeDeROM, SQ7MHZ, http://www.tomek.cedro.info -----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of Andreas Fritiofson Sent: Tuesday, 6 November 2012 9:56 AM To: Evan Hunter Cc: [email protected] Subject: Re: [OpenOCD-devel] Interface driver change On Mon, Nov 5, 2012 at 4:40 AM, Evan Hunter <[email protected]> wrote: > I would like to modify the adapter driver interfaces to allow > per-instance driver variable structures. > > This would: > • Get rid of loads of static and global variables. > • Allow multiple interfaces > > The proposed prototypes below mostly add this per-instance variable as > an instance handle. > Drivers could keep using the current system and be modified when > suitable to use the per –instance parameters. > > Is this something that is wanted? While this is a worthwhile goal, I don't think the global state of *the current* adapter interface is a big deal, because there are far worse flaws in it. I would rather see a fresh new adapter API designed and implemented in parallel. Such an API should of course be designed with these features in mind. > > Do you have any thoughts on the proposed organisation below? > Yes, I don't like it. Because it looks a lot like what we have today, and what we have today kind of sucks. It could work as a first step, but unfortunately I don't buy the idea that this change is better done incrementally. Below is an outline of how we could redesign the API completely it in one go without having to break or rewrite lots of stuff. 1. Design a fresh set of APIs, one for each "transport" or function of an adapter. The APIs should correspond exactly to the features of the function, no more, no less. For example, the jtag API should have functions to perform statemoves, ir and dr scans, setting a desired frequency and so on. It should NOT include any reset signal handling, since such features are outside the JTAG spec. Reset signals should be exposed via a separate gpio API, along with any other signal an adapter may support. The APIs should, where relevant, include session handling. For example, and adapter driver may provide both a jtag and an swd interface, but opening a jtag session should fail if there's already an swd session open, if they're mutually exclusive. An important aspect of the APIs is that no assumption can be made as to WHEN the adapter driver actually performs the operation. Different adapters have different requirements. For a USB type of adapter, operations must be batched for efficiency reasons, but if OpenOCD is running on the adapter itself and have direct access to I/Os it makes most sense to perform the operations synchronously. Therefore, the APIs should be designed such that operations are not guaranteed to have executed until a flush/execute-function is called. It could be taken a step further than the current jtag queue API by not even returning errors from operations but instead discarding further operations if there is an error in executing a previous one. Much like the sticky error in ADIv5. The error message is returned and the error cleared by the flush function. 2. Create a minidriver that implements the legacy minidriver API in terms of the new jtag API and the reset parts of the gpio API. 3. Create a new style "adapter" that implements the jtag and gpio API using the legacy jtag queue API (in effect calling into a replica of driver.c but with its functions renamed to not clash with the minidriver API). 4. With this in place, all legacy targets and adapter drivers should be usable as before, but with the new APIs sitting in the loop. This way the (JTAG parts of the) API can be tested so it's possible to implement all the needed features over it. (But no adding awkward features to the new APIs because of odd target requirements that can be worked around in the target.) Life cycle management, Jim hooks and configuration commands can be tested at this point. And the conversion of target and adapter drivers is decoupled because old style and new style drivers can be used interchangeably. 5. Convert some target to call the new APIs directly. The Cortex-M (ADIv5) target is a good candidate because it's a multi-transport target which is widely used and well working. 6. Convert (or create) a real adapter driver to implement the new APIs directly. The ftdi driver is a good candidate because it provides low-level access, enabling virtually any transport to be implemented. A new CMSIS-DAP driver could be another suitable option. 7. Implement another transport for the selected target and adapter, such as swd or high level dap (as used in CMSIS-DAP) and the necessary infrastructure to select which to use (the existing transport framework *may* help here). The first point should be where most of the effort should be put and the APIs probably need to evolve over several iterations of the above. I just added some comments below, not necessarily directed at your proposal as most of the stuff we have already. Just highlighting some thoughts I have about the API. > Proposed interface driver prototypes: > { > char *name; > const char **transports; > const struct swd_driver *swd; I haven't got a clear picture of how the transport framework was supposed to be used or what it's trying to solve and I'm afraid we won't get any help from it's author. On the bright side, since nothing is really using it, we're free to include it in the API or not, or tweak it if it could be made to fit. > const struct command_registration *commands; Why can't the adapter constructor register the commands it wants to provide and unreg them when it's destroyed? I haven't looked much at the command framework, but I feel it's backwards. > int (*create)(Jim_GetOptInfo *options, int > interface_num, adapter_handle_t *new_handle); > int (*destroy)(adapter_handle_t handle); I think these two hint at the dynamic nature you want to introduce. That's good. Adapters should be able to dynamically added or removed. The adapter driver life cycle is probably one of the most important design points. > int (*init)(adapter_handle_t handle); > int (*quit)(adapter_handle_t handle); What did you have in mind for these? I'm not sure we need another pair of constructors/destructors. > int (*speed)(adapter_handle_t handle, int speed); Speed is a transport specific concept. > int (*power_dropout)(adapter_handle_t handle, int > *power_dropout); > int (*srst_asserted)(adapter_handle_t handle, int > *srst_asserted); These are very specific functions that not even a jtag adapter needs to have. Reset signals, power detection/enabling and other I/O or event related activity should be handled with separate API(s). > /* JTAG functions - to be split out into separate > structure soon */ > int (*execute_queue)(adapter_handle_t handle, struct > jtag_command *cmd_queue); Queuing doesn't belong in the adapter API. > int (*khz)(adapter_handle_t handle, int khz, int > *jtag_speed); > int (*speed_div)(adapter_handle_t handle, int speed, > int *khz); We really don't need three separate functions to set the operating frequency. > }; I'm thinking something along the lines of: /* the "vtable" which each driver populates with the transports it * supports, leaving the rest null */ struct adapter_ops { void (*destroy)(void); const struct jtag_ops *jtag; const struct swd_ops *swd; const struct dap_ops *dap; const struct gpio_ops *gpio; /* ... add more as required */ }; /* the instance struct which is allocated, initialized and returned by * an adapter driver's constructor */ struct adapter { const char *name; const struct adapter_ops *ops; /* any other per-instance data that may be needed, if any */ }; /* an example swd api */ struct swd_ops { struct swd_session *(*open_session)(struct adapter *, ...); void (*close_session)(struct swd_session *); void (*read_reg)(struct swd_session *, uint8_t cmd, uint32_t *value); void (*write_reg)(struct swd_session *,uint8_t cmd, uint32_t value); int (*flush)(struct swd_session *); /* ... */ }; /Andreas ------------------------------------------------------------------------------ Everyone hates slow websites. So do we. Make your web apps faster with AppDynamics Download AppDynamics Lite for free today: http://p.sf.net/sfu/appdyn_d2d_nov _______________________________________________ OpenOCD-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openocd-devel
