All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH v1] watchdog: Introduce watchdog driver for Intel Tangier
@ 2017-04-18 13:51 Andy Shevchenko
  2017-04-18 14:49 ` Lukasz Majewski
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2017-04-18 13:51 UTC (permalink / raw)
  To: u-boot

From: Felipe Balbi <felipe.balbi@linux.intel.com>

Add watchdog driver for Intel Tangier based platforms.

Signed-off-by: Vincent Tinelli <vincent.tinelli@intel.com>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 common/board_f.c               |  1 +
 drivers/watchdog/Kconfig       |  8 ++++++
 drivers/watchdog/Makefile      |  1 +
 drivers/watchdog/tangier_wdt.c | 63 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 73 insertions(+)
 create mode 100644 drivers/watchdog/tangier_wdt.c

diff --git a/common/board_f.c b/common/board_f.c
index d9431ee79a..ad1eae98a5 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -91,6 +91,7 @@ static int init_func_watchdog_init(void)
 	(defined(CONFIG_M68K) || defined(CONFIG_MICROBLAZE) || \
 	defined(CONFIG_SH) || defined(CONFIG_AT91SAM9_WATCHDOG) || \
 	defined(CONFIG_DESIGNWARE_WATCHDOG) || \
+	defined(CONFIG_TANGIER_WATCHDOG) || \
 	defined(CONFIG_IMX_WATCHDOG))
 	hw_watchdog_init();
 	puts("       Watchdog enabled\n");
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index dbdaafc149..66fe70dba1 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1,5 +1,13 @@
 menu "WATCHDOG support"
 
+config TANGIER_WATCHDOG
+	bool "Intel Tangier watchdog"
+	depends on INTEL_MID
+	help
+	  This enables support for watchdog controller available on
+	  Intel Tangier SoC. If you're using a board with Intel Tangier
+	  SoC, say Y here.
+
 config ULP_WATCHDOG
 	bool "i.MX7ULP watchdog"
 	help
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index dea18363ca..7b77d8379f 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -15,4 +15,5 @@ obj-$(CONFIG_XILINX_TB_WATCHDOG) += xilinx_tb_wdt.o
 obj-$(CONFIG_BFIN_WATCHDOG)  += bfin_wdt.o
 obj-$(CONFIG_OMAP_WATCHDOG) += omap_wdt.o
 obj-$(CONFIG_DESIGNWARE_WATCHDOG) += designware_wdt.o
+obj-$(CONFIG_TANGIER_WATCHDOG) += tangier_wdt.o
 obj-$(CONFIG_ULP_WATCHDOG) += ulp_wdog.o
diff --git a/drivers/watchdog/tangier_wdt.c b/drivers/watchdog/tangier_wdt.c
new file mode 100644
index 0000000000..23be71a42f
--- /dev/null
+++ b/drivers/watchdog/tangier_wdt.c
@@ -0,0 +1,63 @@
+/*
+ * Copyright (c) 2017 Intel Corporation
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+#include <common.h>
+#include <watchdog.h>
+#include <asm/scu.h>
+
+/* Hardware timeout in seconds */
+#ifndef CONFIG_WATCHDOG_TIMEOUT_MSECS
+#define WATCHDOG_HEARTBEAT 60000
+#else
+#define WATCHDOG_HEARTBEAT CONFIG_WATCHDOG_TIMEOUT_MSECS
+#endif
+
+enum {
+	SCU_WATCHDOG_START			= 0,
+	SCU_WATCHDOG_STOP			= 1,
+	SCU_WATCHDOG_KEEPALIVE			= 2,
+	SCU_WATCHDOG_SET_ACTION_ON_TIMEOUT	= 3,
+};
+
+void hw_watchdog_reset(void)
+{
+	static unsigned long prev;
+	unsigned long now;
+
+	if (gd->timer)
+		now = timer_get_us();
+	else
+		now = rdtsc() / 1000000;
+
+	/* Do not flood SCU */
+	if (unlikely((now - prev) > (WATCHDOG_HEARTBEAT * 1000))) {
+		prev = now;
+		scu_ipc_simple_command(IPCMSG_WATCHDOG_TIMER, SCU_WATCHDOG_KEEPALIVE);
+	}
+}
+
+int hw_watchdog_disable(void)
+{
+	return scu_ipc_simple_command(IPCMSG_WATCHDOG_TIMER, SCU_WATCHDOG_STOP);
+}
+
+void hw_watchdog_init(void)
+{
+	u32 timeout = WATCHDOG_HEARTBEAT / 1000;
+	int in_size;
+	struct ipc_wd_start {
+		u32 pretimeout;
+		u32 timeout;
+	} ipc_wd_start = { timeout, timeout };
+
+	/*
+	 * SCU expects the input size for watchdog IPC
+	 * to be based on 4 bytes
+	 */
+	in_size = DIV_ROUND_UP(sizeof(ipc_wd_start), 4);
+
+	scu_ipc_command(IPCMSG_WATCHDOG_TIMER, SCU_WATCHDOG_START,
+			(u32 *)&ipc_wd_start, in_size, NULL, 0);
+}
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH v1] watchdog: Introduce watchdog driver for Intel Tangier
  2017-04-18 13:51 [U-Boot] [PATCH v1] watchdog: Introduce watchdog driver for Intel Tangier Andy Shevchenko
@ 2017-04-18 14:49 ` Lukasz Majewski
  2017-04-18 14:57   ` Andy Shevchenko
  2017-07-04 19:48   ` Andy Shevchenko
  0 siblings, 2 replies; 10+ messages in thread
From: Lukasz Majewski @ 2017-04-18 14:49 UTC (permalink / raw)
  To: u-boot

Hi Andy,

> From: Felipe Balbi <felipe.balbi@linux.intel.com>
> 
> Add watchdog driver for Intel Tangier based platforms.
> 
> Signed-off-by: Vincent Tinelli <vincent.tinelli@intel.com>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  common/board_f.c               |  1 +
>  drivers/watchdog/Kconfig       |  8 ++++++
>  drivers/watchdog/Makefile      |  1 +
>  drivers/watchdog/tangier_wdt.c | 63
> ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 73
> insertions(+) create mode 100644 drivers/watchdog/tangier_wdt.c
> 
> diff --git a/common/board_f.c b/common/board_f.c
> index d9431ee79a..ad1eae98a5 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -91,6 +91,7 @@ static int init_func_watchdog_init(void)
>  	(defined(CONFIG_M68K) || defined(CONFIG_MICROBLAZE) || \
>  	defined(CONFIG_SH) || defined(CONFIG_AT91SAM9_WATCHDOG) || \
>  	defined(CONFIG_DESIGNWARE_WATCHDOG) || \
> +	defined(CONFIG_TANGIER_WATCHDOG) || \
>  	defined(CONFIG_IMX_WATCHDOG))

I have stumbled upon similar patch... There should be new Kconfig
option created and enabled in required SoCs.

>  	hw_watchdog_init();
>  	puts("       Watchdog enabled\n");
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index dbdaafc149..66fe70dba1 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1,5 +1,13 @@
>  menu "WATCHDOG support"
>  
> +config TANGIER_WATCHDOG
> +	bool "Intel Tangier watchdog"
> +	depends on INTEL_MID
> +	help
> +	  This enables support for watchdog controller available on
> +	  Intel Tangier SoC. If you're using a board with Intel
> Tangier
> +	  SoC, say Y here.
> +
>  config ULP_WATCHDOG
>  	bool "i.MX7ULP watchdog"
>  	help
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index dea18363ca..7b77d8379f 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -15,4 +15,5 @@ obj-$(CONFIG_XILINX_TB_WATCHDOG) += xilinx_tb_wdt.o
>  obj-$(CONFIG_BFIN_WATCHDOG)  += bfin_wdt.o
>  obj-$(CONFIG_OMAP_WATCHDOG) += omap_wdt.o
>  obj-$(CONFIG_DESIGNWARE_WATCHDOG) += designware_wdt.o
> +obj-$(CONFIG_TANGIER_WATCHDOG) += tangier_wdt.o
>  obj-$(CONFIG_ULP_WATCHDOG) += ulp_wdog.o
> diff --git a/drivers/watchdog/tangier_wdt.c
> b/drivers/watchdog/tangier_wdt.c new file mode 100644
> index 0000000000..23be71a42f
> --- /dev/null
> +++ b/drivers/watchdog/tangier_wdt.c
> @@ -0,0 +1,63 @@
> +/*
> + * Copyright (c) 2017 Intel Corporation
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +#include <common.h>
> +#include <watchdog.h>
> +#include <asm/scu.h>
> +
> +/* Hardware timeout in seconds */
> +#ifndef CONFIG_WATCHDOG_TIMEOUT_MSECS
> +#define WATCHDOG_HEARTBEAT 60000
> +#else
> +#define WATCHDOG_HEARTBEAT CONFIG_WATCHDOG_TIMEOUT_MSECS
> +#endif
> +
> +enum {
> +	SCU_WATCHDOG_START			= 0,
> +	SCU_WATCHDOG_STOP			= 1,
> +	SCU_WATCHDOG_KEEPALIVE			= 2,
> +	SCU_WATCHDOG_SET_ACTION_ON_TIMEOUT	= 3,
> +};
> +
> +void hw_watchdog_reset(void)
> +{
> +	static unsigned long prev;
> +	unsigned long now;
> +
> +	if (gd->timer)
> +		now = timer_get_us();
> +	else
> +		now = rdtsc() / 1000000;
> +
> +	/* Do not flood SCU */
> +	if (unlikely((now - prev) > (WATCHDOG_HEARTBEAT * 1000))) {
> +		prev = now;
> +		scu_ipc_simple_command(IPCMSG_WATCHDOG_TIMER,
> SCU_WATCHDOG_KEEPALIVE);
> +	}
> +}
> +
> +int hw_watchdog_disable(void)
> +{
> +	return scu_ipc_simple_command(IPCMSG_WATCHDOG_TIMER,
> SCU_WATCHDOG_STOP); +}
> +
> +void hw_watchdog_init(void)
> +{
> +	u32 timeout = WATCHDOG_HEARTBEAT / 1000;
> +	int in_size;
> +	struct ipc_wd_start {
> +		u32 pretimeout;
> +		u32 timeout;
> +	} ipc_wd_start = { timeout, timeout };
> +
> +	/*
> +	 * SCU expects the input size for watchdog IPC
> +	 * to be based on 4 bytes
> +	 */
> +	in_size = DIV_ROUND_UP(sizeof(ipc_wd_start), 4);
> +
> +	scu_ipc_command(IPCMSG_WATCHDOG_TIMER, SCU_WATCHDOG_START,
> +			(u32 *)&ipc_wd_start, in_size, NULL, 0);
> +}

The code seems OK, but recently patches to add wdt-uclass has been
posted:

http://patchwork.ozlabs.org/patch/751448/
http://patchwork.ozlabs.org/patch/751451/

Maybe it would be better to port this driver to the uclass from the
very beginning?


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH v1] watchdog: Introduce watchdog driver for Intel Tangier
  2017-04-18 14:49 ` Lukasz Majewski
@ 2017-04-18 14:57   ` Andy Shevchenko
  2017-04-18 16:30     ` Tom Rini
  2017-07-04 19:48   ` Andy Shevchenko
  1 sibling, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2017-04-18 14:57 UTC (permalink / raw)
  To: u-boot

On Tue, 2017-04-18 at 16:49 +0200, Lukasz Majewski wrote:
> Hi Andy,
> 
> > From: Felipe Balbi <felipe.balbi@linux.intel.com>
> > 
> > Add watchdog driver for Intel Tangier based platforms.
> > 
> > Signed-off-by: Vincent Tinelli <vincent.tinelli@intel.com>
> > Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  common/board_f.c               |  1 +
> >  drivers/watchdog/Kconfig       |  8 ++++++
> >  drivers/watchdog/Makefile      |  1 +
> >  drivers/watchdog/tangier_wdt.c | 63
> > ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 73
> > insertions(+) create mode 100644 drivers/watchdog/tangier_wdt.c
> > 
> > diff --git a/common/board_f.c b/common/board_f.c
> > index d9431ee79a..ad1eae98a5 100644
> > --- a/common/board_f.c
> > +++ b/common/board_f.c
> > @@ -91,6 +91,7 @@ static int init_func_watchdog_init(void)
> >  	(defined(CONFIG_M68K) || defined(CONFIG_MICROBLAZE) || \
> >  	defined(CONFIG_SH) || defined(CONFIG_AT91SAM9_WATCHDOG) ||
> > \
> >  	defined(CONFIG_DESIGNWARE_WATCHDOG) || \
> > +	defined(CONFIG_TANGIER_WATCHDOG) || \
> >  	defined(CONFIG_IMX_WATCHDOG))
> 
> I have stumbled upon similar patch... There should be new Kconfig
> option created and enabled in required SoCs.

I'm glad to use one if there is one.
For now I prefer to have it functional.

> The code seems OK, but recently patches to add wdt-uclass has been
> posted:
> 
> http://patchwork.ozlabs.org/patch/751448/
> http://patchwork.ozlabs.org/patch/751451/
> 
> Maybe it would be better to port this driver to the uclass from the
> very beginning?

Good point. Will do.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH v1] watchdog: Introduce watchdog driver for Intel Tangier
  2017-04-18 14:57   ` Andy Shevchenko
@ 2017-04-18 16:30     ` Tom Rini
  0 siblings, 0 replies; 10+ messages in thread
From: Tom Rini @ 2017-04-18 16:30 UTC (permalink / raw)
  To: u-boot

On Tue, Apr 18, 2017 at 05:57:31PM +0300, Andy Shevchenko wrote:
> On Tue, 2017-04-18 at 16:49 +0200, Lukasz Majewski wrote:
> > Hi Andy,
> > 
> > > From: Felipe Balbi <felipe.balbi@linux.intel.com>
> > > 
> > > Add watchdog driver for Intel Tangier based platforms.
> > > 
> > > Signed-off-by: Vincent Tinelli <vincent.tinelli@intel.com>
> > > Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  common/board_f.c               |  1 +
> > >  drivers/watchdog/Kconfig       |  8 ++++++
> > >  drivers/watchdog/Makefile      |  1 +
> > >  drivers/watchdog/tangier_wdt.c | 63
> > > ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 73
> > > insertions(+) create mode 100644 drivers/watchdog/tangier_wdt.c
> > > 
> > > diff --git a/common/board_f.c b/common/board_f.c
> > > index d9431ee79a..ad1eae98a5 100644
> > > --- a/common/board_f.c
> > > +++ b/common/board_f.c
> > > @@ -91,6 +91,7 @@ static int init_func_watchdog_init(void)
> > >  	(defined(CONFIG_M68K) || defined(CONFIG_MICROBLAZE) || \
> > >  	defined(CONFIG_SH) || defined(CONFIG_AT91SAM9_WATCHDOG) ||
> > > \
> > >  	defined(CONFIG_DESIGNWARE_WATCHDOG) || \
> > > +	defined(CONFIG_TANGIER_WATCHDOG) || \
> > >  	defined(CONFIG_IMX_WATCHDOG))
> > 
> > I have stumbled upon similar patch... There should be new Kconfig
> > option created and enabled in required SoCs.
> 
> I'm glad to use one if there is one.
> For now I prefer to have it functional.

We have time to get this right.  Looking at the code context,
CONFIG_HW_WATCHDOG_ENABLE, and some help text saying that it enables the
hardware watchdog during U-Boot's initialization process would be
enough.  Then select that in your next patch and you can leave the rest
of the conversion for later.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170418/9fe36ea0/attachment.sig>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH v1] watchdog: Introduce watchdog driver for Intel Tangier
  2017-04-18 14:49 ` Lukasz Majewski
  2017-04-18 14:57   ` Andy Shevchenko
@ 2017-07-04 19:48   ` Andy Shevchenko
  2017-07-04 20:31     ` Tom Rini
  1 sibling, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2017-07-04 19:48 UTC (permalink / raw)
  To: u-boot

On Tue, 2017-04-18 at 16:49 +0200, Lukasz Majewski wrote:
> 
> The code seems OK, but recently patches to add wdt-uclass has been
> posted:
> 
> http://patchwork.ozlabs.org/patch/751448/
> http://patchwork.ozlabs.org/patch/751451/
> 
> Maybe it would be better to port this driver to the uclass from the
> very beginning?

I started looking into this direction and have a question:
what the point to move to that class if it's broken from the beginning?

I really do not understand who on the earth would like to have
wdt_stop() at ->probe() (the only two users do exactly that!).

So, it looks to me now as bikeshedding, otherwise where is the
documentation which describes how this all stuff should work?

Can we go with the initial patch?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH v1] watchdog: Introduce watchdog driver for Intel Tangier
  2017-07-04 19:48   ` Andy Shevchenko
@ 2017-07-04 20:31     ` Tom Rini
  2017-07-04 20:41       ` Andy Shevchenko
  2017-07-05 15:59       ` Andy Shevchenko
  0 siblings, 2 replies; 10+ messages in thread
From: Tom Rini @ 2017-07-04 20:31 UTC (permalink / raw)
  To: u-boot

On Tue, Jul 04, 2017 at 10:48:36PM +0300, Andy Shevchenko wrote:
> On Tue, 2017-04-18 at 16:49 +0200, Lukasz Majewski wrote:
> > 
> > The code seems OK, but recently patches to add wdt-uclass has been
> > posted:
> > 
> > http://patchwork.ozlabs.org/patch/751448/
> > http://patchwork.ozlabs.org/patch/751451/
> > 
> > Maybe it would be better to port this driver to the uclass from the
> > very beginning?
> 
> I started looking into this direction and have a question:
> what the point to move to that class if it's broken from the beginning?
> 
> I really do not understand who on the earth would like to have
> wdt_stop() at ->probe() (the only two users do exactly that!).

It's quite possible I'm just missing something, but I don't see it.  If
the uclass is totally broken, can you make a suggestion on fixing it?  I
can see a common case being "lets turn off the watchdog now" and not
want a wdt until the full OS, but we must also fully support "U-Boot has
and pets the Watchdog".

> So, it looks to me now as bikeshedding, otherwise where is the
> documentation which describes how this all stuff should work?
> 
> Can we go with the initial patch?

If it looks like some nightmare to fix the DM uclass, I suppose, but, is
it really that bad off atm?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170704/b839097c/attachment.sig>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH v1] watchdog: Introduce watchdog driver for Intel Tangier
  2017-07-04 20:31     ` Tom Rini
@ 2017-07-04 20:41       ` Andy Shevchenko
  2017-07-04 21:23         ` lukma
  2017-07-05 15:59       ` Andy Shevchenko
  1 sibling, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2017-07-04 20:41 UTC (permalink / raw)
  To: u-boot

On Tue, 2017-07-04 at 16:31 -0400, Tom Rini wrote:
> On Tue, Jul 04, 2017 at 10:48:36PM +0300, Andy Shevchenko wrote:
> > On Tue, 2017-04-18 at 16:49 +0200, Lukasz Majewski wrote:
> > > 
> > > The code seems OK, but recently patches to add wdt-uclass has been
> > > posted:
> > > 
> > > http://patchwork.ozlabs.org/patch/751448/
> > > http://patchwork.ozlabs.org/patch/751451/
> > > 
> > > Maybe it would be better to port this driver to the uclass from
> > > the
> > > very beginning?
> > 
> > I started looking into this direction and have a question:
> > what the point to move to that class if it's broken from the
> > beginning?
> > 
> > I really do not understand who on the earth would like to have
> > wdt_stop() at ->probe() (the only two users do exactly that!).
> 
> It's quite possible I'm just missing something, but I don't see
> it.  If
> the uclass is totally broken, can you make a suggestion on fixing it?

I need to think about it (I have an idea, though it requires time to
try).

>   I
> can see a common case being "lets turn off the watchdog now" and not
> want a wdt until the full OS, but we must also fully support "U-Boot
> has
> and pets the Watchdog".

The users of WDT class now just disable watchdog. One of them, though,
is using it for reset (not my case for Tangier).

> > So, it looks to me now as bikeshedding, otherwise where is the
> > documentation which describes how this all stuff should work?
> > 
> > Can we go with the initial patch?
> 
> If it looks like some nightmare to fix the DM uclass, I suppose, but,
> is
> it really that bad off atm?

Not nightmare, but I really have not much time. Simon approached me
couple of weeks ago asking what is the status with my patches for
Tangier. That's why I found a slot to look at it.

So, I just have sent a v2 of this driver with old approach to move
things forward, otherwise I have no idea when it could be done.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH v1] watchdog: Introduce watchdog driver for Intel Tangier
  2017-07-04 20:41       ` Andy Shevchenko
@ 2017-07-04 21:23         ` lukma
  2017-07-04 22:36           ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: lukma @ 2017-07-04 21:23 UTC (permalink / raw)
  To: u-boot

Hi Andy,

> On Tue, 2017-07-04 at 16:31 -0400, Tom Rini wrote:
>> On Tue, Jul 04, 2017 at 10:48:36PM +0300, Andy Shevchenko wrote:
>>> On Tue, 2017-04-18 at 16:49 +0200, Lukasz Majewski wrote:
>>>>
>>>> The code seems OK, but recently patches to add wdt-uclass has been
>>>> posted:
>>>>
>>>> http://patchwork.ozlabs.org/patch/751448/
>>>> http://patchwork.ozlabs.org/patch/751451/
>>>>
>>>> Maybe it would be better to port this driver to the uclass from
>>>> the
>>>> very beginning?
>>>
>>> I started looking into this direction and have a question:
>>> what the point to move to that class if it's broken from the
>>> beginning?
>>>
>>> I really do not understand who on the earth would like to have
>>> wdt_stop() at ->probe() (the only two users do exactly that!).

At least for TI (omap), please find explanation in the description of 
this commit:

https://patchwork.ozlabs.org/patch/729819/

>>
>> It's quite possible I'm just missing something, but I don't see
>> it.  If
>> the uclass is totally broken, can you make a suggestion on fixing it?
>
> I need to think about it (I have an idea, though it requires time to
> try).
>
>>   I
>> can see a common case being "lets turn off the watchdog now" and not
>> want a wdt until the full OS, but we must also fully support "U-Boot
>> has
>> and pets the Watchdog".
>
> The users of WDT class now just disable watchdog. One of them, though,
> is using it for reset (not my case for Tangier).
>
>>> So, it looks to me now as bikeshedding, otherwise where is the
>>> documentation which describes how this all stuff should work?
>>>
>>> Can we go with the initial patch?
>>
>> If it looks like some nightmare to fix the DM uclass, I suppose, but,
>> is
>> it really that bad off atm?
>
> Not nightmare, but I really have not much time. Simon approached me
> couple of weeks ago asking what is the status with my patches for
> Tangier. That's why I found a slot to look at it.
>
> So, I just have sent a v2 of this driver with old approach to move
> things forward, otherwise I have no idea when it could be done.
>


-- 
Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH v1] watchdog: Introduce watchdog driver for Intel Tangier
  2017-07-04 21:23         ` lukma
@ 2017-07-04 22:36           ` Andy Shevchenko
  0 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2017-07-04 22:36 UTC (permalink / raw)
  To: u-boot

On Wed, Jul 5, 2017 at 12:23 AM, lukma <lukma@denx.de> wrote:
>> On Tue, 2017-07-04 at 16:31 -0400, Tom Rini wrote:
>>> On Tue, Jul 04, 2017 at 10:48:36PM +0300, Andy Shevchenko wrote:
>>>> On Tue, 2017-04-18 at 16:49 +0200, Lukasz Majewski wrote:

>>>> I started looking into this direction and have a question:
>>>> what the point to move to that class if it's broken from the
>>>> beginning?
>>>>
>>>> I really do not understand who on the earth would like to have
>>>> wdt_stop() at ->probe() (the only two users do exactly that!).
>
>
> At least for TI (omap), please find explanation in the description of this
> commit:
>
> https://patchwork.ozlabs.org/patch/729819/

No, it doesn't clarify for users of WDT class. It's another story AFAICS.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH v1] watchdog: Introduce watchdog driver for Intel Tangier
  2017-07-04 20:31     ` Tom Rini
  2017-07-04 20:41       ` Andy Shevchenko
@ 2017-07-05 15:59       ` Andy Shevchenko
  1 sibling, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2017-07-05 15:59 UTC (permalink / raw)
  To: u-boot

On Tue, 2017-07-04 at 16:31 -0400, Tom Rini wrote:
> On Tue, Jul 04, 2017 at 10:48:36PM +0300, Andy Shevchenko wrote:
> > On Tue, 2017-04-18 at 16:49 +0200, Lukasz Majewski wrote:

> > So, it looks to me now as bikeshedding, otherwise where is the
> > documentation which describes how this all stuff should work?
> > 
> > Can we go with the initial patch?
> 
> If it looks like some nightmare to fix the DM uclass, I suppose, but,
> is
> it really that bad off atm?

So, I have looked more, I even start creating patches and it's a deep
hole. It looks like that class has nothing to do with the main purpose
of watchdog. There is no integration into U-Boot watchdog infrastructure
(to actually use it).

I'm about to test what I have, I need to correct a bit still, and will
send a v3 using old approach. I will send patches against WDT class  as
RFC, but I'm not going to support them. My opinion is steady now -- that
class is unusable in the current state.

P.S. If I'm missing something obvious I would like to hear it ASAP to
make my driver use that class.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-07-05 15:59 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-18 13:51 [U-Boot] [PATCH v1] watchdog: Introduce watchdog driver for Intel Tangier Andy Shevchenko
2017-04-18 14:49 ` Lukasz Majewski
2017-04-18 14:57   ` Andy Shevchenko
2017-04-18 16:30     ` Tom Rini
2017-07-04 19:48   ` Andy Shevchenko
2017-07-04 20:31     ` Tom Rini
2017-07-04 20:41       ` Andy Shevchenko
2017-07-04 21:23         ` lukma
2017-07-04 22:36           ` Andy Shevchenko
2017-07-05 15:59       ` Andy Shevchenko

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.