Hi folks,
This is a v4 of the series to add support for protected PMU registers found
on gs101 and derivative SoCs. In v2 and later it was re-worked to be based
on a regmap abstraction that I think leads to a much neater overall
solution.
The advantage of the regmap abstraction is that most leaf drivers that
read/write PMU registers need minimal changes.
Example of Exynos drivers that require PMU register access are:
* watchdog
* usb phy
* mipi phy
* ufs phy
This series has been tested on Pixel 6 / gs101. If the various maintainers/
contributors of other Exynos SoCs like exynos850, exynosautov9, exynos5422
etc can test these patches on your respective systems that would be most
appreciated.
The expectation is this series would be merged via Krzysztofs Samsung
Exynos tree.
regards,
Peter
Changes since v3:
- Fix PMUALIVE_MASK
- Add TENSOR_ prefix
- clear SET_BITS bits on each loop iteration
- change set_bit to set_bits func name
- Fix some alignment
- Add missing return on dev_err_probe
- Reduce indentation in loop
Changes since v2
- Add select REGMAP to Kconfig
- Add constant for SET/CLEAR bits
- Replace kerneldoc with one line comment
- Fix kerneldoc for EXPORT_SYMBOL_GPL funcs
- remove superfluous extern keyword
- dev_err_probe() on probe error
- shorten regmcfg name
- no compatibles inside probe, use match data
- don't mix declarations with/without initializations
- tensor_sec_reg_read() use mmio to avoid access restrictions
- Collect up Reviewed-by
- const for regmap_config structs
Changes since v1:
- Refactor to use custom regmap to abstract SMC register access
(Sam / Guenter)
- Add deferred probing support (Saravana / Krzysztof)
v3 lore: https://lore.kernel.org/linux-arm-kernel/[email protected]/
v2 lore: https://lore.kernel.org/lkml/[email protected]/
v1 lore: https://lore.kernel.org/all/[email protected]/
Peter Griffin (2):
soc: samsung: exynos-pmu: Add regmap support for SoCs that protect PMU
regs
watchdog: s3c2410_wdt: use exynos_get_pmu_regmap_by_phandle() for PMU
regs
drivers/soc/samsung/Kconfig | 1 +
drivers/soc/samsung/exynos-pmu.c | 235 ++++++++++++++++++++++++-
drivers/soc/samsung/exynos-pmu.h | 1 +
drivers/watchdog/Kconfig | 1 -
drivers/watchdog/s3c2410_wdt.c | 8 +-
include/linux/soc/samsung/exynos-pmu.h | 11 +-
6 files changed, 247 insertions(+), 10 deletions(-)
--
2.43.0.594.gd9cf4e227d-goog
Obtain the PMU regmap using the new API added to exynos-pmu driver rather
than syscon_regmap_lookup_by_phandle(). As this driver no longer depends
on mfd syscon remove that header and Kconfig dependency.
Reviewed-by: Sam Protsenko <[email protected]>
Signed-off-by: Peter Griffin <[email protected]>
---
drivers/watchdog/Kconfig | 1 -
drivers/watchdog/s3c2410_wdt.c | 8 ++++----
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 7d22051b15a2..d78fe7137799 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -512,7 +512,6 @@ config S3C2410_WATCHDOG
tristate "S3C6410/S5Pv210/Exynos Watchdog"
depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
select WATCHDOG_CORE
- select MFD_SYSCON if ARCH_EXYNOS
help
Watchdog timer block in the Samsung S3C64xx, S5Pv210 and Exynos
SoCs. This will reboot the system when the timer expires with
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 349d30462c8c..686cf544d0ae 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -24,9 +24,9 @@
#include <linux/slab.h>
#include <linux/err.h>
#include <linux/of.h>
-#include <linux/mfd/syscon.h>
#include <linux/regmap.h>
#include <linux/delay.h>
+#include <linux/soc/samsung/exynos-pmu.h>
#define S3C2410_WTCON 0x00
#define S3C2410_WTDAT 0x04
@@ -699,11 +699,11 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
return ret;
if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
- wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
- "samsung,syscon-phandle");
+ wdt->pmureg = exynos_get_pmu_regmap_by_phandle(dev->of_node,
+ "samsung,syscon-phandle");
if (IS_ERR(wdt->pmureg))
return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
- "syscon regmap lookup failed.\n");
+ "PMU regmap lookup failed.\n");
}
wdt_irq = platform_get_irq(pdev, 0);
--
2.43.0.594.gd9cf4e227d-goog
Some Exynos based SoCs like Tensor gs101 protect the PMU registers for
security hardening reasons so that they are only write accessible in el3
via an SMC call.
As most Exynos drivers that need to write PMU registers currently obtain a
regmap via syscon (phys, pinctrl, watchdog). Support for the above usecase
is implemented in this driver using a custom regmap similar to syscon to
handle the SMC call. Platforms that don't secure PMU registers, get a mmio
regmap like before. As regmaps abstract out the underlying register access
changes to the leaf drivers are minimal.
A new API exynos_get_pmu_regmap_by_phandle() is provided for leaf drivers
that currently use syscon_regmap_lookup_by_phandle(). This also handles
deferred probing.
Signed-off-by: Peter Griffin <[email protected]>
---
Changes since v3:
- Fix PMUALIVE_MASK
- Add TENSOR_ prefix
- clear SET_BITS bits on each loop iteration
- change set_bit to set_bits in func name
- Fix some alignment
- Add missing return on dev_err_probe
- Reduce indentation in loop
Changes since v2
- Add select REGMAP to Kconfig
- Add constant for SET/CLEAR bits
- Replace kerneldoc with one line comment
- Fix kerneldoc for EXPORT_SYMBOL_GPL funcs
- remove superfluous extern keyword
- dev_err_probe() on probe error
- shorten regmcfg name
- no compatibles inside probe, use match data
- don't mix declarations with/without initializations
- tensor_sec_reg_read() use mmio to avoid access restrictions
- Collect up Reviewed-by
- const for regmap_config structs
---
drivers/soc/samsung/Kconfig | 1 +
drivers/soc/samsung/exynos-pmu.c | 235 ++++++++++++++++++++++++-
drivers/soc/samsung/exynos-pmu.h | 1 +
include/linux/soc/samsung/exynos-pmu.h | 11 +-
4 files changed, 243 insertions(+), 5 deletions(-)
diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
index 27ec99af77e3..1a5dfdc978dc 100644
--- a/drivers/soc/samsung/Kconfig
+++ b/drivers/soc/samsung/Kconfig
@@ -42,6 +42,7 @@ config EXYNOS_PMU
depends on ARCH_EXYNOS || ((ARM || ARM64) && COMPILE_TEST)
select EXYNOS_PMU_ARM_DRIVERS if ARM && ARCH_EXYNOS
select MFD_CORE
+ select REGMAP_MMIO
# There is no need to enable these drivers for ARMv8
config EXYNOS_PMU_ARM_DRIVERS
diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
index 250537d7cfd6..b846e343fcdd 100644
--- a/drivers/soc/samsung/exynos-pmu.c
+++ b/drivers/soc/samsung/exynos-pmu.c
@@ -5,6 +5,7 @@
//
// Exynos - CPU PMU(Power Management Unit) support
+#include <linux/arm-smccc.h>
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/mfd/core.h>
@@ -12,19 +13,132 @@
#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <linux/delay.h>
+#include <linux/regmap.h>
#include <linux/soc/samsung/exynos-regs-pmu.h>
#include <linux/soc/samsung/exynos-pmu.h>
#include "exynos-pmu.h"
+#define PMUALIVE_MASK GENMASK(13, 0)
+#define TENSOR_SET_BITS (BIT(15) | BIT(14))
+#define TENSOR_CLR_BITS BIT(15)
+#define TENSOR_SMC_PMU_SEC_REG 0x82000504
+#define TENSOR_PMUREG_READ 0
+#define TENSOR_PMUREG_WRITE 1
+#define TENSOR_PMUREG_RMW 2
+
struct exynos_pmu_context {
struct device *dev;
const struct exynos_pmu_data *pmu_data;
+ struct regmap *pmureg;
};
void __iomem *pmu_base_addr;
static struct exynos_pmu_context *pmu_context;
+/* forward declaration */
+static struct platform_driver exynos_pmu_driver;
+
+/*
+ * Tensor SoCs are configured so that PMU_ALIVE registers can only be written
+ * from EL3, but are still read accessible. As Linux needs to write some of
+ * these registers, the following functions are provided and exposed via
+ * regmap.
+ *
+ * Note: This SMC interface is known to be implemented on gs101 and derivative
+ * SoCs.
+ */
+
+/* Write to a protected PMU register. */
+static int tensor_sec_reg_write(void *base, unsigned int reg, unsigned int val)
+{
+ struct arm_smccc_res res;
+ unsigned long pmu_base = (unsigned long)base;
+
+ arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG, pmu_base + reg,
+ TENSOR_PMUREG_WRITE, val, 0, 0, 0, 0, &res);
+
+ /* returns -EINVAL if access isn't allowed or 0 */
+ if (res.a0)
+ pr_warn("%s(): SMC failed: %d\n", __func__, (int)res.a0);
+
+ return (int)res.a0;
+}
+
+/* Read/Modify/Write a protected PMU register. */
+static int tensor_sec_reg_rmw(void *base, unsigned int reg,
+ unsigned int mask, unsigned int val)
+{
+ struct arm_smccc_res res;
+ unsigned long pmu_base = (unsigned long)base;
+
+ arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG, pmu_base + reg,
+ TENSOR_PMUREG_RMW, mask, val, 0, 0, 0, &res);
+
+ /* returns -EINVAL if access isn't allowed or 0 */
+ if (res.a0)
+ pr_warn("%s(): SMC failed: %d\n", __func__, (int)res.a0);
+
+ return (int)res.a0;
+}
+
+/*
+ * Read a protected PMU register. All PMU registers can be read by Linux.
+ * Note: The SMC read register is not used, as only registers that can be
+ * written are readable via SMC.
+ */
+static int tensor_sec_reg_read(void *base, unsigned int reg, unsigned int *val)
+{
+ *val = pmu_raw_readl(reg);
+ return 0;
+}
+
+/*
+ * For SoCs that have set/clear bit hardware this function can be used when
+ * the PMU register will be accessed by multiple masters.
+ *
+ * For example, to set bits 13:8 in PMU reg offset 0x3e80
+ * tensor_set_bits_atomic(ctx, 0x3e80, 0x3f00, 0x3f00);
+ *
+ * Set bit 8, and clear bits 13:9 PMU reg offset 0x3e80
+ * tensor_set_bits_atomic(0x3e80, 0x100, 0x3f00);
+ */
+static inline int tensor_set_bits_atomic(void *ctx, unsigned int offset,
+ u32 val, u32 mask)
+{
+ int ret;
+ unsigned int i;
+
+ for (i = 0; i < 32; i++) {
+ if (!(mask & BIT(i)))
+ continue;
+
+ offset &= ~TENSOR_SET_BITS;
+
+ if (val & BIT(i))
+ offset |= TENSOR_SET_BITS;
+ else
+ offset |= TENSOR_CLR_BITS;
+
+ ret = tensor_sec_reg_write(ctx, offset, i);
+ if (ret)
+ return ret;
+ }
+ return ret;
+}
+
+static int tensor_sec_update_bits(void *ctx, unsigned int reg,
+ unsigned int mask, unsigned int val)
+{
+ /*
+ * Use atomic operations for PMU_ALIVE registers (offset 0~0x3FFF)
+ * as the target registers can be accessed by multiple masters.
+ */
+ if (reg > PMUALIVE_MASK)
+ return tensor_sec_reg_rmw(ctx, reg, mask, val);
+
+ return tensor_set_bits_atomic(ctx, reg, val, mask);
+}
void pmu_raw_writel(u32 val, u32 offset)
{
@@ -75,11 +189,41 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode)
#define exynos_pmu_data_arm_ptr(data) NULL
#endif
+static const struct regmap_config regmap_smccfg = {
+ .name = "pmu_regs",
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .fast_io = true,
+ .use_single_read = true,
+ .use_single_write = true,
+ .reg_read = tensor_sec_reg_read,
+ .reg_write = tensor_sec_reg_write,
+ .reg_update_bits = tensor_sec_update_bits,
+};
+
+static const struct regmap_config regmap_mmiocfg = {
+ .name = "pmu_regs",
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .fast_io = true,
+ .use_single_read = true,
+ .use_single_write = true,
+};
+
+static const struct exynos_pmu_data gs101_pmu_data = {
+ .pmu_secure = true
+};
+
/*
* PMU platform driver and devicetree bindings.
*/
static const struct of_device_id exynos_pmu_of_device_ids[] = {
{
+ .compatible = "google,gs101-pmu",
+ .data = &gs101_pmu_data,
+ }, {
.compatible = "samsung,exynos3250-pmu",
.data = exynos_pmu_data_arm_ptr(exynos3250_pmu_data),
}, {
@@ -113,19 +257,73 @@ static const struct mfd_cell exynos_pmu_devs[] = {
{ .name = "exynos-clkout", },
};
+/**
+ * exynos_get_pmu_regmap() - Obtain pmureg regmap
+ *
+ * Find the pmureg regmap previously configured in probe() and return regmap
+ * pointer.
+ *
+ * Return: A pointer to regmap if found or ERR_PTR error value.
+ */
struct regmap *exynos_get_pmu_regmap(void)
{
struct device_node *np = of_find_matching_node(NULL,
exynos_pmu_of_device_ids);
if (np)
- return syscon_node_to_regmap(np);
+ return exynos_get_pmu_regmap_by_phandle(np, NULL);
return ERR_PTR(-ENODEV);
}
EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap);
+/**
+ * exynos_get_pmu_regmap_by_phandle() - Obtain pmureg regmap via phandle
+ * @np: Pointer to device's Device Tree node
+ * @property: Device Tree property name which references the pmu
+ *
+ * Find the pmureg regmap previously configured in probe() and return regmap
+ * pointer.
+ *
+ * Return: A pointer to regmap if found or ERR_PTR error value.
+ */
+struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
+ const char *property)
+{
+ struct device *dev;
+ struct exynos_pmu_context *ctx;
+ struct device_node *pmu_np;
+
+ if (property)
+ pmu_np = of_parse_phandle(np, property, 0);
+ else
+ pmu_np = np;
+
+ if (!pmu_np)
+ return ERR_PTR(-ENODEV);
+
+ /*
+ * Determine if exynos-pmu device has probed and therefore regmap
+ * has been created and can be returned to the caller. Otherwise we
+ * return -EPROBE_DEFER.
+ */
+ dev = driver_find_device_by_of_node(&exynos_pmu_driver.driver,
+ (void *)pmu_np);
+
+ of_node_put(pmu_np);
+ if (!dev)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ ctx = dev_get_drvdata(dev);
+
+ return ctx->pmureg;
+}
+EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
+
static int exynos_pmu_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
+ struct regmap_config pmu_regmcfg;
+ struct regmap *regmap;
+ struct resource *res;
int ret;
pmu_base_addr = devm_platform_ioremap_resource(pdev, 0);
@@ -133,13 +331,42 @@ static int exynos_pmu_probe(struct platform_device *pdev)
return PTR_ERR(pmu_base_addr);
pmu_context = devm_kzalloc(&pdev->dev,
- sizeof(struct exynos_pmu_context),
- GFP_KERNEL);
+ sizeof(struct exynos_pmu_context),
+ GFP_KERNEL);
if (!pmu_context)
return -ENOMEM;
- pmu_context->dev = dev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -ENODEV;
+
pmu_context->pmu_data = of_device_get_match_data(dev);
+ /* For SoCs that secure PMU register writes use custom regmap */
+ if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_secure) {
+ pmu_regmcfg = regmap_smccfg;
+ pmu_regmcfg.max_register = resource_size(res) -
+ pmu_regmcfg.reg_stride;
+ /* Need physical address for SMC call */
+ regmap = devm_regmap_init(dev, NULL,
+ (void *)(uintptr_t)res->start,
+ &pmu_regmcfg);
+ } else {
+ /* All other SoCs use a MMIO regmap */
+ pmu_regmcfg = regmap_mmiocfg;
+ pmu_regmcfg.max_register = resource_size(res) -
+ pmu_regmcfg.reg_stride;
+ regmap = devm_regmap_init_mmio(dev, pmu_base_addr,
+ &pmu_regmcfg);
+ }
+
+ if (IS_ERR(regmap))
+ return dev_err_probe(&pdev->dev, PTR_ERR(regmap),
+ "regmap init failed\n");
+
+ pmu_context->pmureg = regmap;
+ pmu_context->dev = dev;
+
if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_init)
pmu_context->pmu_data->pmu_init();
diff --git a/drivers/soc/samsung/exynos-pmu.h b/drivers/soc/samsung/exynos-pmu.h
index 1c652ffd79b4..0a49a2c9a08e 100644
--- a/drivers/soc/samsung/exynos-pmu.h
+++ b/drivers/soc/samsung/exynos-pmu.h
@@ -21,6 +21,7 @@ struct exynos_pmu_conf {
struct exynos_pmu_data {
const struct exynos_pmu_conf *pmu_config;
const struct exynos_pmu_conf *pmu_config_extra;
+ bool pmu_secure;
void (*pmu_init)(void);
void (*powerdown_conf)(enum sys_powerdown);
diff --git a/include/linux/soc/samsung/exynos-pmu.h b/include/linux/soc/samsung/exynos-pmu.h
index a4f5516cc956..e1c86640f6f7 100644
--- a/include/linux/soc/samsung/exynos-pmu.h
+++ b/include/linux/soc/samsung/exynos-pmu.h
@@ -10,6 +10,7 @@
#define __LINUX_SOC_EXYNOS_PMU_H
struct regmap;
+struct device_node;
enum sys_powerdown {
SYS_AFTR,
@@ -20,12 +21,20 @@ enum sys_powerdown {
extern void exynos_sys_powerdown_conf(enum sys_powerdown mode);
#ifdef CONFIG_EXYNOS_PMU
-extern struct regmap *exynos_get_pmu_regmap(void);
+struct regmap *exynos_get_pmu_regmap(void);
+struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
+ const char *property);
#else
static inline struct regmap *exynos_get_pmu_regmap(void)
{
return ERR_PTR(-ENODEV);
}
+
+static inline struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
+ const char *property)
+{
+ return ERR_PTR(-ENODEV);
+}
#endif
#endif /* __LINUX_SOC_EXYNOS_PMU_H */
--
2.43.0.594.gd9cf4e227d-goog
On 2/8/24 08:17, Peter Griffin wrote:
> Obtain the PMU regmap using the new API added to exynos-pmu driver rather
> than syscon_regmap_lookup_by_phandle(). As this driver no longer depends
> on mfd syscon remove that header and Kconfig dependency.
>
> Reviewed-by: Sam Protsenko <[email protected]>
> Signed-off-by: Peter Griffin <[email protected]>
Acked-by: Guenter Roeck <[email protected]>
> ---
> drivers/watchdog/Kconfig | 1 -
> drivers/watchdog/s3c2410_wdt.c | 8 ++++----
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7d22051b15a2..d78fe7137799 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -512,7 +512,6 @@ config S3C2410_WATCHDOG
> tristate "S3C6410/S5Pv210/Exynos Watchdog"
> depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
> select WATCHDOG_CORE
> - select MFD_SYSCON if ARCH_EXYNOS
> help
> Watchdog timer block in the Samsung S3C64xx, S5Pv210 and Exynos
> SoCs. This will reboot the system when the timer expires with
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 349d30462c8c..686cf544d0ae 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -24,9 +24,9 @@
> #include <linux/slab.h>
> #include <linux/err.h>
> #include <linux/of.h>
> -#include <linux/mfd/syscon.h>
> #include <linux/regmap.h>
> #include <linux/delay.h>
> +#include <linux/soc/samsung/exynos-pmu.h>
>
> #define S3C2410_WTCON 0x00
> #define S3C2410_WTDAT 0x04
> @@ -699,11 +699,11 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> return ret;
>
> if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> - wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> - "samsung,syscon-phandle");
> + wdt->pmureg = exynos_get_pmu_regmap_by_phandle(dev->of_node,
> + "samsung,syscon-phandle");
> if (IS_ERR(wdt->pmureg))
> return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
> - "syscon regmap lookup failed.\n");
> + "PMU regmap lookup failed.\n");
> }
>
> wdt_irq = platform_get_irq(pdev, 0);
On Thu, Feb 8, 2024 at 10:21 AM Peter Griffin <peter.griffin@linaroorg> wrote:
>
> Some Exynos based SoCs like Tensor gs101 protect the PMU registers for
> security hardening reasons so that they are only write accessible in el3
> via an SMC call.
>
> As most Exynos drivers that need to write PMU registers currently obtain a
> regmap via syscon (phys, pinctrl, watchdog). Support for the above usecase
> is implemented in this driver using a custom regmap similar to syscon to
> handle the SMC call. Platforms that don't secure PMU registers, get a mmio
> regmap like before. As regmaps abstract out the underlying register access
> changes to the leaf drivers are minimal.
>
> A new API exynos_get_pmu_regmap_by_phandle() is provided for leaf drivers
> that currently use syscon_regmap_lookup_by_phandle(). This also handles
> deferred probing.
>
> Signed-off-by: Peter Griffin <[email protected]>
> ---
Reviewed-by: Sam Protsenko <[email protected]>
I'll test it on E850-96 shortly.
> Changes since v3:
> - Fix PMUALIVE_MASK
> - Add TENSOR_ prefix
> - clear SET_BITS bits on each loop iteration
> - change set_bit to set_bits in func name
> - Fix some alignment
> - Add missing return on dev_err_probe
> - Reduce indentation in loop
>
> Changes since v2
> - Add select REGMAP to Kconfig
> - Add constant for SET/CLEAR bits
> - Replace kerneldoc with one line comment
> - Fix kerneldoc for EXPORT_SYMBOL_GPL funcs
> - remove superfluous extern keyword
> - dev_err_probe() on probe error
> - shorten regmcfg name
> - no compatibles inside probe, use match data
> - don't mix declarations with/without initializations
> - tensor_sec_reg_read() use mmio to avoid access restrictions
> - Collect up Reviewed-by
> - const for regmap_config structs
> ---
> drivers/soc/samsung/Kconfig | 1 +
> drivers/soc/samsung/exynos-pmu.c | 235 ++++++++++++++++++++++++-
> drivers/soc/samsung/exynos-pmu.h | 1 +
> include/linux/soc/samsung/exynos-pmu.h | 11 +-
> 4 files changed, 243 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
> index 27ec99af77e3..1a5dfdc978dc 100644
> --- a/drivers/soc/samsung/Kconfig
> +++ b/drivers/soc/samsung/Kconfig
> @@ -42,6 +42,7 @@ config EXYNOS_PMU
> depends on ARCH_EXYNOS || ((ARM || ARM64) && COMPILE_TEST)
> select EXYNOS_PMU_ARM_DRIVERS if ARM && ARCH_EXYNOS
> select MFD_CORE
> + select REGMAP_MMIO
>
> # There is no need to enable these drivers for ARMv8
> config EXYNOS_PMU_ARM_DRIVERS
> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> index 250537d7cfd6..b846e343fcdd 100644
> --- a/drivers/soc/samsung/exynos-pmu.c
> +++ b/drivers/soc/samsung/exynos-pmu.c
> @@ -5,6 +5,7 @@
> //
> // Exynos - CPU PMU(Power Management Unit) support
>
> +#include <linux/arm-smccc.h>
> #include <linux/of.h>
> #include <linux/of_address.h>
> #include <linux/mfd/core.h>
> @@ -12,19 +13,132 @@
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> #include <linux/delay.h>
> +#include <linux/regmap.h>
>
> #include <linux/soc/samsung/exynos-regs-pmu.h>
> #include <linux/soc/samsung/exynos-pmu.h>
>
> #include "exynos-pmu.h"
>
> +#define PMUALIVE_MASK GENMASK(13, 0)
> +#define TENSOR_SET_BITS (BIT(15) | BIT(14))
> +#define TENSOR_CLR_BITS BIT(15)
> +#define TENSOR_SMC_PMU_SEC_REG 0x82000504
> +#define TENSOR_PMUREG_READ 0
> +#define TENSOR_PMUREG_WRITE 1
> +#define TENSOR_PMUREG_RMW 2
> +
> struct exynos_pmu_context {
> struct device *dev;
> const struct exynos_pmu_data *pmu_data;
> + struct regmap *pmureg;
> };
>
> void __iomem *pmu_base_addr;
> static struct exynos_pmu_context *pmu_context;
> +/* forward declaration */
> +static struct platform_driver exynos_pmu_driver;
> +
> +/*
> + * Tensor SoCs are configured so that PMU_ALIVE registers can only be written
> + * from EL3, but are still read accessible. As Linux needs to write some of
> + * these registers, the following functions are provided and exposed via
> + * regmap.
> + *
> + * Note: This SMC interface is known to be implemented on gs101 and derivative
> + * SoCs.
> + */
> +
> +/* Write to a protected PMU register. */
> +static int tensor_sec_reg_write(void *base, unsigned int reg, unsigned int val)
> +{
> + struct arm_smccc_res res;
> + unsigned long pmu_base = (unsigned long)base;
> +
> + arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG, pmu_base + reg,
> + TENSOR_PMUREG_WRITE, val, 0, 0, 0, 0, &res);
> +
> + /* returns -EINVAL if access isn't allowed or 0 */
> + if (res.a0)
> + pr_warn("%s(): SMC failed: %d\n", __func__, (int)res.a0);
> +
> + return (int)res.a0;
> +}
> +
> +/* Read/Modify/Write a protected PMU register. */
> +static int tensor_sec_reg_rmw(void *base, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + struct arm_smccc_res res;
> + unsigned long pmu_base = (unsigned long)base;
> +
> + arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG, pmu_base + reg,
> + TENSOR_PMUREG_RMW, mask, val, 0, 0, 0, &res);
> +
> + /* returns -EINVAL if access isn't allowed or 0 */
> + if (res.a0)
> + pr_warn("%s(): SMC failed: %d\n", __func__, (int)res.a0);
> +
> + return (int)res.a0;
> +}
> +
> +/*
> + * Read a protected PMU register. All PMU registers can be read by Linux.
> + * Note: The SMC read register is not used, as only registers that can be
> + * written are readable via SMC.
> + */
> +static int tensor_sec_reg_read(void *base, unsigned int reg, unsigned int *val)
> +{
> + *val = pmu_raw_readl(reg);
> + return 0;
> +}
> +
> +/*
> + * For SoCs that have set/clear bit hardware this function can be used when
> + * the PMU register will be accessed by multiple masters.
> + *
> + * For example, to set bits 13:8 in PMU reg offset 0x3e80
> + * tensor_set_bits_atomic(ctx, 0x3e80, 0x3f00, 0x3f00);
> + *
> + * Set bit 8, and clear bits 13:9 PMU reg offset 0x3e80
> + * tensor_set_bits_atomic(0x3e80, 0x100, 0x3f00);
> + */
> +static inline int tensor_set_bits_atomic(void *ctx, unsigned int offset,
> + u32 val, u32 mask)
> +{
> + int ret;
> + unsigned int i;
> +
> + for (i = 0; i < 32; i++) {
> + if (!(mask & BIT(i)))
> + continue;
> +
> + offset &= ~TENSOR_SET_BITS;
> +
> + if (val & BIT(i))
> + offset |= TENSOR_SET_BITS;
> + else
> + offset |= TENSOR_CLR_BITS;
> +
> + ret = tensor_sec_reg_write(ctx, offset, i);
> + if (ret)
> + return ret;
> + }
> + return ret;
> +}
> +
> +static int tensor_sec_update_bits(void *ctx, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + /*
> + * Use atomic operations for PMU_ALIVE registers (offset 0~0x3FFF)
> + * as the target registers can be accessed by multiple masters.
> + */
> + if (reg > PMUALIVE_MASK)
> + return tensor_sec_reg_rmw(ctx, reg, mask, val);
> +
> + return tensor_set_bits_atomic(ctx, reg, val, mask);
> +}
>
> void pmu_raw_writel(u32 val, u32 offset)
> {
> @@ -75,11 +189,41 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode)
> #define exynos_pmu_data_arm_ptr(data) NULL
> #endif
>
> +static const struct regmap_config regmap_smccfg = {
> + .name = "pmu_regs",
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .fast_io = true,
> + .use_single_read = true,
> + .use_single_write = true,
> + .reg_read = tensor_sec_reg_read,
> + .reg_write = tensor_sec_reg_write,
> + .reg_update_bits = tensor_sec_update_bits,
> +};
> +
> +static const struct regmap_config regmap_mmiocfg = {
> + .name = "pmu_regs",
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .fast_io = true,
> + .use_single_read = true,
> + .use_single_write = true,
> +};
> +
> +static const struct exynos_pmu_data gs101_pmu_data = {
> + .pmu_secure = true
> +};
> +
> /*
> * PMU platform driver and devicetree bindings.
> */
> static const struct of_device_id exynos_pmu_of_device_ids[] = {
> {
> + .compatible = "google,gs101-pmu",
> + .data = &gs101_pmu_data,
> + }, {
> .compatible = "samsung,exynos3250-pmu",
> .data = exynos_pmu_data_arm_ptr(exynos3250_pmu_data),
> }, {
> @@ -113,19 +257,73 @@ static const struct mfd_cell exynos_pmu_devs[] = {
> { .name = "exynos-clkout", },
> };
>
> +/**
> + * exynos_get_pmu_regmap() - Obtain pmureg regmap
> + *
> + * Find the pmureg regmap previously configured in probe() and return regmap
> + * pointer.
> + *
> + * Return: A pointer to regmap if found or ERR_PTR error value.
> + */
> struct regmap *exynos_get_pmu_regmap(void)
> {
> struct device_node *np = of_find_matching_node(NULL,
> exynos_pmu_of_device_ids);
> if (np)
> - return syscon_node_to_regmap(np);
> + return exynos_get_pmu_regmap_by_phandle(np, NULL);
> return ERR_PTR(-ENODEV);
> }
> EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap);
>
> +/**
> + * exynos_get_pmu_regmap_by_phandle() - Obtain pmureg regmap via phandle
> + * @np: Pointer to device's Device Tree node
> + * @property: Device Tree property name which references the pmu
> + *
> + * Find the pmureg regmap previously configured in probe() and return regmap
> + * pointer.
> + *
> + * Return: A pointer to regmap if found or ERR_PTR error value.
> + */
> +struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
> + const char *property)
> +{
> + struct device *dev;
> + struct exynos_pmu_context *ctx;
> + struct device_node *pmu_np;
> +
> + if (property)
> + pmu_np = of_parse_phandle(np, property, 0);
> + else
> + pmu_np = np;
> +
> + if (!pmu_np)
> + return ERR_PTR(-ENODEV);
> +
> + /*
> + * Determine if exynos-pmu device has probed and therefore regmap
> + * has been created and can be returned to the caller. Otherwise we
> + * return -EPROBE_DEFER.
> + */
> + dev = driver_find_device_by_of_node(&exynos_pmu_driver.driver,
> + (void *)pmu_np);
> +
> + of_node_put(pmu_np);
> + if (!dev)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + ctx = dev_get_drvdata(dev);
> +
> + return ctx->pmureg;
> +}
> +EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
> +
> static int exynos_pmu_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> + struct regmap_config pmu_regmcfg;
> + struct regmap *regmap;
> + struct resource *res;
> int ret;
>
> pmu_base_addr = devm_platform_ioremap_resource(pdev, 0);
> @@ -133,13 +331,42 @@ static int exynos_pmu_probe(struct platform_device *pdev)
> return PTR_ERR(pmu_base_addr);
>
> pmu_context = devm_kzalloc(&pdev->dev,
> - sizeof(struct exynos_pmu_context),
> - GFP_KERNEL);
> + sizeof(struct exynos_pmu_context),
> + GFP_KERNEL);
> if (!pmu_context)
> return -ENOMEM;
> - pmu_context->dev = dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENODEV;
> +
> pmu_context->pmu_data = of_device_get_match_data(dev);
>
> + /* For SoCs that secure PMU register writes use custom regmap */
> + if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_secure) {
> + pmu_regmcfg = regmap_smccfg;
> + pmu_regmcfg.max_register = resource_size(res) -
> + pmu_regmcfg.reg_stride;
> + /* Need physical address for SMC call */
> + regmap = devm_regmap_init(dev, NULL,
> + (void *)(uintptr_t)res->start,
> + &pmu_regmcfg);
> + } else {
> + /* All other SoCs use a MMIO regmap */
> + pmu_regmcfg = regmap_mmiocfg;
> + pmu_regmcfg.max_register = resource_size(res) -
> + pmu_regmcfg.reg_stride;
> + regmap = devm_regmap_init_mmio(dev, pmu_base_addr,
> + &pmu_regmcfg);
> + }
> +
> + if (IS_ERR(regmap))
> + return dev_err_probe(&pdev->dev, PTR_ERR(regmap),
> + "regmap init failed\n");
> +
> + pmu_context->pmureg = regmap;
> + pmu_context->dev = dev;
> +
> if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_init)
> pmu_context->pmu_data->pmu_init();
>
> diff --git a/drivers/soc/samsung/exynos-pmu.h b/drivers/soc/samsung/exynos-pmu.h
> index 1c652ffd79b4..0a49a2c9a08e 100644
> --- a/drivers/soc/samsung/exynos-pmu.h
> +++ b/drivers/soc/samsung/exynos-pmu.h
> @@ -21,6 +21,7 @@ struct exynos_pmu_conf {
> struct exynos_pmu_data {
> const struct exynos_pmu_conf *pmu_config;
> const struct exynos_pmu_conf *pmu_config_extra;
> + bool pmu_secure;
>
> void (*pmu_init)(void);
> void (*powerdown_conf)(enum sys_powerdown);
> diff --git a/include/linux/soc/samsung/exynos-pmu.h b/include/linux/soc/samsung/exynos-pmu.h
> index a4f5516cc956..e1c86640f6f7 100644
> --- a/include/linux/soc/samsung/exynos-pmu.h
> +++ b/include/linux/soc/samsung/exynos-pmu.h
> @@ -10,6 +10,7 @@
> #define __LINUX_SOC_EXYNOS_PMU_H
>
> struct regmap;
> +struct device_node;
>
> enum sys_powerdown {
> SYS_AFTR,
> @@ -20,12 +21,20 @@ enum sys_powerdown {
>
> extern void exynos_sys_powerdown_conf(enum sys_powerdown mode);
> #ifdef CONFIG_EXYNOS_PMU
> -extern struct regmap *exynos_get_pmu_regmap(void);
> +struct regmap *exynos_get_pmu_regmap(void);
> +struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
> + const char *property);
> #else
> static inline struct regmap *exynos_get_pmu_regmap(void)
> {
> return ERR_PTR(-ENODEV);
> }
> +
> +static inline struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
> + const char *property)
> +{
> + return ERR_PTR(-ENODEV);
> +}
> #endif
>
> #endif /* __LINUX_SOC_EXYNOS_PMU_H */
> --
> 2.43.0.594.gd9cf4e227d-goog
>
On Thu, 8 Feb 2024 at 16:21, Peter Griffin <[email protected]> wrote:
>
> Obtain the PMU regmap using the new API added to exynos-pmu driver rather
> than syscon_regmap_lookup_by_phandle(). As this driver no longer depends
> on mfd syscon remove that header and Kconfig dependency.
>
> Reviewed-by: Sam Protsenko <[email protected]>
> Signed-off-by: Peter Griffin <[email protected]>
Tested-by: Alexey Klimov <[email protected]>
Tested on odroid xu3 (exynos5422). Watchdog works as expected and can
reset the system.
> ---
> drivers/watchdog/Kconfig | 1 -
> drivers/watchdog/s3c2410_wdt.c | 8 ++++----
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7d22051b15a2..d78fe7137799 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -512,7 +512,6 @@ config S3C2410_WATCHDOG
> tristate "S3C6410/S5Pv210/Exynos Watchdog"
> depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
> select WATCHDOG_CORE
> - select MFD_SYSCON if ARCH_EXYNOS
> help
> Watchdog timer block in the Samsung S3C64xx, S5Pv210 and Exynos
> SoCs. This will reboot the system when the timer expires with
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 349d30462c8c..686cf544d0ae 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -24,9 +24,9 @@
> #include <linux/slab.h>
> #include <linux/err.h>
> #include <linux/of.h>
> -#include <linux/mfd/syscon.h>
> #include <linux/regmap.h>
> #include <linux/delay.h>
> +#include <linux/soc/samsung/exynos-pmu.h>
>
> #define S3C2410_WTCON 0x00
> #define S3C2410_WTDAT 0x04
> @@ -699,11 +699,11 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> return ret;
>
> if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> - wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> - "samsung,syscon-phandle");
> + wdt->pmureg = exynos_get_pmu_regmap_by_phandle(dev->of_node,
> + "samsung,syscon-phandle");
> if (IS_ERR(wdt->pmureg))
> return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
> - "syscon regmap lookup failed.\n");
> + "PMU regmap lookup failed.\n");
> }
>
> wdt_irq = platform_get_irq(pdev, 0);
> --
> 2.43.0.594.gd9cf4e227d-goog
>
On Thu, 8 Feb 2024 at 16:21, Peter Griffin <[email protected]> wrote:
>
> Some Exynos based SoCs like Tensor gs101 protect the PMU registers for
> security hardening reasons so that they are only write accessible in el3
> via an SMC call.
>
> As most Exynos drivers that need to write PMU registers currently obtain a
> regmap via syscon (phys, pinctrl, watchdog). Support for the above usecase
> is implemented in this driver using a custom regmap similar to syscon to
> handle the SMC call. Platforms that don't secure PMU registers, get a mmio
> regmap like before. As regmaps abstract out the underlying register access
> changes to the leaf drivers are minimal.
>
> A new API exynos_get_pmu_regmap_by_phandle() is provided for leaf drivers
> that currently use syscon_regmap_lookup_by_phandle(). This also handles
> deferred probing.
>
> Signed-off-by: Peter Griffin <[email protected]>
Tested-by: Alexey Klimov <[email protected]>
Apparently it seems tested-by should be provided for both patches. This was
also tested on odroid xu3 and I also have WIP code that depends on this
SMC-based regmap. No issues are observed and behaves as expected.
> ---
> Changes since v3:
> - Fix PMUALIVE_MASK
> - Add TENSOR_ prefix
> - clear SET_BITS bits on each loop iteration
> - change set_bit to set_bits in func name
> - Fix some alignment
> - Add missing return on dev_err_probe
> - Reduce indentation in loop
I no longer see the compilation warning related to struct device_node declared
inside parameter list with v4, I guess one line change addition in exynos-pmu.h
does the job.
Thank you!
BR,
Alexey
Hi Alexey,
On Fri, 9 Feb 2024 at 14:52, Alexey Klimov <[email protected]> wrote:
>
> On Thu, 8 Feb 2024 at 16:21, Peter Griffin <[email protected]> wrote:
> >
> > Some Exynos based SoCs like Tensor gs101 protect the PMU registers for
> > security hardening reasons so that they are only write accessible in el3
> > via an SMC call.
> >
> > As most Exynos drivers that need to write PMU registers currently obtain a
> > regmap via syscon (phys, pinctrl, watchdog). Support for the above usecase
> > is implemented in this driver using a custom regmap similar to syscon to
> > handle the SMC call. Platforms that don't secure PMU registers, get a mmio
> > regmap like before. As regmaps abstract out the underlying register access
> > changes to the leaf drivers are minimal.
> >
> > A new API exynos_get_pmu_regmap_by_phandle() is provided for leaf drivers
> > that currently use syscon_regmap_lookup_by_phandle(). This also handles
> > deferred probing.
> >
> > Signed-off-by: Peter Griffin <[email protected]>
>
> Tested-by: Alexey Klimov <[email protected]>
Thanks for testing this on odroid xu3 hardware!
>
> Apparently it seems tested-by should be provided for both patches. This was
> also tested on odroid xu3 and I also have WIP code that depends on this
> SMC-based regmap. No issues are observed and behaves as expected.
>
> > ---
> > Changes since v3:
> > - Fix PMUALIVE_MASK
> > - Add TENSOR_ prefix
> > - clear SET_BITS bits on each loop iteration
> > - change set_bit to set_bits in func name
> > - Fix some alignment
> > - Add missing return on dev_err_probe
> > - Reduce indentation in loop
>
> I no longer see the compilation warning related to struct device_node declared
> inside parameter list with v4, I guess one line change addition in exynos-pmu.h
> does the job.
I added a forward declaration in the header to get rid of that
compiler warning in v4, but it's not explicitly mentioned in the above
changelog.
Thanks,
Peter.
Peter
On Thu, Feb 8, 2024 at 10:21 AM Peter Griffin <peter.griffin@linaroorg> wrote:
>
> Obtain the PMU regmap using the new API added to exynos-pmu driver rather
> than syscon_regmap_lookup_by_phandle(). As this driver no longer depends
> on mfd syscon remove that header and Kconfig dependency.
>
> Reviewed-by: Sam Protsenko <[email protected]>
> Signed-off-by: Peter Griffin <[email protected]>
> ---
Tested-by: Sam Protsenko <[email protected]>
Tested on my E850-96 board (Exynos850 chip) with Debian rootfs. Test procedure:
1. Added this to /chosen node in dts:
bootargs = "s3c2410_wdt.tmr_atboot=1 s3c2410_wdt.nowayout=1
s3c2410_wdt.soft_noboot=0";
2. Check if watchdogs are active:
# dmesg | grep watchdog
[ 1.488149] s3c2410-wdt 10050000.watchdog: starting watchdog timer
[ 1.489003] s3c2410-wdt 10050000.watchdog: watchdog active,
reset enabled, irq disabled
[ 1.496928] s3c2410-wdt 10060000.watchdog: starting watchdog timer
[ 1.502984] s3c2410-wdt 10060000.watchdog: watchdog active,
reset enabled, irq disabled
3. Generate a panic to cause wdt reset:
# echo c > /proc/sysrq-trigger
In 15 seconds (wdt timeout) reboot happens, and bootloader shows this
message for the reboot reason:
Watchdog or Warm Reset Detected
That proves regmap works fine with this patch. Otherwise reboot wouldn't happen.
> drivers/watchdog/Kconfig | 1 -
> drivers/watchdog/s3c2410_wdt.c | 8 ++++----
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7d22051b15a2..d78fe7137799 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -512,7 +512,6 @@ config S3C2410_WATCHDOG
> tristate "S3C6410/S5Pv210/Exynos Watchdog"
> depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
> select WATCHDOG_CORE
> - select MFD_SYSCON if ARCH_EXYNOS
> help
> Watchdog timer block in the Samsung S3C64xx, S5Pv210 and Exynos
> SoCs. This will reboot the system when the timer expires with
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 349d30462c8c..686cf544d0ae 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -24,9 +24,9 @@
> #include <linux/slab.h>
> #include <linux/err.h>
> #include <linux/of.h>
> -#include <linux/mfd/syscon.h>
> #include <linux/regmap.h>
> #include <linux/delay.h>
> +#include <linux/soc/samsung/exynos-pmu.h>
>
> #define S3C2410_WTCON 0x00
> #define S3C2410_WTDAT 0x04
> @@ -699,11 +699,11 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> return ret;
>
> if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> - wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> - "samsung,syscon-phandle");
> + wdt->pmureg = exynos_get_pmu_regmap_by_phandle(dev->of_node,
> + "samsung,syscon-phandle");
> if (IS_ERR(wdt->pmureg))
> return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
> - "syscon regmap lookup failed\n");
> + "PMU regmap lookup failed.\n");
> }
>
> wdt_irq = platform_get_irq(pdev, 0);
> --
> 2.43.0.594.gd9cf4e227d-goog
>
On Thu, Feb 8, 2024 at 10:21 AM Peter Griffin <peter.griffin@linaroorg> wrote:
>
> Some Exynos based SoCs like Tensor gs101 protect the PMU registers for
> security hardening reasons so that they are only write accessible in el3
> via an SMC call.
>
> As most Exynos drivers that need to write PMU registers currently obtain a
> regmap via syscon (phys, pinctrl, watchdog). Support for the above usecase
> is implemented in this driver using a custom regmap similar to syscon to
> handle the SMC call. Platforms that don't secure PMU registers, get a mmio
> regmap like before. As regmaps abstract out the underlying register access
> changes to the leaf drivers are minimal.
>
> A new API exynos_get_pmu_regmap_by_phandle() is provided for leaf drivers
> that currently use syscon_regmap_lookup_by_phandle(). This also handles
> deferred probing.
>
> Signed-off-by: Peter Griffin <[email protected]>
> ---
> Changes since v3:
> - Fix PMUALIVE_MASK
> - Add TENSOR_ prefix
> - clear SET_BITS bits on each loop iteration
> - change set_bit to set_bits in func name
> - Fix some alignment
> - Add missing return on dev_err_probe
> - Reduce indentation in loop
>
> Changes since v2
> - Add select REGMAP to Kconfig
> - Add constant for SET/CLEAR bits
> - Replace kerneldoc with one line comment
> - Fix kerneldoc for EXPORT_SYMBOL_GPL funcs
> - remove superfluous extern keyword
> - dev_err_probe() on probe error
> - shorten regmcfg name
> - no compatibles inside probe, use match data
> - don't mix declarations with/without initializations
> - tensor_sec_reg_read() use mmio to avoid access restrictions
> - Collect up Reviewed-by
> - const for regmap_config structs
> ---
Tested-by: Sam Protsenko <[email protected]>
Tested on my E850-96. All modules that use PMU are still functional
with this patch (watchdog, USB host and Ethernet). No regressions.
[snip]
On 08/02/2024 17:16, Peter Griffin wrote:
> Some Exynos based SoCs like Tensor gs101 protect the PMU registers for
> security hardening reasons so that they are only write accessible in el3
> via an SMC call.
>
> As most Exynos drivers that need to write PMU registers currently obtain a
> regmap via syscon (phys, pinctrl, watchdog). Support for the above usecase
> is implemented in this driver using a custom regmap similar to syscon to
> handle the SMC call. Platforms that don't secure PMU registers, get a mmio
> regmap like before. As regmaps abstract out the underlying register access
> changes to the leaf drivers are minimal.
>
> A new API exynos_get_pmu_regmap_by_phandle() is provided for leaf drivers
> that currently use syscon_regmap_lookup_by_phandle(). This also handles
> deferred probing.
I found an issue, which needs new version, so I also found few more nits.
>
> Signed-off-by: Peter Griffin <[email protected]>
> ---
> Changes since v3:
> - Fix PMUALIVE_MASK
> - Add TENSOR_ prefix
> - clear SET_BITS bits on each loop iteration
> - change set_bit to set_bits in func name
> - Fix some alignment
> - Add missing return on dev_err_probe
> - Reduce indentation in loop
>
> Changes since v2
> - Add select REGMAP to Kconfig
> - Add constant for SET/CLEAR bits
> - Replace kerneldoc with one line comment
> - Fix kerneldoc for EXPORT_SYMBOL_GPL funcs
> - remove superfluous extern keyword
> - dev_err_probe() on probe error
> - shorten regmcfg name
> - no compatibles inside probe, use match data
> - don't mix declarations with/without initializations
> - tensor_sec_reg_read() use mmio to avoid access restrictions
> - Collect up Reviewed-by
> - const for regmap_config structs
> ---
> drivers/soc/samsung/Kconfig | 1 +
> drivers/soc/samsung/exynos-pmu.c | 235 ++++++++++++++++++++++++-
> drivers/soc/samsung/exynos-pmu.h | 1 +
> include/linux/soc/samsung/exynos-pmu.h | 11 +-
> 4 files changed, 243 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
> index 27ec99af77e3..1a5dfdc978dc 100644
> --- a/drivers/soc/samsung/Kconfig
> +++ b/drivers/soc/samsung/Kconfig
> @@ -42,6 +42,7 @@ config EXYNOS_PMU
> depends on ARCH_EXYNOS || ((ARM || ARM64) && COMPILE_TEST)
> select EXYNOS_PMU_ARM_DRIVERS if ARM && ARCH_EXYNOS
> select MFD_CORE
> + select REGMAP_MMIO
>
> # There is no need to enable these drivers for ARMv8
> config EXYNOS_PMU_ARM_DRIVERS
> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> index 250537d7cfd6..b846e343fcdd 100644
> --- a/drivers/soc/samsung/exynos-pmu.c
> +++ b/drivers/soc/samsung/exynos-pmu.c
> @@ -5,6 +5,7 @@
> //
> // Exynos - CPU PMU(Power Management Unit) support
>
> +#include <linux/arm-smccc.h>
> #include <linux/of.h>
> #include <linux/of_address.h>
> #include <linux/mfd/core.h>
> @@ -12,19 +13,132 @@
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> #include <linux/delay.h>
> +#include <linux/regmap.h>
>
> #include <linux/soc/samsung/exynos-regs-pmu.h>
> #include <linux/soc/samsung/exynos-pmu.h>
>
> #include "exynos-pmu.h"
>
> +#define PMUALIVE_MASK GENMASK(13, 0)
> +#define TENSOR_SET_BITS (BIT(15) | BIT(14))
> +#define TENSOR_CLR_BITS BIT(15)
> +#define TENSOR_SMC_PMU_SEC_REG 0x82000504
> +#define TENSOR_PMUREG_READ 0
> +#define TENSOR_PMUREG_WRITE 1
> +#define TENSOR_PMUREG_RMW 2
> +
> struct exynos_pmu_context {
> struct device *dev;
> const struct exynos_pmu_data *pmu_data;
> + struct regmap *pmureg;
> };
>
> void __iomem *pmu_base_addr;
> static struct exynos_pmu_context *pmu_context;
> +/* forward declaration */
> +static struct platform_driver exynos_pmu_driver;
> +
> +/*
> + * Tensor SoCs are configured so that PMU_ALIVE registers can only be written
> + * from EL3, but are still read accessible. As Linux needs to write some of
> + * these registers, the following functions are provided and exposed via
> + * regmap.
> + *
> + * Note: This SMC interface is known to be implemented on gs101 and derivative
> + * SoCs.
> + */
> +
> +/* Write to a protected PMU register. */
> +static int tensor_sec_reg_write(void *base, unsigned int reg, unsigned int val)
Please use the same argument names in all these regmap functions as in
struct regmap_config, so base->context
> +{
> + struct arm_smccc_res res;
> + unsigned long pmu_base = (unsigned long)base;
> +
> + arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG, pmu_base + reg,
> + TENSOR_PMUREG_WRITE, val, 0, 0, 0, 0, &res);
> +
> + /* returns -EINVAL if access isn't allowed or 0 */
> + if (res.a0)
> + pr_warn("%s(): SMC failed: %d\n", __func__, (int)res.a0);
> +
> + return (int)res.a0;
> +}
> +
> +/* Read/Modify/Write a protected PMU register. */
> +static int tensor_sec_reg_rmw(void *base, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + struct arm_smccc_res res;
> + unsigned long pmu_base = (unsigned long)base;
> +
> + arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG, pmu_base + reg,
> + TENSOR_PMUREG_RMW, mask, val, 0, 0, 0, &res);
> +
> + /* returns -EINVAL if access isn't allowed or 0 */
> + if (res.a0)
> + pr_warn("%s(): SMC failed: %d\n", __func__, (int)res.a0);
> +
> + return (int)res.a0;
> +}
> +
> +/*
> + * Read a protected PMU register. All PMU registers can be read by Linux.
> + * Note: The SMC read register is not used, as only registers that can be
> + * written are readable via SMC.
> + */
> +static int tensor_sec_reg_read(void *base, unsigned int reg, unsigned int *val)
> +{
> + *val = pmu_raw_readl(reg);
> + return 0;
> +}
> +
> +/*
> + * For SoCs that have set/clear bit hardware this function can be used when
> + * the PMU register will be accessed by multiple masters.
> + *
> + * For example, to set bits 13:8 in PMU reg offset 0x3e80
> + * tensor_set_bits_atomic(ctx, 0x3e80, 0x3f00, 0x3f00);
> + *
> + * Set bit 8, and clear bits 13:9 PMU reg offset 0x3e80
> + * tensor_set_bits_atomic(0x3e80, 0x100, 0x3f00);
> + */
> +static inline int tensor_set_bits_atomic(void *ctx, unsigned int offset,
Usual practice is to rely on compiler to inline, so let's drop the
keyword here.
> + u32 val, u32 mask)
> +{
> + int ret;
> + unsigned int i;
> +
> + for (i = 0; i < 32; i++) {
> + if (!(mask & BIT(i)))
> + continue;
> +
> + offset &= ~TENSOR_SET_BITS;
> +
> + if (val & BIT(i))
> + offset |= TENSOR_SET_BITS;
> + else
> + offset |= TENSOR_CLR_BITS;
> +
> + ret = tensor_sec_reg_write(ctx, offset, i);
> + if (ret)
> + return ret;
> + }
> + return ret;
> +}
> +
> +static int tensor_sec_update_bits(void *ctx, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + /*
> + * Use atomic operations for PMU_ALIVE registers (offset 0~0x3FFF)
> + * as the target registers can be accessed by multiple masters.
> + */
> + if (reg > PMUALIVE_MASK)
> + return tensor_sec_reg_rmw(ctx, reg, mask, val);
> +
> + return tensor_set_bits_atomic(ctx, reg, val, mask);
> +}
>
> void pmu_raw_writel(u32 val, u32 offset)
> {
> @@ -75,11 +189,41 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode)
> #define exynos_pmu_data_arm_ptr(data) NULL
> #endif
>
> +static const struct regmap_config regmap_smccfg = {
> + .name = "pmu_regs",
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .fast_io = true,
> + .use_single_read = true,
> + .use_single_write = true,
> + .reg_read = tensor_sec_reg_read,
> + .reg_write = tensor_sec_reg_write,
> + .reg_update_bits = tensor_sec_update_bits,
> +};
> +
> +static const struct regmap_config regmap_mmiocfg = {
> + .name = "pmu_regs",
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .fast_io = true,
> + .use_single_read = true,
> + .use_single_write = true,
> +};
> +
> +static const struct exynos_pmu_data gs101_pmu_data = {
> + .pmu_secure = true
> +};
> +
> /*
> * PMU platform driver and devicetree bindings.
> */
> static const struct of_device_id exynos_pmu_of_device_ids[] = {
> {
> + .compatible = "google,gs101-pmu",
> + .data = &gs101_pmu_data,
> + }, {
> .compatible = "samsung,exynos3250-pmu",
> .data = exynos_pmu_data_arm_ptr(exynos3250_pmu_data),
> }, {
> @@ -113,19 +257,73 @@ static const struct mfd_cell exynos_pmu_devs[] = {
> { .name = "exynos-clkout", },
> };
>
> +/**
> + * exynos_get_pmu_regmap() - Obtain pmureg regmap
> + *
> + * Find the pmureg regmap previously configured in probe() and return regmap
> + * pointer.
> + *
> + * Return: A pointer to regmap if found or ERR_PTR error value.
> + */
> struct regmap *exynos_get_pmu_regmap(void)
> {
> struct device_node *np = of_find_matching_node(NULL,
> exynos_pmu_of_device_ids);
> if (np)
> - return syscon_node_to_regmap(np);
> + return exynos_get_pmu_regmap_by_phandle(np, NULL);
> return ERR_PTR(-ENODEV);
> }
> EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap);
>
> +/**
> + * exynos_get_pmu_regmap_by_phandle() - Obtain pmureg regmap via phandle
> + * @np: Pointer to device's Device Tree node
A bit unusual naming... drop "Device Tree" anywhere here. This is:
"device node holding PMU phandle property"
> + * @property: Device Tree property name which references the pmu
Name of property holding a phandle value
> + *
> + * Find the pmureg regmap previously configured in probe() and return regmap
> + * pointer.
> + *
> + * Return: A pointer to regmap if found or ERR_PTR error value.
> + */
> +struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
> + const char *property)
property -> propname
> +{
> + struct device *dev;
> + struct exynos_pmu_context *ctx;
> + struct device_node *pmu_np;
Reversed christmass tree.
> +
> + if (property)
> + pmu_np = of_parse_phandle(np, property, 0);
> + else
> + pmu_np = np;
> +
> + if (!pmu_np)
> + return ERR_PTR(-ENODEV);
> +
> + /*
> + * Determine if exynos-pmu device has probed and therefore regmap
> + * has been created and can be returned to the caller. Otherwise we
> + * return -EPROBE_DEFER.
> + */
> + dev = driver_find_device_by_of_node(&exynos_pmu_driver.driver,
> + (void *)pmu_np);
> +
> + of_node_put(pmu_np);
You are dropping now referencen from np when property==NULL. This does
no look right.
> + if (!dev)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + ctx = dev_get_drvdata(dev);
> +
> + return ctx->pmureg;
> +}
> +EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
> +
> static int exynos_pmu_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> + struct regmap_config pmu_regmcfg;
> + struct regmap *regmap;
> + struct resource *res;
> int ret;
>
> pmu_base_addr = devm_platform_ioremap_resource(pdev, 0);
> @@ -133,13 +331,42 @@ static int exynos_pmu_probe(struct platform_device *pdev)
> return PTR_ERR(pmu_base_addr);
>
> pmu_context = devm_kzalloc(&pdev->dev,
> - sizeof(struct exynos_pmu_context),
> - GFP_KERNEL);
> + sizeof(struct exynos_pmu_context),
> + GFP_KERNEL);
Not related here. You could have separate patch for cleanups or just
skip such change.
> if (!pmu_context)
> return -ENOMEM;
> - pmu_context->dev = dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENODEV;
> +
Best regards,
Krzysztof
Hi Krzysztof,
Thanks for the review. I was OoO last week so just getting to this now.
On Thu, 15 Feb 2024 at 18:36, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 08/02/2024 17:16, Peter Griffin wrote:
> > Some Exynos based SoCs like Tensor gs101 protect the PMU registers for
> > security hardening reasons so that they are only write accessible in el3
> > via an SMC call.
> >
> > As most Exynos drivers that need to write PMU registers currently obtain a
> > regmap via syscon (phys, pinctrl, watchdog). Support for the above usecase
> > is implemented in this driver using a custom regmap similar to syscon to
> > handle the SMC call. Platforms that don't secure PMU registers, get a mmio
> > regmap like before. As regmaps abstract out the underlying register access
> > changes to the leaf drivers are minimal.
> >
> > A new API exynos_get_pmu_regmap_by_phandle() is provided for leaf drivers
> > that currently use syscon_regmap_lookup_by_phandle(). This also handles
> > deferred probing.
>
> I found an issue, which needs new version, so I also found few more nits.
>
> >
> > Signed-off-by: Peter Griffin <[email protected]>
> > ---
> > Changes since v3:
> > - Fix PMUALIVE_MASK
> > - Add TENSOR_ prefix
> > - clear SET_BITS bits on each loop iteration
> > - change set_bit to set_bits in func name
> > - Fix some alignment
> > - Add missing return on dev_err_probe
> > - Reduce indentation in loop
> >
> > Changes since v2
> > - Add select REGMAP to Kconfig
> > - Add constant for SET/CLEAR bits
> > - Replace kerneldoc with one line comment
> > - Fix kerneldoc for EXPORT_SYMBOL_GPL funcs
> > - remove superfluous extern keyword
> > - dev_err_probe() on probe error
> > - shorten regmcfg name
> > - no compatibles inside probe, use match data
> > - don't mix declarations with/without initializations
> > - tensor_sec_reg_read() use mmio to avoid access restrictions
> > - Collect up Reviewed-by
> > - const for regmap_config structs
> > ---
> > drivers/soc/samsung/Kconfig | 1 +
> > drivers/soc/samsung/exynos-pmu.c | 235 ++++++++++++++++++++++++-
> > drivers/soc/samsung/exynos-pmu.h | 1 +
> > include/linux/soc/samsung/exynos-pmu.h | 11 +-
> > 4 files changed, 243 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
> > index 27ec99af77e3..1a5dfdc978dc 100644
> > --- a/drivers/soc/samsung/Kconfig
> > +++ b/drivers/soc/samsung/Kconfig
> > @@ -42,6 +42,7 @@ config EXYNOS_PMU
> > depends on ARCH_EXYNOS || ((ARM || ARM64) && COMPILE_TEST)
> > select EXYNOS_PMU_ARM_DRIVERS if ARM && ARCH_EXYNOS
> > select MFD_CORE
> > + select REGMAP_MMIO
> >
> > # There is no need to enable these drivers for ARMv8
> > config EXYNOS_PMU_ARM_DRIVERS
> > diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> > index 250537d7cfd6..b846e343fcdd 100644
> > --- a/drivers/soc/samsung/exynos-pmu.c
> > +++ b/drivers/soc/samsung/exynos-pmu.c
> > @@ -5,6 +5,7 @@
> > //
> > // Exynos - CPU PMU(Power Management Unit) support
> >
> > +#include <linux/arm-smccc.h>
> > #include <linux/of.h>
> > #include <linux/of_address.h>
> > #include <linux/mfd/core.h>
> > @@ -12,19 +13,132 @@
> > #include <linux/of_platform.h>
> > #include <linux/platform_device.h>
> > #include <linux/delay.h>
> > +#include <linux/regmap.h>
> >
> > #include <linux/soc/samsung/exynos-regs-pmu.h>
> > #include <linux/soc/samsung/exynos-pmu.h>
> >
> > #include "exynos-pmu.h"
> >
> > +#define PMUALIVE_MASK GENMASK(13, 0)
> > +#define TENSOR_SET_BITS (BIT(15) | BIT(14))
> > +#define TENSOR_CLR_BITS BIT(15)
> > +#define TENSOR_SMC_PMU_SEC_REG 0x82000504
> > +#define TENSOR_PMUREG_READ 0
> > +#define TENSOR_PMUREG_WRITE 1
> > +#define TENSOR_PMUREG_RMW 2
> > +
> > struct exynos_pmu_context {
> > struct device *dev;
> > const struct exynos_pmu_data *pmu_data;
> > + struct regmap *pmureg;
> > };
> >
> > void __iomem *pmu_base_addr;
> > static struct exynos_pmu_context *pmu_context;
> > +/* forward declaration */
> > +static struct platform_driver exynos_pmu_driver;
> > +
> > +/*
> > + * Tensor SoCs are configured so that PMU_ALIVE registers can only be written
> > + * from EL3, but are still read accessible. As Linux needs to write some of
> > + * these registers, the following functions are provided and exposed via
> > + * regmap.
> > + *
> > + * Note: This SMC interface is known to be implemented on gs101 and derivative
> > + * SoCs.
> > + */
> > +
> > +/* Write to a protected PMU register. */
> > +static int tensor_sec_reg_write(void *base, unsigned int reg, unsigned int val)
>
> Please use the same argument names in all these regmap functions as in
> struct regmap_config, so base->context
Will fix.
>
> > +{
> > + struct arm_smccc_res res;
> > + unsigned long pmu_base = (unsigned long)base;
> > +
> > + arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG, pmu_base + reg,
> > + TENSOR_PMUREG_WRITE, val, 0, 0, 0, 0, &res);
> > +
> > + /* returns -EINVAL if access isn't allowed or 0 */
> > + if (res.a0)
> > + pr_warn("%s(): SMC failed: %d\n", __func__, (int)res.a0);
> > +
> > + return (int)res.a0;
> > +}
> > +
> > +/* Read/Modify/Write a protected PMU register. */
> > +static int tensor_sec_reg_rmw(void *base, unsigned int reg,
> > + unsigned int mask, unsigned int val)
> > +{
> > + struct arm_smccc_res res;
> > + unsigned long pmu_base = (unsigned long)base;
> > +
> > + arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG, pmu_base + reg,
> > + TENSOR_PMUREG_RMW, mask, val, 0, 0, 0, &res);
> > +
> > + /* returns -EINVAL if access isn't allowed or 0 */
> > + if (res.a0)
> > + pr_warn("%s(): SMC failed: %d\n", __func__, (int)res.a0);
> > +
> > + return (int)res.a0;
> > +}
> > +
> > +/*
> > + * Read a protected PMU register. All PMU registers can be read by Linux.
> > + * Note: The SMC read register is not used, as only registers that can be
> > + * written are readable via SMC.
> > + */
> > +static int tensor_sec_reg_read(void *base, unsigned int reg, unsigned int *val)
> > +{
> > + *val = pmu_raw_readl(reg);
> > + return 0;
> > +}
> > +
> > +/*
> > + * For SoCs that have set/clear bit hardware this function can be used when
> > + * the PMU register will be accessed by multiple masters.
> > + *
> > + * For example, to set bits 13:8 in PMU reg offset 0x3e80
> > + * tensor_set_bits_atomic(ctx, 0x3e80, 0x3f00, 0x3f00);
> > + *
> > + * Set bit 8, and clear bits 13:9 PMU reg offset 0x3e80
> > + * tensor_set_bits_atomic(0x3e80, 0x100, 0x3f00);
> > + */
> > +static inline int tensor_set_bits_atomic(void *ctx, unsigned int offset,
>
> Usual practice is to rely on compiler to inline, so let's drop the
> keyword here.
Will fix
>
> > + u32 val, u32 mask)
> > +{
> > + int ret;
> > + unsigned int i;
> > +
> > + for (i = 0; i < 32; i++) {
> > + if (!(mask & BIT(i)))
> > + continue;
> > +
> > + offset &= ~TENSOR_SET_BITS;
> > +
> > + if (val & BIT(i))
> > + offset |= TENSOR_SET_BITS;
> > + else
> > + offset |= TENSOR_CLR_BITS;
> > +
> > + ret = tensor_sec_reg_write(ctx, offset, i);
> > + if (ret)
> > + return ret;
> > + }
> > + return ret;
> > +}
> > +
> > +static int tensor_sec_update_bits(void *ctx, unsigned int reg,
> > + unsigned int mask, unsigned int val)
> > +{
> > + /*
> > + * Use atomic operations for PMU_ALIVE registers (offset 0~0x3FFF)
> > + * as the target registers can be accessed by multiple masters.
> > + */
> > + if (reg > PMUALIVE_MASK)
> > + return tensor_sec_reg_rmw(ctx, reg, mask, val);
> > +
> > + return tensor_set_bits_atomic(ctx, reg, val, mask);
> > +}
> >
> > void pmu_raw_writel(u32 val, u32 offset)
> > {
> > @@ -75,11 +189,41 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode)
> > #define exynos_pmu_data_arm_ptr(data) NULL
> > #endif
> >
> > +static const struct regmap_config regmap_smccfg = {
> > + .name = "pmu_regs",
> > + .reg_bits = 32,
> > + .reg_stride = 4,
> > + .val_bits = 32,
> > + .fast_io = true,
> > + .use_single_read = true,
> > + .use_single_write = true,
> > + .reg_read = tensor_sec_reg_read,
> > + .reg_write = tensor_sec_reg_write,
> > + .reg_update_bits = tensor_sec_update_bits,
>
> > +};
> > +
> > +static const struct regmap_config regmap_mmiocfg = {
> > + .name = "pmu_regs",
> > + .reg_bits = 32,
> > + .reg_stride = 4,
> > + .val_bits = 32,
> > + .fast_io = true,
> > + .use_single_read = true,
> > + .use_single_write = true,
> > +};
> > +
> > +static const struct exynos_pmu_data gs101_pmu_data = {
> > + .pmu_secure = true
> > +};
> > +
> > /*
> > * PMU platform driver and devicetree bindings.
> > */
> > static const struct of_device_id exynos_pmu_of_device_ids[] = {
> > {
> > + .compatible = "google,gs101-pmu",
> > + .data = &gs101_pmu_data,
> > + }, {
> > .compatible = "samsung,exynos3250-pmu",
> > .data = exynos_pmu_data_arm_ptr(exynos3250_pmu_data),
> > }, {
> > @@ -113,19 +257,73 @@ static const struct mfd_cell exynos_pmu_devs[] = {
> > { .name = "exynos-clkout", },
> > };
> >
> > +/**
> > + * exynos_get_pmu_regmap() - Obtain pmureg regmap
> > + *
> > + * Find the pmureg regmap previously configured in probe() and return regmap
> > + * pointer.
> > + *
> > + * Return: A pointer to regmap if found or ERR_PTR error value.
> > + */
> > struct regmap *exynos_get_pmu_regmap(void)
> > {
> > struct device_node *np = of_find_matching_node(NULL,
> > exynos_pmu_of_device_ids);
> > if (np)
> > - return syscon_node_to_regmap(np);
> > + return exynos_get_pmu_regmap_by_phandle(np, NULL);
> > return ERR_PTR(-ENODEV);
> > }
> > EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap);
> >
> > +/**
> > + * exynos_get_pmu_regmap_by_phandle() - Obtain pmureg regmap via phandle
> > + * @np: Pointer to device's Device Tree node
>
> A bit unusual naming... drop "Device Tree" anywhere here. This is:
> "device node holding PMU phandle property"
Will fix
>
>
> > + * @property: Device Tree property name which references the pmu
>
> Name of property holding a phandle value
Will fix
>
> > + *
> > + * Find the pmureg regmap previously configured in probe() and return regmap
> > + * pointer.
> > + *
> > + * Return: A pointer to regmap if found or ERR_PTR error value.
> > + */
> > +struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
> > + const char *property)
>
> property -> propname
Will fix
>
> > +{
> > + struct device *dev;
> > + struct exynos_pmu_context *ctx;
> > + struct device_node *pmu_np;
>
> Reversed christmass tree.
Will fix
>
> > +
> > + if (property)
> > + pmu_np = of_parse_phandle(np, property, 0);
> > + else
> > + pmu_np = np;
> > +
> > + if (!pmu_np)
> > + return ERR_PTR(-ENODEV);
> > +
> > + /*
> > + * Determine if exynos-pmu device has probed and therefore regmap
> > + * has been created and can be returned to the caller. Otherwise we
> > + * return -EPROBE_DEFER.
> > + */
> > + dev = driver_find_device_by_of_node(&exynos_pmu_driver.driver,
> > + (void *)pmu_np);
> > +
> > + of_node_put(pmu_np);
>
> You are dropping now referencen from np when property==NULL. This does
> no look right.
Good spot, will fix. It seems syscon.c and altera-sysmgr also have the
same issue.
>
> > + if (!dev)
> > + return ERR_PTR(-EPROBE_DEFER);
> > +
> > + ctx = dev_get_drvdata(dev);
> > +
> > + return ctx->pmureg;
> > +}
> > +EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
> > +
> > static int exynos_pmu_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > + struct regmap_config pmu_regmcfg;
> > + struct regmap *regmap;
> > + struct resource *res;
> > int ret;
> >
> > pmu_base_addr = devm_platform_ioremap_resource(pdev, 0);
> > @@ -133,13 +331,42 @@ static int exynos_pmu_probe(struct platform_device *pdev)
> > return PTR_ERR(pmu_base_addr);
> >
> > pmu_context = devm_kzalloc(&pdev->dev,
> > - sizeof(struct exynos_pmu_context),
> > - GFP_KERNEL);
> > + sizeof(struct exynos_pmu_context),
> > + GFP_KERNEL);
>
> Not related here. You could have separate patch for cleanups or just
> skip such change.
Will remove.
kind regards,
Peter.
On 19/02/2024 20:47, Peter Griffin wrote:
>>
>>> +
>>> + if (property)
>>> + pmu_np = of_parse_phandle(np, property, 0);
>>> + else
>>> + pmu_np = np;
>>> +
>>> + if (!pmu_np)
>>> + return ERR_PTR(-ENODEV);
>>> +
>>> + /*
>>> + * Determine if exynos-pmu device has probed and therefore regmap
>>> + * has been created and can be returned to the caller. Otherwise we
>>> + * return -EPROBE_DEFER.
>>> + */
>>> + dev = driver_find_device_by_of_node(&exynos_pmu_driver.driver,
>>> + (void *)pmu_np);
>>> +
>>> + of_node_put(pmu_np);
>>
>> You are dropping now referencen from np when property==NULL. This does
>> no look right.
>
> Good spot, will fix. It seems syscon.c and altera-sysmgr also have the
> same issue.
>
Do you plan on fixing them as well in such case?
Best regards,
Krzysztof
Hi Krzysztof,
On Tue, 20 Feb 2024 at 06:56, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 19/02/2024 20:47, Peter Griffin wrote:
> >>
> >>> +
> >>> + if (property)
> >>> + pmu_np = of_parse_phandle(np, property, 0);
> >>> + else
> >>> + pmu_np = np;
> >>> +
> >>> + if (!pmu_np)
> >>> + return ERR_PTR(-ENODEV);
> >>> +
> >>> + /*
> >>> + * Determine if exynos-pmu device has probed and therefore regmap
> >>> + * has been created and can be returned to the caller. Otherwise we
> >>> + * return -EPROBE_DEFER.
> >>> + */
> >>> + dev = driver_find_device_by_of_node(&exynos_pmu_driver.driver,
> >>> + (void *)pmu_np);
> >>> +
> >>> + of_node_put(pmu_np);
> >>
> >> You are dropping now referencen from np when property==NULL. This does
> >> no look right.
> >
> > Good spot, will fix. It seems syscon.c and altera-sysmgr also have the
> > same issue.
> >
>
> Do you plan on fixing them as well in such case?
Yes I'll send some patches to fix syscon and altera-sysmgr as well.
Thanks,
Peter