[Pdbg] [RFC 03/12] sbefifo: Rework thread functions

[Amitay Isaacs]

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 at 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.
-- 

Good judgement comes from experience, which comes from bad judgement.