diff mbox series

[3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis

Message ID 20240122225710.1952066-4-peter.griffin@linaro.org (mailing list archive)
State New
Headers show
Series Add exynos_pmu_update/read/write() APIs to exynos-pmu | expand

Commit Message

Peter Griffin Jan. 22, 2024, 10:57 p.m. UTC
Instead of obtaining the PMU regmap directly use the new exynos_pmu_*()
APIs. The exynos_pmu_ APIs allow support of newer Exynos SoCs that have
atomic set/clear bit hardware and platforms where the PMU registers can
only be accessed via SMC call.

As all platforms that have PMU registers use these new APIs, remove the
syscon regmap lookup code, as it is now redundant.

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

Comments

Guenter Roeck Jan. 23, 2024, 10:33 a.m. UTC | #1
On 1/22/24 14:57, Peter Griffin wrote:
> Instead of obtaining the PMU regmap directly use the new exynos_pmu_*()
> APIs. The exynos_pmu_ APIs allow support of newer Exynos SoCs that have
> atomic set/clear bit hardware and platforms where the PMU registers can
> only be accessed via SMC call.
> 

Not really sure about using a direect API instead of regmap. I personally
think that regmap is more generic and like the idea of abstracting hardware
accesses this way. Since that is POV, I won't argue about it. However,

> As all platforms that have PMU registers use these new APIs, remove the
> syscon regmap lookup code, as it is now redundant.
>

if syscon is now no longer needed, why keep selecting MFD_SYSCON below,
and why are linux/mfd/syscon.h and linux/regmap.h still included ?

Also, the driver did not previously only support ARCH_EXYNOS but also
ARCH_S3C64XX and ARCH_S5PV210. It is not entirely (actually, not at all)
clear to me if and how those platforms are now supported. EXYNOS_PMU
still seems to depend on ARCH_EXYNOS. How can the driver select
EXYNOS_PMU if ARCH_EXYNOS=n ?

Also, ARCH_EXYNOS already selects EXYNOS_PMU, so a conditional
"select EXYNOS_PMU if ARCH_EXYNOS" would not make sense (or be required)
either.

Please explain all the above.

Thanks,
Guenter

> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>   drivers/watchdog/Kconfig       |  1 +
>   drivers/watchdog/s3c2410_wdt.c | 25 +++++++++----------------
>   2 files changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7d22051b15a2..b3e90e1ddf14 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -513,6 +513,7 @@ config S3C2410_WATCHDOG
>   	depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
>   	select WATCHDOG_CORE
>   	select MFD_SYSCON if ARCH_EXYNOS
> +	select EXYNOS_PMU
>   	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..fd3a9ce870a0 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -28,6 +28,8 @@
>   #include <linux/regmap.h>
>   #include <linux/delay.h>
>   
> +#include <linux/soc/samsung/exynos-pmu.h>
> +
>   #define S3C2410_WTCON		0x00
>   #define S3C2410_WTDAT		0x04
>   #define S3C2410_WTCNT		0x08
> @@ -187,7 +189,6 @@ struct s3c2410_wdt {
>   	struct watchdog_device	wdt_device;
>   	struct notifier_block	freq_transition;
>   	const struct s3c2410_wdt_variant *drv_data;
> -	struct regmap *pmureg;
>   };
>   
>   static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
> @@ -355,8 +356,8 @@ static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
>   	const u32 val = mask ? mask_val : 0;
>   	int ret;
>   
> -	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->disable_reg,
> -				 mask_val, val);
> +	ret = exynos_pmu_update(wdt->drv_data->disable_reg,
> +				mask_val, val);
>   	if (ret < 0)
>   		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
>   
> @@ -370,8 +371,8 @@ static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
>   	const u32 val = (mask ^ val_inv) ? mask_val : 0;
>   	int ret;
>   
> -	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg,
> -				 mask_val, val);
> +	ret = exynos_pmu_update(wdt->drv_data->mask_reset_reg,
> +				mask_val, val);
>   	if (ret < 0)
>   		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
>   
> @@ -384,8 +385,8 @@ static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en)
>   	const u32 val = en ? mask_val : 0;
>   	int ret;
>   
> -	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->cnt_en_reg,
> -				 mask_val, val);
> +	ret = exynos_pmu_update(wdt->drv_data->cnt_en_reg,
> +				mask_val, val);
>   	if (ret < 0)
>   		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
>   
> @@ -617,7 +618,7 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
>   	if (!(wdt->drv_data->quirks & QUIRK_HAS_PMU_RST_STAT))
>   		return 0;
>   
> -	ret = regmap_read(wdt->pmureg, wdt->drv_data->rst_stat_reg, &rst_stat);
> +	ret = exynos_pmu_read(wdt->drv_data->rst_stat_reg, &rst_stat);
>   	if (ret)
>   		dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
>   	else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> -	if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> -		wdt->pmureg = syscon_regmap_lookup_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");
> -	}
> -
>   	wdt_irq = platform_get_irq(pdev, 0);
>   	if (wdt_irq < 0)
>   		return wdt_irq;
Krzysztof Kozlowski Jan. 23, 2024, 11:19 a.m. UTC | #2
On 22/01/2024 23:57, Peter Griffin wrote:
> Instead of obtaining the PMU regmap directly use the new exynos_pmu_*()
> APIs. The exynos_pmu_ APIs allow support of newer Exynos SoCs that have
> atomic set/clear bit hardware and platforms where the PMU registers can
> only be accessed via SMC call.
> 
> As all platforms that have PMU registers use these new APIs, remove the
> syscon regmap lookup code, as it is now redundant.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  drivers/watchdog/Kconfig       |  1 +
>  drivers/watchdog/s3c2410_wdt.c | 25 +++++++++----------------
>  2 files changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7d22051b15a2..b3e90e1ddf14 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -513,6 +513,7 @@ config S3C2410_WATCHDOG
>  	depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
>  	select WATCHDOG_CORE
>  	select MFD_SYSCON if ARCH_EXYNOS
> +	select EXYNOS_PMU

This does not look compatible with S3C64xx and S5Pv210.

>  	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..fd3a9ce870a0 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -28,6 +28,8 @@
>  #include <linux/regmap.h>
>  #include <linux/delay.h>
>  
> +#include <linux/soc/samsung/exynos-pmu.h>
> +
>  #define S3C2410_WTCON		0x00
>  #define S3C2410_WTDAT		0x04
>  #define S3C2410_WTCNT		0x08
> @@ -187,7 +189,6 @@ struct s3c2410_wdt {
>  	struct watchdog_device	wdt_device;
>  	struct notifier_block	freq_transition;
>  	const struct s3c2410_wdt_variant *drv_data;
> -	struct regmap *pmureg;
>  };
>  
>  static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
> @@ -355,8 +356,8 @@ static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
>  	const u32 val = mask ? mask_val : 0;
>  	int ret;
>  
> -	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->disable_reg,
> -				 mask_val, val);
> +	ret = exynos_pmu_update(wdt->drv_data->disable_reg,
> +				mask_val, val);
>  	if (ret < 0)
>  		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
>  
> @@ -370,8 +371,8 @@ static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
>  	const u32 val = (mask ^ val_inv) ? mask_val : 0;
>  	int ret;
>  
> -	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg,
> -				 mask_val, val);
> +	ret = exynos_pmu_update(wdt->drv_data->mask_reset_reg,
> +				mask_val, val);
>  	if (ret < 0)
>  		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
>  
> @@ -384,8 +385,8 @@ static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en)
>  	const u32 val = en ? mask_val : 0;
>  	int ret;
>  
> -	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->cnt_en_reg,
> -				 mask_val, val);
> +	ret = exynos_pmu_update(wdt->drv_data->cnt_en_reg,
> +				mask_val, val);
>  	if (ret < 0)
>  		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
>  
> @@ -617,7 +618,7 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
>  	if (!(wdt->drv_data->quirks & QUIRK_HAS_PMU_RST_STAT))
>  		return 0;
>  
> -	ret = regmap_read(wdt->pmureg, wdt->drv_data->rst_stat_reg, &rst_stat);
> +	ret = exynos_pmu_read(wdt->drv_data->rst_stat_reg, &rst_stat);
>  	if (ret)
>  		dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
>  	else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> -		wdt->pmureg = syscon_regmap_lookup_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");


Continuing topic from the binding: I don't see how you handle probe
deferral, suspend ordering.

Best regards,
Krzysztof
Peter Griffin Jan. 23, 2024, 3:35 p.m. UTC | #3
Hi Guenter,

Thanks for the review feedback.

On Tue, 23 Jan 2024 at 10:33, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 1/22/24 14:57, Peter Griffin wrote:
> > Instead of obtaining the PMU regmap directly use the new exynos_pmu_*()
> > APIs. The exynos_pmu_ APIs allow support of newer Exynos SoCs that have
> > atomic set/clear bit hardware and platforms where the PMU registers can
> > only be accessed via SMC call.
> >
>
> Not really sure about using a direect API instead of regmap. I personally
> think that regmap is more generic and like the idea of abstracting hardware
> accesses this way. Since that is POV, I won't argue about it. However,

I did also look into the possibility of a SMC backend to regmap but that was
already tried and nacked upstream previously.

>
> > As all platforms that have PMU registers use these new APIs, remove the
> > syscon regmap lookup code, as it is now redundant.
> >
>
> if syscon is now no longer needed, why keep selecting MFD_SYSCON below,
> and why are linux/mfd/syscon.h and linux/regmap.h still included ?

Good point, those headers and the select of MFD_SYSCON are now superfluous.
Will fix it in v2.

> Also, the driver did not previously only support ARCH_EXYNOS but also
> ARCH_S3C64XX and ARCH_S5PV210. It is not entirely (actually, not at all)
> clear to me if and how those platforms are now supported. EXYNOS_PMU
> still seems to depend on ARCH_EXYNOS. How can the driver select
> EXYNOS_PMU if ARCH_EXYNOS=n ?
>
> Also, ARCH_EXYNOS already selects EXYNOS_PMU, so a conditional
> "select EXYNOS_PMU if ARCH_EXYNOS" would not make sense (or be required)
> either.
>
> Please explain all the above.

Fixing this for ARCH_S3C64XX and ARCH_S5PV210 looks to be a case of

+++ b/drivers/watchdog/Kconfig
@@ -512,8 +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
-       select EXYNOS_PMU

and fixing the return type in the stubs that Arnd pointed out.

static inline int exynos_pmu_write(unsigned int offset, unsigned int val)
 {
-       return ERR_PTR(-ENODEV);
+       return -ENODEV;
 }

That then compiles OK with s5pv210_defconfig and s3c6400_defconfig.

Neither ARCH_S3C64XX or ARCH_S5PV210 have PMU registers or set the PMU
register quirk flags so none of the code for setting PMU registers
would get called at runtime on those platforms.
I can make the changes described above in v2 which should fix the
ARCH_S3C64XX and ARCH_S5PV210 compatibility.

Thanks,

Peter
Peter Griffin Jan. 23, 2024, 5:30 p.m. UTC | #4
Hi Krzysztof,

Thanks for your review feedback.

On Tue, 23 Jan 2024 at 11:19, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 22/01/2024 23:57, Peter Griffin wrote:
> > Instead of obtaining the PMU regmap directly use the new exynos_pmu_*()
> > APIs. The exynos_pmu_ APIs allow support of newer Exynos SoCs that have
> > atomic set/clear bit hardware and platforms where the PMU registers can
> > only be accessed via SMC call.
> >
> > As all platforms that have PMU registers use these new APIs, remove the
> > syscon regmap lookup code, as it is now redundant.
> >
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > ---
> >  drivers/watchdog/Kconfig       |  1 +
> >  drivers/watchdog/s3c2410_wdt.c | 25 +++++++++----------------
> >  2 files changed, 10 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 7d22051b15a2..b3e90e1ddf14 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -513,6 +513,7 @@ config S3C2410_WATCHDOG
> >       depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
> >       select WATCHDOG_CORE
> >       select MFD_SYSCON if ARCH_EXYNOS
> > +     select EXYNOS_PMU
>
> This does not look compatible with S3C64xx and S5Pv210.

Please refer to my reply to Guenter on how I propose fixing that in v2.

>
> >       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..fd3a9ce870a0 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -28,6 +28,8 @@
> >  #include <linux/regmap.h>
> >  #include <linux/delay.h>
> >
> > +#include <linux/soc/samsung/exynos-pmu.h>
> > +
> >  #define S3C2410_WTCON                0x00
> >  #define S3C2410_WTDAT                0x04
> >  #define S3C2410_WTCNT                0x08
> > @@ -187,7 +189,6 @@ struct s3c2410_wdt {
> >       struct watchdog_device  wdt_device;
> >       struct notifier_block   freq_transition;
> >       const struct s3c2410_wdt_variant *drv_data;
> > -     struct regmap *pmureg;
> >  };
> >
> >  static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
> > @@ -355,8 +356,8 @@ static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
> >       const u32 val = mask ? mask_val : 0;
> >       int ret;
> >
> > -     ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->disable_reg,
> > -                              mask_val, val);
> > +     ret = exynos_pmu_update(wdt->drv_data->disable_reg,
> > +                             mask_val, val);
> >       if (ret < 0)
> >               dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
> >
> > @@ -370,8 +371,8 @@ static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
> >       const u32 val = (mask ^ val_inv) ? mask_val : 0;
> >       int ret;
> >
> > -     ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg,
> > -                              mask_val, val);
> > +     ret = exynos_pmu_update(wdt->drv_data->mask_reset_reg,
> > +                             mask_val, val);
> >       if (ret < 0)
> >               dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
> >
> > @@ -384,8 +385,8 @@ static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en)
> >       const u32 val = en ? mask_val : 0;
> >       int ret;
> >
> > -     ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->cnt_en_reg,
> > -                              mask_val, val);
> > +     ret = exynos_pmu_update(wdt->drv_data->cnt_en_reg,
> > +                             mask_val, val);
> >       if (ret < 0)
> >               dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
> >
> > @@ -617,7 +618,7 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
> >       if (!(wdt->drv_data->quirks & QUIRK_HAS_PMU_RST_STAT))
> >               return 0;
> >
> > -     ret = regmap_read(wdt->pmureg, wdt->drv_data->rst_stat_reg, &rst_stat);
> > +     ret = exynos_pmu_read(wdt->drv_data->rst_stat_reg, &rst_stat);
> >       if (ret)
> >               dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> >       else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> > @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >       if (ret)
> >               return ret;
> >
> > -     if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> > -             wdt->pmureg = syscon_regmap_lookup_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");
>
>
> Continuing topic from the binding: I don't see how you handle probe
> deferral, suspend ordering.

The current implementation is simply relying on exynos-pmu being
postcore_initcall level.

I was just looking around for any existing Linux APIs that could be a
more robust solution. It looks like

of_parse_phandle()
and
of_find_device_by_node();

Are often used to solve this type of probe deferral issue between
devices. Is that what you would recommend using? Or is there something
even better?

Thanks,

Peter
Krzysztof Kozlowski Jan. 23, 2024, 6:12 p.m. UTC | #5
On 23/01/2024 18:30, Peter Griffin wrote:
>>>               dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
>>>       else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>>>       if (ret)
>>>               return ret;
>>>
>>> -     if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
>>> -             wdt->pmureg = syscon_regmap_lookup_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");
>>
>>
>> Continuing topic from the binding: I don't see how you handle probe
>> deferral, suspend ordering.
> 
> The current implementation is simply relying on exynos-pmu being
> postcore_initcall level.
> 
> I was just looking around for any existing Linux APIs that could be a
> more robust solution. It looks like
> 
> of_parse_phandle()
> and
> of_find_device_by_node();
> 
> Are often used to solve this type of probe deferral issue between
> devices. Is that what you would recommend using? Or is there something
> even better?

I think you should keep the phandle and then set device link based on
of_find_device_by_node(). This would actually improve the code, because
syscon_regmap_lookup_by_phandle() does not create device links.

Best regards,
Krzysztof
Saravana Kannan Jan. 24, 2024, 3:37 a.m. UTC | #6
On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 23/01/2024 18:30, Peter Griffin wrote:
> >>>               dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> >>>       else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> >>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >>>       if (ret)
> >>>               return ret;
> >>>
> >>> -     if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> >>> -             wdt->pmureg = syscon_regmap_lookup_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");
> >>
> >>
> >> Continuing topic from the binding: I don't see how you handle probe
> >> deferral, suspend ordering.
> >
> > The current implementation is simply relying on exynos-pmu being
> > postcore_initcall level.
> >
> > I was just looking around for any existing Linux APIs that could be a
> > more robust solution. It looks like
> >
> > of_parse_phandle()
> > and
> > of_find_device_by_node();
> >
> > Are often used to solve this type of probe deferral issue between
> > devices. Is that what you would recommend using? Or is there something
> > even better?
>
> I think you should keep the phandle and then set device link based on
> of_find_device_by_node(). This would actually improve the code, because
> syscon_regmap_lookup_by_phandle() does not create device links.

I kinda agree with this. Just because we no longer use a syscon API to
find the PMU register address doesn't mean the WDT doesn't depend on
the PMU.

However, I think we should move to a generic "syscon" property. Then I
can add support for "syscon" property to fw_devlink and then things
will just work in terms of probe ordering, suspend/resume and also
showing the dependency in DT even if you don't use the syscon APIs.

Side note 1:

I think we really should officially document a generic syscon DT
property similar to how we have a generic "clocks" or "dmas" property.
Then we can have a syscon_get_regmap() that's like so:

struct regmap *syscon_get_regmap(struct device *dev)
{
        return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
}

Instead of every device defining its own bespoke DT property to do the
exact same thing. I did a quick "back of the envelope" grep on this
and I get about 143 unique properties just to get the syscon regmap.
$ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
143

Side note 2:

How are we making sure that it's the exynos-pmu driver that ends up
probing the PMU and not the generic syscon driver? Both of these are
platform drivers. And the exynos PMU device lists both the exynos
compatible string and the syscon property. Is it purely a link order
coincidence?

-Saravana
Krzysztof Kozlowski Jan. 24, 2024, 6:27 a.m. UTC | #7
On 24/01/2024 04:37, Saravana Kannan wrote:
> On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 23/01/2024 18:30, Peter Griffin wrote:
>>>>>               dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
>>>>>       else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
>>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>>>>>       if (ret)
>>>>>               return ret;
>>>>>
>>>>> -     if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
>>>>> -             wdt->pmureg = syscon_regmap_lookup_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");
>>>>
>>>>
>>>> Continuing topic from the binding: I don't see how you handle probe
>>>> deferral, suspend ordering.
>>>
>>> The current implementation is simply relying on exynos-pmu being
>>> postcore_initcall level.
>>>
>>> I was just looking around for any existing Linux APIs that could be a
>>> more robust solution. It looks like
>>>
>>> of_parse_phandle()
>>> and
>>> of_find_device_by_node();
>>>
>>> Are often used to solve this type of probe deferral issue between
>>> devices. Is that what you would recommend using? Or is there something
>>> even better?
>>
>> I think you should keep the phandle and then set device link based on
>> of_find_device_by_node(). This would actually improve the code, because
>> syscon_regmap_lookup_by_phandle() does not create device links.
> 
> I kinda agree with this. Just because we no longer use a syscon API to
> find the PMU register address doesn't mean the WDT doesn't depend on
> the PMU.
> 
> However, I think we should move to a generic "syscon" property. Then I
> can add support for "syscon" property to fw_devlink and then things
> will just work in terms of probe ordering, suspend/resume and also
> showing the dependency in DT even if you don't use the syscon APIs.
> 
> Side note 1:
> 
> I think we really should officially document a generic syscon DT
> property similar to how we have a generic "clocks" or "dmas" property.
> Then we can have a syscon_get_regmap() that's like so:
> 
> struct regmap *syscon_get_regmap(struct device *dev)
> {
>         return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> }
> 
> Instead of every device defining its own bespoke DT property to do the
> exact same thing. I did a quick "back of the envelope" grep on this
> and I get about 143 unique properties just to get the syscon regmap.
> $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
> 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
> 143

Sorry, generic "syscon" property won't fly with DT maintainers, because
there is no such thing as syscon in any of hardware.

> 
> Side note 2:
> 
> How are we making sure that it's the exynos-pmu driver that ends up
> probing the PMU and not the generic syscon driver? Both of these are
> platform drivers. And the exynos PMU device lists both the exynos
> compatible string and the syscon property. Is it purely a link order
> coincidence?

initcall ordering

Best regards,
Krzysztof
Saravana Kannan Jan. 24, 2024, 9:27 p.m. UTC | #8
On Tue, Jan 23, 2024 at 10:27 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 24/01/2024 04:37, Saravana Kannan wrote:
> > On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 23/01/2024 18:30, Peter Griffin wrote:
> >>>>>               dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> >>>>>       else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> >>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >>>>>       if (ret)
> >>>>>               return ret;
> >>>>>
> >>>>> -     if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> >>>>> -             wdt->pmureg = syscon_regmap_lookup_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");
> >>>>
> >>>>
> >>>> Continuing topic from the binding: I don't see how you handle probe
> >>>> deferral, suspend ordering.
> >>>
> >>> The current implementation is simply relying on exynos-pmu being
> >>> postcore_initcall level.
> >>>
> >>> I was just looking around for any existing Linux APIs that could be a
> >>> more robust solution. It looks like
> >>>
> >>> of_parse_phandle()
> >>> and
> >>> of_find_device_by_node();
> >>>
> >>> Are often used to solve this type of probe deferral issue between
> >>> devices. Is that what you would recommend using? Or is there something
> >>> even better?
> >>
> >> I think you should keep the phandle and then set device link based on
> >> of_find_device_by_node(). This would actually improve the code, because
> >> syscon_regmap_lookup_by_phandle() does not create device links.
> >
> > I kinda agree with this. Just because we no longer use a syscon API to
> > find the PMU register address doesn't mean the WDT doesn't depend on
> > the PMU.
> >
> > However, I think we should move to a generic "syscon" property. Then I
> > can add support for "syscon" property to fw_devlink and then things
> > will just work in terms of probe ordering, suspend/resume and also
> > showing the dependency in DT even if you don't use the syscon APIs.
> >
> > Side note 1:
> >
> > I think we really should officially document a generic syscon DT
> > property similar to how we have a generic "clocks" or "dmas" property.
> > Then we can have a syscon_get_regmap() that's like so:
> >
> > struct regmap *syscon_get_regmap(struct device *dev)
> > {
> >         return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> > }
> >
> > Instead of every device defining its own bespoke DT property to do the
> > exact same thing. I did a quick "back of the envelope" grep on this
> > and I get about 143 unique properties just to get the syscon regmap.
> > $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
> > 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
> > 143
>
> Sorry, generic "syscon" property won't fly with DT maintainers, because
> there is no such thing as syscon in any of hardware.

Then why do we allow a "syscon" compatible string and nodes? If the
"syscon" property isn't clear enough, we can make it something like
gpios and have it be <whatever>-syscon or have syscon-names property
if you want to give it a name.
143 bespoke properties all to say "here are some registers I need to
twiddle that's outside my regmap" doesn't seem great.

> >
> > Side note 2:
> >
> > How are we making sure that it's the exynos-pmu driver that ends up
> > probing the PMU and not the generic syscon driver? Both of these are
> > platform drivers. And the exynos PMU device lists both the exynos
> > compatible string and the syscon property. Is it purely a link order
> > coincidence?
>
> initcall ordering

Both these drivers usr postcore_initcall(). So it's purely because
soc/ is listed earlier in drivers/Makefile than mfd/. And as soon as
drivers are made into modules this is going to break. This is
terrible. If you want to have a modular system, this is going to throw
in a wrench.

-Saravana
Krzysztof Kozlowski Jan. 25, 2024, 7:37 a.m. UTC | #9
On 24/01/2024 22:27, Saravana Kannan wrote:
> On Tue, Jan 23, 2024 at 10:27 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 24/01/2024 04:37, Saravana Kannan wrote:
>>> On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 23/01/2024 18:30, Peter Griffin wrote:
>>>>>>>               dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
>>>>>>>       else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
>>>>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>>>>>>>       if (ret)
>>>>>>>               return ret;
>>>>>>>
>>>>>>> -     if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
>>>>>>> -             wdt->pmureg = syscon_regmap_lookup_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");
>>>>>>
>>>>>>
>>>>>> Continuing topic from the binding: I don't see how you handle probe
>>>>>> deferral, suspend ordering.
>>>>>
>>>>> The current implementation is simply relying on exynos-pmu being
>>>>> postcore_initcall level.
>>>>>
>>>>> I was just looking around for any existing Linux APIs that could be a
>>>>> more robust solution. It looks like
>>>>>
>>>>> of_parse_phandle()
>>>>> and
>>>>> of_find_device_by_node();
>>>>>
>>>>> Are often used to solve this type of probe deferral issue between
>>>>> devices. Is that what you would recommend using? Or is there something
>>>>> even better?
>>>>
>>>> I think you should keep the phandle and then set device link based on
>>>> of_find_device_by_node(). This would actually improve the code, because
>>>> syscon_regmap_lookup_by_phandle() does not create device links.
>>>
>>> I kinda agree with this. Just because we no longer use a syscon API to
>>> find the PMU register address doesn't mean the WDT doesn't depend on
>>> the PMU.
>>>
>>> However, I think we should move to a generic "syscon" property. Then I
>>> can add support for "syscon" property to fw_devlink and then things
>>> will just work in terms of probe ordering, suspend/resume and also
>>> showing the dependency in DT even if you don't use the syscon APIs.
>>>
>>> Side note 1:
>>>
>>> I think we really should officially document a generic syscon DT
>>> property similar to how we have a generic "clocks" or "dmas" property.
>>> Then we can have a syscon_get_regmap() that's like so:
>>>
>>> struct regmap *syscon_get_regmap(struct device *dev)
>>> {
>>>         return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
>>> }
>>>
>>> Instead of every device defining its own bespoke DT property to do the
>>> exact same thing. I did a quick "back of the envelope" grep on this
>>> and I get about 143 unique properties just to get the syscon regmap.
>>> $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
>>> 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
>>> 143
>>
>> Sorry, generic "syscon" property won't fly with DT maintainers, because
>> there is no such thing as syscon in any of hardware.
> 
> Then why do we allow a "syscon" compatible string and nodes? If the

To bind Linux drivers.

> "syscon" property isn't clear enough, we can make it something like
> gpios and have it be <whatever>-syscon or have syscon-names property
> if you want to give it a name.

This could work.

> 143 bespoke properties all to say "here are some registers I need to
> twiddle that's outside my regmap" doesn't seem great.

Why? 143 of these registers are not the same.

> 
>>>
>>> Side note 2:
>>>
>>> How are we making sure that it's the exynos-pmu driver that ends up
>>> probing the PMU and not the generic syscon driver? Both of these are
>>> platform drivers. And the exynos PMU device lists both the exynos
>>> compatible string and the syscon property. Is it purely a link order
>>> coincidence?
>>
>> initcall ordering
> 
> Both these drivers usr postcore_initcall(). So it's purely because
> soc/ is listed earlier in drivers/Makefile than mfd/. And as soon as

Oh... great :/.

> drivers are made into modules this is going to break. This is
> terrible. If you want to have a modular system, this is going to throw
> in a wrench.

Best regards,
Krzysztof
Lee Jones Jan. 25, 2024, 11:46 a.m. UTC | #10
On Thu, 25 Jan 2024, Krzysztof Kozlowski wrote:

> On 24/01/2024 22:27, Saravana Kannan wrote:
> > On Tue, Jan 23, 2024 at 10:27 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 24/01/2024 04:37, Saravana Kannan wrote:
> >>> On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
> >>> <krzysztof.kozlowski@linaro.org> wrote:
> >>>>
> >>>> On 23/01/2024 18:30, Peter Griffin wrote:
> >>>>>>>               dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> >>>>>>>       else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> >>>>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >>>>>>>       if (ret)
> >>>>>>>               return ret;
> >>>>>>>
> >>>>>>> -     if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> >>>>>>> -             wdt->pmureg = syscon_regmap_lookup_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");
> >>>>>>
> >>>>>>
> >>>>>> Continuing topic from the binding: I don't see how you handle probe
> >>>>>> deferral, suspend ordering.
> >>>>>
> >>>>> The current implementation is simply relying on exynos-pmu being
> >>>>> postcore_initcall level.
> >>>>>
> >>>>> I was just looking around for any existing Linux APIs that could be a
> >>>>> more robust solution. It looks like
> >>>>>
> >>>>> of_parse_phandle()
> >>>>> and
> >>>>> of_find_device_by_node();
> >>>>>
> >>>>> Are often used to solve this type of probe deferral issue between
> >>>>> devices. Is that what you would recommend using? Or is there something
> >>>>> even better?
> >>>>
> >>>> I think you should keep the phandle and then set device link based on
> >>>> of_find_device_by_node(). This would actually improve the code, because
> >>>> syscon_regmap_lookup_by_phandle() does not create device links.
> >>>
> >>> I kinda agree with this. Just because we no longer use a syscon API to
> >>> find the PMU register address doesn't mean the WDT doesn't depend on
> >>> the PMU.
> >>>
> >>> However, I think we should move to a generic "syscon" property. Then I
> >>> can add support for "syscon" property to fw_devlink and then things
> >>> will just work in terms of probe ordering, suspend/resume and also
> >>> showing the dependency in DT even if you don't use the syscon APIs.
> >>>
> >>> Side note 1:
> >>>
> >>> I think we really should officially document a generic syscon DT
> >>> property similar to how we have a generic "clocks" or "dmas" property.
> >>> Then we can have a syscon_get_regmap() that's like so:
> >>>
> >>> struct regmap *syscon_get_regmap(struct device *dev)
> >>> {
> >>>         return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> >>> }
> >>>
> >>> Instead of every device defining its own bespoke DT property to do the
> >>> exact same thing. I did a quick "back of the envelope" grep on this
> >>> and I get about 143 unique properties just to get the syscon regmap.
> >>> $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
> >>> 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
> >>> 143
> >>
> >> Sorry, generic "syscon" property won't fly with DT maintainers, because
> >> there is no such thing as syscon in any of hardware.
> > 
> > Then why do we allow a "syscon" compatible string and nodes? If the
> 
> To bind Linux drivers.
> 
> > "syscon" property isn't clear enough, we can make it something like
> > gpios and have it be <whatever>-syscon or have syscon-names property
> > if you want to give it a name.
> 
> This could work.

I'm not opposed to this idea.  The issue you'll have is keeping the
kernel backwards compatible with older DTBs, thus this solution may only
be possible for newly created bindings.  More than happy to be proven
wrong here though.

> >>> How are we making sure that it's the exynos-pmu driver that ends up
> >>> probing the PMU and not the generic syscon driver? Both of these are
> >>> platform drivers. And the exynos PMU device lists both the exynos
> >>> compatible string and the syscon property. Is it purely a link order
> >>> coincidence?
> >>
> >> initcall ordering
> > 
> > Both these drivers usr postcore_initcall(). So it's purely because
> > soc/ is listed earlier in drivers/Makefile than mfd/. And as soon as
> 
> Oh... great :/.

Agree.

Even using initcalls for ordering is fragile.  Relying on the
lexicographical order of a directory / filename structure is akin to
rolling a dice.  It would be far nicer if you are able to find a more
robust method of ensuring load order e.g. dynamically poking at
hardware and / or utilising -EPROBE_DEFER.
Peter Griffin Jan. 25, 2024, 1:19 p.m. UTC | #11
Hi Saravana,

Thanks for the feedback!

On Wed, 24 Jan 2024 at 21:27, Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Jan 23, 2024 at 10:27 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 24/01/2024 04:37, Saravana Kannan wrote:
> > > On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
> > > <krzysztof.kozlowski@linaro.org> wrote:
> > >>
> > >> On 23/01/2024 18:30, Peter Griffin wrote:
> > >>>>>               dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> > >>>>>       else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> > >>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > >>>>>       if (ret)
> > >>>>>               return ret;
> > >>>>>
> > >>>>> -     if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> > >>>>> -             wdt->pmureg = syscon_regmap_lookup_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");
> > >>>>
> > >>>>
> > >>>> Continuing topic from the binding: I don't see how you handle probe
> > >>>> deferral, suspend ordering.
> > >>>
> > >>> The current implementation is simply relying on exynos-pmu being
> > >>> postcore_initcall level.
> > >>>
> > >>> I was just looking around for any existing Linux APIs that could be a
> > >>> more robust solution. It looks like
> > >>>
> > >>> of_parse_phandle()
> > >>> and
> > >>> of_find_device_by_node();
> > >>>
> > >>> Are often used to solve this type of probe deferral issue between
> > >>> devices. Is that what you would recommend using? Or is there something
> > >>> even better?
> > >>
> > >> I think you should keep the phandle and then set device link based on
> > >> of_find_device_by_node(). This would actually improve the code, because
> > >> syscon_regmap_lookup_by_phandle() does not create device links.
> > >
> > > I kinda agree with this. Just because we no longer use a syscon API to
> > > find the PMU register address doesn't mean the WDT doesn't depend on
> > > the PMU.
> > >
> > > However, I think we should move to a generic "syscon" property. Then I
> > > can add support for "syscon" property to fw_devlink and then things
> > > will just work in terms of probe ordering, suspend/resume and also
> > > showing the dependency in DT even if you don't use the syscon APIs.
> > >
> > > Side note 1:
> > >
> > > I think we really should officially document a generic syscon DT
> > > property similar to how we have a generic "clocks" or "dmas" property.
> > > Then we can have a syscon_get_regmap() that's like so:
> > >
> > > struct regmap *syscon_get_regmap(struct device *dev)
> > > {
> > >         return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> > > }
> > >
> > > Instead of every device defining its own bespoke DT property to do the
> > > exact same thing. I did a quick "back of the envelope" grep on this
> > > and I get about 143 unique properties just to get the syscon regmap.
> > > $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
> > > 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
> > > 143
> >
> > Sorry, generic "syscon" property won't fly with DT maintainers, because
> > there is no such thing as syscon in any of hardware.
>
> Then why do we allow a "syscon" compatible string and nodes? If the
> "syscon" property isn't clear enough, we can make it something like
> gpios and have it be <whatever>-syscon or have syscon-names property
> if you want to give it a name.
> 143 bespoke properties all to say "here are some registers I need to
> twiddle that's outside my regmap" doesn't seem great.

Some sort of standardization on the naming seems like a good idea to
me. Especially if it then means fw_devlink support can be added.

>
> > >
> > > Side note 2:
> > >
> > > How are we making sure that it's the exynos-pmu driver that ends up
> > > probing the PMU and not the generic syscon driver? Both of these are
> > > platform drivers. And the exynos PMU device lists both the exynos
> > > compatible string and the syscon property. Is it purely a link order
> > > coincidence?
> >
> > initcall ordering
>
> Both these drivers usr postcore_initcall(). So it's purely because
> soc/ is listed earlier in drivers/Makefile than mfd/. And as soon as
> drivers are made into modules this is going to break. This is
> terrible. If you want to have a modular system, this is going to throw
> in a wrench.
>

That does look to be a bug, or fragility at least with the current
upstream exynos-pmu driver. I think upstream Exynos most likely hasn't
encountered these types of issues because ARCH_EXYNOS has these
drivers as built-in, and as you say the alphabetical ordering in the
Makefile.

regards,

Peter.
Saravana Kannan Jan. 26, 2024, 2:17 a.m. UTC | #12
On Wed, Jan 24, 2024 at 11:37 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 24/01/2024 22:27, Saravana Kannan wrote:
> > On Tue, Jan 23, 2024 at 10:27 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 24/01/2024 04:37, Saravana Kannan wrote:
> >>> On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
> >>> <krzysztof.kozlowski@linaro.org> wrote:
> >>>>
> >>>> On 23/01/2024 18:30, Peter Griffin wrote:
> >>>>>>>               dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> >>>>>>>       else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> >>>>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >>>>>>>       if (ret)
> >>>>>>>               return ret;
> >>>>>>>
> >>>>>>> -     if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> >>>>>>> -             wdt->pmureg = syscon_regmap_lookup_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");
> >>>>>>
> >>>>>>
> >>>>>> Continuing topic from the binding: I don't see how you handle probe
> >>>>>> deferral, suspend ordering.
> >>>>>
> >>>>> The current implementation is simply relying on exynos-pmu being
> >>>>> postcore_initcall level.
> >>>>>
> >>>>> I was just looking around for any existing Linux APIs that could be a
> >>>>> more robust solution. It looks like
> >>>>>
> >>>>> of_parse_phandle()
> >>>>> and
> >>>>> of_find_device_by_node();
> >>>>>
> >>>>> Are often used to solve this type of probe deferral issue between
> >>>>> devices. Is that what you would recommend using? Or is there something
> >>>>> even better?
> >>>>
> >>>> I think you should keep the phandle and then set device link based on
> >>>> of_find_device_by_node(). This would actually improve the code, because
> >>>> syscon_regmap_lookup_by_phandle() does not create device links.
> >>>
> >>> I kinda agree with this. Just because we no longer use a syscon API to
> >>> find the PMU register address doesn't mean the WDT doesn't depend on
> >>> the PMU.
> >>>
> >>> However, I think we should move to a generic "syscon" property. Then I
> >>> can add support for "syscon" property to fw_devlink and then things
> >>> will just work in terms of probe ordering, suspend/resume and also
> >>> showing the dependency in DT even if you don't use the syscon APIs.
> >>>
> >>> Side note 1:
> >>>
> >>> I think we really should officially document a generic syscon DT
> >>> property similar to how we have a generic "clocks" or "dmas" property.
> >>> Then we can have a syscon_get_regmap() that's like so:
> >>>
> >>> struct regmap *syscon_get_regmap(struct device *dev)
> >>> {
> >>>         return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> >>> }
> >>>
> >>> Instead of every device defining its own bespoke DT property to do the
> >>> exact same thing. I did a quick "back of the envelope" grep on this
> >>> and I get about 143 unique properties just to get the syscon regmap.
> >>> $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
> >>> 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
> >>> 143
> >>
> >> Sorry, generic "syscon" property won't fly with DT maintainers, because
> >> there is no such thing as syscon in any of hardware.
> >
> > Then why do we allow a "syscon" compatible string and nodes? If the
>
> To bind Linux drivers.

My point was that if your statement "there's no such thing as syscon
in any of hardware" is true, then we shouldn't have a syscon node
either because I don't think there's any "syscon" IP block. It's just
a random set of registers grouped together for system control.

>
> > "syscon" property isn't clear enough, we can make it something like
> > gpios and have it be <whatever>-syscon or have syscon-names property
> > if you want to give it a name.
>
> This could work.
>
> > 143 bespoke properties all to say "here are some registers I need to
> > twiddle that's outside my regmap" doesn't seem great.
>
> Why? 143 of these registers are not the same.

You could make the same argument about clocks. None of the "clocks"
listed in DT is the same clock across all devices. But the idea behind
the clocks property is that in DT we are representing that the device
needs clocks and in DT we don't really care how/what it is used for
(the driver deals with that). Similarly "syscon" is a system control
register needed by a device (the why/how is left to the driver).

-Saravana

> >
> >>>
> >>> Side note 2:
> >>>
> >>> How are we making sure that it's the exynos-pmu driver that ends up
> >>> probing the PMU and not the generic syscon driver? Both of these are
> >>> platform drivers. And the exynos PMU device lists both the exynos
> >>> compatible string and the syscon property. Is it purely a link order
> >>> coincidence?
> >>
> >> initcall ordering
> >
> > Both these drivers usr postcore_initcall(). So it's purely because
> > soc/ is listed earlier in drivers/Makefile than mfd/. And as soon as
>
> Oh... great :/.
>
> > drivers are made into modules this is going to break. This is
> > terrible. If you want to have a modular system, this is going to throw
> > in a wrench.
>
> Best regards,
> Krzysztof
>
Saravana Kannan Jan. 26, 2024, 2:23 a.m. UTC | #13
On Thu, Jan 25, 2024 at 3:46 AM Lee Jones <lee@kernel.org> wrote:
>
> On Thu, 25 Jan 2024, Krzysztof Kozlowski wrote:
>
> > On 24/01/2024 22:27, Saravana Kannan wrote:
> > > On Tue, Jan 23, 2024 at 10:27 PM Krzysztof Kozlowski
> > > <krzysztof.kozlowski@linaro.org> wrote:
> > >>
> > >> On 24/01/2024 04:37, Saravana Kannan wrote:
> > >>> On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
> > >>> <krzysztof.kozlowski@linaro.org> wrote:
> > >>>>
> > >>>> On 23/01/2024 18:30, Peter Griffin wrote:
> > >>>>>>>               dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> > >>>>>>>       else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> > >>>>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > >>>>>>>       if (ret)
> > >>>>>>>               return ret;
> > >>>>>>>
> > >>>>>>> -     if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> > >>>>>>> -             wdt->pmureg = syscon_regmap_lookup_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");
> > >>>>>>
> > >>>>>>
> > >>>>>> Continuing topic from the binding: I don't see how you handle probe
> > >>>>>> deferral, suspend ordering.
> > >>>>>
> > >>>>> The current implementation is simply relying on exynos-pmu being
> > >>>>> postcore_initcall level.
> > >>>>>
> > >>>>> I was just looking around for any existing Linux APIs that could be a
> > >>>>> more robust solution. It looks like
> > >>>>>
> > >>>>> of_parse_phandle()
> > >>>>> and
> > >>>>> of_find_device_by_node();
> > >>>>>
> > >>>>> Are often used to solve this type of probe deferral issue between
> > >>>>> devices. Is that what you would recommend using? Or is there something
> > >>>>> even better?
> > >>>>
> > >>>> I think you should keep the phandle and then set device link based on
> > >>>> of_find_device_by_node(). This would actually improve the code, because
> > >>>> syscon_regmap_lookup_by_phandle() does not create device links.
> > >>>
> > >>> I kinda agree with this. Just because we no longer use a syscon API to
> > >>> find the PMU register address doesn't mean the WDT doesn't depend on
> > >>> the PMU.
> > >>>
> > >>> However, I think we should move to a generic "syscon" property. Then I
> > >>> can add support for "syscon" property to fw_devlink and then things
> > >>> will just work in terms of probe ordering, suspend/resume and also
> > >>> showing the dependency in DT even if you don't use the syscon APIs.
> > >>>
> > >>> Side note 1:
> > >>>
> > >>> I think we really should officially document a generic syscon DT
> > >>> property similar to how we have a generic "clocks" or "dmas" property.
> > >>> Then we can have a syscon_get_regmap() that's like so:
> > >>>
> > >>> struct regmap *syscon_get_regmap(struct device *dev)
> > >>> {
> > >>>         return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> > >>> }
> > >>>
> > >>> Instead of every device defining its own bespoke DT property to do the
> > >>> exact same thing. I did a quick "back of the envelope" grep on this
> > >>> and I get about 143 unique properties just to get the syscon regmap.
> > >>> $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
> > >>> 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
> > >>> 143
> > >>
> > >> Sorry, generic "syscon" property won't fly with DT maintainers, because
> > >> there is no such thing as syscon in any of hardware.
> > >
> > > Then why do we allow a "syscon" compatible string and nodes? If the
> >
> > To bind Linux drivers.
> >
> > > "syscon" property isn't clear enough, we can make it something like
> > > gpios and have it be <whatever>-syscon or have syscon-names property
> > > if you want to give it a name.
> >
> > This could work.
>
> I'm not opposed to this idea.  The issue you'll have is keeping the
> kernel backwards compatible with older DTBs, thus this solution may only
> be possible for newly created bindings.  More than happy to be proven
> wrong here though.

You are right about backwards compatibility. Technically, we might be
able to fix up the DT at runtime (by keeping a list of those 143
property names) to maintain backward compatibility, but I'm not
suggesting that.

We can leave the existing ones as is, but we can at least use the new
property going forward to make dependencies easier to track and handle

-Saravana

>
> > >>> How are we making sure that it's the exynos-pmu driver that ends up
> > >>> probing the PMU and not the generic syscon driver? Both of these are
> > >>> platform drivers. And the exynos PMU device lists both the exynos
> > >>> compatible string and the syscon property. Is it purely a link order
> > >>> coincidence?
> > >>
> > >> initcall ordering
> > >
> > > Both these drivers usr postcore_initcall(). So it's purely because
> > > soc/ is listed earlier in drivers/Makefile than mfd/. And as soon as
> >
> > Oh... great :/.
>
> Agree.
>
> Even using initcalls for ordering is fragile.  Relying on the
> lexicographical order of a directory / filename structure is akin to
> rolling a dice.  It would be far nicer if you are able to find a more
> robust method of ensuring load order e.g. dynamically poking at
> hardware and / or utilising -EPROBE_DEFER.

Let me dig in to see if all the existing examples of listing syscon in
compatible AND have a different driver that needs to probe it always
list syscon as a secondary compatible string. In that case, we might
be able to make the syscon driver only match with the device it it's
the first entry in the compatible string.

-Saravana
Lee Jones Jan. 26, 2024, 8:43 a.m. UTC | #14
On Thu, 25 Jan 2024, Saravana Kannan wrote:

> On Thu, Jan 25, 2024 at 3:46 AM Lee Jones <lee@kernel.org> wrote:
> >
> > On Thu, 25 Jan 2024, Krzysztof Kozlowski wrote:
> >
> > > On 24/01/2024 22:27, Saravana Kannan wrote:
> > > > On Tue, Jan 23, 2024 at 10:27 PM Krzysztof Kozlowski
> > > > <krzysztof.kozlowski@linaro.org> wrote:
> > > >>
> > > >> On 24/01/2024 04:37, Saravana Kannan wrote:
> > > >>> On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
> > > >>> <krzysztof.kozlowski@linaro.org> wrote:
> > > >>>>
> > > >>>> On 23/01/2024 18:30, Peter Griffin wrote:
> > > >>>>>>>               dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> > > >>>>>>>       else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> > > >>>>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > > >>>>>>>       if (ret)
> > > >>>>>>>               return ret;
> > > >>>>>>>
> > > >>>>>>> -     if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> > > >>>>>>> -             wdt->pmureg = syscon_regmap_lookup_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");
> > > >>>>>>
> > > >>>>>>
> > > >>>>>> Continuing topic from the binding: I don't see how you handle probe
> > > >>>>>> deferral, suspend ordering.
> > > >>>>>
> > > >>>>> The current implementation is simply relying on exynos-pmu being
> > > >>>>> postcore_initcall level.
> > > >>>>>
> > > >>>>> I was just looking around for any existing Linux APIs that could be a
> > > >>>>> more robust solution. It looks like
> > > >>>>>
> > > >>>>> of_parse_phandle()
> > > >>>>> and
> > > >>>>> of_find_device_by_node();
> > > >>>>>
> > > >>>>> Are often used to solve this type of probe deferral issue between
> > > >>>>> devices. Is that what you would recommend using? Or is there something
> > > >>>>> even better?
> > > >>>>
> > > >>>> I think you should keep the phandle and then set device link based on
> > > >>>> of_find_device_by_node(). This would actually improve the code, because
> > > >>>> syscon_regmap_lookup_by_phandle() does not create device links.
> > > >>>
> > > >>> I kinda agree with this. Just because we no longer use a syscon API to
> > > >>> find the PMU register address doesn't mean the WDT doesn't depend on
> > > >>> the PMU.
> > > >>>
> > > >>> However, I think we should move to a generic "syscon" property. Then I
> > > >>> can add support for "syscon" property to fw_devlink and then things
> > > >>> will just work in terms of probe ordering, suspend/resume and also
> > > >>> showing the dependency in DT even if you don't use the syscon APIs.
> > > >>>
> > > >>> Side note 1:
> > > >>>
> > > >>> I think we really should officially document a generic syscon DT
> > > >>> property similar to how we have a generic "clocks" or "dmas" property.
> > > >>> Then we can have a syscon_get_regmap() that's like so:
> > > >>>
> > > >>> struct regmap *syscon_get_regmap(struct device *dev)
> > > >>> {
> > > >>>         return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> > > >>> }
> > > >>>
> > > >>> Instead of every device defining its own bespoke DT property to do the
> > > >>> exact same thing. I did a quick "back of the envelope" grep on this
> > > >>> and I get about 143 unique properties just to get the syscon regmap.
> > > >>> $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
> > > >>> 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
> > > >>> 143
> > > >>
> > > >> Sorry, generic "syscon" property won't fly with DT maintainers, because
> > > >> there is no such thing as syscon in any of hardware.
> > > >
> > > > Then why do we allow a "syscon" compatible string and nodes? If the
> > >
> > > To bind Linux drivers.
> > >
> > > > "syscon" property isn't clear enough, we can make it something like
> > > > gpios and have it be <whatever>-syscon or have syscon-names property
> > > > if you want to give it a name.
> > >
> > > This could work.
> >
> > I'm not opposed to this idea.  The issue you'll have is keeping the
> > kernel backwards compatible with older DTBs, thus this solution may only
> > be possible for newly created bindings.  More than happy to be proven
> > wrong here though.
> 
> You are right about backwards compatibility. Technically, we might be
> able to fix up the DT at runtime (by keeping a list of those 143
> property names) to maintain backward compatibility, but I'm not
> suggesting that.
> 
> We can leave the existing ones as is, but we can at least use the new
> property going forward to make dependencies easier to track and handle

Automatic tracking and device linking sounds like a worthwhile endeavour.

> -Saravana

I nearly stopped reading here.

> > > >>> How are we making sure that it's the exynos-pmu driver that ends up
> > > >>> probing the PMU and not the generic syscon driver? Both of these are
> > > >>> platform drivers. And the exynos PMU device lists both the exynos
> > > >>> compatible string and the syscon property. Is it purely a link order
> > > >>> coincidence?
> > > >>
> > > >> initcall ordering
> > > >
> > > > Both these drivers usr postcore_initcall(). So it's purely because
> > > > soc/ is listed earlier in drivers/Makefile than mfd/. And as soon as
> > >
> > > Oh... great :/.
> >
> > Agree.
> >
> > Even using initcalls for ordering is fragile.  Relying on the
> > lexicographical order of a directory / filename structure is akin to
> > rolling a dice.  It would be far nicer if you are able to find a more
> > robust method of ensuring load order e.g. dynamically poking at
> > hardware and / or utilising -EPROBE_DEFER.
> 
> Let me dig in to see if all the existing examples of listing syscon in
> compatible AND have a different driver that needs to probe it always
> list syscon as a secondary compatible string. In that case, we might
> be able to make the syscon driver only match with the device it it's
> the first entry in the compatible string.

If using clever or non-obvious means by which to ensure correct
ordering, I would suggest putting in place very obvious
documentation/commentary verbosely describing the aim and method.
kernel test robot Jan. 26, 2024, 5:04 p.m. UTC | #15
Hi Peter,

kernel test robot noticed the following build errors:

[auto build test ERROR on krzk/for-next]
[also build test ERROR on robh/for-next soc/for-next linus/master v6.8-rc1 next-20240125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Peter-Griffin/dt-bindings-watchdog-samsung-wdt-deprecate-samsung-syscon-phandle/20240123-070052
base:   https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux.git for-next
patch link:    https://lore.kernel.org/r/20240122225710.1952066-4-peter.griffin%40linaro.org
patch subject: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240127/202401270001.630IWRta-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240127/202401270001.630IWRta-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401270001.630IWRta-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/soc/samsung/exynos5420-pmu.c:12:10: fatal error: 'asm/cputype.h' file not found
      12 | #include <asm/cputype.h>
         |          ^~~~~~~~~~~~~~~
   1 error generated.

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for EXYNOS_PMU
   Depends on [n]: SOC_SAMSUNG [=y] && (ARCH_EXYNOS || (ARM || ARM64) && COMPILE_TEST [=y])
   Selected by [y]:
   - S3C2410_WATCHDOG [=y] && WATCHDOG [=y] && (ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST [=y])


vim +12 drivers/soc/samsung/exynos5420-pmu.c

92c4bf04735130 arch/arm/mach-exynos/exynos5420-pmu.c Pankaj Dubey 2015-12-18  11  
92c4bf04735130 arch/arm/mach-exynos/exynos5420-pmu.c Pankaj Dubey 2015-12-18 @12  #include <asm/cputype.h>
92c4bf04735130 arch/arm/mach-exynos/exynos5420-pmu.c Pankaj Dubey 2015-12-18  13
Rob Herring (Arm) Jan. 30, 2024, 8:27 p.m. UTC | #16
On Wed, Jan 24, 2024 at 01:27:01PM -0800, Saravana Kannan wrote:
> On Tue, Jan 23, 2024 at 10:27 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 24/01/2024 04:37, Saravana Kannan wrote:
> > > On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
> > > <krzysztof.kozlowski@linaro.org> wrote:
> > >>
> > >> On 23/01/2024 18:30, Peter Griffin wrote:
> > >>>>>               dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> > >>>>>       else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> > >>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > >>>>>       if (ret)
> > >>>>>               return ret;
> > >>>>>
> > >>>>> -     if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> > >>>>> -             wdt->pmureg = syscon_regmap_lookup_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");
> > >>>>
> > >>>>
> > >>>> Continuing topic from the binding: I don't see how you handle probe
> > >>>> deferral, suspend ordering.
> > >>>
> > >>> The current implementation is simply relying on exynos-pmu being
> > >>> postcore_initcall level.
> > >>>
> > >>> I was just looking around for any existing Linux APIs that could be a
> > >>> more robust solution. It looks like
> > >>>
> > >>> of_parse_phandle()
> > >>> and
> > >>> of_find_device_by_node();
> > >>>
> > >>> Are often used to solve this type of probe deferral issue between
> > >>> devices. Is that what you would recommend using? Or is there something
> > >>> even better?
> > >>
> > >> I think you should keep the phandle and then set device link based on
> > >> of_find_device_by_node(). This would actually improve the code, because
> > >> syscon_regmap_lookup_by_phandle() does not create device links.
> > >
> > > I kinda agree with this. Just because we no longer use a syscon API to
> > > find the PMU register address doesn't mean the WDT doesn't depend on
> > > the PMU.
> > >
> > > However, I think we should move to a generic "syscon" property. Then I
> > > can add support for "syscon" property to fw_devlink and then things
> > > will just work in terms of probe ordering, suspend/resume and also
> > > showing the dependency in DT even if you don't use the syscon APIs.
> > >
> > > Side note 1:
> > >
> > > I think we really should officially document a generic syscon DT
> > > property similar to how we have a generic "clocks" or "dmas" property.
> > > Then we can have a syscon_get_regmap() that's like so:

The difference is we know what to do with clocks, dma, etc. The only 
thing we know from "syscon" is it's random register bits.

> > >
> > > struct regmap *syscon_get_regmap(struct device *dev)
> > > {
> > >         return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> > > }
> > >
> > > Instead of every device defining its own bespoke DT property to do the
> > > exact same thing. I did a quick "back of the envelope" grep on this
> > > and I get about 143 unique properties just to get the syscon regmap.
> > > $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
> > > 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
> > > 143
> >
> > Sorry, generic "syscon" property won't fly with DT maintainers, because
> > there is no such thing as syscon in any of hardware.
> 
> Then why do we allow a "syscon" compatible string and nodes? If the
> "syscon" property isn't clear enough, we can make it something like
> gpios and have it be <whatever>-syscon or have syscon-names property
> if you want to give it a name.

I'm pretty hesistant to expand anything syscon related. Really, I'd like 
to get rid of "syscon" compatible. It's just a hint to create a regmap.

> 143 bespoke properties all to say "here are some registers I need to
> twiddle that's outside my regmap" doesn't seem great.

I wonder how many aren't outside of the node's main registers, but are 
the only registers. That's quite common, but that would have largely 
been before we started saying to make those a child node of the syscon.

Changing wouldn't do anything to get rid of the bespoke strings. It just 
shifts them from property names to property name prefix or -names 
string.

> 
> > >
> > > Side note 2:
> > >
> > > How are we making sure that it's the exynos-pmu driver that ends up
> > > probing the PMU and not the generic syscon driver? Both of these are
> > > platform drivers. And the exynos PMU device lists both the exynos
> > > compatible string and the syscon property. Is it purely a link order
> > > coincidence?
> >
> > initcall ordering
> 
> Both these drivers usr postcore_initcall(). So it's purely because
> soc/ is listed earlier in drivers/Makefile than mfd/. And as soon as
> drivers are made into modules this is going to break. This is
> terrible. If you want to have a modular system, this is going to throw
> in a wrench.

IMO, a "syscon" shouldn't be a module. It's just a regmap.

Rob
diff mbox series

Patch

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 7d22051b15a2..b3e90e1ddf14 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -513,6 +513,7 @@  config S3C2410_WATCHDOG
 	depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
 	select WATCHDOG_CORE
 	select MFD_SYSCON if ARCH_EXYNOS
+	select EXYNOS_PMU
 	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..fd3a9ce870a0 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -28,6 +28,8 @@ 
 #include <linux/regmap.h>
 #include <linux/delay.h>
 
+#include <linux/soc/samsung/exynos-pmu.h>
+
 #define S3C2410_WTCON		0x00
 #define S3C2410_WTDAT		0x04
 #define S3C2410_WTCNT		0x08
@@ -187,7 +189,6 @@  struct s3c2410_wdt {
 	struct watchdog_device	wdt_device;
 	struct notifier_block	freq_transition;
 	const struct s3c2410_wdt_variant *drv_data;
-	struct regmap *pmureg;
 };
 
 static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
@@ -355,8 +356,8 @@  static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
 	const u32 val = mask ? mask_val : 0;
 	int ret;
 
-	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->disable_reg,
-				 mask_val, val);
+	ret = exynos_pmu_update(wdt->drv_data->disable_reg,
+				mask_val, val);
 	if (ret < 0)
 		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
 
@@ -370,8 +371,8 @@  static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
 	const u32 val = (mask ^ val_inv) ? mask_val : 0;
 	int ret;
 
-	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg,
-				 mask_val, val);
+	ret = exynos_pmu_update(wdt->drv_data->mask_reset_reg,
+				mask_val, val);
 	if (ret < 0)
 		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
 
@@ -384,8 +385,8 @@  static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en)
 	const u32 val = en ? mask_val : 0;
 	int ret;
 
-	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->cnt_en_reg,
-				 mask_val, val);
+	ret = exynos_pmu_update(wdt->drv_data->cnt_en_reg,
+				mask_val, val);
 	if (ret < 0)
 		dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
 
@@ -617,7 +618,7 @@  static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
 	if (!(wdt->drv_data->quirks & QUIRK_HAS_PMU_RST_STAT))
 		return 0;
 
-	ret = regmap_read(wdt->pmureg, wdt->drv_data->rst_stat_reg, &rst_stat);
+	ret = exynos_pmu_read(wdt->drv_data->rst_stat_reg, &rst_stat);
 	if (ret)
 		dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
 	else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
@@ -698,14 +699,6 @@  static int s3c2410wdt_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
-		wdt->pmureg = syscon_regmap_lookup_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");
-	}
-
 	wdt_irq = platform_get_irq(pdev, 0);
 	if (wdt_irq < 0)
 		return wdt_irq;