From: Kevin Hilman <khilman@baylibre.com>
To: Jianxin Pan <jianxin.pan@amlogic.com>, linux-amlogic@lists.infradead.org
Cc: Jianxin Pan <jianxin.pan@amlogic.com>,
Rob Herring <robh+dt@kernel.org>,
Neil Armstrong <narmstrong@baylibre.com>,
Jerome Brunet <jbrunet@baylibre.com>,
Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
Jian Hu <jian.hu@amlogic.com>,
Hanjie Lin <hanjie.lin@amlogic.com>,
Victor Wan <victor.wan@amlogic.com>,
Xingyu Chen <xingyu.chen@amlogic.com>
Subject: Re: [PATCH v4 3/4] soc: amlogic: Add support for Secure power domains controller
Date: Sat, 09 Nov 2019 21:09:31 +0100 [thread overview]
Message-ID: <7hmud4stfo.fsf@baylibre.com> (raw)
In-Reply-To: <1572868028-73076-4-git-send-email-jianxin.pan@amlogic.com>
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>
This driver is looking pretty good. A few more minor comments below.
[...]
> +static bool pwrc_secure_is_off(struct meson_secure_pwrc_domain *pwrc_domain)
> +{
> + int sts = 1;
What does 'sts' mean? status? or something else? Please use a more
descriptive name.
> + if (meson_sm_call(pwrc_domain->pwrc->fw, SM_PWRC_GET, &sts,
> + pwrc_domain->index, 0, 0, 0, 0) < 0)
> + pr_err("failed to get power domain status\n");
Does any bit in this register mean the power domain is off? I think it
would be better (and more future proof) if you checked the specific bit
(or mask)
> + return !!sts;
and then:
return sts & bitmask;
> +}
> +
> +static int meson_secure_pwrc_off(struct generic_pm_domain *domain)
> +{
> + int sts = 0;
Like above, what does sts mean?
> + struct meson_secure_pwrc_domain *pwrc_domain =
> + container_of(domain, struct meson_secure_pwrc_domain, base);
> +
> + if (meson_sm_call(pwrc_domain->pwrc->fw, SM_PWRC_SET, NULL,
> + pwrc_domain->index, PWRC_OFF, 0, 0, 0) < 0) {
> + pr_err("failed to set power domain off\n");
> + sts = -EINVAL;
> + }
> +
> + return sts;
It looks to me like sts is only used as a return code, so maybe call it
ret for clarity? or rename it to something more descriptive.
> +}
> +
> +static int meson_secure_pwrc_on(struct generic_pm_domain *domain)
> +{
> + int sts = 0;
> + struct meson_secure_pwrc_domain *pwrc_domain =
> + container_of(domain, struct meson_secure_pwrc_domain, base);
> +
> + if (meson_sm_call(pwrc_domain->pwrc->fw, SM_PWRC_SET, NULL,
> + pwrc_domain->index, PWRC_ON, 0, 0, 0) < 0) {
> + pr_err("failed to set power domain on\n");
> + sts = -EINVAL;
> + }
> +
> + return sts;
same comment as above.
> +}
> +
> +#define SEC_PD(__name, __flag) \
> +[PWRC_##__name##_ID] = \
> +{ \
> + .name = #__name, \
> + .index = PWRC_##__name##_ID, \
> + .is_off = pwrc_secure_is_off, \
> + .flags = __flag, \
> +}
> +
> +static struct meson_secure_pwrc_domain_desc a1_pwrc_domains[] = {
> + SEC_PD(DSPA, 0),
> + SEC_PD(DSPB, 0),
> + /* UART should keep working in ATF after suspend and before resume */
> + SEC_PD(UART, GENPD_FLAG_ALWAYS_ON),
> + /* DMC is for DDR PHY ana/dig and DMC, and should be 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),
> + /* SRAMB is used as AFT runtime memory, and should be always on */
AFT? I assume you mean ATF?
> + SEC_PD(RAMB, GENPD_FLAG_ALWAYS_ON),
> + SEC_PD(IR, 0),
> + SEC_PD(SPICC, 0),
> + SEC_PD(SPIFC, 0),
> + SEC_PD(USB, 0),
> + /* NIC is for NIC400, and should be always on */
Why?
> + SEC_PD(NIC, GENPD_FLAG_ALWAYS_ON),
> + SEC_PD(PDMIN, 0),
> + SEC_PD(RSA, 0),
> +};
[...]
Kevin
WARNING: multiple messages have this Message-ID (diff)
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>,
Jianxin Pan <jianxin.pan@amlogic.com>,
linux-pm@vger.kernel.org,
Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
Neil Armstrong <narmstrong@baylibre.com>,
linux-kernel@vger.kernel.org, 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 v4 3/4] soc: amlogic: Add support for Secure power domains controller
Date: Sat, 09 Nov 2019 21:09:31 +0100 [thread overview]
Message-ID: <7hmud4stfo.fsf@baylibre.com> (raw)
In-Reply-To: <1572868028-73076-4-git-send-email-jianxin.pan@amlogic.com>
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>
This driver is looking pretty good. A few more minor comments below.
[...]
> +static bool pwrc_secure_is_off(struct meson_secure_pwrc_domain *pwrc_domain)
> +{
> + int sts = 1;
What does 'sts' mean? status? or something else? Please use a more
descriptive name.
> + if (meson_sm_call(pwrc_domain->pwrc->fw, SM_PWRC_GET, &sts,
> + pwrc_domain->index, 0, 0, 0, 0) < 0)
> + pr_err("failed to get power domain status\n");
Does any bit in this register mean the power domain is off? I think it
would be better (and more future proof) if you checked the specific bit
(or mask)
> + return !!sts;
and then:
return sts & bitmask;
> +}
> +
> +static int meson_secure_pwrc_off(struct generic_pm_domain *domain)
> +{
> + int sts = 0;
Like above, what does sts mean?
> + struct meson_secure_pwrc_domain *pwrc_domain =
> + container_of(domain, struct meson_secure_pwrc_domain, base);
> +
> + if (meson_sm_call(pwrc_domain->pwrc->fw, SM_PWRC_SET, NULL,
> + pwrc_domain->index, PWRC_OFF, 0, 0, 0) < 0) {
> + pr_err("failed to set power domain off\n");
> + sts = -EINVAL;
> + }
> +
> + return sts;
It looks to me like sts is only used as a return code, so maybe call it
ret for clarity? or rename it to something more descriptive.
> +}
> +
> +static int meson_secure_pwrc_on(struct generic_pm_domain *domain)
> +{
> + int sts = 0;
> + struct meson_secure_pwrc_domain *pwrc_domain =
> + container_of(domain, struct meson_secure_pwrc_domain, base);
> +
> + if (meson_sm_call(pwrc_domain->pwrc->fw, SM_PWRC_SET, NULL,
> + pwrc_domain->index, PWRC_ON, 0, 0, 0) < 0) {
> + pr_err("failed to set power domain on\n");
> + sts = -EINVAL;
> + }
> +
> + return sts;
same comment as above.
> +}
> +
> +#define SEC_PD(__name, __flag) \
> +[PWRC_##__name##_ID] = \
> +{ \
> + .name = #__name, \
> + .index = PWRC_##__name##_ID, \
> + .is_off = pwrc_secure_is_off, \
> + .flags = __flag, \
> +}
> +
> +static struct meson_secure_pwrc_domain_desc a1_pwrc_domains[] = {
> + SEC_PD(DSPA, 0),
> + SEC_PD(DSPB, 0),
> + /* UART should keep working in ATF after suspend and before resume */
> + SEC_PD(UART, GENPD_FLAG_ALWAYS_ON),
> + /* DMC is for DDR PHY ana/dig and DMC, and should be 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),
> + /* SRAMB is used as AFT runtime memory, and should be always on */
AFT? I assume you mean ATF?
> + SEC_PD(RAMB, GENPD_FLAG_ALWAYS_ON),
> + SEC_PD(IR, 0),
> + SEC_PD(SPICC, 0),
> + SEC_PD(SPIFC, 0),
> + SEC_PD(USB, 0),
> + /* NIC is for NIC400, and should be always on */
Why?
> + SEC_PD(NIC, GENPD_FLAG_ALWAYS_ON),
> + SEC_PD(PDMIN, 0),
> + SEC_PD(RSA, 0),
> +};
[...]
Kevin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
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>,
Jianxin Pan <jianxin.pan@amlogic.com>,
linux-pm@vger.kernel.org,
Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
Neil Armstrong <narmstrong@baylibre.com>,
linux-kernel@vger.kernel.org, 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 v4 3/4] soc: amlogic: Add support for Secure power domains controller
Date: Sat, 09 Nov 2019 21:09:31 +0100 [thread overview]
Message-ID: <7hmud4stfo.fsf@baylibre.com> (raw)
In-Reply-To: <1572868028-73076-4-git-send-email-jianxin.pan@amlogic.com>
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>
This driver is looking pretty good. A few more minor comments below.
[...]
> +static bool pwrc_secure_is_off(struct meson_secure_pwrc_domain *pwrc_domain)
> +{
> + int sts = 1;
What does 'sts' mean? status? or something else? Please use a more
descriptive name.
> + if (meson_sm_call(pwrc_domain->pwrc->fw, SM_PWRC_GET, &sts,
> + pwrc_domain->index, 0, 0, 0, 0) < 0)
> + pr_err("failed to get power domain status\n");
Does any bit in this register mean the power domain is off? I think it
would be better (and more future proof) if you checked the specific bit
(or mask)
> + return !!sts;
and then:
return sts & bitmask;
> +}
> +
> +static int meson_secure_pwrc_off(struct generic_pm_domain *domain)
> +{
> + int sts = 0;
Like above, what does sts mean?
> + struct meson_secure_pwrc_domain *pwrc_domain =
> + container_of(domain, struct meson_secure_pwrc_domain, base);
> +
> + if (meson_sm_call(pwrc_domain->pwrc->fw, SM_PWRC_SET, NULL,
> + pwrc_domain->index, PWRC_OFF, 0, 0, 0) < 0) {
> + pr_err("failed to set power domain off\n");
> + sts = -EINVAL;
> + }
> +
> + return sts;
It looks to me like sts is only used as a return code, so maybe call it
ret for clarity? or rename it to something more descriptive.
> +}
> +
> +static int meson_secure_pwrc_on(struct generic_pm_domain *domain)
> +{
> + int sts = 0;
> + struct meson_secure_pwrc_domain *pwrc_domain =
> + container_of(domain, struct meson_secure_pwrc_domain, base);
> +
> + if (meson_sm_call(pwrc_domain->pwrc->fw, SM_PWRC_SET, NULL,
> + pwrc_domain->index, PWRC_ON, 0, 0, 0) < 0) {
> + pr_err("failed to set power domain on\n");
> + sts = -EINVAL;
> + }
> +
> + return sts;
same comment as above.
> +}
> +
> +#define SEC_PD(__name, __flag) \
> +[PWRC_##__name##_ID] = \
> +{ \
> + .name = #__name, \
> + .index = PWRC_##__name##_ID, \
> + .is_off = pwrc_secure_is_off, \
> + .flags = __flag, \
> +}
> +
> +static struct meson_secure_pwrc_domain_desc a1_pwrc_domains[] = {
> + SEC_PD(DSPA, 0),
> + SEC_PD(DSPB, 0),
> + /* UART should keep working in ATF after suspend and before resume */
> + SEC_PD(UART, GENPD_FLAG_ALWAYS_ON),
> + /* DMC is for DDR PHY ana/dig and DMC, and should be 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),
> + /* SRAMB is used as AFT runtime memory, and should be always on */
AFT? I assume you mean ATF?
> + SEC_PD(RAMB, GENPD_FLAG_ALWAYS_ON),
> + SEC_PD(IR, 0),
> + SEC_PD(SPICC, 0),
> + SEC_PD(SPIFC, 0),
> + SEC_PD(USB, 0),
> + /* NIC is for NIC400, and should be always on */
Why?
> + SEC_PD(NIC, GENPD_FLAG_ALWAYS_ON),
> + SEC_PD(PDMIN, 0),
> + SEC_PD(RSA, 0),
> +};
[...]
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-11-10 0:30 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-04 11:47 [PATCH v4 0/4] arm64: meson: add support for A1 Power Domains Jianxin Pan
2019-11-04 11:47 ` Jianxin Pan
2019-11-04 11:47 ` Jianxin Pan
2019-11-04 11:47 ` [PATCH v4 1/4] dt-bindings: power: add Amlogic secure power domains bindings Jianxin Pan
2019-11-04 11:47 ` Jianxin Pan
2019-11-04 11:47 ` Jianxin Pan
2019-11-04 11:47 ` [PATCH v4 2/4] firmware: meson_sm: Add secure power domain support Jianxin Pan
2019-11-04 11:47 ` Jianxin Pan
2019-11-04 11:47 ` Jianxin Pan
2019-11-09 20:11 ` Kevin Hilman
2019-11-09 20:11 ` Kevin Hilman
2019-11-09 20:11 ` Kevin Hilman
2019-11-11 10:46 ` Jianxin Pan
2019-11-11 10:46 ` Jianxin Pan
2019-11-11 10:46 ` Jianxin Pan
2019-11-11 14:40 ` Kevin Hilman
2019-11-11 14:40 ` Kevin Hilman
2019-11-11 14:40 ` Kevin Hilman
2019-11-11 14:59 ` Jianxin Pan
2019-11-11 14:59 ` Jianxin Pan
2019-11-11 14:59 ` Jianxin Pan
2019-11-04 11:47 ` [PATCH v4 3/4] soc: amlogic: Add support for Secure power domains controller Jianxin Pan
2019-11-04 11:47 ` Jianxin Pan
2019-11-04 11:47 ` Jianxin Pan
2019-11-09 20:09 ` Kevin Hilman [this message]
2019-11-09 20:09 ` Kevin Hilman
2019-11-09 20:09 ` Kevin Hilman
2019-11-11 11:53 ` Jianxin Pan
2019-11-11 11:53 ` Jianxin Pan
2019-11-11 11:53 ` Jianxin Pan
2019-11-11 14:44 ` Kevin Hilman
2019-11-11 14:44 ` Kevin Hilman
2019-11-11 14:44 ` Kevin Hilman
2019-11-11 15:00 ` Jianxin Pan
2019-11-11 15:00 ` Jianxin Pan
2019-11-11 15:00 ` Jianxin Pan
2019-11-04 11:47 ` [PATCH v4 4/4] arm64: dts: meson: a1: add secure power domain controller Jianxin Pan
2019-11-04 11:47 ` Jianxin Pan
2019-11-04 11:47 ` 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=7hmud4stfo.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 \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.