| Message ID | 20240129211912.3068411-1-peter.griffin@linaro.org |
|---|---|
| Headers | show |
| Series | Add regmap support to exynos-pmu for protected PMU regs | expand |
Hi Sam, Thanks for the review feedback. On Mon, 29 Jan 2024 at 23:01, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Mon, Jan 29, 2024 at 3:19 PM 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 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> > > --- > > 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 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/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/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(-)