| Message ID | 3f5ed9656ea66637d259e9771ed852511369ba9b.1699980962.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 watchdog and toprgu | expand |
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
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
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
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 },
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(+)