[Pdbg] [PATCH v2 1/6] libsbefifo: Support a transport callback when talking to sbefifo

[Alistair Popple]

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