| Message ID | 1637040382-22987-2-git-send-email-rnayak@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] soc: qcom: rpmhpd: Rename rpmhpd struct names | expand |
On Mon 15 Nov 23:26 CST 2021, Rajendra Nayak wrote: > The requirement to specify the active + sleep and active-only MX power > domains as the parents of the corresponding CX power domains is applicable > only on the sdm845 SoC. With the same struct definition reused for all the > SoCs this condition was wrongly applied to all those SoCs as well, which > isn't needed. Define new sdm845 specific structures to manage this > dependency and remove the parent assignements from the common structure. > Looking at the downstream sm8150 dts I see that both cx and mmcx specifies mx as parent "supply". Is this not needed or should we instead name these resources "cx_with_mx_parent" and have sm8150 opt in as well? Regards, Bjorn > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > --- > drivers/soc/qcom/rpmhpd.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c > index c71481d..12d8ce9 100644 > --- a/drivers/soc/qcom/rpmhpd.c > +++ b/drivers/soc/qcom/rpmhpd.c > @@ -108,7 +108,6 @@ static struct rpmhpd cx_ao; > static struct rpmhpd cx = { > .pd = { .name = "cx", }, > .peer = &cx_ao, > - .parent = &mx.pd, > .res_name = "cx.lvl", > }; > > @@ -116,7 +115,6 @@ static struct rpmhpd cx_ao = { > .pd = { .name = "cx_ao", }, > .active_only = true, > .peer = &cx, > - .parent = &mx_ao.pd, > .res_name = "cx.lvl", > }; > > @@ -149,12 +147,28 @@ static struct rpmhpd mxc_ao = { > }; > > /* SDM845 RPMH powerdomains */ > +static struct rpmhpd sdm845_cx_ao; > +static struct rpmhpd sdm845_cx = { > + .pd = { .name = "cx", }, > + .peer = &sdm845_cx_ao, > + .parent = &mx.pd, > + .res_name = "cx.lvl", > +}; > + > +static struct rpmhpd sdm845_cx_ao = { > + .pd = { .name = "cx_ao", }, > + .active_only = true, > + .peer = &sdm845_cx, > + .parent = &mx_ao.pd, > + .res_name = "cx.lvl", > +}; > + > static struct rpmhpd *sdm845_rpmhpds[] = { > [SDM845_EBI] = &ebi, > [SDM845_MX] = &mx, > [SDM845_MX_AO] = &mx_ao, > - [SDM845_CX] = &cx, > - [SDM845_CX_AO] = &cx_ao, > + [SDM845_CX] = &sdm845_cx, > + [SDM845_CX_AO] = &sdm845_cx_ao, > [SDM845_LMX] = &lmx, > [SDM845_LCX] = &lcx, > [SDM845_GFX] = &gfx, > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation >
On 11/19/2021 8:49 AM, Bjorn Andersson wrote: > On Mon 15 Nov 23:26 CST 2021, Rajendra Nayak wrote: > >> The requirement to specify the active + sleep and active-only MX power >> domains as the parents of the corresponding CX power domains is applicable >> only on the sdm845 SoC. With the same struct definition reused for all the >> SoCs this condition was wrongly applied to all those SoCs as well, which >> isn't needed. Define new sdm845 specific structures to manage this >> dependency and remove the parent assignements from the common structure. >> > > Looking at the downstream sm8150 dts I see that both cx and mmcx > specifies mx as parent "supply". > > Is this not needed or should we instead name these resources > "cx_with_mx_parent" and have sm8150 opt in as well? Right, looks like these are needed, after talking to some more folks I was told RPMh does not really enforce any dependencies on any of the SoCs, so my earlier statement was wrong that this was managed by RPMh. Some SoCs just have some digital domain requirements which need these dependencies to be managed (not all SoCs) and when we end up with such a situation its almost always expected to be managed by the RPMh masters (APPS running hlos in this case) This is not just across cx/mx but others as well like mmcx/mxc/gfx etc. Unfortunately I could not find this very well documented at an SoC level, so perhaps the best way to go about is to look at downstream dependencies and try to match them upstream :/ I will respin this to add the 8150 dependencies back (and if I see any more for the others) > > Regards, > Bjorn > >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >> --- >> drivers/soc/qcom/rpmhpd.c | 22 ++++++++++++++++++---- >> 1 file changed, 18 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c >> index c71481d..12d8ce9 100644 >> --- a/drivers/soc/qcom/rpmhpd.c >> +++ b/drivers/soc/qcom/rpmhpd.c >> @@ -108,7 +108,6 @@ static struct rpmhpd cx_ao; >> static struct rpmhpd cx = { >> .pd = { .name = "cx", }, >> .peer = &cx_ao, >> - .parent = &mx.pd, >> .res_name = "cx.lvl", >> }; >> >> @@ -116,7 +115,6 @@ static struct rpmhpd cx_ao = { >> .pd = { .name = "cx_ao", }, >> .active_only = true, >> .peer = &cx, >> - .parent = &mx_ao.pd, >> .res_name = "cx.lvl", >> }; >> >> @@ -149,12 +147,28 @@ static struct rpmhpd mxc_ao = { >> }; >> >> /* SDM845 RPMH powerdomains */ >> +static struct rpmhpd sdm845_cx_ao; >> +static struct rpmhpd sdm845_cx = { >> + .pd = { .name = "cx", }, >> + .peer = &sdm845_cx_ao, >> + .parent = &mx.pd, >> + .res_name = "cx.lvl", >> +}; >> + >> +static struct rpmhpd sdm845_cx_ao = { >> + .pd = { .name = "cx_ao", }, >> + .active_only = true, >> + .peer = &sdm845_cx, >> + .parent = &mx_ao.pd, >> + .res_name = "cx.lvl", >> +}; >> + >> static struct rpmhpd *sdm845_rpmhpds[] = { >> [SDM845_EBI] = &ebi, >> [SDM845_MX] = &mx, >> [SDM845_MX_AO] = &mx_ao, >> - [SDM845_CX] = &cx, >> - [SDM845_CX_AO] = &cx_ao, >> + [SDM845_CX] = &sdm845_cx, >> + [SDM845_CX_AO] = &sdm845_cx_ao, >> [SDM845_LMX] = &lmx, >> [SDM845_LCX] = &lcx, >> [SDM845_GFX] = &gfx, >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member >> of Code Aurora Forum, hosted by The Linux Foundation >>
On 11/23/2021 12:45 PM, Rajendra Nayak wrote: > > On 11/19/2021 8:49 AM, Bjorn Andersson wrote: >> On Mon 15 Nov 23:26 CST 2021, Rajendra Nayak wrote: >> >>> The requirement to specify the active + sleep and active-only MX power >>> domains as the parents of the corresponding CX power domains is applicable >>> only on the sdm845 SoC. With the same struct definition reused for all the >>> SoCs this condition was wrongly applied to all those SoCs as well, which >>> isn't needed. Define new sdm845 specific structures to manage this >>> dependency and remove the parent assignements from the common structure. >>> >> >> Looking at the downstream sm8150 dts I see that both cx and mmcx >> specifies mx as parent "supply". >> >> Is this not needed or should we instead name these resources >> "cx_with_mx_parent" and have sm8150 opt in as well? > > Right, looks like these are needed, after talking to some more folks > I was told RPMh does not really enforce any dependencies on any of the > SoCs, so my earlier statement was wrong that this was managed by RPMh. > Some SoCs just have some digital domain requirements which need these > dependencies to be managed (not all SoCs) and when we end up with such > a situation its almost always expected to be managed by the RPMh masters > (APPS running hlos in this case) > This is not just across cx/mx but others as well like mmcx/mxc/gfx etc. > > Unfortunately I could not find this very well documented at an SoC level, > so perhaps the best way to go about is to look at downstream dependencies > and try to match them upstream :/ > I will respin this to add the 8150 dependencies back (and if I see any more > for the others) Looking through this more in downstream files, I see atleast the mx being a parent of cx is needed on pretty much all upstream supported SoCs, except the sc7280. There seem to be more complex dependencies that downstream models across other rails, mainly for 8150/8250/8350 and the most recent 8450 but looks like we have been able to live without those upstream so I plan to leave them for now and just re-post this with an additional cx_with_mx_parent and a cx to distinguish between these 2 cases. > >> >> Regards, >> Bjorn >> >>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >>> --- >>> drivers/soc/qcom/rpmhpd.c | 22 ++++++++++++++++++---- >>> 1 file changed, 18 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c >>> index c71481d..12d8ce9 100644 >>> --- a/drivers/soc/qcom/rpmhpd.c >>> +++ b/drivers/soc/qcom/rpmhpd.c >>> @@ -108,7 +108,6 @@ static struct rpmhpd cx_ao; >>> static struct rpmhpd cx = { >>> .pd = { .name = "cx", }, >>> .peer = &cx_ao, >>> - .parent = &mx.pd, >>> .res_name = "cx.lvl", >>> }; >>> @@ -116,7 +115,6 @@ static struct rpmhpd cx_ao = { >>> .pd = { .name = "cx_ao", }, >>> .active_only = true, >>> .peer = &cx, >>> - .parent = &mx_ao.pd, >>> .res_name = "cx.lvl", >>> }; >>> @@ -149,12 +147,28 @@ static struct rpmhpd mxc_ao = { >>> }; >>> /* SDM845 RPMH powerdomains */ >>> +static struct rpmhpd sdm845_cx_ao; >>> +static struct rpmhpd sdm845_cx = { >>> + .pd = { .name = "cx", }, >>> + .peer = &sdm845_cx_ao, >>> + .parent = &mx.pd, >>> + .res_name = "cx.lvl", >>> +}; >>> + >>> +static struct rpmhpd sdm845_cx_ao = { >>> + .pd = { .name = "cx_ao", }, >>> + .active_only = true, >>> + .peer = &sdm845_cx, >>> + .parent = &mx_ao.pd, >>> + .res_name = "cx.lvl", >>> +}; >>> + >>> static struct rpmhpd *sdm845_rpmhpds[] = { >>> [SDM845_EBI] = &ebi, >>> [SDM845_MX] = &mx, >>> [SDM845_MX_AO] = &mx_ao, >>> - [SDM845_CX] = &cx, >>> - [SDM845_CX_AO] = &cx_ao, >>> + [SDM845_CX] = &sdm845_cx, >>> + [SDM845_CX_AO] = &sdm845_cx_ao, >>> [SDM845_LMX] = &lmx, >>> [SDM845_LCX] = &lcx, >>> [SDM845_GFX] = &gfx, >>> -- >>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member >>> of Code Aurora Forum, hosted by The Linux Foundation >>> >
diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c index c71481d..12d8ce9 100644 --- a/drivers/soc/qcom/rpmhpd.c +++ b/drivers/soc/qcom/rpmhpd.c @@ -108,7 +108,6 @@ static struct rpmhpd cx_ao; static struct rpmhpd cx = { .pd = { .name = "cx", }, .peer = &cx_ao, - .parent = &mx.pd, .res_name = "cx.lvl", }; @@ -116,7 +115,6 @@ static struct rpmhpd cx_ao = { .pd = { .name = "cx_ao", }, .active_only = true, .peer = &cx, - .parent = &mx_ao.pd, .res_name = "cx.lvl", }; @@ -149,12 +147,28 @@ static struct rpmhpd mxc_ao = { }; /* SDM845 RPMH powerdomains */ +static struct rpmhpd sdm845_cx_ao; +static struct rpmhpd sdm845_cx = { + .pd = { .name = "cx", }, + .peer = &sdm845_cx_ao, + .parent = &mx.pd, + .res_name = "cx.lvl", +}; + +static struct rpmhpd sdm845_cx_ao = { + .pd = { .name = "cx_ao", }, + .active_only = true, + .peer = &sdm845_cx, + .parent = &mx_ao.pd, + .res_name = "cx.lvl", +}; + static struct rpmhpd *sdm845_rpmhpds[] = { [SDM845_EBI] = &ebi, [SDM845_MX] = &mx, [SDM845_MX_AO] = &mx_ao, - [SDM845_CX] = &cx, - [SDM845_CX_AO] = &cx_ao, + [SDM845_CX] = &sdm845_cx, + [SDM845_CX_AO] = &sdm845_cx_ao, [SDM845_LMX] = &lmx, [SDM845_LCX] = &lcx, [SDM845_GFX] = &gfx,
The requirement to specify the active + sleep and active-only MX power domains as the parents of the corresponding CX power domains is applicable only on the sdm845 SoC. With the same struct definition reused for all the SoCs this condition was wrongly applied to all those SoCs as well, which isn't needed. Define new sdm845 specific structures to manage this dependency and remove the parent assignements from the common structure. Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> --- drivers/soc/qcom/rpmhpd.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)