| Message ID | 20240122225710.1952066-4-peter.griffin@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add exynos_pmu_update/read/write() APIs to exynos-pmu | expand |
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;
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
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
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
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
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
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
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
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
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.
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.
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 >
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
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.
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
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;
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(-)