Re: [PATCH 1/6] crypto: starfive - Add StarFive crypto engine support

From: Krzysztof Kozlowski
Date: Wed Nov 30 2022 - 08:15:50 EST

Next message: Petr Mladek: "Re: [PATCH] module: Don't wait for GOING modules"
Previous message: Michal Hocko: "Re: [PATCH] mm: memcontrol: protect the memory in cgroup from being oom killed"
In reply to: Jia Jie Ho: "[PATCH 1/6] crypto: starfive - Add StarFive crypto engine support"
Next in thread: Jia Jie Ho: "[PATCH 3/6] crypto: starfive - Add AES skcipher and aead support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 30/11/2022 06:52, Jia Jie Ho wrote:
> Adding device probe and DMA init for StarFive
> hardware crypto engine.
>
> Signed-off-by: Jia Jie Ho <jiajie.ho@xxxxxxxxxxxxxxxx>
> Signed-off-by: Huan Feng <huan.feng@xxxxxxxxxxxxxxxx>
> ---

> +
> +static const struct of_device_id starfive_dt_ids[] = {
> + { .compatible = "starfive,jh7110-crypto", .data = NULL},
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, starfive_dt_ids);

Keep your table close to its usage, just like in many other drivers.

> +
> +static int starfive_dma_init(struct starfive_sec_dev *sdev)
> +{
> + dma_cap_mask_t mask;
> + int err;
> +
> + sdev->sec_xm_m = NULL;
> + sdev->sec_xm_p = NULL;
> +
> + dma_cap_zero(mask);
> + dma_cap_set(DMA_SLAVE, mask);
> +
> + sdev->sec_xm_m = dma_request_chan(sdev->dev, "sec_m");
> + if (IS_ERR(sdev->sec_xm_m)) {
> + dev_err(sdev->dev, "sec_m dma channel request failed.\n");

return dev_err_probe

> + return PTR_ERR(sdev->sec_xm_m);
> + }
> +
> + sdev->sec_xm_p = dma_request_chan(sdev->dev, "sec_p");
> + if (IS_ERR(sdev->sec_xm_p)) {
> + dev_err(sdev->dev, "sec_p dma channel request failed.\n");

dev_err_probe

> + goto err_dma_out;
> + }
> +
> + init_completion(&sdev->sec_comp_m);
> + init_completion(&sdev->sec_comp_p);
> +
> + return 0;
> +
> +err_dma_out:
> + dma_release_channel(sdev->sec_xm_m);

I don't think you tested it. Not even built with proper tools liek
smatch, sparse, W=1. Where do you set err?

> +
> + return err;
> +}
> +
> +static void starfive_dma_cleanup(struct starfive_sec_dev *sdev)
> +{
> + dma_release_channel(sdev->sec_xm_p);
> + dma_release_channel(sdev->sec_xm_m);
> +}
> +
> +static int starfive_cryp_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct starfive_sec_dev *sdev;
> + struct resource *res;
> + int pages = 0;
> + int ret;
> +
> + sdev = devm_kzalloc(dev, sizeof(*sdev), GFP_KERNEL);
> + if (!sdev)
> + return -ENOMEM;
> +
> + sdev->dev = dev;
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "secreg");
> + sdev->io_base = devm_ioremap_resource(dev, res);

I think there is a wrapper for both calls...

> +
> + if (IS_ERR(sdev->io_base))
> + return PTR_ERR(sdev->io_base);
> +
> + sdev->use_side_channel_mitigation =
> + device_property_read_bool(dev, "enable-side-channel-mitigation");
> + sdev->use_dma = device_property_read_bool(dev, "enable-dma");
> + sdev->dma_maxburst = 32;
> +
> + sdev->sec_hclk = devm_clk_get(dev, "sec_hclk");
> + if (IS_ERR(sdev->sec_hclk)) {
> + dev_err(dev, "Failed to get sec_hclk.\n");
> + return PTR_ERR(sdev->sec_hclk);

return dev_err_probe

> + }
> +
> + sdev->sec_ahb = devm_clk_get(dev, "sec_ahb");
> + if (IS_ERR(sdev->sec_ahb)) {
> + dev_err(dev, "Failed to get sec_ahb.\n");
> + return PTR_ERR(sdev->sec_ahb);

return dev_err_probe

> + }
> +
> + sdev->rst_hresetn = devm_reset_control_get_shared(sdev->dev, "sec_hre");
> + if (IS_ERR(sdev->rst_hresetn)) {
> + dev_err(sdev->dev, "Failed to get sec_hre.\n");
> + return PTR_ERR(sdev->rst_hresetn);

return dev_err_probe

> + }
> +
> + clk_prepare_enable(sdev->sec_hclk);
> + clk_prepare_enable(sdev->sec_ahb);
> + reset_control_deassert(sdev->rst_hresetn);
> +
> + platform_set_drvdata(pdev, sdev);
> +
> + spin_lock(&dev_list.lock);
> + list_add(&sdev->list, &dev_list.dev_list);
> + spin_unlock(&dev_list.lock);
> +
> + if (sdev->use_dma) {
> + ret = starfive_dma_init(sdev);
> + if (ret) {
> + dev_err(dev, "Failed to initialize DMA channel.\n");
> + goto err_dma_init;
> + }
> + }
> +
> + pages = get_order(STARFIVE_MSG_BUFFER_SIZE);
> +
> + sdev->pages_count = pages >> 1;
> + sdev->data_buf_len = STARFIVE_MSG_BUFFER_SIZE >> 1;
> +
> + /* Initialize crypto engine */
> + sdev->engine = crypto_engine_alloc_init(dev, 1);
> + if (!sdev->engine) {
> + ret = -ENOMEM;
> + goto err_engine;
> + }
> +
> + ret = crypto_engine_start(sdev->engine);
> + if (ret)
> + goto err_engine_start;
> +
> + dev_info(dev, "Crypto engine started\n");

Drop silly probe success messages.

> +
> + return 0;
> +
> +err_engine_start:
> + crypto_engine_exit(sdev->engine);
> +err_engine:
> + starfive_dma_cleanup(sdev);
> +err_dma_init:
> + spin_lock(&dev_list.lock);
> + list_del(&sdev->list);
> + spin_unlock(&dev_list.lock);
> +
> + return ret;
> +}
> +
> +static int starfive_cryp_remove(struct platform_device *pdev)
> +{
> + struct starfive_sec_dev *sdev = platform_get_drvdata(pdev);
> +
> + if (!sdev)
> + return -ENODEV;
> +
> + crypto_engine_stop(sdev->engine);
> + crypto_engine_exit(sdev->engine);
> +
> + starfive_dma_cleanup(sdev);
> +
> + spin_lock(&dev_list.lock);
> + list_del(&sdev->list);
> + spin_unlock(&dev_list.lock);
> +
> + clk_disable_unprepare(sdev->sec_hclk);
> + clk_disable_unprepare(sdev->sec_ahb);
> + reset_control_assert(sdev->rst_hresetn);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int starfive_cryp_runtime_suspend(struct device *dev)
> +{
> + struct starfive_sec_dev *sdev = dev_get_drvdata(dev);
> +
> + clk_disable_unprepare(sdev->sec_ahb);
> + clk_disable_unprepare(sdev->sec_hclk);
> +
> + return 0;
> +}
> +
> +static int starfive_cryp_runtime_resume(struct device *dev)
> +{
> + struct starfive_sec_dev *sdev = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = clk_prepare_enable(sdev->sec_ahb);
> + if (ret) {
> + dev_err(sdev->dev, "Failed to prepare_enable sec_ahb clock\n");
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(sdev->sec_hclk);
> + if (ret) {
> + dev_err(sdev->dev, "Failed to prepare_enable sec_hclk clock\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops starfive_cryp_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> + pm_runtime_force_resume)
> + SET_RUNTIME_PM_OPS(starfive_cryp_runtime_suspend,
> + starfive_cryp_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver starfive_cryp_driver = {
> + .probe = starfive_cryp_probe,
> + .remove = starfive_cryp_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + .pm = &starfive_cryp_pm_ops,
> + .of_match_table = starfive_dt_ids,
> + },
> +};
> +
> +module_platform_driver(starfive_cryp_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("StarFive hardware crypto acceleration");
> diff --git a/drivers/crypto/starfive/starfive-regs.h b/drivers/crypto/starfive/starfive-regs.h
> new file mode 100644
> index 000000000000..0d680cb1f502
> --- /dev/null
> +++ b/drivers/crypto/starfive/starfive-regs.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __STARFIVE_REGS_H__
> +#define __STARFIVE_REGS_H__
> +
> +#define STARFIVE_ALG_CR_OFFSET 0x0
> +#define STARFIVE_ALG_FIFO_OFFSET 0x4
> +#define STARFIVE_IE_MASK_OFFSET 0x8
> +#define STARFIVE_IE_FLAG_OFFSET 0xc
> +#define STARFIVE_DMA_IN_LEN_OFFSET 0x10
> +#define STARFIVE_DMA_OUT_LEN_OFFSET 0x14
> +
> +union starfive_alg_cr {
> + u32 v;
> + struct {
> + u32 start :1;
> + u32 aes_dma_en :1;
> + u32 rsvd_0 :1;
> + u32 hash_dma_en :1;
> + u32 alg_done :1;
> + u32 rsvd_1 :3;
> + u32 clear :1;
> + u32 rsvd_2 :23;
> + };
> +};
> +
> +#endif
> diff --git a/drivers/crypto/starfive/starfive-str.h b/drivers/crypto/starfive/starfive-str.h
> new file mode 100644
> index 000000000000..4ba3c56f0573
> --- /dev/null
> +++ b/drivers/crypto/starfive/starfive-str.h
> @@ -0,0 +1,74 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __STARFIVE_STR_H__
> +#define __STARFIVE_STR_H__
> +
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +
> +#include <crypto/engine.h>
> +
> +#include "starfive-regs.h"
> +
> +#define STARFIVE_MSG_BUFFER_SIZE SZ_16K
> +
> +struct starfive_sec_ctx {
> + struct crypto_engine_ctx enginectx;
> + struct starfive_sec_dev *sdev;
> +
> + u8 *buffer;
> +};
> +
> +struct starfive_sec_dev {
> + struct list_head list;
> + struct device *dev;
> +
> + struct clk *sec_hclk;
> + struct clk *sec_ahb;
> + struct reset_control *rst_hresetn;
> +
> + void __iomem *io_base;
> + phys_addr_t io_phys_base;
> +
> + size_t data_buf_len;
> + int pages_count;
> + u32 use_side_channel_mitigation;
> + u32 use_dma;
> + u32 dma_maxburst;
> + struct dma_chan *sec_xm_m;
> + struct dma_chan *sec_xm_p;
> + struct dma_slave_config cfg_in;
> + struct dma_slave_config cfg_out;
> + struct completion sec_comp_m;
> + struct completion sec_comp_p;
> +
> + struct crypto_engine *engine;
> +
> + union starfive_alg_cr alg_cr;
> +};
> +
> +static inline u32 starfive_sec_read(struct starfive_sec_dev *sdev, u32 offset)
> +{
> + return __raw_readl(sdev->io_base + offset);

I don't think these read/write wrappers help anyhow...

Best regards,
Krzysztof

Next message: Petr Mladek: "Re: [PATCH] module: Don't wait for GOING modules"
Previous message: Michal Hocko: "Re: [PATCH] mm: memcontrol: protect the memory in cgroup from being oom killed"
In reply to: Jia Jie Ho: "[PATCH 1/6] crypto: starfive - Add StarFive crypto engine support"
Next in thread: Jia Jie Ho: "[PATCH 3/6] crypto: starfive - Add AES skcipher and aead support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]