[RFC,03/12] sbefifo: Rework thread functions
diff mbox series

Message ID 20190806013723.4047-4-alistair@popple.id.au
State New
Headers show
Series
  • Split backends from system description
Related show

Checks

Context Check Description
snowpatch_ozlabs/build-multiarch fail Test build-multiarch on branch master
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (3a46123baa267a1bc6e03c50449a42e8d321bf86)

Commit Message

Alistair Popple Aug. 6, 2019, 1:37 a.m. UTC
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(-)

Comments

Amitay Isaacs Aug. 20, 2019, 3:50 a.m. UTC | #1
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.
Alistair Popple Aug. 20, 2019, 4:55 a.m. UTC | #2
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.
>
Amitay Isaacs Aug. 20, 2019, 5:25 a.m. UTC | #3
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.
Alistair Popple Aug. 20, 2019, 5:37 a.m. UTC | #4
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.
>

Patch
diff mbox series

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);
 }