RE: [EXT] Re: [V3 1/2] dmaengine: fsl-dpaa2-qdma: Add the DPDMAI(Data Path DMA Interface) support

[Peng Ma]

Hi Vinod

Sorry to replay so late.
The dpaa2 qdma driver is based on FSL_MC_BUS and FSL_MC_DPIO, so It will used those two drivers
Functions or structs. This patch provides some necessary functions and structs for qdma driver(next patch: dpaa2-qdma.c)
The dpaa2 driver is not only to write some register on driver side but also need enable FSL_MC_BUS and FSL_MC_DPIO to
call some functions provided by them. Such as: mc_encode_cmd_header, mc_send_command etc. I will clear up this driver
again. And send V4 to review, thanks for your review.

Best Regards,
Peng
>-----Original Message-----
>From: Vinod Koul <vkoul@xxxxxxxxxx>
>Sent: 2019年4月26日 20:46
>To: Peng Ma <peng.ma@xxxxxxx>
>Cc: dan.j.williams@xxxxxxxxx; Leo Li <leoyang.li@xxxxxxx>;
>linux-kernel@xxxxxxxxxxxxxxx; dmaengine@xxxxxxxxxxxxxxx
>Subject: [EXT] Re: [V3 1/2] dmaengine: fsl-dpaa2-qdma: Add the DPDMAI(Data
>Path DMA Interface) support
>
>Caution: EXT Email
>
>On 09-04-19, 15:22, Peng Ma wrote:
>
>Subject missed PATCH tag!
>
>> The MC exports the DPDMAI object as an interface to operate the DPAA2
>> QDMA
>
>whats MC, DPDMAI, DPAA2
>
[Peng Ma] MC: Management Complex
DPDMAI: Data Path DMA Interface
DPAA2: Data Path Acceleration Architecture, Second Generation
I will explain them on next version.
>> Engine. The DPDMAI enables sending frame-based requests to QDMA and
>> receiving back confirmation response on transaction completion,
>> utilizing the DPAA2 QBMan infrastructure. DPDMAI object provides up to
>> two priorities for processing QDMA requests.
>
>> +int dpdmai_open(struct fsl_mc_io *mc_io,
>> +             u32 cmd_flags,
>> +             int dpdmai_id,
>> +             u16 *token)
>
>could be written as:
>
>int dpdmai_open(struct fsl_mc_io *mc_io, u32 cmd_flags,
>                int dpdmai_id, u16 *token)
>
>Looks neater, right? It would be to reread coding guidelines and run checkpatch
>with --strict on this
>
[Peng Ma] Ok, got it.
>
>> +{
>> +     struct fsl_mc_command cmd = { 0 };
>
>where is this defined?
[Peng Ma] include/linux/fsl/mc.h
>
>> +     struct dpdmai_cmd_open *cmd_params;
>> +     int err;
>> +
>> +     /* prepare command */
>> +     cmd.header = mc_encode_cmd_header(DPDMAI_CMDID_OPEN,
>> +                                       cmd_flags,
>> +                                       0);
>> +
>> +     cmd_params = (struct dpdmai_cmd_open *)cmd.params;
>
>I dont like casts, can you explain
[Peng Ma] If I didn't use the casts, There are two case:
1: struct fsl_mc_command cmd_org = { 0 };
  Struct fsl_mc_command *cmd = &cmd_org;
  In this case, we will define one more temporary variable in the stack.
2: Struct fsl_mc_command *cmd = kmalloc();
  In this case, we will alloc some memory, It may be produce memory
Fragmentation. Those functions will be called on driver init It is not necessary 
use this case to those function.
>
>> +     cmd_params->dpdmai_id = cpu_to_le32(dpdmai_id);
>> +
>> +     /* 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);
>> +     return 0;
>> +}
>
>who will call this API?
>
[Peng Ma] This file is not a driver, some of functions define in this file, the dma driver(dpaa2-qdma.c) will call some of those API and I will
delete unuseless API in the future.
>> +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);
>
>why waste a line, last arg can be in previous line!
>
[Peng Ma] sorry, I will remove it.
>> +int dpdmai_get_tx_queue(struct fsl_mc_io *mc_io,
>> +                     u32 cmd_flags,
>> +                     u16 token,
>> +                     u8 priority,
>> +                     struct dpdmai_tx_queue_attr *attr) {
>> +     struct fsl_mc_command cmd = { 0 };
>> +     struct dpdmai_cmd_queue *cmd_params;
>> +     struct dpdmai_rsp_get_tx_queue *rsp_params;
>> +     int err;
>> +
>> +     /* prepare command */
>> +     cmd.header =
>mc_encode_cmd_header(DPDMAI_CMDID_GET_TX_QUEUE,
>> +                                       cmd_flags,
>> +                                       token);
>> +
>> +     cmd_params = (struct dpdmai_cmd_queue *)cmd.params;
>> +     cmd_params->queue = priority;
>> +
>> +     /* send command to mc*/
>> +     err = mc_send_command(mc_io, &cmd);
>> +     if (err)
>> +             return err;
>> +
>> +     /* retrieve response parameters */
>> +
>> +     rsp_params = (struct dpdmai_rsp_get_tx_queue *)cmd.params;
>> +     attr->fqid = le32_to_cpu(rsp_params->fqid);
>> +
>> +     return 0;
>> +}
>
>Okay this file does not use any of dmaengine apis, is this a dmaengine driver?
[Peng Ma] please the previous comment.
>
>> diff --git a/drivers/dma/fsl-dpaa2-qdma/dpdmai.h
>> b/drivers/dma/fsl-dpaa2-qdma/dpdmai.h
>> new file mode 100644
>> index 0000000..c8a7b7f
>> --- /dev/null
>> +++ b/drivers/dma/fsl-dpaa2-qdma/dpdmai.h
>> @@ -0,0 +1,524 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright 2014-2018 NXP */
>> +
>> +/*
>> + * 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 the above-listed copyright holders nor the
>> + * names of any 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 THE COPYRIGHT HOLDERS AND
>CONTRIBUTORS "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 THE COPYRIGHT HOLDERS OR
>> +CONTRIBUTORS 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.
>> + */
>
>we have SiPDX tag, why do you need ths here!
>
[Peng Ma] It is my mistake. I will fixe it.
>--
>~Vinod