| Message ID | 20220508185957.3629088-15-clabbe@baylibre.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: rockchip: permit to pass self-tests | expand |
On Sun, May 08, 2022 at 06:59:38PM +0000, Corentin Labbe wrote: > reset could be handled by PM functions. Is there any further rationale for this? After this change there is no longer a guaranteed reset pulse on probe since the reset control may already be de-asserted. This is normally the most important case for a reset as it's the only time when the state of the hardware is unknown. The original use of devm_add_action_or_reset() seems a bit weird already since there doesn't seem to be any need to assert reset when the driver is unloaded. > Signed-off-by: Corentin Labbe <clabbe@baylibre.com> > --- > drivers/crypto/rockchip/rk3288_crypto.c | 19 ++++--------------- > 1 file changed, 4 insertions(+), 15 deletions(-) > > diff --git a/drivers/crypto/rockchip/rk3288_crypto.c b/drivers/crypto/rockchip/rk3288_crypto.c > index d9258b9e71b3..a11a92e1f3fd 100644 > --- a/drivers/crypto/rockchip/rk3288_crypto.c > +++ b/drivers/crypto/rockchip/rk3288_crypto.c > @@ -73,6 +73,8 @@ static int rk_crypto_pm_suspend(struct device *dev) > { > struct rk_crypto_info *rkdev = dev_get_drvdata(dev); > > + reset_control_assert(rkdev->rst); > + > rk_crypto_disable_clk(rkdev); > return 0; > } > @@ -81,6 +83,8 @@ static int rk_crypto_pm_resume(struct device *dev) > { > struct rk_crypto_info *rkdev = dev_get_drvdata(dev); > > + reset_control_deassert(rkdev->rst); > + > return rk_crypto_enable_clk(rkdev); > } > > @@ -222,13 +226,6 @@ static void rk_crypto_unregister(void) > } > } > > -static void rk_crypto_action(void *data) > -{ > - struct rk_crypto_info *crypto_info = data; > - > - reset_control_assert(crypto_info->rst); > -} > - > static const struct of_device_id crypto_of_id_table[] = { > { .compatible = "rockchip,rk3288-crypto" }, > {} > @@ -254,14 +251,6 @@ static int rk_crypto_probe(struct platform_device *pdev) > goto err_crypto; > } > > - reset_control_assert(crypto_info->rst); > - usleep_range(10, 20); > - reset_control_deassert(crypto_info->rst); > - > - err = devm_add_action_or_reset(dev, rk_crypto_action, crypto_info); > - if (err) > - goto err_crypto; > - > crypto_info->reg = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(crypto_info->reg)) { > err = PTR_ERR(crypto_info->reg); > -- > 2.35.1
Le Mon, Jun 20, 2022 at 12:04:24PM +0100, John Keeping a écrit : > On Sun, May 08, 2022 at 06:59:38PM +0000, Corentin Labbe wrote: > > reset could be handled by PM functions. > > Is there any further rationale for this? > > After this change there is no longer a guaranteed reset pulse on probe > since the reset control may already be de-asserted. This is normally > the most important case for a reset as it's the only time when the state > of the hardware is unknown. > > The original use of devm_add_action_or_reset() seems a bit weird already > since there doesn't seem to be any need to assert reset when the driver > is unloaded. > I am not an hw engineer, so my knowledge on reset is low. So why not having a reset pulse on probe is a problem ? Do you mean I must put reset asserted on probe ?
On Tue, Jun 21, 2022 at 10:05:54AM +0200, LABBE Corentin wrote: > Le Mon, Jun 20, 2022 at 12:04:24PM +0100, John Keeping a écrit : > > On Sun, May 08, 2022 at 06:59:38PM +0000, Corentin Labbe wrote: > > > reset could be handled by PM functions. > > > > Is there any further rationale for this? > > > > After this change there is no longer a guaranteed reset pulse on probe > > since the reset control may already be de-asserted. This is normally > > the most important case for a reset as it's the only time when the state > > of the hardware is unknown. > > > > The original use of devm_add_action_or_reset() seems a bit weird already > > since there doesn't seem to be any need to assert reset when the driver > > is unloaded. > > > > I am not an hw engineer, so my knowledge on reset is low. > So why not having a reset pulse on probe is a problem ? The point of the reset is to bring the hardware back to a known state. Since we don't know what state the hardware will be in following the bootloader or previous OS, I think the reset in probe is the only place that it is important. If this patch isn't fixing anything, I suggest just dropping it.
Le Tue, Jun 21, 2022 at 02:37:35PM +0100, John Keeping a écrit : > On Tue, Jun 21, 2022 at 10:05:54AM +0200, LABBE Corentin wrote: > > Le Mon, Jun 20, 2022 at 12:04:24PM +0100, John Keeping a écrit : > > > On Sun, May 08, 2022 at 06:59:38PM +0000, Corentin Labbe wrote: > > > > reset could be handled by PM functions. > > > > > > Is there any further rationale for this? > > > > > > After this change there is no longer a guaranteed reset pulse on probe > > > since the reset control may already be de-asserted. This is normally > > > the most important case for a reset as it's the only time when the state > > > of the hardware is unknown. > > > > > > The original use of devm_add_action_or_reset() seems a bit weird already > > > since there doesn't seem to be any need to assert reset when the driver > > > is unloaded. > > > > > > > I am not an hw engineer, so my knowledge on reset is low. > > So why not having a reset pulse on probe is a problem ? > > The point of the reset is to bring the hardware back to a known state. > Since we don't know what state the hardware will be in following the > bootloader or previous OS, I think the reset in probe is the only place > that it is important. > > If this patch isn't fixing anything, I suggest just dropping it. Thanks for the explanation, I will re-add the reset at probe.
diff --git a/drivers/crypto/rockchip/rk3288_crypto.c b/drivers/crypto/rockchip/rk3288_crypto.c index d9258b9e71b3..a11a92e1f3fd 100644 --- a/drivers/crypto/rockchip/rk3288_crypto.c +++ b/drivers/crypto/rockchip/rk3288_crypto.c @@ -73,6 +73,8 @@ static int rk_crypto_pm_suspend(struct device *dev) { struct rk_crypto_info *rkdev = dev_get_drvdata(dev); + reset_control_assert(rkdev->rst); + rk_crypto_disable_clk(rkdev); return 0; } @@ -81,6 +83,8 @@ static int rk_crypto_pm_resume(struct device *dev) { struct rk_crypto_info *rkdev = dev_get_drvdata(dev); + reset_control_deassert(rkdev->rst); + return rk_crypto_enable_clk(rkdev); } @@ -222,13 +226,6 @@ static void rk_crypto_unregister(void) } } -static void rk_crypto_action(void *data) -{ - struct rk_crypto_info *crypto_info = data; - - reset_control_assert(crypto_info->rst); -} - static const struct of_device_id crypto_of_id_table[] = { { .compatible = "rockchip,rk3288-crypto" }, {} @@ -254,14 +251,6 @@ static int rk_crypto_probe(struct platform_device *pdev) goto err_crypto; } - reset_control_assert(crypto_info->rst); - usleep_range(10, 20); - reset_control_deassert(crypto_info->rst); - - err = devm_add_action_or_reset(dev, rk_crypto_action, crypto_info); - if (err) - goto err_crypto; - crypto_info->reg = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(crypto_info->reg)) { err = PTR_ERR(crypto_info->reg);
reset could be handled by PM functions. Signed-off-by: Corentin Labbe <clabbe@baylibre.com> --- drivers/crypto/rockchip/rk3288_crypto.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-)