Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Neil Armstrong]

On 05/27/2016 03:48 PM, Guenter Roeck wrote:
> On 05/27/2016 01:25 AM, Neil Armstrong wrote:
> [ ... ]
 +data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
 +data->wdt_dev.min_hw_heartbeat_ms = 1;
>>>
>>> Does the device require a minimum time between heartbeats ?
>>> Just asking, because you violate it yourself below.
>>>
>>> If you want to set the minimum timeout, that would be min_timeout.
>>
>> Ok, these values are not very clear actually.
>>
> Hmmm .. yes, reading the description again, it doesn't really describe well
> what it is doing. Essentially, min_hw_heartbeat_ms is enforced by the watchdog
> core, and should be used if a watchdog hardware can not tolerate short 
> intervals
> between heartbeats. min_timeout is the minimum timeout value configurable from
> user space.
OK, I'll switch to min_timeout = 1.

> 

[ ... ]

>>>
>>> This is unusual. If the watchdog can be already running, it might make sense
>>> to tell the core about it (set WDOG_HW_RUNNING in the status field), so it
>>> can send heartbeats until user space opens the device.
>>
>> Yes, since meson_gxbb_wdt_set_timeout() already ping, this is useless.
>>
> 
> Not only that - if the watchdog _is_ already running at boot time, you should
> really set WDOG_HW_RUNNING to let the watchdog core know. You status function
> would come handy there.
> 
> if (meson_gxbb_wdt_status(data))// note the changed parameter
> set_bit(WDOG_HW_RUNNING, >wdt_dev.status);
Yes, it can be handy.

I will push this feature in a next version, I'll stick to a simpler behavior 
and check
if it would be running before the kernel starts.

Thanks,
Neil

> 
> Thanks,
> Guenter
> 

Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Neil Armstrong]

On 05/27/2016 03:48 PM, Guenter Roeck wrote:
> On 05/27/2016 01:25 AM, Neil Armstrong wrote:
> [ ... ]
 +data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
 +data->wdt_dev.min_hw_heartbeat_ms = 1;
>>>
>>> Does the device require a minimum time between heartbeats ?
>>> Just asking, because you violate it yourself below.
>>>
>>> If you want to set the minimum timeout, that would be min_timeout.
>>
>> Ok, these values are not very clear actually.
>>
> Hmmm .. yes, reading the description again, it doesn't really describe well
> what it is doing. Essentially, min_hw_heartbeat_ms is enforced by the watchdog
> core, and should be used if a watchdog hardware can not tolerate short 
> intervals
> between heartbeats. min_timeout is the minimum timeout value configurable from
> user space.
OK, I'll switch to min_timeout = 1.

> 

[ ... ]

>>>
>>> This is unusual. If the watchdog can be already running, it might make sense
>>> to tell the core about it (set WDOG_HW_RUNNING in the status field), so it
>>> can send heartbeats until user space opens the device.
>>
>> Yes, since meson_gxbb_wdt_set_timeout() already ping, this is useless.
>>
> 
> Not only that - if the watchdog _is_ already running at boot time, you should
> really set WDOG_HW_RUNNING to let the watchdog core know. You status function
> would come handy there.
> 
> if (meson_gxbb_wdt_status(data))// note the changed parameter
> set_bit(WDOG_HW_RUNNING, >wdt_dev.status);
Yes, it can be handy.

I will push this feature in a next version, I'll stick to a simpler behavior 
and check
if it would be running before the kernel starts.

Thanks,
Neil

> 
> Thanks,
> Guenter
> 

Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Guenter Roeck]

On 05/27/2016 01:25 AM, Neil Armstrong wrote:
[ ... ]

+data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
+data->wdt_dev.min_hw_heartbeat_ms = 1;


Does the device require a minimum time between heartbeats ?
Just asking, because you violate it yourself below.

If you want to set the minimum timeout, that would be min_timeout.


Ok, these values are not very clear actually.


Hmmm .. yes, reading the description again, it doesn't really describe well
what it is doing. Essentially, min_hw_heartbeat_ms is enforced by the watchdog
core, and should be used if a watchdog hardware can not tolerate short intervals
between heartbeats. min_timeout is the minimum timeout value configurable from
user space.


+data->wdt_dev.timeout = DEFAULT_TIMEOUT;
+watchdog_set_drvdata(>wdt_dev, data);
+
+watchdog_init_timeout(>wdt_dev, DEFAULT_TIMEOUT, >dev);
+


DEFAULT_TIMEOUT is unnecessary here. Since the default timeout is already set,
it makes the call useless.


+ret = watchdog_register_device(>wdt_dev);
+if (ret)
+return ret;
+
+/* Setup with 1ms timebase */
+writel(((clk_get_rate(data->clk) / 1000) & GXBB_WDT_CTRL_DIV_MASK) |
+GXBB_WDT_CTRL_EE_RESET |
+GXBB_WDT_CTRL_CLK_EN |
+GXBB_WDT_CTRL_CLKDIV_EN,
+data->reg_base + GXBB_WDT_CTRL_REG);
+meson_gxbb_wdt_set_timeout(>wdt_dev, data->wdt_dev.timeout);


set_timeout already calls the ping function. Also, the functions should be 
called
prior to watchdog registration to avoid race conditions.


+meson_gxbb_wdt_ping(>wdt_dev);


This is unusual. If the watchdog can be already running, it might make sense
to tell the core about it (set WDOG_HW_RUNNING in the status field), so it
can send heartbeats until user space opens the device.


Yes, since meson_gxbb_wdt_set_timeout() already ping, this is useless.



Not only that - if the watchdog _is_ already running at boot time, you should
really set WDOG_HW_RUNNING to let the watchdog core know. You status function
would come handy there.

if (meson_gxbb_wdt_status(data))// note the changed parameter
set_bit(WDOG_HW_RUNNING, >wdt_dev.status);

Thanks,
Guenter

Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Guenter Roeck]

On 05/27/2016 01:25 AM, Neil Armstrong wrote:
[ ... ]

+data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
+data->wdt_dev.min_hw_heartbeat_ms = 1;


Does the device require a minimum time between heartbeats ?
Just asking, because you violate it yourself below.

If you want to set the minimum timeout, that would be min_timeout.


Ok, these values are not very clear actually.


Hmmm .. yes, reading the description again, it doesn't really describe well
what it is doing. Essentially, min_hw_heartbeat_ms is enforced by the watchdog
core, and should be used if a watchdog hardware can not tolerate short intervals
between heartbeats. min_timeout is the minimum timeout value configurable from
user space.


+data->wdt_dev.timeout = DEFAULT_TIMEOUT;
+watchdog_set_drvdata(>wdt_dev, data);
+
+watchdog_init_timeout(>wdt_dev, DEFAULT_TIMEOUT, >dev);
+


DEFAULT_TIMEOUT is unnecessary here. Since the default timeout is already set,
it makes the call useless.


+ret = watchdog_register_device(>wdt_dev);
+if (ret)
+return ret;
+
+/* Setup with 1ms timebase */
+writel(((clk_get_rate(data->clk) / 1000) & GXBB_WDT_CTRL_DIV_MASK) |
+GXBB_WDT_CTRL_EE_RESET |
+GXBB_WDT_CTRL_CLK_EN |
+GXBB_WDT_CTRL_CLKDIV_EN,
+data->reg_base + GXBB_WDT_CTRL_REG);
+meson_gxbb_wdt_set_timeout(>wdt_dev, data->wdt_dev.timeout);


set_timeout already calls the ping function. Also, the functions should be 
called
prior to watchdog registration to avoid race conditions.


+meson_gxbb_wdt_ping(>wdt_dev);


This is unusual. If the watchdog can be already running, it might make sense
to tell the core about it (set WDOG_HW_RUNNING in the status field), so it
can send heartbeats until user space opens the device.


Yes, since meson_gxbb_wdt_set_timeout() already ping, this is useless.



Not only that - if the watchdog _is_ already running at boot time, you should
really set WDOG_HW_RUNNING to let the watchdog core know. You status function
would come handy there.

if (meson_gxbb_wdt_status(data))// note the changed parameter
set_bit(WDOG_HW_RUNNING, >wdt_dev.status);

Thanks,
Guenter

Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Neil Armstrong]

On 05/26/2016 03:54 PM, Guenter Roeck wrote:
> On 05/26/2016 12:51 AM, Neil Armstrong wrote:
>> Add watchdog specific driver for Amlogic Meson GXBB SoC.
>>
> 
> Wondering - why RFC ?

For these precious comments !
Thanks Guenter.

> 
>> Signed-off-by: Neil Armstrong 
>> ---
>>   drivers/watchdog/Makefile |   1 +
>>   drivers/watchdog/meson_gxbb_wdt.c | 287 
>> ++
>>   2 files changed, 288 insertions(+)
>>   create mode 100644 drivers/watchdog/meson_gxbb_wdt.c
>>
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 9bde095..7679d93 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile

[...]

>> +
>> +unsigned int meson_gxbb_wdt_status(struct watchdog_device *wdt_dev)
>> +{
>> +struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +
>> +return !!(readl(data->reg_base + GXBB_WDT_CTRL_REG) & GXBB_WDT_CTRL_EN);
> 
> This function is not supposed to return 0/1 but a bit mask with relevant
> WDIOF_bits set. sbsa_gwdt.c is buggy (oops, and I reviewed it ;-), so
> please don't use it as example; it doesn't make much sense to return
> a kernel-only flag to user space.
> 
> Overall I would suggest to not implement the function at all. We'll have
> to revisit it - its current implementation in the watchdog core does not
> make much sense and for the most part only returns 0 to user space.
> The only driver implementing it (sbsa) returns a bad value. It is also
> _not_ supposed to return the "watchdog running" status as currently
> implemented (there is no WDIOF_ flag indicating that the watchdog is
> running).

Ok, will remove it.

>> +}
>> +
>> +int meson_gxbb_wdt_ping(struct watchdog_device *wdt_dev)
>> +{
>> +struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +
>> +writel(0, data->reg_base + GXBB_WDT_RSET_REG);
>> +
>> +return 0;
>> +}
>> +
>> +int meson_gxbb_wdt_set_timeout(struct watchdog_device *wdt_dev,
>> +   unsigned int timeout)
>> +{
>> +struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +
>> +if (watchdog_active(wdt_dev))
>> +meson_gxbb_wdt_stop(wdt_dev);
>> +
> 
> Is the stop/start sequence mandatory ? It leaves the system unprotected
> for a few cycles, so it is undesirable.

No, it's not mandatory, it's a good point.

>> +
>> +data->wdt_dev.parent = >dev;
>> +data->wdt_dev.info = _gxbb_wdt_info;
>> +data->wdt_dev.ops = _gxbb_wdt_ops;
>> +data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
>> +data->wdt_dev.min_hw_heartbeat_ms = 1;
> 
> Does the device require a minimum time between heartbeats ?
> Just asking, because you violate it yourself below.
> 
> If you want to set the minimum timeout, that would be min_timeout.

Ok, these values are not very clear actually.

>> +data->wdt_dev.timeout = DEFAULT_TIMEOUT;
>> +watchdog_set_drvdata(>wdt_dev, data);
>> +
>> +watchdog_init_timeout(>wdt_dev, DEFAULT_TIMEOUT, >dev);
>> +
> 
> DEFAULT_TIMEOUT is unnecessary here. Since the default timeout is already set,
> it makes the call useless.
> 
>> +ret = watchdog_register_device(>wdt_dev);
>> +if (ret)
>> +return ret;
>> +
>> +/* Setup with 1ms timebase */
>> +writel(((clk_get_rate(data->clk) / 1000) & GXBB_WDT_CTRL_DIV_MASK) |
>> +GXBB_WDT_CTRL_EE_RESET |
>> +GXBB_WDT_CTRL_CLK_EN |
>> +GXBB_WDT_CTRL_CLKDIV_EN,
>> +data->reg_base + GXBB_WDT_CTRL_REG);
>> +meson_gxbb_wdt_set_timeout(>wdt_dev, data->wdt_dev.timeout);
> 
> set_timeout already calls the ping function. Also, the functions should be 
> called
> prior to watchdog registration to avoid race conditions.
> 
>> +meson_gxbb_wdt_ping(>wdt_dev);
> 
> This is unusual. If the watchdog can be already running, it might make sense
> to tell the core about it (set WDOG_HW_RUNNING in the status field), so it
> can send heartbeats until user space opens the device.

Yes, since meson_gxbb_wdt_set_timeout() already ping, this is useless.

Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Neil Armstrong]

On 05/26/2016 03:54 PM, Guenter Roeck wrote:
> On 05/26/2016 12:51 AM, Neil Armstrong wrote:
>> Add watchdog specific driver for Amlogic Meson GXBB SoC.
>>
> 
> Wondering - why RFC ?

For these precious comments !
Thanks Guenter.

> 
>> Signed-off-by: Neil Armstrong 
>> ---
>>   drivers/watchdog/Makefile |   1 +
>>   drivers/watchdog/meson_gxbb_wdt.c | 287 
>> ++
>>   2 files changed, 288 insertions(+)
>>   create mode 100644 drivers/watchdog/meson_gxbb_wdt.c
>>
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 9bde095..7679d93 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile

[...]

>> +
>> +unsigned int meson_gxbb_wdt_status(struct watchdog_device *wdt_dev)
>> +{
>> +struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +
>> +return !!(readl(data->reg_base + GXBB_WDT_CTRL_REG) & GXBB_WDT_CTRL_EN);
> 
> This function is not supposed to return 0/1 but a bit mask with relevant
> WDIOF_bits set. sbsa_gwdt.c is buggy (oops, and I reviewed it ;-), so
> please don't use it as example; it doesn't make much sense to return
> a kernel-only flag to user space.
> 
> Overall I would suggest to not implement the function at all. We'll have
> to revisit it - its current implementation in the watchdog core does not
> make much sense and for the most part only returns 0 to user space.
> The only driver implementing it (sbsa) returns a bad value. It is also
> _not_ supposed to return the "watchdog running" status as currently
> implemented (there is no WDIOF_ flag indicating that the watchdog is
> running).

Ok, will remove it.

>> +}
>> +
>> +int meson_gxbb_wdt_ping(struct watchdog_device *wdt_dev)
>> +{
>> +struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +
>> +writel(0, data->reg_base + GXBB_WDT_RSET_REG);
>> +
>> +return 0;
>> +}
>> +
>> +int meson_gxbb_wdt_set_timeout(struct watchdog_device *wdt_dev,
>> +   unsigned int timeout)
>> +{
>> +struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +
>> +if (watchdog_active(wdt_dev))
>> +meson_gxbb_wdt_stop(wdt_dev);
>> +
> 
> Is the stop/start sequence mandatory ? It leaves the system unprotected
> for a few cycles, so it is undesirable.

No, it's not mandatory, it's a good point.

>> +
>> +data->wdt_dev.parent = >dev;
>> +data->wdt_dev.info = _gxbb_wdt_info;
>> +data->wdt_dev.ops = _gxbb_wdt_ops;
>> +data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
>> +data->wdt_dev.min_hw_heartbeat_ms = 1;
> 
> Does the device require a minimum time between heartbeats ?
> Just asking, because you violate it yourself below.
> 
> If you want to set the minimum timeout, that would be min_timeout.

Ok, these values are not very clear actually.

>> +data->wdt_dev.timeout = DEFAULT_TIMEOUT;
>> +watchdog_set_drvdata(>wdt_dev, data);
>> +
>> +watchdog_init_timeout(>wdt_dev, DEFAULT_TIMEOUT, >dev);
>> +
> 
> DEFAULT_TIMEOUT is unnecessary here. Since the default timeout is already set,
> it makes the call useless.
> 
>> +ret = watchdog_register_device(>wdt_dev);
>> +if (ret)
>> +return ret;
>> +
>> +/* Setup with 1ms timebase */
>> +writel(((clk_get_rate(data->clk) / 1000) & GXBB_WDT_CTRL_DIV_MASK) |
>> +GXBB_WDT_CTRL_EE_RESET |
>> +GXBB_WDT_CTRL_CLK_EN |
>> +GXBB_WDT_CTRL_CLKDIV_EN,
>> +data->reg_base + GXBB_WDT_CTRL_REG);
>> +meson_gxbb_wdt_set_timeout(>wdt_dev, data->wdt_dev.timeout);
> 
> set_timeout already calls the ping function. Also, the functions should be 
> called
> prior to watchdog registration to avoid race conditions.
> 
>> +meson_gxbb_wdt_ping(>wdt_dev);
> 
> This is unusual. If the watchdog can be already running, it might make sense
> to tell the core about it (set WDOG_HW_RUNNING in the status field), so it
> can send heartbeats until user space opens the device.

Yes, since meson_gxbb_wdt_set_timeout() already ping, this is useless.

Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Neil Armstrong]

On 05/26/2016 12:06 PM, Carlo Caione wrote:
> On 26/05/16 09:51, Neil Armstrong wrote:
>> Add watchdog specific driver for Amlogic Meson GXBB SoC.
>>
>> Signed-off-by: Neil Armstrong 
>> +
> 
> [...]
>> +#define DEFAULT_TIMEOUT 10  /* seconds */
>> +
>> +#define GXBB_WDT_CTRL_REG   0x0
>> +#define GXBB_WDT_CTRL1_REG  0x4
>> +#define GXBB_WDT_TCNT_REG   0x8
>> +#define GXBB_WDT_RSET_REG   0xc
>> +
>> +#define GXBB_WDT_CTRL_EE_RESET_NOW  BIT(26)
>> +
>> +#define GXBB_WDT_CTRL_CLKDIV_EN BIT(25)
>> +#define GXBB_WDT_CTRL_CLK_ENBIT(24)
>> +#define GXBB_WDT_CTRL_IRQ_ENBIT(23)
>> +#define GXBB_WDT_CTRL_EE_RESET  BIT(21)
>> +#define GXBB_WDT_CTRL_XTAL_SEL  (0)
>> +#define GXBB_WDT_CTRL_CLK81_SEL BIT(19)
>> +#define GXBB_WDT_CTRL_ENBIT(18)
>> +#define GXBB_WDT_CTRL_DIV_MASK  (BIT(18)-1)
>> +
>> +#define GXBB_WDT_CTRL1_GPIO_PULSE   BIT(17)
>> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0 BIT(16)
>> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1 (0)
>> +#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT   (BIT(16)-1)
>> +
>> +#define GXBB_WDT_TCNT_SETUP_MASK(BIT(16)-1)
>> +#define GXBB_WDT_TCNT_CNT_SHIFT 16
> 
> Indentation
> 
> [...]
>> +int meson_gxbb_wdt_set_timeout(struct watchdog_device *wdt_dev,
>> +   unsigned int timeout)
>> +{
>> +struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +
>> +if (watchdog_active(wdt_dev))
>> +meson_gxbb_wdt_stop(wdt_dev);
>> +
>> +meson_gxbb_wdt_ping(wdt_dev);
>> +
>> +writel(timeout*1000, data->reg_base + GXBB_WDT_TCNT_REG);
> 
> nit: spaces around "*"
> 
> [...]
>> +data->clk = devm_clk_get(>dev, NULL);
>> +if (IS_ERR(data->clk))
>> +return PTR_ERR(data->clk);
>> +
>> +clk_prepare_enable(data->clk);
> 
> Do we need to merge the clock controller driver before this?

It's not necessary, currently it only selects the xtal source, so it's works 
with the current upstream architecture.

Neil

> 
> Cheers,
> 

Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Neil Armstrong]

On 05/26/2016 12:06 PM, Carlo Caione wrote:
> On 26/05/16 09:51, Neil Armstrong wrote:
>> Add watchdog specific driver for Amlogic Meson GXBB SoC.
>>
>> Signed-off-by: Neil Armstrong 
>> +
> 
> [...]
>> +#define DEFAULT_TIMEOUT 10  /* seconds */
>> +
>> +#define GXBB_WDT_CTRL_REG   0x0
>> +#define GXBB_WDT_CTRL1_REG  0x4
>> +#define GXBB_WDT_TCNT_REG   0x8
>> +#define GXBB_WDT_RSET_REG   0xc
>> +
>> +#define GXBB_WDT_CTRL_EE_RESET_NOW  BIT(26)
>> +
>> +#define GXBB_WDT_CTRL_CLKDIV_EN BIT(25)
>> +#define GXBB_WDT_CTRL_CLK_ENBIT(24)
>> +#define GXBB_WDT_CTRL_IRQ_ENBIT(23)
>> +#define GXBB_WDT_CTRL_EE_RESET  BIT(21)
>> +#define GXBB_WDT_CTRL_XTAL_SEL  (0)
>> +#define GXBB_WDT_CTRL_CLK81_SEL BIT(19)
>> +#define GXBB_WDT_CTRL_ENBIT(18)
>> +#define GXBB_WDT_CTRL_DIV_MASK  (BIT(18)-1)
>> +
>> +#define GXBB_WDT_CTRL1_GPIO_PULSE   BIT(17)
>> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0 BIT(16)
>> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1 (0)
>> +#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT   (BIT(16)-1)
>> +
>> +#define GXBB_WDT_TCNT_SETUP_MASK(BIT(16)-1)
>> +#define GXBB_WDT_TCNT_CNT_SHIFT 16
> 
> Indentation
> 
> [...]
>> +int meson_gxbb_wdt_set_timeout(struct watchdog_device *wdt_dev,
>> +   unsigned int timeout)
>> +{
>> +struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +
>> +if (watchdog_active(wdt_dev))
>> +meson_gxbb_wdt_stop(wdt_dev);
>> +
>> +meson_gxbb_wdt_ping(wdt_dev);
>> +
>> +writel(timeout*1000, data->reg_base + GXBB_WDT_TCNT_REG);
> 
> nit: spaces around "*"
> 
> [...]
>> +data->clk = devm_clk_get(>dev, NULL);
>> +if (IS_ERR(data->clk))
>> +return PTR_ERR(data->clk);
>> +
>> +clk_prepare_enable(data->clk);
> 
> Do we need to merge the clock controller driver before this?

It's not necessary, currently it only selects the xtal source, so it's works 
with the current upstream architecture.

Neil

> 
> Cheers,
> 

Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Guenter Roeck]

On 05/26/2016 12:51 AM, Neil Armstrong wrote:

Add watchdog specific driver for Amlogic Meson GXBB SoC.



Wondering - why RFC ?


Signed-off-by: Neil Armstrong 
---
  drivers/watchdog/Makefile |   1 +
  drivers/watchdog/meson_gxbb_wdt.c | 287 ++
  2 files changed, 288 insertions(+)
  create mode 100644 drivers/watchdog/meson_gxbb_wdt.c

diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 9bde095..7679d93 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o
  obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
  obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
  obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
+obj-$(CONFIG_MESON_WATCHDOG) += meson_gxbb_wdt.o
  obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
  obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
  obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
diff --git a/drivers/watchdog/meson_gxbb_wdt.c 
b/drivers/watchdog/meson_gxbb_wdt.c
new file mode 100644
index 000..0c7c0d9
--- /dev/null
+++ b/drivers/watchdog/meson_gxbb_wdt.c
@@ -0,0 +1,287 @@
+/*
+ * This file is provided under a dual BSD/GPLv2 license.  When using or
+ * redistributing this file, you may do so under either license.
+ *
+ * GPL LICENSE SUMMARY
+ *
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see .
+ * The full GNU General Public License is included in this distribution
+ * in the file called COPYING.
+ *
+ * BSD LICENSE
+ *
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong 
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ *   * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *   * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in
+ * the documentation and/or other materials provided with the
+ * distribution.
+ *   * Neither the name of Intel Corporation nor the names of its
+ * contributors may be used to endorse or promote products derived
+ * from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DEFAULT_TIMEOUT10  /* seconds */
+
+#define GXBB_WDT_CTRL_REG  0x0
+#define GXBB_WDT_CTRL1_REG 0x4
+#define GXBB_WDT_TCNT_REG  0x8
+#define GXBB_WDT_RSET_REG  0xc
+
+#define GXBB_WDT_CTRL_EE_RESET_NOW BIT(26)
+
+#define GXBB_WDT_CTRL_CLKDIV_ENBIT(25)
+#define GXBB_WDT_CTRL_CLK_EN   BIT(24)
+#define GXBB_WDT_CTRL_IRQ_EN   BIT(23)
+#define GXBB_WDT_CTRL_EE_RESET BIT(21)
+#define GXBB_WDT_CTRL_XTAL_SEL (0)
+#define GXBB_WDT_CTRL_CLK81_SELBIT(19)
+#define GXBB_WDT_CTRL_EN   BIT(18)
+#define GXBB_WDT_CTRL_DIV_MASK (BIT(18)-1)
+
+#define GXBB_WDT_CTRL1_GPIO_PULSE  BIT(17)
+#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0BIT(16)
+#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1(0)
+#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT  (BIT(16)-1)
+


Spaces around operators, please.


+#define GXBB_WDT_TCNT_SETUP_MASK   (BIT(16)-1)
+#define GXBB_WDT_TCNT_CNT_SHIFT16
+
+struct meson_gxbb_wdt {
+   void __iomem *reg_base;
+   struct watchdog_device wdt_dev;
+   struct clk *clk;
+};
+
+int 

Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Guenter Roeck]

On 05/26/2016 12:51 AM, Neil Armstrong wrote:

Add watchdog specific driver for Amlogic Meson GXBB SoC.



Wondering - why RFC ?


Signed-off-by: Neil Armstrong 
---
  drivers/watchdog/Makefile |   1 +
  drivers/watchdog/meson_gxbb_wdt.c | 287 ++
  2 files changed, 288 insertions(+)
  create mode 100644 drivers/watchdog/meson_gxbb_wdt.c

diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 9bde095..7679d93 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o
  obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
  obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
  obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
+obj-$(CONFIG_MESON_WATCHDOG) += meson_gxbb_wdt.o
  obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
  obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
  obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
diff --git a/drivers/watchdog/meson_gxbb_wdt.c 
b/drivers/watchdog/meson_gxbb_wdt.c
new file mode 100644
index 000..0c7c0d9
--- /dev/null
+++ b/drivers/watchdog/meson_gxbb_wdt.c
@@ -0,0 +1,287 @@
+/*
+ * This file is provided under a dual BSD/GPLv2 license.  When using or
+ * redistributing this file, you may do so under either license.
+ *
+ * GPL LICENSE SUMMARY
+ *
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see .
+ * The full GNU General Public License is included in this distribution
+ * in the file called COPYING.
+ *
+ * BSD LICENSE
+ *
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong 
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ *   * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *   * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in
+ * the documentation and/or other materials provided with the
+ * distribution.
+ *   * Neither the name of Intel Corporation nor the names of its
+ * contributors may be used to endorse or promote products derived
+ * from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DEFAULT_TIMEOUT10  /* seconds */
+
+#define GXBB_WDT_CTRL_REG  0x0
+#define GXBB_WDT_CTRL1_REG 0x4
+#define GXBB_WDT_TCNT_REG  0x8
+#define GXBB_WDT_RSET_REG  0xc
+
+#define GXBB_WDT_CTRL_EE_RESET_NOW BIT(26)
+
+#define GXBB_WDT_CTRL_CLKDIV_ENBIT(25)
+#define GXBB_WDT_CTRL_CLK_EN   BIT(24)
+#define GXBB_WDT_CTRL_IRQ_EN   BIT(23)
+#define GXBB_WDT_CTRL_EE_RESET BIT(21)
+#define GXBB_WDT_CTRL_XTAL_SEL (0)
+#define GXBB_WDT_CTRL_CLK81_SELBIT(19)
+#define GXBB_WDT_CTRL_EN   BIT(18)
+#define GXBB_WDT_CTRL_DIV_MASK (BIT(18)-1)
+
+#define GXBB_WDT_CTRL1_GPIO_PULSE  BIT(17)
+#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0BIT(16)
+#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1(0)
+#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT  (BIT(16)-1)
+


Spaces around operators, please.


+#define GXBB_WDT_TCNT_SETUP_MASK   (BIT(16)-1)
+#define GXBB_WDT_TCNT_CNT_SHIFT16
+
+struct meson_gxbb_wdt {
+   void __iomem *reg_base;
+   struct watchdog_device wdt_dev;
+   struct clk *clk;
+};
+
+int meson_gxbb_wdt_start(struct watchdog_device *wdt_dev)


Functions should all be static.

Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Carlo Caione]

On 26/05/16 09:51, Neil Armstrong wrote:
> Add watchdog specific driver for Amlogic Meson GXBB SoC.
> 
> Signed-off-by: Neil Armstrong 
> +

[...]
> +#define DEFAULT_TIMEOUT  10  /* seconds */
> +
> +#define GXBB_WDT_CTRL_REG0x0
> +#define GXBB_WDT_CTRL1_REG   0x4
> +#define GXBB_WDT_TCNT_REG0x8
> +#define GXBB_WDT_RSET_REG0xc
> +
> +#define GXBB_WDT_CTRL_EE_RESET_NOW   BIT(26)
> +
> +#define GXBB_WDT_CTRL_CLKDIV_EN  BIT(25)
> +#define GXBB_WDT_CTRL_CLK_EN BIT(24)
> +#define GXBB_WDT_CTRL_IRQ_EN BIT(23)
> +#define GXBB_WDT_CTRL_EE_RESET   BIT(21)
> +#define GXBB_WDT_CTRL_XTAL_SEL   (0)
> +#define GXBB_WDT_CTRL_CLK81_SEL  BIT(19)
> +#define GXBB_WDT_CTRL_EN BIT(18)
> +#define GXBB_WDT_CTRL_DIV_MASK   (BIT(18)-1)
> +
> +#define GXBB_WDT_CTRL1_GPIO_PULSEBIT(17)
> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0  BIT(16)
> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1  (0)
> +#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT(BIT(16)-1)
> +
> +#define GXBB_WDT_TCNT_SETUP_MASK (BIT(16)-1)
> +#define GXBB_WDT_TCNT_CNT_SHIFT  16

Indentation

[...]
> +int meson_gxbb_wdt_set_timeout(struct watchdog_device *wdt_dev,
> +unsigned int timeout)
> +{
> + struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
> +
> + if (watchdog_active(wdt_dev))
> + meson_gxbb_wdt_stop(wdt_dev);
> +
> + meson_gxbb_wdt_ping(wdt_dev);
> +
> + writel(timeout*1000, data->reg_base + GXBB_WDT_TCNT_REG);

nit: spaces around "*"

[...]
> + data->clk = devm_clk_get(>dev, NULL);
> + if (IS_ERR(data->clk))
> + return PTR_ERR(data->clk);
> +
> + clk_prepare_enable(data->clk);

Do we need to merge the clock controller driver before this?

Cheers,

-- 
Carlo Caione

Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Carlo Caione]

On 26/05/16 09:51, Neil Armstrong wrote:
> Add watchdog specific driver for Amlogic Meson GXBB SoC.
> 
> Signed-off-by: Neil Armstrong 
> +

[...]
> +#define DEFAULT_TIMEOUT  10  /* seconds */
> +
> +#define GXBB_WDT_CTRL_REG0x0
> +#define GXBB_WDT_CTRL1_REG   0x4
> +#define GXBB_WDT_TCNT_REG0x8
> +#define GXBB_WDT_RSET_REG0xc
> +
> +#define GXBB_WDT_CTRL_EE_RESET_NOW   BIT(26)
> +
> +#define GXBB_WDT_CTRL_CLKDIV_EN  BIT(25)
> +#define GXBB_WDT_CTRL_CLK_EN BIT(24)
> +#define GXBB_WDT_CTRL_IRQ_EN BIT(23)
> +#define GXBB_WDT_CTRL_EE_RESET   BIT(21)
> +#define GXBB_WDT_CTRL_XTAL_SEL   (0)
> +#define GXBB_WDT_CTRL_CLK81_SEL  BIT(19)
> +#define GXBB_WDT_CTRL_EN BIT(18)
> +#define GXBB_WDT_CTRL_DIV_MASK   (BIT(18)-1)
> +
> +#define GXBB_WDT_CTRL1_GPIO_PULSEBIT(17)
> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0  BIT(16)
> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1  (0)
> +#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT(BIT(16)-1)
> +
> +#define GXBB_WDT_TCNT_SETUP_MASK (BIT(16)-1)
> +#define GXBB_WDT_TCNT_CNT_SHIFT  16

Indentation

[...]
> +int meson_gxbb_wdt_set_timeout(struct watchdog_device *wdt_dev,
> +unsigned int timeout)
> +{
> + struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
> +
> + if (watchdog_active(wdt_dev))
> + meson_gxbb_wdt_stop(wdt_dev);
> +
> + meson_gxbb_wdt_ping(wdt_dev);
> +
> + writel(timeout*1000, data->reg_base + GXBB_WDT_TCNT_REG);

nit: spaces around "*"

[...]
> + data->clk = devm_clk_get(>dev, NULL);
> + if (IS_ERR(data->clk))
> + return PTR_ERR(data->clk);
> +
> + clk_prepare_enable(data->clk);

Do we need to merge the clock controller driver before this?

Cheers,

-- 
Carlo Caione

[RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Neil Armstrong]

Add watchdog specific driver for Amlogic Meson GXBB SoC.

Signed-off-by: Neil Armstrong 
---
 drivers/watchdog/Makefile |   1 +
 drivers/watchdog/meson_gxbb_wdt.c | 287 ++
 2 files changed, 288 insertions(+)
 create mode 100644 drivers/watchdog/meson_gxbb_wdt.c

diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 9bde095..7679d93 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o
 obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
 obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
 obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
+obj-$(CONFIG_MESON_WATCHDOG) += meson_gxbb_wdt.o
 obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
 obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
 obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
diff --git a/drivers/watchdog/meson_gxbb_wdt.c 
b/drivers/watchdog/meson_gxbb_wdt.c
new file mode 100644
index 000..0c7c0d9
--- /dev/null
+++ b/drivers/watchdog/meson_gxbb_wdt.c
@@ -0,0 +1,287 @@
+/*
+ * This file is provided under a dual BSD/GPLv2 license.  When using or
+ * redistributing this file, you may do so under either license.
+ *
+ * GPL LICENSE SUMMARY
+ *
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see .
+ * The full GNU General Public License is included in this distribution
+ * in the file called COPYING.
+ *
+ * BSD LICENSE
+ *
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong 
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ *   * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *   * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in
+ * the documentation and/or other materials provided with the
+ * distribution.
+ *   * Neither the name of Intel Corporation nor the names of its
+ * contributors may be used to endorse or promote products derived
+ * from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DEFAULT_TIMEOUT10  /* seconds */
+
+#define GXBB_WDT_CTRL_REG  0x0
+#define GXBB_WDT_CTRL1_REG 0x4
+#define GXBB_WDT_TCNT_REG  0x8
+#define GXBB_WDT_RSET_REG  0xc
+
+#define GXBB_WDT_CTRL_EE_RESET_NOW BIT(26)
+
+#define GXBB_WDT_CTRL_CLKDIV_ENBIT(25)
+#define GXBB_WDT_CTRL_CLK_EN   BIT(24)
+#define GXBB_WDT_CTRL_IRQ_EN   BIT(23)
+#define GXBB_WDT_CTRL_EE_RESET BIT(21)
+#define GXBB_WDT_CTRL_XTAL_SEL (0)
+#define GXBB_WDT_CTRL_CLK81_SELBIT(19)
+#define GXBB_WDT_CTRL_EN   BIT(18)
+#define GXBB_WDT_CTRL_DIV_MASK (BIT(18)-1)
+
+#define GXBB_WDT_CTRL1_GPIO_PULSE  BIT(17)
+#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0BIT(16)
+#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1(0)
+#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT  (BIT(16)-1)
+
+#define GXBB_WDT_TCNT_SETUP_MASK   (BIT(16)-1)
+#define GXBB_WDT_TCNT_CNT_SHIFT16
+
+struct meson_gxbb_wdt {
+   void __iomem *reg_base;
+   struct watchdog_device wdt_dev;
+   struct clk *clk;
+};
+
+int meson_gxbb_wdt_start(struct watchdog_device *wdt_dev)
+{
+   struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
+
+   

[RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

[Neil Armstrong]

Add watchdog specific driver for Amlogic Meson GXBB SoC.

Signed-off-by: Neil Armstrong 
---
 drivers/watchdog/Makefile |   1 +
 drivers/watchdog/meson_gxbb_wdt.c | 287 ++
 2 files changed, 288 insertions(+)
 create mode 100644 drivers/watchdog/meson_gxbb_wdt.c

diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 9bde095..7679d93 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o
 obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
 obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
 obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
+obj-$(CONFIG_MESON_WATCHDOG) += meson_gxbb_wdt.o
 obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
 obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
 obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
diff --git a/drivers/watchdog/meson_gxbb_wdt.c 
b/drivers/watchdog/meson_gxbb_wdt.c
new file mode 100644
index 000..0c7c0d9
--- /dev/null
+++ b/drivers/watchdog/meson_gxbb_wdt.c
@@ -0,0 +1,287 @@
+/*
+ * This file is provided under a dual BSD/GPLv2 license.  When using or
+ * redistributing this file, you may do so under either license.
+ *
+ * GPL LICENSE SUMMARY
+ *
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see .
+ * The full GNU General Public License is included in this distribution
+ * in the file called COPYING.
+ *
+ * BSD LICENSE
+ *
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong 
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ *   * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *   * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in
+ * the documentation and/or other materials provided with the
+ * distribution.
+ *   * Neither the name of Intel Corporation nor the names of its
+ * contributors may be used to endorse or promote products derived
+ * from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DEFAULT_TIMEOUT10  /* seconds */
+
+#define GXBB_WDT_CTRL_REG  0x0
+#define GXBB_WDT_CTRL1_REG 0x4
+#define GXBB_WDT_TCNT_REG  0x8
+#define GXBB_WDT_RSET_REG  0xc
+
+#define GXBB_WDT_CTRL_EE_RESET_NOW BIT(26)
+
+#define GXBB_WDT_CTRL_CLKDIV_ENBIT(25)
+#define GXBB_WDT_CTRL_CLK_EN   BIT(24)
+#define GXBB_WDT_CTRL_IRQ_EN   BIT(23)
+#define GXBB_WDT_CTRL_EE_RESET BIT(21)
+#define GXBB_WDT_CTRL_XTAL_SEL (0)
+#define GXBB_WDT_CTRL_CLK81_SELBIT(19)
+#define GXBB_WDT_CTRL_EN   BIT(18)
+#define GXBB_WDT_CTRL_DIV_MASK (BIT(18)-1)
+
+#define GXBB_WDT_CTRL1_GPIO_PULSE  BIT(17)
+#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0BIT(16)
+#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1(0)
+#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT  (BIT(16)-1)
+
+#define GXBB_WDT_TCNT_SETUP_MASK   (BIT(16)-1)
+#define GXBB_WDT_TCNT_CNT_SHIFT16
+
+struct meson_gxbb_wdt {
+   void __iomem *reg_base;
+   struct watchdog_device wdt_dev;
+   struct clk *clk;
+};
+
+int meson_gxbb_wdt_start(struct watchdog_device *wdt_dev)
+{
+   struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
+
+   writel(readl(data->reg_base + GXBB_WDT_CTRL_REG) | GXBB_WDT_CTRL_EN,
+