Re: [PATCH v2 1/8] i2c-mux: add common core data for every mux instance
On 01/05/2016 07:57 AM, Peter Rosin wrote:
From: Peter Rosin
The initial core mux structure starts off small with only the parent
adapter pointer, which all muxes have, and a priv pointer for mux
driver private data.
Add i2c_mux_alloc function to unify the creation of a mux.
Where appropriate, pass around the mux core structure instead of the
parent adapter or the driver private data.
Remove the parent adapter pointer from the driver private data for all
mux drivers.
Signed-off-by: Peter Rosin
---
drivers/i2c/i2c-mux.c| 28 +-
drivers/i2c/muxes/i2c-arb-gpio-challenge.c | 24 +--
drivers/i2c/muxes/i2c-mux-gpio.c | 20
drivers/i2c/muxes/i2c-mux-pca9541.c | 35 ++--
drivers/i2c/muxes/i2c-mux-pca954x.c | 19 ++-
drivers/i2c/muxes/i2c-mux-pinctrl.c | 23 +-
drivers/i2c/muxes/i2c-mux-reg.c | 23 ++
drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 10 +++-
drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h| 1 +
drivers/media/dvb-frontends/m88ds3103.c | 10 +++-
drivers/media/dvb-frontends/m88ds3103_priv.h | 1 +
drivers/media/dvb-frontends/rtl2830.c| 10 +++-
drivers/media/dvb-frontends/rtl2830_priv.h | 1 +
drivers/media/dvb-frontends/rtl2832.c| 10 +++-
drivers/media/dvb-frontends/rtl2832_priv.h | 1 +
drivers/media/dvb-frontends/si2168.c | 10 +++-
drivers/media/dvb-frontends/si2168_priv.h| 1 +
drivers/media/usb/cx231xx/cx231xx-core.c | 3 +++
drivers/media/usb/cx231xx/cx231xx-i2c.c | 13 +--
drivers/media/usb/cx231xx/cx231xx.h | 2 ++
drivers/of/unittest.c| 16 +++--
include/linux/i2c-mux.h | 14 ++-
22 files changed, 187 insertions(+), 88 deletions(-)
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 00fc5b1c7b66..c2163f6b51d5 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -31,8 +31,8 @@
struct i2c_mux_priv {
struct i2c_adapter adap;
struct i2c_algorithm algo;
+ struct i2c_mux_core *muxc;
- struct i2c_adapter *parent;
struct device *mux_dev;
void *mux_priv;
u32 chan_id;
@@ -45,7 +45,8 @@ static int i2c_mux_master_xfer(struct i2c_adapter *adap,
struct i2c_msg msgs[], int num)
{
struct i2c_mux_priv *priv = adap->algo_data;
- struct i2c_adapter *parent = priv->parent;
+ struct i2c_mux_core *muxc = priv->muxc;
+ struct i2c_adapter *parent = muxc->parent;
int ret;
/* Switch to the right mux port and perform the transfer. */
@@ -65,7 +66,8 @@ static int i2c_mux_smbus_xfer(struct i2c_adapter *adap,
int size, union i2c_smbus_data *data)
{
struct i2c_mux_priv *priv = adap->algo_data;
- struct i2c_adapter *parent = priv->parent;
+ struct i2c_mux_core *muxc = priv->muxc;
+ struct i2c_adapter *parent = muxc->parent;
int ret;
/* Select the right mux port and perform the transfer. */
@@ -84,7 +86,7 @@ static int i2c_mux_smbus_xfer(struct i2c_adapter *adap,
static u32 i2c_mux_functionality(struct i2c_adapter *adap)
{
struct i2c_mux_priv *priv = adap->algo_data;
- struct i2c_adapter *parent = priv->parent;
+ struct i2c_adapter *parent = priv->muxc->parent;
return parent->algo->functionality(parent);
}
@@ -102,7 +104,20 @@ static unsigned int i2c_mux_parent_classes(struct
i2c_adapter *parent)
return class;
}
-struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
+struct i2c_mux_core *i2c_mux_alloc(struct device *dev, int sizeof_priv)
+{
+ struct i2c_mux_core *muxc;
+
+ muxc = devm_kzalloc(dev, sizeof(*muxc) + sizeof_priv, GFP_KERNEL);
+ if (!muxc)
+ return NULL;
+ if (sizeof_priv)
+ muxc->priv = muxc + 1;
+ return muxc;
+}
+EXPORT_SYMBOL_GPL(i2c_mux_alloc);
+
+struct i2c_adapter *i2c_add_mux_adapter(struct i2c_mux_core *muxc,
struct device *mux_dev,
void *mux_priv, u32 force_nr, u32 chan_id,
unsigned int class,
@@ -111,6 +126,7 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter
*parent,
int (*deselect) (struct i2c_adapter *,
void *, u32))
{
+ struct i2c_adapter *parent = muxc->parent;
struct i2c_mux_priv *priv;
char symlink_name[20];
int ret;
@@ -120,7 +136,7 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter
*parent,
return NULL;
/* Set up private adapter data */
- priv->parent = parent;
+ priv->muxc = muxc;
priv->mux_dev = mux
Re: [PATCH 01/10] i2c-mux: add common core data for every mux instance
On 01/04/2016 07:10 AM, Peter Rosin wrote:
From: Peter Rosin
The initial core mux structure starts off small with only the parent
adapter pointer, which all muxes have, and a priv pointer for mux
driver private data.
Add i2c_mux_alloc function to unify the creation of a mux.
Where appropriate, pass around the mux core structure instead of the
parent adapter or the driver private data.
Remove the parent adapter pointer from the driver private data for all
mux drivers.
Signed-off-by: Peter Rosin
---
drivers/i2c/i2c-mux.c | 35 -
drivers/i2c/muxes/i2c-arb-gpio-challenge.c | 24 +++-
drivers/i2c/muxes/i2c-mux-gpio.c | 20 +
drivers/i2c/muxes/i2c-mux-pca9541.c| 36 --
drivers/i2c/muxes/i2c-mux-pca954x.c| 22 +-
drivers/i2c/muxes/i2c-mux-pinctrl.c| 24 +++-
drivers/i2c/muxes/i2c-mux-reg.c| 25 -
include/linux/i2c-mux.h| 14 +++-
8 files changed, 129 insertions(+), 71 deletions(-)
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 00fc5b1c7b66..99fd9106abc6 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -31,8 +31,8 @@
struct i2c_mux_priv {
struct i2c_adapter adap;
struct i2c_algorithm algo;
+ struct i2c_mux_core *muxc;
- struct i2c_adapter *parent;
struct device *mux_dev;
void *mux_priv;
u32 chan_id;
@@ -45,7 +45,8 @@ static int i2c_mux_master_xfer(struct i2c_adapter *adap,
struct i2c_msg msgs[], int num)
{
struct i2c_mux_priv *priv = adap->algo_data;
- struct i2c_adapter *parent = priv->parent;
+ struct i2c_mux_core *muxc = priv->muxc;
+ struct i2c_adapter *parent = muxc->parent;
int ret;
/* Switch to the right mux port and perform the transfer. */
@@ -65,7 +66,8 @@ static int i2c_mux_smbus_xfer(struct i2c_adapter *adap,
int size, union i2c_smbus_data *data)
{
struct i2c_mux_priv *priv = adap->algo_data;
- struct i2c_adapter *parent = priv->parent;
+ struct i2c_mux_core *muxc = priv->muxc;
+ struct i2c_adapter *parent = muxc->parent;
int ret;
/* Select the right mux port and perform the transfer. */
@@ -84,7 +86,7 @@ static int i2c_mux_smbus_xfer(struct i2c_adapter *adap,
static u32 i2c_mux_functionality(struct i2c_adapter *adap)
{
struct i2c_mux_priv *priv = adap->algo_data;
- struct i2c_adapter *parent = priv->parent;
+ struct i2c_adapter *parent = priv->muxc->parent;
return parent->algo->functionality(parent);
}
@@ -102,7 +104,27 @@ static unsigned int i2c_mux_parent_classes(struct
i2c_adapter *parent)
return class;
}
-struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
+struct i2c_mux_core *i2c_mux_alloc(struct device *dev, int sizeof_priv)
+{
+ struct i2c_mux_core *muxc;
+
+ muxc = devm_kzalloc(dev, sizeof(*muxc), GFP_KERNEL);
+ if (!muxc)
+ return NULL;
+ if (sizeof_priv) {
+ muxc->priv = devm_kzalloc(dev, sizeof_priv, GFP_KERNEL);
+ if (!muxc->priv)
+ goto fail;
+ }
Why not just allocate sizeof(*muxc) + sizeof_priv in a single operation
and then assign muxc->priv to muxc + 1 if sizeof_priv > 0 ?
+ return muxc;
+
+fail:
+ devm_kfree(dev, muxc);
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(i2c_mux_alloc);
+
+struct i2c_adapter *i2c_add_mux_adapter(struct i2c_mux_core *muxc,
struct device *mux_dev,
void *mux_priv, u32 force_nr, u32 chan_id,
unsigned int class,
@@ -111,6 +133,7 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter
*parent,
int (*deselect) (struct i2c_adapter *,
void *, u32))
{
+ struct i2c_adapter *parent = muxc->parent;
struct i2c_mux_priv *priv;
char symlink_name[20];
int ret;
@@ -120,7 +143,7 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter
*parent,
return NULL;
/* Set up private adapter data */
- priv->parent = parent;
+ priv->muxc = muxc;
priv->mux_dev = mux_dev;
priv->mux_priv = mux_priv;
priv->chan_id = chan_id;
diff --git a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
index 402e3a6c671a..dd616c0280ad 100644
--- a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
+++ b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
@@ -42,7 +42,6 @@
*/
struct i2c_arbitrator_data {
- struct i2c_adapter *parent;
struct i2c_adapter *child;
int our_gpio;
int our_gpio_release;
@@ -119,6 +118,7 @@ sta
Re: [PATCH v8] watchdog: ts4800: add driver for TS-4800 watchdog
On 12/23/2015 07:43 AM, Damien Riegel wrote: On Tue, Dec 08, 2015 at 11:37:28AM -0500, Damien Riegel wrote: This watchdog is instantiated in a FPGA that is memory mapped. It is made of only one register, called the feed register. Writing to this register will re-arm the watchdog for a given time (and enable it if it was disable). It can be disabled by writing a special value into it. It is part of a syscon block, and the watchdog register offset in this block varies from board to board. This offset is passed in the syscon property after the phandle to the syscon node. Signed-off-by: Damien Riegel Acked-by: Rob Herring Reviewed-by: Guenter Roeck Hi Guenter, You have reviewed this patch but not picked it up in your tree. Shall I expect Wim to pick it up directly for the next merge window? The board would be quite useless without its watchdog driver. My tree is in pretty bad shape right now; It includes some older patches which have to be replaced. I'll have to rebase it against Wim's tree and clean it up. Hope I can do that before the weekend. But, yes, of course, Wim can pick up your patch directly. Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop
On 12/17/2015 07:02 AM, Tim Harvey wrote: On Wed, Dec 2, 2015 at 12:54 PM, Tim Harvey wrote: On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat wrote: On 11/06/2015 05:02 PM, Guenter Roeck wrote: On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote: On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck wrote: On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote: From: Tim Harvey The IMX6 watchdog supports assertion of a signal (WDOG_B) which can be pinmux'd to an external pin. This is typically used for boards that have PMIC's in control of the IMX6 power rails. In fact, failure to use such an external reset on boards with external PMIC's can result in various hangs due to the IMX6 not being fully reset [1] as well as the board failing to reset because its PMIC has not been reset to provide adequate voltage for the CPU when coming out of reset at 800Mhz. This uses a new device-tree property 'ext-reset-output' to indicate the board has such a reset and to cause the watchdog to be configured to assert WDOG_B instead of an internal reset both on a watchdog timeout and in system_restart. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html Cc: Lucas Stach Signed-off-by: Tim Harvey --- .../devicetree/bindings/watchdog/fsl-imx-wdt.txt | 2 ++ drivers/watchdog/imx2_wdt.c | 20 ++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt index 8dab6fd..9b89b3a 100644 --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt @@ -9,6 +9,8 @@ Optional property: properties ? - big-endian: If present the watchdog device's registers are implemented in big endian mode, otherwise in native mode(same with CPU), for more detail please see: Documentation/devicetree/bindings/regmap/regmap.txt. +- ext-reset-output: If present the watchdog device is configured to assert its Should that have a vendor prefix ? Also, not sure if "-output" has any real value in the property name. "fsl,external-reset", maybe ? Hi Guenter, I don't see why a vendor prefix is necessary - its a feature of the IMX6 watchdog supported by this driver to be able to trigger an internal chip-level reset and/or an external signal that can be hooked to additional hardware. Sounded like vendor specific to me, but then I am not a devicetree maintainer, so I am not an authority on the subject. Devicetree maintainers, Any thoughts? Tim, After looking at all the other watchdog drivers, it does not appear that there is any other processor which uses a similar feature. Since imx is the only processor that appears to support this feature, it might make sense in making this vendor specific. If in the future it is found more processors support a similar functionality, it can be revisited and moved out from being vendor specific? I'm certainly no expert on device-tree policy. I understand your point, but realize that the driver in question is imx2_wdt.c (compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon of only Freescale chips, so its not like a future omap chip would be using this driver - only fsl devices. So why would it need a 'vendor' property any more than its other properties? Regards, Tim Wim, Does the lack of response mean overwhelming approval? I haven't heard any valid complaints - what does it take to get this approved? Tim, Do you have an Ack from a devicetree maintainer ? I don't recall seeing one. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH -next] of: Move of_io_request_and_map prototype to correct context
of_io_request_and_map() is defined in of/address.c, which is compiled
if CONFIG_OF_ADDRESS is configured. However, it is defined in the context
of CONFIG_OF. This results in the following build errors for
sparc:allmodconfig (which defines CONFIG_OF but not CONFIG_OF_ADDRESS).
drivers/built-in.o: In function `meson6_timer_init':
meson6_timer.c:(.init.text+0x62a8):
undefined reference to `of_io_request_and_map'
drivers/built-in.o: In function `mtk_timer_init':
mtk_timer.c:(.init.text+0x6e70):
undefined reference to `of_io_request_and_map'
drivers/built-in.o: In function `asm9260_timer_init':
asm9260_timer.c:(.init.text+0x6fcc):
undefined reference to `of_io_request_and_map'
Move function prototype and dummy function into the CONFIG_OF_ADDRESS
conditional to fix the problem.
Exposed by commit bec8c4617611 ("clocksource/drivers/mediatek: Add the
COMPILE_TEST option").
Cc: Daniel Lezcano
Signed-off-by: Guenter Roeck
---
include/linux/of_address.h | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 507daad0bc8d..b5324c0553bd 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -56,6 +56,8 @@ extern struct of_pci_range *of_pci_range_parser_one(
extern int of_dma_get_range(struct device_node *np, u64 *dma_addr,
u64 *paddr, u64 *size);
extern bool of_dma_is_coherent(struct device_node *np);
+void __iomem *of_io_request_and_map(struct device_node *device,
+ int index, const char *name);
#else /* CONFIG_OF_ADDRESS */
static inline u64 of_translate_address(struct device_node *np,
@@ -106,18 +108,22 @@ static inline bool of_dma_is_coherent(struct device_node
*np)
{
return false;
}
+
+#include
+
+static inline void __iomem *of_io_request_and_map(struct device_node *device,
+ int index, const char *name)
+{
+ return IOMEM_ERR_PTR(-EINVAL);
+}
#endif /* CONFIG_OF_ADDRESS */
#ifdef CONFIG_OF
extern int of_address_to_resource(struct device_node *dev, int index,
struct resource *r);
void __iomem *of_iomap(struct device_node *node, int index);
-void __iomem *of_io_request_and_map(struct device_node *device,
- int index, const char *name);
#else
-#include
-
static inline int of_address_to_resource(struct device_node *dev, int index,
struct resource *r)
{
@@ -129,11 +135,6 @@ static inline void __iomem *of_iomap(struct device_node
*device, int index)
return NULL;
}
-static inline void __iomem *of_io_request_and_map(struct device_node *device,
- int index, const char *name)
-{
- return IOMEM_ERR_PTR(-EINVAL);
-}
#endif
#if defined(CONFIG_OF_ADDRESS) && defined(CONFIG_PCI)
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 45/50] drivers/of: Avoid recursively calling unflatten_dt_node()
On 12/06/2015 06:33 PM, Rob Herring wrote: On Sun, Dec 6, 2015 at 8:21 PM, Guenter Roeck wrote: On 12/06/2015 03:54 PM, Benjamin Herrenschmidt wrote: On Sun, 2015-12-06 at 14:28 -0600, Rob Herring wrote: Do you plan to respin the OF parts at least soon? There's another problem Guenter found that of_fdt_unflatten_tree is not re-entrant due to "depth" being static and this series fixes that. So I'd rather apply this and avoid adding a mutex if possible. Gavin is on vacation until next year. That is a bit more than the timeline I am looking for. Rob, any chance to accept my patch for now ? After all, it can be reverted after the rework is complete, and it would be easier to apply to earlier kernels. Yes, will do. It's only 4.1 and later that it should be marked for stable? Yes, I think so. Earlier kernels would need a manual backport. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 45/50] drivers/of: Avoid recursively calling unflatten_dt_node()
On 12/06/2015 03:54 PM, Benjamin Herrenschmidt wrote: On Sun, 2015-12-06 at 14:28 -0600, Rob Herring wrote: Do you plan to respin the OF parts at least soon? There's another problem Guenter found that of_fdt_unflatten_tree is not re-entrant due to "depth" being static and this series fixes that. So I'd rather apply this and avoid adding a mutex if possible. Gavin is on vacation until next year. That is a bit more than the timeline I am looking for. Rob, any chance to accept my patch for now ? After all, it can be reverted after the rework is complete, and it would be easier to apply to earlier kernels. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 45/50] drivers/of: Avoid recursively calling unflatten_dt_node()
On 12/06/2015 12:28 PM, Rob Herring wrote: +Guenter On Wed, Nov 4, 2015 at 7:12 AM, Gavin Shan wrote: In current implementation, unflatten_dt_node() is called recursively to unflatten device nodes in FDT blob. It's stress to limited stack capacity. This avoids calling the function recursively, meaning the device nodes are unflattened in one call on unflatten_dt_node(): two arrays are introduced to track the parent path size and the device node of current level of depth, which will be used by the device node on next level of depth to be unflattened. Also, the parameter "poffset" and "fpsize" are unused and dropped. Do you plan to respin the OF parts at least soon? There's another problem Guenter found that of_fdt_unflatten_tree is not re-entrant due to "depth" being static and this series fixes that. So I'd rather apply this and avoid adding a mutex if possible. Hi Rob, We see this problem in 4.1, so whatever patch you accept should be back-ported to at least that release. Any idea when this patch will be accepted ? We actively see the problem in our kernel, so I'll need a solution soon. Otherwise I'll have to apply my patch to our kernel and revert it as soon as the 'real' patch has been back-ported. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] of/fdt: Add mutex protection for calls to __unflatten_device_tree()
__unflatten_device_tree() calls unflatten_dt_node(), which declares
a static variable. It is therefore not reentrant.
One of the callers of __unflatten_device_tree(), unflatten_device_tree(),
is only called once during early initialization and does not need to be
protected. The other caller, of_fdt_unflatten_tree(), can be called at
any time, possibly multiple times in parallel. This can happen, for
example, if multiple devicetree overlays have to be loaded and installed.
Without this protection, errors such as the following may be seen.
kernel: End of tree marker overwritten: e6a3a458
kernel: find_target_node:
Failed to find target-indirect node at /fragment@0
kernel: __of_overlay_create: of_build_overlay_info() failed for tree@/
Add a mutex to of_fdt_unflatten_tree() to make the call reentrant.
Cc: Pantelis Antoniou
Signed-off-by: Guenter Roeck
---
drivers/of/fdt.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index d2430298a309..f13565d8b964 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -436,6 +437,8 @@ static void *kernel_tree_alloc(u64 size, u64 align)
return kzalloc(size, GFP_KERNEL);
}
+static DEFINE_MUTEX(of_fdt_unflatten_mutex);
+
/**
* of_fdt_unflatten_tree - create tree of device_nodes from flat blob
*
@@ -447,7 +450,9 @@ static void *kernel_tree_alloc(u64 size, u64 align)
void of_fdt_unflatten_tree(const unsigned long *blob,
struct device_node **mynodes)
{
+ mutex_lock(&of_fdt_unflatten_mutex);
__unflatten_device_tree(blob, mynodes, &kernel_tree_alloc);
+ mutex_unlock(&of_fdt_unflatten_mutex);
}
EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [v3,1/2] Add binding documentation for Zodiac Watchdog Timer
On Tue, Dec 01, 2015 at 03:38:38PM +, Martyn Welch wrote: > > > On 01/12/15 15:15, Guenter Roeck wrote: > >On Wed, Nov 25, 2015 at 12:03:34PM +, Martyn Welch wrote: > >>This patchs adds documentation for the binding of the Zodiac RAVE > >>Switch Watchdog Processor. This is an i2c based watchdog. > >> > >>Cc: Rob Herring > >>Cc: Pawel Moll > >>Cc: Mark Rutland > >>Cc: Ian Campbell > >>Cc: Kumar Gala > >>Cc: devicetree@vger.kernel.org > >>Signed-off-by: Martyn Welch > >>Acked-by: Rob Herring > > > >Reviewed-by: Guenter Roeck > > > > Would I be right in thinking this will be going via the watchdog tree as > well? > I would think so. I'll queue it in my tree and send it to Wim in my next pull request. Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [v3,1/2] Add binding documentation for Zodiac Watchdog Timer
On Wed, Nov 25, 2015 at 12:03:34PM +, Martyn Welch wrote: > This patchs adds documentation for the binding of the Zodiac RAVE > Switch Watchdog Processor. This is an i2c based watchdog. > > Cc: Rob Herring > Cc: Pawel Moll > Cc: Mark Rutland > Cc: Ian Campbell > Cc: Kumar Gala > Cc: devicetree@vger.kernel.org > Signed-off-by: Martyn Welch > Acked-by: Rob Herring Reviewed-by: Guenter Roeck -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH (v5) 3/11] MIPS: bmips: Add bcm6345-l2-timer interrupt controller
On 11/28/2015 04:26 AM, Simon Arlott wrote: Add the BCM6345/BCM6318 timer as an interrupt controller so that it can be used by the watchdog to warn that its timer will expire soon. Support for clocksource/clockevents is not implemented as the timer interrupt is not per CPU (except on the BCM6318) and the MIPS clock is better. This could be added later if required without changing the device tree binding. Signed-off-by: Simon Arlott Hi Simon, can you please re-send the entire series, with all Acked-by:/Reviewed-by: tags as appropriate ? Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 3/6] watchdog: ts4800: add driver for TS-4800 watchdog
On 11/30/2015 07:59 AM, Damien Riegel wrote:
This watchdog is instantiated in a FPGA that is memory mapped. It is
made of only one register, called the feed register. Writing to this
register will re-arm the watchdog for a given time (and enable it if it
was disable). It can be disabled by writing a special value into it.
It is part of a syscon block, and the watchdog register offset in this
block varies from board to board. This offset is passed in the syscon
property after the phandle to the syscon node.
Signed-off-by: Damien Riegel
Reviewed-by: Guenter Roeck
---
.../devicetree/bindings/watchdog/ts4800-wdt.txt| 25 +++
drivers/watchdog/Kconfig | 10 +
drivers/watchdog/Makefile | 1 +
drivers/watchdog/ts4800_wdt.c | 215 +
4 files changed, 251 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt
create mode 100644 drivers/watchdog/ts4800_wdt.c
diff --git a/Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt
b/Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt
new file mode 100644
index 000..8f6caad
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt
@@ -0,0 +1,25 @@
+Technologic Systems Watchdog
+
+Required properties:
+- compatible: must be "technologic,ts4800-wdt"
+- syscon: phandle / integer array that points to the syscon node which
+ describes the FPGA's syscon registers.
+ - phandle to FPGA's syscon
+ - offset to the watchdog register
+
+Optional property:
+- timeout-sec: contains the watchdog timeout in seconds.
+
+Example:
+
+syscon: syscon@b001 {
+ compatible = "syscon", "simple-mfd";
+ reg = <0xb001 0x3d>;
+ reg-io-width = <2>;
+
+ wdt@e {
+ compatible = "technologic,ts4800-wdt";
+ syscon = <&syscon 0xe>;
+ timeout-sec = <10>;
+ };
+}
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 79e1aa1..bb624d2 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -426,6 +426,16 @@ config NUC900_WATCHDOG
To compile this driver as a module, choose M here: the
module will be called nuc900_wdt.
+config TS4800_WATCHDOG
+ tristate "TS-4800 Watchdog"
+ depends on HAS_IOMEM && OF
+ select WATCHDOG_CORE
+ select MFD_SYSCON
+ help
+ Technologic Systems TS-4800 has watchdog timer implemented in
+ an external FPGA. Say Y here if you want to support for the
+ watchdog timer on TS-4800 board.
+
config TS72XX_WATCHDOG
tristate "TS-72XX SBC Watchdog"
depends on MACH_TS72XX
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 0c616e3..3863ce0 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_RN5T618_WATCHDOG) += rn5t618_wdt.o
obj-$(CONFIG_COH901327_WATCHDOG) += coh901327_wdt.o
obj-$(CONFIG_STMP3XXX_RTC_WATCHDOG) += stmp3xxx_rtc_wdt.o
obj-$(CONFIG_NUC900_WATCHDOG) += nuc900_wdt.o
+obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
diff --git a/drivers/watchdog/ts4800_wdt.c b/drivers/watchdog/ts4800_wdt.c
new file mode 100644
index 000..2b8de86
--- /dev/null
+++ b/drivers/watchdog/ts4800_wdt.c
@@ -0,0 +1,215 @@
+/*
+ * Watchdog driver for TS-4800 based boards
+ *
+ * Copyright (c) 2015 - Savoir-faire Linux
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout,
+ "Watchdog cannot be stopped once started (default="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+/* possible feed values */
+#define TS4800_WDT_FEED_2S 0x1
+#define TS4800_WDT_FEED_10S 0x2
+#define TS4800_WDT_DISABLE 0x3
+
+struct ts4800_wdt {
+ struct watchdog_device wdd;
+ struct regmap *regmap;
+ u32 feed_offset;
+ u32 feed_val;
+};
+
+/*
+ * TS-4800 supports the following timeout values:
+ *
+ * value desc
+ * -
+ * 0feed for 338ms
+ * 1feed for 2.706s
+ * 2feed for 10.824s
+ * 3disable watchdog
+ *
+ * Keep the regmap/timeout map ordered by timeout
+ */
+static const struct {
+ const int timeout;
+ const int regval;
+} ts4800_wdt_map[] = {
+ { 2, TS4800_WDT_FEED_2S },
+ { 10
Re: [PATCH v4 1/2] watchdog: add Alphascale asm9260-wdt driver
On 11/25/2015 11:33 AM, Oleksij Rempel wrote:
Add WD support for Alphascale asm9260 SoC. This driver
provide support for different function modes:
- HW mode to trigger SoC reset on timeout
- SW mode do soft reset if needed
- DEBUG mode
Signed-off-by: Oleksij Rempel
Reviewed-by: Guenter Roeck
---
drivers/watchdog/Kconfig | 10 +
drivers/watchdog/Makefile | 1 +
drivers/watchdog/asm9260_wdt.c | 403 +
3 files changed, 414 insertions(+)
create mode 100644 drivers/watchdog/asm9260_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c68edc1..9cd9b75 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -173,6 +173,16 @@ config ARM_SP805_WATCHDOG
ARM Primecell SP805 Watchdog timer. This will reboot your system when
the timeout is reached.
+config ASM9260_WATCHDOG
+ tristate "Alphascale ASM9260 watchdog"
+ depends on MACH_ASM9260
+ depends on OF
+ select WATCHDOG_CORE
+ select RESET_CONTROLLER
+ help
+ Watchdog timer embedded into Alphascale asm9260 chips. This will
reboot your
+ system when the timeout is reached.
+
config AT91RM9200_WATCHDOG
tristate "AT91RM9200 watchdog"
depends on SOC_AT91RM9200 && MFD_SYSCON
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 0c616e3..bd7b0cd 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o
# ARM Architecture
obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o
+obj-$(CONFIG_ASM9260_WATCHDOG) += asm9260_wdt.o
obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o
obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o
obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o
diff --git a/drivers/watchdog/asm9260_wdt.c b/drivers/watchdog/asm9260_wdt.c
new file mode 100644
index 000..1c22ff4
--- /dev/null
+++ b/drivers/watchdog/asm9260_wdt.c
@@ -0,0 +1,403 @@
+/*
+ * Watchdog driver for Alphascale ASM9260.
+ *
+ * Copyright (c) 2014 Oleksij Rempel
+ *
+ * Licensed under GPLv2 or later.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define CLOCK_FREQ 100
+
+/* Watchdog Mode register */
+#define HW_WDMOD 0x00
+/* Wake interrupt. Set by HW, can't be cleared. */
+#define BM_MOD_WDINT BIT(3)
+/* This bit set if timeout reached. Cleared by SW. */
+#define BM_MOD_WDTOF BIT(2)
+/* HW Reset on timeout */
+#define BM_MOD_WDRESET BIT(1)
+/* WD enable */
+#define BM_MOD_WDENBIT(0)
+
+/*
+ * Watchdog Timer Constant register
+ * Minimal value is 0xff, the meaning of this value
+ * depends on used clock: T = WDCLK * (0xff + 1) * 4
+ */
+#define HW_WDTC0x04
+#define BM_WDTC_MAX(freq) (0x7fff / (freq))
+
+/* Watchdog Feed register */
+#define HW_WDFEED 0x08
+
+/* Watchdog Timer Value register */
+#define HW_WDTV0x0c
+
+#define ASM9260_WDT_DEFAULT_TIMEOUT30
+
+enum asm9260_wdt_mode {
+ HW_RESET,
+ SW_RESET,
+ DEBUG,
+};
+
+struct asm9260_wdt_priv {
+ struct device *dev;
+ struct watchdog_device wdd;
+ struct clk *clk;
+ struct clk *clk_ahb;
+ struct reset_control*rst;
+ struct notifier_block restart_handler;
+
+ void __iomem*iobase;
+ int irq;
+ unsigned long wdt_freq;
+ enum asm9260_wdt_mode mode;
+};
+
+static int asm9260_wdt_feed(struct watchdog_device *wdd)
+{
+ struct asm9260_wdt_priv *priv = watchdog_get_drvdata(wdd);
+
+ iowrite32(0xaa, priv->iobase + HW_WDFEED);
+ iowrite32(0x55, priv->iobase + HW_WDFEED);
+
+ return 0;
+}
+
+static unsigned int asm9260_wdt_gettimeleft(struct watchdog_device *wdd)
+{
+ struct asm9260_wdt_priv *priv = watchdog_get_drvdata(wdd);
+ u32 counter;
+
+ counter = ioread32(priv->iobase + HW_WDTV);
+
+ return DIV_ROUND_CLOSEST(counter, priv->wdt_freq);
+}
+
+static int asm9260_wdt_updatetimeout(struct watchdog_device *wdd)
+{
+ struct asm9260_wdt_priv *priv = watchdog_get_drvdata(wdd);
+ u32 counter;
+
+ counter = wdd->timeout * priv->wdt_freq;
+
+ iowrite32(counter, priv->iobase + HW_WDTC);
+
+ return 0;
+}
+
+static int asm9260_wdt_enable(struct watchdog_device *wdd)
+{
+ struct asm9260_wdt_priv *priv = watchdog_get_drvdata(wdd);
+ u32 mode = 0;
+
+ if (priv->mode == HW_RESET)
+ mode = BM_MOD_WDRESET;
+
+ iowrite32(BM_MOD_WDEN | mode, priv->iobase + HW_WDMOD);
+
+ asm9260_wdt_updatetimeout(wdd);
+
+ asm9260_wdt_feed(wdd);
+
+
Re: [PATCH v4 1/2] watchdog: add Alphascale asm9260-wdt driver
On 11/25/2015 11:33 AM, Oleksij Rempel wrote: Add WD support for Alphascale asm9260 SoC. This driver provide support for different function modes: - HW mode to trigger SoC reset on timeout - SW mode do soft reset if needed - DEBUG mode Signed-off-by: Oleksij Rempel Reviewed-by: Guenter Roeck -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/10] watchdog: bcm63xx_wdt: Use WATCHDOG_CORE
On 11/25/2015 05:02 AM, Simon Arlott wrote:
On Wed, November 25, 2015 02:44, Guenter Roeck wrote:
The "running" flag should no longer be needed. watchdog_active()
should provide that information.
I'm going to need to keep that because I need to know if it's running
in the interrupt handler, and wdd->lock is a mutex.
@@ -306,17 +202,18 @@ unregister_timer:
static int bcm63xx_wdt_remove(struct platform_device *pdev)
{
- if (!nowayout)
- bcm63xx_wdt_hw_stop();
+ struct watchdog_device *wdd = platform_get_drvdata(pdev);
- misc_deregister(&bcm63xx_wdt_miscdev);
bcm63xx_timer_unregister(TIMER_WDT_ID);
+ watchdog_unregister_device(wdd);
Shouldn't that come first, before unregistering the timer ?
No, because wdd->dev is used in the interrupt handler. I will have to
move registration of the interrupt to after creating the watchdog
because it could currently be used before wdd->dev is set.
Does unregistering the timer disable the interrupt ?
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] watchdog: add Alphascale asm9260-wdt driver
On 11/24/2015 01:40 PM, Oleksij Rempel wrote:
Add WD support for Alphascale asm9260 SoC. This driver
provide support for different function modes:
- HW mode to trigger SoC reset on timeout
- SW mode do soft reset if needed
- DEBUG mode
Optional support for stopping watchdog. If reset binding are not provided
this driver will work in nowayout mode.
I think this is no longer optional ?
Signed-off-by: Oleksij Rempel
---
drivers/watchdog/Kconfig | 10 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/asm9260_wdt.c | 400 +
3 files changed, 411 insertions(+)
create mode 100644 drivers/watchdog/asm9260_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c68edc1..9cd9b75 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -173,6 +173,16 @@ config ARM_SP805_WATCHDOG
ARM Primecell SP805 Watchdog timer. This will reboot your system when
the timeout is reached.
+config ASM9260_WATCHDOG
+ tristate "Alphascale ASM9260 watchdog"
+ depends on MACH_ASM9260
+ depends on OF
+ select WATCHDOG_CORE
+ select RESET_CONTROLLER
+ help
+ Watchdog timer embedded into Alphascale asm9260 chips. This will
reboot your
+ system when the timeout is reached.
+
config AT91RM9200_WATCHDOG
tristate "AT91RM9200 watchdog"
depends on SOC_AT91RM9200 && MFD_SYSCON
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 0c616e3..bd7b0cd 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o
# ARM Architecture
obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o
+obj-$(CONFIG_ASM9260_WATCHDOG) += asm9260_wdt.o
obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o
obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o
obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o
diff --git a/drivers/watchdog/asm9260_wdt.c b/drivers/watchdog/asm9260_wdt.c
new file mode 100644
index 000..c8daeff
--- /dev/null
+++ b/drivers/watchdog/asm9260_wdt.c
@@ -0,0 +1,400 @@
+/*
+ * Watchdog driver for Alphascale ASM9260.
+ *
+ * Copyright (c) 2014 Oleksij Rempel
+ *
+ * Licensed under GPLv2 or later.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define CLOCK_FREQ 100
+
+/* Watchdog Mode register */
+#define HW_WDMOD 0x00
+/* Wake interrupt. Set by HW, can't be cleared. */
+#define BM_MOD_WDINT BIT(3)
+/* This bit set if timeout reached. Cleared by SW. */
+#define BM_MOD_WDTOF BIT(2)
+/* HW Reset on timeout */
+#define BM_MOD_WDRESET BIT(1)
+/* WD enable */
+#define BM_MOD_WDENBIT(0)
+
+/*
+ * Watchdog Timer Constant register
+ * Minimal value is 0xff, the meaning of this value
+ * depends on used clock: T = WDCLK * (0xff + 1) * 4
+ */
+#define HW_WDTC0x04
+#define BM_WDTC_MAX(freq) (0x7fff / (freq))
+
+/* Watchdog Feed register */
+#define HW_WDFEED 0x08
+
+/* Watchdog Timer Value register */
+#define HW_WDTV0x0c
+
+#define ASM9260_WDT_DEFAULT_TIMEOUT30
+
+enum asm9260_wdt_mode {
+ HW_RESET,
+ SW_RESET,
+ DEBUG,
+};
+
+struct asm9260_wdt_priv {
+ struct device *dev;
+ struct watchdog_device wdd;
+ struct clk *clk;
+ struct clk *clk_ahb;
+ struct reset_control*rst;
+ struct notifier_block restart_handler;
+
+ void __iomem*iobase;
+ int irq;
+ unsigned long wdt_freq;
+ enum asm9260_wdt_mode mode;
+};
+
+static int asm9260_wdt_feed(struct watchdog_device *wdd)
+{
+ struct asm9260_wdt_priv *priv = watchdog_get_drvdata(wdd);
+
+ iowrite32(0xaa, priv->iobase + HW_WDFEED);
+ iowrite32(0x55, priv->iobase + HW_WDFEED);
+
+ return 0;
+}
+
+static unsigned int asm9260_wdt_gettimeleft(struct watchdog_device *wdd)
+{
+ struct asm9260_wdt_priv *priv = watchdog_get_drvdata(wdd);
+ u32 counter;
+
+ counter = ioread32(priv->iobase + HW_WDTV);
+
+ return DIV_ROUND_CLOSEST(counter, priv->wdt_freq);
+}
+
+static int asm9260_wdt_updatetimeout(struct watchdog_device *wdd)
+{
+ struct asm9260_wdt_priv *priv = watchdog_get_drvdata(wdd);
+ u32 counter;
+
+ counter = wdd->timeout * priv->wdt_freq;
+
+ iowrite32(counter, priv->iobase + HW_WDTC);
+
+ return 0;
+}
+
+static int asm9260_wdt_enable(struct watchdog_device *wdd)
+{
+ struct asm9260_wdt_priv *priv = watchdog_get_drvdata(wdd);
+ u32 mode = 0;
+
+ if (priv->mode == HW_RESET)
+ mode = BM_MOD_WDRESET;
+
+ iowrite32(BM_MOD_WDEN | mode, priv->iobase + HW_WDMOD);
+
+ asm9260
Re: [PATCH (v2) 7/10] watchdog: bcm63xx_wdt: Add get_timeleft function
On 11/24/2015 02:15 PM, Simon Arlott wrote:
Return the remaining time from the hardware control register.
Warn when the device is registered if the hardware watchdog is currently
running and report the remaining time left.
This is really two logical changes, isn't it ?
Nice trick to figure out if the watchdog is running.
What is the impact ? Will this result in interrupts ?
If so, would it make sense to _not_ reset the system after a timeout
in this case, but to keep pinging the watchdog while the watchdog device
is not open ?
Thanks,
Guenter
Signed-off-by: Simon Arlott
---
Changed "if (timeleft > 0)" to "if (hw->running)" when checking if a
warning should be printed, in case the time left is truncated down to
0 seconds.
drivers/watchdog/bcm63xx_wdt.c | 37 +
1 file changed, 37 insertions(+)
diff --git a/drivers/watchdog/bcm63xx_wdt.c b/drivers/watchdog/bcm63xx_wdt.c
index 3c7667a..9d099e0 100644
--- a/drivers/watchdog/bcm63xx_wdt.c
+++ b/drivers/watchdog/bcm63xx_wdt.c
@@ -14,6 +14,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include
+#include
#include
#include
#include
@@ -75,6 +76,19 @@ static int bcm63xx_wdt_stop(struct watchdog_device *wdd)
return 0;
}
+static unsigned int bcm63xx_wdt_get_timeleft(struct watchdog_device *wdd)
+{
+ struct bcm63xx_wdt_hw *hw = watchdog_get_drvdata(wdd);
+ unsigned long flags;
+ u32 val;
+
+ raw_spin_lock_irqsave(&hw->lock, flags);
+ val = __raw_readl(hw->regs + WDT_CTL_REG);
+ val /= hw->clock_hz;
+ raw_spin_unlock_irqrestore(&hw->lock, flags);
+ return val;
+}
+
static int bcm63xx_wdt_set_timeout(struct watchdog_device *wdd,
unsigned int timeout)
{
@@ -130,6 +144,7 @@ static struct watchdog_ops bcm63xx_wdt_ops = {
.owner = THIS_MODULE,
.start = bcm63xx_wdt_start,
.stop = bcm63xx_wdt_stop,
+ .get_timeleft = bcm63xx_wdt_get_timeleft,
.set_timeout = bcm63xx_wdt_set_timeout,
};
@@ -144,6 +159,8 @@ static int bcm63xx_wdt_probe(struct platform_device *pdev)
struct bcm63xx_wdt_hw *hw;
struct watchdog_device *wdd;
struct resource *r;
+ u32 timeleft1, timeleft2;
+ unsigned int timeleft;
int ret;
hw = devm_kzalloc(&pdev->dev, sizeof(*hw), GFP_KERNEL);
@@ -197,6 +214,23 @@ static int bcm63xx_wdt_probe(struct platform_device *pdev)
watchdog_init_timeout(wdd, 0, &pdev->dev);
watchdog_set_nowayout(wdd, nowayout);
+ /* Compare two reads of the time left value, 2 clock ticks apart */
+ rmb();
+ timeleft1 = __raw_readl(hw->regs + WDT_CTL_REG);
+ udelay(DIV_ROUND_UP(100, hw->clock_hz / 2));
+ /* Ensure the register is read twice */
+ rmb();
+ timeleft2 = __raw_readl(hw->regs + WDT_CTL_REG);
+
+ /* If the time left is changing, the watchdog is running */
+ if (timeleft1 != timeleft2) {
+ hw->running = true;
+ timeleft = bcm63xx_wdt_get_timeleft(wdd);
+ } else {
+ hw->running = false;
+ timeleft = 0;
+ }
+
ret = bcm63xx_timer_register(TIMER_WDT_ID, bcm63xx_wdt_isr, wdd);
if (ret < 0) {
dev_err(&pdev->dev, "failed to register wdt timer isr\n");
@@ -214,6 +248,8 @@ static int bcm63xx_wdt_probe(struct platform_device *pdev)
dev_name(wdd->dev), hw->regs,
wdd->timeout, wdd->max_timeout);
+ if (hw->running)
+ dev_alert(wdd->dev, "running, reboot in %us\n", timeleft);
return 0;
unregister_timer:
@@ -255,6 +291,7 @@ module_platform_driver(bcm63xx_wdt_driver);
MODULE_AUTHOR("Miguel Gaio ");
MODULE_AUTHOR("Florian Fainelli ");
+MODULE_AUTHOR("Simon Arlott");
MODULE_DESCRIPTION("Driver for the Broadcom BCM63xx SoC watchdog");
MODULE_LICENSE("GPL");
MODULE_ALIAS("platform:bcm63xx-wdt");
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/10] watchdog: bcm63xx_wdt: Use WATCHDOG_CORE
Hi Simon,
On 11/22/2015 06:06 AM, Simon Arlott wrote:
Convert bcm63xx_wdt to use WATCHDOG_CORE.
The default and maximum time constants that are only used once have been
moved to the initialisation of the struct watchdog_device.
Comments inline.
Thanks,
Guenter
Signed-off-by: Simon Arlott
---
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/bcm63xx_wdt.c | 249 -
2 files changed, 74 insertions(+), 176 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 7a8a6c6..6815b74 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1273,6 +1273,7 @@ config OCTEON_WDT
config BCM63XX_WDT
tristate "Broadcom BCM63xx hardware watchdog"
depends on BCM63XX
+ select WATCHDOG_CORE
help
Watchdog driver for the built in watchdog hardware in Broadcom
BCM63xx SoC.
diff --git a/drivers/watchdog/bcm63xx_wdt.c b/drivers/watchdog/bcm63xx_wdt.c
index f88fc97..1d2a501 100644
--- a/drivers/watchdog/bcm63xx_wdt.c
+++ b/drivers/watchdog/bcm63xx_wdt.c
@@ -13,20 +13,15 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#include
#include
-#include
#include
#include
-#include
#include
#include
#include
#include
-#include
#include
#include
-#include
#include
#include
@@ -38,53 +33,57 @@
#define PFX KBUILD_MODNAME
#define WDT_HZ5000/* Fclk */
-#define WDT_DEFAULT_TIME 30 /* seconds */
-#define WDT_MAX_TIME (0x / WDT_HZ) /* seconds */
struct bcm63xx_wdt_hw {
raw_spinlock_t lock;
void __iomem *regs;
- unsigned long inuse;
bool running;
The "running" flag should no longer be needed. watchdog_active()
should provide that information.
};
-static struct bcm63xx_wdt_hw bcm63xx_wdt_device;
-static int expect_close;
-
-static int wdt_time = WDT_DEFAULT_TIME;
static bool nowayout = WATCHDOG_NOWAYOUT;
module_param(nowayout, bool, 0);
MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
-/* HW functions */
-static void bcm63xx_wdt_hw_start(void)
+static int bcm63xx_wdt_start(struct watchdog_device *wdd)
{
+ struct bcm63xx_wdt_hw *hw = watchdog_get_drvdata(wdd);
unsigned long flags;
- raw_spin_lock_irqsave(&bcm63xx_wdt_device.lock, flags);
- bcm_writel(wdt_time * WDT_HZ, bcm63xx_wdt_device.regs + WDT_DEFVAL_REG);
- bcm_writel(WDT_START_1, bcm63xx_wdt_device.regs + WDT_CTL_REG);
- bcm_writel(WDT_START_2, bcm63xx_wdt_device.regs + WDT_CTL_REG);
- bcm63xx_wdt_device.running = true;
- raw_spin_unlock_irqrestore(&bcm63xx_wdt_device.lock, flags);
+ raw_spin_lock_irqsave(&hw->lock, flags);
+ bcm_writel(wdd->timeout * WDT_HZ, hw->regs + WDT_DEFVAL_REG);
+ bcm_writel(WDT_START_1, hw->regs + WDT_CTL_REG);
+ bcm_writel(WDT_START_2, hw->regs + WDT_CTL_REG);
+ hw->running = true;
+ raw_spin_unlock_irqrestore(&hw->lock, flags);
+ return 0;
}
-static void bcm63xx_wdt_hw_stop(void)
+static int bcm63xx_wdt_stop(struct watchdog_device *wdd)
{
+ struct bcm63xx_wdt_hw *hw = watchdog_get_drvdata(wdd);
unsigned long flags;
- raw_spin_lock_irqsave(&bcm63xx_wdt_device.lock, flags);
- bcm_writel(WDT_STOP_1, bcm63xx_wdt_device.regs + WDT_CTL_REG);
- bcm_writel(WDT_STOP_2, bcm63xx_wdt_device.regs + WDT_CTL_REG);
- bcm63xx_wdt_device.running = false;
- raw_spin_unlock_irqrestore(&bcm63xx_wdt_device.lock, flags);
+ raw_spin_lock_irqsave(&hw->lock, flags);
+ bcm_writel(WDT_STOP_1, hw->regs + WDT_CTL_REG);
+ bcm_writel(WDT_STOP_2, hw->regs + WDT_CTL_REG);
+ hw->running = false;
+ raw_spin_unlock_irqrestore(&hw->lock, flags);
+ return 0;
+}
+
+static int bcm63xx_wdt_set_timeout(struct watchdog_device *wdd,
+ unsigned int timeout)
+{
+ wdd->timeout = timeout;
+ return bcm63xx_wdt_start(wdd);
If I see correctly, there is no ping function. In that case, the watchdog core
will call the start function after updating the timeout, so there is no need
to do it here.
}
/* The watchdog interrupt occurs when half the timeout is remaining */
static void bcm63xx_wdt_isr(void *data)
{
- struct bcm63xx_wdt_hw *hw = &bcm63xx_wdt_device;
+ struct watchdog_device *wdd = data;
+ struct bcm63xx_wdt_hw *hw = watchdog_get_drvdata(wdd);
unsigned long flags;
raw_spin_lock_irqsave(&hw->lock, flags);
@@ -118,147 +117,36 @@ static void bcm63xx_wdt_isr(void *data)
}
ms = timeleft / (WDT_HZ / 1000);
- pr_alert("warning timer fired, reboot in %ums\n", ms);
+ dev_alert(wdd->dev,
+ "warning timer fired, reboot in %ums\n", ms);
}
raw_spin_unlock_irqrestore(&hw->lock
Re: [PATCH 4/10] watchdog: bcm63xx_wdt: Handle hardware interrupt and remove software timer
On Sun, Nov 22, 2015 at 02:05:16PM +, Simon Arlott wrote:
> There is a level triggered interrupt for the watchdog timer as part of
> the bcm63xx_timer device. The interrupt occurs when the hardware watchdog
> timer reaches 50% of the remaining time.
>
> It is not possible to mask the interrupt within the bcm63xx_timer device.
> To get around this limitation, handle the interrupt by restarting the
> watchdog with the current remaining time (which will be half the previous
> timeout) so that the interrupt occurs again at 1/4th, 1/8th, etc. of the
> original timeout value until the watchdog forces a reboot.
>
> The software timer was restarting the hardware watchdog with a 85 second
> timeout until the software timer expired, and then causing a panic()
> about 42.5 seconds later when the hardware interrupt occurred. The
> hardware watchdog would not reboot until a further 42.5 seconds had
> passed.
>
> Remove the software timer and rely on the hardware timer directly,
> reducing the maximum timeout from 256 seconds to 85 seconds
> (2^32 / WDT_HZ).
>
Florian,
can you have a look into this patch and confirm that there is no better
way to clear the interrupt status ?
Thanks,
Guenter
> Signed-off-by: Simon Arlott
> ---
> drivers/watchdog/bcm63xx_wdt.c | 124
> -
> 1 file changed, 72 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/watchdog/bcm63xx_wdt.c b/drivers/watchdog/bcm63xx_wdt.c
> index ab26fd9..f88fc97 100644
> --- a/drivers/watchdog/bcm63xx_wdt.c
> +++ b/drivers/watchdog/bcm63xx_wdt.c
> @@ -3,6 +3,7 @@
> *
> * Copyright (C) 2007, Miguel Gaio
> * Copyright (C) 2008, Florian Fainelli
> + * Copyright 2015 Simon Arlott
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License
> @@ -20,11 +21,10 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> -#include
> -#include
> #include
> #include
> #include
> @@ -37,16 +37,17 @@
>
> #define PFX KBUILD_MODNAME
>
> -#define WDT_HZ 5000 /* Fclk */
> -#define WDT_DEFAULT_TIME 30 /* seconds */
> -#define WDT_MAX_TIME 256 /* seconds */
> +#define WDT_HZ 5000/* Fclk */
> +#define WDT_DEFAULT_TIME 30 /* seconds */
> +#define WDT_MAX_TIME (0x / WDT_HZ) /* seconds */
>
> -static struct {
> +struct bcm63xx_wdt_hw {
> + raw_spinlock_t lock;
> void __iomem *regs;
> - struct timer_list timer;
> unsigned long inuse;
> - atomic_t ticks;
> -} bcm63xx_wdt_device;
> + bool running;
> +};
> +static struct bcm63xx_wdt_hw bcm63xx_wdt_device;
>
> static int expect_close;
>
> @@ -59,48 +60,67 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped
> once started (default="
> /* HW functions */
> static void bcm63xx_wdt_hw_start(void)
> {
> - bcm_writel(0xfffe, bcm63xx_wdt_device.regs + WDT_DEFVAL_REG);
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&bcm63xx_wdt_device.lock, flags);
> + bcm_writel(wdt_time * WDT_HZ, bcm63xx_wdt_device.regs + WDT_DEFVAL_REG);
> bcm_writel(WDT_START_1, bcm63xx_wdt_device.regs + WDT_CTL_REG);
> bcm_writel(WDT_START_2, bcm63xx_wdt_device.regs + WDT_CTL_REG);
> + bcm63xx_wdt_device.running = true;
> + raw_spin_unlock_irqrestore(&bcm63xx_wdt_device.lock, flags);
> }
>
> static void bcm63xx_wdt_hw_stop(void)
> {
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&bcm63xx_wdt_device.lock, flags);
> bcm_writel(WDT_STOP_1, bcm63xx_wdt_device.regs + WDT_CTL_REG);
> bcm_writel(WDT_STOP_2, bcm63xx_wdt_device.regs + WDT_CTL_REG);
> + bcm63xx_wdt_device.running = false;
> + raw_spin_unlock_irqrestore(&bcm63xx_wdt_device.lock, flags);
> }
>
> +/* The watchdog interrupt occurs when half the timeout is remaining */
> static void bcm63xx_wdt_isr(void *data)
> {
> - struct pt_regs *regs = get_irq_regs();
> -
> - die(PFX " fire", regs);
> -}
> -
> -static void bcm63xx_timer_tick(unsigned long unused)
> -{
> - if (!atomic_dec_and_test(&bcm63xx_wdt_device.ticks)) {
> - bcm63xx_wdt_hw_start();
> - mod_timer(&bcm63xx_wdt_device.timer, jiffies + HZ);
> - } else
> - pr_crit("watchdog will restart system\n");
> -}
> -
> -static void bcm63xx_wdt_pet(void)
> -{
> - atomic_set(&bcm63xx_wdt_device.ticks, wdt_time);
> -}
> -
> -static void bcm63xx_wdt_start(void)
> -{
> - bcm63xx_wdt_pet();
> - bcm63xx_timer_tick(0);
> -}
> + struct bcm63xx_wdt_hw *hw = &bcm63xx_wdt_device;
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&hw->lock, flags);
> + if (!hw->running) {
> + /* Stop the watchdog as it shouldn't be running */
> + bcm_writel(WDT_STOP_1, hw->regs + WDT_CTL_REG);
> + bcm_writel(WDT_STOP_2, hw->regs + WD
Re: [PATCH v2] watchdog: add Alphascale asm9260-wdt driver
On 11/23/2015 01:51 AM, Oleksij Rempel wrote: Seems like this should be a generic wdog property. what do you mean? Using "mode" instead of "alphascale,mode" and implement it in WD framework? You should still read the property in the driver, even it if is made generic. The watchdog framework can't really use the information, most drivers don't support selecting the mode. We can consider moving it into the framework if/when it benefits other drivers. On a side note, I would suggest "software" and "hardware" instead of sw / hw, but that is just a personal preference. Ok, then i will leave it for now as is. If "mode" will be introduced, it can brake compatibility. Using "alphascale,mode" feels save :) Not sure I understand. Using a generic property name doesn't mean anything about its implementation. Devicetree properties are supposed to be OS and implementation independent, no ? Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] watchdog: add Alphascale asm9260-wdt driver
On 11/22/2015 11:11 PM, Oleksij Rempel wrote: Hi, thank you for review! Am 05.11.2015 um 21:43 schrieb Rob Herring: On Thu, Nov 05, 2015 at 10:06:56AM +0100, Oleksij Rempel wrote: Add WD support for Alphascale asm9260 SoC. This driver provide support for different function modes: - HW mode to trigger SoC reset on timeout - SW mode do soft reset if needed - DEBUG mode Optional support for stopping watchdog. If reset binding are not provided this driver will work in nowayout mode. Signed-off-by: Oleksij Rempel --- .../bindings/watchdog/alphascale-asm9260.txt | 39 ++ It is preferred that bindings are a separate patch. drivers/watchdog/Kconfig | 9 + drivers/watchdog/Makefile | 1 + drivers/watchdog/asm9260_wdt.c | 415 + 4 files changed, 464 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/alphascale-asm9260.txt create mode 100644 drivers/watchdog/asm9260_wdt.c diff --git a/Documentation/devicetree/bindings/watchdog/alphascale-asm9260.txt b/Documentation/devicetree/bindings/watchdog/alphascale-asm9260.txt new file mode 100644 index 000..6e54d1f --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/alphascale-asm9260.txt @@ -0,0 +1,39 @@ +Alphascale asm9260 Watchdog timer + +Required properties: + +- compatible : should be "alphascale,asm9260-wdt". +- reg : Specifies base physical address and size of the registers. +- clocks : the clocks feeding the watchdog timer. See clock-bindings.txt +- clock-names : should be set to + "mod" - source for tick counter. + "ahb" - ahb gate. + +Optional properties: +- resets : phandle pointing to the system reset controller with correct + reset line index for watchdog controller reset. This propertie is s/propertie/property/ + required if you need to disable "nowayout" and it neened only with + CONFIG_WATCHDOG_NOWAYOUT=n. The DT cannot depend on certain kernel configs. + Without reseting this WD controller, it is not possible to stop s/reseting/resetting/ + counter. +- reset-names : should be set to "wdt_rst" if "resets" is used. +- timeout-sec : shall contain the default watchdog timeout in seconds, + if unset, the default timeout is 30 seconds. +- alphascale,mode : tree modes are supported s/tree/three/ + "hw" - hw reset (defaul). s/defaul/default/ + "sw" - sw reset. + "debug" - no action is taken. Seems like this should be a generic wdog property. what do you mean? Using "mode" instead of "alphascale,mode" and implement it in WD framework? You should still read the property in the driver, even it if is made generic. The watchdog framework can't really use the information, most drivers don't support selecting the mode. We can consider moving it into the framework if/when it benefits other drivers. On a side note, I would suggest "software" and "hardware" instead of sw / hw, but that is just a personal preference. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] MIPS: bmips: Convert bcm63xx_wdt to use WATCHDOG_CORE
On 11/21/2015 01:44 PM, Simon Arlott wrote: On 21/11/15 21:32, Guenter Roeck wrote: On 11/21/2015 11:05 AM, Simon Arlott wrote: Convert bcm63xx_wdt to use WATCHDOG_CORE and add a device tree binding. Adds support for the time left value and provides a more effective interrupt handler based on the watchdog warning interrupt behaviour. This removes the unnecessary software countdown timer and replaces the use of bcm63xx_timer with a normal interrupt when not using mach-bcm63xx. Hi Simon, this is really doing a bit too much in a single patch. Conversion to the watchdog infrastructure should probably be the first step, followed by further optimizations and improvements. I'll split it into two patches, but that won't remove the need for #ifdefs. In general, it would be great if we can avoid #ifdef in the code. Maybe there is some other means to determine if one code path needs to be taken or another. The driver may be part of a multi-platform image, and #ifdefs in the code make that all but impossible. Besides, it makes the code really hard to read and understand. It's impossible to avoid the #ifdefs because the driver needs to support mach-bmips while still supporting mach-bcm63xx. I don't think they make it too difficult to understand. Until there are device tree supporting drivers for everything mach-bcm63xx needs, it can't be removed. Even if ifdefs are needed, they don't need to be as extensive as they are. #ifdef around function names can be handled with shim functions, different clock names can be handled by defining the clock name per platform. The interrupt handler registration may not require an #ifdef if it is just made optional. Conditional include files are typically not needed at all. We have some infrastructure changes in the works which will move the need for soft-timers from individual drivers into the watchdog core. Would this possibly be helpful here ? The timer-driven watchdog ping seems to accomplish pretty much the same. There is no need for a software timer. This is not a timer-driven watchdog ping, there is an unmaskable timer interrupt when the watchdog timer has less than 50% remaining. Ok. Maybe I got confused by the interrupt-triggered watchdog ping. I'll have to look into that much more closely; it is quite unusual and complex. The explanation is also not easy to understand. What does "The only way to stop this interrupt" mean ? Repeatedly triggering the interrupt in 1/2, 1/4, 1/8 of the remaining time is really odd. On side note, the subject tag should be "watchdog:", not "MIPS:". Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] MIPS: bmips: Convert bcm63xx_wdt to use WATCHDOG_CORE
On 11/21/2015 11:05 AM, Simon Arlott wrote:
Convert bcm63xx_wdt to use WATCHDOG_CORE and add a device tree binding.
Adds support for the time left value and provides a more effective
interrupt handler based on the watchdog warning interrupt behaviour.
This removes the unnecessary software countdown timer and replaces the
use of bcm63xx_timer with a normal interrupt when not using mach-bcm63xx.
Hi Simon,
this is really doing a bit too much in a single patch.
Conversion to the watchdog infrastructure should probably be
the first step, followed by further optimizations and improvements.
In general, it would be great if we can avoid #ifdef in the code.
Maybe there is some other means to determine if one code path
needs to be taken or another. The driver may be part of a
multi-platform image, and #ifdefs in the code make that all
but impossible. Besides, it makes the code really hard to read
and understand.
We have some infrastructure changes in the works which will move
the need for soft-timers from individual drivers into the watchdog core.
Would this possibly be helpful here ? The timer-driven watchdog ping
seems to accomplish pretty much the same.
Thanks,
Guenter
Signed-off-by: Simon Arlott
---
arch/mips/bcm63xx/prom.c | 1 +
arch/mips/bcm63xx/setup.c | 1 +
arch/mips/include/asm/mach-bcm63xx/bcm63xx_regs.h | 22 --
drivers/watchdog/Kconfig | 4 +-
drivers/watchdog/bcm63xx_wdt.c| 420 +++---
include/linux/bcm63xx_wdt.h | 22 ++
6 files changed, 244 insertions(+), 226 deletions(-)
create mode 100644 include/linux/bcm63xx_wdt.h
diff --git a/arch/mips/bcm63xx/prom.c b/arch/mips/bcm63xx/prom.c
index 7019e29..ba8b354 100644
--- a/arch/mips/bcm63xx/prom.c
+++ b/arch/mips/bcm63xx/prom.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
void __init prom_init(void)
{
diff --git a/arch/mips/bcm63xx/setup.c b/arch/mips/bcm63xx/setup.c
index 240fb4f..6abf364 100644
--- a/arch/mips/bcm63xx/setup.c
+++ b/arch/mips/bcm63xx/setup.c
@@ -21,6 +21,7 @@
#include
#include
#include
+#include
void bcm63xx_machine_halt(void)
{
diff --git a/arch/mips/include/asm/mach-bcm63xx/bcm63xx_regs.h
b/arch/mips/include/asm/mach-bcm63xx/bcm63xx_regs.h
index 5035f09..16a745b 100644
--- a/arch/mips/include/asm/mach-bcm63xx/bcm63xx_regs.h
+++ b/arch/mips/include/asm/mach-bcm63xx/bcm63xx_regs.h
@@ -441,28 +441,6 @@
/*
- * _REG relative to RSET_WDT
- */
-
-/* Watchdog default count register */
-#define WDT_DEFVAL_REG 0x0
-
-/* Watchdog control register */
-#define WDT_CTL_REG0x4
-
-/* Watchdog control register constants */
-#define WDT_START_1(0xff00)
-#define WDT_START_2(0x00ff)
-#define WDT_STOP_1 (0xee00)
-#define WDT_STOP_2 (0x00ee)
-
-/* Watchdog reset length register */
-#define WDT_RSTLEN_REG 0x8
-
-/* Watchdog soft reset register (BCM6328 only) */
-#define WDT_SOFTRESET_REG 0xc
-
-/*
* _REG relative to RSET_GPIO
*/
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 7a8a6c6..0c50add 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1272,7 +1272,9 @@ config OCTEON_WDT
config BCM63XX_WDT
tristate "Broadcom BCM63xx hardware watchdog"
- depends on BCM63XX
+ depends on BCM63XX || BMIPS_GENERIC
+ select WATCHDOG_CORE
+ select BCM6345_L2_TIMER_IRQ if BMIPS_GENERIC
help
Watchdog driver for the built in watchdog hardware in Broadcom
BCM63xx SoC.
diff --git a/drivers/watchdog/bcm63xx_wdt.c b/drivers/watchdog/bcm63xx_wdt.c
index ab26fd9..fff92d0 100644
--- a/drivers/watchdog/bcm63xx_wdt.c
+++ b/drivers/watchdog/bcm63xx_wdt.c
@@ -3,6 +3,7 @@
*
* Copyright (C) 2007, Miguel Gaio
* Copyright (C) 2008, Florian Fainelli
+ * Copyright 2015 Simon Arlott
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
@@ -12,235 +13,165 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#include
+#include
+#include
+#include
#include
-#include
+#include
#include
#include
-#include
#include
#include
+#include
+#include
+#include
#include
-#include
#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
+#ifdef CONFIG_BCM63XX
+# include
+# include
+#endif
#define PFX KBUILD_MODNAME
-#define WDT_HZ 5000 /* Fclk */
-#define WDT_
Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver
On 11/12/2015 04:06 PM, Al Stone wrote: On 11/05/2015 09:41 AM, Guenter Roeck wrote: On 11/05/2015 07:00 AM, Fu Wei wrote: Hi Timur, On 5 November 2015 at 22:40, Timur Tabi wrote: Fu Wei wrote: Did you really read the "Note" above OK, let me paste it again and again: SBSA 2.3 Page 23 : If a larger watch period is required then the compare value can be programmed directly into the compare value register. Well, okay. Sorry, I should have read what you pasted more closely. But I Thanks for reading it again. think that means during initialization, not during the WS0 timeout. I really don't see SBSA say "during initialization, not during the WS0 timeout", please point it out the page number and the line number in SBSA spec. maybe I miss it? Thanks for your help in advance. Anyway, I still don't like the fact that you're programming WCV in the "you don't like" doesn't mean "it is wrong" or "we can't do this", so I will keep this way unless we have better idea to extend second stage timeout. interrupt handler, but I'm not going to make a big deal about it any more. Deal, Thanks a lot. The problem with your driver, as I see it, is that dealing with WS0/WS1 and pretimeout makes the driver so complex that, at least for my part, I am very wary about it. The driver could long since have been accepted if it were not for that. Besides that, I really believe that any system designer using the highest permitted frequency should be willing to live with the consequences, and not force the implementation of a a complex driver. Ultimately, you'll have to decide if you want a simple driver accepted, or a complex driver hanging in the review queue forever. Thanks, Guenter Sorry to poke my head in late like this, but I do have a vested interest in the outcome so I'm very curious. For my work, I need to have an ACPI-supported, SBSA-compliant watchdog timer for arm64, and this series is one of the key pieces to getting there. The plan for me has been: (1) get an FDT based SBSA watchdog timer, (2) add in kernel code to handle the ACPI GTDT table describing timers, then (3) munge the SBSA watchdog timer for use by ACPI. So, is this an actual NAK of the patch series as is? I think it is, but I want it to be clear, and it has not been explicit yet. I am not the maintainer, so I don't make the call. All I am saying is that I don't feel comfortable with the code as is. Part of it is due to the the specification's complexity, which leaves space for (mis)interpretations and bad implementations. Either case, this is just my personal opinion. All you'll have to do is to convince Wim to accept your patch. If it is a NAK, that's fine, but I also want to be sure I understand what the objections are. Based on my understanding of the discussion so far over the multiple versions, I think the primary objection is that the use of pretimeout makes this driver too complex, and indeed complex enough that there is some concern that it could destabilize a running system. Do I have that right? The other possible item I could conclude out of the discussion is that we do not want to have the pretimeout code as part of the watchdog framework; is that also the case or did I misunderstand? Nothing really to do with pretimeout, but with the complexity of implementing it for this driver. As for pretimeout, it does have its issues. Some people say that hitting a pretimeout should result in a panic, others just as strongly say that it should just dump a message to the console. Which does make me a bit wary, since it means that it may be implemented differently in different drivers, which I consider highly undesirable. And finally, a simpler, single stage timeout watchdog driver would be a reasonable thing to accept, yes? I can see where that would make sense. I am quite sure that such a driver would long since have been accepted. The issue for me in that case is that the SBSA requires a two stage timeout, Hmm - really ? This makes me want to step back a bit and re-read the specification to understand where it says that, and what the reasoning might be for such a requirement. so a single stage driver has no real value for me. Now, if I can later add in changes to make the driver into a two stage driver so it is SBSA-compliant, that would also work, but it will make the driver more complex again. At that point, I think I've now gone in a logical circle and the changes would not be accepted so I could never get to my goal of an SBSA-compliant driver. I don't think that's what was meant, so what did I miss? Thanks in advance for any clarifications that can be provided.I really do appreciate it. Email is not always the clearest mechanism for communication so sometimes I have to ask odd questions like these so I can understand what is happening.
[PATCH] of: Provide static inline function for of_translate_address if needed
If OF_ADDRESS is not configured, builds can fail with errors such as
drivers/net/ethernet/hisilicon/hns_mdio.c:
In function 'hns_mdio_bus_name':
drivers/net/ethernet/hisilicon/hns_mdio.c:411:3:
error: implicit declaration of function 'of_translate_address'
as currently seen when building sparc:allmodconfig.
Introduce a static inline function if OF_ADDRESS is not configured to fix
the build failure. Return OF_BAD_ADDR in this case. For this to work, the
definition of OF_BAD_ADDR has to be moved outside CONFIG_OF conditional
code.
Fixes: 876133d3161d ("net: hisilicon: add OF dependency")
Cc: Arnd Bergmann
Signed-off-by: Guenter Roeck
---
include/linux/of.h | 4 ++--
include/linux/of_address.h | 7 +++
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/include/linux/of.h b/include/linux/of.h
index 2194b8ca41f9..dd10626a615f 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -126,6 +126,8 @@ extern raw_spinlock_t devtree_lock;
#define OF_POPULATED 3 /* device already created for the node */
#define OF_POPULATED_BUS 4 /* of_platform_populate recursed to children
of this node */
+#define OF_BAD_ADDR((u64)-1)
+
#ifdef CONFIG_OF
void of_core_init(void);
@@ -229,8 +231,6 @@ static inline unsigned long of_read_ulong(const __be32
*cell, int size)
#define OF_IS_DYNAMIC(x) test_bit(OF_DYNAMIC, &x->_flags)
#define OF_MARK_DYNAMIC(x) set_bit(OF_DYNAMIC, &x->_flags)
-#define OF_BAD_ADDR((u64)-1)
-
static inline const char *of_node_full_name(const struct device_node *np)
{
return np ? np->full_name : "";
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index d88e81be6368..507daad0bc8d 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -57,6 +57,13 @@ extern int of_dma_get_range(struct device_node *np, u64
*dma_addr,
u64 *paddr, u64 *size);
extern bool of_dma_is_coherent(struct device_node *np);
#else /* CONFIG_OF_ADDRESS */
+
+static inline u64 of_translate_address(struct device_node *np,
+ const __be32 *addr)
+{
+ return OF_BAD_ADDR;
+}
+
static inline struct device_node *of_find_matching_node_by_address(
struct device_node *from,
const struct of_device_id *matches,
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop
On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote: > On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck wrote: > > On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote: > >> From: Tim Harvey > >> > >> The IMX6 watchdog supports assertion of a signal (WDOG_B) which > >> can be pinmux'd to an external pin. This is typically used for boards that > >> have PMIC's in control of the IMX6 power rails. In fact, failure to use > >> such an external reset on boards with external PMIC's can result in various > >> hangs due to the IMX6 not being fully reset [1] as well as the board > >> failing > >> to reset because its PMIC has not been reset to provide adequate voltage > >> for > >> the CPU when coming out of reset at 800Mhz. > >> > >> This uses a new device-tree property 'ext-reset-output' to indicate the > >> board has such a reset and to cause the watchdog to be configured to assert > >> WDOG_B instead of an internal reset both on a watchdog timeout and in > >> system_restart. > >> > >> [1] > >> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html > >> > >> Cc: Lucas Stach > >> Signed-off-by: Tim Harvey > >> --- > >> .../devicetree/bindings/watchdog/fsl-imx-wdt.txt | 2 ++ > >> drivers/watchdog/imx2_wdt.c | 20 > >> ++-- > >> 2 files changed, 20 insertions(+), 2 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt > >> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt > >> index 8dab6fd..9b89b3a 100644 > >> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt > >> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt > >> @@ -9,6 +9,8 @@ Optional property: > > > > properties ? > > > >> - big-endian: If present the watchdog device's registers are implemented > >>in big endian mode, otherwise in native mode(same with CPU), for more > >>detail please see: Documentation/devicetree/bindings/regmap/regmap.txt. > >> +- ext-reset-output: If present the watchdog device is configured to > >> assert its > > > > Should that have a vendor prefix ? Also, not sure if "-output" > > has any real value in the property name. "fsl,external-reset", maybe ? > > Hi Guenter, > > I don't see why a vendor prefix is necessary - its a feature of the > IMX6 watchdog supported by this driver to be able to trigger an > internal chip-level reset and/or an external signal that can be hooked > to additional hardware. > Sounded like vendor specific to me, but then I am not a devicetree maintainer, so I am not an authority on the subject. > I started with ext-reset, but it was suggested > (http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347437.html) > to also make it clear that this is an 'output' and not a reset input. > No problem, as long as you get an Ack from a devicetree maintainer. Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop
On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
> From: Tim Harvey
>
> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
> can be pinmux'd to an external pin. This is typically used for boards that
> have PMIC's in control of the IMX6 power rails. In fact, failure to use
> such an external reset on boards with external PMIC's can result in various
> hangs due to the IMX6 not being fully reset [1] as well as the board failing
> to reset because its PMIC has not been reset to provide adequate voltage for
> the CPU when coming out of reset at 800Mhz.
>
> This uses a new device-tree property 'ext-reset-output' to indicate the
> board has such a reset and to cause the watchdog to be configured to assert
> WDOG_B instead of an internal reset both on a watchdog timeout and in
> system_restart.
>
> [1]
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>
> Cc: Lucas Stach
> Signed-off-by: Tim Harvey
> ---
> .../devicetree/bindings/watchdog/fsl-imx-wdt.txt | 2 ++
> drivers/watchdog/imx2_wdt.c | 20
> ++--
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> index 8dab6fd..9b89b3a 100644
> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> @@ -9,6 +9,8 @@ Optional property:
properties ?
> - big-endian: If present the watchdog device's registers are implemented
>in big endian mode, otherwise in native mode(same with CPU), for more
>detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
> +- ext-reset-output: If present the watchdog device is configured to assert
> its
Should that have a vendor prefix ? Also, not sure if "-output"
has any real value in the property name. "fsl,external-reset", maybe ?
> + external reset (WDOG_B) instead of issuing a software reset.
>
> Examples:
>
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index 29ef719..84bfec4 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -41,6 +41,8 @@
>
> #define IMX2_WDT_WCR 0x00/* Control Register */
> #define IMX2_WDT_WCR_WT (0xFF << 8) /* -> Watchdog Timeout
> Field */
> +#define IMX2_WDT_WCR_WDA (1 << 5)/* -> External Reset WDOG_B */
> +#define IMX2_WDT_WCR_SRS (1 << 4)/* -> Software Reset Signal */
> #define IMX2_WDT_WCR_WRE (1 << 3)/* -> WDOG Reset Enable */
> #define IMX2_WDT_WCR_WDE (1 << 2)/* -> Watchdog Enable */
> #define IMX2_WDT_WCR_WDZST (1 << 0)/* -> Watchdog timer Suspend */
> @@ -65,6 +67,7 @@ struct imx2_wdt_device {
> struct timer_list timer;/* Pings the watchdog when closed */
> struct watchdog_device wdog;
> struct notifier_block restart_handler;
> + bool ext_reset;
> };
>
> static bool nowayout = WATCHDOG_NOWAYOUT;
> @@ -90,6 +93,13 @@ static int imx2_restart_handler(struct notifier_block
> *this, unsigned long mode,
> struct imx2_wdt_device *wdev = container_of(this,
> struct imx2_wdt_device,
> restart_handler);
> +
> + /* Use internal reset or external - not both */
> + if (wdev->ext_reset)
> + wcr_enable |= IMX2_WDT_WCR_SRS; /* do not assert int reset */
> + else
> + wcr_enable |= IMX2_WDT_WCR_WDA; /* do not assert ext-reset */
> +
> /* Assert SRS signal */
> regmap_write(wdev->regmap, IMX2_WDT_WCR, wcr_enable);
> /*
> @@ -119,8 +129,12 @@ static inline void imx2_wdt_setup(struct watchdog_device
> *wdog)
> val |= IMX2_WDT_WCR_WDZST;
> /* Strip the old watchdog Time-Out value */
> val &= ~IMX2_WDT_WCR_WT;
> - /* Generate reset if WDOG times out */
> - val &= ~IMX2_WDT_WCR_WRE;
> + /* Generate internal chip-level reset if WDOG times out */
> + if (!wdev->ext_reset)
> + val &= ~IMX2_WDT_WCR_WRE;
> + /* Or if external-reset assert WDOG_B reset only on time-out */
> + else
> + val |= IMX2_WDT_WCR_WRE;
I don't really understand what this means. Normally I would hope that a watchdog
only generates a reset if/when it times out.
> /* Keep Watchdog Disabled */
> val &= ~IMX2_WDT_WCR_WDE;
> /* Set the watchdog's Time-Out value */
> @@ -267,6 +281,8 @@ static int __init imx2_wdt_probe(struct platform_device
> *pdev)
> regmap_read(wdev->regmap, IMX2_WDT_WRSR, &val);
> wdog->bootstatus = val & IMX2_WDT_WRSR_TOUT ? WDIOF_CARDRESET : 0;
>
> + wdev->ext_reset = of_property_read_bool(pdev->dev.of_node,
> + "ext-reset-output");
> wdog->timeout = clamp_t(unsigned, timeout, 1, IM
Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver
On 11/05/2015 07:00 AM, Fu Wei wrote: Hi Timur, On 5 November 2015 at 22:40, Timur Tabi wrote: Fu Wei wrote: Did you really read the "Note" above OK, let me paste it again and again: SBSA 2.3 Page 23 : If a larger watch period is required then the compare value can be programmed directly into the compare value register. Well, okay. Sorry, I should have read what you pasted more closely. But I Thanks for reading it again. think that means during initialization, not during the WS0 timeout. I really don't see SBSA say "during initialization, not during the WS0 timeout", please point it out the page number and the line number in SBSA spec. maybe I miss it? Thanks for your help in advance. Anyway, I still don't like the fact that you're programming WCV in the "you don't like" doesn't mean "it is wrong" or "we can't do this", so I will keep this way unless we have better idea to extend second stage timeout. interrupt handler, but I'm not going to make a big deal about it any more. Deal, Thanks a lot. The problem with your driver, as I see it, is that dealing with WS0/WS1 and pretimeout makes the driver so complex that, at least for my part, I am very wary about it. The driver could long since have been accepted if it were not for that. Besides that, I really believe that any system designer using the highest permitted frequency should be willing to live with the consequences, and not force the implementation of a a complex driver. Ultimately, you'll have to decide if you want a simple driver accepted, or a complex driver hanging in the review queue forever. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] watchdog: add Alphascale asm9260-wdt driver
On 11/05/2015 01:06 AM, Oleksij Rempel wrote:
Add WD support for Alphascale asm9260 SoC. This driver
provide support for different function modes:
- HW mode to trigger SoC reset on timeout
- SW mode do soft reset if needed
- DEBUG mode
Optional support for stopping watchdog. If reset binding are not provided
this driver will work in nowayout mode.
In general, this is considered to be orthogonal: If the user doesn't want
nowayout, and if the watchdog can not be stopped, other drivers issue a warning
that the watchdog was not stopped, and that the system will likely restart.
Signed-off-by: Oleksij Rempel
---
.../bindings/watchdog/alphascale-asm9260.txt | 39 ++
drivers/watchdog/Kconfig | 9 +
drivers/watchdog/Makefile | 1 +
drivers/watchdog/asm9260_wdt.c | 415 +
4 files changed, 464 insertions(+)
create mode 100644
Documentation/devicetree/bindings/watchdog/alphascale-asm9260.txt
create mode 100644 drivers/watchdog/asm9260_wdt.c
diff --git a/Documentation/devicetree/bindings/watchdog/alphascale-asm9260.txt
b/Documentation/devicetree/bindings/watchdog/alphascale-asm9260.txt
new file mode 100644
index 000..6e54d1f
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/alphascale-asm9260.txt
@@ -0,0 +1,39 @@
+Alphascale asm9260 Watchdog timer
+
+Required properties:
+
+- compatible : should be "alphascale,asm9260-wdt".
+- reg : Specifies base physical address and size of the registers.
+- clocks : the clocks feeding the watchdog timer. See clock-bindings.txt
+- clock-names : should be set to
+ "mod" - source for tick counter.
+ "ahb" - ahb gate.
+
+Optional properties:
+- resets : phandle pointing to the system reset controller with correct
+ reset line index for watchdog controller reset. This propertie is
+ required if you need to disable "nowayout" and it neened only with
+ CONFIG_WATCHDOG_NOWAYOUT=n.
+ Without reseting this WD controller, it is not possible to stop
+ counter.
+- reset-names : should be set to "wdt_rst" if "resets" is used.
+- timeout-sec : shall contain the default watchdog timeout in seconds,
+ if unset, the default timeout is 30 seconds.
+- alphascale,mode : tree modes are supported
+ "hw" - hw reset (defaul).
+ "sw" - sw reset.
+ "debug" - no action is taken.
+
+Example:
+
+watchdog0: watchdog@80048000 {
+ compatible = "alphascale,asm9260-wdt";
+ reg = <0x80048000 0x10>;
+ clocks = <&acc CLKID_SYS_WDT>, <&acc CLKID_AHB_WDT>;
+ clock-names = "mod", "ahb";
+ interrupts = <55>;
+ resets = <&rst WDT_RESET>;
+ reset-names = "wdt_rst";
+ timeout-sec = <30>;
+ alphascale,mode = "hw";
+};
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c68edc1..cc5f675 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -173,6 +173,15 @@ config ARM_SP805_WATCHDOG
ARM Primecell SP805 Watchdog timer. This will reboot your system when
the timeout is reached.
+config ASM9260_WATCHDOG
+ tristate "Alphascale ASM9260 watchdog"
+ depends on MACH_ASM9260
+ depends on OF
+ select WATCHDOG_CORE
+ help
+ Watchdog timer embedded into Alphascale asm9260 chips. This will
reboot your
+ system when the timeout is reached.
+
config AT91RM9200_WATCHDOG
tristate "AT91RM9200 watchdog"
depends on SOC_AT91RM9200 && MFD_SYSCON
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 0c616e3..bd7b0cd 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o
# ARM Architecture
obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o
+obj-$(CONFIG_ASM9260_WATCHDOG) += asm9260_wdt.o
obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o
obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o
obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o
diff --git a/drivers/watchdog/asm9260_wdt.c b/drivers/watchdog/asm9260_wdt.c
new file mode 100644
index 000..9f2c321
--- /dev/null
+++ b/drivers/watchdog/asm9260_wdt.c
@@ -0,0 +1,415 @@
+/*
+ * Watchdog driver for Alphascale ASM9260.
+ *
+ * Copyright (c) 2014 Oleksij Rempel
+ *
+ * Licensed under GPLv2 or later.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
I don't see any module parameters. Is this include needed ?
+#include
+#include
+#include
+#include
+#include
Is this include needed ?
+#include
+
+#define CLOCK_FREQ 100
+
+/* Watchdog Mode register */
+#define HW_WDMOD 0x00
+/* Wake interrupt. Set by HW, can't be cleared. */
+#define BM_MOD_WDINT BIT(3)
+/* This bit set if timeout reached. Cleared by SW. */
+#define BM_MOD_WDTOF BIT(2)
+/* HW Reset on timeout */
+#define BM_MOD_WDRESET BIT(1)
+/* WD
Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver
On 11/04/2015 05:59 PM, Timur Tabi wrote:
On Tue, Oct 27, 2015 at 11:06 AM, wrote:
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
+{
+ struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
+ struct watchdog_device *wdd = &gwdt->wdd;
+
+ /* We don't use pretimeout, trigger WS1 now */
+ if (!wdd->pretimeout)
+ sbsa_gwdt_set_wcv(wdd, 0);
So I'm still concerned about the fact this driver depends on an
interrupt handler in order to properly program the hardware. Unlike
some other devices, the SBSA watchdog does not need assistance to
reset on a timeout -- it is a "fire and forget" device. What happens
if there is a hard lockup, and interrupts no longer work?
Keep in mind that 99% of watchdog daemons will not enable the
pre-timeout feature because it's new.
Same here, really.
I would feel much more comfortable if the driver would just use the standard
watchdog timeout and live with (worst case) 20 seconds timeout for now.
This limitation will be gone once the infrastructure is in place to handle
such short timeouts in the watchdog core. Until then, I would argue that the
system designers asked for it if they really select the highest possible
clock rate.
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 14/16] hwmon: add TI LMU hardware fault monitoring driver
On 11/01/2015 09:24 PM, Milo Kim wrote: LM3633 and LM3697 are TI LMU MFD device. Those device have hardware monitoring feature which detects opened or shorted circuit case. Sure, but it only makes sense if you provide standard hwmon attributes which can be interpreted by the "sensors" command. Which is not the case here. You neither have a standard device type (light is not handled by hwmon), nor standard attributes, nor do the attributes return standard values. I think there may be a basic misunderstanding. hwmon is not intended to monitor a specific chip on the board. Its scope is to monitor the system (such as temperature, voltages, or current). In your case, it might be better to attach the attributes to the mfd device. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] watchdog: ts4800: add new driver for TS-4800 watchdog
On Tue, Oct 27, 2015 at 05:51:35PM -0400, Damien Riegel wrote:
> On Tue, Oct 27, 2015 at 04:20:52PM -0500, Dinh Nguyen wrote:
> > On Tue, Oct 27, 2015 at 3:33 PM, Damien Riegel
> > wrote:
> > > Signed-off-by: Damien Riegel
> > > ---
> > > .../devicetree/bindings/watchdog/ts4800-wdt.txt| 12 ++
> > > drivers/watchdog/Kconfig | 9 +
> > > drivers/watchdog/Makefile | 1 +
> > > drivers/watchdog/ts4800_wdt.c | 212
> > > +
> > > 4 files changed, 234 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt
> > > create mode 100644 drivers/watchdog/ts4800_wdt.c
> > >
> > > diff --git a/Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt
> > > b/Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt
> > > new file mode 100644
> > > index 000..06bdb5f
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt
> > > @@ -0,0 +1,12 @@
> > > +Technologic Systems Watchdog
> > > +
> > > +Required properties:
> > > +- compatible : must be "ts,ts4800-wdt"
> > > +- reg : physical base address and length of memory mapped region
> > > +
> > > +Example:
> > > +
> > > +wdt1: wdt@b001 {
> > > + compatible = "ts,ts4800-wdt";
> > > + reg = <0xb001 0x1000>;
> > > +};
> > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > index 241fafd..cf30f3b 100644
> > > --- a/drivers/watchdog/Kconfig
> > > +++ b/drivers/watchdog/Kconfig
> > > @@ -417,6 +417,15 @@ config NUC900_WATCHDOG
> > > To compile this driver as a module, choose M here: the
> > > module will be called nuc900_wdt.
> > >
> > > +config TS4800_WATCHDOG
> > > + tristate "TS-4800 Watchdog"
> > > + depends on SOC_IMX51
> >
> > From the DTS, I saw that this watchdog is on an FPGA, is it limited to only
> > the i.MX51?
>
> Actually, no. I took a quick look at other TS's boards and it is used on
> other boards: TS-4740, TS-4712, TS-4710, and TS-4700 (Marvell
> PXA166/PXA168); TS-4600 (iMX283).
>
> But as far as I know, these boards are not supported by Linux. So,
> should I add more "depends on" even if we don't support them yet; or
> should I let it as is and add other dependances when adding support for
> other boards ?
> I could also drop that line completely.
>
> Anyway, I will rename the driver to have a more generic name. Is
> ts_wdt.c fine for you ?
Can you at least spell that out ? "ts" is a bit _too_ generic.
In general, naming a driver for the first chip it supports is not
problematic. Making it too generic is, on the other side, problematic.
What are you going to do if ts4900 (or ts4801) implements a different
watchdog ?
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Please suggest proper format for DT properties.
On 09/22/2015 01:36 AM, Arnd Bergmann wrote:
On Saturday 19 September 2015 01:36:43 Constantine Shulyupin wrote:
I am designing DT support for a hwmon chip.
It has some sensors, each of them can be:
- "disabled"
- "thermal diode"
- "thermistor"
- "voltage"
Four possible options for DT properties format.
Option 1: Separated property for each sensor.
Example nct7802 node:
nct7802 {
compatible = "nuvoton,nct7802";
reg = <0x2a>;
nuvoton,sensor1-type = "thermistor";
nuvoton,sensor2-type = "disabled";
nuvoton,sensor3-type = "voltage";
};
Option 2: Array of strings for all sensors.
nct7802 {
compatible = "nuvoton,nct7802";
reg = <0x2a>;
nuvoton,sensors-types = "thermistor", "disabled", "voltage";
};
Option 3: Sets of 4 cells.
Borrowed from marvell,reg-init and broadcom,c45-reg-init.
The first cell is the page address,
the second a register address within the page,
the third cell contains a mask to be ANDed with the existing register
value, and the fourth cell is ORed with the result to yield the
new register value. If the third cell has a value of zero,
no read of the existing value is performed.
Example nct7802 node:
nct7802 {
compatible = "nuvoton,nct7802";
reg = <0x2a>;
nct7802,reg-init =
<0 0x21 0 0x01 > // START = 1
<0 0x22 0x03 0x02>; // RTD1_MD = 2
};
I would strongly prefer Option 1 or 2 over option 3.
Between 1 and 2, I'd probably go for 1. Another option might
be to have a subnode per sensor:
nct7802@2a {
compatible = "nuvoton,nct7802";
reg = <0x2a>;
#address-cells=<1>;
#size-cells=<0>;
sensor@1 {
compatible = "nuvoton,nct7802-thermistor";
further-properties;
};
sensor@3 {
compatible = "nuvoton,nct7802-voltage";
for-example-range-mv = <0 5000>;
};
};
I personally would prefer this approach. It would also make it easier to add
more
properties. Wonder what is more appropriate, though - a compatible property or
something like the following ?
sensor-type = "xxx";
I don't have a preference, just asking.
Also, would the index be derived from "@1", or should there be a reg property ?
In either case, I'd say that disabled sensors should not need to
be listed.
Agreed.
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] sysfs: Support is_visible() on binary attributes
On 09/14/2015 05:34 AM, Emilio López wrote:
According to the sysfs header file:
"The returned value will replace static permissions defined in
struct attribute or struct bin_attribute."
but this isn't the case, as is_visible is only called on struct attribute
only. This patch introduces a new is_bin_visible() function to implement
the same functionality for binary attributes, and updates documentation
accordingly.
Signed-off-by: Emilio López
Nitpick below, but otherwise looks ok to me.
Reviewed-by: Guenter Roeck
Guenter
---
Changes from v1:
- Don't overload is_visible, introduce is_bin_visible instead as
discussed on the list.
fs/sysfs/group.c | 17 +++--
include/linux/sysfs.h | 18 ++
2 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 39a0199..51b56e6 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -73,13 +73,26 @@ static int create_files(struct kernfs_node *parent, struct
kobject *kobj,
}
if (grp->bin_attrs) {
- for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
+ for (i = 0, bin_attr = grp->bin_attrs; *bin_attr; i++,
bin_attr++) {
+ umode_t mode = (*bin_attr)->attr.mode;
+
if (update)
kernfs_remove_by_name(parent,
(*bin_attr)->attr.name);
+ if (grp->is_bin_visible) {
+ mode = grp->is_bin_visible(kobj, *bin_attr, i);
+ if (!mode)
+ continue;
+ }
+
+ WARN(mode & ~(SYSFS_PREALLOC | 0664),
+"Attribute %s: Invalid permissions 0%o\n",
+(*bin_attr)->attr.name, mode);
+
+ mode &= SYSFS_PREALLOC | 0664;
Strictly speaking, the mode validation for binary attributes is new and
logically
separate. Should it be mentioned in the commit log, or even be a separate patch
?
error = sysfs_add_file_mode_ns(parent,
&(*bin_attr)->attr, true,
- (*bin_attr)->attr.mode, NULL);
+ mode, NULL);
if (error)
break;
}
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 9f65758..2f66050 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -64,10 +64,18 @@ do {
\
*a new subdirectory with this name.
* @is_visible: Optional: Function to return permissions associated
with an
*attribute of the group. Will be called repeatedly for each
- * attribute in the group. Only read/write permissions as well as
- * SYSFS_PREALLOC are accepted. Must return 0 if an attribute is
- * not visible. The returned value will replace static permissions
- * defined in struct attribute or struct bin_attribute.
+ * non-binary attribute in the group. Only read/write
+ * permissions as well as SYSFS_PREALLOC are accepted. Must
+ * return 0 if an attribute is not visible. The returned value
+ * will replace static permissions defined in struct attribute.
+ * @is_bin_visible:
+ * Optional: Function to return permissions associated with a
+ * binary attribute of the group. Will be called repeatedly
+ * for each binary attribute in the group. Only read/write
+ * permissions as well as SYSFS_PREALLOC are accepted. Must
+ * return 0 if a binary attribute is not visible. The returned
+ * value will replace static permissions defined in
+ * struct bin_attribute.
* @attrs:Pointer to NULL terminated list of attributes.
* @bin_attrs:Pointer to NULL terminated list of binary attributes.
*Either attrs or bin_attrs or both must be provided.
@@ -76,6 +84,8 @@ struct attribute_group {
const char *name;
umode_t (*is_visible)(struct kobject *,
struct attribute *, int);
+ umode_t (*is_bin_visible)(struct kobject *,
+ struct bin_attribute *, int);
struct attribute**attrs;
struct bin_attribute**bin_attrs;
};
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 5/8] Watchdog: introduce ARM SBSA watchdog driver
On 09/10/2015 03:29 PM, Jon Masters wrote: On 08/24/2015 01:01 PM, fu@linaro.org wrote: + /* +* Get the frequency of system counter from the cp15 interface of ARM +* Generic timer. We don't need to check it, because if it returns "0", +* system would panic in very early stage. +*/ + gwdt->clk = arch_timer_get_cntfrq(); Just thinking out loud... What happens later if we virtualize this device within KVM/QEMU/Xen and then live migrate to another system in which the frequency changes? Thinking about it, this scenario would cause severe trouble. I think clocks (like I would assume pretty much all other hardware parameters / registers) need to be virtualized and must not change. Example: clock is set to 100 kHz on original system, and 400 kHz on new system. Timeout is set to 30s, and registers are programmed accordingly. User space sends heartbeats every 15 seconds. In this scenario, the watchdog would time out after 30/4 = 7.5 seconds on the new system, or in other words almost immediately. This would be even worse if the original system had a clock of, say, 10 kHz and the new system would use 400 kHz. This just doesn't work. Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 5/8] Watchdog: introduce ARM SBSA watchdog driver
On Thu, Sep 10, 2015 at 06:29:53PM -0400, Jon Masters wrote: > On 08/24/2015 01:01 PM, fu@linaro.org wrote: > > > + /* > > +* Get the frequency of system counter from the cp15 interface of ARM > > +* Generic timer. We don't need to check it, because if it returns "0", > > +* system would panic in very early stage. > > +*/ > > + gwdt->clk = arch_timer_get_cntfrq(); > > Just thinking out loud... > > What happens later if we virtualize this device within KVM/QEMU/Xen and > then live migrate to another system in which the frequency changes? > I don't know, but I would suspect that we might end up in all kinds of trouble if clocks can change like that, and not just in this driver. Many drivers make the assumption that clock rates are not changed under the hood. If it can happen, shouldn't there be a callback into the drivers using the affected clock(s) ? Also, it seems to me that changing a clock like that would be inherently unsafe. For example, what will happen if the system is stopped and migrated right after the clock frequency is read, but before the returned value is used ? Can that happen ? Or does it only happen during suspend/resume cycles, if it happens ? Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes
On 09/09/2015 06:14 AM, Emilio López wrote: On 09/09/15 01:12, Guenter Roeck wrote: On 09/08/2015 08:58 PM, Greg KH wrote: On Tue, Sep 08, 2015 at 06:10:16PM -0700, Guenter Roeck wrote: Hi Emilio, On 09/08/2015 05:51 PM, Emilio López wrote: Hi Greg & Guenter, [ ... ] Unless I am missing something, this is not explained anywhere, but it is not entirely trivial to understand. I think it should be documented. I agree. I couldn't find any mention of what this int was supposed to be by looking at Documentation/ (is_visible is not even mentioned :/) or include/linux/sysfs.h. Once we settle on something I'll document it before sending a v2. In the include file ? No strong preference, though. By the way, I wrote a quick coccinelle script to match is_visible() users which reference the index (included below), and it found references to drivers which do not seem to use any binary attributes, so I believe changing the index meaning shouldn't be an issue. Good. I agree, make i the number of the bin attribute and that should solve this issue. No, that would conflict with the "normal" use of is_visible for non-binary attributes, and make the index all but useless, since the is_visible function would have to search through all the attributes anyway to figure out which one is being checked. Yeah, using the same indexes would be somewhat pointless, although not many seem to be using it anyway (only 14 files matched). Others seem to be comparing the attr* instead. An alternative would be to use negative indexes for binary attributes and positive indexes for normal attributes. ... and I probably wrote or reviewed a significant percentage of those ;-). Using negative numbers for binary attributes is an interesting idea. Kind of unusual, though. Greg, any thoughts on that ? Ick, no, that's a mess, maybe we just could drop the index alltogether? No, please don't. Having to manually compare dozens of index pointers would be even more of a mess. So, what about keeping it the way it is in the patch, and documenting it thoroughly? Otherwise, we could introduce another "is_bin_visible" function to do this same thing but just on binary attributes, but I'd rather not add a new function pointer if possible. I would prefer to keep and document it. Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes
On 09/08/2015 08:58 PM, Greg KH wrote: On Tue, Sep 08, 2015 at 06:10:16PM -0700, Guenter Roeck wrote: Hi Emilio, On 09/08/2015 05:51 PM, Emilio López wrote: Hi Greg & Guenter, [ ... ] Unless I am missing something, this is not explained anywhere, but it is not entirely trivial to understand. I think it should be documented. I agree. I couldn't find any mention of what this int was supposed to be by looking at Documentation/ (is_visible is not even mentioned :/) or include/linux/sysfs.h. Once we settle on something I'll document it before sending a v2. In the include file ? No strong preference, though. By the way, I wrote a quick coccinelle script to match is_visible() users which reference the index (included below), and it found references to drivers which do not seem to use any binary attributes, so I believe changing the index meaning shouldn't be an issue. Good. I agree, make i the number of the bin attribute and that should solve this issue. No, that would conflict with the "normal" use of is_visible for non-binary attributes, and make the index all but useless, since the is_visible function would have to search through all the attributes anyway to figure out which one is being checked. Yeah, using the same indexes would be somewhat pointless, although not many seem to be using it anyway (only 14 files matched). Others seem to be comparing the attr* instead. An alternative would be to use negative indexes for binary attributes and positive indexes for normal attributes. ... and I probably wrote or reviewed a significant percentage of those ;-). Using negative numbers for binary attributes is an interesting idea. Kind of unusual, though. Greg, any thoughts on that ? Ick, no, that's a mess, maybe we just could drop the index alltogether? No, please don't. Having to manually compare dozens of index pointers would be even more of a mess. Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes
Hi Emilio, On 09/08/2015 05:51 PM, Emilio López wrote: Hi Greg & Guenter, [ ... ] Unless I am missing something, this is not explained anywhere, but it is not entirely trivial to understand. I think it should be documented. I agree. I couldn't find any mention of what this int was supposed to be by looking at Documentation/ (is_visible is not even mentioned :/) or include/linux/sysfs.h. Once we settle on something I'll document it before sending a v2. In the include file ? No strong preference, though. By the way, I wrote a quick coccinelle script to match is_visible() users which reference the index (included below), and it found references to drivers which do not seem to use any binary attributes, so I believe changing the index meaning shouldn't be an issue. Good. I agree, make i the number of the bin attribute and that should solve this issue. No, that would conflict with the "normal" use of is_visible for non-binary attributes, and make the index all but useless, since the is_visible function would have to search through all the attributes anyway to figure out which one is being checked. Yeah, using the same indexes would be somewhat pointless, although not many seem to be using it anyway (only 14 files matched). Others seem to be comparing the attr* instead. An alternative would be to use negative indexes for binary attributes and positive indexes for normal attributes. ... and I probably wrote or reviewed a significant percentage of those ;-). Using negative numbers for binary attributes is an interesting idea. Kind of unusual, though. Greg, any thoughts on that ? Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes
On Tue, Sep 08, 2015 at 12:10:02PM -0700, Greg KH wrote:
> On Tue, Sep 08, 2015 at 08:30:13AM -0700, Guenter Roeck wrote:
> > Emilio,
> >
> > On Tue, Sep 08, 2015 at 09:07:44AM -0300, Emilio López wrote:
> > > According to the sysfs header file:
> > >
> > > "The returned value will replace static permissions defined in
> > > struct attribute or struct bin_attribute."
> > >
> > > but this isn't the case, as is_visible is only called on
> > > struct attribute only. This patch adds the code paths required
> > > to support is_visible() on binary attributes.
> > >
> > > Signed-off-by: Emilio López
> > > ---
> > > fs/sysfs/group.c | 22 ++
> > > 1 file changed, 18 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > > index 39a0199..eb6996a 100644
> > > --- a/fs/sysfs/group.c
> > > +++ b/fs/sysfs/group.c
> > > @@ -37,10 +37,10 @@ static int create_files(struct kernfs_node *parent,
> > > struct kobject *kobj,
> > > {
> > > struct attribute *const *attr;
> > > struct bin_attribute *const *bin_attr;
> > > - int error = 0, i;
> > > + int error = 0, i = 0;
> > >
> > > if (grp->attrs) {
> > > - for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
> > > + for (attr = grp->attrs; *attr && !error; i++, attr++) {
> > > umode_t mode = (*attr)->mode;
> > >
> > > /*
> > > @@ -73,13 +73,27 @@ static int create_files(struct kernfs_node *parent,
> > > struct kobject *kobj,
> > > }
> > >
> > > if (grp->bin_attrs) {
> > > - for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
> > > + for (bin_attr = grp->bin_attrs; *bin_attr; i++, bin_attr++) {
> > > + umode_t mode = (*bin_attr)->attr.mode;
> > > +
> > > if (update)
> > > kernfs_remove_by_name(parent,
> > > (*bin_attr)->attr.name);
> > > + if (grp->is_visible) {
> > > + mode = grp->is_visible(kobj,
> > > +&(*bin_attr)->attr, i);
> >
> > With this, if 'n' is the number of non-binary attributes,
> >
> > for i < n:
> > The index passed to is_visible points to a non-binary attribute.
> > for i >= n:
> > The index passed to is_visible points to the (index - n)th binary
> > attribute.
> >
> > Unless I am missing something, this is not explained anywhere, but it is
> > not entirely trivial to understand. I think it should be documented.
>
> I agree, make i the number of the bin attribute and that should solve
> this issue.
>
No, that would conflict with the "normal" use of is_visible for non-binary
attributes, and make the index all but useless, since the is_visible function
would have to search through all the attributes anyway to figure out which one
is being checked.
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes
Emilio,
On Tue, Sep 08, 2015 at 09:07:44AM -0300, Emilio López wrote:
> According to the sysfs header file:
>
> "The returned value will replace static permissions defined in
> struct attribute or struct bin_attribute."
>
> but this isn't the case, as is_visible is only called on
> struct attribute only. This patch adds the code paths required
> to support is_visible() on binary attributes.
>
> Signed-off-by: Emilio López
> ---
> fs/sysfs/group.c | 22 ++
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 39a0199..eb6996a 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -37,10 +37,10 @@ static int create_files(struct kernfs_node *parent,
> struct kobject *kobj,
> {
> struct attribute *const *attr;
> struct bin_attribute *const *bin_attr;
> - int error = 0, i;
> + int error = 0, i = 0;
>
> if (grp->attrs) {
> - for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
> + for (attr = grp->attrs; *attr && !error; i++, attr++) {
> umode_t mode = (*attr)->mode;
>
> /*
> @@ -73,13 +73,27 @@ static int create_files(struct kernfs_node *parent,
> struct kobject *kobj,
> }
>
> if (grp->bin_attrs) {
> - for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
> + for (bin_attr = grp->bin_attrs; *bin_attr; i++, bin_attr++) {
> + umode_t mode = (*bin_attr)->attr.mode;
> +
> if (update)
> kernfs_remove_by_name(parent,
> (*bin_attr)->attr.name);
> + if (grp->is_visible) {
> + mode = grp->is_visible(kobj,
> +&(*bin_attr)->attr, i);
With this, if 'n' is the number of non-binary attributes,
for i < n:
The index passed to is_visible points to a non-binary attribute.
for i >= n:
The index passed to is_visible points to the (index - n)th binary
attribute.
Unless I am missing something, this is not explained anywhere, but it is
not entirely trivial to understand. I think it should be documented.
Also, it might be a good idea to check through existing code to ensure
that this change doesn't accidentially cause trouble (existing drivers
implementing both attribute types may not expect to see an index variable
pointing to a value larger than the number of elements in the attrs array).
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] watchdog: Watchdog driver for Broadcom Set-Top Box
On 08/31/2015 11:02 AM, Justin Chen wrote: Watchdog driver for Broadcom 7038 and newer chips. Signed-off-by: Justin Chen Reviewed-by: Guenter Roeck -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/2] drivers: watchdog: add a driver to support SAMA5D4 watchdog timer
On 08/27/2015 06:30 PM, Yang, Wenyou wrote: -Original Message- From: Guenter Roeck [mailto:li...@roeck-us.net] Sent: 2015年8月7日 23:26 To: Yang, Wenyou; w...@iguana.be; robh...@kernel.org; pawel.m...@arm.com; mark.rutl...@arm.com; ijc+devicet...@hellion.org.uk; ga...@codeaurora.org Cc: sylvain.roc...@finsecur.com; Ferre, Nicolas; boris.brezillon@free- electrons.com; devicetree@vger.kernel.org; linux-ker...@vger.kernel.org; linux- watch...@vger.kernel.org Subject: Re: [PATCH v6 1/2] drivers: watchdog: add a driver to support SAMA5D4 watchdog timer On 08/06/2015 03:16 AM, Wenyou Yang wrote: >From SAMA5D4, the watchdog timer is upgrated with a new feature, which is describled as in the datasheet, "WDT_MR can be written until a LOCKMR command is issued in WDT_CR". That is to say, as long as the bootstrap and u-boot don't issue a LOCKMR command, WDT_MR can be written more than once in the driver. So the SAMA5D4 watchdog driver's implementation is different from the at91sam9260 watchdog driver implemented in file at91sam9_wdt.c. The user application open the device file to enable the watchdog timer hardware, and close to disable it, and set the watchdog timer timeout by seting WDV and WDD fields of WDT_MR register, and ping the watchdog by issuing WDRSTT command to WDT_CR register with hard-coded key. Signed-off-by: Wenyou Yang Reviewed-by: Guenter Roeck Hi Vim, Can this patch series be merged? Could you please? It is included in the pull request I sent to Wim last week. Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] watchdog: bcm7038: add device tree binding documentation
On Thu, Aug 27, 2015 at 03:53:23PM -0700, Justin Chen wrote:
> Add device tree binding documentation for the watchdog hardware block
> on bcm7038 and newer SoCs.
>
> Signed-off-by: Justin Chen
Acked-by: Guenter Roeck
> ---
> .../devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt | 19
> +++
> 1 file changed, 19 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt
>
> diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt
> b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt
> new file mode 100644
> index 000..39e5cf5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt
> @@ -0,0 +1,19 @@
> +BCM7038 Watchdog timer
> +
> +Required properties:
> +
> +- compatible : should be "brcm,bcm7038-wdt"
> +- reg : Specifies base physical address and size of the registers.
> +
> +Optional properties:
> +
> +- clocks: The clock running the watchdog. If no clock is found the
> + driver will default to 2700 HZ.
> +
> +Example:
> +
> +watchdog {
> + compatible = "brcm,bcm7038-wdt";
> + clocks = <&upg_fixed>;
> + reg = <0xf040a7e8 0x16>;
> +};
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] watchdog: Watchdog driver for Broadcom Set-Top Box
Hi Justin,
On Thu, Aug 27, 2015 at 03:53:24PM -0700, Justin Chen wrote:
> Watchdog driver for Broadcom 7038 and newer chips.
>
> Signed-off-by: Justin Chen
Almost good. Two little but important details left ...
> ---
> drivers/watchdog/Kconfig | 8 ++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/bcm7038_wdt.c | 235
> +
> 3 files changed, 244 insertions(+)
> create mode 100644 drivers/watchdog/bcm7038_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 241fafd..4fbe8ab 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1291,6 +1291,14 @@ config BCM_KONA_WDT_DEBUG
>
> If in doubt, say 'N'.
>
> +config BCM7038_WDT
> + tristate "BCM7038 Watchdog"
> + select WATCHDOG_CORE
> + help
> + Watchdog driver for the built-in hardware in Broadcom 7038 SoCs.
> +
> + Say 'Y or 'M' here to enable the driver.
> +
> config IMGPDC_WDT
> tristate "Imagination Technologies PDC Watchdog Timer"
> depends on HAS_IOMEM
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 59ea9a1..65d4169 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
> obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
> obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
> obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
> +obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
>
> # AVR32 Architecture
> obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
> new file mode 100644
> index 000..5e54c1b
> --- /dev/null
> +++ b/drivers/watchdog/bcm7038_wdt.c
> @@ -0,0 +1,235 @@
> +/*
> + * Copyright (C) 2015 Broadcom Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * 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.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define WDT_START_1 0xff00
> +#define WDT_START_2 0x00ff
> +#define WDT_STOP_1 0xee00
> +#define WDT_STOP_2 0x00ee
> +
> +#define WDT_TIMEOUT_REG 0x0
> +#define WDT_CMD_REG 0x4
> +
> +#define WDT_MIN_TIMEOUT 1 /* seconds */
> +#define WDT_DEFAULT_TIMEOUT 30 /* seconds */
> +#define WDT_DEFAULT_RATE 2700
> +
> +struct bcm7038_watchdog {
> + void __iomem*base;
> + struct watchdog_device wdd;
> + u32 rate;
> + struct clk *clk;
> +};
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +
> +static void bcm7038_wdt_set_timeout_reg(struct watchdog_device *wdog)
> +{
> + struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
> + u32 timeout;
> +
> + timeout = wdt->rate * wdog->timeout;
> +
> + writel(timeout, wdt->base + WDT_TIMEOUT_REG);
> +}
> +
> +static int bcm7038_wdt_ping(struct watchdog_device *wdog)
> +{
> + struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
> +
> + writel(WDT_START_1, wdt->base + WDT_CMD_REG);
> + writel(WDT_START_2, wdt->base + WDT_CMD_REG);
> +
> + return 0;
> +}
> +
> +static int bcm7038_wdt_start(struct watchdog_device *wdog)
> +{
> + bcm7038_wdt_set_timeout_reg(wdog);
> + bcm7038_wdt_ping(wdog);
> +
> + return 0;
> +}
> +
> +static int bcm7038_wdt_stop(struct watchdog_device *wdog)
> +{
> + struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
> +
> + writel(WDT_STOP_1, wdt->base + WDT_CMD_REG);
> + writel(WDT_STOP_2, wdt->base + WDT_CMD_REG);
> +
> + return 0;
> +}
> +
> +static int bcm7038_wdt_set_timeout(struct watchdog_device *wdog,
> +unsigned int t)
> +{
> + /* Can't modify timeout value if watchdog timer is running */
> + bcm7038_wdt_stop(wdog);
> + wdog->timeout = t;
> + bcm7038_wdt_start(wdog);
> +
> + return 0;
> +}
> +
> +static unsigned int bcm7038_wdt_get_timeleft(struct watchdog_device *wdog)
> +{
> + struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
> + u32 time_left;
> +
> + time_left = readl(wdt->base + WDT_CMD_REG);
> +
> + return time_left / wdt->rate;
> +}
> +
> +static struct watchdog_info bcm7038_wdt_info = {
> + .identity = "Broadcom BCM7038 Watchdog Timer",
> + .options= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> + WDIOF_MAGICCLOSE
> +};
> +
> +static struct watc
Re: [PATCH 1/2] watchdog: bcm7038: add device tree binding documentation
Justin, On Tue, Aug 25, 2015 at 10:55:40AM -0700, Justin Chen wrote: > > Hello Guenter, > > > Is 'clock-frequency' really needed (and useful), or would it make more sense > > to expect the user to configure a fixed clock if nothing else is available ? > > How do other drivers handle this ? > > The reason for 'clock-frequency' was for a case where our device tree did not > have clocks. Creating a new fixed clock for a single device seems unnecessary > compared to a 'clock-frequency' property. Their is a use for > 'clock-frequency', > but it is not really necessary. However, this is my first linux patch, so I > do > not fully trust my judgement on this... > All that is needed for a fixed clock is a devicetree entry for it. Not sure I understand your line of argument; you add a lot of complexity and code just to avoid those few lines in the dts file (especially with 500+ "fixed-clock" nodes in other devicetree files). Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] watchdog: bcm7038: add device tree binding documentation
Hi Justin,
On 08/20/2015 10:41 AM, Justin Chen wrote:
Add device tree binding docmentation for the watchdog hardware block
on bcm7038 and newer SoCs.
Signed-off-by: Justin Chen
---
.../devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt | 19 +++
1 file changed, 19 insertions(+)
create mode 100644
Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt
diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt
b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt
new file mode 100644
index 000..adb8260
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.txt
@@ -0,0 +1,19 @@
+BCM7038 Watchdog timer
+
+Required properties:
+
+- compatible : should be "brcm,bcm7038-wdt"
+- reg : Specifies base physical address and size of the registers.
+
+Optional properties:
+
+- clocks: the clock running the watchdog
+- clock-frequency: the rate of the clock
Is 'clock-frequency' really needed (and useful), or would it make more sense
to expect the user to configure a fixed clock if nothing else is available ?
How do other drivers handle this ?
Thanks,
Guenter
+
+Example:
+
+watchdog {
+ compatible = "brcm,bcm7038-wdt";
+ clocks = <&upg_fixed>;
+ reg = <0xf040a7e8 0x16>;
+};
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] watchdog: Watchdog driver for Broadcom Set-Top Box
Hi Justin,
On 08/20/2015 10:41 AM, Justin Chen wrote:
Watchdog driver for Broadcom 7038 and newer chips.
Signed-off-by: Justin Chen
Looks pretty good. Couple of comments below.
Thanks,
Guenter
---
drivers/watchdog/Kconfig | 8 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/bcm7038_wdt.c | 253 +
3 files changed, 262 insertions(+)
create mode 100644 drivers/watchdog/bcm7038_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 241fafd..4fbe8ab 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1291,6 +1291,14 @@ config BCM_KONA_WDT_DEBUG
If in doubt, say 'N'.
+config BCM7038_WDT
+ tristate "BCM7038 Watchdog"
+ select WATCHDOG_CORE
+ help
+Watchdog driver for the built-in hardware in Broadcom 7038 SoCs.
+
+Say 'Y or 'M' here to enable the driver.
+
config IMGPDC_WDT
tristate "Imagination Technologies PDC Watchdog Timer"
depends on HAS_IOMEM
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 59ea9a1..65d4169 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
+obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
Can you try to insert this in alphabetic order ?
# AVR32 Architecture
obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
new file mode 100644
index 000..a70730b
--- /dev/null
+++ b/drivers/watchdog/bcm7038_wdt.c
@@ -0,0 +1,253 @@
+/*
+ * Copyright (C) 2015 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * 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.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
Please order include files in alphabetic order.
+#define WDT_START_10xff00
+#define WDT_START_20x00ff
+#define WDT_STOP_1 0xee00
+#define WDT_STOP_2 0x00ee
+
+#define WDT_TIMEOUT_REG0x0
+#define WDT_CMD_REG0x4
+
+#define WDT_MIN_TIMEOUT1 /* seconds */
+#define WDT_DEFAULT_TIMEOUT30 /* seconds */
+#define WDT_DEFAULT_RATE 2700
+
+struct bcm7038_watchdog {
+ void __iomem*reg;
'base' would be a better name for this variable.
+ struct clk *wdt_clk;
+ struct watchdog_device wdd;
+ u32 hz;
How about "rate" ?
+};
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+
+static unsigned long bcm7038_wdt_get_rate(struct bcm7038_watchdog *wdt)
+{
+ /* if clock is missing return hz */
+ if (!wdt->wdt_clk)
+ return wdt->hz;
+
+ return clk_get_rate(wdt->wdt_clk);
+
Unnecessary empty line.
This is unnecessary complex. See below.
+}
+
+static void bcm7038_wdt_set_timeout_reg(struct watchdog_device *wdog)
+{
+ struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
+ u32 timeout;
+
+ timeout = bcm7038_wdt_get_rate(wdt) * wdog->timeout;
+
+ writel(timeout, wdt->reg + WDT_TIMEOUT_REG);
+}
+
+static int bcm7038_wdt_ping(struct watchdog_device *wdog)
+{
+ struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
+
+ writel(WDT_START_1, wdt->reg + WDT_CMD_REG);
+ writel(WDT_START_2, wdt->reg + WDT_CMD_REG);
+
+ return 0;
+}
+
+static int bcm7038_wdt_start(struct watchdog_device *wdog)
+{
+ bcm7038_wdt_set_timeout_reg(wdog);
+ bcm7038_wdt_ping(wdog);
+
+ return 0;
+}
+
+static int bcm7038_wdt_stop(struct watchdog_device *wdog)
+{
+ struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
+
+ writel(WDT_STOP_1, wdt->reg + WDT_CMD_REG);
+ writel(WDT_STOP_2, wdt->reg + WDT_CMD_REG);
+
+ return 0;
+}
+
+static int bcm7038_wdt_set_timeout(struct watchdog_device *wdog,
+ unsigned int t)
Please align continuation lines to '('.
+{
+ if (watchdog_timeout_invalid(wdog, t))
+ return -EINVAL;
+
Unnecessary; checked by infrastructure.
+ /* Can't modify timeout value if watchdog timer is running */
+ bcm7038_wdt_stop(wdog);
+ wdog->timeout = t;
+ bcm7038_wdt_start(wdog);
+
+ return 0;
+}
+
+static unsigned int bcm7038_wdt_get_timeleft(struct watchdog_device *wdog)
Re: [PATCH v6 2/2] Documentation: dt: binding: atmel-sama5d4-wdt: for SAMA5D4 watchdog driver
On 08/06/2015 03:17 AM, Wenyou Yang wrote:
The compatible "atmel,sama5d4-wdt" supports the SAMA5D4 watchdog driver
and the watchdog's WDT_MR register can be written more than once.
Signed-off-by: Wenyou Yang
Reviewed-by: Guenter Roeck
---
.../bindings/watchdog/atmel-sama5d4-wdt.txt| 35
1 file changed, 35 insertions(+)
create mode 100644
Documentation/devicetree/bindings/watchdog/atmel-sama5d4-wdt.txt
diff --git a/Documentation/devicetree/bindings/watchdog/atmel-sama5d4-wdt.txt
b/Documentation/devicetree/bindings/watchdog/atmel-sama5d4-wdt.txt
new file mode 100644
index 000..f7cc7c0
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/atmel-sama5d4-wdt.txt
@@ -0,0 +1,35 @@
+* Atmel SAMA5D4 Watchdog Timer (WDT) Controller
+
+Required properties:
+- compatible: "atmel,sama5d4-wdt"
+- reg: base physical address and length of memory mapped region.
+
+Optional properties:
+- timeout-sec: watchdog timeout value (in seconds).
+- interrupts: interrupt number to the CPU.
+- atmel,watchdog-type: should be "hardware" or "software".
+ "hardware": enable watchdog fault reset. A watchdog fault triggers
+ watchdog reset.
+ "software": enable watchdog fault interrupt. A watchdog fault asserts
+ watchdog interrupt.
+- atmel,idle-halt: present if you want to stop the watchdog when the CPU is
+ in idle state.
+ CAUTION: This property should be used with care, it actually makes the
+ watchdog not counting when the CPU is in idle state, therefore the
+ watchdog reset time depends on mean CPU usage and will not reset at all
+ if the CPU stop working while it is in idle state, which is probably
+ not what you want.
+- atmel,dbg-halt: present if you want to stop the watchdog when the CPU is
+ in debug state.
+
+Example:
+ watchdog@fc068640 {
+ compatible = "atmel,sama5d4-wdt";
+ reg = <0xfc068640 0x10>;
+ interrupts = <4 IRQ_TYPE_LEVEL_HIGH 5>;
+ timeout-sec = <10>;
+ atmel,watchdog-type = "hardware";
+ atmel,dbg-halt;
+ atmel,idle-halt;
+ status = "okay";
+ };
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/2] drivers: watchdog: add a driver to support SAMA5D4 watchdog timer
On 08/06/2015 03:16 AM, Wenyou Yang wrote: From SAMA5D4, the watchdog timer is upgrated with a new feature, which is describled as in the datasheet, "WDT_MR can be written until a LOCKMR command is issued in WDT_CR". That is to say, as long as the bootstrap and u-boot don't issue a LOCKMR command, WDT_MR can be written more than once in the driver. So the SAMA5D4 watchdog driver's implementation is different from the at91sam9260 watchdog driver implemented in file at91sam9_wdt.c. The user application open the device file to enable the watchdog timer hardware, and close to disable it, and set the watchdog timer timeout by seting WDV and WDD fields of WDT_MR register, and ping the watchdog by issuing WDRSTT command to WDT_CR register with hard-coded key. Signed-off-by: Wenyou Yang Reviewed-by: Guenter Roeck -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/2] drivers: watchdog: add a driver to support SAMA5D4 watchdog timer
Hi,
On 08/06/2015 01:34 AM, Wenyou Yang wrote:
From SAMA5D4, the watchdog timer is upgrated with a new feature,
Where does the additional ">" come from ?
which is describled as in the datasheet, "WDT_MR can be written
until a LOCKMR command is issued in WDT_CR".
That is to say, as long as the bootstrap and u-boot don't issue
a LOCKMR command, WDT_MR can be written more than once in the driver.
So the SAMA5D4 watchdog driver's implementation is different from
the at91sam9260 watchdog driver implemented in file at91sam9_wdt.c.
The user application open the device file to enable the watchdog timer
hardware, and close to disable it, and set the watchdog timer timeout
by seting WDV and WDD fields of WDT_MR register, and ping the watchdog
by issuing WDRSTT command to WDT_CR register with hard-coded key.
Signed-off-by: Wenyou Yang
---
drivers/watchdog/Kconfig|9 ++
drivers/watchdog/Makefile |1 +
drivers/watchdog/at91sam9_wdt.h |2 +
drivers/watchdog/sama5d4_wdt.c | 280 +++
4 files changed, 292 insertions(+)
create mode 100644 drivers/watchdog/sama5d4_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index e5e7c55..47ad39a 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -167,6 +167,15 @@ config AT91SAM9X_WATCHDOG
Watchdog timer embedded into AT91SAM9X and AT91CAP9 chips. This will
reboot your system when the timeout is reached.
+config SAMA5D4_WATCHDOG
+ tristate "Atmel SAMA5D4 Watchdog Timer"
+ depends on ARCH_AT91
+ select WATCHDOG_CORE
+ help
+ Atmel SAMA5D4 watchdog timer is embedded into SAMA5D4 chips.
+ Its Watchdog Timer Mode Register can be written more than once.
+ This will reboot your system when the timeout is reached.
+
config CADENCE_WATCHDOG
tristate "Cadence Watchdog Timer"
select WATCHDOG_CORE
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 5c19294..f24b820 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_IXP4XX_WATCHDOG) += ixp4xx_wdt.o
obj-$(CONFIG_KS8695_WATCHDOG) += ks8695_wdt.o
obj-$(CONFIG_S3C2410_WATCHDOG) += s3c2410_wdt.o
obj-$(CONFIG_SA1100_WATCHDOG) += sa1100_wdt.o
+obj-$(CONFIG_SAMA5D4_WATCHDOG) += sama5d4_wdt.o
obj-$(CONFIG_DW_WATCHDOG) += dw_wdt.o
obj-$(CONFIG_EP93XX_WATCHDOG) += ep93xx_wdt.o
obj-$(CONFIG_PNX4008_WATCHDOG) += pnx4008_wdt.o
diff --git a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h
index c6fbb2e6..b79a83b 100644
--- a/drivers/watchdog/at91sam9_wdt.h
+++ b/drivers/watchdog/at91sam9_wdt.h
@@ -22,11 +22,13 @@
#define AT91_WDT_MR 0x04/* Watchdog Mode
Register */
#define AT91_WDT_WDV(0xfff << 0) /*
Counter Value */
+#defineAT91_WDT_SET_WDV(x) ((x) & AT91_WDT_WDV)
#define AT91_WDT_WDFIEN (1 << 12) /*
Fault Interrupt Enable */
#define AT91_WDT_WDRSTEN(1 << 13) /*
Reset Processor */
#define AT91_WDT_WDRPROC(1 << 14) /*
Timer Restart */
#define AT91_WDT_WDDIS (1 << 15) /*
Watchdog Disable */
#define AT91_WDT_WDD(0xfff << 16) /*
Delta Value */
+#defineAT91_WDT_SET_WDD(x) (((x) << 16) &
AT91_WDT_WDD)
#define AT91_WDT_WDDBGHLT (1 << 28) /*
Debug Halt */
#define AT91_WDT_WDIDLEHLT (1 << 29) /*
Idle Halt */
diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
new file mode 100644
index 000..a412215
--- /dev/null
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -0,0 +1,280 @@
+/*
+ * Driver for Atmel SAMA5D4 Watchdog Timer
+ *
+ * Copyright (C) 2015 Atmel Corporation
+ *
+ * Licensed under GPLv2.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "at91sam9_wdt.h"
+
+/* minimum and maximum watchdog timeout, in seconds */
+#define MIN_WDT_TIMEOUT1
+#define MAX_WDT_TIMEOUT16
+#define WDT_DEFAULT_TIMEOUTMAX_WDT_TIMEOUT
+
+#define WDT_SEC2TICKS(s) ((s) ? (((s) << 8) - 1) : 0)
+
+struct sama5d4_wdt {
+ struct watchdog_device wdd;
+ void __iomem*reg_base;
+ u32 config;
+};
+
+static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
+static bool nowayout = WATCHDOG_NOWAYOUT;
+
+module_param(wdt_timeout, int, 0);
+MODULE_PARM_DESC(wdt_timeout,
+ "Watchdog timeout in seconds. (default = "
+ __MODULE_STRING(WDT_DEFAULT_TIMEOUT) ")");
+
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout,
+ "Watchdog cannot be stopped once started (default="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
Re: [PATCH v4 1/2] drivers: watchdog: add a driver to support SAMA5D4 watchdog timer
On 08/05/2015 09:59 PM, Wenyou Yang wrote:
From SAMA5D4, the watchdog timer is upgrated with a new feature,
which is describled as in the datasheet, "WDT_MR can be written
until a LOCKMR command is issued in WDT_CR".
That is to say, as long as the bootstrap and u-boot don't issue
a LOCKMR command, WDT_MR can be written more than once in the driver.
So the SAMA5D4 watchdog driver's implementation is different from
the at91sam9260 watchdog driver implemented in file at91sam9_wdt.c.
The user application open the device file to enable the watchdog timer
hardware, and close to disable it, and set the watchdog timer timeout
by seting WDV and WDD fields of WDT_MR register, and ping the watchdog
by issuing WDRSTT command to WDT_CR register with hard-coded key.
Signed-off-by: Wenyou Yang
---
[ ... ]
+
+/* minimum and maximum watchdog timeout, in seconds */
+#defineMIN_WDT_TIMEOUT 1
+#defineMAX_WDT_TIMEOUT 16
+#defineWDT_DEFAULT_TIMEOUT MAX_WDT_TIMEOUT
+
+#defineWDT_SEC2TICKS(s)((s) ? (((s) << 8) - 1) : 0)
+
Why did you replace the spaces after #define with tabs ?
I understand this is done in the at91.h file, but that is bad enough,
it doesn't add any value, and I don't see a reason to do it here.
+
+ if ((wdt->config & AT91_WDT_WDFIEN) && irq) {
+ ret = devm_request_irq(&pdev->dev, irq, sama5d4_wdt_irq_handler,
+ 0, pdev->name, pdev);
I just realized - this interrupt is registered with flags set to 0,
while in the at91sam driver the flags are "IRQF_SHARED | IRQF_IRQPOLL |
IRQF_NO_SUSPEND". Is this different with the new SOC ?
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] drivers: watchdog: add a driver to support SAMA5D4 watchdog timer
On 08/05/2015 01:57 AM, Wenyou Yang wrote:
From SAMA5D4, the watchdog timer is upgrated with a new feature,
which is describled as in the datasheet, "WDT_MR can be written
until a LOCKMR command is issued in WDT_CR".
That is to say, as long as the bootstrap and u-boot don't issue
a LOCKMR command, WDT_MR can be written more than once in the driver.
So the SAMA5D4 watchdog driver's implementation is different from
the at91sam9260 watchdog driver implemented in file at91sam9_wdt.c.
The user application open the device file to enable the watchdog timer
hardware, and close to disable it, and set the watchdog timer timeout
by seting WDV and WDD fields of WDT_MR register, and ping the watchdog
by issuing WDRSTT command to WDT_CR register with hard-coded key.
Signed-off-by: Wenyou Yang
---
drivers/watchdog/Kconfig|9 ++
drivers/watchdog/Makefile |1 +
drivers/watchdog/at91_sama5d4_wdt.c | 279 +++
drivers/watchdog/at91sam9_wdt.h |2 +
4 files changed, 291 insertions(+)
create mode 100644 drivers/watchdog/at91_sama5d4_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index e5e7c55..4ce8346 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -152,6 +152,15 @@ config ARM_SP805_WATCHDOG
ARM Primecell SP805 Watchdog timer. This will reboot your system when
the timeout is reached.
+config AT91_SAMA5D4_WATCHDOG
Looking into the chip ordering documentation. The chip goes by ATSAMA5D4,
while the other chips go by AT91SAM9xxx.
So I think ATSAMA5D4 would be better (same for the driver name).
Couple of additional nitpicks below.
Thanks,
Guenter
+ tristate "Atmel SAMA5D4 Watchdog Timer"
+ depends on ARCH_AT91
+ select WATCHDOG_CORE
+ help
+ Atmel SAMA5D4 watchdog timer is embedded into SAMA5D4 chips.
+ Its Watchdog Timer Mode Register can be written more than once.
+ This will reboot your system when the timeout is reached.
+
config AT91RM9200_WATCHDOG
tristate "AT91RM9200 watchdog"
depends on SOC_AT91RM9200 && MFD_SYSCON
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 5c19294..c57569c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o
# ARM Architecture
obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o
+obj-$(CONFIG_AT91_SAMA5D4_WATCHDOG) += at91_sama5d4_wdt.o
obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o
obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o
obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o
diff --git a/drivers/watchdog/at91_sama5d4_wdt.c
b/drivers/watchdog/at91_sama5d4_wdt.c
new file mode 100644
index 000..f2e1995
--- /dev/null
+++ b/drivers/watchdog/at91_sama5d4_wdt.c
@@ -0,0 +1,279 @@
+/*
+ * Driver for Atmel SAMA5D4 Watchdog Timer
+ *
+ * Copyright (C) 2015 Atmel Corporation
+ *
+ * Licensed under GPLv2.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "at91sam9_wdt.h"
+
+/* minimum and maximum watchdog timeout, in seconds */
+#define MIN_WDT_TIMEOUT1
+#define MAX_WDT_TIMEOUT16
+#define WDT_DEFAULT_TIMEOUTMAX_WDT_TIMEOUT
+
+#define WDT_SEC2TICKS(s) ((s) ? (((s) << 8) - 1) : 0)
+
+struct atmel_wdt {
If you don't mind, please use "sama5d4" as prefix here and for function names.
+ struct watchdog_device wdd;
+ void __iomem*reg_base;
+ u32 config;
+};
+
+static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
+static bool nowayout = WATCHDOG_NOWAYOUT;
+
+module_param(wdt_timeout, int, 0);
+MODULE_PARM_DESC(wdt_timeout,
+ "Watchdog wdt_timeout in seconds. (default = "
+ __MODULE_STRING(WDT_DEFAULT_TIMEOUT) ")");
+
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout,
+ "Watchdog cannot be stopped once started (default="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+#define wdt_read(wdt, field) \
+ readl_relaxed((wdt)->reg_base + (field))
+
+#define wdt_write(wtd, field, val) \
+ writel_relaxed((val), (wdt)->reg_base + (field))
+
+static int atmel_wdt_start(struct watchdog_device *wdd)
+{
+ struct atmel_wdt *wdt = watchdog_get_drvdata(wdd);
+ u32 reg;
+
+ reg = wdt_read(wdt, AT91_WDT_MR);
+ reg &= ~AT91_WDT_WDDIS;
+ wdt_write(wdt, AT91_WDT_MR, reg);
+
+ return 0;
+}
+
+static int atmel_wdt_stop(struct watchdog_device *wdd)
+{
+ struct atmel_wdt *wdt = watchdog_get_drvdata(wdd);
+ u32 reg;
+
+ reg = wdt_read(wdt, AT91_WDT_MR);
+ reg |= AT91_WDT_WDDIS;
+ wdt_write(wdt, AT91_WDT_MR, reg);
+
+ return 0;
+}
+
+static int atmel_wdt_ping(struct watchdog_device *wdd)
+{
+ struct atmel_wdt *wdt = watchdog_get_drvdata(wdd);
+
+ wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
+
+ return 0;
+}
+
+static
Re: [PATCH v3] thermal: Fix thermal_zone_of_sensor_register to match documentation
On 08/05/2015 02:57 AM, Punit Agrawal wrote:
thermal_zone_of_sensor_register is documented as returning a pointer
to either a valid thermal_zone_device on success, or a corresponding
ERR_PTR() value.
In contrast, the function returns NULL when THERMAL_OF is configured
off. Fix this.
Signed-off-by: Punit Agrawal
Cc: Guenter Roeck
Cc: Eduardo Valentin
Cc: Zhang Rui
Acked-by: Guenter Roeck
---
Hi Guenter,
It was pointed out that ENOSYS is frowned upon for anything other than
indicating lack of support for a syscall. The rest of the functions in
the file use ENODEV. Updating thermal_zone_of_sensor_register to do
the same.
Could you please re-ack if you're ok with this change?
Thanks,
Punit
include/linux/thermal.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 037e9df..f344e51 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -364,7 +364,7 @@ static inline struct thermal_zone_device *
thermal_zone_of_sensor_register(struct device *dev, int id, void *data,
const struct thermal_zone_of_device_ops *ops)
{
- return NULL;
+ return ERR_PTR(-ENODEV);
}
static inline
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] drivers: watchdog: add a driver to support SAMA5D4 watchdog timer
On 08/04/2015 07:35 PM, Wenyou Yang wrote: From SAMA5D4, the watchdog timer is upgrated with a new feature, which is describled as in the datasheet, "WDT_MR can be written until a LOCKMR command is issued in WDT_CR". That is to say, as long as the bootstrap and u-boot don't issue a LOCKMR command, WDT_MR can be written more than once in the driver. So the SAMA5D4 watchdog driver's implementation is different from the previous: the user application open the device file to enable Which previous ? Please let the reader know which one you refer to. the watchdog timer hardware, and close to disable it, and set the watchdog timer timeout by seting WDV and WDD fields of WDT_MR register, and ping the watchdog by issuing WDRSTT command to WDT_CR register with hard-coded key. Signed-off-by: Wenyou Yang --- drivers/watchdog/Kconfig|9 ++ drivers/watchdog/Makefile |1 + drivers/watchdog/at91sam9_wdt.h |4 + drivers/watchdog/atmel_wdt.c| 290 +++ 4 files changed, 304 insertions(+) create mode 100644 drivers/watchdog/atmel_wdt.c diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..4425813 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,15 @@ config ARM_SP805_WATCHDOG ARM Primecell SP805 Watchdog timer. This will reboot your system when the timeout is reached. +config ATMEL_WATCHDOG + tristate "Atmel SAMA5D4 Watchdog Timer" + depends on ARCH_AT91 + select WATCHDOG_CORE + help + Atmel watchdog timer embedded into SAMA5D4 chips. Its Watchdog Timer + Mode Register(WDT_MR) can be written more than once. + This will reboot your system when the timeout is reached. + ATMEL is highly generic. Can you find a more specific name ? If this is part of the AT91 family, it could be something like AT91SAMAD54, for example. Same goes for the driver name. config AT91RM9200_WATCHDOG tristate "AT91RM9200 watchdog" depends on SOC_AT91RM9200 && MFD_SYSCON diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 5c19294..c24a8fc 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -30,6 +30,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o # ARM Architecture obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o +obj-$(CONFIG_ATMEL_WATCHDOG) += atmel_wdt.o obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o diff --git a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h index c6fbb2e6..79add4f 100644 --- a/drivers/watchdog/at91sam9_wdt.h +++ b/drivers/watchdog/at91sam9_wdt.h @@ -22,11 +22,15 @@ #define AT91_WDT_MR 0x04/* Watchdog Mode Register */ #define AT91_WDT_WDV(0xfff << 0) /* Counter Value */ +#defineAT91_WDT_WDV_MSK(0xfff) The ( ) is not necessary. AT91_WDT_WDV is already used as mask. You should not need a second one. +#defineAT91_WDT_WDV_(x)(((x) & AT91_WDT_WDV_MSK) << 0) Please no '_' at the end of the macro name; find another name that isn't as close to AT91_WDT_WDV. #define AT91_WDT_WDFIEN (1 << 12) /* Fault Interrupt Enable */ #define AT91_WDT_WDRSTEN(1 << 13) /* Reset Processor */ #define AT91_WDT_WDRPROC(1 << 14) /* Timer Restart */ #define AT91_WDT_WDDIS (1 << 15) /* Watchdog Disable */ #define AT91_WDT_WDD(0xfff << 16) /* Delta Value */ +#defineAT91_WDT_WDD_MSK(0xfff) ( ) is unnecessary. Also, AT91_WDT_WDD is already used as mask. No need to specify a second one. +#defineAT91_WDT_WDD_(x)(((x) & AT91_WDT_WDD_MSK) << 16) Again, please no '_' at the end of the macro name. #define AT91_WDT_WDDBGHLT (1 << 28) /* Debug Halt */ #define AT91_WDT_WDIDLEHLT (1 << 29) /* Idle Halt */ diff --git a/drivers/watchdog/atmel_wdt.c b/drivers/watchdog/atmel_wdt.c new file mode 100644 index 000..e1cdc84 --- /dev/null +++ b/drivers/watchdog/atmel_wdt.c @@ -0,0 +1,290 @@ +/* + * Driver for Atmel SAMA5D4 Watchdog Timer + * + * Copyright (C) 2015 Atmel Corporation + * + * Licensed under GPLv2. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include Please use alphabetic order for include files. That makes it easier to find files, and simplifies later additions. + +#include "at91sam9_wdt.h" + +/* minimum and maximum watchdog timeout, in seconds */ +#define MIN_WDT_TIMEOUT1 +#define MAX_WDT_TIMEO
Re: [PATCH v2 08/10] hwmon: Support registration of thermal zones for SCP temperature sensors
On 08/03/2015 08:22 AM, Punit Agrawal wrote:
Add support to create thermal zones based on the temperature sensors
provided by the SCP. The thermal zones can be defined using the
thermal DT bindings and should refer to the SCP sensor id to select
the sensor.
Signed-off-by: Punit Agrawal
Cc: Jean Delvare
Cc: Guenter Roeck
Cc: Eduardo Valentin
Hi,
assuming you fix up the nitpicks below,
Acked-by: Guenter Roeck
---
drivers/hwmon/scpi-hwmon.c | 103 -
1 file changed, 102 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c
index 7e7e06b..d96a3e5 100644
--- a/drivers/hwmon/scpi-hwmon.c
+++ b/drivers/hwmon/scpi-hwmon.c
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
struct sensor_data {
struct scpi_sensor_info info;
@@ -29,14 +30,39 @@ struct sensor_data {
char label[20];
};
+struct scpi_thermal_zone {
+ struct list_head list;
+ int sensor_id;
+ struct scpi_sensors *scpi_sensors;
+ struct thermal_zone_device *tzd;
+};
+
struct scpi_sensors {
struct scpi_ops *scpi_ops;
struct sensor_data *data;
+ struct list_head thermal_zones;
struct attribute **attrs;
struct attribute_group group;
const struct attribute_group *groups[2];
};
+static int scpi_read_temp(void *dev, long *temp)
+{
+ struct scpi_thermal_zone *zone = dev;
+ struct scpi_sensors *scpi_sensors = zone->scpi_sensors;
+ struct scpi_ops *scpi_ops = scpi_sensors->scpi_ops;
+ struct sensor_data *sensor = &scpi_sensors->data[zone->sensor_id];
+ u32 value;
+ int ret;
+
+ ret = scpi_ops->sensor_get_value(sensor->info.sensor_id, &value);
+ if (ret)
+ return ret;
+
+ *temp = value;
+ return 0;
+}
+
/* hwmon callback functions */
static ssize_t
scpi_show_sensor(struct device *dev, struct device_attribute *attr, char *buf)
@@ -66,6 +92,24 @@ scpi_show_label(struct device *dev, struct device_attribute
*attr, char *buf)
return sprintf(buf, "%s\n", sensor->info.name);
}
+static void
+unregister_thermal_zones(struct platform_device *pdev,
+struct scpi_sensors *scpi_sensors)
+{
+ struct list_head *pos;
+
+ list_for_each(pos, &scpi_sensors->thermal_zones) {
+ struct scpi_thermal_zone *zone;
+
+ zone = list_entry(pos, struct scpi_thermal_zone, list);
+ thermal_zone_of_sensor_unregister(&pdev->dev, zone->tzd);
+ }
+}
+
+static struct thermal_zone_of_device_ops scpi_sensor_ops = {
+ .get_temp = scpi_read_temp,
+};
+
static int scpi_hwmon_probe(struct platform_device *pdev)
{
u16 nr_sensors, i;
@@ -157,10 +201,66 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
scpi_sensors->group.attrs = scpi_sensors->attrs;
scpi_sensors->groups[0] = &scpi_sensors->group;
+ platform_set_drvdata(pdev, scpi_sensors);
+
hwdev = devm_hwmon_device_register_with_groups(dev,
"scpi_sensors", scpi_sensors, scpi_sensors->groups);
- return PTR_ERR_OR_ZERO(hwdev);
+ if (IS_ERR(hwdev))
+ return PTR_ERR(hwdev);
+
+ /*
+* Register the temperature sensors with the thermal framework
+* to allow their usage in setting up the thermal zones from
+* device tree.
+*
+* NOTE: Not all temperature sensors maybe used for thermal
+* control
+*/
+ INIT_LIST_HEAD(&scpi_sensors->thermal_zones);
+ for (i = 0; i < nr_sensors; i++) {
+ struct sensor_data *sensor = &scpi_sensors->data[i];
+ struct scpi_thermal_zone *zone;
+
+ if (sensor->info.class != TEMPERATURE)
+ continue;
+
+ zone = devm_kzalloc(&pdev->dev, sizeof(*zone), GFP_KERNEL);
devm_kzalloc(dev, ...);
for consistency.
+ if (!zone) {
+ ret = -ENOMEM;
+ goto unregister_tzd;
+ }
+
+ zone->sensor_id = i;
+ zone->scpi_sensors = scpi_sensors;
+ zone->tzd = thermal_zone_of_sensor_register(dev, i, zone,
+ &scpi_sensor_ops);
+ /*
+* The call to thermal_zone_of_sensor_register returns
+* an error for sensors that are not associated with
+* any thermal zones.
... or if the thermal subsystem is not configured.
+*/
+ if (IS_ERR(zone->tzd)) {
+ devm_kfree(dev, zone);
+ continue;
+ }
+ list_add(&zone->list, &scpi_sensors
Re: [PATCH v2 07/10] hwmon: Support sensors exported via ARM SCP interface
On 08/03/2015 08:22 AM, Punit Agrawal wrote:
Create a driver to add support for SoC sensors exported by the System
Control Processor (SCP) via the System Control and Power Interface
(SCPI). The supported sensor types is one of voltage, temperature,
current, and power.
The sensor labels and values provided by the SCP are exported via the
hwmon sysfs interface.
Signed-off-by: Punit Agrawal
Cc: Jean Delvare
Cc: Guenter Roeck
Cc: Sudeep Holla
Looks good for the most part. Single comment below.
Assuming you fix it up, feel free to add
Acked-by: Guenter Roeck
for v3.
Thanks,
Guenter
---
Documentation/hwmon/scpi-hwmon | 33
drivers/hwmon/Kconfig | 8 ++
drivers/hwmon/Makefile | 1 +
drivers/hwmon/scpi-hwmon.c | 183 +
4 files changed, 225 insertions(+)
create mode 100644 Documentation/hwmon/scpi-hwmon
create mode 100644 drivers/hwmon/scpi-hwmon.c
diff --git a/Documentation/hwmon/scpi-hwmon b/Documentation/hwmon/scpi-hwmon
new file mode 100644
index 000..4cfcdf2d
--- /dev/null
+++ b/Documentation/hwmon/scpi-hwmon
@@ -0,0 +1,33 @@
+Kernel driver scpi-hwmon
+
+
+Supported chips:
+ * Chips based on ARM System Control Processor Interface
+ Addresses scanned: -
+ Datasheet:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0922b/index.html
+
+Author: Punit Agrawal
+
+Description
+---
+
+This driver supports hardware monitoring for SoC's based on the ARM
+System Control Processor (SCP) implementing the System Control
+Processor Interface (SCPI). The following sensor types are supported
+by the SCP -
+
+ * temperature
+ * voltage
+ * current
+ * power
+
+The SCP interface provides an API to query the available sensors and
+their values which are then exported to userspace by this driver.
+
+Usage Notes
+---
+
+The driver relies on device tree node to indicate the presence of SCPI
+support in the kernel. See
+Documentation/devicetree/bindings/arm/arm,scpi.txt for details of the
+devicetree node.
\ No newline at end of file
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 4943c3c..c9714b0 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1551,6 +1551,14 @@ config SENSORS_VEXPRESS
the ARM Ltd's Versatile Express platform. It can provide wide
range of information like temperature, power, energy.
+config SENSORS_ARM_SCPI
+ tristate "ARM SCPI Sensors"
+ depends on ARM_SCPI_PROTOCOL
+ help
+ This driver provides support for temperature, voltage, current
+ and power sensors available on ARM Ltd's SCP based platforms. The
+ actual number and type of sensors exported depend the platform.
+
config SENSORS_VIA_CPUTEMP
tristate "VIA CPU temperature sensor"
depends on X86
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 8aba87f..4961710 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -150,6 +150,7 @@ obj-$(CONFIG_SENSORS_TMP421)+= tmp421.o
obj-$(CONFIG_SENSORS_TWL4030_MADC)+= twl4030-madc-hwmon.o
obj-$(CONFIG_SENSORS_V2M_JUNO)+= v2m-juno.o
obj-$(CONFIG_SENSORS_VEXPRESS)+= vexpress.o
+obj-$(CONFIG_SENSORS_ARM_SCPI) += scpi-hwmon.o
obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
obj-$(CONFIG_SENSORS_VIA686A) += via686a.o
obj-$(CONFIG_SENSORS_VT1211) += vt1211.o
diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c
new file mode 100644
index 000..7e7e06b
--- /dev/null
+++ b/drivers/hwmon/scpi-hwmon.c
@@ -0,0 +1,183 @@
+/*
+ * System Control and Power Interface(SCPI) based hwmon sensor driver
+ *
+ * Copyright (C) 2015 ARM Ltd.
+ * Punit Agrawal
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct sensor_data {
+ struct scpi_sensor_info info;
+ struct device_attribute dev_attr_input;
+ struct device_attribute dev_attr_label;
+ char input[20];
+ char label[20];
+};
+
+struct scpi_sensors {
+ struct scpi_ops *scpi_ops;
+ struct sensor_data *data;
+ struct attribute **attrs;
+ struct attribute_group group;
+ const struct attribute_group *groups[2];
+};
+
+/* hwmon callback functions */
+static ssize_t
+scpi_show_sensor(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct scpi_sensors *scpi_sensors = dev_get_drvdata(dev);
+ struct scpi_ops *scpi_ops = scpi_
Re: [PATCH v2 04/10] thermal: Fix thermal_zone_of_sensor_register to match documentation
On 08/03/2015 08:22 AM, Punit Agrawal wrote:
thermal_zone_of_sensor_register is documented as returning a pointer
to either a valid thermal_zone_device on success, or a corresponding
ERR_PTR() value.
In contrast, the function returns NULL when THERMAL_OF is configured
off. Fix this.
Signed-off-by: Punit Agrawal
Cc: Guenter Roeck
Cc: Eduardo Valentin
Cc: Zhang Rui
Acked-by: Guenter Roeck
---
include/linux/thermal.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 037e9df..a47b34a 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -364,7 +364,7 @@ static inline struct thermal_zone_device *
thermal_zone_of_sensor_register(struct device *dev, int id, void *data,
const struct thermal_zone_of_device_ops *ops)
{
- return NULL;
+ return ERR_PTR(-ENOSYS);
}
static inline
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] serial: etraxfs-uart: use mctrl_gpio helpers for handling modem signals
On Sat, Jul 25, 2015 at 02:02:46AM +0200, Niklas Cassel wrote: > In order to use the mctrl_gpio helpers, we change the DT bindings: > ri-gpios renamed to rng-gpios. cd-gpios renamed to dcd-gpios. > However, no in-tree dts/dtsi specifies these, so no worries. > > Signed-off-by: Niklas Cassel Acked-by: Guenter Roeck Tested-by: Guenter Roeck Any chance to get this into -next ? The problem it fixes still causes my crisv32 qemu tests to fail. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] hwmon: (nct7802) Add device tree support
Hi Constantine,
On 07/31/2015 03:00 PM, Constantine Shulyupin wrote:
Please add a description of what you are doing here.
Signed-off-by: Constantine Shulyupin
---
The first trial.
Question: how to configure local temp4 (EnLTD)?
Allow "temp4_type = <3>" (EnLTD=3-2=1) or "temp4_enable = <1>" or else?
I don't see a reason to disable it. After all, it is always present.
Please make sure you copy the devicetree mailing list and the devicetree
maintainers for discussing devicetree properties. You can use
scripts/get_maintainer.pl to determine who needs to be copied.
The limited scope of the properties suggests that you might plan to submit
further patches to add more properties. Specifically, the chip also has
configurable voltage sensors, fan status, and fan control, for which
you would probably need properties as well. Splitting patch submission
for the properties into multiple chunks will make it difficult to review
for the devicetree maintainers. I would suggest to determine all required
bindings and submit at least the complete bindings document in one go.
---
.../devicetree/bindings/hwmon/nct7802.txt | 28
.../devicetree/bindings/vendor-prefixes.txt| 1 +
drivers/hwmon/nct7802.c| 52 +-
You should probably split this into three patches.
- Add vendor ID to vendor prefixes
- Add devicetree properties
- Add implementation
3 files changed, 71 insertions(+), 10 deletions(-)
create mode 100644 Documentation/devicetree/bindings/hwmon/nct7802.txt
diff --git a/Documentation/devicetree/bindings/hwmon/nct7802.txt
b/Documentation/devicetree/bindings/hwmon/nct7802.txt
new file mode 100644
index 000..568d3aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/nct7802.txt
@@ -0,0 +1,28 @@
+Nuvoton NCT7802Y Hardware Monitoring IC
+
+Required node properties:
+
+ - "compatible": must be "nuvoton,nct7802"
+ - "reg": I2C bus address of the device
+
+Optional properties:
+
+ - temp1_type
+ - temp2_type
+ - temp3_type
Please use '-' instead of '_', and use full words.
Not sure how to enumerate the different sensors - looking for advice from
devicetree
maintainers.
+
+Valid values:
+
+ 0 - disabled
+ 3 - thermal diode
+ 4 - thermistor
The numbering ties into implementation details (sysfs representation). This is
not desirable for devicetree properties, which are supposed to be OS and
implementation
independent.
It might make sense to use strings here. 'disabled' seems redundant, in a way -
a temperature sensor might be considered disabled if it is not listed.
Another option might be to have a single property, such as
temperature-sensors = <0, 1, 2, 1>;
where each value indicates one of the sensors, with
0 - disabled
1 - diode
2 - thermistor
I don't really have a strong opinion, though. Again looking for advice
from devicetree maintainers.
+
+Example nct7802 node:
+
+nct7802 {
+ compatible = "nuvoton,nct7802";
+ reg = <0x2a>;
+ temp1_type = <4>;
+ temp2_type = <4>;
+ temp3_type = <4>;
+};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt
b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 181b53e..821e000 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -149,6 +149,7 @@ netxeon Shenzhen Netxeon Technology CO., LTD
newhaven Newhaven Display International
nintendo Nintendo
nokia Nokia
+nuvotonNuvoton
nvidiaNVIDIA
nxp NXP Semiconductors
onnn ON Semiconductor Corp.
diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c
index 3ce33d2..2be995d 100644
--- a/drivers/hwmon/nct7802.c
+++ b/drivers/hwmon/nct7802.c
@@ -84,24 +84,30 @@ static ssize_t show_temp_type(struct device *dev, struct
device_attribute *attr,
return sprintf(buf, "%u\n", (mode >> (2 * sattr->index) & 3) + 2);
}
+int set_temp_type(struct nct7802_data *data, int index, u8 type)
+{
+ if (index == 2 && type != 4) /* RD3 */
+ return -EINVAL;
+ if ((type > 0 && type < 3) || type > 4)
+ return -EINVAL;
+ return regmap_update_bits(data->regmap, REG_MODE,
+ 3 << 2 * index,
+ (type ? type - 2 : 0) << 2 * index);
+}
+
static ssize_t store_temp_type(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
struct nct7802_data *data = dev_get_drvdata(dev);
struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
- unsigned int type;
+ u8 type;
int err;
- err = kstrtouint(buf, 0, &type);
+ err = kstrtou8(buf, 0, &type);
if (err < 0)
return err;
- if (sattr->index == 2 && type != 4) /* RD3 */
- return -EINVAL;
-
Re: [RFC] improve binding for linux,wdt-gpio
Hi Uwe, On 07/29/2015 11:59 PM, Uwe Kleine-König wrote: Hello Guenter, On Wed, Jul 29, 2015 at 08:49:23AM -0700, Guenter Roeck wrote: On 07/29/2015 12:35 AM, Uwe Kleine-König wrote: always-running is meant to indicate that the watchdog can not be stopped (meaning a timer has to be used to send keepalives while the watchdog device is closed). The documentation specifically states that. "If the watchdog timer cannot be disabled ..." How would you express that condition without always-running or a similar attribute ? I am also not sure how that relates to hw_algo; I thought those properties are orthogonal. For hw_algo = "level" the inactive level of the gpio disables the watchdog (and resets the counter). So always-running doesn't make sense for this type. That is not what is currently implemented. "level" just means that I'm not talking (yet) about the implementation. the watchdog is pinged using a -high-low-high- or -low-high-low- sequence, while toggle means that the level is changed with each ping (-low-(wait)-high-(wait)-low-(wait)-high-...). Currently the document tells us: hw_algo: [...] - level: Low or high level starts counting WDT timeout, the opposite level disables the WDT. So similar to the "toggle" description there is some behaviour about being disabled in certain states implied. It might be that you put too much emphasis on the documentation and little or none on the code. We already established that the documentation is less than perfect. It might make sense to clean up the documentation first to ensure that it matches what it is actually meant to document. Sure, I understand, the devicetree bindings are supposed to be implementation independent. We all know that this is an ideal view which often does not match reality, as the code tends to be written first and the devicetree bindings tend to be an afterthought. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] improve binding for linux,wdt-gpio
Hi Uwe,
On 07/29/2015 12:35 AM, Uwe Kleine-König wrote:
Hello Guenter,
[ ... ]
always-running is meant to indicate that the watchdog can not be stopped
(meaning a timer has to be used to send keepalives while the watchdog
device is closed). The documentation specifically states that.
"If the watchdog timer cannot be disabled ..."
How would you express that condition without always-running or a similar
attribute ? I am also not sure how that relates to hw_algo; I thought
those properties are orthogonal.
For hw_algo = "level" the inactive level of the gpio disables the
watchdog (and resets the counter). So always-running doesn't make sense
for this type.
That is not what is currently implemented. "level" just means that
the watchdog is pinged using a -high-low-high- or -low-high-low-
sequence, while toggle means that the level is changed with each
ping (-low-(wait)-high-(wait)-low-(wait)-high-...).
Of course, 'always-running' _may_ describe driver behavior, but that doesn't
Well in the current state of the binding document we have:
add this flag to have the driver keep toggling the signal
without a client.
Sure it is meant to describe a hardware specific property, but it talks
about the driver.
Then the fix is to update the binding document.
I'd go for these properties then:
toggle-gpio: the gpio used to stroke the watchdog
'toggle' means something different in the current implementation.
enable-gpio: optional, the gpio to enable and disable the watchdog
disable-on-tri-state: optional, signals that the watchdog can
be stopped by setting the trigger-gpio to tri-state.
compatible, hw_algo and hw_margin_ms: as before.
I would agree that a separate 'enable' property would make sense (if you
have a watchdog needing it). Similar to disable-on-tri-state, if there
is some hardware which is implemented that way (mixing up hw_algo==toggle
with that state doesn't seem correct). Deprecating hw_algo and replacing it
with something more sensible might make sense as well ('algorithm' ?).
We have to be careful not to mix up hw_algo and enable, though.
I think there is no need for a property that signals that the watchdog
is unstoppable. For level-gpio-watchdogs you can do it by setting the
trigger gpio to inactive, and you cannot stop level-gpio-watchdogs
unless enable-gpio or disable-on-tri-state is specified.
If we ever feel the need to describe a gpio watchdog with a input that
starts the device but cannot stop it, I'd suggest to use "start-gpio"
for that one.
have to be the case. There are lots of watchdogs out there which can not be
stopped. Some of them run all the time, others can not be stopped once
started.
Yeah, I'm aware of that. For this driver however I wouldn't expect that
you have a dedicated enable-gpio if you cannot disable the device with it.
Why ? There are lots of chips which implement exactly that. There is an
enable bit in some register which can be used to enable the watchdog,
but once enabled it can not be stopped. I don't see why a gpio driven
watchdog would have to be any different.
For hw_algo = "level" there is probably no device with an enable input
Why should that be the case ?
and for hw_algo = "toggle" any sane hardware engineer would simply
enable the watchdog once the first toggle is detected on WDI. (OK,
assuming hardware engineers being sane turned out to be a weak argument
often in the past.)
I still don't see the relationship between enable and the toggle/level
algorithm. Really, those two properties are orthogonal.
I'm aware that using ...-gpios is more common than ...-gpio. I don't
feel strong here, but as only a single gpio makes sense here, having
-gpios seems wrong.
Documentation/devicetree/bindings/gpio/gpio.txt states that gpio pin
references should be named -gpios. It even lists examples such as
enable-gpios = <&gpio2 2>;
I thought this was a hard rule, and I seem to recall requests to change
something-gpio to something-spios, but I may be wrong.
Yeah, I'm aware of that. I talked about that in #armlinux yesterday, and
Mark Brown (added to Cc:) said:
Well, I'd prefer to change the standard TBH and allow singular.
This keeps coming up and causing confusion for no good reason.
Sounds sensible in my ears.
Makes sense to me, but I'd like to see that done first.
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support
On 07/28/2015 05:38 PM, Yang, Wenyou wrote: Hi Guenter, Thank you very much for your review. -Original Message- From: Guenter Roeck [mailto:li...@roeck-us.net] Sent: 2015年7月28日 15:14 To: Yang, Wenyou; w...@iguana.be; robh...@kernel.org; pawel.m...@arm.com; mark.rutl...@arm.com; ijc+devicet...@hellion.org.uk; ga...@codeaurora.org Cc: sylvain.roc...@finsecur.com; Ferre, Nicolas; boris.brezillon@free- electrons.com; devicetree@vger.kernel.org; linux-ker...@vger.kernel.org; linux- watch...@vger.kernel.org; linux-arm-ker...@lists.infradead.org Subject: Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support On 07/28/2015 12:00 AM, Wenyou Yang wrote: In the datasheet, the new feature is describled as "WDT_MR can be written until a LOCKMR command is issued in WDT_CR". That is to say, as long as the bootstrap and u-boot don't issue a LOCKMR command, WDT_MR can be written in kernel. So the driver can enable/disable the watchdog timer hardware, set WDV(Watchdog Counter Value) and WDD(Watchdog Delta Value) fields of WDT_MR register to set the watchdog timer timeout. The timer is not necessary that regularly sends a keepalive ping to the watchdog timer hardware. It is introduced from sama5d4 SoCs. Since there are so many changes, I wonder is a separate driver would make more sense. Yes, a bit many changes. I thought reuse the driver code. If a separate driver, I am afraid it includes much duplicated code. After all, it is for the same device with different feature. I don't think it is necessary to have multiple drivers for the same peripheral with different feature. The concept for the two mechanisms is all different: In one, the watchdog keepalive is triggered from timer code. In the other, the watchdog timeout is triggered directly from the heartbeat function. One assumes that the watchdog is always running, and that it must be pinged even if closed. The other disables the watchdog on close. What I _can_ see is that the driver is becoming an unmaintainable mess, with lots of if/else in pretty much every function. I consider this much less desirable than a bit of code duplication - if there is any. Seriously, most of the added code might as well be for a completely different chip. Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] improve binding for linux,wdt-gpio
Hi Uwe,
On Tue, Jul 28, 2015 at 10:33:48PM +0200, Uwe Kleine-König wrote:
> This is just a suggestion up to now, I don't have any code yet to share.
>
> Apart from minor rewording to make the document easier to understand and
> less ambiguous the relevant changes are:
>
> - add an "enable-gpio" property.
>I admit the device I'm currently working with doesn't have this.
>Still I imagine this to be a common hardware property. I added it
>mainly to demonstrate the shortcomings of the existing binding.
> - rename "gpios" to "trigger-gpio"
>This is more descriptive. And given there is "enable-gpio" now, too,
>using plain "gpios" seems wrong.
> - deprecate always-running
>Apart from the description describing the driver behaviour and not
>the device property, always-running only seems to make sense in
>combination with hw_algo = "level" and in reality should only
>invalidate the sentence: "The watchdog timer is disabled when GPIO is
>left floating or connected to a three-state buffer."
always-running is meant to indicate that the watchdog can not be stopped
(meaning a timer has to be used to send keepalives while the watchdog
device is closed). The documentation specifically states that.
"If the watchdog timer cannot be disabled ..."
How would you express that condition without always-running or a similar
attribute ? I am also not sure how that relates to hw_algo; I thought
those properties are orthogonal.
Of course, 'always-running' _may_ describe driver behavior, but that doesn't
have to be the case. There are lots of watchdogs out there which can not be
stopped. Some of them run all the time, others can not be stopped once started.
That gets us into the rat-hole of arguing if property X describes driver
behavior or the hardware, and of rejecting properties because they may
be driver descriptions in some use cases (because 'always-running' can
be set even if the hardware doesn't mandate it, making it driver behavior).
I'd rather not go there.
>With this semantic using a property "disable-on-tri-state" sounds
>more sensible. And note that even the following would make sense
>hardware-wise, while the device tree looks stupid:
>
> watchdog {
> compatible = "linux,wdt-gpio";
> trigger-gpio = ...;
> hw_algo = "toggle";
> always-running;
> enable-gpio = ...;
> };
>
>(i.e. always-running, but disable possible by a dedicated gpio.)
>
It might also mean that _enable_ is possible with a dedicated gpio.
That doesn't mean it can be stopped once started. Again, there are lots
of watchdogs out there which can be enabled/started but not stopped.
> I'm aware that using ...-gpios is more common than ...-gpio. I don't
> feel strong here, but as only a single gpio makes sense here, having
> -gpios seems wrong.
>
Documentation/devicetree/bindings/gpio/gpio.txt states that gpio pin
references should be named -gpios. It even lists examples such as
enable-gpios = <&gpio2 2>;
I thought this was a hard rule, and I seem to recall requests to change
something-gpio to something-spios, but I may be wrong.
Thanks,
Guenter
> Also I considered renaming hw_margin_ms and hw_algo to use - instead of
> _. Doing this might even ease to implement the changes above in a
> compatible way. I.e. assume the watchdog can be disabled by tristating
> the gpio if the description uses underscores instead of hyphen.
>
> Feedback very welcome!
>
> Best regards
> Uwe
>
> ---
> .../devicetree/bindings/watchdog/gpio-wdt.txt | 37
> ++
> 1 file changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> index 198794963786..ceb1a5f95f44 100644
> --- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> @@ -2,27 +2,36 @@
>
> Required Properties:
> - compatible: Should contain "linux,wdt-gpio".
> -- gpios: From common gpio binding; gpio connection to WDT reset pin.
> -- hw_algo: The algorithm used by the driver. Should be one of the
> +- trigger-gpio: reference to the gpio connected to watchdog's input pin
> + (typically called WDI).
> +- hw_algo: The algorithm used by the device. Should be one of the
>following values:
> - - toggle: Either a high-to-low or a low-to-high transition clears
> -the WDT counter. The watchdog timer is disabled when GPIO is
> -left floating or connected to a three-state buffer.
> - - level: Low or high level starts counting WDT timeout,
> -the opposite level disables the WDT. Active level is determined
> -by the GPIO flags.
> -- hw_margin_ms: Maximum time to reset watchdog circuit (milliseconds).
> + - toggle: Both a high-to-low and a low-to-high transition clear
> +the watchdog counter.
> + - level: Low or high leve
Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support
On 07/28/2015 12:00 AM, Wenyou Yang wrote:
In the datasheet, the new feature is describled as
"WDT_MR can be written until a LOCKMR command is issued in WDT_CR".
That is to say, as long as the bootstrap and u-boot don't issue a LOCKMR
command, WDT_MR can be written in kernel.
So the driver can enable/disable the watchdog timer hardware,
set WDV(Watchdog Counter Value) and WDD(Watchdog Delta Value) fields
of WDT_MR register to set the watchdog timer timeout.
The timer is not necessary that regularly sends a keepalive ping to
the watchdog timer hardware.
It is introduced from sama5d4 SoCs.
Since there are so many changes, I wonder is a separate driver would make more
sense.
Thoughts ?
Guenter
Signed-off-by: Wenyou Yang
---
drivers/watchdog/at91sam9_wdt.c | 255 ---
drivers/watchdog/at91sam9_wdt.h |4 +
2 files changed, 190 insertions(+), 69 deletions(-)
diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index 1443b3c..6b61084 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -10,9 +10,12 @@
*/
/*
+ * For AT91SAM9x SoCs, the Watchdog Timer has the following constraint.
* The Watchdog Timer Mode Register can be only written to once. If the
* timeout need to be set from Linux, be sure that the bootstrap or the
* bootloader doesn't write to this register.
+ * From SAMA5D4, the Watchdog Timer Mode Register can be written
+ * until a LOCKMR command is issued in WDT_CR.
*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -80,6 +83,11 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once
started "
"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
#define to_wdt(wdd) container_of(wdd, struct at91wdt, wdd)
+
+struct at91wdt_variant {
+ bool mr_writable;
+};
+
struct at91wdt {
struct watchdog_device wdd;
void __iomem *base;
@@ -90,6 +98,9 @@ struct at91wdt {
unsigned long heartbeat;/* WDT heartbeat in jiffies */
bool nowayout;
unsigned int irq;
+ bool use_timer;
+ bool enabled;
+ struct at91wdt_variant *drv_data;
};
/* .
*/
@@ -133,21 +144,67 @@ static void at91_ping(unsigned long data)
static int at91_wdt_start(struct watchdog_device *wdd)
{
struct at91wdt *wdt = to_wdt(wdd);
- /* calculate when the next userspace timeout will be */
- wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
+ u32 reg;
+
+ if (wdt->drv_data->mr_writable) {
+ reg = wdt_read(wdt, AT91_WDT_MR);
+ reg &= ~AT91_WDT_WDDIS;
+ wdt_write(wdt, AT91_WDT_MR, reg);
+ } else {
+ /* calculate when the next userspace timeout will be */
+ wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
+ }
+
return 0;
}
static int at91_wdt_stop(struct watchdog_device *wdd)
{
- /* The watchdog timer hardware can not be stopped... */
+ struct at91wdt *wdt = to_wdt(wdd);
+ u32 reg;
+
+ if (wdt->drv_data->mr_writable) {
+ reg = wdt_read(wdt, AT91_WDT_MR);
+ reg |= AT91_WDT_WDDIS;
+ wdt_write(wdt, AT91_WDT_MR, reg);
+ }
+
+ return 0;
+}
+
+static int at91_wdt_ping(struct watchdog_device *wdd)
+{
+ struct at91wdt *wdt = to_wdt(wdd);
+
+ wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
+
return 0;
}
static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int
new_timeout)
{
- wdd->timeout = new_timeout;
- return at91_wdt_start(wdd);
+ struct at91wdt *wdt = to_wdt(wdd);
+ u32 reg, timeout;
+
+ if (wdt->drv_data->mr_writable) {
+ timeout = secs_to_ticks(new_timeout);
+ if (timeout > WDT_COUNTER_MAX_TICKS)
+ return -EINVAL;
+
+ reg = wdt_read(wdt, AT91_WDT_MR);
+ reg &= ~AT91_WDT_WDV;
+ reg |= AT91_WDT_WDV_(timeout);
+ reg &= ~AT91_WDT_WDD;
+ reg |= AT91_WDT_WDD_(timeout);
+ wdt_write(wdt, AT91_WDT_MR, reg);
+
+ wdd->timeout = new_timeout;
+
+ return 0;
+ } else {
+ wdd->timeout = new_timeout;
+ return at91_wdt_start(wdd);
+ }
}
static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
@@ -161,50 +218,65 @@ static int at91_wdt_init(struct platform_device *pdev,
struct at91wdt *wdt)
unsigned long max_heartbeat;
struct device *dev = &pdev->dev;
- tmp = wdt_read(wdt, AT91_WDT_MR);
- if ((tmp & mask) != (wdt->mr & mask)) {
- if (tmp == WDT_MR_RESET) {
- wdt_write(wdt, AT91_WDT_MR, wdt->mr);
- tmp = wdt_read(wdt, AT91_WDT_MR);
+ if (wdt->drv_data->mr_writable) {
+
Re: [lm-sensors] [PATCH 3/3] hwmon: ads7828: Add devicetree documentation
On 07/22/2015 09:30 AM, Denis Carikli wrote: Signed-off-by: Denis Carikli Acked-by: Guenter Roeck You should probably send this patch to the i2c mailing list. Guenter --- Documentation/devicetree/bindings/i2c/trivial-devices.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt index 00f8652..d77d412 100644 --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt @@ -95,6 +95,8 @@ stm,m41t00Serial Access TIMEKEEPER stm,m41t62Serial real-time clock (RTC) with alarm stm,m41t80M41T80 - SERIAL ACCESS RTC WITH ALARMS taos,tsl2550 Ambient Light Sensor with SMBUS/Two Wire Serial Interface +ti,ads7828 8-Channels, 12-bit ADC +ti,ads7830 8-Channels, 8-bit ADC ti,tsc2003I2C Touch-Screen Controller ti,tmp102 Low Power Digital Temperature Sensor with SMBUS/Two Wire Serial Interface ti,tmp103 Low Power Digital Temperature Sensor with SMBUS/Two Wire Serial Interface -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lm-sensors] [PATCH 2/3] hwmon: ads7828: Add devicetree support
On 07/22/2015 09:30 AM, Denis Carikli wrote:
Signed-off-by: Denis Carikli
---
i2c drivers do not need explicit devicetree support.
Guenter
drivers/hwmon/ads7828.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/hwmon/ads7828.c b/drivers/hwmon/ads7828.c
index 6c99ee7..a2d297f 100644
--- a/drivers/hwmon/ads7828.c
+++ b/drivers/hwmon/ads7828.c
@@ -31,6 +31,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -160,6 +161,15 @@ static int ads7828_probe(struct i2c_client *client,
return PTR_ERR_OR_ZERO(hwmon_dev);
}
+#ifdef CONFIG_OF
+static const struct of_device_id ads7828_of_match[] = {
+ { .compatible = "ti,ads7828", .data = (void *) ads7828, },
+ { .compatible = "ti,ads7830", .data = (void *) ads7830, },
+
+};
+MODULE_DEVICE_TABLE(of, ads7828_of_match);
+#endif
+
static const struct i2c_device_id ads7828_device_ids[] = {
{ "ads7828", ads7828 },
{ "ads7830", ads7830 },
@@ -170,6 +180,7 @@ MODULE_DEVICE_TABLE(i2c, ads7828_device_ids);
static struct i2c_driver ads7828_driver = {
.driver = {
.name = "ads7828",
+ .of_match_table = of_match_ptr(ads7828_of_match),
},
.id_table = ads7828_device_ids,
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lm-sensors] [PATCH 1/3] hwmon: lm92: Add devicetree support
On 07/22/2015 09:30 AM, Denis Carikli wrote:
Signed-off-by: Denis Carikli
---
i2c drivers do not need explicit devicetree support.
Guenter
drivers/hwmon/lm92.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/hwmon/lm92.c b/drivers/hwmon/lm92.c
index cfaf70b..a1e10cd 100644
--- a/drivers/hwmon/lm92.c
+++ b/drivers/hwmon/lm92.c
@@ -44,6 +44,7 @@
#include
#include
#include
+#include
#include
/*
@@ -386,6 +387,13 @@ static int lm92_probe(struct i2c_client *new_client,
* Module and driver stuff
*/
+#ifdef CONFIG_OF
+static const struct of_device_id lm92_of_match[] = {
+ { .compatible = "national,lm92", },
+};
+MODULE_DEVICE_TABLE(of, lm92_of_match);
+#endif
+
static const struct i2c_device_id lm92_id[] = {
{ "lm92", 0 },
/* max6635 could be added here */
@@ -397,6 +405,7 @@ static struct i2c_driver lm92_driver = {
.class = I2C_CLASS_HWMON,
.driver = {
.name = "lm92",
+ .of_match_table = of_match_ptr(lm92_of_match),
},
.probe = lm92_probe,
.id_table = lm92_id,
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/9] hwmon: Support registration of thermal zones for SCP temperature sensors
On 07/22/2015 07:02 AM, Punit Agrawal wrote:
Add support to create thermal zones based on the temperature sensors
provided by the SCP. The thermal zones can be defined using the
thermal DT bindings and should refer to the SCP sensor id to select
the sensor.
Signed-off-by: Punit Agrawal
Cc: Jean Delvare
Cc: Guenter Roeck
Cc: Eduardo Valentin
---
drivers/hwmon/scpi-hwmon.c | 53 ++
1 file changed, 53 insertions(+)
diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c
index dd0a6f1..1e52ced 100644
--- a/drivers/hwmon/scpi-hwmon.c
+++ b/drivers/hwmon/scpi-hwmon.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
static struct scpi_ops *scpi_ops;
@@ -33,12 +34,18 @@ struct sensor_dev {
char label[20];
};
+struct scpi_thermal_zone {
+ struct list_head list;
+ struct thermal_zone_device *tzd;
+};
+
struct scpi_sensors {
int num_volt;
int num_temp;
int num_current;
int num_power;
struct sensor_dev *device;
+ struct list_head thermal_zones;
struct device *hwdev;
};
struct scpi_sensors scpi_sensors;
@@ -54,6 +61,20 @@ static int scpi_read_sensor(struct sensor_dev *sensor, u32
*value)
return 0;
}
+static int scpi_read_temp(void *dev, long *temp)
+{
+ struct sensor_dev *sensor = dev;
+ u32 value;
+ int ret;
+
+ ret = scpi_read_sensor(sensor, &value);
+ if (ret)
+ return ret;
+
+ *temp = value;
+ return 0;
+}
+
/* hwmon callback functions */
static ssize_t
scpi_hwmon_show_sensor(struct device *dev,
@@ -90,6 +111,10 @@ struct attribute *scpi_attrs[24] = { 0 };
struct attribute_group scpi_group;
const struct attribute_group *scpi_groups[2] = { 0 };
+struct thermal_zone_of_device_ops scpi_sensor_ops = {
static struct ...
+ .get_temp = scpi_read_temp,
+};
+
static int scpi_hwmon_probe(struct platform_device *pdev)
{
u16 sensors, i;
@@ -108,9 +133,12 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
if (!scpi_sensors.device)
return -ENOMEM;
+ INIT_LIST_HEAD(&scpi_sensors.thermal_zones);
+
dev_info(&pdev->dev, "Found %d sensors\n", sensors);
for (i = 0; i < sensors; i++) {
struct sensor_dev *dev = &scpi_sensors.device[i];
+ struct scpi_thermal_zone *zone;
ret = scpi_ops->sensor_get_info(i, &dev->info);
if (ret) {
@@ -130,6 +158,20 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
snprintf(dev->label, 20,
"temp%d_label", scpi_sensors.num_temp);
scpi_sensors.num_temp++;
+
+ zone = devm_kmalloc(&pdev->dev, sizeof(*zone),
+ GFP_KERNEL);
Please consider using devm_kzalloc().
+ if (!zone)
+ return -ENOMEM;
+
+ zone->tzd = thermal_zone_of_sensor_register(&pdev->dev,
+ i, dev, &scpi_sensor_ops);
+ if (!IS_ERR(zone->tzd))
+ list_add(&zone->list,
+&scpi_sensors.thermal_zones);
+ else
+ devm_kfree(&pdev->dev, zone);
+
I would prefer
if (IS_ERR_OR_NULL(zone->tzd)) {
devm_kfree(&pdev->dev, zone);
break;
}
list_add(&zone->list, &scpi_sensors.thermal_zones);
The code has a problem, though: You don't clean up thermal zones if
there is an error later on in the probe function. Either you need to
implement a cleanup function to be called from an error handler (and
from the remove function), or you need to wait with registering
thermal zones to the very end of the probe function.
Note that thermal_zone_of_sensor_register can return NULL if thermal
zones are not configured, so you have to use IS_ERR_OR_NULL
when checking for errors.
break;
case VOLTAGE:
snprintf(dev->input, 20,
@@ -187,7 +229,18 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
static int scpi_hwmon_remove(struct platform_device *pdev)
{
+ struct list_head *pos;
+
scpi_ops = NULL;
+
+ list_for_each(pos, &scpi_sensors.thermal_zones) {
+ struct scpi_thermal_zone *zone;
+
+ zone = list_entry(pos, struct scpi_thermal_zone, list);
+ thermal_zone_of_sensor_unregister(scpi_sensors.hwdev,
Not sure how this can work. The registration was with &pdev
Re: [PATCH 6/9] hwmon: Support sensors exported via ARM SCP interface
On 07/22/2015 07:02 AM, Punit Agrawal wrote:
Create a driver to add support for SoC sensors exported by the System
Control Processor (SCP) via the System Control and Power Interface
(SCPI). The supported sensor types is one of voltage, temperature,
current, and power.
The sensor labels and values provided by the SCP are exported via the
hwmon sysfs interface.
Signed-off-by: Punit Agrawal
Cc: Jean Delvare
Cc: Guenter Roeck
Cc: Sudeep Holla
---
drivers/hwmon/Kconfig | 8 ++
drivers/hwmon/Makefile | 1 +
drivers/hwmon/scpi-hwmon.c | 212 +
Please also provide Documentation/hwmon/scpi-hwmon.
3 files changed, 221 insertions(+)
create mode 100644 drivers/hwmon/scpi-hwmon.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 4943c3c..f5e0862 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1551,6 +1551,14 @@ config SENSORS_VEXPRESS
the ARM Ltd's Versatile Express platform. It can provide wide
range of information like temperature, power, energy.
+config SENSORS_ARM_SCPI
+ tristate "ARM SCPI Sensors"
+ depends on ARM_SCPI_PROTOCOL
+ help
+ This driver provides support for hardware sensors available on
+ the ARM Ltd's SCP based platforms. It can provide temperature
can provide or provides ?
+ and voltage for range of devices on the SoC.
current, power ?
+
config SENSORS_VIA_CPUTEMP
tristate "VIA CPU temperature sensor"
depends on X86
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 8aba87f..4961710 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -150,6 +150,7 @@ obj-$(CONFIG_SENSORS_TMP421)+= tmp421.o
obj-$(CONFIG_SENSORS_TWL4030_MADC)+= twl4030-madc-hwmon.o
obj-$(CONFIG_SENSORS_V2M_JUNO)+= v2m-juno.o
obj-$(CONFIG_SENSORS_VEXPRESS)+= vexpress.o
+obj-$(CONFIG_SENSORS_ARM_SCPI) += scpi-hwmon.o
obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
obj-$(CONFIG_SENSORS_VIA686A) += via686a.o
obj-$(CONFIG_SENSORS_VT1211) += vt1211.o
diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c
new file mode 100644
index 000..dd0a6f1
--- /dev/null
+++ b/drivers/hwmon/scpi-hwmon.c
@@ -0,0 +1,212 @@
+/*
+ * System Control and Power Interface(SCPI) based hwmon sensor driver
+ *
+ * Copyright (C) 2015 ARM Ltd.
+ * Punit Agrawal
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Not needed since there are no pr_xxx() messages in this driver.
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+static struct scpi_ops *scpi_ops;
+
This variable should be kept in a local data structure (scpi_sensors).
+struct sensor_dev {
+ struct scpi_sensor_info info;
+ struct device_attribute dev_attr_input;
+ struct device_attribute dev_attr_label;
+ char input[20];
+ char label[20];
+};
+
+struct scpi_sensors {
+ int num_volt;
+ int num_temp;
+ int num_current;
+ int num_power;
num_volt, num_temp, num_current, and num_power are not used
outside the probe function and are thus not needed in this structure.
+ struct sensor_dev *device;
'device' is a bit misleading here. It might be better to use
something like sensor_data (and maybe struct sensor_data),
or just 'data'.
+ struct device *hwdev;
hwdev is not used and thus not needed in this structure.
+};
+struct scpi_sensors scpi_sensors;
This variable should be allocated in the probe function (and either case not be
global).
+
+static int scpi_read_sensor(struct sensor_dev *sensor, u32 *value)
+{
+ int ret;
+
+ ret = scpi_ops->sensor_get_value(sensor->info.sensor_id, value);
+ if (ret)
+ return ret;
+
+ return 0;
This can be reduced to
return scpi_ops->sensor_get_value(sensor->info.sensor_id, value);
which makes me wonder if the function is that useful to start with,
but I leave that up to you.
+}
+
+/* hwmon callback functions */
+static ssize_t
+scpi_hwmon_show_sensor(struct device *dev,
+ struct device_attribute *attr, char *buf)
_hwmon in the function names do not really add value.
+{
+ struct sensor_dev *sensor;
+ u32 value;
+ int ret;
+
+ sensor = container_of(attr, struct sensor_dev, dev_attr_input);
+
+ ret = scpi_read_sensor(sensor, &value);
+ i
Re: [PATCH] hwmon: (lm70) add device tree support
On 07/18/2015 03:29 PM, Rabin Vincent wrote: Allow the lm70 to be probed from a device tree. Signed-off-by: Rabin Vincent Applied to -next. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] power: reset: at91: add sama5d3 reset function
On 07/20/2015 02:32 AM, Josh Wu wrote:
This patch introduces a new compatible string: "atmel,sama5d3-rstc" and
new reset function for sama5d3 and later chips.
As in sama5d3 or later chips, we don't have to shutdown the DDR
controller before reset. Shutdown the DDR controller before reset is a
workaround to avoid DDR signal driving the bus, but since sama5d3 and
later chips there is no such a conflict.
So in this patch:
1. the sama5d3 reset function only need to write the rstc register
and return.
2. we can remove the code related with sama5d3 DDR controller as
we don't use it at all.
Signed-off-by: Josh Wu
Acked-by: Nicolas Ferre
Reviewed-by: Guenter Roeck
---
Changes in v2:
- aligned the function parameters to be consist with the coding style
- refined the commit log
- add binding document changes
- use of_device_is_compitable() instead
.../devicetree/bindings/arm/atmel-at91.txt | 2 +-
drivers/power/reset/at91-reset.c | 26 --
2 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt
b/Documentation/devicetree/bindings/arm/atmel-at91.txt
index 424ac8c..dd998b9 100644
--- a/Documentation/devicetree/bindings/arm/atmel-at91.txt
+++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
@@ -87,7 +87,7 @@ One interrupt per TC channel in a TC block:
RSTC Reset Controller required properties:
- compatible: Should be "atmel,-rstc".
- can be "at91sam9260" or "at91sam9g45"
+ can be "at91sam9260" or "at91sam9g45" or "sama5d3"
- reg: Should contain registers location and length
Example:
diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
index 36dc52f..c378d4e 100644
--- a/drivers/power/reset/at91-reset.c
+++ b/drivers/power/reset/at91-reset.c
@@ -123,6 +123,15 @@ static int at91sam9g45_restart(struct notifier_block
*this, unsigned long mode,
return NOTIFY_DONE;
}
+static int sama5d3_restart(struct notifier_block *this, unsigned long mode,
+ void *cmd)
+{
+ writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST |
AT91_RSTC_PROCRST),
+ at91_rstc_base);
+
+ return NOTIFY_DONE;
+}
+
static void __init at91_reset_status(struct platform_device *pdev)
{
u32 reg = readl(at91_rstc_base + AT91_RSTC_SR);
@@ -155,13 +164,13 @@ static void __init at91_reset_status(struct
platform_device *pdev)
static const struct of_device_id at91_ramc_of_match[] = {
{ .compatible = "atmel,at91sam9260-sdramc", },
{ .compatible = "atmel,at91sam9g45-ddramc", },
- { .compatible = "atmel,sama5d3-ddramc", },
{ /* sentinel */ }
};
static const struct of_device_id at91_reset_of_match[] = {
{ .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart },
{ .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart },
+ { .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart },
{ /* sentinel */ }
};
@@ -181,13 +190,16 @@ static int at91_reset_of_probe(struct platform_device
*pdev)
return -ENODEV;
}
- for_each_matching_node(np, at91_ramc_of_match) {
- at91_ramc_base[idx] = of_iomap(np, 0);
- if (!at91_ramc_base[idx]) {
- dev_err(&pdev->dev, "Could not map ram controller
address\n");
- return -ENODEV;
+ if (!of_device_is_compatible(pdev->dev.of_node, "atmel,sama5d3-rstc")) {
+ /* we need to shutdown the ddr controller, so get ramc base */
+ for_each_matching_node(np, at91_ramc_of_match) {
+ at91_ramc_base[idx] = of_iomap(np, 0);
+ if (!at91_ramc_base[idx]) {
+ dev_err(&pdev->dev, "Could not map ram controller
address\n");
+ return -ENODEV;
+ }
+ idx++;
}
- idx++;
}
match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] iio: temperature: add mcp98xx driver support
On Sun, Jul 19, 2015 at 10:24:53AM +0100, Jonathan Cameron wrote: > On 19/07/15 04:04, Matt Ranostay wrote: > > This changeset adds driver support for the Microchip mcp98xx series of > > temperature sensors. > > MCP98xx is pretty a pretty far reaching claim. This could also be MCP9804 or MCP9843, which are JC42 compatible sensor chips and supported by the jc42 driver. Does the new driver claim to support those as well ? Guenter > > This includes temperature reading, and rising/falling threshold events. > Why an IIO driver? These parts already look to be supported in hwmon by > the lm75 driver. We need a pretty strong reason to contemplate having > support in both subsystems... > > > > Matt Ranostay (2): > > iio: temperature: DT binding doc for mcp98xx > > iio: temperature: add support for mcp98xx sensors > > > > .../bindings/iio/temperature/mcp98xx.txt | 22 + > > drivers/iio/temperature/Kconfig| 10 + > > drivers/iio/temperature/Makefile | 1 + > > drivers/iio/temperature/mcp98xx.c | 588 > > + > > 4 files changed, 621 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/iio/temperature/mcp98xx.txt > > create mode 100644 drivers/iio/temperature/mcp98xx.c > > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] Watchdog: introduce ARM SBSA watchdog driver
On Mon, Jul 13, 2015 at 05:09:57PM +0800, Fu Wei wrote: > Hi Guenter, > > If you get some time, could you help me on this patchset again? > Great thanks for your help! > I am on vacation this week. Hopefully next week or two weeks from now, depending on my work load. Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] Watchdog: introduce ARM SBSA watchdog driver
On 06/29/2015 09:53 AM, Fu Wei wrote: Hi Guenter, Any suggestion on this v6 patchset, for now , I only got : Problem is that each version of your patchset tends to introduce substantially new or different functionality, meaning the review has to pretty much start from scratch. Right now I just don't have time to do that, so you'll have to be a bit patient. Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [non-pretimeout,4/7] Watchdog: introduce ARM SBSA watchdog driver
On Wed, Jun 24, 2015 at 12:17:19AM +0800, Fu Wei wrote:
> Hi Guenter,
>
> you always can provide help very quickly, thank you very much :-)
>
> On 23 June 2015 at 23:21, Guenter Roeck wrote:
> > On Tue, Jun 23, 2015 at 09:26:35PM +0800, Fu Wei wrote:
> >> Hi Guenter,
> > [ ...]
> >
> >> >
> >> >> + * When the first timeout occurs, WS0(SPI or LPI) is triggered,
> >> >> + * the second timeout period(as long as the first timeout
> >> >> period) starts.
> >> >
> >> > no longer accurate if WOR is used for the second period.
> >> >
> >> >> + * In WS0 interrupt routine, panic() will be called for
> >> >> collecting
> >> >> + * crashdown info.
> >> >> + * If system can not recover from WS0 interrupt routine, then
> >> >> second
> >> >> + * timeout occurs, WS1(reset or higher level interrupt) is
> >> >> triggered.
> >> >> + * The two timeout period can be set by WOR(32bit).
> >> >
> >> > The second timeout period is determined by ...
> >> >
> >> >> + * WOR gives a maximum watch period of around 10s at the maximum
> >> >> + * system counter frequency.
> >> >> + * The System Counter shall run at maximum of 400MHz.
> >> >
> >> > "... at the maximum system counter frequency of 400 MHz.", and drop the
> >> > last sentence.
> >>
> >> For the second timeout period, I have discussed with a kdump developers,
> >> (1)10s maybe not good enough for all the case of panic + kdump, so
> >> maybe we still need to use WCV in the second timeout period
> >> (2)in the second timeout period, maybe we need to programme WCV for
> >> two reason: a, trigger WS1 to reboot system ASAP; b, feed the watchdog
> >> without cleanning WS0 flag.
> >>
> >> WHY we want to feed the watchdog (keepalive) without cleanning WS0 flag??
> >> REASON:
> >> (1)if the system context is large, we may need to feed the dog until
> >> we get all the things backed up.
> >> (2)if system goes wrong, WS0 triggered, then panic--> kdump. if we
> >> feed the dog by WRR or programming WOR, WS0 flag will be cleaned. Once
> >> system goes wrong again, then panic again.
> >> So this system will be in a panic--kdump--panic--kdump loop, have not
> >> chance to reset.
> >>
> >> So if we are in the second timeout period, we may need to always programme
> >> WCV.
> >>
> > The crashdump kernel is supposed to reload the watchdog driver, which will
> > ping
> > the watchdog. If it isn't able to do that in 10 seconds, something is wrong.
>
> yes, 10s maybe not enough for all case.
> When I tested kdump on arm64, sometimes , it took 20s. So I am
> thinking : can we make the max value of pretimeout > 10s in this
> driver.
>
It takes more than 10 seconds to load the crashdump kernel,
or it takes more than 10 seconds to complete the dump ?
>
> >
> >> >> +
> >> >> + status = readl_relaxed(gwdt->control_base + SBSA_GWDT_WCS);
> >> >> + if (status & SBSA_GWDT_WCS_WS1) {
> >> >> + dev_warn(dev, "System reset by WDT(WCV: %llx)\n",
> >> >> + sbsa_gwdt_get_wcv(wdd));
> >> >
> >> > WCV here only tells us how many clock cycles were executed since the
> >> > system started (or something like that). So I still don't understand
> >> > why it is valuable to print that number.
> >>
> >> this number provides the time of system reset, I thinks that may help
> >> admin to analyse the system failure.
> >>
> > It doesn't mean anything to anyone but you since it is not in a well defined
> > time scale.
>
> maybe I should convert it to second?
> I think the original value is better?
>
I think you should drop it.
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [non-pretimeout,4/7] Watchdog: introduce ARM SBSA watchdog driver
On Tue, Jun 23, 2015 at 09:26:35PM +0800, Fu Wei wrote:
> Hi Guenter,
[ ...]
> >
> >> + * When the first timeout occurs, WS0(SPI or LPI) is triggered,
> >> + * the second timeout period(as long as the first timeout period)
> >> starts.
> >
> > no longer accurate if WOR is used for the second period.
> >
> >> + * In WS0 interrupt routine, panic() will be called for collecting
> >> + * crashdown info.
> >> + * If system can not recover from WS0 interrupt routine, then second
> >> + * timeout occurs, WS1(reset or higher level interrupt) is
> >> triggered.
> >> + * The two timeout period can be set by WOR(32bit).
> >
> > The second timeout period is determined by ...
> >
> >> + * WOR gives a maximum watch period of around 10s at the maximum
> >> + * system counter frequency.
> >> + * The System Counter shall run at maximum of 400MHz.
> >
> > "... at the maximum system counter frequency of 400 MHz.", and drop the
> > last sentence.
>
> For the second timeout period, I have discussed with a kdump developers,
> (1)10s maybe not good enough for all the case of panic + kdump, so
> maybe we still need to use WCV in the second timeout period
> (2)in the second timeout period, maybe we need to programme WCV for
> two reason: a, trigger WS1 to reboot system ASAP; b, feed the watchdog
> without cleanning WS0 flag.
>
> WHY we want to feed the watchdog (keepalive) without cleanning WS0 flag??
> REASON:
> (1)if the system context is large, we may need to feed the dog until
> we get all the things backed up.
> (2)if system goes wrong, WS0 triggered, then panic--> kdump. if we
> feed the dog by WRR or programming WOR, WS0 flag will be cleaned. Once
> system goes wrong again, then panic again.
> So this system will be in a panic--kdump--panic--kdump loop, have not
> chance to reset.
>
> So if we are in the second timeout period, we may need to always programme
> WCV.
>
The crashdump kernel is supposed to reload the watchdog driver, which will ping
the watchdog. If it isn't able to do that in 10 seconds, something is wrong.
> >> +
> >> + status = readl_relaxed(gwdt->control_base + SBSA_GWDT_WCS);
> >> + if (status & SBSA_GWDT_WCS_WS1) {
> >> + dev_warn(dev, "System reset by WDT(WCV: %llx)\n",
> >> + sbsa_gwdt_get_wcv(wdd));
> >
> > WCV here only tells us how many clock cycles were executed since the
> > system started (or something like that). So I still don't understand
> > why it is valuable to print that number.
>
> this number provides the time of system reset, I thinks that may help
> admin to analyse the system failure.
>
It doesn't mean anything to anyone but you since it is not in a well defined
time scale. Also, I would be somewhat surprised if WCV would retain its value
on reset. Much more likely it is the time (in clock cycles) since reset.
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/44] kernel: Add support for poweroff handler call chain
On Wed, Jun 17, 2015 at 06:04:54PM -0700, Stephen Boyd wrote: [ ... ] > > What happened to this series? I want to add shutdown support to my > platform and I need to write a register on the PMIC in one driver to > configure it for shutdown instead of restart and then write an MMIO > register to tell the PMIC to actually do the shutdown in another driver. > It seems that the notifier solves this case for me, albeit with the > slight complication that I need to order the two with some priority. > Can you use the .shutdown driver callback instead ? I see other drivers use that, and check for system_state == SYSTEM_POWER_OFF to power off the hardware. Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/44] kernel: Add support for poweroff handler call chain
On 06/17/2015 11:53 PM, Frans Klaver wrote: On Thu, Jun 18, 2015 at 3:04 AM, Stephen Boyd wrote: On 10/06/2014 10:28 PM, Guenter Roeck wrote: Various drivers implement architecture and/or device specific means to remove power from the system. For the most part, those drivers set the global variable pm_power_off to point to a function within the driver. This mechanism has a number of drawbacks. Typically only one scheme to remove power is supported (at least if pm_power_off is used). At least in theory there can be multiple means remove power, some of which may be less desirable. For example, some mechanisms may only power off the CPU or the CPU card, while another may power off the entire system. Others may really just execute a restart sequence or drop into the ROM monitor. Using pm_power_off can also be racy if the function pointer is set from a driver built as module, as the driver may be in the process of being unloaded when pm_power_off is called. If there are multiple poweroff handlers in the system, removing a module with such a handler may inadvertently reset the pointer to pm_power_off to NULL, leaving the system with no means to remove power. Introduce a system poweroff handler call chain to solve the described problems. This call chain is expected to be executed from the architecture specific machine_power_off() function. Drivers providing system poweroff functionality are expected to register with this call chain. By using the priority field in the notifier block, callers can control poweroff handler execution sequence and thus ensure that the poweroff handler with the optimal capabilities to remove power for a given system is called first. What happened to this series? I want to add shutdown support to my platform and I need to write a register on the PMIC in one driver to configure it for shutdown instead of restart and then write an MMIO register to tell the PMIC to actually do the shutdown in another driver. It seems that the notifier solves this case for me, albeit with the slight complication that I need to order the two with some priority. I was wondering the same thing. I did find out that things kind of stalled after Linus cast doubt on the chosen path [1]. I'm not sure there's any consensus on what would be best to do instead. Linus cast doubt on it, then the maintainers started picking it apart. At the end, trying not to use notifier callbacks made the code so complicated that even I didn't understand it anymore. With no consensus in sight, I abandoned it. Problem is really that the notifier call chain would be perfect to solve the problem, yet Linus didn't like priorities (which are essential), and the power maintainers didn't like that a call chain is supposed to execute _all_ callbacks, which would not be the case here. If I were to start again, I would insist to use notifiers. However, I don't see a chance to get that accepted, so I won't. Feel free to pick it up and give it a try yourself. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/8] of: overlay: Implement indirect target support
On 06/17/2015 05:10 PM, Rob Herring wrote:
On Fri, Jun 12, 2015 at 2:54 PM, Pantelis Antoniou
wrote:
Some applications require applying the same overlay to a different
target according to some external condition (for instance depending
on the slot a card has been inserted, the overlay target is different).
The indirect target use requires using the new
of_overlay_create_indirect() API which uses a text selector.
The format requires the use of a target-indirect node as follows:
fragment@0 {
target-indirect {
foo { target = <&foo_target>; };
bar { target = <&bar_target>; };
};
};
The problem with this is it does not scale. The overlay has to be
changed for every new target. If you had an add-on board (possibly
Not really. For our use case, at least, each target is a slot in the
chassis, so we end up with something like
target-indirect {
slot0 { target = <&sib0i2c>; };
slot1 { target = <&sib1i2c>; };
slot2 { target = <&sib2i2c>; };
slot3 { target = <&sib3i2c>; };
slot4 { target = <&sib4i2c>; };
slot5 { target = <&sib5i2c>; };
slot6 { target = <&sib6i2c>; };
slot7 { target = <&sib7i2c>; };
slot8 { target = <&sib8i2c>; };
};
where sib[0-8]i2c is defined in the master dts file.
Since the number of slots is well defined, the overlay will
always work. Sure, it may have to be updated if it is used in
a chassis with 20 slots, but that doesn't happen that often.
providing an overlay from an EEPROM), you would not want to have to
rebuild overlays with every new host board. It also only handles
translations of where to apply the overlay, but doesn't provide
translations of phandles within the overlay. Say a node that is a
clock or regulator consumer for example. Or am I missing something.
Grant and I discussed this briefly. I think we need a connector
definition in the base dtb which can provide the target for an
overlay. The connector should provide the translation between
connector local signals/buses and host signals/buses. We need to
define what this translation would look like for each binding.
At least for GPIO, we could have something similar to interrupt-map
properties. For example, to map connector gpio 0 to host gpio 66 and
connector gpio 1 to host gpio 44:
gpio-map = <0 &host-gpio 66>, <1 &host-gpio 44>;
We'd need to define how to handle I2C, SPI, regulators, and clocks
minimally. Perhaps rather than trying to apply nodes into the base
dtb, they should be under the connector and the kernel has to learn to
not just look for child nodes for various bindings. Just thinking
aloud...
Anything is fine with me, as long as it is usable (and does not require
us to create 9 overlay files for sib[0-8] from the example above).
The real tricky part is pci, where it is not just about simple target
redirection but irq numbers, memory address ranges, and bus number ranges.
It would be quite useful to have a workable solution for that as well,
but at least I don't have an idea how it could be done.
Talking about connector ...
Right now we have something like
sib0 {
compatible = "jnx,sib-connector", "simple-bus";
... (various properties)
};
Maybe we could use something like the following ?
sib0 {
compatible = "jnx,sib-connector", "simple-bus";
... (various attributes)
ref0 = <&sib0i2c>;
ref1 = <&sib0spi>;
};
and then just reference ref0 and ref1 as targets in the overlay itself ?
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH non-pretimeout 3/7] ARM64: add SBSA Generic Watchdog device node in amd-seattle-soc.dtsi
On 06/14/2015 03:05 AM, Fu Wei wrote: On 13 June 2015 at 04:54, Timur Tabi wrote: On 06/10/2015 12:47 PM, fu@linaro.org wrote: + reg = <0x0 0xe0bb 0 0x1>, + <0x0 0xe0bc 0 0x1>; I think the sizes are wrong. They should be 0x1000 instead of 0x1. This has been proved by test, it works well on Seattle Foundation model has same value. So I don't think it is wrong otherwise someone has the data sheet of Seattle B0, and it shows that is wrong. If only 0x1000 is used, why would you have to reserve 0x1 ? You never access any higher addresses, so no matter what the datasheet says, 0x1000 should be sufficient. What matters is what the driver uses. Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] watchdog: bcm2835: Add poweroff code for the Raspberry Pi
On 06/09/2015 03:21 AM, Noralf Trønnes wrote: This adds a new poweroff function to the watchdog driver for the Raspberry Pi. Currently poweroff/halt results in a reboot. The Raspberry Pi firmware uses the RSTS register to know which partiton to boot from. The partiton value is spread into bits 0, 2, 4, 6, 8, 10. Partiton 63 is a special partition used by the firmware to indicate halt. The firmware made this change in 19 Aug 2013 and was matched by the downstream commit: Changes for new NOOBS multi partition booting from gsh Signed-off-by: Noralf Trønnes This poweroff handler stuff is getting really messy :-(. Nothing we can do about that here, so Acked-by: Guenter Roeck Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] watchdog: bcm2835: Add poweroff code for the Raspberry Pi
On 06/12/2015 04:26 AM, Stefan Wahren wrote:
Hi Noralf,
Am 09.06.2015 um 12:21 schrieb Noralf Trønnes:
This adds a new poweroff function to the watchdog driver for the
Raspberry Pi. Currently poweroff/halt results in a reboot.
[...]
+static void rpi_power_off(void)
+{
+struct device_node *np =
+of_find_compatible_node(NULL, NULL, "brcm,raspberrypi-pm-wdt");
+struct platform_device *pdev = of_find_device_by_node(np);
+struct bcm2835_wdt *wdt = platform_get_drvdata(pdev);
+u32 val;
+
+val = readl_relaxed(wdt->base + PM_RSTS);
do you think it's safe here to assume wdt could never be NULL?
If the call is made, the driver must be instantiated. We can therefore assume
that neither np, pdev, nor wdt is NULL. If one of those is NULL, it would be
a bug, which should not be ignored.
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/8] Watchdog: introdouce "pretimeout" into framework
On Thu, Jun 11, 2015 at 07:22:44PM +0800, Fu Wei wrote:
> Hi Guenter,
>
[ ... ]
>
> > value we are trying to set. Effectively the above accepts every pretimeout
> > if wdd->pretimeout is 0. It also accepts every pretimeout if
> > max_pretimeout == 0, even if wdd->timeout is set and t >= wdd->timeout.
> >
> > Try
> >
> > return (wdd->max_pretimeout && (t < wdd->min_pretimeout ||
> > t > wdd->max_pretimeout)) ||
> > (wdd->timeout && t >= wdd->timeout);
> >
> >> }
>
> /*
> * Use the following function to check if a pretimeout value is invalid.
> * It can be "0", that means we don't use pretimeout.
> * This function returns 0, when pretimeout is 0.
returns false if pretimeout is 0.
> */
> static inline bool watchdog_pretimeout_invalid(struct watchdog_device *wdd,
>unsigned int t)
> {
> return t && (wdd->max_pretimeout &&
> (t < wdd->min_pretimeout || t > wdd->max_pretimeout)) ||
>(wdd->timeout && t >= wdd->timeout);
> }
>
Makes sense. Be careful with (), though. This evaluates to
return t && (...) || (wdd->timeout && t >= wdd->timeout);
but it should probably be
return t && ((...) || (wdd->timeout && t >= wdd->timeout));
That doesn't make a difference in practice (if t == 0, it is never >= timeout
if timeout is > 0), but it would be a bit cleaner.
>
> >>
> >> /* Use the following functions to manipulate watchdog driver specific
> >> data */
> >> @@ -132,11 +153,20 @@ static inline void *watchdog_get_drvdata(struct
> >> watchdog_device *wdd)
> >> }
> >>
> >> /* drivers/watchdog/watchdog_core.c */
> >> -extern int watchdog_init_timeout(struct watchdog_device *wdd,
> >> - unsigned int timeout_parm, struct device
> >> *dev);
> >> +int watchdog_init_timeouts(struct watchdog_device *wdd,
> >> + unsigned int pretimeout_parm,
> >> + unsigned int timeout_parm,
> >> + struct device *dev);
> >
> >
> > Please align continuation lines with '('.
>
> Fixed , thanks
>
> >
> >
> >> extern int watchdog_register_device(struct watchdog_device *);
> >> extern void watchdog_unregister_device(struct watchdog_device *);
> >>
> >> +static inline int watchdog_init_timeout(struct watchdog_device *wdd,
> >> + unsigned int timeout_parm,
> >> + struct device *dev)
> >> +{
> >> + return watchdog_init_timeouts(wdd, 0, timeout_parm, dev);
> >> +}
> >> +
> >> #ifdef CONFIG_HARDLOCKUP_DETECTOR
> >> void watchdog_nmi_disable_all(void);
> >> void watchdog_nmi_enable_all(void);
> >>
> >
>
>
>
> --
> Best regards,
>
> Fu Wei
> Software Engineer
> Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
> Ph: +86 21 61221326(direct)
> Ph: +86 186 2020 4684 (mobile)
> Room 1512, Regus One Corporate Avenue,Level 15,
> One Corporate Avenue,222 Hubin Road,Huangpu District,
> Shanghai,China 200021
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [non-pretimeout,4/7] Watchdog: introduce ARM SBSA watchdog driver
On Thu, Jun 11, 2015 at 01:47:29AM +0800, fu@linaro.org wrote:
> From: Fu Wei
>
> This driver bases on linux kernel watchdog framework.
> It supports getting timeout from parameter and FDT
> at the driver init stage.
> The first timeout period expires, the interrupt routine
> got another timeout period to run panic for saving
> system context.
>
Comments inline.
Thanks,
Guenter
> Signed-off-by: Fu Wei
> ---
> drivers/watchdog/Kconfig | 11 ++
> drivers/watchdog/Makefile| 1 +
> drivers/watchdog/sbsa_gwdt.c | 383
> +++
> 3 files changed, 395 insertions(+)
> create mode 100644 drivers/watchdog/sbsa_gwdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index e5e7c55..554f18a 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -152,6 +152,17 @@ config ARM_SP805_WATCHDOG
> ARM Primecell SP805 Watchdog timer. This will reboot your system when
> the timeout is reached.
>
> +config ARM_SBSA_WATCHDOG
> + tristate "ARM SBSA Generic Watchdog"
> + depends on ARM64
> + depends on ARM_ARCH_TIMER
> + select WATCHDOG_CORE
> + help
> + ARM SBSA Generic Watchdog. This watchdog has two Watchdog timeouts.
> + The first timeout will trigger a panic; the second timeout will
> + trigger a system reset.
> + More details: ARM DEN0029B - Server Base System Architecture (SBSA)
> +
To compile this driver as module, choose M here: The module
will be called sbsa_gwdt.
> config AT91RM9200_WATCHDOG
> tristate "AT91RM9200 watchdog"
> depends on SOC_AT91RM9200 && MFD_SYSCON
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 5c19294..471f1b7c 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o
>
> # ARM Architecture
> obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o
> +obj-$(CONFIG_ARM_SBSA_WATCHDOG) += sbsa_gwdt.o
> obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o
> obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o
> obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o
> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
> new file mode 100644
> index 000..1ddc10f
> --- /dev/null
> +++ b/drivers/watchdog/sbsa_gwdt.c
> @@ -0,0 +1,383 @@
> +/*
> + * SBSA(Server Base System Architecture) Generic Watchdog driver
> + *
> + * Copyright (c) 2015, Linaro Ltd.
> + * Author: Fu Wei
> + * Suravee Suthikulpanit
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License 2 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.
> + *
> + * Note: This SBSA Generic watchdog has two stage timeouts,
s/This/The/
"has two stages".
I would suggest to drop "Note:", but that is up to you.
> + * When the first timeout occurs, WS0(SPI or LPI) is triggered,
> + * the second timeout period(as long as the first timeout period)
> starts.
no longer accurate if WOR is used for the second period.
> + * In WS0 interrupt routine, panic() will be called for collecting
> + * crashdown info.
> + * If system can not recover from WS0 interrupt routine, then second
> + * timeout occurs, WS1(reset or higher level interrupt) is triggered.
> + * The two timeout period can be set by WOR(32bit).
The second timeout period is determined by ...
> + * WOR gives a maximum watch period of around 10s at the maximum
> + * system counter frequency.
> + * The System Counter shall run at maximum of 400MHz.
"... at the maximum system counter frequency of 400 MHz.", and drop the
last sentence.
Please uses spaces before '('.
> + *
> + * But If we need a larger timeout period, this driver will programme
> WCV
s/But //
s/this/the/
s/programme/program/
> + * directly. That can support more than 10s timeout at the maximum
> + * system counter frequency.
Drop the last sentence.
> + * More details: ARM DEN0029B - Server Base System Architecture (SBSA)
> + *
> + * SBSA GWDT:|---WOR(or WCV)---WS0---WOR(or WCV)---WS1
> + * |-timeout-WS0-timeout-WS1
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* SBSA Generic Watchdog register definitions */
> +/* refresh frame */
> +#define SBSA_GWDT_WRR0x000
> +
> +/* control frame */
> +#define SBSA_GWDT_WCS0x000
> +#define SBSA_GWDT_WOR0x008
> +#def
Re: [PATCH non-pretimeout 4/7] Watchdog: introduce ARM SBSA watchdog driver
On 06/10/2015 10:44 PM, Fu Wei wrote: Hi Guenter, On 11 June 2015 at 13:33, Guenter Roeck wrote: On 06/10/2015 10:47 AM, fu@linaro.org wrote: From: Fu Wei This driver bases on linux kernel watchdog framework. It supports getting timeout from parameter and FDT at the driver init stage. The first timeout period expires, the interrupt routine got another timeout period to run panic for saving system context. Signed-off-by: Fu Wei --- drivers/watchdog/Kconfig | 11 ++ drivers/watchdog/Makefile| 1 + drivers/watchdog/sbsa_gwdt.c | 383 +++ 3 files changed, 395 insertions(+) create mode 100644 drivers/watchdog/sbsa_gwdt.c diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..554f18a 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,17 @@ config ARM_SP805_WATCHDOG ARM Primecell SP805 Watchdog timer. This will reboot your system when the timeout is reached. +config ARM_SBSA_WATCHDOG + tristate "ARM SBSA Generic Watchdog" + depends on ARM64 + depends on ARM_ARCH_TIMER + select WATCHDOG_CORE + help + ARM SBSA Generic Watchdog. This watchdog has two Watchdog timeouts. + The first timeout will trigger a panic; the second timeout will + trigger a system reset. + More details: ARM DEN0029B - Server Base System Architecture (SBSA) + config AT91RM9200_WATCHDOG tristate "AT91RM9200 watchdog" depends on SOC_AT91RM9200 && MFD_SYSCON diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 5c19294..471f1b7c 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -30,6 +30,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o # ARM Architecture obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o +obj-$(CONFIG_ARM_SBSA_WATCHDOG) += sbsa_gwdt.o obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c new file mode 100644 index 000..1ddc10f --- /dev/null +++ b/drivers/watchdog/sbsa_gwdt.c @@ -0,0 +1,383 @@ +/* + * SBSA(Server Base System Architecture) Generic Watchdog driver + * + * Copyright (c) 2015, Linaro Ltd. + * Author: Fu Wei + * Suravee Suthikulpanit + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License 2 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. + * + * Note: This SBSA Generic watchdog has two stage timeouts, + * When the first timeout occurs, WS0(SPI or LPI) is triggered, + * the second timeout period(as long as the first timeout period) starts. + * In WS0 interrupt routine, panic() will be called for collecting + * crashdown info. + * If system can not recover from WS0 interrupt routine, then second + * timeout occurs, WS1(reset or higher level interrupt) is triggered. + * The two timeout period can be set by WOR(32bit). + * WOR gives a maximum watch period of around 10s at the maximum + * system counter frequency. + * The System Counter shall run at maximum of 400MHz. + * + * But If we need a larger timeout period, this driver will programme WCV + * directly. That can support more than 10s timeout at the maximum + * system counter frequency. + * More details: ARM DEN0029B - Server Base System Architecture (SBSA) + * + * SBSA GWDT:|---WOR(or WCV)---WS0---WOR(or WCV)---WS1 + * |-timeout-WS0-timeout-WS1 If we use WCV at all, I would like to see something like * SBSA GWDT:|---WOR(or WCV)---WS0WOR--WS1 * |-timeout-WS0-WS1 * panic hw reset where WOR would be used up to its maximum, to be replaced by WCV (but kept at maximum) if the selected timeout is larger than the maximum timeout selectable with WOR. Would this be possible ? for this part |-timeout-WS0, I am doing this way in non-pretimeout version. but for WS0-WS1, do you mean WOR would always be used up to its maximum??? yes I don't see any variable attached on it. So I would like to confirm what is this value min(timeout, max_wor_timeout) Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH non-pretimeout 4/7] Watchdog: introduce ARM SBSA watchdog driver
On 06/10/2015 10:47 AM, fu@linaro.org wrote: From: Fu Wei This driver bases on linux kernel watchdog framework. It supports getting timeout from parameter and FDT at the driver init stage. The first timeout period expires, the interrupt routine got another timeout period to run panic for saving system context. Signed-off-by: Fu Wei --- drivers/watchdog/Kconfig | 11 ++ drivers/watchdog/Makefile| 1 + drivers/watchdog/sbsa_gwdt.c | 383 +++ 3 files changed, 395 insertions(+) create mode 100644 drivers/watchdog/sbsa_gwdt.c diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..554f18a 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,17 @@ config ARM_SP805_WATCHDOG ARM Primecell SP805 Watchdog timer. This will reboot your system when the timeout is reached. +config ARM_SBSA_WATCHDOG + tristate "ARM SBSA Generic Watchdog" + depends on ARM64 + depends on ARM_ARCH_TIMER + select WATCHDOG_CORE + help + ARM SBSA Generic Watchdog. This watchdog has two Watchdog timeouts. + The first timeout will trigger a panic; the second timeout will + trigger a system reset. + More details: ARM DEN0029B - Server Base System Architecture (SBSA) + config AT91RM9200_WATCHDOG tristate "AT91RM9200 watchdog" depends on SOC_AT91RM9200 && MFD_SYSCON diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 5c19294..471f1b7c 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -30,6 +30,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o # ARM Architecture obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o +obj-$(CONFIG_ARM_SBSA_WATCHDOG) += sbsa_gwdt.o obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c new file mode 100644 index 000..1ddc10f --- /dev/null +++ b/drivers/watchdog/sbsa_gwdt.c @@ -0,0 +1,383 @@ +/* + * SBSA(Server Base System Architecture) Generic Watchdog driver + * + * Copyright (c) 2015, Linaro Ltd. + * Author: Fu Wei + * Suravee Suthikulpanit + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License 2 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. + * + * Note: This SBSA Generic watchdog has two stage timeouts, + * When the first timeout occurs, WS0(SPI or LPI) is triggered, + * the second timeout period(as long as the first timeout period) starts. + * In WS0 interrupt routine, panic() will be called for collecting + * crashdown info. + * If system can not recover from WS0 interrupt routine, then second + * timeout occurs, WS1(reset or higher level interrupt) is triggered. + * The two timeout period can be set by WOR(32bit). + * WOR gives a maximum watch period of around 10s at the maximum + * system counter frequency. + * The System Counter shall run at maximum of 400MHz. + * + * But If we need a larger timeout period, this driver will programme WCV + * directly. That can support more than 10s timeout at the maximum + * system counter frequency. + * More details: ARM DEN0029B - Server Base System Architecture (SBSA) + * + * SBSA GWDT:|---WOR(or WCV)---WS0---WOR(or WCV)---WS1 + * |-timeout-WS0-timeout-WS1 If we use WCV at all, I would like to see something like * SBSA GWDT:|---WOR(or WCV)---WS0WOR--WS1 * |-timeout-WS0-WS1 * panic hw reset where WOR would be used up to its maximum, to be replaced by WCV (but kept at maximum) if the selected timeout is larger than the maximum timeout selectable with WOR. Would this be possible ? Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver
On 06/10/2015 08:45 PM, Timur Tabi wrote: Fu Wei wrote: Could you suggest a good way to use WS0, so we can follow SBSA spec? To avoid the timeout/2 problem, WS0 calls panic, which is the "real" timeout/reset. WS1 is then a "backup" that is ignored by the driver. That is, the driver doesn't do anything with WS1 and it doesn't tell the kernel about WS1. I know you don't like the idea of WS1 as a backup timeout, but this is one way to solve the problem. This is what I would do. Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/8] Watchdog: introdouce "pretimeout" into framework
On 06/10/2015 06:41 AM, fu@linaro.org wrote:
From: Fu Wei
Also update Documentation/watchdog/watchdog-kernel-api.txt to
introduce:
(1)the new elements in the watchdog_device and watchdog_ops struct;
(2)the new API "watchdog_init_timeouts"
Reasons:
(1)kernel already has two watchdog drivers are using "pretimeout":
drivers/char/ipmi/ipmi_watchdog.c
drivers/watchdog/kempld_wdt.c(but the definition is different)
(2)some other drivers are going to use this: ARM SBSA Generic Watchdog
Signed-off-by: Fu Wei
---
Documentation/watchdog/watchdog-kernel-api.txt | 47 ++--
drivers/watchdog/watchdog_core.c | 100 +
drivers/watchdog/watchdog_dev.c| 53 +
include/linux/watchdog.h | 36 -
4 files changed, 197 insertions(+), 39 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt
b/Documentation/watchdog/watchdog-kernel-api.txt
index a0438f3..95b355d 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -49,6 +49,9 @@ struct watchdog_device {
unsigned int timeout;
unsigned int min_timeout;
unsigned int max_timeout;
+ unsigned int pretimeout;
+ unsigned int min_pretimeout;
+ unsigned int max_pretimeout;
void *driver_data;
struct mutex lock;
unsigned long status;
@@ -70,6 +73,9 @@ It contains following fields:
* timeout: the watchdog timer's timeout value (in seconds).
* min_timeout: the watchdog timer's minimum timeout value (in seconds).
* max_timeout: the watchdog timer's maximum timeout value (in seconds).
+* pretimeout: the watchdog timer's pretimeout value (in seconds).
+* min_pretimeout: the watchdog timer's minimum pretimeout value (in seconds).
+* max_pretimeout: the watchdog timer's maximum pretimeout value (in seconds).
* bootstatus: status of the device after booting (reported with watchdog
WDIOF_* status bits).
* driver_data: a pointer to the drivers private data of a watchdog device.
@@ -92,6 +98,7 @@ struct watchdog_ops {
int (*ping)(struct watchdog_device *);
unsigned int (*status)(struct watchdog_device *);
int (*set_timeout)(struct watchdog_device *, unsigned int);
+ int (*set_pretimeout)(struct watchdog_device *, unsigned int);
unsigned int (*get_timeleft)(struct watchdog_device *);
void (*ref)(struct watchdog_device *);
void (*unref)(struct watchdog_device *);
@@ -153,9 +160,19 @@ they are supported. These optional routines/operations are:
and -EIO for "could not write value to the watchdog". On success this
routine should set the timeout value of the watchdog_device to the
achieved timeout value (which may be different from the requested one
- because the watchdog does not necessarily has a 1 second resolution).
+ because the watchdog does not necessarily has a 1 second resolution;
+ If the driver supports pretimeout, then the timeout value must be greater
+ than that).
(Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
watchdog's info structure).
+* set_pretimeout: this routine checks and changes the pretimeout of the
+ watchdog timer device. It returns 0 on success, -EINVAL for "parameter out of
+ range" and -EIO for "could not write value to the watchdog". On success this
+ routine should set the pretimeout value of the watchdog_device to the
+ achieved pretimeout value (which may be different from the requested one
+ because the watchdog does not necessarily has a 1 second resolution).
+ (Note: the WDIOF_PRETIMEOUT needs to be set in the options field of the
+ watchdog's info structure).
* get_timeleft: this routines returns the time that's left before a reset.
* ref: the operation that calls kref_get on the kref of a dynamically
allocated watchdog_device struct.
@@ -219,8 +236,28 @@ extern int watchdog_init_timeout(struct watchdog_device
*wdd,
unsigned int timeout_parm, struct device
*dev);
The watchdog_init_timeout function allows you to initialize the timeout field
-using the module timeout parameter or by retrieving the timeout-sec property
from
-the device tree (if the module timeout parameter is invalid). Best practice is
-to set the default timeout value as timeout value in the watchdog_device and
-then use this function to set the user "preferred" timeout value.
+using the module timeout parameter or by retrieving the first element of
+the timeout-sec property from the device tree (if the module timeout parameter
Missing space before (.
+is invalid). Best practice is to set the default timeout value as timeout value
+in the watchdog_device and then use this function to set the user "preferred"
+timeout value.
+This routine returns zero on success and a negative errno code for failure.
+
+Some watchdog timers have two stage of timeouts(tim
Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver
On 06/09/2015 09:29 AM, Timur Tabi wrote: On 06/09/2015 11:22 AM, Guenter Roeck wrote: but I see your point. Essentially, the specification is broken for all practical purposes, since, as you point out, enabling the watchdog overwrites and explicitly sets WCV. Effectively this means that just using WCV to program the timeout period is not really possible. I am not really sure how to address this. We can either only use WOR, and forget about pretimeout, or we can enforce a minimum pretimeout. In the latter case, we'll have to write WCV after writing WOR. In talking with our hardware engineers, using WCV to program the timeout period is not a valid operation. This is why I keep arguing against the pre-timeout feature, and I don't agree that servers should always use pre-timeout. Not sure if "not valid" is correct - after all, it is mentioned in the specification. However, it is at the very least fragile. I tend to agree that we should just forget about pretimeout and use your original approach, where the timeout value is used to program WOR. Everything else is really just asking for trouble. Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver
On 06/09/2015 03:46 AM, Fu Wei wrote: Yes, if WOR only affect in TimeoutRefresh, we cat always make WOR == pretimeout But the problem is if we enable watchdog (write 0x01 to WCS will cause an explicit watchdog refresh), then 1) if ExplicitRefresh = True: CompareValue := SystemCounter + WOR WS0 := True 2) TimeoutRefresh is True again, WS0 == True: WS1 = True so once we enable watchdog, system reset, that is not what we want. this behavior is following SBSA spec. Ok, I admit I am a bit slow ;-). WS0 := True would be set the next time around, since if ExplicitRefresh == True WS0 = False WS1 = False but I see your point. Essentially, the specification is broken for all practical purposes, since, as you point out, enabling the watchdog overwrites and explicitly sets WCV. Effectively this means that just using WCV to program the timeout period is not really possible. I am not really sure how to address this. We can either only use WOR, and forget about pretimeout, or we can enforce a minimum pretimeout. In the latter case, we'll have to write WCV after writing WOR. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver
On 06/08/2015 11:37 PM, Fu Wei wrote: I would like to stress that pretimeout == 0 should not happen in a real server system, that is why we defined a SBSA watchdog, but not a normal one Clarification - In _your opinion_, a server should always use pretimeout. But we still need to thinking about the situation that administrator want to do this on a very special purpose. I could as well argue that setting pretimeout is the special situation. Some administrators may not want to bother but just want the system to reset if a watchdog timeout occurs. _Maybe_ if it happens multiple times, they might want to set up pretimeout to figure out why. Declaring that one _has_ to configure pretimeout is just a personal opinion, nothing else. We don't know what server administrators want, and we should not dictate anything unless technically necessary. maybe we can set min_pretimeout = 1 for this driver. that is just a suggestion. No. It is not technically necessary. if it must be set to a value > 0 I don't understand why setting it to 1 (instead of 1 second) would not be sufficient. it don't have to be 1 second , it can be 0.1, 0.5 or 0.01 second. like I said before, we just need a time slot to setup WCV in sbsa_gwdt_start. I have commented this in sbsa_gwdt_set_pretimeout in this patchset. I think what you really want to say is that you want to have time to handle the interrupt. But handling the interrupt is not asked for if pretimeout == 0. But I think the minimum time slot depend on implementation, the spec doesn't mention about this. Yes, because a minimum value does not exist. If pretimeout == 0, and we set up WOR a little bigger, it *ONLY* affect "the worst case" I mention above. in this case, a administrator set up pretimeout == 0 which should not happen in a real server, then the system goes very wrong. I don't think it is important that this server system reset in 1 second or 0.0001 second, at this time, this server can not provide any useful info anyway(because we don't use pretimeout). If system may go wrong, the administrator should set up pretimeout > 0 to figure out what is wrong with it just like he always should do. Yes, exactly. But otherwise, if pretimeout is set to 0, we want to reset immediately as directed. As I interpret the specification, WOR=0 forces WS1 immediately after WS0. 1) if TimeoutRefresh = True: CompareValue := SystemCounter + WOR (= SystemCounter WS0 := True 2) TimeoutRefresh is True again, WS0 == True: WS1 = True This is exactly the behavior we want if pretimeout == 0. In this situation, we don't want to handle the interrupt, we just want to reset the system as fast as possible. Having said that, have you tested what happens in your system if you set WOR=0 ? Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver
On 06/08/2015 08:59 PM, Fu Wei wrote:
Hi Guenter,
On 9 June 2015 at 02:26, Guenter Roeck wrote:
On 06/08/2015 09:05 AM, Fu Wei wrote:
Hi Gurnter
On 3 June 2015 at 01:07, Guenter Roeck wrote:
On 06/02/2015 09:55 AM, Fu Wei wrote:
Hi Timur,
Thanks , feedback inline
On 2 June 2015 at 23:32, Timur Tabi wrote:
On 06/01/2015 11:05 PM, fu@linaro.org wrote:
+/*
+ * help functions for accessing 32bit registers of SBSA Generic
Watchdog
+ */
+static void sbsa_gwdt_cf_write(unsigned int reg, u32 val,
+ struct watchdog_device *wdd)
+{
+ struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+ writel_relaxed(val, gwdt->control_base + reg);
+}
+
+static void sbsa_gwdt_rf_write(unsigned int reg, u32 val,
+ struct watchdog_device *wdd)
+{
+ struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+ writel_relaxed(val, gwdt->refresh_base + reg);
+}
+
+static u32 sbsa_gwdt_cf_read(unsigned int reg, struct watchdog_device
*wdd)
+{
+ struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+ return readl_relaxed(gwdt->control_base + reg);
+}
I still think you should get rid of these functions and just call
readl_relaxed() and writel_relaxed() every time, but I won't complain
again
if you keep them.
yes, that make sense, and will reduce the size of code, and I think
the code's readability will be OK too.
will try in my next patch,
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
+{
+ struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
+ struct watchdog_device *wdd = &gwdt->wdd;
+
+ if (wdd->pretimeout)
+ /* The pretimeout is valid, go panic */
+ panic("SBSA Watchdog pre-timeout");
+ else
+ /* We don't use pretimeout, trigger WS1 now*/
+ sbsa_gwdt_set_wcv(wdd, 0);
I don't like this.
If so, what is your idea ,if pretimeout == 0?
the reason of using WCV as (timout - pretimeout): it can provide the
longer timeout period,
(1)If we use WOR, it can only provide 10s @ 400MHz(max).
as Guenter said earlier, the default timer out for most watchdog will
be 30s, so I think 10s limit will be a little short
(2)we can always program WCV just like ping.
(3)if a timeout arrives, WOR will be use, so use it as pretimeout, but
we still can make this pretimeout longer by programming WCV(I don't
think it's necessary)
The triggering of the hardware reset should never depend
on an interrupt being handled properly.
if this fail, system reset in 1S, because WOR == (1s)
So ?
Even the interrupt routine isn't triggered, (WOR + system counter) -->
WCV,
then, sy system reset in 1S.
the hardware reset doesn't depend on an interrupt.
You should always program WCV
correctly in advance. This is especially true since pre-timeout will
probably rarely be used.
always programming WCV is doable. But I absolutely can not agree "
pre-timeout will probably rarely be used"
If so, SBSA watchdog is just a normal watchdog, This use case just
makes this HW useless.
If so, go to use SP805.
you still don't see the importance of this warning and pretimeout to a
real server.
If pretimeout isn't used, why not just set WCV = timeout, WOR = 0 ?
Because if WOR = 0 , according to SBSA, once you want to enable watchdog,
(0 + system counter) --> WCV , then , WS0 and WS1 will be triggered
immediately.
we have not a chance(a time slot) to update WCV.
I would have thought that this is exactly what we want if pretimeout is not
used.
Although pretimeout == 0 is not good for a server, but If
administrator set up pretimeout == 0. *I thinks we should trigger WS1
ASAP* . Because WS1 maybe a interrupter or a reboot signal, that is
why we can not reboot system directly.
This driver is SBSA watchdog driver, that means we need to follow SBSA spec:
(1) SBSA watchdog has two stage timeouts --> timeout and pretimeout is
the best solution in watchdog framework, at least for now.
(2) The watchdog has the following output signals:
Watchdog Signal 0 (WS0)---> "The initial signal is typically wired
to an interrupt and alerts the system."(original word from spec), I
thinks the key work should be "interrupt" and "alerts". So in WS0
interrupt routine, reset is absolutely a wrong operation. Although I
think we should make this "alerts" more useful. But for the first
version of driver, I thinks panic is useful enough.
Watchdog Signal 1 (WS1). ---> "The signal is fed to a higher agent
as an interrupt or reset for it to take executive action." (original
word from spec) . The key work should be "interrupt", "or" and "reset"
. So WS1 maybe a interrupt.
so even in the WS0 interrupt routine, if pretimeout == 0 , we need to
trigger WS1(that is what my patch is doing now, set WCV
Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver
On 06/08/2015 09:05 AM, Fu Wei wrote:
Hi Gurnter
On 3 June 2015 at 01:07, Guenter Roeck wrote:
On 06/02/2015 09:55 AM, Fu Wei wrote:
Hi Timur,
Thanks , feedback inline
On 2 June 2015 at 23:32, Timur Tabi wrote:
On 06/01/2015 11:05 PM, fu@linaro.org wrote:
+/*
+ * help functions for accessing 32bit registers of SBSA Generic
Watchdog
+ */
+static void sbsa_gwdt_cf_write(unsigned int reg, u32 val,
+ struct watchdog_device *wdd)
+{
+ struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+ writel_relaxed(val, gwdt->control_base + reg);
+}
+
+static void sbsa_gwdt_rf_write(unsigned int reg, u32 val,
+ struct watchdog_device *wdd)
+{
+ struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+ writel_relaxed(val, gwdt->refresh_base + reg);
+}
+
+static u32 sbsa_gwdt_cf_read(unsigned int reg, struct watchdog_device
*wdd)
+{
+ struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+ return readl_relaxed(gwdt->control_base + reg);
+}
I still think you should get rid of these functions and just call
readl_relaxed() and writel_relaxed() every time, but I won't complain
again
if you keep them.
yes, that make sense, and will reduce the size of code, and I think
the code's readability will be OK too.
will try in my next patch,
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
+{
+ struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
+ struct watchdog_device *wdd = &gwdt->wdd;
+
+ if (wdd->pretimeout)
+ /* The pretimeout is valid, go panic */
+ panic("SBSA Watchdog pre-timeout");
+ else
+ /* We don't use pretimeout, trigger WS1 now*/
+ sbsa_gwdt_set_wcv(wdd, 0);
I don't like this.
If so, what is your idea ,if pretimeout == 0?
the reason of using WCV as (timout - pretimeout): it can provide the
longer timeout period,
(1)If we use WOR, it can only provide 10s @ 400MHz(max).
as Guenter said earlier, the default timer out for most watchdog will
be 30s, so I think 10s limit will be a little short
(2)we can always program WCV just like ping.
(3)if a timeout arrives, WOR will be use, so use it as pretimeout, but
we still can make this pretimeout longer by programming WCV(I don't
think it's necessary)
The triggering of the hardware reset should never depend
on an interrupt being handled properly.
if this fail, system reset in 1S, because WOR == (1s)
So ?
Even the interrupt routine isn't triggered, (WOR + system counter) --> WCV,
then, sy system reset in 1S.
the hardware reset doesn't depend on an interrupt.
You should always program WCV
correctly in advance. This is especially true since pre-timeout will
probably rarely be used.
always programming WCV is doable. But I absolutely can not agree "
pre-timeout will probably rarely be used"
If so, SBSA watchdog is just a normal watchdog, This use case just
makes this HW useless.
If so, go to use SP805.
you still don't see the importance of this warning and pretimeout to a
real server.
If pretimeout isn't used, why not just set WCV = timeout, WOR = 0 ?
Because if WOR = 0 , according to SBSA, once you want to enable watchdog,
(0 + system counter) --> WCV , then , WS0 and WS1 will be triggered immediately.
we have not a chance(a time slot) to update WCV.
I would have thought that this is exactly what we want if pretimeout is not
used.
What am I missing here ?
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Dealing with optional i2c devices in a devicetree
On 06/08/2015 05:11 AM, Enrico Weigelt, metux IT consult wrote: Am 08.06.2015 um 08:01 schrieb Chris Packham: Hi folks, > Sounds like my best bet is to mark the nodes as disabled in the > dts and have my bootloader update them on the way through. In case your bootloader can take that decision (eg. if the device is present at boot time, and the bootloader has the proper probing logic or simply knows the device has to be there), that would be an pretty easy way. But let me add another usecase, which might be a bit more tricky: Let's assume we can plug in some more complex device, which consists of several (pretty standard) subdevices, behind certain bus'es (maybe it's attached via USB, and somewhere behind are some I2C bus'es with regulators, pwm-generators, etc). Let's further assume, we already got some DTS or some piece of memory with an DTB subtree for that device (eg. some simple bulk endpoint that just gives back a bunch of bytes with the DTB). Now, when the device is plugged in, I'd like to get that piece of DTB loaded and the corresponding drivers initialized. And, of course, when it's plugged-out, everything should be shut down cleanly. How could we achieve that ? Use devicetree overlays. Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Dealing with optional i2c devices in a devicetree
Hi Chris,
On 06/04/2015 11:03 PM, Chris Packham wrote:
Hi,
I'm working on a new board and one feature it as is a plug-in module
with an ADS7830 voltage monitor on it. This will be used during
manufacturing to sanity check that various voltage rails are within
expected ranges.
I have a dts entry for the device as below (with some omissions for the
sake of clarity)
soc {
internal-regs {
i2c@11000 {
ads7830@48 {
compatible = "ads7830";
reg = <0x48>;
};
};
};
};
The problem is that when the manufacturing card is not installed the
device still shows up in /sys/class/hwmon/hwmon0/ and
/sys/bus/i2c/devices/0-0048/ despite it not actually being present. If I
was using an old style initialization I could use
i2c_new_probed_device() which I think would stop the drivers probe()
function from being called
Looking at the ads7828_probe() function it doesn't actually do anything
with the i2c device before calling hwmon_device_register(). Some hwmon
drivers like lm73_probe() do attempt to read from the device and bail if
the read fails. I can probably fix my problem by doing something similar
in the ads7828_probe(), but there are other drivers that have a similar
probe function.
But that would be artificial in this case, and it could not be used to really
detect the chip but would only prove that there is a chip at the selected i2c
address. This is not really useful.
Is there a better way of getting the devicetree machinery to avoid the
call to the driver probe function in the first place?
The easiest solution would probably be to drop the entry from the
devicetree file and instantiate the driver manually via sysfs when
needed. Another option would be to load a different devicetree file
if the chip is plugged in.
You might also be able to use a devicetree overlay, but that would
probably add too much complexity if the chip is only used in
manufacturing.
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver
On Wed, Jun 03, 2015 at 01:16:41PM -0500, Timur Tabi wrote:
> On 06/01/2015 11:05 PM, fu@linaro.org wrote:
> >+if (wdd->pretimeout)
> >+/* The pretimeout is valid, go panic */
> >+panic("SBSA Watchdog pre-timeout");
>
> The problem with this is that WS1 will still occur. So a few seconds after
> the panic() call, the hardware will reset. There won't be any time to debug
> or log anything.
>
In general the idea here would be to use a crashdump kernel, which,
when loaded, would reset the watchdog before it fires. This kernel
would then write a core dump to a specified location.
If arm64 doesn't support a crashdump kernel, it might still be possible
to log the backtrace somewhere (eg in nvram using pstore if that is
supported via acpi or efi).
Is there reason to believe that this all won't work on arm64 ?
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver
On 06/02/2015 09:55 AM, Fu Wei wrote:
Hi Timur,
Thanks , feedback inline
On 2 June 2015 at 23:32, Timur Tabi wrote:
On 06/01/2015 11:05 PM, fu@linaro.org wrote:
+/*
+ * help functions for accessing 32bit registers of SBSA Generic Watchdog
+ */
+static void sbsa_gwdt_cf_write(unsigned int reg, u32 val,
+ struct watchdog_device *wdd)
+{
+ struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+ writel_relaxed(val, gwdt->control_base + reg);
+}
+
+static void sbsa_gwdt_rf_write(unsigned int reg, u32 val,
+ struct watchdog_device *wdd)
+{
+ struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+ writel_relaxed(val, gwdt->refresh_base + reg);
+}
+
+static u32 sbsa_gwdt_cf_read(unsigned int reg, struct watchdog_device
*wdd)
+{
+ struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+ return readl_relaxed(gwdt->control_base + reg);
+}
I still think you should get rid of these functions and just call
readl_relaxed() and writel_relaxed() every time, but I won't complain again
if you keep them.
yes, that make sense, and will reduce the size of code, and I think
the code's readability will be OK too.
will try in my next patch,
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
+{
+ struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
+ struct watchdog_device *wdd = &gwdt->wdd;
+
+ if (wdd->pretimeout)
+ /* The pretimeout is valid, go panic */
+ panic("SBSA Watchdog pre-timeout");
+ else
+ /* We don't use pretimeout, trigger WS1 now*/
+ sbsa_gwdt_set_wcv(wdd, 0);
I don't like this.
If so, what is your idea ,if pretimeout == 0?
the reason of using WCV as (timout - pretimeout): it can provide the
longer timeout period,
(1)If we use WOR, it can only provide 10s @ 400MHz(max).
as Guenter said earlier, the default timer out for most watchdog will
be 30s, so I think 10s limit will be a little short
(2)we can always program WCV just like ping.
(3)if a timeout arrives, WOR will be use, so use it as pretimeout, but
we still can make this pretimeout longer by programming WCV(I don't
think it's necessary)
The triggering of the hardware reset should never depend
on an interrupt being handled properly.
if this fail, system reset in 1S, because WOR == (1s)
So ?
You should always program WCV
correctly in advance. This is especially true since pre-timeout will
probably rarely be used.
always programming WCV is doable. But I absolutely can not agree "
pre-timeout will probably rarely be used"
If so, SBSA watchdog is just a normal watchdog, This use case just
makes this HW useless.
If so, go to use SP805.
you still don't see the importance of this warning and pretimeout to a
real server.
If pretimeout isn't used, why not just set WCV = timeout, WOR = 0 ?
Guenter
If the software of a real server goes wrong, then you just directly restart it ,
never try to sync/protect the current data, never try to figure out
what is wrong with it.
I don't think that is a good server software.
At least, I don't thinks " pre-timeout will probably rarely be used"
is a good idea for a server.
in another word, in a server ,pre-timeout should always be used.
So what happens if the CPU is totally hung and
Again, system reset in 1S, because WOR == (1s).
this interrupt handler is never called? When will the timeout occur?
if this interrupt hardler is never called, what I can see is "some
one is feeding the dog".
OK, in case, WS0 is triggered, but this interrupt hardler isn't
called, then software really goes wrong. Then , Again, Again system
reset in 1S, because WOR == (1s).
+static int sbsa_gwdt_probe(struct platform_device *pdev)
+{
+ u64 first_period_max = U64_MAX;
+ struct device *dev = &pdev->dev;
+ struct watchdog_device *wdd;
+ struct sbsa_gwdt *gwdt;
+ struct resource *res;
+ void *rf_base, *cf_base;
+ int ret, irq;
+ u32 status;
+
+ gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
+ if (!gwdt)
+ return -ENOMEM;
+ platform_set_drvdata(pdev, gwdt);
You should probably do this *after* calling platform_get_irq_byname().
it just dose (pdev->) dev->driver_data = gwdt
If we got gwdt, we can do that.
But maybe I miss something(or a rule of usage), so please let know
why this has to be called *after* calling platform_get_irq_byname().
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
"refresh");
+ rf_base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(rf_base))
+ return PTR_ERR(rf_base);
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
"control");
+ cf_base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(cf_base))
+ return PTR_ERR(cf_base);
+
+ irq = platform_get_irq_byname(pdev, "ws0");
+ if (irq < 0) {
+ dev_err(dev, "unable to ge
Re: [PATCH v4 4/7] Watchdog: introdouce "pretimeout" into framework
On 06/01/2015 09:05 PM, fu@linaro.org wrote:
From: Fu Wei
Also update Documentation/watchdog/watchdog-kernel-api.txt to
introduce:
(1)the new elements in the watchdog_device and watchdog_ops struct;
(2)the new API "watchdog_init_timeouts"
Reasons:
(1)kernel already has two watchdog drivers are using "pretimeout":
drivers/char/ipmi/ipmi_watchdog.c
drivers/watchdog/kempld_wdt.c(but the definition is different)
(2)some other drivers are going to use this: ARM SBSA Generic Watchdog
Signed-off-by: Fu Wei
---
Documentation/watchdog/watchdog-kernel-api.txt | 47 --
drivers/watchdog/watchdog_core.c | 115 +++--
drivers/watchdog/watchdog_dev.c| 53
include/linux/watchdog.h | 33 ++-
4 files changed, 212 insertions(+), 36 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt
b/Documentation/watchdog/watchdog-kernel-api.txt
index a0438f3..95b355d 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -49,6 +49,9 @@ struct watchdog_device {
unsigned int timeout;
unsigned int min_timeout;
unsigned int max_timeout;
+ unsigned int pretimeout;
+ unsigned int min_pretimeout;
+ unsigned int max_pretimeout;
void *driver_data;
struct mutex lock;
unsigned long status;
@@ -70,6 +73,9 @@ It contains following fields:
* timeout: the watchdog timer's timeout value (in seconds).
* min_timeout: the watchdog timer's minimum timeout value (in seconds).
* max_timeout: the watchdog timer's maximum timeout value (in seconds).
+* pretimeout: the watchdog timer's pretimeout value (in seconds).
+* min_pretimeout: the watchdog timer's minimum pretimeout value (in seconds).
+* max_pretimeout: the watchdog timer's maximum pretimeout value (in seconds).
* bootstatus: status of the device after booting (reported with watchdog
WDIOF_* status bits).
* driver_data: a pointer to the drivers private data of a watchdog device.
@@ -92,6 +98,7 @@ struct watchdog_ops {
int (*ping)(struct watchdog_device *);
unsigned int (*status)(struct watchdog_device *);
int (*set_timeout)(struct watchdog_device *, unsigned int);
+ int (*set_pretimeout)(struct watchdog_device *, unsigned int);
unsigned int (*get_timeleft)(struct watchdog_device *);
void (*ref)(struct watchdog_device *);
void (*unref)(struct watchdog_device *);
@@ -153,9 +160,19 @@ they are supported. These optional routines/operations are:
and -EIO for "could not write value to the watchdog". On success this
routine should set the timeout value of the watchdog_device to the
achieved timeout value (which may be different from the requested one
- because the watchdog does not necessarily has a 1 second resolution).
+ because the watchdog does not necessarily has a 1 second resolution;
+ If the driver supports pretimeout, then the timeout value must be greater
+ than that).
(Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
watchdog's info structure).
+* set_pretimeout: this routine checks and changes the pretimeout of the
+ watchdog timer device. It returns 0 on success, -EINVAL for "parameter out of
+ range" and -EIO for "could not write value to the watchdog". On success this
+ routine should set the pretimeout value of the watchdog_device to the
+ achieved pretimeout value (which may be different from the requested one
+ because the watchdog does not necessarily has a 1 second resolution).
+ (Note: the WDIOF_PRETIMEOUT needs to be set in the options field of the
+ watchdog's info structure).
* get_timeleft: this routines returns the time that's left before a reset.
* ref: the operation that calls kref_get on the kref of a dynamically
allocated watchdog_device struct.
@@ -219,8 +236,28 @@ extern int watchdog_init_timeout(struct watchdog_device
*wdd,
unsigned int timeout_parm, struct device
*dev);
The watchdog_init_timeout function allows you to initialize the timeout field
-using the module timeout parameter or by retrieving the timeout-sec property
from
-the device tree (if the module timeout parameter is invalid). Best practice is
-to set the default timeout value as timeout value in the watchdog_device and
-then use this function to set the user "preferred" timeout value.
+using the module timeout parameter or by retrieving the first element of
+the timeout-sec property from the device tree (if the module timeout parameter
+is invalid). Best practice is to set the default timeout value as timeout value
+in the watchdog_device and then use this function to set the user "preferred"
+timeout value.
+This routine returns zero on success and a negative errno code for failure.
+
+Some watchdog timers have two stage of timeouts(timeout and pretimeout),
+to initial
