Re: [PATCH V4 2/2] Documentation: crypto: Add DT binding info for the img hw hash accelerator
Hi James, On Wed, Mar 11, 2015 at 7:06 PM, James Hartley james.hart...@imgtec.com wrote: This adds the binding documentation for the Imagination Technologies hash accelerator that provides hardware acceleration for SHA1/SHA224/SHA256/MD5 hashes. This hardware will be present in the upcoming pistachio SoC. Signed-off-by: James Hartley james.hart...@imgtec.com Reviewed-by: Andrew Bresticker abres...@chromium.org -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/2] crypto: Add Imagination Technologies hw hash accelerator
Hi James,
On Wed, Mar 11, 2015 at 7:06 PM, James Hartley james.hart...@imgtec.com wrote:
This adds support for the Imagination Technologies hash accelerator which
provides hardware acceleration for SHA1 SHA224 SHA256 and MD5 hashes.
Signed-off-by: James Hartley james.hart...@imgtec.com
One comment below, otherwise this looks fine to me.
--- /dev/null
+++ b/drivers/crypto/img-hash.c
+static int img_hash_hw_init(struct img_hash_dev *hdev)
+{
+ unsigned long long nbits;
+ u32 u, l;
+ int ret;
+
+ ret = clk_prepare_enable(hdev-hash_clk);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(hdev-sys_clk);
+ if (ret) {
+ clk_disable_unprepare(hdev-hash_clk);
+ return ret;
+ }
I think you'll still end up with inflated prepare/enable counts for
these clocks since this function may get called multiple times and the
only clk_disable_unprepare() calls are in the remove() path. Perhaps
it's best to just enable the clocks in probe() until runtime PM
support is added?
Thanks,
Andrew
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 1/2] crypto: Add Imagination Technologies hw hash accelerator
Hi James, On Thu, Mar 12, 2015 at 4:17 PM, James Hartley james.hart...@imgtec.com wrote: This adds support for the Imagination Technologies hash accelerator which provides hardware acceleration for SHA1 SHA224 SHA256 and MD5 hashes. Signed-off-by: James Hartley james.hart...@imgtec.com Looks good to me. Reviewed-by: Andrew Bresticker abres...@chromium.org -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 1/2] crypto: Add Imagination Technologies hw hash accelerator
Hi James,
+static irqreturn_t img_irq_handler(int irq, void *dev_id) {
+ struct img_hash_dev *hdev = dev_id;
+ u32 reg;
+
+ reg = img_hash_read(hdev, CR_INTSTAT);
+ img_hash_write(hdev, CR_INTCLEAR, reg);
+
+ if (reg CR_INT_NEW_RESULTS_SET) {
+ dev_dbg(hdev-dev, IRQ CR_INT_NEW_RESULTS_SET\n);
+ if (DRIVER_FLAGS_BUSY hdev-flags) {
+ hdev-flags |= DRIVER_FLAGS_OUTPUT_READY;
+ if (!(DRIVER_FLAGS_CPU hdev-flags))
+ hdev-flags |= DRIVER_FLAGS_DMA_READY;
+ tasklet_schedule(hdev-done_task);
Since this done_task only gets scheduled from here, why not make this a
threaded IRQ handler and just do the work here instead of separating it
between a hard IRQ handler and a tasklet?
What is the advantage of doing that? i.e is this simple case worth setting
up an extra thread?
I believe threaded IRQ handlers are now preferred to using a hard IRQ
handler + tasklet when possible, partly because people tend to screw
up tasklet usage.
+ err = PTR_ERR(hdev-io_base);
+ goto hash_io_err;
+ }
+
+ /* Write port (DMA or CPU) */
+ hash_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!hash_res) {
+ dev_err(dev, no write port resource info\n);
+ err = -ENODEV;
+ goto res_err;
+ }
+ hdev-bus_addr = hash_res-start;
Maybe set this after devm_ioremap_resource() succeeds and avoid the extra
error check?
Not quite following you here - which check are you saying can be removed?
You can remove the if (hash_res) check if you set hdev-bus_addr after
devm_ioremap_resource().
Thanks,
Andrew
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 1/2] crypto: Add Imagination Technologies hw hash accelerator
Hi James,
On Thu, Mar 5, 2015 at 7:01 PM, James Hartley james.hart...@imgtec.com wrote:
This adds support for the Imagination Technologies hash
accelerator which provides hardware acceleration for
SHA1 SHA224 SHA256 and MD5 hashes.
Signed-off-by: James Hartley james.hart...@imgtec.com
Some general comments below, I'll leave review of the crypto-specific
stuff to Herbert.
diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
index 3924f93..fb84be7 100644
--- a/drivers/crypto/Makefile
+++ b/drivers/crypto/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_CRYPTO_DEV_CCP) += ccp/
obj-$(CONFIG_CRYPTO_DEV_FSL_CAAM) += caam/
obj-$(CONFIG_CRYPTO_DEV_GEODE) += geode-aes.o
obj-$(CONFIG_CRYPTO_DEV_HIFN_795X) += hifn_795x.o
+obj-$(CONFIG_CRYPTO_DEV_IMGTEC_HASH) += img-hash.o
obj-$(CONFIG_CRYPTO_DEV_IXP4XX) += ixp4xx_crypto.o
obj-$(CONFIG_CRYPTO_DEV_MV_CESA) += mv_cesa.o
obj-$(CONFIG_CRYPTO_DEV_MXS_DCP) += mxs-dcp.o
@@ -25,3 +26,4 @@ obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o
obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/
obj-$(CONFIG_CRYPTO_DEV_QAT) += qat/
obj-$(CONFIG_CRYPTO_DEV_QCE) += qce/
+obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
Unrelated change - perhaps a bad merge conflict resolution?
diff --git a/drivers/crypto/img-hash.c b/drivers/crypto/img-hash.c
new file mode 100644
index 000..94a3a6f
--- /dev/null
+++ b/drivers/crypto/img-hash.c
@@ -0,0 +1,1037 @@
+/*
+ * Copyright (c) 2014 Imagination Technologies
+ * Authors: Will Thomas, James Hartley
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
published
+ * by the Free Software Foundation.
+ *
+ * Interface structure taken from omap-sham driver
+ */
+
+#include linux/clk.h
+#include linux/dmaengine.h
+#include linux/interrupt.h
+#include linux/io.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/of_device.h
+#include linux/platform_device.h
+#include linux/scatterlist.h
+
+#include crypto/internal/hash.h
+#include crypto/md5.h
+#include crypto/sha.h
+
+#define CR_RESET 0
+#define CR_RESET_SET 1
+#define CR_RESET_UNSET 0
+
+#define CR_MESSAGE_LENGTH_H0x4
+#define CR_MESSAGE_LENGTH_L0x8
+
+#define CR_CONTROL 0xc
+#define CR_CONTROL_BYTE_ORDER_3210 0
+#define CR_CONTROL_BYTE_ORDER_0123 1
+#define CR_CONTROL_BYTE_ORDER_2310 2
+#define CR_CONTROL_BYTE_ORDER_1032 3
+#define CR_CONTROL_BYTE_ORDER_SHIFT8
+#define CR_CONTROL_ALGO_MD50
+#define CR_CONTROL_ALGO_SHA1 1
+#define CR_CONTROL_ALGO_SHA224 2
+#define CR_CONTROL_ALGO_SHA256 3
+
+#define CR_INTSTAT 0x10
+#define CR_INTENAB 0x14
+#define CR_INTCLEAR0x18
+#define CR_INT_RESULTS_AVAILABLE BIT(0)
+#define CR_INT_NEW_RESULTS_SET BIT(1)
+#define CR_INT_RESULT_READ_ERR BIT(2)
+#define CR_INT_MESSAGE_WRITE_ERROR BIT(3)
+#define CR_INT_STATUS BIT(8)
+
+#define CR_RESULT_QUEUE0x1c
+#define CR_RSD00x40
+#define CR_CORE_REV0x50
+#define CR_CORE_DES1 0x60
+#define CR_CORE_DES2 0x70
+
+#define DRIVER_FLAGS_BUSY BIT(0)
+#define DRIVER_FLAGS_FINAL BIT(1)
+#define DRIVER_FLAGS_DMA_ACTIVEBIT(2)
+#define DRIVER_FLAGS_OUTPUT_READY BIT(3)
+#define DRIVER_FLAGS_INIT BIT(4)
+#define DRIVER_FLAGS_CPU BIT(5)
+#define DRIVER_FLAGS_DMA_READY BIT(6)
+#define DRIVER_FLAGS_ERROR BIT(7)
+#define DRIVER_FLAGS_SGBIT(8)
+#define DRIVER_FLAGS_SHA1 BIT(18)
+#define DRIVER_FLAGS_SHA224BIT(19)
+#define DRIVER_FLAGS_SHA256BIT(20)
+#define DRIVER_FLAGS_MD5 BIT(21)
+
+#define IMG_HASH_QUEUE_LENGTH 20
+#define IMG_HASH_DMA_THRESHOLD 64
+
+#ifdef __LITTLE_ENDIAN
+#define IMG_HASH_BYTE_ORDER(CR_CONTROL_BYTE_ORDER_3210)
+#else
+#define IMG_HASH_BYTE_ORDER(CR_CONTROL_BYTE_ORDER_0123)
+#endif
Unnecessary parenthesis.
+struct img_hash_dev;
+
+struct img_hash_request_ctx {
+ struct img_hash_dev *hdev;
+ u8 digest[SHA256_DIGEST_SIZE] __aligned(sizeof(u32));
+ unsigned long flags;
+ size_t digsize;
+
+ dma_addr_t dma_addr;
+ size_t dma_ct;
+
+ /* sg root */
+ struct scatterlist *sgfirst;
+ /* walk state */
+ struct scatterlist *sg;
+ size_t nents;
+ size_t offset;
+ unsigned inttotal;
+ size_t sent;
+
+ unsigned long op;
+
+ size_t
Re: [PATCH 1/2] crypto: Add Imagination Technologies hw hash accelerator
On Fri, Nov 14, 2014 at 11:55 PM, Corentin LABBE
clabbe.montj...@gmail.com wrote:
Le 15/11/2014 00:59, Andrew Bresticker a écrit :
Hi James,
+
+struct img_hash_drv {
+ struct list_head dev_list;
+ spinlock_t lock;
+};
+
+static struct img_hash_drv img_hash = {
+ .dev_list = LIST_HEAD_INIT(img_hash.dev_list),
+ .lock = __SPIN_LOCK_UNLOCKED(img_hash.lock),
+};
It looks like the only purpose of this list is to get the
corresponding struct img_hash_dev in img_hash_init(). If there's
never going to be multiple instances within an SoC, perhaps you could
just use a global? Otherwise, you could do something like the
qualcomm driver, see drivers/crypto/qce/sha.c. It looks like there is
some precedent for this device list though...
I don't understand, you propose to use a global, something that lots of
people want to be removed in my driver.
It is not better than this global list.
I have the fealing that there no good way to get a pointer to a driver
structure inside the cryptoAPI.
What to you think about adding a void *data in struct crypto_alg
Before registering an alg you could do:
mv_aes_alg_ecb.data = myprivatedriverdata;
ret = crypto_register_alg(mv_aes_alg_ecb);
and then get it via
struct crypto_priv *cp = req-base.tfm-__crt_alg-data;
(a function will be better than that)
That sounds good to me.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Documentation: crypto: Add DT binding info for the img hw hash accelerator
Hi James, On Mon, Nov 10, 2014 at 4:10 AM, James Hartley james.hart...@imgtec.com wrote: Signed-off-by: James Hartley james.hart...@imgtec.com A brief commit message describing the hardware and where it's found would be nice. diff --git a/Documentation/devicetree/bindings/crypto/img-hash.txt b/Documentation/devicetree/bindings/crypto/img-hash.txt @@ -0,0 +1,28 @@ +* Imagination Technologies Ltd. Hash Accelerator + +The hash accelerator provides hardware hashing acceleration for +SHA1, SHA224, SHA256 and MD5 hashes + +Required properties: + +- compatible : img,img-hash-accelerator-rev1 I know I mentioned in the internal review that it would be good to have some sort of version indicator, but it looks like from the TRM that the version is probable (CR_HASH_CORE_REV). If we expect probing for the revision number to be sufficient, then perhaps rev1 can be dropped? Also, the second img is redundant. +- reg : Offset and length of the register set for the module, and the DMA port +- interrupts : The designated IRQ line for the hashing module. +- dmas : DMA specifier as per Documentation/devicetree/bindings/dma/dma.txt +- dma-names : Should be tx +- bus-addr : The bus address for the input data for hashing block I think this can be dropped. This is the same as the second reg entry above, is it not? -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
