[Pdbg] [PATCH v2 1/6] libsbefifo: Support a transport callback when talking to sbefifo
Alistair Popple
alistair at popple.id.au
Tue Oct 29 11:39:14 AEDT 2019
On Thursday, 24 October 2019 12:53:26 PM AEDT Amitay Isaacs wrote:
> This will allow to send sbefifo chip-op over transports other than sbefifo
> device. This is designed to use with cronus backend.
>
> Signed-off-by: Amitay Isaacs <amitay at ozlabs.org>
> ---
> libsbefifo/libsbefifo.h | 7 +++++++
> libsbefifo/operation.c | 30 +++++++++++++++++++++---------
> libsbefifo/sbefifo_private.h | 5 +++++
> 3 files changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/libsbefifo/libsbefifo.h b/libsbefifo/libsbefifo.h
> index 84b0169..2963450 100644
> --- a/libsbefifo/libsbefifo.h
> +++ b/libsbefifo/libsbefifo.h
> @@ -18,6 +18,7 @@
> #define __LIBSBEFIFO_H__
>
> #include <stdint.h>
> +#include <unistd.h>
>
> #define SBEFIFO_PRI_SUCCESS 0x00000000
> #define SBEFIFO_PRI_INVALID_COMMAND 0x00010000
> @@ -48,6 +49,12 @@
>
> struct sbefifo_context;
>
> +typedef int (*sbefifo_transport_fn)(uint8_t *request_buf,
> + uint32_t request_len,
> + uint8_t *reply_buf,
> + uint32_t *reply_len,
> + void *priv);
> +
It feels like the abstraction/layering isn't quite right here if libsbefifo
needs to understand the specific transports used as that is the role of libpdbg
in the system.
> int sbefifo_connect(const char *fifo_path, struct sbefifo_context **out);
> void sbefifo_disconnect(struct sbefifo_context *sctx);
>
> diff --git a/libsbefifo/operation.c b/libsbefifo/operation.c
> index 1ad577c..3056c36 100644
> --- a/libsbefifo/operation.c
> +++ b/libsbefifo/operation.c
> @@ -80,16 +80,28 @@ int sbefifo_operation(struct sbefifo_context *sctx,
>
> LOG("request: cmd=%08x, len=%u\n", cmd, msg_len);
>
> - rc = sbefifo_write(sctx, msg, msg_len);
> - if (rc) {
> - LOG("write: cmd=%08x, rc=%d\n", cmd, rc);
> - return rc;
> - }
> + if (sctx->transport_func) {
> + uint32_t reply_len = buflen;
> +
> + rc = sctx->transport_func(msg, msg_len, buf, &reply_len, sctx->priv);
> + if (rc) {
> + LOG("msg: cmd=%08x, rc=%d\n", cmd, rc);
> + return rc;
> + }
> + buflen = reply_len;
>
> - rc = sbefifo_read(sctx, buf, &buflen);
> - if (rc) {
> - LOG("read: cmd=%08x, buflen=%zu, rc=%d\n", cmd, buflen, rc);
> - return rc;
> + } else {
> + rc = sbefifo_write(sctx, msg, msg_len);
> + if (rc) {
> + LOG("write: cmd=%08x, rc=%d\n", cmd, rc);
> + return rc;
> + }
> +
> + rc = sbefifo_read(sctx, buf, &buflen);
> + if (rc) {
> + LOG("read: cmd=%08x, buflen=%zu, rc=%d\n", cmd, buflen, rc);
> + return rc;
> + }
> }
It looks to me like the sbefifo_read/write calls really belong in libpdbg and
that the sbefifo_context should simply contain the transport_func callback and
libsbefifo should just be concerned with formatting and handling of the chip-
ops themselves.
As always there are multiple hardware paths to the sbefifo (eg. there are also
ways of accessing it from the host) and we don't want that knowledge encoded
in libsbefifo.
- Alistair
> /*
> diff --git a/libsbefifo/sbefifo_private.h b/libsbefifo/sbefifo_private.h
> index 87416b7..c985bba 100644
> --- a/libsbefifo/sbefifo_private.h
> +++ b/libsbefifo/sbefifo_private.h
> @@ -19,6 +19,8 @@
>
> #include <stdint.h>
>
> +#include "libsbefifo.h"
> +
> #define SBEFIFO_CMD_CLASS_CONTROL 0xA100
> #define SBEFIFO_CMD_EXECUTE_ISTEP 0x01
>
> @@ -65,6 +67,9 @@
> struct sbefifo_context {
> int fd;
>
> + sbefifo_transport_fn transport_func;
> + void *priv;
> +
> uint32_t status;
> uint8_t *ffdc;
> uint32_t ffdc_len;
>
More information about the Pdbg
mailing list