| Message ID | 20240122225710.1952066-1-peter.griffin@linaro.org |
|---|---|
| Headers | show |
| Series | Add exynos_pmu_update/read/write() APIs to exynos-pmu | expand |
On Mon, Jan 22, 2024, at 23:57, Peter Griffin wrote: > samsung,syscon-phandle is no longer used by the Samsung watchdog driver > to access PMU registers. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> Can you mention the driver commit that led to the property no longer being required? Since this change would break compatibility with older kernels, I would want to make sure that at least a couple of years have passed since then to give users an upgrade path. Arnd
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: > The watchdog driver no longer requires a phandle to obtain a regmap > to the PMU registers. So mark this as deprecated. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > --- > .../devicetree/bindings/watchdog/samsung-wdt.yaml | 15 +-------------- > 1 file changed, 1 insertion(+), 14 deletions(-) > > diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml > index 77a5ddd0426e..3970d6bf8576 100644 > --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml > +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml > @@ -56,6 +56,7 @@ properties: > description: > Phandle to the PMU system controller node (in case of Exynos5250, > Exynos5420, Exynos7, Exynos850 and gs101). > + deprecated: true I don't see how your driver handles probe or suspend ordering, so I don't think this is correct approach. Handling of the watchdog requires poking PMU, thus the watchdog device node must have reference to the PMU node. Removing it from DTS makes the hardware representation incomplete, beside mentioned driver issue. Best regards, Krzysztof
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 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
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.
Hi folks, This series adds 4 new exynos_pmu_update/read/write() APIs to the exynos-pmu driver. The APIs are intended to be used by drivers that need to read and write PMU registers on Exynos based SoCs. Examples of such Exynos drivers are: * watchdog * usb phy * mipi phy * ufs phy Currently upstream most drivers that require PMU register access have a phandle to the pmu_system_controller in their dt node and obtain a regmap using the syscon_regmap_lookup_by_phandle() API. On newer Exynos SoCs this approach is insufficient, for the following reasons 1) Newer Exynos Socs have special set/clear bit hardware on PMU registers. The intention is this should be used when the register can be accessed by multiple masters. Support is added for this hw via the QUIRK_HAS_ATOMIC_BITSETHW and is implemented in exynos_pmu_set_bit_atomic() function. 2) On some SoCs the PMU registers can only be written from secure state (el3). As Linux needs to write some of these registers a SMC read/write interface is provided in the secure monitor, and the register offset is checked against an allowlist. This is done for security hardenning reasons so that normal world can't tamper with the power to root of trust IPs. Support for this is added via QUIRK_PMU_ALIVE_WRITE_SEC. This SMC interface is known to be implemented on gs101 and derivative based SoCs firmware. The above (2) is a similar mechanism to what drivers/soc/tegra/pmc.c is providing for it's SoCs where PMC registers can only be accessed from secure state on some SoCs. As an example of a consumer of these APIs I have updated the s3c2410_wdt watchdog driver to use these new exynos_pmu_*() APIs. As the samsung,syscon-phandle logic has been removed from the s3c2410_wdt.c driver I have also updated the bindings documentation to mark this as deprecated, and updated all existing platforms DT to remove samsung,syscon-phandle from the Watchdog DT node. Please let me know if there is alternative process I should follow with regards to to deprecating this and updating existing platforms. This series has been tested on Pixel 6 / gs101. If the various maintainers/ contributors of fsd, exynos850, exynosautov9 etc can test these patches on your respective systems that would also be most appreciated! The expectation is this series would be merged via Krzysztofs Samsung Exynos tree. Many thanks, Peter. Peter Griffin (9): dt-bindings: watchdog: samsung-wdt: deprecate samsung,syscon-phandle soc: samsung: exynos-pmu: Add exynos_pmu_update/read/write APIs and SoC quirks watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis arm64: dts: fsd: remove deprecated samsung,syscon-phandle arm64: dts: exynosautov9: remove deprecated samsung,syscon-phandle arm64: dts: exynos850: remove deprecated samsung,syscon-phandle arm64: dts: exynos7: remove deprecated samsung,syscon-phandle ARM: dts: samsung: exynos4: remove deprecated samsung,syscon-phandle ARM: dts: exynos5250: remove deprecated samsung,syscon-phandle .../bindings/watchdog/samsung-wdt.yaml | 15 +- arch/arm/boot/dts/samsung/exynos4x12.dtsi | 1 - arch/arm/boot/dts/samsung/exynos5250.dtsi | 1 - arch/arm64/boot/dts/exynos/exynos7.dtsi | 1 - arch/arm64/boot/dts/exynos/exynos850.dtsi | 2 - arch/arm64/boot/dts/exynos/exynosautov9.dtsi | 2 - arch/arm64/boot/dts/tesla/fsd.dtsi | 3 - drivers/soc/samsung/exynos-pmu.c | 209 +++++++++++++++++- drivers/soc/samsung/exynos-pmu.h | 4 + drivers/watchdog/Kconfig | 1 + drivers/watchdog/s3c2410_wdt.c | 25 +-- include/linux/soc/samsung/exynos-pmu.h | 28 +++ 12 files changed, 245 insertions(+), 47 deletions(-)