[00/53] icc-rpmh multi-RSC voting groundwork

  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,
  

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 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