| Message ID | 20181109071011.253734-14-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 |
Hi Amitay, > > test_result 0 <<EOF > -p0:0xc09 = HEX8 > +/kernelfsi@0: 0xc09 = HEX8 We may have discussed this sorry if I'm changing my mind but can we keep the output the same for the time being? ie. Something like ("p%d", pdbg_target_index(fsi))? > EOF > > do_skip > -test_run $PDBG -p0 getcfam 0xc09 This test should still pass though? Do you think it would it be better to just add another test or are you do think the other selection tests cover this case adequately? - Alistair > +test_run $PDBG -P fsi getcfam 0xc09 > > test_result 0 <<EOF > p0:0xf000f = HEX16
On Tue, 2018-11-13 at 16:01 +1100, Alistair Popple wrote: > Hi Amitay, > > > test_result 0 <<EOF > > -p0:0xc09 = HEX8 > > +/kernelfsi@0: 0xc09 = HEX8 > > We may have discussed this sorry if I'm changing my mind but can we > keep the > output the same for the time being? ie. Something like ("p%d", > pdbg_target_index(fsi))? Well the question is what targets we want for getcfam/putcfam? As per your thinking, if we want pib target to be selected for getcfam/putcfam commands, then the the existing output makes sense. In that case, the implementation should find the fsi target for selected pib target for actual operations. Does that work? > > > EOF > > > > do_skip > > -test_run $PDBG -p0 getcfam 0xc09 > > This test should still pass though? Do you think it would it be > better to just > add another test or are you do think the other selection tests cover > this case > adequately? > > - Alistair > > > +test_run $PDBG -P fsi getcfam 0xc09 > > > > test_result 0 <<EOF > > p0:0xf000f = HEX16 Amitay.
On Wednesday, 14 November 2018 4:42:59 PM AEDT Amitay Isaacs wrote: > On Tue, 2018-11-13 at 16:01 +1100, Alistair Popple wrote: > > Hi Amitay, > > > > > test_result 0 <<EOF > > > > > > -p0:0xc09 = HEX8 > > > +/kernelfsi@0: 0xc09 = HEX8 > > > > We may have discussed this sorry if I'm changing my mind but can we > > keep the > > output the same for the time being? ie. Something like ("p%d", > > pdbg_target_index(fsi))? > > Well the question is what targets we want for getcfam/putcfam? I think just the root/top level fsi targets are fine for now. In practice we don't have any FSI children that would need translation atm. > As per your thinking, if we want pib target to be selected for > getcfam/putcfam commands, then the the existing output makes sense. > In that case, the implementation should find the fsi target for > selected pib target for actual operations. I don't think we actually want the pib targets selected. I'm really just talking about the output formatting which it would be nice to keep the same - ie: "p<chip index>:<addr> = <value>" - Alistair > Does that work? > > > > EOF > > > > > > do_skip > > > > > > -test_run $PDBG -p0 getcfam 0xc09 > > > > This test should still pass though? Do you think it would it be > > better to just > > add another test or are you do think the other selection tests cover > > this case > > adequately? > > > > - Alistair > > > > > +test_run $PDBG -P fsi getcfam 0xc09 > > > > > > test_result 0 <<EOF > > > p0:0xf000f = HEX16 > > Amitay.
On Thu, 2018-11-15 at 15:13 +1100, Alistair Popple wrote: > On Wednesday, 14 November 2018 4:42:59 PM AEDT Amitay Isaacs wrote: > > On Tue, 2018-11-13 at 16:01 +1100, Alistair Popple wrote: > > > Hi Amitay, > > > > > > > test_result 0 <<EOF > > > > > > > > -p0:0xc09 = HEX8 > > > > +/kernelfsi@0: 0xc09 = HEX8 > > > > > > We may have discussed this sorry if I'm changing my mind but can > > > we > > > keep the > > > output the same for the time being? ie. Something like ("p%d", > > > pdbg_target_index(fsi))? > > > > Well the question is what targets we want for getcfam/putcfam? > > I think just the root/top level fsi targets are fine for now. In > practice we > don't have any FSI children that would need translation atm. > > > As per your thinking, if we want pib target to be selected for > > getcfam/putcfam commands, then the the existing output makes sense. > > In that case, the implementation should find the fsi target for > > selected pib target for actual operations. > > I don't think we actually want the pib targets selected. I'm really > just > talking about the output formatting which it would be nice to keep > the same - > ie: > > "p<chip index>:<addr> = <value>" I find it a bit confusing since 'p' is used to indicate pib class (-p). It's also the same as processor (or chip). For example, how do you do getcfam on second chip? (Does that even make sense? Read more pdfs Amitay!) If we need fsi target, then may be "-P fsi1" will work provided we have the second fsi target with correct index. If there is no fsi target with index 1, then we might have to actually know the topology to select the right fsi target (e.g. "-P fsi/fsi"). In that how do we generate the output "p1: <addr> = <value>"? It's more intuitive if we always use pib as a package/processor/chip. It's easier to either use "-p 1" or "-P pib1" for second processor/chip and then we can internally pick the "right" target to actually perform the fsi_read() operation. Amitay.
On Thursday, 15 November 2018 3:31:07 PM AEDT Amitay Isaacs wrote: > On Thu, 2018-11-15 at 15:13 +1100, Alistair Popple wrote: > > On Wednesday, 14 November 2018 4:42:59 PM AEDT Amitay Isaacs wrote: > > > On Tue, 2018-11-13 at 16:01 +1100, Alistair Popple wrote: > > > > Hi Amitay, > > > > > > > > > test_result 0 <<EOF > > > > > > > > > > -p0:0xc09 = HEX8 > > > > > +/kernelfsi@0: 0xc09 = HEX8 > > > > > > > > We may have discussed this sorry if I'm changing my mind but can > > > > we > > > > keep the > > > > output the same for the time being? ie. Something like ("p%d", > > > > pdbg_target_index(fsi))? > > > > > > Well the question is what targets we want for getcfam/putcfam? > > > > I think just the root/top level fsi targets are fine for now. In > > practice we > > don't have any FSI children that would need translation atm. > > > > > As per your thinking, if we want pib target to be selected for > > > getcfam/putcfam commands, then the the existing output makes sense. > > > In that case, the implementation should find the fsi target for > > > selected pib target for actual operations. > > > > I don't think we actually want the pib targets selected. I'm really > > just > > talking about the output formatting which it would be nice to keep > > the same - > > ie: > > > > "p<chip index>:<addr> = <value>" > > I find it a bit confusing since 'p' is used to indicate pib class (-p). > It's also the same as processor (or chip). It was actually meant to refer to processor rather than pib. > For example, how do you do getcfam on second chip? (Does that even make > sense? Read more pdfs Amitay!) Yes and yes (and feel free to ask the interactive pdf) :-) Every processor/module is physically identical and therefore each has a CFAM (as do the memory buffers but lets ignore that for now ;). These CFAM are all mostly accesable (there are a few exceptions, notably when using i2c on P8 the primary CFAM can't be accessed) Currently you would do `pdbg -p1 getcfam 0xc09` to get a cfam register on the second processor. > If we need fsi target, then may be "-P fsi1" will work provided we have > the second fsi target with correct index. If there is no fsi target > with index 1, then we might have to actually know the topology to > select the right fsi target (e.g. "-P fsi/fsi"). In that how do we > generate the output "p1: <addr> = <value>"? > > It's more intuitive if we always use pib as a package/processor/chip. > It's easier to either use "-p 1" or "-P pib1" for second processor/chip > and then we can internally pick the "right" target to actually perform > the fsi_read() operation. For `pdbg -p1 getcfam` sure, all I'm really thinking is that we should keep the existing behaviour the same. I am not so convinced `pdbg -P pib1 getcfam` should work. It would become difficult for example if we add all the cfam targets (such as SBE or memory buffer fsi/cfams). We should really just bail with an error in this case. I think the path needs to match what you're targetting - the existing shortcuts can be used for "magic" to select what the user really wants. Ie. users can always stick to things like `-p1` to specify "getcfam blah on processor 1" if they want convenient shortcuts (or we can add other shortcuts if you think -p is not sufficient?). - Alistair > Amitay.
diff --git a/src/cfam.c b/src/cfam.c index 6dab388..69b21a4 100644 --- a/src/cfam.c +++ b/src/cfam.c @@ -18,42 +18,64 @@ #include <stdlib.h> #include <string.h> #include <inttypes.h> +#include <assert.h> #include "main.h" #include "optcmd.h" +#include "path.h" -static int _getcfam(struct pdbg_target *target, uint32_t index, uint64_t *addr, uint64_t *unused) +static int getcfam(uint32_t addr) { + struct pdbg_target *target; + char *path; uint32_t value; + int count = 0; - if (fsi_read(target, *addr, &value)) - return 0; + for_each_path_target_class("fsi", target) { + if (pdbg_target_status(target) != PDBG_TARGET_ENABLED) + continue; - printf("p%d:0x%x = 0x%08x\n", index, (uint32_t) *addr, value); + path = pdbg_target_path(target); + assert(path); - return 1; -} + if (fsi_read(target, addr, &value)) { + printf("%s: failed\n", path); + free(path); + continue; -static int getcfam(uint32_t addr) -{ - uint64_t addr64 = addr; + } + + printf("%s: 0x%x = 0x%08x\n", path, addr, value); + free(path); + count++; + } - return for_each_target("fsi", _getcfam, &addr64, NULL); + return count; } OPTCMD_DEFINE_CMD_WITH_ARGS(getcfam, getcfam, (ADDRESS32)); -static int _putcfam(struct pdbg_target *target, uint32_t index, uint64_t *addr, uint64_t *data) +static int putcfam(uint32_t addr, uint32_t data) { - if (fsi_write(target, *addr, *data)) - return 0; + struct pdbg_target *target; + char *path; + int count = 0; - return 1; -} + for_each_path_target_class("fsi", target) { + if (pdbg_target_status(target) != PDBG_TARGET_ENABLED) + continue; -static int putcfam(uint32_t addr, uint32_t data) -{ - uint64_t addr64 = addr, data64 = data; + path = pdbg_target_path(target); + assert(path); + + if (fsi_write(target, addr, data)) { + printf("%s: failed\n", path); + free(path); + continue; + } + + count++; + } - return for_each_target("fsi", _putcfam, &addr64, &data64); + return count; } OPTCMD_DEFINE_CMD_WITH_ARGS(putcfam, putcfam, (ADDRESS32, DATA32)); diff --git a/tests/test_hw_bmc.sh b/tests/test_hw_bmc.sh index c35597b..9bc1deb 100755 --- a/tests/test_hw_bmc.sh +++ b/tests/test_hw_bmc.sh @@ -84,11 +84,11 @@ result_filter () } test_result 0 <<EOF -p0:0xc09 = HEX8 +/kernelfsi@0: 0xc09 = HEX8 EOF do_skip -test_run $PDBG -p0 getcfam 0xc09 +test_run $PDBG -P fsi getcfam 0xc09 test_result 0 <<EOF p0:0xf000f = HEX16
Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> --- src/cfam.c | 60 ++++++++++++++++++++++++++++++-------------- tests/test_hw_bmc.sh | 4 +-- 2 files changed, 43 insertions(+), 21 deletions(-)