Re: [PATCH v2 2/2] mtd: rawnand: denali_dt: add reset controlling

[Masahiro Yamada <masahiroy@kernel.org>]

On Thu, Dec 12, 2019 at 10:05 AM Marek Vasut <marex@denx.de> wrote:
>
> On 12/11/19 6:45 AM, Masahiro Yamada wrote:
> [...]
> > diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
> > index 8b779a899dcf..9a294c3f6ec8 100644
> > --- a/drivers/mtd/nand/raw/denali_dt.c
> > +++ b/drivers/mtd/nand/raw/denali_dt.c
> > @@ -6,6 +6,7 @@
> >   */
> >
> >  #include <linux/clk.h>
> > +#include <linux/delay.h>
> >  #include <linux/err.h>
> >  #include <linux/io.h>
> >  #include <linux/ioport.h>
> > @@ -14,6 +15,7 @@
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/reset.h>
> >
> >  #include "denali.h"
> >
> > @@ -22,6 +24,8 @@ struct denali_dt {
> >       struct clk *clk;        /* core clock */
> >       struct clk *clk_x;      /* bus interface clock */
> >       struct clk *clk_ecc;    /* ECC circuit clock */
> > +     struct reset_control *rst;      /* core reset */
> > +     struct reset_control *rst_reg;  /* register reset */
> >  };
> >
> >  struct denali_dt_data {
> > @@ -151,6 +155,14 @@ static int denali_dt_probe(struct platform_device *pdev)
> >       if (IS_ERR(dt->clk_ecc))
> >               return PTR_ERR(dt->clk_ecc);
> >
> > +     dt->rst = devm_reset_control_get_optional_shared(dev, "nand");
> > +     if (IS_ERR(dt->rst))
> > +             return PTR_ERR(dt->rst);
> > +
> > +     dt->rst_reg = devm_reset_control_get_optional_shared(dev, "reg");
> > +     if (IS_ERR(dt->rst_reg))
> > +             return PTR_ERR(dt->rst_reg);
> > +
> >       ret = clk_prepare_enable(dt->clk);
> >       if (ret)
> >               return ret;
> > @@ -166,10 +178,30 @@ static int denali_dt_probe(struct platform_device *pdev)
> >       denali->clk_rate = clk_get_rate(dt->clk);
> >       denali->clk_x_rate = clk_get_rate(dt->clk_x);
> >
> > -     ret = denali_init(denali);
> > +     /*
> > +      * Deassert the register reset, and the core reset in this order.
> > +      * Deasserting the core reset while the register reset is asserted
> > +      * will cause unpredictable behavior in the controller.
> > +      */
> > +     ret = reset_control_deassert(dt->rst_reg);
> >       if (ret)
> >               goto out_disable_clk_ecc;
> >
> > +     ret = reset_control_deassert(dt->rst);
> > +     if (ret)
> > +             goto out_assert_rst_reg;
> > +
> > +     /*
> > +      * When the reset is deasserted, the initialization sequence is kicked
> > +      * (bootstrap process). The driver must wait until it finished.
> > +      * Otherwise, it will result in unpredictable behavior.
> > +      */
> > +     usleep_range(200, 1000);
> > +
> > +     ret = denali_init(denali);
> > +     if (ret)
> > +             goto out_assert_rst;
> > +
> >       for_each_child_of_node(dev->of_node, np) {
> >               ret = denali_dt_chip_init(denali, np);
> >               if (ret) {
> > @@ -184,6 +216,10 @@ static int denali_dt_probe(struct platform_device *pdev)
> >
> >  out_remove_denali:
> >       denali_remove(denali);
> > +out_assert_rst:
> > +     reset_control_assert(dt->rst);
> > +out_assert_rst_reg:
> > +     reset_control_assert(dt->rst_reg);
>
> Maybe you can use devm_add_action_or_reset() here , like in e.g.
> drivers/input/touchscreen/ili210x.c , to avoid this unwinding ?


No.

Drivers should be explicit about what and when
to do about the hardware access.


This comes down to a question about why
Linux kernel does not have such APIs as:

devm_clk_prepare_enable()
devm_reset_control_deassert()
devm_regulator_enable()

In fact, I saw some people sending such patches in the past.


Mark Brown clearly answered the question.
https://lkml.org/lkml/2014/2/1/131

I really support his thinking.





--
Best Regards

Masahiro Yamada