RE: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2

[Ruxandra Ioana Radulescu]

> -----Original Message-----
> From: Stuart Yoder [mailto:stuart.yoder@xxxxxxx]
> Sent: Friday, October 21, 2016 9:02 AM
> To: gregkh@xxxxxxxxxxxxxxxxxxx
> Cc: German Rivera <german.rivera@xxxxxxx>; devel@xxxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; agraf@xxxxxxx; arnd@xxxxxxxx; Leo Li
> <leoyang.li@xxxxxxx>; Roy Pledge <roy.pledge@xxxxxxx>; Roy Pledge
> <roy.pledge@xxxxxxx>; Haiying Wang <haiying.wang@xxxxxxx>; Stuart
> Yoder <stuart.yoder@xxxxxxx>
> Subject: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
> 
> From: Roy Pledge <Roy.Pledge@xxxxxxx>
> 
> Add QBman APIs for frame queue and buffer pool operations.
> 
> Signed-off-by: Roy Pledge <roy.pledge@xxxxxxx>
> Signed-off-by: Haiying Wang <haiying.wang@xxxxxxx>
> Signed-off-by: Stuart Yoder <stuart.yoder@xxxxxxx>
> ---
>  drivers/bus/fsl-mc/dpio/Makefile       |    2 +-
>  drivers/bus/fsl-mc/dpio/qbman-portal.c | 1009
> ++++++++++++++++++++++++++++++++
>  drivers/bus/fsl-mc/dpio/qbman-portal.h |  464 +++++++++++++++
>  3 files changed, 1474 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/bus/fsl-mc/dpio/qbman-portal.c
>  create mode 100644 drivers/bus/fsl-mc/dpio/qbman-portal.h
> 
> diff --git a/drivers/bus/fsl-mc/dpio/Makefile b/drivers/bus/fsl-
> mc/dpio/Makefile
> index 128befc..6588498 100644
> --- a/drivers/bus/fsl-mc/dpio/Makefile
> +++ b/drivers/bus/fsl-mc/dpio/Makefile
> @@ -6,4 +6,4 @@ subdir-ccflags-y := -Werror
> 
>  obj-$(CONFIG_FSL_MC_DPIO) += fsl-mc-dpio.o
> 
> -fsl-mc-dpio-objs := dpio.o
> +fsl-mc-dpio-objs := dpio.o qbman-portal.o
> diff --git a/drivers/bus/fsl-mc/dpio/qbman-portal.c b/drivers/bus/fsl-
> mc/dpio/qbman-portal.c
> new file mode 100644
> index 0000000..1eb3dd9
> --- /dev/null
> +++ b/drivers/bus/fsl-mc/dpio/qbman-portal.c
> @@ -0,0 +1,1009 @@

[...] 

> +/**
> + * qbman_swp_release() - Issue a buffer release command
> + * @s:           the software portal object
> + * @d:           the release descriptor
> + * @buffers:     a pointer pointing to the buffer address to be released
> + * @num_buffers: number of buffers to be released,  must be less than 8
> + *
> + * Return 0 for success, -EBUSY if the release command ring is not ready.
> + */
> +int qbman_swp_release(struct qbman_swp *s, const struct
> qbman_release_desc *d,
> +		      const u64 *buffers, unsigned int num_buffers)
> +{
> +	int i;
> +	struct qbman_release_desc *p;
> +	u32 rar;
> +
> +	if (!num_buffers || (num_buffers > 7))
> +		return -EINVAL;
> +
> +	rar = qbman_read_register(s, QBMAN_CINH_SWP_RAR);
> +	if (!RAR_SUCCESS(rar))
> +		return -EBUSY;
> +
> +	/* Start the release command */
> +	p = qbman_get_cmd(s, QBMAN_CENA_SWP_RCR(RAR_IDX(rar)));
> +	/* Copy the caller's buffer pointers to the command */
> +	for (i = 0; i < num_buffers; i++)
> +		p->buf[i] = cpu_to_le64(buffers[i]);
> +

Hi Stuart,
We also need to set BPID field in the buffer release command, something like:
+       p->bpid = d->bpid;
Without this all buffers will be released to buffer pool id 0, which is incorrect.

> +	/*
> +	 * Set the verb byte, have to substitute in the valid-bit and the
> number
> +	 * of buffers.
> +	 */
> +	dma_wmb();
> +	p->verb = d->verb | RAR_VB(rar) | num_buffers;
> +
> +	return 0;
> +}
> +
> +struct qbman_acquire_desc {
> +	u8 verb;
> +	u8 reserved;
> +	u16 bpid;
> +	u8 num;
> +	u8 reserved2[59];
> +};
> +
> +struct qbman_acquire_rslt {
> +	u8 verb;
> +	u8 rslt;
> +	u16 reserved;
> +	u8 num;
> +	u8 reserved2[3];
> +	u64 buf[7];
> +};
> +
> +/**
> + * qbman_swp_acquire() - Issue a buffer acquire command
> + * @s:           the software portal object
> + * @bpid:        the buffer pool index
> + * @buffers:     a pointer pointing to the acquired buffer addresses
> + * @num_buffers: number of buffers to be acquired, must be less than 8
> + *
> + * Return 0 for success, or negative error code if the acquire command
> + * fails.
> + */
> +int qbman_swp_acquire(struct qbman_swp *s, u16 bpid, u64 *buffers,
> +		      unsigned int num_buffers)
> +{
> +	struct qbman_acquire_desc *p;
> +	struct qbman_acquire_rslt *r;
> +	int i;
> +
> +	if (!num_buffers || (num_buffers > 7))
> +		return -EINVAL;
> +
> +	/* Start the management command */
> +	p = qbman_swp_mc_start(s);

qbman_swp_mc_start() returns a pointer to where the QBMan
management command must be written, but doesn't clear any
previous values found there.
We should memset the area to zero before using it.
Same comment applies to other places where this function is used.

> +
> +	if (!p)
> +		return -EBUSY;
> +
> +	/* Encode the caller-provided attributes */
> +	p->bpid = cpu_to_le16(bpid);
> +	p->num = num_buffers;
> +
> +	/* Complete the management command */
> +	r = qbman_swp_mc_complete(s, p, p->verb |
> QBMAN_MC_ACQUIRE);

qbman_swp_mc_complete() may return NULL, if for instance the hardware 
is unresponsive. We need to check this before dereferencing r.
Same for other instances of usage throughout the code.

Thanks,
Ioana