| Message ID | 20230213200519.889503-4-sergio.paracuellos@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | watchdog: mt7621-wdt: avoid globals and arch dependencies | expand |
On 13/02/2023 21:05, Sergio Paracuellos wrote: > MT7621 SoC has a system controller node. Watchdog need to access to reset > status register. Ralink architecture and related driver are old and from > the beggining they are using some architecture dependent operations for > accessing this shared registers through 'asm/mach-ralink/ralink_regs.h' > header file. However this is not ideal from a driver perspective which can > just access to the system controller registers in an arch independent way > using regmap syscon APIs. Update Kconfig accordingly to select new added > dependencies and allow driver to be compile tested. > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> > --- > drivers/watchdog/Kconfig | 4 +++- > drivers/watchdog/mt7621_wdt.c | 18 +++++++++++++----- > 2 files changed, 16 insertions(+), 6 deletions(-) > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index b64bc49c7..cf752ad64 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -1865,7 +1865,9 @@ config GXP_WATCHDOG > config MT7621_WDT > tristate "Mediatek SoC watchdog" > select WATCHDOG_CORE > - depends on SOC_MT7620 || SOC_MT7621 > + select REGMAP_MMIO > + select MFD_SYSCON > + depends on SOC_MT7620 || SOC_MT7621 || COMPILE_TEST > help > Hardware driver for the Mediatek/Ralink MT7621/8 SoC Watchdog Timer. > > diff --git a/drivers/watchdog/mt7621_wdt.c b/drivers/watchdog/mt7621_wdt.c > index 40fb2c9ba..22e979bdd 100644 > --- a/drivers/watchdog/mt7621_wdt.c > +++ b/drivers/watchdog/mt7621_wdt.c > @@ -15,8 +15,8 @@ > #include <linux/moduleparam.h> > #include <linux/platform_device.h> > #include <linux/mod_devicetable.h> > - > -#include <asm/mach-ralink/ralink_regs.h> > +#include <linux/mfd/syscon.h> > +#include <linux/regmap.h> > > #define SYSC_RSTSTAT 0x38 > #define WDT_RST_CAUSE BIT(1) > @@ -34,6 +34,7 @@ > struct mt7621_wdt_data { > void __iomem *base; > struct reset_control *rst; > + struct regmap *sysc; > struct watchdog_device wdt; > }; > > @@ -104,9 +105,12 @@ static int mt7621_wdt_stop(struct watchdog_device *w) > return 0; > } > > -static int mt7621_wdt_bootcause(void) > +static int mt7621_wdt_bootcause(struct mt7621_wdt_data *d) > { > - if (rt_sysc_r32(SYSC_RSTSTAT) & WDT_RST_CAUSE) > + u32 val; > + > + regmap_read(d->sysc, SYSC_RSTSTAT, &val); > + if (val & WDT_RST_CAUSE) > return WDIOF_CARDRESET; > > return 0; > @@ -143,6 +147,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev) > if (!drvdata) > return -ENOMEM; > > + drvdata->sysc = syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc"); > + if (IS_ERR(drvdata->sysc)) > + return PTR_ERR(drvdata->sysc); This should be the backup/error path for original code using syscon property. Looking up by compatible is really not portable/re-usable. Best regards, Krzysztof
On Tue, Feb 14, 2023 at 9:31 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 13/02/2023 21:05, Sergio Paracuellos wrote: > > MT7621 SoC has a system controller node. Watchdog need to access to reset > > status register. Ralink architecture and related driver are old and from > > the beggining they are using some architecture dependent operations for > > accessing this shared registers through 'asm/mach-ralink/ralink_regs.h' > > header file. However this is not ideal from a driver perspective which can > > just access to the system controller registers in an arch independent way > > using regmap syscon APIs. Update Kconfig accordingly to select new added > > dependencies and allow driver to be compile tested. > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> > > --- > > drivers/watchdog/Kconfig | 4 +++- > > drivers/watchdog/mt7621_wdt.c | 18 +++++++++++++----- > > 2 files changed, 16 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > > index b64bc49c7..cf752ad64 100644 > > --- a/drivers/watchdog/Kconfig > > +++ b/drivers/watchdog/Kconfig > > @@ -1865,7 +1865,9 @@ config GXP_WATCHDOG > > config MT7621_WDT > > tristate "Mediatek SoC watchdog" > > select WATCHDOG_CORE > > - depends on SOC_MT7620 || SOC_MT7621 > > + select REGMAP_MMIO > > + select MFD_SYSCON > > + depends on SOC_MT7620 || SOC_MT7621 || COMPILE_TEST > > help > > Hardware driver for the Mediatek/Ralink MT7621/8 SoC Watchdog Timer. > > > > diff --git a/drivers/watchdog/mt7621_wdt.c b/drivers/watchdog/mt7621_wdt.c > > index 40fb2c9ba..22e979bdd 100644 > > --- a/drivers/watchdog/mt7621_wdt.c > > +++ b/drivers/watchdog/mt7621_wdt.c > > @@ -15,8 +15,8 @@ > > #include <linux/moduleparam.h> > > #include <linux/platform_device.h> > > #include <linux/mod_devicetable.h> > > - > > -#include <asm/mach-ralink/ralink_regs.h> > > +#include <linux/mfd/syscon.h> > > +#include <linux/regmap.h> > > > > #define SYSC_RSTSTAT 0x38 > > #define WDT_RST_CAUSE BIT(1) > > @@ -34,6 +34,7 @@ > > struct mt7621_wdt_data { > > void __iomem *base; > > struct reset_control *rst; > > + struct regmap *sysc; > > struct watchdog_device wdt; > > }; > > > > @@ -104,9 +105,12 @@ static int mt7621_wdt_stop(struct watchdog_device *w) > > return 0; > > } > > > > -static int mt7621_wdt_bootcause(void) > > +static int mt7621_wdt_bootcause(struct mt7621_wdt_data *d) > > { > > - if (rt_sysc_r32(SYSC_RSTSTAT) & WDT_RST_CAUSE) > > + u32 val; > > + > > + regmap_read(d->sysc, SYSC_RSTSTAT, &val); > > + if (val & WDT_RST_CAUSE) > > return WDIOF_CARDRESET; > > > > return 0; > > @@ -143,6 +147,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev) > > if (!drvdata) > > return -ENOMEM; > > > > + drvdata->sysc = syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc"); > > + if (IS_ERR(drvdata->sysc)) > > + return PTR_ERR(drvdata->sysc); > > This should be the backup/error path for original code using syscon > property. Looking up by compatible is really not portable/re-usable. I can change the code in the way you are pointing out here but... Why is it not re-usable? Compatible is not supposed to be changed since in other cases the DTB ABI will be broken. I am totally lost about what is an ABI breakage, then. Thanks in advance for clarification. Best regards, Sergio Paracuellos > > Best regards, > Krzysztof >
On 2/14/23 00:31, Krzysztof Kozlowski wrote: > On 13/02/2023 21:05, Sergio Paracuellos wrote: >> MT7621 SoC has a system controller node. Watchdog need to access to reset >> status register. Ralink architecture and related driver are old and from >> the beggining they are using some architecture dependent operations for >> accessing this shared registers through 'asm/mach-ralink/ralink_regs.h' >> header file. However this is not ideal from a driver perspective which can >> just access to the system controller registers in an arch independent way >> using regmap syscon APIs. Update Kconfig accordingly to select new added >> dependencies and allow driver to be compile tested. >> >> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> >> --- >> drivers/watchdog/Kconfig | 4 +++- >> drivers/watchdog/mt7621_wdt.c | 18 +++++++++++++----- >> 2 files changed, 16 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >> index b64bc49c7..cf752ad64 100644 >> --- a/drivers/watchdog/Kconfig >> +++ b/drivers/watchdog/Kconfig >> @@ -1865,7 +1865,9 @@ config GXP_WATCHDOG >> config MT7621_WDT >> tristate "Mediatek SoC watchdog" >> select WATCHDOG_CORE >> - depends on SOC_MT7620 || SOC_MT7621 >> + select REGMAP_MMIO >> + select MFD_SYSCON >> + depends on SOC_MT7620 || SOC_MT7621 || COMPILE_TEST >> help >> Hardware driver for the Mediatek/Ralink MT7621/8 SoC Watchdog Timer. >> >> diff --git a/drivers/watchdog/mt7621_wdt.c b/drivers/watchdog/mt7621_wdt.c >> index 40fb2c9ba..22e979bdd 100644 >> --- a/drivers/watchdog/mt7621_wdt.c >> +++ b/drivers/watchdog/mt7621_wdt.c >> @@ -15,8 +15,8 @@ >> #include <linux/moduleparam.h> >> #include <linux/platform_device.h> >> #include <linux/mod_devicetable.h> >> - >> -#include <asm/mach-ralink/ralink_regs.h> >> +#include <linux/mfd/syscon.h> >> +#include <linux/regmap.h> >> >> #define SYSC_RSTSTAT 0x38 >> #define WDT_RST_CAUSE BIT(1) >> @@ -34,6 +34,7 @@ >> struct mt7621_wdt_data { >> void __iomem *base; >> struct reset_control *rst; >> + struct regmap *sysc; >> struct watchdog_device wdt; >> }; >> >> @@ -104,9 +105,12 @@ static int mt7621_wdt_stop(struct watchdog_device *w) >> return 0; >> } >> >> -static int mt7621_wdt_bootcause(void) >> +static int mt7621_wdt_bootcause(struct mt7621_wdt_data *d) >> { >> - if (rt_sysc_r32(SYSC_RSTSTAT) & WDT_RST_CAUSE) >> + u32 val; >> + >> + regmap_read(d->sysc, SYSC_RSTSTAT, &val); >> + if (val & WDT_RST_CAUSE) >> return WDIOF_CARDRESET; >> >> return 0; >> @@ -143,6 +147,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev) >> if (!drvdata) >> return -ENOMEM; >> >> + drvdata->sysc = syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc"); >> + if (IS_ERR(drvdata->sysc)) >> + return PTR_ERR(drvdata->sysc); > > This should be the backup/error path for original code using syscon > property. Looking up by compatible is really not portable/re-usable. > I really disagree here. $ git grep syscon_regmap_lookup_by_compatible | wc 90 326 8940 I have not yet reviewed this code, but I do not accept this argument against it. Guenter
On 14/02/2023 13:10, Guenter Roeck wrote: > On 2/14/23 00:31, Krzysztof Kozlowski wrote: >> On 13/02/2023 21:05, Sergio Paracuellos wrote: >>> MT7621 SoC has a system controller node. Watchdog need to access to reset >>> status register. Ralink architecture and related driver are old and from >>> the beggining they are using some architecture dependent operations for >>> accessing this shared registers through 'asm/mach-ralink/ralink_regs.h' >>> header file. However this is not ideal from a driver perspective which can >>> just access to the system controller registers in an arch independent way >>> using regmap syscon APIs. Update Kconfig accordingly to select new added >>> dependencies and allow driver to be compile tested. >>> >>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> >>> --- >>> drivers/watchdog/Kconfig | 4 +++- >>> drivers/watchdog/mt7621_wdt.c | 18 +++++++++++++----- >>> 2 files changed, 16 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >>> index b64bc49c7..cf752ad64 100644 >>> --- a/drivers/watchdog/Kconfig >>> +++ b/drivers/watchdog/Kconfig >>> @@ -1865,7 +1865,9 @@ config GXP_WATCHDOG >>> config MT7621_WDT >>> tristate "Mediatek SoC watchdog" >>> select WATCHDOG_CORE >>> - depends on SOC_MT7620 || SOC_MT7621 >>> + select REGMAP_MMIO >>> + select MFD_SYSCON >>> + depends on SOC_MT7620 || SOC_MT7621 || COMPILE_TEST >>> help >>> Hardware driver for the Mediatek/Ralink MT7621/8 SoC Watchdog Timer. >>> >>> diff --git a/drivers/watchdog/mt7621_wdt.c b/drivers/watchdog/mt7621_wdt.c >>> index 40fb2c9ba..22e979bdd 100644 >>> --- a/drivers/watchdog/mt7621_wdt.c >>> +++ b/drivers/watchdog/mt7621_wdt.c >>> @@ -15,8 +15,8 @@ >>> #include <linux/moduleparam.h> >>> #include <linux/platform_device.h> >>> #include <linux/mod_devicetable.h> >>> - >>> -#include <asm/mach-ralink/ralink_regs.h> >>> +#include <linux/mfd/syscon.h> >>> +#include <linux/regmap.h> >>> >>> #define SYSC_RSTSTAT 0x38 >>> #define WDT_RST_CAUSE BIT(1) >>> @@ -34,6 +34,7 @@ >>> struct mt7621_wdt_data { >>> void __iomem *base; >>> struct reset_control *rst; >>> + struct regmap *sysc; >>> struct watchdog_device wdt; >>> }; >>> >>> @@ -104,9 +105,12 @@ static int mt7621_wdt_stop(struct watchdog_device *w) >>> return 0; >>> } >>> >>> -static int mt7621_wdt_bootcause(void) >>> +static int mt7621_wdt_bootcause(struct mt7621_wdt_data *d) >>> { >>> - if (rt_sysc_r32(SYSC_RSTSTAT) & WDT_RST_CAUSE) >>> + u32 val; >>> + >>> + regmap_read(d->sysc, SYSC_RSTSTAT, &val); >>> + if (val & WDT_RST_CAUSE) >>> return WDIOF_CARDRESET; >>> >>> return 0; >>> @@ -143,6 +147,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev) >>> if (!drvdata) >>> return -ENOMEM; >>> >>> + drvdata->sysc = syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc"); >>> + if (IS_ERR(drvdata->sysc)) >>> + return PTR_ERR(drvdata->sysc); >> >> This should be the backup/error path for original code using syscon >> property. Looking up by compatible is really not portable/re-usable. >> > > I really disagree here. > > $ git grep syscon_regmap_lookup_by_compatible | wc > 90 326 8940 > > I have not yet reviewed this code, but I do not accept this argument against it. First, argument that bad pattern is being used is not an argument to keep it and repeat it. Second, we already had examples that: 1. Author used syscon_regmap_lookup_by_compatible() and assumed "we will never add new variant/soc". 2. Then turns out that new variants are obviously added and syscon_regmap_lookup_by_compatible() stops scaling. Whether any new variant/compatible/platform can appear for this watchdog - I don't know. Third, with syscon_regmap_lookup_by_compatible() you have undocumented (not in the binding) dependency between blocks which: a. stops any reusability, b. affects device links and probe ordering (simply - there is no, device must defer probe), c. is simply undocumented. The usage of syscon_regmap_lookup_by_compatible() has clear drawbacks thus new code should rather use syscon phandles which solve all of above. Best regards, Krzysztof
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index b64bc49c7..cf752ad64 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -1865,7 +1865,9 @@ config GXP_WATCHDOG config MT7621_WDT tristate "Mediatek SoC watchdog" select WATCHDOG_CORE - depends on SOC_MT7620 || SOC_MT7621 + select REGMAP_MMIO + select MFD_SYSCON + depends on SOC_MT7620 || SOC_MT7621 || COMPILE_TEST help Hardware driver for the Mediatek/Ralink MT7621/8 SoC Watchdog Timer. diff --git a/drivers/watchdog/mt7621_wdt.c b/drivers/watchdog/mt7621_wdt.c index 40fb2c9ba..22e979bdd 100644 --- a/drivers/watchdog/mt7621_wdt.c +++ b/drivers/watchdog/mt7621_wdt.c @@ -15,8 +15,8 @@ #include <linux/moduleparam.h> #include <linux/platform_device.h> #include <linux/mod_devicetable.h> - -#include <asm/mach-ralink/ralink_regs.h> +#include <linux/mfd/syscon.h> +#include <linux/regmap.h> #define SYSC_RSTSTAT 0x38 #define WDT_RST_CAUSE BIT(1) @@ -34,6 +34,7 @@ struct mt7621_wdt_data { void __iomem *base; struct reset_control *rst; + struct regmap *sysc; struct watchdog_device wdt; }; @@ -104,9 +105,12 @@ static int mt7621_wdt_stop(struct watchdog_device *w) return 0; } -static int mt7621_wdt_bootcause(void) +static int mt7621_wdt_bootcause(struct mt7621_wdt_data *d) { - if (rt_sysc_r32(SYSC_RSTSTAT) & WDT_RST_CAUSE) + u32 val; + + regmap_read(d->sysc, SYSC_RSTSTAT, &val); + if (val & WDT_RST_CAUSE) return WDIOF_CARDRESET; return 0; @@ -143,6 +147,10 @@ static int mt7621_wdt_probe(struct platform_device *pdev) if (!drvdata) return -ENOMEM; + drvdata->sysc = syscon_regmap_lookup_by_compatible("mediatek,mt7621-sysc"); + if (IS_ERR(drvdata->sysc)) + return PTR_ERR(drvdata->sysc); + drvdata->base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(drvdata->base)) return PTR_ERR(drvdata->base); @@ -158,7 +166,7 @@ static int mt7621_wdt_probe(struct platform_device *pdev) mt7621_wdt->max_timeout = 0xfffful / 1000; mt7621_wdt->parent = dev; - mt7621_wdt->bootstatus = mt7621_wdt_bootcause(); + mt7621_wdt->bootstatus = mt7621_wdt_bootcause(drvdata); watchdog_init_timeout(mt7621_wdt, mt7621_wdt->max_timeout, dev); watchdog_set_nowayout(mt7621_wdt, nowayout);
MT7621 SoC has a system controller node. Watchdog need to access to reset status register. Ralink architecture and related driver are old and from the beggining they are using some architecture dependent operations for accessing this shared registers through 'asm/mach-ralink/ralink_regs.h' header file. However this is not ideal from a driver perspective which can just access to the system controller registers in an arch independent way using regmap syscon APIs. Update Kconfig accordingly to select new added dependencies and allow driver to be compile tested. Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> --- drivers/watchdog/Kconfig | 4 +++- drivers/watchdog/mt7621_wdt.c | 18 +++++++++++++----- 2 files changed, 16 insertions(+), 6 deletions(-)