On 10/21/14 01:28, Vladimir Zapolskiy wrote:
> Hello LABBE,
>
> On 19.10.2014 17:16, LABBE Corentin wrote:
>> Add support for the Security System included in Allwinner SoC A20.
>> The Security System is a hardware cryptographic accelerator that support AES/MD5/SHA1/DES/3DES/PRNG algorithms.
>>
[]
>> +
>> + /* If we have only one SG, we can use kmap_atomic */
>> + if (sg_next(in_sg) == NULL && sg_next(out_sg) == NULL)
>> + return sunxi_ss_aes_poll_atomic(areq);
>
> for clarity it might be better to move all "mutex_unlock(&ss->lock)"
> calls from sunxi_ss_aes_poll_atomic() body right to here.
>
Ok
I have moved all mutex_unlock/writel(0, SS_CTL) at the end of function, it is cleaner now.
>> +
>> +};
>> +
>> +static int sunxi_ss_probe(struct platform_device *pdev)
>> +{
>> + struct resource *res;
>> + u32 v;
>> + int err;
>> + unsigned long cr;
>> + const unsigned long cr_ahb = 24 * 1000 * 1000;
>> + const unsigned long cr_mod = 150 * 1000 * 1000;
>> +
>> + if (!pdev->dev.of_node)
>> + return -ENODEV;
>> +
>> + ss = devm_kzalloc(&pdev->dev, sizeof(*ss), GFP_KERNEL);
>> + if (ss == NULL)
>> + return -ENOMEM;
>
> Why do you dynamically allocate memory for "struct sunxi_ss_ctx *ss"?
> Since you have a single global pointer, it makes sense to declare
> "struct sunxi_ss_ctx ss" statically instead.
>
> And even a better solution is to remove a single global pointer.
All other crypto driver I have read use a global structure and it made things easy.
Thanks to M. Ripard that pointed to me the talitos driver that solve the global device pointer by using alg template and container_of().
But since I think there will never 2 Security System at the same time on the same SoC, I do not know if it is worth the cost to add more complexity just to remove a pointer.
>
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + ss->base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(ss->base)) {
>> + dev_err(&pdev->dev, "Cannot request MMIO\n");
>> + return PTR_ERR(ss->base);
>> + }
>> +
>> + ss->ssclk = devm_clk_get(&pdev->dev, "mod");
>> + if (IS_ERR(ss->ssclk)) {
>> + err = PTR_ERR(ss->ssclk);
>> + dev_err(&pdev->dev, "Cannot get SS clock err=%d\n", err);
>> + return err;
>> + }
>> + dev_dbg(&pdev->dev, "clock ss acquired\n");
>> +
>> + ss->busclk = devm_clk_get(&pdev->dev, "ahb");
>> + if (IS_ERR(ss->busclk)) {
>> + err = PTR_ERR(ss->busclk);
>> + dev_err(&pdev->dev, "Cannot get AHB SS clock err=%d\n", err);
>> + return err;
>> + }
>> + dev_dbg(&pdev->dev, "clock ahb_ss acquired\n");
>> +
>> + /* Enable both clocks */
>> + err = clk_prepare_enable(ss->busclk);
>> + if (err != 0) {
>> + dev_err(&pdev->dev, "Cannot prepare_enable busclk\n");
>> + return err;
>> + }
>> + err = clk_prepare_enable(ss->ssclk);
>> + if (err != 0) {
>> + dev_err(&pdev->dev, "Cannot prepare_enable ssclk\n");
>> + clk_disable_unprepare(ss->busclk);
>
> goto somewhere to the end of the function?
OK
>
>> + return err;
>> + }
>> +
>> + /*
>> + * Check that clock have the correct rates gived in the datasheet
>> + * Try to set the clock to the maximum allowed
>> + */
>> + err = clk_set_rate(ss->ssclk, cr_mod);
>> + if (err != 0) {
>> + dev_err(&pdev->dev, "Cannot set clock rate to ssclk\n");
>> + clk_disable_unprepare(ss->ssclk);
>> + clk_disable_unprepare(ss->busclk);
>
> goto "error_md5"?
Ok
>
>> + return err;
>> + }
>> +
>> + cr = clk_get_rate(ss->busclk);
>> + if (cr >= cr_ahb)
>> + dev_dbg(&pdev->dev, "Clock bus %lu (%lu MHz) (must be >= %lu)\n",
>> + cr, cr / 1000000, cr_ahb);
>> + else
>> + dev_warn(&pdev->dev, "Clock bus %lu (%lu MHz) (must be >= %lu)\n",
>> + cr, cr / 1000000, cr_ahb);
>
> See next comment.
>
>> + cr = clk_get_rate(ss->ssclk);
>> + if (cr <= cr_mod)
>> + if (cr < cr_mod)
>> + dev_info(&pdev->dev, "Clock ss %lu (%lu MHz) (must be <= %lu)\n",
>> + cr, cr / 1000000, cr_mod);
>> + else
>> + dev_dbg(&pdev->dev, "Clock ss %lu (%lu MHz) (must be <= %lu)\n",
>> + cr, cr / 1000000, cr_mod);
>> + else
>> + dev_warn(&pdev->dev, "Clock ss is at %lu (%lu MHz) (must be <= %lu)\n",
>> + cr, cr / 1000000, cr_mod);
>
> The management of kernel log levels looks pretty strange. As far as I
> understand there is no error on any clock rate, I'd recommend to keep
> only one information message.
>
If clock rate are below the recommended value, the only impact I found was bad performance.
So it explain the warn and no error. (yes the info must be warn, ...fixed)
But I will put comment for explain that.
>> + /*
>> + * Datasheet named it "Die Bonding ID"
>> + * I expect to be a sort of Security System Revision number.
>> + * Since the A80 seems to have an other version of SS
>> + * this info could be useful
>> + */
>> + writel(SS_ENABLED, ss->base + SS_CTL);
>> + v = readl(ss->base + SS_CTL);
>> + v >>= 16;
>> + v &= 0x07;
>> + dev_info(&pdev->dev, "Die ID %d\n", v);
>> + writel(0, ss->base + SS_CTL);
>> +
>> + ss->dev = &pdev->dev;
>> +
>> + mutex_init(&ss->lock);
>> + mutex_init(&ss->bufin_lock);
>> + mutex_init(&ss->bufout_lock);
>> +
>> + err = crypto_register_ahash(&sunxi_md5_alg);
>> + if (err)
>> + goto error_md5;
>> + err = crypto_register_ahash(&sunxi_sha1_alg);
>> + if (err)
>> + goto error_sha1;
>> + err = crypto_register_algs(sunxi_cipher_algs,
>> + ARRAY_SIZE(sunxi_cipher_algs));
>> + if (err)
>> + goto error_ciphers;
>> +
>> + return 0;
>> +error_ciphers:
>> + crypto_unregister_ahash(&sunxi_sha1_alg);
>> +error_sha1:
>> + crypto_unregister_ahash(&sunxi_md5_alg);
>> +error_md5:
>> + clk_disable_unprepare(ss->ssclk);
>> + clk_disable_unprepare(ss->busclk);
>> + return err;
>> +}
>> +
>> +static int __exit sunxi_ss_remove(struct platform_device *pdev)
>> +{
>> + if (!pdev->dev.of_node)
>> + return 0;
>
> Redundant check.
>
Ok
>
>
> --
> With best wishes,
> Vladimir
>
Thanks for the review
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
