crypto: vf-crc - Add new driver (and required clk, DTS...)
Subsystems (7)
cover crypto: vf-crc - Add new driver (and required clk, DTS...)
Hi,
Add new driver for Freescale/NXP Vybrid CRC HW accelerator along with prerequisities:
1. Clock,
2. Bindings,
3. CRC16 test tables for testing.
Patch 3/6 (ARM: dts: vfxxx: Add node for CRC hardware block) depends on patch 2/6 (clk) so it should go in following cycle (after clk change hits mainline).
Best regards,
Krzysztof
Krzysztof Kozlowski (6):
dt-bindings: crypto: Add Freescale Vybrid CRC
clk: imx: vf610: Add CRC clock
ARM: dts: vfxxx: Add node for CRC hardware block
crypto: testmgr - Add CRC16 test tables
crypto - Group Freescale/NXP ARM architecture Kconfig entires together
crypto: vf-crc - Add new driver for Freescale Vybrid CRC
.../devicetree/bindings/crypto/fsl-vf610-crc.txt | 16 + MAINTAINERS | 7 + arch/arm/boot/dts/vfxxx.dtsi | 7 + crypto/testmgr.c | 6 + crypto/testmgr.h | 386 ++++++++++++++++++++ drivers/clk/imx/clk-vf610.c | 1 + drivers/crypto/Kconfig | 42 ++- drivers/crypto/Makefile | 1 + drivers/crypto/vf-crc.c | 387 +++++++++++++++++++++ include/dt-bindings/clock/vf610-clock.h | 3 +- include/linux/crc32poly.h | 7 + 11 files changed, 846 insertions(+), 17 deletions(-) create mode 100644 Documentation/devicetree/bindings/crypto/fsl-vf610-crc.txt create mode 100644 drivers/crypto/vf-crc.c
Signature · 1 line
2.14.1
PATCH 1/6 dt-bindings: crypto: Add Freescale Vybrid CRC
Add bindings for Freescale/NXP Vybrid CRC HW accelerator.
--- .../devicetree/bindings/crypto/fsl-vf610-crc.txt | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 Documentation/devicetree/bindings/crypto/fsl-vf610-crc.txt
Documentation/devicetree/bindings/crypto/fsl-vf610-crc.txt new file +16 -0
@@ -0,0 +1,16 @@ +Freescale/NXP Vybrid VF500/VF610 CRC HW accelerator + +Required properties: +- compatible : Should be "fsl,vf610-crc" +- reg : Should contain register location and length. +- clocks : Should contain the clock driving the CRC block. +- clock-names : Should be set to "crc". + +Example: + +crc at 40033000 { + compatible = "fsl,vf610-crc"; + reg = <0x40033000 0x10>; + clocks = <&clks VF610_CLK_CRC>; + clock-names = "crc"; +};
Signature · 1 line
2.14.1
Rob Herring
maintainerPATCH 1/6 dt-bindings: crypto: Add Freescale Vybrid CRC
Quoted 8 lines · Krzysztof Kozlowski
Add bindings for Freescale/NXP Vybrid CRC HW accelerator.
Signed-off-by Krzysztof Kozlowski <[email protected]>--- .../devicetree/bindings/crypto/fsl-vf610-crc.txt | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 Documentation/devicetree/bindings/crypto/fsl-vf610-crc.txt
PATCH 2/6 clk: imx: vf610: Add CRC clock
In reply to Krzysztof KozlowskiAdd the clock for CRC block allowing it to be enabled by HW CRC driver.
--- drivers/clk/imx/clk-vf610.c | 1 + include/dt-bindings/clock/vf610-clock.h | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-)
drivers/clk/imx/clk-vf610.c +1 -0
@@ -332,6 +332,7 @@ static void __init vf610_clocks_init(struct device_node *ccm_node) clk[VF610_CLK_DSPI2 ] =imx_clk_gate2 ("dspi2" ,"ipg_bus" ,CCM_CCGR6 ,CCM_CCGRx_CGn (12 )); clk[VF610_CLK_DSPI3 ] =imx_clk_gate2 ("dspi3" ,"ipg_bus" ,CCM_CCGR6 ,CCM_CCGRx_CGn (13 )); + clk[VF610_CLK_CRC ] =imx_clk_gate2 ("crc" ,"ipg_bus" ,CCM_CCGR1 ,CCM_CCGRx_CGn (3 )); clk[VF610_CLK_WDT ] =imx_clk_gate2 ("wdt" ,"ipg_bus" ,CCM_CCGR1 ,CCM_CCGRx_CGn (14 )); clk[VF610_CLK_ESDHC0_SEL ] =imx_clk_mux ("esdhc0_sel" ,CCM_CSCMR1 ,16 ,2 , esdhc_sels,4 );
include/dt-bindings/clock/vf610-clock.h +2 -1
@@ -199,6 +199,7 @@#define VF610_CLK_WKPU 186#define VF610_CLK_TCON0 187#define VF610_CLK_TCON1 188 -#define VF610_CLK_END 189 +#define VF610_CLK_CRC 189 +#define VF610_CLK_END 190#endif /* __DT_BINDINGS_CLOCK_VF610_H */
Signature · 1 line
2.14.1
Re: [PATCH 2/6] clk: imx: vf610: Add CRC clock
Quoting Krzysztof Kozlowski (2018-08-30 10:15:35)
Quoted 4 lines
Add the clock for CRC block allowing it to be enabled by HW CRC driver.
Signed-off-by Krzysztof Kozlowski <[email protected]>---
Rob Herring
maintainerPATCH 2/6 clk: imx: vf610: Add CRC clock
In reply to Krzysztof KozlowskiQuoted 8 lines · Krzysztof Kozlowski
Add the clock for CRC block allowing it to be enabled by HW CRC driver.
Signed-off-by Krzysztof Kozlowski <[email protected]>--- drivers/clk/imx/clk-vf610.c | 1 + include/dt-bindings/clock/vf610-clock.h | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-)
PATCH 3/6 ARM: dts: vfxxx: Add node for CRC hardware block
In reply to Krzysztof KozlowskiAdd node for HW CRC16/CRC32 accelerator present on VF500 and VF610 SoCs.
--- arch/arm/boot/dts/vfxxx.dtsi | 7 +++++++ 1 file changed, 7 insertions(+)
arch/arm/boot/dts/vfxxx.dtsi +7 -0
@@ -276,6 +276,13 @@ status = "disabled"; }; + crc: crc at 40033000 { + compatible = "fsl,vf610-crc"; + reg = <0x40033000 0x10>; + clocks = <&clks VF610_CLK_CRC>; + clock-names = "crc"; + }; + pit: pit at 40037000 { compatible = "fsl,vf610-pit"; reg = <0x40037000 0x1000>;
Signature · 1 line
2.14.1
PATCH 4/6 crypto: testmgr - Add CRC16 test tables
In reply to Krzysztof KozlowskiAdd test tables for CRC16. Use similar input data as for existing CRC32 test vectors.
--- crypto/testmgr.c | 6 + crypto/testmgr.h | 386 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 392 insertions(+)
crypto/testmgr.c +6 -0
@@ -2707,6 +2707,12 @@ static const struct alg_test_desc alg_test_descs[] = { }, { .alg = "compress_null", .test = alg_test_null, + }, { + .alg = "crc16", + .test = alg_test_hash, + .suite = { + .hash = __VECS(crc16_tv_template) + } }, { .alg = "crc32", .test = alg_test_hash,
crypto/testmgr.h +386 -0
392 lines not on this page · raw message
Signature · 1 line
2.14.1
PATCH 5/6 crypto - Group Freescale/NXP ARM architecture Kconfig entires together
In reply to Krzysztof KozlowskiGroup Freescale/NXP Kconfig entires for ARM architecture CPUs/SoCs together to make it more consistent and easy to read.
--- drivers/crypto/Kconfig | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
drivers/crypto/Kconfig +16 -16
@@ -402,6 +402,22 @@ config CRYPTO_DEV_MXC_SCC This option enables support for the Security Controller (SCC) found in Freescale i.MX25 chips. +config CRYPTO_DEV_MXS_DCP + tristate "Support for Freescale MXS DCP" + depends on (ARCH_MXS || ARCH_MXC) + select STMP_DEVICE + select CRYPTO_CBC + select CRYPTO_ECB + select CRYPTO_AES + select CRYPTO_BLKCIPHER + select CRYPTO_HASH + help + The Freescale i.MX23/i.MX28 has SHA1/SHA256 and AES128 CBC/ECB + co-processor on the die. + + To compile this driver as a module, choose M here: the module + will be called mxs-dcp. + config CRYPTO_DEV_EXYNOS_RNG tristate "EXYNOS HW pseudo random number generator support" depends on ARCH_EXYNOS || COMPILE_TEST
@@ -542,22 +558,6 @@ if CRYPTO_DEV_CCP source "drivers/crypto/ccp/Kconfig" endif -config CRYPTO_DEV_MXS_DCP - tristate "Support for Freescale MXS DCP" - depends on (ARCH_MXS || ARCH_MXC) - select STMP_DEVICE - select CRYPTO_CBC - select CRYPTO_ECB - select CRYPTO_AES - select CRYPTO_BLKCIPHER - select CRYPTO_HASH - help - The Freescale i.MX23/i.MX28 has SHA1/SHA256 and AES128 CBC/ECB - co-processor on the die. - - To compile this driver as a module, choose M here: the module - will be called mxs-dcp. - source "drivers/crypto/qat/Kconfig" source "drivers/crypto/cavium/cpt/Kconfig" source "drivers/crypto/cavium/nitrox/Kconfig"
Signature · 1 line
2.14.1
PATCH 6/6 crypto: vf-crc - Add new driver for Freescale Vybrid CRC
In reply to Krzysztof KozlowskiAdd driver for using the Freescale/NXP Vybrid processor CRC block for CRC16 and CRC32 offloading. The driver implements shash_alg and was tested using internal testmgr tests and libkcapi.
--- MAINTAINERS | 7 + drivers/crypto/Kconfig | 10 ++ drivers/crypto/Makefile | 1 + drivers/crypto/vf-crc.c | 387 ++++++++++++++++++++++++++++++++++++++++++++++ include/linux/crc32poly.h | 7 + 5 files changed, 412 insertions(+) create mode 100644 drivers/crypto/vf-crc.c
MAINTAINERS +7 -0
@@ -15388,6 +15388,13 @@ S: Maintained F: Documentation/fb/uvesafb.txt F: drivers/video/fbdev/uvesafb.* +VF500/VF610 HW CRC DRIVER +M: Krzysztof Kozlowski <[email protected]> +L: linux-crypto at vger.kernel.org +S: Maintained +F: drivers/crypto/vf-crc.c +F: Documentation/devicetree/bindings/crypto/fsl-vf610-crc.txt + VF610 NAND DRIVER M: Stefan Agner <[email protected]> L: linux-mtd at lists.infradead.org
drivers/crypto/Kconfig +10 -0
@@ -418,6 +418,16 @@ config CRYPTO_DEV_MXS_DCP To compile this driver as a module, choose M here: the module will be called mxs-dcp. +config CRYPTO_DEV_VF_CRC + tristate "Support for Freescale/NXP Vybrid CRC HW accelerator" + select CRYPTO_HASH + help + This option enables support for the CRC16/32 hardware accelerator + on Freescale/NXP Vybrid VF500/VF610 SoCs. + + To compile this driver as a module, choose M here: the module + will be called vf-crc. + config CRYPTO_DEV_EXYNOS_RNG tristate "EXYNOS HW pseudo random number generator support" depends on ARCH_EXYNOS || COMPILE_TEST
drivers/crypto/Makefile +1 -0
@@ -41,6 +41,7 @@ obj-$(CONFIG_ARCH_STM32) += stm32/ obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/ obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/ +obj-$(CONFIG_CRYPTO_DEV_VF_CRC) += vf-crc.o obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio/ obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/ obj-$(CONFIG_CRYPTO_DEV_BCM_SPU) += bcm/
drivers/crypto/vf-crc.c new file +387 -0
387 lines not on this page · raw message
include/linux/crc32poly.h +7 -0
@@ -17,4 +17,11 @@*/ #define CRC32C_POLY_LE 0x82F63B78 +/* +* CRC16 polynomial as defined by ITU-T V.41 (ITU Telecommunication +* Standardization Sector recommendations) +* x^16 + x^12 + x^5 + x^0 +*/ +#define CRC16_POLY_BE 0x1021 +#endif /* _LINUX_CRC32_POLY_H */
Signature · 1 line
2.14.1
Sascha Hauer
maintainerRe: [PATCH 6/6] crypto: vf-crc - Add new driver for Freescale Vybrid CRC
Hi Krzysztof,
Some comments inline.
Quoted 66 lines · Krzysztof Kozlowski
Add driver for using the Freescale/NXP Vybrid processor CRC block for CRC16 and CRC32 offloading. The driver implements
shash_algand was tested using internal testmgr tests and libkcapi.Signed-off-by Krzysztof Kozlowski <[email protected]>--- MAINTAINERS | 7 + drivers/crypto/Kconfig | 10 ++ drivers/crypto/Makefile | 1 + drivers/crypto/vf-crc.c | 387 ++++++++++++++++++++++++++++++++++++++++++++++ include/linux/crc32poly.h | 7 + 5 files changed, 412 insertions(+) create mode 100644 drivers/crypto/vf-crc.cMAINTAINERS +7
-0@@ -15388,6 +15388,13 @@ S: Maintained F: Documentation/fb/uvesafb.txt F: drivers/video/fbdev/uvesafb.* +VF500/VF610 HW CRC DRIVER +M: Krzysztof Kozlowski <[email protected]> +L: linux-crypto at vger.kernel.org +S: Maintained +F: drivers/crypto/vf-crc.c +F: Documentation/devicetree/bindings/crypto/fsl-vf610-crc.txt + VF610 NAND DRIVER M: Stefan Agner <[email protected]> L: linux-mtd at lists.infradead.orgdrivers/crypto/Kconfig +10
-0@@ -418,6 +418,16 @@ config CRYPTO_DEV_MXS_DCP To compile this driver as a module, choose M here: the module will be called mxs-dcp. +config CRYPTO_DEV_VF_CRC + tristate "Support for Freescale/NXP Vybrid CRC HW accelerator" + select CRYPTO_HASH + help + This option enables support for the CRC16/32 hardware accelerator + on Freescale/NXP Vybrid VF500/VF610 SoCs. + + To compile this driver as a module, choose M here: the module + will be called vf-crc. + config CRYPTO_DEV_EXYNOS_RNG tristate "EXYNOS HW pseudo random number generator support" depends on ARCH_EXYNOS || COMPILE_TESTdrivers/crypto/Makefile +2
-0@@ -41,6 +41,7 @@ obj-$(CONFIG_ARCH_STM32) += stm32/ obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/ obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/ +obj-$(CONFIG_CRYPTO_DEV_VF_CRC) += vf-crc.o obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio/ obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/ obj-$(CONFIG_CRYPTO_DEV_BCM_SPU) += bcm/ +static int vf_crc_update_prepare(struct vf_crc_tfm_ctx *mctx,
drivers/crypto/Makefile obj-$(CONFIG_ARCH_STM32) += stm32/
Quoted 10 lines
drivers/crypto/Makefile +10
-0+ struct vf_crc_desc_ctx *desc_ctx) +{ + struct vf_crc *crc = desc_ctx->crc; + int ret; + + ret = clk_prepare_enable(crc->clk); + if (ret) { + dev_err(crc->dev, "Failed to enable clock\n"); + return ret; + }
Generally have you measured the performance of this driver? Is it faster than the software implementation?
Under certain circumstances a clk_prepare_enable might become expensive, so it could happen that all this clk enabling/disabling takes longer than the action you do in between. Using pm_runtime might help here.
drivers/crypto/Makefile
Quoted 10 lines
drivers/crypto/Makefile +10
-0+ + mutex_lock(&crc->lock); + + /* + * Check if we are continuing to process request already configured + * in HW. HW has to be re-initialized only on first update() for given + * request or if new request was processed after last call to update(). + */ + if (crc->processed_desc == desc_ctx) + return 0;
You never set crc->processed_desc to anything, so this optimization never triggers.
Unless properly implementing this skip-to-reinitialize-hardware really brings a measurerable performance gain I would just drop this optimization. In the end you only save a few register writes, but it makes the driver more complicated.
Quoted 9 lines
drivers/crypto/Makefile +8
-0+ + vf_crc_initialize_regs(mctx, desc_ctx); + + return 0; +} + +static int vf_crc_finup(struct shash_desc *desc, const u8 *data, + unsigned int len, u8 *out)
drivers/crypto/Makefile
Quoted 10 lines
drivers/crypto/Makefile +10
-0+{ + return vf_crc_update(desc, data, len) ?: + vf_crc_final(desc, out); +} + +static int vf_crc_digest(struct shash_desc *desc, const u8 *data, + unsigned int leng, u8 *out) +{ + return vf_crc_init(desc) ?: vf_crc_finup(desc, data, leng, out); +}
These seem unnecessary. The crypto core will set these with similar wrappers if unspecified.
Quoted 28 lines
drivers/crypto/Makefile +28
-0+static int vf_crc_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct resource *res; + struct vf_crc *crc; + int ret; + + if (vf_crc_data) { + dev_err(dev, "Device already registered (only one instance allowed)\n"); + return -EINVAL; + } + + crc = devm_kzalloc(dev, sizeof(*crc), GFP_KERNEL); + if (!crc) + return -ENOMEM; + + crc->dev = dev; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + crc->iobase = devm_ioremap_resource(dev, res); + if (IS_ERR(crc->iobase)) + return PTR_ERR(crc->iobase); + + crc->clk = devm_clk_get(dev, "crc"); + if (IS_ERR(crc->clk)) { + dev_err(dev, "Could not get clock\n"); + return PTR_ERR(crc->clk); + }
drivers/crypto/Makefile
Quoted 10 lines
drivers/crypto/Makefile +10
-0+ + vf_crc_data = crc; + + ret = crypto_register_shashes(algs, ARRAY_SIZE(algs)); + if (ret) { + dev_err(dev, "Failed to register crypto algorithms\n"); + goto err; + } + + mutex_init(&crc->lock);
Should be done before the shashes are registered.
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Re: [PATCH 6/6] crypto: vf-crc - Add new driver for Freescale Vybrid CRC
Quoted 77 lines · Sascha Hauer
Hi Krzysztof,
Some comments inline.
Quoted 71 lines · Krzysztof Kozlowski
Add driver for using the Freescale/NXP Vybrid processor CRC block for CRC16 and CRC32 offloading. The driver implements
shash_algand was tested using internal testmgr tests and libkcapi.Signed-off-by Krzysztof Kozlowski <[email protected]>--- MAINTAINERS | 7 + drivers/crypto/Kconfig | 10 ++ drivers/crypto/Makefile | 1 + drivers/crypto/vf-crc.c | 387 ++++++++++++++++++++++++++++++++++++++++++++++ include/linux/crc32poly.h | 7 + 5 files changed, 412 insertions(+) create mode 100644 drivers/crypto/vf-crc.cMAINTAINERS +7
-0@@ -15388,6 +15388,13 @@ S: Maintained F: Documentation/fb/uvesafb.txt F: drivers/video/fbdev/uvesafb.* +VF500/VF610 HW CRC DRIVER +M: Krzysztof Kozlowski <[email protected]> +L: linux-crypto at vger.kernel.org +S: Maintained +F: drivers/crypto/vf-crc.c +F: Documentation/devicetree/bindings/crypto/fsl-vf610-crc.txt + VF610 NAND DRIVER M: Stefan Agner <[email protected]> L: linux-mtd at lists.infradead.orgdrivers/crypto/Kconfig +10
-0@@ -418,6 +418,16 @@ config CRYPTO_DEV_MXS_DCP To compile this driver as a module, choose M here: the module will be called mxs-dcp. +config CRYPTO_DEV_VF_CRC + tristate "Support for Freescale/NXP Vybrid CRC HW accelerator" + select CRYPTO_HASH + help + This option enables support for the CRC16/32 hardware accelerator + on Freescale/NXP Vybrid VF500/VF610 SoCs. + + To compile this driver as a module, choose M here: the module + will be called vf-crc. + config CRYPTO_DEV_EXYNOS_RNG tristate "EXYNOS HW pseudo random number generator support" depends on ARCH_EXYNOS || COMPILE_TESTdrivers/crypto/Makefile +7
-0@@ -41,6 +41,7 @@ obj-$(CONFIG_ARCH_STM32) += stm32/ obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/ obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/ +obj-$(CONFIG_CRYPTO_DEV_VF_CRC) += vf-crc.o obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio/ obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/ obj-$(CONFIG_CRYPTO_DEV_BCM_SPU) += bcm/ +static int vf_crc_update_prepare(struct vf_crc_tfm_ctx *mctx, + struct vf_crc_desc_ctx *desc_ctx) +{ + struct vf_crc *crc = desc_ctx->crc; + int ret; +
Quoted 8 lines
drivers/crypto/Makefile
Quoted 5 lines
drivers/crypto/Makefile +5
-0+ ret = clk_prepare_enable(crc->clk); + if (ret) { + dev_err(crc->dev, "Failed to enable clock\n"); + return ret; + }Generally have you measured the performance of this driver? Is it faster than the software implementation?
I wanted to replace our in-house out-of-tree, hacky ioctl-based driver with something more upstreamable. I run few simple user-space performance tests and in fact SW implementation is faster. Around 5x faster for this version of driver. However it depends highly on size of message (buffer) because there is big overhead of libkcapi.
The typical SW implementation (with lookup tables) is just fetching of data from memory and computing. Usage of libkcapi is at least three library function calls on user-space side and a bunch of other code on kernel side.
There are two benefits:
1. CPU could be offloaded and do something in parallel. However for this I should probably implement asymmetric hash. Otherwise wastes cycles on reading from CRC registers... and of course on clk prepare and mutex handing.
2. Theoretically it could lower energy consumption... as CPU would not be that busy. I found 3% lower power usage (0.18 A -> 0.175 A) but if you multiply it per time then total energy spent would be higher.
Does this driver makes sense in such case? In fact I have doubts...
It was nice exercise for me though. :)
Quoted 4 lines
Under certain circumstances a
clk_prepare_enablemight become expensive, so it could happen that all this clk enabling/disabling takes longer than the action you do in between. Usingpm_runtimemight help here.
I should convert them to just clk_enable/disable. The pm_runtime is also a huge framework and adds its own overhead. Using it just to toggle one clock is a lot.
Quoted 5 lines
Quoted 5 lines
drivers/crypto/Makefile +5
-0+ + mutex_lock(&crc->lock); + + /* + * Check if we are continuing to process request already configured
Quoted 8 lines
drivers/crypto/Makefile
Quoted 5 lines
drivers/crypto/Makefile +5
-0+ * in HW. HW has to be re-initialized only on first update() for given + * request or if new request was processed after last call to update(). + */ + if (crc->processed_desc == desc_ctx) + return 0;You never set
crc->processed_descto anything, so this optimization never triggers.
Ah, damn, I missed setting it!
Quoted 4 lines
Unless properly implementing this skip-to-reinitialize-hardware really brings a measurerable performance gain I would just drop this optimization. In the end you only save a few register writes, but it makes the driver more complicated.
I measured it now... and indeed - removal of this optimization allows to remove also one mutex lock/unlock - so the total net is 0.8% faster with the optimization.
Quoted 7 lines
Quoted 6 lines
drivers/crypto/Makefile +6
-0+ + vf_crc_initialize_regs(mctx, desc_ctx); + + return 0; +} +
Quoted 15 lines
drivers/crypto/Makefile
Quoted 12 lines
drivers/crypto/Makefile +12
-0+static int vf_crc_finup(struct shash_desc *desc, const u8 *data, + unsigned int len, u8 *out) +{ + return vf_crc_update(desc, data, len) ?: + vf_crc_final(desc, out); +} + +static int vf_crc_digest(struct shash_desc *desc, const u8 *data, + unsigned int leng, u8 *out) +{ + return vf_crc_init(desc) ?: vf_crc_finup(desc, data, leng, out); +}These seem unnecessary. The crypto core will set these with similar wrappers if unspecified.
Sure.
Quoted 33 lines
Quoted 33 lines
drivers/crypto/Makefile +32
-0+static int vf_crc_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct resource *res; + struct vf_crc *crc; + int ret; + + if (vf_crc_data) { + dev_err(dev, "Device already registered (only one instance allowed)\n"); + return -EINVAL; + } + + crc = devm_kzalloc(dev, sizeof(*crc), GFP_KERNEL); + if (!crc) + return -ENOMEM; + + crc->dev = dev; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + crc->iobase = devm_ioremap_resource(dev, res); + if (IS_ERR(crc->iobase)) + return PTR_ERR(crc->iobase); + + crc->clk = devm_clk_get(dev, "crc"); + if (IS_ERR(crc->clk)) { + dev_err(dev, "Could not get clock\n"); + return PTR_ERR(crc->clk); + } + + vf_crc_data = crc; + + ret = crypto_register_shashes(algs, ARRAY_SIZE(algs));
Quoted 8 lines
drivers/crypto/Makefile
Quoted 6 lines
drivers/crypto/Makefile +6
-0+ if (ret) { + dev_err(dev, "Failed to register crypto algorithms\n"); + goto err; + } + + mutex_init(&crc->lock);Should be done before the shashes are registered.
Right.
Thanks for the review!
Krzysztof
Sascha Hauer
maintainerRe: [PATCH 6/6] crypto: vf-crc - Add new driver for Freescale Vybrid CRC
Quoted 87 lines · Krzysztof Kozlowski
Quoted 77 lines · Sascha Hauer
Hi Krzysztof,
Some comments inline.
Quoted 71 lines · Krzysztof Kozlowski
Add driver for using the Freescale/NXP Vybrid processor CRC block for CRC16 and CRC32 offloading. The driver implements
shash_algand was tested using internal testmgr tests and libkcapi.Signed-off-by Krzysztof Kozlowski <[email protected]>--- MAINTAINERS | 7 + drivers/crypto/Kconfig | 10 ++ drivers/crypto/Makefile | 1 + drivers/crypto/vf-crc.c | 387 ++++++++++++++++++++++++++++++++++++++++++++++ include/linux/crc32poly.h | 7 + 5 files changed, 412 insertions(+) create mode 100644 drivers/crypto/vf-crc.cMAINTAINERS +7
-0@@ -15388,6 +15388,13 @@ S: Maintained F: Documentation/fb/uvesafb.txt F: drivers/video/fbdev/uvesafb.* +VF500/VF610 HW CRC DRIVER +M: Krzysztof Kozlowski <[email protected]> +L: linux-crypto at vger.kernel.org +S: Maintained +F: drivers/crypto/vf-crc.c +F: Documentation/devicetree/bindings/crypto/fsl-vf610-crc.txt + VF610 NAND DRIVER M: Stefan Agner <[email protected]> L: linux-mtd at lists.infradead.orgdrivers/crypto/Kconfig +10
-0@@ -418,6 +418,16 @@ config CRYPTO_DEV_MXS_DCP To compile this driver as a module, choose M here: the module will be called mxs-dcp. +config CRYPTO_DEV_VF_CRC + tristate "Support for Freescale/NXP Vybrid CRC HW accelerator" + select CRYPTO_HASH + help + This option enables support for the CRC16/32 hardware accelerator + on Freescale/NXP Vybrid VF500/VF610 SoCs. + + To compile this driver as a module, choose M here: the module + will be called vf-crc. + config CRYPTO_DEV_EXYNOS_RNG tristate "EXYNOS HW pseudo random number generator support" depends on ARCH_EXYNOS || COMPILE_TESTdrivers/crypto/Makefile +7
-0@@ -41,6 +41,7 @@ obj-$(CONFIG_ARCH_STM32) += stm32/ obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/ obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/ +obj-$(CONFIG_CRYPTO_DEV_VF_CRC) += vf-crc.o obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio/ obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/ obj-$(CONFIG_CRYPTO_DEV_BCM_SPU) += bcm/ +static int vf_crc_update_prepare(struct vf_crc_tfm_ctx *mctx, + struct vf_crc_desc_ctx *desc_ctx) +{ + struct vf_crc *crc = desc_ctx->crc; + int ret; +Quoted 8 lines
drivers/crypto/Makefile
Quoted 5 lines
drivers/crypto/Makefile +5
-0+ ret = clk_prepare_enable(crc->clk); + if (ret) { + dev_err(crc->dev, "Failed to enable clock\n"); + return ret; + }Generally have you measured the performance of this driver? Is it faster than the software implementation?
Quoted 5 lines
I wanted to replace our in-house out-of-tree, hacky ioctl-based driver with something more upstreamable. I run few simple user-space performance tests and in fact SW implementation is faster. Around 5x faster for this version of driver. However it depends highly on size of message (buffer) because there is big overhead of libkcapi.
Well, I meant comparing the hardware vs. software implementation directly in the kernel. Of course when a userspace API is involved the comparison is not fair.
Quoted 11 lines
The typical SW implementation (with lookup tables) is just fetching of data from memory and computing. Usage of libkcapi is at least three library function calls on user-space side and a bunch of other code on kernel side.
There are two benefits:
1. CPU could be offloaded and do something in parallel. However for this I should probably implement asymmetric hash. Otherwise wastes cycles on reading from CRC registers... and of course on clk prepare and mutex handing.
The CPU can only do something in parallel when it's otherwise idle. In your driver the CPU is 100% busy, so no time to do something else.
Quoted 9 lines
2. Theoretically it could lower energy consumption... as CPU would not be that busy. I found 3% lower power usage (0.18 A -> 0.175 A) but if you multiply it per time then total energy spent would be higher.
Does this driver makes sense in such case? In fact I have doubts...
It was nice exercise for me though. :)
Quoted 1 line
Quoted 7 lines
Quoted 3 lines
Under certain circumstances a
clk_prepare_enablemight become expensive, so it could happen that all this clk enabling/disabling takes longer than the action you do in between. Usingpm_runtimemight help here.I should convert them to just clk_enable/disable. The
pm_runtimeis also a huge framework and adds its own overhead. Using it just to toggle one clock is a lot.
There are probably more drivers in your system that make use of pm_runtime, so no need to add it only for this one driver.
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |