From: Stuart Yoder <stuart.yoder@nxp.com>
To: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
CC: German Rivera <german.rivera@nxp.com>,
        "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "agraf@suse.de" <agraf@suse.de>, "arnd@arndb.de" <arnd@arndb.de>,
        Leo Li <leoyang.li@nxp.com>, Roy Pledge <roy.pledge@nxp.com>,
        Haiying Wang <haiying.wang@nxp.com>
Subject: RE: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
Thread-Topic: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
Thread-Index: AQHSK6SU/JJyAb48sEOHR53H+I1ID6DHaGwwgAsET1CAACqIYA==
Date: Thu, 10 Nov 2016 17:25:38 +0000
Message-ID: <VI1PR0401MB263805FEF8B0055215D8E2688DB80@VI1PR0401MB2638.eurprd04.prod.outlook.com>
References: <1477058509-12547-1-git-send-email-stuart.yoder@nxp.com>
 <1477058509-12547-7-git-send-email-stuart.yoder@nxp.com>
 <VI1PR0401MB26389BCC00429B2C3396C3CD8DA30@VI1PR0401MB2638.eurprd04.prod.outlook.com>
 <VI1PR0402MB284781F3750D60017160690794B80@VI1PR0402MB2847.eurprd04.prod.outlook.com>
In-Reply-To: <VI1PR0402MB284781F3750D60017160690794B80@VI1PR0402MB2847.eurprd04.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Nov 2016 17:25:38.4976
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0401MB1759
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uAAHebxO025323
Content-Length: 5847
Lines: 178


> -----Original Message-----
> From: Ruxandra Ioana Radulescu
> Sent: Thursday, November 10, 2016 9:04 AM
> To: Stuart Yoder <stuart.yoder@nxp.com>; gregkh@linuxfoundation.org
> Cc: German Rivera <german.rivera@nxp.com>; devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org;
> agraf@suse.de; arnd@arndb.de; Leo Li <leoyang.li@nxp.com>; Roy Pledge <roy.pledge@nxp.com>; Haiying Wang
> <haiying.wang@nxp.com>; Stuart Yoder <stuart.yoder@nxp.com>
> Subject: RE: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
> 
> 
> > -----Original Message-----
> > From: Stuart Yoder [mailto:stuart.yoder@nxp.com]
> > Sent: Friday, October 21, 2016 9:02 AM
> > To: gregkh@linuxfoundation.org
> > Cc: German Rivera <german.rivera@nxp.com>; devel@driverdev.osuosl.org;
> > linux-kernel@vger.kernel.org; agraf@suse.de; arnd@arndb.de; Leo Li
> > <leoyang.li@nxp.com>; Roy Pledge <roy.pledge@nxp.com>; Roy Pledge
> > <roy.pledge@nxp.com>; Haiying Wang <haiying.wang@nxp.com>; Stuart
> > Yoder <stuart.yoder@nxp.com>
> > Subject: [PATCH 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2
> >
> > From: Roy Pledge <Roy.Pledge@nxp.com>
> >
> > Add QBman APIs for frame queue and buffer pool operations.
> >
> > Signed-off-by: Roy Pledge <roy.pledge@nxp.com>
> > Signed-off-by: Haiying Wang <haiying.wang@nxp.com>
> > Signed-off-by: Stuart Yoder <stuart.yoder@nxp.com>
> > ---
> >  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