* [PATCH v5 0/2] ASoC: mt8183: fix audio playback slowly after playback
@ 2019-11-25 3:03 Jiaxin Yu
2019-11-25 3:03 ` [PATCH v5 1/2] dt-bindings: mediatek: mt8183: Add #reset-cells Jiaxin Yu
2019-11-25 3:03 ` [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller Jiaxin Yu
0 siblings, 2 replies; 16+ 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, perex, 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
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v5 1/2] dt-bindings: mediatek: mt8183: Add #reset-cells
2019-11-25 3:03 [PATCH v5 0/2] ASoC: mt8183: fix audio playback slowly after playback Jiaxin Yu
@ 2019-11-25 3:03 ` Jiaxin Yu
2019-11-25 5:59 ` Guenter Roeck
2019-11-25 10:07 ` Philipp Zabel
2019-11-25 3:03 ` [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller Jiaxin Yu
1 sibling, 2 replies; 16+ 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, perex, tzungbi,
linux-mediatek, eason.yen, linux-arm-kernel
From: "yong.liang" <yong.liang@mediatek.com>
Add #reset-cells property and update example
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>
---
.../devicetree/bindings/watchdog/mtk-wdt.txt | 10 ++++++---
.../reset-controller/mt2712-resets.h | 22 +++++++++++++++++++
.../reset-controller/mt8183-resets.h | 15 +++++++++++++
3 files changed, 44 insertions(+), 3 deletions(-)
create mode 100644 include/dt-bindings/reset-controller/mt2712-resets.h
diff --git a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
index 3ee625d0812f..4dd36bd3f1ad 100644
--- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
@@ -4,6 +4,7 @@ Required properties:
- compatible should contain:
"mediatek,mt2701-wdt", "mediatek,mt6589-wdt": for MT2701
+ "mediatek,mt2712-wdt", "mediatek,mt6589-wdt": for MT2712
"mediatek,mt6589-wdt": for MT6589
"mediatek,mt6797-wdt", "mediatek,mt6589-wdt": for MT6797
"mediatek,mt7622-wdt", "mediatek,mt6589-wdt": for MT7622
@@ -16,11 +17,14 @@ Required properties:
Optional properties:
- timeout-sec: contains the watchdog timeout in seconds.
+- #reset-cells: Should be 1.
Example:
-wdt: watchdog@10000000 {
- compatible = "mediatek,mt6589-wdt";
- reg = <0x10000000 0x18>;
+watchdog: watchdog@10007000 {
+ compatible = "mediatek,mt8183-wdt",
+ "mediatek,mt6589-wdt";
+ reg = <0 0x10007000 0 0x100>;
timeout-sec = <10>;
+ #reset-cells = <1>;
};
diff --git a/include/dt-bindings/reset-controller/mt2712-resets.h b/include/dt-bindings/reset-controller/mt2712-resets.h
new file mode 100644
index 000000000000..e81c8bb311b7
--- /dev/null
+++ b/include/dt-bindings/reset-controller/mt2712-resets.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2019 MediaTek Inc.
+ * Author: Yong Liang <yong.liang@mediatek.com>
+ */
+
+#ifndef _DT_BINDINGS_RESET_CONTROLLER_MT2712
+#define _DT_BINDINGS_RESET_CONTROLLER_MT2712
+
+#define MT2712_TOPRGU_INFRA_SW_RST 0
+#define MT2712_TOPRGU_MM_SW_RST 1
+#define MT2712_TOPRGU_MFG_SW_RST 2
+#define MT2712_TOPRGU_VENC_SW_RST 3
+#define MT2712_TOPRGU_VDEC_SW_RST 4
+#define MT2712_TOPRGU_IMG_SW_RST 5
+#define MT2712_TOPRGU_INFRA_AO_SW_RST 8
+#define MT2712_TOPRGU_USB_SW_RST 9
+#define MT2712_TOPRGU_APMIXED_SW_RST 10
+
+#define MT2712_TOPRGU_SW_RST_NUM 10
+
+#endif /* _DT_BINDINGS_RESET_CONTROLLER_MT2712 */
diff --git a/include/dt-bindings/reset-controller/mt8183-resets.h b/include/dt-bindings/reset-controller/mt8183-resets.h
index 8804e34ebdd4..d582da6bedae 100644
--- a/include/dt-bindings/reset-controller/mt8183-resets.h
+++ b/include/dt-bindings/reset-controller/mt8183-resets.h
@@ -78,4 +78,19 @@
#define MT8183_INFRACFG_AO_I2C7_SW_RST 126
#define MT8183_INFRACFG_AO_I2C8_SW_RST 127
+#define MT8183_TOPRGU_MM_SW_RST 1
+#define MT8183_TOPRGU_MFG_SW_RST 2
+#define MT8183_TOPRGU_VENC_SW_RST 3
+#define MT8183_TOPRGU_VDEC_SW_RST 4
+#define MT8183_TOPRGU_IMG_SW_RST 5
+#define MT8183_TOPRGU_MD_SW_RST 7
+#define MT8183_TOPRGU_CONN_SW_RST 9
+#define MT8183_TOPRGU_CONN_MCU_SW_RST 12
+#define MT8183_TOPRGU_IPU0_SW_RST 14
+#define MT8183_TOPRGU_IPU1_SW_RST 15
+#define MT8183_TOPRGU_AUDIO_SW_RST 17
+#define MT8183_TOPRGU_CAMSYS_SW_RST 18
+
+#define MT8183_TOPRGU_SW_RST_NUM 18
+
#endif /* _DT_BINDINGS_RESET_CONTROLLER_MT8183 */
--
2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller
2019-11-25 3:03 [PATCH v5 0/2] ASoC: mt8183: fix audio playback slowly after playback Jiaxin Yu
2019-11-25 3:03 ` [PATCH v5 1/2] dt-bindings: mediatek: mt8183: Add #reset-cells Jiaxin Yu
@ 2019-11-25 3:03 ` Jiaxin Yu
1 sibling, 0 replies; 16+ 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, perex, 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
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v5 1/2] dt-bindings: mediatek: mt8183: Add #reset-cells
2019-11-25 3:03 ` [PATCH v5 1/2] dt-bindings: mediatek: mt8183: Add #reset-cells Jiaxin Yu
@ 2019-11-25 5:59 ` Guenter Roeck
2019-11-25 10:07 ` Philipp Zabel
1 sibling, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2019-11-25 5:59 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:49AM +0800, Jiaxin Yu wrote:
> From: "yong.liang" <yong.liang@mediatek.com>
>
> Add #reset-cells property and update example
>
> 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>
I keep wondering why your series does not show up in the watchdog patchwork.
Turns out that is not surprising, since you don't copy the watchdog mailing
list. That all but guarantees that it gets lost from my workflow. That is
kind of interesting - there are lots of lists in Cc:, but the two lists
that matter because maintainers need to review it (watchdog and devicetree)
are missing.
Anyway, comment and question below.
> ---
> .../devicetree/bindings/watchdog/mtk-wdt.txt | 10 ++++++---
> .../reset-controller/mt2712-resets.h | 22 +++++++++++++++++++
> .../reset-controller/mt8183-resets.h | 15 +++++++++++++
> 3 files changed, 44 insertions(+), 3 deletions(-)
> create mode 100644 include/dt-bindings/reset-controller/mt2712-resets.h
>
> diff --git a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> index 3ee625d0812f..4dd36bd3f1ad 100644
> --- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> @@ -4,6 +4,7 @@ Required properties:
>
> - compatible should contain:
> "mediatek,mt2701-wdt", "mediatek,mt6589-wdt": for MT2701
> + "mediatek,mt2712-wdt", "mediatek,mt6589-wdt": for MT2712
Is that (still) correct ? After all, behavior is different for those two now.
Just wondering.
Guenter
> "mediatek,mt6589-wdt": for MT6589
> "mediatek,mt6797-wdt", "mediatek,mt6589-wdt": for MT6797
> "mediatek,mt7622-wdt", "mediatek,mt6589-wdt": for MT7622
> @@ -16,11 +17,14 @@ Required properties:
>
> Optional properties:
> - timeout-sec: contains the watchdog timeout in seconds.
> +- #reset-cells: Should be 1.
>
> Example:
>
> -wdt: watchdog@10000000 {
> - compatible = "mediatek,mt6589-wdt";
> - reg = <0x10000000 0x18>;
> +watchdog: watchdog@10007000 {
> + compatible = "mediatek,mt8183-wdt",
> + "mediatek,mt6589-wdt";
> + reg = <0 0x10007000 0 0x100>;
> timeout-sec = <10>;
> + #reset-cells = <1>;
> };
> diff --git a/include/dt-bindings/reset-controller/mt2712-resets.h b/include/dt-bindings/reset-controller/mt2712-resets.h
> new file mode 100644
> index 000000000000..e81c8bb311b7
> --- /dev/null
> +++ b/include/dt-bindings/reset-controller/mt2712-resets.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2019 MediaTek Inc.
> + * Author: Yong Liang <yong.liang@mediatek.com>
> + */
> +
> +#ifndef _DT_BINDINGS_RESET_CONTROLLER_MT2712
> +#define _DT_BINDINGS_RESET_CONTROLLER_MT2712
> +
> +#define MT2712_TOPRGU_INFRA_SW_RST 0
> +#define MT2712_TOPRGU_MM_SW_RST 1
> +#define MT2712_TOPRGU_MFG_SW_RST 2
> +#define MT2712_TOPRGU_VENC_SW_RST 3
> +#define MT2712_TOPRGU_VDEC_SW_RST 4
> +#define MT2712_TOPRGU_IMG_SW_RST 5
> +#define MT2712_TOPRGU_INFRA_AO_SW_RST 8
> +#define MT2712_TOPRGU_USB_SW_RST 9
> +#define MT2712_TOPRGU_APMIXED_SW_RST 10
> +
> +#define MT2712_TOPRGU_SW_RST_NUM 10
> +
> +#endif /* _DT_BINDINGS_RESET_CONTROLLER_MT2712 */
> diff --git a/include/dt-bindings/reset-controller/mt8183-resets.h b/include/dt-bindings/reset-controller/mt8183-resets.h
> index 8804e34ebdd4..d582da6bedae 100644
> --- a/include/dt-bindings/reset-controller/mt8183-resets.h
> +++ b/include/dt-bindings/reset-controller/mt8183-resets.h
> @@ -78,4 +78,19 @@
> #define MT8183_INFRACFG_AO_I2C7_SW_RST 126
> #define MT8183_INFRACFG_AO_I2C8_SW_RST 127
>
> +#define MT8183_TOPRGU_MM_SW_RST 1
> +#define MT8183_TOPRGU_MFG_SW_RST 2
> +#define MT8183_TOPRGU_VENC_SW_RST 3
> +#define MT8183_TOPRGU_VDEC_SW_RST 4
> +#define MT8183_TOPRGU_IMG_SW_RST 5
> +#define MT8183_TOPRGU_MD_SW_RST 7
> +#define MT8183_TOPRGU_CONN_SW_RST 9
> +#define MT8183_TOPRGU_CONN_MCU_SW_RST 12
> +#define MT8183_TOPRGU_IPU0_SW_RST 14
> +#define MT8183_TOPRGU_IPU1_SW_RST 15
> +#define MT8183_TOPRGU_AUDIO_SW_RST 17
> +#define MT8183_TOPRGU_CAMSYS_SW_RST 18
> +
> +#define MT8183_TOPRGU_SW_RST_NUM 18
> +
> #endif /* _DT_BINDINGS_RESET_CONTROLLER_MT8183 */
> --
> 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] 16+ messages in thread
* Re: [PATCH v5 1/2] dt-bindings: mediatek: mt8183: Add #reset-cells
2019-11-25 3:03 ` [PATCH v5 1/2] dt-bindings: mediatek: mt8183: Add #reset-cells Jiaxin Yu
2019-11-25 5:59 ` Guenter Roeck
@ 2019-11-25 10:07 ` Philipp Zabel
2019-11-29 6:33 ` Yong Liang
` (3 more replies)
1 sibling, 4 replies; 16+ messages in thread
From: Philipp Zabel @ 2019-11-25 10:07 UTC (permalink / raw)
To: Jiaxin Yu, broonie, mark.rutland, yingjoe.chen, robh+dt, linux, wim
Cc: alsa-devel, yong.liang, lgirdwood, perex, tzungbi,
linux-mediatek, eason.yen, linux-arm-kernel
On Mon, 2019-11-25 at 11:03 +0800, Jiaxin Yu wrote:
> From: "yong.liang" <yong.liang@mediatek.com>
>
> Add #reset-cells property and update example
>
> 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>
> ---
> .../devicetree/bindings/watchdog/mtk-wdt.txt | 10 ++++++---
> .../reset-controller/mt2712-resets.h | 22 +++++++++++++++++++
> .../reset-controller/mt8183-resets.h | 15 +++++++++++++
> 3 files changed, 44 insertions(+), 3 deletions(-)
> create mode 100644 include/dt-bindings/reset-controller/mt2712-resets.h
>
> diff --git a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> index 3ee625d0812f..4dd36bd3f1ad 100644
> --- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> @@ -4,6 +4,7 @@ Required properties:
>
> - compatible should contain:
> "mediatek,mt2701-wdt", "mediatek,mt6589-wdt": for MT2701
> + "mediatek,mt2712-wdt", "mediatek,mt6589-wdt": for MT2712
> "mediatek,mt6589-wdt": for MT6589
> "mediatek,mt6797-wdt", "mediatek,mt6589-wdt": for MT6797
> "mediatek,mt7622-wdt", "mediatek,mt6589-wdt": for MT7622
> @@ -16,11 +17,14 @@ Required properties:
>
> Optional properties:
> - timeout-sec: contains the watchdog timeout in seconds.
> +- #reset-cells: Should be 1.
>
> Example:
>
> -wdt: watchdog@10000000 {
> - compatible = "mediatek,mt6589-wdt";
> - reg = <0x10000000 0x18>;
> +watchdog: watchdog@10007000 {
> + compatible = "mediatek,mt8183-wdt",
> + "mediatek,mt6589-wdt";
> + reg = <0 0x10007000 0 0x100>;
> timeout-sec = <10>;
> + #reset-cells = <1>;
> };
> diff --git a/include/dt-bindings/reset-controller/mt2712-resets.h b/include/dt-bindings/reset-controller/mt2712-resets.h
> new file mode 100644
> index 000000000000..e81c8bb311b7
> --- /dev/null
> +++ b/include/dt-bindings/reset-controller/mt2712-resets.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2019 MediaTek Inc.
> + * Author: Yong Liang <yong.liang@mediatek.com>
> + */
> +
> +#ifndef _DT_BINDINGS_RESET_CONTROLLER_MT2712
> +#define _DT_BINDINGS_RESET_CONTROLLER_MT2712
> +
> +#define MT2712_TOPRGU_INFRA_SW_RST 0
> +#define MT2712_TOPRGU_MM_SW_RST 1
> +#define MT2712_TOPRGU_MFG_SW_RST 2
> +#define MT2712_TOPRGU_VENC_SW_RST 3
> +#define MT2712_TOPRGU_VDEC_SW_RST 4
> +#define MT2712_TOPRGU_IMG_SW_RST 5
> +#define MT2712_TOPRGU_INFRA_AO_SW_RST 8
> +#define MT2712_TOPRGU_USB_SW_RST 9
> +#define MT2712_TOPRGU_APMIXED_SW_RST 10
> +
> +#define MT2712_TOPRGU_SW_RST_NUM 10
Setting rcdev->nr_resets to 10 will make the check in
of_reset_simple_xlate() fail for MT2712_TOPRGU_APMIXED_SW_RST.
> +
> +#endif /* _DT_BINDINGS_RESET_CONTROLLER_MT2712 */
> diff --git a/include/dt-bindings/reset-controller/mt8183-resets.h b/include/dt-bindings/reset-controller/mt8183-resets.h
> index 8804e34ebdd4..d582da6bedae 100644
> --- a/include/dt-bindings/reset-controller/mt8183-resets.h
> +++ b/include/dt-bindings/reset-controller/mt8183-resets.h
> @@ -78,4 +78,19 @@
> #define MT8183_INFRACFG_AO_I2C7_SW_RST 126
> #define MT8183_INFRACFG_AO_I2C8_SW_RST 127
>
> +#define MT8183_TOPRGU_MM_SW_RST 1
> +#define MT8183_TOPRGU_MFG_SW_RST 2
> +#define MT8183_TOPRGU_VENC_SW_RST 3
> +#define MT8183_TOPRGU_VDEC_SW_RST 4
> +#define MT8183_TOPRGU_IMG_SW_RST 5
> +#define MT8183_TOPRGU_MD_SW_RST 7
> +#define MT8183_TOPRGU_CONN_SW_RST 9
> +#define MT8183_TOPRGU_CONN_MCU_SW_RST 12
> +#define MT8183_TOPRGU_IPU0_SW_RST 14
> +#define MT8183_TOPRGU_IPU1_SW_RST 15
> +#define MT8183_TOPRGU_AUDIO_SW_RST 17
> +#define MT8183_TOPRGU_CAMSYS_SW_RST 18
> +
> +#define MT8183_TOPRGU_SW_RST_NUM 18
Same here. If the driver uses the default of_xlate function, this has to
be larger than the index of the last reset.
regards
Philipp
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 1/2] dt-bindings: mediatek: mt8183: Add #reset-cells
2019-11-25 10:07 ` Philipp Zabel
@ 2019-11-29 6:33 ` Yong Liang
2019-11-29 6:36 ` Yong Liang
` (2 subsequent siblings)
3 siblings, 0 replies; 16+ messages in thread
From: Yong Liang @ 2019-11-29 6:33 UTC (permalink / raw)
To: Philipp Zabel
Cc: mark.rutland, alsa-devel, lgirdwood, robh+dt, perex, tzungbi,
broonie, linux-mediatek, Jiaxin Yu (俞家鑫),
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲),
wim, linux, linux-arm-kernel
On Mon, 2019-11-25 at 18:07 +0800, Philipp Zabel wrote:
> On Mon, 2019-11-25 at 11:03 +0800, Jiaxin Yu wrote:
> > From: "yong.liang" <yong.liang@mediatek.com>
> >
> > Add #reset-cells property and update example
> >
> > 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>
> > ---
> > .../devicetree/bindings/watchdog/mtk-wdt.txt | 10 ++++++---
> > .../reset-controller/mt2712-resets.h | 22 +++++++++++++++++++
> > .../reset-controller/mt8183-resets.h | 15 +++++++++++++
> > 3 files changed, 44 insertions(+), 3 deletions(-)
> > create mode 100644 include/dt-bindings/reset-controller/mt2712-resets.h
> >
> > diff --git a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > index 3ee625d0812f..4dd36bd3f1ad 100644
> > --- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > +++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > @@ -4,6 +4,7 @@ Required properties:
> >
> > - compatible should contain:
> > "mediatek,mt2701-wdt", "mediatek,mt6589-wdt": for MT2701
> > + "mediatek,mt2712-wdt", "mediatek,mt6589-wdt": for MT2712
> > "mediatek,mt6589-wdt": for MT6589
> > "mediatek,mt6797-wdt", "mediatek,mt6589-wdt": for MT6797
> > "mediatek,mt7622-wdt", "mediatek,mt6589-wdt": for MT7622
> > @@ -16,11 +17,14 @@ Required properties:
> >
> > Optional properties:
> > - timeout-sec: contains the watchdog timeout in seconds.
> > +- #reset-cells: Should be 1.
> >
> > Example:
> >
> > -wdt: watchdog@10000000 {
> > - compatible = "mediatek,mt6589-wdt";
> > - reg = <0x10000000 0x18>;
> > +watchdog: watchdog@10007000 {
> > + compatible = "mediatek,mt8183-wdt",
> > + "mediatek,mt6589-wdt";
> > + reg = <0 0x10007000 0 0x100>;
> > timeout-sec = <10>;
> > + #reset-cells = <1>;
> > };
> > diff --git a/include/dt-bindings/reset-controller/mt2712-resets.h b/include/dt-bindings/reset-controller/mt2712-resets.h
> > new file mode 100644
> > index 000000000000..e81c8bb311b7
> > --- /dev/null
> > +++ b/include/dt-bindings/reset-controller/mt2712-resets.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + * Author: Yong Liang <yong.liang@mediatek.com>
> > + */
> > +
> > +#ifndef _DT_BINDINGS_RESET_CONTROLLER_MT2712
> > +#define _DT_BINDINGS_RESET_CONTROLLER_MT2712
> > +
> > +#define MT2712_TOPRGU_INFRA_SW_RST 0
> > +#define MT2712_TOPRGU_MM_SW_RST 1
> > +#define MT2712_TOPRGU_MFG_SW_RST 2
> > +#define MT2712_TOPRGU_VENC_SW_RST 3
> > +#define MT2712_TOPRGU_VDEC_SW_RST 4
> > +#define MT2712_TOPRGU_IMG_SW_RST 5
> > +#define MT2712_TOPRGU_INFRA_AO_SW_RST 8
> > +#define MT2712_TOPRGU_USB_SW_RST 9
> > +#define MT2712_TOPRGU_APMIXED_SW_RST 10
> > +
> > +#define MT2712_TOPRGU_SW_RST_NUM 10
>
> Setting rcdev->nr_resets to 10 will make the check in
> of_reset_simple_xlate() fail for MT2712_TOPRGU_APMIXED_SW_RST.
-> OK. I will change MT2712_TOPRGU_SW_RST_NUM from 10 to 11
>
> > +
> > +#endif /* _DT_BINDINGS_RESET_CONTROLLER_MT2712 */
> > diff --git a/include/dt-bindings/reset-controller/mt8183-resets.h b/include/dt-bindings/reset-controller/mt8183-resets.h
> > index 8804e34ebdd4..d582da6bedae 100644
> > --- a/include/dt-bindings/reset-controller/mt8183-resets.h
> > +++ b/include/dt-bindings/reset-controller/mt8183-resets.h
> > @@ -78,4 +78,19 @@
> > #define MT8183_INFRACFG_AO_I2C7_SW_RST 126
> > #define MT8183_INFRACFG_AO_I2C8_SW_RST 127
> >
> > +#define MT8183_TOPRGU_MM_SW_RST 1
> > +#define MT8183_TOPRGU_MFG_SW_RST 2
> > +#define MT8183_TOPRGU_VENC_SW_RST 3
> > +#define MT8183_TOPRGU_VDEC_SW_RST 4
> > +#define MT8183_TOPRGU_IMG_SW_RST 5
> > +#define MT8183_TOPRGU_MD_SW_RST 7
> > +#define MT8183_TOPRGU_CONN_SW_RST 9
> > +#define MT8183_TOPRGU_CONN_MCU_SW_RST 12
> > +#define MT8183_TOPRGU_IPU0_SW_RST 14
> > +#define MT8183_TOPRGU_IPU1_SW_RST 15
> > +#define MT8183_TOPRGU_AUDIO_SW_RST 17
> > +#define MT8183_TOPRGU_CAMSYS_SW_RST 18
> > +
> > +#define MT8183_TOPRGU_SW_RST_NUM 18
>
> Same here. If the driver uses the default of_xlate function, this has to
> be larger than the index of the last reset.
-> I will change MT8183_TOPRGU_SW_RST_NUM from 18 to 19
>
> 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] 16+ messages in thread
* Re: [PATCH v5 1/2] dt-bindings: mediatek: mt8183: Add #reset-cells
2019-11-25 10:07 ` Philipp Zabel
2019-11-29 6:33 ` Yong Liang
@ 2019-11-29 6:36 ` Yong Liang
2019-11-29 6:37 ` Yong Liang
2019-11-29 6:39 ` Yong Liang
3 siblings, 0 replies; 16+ messages in thread
From: Yong Liang @ 2019-11-29 6:36 UTC (permalink / raw)
To: Philipp Zabel
Cc: mark.rutland, alsa-devel, lgirdwood, robh+dt, perex, tzungbi,
broonie, linux-mediatek, Jiaxin Yu (俞家鑫),
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲),
wim, linux, linux-arm-kernel
On Mon, 2019-11-25 at 18:07 +0800, Philipp Zabel wrote:
> On Mon, 2019-11-25 at 11:03 +0800, Jiaxin Yu wrote:
> > From: "yong.liang" <yong.liang@mediatek.com>
> >
> > Add #reset-cells property and update example
> >
> > 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>
> > ---
> > .../devicetree/bindings/watchdog/mtk-wdt.txt | 10 ++++++---
> > .../reset-controller/mt2712-resets.h | 22 +++++++++++++++++++
> > .../reset-controller/mt8183-resets.h | 15 +++++++++++++
> > 3 files changed, 44 insertions(+), 3 deletions(-)
> > create mode 100644 include/dt-bindings/reset-controller/mt2712-resets.h
> >
> > diff --git a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > index 3ee625d0812f..4dd36bd3f1ad 100644
> > --- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > +++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > @@ -4,6 +4,7 @@ Required properties:
> >
> > - compatible should contain:
> > "mediatek,mt2701-wdt", "mediatek,mt6589-wdt": for MT2701
> > + "mediatek,mt2712-wdt", "mediatek,mt6589-wdt": for MT2712
> > "mediatek,mt6589-wdt": for MT6589
> > "mediatek,mt6797-wdt", "mediatek,mt6589-wdt": for MT6797
> > "mediatek,mt7622-wdt", "mediatek,mt6589-wdt": for MT7622
> > @@ -16,11 +17,14 @@ Required properties:
> >
> > Optional properties:
> > - timeout-sec: contains the watchdog timeout in seconds.
> > +- #reset-cells: Should be 1.
> >
> > Example:
> >
> > -wdt: watchdog@10000000 {
> > - compatible = "mediatek,mt6589-wdt";
> > - reg = <0x10000000 0x18>;
> > +watchdog: watchdog@10007000 {
> > + compatible = "mediatek,mt8183-wdt",
> > + "mediatek,mt6589-wdt";
> > + reg = <0 0x10007000 0 0x100>;
> > timeout-sec = <10>;
> > + #reset-cells = <1>;
> > };
> > diff --git a/include/dt-bindings/reset-controller/mt2712-resets.h b/include/dt-bindings/reset-controller/mt2712-resets.h
> > new file mode 100644
> > index 000000000000..e81c8bb311b7
> > --- /dev/null
> > +++ b/include/dt-bindings/reset-controller/mt2712-resets.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + * Author: Yong Liang <yong.liang@mediatek.com>
> > + */
> > +
> > +#ifndef _DT_BINDINGS_RESET_CONTROLLER_MT2712
> > +#define _DT_BINDINGS_RESET_CONTROLLER_MT2712
> > +
> > +#define MT2712_TOPRGU_INFRA_SW_RST 0
> > +#define MT2712_TOPRGU_MM_SW_RST 1
> > +#define MT2712_TOPRGU_MFG_SW_RST 2
> > +#define MT2712_TOPRGU_VENC_SW_RST 3
> > +#define MT2712_TOPRGU_VDEC_SW_RST 4
> > +#define MT2712_TOPRGU_IMG_SW_RST 5
> > +#define MT2712_TOPRGU_INFRA_AO_SW_RST 8
> > +#define MT2712_TOPRGU_USB_SW_RST 9
> > +#define MT2712_TOPRGU_APMIXED_SW_RST 10
> > +
> > +#define MT2712_TOPRGU_SW_RST_NUM 10
>
> Setting rcdev->nr_resets to 10 will make the check in
> of_reset_simple_xlate() fail for MT2712_TOPRGU_APMIXED_SW_RST.
-> OK. I will change MT2712_TOPRGU_SW_RST_NUM from 10 to 11
>
> > +
> > +#endif /* _DT_BINDINGS_RESET_CONTROLLER_MT2712 */
> > diff --git a/include/dt-bindings/reset-controller/mt8183-resets.h b/include/dt-bindings/reset-controller/mt8183-resets.h
> > index 8804e34ebdd4..d582da6bedae 100644
> > --- a/include/dt-bindings/reset-controller/mt8183-resets.h
> > +++ b/include/dt-bindings/reset-controller/mt8183-resets.h
> > @@ -78,4 +78,19 @@
> > #define MT8183_INFRACFG_AO_I2C7_SW_RST 126
> > #define MT8183_INFRACFG_AO_I2C8_SW_RST 127
> >
> > +#define MT8183_TOPRGU_MM_SW_RST 1
> > +#define MT8183_TOPRGU_MFG_SW_RST 2
> > +#define MT8183_TOPRGU_VENC_SW_RST 3
> > +#define MT8183_TOPRGU_VDEC_SW_RST 4
> > +#define MT8183_TOPRGU_IMG_SW_RST 5
> > +#define MT8183_TOPRGU_MD_SW_RST 7
> > +#define MT8183_TOPRGU_CONN_SW_RST 9
> > +#define MT8183_TOPRGU_CONN_MCU_SW_RST 12
> > +#define MT8183_TOPRGU_IPU0_SW_RST 14
> > +#define MT8183_TOPRGU_IPU1_SW_RST 15
> > +#define MT8183_TOPRGU_AUDIO_SW_RST 17
> > +#define MT8183_TOPRGU_CAMSYS_SW_RST 18
> > +
> > +#define MT8183_TOPRGU_SW_RST_NUM 18
>
> Same here. If the driver uses the default of_xlate function, this has to
> be larger than the index of the last reset.
-> I will change MT8183_TOPRGU_SW_RST_NUM from 18 to 19
>
> 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] 16+ messages in thread
* Re: [PATCH v5 1/2] dt-bindings: mediatek: mt8183: Add #reset-cells
2019-11-25 10:07 ` Philipp Zabel
2019-11-29 6:33 ` Yong Liang
2019-11-29 6:36 ` Yong Liang
@ 2019-11-29 6:37 ` Yong Liang
2019-11-29 6:39 ` Yong Liang
3 siblings, 0 replies; 16+ messages in thread
From: Yong Liang @ 2019-11-29 6:37 UTC (permalink / raw)
To: Philipp Zabel
Cc: mark.rutland, alsa-devel, lgirdwood, robh+dt, perex, tzungbi,
broonie, linux-mediatek, Jiaxin Yu (俞家鑫),
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲),
wim, linux, linux-arm-kernel
On Mon, 2019-11-25 at 18:07 +0800, Philipp Zabel wrote:
> On Mon, 2019-11-25 at 11:03 +0800, Jiaxin Yu wrote:
> > From: "yong.liang" <yong.liang@mediatek.com>
> >
> > Add #reset-cells property and update example
> >
> > 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>
> > ---
> > .../devicetree/bindings/watchdog/mtk-wdt.txt | 10 ++++++---
> > .../reset-controller/mt2712-resets.h | 22 +++++++++++++++++++
> > .../reset-controller/mt8183-resets.h | 15 +++++++++++++
> > 3 files changed, 44 insertions(+), 3 deletions(-)
> > create mode 100644 include/dt-bindings/reset-controller/mt2712-resets.h
> >
> > diff --git a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > index 3ee625d0812f..4dd36bd3f1ad 100644
> > --- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > +++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > @@ -4,6 +4,7 @@ Required properties:
> >
> > - compatible should contain:
> > "mediatek,mt2701-wdt", "mediatek,mt6589-wdt": for MT2701
> > + "mediatek,mt2712-wdt", "mediatek,mt6589-wdt": for MT2712
> > "mediatek,mt6589-wdt": for MT6589
> > "mediatek,mt6797-wdt", "mediatek,mt6589-wdt": for MT6797
> > "mediatek,mt7622-wdt", "mediatek,mt6589-wdt": for MT7622
> > @@ -16,11 +17,14 @@ Required properties:
> >
> > Optional properties:
> > - timeout-sec: contains the watchdog timeout in seconds.
> > +- #reset-cells: Should be 1.
> >
> > Example:
> >
> > -wdt: watchdog@10000000 {
> > - compatible = "mediatek,mt6589-wdt";
> > - reg = <0x10000000 0x18>;
> > +watchdog: watchdog@10007000 {
> > + compatible = "mediatek,mt8183-wdt",
> > + "mediatek,mt6589-wdt";
> > + reg = <0 0x10007000 0 0x100>;
> > timeout-sec = <10>;
> > + #reset-cells = <1>;
> > };
> > diff --git a/include/dt-bindings/reset-controller/mt2712-resets.h b/include/dt-bindings/reset-controller/mt2712-resets.h
> > new file mode 100644
> > index 000000000000..e81c8bb311b7
> > --- /dev/null
> > +++ b/include/dt-bindings/reset-controller/mt2712-resets.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + * Author: Yong Liang <yong.liang@mediatek.com>
> > + */
> > +
> > +#ifndef _DT_BINDINGS_RESET_CONTROLLER_MT2712
> > +#define _DT_BINDINGS_RESET_CONTROLLER_MT2712
> > +
> > +#define MT2712_TOPRGU_INFRA_SW_RST 0
> > +#define MT2712_TOPRGU_MM_SW_RST 1
> > +#define MT2712_TOPRGU_MFG_SW_RST 2
> > +#define MT2712_TOPRGU_VENC_SW_RST 3
> > +#define MT2712_TOPRGU_VDEC_SW_RST 4
> > +#define MT2712_TOPRGU_IMG_SW_RST 5
> > +#define MT2712_TOPRGU_INFRA_AO_SW_RST 8
> > +#define MT2712_TOPRGU_USB_SW_RST 9
> > +#define MT2712_TOPRGU_APMIXED_SW_RST 10
> > +
> > +#define MT2712_TOPRGU_SW_RST_NUM 10
>
> Setting rcdev->nr_resets to 10 will make the check in
> of_reset_simple_xlate() fail for MT2712_TOPRGU_APMIXED_SW_RST.
-> OK. I will change MT2712_TOPRGU_SW_RST_NUM from 10 to 11
>
> > +
> > +#endif /* _DT_BINDINGS_RESET_CONTROLLER_MT2712 */
> > diff --git a/include/dt-bindings/reset-controller/mt8183-resets.h b/include/dt-bindings/reset-controller/mt8183-resets.h
> > index 8804e34ebdd4..d582da6bedae 100644
> > --- a/include/dt-bindings/reset-controller/mt8183-resets.h
> > +++ b/include/dt-bindings/reset-controller/mt8183-resets.h
> > @@ -78,4 +78,19 @@
> > #define MT8183_INFRACFG_AO_I2C7_SW_RST 126
> > #define MT8183_INFRACFG_AO_I2C8_SW_RST 127
> >
> > +#define MT8183_TOPRGU_MM_SW_RST 1
> > +#define MT8183_TOPRGU_MFG_SW_RST 2
> > +#define MT8183_TOPRGU_VENC_SW_RST 3
> > +#define MT8183_TOPRGU_VDEC_SW_RST 4
> > +#define MT8183_TOPRGU_IMG_SW_RST 5
> > +#define MT8183_TOPRGU_MD_SW_RST 7
> > +#define MT8183_TOPRGU_CONN_SW_RST 9
> > +#define MT8183_TOPRGU_CONN_MCU_SW_RST 12
> > +#define MT8183_TOPRGU_IPU0_SW_RST 14
> > +#define MT8183_TOPRGU_IPU1_SW_RST 15
> > +#define MT8183_TOPRGU_AUDIO_SW_RST 17
> > +#define MT8183_TOPRGU_CAMSYS_SW_RST 18
> > +
> > +#define MT8183_TOPRGU_SW_RST_NUM 18
>
> Same here. If the driver uses the default of_xlate function, this has to
> be larger than the index of the last reset.
-> I will change MT8183_TOPRGU_SW_RST_NUM from 18 to 19
>
> 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] 16+ messages in thread
* Re: [PATCH v5 1/2] dt-bindings: mediatek: mt8183: Add #reset-cells
2019-11-25 10:07 ` Philipp Zabel
` (2 preceding siblings ...)
2019-11-29 6:37 ` Yong Liang
@ 2019-11-29 6:39 ` Yong Liang
3 siblings, 0 replies; 16+ messages in thread
From: Yong Liang @ 2019-11-29 6:39 UTC (permalink / raw)
To: Philipp Zabel
Cc: mark.rutland, alsa-devel, lgirdwood, robh+dt, perex, tzungbi,
broonie, linux-mediatek, Jiaxin Yu (俞家鑫),
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲),
wim, linux, linux-arm-kernel
On Mon, 2019-11-25 at 18:07 +0800, Philipp Zabel wrote:
> On Mon, 2019-11-25 at 11:03 +0800, Jiaxin Yu wrote:
> > From: "yong.liang" <yong.liang@mediatek.com>
> >
> > Add #reset-cells property and update example
> >
> > 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>
> > ---
> > .../devicetree/bindings/watchdog/mtk-wdt.txt | 10 ++++++---
> > .../reset-controller/mt2712-resets.h | 22 +++++++++++++++++++
> > .../reset-controller/mt8183-resets.h | 15 +++++++++++++
> > 3 files changed, 44 insertions(+), 3 deletions(-)
> > create mode 100644 include/dt-bindings/reset-controller/mt2712-resets.h
> >
> > diff --git a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > index 3ee625d0812f..4dd36bd3f1ad 100644
> > --- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > +++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > @@ -4,6 +4,7 @@ Required properties:
> >
> > - compatible should contain:
> > "mediatek,mt2701-wdt", "mediatek,mt6589-wdt": for MT2701
> > + "mediatek,mt2712-wdt", "mediatek,mt6589-wdt": for MT2712
> > "mediatek,mt6589-wdt": for MT6589
> > "mediatek,mt6797-wdt", "mediatek,mt6589-wdt": for MT6797
> > "mediatek,mt7622-wdt", "mediatek,mt6589-wdt": for MT7622
> > @@ -16,11 +17,14 @@ Required properties:
> >
> > Optional properties:
> > - timeout-sec: contains the watchdog timeout in seconds.
> > +- #reset-cells: Should be 1.
> >
> > Example:
> >
> > -wdt: watchdog@10000000 {
> > - compatible = "mediatek,mt6589-wdt";
> > - reg = <0x10000000 0x18>;
> > +watchdog: watchdog@10007000 {
> > + compatible = "mediatek,mt8183-wdt",
> > + "mediatek,mt6589-wdt";
> > + reg = <0 0x10007000 0 0x100>;
> > timeout-sec = <10>;
> > + #reset-cells = <1>;
> > };
> > diff --git a/include/dt-bindings/reset-controller/mt2712-resets.h b/include/dt-bindings/reset-controller/mt2712-resets.h
> > new file mode 100644
> > index 000000000000..e81c8bb311b7
> > --- /dev/null
> > +++ b/include/dt-bindings/reset-controller/mt2712-resets.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + * Author: Yong Liang <yong.liang@mediatek.com>
> > + */
> > +
> > +#ifndef _DT_BINDINGS_RESET_CONTROLLER_MT2712
> > +#define _DT_BINDINGS_RESET_CONTROLLER_MT2712
> > +
> > +#define MT2712_TOPRGU_INFRA_SW_RST 0
> > +#define MT2712_TOPRGU_MM_SW_RST 1
> > +#define MT2712_TOPRGU_MFG_SW_RST 2
> > +#define MT2712_TOPRGU_VENC_SW_RST 3
> > +#define MT2712_TOPRGU_VDEC_SW_RST 4
> > +#define MT2712_TOPRGU_IMG_SW_RST 5
> > +#define MT2712_TOPRGU_INFRA_AO_SW_RST 8
> > +#define MT2712_TOPRGU_USB_SW_RST 9
> > +#define MT2712_TOPRGU_APMIXED_SW_RST 10
> > +
> > +#define MT2712_TOPRGU_SW_RST_NUM 10
>
> Setting rcdev->nr_resets to 10 will make the check in
> of_reset_simple_xlate() fail for MT2712_TOPRGU_APMIXED_SW_RST.
-> OK. I will change MT2712_TOPRGU_SW_RST_NUM from 10 to 11
>
> > +
> > +#endif /* _DT_BINDINGS_RESET_CONTROLLER_MT2712 */
> > diff --git a/include/dt-bindings/reset-controller/mt8183-resets.h b/include/dt-bindings/reset-controller/mt8183-resets.h
> > index 8804e34ebdd4..d582da6bedae 100644
> > --- a/include/dt-bindings/reset-controller/mt8183-resets.h
> > +++ b/include/dt-bindings/reset-controller/mt8183-resets.h
> > @@ -78,4 +78,19 @@
> > #define MT8183_INFRACFG_AO_I2C7_SW_RST 126
> > #define MT8183_INFRACFG_AO_I2C8_SW_RST 127
> >
> > +#define MT8183_TOPRGU_MM_SW_RST 1
> > +#define MT8183_TOPRGU_MFG_SW_RST 2
> > +#define MT8183_TOPRGU_VENC_SW_RST 3
> > +#define MT8183_TOPRGU_VDEC_SW_RST 4
> > +#define MT8183_TOPRGU_IMG_SW_RST 5
> > +#define MT8183_TOPRGU_MD_SW_RST 7
> > +#define MT8183_TOPRGU_CONN_SW_RST 9
> > +#define MT8183_TOPRGU_CONN_MCU_SW_RST 12
> > +#define MT8183_TOPRGU_IPU0_SW_RST 14
> > +#define MT8183_TOPRGU_IPU1_SW_RST 15
> > +#define MT8183_TOPRGU_AUDIO_SW_RST 17
> > +#define MT8183_TOPRGU_CAMSYS_SW_RST 18
> > +
> > +#define MT8183_TOPRGU_SW_RST_NUM 18
>
> Same here. If the driver uses the default of_xlate function, this has to
> be larger than the index of the last reset.
-> I will change MT8183_TOPRGU_SW_RST_NUM from 18 to 19
>
> 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] 16+ messages in thread
* Re: [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
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2019-12-03 5:05 UTC (permalink / raw)
To: Yong Liang, Philipp Zabel
Cc: mark.rutland, alsa-devel, robh+dt, lgirdwood,
Jiaxin Yu (俞家鑫),
perex, tzungbi, broonie, linux-mediatek,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲),
wim, linux-arm-kernel
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] 16+ messages in thread
* Re: [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
2019-12-03 5:05 ` Guenter Roeck
0 siblings, 1 reply; 16+ messages in thread
From: Yong Liang @ 2019-12-03 3:02 UTC (permalink / raw)
To: Philipp Zabel
Cc: mark.rutland, alsa-devel, robh+dt, lgirdwood,
Jiaxin Yu (俞家鑫),
perex, tzungbi, broonie, linux-mediatek, Guenter Roeck,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲),
wim, linux-arm-kernel
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] 16+ messages in thread
* Re: [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
2019-12-03 3:02 ` Yong Liang
0 siblings, 1 reply; 16+ messages in thread
From: Philipp Zabel @ 2019-12-02 13:02 UTC (permalink / raw)
To: Yong Liang
Cc: mark.rutland, alsa-devel, broonie, lgirdwood,
Jiaxin Yu (俞家鑫),
perex, tzungbi, robh+dt, linux-mediatek, linux-arm-kernel,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲),
wim, 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] 16+ messages in thread
* Re: [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
@ 2019-11-29 10:39 ` Yong Liang
1 sibling, 0 replies; 16+ messages in thread
From: Yong Liang @ 2019-11-29 10:39 UTC (permalink / raw)
To: Guenter Roeck
Cc: mark.rutland, alsa-devel, lgirdwood, robh+dt, perex, tzungbi,
broonie, linux-mediatek, Jiaxin Yu (俞家鑫),
p.zabel, Eason Yen (顏廷任),
Yingjoe Chen (陳英洲),
wim, linux-arm-kernel
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] 16+ messages in thread
* Re: [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
2019-12-02 13:02 ` Philipp Zabel
0 siblings, 1 reply; 16+ messages in thread
From: Yong Liang @ 2019-11-29 8:36 UTC (permalink / raw)
To: Philipp Zabel
Cc: mark.rutland, alsa-devel, broonie, lgirdwood,
Jiaxin Yu (俞家鑫),
perex, tzungbi, robh+dt, linux-mediatek, linux-arm-kernel,
Eason Yen (顏廷任),
Yingjoe Chen (陳英洲),
wim, 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] 16+ messages in thread
* Re: [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
2019-11-29 8:36 ` Yong Liang
2019-11-29 10:39 ` Yong Liang
1 sibling, 1 reply; 16+ 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] 16+ messages in thread
* Re: [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
2019-11-29 10:39 ` Yong Liang
0 siblings, 2 replies; 16+ 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] 16+ messages in thread
end of thread, other threads:[~2019-12-03 5:05 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-25 3:03 [PATCH v5 0/2] ASoC: mt8183: fix audio playback slowly after playback Jiaxin Yu
2019-11-25 3:03 ` [PATCH v5 1/2] dt-bindings: mediatek: mt8183: Add #reset-cells Jiaxin Yu
2019-11-25 5:59 ` Guenter Roeck
2019-11-25 10:07 ` Philipp Zabel
2019-11-29 6:33 ` Yong Liang
2019-11-29 6:36 ` Yong Liang
2019-11-29 6:37 ` Yong Liang
2019-11-29 6:39 ` Yong Liang
2019-11-25 3:03 ` [PATCH v5 2/2] watchdog: mtk_wdt: mt8183: Add reset controller Jiaxin Yu
2019-11-25 6:16 Guenter Roeck
2019-11-25 9:51 ` Philipp Zabel
2019-11-29 8:36 ` Yong Liang
2019-12-02 13:02 ` Philipp Zabel
2019-12-03 3:02 ` Yong Liang
2019-12-03 5:05 ` Guenter Roeck
2019-11-29 10:39 ` Yong Liang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).