<karlp>
I think the decision making processis mostly $ and laziness, not "can we absovel ourselves of resposibily via this method"
<karlp>
at least in my experience.
tsal has quit [Ping timeout: 240 seconds]
tsal has joined #openocd
nerozero has joined #openocd
Hawk777 has joined #openocd
Hawk777 has quit [Quit: Leaving.]
crabbedhaloablut has joined #openocd
<olerem>
zear: hi. how about introducing a new cpu target to avoid this kind of regression? Rest of the code can be reused as is.
<olerem>
zear: do it a similar ways as it is done for kernel
<zear>
I see
<zear>
thanks for the review :)
firelegend has joined #openocd
<zear>
olerem, do you have any pointers to what you have in mind? E.g. am I supposed to extend `static struct target_type *target_types[]` by a new `mips_xburst_target`, as opposed to using the existing `mips_m4k_target`?
<olerem>
zear: yes.
<olerem>
zear: or do you see any way to autodetect the xburst within the m4k code?
<zear>
I'm looking for that way right now. The thing with creating a new target that borrows all the logic from mips_m4k_target is, that m4k's implementation is all static, so I'd need to create a new mips32_common.h header and add the function prototypes, just so I can create a new target and change a false to true in one line :)
<olerem>
zear: just adding extra target to the src/target/mips_m4k.c ?
<zear>
that could work, if it's not considered too dirty
<olerem>
zear: i see it as one driver supporting multiple compatibles
<zear>
you should be able to detect xburst by reading the coprocessor registers
<zear>
I see that Linux is doing exactly that
<olerem>
zear: is it possible to do it in a non destructive maner to avoid conflict with debug system?
<zear>
I have no idea :)
<zear>
do you mean that I need to decide whether to use m4k or xburst before I can access CPU registers?
<olerem>
zear: yes
<olerem>
zear: imagine, you need to debug some system adhock. Something happenend any you need to extract needed information. In this case you wont to preserve content of all registers and not change state of anything before starting to debug
<zear>
won't the jtag connection need to preserve the existing context in order to even initialize itself?
<zear>
e.g. it's either being already preserved, or destroyed on jtag connection
<olerem>
zear: yes, your right. we try to store some registers here mips32_pracc_read_regs()
<zear>
I am thinking of hooking up somewhere in mips_m4k_examine
<zear>
just keep in mind this is the first time I am looking into the way ejtag debug mode works
<zear>
so I don't understand the big picture yet
<olerem>
zear: no problem
<zear>
alternatively, we could probably derive this info from the TAP:
<olerem>
zear: the problem is, there are different of MIPS implementations made not by MIPS. For example Broadcom MIPS (BMIPS), Loongson MIPS, Realtek (Lexra), etc. I'm 100% sure, all of this variants are not very compatible to each other. So, we will need to have different target soon or later
<olerem>
zear: part =0 0, ver: 0, is not very informative :)
<zear>
yea... the JTAG IDCODE returns 0x0
<zear>
oh well, I can add that new target in mips_m4k.c and call it done :)
<olerem>
zear: i assume it will be the best way. MIPS 4K != MIPS Xburst. It is just differnt beast.
<zear>
yep
<zear>
I will for now make it identical with M4K, except for different logic in mips_ejtag_exit_debug
<olerem>
zear: add as many comments as possible. Otherwise no one will know why things exist
<zear>
yep
firelegend has quit [Quit: Client closed]
<zear>
<zear> yea... the JTAG IDCODE returns 0x0
<zear>
disregard that, the IDCODE returned is 0x24f
<zear>
another jtag software, ejtagproxy, uses that to detect Ingenic devices:
<zear>
heh, Ingenic screwed up and they put the cp0 15, 0 (CPU id) info backwards
<zear>
it matches the idcode, but has the company id where cpu revision should be :)
<olerem>
BIT(0) is LSB bit which is always 1. 0x0000024f can fit to nealry any MIPS product
<zear>
the populated bits are in the ManufID mask
<zear>
so it narrows it down to any Ingenic product, at least
<olerem>
you mean MIPS, not Ingenic?
<zear>
I mean Ingenic
<zear>
oh wait, you are right:
<zear>
"The MIPS Technologies, Inc. Standard Manufacturer's
<zear>
Identification Code is 0x127."
<olerem>
i would not recommend use hack from ejtagproxy :)
<olerem>
It will add more confusion
<zear>
agreed
<zear>
but I'm also trying to avoid adding a new target that just re-uses m4k logic
<zear>
both because I don't know how much of it is correct, and because there's some user interface references to "m4k", despite selecting xburst target, afterwards
<olerem>
zear: i'm sure, initial patch wich will make part of xburst work, will not provide complete support. It is just somewhat initial. If there are more differences, we will have even more xburst specific patches.
<olerem>
zear: if it is not possible to detect it by the TAPid and CPUid, it is better to let user properly choice the target
<zear>
takes place in mips_m4k_debug_entry(), which already saves context
<zear>
and it already re-reads the config registers
<zear>
so it's just an extra cp0 register to re-read
<zear>
and having cp0 PRId stored might come in handy for other SoCs too
<olerem>
zear: hm, may be reuse this code arch/mips/kernel/cpu-probe.c ?
<zear>
I will check it shortly, but in the meantime I have yet another suggestion, just let me clean up the patch a little
<olerem>
with other words, ack for mips32_read_prid_reg, may be rename it to mips32_read_c0_prid() to make it compatible with kernel code. And port part of cpu_probe() and cpu_probe_ingenic(). I assume it should be enough and it will be solid base for other CPUs
<zear>
now let me inspect the code that you suggested
<olerem>
zear: looks good. No need to set quirks = 0. It is calloced.
<zear>
I guess same goes for prid = 0 there?
<olerem>
ack
<olerem>
and for all others, but it was before your patch
<zear>
yep
<olerem>
zear: please move cpu detection to a separate function
<zear>
sure
<olerem>
olerem: and print detected CPU varaint at LOG_DEBUG level
<zear>
do we want to print the numerical value, or an ASCII string (e.g. "INGENIC JZ4780")?
<olerem>
both. Are you sure you will be able to detect SoC variant based on PRid?
<zear>
<olerem> zear: hm, may be reuse this code arch/mips/kernel/cpu-probe.c ?
<zear>
heh, so I see I already did that in parallel :)
<olerem>
zear: ack. close to kernel code
<olerem>
kernel is able to detect Xburst CPU variant with some flags
<olerem>
probaly not actual SoC
<olerem>
lol "The config0 register in the XBurst CPUs with a processor ID of PRID_COMP_INGENIC_D0 or PRID_COMP_INGENIC_D1 has an abandoned huge page tlb mode, this mode is not compatible with the MIPS standard, it will cause tlbmiss and into an infinite loop ..."
<zear>
I have a bunch of those JZ47xx devices and only one of those has external JTAG pads (CI20, JZ4780 based)
<zear>
so for now I think it's enough we support this cpu variant only
<olerem>
Looks promising: "The config0 register in the XBurst CPUs with a processor ID of PRID_COMP_INGENIC_D0 report themselves as MIPS32r2 compatible but they don't actually support this ISA."
<zear>
> it will cause tlbmiss and into an infinite loop
<zear>
yikes!
<zear>
I'm glad that's not the one I'm trying to support
<olerem>
it looks like kernel comunity has more expiriance with this type of CPU. To reduce maintainance overhaead it is better to port kernel code as close as possible with all comments and a note where to look for updates
<olerem>
zear: if possible in a separate file, this code can be reused by mips64 target
<zear>
ack
<zear>
uh-oh, there's quite a lot of stuff those Linux files do that we don't need
<zear>
can't I keep it minimal on our end and just point to the Linux variants in a comment?
<zear>
I don't expect to ever support more than JZ4780
<zear>
as like I said, none of those devices (I am aware of) have JTAG pins exported anywhere on the PCB
<zear>
with the sole exception of MIPS Creator CI20 (JZ4780), the one I'm adding support for right now
<olerem>
zear: just take over what you need and can test, no need to support every thing. Just keep reduced version of cpu_probe() so other vendors can be added later and part of cpu_probe_ingenic(), only you CPU
<PaulFertser>
zear: hi. Did you keep Change-Id as they currently are on Gerrit?
<zear>
no, I started from scratch
<zear>
should I manually insert the old Change-Ids?
<zear>
in fact, the 2nd patch from the existing chain is obsolete and a different patch comes after the first one now
<PaulFertser>
zear: Gerrit would assign the patch to the same changset when Change-Id is kepts, so you want to keep the comments and the history for evolution of same code. If some patch isn't needed at all anymore tell me which one and I'll press "Abandon".
<zear>
Actually, scratch all of that - the new patches have very little in common with the old ones. In terms of Linux, these would be considered v2, because I'm still trying to implement the same feature. But in terms of code content, they're almost completely different.
<zear>
PaulFertser, I think you can abandon both of them and send in the new series as a separate chain