* Re: [alsa-devel] [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
@ 2019-11-25 6:16 ` Guenter Roeck
0 siblings, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2019-11-25 6:16 UTC (permalink / raw)
To: Jiaxin Yu
Cc: mark.rutland, alsa-devel, yong.liang, lgirdwood, robh+dt, tzungbi,
broonie, linux-mediatek, p.zabel, eason.yen, yingjoe.chen, wim,
linux-arm-kernel
On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> From: "yong.liang" <yong.liang@mediatek.com>
>
> Add reset controller API in watchdog driver.
> Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> camera, codec and connectivity) software reset functionality.
>
> Signed-off-by: yong.liang <yong.liang@mediatek.com>
> Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> ---
> drivers/watchdog/Kconfig | 1 +
> drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> 2 files changed, 111 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 2e07caab9db2..629249fe5305 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> tristate "Mediatek SoCs watchdog support"
> depends on ARCH_MEDIATEK || COMPILE_TEST
> select WATCHDOG_CORE
> + select RESET_CONTROLLER
> help
> Say Y here to include support for the watchdog timer
> in Mediatek SoCs.
> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> index 9c3d0033260d..d29484c7940a 100644
> --- a/drivers/watchdog/mtk_wdt.c
> +++ b/drivers/watchdog/mtk_wdt.c
> @@ -9,6 +9,9 @@
> * Based on sunxi_wdt.c
> */
>
> +#include <dt-bindings/reset-controller/mt2712-resets.h>
> +#include <dt-bindings/reset-controller/mt8183-resets.h>
> +#include <linux/delay.h>
> #include <linux/err.h>
> #include <linux/init.h>
> #include <linux/io.h>
> @@ -16,10 +19,12 @@
> #include <linux/module.h>
> #include <linux/moduleparam.h>
> #include <linux/of.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/slab.h>
> #include <linux/types.h>
> #include <linux/watchdog.h>
> -#include <linux/delay.h>
>
> #define WDT_MAX_TIMEOUT 31
> #define WDT_MIN_TIMEOUT 1
> @@ -44,6 +49,9 @@
> #define WDT_SWRST 0x14
> #define WDT_SWRST_KEY 0x1209
>
> +#define WDT_SWSYSRST 0x18U
> +#define WDT_SWSYS_RST_KEY 0x88000000
> +
> #define DRV_NAME "mtk-wdt"
> #define DRV_VERSION "1.0"
>
> @@ -53,8 +61,99 @@ static unsigned int timeout;
> struct mtk_wdt_dev {
> struct watchdog_device wdt_dev;
> void __iomem *wdt_base;
> + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> + struct reset_controller_dev rcdev;
> +};
> +
> +struct mtk_wdt_data {
> + int sw_rst_num;
> };
>
> +static const struct mtk_wdt_data mt2712_data = {
> + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> +};
> +
> +static const struct mtk_wdt_data mt8183_data = {
> + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> +};
The number of resets can be set in .data directly; there is no need
for the structures.
> +
> +static int toprgu_reset_assert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + unsigned int tmp;
> + unsigned long flags;
> + struct mtk_wdt_dev *data =
> + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> +
> + spin_lock_irqsave(&data->lock, flags);
> +
> + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> + tmp |= BIT(id);
> + tmp |= WDT_SWSYS_RST_KEY;
> + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> +
> + spin_unlock_irqrestore(&data->lock, flags);
> +
> + return 0;
> +}
> +
> +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + unsigned int tmp;
> + unsigned long flags;
> + struct mtk_wdt_dev *data =
> + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> +
> + spin_lock_irqsave(&data->lock, flags);
> +
> + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> + tmp &= ~BIT(id);
> + tmp |= WDT_SWSYS_RST_KEY;
> + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> +
> + spin_unlock_irqrestore(&data->lock, flags);
> +
> + return 0;
> +}
There is a lot of duplication in those functions. Only one line
is different. I think this is a good example where a helper function
with an additional argument indicating set or reset would be helpful.
> +
> +static int toprgu_reset(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + int ret;
> +
> + ret = toprgu_reset_assert(rcdev, id);
> + if (ret)
> + return ret;
> +
> + return toprgu_reset_deassert(rcdev, id);
> +}
> +
> +static const struct reset_control_ops toprgu_reset_ops = {
> + .assert = toprgu_reset_assert,
> + .deassert = toprgu_reset_deassert,
> + .reset = toprgu_reset,
> +};
> +
> +static int toprgu_register_reset_controller(struct platform_device *pdev,
> + int rst_num)
> +{
> + int ret;
> + struct mtk_wdt_dev *mtk_wdt = platform_get_drvdata(pdev);
> +
> + spin_lock_init(&mtk_wdt->lock);
> +
> + mtk_wdt->rcdev.owner = THIS_MODULE;
> + mtk_wdt->rcdev.nr_resets = rst_num;
> + mtk_wdt->rcdev.ops = &toprgu_reset_ops;
> + mtk_wdt->rcdev.of_node = pdev->dev.of_node;
> + ret = reset_controller_register(&mtk_wdt->rcdev);
> + if (ret != 0)
> + dev_err(&pdev->dev,
> + "couldn't register wdt reset controller: %d\n", ret);
> + return ret;
> +}
> +
> static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
> unsigned long action, void *data)
> {
> @@ -155,6 +254,7 @@ static int mtk_wdt_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct mtk_wdt_dev *mtk_wdt;
> + struct mtk_wdt_data *wdt_data;
> int err;
>
> mtk_wdt = devm_kzalloc(dev, sizeof(*mtk_wdt), GFP_KERNEL);
> @@ -190,6 +290,13 @@ static int mtk_wdt_probe(struct platform_device *pdev)
> dev_info(dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)\n",
> mtk_wdt->wdt_dev.timeout, nowayout);
>
> + wdt_data = (struct mtk_wdt_data *)of_device_get_match_data(dev);
> + if (wdt_data) {
> + err = toprgu_register_reset_controller(pdev,
> + wdt_data->sw_rst_num);
> + if (err)
> + return err;
If this happens, the user will wonder why there was a message "Watchdog
enabled", but there is no watchdog. It would be better to call this
function before the dev_info() above.
Guenter
> + }
> return 0;
> }
>
> @@ -218,7 +325,9 @@ static int mtk_wdt_resume(struct device *dev)
> #endif
>
> static const struct of_device_id mtk_wdt_dt_ids[] = {
> + { .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },
> { .compatible = "mediatek,mt6589-wdt" },
> + { .compatible = "mediatek,mt8183-wdt", .data = &mt8183_data },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, mtk_wdt_dt_ids);
> --
> 2.18.0
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
@ 2019-11-25 6:16 ` Guenter Roeck
0 siblings, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2019-11-25 6:16 UTC (permalink / raw)
To: Jiaxin Yu
Cc: mark.rutland, alsa-devel, yong.liang, lgirdwood, robh+dt, perex,
tzungbi, broonie, linux-mediatek, p.zabel, eason.yen,
yingjoe.chen, wim, linux-arm-kernel
On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> From: "yong.liang" <yong.liang@mediatek.com>
>
> Add reset controller API in watchdog driver.
> Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> camera, codec and connectivity) software reset functionality.
>
> Signed-off-by: yong.liang <yong.liang@mediatek.com>
> Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> ---
> drivers/watchdog/Kconfig | 1 +
> drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> 2 files changed, 111 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 2e07caab9db2..629249fe5305 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> tristate "Mediatek SoCs watchdog support"
> depends on ARCH_MEDIATEK || COMPILE_TEST
> select WATCHDOG_CORE
> + select RESET_CONTROLLER
> help
> Say Y here to include support for the watchdog timer
> in Mediatek SoCs.
> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> index 9c3d0033260d..d29484c7940a 100644
> --- a/drivers/watchdog/mtk_wdt.c
> +++ b/drivers/watchdog/mtk_wdt.c
> @@ -9,6 +9,9 @@
> * Based on sunxi_wdt.c
> */
>
> +#include <dt-bindings/reset-controller/mt2712-resets.h>
> +#include <dt-bindings/reset-controller/mt8183-resets.h>
> +#include <linux/delay.h>
> #include <linux/err.h>
> #include <linux/init.h>
> #include <linux/io.h>
> @@ -16,10 +19,12 @@
> #include <linux/module.h>
> #include <linux/moduleparam.h>
> #include <linux/of.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/slab.h>
> #include <linux/types.h>
> #include <linux/watchdog.h>
> -#include <linux/delay.h>
>
> #define WDT_MAX_TIMEOUT 31
> #define WDT_MIN_TIMEOUT 1
> @@ -44,6 +49,9 @@
> #define WDT_SWRST 0x14
> #define WDT_SWRST_KEY 0x1209
>
> +#define WDT_SWSYSRST 0x18U
> +#define WDT_SWSYS_RST_KEY 0x88000000
> +
> #define DRV_NAME "mtk-wdt"
> #define DRV_VERSION "1.0"
>
> @@ -53,8 +61,99 @@ static unsigned int timeout;
> struct mtk_wdt_dev {
> struct watchdog_device wdt_dev;
> void __iomem *wdt_base;
> + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> + struct reset_controller_dev rcdev;
> +};
> +
> +struct mtk_wdt_data {
> + int sw_rst_num;
> };
>
> +static const struct mtk_wdt_data mt2712_data = {
> + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> +};
> +
> +static const struct mtk_wdt_data mt8183_data = {
> + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> +};
The number of resets can be set in .data directly; there is no need
for the structures.
> +
> +static int toprgu_reset_assert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + unsigned int tmp;
> + unsigned long flags;
> + struct mtk_wdt_dev *data =
> + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> +
> + spin_lock_irqsave(&data->lock, flags);
> +
> + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> + tmp |= BIT(id);
> + tmp |= WDT_SWSYS_RST_KEY;
> + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> +
> + spin_unlock_irqrestore(&data->lock, flags);
> +
> + return 0;
> +}
> +
> +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + unsigned int tmp;
> + unsigned long flags;
> + struct mtk_wdt_dev *data =
> + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> +
> + spin_lock_irqsave(&data->lock, flags);
> +
> + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> + tmp &= ~BIT(id);
> + tmp |= WDT_SWSYS_RST_KEY;
> + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> +
> + spin_unlock_irqrestore(&data->lock, flags);
> +
> + return 0;
> +}
There is a lot of duplication in those functions. Only one line
is different. I think this is a good example where a helper function
with an additional argument indicating set or reset would be helpful.
> +
> +static int toprgu_reset(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + int ret;
> +
> + ret = toprgu_reset_assert(rcdev, id);
> + if (ret)
> + return ret;
> +
> + return toprgu_reset_deassert(rcdev, id);
> +}
> +
> +static const struct reset_control_ops toprgu_reset_ops = {
> + .assert = toprgu_reset_assert,
> + .deassert = toprgu_reset_deassert,
> + .reset = toprgu_reset,
> +};
> +
> +static int toprgu_register_reset_controller(struct platform_device *pdev,
> + int rst_num)
> +{
> + int ret;
> + struct mtk_wdt_dev *mtk_wdt = platform_get_drvdata(pdev);
> +
> + spin_lock_init(&mtk_wdt->lock);
> +
> + mtk_wdt->rcdev.owner = THIS_MODULE;
> + mtk_wdt->rcdev.nr_resets = rst_num;
> + mtk_wdt->rcdev.ops = &toprgu_reset_ops;
> + mtk_wdt->rcdev.of_node = pdev->dev.of_node;
> + ret = reset_controller_register(&mtk_wdt->rcdev);
> + if (ret != 0)
> + dev_err(&pdev->dev,
> + "couldn't register wdt reset controller: %d\n", ret);
> + return ret;
> +}
> +
> static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
> unsigned long action, void *data)
> {
> @@ -155,6 +254,7 @@ static int mtk_wdt_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct mtk_wdt_dev *mtk_wdt;
> + struct mtk_wdt_data *wdt_data;
> int err;
>
> mtk_wdt = devm_kzalloc(dev, sizeof(*mtk_wdt), GFP_KERNEL);
> @@ -190,6 +290,13 @@ static int mtk_wdt_probe(struct platform_device *pdev)
> dev_info(dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)\n",
> mtk_wdt->wdt_dev.timeout, nowayout);
>
> + wdt_data = (struct mtk_wdt_data *)of_device_get_match_data(dev);
> + if (wdt_data) {
> + err = toprgu_register_reset_controller(pdev,
> + wdt_data->sw_rst_num);
> + if (err)
> + return err;
If this happens, the user will wonder why there was a message "Watchdog
enabled", but there is no watchdog. It would be better to call this
function before the dev_info() above.
Guenter
> + }
> return 0;
> }
>
> @@ -218,7 +325,9 @@ static int mtk_wdt_resume(struct device *dev)
> #endif
>
> static const struct of_device_id mtk_wdt_dt_ids[] = {
> + { .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },
> { .compatible = "mediatek,mt6589-wdt" },
> + { .compatible = "mediatek,mt8183-wdt", .data = &mt8183_data },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, mtk_wdt_dt_ids);
> --
> 2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [alsa-devel] [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
2019-11-25 6:16 ` Guenter Roeck
(?)
@ 2019-11-25 9:51 ` Philipp Zabel
-1 siblings, 0 replies; 21+ messages in thread
From: Philipp Zabel @ 2019-11-25 9:51 UTC (permalink / raw)
To: Guenter Roeck, Jiaxin Yu
Cc: mark.rutland, alsa-devel, yong.liang, lgirdwood, robh+dt, tzungbi,
broonie, linux-mediatek, eason.yen, yingjoe.chen, wim,
linux-arm-kernel
Hi,
On Sun, 2019-11-24 at 22:16 -0800, Guenter Roeck wrote:
> On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> > From: "yong.liang" <yong.liang@mediatek.com>
> >
> > Add reset controller API in watchdog driver.
> > Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> > camera, codec and connectivity) software reset functionality.
> >
> > Signed-off-by: yong.liang <yong.liang@mediatek.com>
> > Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> > Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > ---
> > drivers/watchdog/Kconfig | 1 +
> > drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 111 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 2e07caab9db2..629249fe5305 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> > tristate "Mediatek SoCs watchdog support"
> > depends on ARCH_MEDIATEK || COMPILE_TEST
> > select WATCHDOG_CORE
> > + select RESET_CONTROLLER
> > help
> > Say Y here to include support for the watchdog timer
> > in Mediatek SoCs.
> > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > index 9c3d0033260d..d29484c7940a 100644
> > --- a/drivers/watchdog/mtk_wdt.c
> > +++ b/drivers/watchdog/mtk_wdt.c
> > @@ -9,6 +9,9 @@
> > * Based on sunxi_wdt.c
> > */
> >
> > +#include <dt-bindings/reset-controller/mt2712-resets.h>
> > +#include <dt-bindings/reset-controller/mt8183-resets.h>
> > +#include <linux/delay.h>
> > #include <linux/err.h>
> > #include <linux/init.h>
> > #include <linux/io.h>
> > @@ -16,10 +19,12 @@
> > #include <linux/module.h>
> > #include <linux/moduleparam.h>
> > #include <linux/of.h>
> > +#include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > +#include <linux/reset-controller.h>
> > +#include <linux/slab.h>
> > #include <linux/types.h>
> > #include <linux/watchdog.h>
> > -#include <linux/delay.h>
> >
> > #define WDT_MAX_TIMEOUT 31
> > #define WDT_MIN_TIMEOUT 1
> > @@ -44,6 +49,9 @@
> > #define WDT_SWRST 0x14
> > #define WDT_SWRST_KEY 0x1209
> >
> > +#define WDT_SWSYSRST 0x18U
> > +#define WDT_SWSYS_RST_KEY 0x88000000
> > +
> > #define DRV_NAME "mtk-wdt"
> > #define DRV_VERSION "1.0"
> >
> > @@ -53,8 +61,99 @@ static unsigned int timeout;
> > struct mtk_wdt_dev {
> > struct watchdog_device wdt_dev;
> > void __iomem *wdt_base;
> > + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> > + struct reset_controller_dev rcdev;
> > +};
> > +
> > +struct mtk_wdt_data {
> > + int sw_rst_num;
> > };
> >
> > +static const struct mtk_wdt_data mt2712_data = {
> > + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> > +};
> > +
> > +static const struct mtk_wdt_data mt8183_data = {
> > + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> > +};
>
> The number of resets can be set in .data directly; there is no need
> for the structures.
>
> > +
> > +static int toprgu_reset_assert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + unsigned int tmp;
> > + unsigned long flags;
> > + struct mtk_wdt_dev *data =
> > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > +
> > + spin_lock_irqsave(&data->lock, flags);
> > +
> > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
I think this should be readl_relaxed() instead. I don't expect this
driver will ever be used on a big-endian architecture, but mixing
__raw_readl() and writel() does look dangerous.
> > + tmp |= BIT(id);
> > + tmp |= WDT_SWSYS_RST_KEY;
> > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > +
> > + spin_unlock_irqrestore(&data->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + unsigned int tmp;
> > + unsigned long flags;
> > + struct mtk_wdt_dev *data =
> > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > +
> > + spin_lock_irqsave(&data->lock, flags);
> > +
> > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > + tmp &= ~BIT(id);
> > + tmp |= WDT_SWSYS_RST_KEY;
> > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > +
> > + spin_unlock_irqrestore(&data->lock, flags);
> > +
> > + return 0;
> > +}
>
> There is a lot of duplication in those functions. Only one line
> is different. I think this is a good example where a helper function
> with an additional argument indicating set or reset would be helpful.
>
> > +
> > +static int toprgu_reset(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + int ret;
> > +
> > + ret = toprgu_reset_assert(rcdev, id);
> > + if (ret)
> > + return ret;
> > +
> > + return toprgu_reset_deassert(rcdev, id);
> > +}
> > +
> > +static const struct reset_control_ops toprgu_reset_ops = {
> > + .assert = toprgu_reset_assert,
> > + .deassert = toprgu_reset_deassert,
> > + .reset = toprgu_reset,
> > +};
> > +
> > +static int toprgu_register_reset_controller(struct platform_device *pdev,
> > + int rst_num)
> > +{
> > + int ret;
> > + struct mtk_wdt_dev *mtk_wdt = platform_get_drvdata(pdev);
> > +
> > + spin_lock_init(&mtk_wdt->lock);
> > +
> > + mtk_wdt->rcdev.owner = THIS_MODULE;
> > + mtk_wdt->rcdev.nr_resets = rst_num;
> > + mtk_wdt->rcdev.ops = &toprgu_reset_ops;
> > + mtk_wdt->rcdev.of_node = pdev->dev.of_node;
> > + ret = reset_controller_register(&mtk_wdt->rcdev);
I see this driver uses devm_kzalloc() below. Should this be
devm_reset_controller_register()?
> > + if (ret != 0)
> > + dev_err(&pdev->dev,
> > + "couldn't register wdt reset controller: %d\n", ret);
> > + return ret;
> > +}
> > +
> > static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
> > unsigned long action, void *data)
> > {
> > @@ -155,6 +254,7 @@ static int mtk_wdt_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > struct mtk_wdt_dev *mtk_wdt;
> > + struct mtk_wdt_data *wdt_data;
> > int err;
> >
> > mtk_wdt = devm_kzalloc(dev, sizeof(*mtk_wdt), GFP_KERNEL);
regards
Philipp
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
@ 2019-11-25 9:51 ` Philipp Zabel
0 siblings, 0 replies; 21+ messages in thread
From: Philipp Zabel @ 2019-11-25 9:51 UTC (permalink / raw)
To: Guenter Roeck, Jiaxin Yu
Cc: mark.rutland, alsa-devel, yong.liang, lgirdwood, robh+dt, perex,
tzungbi, broonie, linux-mediatek, eason.yen, yingjoe.chen, wim,
linux-arm-kernel
Hi,
On Sun, 2019-11-24 at 22:16 -0800, Guenter Roeck wrote:
> On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> > From: "yong.liang" <yong.liang@mediatek.com>
> >
> > Add reset controller API in watchdog driver.
> > Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> > camera, codec and connectivity) software reset functionality.
> >
> > Signed-off-by: yong.liang <yong.liang@mediatek.com>
> > Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> > Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > ---
> > drivers/watchdog/Kconfig | 1 +
> > drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 111 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 2e07caab9db2..629249fe5305 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> > tristate "Mediatek SoCs watchdog support"
> > depends on ARCH_MEDIATEK || COMPILE_TEST
> > select WATCHDOG_CORE
> > + select RESET_CONTROLLER
> > help
> > Say Y here to include support for the watchdog timer
> > in Mediatek SoCs.
> > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > index 9c3d0033260d..d29484c7940a 100644
> > --- a/drivers/watchdog/mtk_wdt.c
> > +++ b/drivers/watchdog/mtk_wdt.c
> > @@ -9,6 +9,9 @@
> > * Based on sunxi_wdt.c
> > */
> >
> > +#include <dt-bindings/reset-controller/mt2712-resets.h>
> > +#include <dt-bindings/reset-controller/mt8183-resets.h>
> > +#include <linux/delay.h>
> > #include <linux/err.h>
> > #include <linux/init.h>
> > #include <linux/io.h>
> > @@ -16,10 +19,12 @@
> > #include <linux/module.h>
> > #include <linux/moduleparam.h>
> > #include <linux/of.h>
> > +#include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > +#include <linux/reset-controller.h>
> > +#include <linux/slab.h>
> > #include <linux/types.h>
> > #include <linux/watchdog.h>
> > -#include <linux/delay.h>
> >
> > #define WDT_MAX_TIMEOUT 31
> > #define WDT_MIN_TIMEOUT 1
> > @@ -44,6 +49,9 @@
> > #define WDT_SWRST 0x14
> > #define WDT_SWRST_KEY 0x1209
> >
> > +#define WDT_SWSYSRST 0x18U
> > +#define WDT_SWSYS_RST_KEY 0x88000000
> > +
> > #define DRV_NAME "mtk-wdt"
> > #define DRV_VERSION "1.0"
> >
> > @@ -53,8 +61,99 @@ static unsigned int timeout;
> > struct mtk_wdt_dev {
> > struct watchdog_device wdt_dev;
> > void __iomem *wdt_base;
> > + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> > + struct reset_controller_dev rcdev;
> > +};
> > +
> > +struct mtk_wdt_data {
> > + int sw_rst_num;
> > };
> >
> > +static const struct mtk_wdt_data mt2712_data = {
> > + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> > +};
> > +
> > +static const struct mtk_wdt_data mt8183_data = {
> > + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> > +};
>
> The number of resets can be set in .data directly; there is no need
> for the structures.
>
> > +
> > +static int toprgu_reset_assert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + unsigned int tmp;
> > + unsigned long flags;
> > + struct mtk_wdt_dev *data =
> > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > +
> > + spin_lock_irqsave(&data->lock, flags);
> > +
> > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
I think this should be readl_relaxed() instead. I don't expect this
driver will ever be used on a big-endian architecture, but mixing
__raw_readl() and writel() does look dangerous.
> > + tmp |= BIT(id);
> > + tmp |= WDT_SWSYS_RST_KEY;
> > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > +
> > + spin_unlock_irqrestore(&data->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + unsigned int tmp;
> > + unsigned long flags;
> > + struct mtk_wdt_dev *data =
> > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > +
> > + spin_lock_irqsave(&data->lock, flags);
> > +
> > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > + tmp &= ~BIT(id);
> > + tmp |= WDT_SWSYS_RST_KEY;
> > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > +
> > + spin_unlock_irqrestore(&data->lock, flags);
> > +
> > + return 0;
> > +}
>
> There is a lot of duplication in those functions. Only one line
> is different. I think this is a good example where a helper function
> with an additional argument indicating set or reset would be helpful.
>
> > +
> > +static int toprgu_reset(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + int ret;
> > +
> > + ret = toprgu_reset_assert(rcdev, id);
> > + if (ret)
> > + return ret;
> > +
> > + return toprgu_reset_deassert(rcdev, id);
> > +}
> > +
> > +static const struct reset_control_ops toprgu_reset_ops = {
> > + .assert = toprgu_reset_assert,
> > + .deassert = toprgu_reset_deassert,
> > + .reset = toprgu_reset,
> > +};
> > +
> > +static int toprgu_register_reset_controller(struct platform_device *pdev,
> > + int rst_num)
> > +{
> > + int ret;
> > + struct mtk_wdt_dev *mtk_wdt = platform_get_drvdata(pdev);
> > +
> > + spin_lock_init(&mtk_wdt->lock);
> > +
> > + mtk_wdt->rcdev.owner = THIS_MODULE;
> > + mtk_wdt->rcdev.nr_resets = rst_num;
> > + mtk_wdt->rcdev.ops = &toprgu_reset_ops;
> > + mtk_wdt->rcdev.of_node = pdev->dev.of_node;
> > + ret = reset_controller_register(&mtk_wdt->rcdev);
I see this driver uses devm_kzalloc() below. Should this be
devm_reset_controller_register()?
> > + if (ret != 0)
> > + dev_err(&pdev->dev,
> > + "couldn't register wdt reset controller: %d\n", ret);
> > + return ret;
> > +}
> > +
> > static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
> > unsigned long action, void *data)
> > {
> > @@ -155,6 +254,7 @@ static int mtk_wdt_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > struct mtk_wdt_dev *mtk_wdt;
> > + struct mtk_wdt_data *wdt_data;
> > int err;
> >
> > mtk_wdt = devm_kzalloc(dev, sizeof(*mtk_wdt), GFP_KERNEL);
regards
Philipp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
@ 2019-11-25 9:51 ` Philipp Zabel
0 siblings, 0 replies; 21+ messages in thread
From: Philipp Zabel @ 2019-11-25 9:51 UTC (permalink / raw)
To: Guenter Roeck, Jiaxin Yu
Cc: mark.rutland, alsa-devel, yong.liang, lgirdwood, robh+dt, perex,
tzungbi, broonie, linux-mediatek, eason.yen, yingjoe.chen, wim,
linux-arm-kernel
Hi,
On Sun, 2019-11-24 at 22:16 -0800, Guenter Roeck wrote:
> On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> > From: "yong.liang" <yong.liang@mediatek.com>
> >
> > Add reset controller API in watchdog driver.
> > Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> > camera, codec and connectivity) software reset functionality.
> >
> > Signed-off-by: yong.liang <yong.liang@mediatek.com>
> > Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> > Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > ---
> > drivers/watchdog/Kconfig | 1 +
> > drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 111 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 2e07caab9db2..629249fe5305 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> > tristate "Mediatek SoCs watchdog support"
> > depends on ARCH_MEDIATEK || COMPILE_TEST
> > select WATCHDOG_CORE
> > + select RESET_CONTROLLER
> > help
> > Say Y here to include support for the watchdog timer
> > in Mediatek SoCs.
> > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > index 9c3d0033260d..d29484c7940a 100644
> > --- a/drivers/watchdog/mtk_wdt.c
> > +++ b/drivers/watchdog/mtk_wdt.c
> > @@ -9,6 +9,9 @@
> > * Based on sunxi_wdt.c
> > */
> >
> > +#include <dt-bindings/reset-controller/mt2712-resets.h>
> > +#include <dt-bindings/reset-controller/mt8183-resets.h>
> > +#include <linux/delay.h>
> > #include <linux/err.h>
> > #include <linux/init.h>
> > #include <linux/io.h>
> > @@ -16,10 +19,12 @@
> > #include <linux/module.h>
> > #include <linux/moduleparam.h>
> > #include <linux/of.h>
> > +#include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > +#include <linux/reset-controller.h>
> > +#include <linux/slab.h>
> > #include <linux/types.h>
> > #include <linux/watchdog.h>
> > -#include <linux/delay.h>
> >
> > #define WDT_MAX_TIMEOUT 31
> > #define WDT_MIN_TIMEOUT 1
> > @@ -44,6 +49,9 @@
> > #define WDT_SWRST 0x14
> > #define WDT_SWRST_KEY 0x1209
> >
> > +#define WDT_SWSYSRST 0x18U
> > +#define WDT_SWSYS_RST_KEY 0x88000000
> > +
> > #define DRV_NAME "mtk-wdt"
> > #define DRV_VERSION "1.0"
> >
> > @@ -53,8 +61,99 @@ static unsigned int timeout;
> > struct mtk_wdt_dev {
> > struct watchdog_device wdt_dev;
> > void __iomem *wdt_base;
> > + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> > + struct reset_controller_dev rcdev;
> > +};
> > +
> > +struct mtk_wdt_data {
> > + int sw_rst_num;
> > };
> >
> > +static const struct mtk_wdt_data mt2712_data = {
> > + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> > +};
> > +
> > +static const struct mtk_wdt_data mt8183_data = {
> > + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> > +};
>
> The number of resets can be set in .data directly; there is no need
> for the structures.
>
> > +
> > +static int toprgu_reset_assert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + unsigned int tmp;
> > + unsigned long flags;
> > + struct mtk_wdt_dev *data =
> > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > +
> > + spin_lock_irqsave(&data->lock, flags);
> > +
> > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
I think this should be readl_relaxed() instead. I don't expect this
driver will ever be used on a big-endian architecture, but mixing
__raw_readl() and writel() does look dangerous.
> > + tmp |= BIT(id);
> > + tmp |= WDT_SWSYS_RST_KEY;
> > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > +
> > + spin_unlock_irqrestore(&data->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + unsigned int tmp;
> > + unsigned long flags;
> > + struct mtk_wdt_dev *data =
> > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > +
> > + spin_lock_irqsave(&data->lock, flags);
> > +
> > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > + tmp &= ~BIT(id);
> > + tmp |= WDT_SWSYS_RST_KEY;
> > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > +
> > + spin_unlock_irqrestore(&data->lock, flags);
> > +
> > + return 0;
> > +}
>
> There is a lot of duplication in those functions. Only one line
> is different. I think this is a good example where a helper function
> with an additional argument indicating set or reset would be helpful.
>
> > +
> > +static int toprgu_reset(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + int ret;
> > +
> > + ret = toprgu_reset_assert(rcdev, id);
> > + if (ret)
> > + return ret;
> > +
> > + return toprgu_reset_deassert(rcdev, id);
> > +}
> > +
> > +static const struct reset_control_ops toprgu_reset_ops = {
> > + .assert = toprgu_reset_assert,
> > + .deassert = toprgu_reset_deassert,
> > + .reset = toprgu_reset,
> > +};
> > +
> > +static int toprgu_register_reset_controller(struct platform_device *pdev,
> > + int rst_num)
> > +{
> > + int ret;
> > + struct mtk_wdt_dev *mtk_wdt = platform_get_drvdata(pdev);
> > +
> > + spin_lock_init(&mtk_wdt->lock);
> > +
> > + mtk_wdt->rcdev.owner = THIS_MODULE;
> > + mtk_wdt->rcdev.nr_resets = rst_num;
> > + mtk_wdt->rcdev.ops = &toprgu_reset_ops;
> > + mtk_wdt->rcdev.of_node = pdev->dev.of_node;
> > + ret = reset_controller_register(&mtk_wdt->rcdev);
I see this driver uses devm_kzalloc() below. Should this be
devm_reset_controller_register()?
> > + if (ret != 0)
> > + dev_err(&pdev->dev,
> > + "couldn't register wdt reset controller: %d\n", ret);
> > + return ret;
> > +}
> > +
> > static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
> > unsigned long action, void *data)
> > {
> > @@ -155,6 +254,7 @@ static int mtk_wdt_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > struct mtk_wdt_dev *mtk_wdt;
> > + struct mtk_wdt_data *wdt_data;
> > int err;
> >
> > mtk_wdt = devm_kzalloc(dev, sizeof(*mtk_wdt), GFP_KERNEL);
regards
Philipp
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [alsa-devel] [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
2019-11-25 9:51 ` Philipp Zabel
(?)
@ 2019-11-29 8:36 ` Yong Liang
-1 siblings, 0 replies; 21+ messages in thread
From: Yong Liang @ 2019-11-29 8:36 UTC (permalink / raw)
To: Philipp Zabel
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
broonie@kernel.org, lgirdwood@gmail.com,
Jiaxin Yu (俞家鑫), tzungbi@google.com,
robh+dt@kernel.org, linux-mediatek@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲), wim@linux-watchdog.org,
Guenter Roeck
On Mon, 2019-11-25 at 17:51 +0800, Philipp Zabel wrote:
> Hi,
>
> On Sun, 2019-11-24 at 22:16 -0800, Guenter Roeck wrote:
> > On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> > > From: "yong.liang" <yong.liang@mediatek.com>
> > >
> > > Add reset controller API in watchdog driver.
> > > Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> > > camera, codec and connectivity) software reset functionality.
> > >
> > > Signed-off-by: yong.liang <yong.liang@mediatek.com>
> > > Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> > > Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > > ---
> > > drivers/watchdog/Kconfig | 1 +
> > > drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> > > 2 files changed, 111 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > index 2e07caab9db2..629249fe5305 100644
> > > --- a/drivers/watchdog/Kconfig
> > > +++ b/drivers/watchdog/Kconfig
> > > @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> > > tristate "Mediatek SoCs watchdog support"
> > > depends on ARCH_MEDIATEK || COMPILE_TEST
> > > select WATCHDOG_CORE
> > > + select RESET_CONTROLLER
> > > help
> > > Say Y here to include support for the watchdog timer
> > > in Mediatek SoCs.
> > > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > > index 9c3d0033260d..d29484c7940a 100644
> > > --- a/drivers/watchdog/mtk_wdt.c
> > > +++ b/drivers/watchdog/mtk_wdt.c
> > > @@ -9,6 +9,9 @@
> > > * Based on sunxi_wdt.c
> > > */
> > >
> > > +#include <dt-bindings/reset-controller/mt2712-resets.h>
> > > +#include <dt-bindings/reset-controller/mt8183-resets.h>
> > > +#include <linux/delay.h>
> > > #include <linux/err.h>
> > > #include <linux/init.h>
> > > #include <linux/io.h>
> > > @@ -16,10 +19,12 @@
> > > #include <linux/module.h>
> > > #include <linux/moduleparam.h>
> > > #include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > #include <linux/platform_device.h>
> > > +#include <linux/reset-controller.h>
> > > +#include <linux/slab.h>
> > > #include <linux/types.h>
> > > #include <linux/watchdog.h>
> > > -#include <linux/delay.h>
> > >
> > > #define WDT_MAX_TIMEOUT 31
> > > #define WDT_MIN_TIMEOUT 1
> > > @@ -44,6 +49,9 @@
> > > #define WDT_SWRST 0x14
> > > #define WDT_SWRST_KEY 0x1209
> > >
> > > +#define WDT_SWSYSRST 0x18U
> > > +#define WDT_SWSYS_RST_KEY 0x88000000
> > > +
> > > #define DRV_NAME "mtk-wdt"
> > > #define DRV_VERSION "1.0"
> > >
> > > @@ -53,8 +61,99 @@ static unsigned int timeout;
> > > struct mtk_wdt_dev {
> > > struct watchdog_device wdt_dev;
> > > void __iomem *wdt_base;
> > > + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> > > + struct reset_controller_dev rcdev;
> > > +};
> > > +
> > > +struct mtk_wdt_data {
> > > + int sw_rst_num;
> > > };
> > >
> > > +static const struct mtk_wdt_data mt2712_data = {
> > > + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> > > +};
> > > +
> > > +static const struct mtk_wdt_data mt8183_data = {
> > > + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> > > +};
> >
> > The number of resets can be set in .data directly; there is no need
> > for the structures.
We want to put all properities in mtxxxx-resets.h and it easy to
manager. If there are new properity in the feture, we can put it in
mtk_wdt_data mtxxxx_data
> >
> > > +
> > > +static int toprgu_reset_assert(struct reset_controller_dev *rcdev,
> > > + unsigned long id)
> > > +{
> > > + unsigned int tmp;
> > > + unsigned long flags;
> > > + struct mtk_wdt_dev *data =
> > > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > > +
> > > + spin_lock_irqsave(&data->lock, flags);
> > > +
> > > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
>
> I think this should be readl_relaxed() instead. I don't expect this
> driver will ever be used on a big-endian architecture, but mixing
> __raw_readl() and writel() does look dangerous.
OK, I will change __raw_readl() to readl()
>
> > > + tmp |= BIT(id);
> > > + tmp |= WDT_SWSYS_RST_KEY;
> > > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > > +
> > > + spin_unlock_irqrestore(&data->lock, flags);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> > > + unsigned long id)
> > > +{
> > > + unsigned int tmp;
> > > + unsigned long flags;
> > > + struct mtk_wdt_dev *data =
> > > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > > +
> > > + spin_lock_irqsave(&data->lock, flags);
> > > +
> > > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > > + tmp &= ~BIT(id);
> > > + tmp |= WDT_SWSYS_RST_KEY;
> > > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > > +
> > > + spin_unlock_irqrestore(&data->lock, flags);
> > > +
> > > + return 0;
> > > +}
> >
> > There is a lot of duplication in those functions. Only one line
> > is different. I think this is a good example where a helper function
> > with an additional argument indicating set or reset would be helpful.
> >
.assert and .dessert are two numbers of struct reset_control_ops.
I think it's better to define both of them.
> > > +
> > > +static int toprgu_reset(struct reset_controller_dev *rcdev,
> > > + unsigned long id)
> > > +{
> > > + int ret;
> > > +
> > > + ret = toprgu_reset_assert(rcdev, id);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return toprgu_reset_deassert(rcdev, id);
> > > +}
> > > +
> > > +static const struct reset_control_ops toprgu_reset_ops = {
> > > + .assert = toprgu_reset_assert,
> > > + .deassert = toprgu_reset_deassert,
> > > + .reset = toprgu_reset,
> > > +};
> > > +
> > > +static int toprgu_register_reset_controller(struct platform_device *pdev,
> > > + int rst_num)
> > > +{
> > > + int ret;
> > > + struct mtk_wdt_dev *mtk_wdt = platform_get_drvdata(pdev);
> > > +
> > > + spin_lock_init(&mtk_wdt->lock);
> > > +
> > > + mtk_wdt->rcdev.owner = THIS_MODULE;
> > > + mtk_wdt->rcdev.nr_resets = rst_num;
> > > + mtk_wdt->rcdev.ops = &toprgu_reset_ops;
> > > + mtk_wdt->rcdev.of_node = pdev->dev.of_node;
> > > + ret = reset_controller_register(&mtk_wdt->rcdev);
>
> I see this driver uses devm_kzalloc() below. Should this be
> devm_reset_controller_register()?
>
> > > + if (ret != 0)
> > > + dev_err(&pdev->dev,
> > > + "couldn't register wdt reset controller: %d\n", ret);
> > > + return ret;
> > > +}
> > > +
> > > static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
> > > unsigned long action, void *data)
> > > {
> > > @@ -155,6 +254,7 @@ static int mtk_wdt_probe(struct platform_device *pdev)
> > > {
> > > struct device *dev = &pdev->dev;
> > > struct mtk_wdt_dev *mtk_wdt;
> > > + struct mtk_wdt_data *wdt_data;
> > > int err;
> > >
> > > mtk_wdt = devm_kzalloc(dev, sizeof(*mtk_wdt), GFP_KERNEL);
>
> regards
> Philipp
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
@ 2019-11-29 8:36 ` Yong Liang
0 siblings, 0 replies; 21+ messages in thread
From: Yong Liang @ 2019-11-29 8:36 UTC (permalink / raw)
To: Philipp Zabel
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
broonie@kernel.org, lgirdwood@gmail.com,
Jiaxin Yu (俞家鑫), perex@perex.cz,
tzungbi@google.com, robh+dt@kernel.org,
linux-mediatek@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲), wim@linux-watchdog.org,
Guenter Roeck
On Mon, 2019-11-25 at 17:51 +0800, Philipp Zabel wrote:
> Hi,
>
> On Sun, 2019-11-24 at 22:16 -0800, Guenter Roeck wrote:
> > On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> > > From: "yong.liang" <yong.liang@mediatek.com>
> > >
> > > Add reset controller API in watchdog driver.
> > > Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> > > camera, codec and connectivity) software reset functionality.
> > >
> > > Signed-off-by: yong.liang <yong.liang@mediatek.com>
> > > Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> > > Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > > ---
> > > drivers/watchdog/Kconfig | 1 +
> > > drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> > > 2 files changed, 111 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > index 2e07caab9db2..629249fe5305 100644
> > > --- a/drivers/watchdog/Kconfig
> > > +++ b/drivers/watchdog/Kconfig
> > > @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> > > tristate "Mediatek SoCs watchdog support"
> > > depends on ARCH_MEDIATEK || COMPILE_TEST
> > > select WATCHDOG_CORE
> > > + select RESET_CONTROLLER
> > > help
> > > Say Y here to include support for the watchdog timer
> > > in Mediatek SoCs.
> > > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > > index 9c3d0033260d..d29484c7940a 100644
> > > --- a/drivers/watchdog/mtk_wdt.c
> > > +++ b/drivers/watchdog/mtk_wdt.c
> > > @@ -9,6 +9,9 @@
> > > * Based on sunxi_wdt.c
> > > */
> > >
> > > +#include <dt-bindings/reset-controller/mt2712-resets.h>
> > > +#include <dt-bindings/reset-controller/mt8183-resets.h>
> > > +#include <linux/delay.h>
> > > #include <linux/err.h>
> > > #include <linux/init.h>
> > > #include <linux/io.h>
> > > @@ -16,10 +19,12 @@
> > > #include <linux/module.h>
> > > #include <linux/moduleparam.h>
> > > #include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > #include <linux/platform_device.h>
> > > +#include <linux/reset-controller.h>
> > > +#include <linux/slab.h>
> > > #include <linux/types.h>
> > > #include <linux/watchdog.h>
> > > -#include <linux/delay.h>
> > >
> > > #define WDT_MAX_TIMEOUT 31
> > > #define WDT_MIN_TIMEOUT 1
> > > @@ -44,6 +49,9 @@
> > > #define WDT_SWRST 0x14
> > > #define WDT_SWRST_KEY 0x1209
> > >
> > > +#define WDT_SWSYSRST 0x18U
> > > +#define WDT_SWSYS_RST_KEY 0x88000000
> > > +
> > > #define DRV_NAME "mtk-wdt"
> > > #define DRV_VERSION "1.0"
> > >
> > > @@ -53,8 +61,99 @@ static unsigned int timeout;
> > > struct mtk_wdt_dev {
> > > struct watchdog_device wdt_dev;
> > > void __iomem *wdt_base;
> > > + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> > > + struct reset_controller_dev rcdev;
> > > +};
> > > +
> > > +struct mtk_wdt_data {
> > > + int sw_rst_num;
> > > };
> > >
> > > +static const struct mtk_wdt_data mt2712_data = {
> > > + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> > > +};
> > > +
> > > +static const struct mtk_wdt_data mt8183_data = {
> > > + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> > > +};
> >
> > The number of resets can be set in .data directly; there is no need
> > for the structures.
We want to put all properities in mtxxxx-resets.h and it easy to
manager. If there are new properity in the feture, we can put it in
mtk_wdt_data mtxxxx_data
> >
> > > +
> > > +static int toprgu_reset_assert(struct reset_controller_dev *rcdev,
> > > + unsigned long id)
> > > +{
> > > + unsigned int tmp;
> > > + unsigned long flags;
> > > + struct mtk_wdt_dev *data =
> > > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > > +
> > > + spin_lock_irqsave(&data->lock, flags);
> > > +
> > > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
>
> I think this should be readl_relaxed() instead. I don't expect this
> driver will ever be used on a big-endian architecture, but mixing
> __raw_readl() and writel() does look dangerous.
OK, I will change __raw_readl() to readl()
>
> > > + tmp |= BIT(id);
> > > + tmp |= WDT_SWSYS_RST_KEY;
> > > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > > +
> > > + spin_unlock_irqrestore(&data->lock, flags);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> > > + unsigned long id)
> > > +{
> > > + unsigned int tmp;
> > > + unsigned long flags;
> > > + struct mtk_wdt_dev *data =
> > > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > > +
> > > + spin_lock_irqsave(&data->lock, flags);
> > > +
> > > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > > + tmp &= ~BIT(id);
> > > + tmp |= WDT_SWSYS_RST_KEY;
> > > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > > +
> > > + spin_unlock_irqrestore(&data->lock, flags);
> > > +
> > > + return 0;
> > > +}
> >
> > There is a lot of duplication in those functions. Only one line
> > is different. I think this is a good example where a helper function
> > with an additional argument indicating set or reset would be helpful.
> >
.assert and .dessert are two numbers of struct reset_control_ops.
I think it's better to define both of them.
> > > +
> > > +static int toprgu_reset(struct reset_controller_dev *rcdev,
> > > + unsigned long id)
> > > +{
> > > + int ret;
> > > +
> > > + ret = toprgu_reset_assert(rcdev, id);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return toprgu_reset_deassert(rcdev, id);
> > > +}
> > > +
> > > +static const struct reset_control_ops toprgu_reset_ops = {
> > > + .assert = toprgu_reset_assert,
> > > + .deassert = toprgu_reset_deassert,
> > > + .reset = toprgu_reset,
> > > +};
> > > +
> > > +static int toprgu_register_reset_controller(struct platform_device *pdev,
> > > + int rst_num)
> > > +{
> > > + int ret;
> > > + struct mtk_wdt_dev *mtk_wdt = platform_get_drvdata(pdev);
> > > +
> > > + spin_lock_init(&mtk_wdt->lock);
> > > +
> > > + mtk_wdt->rcdev.owner = THIS_MODULE;
> > > + mtk_wdt->rcdev.nr_resets = rst_num;
> > > + mtk_wdt->rcdev.ops = &toprgu_reset_ops;
> > > + mtk_wdt->rcdev.of_node = pdev->dev.of_node;
> > > + ret = reset_controller_register(&mtk_wdt->rcdev);
>
> I see this driver uses devm_kzalloc() below. Should this be
> devm_reset_controller_register()?
>
> > > + if (ret != 0)
> > > + dev_err(&pdev->dev,
> > > + "couldn't register wdt reset controller: %d\n", ret);
> > > + return ret;
> > > +}
> > > +
> > > static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
> > > unsigned long action, void *data)
> > > {
> > > @@ -155,6 +254,7 @@ static int mtk_wdt_probe(struct platform_device *pdev)
> > > {
> > > struct device *dev = &pdev->dev;
> > > struct mtk_wdt_dev *mtk_wdt;
> > > + struct mtk_wdt_data *wdt_data;
> > > int err;
> > >
> > > mtk_wdt = devm_kzalloc(dev, sizeof(*mtk_wdt), GFP_KERNEL);
>
> regards
> Philipp
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
@ 2019-11-29 8:36 ` Yong Liang
0 siblings, 0 replies; 21+ messages in thread
From: Yong Liang @ 2019-11-29 8:36 UTC (permalink / raw)
To: Philipp Zabel
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
broonie@kernel.org, lgirdwood@gmail.com,
Jiaxin Yu (俞家鑫), perex@perex.cz,
tzungbi@google.com, robh+dt@kernel.org,
linux-mediatek@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲), wim@linux-watchdog.org,
Guenter Roeck
On Mon, 2019-11-25 at 17:51 +0800, Philipp Zabel wrote:
> Hi,
>
> On Sun, 2019-11-24 at 22:16 -0800, Guenter Roeck wrote:
> > On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> > > From: "yong.liang" <yong.liang@mediatek.com>
> > >
> > > Add reset controller API in watchdog driver.
> > > Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> > > camera, codec and connectivity) software reset functionality.
> > >
> > > Signed-off-by: yong.liang <yong.liang@mediatek.com>
> > > Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> > > Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > > ---
> > > drivers/watchdog/Kconfig | 1 +
> > > drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> > > 2 files changed, 111 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > index 2e07caab9db2..629249fe5305 100644
> > > --- a/drivers/watchdog/Kconfig
> > > +++ b/drivers/watchdog/Kconfig
> > > @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> > > tristate "Mediatek SoCs watchdog support"
> > > depends on ARCH_MEDIATEK || COMPILE_TEST
> > > select WATCHDOG_CORE
> > > + select RESET_CONTROLLER
> > > help
> > > Say Y here to include support for the watchdog timer
> > > in Mediatek SoCs.
> > > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > > index 9c3d0033260d..d29484c7940a 100644
> > > --- a/drivers/watchdog/mtk_wdt.c
> > > +++ b/drivers/watchdog/mtk_wdt.c
> > > @@ -9,6 +9,9 @@
> > > * Based on sunxi_wdt.c
> > > */
> > >
> > > +#include <dt-bindings/reset-controller/mt2712-resets.h>
> > > +#include <dt-bindings/reset-controller/mt8183-resets.h>
> > > +#include <linux/delay.h>
> > > #include <linux/err.h>
> > > #include <linux/init.h>
> > > #include <linux/io.h>
> > > @@ -16,10 +19,12 @@
> > > #include <linux/module.h>
> > > #include <linux/moduleparam.h>
> > > #include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > #include <linux/platform_device.h>
> > > +#include <linux/reset-controller.h>
> > > +#include <linux/slab.h>
> > > #include <linux/types.h>
> > > #include <linux/watchdog.h>
> > > -#include <linux/delay.h>
> > >
> > > #define WDT_MAX_TIMEOUT 31
> > > #define WDT_MIN_TIMEOUT 1
> > > @@ -44,6 +49,9 @@
> > > #define WDT_SWRST 0x14
> > > #define WDT_SWRST_KEY 0x1209
> > >
> > > +#define WDT_SWSYSRST 0x18U
> > > +#define WDT_SWSYS_RST_KEY 0x88000000
> > > +
> > > #define DRV_NAME "mtk-wdt"
> > > #define DRV_VERSION "1.0"
> > >
> > > @@ -53,8 +61,99 @@ static unsigned int timeout;
> > > struct mtk_wdt_dev {
> > > struct watchdog_device wdt_dev;
> > > void __iomem *wdt_base;
> > > + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> > > + struct reset_controller_dev rcdev;
> > > +};
> > > +
> > > +struct mtk_wdt_data {
> > > + int sw_rst_num;
> > > };
> > >
> > > +static const struct mtk_wdt_data mt2712_data = {
> > > + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> > > +};
> > > +
> > > +static const struct mtk_wdt_data mt8183_data = {
> > > + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> > > +};
> >
> > The number of resets can be set in .data directly; there is no need
> > for the structures.
We want to put all properities in mtxxxx-resets.h and it easy to
manager. If there are new properity in the feture, we can put it in
mtk_wdt_data mtxxxx_data
> >
> > > +
> > > +static int toprgu_reset_assert(struct reset_controller_dev *rcdev,
> > > + unsigned long id)
> > > +{
> > > + unsigned int tmp;
> > > + unsigned long flags;
> > > + struct mtk_wdt_dev *data =
> > > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > > +
> > > + spin_lock_irqsave(&data->lock, flags);
> > > +
> > > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
>
> I think this should be readl_relaxed() instead. I don't expect this
> driver will ever be used on a big-endian architecture, but mixing
> __raw_readl() and writel() does look dangerous.
OK, I will change __raw_readl() to readl()
>
> > > + tmp |= BIT(id);
> > > + tmp |= WDT_SWSYS_RST_KEY;
> > > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > > +
> > > + spin_unlock_irqrestore(&data->lock, flags);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> > > + unsigned long id)
> > > +{
> > > + unsigned int tmp;
> > > + unsigned long flags;
> > > + struct mtk_wdt_dev *data =
> > > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > > +
> > > + spin_lock_irqsave(&data->lock, flags);
> > > +
> > > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > > + tmp &= ~BIT(id);
> > > + tmp |= WDT_SWSYS_RST_KEY;
> > > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > > +
> > > + spin_unlock_irqrestore(&data->lock, flags);
> > > +
> > > + return 0;
> > > +}
> >
> > There is a lot of duplication in those functions. Only one line
> > is different. I think this is a good example where a helper function
> > with an additional argument indicating set or reset would be helpful.
> >
.assert and .dessert are two numbers of struct reset_control_ops.
I think it's better to define both of them.
> > > +
> > > +static int toprgu_reset(struct reset_controller_dev *rcdev,
> > > + unsigned long id)
> > > +{
> > > + int ret;
> > > +
> > > + ret = toprgu_reset_assert(rcdev, id);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return toprgu_reset_deassert(rcdev, id);
> > > +}
> > > +
> > > +static const struct reset_control_ops toprgu_reset_ops = {
> > > + .assert = toprgu_reset_assert,
> > > + .deassert = toprgu_reset_deassert,
> > > + .reset = toprgu_reset,
> > > +};
> > > +
> > > +static int toprgu_register_reset_controller(struct platform_device *pdev,
> > > + int rst_num)
> > > +{
> > > + int ret;
> > > + struct mtk_wdt_dev *mtk_wdt = platform_get_drvdata(pdev);
> > > +
> > > + spin_lock_init(&mtk_wdt->lock);
> > > +
> > > + mtk_wdt->rcdev.owner = THIS_MODULE;
> > > + mtk_wdt->rcdev.nr_resets = rst_num;
> > > + mtk_wdt->rcdev.ops = &toprgu_reset_ops;
> > > + mtk_wdt->rcdev.of_node = pdev->dev.of_node;
> > > + ret = reset_controller_register(&mtk_wdt->rcdev);
>
> I see this driver uses devm_kzalloc() below. Should this be
> devm_reset_controller_register()?
>
> > > + if (ret != 0)
> > > + dev_err(&pdev->dev,
> > > + "couldn't register wdt reset controller: %d\n", ret);
> > > + return ret;
> > > +}
> > > +
> > > static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
> > > unsigned long action, void *data)
> > > {
> > > @@ -155,6 +254,7 @@ static int mtk_wdt_probe(struct platform_device *pdev)
> > > {
> > > struct device *dev = &pdev->dev;
> > > struct mtk_wdt_dev *mtk_wdt;
> > > + struct mtk_wdt_data *wdt_data;
> > > int err;
> > >
> > > mtk_wdt = devm_kzalloc(dev, sizeof(*mtk_wdt), GFP_KERNEL);
>
> regards
> Philipp
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [alsa-devel] [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
2019-11-29 8:36 ` Yong Liang
(?)
@ 2019-12-02 13:02 ` Philipp Zabel
-1 siblings, 0 replies; 21+ messages in thread
From: Philipp Zabel @ 2019-12-02 13:02 UTC (permalink / raw)
To: Yong Liang
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
broonie@kernel.org, lgirdwood@gmail.com,
Jiaxin Yu (俞家鑫), tzungbi@google.com,
robh+dt@kernel.org, linux-mediatek@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲), wim@linux-watchdog.org,
Guenter Roeck
On Fri, 2019-11-29 at 16:36 +0800, Yong Liang wrote:
> On Mon, 2019-11-25 at 17:51 +0800, Philipp Zabel wrote:
> > On Sun, 2019-11-24 at 22:16 -0800, Guenter Roeck wrote:
> > > On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> > > > From: "yong.liang" <yong.liang@mediatek.com>
> > > >
> > > > Add reset controller API in watchdog driver.
> > > > Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> > > > camera, codec and connectivity) software reset functionality.
> > > >
> > > > Signed-off-by: yong.liang <yong.liang@mediatek.com>
> > > > Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> > > > Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > > > ---
> > > > drivers/watchdog/Kconfig | 1 +
> > > > drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> > > > 2 files changed, 111 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > > index 2e07caab9db2..629249fe5305 100644
> > > > --- a/drivers/watchdog/Kconfig
> > > > +++ b/drivers/watchdog/Kconfig
> > > > @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> > > > tristate "Mediatek SoCs watchdog support"
> > > > depends on ARCH_MEDIATEK || COMPILE_TEST
> > > > select WATCHDOG_CORE
> > > > + select RESET_CONTROLLER
> > > > help
> > > > Say Y here to include support for the watchdog timer
> > > > in Mediatek SoCs.
> > > > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > > > index 9c3d0033260d..d29484c7940a 100644
> > > > --- a/drivers/watchdog/mtk_wdt.c
> > > > +++ b/drivers/watchdog/mtk_wdt.c
> > > > @@ -9,6 +9,9 @@
> > > > * Based on sunxi_wdt.c
> > > > */
> > > >
> > > > +#include <dt-bindings/reset-controller/mt2712-resets.h>
> > > > +#include <dt-bindings/reset-controller/mt8183-resets.h>
> > > > +#include <linux/delay.h>
> > > > #include <linux/err.h>
> > > > #include <linux/init.h>
> > > > #include <linux/io.h>
> > > > @@ -16,10 +19,12 @@
> > > > #include <linux/module.h>
> > > > #include <linux/moduleparam.h>
> > > > #include <linux/of.h>
> > > > +#include <linux/of_device.h>
> > > > #include <linux/platform_device.h>
> > > > +#include <linux/reset-controller.h>
> > > > +#include <linux/slab.h>
> > > > #include <linux/types.h>
> > > > #include <linux/watchdog.h>
> > > > -#include <linux/delay.h>
> > > >
> > > > #define WDT_MAX_TIMEOUT 31
> > > > #define WDT_MIN_TIMEOUT 1
> > > > @@ -44,6 +49,9 @@
> > > > #define WDT_SWRST 0x14
> > > > #define WDT_SWRST_KEY 0x1209
> > > >
> > > > +#define WDT_SWSYSRST 0x18U
> > > > +#define WDT_SWSYS_RST_KEY 0x88000000
> > > > +
> > > > #define DRV_NAME "mtk-wdt"
> > > > #define DRV_VERSION "1.0"
> > > >
> > > > @@ -53,8 +61,99 @@ static unsigned int timeout;
> > > > struct mtk_wdt_dev {
> > > > struct watchdog_device wdt_dev;
> > > > void __iomem *wdt_base;
> > > > + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> > > > + struct reset_controller_dev rcdev;
> > > > +};
> > > > +
> > > > +struct mtk_wdt_data {
> > > > + int sw_rst_num;
> > > > };
> > > >
> > > > +static const struct mtk_wdt_data mt2712_data = {
> > > > + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> > > > +};
> > > > +
> > > > +static const struct mtk_wdt_data mt8183_data = {
> > > > + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> > > > +};
> > >
> > > The number of resets can be set in .data directly; there is no need
> > > for the structures.
>
> We want to put all properities in mtxxxx-resets.h and it easy to
> manager. If there are new properity in the feture, we can put it in
> mtk_wdt_data mtxxxx_data
Do you expect there will be more properties in the future?
> > > > +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> > > > + unsigned long id)
> > > > +{
> > > > + unsigned int tmp;
> > > > + unsigned long flags;
> > > > + struct mtk_wdt_dev *data =
> > > > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > > > +
> > > > + spin_lock_irqsave(&data->lock, flags);
> > > > +
> > > > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > > > + tmp &= ~BIT(id);
> > > > + tmp |= WDT_SWSYS_RST_KEY;
> > > > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > > > +
> > > > + spin_unlock_irqrestore(&data->lock, flags);
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > There is a lot of duplication in those functions. Only one line
> > > is different. I think this is a good example where a helper function
> > > with an additional argument indicating set or reset would be helpful.
> > >
> .assert and .dessert are two numbers of struct reset_control_ops.
> I think it's better to define both of them.
The suggestion was to have two very short _assert and _deassert
functions that only contain a single call to a common helper function.
See the reset-a10sr.c driver for an example.
regards
Philipp
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
@ 2019-12-02 13:02 ` Philipp Zabel
0 siblings, 0 replies; 21+ messages in thread
From: Philipp Zabel @ 2019-12-02 13:02 UTC (permalink / raw)
To: Yong Liang
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
broonie@kernel.org, lgirdwood@gmail.com,
Jiaxin Yu (俞家鑫), perex@perex.cz,
tzungbi@google.com, robh+dt@kernel.org,
linux-mediatek@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲), wim@linux-watchdog.org,
Guenter Roeck
On Fri, 2019-11-29 at 16:36 +0800, Yong Liang wrote:
> On Mon, 2019-11-25 at 17:51 +0800, Philipp Zabel wrote:
> > On Sun, 2019-11-24 at 22:16 -0800, Guenter Roeck wrote:
> > > On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> > > > From: "yong.liang" <yong.liang@mediatek.com>
> > > >
> > > > Add reset controller API in watchdog driver.
> > > > Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> > > > camera, codec and connectivity) software reset functionality.
> > > >
> > > > Signed-off-by: yong.liang <yong.liang@mediatek.com>
> > > > Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> > > > Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > > > ---
> > > > drivers/watchdog/Kconfig | 1 +
> > > > drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> > > > 2 files changed, 111 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > > index 2e07caab9db2..629249fe5305 100644
> > > > --- a/drivers/watchdog/Kconfig
> > > > +++ b/drivers/watchdog/Kconfig
> > > > @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> > > > tristate "Mediatek SoCs watchdog support"
> > > > depends on ARCH_MEDIATEK || COMPILE_TEST
> > > > select WATCHDOG_CORE
> > > > + select RESET_CONTROLLER
> > > > help
> > > > Say Y here to include support for the watchdog timer
> > > > in Mediatek SoCs.
> > > > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > > > index 9c3d0033260d..d29484c7940a 100644
> > > > --- a/drivers/watchdog/mtk_wdt.c
> > > > +++ b/drivers/watchdog/mtk_wdt.c
> > > > @@ -9,6 +9,9 @@
> > > > * Based on sunxi_wdt.c
> > > > */
> > > >
> > > > +#include <dt-bindings/reset-controller/mt2712-resets.h>
> > > > +#include <dt-bindings/reset-controller/mt8183-resets.h>
> > > > +#include <linux/delay.h>
> > > > #include <linux/err.h>
> > > > #include <linux/init.h>
> > > > #include <linux/io.h>
> > > > @@ -16,10 +19,12 @@
> > > > #include <linux/module.h>
> > > > #include <linux/moduleparam.h>
> > > > #include <linux/of.h>
> > > > +#include <linux/of_device.h>
> > > > #include <linux/platform_device.h>
> > > > +#include <linux/reset-controller.h>
> > > > +#include <linux/slab.h>
> > > > #include <linux/types.h>
> > > > #include <linux/watchdog.h>
> > > > -#include <linux/delay.h>
> > > >
> > > > #define WDT_MAX_TIMEOUT 31
> > > > #define WDT_MIN_TIMEOUT 1
> > > > @@ -44,6 +49,9 @@
> > > > #define WDT_SWRST 0x14
> > > > #define WDT_SWRST_KEY 0x1209
> > > >
> > > > +#define WDT_SWSYSRST 0x18U
> > > > +#define WDT_SWSYS_RST_KEY 0x88000000
> > > > +
> > > > #define DRV_NAME "mtk-wdt"
> > > > #define DRV_VERSION "1.0"
> > > >
> > > > @@ -53,8 +61,99 @@ static unsigned int timeout;
> > > > struct mtk_wdt_dev {
> > > > struct watchdog_device wdt_dev;
> > > > void __iomem *wdt_base;
> > > > + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> > > > + struct reset_controller_dev rcdev;
> > > > +};
> > > > +
> > > > +struct mtk_wdt_data {
> > > > + int sw_rst_num;
> > > > };
> > > >
> > > > +static const struct mtk_wdt_data mt2712_data = {
> > > > + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> > > > +};
> > > > +
> > > > +static const struct mtk_wdt_data mt8183_data = {
> > > > + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> > > > +};
> > >
> > > The number of resets can be set in .data directly; there is no need
> > > for the structures.
>
> We want to put all properities in mtxxxx-resets.h and it easy to
> manager. If there are new properity in the feture, we can put it in
> mtk_wdt_data mtxxxx_data
Do you expect there will be more properties in the future?
> > > > +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> > > > + unsigned long id)
> > > > +{
> > > > + unsigned int tmp;
> > > > + unsigned long flags;
> > > > + struct mtk_wdt_dev *data =
> > > > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > > > +
> > > > + spin_lock_irqsave(&data->lock, flags);
> > > > +
> > > > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > > > + tmp &= ~BIT(id);
> > > > + tmp |= WDT_SWSYS_RST_KEY;
> > > > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > > > +
> > > > + spin_unlock_irqrestore(&data->lock, flags);
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > There is a lot of duplication in those functions. Only one line
> > > is different. I think this is a good example where a helper function
> > > with an additional argument indicating set or reset would be helpful.
> > >
> .assert and .dessert are two numbers of struct reset_control_ops.
> I think it's better to define both of them.
The suggestion was to have two very short _assert and _deassert
functions that only contain a single call to a common helper function.
See the reset-a10sr.c driver for an example.
regards
Philipp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
@ 2019-12-02 13:02 ` Philipp Zabel
0 siblings, 0 replies; 21+ messages in thread
From: Philipp Zabel @ 2019-12-02 13:02 UTC (permalink / raw)
To: Yong Liang
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
broonie@kernel.org, lgirdwood@gmail.com,
Jiaxin Yu (俞家鑫), perex@perex.cz,
tzungbi@google.com, robh+dt@kernel.org,
linux-mediatek@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲), wim@linux-watchdog.org,
Guenter Roeck
On Fri, 2019-11-29 at 16:36 +0800, Yong Liang wrote:
> On Mon, 2019-11-25 at 17:51 +0800, Philipp Zabel wrote:
> > On Sun, 2019-11-24 at 22:16 -0800, Guenter Roeck wrote:
> > > On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> > > > From: "yong.liang" <yong.liang@mediatek.com>
> > > >
> > > > Add reset controller API in watchdog driver.
> > > > Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> > > > camera, codec and connectivity) software reset functionality.
> > > >
> > > > Signed-off-by: yong.liang <yong.liang@mediatek.com>
> > > > Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> > > > Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > > > ---
> > > > drivers/watchdog/Kconfig | 1 +
> > > > drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> > > > 2 files changed, 111 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > > index 2e07caab9db2..629249fe5305 100644
> > > > --- a/drivers/watchdog/Kconfig
> > > > +++ b/drivers/watchdog/Kconfig
> > > > @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> > > > tristate "Mediatek SoCs watchdog support"
> > > > depends on ARCH_MEDIATEK || COMPILE_TEST
> > > > select WATCHDOG_CORE
> > > > + select RESET_CONTROLLER
> > > > help
> > > > Say Y here to include support for the watchdog timer
> > > > in Mediatek SoCs.
> > > > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > > > index 9c3d0033260d..d29484c7940a 100644
> > > > --- a/drivers/watchdog/mtk_wdt.c
> > > > +++ b/drivers/watchdog/mtk_wdt.c
> > > > @@ -9,6 +9,9 @@
> > > > * Based on sunxi_wdt.c
> > > > */
> > > >
> > > > +#include <dt-bindings/reset-controller/mt2712-resets.h>
> > > > +#include <dt-bindings/reset-controller/mt8183-resets.h>
> > > > +#include <linux/delay.h>
> > > > #include <linux/err.h>
> > > > #include <linux/init.h>
> > > > #include <linux/io.h>
> > > > @@ -16,10 +19,12 @@
> > > > #include <linux/module.h>
> > > > #include <linux/moduleparam.h>
> > > > #include <linux/of.h>
> > > > +#include <linux/of_device.h>
> > > > #include <linux/platform_device.h>
> > > > +#include <linux/reset-controller.h>
> > > > +#include <linux/slab.h>
> > > > #include <linux/types.h>
> > > > #include <linux/watchdog.h>
> > > > -#include <linux/delay.h>
> > > >
> > > > #define WDT_MAX_TIMEOUT 31
> > > > #define WDT_MIN_TIMEOUT 1
> > > > @@ -44,6 +49,9 @@
> > > > #define WDT_SWRST 0x14
> > > > #define WDT_SWRST_KEY 0x1209
> > > >
> > > > +#define WDT_SWSYSRST 0x18U
> > > > +#define WDT_SWSYS_RST_KEY 0x88000000
> > > > +
> > > > #define DRV_NAME "mtk-wdt"
> > > > #define DRV_VERSION "1.0"
> > > >
> > > > @@ -53,8 +61,99 @@ static unsigned int timeout;
> > > > struct mtk_wdt_dev {
> > > > struct watchdog_device wdt_dev;
> > > > void __iomem *wdt_base;
> > > > + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> > > > + struct reset_controller_dev rcdev;
> > > > +};
> > > > +
> > > > +struct mtk_wdt_data {
> > > > + int sw_rst_num;
> > > > };
> > > >
> > > > +static const struct mtk_wdt_data mt2712_data = {
> > > > + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> > > > +};
> > > > +
> > > > +static const struct mtk_wdt_data mt8183_data = {
> > > > + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> > > > +};
> > >
> > > The number of resets can be set in .data directly; there is no need
> > > for the structures.
>
> We want to put all properities in mtxxxx-resets.h and it easy to
> manager. If there are new properity in the feture, we can put it in
> mtk_wdt_data mtxxxx_data
Do you expect there will be more properties in the future?
> > > > +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> > > > + unsigned long id)
> > > > +{
> > > > + unsigned int tmp;
> > > > + unsigned long flags;
> > > > + struct mtk_wdt_dev *data =
> > > > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > > > +
> > > > + spin_lock_irqsave(&data->lock, flags);
> > > > +
> > > > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > > > + tmp &= ~BIT(id);
> > > > + tmp |= WDT_SWSYS_RST_KEY;
> > > > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > > > +
> > > > + spin_unlock_irqrestore(&data->lock, flags);
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > There is a lot of duplication in those functions. Only one line
> > > is different. I think this is a good example where a helper function
> > > with an additional argument indicating set or reset would be helpful.
> > >
> .assert and .dessert are two numbers of struct reset_control_ops.
> I think it's better to define both of them.
The suggestion was to have two very short _assert and _deassert
functions that only contain a single call to a common helper function.
See the reset-a10sr.c driver for an example.
regards
Philipp
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [alsa-devel] [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
2019-12-02 13:02 ` Philipp Zabel
(?)
@ 2019-12-03 3:02 ` Yong Liang
-1 siblings, 0 replies; 21+ messages in thread
From: Yong Liang @ 2019-12-03 3:02 UTC (permalink / raw)
To: Philipp Zabel
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
robh+dt@kernel.org, lgirdwood@gmail.com,
Jiaxin Yu (俞家鑫), tzungbi@google.com,
broonie@kernel.org, linux-mediatek@lists.infradead.org,
Guenter Roeck, Eason Yen (顏廷任),
Yingjoe Chen (陳英洲), wim@linux-watchdog.org,
linux-arm-kernel@lists.infradead.org
On Mon, 2019-12-02 at 21:02 +0800, Philipp Zabel wrote:
> On Fri, 2019-11-29 at 16:36 +0800, Yong Liang wrote:
> > On Mon, 2019-11-25 at 17:51 +0800, Philipp Zabel wrote:
> > > On Sun, 2019-11-24 at 22:16 -0800, Guenter Roeck wrote:
> > > > On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> > > > > From: "yong.liang" <yong.liang@mediatek.com>
> > > > >
> > > > > Add reset controller API in watchdog driver.
> > > > > Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> > > > > camera, codec and connectivity) software reset functionality.
> > > > >
> > > > > Signed-off-by: yong.liang <yong.liang@mediatek.com>
> > > > > Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> > > > > Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > > > > ---
> > > > > drivers/watchdog/Kconfig | 1 +
> > > > > drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> > > > > 2 files changed, 111 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > > > index 2e07caab9db2..629249fe5305 100644
> > > > > --- a/drivers/watchdog/Kconfig
> > > > > +++ b/drivers/watchdog/Kconfig
> > > > > @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> > > > > tristate "Mediatek SoCs watchdog support"
> > > > > depends on ARCH_MEDIATEK || COMPILE_TEST
> > > > > select WATCHDOG_CORE
> > > > > + select RESET_CONTROLLER
> > > > > help
> > > > > Say Y here to include support for the watchdog timer
> > > > > in Mediatek SoCs.
> > > > > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > > > > index 9c3d0033260d..d29484c7940a 100644
> > > > > --- a/drivers/watchdog/mtk_wdt.c
> > > > > +++ b/drivers/watchdog/mtk_wdt.c
> > > > > @@ -9,6 +9,9 @@
> > > > > * Based on sunxi_wdt.c
> > > > > */
> > > > >
> > > > > +#include <dt-bindings/reset-controller/mt2712-resets.h>
> > > > > +#include <dt-bindings/reset-controller/mt8183-resets.h>
> > > > > +#include <linux/delay.h>
> > > > > #include <linux/err.h>
> > > > > #include <linux/init.h>
> > > > > #include <linux/io.h>
> > > > > @@ -16,10 +19,12 @@
> > > > > #include <linux/module.h>
> > > > > #include <linux/moduleparam.h>
> > > > > #include <linux/of.h>
> > > > > +#include <linux/of_device.h>
> > > > > #include <linux/platform_device.h>
> > > > > +#include <linux/reset-controller.h>
> > > > > +#include <linux/slab.h>
> > > > > #include <linux/types.h>
> > > > > #include <linux/watchdog.h>
> > > > > -#include <linux/delay.h>
> > > > >
> > > > > #define WDT_MAX_TIMEOUT 31
> > > > > #define WDT_MIN_TIMEOUT 1
> > > > > @@ -44,6 +49,9 @@
> > > > > #define WDT_SWRST 0x14
> > > > > #define WDT_SWRST_KEY 0x1209
> > > > >
> > > > > +#define WDT_SWSYSRST 0x18U
> > > > > +#define WDT_SWSYS_RST_KEY 0x88000000
> > > > > +
> > > > > #define DRV_NAME "mtk-wdt"
> > > > > #define DRV_VERSION "1.0"
> > > > >
> > > > > @@ -53,8 +61,99 @@ static unsigned int timeout;
> > > > > struct mtk_wdt_dev {
> > > > > struct watchdog_device wdt_dev;
> > > > > void __iomem *wdt_base;
> > > > > + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> > > > > + struct reset_controller_dev rcdev;
> > > > > +};
> > > > > +
> > > > > +struct mtk_wdt_data {
> > > > > + int sw_rst_num;
> > > > > };
> > > > >
> > > > > +static const struct mtk_wdt_data mt2712_data = {
> > > > > + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> > > > > +};
> > > > > +
> > > > > +static const struct mtk_wdt_data mt8183_data = {
> > > > > + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> > > > > +};
> > > >
> > > > The number of resets can be set in .data directly; there is no need
> > > > for the structures.
> >
> > We want to put all properities in mtxxxx-resets.h and it easy to
> > manager. If there are new properity in the feture, we can put it in
> > mtk_wdt_data mtxxxx_data
>
> Do you expect there will be more properties in the future?
Yes, We may put some infra reset bit and max number in mtxxxx-resets.h
>
> > > > > +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> > > > > + unsigned long id)
> > > > > +{
> > > > > + unsigned int tmp;
> > > > > + unsigned long flags;
> > > > > + struct mtk_wdt_dev *data =
> > > > > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > > > > +
> > > > > + spin_lock_irqsave(&data->lock, flags);
> > > > > +
> > > > > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > > > > + tmp &= ~BIT(id);
> > > > > + tmp |= WDT_SWSYS_RST_KEY;
> > > > > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > > > > +
> > > > > + spin_unlock_irqrestore(&data->lock, flags);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > >
> > > > There is a lot of duplication in those functions. Only one line
> > > > is different. I think this is a good example where a helper function
> > > > with an additional argument indicating set or reset would be helpful.
> > > >
> > .assert and .dessert are two numbers of struct reset_control_ops.
> > I think it's better to define both of them.
>
> The suggestion was to have two very short _assert and _deassert
> functions that only contain a single call to a common helper function.
> See the reset-a10sr.c driver for an example.
OK. I will modify it as reset-a10sr.c do.
>
> regards
> Philipp
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
@ 2019-12-03 3:02 ` Yong Liang
0 siblings, 0 replies; 21+ messages in thread
From: Yong Liang @ 2019-12-03 3:02 UTC (permalink / raw)
To: Philipp Zabel
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
robh+dt@kernel.org, lgirdwood@gmail.com,
Jiaxin Yu (俞家鑫), perex@perex.cz,
tzungbi@google.com, broonie@kernel.org,
linux-mediatek@lists.infradead.org, Guenter Roeck,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲), wim@linux-watchdog.org,
linux-arm-kernel@lists.infradead.org
On Mon, 2019-12-02 at 21:02 +0800, Philipp Zabel wrote:
> On Fri, 2019-11-29 at 16:36 +0800, Yong Liang wrote:
> > On Mon, 2019-11-25 at 17:51 +0800, Philipp Zabel wrote:
> > > On Sun, 2019-11-24 at 22:16 -0800, Guenter Roeck wrote:
> > > > On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> > > > > From: "yong.liang" <yong.liang@mediatek.com>
> > > > >
> > > > > Add reset controller API in watchdog driver.
> > > > > Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> > > > > camera, codec and connectivity) software reset functionality.
> > > > >
> > > > > Signed-off-by: yong.liang <yong.liang@mediatek.com>
> > > > > Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> > > > > Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > > > > ---
> > > > > drivers/watchdog/Kconfig | 1 +
> > > > > drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> > > > > 2 files changed, 111 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > > > index 2e07caab9db2..629249fe5305 100644
> > > > > --- a/drivers/watchdog/Kconfig
> > > > > +++ b/drivers/watchdog/Kconfig
> > > > > @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> > > > > tristate "Mediatek SoCs watchdog support"
> > > > > depends on ARCH_MEDIATEK || COMPILE_TEST
> > > > > select WATCHDOG_CORE
> > > > > + select RESET_CONTROLLER
> > > > > help
> > > > > Say Y here to include support for the watchdog timer
> > > > > in Mediatek SoCs.
> > > > > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > > > > index 9c3d0033260d..d29484c7940a 100644
> > > > > --- a/drivers/watchdog/mtk_wdt.c
> > > > > +++ b/drivers/watchdog/mtk_wdt.c
> > > > > @@ -9,6 +9,9 @@
> > > > > * Based on sunxi_wdt.c
> > > > > */
> > > > >
> > > > > +#include <dt-bindings/reset-controller/mt2712-resets.h>
> > > > > +#include <dt-bindings/reset-controller/mt8183-resets.h>
> > > > > +#include <linux/delay.h>
> > > > > #include <linux/err.h>
> > > > > #include <linux/init.h>
> > > > > #include <linux/io.h>
> > > > > @@ -16,10 +19,12 @@
> > > > > #include <linux/module.h>
> > > > > #include <linux/moduleparam.h>
> > > > > #include <linux/of.h>
> > > > > +#include <linux/of_device.h>
> > > > > #include <linux/platform_device.h>
> > > > > +#include <linux/reset-controller.h>
> > > > > +#include <linux/slab.h>
> > > > > #include <linux/types.h>
> > > > > #include <linux/watchdog.h>
> > > > > -#include <linux/delay.h>
> > > > >
> > > > > #define WDT_MAX_TIMEOUT 31
> > > > > #define WDT_MIN_TIMEOUT 1
> > > > > @@ -44,6 +49,9 @@
> > > > > #define WDT_SWRST 0x14
> > > > > #define WDT_SWRST_KEY 0x1209
> > > > >
> > > > > +#define WDT_SWSYSRST 0x18U
> > > > > +#define WDT_SWSYS_RST_KEY 0x88000000
> > > > > +
> > > > > #define DRV_NAME "mtk-wdt"
> > > > > #define DRV_VERSION "1.0"
> > > > >
> > > > > @@ -53,8 +61,99 @@ static unsigned int timeout;
> > > > > struct mtk_wdt_dev {
> > > > > struct watchdog_device wdt_dev;
> > > > > void __iomem *wdt_base;
> > > > > + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> > > > > + struct reset_controller_dev rcdev;
> > > > > +};
> > > > > +
> > > > > +struct mtk_wdt_data {
> > > > > + int sw_rst_num;
> > > > > };
> > > > >
> > > > > +static const struct mtk_wdt_data mt2712_data = {
> > > > > + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> > > > > +};
> > > > > +
> > > > > +static const struct mtk_wdt_data mt8183_data = {
> > > > > + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> > > > > +};
> > > >
> > > > The number of resets can be set in .data directly; there is no need
> > > > for the structures.
> >
> > We want to put all properities in mtxxxx-resets.h and it easy to
> > manager. If there are new properity in the feture, we can put it in
> > mtk_wdt_data mtxxxx_data
>
> Do you expect there will be more properties in the future?
Yes, We may put some infra reset bit and max number in mtxxxx-resets.h
>
> > > > > +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> > > > > + unsigned long id)
> > > > > +{
> > > > > + unsigned int tmp;
> > > > > + unsigned long flags;
> > > > > + struct mtk_wdt_dev *data =
> > > > > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > > > > +
> > > > > + spin_lock_irqsave(&data->lock, flags);
> > > > > +
> > > > > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > > > > + tmp &= ~BIT(id);
> > > > > + tmp |= WDT_SWSYS_RST_KEY;
> > > > > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > > > > +
> > > > > + spin_unlock_irqrestore(&data->lock, flags);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > >
> > > > There is a lot of duplication in those functions. Only one line
> > > > is different. I think this is a good example where a helper function
> > > > with an additional argument indicating set or reset would be helpful.
> > > >
> > .assert and .dessert are two numbers of struct reset_control_ops.
> > I think it's better to define both of them.
>
> The suggestion was to have two very short _assert and _deassert
> functions that only contain a single call to a common helper function.
> See the reset-a10sr.c driver for an example.
OK. I will modify it as reset-a10sr.c do.
>
> regards
> Philipp
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
@ 2019-12-03 3:02 ` Yong Liang
0 siblings, 0 replies; 21+ messages in thread
From: Yong Liang @ 2019-12-03 3:02 UTC (permalink / raw)
To: Philipp Zabel
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
robh+dt@kernel.org, lgirdwood@gmail.com,
Jiaxin Yu (俞家鑫), perex@perex.cz,
tzungbi@google.com, broonie@kernel.org,
linux-mediatek@lists.infradead.org, Guenter Roeck,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲), wim@linux-watchdog.org,
linux-arm-kernel@lists.infradead.org
On Mon, 2019-12-02 at 21:02 +0800, Philipp Zabel wrote:
> On Fri, 2019-11-29 at 16:36 +0800, Yong Liang wrote:
> > On Mon, 2019-11-25 at 17:51 +0800, Philipp Zabel wrote:
> > > On Sun, 2019-11-24 at 22:16 -0800, Guenter Roeck wrote:
> > > > On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> > > > > From: "yong.liang" <yong.liang@mediatek.com>
> > > > >
> > > > > Add reset controller API in watchdog driver.
> > > > > Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> > > > > camera, codec and connectivity) software reset functionality.
> > > > >
> > > > > Signed-off-by: yong.liang <yong.liang@mediatek.com>
> > > > > Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> > > > > Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > > > > ---
> > > > > drivers/watchdog/Kconfig | 1 +
> > > > > drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> > > > > 2 files changed, 111 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > > > index 2e07caab9db2..629249fe5305 100644
> > > > > --- a/drivers/watchdog/Kconfig
> > > > > +++ b/drivers/watchdog/Kconfig
> > > > > @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> > > > > tristate "Mediatek SoCs watchdog support"
> > > > > depends on ARCH_MEDIATEK || COMPILE_TEST
> > > > > select WATCHDOG_CORE
> > > > > + select RESET_CONTROLLER
> > > > > help
> > > > > Say Y here to include support for the watchdog timer
> > > > > in Mediatek SoCs.
> > > > > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > > > > index 9c3d0033260d..d29484c7940a 100644
> > > > > --- a/drivers/watchdog/mtk_wdt.c
> > > > > +++ b/drivers/watchdog/mtk_wdt.c
> > > > > @@ -9,6 +9,9 @@
> > > > > * Based on sunxi_wdt.c
> > > > > */
> > > > >
> > > > > +#include <dt-bindings/reset-controller/mt2712-resets.h>
> > > > > +#include <dt-bindings/reset-controller/mt8183-resets.h>
> > > > > +#include <linux/delay.h>
> > > > > #include <linux/err.h>
> > > > > #include <linux/init.h>
> > > > > #include <linux/io.h>
> > > > > @@ -16,10 +19,12 @@
> > > > > #include <linux/module.h>
> > > > > #include <linux/moduleparam.h>
> > > > > #include <linux/of.h>
> > > > > +#include <linux/of_device.h>
> > > > > #include <linux/platform_device.h>
> > > > > +#include <linux/reset-controller.h>
> > > > > +#include <linux/slab.h>
> > > > > #include <linux/types.h>
> > > > > #include <linux/watchdog.h>
> > > > > -#include <linux/delay.h>
> > > > >
> > > > > #define WDT_MAX_TIMEOUT 31
> > > > > #define WDT_MIN_TIMEOUT 1
> > > > > @@ -44,6 +49,9 @@
> > > > > #define WDT_SWRST 0x14
> > > > > #define WDT_SWRST_KEY 0x1209
> > > > >
> > > > > +#define WDT_SWSYSRST 0x18U
> > > > > +#define WDT_SWSYS_RST_KEY 0x88000000
> > > > > +
> > > > > #define DRV_NAME "mtk-wdt"
> > > > > #define DRV_VERSION "1.0"
> > > > >
> > > > > @@ -53,8 +61,99 @@ static unsigned int timeout;
> > > > > struct mtk_wdt_dev {
> > > > > struct watchdog_device wdt_dev;
> > > > > void __iomem *wdt_base;
> > > > > + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> > > > > + struct reset_controller_dev rcdev;
> > > > > +};
> > > > > +
> > > > > +struct mtk_wdt_data {
> > > > > + int sw_rst_num;
> > > > > };
> > > > >
> > > > > +static const struct mtk_wdt_data mt2712_data = {
> > > > > + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> > > > > +};
> > > > > +
> > > > > +static const struct mtk_wdt_data mt8183_data = {
> > > > > + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> > > > > +};
> > > >
> > > > The number of resets can be set in .data directly; there is no need
> > > > for the structures.
> >
> > We want to put all properities in mtxxxx-resets.h and it easy to
> > manager. If there are new properity in the feture, we can put it in
> > mtk_wdt_data mtxxxx_data
>
> Do you expect there will be more properties in the future?
Yes, We may put some infra reset bit and max number in mtxxxx-resets.h
>
> > > > > +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> > > > > + unsigned long id)
> > > > > +{
> > > > > + unsigned int tmp;
> > > > > + unsigned long flags;
> > > > > + struct mtk_wdt_dev *data =
> > > > > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > > > > +
> > > > > + spin_lock_irqsave(&data->lock, flags);
> > > > > +
> > > > > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > > > > + tmp &= ~BIT(id);
> > > > > + tmp |= WDT_SWSYS_RST_KEY;
> > > > > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > > > > +
> > > > > + spin_unlock_irqrestore(&data->lock, flags);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > >
> > > > There is a lot of duplication in those functions. Only one line
> > > > is different. I think this is a good example where a helper function
> > > > with an additional argument indicating set or reset would be helpful.
> > > >
> > .assert and .dessert are two numbers of struct reset_control_ops.
> > I think it's better to define both of them.
>
> The suggestion was to have two very short _assert and _deassert
> functions that only contain a single call to a common helper function.
> See the reset-a10sr.c driver for an example.
OK. I will modify it as reset-a10sr.c do.
>
> regards
> Philipp
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [alsa-devel] [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
2019-12-03 3:02 ` Yong Liang
(?)
@ 2019-12-03 5:05 ` Guenter Roeck
-1 siblings, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2019-12-03 5:05 UTC (permalink / raw)
To: Yong Liang, Philipp Zabel
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
robh+dt@kernel.org, lgirdwood@gmail.com,
Jiaxin Yu (俞家鑫), tzungbi@google.com,
broonie@kernel.org, linux-mediatek@lists.infradead.org,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲), wim@linux-watchdog.org,
linux-arm-kernel@lists.infradead.org
On 12/2/19 7:02 PM, Yong Liang wrote:
> On Mon, 2019-12-02 at 21:02 +0800, Philipp Zabel wrote:
>> On Fri, 2019-11-29 at 16:36 +0800, Yong Liang wrote:
>>> On Mon, 2019-11-25 at 17:51 +0800, Philipp Zabel wrote:
>>>> On Sun, 2019-11-24 at 22:16 -0800, Guenter Roeck wrote:
>>>>> On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
>>>>>> From: "yong.liang" <yong.liang@mediatek.com>
>>>>>>
>>>>>> Add reset controller API in watchdog driver.
>>>>>> Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
>>>>>> camera, codec and connectivity) software reset functionality.
>>>>>>
>>>>>> Signed-off-by: yong.liang <yong.liang@mediatek.com>
>>>>>> Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
>>>>>> Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
>>>>>> ---
>>>>>> drivers/watchdog/Kconfig | 1 +
>>>>>> drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
>>>>>> 2 files changed, 111 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>>> index 2e07caab9db2..629249fe5305 100644
>>>>>> --- a/drivers/watchdog/Kconfig
>>>>>> +++ b/drivers/watchdog/Kconfig
>>>>>> @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
>>>>>> tristate "Mediatek SoCs watchdog support"
>>>>>> depends on ARCH_MEDIATEK || COMPILE_TEST
>>>>>> select WATCHDOG_CORE
>>>>>> + select RESET_CONTROLLER
>>>>>> help
>>>>>> Say Y here to include support for the watchdog timer
>>>>>> in Mediatek SoCs.
>>>>>> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
>>>>>> index 9c3d0033260d..d29484c7940a 100644
>>>>>> --- a/drivers/watchdog/mtk_wdt.c
>>>>>> +++ b/drivers/watchdog/mtk_wdt.c
>>>>>> @@ -9,6 +9,9 @@
>>>>>> * Based on sunxi_wdt.c
>>>>>> */
>>>>>>
>>>>>> +#include <dt-bindings/reset-controller/mt2712-resets.h>
>>>>>> +#include <dt-bindings/reset-controller/mt8183-resets.h>
>>>>>> +#include <linux/delay.h>
>>>>>> #include <linux/err.h>
>>>>>> #include <linux/init.h>
>>>>>> #include <linux/io.h>
>>>>>> @@ -16,10 +19,12 @@
>>>>>> #include <linux/module.h>
>>>>>> #include <linux/moduleparam.h>
>>>>>> #include <linux/of.h>
>>>>>> +#include <linux/of_device.h>
>>>>>> #include <linux/platform_device.h>
>>>>>> +#include <linux/reset-controller.h>
>>>>>> +#include <linux/slab.h>
>>>>>> #include <linux/types.h>
>>>>>> #include <linux/watchdog.h>
>>>>>> -#include <linux/delay.h>
>>>>>>
>>>>>> #define WDT_MAX_TIMEOUT 31
>>>>>> #define WDT_MIN_TIMEOUT 1
>>>>>> @@ -44,6 +49,9 @@
>>>>>> #define WDT_SWRST 0x14
>>>>>> #define WDT_SWRST_KEY 0x1209
>>>>>>
>>>>>> +#define WDT_SWSYSRST 0x18U
>>>>>> +#define WDT_SWSYS_RST_KEY 0x88000000
>>>>>> +
>>>>>> #define DRV_NAME "mtk-wdt"
>>>>>> #define DRV_VERSION "1.0"
>>>>>>
>>>>>> @@ -53,8 +61,99 @@ static unsigned int timeout;
>>>>>> struct mtk_wdt_dev {
>>>>>> struct watchdog_device wdt_dev;
>>>>>> void __iomem *wdt_base;
>>>>>> + spinlock_t lock; /* protects WDT_SWSYSRST reg */
>>>>>> + struct reset_controller_dev rcdev;
>>>>>> +};
>>>>>> +
>>>>>> +struct mtk_wdt_data {
>>>>>> + int sw_rst_num;
>>>>>> };
>>>>>>
>>>>>> +static const struct mtk_wdt_data mt2712_data = {
>>>>>> + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct mtk_wdt_data mt8183_data = {
>>>>>> + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
>>>>>> +};
>>>>>
>>>>> The number of resets can be set in .data directly; there is no need
>>>>> for the structures.
>>>
>>> We want to put all properities in mtxxxx-resets.h and it easy to
>>> manager. If there are new properity in the feture, we can put it in
>>> mtk_wdt_data mtxxxx_data
>>
>> Do you expect there will be more properties in the future?
>
> Yes, We may put some infra reset bit and max number in mtxxxx-resets.h
Please either do that now or introduce the complexity when needed.
Thanks,
Guenter
>>
>>>>>> +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
>>>>>> + unsigned long id)
>>>>>> +{
>>>>>> + unsigned int tmp;
>>>>>> + unsigned long flags;
>>>>>> + struct mtk_wdt_dev *data =
>>>>>> + container_of(rcdev, struct mtk_wdt_dev, rcdev);
>>>>>> +
>>>>>> + spin_lock_irqsave(&data->lock, flags);
>>>>>> +
>>>>>> + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
>>>>>> + tmp &= ~BIT(id);
>>>>>> + tmp |= WDT_SWSYS_RST_KEY;
>>>>>> + writel(tmp, data->wdt_base + WDT_SWSYSRST);
>>>>>> +
>>>>>> + spin_unlock_irqrestore(&data->lock, flags);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>
>>>>> There is a lot of duplication in those functions. Only one line
>>>>> is different. I think this is a good example where a helper function
>>>>> with an additional argument indicating set or reset would be helpful.
>>>>>
>>> .assert and .dessert are two numbers of struct reset_control_ops.
>>> I think it's better to define both of them.
>>
>> The suggestion was to have two very short _assert and _deassert
>> functions that only contain a single call to a common helper function.
>> See the reset-a10sr.c driver for an example.
>
> OK. I will modify it as reset-a10sr.c do.
>>
>> regards
>> Philipp
>>
>>
>> _______________________________________________
>> Linux-mediatek mailing list
>> Linux-mediatek@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-mediatek
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
@ 2019-12-03 5:05 ` Guenter Roeck
0 siblings, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2019-12-03 5:05 UTC (permalink / raw)
To: Yong Liang, Philipp Zabel
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
robh+dt@kernel.org, lgirdwood@gmail.com,
Jiaxin Yu (俞家鑫), perex@perex.cz,
tzungbi@google.com, broonie@kernel.org,
linux-mediatek@lists.infradead.org,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲), wim@linux-watchdog.org,
linux-arm-kernel@lists.infradead.org
On 12/2/19 7:02 PM, Yong Liang wrote:
> On Mon, 2019-12-02 at 21:02 +0800, Philipp Zabel wrote:
>> On Fri, 2019-11-29 at 16:36 +0800, Yong Liang wrote:
>>> On Mon, 2019-11-25 at 17:51 +0800, Philipp Zabel wrote:
>>>> On Sun, 2019-11-24 at 22:16 -0800, Guenter Roeck wrote:
>>>>> On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
>>>>>> From: "yong.liang" <yong.liang@mediatek.com>
>>>>>>
>>>>>> Add reset controller API in watchdog driver.
>>>>>> Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
>>>>>> camera, codec and connectivity) software reset functionality.
>>>>>>
>>>>>> Signed-off-by: yong.liang <yong.liang@mediatek.com>
>>>>>> Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
>>>>>> Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
>>>>>> ---
>>>>>> drivers/watchdog/Kconfig | 1 +
>>>>>> drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
>>>>>> 2 files changed, 111 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>>> index 2e07caab9db2..629249fe5305 100644
>>>>>> --- a/drivers/watchdog/Kconfig
>>>>>> +++ b/drivers/watchdog/Kconfig
>>>>>> @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
>>>>>> tristate "Mediatek SoCs watchdog support"
>>>>>> depends on ARCH_MEDIATEK || COMPILE_TEST
>>>>>> select WATCHDOG_CORE
>>>>>> + select RESET_CONTROLLER
>>>>>> help
>>>>>> Say Y here to include support for the watchdog timer
>>>>>> in Mediatek SoCs.
>>>>>> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
>>>>>> index 9c3d0033260d..d29484c7940a 100644
>>>>>> --- a/drivers/watchdog/mtk_wdt.c
>>>>>> +++ b/drivers/watchdog/mtk_wdt.c
>>>>>> @@ -9,6 +9,9 @@
>>>>>> * Based on sunxi_wdt.c
>>>>>> */
>>>>>>
>>>>>> +#include <dt-bindings/reset-controller/mt2712-resets.h>
>>>>>> +#include <dt-bindings/reset-controller/mt8183-resets.h>
>>>>>> +#include <linux/delay.h>
>>>>>> #include <linux/err.h>
>>>>>> #include <linux/init.h>
>>>>>> #include <linux/io.h>
>>>>>> @@ -16,10 +19,12 @@
>>>>>> #include <linux/module.h>
>>>>>> #include <linux/moduleparam.h>
>>>>>> #include <linux/of.h>
>>>>>> +#include <linux/of_device.h>
>>>>>> #include <linux/platform_device.h>
>>>>>> +#include <linux/reset-controller.h>
>>>>>> +#include <linux/slab.h>
>>>>>> #include <linux/types.h>
>>>>>> #include <linux/watchdog.h>
>>>>>> -#include <linux/delay.h>
>>>>>>
>>>>>> #define WDT_MAX_TIMEOUT 31
>>>>>> #define WDT_MIN_TIMEOUT 1
>>>>>> @@ -44,6 +49,9 @@
>>>>>> #define WDT_SWRST 0x14
>>>>>> #define WDT_SWRST_KEY 0x1209
>>>>>>
>>>>>> +#define WDT_SWSYSRST 0x18U
>>>>>> +#define WDT_SWSYS_RST_KEY 0x88000000
>>>>>> +
>>>>>> #define DRV_NAME "mtk-wdt"
>>>>>> #define DRV_VERSION "1.0"
>>>>>>
>>>>>> @@ -53,8 +61,99 @@ static unsigned int timeout;
>>>>>> struct mtk_wdt_dev {
>>>>>> struct watchdog_device wdt_dev;
>>>>>> void __iomem *wdt_base;
>>>>>> + spinlock_t lock; /* protects WDT_SWSYSRST reg */
>>>>>> + struct reset_controller_dev rcdev;
>>>>>> +};
>>>>>> +
>>>>>> +struct mtk_wdt_data {
>>>>>> + int sw_rst_num;
>>>>>> };
>>>>>>
>>>>>> +static const struct mtk_wdt_data mt2712_data = {
>>>>>> + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct mtk_wdt_data mt8183_data = {
>>>>>> + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
>>>>>> +};
>>>>>
>>>>> The number of resets can be set in .data directly; there is no need
>>>>> for the structures.
>>>
>>> We want to put all properities in mtxxxx-resets.h and it easy to
>>> manager. If there are new properity in the feture, we can put it in
>>> mtk_wdt_data mtxxxx_data
>>
>> Do you expect there will be more properties in the future?
>
> Yes, We may put some infra reset bit and max number in mtxxxx-resets.h
Please either do that now or introduce the complexity when needed.
Thanks,
Guenter
>>
>>>>>> +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
>>>>>> + unsigned long id)
>>>>>> +{
>>>>>> + unsigned int tmp;
>>>>>> + unsigned long flags;
>>>>>> + struct mtk_wdt_dev *data =
>>>>>> + container_of(rcdev, struct mtk_wdt_dev, rcdev);
>>>>>> +
>>>>>> + spin_lock_irqsave(&data->lock, flags);
>>>>>> +
>>>>>> + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
>>>>>> + tmp &= ~BIT(id);
>>>>>> + tmp |= WDT_SWSYS_RST_KEY;
>>>>>> + writel(tmp, data->wdt_base + WDT_SWSYSRST);
>>>>>> +
>>>>>> + spin_unlock_irqrestore(&data->lock, flags);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>
>>>>> There is a lot of duplication in those functions. Only one line
>>>>> is different. I think this is a good example where a helper function
>>>>> with an additional argument indicating set or reset would be helpful.
>>>>>
>>> .assert and .dessert are two numbers of struct reset_control_ops.
>>> I think it's better to define both of them.
>>
>> The suggestion was to have two very short _assert and _deassert
>> functions that only contain a single call to a common helper function.
>> See the reset-a10sr.c driver for an example.
>
> OK. I will modify it as reset-a10sr.c do.
>>
>> regards
>> Philipp
>>
>>
>> _______________________________________________
>> Linux-mediatek mailing list
>> Linux-mediatek@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-mediatek
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
@ 2019-12-03 5:05 ` Guenter Roeck
0 siblings, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2019-12-03 5:05 UTC (permalink / raw)
To: Yong Liang, Philipp Zabel
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
robh+dt@kernel.org, lgirdwood@gmail.com,
Jiaxin Yu (俞家鑫), perex@perex.cz,
tzungbi@google.com, broonie@kernel.org,
linux-mediatek@lists.infradead.org,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲), wim@linux-watchdog.org,
linux-arm-kernel@lists.infradead.org
On 12/2/19 7:02 PM, Yong Liang wrote:
> On Mon, 2019-12-02 at 21:02 +0800, Philipp Zabel wrote:
>> On Fri, 2019-11-29 at 16:36 +0800, Yong Liang wrote:
>>> On Mon, 2019-11-25 at 17:51 +0800, Philipp Zabel wrote:
>>>> On Sun, 2019-11-24 at 22:16 -0800, Guenter Roeck wrote:
>>>>> On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
>>>>>> From: "yong.liang" <yong.liang@mediatek.com>
>>>>>>
>>>>>> Add reset controller API in watchdog driver.
>>>>>> Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
>>>>>> camera, codec and connectivity) software reset functionality.
>>>>>>
>>>>>> Signed-off-by: yong.liang <yong.liang@mediatek.com>
>>>>>> Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
>>>>>> Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
>>>>>> ---
>>>>>> drivers/watchdog/Kconfig | 1 +
>>>>>> drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
>>>>>> 2 files changed, 111 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>>> index 2e07caab9db2..629249fe5305 100644
>>>>>> --- a/drivers/watchdog/Kconfig
>>>>>> +++ b/drivers/watchdog/Kconfig
>>>>>> @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
>>>>>> tristate "Mediatek SoCs watchdog support"
>>>>>> depends on ARCH_MEDIATEK || COMPILE_TEST
>>>>>> select WATCHDOG_CORE
>>>>>> + select RESET_CONTROLLER
>>>>>> help
>>>>>> Say Y here to include support for the watchdog timer
>>>>>> in Mediatek SoCs.
>>>>>> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
>>>>>> index 9c3d0033260d..d29484c7940a 100644
>>>>>> --- a/drivers/watchdog/mtk_wdt.c
>>>>>> +++ b/drivers/watchdog/mtk_wdt.c
>>>>>> @@ -9,6 +9,9 @@
>>>>>> * Based on sunxi_wdt.c
>>>>>> */
>>>>>>
>>>>>> +#include <dt-bindings/reset-controller/mt2712-resets.h>
>>>>>> +#include <dt-bindings/reset-controller/mt8183-resets.h>
>>>>>> +#include <linux/delay.h>
>>>>>> #include <linux/err.h>
>>>>>> #include <linux/init.h>
>>>>>> #include <linux/io.h>
>>>>>> @@ -16,10 +19,12 @@
>>>>>> #include <linux/module.h>
>>>>>> #include <linux/moduleparam.h>
>>>>>> #include <linux/of.h>
>>>>>> +#include <linux/of_device.h>
>>>>>> #include <linux/platform_device.h>
>>>>>> +#include <linux/reset-controller.h>
>>>>>> +#include <linux/slab.h>
>>>>>> #include <linux/types.h>
>>>>>> #include <linux/watchdog.h>
>>>>>> -#include <linux/delay.h>
>>>>>>
>>>>>> #define WDT_MAX_TIMEOUT 31
>>>>>> #define WDT_MIN_TIMEOUT 1
>>>>>> @@ -44,6 +49,9 @@
>>>>>> #define WDT_SWRST 0x14
>>>>>> #define WDT_SWRST_KEY 0x1209
>>>>>>
>>>>>> +#define WDT_SWSYSRST 0x18U
>>>>>> +#define WDT_SWSYS_RST_KEY 0x88000000
>>>>>> +
>>>>>> #define DRV_NAME "mtk-wdt"
>>>>>> #define DRV_VERSION "1.0"
>>>>>>
>>>>>> @@ -53,8 +61,99 @@ static unsigned int timeout;
>>>>>> struct mtk_wdt_dev {
>>>>>> struct watchdog_device wdt_dev;
>>>>>> void __iomem *wdt_base;
>>>>>> + spinlock_t lock; /* protects WDT_SWSYSRST reg */
>>>>>> + struct reset_controller_dev rcdev;
>>>>>> +};
>>>>>> +
>>>>>> +struct mtk_wdt_data {
>>>>>> + int sw_rst_num;
>>>>>> };
>>>>>>
>>>>>> +static const struct mtk_wdt_data mt2712_data = {
>>>>>> + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct mtk_wdt_data mt8183_data = {
>>>>>> + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
>>>>>> +};
>>>>>
>>>>> The number of resets can be set in .data directly; there is no need
>>>>> for the structures.
>>>
>>> We want to put all properities in mtxxxx-resets.h and it easy to
>>> manager. If there are new properity in the feture, we can put it in
>>> mtk_wdt_data mtxxxx_data
>>
>> Do you expect there will be more properties in the future?
>
> Yes, We may put some infra reset bit and max number in mtxxxx-resets.h
Please either do that now or introduce the complexity when needed.
Thanks,
Guenter
>>
>>>>>> +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
>>>>>> + unsigned long id)
>>>>>> +{
>>>>>> + unsigned int tmp;
>>>>>> + unsigned long flags;
>>>>>> + struct mtk_wdt_dev *data =
>>>>>> + container_of(rcdev, struct mtk_wdt_dev, rcdev);
>>>>>> +
>>>>>> + spin_lock_irqsave(&data->lock, flags);
>>>>>> +
>>>>>> + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
>>>>>> + tmp &= ~BIT(id);
>>>>>> + tmp |= WDT_SWSYS_RST_KEY;
>>>>>> + writel(tmp, data->wdt_base + WDT_SWSYSRST);
>>>>>> +
>>>>>> + spin_unlock_irqrestore(&data->lock, flags);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>
>>>>> There is a lot of duplication in those functions. Only one line
>>>>> is different. I think this is a good example where a helper function
>>>>> with an additional argument indicating set or reset would be helpful.
>>>>>
>>> .assert and .dessert are two numbers of struct reset_control_ops.
>>> I think it's better to define both of them.
>>
>> The suggestion was to have two very short _assert and _deassert
>> functions that only contain a single call to a common helper function.
>> See the reset-a10sr.c driver for an example.
>
> OK. I will modify it as reset-a10sr.c do.
>>
>> regards
>> Philipp
>>
>>
>> _______________________________________________
>> Linux-mediatek mailing list
>> Linux-mediatek@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-mediatek
>
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [alsa-devel] [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
2019-11-25 6:16 ` Guenter Roeck
(?)
@ 2019-11-29 10:39 ` Yong Liang
-1 siblings, 0 replies; 21+ messages in thread
From: Yong Liang @ 2019-11-29 10:39 UTC (permalink / raw)
To: Guenter Roeck
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
lgirdwood@gmail.com, robh+dt@kernel.org, tzungbi@google.com,
broonie@kernel.org, linux-mediatek@lists.infradead.org,
Jiaxin Yu (俞家鑫), p.zabel@pengutronix.de,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲), wim@linux-watchdog.org,
linux-arm-kernel@lists.infradead.org
On Mon, 2019-11-25 at 14:16 +0800, Guenter Roeck wrote:
> On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> > From: "yong.liang" <yong.liang@mediatek.com>
> >
> > Add reset controller API in watchdog driver.
> > Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> > camera, codec and connectivity) software reset functionality.
> >
> > Signed-off-by: yong.liang <yong.liang@mediatek.com>
> > Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> > Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > ---
> > drivers/watchdog/Kconfig | 1 +
> > drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 111 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 2e07caab9db2..629249fe5305 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> > tristate "Mediatek SoCs watchdog support"
> > depends on ARCH_MEDIATEK || COMPILE_TEST
> > select WATCHDOG_CORE
> > + select RESET_CONTROLLER
> > help
> > Say Y here to include support for the watchdog timer
> > in Mediatek SoCs.
> > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > index 9c3d0033260d..d29484c7940a 100644
> > --- a/drivers/watchdog/mtk_wdt.c
> > +++ b/drivers/watchdog/mtk_wdt.c
> > @@ -9,6 +9,9 @@
> > * Based on sunxi_wdt.c
> > */
> >
> > +#include <dt-bindings/reset-controller/mt2712-resets.h>
> > +#include <dt-bindings/reset-controller/mt8183-resets.h>
> > +#include <linux/delay.h>
> > #include <linux/err.h>
> > #include <linux/init.h>
> > #include <linux/io.h>
> > @@ -16,10 +19,12 @@
> > #include <linux/module.h>
> > #include <linux/moduleparam.h>
> > #include <linux/of.h>
> > +#include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > +#include <linux/reset-controller.h>
> > +#include <linux/slab.h>
> > #include <linux/types.h>
> > #include <linux/watchdog.h>
> > -#include <linux/delay.h>
> >
> > #define WDT_MAX_TIMEOUT 31
> > #define WDT_MIN_TIMEOUT 1
> > @@ -44,6 +49,9 @@
> > #define WDT_SWRST 0x14
> > #define WDT_SWRST_KEY 0x1209
> >
> > +#define WDT_SWSYSRST 0x18U
> > +#define WDT_SWSYS_RST_KEY 0x88000000
> > +
> > #define DRV_NAME "mtk-wdt"
> > #define DRV_VERSION "1.0"
> >
> > @@ -53,8 +61,99 @@ static unsigned int timeout;
> > struct mtk_wdt_dev {
> > struct watchdog_device wdt_dev;
> > void __iomem *wdt_base;
> > + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> > + struct reset_controller_dev rcdev;
> > +};
> > +
> > +struct mtk_wdt_data {
> > + int sw_rst_num;
> > };
> >
> > +static const struct mtk_wdt_data mt2712_data = {
> > + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> > +};
> > +
> > +static const struct mtk_wdt_data mt8183_data = {
> > + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> > +};
>
> The number of resets can be set in .data directly; there is no need
> for the structures.
>
> > +
> > +static int toprgu_reset_assert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + unsigned int tmp;
> > + unsigned long flags;
> > + struct mtk_wdt_dev *data =
> > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > +
> > + spin_lock_irqsave(&data->lock, flags);
> > +
> > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > + tmp |= BIT(id);
> > + tmp |= WDT_SWSYS_RST_KEY;
> > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > +
> > + spin_unlock_irqrestore(&data->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + unsigned int tmp;
> > + unsigned long flags;
> > + struct mtk_wdt_dev *data =
> > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > +
> > + spin_lock_irqsave(&data->lock, flags);
> > +
> > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > + tmp &= ~BIT(id);
> > + tmp |= WDT_SWSYS_RST_KEY;
> > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > +
> > + spin_unlock_irqrestore(&data->lock, flags);
> > +
> > + return 0;
> > +}
>
> There is a lot of duplication in those functions. Only one line
> is different. I think this is a good example where a helper function
> with an additional argument indicating set or reset would be helpful.
>
> > +
> > +static int toprgu_reset(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + int ret;
> > +
> > + ret = toprgu_reset_assert(rcdev, id);
> > + if (ret)
> > + return ret;
> > +
> > + return toprgu_reset_deassert(rcdev, id);
> > +}
> > +
> > +static const struct reset_control_ops toprgu_reset_ops = {
> > + .assert = toprgu_reset_assert,
> > + .deassert = toprgu_reset_deassert,
> > + .reset = toprgu_reset,
> > +};
> > +
> > +static int toprgu_register_reset_controller(struct platform_device *pdev,
> > + int rst_num)
> > +{
> > + int ret;
> > + struct mtk_wdt_dev *mtk_wdt = platform_get_drvdata(pdev);
> > +
> > + spin_lock_init(&mtk_wdt->lock);
> > +
> > + mtk_wdt->rcdev.owner = THIS_MODULE;
> > + mtk_wdt->rcdev.nr_resets = rst_num;
> > + mtk_wdt->rcdev.ops = &toprgu_reset_ops;
> > + mtk_wdt->rcdev.of_node = pdev->dev.of_node;
> > + ret = reset_controller_register(&mtk_wdt->rcdev);
> > + if (ret != 0)
> > + dev_err(&pdev->dev,
> > + "couldn't register wdt reset controller: %d\n", ret);
> > + return ret;
> > +}
> > +
> > static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
> > unsigned long action, void *data)
> > {
> > @@ -155,6 +254,7 @@ static int mtk_wdt_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > struct mtk_wdt_dev *mtk_wdt;
> > + struct mtk_wdt_data *wdt_data;
> > int err;
> >
> > mtk_wdt = devm_kzalloc(dev, sizeof(*mtk_wdt), GFP_KERNEL);
> > @@ -190,6 +290,13 @@ static int mtk_wdt_probe(struct platform_device *pdev)
> > dev_info(dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)\n",
> > mtk_wdt->wdt_dev.timeout, nowayout);
> >
> > + wdt_data = (struct mtk_wdt_data *)of_device_get_match_data(dev);
> > + if (wdt_data) {
> > + err = toprgu_register_reset_controller(pdev,
> > + wdt_data->sw_rst_num);
> > + if (err)
> > + return err;
>
> If this happens, the user will wonder why there was a message "Watchdog
> enabled", but there is no watchdog. It would be better to call this
> function before the dev_info() above.
>
> Guenter
It will return err just if .compatible has no .data value, just like
"mediatek,mt6589-wdt". Bus watchdog is still enable.
>
> > + }
> > return 0;
> > }
> >
> > @@ -218,7 +325,9 @@ static int mtk_wdt_resume(struct device *dev)
> > #endif
> >
> > static const struct of_device_id mtk_wdt_dt_ids[] = {
> > + { .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },
> > { .compatible = "mediatek,mt6589-wdt" },
> > + { .compatible = "mediatek,mt8183-wdt", .data = &mt8183_data },
> > { /* sentinel */ }
> > };
> > MODULE_DEVICE_TABLE(of, mtk_wdt_dt_ids);
> > --
> > 2.18.0
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
@ 2019-11-29 10:39 ` Yong Liang
0 siblings, 0 replies; 21+ messages in thread
From: Yong Liang @ 2019-11-29 10:39 UTC (permalink / raw)
To: Guenter Roeck
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
lgirdwood@gmail.com, robh+dt@kernel.org, perex@perex.cz,
tzungbi@google.com, broonie@kernel.org,
linux-mediatek@lists.infradead.org,
Jiaxin Yu (俞家鑫), p.zabel@pengutronix.de,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲), wim@linux-watchdog.org,
linux-arm-kernel@lists.infradead.org
On Mon, 2019-11-25 at 14:16 +0800, Guenter Roeck wrote:
> On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> > From: "yong.liang" <yong.liang@mediatek.com>
> >
> > Add reset controller API in watchdog driver.
> > Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> > camera, codec and connectivity) software reset functionality.
> >
> > Signed-off-by: yong.liang <yong.liang@mediatek.com>
> > Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> > Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > ---
> > drivers/watchdog/Kconfig | 1 +
> > drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 111 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 2e07caab9db2..629249fe5305 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> > tristate "Mediatek SoCs watchdog support"
> > depends on ARCH_MEDIATEK || COMPILE_TEST
> > select WATCHDOG_CORE
> > + select RESET_CONTROLLER
> > help
> > Say Y here to include support for the watchdog timer
> > in Mediatek SoCs.
> > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > index 9c3d0033260d..d29484c7940a 100644
> > --- a/drivers/watchdog/mtk_wdt.c
> > +++ b/drivers/watchdog/mtk_wdt.c
> > @@ -9,6 +9,9 @@
> > * Based on sunxi_wdt.c
> > */
> >
> > +#include <dt-bindings/reset-controller/mt2712-resets.h>
> > +#include <dt-bindings/reset-controller/mt8183-resets.h>
> > +#include <linux/delay.h>
> > #include <linux/err.h>
> > #include <linux/init.h>
> > #include <linux/io.h>
> > @@ -16,10 +19,12 @@
> > #include <linux/module.h>
> > #include <linux/moduleparam.h>
> > #include <linux/of.h>
> > +#include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > +#include <linux/reset-controller.h>
> > +#include <linux/slab.h>
> > #include <linux/types.h>
> > #include <linux/watchdog.h>
> > -#include <linux/delay.h>
> >
> > #define WDT_MAX_TIMEOUT 31
> > #define WDT_MIN_TIMEOUT 1
> > @@ -44,6 +49,9 @@
> > #define WDT_SWRST 0x14
> > #define WDT_SWRST_KEY 0x1209
> >
> > +#define WDT_SWSYSRST 0x18U
> > +#define WDT_SWSYS_RST_KEY 0x88000000
> > +
> > #define DRV_NAME "mtk-wdt"
> > #define DRV_VERSION "1.0"
> >
> > @@ -53,8 +61,99 @@ static unsigned int timeout;
> > struct mtk_wdt_dev {
> > struct watchdog_device wdt_dev;
> > void __iomem *wdt_base;
> > + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> > + struct reset_controller_dev rcdev;
> > +};
> > +
> > +struct mtk_wdt_data {
> > + int sw_rst_num;
> > };
> >
> > +static const struct mtk_wdt_data mt2712_data = {
> > + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> > +};
> > +
> > +static const struct mtk_wdt_data mt8183_data = {
> > + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> > +};
>
> The number of resets can be set in .data directly; there is no need
> for the structures.
>
> > +
> > +static int toprgu_reset_assert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + unsigned int tmp;
> > + unsigned long flags;
> > + struct mtk_wdt_dev *data =
> > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > +
> > + spin_lock_irqsave(&data->lock, flags);
> > +
> > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > + tmp |= BIT(id);
> > + tmp |= WDT_SWSYS_RST_KEY;
> > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > +
> > + spin_unlock_irqrestore(&data->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + unsigned int tmp;
> > + unsigned long flags;
> > + struct mtk_wdt_dev *data =
> > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > +
> > + spin_lock_irqsave(&data->lock, flags);
> > +
> > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > + tmp &= ~BIT(id);
> > + tmp |= WDT_SWSYS_RST_KEY;
> > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > +
> > + spin_unlock_irqrestore(&data->lock, flags);
> > +
> > + return 0;
> > +}
>
> There is a lot of duplication in those functions. Only one line
> is different. I think this is a good example where a helper function
> with an additional argument indicating set or reset would be helpful.
>
> > +
> > +static int toprgu_reset(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + int ret;
> > +
> > + ret = toprgu_reset_assert(rcdev, id);
> > + if (ret)
> > + return ret;
> > +
> > + return toprgu_reset_deassert(rcdev, id);
> > +}
> > +
> > +static const struct reset_control_ops toprgu_reset_ops = {
> > + .assert = toprgu_reset_assert,
> > + .deassert = toprgu_reset_deassert,
> > + .reset = toprgu_reset,
> > +};
> > +
> > +static int toprgu_register_reset_controller(struct platform_device *pdev,
> > + int rst_num)
> > +{
> > + int ret;
> > + struct mtk_wdt_dev *mtk_wdt = platform_get_drvdata(pdev);
> > +
> > + spin_lock_init(&mtk_wdt->lock);
> > +
> > + mtk_wdt->rcdev.owner = THIS_MODULE;
> > + mtk_wdt->rcdev.nr_resets = rst_num;
> > + mtk_wdt->rcdev.ops = &toprgu_reset_ops;
> > + mtk_wdt->rcdev.of_node = pdev->dev.of_node;
> > + ret = reset_controller_register(&mtk_wdt->rcdev);
> > + if (ret != 0)
> > + dev_err(&pdev->dev,
> > + "couldn't register wdt reset controller: %d\n", ret);
> > + return ret;
> > +}
> > +
> > static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
> > unsigned long action, void *data)
> > {
> > @@ -155,6 +254,7 @@ static int mtk_wdt_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > struct mtk_wdt_dev *mtk_wdt;
> > + struct mtk_wdt_data *wdt_data;
> > int err;
> >
> > mtk_wdt = devm_kzalloc(dev, sizeof(*mtk_wdt), GFP_KERNEL);
> > @@ -190,6 +290,13 @@ static int mtk_wdt_probe(struct platform_device *pdev)
> > dev_info(dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)\n",
> > mtk_wdt->wdt_dev.timeout, nowayout);
> >
> > + wdt_data = (struct mtk_wdt_data *)of_device_get_match_data(dev);
> > + if (wdt_data) {
> > + err = toprgu_register_reset_controller(pdev,
> > + wdt_data->sw_rst_num);
> > + if (err)
> > + return err;
>
> If this happens, the user will wonder why there was a message "Watchdog
> enabled", but there is no watchdog. It would be better to call this
> function before the dev_info() above.
>
> Guenter
It will return err just if .compatible has no .data value, just like
"mediatek,mt6589-wdt". Bus watchdog is still enable.
>
> > + }
> > return 0;
> > }
> >
> > @@ -218,7 +325,9 @@ static int mtk_wdt_resume(struct device *dev)
> > #endif
> >
> > static const struct of_device_id mtk_wdt_dt_ids[] = {
> > + { .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },
> > { .compatible = "mediatek,mt6589-wdt" },
> > + { .compatible = "mediatek,mt8183-wdt", .data = &mt8183_data },
> > { /* sentinel */ }
> > };
> > MODULE_DEVICE_TABLE(of, mtk_wdt_dt_ids);
> > --
> > 2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
@ 2019-11-29 10:39 ` Yong Liang
0 siblings, 0 replies; 21+ messages in thread
From: Yong Liang @ 2019-11-29 10:39 UTC (permalink / raw)
To: Guenter Roeck
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
lgirdwood@gmail.com, robh+dt@kernel.org, perex@perex.cz,
tzungbi@google.com, broonie@kernel.org,
linux-mediatek@lists.infradead.org,
Jiaxin Yu (俞家鑫), p.zabel@pengutronix.de,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲), wim@linux-watchdog.org,
linux-arm-kernel@lists.infradead.org
On Mon, 2019-11-25 at 14:16 +0800, Guenter Roeck wrote:
> On Mon, Nov 25, 2019 at 11:03:50AM +0800, Jiaxin Yu wrote:
> > From: "yong.liang" <yong.liang@mediatek.com>
> >
> > Add reset controller API in watchdog driver.
> > Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
> > camera, codec and connectivity) software reset functionality.
> >
> > Signed-off-by: yong.liang <yong.liang@mediatek.com>
> > Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
> > Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > ---
> > drivers/watchdog/Kconfig | 1 +
> > drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 111 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 2e07caab9db2..629249fe5305 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
> > tristate "Mediatek SoCs watchdog support"
> > depends on ARCH_MEDIATEK || COMPILE_TEST
> > select WATCHDOG_CORE
> > + select RESET_CONTROLLER
> > help
> > Say Y here to include support for the watchdog timer
> > in Mediatek SoCs.
> > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > index 9c3d0033260d..d29484c7940a 100644
> > --- a/drivers/watchdog/mtk_wdt.c
> > +++ b/drivers/watchdog/mtk_wdt.c
> > @@ -9,6 +9,9 @@
> > * Based on sunxi_wdt.c
> > */
> >
> > +#include <dt-bindings/reset-controller/mt2712-resets.h>
> > +#include <dt-bindings/reset-controller/mt8183-resets.h>
> > +#include <linux/delay.h>
> > #include <linux/err.h>
> > #include <linux/init.h>
> > #include <linux/io.h>
> > @@ -16,10 +19,12 @@
> > #include <linux/module.h>
> > #include <linux/moduleparam.h>
> > #include <linux/of.h>
> > +#include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > +#include <linux/reset-controller.h>
> > +#include <linux/slab.h>
> > #include <linux/types.h>
> > #include <linux/watchdog.h>
> > -#include <linux/delay.h>
> >
> > #define WDT_MAX_TIMEOUT 31
> > #define WDT_MIN_TIMEOUT 1
> > @@ -44,6 +49,9 @@
> > #define WDT_SWRST 0x14
> > #define WDT_SWRST_KEY 0x1209
> >
> > +#define WDT_SWSYSRST 0x18U
> > +#define WDT_SWSYS_RST_KEY 0x88000000
> > +
> > #define DRV_NAME "mtk-wdt"
> > #define DRV_VERSION "1.0"
> >
> > @@ -53,8 +61,99 @@ static unsigned int timeout;
> > struct mtk_wdt_dev {
> > struct watchdog_device wdt_dev;
> > void __iomem *wdt_base;
> > + spinlock_t lock; /* protects WDT_SWSYSRST reg */
> > + struct reset_controller_dev rcdev;
> > +};
> > +
> > +struct mtk_wdt_data {
> > + int sw_rst_num;
> > };
> >
> > +static const struct mtk_wdt_data mt2712_data = {
> > + .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> > +};
> > +
> > +static const struct mtk_wdt_data mt8183_data = {
> > + .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> > +};
>
> The number of resets can be set in .data directly; there is no need
> for the structures.
>
> > +
> > +static int toprgu_reset_assert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + unsigned int tmp;
> > + unsigned long flags;
> > + struct mtk_wdt_dev *data =
> > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > +
> > + spin_lock_irqsave(&data->lock, flags);
> > +
> > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > + tmp |= BIT(id);
> > + tmp |= WDT_SWSYS_RST_KEY;
> > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > +
> > + spin_unlock_irqrestore(&data->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + unsigned int tmp;
> > + unsigned long flags;
> > + struct mtk_wdt_dev *data =
> > + container_of(rcdev, struct mtk_wdt_dev, rcdev);
> > +
> > + spin_lock_irqsave(&data->lock, flags);
> > +
> > + tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
> > + tmp &= ~BIT(id);
> > + tmp |= WDT_SWSYS_RST_KEY;
> > + writel(tmp, data->wdt_base + WDT_SWSYSRST);
> > +
> > + spin_unlock_irqrestore(&data->lock, flags);
> > +
> > + return 0;
> > +}
>
> There is a lot of duplication in those functions. Only one line
> is different. I think this is a good example where a helper function
> with an additional argument indicating set or reset would be helpful.
>
> > +
> > +static int toprgu_reset(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + int ret;
> > +
> > + ret = toprgu_reset_assert(rcdev, id);
> > + if (ret)
> > + return ret;
> > +
> > + return toprgu_reset_deassert(rcdev, id);
> > +}
> > +
> > +static const struct reset_control_ops toprgu_reset_ops = {
> > + .assert = toprgu_reset_assert,
> > + .deassert = toprgu_reset_deassert,
> > + .reset = toprgu_reset,
> > +};
> > +
> > +static int toprgu_register_reset_controller(struct platform_device *pdev,
> > + int rst_num)
> > +{
> > + int ret;
> > + struct mtk_wdt_dev *mtk_wdt = platform_get_drvdata(pdev);
> > +
> > + spin_lock_init(&mtk_wdt->lock);
> > +
> > + mtk_wdt->rcdev.owner = THIS_MODULE;
> > + mtk_wdt->rcdev.nr_resets = rst_num;
> > + mtk_wdt->rcdev.ops = &toprgu_reset_ops;
> > + mtk_wdt->rcdev.of_node = pdev->dev.of_node;
> > + ret = reset_controller_register(&mtk_wdt->rcdev);
> > + if (ret != 0)
> > + dev_err(&pdev->dev,
> > + "couldn't register wdt reset controller: %d\n", ret);
> > + return ret;
> > +}
> > +
> > static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
> > unsigned long action, void *data)
> > {
> > @@ -155,6 +254,7 @@ static int mtk_wdt_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > struct mtk_wdt_dev *mtk_wdt;
> > + struct mtk_wdt_data *wdt_data;
> > int err;
> >
> > mtk_wdt = devm_kzalloc(dev, sizeof(*mtk_wdt), GFP_KERNEL);
> > @@ -190,6 +290,13 @@ static int mtk_wdt_probe(struct platform_device *pdev)
> > dev_info(dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)\n",
> > mtk_wdt->wdt_dev.timeout, nowayout);
> >
> > + wdt_data = (struct mtk_wdt_data *)of_device_get_match_data(dev);
> > + if (wdt_data) {
> > + err = toprgu_register_reset_controller(pdev,
> > + wdt_data->sw_rst_num);
> > + if (err)
> > + return err;
>
> If this happens, the user will wonder why there was a message "Watchdog
> enabled", but there is no watchdog. It would be better to call this
> function before the dev_info() above.
>
> Guenter
It will return err just if .compatible has no .data value, just like
"mediatek,mt6589-wdt". Bus watchdog is still enable.
>
> > + }
> > return 0;
> > }
> >
> > @@ -218,7 +325,9 @@ static int mtk_wdt_resume(struct device *dev)
> > #endif
> >
> > static const struct of_device_id mtk_wdt_dt_ids[] = {
> > + { .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },
> > { .compatible = "mediatek,mt6589-wdt" },
> > + { .compatible = "mediatek,mt8183-wdt", .data = &mt8183_data },
> > { /* sentinel */ }
> > };
> > MODULE_DEVICE_TABLE(of, mtk_wdt_dt_ids);
> > --
> > 2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 21+ messages in thread
* [alsa-devel] [PATCH v5 0/2] ASoC: mt8183: fix audio playback slowly after playback
@ 2019-11-25 3:03 Jiaxin Yu
2019-11-25 3:03 ` [alsa-devel] [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller Jiaxin Yu
0 siblings, 1 reply; 21+ messages in thread
From: Jiaxin Yu @ 2019-11-25 3:03 UTC (permalink / raw)
To: broonie, mark.rutland, yingjoe.chen, p.zabel, robh+dt, linux, wim
Cc: alsa-devel, yong.liang, lgirdwood, jiaxin.yu, tzungbi,
linux-mediatek, eason.yen, linux-arm-kernel
This series patches add reset controller for MT8183, and audio will use it in
machine driver during bootup, they depend on the for-next.
v5 changes:
1. Add Signed-off-by tag and Reviewed-by tag.
v4 changes:
1. Fixed wrong signed-off as correct mail suffix.
2. Fixed patch subject that add patch version.
v3 changes:
1. https://patchwork.kernel.org/patch/11164283/ and
https://patchwork.kernel.org/patch/11164305/ has been merged.
2. Change the name of mtk_wdt_compatible to mtk_wdt_data.
3. Remove toprgu_reset struct and use mtk_wdt_dev instead.
4. Get the value of sw_rst_num from .h file.
5. Adddd mt2712-resets.h for mt2712.
6. Improve commit message.
v2 changes:
1. remove "WIP" that in the title of patches
2. add hyper link for the patch that depends on
3. patchwork list:
https://patchwork.kernel.org/cover/11164285/
https://patchwork.kernel.org/patch/11164295/
https://patchwork.kernel.org/patch/11164299/
https://patchwork.kernel.org/patch/11164283/
https://patchwork.kernel.org/patch/11164305/
v1 changes:
1. patchwork list:
https://patchwork.kernel.org/cover/11164173/
https://patchwork.kernel.org/patch/11164181/
https://patchwork.kernel.org/patch/11164185/
https://patchwork.kernel.org/patch/11164187/
https://patchwork.kernel.org/patch/11164175/
yong.liang (2):
dt-bindings: mediatek: mt8183: Add #reset-cells
watchdog: mtk_wdt: mt8183: Add reset controller
.../devicetree/bindings/watchdog/mtk-wdt.txt | 10 +-
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/mtk_wdt.c | 111 +++++++++++++++++-
.../reset-controller/mt2712-resets.h | 22 ++++
.../reset-controller/mt8183-resets.h | 15 +++
5 files changed, 155 insertions(+), 4 deletions(-)
create mode 100644 include/dt-bindings/reset-controller/mt2712-resets.h
--
2.18.0
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [alsa-devel] [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
2019-11-25 3:03 [alsa-devel] [PATCH v5 0/2] ASoC: mt8183: fix audio playback slowly after playback Jiaxin Yu
@ 2019-11-25 3:03 ` Jiaxin Yu
0 siblings, 0 replies; 21+ messages in thread
From: Jiaxin Yu @ 2019-11-25 3:03 UTC (permalink / raw)
To: broonie, mark.rutland, yingjoe.chen, p.zabel, robh+dt, linux, wim
Cc: alsa-devel, yong.liang, lgirdwood, jiaxin.yu, tzungbi,
linux-mediatek, eason.yen, linux-arm-kernel
From: "yong.liang" <yong.liang@mediatek.com>
Add reset controller API in watchdog driver.
Besides watchdog, MTK toprgu module also provide sub-system (eg, audio,
camera, codec and connectivity) software reset functionality.
Signed-off-by: yong.liang <yong.liang@mediatek.com>
Signed-off-by: jiaxin.yu <jiaxin.yu@mediatek.com>
Reviewed-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/mtk_wdt.c | 111 ++++++++++++++++++++++++++++++++++++-
2 files changed, 111 insertions(+), 1 deletion(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 2e07caab9db2..629249fe5305 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
tristate "Mediatek SoCs watchdog support"
depends on ARCH_MEDIATEK || COMPILE_TEST
select WATCHDOG_CORE
+ select RESET_CONTROLLER
help
Say Y here to include support for the watchdog timer
in Mediatek SoCs.
diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
index 9c3d0033260d..d29484c7940a 100644
--- a/drivers/watchdog/mtk_wdt.c
+++ b/drivers/watchdog/mtk_wdt.c
@@ -9,6 +9,9 @@
* Based on sunxi_wdt.c
*/
+#include <dt-bindings/reset-controller/mt2712-resets.h>
+#include <dt-bindings/reset-controller/mt8183-resets.h>
+#include <linux/delay.h>
#include <linux/err.h>
#include <linux/init.h>
#include <linux/io.h>
@@ -16,10 +19,12 @@
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/of.h>
+#include <linux/of_device.h>
#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/slab.h>
#include <linux/types.h>
#include <linux/watchdog.h>
-#include <linux/delay.h>
#define WDT_MAX_TIMEOUT 31
#define WDT_MIN_TIMEOUT 1
@@ -44,6 +49,9 @@
#define WDT_SWRST 0x14
#define WDT_SWRST_KEY 0x1209
+#define WDT_SWSYSRST 0x18U
+#define WDT_SWSYS_RST_KEY 0x88000000
+
#define DRV_NAME "mtk-wdt"
#define DRV_VERSION "1.0"
@@ -53,8 +61,99 @@ static unsigned int timeout;
struct mtk_wdt_dev {
struct watchdog_device wdt_dev;
void __iomem *wdt_base;
+ spinlock_t lock; /* protects WDT_SWSYSRST reg */
+ struct reset_controller_dev rcdev;
+};
+
+struct mtk_wdt_data {
+ int sw_rst_num;
};
+static const struct mtk_wdt_data mt2712_data = {
+ .sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
+};
+
+static const struct mtk_wdt_data mt8183_data = {
+ .sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
+};
+
+static int toprgu_reset_assert(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ unsigned int tmp;
+ unsigned long flags;
+ struct mtk_wdt_dev *data =
+ container_of(rcdev, struct mtk_wdt_dev, rcdev);
+
+ spin_lock_irqsave(&data->lock, flags);
+
+ tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
+ tmp |= BIT(id);
+ tmp |= WDT_SWSYS_RST_KEY;
+ writel(tmp, data->wdt_base + WDT_SWSYSRST);
+
+ spin_unlock_irqrestore(&data->lock, flags);
+
+ return 0;
+}
+
+static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ unsigned int tmp;
+ unsigned long flags;
+ struct mtk_wdt_dev *data =
+ container_of(rcdev, struct mtk_wdt_dev, rcdev);
+
+ spin_lock_irqsave(&data->lock, flags);
+
+ tmp = __raw_readl(data->wdt_base + WDT_SWSYSRST);
+ tmp &= ~BIT(id);
+ tmp |= WDT_SWSYS_RST_KEY;
+ writel(tmp, data->wdt_base + WDT_SWSYSRST);
+
+ spin_unlock_irqrestore(&data->lock, flags);
+
+ return 0;
+}
+
+static int toprgu_reset(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ int ret;
+
+ ret = toprgu_reset_assert(rcdev, id);
+ if (ret)
+ return ret;
+
+ return toprgu_reset_deassert(rcdev, id);
+}
+
+static const struct reset_control_ops toprgu_reset_ops = {
+ .assert = toprgu_reset_assert,
+ .deassert = toprgu_reset_deassert,
+ .reset = toprgu_reset,
+};
+
+static int toprgu_register_reset_controller(struct platform_device *pdev,
+ int rst_num)
+{
+ int ret;
+ struct mtk_wdt_dev *mtk_wdt = platform_get_drvdata(pdev);
+
+ spin_lock_init(&mtk_wdt->lock);
+
+ mtk_wdt->rcdev.owner = THIS_MODULE;
+ mtk_wdt->rcdev.nr_resets = rst_num;
+ mtk_wdt->rcdev.ops = &toprgu_reset_ops;
+ mtk_wdt->rcdev.of_node = pdev->dev.of_node;
+ ret = reset_controller_register(&mtk_wdt->rcdev);
+ if (ret != 0)
+ dev_err(&pdev->dev,
+ "couldn't register wdt reset controller: %d\n", ret);
+ return ret;
+}
+
static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
unsigned long action, void *data)
{
@@ -155,6 +254,7 @@ static int mtk_wdt_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct mtk_wdt_dev *mtk_wdt;
+ struct mtk_wdt_data *wdt_data;
int err;
mtk_wdt = devm_kzalloc(dev, sizeof(*mtk_wdt), GFP_KERNEL);
@@ -190,6 +290,13 @@ static int mtk_wdt_probe(struct platform_device *pdev)
dev_info(dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)\n",
mtk_wdt->wdt_dev.timeout, nowayout);
+ wdt_data = (struct mtk_wdt_data *)of_device_get_match_data(dev);
+ if (wdt_data) {
+ err = toprgu_register_reset_controller(pdev,
+ wdt_data->sw_rst_num);
+ if (err)
+ return err;
+ }
return 0;
}
@@ -218,7 +325,9 @@ static int mtk_wdt_resume(struct device *dev)
#endif
static const struct of_device_id mtk_wdt_dt_ids[] = {
+ { .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },
{ .compatible = "mediatek,mt6589-wdt" },
+ { .compatible = "mediatek,mt8183-wdt", .data = &mt8183_data },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, mtk_wdt_dt_ids);
--
2.18.0
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread
end of thread, other threads:[~2019-12-03 5:06 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-11-25 6:16 [alsa-devel] [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller Guenter Roeck
2019-11-25 6:16 ` Guenter Roeck
2019-11-25 9:51 ` [alsa-devel] " Philipp Zabel
2019-11-25 9:51 ` Philipp Zabel
2019-11-25 9:51 ` Philipp Zabel
2019-11-29 8:36 ` [alsa-devel] " Yong Liang
2019-11-29 8:36 ` Yong Liang
2019-11-29 8:36 ` Yong Liang
2019-12-02 13:02 ` [alsa-devel] " Philipp Zabel
2019-12-02 13:02 ` Philipp Zabel
2019-12-02 13:02 ` Philipp Zabel
2019-12-03 3:02 ` [alsa-devel] " Yong Liang
2019-12-03 3:02 ` Yong Liang
2019-12-03 3:02 ` Yong Liang
2019-12-03 5:05 ` [alsa-devel] " Guenter Roeck
2019-12-03 5:05 ` Guenter Roeck
2019-12-03 5:05 ` Guenter Roeck
2019-11-29 10:39 ` [alsa-devel] " Yong Liang
2019-11-29 10:39 ` Yong Liang
2019-11-29 10:39 ` Yong Liang
-- strict thread matches above, loose matches on Subject: below --
2019-11-25 3:03 [alsa-devel] [PATCH v5 0/2] ASoC: mt8183: fix audio playback slowly after playback Jiaxin Yu
2019-11-25 3:03 ` [alsa-devel] [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller Jiaxin Yu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.