| Message ID | 20240122225710.1952066-3-peter.griffin@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add exynos_pmu_update/read/write() APIs to exynos-pmu | expand |
On Mon, Jan 22, 2024, at 23:57, Peter Griffin wrote: > --- a/include/linux/soc/samsung/exynos-pmu.h > +++ b/include/linux/soc/samsung/exynos-pmu.h > @@ -21,11 +21,39 @@ 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 int exynos_pmu_update_bits(unsigned int offset, unsigned int > mask, > + unsigned int val); > +extern int exynos_pmu_update(unsigned int offset, unsigned int mask, > + unsigned int val); > +extern int exynos_pmu_write(unsigned int offset, unsigned int val); > +extern int exynos_pmu_read(unsigned int offset, unsigned int *val); > #else > static inline struct regmap *exynos_get_pmu_regmap(void) > { > return ERR_PTR(-ENODEV); > } > + > +static inline int exynos_pmu_update_bits(unsigned int offset, unsigned > int mask, > + unsigned int val); > +{ > + return ERR_PTR(-ENODEV); > +} > + > +static inline int exynos_pmu_update(unsigned int offset, unsigned int > mask, > + unsigned int val); > +{ > + return ERR_PTR(-ENODEV); > +} This won't build since you have the wrong return type. I would suggest you just remove the #ifdef check entirely and instead require drivers using this to have correct dependencies. Arnd
On Mon, Jan 22, 2024 at 4:57 PM Peter Griffin <peter.griffin@linaro.org> wrote: > > Newer Exynos SoCs have atomic set/clear bit hardware for PMU registers as > these registers can be accessed by multiple masters. Some platforms also > protect the PMU registers for security hardening reasons so they can't be > written by normal world and are only write acessible in el3 via a SMC call. > > Add support for both of these usecases using SoC specific quirks that are > determined from the DT compatible string. > > Drivers which need to read and write PMU registers should now use these > new exynos_pmu_*() APIs instead of obtaining a regmap using > syscon_regmap_lookup_by_phandle() > > Depending on the SoC specific quirks, the exynos_pmu_*() APIs will access > the PMU register in the appropriate way. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > --- > drivers/soc/samsung/exynos-pmu.c | 209 ++++++++++++++++++++++++- > drivers/soc/samsung/exynos-pmu.h | 4 + > include/linux/soc/samsung/exynos-pmu.h | 28 ++++ > 3 files changed, 234 insertions(+), 7 deletions(-) > [snip] > + > +int exynos_pmu_update_bits(unsigned int offset, unsigned int mask, > + unsigned int val) > +{ > + if (pmu_context->pmu_data && > + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC) > + return rmw_priv_reg(pmu_context->pmu_base_pa + offset, > + mask, val); > + > + return regmap_update_bits(pmu_context->pmureg, offset, mask, val); > +} > +EXPORT_SYMBOL(exynos_pmu_update_bits); > + This seems a bit hacky, from the design perspective. This way the user will have to worry about things like driver dependencies, making sure everything is instantiated in a correct order, etc. It also hides the details otherwise visible through "syscon-phandle" property in the device tree. Can we instead rework it by overriding regmap implementation for Exynos specifics, and then continue to use it in the leaf drivers via "syscon-phandle" property? [snip]
Hi Sam, Thanks for the review feedback. On Tue, 23 Jan 2024 at 18:56, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Mon, Jan 22, 2024 at 4:57 PM Peter Griffin <peter.griffin@linaro.org> wrote: > > > > Newer Exynos SoCs have atomic set/clear bit hardware for PMU registers as > > these registers can be accessed by multiple masters. Some platforms also > > protect the PMU registers for security hardening reasons so they can't be > > written by normal world and are only write acessible in el3 via a SMC call. > > > > Add support for both of these usecases using SoC specific quirks that are > > determined from the DT compatible string. > > > > Drivers which need to read and write PMU registers should now use these > > new exynos_pmu_*() APIs instead of obtaining a regmap using > > syscon_regmap_lookup_by_phandle() > > > > Depending on the SoC specific quirks, the exynos_pmu_*() APIs will access > > the PMU register in the appropriate way. > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > --- > > drivers/soc/samsung/exynos-pmu.c | 209 ++++++++++++++++++++++++- > > drivers/soc/samsung/exynos-pmu.h | 4 + > > include/linux/soc/samsung/exynos-pmu.h | 28 ++++ > > 3 files changed, 234 insertions(+), 7 deletions(-) > > > > [snip] > > > + > > +int exynos_pmu_update_bits(unsigned int offset, unsigned int mask, > > + unsigned int val) > > +{ > > + if (pmu_context->pmu_data && > > + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC) > > + return rmw_priv_reg(pmu_context->pmu_base_pa + offset, > > + mask, val); > > + > > + return regmap_update_bits(pmu_context->pmureg, offset, mask, val); > > +} > > +EXPORT_SYMBOL(exynos_pmu_update_bits); > > + > > This seems a bit hacky, from the design perspective. This way the user > will have to worry about things like driver dependencies, making sure > everything is instantiated in a correct order, etc. It also hides the > details otherwise visible through "syscon-phandle" property in the > device tree. In v2 I will keep the phandle to pmu_system_controller in DT, and add some -EPROBE_DEFER logic (See my email with Krzysztof). > Can we instead rework it by overriding regmap > implementation for Exynos specifics, and then continue to use it in > the leaf drivers via "syscon-phandle" property? I did look at that possibility first, as like you say it would avoid updating the leaf drivers to use the new API. Unfortunately a SMC backend to regmap was already tried and nacked upstream pretty hard. See here https://lore.kernel.org/lkml/20210723163759.GI5221@sirena.org.uk/T/ regards, Peter.
Hi Arnd, On Tue, 23 Jan 2024 at 08:11, Arnd Bergmann <arnd@arndb.de> wrote: > > On Mon, Jan 22, 2024, at 23:57, Peter Griffin wrote: > > > --- a/include/linux/soc/samsung/exynos-pmu.h > > +++ b/include/linux/soc/samsung/exynos-pmu.h > > @@ -21,11 +21,39 @@ 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 int exynos_pmu_update_bits(unsigned int offset, unsigned int > > mask, > > + unsigned int val); > > +extern int exynos_pmu_update(unsigned int offset, unsigned int mask, > > + unsigned int val); > > +extern int exynos_pmu_write(unsigned int offset, unsigned int val); > > +extern int exynos_pmu_read(unsigned int offset, unsigned int *val); > > #else > > static inline struct regmap *exynos_get_pmu_regmap(void) > > { > > return ERR_PTR(-ENODEV); > > } > > + > > +static inline int exynos_pmu_update_bits(unsigned int offset, unsigned > > int mask, > > + unsigned int val); > > +{ > > + return ERR_PTR(-ENODEV); > > +} > > + > > +static inline int exynos_pmu_update(unsigned int offset, unsigned int > > mask, > > + unsigned int val); > > +{ > > + return ERR_PTR(-ENODEV); > > +} > > This won't build since you have the wrong return type. > I would suggest you just remove the #ifdef check entirely > and instead require drivers using this to have correct > dependencies. Whoops, will fix it in v2. We need those stubs for platforms like ARCH_S3C64XX that don't have a PMU but use some of the same drivers. Thanks, Peter.
Hi Krzysztof, Thanks for the review feedback. On Tue, 23 Jan 2024 at 11:17, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 22/01/2024 23:57, Peter Griffin wrote: > > Newer Exynos SoCs have atomic set/clear bit hardware for PMU registers as > > these registers can be accessed by multiple masters. Some platforms also > > protect the PMU registers for security hardening reasons so they can't be > > written by normal world and are only write acessible in el3 via a SMC call. > > > Typo? accessible? Will fix in v2. > > > > > Add support for both of these usecases using SoC specific quirks that are > > determined from the DT compatible string.> > > Drivers which need to read and write PMU registers should now use these > > new exynos_pmu_*() APIs instead of obtaining a regmap using > > syscon_regmap_lookup_by_phandle() > > > > Depending on the SoC specific quirks, the exynos_pmu_*() APIs will access > > the PMU register in the appropriate way. > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > --- > > drivers/soc/samsung/exynos-pmu.c | 209 ++++++++++++++++++++++++- > > drivers/soc/samsung/exynos-pmu.h | 4 + > > include/linux/soc/samsung/exynos-pmu.h | 28 ++++ > > 3 files changed, 234 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c > > index 250537d7cfd6..e9e933ede568 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,29 +13,204 @@ > > #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" > > > > +/** > > + * DOC: Quirk flags for different Exynos PMU IP-cores > > + * > > + * This driver supports multiple Exynos based SoCs, each of which might have a > > + * different set of registers and features supported. > > + * > > + * Quirk flags described below serve the purpose of telling the driver about > > + * mentioned SoC traits, and can be specified in driver data for each particular > > + * supported device. > > + * > > + * %QUIRK_HAS_ATOMIC_BITSETHW: PMU IP has special atomic bit set/clear HW > > + * to protect against PMU registers being accessed from multiple bus masters. > > + * > > + * %QUIRK_PMU_ALIVE_WRITE_SEC: PMU registers are *not* write accesible from > > + * normal world. This is found on some SoCs as a security hardening measure. PMU > > + * registers on these SoCs can only be written via a SMC call and registers are > > + * checked by EL3 firmware against an allowlist before the write can procede. > > + * Note: This quirk should only be set for platforms whose el3 firmware > > + * implements the TENSOR_SMC_PMU_SEC_REG interface below. > > + */ > > + > > +#define QUIRK_HAS_ATOMIC_BITSETHW BIT(0) > > +#define QUIRK_PMU_ALIVE_WRITE_SEC BIT(1) > > + > > +#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; > > + phys_addr_t pmu_base_pa; > > + /* protect PMU reg atomic update operations */ > > + spinlock_t update_lock; > > }; > > > > -void __iomem *pmu_base_addr; > > static struct exynos_pmu_context *pmu_context; > > > > +/* > > + * Some 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 > > These are tensor specific... > > > + > > +int set_priv_reg(phys_addr_t reg, u32 val) > > ...but this not... > > > +{ > > + struct arm_smccc_res res; > > + > > + arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG, > > ... and this is again. > > Some naming should be clarified, e.g. tensor specific functions should > have some prefix as well, e.g. tensor_writel(), tensor_cmpxchg() or > something similar. Noted. I will add a tensor prefix on these two functions as well in v2. > > > > + 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; > > +} > > + > > +int rmw_priv_reg(phys_addr_t reg, u32 mask, u32 val) > > +{ > > + struct arm_smccc_res res; > > + > > + arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG, > > + 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; > > +} > > + > > +/* > > + * For SoCs that have set/clear bit hardware (as indicated by > > + * QUIRK_HAS_ATOMIC_BITSETHW) 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 > > + * exynos_pmu_set_bit_atomic(0x3e80, 0x3f00, 0x3f00); > > + * > > + * To clear bits 13:8 in PMU offset 0x3e80 > > + * exynos_pmu_set_bit_atomic(0x3e80, 0x0, 0x3f00); > > + */ > > +static inline void exynos_pmu_set_bit_atomic(unsigned int offset, > > + u32 val, u32 mask) > > +{ > > + unsigned long flags; > > + unsigned int i; > > + > > + spin_lock_irqsave(&pmu_context->update_lock, flags); > > + for (i = 0; i < 32; i++) { > > + if (mask & BIT(i)) { > > + if (val & BIT(i)) { > > + offset |= 0xc000; > > + pmu_raw_writel(i, offset); > > + } else { > > + offset |= 0x8000; > > + pmu_raw_writel(i, offset); > > + } > > + } > > + } > > + spin_unlock_irqrestore(&pmu_context->update_lock, flags); > > +} > > + > > +int exynos_pmu_update_bits(unsigned int offset, unsigned int mask, > > + unsigned int val) > > +{ > > + if (pmu_context->pmu_data && > > + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC) > > + return rmw_priv_reg(pmu_context->pmu_base_pa + offset, > > + mask, val); > > + > > + return regmap_update_bits(pmu_context->pmureg, offset, mask, val); > > +} > > +EXPORT_SYMBOL(exynos_pmu_update_bits); > > You need kerneldoc for all exported functions. > > Also, EXPORT_SYMBOL_GPL Will fix both in v2. > > > + > > void pmu_raw_writel(u32 val, u32 offset) > > { > > - writel_relaxed(val, pmu_base_addr + offset); > > + if (pmu_context->pmu_data && > > + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC) > > + return (void)set_priv_reg(pmu_context->pmu_base_pa + offset, > > + val); > > + > > + return writel_relaxed(val, pmu_context->pmu_base_addr + offset); > > } > > > > ... > > > diff --git a/drivers/soc/samsung/exynos-pmu.h b/drivers/soc/samsung/exynos-pmu.h > > index 1c652ffd79b4..570c6e4dc8c3 100644 > > --- a/drivers/soc/samsung/exynos-pmu.h > > +++ b/drivers/soc/samsung/exynos-pmu.h > > @@ -25,8 +25,12 @@ struct exynos_pmu_data { > > void (*pmu_init)(void); > > void (*powerdown_conf)(enum sys_powerdown); > > void (*powerdown_conf_extra)(enum sys_powerdown); > > + u32 quirks; > > }; > > > > +int set_priv_reg(phys_addr_t reg, u32 val); > > +int rmw_priv_reg(phys_addr_t reg, u32 mask, u32 val); > > Why these are in the header? Will fix in v2. regards, Peter.
On Wed, Jan 24, 2024 at 4:02 AM Peter Griffin <peter.griffin@linaro.org> wrote: > > Hi Sam, > > Thanks for the review feedback. > > On Tue, 23 Jan 2024 at 18:56, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > On Mon, Jan 22, 2024 at 4:57 PM Peter Griffin <peter.griffin@linaro.org> wrote: > > > > > > Newer Exynos SoCs have atomic set/clear bit hardware for PMU registers as > > > these registers can be accessed by multiple masters. Some platforms also > > > protect the PMU registers for security hardening reasons so they can't be > > > written by normal world and are only write acessible in el3 via a SMC call. > > > > > > Add support for both of these usecases using SoC specific quirks that are > > > determined from the DT compatible string. > > > > > > Drivers which need to read and write PMU registers should now use these > > > new exynos_pmu_*() APIs instead of obtaining a regmap using > > > syscon_regmap_lookup_by_phandle() > > > > > > Depending on the SoC specific quirks, the exynos_pmu_*() APIs will access > > > the PMU register in the appropriate way. > > > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > > --- > > > drivers/soc/samsung/exynos-pmu.c | 209 ++++++++++++++++++++++++- > > > drivers/soc/samsung/exynos-pmu.h | 4 + > > > include/linux/soc/samsung/exynos-pmu.h | 28 ++++ > > > 3 files changed, 234 insertions(+), 7 deletions(-) > > > > > > > [snip] > > > > > + > > > +int exynos_pmu_update_bits(unsigned int offset, unsigned int mask, > > > + unsigned int val) > > > +{ > > > + if (pmu_context->pmu_data && > > > + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC) > > > + return rmw_priv_reg(pmu_context->pmu_base_pa + offset, > > > + mask, val); > > > + > > > + return regmap_update_bits(pmu_context->pmureg, offset, mask, val); > > > +} > > > +EXPORT_SYMBOL(exynos_pmu_update_bits); > > > + > > > > This seems a bit hacky, from the design perspective. This way the user > > will have to worry about things like driver dependencies, making sure > > everything is instantiated in a correct order, etc. It also hides the > > details otherwise visible through "syscon-phandle" property in the > > device tree. > > In v2 I will keep the phandle to pmu_system_controller in DT, and add > some -EPROBE_DEFER logic (See my email with Krzysztof). > > > Can we instead rework it by overriding regmap > > implementation for Exynos specifics, and then continue to use it in > > the leaf drivers via "syscon-phandle" property? > > I did look at that possibility first, as like you say it would avoid > updating the leaf drivers to use the new API. Unfortunately a SMC > backend to regmap was already tried and nacked upstream pretty hard. > See here https://lore.kernel.org/lkml/20210723163759.GI5221@sirena.org.uk/T/ > Oh, I didn't mean creating a new regmap implementation :) To illustrate what I meant, please look at these: - drivers/mfd/altera-sysmgr.c - altr_sysmgr_regmap_lookup_by_phandle() - arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi - drivers/mmc/host/dw_mmc-pltfm.c They basically implement their own regmap operations (with smcc too) in their syscon implementation. So they can actually reference that syscon as phandle in device tree and avoid exporting and calling read/write operations (which I think looks hacky). Instead they use altr_sysmgr_regmap_lookup_by_phandle() to get their regmap (which performs smcc), and then they just use regular regmap_read() / regmap_write or whatever functions to operate on their regmap object. That's what I meant by "overriding" the regmap. Do you think this approach would be clearer and more "productizable" so to speak? Just a thought. > regards, > > Peter.
Hi Sam, Thanks for your review feedback! On Wed, 24 Jan 2024 at 20:23, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Wed, Jan 24, 2024 at 4:02 AM Peter Griffin <peter.griffin@linaro.org> wrote: > > > > Hi Sam, > > > > Thanks for the review feedback. > > > > On Tue, 23 Jan 2024 at 18:56, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > > > On Mon, Jan 22, 2024 at 4:57 PM Peter Griffin <peter.griffin@linaro.org> wrote: > > > > > > > > Newer Exynos SoCs have atomic set/clear bit hardware for PMU registers as > > > > these registers can be accessed by multiple masters. Some platforms also > > > > protect the PMU registers for security hardening reasons so they can't be > > > > written by normal world and are only write acessible in el3 via a SMC call. > > > > > > > > Add support for both of these usecases using SoC specific quirks that are > > > > determined from the DT compatible string. > > > > > > > > Drivers which need to read and write PMU registers should now use these > > > > new exynos_pmu_*() APIs instead of obtaining a regmap using > > > > syscon_regmap_lookup_by_phandle() > > > > > > > > Depending on the SoC specific quirks, the exynos_pmu_*() APIs will access > > > > the PMU register in the appropriate way. > > > > > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > > > --- > > > > drivers/soc/samsung/exynos-pmu.c | 209 ++++++++++++++++++++++++- > > > > drivers/soc/samsung/exynos-pmu.h | 4 + > > > > include/linux/soc/samsung/exynos-pmu.h | 28 ++++ > > > > 3 files changed, 234 insertions(+), 7 deletions(-) > > > > > > > > > > [snip] > > > > > > > + > > > > +int exynos_pmu_update_bits(unsigned int offset, unsigned int mask, > > > > + unsigned int val) > > > > +{ > > > > + if (pmu_context->pmu_data && > > > > + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC) > > > > + return rmw_priv_reg(pmu_context->pmu_base_pa + offset, > > > > + mask, val); > > > > + > > > > + return regmap_update_bits(pmu_context->pmureg, offset, mask, val); > > > > +} > > > > +EXPORT_SYMBOL(exynos_pmu_update_bits); > > > > + > > > > > > This seems a bit hacky, from the design perspective. This way the user > > > will have to worry about things like driver dependencies, making sure > > > everything is instantiated in a correct order, etc. It also hides the > > > details otherwise visible through "syscon-phandle" property in the > > > device tree. > > > > In v2 I will keep the phandle to pmu_system_controller in DT, and add > > some -EPROBE_DEFER logic (See my email with Krzysztof). > > > > > Can we instead rework it by overriding regmap > > > implementation for Exynos specifics, and then continue to use it in > > > the leaf drivers via "syscon-phandle" property? > > > > I did look at that possibility first, as like you say it would avoid > > updating the leaf drivers to use the new API. Unfortunately a SMC > > backend to regmap was already tried and nacked upstream pretty hard. > > See here https://lore.kernel.org/lkml/20210723163759.GI5221@sirena.org.uk/T/ > > > > Oh, I didn't mean creating a new regmap implementation :) To > illustrate what I meant, please look at these: > > - drivers/mfd/altera-sysmgr.c > - altr_sysmgr_regmap_lookup_by_phandle() > - arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi > - drivers/mmc/host/dw_mmc-pltfm.c Thanks for the pointers :) I hadn't spotted this when looking previously. I did find the previous threads I linked to and (it appears wrongly concluded) that such a regmap SMC would not be acceptable. > > They basically implement their own regmap operations (with smcc too) > in their syscon implementation. So they can actually reference that > syscon as phandle in device tree and avoid exporting and calling > read/write operations (which I think looks hacky). Instead they use > altr_sysmgr_regmap_lookup_by_phandle() to get their regmap (which > performs smcc), and then they just use regular regmap_read() / > regmap_write or whatever functions to operate on their regmap object. > That's what I meant by "overriding" the regmap. > > Do you think this approach would be clearer and more "productizable" > so to speak? Just a thought. Keeping it as a regmap was certainly always my preference. I'll try and re-work it in a similar way and see if I hit any blocking issues. Thanks, Peter.
Hi Peter, kernel test robot noticed the following build errors: [auto build test ERROR on krzk/for-next] [also build test ERROR on robh/for-next soc/for-next linus/master v6.8-rc1 next-20240125] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Peter-Griffin/dt-bindings-watchdog-samsung-wdt-deprecate-samsung-syscon-phandle/20240123-070052 base: https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux.git for-next patch link: https://lore.kernel.org/r/20240122225710.1952066-3-peter.griffin%40linaro.org patch subject: [PATCH 2/9] soc: samsung: exynos-pmu: Add exynos_pmu_update/read/write APIs and SoC quirks config: arm-defconfig (https://download.01.org/0day-ci/archive/20240127/202401270110.YlAvkNYL-lkp@intel.com/config) compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240127/202401270110.YlAvkNYL-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202401270110.YlAvkNYL-lkp@intel.com/ All errors (new ones prefixed by >>): >> ld.lld: error: undefined symbol: pmu_base_addr >>> referenced by exynos.c >>> arch/arm/mach-exynos/exynos.o:(exynos_set_delayed_reset_assertion) in archive vmlinux.a >>> referenced by exynos.c >>> arch/arm/mach-exynos/exynos.o:(exynos_set_delayed_reset_assertion) in archive vmlinux.a >>> referenced by exynos.c >>> arch/arm/mach-exynos/exynos.o:(exynos_init_irq) in archive vmlinux.a >>> referenced 75 more times
Hi Peter, kernel test robot noticed the following build errors: [auto build test ERROR on krzk/for-next] [also build test ERROR on robh/for-next soc/for-next linus/master v6.8-rc1 next-20240125] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Peter-Griffin/dt-bindings-watchdog-samsung-wdt-deprecate-samsung-syscon-phandle/20240123-070052 base: https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux.git for-next patch link: https://lore.kernel.org/r/20240122225710.1952066-3-peter.griffin%40linaro.org patch subject: [PATCH 2/9] soc: samsung: exynos-pmu: Add exynos_pmu_update/read/write APIs and SoC quirks config: x86_64-buildonly-randconfig-006-20240126 (https://download.01.org/0day-ci/archive/20240127/202401270633.VorEna6q-lkp@intel.com/config) compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240127/202401270633.VorEna6q-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202401270633.VorEna6q-lkp@intel.com/ All errors (new ones prefixed by >>): In file included from drivers/pinctrl/samsung/pinctrl-exynos.c:27: >> include/linux/soc/samsung/exynos-pmu.h:38:1: error: expected identifier or '(' 38 | { | ^ include/linux/soc/samsung/exynos-pmu.h:44:1: error: expected identifier or '(' 44 | { | ^ >> include/linux/soc/samsung/exynos-pmu.h:50:9: error: incompatible pointer to integer conversion returning 'void *' from a function with result type 'int' [-Wint-conversion] 50 | return ERR_PTR(-ENODEV); | ^~~~~~~~~~~~~~~~ include/linux/soc/samsung/exynos-pmu.h:55:9: error: incompatible pointer to integer conversion returning 'void *' from a function with result type 'int' [-Wint-conversion] 55 | return ERR_PTR(-ENODEV); | ^~~~~~~~~~~~~~~~ 4 errors generated. vim +38 include/linux/soc/samsung/exynos-pmu.h 35 36 static inline int exynos_pmu_update_bits(unsigned int offset, unsigned int mask, 37 unsigned int val); > 38 { 39 return ERR_PTR(-ENODEV); 40 } 41 42 static inline int exynos_pmu_update(unsigned int offset, unsigned int mask, 43 unsigned int val); 44 { 45 return ERR_PTR(-ENODEV); 46 } 47 48 static inline int exynos_pmu_write(unsigned int offset, unsigned int val) 49 { > 50 return ERR_PTR(-ENODEV); 51 } 52
diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c index 250537d7cfd6..e9e933ede568 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,29 +13,204 @@ #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" +/** + * DOC: Quirk flags for different Exynos PMU IP-cores + * + * This driver supports multiple Exynos based SoCs, each of which might have a + * different set of registers and features supported. + * + * Quirk flags described below serve the purpose of telling the driver about + * mentioned SoC traits, and can be specified in driver data for each particular + * supported device. + * + * %QUIRK_HAS_ATOMIC_BITSETHW: PMU IP has special atomic bit set/clear HW + * to protect against PMU registers being accessed from multiple bus masters. + * + * %QUIRK_PMU_ALIVE_WRITE_SEC: PMU registers are *not* write accesible from + * normal world. This is found on some SoCs as a security hardening measure. PMU + * registers on these SoCs can only be written via a SMC call and registers are + * checked by EL3 firmware against an allowlist before the write can procede. + * Note: This quirk should only be set for platforms whose el3 firmware + * implements the TENSOR_SMC_PMU_SEC_REG interface below. + */ + +#define QUIRK_HAS_ATOMIC_BITSETHW BIT(0) +#define QUIRK_PMU_ALIVE_WRITE_SEC BIT(1) + +#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; + phys_addr_t pmu_base_pa; + /* protect PMU reg atomic update operations */ + spinlock_t update_lock; }; -void __iomem *pmu_base_addr; static struct exynos_pmu_context *pmu_context; +/* + * Some 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 + +int set_priv_reg(phys_addr_t reg, u32 val) +{ + struct arm_smccc_res res; + + arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG, + 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; +} + +int rmw_priv_reg(phys_addr_t reg, u32 mask, u32 val) +{ + struct arm_smccc_res res; + + arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG, + 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; +} + +/* + * For SoCs that have set/clear bit hardware (as indicated by + * QUIRK_HAS_ATOMIC_BITSETHW) 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 + * exynos_pmu_set_bit_atomic(0x3e80, 0x3f00, 0x3f00); + * + * To clear bits 13:8 in PMU offset 0x3e80 + * exynos_pmu_set_bit_atomic(0x3e80, 0x0, 0x3f00); + */ +static inline void exynos_pmu_set_bit_atomic(unsigned int offset, + u32 val, u32 mask) +{ + unsigned long flags; + unsigned int i; + + spin_lock_irqsave(&pmu_context->update_lock, flags); + for (i = 0; i < 32; i++) { + if (mask & BIT(i)) { + if (val & BIT(i)) { + offset |= 0xc000; + pmu_raw_writel(i, offset); + } else { + offset |= 0x8000; + pmu_raw_writel(i, offset); + } + } + } + spin_unlock_irqrestore(&pmu_context->update_lock, flags); +} + +int exynos_pmu_update_bits(unsigned int offset, unsigned int mask, + unsigned int val) +{ + if (pmu_context->pmu_data && + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC) + return rmw_priv_reg(pmu_context->pmu_base_pa + offset, + mask, val); + + return regmap_update_bits(pmu_context->pmureg, offset, mask, val); +} +EXPORT_SYMBOL(exynos_pmu_update_bits); + void pmu_raw_writel(u32 val, u32 offset) { - writel_relaxed(val, pmu_base_addr + offset); + if (pmu_context->pmu_data && + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC) + return (void)set_priv_reg(pmu_context->pmu_base_pa + offset, + val); + + return writel_relaxed(val, pmu_context->pmu_base_addr + offset); } u32 pmu_raw_readl(u32 offset) { - return readl_relaxed(pmu_base_addr + offset); + return readl_relaxed(pmu_context->pmu_base_addr + offset); +} + +int exynos_pmu_read(unsigned int offset, unsigned int *val) +{ + if (!pmu_context) + return -ENODEV; + + /* + * For platforms that protect PMU registers they + * are still accessible to read from normal world + */ + return regmap_read(pmu_context->pmureg, offset, val); +} +EXPORT_SYMBOL(exynos_pmu_read); + +int exynos_pmu_write(unsigned int offset, unsigned int val) +{ + if (!pmu_context) + return -ENODEV; + + if (pmu_context->pmu_data && + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC) + return set_priv_reg(pmu_context->pmu_base_pa + offset, val); + + return regmap_write(pmu_context->pmureg, offset, val); +} +EXPORT_SYMBOL(exynos_pmu_write); + +int exynos_pmu_update(unsigned int offset, unsigned int mask, unsigned int val) +{ + int ret = 0; + + if (!pmu_context) + return -ENODEV; + + if (pmu_context->pmu_data && + pmu_context->pmu_data->quirks & QUIRK_HAS_ATOMIC_BITSETHW) { + /* + * Use atomic operations for PMU_ALIVE registers (offset 0~0x3FFF) + * as the target registers can be accessed by multiple masters. + */ + if (offset > PMUALIVE_MASK) + return exynos_pmu_update_bits(offset, mask, val); + + exynos_pmu_set_bit_atomic(offset, val, mask); + + } else { + return exynos_pmu_update_bits(offset, mask, val); + } + + return ret; } +EXPORT_SYMBOL(exynos_pmu_update); void exynos_sys_powerdown_conf(enum sys_powerdown mode) { @@ -75,11 +251,18 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode) #define exynos_pmu_data_arm_ptr(data) NULL #endif +static const struct exynos_pmu_data gs101_pmu_data = { + .quirks = QUIRK_HAS_ATOMIC_BITSETHW | QUIRK_PMU_ALIVE_WRITE_SEC, +}; + /* * 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), }, { @@ -125,18 +308,30 @@ EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap); static int exynos_pmu_probe(struct platform_device *pdev) { + struct resource *res; struct device *dev = &pdev->dev; int ret; - pmu_base_addr = devm_platform_ioremap_resource(pdev, 0); - if (IS_ERR(pmu_base_addr)) - return PTR_ERR(pmu_base_addr); - pmu_context = devm_kzalloc(&pdev->dev, sizeof(struct exynos_pmu_context), GFP_KERNEL); if (!pmu_context) return -ENOMEM; + + pmu_context->pmu_base_addr = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(pmu_context->pmu_base_addr)) + return PTR_ERR(pmu_context->pmu_base_addr); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) + return -ENODEV; + + pmu_context->pmu_base_pa = res->start; + pmu_context->pmureg = exynos_get_pmu_regmap(); + if (IS_ERR(pmu_context->pmureg)) + return PTR_ERR(pmu_context->pmureg); + + spin_lock_init(&pmu_context->update_lock); pmu_context->dev = dev; pmu_context->pmu_data = of_device_get_match_data(dev); diff --git a/drivers/soc/samsung/exynos-pmu.h b/drivers/soc/samsung/exynos-pmu.h index 1c652ffd79b4..570c6e4dc8c3 100644 --- a/drivers/soc/samsung/exynos-pmu.h +++ b/drivers/soc/samsung/exynos-pmu.h @@ -25,8 +25,12 @@ struct exynos_pmu_data { void (*pmu_init)(void); void (*powerdown_conf)(enum sys_powerdown); void (*powerdown_conf_extra)(enum sys_powerdown); + u32 quirks; }; +int set_priv_reg(phys_addr_t reg, u32 val); +int rmw_priv_reg(phys_addr_t reg, u32 mask, u32 val); + extern void __iomem *pmu_base_addr; #ifdef CONFIG_EXYNOS_PMU_ARM_DRIVERS diff --git a/include/linux/soc/samsung/exynos-pmu.h b/include/linux/soc/samsung/exynos-pmu.h index a4f5516cc956..2c5ce21fb00b 100644 --- a/include/linux/soc/samsung/exynos-pmu.h +++ b/include/linux/soc/samsung/exynos-pmu.h @@ -21,11 +21,39 @@ 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 int exynos_pmu_update_bits(unsigned int offset, unsigned int mask, + unsigned int val); +extern int exynos_pmu_update(unsigned int offset, unsigned int mask, + unsigned int val); +extern int exynos_pmu_write(unsigned int offset, unsigned int val); +extern int exynos_pmu_read(unsigned int offset, unsigned int *val); #else static inline struct regmap *exynos_get_pmu_regmap(void) { return ERR_PTR(-ENODEV); } + +static inline int exynos_pmu_update_bits(unsigned int offset, unsigned int mask, + unsigned int val); +{ + return ERR_PTR(-ENODEV); +} + +static inline int exynos_pmu_update(unsigned int offset, unsigned int mask, + unsigned int val); +{ + return ERR_PTR(-ENODEV); +} + +static inline int exynos_pmu_write(unsigned int offset, unsigned int val) +{ + return ERR_PTR(-ENODEV); +} + +static inline int exynos_pmu_read(unsigned int offset, unsigned int *val) +{ + return ERR_PTR(-ENODEV); +} #endif #endif /* __LINUX_SOC_EXYNOS_PMU_H */
Newer Exynos SoCs have atomic set/clear bit hardware for PMU registers as these registers can be accessed by multiple masters. Some platforms also protect the PMU registers for security hardening reasons so they can't be written by normal world and are only write acessible in el3 via a SMC call. Add support for both of these usecases using SoC specific quirks that are determined from the DT compatible string. Drivers which need to read and write PMU registers should now use these new exynos_pmu_*() APIs instead of obtaining a regmap using syscon_regmap_lookup_by_phandle() Depending on the SoC specific quirks, the exynos_pmu_*() APIs will access the PMU register in the appropriate way. Signed-off-by: Peter Griffin <peter.griffin@linaro.org> --- drivers/soc/samsung/exynos-pmu.c | 209 ++++++++++++++++++++++++- drivers/soc/samsung/exynos-pmu.h | 4 + include/linux/soc/samsung/exynos-pmu.h | 28 ++++ 3 files changed, 234 insertions(+), 7 deletions(-)