Hi Kevin, Thanks for your review. Please see my comments below.
On 2019/9/26 6:41, Kevin Hilman wrote: > Hi Jianxin, > > Jianxin Pan <jianxin....@amlogic.com> 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 <jianxin....@amlogic.com> >> Signed-off-by: Zhiqiang Liang <zhiqiang.li...@amlogic.com> > > 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? >> +} > > 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/20190925213528.21515-2-khil...@kernel.org/ > 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. > >> + 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. >> + SEC_PD(I2C, 0), >> + SEC_PD(PSRAM, 0), >> + SEC_PD(ACODEC, 0), >> + SEC_PD(AUDIO, 0), >> + SEC_PD(OTP, 0), >> + SEC_PD(DMA, 0), >> + SEC_PD(SD_EMMC, 0), >> + SEC_PD(RAMA, 0), >> + SEC_PD(RAMB, GENPD_FLAG_ALWAYS_ON), > > Please explain the need for ALWAYS_ON. > In A1, SRAMB is used for bl31 ATF. >> + SEC_PD(IR, 0), >> + SEC_PD(SPICC, 0), >> + SEC_PD(SPIFC, 0), >> + SEC_PD(USB, 0), >> + SEC_PD(NIC, GENPD_FLAG_ALWAYS_ON), > > Please explain the need for ALWAYS_ON. > PD_NIC is used for NIC400, and should keep on. >> + SEC_PD(PDMIN, 0), >> + SEC_PD(RSA, 0), >> +}; >> + >> +static int meson_secure_pwrc_probe(struct platform_device *pdev) >> +{ >> + const struct meson_secure_pwrc_domain_data *match; >> + struct meson_secure_pwrc *pwrc; >> + int i; [...] >> + >> + return of_genpd_add_provider_onecell(pdev->dev.of_node, &pwrc->xlate); >> +} >> + >> +static struct meson_secure_pwrc_domain_data meson_secure_a1_pwrc_data = { >> + .domains = a1_pwrc_domains, >> + .count = ARRAY_SIZE(a1_pwrc_domains), >> +}; >> + >> +static const struct of_device_id meson_secure_pwrc_match_table[] = { >> + { >> + .compatible = "amlogic,meson-a1-pwrc", >> + .data = &meson_secure_a1_pwrc_data, >> + }, >> + { } > > as mentioned by Martin, please add the sentinel string here. Helps for > readability. > OK, I will fix it. Thank you. >> +}; >> + >> +static struct platform_driver meson_secure_pwrc_driver = { >> + .probe = meson_secure_pwrc_probe, >> + .driver = { >> + .name = "meson_secure_pwrc", >> + .of_match_table = meson_secure_pwrc_match_table, >> + }, >> +}; >> + >> +static int meson_secure_pwrc_init(void) >> +{ >> + return platform_driver_register(&meson_secure_pwrc_driver); >> +} >> +arch_initcall_sync(meson_secure_pwrc_init); > > Please use builtin_platform_driver() or explain in detail why the > initcall is needed. > OK, I will use builtin_platform_driver instead. > Thanks, > > Kevin > > . >