Message-ID: <582F4EA7.9030303@caviumnetworks.com>
Date: Fri, 18 Nov 2016 10:55:35 -0800
From: David Daney <ddaney@caviumnetworks.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7
MIME-Version: 1.0
To: <gcherianv@gmail.com>
CC: <linux-kernel@vger.kernel.org>, <linux-crypto@vger.kernel.org>,
        <davem@davemloft.net>, <herbert@gondor.apana.org.au>,
        George Cherian <george.cherian@cavium.com>
Subject: Re: [PATCH 1/3] drivers: crypto: Add Support for Octeon-tx CPT Engine
References: <1479481209-11475-1-git-send-email-gcherianv@gmail.com> <1479481209-11475-2-git-send-email-gcherianv@gmail.com>
In-Reply-To: <1479481209-11475-2-git-send-email-gcherianv@gmail.com>
Content-Type: text/plain; charset="ISO-8859-1"; format=flowed
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Nov 2016 18:55:38.2260 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR07MB2134
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 6835
Lines: 236

On 11/18/2016 07:00 AM, gcherianv@gmail.com wrote:
> From: George Cherian <george.cherian@cavium.com>
>
> Enable the Physical Function diver for the Cavium Crypto Engine (CPT)
> found in Octeon-tx series of SoC's. CPT is the Cryptographic Acceleration
> Unit. CPT includes microcoded GigaCypher symmetric engines (SEs) and
> asymmetric engines (AEs).
>
> Signed-off-by: George Cherian <george.cherian@cavium.com>


How was this tested?

> ---
>   drivers/crypto/cavium/cpt/Kconfig        |  22 +
>   drivers/crypto/cavium/cpt/Makefile       |   2 +
>   drivers/crypto/cavium/cpt/cpt.h          |  90 +++
>   drivers/crypto/cavium/cpt/cpt_common.h   | 377 +++++++++++++
>   drivers/crypto/cavium/cpt/cpt_hw_types.h | 940 +++++++++++++++++++++++++++++++
>   drivers/crypto/cavium/cpt/cpt_main.c     | 891 +++++++++++++++++++++++++++++
>   drivers/crypto/cavium/cpt/cpt_pf_mbox.c  | 174 ++++++
>   7 files changed, 2496 insertions(+)
>   create mode 100644 drivers/crypto/cavium/cpt/Kconfig
>   create mode 100644 drivers/crypto/cavium/cpt/Makefile
>   create mode 100644 drivers/crypto/cavium/cpt/cpt.h
>   create mode 100644 drivers/crypto/cavium/cpt/cpt_common.h
>   create mode 100644 drivers/crypto/cavium/cpt/cpt_hw_types.h
>   create mode 100644 drivers/crypto/cavium/cpt/cpt_main.c
>   create mode 100644 drivers/crypto/cavium/cpt/cpt_pf_mbox.c
>
> diff --git a/drivers/crypto/cavium/cpt/Kconfig b/drivers/crypto/cavium/cpt/Kconfig
> new file mode 100644
> index 0000000..8fe3f44
> --- /dev/null
> +++ b/drivers/crypto/cavium/cpt/Kconfig
> @@ -0,0 +1,22 @@
> +#
> +# Cavium crypto device configuration
> +#
> +
> +config CRYPTO_DEV_CPT
> +	tristate
> +	select HW_RANDOM_OCTEON

This makes no sense.  HW_RANDOM_OCTEON is for a mips64 based SOC and 
isn't present on devices that have this crypto block.  Why select this?


> +	select CRYPTO_AES
> +	select CRYPTO_DES
> +	select CRYPTO_BLKCIPHER
> +	select FW_LOADER
> +
> +config OCTEONTX_CPT_PF
> +	tristate "Octeon-tx CPT Physical function driver"
> +	depends on ARCH_THUNDER
> +	select CRYPTO_DEV_CPT
> +	help
> +	  Support for Cavium CPT block found in octeon-tx series of
> +	  processors.
> +
> +	  To compile this as a module, choose M here: the module will be
> +	  called cptpf.
> diff --git a/drivers/crypto/cavium/cpt/Makefile b/drivers/crypto/cavium/cpt/Makefile
> new file mode 100644
> index 0000000..bf758e2
> --- /dev/null
> +++ b/drivers/crypto/cavium/cpt/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_OCTEONTX_CPT_PF) += cptpf.o
> +cptpf-objs := cpt_main.o cpt_pf_mbox.o
> diff --git a/drivers/crypto/cavium/cpt/cpt.h b/drivers/crypto/cavium/cpt/cpt.h
> new file mode 100644
> index 0000000..63d12da
> --- /dev/null
> +++ b/drivers/crypto/cavium/cpt/cpt.h
> @@ -0,0 +1,90 @@
> +/*
> + * Copyright (C) 2016 Cavium, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of version 2 of the GNU General Public License
> + * as published by the Free Software Foundation.
> + */
> +
> +#ifndef __CPT_H
> +#define __CPT_H
> +
> +#include "cpt_common.h"
> +
> +#define BASE_PROC_DIR	"cavium"
> +
> +#define PF  0
> +#define VF  1
> +
> +struct cpt_device;
> +
> +struct microcode {
> +	uint8_t  is_mc_valid;

s/uint8_t/u8/  ??

That could be done everywhere.

[...]
> diff --git a/drivers/crypto/cavium/cpt/cpt_common.h b/drivers/crypto/cavium/cpt/cpt_common.h
> new file mode 100644
> index 0000000..351ed4a
> --- /dev/null
> +++ b/drivers/crypto/cavium/cpt/cpt_common.h
> @@ -0,0 +1,377 @@
> +/*
> + * Copyright (C) 2016 Cavium, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of version 2 of the GNU General Public License
> + * as published by the Free Software Foundation.
> + */
> +
> +#ifndef __CPT_COMMON_H
> +#define __CPT_COMMON_H
> +
> +#include <asm/byteorder.h>
> +#include <linux/uaccess.h>
> +#include <linux/types.h>
> +#include <linux/spinlock.h>
> +#include <linux/pci.h>
> +#include <linux/cpumask.h>
> +#include <linux/string.h>
> +#include <linux/pci_regs.h>
> +#include <linux/delay.h>
> +#include <linux/printk.h>
> +#include <linux/sched.h>
> +#include <linux/completion.h>
> +#include <asm/arch_timer.h>
> +#include <linux/types.h>
> +
> +#include "cpt_hw_types.h"
> +
> +/* configuration space offsets */
> +#ifndef PCI_VENDOR_ID
> +#define PCI_VENDOR_ID 0x00 /* 16 bits */
> +#endif
> +#ifndef PCI_DEVICE_ID
> +#define PCI_DEVICE_ID 0x02 /* 16 bits */
> +#endif
> +#ifndef PCI_REVISION_ID
> +#define PCI_REVISION_ID 0x08 /* Revision ID */
> +#endif
> +#ifndef PCI_CAPABILITY_LIST
> +#define PCI_CAPABILITY_LIST 0x34 /* first capability list entry */
> +#endif
> +

Standard PCI core functions give you access to all that information, use 
pdev->device, pdev->revision, etc. instead of reinventing the wheel here 
with all these #defines.


> +/* Device ID */
> +#define PCI_VENDOR_ID_CAVIUM 0x177d

This is defined in pci_ids.h, use value from there instead of placing a 
duplicate definition here.

> +#define CPT_81XX_PCI_PF_DEVICE_ID 0xa040
> +#define CPT_81XX_PCI_VF_DEVICE_ID 0xa041
> +
> +#define PASS_1_0 0x0
> +
> +/* CPT Models ((Device ID<<16)|Revision ID) */
> +/* CPT models */
> +#define CPT_81XX_PASS1_0 ((CPT_81XX_PCI_PF_DEVICE_ID << 8) | PASS_1_0)
> +#define CPTVF_81XX_PASS1_0 ((CPT_81XX_PCI_VF_DEVICE_ID << 8) | PASS_1_0)
> +
> +#define PF 0
> +#define VF 1
> +
> +#define DEFAULT_DEVICE_QUEUES CPT_NUM_QS_PER_VF
> +
> +#define SUCCESS	(0)
> +#define FAIL	(1)
> +
> +#ifndef ROUNDUP4
> +#define ROUNDUP4(val) (((val) + 3) & 0xfffffffc)
> +#endif
> +
> +#ifndef ROUNDUP8
> +#define ROUNDUP8(val) (((val) + 7) & 0xfffffff8)
> +#endif
> +
> +#ifndef ROUNDUP16
> +#define ROUNDUP16(val) (((val) + 15) & 0xfffffff0)
> +#endif
> +

kernel.h has round_up(), use that instead of defining all these.

> +#define ERR_ADDR_LEN 8
> +

What is that for?  It looks unused.

[...]
> +/*###### PCIE EP-Mode Configuration Registers #########*/
> +#define PCIEEP0_CFG000 (0x0)
> +#define PCIEEP0_CFG002 (0x8)
> +#define PCIEEP0_CFG011 (0x2C)
> +#define PCIEEP0_CFG020 (0x50)
> +#define PCIEEP0_CFG025 (0x64)
> +#define PCIEEP0_CFG030 (0x78)
> +#define PCIEEP0_CFG044 (0xB0)
> +#define PCIEEP0_CFG045 (0xB4)
> +#define PCIEEP0_CFG082 (0x148)
> +#define PCIEEP0_CFG095 (0x17C)
> +#define PCIEEP0_CFG096 (0x180)
> +#define PCIEEP0_CFG097 (0x184)
> +#define PCIEEP0_CFG103 (0x19C)
> +#define PCIEEP0_CFG460 (0x730)
> +#define PCIEEP0_CFG461 (0x734)
> +#define PCIEEP0_CFG462 (0x738)
> +
> +/*#######  PCIe EP-Mode SR-IOV Configuration Registers  #####*/
> +#define PCIEEPVF0_CFG000 (0x0)
> +#define PCIEEPVF0_CFG002 (0x8)
> +#define PCIEEPVF0_CFG011 (0x2C)
> +#define PCIEEPVF0_CFG030 (0x78)
> +#define PCIEEPVF0_CFG044 (0xB0)
> +

Where are all those defines used?  What are they for?


That's all I can look at for now.

David.