[Pdbg] [PATCH 14/17] libpdbg: Expand sbefifo data marshalling and calling chip-op
Alistair Popple
alistair at popple.id.au
Tue Nov 5 17:22:15 AEDT 2019
On Tuesday, 5 November 2019 3:39:42 PM AEDT Amitay Isaacs wrote:
> 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.
This wouldn't change that though, the transport layer would just initialise
libsbefifo differently as part of the _probe() function. Really though I think
we are just arguing over two equally viable approaches:
1. Have the complete chip-ops (eg. getmem) defined in libsbefifo and have
libsbefifo do a context-specific callback to handle the transport.
2. Make libsbefifo purely a request marshaling/formatting and response parsing
library and have the complete chip-op implementation defined in libpdbg.
At the moment we seem to doing both approaches which is what is leading to the
code duplication from libsbefifo in this patch. I think we should either have
libsbefifo define just the _push()/_pull() operations and not the overall chip-
ops (eg. by removing sbefifo_mem_get and the sbefifo_context struct from
libsbefifo) or we should have libsbefifo take a call-back during
inititialisation of the sbefifo_context.
- Alistair
> 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.
>
More information about the Pdbg
mailing list