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

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

[Daniel Golle]

Add compatible mediatek,mt7988-wdt.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
v3: no changes
v2: Drop adding include/dt-binding/mt7988-resets.h as that header is not
    actually a binding header.


 Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml | 1 +
 1 file changed, 1 insertion(+)

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
-- 
2.42.1

[PATCH v3 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>
---
v3: fix wrong function parameter name in kernel-doc comment
v2: call new toprgu_reset_sw_en_unlocked from toprgu_reset_update while
    holding lock.

 drivers/watchdog/mtk_wdt.c | 40 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
index b2330b16b497a..3b4ee7185feed 100644
--- a/drivers/watchdog/mtk_wdt.c
+++ b/drivers/watchdog/mtk_wdt.c
@@ -58,6 +58,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,10 +73,12 @@ 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 = {
@@ -89,6 +93,11 @@ static const struct mtk_wdt_data mt7986_data = {
 	.toprgu_sw_rst_num = MT7986_TOPRGU_SW_RST_NUM,
 };
 
+static const struct mtk_wdt_data mt7988_data = {
+	.toprgu_sw_rst_num = 24,
+	.has_swsysrst_en = true,
+};
+
 static const struct mtk_wdt_data mt8183_data = {
 	.toprgu_sw_rst_num = MT8183_TOPRGU_SW_RST_NUM,
 };
@@ -109,6 +118,28 @@ static const struct mtk_wdt_data mt8195_data = {
 	.toprgu_sw_rst_num = MT8195_TOPRGU_SW_RST_NUM,
 };
 
+/**
+ * toprgu_reset_sw_en_unlocked() - enable/disable software control for reset bit
+ * @data: Pointer to instance of driver data.
+ * @id: Bit number identifying the reset to be enabled or disabled.
+ * @enable: If true, enable software control for that bit, disable otherwise.
+ *
+ * Context: The caller must hold lock of struct mtk_wdt_dev.
+ */
+static void toprgu_reset_sw_en_unlocked(struct mtk_wdt_dev *data,
+					unsigned long id, bool enable)
+{
+	u32 tmp;
+
+	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);
+}
+
 static int toprgu_reset_update(struct reset_controller_dev *rcdev,
 			       unsigned long id, bool assert)
 {
@@ -119,6 +150,9 @@ static int toprgu_reset_update(struct reset_controller_dev *rcdev,
 
 	spin_lock_irqsave(&data->lock, flags);
 
+	if (assert && data->has_swsysrst_en)
+		toprgu_reset_sw_en_unlocked(data, id, true);
+
 	tmp = readl(data->wdt_base + WDT_SWSYSRST);
 	if (assert)
 		tmp |= BIT(id);
@@ -127,6 +161,9 @@ static int toprgu_reset_update(struct reset_controller_dev *rcdev,
 	tmp |= WDT_SWSYS_RST_KEY;
 	writel(tmp, data->wdt_base + WDT_SWSYSRST);
 
+	if (!assert && data->has_swsysrst_en)
+		toprgu_reset_sw_en_unlocked(data, id, false);
+
 	spin_unlock_irqrestore(&data->lock, flags);
 
 	return 0;
@@ -406,6 +443,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 +483,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 v3 2/2] watchdog: mediatek: mt7988: add wdt support

[Guenter Roeck]

On 11/14/23 09:04, Daniel Golle wrote:
> Add support for watchdog and reset generator unit of the MediaTek
> MT7988 SoC.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
> v3: fix wrong function parameter name in kernel-doc comment
> v2: call new toprgu_reset_sw_en_unlocked from toprgu_reset_update while
>      holding lock.
> 
>   drivers/watchdog/mtk_wdt.c | 40 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> index b2330b16b497a..3b4ee7185feed 100644
> --- a/drivers/watchdog/mtk_wdt.c
> +++ b/drivers/watchdog/mtk_wdt.c
> @@ -58,6 +58,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,10 +73,12 @@ 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 = {
> @@ -89,6 +93,11 @@ static const struct mtk_wdt_data mt7986_data = {
>   	.toprgu_sw_rst_num = MT7986_TOPRGU_SW_RST_NUM,
>   };
>   
> +static const struct mtk_wdt_data mt7988_data = {
> +	.toprgu_sw_rst_num = 24,

Kind of odd to have this defined locally, while the others are in include files,
but not worth arguing about. Please make it a define at the top of the file,
though.

Thanks,
Guenter

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

[Daniel Golle]

On Mon, Nov 20, 2023 at 09:19:46AM -0800, Guenter Roeck wrote:
> On 11/14/23 09:04, Daniel Golle wrote:
> > [...]
> > @@ -89,6 +93,11 @@ static const struct mtk_wdt_data mt7986_data = {
> >   	.toprgu_sw_rst_num = MT7986_TOPRGU_SW_RST_NUM,
> >   };
> > +static const struct mtk_wdt_data mt7988_data = {
> > +	.toprgu_sw_rst_num = 24,
> 
> Kind of odd to have this defined locally, while the others are in include files,
> but not worth arguing about.

From I have just learned from Krzysztof Kozlowski those headers shouldn't
even exist in first place, as the listed IDs are not actually referenced
anywhere in the driver, hence they aren't actually bindings [1].

Quote from that thread:
| >>> Where is the driver change using these IDs?
| >> It isn't needed as the driver doesn't list the IDs. [...]
| > Then it is not a binding.
---

Now that they do exist it's too late to change that for everything
already existing, I suppose. However, it also doesn't seem like adding
such a header for MT7988 as well is going to be acknowledged, hence we
will have to live with the inconsistency in the driver in which older
SoCs will obtain the number of resets from a macro in their respective
dt-bindings header while newer SoCs won't have such header and hence
it will have to be defined in the driver itself (as that's also the
only place where that number is being used).

> Please make it a define at the top of the file, though.

Ack. Will do.


[1]: https://lore.kernel.org/lkml/6912f6f406bc45674020681184f3eeca2f2cb63f.1699576174.git.daniel@makrotopia.org/T/#ef01d7efc6c4fbf1d4830bafe7c90e39746939671

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

[Guenter Roeck]

On 11/20/23 09:33, Daniel Golle wrote:
> On Mon, Nov 20, 2023 at 09:19:46AM -0800, Guenter Roeck wrote:
>> On 11/14/23 09:04, Daniel Golle wrote:
>>> [...]
>>> @@ -89,6 +93,11 @@ static const struct mtk_wdt_data mt7986_data = {
>>>    	.toprgu_sw_rst_num = MT7986_TOPRGU_SW_RST_NUM,
>>>    };
>>> +static const struct mtk_wdt_data mt7988_data = {
>>> +	.toprgu_sw_rst_num = 24,
>>
>> Kind of odd to have this defined locally, while the others are in include files,
>> but not worth arguing about.
> 
>>From I have just learned from Krzysztof Kozlowski those headers shouldn't
> even exist in first place, as the listed IDs are not actually referenced
> anywhere in the driver, hence they aren't actually bindings [1].
> 
> Quote from that thread:
> | >>> Where is the driver change using these IDs?
> | >> It isn't needed as the driver doesn't list the IDs. [...]
> | > Then it is not a binding.
> ---
> 
> Now that they do exist it's too late to change that for everything
> already existing, I suppose. However, it also doesn't seem like adding
> such a header for MT7988 as well is going to be acknowledged, hence we
> will have to live with the inconsistency in the driver in which older
> SoCs will obtain the number of resets from a macro in their respective
> dt-bindings header while newer SoCs won't have such header and hence
> it will have to be defined in the driver itself (as that's also the
> only place where that number is being used).
> 

As I said, not worth arguing about. However, it seems to me that "too late
to change that for everything" isn't really correct. If MTxxxx_TOPRGU_RST_NUM
isn't supposed to be in devicetree include files, all those defines could be
removed from the from there and be added to the watchdog driver. I don't know
about the other defines in include/dt-bindings/reset/mediatek,mtXXXX-resets.h -
many of those _are_ used in dtsi files, but many others are not.

In summary, while I don't really know/understand what is supposed to be defined
in include/dt-bindings/, whatever is known to _not_ to be there (such as the
total number of reset pins on a SoC) could be moved into the driver(s) using it.

Of course, it might well be that there is a rule saying that anything in
include/dt-bindings/ must not be removed from it even if it is not supposed
to be there. In that case, my apologies for the noise.

Thanks,
Guenter