Jianxin Pan <[email protected]> writes: > Hi Kevin, > > Thanks for your review. Please see my comments below. > > > On 2019/9/26 6:41, Kevin Hilman wrote: >> Hi Jianxin, >> >> Jianxin Pan <[email protected]> writes: >> >>> Add support for the Amlogic Secure Power controller. In A1/C1 series, power >>> control registers are in secure domain, and should be accessed by smc. >>> >>> Signed-off-by: Jianxin Pan <[email protected]> >>> Signed-off-by: Zhiqiang Liang <[email protected]> >> >> Thanks for the new power domain driver. >> >>> --- >>> drivers/soc/amlogic/Kconfig | 13 +++ >>> drivers/soc/amlogic/Makefile | 1 + >>> drivers/soc/amlogic/meson-secure-pwrc.c | 182 >>> ++++++++++++++++++++++++++++++++ >>> 3 files changed, 196 insertions(+) >>> create mode 100644 drivers/soc/amlogic/meson-secure-pwrc.c >>> >>> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig >>> index bc2c912..6cb06e7 100644 >>> --- a/drivers/soc/amlogic/Kconfig >>> +++ b/drivers/soc/amlogic/Kconfig >>> @@ -48,6 +48,19 @@ config MESON_EE_PM_DOMAINS >>> Say yes to expose Amlogic Meson Everything-Else Power Domains as >>> Generic Power Domains. >>> >>> +config MESON_SECURE_PM_DOMAINS >>> + bool "Amlogic Meson Secure Power Domains driver" >>> + depends on ARCH_MESON || COMPILE_TEST >>> + depends on PM && OF >>> + depends on HAVE_ARM_SMCCC >>> + default ARCH_MESON >>> + select PM_GENERIC_DOMAINS >>> + select PM_GENERIC_DOMAINS_OF >>> + help >>> + Support for the power controller on Amlogic A1/C1 series. >>> + Say yes to expose Amlogic Meson Secure Power Domains as Generic >>> + Power Domains. >>> + >>> config MESON_MX_SOCINFO >>> bool "Amlogic Meson MX SoC Information driver" >>> depends on ARCH_MESON || COMPILE_TEST >>> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile >>> index de79d044..7b8c5d3 100644 >>> --- a/drivers/soc/amlogic/Makefile >>> +++ b/drivers/soc/amlogic/Makefile >>> @@ -5,3 +5,4 @@ obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o >>> obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o >>> obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o >>> obj-$(CONFIG_MESON_EE_PM_DOMAINS) += meson-ee-pwrc.o >>> +obj-$(CONFIG_MESON_SECURE_PM_DOMAINS) += meson-secure-pwrc.o >>> diff --git a/drivers/soc/amlogic/meson-secure-pwrc.c >>> b/drivers/soc/amlogic/meson-secure-pwrc.c >>> new file mode 100644 >>> index 00000000..00c7232 >>> --- /dev/null >>> +++ b/drivers/soc/amlogic/meson-secure-pwrc.c >>> @@ -0,0 +1,182 @@ >>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > [...] >>> + >>> +static bool pwrc_secure_get_power(struct meson_secure_pwrc_domain >>> *pwrc_domain) >>> +{ >>> + struct arm_smccc_res res; >>> + >>> + arm_smccc_smc(SMC_PWRC_GET, pwrc_domain->index, 0, >>> + 0, 0, 0, 0, 0, &res); >>> + >>> + return res.a0 & 0x1; >> >> Please use a #define with a readable name for this mask. >> The return type of this smc is bool. I will remove 0x1 mask in next version. > > Another question about smc: > In this driver, no share memory is needed, and I use arm_smccc_smc() directly. > Should I add secure-monitor = <&sm> in dtb and use meson_sm_call() from sm > driver instead?
Yes, that would be preferred. >>> +} >> >> What does the return value for this function mean? Does true mean >> "powered off" or "powered on"> > The return vaule for SMC_PWRC_GET : > 0 -> power on > 1 -> power off> See the rename I just did on the ee-pwrc driver: >> https://lore.kernel.org/linux-amlogic/[email protected]/ >> I will follow and rename to _is_off() in the next verson. >>> +static int meson_secure_pwrc_off(struct generic_pm_domain *domain) >>> +{ >>> + struct arm_smccc_res res; >>> + struct meson_secure_pwrc_domain *pwrc_domain = > [...] >>> + >>> +#define SEC_PD(__name, __flag) \ >>> +{ \ >>> + .name = #__name, \ >>> + .index = PWRC_##__name##_ID, \ >>> + .get_power = pwrc_secure_get_power, \ >>> + .flags = __flag, \ >>> +} >>> + >>> +static struct meson_secure_pwrc_domain_desc a1_pwrc_domains[] = { >>> + SEC_PD(DSPA, 0), >>> + SEC_PD(DSPB, 0), >>> + SEC_PD(UART, GENPD_FLAG_ALWAYS_ON), >> >> This flag should only be used for domains where there are no linux >> drivers. >> >> Rather than using this flag, you need to add a 'power-domain' property >> to the uart driver in DT, and then update the meson_uart driver to use >> the runtime PM API so that the domain is enabled whenever the UART is in >> use. > > PM_UART Power domain is shared by uart, msr, jtag and cec. > Uart should keep working in BL31, after kernel suspend and before kernel > resume. OK. >> >>> + SEC_PD(DMC, GENPD_FLAG_ALWAYS_ON), >> >> Please explain the need for ALWAYS_ON. >> > PM_DMC is used for DDR PHY ana/dig and DMC. > There is no linux drver for them, and it should be always on. > > I will add comments for all these always on domains. OK, thank you. Kevin
