[PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver
Neil Armstrong
narmstrong at baylibre.com
Tue May 31 02:57:03 PDT 2016
Hi Guenter,
On 05/30/2016 06:50 PM, Guenter Roeck wrote:
> On 05/30/2016 06:29 AM, Neil Armstrong wrote:
>> Add watchdog specific driver for Amlogic Meson GXBB SoC.
>>
>> Signed-off-by: Neil Armstrong <narmstrong at baylibre.com>
>> ---
>> drivers/watchdog/Makefile | 1 +
>> drivers/watchdog/meson_gxbb_wdt.c | 277 ++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 278 insertions(+)
>> create mode 100644 drivers/watchdog/meson_gxbb_wdt.c
>>
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 9bde095..7679d93 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -68,6 +68,7 @@ obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o
>> obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
>> obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
>> obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
>> +obj-$(CONFIG_MESON_WATCHDOG) += meson_gxbb_wdt.o
>
> I would really prefer a separate configuration option.
OK
>
>> obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
>> obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
>> obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
>> diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
>> new file mode 100644
>> index 0000000..9619c5e
>> --- /dev/null
>> +++ b/drivers/watchdog/meson_gxbb_wdt.c
>> @@ -0,0 +1,277 @@
>> +/*
>> + * This file is provided under a dual BSD/GPLv2 license. When using or
>> + * redistributing this file, you may do so under either license.
>> + *
>> + * GPL LICENSE SUMMARY
>> + *
>> + * Copyright (c) 2016 BayLibre, SAS.
>> + * Author: Neil Armstrong <narmstrong at baylibre.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of version 2 of the GNU General Public License as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
>> + * The full GNU General Public License is included in this distribution
>> + * in the file called COPYING.
>> + *
>> + * BSD LICENSE
>> + *
>> + * Copyright (c) 2016 BayLibre, SAS.
>> + * Author: Neil Armstrong <narmstrong at baylibre.com>
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * * Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * * Redistributions in binary form must reproduce the above copyright
>> + * notice, this list of conditions and the following disclaimer in
>> + * the documentation and/or other materials provided with the
>> + * distribution.
>> + * * Neither the name of Intel Corporation nor the names of its
>> + * contributors may be used to endorse or promote products derived
>> + * from this software without specific prior written permission.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
>> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
>> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
>> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> + */
>> +#include <linux/err.h>
>> +#include <linux/module.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/clk.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>> +#include <linux/watchdog.h>
>
> Please order include files alphabetically; that simplifies later changes
> and helps finding duplicates.
>
>> +
>> +#define DEFAULT_TIMEOUT 10 /* seconds */
>> +
>
> Sure you want the default timeout to be that low ?
I'm not sure of anything actually, what is generally set ?
>
>> +#define GXBB_WDT_CTRL_REG 0x0
>> +#define GXBB_WDT_CTRL1_REG 0x4
>> +#define GXBB_WDT_TCNT_REG 0x8
>> +#define GXBB_WDT_RSET_REG 0xc
>> +
>> +#define GXBB_WDT_CTRL_EE_RESET_NOW BIT(26)
>> +
>> +#define GXBB_WDT_CTRL_CLKDIV_EN BIT(25)
>> +#define GXBB_WDT_CTRL_CLK_EN BIT(24)
>> +#define GXBB_WDT_CTRL_IRQ_EN BIT(23)
>> +#define GXBB_WDT_CTRL_EE_RESET BIT(21)
>> +#define GXBB_WDT_CTRL_XTAL_SEL (0)
>> +#define GXBB_WDT_CTRL_CLK81_SEL BIT(19)
>> +#define GXBB_WDT_CTRL_EN BIT(18)
>> +#define GXBB_WDT_CTRL_DIV_MASK (BIT(18) - 1)
>> +
>> +#define GXBB_WDT_CTRL1_GPIO_PULSE BIT(17)
>> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0 BIT(16)
>> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1 (0)
>> +#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT (BIT(16) - 1)
>> +
>> +#define GXBB_WDT_TCNT_SETUP_MASK (BIT(16) - 1)
>> +#define GXBB_WDT_TCNT_CNT_SHIFT 16
>> +
>> +struct meson_gxbb_wdt {
>> + void __iomem *reg_base;
>> + struct watchdog_device wdt_dev;
>> + struct clk *clk;
>> +};
>> +
>> +static int meson_gxbb_wdt_start(struct watchdog_device *wdt_dev)
>> +{
>> + struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +
>> + writel(readl(data->reg_base + GXBB_WDT_CTRL_REG) | GXBB_WDT_CTRL_EN,
>> + data->reg_base + GXBB_WDT_CTRL_REG);
>> +
>> + return 0;
>> +}
>> +
>> +static int meson_gxbb_wdt_stop(struct watchdog_device *wdt_dev)
>> +{
>> + struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +
>> + writel(readl(data->reg_base + GXBB_WDT_CTRL_REG) & ~GXBB_WDT_CTRL_EN,
>> + data->reg_base + GXBB_WDT_CTRL_REG);
>
> Please align continuation lines with '('.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int meson_gxbb_wdt_ping(struct watchdog_device *wdt_dev)
>> +{
>> + struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +
>> + writel(0, data->reg_base + GXBB_WDT_RSET_REG);
>> +
>> + return 0;
>> +}
>> +
>> +static int meson_gxbb_wdt_get_timeout(struct meson_gxbb_wdt *data)
>> +{
>> + return (readl(data->reg_base + GXBB_WDT_TCNT_REG) &
>> + GXBB_WDT_TCNT_SETUP_MASK) / 1000;
>> +}
>> +
> Unused function ?
Crap, forgot to remove this.
>
>> +static int meson_gxbb_wdt_set_timeout(struct watchdog_device *wdt_dev,
>> + unsigned int timeout)
>> +{
>> + struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> +
>> + meson_gxbb_wdt_ping(wdt_dev);
>
> Is this necessary ? The infrastructure calls it after calling this function.
Is it really safe to change the timeout value before calling ping ?
On the HW, there is a running counter that is reset to 0 when writing any value to the RSET register.
A comparator triggers a reset when the counter is >= the timeout value.
If somehow we change to a lower value but the counter is > the new timeout value, it will reset the system.
Calling ping right before would keep this safe.
>
>> +
>> + writel(timeout * 1000, data->reg_base + GXBB_WDT_TCNT_REG);
>
> wdt_dev->timeout = timeout;
>
> Also, timeout may be larger than the maximum supported by the hardware,
> so you need to ensure that the value your write is not larger than
> GXBB_WDT_TCNT_SETUP_MASK.
Isn't already checked by the watchdog-core ?
>
>> +
>> + return 0;
>> +}
>> +
>> +static unsigned int meson_gxbb_wdt_get_timeleft(struct watchdog_device *wdt_dev)
>> +{
>> + struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>> + unsigned long reg;
>> +
>> + reg = readl(data->reg_base + GXBB_WDT_TCNT_REG);
>> +
>> + return ((reg >> GXBB_WDT_TCNT_CNT_SHIFT)-
>
> Space between operators, please.
>
>> + (reg & GXBB_WDT_TCNT_SETUP_MASK)) / 1000;
>> +}
>> +
>> +static const struct watchdog_ops meson_gxbb_wdt_ops = {
>> + .start = meson_gxbb_wdt_start,
>> + .stop = meson_gxbb_wdt_stop,
>> + .ping = meson_gxbb_wdt_ping,
>> + .set_timeout = meson_gxbb_wdt_set_timeout,
>> + .get_timeleft = meson_gxbb_wdt_get_timeleft,
>> +};
>> +
>> +static const struct watchdog_info meson_gxbb_wdt_info = {
>> + .identity = "meson-gxbb-wdt",
>
> It is common here to provide a user readable string, which would be something
> like "Meson GXBB Watchdog".
>
Why not.
[...]
>> +
>> +static int meson_gxbb_wdt_probe(struct platform_device *pdev)
>> +{
>> + struct meson_gxbb_wdt *data;
>> + struct resource *res;
>> + int ret;
>> +
>> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + data->reg_base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(data->reg_base))
>> + return PTR_ERR(data->reg_base);
>> +
>> + data->clk = devm_clk_get(&pdev->dev, NULL);
>> + if (IS_ERR(data->clk))
>> + return PTR_ERR(data->clk);
>> +
>> + clk_prepare_enable(data->clk);
>> +
>> + platform_set_drvdata(pdev, data);
>> +
>> + data->wdt_dev.parent = &pdev->dev;
>> + data->wdt_dev.info = &meson_gxbb_wdt_info;
>> + data->wdt_dev.ops = &meson_gxbb_wdt_ops;
>> + data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
>> + data->wdt_dev.min_timeout = 1;
>> + data->wdt_dev.timeout = DEFAULT_TIMEOUT;
>> + watchdog_set_drvdata(&data->wdt_dev, data);
>> +
>> + /* Setup with 1ms timebase */
>> + writel(((clk_get_rate(data->clk) / 1000) & GXBB_WDT_CTRL_DIV_MASK) |
>> + GXBB_WDT_CTRL_EE_RESET |
>> + GXBB_WDT_CTRL_CLK_EN |
>> + GXBB_WDT_CTRL_CLKDIV_EN,
>> + data->reg_base + GXBB_WDT_CTRL_REG);
>> +
>> + meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout);
>> +
>> + ret = watchdog_register_device(&data->wdt_dev);
>> + if (ret)
>> + return ret;
>
> clk_disable_unprepare() ?
Sure.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int meson_gxbb_wdt_remove(struct platform_device *pdev)
>> +{
>> + struct meson_gxbb_wdt *data = platform_get_drvdata(pdev);
>> +
>> + watchdog_unregister_device(&data->wdt_dev);
>> +
>
> clk_disable_unprepare() ?
Same.
Thanks,
Neil
More information about the linux-amlogic
mailing list