by zain

[permalink] [raw]

Subject: Re: [PATCH v3 3/4] Crypto: rockchip/crypto - add crypto driver for rk3288

On 2015年11月15日 06:41, Heiko Stuebner wrote:
> Hi Zain,
>
> Am Freitag, 13. November 2015, 14:44:43 schrieb Zain:
>> On 2015年11月12日 20:32, Heiko Stuebner wrote:
>>> Hi Zain,
>>>
>>> I was able to sucessfully test your crypto-driver, but have found some
>>> improvements below that should probably get looked at:
>>>
>>> Am Mittwoch, 11. November 2015, 14:35:58 schrieb Zain Wang:
>>>> Crypto driver support:
>>>> ecb(aes) cbc(aes) ecb(des) cbc(des) ecb(des3_ede) cbc(des3_ede)
>>>> You can alloc tags above in your case.
>>>>
>>>> And other algorithms and platforms will be added later on.
>>>>
>>>> Signed-off-by: Zain Wang <[email protected]>
>>>> ---
>>> [...]
>>>
>>>> diff --git a/drivers/crypto/rockchip/rk3288_crypto.c b/drivers/crypto/rockchip/rk3288_crypto.c
>>>> new file mode 100644
>>>> index 0000000..bb36baa
>>>> --- /dev/null
>>>> +++ b/drivers/crypto/rockchip/rk3288_crypto.c
>>>> @@ -0,0 +1,392 @@
> [...]
>
> static void rk_crypto_action(void *data)
> {
> struct rk_crypto_info *crypto_info = data;
>
> reset_control_assert(crypto_info->rst);
> }
>
>>>> +static int rk_crypto_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct resource *res;
>>>> + struct device *dev = &pdev->dev;
>>>> + struct rk_crypto_info *crypto_info;
>>>> + int err = 0;
>>>> +
>>>> + crypto_info = devm_kzalloc(&pdev->dev,
>>>> + sizeof(*crypto_info), GFP_KERNEL);
>>>> + if (!crypto_info) {
>>>> + err = -ENOMEM;
>>>> + goto err_crypto;
>>>> + }
>>>> +
>>>> + crypto_info->rst = devm_reset_control_get(dev, "crypto-rst");
>>>> + if (IS_ERR(crypto_info->rst)) {
>>>> + err = PTR_ERR(crypto_info->rst);
>>>> + goto err_crypto;
>>>> + }
>>>> +
>>>> + reset_control_assert(crypto_info->rst);
>>>> + usleep_range(10, 20);
>>>> + reset_control_deassert(crypto_info->rst);
> err = devm_add_action(dev, rk_crypto_action, crypto_info);
> if (err) {
> reset_control_assert(crypto_info->rst);
> return err;
> }
>
> from here onwards the devm_action will always be executed when either
> _probe fails, or after remove finishes, so no need to assert the reset
> manually.
I have known this function,
rk_crypto_action will be executed next to either failed _probe, or
_remove automatically.
It also make sure reset_control_assert can be executed after tasklet_kill.

OK! Done!
>
>>>> +
>>>> + spin_lock_init(&crypto_info->lock);
>>>> +
>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> + crypto_info->reg = devm_ioremap_resource(&pdev->dev, res);
>>>> + if (IS_ERR(crypto_info->reg)) {
>>>> + err = PTR_ERR(crypto_info->reg);
>>>> + goto err_ioremap;
>>>> + }
>>>> +
>>>> + crypto_info->aclk = devm_clk_get(&pdev->dev, "aclk");
>>>> + if (IS_ERR(crypto_info->aclk)) {
>>>> + err = PTR_ERR(crypto_info->aclk);
>>>> + goto err_ioremap;
>>>> + }
>>>> +
>>>> + crypto_info->hclk = devm_clk_get(&pdev->dev, "hclk");
>>>> + if (IS_ERR(crypto_info->hclk)) {
>>>> + err = PTR_ERR(crypto_info->hclk);
>>>> + goto err_ioremap;
>>>> + }
>>>> +
>>>> + crypto_info->sclk = devm_clk_get(&pdev->dev, "sclk");
>>>> + if (IS_ERR(crypto_info->sclk)) {
>>>> + err = PTR_ERR(crypto_info->sclk);
>>>> + goto err_ioremap;
>>>> + }
>>>> +
>>>> + crypto_info->dmaclk = devm_clk_get(&pdev->dev, "apb_pclk");
>>>> + if (IS_ERR(crypto_info->dmaclk)) {
>>>> + err = PTR_ERR(crypto_info->dmaclk);
>>>> + goto err_ioremap;
>>>> + }
>>>> +
>>>> + crypto_info->irq = platform_get_irq(pdev, 0);
>>>> + if (crypto_info->irq < 0) {
>>>> + dev_warn(crypto_info->dev,
>>>> + "control Interrupt is not available.\n");
>>>> + err = crypto_info->irq;
>>>> + goto err_ioremap;
>>>> + }
>>>> +
>>>> + err = devm_request_irq(&pdev->dev, crypto_info->irq, crypto_irq_handle,
>>>> + IRQF_SHARED, "rk-crypto", pdev);
>>> you are freeing the irq manually below and in _remove too. As it stands
>>> with putting the ip block in reset again on remove this should either loose
>>> the devm_ or you could add a devm_action that handles the reset assert
>>> like in [0] - registering the devm_action above where the reset is done.
>>> That way you could really use dev_request_irq and loose the free_irq
>>> calls here and in remove.
>>>
>>> [0] https://lkml.org/lkml/2015/11/8/159
>> I made a mistake here. I did not remove free_irq when using
>> devm_request_irq.
>>
>> As I do not konw enough about devm_action and reset-assert , can I
>> remove free_irq
>> simply like drivers/i2c/buses/i2c-sun6i-p2wi.c used devm_request_irq and
>> reset_assert?
> I did insert suitable code on how that could look a bit above :-)
Thanks, done!
>
>>> [...]
>>>
>>>> +static int rk_crypto_remove(struct platform_device *pdev)
>>>> +{
>>>> + struct rk_crypto_info *crypto_tmp = platform_get_drvdata(pdev);
>>>> +
>>>> + rk_crypto_unregister();
>>>> + reset_control_assert(crypto_tmp->rst);
>>> mainly my comment from above applies, but in any case the reset-assert
>>> should definitly happen _after_ the tasklet is killed and the irq freed,
>>> to make sure nothing will want to access device-registers anymore.
>>>
>>> [devm works sequentially, so the devm_action would solve that automatically]
>> As I said above, it seem unnecessary to add devm_action.
>>
>> And if modification above is good, I will push tasklet_kill before
>> reset_control_assert in next version.
> I'm unsure ... but I guess if you move the reset-assert after the
> tasklet_kill it might be ok.
I guess I can remove it if I added code you provided above since
reset-assert will be
executed after _remove by rk_crypto_action.
>
>>>> + tasklet_kill(&crypto_tmp->crypto_tasklet);
>>>> + free_irq(crypto_tmp->irq, crypto_tmp);
>>>> +
>>>> + return 0;
>>>> +}
>>> [...]
>>>
>>>> diff --git a/drivers/crypto/rockchip/rk3288_crypto.h b/drivers/crypto/rockchip/rk3288_crypto.h
>>>> new file mode 100644
>>>> index 0000000..b5b949a
>>>> --- /dev/null
>>>> +++ b/drivers/crypto/rockchip/rk3288_crypto.h
>>>> @@ -0,0 +1,220 @@
>>>> +#define _SBF(v, f) ((v) << (f))
>>> you are using that macro in this header for simple value shifts like
>>> #define RK_CYYPTO_HASHINSEL_BLOCK_CIPHER_INPUT _SBF(0x01, 0)
>>>
>>> Removing that macro and doing the shift regularly without any macro, like
>>> #define RK_CYYPTO_HASHINSEL_BLOCK_CIPHER_INPUT (0x01 << 0)
>>>
>>> would improve future readability, because now you need to look up what
>>> the macro actually does and the _SBF macro also does not simplify anything.
>>> Also that name is quite generic so may conflict with something else easily.
>> Ok! Done!
>>> [...]
>>>
>>>> +#define CRYPTO_READ(dev, offset) \
>>>> + readl_relaxed(((dev)->reg + (offset)))
>>>> +#define CRYPTO_WRITE(dev, offset, val) \
>>>> + writel_relaxed((val), ((dev)->reg + (offset)))
>>>> +/* get register virt address */
>>>> +#define CRYPTO_GET_REG_VIRT(dev, offset) ((dev)->reg + (offset))
>>> same argument as above about readability of the code. What do these
>>> macros improve from just doing the readl and writel calls normally?
>> I am sorry that this macro is define for hash and not be used here.
>> because there are some continuous registers in hash,
>> I think we can use this macro with memcpy like
>> output = CRYPTO_GET_REG_VIRT(dev, RK_CRYPTO_HASH_DOUT_0);
>> memcpy(dev->ahash_req->result, output,
>> crypto_ahash_digestsize(tfm));
>> instead of multiple readl.
>>
>> I will remove it in next version and add it to hash patch later on.
> I actuall meant all 3 of those macros :-) ... all of them just diguise
> what the code actually does, so don't provide additional value over
> just using readl_relaxed etc directly.
Right.

Thanks
Zain