| Message ID | 1638871712-18636-3-git-send-email-quic_rjendra@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | soc: qcom: rpmhpd: Cleanups and mx/cx fixup for sc7280 | expand |
On Tue 07 Dec 04:08 CST 2021, Rajendra Nayak wrote: > While the requirement to specify the active + sleep and active-only MX > power-domains as the parents of the corresponding CX power domains is > applicable for most SoCs, we have some like the sc7280 where this > dependency is not applicable. > Define new rpmhpd structs for cx and cx_ao without the mx as > parent and use them for sc7280. > > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> > --- > drivers/soc/qcom/rpmhpd.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c > index c71481d..4599efe 100644 > --- a/drivers/soc/qcom/rpmhpd.c > +++ b/drivers/soc/qcom/rpmhpd.c > @@ -120,6 +120,20 @@ static struct rpmhpd cx_ao = { > .res_name = "cx.lvl", > }; > > +static struct rpmhpd cx_ao_no_parent; > +static struct rpmhpd cx_no_parent = { There are multiple variations of how each of these can be parented, but only one way they can be without a parent. So how about we turn this the other way around? I.e. let's name this one "cx" and the existing one "cx_w_mx_parent". This will be particularly useful when you look at mmcx, which on 8150/8180 has mx as parent and on 8450 has cx as parent. PS. Unfortunately I had merged 8450 since you wrote this series, I tried to just fix it up as I applied your patch, but noticed 8450_cx and 8450_mmcx and wanted to get your opinion on this first. Regards, Bjorn > + .pd = { .name = "cx", }, > + .peer = &cx_ao_no_parent, > + .res_name = "cx.lvl", > +}; > + > +static struct rpmhpd cx_ao_no_parent = { > + .pd = { .name = "cx_ao", }, > + .active_only = true, > + .peer = &cx_no_parent, > + .res_name = "cx.lvl", > +}; > + > static struct rpmhpd mmcx_ao; > static struct rpmhpd mmcx = { > .pd = { .name = "mmcx", }, > @@ -273,8 +287,8 @@ static const struct rpmhpd_desc sc7180_desc = { > > /* SC7280 RPMH powerdomains */ > static struct rpmhpd *sc7280_rpmhpds[] = { > - [SC7280_CX] = &cx, > - [SC7280_CX_AO] = &cx_ao, > + [SC7280_CX] = &cx_no_parent, > + [SC7280_CX_AO] = &cx_ao_no_parent, > [SC7280_EBI] = &ebi, > [SC7280_GFX] = &gfx, > [SC7280_MX] = &mx, > -- > 2.7.4 >
On 12/9/2021 2:12 AM, Bjorn Andersson wrote: > On Tue 07 Dec 04:08 CST 2021, Rajendra Nayak wrote: > >> While the requirement to specify the active + sleep and active-only MX >> power-domains as the parents of the corresponding CX power domains is >> applicable for most SoCs, we have some like the sc7280 where this >> dependency is not applicable. >> Define new rpmhpd structs for cx and cx_ao without the mx as >> parent and use them for sc7280. >> >> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> >> --- >> drivers/soc/qcom/rpmhpd.c | 18 ++++++++++++++++-- >> 1 file changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c >> index c71481d..4599efe 100644 >> --- a/drivers/soc/qcom/rpmhpd.c >> +++ b/drivers/soc/qcom/rpmhpd.c >> @@ -120,6 +120,20 @@ static struct rpmhpd cx_ao = { >> .res_name = "cx.lvl", >> }; >> >> +static struct rpmhpd cx_ao_no_parent; >> +static struct rpmhpd cx_no_parent = { > > There are multiple variations of how each of these can be parented, but > only one way they can be without a parent. So how about we turn this the > other way around? > > I.e. let's name this one "cx" and the existing one "cx_w_mx_parent". > > > This will be particularly useful when you look at mmcx, which on > 8150/8180 has mx as parent and on 8450 has cx as parent. > > > PS. Unfortunately I had merged 8450 since you wrote this series, I tried > to just fix it up as I applied your patch, but noticed 8450_cx and > 8450_mmcx and wanted to get your opinion on this first. I agree that sounds like a reasonable thing to do, I hadn't looked at 8450 so did not notice it, I will rebase my patches on top and repost. > > Regards, > Bjorn > >> + .pd = { .name = "cx", }, >> + .peer = &cx_ao_no_parent, >> + .res_name = "cx.lvl", >> +}; >> + >> +static struct rpmhpd cx_ao_no_parent = { >> + .pd = { .name = "cx_ao", }, >> + .active_only = true, >> + .peer = &cx_no_parent, >> + .res_name = "cx.lvl", >> +}; >> + >> static struct rpmhpd mmcx_ao; >> static struct rpmhpd mmcx = { >> .pd = { .name = "mmcx", }, >> @@ -273,8 +287,8 @@ static const struct rpmhpd_desc sc7180_desc = { >> >> /* SC7280 RPMH powerdomains */ >> static struct rpmhpd *sc7280_rpmhpds[] = { >> - [SC7280_CX] = &cx, >> - [SC7280_CX_AO] = &cx_ao, >> + [SC7280_CX] = &cx_no_parent, >> + [SC7280_CX_AO] = &cx_ao_no_parent, >> [SC7280_EBI] = &ebi, >> [SC7280_GFX] = &gfx, >> [SC7280_MX] = &mx, >> -- >> 2.7.4 >>
On 12/9/2021 12:29 PM, Rajendra Nayak wrote: > > On 12/9/2021 2:12 AM, Bjorn Andersson wrote: >> On Tue 07 Dec 04:08 CST 2021, Rajendra Nayak wrote: >> >>> While the requirement to specify the active + sleep and active-only MX >>> power-domains as the parents of the corresponding CX power domains is >>> applicable for most SoCs, we have some like the sc7280 where this >>> dependency is not applicable. >>> Define new rpmhpd structs for cx and cx_ao without the mx as >>> parent and use them for sc7280. >>> >>> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> >>> --- >>> drivers/soc/qcom/rpmhpd.c | 18 ++++++++++++++++-- >>> 1 file changed, 16 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c >>> index c71481d..4599efe 100644 >>> --- a/drivers/soc/qcom/rpmhpd.c >>> +++ b/drivers/soc/qcom/rpmhpd.c >>> @@ -120,6 +120,20 @@ static struct rpmhpd cx_ao = { >>> .res_name = "cx.lvl", >>> }; >>> +static struct rpmhpd cx_ao_no_parent; >>> +static struct rpmhpd cx_no_parent = { >> >> There are multiple variations of how each of these can be parented, but >> only one way they can be without a parent. So how about we turn this the >> other way around? >> >> I.e. let's name this one "cx" and the existing one "cx_w_mx_parent". >> >> >> This will be particularly useful when you look at mmcx, which on >> 8150/8180 has mx as parent and on 8450 has cx as parent. I noticed mmcx on 8150/8180 does not have mx as parent, nevertheless I went ahead and moved to the _w_<parent-name>_parent suffix because it made sense if we did run into a situation like this in the future. >> >> >> PS. Unfortunately I had merged 8450 since you wrote this series, I tried >> to just fix it up as I applied your patch, but noticed 8450_cx and >> 8450_mmcx and wanted to get your opinion on this first. > > I agree that sounds like a reasonable thing to do, I hadn't looked at 8450 > so did not notice it, I will rebase my patches on top and repost. > >> >> Regards, >> Bjorn >> >>> + .pd = { .name = "cx", }, >>> + .peer = &cx_ao_no_parent, >>> + .res_name = "cx.lvl", >>> +}; >>> + >>> +static struct rpmhpd cx_ao_no_parent = { >>> + .pd = { .name = "cx_ao", }, >>> + .active_only = true, >>> + .peer = &cx_no_parent, >>> + .res_name = "cx.lvl", >>> +}; >>> + >>> static struct rpmhpd mmcx_ao; >>> static struct rpmhpd mmcx = { >>> .pd = { .name = "mmcx", }, >>> @@ -273,8 +287,8 @@ static const struct rpmhpd_desc sc7180_desc = { >>> /* SC7280 RPMH powerdomains */ >>> static struct rpmhpd *sc7280_rpmhpds[] = { >>> - [SC7280_CX] = &cx, >>> - [SC7280_CX_AO] = &cx_ao, >>> + [SC7280_CX] = &cx_no_parent, >>> + [SC7280_CX_AO] = &cx_ao_no_parent, >>> [SC7280_EBI] = &ebi, >>> [SC7280_GFX] = &gfx, >>> [SC7280_MX] = &mx, >>> -- >>> 2.7.4 >>>
On Thu 09 Dec 07:37 PST 2021, Rajendra Nayak wrote: > > On 12/9/2021 12:29 PM, Rajendra Nayak wrote: > > > > On 12/9/2021 2:12 AM, Bjorn Andersson wrote: > > > On Tue 07 Dec 04:08 CST 2021, Rajendra Nayak wrote: > > > > > > > While the requirement to specify the active + sleep and active-only MX > > > > power-domains as the parents of the corresponding CX power domains is > > > > applicable for most SoCs, we have some like the sc7280 where this > > > > dependency is not applicable. > > > > Define new rpmhpd structs for cx and cx_ao without the mx as > > > > parent and use them for sc7280. > > > > > > > > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> > > > > --- > > > > drivers/soc/qcom/rpmhpd.c | 18 ++++++++++++++++-- > > > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c > > > > index c71481d..4599efe 100644 > > > > --- a/drivers/soc/qcom/rpmhpd.c > > > > +++ b/drivers/soc/qcom/rpmhpd.c > > > > @@ -120,6 +120,20 @@ static struct rpmhpd cx_ao = { > > > > .res_name = "cx.lvl", > > > > }; > > > > +static struct rpmhpd cx_ao_no_parent; > > > > +static struct rpmhpd cx_no_parent = { > > > > > > There are multiple variations of how each of these can be parented, but > > > only one way they can be without a parent. So how about we turn this the > > > other way around? > > > > > > I.e. let's name this one "cx" and the existing one "cx_w_mx_parent". > > > > > > > > > This will be particularly useful when you look at mmcx, which on > > > 8150/8180 has mx as parent and on 8450 has cx as parent. > > I noticed mmcx on 8150/8180 does not have mx as parent, nevertheless > I went ahead and moved to the _w_<parent-name>_parent suffix because > it made sense if we did run into a situation like this in the future. > You're correct, the 8150/8180 mmcx are wrong, let's fix that once your patches has settled (probably later today). Thanks for cleaning up the driver! Regards, Bjorn > > > > > > > > > PS. Unfortunately I had merged 8450 since you wrote this series, I tried > > > to just fix it up as I applied your patch, but noticed 8450_cx and > > > 8450_mmcx and wanted to get your opinion on this first. > > > > I agree that sounds like a reasonable thing to do, I hadn't looked at 8450 > > so did not notice it, I will rebase my patches on top and repost. > > > > > > > > Regards, > > > Bjorn > > > > > > > + .pd = { .name = "cx", }, > > > > + .peer = &cx_ao_no_parent, > > > > + .res_name = "cx.lvl", > > > > +}; > > > > + > > > > +static struct rpmhpd cx_ao_no_parent = { > > > > + .pd = { .name = "cx_ao", }, > > > > + .active_only = true, > > > > + .peer = &cx_no_parent, > > > > + .res_name = "cx.lvl", > > > > +}; > > > > + > > > > static struct rpmhpd mmcx_ao; > > > > static struct rpmhpd mmcx = { > > > > .pd = { .name = "mmcx", }, > > > > @@ -273,8 +287,8 @@ static const struct rpmhpd_desc sc7180_desc = { > > > > /* SC7280 RPMH powerdomains */ > > > > static struct rpmhpd *sc7280_rpmhpds[] = { > > > > - [SC7280_CX] = &cx, > > > > - [SC7280_CX_AO] = &cx_ao, > > > > + [SC7280_CX] = &cx_no_parent, > > > > + [SC7280_CX_AO] = &cx_ao_no_parent, > > > > [SC7280_EBI] = &ebi, > > > > [SC7280_GFX] = &gfx, > > > > [SC7280_MX] = &mx, > > > > -- > > > > 2.7.4 > > > >
diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c index c71481d..4599efe 100644 --- a/drivers/soc/qcom/rpmhpd.c +++ b/drivers/soc/qcom/rpmhpd.c @@ -120,6 +120,20 @@ static struct rpmhpd cx_ao = { .res_name = "cx.lvl", }; +static struct rpmhpd cx_ao_no_parent; +static struct rpmhpd cx_no_parent = { + .pd = { .name = "cx", }, + .peer = &cx_ao_no_parent, + .res_name = "cx.lvl", +}; + +static struct rpmhpd cx_ao_no_parent = { + .pd = { .name = "cx_ao", }, + .active_only = true, + .peer = &cx_no_parent, + .res_name = "cx.lvl", +}; + static struct rpmhpd mmcx_ao; static struct rpmhpd mmcx = { .pd = { .name = "mmcx", }, @@ -273,8 +287,8 @@ static const struct rpmhpd_desc sc7180_desc = { /* SC7280 RPMH powerdomains */ static struct rpmhpd *sc7280_rpmhpds[] = { - [SC7280_CX] = &cx, - [SC7280_CX_AO] = &cx_ao, + [SC7280_CX] = &cx_no_parent, + [SC7280_CX_AO] = &cx_ao_no_parent, [SC7280_EBI] = &ebi, [SC7280_GFX] = &gfx, [SC7280_MX] = &mx,
While the requirement to specify the active + sleep and active-only MX power-domains as the parents of the corresponding CX power domains is applicable for most SoCs, we have some like the sc7280 where this dependency is not applicable. Define new rpmhpd structs for cx and cx_ao without the mx as parent and use them for sc7280. Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com> --- drivers/soc/qcom/rpmhpd.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)