| Message ID | 20181109071011.253734-15-amitay@ozlabs.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Device tree path based targeting | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/snowpatch_job_snowpatch-pdbg | success | Test snowpatch/job/snowpatch-pdbg on branch master |
On Friday, 9 November 2018 6:10:10 PM AEDT Amitay Isaacs wrote: > +static bool scommable(struct pdbg_target *target) > { > - uint64_t value; > - > - if (pib_read(target, *addr, &value)) > - return 0; > + char *classname; > > - printf("p%d:0x%" PRIx64 " = 0x%016" PRIx64 "\n", index, *addr, value); > + classname = pdbg_target_class_name(target); > + if (!strcmp(classname, "pib") || > + !strcmp(classname, "core") || > + !strcmp(classname, "thread")) > + return true; As we've discussed/agreed this is suboptimal but will work for the time being until we come up with a better solution. > > - return 1; > + return false; > } <snip> > + /* TODO: Restore the <mask> functionality */ We really should fix that at some point :-) I will take a look at it once this series is done. > + if (pib_write(target, addr, data)) { > + printf("%s: failed\n", path); > + free(path); > + continue; > + } > + > + count++; > + } > + > + return count; > } > OPTCMD_DEFINE_CMD_WITH_ARGS(putscom, putscom, (ADDRESS, DATA, > DEFAULT_DATA("0xffffffffffffffff"))); diff --git a/tests/test_hw_bmc.sh > b/tests/test_hw_bmc.sh > index 9bc1deb..d656a3f 100755 > --- a/tests/test_hw_bmc.sh > +++ b/tests/test_hw_bmc.sh > @@ -91,7 +91,7 @@ do_skip > test_run $PDBG -P fsi getcfam 0xc09 > > test_result 0 <<EOF > -p0:0xf000f = HEX16 > +/kernelfsi@0/pib@1000: 0xf000f = HEX16 As mentioned in the previous patch I think for the time being we should try and maintain the same output if possible. I suspect printing the full path will just be confusing so perhaps we could add it as an option? Also I think we should print the complete translated SCOM address rather than the offset pdbg was called with. Thanks! - Alistair > EOF > > do_skip
On Tue, 2018-11-13 at 16:09 +1100, Alistair Popple wrote: > On Friday, 9 November 2018 6:10:10 PM AEDT Amitay Isaacs wrote: > > +static bool scommable(struct pdbg_target *target) > > { > > - uint64_t value; > > - > > - if (pib_read(target, *addr, &value)) > > - return 0; > > + char *classname; > > > > - printf("p%d:0x%" PRIx64 " = 0x%016" PRIx64 "\n", index, *addr, > > value); > > + classname = pdbg_target_class_name(target); > > + if (!strcmp(classname, "pib") || > > + !strcmp(classname, "core") || > > + !strcmp(classname, "thread")) > > + return true; > > As we've discussed/agreed this is suboptimal but will work for the > time being > until we come up with a better solution. Just to confirm, does getscom/putscom work on thread class? Or it's only pib and core classes? > > > - return 1; > > + return false; > > } > > <snip> > > > + /* TODO: Restore the <mask> functionality */ > > We really should fix that at some point :-) I will take a look at it > once this > series is done. > > > + if (pib_write(target, addr, data)) { > > + printf("%s: failed\n", path); > > + free(path); > > + continue; > > + } > > + > > + count++; > > + } > > + > > + return count; > > } > > OPTCMD_DEFINE_CMD_WITH_ARGS(putscom, putscom, (ADDRESS, DATA, > > DEFAULT_DATA("0xffffffffffffffff"))); diff --git > > a/tests/test_hw_bmc.sh > > b/tests/test_hw_bmc.sh > > index 9bc1deb..d656a3f 100755 > > --- a/tests/test_hw_bmc.sh > > +++ b/tests/test_hw_bmc.sh > > @@ -91,7 +91,7 @@ do_skip > > test_run $PDBG -P fsi getcfam 0xc09 > > > > test_result 0 <<EOF > > -p0:0xf000f = HEX16 > > +/kernelfsi@0/pib@1000: 0xf000f = HEX16 > > As mentioned in the previous patch I think for the time being we > should try > and maintain the same output if possible. I suspect printing the full > path > will just be confusing so perhaps we could add it as an option? Sure. This means that if the selected target is a core, then it will print "p0:c0:" and for a thread it will print "p0:c0:t0:". Can we do getscom/putscom on non pib/core/thread targets? If yes, what should be the prefix for those targets? > Also I think we should print the complete translated SCOM address > rather than > the offset pdbg was called with. Thanks! We need pdbg_target_address_translate() or something similar. IIRC, there was a patch that added a public wrapper around get_class_target_addr() but used a bad name. :-) I can add that patch back. Amitay.
On Wednesday, 14 November 2018 4:56:29 PM AEDT Amitay Isaacs wrote: > On Tue, 2018-11-13 at 16:09 +1100, Alistair Popple wrote: > > On Friday, 9 November 2018 6:10:10 PM AEDT Amitay Isaacs wrote: > > > +static bool scommable(struct pdbg_target *target) > > > > > > { > > > > > > - uint64_t value; > > > - > > > - if (pib_read(target, *addr, &value)) > > > - return 0; > > > + char *classname; > > > > > > - printf("p%d:0x%" PRIx64 " = 0x%016" PRIx64 "\n", index, *addr, > > > value); > > > + classname = pdbg_target_class_name(target); > > > + if (!strcmp(classname, "pib") || > > > + !strcmp(classname, "core") || > > > + !strcmp(classname, "thread")) > > > + return true; > > > > As we've discussed/agreed this is suboptimal but will work for the > > time being > > until we come up with a better solution. > > Just to confirm, does getscom/putscom work on thread class? Or it's > only pib and core classes? It depends :-) On P8 it theorectically could, P9 not so much. In practice it's unlikely to be ever used on P8 so until we come up with a better solution I think it's safe to conclude that get/putscom won't work on a thread class. > > > - return 1; > > > + return false; > > > > > > } > > > > <snip> > > > > > + /* TODO: Restore the <mask> functionality */ > > > > We really should fix that at some point :-) I will take a look at it > > once this > > series is done. > > > > > + if (pib_write(target, addr, data)) { > > > + printf("%s: failed\n", path); > > > + free(path); > > > + continue; > > > + } > > > + > > > + count++; > > > + } > > > + > > > + return count; > > > > > > } > > > OPTCMD_DEFINE_CMD_WITH_ARGS(putscom, putscom, (ADDRESS, DATA, > > > > > > DEFAULT_DATA("0xffffffffffffffff"))); diff --git > > > a/tests/test_hw_bmc.sh > > > b/tests/test_hw_bmc.sh > > > index 9bc1deb..d656a3f 100755 > > > --- a/tests/test_hw_bmc.sh > > > +++ b/tests/test_hw_bmc.sh > > > @@ -91,7 +91,7 @@ do_skip > > > > > > test_run $PDBG -P fsi getcfam 0xc09 > > > > > > test_result 0 <<EOF > > > > > > -p0:0xf000f = HEX16 > > > +/kernelfsi@0/pib@1000: 0xf000f = HEX16 > > > > As mentioned in the previous patch I think for the time being we > > should try > > and maintain the same output if possible. I suspect printing the full > > path > > will just be confusing so perhaps we could add it as an option? > > Sure. This means that if the selected target is a core, then it will > print "p0:c0:" and for a thread it will print "p0:c0:t0:". Actually that's more complicated than what I'm thinking. I was thinking along the lines of something like: # pdbg -P /fsi0/pib0/chiplet5 getscom 0x1010 p0:0x5001010 = 0xdeadbeef Where 0x5001010 is the final translated address that is actually read/written. However now that I've typed that out I can see a problem with that approach - when multiple targets are selected it would be hard to correlate which was which. So perhaps if you're specifying the path it would imply that you shouldn't be confused by it in a print out. I'd still like to see the complete translated address in the outut though. So how about we do this: 1. If -P is specified print the full path and complete translated address. eg: # pdbg -P /fsi0/pib0/chiplet5 getscom 0x1010 /fsi0/pib0/chiplet5:0x5001010 = 0xdeadbeef 2. If -p/-c is specified print the same short-hand notation (p0:c0:<addr> = <data>) > Can we do getscom/putscom on non pib/core/thread targets? If yes, what > should be the prefix for those targets? Yes, but lets not worry about prefixing those. Lets just print the path in that case. My primary concern was changing the output of something like: # pdbg -p0 getscom 0xf000f From `p0:0xf000f = <data>` to `/fsi0/pib1000 = <data>` which I suspect may confuse existing users. Anyone who figures out how to specify a path wont (or at least shouldn't) be surprised to see it in the output. > > Also I think we should print the complete translated SCOM address > > rather than > > the offset pdbg was called with. Thanks! > > We need pdbg_target_address_translate() or something similar. IIRC, > there was a patch that added a public wrapper around > get_class_target_addr() but used a bad name. :-) Yeah. But "somebody" said it was a bad API and we shouldn't need it :-) > I can add that patch back. Turns out that somebody was correct though and we need a better name and API. Instead of manually specifying the target address class the API should just take target and address offset and magically return the translated address for the address space it belongs to. Something like: uint64_t target_parent_addr(struct pdbg_target *target, uint64_t offset) Although happy to bike shed names. For the moment the "magic" could just be hardcoded to check scommable() and use the existing get_class_target_addr(tgt, "pib", offset). We can add better magic once this series is done. - Alistair > Amitay.
On Wed, 2018-11-14 at 17:30 +1100, Alistair Popple wrote: > On Wednesday, 14 November 2018 4:56:29 PM AEDT Amitay Isaacs wrote: > > On Tue, 2018-11-13 at 16:09 +1100, Alistair Popple wrote: > > > On Friday, 9 November 2018 6:10:10 PM AEDT Amitay Isaacs wrote: > > > > +static bool scommable(struct pdbg_target *target) > > > > > > > > { > > > > > > > > - uint64_t value; > > > > - > > > > - if (pib_read(target, *addr, &value)) > > > > - return 0; > > > > + char *classname; > > > > > > > > - printf("p%d:0x%" PRIx64 " = 0x%016" PRIx64 "\n", index, > > > > *addr, > > > > value); > > > > + classname = pdbg_target_class_name(target); > > > > + if (!strcmp(classname, "pib") || > > > > + !strcmp(classname, "core") || > > > > + !strcmp(classname, "thread")) > > > > + return true; > > > > > > As we've discussed/agreed this is suboptimal but will work for > > > the > > > time being > > > until we come up with a better solution. > > > > Just to confirm, does getscom/putscom work on thread class? Or > > it's > > only pib and core classes? > > It depends :-) > > On P8 it theorectically could, P9 not so much. In practice it's > unlikely to be > ever used on P8 so until we come up with a better solution I think > it's safe > to conclude that get/putscom won't work on a thread class. > > > > > - return 1; > > > > + return false; > > > > > > > > } > > > > > > <snip> > > > > > > > + /* TODO: Restore the <mask> functionality */ > > > > > > We really should fix that at some point :-) I will take a look at > > > it > > > once this > > > series is done. > > > > > > > + if (pib_write(target, addr, data)) { > > > > + printf("%s: failed\n", path); > > > > + free(path); > > > > + continue; > > > > + } > > > > + > > > > + count++; > > > > + } > > > > + > > > > + return count; > > > > > > > > } > > > > OPTCMD_DEFINE_CMD_WITH_ARGS(putscom, putscom, (ADDRESS, DATA, > > > > > > > > DEFAULT_DATA("0xffffffffffffffff"))); diff --git > > > > a/tests/test_hw_bmc.sh > > > > b/tests/test_hw_bmc.sh > > > > index 9bc1deb..d656a3f 100755 > > > > --- a/tests/test_hw_bmc.sh > > > > +++ b/tests/test_hw_bmc.sh > > > > @@ -91,7 +91,7 @@ do_skip > > > > > > > > test_run $PDBG -P fsi getcfam 0xc09 > > > > > > > > test_result 0 <<EOF > > > > > > > > -p0:0xf000f = HEX16 > > > > +/kernelfsi@0/pib@1000: 0xf000f = HEX16 > > > > > > As mentioned in the previous patch I think for the time being we > > > should try > > > and maintain the same output if possible. I suspect printing the > > > full > > > path > > > will just be confusing so perhaps we could add it as an option? > > > > Sure. This means that if the selected target is a core, then it > > will > > print "p0:c0:" and for a thread it will print "p0:c0:t0:". > > Actually that's more complicated than what I'm thinking. I was > thinking along > the lines of something like: > > # pdbg -P /fsi0/pib0/chiplet5 getscom 0x1010 > p0:0x5001010 = 0xdeadbeef > > Where 0x5001010 is the final translated address that is actually > read/written. > However now that I've typed that out I can see a problem with that > approach - > when multiple targets are selected it would be hard to correlate > which was > which. So perhaps if you're specifying the path it would imply that > you > shouldn't be confused by it in a print out. I'd still like to see the > complete > translated address in the outut though. > > So how about we do this: > 1. If -P is specified print the full path and complete translated > address. eg: > > # pdbg -P /fsi0/pib0/chiplet5 getscom 0x1010 > /fsi0/pib0/chiplet5:0x5001010 = 0xdeadbeef > > 2. If -p/-c is specified print the same short-hand notation > (p0:c0:<addr> = > <data>) > If we print the translated address then it makes sense to print the target relative to which the address is translated. So even for chipsets specified with -P, printing "p0:<xlate-addr>" makes sense. Printing the translated address may be a way to confirm that the address translation is working correctly, specially once we add weird translation schemes. Is that the motivation behind printing translated addresses? To be consistent we have to use either of the following formats. 1. /fsi0/pib0/chiplet5: 0x1010 = 0xdeadbeef This makes the explicit the target and the scom address specified. 2. p0: 0x5001010 = 0xdeadbeef This is more engineering output which tells what address space the scom address falls under and the actual translated address relative to that address space. The first format is advantageous if there are multiple targets printed: /fsi0/pib0/chiplet1: 0x1010 = 0xdeadbeef /fsi0/pib0/chiplet3: 0x1010 = 0xdeadbeef /fsi0/pib0/chiplet5: 0x1010 = 0xdeadbeef Rather than, p0: 0x1001010 = 0xdeadbeef p0: 0x3001010 = 0xdeadbeef p0: 0x5001010 = 0xdeadbeef 3. May be we want both formats and can be specified by user. We can add an option (e.g. -x) to print translated address. 4. How about combined output? May be too verbose. p0: 0x1001010 = 0xdeadbeef (/fsi0/pib0/chiplet1) p0: 0x3001010 = 0xdeadbeef (/fsi0/pib0/chiplet3) p0: 0x5001010 = 0xdeadbeef (/fsi0/pib0/chiplet5) > > Can we do getscom/putscom on non pib/core/thread targets? If yes, > > what > > should be the prefix for those targets? > > Yes, but lets not worry about prefixing those. Lets just print the > path in that > case. My primary concern was changing the output of something like: > > # pdbg -p0 getscom 0xf000f > > From `p0:0xf000f = <data>` to `/fsi0/pib1000 = <data>` which I > suspect may > confuse existing users. Anyone who figures out how to specify a path > wont (or > at least shouldn't) be surprised to see it in the output. > > > > Also I think we should print the complete translated SCOM address > > > rather than > > > the offset pdbg was called with. Thanks! > > > > We need pdbg_target_address_translate() or something similar. IIRC, > > there was a patch that added a public wrapper around > > get_class_target_addr() but used a bad name. :-) > > Yeah. But "somebody" said it was a bad API and we shouldn't need it > :-) > > > I can add that patch back. > > Turns out that somebody was correct though and we need a better name > and API. > Instead of manually specifying the target address class the API > should just > take target and address offset and magically return the translated > address for > the address space it belongs to. Something like: > > uint64_t target_parent_addr(struct pdbg_target *target, uint64_t > offset) > > Although happy to bike shed names. For the moment the "magic" could > just be pdbg_address_relative(), or pdbg_address_translate() > hardcoded to check scommable() and use the existing > get_class_target_addr(tgt, > "pib", offset). We can add better magic once this series is done. If we do automagic translation relative to the correct address space, I think the api should also return the base target whose address space the scom address resides in. Amitay.
On Thursday, 15 November 2018 12:05:52 PM AEDT Amitay Isaacs wrote: > > So how about we do this: > > 1. If -P is specified print the full path and complete translated > > address. eg: > > > > # pdbg -P /fsi0/pib0/chiplet5 getscom 0x1010 > > /fsi0/pib0/chiplet5:0x5001010 = 0xdeadbeef > > > > 2. If -p/-c is specified print the same short-hand notation > > (p0:c0:<addr> = > > <data>) > > If we print the translated address then it makes sense to print the > target relative to which the address is translated. So even for > chipsets specified with -P, printing "p0:<xlate-addr>" makes sense. > > Printing the translated address may be a way to confirm that the > address translation is working correctly, specially once we add weird > translation schemes. Is that the motivation behind printing translated > addresses? Yep, I should probably have been clearer about the motivation. In general when debugging systems I've found it to be good not to hide magic - specifying a path+offset is really just a convenient way of specifying a specific SCOM operation, but it should always report the exact operation that was done (ie. getscom <xlate-addr>) It may also help new users figure out what specifying a path actually does. > To be consistent we have to use either of the following formats. > > 1. /fsi0/pib0/chiplet5: 0x1010 = 0xdeadbeef > > This makes the explicit the target and the scom address specified. > > 2. p0: 0x5001010 = 0xdeadbeef > > This is more engineering output which tells what address space the scom > address falls under and the actual translated address relative to that > address space. > > The first format is advantageous if there are multiple targets printed: > > /fsi0/pib0/chiplet1: 0x1010 = 0xdeadbeef > /fsi0/pib0/chiplet3: 0x1010 = 0xdeadbeef > /fsi0/pib0/chiplet5: 0x1010 = 0xdeadbeef > > Rather than, > > p0: 0x1001010 = 0xdeadbeef > p0: 0x3001010 = 0xdeadbeef > p0: 0x5001010 = 0xdeadbeef > > 3. May be we want both formats and can be specified by user. We can > add an option (e.g. -x) to print translated address. > > 4. How about combined output? May be too verbose. > > p0: 0x1001010 = 0xdeadbeef (/fsi0/pib0/chiplet1) > p0: 0x3001010 = > 0xdeadbeef (/fsi0/pib0/chiplet3) > p0: 0x5001010 = 0xdeadbeef > (/fsi0/pib0/chiplet5) Actually I kinda like the combined output. I don't think it's too verbose or any harder to read as the full paths should be aligned in a column anyway. > > > Can we do getscom/putscom on non pib/core/thread targets? If yes, > > > what > > > should be the prefix for those targets? > > > > Yes, but lets not worry about prefixing those. Lets just print the > > path in that > > case. My primary concern was changing the output of something like: > > > > # pdbg -p0 getscom 0xf000f > > > > From `p0:0xf000f = <data>` to `/fsi0/pib1000 = <data>` which I > > suspect may > > confuse existing users. Anyone who figures out how to specify a path > > wont (or > > at least shouldn't) be surprised to see it in the output. > > > > > > Also I think we should print the complete translated SCOM address > > > > rather than > > > > the offset pdbg was called with. Thanks! > > > > > > We need pdbg_target_address_translate() or something similar. IIRC, > > > there was a patch that added a public wrapper around > > > get_class_target_addr() but used a bad name. :-) > > > > Yeah. But "somebody" said it was a bad API and we shouldn't need it > > > > :-) > > : > > > I can add that patch back. > > > > Turns out that somebody was correct though and we need a better name > > and API. > > Instead of manually specifying the target address class the API > > should just > > take target and address offset and magically return the translated > > address for > > the address space it belongs to. Something like: > > > > uint64_t target_parent_addr(struct pdbg_target *target, uint64_t > > offset) > > > > Although happy to bike shed names. For the moment the "magic" could > > just be > > pdbg_address_relative(), or > pdbg_address_translate() pdbg_address_absolute()? pdbg_address_translate() works as well though. relative seems confusing to me because your passing in a relative address. > > hardcoded to check scommable() and use the existing > > get_class_target_addr(tgt, > > "pib", offset). We can add better magic once this series is done. > > If we do automagic translation relative to the correct address space, I > think the api should also return the base target whose address space > the scom address resides in. Excellent point, I agree. So basically just an automagic version of the existing get_class_target_addr(). For the moment we can keep the implementation simple. Something like the below psuedo-code should work: struct target *pdbg_address_absolute(tgt, offset, addr) { assert(pdbg_target_class(tgt) == "pib", "chiplet", "xbus", <rest of the scommable classes>, ...); tgt = get_class_target_addr(tgt, "pib", &addr); return tgt; } I can start working on more generalised "magic". - Alistair > Amitay.
diff --git a/src/scom.c b/src/scom.c index 2372e91..ad70523 100644 --- a/src/scom.c +++ b/src/scom.c @@ -18,41 +18,92 @@ #include <stdlib.h> #include <string.h> #include <inttypes.h> +#include <assert.h> #include <libpdbg.h> #include "main.h" #include "optcmd.h" +#include "path.h" -static int _getscom(struct pdbg_target *target, uint32_t index, uint64_t *addr, uint64_t *unused) +/* Check if a target has scom region */ +static bool scommable(struct pdbg_target *target) { - uint64_t value; - - if (pib_read(target, *addr, &value)) - return 0; + char *classname; - printf("p%d:0x%" PRIx64 " = 0x%016" PRIx64 "\n", index, *addr, value); + classname = pdbg_target_class_name(target); + if (!strcmp(classname, "pib") || + !strcmp(classname, "core") || + !strcmp(classname, "thread")) + return true; - return 1; + return false; } - int getscom(uint64_t addr) +int getscom(uint64_t addr) { - return for_each_target("pib", _getscom, &addr, NULL); + struct pdbg_target *target; + char *path; + uint64_t value; + int count = 0; + + for_each_path_target(target) { + if (pdbg_target_status(target) != PDBG_TARGET_ENABLED) + continue; + + path = pdbg_target_path(target); + assert(path); + + if (!scommable(target)) { + printf("%s: invalid target\n", path); + free(path); + continue; + } + + if (pib_read(target, addr, &value)) { + printf("%s: failed\n", path); + free(path); + continue; + } + + printf("%s: 0x%" PRIx64 " = 0x%016" PRIx64 "\n", path, addr, value); + free(path); + count++; + } + + return count; } OPTCMD_DEFINE_CMD_WITH_ARGS(getscom, getscom, (ADDRESS)); -static int _putscom(struct pdbg_target *target, uint32_t index, uint64_t *addr, uint64_t *data) +int putscom(uint64_t addr, uint64_t data, uint64_t mask) { - if (pib_write(target, *addr, *data)) - return 0; + struct pdbg_target *target; + char *path; + int count = 0; - return 1; -} + for_each_path_target(target) { + if (pdbg_target_status(target) != PDBG_TARGET_ENABLED) + continue; - int putscom(uint64_t addr, uint64_t data, uint64_t mask) -{ - /* TODO: Restore the <mask> functionality */ - return for_each_target("pib", _putscom, &addr, &data); + path = pdbg_target_path(target); + assert(path); + + if (!scommable(target)) { + printf("%s: invalid target\n", path); + free(path); + continue; + } + + /* TODO: Restore the <mask> functionality */ + if (pib_write(target, addr, data)) { + printf("%s: failed\n", path); + free(path); + continue; + } + + count++; + } + + return count; } OPTCMD_DEFINE_CMD_WITH_ARGS(putscom, putscom, (ADDRESS, DATA, DEFAULT_DATA("0xffffffffffffffff"))); diff --git a/tests/test_hw_bmc.sh b/tests/test_hw_bmc.sh index 9bc1deb..d656a3f 100755 --- a/tests/test_hw_bmc.sh +++ b/tests/test_hw_bmc.sh @@ -91,7 +91,7 @@ do_skip test_run $PDBG -P fsi getcfam 0xc09 test_result 0 <<EOF -p0:0xf000f = HEX16 +/kernelfsi@0/pib@1000: 0xf000f = HEX16 EOF do_skip
Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> --- src/scom.c | 87 +++++++++++++++++++++++++++++++++++--------- tests/test_hw_bmc.sh | 2 +- 2 files changed, 70 insertions(+), 19 deletions(-)