[Pdbg] [PATCH 14/17] libpdbg: Expand sbefifo data marshalling and calling chip-op
Amitay Isaacs
amitay at ozlabs.org
Tue Nov 5 15:39:42 AEDT 2019
On Tue, 2019-11-05 at 15:18 +1100, Alistair Popple wrote:
> On Thursday, 31 October 2019 5:34:25 PM AEDT Amitay Isaacs wrote:
> > This will allow to separate sbefifo transport layer to use
> > something other
> > than sbefifo kernel device driver.
> >
> > Signed-off-by: Amitay Isaacs <amitay at ozlabs.org>
> > ---
> > libpdbg/sbefifo.c | 132 ++++++++++++++++++++++++++++------------
> > ------
> > 1 file changed, 80 insertions(+), 52 deletions(-)
> >
> > diff --git a/libpdbg/sbefifo.c b/libpdbg/sbefifo.c
> > index 030e6e9..9e3532b 100644
> > --- a/libpdbg/sbefifo.c
> > +++ b/libpdbg/sbefifo.c
> > @@ -30,12 +30,29 @@ static uint32_t sbefifo_op_ffdc_get(struct
> > sbefifo *sbefifo,
> const uint8_t **ffd
> > return sbefifo_ffdc_get(sbefifo->sf_ctx, ffdc, ffdc_len);
> > }
> >
> > -static int sbefifo_op_istep(struct sbefifo *sbefifo,
> > - uint32_t major, uint32_t minor)
> > +static int sbefifo_op_istep(struct sbefifo *sbefifo, uint32_t
> > major, uint32_t
> minor)
> > {
> > + uint8_t *msg, *out;
> > + uint32_t msg_len, out_len;
> > + int rc;
> > +
> > PR_NOTICE("sbefifo: istep %u.%u\n", major, minor);
> >
> > - return sbefifo_istep_execute(sbefifo->sf_ctx, major & 0xff,
> > minor & 0xff);
> > + rc = sbefifo_istep_execute_push(major & 0xff, minor & 0xff,
> > &msg, &msg_len);
> > + if (rc)
> > + return rc;
> > +
> > + out_len = 0;
> > + rc = sbefifo_operation(sbefifo->sf_ctx, msg, msg_len, &out,
> > &out_len);
>
> This code looks pretty much the same as what is in libsbefifo, so why
> not just
> call that?
>
> I see that later on in the series you change the sbefifo_operation()
> call to
> sbefifo->chipop(), but given you have defined the struct
> sbefifo_context and it's
> still required for calls to libsbefifo why not just define a callback
> in the
> struct? That way pdbg can just call sbefifo_op_istep() and the
> libsbefifo
> library can call something like sf_ctx->transport() which does the
> right thing
> based on how the context is initialised.
Well, the whole point was not to embed the sbefifo transport deep
inside libsbefifo. But allow it to be layered by device tree.
The idea of registering transport was a hack to get cronus working.
But it's not as elegant as organizing the right targets in device tree
and the transport then works automatically.
I guess if you see the other patch series, it might become apparent why
I have taken this approach.
>
> - Alistair
>
> > + free(msg);
> > + if (rc)
> > + return rc;
> > +
> > + rc = sbefifo_istep_execute_pull(out, out_len);
> > + if (out)
> > + free(out);
> > +
> > + return rc;
> > }
> >
> > static int sbefifo_op_getmem(struct mem *sbefifo_mem,
> > @@ -43,48 +60,45 @@ static int sbefifo_op_getmem(struct mem
> > *sbefifo_mem,
> > uint8_t block_size, bool ci)
> > {
> > struct sbefifo *sbefifo = target_to_sbefifo(sbefifo_mem-
> > >target.parent);
> > - uint8_t *out;
> > - uint64_t start_addr, end_addr;
> > - uint32_t align, offset, len;
> > + uint8_t *msg, *out;
> > + uint32_t msg_len, out_len, len;
> > uint16_t flags;
> > int rc;
> >
> > - align = 8;
> > -
> > - if (block_size && block_size != 8) {
> > - PR_ERROR("sbefifo: Only 8 byte block sizes are
> > supported\n");
> > - return -1;
> > - };
> > -
> > - start_addr = addr & (~(uint64_t)(align-1));
> > - end_addr = (addr + size + (align-1)) & (~(uint64_t)(align-1));
> > -
> > - if (end_addr - start_addr > UINT32_MAX) {
> > - PR_ERROR("sbefifo: size too large\n");
> > - return -EINVAL;
> > + if (size > UINT32_MAX) {
> > + PR_ERROR("sbefifo: Invalid size for getmem\n");
> > + return EINVAL;
> > }
> >
> > - offset = addr - start_addr;
> > - len = end_addr - start_addr;
> > + len = size & 0xffffffff;
> >
> > - PR_NOTICE("sbefifo: getmem addr=0x%016" PRIx64 ", len=%u\n",
> > - start_addr, len);
> > + PR_NOTICE("sbefifo: getmem addr=0x%016" PRIx64 ", len=%u\n",
> > addr, len);
> >
> > flags = SBEFIFO_MEMORY_FLAG_PROC;
> > if (ci)
> > flags |= SBEFIFO_MEMORY_FLAG_CI;
> >
> > - rc = sbefifo_mem_get(sbefifo->sf_ctx, start_addr, len, flags,
> > &out);
> > -
> > - pdbg_progress_tick(len, len);
> > + rc = sbefifo_mem_get_push(addr, len, flags, &msg, &msg_len);
> > + if (rc)
> > + return rc;
> >
> > + out_len = len + 4;
> > + rc = sbefifo_operation(sbefifo->sf_ctx, msg, msg_len, &out,
> > &out_len);
> > + free(msg);
> > if (rc)
> > return rc;
> >
> > - memcpy(data, out+offset, size);
> > - free(out);
> > + rc = sbefifo_mem_get_pull(out, out_len, addr, size, flags,
> > &msg);
> > + if (out)
> > + free(out);
> >
> > - return 0;
> > + if (!rc) {
> > + memcpy(data, msg, len);
> > + free(msg);
> > + pdbg_progress_tick(len, len);
> > + }
> > +
> > + return rc;
> > }
> >
> > static int sbefifo_op_putmem(struct mem *sbefifo_mem,
> > @@ -92,30 +106,14 @@ static int sbefifo_op_putmem(struct mem
> > *sbefifo_mem,
> > uint8_t block_size, bool ci)
> > {
> > struct sbefifo *sbefifo = target_to_sbefifo(sbefifo_mem-
> > >target.parent);
> > - uint32_t align, len;
> > + uint8_t *msg, *out;
> > + uint32_t msg_len, out_len, len;
> > uint16_t flags;
> > int rc;
> >
> > - align = 8;
> > -
> > - if (block_size && block_size != 8) {
> > - PR_ERROR("sbefifo: Only 8 byte block sizes are
> > supported\n");
> > - return -1;
> > - };
> > -
> > - if (addr & (align-1)) {
> > - PR_ERROR("sbefifo: Address must be aligned to %d
> > bytes\n", align);
> > - return -1;
> > - }
> > -
> > - if (size & (align-1)) {
> > - PR_ERROR("sbefifo: Data must be multiple of %d
> > bytes\n", align);
> > - return -1;
> > - }
> > -
> > if (size > UINT32_MAX) {
> > - PR_ERROR("sbefifo: size too large\n");
> > - return -1;
> > + PR_ERROR("sbefifo: Invalid size for putmem\n");
> > + return EINVAL;
> > }
> >
> > len = size & 0xffffffff;
> > @@ -126,9 +124,22 @@ static int sbefifo_op_putmem(struct mem
> > *sbefifo_mem,
> > if (ci)
> > flags |= SBEFIFO_MEMORY_FLAG_CI;
> >
> > - rc = sbefifo_mem_put(sbefifo->sf_ctx, addr, data, len, flags);
> > + rc = sbefifo_mem_put_push(addr, data, len, flags, &msg,
> > &msg_len);
> > + if (rc)
> > + return rc;
> > +
> > + out_len = 4;
> > + rc = sbefifo_operation(sbefifo->sf_ctx, msg, msg_len, &out,
> > &out_len);
> > + free(msg);
> > + if (rc)
> > + return rc;
> >
> > - pdbg_progress_tick(len, len);
> > + rc = sbefifo_mem_put_pull(out, out_len);
> > + if (out)
> > + free(out);
> > +
> > + if (!rc)
> > + pdbg_progress_tick(len, len);
> >
> > return rc;
> > }
> > @@ -137,6 +148,9 @@ static int sbefifo_op_control(struct sbefifo
> > *sbefifo,
> > uint32_t core_id, uint32_t thread_id,
> > uint32_t oper)
> > {
> > + uint8_t *msg, *out;
> > + uint32_t msg_len, out_len;
> > + int rc;
> > uint8_t mode = 0;
> >
> > /* Enforce special-wakeup for thread stop and sreset */
> > @@ -146,7 +160,21 @@ static int sbefifo_op_control(struct sbefifo
> > *sbefifo,
> >
> > PR_NOTICE("sbefifo: control c:0x%x, t:0x%x, op:%u mode:%u\n",
> > core_id,
> thread_id, oper, mode);
> >
> > - return sbefifo_control_insn(sbefifo->sf_ctx, core_id & 0xff,
> > thread_id &
> 0xff, oper & 0xff, mode);
> > + rc = sbefifo_control_insn_push(core_id & 0xff, thread_id &
> > 0xff, oper & 0xff,
> mode, &msg, &msg_len);
> > + if (rc)
> > + return rc;
> > +
> > + out_len = 0;
> > + rc = sbefifo_operation(sbefifo->sf_ctx, msg, msg_len, &out,
> > &out_len);
> > + free(msg);
> > + if (rc)
> > + return rc;
> > +
> > + rc = sbefifo_control_insn_pull(out, out_len);
> > + if (out)
> > + free(out);
> > +
> > + return rc;
> > }
> >
> > static int sbefifo_op_thread_start(struct sbefifo *sbefifo,
> >
>
>
>
Amitay.
--
He, that lives upon hope will die fasting.
More information about the Pdbg
mailing list