| Message ID | 20240202145731.4082033-1-peter.griffin@linaro.org |
|---|---|
| Headers | show |
| Series | Add regmap support to exynos-pmu for protected PMU regs | expand |
On Fri, Feb 2, 2024 at 8:57 AM Peter Griffin <peter.griffin@linaro.org> 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 <peter.griffin@linaro.org> > --- > 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 superflous 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 | 233 ++++++++++++++++++++++++- > drivers/soc/samsung/exynos-pmu.h | 1 + > include/linux/soc/samsung/exynos-pmu.h | 11 +- > 4 files changed, 241 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..adf3549370d6 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,130 @@ > #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(14, 0) Are you sure it's not GENMASK(13, 0)? Because SET_BITS has bit #14 set, which overlaps with bit #14 from PMUALIVE_MASK, when being added in tensor_set_bit_atomic(). This also can be aligned with below definitions. > +#define SET_BITS 0xc000 > +#define CLEAR_BITS 0x8000 All 3 above values seem to be gs101 specific. At least I can't find any similar atomic registers in Exynos850 TRM, in PMU block. So I'd suggest also adding TENSOR_ prefix to those to make it clear and to prevent possible naming conflicts in future. Also, not sure if it makes things better, but FWIW: #define CLEAR_BITS BIT(15) #define SET_BITS (BIT(15) | BIT(14)) Just to show that those two bits don't overlap with PMUALIVE_MASK (if it can be fixed to 13:0), and show their relation. If I understand correctly what's going on anyway. > + > +#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; > +static struct platform_driver exynos_pmu_driver; Just an idea: maybe add a comment saying it's a forward declaration, and the variable is assigned below, as it might be confusing. Not sure if it's worth it though. > + > +/* > + * 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_bit_atomic(0x3e80, 0x3f00, 0x3f00); > + * > + * To clear bits 13:8 in PMU offset 0x3e80 > + * tensor_set_bit_atomic(0x3e80, 0x0, 0x3f00); > + */ > +static inline int tensor_set_bit_atomic(void *ctx, unsigned int offset, set_bit -> set_bits? > + u32 val, u32 mask) > +{ > + int ret; > + unsigned int i; > + > + for (i = 0; i < 32; i++) { > + if (mask & BIT(i)) { Maybe replace it with: if (!(mask & BIT(i))) continue; to reduce the indentation level? > + if (val & BIT(i)) > + offset |= SET_BITS; > + else > + offset |= CLEAR_BITS; What if someone calls this functions like this: tensor_set_bit_atomic(0x3e80, 0x100, 0x3f00); which means "set bit #8, and clear bits 13:9). But because the offset variable will hold SET_BITS set during bit #8 handling, bits 13:9 are also going to be set, effectively making that call act like tensor_set_bit_atomic(0x3e80, 0x3f00, 0x3f00) instead. So I'd add something like: offset &= ~SET_BITS; before doing |= operations. > + > + ret = tensor_sec_reg_write(ctx, offset, i); > + if (ret) > + goto out; Maybe remove "out" and just do return ret here? > + } > + } > +out: > + 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_bit_atomic(ctx, reg, val, mask); > +} > > void pmu_raw_writel(u32 val, u32 offset) > { > @@ -75,11 +187,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 +255,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 +329,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 */ Suggest starting with a capital letter, for consistency with previous comments. > + 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)) > + dev_err_probe(&pdev->dev, PTR_ERR(regmap), > + "regmap init failed\n"); Why not "return dev_err_probe()"? Is it ok to continue with no regmap created? > + > + 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..406ed73614fd 100644 > --- a/include/linux/soc/samsung/exynos-pmu.h > +++ b/include/linux/soc/samsung/exynos-pmu.h > @@ -20,12 +20,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); > +struct regmap *exynos_get_pmu_regmap(void); > + Usually empty line delimeter is not needed in cases like that. > +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 >
Hi Sam, Thanks for the great review feedback. On Fri, 2 Feb 2024 at 17:35, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Fri, Feb 2, 2024 at 8:57 AM Peter Griffin <peter.griffin@linaro.org> 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 <peter.griffin@linaro.org> > > --- > > 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 superflous 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 | 233 ++++++++++++++++++++++++- > > drivers/soc/samsung/exynos-pmu.h | 1 + > > include/linux/soc/samsung/exynos-pmu.h | 11 +- > > 4 files changed, 241 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..adf3549370d6 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,130 @@ > > #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(14, 0) > > Are you sure it's not GENMASK(13, 0)? Yes, you're right, mask should be 0x3fff. > Because SET_BITS has bit #14 > set, which overlaps with bit #14 from PMUALIVE_MASK, when being added > in tensor_set_bit_atomic(). > > This also can be aligned with below definitions. Will update alignment > > > +#define SET_BITS 0xc000 > > +#define CLEAR_BITS 0x8000 > > All 3 above values seem to be gs101 specific. At least I can't find > any similar atomic registers in Exynos850 TRM, in PMU block. So I'd > suggest also adding TENSOR_ prefix to those to make it clear and to > prevent possible naming conflicts in future. Sure, I can add a Tensor prefix. Unfortunately these atomic registers aren't mentioned anywhere in gs101 TRM's either, so it is a bit of a nature study :( It was implemented and used in the downstream drivers though (and doesn't look to be implemented downstream for exynos850, so you're likely correct it doesn't have it.). I tried to check some other downstream product tree's, I found the hardware looks to exist on Exynos 9820 [1]. But I think we can update the name if somebody tries to upstream that platform in the future. [1] https://github.com/PixelOS-Devices/kernel_samsung_exynos9820/blob/thirteen/drivers/soc/samsung/exynos-pmu.c#L32 > > Also, not sure if it makes things better, but FWIW: > > #define CLEAR_BITS BIT(15) > #define SET_BITS (BIT(15) | BIT(14)) I agree that would make it clearer. > > Just to show that those two bits don't overlap with PMUALIVE_MASK (if > it can be fixed to 13:0), and show their relation. If I understand > correctly what's going on anyway. I think you've understood what's happening here perfectly > > > + > > +#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; > > +static struct platform_driver exynos_pmu_driver; > > Just an idea: maybe add a comment saying it's a forward declaration, > and the variable is assigned below, as it might be confusing. Not sure > if it's worth it though. > > > + > > +/* > > + * 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_bit_atomic(0x3e80, 0x3f00, 0x3f00); > > + * > > + * To clear bits 13:8 in PMU offset 0x3e80 > > + * tensor_set_bit_atomic(0x3e80, 0x0, 0x3f00); > > + */ > > +static inline int tensor_set_bit_atomic(void *ctx, unsigned int offset, > > set_bit -> set_bits? Will change > > > + u32 val, u32 mask) > > +{ > > + int ret; > > + unsigned int i; > > + > > + for (i = 0; i < 32; i++) { > > + if (mask & BIT(i)) { > > Maybe replace it with: > > if (!(mask & BIT(i))) > continue; > > to reduce the indentation level? Will do > > > + if (val & BIT(i)) > > + offset |= SET_BITS; > > + else > > + offset |= CLEAR_BITS; > > What if someone calls this functions like this: > > tensor_set_bit_atomic(0x3e80, 0x100, 0x3f00); > > which means "set bit #8, and clear bits 13:9). But because the offset > variable will hold SET_BITS set during bit #8 handling, bits 13:9 are > also going to be set, effectively making that call act like > tensor_set_bit_atomic(0x3e80, 0x3f00, 0x3f00) instead. So I'd add > something like: > > offset &= ~SET_BITS; > > before doing |= operations. Good catch! Will fix in v4 > > > + > > + ret = tensor_sec_reg_write(ctx, offset, i); > > + if (ret) > > + goto out; > > Maybe remove "out" and just do return ret here? Will update > > > + } > > + } > > +out: > > + 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_bit_atomic(ctx, reg, val, mask); > > +} > > > > void pmu_raw_writel(u32 val, u32 offset) > > { > > @@ -75,11 +187,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 +255,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 +329,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 */ > > Suggest starting with a capital letter, for consistency with previous comments. Will update > > > + 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)) > > + dev_err_probe(&pdev->dev, PTR_ERR(regmap), > > + "regmap init failed\n"); > > Why not "return dev_err_probe()"? Is it ok to continue with no regmap created? That should have been return dev_err_probe. Will fix Peter > > > + > > + 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..406ed73614fd 100644 > > --- a/include/linux/soc/samsung/exynos-pmu.h > > +++ b/include/linux/soc/samsung/exynos-pmu.h > > @@ -20,12 +20,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); > > +struct regmap *exynos_get_pmu_regmap(void); > > + > > Usually empty line delimeter is not needed in cases like that. > > > +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 > >
Hi folks, This is a v3 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 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 superflous 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) v2 lore: https://lore.kernel.org/lkml/20240129211912.3068411-1-peter.griffin@linaro.org/ v1 lore: https://lore.kernel.org/all/20240122225710.1952066-1-peter.griffin@linaro.org/ 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 | 233 ++++++++++++++++++++++++- 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, 245 insertions(+), 10 deletions(-)