| Message ID | 20231213122030.11734-1-akhilrajeev@nvidia.com |
|---|---|
| Headers | show |
| Series | Add Tegra Security Engine driver | expand |
On 13/12/2023 13:20, Akhil R wrote: > Add support for Tegra Security Engine which can accelerates various > crypto algorithms. The Engine has two separate instances within for > AES and HASH algorithms respectively. > > The driver registers two crypto engines - one for AES and another for > HASH algorithms and these operate independently and both uses the host1x > bus. Additionally, it provides hardware-assisted key protection for up > to 15 symmetric keys which it can use for the cipher operations. > > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > --- ... > + > +int tegra_init_hash(struct tegra_se *se) > +{ > + struct ahash_engine_alg *alg; > + int i, ret; > + > + se->manifest = tegra_hash_kac_manifest; > + > + for (i = 0; i < ARRAY_SIZE(tegra_hash_algs); i++) { > + tegra_hash_algs[i].se_dev = se; > + alg = &tegra_hash_algs[i].alg.ahash; > + > + ret = crypto_engine_register_ahash(alg); > + if (ret) { > + dev_err(se->dev, "failed to register %s\n", > + alg->base.halg.base.cra_name); > + goto sha_err; > + } > + } > + > + dev_info(se->dev, "registered HASH algorithms\n"); Drop, not needed. Actually drop simple success messages. Drivers do not spam dmesg without need. ... > + > +int tegra_se_host1x_register(struct tegra_se *se) > +{ > + INIT_LIST_HEAD(&se->client.list); > + se->client.dev = se->dev; > + se->client.ops = &tegra_se_client_ops; > + se->client.class = se->hw->host1x_class; > + se->client.num_syncpts = 1; > + > + host1x_client_register(&se->client); > + > + return 0; > +} > + > +static int tegra_se_clk_init(struct tegra_se *se) > +{ > + int i, ret; > + > + se->clk = devm_clk_get(se->dev, NULL); > + if (IS_ERR(se->clk)) { > + dev_err(se->dev, "failed to get clock\n"); Why do you print failures multiple times? Once here, second in probe. return dev_err_probe > + return PTR_ERR(se->clk); > + } > + > + ret = clk_set_rate(se->clk, ULONG_MAX); > + if (ret) { > + dev_err(se->dev, "failed to set %d clock rate", i); Same comments > + return ret; > + } > + > + ret = clk_prepare_enable(se->clk); > + if (ret) { > + dev_err(se->dev, "failed to enable clocks\n"); Same comments > + return ret; > + } > + > + return 0; > +} > + > +static void tegra_se_clk_deinit(struct tegra_se *se) > +{ > + clk_disable_unprepare(se->clk); Why aren't you using devm_clk_get_enabled? This looks like porting some old, out-of-tree vendor crappy driver :( > +} > + > +static int tegra_se_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct tegra_se *se; > + int ret; > + > + se = devm_kzalloc(dev, sizeof(*se), GFP_KERNEL); > + if (!se) > + return -ENOMEM; > + > + se->dev = dev; > + se->owner = TEGRA_GPSE_ID; > + se->hw = device_get_match_data(&pdev->dev); > + > + se->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(se->base)) > + return PTR_ERR(se->base); > + > + dma_set_mask_and_coherent(dev, DMA_BIT_MASK(39)); > + platform_set_drvdata(pdev, se); > + > + ret = tegra_se_clk_init(se); > + if (ret) { > + dev_err(dev, "failed to init clocks\n"); Syntax is: return dev_err_probe > + return ret; > + } > + > + if (!tegra_dev_iommu_get_stream_id(dev, &se->stream_id)) { > + dev_err(dev, "failed to get IOMMU stream ID\n"); dev_err_probe > + goto clk_deinit; > + } > + > + se_writel(se, se->stream_id, SE_STREAM_ID); > + > + se->engine = crypto_engine_alloc_init(dev, 0); > + if (!se->engine) { > + dev_err(dev, "failed to init crypto engine\n"); Really? Test your code with coccinelle. Drop. > + ret = -ENOMEM; > + goto iommu_free; > + } > + > + ret = crypto_engine_start(se->engine); > + if (ret) { > + dev_err(dev, "failed to start crypto engine\n"); dev_err_probe > + goto engine_exit; > + } > + > + ret = tegra_se_host1x_register(se); > + if (ret) { > + dev_err(dev, "failed to init host1x params\n"); dev_err_probe > + goto engine_stop; > + } > + > + return 0; > + > +engine_stop: > + crypto_engine_stop(se->engine); > +engine_exit: > + crypto_engine_exit(se->engine); > +iommu_free: > + iommu_fwspec_free(se->dev); > +clk_deinit: > + tegra_se_clk_deinit(se); > + > + return ret; > +} > + > +static int tegra_se_remove(struct platform_device *pdev) > +{ > + struct tegra_se *se = platform_get_drvdata(pdev); > + > + crypto_engine_stop(se->engine); > + crypto_engine_exit(se->engine); > + iommu_fwspec_free(se->dev); > + host1x_client_unregister(&se->client); > + tegra_se_clk_deinit(se); > + > + return 0; > +} > + > +static const struct tegra_se_regs tegra234_aes1_regs = { > + .config = SE_AES1_CFG, > + .op = SE_AES1_OPERATION, > + .last_blk = SE_AES1_LAST_BLOCK, > + .linear_ctr = SE_AES1_LINEAR_CTR, > + .aad_len = SE_AES1_AAD_LEN, > + .cryp_msg_len = SE_AES1_CRYPTO_MSG_LEN, > + .manifest = SE_AES1_KEYMANIFEST, > + .key_addr = SE_AES1_KEY_ADDR, > + .key_data = SE_AES1_KEY_DATA, > + .key_dst = SE_AES1_KEY_DST, > + .result = SE_AES1_CMAC_RESULT, > +}; > + > +static const struct tegra_se_regs tegra234_hash_regs = { > + .config = SE_SHA_CFG, > + .op = SE_SHA_OPERATION, > + .manifest = SE_SHA_KEYMANIFEST, > + .key_addr = SE_SHA_KEY_ADDR, > + .key_data = SE_SHA_KEY_DATA, > + .key_dst = SE_SHA_KEY_DST, > + .result = SE_SHA_HASH_RESULT, > +}; > + > +static const struct tegra_se_hw tegra234_aes_hw = { > + .regs = &tegra234_aes1_regs, > + .kac_ver = 1, > + .host1x_class = 0x3b, > + .init_alg = tegra_init_aes, > + .deinit_alg = tegra_deinit_aes, > +}; > + > +static const struct tegra_se_hw tegra234_hash_hw = { > + .regs = &tegra234_hash_regs, > + .kac_ver = 1, > + .host1x_class = 0x3d, > + .init_alg = tegra_init_hash, > + .deinit_alg = tegra_deinit_hash, > +}; > + > +static const struct of_device_id tegra_se_of_match[] = { > + { > + .compatible = "nvidia,tegra234-se2-aes", > + .data = &tegra234_aes_hw > + }, { > + .compatible = "nvidia,tegra234-se4-hash", > + .data = &tegra234_hash_hw, > + }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, tegra_se_of_match); > + > +static struct platform_driver tegra_se_driver = { > + .driver = { > + .name = "tegra-se", > + .of_match_table = tegra_se_of_match, > + }, > + .probe = tegra_se_probe, > + .remove = tegra_se_remove, > +}; > + > +static int tegra_se_host1x_probe(struct host1x_device *dev) > +{ > + return host1x_device_init(dev); > +} > + > +static int tegra_se_host1x_remove(struct host1x_device *dev) > +{ > + host1x_device_exit(dev); > + > + return 0; > +} > + ... > + return -EINVAL; > +} > + > +/* Functions */ > +int tegra_init_aead(struct tegra_se *se); I look for it and cannot find it... Drop. > +int tegra_init_aes(struct tegra_se *se); > +int tegra_init_hash(struct tegra_se *se); > +void tegra_deinit_aes(struct tegra_se *se); > +void tegra_deinit_hash(struct tegra_se *se); > + > +int tegra_key_submit(struct tegra_se *se, const u8 *key, u32 keylen, u32 alg, u32 *keyid); > +unsigned int tegra_key_get_idx(struct tegra_se *se, u32 keyid); > +void tegra_key_invalidate(struct tegra_se *se, u32 keyid, u32 alg); > + > +int tegra_se_host1x_register(struct tegra_se *se); > +int tegra_se_host1x_submit(struct tegra_se *se, u32 size); Everything looks bogus... > + > +static inline void se_writel(struct tegra_se *se, u32 val, > + unsigned int offset) > +{ > + writel_relaxed(val, se->base + offset); > +} > + > +static inline u32 se_readl(struct tegra_se *se, unsigned int offset) > +{ > + return readl_relaxed(se->base + offset); > +} Both wrappers are useless. > + > +/**** > + * Use Linux coding style comments. > + * HOST1x OPCODES > + * > + ****/ > + ... > + > +static inline u32 host1x_opcode_nonincr(unsigned int offset, unsigned int count) > +{ > + return (2 << 28) | (offset << 16) | count; > +} > + > +static inline u32 host1x_uclass_incr_syncpt_cond_f(u32 v) > +{ > + return (v & 0xff) << 10; Fix indentation, in other places as well. Best regards, Krzysztof
> > Add support for Tegra Security Engine which can accelerates various > > crypto algorithms. The Engine has two separate instances within for > > AES and HASH algorithms respectively. > > > > The driver registers two crypto engines - one for AES and another for > > HASH algorithms and these operate independently and both uses the > > host1x bus. Additionally, it provides hardware-assisted key > > protection for up to 15 symmetric keys which it can use for the cipher > operations. > > > > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > > --- > > ... > > > + > > +int tegra_init_hash(struct tegra_se *se) { > > + struct ahash_engine_alg *alg; > > + int i, ret; > > + > > + se->manifest = tegra_hash_kac_manifest; > > + > > + for (i = 0; i < ARRAY_SIZE(tegra_hash_algs); i++) { > > + tegra_hash_algs[i].se_dev = se; > > + alg = &tegra_hash_algs[i].alg.ahash; > > + > > + ret = crypto_engine_register_ahash(alg); > > + if (ret) { > > + dev_err(se->dev, "failed to register %s\n", > > + alg->base.halg.base.cra_name); > > + goto sha_err; > > + } > > + } > > + > > + dev_info(se->dev, "registered HASH algorithms\n"); > > Drop, not needed. Actually drop simple success messages. Drivers do not spam > dmesg without need. > > ... > > > + > > +int tegra_se_host1x_register(struct tegra_se *se) { > > + INIT_LIST_HEAD(&se->client.list); > > + se->client.dev = se->dev; > > + se->client.ops = &tegra_se_client_ops; > > + se->client.class = se->hw->host1x_class; > > + se->client.num_syncpts = 1; > > + > > + host1x_client_register(&se->client); > > + > > + return 0; > > +} > > + > > +static int tegra_se_clk_init(struct tegra_se *se) { > > + int i, ret; > > + > > + se->clk = devm_clk_get(se->dev, NULL); > > + if (IS_ERR(se->clk)) { > > + dev_err(se->dev, "failed to get clock\n"); > > Why do you print failures multiple times? Once here, second in probe. > > return dev_err_probe > > > + return PTR_ERR(se->clk); > > + } > > + > > + ret = clk_set_rate(se->clk, ULONG_MAX); > > + if (ret) { > > + dev_err(se->dev, "failed to set %d clock rate", i); > > Same comments > > > + return ret; > > + } > > + > > + ret = clk_prepare_enable(se->clk); > > + if (ret) { > > + dev_err(se->dev, "failed to enable clocks\n"); > > Same comments > > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static void tegra_se_clk_deinit(struct tegra_se *se) { > > + clk_disable_unprepare(se->clk); > > Why aren't you using devm_clk_get_enabled? This looks like porting some old, > out-of-tree vendor crappy driver :( > > > +} > > + > > +static int tegra_se_probe(struct platform_device *pdev) { > > + struct device *dev = &pdev->dev; > > + struct tegra_se *se; > > + int ret; > > + > > + se = devm_kzalloc(dev, sizeof(*se), GFP_KERNEL); > > + if (!se) > > + return -ENOMEM; > > + > > + se->dev = dev; > > + se->owner = TEGRA_GPSE_ID; > > + se->hw = device_get_match_data(&pdev->dev); > > + > > + se->base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(se->base)) > > + return PTR_ERR(se->base); > > + > > + dma_set_mask_and_coherent(dev, DMA_BIT_MASK(39)); > > + platform_set_drvdata(pdev, se); > > + > > + ret = tegra_se_clk_init(se); > > + if (ret) { > > + dev_err(dev, "failed to init clocks\n"); > > Syntax is: > return dev_err_probe > > > + return ret; > > + } > > + > > + if (!tegra_dev_iommu_get_stream_id(dev, &se->stream_id)) { > > + dev_err(dev, "failed to get IOMMU stream ID\n"); > > dev_err_probe > > > + goto clk_deinit; > > + } > > + > > + se_writel(se, se->stream_id, SE_STREAM_ID); > > + > > + se->engine = crypto_engine_alloc_init(dev, 0); > > + if (!se->engine) { > > + dev_err(dev, "failed to init crypto engine\n"); > > Really? Test your code with coccinelle. Drop. > > > + ret = -ENOMEM; > > + goto iommu_free; > > + } > > + > > + ret = crypto_engine_start(se->engine); > > + if (ret) { > > + dev_err(dev, "failed to start crypto engine\n"); > > dev_err_probe > > > + goto engine_exit; > > + } > > + > > + ret = tegra_se_host1x_register(se); > > + if (ret) { > > + dev_err(dev, "failed to init host1x params\n"); > > dev_err_probe > > > + goto engine_stop; > > + } > > + > > + return 0; > > + > > +engine_stop: > > + crypto_engine_stop(se->engine); > > +engine_exit: > > + crypto_engine_exit(se->engine); > > +iommu_free: > > + iommu_fwspec_free(se->dev); > > +clk_deinit: > > + tegra_se_clk_deinit(se); > > + > > + return ret; > > +} > > + > > +static int tegra_se_remove(struct platform_device *pdev) { > > + struct tegra_se *se = platform_get_drvdata(pdev); > > + > > + crypto_engine_stop(se->engine); > > + crypto_engine_exit(se->engine); > > + iommu_fwspec_free(se->dev); > > + host1x_client_unregister(&se->client); > > + tegra_se_clk_deinit(se); > > + > > + return 0; > > +} > > + > > +static const struct tegra_se_regs tegra234_aes1_regs = { > > + .config = SE_AES1_CFG, > > + .op = SE_AES1_OPERATION, > > + .last_blk = SE_AES1_LAST_BLOCK, > > + .linear_ctr = SE_AES1_LINEAR_CTR, > > + .aad_len = SE_AES1_AAD_LEN, > > + .cryp_msg_len = SE_AES1_CRYPTO_MSG_LEN, > > + .manifest = SE_AES1_KEYMANIFEST, > > + .key_addr = SE_AES1_KEY_ADDR, > > + .key_data = SE_AES1_KEY_DATA, > > + .key_dst = SE_AES1_KEY_DST, > > + .result = SE_AES1_CMAC_RESULT, > > +}; > > + > > +static const struct tegra_se_regs tegra234_hash_regs = { > > + .config = SE_SHA_CFG, > > + .op = SE_SHA_OPERATION, > > + .manifest = SE_SHA_KEYMANIFEST, > > + .key_addr = SE_SHA_KEY_ADDR, > > + .key_data = SE_SHA_KEY_DATA, > > + .key_dst = SE_SHA_KEY_DST, > > + .result = SE_SHA_HASH_RESULT, > > +}; > > + > > +static const struct tegra_se_hw tegra234_aes_hw = { > > + .regs = &tegra234_aes1_regs, > > + .kac_ver = 1, > > + .host1x_class = 0x3b, > > + .init_alg = tegra_init_aes, > > + .deinit_alg = tegra_deinit_aes, > > +}; > > + > > +static const struct tegra_se_hw tegra234_hash_hw = { > > + .regs = &tegra234_hash_regs, > > + .kac_ver = 1, > > + .host1x_class = 0x3d, > > + .init_alg = tegra_init_hash, > > + .deinit_alg = tegra_deinit_hash, }; > > + > > +static const struct of_device_id tegra_se_of_match[] = { > > + { > > + .compatible = "nvidia,tegra234-se2-aes", > > + .data = &tegra234_aes_hw > > + }, { > > + .compatible = "nvidia,tegra234-se4-hash", > > + .data = &tegra234_hash_hw, > > + }, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(of, tegra_se_of_match); > > + > > +static struct platform_driver tegra_se_driver = { > > + .driver = { > > + .name = "tegra-se", > > + .of_match_table = tegra_se_of_match, > > + }, > > + .probe = tegra_se_probe, > > + .remove = tegra_se_remove, > > +}; > > + > > +static int tegra_se_host1x_probe(struct host1x_device *dev) { > > + return host1x_device_init(dev); > > +} > > + > > +static int tegra_se_host1x_remove(struct host1x_device *dev) { > > + host1x_device_exit(dev); > > + > > + return 0; > > +} > > + > > > ... > > > + return -EINVAL; > > +} > > + > > +/* Functions */ > > +int tegra_init_aead(struct tegra_se *se); > > I look for it and cannot find it... Drop. > > > +int tegra_init_aes(struct tegra_se *se); int tegra_init_hash(struct > > +tegra_se *se); void tegra_deinit_aes(struct tegra_se *se); void > > +tegra_deinit_hash(struct tegra_se *se); > > + > > +int tegra_key_submit(struct tegra_se *se, const u8 *key, u32 keylen, > > +u32 alg, u32 *keyid); unsigned int tegra_key_get_idx(struct tegra_se > > +*se, u32 keyid); void tegra_key_invalidate(struct tegra_se *se, u32 > > +keyid, u32 alg); > > + > > +int tegra_se_host1x_register(struct tegra_se *se); int > > +tegra_se_host1x_submit(struct tegra_se *se, u32 size); > > Everything looks bogus... > > > + > > +static inline void se_writel(struct tegra_se *se, u32 val, > > + unsigned int offset) { > > + writel_relaxed(val, se->base + offset); } > > + > > +static inline u32 se_readl(struct tegra_se *se, unsigned int offset) > > +{ > > + return readl_relaxed(se->base + offset); } > > Both wrappers are useless. > > > + > > +/**** > > + * > > Use Linux coding style comments. > > > + * HOST1x OPCODES > > + * > > + ****/ > > + > > ... > > > + > > +static inline u32 host1x_opcode_nonincr(unsigned int offset, unsigned > > +int count) { > > + return (2 << 28) | (offset << 16) | count; } > > + > > +static inline u32 host1x_uclass_incr_syncpt_cond_f(u32 v) { > > + return (v & 0xff) << 10; > > Fix indentation, in other places as well. > > > Best regards, > Krzysztof Thanks for the comments. Updated and posted a new version with the changes. Regards, Akhil