| Message ID | 20230708-topic-rpmh_icc_rsc-v1-0-b223bd2ac8dd@linaro.org |
|---|---|
| Headers | show |
| Series | icc-rpmh multi-RSC voting groundwork | expand |
On 11/07/2023 14:18, Konrad Dybcio wrote: > It makes zero (or less) sense to consume BCM voters per interconnect > provider. They are shared throughout the entire system and it's enough > to keep a single reference to each of them. > > Storing them in a shared array at fixed indices will let us improve both > the representation of the RPMh architecture (every RSC can hold a resource > vote on any bus, they're not limited in that regard) and save as much as > kilobytes worth of RAM. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > include/dt-bindings/interconnect/qcom,icc.h | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/include/dt-bindings/interconnect/qcom,icc.h b/include/dt-bindings/interconnect/qcom,icc.h > index cd34f36daaaa..9c13ef8a044e 100644 > --- a/include/dt-bindings/interconnect/qcom,icc.h > +++ b/include/dt-bindings/interconnect/qcom,icc.h > @@ -23,4 +23,12 @@ > #define QCOM_ICC_TAG_ALWAYS (QCOM_ICC_TAG_AMC | QCOM_ICC_TAG_WAKE |\ > QCOM_ICC_TAG_SLEEP) > > +#define ICC_BCM_VOTER_APPS 0 > +#define ICC_BCM_VOTER_DISP 1 > +#define ICC_BCM_VOTER_CAM0 2 > +#define ICC_BCM_VOTER_CAM1 3 > +#define ICC_BCM_VOTER_CAM2 4 > + > +#define ICC_BCM_VOTER_MAX 64 I proposed to skip the max. If you actually use it, you won't be able to change it ever. Best regards, Krzysztof
On 12.07.2023 22:39, Krzysztof Kozlowski wrote: > On 11/07/2023 14:18, Konrad Dybcio wrote: >> It makes zero (or less) sense to consume BCM voters per interconnect >> provider. They are shared throughout the entire system and it's enough >> to keep a single reference to each of them. >> >> Storing them in a shared array at fixed indices will let us improve both >> the representation of the RPMh architecture (every RSC can hold a resource >> vote on any bus, they're not limited in that regard) and save as much as >> kilobytes worth of RAM. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- >> include/dt-bindings/interconnect/qcom,icc.h | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/include/dt-bindings/interconnect/qcom,icc.h b/include/dt-bindings/interconnect/qcom,icc.h >> index cd34f36daaaa..9c13ef8a044e 100644 >> --- a/include/dt-bindings/interconnect/qcom,icc.h >> +++ b/include/dt-bindings/interconnect/qcom,icc.h >> @@ -23,4 +23,12 @@ >> #define QCOM_ICC_TAG_ALWAYS (QCOM_ICC_TAG_AMC | QCOM_ICC_TAG_WAKE |\ >> QCOM_ICC_TAG_SLEEP) >> >> +#define ICC_BCM_VOTER_APPS 0 >> +#define ICC_BCM_VOTER_DISP 1 >> +#define ICC_BCM_VOTER_CAM0 2 >> +#define ICC_BCM_VOTER_CAM1 3 >> +#define ICC_BCM_VOTER_CAM2 4 >> + >> +#define ICC_BCM_VOTER_MAX 64 > > I proposed to skip the max. If you actually use it, you won't be able to > change it ever. I guess I can just add the max in the driver. Konrad > > > Best regards, > Krzysztof >
Hi Konrad, On 11.07.23 15:17, Konrad Dybcio wrote: > Many parts of Qualcomm SoCs are entirely independent of each other and can > run when the other parts are off. The RPMh system architecture embraces > this by giving each (loosely defined) subsystem its own connection (as in, > physical wires) to the AOSS, terminated by per-subsystem RSCs (Resource > State Coordinators) that barter for power, bandwidth etc. > > This series introduces the groundwork necessary for voting for resources > through non-APPS RSCs. It should allow for lower-latency vote adjustments > (e.g. for very high bandwidth / multiple displays) and could potentially > allow for full APSS collapse while keeping e.g. MDSS operating (say > refreshing an image from a RAM buffer). This is good stuff. Thanks for working on it! Actually the path tagging, that have been introduced some time ago could be used for supporting the multiple RSCs. Today we can get the tags from DT, and tag the path with some DISP_RSC flag (for example) and avoid the qcom,bcm-voter-idx property. Mike has been also looking into this, so maybe he can share his thoughts. > > On top of that, a rather necessary and overdue cleanup is performed to > stop adding more and more arguments to the insane preprocessor macros. > Retiring the DEFINE_QNODE is good clean-up, but some patches failed to apply so a re-base would be needed. Thanks, Georgi > Partially reverting (or reimplementing the revert) [1] will be necessary > to coordinate the rather complex relationship between the DPU and RSC > drivers. > > The "Point x paths to the x RSC" patches won't do anything (check the > compatibility workaround qcom_icc_pre_aggregate()) until disp_rsc is > properly described in the device tree, along with its BCM voter), > but they prepare ground for when that happens. > > I was able to test sending requests through the DISP_RSC on SM8450, but > I had to hack its clocks (_rscc_ in dispcc) to be always-on, as we don't > have any clk handling for qcom,rpmh-rsc today. > > Boot-tested on SM8350 and SM8450, nothing exploded. > > [1] https://patchwork.kernel.org/project/dri-devel/patch/1521827074-28424-1-git-send-email-ryadav@codeaurora.org/ > > Dependencies: > [2] https://lore.kernel.org/linux-arm-msm/113b50f8-35f6-73fc-4fc9-302262927c5e@quicinc.com/ > [3] https://lore.kernel.org/linux-arm-msm/20230703-topic-8250_qup_icc-v2-0-9ba0a9460be2@linaro.org/ > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > Konrad Dybcio (53): > dt-bindings: interconnect: qcom,icc: Introduce fixed BCM voter indices > dt-bindings: interconnect: qcom,bcm-voter: Add qcom,bcm-voter-idx > interconnect: qcom: icc-rpmh: Store direct BCM voter references > interconnect: qcom: icc-rpmh: Retire dead code > interconnect: qcom: icc-rpmh: Implement voting on non-APPS RSCs > interconnect: qcom: sc7180: Retire DEFINE_QNODE > interconnect: qcom: sdm670: Retire DEFINE_QNODE > interconnect: qcom: sdm845: Retire DEFINE_QNODE > interconnect: qcom: sdx55: Retire DEFINE_QNODE > interconnect: qcom: sdx65: Retire DEFINE_QNODE > interconnect: qcom: sm6350: Retire DEFINE_QNODE > interconnect: qcom: sm8150: Retire DEFINE_QNODE > interconnect: qcom: sm8250: Retire DEFINE_QNODE > interconnect: qcom: sm8350: Retire DEFINE_QNODE > interconnect: qcom: icc-rpmh: Retire DEFINE_QNODE > interconnect: qcom: sc7180: Retire DEFINE_QBCM > interconnect: qcom: sdm670: Retire DEFINE_QBCM > interconnect: qcom: sdm845: Retire DEFINE_QBCM > interconnect: qcom: sdx55: Retire DEFINE_QBCM > interconnect: qcom: sdx65: Retire DEFINE_QBCM > interconnect: qcom: sm6350: Retire DEFINE_QBCM > interconnect: qcom: sm8150: Retire DEFINE_QBCM > interconnect: qcom: sm8250: Retire DEFINE_QBCM > interconnect: qcom: sm8350: Retire DEFINE_QBCM > interconnect: qcom: icc-rpmh: Retire DEFINE_QBCM > interconnect: qcom: qdu1000: Explicitly assign voter_idx > interconnect: qcom: sa8775p: Explicitly assign voter_idx > interconnect: qcom: sc7280: Explicitly assign voter_idx > interconnect: qcom: sc8180x: Explicitly assign voter_idx > interconnect: qcom: sc8280xp: Explicitly assign voter_idx > interconnect: qcom: sm8450: Explicitly assign voter_idx > interconnect: qcom: sm8550: Explicitly assign voter_idx > arm64: dts: qcom: qdu1000: add qcom,bcm-voter-idx > arm64: dts: qcom: sa8775p: add qcom,bcm-voter-idx > arm64: dts: qcom: sc7180: add qcom,bcm-voter-idx > arm64: dts: qcom: sc7280: add qcom,bcm-voter-idx > arm64: dts: qcom: sc8180x: add qcom,bcm-voter-idx > arm64: dts: qcom: sc8280xp: add qcom,bcm-voter-idx > arm64: dts: qcom: sdm670: add qcom,bcm-voter-idx > arm64: dts: qcom: sdm845: add qcom,bcm-voter-idx > arm64: dts: qcom: sdx75: add qcom,bcm-voter-idx > arm64: dts: qcom: sm6350: add qcom,bcm-voter-idx > arm64: dts: qcom: sm8150: add qcom,bcm-voter-idx > arm64: dts: qcom: sm8250: add qcom,bcm-voter-idx > arm64: dts: qcom: sm8350: add qcom,bcm-voter-idx > arm64: dts: qcom: sm8450: add qcom,bcm-voter-idx > arm64: dts: qcom: sm8550: add qcom,bcm-voter-idx > arm64: dts: qcom: sdx55: add qcom,bcm-voter-idx > arm64: dts: qcom: sdx65: add qcom,bcm-voter-idx > interconnect: qcom: sm8350: Point display paths to the display RSC > interconnect: qcom: sm8450: Point display paths to the display RSC > interconnect: qcom: sm8550: Point display paths to the display RSC > interconnect: qcom: sm8550: Point camera paths to the camera RSC > > .../bindings/interconnect/qcom,bcm-voter.yaml | 10 + > arch/arm/boot/dts/qcom/qcom-sdx55.dtsi | 1 + > arch/arm/boot/dts/qcom/qcom-sdx65.dtsi | 1 + > arch/arm64/boot/dts/qcom/qdu1000.dtsi | 2 + > arch/arm64/boot/dts/qcom/sa8775p.dtsi | 1 + > arch/arm64/boot/dts/qcom/sc7180.dtsi | 1 + > arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 + > arch/arm64/boot/dts/qcom/sc8180x.dtsi | 2 + > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 2 + > arch/arm64/boot/dts/qcom/sdm670.dtsi | 2 + > arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 + > arch/arm64/boot/dts/qcom/sdx75.dtsi | 2 + > arch/arm64/boot/dts/qcom/sm6350.dtsi | 1 + > arch/arm64/boot/dts/qcom/sm8150.dtsi | 2 + > arch/arm64/boot/dts/qcom/sm8250.dtsi | 2 + > arch/arm64/boot/dts/qcom/sm8350.dtsi | 2 + > arch/arm64/boot/dts/qcom/sm8450.dtsi | 2 + > arch/arm64/boot/dts/qcom/sm8550.dtsi | 2 + > drivers/interconnect/qcom/bcm-voter.c | 75 +- > drivers/interconnect/qcom/bcm-voter.h | 9 - > drivers/interconnect/qcom/icc-rpmh.c | 53 +- > drivers/interconnect/qcom/icc-rpmh.h | 14 +- > drivers/interconnect/qcom/qdu1000.c | 11 + > drivers/interconnect/qcom/sa8775p.c | 28 + > drivers/interconnect/qcom/sc7180.c | 1637 +++++++++++++++-- > drivers/interconnect/qcom/sc7280.c | 27 + > drivers/interconnect/qcom/sc8180x.c | 23 + > drivers/interconnect/qcom/sc8280xp.c | 27 + > drivers/interconnect/qcom/sdm670.c | 1410 +++++++++++++-- > drivers/interconnect/qcom/sdm845.c | 1683 ++++++++++++++++-- > drivers/interconnect/qcom/sdx55.c | 863 ++++++++- > drivers/interconnect/qcom/sdx65.c | 850 ++++++++- > drivers/interconnect/qcom/sm6350.c | 1551 +++++++++++++++-- > drivers/interconnect/qcom/sm8150.c | 1714 ++++++++++++++++-- > drivers/interconnect/qcom/sm8250.c | 1772 +++++++++++++++++-- > drivers/interconnect/qcom/sm8350.c | 1830 ++++++++++++++++++-- > drivers/interconnect/qcom/sm8450.c | 24 + > drivers/interconnect/qcom/sm8550.c | 42 + > include/dt-bindings/interconnect/qcom,icc.h | 8 + > 39 files changed, 12320 insertions(+), 1370 deletions(-) > --- > base-commit: 82cee168d497ffcb79e4889fe3c7384788e89f4d > change-id: 20230708-topic-rpmh_icc_rsc-f897080fb207 > > Best regards,
On Thu, Aug 03, 2023 at 07:48:08PM +0300, Georgi Djakov wrote: > Hi Konrad, > > On 11.07.23 15:17, Konrad Dybcio wrote: > > Many parts of Qualcomm SoCs are entirely independent of each other and can > > run when the other parts are off. The RPMh system architecture embraces > > this by giving each (loosely defined) subsystem its own connection (as in, > > physical wires) to the AOSS, terminated by per-subsystem RSCs (Resource > > State Coordinators) that barter for power, bandwidth etc. > > > > This series introduces the groundwork necessary for voting for resources > > through non-APPS RSCs. It should allow for lower-latency vote adjustments > > (e.g. for very high bandwidth / multiple displays) and could potentially > > allow for full APSS collapse while keeping e.g. MDSS operating (say > > refreshing an image from a RAM buffer). > > This is good stuff. Thanks for working on it! Actually the path tagging, > that have been introduced some time ago could be used for supporting the > multiple RSCs. Today we can get the tags from DT, and tag the path with > some DISP_RSC flag (for example) and avoid the qcom,bcm-voter-idx property. > > Mike has been also looking into this, so maybe he can share his thoughts. > Yeah, the current way we've been supporting multiple voters (e.g. RSCs) doesn't scale. We currently duplicate the topology for any path that requires a secondary, non-APSS voter. Which means we have duplicates nodes and bindings for each hop in those paths, even though there's only a single logical path. For example, in qcom/sm8550.c, each node and BCM ending with _disp, _ife_0, _ife_1, or _ife_2 is a duplicate. The only reason they exist is to allow clients to target their votes to the non-APPS voters. And to provide separate, voter-specific buckets of aggregation. But everything else about them is 100% identical to their default APPS counterparts. For sm8550, this amounts to roughly 643 extra lines of code. Initially there was only the one secondary display voter, so the scaling problem wasn't a huge issue. But sm8550 has four voters. And future SOCs will have even more. We should only define the logical topology once. The ratio of NOC ports to interconnect nodes should be 1:1, rather than 1:N where N is the number of voters that care about them. The general idea is that we could use tags for this. So, instead of... path = icc_get(dev, MASTER_MDP_DISP, SLAVE_EBI1_DISP); it would be... path = icc_get(dev, MASTER_MDP, SLAVE_EBI1); icc_set_tag(path, QCOM_ICC_TAG_VOTER_DISP); I have an early prototype with basic testing already. I can hopefully clean it up and post for review in the next couple of weeks.
On 7.08.2023 23:57, Mike Tipton wrote: > On Thu, Aug 03, 2023 at 07:48:08PM +0300, Georgi Djakov wrote: >> Hi Konrad, >> >> On 11.07.23 15:17, Konrad Dybcio wrote: >>> Many parts of Qualcomm SoCs are entirely independent of each other and can >>> run when the other parts are off. The RPMh system architecture embraces >>> this by giving each (loosely defined) subsystem its own connection (as in, >>> physical wires) to the AOSS, terminated by per-subsystem RSCs (Resource >>> State Coordinators) that barter for power, bandwidth etc. >>> >>> This series introduces the groundwork necessary for voting for resources >>> through non-APPS RSCs. It should allow for lower-latency vote adjustments >>> (e.g. for very high bandwidth / multiple displays) and could potentially >>> allow for full APSS collapse while keeping e.g. MDSS operating (say >>> refreshing an image from a RAM buffer). >> >> This is good stuff. Thanks for working on it! Actually the path tagging, >> that have been introduced some time ago could be used for supporting the >> multiple RSCs. Today we can get the tags from DT, and tag the path with >> some DISP_RSC flag (for example) and avoid the qcom,bcm-voter-idx property. >> >> Mike has been also looking into this, so maybe he can share his thoughts. >> > > Yeah, the current way we've been supporting multiple voters (e.g. RSCs) > doesn't scale. We currently duplicate the topology for any path that > requires a secondary, non-APSS voter. Which means we have duplicates > nodes and bindings for each hop in those paths, even though there's only > a single logical path. > > For example, in qcom/sm8550.c, each node and BCM ending with _disp, > _ife_0, _ife_1, or _ife_2 is a duplicate. The only reason they exist is > to allow clients to target their votes to the non-APPS voters. And to > provide separate, voter-specific buckets of aggregation. But everything > else about them is 100% identical to their default APPS counterparts. > For sm8550, this amounts to roughly 643 extra lines of code. > > Initially there was only the one secondary display voter, so the scaling > problem wasn't a huge issue. But sm8550 has four voters. And future SOCs > will have even more. > > We should only define the logical topology once. The ratio of NOC ports > to interconnect nodes should be 1:1, rather than 1:N where N is the > number of voters that care about them. > > The general idea is that we could use tags for this. So, instead of... > > path = icc_get(dev, MASTER_MDP_DISP, SLAVE_EBI1_DISP); > > it would be... > > path = icc_get(dev, MASTER_MDP, SLAVE_EBI1); > icc_set_tag(path, QCOM_ICC_TAG_VOTER_DISP); > > I have an early prototype with basic testing already. I can hopefully > clean it up and post for review in the next couple of weeks. I was initially not very happy with this approach (overloading tags with additional information), but it grew on me over time. My only concern is that if we reserve say bits 16-31 for path tags (remember, dt-bindings are ABI), we may eventually run out of them. Konrad
On Wed, Sep 06, 2023 at 02:14:14PM +0200, Konrad Dybcio wrote: > > The general idea is that we could use tags for this. So, instead of... > > > > path = icc_get(dev, MASTER_MDP_DISP, SLAVE_EBI1_DISP); > > > > it would be... > > > > path = icc_get(dev, MASTER_MDP, SLAVE_EBI1); > > icc_set_tag(path, QCOM_ICC_TAG_VOTER_DISP); > > > > I have an early prototype with basic testing already. I can hopefully > > clean it up and post for review in the next couple of weeks. > I was initially not very happy with this approach (overloading tags > with additional information), but it grew on me over time. > > My only concern is that if we reserve say bits 16-31 for path tags > (remember, dt-bindings are ABI), we may eventually run out of them. The voter tags wouldn't require bitmasks like the bucket tags do. We'd just need an integer for each voter shifted into the proper position in the tag value. Thus, reserving N bits for the voters would give us 2**N voters, which should be plenty. For example: #define QCOM_ICC_VOTERS_START 16 #define QCOM_ICC_VOTERS_END 23 #define QCOM_ICC_TAG_VOTER_HLOS (0 << QCOM_ICC_VOTERS_START) #define QCOM_ICC_TAG_VOTER_DISP (1 << QCOM_ICC_VOTERS_START) #define QCOM_ICC_TAG_VOTER_CAM_IFE_0 (2 << QCOM_ICC_VOTERS_START) #define QCOM_ICC_TAG_VOTER_CAM_IFE_1 (3 << QCOM_ICC_VOTERS_START) #define QCOM_ICC_TAG_VOTER_CAM_IFE_2 (4 << QCOM_ICC_VOTERS_START) The applicable voters should likely be defined in the target-specific headers, rather than the common qcom,icc.h. The bit range used for them could be common, but each target may only support a small subset of the total set of possible voters across all targets. Clients requiring multiple voters for the same logical path should be rare. On the off-chance they require that, they could just request the same path multiple times with different voter tags applied and call icc_set_bw() for each of them separately. I'm back from travel and vacation and plan to pick this up again soon.
On 13.09.2023 03:29, Mike Tipton wrote: > On Wed, Sep 06, 2023 at 02:14:14PM +0200, Konrad Dybcio wrote: >>> The general idea is that we could use tags for this. So, instead of... >>> >>> path = icc_get(dev, MASTER_MDP_DISP, SLAVE_EBI1_DISP); >>> >>> it would be... >>> >>> path = icc_get(dev, MASTER_MDP, SLAVE_EBI1); >>> icc_set_tag(path, QCOM_ICC_TAG_VOTER_DISP); >>> >>> I have an early prototype with basic testing already. I can hopefully >>> clean it up and post for review in the next couple of weeks. >> I was initially not very happy with this approach (overloading tags >> with additional information), but it grew on me over time. >> >> My only concern is that if we reserve say bits 16-31 for path tags >> (remember, dt-bindings are ABI), we may eventually run out of them. > > The voter tags wouldn't require bitmasks like the bucket tags do. We'd > just need an integer for each voter shifted into the proper position in > the tag value. Thus, reserving N bits for the voters would give us 2**N > voters, which should be plenty. For example: > > #define QCOM_ICC_VOTERS_START 16 > #define QCOM_ICC_VOTERS_END 23 > > #define QCOM_ICC_TAG_VOTER_HLOS (0 << QCOM_ICC_VOTERS_START) > #define QCOM_ICC_TAG_VOTER_DISP (1 << QCOM_ICC_VOTERS_START) > #define QCOM_ICC_TAG_VOTER_CAM_IFE_0 (2 << QCOM_ICC_VOTERS_START) > #define QCOM_ICC_TAG_VOTER_CAM_IFE_1 (3 << QCOM_ICC_VOTERS_START) > #define QCOM_ICC_TAG_VOTER_CAM_IFE_2 (4 << QCOM_ICC_VOTERS_START) > > The applicable voters should likely be defined in the target-specific > headers, rather than the common qcom,icc.h. The bit range used for them > could be common, but each target may only support a small subset of the > total set of possible voters across all targets. I'm not sure how client drivers would then choose the correct path other than switch (soc) { case 8450: tag = QCOM_ICC_TAG_VOTER_8450_HLOS; break; case 8550: tag = QCOM_ICC_TAG_VOTER_8550_HLOS; break; ... } which would be unacceptable. > Clients requiring multiple voters for the same logical path should be > rare. On the off-chance they require that, they could just request the > same path multiple times with different voter tags applied and call > icc_set_bw() for each of them separately. > > I'm back from travel and vacation and plan to pick this up again soon. Happy to hear that! Konrad
On Wed, Sep 13, 2023 at 10:31:49AM +0200, Konrad Dybcio wrote: > > The applicable voters should likely be defined in the target-specific > > headers, rather than the common qcom,icc.h. The bit range used for them > > could be common, but each target may only support a small subset of the > > total set of possible voters across all targets. > I'm not sure how client drivers would then choose the > correct path other than > > switch (soc) { > case 8450: > tag = QCOM_ICC_TAG_VOTER_8450_HLOS; > break; > case 8550: > tag = QCOM_ICC_TAG_VOTER_8550_HLOS; > break; > ... > } > > which would be unacceptable. The same general way it's handled for the endpoint bindings, which are already target-specific. Any client drivers hardcoding the endpoint bindings in their driver would have to include the appropriate, target-specific binding header (e.g. qcom,sm8550-rpmh.h). That would only be possible if their driver file is itself target-specific. Otherwise, it would have to pull the endpoint bindings from devicetree. Or just use the recommended of_icc_get() and let devicetree do everything for them. Same for the target-specific voter tag bindings. Clients can also specify their tags in devicetree. They don't actually have to call icc_set_tag() directly. For example: #include <dt-bindings/interconnect/qcom,sm8450.h> interconnects = <&mmss_noc MASTER_MDP QCOM_ICC_TAG_VOTER_DISP &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_VOTER_DISP>; Then when they call of_icc_get() for this path it'll automatically have QCOM_ICC_TAG_VOTER_DISP set for them.
On 14.09.2023 04:32, Mike Tipton wrote: > On Wed, Sep 13, 2023 at 10:31:49AM +0200, Konrad Dybcio wrote: >>> The applicable voters should likely be defined in the target-specific >>> headers, rather than the common qcom,icc.h. The bit range used for them >>> could be common, but each target may only support a small subset of the >>> total set of possible voters across all targets. >> I'm not sure how client drivers would then choose the >> correct path other than >> >> switch (soc) { >> case 8450: >> tag = QCOM_ICC_TAG_VOTER_8450_HLOS; >> break; >> case 8550: >> tag = QCOM_ICC_TAG_VOTER_8550_HLOS; >> break; >> ... >> } >> >> which would be unacceptable. > > The same general way it's handled for the endpoint bindings, which are > already target-specific. > > Any client drivers hardcoding the endpoint bindings in their driver > would have to include the appropriate, target-specific binding header > (e.g. qcom,sm8550-rpmh.h). That would only be possible if their driver > file is itself target-specific. Otherwise, it would have to pull the > endpoint bindings from devicetree. Or just use the recommended > of_icc_get() and let devicetree do everything for them. Same for the > target-specific voter tag bindings. > > Clients can also specify their tags in devicetree. They don't actually > have to call icc_set_tag() directly. For example: > > #include <dt-bindings/interconnect/qcom,sm8450.h> > > interconnects = <&mmss_noc MASTER_MDP QCOM_ICC_TAG_VOTER_DISP > &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_VOTER_DISP>; > > Then when they call of_icc_get() for this path it'll automatically have > QCOM_ICC_TAG_VOTER_DISP set for them. I think I'd skew towards the "define everything in the DT" approach. One thing that makes me uneasy to go on with this approach is the question whether there is a case in which we would want to switch from e.g. voting through DISP to voting through APPS (or similar) from within a single device. Konrad
On Fri, Sep 15, 2023 at 03:43:27PM +0200, Konrad Dybcio wrote: > On 14.09.2023 04:32, Mike Tipton wrote: > > On Wed, Sep 13, 2023 at 10:31:49AM +0200, Konrad Dybcio wrote: > >>> The applicable voters should likely be defined in the target-specific > >>> headers, rather than the common qcom,icc.h. The bit range used for them > >>> could be common, but each target may only support a small subset of the > >>> total set of possible voters across all targets. > >> I'm not sure how client drivers would then choose the > >> correct path other than > >> > >> switch (soc) { > >> case 8450: > >> tag = QCOM_ICC_TAG_VOTER_8450_HLOS; > >> break; > >> case 8550: > >> tag = QCOM_ICC_TAG_VOTER_8550_HLOS; > >> break; > >> ... > >> } > >> > >> which would be unacceptable. > > > > The same general way it's handled for the endpoint bindings, which are > > already target-specific. > > > > Any client drivers hardcoding the endpoint bindings in their driver > > would have to include the appropriate, target-specific binding header > > (e.g. qcom,sm8550-rpmh.h). That would only be possible if their driver > > file is itself target-specific. Otherwise, it would have to pull the > > endpoint bindings from devicetree. Or just use the recommended > > of_icc_get() and let devicetree do everything for them. Same for the > > target-specific voter tag bindings. > > > > Clients can also specify their tags in devicetree. They don't actually > > have to call icc_set_tag() directly. For example: > > > > #include <dt-bindings/interconnect/qcom,sm8450.h> > > > > interconnects = <&mmss_noc MASTER_MDP QCOM_ICC_TAG_VOTER_DISP > > &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_VOTER_DISP>; > > > > Then when they call of_icc_get() for this path it'll automatically have > > QCOM_ICC_TAG_VOTER_DISP set for them. > I think I'd skew towards the "define everything in the DT" approach. > > One thing that makes me uneasy to go on with this approach is the > question whether there is a case in which we would want to switch > from e.g. voting through DISP to voting through APPS (or similar) > from within a single device. It shouldn't be common. But it could be done fairly simply by listing paths for each different voter in the dt properties. interconnects = <&mmss_noc MASTER_MDP QCOM_ICC_TAG_VOTER_APPS &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_VOTER_APPS>, <&mmss_noc MASTER_MDP QCOM_ICC_TAG_VOTER_DISP &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_VOTER_DISP>, interconnect-names = "path-apps-voter", "path-disp-voter";
On 15.09.2023 18:05, Mike Tipton wrote: > On Fri, Sep 15, 2023 at 03:43:27PM +0200, Konrad Dybcio wrote: >> On 14.09.2023 04:32, Mike Tipton wrote: >>> On Wed, Sep 13, 2023 at 10:31:49AM +0200, Konrad Dybcio wrote: >>>>> The applicable voters should likely be defined in the target-specific >>>>> headers, rather than the common qcom,icc.h. The bit range used for them >>>>> could be common, but each target may only support a small subset of the >>>>> total set of possible voters across all targets. >>>> I'm not sure how client drivers would then choose the >>>> correct path other than >>>> >>>> switch (soc) { >>>> case 8450: >>>> tag = QCOM_ICC_TAG_VOTER_8450_HLOS; >>>> break; >>>> case 8550: >>>> tag = QCOM_ICC_TAG_VOTER_8550_HLOS; >>>> break; >>>> ... >>>> } >>>> >>>> which would be unacceptable. >>> >>> The same general way it's handled for the endpoint bindings, which are >>> already target-specific. >>> >>> Any client drivers hardcoding the endpoint bindings in their driver >>> would have to include the appropriate, target-specific binding header >>> (e.g. qcom,sm8550-rpmh.h). That would only be possible if their driver >>> file is itself target-specific. Otherwise, it would have to pull the >>> endpoint bindings from devicetree. Or just use the recommended >>> of_icc_get() and let devicetree do everything for them. Same for the >>> target-specific voter tag bindings. >>> >>> Clients can also specify their tags in devicetree. They don't actually >>> have to call icc_set_tag() directly. For example: >>> >>> #include <dt-bindings/interconnect/qcom,sm8450.h> >>> >>> interconnects = <&mmss_noc MASTER_MDP QCOM_ICC_TAG_VOTER_DISP >>> &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_VOTER_DISP>; >>> >>> Then when they call of_icc_get() for this path it'll automatically have >>> QCOM_ICC_TAG_VOTER_DISP set for them. >> I think I'd skew towards the "define everything in the DT" approach. >> >> One thing that makes me uneasy to go on with this approach is the >> question whether there is a case in which we would want to switch >> from e.g. voting through DISP to voting through APPS (or similar) >> from within a single device. > > It shouldn't be common. But it could be done fairly simply by listing > paths for each different voter in the dt properties. > > interconnects = <&mmss_noc MASTER_MDP QCOM_ICC_TAG_VOTER_APPS > &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_VOTER_APPS>, > <&mmss_noc MASTER_MDP QCOM_ICC_TAG_VOTER_DISP > &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_VOTER_DISP>, > interconnect-names = "path-apps-voter", > "path-disp-voter"; Eeeeeh, I don't know.. this almost sounds like a patch-up solution to a problem that doesn't quite yet exist. I debated introducing a third interconnect cell for this, but I am not sure the added complexity is worth it. Having a global set of RSC-bound tags would be a "nice" and tidy solution.. Maybe we could even allocate like 24 bits to these, as I don't think you'll be introducing new buckets (or at least I hope you won't!). 24 is an obscene amount of RSCs to have, even counting virtual channels, so unless you folks have some dark plans to make all pieces of hardware powered completely separately from each other, I suppose I could ask for a pinky-promise to not exceed that number, ever :D Konrad
Many parts of Qualcomm SoCs are entirely independent of each other and can run when the other parts are off. The RPMh system architecture embraces this by giving each (loosely defined) subsystem its own connection (as in, physical wires) to the AOSS, terminated by per-subsystem RSCs (Resource State Coordinators) that barter for power, bandwidth etc. This series introduces the groundwork necessary for voting for resources through non-APPS RSCs. It should allow for lower-latency vote adjustments (e.g. for very high bandwidth / multiple displays) and could potentially allow for full APSS collapse while keeping e.g. MDSS operating (say refreshing an image from a RAM buffer). On top of that, a rather necessary and overdue cleanup is performed to stop adding more and more arguments to the insane preprocessor macros. Partially reverting (or reimplementing the revert) [1] will be necessary to coordinate the rather complex relationship between the DPU and RSC drivers. The "Point x paths to the x RSC" patches won't do anything (check the compatibility workaround qcom_icc_pre_aggregate()) until disp_rsc is properly described in the device tree, along with its BCM voter), but they prepare ground for when that happens. I was able to test sending requests through the DISP_RSC on SM8450, but I had to hack its clocks (_rscc_ in dispcc) to be always-on, as we don't have any clk handling for qcom,rpmh-rsc today. Boot-tested on SM8350 and SM8450, nothing exploded. [1] https://patchwork.kernel.org/project/dri-devel/patch/1521827074-28424-1-git-send-email-ryadav@codeaurora.org/ Dependencies: [2] https://lore.kernel.org/linux-arm-msm/113b50f8-35f6-73fc-4fc9-302262927c5e@quicinc.com/ [3] https://lore.kernel.org/linux-arm-msm/20230703-topic-8250_qup_icc-v2-0-9ba0a9460be2@linaro.org/ Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- Konrad Dybcio (53): dt-bindings: interconnect: qcom,icc: Introduce fixed BCM voter indices dt-bindings: interconnect: qcom,bcm-voter: Add qcom,bcm-voter-idx interconnect: qcom: icc-rpmh: Store direct BCM voter references interconnect: qcom: icc-rpmh: Retire dead code interconnect: qcom: icc-rpmh: Implement voting on non-APPS RSCs interconnect: qcom: sc7180: Retire DEFINE_QNODE interconnect: qcom: sdm670: Retire DEFINE_QNODE interconnect: qcom: sdm845: Retire DEFINE_QNODE interconnect: qcom: sdx55: Retire DEFINE_QNODE interconnect: qcom: sdx65: Retire DEFINE_QNODE interconnect: qcom: sm6350: Retire DEFINE_QNODE interconnect: qcom: sm8150: Retire DEFINE_QNODE interconnect: qcom: sm8250: Retire DEFINE_QNODE interconnect: qcom: sm8350: Retire DEFINE_QNODE interconnect: qcom: icc-rpmh: Retire DEFINE_QNODE interconnect: qcom: sc7180: Retire DEFINE_QBCM interconnect: qcom: sdm670: Retire DEFINE_QBCM interconnect: qcom: sdm845: Retire DEFINE_QBCM interconnect: qcom: sdx55: Retire DEFINE_QBCM interconnect: qcom: sdx65: Retire DEFINE_QBCM interconnect: qcom: sm6350: Retire DEFINE_QBCM interconnect: qcom: sm8150: Retire DEFINE_QBCM interconnect: qcom: sm8250: Retire DEFINE_QBCM interconnect: qcom: sm8350: Retire DEFINE_QBCM interconnect: qcom: icc-rpmh: Retire DEFINE_QBCM interconnect: qcom: qdu1000: Explicitly assign voter_idx interconnect: qcom: sa8775p: Explicitly assign voter_idx interconnect: qcom: sc7280: Explicitly assign voter_idx interconnect: qcom: sc8180x: Explicitly assign voter_idx interconnect: qcom: sc8280xp: Explicitly assign voter_idx interconnect: qcom: sm8450: Explicitly assign voter_idx interconnect: qcom: sm8550: Explicitly assign voter_idx arm64: dts: qcom: qdu1000: add qcom,bcm-voter-idx arm64: dts: qcom: sa8775p: add qcom,bcm-voter-idx arm64: dts: qcom: sc7180: add qcom,bcm-voter-idx arm64: dts: qcom: sc7280: add qcom,bcm-voter-idx arm64: dts: qcom: sc8180x: add qcom,bcm-voter-idx arm64: dts: qcom: sc8280xp: add qcom,bcm-voter-idx arm64: dts: qcom: sdm670: add qcom,bcm-voter-idx arm64: dts: qcom: sdm845: add qcom,bcm-voter-idx arm64: dts: qcom: sdx75: add qcom,bcm-voter-idx arm64: dts: qcom: sm6350: add qcom,bcm-voter-idx arm64: dts: qcom: sm8150: add qcom,bcm-voter-idx arm64: dts: qcom: sm8250: add qcom,bcm-voter-idx arm64: dts: qcom: sm8350: add qcom,bcm-voter-idx arm64: dts: qcom: sm8450: add qcom,bcm-voter-idx arm64: dts: qcom: sm8550: add qcom,bcm-voter-idx arm64: dts: qcom: sdx55: add qcom,bcm-voter-idx arm64: dts: qcom: sdx65: add qcom,bcm-voter-idx interconnect: qcom: sm8350: Point display paths to the display RSC interconnect: qcom: sm8450: Point display paths to the display RSC interconnect: qcom: sm8550: Point display paths to the display RSC interconnect: qcom: sm8550: Point camera paths to the camera RSC .../bindings/interconnect/qcom,bcm-voter.yaml | 10 + arch/arm/boot/dts/qcom/qcom-sdx55.dtsi | 1 + arch/arm/boot/dts/qcom/qcom-sdx65.dtsi | 1 + arch/arm64/boot/dts/qcom/qdu1000.dtsi | 2 + arch/arm64/boot/dts/qcom/sa8775p.dtsi | 1 + arch/arm64/boot/dts/qcom/sc7180.dtsi | 1 + arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 + arch/arm64/boot/dts/qcom/sc8180x.dtsi | 2 + arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 2 + arch/arm64/boot/dts/qcom/sdm670.dtsi | 2 + arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 + arch/arm64/boot/dts/qcom/sdx75.dtsi | 2 + arch/arm64/boot/dts/qcom/sm6350.dtsi | 1 + arch/arm64/boot/dts/qcom/sm8150.dtsi | 2 + arch/arm64/boot/dts/qcom/sm8250.dtsi | 2 + arch/arm64/boot/dts/qcom/sm8350.dtsi | 2 + arch/arm64/boot/dts/qcom/sm8450.dtsi | 2 + arch/arm64/boot/dts/qcom/sm8550.dtsi | 2 + drivers/interconnect/qcom/bcm-voter.c | 75 +- drivers/interconnect/qcom/bcm-voter.h | 9 - drivers/interconnect/qcom/icc-rpmh.c | 53 +- drivers/interconnect/qcom/icc-rpmh.h | 14 +- drivers/interconnect/qcom/qdu1000.c | 11 + drivers/interconnect/qcom/sa8775p.c | 28 + drivers/interconnect/qcom/sc7180.c | 1637 +++++++++++++++-- drivers/interconnect/qcom/sc7280.c | 27 + drivers/interconnect/qcom/sc8180x.c | 23 + drivers/interconnect/qcom/sc8280xp.c | 27 + drivers/interconnect/qcom/sdm670.c | 1410 +++++++++++++-- drivers/interconnect/qcom/sdm845.c | 1683 ++++++++++++++++-- drivers/interconnect/qcom/sdx55.c | 863 ++++++++- drivers/interconnect/qcom/sdx65.c | 850 ++++++++- drivers/interconnect/qcom/sm6350.c | 1551 +++++++++++++++-- drivers/interconnect/qcom/sm8150.c | 1714 ++++++++++++++++-- drivers/interconnect/qcom/sm8250.c | 1772 +++++++++++++++++-- drivers/interconnect/qcom/sm8350.c | 1830 ++++++++++++++++++-- drivers/interconnect/qcom/sm8450.c | 24 + drivers/interconnect/qcom/sm8550.c | 42 + include/dt-bindings/interconnect/qcom,icc.h | 8 + 39 files changed, 12320 insertions(+), 1370 deletions(-) --- base-commit: 82cee168d497ffcb79e4889fe3c7384788e89f4d change-id: 20230708-topic-rpmh_icc_rsc-f897080fb207 Best regards,