Hi folks,
This is a v2 of the series to add support for protected PMU registers found
on gs101 and derivative SoCs. In v2 it has been 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 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 v1:
- Refactor to use custom regmap to abstract SMC register access (Sam / Guenter)
- Add deferred probing support (Saravana / Krzysztof)
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/exynos-pmu.c | 227 ++++++++++++++++++++++++-
drivers/watchdog/Kconfig | 1 -
drivers/watchdog/s3c2410_wdt.c | 9 +-
include/linux/soc/samsung/exynos-pmu.h | 10 ++
4 files changed, 241 insertions(+), 6 deletions(-)
--
2.43.0.429.g432eaa2c6b-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.
Signed-off-by: Peter Griffin <[email protected]>
---
drivers/watchdog/Kconfig | 1 -
drivers/watchdog/s3c2410_wdt.c | 9 +++++----
2 files changed, 5 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..a1e2682c7e57 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,12 @@ 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.429.g432eaa2c6b-goog
Some Exynos based SoCs like Tensor gs101 protect the PMU registers for
security hardening reasons so that they are only 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]>
---
drivers/soc/samsung/exynos-pmu.c | 227 ++++++++++++++++++++++++-
include/linux/soc/samsung/exynos-pmu.h | 10 ++
2 files changed, 236 insertions(+), 1 deletion(-)
diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
index 250537d7cfd6..7bcc144e53a2 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,20 +13,159 @@
#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"
+static struct platform_driver exynos_pmu_driver;
+
+#define PMUALIVE_MASK GENMASK(14, 0)
+
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;
+/*
+ * Tensor SoCs are configured so that PMU_ALIVE registers can only be written
+ * from el3. As Linux needs to write some of these registers, the following
+ * SMC register read/write/read,write,modify interface is used.
+ *
+ * Note: This SMC interface is known to be implemented on gs101 and derivative
+ * SoCs.
+ */
+#define TENSOR_SMC_PMU_SEC_REG (0x82000504)
+#define TENSOR_PMUREG_READ 0
+#define TENSOR_PMUREG_WRITE 1
+#define TENSOR_PMUREG_RMW 2
+
+/**
+ * tensor_sec_reg_write
+ * Write to a protected SMC register.
+ * @base: Base address of PMU
+ * @reg: Address offset of register
+ * @val: Value to write
+ * Return: (0) on success
+ *
+ */
+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);
+
+ if (res.a0)
+ pr_warn("%s(): SMC failed: %lu\n", __func__, res.a0);
+
+ return (int)res.a0;
+}
+
+/**
+ * tensor_sec_reg_rmw
+ * Read/Modify/Write to a protected SMC register.
+ * @base: Base address of PMU
+ * @reg: Address offset of register
+ * @val: Value to write
+ * Return: (0) on success
+ *
+ */
+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);
+
+ if (res.a0)
+ pr_warn("%s(): SMC failed: %lu\n", __func__, res.a0);
+
+ return (int)res.a0;
+}
+
+/**
+ * tensor_sec_reg_read
+ * Read a protected SMC register.
+ * @base: Base address of PMU
+ * @reg: Address offset of register
+ * @val: Value read
+ * Return: (0) on success
+ */
+static int tensor_sec_reg_read(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_READ,
+ 0, 0, 0, 0, 0, &res);
+
+ *val = (unsigned int)res.a0;
+
+ 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_bit_atomic(0x3e80, 0x3f00, 0x3f00);
+ *
+ * To clear bits 13:8 in PMU offset 0x3e80
+ * tensor_set_bit_atomic(0x3e80, 0x0, 0x3f00);
+ */
+static inline void tensor_set_bit_atomic(void *ctx, unsigned int offset,
+ u32 val, u32 mask)
+{
+ unsigned int i;
+
+ for (i = 0; i < 32; i++) {
+ if (mask & BIT(i)) {
+ if (val & BIT(i)) {
+ offset |= 0xc000;
+ tensor_sec_reg_write(ctx, offset, i);
+ } else {
+ offset |= 0x8000;
+ tensor_sec_reg_write(ctx, offset, i);
+ }
+ }
+ }
+}
+
+int tensor_sec_update_bits(void *ctx, unsigned int reg, unsigned int mask, unsigned int val)
+{
+ int ret = 0;
+
+ /*
+ * 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);
+
+ tensor_set_bit_atomic(ctx, reg, val, mask);
+
+ return ret;
+}
+
void pmu_raw_writel(u32 val, u32 offset)
{
writel_relaxed(val, pmu_base_addr + offset);
@@ -80,6 +220,8 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode)
*/
static const struct of_device_id exynos_pmu_of_device_ids[] = {
{
+ .compatible = "google,gs101-pmu",
+ }, {
.compatible = "samsung,exynos3250-pmu",
.data = exynos_pmu_data_arm_ptr(exynos3250_pmu_data),
}, {
@@ -113,19 +255,73 @@ static const struct mfd_cell exynos_pmu_devs[] = {
{ .name = "exynos-clkout", },
};
+/**
+ * exynos_get_pmu_regmap
+ * Find the pmureg previously configured in probe() and return regmap property.
+ * Return: regmap if found or error if not found.
+ */
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
+ * Find the pmureg previously configured in probe() and return regmap property.
+ * Return: regmap if found or error if not found.
+ *
+ * @np: Pointer to device's Device Tree node
+ * @property: Device Tree property name which references the pmu
+ */
+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);
+
+ 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 struct regmap_config pmu_regs_regmap_cfg = {
+ .name = "pmu_regs",
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .fast_io = true,
+ .use_single_read = true,
+ .use_single_write = true,
+};
+
static int exynos_pmu_probe(struct platform_device *pdev)
{
+ struct resource *res;
+ struct regmap *regmap;
+ struct regmap_config pmuregmap_config = pmu_regs_regmap_cfg;
struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
int ret;
pmu_base_addr = devm_platform_ioremap_resource(pdev, 0);
@@ -137,6 +333,35 @@ static int exynos_pmu_probe(struct platform_device *pdev)
GFP_KERNEL);
if (!pmu_context)
return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -ENODEV;
+
+ pmuregmap_config.max_register = resource_size(res) -
+ pmuregmap_config.reg_stride;
+
+ if (of_device_is_compatible(np, "google,gs101-pmu")) {
+ pmuregmap_config.reg_read = tensor_sec_reg_read;
+ pmuregmap_config.reg_write = tensor_sec_reg_write;
+ pmuregmap_config.reg_update_bits = tensor_sec_update_bits;
+
+ /* Need physical address for SMC call */
+ regmap = devm_regmap_init(dev, NULL,
+ (void *)(uintptr_t)res->start,
+ &pmuregmap_config);
+ } else {
+ pmuregmap_config.max_register = resource_size(res) - 4;
+ regmap = devm_regmap_init_mmio(dev, pmu_base_addr,
+ &pmuregmap_config);
+ }
+
+ if (IS_ERR(regmap)) {
+ pr_err("regmap init failed\n");
+ return PTR_ERR(regmap);
+ }
+
+ pmu_context->pmureg = regmap;
pmu_context->dev = dev;
pmu_context->pmu_data = of_device_get_match_data(dev);
diff --git a/include/linux/soc/samsung/exynos-pmu.h b/include/linux/soc/samsung/exynos-pmu.h
index a4f5516cc956..68fb01ba6bef 100644
--- a/include/linux/soc/samsung/exynos-pmu.h
+++ b/include/linux/soc/samsung/exynos-pmu.h
@@ -21,11 +21,21 @@ 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);
+
+extern 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.429.g432eaa2c6b-goog
On Mon, Jan 29, 2024 at 3:19 PM 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 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]>
> ---
> drivers/soc/samsung/exynos-pmu.c | 227 ++++++++++++++++++++++++-
> include/linux/soc/samsung/exynos-pmu.h | 10 ++
> 2 files changed, 236 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> index 250537d7cfd6..7bcc144e53a2 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,20 +13,159 @@
> #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"
>
> +static struct platform_driver exynos_pmu_driver;
> +
> +#define PMUALIVE_MASK GENMASK(14, 0)
I'd advice to keep all #define's right after #include's block.
> +
> 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;
>
> +/*
> + * Tensor SoCs are configured so that PMU_ALIVE registers can only be written
> + * from el3. As Linux needs to write some of these registers, the following
Suggest changing el3 to EL3.
> + * SMC register read/write/read,write,modify interface is used.
Frankly, I don't really get what does this line mean.
> + *
> + * Note: This SMC interface is known to be implemented on gs101 and derivative
> + * SoCs.
> + */
> +#define TENSOR_SMC_PMU_SEC_REG (0x82000504)
Braces are probably not needed here.
> +#define TENSOR_PMUREG_READ 0
> +#define TENSOR_PMUREG_WRITE 1
> +#define TENSOR_PMUREG_RMW 2
I'd advice to keep all #define's right after #include's block.
> +
> +/**
> + * tensor_sec_reg_write
That doesn't look like a commonly used kernel-doc style. Please check
[1] and re-format accordingly. I'd also add that this function's
signature is quite self-explanatory, and it's also static, so I'm not
sure if it deserves kernel-doc comment or if it just makes things more
cluttered in this case. Maybe one-line regular comment will do here?
If you still thinks kernel-doc works better, please also check it with
$ scripts/kernel-doc -v -none drivers/soc/samsung/exynos-pmu.c
$ scripts/kernel-doc -v drivers/soc/samsung/exynos-pmu.c
The same comment goes for below kernel-doc functions.
[1] https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#writing-kernel-doc-comments
> + * Write to a protected SMC register.
> + * @base: Base address of PMU
> + * @reg: Address offset of register
> + * @val: Value to write
AFAIR, alignment with spaces is discouraged by kernel coding style.
> + * Return: (0) on success
Not sure if braces are needed around 0 here. Also, is it only 0 value,
or some other values can be returned?
> + *
This line is not needed.
> + */
> +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);
It can be 2 lines instead 4.
> +
> + if (res.a0)
> + pr_warn("%s(): SMC failed: %lu\n", __func__, res.a0);
> +
> + return (int)res.a0;
res.a0 are positive numbers, but in kernel the error codes are usually
negative numbers. I'm not sure if it's ok to use positive numbers for
regmap ops, but at least error codes should be documented.
> +}
> +
> +/**
> + * tensor_sec_reg_rmw
> + * Read/Modify/Write to a protected SMC register.
> + * @base: Base address of PMU
> + * @reg: Address offset of register
@mask is missing? Guess "make W=n" should complain, and kernel-doc too.
> + * @val: Value to write
> + * Return: (0) on success
> + *
> + */
> +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);
> +
> + if (res.a0)
> + pr_warn("%s(): SMC failed: %lu\n", __func__, res.a0);
> +
> + return (int)res.a0;
> +}
> +
> +/**
> + * tensor_sec_reg_read
> + * Read a protected SMC register.
> + * @base: Base address of PMU
> + * @reg: Address offset of register
> + * @val: Value read
> + * Return: (0) on success
> + */
> +static int tensor_sec_reg_read(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_READ,
> + 0, 0, 0, 0, 0, &res);
> +
> + *val = (unsigned int)res.a0;
> +
> + return 0;
> +}
> +
> +
Double empty line.
> +/*
> + * 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_bit_atomic(0x3e80, 0x3f00, 0x3f00);
> + *
> + * To clear bits 13:8 in PMU offset 0x3e80
> + * tensor_set_bit_atomic(0x3e80, 0x0, 0x3f00);
> + */
> +static inline void tensor_set_bit_atomic(void *ctx, unsigned int offset,
> + u32 val, u32 mask)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < 32; i++) {
> + if (mask & BIT(i)) {
Maybe use for_each_set_bit() or something like that?
> + if (val & BIT(i)) {
> + offset |= 0xc000;
> + tensor_sec_reg_write(ctx, offset, i);
> + } else {
> + offset |= 0x8000;
Magic number? Maybe makes sense to replace it with a named constant.
> + tensor_sec_reg_write(ctx, offset, i);
Common line, can be extracted out of if/else block.
> + }
> + }
> + }
> +}
> +
> +int tensor_sec_update_bits(void *ctx, unsigned int reg, unsigned int mask, unsigned int val)
Unnecessary exceeds 80 characters-per-line limit.
> +{
> + int ret = 0;
Why is this needed at all?
> +
> + /*
> + * 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);
> +
> + tensor_set_bit_atomic(ctx, reg, val, mask);
> +
> + return ret;
> +}
> +
> void pmu_raw_writel(u32 val, u32 offset)
> {
> writel_relaxed(val, pmu_base_addr + offset);
> @@ -80,6 +220,8 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode)
> */
> static const struct of_device_id exynos_pmu_of_device_ids[] = {
> {
> + .compatible = "google,gs101-pmu",
> + }, {
> .compatible = "samsung,exynos3250-pmu",
> .data = exynos_pmu_data_arm_ptr(exynos3250_pmu_data),
> }, {
> @@ -113,19 +255,73 @@ static const struct mfd_cell exynos_pmu_devs[] = {
> { .name = "exynos-clkout", },
> };
>
> +/**
> + * exynos_get_pmu_regmap
> + * Find the pmureg previously configured in probe() and return regmap property.
> + * Return: regmap if found or error if not found.
> + */
> 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);
Maybe move !np case handling into exynos_get_pmu_regmap_by_phandle()?
> return ERR_PTR(-ENODEV);
> }
> EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap);
>
> +/**
> + * exynos_get_pmu_regmap_by_phandle
> + * Find the pmureg previously configured in probe() and return regmap property.
> + * Return: regmap if found or error if not found.
> + *
> + * @np: Pointer to device's Device Tree node
> + * @property: Device Tree property name which references the pmu
> + */
> +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);
> +
> + 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 struct regmap_config pmu_regs_regmap_cfg = {
> + .name = "pmu_regs",
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .fast_io = true,
> + .use_single_read = true,
> + .use_single_write = true,
> +};
> +
> static int exynos_pmu_probe(struct platform_device *pdev)
> {
> + struct resource *res;
> + struct regmap *regmap;
> + struct regmap_config pmuregmap_config = pmu_regs_regmap_cfg;
Why copy that struct? IMHO, either use it as is, or if you want to
copy it for some particular reason, maybe make pmu_regs_regmap_cfg a
const?
Also, suggest reducing the variable name length. Maybe regmap_cfg would do?
> struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> int ret;
>
> pmu_base_addr = devm_platform_ioremap_resource(pdev, 0);
> @@ -137,6 +333,35 @@ static int exynos_pmu_probe(struct platform_device *pdev)
> GFP_KERNEL);
> if (!pmu_context)
> return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENODEV;
> +
> + pmuregmap_config.max_register = resource_size(res) -
> + pmuregmap_config.reg_stride;
> +
> + if (of_device_is_compatible(np, "google,gs101-pmu")) {
> + pmuregmap_config.reg_read = tensor_sec_reg_read;
> + pmuregmap_config.reg_write = tensor_sec_reg_write;
> + pmuregmap_config.reg_update_bits = tensor_sec_update_bits;
> +
> + /* Need physical address for SMC call */
> + regmap = devm_regmap_init(dev, NULL,
> + (void *)(uintptr_t)res->start,
> + &pmuregmap_config);
> + } else {
> + pmuregmap_config.max_register = resource_size(res) - 4;
> + regmap = devm_regmap_init_mmio(dev, pmu_base_addr,
> + &pmuregmap_config);
> + }
> +
> + if (IS_ERR(regmap)) {
> + pr_err("regmap init failed\n");
dev_err()? Or even better, return dev_err_probe()?
> + return PTR_ERR(regmap);
> + }
> +
> + pmu_context->pmureg = regmap;
> pmu_context->dev = dev;
> pmu_context->pmu_data = of_device_get_match_data(dev);
>
> diff --git a/include/linux/soc/samsung/exynos-pmu.h b/include/linux/soc/samsung/exynos-pmu.h
> index a4f5516cc956..68fb01ba6bef 100644
> --- a/include/linux/soc/samsung/exynos-pmu.h
> +++ b/include/linux/soc/samsung/exynos-pmu.h
> @@ -21,11 +21,21 @@ 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);
> +
> +extern struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
> + const char *property);
Why use "extern" here, it's just a function declaration.
> +
Either remove this empty line, or add more empty lines around all
parts of #ifdef block for consistency.
> #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.429.g432eaa2c6b-goog
>
On Mon, Jan 29, 2024 at 4:25 PM Saravana Kannan <[email protected]> wrote:
>
> On Mon, Jan 29, 2024 at 1:19 PM 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.
> >
> > Signed-off-by: Peter Griffin <[email protected]>
> > ---
> > drivers/watchdog/Kconfig | 1 -
> > drivers/watchdog/s3c2410_wdt.c | 9 +++++----
> > 2 files changed, 5 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
That reminds me: now that exynos-pmu driver uses regmap API, does it
make sense to add something like "select REGMAP" to EXYNOS_PMU option?
> > 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..a1e2682c7e57 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,12 @@ 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");
>
This looks so much better than approach taken in v1, as for my taste.
For this patch:
Reviewed-by: Sam Protsenko <[email protected]>
> IIUC, the exynos PMU driver is registering a regmap interface with
> regmap framework. So, can't we get the remap from the framework
> instead of directly talking to the PMU driver?
>
Peter is basically re-implementing syscon driver with overridden
operations, as a part of exynos-pmu driver, in previous patch. Which
means syscon API can't be used anymore to obtain the regmap. Do you
have particular API in mind that allows getting a random regmap
registered with devm_regmap_init()?
> -Saravana
>
> > 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.429.g432eaa2c6b-goog
> >
On 1/29/24 13:19, Peter Griffin wrote:
> Some Exynos based SoCs like Tensor gs101 protect the PMU registers for
> security hardening reasons so that they are only 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]>
> ---
[ ... ]
> +/**
> + * exynos_get_pmu_regmap
> + * Find the pmureg previously configured in probe() and return regmap property.
> + * Return: regmap if found or error if not found.
> + */
> 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
> + * Find the pmureg previously configured in probe() and return regmap property.
> + * Return: regmap if found or error if not found.
> + *
> + * @np: Pointer to device's Device Tree node
> + * @property: Device Tree property name which references the pmu
> + */
> +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);
> +
> + 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);
> +
I think there should be a detailed comment explaining why the complexity
is necessary instead of just returning pmu_context->pmureg.
Thanks,
Guenter
Hi Sam,
Thanks for the review feedback.
On Mon, 29 Jan 2024 at 23:01, Sam Protsenko <[email protected]> wrote:
>
> On Mon, Jan 29, 2024 at 3:19 PM 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 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]>
> > ---
> > drivers/soc/samsung/exynos-pmu.c | 227 ++++++++++++++++++++++++-
> > include/linux/soc/samsung/exynos-pmu.h | 10 ++
> > 2 files changed, 236 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> > index 250537d7cfd6..7bcc144e53a2 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,20 +13,159 @@
> > #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"
> >
> > +static struct platform_driver exynos_pmu_driver;
> > +
> > +#define PMUALIVE_MASK GENMASK(14, 0)
>
> I'd advice to keep all #define's right after #include's block.
OK will move
>
> > +
> > 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;
> >
> > +/*
> > + * Tensor SoCs are configured so that PMU_ALIVE registers can only be written
> > + * from el3. As Linux needs to write some of these registers, the following
>
> Suggest changing el3 to EL3.
Will fix
>
> > + * SMC register read/write/read,write,modify interface is used.
>
> Frankly, I don't really get what does this line mean.
It was just trying to describe the 3 defines below (PMUREG_READ,
PMUREG_WRITE, PMUREG_RMW but if it is unclear then I will remove it.
The idea of the comment was to make things clearer, not add confusion
;-)
>
> > + *
> > + * Note: This SMC interface is known to be implemented on gs101 and derivative
> > + * SoCs.
> > + */
> > +#define TENSOR_SMC_PMU_SEC_REG (0x82000504)
>
> Braces are probably not needed here.
Will remove
>
> > +#define TENSOR_PMUREG_READ 0
> > +#define TENSOR_PMUREG_WRITE 1
> > +#define TENSOR_PMUREG_RMW 2
>
> I'd advice to keep all #define's right after #include's block.
Will move
>
> > +
> > +/**
> > + * tensor_sec_reg_write
>
> That doesn't look like a commonly used kernel-doc style. Please check
> [1] and re-format accordingly. I'd also add that this function's
> signature is quite self-explanatory, and it's also static, so I'm not
> sure if it deserves kernel-doc comment or if it just makes things more
> cluttered in this case. Maybe one-line regular comment will do here?
Ok will update to one line comment
> If you still thinks kernel-doc works better, please also check it with
>
> $ scripts/kernel-doc -v -none drivers/soc/samsung/exynos-pmu.c
> $ scripts/kernel-doc -v drivers/soc/samsung/exynos-pmu.c
>
> The same comment goes for below kernel-doc functions.
>
> [1] https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#writing-kernel-doc-comments
Thanks for the references/pointers.
>
> > + * Write to a protected SMC register.
> > + * @base: Base address of PMU
> > + * @reg: Address offset of register
> > + * @val: Value to write
>
> AFAIR, alignment with spaces is discouraged by kernel coding style.
>
> > + * Return: (0) on success
>
> Not sure if braces are needed around 0 here. Also, is it only 0 value,
> or some other values can be returned?
I don't have access to the bootloader code, but I will try and check this point.
>
> > + *
>
> This line is not needed.
Will fix
>
> > + */
> > +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);
>
> It can be 2 lines instead 4.
Will fix
>
> > +
> > + if (res.a0)
> > + pr_warn("%s(): SMC failed: %lu\n", __func__, res.a0);
> > +
> > + return (int)res.a0;
>
> res.a0 are positive numbers, but in kernel the error codes are usually
> negative numbers. I'm not sure if it's ok to use positive numbers for
> regmap ops, but at least error codes should be documented.
I will see if I can get more information about the error codes
returned. I don't have access to firmware code though so that may not
be possible. The downstream production kernel returned `(int)res.a0`
as an error for functions returning int. So I believe this is fine.
>
> > +}
> > +
> > +/**
> > + * tensor_sec_reg_rmw
> > + * Read/Modify/Write to a protected SMC register.
> > + * @base: Base address of PMU
> > + * @reg: Address offset of register
>
> @mask is missing? Guess "make W=n" should complain, and kernel-doc too.
Will update to a one line comment as suggested above.
>
> > + * @val: Value to write
> > + * Return: (0) on success
> > + *
> > + */
> > +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);
> > +
> > + if (res.a0)
> > + pr_warn("%s(): SMC failed: %lu\n", __func__, res.a0);
> > +
> > + return (int)res.a0;
> > +}
> > +
> > +/**
> > + * tensor_sec_reg_read
> > + * Read a protected SMC register.
> > + * @base: Base address of PMU
> > + * @reg: Address offset of register
> > + * @val: Value read
> > + * Return: (0) on success
> > + */
> > +static int tensor_sec_reg_read(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_READ,
> > + 0, 0, 0, 0, 0, &res);
> > +
> > + *val = (unsigned int)res.a0;
> > +
> > + return 0;
> > +}
> > +
> > +
>
> Double empty line.
Will fix
>
> > +/*
> > + * 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_bit_atomic(0x3e80, 0x3f00, 0x3f00);
> > + *
> > + * To clear bits 13:8 in PMU offset 0x3e80
> > + * tensor_set_bit_atomic(0x3e80, 0x0, 0x3f00);
> > + */
> > +static inline void tensor_set_bit_atomic(void *ctx, unsigned int offset,
> > + u32 val, u32 mask)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < 32; i++) {
> > + if (mask & BIT(i)) {
>
> Maybe use for_each_set_bit() or something like that?
I'll take a look and see if it looks better.
>
> > + if (val & BIT(i)) {
> > + offset |= 0xc000;
> > + tensor_sec_reg_write(ctx, offset, i);
> > + } else {
> > + offset |= 0x8000;
>
> Magic number? Maybe makes sense to replace it with a named constant.
Will fix
>
> > + tensor_sec_reg_write(ctx, offset, i);
>
> Common line, can be extracted out of if/else block.
Will fix
>
> > + }
> > + }
> > + }
> > +}
> > +
> > +int tensor_sec_update_bits(void *ctx, unsigned int reg, unsigned int mask, unsigned int val)
>
> Unnecessary exceeds 80 characters-per-line limit.
Will fix
>
> > +{
> > + int ret = 0;
>
> Why is this needed at all?
I will re-work that to propagate the error from tensor_sec_reg_write()
>
> > +
> > + /*
> > + * 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);
> > +
> > + tensor_set_bit_atomic(ctx, reg, val, mask);
> > +
> > + return ret;
> > +}
> > +
> > void pmu_raw_writel(u32 val, u32 offset)
> > {
> > writel_relaxed(val, pmu_base_addr + offset);
> > @@ -80,6 +220,8 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode)
> > */
> > static const struct of_device_id exynos_pmu_of_device_ids[] = {
> > {
> > + .compatible = "google,gs101-pmu",
> > + }, {
> > .compatible = "samsung,exynos3250-pmu",
> > .data = exynos_pmu_data_arm_ptr(exynos3250_pmu_data),
> > }, {
> > @@ -113,19 +255,73 @@ static const struct mfd_cell exynos_pmu_devs[] = {
> > { .name = "exynos-clkout", },
> > };
> >
> > +/**
> > + * exynos_get_pmu_regmap
> > + * Find the pmureg previously configured in probe() and return regmap property.
> > + * Return: regmap if found or error if not found.
> > + */
> > 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);
>
> Maybe move !np case handling into exynos_get_pmu_regmap_by_phandle()?
I did consider doing that but decided against it. The idea is to have
the same behaviour as syscon_regmap_lookup_by_phandle() and
altr_sysmgr_regmap_lookup_by_phandle().
>
> > return ERR_PTR(-ENODEV);
> > }
> > EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap);
> >
> > +/**
> > + * exynos_get_pmu_regmap_by_phandle
> > + * Find the pmureg previously configured in probe() and return regmap property.
> > + * Return: regmap if found or error if not found.
> > + *
> > + * @np: Pointer to device's Device Tree node
> > + * @property: Device Tree property name which references the pmu
> > + */
> > +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);
> > +
> > + 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 struct regmap_config pmu_regs_regmap_cfg = {
> > + .name = "pmu_regs",
> > + .reg_bits = 32,
> > + .reg_stride = 4,
> > + .val_bits = 32,
> > + .fast_io = true,
> > + .use_single_read = true,
> > + .use_single_write = true,
> > +};
> > +
> > static int exynos_pmu_probe(struct platform_device *pdev)
> > {
> > + struct resource *res;
> > + struct regmap *regmap;
> > + struct regmap_config pmuregmap_config = pmu_regs_regmap_cfg;
>
> Why copy that struct? IMHO, either use it as is, or if you want to
> copy it for some particular reason, maybe make pmu_regs_regmap_cfg a
> const?
>
> Also, suggest reducing the variable name length. Maybe regmap_cfg would do?
will fix
>
> > struct device *dev = &pdev->dev;
> > + struct device_node *np = dev->of_node;
> > int ret;
> >
> > pmu_base_addr = devm_platform_ioremap_resource(pdev, 0);
> > @@ -137,6 +333,35 @@ static int exynos_pmu_probe(struct platform_device *pdev)
> > GFP_KERNEL);
> > if (!pmu_context)
> > return -ENOMEM;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res)
> > + return -ENODEV;
> > +
> > + pmuregmap_config.max_register = resource_size(res) -
> > + pmuregmap_config.reg_stride;
> > +
> > + if (of_device_is_compatible(np, "google,gs101-pmu")) {
> > + pmuregmap_config.reg_read = tensor_sec_reg_read;
> > + pmuregmap_config.reg_write = tensor_sec_reg_write;
> > + pmuregmap_config.reg_update_bits = tensor_sec_update_bits;
> > +
> > + /* Need physical address for SMC call */
> > + regmap = devm_regmap_init(dev, NULL,
> > + (void *)(uintptr_t)res->start,
> > + &pmuregmap_config);
> > + } else {
> > + pmuregmap_config.max_register = resource_size(res) - 4;
> > + regmap = devm_regmap_init_mmio(dev, pmu_base_addr,
> > + &pmuregmap_config);
> > + }
> > +
> > + if (IS_ERR(regmap)) {
> > + pr_err("regmap init failed\n");
>
> dev_err()? Or even better, return dev_err_probe()?
Will update to dev_err_probe()
>
> > + return PTR_ERR(regmap);
> > + }
> > +
> > + pmu_context->pmureg = regmap;
> > pmu_context->dev = dev;
> > pmu_context->pmu_data = of_device_get_match_data(dev);
> >
> > diff --git a/include/linux/soc/samsung/exynos-pmu.h b/include/linux/soc/samsung/exynos-pmu.h
> > index a4f5516cc956..68fb01ba6bef 100644
> > --- a/include/linux/soc/samsung/exynos-pmu.h
> > +++ b/include/linux/soc/samsung/exynos-pmu.h
> > @@ -21,11 +21,21 @@ 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);
> > +
> > +extern struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
> > + const char *property);
>
> Why use "extern" here, it's just a function declaration.
Will fix. I see that mfd/syscon.h is actually declared the same with
extern which is likely why this ended up here.
>
> > +
>
> Either remove this empty line, or add more empty lines around all
> parts of #ifdef block for consistency.
Will fix
regards,
Peter
Hi Guenter,
Thanks for the feedback.
On Tue, 30 Jan 2024 at 06:26, Guenter Roeck <[email protected]> wrote:
>
> On 1/29/24 13:19, Peter Griffin wrote:
> > Some Exynos based SoCs like Tensor gs101 protect the PMU registers for
> > security hardening reasons so that they are only 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]>
> > ---
> [ ... ]
>
> > +/**
> > + * exynos_get_pmu_regmap
> > + * Find the pmureg previously configured in probe() and return regmap property.
> > + * Return: regmap if found or error if not found.
> > + */
> > 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
> > + * Find the pmureg previously configured in probe() and return regmap property.
> > + * Return: regmap if found or error if not found.
> > + *
> > + * @np: Pointer to device's Device Tree node
> > + * @property: Device Tree property name which references the pmu
> > + */
> > +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);
> > +
> > + 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);
> > +
>
> I think there should be a detailed comment explaining why the complexity
> is necessary instead of just returning pmu_context->pmureg.
Ok, I'll add a detailed comment for v3.
Thanks,
Peter
Hi Sam & Saravana,
On Tue, 30 Jan 2024 at 03:38, Sam Protsenko <[email protected]> wrote:
>
> On Mon, Jan 29, 2024 at 4:25 PM Saravana Kannan <saravanak@googlecom> wrote:
> >
> > On Mon, Jan 29, 2024 at 1:19 PM 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.
> > >
> > > Signed-off-by: Peter Griffin <[email protected]>
> > > ---
> > > drivers/watchdog/Kconfig | 1 -
> > > drivers/watchdog/s3c2410_wdt.c | 9 +++++----
> > > 2 files changed, 5 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
>
> That reminds me: now that exynos-pmu driver uses regmap API, does it
> make sense to add something like "select REGMAP" to EXYNOS_PMU option?
Good point, I will add that in v3.
>
> > > 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..a1e2682c7e57 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,12 @@ 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");
> >
>
> This looks so much better than approach taken in v1, as for my taste.
> For this patch:
>
> Reviewed-by: Sam Protsenko <[email protected]>
Thanks!
>
> > IIUC, the exynos PMU driver is registering a regmap interface with
> > regmap framework. So, can't we get the remap from the framework
> > instead of directly talking to the PMU driver?
> >
I'm not aware of any existing regmap API that does that. A quick look
through regmap code I can't see any global state that is stored on a
regmap_init() for example of all the regmaps created in the system.
As Sam mentions below, prior to these patches the syscon device would
be creating the PMU mmio regmap and consumers drivers would be
obtaining the regmap by going through the syscon driver. After this
series we instead talk to the exynos-pmu driver to obtain the pmu
regmap which can either be one with overridden operations for issuing
SMC calls to do the register accesses or still a mmio regmap.
Folks tried in the past to add a SMC backend to regmap similar to
regmap_mmio and add support for it into syscon driver but this was
nacked (see here
https://lore.kernel.org/lkml/[email protected]/T/)
>
> Peter is basically re-implementing syscon driver with overridden
> operations, as a part of exynos-pmu driver, in previous patch. Which
> means syscon API can't be used anymore to obtain the regmap. Do you
> have particular API in mind that allows getting a random regmap
> registered with devm_regmap_init()?
Thanks,
Peter
On 29/01/2024 22:19, Peter Griffin wrote:
> Some Exynos based SoCs like Tensor gs101 protect the PMU registers for
> security hardening reasons so that they are only 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]>
> ---
> drivers/soc/samsung/exynos-pmu.c | 227 ++++++++++++++++++++++++-
> include/linux/soc/samsung/exynos-pmu.h | 10 ++
> 2 files changed, 236 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> index 250537d7cfd6..7bcc144e53a2 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,20 +13,159 @@
> #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"
>
> +static struct platform_driver exynos_pmu_driver;
I don't understand why do you need it. You can have only one
pmu_context. The moment you probe second one, previous becomes invalid.
I guess you want to parse phandle and check if just in case if it points
to the right device, but still the original code is not ready for two
PMU devices. I say either this problem should be solved entirely,
allowing two devices, or just compare device node from phandle with
device node of exynos_pmu_context->dev and return -EINVAL on mismatches.
Anyway, keep all file scope declarations together.
> +
> +#define PMUALIVE_MASK GENMASK(14, 0)
> +
> 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;
>
> +/*
> + * Tensor SoCs are configured so that PMU_ALIVE registers can only be written
> + * from el3. As Linux needs to write some of these registers, the following
> + * SMC register read/write/read,write,modify interface is used.
> + *
> + * Note: This SMC interface is known to be implemented on gs101 and derivative
> + * SoCs.
> + */
> +#define TENSOR_SMC_PMU_SEC_REG (0x82000504)
> +#define TENSOR_PMUREG_READ 0
> +#define TENSOR_PMUREG_WRITE 1
> +#define TENSOR_PMUREG_RMW 2
> +
> +/**
> + * tensor_sec_reg_write
> + * Write to a protected SMC register.
> + * @base: Base address of PMU
> + * @reg: Address offset of register
> + * @val: Value to write
> + * Return: (0) on success
> + *
This does not really look like kerneldoc...
> + */
> +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);
> +
> + if (res.a0)
> + pr_warn("%s(): SMC failed: %lu\n", __func__, res.a0);
> +
> + return (int)res.a0;
> +}
> +
> +/**
> + * tensor_sec_reg_rmw
> + * Read/Modify/Write to a protected SMC register.
> + * @base: Base address of PMU
> + * @reg: Address offset of register
> + * @val: Value to write
> + * Return: (0) on success
> + *
> + */
> +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);
> +
> + if (res.a0)
> + pr_warn("%s(): SMC failed: %lu\n", __func__, res.a0);
> +
> + return (int)res.a0;
> +}
> +
> +/**
> + * tensor_sec_reg_read
> + * Read a protected SMC register.
> + * @base: Base address of PMU
> + * @reg: Address offset of register
> + * @val: Value read
> + * Return: (0) on success
> + */
> +static int tensor_sec_reg_read(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_READ,
> + 0, 0, 0, 0, 0, &res);
> +
> + *val = (unsigned int)res.a0;
> +
> + 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_bit_atomic(0x3e80, 0x3f00, 0x3f00);
> + *
> + * To clear bits 13:8 in PMU offset 0x3e80
> + * tensor_set_bit_atomic(0x3e80, 0x0, 0x3f00);
> + */
> +static inline void tensor_set_bit_atomic(void *ctx, unsigned int offset,
> + u32 val, u32 mask)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < 32; i++) {
> + if (mask & BIT(i)) {
> + if (val & BIT(i)) {
> + offset |= 0xc000;
> + tensor_sec_reg_write(ctx, offset, i);
> + } else {
> + offset |= 0x8000;
> + tensor_sec_reg_write(ctx, offset, i);
> + }
> + }
> + }
> +}
> +
> +int tensor_sec_update_bits(void *ctx, unsigned int reg, unsigned int mask, unsigned int val)
> +{
> + int ret = 0;
> +
> + /*
> + * 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);
> +
> + tensor_set_bit_atomic(ctx, reg, val, mask);
> +
> + return ret;
> +}
> +
> void pmu_raw_writel(u32 val, u32 offset)
> {
> writel_relaxed(val, pmu_base_addr + offset);
> @@ -80,6 +220,8 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode)
> */
> static const struct of_device_id exynos_pmu_of_device_ids[] = {
> {
> + .compatible = "google,gs101-pmu",
> + }, {
> .compatible = "samsung,exynos3250-pmu",
> .data = exynos_pmu_data_arm_ptr(exynos3250_pmu_data),
> }, {
> @@ -113,19 +255,73 @@ static const struct mfd_cell exynos_pmu_devs[] = {
> { .name = "exynos-clkout", },
> };
>
> +/**
> + * exynos_get_pmu_regmap
> + * Find the pmureg previously configured in probe() and return regmap property.
> + * Return: regmap if found or error if not found.
> + */
> 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
> + * Find the pmureg previously configured in probe() and return regmap property.
> + * Return: regmap if found or error if not found.
Return is the last. This does not look tested - make htmldocs, make W=1
> + *
> + * @np: Pointer to device's Device Tree node
> + * @property: Device Tree property name which references the pmu
> + */
> +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);
> +
> + 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 struct regmap_config pmu_regs_regmap_cfg = {
> + .name = "pmu_regs",
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .fast_io = true,
> + .use_single_read = true,
> + .use_single_write = true,
> +};
> +
> static int exynos_pmu_probe(struct platform_device *pdev)
> {
> + struct resource *res;
> + struct regmap *regmap;
> + struct regmap_config pmuregmap_config = pmu_regs_regmap_cfg;
> struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
Please do not mix order declarations with and without initializations. I
propose first ones with initializations, followed by ones without.
> int ret;
>
> pmu_base_addr = devm_platform_ioremap_resource(pdev, 0);
> @@ -137,6 +333,35 @@ static int exynos_pmu_probe(struct platform_device *pdev)
> GFP_KERNEL);
> if (!pmu_context)
> return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENODEV;
> +
> + pmuregmap_config.max_register = resource_size(res) -
> + pmuregmap_config.reg_stride;
> +
> + if (of_device_is_compatible(np, "google,gs101-pmu")) {
No compatibles inside the probe. Use driver match data. This applies to
all drivers in all subsystems.
> + pmuregmap_config.reg_read = tensor_sec_reg_read;
> + pmuregmap_config.reg_write = tensor_sec_reg_write;
> + pmuregmap_config.reg_update_bits = tensor_sec_update_bits;
No, regmap_config should be const and please use match data.
> +
> + /* Need physical address for SMC call */
> + regmap = devm_regmap_init(dev, NULL,
> + (void *)(uintptr_t)res->start,
> + &pmuregmap_config);
> + } else {
> + pmuregmap_config.max_register = resource_size(res) - 4;
> + regmap = devm_regmap_init_mmio(dev, pmu_base_addr,
> + &pmuregmap_config);
> + }
> +
> + if (IS_ERR(regmap)) {
> + pr_err("regmap init failed\n");
dev_err
> + return PTR_ERR(regmap);
> + }
> +
> + pmu_context->pmureg = regmap;
> pmu_context->dev = dev;
> pmu_context->pmu_data = of_device_get_match_data(dev);
>
> diff --git a/include/linux/soc/samsung/exynos-pmu.h b/include/linux/soc/samsung/exynos-pmu.h
> index a4f5516cc956..68fb01ba6bef 100644
> --- a/include/linux/soc/samsung/exynos-pmu.h
> +++ b/include/linux/soc/samsung/exynos-pmu.h
> @@ -21,11 +21,21 @@ 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);
> +
> +extern struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
> + const char *property);
Drop extern from new code. I understand it makes it inconsistent but it
extern does not matter, so at some point we will clean all existing code...
Best regards,
Krzysztof
Hi Krzysztof,
Thanks for the review feedback.
On Tue, 30 Jan 2024 at 16:01, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 29/01/2024 22:19, Peter Griffin wrote:
> > Some Exynos based SoCs like Tensor gs101 protect the PMU registers for
> > security hardening reasons so that they are only 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]>
> > ---
> > drivers/soc/samsung/exynos-pmu.c | 227 ++++++++++++++++++++++++-
> > include/linux/soc/samsung/exynos-pmu.h | 10 ++
> > 2 files changed, 236 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> > index 250537d7cfd6..7bcc144e53a2 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,20 +13,159 @@
> > #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"
> >
> > +static struct platform_driver exynos_pmu_driver;
>
> I don't understand why do you need it. You can have only one
> pmu_context. The moment you probe second one, previous becomes invalid.
>
> I guess you want to parse phandle and check if just in case if it points
> to the right device, but still the original code is not ready for two
> PMU devices. I say either this problem should be solved entirely,
> allowing two devices, or just compare device node from phandle with
> device node of exynos_pmu_context->dev and return -EINVAL on mismatches.
I'll update like you suggest in v3.
>
> Anyway, keep all file scope declarations together.
OK
>
>
> > +
> > +#define PMUALIVE_MASK GENMASK(14, 0)
> > +
> > 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;
> >
> > +/*
> > + * Tensor SoCs are configured so that PMU_ALIVE registers can only be written
> > + * from el3. As Linux needs to write some of these registers, the following
> > + * SMC register read/write/read,write,modify interface is used.
> > + *
> > + * Note: This SMC interface is known to be implemented on gs101 and derivative
> > + * SoCs.
> > + */
> > +#define TENSOR_SMC_PMU_SEC_REG (0x82000504)
> > +#define TENSOR_PMUREG_READ 0
> > +#define TENSOR_PMUREG_WRITE 1
> > +#define TENSOR_PMUREG_RMW 2
> > +
> > +/**
> > + * tensor_sec_reg_write
> > + * Write to a protected SMC register.
> > + * @base: Base address of PMU
> > + * @reg: Address offset of register
> > + * @val: Value to write
> > + * Return: (0) on success
> > + *
>
> This does not really look like kerneldoc...
Sam suggested that I remove the kerneldoc and replace with a one line
comment in v3
>
> > + */
> > +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);
> > +
> > + if (res.a0)
> > + pr_warn("%s(): SMC failed: %lu\n", __func__, res.a0);
> > +
> > + return (int)res.a0;
> > +}
> > +
> > +/**
> > + * tensor_sec_reg_rmw
> > + * Read/Modify/Write to a protected SMC register.
> > + * @base: Base address of PMU
> > + * @reg: Address offset of register
> > + * @val: Value to write
> > + * Return: (0) on success
> > + *
> > + */
> > +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);
> > +
> > + if (res.a0)
> > + pr_warn("%s(): SMC failed: %lu\n", __func__, res.a0);
> > +
> > + return (int)res.a0;
> > +}
> > +
> > +/**
> > + * tensor_sec_reg_read
> > + * Read a protected SMC register.
> > + * @base: Base address of PMU
> > + * @reg: Address offset of register
> > + * @val: Value read
> > + * Return: (0) on success
> > + */
> > +static int tensor_sec_reg_read(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_READ,
> > + 0, 0, 0, 0, 0, &res);
> > +
> > + *val = (unsigned int)res.a0;
> > +
> > + 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_bit_atomic(0x3e80, 0x3f00, 0x3f00);
> > + *
> > + * To clear bits 13:8 in PMU offset 0x3e80
> > + * tensor_set_bit_atomic(0x3e80, 0x0, 0x3f00);
> > + */
> > +static inline void tensor_set_bit_atomic(void *ctx, unsigned int offset,
> > + u32 val, u32 mask)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < 32; i++) {
> > + if (mask & BIT(i)) {
> > + if (val & BIT(i)) {
> > + offset |= 0xc000;
> > + tensor_sec_reg_write(ctx, offset, i);
> > + } else {
> > + offset |= 0x8000;
> > + tensor_sec_reg_write(ctx, offset, i);
> > + }
> > + }
> > + }
> > +}
> > +
> > +int tensor_sec_update_bits(void *ctx, unsigned int reg, unsigned int mask, unsigned int val)
> > +{
> > + int ret = 0;
> > +
> > + /*
> > + * 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);
> > +
> > + tensor_set_bit_atomic(ctx, reg, val, mask);
> > +
> > + return ret;
> > +}
> > +
> > void pmu_raw_writel(u32 val, u32 offset)
> > {
> > writel_relaxed(val, pmu_base_addr + offset);
> > @@ -80,6 +220,8 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode)
> > */
> > static const struct of_device_id exynos_pmu_of_device_ids[] = {
> > {
> > + .compatible = "google,gs101-pmu",
> > + }, {
> > .compatible = "samsung,exynos3250-pmu",
> > .data = exynos_pmu_data_arm_ptr(exynos3250_pmu_data),
> > }, {
> > @@ -113,19 +255,73 @@ static const struct mfd_cell exynos_pmu_devs[] = {
> > { .name = "exynos-clkout", },
> > };
> >
> > +/**
> > + * exynos_get_pmu_regmap
> > + * Find the pmureg previously configured in probe() and return regmap property.
> > + * Return: regmap if found or error if not found.
> > + */
> > 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
> > + * Find the pmureg previously configured in probe() and return regmap property.
> > + * Return: regmap if found or error if not found.
>
> Return is the last. This does not look tested - make htmldocs, make W=1
Thanks, will fix and test in v3.
>
> > + *
> > + * @np: Pointer to device's Device Tree node
> > + * @property: Device Tree property name which references the pmu
> > + */
> > +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);
> > +
> > + 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 struct regmap_config pmu_regs_regmap_cfg = {
> > + .name = "pmu_regs",
> > + .reg_bits = 32,
> > + .reg_stride = 4,
> > + .val_bits = 32,
> > + .fast_io = true,
> > + .use_single_read = true,
> > + .use_single_write = true,
> > +};
> > +
> > static int exynos_pmu_probe(struct platform_device *pdev)
> > {
> > + struct resource *res;
> > + struct regmap *regmap;
> > + struct regmap_config pmuregmap_config = pmu_regs_regmap_cfg;
> > struct device *dev = &pdev->dev;
> > + struct device_node *np = dev->of_node;
>
> Please do not mix order declarations with and without initializations. I
> propose first ones with initializations, followed by ones without.
Will fix.
>
> > int ret;
> >
> > pmu_base_addr = devm_platform_ioremap_resource(pdev, 0);
> > @@ -137,6 +333,35 @@ static int exynos_pmu_probe(struct platform_device *pdev)
> > GFP_KERNEL);
> > if (!pmu_context)
> > return -ENOMEM;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res)
> > + return -ENODEV;
> > +
> > + pmuregmap_config.max_register = resource_size(res) -
> > + pmuregmap_config.reg_stride;
> > +
> > + if (of_device_is_compatible(np, "google,gs101-pmu")) {
>
> No compatibles inside the probe. Use driver match data. This applies to
> all drivers in all subsystems.
Noted, will fix in v3.
>
> > + pmuregmap_config.reg_read = tensor_sec_reg_read;
> > + pmuregmap_config.reg_write = tensor_sec_reg_write;
> > + pmuregmap_config.reg_update_bits = tensor_sec_update_bits;
>
> No, regmap_config should be const and please use match data.
Are you sure you want the regmap_config struct const?
In my draft v3 I have implemented it so far having a regmap_smccfg
struct which sets all the configuration apart from max_register field
(used by gs101) and a regmap_mmiocfg struct (used by all other
exynos-pmu SoCs). The choice over which regmap_config to register is
made via match data exynos_pmu_data flag 'pmu_secure' which is set
only for gs101. That avoids having to define exynos_pmu_data structs
for the other exynos SoCs that currently don't really need them
(exynos7, exynos850, exynos5443, exyno5410 etc).
But I still wish to set at runtime the regmap_config.max_register
field based on the resource size coming from DT. Having the structs
const would prohibit that and mean we need to specify many more
regmap_config structs where the only difference is the max_register
field.
Is the above approach acceptable for you?
>
> > +
> > + /* Need physical address for SMC call */
> > + regmap = devm_regmap_init(dev, NULL,
> > + (void *)(uintptr_t)res->start,
> > + &pmuregmap_config);
> > + } else {
> > + pmuregmap_config.max_register = resource_size(res) - 4;
> > + regmap = devm_regmap_init_mmio(dev, pmu_base_addr,
> > + &pmuregmap_config);
> > + }
> > +
> > + if (IS_ERR(regmap)) {
> > + pr_err("regmap init failed\n");
>
> dev_err
Will fix
>
> > + return PTR_ERR(regmap);
> > + }
> > +
> > + pmu_context->pmureg = regmap;
> > pmu_context->dev = dev;
> > pmu_context->pmu_data = of_device_get_match_data(dev);
> >
> > diff --git a/include/linux/soc/samsung/exynos-pmu.h b/include/linux/soc/samsung/exynos-pmu.h
> > index a4f5516cc956..68fb01ba6bef 100644
> > --- a/include/linux/soc/samsung/exynos-pmu.h
> > +++ b/include/linux/soc/samsung/exynos-pmu.h
> > @@ -21,11 +21,21 @@ 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);
> > +
> > +extern struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
> > + const char *property);
>
> Drop extern from new code. I understand it makes it inconsistent but it
> extern does not matter, so at some point we will clean all existing code...
Will fix
Thanks,
Peter
Hi Krzysztof,
On Tue, 30 Jan 2024 at 16:01, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 29/01/2024 22:19, Peter Griffin wrote:
> > Some Exynos based SoCs like Tensor gs101 protect the PMU registers for
> > security hardening reasons so that they are only 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]>
> > ---
> > drivers/soc/samsung/exynos-pmu.c | 227 ++++++++++++++++++++++++-
> > include/linux/soc/samsung/exynos-pmu.h | 10 ++
> > 2 files changed, 236 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> > index 250537d7cfd6..7bcc144e53a2 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,20 +13,159 @@
> > #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"
> >
> > +static struct platform_driver exynos_pmu_driver;
>
> I don't understand why do you need it. You can have only one
> pmu_context. The moment you probe second one, previous becomes invalid.
>
> I guess you want to parse phandle and check if just in case if it points
> to the right device, but still the original code is not ready for two
> PMU devices. I say either this problem should be solved entirely,
> allowing two devices, or just compare device node from phandle with
> device node of exynos_pmu_context->dev and return -EINVAL on mismatches.
Apologies I didn't answer your original question. This wasn't about
having partial support for multiple pmu devices. It is being used by
driver_find_device_by_of_node() in exynos_get_pmu_regmap_by_phandle()
to determine that the exynos-pmu device has probed and therefore a
pmu_context exists and a regmap has been created and can be returned
to the caller (as opposed to doing a -EPROBE_DEFER).
Is there some better/other API you recommend for this purpose? Just
checking pmu_context directly seems racy, so I don't think we should
do that.
Peter
[...]
On 01/02/2024 12:29, Peter Griffin wrote:
>>> int ret;
>>>
>>> pmu_base_addr = devm_platform_ioremap_resource(pdev, 0);
>>> @@ -137,6 +333,35 @@ static int exynos_pmu_probe(struct platform_device *pdev)
>>> GFP_KERNEL);
>>> if (!pmu_context)
>>> return -ENOMEM;
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + if (!res)
>>> + return -ENODEV;
>>> +
>>> + pmuregmap_config.max_register = resource_size(res) -
>>> + pmuregmap_config.reg_stride;
>>> +
>>> + if (of_device_is_compatible(np, "google,gs101-pmu")) {
>>
>> No compatibles inside the probe. Use driver match data. This applies to
>> all drivers in all subsystems.
>
> Noted, will fix in v3.
>
>>
>>> + pmuregmap_config.reg_read = tensor_sec_reg_read;
>>> + pmuregmap_config.reg_write = tensor_sec_reg_write;
>>> + pmuregmap_config.reg_update_bits = tensor_sec_update_bits;
>>
>> No, regmap_config should be const and please use match data.
>
> Are you sure you want the regmap_config struct const?
>
> In my draft v3 I have implemented it so far having a regmap_smccfg
> struct which sets all the configuration apart from max_register field
> (used by gs101) and a regmap_mmiocfg struct (used by all other
> exynos-pmu SoCs). The choice over which regmap_config to register is
> made via match data exynos_pmu_data flag 'pmu_secure' which is set
> only for gs101. That avoids having to define exynos_pmu_data structs
> for the other exynos SoCs that currently don't really need them
> (exynos7, exynos850, exynos5443, exyno5410 etc).
>
> But I still wish to set at runtime the regmap_config.max_register
> field based on the resource size coming from DT. Having the structs
> const would prohibit that and mean we need to specify many more
> regmap_config structs where the only difference is the max_register
> field.
>
> Is the above approach acceptable for you?
Having it non-const is one more step of supporting only one instance of
PMU device, but we already rely on such design choice, so I guess it is
fine. If ever needed, this can be easily converted to devm_kmemdup...
Best regards,
Krzysztof
On 01/02/2024 13:51, Peter Griffin wrote:
> Hi Krzysztof,
>
> On Tue, 30 Jan 2024 at 16:01, Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 29/01/2024 22:19, Peter Griffin wrote:
>>> Some Exynos based SoCs like Tensor gs101 protect the PMU registers for
>>> security hardening reasons so that they are only 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]>
>>> ---
>>> drivers/soc/samsung/exynos-pmu.c | 227 ++++++++++++++++++++++++-
>>> include/linux/soc/samsung/exynos-pmu.h | 10 ++
>>> 2 files changed, 236 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
>>> index 250537d7cfd6..7bcc144e53a2 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,20 +13,159 @@
>>> #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"
>>>
>>> +static struct platform_driver exynos_pmu_driver;
>>
>> I don't understand why do you need it. You can have only one
>> pmu_context. The moment you probe second one, previous becomes invalid.
>>
>> I guess you want to parse phandle and check if just in case if it points
>> to the right device, but still the original code is not ready for two
>> PMU devices. I say either this problem should be solved entirely,
>> allowing two devices, or just compare device node from phandle with
>> device node of exynos_pmu_context->dev and return -EINVAL on mismatches.
>
> Apologies I didn't answer your original question. This wasn't about
> having partial support for multiple pmu devices. It is being used by
> driver_find_device_by_of_node() in exynos_get_pmu_regmap_by_phandle()
> to determine that the exynos-pmu device has probed and therefore a
> pmu_context exists and a regmap has been created and can be returned
> to the caller (as opposed to doing a -EPROBE_DEFER).
>
> Is there some better/other API you recommend for this purpose? Just
> checking pmu_context directly seems racy, so I don't think we should
> do that.
Hm, I don't quite get why you cannot use of_find_device_by_node()?
Best regards,
Krzysztof
Hi Krzysztof,
Thanks for your review feedback.
On Mon, 5 Feb 2024 at 13:13, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 01/02/2024 13:51, Peter Griffin wrote:
> > Hi Krzysztof,
> >
> > On Tue, 30 Jan 2024 at 16:01, Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 29/01/2024 22:19, Peter Griffin wrote:
> >>> Some Exynos based SoCs like Tensor gs101 protect the PMU registers for
> >>> security hardening reasons so that they are only 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]>
> >>> ---
> >>> drivers/soc/samsung/exynos-pmu.c | 227 ++++++++++++++++++++++++-
> >>> include/linux/soc/samsung/exynos-pmu.h | 10 ++
> >>> 2 files changed, 236 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> >>> index 250537d7cfd6..7bcc144e53a2 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,20 +13,159 @@
> >>> #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"
> >>>
> >>> +static struct platform_driver exynos_pmu_driver;
> >>
> >> I don't understand why do you need it. You can have only one
> >> pmu_context. The moment you probe second one, previous becomes invalid.
> >>
> >> I guess you want to parse phandle and check if just in case if it points
> >> to the right device, but still the original code is not ready for two
> >> PMU devices. I say either this problem should be solved entirely,
> >> allowing two devices, or just compare device node from phandle with
> >> device node of exynos_pmu_context->dev and return -EINVAL on mismatches.
> >
> > Apologies I didn't answer your original question. This wasn't about
> > having partial support for multiple pmu devices. It is being used by
> > driver_find_device_by_of_node() in exynos_get_pmu_regmap_by_phandle()
> > to determine that the exynos-pmu device has probed and therefore a
> > pmu_context exists and a regmap has been created and can be returned
> > to the caller (as opposed to doing a -EPROBE_DEFER).
> >
> > Is there some better/other API you recommend for this purpose? Just
> > checking pmu_context directly seems racy, so I don't think we should
> > do that.
>
> Hm, I don't quite get why you cannot use of_find_device_by_node()?
of_find_device_by_node() returns a platform_device, even if the driver
hasn't probed. Whereas driver_find_device_by_of_node() iterates
devices bound to a driver.
If using of_find_device_by_node() API I could check the result of
platform_get_drvdata(), and -EPROBE_DEFER if NULL (that pattern seems
to be used by a few drivers). But that AFAIK only guarantees you
reached the platform_set_drvdata() call in your driver probe()
function, not that it has completed.
IMHO the drivers using driver_find_device_by_of_node() for probe
deferral are doing it more robustly than those using
of_find_device_by_node() and checking if platform_get_drvdata() is
NULL.
Or is there some other way you had in mind of using
of_find_device_by_node() I've not thought of to implement this?
Thanks,
Peter.
On 07/02/2024 12:42, Peter Griffin wrote:
>>>>> #include <linux/soc/samsung/exynos-regs-pmu.h>
>>>>> #include <linux/soc/samsung/exynos-pmu.h>
>>>>>
>>>>> #include "exynos-pmu.h"
>>>>>
>>>>> +static struct platform_driver exynos_pmu_driver;
>>>>
>>>> I don't understand why do you need it. You can have only one
>>>> pmu_context. The moment you probe second one, previous becomes invalid.
>>>>
>>>> I guess you want to parse phandle and check if just in case if it points
>>>> to the right device, but still the original code is not ready for two
>>>> PMU devices. I say either this problem should be solved entirely,
>>>> allowing two devices, or just compare device node from phandle with
>>>> device node of exynos_pmu_context->dev and return -EINVAL on mismatches.
>>>
>>> Apologies I didn't answer your original question. This wasn't about
>>> having partial support for multiple pmu devices. It is being used by
>>> driver_find_device_by_of_node() in exynos_get_pmu_regmap_by_phandle()
>>> to determine that the exynos-pmu device has probed and therefore a
>>> pmu_context exists and a regmap has been created and can be returned
>>> to the caller (as opposed to doing a -EPROBE_DEFER).
>>>
>>> Is there some better/other API you recommend for this purpose? Just
>>> checking pmu_context directly seems racy, so I don't think we should
>>> do that.
>>
>> Hm, I don't quite get why you cannot use of_find_device_by_node()?
>
> of_find_device_by_node() returns a platform_device, even if the driver
> hasn't probed. Whereas driver_find_device_by_of_node() iterates
> devices bound to a driver.
>
> If using of_find_device_by_node() API I could check the result of
> platform_get_drvdata(), and -EPROBE_DEFER if NULL (that pattern seems
> to be used by a few drivers). But that AFAIK only guarantees you
> reached the platform_set_drvdata() call in your driver probe()
> function, not that it has completed.
All drivers, except two, use of_find_device_by_node(), so basically you
claim they are all broken. If that's true, the core API and these
drivers should be fixed, instead of implementing here entirely different
pattern.
of_find_device_by_node() goes via platform_bus_type->sp->klist_devices
and devices are added to the list in device_add() after
bus_probe_device(dev), regardless of its success. Therefore after
successful first probe, you will have the same result.
>
> IMHO the drivers using driver_find_device_by_of_node() for probe
> deferral are doing it more robustly than those using
> of_find_device_by_node() and checking if platform_get_drvdata() is
> NULL.
Some are checking dev->driver, but this also looks buggy, because it is
called before actual drv->probe().
OK, let's go with this method. I dislike the difference from everyone
else, but it seems everyone else is doing it wrong. :(
Best regards,
Krzysztof