| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CADrjBPor5tMY4r0jOy7GH36auCU7dWn6Qn4ct89bsSMW4vAQOA@mail.gmail.com>
Date: Tue, 23 Jan 2024 17:30:55 +0000
From: Peter Griffin <peter.griffin@...aro.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: arnd@...db.de, robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
linux@...ck-us.net, wim@...ux-watchdog.org, conor+dt@...nel.org,
alim.akhtar@...sung.com, jaewon02.kim@...sung.com, chanho61.park@...sung.com,
semen.protsenko@...aro.org, kernel-team@...roid.com, tudor.ambarus@...aro.org,
andre.draszik@...aro.org, saravanak@...gle.com, willmcvicker@...gle.com,
linux-fsd@...la.com, linux-watchdog@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new
exynos_pmu_*() apis
Hi Krzysztof,
Thanks for your review feedback.
On Tue, 23 Jan 2024 at 11:19, Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.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@...aro.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
Powered by blists - more mailing lists