dmaengine Archive on lore.kernel.org
 help / color / Atom feed
From: Vinod Koul <vkoul@kernel.org>
To: Peng Ma <peng.ma@nxp.com>
Cc: dan.j.williams@intel.com, leoyang.li@nxp.com,
	linux-kernel@vger.kernel.org, dmaengine@vger.kernel.org
Subject: Re: [V4 1/2] dmaengine: fsl-dpaa2-qdma: Add the DPDMAI(Data Path DMA Interface) support
Date: Mon, 24 Jun 2019 17:55:09 +0530
Message-ID: <20190624122509.GC2962@vkoul-mobl> (raw)
In-Reply-To: <20190613101341.21169-1-peng.ma@nxp.com>

On 13-06-19, 10:13, Peng Ma wrote:
> The MC(Management Complex) exports the DPDMAI(Data Path DMA Interface)
> object as an interface to operate the DPAA2(Data Path Acceleration
> Architecture 2) qDMA Engine. The DPDMAI enables sending frame-based
> requests to qDMA and receiving back confirmation response on transaction
> completion, utilizing the DPAA2 QBMan(Queue Manager and Buffer Manager
> hardware) infrastructure. DPDMAI object provides up to two priorities for
> processing qDMA requests.
> The following list summarizes the DPDMAI main features and capabilities:
> 	1. Supports up to two scheduling priorities for processing
> 	service requests.
> 	- Each DPDMAI transmit queue is mapped to one of two service
> 	priorities, allowing further prioritization in hardware between
> 	requests from different DPDMAI objects.
> 	2. Supports up to two receive queues for incoming transaction
> 	completion confirmations.
> 	- Each DPDMAI receive queue is mapped to one of two receive
> 	priorities, allowing further prioritization between other
> 	interfaces when associating the DPDMAI receive queues to DPIO
> 	or DPCON(Data Path Concentrator) objects.
> 	3. Supports different scheduling options for processing received
> 	packets:
> 	- Queues can be configured either in 'parked' mode (default),
> 	oattached to a DPIO object, or attached to DPCON object.
        ^^^^^^^^^
typo?

> +struct dpdmai_cmd_open {
> +	__le32 dpdmai_id;
> +};

Do you really need a struct to handle a 32bit value?

And seeing it used once, we can skip and avoid needless cast in usage as
well!

> +/* cmd, param, offset, width, type, arg_name */
> +#define DPDMAI_CMD_CREATE(_cmd, _cfg) \
> +do { \
> +	typeof(_cmd) (cmd) = (_cmd); \
> +	typeof(_cfg) (cfg) = (_cfg); \
> +	MC_CMD_OP(cmd, 0, 8,  8,  u8,  (cfg)->priorities[0]);\
> +	MC_CMD_OP(cmd, 0, 16, 8,  u8,  (cfg)->priorities[1]);\
> +} while (0)
> +
> +static inline u64 mc_enc(int lsoffset, int width, u64 val)
> +{
> +	return (u64)(((u64)val & MAKE_UMASK64(width)) << lsoffset);

this looks not so nice. val is u64 so why is it required to cast to u64
again?

> +}
> +
> +static inline u64 mc_dec(u64 val, int lsoffset, int width)
> +{
> +	return (u64)((val >> lsoffset) & MAKE_UMASK64(width));

do we need the cast here?

> +int dpdmai_create(struct fsl_mc_io *mc_io, u32 cmd_flags,
> +		  const struct dpdmai_cfg *cfg, u16 *token)
> +{
> +	struct fsl_mc_command cmd = { 0 };
> +	int err;
> +
> +	/* prepare command */
> +	cmd.header = mc_encode_cmd_header(DPDMAI_CMDID_CREATE,
> +					  cmd_flags,
> +					  0);

This seems to fit in a single line!

> +	DPDMAI_CMD_CREATE(cmd, cfg);
> +
> +	/* send command to mc*/
> +	err = mc_send_command(mc_io, &cmd);
> +	if (err)
> +		return err;
> +
> +	/* retrieve response parameters */
> +	*token = MC_CMD_HDR_READ_TOKEN(cmd.header);

This looks very similar to dpdmai_open() and I suppose you can create a
macro to create and send cmd with optional params!

> +
> +	return 0;
> +}
> +
> +int dpdmai_enable(struct fsl_mc_io *mc_io, u32 cmd_flags,
> +		  u16 token)
> +{
> +	struct fsl_mc_command cmd = { 0 };
> +
> +	/* prepare command */
> +	cmd.header = mc_encode_cmd_header(DPDMAI_CMDID_ENABLE,
> +					  cmd_flags,
> +					  token);

This can fit in two lines

> +
> +	/* send command to mc*/
> +	return mc_send_command(mc_io, &cmd);
> +}
> +
> +int dpdmai_disable(struct fsl_mc_io *mc_io, u32 cmd_flags,
> +		   u16 token)

why two lines for this!

> +{
> +	struct fsl_mc_command cmd = { 0 };
> +
> +	/* prepare command */
> +	cmd.header = mc_encode_cmd_header(DPDMAI_CMDID_DISABLE,
> +					  cmd_flags,
> +					  token);

This also!

Please check rest of the driver for these style issues and see bunch of
places can fit into a line or two!

> +/**
> + * dpdmai_open() - Open a control session for the specified object
> + * @mc_io:	Pointer to MC portal's I/O object
> + * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
> + * @dpdmai_id:	DPDMAI unique ID
> + * @token:	Returned token; use in subsequent API calls
> + *
> + * This function can be used to open a control session for an
> + * already created object; an object may have been declared in
> + * the DPL or by calling the dpdmai_create() function.
> + * This function returns a unique authentication token,
> + * associated with the specific object ID and the specific MC
> + * portal; this token must be used in all subsequent commands for
> + * this specific object.

This is good but can you move them to C file with the function

-- 
~Vinod

      parent reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-13 10:13 Peng Ma
2019-06-13 10:13 ` [V4 2/2] dmaengine: fsl-dpaa2-qdma: Add NXP dpaa2 qDMA controller driver for Layerscape SoCs Peng Ma
2019-06-24 16:45   ` Vinod Koul
2019-09-11  2:01     ` [EXT] " Peng Ma
2019-09-24 19:34       ` Vinod Koul
2019-09-25  2:27         ` Peng Ma
2019-09-25 16:34           ` Vinod Koul
2019-09-26  6:53             ` Peng Ma
2019-06-14  1:36 ` [V4 1/2] dmaengine: fsl-dpaa2-qdma: Add the DPDMAI(Data Path DMA Interface) support Peng Ma
2019-06-24 12:25 ` Vinod Koul [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190624122509.GC2962@vkoul-mobl \
    --to=vkoul@kernel.org \
    --cc=dan.j.williams@intel.com \
    --cc=dmaengine@vger.kernel.org \
    --cc=leoyang.li@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peng.ma@nxp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

dmaengine Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dmaengine/0 dmaengine/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dmaengine dmaengine/ https://lore.kernel.org/dmaengine \
		dmaengine@vger.kernel.org
	public-inbox-index dmaengine

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.dmaengine


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git