Hi Sam, Thanks for the review feedback. On Mon, 29 Jan 2024 at 23:01, Sam Protsenko <semen.protsenko@xxxxxxxxxx> wrote: > > On Mon, Jan 29, 2024 at 3:19 PM Peter Griffin <peter.griffin@xxxxxxxxxx> 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@xxxxxxxxxx> > > --- > > 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