Messages in this thread | | | From | Nicolas Boichat <> | Date | Wed, 28 Oct 2020 09:13:23 +0800 | Subject | 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 <enric.balletbo@collabora.com> 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@collabora.com> 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?)
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 = &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] > >
| |