| Message ID | 1572868028-73076-1-git-send-email-jianxin.pan@amlogic.com |
|---|---|
| Headers | show |
| Series | arm64: meson: add support for A1 Power Domains | expand |
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
Jianxin Pan <jianxin.pan@amlogic.com> writes: > The Amlogic Meson A1/C1 Secure Monitor implements calls to control power > domain. > > Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com> > --- > drivers/firmware/meson/meson_sm.c | 2 ++ > include/linux/firmware/meson/meson_sm.h | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c > index 1d5b4d7..7ec09f5 100644 > --- a/drivers/firmware/meson/meson_sm.c > +++ b/drivers/firmware/meson/meson_sm.c > @@ -44,6 +44,8 @@ static const struct meson_sm_chip gxbb_chip = { > CMD(SM_EFUSE_WRITE, 0x82000031), > CMD(SM_EFUSE_USER_MAX, 0x82000033), > CMD(SM_GET_CHIP_ID, 0x82000044), > + CMD(SM_PWRC_SET, 0x82000093), > + CMD(SM_PWRC_GET, 0x82000095), > { /* sentinel */ }, > }, > }; > diff --git a/include/linux/firmware/meson/meson_sm.h b/include/linux/firmware/meson/meson_sm.h > index 6669e2a..4ed3989 100644 > --- a/include/linux/firmware/meson/meson_sm.h > +++ b/include/linux/firmware/meson/meson_sm.h > @@ -12,6 +12,8 @@ enum { > SM_EFUSE_WRITE, > SM_EFUSE_USER_MAX, > SM_GET_CHIP_ID, > + SM_PWRC_SET, > + SM_PWRC_GET, These new IDs are unique to the A1/C1 family. Maybe we should add a prefix to better indicate that. Maybe: SM_A1_PWRC_SET, SM_A1_PWRC_GET, Thoughts? Kevin
Hi Kevin, Please see my comments below: On 2019/11/10 4:11, Kevin Hilman wrote: > Jianxin Pan <jianxin.pan@amlogic.com> writes: > >> The Amlogic Meson A1/C1 Secure Monitor implements calls to control power >> domain. >> >> Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com> >> --- >> drivers/firmware/meson/meson_sm.c | 2 ++ >> include/linux/firmware/meson/meson_sm.h | 2 ++ >> 2 files changed, 4 insertions(+) >> [...] >> diff --git a/include/linux/firmware/meson/meson_sm.h b/include/linux/firmware/meson/meson_sm.h >> index 6669e2a..4ed3989 100644 >> --- a/include/linux/firmware/meson/meson_sm.h >> +++ b/include/linux/firmware/meson/meson_sm.h >> @@ -12,6 +12,8 @@ enum { >> SM_EFUSE_WRITE, >> SM_EFUSE_USER_MAX, >> SM_GET_CHIP_ID, >> + SM_PWRC_SET, >> + SM_PWRC_GET, > > These new IDs are unique to the A1/C1 family. Maybe we should add a > prefix to better indicate that. Maybe: > > SM_A1_PWRC_SET, > SM_A1_PWRC_GET, > > Thoughts? > I consulted with the internal VLSI team, and it's likely that the latter new SOC will follow A1/C1. And then it may become common function in the future. > Kevin > > . >
Hi Kevin, Thanks for the review, please see the comments below: 2019/11/10 4:09, 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> > > 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) > sts=1 means, the domain is powered off. I can rename it to is_off in the next version. now, only bit[0] is used in BL31, so I can use sts directly instead of !!sts. >> + 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. > sts here indicates if smc call is failed (such as due to inlvaid command id). I can rename it to ret in the next version. >> +} >> + >> +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. > OK, I will fix it. >> +} >> + >> +#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? > Yes, I will fix it, thank you. >> + 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? > NIC domain is for ARM CoreLink NIC-400 Network Interconnect, and should be always on since bootloader. >> + SEC_PD(NIC, GENPD_FLAG_ALWAYS_ON), >> + SEC_PD(PDMIN, 0), >> + SEC_PD(RSA, 0), >> +}; > > [...] > > Kevin > > . >
Jianxin Pan <jianxin.pan@amlogic.com> writes: > Hi Kevin, > > Please see my comments below: > > On 2019/11/10 4:11, Kevin Hilman wrote: >> Jianxin Pan <jianxin.pan@amlogic.com> writes: >> >>> The Amlogic Meson A1/C1 Secure Monitor implements calls to control power >>> domain. >>> >>> Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com> >>> --- >>> drivers/firmware/meson/meson_sm.c | 2 ++ >>> include/linux/firmware/meson/meson_sm.h | 2 ++ >>> 2 files changed, 4 insertions(+) >>> > [...] >>> diff --git a/include/linux/firmware/meson/meson_sm.h b/include/linux/firmware/meson/meson_sm.h >>> index 6669e2a..4ed3989 100644 >>> --- a/include/linux/firmware/meson/meson_sm.h >>> +++ b/include/linux/firmware/meson/meson_sm.h >>> @@ -12,6 +12,8 @@ enum { >>> SM_EFUSE_WRITE, >>> SM_EFUSE_USER_MAX, >>> SM_GET_CHIP_ID, >>> + SM_PWRC_SET, >>> + SM_PWRC_GET, >> >> These new IDs are unique to the A1/C1 family. Maybe we should add a >> prefix to better indicate that. Maybe: >> >> SM_A1_PWRC_SET, >> SM_A1_PWRC_GET, >> >> Thoughts? > > I consulted with the internal VLSI team, and it's likely that the latter new SOC will follow A1/C1. > And then it may become common function in the future. OK, but it's not a common function for the past, so it's useful to mark that distinction. Just like in device-tree, we often have compatibles named for previous SoC families (e.g. "gxbb") used on newer SoCs, but we use that to mean "GXBB or newer". Similarily here, we can use SM_A1_ prefix to mean "A1 or newer. Kevin
Hi Jianxin, Jianxin Pan <jianxin.pan@amlogic.com> writes: [...] >>> + 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? >> > NIC domain is for ARM CoreLink NIC-400 Network Interconnect, and should be always on since bootloader. OK, makes sense. I suggest a minor change to the comment to remind that this is an interconnect: /* NIC is for the Arm NIC-400 interconnect, and should be always on */ Thanks, Kevin
Hi Kevin, On 2019/11/11 22:40, Kevin Hilman wrote: > Jianxin Pan <jianxin.pan@amlogic.com> writes: > >> Hi Kevin, >> >> Please see my comments below: >> >> On 2019/11/10 4:11, Kevin Hilman wrote: >>> Jianxin Pan <jianxin.pan@amlogic.com> writes: >>> >>>> The Amlogic Meson A1/C1 Secure Monitor implements calls to control power >>>> domain. >>>> >>>> Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com> >>>> --- >>>> drivers/firmware/meson/meson_sm.c | 2 ++ >>>> include/linux/firmware/meson/meson_sm.h | 2 ++ >>>> 2 files changed, 4 insertions(+) >>>> >> [...] >>>> diff --git a/include/linux/firmware/meson/meson_sm.h b/include/linux/firmware/meson/meson_sm.h >>>> index 6669e2a..4ed3989 100644 >>>> --- a/include/linux/firmware/meson/meson_sm.h >>>> +++ b/include/linux/firmware/meson/meson_sm.h >>>> @@ -12,6 +12,8 @@ enum { >>>> SM_EFUSE_WRITE, >>>> SM_EFUSE_USER_MAX, >>>> SM_GET_CHIP_ID, >>>> + SM_PWRC_SET, >>>> + SM_PWRC_GET, >>> >>> These new IDs are unique to the A1/C1 family. Maybe we should add a >>> prefix to better indicate that. Maybe: >>> >>> SM_A1_PWRC_SET, >>> SM_A1_PWRC_GET, >>> >>> Thoughts? >> >> I consulted with the internal VLSI team, and it's likely that the latter new SOC will follow A1/C1. >> And then it may become common function in the future. > > OK, but it's not a common function for the past, so it's useful to mark > that distinction. > > Just like in device-tree, we often have compatibles named for previous > SoC families (e.g. "gxbb") used on newer SoCs, but we use that to mean > "GXBB or newer". > > Similarily here, we can use SM_A1_ prefix to mean "A1 or newer. > Thanks for your explaination, I will fix it in the next version. > Kevin > > . >
Hi Kevin, On 2019/11/11 22:44, Kevin Hilman wrote: > Hi Jianxin, > > Jianxin Pan <jianxin.pan@amlogic.com> writes: > > [...] > >>>> + 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? >>> >> NIC domain is for ARM CoreLink NIC-400 Network Interconnect, and should be always on since bootloader. > > OK, makes sense. I suggest a minor change to the comment to remind that > this is an interconnect: > > /* NIC is for the Arm NIC-400 interconnect, and should be always on */ > OK, I will update it, and thanks for the advice. > Thanks, > > Kevin > > . >