On Sun, 2009-10-25 at 10:56 +0200, Øyvind Harboe wrote: > btw, the mrc/mcr work was just a natural followup on > the work I did on physical memory read/write. > > It's something that's supported very differently across lots > of targets today, but really should be handled in the same > manner.
Yes, by keeping it in a branch until the complete series has been ratified by the community. ;) > On Sun, Oct 25, 2009 at 5:31 AM, Zach Welch <[email protected]> wrote: > > On Sat, 2009-10-24 at 15:03 +0200, Øyvind Harboe wrote: > >> Attached are the remaining patches for the first round of > >> target->type->mcr/mrc support. > >> > >> There is a writeup in TODO of the harder targets > >> that remain. E.g. arm966e support requires good knowledge > >> of that target + ideally access to test hardware, so that's not > >> something I can or should attempt at this point. > >> > >> Old cp15 commands still work and should only be retired once > >> the new mrc/mcr can be tested on actual targets. > > > > If there are existing commands, then this feature definitely should have > > been committed once it was complete, after a series of patches to > > gracefully cut over to the new system had been developed and tested. > > Generally yes, but take a careful look at xscale and arm966e > specifically. They can both support mcr/mrc, but it is unrealistic > to try to do this in a single patch. The job is too big, it would > never have been done that way, certainly I would be unable > to attempt it. But you could do what you did and publish the branch publicly. Others can then add the support for other targets as required. > I *did* complete all the cases that could be relatively easily > done with the latest round of patches, so I claim to have lived up > to your requirement above. But you said it yourself: we now have two functionally identical sets of commands. I claim a series is not complete until it both implements the new alternative and also retires the old one. > > I am striving to gain a better perspective of the system at this level, but > > it seems that your present implementation weakens OpenOCD's > > architecture and thus creates a new barrier to improving it. > > Could you be more specific w.r.t. mrc/mcr? OpenOCD's target support should be aiming for architecture neutrality, but these commands are ARM-specific. That moves us away from our goal, making OpenOCD's architecture more difficult to abstract later. > As soon as each of the implementations of mrc/mcr can be tested, > we can retire the special commands for that target. This means > that knowledge for one target will be reusable on another, > reducing barriers to improving it. The mrc/mcr shows that the > targets really are a lot more similar than the copy & modified > code would have us believe... I am not saying that your patches do not bring improvement, but rather that they should have been finished before being pushed. > > Obviously, I am not trying to discourage you from continuing to work on > > these features, but this should underscore the notion that only complete > > and reviewed work should be pushed. > > I agree with this principle and I agree that I was too quick with mcr/mrc. > > However, to me it was it was a natural extension of the physical memory > read/write polymorphism work and didn't occur to me as very controversial. > Especially in light of the positive reception that the physical memory > read/write got. I think that may also prove architecturally shortsighted, but other architectures than ARM require this support. It was a far better candidate for inclusion at the target.h level. > With the latest patches I believe that I've found a "local optimium" > where it is time to commit and find some other time where e.g. XScale > and arm966e can be attacked. XScale and arm966e require significant > insight into the target and preferably access to target hardware. I agree, but "commit" and "push" do not need to mean the SF.net repo. > Note that the mrc/mcr does not change any of the existing code or > commands, so while it's "noise" until the old code can be retired, > the chance of regressions are minimal. But it is noise. That detracts from the release. Again, it should have been kept out of the master branch until all of these changes could be pushed as a set. > > Please publish a mirror on repo.or.cz and push this branch regularly so > > others can pull it for testing. > > I find it hard to believe that anyone would step up and do the > testing that way. There is very little evidence to the effect. > > A more realistic testing plan would be to wait until 0.3 is out of > the door and then switch over e.g. arm926ejs to retire the > cp15 specific commands. This model reflects the best practices of distributed development, and it is used by the Linux kernel developers. So, I beg to differ. > >> This whole mrc/mcr thing is about driving > >> OpenOCD in the direction of polymorphic interfaces > >> at the C and command level, ref recent "mww phys" > >> work. > > > > Is this the right level though? Anything that is specific to ARM should > > not be in target.[ch]. > > If the current programming model supported the concepts > of interfaces, then this would have gone into some arm > generic files. It does support the concept. I think we lack a clear concept of how and here they should be layered. > We have a separate problem(which I won't try to attempt solving > anytime soon) in that we have a very weak > model(not any really) to define an interface which a target > can either support or not. I wasn't trying to address this problem > here, but I wrote the code to be trivially converted to any > such scheme that is worth it's salt. This was the "perspective" to which I referred above. I think it deserves to be addressed *before* things like mrc/mcr are added. > Such interface is stuff is trivial in C++/java, but I haven't > seen it done elegantly in C yet. I hear it can be done > and certainly well enough for OpenOCD's simple needs. It's definitely possible, and I am slowly working toward this type of cleanup effort. > I had a crack at interface stuff when I made the arm > instruction simulator work w/arm11 to implement > single stepping when there was no hardware support > for single stepping, perhaps that would have been a way > to go? (combined with some method in target to query > for an interface pointer by ID?) I am not sure what to make of this but the process is simple: 1) Fix the code. 2) Post patches for review (for more than 24 hours!!). 3) If the community objects to your patches, goto step 1. The idea of having someone push your patches for you appeals to me, because it prevents you from prematurely bypassing step 3. Cheers, Zach _______________________________________________ Openocd-development mailing list [email protected] https://lists.berlios.de/mailman/listinfo/openocd-development
