Re: [PATCH v2 02/12] soc: mediatek: Add MediaTek SCPSYS power domains
On Fri, Oct 30, 2020 at 6:30 PM Enric Balletbo i Serra
wrote:
>
> Hi Nicolas,
>
> On 28/10/20 2:13, Nicolas Boichat wrote:
> > On Wed, Oct 28, 2020 at 12:25 AM Enric Balletbo i Serra
> > wrote:
> >>
> >> Hi Nicolas,
> >>
> >> On 27/10/20 1:19, Nicolas Boichat wrote:
> >>> Hi Enric,
> >>>
> >>> On Mon, Oct 26, 2020 at 11:17 PM Enric Balletbo i Serra
> >>> wrote:
>
> Hi Nicolas,
>
> Many thanks for looking at this.
> >>>
> >>> Thanks to you ,-)
> >>>
> >>> [snip]
> >> + if (id >= scpsys->soc_data->num_domains) {
> >> + dev_err_probe(scpsys->dev, -EINVAL, "%pOFn: invalid
> >> domain id %d\n", node, id);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + domain_data = >soc_data->domains[id];
> >> + if (!domain_data) {
> >
> > Is that even possible at all? I mean, even if
> > scpsys->soc_data->domains is NULL, as long as id != 0, this will no
> > happen.
> >
>
> I think could happen with a bad DT definition. I.e if for the definition
> of the
> MT8173 domains you use a wrong value for the reg property, a value that
> is not
> present in the SoC data. It is unlikely if you use the defines but could
> happen
> if you hardcore the value. We cannot check this with the DT json-schema.
> >>>
> >>> I wasn't clear in my explanation, and looking further there is more
> >>> that looks wrong.
> >>>
> >>> This expression >soc_data->domains[id] is a pointer to element
> >>> "id" of the array domains. So if you convert to integer arithmetic,
> >>> it'll be something like `(long)scpsys->soc_data->domains +
> >>> (sizeof(struct generic_pm_domain *)) * id`. The only way this can be
> >>> NULL is if scpsys->soc_data->domains pointer is NULL, which, actually,
> >>> can't really happen as it's the 5th element of a struct scpsys
> >>> structure `(long)scpsys->soc_data + offset_of(domains, struct scpsys)
> >>> + (sizeof(struct generic_pm_domain *)) * id`.
> >>>
> >>> I think what you mean is either:
> >>> domain_data = >soc_data->domains[id];
> >>> if (!*domain_data)
> >>> [but then domain_data type should be `struct generic_pm_domain **`?
> >>
> >> I think you're confusing the field `struct generic_pm_domain
> >> *domains[]`from the
> >> `struct scpsys` with `const struct scpsys_domain_data *domains` from
> >> `struct
> >> scpsys_soc_data`. My bad they have the same name, I should probably rename
> >> the
> >> second one as domain_info or domain_data to avoid that confusion.
> >
> > Oh, okay, get it, thanks for clarifying, I got myself confused indeed ,-P
> >
> > But, still, part of my integer arithmetics still holds...
> >
> > >soc_data->domains[id] = (long)scpsys->soc_data->domains +
> > (sizeof(struct generic_pm_domain *)) * id. The only way domain_data
> > can be NULL is if scpsys->soc_data->domains pointer is NULL (it can't
> > be, really, assuming scpsys_soc_data structures are well defined) AND
> > id is 0.
> >
> > Now, if I understand what you want to check here. If a domain id is
> > not specified in scpsys_domain_data (e.g. if there is a gap in
> > MT8XXX_POWER_DOMAIN_YYY indices and if `id` points at one of those
> > gaps), you'll get an all-zero entry in domain_data. So maybe you can
> > just check that domain_data->sta_mask != 0? Would that be enough? (I
> > expect that sta_mask would always need to be set?)
> >
>
> Yes, that would be enough. I'll change for the next version.
>
> > But then again, are there ever gaps in MT8XXX_POWER_DOMAIN_YYY indices?
> >
>
> AFAIK, there is no gaps, but one could make gaps when filling that info. I
> still think is worth have this check although is "unlikely" to happen due an
> human error :-). I'll maintain for the next version, but I don't really care
> to
> remove it if all you prefer I remove it.
I'm fine with the sta_mask check. Thanks!
>
> Thanks,
> Enric
>
>
> >>
> >>
> >> diff --git a/drivers/soc/mediatek/mtk-pm-domains.h
> >> b/drivers/soc/mediatek/mtk-pm-domains.h
> >> index 7c8efcb3cef2..6ff095db8a27 100644
> >> --- a/drivers/soc/mediatek/mtk-pm-domains.h
> >> +++ b/drivers/soc/mediatek/mtk-pm-domains.h
> >> @@ -56,7 +56,7 @@ struct scpsys_domain_data {
> >> };
> >>
> >> struct scpsys_soc_data {
> >> - const struct scpsys_domain_data *domains;
> >> + const struct scpsys_domain_data *domain_data;
> >> int num_domains;
> >> int pwr_sta_offs;
> >> int pwr_sta2nd_offs;
> >>
> >> ---
> >>
> >> struct scpsys {
> >> ...
> >> const struct scpsys_soc_data *soc_data;
> >> ...
> >> struct generic_pm_domain *domains[];
> >> }
> >>
> >>
> >> domain_data = >soc_data->domain_data[id];
> >> if (!domain_data)
> >>
> >> Thanks,
> >> Enric
> >>
> >>
> >>> Does your code compile with warnings enabled?]
> >>> or:
> >>> domain_data = scpsys->soc_data->domains[id];
> >>> if (!domain_data)
> >>> [then the test makes sense]
> >>>
> >>> [snip]
> >>>
Re: [PATCH v2 02/12] soc: mediatek: Add MediaTek SCPSYS power domains
Hi Nicolas,
On 28/10/20 2:13, Nicolas Boichat wrote:
> On Wed, Oct 28, 2020 at 12:25 AM Enric Balletbo i Serra
> wrote:
>>
>> Hi Nicolas,
>>
>> On 27/10/20 1:19, Nicolas Boichat wrote:
>>> Hi Enric,
>>>
>>> On Mon, Oct 26, 2020 at 11:17 PM Enric Balletbo i Serra
>>> wrote:
Hi Nicolas,
Many thanks for looking at this.
>>>
>>> Thanks to you ,-)
>>>
>>> [snip]
>> + if (id >= scpsys->soc_data->num_domains) {
>> + dev_err_probe(scpsys->dev, -EINVAL, "%pOFn: invalid
>> domain id %d\n", node, id);
>> + return -EINVAL;
>> + }
>> +
>> + domain_data = >soc_data->domains[id];
>> + if (!domain_data) {
>
> Is that even possible at all? I mean, even if
> scpsys->soc_data->domains is NULL, as long as id != 0, this will no
> happen.
>
I think could happen with a bad DT definition. I.e if for the definition
of the
MT8173 domains you use a wrong value for the reg property, a value that is
not
present in the SoC data. It is unlikely if you use the defines but could
happen
if you hardcore the value. We cannot check this with the DT json-schema.
>>>
>>> I wasn't clear in my explanation, and looking further there is more
>>> that looks wrong.
>>>
>>> This expression >soc_data->domains[id] is a pointer to element
>>> "id" of the array domains. So if you convert to integer arithmetic,
>>> it'll be something like `(long)scpsys->soc_data->domains +
>>> (sizeof(struct generic_pm_domain *)) * id`. The only way this can be
>>> NULL is if scpsys->soc_data->domains pointer is NULL, which, actually,
>>> can't really happen as it's the 5th element of a struct scpsys
>>> structure `(long)scpsys->soc_data + offset_of(domains, struct scpsys)
>>> + (sizeof(struct generic_pm_domain *)) * id`.
>>>
>>> I think what you mean is either:
>>> domain_data = >soc_data->domains[id];
>>> if (!*domain_data)
>>> [but then domain_data type should be `struct generic_pm_domain **`?
>>
>> I think you're confusing the field `struct generic_pm_domain *domains[]`from
>> the
>> `struct scpsys` with `const struct scpsys_domain_data *domains` from `struct
>> scpsys_soc_data`. My bad they have the same name, I should probably rename
>> the
>> second one as domain_info or domain_data to avoid that confusion.
>
> Oh, okay, get it, thanks for clarifying, I got myself confused indeed ,-P
>
> But, still, part of my integer arithmetics still holds...
>
> >soc_data->domains[id] = (long)scpsys->soc_data->domains +
> (sizeof(struct generic_pm_domain *)) * id. The only way domain_data
> can be NULL is if scpsys->soc_data->domains pointer is NULL (it can't
> be, really, assuming scpsys_soc_data structures are well defined) AND
> id is 0.
>
> Now, if I understand what you want to check here. If a domain id is
> not specified in scpsys_domain_data (e.g. if there is a gap in
> MT8XXX_POWER_DOMAIN_YYY indices and if `id` points at one of those
> gaps), you'll get an all-zero entry in domain_data. So maybe you can
> just check that domain_data->sta_mask != 0? Would that be enough? (I
> expect that sta_mask would always need to be set?)
>
Yes, that would be enough. I'll change for the next version.
> But then again, are there ever gaps in MT8XXX_POWER_DOMAIN_YYY indices?
>
AFAIK, there is no gaps, but one could make gaps when filling that info. I
still think is worth have this check although is "unlikely" to happen due an
human error :-). I'll maintain for the next version, but I don't really care to
remove it if all you prefer I remove it.
Thanks,
Enric
>>
>>
>> diff --git a/drivers/soc/mediatek/mtk-pm-domains.h
>> b/drivers/soc/mediatek/mtk-pm-domains.h
>> index 7c8efcb3cef2..6ff095db8a27 100644
>> --- a/drivers/soc/mediatek/mtk-pm-domains.h
>> +++ b/drivers/soc/mediatek/mtk-pm-domains.h
>> @@ -56,7 +56,7 @@ struct scpsys_domain_data {
>> };
>>
>> struct scpsys_soc_data {
>> - const struct scpsys_domain_data *domains;
>> + const struct scpsys_domain_data *domain_data;
>> int num_domains;
>> int pwr_sta_offs;
>> int pwr_sta2nd_offs;
>>
>> ---
>>
>> struct scpsys {
>> ...
>> const struct scpsys_soc_data *soc_data;
>> ...
>> struct generic_pm_domain *domains[];
>> }
>>
>>
>> domain_data = >soc_data->domain_data[id];
>> if (!domain_data)
>>
>> Thanks,
>> Enric
>>
>>
>>> Does your code compile with warnings enabled?]
>>> or:
>>> domain_data = scpsys->soc_data->domains[id];
>>> if (!domain_data)
>>> [then the test makes sense]
>>>
>>> [snip]
>>>
Re: [PATCH v2 02/12] soc: mediatek: Add MediaTek SCPSYS power domains
On Wed, Oct 28, 2020 at 12:25 AM Enric Balletbo i Serra
wrote:
>
> Hi Nicolas,
>
> On 27/10/20 1:19, Nicolas Boichat wrote:
> > Hi Enric,
> >
> > On Mon, Oct 26, 2020 at 11:17 PM Enric Balletbo i Serra
> > wrote:
> >>
> >> Hi Nicolas,
> >>
> >> Many thanks for looking at this.
> >
> > Thanks to you ,-)
> >
> > [snip]
> + if (id >= scpsys->soc_data->num_domains) {
> + dev_err_probe(scpsys->dev, -EINVAL, "%pOFn: invalid
> domain id %d\n", node, id);
> + return -EINVAL;
> + }
> +
> + domain_data = >soc_data->domains[id];
> + if (!domain_data) {
> >>>
> >>> Is that even possible at all? I mean, even if
> >>> scpsys->soc_data->domains is NULL, as long as id != 0, this will no
> >>> happen.
> >>>
> >>
> >> I think could happen with a bad DT definition. I.e if for the definition
> >> of the
> >> MT8173 domains you use a wrong value for the reg property, a value that is
> >> not
> >> present in the SoC data. It is unlikely if you use the defines but could
> >> happen
> >> if you hardcore the value. We cannot check this with the DT json-schema.
> >
> > I wasn't clear in my explanation, and looking further there is more
> > that looks wrong.
> >
> > This expression >soc_data->domains[id] is a pointer to element
> > "id" of the array domains. So if you convert to integer arithmetic,
> > it'll be something like `(long)scpsys->soc_data->domains +
> > (sizeof(struct generic_pm_domain *)) * id`. The only way this can be
> > NULL is if scpsys->soc_data->domains pointer is NULL, which, actually,
> > can't really happen as it's the 5th element of a struct scpsys
> > structure `(long)scpsys->soc_data + offset_of(domains, struct scpsys)
> > + (sizeof(struct generic_pm_domain *)) * id`.
> >
> > I think what you mean is either:
> > domain_data = >soc_data->domains[id];
> > if (!*domain_data)
> > [but then domain_data type should be `struct generic_pm_domain **`?
>
> I think you're confusing the field `struct generic_pm_domain *domains[]`from
> the
> `struct scpsys` with `const struct scpsys_domain_data *domains` from `struct
> scpsys_soc_data`. My bad they have the same name, I should probably rename the
> second one as domain_info or domain_data to avoid that confusion.
Oh, okay, get it, thanks for clarifying, I got myself confused indeed ,-P
But, still, part of my integer arithmetics still holds...
>soc_data->domains[id] = (long)scpsys->soc_data->domains +
(sizeof(struct generic_pm_domain *)) * id. The only way domain_data
can be NULL is if scpsys->soc_data->domains pointer is NULL (it can't
be, really, assuming scpsys_soc_data structures are well defined) AND
id is 0.
Now, if I understand what you want to check here. If a domain id is
not specified in scpsys_domain_data (e.g. if there is a gap in
MT8XXX_POWER_DOMAIN_YYY indices and if `id` points at one of those
gaps), you'll get an all-zero entry in domain_data. So maybe you can
just check that domain_data->sta_mask != 0? Would that be enough? (I
expect that sta_mask would always need to be set?)
But then again, are there ever gaps in MT8XXX_POWER_DOMAIN_YYY indices?
>
>
> diff --git a/drivers/soc/mediatek/mtk-pm-domains.h
> b/drivers/soc/mediatek/mtk-pm-domains.h
> index 7c8efcb3cef2..6ff095db8a27 100644
> --- a/drivers/soc/mediatek/mtk-pm-domains.h
> +++ b/drivers/soc/mediatek/mtk-pm-domains.h
> @@ -56,7 +56,7 @@ struct scpsys_domain_data {
> };
>
> struct scpsys_soc_data {
> - const struct scpsys_domain_data *domains;
> + const struct scpsys_domain_data *domain_data;
> int num_domains;
> int pwr_sta_offs;
> int pwr_sta2nd_offs;
>
> ---
>
> struct scpsys {
> ...
> const struct scpsys_soc_data *soc_data;
> ...
> struct generic_pm_domain *domains[];
> }
>
>
> domain_data = >soc_data->domain_data[id];
> if (!domain_data)
>
> Thanks,
> Enric
>
>
> > Does your code compile with warnings enabled?]
> > or:
> > domain_data = scpsys->soc_data->domains[id];
> > if (!domain_data)
> > [then the test makes sense]
> >
> > [snip]
> >
Re: [PATCH v2 02/12] soc: mediatek: Add MediaTek SCPSYS power domains
Hi Nicolas,
On 27/10/20 1:19, Nicolas Boichat wrote:
> Hi Enric,
>
> On Mon, Oct 26, 2020 at 11:17 PM Enric Balletbo i Serra
> wrote:
>>
>> Hi Nicolas,
>>
>> Many thanks for looking at this.
>
> Thanks to you ,-)
>
> [snip]
+ if (id >= scpsys->soc_data->num_domains) {
+ dev_err_probe(scpsys->dev, -EINVAL, "%pOFn: invalid domain
id %d\n", node, id);
+ return -EINVAL;
+ }
+
+ domain_data = >soc_data->domains[id];
+ if (!domain_data) {
>>>
>>> Is that even possible at all? I mean, even if
>>> scpsys->soc_data->domains is NULL, as long as id != 0, this will no
>>> happen.
>>>
>>
>> I think could happen with a bad DT definition. I.e if for the definition of
>> the
>> MT8173 domains you use a wrong value for the reg property, a value that is
>> not
>> present in the SoC data. It is unlikely if you use the defines but could
>> happen
>> if you hardcore the value. We cannot check this with the DT json-schema.
>
> I wasn't clear in my explanation, and looking further there is more
> that looks wrong.
>
> This expression >soc_data->domains[id] is a pointer to element
> "id" of the array domains. So if you convert to integer arithmetic,
> it'll be something like `(long)scpsys->soc_data->domains +
> (sizeof(struct generic_pm_domain *)) * id`. The only way this can be
> NULL is if scpsys->soc_data->domains pointer is NULL, which, actually,
> can't really happen as it's the 5th element of a struct scpsys
> structure `(long)scpsys->soc_data + offset_of(domains, struct scpsys)
> + (sizeof(struct generic_pm_domain *)) * id`.
>
> I think what you mean is either:
> domain_data = >soc_data->domains[id];
> if (!*domain_data)
> [but then domain_data type should be `struct generic_pm_domain **`?
I think you're confusing the field `struct generic_pm_domain *domains[]`from the
`struct scpsys` with `const struct scpsys_domain_data *domains` from `struct
scpsys_soc_data`. My bad they have the same name, I should probably rename the
second one as domain_info or domain_data to avoid that confusion.
diff --git a/drivers/soc/mediatek/mtk-pm-domains.h
b/drivers/soc/mediatek/mtk-pm-domains.h
index 7c8efcb3cef2..6ff095db8a27 100644
--- a/drivers/soc/mediatek/mtk-pm-domains.h
+++ b/drivers/soc/mediatek/mtk-pm-domains.h
@@ -56,7 +56,7 @@ struct scpsys_domain_data {
};
struct scpsys_soc_data {
- const struct scpsys_domain_data *domains;
+ const struct scpsys_domain_data *domain_data;
int num_domains;
int pwr_sta_offs;
int pwr_sta2nd_offs;
---
struct scpsys {
...
const struct scpsys_soc_data *soc_data;
...
struct generic_pm_domain *domains[];
}
domain_data = >soc_data->domain_data[id];
if (!domain_data)
Thanks,
Enric
> Does your code compile with warnings enabled?]
> or:
> domain_data = scpsys->soc_data->domains[id];
> if (!domain_data)
> [then the test makes sense]
>
> [snip]
>
Re: [PATCH v2 02/12] soc: mediatek: Add MediaTek SCPSYS power domains
Hi Enric,
On Mon, Oct 26, 2020 at 11:17 PM Enric Balletbo i Serra
wrote:
>
> Hi Nicolas,
>
> Many thanks for looking at this.
Thanks to you ,-)
[snip]
> >> + if (id >= scpsys->soc_data->num_domains) {
> >> + dev_err_probe(scpsys->dev, -EINVAL, "%pOFn: invalid domain
> >> id %d\n", node, id);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + domain_data = >soc_data->domains[id];
> >> + if (!domain_data) {
> >
> > Is that even possible at all? I mean, even if
> > scpsys->soc_data->domains is NULL, as long as id != 0, this will no
> > happen.
> >
>
> I think could happen with a bad DT definition. I.e if for the definition of
> the
> MT8173 domains you use a wrong value for the reg property, a value that is not
> present in the SoC data. It is unlikely if you use the defines but could
> happen
> if you hardcore the value. We cannot check this with the DT json-schema.
I wasn't clear in my explanation, and looking further there is more
that looks wrong.
This expression >soc_data->domains[id] is a pointer to element
"id" of the array domains. So if you convert to integer arithmetic,
it'll be something like `(long)scpsys->soc_data->domains +
(sizeof(struct generic_pm_domain *)) * id`. The only way this can be
NULL is if scpsys->soc_data->domains pointer is NULL, which, actually,
can't really happen as it's the 5th element of a struct scpsys
structure `(long)scpsys->soc_data + offset_of(domains, struct scpsys)
+ (sizeof(struct generic_pm_domain *)) * id`.
I think what you mean is either:
domain_data = >soc_data->domains[id];
if (!*domain_data)
[but then domain_data type should be `struct generic_pm_domain **`?
Does your code compile with warnings enabled?]
or:
domain_data = scpsys->soc_data->domains[id];
if (!domain_data)
[then the test makes sense]
[snip]
Re: [PATCH v2 02/12] soc: mediatek: Add MediaTek SCPSYS power domains
Hi Nicolas,
Many thanks for looking at this.
On 5/10/20 3:39, Nicolas Boichat wrote:
> On Fri, Oct 2, 2020 at 12:02 AM Enric Balletbo i Serra
> wrote:
>>
>> The System Control Processor System (SCPSYS) has several power management
>> related tasks in the system. This driver implements support to handle
>> the different power domains supported in order to meet high performance
>> and low power requirements.
>>
>> Co-developed-by: Matthias Brugger
>> Signed-off-by: Matthias Brugger
>> Signed-off-by: Enric Balletbo i Serra
>> ---
>>
>> Changes in v2:
>> - Get base address from parent syscon. We have now a scpsys syscon node
>> and a child for the SPM (System Power Manager).
>> - Use regmap API to acces de base address.
>>
>> drivers/soc/mediatek/Kconfig | 13 +
>> drivers/soc/mediatek/Makefile | 1 +
>> drivers/soc/mediatek/mtk-pm-domains.c | 628 ++
>> 3 files changed, 642 insertions(+)
>> create mode 100644 drivers/soc/mediatek/mtk-pm-domains.c
>>
>> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
>> index 59a56cd790ec..68d800f9e4a5 100644
>> --- a/drivers/soc/mediatek/Kconfig
>> +++ b/drivers/soc/mediatek/Kconfig
>> @@ -44,6 +44,19 @@ config MTK_SCPSYS
>> Say yes here to add support for the MediaTek SCPSYS power domain
>> driver.
>>
>> +config MTK_SCPSYS_PM_DOMAINS
>> + bool "MediaTek SCPSYS generic power domain"
>> + default ARCH_MEDIATEK
>> + depends on PM
>> + depends on MTK_INFRACFG
>> + select PM_GENERIC_DOMAINS
>> + select REGMAP
>> + help
>> + Say y here to enable power domain support.
>> + In order to meet high performance and low power requirements, the
>> System
>> + Control Processor System (SCPSYS) has several power management
>> related
>> + tasks in the system.
>> +
>> config MTK_MMSYS
>> bool "MediaTek MMSYS Support"
>> default ARCH_MEDIATEK
>> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
>> index 01f9f873634a..1e60fb4f89d4 100644
>> --- a/drivers/soc/mediatek/Makefile
>> +++ b/drivers/soc/mediatek/Makefile
>> @@ -3,4 +3,5 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
>> obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
>> obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
>> obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
>> +obj-$(CONFIG_MTK_SCPSYS_PM_DOMAINS) += mtk-pm-domains.o
>> obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o
>> diff --git a/drivers/soc/mediatek/mtk-pm-domains.c
>> b/drivers/soc/mediatek/mtk-pm-domains.c
>> new file mode 100644
>> index ..68886bf437f9
>> --- /dev/null
>> +++ b/drivers/soc/mediatek/mtk-pm-domains.c
>> @@ -0,0 +1,628 @@
>> [snip]
>> +
>> +static int scpsys_domain_is_on(struct scpsys_domain *pd)
>> +{
>> + struct scpsys *scpsys = pd->scpsys;
>> + u32 status, status2;
>> +
>> + regmap_read(scpsys->base, scpsys->soc_data->pwr_sta_offs, );
>> + status &= pd->data->sta_mask;
>> +
>> + regmap_read(scpsys->base, scpsys->soc_data->pwr_sta2nd_offs,
>> );
>> + status2 &= pd->data->sta_mask;
>> +
>> + /*
>> +* A domain is on when both status bits are set. If only one is set
>> +* return an error. This happens while powering up a domain
>> +*/
>> +
>> + if (status && status2)
>> + return true;
>> + if (!status && !status2)
>> + return false;
>> +
>> + return -EINVAL;
>
>
> Why do you care? It seems like all you ever do with this is check if
> the domain is powered on, so it seems like you could just
> return (status && status2); ?
>
Changed in v3
>> +}
>> +
>> +static int scpsys_sram_enable(struct scpsys_domain *pd)
>> +{
>> + u32 pdn_ack = pd->data->sram_pdn_ack_bits;
>> + struct scpsys *scpsys = pd->scpsys;
>> + u32 val;
>> + int tmp;
>
> regmap_read_poll_timeout => regmap_read technically takes a `unsigned
> int *` parameter, so this should probably be unsigned too (or... just
> reuse val?)
>
Changed in v3
>> +
>> + regmap_read(scpsys->base, pd->data->ctl_offs, );
>> + val &= ~pd->data->sram_pdn_bits;
>> + regmap_write(scpsys->base, pd->data->ctl_offs, val);
>
> Replace with regmap_update_bits?
>
Right, I'll replace here and in the other places where is possible.
>> +
>> + /* Either wait until SRAM_PDN_ACK all 1 or 0 */
>> + return regmap_read_poll_timeout(scpsys->base, pd->data->ctl_offs,
>> tmp,
>> + (tmp & pdn_ack) == 0,
>> MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>> +}
>> +
>> +static int scpsys_sram_disable(struct scpsys_domain *pd)
>> +{
>> + u32 pdn_ack = pd->data->sram_pdn_ack_bits;
>> + struct scpsys *scpsys = pd->scpsys;
>> + u32 val;
>> + int tmp;
>> +
>> + regmap_read(scpsys->base, pd->data->ctl_offs, );
>> + val |= pd->data->sram_pdn_bits;
>> + regmap_write(scpsys->base,
Re: [PATCH v2 02/12] soc: mediatek: Add MediaTek SCPSYS power domains
On Fri, Oct 2, 2020 at 12:02 AM Enric Balletbo i Serra
wrote:
>
> The System Control Processor System (SCPSYS) has several power management
> related tasks in the system. This driver implements support to handle
> the different power domains supported in order to meet high performance
> and low power requirements.
>
> Co-developed-by: Matthias Brugger
> Signed-off-by: Matthias Brugger
> Signed-off-by: Enric Balletbo i Serra
> ---
>
> Changes in v2:
> - Get base address from parent syscon. We have now a scpsys syscon node
> and a child for the SPM (System Power Manager).
> - Use regmap API to acces de base address.
>
> drivers/soc/mediatek/Kconfig | 13 +
> drivers/soc/mediatek/Makefile | 1 +
> drivers/soc/mediatek/mtk-pm-domains.c | 628 ++
> 3 files changed, 642 insertions(+)
> create mode 100644 drivers/soc/mediatek/mtk-pm-domains.c
>
> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> index 59a56cd790ec..68d800f9e4a5 100644
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> @@ -44,6 +44,19 @@ config MTK_SCPSYS
> Say yes here to add support for the MediaTek SCPSYS power domain
> driver.
>
> +config MTK_SCPSYS_PM_DOMAINS
> + bool "MediaTek SCPSYS generic power domain"
> + default ARCH_MEDIATEK
> + depends on PM
> + depends on MTK_INFRACFG
> + select PM_GENERIC_DOMAINS
> + select REGMAP
> + help
> + Say y here to enable power domain support.
> + In order to meet high performance and low power requirements, the
> System
> + Control Processor System (SCPSYS) has several power management
> related
> + tasks in the system.
> +
> config MTK_MMSYS
> bool "MediaTek MMSYS Support"
> default ARCH_MEDIATEK
> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> index 01f9f873634a..1e60fb4f89d4 100644
> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile
> @@ -3,4 +3,5 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
> obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> +obj-$(CONFIG_MTK_SCPSYS_PM_DOMAINS) += mtk-pm-domains.o
> obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o
> diff --git a/drivers/soc/mediatek/mtk-pm-domains.c
> b/drivers/soc/mediatek/mtk-pm-domains.c
> new file mode 100644
> index ..68886bf437f9
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-pm-domains.c
> @@ -0,0 +1,628 @@
> [snip]
> +
> +static int scpsys_domain_is_on(struct scpsys_domain *pd)
> +{
> + struct scpsys *scpsys = pd->scpsys;
> + u32 status, status2;
> +
> + regmap_read(scpsys->base, scpsys->soc_data->pwr_sta_offs, );
> + status &= pd->data->sta_mask;
> +
> + regmap_read(scpsys->base, scpsys->soc_data->pwr_sta2nd_offs,
> );
> + status2 &= pd->data->sta_mask;
> +
> + /*
> +* A domain is on when both status bits are set. If only one is set
> +* return an error. This happens while powering up a domain
> +*/
> +
> + if (status && status2)
> + return true;
> + if (!status && !status2)
> + return false;
> +
> + return -EINVAL;
Why do you care? It seems like all you ever do with this is check if
the domain is powered on, so it seems like you could just
return (status && status2); ?
> +}
> +
> +static int scpsys_sram_enable(struct scpsys_domain *pd)
> +{
> + u32 pdn_ack = pd->data->sram_pdn_ack_bits;
> + struct scpsys *scpsys = pd->scpsys;
> + u32 val;
> + int tmp;
regmap_read_poll_timeout => regmap_read technically takes a `unsigned
int *` parameter, so this should probably be unsigned too (or... just
reuse val?)
> +
> + regmap_read(scpsys->base, pd->data->ctl_offs, );
> + val &= ~pd->data->sram_pdn_bits;
> + regmap_write(scpsys->base, pd->data->ctl_offs, val);
Replace with regmap_update_bits?
> +
> + /* Either wait until SRAM_PDN_ACK all 1 or 0 */
> + return regmap_read_poll_timeout(scpsys->base, pd->data->ctl_offs, tmp,
> + (tmp & pdn_ack) == 0,
> MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> +}
> +
> +static int scpsys_sram_disable(struct scpsys_domain *pd)
> +{
> + u32 pdn_ack = pd->data->sram_pdn_ack_bits;
> + struct scpsys *scpsys = pd->scpsys;
> + u32 val;
> + int tmp;
> +
> + regmap_read(scpsys->base, pd->data->ctl_offs, );
> + val |= pd->data->sram_pdn_bits;
> + regmap_write(scpsys->base, pd->data->ctl_offs, val);
> +
> + /* Either wait until SRAM_PDN_ACK all 1 or 0 */
> + return regmap_read_poll_timeout(scpsys->base, pd->data->ctl_offs, tmp,
> + (tmp & pdn_ack) == pdn_ack,
> MTK_POLL_DELAY_US,
> + MTK_POLL_TIMEOUT);
> +}
> +
> +static int
[PATCH v2 02/12] soc: mediatek: Add MediaTek SCPSYS power domains
The System Control Processor System (SCPSYS) has several power management
related tasks in the system. This driver implements support to handle
the different power domains supported in order to meet high performance
and low power requirements.
Co-developed-by: Matthias Brugger
Signed-off-by: Matthias Brugger
Signed-off-by: Enric Balletbo i Serra
---
Changes in v2:
- Get base address from parent syscon. We have now a scpsys syscon node
and a child for the SPM (System Power Manager).
- Use regmap API to acces de base address.
drivers/soc/mediatek/Kconfig | 13 +
drivers/soc/mediatek/Makefile | 1 +
drivers/soc/mediatek/mtk-pm-domains.c | 628 ++
3 files changed, 642 insertions(+)
create mode 100644 drivers/soc/mediatek/mtk-pm-domains.c
diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
index 59a56cd790ec..68d800f9e4a5 100644
--- a/drivers/soc/mediatek/Kconfig
+++ b/drivers/soc/mediatek/Kconfig
@@ -44,6 +44,19 @@ config MTK_SCPSYS
Say yes here to add support for the MediaTek SCPSYS power domain
driver.
+config MTK_SCPSYS_PM_DOMAINS
+ bool "MediaTek SCPSYS generic power domain"
+ default ARCH_MEDIATEK
+ depends on PM
+ depends on MTK_INFRACFG
+ select PM_GENERIC_DOMAINS
+ select REGMAP
+ help
+ Say y here to enable power domain support.
+ In order to meet high performance and low power requirements, the
System
+ Control Processor System (SCPSYS) has several power management related
+ tasks in the system.
+
config MTK_MMSYS
bool "MediaTek MMSYS Support"
default ARCH_MEDIATEK
diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
index 01f9f873634a..1e60fb4f89d4 100644
--- a/drivers/soc/mediatek/Makefile
+++ b/drivers/soc/mediatek/Makefile
@@ -3,4 +3,5 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
+obj-$(CONFIG_MTK_SCPSYS_PM_DOMAINS) += mtk-pm-domains.o
obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o
diff --git a/drivers/soc/mediatek/mtk-pm-domains.c
b/drivers/soc/mediatek/mtk-pm-domains.c
new file mode 100644
index ..68886bf437f9
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-pm-domains.c
@@ -0,0 +1,628 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2020 Collabora Ltd.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#define MTK_POLL_DELAY_US 10
+#define MTK_POLL_TIMEOUTUSEC_PER_SEC
+
+#define MTK_SCPD_ACTIVE_WAKEUP BIT(0)
+#define MTK_SCPD_FWAIT_SRAMBIT(1)
+#define MTK_SCPD_CAPS(_scpd, _x) ((_scpd)->data->caps & (_x))
+
+#define SPM_VDE_PWR_CON0x0210
+#define SPM_MFG_PWR_CON0x0214
+#define SPM_VEN_PWR_CON0x0230
+#define SPM_ISP_PWR_CON0x0238
+#define SPM_DIS_PWR_CON0x023c
+#define SPM_VEN2_PWR_CON 0x0298
+#define SPM_AUDIO_PWR_CON 0x029c
+#define SPM_MFG_2D_PWR_CON 0x02c0
+#define SPM_MFG_ASYNC_PWR_CON 0x02c4
+#define SPM_USB_PWR_CON0x02cc
+
+#define SPM_PWR_STATUS 0x060c
+#define SPM_PWR_STATUS_2ND 0x0610
+
+#define PWR_RST_B_BIT BIT(0)
+#define PWR_ISO_BITBIT(1)
+#define PWR_ON_BIT BIT(2)
+#define PWR_ON_2ND_BIT BIT(3)
+#define PWR_CLK_DIS_BITBIT(4)
+
+#define PWR_STATUS_DISPBIT(3)
+#define PWR_STATUS_MFG BIT(4)
+#define PWR_STATUS_ISP BIT(5)
+#define PWR_STATUS_VDECBIT(7)
+#define PWR_STATUS_VENC_LT BIT(20)
+#define PWR_STATUS_VENCBIT(21)
+#define PWR_STATUS_MFG_2D BIT(22)
+#define PWR_STATUS_MFG_ASYNC BIT(23)
+#define PWR_STATUS_AUDIO BIT(24)
+#define PWR_STATUS_USB BIT(25)
+
+struct scpsys_bus_prot_data {
+ u32 bus_prot_mask;
+ bool bus_prot_reg_update;
+};
+
+/**
+ * struct scpsys_domain_data - scp domain data for power on/off flow
+ * @sta_mask: The mask for power on/off status bit.
+ * @ctl_offs: The offset for main power control register.
+ * @sram_pdn_bits: The mask for sram power control bits.
+ * @sram_pdn_ack_bits: The mask for sram power control acked bits.
+ * @caps: The flag for active wake-up action.
+ * @bp_infracfg: bus protection for infracfg subsystem
+ */
+struct scpsys_domain_data {
+ u32 sta_mask;
+ int ctl_offs;
+ u32 sram_pdn_bits;
+ u32 sram_pdn_ack_bits;
+ u8 caps;
+ const struct scpsys_bus_prot_data bp_infracfg;
+};
+
+struct scpsys_domain {
+ struct
