* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-11 1:06 ` Vladimir Zapolskiy
0 siblings, 0 replies; 48+ messages in thread
From: Vladimir Zapolskiy @ 2016-12-11 1:06 UTC (permalink / raw)
To: Shawn Guo, Wim Van Sebroeck, Guenter Roeck
Cc: Fabio Estevam, Sascha Hauer, Uwe Kleine-König, Magnus Lilja,
linux-watchdog, linux-arm-kernel
Power down counter enable/disable bit switch is located in WMCR
register, but watchdog controllers found on legacy i.MX21, i.MX27 and
i.MX31 SoCs don't have this register. As a result of writing data to
the non-existing register on driver probe the SoC hangs up, to fix the
problem add more OF compatible strings and on this basis get
information about availability of the WMCR register.
Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
Changes from RFC to v1:
* replaced private data struct with a macro as suggested by Guenter,
* updated the comment in the source code to reflect the change,
* rearranged and simplified the logic of detecting WMCR presence,
* pulled the fix out from the series with optional proposed DTS changes.
drivers/watchdog/imx2_wdt.c | 30 +++++++++++++++++++++++++-----
1 file changed, 25 insertions(+), 5 deletions(-)
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 4874b0f..66bd454 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -30,6 +30,7 @@
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/of_address.h>
+#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <linux/watchdog.h>
@@ -57,6 +58,7 @@
#define IMX2_WDT_WICR_WICT 0xFF /* -> Interrupt Count Timeout */
#define IMX2_WDT_WMCR 0x08 /* Misc Register */
+#define IMX2_WDT_NO_WMCR ((void *)true) /* Indicates unavailable WMCR */
#define IMX2_WDT_MAX_TIME 128
#define IMX2_WDT_DEFAULT_TIME 60 /* in seconds */
@@ -70,6 +72,8 @@ struct imx2_wdt_device {
bool ext_reset;
};
+static const struct of_device_id imx2_wdt_dt_ids[];
+
static bool nowayout = WATCHDOG_NOWAYOUT;
module_param(nowayout, bool, 0);
MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
@@ -242,6 +246,7 @@ static const struct regmap_config imx2_wdt_regmap_config = {
static int __init imx2_wdt_probe(struct platform_device *pdev)
{
+ const struct of_device_id *of_id;
struct imx2_wdt_device *wdev;
struct watchdog_device *wdog;
struct resource *res;
@@ -310,11 +315,13 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
}
/*
- * Disable the watchdog power down counter at boot. Otherwise the power
- * down counter will pull down the #WDOG interrupt line for one clock
- * cycle.
+ * If watchdog controller has WMCR, disable the watchdog power
+ * down counter at boot. Otherwise the power down counter will
+ * pull down the #WDOG interrupt line for one clock cycle.
*/
- regmap_write(wdev->regmap, IMX2_WDT_WMCR, 0);
+ of_id = of_match_device(imx2_wdt_dt_ids, &pdev->dev);
+ if (of_id && !of_id->data)
+ regmap_write(wdev->regmap, IMX2_WDT_WMCR, 0);
ret = watchdog_register_device(wdog);
if (ret) {
@@ -412,7 +419,20 @@ static SIMPLE_DEV_PM_OPS(imx2_wdt_pm_ops, imx2_wdt_suspend,
imx2_wdt_resume);
static const struct of_device_id imx2_wdt_dt_ids[] = {
- { .compatible = "fsl,imx21-wdt", },
+ { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
+ { .compatible = "fsl,imx25-wdt", },
+ { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
+ { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
+ { .compatible = "fsl,imx35-wdt", },
+ { .compatible = "fsl,imx50-wdt", },
+ { .compatible = "fsl,imx51-wdt", },
+ { .compatible = "fsl,imx53-wdt", },
+ { .compatible = "fsl,imx6q-wdt", },
+ { .compatible = "fsl,imx6sl-wdt", },
+ { .compatible = "fsl,imx6sx-wdt", },
+ { .compatible = "fsl,imx6ul-wdt", },
+ { .compatible = "fsl,imx7d-wdt", },
+ { .compatible = "fsl,vf610-wdt", },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, imx2_wdt_dt_ids);
--
2.10.2
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-11 1:06 ` Vladimir Zapolskiy
0 siblings, 0 replies; 48+ messages in thread
From: Vladimir Zapolskiy @ 2016-12-11 1:06 UTC (permalink / raw)
To: linux-arm-kernel
Power down counter enable/disable bit switch is located in WMCR
register, but watchdog controllers found on legacy i.MX21, i.MX27 and
i.MX31 SoCs don't have this register. As a result of writing data to
the non-existing register on driver probe the SoC hangs up, to fix the
problem add more OF compatible strings and on this basis get
information about availability of the WMCR register.
Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
Changes from RFC to v1:
* replaced private data struct with a macro as suggested by Guenter,
* updated the comment in the source code to reflect the change,
* rearranged and simplified the logic of detecting WMCR presence,
* pulled the fix out from the series with optional proposed DTS changes.
drivers/watchdog/imx2_wdt.c | 30 +++++++++++++++++++++++++-----
1 file changed, 25 insertions(+), 5 deletions(-)
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 4874b0f..66bd454 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -30,6 +30,7 @@
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/of_address.h>
+#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <linux/watchdog.h>
@@ -57,6 +58,7 @@
#define IMX2_WDT_WICR_WICT 0xFF /* -> Interrupt Count Timeout */
#define IMX2_WDT_WMCR 0x08 /* Misc Register */
+#define IMX2_WDT_NO_WMCR ((void *)true) /* Indicates unavailable WMCR */
#define IMX2_WDT_MAX_TIME 128
#define IMX2_WDT_DEFAULT_TIME 60 /* in seconds */
@@ -70,6 +72,8 @@ struct imx2_wdt_device {
bool ext_reset;
};
+static const struct of_device_id imx2_wdt_dt_ids[];
+
static bool nowayout = WATCHDOG_NOWAYOUT;
module_param(nowayout, bool, 0);
MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
@@ -242,6 +246,7 @@ static const struct regmap_config imx2_wdt_regmap_config = {
static int __init imx2_wdt_probe(struct platform_device *pdev)
{
+ const struct of_device_id *of_id;
struct imx2_wdt_device *wdev;
struct watchdog_device *wdog;
struct resource *res;
@@ -310,11 +315,13 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
}
/*
- * Disable the watchdog power down counter at boot. Otherwise the power
- * down counter will pull down the #WDOG interrupt line for one clock
- * cycle.
+ * If watchdog controller has WMCR, disable the watchdog power
+ * down counter at boot. Otherwise the power down counter will
+ * pull down the #WDOG interrupt line for one clock cycle.
*/
- regmap_write(wdev->regmap, IMX2_WDT_WMCR, 0);
+ of_id = of_match_device(imx2_wdt_dt_ids, &pdev->dev);
+ if (of_id && !of_id->data)
+ regmap_write(wdev->regmap, IMX2_WDT_WMCR, 0);
ret = watchdog_register_device(wdog);
if (ret) {
@@ -412,7 +419,20 @@ static SIMPLE_DEV_PM_OPS(imx2_wdt_pm_ops, imx2_wdt_suspend,
imx2_wdt_resume);
static const struct of_device_id imx2_wdt_dt_ids[] = {
- { .compatible = "fsl,imx21-wdt", },
+ { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
+ { .compatible = "fsl,imx25-wdt", },
+ { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
+ { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
+ { .compatible = "fsl,imx35-wdt", },
+ { .compatible = "fsl,imx50-wdt", },
+ { .compatible = "fsl,imx51-wdt", },
+ { .compatible = "fsl,imx53-wdt", },
+ { .compatible = "fsl,imx6q-wdt", },
+ { .compatible = "fsl,imx6sl-wdt", },
+ { .compatible = "fsl,imx6sx-wdt", },
+ { .compatible = "fsl,imx6ul-wdt", },
+ { .compatible = "fsl,imx7d-wdt", },
+ { .compatible = "fsl,vf610-wdt", },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, imx2_wdt_dt_ids);
--
2.10.2
^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-11 1:06 ` Vladimir Zapolskiy
@ 2016-12-11 2:18 ` Guenter Roeck
-1 siblings, 0 replies; 48+ messages in thread
From: Guenter Roeck @ 2016-12-11 2:18 UTC (permalink / raw)
To: Vladimir Zapolskiy, Shawn Guo, Wim Van Sebroeck
Cc: Fabio Estevam, Sascha Hauer, Uwe Kleine-König, Magnus Lilja,
linux-watchdog, linux-arm-kernel
On 12/10/2016 05:06 PM, Vladimir Zapolskiy wrote:
> Power down counter enable/disable bit switch is located in WMCR
> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
> i.MX31 SoCs don't have this register. As a result of writing data to
> the non-existing register on driver probe the SoC hangs up, to fix the
> problem add more OF compatible strings and on this basis get
> information about availability of the WMCR register.
>
> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Changes from RFC to v1:
> * replaced private data struct with a macro as suggested by Guenter,
> * updated the comment in the source code to reflect the change,
> * rearranged and simplified the logic of detecting WMCR presence,
> * pulled the fix out from the series with optional proposed DTS changes.
>
> drivers/watchdog/imx2_wdt.c | 30 +++++++++++++++++++++++++-----
> 1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index 4874b0f..66bd454 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -30,6 +30,7 @@
> #include <linux/module.h>
> #include <linux/moduleparam.h>
> #include <linux/of_address.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
> #include <linux/watchdog.h>
> @@ -57,6 +58,7 @@
> #define IMX2_WDT_WICR_WICT 0xFF /* -> Interrupt Count Timeout */
>
> #define IMX2_WDT_WMCR 0x08 /* Misc Register */
> +#define IMX2_WDT_NO_WMCR ((void *)true) /* Indicates unavailable WMCR */
>
> #define IMX2_WDT_MAX_TIME 128
> #define IMX2_WDT_DEFAULT_TIME 60 /* in seconds */
> @@ -70,6 +72,8 @@ struct imx2_wdt_device {
> bool ext_reset;
> };
>
> +static const struct of_device_id imx2_wdt_dt_ids[];
> +
> static bool nowayout = WATCHDOG_NOWAYOUT;
> module_param(nowayout, bool, 0);
> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> @@ -242,6 +246,7 @@ static const struct regmap_config imx2_wdt_regmap_config = {
>
> static int __init imx2_wdt_probe(struct platform_device *pdev)
> {
> + const struct of_device_id *of_id;
> struct imx2_wdt_device *wdev;
> struct watchdog_device *wdog;
> struct resource *res;
> @@ -310,11 +315,13 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
> }
>
> /*
> - * Disable the watchdog power down counter at boot. Otherwise the power
> - * down counter will pull down the #WDOG interrupt line for one clock
> - * cycle.
> + * If watchdog controller has WMCR, disable the watchdog power
> + * down counter at boot. Otherwise the power down counter will
> + * pull down the #WDOG interrupt line for one clock cycle.
> */
> - regmap_write(wdev->regmap, IMX2_WDT_WMCR, 0);
> + of_id = of_match_device(imx2_wdt_dt_ids, &pdev->dev);
> + if (of_id && !of_id->data)
> + regmap_write(wdev->regmap, IMX2_WDT_WMCR, 0);
>
> ret = watchdog_register_device(wdog);
> if (ret) {
> @@ -412,7 +419,20 @@ static SIMPLE_DEV_PM_OPS(imx2_wdt_pm_ops, imx2_wdt_suspend,
> imx2_wdt_resume);
>
> static const struct of_device_id imx2_wdt_dt_ids[] = {
> - { .compatible = "fsl,imx21-wdt", },
> + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx25-wdt", },
> + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx35-wdt", },
> + { .compatible = "fsl,imx50-wdt", },
> + { .compatible = "fsl,imx51-wdt", },
> + { .compatible = "fsl,imx53-wdt", },
> + { .compatible = "fsl,imx6q-wdt", },
> + { .compatible = "fsl,imx6sl-wdt", },
> + { .compatible = "fsl,imx6sx-wdt", },
> + { .compatible = "fsl,imx6ul-wdt", },
> + { .compatible = "fsl,imx7d-wdt", },
> + { .compatible = "fsl,vf610-wdt", },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, imx2_wdt_dt_ids);
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-11 2:18 ` Guenter Roeck
0 siblings, 0 replies; 48+ messages in thread
From: Guenter Roeck @ 2016-12-11 2:18 UTC (permalink / raw)
To: linux-arm-kernel
On 12/10/2016 05:06 PM, Vladimir Zapolskiy wrote:
> Power down counter enable/disable bit switch is located in WMCR
> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
> i.MX31 SoCs don't have this register. As a result of writing data to
> the non-existing register on driver probe the SoC hangs up, to fix the
> problem add more OF compatible strings and on this basis get
> information about availability of the WMCR register.
>
> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Changes from RFC to v1:
> * replaced private data struct with a macro as suggested by Guenter,
> * updated the comment in the source code to reflect the change,
> * rearranged and simplified the logic of detecting WMCR presence,
> * pulled the fix out from the series with optional proposed DTS changes.
>
> drivers/watchdog/imx2_wdt.c | 30 +++++++++++++++++++++++++-----
> 1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index 4874b0f..66bd454 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -30,6 +30,7 @@
> #include <linux/module.h>
> #include <linux/moduleparam.h>
> #include <linux/of_address.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
> #include <linux/watchdog.h>
> @@ -57,6 +58,7 @@
> #define IMX2_WDT_WICR_WICT 0xFF /* -> Interrupt Count Timeout */
>
> #define IMX2_WDT_WMCR 0x08 /* Misc Register */
> +#define IMX2_WDT_NO_WMCR ((void *)true) /* Indicates unavailable WMCR */
>
> #define IMX2_WDT_MAX_TIME 128
> #define IMX2_WDT_DEFAULT_TIME 60 /* in seconds */
> @@ -70,6 +72,8 @@ struct imx2_wdt_device {
> bool ext_reset;
> };
>
> +static const struct of_device_id imx2_wdt_dt_ids[];
> +
> static bool nowayout = WATCHDOG_NOWAYOUT;
> module_param(nowayout, bool, 0);
> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> @@ -242,6 +246,7 @@ static const struct regmap_config imx2_wdt_regmap_config = {
>
> static int __init imx2_wdt_probe(struct platform_device *pdev)
> {
> + const struct of_device_id *of_id;
> struct imx2_wdt_device *wdev;
> struct watchdog_device *wdog;
> struct resource *res;
> @@ -310,11 +315,13 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
> }
>
> /*
> - * Disable the watchdog power down counter at boot. Otherwise the power
> - * down counter will pull down the #WDOG interrupt line for one clock
> - * cycle.
> + * If watchdog controller has WMCR, disable the watchdog power
> + * down counter at boot. Otherwise the power down counter will
> + * pull down the #WDOG interrupt line for one clock cycle.
> */
> - regmap_write(wdev->regmap, IMX2_WDT_WMCR, 0);
> + of_id = of_match_device(imx2_wdt_dt_ids, &pdev->dev);
> + if (of_id && !of_id->data)
> + regmap_write(wdev->regmap, IMX2_WDT_WMCR, 0);
>
> ret = watchdog_register_device(wdog);
> if (ret) {
> @@ -412,7 +419,20 @@ static SIMPLE_DEV_PM_OPS(imx2_wdt_pm_ops, imx2_wdt_suspend,
> imx2_wdt_resume);
>
> static const struct of_device_id imx2_wdt_dt_ids[] = {
> - { .compatible = "fsl,imx21-wdt", },
> + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx25-wdt", },
> + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx35-wdt", },
> + { .compatible = "fsl,imx50-wdt", },
> + { .compatible = "fsl,imx51-wdt", },
> + { .compatible = "fsl,imx53-wdt", },
> + { .compatible = "fsl,imx6q-wdt", },
> + { .compatible = "fsl,imx6sl-wdt", },
> + { .compatible = "fsl,imx6sx-wdt", },
> + { .compatible = "fsl,imx6ul-wdt", },
> + { .compatible = "fsl,imx7d-wdt", },
> + { .compatible = "fsl,vf610-wdt", },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, imx2_wdt_dt_ids);
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-11 1:06 ` Vladimir Zapolskiy
@ 2016-12-11 8:58 ` Magnus Lilja
-1 siblings, 0 replies; 48+ messages in thread
From: Magnus Lilja @ 2016-12-11 8:58 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Shawn Guo, Wim Van Sebroeck, Guenter Roeck, Fabio Estevam,
Sascha Hauer, Uwe Kleine-König, linux-watchdog,
linux-arm-kernel
Hi
Excellent!
On 11 December 2016 at 02:06, Vladimir Zapolskiy <vz@mleia.com> wrote:
> Power down counter enable/disable bit switch is located in WMCR
> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
> i.MX31 SoCs don't have this register. As a result of writing data to
> the non-existing register on driver probe the SoC hangs up, to fix the
> problem add more OF compatible strings and on this basis get
> information about availability of the WMCR register.
>
> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
Tested-by: Magnus Lilja <lilja.magnus@gmail.com>
(Tested on i.MX31 without device tree)
/Magnus
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-11 8:58 ` Magnus Lilja
0 siblings, 0 replies; 48+ messages in thread
From: Magnus Lilja @ 2016-12-11 8:58 UTC (permalink / raw)
To: linux-arm-kernel
Hi
Excellent!
On 11 December 2016 at 02:06, Vladimir Zapolskiy <vz@mleia.com> wrote:
> Power down counter enable/disable bit switch is located in WMCR
> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
> i.MX31 SoCs don't have this register. As a result of writing data to
> the non-existing register on driver probe the SoC hangs up, to fix the
> problem add more OF compatible strings and on this basis get
> information about availability of the WMCR register.
>
> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
Tested-by: Magnus Lilja <lilja.magnus@gmail.com>
(Tested on i.MX31 without device tree)
/Magnus
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-11 1:06 ` Vladimir Zapolskiy
@ 2016-12-11 9:35 ` Uwe Kleine-König
-1 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-11 9:35 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Shawn Guo, Wim Van Sebroeck, Guenter Roeck, Fabio Estevam,
Sascha Hauer, Magnus Lilja, linux-watchdog, linux-arm-kernel
On Sun, Dec 11, 2016 at 03:06:48AM +0200, Vladimir Zapolskiy wrote:
> Power down counter enable/disable bit switch is located in WMCR
> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
> i.MX31 SoCs don't have this register. As a result of writing data to
> the non-existing register on driver probe the SoC hangs up, to fix the
> problem add more OF compatible strings and on this basis get
> information about availability of the WMCR register.
>
> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
> Changes from RFC to v1:
> * replaced private data struct with a macro as suggested by Guenter,
> * updated the comment in the source code to reflect the change,
> * rearranged and simplified the logic of detecting WMCR presence,
> * pulled the fix out from the series with optional proposed DTS changes.
>
> drivers/watchdog/imx2_wdt.c | 30 +++++++++++++++++++++++++-----
> 1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index 4874b0f..66bd454 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -30,6 +30,7 @@
> #include <linux/module.h>
> #include <linux/moduleparam.h>
> #include <linux/of_address.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
> #include <linux/watchdog.h>
> @@ -57,6 +58,7 @@
> #define IMX2_WDT_WICR_WICT 0xFF /* -> Interrupt Count Timeout */
>
> #define IMX2_WDT_WMCR 0x08 /* Misc Register */
> +#define IMX2_WDT_NO_WMCR ((void *)true) /* Indicates unavailable WMCR */
>
> #define IMX2_WDT_MAX_TIME 128
> #define IMX2_WDT_DEFAULT_TIME 60 /* in seconds */
> @@ -70,6 +72,8 @@ struct imx2_wdt_device {
> bool ext_reset;
> };
>
> +static const struct of_device_id imx2_wdt_dt_ids[];
> +
> static bool nowayout = WATCHDOG_NOWAYOUT;
> module_param(nowayout, bool, 0);
> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> @@ -242,6 +246,7 @@ static const struct regmap_config imx2_wdt_regmap_config = {
>
> static int __init imx2_wdt_probe(struct platform_device *pdev)
> {
> + const struct of_device_id *of_id;
> struct imx2_wdt_device *wdev;
> struct watchdog_device *wdog;
> struct resource *res;
> @@ -310,11 +315,13 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
> }
>
> /*
> - * Disable the watchdog power down counter at boot. Otherwise the power
> - * down counter will pull down the #WDOG interrupt line for one clock
> - * cycle.
> + * If watchdog controller has WMCR, disable the watchdog power
> + * down counter at boot. Otherwise the power down counter will
> + * pull down the #WDOG interrupt line for one clock cycle.
> */
> - regmap_write(wdev->regmap, IMX2_WDT_WMCR, 0);
> + of_id = of_match_device(imx2_wdt_dt_ids, &pdev->dev);
> + if (of_id && !of_id->data)
> + regmap_write(wdev->regmap, IMX2_WDT_WMCR, 0);
>
> ret = watchdog_register_device(wdog);
> if (ret) {
> @@ -412,7 +419,20 @@ static SIMPLE_DEV_PM_OPS(imx2_wdt_pm_ops, imx2_wdt_suspend,
> imx2_wdt_resume);
>
> static const struct of_device_id imx2_wdt_dt_ids[] = {
> - { .compatible = "fsl,imx21-wdt", },
> + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx25-wdt", },
> + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx35-wdt", },
> + { .compatible = "fsl,imx50-wdt", },
> + { .compatible = "fsl,imx51-wdt", },
> + { .compatible = "fsl,imx53-wdt", },
> + { .compatible = "fsl,imx6q-wdt", },
> + { .compatible = "fsl,imx6sl-wdt", },
> + { .compatible = "fsl,imx6sx-wdt", },
> + { .compatible = "fsl,imx6ul-wdt", },
> + { .compatible = "fsl,imx7d-wdt", },
> + { .compatible = "fsl,vf610-wdt", },
Up to now we only had fsl,imx21-wdt, now it seems that iMX31, i.MX27 and
i.MX21 form a group of equal watchdog IPs and the others form another
group, right?
So conceptually we should differenciate between
fsl,imx21-wdt (which is used on i.MX21, i.MX27 and i.MX31)
fsl,imx35-wdt (which is used on all others)
A reason to add e.g. fsl,imx6sl-wdt is that dtbs in the wild use
"fsl,imx6sl-wdt", "fsl,imx21-wdt"
. But then I wonder if
5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
is that important to justify to add these all.
Independant of this I'd like to have all dtsi for the newer SoCs changed
to get
"fsl,imx6sl-wdt", "fsl,imx35-wdt"
and if you really want to add all these compatibles, add a comment that
these are really fsl,imx35-wdt and were added to work around broken
dtbs.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-11 9:35 ` Uwe Kleine-König
0 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-11 9:35 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Dec 11, 2016 at 03:06:48AM +0200, Vladimir Zapolskiy wrote:
> Power down counter enable/disable bit switch is located in WMCR
> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
> i.MX31 SoCs don't have this register. As a result of writing data to
> the non-existing register on driver probe the SoC hangs up, to fix the
> problem add more OF compatible strings and on this basis get
> information about availability of the WMCR register.
>
> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
> Changes from RFC to v1:
> * replaced private data struct with a macro as suggested by Guenter,
> * updated the comment in the source code to reflect the change,
> * rearranged and simplified the logic of detecting WMCR presence,
> * pulled the fix out from the series with optional proposed DTS changes.
>
> drivers/watchdog/imx2_wdt.c | 30 +++++++++++++++++++++++++-----
> 1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index 4874b0f..66bd454 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -30,6 +30,7 @@
> #include <linux/module.h>
> #include <linux/moduleparam.h>
> #include <linux/of_address.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
> #include <linux/watchdog.h>
> @@ -57,6 +58,7 @@
> #define IMX2_WDT_WICR_WICT 0xFF /* -> Interrupt Count Timeout */
>
> #define IMX2_WDT_WMCR 0x08 /* Misc Register */
> +#define IMX2_WDT_NO_WMCR ((void *)true) /* Indicates unavailable WMCR */
>
> #define IMX2_WDT_MAX_TIME 128
> #define IMX2_WDT_DEFAULT_TIME 60 /* in seconds */
> @@ -70,6 +72,8 @@ struct imx2_wdt_device {
> bool ext_reset;
> };
>
> +static const struct of_device_id imx2_wdt_dt_ids[];
> +
> static bool nowayout = WATCHDOG_NOWAYOUT;
> module_param(nowayout, bool, 0);
> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> @@ -242,6 +246,7 @@ static const struct regmap_config imx2_wdt_regmap_config = {
>
> static int __init imx2_wdt_probe(struct platform_device *pdev)
> {
> + const struct of_device_id *of_id;
> struct imx2_wdt_device *wdev;
> struct watchdog_device *wdog;
> struct resource *res;
> @@ -310,11 +315,13 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
> }
>
> /*
> - * Disable the watchdog power down counter at boot. Otherwise the power
> - * down counter will pull down the #WDOG interrupt line for one clock
> - * cycle.
> + * If watchdog controller has WMCR, disable the watchdog power
> + * down counter at boot. Otherwise the power down counter will
> + * pull down the #WDOG interrupt line for one clock cycle.
> */
> - regmap_write(wdev->regmap, IMX2_WDT_WMCR, 0);
> + of_id = of_match_device(imx2_wdt_dt_ids, &pdev->dev);
> + if (of_id && !of_id->data)
> + regmap_write(wdev->regmap, IMX2_WDT_WMCR, 0);
>
> ret = watchdog_register_device(wdog);
> if (ret) {
> @@ -412,7 +419,20 @@ static SIMPLE_DEV_PM_OPS(imx2_wdt_pm_ops, imx2_wdt_suspend,
> imx2_wdt_resume);
>
> static const struct of_device_id imx2_wdt_dt_ids[] = {
> - { .compatible = "fsl,imx21-wdt", },
> + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx25-wdt", },
> + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx35-wdt", },
> + { .compatible = "fsl,imx50-wdt", },
> + { .compatible = "fsl,imx51-wdt", },
> + { .compatible = "fsl,imx53-wdt", },
> + { .compatible = "fsl,imx6q-wdt", },
> + { .compatible = "fsl,imx6sl-wdt", },
> + { .compatible = "fsl,imx6sx-wdt", },
> + { .compatible = "fsl,imx6ul-wdt", },
> + { .compatible = "fsl,imx7d-wdt", },
> + { .compatible = "fsl,vf610-wdt", },
Up to now we only had fsl,imx21-wdt, now it seems that iMX31, i.MX27 and
i.MX21 form a group of equal watchdog IPs and the others form another
group, right?
So conceptually we should differenciate between
fsl,imx21-wdt (which is used on i.MX21, i.MX27 and i.MX31)
fsl,imx35-wdt (which is used on all others)
A reason to add e.g. fsl,imx6sl-wdt is that dtbs in the wild use
"fsl,imx6sl-wdt", "fsl,imx21-wdt"
. But then I wonder if
5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
is that important to justify to add these all.
Independant of this I'd like to have all dtsi for the newer SoCs changed
to get
"fsl,imx6sl-wdt", "fsl,imx35-wdt"
and if you really want to add all these compatibles, add a comment that
these are really fsl,imx35-wdt and were added to work around broken
dtbs.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-11 9:35 ` Uwe Kleine-König
@ 2016-12-11 10:01 ` Vladimir Zapolskiy
-1 siblings, 0 replies; 48+ messages in thread
From: Vladimir Zapolskiy @ 2016-12-11 10:01 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Shawn Guo, Wim Van Sebroeck, Guenter Roeck, Fabio Estevam,
Sascha Hauer, Magnus Lilja, linux-watchdog, linux-arm-kernel
Hello Uwe,
On 12/11/2016 11:35 AM, Uwe Kleine-König wrote:
> On Sun, Dec 11, 2016 at 03:06:48AM +0200, Vladimir Zapolskiy wrote:
>> Power down counter enable/disable bit switch is located in WMCR
>> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
>> i.MX31 SoCs don't have this register. As a result of writing data to
>> the non-existing register on driver probe the SoC hangs up, to fix the
>> problem add more OF compatible strings and on this basis get
>> information about availability of the WMCR register.
>>
>> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>> ---
[snip]
>>
>> static const struct of_device_id imx2_wdt_dt_ids[] = {
>> - { .compatible = "fsl,imx21-wdt", },
>> + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
>> + { .compatible = "fsl,imx25-wdt", },
>> + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
>> + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
>> + { .compatible = "fsl,imx35-wdt", },
>> + { .compatible = "fsl,imx50-wdt", },
>> + { .compatible = "fsl,imx51-wdt", },
>> + { .compatible = "fsl,imx53-wdt", },
>> + { .compatible = "fsl,imx6q-wdt", },
>> + { .compatible = "fsl,imx6sl-wdt", },
>> + { .compatible = "fsl,imx6sx-wdt", },
>> + { .compatible = "fsl,imx6ul-wdt", },
>> + { .compatible = "fsl,imx7d-wdt", },
>> + { .compatible = "fsl,vf610-wdt", },
>
> Up to now we only had fsl,imx21-wdt, now it seems that iMX31, i.MX27 and
> i.MX21 form a group of equal watchdog IPs and the others form another
> group, right?
>
> So conceptually we should differenciate between
>
> fsl,imx21-wdt (which is used on i.MX21, i.MX27 and i.MX31)
> fsl,imx35-wdt (which is used on all others)
>
The problem is that "fsl,imx35-wdt" is not used on all others in the
existing DTS, i.e. in the old firmware, which shall be compatible with
new changes.
$ grep -H 'fsl,imx.*-wdt' arch/arm/boot/dts/*.dtsi
arch/arm/boot/dts/imx25.dtsi: compatible = "fsl,imx25-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx27.dtsi: compatible = "fsl,imx27-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx35.dtsi: compatible = "fsl,imx35-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx50.dtsi: compatible = "fsl,imx50-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/ls1021a.dtsi: compatible = "fsl,imx21-wdt";
arch/arm/boot/dts/vfxxx.dtsi: compatible = "fsl,vf610-wdt", "fsl,imx21-wdt";
I don't see a confirmation of the fact that "fsl,imx35-wdt" is the best
selection, hence definitely "fsl,imx25-wdt" overscores it.
> A reason to add e.g. fsl,imx6sl-wdt is that dtbs in the wild use
>
> "fsl,imx6sl-wdt", "fsl,imx21-wdt"
>
> . But then I wonder if
>
> 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
>
> is that important to justify to add these all.
About this comment I don't know, you may have better insight about the original
change. By the way, in barebox you may want to fix the same hang-up, because
you touch WMCR unconditionally.
>
> Independant of this I'd like to have all dtsi for the newer SoCs changed
> to get
>
> "fsl,imx6sl-wdt", "fsl,imx35-wdt"
>
> and if you really want to add all these compatibles, add a comment that
> these are really fsl,imx35-wdt and were added to work around broken
> dtbs.
No objections, but
1) I'd like to see "fsl,imx25-wdt" as a new common compatible, and that's
what have been proposed and shared in RFC 2/2.
2) Please remind me, why do you ask to drop "fsl,imx21-wdt" from the list?
compatible = "fsl,imx6sx-wdt", "fsl,imx25-wdt", "fsl,imx21-wdt";
The list of compatibles above is valid (from the most specific on the left
to the most generic on the right), and that compatible property is rightly
handled in the driver with this change applied. I don't see a need to
drop "fsl,imx21-wdt".
As a reminder while changing the source code new (updated) kernel must be
compatible with the old (not updated) DTB firmware, and not the other way
round.
--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-11 10:01 ` Vladimir Zapolskiy
0 siblings, 0 replies; 48+ messages in thread
From: Vladimir Zapolskiy @ 2016-12-11 10:01 UTC (permalink / raw)
To: linux-arm-kernel
Hello Uwe,
On 12/11/2016 11:35 AM, Uwe Kleine-K?nig wrote:
> On Sun, Dec 11, 2016 at 03:06:48AM +0200, Vladimir Zapolskiy wrote:
>> Power down counter enable/disable bit switch is located in WMCR
>> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
>> i.MX31 SoCs don't have this register. As a result of writing data to
>> the non-existing register on driver probe the SoC hangs up, to fix the
>> problem add more OF compatible strings and on this basis get
>> information about availability of the WMCR register.
>>
>> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>> ---
[snip]
>>
>> static const struct of_device_id imx2_wdt_dt_ids[] = {
>> - { .compatible = "fsl,imx21-wdt", },
>> + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
>> + { .compatible = "fsl,imx25-wdt", },
>> + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
>> + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
>> + { .compatible = "fsl,imx35-wdt", },
>> + { .compatible = "fsl,imx50-wdt", },
>> + { .compatible = "fsl,imx51-wdt", },
>> + { .compatible = "fsl,imx53-wdt", },
>> + { .compatible = "fsl,imx6q-wdt", },
>> + { .compatible = "fsl,imx6sl-wdt", },
>> + { .compatible = "fsl,imx6sx-wdt", },
>> + { .compatible = "fsl,imx6ul-wdt", },
>> + { .compatible = "fsl,imx7d-wdt", },
>> + { .compatible = "fsl,vf610-wdt", },
>
> Up to now we only had fsl,imx21-wdt, now it seems that iMX31, i.MX27 and
> i.MX21 form a group of equal watchdog IPs and the others form another
> group, right?
>
> So conceptually we should differenciate between
>
> fsl,imx21-wdt (which is used on i.MX21, i.MX27 and i.MX31)
> fsl,imx35-wdt (which is used on all others)
>
The problem is that "fsl,imx35-wdt" is not used on all others in the
existing DTS, i.e. in the old firmware, which shall be compatible with
new changes.
$ grep -H 'fsl,imx.*-wdt' arch/arm/boot/dts/*.dtsi
arch/arm/boot/dts/imx25.dtsi: compatible = "fsl,imx25-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx27.dtsi: compatible = "fsl,imx27-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx35.dtsi: compatible = "fsl,imx35-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx50.dtsi: compatible = "fsl,imx50-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
arch/arm/boot/dts/ls1021a.dtsi: compatible = "fsl,imx21-wdt";
arch/arm/boot/dts/vfxxx.dtsi: compatible = "fsl,vf610-wdt", "fsl,imx21-wdt";
I don't see a confirmation of the fact that "fsl,imx35-wdt" is the best
selection, hence definitely "fsl,imx25-wdt" overscores it.
> A reason to add e.g. fsl,imx6sl-wdt is that dtbs in the wild use
>
> "fsl,imx6sl-wdt", "fsl,imx21-wdt"
>
> . But then I wonder if
>
> 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
>
> is that important to justify to add these all.
About this comment I don't know, you may have better insight about the original
change. By the way, in barebox you may want to fix the same hang-up, because
you touch WMCR unconditionally.
>
> Independant of this I'd like to have all dtsi for the newer SoCs changed
> to get
>
> "fsl,imx6sl-wdt", "fsl,imx35-wdt"
>
> and if you really want to add all these compatibles, add a comment that
> these are really fsl,imx35-wdt and were added to work around broken
> dtbs.
No objections, but
1) I'd like to see "fsl,imx25-wdt" as a new common compatible, and that's
what have been proposed and shared in RFC 2/2.
2) Please remind me, why do you ask to drop "fsl,imx21-wdt" from the list?
compatible = "fsl,imx6sx-wdt", "fsl,imx25-wdt", "fsl,imx21-wdt";
The list of compatibles above is valid (from the most specific on the left
to the most generic on the right), and that compatible property is rightly
handled in the driver with this change applied. I don't see a need to
drop "fsl,imx21-wdt".
As a reminder while changing the source code new (updated) kernel must be
compatible with the old (not updated) DTB firmware, and not the other way
round.
--
With best wishes,
Vladimir
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-11 10:01 ` Vladimir Zapolskiy
@ 2016-12-11 10:26 ` Uwe Kleine-König
-1 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-11 10:26 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Shawn Guo, Wim Van Sebroeck, Guenter Roeck, Fabio Estevam,
Sascha Hauer, Magnus Lilja, linux-watchdog, linux-arm-kernel
Hello Vladimir,
On Sun, Dec 11, 2016 at 12:01:08PM +0200, Vladimir Zapolskiy wrote:
> On 12/11/2016 11:35 AM, Uwe Kleine-König wrote:
> > On Sun, Dec 11, 2016 at 03:06:48AM +0200, Vladimir Zapolskiy wrote:
> >> Power down counter enable/disable bit switch is located in WMCR
> >> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
> >> i.MX31 SoCs don't have this register. As a result of writing data to
> >> the non-existing register on driver probe the SoC hangs up, to fix the
> >> problem add more OF compatible strings and on this basis get
> >> information about availability of the WMCR register.
> >>
> >> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
> >> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> >> ---
>
> [snip]
>
> >>
> >> static const struct of_device_id imx2_wdt_dt_ids[] = {
> >> - { .compatible = "fsl,imx21-wdt", },
> >> + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
> >> + { .compatible = "fsl,imx25-wdt", },
> >> + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
> >> + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
> >> + { .compatible = "fsl,imx35-wdt", },
> >> + { .compatible = "fsl,imx50-wdt", },
> >> + { .compatible = "fsl,imx51-wdt", },
> >> + { .compatible = "fsl,imx53-wdt", },
> >> + { .compatible = "fsl,imx6q-wdt", },
> >> + { .compatible = "fsl,imx6sl-wdt", },
> >> + { .compatible = "fsl,imx6sx-wdt", },
> >> + { .compatible = "fsl,imx6ul-wdt", },
> >> + { .compatible = "fsl,imx7d-wdt", },
> >> + { .compatible = "fsl,vf610-wdt", },
> >
> > Up to now we only had fsl,imx21-wdt, now it seems that iMX31, i.MX27 and
> > i.MX21 form a group of equal watchdog IPs and the others form another
> > group, right?
> >
> > So conceptually we should differenciate between
> >
> > fsl,imx21-wdt (which is used on i.MX21, i.MX27 and i.MX31)
> > fsl,imx35-wdt (which is used on all others)
> >
>
> The problem is that "fsl,imx35-wdt" is not used on all others in the
> existing DTS, i.e. in the old firmware, which shall be compatible with
> new changes.
So old i.MX53 has
compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
. With the change to the driver it's like using the watchdog driver in
it's pre-5fe65ce7ccbb4-state on it. If this is bad, we must indeed make
fsl,imx53-wdt known to the driver. If not, just adding a single new
compatible to the driver is just fine.
If you want to call it imx25 or imx35 doesn't matter much. The reason I
picked imx35 is that this one was already picked before for cspi. This
suggests that i.MX35 is older than i.MX25 and so this is the canonical
choice.
> $ grep -H 'fsl,imx.*-wdt' arch/arm/boot/dts/*.dtsi
> arch/arm/boot/dts/imx25.dtsi: compatible = "fsl,imx25-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx27.dtsi: compatible = "fsl,imx27-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx35.dtsi: compatible = "fsl,imx35-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx50.dtsi: compatible = "fsl,imx50-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/ls1021a.dtsi: compatible = "fsl,imx21-wdt";
> arch/arm/boot/dts/vfxxx.dtsi: compatible = "fsl,vf610-wdt", "fsl,imx21-wdt";
>
> I don't see a confirmation of the fact that "fsl,imx35-wdt" is the best
> selection, hence definitely "fsl,imx25-wdt" overscores it.
I stated my reason to pick imx35 over imx25 above. Why do yo prefer
imx25?
> > A reason to add e.g. fsl,imx6sl-wdt is that dtbs in the wild use
> >
> > "fsl,imx6sl-wdt", "fsl,imx21-wdt"
> >
> > . But then I wonder if
> >
> > 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
> >
> > is that important to justify to add these all.
>
> About this comment I don't know, you may have better insight about the original
> change. By the way, in barebox you may want to fix the same hang-up, because
> you touch WMCR unconditionally.
Sure.
> > Independant of this I'd like to have all dtsi for the newer SoCs changed
> > to get
> >
> > "fsl,imx6sl-wdt", "fsl,imx35-wdt"
> >
> > and if you really want to add all these compatibles, add a comment that
> > these are really fsl,imx35-wdt and were added to work around broken
> > dtbs.
>
> No objections, but
>
> 1) I'd like to see "fsl,imx25-wdt" as a new common compatible, and that's
> what have been proposed and shared in RFC 2/2.
Yeah, I missed that other patch (which was not part of this series).
> 2) Please remind me, why do you ask to drop "fsl,imx21-wdt" from the list?
>
> compatible = "fsl,imx6sx-wdt", "fsl,imx25-wdt", "fsl,imx21-wdt";
>
> The list of compatibles above is valid (from the most specific on the left
> to the most generic on the right), and that compatible property is rightly
> handled in the driver with this change applied. I don't see a need to
> drop "fsl,imx21-wdt".
If the wdt in the i.MX6SX can be operated by the watchdog driver in it's
imx21 mode, you should keep the imx21 entry. If not, you shouldn't.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-11 10:26 ` Uwe Kleine-König
0 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-11 10:26 UTC (permalink / raw)
To: linux-arm-kernel
Hello Vladimir,
On Sun, Dec 11, 2016 at 12:01:08PM +0200, Vladimir Zapolskiy wrote:
> On 12/11/2016 11:35 AM, Uwe Kleine-K?nig wrote:
> > On Sun, Dec 11, 2016 at 03:06:48AM +0200, Vladimir Zapolskiy wrote:
> >> Power down counter enable/disable bit switch is located in WMCR
> >> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
> >> i.MX31 SoCs don't have this register. As a result of writing data to
> >> the non-existing register on driver probe the SoC hangs up, to fix the
> >> problem add more OF compatible strings and on this basis get
> >> information about availability of the WMCR register.
> >>
> >> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
> >> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> >> ---
>
> [snip]
>
> >>
> >> static const struct of_device_id imx2_wdt_dt_ids[] = {
> >> - { .compatible = "fsl,imx21-wdt", },
> >> + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
> >> + { .compatible = "fsl,imx25-wdt", },
> >> + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
> >> + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
> >> + { .compatible = "fsl,imx35-wdt", },
> >> + { .compatible = "fsl,imx50-wdt", },
> >> + { .compatible = "fsl,imx51-wdt", },
> >> + { .compatible = "fsl,imx53-wdt", },
> >> + { .compatible = "fsl,imx6q-wdt", },
> >> + { .compatible = "fsl,imx6sl-wdt", },
> >> + { .compatible = "fsl,imx6sx-wdt", },
> >> + { .compatible = "fsl,imx6ul-wdt", },
> >> + { .compatible = "fsl,imx7d-wdt", },
> >> + { .compatible = "fsl,vf610-wdt", },
> >
> > Up to now we only had fsl,imx21-wdt, now it seems that iMX31, i.MX27 and
> > i.MX21 form a group of equal watchdog IPs and the others form another
> > group, right?
> >
> > So conceptually we should differenciate between
> >
> > fsl,imx21-wdt (which is used on i.MX21, i.MX27 and i.MX31)
> > fsl,imx35-wdt (which is used on all others)
> >
>
> The problem is that "fsl,imx35-wdt" is not used on all others in the
> existing DTS, i.e. in the old firmware, which shall be compatible with
> new changes.
So old i.MX53 has
compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
. With the change to the driver it's like using the watchdog driver in
it's pre-5fe65ce7ccbb4-state on it. If this is bad, we must indeed make
fsl,imx53-wdt known to the driver. If not, just adding a single new
compatible to the driver is just fine.
If you want to call it imx25 or imx35 doesn't matter much. The reason I
picked imx35 is that this one was already picked before for cspi. This
suggests that i.MX35 is older than i.MX25 and so this is the canonical
choice.
> $ grep -H 'fsl,imx.*-wdt' arch/arm/boot/dts/*.dtsi
> arch/arm/boot/dts/imx25.dtsi: compatible = "fsl,imx25-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx27.dtsi: compatible = "fsl,imx27-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx35.dtsi: compatible = "fsl,imx35-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx50.dtsi: compatible = "fsl,imx50-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/ls1021a.dtsi: compatible = "fsl,imx21-wdt";
> arch/arm/boot/dts/vfxxx.dtsi: compatible = "fsl,vf610-wdt", "fsl,imx21-wdt";
>
> I don't see a confirmation of the fact that "fsl,imx35-wdt" is the best
> selection, hence definitely "fsl,imx25-wdt" overscores it.
I stated my reason to pick imx35 over imx25 above. Why do yo prefer
imx25?
> > A reason to add e.g. fsl,imx6sl-wdt is that dtbs in the wild use
> >
> > "fsl,imx6sl-wdt", "fsl,imx21-wdt"
> >
> > . But then I wonder if
> >
> > 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
> >
> > is that important to justify to add these all.
>
> About this comment I don't know, you may have better insight about the original
> change. By the way, in barebox you may want to fix the same hang-up, because
> you touch WMCR unconditionally.
Sure.
> > Independant of this I'd like to have all dtsi for the newer SoCs changed
> > to get
> >
> > "fsl,imx6sl-wdt", "fsl,imx35-wdt"
> >
> > and if you really want to add all these compatibles, add a comment that
> > these are really fsl,imx35-wdt and were added to work around broken
> > dtbs.
>
> No objections, but
>
> 1) I'd like to see "fsl,imx25-wdt" as a new common compatible, and that's
> what have been proposed and shared in RFC 2/2.
Yeah, I missed that other patch (which was not part of this series).
> 2) Please remind me, why do you ask to drop "fsl,imx21-wdt" from the list?
>
> compatible = "fsl,imx6sx-wdt", "fsl,imx25-wdt", "fsl,imx21-wdt";
>
> The list of compatibles above is valid (from the most specific on the left
> to the most generic on the right), and that compatible property is rightly
> handled in the driver with this change applied. I don't see a need to
> drop "fsl,imx21-wdt".
If the wdt in the i.MX6SX can be operated by the watchdog driver in it's
imx21 mode, you should keep the imx21 entry. If not, you shouldn't.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-11 10:26 ` Uwe Kleine-König
@ 2016-12-11 11:21 ` Vladimir Zapolskiy
-1 siblings, 0 replies; 48+ messages in thread
From: Vladimir Zapolskiy @ 2016-12-11 11:21 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Shawn Guo, Wim Van Sebroeck, Guenter Roeck, Fabio Estevam,
Sascha Hauer, Magnus Lilja, linux-watchdog, linux-arm-kernel
On 12/11/2016 12:26 PM, Uwe Kleine-König wrote:
> Hello Vladimir,
>
> On Sun, Dec 11, 2016 at 12:01:08PM +0200, Vladimir Zapolskiy wrote:
>> On 12/11/2016 11:35 AM, Uwe Kleine-König wrote:
>>> On Sun, Dec 11, 2016 at 03:06:48AM +0200, Vladimir Zapolskiy wrote:
>>>> Power down counter enable/disable bit switch is located in WMCR
>>>> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
>>>> i.MX31 SoCs don't have this register. As a result of writing data to
>>>> the non-existing register on driver probe the SoC hangs up, to fix the
>>>> problem add more OF compatible strings and on this basis get
>>>> information about availability of the WMCR register.
>>>>
>>>> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
>>>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>>>> ---
>>
>> [snip]
>>
>>>>
>>>> static const struct of_device_id imx2_wdt_dt_ids[] = {
>>>> - { .compatible = "fsl,imx21-wdt", },
>>>> + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
>>>> + { .compatible = "fsl,imx25-wdt", },
>>>> + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
>>>> + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
>>>> + { .compatible = "fsl,imx35-wdt", },
>>>> + { .compatible = "fsl,imx50-wdt", },
>>>> + { .compatible = "fsl,imx51-wdt", },
>>>> + { .compatible = "fsl,imx53-wdt", },
>>>> + { .compatible = "fsl,imx6q-wdt", },
>>>> + { .compatible = "fsl,imx6sl-wdt", },
>>>> + { .compatible = "fsl,imx6sx-wdt", },
>>>> + { .compatible = "fsl,imx6ul-wdt", },
>>>> + { .compatible = "fsl,imx7d-wdt", },
>>>> + { .compatible = "fsl,vf610-wdt", },
>>>
>>> Up to now we only had fsl,imx21-wdt, now it seems that iMX31, i.MX27 and
>>> i.MX21 form a group of equal watchdog IPs and the others form another
>>> group, right?
>>>
>>> So conceptually we should differenciate between
>>>
>>> fsl,imx21-wdt (which is used on i.MX21, i.MX27 and i.MX31)
>>> fsl,imx35-wdt (which is used on all others)
>>>
>>
>> The problem is that "fsl,imx35-wdt" is not used on all others in the
>> existing DTS, i.e. in the old firmware, which shall be compatible with
>> new changes.
>
> So old i.MX53 has
>
> compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
>
> . With the change to the driver it's like using the watchdog driver in
> it's pre-5fe65ce7ccbb4-state on it. If this is bad, we must indeed make
> fsl,imx53-wdt known to the driver. If not, just adding a single new
> compatible to the driver is just fine.
The change under discussion preserves the current runtime behaviour for
i.MX53 and its friends, the watchdog power down counter in WMCR is
disabled on boot (you may want to confirm it by testing though), another
question is if it is wanted for e.g. i.MX53 right from the beginning.
To keep the runtime compatibility of a newer kernel with old DTBs such
a long list of device compatibles has to be inserted.
>
> If you want to call it imx25 or imx35 doesn't matter much. The reason I
> picked imx35 is that this one was already picked before for cspi. This
> suggests that i.MX35 is older than i.MX25 and so this is the canonical
> choice.
Accepted, from the Wikipedia both i.MX25 and i.MX35 are released in 2009,
which one is the first I don't know for sure, I supposed it could be
i.MX25 as a SoC with a weaker core.
>
>> $ grep -H 'fsl,imx.*-wdt' arch/arm/boot/dts/*.dtsi
>> arch/arm/boot/dts/imx25.dtsi: compatible = "fsl,imx25-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx27.dtsi: compatible = "fsl,imx27-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx35.dtsi: compatible = "fsl,imx35-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx50.dtsi: compatible = "fsl,imx50-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/ls1021a.dtsi: compatible = "fsl,imx21-wdt";
>> arch/arm/boot/dts/vfxxx.dtsi: compatible = "fsl,vf610-wdt", "fsl,imx21-wdt";
>>
>> I don't see a confirmation of the fact that "fsl,imx35-wdt" is the best
>> selection, hence definitely "fsl,imx25-wdt" overscores it.
>
> I stated my reason to pick imx35 over imx25 above. Why do yo prefer
> imx25?
As you said right now and until it is added into DTS files "fsl,imx25-wdt" and
"fsl,imx35-wdt" are equal as a compatible representation of WMCR-aware watchdogs.
I may be wrong, if I assume that i.MX25 is released before i.MX35 (looks like
they are released in parallel), but "imx25" precedes in alphanumerically sorted
list of SoC names.
>>> A reason to add e.g. fsl,imx6sl-wdt is that dtbs in the wild use
>>>
>>> "fsl,imx6sl-wdt", "fsl,imx21-wdt"
>>>
>>> . But then I wonder if
>>>
>>> 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
>>>
>>> is that important to justify to add these all.
>>
>> About this comment I don't know, you may have better insight about the original
>> change. By the way, in barebox you may want to fix the same hang-up, because
>> you touch WMCR unconditionally.
>
> Sure.
>
>>> Independant of this I'd like to have all dtsi for the newer SoCs changed
>>> to get
>>>
>>> "fsl,imx6sl-wdt", "fsl,imx35-wdt"
>>>
>>> and if you really want to add all these compatibles, add a comment that
>>> these are really fsl,imx35-wdt and were added to work around broken
>>> dtbs.
>>
>> No objections, but
>>
>> 1) I'd like to see "fsl,imx25-wdt" as a new common compatible, and that's
>> what have been proposed and shared in RFC 2/2.
>
> Yeah, I missed that other patch (which was not part of this series).
>
>> 2) Please remind me, why do you ask to drop "fsl,imx21-wdt" from the list?
>>
>> compatible = "fsl,imx6sx-wdt", "fsl,imx25-wdt", "fsl,imx21-wdt";
>>
>> The list of compatibles above is valid (from the most specific on the left
>> to the most generic on the right), and that compatible property is rightly
>> handled in the driver with this change applied. I don't see a need to
>> drop "fsl,imx21-wdt".
>
> If the wdt in the i.MX6SX can be operated by the watchdog driver in it's
> imx21 mode, you should keep the imx21 entry. If not, you shouldn't.
The DTS description shall be rather independent from any particular software
implementation, someone may want to reuse the same DTB in bootloaders and OS
kernels of different versions. Assuming that I have a bootloader/kernel
with a pure i.MX21 watchdog driver and run it on i.MX6SX, I'd prefer to match
"fsl,imx21-wdt" compatible.
--
With best wishes,
Vladimir
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-11 11:21 ` Vladimir Zapolskiy
0 siblings, 0 replies; 48+ messages in thread
From: Vladimir Zapolskiy @ 2016-12-11 11:21 UTC (permalink / raw)
To: linux-arm-kernel
On 12/11/2016 12:26 PM, Uwe Kleine-K?nig wrote:
> Hello Vladimir,
>
> On Sun, Dec 11, 2016 at 12:01:08PM +0200, Vladimir Zapolskiy wrote:
>> On 12/11/2016 11:35 AM, Uwe Kleine-K?nig wrote:
>>> On Sun, Dec 11, 2016 at 03:06:48AM +0200, Vladimir Zapolskiy wrote:
>>>> Power down counter enable/disable bit switch is located in WMCR
>>>> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
>>>> i.MX31 SoCs don't have this register. As a result of writing data to
>>>> the non-existing register on driver probe the SoC hangs up, to fix the
>>>> problem add more OF compatible strings and on this basis get
>>>> information about availability of the WMCR register.
>>>>
>>>> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
>>>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>>>> ---
>>
>> [snip]
>>
>>>>
>>>> static const struct of_device_id imx2_wdt_dt_ids[] = {
>>>> - { .compatible = "fsl,imx21-wdt", },
>>>> + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
>>>> + { .compatible = "fsl,imx25-wdt", },
>>>> + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
>>>> + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
>>>> + { .compatible = "fsl,imx35-wdt", },
>>>> + { .compatible = "fsl,imx50-wdt", },
>>>> + { .compatible = "fsl,imx51-wdt", },
>>>> + { .compatible = "fsl,imx53-wdt", },
>>>> + { .compatible = "fsl,imx6q-wdt", },
>>>> + { .compatible = "fsl,imx6sl-wdt", },
>>>> + { .compatible = "fsl,imx6sx-wdt", },
>>>> + { .compatible = "fsl,imx6ul-wdt", },
>>>> + { .compatible = "fsl,imx7d-wdt", },
>>>> + { .compatible = "fsl,vf610-wdt", },
>>>
>>> Up to now we only had fsl,imx21-wdt, now it seems that iMX31, i.MX27 and
>>> i.MX21 form a group of equal watchdog IPs and the others form another
>>> group, right?
>>>
>>> So conceptually we should differenciate between
>>>
>>> fsl,imx21-wdt (which is used on i.MX21, i.MX27 and i.MX31)
>>> fsl,imx35-wdt (which is used on all others)
>>>
>>
>> The problem is that "fsl,imx35-wdt" is not used on all others in the
>> existing DTS, i.e. in the old firmware, which shall be compatible with
>> new changes.
>
> So old i.MX53 has
>
> compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
>
> . With the change to the driver it's like using the watchdog driver in
> it's pre-5fe65ce7ccbb4-state on it. If this is bad, we must indeed make
> fsl,imx53-wdt known to the driver. If not, just adding a single new
> compatible to the driver is just fine.
The change under discussion preserves the current runtime behaviour for
i.MX53 and its friends, the watchdog power down counter in WMCR is
disabled on boot (you may want to confirm it by testing though), another
question is if it is wanted for e.g. i.MX53 right from the beginning.
To keep the runtime compatibility of a newer kernel with old DTBs such
a long list of device compatibles has to be inserted.
>
> If you want to call it imx25 or imx35 doesn't matter much. The reason I
> picked imx35 is that this one was already picked before for cspi. This
> suggests that i.MX35 is older than i.MX25 and so this is the canonical
> choice.
Accepted, from the Wikipedia both i.MX25 and i.MX35 are released in 2009,
which one is the first I don't know for sure, I supposed it could be
i.MX25 as a SoC with a weaker core.
>
>> $ grep -H 'fsl,imx.*-wdt' arch/arm/boot/dts/*.dtsi
>> arch/arm/boot/dts/imx25.dtsi: compatible = "fsl,imx25-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx27.dtsi: compatible = "fsl,imx27-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx35.dtsi: compatible = "fsl,imx35-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx50.dtsi: compatible = "fsl,imx50-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
>> arch/arm/boot/dts/ls1021a.dtsi: compatible = "fsl,imx21-wdt";
>> arch/arm/boot/dts/vfxxx.dtsi: compatible = "fsl,vf610-wdt", "fsl,imx21-wdt";
>>
>> I don't see a confirmation of the fact that "fsl,imx35-wdt" is the best
>> selection, hence definitely "fsl,imx25-wdt" overscores it.
>
> I stated my reason to pick imx35 over imx25 above. Why do yo prefer
> imx25?
As you said right now and until it is added into DTS files "fsl,imx25-wdt" and
"fsl,imx35-wdt" are equal as a compatible representation of WMCR-aware watchdogs.
I may be wrong, if I assume that i.MX25 is released before i.MX35 (looks like
they are released in parallel), but "imx25" precedes in alphanumerically sorted
list of SoC names.
>>> A reason to add e.g. fsl,imx6sl-wdt is that dtbs in the wild use
>>>
>>> "fsl,imx6sl-wdt", "fsl,imx21-wdt"
>>>
>>> . But then I wonder if
>>>
>>> 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
>>>
>>> is that important to justify to add these all.
>>
>> About this comment I don't know, you may have better insight about the original
>> change. By the way, in barebox you may want to fix the same hang-up, because
>> you touch WMCR unconditionally.
>
> Sure.
>
>>> Independant of this I'd like to have all dtsi for the newer SoCs changed
>>> to get
>>>
>>> "fsl,imx6sl-wdt", "fsl,imx35-wdt"
>>>
>>> and if you really want to add all these compatibles, add a comment that
>>> these are really fsl,imx35-wdt and were added to work around broken
>>> dtbs.
>>
>> No objections, but
>>
>> 1) I'd like to see "fsl,imx25-wdt" as a new common compatible, and that's
>> what have been proposed and shared in RFC 2/2.
>
> Yeah, I missed that other patch (which was not part of this series).
>
>> 2) Please remind me, why do you ask to drop "fsl,imx21-wdt" from the list?
>>
>> compatible = "fsl,imx6sx-wdt", "fsl,imx25-wdt", "fsl,imx21-wdt";
>>
>> The list of compatibles above is valid (from the most specific on the left
>> to the most generic on the right), and that compatible property is rightly
>> handled in the driver with this change applied. I don't see a need to
>> drop "fsl,imx21-wdt".
>
> If the wdt in the i.MX6SX can be operated by the watchdog driver in it's
> imx21 mode, you should keep the imx21 entry. If not, you shouldn't.
The DTS description shall be rather independent from any particular software
implementation, someone may want to reuse the same DTB in bootloaders and OS
kernels of different versions. Assuming that I have a bootloader/kernel
with a pure i.MX21 watchdog driver and run it on i.MX6SX, I'd prefer to match
"fsl,imx21-wdt" compatible.
--
With best wishes,
Vladimir
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-11 11:21 ` Vladimir Zapolskiy
@ 2016-12-11 12:28 ` Uwe Kleine-König
-1 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-11 12:28 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Shawn Guo, Wim Van Sebroeck, Guenter Roeck, Fabio Estevam,
Sascha Hauer, Magnus Lilja, linux-watchdog, linux-arm-kernel
On Sun, Dec 11, 2016 at 01:21:16PM +0200, Vladimir Zapolskiy wrote:
> On 12/11/2016 12:26 PM, Uwe Kleine-König wrote:
> > . With the change to the driver it's like using the watchdog driver in
> > it's pre-5fe65ce7ccbb4-state on it. If this is bad, we must indeed make
> > fsl,imx53-wdt known to the driver. If not, just adding a single new
> > compatible to the driver is just fine.
<sarcasm>You can also add "marvell,armada-xp-wdt" to the end of the
list. If your kernel is configured correctly still the right thing will
happen.</sarcasm>
> The change under discussion preserves the current runtime behaviour for
> i.MX53 and its friends, the watchdog power down counter in WMCR is
> disabled on boot (you may want to confirm it by testing though), another
> question is if it is wanted for e.g. i.MX53 right from the beginning.
>
> To keep the runtime compatibility of a newer kernel with old DTBs such
> a long list of device compatibles has to be inserted.
>
> >
> > If you want to call it imx25 or imx35 doesn't matter much. The reason I
> > picked imx35 is that this one was already picked before for cspi. This
> > suggests that i.MX35 is older than i.MX25 and so this is the canonical
> > choice.
>
> Accepted, from the Wikipedia both i.MX25 and i.MX35 are released in 2009,
> which one is the first I don't know for sure, I supposed it could be
> i.MX25 as a SoC with a weaker core.
>
> >
> >> $ grep -H 'fsl,imx.*-wdt' arch/arm/boot/dts/*.dtsi
> >> arch/arm/boot/dts/imx25.dtsi: compatible = "fsl,imx25-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx27.dtsi: compatible = "fsl,imx27-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx35.dtsi: compatible = "fsl,imx35-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx50.dtsi: compatible = "fsl,imx50-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/ls1021a.dtsi: compatible = "fsl,imx21-wdt";
> >> arch/arm/boot/dts/vfxxx.dtsi: compatible = "fsl,vf610-wdt", "fsl,imx21-wdt";
> >>
> >> I don't see a confirmation of the fact that "fsl,imx35-wdt" is the best
> >> selection, hence definitely "fsl,imx25-wdt" overscores it.
> >
> > I stated my reason to pick imx35 over imx25 above. Why do yo prefer
> > imx25?
>
> As you said right now and until it is added into DTS files "fsl,imx25-wdt" and
> "fsl,imx35-wdt" are equal as a compatible representation of WMCR-aware watchdogs.
>
> I may be wrong, if I assume that i.MX25 is released before i.MX35 (looks like
> they are released in parallel), but "imx25" precedes in alphanumerically sorted
> list of SoC names.
I'm sure Shawn could say something here, but I would be surprised if the
i.MX25 came first.
> >>> A reason to add e.g. fsl,imx6sl-wdt is that dtbs in the wild use
> >>>
> >>> "fsl,imx6sl-wdt", "fsl,imx21-wdt"
> >>>
> >>> . But then I wonder if
> >>>
> >>> 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
> >>>
> >>> is that important to justify to add these all.
> >>
> >> About this comment I don't know, you may have better insight about the original
> >> change. By the way, in barebox you may want to fix the same hang-up, because
> >> you touch WMCR unconditionally.
> >
> > Sure.
> >
> >>> Independant of this I'd like to have all dtsi for the newer SoCs changed
> >>> to get
> >>>
> >>> "fsl,imx6sl-wdt", "fsl,imx35-wdt"
> >>>
> >>> and if you really want to add all these compatibles, add a comment that
> >>> these are really fsl,imx35-wdt and were added to work around broken
> >>> dtbs.
> >>
> >> No objections, but
> >>
> >> 1) I'd like to see "fsl,imx25-wdt" as a new common compatible, and that's
> >> what have been proposed and shared in RFC 2/2.
> >
> > Yeah, I missed that other patch (which was not part of this series).
> >
> >> 2) Please remind me, why do you ask to drop "fsl,imx21-wdt" from the list?
> >>
> >> compatible = "fsl,imx6sx-wdt", "fsl,imx25-wdt", "fsl,imx21-wdt";
> >>
> >> The list of compatibles above is valid (from the most specific on the left
> >> to the most generic on the right), and that compatible property is rightly
> >> handled in the driver with this change applied. I don't see a need to
> >> drop "fsl,imx21-wdt".
> >
> > If the wdt in the i.MX6SX can be operated by the watchdog driver in it's
> > imx21 mode, you should keep the imx21 entry. If not, you shouldn't.
>
> The DTS description shall be rather independent from any particular software
> implementation, someone may want to reuse the same DTB in bootloaders and OS
> kernels of different versions.
Full ack.
> Assuming that I have a bootloader/kernel with a pure i.MX21 watchdog
> driver and run it on i.MX6SX, I'd prefer to match
> "fsl,imx21-wdt" compatible.
I'd prefer to notice that the i.MX6SX has a (maybe only slightly)
different watchdog and so the i.MX21 aware driver should not bind to the
i.MX6SX hardware. So (as you said above) the compatible should be
independent from already existing i.MX21 wdt support in a driver.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-11 12:28 ` Uwe Kleine-König
0 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-11 12:28 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Dec 11, 2016 at 01:21:16PM +0200, Vladimir Zapolskiy wrote:
> On 12/11/2016 12:26 PM, Uwe Kleine-K?nig wrote:
> > . With the change to the driver it's like using the watchdog driver in
> > it's pre-5fe65ce7ccbb4-state on it. If this is bad, we must indeed make
> > fsl,imx53-wdt known to the driver. If not, just adding a single new
> > compatible to the driver is just fine.
<sarcasm>You can also add "marvell,armada-xp-wdt" to the end of the
list. If your kernel is configured correctly still the right thing will
happen.</sarcasm>
> The change under discussion preserves the current runtime behaviour for
> i.MX53 and its friends, the watchdog power down counter in WMCR is
> disabled on boot (you may want to confirm it by testing though), another
> question is if it is wanted for e.g. i.MX53 right from the beginning.
>
> To keep the runtime compatibility of a newer kernel with old DTBs such
> a long list of device compatibles has to be inserted.
>
> >
> > If you want to call it imx25 or imx35 doesn't matter much. The reason I
> > picked imx35 is that this one was already picked before for cspi. This
> > suggests that i.MX35 is older than i.MX25 and so this is the canonical
> > choice.
>
> Accepted, from the Wikipedia both i.MX25 and i.MX35 are released in 2009,
> which one is the first I don't know for sure, I supposed it could be
> i.MX25 as a SoC with a weaker core.
>
> >
> >> $ grep -H 'fsl,imx.*-wdt' arch/arm/boot/dts/*.dtsi
> >> arch/arm/boot/dts/imx25.dtsi: compatible = "fsl,imx25-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx27.dtsi: compatible = "fsl,imx27-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx35.dtsi: compatible = "fsl,imx35-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx50.dtsi: compatible = "fsl,imx50-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> >> arch/arm/boot/dts/ls1021a.dtsi: compatible = "fsl,imx21-wdt";
> >> arch/arm/boot/dts/vfxxx.dtsi: compatible = "fsl,vf610-wdt", "fsl,imx21-wdt";
> >>
> >> I don't see a confirmation of the fact that "fsl,imx35-wdt" is the best
> >> selection, hence definitely "fsl,imx25-wdt" overscores it.
> >
> > I stated my reason to pick imx35 over imx25 above. Why do yo prefer
> > imx25?
>
> As you said right now and until it is added into DTS files "fsl,imx25-wdt" and
> "fsl,imx35-wdt" are equal as a compatible representation of WMCR-aware watchdogs.
>
> I may be wrong, if I assume that i.MX25 is released before i.MX35 (looks like
> they are released in parallel), but "imx25" precedes in alphanumerically sorted
> list of SoC names.
I'm sure Shawn could say something here, but I would be surprised if the
i.MX25 came first.
> >>> A reason to add e.g. fsl,imx6sl-wdt is that dtbs in the wild use
> >>>
> >>> "fsl,imx6sl-wdt", "fsl,imx21-wdt"
> >>>
> >>> . But then I wonder if
> >>>
> >>> 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
> >>>
> >>> is that important to justify to add these all.
> >>
> >> About this comment I don't know, you may have better insight about the original
> >> change. By the way, in barebox you may want to fix the same hang-up, because
> >> you touch WMCR unconditionally.
> >
> > Sure.
> >
> >>> Independant of this I'd like to have all dtsi for the newer SoCs changed
> >>> to get
> >>>
> >>> "fsl,imx6sl-wdt", "fsl,imx35-wdt"
> >>>
> >>> and if you really want to add all these compatibles, add a comment that
> >>> these are really fsl,imx35-wdt and were added to work around broken
> >>> dtbs.
> >>
> >> No objections, but
> >>
> >> 1) I'd like to see "fsl,imx25-wdt" as a new common compatible, and that's
> >> what have been proposed and shared in RFC 2/2.
> >
> > Yeah, I missed that other patch (which was not part of this series).
> >
> >> 2) Please remind me, why do you ask to drop "fsl,imx21-wdt" from the list?
> >>
> >> compatible = "fsl,imx6sx-wdt", "fsl,imx25-wdt", "fsl,imx21-wdt";
> >>
> >> The list of compatibles above is valid (from the most specific on the left
> >> to the most generic on the right), and that compatible property is rightly
> >> handled in the driver with this change applied. I don't see a need to
> >> drop "fsl,imx21-wdt".
> >
> > If the wdt in the i.MX6SX can be operated by the watchdog driver in it's
> > imx21 mode, you should keep the imx21 entry. If not, you shouldn't.
>
> The DTS description shall be rather independent from any particular software
> implementation, someone may want to reuse the same DTB in bootloaders and OS
> kernels of different versions.
Full ack.
> Assuming that I have a bootloader/kernel with a pure i.MX21 watchdog
> driver and run it on i.MX6SX, I'd prefer to match
> "fsl,imx21-wdt" compatible.
I'd prefer to notice that the i.MX6SX has a (maybe only slightly)
different watchdog and so the i.MX21 aware driver should not bind to the
i.MX6SX hardware. So (as you said above) the compatible should be
independent from already existing i.MX21 wdt support in a driver.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-11 11:21 ` Vladimir Zapolskiy
@ 2016-12-23 1:55 ` Guenter Roeck
-1 siblings, 0 replies; 48+ messages in thread
From: Guenter Roeck @ 2016-12-23 1:55 UTC (permalink / raw)
To: Vladimir Zapolskiy, Uwe Kleine-König
Cc: Shawn Guo, Wim Van Sebroeck, Fabio Estevam, Sascha Hauer,
Magnus Lilja, linux-watchdog, linux-arm-kernel
On 12/11/2016 03:21 AM, Vladimir Zapolskiy wrote:
> On 12/11/2016 12:26 PM, Uwe Kleine-König wrote:
>> Hello Vladimir,
>>
>> On Sun, Dec 11, 2016 at 12:01:08PM +0200, Vladimir Zapolskiy wrote:
>>> On 12/11/2016 11:35 AM, Uwe Kleine-König wrote:
>>>> On Sun, Dec 11, 2016 at 03:06:48AM +0200, Vladimir Zapolskiy wrote:
>>>>> Power down counter enable/disable bit switch is located in WMCR
>>>>> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
>>>>> i.MX31 SoCs don't have this register. As a result of writing data to
>>>>> the non-existing register on driver probe the SoC hangs up, to fix the
>>>>> problem add more OF compatible strings and on this basis get
>>>>> information about availability of the WMCR register.
>>>>>
>>>>> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
>>>>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>>>>> ---
>>>
What is the ultimate conclusion of this exchange ?
Are we going to get another version of the patch, or did everyone agree that
the patch is good as it is and does not require further changes ?
Guenter
>>> [snip]
>>>
>>>>>
>>>>> static const struct of_device_id imx2_wdt_dt_ids[] = {
>>>>> - { .compatible = "fsl,imx21-wdt", },
>>>>> + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
>>>>> + { .compatible = "fsl,imx25-wdt", },
>>>>> + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
>>>>> + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
>>>>> + { .compatible = "fsl,imx35-wdt", },
>>>>> + { .compatible = "fsl,imx50-wdt", },
>>>>> + { .compatible = "fsl,imx51-wdt", },
>>>>> + { .compatible = "fsl,imx53-wdt", },
>>>>> + { .compatible = "fsl,imx6q-wdt", },
>>>>> + { .compatible = "fsl,imx6sl-wdt", },
>>>>> + { .compatible = "fsl,imx6sx-wdt", },
>>>>> + { .compatible = "fsl,imx6ul-wdt", },
>>>>> + { .compatible = "fsl,imx7d-wdt", },
>>>>> + { .compatible = "fsl,vf610-wdt", },
>>>>
>>>> Up to now we only had fsl,imx21-wdt, now it seems that iMX31, i.MX27 and
>>>> i.MX21 form a group of equal watchdog IPs and the others form another
>>>> group, right?
>>>>
>>>> So conceptually we should differenciate between
>>>>
>>>> fsl,imx21-wdt (which is used on i.MX21, i.MX27 and i.MX31)
>>>> fsl,imx35-wdt (which is used on all others)
>>>>
>>>
>>> The problem is that "fsl,imx35-wdt" is not used on all others in the
>>> existing DTS, i.e. in the old firmware, which shall be compatible with
>>> new changes.
>>
>> So old i.MX53 has
>>
>> compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
>>
>> . With the change to the driver it's like using the watchdog driver in
>> it's pre-5fe65ce7ccbb4-state on it. If this is bad, we must indeed make
>> fsl,imx53-wdt known to the driver. If not, just adding a single new
>> compatible to the driver is just fine.
>
> The change under discussion preserves the current runtime behaviour for
> i.MX53 and its friends, the watchdog power down counter in WMCR is
> disabled on boot (you may want to confirm it by testing though), another
> question is if it is wanted for e.g. i.MX53 right from the beginning.
>
> To keep the runtime compatibility of a newer kernel with old DTBs such
> a long list of device compatibles has to be inserted.
>
>>
>> If you want to call it imx25 or imx35 doesn't matter much. The reason I
>> picked imx35 is that this one was already picked before for cspi. This
>> suggests that i.MX35 is older than i.MX25 and so this is the canonical
>> choice.
>
> Accepted, from the Wikipedia both i.MX25 and i.MX35 are released in 2009,
> which one is the first I don't know for sure, I supposed it could be
> i.MX25 as a SoC with a weaker core.
>
>>
>>> $ grep -H 'fsl,imx.*-wdt' arch/arm/boot/dts/*.dtsi
>>> arch/arm/boot/dts/imx25.dtsi: compatible = "fsl,imx25-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx27.dtsi: compatible = "fsl,imx27-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx35.dtsi: compatible = "fsl,imx35-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx50.dtsi: compatible = "fsl,imx50-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/ls1021a.dtsi: compatible = "fsl,imx21-wdt";
>>> arch/arm/boot/dts/vfxxx.dtsi: compatible = "fsl,vf610-wdt", "fsl,imx21-wdt";
>>>
>>> I don't see a confirmation of the fact that "fsl,imx35-wdt" is the best
>>> selection, hence definitely "fsl,imx25-wdt" overscores it.
>>
>> I stated my reason to pick imx35 over imx25 above. Why do yo prefer
>> imx25?
>
> As you said right now and until it is added into DTS files "fsl,imx25-wdt" and
> "fsl,imx35-wdt" are equal as a compatible representation of WMCR-aware watchdogs.
>
> I may be wrong, if I assume that i.MX25 is released before i.MX35 (looks like
> they are released in parallel), but "imx25" precedes in alphanumerically sorted
> list of SoC names.
>
>>>> A reason to add e.g. fsl,imx6sl-wdt is that dtbs in the wild use
>>>>
>>>> "fsl,imx6sl-wdt", "fsl,imx21-wdt"
>>>>
>>>> . But then I wonder if
>>>>
>>>> 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
>>>>
>>>> is that important to justify to add these all.
>>>
>>> About this comment I don't know, you may have better insight about the original
>>> change. By the way, in barebox you may want to fix the same hang-up, because
>>> you touch WMCR unconditionally.
>>
>> Sure.
>>
>>>> Independant of this I'd like to have all dtsi for the newer SoCs changed
>>>> to get
>>>>
>>>> "fsl,imx6sl-wdt", "fsl,imx35-wdt"
>>>>
>>>> and if you really want to add all these compatibles, add a comment that
>>>> these are really fsl,imx35-wdt and were added to work around broken
>>>> dtbs.
>>>
>>> No objections, but
>>>
>>> 1) I'd like to see "fsl,imx25-wdt" as a new common compatible, and that's
>>> what have been proposed and shared in RFC 2/2.
>>
>> Yeah, I missed that other patch (which was not part of this series).
>>
>>> 2) Please remind me, why do you ask to drop "fsl,imx21-wdt" from the list?
>>>
>>> compatible = "fsl,imx6sx-wdt", "fsl,imx25-wdt", "fsl,imx21-wdt";
>>>
>>> The list of compatibles above is valid (from the most specific on the left
>>> to the most generic on the right), and that compatible property is rightly
>>> handled in the driver with this change applied. I don't see a need to
>>> drop "fsl,imx21-wdt".
>>
>> If the wdt in the i.MX6SX can be operated by the watchdog driver in it's
>> imx21 mode, you should keep the imx21 entry. If not, you shouldn't.
>
> The DTS description shall be rather independent from any particular software
> implementation, someone may want to reuse the same DTB in bootloaders and OS
> kernels of different versions. Assuming that I have a bootloader/kernel
> with a pure i.MX21 watchdog driver and run it on i.MX6SX, I'd prefer to match
> "fsl,imx21-wdt" compatible.
>
> --
> With best wishes,
> Vladimir
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-23 1:55 ` Guenter Roeck
0 siblings, 0 replies; 48+ messages in thread
From: Guenter Roeck @ 2016-12-23 1:55 UTC (permalink / raw)
To: linux-arm-kernel
On 12/11/2016 03:21 AM, Vladimir Zapolskiy wrote:
> On 12/11/2016 12:26 PM, Uwe Kleine-K?nig wrote:
>> Hello Vladimir,
>>
>> On Sun, Dec 11, 2016 at 12:01:08PM +0200, Vladimir Zapolskiy wrote:
>>> On 12/11/2016 11:35 AM, Uwe Kleine-K?nig wrote:
>>>> On Sun, Dec 11, 2016 at 03:06:48AM +0200, Vladimir Zapolskiy wrote:
>>>>> Power down counter enable/disable bit switch is located in WMCR
>>>>> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
>>>>> i.MX31 SoCs don't have this register. As a result of writing data to
>>>>> the non-existing register on driver probe the SoC hangs up, to fix the
>>>>> problem add more OF compatible strings and on this basis get
>>>>> information about availability of the WMCR register.
>>>>>
>>>>> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
>>>>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>>>>> ---
>>>
What is the ultimate conclusion of this exchange ?
Are we going to get another version of the patch, or did everyone agree that
the patch is good as it is and does not require further changes ?
Guenter
>>> [snip]
>>>
>>>>>
>>>>> static const struct of_device_id imx2_wdt_dt_ids[] = {
>>>>> - { .compatible = "fsl,imx21-wdt", },
>>>>> + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
>>>>> + { .compatible = "fsl,imx25-wdt", },
>>>>> + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
>>>>> + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
>>>>> + { .compatible = "fsl,imx35-wdt", },
>>>>> + { .compatible = "fsl,imx50-wdt", },
>>>>> + { .compatible = "fsl,imx51-wdt", },
>>>>> + { .compatible = "fsl,imx53-wdt", },
>>>>> + { .compatible = "fsl,imx6q-wdt", },
>>>>> + { .compatible = "fsl,imx6sl-wdt", },
>>>>> + { .compatible = "fsl,imx6sx-wdt", },
>>>>> + { .compatible = "fsl,imx6ul-wdt", },
>>>>> + { .compatible = "fsl,imx7d-wdt", },
>>>>> + { .compatible = "fsl,vf610-wdt", },
>>>>
>>>> Up to now we only had fsl,imx21-wdt, now it seems that iMX31, i.MX27 and
>>>> i.MX21 form a group of equal watchdog IPs and the others form another
>>>> group, right?
>>>>
>>>> So conceptually we should differenciate between
>>>>
>>>> fsl,imx21-wdt (which is used on i.MX21, i.MX27 and i.MX31)
>>>> fsl,imx35-wdt (which is used on all others)
>>>>
>>>
>>> The problem is that "fsl,imx35-wdt" is not used on all others in the
>>> existing DTS, i.e. in the old firmware, which shall be compatible with
>>> new changes.
>>
>> So old i.MX53 has
>>
>> compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
>>
>> . With the change to the driver it's like using the watchdog driver in
>> it's pre-5fe65ce7ccbb4-state on it. If this is bad, we must indeed make
>> fsl,imx53-wdt known to the driver. If not, just adding a single new
>> compatible to the driver is just fine.
>
> The change under discussion preserves the current runtime behaviour for
> i.MX53 and its friends, the watchdog power down counter in WMCR is
> disabled on boot (you may want to confirm it by testing though), another
> question is if it is wanted for e.g. i.MX53 right from the beginning.
>
> To keep the runtime compatibility of a newer kernel with old DTBs such
> a long list of device compatibles has to be inserted.
>
>>
>> If you want to call it imx25 or imx35 doesn't matter much. The reason I
>> picked imx35 is that this one was already picked before for cspi. This
>> suggests that i.MX35 is older than i.MX25 and so this is the canonical
>> choice.
>
> Accepted, from the Wikipedia both i.MX25 and i.MX35 are released in 2009,
> which one is the first I don't know for sure, I supposed it could be
> i.MX25 as a SoC with a weaker core.
>
>>
>>> $ grep -H 'fsl,imx.*-wdt' arch/arm/boot/dts/*.dtsi
>>> arch/arm/boot/dts/imx25.dtsi: compatible = "fsl,imx25-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx27.dtsi: compatible = "fsl,imx27-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx35.dtsi: compatible = "fsl,imx35-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx50.dtsi: compatible = "fsl,imx50-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
>>> arch/arm/boot/dts/ls1021a.dtsi: compatible = "fsl,imx21-wdt";
>>> arch/arm/boot/dts/vfxxx.dtsi: compatible = "fsl,vf610-wdt", "fsl,imx21-wdt";
>>>
>>> I don't see a confirmation of the fact that "fsl,imx35-wdt" is the best
>>> selection, hence definitely "fsl,imx25-wdt" overscores it.
>>
>> I stated my reason to pick imx35 over imx25 above. Why do yo prefer
>> imx25?
>
> As you said right now and until it is added into DTS files "fsl,imx25-wdt" and
> "fsl,imx35-wdt" are equal as a compatible representation of WMCR-aware watchdogs.
>
> I may be wrong, if I assume that i.MX25 is released before i.MX35 (looks like
> they are released in parallel), but "imx25" precedes in alphanumerically sorted
> list of SoC names.
>
>>>> A reason to add e.g. fsl,imx6sl-wdt is that dtbs in the wild use
>>>>
>>>> "fsl,imx6sl-wdt", "fsl,imx21-wdt"
>>>>
>>>> . But then I wonder if
>>>>
>>>> 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
>>>>
>>>> is that important to justify to add these all.
>>>
>>> About this comment I don't know, you may have better insight about the original
>>> change. By the way, in barebox you may want to fix the same hang-up, because
>>> you touch WMCR unconditionally.
>>
>> Sure.
>>
>>>> Independant of this I'd like to have all dtsi for the newer SoCs changed
>>>> to get
>>>>
>>>> "fsl,imx6sl-wdt", "fsl,imx35-wdt"
>>>>
>>>> and if you really want to add all these compatibles, add a comment that
>>>> these are really fsl,imx35-wdt and were added to work around broken
>>>> dtbs.
>>>
>>> No objections, but
>>>
>>> 1) I'd like to see "fsl,imx25-wdt" as a new common compatible, and that's
>>> what have been proposed and shared in RFC 2/2.
>>
>> Yeah, I missed that other patch (which was not part of this series).
>>
>>> 2) Please remind me, why do you ask to drop "fsl,imx21-wdt" from the list?
>>>
>>> compatible = "fsl,imx6sx-wdt", "fsl,imx25-wdt", "fsl,imx21-wdt";
>>>
>>> The list of compatibles above is valid (from the most specific on the left
>>> to the most generic on the right), and that compatible property is rightly
>>> handled in the driver with this change applied. I don't see a need to
>>> drop "fsl,imx21-wdt".
>>
>> If the wdt in the i.MX6SX can be operated by the watchdog driver in it's
>> imx21 mode, you should keep the imx21 entry. If not, you shouldn't.
>
> The DTS description shall be rather independent from any particular software
> implementation, someone may want to reuse the same DTB in bootloaders and OS
> kernels of different versions. Assuming that I have a bootloader/kernel
> with a pure i.MX21 watchdog driver and run it on i.MX6SX, I'd prefer to match
> "fsl,imx21-wdt" compatible.
>
> --
> With best wishes,
> Vladimir
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-23 1:55 ` Guenter Roeck
@ 2016-12-23 8:20 ` Vladimir Zapolskiy
-1 siblings, 0 replies; 48+ messages in thread
From: Vladimir Zapolskiy @ 2016-12-23 8:20 UTC (permalink / raw)
To: Guenter Roeck, Uwe Kleine-König
Cc: Shawn Guo, Wim Van Sebroeck, Fabio Estevam, Sascha Hauer,
Magnus Lilja, linux-watchdog, linux-arm-kernel
On 12/23/2016 03:55 AM, Guenter Roeck wrote:
> On 12/11/2016 03:21 AM, Vladimir Zapolskiy wrote:
>> On 12/11/2016 12:26 PM, Uwe Kleine-König wrote:
>>> Hello Vladimir,
>>>
>>> On Sun, Dec 11, 2016 at 12:01:08PM +0200, Vladimir Zapolskiy wrote:
>>>> On 12/11/2016 11:35 AM, Uwe Kleine-König wrote:
>>>>> On Sun, Dec 11, 2016 at 03:06:48AM +0200, Vladimir Zapolskiy wrote:
>>>>>> Power down counter enable/disable bit switch is located in WMCR
>>>>>> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
>>>>>> i.MX31 SoCs don't have this register. As a result of writing data to
>>>>>> the non-existing register on driver probe the SoC hangs up, to fix the
>>>>>> problem add more OF compatible strings and on this basis get
>>>>>> information about availability of the WMCR register.
>>>>>>
>>>>>> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
>>>>>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>>>>>> ---
>>>>
>
>
> What is the ultimate conclusion of this exchange ?
>
> Are we going to get another version of the patch, or did everyone agree that
> the patch is good as it is and does not require further changes ?
>
I can not imagine a different fix.
--
With best wishes,
Vladimir
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-23 8:20 ` Vladimir Zapolskiy
0 siblings, 0 replies; 48+ messages in thread
From: Vladimir Zapolskiy @ 2016-12-23 8:20 UTC (permalink / raw)
To: linux-arm-kernel
On 12/23/2016 03:55 AM, Guenter Roeck wrote:
> On 12/11/2016 03:21 AM, Vladimir Zapolskiy wrote:
>> On 12/11/2016 12:26 PM, Uwe Kleine-K?nig wrote:
>>> Hello Vladimir,
>>>
>>> On Sun, Dec 11, 2016 at 12:01:08PM +0200, Vladimir Zapolskiy wrote:
>>>> On 12/11/2016 11:35 AM, Uwe Kleine-K?nig wrote:
>>>>> On Sun, Dec 11, 2016 at 03:06:48AM +0200, Vladimir Zapolskiy wrote:
>>>>>> Power down counter enable/disable bit switch is located in WMCR
>>>>>> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
>>>>>> i.MX31 SoCs don't have this register. As a result of writing data to
>>>>>> the non-existing register on driver probe the SoC hangs up, to fix the
>>>>>> problem add more OF compatible strings and on this basis get
>>>>>> information about availability of the WMCR register.
>>>>>>
>>>>>> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
>>>>>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>>>>>> ---
>>>>
>
>
> What is the ultimate conclusion of this exchange ?
>
> Are we going to get another version of the patch, or did everyone agree that
> the patch is good as it is and does not require further changes ?
>
I can not imagine a different fix.
--
With best wishes,
Vladimir
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-23 8:20 ` Vladimir Zapolskiy
@ 2016-12-23 8:32 ` Uwe Kleine-König
-1 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-23 8:32 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Guenter Roeck, Shawn Guo, Wim Van Sebroeck, Fabio Estevam,
Sascha Hauer, Magnus Lilja, linux-watchdog, linux-arm-kernel
Hello,
On Fri, Dec 23, 2016 at 10:20:20AM +0200, Vladimir Zapolskiy wrote:
> On 12/23/2016 03:55 AM, Guenter Roeck wrote:
> > What is the ultimate conclusion of this exchange ?
> >
> > Are we going to get another version of the patch, or did everyone agree that
> > the patch is good as it is and does not require further changes ?
> >
>
> I can not imagine a different fix.
my preferred fix would be:
- add an imx35 compatible to all newer dtsi
- update the driver to only write the wmcr on imx35 compatible devices
adding only imx35.
Compared to Vladimir's change this undoes 5fe65ce7ccbb ("watchdog:
imx2_wdt: Disable power down counter on boot") on the newer generation
SoCs until their dtb is updated. 5fe65ce7ccbb fixed a problem in 2014
for these newer SoCs which wasn't urgent enough to be fixed earlier, so
I'd say this isn't a big problem and in return the drivers and device
trees stay simpler.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-23 8:32 ` Uwe Kleine-König
0 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-23 8:32 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On Fri, Dec 23, 2016 at 10:20:20AM +0200, Vladimir Zapolskiy wrote:
> On 12/23/2016 03:55 AM, Guenter Roeck wrote:
> > What is the ultimate conclusion of this exchange ?
> >
> > Are we going to get another version of the patch, or did everyone agree that
> > the patch is good as it is and does not require further changes ?
> >
>
> I can not imagine a different fix.
my preferred fix would be:
- add an imx35 compatible to all newer dtsi
- update the driver to only write the wmcr on imx35 compatible devices
adding only imx35.
Compared to Vladimir's change this undoes 5fe65ce7ccbb ("watchdog:
imx2_wdt: Disable power down counter on boot") on the newer generation
SoCs until their dtb is updated. 5fe65ce7ccbb fixed a problem in 2014
for these newer SoCs which wasn't urgent enough to be fixed earlier, so
I'd say this isn't a big problem and in return the drivers and device
trees stay simpler.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-23 8:32 ` Uwe Kleine-König
@ 2016-12-23 9:27 ` Vladimir Zapolskiy
-1 siblings, 0 replies; 48+ messages in thread
From: Vladimir Zapolskiy @ 2016-12-23 9:27 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Guenter Roeck, Shawn Guo, Wim Van Sebroeck, Fabio Estevam,
Sascha Hauer, Magnus Lilja, linux-watchdog, linux-arm-kernel
On 12/23/2016 10:32 AM, Uwe Kleine-König wrote:
> Hello,
>
> On Fri, Dec 23, 2016 at 10:20:20AM +0200, Vladimir Zapolskiy wrote:
>> On 12/23/2016 03:55 AM, Guenter Roeck wrote:
>>> What is the ultimate conclusion of this exchange ?
>>>
>>> Are we going to get another version of the patch, or did everyone agree that
>>> the patch is good as it is and does not require further changes ?
>>>
>>
>> I can not imagine a different fix.
>
> my preferred fix would be:
>
> - add an imx35 compatible to all newer dtsi
> - update the driver to only write the wmcr on imx35 compatible devices
> adding only imx35.
>
It breaks old DTS vs. new kernel compatibility, I've already mentioned this.
--
With best wishes,
Vladimir
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-23 9:27 ` Vladimir Zapolskiy
0 siblings, 0 replies; 48+ messages in thread
From: Vladimir Zapolskiy @ 2016-12-23 9:27 UTC (permalink / raw)
To: linux-arm-kernel
On 12/23/2016 10:32 AM, Uwe Kleine-K?nig wrote:
> Hello,
>
> On Fri, Dec 23, 2016 at 10:20:20AM +0200, Vladimir Zapolskiy wrote:
>> On 12/23/2016 03:55 AM, Guenter Roeck wrote:
>>> What is the ultimate conclusion of this exchange ?
>>>
>>> Are we going to get another version of the patch, or did everyone agree that
>>> the patch is good as it is and does not require further changes ?
>>>
>>
>> I can not imagine a different fix.
>
> my preferred fix would be:
>
> - add an imx35 compatible to all newer dtsi
> - update the driver to only write the wmcr on imx35 compatible devices
> adding only imx35.
>
It breaks old DTS vs. new kernel compatibility, I've already mentioned this.
--
With best wishes,
Vladimir
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-23 9:27 ` Vladimir Zapolskiy
@ 2016-12-23 10:01 ` Uwe Kleine-König
-1 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-23 10:01 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Guenter Roeck, Shawn Guo, Wim Van Sebroeck, Fabio Estevam,
Sascha Hauer, Magnus Lilja, linux-watchdog, linux-arm-kernel
On Fri, Dec 23, 2016 at 11:27:24AM +0200, Vladimir Zapolskiy wrote:
> On 12/23/2016 10:32 AM, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Fri, Dec 23, 2016 at 10:20:20AM +0200, Vladimir Zapolskiy wrote:
> >> On 12/23/2016 03:55 AM, Guenter Roeck wrote:
> >>> What is the ultimate conclusion of this exchange ?
> >>>
> >>> Are we going to get another version of the patch, or did everyone agree that
> >>> the patch is good as it is and does not require further changes ?
> >>>
> >>
> >> I can not imagine a different fix.
> >
> > my preferred fix would be:
> >
> > - add an imx35 compatible to all newer dtsi
> > - update the driver to only write the wmcr on imx35 compatible devices
> > adding only imx35.
> >
>
> It breaks old DTS vs. new kernel compatibility, I've already mentioned this.
Correct, and I didn't deny that. In my mail I pointed out the problem
this imposes and I think it's small enough to not justify the complexity
introduced in your proposed change.
If we cannot agree then at least this needs to be better documented in
the driver because otherwise someone makes a cleanup later dropping all
the compatibles that are needed to keep backwards compatibility.
But my suggestion is to first do the minimal fix, and if in the next
merge window people lament about breakages on their machine, we can
still extend the simple fix to a fully backwards compatible change.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-23 10:01 ` Uwe Kleine-König
0 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-23 10:01 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Dec 23, 2016 at 11:27:24AM +0200, Vladimir Zapolskiy wrote:
> On 12/23/2016 10:32 AM, Uwe Kleine-K?nig wrote:
> > Hello,
> >
> > On Fri, Dec 23, 2016 at 10:20:20AM +0200, Vladimir Zapolskiy wrote:
> >> On 12/23/2016 03:55 AM, Guenter Roeck wrote:
> >>> What is the ultimate conclusion of this exchange ?
> >>>
> >>> Are we going to get another version of the patch, or did everyone agree that
> >>> the patch is good as it is and does not require further changes ?
> >>>
> >>
> >> I can not imagine a different fix.
> >
> > my preferred fix would be:
> >
> > - add an imx35 compatible to all newer dtsi
> > - update the driver to only write the wmcr on imx35 compatible devices
> > adding only imx35.
> >
>
> It breaks old DTS vs. new kernel compatibility, I've already mentioned this.
Correct, and I didn't deny that. In my mail I pointed out the problem
this imposes and I think it's small enough to not justify the complexity
introduced in your proposed change.
If we cannot agree then at least this needs to be better documented in
the driver because otherwise someone makes a cleanup later dropping all
the compatibles that are needed to keep backwards compatibility.
But my suggestion is to first do the minimal fix, and if in the next
merge window people lament about breakages on their machine, we can
still extend the simple fix to a fully backwards compatible change.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-23 10:01 ` Uwe Kleine-König
@ 2016-12-23 11:21 ` Vladimir Zapolskiy
-1 siblings, 0 replies; 48+ messages in thread
From: Vladimir Zapolskiy @ 2016-12-23 11:21 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Vladimir Zapolskiy, Guenter Roeck, Shawn Guo, Wim Van Sebroeck,
Fabio Estevam, Sascha Hauer, Magnus Lilja, linux-watchdog,
linux-arm-kernel
Hi Uwe,
On 12/23/2016 12:01 PM, Uwe Kleine-König wrote:
> On Fri, Dec 23, 2016 at 11:27:24AM +0200, Vladimir Zapolskiy wrote:
>> On 12/23/2016 10:32 AM, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> On Fri, Dec 23, 2016 at 10:20:20AM +0200, Vladimir Zapolskiy wrote:
>>>> On 12/23/2016 03:55 AM, Guenter Roeck wrote:
>>>>> What is the ultimate conclusion of this exchange ?
>>>>>
>>>>> Are we going to get another version of the patch, or did everyone agree that
>>>>> the patch is good as it is and does not require further changes ?
>>>>>
>>>>
>>>> I can not imagine a different fix.
>>>
>>> my preferred fix would be:
>>>
>>> - add an imx35 compatible to all newer dtsi
>>> - update the driver to only write the wmcr on imx35 compatible devices
>>> adding only imx35.
>>>
>>
>> It breaks old DTS vs. new kernel compatibility, I've already mentioned this.
>
> Correct, and I didn't deny that. In my mail I pointed out the problem
> this imposes and I think it's small enough to not justify the complexity
> introduced in your proposed change.
>
I can not statistically estimate well the severity of the problem which was
fixed by commit 5fe65ce7cc (all boards with a similar change not found in
a bootloader will be affected, I believe) multiplied by the rate of users,
who don't update DTB. But statistically the probability is non-zero, and
thus it is better to consider that the new problem which I still resist to
introduce will hit somebody.
> If we cannot agree then at least this needs to be better documented in
> the driver because otherwise someone makes a cleanup later dropping all
> the compatibles that are needed to keep backwards compatibility.
>
My position is to avoid fixing a bug by introducing another known in
advance bug, IMHO it overrules your statement about data redundancy.
Here I'd like to separate responsibilities. If somebody cleans the list
of compatibles up and it accidentally causes problems, the author of that
commit will be blamed and that commit will be reverted, and the revert
commit won't touch my fix -- make i.MX31 boards boot again.
> But my suggestion is to first do the minimal fix, and if in the next
> merge window people lament about breakages on their machine, we can
> still extend the simple fix to a fully backwards compatible change.
Let's fix one problem stated in the commit subject cleanly and without
any known side effects like I suggest, a questionable and optional purge
of compatibles can be done by somebody else later on.
--
With best wishes,
Vladimir
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-23 11:21 ` Vladimir Zapolskiy
0 siblings, 0 replies; 48+ messages in thread
From: Vladimir Zapolskiy @ 2016-12-23 11:21 UTC (permalink / raw)
To: linux-arm-kernel
Hi Uwe,
On 12/23/2016 12:01 PM, Uwe Kleine-K?nig wrote:
> On Fri, Dec 23, 2016 at 11:27:24AM +0200, Vladimir Zapolskiy wrote:
>> On 12/23/2016 10:32 AM, Uwe Kleine-K?nig wrote:
>>> Hello,
>>>
>>> On Fri, Dec 23, 2016 at 10:20:20AM +0200, Vladimir Zapolskiy wrote:
>>>> On 12/23/2016 03:55 AM, Guenter Roeck wrote:
>>>>> What is the ultimate conclusion of this exchange ?
>>>>>
>>>>> Are we going to get another version of the patch, or did everyone agree that
>>>>> the patch is good as it is and does not require further changes ?
>>>>>
>>>>
>>>> I can not imagine a different fix.
>>>
>>> my preferred fix would be:
>>>
>>> - add an imx35 compatible to all newer dtsi
>>> - update the driver to only write the wmcr on imx35 compatible devices
>>> adding only imx35.
>>>
>>
>> It breaks old DTS vs. new kernel compatibility, I've already mentioned this.
>
> Correct, and I didn't deny that. In my mail I pointed out the problem
> this imposes and I think it's small enough to not justify the complexity
> introduced in your proposed change.
>
I can not statistically estimate well the severity of the problem which was
fixed by commit 5fe65ce7cc (all boards with a similar change not found in
a bootloader will be affected, I believe) multiplied by the rate of users,
who don't update DTB. But statistically the probability is non-zero, and
thus it is better to consider that the new problem which I still resist to
introduce will hit somebody.
> If we cannot agree then at least this needs to be better documented in
> the driver because otherwise someone makes a cleanup later dropping all
> the compatibles that are needed to keep backwards compatibility.
>
My position is to avoid fixing a bug by introducing another known in
advance bug, IMHO it overrules your statement about data redundancy.
Here I'd like to separate responsibilities. If somebody cleans the list
of compatibles up and it accidentally causes problems, the author of that
commit will be blamed and that commit will be reverted, and the revert
commit won't touch my fix -- make i.MX31 boards boot again.
> But my suggestion is to first do the minimal fix, and if in the next
> merge window people lament about breakages on their machine, we can
> still extend the simple fix to a fully backwards compatible change.
Let's fix one problem stated in the commit subject cleanly and without
any known side effects like I suggest, a questionable and optional purge
of compatibles can be done by somebody else later on.
--
With best wishes,
Vladimir
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-23 11:21 ` Vladimir Zapolskiy
@ 2016-12-23 14:11 ` Uwe Kleine-König
-1 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-23 14:11 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: linux-watchdog, Magnus Lilja, Vladimir Zapolskiy,
Wim Van Sebroeck, linux-arm-kernel, Sascha Hauer, Fabio Estevam,
Shawn Guo, Guenter Roeck
On Fri, Dec 23, 2016 at 01:21:20PM +0200, Vladimir Zapolskiy wrote:
> Hi Uwe,
>
> On 12/23/2016 12:01 PM, Uwe Kleine-König wrote:
> > On Fri, Dec 23, 2016 at 11:27:24AM +0200, Vladimir Zapolskiy wrote:
> >> On 12/23/2016 10:32 AM, Uwe Kleine-König wrote:
> >>> Hello,
> >>>
> >>> On Fri, Dec 23, 2016 at 10:20:20AM +0200, Vladimir Zapolskiy wrote:
> >>>> On 12/23/2016 03:55 AM, Guenter Roeck wrote:
> >>>>> What is the ultimate conclusion of this exchange ?
> >>>>>
> >>>>> Are we going to get another version of the patch, or did everyone agree that
> >>>>> the patch is good as it is and does not require further changes ?
> >>>>>
> >>>>
> >>>> I can not imagine a different fix.
> >>>
> >>> my preferred fix would be:
> >>>
> >>> - add an imx35 compatible to all newer dtsi
> >>> - update the driver to only write the wmcr on imx35 compatible devices
> >>> adding only imx35.
> >>>
> >>
> >> It breaks old DTS vs. new kernel compatibility, I've already mentioned this.
> >
> > Correct, and I didn't deny that. In my mail I pointed out the problem
> > this imposes and I think it's small enough to not justify the complexity
> > introduced in your proposed change.
> >
>
> I can not statistically estimate well the severity of the problem which was
> fixed by commit 5fe65ce7cc (all boards with a similar change not found in
> a bootloader will be affected, I believe) multiplied by the rate of users,
> who don't update DTB.
I think most people updating the kernel will update the dtb at the same
time. And for those who don't it should be quickly obvious that
something is wrong during development if the machine resets
unexpectedly. Plus I think that most users are not affected anyhow
(either because their bootloader fixes up accordingly or because most
machines don't reset when the timer expires because the hardware isn't
designed accordingly). After all between introduction of i.MX35/i.MX25
(not later than Thu Jun 4 11:32:12 2009 +0200, commit
8c25c36f33157a2e2a1fcd60b6dc00feace80631) and the broken commit
5fe65ce7cc (Mon Sep 8 09:14:07 2014 +0200) it seems it wasn't an issue.
With my suggestion the situation on an affected machine with old dtb and
new kernel is just as it was in these 5 years where nobody complained.
It's fine to target a bugfree system, and if you want to target that
that's applaudable. But then please make it easy for others after you to
not undo your good and add a big comment to the compatible array of the
driver explaining why they are all listed explicitly and how to prevent
to have to expand the list further.
So yes, my suggestion has a risk, but I think when weighting that
against the overhead that is introduced in the driver, I'd go the
simpler way. That's of course something personal and as it's you who
seems to have in interest in fixing the driver, it's your way that
matters more. And if you document your way good enough, I won't stop
you.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-23 14:11 ` Uwe Kleine-König
0 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-23 14:11 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Dec 23, 2016 at 01:21:20PM +0200, Vladimir Zapolskiy wrote:
> Hi Uwe,
>
> On 12/23/2016 12:01 PM, Uwe Kleine-K?nig wrote:
> > On Fri, Dec 23, 2016 at 11:27:24AM +0200, Vladimir Zapolskiy wrote:
> >> On 12/23/2016 10:32 AM, Uwe Kleine-K?nig wrote:
> >>> Hello,
> >>>
> >>> On Fri, Dec 23, 2016 at 10:20:20AM +0200, Vladimir Zapolskiy wrote:
> >>>> On 12/23/2016 03:55 AM, Guenter Roeck wrote:
> >>>>> What is the ultimate conclusion of this exchange ?
> >>>>>
> >>>>> Are we going to get another version of the patch, or did everyone agree that
> >>>>> the patch is good as it is and does not require further changes ?
> >>>>>
> >>>>
> >>>> I can not imagine a different fix.
> >>>
> >>> my preferred fix would be:
> >>>
> >>> - add an imx35 compatible to all newer dtsi
> >>> - update the driver to only write the wmcr on imx35 compatible devices
> >>> adding only imx35.
> >>>
> >>
> >> It breaks old DTS vs. new kernel compatibility, I've already mentioned this.
> >
> > Correct, and I didn't deny that. In my mail I pointed out the problem
> > this imposes and I think it's small enough to not justify the complexity
> > introduced in your proposed change.
> >
>
> I can not statistically estimate well the severity of the problem which was
> fixed by commit 5fe65ce7cc (all boards with a similar change not found in
> a bootloader will be affected, I believe) multiplied by the rate of users,
> who don't update DTB.
I think most people updating the kernel will update the dtb at the same
time. And for those who don't it should be quickly obvious that
something is wrong during development if the machine resets
unexpectedly. Plus I think that most users are not affected anyhow
(either because their bootloader fixes up accordingly or because most
machines don't reset when the timer expires because the hardware isn't
designed accordingly). After all between introduction of i.MX35/i.MX25
(not later than Thu Jun 4 11:32:12 2009 +0200, commit
8c25c36f33157a2e2a1fcd60b6dc00feace80631) and the broken commit
5fe65ce7cc (Mon Sep 8 09:14:07 2014 +0200) it seems it wasn't an issue.
With my suggestion the situation on an affected machine with old dtb and
new kernel is just as it was in these 5 years where nobody complained.
It's fine to target a bugfree system, and if you want to target that
that's applaudable. But then please make it easy for others after you to
not undo your good and add a big comment to the compatible array of the
driver explaining why they are all listed explicitly and how to prevent
to have to expand the list further.
So yes, my suggestion has a risk, but I think when weighting that
against the overhead that is introduced in the driver, I'd go the
simpler way. That's of course something personal and as it's you who
seems to have in interest in fixing the driver, it's your way that
matters more. And if you document your way good enough, I won't stop
you.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-23 14:11 ` Uwe Kleine-König
@ 2016-12-24 5:08 ` Vladimir Zapolskiy
-1 siblings, 0 replies; 48+ messages in thread
From: Vladimir Zapolskiy @ 2016-12-24 5:08 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-watchdog, Magnus Lilja, Wim Van Sebroeck, linux-arm-kernel,
Sascha Hauer, Fabio Estevam, Shawn Guo, Guenter Roeck
Hello Uwe,
On 12/23/2016 04:11 PM, Uwe Kleine-König wrote:
> On Fri, Dec 23, 2016 at 01:21:20PM +0200, Vladimir Zapolskiy wrote:
>> Hi Uwe,
>>
>> On 12/23/2016 12:01 PM, Uwe Kleine-König wrote:
>>> On Fri, Dec 23, 2016 at 11:27:24AM +0200, Vladimir Zapolskiy wrote:
>>>> On 12/23/2016 10:32 AM, Uwe Kleine-König wrote:
>>>>> Hello,
>>>>>
>>>>> On Fri, Dec 23, 2016 at 10:20:20AM +0200, Vladimir Zapolskiy wrote:
>>>>>> On 12/23/2016 03:55 AM, Guenter Roeck wrote:
>>>>>>> What is the ultimate conclusion of this exchange ?
>>>>>>>
>>>>>>> Are we going to get another version of the patch, or did everyone agree that
>>>>>>> the patch is good as it is and does not require further changes ?
>>>>>>>
>>>>>>
>>>>>> I can not imagine a different fix.
>>>>>
>>>>> my preferred fix would be:
>>>>>
>>>>> - add an imx35 compatible to all newer dtsi
>>>>> - update the driver to only write the wmcr on imx35 compatible devices
>>>>> adding only imx35.
>>>>>
>>>>
>>>> It breaks old DTS vs. new kernel compatibility, I've already mentioned this.
>>>
>>> Correct, and I didn't deny that. In my mail I pointed out the problem
>>> this imposes and I think it's small enough to not justify the complexity
>>> introduced in your proposed change.
>>>
>>
>> I can not statistically estimate well the severity of the problem which was
>> fixed by commit 5fe65ce7cc (all boards with a similar change not found in
>> a bootloader will be affected, I believe) multiplied by the rate of users,
>> who don't update DTB.
>
> I think most people updating the kernel will update the dtb at the same
> time. And for those who don't it should be quickly obvious that
> something is wrong during development if the machine resets
> unexpectedly. Plus I think that most users are not affected anyhow
> (either because their bootloader fixes up accordingly or because most
> machines don't reset when the timer expires because the hardware isn't
> designed accordingly). After all between introduction of i.MX35/i.MX25
> (not later than Thu Jun 4 11:32:12 2009 +0200, commit
> 8c25c36f33157a2e2a1fcd60b6dc00feace80631) and the broken commit
> 5fe65ce7cc (Mon Sep 8 09:14:07 2014 +0200) it seems it wasn't an issue.
>
> With my suggestion the situation on an affected machine with old dtb and
> new kernel is just as it was in these 5 years where nobody complained.
Please feel free to send the revert of 5fe65ce7cc, if you think that
the commit is unneeded.
>
> It's fine to target a bugfree system, and if you want to target that
> that's applaudable. But then please make it easy for others after you to
> not undo your good and add a big comment to the compatible array of the
> driver explaining why they are all listed explicitly and how to prevent
> to have to expand the list further.
Please clarify, what do you mean here by "undo my good"? Revert my fix
and break boot on i.MX31 again or something else?
>
> So yes, my suggestion has a risk, but I think when weighting that
> against the overhead that is introduced in the driver, I'd go the
> simpler way.
What overhead do you mean here? Prolonged time in a loop while finding
a compatible by looking at 10 more values?
That's the price of the accepted commit 5fe65ce7cc and overlooked
incompatibilities in hardware while writing DTSI files for i.MX SoCs.
> That's of course something personal and as it's you who
> seems to have in interest in fixing the driver, it's your way that
> matters more. And if you document your way good enough, I won't stop
> you.
>
We're going round in circles. And I don't understand what do you mean
by "documenting my way" here.
For clarity I do NOT object against changes in DTS, I do NOT object
to shrink the list of compatibles, I object to mix up the requested
DTS changes for practically every i.MX powered board, bug fix and
a new potential bug in one single change. The series of commits is
generally good, but it lacks atomicity property of a fix, and in
the middle of the series users have to update DTB, it is an overkill.
Is it possible to change DTS *only* and fix kernel boot problem? NO.
Is it possible to change driver *only* and fix kernel boot problem? YES.
Conclusion: the problem is within the driver, to solve the problem
it is sufficient to fix the driver.
As I see it what you propose with involving DTS changes is a solution
to *another* problem (boot time performance optimisation? amount of
driver's lines of code? DTS beautification?).
And obviously it makes no sense to send a DTS change plus error-prone
change plus the actual fix to linux-stable then, even if they are
split by commits.
Uwe, I'm disappointed that we still did not come to a conclusion,
would you agree to a compromise?
I'll add a comment to v2:
/*
* The list of compatibles is added to achieve backward compatibility
* between DTS and kernel while fixing the incompatibility between
* i.MX21/i.MX27/i.MX31 and i.MX25/i.MX35/etc. watchdog controllers.
*
* Please do *NOT* extend the list by adding a compatible value for
* a controller, which is compatible with one of the already listed.
*
* You may consider to shrink the list at your own risk, but this may
* break backward compatibility and users may have to update their DTB,
* which is good eventually.
*/
And you send the removal of the comment and compatibles as a *separate*
change, if you find such a long list of compatibles redundant.
--
With best wishes,
Vladimir
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-24 5:08 ` Vladimir Zapolskiy
0 siblings, 0 replies; 48+ messages in thread
From: Vladimir Zapolskiy @ 2016-12-24 5:08 UTC (permalink / raw)
To: linux-arm-kernel
Hello Uwe,
On 12/23/2016 04:11 PM, Uwe Kleine-K?nig wrote:
> On Fri, Dec 23, 2016 at 01:21:20PM +0200, Vladimir Zapolskiy wrote:
>> Hi Uwe,
>>
>> On 12/23/2016 12:01 PM, Uwe Kleine-K?nig wrote:
>>> On Fri, Dec 23, 2016 at 11:27:24AM +0200, Vladimir Zapolskiy wrote:
>>>> On 12/23/2016 10:32 AM, Uwe Kleine-K?nig wrote:
>>>>> Hello,
>>>>>
>>>>> On Fri, Dec 23, 2016 at 10:20:20AM +0200, Vladimir Zapolskiy wrote:
>>>>>> On 12/23/2016 03:55 AM, Guenter Roeck wrote:
>>>>>>> What is the ultimate conclusion of this exchange ?
>>>>>>>
>>>>>>> Are we going to get another version of the patch, or did everyone agree that
>>>>>>> the patch is good as it is and does not require further changes ?
>>>>>>>
>>>>>>
>>>>>> I can not imagine a different fix.
>>>>>
>>>>> my preferred fix would be:
>>>>>
>>>>> - add an imx35 compatible to all newer dtsi
>>>>> - update the driver to only write the wmcr on imx35 compatible devices
>>>>> adding only imx35.
>>>>>
>>>>
>>>> It breaks old DTS vs. new kernel compatibility, I've already mentioned this.
>>>
>>> Correct, and I didn't deny that. In my mail I pointed out the problem
>>> this imposes and I think it's small enough to not justify the complexity
>>> introduced in your proposed change.
>>>
>>
>> I can not statistically estimate well the severity of the problem which was
>> fixed by commit 5fe65ce7cc (all boards with a similar change not found in
>> a bootloader will be affected, I believe) multiplied by the rate of users,
>> who don't update DTB.
>
> I think most people updating the kernel will update the dtb at the same
> time. And for those who don't it should be quickly obvious that
> something is wrong during development if the machine resets
> unexpectedly. Plus I think that most users are not affected anyhow
> (either because their bootloader fixes up accordingly or because most
> machines don't reset when the timer expires because the hardware isn't
> designed accordingly). After all between introduction of i.MX35/i.MX25
> (not later than Thu Jun 4 11:32:12 2009 +0200, commit
> 8c25c36f33157a2e2a1fcd60b6dc00feace80631) and the broken commit
> 5fe65ce7cc (Mon Sep 8 09:14:07 2014 +0200) it seems it wasn't an issue.
>
> With my suggestion the situation on an affected machine with old dtb and
> new kernel is just as it was in these 5 years where nobody complained.
Please feel free to send the revert of 5fe65ce7cc, if you think that
the commit is unneeded.
>
> It's fine to target a bugfree system, and if you want to target that
> that's applaudable. But then please make it easy for others after you to
> not undo your good and add a big comment to the compatible array of the
> driver explaining why they are all listed explicitly and how to prevent
> to have to expand the list further.
Please clarify, what do you mean here by "undo my good"? Revert my fix
and break boot on i.MX31 again or something else?
>
> So yes, my suggestion has a risk, but I think when weighting that
> against the overhead that is introduced in the driver, I'd go the
> simpler way.
What overhead do you mean here? Prolonged time in a loop while finding
a compatible by looking at 10 more values?
That's the price of the accepted commit 5fe65ce7cc and overlooked
incompatibilities in hardware while writing DTSI files for i.MX SoCs.
> That's of course something personal and as it's you who
> seems to have in interest in fixing the driver, it's your way that
> matters more. And if you document your way good enough, I won't stop
> you.
>
We're going round in circles. And I don't understand what do you mean
by "documenting my way" here.
For clarity I do NOT object against changes in DTS, I do NOT object
to shrink the list of compatibles, I object to mix up the requested
DTS changes for practically every i.MX powered board, bug fix and
a new potential bug in one single change. The series of commits is
generally good, but it lacks atomicity property of a fix, and in
the middle of the series users have to update DTB, it is an overkill.
Is it possible to change DTS *only* and fix kernel boot problem? NO.
Is it possible to change driver *only* and fix kernel boot problem? YES.
Conclusion: the problem is within the driver, to solve the problem
it is sufficient to fix the driver.
As I see it what you propose with involving DTS changes is a solution
to *another* problem (boot time performance optimisation? amount of
driver's lines of code? DTS beautification?).
And obviously it makes no sense to send a DTS change plus error-prone
change plus the actual fix to linux-stable then, even if they are
split by commits.
Uwe, I'm disappointed that we still did not come to a conclusion,
would you agree to a compromise?
I'll add a comment to v2:
/*
* The list of compatibles is added to achieve backward compatibility
* between DTS and kernel while fixing the incompatibility between
* i.MX21/i.MX27/i.MX31 and i.MX25/i.MX35/etc. watchdog controllers.
*
* Please do *NOT* extend the list by adding a compatible value for
* a controller, which is compatible with one of the already listed.
*
* You may consider to shrink the list at your own risk, but this may
* break backward compatibility and users may have to update their DTB,
* which is good eventually.
*/
And you send the removal of the comment and compatibles as a *separate*
change, if you find such a long list of compatibles redundant.
--
With best wishes,
Vladimir
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-11 1:06 ` Vladimir Zapolskiy
@ 2016-12-24 13:00 ` Fabio Estevam
-1 siblings, 0 replies; 48+ messages in thread
From: Fabio Estevam @ 2016-12-24 13:00 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Shawn Guo, Wim Van Sebroeck, Guenter Roeck, linux-watchdog,
Magnus Lilja, Sascha Hauer, Uwe Kleine-König, Fabio Estevam,
linux-arm-kernel
On Sat, Dec 10, 2016 at 11:06 PM, Vladimir Zapolskiy <vz@mleia.com> wrote:
> static const struct of_device_id imx2_wdt_dt_ids[] = {
> - { .compatible = "fsl,imx21-wdt", },
> + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx25-wdt", },
> + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx35-wdt", },
> + { .compatible = "fsl,imx50-wdt", },
> + { .compatible = "fsl,imx51-wdt", },
> + { .compatible = "fsl,imx53-wdt", },
> + { .compatible = "fsl,imx6q-wdt", },
> + { .compatible = "fsl,imx6sl-wdt", },
> + { .compatible = "fsl,imx6sx-wdt", },
> + { .compatible = "fsl,imx6ul-wdt", },
> + { .compatible = "fsl,imx7d-wdt", },
> + { .compatible = "fsl,vf610-wdt", },
> { /* sentinel */ }
I understand this compatible list is not very nice, but in order to
keep old dtb's working I don't see a better solution, so:
Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-24 13:00 ` Fabio Estevam
0 siblings, 0 replies; 48+ messages in thread
From: Fabio Estevam @ 2016-12-24 13:00 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Dec 10, 2016 at 11:06 PM, Vladimir Zapolskiy <vz@mleia.com> wrote:
> static const struct of_device_id imx2_wdt_dt_ids[] = {
> - { .compatible = "fsl,imx21-wdt", },
> + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx25-wdt", },
> + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
> + { .compatible = "fsl,imx35-wdt", },
> + { .compatible = "fsl,imx50-wdt", },
> + { .compatible = "fsl,imx51-wdt", },
> + { .compatible = "fsl,imx53-wdt", },
> + { .compatible = "fsl,imx6q-wdt", },
> + { .compatible = "fsl,imx6sl-wdt", },
> + { .compatible = "fsl,imx6sx-wdt", },
> + { .compatible = "fsl,imx6ul-wdt", },
> + { .compatible = "fsl,imx7d-wdt", },
> + { .compatible = "fsl,vf610-wdt", },
> { /* sentinel */ }
I understand this compatible list is not very nice, but in order to
keep old dtb's working I don't see a better solution, so:
Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-24 13:00 ` Fabio Estevam
@ 2016-12-28 20:38 ` Uwe Kleine-König
-1 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-28 20:38 UTC (permalink / raw)
To: Fabio Estevam
Cc: Vladimir Zapolskiy, linux-watchdog, Wim Van Sebroeck,
Magnus Lilja, linux-arm-kernel, Sascha Hauer, Fabio Estevam,
Shawn Guo, Guenter Roeck
On Sat, Dec 24, 2016 at 11:00:59AM -0200, Fabio Estevam wrote:
> On Sat, Dec 10, 2016 at 11:06 PM, Vladimir Zapolskiy <vz@mleia.com> wrote:
>
> > static const struct of_device_id imx2_wdt_dt_ids[] = {
> > - { .compatible = "fsl,imx21-wdt", },
> > + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
> > + { .compatible = "fsl,imx25-wdt", },
> > + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
> > + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
> > + { .compatible = "fsl,imx35-wdt", },
> > + { .compatible = "fsl,imx50-wdt", },
> > + { .compatible = "fsl,imx51-wdt", },
> > + { .compatible = "fsl,imx53-wdt", },
> > + { .compatible = "fsl,imx6q-wdt", },
> > + { .compatible = "fsl,imx6sl-wdt", },
> > + { .compatible = "fsl,imx6sx-wdt", },
> > + { .compatible = "fsl,imx6ul-wdt", },
> > + { .compatible = "fsl,imx7d-wdt", },
> > + { .compatible = "fsl,vf610-wdt", },
> > { /* sentinel */ }
>
> I understand this compatible list is not very nice, but in order to
> keep old dtb's working I don't see a better solution, so:
>
> Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
You can at least drop fsl,imx27-wdt and fsl,imx31-wdt which then fall
back correctly to fsl,imx21-wdt.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-28 20:38 ` Uwe Kleine-König
0 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-28 20:38 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Dec 24, 2016 at 11:00:59AM -0200, Fabio Estevam wrote:
> On Sat, Dec 10, 2016 at 11:06 PM, Vladimir Zapolskiy <vz@mleia.com> wrote:
>
> > static const struct of_device_id imx2_wdt_dt_ids[] = {
> > - { .compatible = "fsl,imx21-wdt", },
> > + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
> > + { .compatible = "fsl,imx25-wdt", },
> > + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
> > + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
> > + { .compatible = "fsl,imx35-wdt", },
> > + { .compatible = "fsl,imx50-wdt", },
> > + { .compatible = "fsl,imx51-wdt", },
> > + { .compatible = "fsl,imx53-wdt", },
> > + { .compatible = "fsl,imx6q-wdt", },
> > + { .compatible = "fsl,imx6sl-wdt", },
> > + { .compatible = "fsl,imx6sx-wdt", },
> > + { .compatible = "fsl,imx6ul-wdt", },
> > + { .compatible = "fsl,imx7d-wdt", },
> > + { .compatible = "fsl,vf610-wdt", },
> > { /* sentinel */ }
>
> I understand this compatible list is not very nice, but in order to
> keep old dtb's working I don't see a better solution, so:
>
> Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
You can at least drop fsl,imx27-wdt and fsl,imx31-wdt which then fall
back correctly to fsl,imx21-wdt.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-24 5:08 ` Vladimir Zapolskiy
@ 2016-12-28 20:54 ` Uwe Kleine-König
-1 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-28 20:54 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: linux-watchdog, Wim Van Sebroeck, Magnus Lilja, Guenter Roeck,
Sascha Hauer, Fabio Estevam, Shawn Guo, linux-arm-kernel
Hello Vladimir,
On Sat, Dec 24, 2016 at 07:08:15AM +0200, Vladimir Zapolskiy wrote:
> Hello Uwe,
>
> On 12/23/2016 04:11 PM, Uwe Kleine-König wrote:
> > On Fri, Dec 23, 2016 at 01:21:20PM +0200, Vladimir Zapolskiy wrote:
> >> Hi Uwe,
> >>
> >> On 12/23/2016 12:01 PM, Uwe Kleine-König wrote:
> >>> On Fri, Dec 23, 2016 at 11:27:24AM +0200, Vladimir Zapolskiy wrote:
> >>>> On 12/23/2016 10:32 AM, Uwe Kleine-König wrote:
> >>>>> Hello,
> >>>>>
> >>>>> On Fri, Dec 23, 2016 at 10:20:20AM +0200, Vladimir Zapolskiy wrote:
> >>>>>> On 12/23/2016 03:55 AM, Guenter Roeck wrote:
> >>>>>>> What is the ultimate conclusion of this exchange ?
> >>>>>>>
> >>>>>>> Are we going to get another version of the patch, or did everyone agree that
> >>>>>>> the patch is good as it is and does not require further changes ?
> >>>>>>>
> >>>>>>
> >>>>>> I can not imagine a different fix.
> >>>>>
> >>>>> my preferred fix would be:
> >>>>>
> >>>>> - add an imx35 compatible to all newer dtsi
> >>>>> - update the driver to only write the wmcr on imx35 compatible devices
> >>>>> adding only imx35.
> >>>>>
> >>>>
> >>>> It breaks old DTS vs. new kernel compatibility, I've already mentioned this.
> >>>
> >>> Correct, and I didn't deny that. In my mail I pointed out the problem
> >>> this imposes and I think it's small enough to not justify the complexity
> >>> introduced in your proposed change.
> >>>
> >>
> >> I can not statistically estimate well the severity of the problem which was
> >> fixed by commit 5fe65ce7cc (all boards with a similar change not found in
> >> a bootloader will be affected, I believe) multiplied by the rate of users,
> >> who don't update DTB.
> >
> > I think most people updating the kernel will update the dtb at the same
> > time. And for those who don't it should be quickly obvious that
> > something is wrong during development if the machine resets
> > unexpectedly. Plus I think that most users are not affected anyhow
> > (either because their bootloader fixes up accordingly or because most
> > machines don't reset when the timer expires because the hardware isn't
> > designed accordingly). After all between introduction of i.MX35/i.MX25
> > (not later than Thu Jun 4 11:32:12 2009 +0200, commit
> > 8c25c36f33157a2e2a1fcd60b6dc00feace80631) and the broken commit
> > 5fe65ce7cc (Mon Sep 8 09:14:07 2014 +0200) it seems it wasn't an issue.
> >
> > With my suggestion the situation on an affected machine with old dtb and
> > new kernel is just as it was in these 5 years where nobody complained.
>
> Please feel free to send the revert of 5fe65ce7cc, if you think that
> the commit is unneeded.
I think there is at least a handful of machines that need it. And I
doubt any of them will get a new kernel without a new dtb.
> > It's fine to target a bugfree system, and if you want to target that
> > that's applaudable. But then please make it easy for others after you to
> > not undo your good and add a big comment to the compatible array of the
> > driver explaining why they are all listed explicitly and how to prevent
> > to have to expand the list further.
>
> Please clarify, what do you mean here by "undo my good"? Revert my fix
> and break boot on i.MX31 again or something else?
What I want is that if you add all these compatibles to the driver,
please also add a big comment explaining why they are there. Otherwise
someone with good intentions will come an cleanup.
> > So yes, my suggestion has a risk, but I think when weighting that
> > against the overhead that is introduced in the driver, I'd go the
> > simpler way.
>
> What overhead do you mean here? Prolonged time in a loop while finding
> a compatible by looking at 10 more values?
No, I don't think this will add relevant overhead. I want to keep the
driver simple for easier maintenance.
> That's the price of the accepted commit 5fe65ce7cc and overlooked
> incompatibilities in hardware while writing DTSI files for i.MX SoCs.
>
> > That's of course something personal and as it's you who
> > seems to have in interest in fixing the driver, it's your way that
> > matters more. And if you document your way good enough, I won't stop
> > you.
> >
>
> We're going round in circles. And I don't understand what do you mean
> by "documenting my way" here.
>
> For clarity I do NOT object against changes in DTS, I do NOT object
> to shrink the list of compatibles, I object to mix up the requested
> DTS changes for practically every i.MX powered board, bug fix and
> a new potential bug in one single change. The series of commits is
> generally good, but it lacks atomicity property of a fix, and in
> the middle of the series users have to update DTB, it is an overkill.
>
> Is it possible to change DTS *only* and fix kernel boot problem? NO.
> Is it possible to change driver *only* and fix kernel boot problem? YES.
> Conclusion: the problem is within the driver, to solve the problem
> it is sufficient to fix the driver.
This argumentation is wrong in general. Any fixable damage can be fixed
in code only by changing how the dtb is interpreted.
The problem here is that 5fe65ce7cc is right for i.MX35 (and others) but
wrong for i.MX21 (and others) and there is no differentiation between
these two types in the driver. So I think we agree the right thing to do
in the end is: Make the driver aware that there are two different
hardware types and in the dtsi make use of the right one.
> I'll add a comment to v2:
>
> /*
> * The list of compatibles is added to achieve backward compatibility
> * between DTS and kernel while fixing the incompatibility between
I'd write: "to achieve compatibility between old DTS and new kernel"
> * i.MX21/i.MX27/i.MX31 and i.MX25/i.MX35/etc. watchdog controllers.
> *
> * Please do *NOT* extend the list by adding a compatible value for
> * a controller, which is compatible with one of the already listed.
> *
> * You may consider to shrink the list at your own risk, but this may
> * break backward compatibility and users may have to update their DTB,
> * which is good eventually.
> */
>
> And you send the removal of the comment and compatibles as a *separate*
> change, if you find such a long list of compatibles redundant.
I'd do it the other way around and so get more and simpler patches:
- make driver aware of i.MX35
- fix dtsi to use i.MX35 type where applicable
and IMHO optionally:
- introduce compatibility stuff with the above comment
I hope we converge to a solution acceptable for you and me.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-28 20:54 ` Uwe Kleine-König
0 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-28 20:54 UTC (permalink / raw)
To: linux-arm-kernel
Hello Vladimir,
On Sat, Dec 24, 2016 at 07:08:15AM +0200, Vladimir Zapolskiy wrote:
> Hello Uwe,
>
> On 12/23/2016 04:11 PM, Uwe Kleine-K?nig wrote:
> > On Fri, Dec 23, 2016 at 01:21:20PM +0200, Vladimir Zapolskiy wrote:
> >> Hi Uwe,
> >>
> >> On 12/23/2016 12:01 PM, Uwe Kleine-K?nig wrote:
> >>> On Fri, Dec 23, 2016 at 11:27:24AM +0200, Vladimir Zapolskiy wrote:
> >>>> On 12/23/2016 10:32 AM, Uwe Kleine-K?nig wrote:
> >>>>> Hello,
> >>>>>
> >>>>> On Fri, Dec 23, 2016 at 10:20:20AM +0200, Vladimir Zapolskiy wrote:
> >>>>>> On 12/23/2016 03:55 AM, Guenter Roeck wrote:
> >>>>>>> What is the ultimate conclusion of this exchange ?
> >>>>>>>
> >>>>>>> Are we going to get another version of the patch, or did everyone agree that
> >>>>>>> the patch is good as it is and does not require further changes ?
> >>>>>>>
> >>>>>>
> >>>>>> I can not imagine a different fix.
> >>>>>
> >>>>> my preferred fix would be:
> >>>>>
> >>>>> - add an imx35 compatible to all newer dtsi
> >>>>> - update the driver to only write the wmcr on imx35 compatible devices
> >>>>> adding only imx35.
> >>>>>
> >>>>
> >>>> It breaks old DTS vs. new kernel compatibility, I've already mentioned this.
> >>>
> >>> Correct, and I didn't deny that. In my mail I pointed out the problem
> >>> this imposes and I think it's small enough to not justify the complexity
> >>> introduced in your proposed change.
> >>>
> >>
> >> I can not statistically estimate well the severity of the problem which was
> >> fixed by commit 5fe65ce7cc (all boards with a similar change not found in
> >> a bootloader will be affected, I believe) multiplied by the rate of users,
> >> who don't update DTB.
> >
> > I think most people updating the kernel will update the dtb at the same
> > time. And for those who don't it should be quickly obvious that
> > something is wrong during development if the machine resets
> > unexpectedly. Plus I think that most users are not affected anyhow
> > (either because their bootloader fixes up accordingly or because most
> > machines don't reset when the timer expires because the hardware isn't
> > designed accordingly). After all between introduction of i.MX35/i.MX25
> > (not later than Thu Jun 4 11:32:12 2009 +0200, commit
> > 8c25c36f33157a2e2a1fcd60b6dc00feace80631) and the broken commit
> > 5fe65ce7cc (Mon Sep 8 09:14:07 2014 +0200) it seems it wasn't an issue.
> >
> > With my suggestion the situation on an affected machine with old dtb and
> > new kernel is just as it was in these 5 years where nobody complained.
>
> Please feel free to send the revert of 5fe65ce7cc, if you think that
> the commit is unneeded.
I think there is at least a handful of machines that need it. And I
doubt any of them will get a new kernel without a new dtb.
> > It's fine to target a bugfree system, and if you want to target that
> > that's applaudable. But then please make it easy for others after you to
> > not undo your good and add a big comment to the compatible array of the
> > driver explaining why they are all listed explicitly and how to prevent
> > to have to expand the list further.
>
> Please clarify, what do you mean here by "undo my good"? Revert my fix
> and break boot on i.MX31 again or something else?
What I want is that if you add all these compatibles to the driver,
please also add a big comment explaining why they are there. Otherwise
someone with good intentions will come an cleanup.
> > So yes, my suggestion has a risk, but I think when weighting that
> > against the overhead that is introduced in the driver, I'd go the
> > simpler way.
>
> What overhead do you mean here? Prolonged time in a loop while finding
> a compatible by looking at 10 more values?
No, I don't think this will add relevant overhead. I want to keep the
driver simple for easier maintenance.
> That's the price of the accepted commit 5fe65ce7cc and overlooked
> incompatibilities in hardware while writing DTSI files for i.MX SoCs.
>
> > That's of course something personal and as it's you who
> > seems to have in interest in fixing the driver, it's your way that
> > matters more. And if you document your way good enough, I won't stop
> > you.
> >
>
> We're going round in circles. And I don't understand what do you mean
> by "documenting my way" here.
>
> For clarity I do NOT object against changes in DTS, I do NOT object
> to shrink the list of compatibles, I object to mix up the requested
> DTS changes for practically every i.MX powered board, bug fix and
> a new potential bug in one single change. The series of commits is
> generally good, but it lacks atomicity property of a fix, and in
> the middle of the series users have to update DTB, it is an overkill.
>
> Is it possible to change DTS *only* and fix kernel boot problem? NO.
> Is it possible to change driver *only* and fix kernel boot problem? YES.
> Conclusion: the problem is within the driver, to solve the problem
> it is sufficient to fix the driver.
This argumentation is wrong in general. Any fixable damage can be fixed
in code only by changing how the dtb is interpreted.
The problem here is that 5fe65ce7cc is right for i.MX35 (and others) but
wrong for i.MX21 (and others) and there is no differentiation between
these two types in the driver. So I think we agree the right thing to do
in the end is: Make the driver aware that there are two different
hardware types and in the dtsi make use of the right one.
> I'll add a comment to v2:
>
> /*
> * The list of compatibles is added to achieve backward compatibility
> * between DTS and kernel while fixing the incompatibility between
I'd write: "to achieve compatibility between old DTS and new kernel"
> * i.MX21/i.MX27/i.MX31 and i.MX25/i.MX35/etc. watchdog controllers.
> *
> * Please do *NOT* extend the list by adding a compatible value for
> * a controller, which is compatible with one of the already listed.
> *
> * You may consider to shrink the list at your own risk, but this may
> * break backward compatibility and users may have to update their DTB,
> * which is good eventually.
> */
>
> And you send the removal of the comment and compatibles as a *separate*
> change, if you find such a long list of compatibles redundant.
I'd do it the other way around and so get more and simpler patches:
- make driver aware of i.MX35
- fix dtsi to use i.MX35 type where applicable
and IMHO optionally:
- introduce compatibility stuff with the above comment
I hope we converge to a solution acceptable for you and me.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-28 20:54 ` Uwe Kleine-König
@ 2016-12-28 21:33 ` Fabio Estevam
-1 siblings, 0 replies; 48+ messages in thread
From: Fabio Estevam @ 2016-12-28 21:33 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Vladimir Zapolskiy, linux-watchdog, Wim Van Sebroeck,
Magnus Lilja, linux-arm-kernel, Sascha Hauer, Fabio Estevam,
Shawn Guo, Guenter Roeck
On Wed, Dec 28, 2016 at 6:54 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> I'd do it the other way around and so get more and simpler patches:
>
> - make driver aware of i.MX35
> - fix dtsi to use i.MX35 type where applicable
This would not work for old dtb's as they are not aware of the i.mx35 type.
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-28 21:33 ` Fabio Estevam
0 siblings, 0 replies; 48+ messages in thread
From: Fabio Estevam @ 2016-12-28 21:33 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Dec 28, 2016 at 6:54 PM, Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> wrote:
> I'd do it the other way around and so get more and simpler patches:
>
> - make driver aware of i.MX35
> - fix dtsi to use i.MX35 type where applicable
This would not work for old dtb's as they are not aware of the i.mx35 type.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-28 21:33 ` Fabio Estevam
@ 2016-12-28 21:46 ` Uwe Kleine-König
-1 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-28 21:46 UTC (permalink / raw)
To: Fabio Estevam
Cc: Vladimir Zapolskiy, linux-watchdog, Wim Van Sebroeck,
Magnus Lilja, linux-arm-kernel, Sascha Hauer, Fabio Estevam,
Shawn Guo, Guenter Roeck
On Wed, Dec 28, 2016 at 07:33:26PM -0200, Fabio Estevam wrote:
> On Wed, Dec 28, 2016 at 6:54 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
>
> > I'd do it the other way around and so get more and simpler patches:
> >
> > - make driver aware of i.MX35
> > - fix dtsi to use i.MX35 type where applicable
>
> This would not work for old dtb's as they are not aware of the i.mx35 type.
Right, so they would fall back to the i.MX21 type and not profit from
Markus' change until either the dtb is updated or the third commit is
added. I'll try to contact Markus to ask if he remembers more details.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-28 21:46 ` Uwe Kleine-König
0 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-28 21:46 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Dec 28, 2016 at 07:33:26PM -0200, Fabio Estevam wrote:
> On Wed, Dec 28, 2016 at 6:54 PM, Uwe Kleine-K?nig
> <u.kleine-koenig@pengutronix.de> wrote:
>
> > I'd do it the other way around and so get more and simpler patches:
> >
> > - make driver aware of i.MX35
> > - fix dtsi to use i.MX35 type where applicable
>
> This would not work for old dtb's as they are not aware of the i.mx35 type.
Right, so they would fall back to the i.MX21 type and not profit from
Markus' change until either the dtb is updated or the third commit is
added. I'll try to contact Markus to ask if he remembers more details.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-28 21:46 ` Uwe Kleine-König
@ 2016-12-28 23:39 ` Fabio Estevam
-1 siblings, 0 replies; 48+ messages in thread
From: Fabio Estevam @ 2016-12-28 23:39 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Vladimir Zapolskiy, linux-watchdog, Wim Van Sebroeck,
Magnus Lilja, linux-arm-kernel, Sascha Hauer, Fabio Estevam,
Shawn Guo, Guenter Roeck
On Wed, Dec 28, 2016 at 7:46 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Right, so they would fall back to the i.MX21 type and not profit from
> Markus' change until either the dtb is updated or the third commit is
> added. I'll try to contact Markus to ask if he remembers more details.
There is no profit from Markus' change for mx21/mx27/mx31.
As Vladimir explained in his commit log these SoCs do not have the
WMCR register, which causes a hang on these SoCs when we try to write
to it.
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-28 23:39 ` Fabio Estevam
0 siblings, 0 replies; 48+ messages in thread
From: Fabio Estevam @ 2016-12-28 23:39 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Dec 28, 2016 at 7:46 PM, Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> wrote:
> Right, so they would fall back to the i.MX21 type and not profit from
> Markus' change until either the dtb is updated or the third commit is
> added. I'll try to contact Markus to ask if he remembers more details.
There is no profit from Markus' change for mx21/mx27/mx31.
As Vladimir explained in his commit log these SoCs do not have the
WMCR register, which causes a hang on these SoCs when we try to write
to it.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-28 23:39 ` Fabio Estevam
@ 2016-12-29 22:04 ` Uwe Kleine-König
-1 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-29 22:04 UTC (permalink / raw)
To: Fabio Estevam
Cc: Vladimir Zapolskiy, linux-watchdog, Wim Van Sebroeck,
Magnus Lilja, linux-arm-kernel, Sascha Hauer, Fabio Estevam,
Shawn Guo, Guenter Roeck
On Wed, Dec 28, 2016 at 09:39:51PM -0200, Fabio Estevam wrote:
> On Wed, Dec 28, 2016 at 7:46 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
>
> > Right, so they would fall back to the i.MX21 type and not profit from
> > Markus' change until either the dtb is updated or the third commit is
> > added. I'll try to contact Markus to ask if he remembers more details.
>
> There is no profit from Markus' change for mx21/mx27/mx31.
Ack. If you teach the driver now about i.MX35 and only make use of the WMCR
register on this one, the possible breakage is that this register isn't
written to on the i.MX35 type until the dtb is updated.
> As Vladimir explained in his commit log these SoCs do not have the
> WMCR register, which causes a hang on these SoCs when we try to write
> to it.
So this should be fixed by adding support for i.MX35.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-29 22:04 ` Uwe Kleine-König
0 siblings, 0 replies; 48+ messages in thread
From: Uwe Kleine-König @ 2016-12-29 22:04 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Dec 28, 2016 at 09:39:51PM -0200, Fabio Estevam wrote:
> On Wed, Dec 28, 2016 at 7:46 PM, Uwe Kleine-K?nig
> <u.kleine-koenig@pengutronix.de> wrote:
>
> > Right, so they would fall back to the i.MX21 type and not profit from
> > Markus' change until either the dtb is updated or the third commit is
> > added. I'll try to contact Markus to ask if he remembers more details.
>
> There is no profit from Markus' change for mx21/mx27/mx31.
Ack. If you teach the driver now about i.MX35 and only make use of the WMCR
register on this one, the possible breakage is that this register isn't
written to on the i.MX35 type until the dtb is updated.
> As Vladimir explained in his commit log these SoCs do not have the
> WMCR register, which causes a hang on these SoCs when we try to write
> to it.
So this should be fixed by adding support for i.MX35.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
2016-12-29 22:04 ` Uwe Kleine-König
@ 2016-12-29 23:40 ` Fabio Estevam
-1 siblings, 0 replies; 48+ messages in thread
From: Fabio Estevam @ 2016-12-29 23:40 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Vladimir Zapolskiy, linux-watchdog, Wim Van Sebroeck,
Magnus Lilja, linux-arm-kernel, Sascha Hauer, Fabio Estevam,
Shawn Guo, Guenter Roeck
On Thu, Dec 29, 2016 at 8:04 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Ack. If you teach the driver now about i.MX35 and only make use of the WMCR
> register on this one, the possible breakage is that this register isn't
> written to on the i.MX35 type until the dtb is updated.
So basically you are saying that you don't care about old dtb compatibility.
As far as I can see this is the reason we have not made any progress
with this patch since September.
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
@ 2016-12-29 23:40 ` Fabio Estevam
0 siblings, 0 replies; 48+ messages in thread
From: Fabio Estevam @ 2016-12-29 23:40 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Dec 29, 2016 at 8:04 PM, Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> wrote:
> Ack. If you teach the driver now about i.MX35 and only make use of the WMCR
> register on this one, the possible breakage is that this register isn't
> written to on the i.MX35 type until the dtb is updated.
So basically you are saying that you don't care about old dtb compatibility.
As far as I can see this is the reason we have not made any progress
with this patch since September.
^ permalink raw reply [flat|nested] 48+ messages in thread
end of thread, other threads:[~2016-12-29 23:40 UTC | newest]
Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-11 1:06 [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs Vladimir Zapolskiy
2016-12-11 1:06 ` Vladimir Zapolskiy
2016-12-11 2:18 ` Guenter Roeck
2016-12-11 2:18 ` Guenter Roeck
2016-12-11 8:58 ` Magnus Lilja
2016-12-11 8:58 ` Magnus Lilja
2016-12-11 9:35 ` Uwe Kleine-König
2016-12-11 9:35 ` Uwe Kleine-König
2016-12-11 10:01 ` Vladimir Zapolskiy
2016-12-11 10:01 ` Vladimir Zapolskiy
2016-12-11 10:26 ` Uwe Kleine-König
2016-12-11 10:26 ` Uwe Kleine-König
2016-12-11 11:21 ` Vladimir Zapolskiy
2016-12-11 11:21 ` Vladimir Zapolskiy
2016-12-11 12:28 ` Uwe Kleine-König
2016-12-11 12:28 ` Uwe Kleine-König
2016-12-23 1:55 ` Guenter Roeck
2016-12-23 1:55 ` Guenter Roeck
2016-12-23 8:20 ` Vladimir Zapolskiy
2016-12-23 8:20 ` Vladimir Zapolskiy
2016-12-23 8:32 ` Uwe Kleine-König
2016-12-23 8:32 ` Uwe Kleine-König
2016-12-23 9:27 ` Vladimir Zapolskiy
2016-12-23 9:27 ` Vladimir Zapolskiy
2016-12-23 10:01 ` Uwe Kleine-König
2016-12-23 10:01 ` Uwe Kleine-König
2016-12-23 11:21 ` Vladimir Zapolskiy
2016-12-23 11:21 ` Vladimir Zapolskiy
2016-12-23 14:11 ` Uwe Kleine-König
2016-12-23 14:11 ` Uwe Kleine-König
2016-12-24 5:08 ` Vladimir Zapolskiy
2016-12-24 5:08 ` Vladimir Zapolskiy
2016-12-28 20:54 ` Uwe Kleine-König
2016-12-28 20:54 ` Uwe Kleine-König
2016-12-28 21:33 ` Fabio Estevam
2016-12-28 21:33 ` Fabio Estevam
2016-12-28 21:46 ` Uwe Kleine-König
2016-12-28 21:46 ` Uwe Kleine-König
2016-12-28 23:39 ` Fabio Estevam
2016-12-28 23:39 ` Fabio Estevam
2016-12-29 22:04 ` Uwe Kleine-König
2016-12-29 22:04 ` Uwe Kleine-König
2016-12-29 23:40 ` Fabio Estevam
2016-12-29 23:40 ` Fabio Estevam
2016-12-24 13:00 ` Fabio Estevam
2016-12-24 13:00 ` Fabio Estevam
2016-12-28 20:38 ` Uwe Kleine-König
2016-12-28 20:38 ` Uwe Kleine-König
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.