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]>
---
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)
+/*
+ * Copyright (c) 2019 Amlogic, Inc.
+ * Author: Jianxin Pan <[email protected]>
+ */
+#include <linux/io.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <dt-bindings/power/meson-a1-power.h>
+#include <linux/arm-smccc.h>
+
+#define PWRC_ON 1
+#define PWRC_OFF 0
+#define SMC_PWRC_SET 0x82000093
+#define SMC_PWRC_GET 0x82000095
+
+struct meson_secure_pwrc_domain {
+ struct generic_pm_domain base;
+ unsigned int index;
+};
+
+struct meson_secure_pwrc {
+ struct meson_secure_pwrc_domain *domains;
+ struct genpd_onecell_data xlate;
+};
+
+struct meson_secure_pwrc_domain_desc {
+ unsigned int index;
+ unsigned int flags;
+ char *name;
+ bool (*get_power)(struct meson_secure_pwrc_domain *pwrc_domain);
+};
+
+struct meson_secure_pwrc_domain_data {
+ unsigned int count;
+ struct meson_secure_pwrc_domain_desc *domains;
+};
+
+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;
+}
+
+static int meson_secure_pwrc_off(struct generic_pm_domain *domain)
+{
+ struct arm_smccc_res res;
+ struct meson_secure_pwrc_domain *pwrc_domain =
+ container_of(domain, struct meson_secure_pwrc_domain, base);
+
+ arm_smccc_smc(SMC_PWRC_SET, pwrc_domain->index, PWRC_OFF,
+ 0, 0, 0, 0, 0, &res);
+
+ return 0;
+}
+
+static int meson_secure_pwrc_on(struct generic_pm_domain *domain)
+{
+ struct arm_smccc_res res;
+ struct meson_secure_pwrc_domain *pwrc_domain =
+ container_of(domain, struct meson_secure_pwrc_domain, base);
+
+ arm_smccc_smc(SMC_PWRC_SET, pwrc_domain->index, PWRC_ON,
+ 0, 0, 0, 0, 0, &res);
+
+ return 0;
+}
+
+#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),
+ SEC_PD(DMC, GENPD_FLAG_ALWAYS_ON),
+ 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),
+ SEC_PD(IR, 0),
+ SEC_PD(SPICC, 0),
+ SEC_PD(SPIFC, 0),
+ SEC_PD(USB, 0),
+ SEC_PD(NIC, GENPD_FLAG_ALWAYS_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;
+
+ match = of_device_get_match_data(&pdev->dev);
+ if (!match) {
+ dev_err(&pdev->dev, "failed to get match data\n");
+ return -ENODEV;
+ }
+
+ pwrc = devm_kzalloc(&pdev->dev, sizeof(*pwrc), GFP_KERNEL);
+ if (!pwrc)
+ return -ENOMEM;
+
+ pwrc->xlate.domains = devm_kcalloc(&pdev->dev, match->count,
+ sizeof(*pwrc->xlate.domains),
+ GFP_KERNEL);
+ if (!pwrc->xlate.domains)
+ return -ENOMEM;
+
+ pwrc->domains = devm_kcalloc(&pdev->dev, match->count,
+ sizeof(*pwrc->domains), GFP_KERNEL);
+ if (!pwrc->domains)
+ return -ENOMEM;
+
+ pwrc->xlate.num_domains = match->count;
+ platform_set_drvdata(pdev, pwrc);
+
+ for (i = 0 ; i < match->count ; ++i) {
+ struct meson_secure_pwrc_domain *dom = &pwrc->domains[i];
+
+ if (!match->domains[i].index)
+ continue;
+
+ dom->index = match->domains[i].index;
+ dom->base.name = match->domains[i].name;
+ dom->base.flags = match->domains[i].flags;
+ dom->base.power_on = meson_secure_pwrc_on;
+ dom->base.power_off = meson_secure_pwrc_off;
+
+ pm_genpd_init(&dom->base, NULL,
+ (match->domains[i].get_power ?
+ match->domains[i].get_power(dom) : true));
+
+ pwrc->xlate.domains[i] = &dom->base;
+ }
+
+ 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,
+ },
+ { }
+};
+
+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);
--
2.7.4
Hi Jianxin,
I added three comments below from a quick glance at this driver (I
didn't have time for a complete review)
On Thu, Sep 19, 2019 at 2:11 PM Jianxin Pan <[email protected]> wrote:
[...]
> + pm_genpd_init(&dom->base, NULL,
> + (match->domains[i].get_power ?
> + match->domains[i].get_power(dom) : true));
.get_power is never NULL in this driver so the ": true" part is
effectively a no-op
[...]
> +static const struct of_device_id meson_secure_pwrc_match_table[] = {
> + {
> + .compatible = "amlogic,meson-a1-pwrc",
> + .data = &meson_secure_a1_pwrc_data,
> + },
> + { }
many drivers use a /* sentinel */ comment inside { }
[...]
> +arch_initcall_sync(meson_secure_pwrc_init);
why arch_initcall_sync instead of builtin_platform_driver?
$ grep -R arch_initcall_sync drivers/soc/
$
Martin
Hi Martin,
On 2019/9/20 4:03, Martin Blumenstingl wrote:
> Hi Jianxin,
>
> I added three comments below from a quick glance at this driver (I
> didn't have time for a complete review)
>
> On Thu, Sep 19, 2019 at 2:11 PM Jianxin Pan <[email protected]> wrote:
> [...]
>> + pm_genpd_init(&dom->base, NULL,
>> + (match->domains[i].get_power ?
>> + match->domains[i].get_power(dom) : true));
> .get_power is never NULL in this driver so the ": true" part is
> effectively a no-op
>
OK, I will remove it. Thanks for your time.
> [...]
>> +static const struct of_device_id meson_secure_pwrc_match_table[] = {
>> + {
>> + .compatible = "amlogic,meson-a1-pwrc",
>> + .data = &meson_secure_a1_pwrc_data,
>> + },
>> + { }
> many drivers use a /* sentinel */ comment inside { }
>
OK, I will add this comment line.
> [...]
>> +arch_initcall_sync(meson_secure_pwrc_init);
> why arch_initcall_sync instead of builtin_platform_driver?
> $ grep -R arch_initcall_sync drivers/soc/
> $
>
>
> Martin
>
The power-domain is depended by many other drivers, arch_initcall_sync is used to make power-domain probe earlier.
Maybe I need to switch back to builtin_platform_driver when use APIs from meson_sm.c.
> .
>
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)
> +/*
> + * Copyright (c) 2019 Amlogic, Inc.
> + * Author: Jianxin Pan <[email protected]>
> + */
> +#include <linux/io.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <dt-bindings/power/meson-a1-power.h>
> +#include <linux/arm-smccc.h>
> +
> +#define PWRC_ON 1
> +#define PWRC_OFF 0
> +#define SMC_PWRC_SET 0x82000093
> +#define SMC_PWRC_GET 0x82000095
> +
> +struct meson_secure_pwrc_domain {
> + struct generic_pm_domain base;
> + unsigned int index;
> +};
> +
> +struct meson_secure_pwrc {
> + struct meson_secure_pwrc_domain *domains;
> + struct genpd_onecell_data xlate;
> +};
> +
> +struct meson_secure_pwrc_domain_desc {
> + unsigned int index;
> + unsigned int flags;
> + char *name;
> + bool (*get_power)(struct meson_secure_pwrc_domain *pwrc_domain);
> +};
> +
> +struct meson_secure_pwrc_domain_data {
> + unsigned int count;
> + struct meson_secure_pwrc_domain_desc *domains;
> +};
> +
> +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.
> +}
What does the return value for this function mean? Does true mean
"powered off" or "powered on"?
See the rename I just did on the ee-pwrc driver:
https://lore.kernel.org/linux-amlogic/[email protected]/
> +static int meson_secure_pwrc_off(struct generic_pm_domain *domain)
> +{
> + struct arm_smccc_res res;
> + struct meson_secure_pwrc_domain *pwrc_domain =
> + container_of(domain, struct meson_secure_pwrc_domain, base);
> +
> + arm_smccc_smc(SMC_PWRC_SET, pwrc_domain->index, PWRC_OFF,
> + 0, 0, 0, 0, 0, &res);
> +
> + return 0;
> +}
> +
> +static int meson_secure_pwrc_on(struct generic_pm_domain *domain)
> +{
> + struct arm_smccc_res res;
> + struct meson_secure_pwrc_domain *pwrc_domain =
> + container_of(domain, struct meson_secure_pwrc_domain, base);
> +
> + arm_smccc_smc(SMC_PWRC_SET, pwrc_domain->index, PWRC_ON,
> + 0, 0, 0, 0, 0, &res);
> +
> + return 0;
> +}
> +
> +#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.
> + SEC_PD(DMC, GENPD_FLAG_ALWAYS_ON),
Please explain the need for ALWAYS_ON.
> + 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.
> + 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.
> + 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;
> +
> + match = of_device_get_match_data(&pdev->dev);
> + if (!match) {
> + dev_err(&pdev->dev, "failed to get match data\n");
> + return -ENODEV;
> + }
> +
> + pwrc = devm_kzalloc(&pdev->dev, sizeof(*pwrc), GFP_KERNEL);
> + if (!pwrc)
> + return -ENOMEM;
> +
> + pwrc->xlate.domains = devm_kcalloc(&pdev->dev, match->count,
> + sizeof(*pwrc->xlate.domains),
> + GFP_KERNEL);
> + if (!pwrc->xlate.domains)
> + return -ENOMEM;
> +
> + pwrc->domains = devm_kcalloc(&pdev->dev, match->count,
> + sizeof(*pwrc->domains), GFP_KERNEL);
> + if (!pwrc->domains)
> + return -ENOMEM;
> +
> + pwrc->xlate.num_domains = match->count;
> + platform_set_drvdata(pdev, pwrc);
> +
> + for (i = 0 ; i < match->count ; ++i) {
> + struct meson_secure_pwrc_domain *dom = &pwrc->domains[i];
> +
> + if (!match->domains[i].index)
> + continue;
> +
> + dom->index = match->domains[i].index;
> + dom->base.name = match->domains[i].name;
> + dom->base.flags = match->domains[i].flags;
> + dom->base.power_on = meson_secure_pwrc_on;
> + dom->base.power_off = meson_secure_pwrc_off;
> +
> + pm_genpd_init(&dom->base, NULL,
> + (match->domains[i].get_power ?
> + match->domains[i].get_power(dom) : true));
> +
> + pwrc->xlate.domains[i] = &dom->base;
> + }
> +
> + 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.
> +};
> +
> +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.
Thanks,
Kevin
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?
>> +}
>
> 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.
>
>> + 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
>
> .
>
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