[PATCH v2 0/4] soc: qcom: icc-bwmon: Add support for llcc and cpu bwmon on sc7280

[PATCH v2 0/4] soc: qcom: icc-bwmon: Add support for llcc and cpu bwmon on sc7280

[Rajendra Nayak]

Changes in v2:
Patch 1/1: rearranged compatibles in alphabetical order, no changes in rest of the patches

This patchset adds support for cpu bwmon (bwmon4) and llcc bwmon (bwmon5) found
on sc7280 SoC.

Patchset is based on top of series [1] that adds support for llcc bwmon on sdm845

[1]
https://patchwork.kernel.org/project/linux-arm-msm/list/?series=663695&state=*

Rajendra Nayak (4):
  dt-bindings: interconnect: qcom,msm8998-bwmon: Add support for sc7280
    BWMONs
  soc: qcom: icc-bwmon: add support for sc7280 LLCC BWMON
  soc: qcom: icc-bwmon: force clear counter/irq registers
  arm64: dts: qcom: sc7280: Add cpu and llcc BWMON

 .../interconnect/qcom,msm8998-bwmon.yaml      |  2 +
 arch/arm64/boot/dts/qcom/sc7280.dtsi          | 76 +++++++++++++++++++
 drivers/soc/qcom/icc-bwmon.c                  | 21 +++++
 3 files changed, 99 insertions(+)

-- 
2.17.1

[PATCH v2 1/4] dt-bindings: interconnect: qcom,msm8998-bwmon: Add support for sc7280 BWMONs

[Rajendra Nayak]

Add a compatible for the cpu BWMON (version 4) instance and one
for the llcc BWMON (version 5) found in sc7280 SoC.

Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
v2: rearranged compatibles in alphabetical order

 .../devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml    | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml b/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml
index 32e2892d736b..0ac5256876a8 100644
--- a/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml
+++ b/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml
@@ -24,9 +24,11 @@ properties:
     oneOf:
       - items:
           - enum:
+              - qcom,sc7280-bwmon
               - qcom,sdm845-bwmon
           - const: qcom,msm8998-bwmon
       - const: qcom,msm8998-bwmon       # BWMON v4
+      - const: qcom,sc7280-llcc-bwmon   # BWMON v5
       - const: qcom,sdm845-llcc-bwmon   # BWMON v5
 
   interconnects:
-- 
2.17.1

[PATCH v2 2/4] soc: qcom: icc-bwmon: add support for sc7280 LLCC BWMON

[Rajendra Nayak]

Add support for sc7280 BWMON instance measuring traffic between LLCC and
memory with the v5 register layout.

Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
v2: no change

 drivers/soc/qcom/icc-bwmon.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
index 47c2c3e7bb3f..44a10009b45e 100644
--- a/drivers/soc/qcom/icc-bwmon.c
+++ b/drivers/soc/qcom/icc-bwmon.c
@@ -656,6 +656,18 @@ static const struct icc_bwmon_data sdm845_llcc_bwmon_data = {
 	.regmap_cfg = &sdm845_llcc_bwmon_regmap_cfg,
 };
 
+static const struct icc_bwmon_data sc7280_llcc_bwmon_data = {
+	.sample_ms = 4,
+	.count_unit_kb = 64,
+	.default_highbw_kbps = 800 * 1024, /* 800 MBps */
+	.default_medbw_kbps = 256 * 1024, /* 256 MBps */
+	.default_lowbw_kbps = 0,
+	.zone1_thres_count = 16,
+	.zone3_thres_count = 1,
+	.regmap_fields = sdm845_llcc_bwmon_reg_fields,
+	.regmap_cfg = &sdm845_llcc_bwmon_regmap_cfg,
+};
+
 static const struct of_device_id bwmon_of_match[] = {
 	{
 		.compatible = "qcom,msm8998-bwmon",
@@ -663,6 +675,9 @@ static const struct of_device_id bwmon_of_match[] = {
 	}, {
 		.compatible = "qcom,sdm845-llcc-bwmon",
 		.data = &sdm845_llcc_bwmon_data
+	}, {
+		.compatible = "qcom,sc7280-llcc-bwmon",
+		.data = &sc7280_llcc_bwmon_data
 	},
 	{}
 };
-- 
2.17.1

[PATCH v2 3/4] soc: qcom: icc-bwmon: force clear counter/irq registers

[Rajendra Nayak]

In some SoCs we have to force clear the counter/irq clear registers as
they are not self clearing after they are written into.
sc7280 seems to be one such SoC, handle this with a quirk flag.

Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
v2: no change

 drivers/soc/qcom/icc-bwmon.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
index 44a10009b45e..17cba2648ae7 100644
--- a/drivers/soc/qcom/icc-bwmon.c
+++ b/drivers/soc/qcom/icc-bwmon.c
@@ -115,6 +115,7 @@
 
 /* Quirks for specific BWMON types */
 #define BWMON_HAS_GLOBAL_IRQ			BIT(0)
+#define BWMON_NEEDS_FORCE_CLEAR			BIT(1)
 
 enum bwmon_fields {
 	F_GLOBAL_IRQ_CLEAR,
@@ -343,6 +344,8 @@ static void bwmon_clear_counters(struct icc_bwmon *bwmon, bool clear_all)
 	 * before we try to clear the IRQ or do any other counter operations.
 	 */
 	regmap_field_force_write(bwmon->regs[F_CLEAR], val);
+	if (bwmon->data->quirks & BWMON_NEEDS_FORCE_CLEAR)
+		regmap_field_force_write(bwmon->regs[F_CLEAR], 0);
 }
 
 static void bwmon_clear_irq(struct icc_bwmon *bwmon)
@@ -364,6 +367,8 @@ static void bwmon_clear_irq(struct icc_bwmon *bwmon)
 	 * interrupt is cleared.
 	 */
 	regmap_field_force_write(bwmon->regs[F_IRQ_CLEAR], BWMON_IRQ_ENABLE_MASK);
+	if (bwmon->data->quirks & BWMON_NEEDS_FORCE_CLEAR)
+		regmap_field_force_write(bwmon->regs[F_IRQ_CLEAR], 0);
 	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
 		regmap_field_force_write(bwmon->regs[F_GLOBAL_IRQ_CLEAR],
 					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
@@ -664,6 +669,7 @@ static const struct icc_bwmon_data sc7280_llcc_bwmon_data = {
 	.default_lowbw_kbps = 0,
 	.zone1_thres_count = 16,
 	.zone3_thres_count = 1,
+	.quirks = BWMON_NEEDS_FORCE_CLEAR,
 	.regmap_fields = sdm845_llcc_bwmon_reg_fields,
 	.regmap_cfg = &sdm845_llcc_bwmon_regmap_cfg,
 };
-- 
2.17.1

[PATCH v2 4/4] arm64: dts: qcom: sc7280: Add cpu and llcc BWMON

[Rajendra Nayak]

Add cpu and llcc BWMON nodes and their corresponding
OPP tables for sc7280 SoC.

Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
v2: no change

 arch/arm64/boot/dts/qcom/sc7280.dtsi | 76 ++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 13d7f267b289..a839ba968d13 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -3275,6 +3275,82 @@
 			};
 		};
 
+		pmu@9091000 {
+			compatible = "qcom,sc7280-llcc-bwmon";
+			reg = <0 0x9091000 0 0x1000>;
+
+			interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>;
+
+			interconnects = <&mc_virt MASTER_LLCC 3 &mc_virt SLAVE_EBI1 3>;
+
+			operating-points-v2 = <&llcc_bwmon_opp_table>;
+
+			llcc_bwmon_opp_table: opp-table {
+				compatible = "operating-points-v2";
+
+				opp-0 {
+					opp-peak-kBps = <800000>;
+				};
+				opp-1 {
+					opp-peak-kBps = <1804000>;
+				};
+				opp-2 {
+					opp-peak-kBps = <2188000>;
+				};
+				opp-3 {
+					opp-peak-kBps = <3072000>;
+				};
+				opp-4 {
+					opp-peak-kBps = <4068000>;
+				};
+				opp-5 {
+					opp-peak-kBps = <6220000>;
+				};
+				opp-6 {
+					opp-peak-kBps = <6832000>;
+				};
+				opp-7 {
+					opp-peak-kBps = <8532000>;
+				};
+			};
+		};
+
+		pmu@90b6000 {
+			compatible = "qcom,sc7280-cpu-bwmon", "qcom,msm8998-bwmon";
+			reg = <0 0x090b6400 0 0x600>;
+
+			interrupts = <GIC_SPI 581 IRQ_TYPE_LEVEL_HIGH>;
+
+			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &gem_noc SLAVE_LLCC 3>;
+			operating-points-v2 = <&cpu_bwmon_opp_table>;
+
+			cpu_bwmon_opp_table: opp-table {
+				compatible = "operating-points-v2";
+
+				opp-0 {
+					opp-peak-kBps = <2400000>;
+				};
+				opp-1 {
+					opp-peak-kBps = <4800000>;
+				};
+				opp-2 {
+					opp-peak-kBps = <7456000>;
+				};
+				opp-3 {
+					opp-peak-kBps = <9600000>;
+				};
+				opp-4 {
+					opp-peak-kBps = <12896000>;
+				};
+				opp-5 {
+					opp-peak-kBps = <14928000>;
+				};
+				opp-6 {
+					opp-peak-kBps = <17056000>;
+				};
+			};
+		};
+
 		dc_noc: interconnect@90e0000 {
 			reg = <0 0x090e0000 0 0x5080>;
 			compatible = "qcom,sc7280-dc-noc";
-- 
2.17.1

Re: (subset) [PATCH v2 0/4] soc: qcom: icc-bwmon: Add support for llcc and cpu bwmon on sc7280

[Bjorn Andersson]

On Fri, 2 Sep 2022 10:05:07 +0530, Rajendra Nayak wrote:
> Changes in v2:
> Patch 1/1: rearranged compatibles in alphabetical order, no changes in rest of the patches
> 
> This patchset adds support for cpu bwmon (bwmon4) and llcc bwmon (bwmon5) found
> on sc7280 SoC.
> 
> Patchset is based on top of series [1] that adds support for llcc bwmon on sdm845
> 
> [...]

Applied, thanks!

[4/4] arm64: dts: qcom: sc7280: Add cpu and llcc BWMON
      commit: b2f3eac1b77c6feba4daff83de9436fcf728a5e5

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

Re: [PATCH v2 4/4] arm64: dts: qcom: sc7280: Add cpu and llcc BWMON

[Stephen Boyd]

Quoting Rajendra Nayak (2022-09-01 21:35:11)
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 13d7f267b289..a839ba968d13 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -3275,6 +3275,82 @@
>                         };
>                 };
>
> +               pmu@9091000 {
> +                       compatible = "qcom,sc7280-llcc-bwmon";
> +                       reg = <0 0x9091000 0 0x1000>;
> +
[...]
> +                       };
> +               };
> +
> +               pmu@90b6000 {

This unit address

> +                       compatible = "qcom,sc7280-cpu-bwmon", "qcom,msm8998-bwmon";
> +                       reg = <0 0x090b6400 0 0x600>;

doesn't match this one. Please fix.

Re: [PATCH v2 4/4] arm64: dts: qcom: sc7280: Add cpu and llcc BWMON

[Krzysztof Kozlowski]

On 07/09/2022 23:33, Stephen Boyd wrote:
> Quoting Rajendra Nayak (2022-09-01 21:35:11)
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index 13d7f267b289..a839ba968d13 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -3275,6 +3275,82 @@
>>                         };
>>                 };
>>
>> +               pmu@9091000 {
>> +                       compatible = "qcom,sc7280-llcc-bwmon";
>> +                       reg = <0 0x9091000 0 0x1000>;
>> +
> [...]
>> +                       };
>> +               };
>> +
>> +               pmu@90b6000 {
> 
> This unit address
> 
>> +                       compatible = "qcom,sc7280-cpu-bwmon", "qcom,msm8998-bwmon";
>> +                       reg = <0 0x090b6400 0 0x600>;
> 
> doesn't match this one. Please fix.

Thanks for catching it. Patch was applied, so I will send a follow up.

Best regards,
Krzysztof

Re: [PATCH v2 4/4] arm64: dts: qcom: sc7280: Add cpu and llcc BWMON (=> interconnect issue)

[Matthias Kaehlcke]

Hi,

On Fri, Sep 02, 2022 at 10:05:11AM +0530, Rajendra Nayak wrote:
> Add cpu and llcc BWMON nodes and their corresponding
> OPP tables for sc7280 SoC.
> 
> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

I found that with a v6.1 kernel AOSS on sc7280 doesn't reach it's low
power state during system. This can be observed on herobrine based
boards on which the AP_SUSPEND signal should transition to 1 during
system suspend. If it doesn't the Embedded Controller (EC) notices
it and wakes the system up again.

Bisection points to this patch, the issue only occurs when
CONFIG_QCOM_ICC_BWMON is *not* set. One might think the patch shouldn't
have any impact at all when the driver is not enabled, but it does.

Debugging shows that the issue is interconnect related. A bare platform
device is created for each bwmon devices, which results in the average
and peak bandwidth of the interconnect link to be set 'initially' to
INT_MAX. The driver is supposed to call icc_sync_state() during probe,
which would set the initially bandwidths to 0 and determine the actually
needed bandwidth. But since the driver isn't probed the initial
bandwidths stay at INT_MAX.

This isn't actually an issue with this patch, but how the interconnect
framework deals with devices that are registered on the bus, but aren't
probed (yet). Not sure how this would be best fixed. Georgi, do you have
any ideas?

Thanks

Matthias

Re: [PATCH v2 4/4] arm64: dts: qcom: sc7280: Add cpu and llcc BWMON (=> interconnect issue)

[Krzysztof Kozlowski]

On 13/01/2023 19:18, Matthias Kaehlcke wrote:
> Hi,
> 
> On Fri, Sep 02, 2022 at 10:05:11AM +0530, Rajendra Nayak wrote:
>> Add cpu and llcc BWMON nodes and their corresponding
>> OPP tables for sc7280 SoC.
>>
>> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> I found that with a v6.1 kernel AOSS on sc7280 doesn't reach it's low
> power state during system. This can be observed on herobrine based
> boards on which the AP_SUSPEND signal should transition to 1 during
> system suspend. If it doesn't the Embedded Controller (EC) notices
> it and wakes the system up again.
> 
> Bisection points to this patch, the issue only occurs when
> CONFIG_QCOM_ICC_BWMON is *not* set. One might think the patch shouldn't
> have any impact at all when the driver is not enabled, but it does.
> 
> Debugging shows that the issue is interconnect related. A bare platform
> device is created for each bwmon devices, which results in the average
> and peak bandwidth of the interconnect link to be set 'initially' to
> INT_MAX. The driver is supposed to call icc_sync_state() during probe,

This is for interconnect providers, not consumers.

> which would set the initially bandwidths to 0 and determine the actually
> needed bandwidth. But since the driver isn't probed the initial
> bandwidths stay at INT_MAX.
> 
> This isn't actually an issue with this patch, but how the interconnect
> framework deals with devices that are registered on the bus, but aren't
> probed (yet). Not sure how this would be best fixed. Georgi, do you have
> any ideas?

Why the device is not probed (yet)? If it is registered, it will come
soon during boot up.

Best regards,
Krzysztof

Re: [PATCH v2 4/4] arm64: dts: qcom: sc7280: Add cpu and llcc BWMON (=> interconnect issue)

[Matthias Kaehlcke]

On Sun, Jan 15, 2023 at 04:13:40PM +0100, Krzysztof Kozlowski wrote:
> On 13/01/2023 19:18, Matthias Kaehlcke wrote:
> > Hi,
> > 
> > On Fri, Sep 02, 2022 at 10:05:11AM +0530, Rajendra Nayak wrote:
> >> Add cpu and llcc BWMON nodes and their corresponding
> >> OPP tables for sc7280 SoC.
> >>
> >> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
> >> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > 
> > I found that with a v6.1 kernel AOSS on sc7280 doesn't reach it's low
> > power state during system. This can be observed on herobrine based
> > boards on which the AP_SUSPEND signal should transition to 1 during
> > system suspend. If it doesn't the Embedded Controller (EC) notices
> > it and wakes the system up again.
> > 
> > Bisection points to this patch, the issue only occurs when
> > CONFIG_QCOM_ICC_BWMON is *not* set. One might think the patch shouldn't
> > have any impact at all when the driver is not enabled, but it does.
> > 
> > Debugging shows that the issue is interconnect related. A bare platform
> > device is created for each bwmon devices, which results in the average
> > and peak bandwidth of the interconnect link to be set 'initially' to
> > INT_MAX. The driver is supposed to call icc_sync_state() during probe,
> 
> This is for interconnect providers, not consumers.

Ah, thanks for the clarification.

Still, for the INT_MAX bandwidth setting remains in place unless the device
is probed.

> > which would set the initially bandwidths to 0 and determine the actually
> > needed bandwidth. But since the driver isn't probed the initial
> > bandwidths stay at INT_MAX.
> > 
> > This isn't actually an issue with this patch, but how the interconnect
> > framework deals with devices that are registered on the bus, but aren't
> > probed (yet). Not sure how this would be best fixed. Georgi, do you have
> > any ideas?
> 
> Why the device is not probed (yet)? If it is registered, it will come
> soon during boot up.

Because CONFIG_QCOM_ICC_BWMON is not enabled for the board in question (see
above). It could be enabled as a short term mitigtion, however we shouldn't
require drivers to be enabled just because the DT has a corresponding node.

Re: [PATCH v2 4/4] arm64: dts: qcom: sc7280: Add cpu and llcc BWMON (=> interconnect issue)

[Krzysztof Kozlowski]

On 17/01/2023 18:27, Matthias Kaehlcke wrote:
> 
>>> which would set the initially bandwidths to 0 and determine the actually
>>> needed bandwidth. But since the driver isn't probed the initial
>>> bandwidths stay at INT_MAX.
>>>
>>> This isn't actually an issue with this patch, but how the interconnect
>>> framework deals with devices that are registered on the bus, but aren't
>>> probed (yet). Not sure how this would be best fixed. Georgi, do you have
>>> any ideas?
>>
>> Why the device is not probed (yet)? If it is registered, it will come
>> soon during boot up.
> 
> Because CONFIG_QCOM_ICC_BWMON is not enabled for the board in question (see
> above). It could be enabled as a short term mitigtion, however we shouldn't
> require drivers to be enabled just because the DT has a corresponding node.

It's the same case as with all other interconnect leafs/consumers. The
same behavior if you do not have it enabled, right? If not, I wonder
what is here different?

Best regards,
Krzysztof

Re: [PATCH v2 4/4] arm64: dts: qcom: sc7280: Add cpu and llcc BWMON (=> interconnect issue)

[Matthias Kaehlcke]

On Tue, Jan 17, 2023 at 06:33:41PM +0100, Krzysztof Kozlowski wrote:
> On 17/01/2023 18:27, Matthias Kaehlcke wrote:
> > 
> >>> which would set the initially bandwidths to 0 and determine the actually
> >>> needed bandwidth. But since the driver isn't probed the initial
> >>> bandwidths stay at INT_MAX.
> >>>
> >>> This isn't actually an issue with this patch, but how the interconnect
> >>> framework deals with devices that are registered on the bus, but aren't
> >>> probed (yet). Not sure how this would be best fixed. Georgi, do you have
> >>> any ideas?
> >>
> >> Why the device is not probed (yet)? If it is registered, it will come
> >> soon during boot up.
> > 
> > Because CONFIG_QCOM_ICC_BWMON is not enabled for the board in question (see
> > above). It could be enabled as a short term mitigtion, however we shouldn't
> > require drivers to be enabled just because the DT has a corresponding node.
> 
> It's the same case as with all other interconnect leafs/consumers. The
> same behavior if you do not have it enabled, right? If not, I wonder
> what is here different?

Right, this is a general issue. The problem on sc7280 (and probably other
Qualcomm SoCs) is that the interconnect link at full throttle prevents the
SoC from entering its low power mode (AOSS sleep) during system suspend.
On many boards this might go unnoticed, on herobrine the condition is
detected by the embedded controller (EC) and considered a failed suspend,
which results in waking up the system.

Re: [PATCH v2 4/4] arm64: dts: qcom: sc7280: Add cpu and llcc BWMON (=> interconnect issue)

[Matthias Kaehlcke]

On Tue, Jan 17, 2023 at 05:47:14PM +0000, Matthias Kaehlcke wrote:
> On Tue, Jan 17, 2023 at 06:33:41PM +0100, Krzysztof Kozlowski wrote:
> > On 17/01/2023 18:27, Matthias Kaehlcke wrote:
> > > 
> > >>> which would set the initially bandwidths to 0 and determine the actually
> > >>> needed bandwidth. But since the driver isn't probed the initial
> > >>> bandwidths stay at INT_MAX.
> > >>>
> > >>> This isn't actually an issue with this patch, but how the interconnect
> > >>> framework deals with devices that are registered on the bus, but aren't
> > >>> probed (yet). Not sure how this would be best fixed. Georgi, do you have
> > >>> any ideas?
> > >>
> > >> Why the device is not probed (yet)? If it is registered, it will come
> > >> soon during boot up.
> > > 
> > > Because CONFIG_QCOM_ICC_BWMON is not enabled for the board in question (see
> > > above). It could be enabled as a short term mitigtion, however we shouldn't
> > > require drivers to be enabled just because the DT has a corresponding node.
> > 
> > It's the same case as with all other interconnect leafs/consumers. The
> > same behavior if you do not have it enabled, right? If not, I wonder
> > what is here different?
> 
> Right, this is a general issue. The problem on sc7280 (and probably other
> Qualcomm SoCs) is that the interconnect link at full throttle prevents the
> SoC from entering its low power mode (AOSS sleep) during system suspend.
> On many boards this might go unnoticed, on herobrine the condition is
> detected by the embedded controller (EC) and considered a failed suspend,
> which results in waking up the system.

I did some hackery to convince the EC to enter/stay in S3, despite AOSS
no entering sleep mode. That allowed me to take power measurements. With
the kernel suspended but the AOSS remaining on the power consumption of
the Qcard is more than 7x higher than when the AOSS enters sleep mode!
On a Qcard system I can't break the power consumption further down, but
I think the extra power consumption must be coming mostly from the SoC
itself, since the kernel and the EC are essentially in the same state as
during a suspend with AOSS in sleep mode.

The enormous increase in power consumption suggests that this is a serious
issue for non-Chrome OS platforms as well. On herobrine and trogdor boards
we have the 'luxury' of being able to detect that AOSS stays awake (though
it comes with the caveat that the system can't suspend when that happens).
On other boards this goes likely unnoticed until someone measures suspend
power or notices a significant regression in S3 battery life.

It seems something needs to be done at the interconnect core to fix this.
Is it really necessary to init all link to max bandwidth? Maybe it is
needed for certain links, but not all of them? What is the background
here?

Re: [PATCH v2 4/4] arm64: dts: qcom: sc7280: Add cpu and llcc BWMON (=> interconnect issue)

[Georgi Djakov]

Hi Matthias,

On 20.01.23 23:32, Matthias Kaehlcke wrote:
> On Tue, Jan 17, 2023 at 05:47:14PM +0000, Matthias Kaehlcke wrote:
>> On Tue, Jan 17, 2023 at 06:33:41PM +0100, Krzysztof Kozlowski wrote:
>>> On 17/01/2023 18:27, Matthias Kaehlcke wrote:
>>>>
>>>>>> which would set the initially bandwidths to 0 and determine the actually
>>>>>> needed bandwidth. But since the driver isn't probed the initial
>>>>>> bandwidths stay at INT_MAX.
>>>>>>
>>>>>> This isn't actually an issue with this patch, but how the interconnect
>>>>>> framework deals with devices that are registered on the bus, but aren't
>>>>>> probed (yet). Not sure how this would be best fixed. Georgi, do you have
>>>>>> any ideas?
>>>>>
>>>>> Why the device is not probed (yet)? If it is registered, it will come
>>>>> soon during boot up.
>>>>
>>>> Because CONFIG_QCOM_ICC_BWMON is not enabled for the board in question (see
>>>> above). It could be enabled as a short term mitigtion, however we shouldn't
>>>> require drivers to be enabled just because the DT has a corresponding node.
>>>
>>> It's the same case as with all other interconnect leafs/consumers. The
>>> same behavior if you do not have it enabled, right? If not, I wonder
>>> what is here different?
>>
>> Right, this is a general issue. The problem on sc7280 (and probably other
>> Qualcomm SoCs) is that the interconnect link at full throttle prevents the
>> SoC from entering its low power mode (AOSS sleep) during system suspend.
>> On many boards this might go unnoticed, on herobrine the condition is
>> detected by the embedded controller (EC) and considered a failed suspend,
>> which results in waking up the system.
> 
> I did some hackery to convince the EC to enter/stay in S3, despite AOSS
> no entering sleep mode. That allowed me to take power measurements. With
> the kernel suspended but the AOSS remaining on the power consumption of
> the Qcard is more than 7x higher than when the AOSS enters sleep mode!
> On a Qcard system I can't break the power consumption further down, but
> I think the extra power consumption must be coming mostly from the SoC
> itself, since the kernel and the EC are essentially in the same state as
> during a suspend with AOSS in sleep mode.
> 
> The enormous increase in power consumption suggests that this is a serious
> issue for non-Chrome OS platforms as well. On herobrine and trogdor boards
> we have the 'luxury' of being able to detect that AOSS stays awake (though
> it comes with the caveat that the system can't suspend when that happens).
> On other boards this goes likely unnoticed until someone measures suspend
> power or notices a significant regression in S3 battery life.
> 
> It seems something needs to be done at the interconnect core to fix this.
> Is it really necessary to init all link to max bandwidth? Maybe it is
> needed for certain links, but not all of them? What is the background
> here?

The basic idea here is to do some initial configuration of the system and
enable the interconnect buses until all consumers have probed. Otherwise
it might disable the bus to some hardware, whose driver (module) is not
loaded yet (and didn't had a chance to express it's bandwidth needs).

The max bandwidth is the default, but we can implement the get_bw() for a
given platform to return the current (or initial) value. It would be best
if we could read this value from the hardware, but as this is not possible
on this board, maybe we can implement get_bw() to return something else.

I guess that you see some int_max values in interconnect_summary for the
ebi and llcc nodes that stay forever?

BR,
Georgi

Re: [PATCH v2 4/4] arm64: dts: qcom: sc7280: Add cpu and llcc BWMON (=> interconnect issue)

[Matthias Kaehlcke]

Hi Georgi,

Thanks for your reply!

On Tue, Jan 24, 2023 at 02:02:36AM +0200, Georgi Djakov wrote:
> Hi Matthias,
> 
> On 20.01.23 23:32, Matthias Kaehlcke wrote:
> > On Tue, Jan 17, 2023 at 05:47:14PM +0000, Matthias Kaehlcke wrote:
> > > On Tue, Jan 17, 2023 at 06:33:41PM +0100, Krzysztof Kozlowski wrote:
> > > > On 17/01/2023 18:27, Matthias Kaehlcke wrote:
> > > > > 
> > > > > > > which would set the initially bandwidths to 0 and determine the actually
> > > > > > > needed bandwidth. But since the driver isn't probed the initial
> > > > > > > bandwidths stay at INT_MAX.
> > > > > > > 
> > > > > > > This isn't actually an issue with this patch, but how the interconnect
> > > > > > > framework deals with devices that are registered on the bus, but aren't
> > > > > > > probed (yet). Not sure how this would be best fixed. Georgi, do you have
> > > > > > > any ideas?
> > > > > > 
> > > > > > Why the device is not probed (yet)? If it is registered, it will come
> > > > > > soon during boot up.
> > > > > 
> > > > > Because CONFIG_QCOM_ICC_BWMON is not enabled for the board in question (see
> > > > > above). It could be enabled as a short term mitigtion, however we shouldn't
> > > > > require drivers to be enabled just because the DT has a corresponding node.
> > > > 
> > > > It's the same case as with all other interconnect leafs/consumers. The
> > > > same behavior if you do not have it enabled, right? If not, I wonder
> > > > what is here different?
> > > 
> > > Right, this is a general issue. The problem on sc7280 (and probably other
> > > Qualcomm SoCs) is that the interconnect link at full throttle prevents the
> > > SoC from entering its low power mode (AOSS sleep) during system suspend.
> > > On many boards this might go unnoticed, on herobrine the condition is
> > > detected by the embedded controller (EC) and considered a failed suspend,
> > > which results in waking up the system.
> > 
> > I did some hackery to convince the EC to enter/stay in S3, despite AOSS
> > no entering sleep mode. That allowed me to take power measurements. With
> > the kernel suspended but the AOSS remaining on the power consumption of
> > the Qcard is more than 7x higher than when the AOSS enters sleep mode!
> > On a Qcard system I can't break the power consumption further down, but
> > I think the extra power consumption must be coming mostly from the SoC
> > itself, since the kernel and the EC are essentially in the same state as
> > during a suspend with AOSS in sleep mode.
> > 
> > The enormous increase in power consumption suggests that this is a serious
> > issue for non-Chrome OS platforms as well. On herobrine and trogdor boards
> > we have the 'luxury' of being able to detect that AOSS stays awake (though
> > it comes with the caveat that the system can't suspend when that happens).
> > On other boards this goes likely unnoticed until someone measures suspend
> > power or notices a significant regression in S3 battery life.
> > 
> > It seems something needs to be done at the interconnect core to fix this.
> > Is it really necessary to init all link to max bandwidth? Maybe it is
> > needed for certain links, but not all of them? What is the background
> > here?
> 
> The basic idea here is to do some initial configuration of the system and
> enable the interconnect buses until all consumers have probed. Otherwise
> it might disable the bus to some hardware, whose driver (module) is not
> loaded yet (and didn't had a chance to express it's bandwidth needs).

I imagine this is an issue for a subset of consumers that are already using
the interconnects before they are probed (like an early-console UART). For
most consumers (like SPI, USB, eMMC, GPU, ...) I'd expect that it should be
fine if the interconnect is disabled until the driver is probed and
specifies the bandwidth requirements.

> The max bandwidth is the default, but we can implement the get_bw() for a
> given platform to return the current (or initial) value. It would be best
> if we could read this value from the hardware, but as this is not possible
> on this board, maybe we can implement get_bw() to return something else.

If my above assumption is correct maybe it would be an option to return
a bandwidth of zero, except for the select links that might be used before
the driver is probed.

> I guess that you see some int_max values in interconnect_summary for the
> ebi and llcc nodes that stay forever?

Exactly:

grep -e llcc_mc -e ebi /sys/kernel/debug/interconnect/interconnect_summary
llcc_mc                                      2148483647   2147483647
ebi                                          2148483647   2147483647