Re: [PATCH v2 02/12] soc: mediatek: Add MediaTek SCPSYS power domains

[Nicolas Boichat]

On Fri, Oct 30, 2020 at 6:30 PM Enric Balletbo i Serra
<enric.balletbo@xxxxxxxxxxxxx> 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
> > <enric.balletbo@xxxxxxxxxxxxx> 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
> >>> <enric.balletbo@xxxxxxxxxxxxx> 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 = &scpsys->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 &scpsys->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 = &scpsys->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...
> >
> > &scpsys->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 = &scpsys->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]
> >>>