RE: [PATCH v3 4/9] bus: fsl-mc: dpio: add frame descriptor and scatter/gather APIs

[Stuart Yoder]

> -----Original Message-----
> From: Laurentiu Tudor
> Sent: Friday, December 02, 2016 6:12 AM
> To: Stuart Yoder <stuart.yoder@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx
> Cc: devel@xxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; agraf@xxxxxxx; arnd@xxxxxxxx; Leo Li
> <leoyang.li@xxxxxxx>; Ioana Ciornei <ioana.ciornei@xxxxxxx>; Catalin Horghidan
> <catalin.horghidan@xxxxxxx>; Ruxandra Ioana Radulescu <ruxandra.radulescu@xxxxxxx>; Roy Pledge
> <roy.pledge@xxxxxxx>
> Subject: Re: [PATCH v3 4/9] bus: fsl-mc: dpio: add frame descriptor and scatter/gather APIs
> 
> 
> Some more bits and pieces inside.
> 
> ---
> Best Regards, Laurentiu
> 
> On 12/02/2016 12:41 AM, Stuart Yoder wrote:
> > From: Roy Pledge <Roy.Pledge@xxxxxxx>
> >
> > Add global definitions for DPAA2 frame descriptors and scatter
> > gather entries.
> >
> > Signed-off-by: Roy Pledge <Roy.Pledge@xxxxxxx>
> > Signed-off-by: Stuart Yoder <stuart.yoder@xxxxxxx>
> > ---
> >
> > Notes:
> >     -v3
> >        -no changes
> >     -v2
> >        -added setter/getter for the FD ctrl field
> >        -corrected comment for SG format_offset field description
> >        -added support for short length field in FD
> >
> >  include/linux/fsl/dpaa2-fd.h | 448 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 448 insertions(+)
> >  create mode 100644 include/linux/fsl/dpaa2-fd.h
> >
> > diff --git a/include/linux/fsl/dpaa2-fd.h b/include/linux/fsl/dpaa2-fd.h
> > new file mode 100644
> > index 0000000..182c8f4
> > --- /dev/null
> > +++ b/include/linux/fsl/dpaa2-fd.h
> > @@ -0,0 +1,448 @@
> > +/*
> > + * Copyright 2014-2016 Freescale Semiconductor Inc.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions are met:
> > + *     * Redistributions of source code must retain the above copyright
> > + *       notice, this list of conditions and the following disclaimer.
> > + *     * Redistributions in binary form must reproduce the above copyright
> > + *       notice, this list of conditions and the following disclaimer in the
> > + *       documentation and/or other materials provided with the distribution.
> > + *     * Neither the name of Freescale Semiconductor nor the
> > + *       names of its contributors may be used to endorse or promote products
> > + *       derived from this software without specific prior written permission.
> > + *
> > + * ALTERNATIVELY, this software may be distributed under the terms of the
> > + * GNU General Public License ("GPL") as published by the Free Software
> > + * Foundation, either version 2 of that License or (at your option) any
> > + * later version.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY Freescale Semiconductor ``AS IS'' AND ANY
> > + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> > + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> > + * DISCLAIMED. IN NO EVENT SHALL Freescale Semiconductor BE LIABLE FOR ANY
> > + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> > + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> > + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> > + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> > + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > + */
> > +#ifndef __FSL_DPAA2_FD_H
> > +#define __FSL_DPAA2_FD_H
> > +
> > +#include <linux/kernel.h>
> > +
> > +/**
> > + * DOC: DPAA2 FD - Frame Descriptor APIs for DPAA2
> > + *
> > + * Frame Descriptors (FDs) are used to describe frame data in the DPAA2.
> > + * Frames can be enqueued and dequeued to Frame Queues (FQs) which are consumed
> > + * by the various DPAA accelerators (WRIOP, SEC, PME, DCE)
> > + *
> > + * There are three types of frames: single, scatter gather, and frame lists.
> > + *
> > + * The set of APIs in this file must be used to create, manipulate and
> > + * query Frame Descriptors.
> > + */
> > +
> > +/**
> > + * struct dpaa2_fd - Struct describing FDs
> > + * @words:         for easier/faster copying the whole FD structure
> > + * @addr:          address in the FD
> > + * @len:           length in the FD
> > + * @bpid:          buffer pool ID
> > + * @format_offset: format, offset, and short-length fields
> > + * @frc:           frame context
> > + * @ctrl:          control bits...including dd, sc, va, err, etc
> > + * @flc:           flow context address
> > + *
> > + * This structure represents the basic Frame Descriptor used in the system.
> > + */
> > +struct dpaa2_fd {
> > +	union {
> > +		u32 words[8];
> > +		struct dpaa2_fd_simple {
> > +			__le64 addr;
> > +			__le32 len;
> > +			__le16 bpid;
> > +			__le16 format_offset;
> > +			__le32 frc;
> > +			__le32 ctrl;
> > +			__le64 flc;
> > +		} simple;
> > +	};
> > +};
> > +
> > +#define FD_SHORT_LEN_FLAG_MASK 0x1
> > +#define FD_SHORT_LEN_FLAG_SHIFT 14
> > +#define FD_SHORT_LEN_MASK 0x1FFFF
> > +#define FD_OFFSET_MASK 0x0FFF
> > +#define FD_FORMAT_MASK 0x3
> > +#define FD_FORMAT_SHIFT 12
> > +#define SG_SHORT_LEN_FLAG_MASK 0x1
> > +#define SG_SHORT_LEN_FLAG_SHIFT 14
> > +#define SG_SHORT_LEN_MASK 0x1FFFF
> > +#define SG_OFFSET_MASK 0x0FFF
> > +#define SG_FORMAT_MASK 0x3
> > +#define SG_FORMAT_SHIFT 12
> > +#define SG_BPID_MASK 0x3FFF
> > +#define SG_FINAL_FLAG_MASK 0x1
> > +#define SG_FINAL_FLAG_SHIFT 15
> 
> We should align the values of these macros as we do in other places.

Yes, agree.

> > +enum dpaa2_fd_format {
> > +	dpaa2_fd_single = 0,
> > +	dpaa2_fd_list,
> > +	dpaa2_fd_sg
> > +};
> > +
> > +/**
> > + * dpaa2_fd_get_addr() - get the addr field of frame descriptor
> > + * @fd: the given frame descriptor
> > + *
> > + * Return the address in the frame descriptor.
> > + */
> > +static inline dma_addr_t dpaa2_fd_get_addr(const struct dpaa2_fd *fd)
> > +{
> > +
> 
> Extra empty line.

Ok.

> > +/**
> > + * dpaa2_fd_set_offset() - Set the offset field of frame descriptor
> > + * @fd: the given frame descriptor
> > + * @offset: the offset needs to be set in frame descriptor
> > + */
> > +static inline void dpaa2_fd_set_offset(struct dpaa2_fd *fd, uint16_t offset)
> > +{
> > +	fd->simple.format_offset &= ~(FD_OFFSET_MASK);
> 
> Paranthesis not needed.

Ok.

> > +/**
> > + * dpaa2_sg_set_offset() - Set the offset in SG entry
> > + * @sg: the given scatter-gathering object
> > + * @offset: the offset to be set
> > + */
> > +static inline void dpaa2_sg_set_offset(struct dpaa2_sg_entry *sg,
> > +				       u16 offset)
> > +{
> > +	sg->format_offset &= cpu_to_le16(~(SG_OFFSET_MASK));
> 
> Extra-paranthesis around SG_OFFSET_MASK not needed.

Ok.

> > +/**
> > + * dpaa2_sg_is_final() - Check final bit in SG entry
> > + * @sg: the given scatter-gathering object
> > + *
> > + * Return bool.
> > + */
> > +static inline bool dpaa2_sg_is_final(const struct dpaa2_sg_entry *sg)
> > +{
> > +	return !!(le16_to_cpu(sg->format_offset) >> SG_FINAL_FLAG_SHIFT);
> 
> In other places in this file we don't use the '!!' to convert to bool. Maybe we should drop it here too.

As per the separate comments with Dan, I'll leave and check for consistency
with other places in this file.

Thanks,
Stuart