From: Kevin Hilman <khilman@baylibre.com>
To: Jianxin Pan <jianxin.pan@amlogic.com>, linux-amlogic@lists.infradead.org
Cc: devicetree@vger.kernel.org, Hanjie Lin <hanjie.lin@amlogic.com>,
Victor Wan <victor.wan@amlogic.com>,
linux-pm@vger.kernel.org,
Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
Neil Armstrong <narmstrong@baylibre.com>,
linux-kernel@vger.kernel.org,
Zhiqiang Liang <zhiqiang.liang@amlogic.com>,
Rob Herring <robh+dt@kernel.org>, Jian Hu <jian.hu@amlogic.com>,
Xingyu Chen <xingyu.chen@amlogic.com>,
linux-arm-kernel@lists.infradead.org,
Jerome Brunet <jbrunet@baylibre.com>
Subject: Re: [PATCH 2/3] soc: amlogic: Add support for Secure power domains controller
Date: Thu, 26 Sep 2019 09:23:42 -0700 [thread overview]
Message-ID: <7ha7arrppt.fsf@baylibre.com> (raw)
In-Reply-To: <3859c748-01f0-4dbd-05d6-20fff31edf11@amlogic.com>
Jianxin Pan <jianxin.pan@amlogic.com> 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 <jianxin.pan@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.pan@amlogic.com>
>>> Signed-off-by: Zhiqiang Liang <zhiqiang.liang@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?
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/20190925213528.21515-2-khilman@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.
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
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
next prev parent reply other threads:[~2019-09-26 16:24 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-19 12:11 [PATCH 0/3] arm64: meson: add support for A1 Power Domains Jianxin Pan
2019-09-19 12:11 ` [PATCH 1/3] dt-bindings: power: add Amlogic secure power domains bindings Jianxin Pan
2019-09-19 20:06 ` Martin Blumenstingl
2019-09-20 7:37 ` Jianxin Pan
2019-10-01 22:09 ` Rob Herring
2019-10-10 3:31 ` Jianxin Pan
2019-09-19 12:11 ` [PATCH 2/3] soc: amlogic: Add support for Secure power domains controller Jianxin Pan
2019-09-19 20:03 ` Martin Blumenstingl
2019-09-20 7:58 ` Jianxin Pan
2019-09-25 22:41 ` Kevin Hilman
2019-09-26 9:17 ` Jianxin Pan
2019-09-26 16:23 ` Kevin Hilman [this message]
2019-09-19 12:11 ` [PATCH 3/3] arm64: dts: meson: a1: add secure power domain controller Jianxin Pan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7ha7arrppt.fsf@baylibre.com \
--to=khilman@baylibre.com \
--cc=devicetree@vger.kernel.org \
--cc=hanjie.lin@amlogic.com \
--cc=jbrunet@baylibre.com \
--cc=jian.hu@amlogic.com \
--cc=jianxin.pan@amlogic.com \
--cc=linux-amlogic@lists.infradead.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=martin.blumenstingl@googlemail.com \
--cc=narmstrong@baylibre.com \
--cc=robh+dt@kernel.org \
--cc=victor.wan@amlogic.com \
--cc=xingyu.chen@amlogic.com \
--cc=zhiqiang.liang@amlogic.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).