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

[Stuart Yoder]

> -----Original Message-----
> From: Ruxandra Ioana Radulescu
> Sent: Thursday, November 10, 2016 9:04 AM
> To: Stuart Yoder <stuart.yoder@xxxxxxx>; 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>; Haiying Wang
> <haiying.wang@xxxxxxx>; Stuart Yoder <stuart.yoder@xxxxxxx>
> Subject: RE: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
> 
> 
> > -----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.

Will fix on next respin.

> > +	/*
> > +	 * 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.

Ok, will fix.

> > +
> > +	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.

Ok, will fix.

Thanks for the review.

Stuart