| Message ID | 20160619113739.30362-1-icenowy@aosc.xyz |
|---|---|
| State | Accepted |
| Commit | 7f657279a6de28573f13420840b2de3e04131d04 |
| Headers | show |
+Philipp On Sun, 19 Jun 2016 19:37:39 +0800 Icenowy Zheng <icenowy@aosc.xyz> wrote: > The NAND controller on some sun8i chips needs its reset line to be deasserted > before they can enter working state. This commit added the reset line process > to the driver. > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> > --- > drivers/mtd/nand/sunxi_nand.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c > index a83a690..1502748 100644 > --- a/drivers/mtd/nand/sunxi_nand.c > +++ b/drivers/mtd/nand/sunxi_nand.c > @@ -39,6 +39,7 @@ > #include <linux/gpio.h> > #include <linux/interrupt.h> > #include <linux/iopoll.h> > +#include <linux/reset.h> > > #define NFC_REG_CTL 0x0000 > #define NFC_REG_ST 0x0004 > @@ -269,6 +270,7 @@ struct sunxi_nfc { > void __iomem *regs; > struct clk *ahb_clk; > struct clk *mod_clk; > + struct reset_control *reset; > unsigned long assigned_cs; > unsigned long clk_rate; > struct list_head chips; > @@ -1871,6 +1873,18 @@ static int sunxi_nfc_probe(struct platform_device *pdev) > if (ret) > goto out_ahb_clk_unprepare; > > + nfc->reset = devm_reset_control_get_optional(dev, "ahb"); > + if (PTR_ERR(nfc->reset) == -EPROBE_DEFER) > + return PTR_ERR(nfc->reset); Actually you should test for != -ENOENT, because all error codes except this one should stop the ->probe(). BTW, this devm_reset_control_get_optional() is really weird. While most _optional() methods return NULL when the element is not defined in the DT, this one returns -ENOTENT, which makes it impossible to differentiate a real error from a undefined reset line (which is a valid case for _optional()). Philipp, is there a good reason for doing that? > + > + if (!IS_ERR(nfc->reset)) { > + ret = reset_control_deassert(nfc->reset); > + if (ret) { > + dev_err(dev, "reset err %d\n", ret); > + goto out_mod_clk_unprepare; > + } > + } > +
To be honest, I copied them from sunxi-mmc.c. What function should be chosen better? 19.06.2016, 20:06, "Boris Brezillon" <boris.brezillon@free-electrons.com>: > +Philipp > > On Sun, 19 Jun 2016 19:37:39 +0800 > Icenowy Zheng <icenowy@aosc.xyz> wrote: > >> The NAND controller on some sun8i chips needs its reset line to be deasserted >> before they can enter working state. This commit added the reset line process >> to the driver. >> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> >> --- >> drivers/mtd/nand/sunxi_nand.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c >> index a83a690..1502748 100644 >> --- a/drivers/mtd/nand/sunxi_nand.c >> +++ b/drivers/mtd/nand/sunxi_nand.c >> @@ -39,6 +39,7 @@ >> #include <linux/gpio.h> >> #include <linux/interrupt.h> >> #include <linux/iopoll.h> >> +#include <linux/reset.h> >> >> #define NFC_REG_CTL 0x0000 >> #define NFC_REG_ST 0x0004 >> @@ -269,6 +270,7 @@ struct sunxi_nfc { >> void __iomem *regs; >> struct clk *ahb_clk; >> struct clk *mod_clk; >> + struct reset_control *reset; >> unsigned long assigned_cs; >> unsigned long clk_rate; >> struct list_head chips; >> @@ -1871,6 +1873,18 @@ static int sunxi_nfc_probe(struct platform_device *pdev) >> if (ret) >> goto out_ahb_clk_unprepare; >> >> + nfc->reset = devm_reset_control_get_optional(dev, "ahb"); >> + if (PTR_ERR(nfc->reset) == -EPROBE_DEFER) >> + return PTR_ERR(nfc->reset); > > Actually you should test for != -ENOENT, because all error codes except > this one should stop the ->probe(). > > BTW, this devm_reset_control_get_optional() is really weird. While most > _optional() methods return NULL when the element is not defined in the > DT, this one returns -ENOTENT, which makes it impossible to > differentiate a real error from a undefined reset line (which is a > valid case for _optional()). > > Philipp, is there a good reason for doing that? > >> + >> + if (!IS_ERR(nfc->reset)) { >> + ret = reset_control_deassert(nfc->reset); >> + if (ret) { >> + dev_err(dev, "reset err %d\n", ret); >> + goto out_mod_clk_unprepare; >> + } >> + } >> +
On Sun, 19 Jun 2016 20:41:09 +0800 icenowy@aosc.xyz wrote: > To be honest, I copied them from sunxi-mmc.c. > > What function should be chosen better? You did the right thing (except for the error detection part). My question was addressed to Philipp (the reset subsystem maintainer). > > > 19.06.2016, 20:06, "Boris Brezillon" <boris.brezillon@free-electrons.com>: > > +Philipp > > > > On Sun, 19 Jun 2016 19:37:39 +0800 > > Icenowy Zheng <icenowy@aosc.xyz> wrote: > > > >> The NAND controller on some sun8i chips needs its reset line to be deasserted > >> before they can enter working state. This commit added the reset line process > >> to the driver. > >> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> > >> --- > >> drivers/mtd/nand/sunxi_nand.c | 14 ++++++++++++++ > >> 1 file changed, 14 insertions(+) > >> > >> diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c > >> index a83a690..1502748 100644 > >> --- a/drivers/mtd/nand/sunxi_nand.c > >> +++ b/drivers/mtd/nand/sunxi_nand.c > >> @@ -39,6 +39,7 @@ > >> #include <linux/gpio.h> > >> #include <linux/interrupt.h> > >> #include <linux/iopoll.h> > >> +#include <linux/reset.h> > >> > >> #define NFC_REG_CTL 0x0000 > >> #define NFC_REG_ST 0x0004 > >> @@ -269,6 +270,7 @@ struct sunxi_nfc { > >> void __iomem *regs; > >> struct clk *ahb_clk; > >> struct clk *mod_clk; > >> + struct reset_control *reset; > >> unsigned long assigned_cs; > >> unsigned long clk_rate; > >> struct list_head chips; > >> @@ -1871,6 +1873,18 @@ static int sunxi_nfc_probe(struct platform_device *pdev) > >> if (ret) > >> goto out_ahb_clk_unprepare; > >> > >> + nfc->reset = devm_reset_control_get_optional(dev, "ahb"); > >> + if (PTR_ERR(nfc->reset) == -EPROBE_DEFER) > >> + return PTR_ERR(nfc->reset); > > > > Actually you should test for != -ENOENT, because all error codes except > > this one should stop the ->probe(). > > > > BTW, this devm_reset_control_get_optional() is really weird. While most > > _optional() methods return NULL when the element is not defined in the > > DT, this one returns -ENOTENT, which makes it impossible to > > differentiate a real error from a undefined reset line (which is a > > valid case for _optional()). > > > > Philipp, is there a good reason for doing that? > > > >> + > >> + if (!IS_ERR(nfc->reset)) { > >> + ret = reset_control_deassert(nfc->reset); > >> + if (ret) { > >> + dev_err(dev, "reset err %d\n", ret); > >> + goto out_mod_clk_unprepare; > >> + } > >> + } > >> +
Then I will soon make the v2 patch set, with the error detection part fixed. But why does sunxi-mmc check only EPROBE_DEFER? 19.06.2016, 20:53, "Boris Brezillon" <boris.brezillon@free-electrons.com>: > On Sun, 19 Jun 2016 20:41:09 +0800 > icenowy@aosc.xyz wrote: > >> To be honest, I copied them from sunxi-mmc.c. >> >> What function should be chosen better? > > You did the right thing (except for the error detection part). My > question was addressed to Philipp (the reset subsystem maintainer). > >> 19.06.2016, 20:06, "Boris Brezillon" <boris.brezillon@free-electrons.com>: >> > +Philipp >> > >> > On Sun, 19 Jun 2016 19:37:39 +0800 >> > Icenowy Zheng <icenowy@aosc.xyz> wrote: >> > >> >> The NAND controller on some sun8i chips needs its reset line to be deasserted >> >> before they can enter working state. This commit added the reset line process >> >> to the driver. >> >> >> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> >> >> --- >> >> drivers/mtd/nand/sunxi_nand.c | 14 ++++++++++++++ >> >> 1 file changed, 14 insertions(+) >> >> >> >> diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c >> >> index a83a690..1502748 100644 >> >> --- a/drivers/mtd/nand/sunxi_nand.c >> >> +++ b/drivers/mtd/nand/sunxi_nand.c >> >> @@ -39,6 +39,7 @@ >> >> #include <linux/gpio.h> >> >> #include <linux/interrupt.h> >> >> #include <linux/iopoll.h> >> >> +#include <linux/reset.h> >> >> >> >> #define NFC_REG_CTL 0x0000 >> >> #define NFC_REG_ST 0x0004 >> >> @@ -269,6 +270,7 @@ struct sunxi_nfc { >> >> void __iomem *regs; >> >> struct clk *ahb_clk; >> >> struct clk *mod_clk; >> >> + struct reset_control *reset; >> >> unsigned long assigned_cs; >> >> unsigned long clk_rate; >> >> struct list_head chips; >> >> @@ -1871,6 +1873,18 @@ static int sunxi_nfc_probe(struct platform_device *pdev) >> >> if (ret) >> >> goto out_ahb_clk_unprepare; >> >> >> >> + nfc->reset = devm_reset_control_get_optional(dev, "ahb"); >> >> + if (PTR_ERR(nfc->reset) == -EPROBE_DEFER) >> >> + return PTR_ERR(nfc->reset); >> > >> > Actually you should test for != -ENOENT, because all error codes except >> > this one should stop the ->probe(). >> > >> > BTW, this devm_reset_control_get_optional() is really weird. While most >> > _optional() methods return NULL when the element is not defined in the >> > DT, this one returns -ENOTENT, which makes it impossible to >> > differentiate a real error from a undefined reset line (which is a >> > valid case for _optional()). >> > >> > Philipp, is there a good reason for doing that? >> > >> >> + >> >> + if (!IS_ERR(nfc->reset)) { >> >> + ret = reset_control_deassert(nfc->reset); >> >> + if (ret) { >> >> + dev_err(dev, "reset err %d\n", ret); >> >> + goto out_mod_clk_unprepare; >> >> + } >> >> + } >> >> +
On Sun, 19 Jun 2016 21:11:00 +0800 icenowy@aosc.xyz wrote: > Then I will soon make the v2 patch set, with the error detection part fixed. > > But why does sunxi-mmc check only EPROBE_DEFER? I guess someone had a probe-dependency problem and fixed this case but ignored all the possible errors. But there may be other reasons for devm_reset_control_get_optional() to fail. The only one that is really reflecting that the reset line is not defined in the DT is -ENOENT. > > 19.06.2016, 20:53, "Boris Brezillon" <boris.brezillon@free-electrons.com>: > > On Sun, 19 Jun 2016 20:41:09 +0800 > > icenowy@aosc.xyz wrote: > > > >> To be honest, I copied them from sunxi-mmc.c. > >> > >> What function should be chosen better? > > > > You did the right thing (except for the error detection part). My > > question was addressed to Philipp (the reset subsystem maintainer). > > > >> 19.06.2016, 20:06, "Boris Brezillon" <boris.brezillon@free-electrons.com>: > >> > +Philipp > >> > > >> > On Sun, 19 Jun 2016 19:37:39 +0800 > >> > Icenowy Zheng <icenowy@aosc.xyz> wrote: > >> > > >> >> The NAND controller on some sun8i chips needs its reset line to be deasserted > >> >> before they can enter working state. This commit added the reset line process > >> >> to the driver. > >> >> > >> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> > >> >> --- > >> >> drivers/mtd/nand/sunxi_nand.c | 14 ++++++++++++++ > >> >> 1 file changed, 14 insertions(+) > >> >> > >> >> diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c > >> >> index a83a690..1502748 100644 > >> >> --- a/drivers/mtd/nand/sunxi_nand.c > >> >> +++ b/drivers/mtd/nand/sunxi_nand.c > >> >> @@ -39,6 +39,7 @@ > >> >> #include <linux/gpio.h> > >> >> #include <linux/interrupt.h> > >> >> #include <linux/iopoll.h> > >> >> +#include <linux/reset.h> > >> >> > >> >> #define NFC_REG_CTL 0x0000 > >> >> #define NFC_REG_ST 0x0004 > >> >> @@ -269,6 +270,7 @@ struct sunxi_nfc { > >> >> void __iomem *regs; > >> >> struct clk *ahb_clk; > >> >> struct clk *mod_clk; > >> >> + struct reset_control *reset; > >> >> unsigned long assigned_cs; > >> >> unsigned long clk_rate; > >> >> struct list_head chips; > >> >> @@ -1871,6 +1873,18 @@ static int sunxi_nfc_probe(struct platform_device *pdev) > >> >> if (ret) > >> >> goto out_ahb_clk_unprepare; > >> >> > >> >> + nfc->reset = devm_reset_control_get_optional(dev, "ahb"); > >> >> + if (PTR_ERR(nfc->reset) == -EPROBE_DEFER) > >> >> + return PTR_ERR(nfc->reset); > >> > > >> > Actually you should test for != -ENOENT, because all error codes except > >> > this one should stop the ->probe(). > >> > > >> > BTW, this devm_reset_control_get_optional() is really weird. While most > >> > _optional() methods return NULL when the element is not defined in the > >> > DT, this one returns -ENOTENT, which makes it impossible to > >> > differentiate a real error from a undefined reset line (which is a > >> > valid case for _optional()). > >> > > >> > Philipp, is there a good reason for doing that? > >> > > >> >> + > >> >> + if (!IS_ERR(nfc->reset)) { > >> >> + ret = reset_control_deassert(nfc->reset); > >> >> + if (ret) { > >> >> + dev_err(dev, "reset err %d\n", ret); > >> >> + goto out_mod_clk_unprepare; > >> >> + } > >> >> + } > >> >> +
Am Sonntag, den 19.06.2016, 14:06 +0200 schrieb Boris Brezillon: > +Philipp > > On Sun, 19 Jun 2016 19:37:39 +0800 > Icenowy Zheng <icenowy@aosc.xyz> wrote: > > > The NAND controller on some sun8i chips needs its reset line to be deasserted > > before they can enter working state. This commit added the reset line process > > to the driver. > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> > > --- > > drivers/mtd/nand/sunxi_nand.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c > > index a83a690..1502748 100644 > > --- a/drivers/mtd/nand/sunxi_nand.c > > +++ b/drivers/mtd/nand/sunxi_nand.c > > @@ -39,6 +39,7 @@ > > #include <linux/gpio.h> > > #include <linux/interrupt.h> > > #include <linux/iopoll.h> > > +#include <linux/reset.h> > > > > #define NFC_REG_CTL 0x0000 > > #define NFC_REG_ST 0x0004 > > @@ -269,6 +270,7 @@ struct sunxi_nfc { > > void __iomem *regs; > > struct clk *ahb_clk; > > struct clk *mod_clk; > > + struct reset_control *reset; > > unsigned long assigned_cs; > > unsigned long clk_rate; > > struct list_head chips; > > @@ -1871,6 +1873,18 @@ static int sunxi_nfc_probe(struct platform_device *pdev) > > if (ret) > > goto out_ahb_clk_unprepare; > > > > + nfc->reset = devm_reset_control_get_optional(dev, "ahb"); > > + if (PTR_ERR(nfc->reset) == -EPROBE_DEFER) > > + return PTR_ERR(nfc->reset); > > Actually you should test for != -ENOENT, because all error codes except > this one should stop the ->probe(). > > BTW, this devm_reset_control_get_optional() is really weird. While most > _optional() methods return NULL when the element is not defined in the > DT, this one returns -ENOTENT, which makes it impossible to > differentiate a real error from a undefined reset line (which is a > valid case for _optional()). Of course it's possible, -ENOENT is only returned if the reset line is not defined in the device tree. Note that gpiod_get_(index_)optional do nothing more that replacing -ENOENT with NULL. And phydev_optional_get replaces -ENODEV with NULL. And regulator_get_optional, if I understand it correctly, never returns NULL. > Philipp, is there a good reason for doing that? Historically, NULL has not been a valid value for rstc. I suppose we could add NULL checks to the reset_control_assert/deassert/reset/status functions and align the reset API a bit with gpiod. I just wouldn't want to see any IS_ERR_OR_NULL error handling in the drivers. regards Philipp
Hi Philipp, On Mon, 20 Jun 2016 14:05:54 +0200 Philipp Zabel <p.zabel@pengutronix.de> wrote: > Am Sonntag, den 19.06.2016, 14:06 +0200 schrieb Boris Brezillon: > > +Philipp > > > > On Sun, 19 Jun 2016 19:37:39 +0800 > > Icenowy Zheng <icenowy@aosc.xyz> wrote: > > > > > The NAND controller on some sun8i chips needs its reset line to be deasserted > > > before they can enter working state. This commit added the reset line process > > > to the driver. > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> > > > --- > > > drivers/mtd/nand/sunxi_nand.c | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c > > > index a83a690..1502748 100644 > > > --- a/drivers/mtd/nand/sunxi_nand.c > > > +++ b/drivers/mtd/nand/sunxi_nand.c > > > @@ -39,6 +39,7 @@ > > > #include <linux/gpio.h> > > > #include <linux/interrupt.h> > > > #include <linux/iopoll.h> > > > +#include <linux/reset.h> > > > > > > #define NFC_REG_CTL 0x0000 > > > #define NFC_REG_ST 0x0004 > > > @@ -269,6 +270,7 @@ struct sunxi_nfc { > > > void __iomem *regs; > > > struct clk *ahb_clk; > > > struct clk *mod_clk; > > > + struct reset_control *reset; > > > unsigned long assigned_cs; > > > unsigned long clk_rate; > > > struct list_head chips; > > > @@ -1871,6 +1873,18 @@ static int sunxi_nfc_probe(struct platform_device *pdev) > > > if (ret) > > > goto out_ahb_clk_unprepare; > > > > > > + nfc->reset = devm_reset_control_get_optional(dev, "ahb"); > > > + if (PTR_ERR(nfc->reset) == -EPROBE_DEFER) > > > + return PTR_ERR(nfc->reset); > > > > Actually you should test for != -ENOENT, because all error codes except > > this one should stop the ->probe(). > > > > BTW, this devm_reset_control_get_optional() is really weird. While most > > _optional() methods return NULL when the element is not defined in the > > DT, this one returns -ENOTENT, which makes it impossible to > > differentiate a real error from a undefined reset line (which is a > > valid case for _optional()). > > Of course it's possible, -ENOENT is only returned if the reset line is > not defined in the device tree. Okay, testing for err != -ENOENT is the right thing to do here. Maybe this could be documented. > > Note that gpiod_get_(index_)optional do nothing more that replacing > -ENOENT with NULL. And phydev_optional_get replaces -ENODEV with NULL. > And regulator_get_optional, if I understand it correctly, never returns > NULL. > > > Philipp, is there a good reason for doing that? > > Historically, NULL has not been a valid value for rstc. I suppose we > could add NULL checks to the reset_control_assert/deassert/reset/status > functions and align the reset API a bit with gpiod. I just wouldn't want > to see any IS_ERR_OR_NULL error handling in the drivers. Well, returning NULL is mainly a convenient way to differentiate real errors from missing optional reset lines (which are not errors). Now, if you say checking for -ENOENT is the right thing to do here, I'm fine with that. Regards, Boris
diff --git a/Documentation/devicetree/bindings/mtd/sunxi-nand.txt b/Documentation/devicetree/bindings/mtd/sunxi-nand.txt index 086d6f4..a328fbb 100644 --- a/Documentation/devicetree/bindings/mtd/sunxi-nand.txt +++ b/Documentation/devicetree/bindings/mtd/sunxi-nand.txt @@ -15,6 +15,8 @@ Optional children nodes: Children nodes represent the available nand chips. Optional properties: +- reset : phandle + reset specifier pair +- reset-names : must contain "ahb" - allwinner,rb : shall contain the native Ready/Busy ids. or - rb-gpios : shall contain the gpios used as R/B pins.
Document the reset lines Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> --- Documentation/devicetree/bindings/mtd/sunxi-nand.txt | 2 ++ 1 file changed, 2 insertions(+)