| Message ID | 20190806013723.4047-4-alistair@popple.id.au |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Split backends from system description | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (3a46123baa267a1bc6e03c50449a42e8d321bf86) |
| snowpatch_ozlabs/build-multiarch | fail | Test build-multiarch on branch master |
On Tue, 2019-08-06 at 11:37 +1000, Alistair Popple wrote: > The thread control functions should implement the standard thread > control interface rather than an sbefifo specific version. Rework the > existing functions to match. > > Signed-off-by: Alistair Popple <alistair@popple.id.au> > --- > libpdbg/chip.c | 6 +--- > libpdbg/hwunit.h | 5 +--- > libpdbg/sbefifo.c | 76 +++++++++++++++++++++++++++++++++++++------ > ---- > 3 files changed, 62 insertions(+), 25 deletions(-) > > diff --git a/libpdbg/chip.c b/libpdbg/chip.c > index 67e3afa..259b106 100644 > --- a/libpdbg/chip.c > +++ b/libpdbg/chip.c > @@ -166,11 +166,7 @@ int thread_sreset_all(void) > if (!sbefifo) > break; > > - /* > - * core_id = 0xff (all SMT4 cores) > - * thread_id = 0xf (all 4 threads in the SMT4 core) > - */ > - rc |= sbefifo->thread_sreset(sbefifo, 0xff, 0xf); > + rc |= sbefifo->sreset_all(sbefifo); > count++; > } > > diff --git a/libpdbg/hwunit.h b/libpdbg/hwunit.h > index a359af0..b8a2dca 100644 > --- a/libpdbg/hwunit.h > +++ b/libpdbg/hwunit.h > @@ -68,11 +68,8 @@ struct mem { > struct sbefifo { > struct pdbg_target target; > int (*istep)(struct sbefifo *, uint32_t major, uint32_t minor); > - int (*thread_start)(struct sbefifo *, uint32_t core_id, > uint32_t thread_id); > - int (*thread_stop)(struct sbefifo *, uint32_t core_id, uint32_t > thread_id); > - int (*thread_step)(struct sbefifo *, uint32_t core_id, uint32_t > thread_id); > - int (*thread_sreset)(struct sbefifo *, uint32_t core_id, > uint32_t thread_id); > int (*chipop)(struct sbefifo *, uint32_t *, uint32_t, uint8_t > **, uint32_t *, uint32_t *); > + int (*sreset_all)(struct sbefifo *); > uint32_t (*ffdc_get)(struct sbefifo *, const uint8_t **, > uint32_t *); > int fd; > uint32_t status; > diff --git a/libpdbg/sbefifo.c b/libpdbg/sbefifo.c > index b7f52a1..ade2d62 100644 > --- a/libpdbg/sbefifo.c > +++ b/libpdbg/sbefifo.c > @@ -418,28 +418,60 @@ static int sbefifo_op_control(struct sbefifo > *sbefifo, > return 0; > } > > -static int sbefifo_op_thread_start(struct sbefifo *sbefifo, > - uint32_t core_id, uint32_t > thread_id) > +static int sbefifo_op_thread(struct thread *thread, uint32_t op) > { > - return sbefifo_op_control(sbefifo, core_id, thread_id, > SBEFIFO_INSN_OP_START); > + return sbefifo_op_control(target_to_sbefifo(thread- > >target.parent), > + thread->id >> 8, thread->id & 0xff, > op); > } > > -static int sbefifo_op_thread_stop(struct sbefifo *sbefifo, > - uint32_t core_id, uint32_t thread_id) > +static int sbefifo_op_thread_start(struct thread *thread) > { > - return sbefifo_op_control(sbefifo, core_id, thread_id, > SBEFIFO_INSN_OP_STOP); > + return sbefifo_op_thread(thread, SBEFIFO_INSN_OP_START); > } > > -static int sbefifo_op_thread_step(struct sbefifo *sbefifo, > - uint32_t core_id, uint32_t thread_id) > +static int sbefifo_op_thread_stop(struct thread *thread) > { > - return sbefifo_op_control(sbefifo, core_id, thread_id, > SBEFIFO_INSN_OP_STEP); > + return sbefifo_op_thread(thread, SBEFIFO_INSN_OP_STOP); > } > > -static int sbefifo_op_thread_sreset(struct sbefifo *sbefifo, > - uint32_t core_id, uint32_t > thread_id) > +static int sbefifo_op_thread_step(struct thread *thread, int count) > { > - return sbefifo_op_control(sbefifo, core_id, thread_id, > SBEFIFO_INSN_OP_SRESET); > + int i, rc; > + > + for (i = 0; i < count; i++) > + if ((rc = sbefifo_op_thread(thread, > SBEFIFO_INSN_OP_STEP))) > + return rc; > + > + return 0; > +} > + > +static int sbefifo_op_thread_sreset(struct thread *thread) > +{ > + return sbefifo_op_thread(thread, SBEFIFO_INSN_OP_SRESET); > +} > + > +static int sbefifo_op_sreset_all(struct sbefifo *sbefifo) > +{ > + /* > + * core_id = 0xff (all SMT4 cores) > + * thread_id = 0xf (all 4 threads in the SMT4 core) > + */ > + return sbefifo_op_control(sbefifo, 0xff, 0xf, > SBEFIFO_INSN_OP_SRESET); > +} > + > +static int sbefifo_thread_probe(struct pdbg_target *target) > +{ > + struct thread *thread = target_to_thread(target); > + uint32_t core_id, thread_id; > + > + if (pdbg_target_u32_property(target, "core-id", &core_id)) > + return -1; > + > + if (pdbg_target_u32_property(target, "thread-id", &thread_id)) > + return -1; > + > + thread->id = (core_id << 8) | (thread_id & 0xff); > + return 0; > } Do we really need to add two more properties for each thread? core-id and thread-id are the same as core-index (provided core-index is the right chiplet-index I think! We need to test this.) and thread-index. We can calculate them run-time; is there a reason to do it at probe time? > > static int sbefifo_op_chipop(struct sbefifo *sbefifo, > @@ -502,6 +534,20 @@ struct mem sbefifo_mem = { > }; > DECLARE_HW_UNIT(sbefifo_mem); > > +struct thread sbefifo_thread = { > + .target = { > + .name = "SBE FIFO Chip-op based thread access", > + .compatible = "ibm,sbefifo-thread", > + .class = "thread", > + .probe = sbefifo_thread_probe, > + }, > + .start = sbefifo_op_thread_start, > + .stop = sbefifo_op_thread_stop, > + .step = sbefifo_op_thread_step, > + .sreset = sbefifo_op_thread_sreset, > +}; > +DECLARE_HW_UNIT(sbefifo_thread); > + > struct sbefifo kernel_sbefifo = { > .target = { > .name = "Kernel based FSI SBE FIFO", > @@ -510,10 +556,7 @@ struct sbefifo kernel_sbefifo = { > .probe = sbefifo_probe, > }, > .istep = sbefifo_op_istep, > - .thread_start = sbefifo_op_thread_start, > - .thread_stop = sbefifo_op_thread_stop, > - .thread_step = sbefifo_op_thread_step, > - .thread_sreset = sbefifo_op_thread_sreset, > + .sreset_all = sbefifo_op_sreset_all, > .chipop = sbefifo_op_chipop, > .ffdc_get = sbefifo_ffdc_get, > .fd = -1, > @@ -525,4 +568,5 @@ static void register_sbefifo(void) > { > pdbg_hwunit_register(&kernel_sbefifo_hw_unit); > pdbg_hwunit_register(&sbefifo_mem_hw_unit); > + pdbg_hwunit_register(&sbefifo_thread_hw_unit); > } > -- > 2.20.1 > Amitay.
On Tuesday, 20 August 2019 1:50:33 PM AEST Amitay Isaacs wrote: > On Tue, 2019-08-06 at 11:37 +1000, Alistair Popple wrote: > > The thread control functions should implement the standard thread > > control interface rather than an sbefifo specific version. Rework the > > existing functions to match. > > > > Signed-off-by: Alistair Popple <alistair@popple.id.au> > > --- > > libpdbg/chip.c | 6 +--- > > libpdbg/hwunit.h | 5 +--- > > libpdbg/sbefifo.c | 76 +++++++++++++++++++++++++++++++++++++------ > > ---- > > 3 files changed, 62 insertions(+), 25 deletions(-) > > > > diff --git a/libpdbg/chip.c b/libpdbg/chip.c > > index 67e3afa..259b106 100644 > > --- a/libpdbg/chip.c > > +++ b/libpdbg/chip.c > > @@ -166,11 +166,7 @@ int thread_sreset_all(void) > > if (!sbefifo) > > break; > > > > - /* > > - * core_id = 0xff (all SMT4 cores) > > - * thread_id = 0xf (all 4 threads in the SMT4 core) > > - */ > > - rc |= sbefifo->thread_sreset(sbefifo, 0xff, 0xf); > > + rc |= sbefifo->sreset_all(sbefifo); > > count++; > > } > > > > diff --git a/libpdbg/hwunit.h b/libpdbg/hwunit.h > > index a359af0..b8a2dca 100644 > > --- a/libpdbg/hwunit.h > > +++ b/libpdbg/hwunit.h > > @@ -68,11 +68,8 @@ struct mem { > > struct sbefifo { > > struct pdbg_target target; > > int (*istep)(struct sbefifo *, uint32_t major, uint32_t minor); > > - int (*thread_start)(struct sbefifo *, uint32_t core_id, > > uint32_t thread_id); > > - int (*thread_stop)(struct sbefifo *, uint32_t core_id, uint32_t > > thread_id); > > - int (*thread_step)(struct sbefifo *, uint32_t core_id, uint32_t > > thread_id); > > - int (*thread_sreset)(struct sbefifo *, uint32_t core_id, > > uint32_t thread_id); > > int (*chipop)(struct sbefifo *, uint32_t *, uint32_t, uint8_t > > **, uint32_t *, uint32_t *); > > + int (*sreset_all)(struct sbefifo *); > > uint32_t (*ffdc_get)(struct sbefifo *, const uint8_t **, > > uint32_t *); > > int fd; > > uint32_t status; > > diff --git a/libpdbg/sbefifo.c b/libpdbg/sbefifo.c > > index b7f52a1..ade2d62 100644 > > --- a/libpdbg/sbefifo.c > > +++ b/libpdbg/sbefifo.c > > @@ -418,28 +418,60 @@ static int sbefifo_op_control(struct sbefifo > > *sbefifo, > > return 0; > > } > > > > -static int sbefifo_op_thread_start(struct sbefifo *sbefifo, > > - uint32_t core_id, uint32_t > > thread_id) > > +static int sbefifo_op_thread(struct thread *thread, uint32_t op) > > { > > - return sbefifo_op_control(sbefifo, core_id, thread_id, > > SBEFIFO_INSN_OP_START); > > + return sbefifo_op_control(target_to_sbefifo(thread- > > >target.parent), > > + thread->id >> 8, thread->id & 0xff, > > op); > > } > > > > -static int sbefifo_op_thread_stop(struct sbefifo *sbefifo, > > - uint32_t core_id, uint32_t thread_id) > > +static int sbefifo_op_thread_start(struct thread *thread) > > { > > - return sbefifo_op_control(sbefifo, core_id, thread_id, > > SBEFIFO_INSN_OP_STOP); > > + return sbefifo_op_thread(thread, SBEFIFO_INSN_OP_START); > > } > > > > -static int sbefifo_op_thread_step(struct sbefifo *sbefifo, > > - uint32_t core_id, uint32_t thread_id) > > +static int sbefifo_op_thread_stop(struct thread *thread) > > { > > - return sbefifo_op_control(sbefifo, core_id, thread_id, > > SBEFIFO_INSN_OP_STEP); > > + return sbefifo_op_thread(thread, SBEFIFO_INSN_OP_STOP); > > } > > > > -static int sbefifo_op_thread_sreset(struct sbefifo *sbefifo, > > - uint32_t core_id, uint32_t > > thread_id) > > +static int sbefifo_op_thread_step(struct thread *thread, int count) > > { > > - return sbefifo_op_control(sbefifo, core_id, thread_id, > > SBEFIFO_INSN_OP_SRESET); > > + int i, rc; > > + > > + for (i = 0; i < count; i++) > > + if ((rc = sbefifo_op_thread(thread, > > SBEFIFO_INSN_OP_STEP))) > > + return rc; > > + > > + return 0; > > +} > > + > > +static int sbefifo_op_thread_sreset(struct thread *thread) > > +{ > > + return sbefifo_op_thread(thread, SBEFIFO_INSN_OP_SRESET); > > +} > > + > > +static int sbefifo_op_sreset_all(struct sbefifo *sbefifo) > > +{ > > + /* > > + * core_id = 0xff (all SMT4 cores) > > + * thread_id = 0xf (all 4 threads in the SMT4 core) > > + */ > > + return sbefifo_op_control(sbefifo, 0xff, 0xf, > > SBEFIFO_INSN_OP_SRESET); > > +} > > + > > +static int sbefifo_thread_probe(struct pdbg_target *target) > > +{ > > + struct thread *thread = target_to_thread(target); > > + uint32_t core_id, thread_id; > > + > > + if (pdbg_target_u32_property(target, "core-id", &core_id)) > > + return -1; > > + > > + if (pdbg_target_u32_property(target, "thread-id", &thread_id)) > > + return -1; > > + > > + thread->id = (core_id << 8) | (thread_id & 0xff); > > + return 0; > > } > > Do we really need to add two more properties for each thread? core-id > and thread-id are the same as core-index (provided core-index is the > right chiplet-index I think! We need to test this.) and thread-index. > We can calculate them run-time; is there a reason to do it at probe > time? It depends on what values the SBE chip-op expects. If you know how we can calculate these at runtime based on the chiplet index and local core index (0-4) then that would certainly be the better option. I could see any examples of this in the existing code though, do you know what values the SBE expects here (other than 0xfff..)? - Alistair > > > > static int sbefifo_op_chipop(struct sbefifo *sbefifo, > > @@ -502,6 +534,20 @@ struct mem sbefifo_mem = { > > }; > > DECLARE_HW_UNIT(sbefifo_mem); > > > > +struct thread sbefifo_thread = { > > + .target = { > > + .name = "SBE FIFO Chip-op based thread access", > > + .compatible = "ibm,sbefifo-thread", > > + .class = "thread", > > + .probe = sbefifo_thread_probe, > > + }, > > + .start = sbefifo_op_thread_start, > > + .stop = sbefifo_op_thread_stop, > > + .step = sbefifo_op_thread_step, > > + .sreset = sbefifo_op_thread_sreset, > > +}; > > +DECLARE_HW_UNIT(sbefifo_thread); > > + > > struct sbefifo kernel_sbefifo = { > > .target = { > > .name = "Kernel based FSI SBE FIFO", > > @@ -510,10 +556,7 @@ struct sbefifo kernel_sbefifo = { > > .probe = sbefifo_probe, > > }, > > .istep = sbefifo_op_istep, > > - .thread_start = sbefifo_op_thread_start, > > - .thread_stop = sbefifo_op_thread_stop, > > - .thread_step = sbefifo_op_thread_step, > > - .thread_sreset = sbefifo_op_thread_sreset, > > + .sreset_all = sbefifo_op_sreset_all, > > .chipop = sbefifo_op_chipop, > > .ffdc_get = sbefifo_ffdc_get, > > .fd = -1, > > @@ -525,4 +568,5 @@ static void register_sbefifo(void) > > { > > pdbg_hwunit_register(&kernel_sbefifo_hw_unit); > > pdbg_hwunit_register(&sbefifo_mem_hw_unit); > > + pdbg_hwunit_register(&sbefifo_thread_hw_unit); > > } > > Amitay. >
On Tue, 2019-08-20 at 14:55 +1000, Alistair Popple wrote: > On Tuesday, 20 August 2019 1:50:33 PM AEST Amitay Isaacs wrote: > > On Tue, 2019-08-06 at 11:37 +1000, Alistair Popple wrote: > > > The thread control functions should implement the standard thread > > > control interface rather than an sbefifo specific version. Rework > > > the > > > existing functions to match. > > > > > > Signed-off-by: Alistair Popple <alistair@popple.id.au> > > > --- > > > libpdbg/chip.c | 6 +--- > > > libpdbg/hwunit.h | 5 +--- > > > libpdbg/sbefifo.c | 76 +++++++++++++++++++++++++++++++++++++-- > > > ---- > > > ---- > > > 3 files changed, 62 insertions(+), 25 deletions(-) > > > > > > diff --git a/libpdbg/chip.c b/libpdbg/chip.c > > > index 67e3afa..259b106 100644 > > > --- a/libpdbg/chip.c > > > +++ b/libpdbg/chip.c > > > @@ -166,11 +166,7 @@ int thread_sreset_all(void) > > > if (!sbefifo) > > > break; > > > > > > - /* > > > - * core_id = 0xff (all SMT4 cores) > > > - * thread_id = 0xf (all 4 threads in the SMT4 core) > > > - */ > > > - rc |= sbefifo->thread_sreset(sbefifo, 0xff, 0xf); > > > + rc |= sbefifo->sreset_all(sbefifo); > > > count++; > > > } > > > > > > diff --git a/libpdbg/hwunit.h b/libpdbg/hwunit.h > > > index a359af0..b8a2dca 100644 > > > --- a/libpdbg/hwunit.h > > > +++ b/libpdbg/hwunit.h > > > @@ -68,11 +68,8 @@ struct mem { > > > struct sbefifo { > > > struct pdbg_target target; > > > int (*istep)(struct sbefifo *, uint32_t major, uint32_t minor); > > > - int (*thread_start)(struct sbefifo *, uint32_t core_id, > > > uint32_t thread_id); > > > - int (*thread_stop)(struct sbefifo *, uint32_t core_id, uint32_t > > > thread_id); > > > - int (*thread_step)(struct sbefifo *, uint32_t core_id, uint32_t > > > thread_id); > > > - int (*thread_sreset)(struct sbefifo *, uint32_t core_id, > > > uint32_t thread_id); > > > int (*chipop)(struct sbefifo *, uint32_t *, uint32_t, uint8_t > > > **, uint32_t *, uint32_t *); > > > + int (*sreset_all)(struct sbefifo *); > > > uint32_t (*ffdc_get)(struct sbefifo *, const uint8_t **, > > > uint32_t *); > > > int fd; > > > uint32_t status; > > > diff --git a/libpdbg/sbefifo.c b/libpdbg/sbefifo.c > > > index b7f52a1..ade2d62 100644 > > > --- a/libpdbg/sbefifo.c > > > +++ b/libpdbg/sbefifo.c > > > @@ -418,28 +418,60 @@ static int sbefifo_op_control(struct > > > sbefifo > > > *sbefifo, > > > return 0; > > > } > > > > > > -static int sbefifo_op_thread_start(struct sbefifo *sbefifo, > > > - uint32_t core_id, uint32_t > > > thread_id) > > > +static int sbefifo_op_thread(struct thread *thread, uint32_t op) > > > { > > > - return sbefifo_op_control(sbefifo, core_id, thread_id, > > > SBEFIFO_INSN_OP_START); > > > + return sbefifo_op_control(target_to_sbefifo(thread- > > > > target.parent), > > > + thread->id >> 8, thread->id & 0xff, > > > op); > > > } > > > > > > -static int sbefifo_op_thread_stop(struct sbefifo *sbefifo, > > > - uint32_t core_id, uint32_t thread_id) > > > +static int sbefifo_op_thread_start(struct thread *thread) > > > { > > > - return sbefifo_op_control(sbefifo, core_id, thread_id, > > > SBEFIFO_INSN_OP_STOP); > > > + return sbefifo_op_thread(thread, SBEFIFO_INSN_OP_START); > > > } > > > > > > -static int sbefifo_op_thread_step(struct sbefifo *sbefifo, > > > - uint32_t core_id, uint32_t thread_id) > > > +static int sbefifo_op_thread_stop(struct thread *thread) > > > { > > > - return sbefifo_op_control(sbefifo, core_id, thread_id, > > > SBEFIFO_INSN_OP_STEP); > > > + return sbefifo_op_thread(thread, SBEFIFO_INSN_OP_STOP); > > > } > > > > > > -static int sbefifo_op_thread_sreset(struct sbefifo *sbefifo, > > > - uint32_t core_id, uint32_t > > > thread_id) > > > +static int sbefifo_op_thread_step(struct thread *thread, int > > > count) > > > { > > > - return sbefifo_op_control(sbefifo, core_id, thread_id, > > > SBEFIFO_INSN_OP_SRESET); > > > + int i, rc; > > > + > > > + for (i = 0; i < count; i++) > > > + if ((rc = sbefifo_op_thread(thread, > > > SBEFIFO_INSN_OP_STEP))) > > > + return rc; > > > + > > > + return 0; > > > +} > > > + > > > +static int sbefifo_op_thread_sreset(struct thread *thread) > > > +{ > > > + return sbefifo_op_thread(thread, SBEFIFO_INSN_OP_SRESET); > > > +} > > > + > > > +static int sbefifo_op_sreset_all(struct sbefifo *sbefifo) > > > +{ > > > + /* > > > + * core_id = 0xff (all SMT4 cores) > > > + * thread_id = 0xf (all 4 threads in the SMT4 core) > > > + */ > > > + return sbefifo_op_control(sbefifo, 0xff, 0xf, > > > SBEFIFO_INSN_OP_SRESET); > > > +} > > > + > > > +static int sbefifo_thread_probe(struct pdbg_target *target) > > > +{ > > > + struct thread *thread = target_to_thread(target); > > > + uint32_t core_id, thread_id; > > > + > > > + if (pdbg_target_u32_property(target, "core-id", &core_id)) > > > + return -1; > > > + > > > + if (pdbg_target_u32_property(target, "thread-id", &thread_id)) > > > + return -1; > > > + > > > + thread->id = (core_id << 8) | (thread_id & 0xff); > > > + return 0; > > > } > > > > Do we really need to add two more properties for each > > thread? core-id > > and thread-id are the same as core-index (provided core-index is > > the > > right chiplet-index I think! We need to test this.) and thread- > > index. > > We can calculate them run-time; is there a reason to do it at probe > > time? > > It depends on what values the SBE chip-op expects. If you know how we > can > calculate these at runtime based on the chiplet index and local core > index > (0-4) then that would certainly be the better option. I could see any > examples > of this in the existing code though, do you know what values the SBE > expects > here (other than 0xfff..)? > > - Alistair As per the document the core-id is the pervasive chiplet id for the core (0x20 - 0x37). So that's not quite the core index, but chiplet index. Thread id is local to each core (0x0 - 0x3). > > > > > > > static int sbefifo_op_chipop(struct sbefifo *sbefifo, > > > @@ -502,6 +534,20 @@ struct mem sbefifo_mem = { > > > }; > > > DECLARE_HW_UNIT(sbefifo_mem); > > > > > > +struct thread sbefifo_thread = { > > > + .target = { > > > + .name = "SBE FIFO Chip-op based thread access", > > > + .compatible = "ibm,sbefifo-thread", > > > + .class = "thread", > > > + .probe = sbefifo_thread_probe, > > > + }, > > > + .start = sbefifo_op_thread_start, > > > + .stop = sbefifo_op_thread_stop, > > > + .step = sbefifo_op_thread_step, > > > + .sreset = sbefifo_op_thread_sreset, > > > +}; > > > +DECLARE_HW_UNIT(sbefifo_thread); > > > + > > > struct sbefifo kernel_sbefifo = { > > > .target = { > > > .name = "Kernel based FSI SBE FIFO", > > > @@ -510,10 +556,7 @@ struct sbefifo kernel_sbefifo = { > > > .probe = sbefifo_probe, > > > }, > > > .istep = sbefifo_op_istep, > > > - .thread_start = sbefifo_op_thread_start, > > > - .thread_stop = sbefifo_op_thread_stop, > > > - .thread_step = sbefifo_op_thread_step, > > > - .thread_sreset = sbefifo_op_thread_sreset, > > > + .sreset_all = sbefifo_op_sreset_all, > > > .chipop = sbefifo_op_chipop, > > > .ffdc_get = sbefifo_ffdc_get, > > > .fd = -1, > > > @@ -525,4 +568,5 @@ static void register_sbefifo(void) > > > { > > > pdbg_hwunit_register(&kernel_sbefifo_hw_unit); > > > pdbg_hwunit_register(&sbefifo_mem_hw_unit); > > > + pdbg_hwunit_register(&sbefifo_thread_hw_unit); > > > } > > > > Amitay. > > > > > Amitay.
On Tuesday, 20 August 2019 3:25:23 PM AEST Amitay Isaacs wrote: > On Tue, 2019-08-20 at 14:55 +1000, Alistair Popple wrote: > > On Tuesday, 20 August 2019 1:50:33 PM AEST Amitay Isaacs wrote: > > > On Tue, 2019-08-06 at 11:37 +1000, Alistair Popple wrote: > > > > The thread control functions should implement the standard thread > > > > control interface rather than an sbefifo specific version. Rework > > > > the > > > > existing functions to match. > > > > > > > > Signed-off-by: Alistair Popple <alistair@popple.id.au> > > > > --- > > > > libpdbg/chip.c | 6 +--- > > > > libpdbg/hwunit.h | 5 +--- > > > > libpdbg/sbefifo.c | 76 +++++++++++++++++++++++++++++++++++++-- > > > > ---- > > > > ---- > > > > 3 files changed, 62 insertions(+), 25 deletions(-) > > > > > > > > diff --git a/libpdbg/chip.c b/libpdbg/chip.c > > > > index 67e3afa..259b106 100644 > > > > --- a/libpdbg/chip.c > > > > +++ b/libpdbg/chip.c > > > > @@ -166,11 +166,7 @@ int thread_sreset_all(void) > > > > if (!sbefifo) > > > > break; > > > > > > > > - /* > > > > - * core_id = 0xff (all SMT4 cores) > > > > - * thread_id = 0xf (all 4 threads in the SMT4 core) > > > > - */ > > > > - rc |= sbefifo->thread_sreset(sbefifo, 0xff, 0xf); > > > > + rc |= sbefifo->sreset_all(sbefifo); > > > > count++; > > > > } > > > > > > > > diff --git a/libpdbg/hwunit.h b/libpdbg/hwunit.h > > > > index a359af0..b8a2dca 100644 > > > > --- a/libpdbg/hwunit.h > > > > +++ b/libpdbg/hwunit.h > > > > @@ -68,11 +68,8 @@ struct mem { > > > > struct sbefifo { > > > > struct pdbg_target target; > > > > int (*istep)(struct sbefifo *, uint32_t major, uint32_t minor); > > > > - int (*thread_start)(struct sbefifo *, uint32_t core_id, > > > > uint32_t thread_id); > > > > - int (*thread_stop)(struct sbefifo *, uint32_t core_id, uint32_t > > > > thread_id); > > > > - int (*thread_step)(struct sbefifo *, uint32_t core_id, uint32_t > > > > thread_id); > > > > - int (*thread_sreset)(struct sbefifo *, uint32_t core_id, > > > > uint32_t thread_id); > > > > int (*chipop)(struct sbefifo *, uint32_t *, uint32_t, uint8_t > > > > **, uint32_t *, uint32_t *); > > > > + int (*sreset_all)(struct sbefifo *); > > > > uint32_t (*ffdc_get)(struct sbefifo *, const uint8_t **, > > > > uint32_t *); > > > > int fd; > > > > uint32_t status; > > > > diff --git a/libpdbg/sbefifo.c b/libpdbg/sbefifo.c > > > > index b7f52a1..ade2d62 100644 > > > > --- a/libpdbg/sbefifo.c > > > > +++ b/libpdbg/sbefifo.c > > > > @@ -418,28 +418,60 @@ static int sbefifo_op_control(struct > > > > sbefifo > > > > *sbefifo, > > > > return 0; > > > > } > > > > > > > > -static int sbefifo_op_thread_start(struct sbefifo *sbefifo, > > > > - uint32_t core_id, uint32_t > > > > thread_id) > > > > +static int sbefifo_op_thread(struct thread *thread, uint32_t op) > > > > { > > > > - return sbefifo_op_control(sbefifo, core_id, thread_id, > > > > SBEFIFO_INSN_OP_START); > > > > + return sbefifo_op_control(target_to_sbefifo(thread- > > > > > target.parent), > > > > + thread->id >> 8, thread->id & 0xff, > > > > op); > > > > } > > > > > > > > -static int sbefifo_op_thread_stop(struct sbefifo *sbefifo, > > > > - uint32_t core_id, uint32_t thread_id) > > > > +static int sbefifo_op_thread_start(struct thread *thread) > > > > { > > > > - return sbefifo_op_control(sbefifo, core_id, thread_id, > > > > SBEFIFO_INSN_OP_STOP); > > > > + return sbefifo_op_thread(thread, SBEFIFO_INSN_OP_START); > > > > } > > > > > > > > -static int sbefifo_op_thread_step(struct sbefifo *sbefifo, > > > > - uint32_t core_id, uint32_t thread_id) > > > > +static int sbefifo_op_thread_stop(struct thread *thread) > > > > { > > > > - return sbefifo_op_control(sbefifo, core_id, thread_id, > > > > SBEFIFO_INSN_OP_STEP); > > > > + return sbefifo_op_thread(thread, SBEFIFO_INSN_OP_STOP); > > > > } > > > > > > > > -static int sbefifo_op_thread_sreset(struct sbefifo *sbefifo, > > > > - uint32_t core_id, uint32_t > > > > thread_id) > > > > +static int sbefifo_op_thread_step(struct thread *thread, int > > > > count) > > > > { > > > > - return sbefifo_op_control(sbefifo, core_id, thread_id, > > > > SBEFIFO_INSN_OP_SRESET); > > > > + int i, rc; > > > > + > > > > + for (i = 0; i < count; i++) > > > > + if ((rc = sbefifo_op_thread(thread, > > > > SBEFIFO_INSN_OP_STEP))) > > > > + return rc; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int sbefifo_op_thread_sreset(struct thread *thread) > > > > +{ > > > > + return sbefifo_op_thread(thread, SBEFIFO_INSN_OP_SRESET); > > > > +} > > > > + > > > > +static int sbefifo_op_sreset_all(struct sbefifo *sbefifo) > > > > +{ > > > > + /* > > > > + * core_id = 0xff (all SMT4 cores) > > > > + * thread_id = 0xf (all 4 threads in the SMT4 core) > > > > + */ > > > > + return sbefifo_op_control(sbefifo, 0xff, 0xf, > > > > SBEFIFO_INSN_OP_SRESET); > > > > +} > > > > + > > > > +static int sbefifo_thread_probe(struct pdbg_target *target) > > > > +{ > > > > + struct thread *thread = target_to_thread(target); > > > > + uint32_t core_id, thread_id; > > > > + > > > > + if (pdbg_target_u32_property(target, "core-id", &core_id)) > > > > + return -1; > > > > + > > > > + if (pdbg_target_u32_property(target, "thread-id", &thread_id)) > > > > + return -1; > > > > + > > > > + thread->id = (core_id << 8) | (thread_id & 0xff); > > > > + return 0; > > > > } > > > > > > Do we really need to add two more properties for each > > > thread? core-id > > > and thread-id are the same as core-index (provided core-index is > > > the > > > right chiplet-index I think! We need to test this.) and thread- > > > index. > > > We can calculate them run-time; is there a reason to do it at probe > > > time? > > > > It depends on what values the SBE chip-op expects. If you know how we > > can > > calculate these at runtime based on the chiplet index and local core > > index > > (0-4) then that would certainly be the better option. I could see any > > examples > > of this in the existing code though, do you know what values the SBE > > expects > > here (other than 0xfff..)? > > > > - Alistair > > As per the document the core-id is the pervasive chiplet id for the > core (0x20 - 0x37). So that's not quite the core index, but chiplet > index. Thread id is local to each core (0x0 - 0x3). Ok, thanks. It looks like at the moment we have incorrect chiplet indices at the moment so I will fix that up and get rid of the extra fields. - Alistair > > > > > > > > > > static int sbefifo_op_chipop(struct sbefifo *sbefifo, > > > > @@ -502,6 +534,20 @@ struct mem sbefifo_mem = { > > > > }; > > > > DECLARE_HW_UNIT(sbefifo_mem); > > > > > > > > +struct thread sbefifo_thread = { > > > > + .target = { > > > > + .name = "SBE FIFO Chip-op based thread access", > > > > + .compatible = "ibm,sbefifo-thread", > > > > + .class = "thread", > > > > + .probe = sbefifo_thread_probe, > > > > + }, > > > > + .start = sbefifo_op_thread_start, > > > > + .stop = sbefifo_op_thread_stop, > > > > + .step = sbefifo_op_thread_step, > > > > + .sreset = sbefifo_op_thread_sreset, > > > > +}; > > > > +DECLARE_HW_UNIT(sbefifo_thread); > > > > + > > > > struct sbefifo kernel_sbefifo = { > > > > .target = { > > > > .name = "Kernel based FSI SBE FIFO", > > > > @@ -510,10 +556,7 @@ struct sbefifo kernel_sbefifo = { > > > > .probe = sbefifo_probe, > > > > }, > > > > .istep = sbefifo_op_istep, > > > > - .thread_start = sbefifo_op_thread_start, > > > > - .thread_stop = sbefifo_op_thread_stop, > > > > - .thread_step = sbefifo_op_thread_step, > > > > - .thread_sreset = sbefifo_op_thread_sreset, > > > > + .sreset_all = sbefifo_op_sreset_all, > > > > .chipop = sbefifo_op_chipop, > > > > .ffdc_get = sbefifo_ffdc_get, > > > > .fd = -1, > > > > @@ -525,4 +568,5 @@ static void register_sbefifo(void) > > > > { > > > > pdbg_hwunit_register(&kernel_sbefifo_hw_unit); > > > > pdbg_hwunit_register(&sbefifo_mem_hw_unit); > > > > + pdbg_hwunit_register(&sbefifo_thread_hw_unit); > > > > } > > > > > > Amitay. > > > > > > > > > > > Amitay. >
diff --git a/libpdbg/chip.c b/libpdbg/chip.c index 67e3afa..259b106 100644 --- a/libpdbg/chip.c +++ b/libpdbg/chip.c @@ -166,11 +166,7 @@ int thread_sreset_all(void) if (!sbefifo) break; - /* - * core_id = 0xff (all SMT4 cores) - * thread_id = 0xf (all 4 threads in the SMT4 core) - */ - rc |= sbefifo->thread_sreset(sbefifo, 0xff, 0xf); + rc |= sbefifo->sreset_all(sbefifo); count++; } diff --git a/libpdbg/hwunit.h b/libpdbg/hwunit.h index a359af0..b8a2dca 100644 --- a/libpdbg/hwunit.h +++ b/libpdbg/hwunit.h @@ -68,11 +68,8 @@ struct mem { struct sbefifo { struct pdbg_target target; int (*istep)(struct sbefifo *, uint32_t major, uint32_t minor); - int (*thread_start)(struct sbefifo *, uint32_t core_id, uint32_t thread_id); - int (*thread_stop)(struct sbefifo *, uint32_t core_id, uint32_t thread_id); - int (*thread_step)(struct sbefifo *, uint32_t core_id, uint32_t thread_id); - int (*thread_sreset)(struct sbefifo *, uint32_t core_id, uint32_t thread_id); int (*chipop)(struct sbefifo *, uint32_t *, uint32_t, uint8_t **, uint32_t *, uint32_t *); + int (*sreset_all)(struct sbefifo *); uint32_t (*ffdc_get)(struct sbefifo *, const uint8_t **, uint32_t *); int fd; uint32_t status; diff --git a/libpdbg/sbefifo.c b/libpdbg/sbefifo.c index b7f52a1..ade2d62 100644 --- a/libpdbg/sbefifo.c +++ b/libpdbg/sbefifo.c @@ -418,28 +418,60 @@ static int sbefifo_op_control(struct sbefifo *sbefifo, return 0; } -static int sbefifo_op_thread_start(struct sbefifo *sbefifo, - uint32_t core_id, uint32_t thread_id) +static int sbefifo_op_thread(struct thread *thread, uint32_t op) { - return sbefifo_op_control(sbefifo, core_id, thread_id, SBEFIFO_INSN_OP_START); + return sbefifo_op_control(target_to_sbefifo(thread->target.parent), + thread->id >> 8, thread->id & 0xff, op); } -static int sbefifo_op_thread_stop(struct sbefifo *sbefifo, - uint32_t core_id, uint32_t thread_id) +static int sbefifo_op_thread_start(struct thread *thread) { - return sbefifo_op_control(sbefifo, core_id, thread_id, SBEFIFO_INSN_OP_STOP); + return sbefifo_op_thread(thread, SBEFIFO_INSN_OP_START); } -static int sbefifo_op_thread_step(struct sbefifo *sbefifo, - uint32_t core_id, uint32_t thread_id) +static int sbefifo_op_thread_stop(struct thread *thread) { - return sbefifo_op_control(sbefifo, core_id, thread_id, SBEFIFO_INSN_OP_STEP); + return sbefifo_op_thread(thread, SBEFIFO_INSN_OP_STOP); } -static int sbefifo_op_thread_sreset(struct sbefifo *sbefifo, - uint32_t core_id, uint32_t thread_id) +static int sbefifo_op_thread_step(struct thread *thread, int count) { - return sbefifo_op_control(sbefifo, core_id, thread_id, SBEFIFO_INSN_OP_SRESET); + int i, rc; + + for (i = 0; i < count; i++) + if ((rc = sbefifo_op_thread(thread, SBEFIFO_INSN_OP_STEP))) + return rc; + + return 0; +} + +static int sbefifo_op_thread_sreset(struct thread *thread) +{ + return sbefifo_op_thread(thread, SBEFIFO_INSN_OP_SRESET); +} + +static int sbefifo_op_sreset_all(struct sbefifo *sbefifo) +{ + /* + * core_id = 0xff (all SMT4 cores) + * thread_id = 0xf (all 4 threads in the SMT4 core) + */ + return sbefifo_op_control(sbefifo, 0xff, 0xf, SBEFIFO_INSN_OP_SRESET); +} + +static int sbefifo_thread_probe(struct pdbg_target *target) +{ + struct thread *thread = target_to_thread(target); + uint32_t core_id, thread_id; + + if (pdbg_target_u32_property(target, "core-id", &core_id)) + return -1; + + if (pdbg_target_u32_property(target, "thread-id", &thread_id)) + return -1; + + thread->id = (core_id << 8) | (thread_id & 0xff); + return 0; } static int sbefifo_op_chipop(struct sbefifo *sbefifo, @@ -502,6 +534,20 @@ struct mem sbefifo_mem = { }; DECLARE_HW_UNIT(sbefifo_mem); +struct thread sbefifo_thread = { + .target = { + .name = "SBE FIFO Chip-op based thread access", + .compatible = "ibm,sbefifo-thread", + .class = "thread", + .probe = sbefifo_thread_probe, + }, + .start = sbefifo_op_thread_start, + .stop = sbefifo_op_thread_stop, + .step = sbefifo_op_thread_step, + .sreset = sbefifo_op_thread_sreset, +}; +DECLARE_HW_UNIT(sbefifo_thread); + struct sbefifo kernel_sbefifo = { .target = { .name = "Kernel based FSI SBE FIFO", @@ -510,10 +556,7 @@ struct sbefifo kernel_sbefifo = { .probe = sbefifo_probe, }, .istep = sbefifo_op_istep, - .thread_start = sbefifo_op_thread_start, - .thread_stop = sbefifo_op_thread_stop, - .thread_step = sbefifo_op_thread_step, - .thread_sreset = sbefifo_op_thread_sreset, + .sreset_all = sbefifo_op_sreset_all, .chipop = sbefifo_op_chipop, .ffdc_get = sbefifo_ffdc_get, .fd = -1, @@ -525,4 +568,5 @@ static void register_sbefifo(void) { pdbg_hwunit_register(&kernel_sbefifo_hw_unit); pdbg_hwunit_register(&sbefifo_mem_hw_unit); + pdbg_hwunit_register(&sbefifo_thread_hw_unit); }
The thread control functions should implement the standard thread control interface rather than an sbefifo specific version. Rework the existing functions to match. Signed-off-by: Alistair Popple <alistair@popple.id.au> --- libpdbg/chip.c | 6 +--- libpdbg/hwunit.h | 5 +--- libpdbg/sbefifo.c | 76 +++++++++++++++++++++++++++++++++++++---------- 3 files changed, 62 insertions(+), 25 deletions(-)