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
---
>>> [...]
>>>
diff --git a/drivers/crypto/rockchip/rk3288_crypto.c
b/drivers/crypto/rockchip/rk3288_crypto.c
new file mode 100644
index 000..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();
+
Re: [PATCH v3 3/4] Crypto: rockchip/crypto - add crypto driver for rk3288
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
> >> ---
> > [...]
> >
> >> diff --git a/drivers/crypto/rockchip/rk3288_crypto.c
> >> b/drivers/crypto/rockchip/rk3288_crypto.c
> >> new file mode 100644
> >> index 000..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.
> >> +
> >> + 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 :-)
> >
> > [...]
> >
> >> +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 sequen
Re: [PATCH v3 3/4] Crypto: rockchip/crypto - add crypto driver for rk3288
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
>> ---
> [...]
>
>> diff --git a/drivers/crypto/rockchip/rk3288_crypto.c
>> b/drivers/crypto/rockchip/rk3288_crypto.c
>> new file mode 100644
>> index 000..bb36baa
>> --- /dev/null
>> +++ b/drivers/crypto/rockchip/rk3288_crypto.c
>> @@ -0,0 +1,392 @@
> [...]
>
>> +static irqreturn_t crypto_irq_handle(int irq, void *dev_id)
> that function should probably also get a "rk_" prefix?
OK! good idea.
I will add it to next version.
>> +{
>> +struct rk_crypto_info *dev = platform_get_drvdata(dev_id);
>> +u32 interrupt_status;
>> +int err = 0;
>> +
>> +spin_lock(&dev->lock);
>> +
>> +if (irq == dev->irq) {
> I'm not sure I understand that line. Interrupt handlers are only
> called for the interrupt they are registered for, which would be dev->irq
> in any case, so that should always be true and therefore be unnecessary?
You are right, it is unnecessary.
It will be remove in next version.
>
>> +interrupt_status = CRYPTO_READ(dev, RK_CRYPTO_INTSTS);
>> +CRYPTO_WRITE(dev, RK_CRYPTO_INTSTS, interrupt_status);
>> +if (interrupt_status & 0x0a) {
>> +dev_warn(dev->dev, "DMA Error\n");
>> +err = -EFAULT;
>> +} else if (interrupt_status & 0x05) {
>> +err = dev->update(dev);
>> +}
>> +
>> +if (err)
>> +dev->complete(dev, err);
>> +}
>> +spin_unlock(&dev->lock);
>> +return IRQ_HANDLED;
>> +}
> [...]
>
>> +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);
>> +
>> +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
Re: [PATCH v3 3/4] Crypto: rockchip/crypto - add crypto driver for rk3288
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
> ---
[...]
> diff --git a/drivers/crypto/rockchip/rk3288_crypto.c
> b/drivers/crypto/rockchip/rk3288_crypto.c
> new file mode 100644
> index 000..bb36baa
> --- /dev/null
> +++ b/drivers/crypto/rockchip/rk3288_crypto.c
> @@ -0,0 +1,392 @@
[...]
> +static irqreturn_t crypto_irq_handle(int irq, void *dev_id)
that function should probably also get a "rk_" prefix?
> +{
> + struct rk_crypto_info *dev = platform_get_drvdata(dev_id);
> + u32 interrupt_status;
> + int err = 0;
> +
> + spin_lock(&dev->lock);
> +
> + if (irq == dev->irq) {
I'm not sure I understand that line. Interrupt handlers are only
called for the interrupt they are registered for, which would be dev->irq
in any case, so that should always be true and therefore be unnecessary?
> + interrupt_status = CRYPTO_READ(dev, RK_CRYPTO_INTSTS);
> + CRYPTO_WRITE(dev, RK_CRYPTO_INTSTS, interrupt_status);
> + if (interrupt_status & 0x0a) {
> + dev_warn(dev->dev, "DMA Error\n");
> + err = -EFAULT;
> + } else if (interrupt_status & 0x05) {
> + err = dev->update(dev);
> + }
> +
> + if (err)
> + dev->complete(dev, err);
> + }
> + spin_unlock(&dev->lock);
> + return IRQ_HANDLED;
> +}
[...]
> +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);
> +
> + 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
[...]
> +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 sequentia
