mbox series

[v8,0/4] soc/arm64: qcom: Add initial version of bwmon

Message ID 20220704121730.127925-1-krzysztof.kozlowski@linaro.org
Headers show
Series soc/arm64: qcom: Add initial version of bwmon | expand

Message

Krzysztof Kozlowski July 4, 2022, 12:17 p.m. UTC 
Hi,

Changes since v7
================
1. After discussions with Bjorn and Rajendra, go back to "SoC-bwmon"
   compatible, so without "llcc".  The other bwmon instance - between
   LLCC and DDR - should be called LLCC.

Changes since v6
================
1. Patch #2 (driver): use MSM8998 compatible.

Changes since v5
================
1. Rename compatible (and files) to qcom,msm8998-llcc-bwmon as Rajendra suggested.
   Keep the reviews/acks as the change is not significant.
2. Update comment in DTS, update description in bindings and in Kconfig.

Changes since v4
================
1. Patch #1 (binding): Use qcom,msm8998-cpu-bwmon fallback compatible, only one
   interconnect. Rename to qcom,msm8998-cpu-bwmon.yaml. This reflects
   discussion with Bjorn, about the proper fallback compatible. Driver was
   tested only on SDM845, so only that one compatible is actually implemented.
   Keep the reviews/acks as the change is not significant.
2. Patch #4 (DTS): Use qcom,msm8998-cpu-bwmon fallback compatible, only one
   interconnect, use the LLCC bandwidth in OPP.

remove unused irq_enable (kbuild robot);
Changes since v3
================
1. Patch #2 (bwmon): remove unused irq_enable (kbuild robot);
   split bwmon_clear() into clearing counters and interrupts, so bwmon_start()
   does not clear the counters twice.

Changes since v2
================
1. Spent a lot of time on benchmarking and learning the BWMON behavior.
2. Drop PM/OPP patch - applied.
3. Patch #1: drop opp-avg-kBps.
4. Patch #2: Add several comments explaining pieces of code and BWMON, extend
   commit msg with measurements, extend help message, add new #defines to document
   some magic values, reorder bwmon clear/disable/enable operations to match
   downstream source and document this with comments, fix unit count from 1 MB
   to 65 kB.
5. Patch #4: drop opp-avg-kBps.
6. Add accumulated Rb tags.

Changes since v1
================
1. Add defconfig change.
2. Fix missing semicolon in MODULE_AUTHOR.
3. Add original downstream (msm-4.9 tree) copyrights to the driver.

Description
===========
BWMON is a data bandwidth monitor providing throughput/bandwidth over certain
interconnect links in a SoC.  It might be used to gather current bus usage and
vote for interconnect bandwidth, thus adjusting the bus speed based on actual
usage.

The work is built on top of Thara Gopinath's patches with several cleanups,
changes and simplifications.

Cc: Rajendra Nayak <quic_rjendra@quicinc.com>

Best regards,
Krzysztof

Krzysztof Kozlowski (4):
  dt-bindings: interconnect: qcom,msm8998-cpu-bwmon: add BWMON device
  soc: qcom: icc-bwmon: Add bandwidth monitoring driver
  arm64: defconfig: enable Qualcomm Bandwidth Monitor
  arm64: dts: qcom: sdm845: Add CPU BWMON

 .../interconnect/qcom,msm8998-bwmon.yaml      |  86 ++++
 MAINTAINERS                                   |   7 +
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |  37 ++
 arch/arm64/configs/defconfig                  |   1 +
 drivers/soc/qcom/Kconfig                      |  15 +
 drivers/soc/qcom/Makefile                     |   1 +
 drivers/soc/qcom/icc-bwmon.c                  | 421 ++++++++++++++++++
 7 files changed, 568 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml
 create mode 100644 drivers/soc/qcom/icc-bwmon.c

Comments

Krzysztof Kozlowski July 4, 2022, 3:22 p.m. UTC | #1 
On 04/07/2022 17:20, Randy Dunlap wrote:
> Hi,
> 
> On 7/4/22 05:17, Krzysztof Kozlowski wrote:
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index e718b8735444..2c8091535bf7 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -228,4 +228,19 @@ config QCOM_APR
>>  	  application processor and QDSP6. APR is
>>  	  used by audio driver to configure QDSP6
>>  	  ASM, ADM and AFE modules.
>> +
>> +config QCOM_ICC_BWMON
>> +	tristate "QCOM Interconnect Bandwidth Monitor driver"
>> +	depends on ARCH_QCOM || COMPILE_TEST
>> +	select PM_OPP
>> +	help
>> +	  Sets up driver monitoring bandwidth on various interconnects and
> 
> 	  Sets up driver bandwidth monitoring
> 
> would be better, I think.

It's a driver which monitors bandwidth, so your version sounds a bit
like monitoring of driver's bandwidth.

Maybe should be:
    Sets up driver which monitors bandwidth...
?
> 
>> +	  based on that voting for interconnect bandwidth, adjusting their
>> +	  speed to current demand.
>> +	  Current implementation brings support for BWMON v4, used for example
>> +	  on SDM845 to measure bandwidth between CPU (gladiator_noc) and Last
>> +	  Level Cache (memnoc).  Usage of this BWMON allows to remove some of
>> +	  the fixed bandwidth votes from cpufreq (CPU nodes) thus achieve high
>> +	  memory throughput even with lower CPU frequencies.
> 


Best regards,
Krzysztof
Randy Dunlap July 4, 2022, 7:15 p.m. UTC | #2 
On 7/4/22 08:22, Krzysztof Kozlowski wrote:
> On 04/07/2022 17:20, Randy Dunlap wrote:
>> Hi,
>>
>> On 7/4/22 05:17, Krzysztof Kozlowski wrote:
>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>>> index e718b8735444..2c8091535bf7 100644
>>> --- a/drivers/soc/qcom/Kconfig
>>> +++ b/drivers/soc/qcom/Kconfig
>>> @@ -228,4 +228,19 @@ config QCOM_APR
>>>  	  application processor and QDSP6. APR is
>>>  	  used by audio driver to configure QDSP6
>>>  	  ASM, ADM and AFE modules.
>>> +
>>> +config QCOM_ICC_BWMON
>>> +	tristate "QCOM Interconnect Bandwidth Monitor driver"
>>> +	depends on ARCH_QCOM || COMPILE_TEST
>>> +	select PM_OPP
>>> +	help
>>> +	  Sets up driver monitoring bandwidth on various interconnects and
>>
>> 	  Sets up driver bandwidth monitoring
>>
>> would be better, I think.
> 
> It's a driver which monitors bandwidth, so your version sounds a bit
> like monitoring of driver's bandwidth.
> 
> Maybe should be:
>     Sets up driver which monitors bandwidth...
> ?

Yes, that's OK.
Thanks.