diff mbox series

[v2,2/2] watchdog: s3c2410_wdt: use exynos_get_pmu_regmap_by_phandle() for PMU regs

Message ID 20240129211912.3068411-3-peter.griffin@linaro.org (mailing list archive)
State New
Headers show
Series Add regmap support to exynos-pmu for protected PMU regs | expand

Commit Message

Peter Griffin Jan. 29, 2024, 9:19 p.m. UTC
Obtain the PMU regmap using the new API added to exynos-pmu driver rather
than syscon_regmap_lookup_by_phandle(). As this driver no longer depends
on mfd syscon remove that header and Kconfig dependency.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 drivers/watchdog/Kconfig       | 1 -
 drivers/watchdog/s3c2410_wdt.c | 9 +++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Saravana Kannan Jan. 29, 2024, 10:25 p.m. UTC | #1
On Mon, Jan 29, 2024 at 1:19 PM Peter Griffin <peter.griffin@linaro.org> wrote:
>
> Obtain the PMU regmap using the new API added to exynos-pmu driver rather
> than syscon_regmap_lookup_by_phandle(). As this driver no longer depends
> on mfd syscon remove that header and Kconfig dependency.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  drivers/watchdog/Kconfig       | 1 -
>  drivers/watchdog/s3c2410_wdt.c | 9 +++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7d22051b15a2..d78fe7137799 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -512,7 +512,6 @@ config S3C2410_WATCHDOG
>         tristate "S3C6410/S5Pv210/Exynos Watchdog"
>         depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
>         select WATCHDOG_CORE
> -       select MFD_SYSCON if ARCH_EXYNOS
>         help
>           Watchdog timer block in the Samsung S3C64xx, S5Pv210 and Exynos
>           SoCs. This will reboot the system when the timer expires with
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 349d30462c8c..a1e2682c7e57 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -24,9 +24,9 @@
>  #include <linux/slab.h>
>  #include <linux/err.h>
>  #include <linux/of.h>
> -#include <linux/mfd/syscon.h>
>  #include <linux/regmap.h>
>  #include <linux/delay.h>
> +#include <linux/soc/samsung/exynos-pmu.h>
>
>  #define S3C2410_WTCON          0x00
>  #define S3C2410_WTDAT          0x04
> @@ -699,11 +699,12 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>                 return ret;
>
>         if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> -               wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> -                                               "samsung,syscon-phandle");
> +
> +               wdt->pmureg = exynos_get_pmu_regmap_by_phandle(dev->of_node,
> +                                                "samsung,syscon-phandle");

IIUC, the exynos PMU driver is registering a regmap interface with
regmap framework. So, can't we get the remap from the framework
instead of directly talking to the PMU driver?

-Saravana

>                 if (IS_ERR(wdt->pmureg))
>                         return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
> -                                            "syscon regmap lookup failed.\n");
> +                                            "PMU regmap lookup failed.\n");
>         }
>
>         wdt_irq = platform_get_irq(pdev, 0);
> --
> 2.43.0.429.g432eaa2c6b-goog
>
Sam Protsenko Jan. 30, 2024, 3:38 a.m. UTC | #2
On Mon, Jan 29, 2024 at 4:25 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Mon, Jan 29, 2024 at 1:19 PM Peter Griffin <peter.griffin@linaro.org> wrote:
> >
> > Obtain the PMU regmap using the new API added to exynos-pmu driver rather
> > than syscon_regmap_lookup_by_phandle(). As this driver no longer depends
> > on mfd syscon remove that header and Kconfig dependency.
> >
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > ---
> >  drivers/watchdog/Kconfig       | 1 -
> >  drivers/watchdog/s3c2410_wdt.c | 9 +++++----
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 7d22051b15a2..d78fe7137799 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -512,7 +512,6 @@ config S3C2410_WATCHDOG
> >         tristate "S3C6410/S5Pv210/Exynos Watchdog"
> >         depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
> >         select WATCHDOG_CORE
> > -       select MFD_SYSCON if ARCH_EXYNOS

That reminds me: now that exynos-pmu driver uses regmap API, does it
make sense to add something like "select REGMAP" to EXYNOS_PMU option?

> >         help
> >           Watchdog timer block in the Samsung S3C64xx, S5Pv210 and Exynos
> >           SoCs. This will reboot the system when the timer expires with
> > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > index 349d30462c8c..a1e2682c7e57 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -24,9 +24,9 @@
> >  #include <linux/slab.h>
> >  #include <linux/err.h>
> >  #include <linux/of.h>
> > -#include <linux/mfd/syscon.h>
> >  #include <linux/regmap.h>
> >  #include <linux/delay.h>
> > +#include <linux/soc/samsung/exynos-pmu.h>
> >
> >  #define S3C2410_WTCON          0x00
> >  #define S3C2410_WTDAT          0x04
> > @@ -699,11 +699,12 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >                 return ret;
> >
> >         if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> > -               wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> > -                                               "samsung,syscon-phandle");
> > +
> > +               wdt->pmureg = exynos_get_pmu_regmap_by_phandle(dev->of_node,
> > +                                                "samsung,syscon-phandle");
>

This looks so much better than approach taken in v1, as for my taste.
For this patch:

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

> IIUC, the exynos PMU driver is registering a regmap interface with
> regmap framework. So, can't we get the remap from the framework
> instead of directly talking to the PMU driver?
>

Peter is basically re-implementing syscon driver with overridden
operations, as a part of exynos-pmu driver, in previous patch. Which
means syscon API can't be used anymore to obtain the regmap. Do you
have particular API in mind that allows getting a random regmap
registered with devm_regmap_init()?

> -Saravana
>
> >                 if (IS_ERR(wdt->pmureg))
> >                         return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
> > -                                            "syscon regmap lookup failed.\n");
> > +                                            "PMU regmap lookup failed.\n");
> >         }
> >
> >         wdt_irq = platform_get_irq(pdev, 0);
> > --
> > 2.43.0.429.g432eaa2c6b-goog
> >
Peter Griffin Jan. 30, 2024, 3:31 p.m. UTC | #3
Hi Sam & Saravana,

On Tue, 30 Jan 2024 at 03:38, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> On Mon, Jan 29, 2024 at 4:25 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Mon, Jan 29, 2024 at 1:19 PM Peter Griffin <peter.griffin@linaro.org> wrote:
> > >
> > > Obtain the PMU regmap using the new API added to exynos-pmu driver rather
> > > than syscon_regmap_lookup_by_phandle(). As this driver no longer depends
> > > on mfd syscon remove that header and Kconfig dependency.
> > >
> > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > > ---
> > >  drivers/watchdog/Kconfig       | 1 -
> > >  drivers/watchdog/s3c2410_wdt.c | 9 +++++----
> > >  2 files changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > index 7d22051b15a2..d78fe7137799 100644
> > > --- a/drivers/watchdog/Kconfig
> > > +++ b/drivers/watchdog/Kconfig
> > > @@ -512,7 +512,6 @@ config S3C2410_WATCHDOG
> > >         tristate "S3C6410/S5Pv210/Exynos Watchdog"
> > >         depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
> > >         select WATCHDOG_CORE
> > > -       select MFD_SYSCON if ARCH_EXYNOS
>
> That reminds me: now that exynos-pmu driver uses regmap API, does it
> make sense to add something like "select REGMAP" to EXYNOS_PMU option?

Good point, I will add that in v3.

>
> > >         help
> > >           Watchdog timer block in the Samsung S3C64xx, S5Pv210 and Exynos
> > >           SoCs. This will reboot the system when the timer expires with
> > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > > index 349d30462c8c..a1e2682c7e57 100644
> > > --- a/drivers/watchdog/s3c2410_wdt.c
> > > +++ b/drivers/watchdog/s3c2410_wdt.c
> > > @@ -24,9 +24,9 @@
> > >  #include <linux/slab.h>
> > >  #include <linux/err.h>
> > >  #include <linux/of.h>
> > > -#include <linux/mfd/syscon.h>
> > >  #include <linux/regmap.h>
> > >  #include <linux/delay.h>
> > > +#include <linux/soc/samsung/exynos-pmu.h>
> > >
> > >  #define S3C2410_WTCON          0x00
> > >  #define S3C2410_WTDAT          0x04
> > > @@ -699,11 +699,12 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > >                 return ret;
> > >
> > >         if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> > > -               wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> > > -                                               "samsung,syscon-phandle");
> > > +
> > > +               wdt->pmureg = exynos_get_pmu_regmap_by_phandle(dev->of_node,
> > > +                                                "samsung,syscon-phandle");
> >
>
> This looks so much better than approach taken in v1, as for my taste.
> For this patch:
>
> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

Thanks!
>
> > IIUC, the exynos PMU driver is registering a regmap interface with
> > regmap framework. So, can't we get the remap from the framework
> > instead of directly talking to the PMU driver?
> >

I'm not aware of any existing regmap API that does that. A quick look
through regmap code I can't see any global state that is stored on a
regmap_init() for example of all the regmaps created in the system.

As Sam mentions below, prior to these patches the syscon device would
be creating the PMU mmio regmap and consumers drivers would be
obtaining the regmap by going through the syscon driver. After this
series we instead talk to the exynos-pmu driver to obtain the pmu
regmap which can either be one with overridden operations for issuing
SMC calls to do the register accesses or still a mmio regmap.

Folks tried in the past to add a SMC backend to regmap similar to
regmap_mmio and add support for it into syscon driver but this was
nacked (see here
https://lore.kernel.org/lkml/20210723163759.GI5221@sirena.org.uk/T/)

>
> Peter is basically re-implementing syscon driver with overridden
> operations, as a part of exynos-pmu driver, in previous patch. Which
> means syscon API can't be used anymore to obtain the regmap. Do you
> have particular API in mind that allows getting a random regmap
> registered with devm_regmap_init()?

Thanks,

Peter
diff mbox series

Patch

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 7d22051b15a2..d78fe7137799 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -512,7 +512,6 @@  config S3C2410_WATCHDOG
 	tristate "S3C6410/S5Pv210/Exynos Watchdog"
 	depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
 	select WATCHDOG_CORE
-	select MFD_SYSCON if ARCH_EXYNOS
 	help
 	  Watchdog timer block in the Samsung S3C64xx, S5Pv210 and Exynos
 	  SoCs. This will reboot the system when the timer expires with
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 349d30462c8c..a1e2682c7e57 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -24,9 +24,9 @@ 
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/of.h>
-#include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
 #include <linux/delay.h>
+#include <linux/soc/samsung/exynos-pmu.h>
 
 #define S3C2410_WTCON		0x00
 #define S3C2410_WTDAT		0x04
@@ -699,11 +699,12 @@  static int s3c2410wdt_probe(struct platform_device *pdev)
 		return ret;
 
 	if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
-		wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
-						"samsung,syscon-phandle");
+
+		wdt->pmureg = exynos_get_pmu_regmap_by_phandle(dev->of_node,
+						 "samsung,syscon-phandle");
 		if (IS_ERR(wdt->pmureg))
 			return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
-					     "syscon regmap lookup failed.\n");
+					     "PMU regmap lookup failed.\n");
 	}
 
 	wdt_irq = platform_get_irq(pdev, 0);