Message-ID: <18560338.ihuYOPNcR6@phil>
Date:	Sat, 14 Nov 2015 23:41:35 +0100
From:	Heiko Stuebner <heiko@...ech.de>
To:	Zain <zain.wang@...k-chips.com>
Cc:	zhengsq@...k-chips.com, hl@...k-chips.com,
	herbert@...dor.apana.org.au, davem@...emloft.net,
	mturquette@...libre.com, pawel.moll@....com,
	ijc+devicetree@...lion.org.uk, robh+dt@...nel.org,
	galak@...eaurora.org, linux@....linux.org.uk, mark.rutland@....com,
	linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
	linux-rockchip@...ts.infradead.org, devicetree@...r.kernel.org,
	eddie.cai@...k-chips.com
Subject: 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 <zain.wang@...k-chips.com>
> >> ---
> > [...]
> >
> >> 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.

> >> +
> >> +	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 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.

> >
> >> +	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.


Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists