mbox series

[v9,0/7] Add SPAcc Crypto Driver

Message ID 20240930093054.215809-1-pavitrakumarm@vayavyalabs.com
Headers show
Series Add SPAcc Crypto Driver | expand

Message

Pavitrakumar Managutte Sept. 30, 2024, 9:30 a.m. UTC
Add the driver for SPAcc(Security Protocol Accelerator), which is a             
crypto acceleration IP from Synopsys. The SPAcc supports multiple ciphers,      
hashes and AEAD algorithms with various modes. The driver currently supports    
below                                                                           
                                                                                
AEAD:                                                                           
- ccm(sm4)                                                                      
- ccm(aes)                                                                      
- gcm(sm4)                                                                      
- gcm(aes)                                                                      
- rfc7539(chacha20,poly1305)                                                    
                                                                                
cipher:                                                                         
- cbc(sm4)                                                                      
- ecb(sm4)                                                                      
- ctr(sm4)                                                                      
- xts(sm4)                                                                      
- cts(cbc(sm4))                                                                 
- cbc(aes)                                                                      
- ecb(aes)                                                                      
- xts(aes)                                                                      
- cts(cbc(aes))                                                                 
- ctr(aes)                                                                      
- chacha20                                                                      
- ecb(des)                                                                      
- cbc(des)                                                                      
- ecb(des3_ede)                                                                 
- cbc(des3_ede)                                                                 
                                                                                
hash:                                                                           
- cmac(aes)                                                                     
- xcbc(aes)                                                                     
- cmac(sm4)                                                                     
- xcbc(sm4)                                                                     
- hmac(md5)                                                                     
- md5                                                                           
- hmac(sha1)                                                                    
- sha1                                                                          
- sha224
- sha256                                                                        
- sha384                                                                        
- sha512                                                                        
- hmac(sha224)                                                                  
- hmac(sha256)                                                                  
- hmac(sha384)                                                                  
- hmac(sha512)                                                                  
- sha3-224                                                                      
- sha3-256                                                                      
- sha3-384                                                                      
- sha3-512                                                                      
- hmac(sm3)                                                                     
- sm3                                                                           
- michael_mic                                              

Pavitrakumar M (7):
  Add SPAcc Skcipher support
  Add SPAcc AUTODETECT Support
  Add SPAcc ahash support
  Add SPAcc AEAD support
  Add SPAcc Kconfig and Makefile
  Add SPAcc compilation in crypto
  dt-bindings: crypto: Document support for SPAcc

changelog:
  v8 -> v9 changes:
    - Driver instance limited to one
    - Removed platfor_get_resource() and replaced with
      devm_platform_get_and_ioremap_resource()
    - vspacc-index renamed to vspacc-id
    - Used Kernel helpers for endian conversions.
    - Added vendor prefix (snps,) for custom properties
    - Added Co-developed tags for all the commits.
    - Removed clock-names from the DT properties.
  Link to v8: https://lore.kernel.org/linux-crypto/CALxtO0kMa0LLUzZOFFuH0bkUW-814=gbFouV3um6KSMHdGT=9A@mail.gmail.com/
              https://lore.kernel.org/all/20240905150910.239832-1-pavitrakumarm@vayavyalabs.com/T/#m793bbbae55d54e51f79578c8f1a8313493920555

  v7 -> v8 changes:
    - Added DT bindings for Documentation
    - Platform driver APIs updated.

 .../bindings/crypto/snps,dwc-spacc.yaml       |   71 +
 drivers/crypto/Kconfig                        |    1 +
 drivers/crypto/Makefile                       |    1 +
 drivers/crypto/dwc-spacc/Kconfig              |   94 +
 drivers/crypto/dwc-spacc/Makefile             |   16 +
 drivers/crypto/dwc-spacc/spacc_aead.c         | 1245 ++++++++
 drivers/crypto/dwc-spacc/spacc_ahash.c        |  916 ++++++
 drivers/crypto/dwc-spacc/spacc_core.c         | 2514 +++++++++++++++++
 drivers/crypto/dwc-spacc/spacc_core.h         |  819 ++++++
 drivers/crypto/dwc-spacc/spacc_device.c       |  296 ++
 drivers/crypto/dwc-spacc/spacc_device.h       |  228 ++
 drivers/crypto/dwc-spacc/spacc_hal.c          |  359 +++
 drivers/crypto/dwc-spacc/spacc_hal.h          |  114 +
 drivers/crypto/dwc-spacc/spacc_interrupt.c    |  317 +++
 drivers/crypto/dwc-spacc/spacc_manager.c      |  658 +++++
 drivers/crypto/dwc-spacc/spacc_skcipher.c     |  720 +++++
 16 files changed, 8369 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml
 create mode 100644 drivers/crypto/dwc-spacc/Kconfig
 create mode 100644 drivers/crypto/dwc-spacc/Makefile
 create mode 100755 drivers/crypto/dwc-spacc/spacc_aead.c
 create mode 100644 drivers/crypto/dwc-spacc/spacc_ahash.c
 create mode 100644 drivers/crypto/dwc-spacc/spacc_core.c
 create mode 100644 drivers/crypto/dwc-spacc/spacc_core.h
 create mode 100644 drivers/crypto/dwc-spacc/spacc_device.c
 create mode 100644 drivers/crypto/dwc-spacc/spacc_device.h
 create mode 100644 drivers/crypto/dwc-spacc/spacc_hal.c
 create mode 100644 drivers/crypto/dwc-spacc/spacc_hal.h
 create mode 100644 drivers/crypto/dwc-spacc/spacc_interrupt.c
 create mode 100644 drivers/crypto/dwc-spacc/spacc_manager.c
 create mode 100644 drivers/crypto/dwc-spacc/spacc_skcipher.c


base-commit: ce212d2afca47acd366a2e74c76fe82c31f785ab

Comments

Krzysztof Kozlowski Sept. 30, 2024, 1:34 p.m. UTC | #1
On 30/09/2024 11:30, Pavitrakumar M wrote:
> Add SPAcc Skcipher support to Synopsys Protocol Accelerator(SPAcc) IP,
> which is a crypto accelerator engine.
> SPAcc supports ciphers, hashes and AEAD algorithms such as
> AES in different modes, SHA variants, AES-GCM, Chacha-poly1305 etc.
> 

...

> +
> +	/* reset registers */
> +	writel(0, spacc->regmap + SPACC_REG_IRQ_CTRL);
> +	writel(0, spacc->regmap + SPACC_REG_IRQ_EN);
> +	writel(0xFFFFFFFF, spacc->regmap + SPACC_REG_IRQ_STAT);
> +
> +	writel(0, spacc->regmap + SPACC_REG_SRC_PTR);
> +	writel(0, spacc->regmap + SPACC_REG_DST_PTR);
> +	writel(0, spacc->regmap + SPACC_REG_PROC_LEN);
> +	writel(0, spacc->regmap + SPACC_REG_ICV_LEN);
> +	writel(0, spacc->regmap + SPACC_REG_PRE_AAD_LEN);
> +
> +	pdu_ddt_free(&ddt);
> +	dma_free_coherent(get_ddt_device(), 256, virt, dma);
> +
> +	return CRYPTO_OK;
> +}
> +
> +/* free up the memory */
> +void spacc_fini(struct spacc_device *spacc)
> +{
> +	vfree(spacc->ctx);
> +	vfree(spacc->job);
> +}
> +
> +int spacc_init(void __iomem *baseaddr, struct spacc_device *spacc,
> +	       struct pdu_info *info)
> +{
> +	unsigned long id;
> +	char version_string[3][16]  = { "SPACC", "SPACC-PDU" };
> +	char idx_string[2][16]      = { "(Normal Port)", "(Secure Port)" };
> +	char dma_type_string[4][16] = { "Unknown", "Scattergather", "Linear",
> +					"Unknown" };
> +
> +	if (!baseaddr) {
> +		pr_debug("ERR: baseaddr is NULL\n");

You must use dev_ instead.

> +		return -1;

What is -1? No, use errno numbers.

> +	}
> +	if (!spacc) {
> +		pr_debug("ERR: spacc is NULL\n");
> +		return -1;

Same problems. How is this even possible? Don't add dead code.

> +	}
> +
> +	memset(spacc, 0, sizeof(*spacc));
> +	spin_lock_init(&spacc->lock);
> +	spin_lock_init(&spacc->ctx_lock);
> +
> +	/* assign the baseaddr*/

The missing space before */ is just sloppy. Please write good, readable
code so review will be able to focus on important things not such nitpicks.

> +	spacc->regmap = baseaddr;
> +
> +	/* version info*/
> +	spacc->config.version     = info->spacc_version.version;
> +	spacc->config.pdu_version = (info->pdu_config.major << 4) |
> +				    info->pdu_config.minor;
> +	spacc->config.project     = info->spacc_version.project;
> +	spacc->config.is_pdu      = info->spacc_version.is_pdu;
> +	spacc->config.is_qos      = info->spacc_version.qos;
> +
> +	/* misc*/
> +	spacc->config.is_partial        = info->spacc_version.partial;
> +	spacc->config.num_ctx           = info->spacc_config.num_ctx;
> +	spacc->config.ciph_page_size    = 1U <<
> +					  info->spacc_config.ciph_ctx_page_size;
> +
> +	spacc->config.hash_page_size    = 1U <<
> +					  info->spacc_config.hash_ctx_page_size;
> +
> +	spacc->config.dma_type          = info->spacc_config.dma_type;
> +	spacc->config.idx               = info->spacc_version.vspacc_id;
> +	spacc->config.cmd0_fifo_depth   = info->spacc_config.cmd0_fifo_depth;
> +	spacc->config.cmd1_fifo_depth   = info->spacc_config.cmd1_fifo_depth;
> +	spacc->config.cmd2_fifo_depth   = info->spacc_config.cmd2_fifo_depth;
> +	spacc->config.stat_fifo_depth   = info->spacc_config.stat_fifo_depth;
> +	spacc->config.fifo_cnt          = 1;
> +	spacc->config.is_ivimport       = info->spacc_version.ivimport;
> +
> +	/* ctrl register map*/
> +	if (spacc->config.version <= 0x4E)
> +		spacc->config.ctrl_map = spacc_ctrl_map[SPACC_CTRL_VER_0];
> +	else if (spacc->config.version <= 0x60)
> +		spacc->config.ctrl_map = spacc_ctrl_map[SPACC_CTRL_VER_1];
> +	else
> +		spacc->config.ctrl_map = spacc_ctrl_map[SPACC_CTRL_VER_2];
> +
> +	spacc->job_next_swid   = 0;
> +	spacc->wdcnt           = 0;
> +	spacc->config.wd_timer = SPACC_WD_TIMER_INIT;
> +
> +	/* version 4.10 uses IRQ,

Please use Linux coding style comments for given subsystem. See Coding
style.

> +	 * above uses WD and we don't support below 4.00
> +	 */
> +	if (spacc->config.version < 0x40) {
> +		pr_debug("ERR: Unsupported SPAcc version\n");
> +		return -EIO;
> +	} else if (spacc->config.version < 0x4B) {
> +		spacc->op_mode = SPACC_OP_MODE_IRQ;
> +	} else {
> +		spacc->op_mode = SPACC_OP_MODE_WD;
> +	}
> +
> +	/* set threshold and enable irq
> +	 * on 4.11 and newer cores we can derive this
> +	 * from the HW reported depths.
> +	 */
> +	if (spacc->config.stat_fifo_depth == 1)
> +		spacc->config.ideal_stat_level = 1;
> +	else if (spacc->config.stat_fifo_depth <= 4)
> +		spacc->config.ideal_stat_level =
> +					spacc->config.stat_fifo_depth - 1;
> +	else if (spacc->config.stat_fifo_depth <= 8)
> +		spacc->config.ideal_stat_level =
> +					spacc->config.stat_fifo_depth - 2;
> +	else
> +		spacc->config.ideal_stat_level =
> +					spacc->config.stat_fifo_depth - 4;
> +
> +	/* determine max PROClen value */
> +	writel(0xFFFFFFFF, spacc->regmap + SPACC_REG_PROC_LEN);
> +	spacc->config.max_msg_size = readl(spacc->regmap + SPACC_REG_PROC_LEN);
> +
> +	/* read config info*/
> +	if (spacc->config.is_pdu) {
> +		pr_debug("PDU:\n");
> +		pr_debug("   MAJOR      : %u\n", info->pdu_config.major);
> +		pr_debug("   MINOR      : %u\n", info->pdu_config.minor);


Why?

> +	}
> +
> +	id = readl(spacc->regmap + SPACC_REG_ID);
> +	pr_debug("SPACC ID: (%08lx)\n", (unsigned long)id);
> +	pr_debug("   MAJOR      : %x\n", info->spacc_version.major);
> +	pr_debug("   MINOR      : %x\n", info->spacc_version.minor);
> +	pr_debug("   QOS        : %x\n", info->spacc_version.qos);
> +	pr_debug("   IVIMPORT   : %x\n", spacc->config.is_ivimport);

What are you going to debug here?

> +
> +	if (spacc->config.version >= 0x48)
> +		pr_debug("   TYPE       : %lx (%s)\n", SPACC_ID_TYPE(id),
> +			version_string[SPACC_ID_TYPE(id) & 3]);
> +
> +	pr_debug("   AUX        : %x\n", info->spacc_version.qos);
> +	pr_debug("   IDX        : %lx %s\n", SPACC_ID_VIDX(id),
> +			spacc->config.is_secure ?
> +			(idx_string[spacc->config.is_secure_port & 1]) : "");
> +	pr_debug("   PARTIAL    : %x\n", info->spacc_version.partial);
> +	pr_debug("   PROJECT    : %x\n", info->spacc_version.project);
> +
> +	if (spacc->config.version >= 0x48)
> +		id = readl(spacc->regmap + SPACC_REG_CONFIG);
> +	else
> +		id = 0xFFFFFFFF;
> +
> +	pr_debug("SPACC CFG: (%08lx)\n", id);
> +	pr_debug("   CTX CNT    : %u\n", info->spacc_config.num_ctx);
> +	pr_debug("   VSPACC CNT : %u\n", info->spacc_config.num_vspacc);
> +	pr_debug("   CIPH SZ    : %-3lu bytes\n", 1UL <<
> +				  info->spacc_config.ciph_ctx_page_size);
> +	pr_debug("   HASH SZ    : %-3lu bytes\n", 1UL <<
> +				  info->spacc_config.hash_ctx_page_size);
> +	pr_debug("   DMA TYPE   : %u (%s)\n", info->spacc_config.dma_type,
> +			dma_type_string[info->spacc_config.dma_type & 3]);
> +	pr_debug("   MAX PROCLEN: %lu bytes\n", (unsigned long)
> +				  spacc->config.max_msg_size);
> +	pr_debug("   FIFO CONFIG :\n");
> +	pr_debug("      CMD0 DEPTH: %d\n", spacc->config.cmd0_fifo_depth);

You could not flood dmesg with more debugging, could you?


> +
> +	if (spacc->config.is_qos) {
> +		pr_debug("      CMD1 DEPTH: %d\n",
> +				spacc->config.cmd1_fifo_depth);
> +		pr_debug("      CMD2 DEPTH: %d\n",
> +				spacc->config.cmd2_fifo_depth);
> +	}
> +	pr_debug("      STAT DEPTH: %d\n", spacc->config.stat_fifo_depth);
> +
> +	if (spacc->config.dma_type == SPACC_DMA_DDT) {
> +		writel(0x1234567F, baseaddr + SPACC_REG_DST_PTR);
> +		writel(0xDEADBEEF, baseaddr + SPACC_REG_SRC_PTR);
> +
> +		if (((readl(baseaddr + SPACC_REG_DST_PTR)) !=
> +					(0x1234567F & SPACC_DST_PTR_PTR)) ||
> +		    ((readl(baseaddr + SPACC_REG_SRC_PTR)) !=
> +		     (0xDEADBEEF & SPACC_SRC_PTR_PTR))) {
> +			pr_debug("ERR: Failed to set pointers\n");
> +			goto ERR;
> +		}
> +	}
> +
> +	/* zero the IRQ CTRL/EN register
> +	 * (to make sure we're in a sane state)
> +	 */
> +	writel(0, spacc->regmap + SPACC_REG_IRQ_CTRL);
> +	writel(0, spacc->regmap + SPACC_REG_IRQ_EN);
> +	writel(0xFFFFFFFF, spacc->regmap + SPACC_REG_IRQ_STAT);
> +
> +	/* init cache*/
> +	memset(&spacc->cache, 0, sizeof(spacc->cache));
> +	writel(0, spacc->regmap + SPACC_REG_SRC_PTR);
> +	writel(0, spacc->regmap + SPACC_REG_DST_PTR);
> +	writel(0, spacc->regmap + SPACC_REG_PROC_LEN);
> +	writel(0, spacc->regmap + SPACC_REG_ICV_LEN);
> +	writel(0, spacc->regmap + SPACC_REG_ICV_OFFSET);
> +	writel(0, spacc->regmap + SPACC_REG_PRE_AAD_LEN);
> +	writel(0, spacc->regmap + SPACC_REG_POST_AAD_LEN);
> +	writel(0, spacc->regmap + SPACC_REG_IV_OFFSET);
> +	writel(0, spacc->regmap + SPACC_REG_OFFSET);
> +	writel(0, spacc->regmap + SPACC_REG_AUX_INFO);
> +
> +	spacc->ctx = vmalloc(sizeof(struct spacc_ctx) * spacc->config.num_ctx);
> +	if (!spacc->ctx)
> +		goto ERR;
> +
> +	spacc->job = vmalloc(sizeof(struct spacc_job) * SPACC_MAX_JOBS);
> +	if (!spacc->job)
> +		goto ERR;
> +
> +	/* initialize job_idx and lookup table */
> +	spacc_job_init_all(spacc);
> +
> +	/* initialize contexts */
> +	spacc_ctx_init_all(spacc);
> +
> +	/* autodetect and set string size setting*/
> +	if (spacc->config.version == 0x61 || spacc->config.version >= 0x65)
> +		spacc_xof_stringsize_autodetect(spacc);
> +
> +	return CRYPTO_OK;

??? Please do not redefine numbers.

Missing blank line

> +ERR:


> +	spacc_fini(spacc);
> +	pr_debug("ERR: Crypto Failed\n");

Drop



> +
> +	return -EIO;
> +}
> +
> +/* callback function to initialize tasklet running */
> +void spacc_pop_jobs(unsigned long data)
> +{
> +	int num = 0;
> +	struct spacc_priv *priv =  (struct spacc_priv *)data;
> +	struct spacc_device *spacc = &priv->spacc;
> +
> +	/* decrement the WD CNT here since
> +	 * now we're actually going to respond
> +	 * to the IRQ completely
> +	 */
> +	if (spacc->wdcnt)
> +		--(spacc->wdcnt);
> +
> +	spacc_pop_packets(spacc, &num);
> +}
> +
> +int spacc_remove(struct platform_device *pdev)
> +{
> +	struct spacc_device *spacc;
> +	struct spacc_priv *priv = platform_get_drvdata(pdev);
> +
> +	/* free test vector memory*/
> +	spacc = &priv->spacc;
> +	spacc_fini(spacc);
> +
> +	tasklet_kill(&priv->pop_jobs);
> +
> +	/* devm functions do proper cleanup */
> +	pdu_mem_deinit(&pdev->dev);
> +	dev_dbg(&pdev->dev, "removed!\n");
> +
> +	return 0;
> +}
> +
> +int spacc_set_key_exp(struct spacc_device *spacc, int job_idx)
> +{
> +	struct spacc_ctx *ctx = NULL;
> +	struct spacc_job *job = NULL;
> +
> +	if (job_idx < 0 || job_idx >= SPACC_MAX_JOBS) {
> +		pr_debug("ERR: Invalid Job id specified (out of range)\n");
> +		return -ENXIO;
> +	}
> +
> +	job = &spacc->job[job_idx];
> +	ctx = context_lookup_by_job(spacc, job_idx);
> +
> +	if (!ctx) {
> +		pr_debug("ERR: Failed to find ctx id\n");
> +		return -EIO;
> +	}
> +
> +	job->ctrl |= SPACC_CTRL_MASK(SPACC_CTRL_KEY_EXP);
> +
> +	return CRYPTO_OK;

This code has terrible quality and does not fit basic Linux coding
style. Why v9 has trivial issues like this? I don't understand.

Please cleanup your code thoroughly from all such weird
all-platform-Windows-downstream junk.

This code is just not ready for submission.

> +}


> +
> +int spacc_compute_xcbc_key(struct spacc_device *spacc, int mode_id,
> +			   int job_idx, const unsigned char *key,
> +			   int keylen, unsigned char *xcbc_out)
> +{
> +	unsigned char *buf;
> +	dma_addr_t bufphys;
> +	struct pdu_ddt ddt;
> +	unsigned char iv[16];
> +	int err, i, handle, usecbc, ctx_idx;
> +
> +	if (job_idx >= 0 && job_idx < SPACC_MAX_JOBS)
> +		ctx_idx = spacc->job[job_idx].ctx_idx;
> +	else
> +		ctx_idx = -1;
> +
> +	if (mode_id == CRYPTO_MODE_MAC_XCBC) {
> +		/* figure out if we can schedule the key  */
> +		if (spacc_isenabled(spacc, CRYPTO_MODE_AES_ECB, 16))
> +			usecbc = 0;
> +		else if (spacc_isenabled(spacc, CRYPTO_MODE_AES_CBC, 16))
> +			usecbc = 1;
> +		else
> +			return -1;
> +	} else if (mode_id == CRYPTO_MODE_MAC_SM4_XCBC) {
> +		/* figure out if we can schedule the key  */
> +		if (spacc_isenabled(spacc, CRYPTO_MODE_SM4_ECB, 16))
> +			usecbc = 0;
> +		else if (spacc_isenabled(spacc, CRYPTO_MODE_SM4_CBC, 16))
> +			usecbc = 1;
> +		else
> +			return -1;
> +	} else {
> +		return -1;

What is -1?

> +	}
> +
> +	memset(iv, 0, sizeof(iv));
> +	memset(&ddt, 0, sizeof(ddt));
> +
> +	buf = dma_alloc_coherent(get_ddt_device(), 64, &bufphys, GFP_KERNEL);
> +	if (!buf)
> +		return -EINVAL;

and here -EINVAL? So what was the meaning of -1?

This is terrible code.



> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_device.c
> @@ -0,0 +1,296 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/platform_device.h>
> +#include "spacc_device.h"
> +
> +static struct platform_device *spacc_pdev;

This is too much. NAK.

Code has terrible quality but what is worse: you ignored several
comments, which means code does not improve. You send the same code over
and over.

Sending same code with same junk is not acceptable. You waste reviewer's
time.


Best regards,
Krzysztof
Krzysztof Kozlowski Oct. 1, 2024, 6:15 a.m. UTC | #2
On 01/10/2024 04:57, Pavitrakumar Managutte wrote:
> Hi Krzysztof,
>    Thanks for the quick review and I do really appreciate everybody's time here.
>    If something got missed, it's just because of the exhaustive
> hardware and the SystemC Model testing.
>    We make minimal/incremental changes and run things in debug mode
> which takes a lot of time,
>    since this is a large code base. Never ignored anything till date.
>    Every single comment has been and will be addressed. We will work
> on code quality as per your inputs.

No, it was not addressed.

Do you want proofs? Look:

1. Drop contains. The list of compatible strings and order must be defined.
Not addressed at all.

2. crypto@40000000
Ignored completely

3.  Generally drivers aren't limited to some number of instances (except...
Also ignored completely

and more..


> 
> Warm regards,
> PK
> 
> 
> On Mon, Sep 30, 2024 at 6:50 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 30/09/2024 11:30, Pavitrakumar M wrote:
>>> Add DT bindings related to the SPAcc driver for Documentation.
>>> DWC Synopsys Security Protocol Accelerator(SPAcc) Hardware Crypto
>>> Engine is a crypto IP designed by Synopsys.
>>>
>>> Co-developed-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com>
>>> Signed-off-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com>
>>> Co-developed-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com>
>>> Signed-off-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com>
>>> Acked-by: Ruud Derwig <Ruud.Derwig@synopsys.com>
>>> ---
>>>  .../bindings/crypto/snps,dwc-spacc.yaml       | 71 +++++++++++++++++++
>>>  1 file changed, 71 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml
>>
>> Bindings come before users, so please re-order your patches.
> 
> PK: Will re-order
>>
>>
>>>
>>> diff --git a/Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml b/Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml
>>> new file mode 100644
>>> index 000000000000..6b94d0aa7280
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml
>>> @@ -0,0 +1,71 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/crypto/snps,dwc-spacc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Synopsys DesignWare Security Protocol Accelerator(SPAcc) Crypto Engine
>>> +
>>> +maintainers:
>>> +  - Ruud Derwig <Ruud.Derwig@synopsys.com>
>>> +
>>> +description:
>>> +  DWC Synopsys Security Protocol Accelerator(SPAcc) Hardware Crypto Engine is
>>> +  a crypto IP designed by Synopsys, that can accelerate cryptographic
>>> +  operations.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    contains:
>>
>> Nope, you cannot have contains. From where did you get it? Please use
>> existing, recent bindings as starting point or just use exampl-eschema.
> 
> PK: Will fix that.
>>
>>
>> Eh, you already got this comment and just ignored it.
> 
> PK: It got missed, never ignored. Too valuable to ignore comments from demigods.
>>
>>
>> You ignored all other comments as well. This is quite disappointing to
>> ask us to do the same review over and over.
> 
> PK: That never was the intent nor the impression I wanted to make.
> Appreciate everybody's time here.

Then why do you ignore review?

Best regards,
Krzysztof
Pavitrakumar Managutte Oct. 1, 2024, 7:23 a.m. UTC | #3
Hi Krzysztof,
  I fully agree, some things got missed out; they were supposed to be addressed
  in the v9 patch. Not contesting that. My apologies.

  We are doing a code cleanup based on your inputs and previous comments.
  I will address everything.

Warm regards,
PK



On Tue, Oct 1, 2024 at 12:03 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Mon, Sep 30, 2024 at 03:00:54PM +0530, Pavitrakumar M wrote:
> > Add DT bindings related to the SPAcc driver for Documentation.
> > DWC Synopsys Security Protocol Accelerator(SPAcc) Hardware Crypto
> > Engine is a crypto IP designed by Synopsys.
> >
> > Co-developed-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com>
> > Signed-off-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com>
> > Co-developed-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com>
> > Signed-off-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com>
> > Acked-by: Ruud Derwig <Ruud.Derwig@synopsys.com>
>
> Please run scripts/checkpatch.pl and fix reported warnings. Then please
> run  and (probably) fix more warnings.
> Some warnings can be ignored, especially from --strict run, but the code
> here looks like it needs a fix. Feel free to get in touch if the warning
> is not clear.
>
> Best regards,
> Krzysztof
>