| Message ID | 20190906184551.17858-1-clabbe.montjoie@gmail.com |
|---|---|
| Headers | show |
| Series | crypto: add sun8i-ce driver for Allwinner crypto engine | expand |
On Fri, Sep 06, 2019 at 08:45:43PM +0200, Corentin Labbe wrote: > Since a second Allwinner crypto driver will be added, it is better to > create a dedicated subdirectory. > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > --- > MAINTAINERS | 6 ++++++ > drivers/crypto/Kconfig | 2 ++ > drivers/crypto/Makefile | 1 + > drivers/crypto/allwinner/Kconfig | 6 ++++++ I guess it would make sense to move the sun4i driver there too? Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Fri, Sep 06, 2019 at 08:45:46PM +0200, Corentin Labbe wrote: > The Crypto Engine is a hardware cryptographic offloader that supports > many algorithms. > It could be found on most Allwinner SoCs. > > This patch enables the Crypto Engine on the Allwinner R40 SoC Device-tree. > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > --- > arch/arm/boot/dts/sun8i-r40.dtsi | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi > index bde068111b85..7eb649cea163 100644 > --- a/arch/arm/boot/dts/sun8i-r40.dtsi > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi > @@ -266,6 +266,17 @@ > #phy-cells = <1>; > }; > > + crypto: crypto-engine@1c15000 { > + compatible = "allwinner,sun8i-r40-crypto"; > + reg = <0x01c15000 0x1000>; > + interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&ccu CLK_BUS_CE>, <&ccu CLK_CE>; > + clock-names = "ahb", "mod"; > + resets = <&ccu RST_BUS_CE>; > + reset-names = "ahb"; > + status = "okay"; The driver will probe if status is not declared, so if you want it always enabled you should simply remove status Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Fri, Sep 06, 2019 at 08:45:48PM +0200, Corentin Labbe wrote: > The Crypto Engine is a hardware cryptographic accelerator that supports > many algorithms. > It could be found on most Allwinner SoCs. > > This patch enables the Crypto Engine on the Allwinner A64 SoC Device-tree. > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > --- > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > index 69128a6dfc46..c9e30d462ab1 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > @@ -487,6 +487,17 @@ > reg = <0x1c14000 0x400>; > }; > > + crypto: crypto@1c15000 { > + compatible = "allwinner,sun50i-a64-crypto"; > + reg = <0x01c15000 0x1000>; > + interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "ce_ns"; You didn't document that property Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Fri, Sep 06, 2019 at 08:45:51PM +0200, Corentin Labbe wrote: > This patch adds the new allwinner crypto configs to sunxi_defconfig > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > --- > arch/arm/configs/sunxi_defconfig | 2 ++ > 1 file changed, 2 insertions(+) Can you also enable it in arm64's defconfig as a module? > > diff --git a/arch/arm/configs/sunxi_defconfig b/arch/arm/configs/sunxi_defconfig > index df433abfcb02..d0ab8ba7710a 100644 > --- a/arch/arm/configs/sunxi_defconfig > +++ b/arch/arm/configs/sunxi_defconfig > @@ -150,4 +150,6 @@ CONFIG_NLS_CODEPAGE_437=y > CONFIG_NLS_ISO8859_1=y > CONFIG_PRINTK_TIME=y > CONFIG_DEBUG_FS=y > +CONFIG_CRYPTO_DEV_ALLWINNER=y > +CONFIG_CRYPTO_DEV_SUN8I_CE=y > CONFIG_CRYPTO_DEV_SUN4I_SS=y > -- > 2.21.0 > -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi, I can't really comment on the crypto side, so my review is going to be pretty boring. On Fri, Sep 06, 2019 at 08:45:44PM +0200, Corentin Labbe wrote: > +static const struct ce_variant ce_h3_variant = { > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > + CE_ID_NOTSUPP, > + }, As far as I can see, it's always the same value, so I'm not sure why it's a parameter. > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > + }, Ditto > + .intreg = CE_ISR, Ditto > + .maxflow = 4, Ditto > + .ce_clks = { > + { "ahb", 200000000 }, This is the default IIRC, and the clock driver will ignore any clock rate change on it anyway, so the clock rate is pretty much useless there. > + { "mod", 48000000 }, 48MHz seems pretty slow, especially compared to the other rates you have listed there. Is that on purpose? Also, I'm not sure what is the point of having the clocks names be parameters there as well. It's constant across all the compatibles, the only thing that isn't is the number of clocks and the module clock rate. It's what you should have in there. > + } > +}; > + > +static const struct ce_variant ce_h5_variant = { > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > + CE_ID_NOTSUPP, > + }, > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > + }, > + .intreg = CE_ISR, > + .maxflow = 4, > + .ce_clks = { > + { "ahb", 200000000 }, > + { "mod", 300000000 }, > + } > +}; > + > +static const struct ce_variant ce_h6_variant = { > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > + CE_ALG_RAES, > + }, > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > + }, > + .model = CE_v2, Can't that be derived from the version register and / or the compatible? This seems to be redundant with each. > + .intreg = CE_ISR, > + .maxflow = 4, > + .ce_clks = { > + { "ahb", 200000000 }, > + { "mod", 300000000 }, > + { "mbus", 400000000 }, That rate is going to be ignored as well. > + } > +}; > + > +static const struct ce_variant ce_a64_variant = { > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > + CE_ID_NOTSUPP, > + }, > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > + }, > + .intreg = CE_ISR, > + .maxflow = 4, > + .ce_clks = { > + { "ahb", 200000000 }, > + { "mod", 300000000 }, > + } > +}; You should order your variants by alphabetical order. > +static const struct ce_variant ce_r40_variant = { > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > + CE_ID_NOTSUPP, > + }, > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > + }, > + .intreg = CE_ISR, > + .maxflow = 4, > + .ce_clks = { > + { "ahb", 200000000 }, > + { "mod", 300000000 }, > + } > +}; > + ... > +int sun8i_ce_get_engine_number(struct sun8i_ce_dev *ce) > +{ > + return atomic_inc_return(&ce->flow) % ce->variant->maxflow; > +} I'm not sure what this is supposed to be doing, but that mod there seems pretty dangerous. ... > +static int sun8i_ce_probe(struct platform_device *pdev) > +{ > + struct resource *res; > + u32 v; > + int err, i, ce_method, id, irq; > + unsigned long cr; > + struct sun8i_ce_dev *ce; > + > + if (!pdev->dev.of_node) > + return -ENODEV; This is pretty much guaranteed already > + ce = devm_kzalloc(&pdev->dev, sizeof(*ce), GFP_KERNEL); > + if (!ce) > + return -ENOMEM; > + > + ce->variant = of_device_get_match_data(&pdev->dev); > + if (!ce->variant) { > + dev_err(&pdev->dev, "Missing Crypto Engine variant\n"); > + return -EINVAL; > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + ce->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(ce->base)) { > + err = PTR_ERR(ce->base); > + dev_err(&pdev->dev, "Cannot request MMIO err=%d\n", err); > + return err; ioremap_resource already prints an error message on failure, so this is redundant. > + } > + > + for (i = 0; i < CE_MAX_CLOCKS; i++) { > + if (!ce->variant->ce_clks[i].name) > + continue; > + dev_info(&pdev->dev, "Get %s clock\n", ce->variant->ce_clks[i].name); There's no reason to print this at the info level > + ce->ceclks[i] = devm_clk_get(&pdev->dev, ce->variant->ce_clks[i].name); > + if (IS_ERR(ce->ceclks[i])) { > + err = PTR_ERR(ce->ceclks[i]); > + dev_err(&pdev->dev, "Cannot get %s CE clock err=%d\n", > + ce->variant->ce_clks[i].name, err); > + } > + cr = clk_get_rate(ce->ceclks[i]); So on error you'd call clk_get_rate on the clock still? That seems pretty fragile, you should return there, it's a hard error. > + if (ce->variant->ce_clks[i].freq) { > + dev_info(&pdev->dev, "Set %s clock to %lu (%lu Mhz) from %lu (%lu Mhz)\n", > + ce->variant->ce_clks[i].name, > + ce->variant->ce_clks[i].freq, > + ce->variant->ce_clks[i].freq / 1000000, > + cr, > + cr / 1000000); Same remark about that message than the previous one. > + err = clk_set_rate(ce->ceclks[i], ce->variant->ce_clks[i].freq); > + if (err) > + dev_err(&pdev->dev, "Fail to set %s clk speed to %lu\n", > + ce->variant->ce_clks[i].name, > + ce->variant->ce_clks[i].freq); > + } else { > + dev_info(&pdev->dev, "%s run at %lu\n", > + ce->variant->ce_clks[i].name, cr); Ditto. > + } > + err = clk_prepare_enable(ce->ceclks[i]); Do you really need this right now though? > + if (err) { > + dev_err(&pdev->dev, "Cannot prepare_enable %s\n", > + ce->variant->ce_clks[i].name); > + return err; > + } > + } > + > + /* Get Non Secure IRQ */ > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(ce->dev, "Cannot get NS IRQ\n"); > + return irq; > + } > + > + err = devm_request_irq(&pdev->dev, irq, ce_irq_handler, 0, > + "sun8i-ce-ns", ce); > + if (err < 0) { > + dev_err(ce->dev, "Cannot request NS IRQ\n"); > + return err; > + } > + > + ce->reset = devm_reset_control_get_optional(&pdev->dev, "ahb"); > + if (IS_ERR(ce->reset)) { > + if (PTR_ERR(ce->reset) == -EPROBE_DEFER) > + return PTR_ERR(ce->reset); > + dev_info(&pdev->dev, "No reset control found\n"); It's not optional though. > + ce->reset = NULL; > + } > + > + err = reset_control_deassert(ce->reset); > + if (err) { > + dev_err(&pdev->dev, "Cannot deassert reset control\n"); > + goto error_clk; > + } Again, you don't really need this at this moment. Using runtime_pm would make more sense. > + v = readl(ce->base + CE_CTR); > + v >>= 16; > + v &= 0x07; This should be in a define > + dev_info(&pdev->dev, "CE_NS Die ID %x\n", v); And if that really makes sense to print it, the error message should be made less cryptic. > + > + ce->dev = &pdev->dev; > + platform_set_drvdata(pdev, ce); > + > + mutex_init(&ce->mlock); > + > + ce->chanlist = devm_kcalloc(ce->dev, ce->variant->maxflow, > + sizeof(struct sun8i_ce_flow), GFP_KERNEL); > + if (!ce->chanlist) { > + err = -ENOMEM; > + goto error_flow; > + } > + > + for (i = 0; i < ce->variant->maxflow; i++) { > + init_completion(&ce->chanlist[i].complete); > + mutex_init(&ce->chanlist[i].lock); > + > + ce->chanlist[i].engine = crypto_engine_alloc_init(ce->dev, true); > + if (!ce->chanlist[i].engine) { > + dev_err(ce->dev, "Cannot allocate engine\n"); > + i--; > + goto error_engine; > + } > + err = crypto_engine_start(ce->chanlist[i].engine); > + if (err) { > + dev_err(ce->dev, "Cannot start engine\n"); > + goto error_engine; > + } > + ce->chanlist[i].tl = dma_alloc_coherent(ce->dev, > + sizeof(struct ce_task), > + &ce->chanlist[i].t_phy, > + GFP_KERNEL); > + if (!ce->chanlist[i].tl) { > + dev_err(ce->dev, "Cannot get DMA memory for task %d\n", > + i); > + err = -ENOMEM; > + goto error_engine; > + } > + } All this initialization should be done before calling request_irq. You're using some of those fields in your handler. > + > +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG > + ce->dbgfs_dir = debugfs_create_dir("sun8i-ce", NULL); > + if (IS_ERR_OR_NULL(ce->dbgfs_dir)) { > + dev_err(ce->dev, "Fail to create debugfs dir"); > + err = -ENOMEM; > + goto error_engine; > + } > + ce->dbgfs_stats = debugfs_create_file("stats", 0444, > + ce->dbgfs_dir, ce, > + &sun8i_ce_debugfs_fops); > + if (IS_ERR_OR_NULL(ce->dbgfs_stats)) { > + dev_err(ce->dev, "Fail to create debugfs stat"); > + err = -ENOMEM; > + goto error_debugfs; > + } > +#endif > + for (i = 0; i < ARRAY_SIZE(ce_algs); i++) { > + ce_algs[i].ce = ce; > + switch (ce_algs[i].type) { > + case CRYPTO_ALG_TYPE_SKCIPHER: > + id = ce_algs[i].ce_algo_id; > + ce_method = ce->variant->alg_cipher[id]; > + if (ce_method == CE_ID_NOTSUPP) { > + dev_info(ce->dev, > + "DEBUG: Algo of %s not supported\n", > + ce_algs[i].alg.skcipher.base.cra_name); > + ce_algs[i].ce = NULL; > + break; > + } > + id = ce_algs[i].ce_blockmode; > + ce_method = ce->variant->op_mode[id]; > + if (ce_method == CE_ID_NOTSUPP) { > + dev_info(ce->dev, "DEBUG: Blockmode of %s not supported\n", > + ce_algs[i].alg.skcipher.base.cra_name); > + ce_algs[i].ce = NULL; > + break; > + } > + dev_info(ce->dev, "DEBUG: Register %s\n", > + ce_algs[i].alg.skcipher.base.cra_name); > + err = crypto_register_skcipher(&ce_algs[i].alg.skcipher); > + if (err) { > + dev_err(ce->dev, "Fail to register %s\n", > + ce_algs[i].alg.skcipher.base.cra_name); > + ce_algs[i].ce = NULL; > + goto error_alg; > + } > + break; > + default: > + dev_err(ce->dev, "ERROR: tryed to register an unknown algo\n"); > + } > + } > + > + return 0; > +error_alg: > + i--; > + for (; i >= 0; i--) { > + switch (ce_algs[i].type) { > + case CRYPTO_ALG_TYPE_SKCIPHER: > + if (ce_algs[i].ce) > + crypto_unregister_skcipher(&ce_algs[i].alg.skcipher); > + break; > + } > + } > +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG > +error_debugfs: > + debugfs_remove_recursive(ce->dbgfs_dir); > +#endif > + i = ce->variant->maxflow; > +error_engine: > + while (i >= 0) { > + crypto_engine_exit(ce->chanlist[i].engine); > + if (ce->chanlist[i].tl) > + dma_free_coherent(ce->dev, sizeof(struct ce_task), > + ce->chanlist[i].tl, > + ce->chanlist[i].t_phy); > + i--; > + } > +error_flow: > + reset_control_assert(ce->reset); > +error_clk: > + for (i = 0; i < CE_MAX_CLOCKS; i++) > + clk_disable_unprepare(ce->ceclks[i]); > + return err; > +} So that function takes around 200-250 LoC, this is definitely too much and should be split into multiple functions. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Sat, Sep 07, 2019 at 06:54:53AM +0300, Maxime Ripard wrote: > On Fri, Sep 06, 2019 at 08:45:43PM +0200, Corentin Labbe wrote: > > Since a second Allwinner crypto driver will be added, it is better to > > create a dedicated subdirectory. > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > --- > > MAINTAINERS | 6 ++++++ > > drivers/crypto/Kconfig | 2 ++ > > drivers/crypto/Makefile | 1 + > > drivers/crypto/allwinner/Kconfig | 6 ++++++ > > I guess it would make sense to move the sun4i driver there too? > Yes it is planned. I will add this move patch in the next iteration. Regards
On Sat, Sep 07, 2019 at 07:02:17AM +0300, Maxime Ripard wrote: > On Fri, Sep 06, 2019 at 08:45:46PM +0200, Corentin Labbe wrote: > > The Crypto Engine is a hardware cryptographic offloader that supports > > many algorithms. > > It could be found on most Allwinner SoCs. > > > > This patch enables the Crypto Engine on the Allwinner R40 SoC Device-tree. > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > --- > > arch/arm/boot/dts/sun8i-r40.dtsi | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi > > index bde068111b85..7eb649cea163 100644 > > --- a/arch/arm/boot/dts/sun8i-r40.dtsi > > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi > > @@ -266,6 +266,17 @@ > > #phy-cells = <1>; > > }; > > > > + crypto: crypto-engine@1c15000 { > > + compatible = "allwinner,sun8i-r40-crypto"; > > + reg = <0x01c15000 0x1000>; > > + interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&ccu CLK_BUS_CE>, <&ccu CLK_CE>; > > + clock-names = "ahb", "mod"; > > + resets = <&ccu RST_BUS_CE>; > > + reset-names = "ahb"; > > + status = "okay"; > > The driver will probe if status is not declared, so if you want it > always enabled you should simply remove status > Fixed, thanks.
On Sat, Sep 07, 2019 at 07:02:54AM +0300, Maxime Ripard wrote: > On Fri, Sep 06, 2019 at 08:45:48PM +0200, Corentin Labbe wrote: > > The Crypto Engine is a hardware cryptographic accelerator that supports > > many algorithms. > > It could be found on most Allwinner SoCs. > > > > This patch enables the Crypto Engine on the Allwinner A64 SoC Device-tree. > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > --- > > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > > index 69128a6dfc46..c9e30d462ab1 100644 > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > > @@ -487,6 +487,17 @@ > > reg = <0x1c14000 0x400>; > > }; > > > > + crypto: crypto@1c15000 { > > + compatible = "allwinner,sun50i-a64-crypto"; > > + reg = <0x01c15000 0x1000>; > > + interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-names = "ce_ns"; > > You didn't document that property > It is unnecessary, I forgot to remove it. Fixed, thanks.
On Sat, Sep 07, 2019 at 07:03:53AM +0300, Maxime Ripard wrote: > On Fri, Sep 06, 2019 at 08:45:51PM +0200, Corentin Labbe wrote: > > This patch adds the new allwinner crypto configs to sunxi_defconfig > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > --- > > arch/arm/configs/sunxi_defconfig | 2 ++ > > 1 file changed, 2 insertions(+) > > Can you also enable it in arm64's defconfig as a module? > Will do. Regards
On Sat, Sep 07, 2019 at 11:19:51AM +0300, Maxime Ripard wrote: > Hi, > > I can't really comment on the crypto side, so my review is going to be > pretty boring. > > On Fri, Sep 06, 2019 at 08:45:44PM +0200, Corentin Labbe wrote: > > +static const struct ce_variant ce_h3_variant = { > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > + CE_ID_NOTSUPP, > > + }, > > As far as I can see, it's always the same value, so I'm not sure why > it's a parameter. > No it is not the same value. If by same value you mean "the list is the same accross variant", it will be different when I will add CTS/CTR/XTS. Note that .alg_cipher was already different on h6, since I forgot to remove the RAES. So it will be the same on PATChv2, but again il will be different after. > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > + }, > > Ditto > > > + .intreg = CE_ISR, > > Ditto > > > + .maxflow = 4, > > Ditto > Both .intreg and .maxflow are remains of sun8i-ss support. I will remove them. > > + .ce_clks = { > > + { "ahb", 200000000 }, > > This is the default IIRC, and the clock driver will ignore any clock > rate change on it anyway, so the clock rate is pretty much useless > there. > > > + { "mod", 48000000 }, > > 48MHz seems pretty slow, especially compared to the other rates you > have listed there. Is that on purpose? On H3, the value used on others SoC bring to random fail. I will add a comment. > > Also, I'm not sure what is the point of having the clocks names be > parameters there as well. It's constant across all the compatibles, > the only thing that isn't is the number of clocks and the module clock > rate. It's what you should have in there. > Since the datasheet give some max frequency, I think I will add a max_freq and add a check to verify if the clock is in the right range > > + } > > +}; > > + > > +static const struct ce_variant ce_h5_variant = { > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > + CE_ID_NOTSUPP, > > + }, > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > + }, > > + .intreg = CE_ISR, > > + .maxflow = 4, > > + .ce_clks = { > > + { "ahb", 200000000 }, > > + { "mod", 300000000 }, > > + } > > +}; > > + > > +static const struct ce_variant ce_h6_variant = { > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > + CE_ALG_RAES, > > + }, > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > + }, > > + .model = CE_v2, > > Can't that be derived from the version register and / or the > compatible? This seems to be redundant with each. > I could use the compatible, but I want to avoid a string comparison on each request. > > + .intreg = CE_ISR, > > + .maxflow = 4, > > + .ce_clks = { > > + { "ahb", 200000000 }, > > + { "mod", 300000000 }, > > + { "mbus", 400000000 }, > > That rate is going to be ignored as well. > > > + } > > +}; > > + > > +static const struct ce_variant ce_a64_variant = { > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > + CE_ID_NOTSUPP, > > + }, > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > + }, > > + .intreg = CE_ISR, > > + .maxflow = 4, > > + .ce_clks = { > > + { "ahb", 200000000 }, > > + { "mod", 300000000 }, > > + } > > +}; > > You should order your variants by alphabetical order. > Will do. > > +static const struct ce_variant ce_r40_variant = { > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > + CE_ID_NOTSUPP, > > + }, > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > + }, > > + .intreg = CE_ISR, > > + .maxflow = 4, > > + .ce_clks = { > > + { "ahb", 200000000 }, > > + { "mod", 300000000 }, > > + } > > +}; > > + > > ... > > > +int sun8i_ce_get_engine_number(struct sun8i_ce_dev *ce) > > +{ > > + return atomic_inc_return(&ce->flow) % ce->variant->maxflow; > > +} > > I'm not sure what this is supposed to be doing, but that mod there > seems pretty dangerous. > > ... > This mod do a round robin on each channel. I dont see why it is dangerous. > > +static int sun8i_ce_probe(struct platform_device *pdev) > > +{ > > + struct resource *res; > > + u32 v; > > + int err, i, ce_method, id, irq; > > + unsigned long cr; > > + struct sun8i_ce_dev *ce; > > + > > + if (!pdev->dev.of_node) > > + return -ENODEV; > > This is pretty much guaranteed already > Ok, removed > > + ce = devm_kzalloc(&pdev->dev, sizeof(*ce), GFP_KERNEL); > > + if (!ce) > > + return -ENOMEM; > > + > > + ce->variant = of_device_get_match_data(&pdev->dev); > > + if (!ce->variant) { > > + dev_err(&pdev->dev, "Missing Crypto Engine variant\n"); > > + return -EINVAL; > > + } > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + ce->base = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(ce->base)) { > > + err = PTR_ERR(ce->base); > > + dev_err(&pdev->dev, "Cannot request MMIO err=%d\n", err); > > + return err; > > ioremap_resource already prints an error message on failure, so this > is redundant. > Will remove. > > + } > > + > > + for (i = 0; i < CE_MAX_CLOCKS; i++) { > > + if (!ce->variant->ce_clks[i].name) > > + continue; > > + dev_info(&pdev->dev, "Get %s clock\n", ce->variant->ce_clks[i].name); > > There's no reason to print this at the info level > Will remove. > > + ce->ceclks[i] = devm_clk_get(&pdev->dev, ce->variant->ce_clks[i].name); > > + if (IS_ERR(ce->ceclks[i])) { > > + err = PTR_ERR(ce->ceclks[i]); > > + dev_err(&pdev->dev, "Cannot get %s CE clock err=%d\n", > > + ce->variant->ce_clks[i].name, err); > > + } > > + cr = clk_get_rate(ce->ceclks[i]); > > So on error you'd call clk_get_rate on the clock still? That seems > pretty fragile, you should return there, it's a hard error. > I will add the missing "return err" > > + if (ce->variant->ce_clks[i].freq) { > > + dev_info(&pdev->dev, "Set %s clock to %lu (%lu Mhz) from %lu (%lu Mhz)\n", > > + ce->variant->ce_clks[i].name, > > + ce->variant->ce_clks[i].freq, > > + ce->variant->ce_clks[i].freq / 1000000, > > + cr, > > + cr / 1000000); > > Same remark about that message than the previous one. > > > + err = clk_set_rate(ce->ceclks[i], ce->variant->ce_clks[i].freq); > > + if (err) > > + dev_err(&pdev->dev, "Fail to set %s clk speed to %lu\n", > > + ce->variant->ce_clks[i].name, > > + ce->variant->ce_clks[i].freq); > > + } else { > > + dev_info(&pdev->dev, "%s run at %lu\n", > > + ce->variant->ce_clks[i].name, cr); > > Ditto. > > > + } > > + err = clk_prepare_enable(ce->ceclks[i]); > > Do you really need this right now though? > Not sure to understand, why I shouldnt do it now ? Does it is related to your pm_runtime remark below ? My feeling was to submit the driver without PM and convert it after. > > + if (err) { > > + dev_err(&pdev->dev, "Cannot prepare_enable %s\n", > > + ce->variant->ce_clks[i].name); > > + return err; > > + } > > + } > > + > > + /* Get Non Secure IRQ */ > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) { > > + dev_err(ce->dev, "Cannot get NS IRQ\n"); > > + return irq; > > + } > > + > > + err = devm_request_irq(&pdev->dev, irq, ce_irq_handler, 0, > > + "sun8i-ce-ns", ce); > > + if (err < 0) { > > + dev_err(ce->dev, "Cannot request NS IRQ\n"); > > + return err; > > + } > > + > > + ce->reset = devm_reset_control_get_optional(&pdev->dev, "ahb"); > > + if (IS_ERR(ce->reset)) { > > + if (PTR_ERR(ce->reset) == -EPROBE_DEFER) > > + return PTR_ERR(ce->reset); > > + dev_info(&pdev->dev, "No reset control found\n"); > > It's not optional though. > I dont understand why. > > + ce->reset = NULL; > > + } > > + > > + err = reset_control_deassert(ce->reset); > > + if (err) { > > + dev_err(&pdev->dev, "Cannot deassert reset control\n"); > > + goto error_clk; > > + } > > Again, you don't really need this at this moment. Using runtime_pm > would make more sense. > > > + v = readl(ce->base + CE_CTR); > > + v >>= 16; > > + v &= 0x07; > > This should be in a define > Will fix. > > + dev_info(&pdev->dev, "CE_NS Die ID %x\n", v); > > And if that really makes sense to print it, the error message should > be made less cryptic. > Will fix. > > + > > + ce->dev = &pdev->dev; > > + platform_set_drvdata(pdev, ce); > > + > > + mutex_init(&ce->mlock); > > + > > + ce->chanlist = devm_kcalloc(ce->dev, ce->variant->maxflow, > > + sizeof(struct sun8i_ce_flow), GFP_KERNEL); > > + if (!ce->chanlist) { > > + err = -ENOMEM; > > + goto error_flow; > > + } > > + > > + for (i = 0; i < ce->variant->maxflow; i++) { > > + init_completion(&ce->chanlist[i].complete); > > + mutex_init(&ce->chanlist[i].lock); > > + > > + ce->chanlist[i].engine = crypto_engine_alloc_init(ce->dev, true); > > + if (!ce->chanlist[i].engine) { > > + dev_err(ce->dev, "Cannot allocate engine\n"); > > + i--; > > + goto error_engine; > > + } > > + err = crypto_engine_start(ce->chanlist[i].engine); > > + if (err) { > > + dev_err(ce->dev, "Cannot start engine\n"); > > + goto error_engine; > > + } > > + ce->chanlist[i].tl = dma_alloc_coherent(ce->dev, > > + sizeof(struct ce_task), > > + &ce->chanlist[i].t_phy, > > + GFP_KERNEL); > > + if (!ce->chanlist[i].tl) { > > + dev_err(ce->dev, "Cannot get DMA memory for task %d\n", > > + i); > > + err = -ENOMEM; > > + goto error_engine; > > + } > > + } > > All this initialization should be done before calling > request_irq. You're using some of those fields in your handler. > No interrupt could fire, since algorithms are still not registred. > > + > > +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG > > + ce->dbgfs_dir = debugfs_create_dir("sun8i-ce", NULL); > > + if (IS_ERR_OR_NULL(ce->dbgfs_dir)) { > > + dev_err(ce->dev, "Fail to create debugfs dir"); > > + err = -ENOMEM; > > + goto error_engine; > > + } > > + ce->dbgfs_stats = debugfs_create_file("stats", 0444, > > + ce->dbgfs_dir, ce, > > + &sun8i_ce_debugfs_fops); > > + if (IS_ERR_OR_NULL(ce->dbgfs_stats)) { > > + dev_err(ce->dev, "Fail to create debugfs stat"); > > + err = -ENOMEM; > > + goto error_debugfs; > > + } > > +#endif > > + for (i = 0; i < ARRAY_SIZE(ce_algs); i++) { > > + ce_algs[i].ce = ce; > > + switch (ce_algs[i].type) { > > + case CRYPTO_ALG_TYPE_SKCIPHER: > > + id = ce_algs[i].ce_algo_id; > > + ce_method = ce->variant->alg_cipher[id]; > > + if (ce_method == CE_ID_NOTSUPP) { > > + dev_info(ce->dev, > > + "DEBUG: Algo of %s not supported\n", > > + ce_algs[i].alg.skcipher.base.cra_name); > > + ce_algs[i].ce = NULL; > > + break; > > + } > > + id = ce_algs[i].ce_blockmode; > > + ce_method = ce->variant->op_mode[id]; > > + if (ce_method == CE_ID_NOTSUPP) { > > + dev_info(ce->dev, "DEBUG: Blockmode of %s not supported\n", > > + ce_algs[i].alg.skcipher.base.cra_name); > > + ce_algs[i].ce = NULL; > > + break; > > + } > > + dev_info(ce->dev, "DEBUG: Register %s\n", > > + ce_algs[i].alg.skcipher.base.cra_name); > > + err = crypto_register_skcipher(&ce_algs[i].alg.skcipher); > > + if (err) { > > + dev_err(ce->dev, "Fail to register %s\n", > > + ce_algs[i].alg.skcipher.base.cra_name); > > + ce_algs[i].ce = NULL; > > + goto error_alg; > > + } > > + break; > > + default: > > + dev_err(ce->dev, "ERROR: tryed to register an unknown algo\n"); > > + } > > + } > > + > > + return 0; > > +error_alg: > > + i--; > > + for (; i >= 0; i--) { > > + switch (ce_algs[i].type) { > > + case CRYPTO_ALG_TYPE_SKCIPHER: > > + if (ce_algs[i].ce) > > + crypto_unregister_skcipher(&ce_algs[i].alg.skcipher); > > + break; > > + } > > + } > > +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG > > +error_debugfs: > > + debugfs_remove_recursive(ce->dbgfs_dir); > > +#endif > > + i = ce->variant->maxflow; > > +error_engine: > > + while (i >= 0) { > > + crypto_engine_exit(ce->chanlist[i].engine); > > + if (ce->chanlist[i].tl) > > + dma_free_coherent(ce->dev, sizeof(struct ce_task), > > + ce->chanlist[i].tl, > > + ce->chanlist[i].t_phy); > > + i--; > > + } > > +error_flow: > > + reset_control_assert(ce->reset); > > +error_clk: > > + for (i = 0; i < CE_MAX_CLOCKS; i++) > > + clk_disable_unprepare(ce->ceclks[i]); > > + return err; > > +} > > So that function takes around 200-250 LoC, this is definitely too > much and should be split into multiple functions. > Will do. Thanks for your review. Regards
On Sat, Sep 07, 2019 at 09:04:08PM +0200, Corentin Labbe wrote: > > Also, I'm not sure what is the point of having the clocks names be > > parameters there as well. It's constant across all the compatibles, > > the only thing that isn't is the number of clocks and the module clock > > rate. It's what you should have in there. > > Since the datasheet give some max frequency, I think I will add a > max_freq and add a check to verify if the clock is in the right > range It's a bit pointless. What are you going to do if it's not correct? What are you trying to fix / report with this? > > > + } > > > +}; > > > + > > > +static const struct ce_variant ce_h5_variant = { > > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > > + CE_ID_NOTSUPP, > > > + }, > > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > > + }, > > > + .intreg = CE_ISR, > > > + .maxflow = 4, > > > + .ce_clks = { > > > + { "ahb", 200000000 }, > > > + { "mod", 300000000 }, > > > + } > > > +}; > > > + > > > +static const struct ce_variant ce_h6_variant = { > > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > > + CE_ALG_RAES, > > > + }, > > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > > + }, > > > + .model = CE_v2, > > > > Can't that be derived from the version register and / or the > > compatible? This seems to be redundant with each. > > I could use the compatible, but I want to avoid a string comparison > on each request. Well, this is specifically what this structure is for then, right? So instead of having the model, just add the information that you want there. > > > +int sun8i_ce_get_engine_number(struct sun8i_ce_dev *ce) > > > +{ > > > + return atomic_inc_return(&ce->flow) % ce->variant->maxflow; > > > +} > > > > I'm not sure what this is supposed to be doing, but that mod there > > seems pretty dangerous. > > > > ... > > This mod do a round robin on each channel. > I dont see why it is dangerous. Well, you're using the atomic API here which is most commonly used for refcounting, while you're using a mod. Plus, while the increment is atomic, the modulo isn't, so you can end up in a case where you would be preempted between the atomic_inc_return and the mod, which is dangerous. Again, I'm not sure what this function is doing (which is also a problem in itself). I guess you should just make it clearer what it does, and then we can discuss it properly. > > > + err = clk_set_rate(ce->ceclks[i], ce->variant->ce_clks[i].freq); > > > + if (err) > > > + dev_err(&pdev->dev, "Fail to set %s clk speed to %lu\n", > > > + ce->variant->ce_clks[i].name, > > > + ce->variant->ce_clks[i].freq); > > > + } else { > > > + dev_info(&pdev->dev, "%s run at %lu\n", > > > + ce->variant->ce_clks[i].name, cr); > > > > Ditto. > > > > > + } > > > + err = clk_prepare_enable(ce->ceclks[i]); > > > > Do you really need this right now though? > > Not sure to understand, why I shouldnt do it now ? > Does it is related to your pm_runtime remark below ? > > My feeling was to submit the driver without PM and convert it after. runtime_pm would be pretty cheap to add though judging by what you're doing there. > > > + if (err) { > > > + dev_err(&pdev->dev, "Cannot prepare_enable %s\n", > > > + ce->variant->ce_clks[i].name); > > > + return err; > > > + } > > > + } > > > + > > > + /* Get Non Secure IRQ */ > > > + irq = platform_get_irq(pdev, 0); > > > + if (irq < 0) { > > > + dev_err(ce->dev, "Cannot get NS IRQ\n"); > > > + return irq; > > > + } > > > + > > > + err = devm_request_irq(&pdev->dev, irq, ce_irq_handler, 0, > > > + "sun8i-ce-ns", ce); > > > + if (err < 0) { > > > + dev_err(ce->dev, "Cannot request NS IRQ\n"); > > > + return err; > > > + } > > > + > > > + ce->reset = devm_reset_control_get_optional(&pdev->dev, "ahb"); > > > + if (IS_ERR(ce->reset)) { > > > + if (PTR_ERR(ce->reset) == -EPROBE_DEFER) > > > + return PTR_ERR(ce->reset); > > > + dev_info(&pdev->dev, "No reset control found\n"); > > > > It's not optional though. > > I dont understand why. On all the SoCs, you need that reset line to be deasserted, otherwise the IP (and therefore the driver) will be non-functional. It's not an option to run without it. > > > + ce->reset = NULL; > > > + } > > > + > > > + err = reset_control_deassert(ce->reset); > > > + if (err) { > > > + dev_err(&pdev->dev, "Cannot deassert reset control\n"); > > > + goto error_clk; > > > + } > > > > Again, you don't really need this at this moment. Using runtime_pm > > would make more sense. > > > > > + v = readl(ce->base + CE_CTR); > > > + v >>= 16; > > > + v &= 0x07; > > > > This should be in a define > > > > Will fix. > > > > + dev_info(&pdev->dev, "CE_NS Die ID %x\n", v); > > > > And if that really makes sense to print it, the error message should > > be made less cryptic. > > > > Will fix. > > > > + > > > + ce->dev = &pdev->dev; > > > + platform_set_drvdata(pdev, ce); > > > + > > > + mutex_init(&ce->mlock); > > > + > > > + ce->chanlist = devm_kcalloc(ce->dev, ce->variant->maxflow, > > > + sizeof(struct sun8i_ce_flow), GFP_KERNEL); > > > + if (!ce->chanlist) { > > > + err = -ENOMEM; > > > + goto error_flow; > > > + } > > > + > > > + for (i = 0; i < ce->variant->maxflow; i++) { > > > + init_completion(&ce->chanlist[i].complete); > > > + mutex_init(&ce->chanlist[i].lock); > > > + > > > + ce->chanlist[i].engine = crypto_engine_alloc_init(ce->dev, true); > > > + if (!ce->chanlist[i].engine) { > > > + dev_err(ce->dev, "Cannot allocate engine\n"); > > > + i--; > > > + goto error_engine; > > > + } > > > + err = crypto_engine_start(ce->chanlist[i].engine); > > > + if (err) { > > > + dev_err(ce->dev, "Cannot start engine\n"); > > > + goto error_engine; > > > + } > > > + ce->chanlist[i].tl = dma_alloc_coherent(ce->dev, > > > + sizeof(struct ce_task), > > > + &ce->chanlist[i].t_phy, > > > + GFP_KERNEL); > > > + if (!ce->chanlist[i].tl) { > > > + dev_err(ce->dev, "Cannot get DMA memory for task %d\n", > > > + i); > > > + err = -ENOMEM; > > > + goto error_engine; > > > + } > > > + } > > > > All this initialization should be done before calling > > request_irq. You're using some of those fields in your handler. > > No interrupt could fire, since algorithms are still not registred. That's not true. Spurious interrupts are a thing, the engine could have been left in a weird state by the bootloader / kexec / reboot with some pending interrupts, etc. You have registered that handler already, you should expect it to be called at any point in time. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Mon, Sep 09, 2019 at 01:38:37PM +0200, Maxime Ripard wrote: > On Sat, Sep 07, 2019 at 09:04:08PM +0200, Corentin Labbe wrote: > > > Also, I'm not sure what is the point of having the clocks names be > > > parameters there as well. It's constant across all the compatibles, > > > the only thing that isn't is the number of clocks and the module clock > > > rate. It's what you should have in there. > > > > Since the datasheet give some max frequency, I think I will add a > > max_freq and add a check to verify if the clock is in the right > > range > > It's a bit pointless. What are you going to do if it's not correct? > What are you trying to fix / report with this? I thinked to print a warning. If someone want to play with overclocking for example, the driver should said that probably some result could be invalid. > > > > > + } > > > > +}; > > > > + > > > > +static const struct ce_variant ce_h5_variant = { > > > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > > > + CE_ID_NOTSUPP, > > > > + }, > > > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > > > + }, > > > > + .intreg = CE_ISR, > > > > + .maxflow = 4, > > > > + .ce_clks = { > > > > + { "ahb", 200000000 }, > > > > + { "mod", 300000000 }, > > > > + } > > > > +}; > > > > + > > > > +static const struct ce_variant ce_h6_variant = { > > > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > > > + CE_ALG_RAES, > > > > + }, > > > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > > > + }, > > > > + .model = CE_v2, > > > > > > Can't that be derived from the version register and / or the > > > compatible? This seems to be redundant with each. > > > > I could use the compatible, but I want to avoid a string comparison > > on each request. > > Well, this is specifically what this structure is for then, right? So > instead of having the model, just add the information that you want > there. > ok, I will change to a "bool all_size_in_bytes" > > > > +int sun8i_ce_get_engine_number(struct sun8i_ce_dev *ce) > > > > +{ > > > > + return atomic_inc_return(&ce->flow) % ce->variant->maxflow; > > > > +} > > > > > > I'm not sure what this is supposed to be doing, but that mod there > > > seems pretty dangerous. > > > > > > ... > > > > This mod do a round robin on each channel. > > I dont see why it is dangerous. > > Well, you're using the atomic API here which is most commonly used for > refcounting, while you're using a mod. > > Plus, while the increment is atomic, the modulo isn't, so you can end > up in a case where you would be preempted between the > atomic_inc_return and the mod, which is dangerous. > > Again, I'm not sure what this function is doing (which is also a > problem in itself). I guess you should just make it clearer what it > does, and then we can discuss it properly. Each request need to be assigned to a channel. Each channel are identified by a number from 1 to 4. So this function return the channel to use, 1 then 2 then 3 then 4 then 1... Note that this is uncritical. If, due to anything, two request are assigned to the same channel, nothing will break. > > > > > + err = clk_set_rate(ce->ceclks[i], ce->variant->ce_clks[i].freq); > > > > + if (err) > > > > + dev_err(&pdev->dev, "Fail to set %s clk speed to %lu\n", > > > > + ce->variant->ce_clks[i].name, > > > > + ce->variant->ce_clks[i].freq); > > > > + } else { > > > > + dev_info(&pdev->dev, "%s run at %lu\n", > > > > + ce->variant->ce_clks[i].name, cr); > > > > > > Ditto. > > > > > > > + } > > > > + err = clk_prepare_enable(ce->ceclks[i]); > > > > > > Do you really need this right now though? > > > > Not sure to understand, why I shouldnt do it now ? > > Does it is related to your pm_runtime remark below ? > > > > My feeling was to submit the driver without PM and convert it after. > > runtime_pm would be pretty cheap to add though judging by what you're > doing there. > I will try to add runtime_pm > > > > + if (err) { > > > > + dev_err(&pdev->dev, "Cannot prepare_enable %s\n", > > > > + ce->variant->ce_clks[i].name); > > > > + return err; > > > > + } > > > > + } > > > > + > > > > + /* Get Non Secure IRQ */ > > > > + irq = platform_get_irq(pdev, 0); > > > > + if (irq < 0) { > > > > + dev_err(ce->dev, "Cannot get NS IRQ\n"); > > > > + return irq; > > > > + } > > > > + > > > > + err = devm_request_irq(&pdev->dev, irq, ce_irq_handler, 0, > > > > + "sun8i-ce-ns", ce); > > > > + if (err < 0) { > > > > + dev_err(ce->dev, "Cannot request NS IRQ\n"); > > > > + return err; > > > > + } > > > > + > > > > + ce->reset = devm_reset_control_get_optional(&pdev->dev, "ahb"); > > > > + if (IS_ERR(ce->reset)) { > > > > + if (PTR_ERR(ce->reset) == -EPROBE_DEFER) > > > > + return PTR_ERR(ce->reset); > > > > + dev_info(&pdev->dev, "No reset control found\n"); > > > > > > It's not optional though. > > > > I dont understand why. > > On all the SoCs, you need that reset line to be deasserted, otherwise > the IP (and therefore the driver) will be non-functional. It's not an > option to run without it. Currently all the SoC have a reset, but nothing prevent a new SoC with CE without reset. Anyway, I will made the reset mandatory for the moment. > > > > > + ce->reset = NULL; > > > > + } > > > > + > > > > + err = reset_control_deassert(ce->reset); > > > > + if (err) { > > > > + dev_err(&pdev->dev, "Cannot deassert reset control\n"); > > > > + goto error_clk; > > > > + } > > > > > > Again, you don't really need this at this moment. Using runtime_pm > > > would make more sense. > > > > > > > + v = readl(ce->base + CE_CTR); > > > > + v >>= 16; > > > > + v &= 0x07; > > > > > > This should be in a define > > > > > > > Will fix. > > > > > > + dev_info(&pdev->dev, "CE_NS Die ID %x\n", v); > > > > > > And if that really makes sense to print it, the error message should > > > be made less cryptic. > > > > > > > Will fix. > > > > > > + > > > > + ce->dev = &pdev->dev; > > > > + platform_set_drvdata(pdev, ce); > > > > + > > > > + mutex_init(&ce->mlock); > > > > + > > > > + ce->chanlist = devm_kcalloc(ce->dev, ce->variant->maxflow, > > > > + sizeof(struct sun8i_ce_flow), GFP_KERNEL); > > > > + if (!ce->chanlist) { > > > > + err = -ENOMEM; > > > > + goto error_flow; > > > > + } > > > > + > > > > + for (i = 0; i < ce->variant->maxflow; i++) { > > > > + init_completion(&ce->chanlist[i].complete); > > > > + mutex_init(&ce->chanlist[i].lock); > > > > + > > > > + ce->chanlist[i].engine = crypto_engine_alloc_init(ce->dev, true); > > > > + if (!ce->chanlist[i].engine) { > > > > + dev_err(ce->dev, "Cannot allocate engine\n"); > > > > + i--; > > > > + goto error_engine; > > > > + } > > > > + err = crypto_engine_start(ce->chanlist[i].engine); > > > > + if (err) { > > > > + dev_err(ce->dev, "Cannot start engine\n"); > > > > + goto error_engine; > > > > + } > > > > + ce->chanlist[i].tl = dma_alloc_coherent(ce->dev, > > > > + sizeof(struct ce_task), > > > > + &ce->chanlist[i].t_phy, > > > > + GFP_KERNEL); > > > > + if (!ce->chanlist[i].tl) { > > > > + dev_err(ce->dev, "Cannot get DMA memory for task %d\n", > > > > + i); > > > > + err = -ENOMEM; > > > > + goto error_engine; > > > > + } > > > > + } > > > > > > All this initialization should be done before calling > > > request_irq. You're using some of those fields in your handler. > > > > No interrupt could fire, since algorithms are still not registred. > > That's not true. Spurious interrupts are a thing, the engine could > have been left in a weird state by the bootloader / kexec / reboot > with some pending interrupts, etc. > > You have registered that handler already, you should expect it to be > called at any point in time. > Ok will fix. Thanks for your review.
On Mon, Sep 09, 2019 at 03:19:06PM +0200, Corentin Labbe wrote: > On Mon, Sep 09, 2019 at 01:38:37PM +0200, Maxime Ripard wrote: > > On Sat, Sep 07, 2019 at 09:04:08PM +0200, Corentin Labbe wrote: > > > > Also, I'm not sure what is the point of having the clocks names be > > > > parameters there as well. It's constant across all the compatibles, > > > > the only thing that isn't is the number of clocks and the module clock > > > > rate. It's what you should have in there. > > > > > > Since the datasheet give some max frequency, I think I will add a > > > max_freq and add a check to verify if the clock is in the right > > > range > > > > It's a bit pointless. What are you going to do if it's not correct? > > What are you trying to fix / report with this? > > I thinked to print a warning. If someone want to play with > overclocking for example, the driver should said that probably some > result could be invalid. If someone wants to play with overclocking, the crypto engine is going to be the least of their concern. > > > > > +int sun8i_ce_get_engine_number(struct sun8i_ce_dev *ce) > > > > > +{ > > > > > + return atomic_inc_return(&ce->flow) % ce->variant->maxflow; > > > > > +} > > > > > > > > I'm not sure what this is supposed to be doing, but that mod there > > > > seems pretty dangerous. > > > > > > > > ... > > > > > > This mod do a round robin on each channel. > > > I dont see why it is dangerous. > > > > Well, you're using the atomic API here which is most commonly used for > > refcounting, while you're using a mod. > > > > Plus, while the increment is atomic, the modulo isn't, so you can end > > up in a case where you would be preempted between the > > atomic_inc_return and the mod, which is dangerous. > > > > Again, I'm not sure what this function is doing (which is also a > > problem in itself). I guess you should just make it clearer what it > > does, and then we can discuss it properly. > > Each request need to be assigned to a channel. > Each channel are identified by a number from 1 to 4. > > So this function return the channel to use, 1 then 2 then 3 then 4 then 1... > > Note that this is uncritical. If, due to anything, two request are > assigned to the same channel, nothing will break. I'm not sure why you're using the atomic API then? Also, I guess a bitfield and find_first_bit (and a different function name) would be more obvious to the reader. Thanks! Maxime
On Sat, Sep 07, 2019 at 07:03:53AM +0300, Maxime Ripard wrote: > On Fri, Sep 06, 2019 at 08:45:51PM +0200, Corentin Labbe wrote: > > This patch adds the new allwinner crypto configs to sunxi_defconfig > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > --- > > arch/arm/configs/sunxi_defconfig | 2 ++ > > 1 file changed, 2 insertions(+) > > Can you also enable it in arm64's defconfig as a module? > Does you prefer adding a Kconfig "DEFAULT m if ARCH_SUNXI" which permit to not touch any defconfig ?
On Fri, Sep 13, 2019 at 10:15:55AM +0200, Corentin Labbe wrote: > On Sat, Sep 07, 2019 at 07:03:53AM +0300, Maxime Ripard wrote: > > On Fri, Sep 06, 2019 at 08:45:51PM +0200, Corentin Labbe wrote: > > > This patch adds the new allwinner crypto configs to sunxi_defconfig > > > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > > --- > > > arch/arm/configs/sunxi_defconfig | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > Can you also enable it in arm64's defconfig as a module? > > > > Does you prefer adding a Kconfig "DEFAULT m if ARCH_SUNXI" which > permit to not touch any defconfig ? It's not the preferred solution, unfortunately Maxime
Hello This patch serie adds support for the Allwinner crypto engine. The Crypto Engine is the third generation of Allwinner cryptogaphic offloader. The first generation is the Security System already handled by the sun4i-ss driver. The second is named also Security System and is present on A80 and A83T SoCs, originaly this driver supported it also, but supporting both IP bringing too much complexity and another driver (sun8i-ss) will came for it. For the moment, the driver support only DES3/AES in ECB/CBC mode. Patchs for CTR/CTS/XTS and RNGs will came later. Regards Corentin Labbe (9): crypto: Add allwinner subdirectory crypto: Add Allwinner sun8i-ce Crypto Engine dt-bindings: crypto: Add DT bindings documentation for sun8i-ce Crypto Engine ARM: dts: sun8i: r40: add crypto engine node ARM: dts: sun8i: h3: Add Crypto Engine node ARM64: dts: allwinner: sun50i: Add Crypto Engine node on A64 ARM64: dts: allwinner: sun50i: Add crypto engine node on H5 ARM64: dts: allwinner: sun50i: Add Crypto Engine node on H6 sunxi_defconfig: add new crypto options .../bindings/crypto/allwinner,sun8i-ce.yaml | 84 +++ MAINTAINERS | 6 + arch/arm/boot/dts/sun8i-h3.dtsi | 11 + arch/arm/boot/dts/sun8i-r40.dtsi | 11 + arch/arm/configs/sunxi_defconfig | 2 + arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 11 + arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi | 11 + arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 10 + drivers/crypto/Kconfig | 2 + drivers/crypto/Makefile | 1 + drivers/crypto/allwinner/Kconfig | 32 + drivers/crypto/allwinner/Makefile | 1 + drivers/crypto/allwinner/sun8i-ce/Makefile | 2 + .../allwinner/sun8i-ce/sun8i-ce-cipher.c | 390 +++++++++++ .../crypto/allwinner/sun8i-ce/sun8i-ce-core.c | 630 ++++++++++++++++++ drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h | 256 +++++++ 16 files changed, 1460 insertions(+) create mode 100644 Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml create mode 100644 drivers/crypto/allwinner/Kconfig create mode 100644 drivers/crypto/allwinner/Makefile create mode 100644 drivers/crypto/allwinner/sun8i-ce/Makefile create mode 100644 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c create mode 100644 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c create mode 100644 drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h