[PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Daniel Golle]

Add binding description for mediatek,mt7988-wdt.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 .../bindings/watchdog/mediatek,mtk-wdt.yaml          |  1 +
 include/dt-bindings/reset/mediatek,mt7988-resets.h   | 12 ++++++++++++
 2 files changed, 13 insertions(+)
 create mode 100644 include/dt-bindings/reset/mediatek,mt7988-resets.h

diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
index cc502838bc398..8d2520241e37f 100644
--- a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
@@ -25,6 +25,7 @@ properties:
           - mediatek,mt6735-wdt
           - mediatek,mt6795-wdt
           - mediatek,mt7986-wdt
+          - mediatek,mt7988-wdt
           - mediatek,mt8183-wdt
           - mediatek,mt8186-wdt
           - mediatek,mt8188-wdt
diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
new file mode 100644
index 0000000000000..fa7c937505e08
--- /dev/null
+++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+
+/* TOPRGU resets */
+#define MT7988_TOPRGU_SGMII0_GRST		1
+#define MT7988_TOPRGU_SGMII1_GRST		2
+#define MT7988_TOPRGU_XFI0_GRST			12
+#define MT7988_TOPRGU_XFI1_GRST			13
+#define MT7988_TOPRGU_XFI_PEXTP0_GRST		14
+#define MT7988_TOPRGU_XFI_PEXTP1_GRST		15
+#define MT7988_TOPRGU_XFI_PLL_GRST		16
+
+#define MT7988_TOPRGU_SW_RST_NUM		24
-- 
2.42.1

[PATCH 2/2] watchdog: mediatek: mt7988: add wdt support

[Daniel Golle]

Add support for watchdog and reset generator unit of the MediaTek
MT7988 SoC.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/watchdog/mtk_wdt.c | 56 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
index b2330b16b497a..b98b8c29735aa 100644
--- a/drivers/watchdog/mtk_wdt.c
+++ b/drivers/watchdog/mtk_wdt.c
@@ -12,6 +12,7 @@
 #include <dt-bindings/reset/mt2712-resets.h>
 #include <dt-bindings/reset/mediatek,mt6795-resets.h>
 #include <dt-bindings/reset/mt7986-resets.h>
+#include <dt-bindings/reset/mediatek,mt7988-resets.h>
 #include <dt-bindings/reset/mt8183-resets.h>
 #include <dt-bindings/reset/mt8186-resets.h>
 #include <dt-bindings/reset/mt8188-resets.h>
@@ -58,6 +59,8 @@
 #define WDT_SWSYSRST		0x18U
 #define WDT_SWSYS_RST_KEY	0x88000000
 
+#define WDT_SWSYSRST_EN		0xfc
+
 #define DRV_NAME		"mtk-wdt"
 #define DRV_VERSION		"1.0"
 
@@ -71,44 +74,85 @@ struct mtk_wdt_dev {
 	struct reset_controller_dev rcdev;
 	bool disable_wdt_extrst;
 	bool reset_by_toprgu;
+	bool has_swsysrst_en;
 };
 
 struct mtk_wdt_data {
 	int toprgu_sw_rst_num;
+	bool has_swsysrst_en;
 };
 
 static const struct mtk_wdt_data mt2712_data = {
 	.toprgu_sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
+	.has_swsysrst_en = false,
 };
 
 static const struct mtk_wdt_data mt6795_data = {
 	.toprgu_sw_rst_num = MT6795_TOPRGU_SW_RST_NUM,
+	.has_swsysrst_en = false,
 };
 
 static const struct mtk_wdt_data mt7986_data = {
 	.toprgu_sw_rst_num = MT7986_TOPRGU_SW_RST_NUM,
+	.has_swsysrst_en = false,
+};
+
+static const struct mtk_wdt_data mt7988_data = {
+	.toprgu_sw_rst_num = MT7988_TOPRGU_SW_RST_NUM,
+	.has_swsysrst_en = true,
 };
 
 static const struct mtk_wdt_data mt8183_data = {
 	.toprgu_sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
+	.has_swsysrst_en = false,
 };
 
 static const struct mtk_wdt_data mt8186_data = {
 	.toprgu_sw_rst_num = MT8186_TOPRGU_SW_RST_NUM,
+	.has_swsysrst_en = false,
 };
 
 static const struct mtk_wdt_data mt8188_data = {
 	.toprgu_sw_rst_num = MT8188_TOPRGU_SW_RST_NUM,
+	.has_swsysrst_en = false,
 };
 
 static const struct mtk_wdt_data mt8192_data = {
 	.toprgu_sw_rst_num = MT8192_TOPRGU_SW_RST_NUM,
+	.has_swsysrst_en = false,
 };
 
 static const struct mtk_wdt_data mt8195_data = {
 	.toprgu_sw_rst_num = MT8195_TOPRGU_SW_RST_NUM,
+	.has_swsysrst_en = false,
 };
 
+static int toprgu_reset_sw_enable(struct reset_controller_dev *rcdev,
+				  unsigned long id, bool enable)
+{
+	unsigned int tmp;
+	unsigned long flags;
+	struct mtk_wdt_dev *data =
+		 container_of(rcdev, struct mtk_wdt_dev, rcdev);
+
+	if (!data->has_swsysrst_en)
+		return 0;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	tmp = readl(data->wdt_base + WDT_SWSYSRST_EN);
+	if (enable)
+		tmp |= BIT(id);
+	else
+		tmp &= ~BIT(id);
+
+	writel(tmp, data->wdt_base + WDT_SWSYSRST_EN);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	return 0;
+}
+
 static int toprgu_reset_update(struct reset_controller_dev *rcdev,
 			       unsigned long id, bool assert)
 {
@@ -135,13 +179,20 @@ static int toprgu_reset_update(struct reset_controller_dev *rcdev,
 static int toprgu_reset_assert(struct reset_controller_dev *rcdev,
 			       unsigned long id)
 {
+	int ret;
+
+	ret = toprgu_reset_sw_enable(rcdev, id, true);
+	if (ret)
+		return ret;
+
 	return toprgu_reset_update(rcdev, id, true);
 }
 
 static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
 				 unsigned long id)
 {
-	return toprgu_reset_update(rcdev, id, false);
+	toprgu_reset_update(rcdev, id, false);
+	return toprgu_reset_sw_enable(rcdev, id, false);
 }
 
 static int toprgu_reset(struct reset_controller_dev *rcdev,
@@ -406,6 +457,8 @@ static int mtk_wdt_probe(struct platform_device *pdev)
 						       wdt_data->toprgu_sw_rst_num);
 		if (err)
 			return err;
+
+		mtk_wdt->has_swsysrst_en = wdt_data->has_swsysrst_en;
 	}
 
 	mtk_wdt->disable_wdt_extrst =
@@ -444,6 +497,7 @@ static const struct of_device_id mtk_wdt_dt_ids[] = {
 	{ .compatible = "mediatek,mt6589-wdt" },
 	{ .compatible = "mediatek,mt6795-wdt", .data = &mt6795_data },
 	{ .compatible = "mediatek,mt7986-wdt", .data = &mt7986_data },
+	{ .compatible = "mediatek,mt7988-wdt", .data = &mt7988_data },
 	{ .compatible = "mediatek,mt8183-wdt", .data = &mt8183_data },
 	{ .compatible = "mediatek,mt8186-wdt", .data = &mt8186_data },
 	{ .compatible = "mediatek,mt8188-wdt", .data = &mt8188_data },
-- 
2.42.1

Re: [PATCH 2/2] watchdog: mediatek: mt7988: add wdt support

[Guenter Roeck]

On 11/9/23 16:30, Daniel Golle wrote:
> Add support for watchdog and reset generator unit of the MediaTek
> MT7988 SoC.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>   drivers/watchdog/mtk_wdt.c | 56 +++++++++++++++++++++++++++++++++++++-
>   1 file changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> index b2330b16b497a..b98b8c29735aa 100644
> --- a/drivers/watchdog/mtk_wdt.c
> +++ b/drivers/watchdog/mtk_wdt.c
> @@ -12,6 +12,7 @@
>   #include <dt-bindings/reset/mt2712-resets.h>
>   #include <dt-bindings/reset/mediatek,mt6795-resets.h>
>   #include <dt-bindings/reset/mt7986-resets.h>
> +#include <dt-bindings/reset/mediatek,mt7988-resets.h>
>   #include <dt-bindings/reset/mt8183-resets.h>
>   #include <dt-bindings/reset/mt8186-resets.h>
>   #include <dt-bindings/reset/mt8188-resets.h>
> @@ -58,6 +59,8 @@
>   #define WDT_SWSYSRST		0x18U
>   #define WDT_SWSYS_RST_KEY	0x88000000
>   
> +#define WDT_SWSYSRST_EN		0xfc
> +
>   #define DRV_NAME		"mtk-wdt"
>   #define DRV_VERSION		"1.0"
>   
> @@ -71,44 +74,85 @@ struct mtk_wdt_dev {
>   	struct reset_controller_dev rcdev;
>   	bool disable_wdt_extrst;
>   	bool reset_by_toprgu;
> +	bool has_swsysrst_en;
>   };
>   
>   struct mtk_wdt_data {
>   	int toprgu_sw_rst_num;
> +	bool has_swsysrst_en;
>   };
>   
>   static const struct mtk_wdt_data mt2712_data = {
>   	.toprgu_sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = false,

Those assignments to false, just like assignments to 0, are unnecessary
for static variables.

>   };
>   
>   static const struct mtk_wdt_data mt6795_data = {
>   	.toprgu_sw_rst_num = MT6795_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = false,
>   };
>   
>   static const struct mtk_wdt_data mt7986_data = {
>   	.toprgu_sw_rst_num = MT7986_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = false,
> +};
> +
> +static const struct mtk_wdt_data mt7988_data = {
> +	.toprgu_sw_rst_num = MT7988_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = true,
>   };
>   
>   static const struct mtk_wdt_data mt8183_data = {
>   	.toprgu_sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = false,
>   };
>   
>   static const struct mtk_wdt_data mt8186_data = {
>   	.toprgu_sw_rst_num = MT8186_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = false,
>   };
>   
>   static const struct mtk_wdt_data mt8188_data = {
>   	.toprgu_sw_rst_num = MT8188_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = false,
>   };
>   
>   static const struct mtk_wdt_data mt8192_data = {
>   	.toprgu_sw_rst_num = MT8192_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = false,
>   };
>   
>   static const struct mtk_wdt_data mt8195_data = {
>   	.toprgu_sw_rst_num = MT8195_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = false,
>   };
>   
> +static int toprgu_reset_sw_enable(struct reset_controller_dev *rcdev,
> +				  unsigned long id, bool enable)

This function name is a bit misleading. It doesn't always
_enable_ something, it updates it based on the enable parameter.

> +{
> +	unsigned int tmp;
> +	unsigned long flags;
> +	struct mtk_wdt_dev *data =
> +		 container_of(rcdev, struct mtk_wdt_dev, rcdev);
> +
> +	if (!data->has_swsysrst_en)
> +		return 0;
> +
> +	spin_lock_irqsave(&data->lock, flags);
> +
> +	tmp = readl(data->wdt_base + WDT_SWSYSRST_EN);
> +	if (enable)
> +		tmp |= BIT(id);
> +	else
> +		tmp &= ~BIT(id);
> +
> +	writel(tmp, data->wdt_base + WDT_SWSYSRST_EN);
> +
> +	spin_unlock_irqrestore(&data->lock, flags);
> +

I find this code quite confusing. If it is really necessary to set both
WDT_SWSYSRST_EN and WDT_SWSYSRST together, what is the point of locking twice ?
Why not just handle this in toprgu_reset_update() while the lock is
alread held ? There is a lot of code duplication and inefficiency between
toprgu_reset_sw_enable() and toprgu_reset_update(), and I really don't
see the value of it if  WDT_SWSYSRST_EN and WDT_SWSYSRST have to be
written together anyway.

> +	return 0;

This function always returns 0. That does not add any value.

> +}
> +
>   static int toprgu_reset_update(struct reset_controller_dev *rcdev,
>   			       unsigned long id, bool assert)
>   {
> @@ -135,13 +179,20 @@ static int toprgu_reset_update(struct reset_controller_dev *rcdev,
>   static int toprgu_reset_assert(struct reset_controller_dev *rcdev,
>   			       unsigned long id)
>   {
> +	int ret;
> +
> +	ret = toprgu_reset_sw_enable(rcdev, id, true);
> +	if (ret)
> +		return ret;
> +

I am kind of missing the point of this return value check. I guess it is in line
with the other unnecessary return values / return value checks in this code,
but this really gets a bit out of control. It kind of creates the wrong
assumption or expectation that the called code _may_ return an error,
but in reality it doesn't.

>   	return toprgu_reset_update(rcdev, id, true);
>   }
>   
>   static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
>   				 unsigned long id)
>   {
> -	return toprgu_reset_update(rcdev, id, false);
> +	toprgu_reset_update(rcdev, id, false);

In a way it is commendable that the unnecessary return value handling was dropped,
but that makes the code inconsistent with the reset_assert() function. Also, it is
inconsistent to have the unnecessary return value check in toprgu_reset_assert()
but not here.

> +	return toprgu_reset_sw_enable(rcdev, id, false);
>   }
>   
>   static int toprgu_reset(struct reset_controller_dev *rcdev,
> @@ -406,6 +457,8 @@ static int mtk_wdt_probe(struct platform_device *pdev)
>   						       wdt_data->toprgu_sw_rst_num);
>   		if (err)
>   			return err;
> +
> +		mtk_wdt->has_swsysrst_en = wdt_data->has_swsysrst_en;

This is too late. The reset controller is already registered here,
and the reset controller functions may already have been called.

>   	}
>   
>   	mtk_wdt->disable_wdt_extrst =

Oh well, this and the next property are also called too late because they
affect watchdog operation and the watchdog device has already been registered,
but that is a different bug and not a reason to add even more race conditions
to the driver.


> @@ -444,6 +497,7 @@ static const struct of_device_id mtk_wdt_dt_ids[] = {
>   	{ .compatible = "mediatek,mt6589-wdt" },
>   	{ .compatible = "mediatek,mt6795-wdt", .data = &mt6795_data },
>   	{ .compatible = "mediatek,mt7986-wdt", .data = &mt7986_data },
> +	{ .compatible = "mediatek,mt7988-wdt", .data = &mt7988_data },
>   	{ .compatible = "mediatek,mt8183-wdt", .data = &mt8183_data },
>   	{ .compatible = "mediatek,mt8186-wdt", .data = &mt8186_data },
>   	{ .compatible = "mediatek,mt8188-wdt", .data = &mt8188_data },

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Krzysztof Kozlowski]

On 10/11/2023 01:30, Daniel Golle wrote:
> Add binding description for mediatek,mt7988-wdt.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---

...

> diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> new file mode 100644
> index 0000000000000..fa7c937505e08
> --- /dev/null
> +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +
> +/* TOPRGU resets */
> +#define MT7988_TOPRGU_SGMII0_GRST		1
> +#define MT7988_TOPRGU_SGMII1_GRST		2
> +#define MT7988_TOPRGU_XFI0_GRST			12
> +#define MT7988_TOPRGU_XFI1_GRST			13
> +#define MT7988_TOPRGU_XFI_PEXTP0_GRST		14
> +#define MT7988_TOPRGU_XFI_PEXTP1_GRST		15
> +#define MT7988_TOPRGU_XFI_PLL_GRST		16

IDs should start from 0 or 1 and increment by 1. If these are not IDs,
then you do not need them in the bindings.

Where is the driver change using these IDs?

> +
> +#define MT7988_TOPRGU_SW_RST_NUM		24

Why 24? I see 7. Why having it in the bindings in the first place.

It's quite likely I asked the same question about other bindings for
Mediatek. I will be asking every time till this is fixed.

Best regards,
Krzysztof

Re: [PATCH 2/2] watchdog: mediatek: mt7988: add wdt support

[AngeloGioacchino Del Regno]

Il 10/11/23 01:30, Daniel Golle ha scritto:
> Add support for watchdog and reset generator unit of the MediaTek
> MT7988 SoC.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>   drivers/watchdog/mtk_wdt.c | 56 +++++++++++++++++++++++++++++++++++++-
>   1 file changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> index b2330b16b497a..b98b8c29735aa 100644
> --- a/drivers/watchdog/mtk_wdt.c
> +++ b/drivers/watchdog/mtk_wdt.c
> @@ -12,6 +12,7 @@
>   #include <dt-bindings/reset/mt2712-resets.h>
>   #include <dt-bindings/reset/mediatek,mt6795-resets.h>
>   #include <dt-bindings/reset/mt7986-resets.h>
> +#include <dt-bindings/reset/mediatek,mt7988-resets.h>
>   #include <dt-bindings/reset/mt8183-resets.h>
>   #include <dt-bindings/reset/mt8186-resets.h>
>   #include <dt-bindings/reset/mt8188-resets.h>
> @@ -58,6 +59,8 @@
>   #define WDT_SWSYSRST		0x18U
>   #define WDT_SWSYS_RST_KEY	0x88000000
>   
> +#define WDT_SWSYSRST_EN		0xfc
> +
>   #define DRV_NAME		"mtk-wdt"
>   #define DRV_VERSION		"1.0"
>   
> @@ -71,44 +74,85 @@ struct mtk_wdt_dev {
>   	struct reset_controller_dev rcdev;
>   	bool disable_wdt_extrst;
>   	bool reset_by_toprgu;
> +	bool has_swsysrst_en;

mtk_wdt_data is always a const and this has_swsysrst_en member is never supposed
to change during runtime.

At this point, just add a pointer to struct mtk_wdt_data in mtk_wdt_dev, then
instead of mtk_wdt->has_swsysrst_en you check mtk_wdt->pdata->has_swsysrst_en.

>   };
>   
>   struct mtk_wdt_data {
>   	int toprgu_sw_rst_num;
> +	bool has_swsysrst_en;
>   };
>   
>   static const struct mtk_wdt_data mt2712_data = {
>   	.toprgu_sw_rst_num = MT2712_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = false,

false == 0; 0 is init default; this assignment is useless.

>   };
>   
>   static const struct mtk_wdt_data mt6795_data = {
>   	.toprgu_sw_rst_num = MT6795_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = false,
>   };
>   
>   static const struct mtk_wdt_data mt7986_data = {
>   	.toprgu_sw_rst_num = MT7986_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = false,
> +};
> +
> +static const struct mtk_wdt_data mt7988_data = {
> +	.toprgu_sw_rst_num = MT7988_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = true,
>   };
>   
>   static const struct mtk_wdt_data mt8183_data = {
>   	.toprgu_sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = false,
>   };
>   
>   static const struct mtk_wdt_data mt8186_data = {
>   	.toprgu_sw_rst_num = MT8186_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = false,
>   };
>   
>   static const struct mtk_wdt_data mt8188_data = {
>   	.toprgu_sw_rst_num = MT8188_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = false,
>   };
>   
>   static const struct mtk_wdt_data mt8192_data = {
>   	.toprgu_sw_rst_num = MT8192_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = false,
>   };
>   
>   static const struct mtk_wdt_data mt8195_data = {
>   	.toprgu_sw_rst_num = MT8195_TOPRGU_SW_RST_NUM,
> +	.has_swsysrst_en = false,
>   };
>   
> +static int toprgu_reset_sw_enable(struct reset_controller_dev *rcdev,
> +				  unsigned long id, bool enable)

I would transfer this logic inside of toprgu_reset_update() (not literally!).

Doing so means that you would change this function to become

/**
  * toprgu_reset_sw_en_unlocked - What this function does
  * params
  * warn about this *requiring* to be called in locked state
  * check kerneldoc documentation :)))
  */
static void toprgu_reset_sw_en_unlocked(struct mtk_wdt_dev *mtk_wdt,
					unsigned long id, bool assert)
{
	u32 tmp;

	tmp = readl....
	if (...
	blahblah

	return 0;
}

and then call it in toprgu_reset_update(), before spin_unlock_irqrestore() like

{
	stuff...
	....

	writel(.... SWSYSRST);

	if (data->pdata->has_swsysrst_en)
		toprgu_reset_sw_en_unlocked(data, id, assert);

	spin_unlock_irqrestore(...)

	return 0;
}

> +{
> +	unsigned int tmp;
> +	unsigned long flags;
> +	struct mtk_wdt_dev *data =
> +		 container_of(rcdev, struct mtk_wdt_dev, rcdev);
> +
> +	if (!data->has_swsysrst_en)
> +		return 0;
> +
> +	spin_lock_irqsave(&data->lock, flags);
> +
> +	tmp = readl(data->wdt_base + WDT_SWSYSRST_EN);
> +	if (enable)
> +		tmp |= BIT(id);
> +	else
> +		tmp &= ~BIT(id);
> +
> +	writel(tmp, data->wdt_base + WDT_SWSYSRST_EN);
> +
> +	spin_unlock_irqrestore(&data->lock, flags);
> +
> +	return 0;
> +}
> +
>   static int toprgu_reset_update(struct reset_controller_dev *rcdev,
>   			       unsigned long id, bool assert)
>   {
> @@ -135,13 +179,20 @@ static int toprgu_reset_update(struct reset_controller_dev *rcdev,
>   static int toprgu_reset_assert(struct reset_controller_dev *rcdev,
>   			       unsigned long id)
>   {
> +	int ret;
> +
> +	ret = toprgu_reset_sw_enable(rcdev, id, true);
> +	if (ret)
> +		return ret;
> +
>   	return toprgu_reset_update(rcdev, id, true);

...so you don't even touch this function...

>   }
>   
>   static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
>   				 unsigned long id)
>   {
> -	return toprgu_reset_update(rcdev, id, false);
> +	toprgu_reset_update(rcdev, id, false);
> +	return toprgu_reset_sw_enable(rcdev, id, false);

...nor that one.

>   }
>   
>   static int toprgu_reset(struct reset_controller_dev *rcdev,

Cheers!
Angelo

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[AngeloGioacchino Del Regno]

Il 10/11/23 01:30, Daniel Golle ha scritto:
> Add binding description for mediatek,mt7988-wdt.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>   .../bindings/watchdog/mediatek,mtk-wdt.yaml          |  1 +
>   include/dt-bindings/reset/mediatek,mt7988-resets.h   | 12 ++++++++++++
>   2 files changed, 13 insertions(+)
>   create mode 100644 include/dt-bindings/reset/mediatek,mt7988-resets.h
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> index cc502838bc398..8d2520241e37f 100644
> --- a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> @@ -25,6 +25,7 @@ properties:
>             - mediatek,mt6735-wdt
>             - mediatek,mt6795-wdt
>             - mediatek,mt7986-wdt
> +          - mediatek,mt7988-wdt
>             - mediatek,mt8183-wdt
>             - mediatek,mt8186-wdt
>             - mediatek,mt8188-wdt
> diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> new file mode 100644
> index 0000000000000..fa7c937505e08
> --- /dev/null
> +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +
> +/* TOPRGU resets */

The first reset is zero, the second reset is one.

Where's the zero'th reset? :-)

Regards,
Angelo

> +#define MT7988_TOPRGU_SGMII0_GRST		1
> +#define MT7988_TOPRGU_SGMII1_GRST		2
> +#define MT7988_TOPRGU_XFI0_GRST			12
> +#define MT7988_TOPRGU_XFI1_GRST			13
> +#define MT7988_TOPRGU_XFI_PEXTP0_GRST		14
> +#define MT7988_TOPRGU_XFI_PEXTP1_GRST		15
> +#define MT7988_TOPRGU_XFI_PLL_GRST		16
> +
> +#define MT7988_TOPRGU_SW_RST_NUM		24

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Daniel Golle]

On Fri, Nov 10, 2023 at 12:56:18PM +0100, AngeloGioacchino Del Regno wrote:
> Il 10/11/23 01:30, Daniel Golle ha scritto:
> > Add binding description for mediatek,mt7988-wdt.
> > 
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> >   .../bindings/watchdog/mediatek,mtk-wdt.yaml          |  1 +
> >   include/dt-bindings/reset/mediatek,mt7988-resets.h   | 12 ++++++++++++
> >   2 files changed, 13 insertions(+)
> >   create mode 100644 include/dt-bindings/reset/mediatek,mt7988-resets.h
> > 
> > diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> > index cc502838bc398..8d2520241e37f 100644
> > --- a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> > +++ b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> > @@ -25,6 +25,7 @@ properties:
> >             - mediatek,mt6735-wdt
> >             - mediatek,mt6795-wdt
> >             - mediatek,mt7986-wdt
> > +          - mediatek,mt7988-wdt
> >             - mediatek,mt8183-wdt
> >             - mediatek,mt8186-wdt
> >             - mediatek,mt8188-wdt
> > diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> > new file mode 100644
> > index 0000000000000..fa7c937505e08
> > --- /dev/null
> > +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> > @@ -0,0 +1,12 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > +
> > +/* TOPRGU resets */
> 
> The first reset is zero, the second reset is one.
> 
> Where's the zero'th reset? :-)

Currently the reset numbers represent the corresponding bit positions in
the toprgu register, as this is how the mtk-wdt driver is organized.

So there is probably something at bit 0, and also at bit 3~11 and
maybe also 17~23, but it's unknown and may be added later once known
and/or needed.

> 
> Regards,
> Angelo
> 
> > +#define MT7988_TOPRGU_SGMII0_GRST		1
> > +#define MT7988_TOPRGU_SGMII1_GRST		2
> > +#define MT7988_TOPRGU_XFI0_GRST			12
> > +#define MT7988_TOPRGU_XFI1_GRST			13
> > +#define MT7988_TOPRGU_XFI_PEXTP0_GRST		14
> > +#define MT7988_TOPRGU_XFI_PEXTP1_GRST		15
> > +#define MT7988_TOPRGU_XFI_PLL_GRST		16
> > +
> > +#define MT7988_TOPRGU_SW_RST_NUM		24
> 
> 

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Krzysztof Kozlowski]

On 10/11/2023 15:17, Daniel Golle wrote:
> On Fri, Nov 10, 2023 at 12:56:18PM +0100, AngeloGioacchino Del Regno wrote:
>> Il 10/11/23 01:30, Daniel Golle ha scritto:
>>> Add binding description for mediatek,mt7988-wdt.
>>>
>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>> ---
>>>   .../bindings/watchdog/mediatek,mtk-wdt.yaml          |  1 +
>>>   include/dt-bindings/reset/mediatek,mt7988-resets.h   | 12 ++++++++++++
>>>   2 files changed, 13 insertions(+)
>>>   create mode 100644 include/dt-bindings/reset/mediatek,mt7988-resets.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
>>> index cc502838bc398..8d2520241e37f 100644
>>> --- a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
>>> +++ b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
>>> @@ -25,6 +25,7 @@ properties:
>>>             - mediatek,mt6735-wdt
>>>             - mediatek,mt6795-wdt
>>>             - mediatek,mt7986-wdt
>>> +          - mediatek,mt7988-wdt
>>>             - mediatek,mt8183-wdt
>>>             - mediatek,mt8186-wdt
>>>             - mediatek,mt8188-wdt
>>> diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
>>> new file mode 100644
>>> index 0000000000000..fa7c937505e08
>>> --- /dev/null
>>> +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
>>> @@ -0,0 +1,12 @@
>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>> +
>>> +/* TOPRGU resets */
>>
>> The first reset is zero, the second reset is one.
>>
>> Where's the zero'th reset? :-)
> 
> Currently the reset numbers represent the corresponding bit positions in
> the toprgu register, as this is how the mtk-wdt driver is organized.
> 
> So there is probably something at bit 0, and also at bit 3~11 and
> maybe also 17~23, but it's unknown and may be added later once known
> and/or needed.

There is no need to put register bits, which are not used by the driver,
in the bindings.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Daniel Golle]

On Fri, Nov 10, 2023 at 03:20:53PM +0100, Krzysztof Kozlowski wrote:
> On 10/11/2023 15:17, Daniel Golle wrote:
> > On Fri, Nov 10, 2023 at 12:56:18PM +0100, AngeloGioacchino Del Regno wrote:
> >> Il 10/11/23 01:30, Daniel Golle ha scritto:
> >>> Add binding description for mediatek,mt7988-wdt.
> >>>
> >>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> >>> ---
> >>>   .../bindings/watchdog/mediatek,mtk-wdt.yaml          |  1 +
> >>>   include/dt-bindings/reset/mediatek,mt7988-resets.h   | 12 ++++++++++++
> >>>   2 files changed, 13 insertions(+)
> >>>   create mode 100644 include/dt-bindings/reset/mediatek,mt7988-resets.h
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> >>> index cc502838bc398..8d2520241e37f 100644
> >>> --- a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> >>> +++ b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> >>> @@ -25,6 +25,7 @@ properties:
> >>>             - mediatek,mt6735-wdt
> >>>             - mediatek,mt6795-wdt
> >>>             - mediatek,mt7986-wdt
> >>> +          - mediatek,mt7988-wdt
> >>>             - mediatek,mt8183-wdt
> >>>             - mediatek,mt8186-wdt
> >>>             - mediatek,mt8188-wdt
> >>> diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> >>> new file mode 100644
> >>> index 0000000000000..fa7c937505e08
> >>> --- /dev/null
> >>> +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> >>> @@ -0,0 +1,12 @@
> >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> >>> +
> >>> +/* TOPRGU resets */
> >>
> >> The first reset is zero, the second reset is one.
> >>
> >> Where's the zero'th reset? :-)
> > 
> > Currently the reset numbers represent the corresponding bit positions in
> > the toprgu register, as this is how the mtk-wdt driver is organized.
> > 
> > So there is probably something at bit 0, and also at bit 3~11 and
> > maybe also 17~23, but it's unknown and may be added later once known
> > and/or needed.
> 
> There is no need to put register bits, which are not used by the driver,
> in the bindings.

There aren't. That's why there isn't a zero'th reset (and also not 3~11, 17~24).

Or should the driver be reorganized to provide a mapping of logical to
physical resets, and then have only the needed once present and start
counting logical resets from 0? This is doable, of course, but it's a
bit of effort just for the aesthetical goal of starting to count from
zero and continous in header file.

And, of course, chances are that other currently still unused bits
will be needed at a later point which then would mean having to add
them in at least 2 places (header file and mapping logical<->physical)
where as currently it would just mean adding a line defining it in the
header file.

A quick looks at all the other headers in
include/dt-binding/reset/mt*-resets.h also shows that currently all of
them have unused bits and e.g. infracfg on MT7986 starts counting from
6.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Krzysztof Kozlowski]

On 10/11/2023 15:40, Daniel Golle wrote:
> On Fri, Nov 10, 2023 at 03:20:53PM +0100, Krzysztof Kozlowski wrote:
>> On 10/11/2023 15:17, Daniel Golle wrote:
>>> On Fri, Nov 10, 2023 at 12:56:18PM +0100, AngeloGioacchino Del Regno wrote:
>>>> Il 10/11/23 01:30, Daniel Golle ha scritto:
>>>>> Add binding description for mediatek,mt7988-wdt.
>>>>>
>>>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>>>> ---
>>>>>   .../bindings/watchdog/mediatek,mtk-wdt.yaml          |  1 +
>>>>>   include/dt-bindings/reset/mediatek,mt7988-resets.h   | 12 ++++++++++++
>>>>>   2 files changed, 13 insertions(+)
>>>>>   create mode 100644 include/dt-bindings/reset/mediatek,mt7988-resets.h
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
>>>>> index cc502838bc398..8d2520241e37f 100644
>>>>> --- a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
>>>>> +++ b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
>>>>> @@ -25,6 +25,7 @@ properties:
>>>>>             - mediatek,mt6735-wdt
>>>>>             - mediatek,mt6795-wdt
>>>>>             - mediatek,mt7986-wdt
>>>>> +          - mediatek,mt7988-wdt
>>>>>             - mediatek,mt8183-wdt
>>>>>             - mediatek,mt8186-wdt
>>>>>             - mediatek,mt8188-wdt
>>>>> diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
>>>>> new file mode 100644
>>>>> index 0000000000000..fa7c937505e08
>>>>> --- /dev/null
>>>>> +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
>>>>> @@ -0,0 +1,12 @@
>>>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>>>> +
>>>>> +/* TOPRGU resets */
>>>>
>>>> The first reset is zero, the second reset is one.
>>>>
>>>> Where's the zero'th reset? :-)
>>>
>>> Currently the reset numbers represent the corresponding bit positions in
>>> the toprgu register, as this is how the mtk-wdt driver is organized.
>>>
>>> So there is probably something at bit 0, and also at bit 3~11 and
>>> maybe also 17~23, but it's unknown and may be added later once known
>>> and/or needed.
>>
>> There is no need to put register bits, which are not used by the driver,
>> in the bindings.
> 
> There aren't. That's why there isn't a zero'th reset (and also not 3~11, 17~24).
> 
> Or should the driver be reorganized to provide a mapping of logical to
> physical resets, and then have only the needed once present and start
> counting logical resets from 0? This is doable, of course, but it's a
> bit of effort just for the aesthetical goal of starting to count from
> zero and continous in header file.
> 
> And, of course, chances are that other currently still unused bits
> will be needed at a later point which then would mean having to add
> them in at least 2 places (header file and mapping logical<->physical)
> where as currently it would just mean adding a line defining it in the
> header file.

You can do it, but it's not what I wrote here. So bear with me:

"There is no need to put register bits in the bindings."

You replied "There aren't", which I don't understand in this context. I
can be clearer:
Drop this hunk.

> 
> A quick looks at all the other headers in
> include/dt-binding/reset/mt*-resets.h also shows that currently all of
> them have unused bits and e.g. infracfg on MT7986 starts counting from
> 6.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Daniel Golle]

On Fri, Nov 10, 2023 at 03:46:14PM +0100, Krzysztof Kozlowski wrote:
> On 10/11/2023 15:40, Daniel Golle wrote:
> > On Fri, Nov 10, 2023 at 03:20:53PM +0100, Krzysztof Kozlowski wrote:
> >> On 10/11/2023 15:17, Daniel Golle wrote:
> >>> On Fri, Nov 10, 2023 at 12:56:18PM +0100, AngeloGioacchino Del Regno wrote:
> >>>> Il 10/11/23 01:30, Daniel Golle ha scritto:
> >>>>> Add binding description for mediatek,mt7988-wdt.
> >>>>>
> >>>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> >>>>> ---
> >>>>>   .../bindings/watchdog/mediatek,mtk-wdt.yaml          |  1 +
> >>>>>   include/dt-bindings/reset/mediatek,mt7988-resets.h   | 12 ++++++++++++
> >>>>>   2 files changed, 13 insertions(+)
> >>>>>   create mode 100644 include/dt-bindings/reset/mediatek,mt7988-resets.h
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> >>>>> index cc502838bc398..8d2520241e37f 100644
> >>>>> --- a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> >>>>> @@ -25,6 +25,7 @@ properties:
> >>>>>             - mediatek,mt6735-wdt
> >>>>>             - mediatek,mt6795-wdt
> >>>>>             - mediatek,mt7986-wdt
> >>>>> +          - mediatek,mt7988-wdt
> >>>>>             - mediatek,mt8183-wdt
> >>>>>             - mediatek,mt8186-wdt
> >>>>>             - mediatek,mt8188-wdt
> >>>>> diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> >>>>> new file mode 100644
> >>>>> index 0000000000000..fa7c937505e08
> >>>>> --- /dev/null
> >>>>> +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> >>>>> @@ -0,0 +1,12 @@
> >>>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> >>>>> +
> >>>>> +/* TOPRGU resets */
> >>>>
> >>>> The first reset is zero, the second reset is one.
> >>>>
> >>>> Where's the zero'th reset? :-)
> >>>
> >>> Currently the reset numbers represent the corresponding bit positions in
> >>> the toprgu register, as this is how the mtk-wdt driver is organized.
> >>>
> >>> So there is probably something at bit 0, and also at bit 3~11 and
> >>> maybe also 17~23, but it's unknown and may be added later once known
> >>> and/or needed.
> >>
> >> There is no need to put register bits, which are not used by the driver,
> >> in the bindings.
> > 
> > There aren't. That's why there isn't a zero'th reset (and also not 3~11, 17~24).
> > 
> > Or should the driver be reorganized to provide a mapping of logical to
> > physical resets, and then have only the needed once present and start
> > counting logical resets from 0? This is doable, of course, but it's a
> > bit of effort just for the aesthetical goal of starting to count from
> > zero and continous in header file.
> > 
> > And, of course, chances are that other currently still unused bits
> > will be needed at a later point which then would mean having to add
> > them in at least 2 places (header file and mapping logical<->physical)
> > where as currently it would just mean adding a line defining it in the
> > header file.
> 
> You can do it, but it's not what I wrote here. So bear with me:
> 
> "There is no need to put register bits in the bindings."
> 
> You replied "There aren't", which I don't understand in this context. I
> can be clearer:
> Drop this hunk.

So adding the file to include/dt-bindings/reset/ should go into a
seperate patch? Because including it with the driver itself gave me
a checkpath warning telling me that dt-bindings should go seperate,
which is why I included it with the binding docs.

> 
> > 
> > A quick looks at all the other headers in
> > include/dt-binding/reset/mt*-resets.h also shows that currently all of
> > them have unused bits and e.g. infracfg on MT7986 starts counting from
> > 6.
> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Krzysztof Kozlowski]

On 10/11/2023 15:51, Daniel Golle wrote:
> On Fri, Nov 10, 2023 at 03:46:14PM +0100, Krzysztof Kozlowski wrote:
>> On 10/11/2023 15:40, Daniel Golle wrote:
>>> On Fri, Nov 10, 2023 at 03:20:53PM +0100, Krzysztof Kozlowski wrote:
>>>> On 10/11/2023 15:17, Daniel Golle wrote:
>>>>> On Fri, Nov 10, 2023 at 12:56:18PM +0100, AngeloGioacchino Del Regno wrote:
>>>>>> Il 10/11/23 01:30, Daniel Golle ha scritto:
>>>>>>> Add binding description for mediatek,mt7988-wdt.
>>>>>>>
>>>>>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>>>>>> ---
>>>>>>>   .../bindings/watchdog/mediatek,mtk-wdt.yaml          |  1 +
>>>>>>>   include/dt-bindings/reset/mediatek,mt7988-resets.h   | 12 ++++++++++++
>>>>>>>   2 files changed, 13 insertions(+)
>>>>>>>   create mode 100644 include/dt-bindings/reset/mediatek,mt7988-resets.h
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
>>>>>>> index cc502838bc398..8d2520241e37f 100644
>>>>>>> --- a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
>>>>>>> @@ -25,6 +25,7 @@ properties:
>>>>>>>             - mediatek,mt6735-wdt
>>>>>>>             - mediatek,mt6795-wdt
>>>>>>>             - mediatek,mt7986-wdt
>>>>>>> +          - mediatek,mt7988-wdt
>>>>>>>             - mediatek,mt8183-wdt
>>>>>>>             - mediatek,mt8186-wdt
>>>>>>>             - mediatek,mt8188-wdt
>>>>>>> diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
>>>>>>> new file mode 100644
>>>>>>> index 0000000000000..fa7c937505e08
>>>>>>> --- /dev/null
>>>>>>> +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
>>>>>>> @@ -0,0 +1,12 @@
>>>>>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>>>>>> +
>>>>>>> +/* TOPRGU resets */
>>>>>>
>>>>>> The first reset is zero, the second reset is one.
>>>>>>
>>>>>> Where's the zero'th reset? :-)
>>>>>
>>>>> Currently the reset numbers represent the corresponding bit positions in
>>>>> the toprgu register, as this is how the mtk-wdt driver is organized.
>>>>>
>>>>> So there is probably something at bit 0, and also at bit 3~11 and
>>>>> maybe also 17~23, but it's unknown and may be added later once known
>>>>> and/or needed.
>>>>
>>>> There is no need to put register bits, which are not used by the driver,
>>>> in the bindings.
>>>
>>> There aren't. That's why there isn't a zero'th reset (and also not 3~11, 17~24).
>>>
>>> Or should the driver be reorganized to provide a mapping of logical to
>>> physical resets, and then have only the needed once present and start
>>> counting logical resets from 0? This is doable, of course, but it's a
>>> bit of effort just for the aesthetical goal of starting to count from
>>> zero and continous in header file.
>>>
>>> And, of course, chances are that other currently still unused bits
>>> will be needed at a later point which then would mean having to add
>>> them in at least 2 places (header file and mapping logical<->physical)
>>> where as currently it would just mean adding a line defining it in the
>>> header file.
>>
>> You can do it, but it's not what I wrote here. So bear with me:
>>
>> "There is no need to put register bits in the bindings."

No comments here, so I assume you agree with this.

>>
>> You replied "There aren't", which I don't understand in this context. I
>> can be clearer:
>> Drop this hunk.
> 
> So adding the file to include/dt-bindings/reset/ should go into a
> seperate patch? Because including it with the driver itself gave me
> a checkpath warning telling me that dt-bindings should go seperate,
> which is why I included it with the binding docs.

No, I said the hunk should be dropped. Removed.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Daniel Golle]

On Fri, Nov 10, 2023 at 04:07:51PM +0100, Krzysztof Kozlowski wrote:
> On 10/11/2023 15:51, Daniel Golle wrote:
> > On Fri, Nov 10, 2023 at 03:46:14PM +0100, Krzysztof Kozlowski wrote:
> >> On 10/11/2023 15:40, Daniel Golle wrote:
> >>> On Fri, Nov 10, 2023 at 03:20:53PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 10/11/2023 15:17, Daniel Golle wrote:
> >>>>> On Fri, Nov 10, 2023 at 12:56:18PM +0100, AngeloGioacchino Del Regno wrote:
> >>>>>> Il 10/11/23 01:30, Daniel Golle ha scritto:
> >>>>>>> Add binding description for mediatek,mt7988-wdt.
> >>>>>>>
> >>>>>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> >>>>>>> ---
> >>>>>>>   .../bindings/watchdog/mediatek,mtk-wdt.yaml          |  1 +
> >>>>>>>   include/dt-bindings/reset/mediatek,mt7988-resets.h   | 12 ++++++++++++
> >>>>>>>   2 files changed, 13 insertions(+)
> >>>>>>>   create mode 100644 include/dt-bindings/reset/mediatek,mt7988-resets.h
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> >>>>>>> index cc502838bc398..8d2520241e37f 100644
> >>>>>>> --- a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> >>>>>>> +++ b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
> >>>>>>> @@ -25,6 +25,7 @@ properties:
> >>>>>>>             - mediatek,mt6735-wdt
> >>>>>>>             - mediatek,mt6795-wdt
> >>>>>>>             - mediatek,mt7986-wdt
> >>>>>>> +          - mediatek,mt7988-wdt
> >>>>>>>             - mediatek,mt8183-wdt
> >>>>>>>             - mediatek,mt8186-wdt
> >>>>>>>             - mediatek,mt8188-wdt
> >>>>>>> diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> >>>>>>> new file mode 100644
> >>>>>>> index 0000000000000..fa7c937505e08
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> >>>>>>> @@ -0,0 +1,12 @@
> >>>>>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> >>>>>>> +
> >>>>>>> +/* TOPRGU resets */
> >>>>>>
> >>>>>> The first reset is zero, the second reset is one.
> >>>>>>
> >>>>>> Where's the zero'th reset? :-)
> >>>>>
> >>>>> Currently the reset numbers represent the corresponding bit positions in
> >>>>> the toprgu register, as this is how the mtk-wdt driver is organized.
> >>>>>
> >>>>> So there is probably something at bit 0, and also at bit 3~11 and
> >>>>> maybe also 17~23, but it's unknown and may be added later once known
> >>>>> and/or needed.
> >>>>
> >>>> There is no need to put register bits, which are not used by the driver,
> >>>> in the bindings.
> >>>
> >>> There aren't. That's why there isn't a zero'th reset (and also not 3~11, 17~24).
> >>>
> >>> Or should the driver be reorganized to provide a mapping of logical to
> >>> physical resets, and then have only the needed once present and start
> >>> counting logical resets from 0? This is doable, of course, but it's a
> >>> bit of effort just for the aesthetical goal of starting to count from
> >>> zero and continous in header file.
> >>>
> >>> And, of course, chances are that other currently still unused bits
> >>> will be needed at a later point which then would mean having to add
> >>> them in at least 2 places (header file and mapping logical<->physical)
> >>> where as currently it would just mean adding a line defining it in the
> >>> header file.
> >>
> >> You can do it, but it's not what I wrote here. So bear with me:
> >>
> >> "There is no need to put register bits in the bindings."
> 
> No comments here, so I assume you agree with this.
> 
> >>
> >> You replied "There aren't", which I don't understand in this context. I
> >> can be clearer:
> >> Drop this hunk.
> > 
> > So adding the file to include/dt-bindings/reset/ should go into a
> > seperate patch? Because including it with the driver itself gave me
> > a checkpath warning telling me that dt-bindings should go seperate,
> > which is why I included it with the binding docs.
> 
> No, I said the hunk should be dropped. Removed.

I guess we are somehow misunderstanding each other.
Lets go with an example. I can put the header into a commit of its own,
just like commit
5794dda109fc8 dt-bindings: reset: mt7986: Add reset-controller header file
https://lore.kernel.org/r/20220105100456.7126-2-sam.shih@mediatek.com

Would that be acceptable? And if not, why?

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/2] watchdog: mediatek: mt7988: add wdt support

[Guenter Roeck]

On 11/10/23 03:55, AngeloGioacchino Del Regno wrote:
> Il 10/11/23 01:30, Daniel Golle ha scritto:
>> Add support for watchdog and reset generator unit of the MediaTek
>> MT7988 SoC.
>>
>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>> ---
>>   drivers/watchdog/mtk_wdt.c | 56 +++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
>> index b2330b16b497a..b98b8c29735aa 100644
>> --- a/drivers/watchdog/mtk_wdt.c
>> +++ b/drivers/watchdog/mtk_wdt.c
>> @@ -12,6 +12,7 @@
>>   #include <dt-bindings/reset/mt2712-resets.h>
>>   #include <dt-bindings/reset/mediatek,mt6795-resets.h>
>>   #include <dt-bindings/reset/mt7986-resets.h>
>> +#include <dt-bindings/reset/mediatek,mt7988-resets.h>
>>   #include <dt-bindings/reset/mt8183-resets.h>
>>   #include <dt-bindings/reset/mt8186-resets.h>
>>   #include <dt-bindings/reset/mt8188-resets.h>
>> @@ -58,6 +59,8 @@
>>   #define WDT_SWSYSRST        0x18U
>>   #define WDT_SWSYS_RST_KEY    0x88000000
>> +#define WDT_SWSYSRST_EN        0xfc
>> +
>>   #define DRV_NAME        "mtk-wdt"
>>   #define DRV_VERSION        "1.0"
>> @@ -71,44 +74,85 @@ struct mtk_wdt_dev {
>>       struct reset_controller_dev rcdev;
>>       bool disable_wdt_extrst;
>>       bool reset_by_toprgu;
>> +    bool has_swsysrst_en;
> 
> mtk_wdt_data is always a const and this has_swsysrst_en member is never supposed
> to change during runtime.
> 
> At this point, just add a pointer to struct mtk_wdt_data in mtk_wdt_dev, then
> instead of mtk_wdt->has_swsysrst_en you check mtk_wdt->pdata->has_swsysrst_en.
> 

Oh, I don't know. Personally I dislike having to execute double dereferences
at runtime. I tend to accept it because people seem to like it, but I really
don't see the point.

Guenter

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Krzysztof Kozlowski]

On 10/11/2023 16:12, Daniel Golle wrote:
>>>>>>>>> diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
>>>>>>>>> new file mode 100644
>>>>>>>>> index 0000000000000..fa7c937505e08
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
>>>>>>>>> @@ -0,0 +1,12 @@
>>>>>>>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>>>>>>>> +
>>>>>>>>> +/* TOPRGU resets */
>>>>>>>>
>>>>>>>> The first reset is zero, the second reset is one.
>>>>>>>>
>>>>>>>> Where's the zero'th reset? :-)
>>>>>>>
>>>>>>> Currently the reset numbers represent the corresponding bit positions in
>>>>>>> the toprgu register, as this is how the mtk-wdt driver is organized.
>>>>>>>
>>>>>>> So there is probably something at bit 0, and also at bit 3~11 and
>>>>>>> maybe also 17~23, but it's unknown and may be added later once known
>>>>>>> and/or needed.
>>>>>>
>>>>>> There is no need to put register bits, which are not used by the driver,
>>>>>> in the bindings.
>>>>>
>>>>> There aren't. That's why there isn't a zero'th reset (and also not 3~11, 17~24).
>>>>>
>>>>> Or should the driver be reorganized to provide a mapping of logical to
>>>>> physical resets, and then have only the needed once present and start
>>>>> counting logical resets from 0? This is doable, of course, but it's a
>>>>> bit of effort just for the aesthetical goal of starting to count from
>>>>> zero and continous in header file.
>>>>>
>>>>> And, of course, chances are that other currently still unused bits
>>>>> will be needed at a later point which then would mean having to add
>>>>> them in at least 2 places (header file and mapping logical<->physical)
>>>>> where as currently it would just mean adding a line defining it in the
>>>>> header file.
>>>>
>>>> You can do it, but it's not what I wrote here. So bear with me:
>>>>
>>>> "There is no need to put register bits in the bindings."
>>
>> No comments here, so I assume you agree with this.

Here is the answer to...

>>
>>>>
>>>> You replied "There aren't", which I don't understand in this context. I
>>>> can be clearer:
>>>> Drop this hunk.
>>>
>>> So adding the file to include/dt-bindings/reset/ should go into a
>>> seperate patch? Because including it with the driver itself gave me
>>> a checkpath warning telling me that dt-bindings should go seperate,
>>> which is why I included it with the binding docs.
>>
>> No, I said the hunk should be dropped. Removed.
> 
> I guess we are somehow misunderstanding each other.
> Lets go with an example. I can put the header into a commit of its own,
> just like commit
> 5794dda109fc8 dt-bindings: reset: mt7986: Add reset-controller header file
> https://lore.kernel.org/r/20220105100456.7126-2-sam.shih@mediatek.com
> 
> Would that be acceptable? And if not, why?

...this question.

Again, whether this is separate patch - it is still hunk which I think
should be removed. I gave the reason "why" in this mail thread and in
multiple other discussions.



Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Krzysztof Kozlowski]

On 10/11/2023 09:09, Krzysztof Kozlowski wrote:
> On 10/11/2023 01:30, Daniel Golle wrote:
>> Add binding description for mediatek,mt7988-wdt.
>>
>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>> ---
> 
> ...
> 
>> diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
>> new file mode 100644
>> index 0000000000000..fa7c937505e08
>> --- /dev/null
>> +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
>> @@ -0,0 +1,12 @@
>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>> +
>> +/* TOPRGU resets */
>> +#define MT7988_TOPRGU_SGMII0_GRST		1
>> +#define MT7988_TOPRGU_SGMII1_GRST		2
>> +#define MT7988_TOPRGU_XFI0_GRST			12
>> +#define MT7988_TOPRGU_XFI1_GRST			13
>> +#define MT7988_TOPRGU_XFI_PEXTP0_GRST		14
>> +#define MT7988_TOPRGU_XFI_PEXTP1_GRST		15
>> +#define MT7988_TOPRGU_XFI_PLL_GRST		16
> 
> IDs should start from 0 or 1 and increment by 1. If these are not IDs,
> then you do not need them in the bindings.
> 
> Where is the driver change using these IDs?

You nicely skipped my email and keep pushing the idea of putting this
into separate patch.

No. Respond to received comments.

> 
>> +
>> +#define MT7988_TOPRGU_SW_RST_NUM		24
> 
> Why 24? I see 7. Why having it in the bindings in the first place.
> 
> It's quite likely I asked the same question about other bindings for
> Mediatek. I will be asking every time till this is fixed.

No response to this, either.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Krzysztof Kozlowski]

On 10/11/2023 16:15, Krzysztof Kozlowski wrote:
>>>> So adding the file to include/dt-bindings/reset/ should go into a
>>>> seperate patch? Because including it with the driver itself gave me
>>>> a checkpath warning telling me that dt-bindings should go seperate,
>>>> which is why I included it with the binding docs.
>>>
>>> No, I said the hunk should be dropped. Removed.
>>
>> I guess we are somehow misunderstanding each other.
>> Lets go with an example. I can put the header into a commit of its own,
>> just like commit
>> 5794dda109fc8 dt-bindings: reset: mt7986: Add reset-controller header file
>> https://lore.kernel.org/r/20220105100456.7126-2-sam.shih@mediatek.com
>>
>> Would that be acceptable? And if not, why?
> 
> ...this question.
> 
> Again, whether this is separate patch - it is still hunk which I think
> should be removed. I gave the reason "why" in this mail thread and in
> multiple other discussions.

I gave you clear reasoning 7 hours ago:
https://lore.kernel.org/all/59629ec1-cc0c-4c5a-87cc-ea30d64ec191@linaro.org/
to which you did not respond.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Daniel Golle]

On Fri, Nov 10, 2023 at 04:21:35PM +0100, Krzysztof Kozlowski wrote:
> On 10/11/2023 16:15, Krzysztof Kozlowski wrote:
> >>>> So adding the file to include/dt-bindings/reset/ should go into a
> >>>> seperate patch? Because including it with the driver itself gave me
> >>>> a checkpath warning telling me that dt-bindings should go seperate,
> >>>> which is why I included it with the binding docs.
> >>>
> >>> No, I said the hunk should be dropped. Removed.
> >>
> >> I guess we are somehow misunderstanding each other.
> >> Lets go with an example. I can put the header into a commit of its own,
> >> just like commit
> >> 5794dda109fc8 dt-bindings: reset: mt7986: Add reset-controller header file
> >> https://lore.kernel.org/r/20220105100456.7126-2-sam.shih@mediatek.com
> >>
> >> Would that be acceptable? And if not, why?
> > 
> > ...this question.

... which you didn't answer. Sorry, but it's not helpful to be polemic
or ironic in a code review involving non-native English speakers
trying to understand each others.

> > 
> > Again, whether this is separate patch - it is still hunk which I think
> > should be removed. I gave the reason "why" in this mail thread and in
> > multiple other discussions.
> 
> I gave you clear reasoning 7 hours ago:
> https://lore.kernel.org/all/59629ec1-cc0c-4c5a-87cc-ea30d64ec191@linaro.org/
> to which you did not respond.

Because it doesn't match anything existing regarding MediaTek reset
drivers, and I was assuming there must be some kind of misunderstanding,
which is why I replied to your later email in the same thread.

My assumption that the problem was merely having documentation and
header combined in a single commit stems from the fact that a very
similar patch for MT7986[1] was Ack'ed by Rob Herring about a year and
a half ago; hence the rule you apply here may have always existed, but
apparently then hasn't been applied in the past.

Literally *all* existing dt-binding headers for MediaTek SoCs follow a
direct 1:1 mapping of reset bit in hardware and reset number in the
header files. The driver is simple, all it cares about is the maximum
number defined in the header (and I like that, because it makes it very
easy to add new SoCs). At this point the abstraction needed to
fulfill your request doesn't exist, not for any of the SoCs using
mtk_wdt.c. It can be implemented, surely, it's a problem computers can
solve. If that's what you (and current maintainers of that driver)
would want me to implement, please say so clearly and spell it out.

Also be clear about if all the other existing headers need to be
converted, mappings for all SoCs created in the driver, ... all before
support for MT7988 can go in?
Or should the existing headers for other MediaTek SoCs remain
untouched because they are already considered stable API or something?


Thank you for your patiente!


Daniel


[1]: https://lore.kernel.org/all/Yd4uplioThv8eJJE@robh.at.kernel.org/

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Krzysztof Kozlowski]

On 10/11/2023 18:07, Daniel Golle wrote:
> On Fri, Nov 10, 2023 at 04:21:35PM +0100, Krzysztof Kozlowski wrote:
>> On 10/11/2023 16:15, Krzysztof Kozlowski wrote:
>>>>>> So adding the file to include/dt-bindings/reset/ should go into a
>>>>>> seperate patch? Because including it with the driver itself gave me
>>>>>> a checkpath warning telling me that dt-bindings should go seperate,
>>>>>> which is why I included it with the binding docs.
>>>>>
>>>>> No, I said the hunk should be dropped. Removed.
>>>>
>>>> I guess we are somehow misunderstanding each other.
>>>> Lets go with an example. I can put the header into a commit of its own,
>>>> just like commit
>>>> 5794dda109fc8 dt-bindings: reset: mt7986: Add reset-controller header file
>>>> https://lore.kernel.org/r/20220105100456.7126-2-sam.shih@mediatek.com
>>>>
>>>> Would that be acceptable? And if not, why?
>>>
>>> ...this question.
> 
> ... which you didn't answer. Sorry, but it's not helpful to be polemic
> or ironic in a code review involving non-native English speakers
> trying to understand each others.

Why do you keep skipping - third time - my earlier reply? The earlier
paragraph?

Cutting lines out of context is not how this should be discussed.

So let me bring it:

>> FOO
> Here is the answer to...

>> BAR
> ...this question.

The "FOO" Is the answer. Go open the emails and read the answer.



> 
>>>
>>> Again, whether this is separate patch - it is still hunk which I think
>>> should be removed. I gave the reason "why" in this mail thread and in
>>> multiple other discussions.
>>
>> I gave you clear reasoning 7 hours ago:
>> https://lore.kernel.org/all/59629ec1-cc0c-4c5a-87cc-ea30d64ec191@linaro.org/
>> to which you did not respond.
> 
> Because it doesn't match anything existing regarding MediaTek reset
> drivers, and I was assuming there must be some kind of misunderstanding,
> which is why I replied to your later email in the same thread.
> 
> My assumption that the problem was merely having documentation and
> header combined in a single commit stems from the fact that a very
> similar patch for MT7986[1] was Ack'ed by Rob Herring about a year and
> a half ago; hence the rule you apply here may have always existed, but
> apparently then hasn't been applied in the past.
> 
> Literally *all* existing dt-binding headers for MediaTek SoCs follow a
> direct 1:1 mapping of reset bit in hardware and reset number in the
> header files. The driver is simple, all it cares about is the maximum
> number defined in the header (and I like that, because it makes it very
> easy to add new SoCs). At this point the abstraction needed to
> fulfill your request doesn't exist, not for any of the SoCs using
> mtk_wdt.c. It can be implemented, surely, it's a problem computers can
> solve. If that's what you (and current maintainers of that driver)
> would want me to implement, please say so clearly and spell it out.
> 
> Also be clear about if all the other existing headers need to be
> converted, mappings for all SoCs created in the driver, ... all before
> support for MT7988 can go in?
> Or should the existing headers for other MediaTek SoCs remain
> untouched because they are already considered stable API or something?

I am repeating myself... but I don't know how to put it other way. I did
not ask you to rewrite your driver. I asked to drop the change to
bindings, because it is entirely pointless.

Drop this change, only this. No need to rewrite drivers, they stay the same.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Krzysztof Kozlowski]

On 10/11/2023 16:20, Krzysztof Kozlowski wrote:
> On 10/11/2023 09:09, Krzysztof Kozlowski wrote:
>> On 10/11/2023 01:30, Daniel Golle wrote:
>>> Add binding description for mediatek,mt7988-wdt.
>>>
>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>> ---
>>
>> ...
>>
>>> diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
>>> new file mode 100644
>>> index 0000000000000..fa7c937505e08
>>> --- /dev/null
>>> +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
>>> @@ -0,0 +1,12 @@
>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>> +
>>> +/* TOPRGU resets */
>>> +#define MT7988_TOPRGU_SGMII0_GRST		1
>>> +#define MT7988_TOPRGU_SGMII1_GRST		2
>>> +#define MT7988_TOPRGU_XFI0_GRST			12
>>> +#define MT7988_TOPRGU_XFI1_GRST			13
>>> +#define MT7988_TOPRGU_XFI_PEXTP0_GRST		14
>>> +#define MT7988_TOPRGU_XFI_PEXTP1_GRST		15
>>> +#define MT7988_TOPRGU_XFI_PLL_GRST		16
>>
>> IDs should start from 0 or 1 and increment by 1. If these are not IDs,
>> then you do not need them in the bindings.
>>
>> Where is the driver change using these IDs?
> 
> You nicely skipped my email and keep pushing the idea of putting this
> into separate patch.
> 
> No. Respond to received comments.
> 
>>
>>> +
>>> +#define MT7988_TOPRGU_SW_RST_NUM		24
>>
>> Why 24? I see 7. Why having it in the bindings in the first place.
>>
>> It's quite likely I asked the same question about other bindings for
>> Mediatek. I will be asking every time till this is fixed.
> 
> No response to this, either.

You still did not respond here. To none of the points here. It's my
third ping because I want this to be resolved. But ignoring my emails,
and skipping paragraphs of my replies will not lead anywhere.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Daniel Golle]

On Fri, Nov 10, 2023 at 08:58:57PM +0100, Krzysztof Kozlowski wrote:
> I am repeating myself... but I don't know how to put it other way. I did
> not ask you to rewrite your driver. I asked to drop the change to
> bindings, because it is entirely pointless.
> 
> Drop this change, only this. No need to rewrite drivers, they stay the same.

Dropping this change (I'm assuming you are referring to the hunk adding
include/dt-bindings/reset/mt7988-resets.h) and also not adding that
header file using a seperate commit means that there won't be a header
defining the reset names.

The result would be having to numerically reference the specific
resets in the device tree.

This is, of course, possible, but I don't understand what the advantage
would be.

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Krzysztof Kozlowski]

On 10/11/2023 21:18, Daniel Golle wrote:
> On Fri, Nov 10, 2023 at 08:58:57PM +0100, Krzysztof Kozlowski wrote:
>> I am repeating myself... but I don't know how to put it other way. I did
>> not ask you to rewrite your driver. I asked to drop the change to
>> bindings, because it is entirely pointless.
>>
>> Drop this change, only this. No need to rewrite drivers, they stay the same.
> 
> Dropping this change (I'm assuming you are referring to the hunk adding
> include/dt-bindings/reset/mt7988-resets.h) and also not adding that
> header file using a seperate commit means that there won't be a header
> defining the reset names.
> 
> The result would be having to numerically reference the specific
> resets in the device tree.

Which is a problem because? Do you see bindings for IO space? For
interrupts? For hundreds of other values? No, because the value is not a
binding.

DT binding binds the driver and DTS. If you have nothing in the driver,
there is no binding.

> 
> This is, of course, possible, but I don't understand what the advantage
> would be.

I pinged you three times on email I expect answer and there is nothing.
You ignored the thread completely.

I am finishing with this thread till you start answering emails.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Daniel Golle]

On Fri, Nov 10, 2023 at 09:00:26PM +0100, Krzysztof Kozlowski wrote:
> On 10/11/2023 16:20, Krzysztof Kozlowski wrote:
> > On 10/11/2023 09:09, Krzysztof Kozlowski wrote:
> >> On 10/11/2023 01:30, Daniel Golle wrote:
> >>> Add binding description for mediatek,mt7988-wdt.
> >>>
> >>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> >>> ---
> >>
> >> ...
> >>
> >>> diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> >>> new file mode 100644
> >>> index 0000000000000..fa7c937505e08
> >>> --- /dev/null
> >>> +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> >>> @@ -0,0 +1,12 @@
> >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> >>> +
> >>> +/* TOPRGU resets */
> >>> +#define MT7988_TOPRGU_SGMII0_GRST		1
> >>> +#define MT7988_TOPRGU_SGMII1_GRST		2
> >>> +#define MT7988_TOPRGU_XFI0_GRST			12
> >>> +#define MT7988_TOPRGU_XFI1_GRST			13
> >>> +#define MT7988_TOPRGU_XFI_PEXTP0_GRST		14
> >>> +#define MT7988_TOPRGU_XFI_PEXTP1_GRST		15
> >>> +#define MT7988_TOPRGU_XFI_PLL_GRST		16
> >>
> >> IDs should start from 0 or 1 and increment by 1. If these are not IDs,
> >> then you do not need them in the bindings.
> >>
> >> Where is the driver change using these IDs?

It isn't needed as the driver doesn't list the IDs. If that would
be true, it would be sufficient to put them into a header next to the
driver or defined inside the driver C file.

The defined IDs here are intended to be used in device tree files.

> > 
> > You nicely skipped my email and keep pushing the idea of putting this
> > into separate patch.
> > 
> > No. Respond to received comments.
> > 
> >>
> >>> +
> >>> +#define MT7988_TOPRGU_SW_RST_NUM		24
> >>
> >> Why 24? I see 7. 

Because the wdt on MT7988 has a total of 24 resets. Most of them are
(currently, as there are no GPL drops, no publicly available devices,
...) undocumented and are not used in Linux **at this point**. Having
to change the driver every time a new reset is discovered or needed to
be used is tideous, so I thought the best would be -- as we know the
total number of resets -- to already define that, as it's safe to do
and won't need to change.

> >> Why having it in the bindings in the first place.

This line can indeed go into the driver, it's not used anywhere else.
I was merely immitating the style of all the existing binding headers
for similar SoCs and didn't want to stick-out style-wise, also in terms
of the added code to the driver which relies on that number being
defined in the header for all other SoCs.

> >>
> >> It's quite likely I asked the same question about other bindings for
> >> Mediatek. I will be asking every time till this is fixed.
> > 
> > No response to this, either.
> 
> You still did not respond here. To none of the points here. It's my
> third ping because I want this to be resolved. But ignoring my emails,
> and skipping paragraphs of my replies will not lead anywhere.

I have answered to this before:
The driver does NOT have any internal list of names of individual
resets, it relies on the reset number from device tree matching the bit
in the controller, just like for any other MediaTek toprgu already
supported by mtk-wdt.c.

Re: [PATCH 1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu

[Krzysztof Kozlowski]

On 10/11/2023 21:45, Daniel Golle wrote:
> On Fri, Nov 10, 2023 at 09:00:26PM +0100, Krzysztof Kozlowski wrote:
>> On 10/11/2023 16:20, Krzysztof Kozlowski wrote:
>>> On 10/11/2023 09:09, Krzysztof Kozlowski wrote:
>>>> On 10/11/2023 01:30, Daniel Golle wrote:
>>>>> Add binding description for mediatek,mt7988-wdt.
>>>>>
>>>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>>>> ---
>>>>
>>>> ...
>>>>
>>>>> diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
>>>>> new file mode 100644
>>>>> index 0000000000000..fa7c937505e08
>>>>> --- /dev/null
>>>>> +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
>>>>> @@ -0,0 +1,12 @@
>>>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>>>> +
>>>>> +/* TOPRGU resets */
>>>>> +#define MT7988_TOPRGU_SGMII0_GRST		1
>>>>> +#define MT7988_TOPRGU_SGMII1_GRST		2
>>>>> +#define MT7988_TOPRGU_XFI0_GRST			12
>>>>> +#define MT7988_TOPRGU_XFI1_GRST			13
>>>>> +#define MT7988_TOPRGU_XFI_PEXTP0_GRST		14
>>>>> +#define MT7988_TOPRGU_XFI_PEXTP1_GRST		15
>>>>> +#define MT7988_TOPRGU_XFI_PLL_GRST		16
>>>>
>>>> IDs should start from 0 or 1 and increment by 1. If these are not IDs,
>>>> then you do not need them in the bindings.
>>>>
>>>> Where is the driver change using these IDs?
> 
> It isn't needed as the driver doesn't list the IDs. If that would

Then it is no a binding.

> be true, it would be sufficient to put them into a header next to the
> driver or defined inside the driver C file.

Not related. Binding header is used by both driver and DTS.

> 
> The defined IDs here are intended to be used in device tree files.

Then not a binding.

> 
>>>
>>> You nicely skipped my email and keep pushing the idea of putting this
>>> into separate patch.
>>>
>>> No. Respond to received comments.
>>>
>>>>
>>>>> +
>>>>> +#define MT7988_TOPRGU_SW_RST_NUM		24
>>>>
>>>> Why 24? I see 7. 
> 
> Because the wdt on MT7988 has a total of 24 resets. Most of them are
> (currently, as there are no GPL drops, no publicly available devices,
> ...) undocumented and are not used in Linux **at this point**. Having
> to change the driver every time a new reset is discovered or needed to

There is no need to change the driver. Once it is set in the binding, to
let's say 7, it must stay like this. Since this is not representing real
binding resets (there are 7, not 24) and it is no used in DTS: this is
not a binding.


> be used is tideous, so I thought the best would be -- as we know the
> total number of resets -- to already define that, as it's safe to do
> and won't need to change.


> 
>>>> Why having it in the bindings in the first place.
> 
> This line can indeed go into the driver, it's not used anywhere else.
> I was merely immitating the style of all the existing binding headers
> for similar SoCs and didn't want to stick-out style-wise, also in terms
> of the added code to the driver which relies on that number being
> defined in the header for all other SoCs.
> 
>>>>
>>>> It's quite likely I asked the same question about other bindings for
>>>> Mediatek. I will be asking every time till this is fixed.
>>>
>>> No response to this, either.
>>
>> You still did not respond here. To none of the points here. It's my
>> third ping because I want this to be resolved. But ignoring my emails,
>> and skipping paragraphs of my replies will not lead anywhere.
> 
> I have answered to this before:
> The driver does NOT have any internal list of names of individual
> resets, it relies on the reset number from device tree matching the bit
> in the controller, just like for any other MediaTek toprgu already
> supported by mtk-wdt.c.

Sure, and this is not a binding. Please do not make binding things which
are not bindings, because later you (you as in plural) come and request
to change it, which must not be allowed. But because people stuff
not-binding-things into the binding they use it later as arguments that
it is allowed to change.

As I wrote before, I complained about this already several times and I
will be complaining every time.

Best regards,
Krzysztof