Hello,
This series brings initial support for memory interconnect to Tegra20,
Tegra30 and Tegra124 SoCs.
For the starter only display controllers and devfreq devices are getting
interconnect API support, others could be supported later on. The display
controllers have the biggest demand for interconnect API right now because
dynamic memory frequency scaling can't be done safely without taking into
account bandwidth requirement from the displays. In particular this series
fixes distorted display output on T30 Ouya and T124 TK1 devices.
Changelog:
v6: - This series was massively reworked in comparison to v5, most of the
patches that previously got r-b need a new round of a review (!).
- Added missed clk-rounding to the set() callback of EMC ICC providers.
Now clk_set_min_rate() doesn't error out on rate overflow.
- Now peak bandwidth is properly taken into account by the set() callback
of EMC ICC providers.
- EMC runs at 2x of the DRAM bus only on Tegra20, this now taken in account
properly by the EMC ICC set() callbacks.
- ICC drivers use new icc_sync_state() and xlate_extended().
- ICC drivers support new TEGRA_MC_ICC_TAG_ISO for ICC paths, which
conveys to ICC driver that memory path uses isochronous transfers.
- Added support for memory latency scaling to Tegra30 ICC provider.
It's required for fixing display FIFO underflows on T30.
- Added basic interconnect support to Tegra124 drivers.
- Tegra20/30/124 EMC drivers now support voltage scaling using generic
OPP API.
- The display bandwidth management is reworked and improved. It now
supports both bandwidth and latency allocations. The nv-display is
now also taken into account properly, i.e. it's kept untouched.
The extra bandwidth reservation required for ISO clients is moved
from DC driver to the ICC drivers.
- Dropped patch that tuned T20 display controller memory client because
turned out that it kills ~30% of memory bandwidth. It should be possible
to support client tuning, but it's too complicated for now.
- Corrected display's cursor and winb-vfilter ICC clients.
The winb-vfilter was erroneously used in place of cursor's client
in device-trees.
- Added devm_tegra_get_memory_controller() and switched drivers to
use it.
- Device-tree OPP tables are now supported by memory and devfreq
drivers.
- Tegra20-devfeq driver is reworked and now uses EMC-stats instead
of IMC-stats (which are nearly identical modules) because previously
I failed to understand how EMC-stats work and succeeded this time,
thinking that it simply doesn't work. This removes a bit icky dependency
on using both EMC and MC drivers simultaneously by the devfreq driver.
- Tegra20-devfeq driver now is a sub-device of the EMC, it also now uses
interconnect API for driving memory bandwidth.
- Tegra30-devfreq got interconnect support.
- Devfreq patches now use dev_err_probe(), which was suggested by
Chanwoo Choi.
- Added acks from Chanwoo Choi and Rob Herring to the reviewed and
unchanged patches.
- Added tested-by from Peter Geis and Nicolas Chauvet, who tested this
series on Ouya and TK1 devices, reporting that it fixes display
corruption on these devices which happened due to insufficient memory
bandwidth.
- Added patches to fix T20 EMC registers size.
- Fixed missing LA entry for PTC in the Tegra MC drivers.
- New and updated patches in v6:
dt-bindings: memory: tegra20: emc: Correct registers range in example
dt-bindings: memory: tegra20: emc: Document nvidia,memory-controller property
dt-bindings: memory: tegra20: emc: Document OPP table and voltage regulator
dt-bindings: memory: tegra20: emc: Document mfd-simple compatible and statistics sub-device
dt-bindings: memory: tegra30: emc: Document OPP table and voltage regulator
dt-bindings: memory: tegra124: mc: Document new interconnect property
dt-bindings: memory: tegra124: emc: Document new interconnect property
dt-bindings: memory: tegra124: emc: Document OPP table and voltage regulator
dt-bindings: tegra30-actmon: Document OPP and interconnect properties
dt-bindings: memory: tegra124: Add memory client IDs
ARM: tegra: Correct EMC registers size in Tegra20 device-tree
ARM: tegra: Add interconnect properties to Tegra124 device-tree
ARM: tegra: Add nvidia,memory-controller phandle to Tegra20 EMC device-tree
ARM: tegra: Add DVFS properties to Tegra20 EMC device-tree node
ARM: tegra: Add DVFS properties to Tegra30 EMC and ACTMON device-tree nodes
ARM: tegra: Add DVFS properties to Tegra124 EMC and ACTMON device-tree nodes
memory: tegra: Add and use devm_tegra_get_memory_controller()
memory: tegra-mc: Add interconnect framework
memory: tegra20: Support interconnect framework
memory: tegra20-emc: Skip parsing of emc-stats DT sub-node
memory: tegra: Add missing latency allowness entry for Page Table Cache
memory: tegra: Add FIFO sizes to Tegra30 memory clients
memory: tegra30: Support interconnect framework
memory: tegra124-emc: Make driver modular
memory: tegra124: Support interconnect framework
memory: tegra: Remove superfluous error messages around platform_get_irq()
drm/tegra: dc: Support memory bandwidth management
drm/tegra: dc: Extend debug stats with total number of events
PM / devfreq: tegra20: Convert to EMC_STAT driver, support interconnect and device-tree
PM / devfreq: tegra30: Support interconnect and OPPs from device-tree
PM / devfreq: tegra30: Separate configurations per-SoC generation
opp: Put interconnect paths outside of opp_table_lock
v5: - The devfreq drivers now won't probe if memory timings aren't specified
in a device-tree, like was suggested by Chanwoo Choi in a review comment
to v4. Initially I wanted to always probe the driver even with a single
fixed memory freq, but after a closer look turned out it can't be done
easily for Tegra20 driver.
- The "interconnect: Relax requirement in of_icc_get_from_provider()"
patch was already applied, hence one less patch in comparison to v4.
- Renamed display interconnect paths in accordance to the names that
were used by Thierry Reding in one of his recent patches that supposed
to update the Host1x's DT binding.
- Added acks from Chanwoo Choi.
- Added clarifying comment to tegra_mc_icc_set() about why it's a dummy
function, this is done in a response to the review comment made by
Georgi Djakov to v4.
v4: - All drivers that use interconnect API now select it in the Kconfig in
order to properly express the build dependency.
- The IS_ENABLED(CONFIG_INTERCONNECT) is dropped now from all patches.
- Added MODULE_AUTHOR() to the modularized drivers, for completeness.
- Added missed TEGRA_MC Kconfig dependency for the Tegra20 EMC driver.
- Added more acks from Rob Herring that I accidentally missed to add in v3.
v3: - Added acks from Rob Herring that were given to some of the v2 patches.
- Specified name of the TRM documentation chapter in the patch
"dt-bindings: host1x: Document new interconnect properties", which was
suggested by Rob Herring in the review comment to v2.
- Added patches that allow EMC drivers to be compiled as a loadable kernel
modules. This came up during of the v2 review when Georgi Djakov pointed
out that interconnect-core could be compiled as a kernel module. Please
note that the Tegra124 EMC driver is compile-tested only, I don't have
Tegra124 HW.
- In the review comment to [1] Stephen Boyd suggested that it will be
better not to make changes to clk API, which was needed in order to
avoid clashing of the interconnect driver with the devfreq in regards
to memory clk-rate rounding.
[1] https://patchwork.ozlabs.org/project/linux-tegra/patch/[email protected]/
Stephen Boyd suggested that instead we should provide OPP table via DT.
I tried to investigate whether this could be done and turned out
it's a bit complicated. Technically it should be doable, but:
1. For now we don't fully support voltage scaling of the CORE regulator
and so OPP table in the DT isn't really needed today. We can
generate table from the memory timings, which is what Tegra devfreq
drivers already do.
2. The OPP table should be defined in the DT for the Memory Controller
node and then its usage somehow should be shared by both interconnect
and devfreq drivers. It's not obvious what's the best way to do it.
So, it will be much better to postpone the DT OPP table addition
until these questions are resolved. We can infer OPPs from the
memory timings and we could get the memory rates from the memory
driver directly, avoiding the problems induced by the clk API usage.
This idea is implemented in v3, see these patches:
PM / devfreq: tegra20: Use MC timings for building OPP table
PM / devfreq: tegra30: Use MC timings for building OPP table
v2: - Instead of a single dma-mem interconnect path, the paths are now
defined per memory client.
- The EMC provider now uses #interconnect-cells=<0>.
- Dropped Tegra124 because there is no enough information about how to
properly calculate required EMC clock rate for it and I don't have
hardware for testing. Somebody else will have to work on it.
- Moved interconnect providers code into drivers/memory/tegra/*.
- Added "Create tegra20-devfreq device" patch because interconnect
is not very usable without the devfreq memory auto-scaling since
memory freq will be fixed to the display's requirement.
Dmitry Osipenko (52):
clk: tegra: Export Tegra20 EMC kernel symbols
soc/tegra: fuse: Export tegra_read_ram_code()
dt-bindings: memory: tegra20: emc: Correct registers range in example
dt-bindings: memory: tegra20: emc: Document nvidia,memory-controller
property
dt-bindings: memory: tegra20: mc: Document new interconnect property
dt-bindings: memory: tegra20: emc: Document new interconnect property
dt-bindings: memory: tegra20: emc: Document OPP table and voltage
regulator
dt-bindings: memory: tegra20: emc: Document mfd-simple compatible and
statistics sub-device
dt-bindings: memory: tegra30: mc: Document new interconnect property
dt-bindings: memory: tegra30: emc: Document new interconnect property
dt-bindings: memory: tegra30: emc: Document OPP table and voltage
regulator
dt-bindings: memory: tegra124: mc: Document new interconnect property
dt-bindings: memory: tegra124: emc: Document new interconnect property
dt-bindings: memory: tegra124: emc: Document OPP table and voltage
regulator
dt-bindings: tegra30-actmon: Document OPP and interconnect properties
dt-bindings: host1x: Document new interconnect properties
dt-bindings: memory: tegra20: Add memory client IDs
dt-bindings: memory: tegra30: Add memory client IDs
dt-bindings: memory: tegra124: Add memory client IDs
ARM: tegra: Correct EMC registers size in Tegra20 device-tree
ARM: tegra: Add interconnect properties to Tegra20 device-tree
ARM: tegra: Add interconnect properties to Tegra30 device-tree
ARM: tegra: Add interconnect properties to Tegra124 device-tree
ARM: tegra: Add nvidia,memory-controller phandle to Tegra20 EMC
device-tree
ARM: tegra: Add DVFS properties to Tegra20 EMC device-tree node
ARM: tegra: Add DVFS properties to Tegra30 EMC and ACTMON device-tree
nodes
ARM: tegra: Add DVFS properties to Tegra124 EMC and ACTMON device-tree
nodes
memory: tegra: Add and use devm_tegra_get_memory_controller()
memory: tegra-mc: Add interconnect framework
memory: tegra20-emc: Make driver modular
memory: tegra20-emc: Use devm_platform_ioremap_resource()
memory: tegra20-emc: Continue probing if timings are missing in
device-tree
memory: tegra20: Support interconnect framework
memory: tegra20-emc: Don't parse emc-stats node
memory: tegra: Add missing latency allowness entry for Page Table
Cache
memory: tegra: Add FIFO sizes to Tegra30 memory clients
memory: tegra30-emc: Make driver modular
memory: tegra30-emc: Continue probing if timings are missing in
device-tree
memory: tegra30: Support interconnect framework
memory: tegra124-emc: Make driver modular
memory: tegra124-emc: Use devm_platform_ioremap_resource()
memory: tegra124: Support interconnect framework
memory: tegra: Remove superfluous error messages around
platform_get_irq()
drm/tegra: dc: Support memory bandwidth management
drm/tegra: dc: Extend debug stats with total number of events
opp: Put interconnect paths outside of opp_table_lock
PM / devfreq: tegra20: Silence deferred probe error
PM / devfreq: tegra20: Relax Kconfig dependency
PM / devfreq: tegra20: Convert to EMC_STAT driver, support
interconnect and device-tree
PM / devfreq: tegra30: Silence deferred probe error
PM / devfreq: tegra30: Support interconnect and OPPs from device-tree
PM / devfreq: tegra30: Separate configurations per-SoC generation
.../arm/tegra/nvidia,tegra30-actmon.txt | 25 ++
.../display/tegra/nvidia,tegra20-host1x.txt | 68 +++
.../nvidia,tegra124-emc.yaml | 18 +
.../nvidia,tegra124-mc.yaml | 5 +
.../memory-controllers/nvidia,tegra20-emc.txt | 63 ++-
.../memory-controllers/nvidia,tegra20-mc.txt | 3 +
.../nvidia,tegra30-emc.yaml | 18 +
.../memory-controllers/nvidia,tegra30-mc.yaml | 5 +
arch/arm/boot/dts/tegra124-apalis-emc.dtsi | 8 +
.../arm/boot/dts/tegra124-jetson-tk1-emc.dtsi | 8 +
arch/arm/boot/dts/tegra124-nyan-big-emc.dtsi | 10 +
.../boot/dts/tegra124-peripherals-opp.dtsi | 419 ++++++++++++++++++
arch/arm/boot/dts/tegra124.dtsi | 31 ++
.../boot/dts/tegra20-acer-a500-picasso.dts | 12 +
arch/arm/boot/dts/tegra20-colibri.dtsi | 8 +
arch/arm/boot/dts/tegra20-paz00.dts | 10 +
.../arm/boot/dts/tegra20-peripherals-opp.dtsi | 181 ++++++++
arch/arm/boot/dts/tegra20.dtsi | 42 +-
.../tegra30-asus-nexus7-grouper-common.dtsi | 16 +
.../arm/boot/dts/tegra30-peripherals-opp.dtsi | 383 ++++++++++++++++
arch/arm/boot/dts/tegra30.dtsi | 33 +-
drivers/clk/tegra/Makefile | 2 +-
drivers/clk/tegra/clk-tegra124-emc.c | 41 +-
drivers/clk/tegra/clk-tegra124.c | 27 +-
drivers/clk/tegra/clk-tegra20-emc.c | 3 +
drivers/clk/tegra/clk.h | 16 +-
drivers/devfreq/Kconfig | 3 +-
drivers/devfreq/tegra20-devfreq.c | 186 ++++----
drivers/devfreq/tegra30-devfreq.c | 166 ++++---
drivers/gpu/drm/tegra/Kconfig | 1 +
drivers/gpu/drm/tegra/dc.c | 340 ++++++++++++++
drivers/gpu/drm/tegra/dc.h | 11 +
drivers/gpu/drm/tegra/drm.c | 14 +
drivers/gpu/drm/tegra/hub.c | 3 +
drivers/gpu/drm/tegra/plane.c | 122 +++++
drivers/gpu/drm/tegra/plane.h | 15 +
drivers/memory/tegra/Kconfig | 12 +-
drivers/memory/tegra/mc.c | 184 +++++++-
drivers/memory/tegra/mc.h | 20 +
drivers/memory/tegra/tegra114.c | 6 +
drivers/memory/tegra/tegra124-emc.c | 235 ++++++++--
drivers/memory/tegra/tegra124.c | 37 ++
drivers/memory/tegra/tegra20-emc.c | 244 ++++++++--
drivers/memory/tegra/tegra20.c | 34 ++
drivers/memory/tegra/tegra210-emc-core.c | 39 +-
drivers/memory/tegra/tegra30-emc.c | 245 ++++++++--
drivers/memory/tegra/tegra30.c | 191 ++++++++
drivers/opp/core.c | 21 +-
drivers/soc/tegra/fuse/tegra-apbmisc.c | 2 +
include/dt-bindings/memory/tegra124-mc.h | 68 +++
include/dt-bindings/memory/tegra20-mc.h | 53 +++
include/dt-bindings/memory/tegra30-mc.h | 67 +++
include/linux/clk/tegra.h | 9 +
include/soc/tegra/emc.h | 16 -
include/soc/tegra/mc.h | 26 ++
55 files changed, 3477 insertions(+), 348 deletions(-)
create mode 100644 arch/arm/boot/dts/tegra124-peripherals-opp.dtsi
create mode 100644 arch/arm/boot/dts/tegra20-peripherals-opp.dtsi
create mode 100644 arch/arm/boot/dts/tegra30-peripherals-opp.dtsi
delete mode 100644 include/soc/tegra/emc.h
--
2.27.0
External Memory Controller can gather various hardware statistics that
are intended to be used for debugging purposes and for dynamic frequency
scaling of memory bus.
Document the new mfd-simple compatible and EMC statistics sub-device.
The subdev contains EMC DFS OPP table and interconnect paths to be used
for dynamic scaling of system's memory bandwidth based on EMC utilization
statistics.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
.../memory-controllers/nvidia,tegra20-emc.txt | 43 +++++++++++++++++--
1 file changed, 40 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
index 8d09b228ac42..382aabcd6952 100644
--- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
+++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
@@ -4,7 +4,7 @@ Properties:
- name : Should be emc
- #address-cells : Should be 1
- #size-cells : Should be 0
-- compatible : Should contain "nvidia,tegra20-emc".
+- compatible : Should contain "nvidia,tegra20-emc" and "simple-mfd".
- reg : Offset and length of the register set for the device
- nvidia,use-ram-code : If present, the sub-nodes will be addressed
and chosen using the ramcode board selector. If omitted, only one
@@ -17,7 +17,8 @@ Properties:
- core-supply: Phandle of voltage regulator of the SoC "core" power domain.
- operating-points-v2: See ../bindings/opp/opp.txt for details.
-Child device nodes describe the memory settings for different configurations and clock rates.
+Child device nodes describe the memory settings for different configurations and clock rates,
+as well as EMC activity statistics collection sub-device.
Example:
@@ -31,17 +32,34 @@ Example:
...
};
+ emc_bw_dfs_opp_table: emc_opp_table1 {
+ compatible = "operating-points-v2";
+
+ opp@36000000 {
+ opp-hz = /bits/ 64 <36000000>;
+ opp-peak-kBps = <144000>;
+ };
+ ...
+ };
+
memory-controller@7000f400 {
#address-cells = < 1 >;
#size-cells = < 0 >;
#interconnect-cells = < 0 >;
- compatible = "nvidia,tegra20-emc";
+ compatible = "nvidia,tegra20-emc", "simple-mfd";
reg = <0x7000f400 0x400>;
interrupts = <0 78 0x04>;
clocks = <&tegra_car TEGRA20_CLK_EMC>;
nvidia,memory-controller = <&mc>;
core-supply = <&core_vdd_reg>;
operating-points-v2 = <&emc_icc_dvfs_opp_table>;
+
+ emc-stats {
+ compatible = "nvidia,tegra20-emc-statistics";
+ operating-points-v2 = <&emc_bw_dfs_opp_table>;
+ interconnects = <&mc TEGRA20_MC_MPCORER &emc>;
+ interconnect-names = "cpu";
+ };
}
@@ -120,3 +138,22 @@ Properties:
0 0 0 0 0 0 0 0 0 0 0 0 0 0
0 0 0 0 >;
};
+
+
+
+Embedded Memory Controller statistics gathering sub-device
+
+EMC statistics subdev gathers information about hardware utilization
+which is intended to be used for debugging purposes and for dynamic
+frequency scaling based on the collected stats.
+
+Properties:
+- name : Should be emc-stats.
+- compatible : Should contain "nvidia,tegra20-emc-statistics".
+- operating-points-v2: See ../bindings/opp/opp.txt for details.
+- interconnects: Should contain entries for memory clients sitting on
+ MC->EMC memory interconnect path.
+- interconnect-names: Should include name of the interconnect path for each
+ interconnect entry. Consult TRM documentation for
+ information about available memory clients, see MEMORY
+ CONTROLLER section.
--
2.27.0
Tegra20 External Memory Controller talks to DRAM chips and it needs to be
reprogrammed when memory frequency changes. Tegra Memory Controller sits
behind EMC and these controllers are tightly coupled. This patch adds the
new phandle property which allows to properly express connection of EMC
and MC hardware in a device-tree, it also put the Tegra20 EMC binding on
par with Tegra30+ EMC bindings, which is handy to have.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
.../bindings/memory-controllers/nvidia,tegra20-emc.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
index 567cffd37f3f..1b0d4417aad8 100644
--- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
+++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
@@ -12,6 +12,7 @@ Properties:
irrespective of ram-code configuration.
- interrupts : Should contain EMC General interrupt.
- clocks : Should contain EMC clock.
+- nvidia,memory-controller : Phandle of the Memory Controller node.
Child device nodes describe the memory settings for different configurations and clock rates.
@@ -24,6 +25,7 @@ Example:
reg = <0x7000f400 0x400>;
interrupts = <0 78 0x04>;
clocks = <&tegra_car TEGRA20_CLK_EMC>;
+ nvidia,memory-controller = <&mc>;
}
--
2.27.0
This patch adds modularization support to the Tegra20 EMC driver. Driver
now can be compiled as a loadable kernel module.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/memory/tegra/Kconfig | 2 +-
drivers/memory/tegra/tegra20-emc.c | 17 ++++++++++++-----
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
index b38e5255effe..ff426747cd7d 100644
--- a/drivers/memory/tegra/Kconfig
+++ b/drivers/memory/tegra/Kconfig
@@ -9,7 +9,7 @@ config TEGRA_MC
NVIDIA Tegra SoCs.
config TEGRA20_EMC
- bool "NVIDIA Tegra20 External Memory Controller driver"
+ tristate "NVIDIA Tegra20 External Memory Controller driver"
default y
depends on ARCH_TEGRA_2x_SOC
help
diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
index 027f46287dbf..0baa6590adea 100644
--- a/drivers/memory/tegra/tegra20-emc.c
+++ b/drivers/memory/tegra/tegra20-emc.c
@@ -724,6 +724,13 @@ static int tegra_emc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, emc);
tegra_emc_debugfs_init(emc);
+ /*
+ * Don't allow the kernel module to be unloaded. Unloading adds some
+ * extra complexity which doesn't really worth the effort in a case of
+ * this driver.
+ */
+ try_module_get(THIS_MODULE);
+
return 0;
unset_cb:
@@ -736,6 +743,7 @@ static const struct of_device_id tegra_emc_of_match[] = {
{ .compatible = "nvidia,tegra20-emc", },
{},
};
+MODULE_DEVICE_TABLE(of, tegra_emc_of_match);
static struct platform_driver tegra_emc_driver = {
.probe = tegra_emc_probe,
@@ -745,9 +753,8 @@ static struct platform_driver tegra_emc_driver = {
.suppress_bind_attrs = true,
},
};
+module_platform_driver(tegra_emc_driver);
-static int __init tegra_emc_init(void)
-{
- return platform_driver_register(&tegra_emc_driver);
-}
-subsys_initcall(tegra_emc_init);
+MODULE_AUTHOR("Dmitry Osipenko <[email protected]>");
+MODULE_DESCRIPTION("NVIDIA Tegra20 EMC driver");
+MODULE_LICENSE("GPL v2");
--
2.27.0
Each memory client have a unique hardware ID, this patch adds these IDs.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
include/dt-bindings/memory/tegra124-mc.h | 68 ++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/include/dt-bindings/memory/tegra124-mc.h b/include/dt-bindings/memory/tegra124-mc.h
index 186e6b7e9b35..7e73bb400eca 100644
--- a/include/dt-bindings/memory/tegra124-mc.h
+++ b/include/dt-bindings/memory/tegra124-mc.h
@@ -54,4 +54,72 @@
#define TEGRA124_MC_RESET_ISP2B 22
#define TEGRA124_MC_RESET_GPU 23
+#define TEGRA124_MC_PTCR 0
+#define TEGRA124_MC_DISPLAY0A 1
+#define TEGRA124_MC_DISPLAY0AB 2
+#define TEGRA124_MC_DISPLAY0B 3
+#define TEGRA124_MC_DISPLAY0BB 4
+#define TEGRA124_MC_DISPLAY0C 5
+#define TEGRA124_MC_DISPLAY0CB 6
+#define TEGRA124_MC_AFIR 14
+#define TEGRA124_MC_AVPCARM7R 15
+#define TEGRA124_MC_DISPLAYHC 16
+#define TEGRA124_MC_DISPLAYHCB 17
+#define TEGRA124_MC_HDAR 21
+#define TEGRA124_MC_HOST1XDMAR 22
+#define TEGRA124_MC_HOST1XR 23
+#define TEGRA124_MC_MSENCSRD 28
+#define TEGRA124_MC_PPCSAHBDMAR 29
+#define TEGRA124_MC_PPCSAHBSLVR 30
+#define TEGRA124_MC_SATAR 31
+#define TEGRA124_MC_VDEBSEVR 34
+#define TEGRA124_MC_VDEMBER 35
+#define TEGRA124_MC_VDEMCER 36
+#define TEGRA124_MC_VDETPER 37
+#define TEGRA124_MC_MPCORELPR 38
+#define TEGRA124_MC_MPCORER 39
+#define TEGRA124_MC_MSENCSWR 43
+#define TEGRA124_MC_AFIW 49
+#define TEGRA124_MC_AVPCARM7W 50
+#define TEGRA124_MC_HDAW 53
+#define TEGRA124_MC_HOST1XW 54
+#define TEGRA124_MC_MPCORELPW 56
+#define TEGRA124_MC_MPCOREW 57
+#define TEGRA124_MC_PPCSAHBDMAW 59
+#define TEGRA124_MC_PPCSAHBSLVW 60
+#define TEGRA124_MC_SATAW 61
+#define TEGRA124_MC_VDEBSEVW 62
+#define TEGRA124_MC_VDEDBGW 63
+#define TEGRA124_MC_VDEMBEW 64
+#define TEGRA124_MC_VDETPMW 65
+#define TEGRA124_MC_ISPRA 68
+#define TEGRA124_MC_ISPWA 70
+#define TEGRA124_MC_ISPWB 71
+#define TEGRA124_MC_XUSB_HOSTR 74
+#define TEGRA124_MC_XUSB_HOSTW 75
+#define TEGRA124_MC_XUSB_DEVR 76
+#define TEGRA124_MC_XUSB_DEVW 77
+#define TEGRA124_MC_ISPRAB 78
+#define TEGRA124_MC_ISPWAB 80
+#define TEGRA124_MC_ISPWBB 81
+#define TEGRA124_MC_TSECSRD 84
+#define TEGRA124_MC_TSECSWR 85
+#define TEGRA124_MC_A9AVPSCR 86
+#define TEGRA124_MC_A9AVPSCW 87
+#define TEGRA124_MC_GPUSRD 88
+#define TEGRA124_MC_GPUSWR 89
+#define TEGRA124_MC_DISPLAYT 90
+#define TEGRA124_MC_SDMMCRA 96
+#define TEGRA124_MC_SDMMCRAA 97
+#define TEGRA124_MC_SDMMCR 98
+#define TEGRA124_MC_SDMMCRAB 99
+#define TEGRA124_MC_SDMMCWA 100
+#define TEGRA124_MC_SDMMCWAA 101
+#define TEGRA124_MC_SDMMCW 102
+#define TEGRA124_MC_SDMMCWAB 103
+#define TEGRA124_MC_VICSRD 108
+#define TEGRA124_MC_VICSWR 109
+#define TEGRA124_MC_VIW 114
+#define TEGRA124_MC_DISPLAYD 115
+
#endif
--
2.27.0
Add EMC OPP DVFS/DFS tables and emc-stats subdev that will be used for
dynamic memory bandwidth scaling, while EMC itself will perform voltage
scaling. Update board device-trees with optional EMC core supply and
remove unsupported OPPs.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
.../boot/dts/tegra20-acer-a500-picasso.dts | 12 ++
arch/arm/boot/dts/tegra20-colibri.dtsi | 8 +
arch/arm/boot/dts/tegra20-paz00.dts | 10 +
.../arm/boot/dts/tegra20-peripherals-opp.dtsi | 181 ++++++++++++++++++
arch/arm/boot/dts/tegra20.dtsi | 12 +-
5 files changed, 222 insertions(+), 1 deletion(-)
create mode 100644 arch/arm/boot/dts/tegra20-peripherals-opp.dtsi
diff --git a/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts b/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
index a0b829738e8f..f5c1591c8ea8 100644
--- a/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
+++ b/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
@@ -1058,9 +1058,21 @@ map0 {
};
};
+ emc_opp_table0 {
+ /delete-node/ opp@666000000;
+ /delete-node/ opp@760000000;
+ };
+
+ emc_opp_table1 {
+ /delete-node/ opp@666000000;
+ /delete-node/ opp@760000000;
+ };
+
memory-controller@7000f400 {
nvidia,use-ram-code;
+ core-supply = <&vdd_core>;
+
emc-tables@0 {
nvidia,ram-code = <0>; /* elpida-8gb */
diff --git a/arch/arm/boot/dts/tegra20-colibri.dtsi b/arch/arm/boot/dts/tegra20-colibri.dtsi
index 6162d193e12c..78a2210bf9ae 100644
--- a/arch/arm/boot/dts/tegra20-colibri.dtsi
+++ b/arch/arm/boot/dts/tegra20-colibri.dtsi
@@ -611,6 +611,14 @@ i2c-thermtrip {
};
};
+ emc_opp_table0 {
+ /delete-node/ opp@760000000;
+ };
+
+ emc_opp_table1 {
+ /delete-node/ opp@760000000;
+ };
+
memory-controller@7000f400 {
emc-table@83250 {
reg = <83250>;
diff --git a/arch/arm/boot/dts/tegra20-paz00.dts b/arch/arm/boot/dts/tegra20-paz00.dts
index ada2bed8b1b5..7b9f0f279744 100644
--- a/arch/arm/boot/dts/tegra20-paz00.dts
+++ b/arch/arm/boot/dts/tegra20-paz00.dts
@@ -311,9 +311,19 @@ nvec@7000c500 {
reset-names = "i2c";
};
+ emc_opp_table0 {
+ /delete-node/ opp@760000000;
+ };
+
+ emc_opp_table1 {
+ /delete-node/ opp@760000000;
+ };
+
memory-controller@7000f400 {
nvidia,use-ram-code;
+ core-supply = <&core_vdd_reg>;
+
emc-tables@0 {
nvidia,ram-code = <0x0>;
#address-cells = <1>;
diff --git a/arch/arm/boot/dts/tegra20-peripherals-opp.dtsi b/arch/arm/boot/dts/tegra20-peripherals-opp.dtsi
new file mode 100644
index 000000000000..d10c61107702
--- /dev/null
+++ b/arch/arm/boot/dts/tegra20-peripherals-opp.dtsi
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/ {
+ emc_icc_dvfs_opp_table: emc_opp_table0 {
+ compatible = "operating-points-v2";
+
+ opp@36000000 {
+ opp-microvolt = <950000 950000 1300000>;
+ opp-hz = /bits/ 64 <36000000>;
+ };
+
+ opp@47500000 {
+ opp-microvolt = <950000 950000 1300000>;
+ opp-hz = /bits/ 64 <47500000>;
+ };
+
+ opp@50000000 {
+ opp-microvolt = <950000 950000 1300000>;
+ opp-hz = /bits/ 64 <50000000>;
+ };
+
+ opp@54000000 {
+ opp-microvolt = <950000 950000 1300000>;
+ opp-hz = /bits/ 64 <54000000>;
+ };
+
+ opp@57000000 {
+ opp-microvolt = <950000 950000 1300000>;
+ opp-hz = /bits/ 64 <57000000>;
+ };
+
+ opp@100000000 {
+ opp-microvolt = <1000000 1000000 1300000>;
+ opp-hz = /bits/ 64 <100000000>;
+ };
+
+ opp@108000000 {
+ opp-microvolt = <1000000 1000000 1300000>;
+ opp-hz = /bits/ 64 <108000000>;
+ };
+
+ opp@126666000 {
+ opp-microvolt = <1000000 1000000 1300000>;
+ opp-hz = /bits/ 64 <126666000>;
+ };
+
+ opp@150000000 {
+ opp-microvolt = <1000000 1000000 1300000>;
+ opp-hz = /bits/ 64 <150000000>;
+ };
+
+ opp@190000000 {
+ opp-microvolt = <1000000 1000000 1300000>;
+ opp-hz = /bits/ 64 <190000000>;
+ };
+
+ opp@216000000 {
+ opp-microvolt = <1000000 1000000 1300000>;
+ opp-hz = /bits/ 64 <216000000>;
+ };
+
+ opp@300000000 {
+ opp-microvolt = <1000000 1000000 1300000>;
+ opp-hz = /bits/ 64 <300000000>;
+ };
+
+ opp@333000000 {
+ opp-microvolt = <1000000 1000000 1300000>;
+ opp-hz = /bits/ 64 <333000000>;
+ };
+
+ opp@380000000 {
+ opp-microvolt = <1100000 1100000 1300000>;
+ opp-hz = /bits/ 64 <380000000>;
+ };
+
+ opp@600000000 {
+ opp-microvolt = <1200000 1200000 1300000>;
+ opp-hz = /bits/ 64 <600000000>;
+ };
+
+ opp@666000000 {
+ opp-microvolt = <1200000 1200000 1300000>;
+ opp-hz = /bits/ 64 <666000000>;
+ };
+
+ opp@760000000 {
+ opp-microvolt = <1300000 1300000 1300000>;
+ opp-hz = /bits/ 64 <760000000>;
+ };
+ };
+
+ emc_bw_dfs_opp_table: emc_opp_table1 {
+ compatible = "operating-points-v2";
+
+ opp@36000000 {
+ opp-hz = /bits/ 64 <36000000>;
+ opp-peak-kBps = <144000>;
+ };
+
+ opp@47500000 {
+ opp-hz = /bits/ 64 <47500000>;
+ opp-peak-kBps = <190000>;
+ };
+
+ opp@50000000 {
+ opp-hz = /bits/ 64 <50000000>;
+ opp-peak-kBps = <200000>;
+ };
+
+ opp@54000000 {
+ opp-hz = /bits/ 64 <54000000>;
+ opp-peak-kBps = <216000>;
+ };
+
+ opp@57000000 {
+ opp-hz = /bits/ 64 <57000000>;
+ opp-peak-kBps = <228000>;
+ };
+
+ opp@100000000 {
+ opp-hz = /bits/ 64 <100000000>;
+ opp-peak-kBps = <400000>;
+ };
+
+ opp@108000000 {
+ opp-hz = /bits/ 64 <108000000>;
+ opp-peak-kBps = <432000>;
+ };
+
+ opp@126666000 {
+ opp-hz = /bits/ 64 <126666000>;
+ opp-peak-kBps = <506664>;
+ };
+
+ opp@150000000 {
+ opp-hz = /bits/ 64 <150000000>;
+ opp-peak-kBps = <600000>;
+ };
+
+ opp@190000000 {
+ opp-hz = /bits/ 64 <190000000>;
+ opp-peak-kBps = <760000>;
+ };
+
+ opp@216000000 {
+ opp-hz = /bits/ 64 <216000000>;
+ opp-peak-kBps = <864000>;
+ };
+
+ opp@300000000 {
+ opp-hz = /bits/ 64 <300000000>;
+ opp-peak-kBps = <1200000>;
+ };
+
+ opp@333000000 {
+ opp-hz = /bits/ 64 <333000000>;
+ opp-peak-kBps = <1332000>;
+ };
+
+ opp@380000000 {
+ opp-hz = /bits/ 64 <380000000>;
+ opp-peak-kBps = <1520000>;
+ };
+
+ opp@600000000 {
+ opp-hz = /bits/ 64 <600000000>;
+ opp-peak-kBps = <2400000>;
+ };
+
+ opp@666000000 {
+ opp-hz = /bits/ 64 <666000000>;
+ opp-peak-kBps = <2664000>;
+ };
+
+ opp@760000000 {
+ opp-hz = /bits/ 64 <760000000>;
+ opp-peak-kBps = <3040000>;
+ };
+ };
+};
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 8f8ad81916e7..8a90d96c8773 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -6,6 +6,8 @@
#include <dt-bindings/interrupt-controller/arm-gic.h>
#include <dt-bindings/soc/tegra-pmc.h>
+#include "tegra20-peripherals-opp.dtsi"
+
/ {
compatible = "nvidia,tegra20";
interrupt-parent = <&lic>;
@@ -656,7 +658,7 @@ mc: memory-controller@7000f000 {
};
emc: memory-controller@7000f400 {
- compatible = "nvidia,tegra20-emc";
+ compatible = "nvidia,tegra20-emc", "simple-mfd";
reg = <0x7000f400 0x400>;
interrupts = <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&tegra_car TEGRA20_CLK_EMC>;
@@ -664,7 +666,15 @@ emc: memory-controller@7000f400 {
#size-cells = <0>;
#interconnect-cells = <0>;
+ operating-points-v2 = <&emc_icc_dvfs_opp_table>;
nvidia,memory-controller = <&mc>;
+
+ emc-stats {
+ compatible = "nvidia,tegra20-emc-statistics";
+ operating-points-v2 = <&emc_bw_dfs_opp_table>;
+ interconnects = <&mc TEGRA20_MC_MPCORER &emc>;
+ interconnect-names = "cpu-read";
+ };
};
fuse@7000f800 {
--
2.27.0
EMC device-tree node now contains new emc-stats sub-node which needs to
be skipped when timing nodes are parsed by EMC driver, otherwise driver
will try to parse the emc-stats as a timing node and will error out.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/memory/tegra/tegra20-emc.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
index 69ccb3fe5b0b..27242659dfd6 100644
--- a/drivers/memory/tegra/tegra20-emc.c
+++ b/drivers/memory/tegra/tegra20-emc.c
@@ -349,7 +349,10 @@ static int tegra_emc_load_timings_from_dt(struct tegra_emc *emc,
int child_count;
int err;
- child_count = of_get_child_count(node);
+ child = of_find_node_by_name(node, "emc-stats");
+ of_node_put(child);
+
+ child_count = of_get_child_count(node) - (child ? 1 : 0);
if (!child_count) {
dev_err(emc->dev, "no memory timings in DT node: %pOF\n", node);
return -EINVAL;
@@ -364,6 +367,9 @@ static int tegra_emc_load_timings_from_dt(struct tegra_emc *emc,
timing = emc->timings;
for_each_child_of_node(node, child) {
+ if (of_device_is_compatible(child, "nvidia,tegra20-emc-statistics"))
+ continue;
+
err = load_one_timing_from_dt(emc, timing++, child);
if (err) {
of_node_put(child);
@@ -391,7 +397,11 @@ tegra_emc_find_node_by_ram_code(struct device *dev)
u32 value, ram_code;
int err;
- if (of_get_child_count(dev->of_node) == 0) {
+ /* old device-trees don't have emc-stats node */
+ np = of_find_node_by_name(dev->of_node, "emc-stats");
+ of_node_put(np);
+
+ if (of_get_child_count(dev->of_node) == (np ? 1 : 0)) {
dev_info(dev, "device-tree doesn't have memory timings\n");
return NULL;
}
--
2.27.0
Tegra EMC driver was turned into a regular kernel driver, meaning that it
could be compiled as a loadable kernel module now. Hence EMC clock isn't
guaranteed to be available and clk_get("emc") may return -EPROBE_DEFER.
Let's silence the deferred probe error.
Acked-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/tegra20-devfreq.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c
index ff82bac9ee4e..fd801534771d 100644
--- a/drivers/devfreq/tegra20-devfreq.c
+++ b/drivers/devfreq/tegra20-devfreq.c
@@ -141,11 +141,9 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
/* EMC is a system-critical clock that is always enabled */
tegra->emc_clock = devm_clk_get(&pdev->dev, "emc");
- if (IS_ERR(tegra->emc_clock)) {
- err = PTR_ERR(tegra->emc_clock);
- dev_err(&pdev->dev, "failed to get emc clock: %d\n", err);
- return err;
- }
+ if (IS_ERR(tegra->emc_clock))
+ return dev_err_probe(&pdev->dev, PTR_ERR(tegra->emc_clock),
+ "failed to get emc clock\n");
tegra->regs = mc->regs;
--
2.27.0
The platform_get_irq() prints error message telling that interrupt is
missing, hence there is no need to duplicated that message in the drivers.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/memory/tegra/mc.c | 4 +---
drivers/memory/tegra/tegra20-emc.c | 1 -
drivers/memory/tegra/tegra30-emc.c | 5 ++---
3 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index 15589bf8f5b6..4a3bf08495c9 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -837,10 +837,8 @@ static int tegra_mc_probe(struct platform_device *pdev)
}
mc->irq = platform_get_irq(pdev, 0);
- if (mc->irq < 0) {
- dev_err(&pdev->dev, "interrupt not specified\n");
+ if (mc->irq < 0)
return mc->irq;
- }
WARN(!mc->soc->client_id_mask, "missing client ID mask for this SoC\n");
diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
index 27242659dfd6..1519d6ce9b28 100644
--- a/drivers/memory/tegra/tegra20-emc.c
+++ b/drivers/memory/tegra/tegra20-emc.c
@@ -844,7 +844,6 @@ static int tegra_emc_probe(struct platform_device *pdev)
irq = platform_get_irq(pdev, 0);
if (irq < 0) {
- dev_err(&pdev->dev, "interrupt not specified\n");
dev_err(&pdev->dev, "please update your device tree\n");
return irq;
}
diff --git a/drivers/memory/tegra/tegra30-emc.c b/drivers/memory/tegra/tegra30-emc.c
index 66eae944ca6d..d2515d7f3c0b 100644
--- a/drivers/memory/tegra/tegra30-emc.c
+++ b/drivers/memory/tegra/tegra30-emc.c
@@ -1472,10 +1472,9 @@ static int tegra_emc_probe(struct platform_device *pdev)
return err;
err = platform_get_irq(pdev, 0);
- if (err < 0) {
- dev_err(&pdev->dev, "interrupt not specified: %d\n", err);
+ if (err < 0)
return err;
- }
+
emc->irq = err;
err = devm_request_irq(&pdev->dev, emc->irq, tegra_emc_isr, 0,
--
2.27.0
The PTC memory client misses the latency allowness entry and this patch
adds it.
This prevents erroneous clearing of MC_INTSTATUS 0x0 register during
of the LA programming in tegra_mc_setup_latency_allowance() due to the
missing entry. Note that this patch doesn't fix any known problems.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/memory/tegra/tegra114.c | 6 ++++++
drivers/memory/tegra/tegra124.c | 6 ++++++
drivers/memory/tegra/tegra30.c | 6 ++++++
3 files changed, 18 insertions(+)
diff --git a/drivers/memory/tegra/tegra114.c b/drivers/memory/tegra/tegra114.c
index 48ef01c3ff90..ed376ba2d2fe 100644
--- a/drivers/memory/tegra/tegra114.c
+++ b/drivers/memory/tegra/tegra114.c
@@ -15,6 +15,12 @@ static const struct tegra_mc_client tegra114_mc_clients[] = {
.id = 0x00,
.name = "ptcr",
.swgroup = TEGRA_SWGROUP_PTC,
+ .la = {
+ .reg = 0x34c,
+ .shift = 0,
+ .mask = 0xff,
+ .def = 0x0,
+ },
}, {
.id = 0x01,
.name = "display0a",
diff --git a/drivers/memory/tegra/tegra124.c b/drivers/memory/tegra/tegra124.c
index 0cede24479bf..e2389573d3c0 100644
--- a/drivers/memory/tegra/tegra124.c
+++ b/drivers/memory/tegra/tegra124.c
@@ -15,6 +15,12 @@ static const struct tegra_mc_client tegra124_mc_clients[] = {
.id = 0x00,
.name = "ptcr",
.swgroup = TEGRA_SWGROUP_PTC,
+ .la = {
+ .reg = 0x34c,
+ .shift = 0,
+ .mask = 0xff,
+ .def = 0x0,
+ },
}, {
.id = 0x01,
.name = "display0a",
diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
index fcdd812eed80..b1990b4133d8 100644
--- a/drivers/memory/tegra/tegra30.c
+++ b/drivers/memory/tegra/tegra30.c
@@ -36,6 +36,12 @@ static const struct tegra_mc_client tegra30_mc_clients[] = {
.id = 0x00,
.name = "ptcr",
.swgroup = TEGRA_SWGROUP_PTC,
+ .la = {
+ .reg = 0x34c,
+ .shift = 0,
+ .mask = 0xff,
+ .def = 0x0,
+ },
}, {
.id = 0x01,
.name = "display0a",
--
2.27.0
Previously we were using count-weight of the T124 for T30 in order to
get EMC clock rate that was reasonable for T30. In fact the count-weight
should be x2 times smaller on T30, but then devfreq was producing a bit
too low EMC clock rate for ISO memory clients, like display controller
for example.
Now both Tegra ACTMON and Tegra DRM display drivers support interconnect
framework and display driver tells to ICC what a minimum memory bandwidth
is needed, preventing FIFO underflows. Thus, now we can use a proper
count-weight value for Tegra30 and MC_ALL device config needs a bit more
aggressive boosting.
This patch adds a separate ACTMON driver configuration that is specific
to Tegra30.
Tested-by: Peter Geis <[email protected]>
Tested-by: Nicolas Chauvet <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/tegra30-devfreq.c | 68 ++++++++++++++++++++++++-------
1 file changed, 54 insertions(+), 14 deletions(-)
diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 1b0b91a71886..95aba89eae88 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -57,13 +57,6 @@
#define ACTMON_BELOW_WMARK_WINDOW 3
#define ACTMON_BOOST_FREQ_STEP 16000
-/*
- * Activity counter is incremented every 256 memory transactions, and each
- * transaction takes 4 EMC clocks for Tegra124; So the COUNT_WEIGHT is
- * 4 * 256 = 1024.
- */
-#define ACTMON_COUNT_WEIGHT 0x400
-
/*
* ACTMON_AVERAGE_WINDOW_LOG2: default value for @DEV_CTRL_K_VAL, which
* translates to 2 ^ (K_VAL + 1). ex: 2 ^ (6 + 1) = 128
@@ -111,7 +104,7 @@ enum tegra_actmon_device {
MCCPU,
};
-static const struct tegra_devfreq_device_config actmon_device_configs[] = {
+static const struct tegra_devfreq_device_config tegra124_device_configs[] = {
{
/* MCALL: All memory accesses (including from the CPUs) */
.offset = 0x1c0,
@@ -133,6 +126,28 @@ static const struct tegra_devfreq_device_config actmon_device_configs[] = {
},
};
+static const struct tegra_devfreq_device_config tegra30_device_configs[] = {
+ {
+ /* MCALL: All memory accesses (including from the CPUs) */
+ .offset = 0x1c0,
+ .irq_mask = 1 << 26,
+ .boost_up_coeff = 200,
+ .boost_down_coeff = 50,
+ .boost_up_threshold = 20,
+ .boost_down_threshold = 10,
+ },
+ {
+ /* MCCPU: memory accesses from the CPUs */
+ .offset = 0x200,
+ .irq_mask = 1 << 25,
+ .boost_up_coeff = 800,
+ .boost_down_coeff = 40,
+ .boost_up_threshold = 27,
+ .boost_down_threshold = 10,
+ .avg_dependency_threshold = 16000, /* 16MHz in kHz units */
+ },
+};
+
/**
* struct tegra_devfreq_device - state specific to an ACTMON device
*
@@ -155,6 +170,12 @@ struct tegra_devfreq_device {
unsigned long target_freq;
};
+struct tegra_devfreq_soc_data {
+ const struct tegra_devfreq_device_config *configs;
+ /* Weight value for count measurements */
+ unsigned int count_weight;
+};
+
struct tegra_devfreq {
struct devfreq *devfreq;
struct opp_table *opp_table;
@@ -171,11 +192,13 @@ struct tegra_devfreq {
struct delayed_work cpufreq_update_work;
struct notifier_block cpu_rate_change_nb;
- struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
+ struct tegra_devfreq_device devices[ARRAY_SIZE(tegra124_device_configs)];
unsigned int irq;
bool started;
+
+ const struct tegra_devfreq_soc_data *soc;
};
struct tegra_actmon_emc_ratio {
@@ -488,7 +511,7 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
tegra_devfreq_update_avg_wmark(tegra, dev);
tegra_devfreq_update_wmark(tegra, dev);
- device_writel(dev, ACTMON_COUNT_WEIGHT, ACTMON_DEV_COUNT_WEIGHT);
+ device_writel(dev, tegra->soc->count_weight, ACTMON_DEV_COUNT_WEIGHT);
device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
val |= ACTMON_DEV_CTRL_ENB_PERIODIC;
@@ -781,6 +804,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
if (!tegra)
return -ENOMEM;
+ tegra->soc = of_device_get_match_data(&pdev->dev);
+
tegra->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(tegra->regs))
return PTR_ERR(tegra->regs);
@@ -858,9 +883,9 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
tegra->max_freq = rate / KHZ;
- for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
+ for (i = 0; i < ARRAY_SIZE(tegra124_device_configs); i++) {
dev = tegra->devices + i;
- dev->config = actmon_device_configs + i;
+ dev->config = tegra->soc->configs + i;
dev->regs = tegra->regs + dev->config->offset;
}
@@ -925,9 +950,24 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
return 0;
}
+static const struct tegra_devfreq_soc_data tegra124_soc = {
+ .configs = tegra124_device_configs,
+
+ /*
+ * Activity counter is incremented every 256 memory transactions,
+ * and each transaction takes 4 EMC clocks.
+ */
+ .count_weight = 4 * 256,
+};
+
+static const struct tegra_devfreq_soc_data tegra30_soc = {
+ .configs = tegra30_device_configs,
+ .count_weight = 2 * 256,
+};
+
static const struct of_device_id tegra_devfreq_of_match[] = {
- { .compatible = "nvidia,tegra30-actmon" },
- { .compatible = "nvidia,tegra124-actmon" },
+ { .compatible = "nvidia,tegra30-actmon", .data = &tegra30_soc, },
+ { .compatible = "nvidia,tegra124-actmon", .data = &tegra124_soc, },
{ },
};
--
2.27.0
This patch moves ACTMON driver away from generating OPP table by itself,
transitioning it to use the table which comes from device-tree. This
change breaks compatibility with older device-trees in order to bring
support for the interconnect framework to the driver. This is a mandatory
change which needs to be done in order to implement interconnect-based
memory DVFS. Users of legacy device-trees will get a message telling that
theirs DT needs to be upgraded. Now ACTMON issues memory bandwidth request
using dev_pm_opp_set_bw(), instead of driving EMC clock rate directly.
Tested-by: Peter Geis <[email protected]>
Tested-by: Nicolas Chauvet <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/tegra30-devfreq.c | 91 ++++++++++++++++---------------
1 file changed, 48 insertions(+), 43 deletions(-)
diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 3f732ab53573..1b0b91a71886 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -19,6 +19,8 @@
#include <linux/reset.h>
#include <linux/workqueue.h>
+#include <soc/tegra/fuse.h>
+
#include "governor.h"
#define ACTMON_GLB_STATUS 0x0
@@ -155,6 +157,7 @@ struct tegra_devfreq_device {
struct tegra_devfreq {
struct devfreq *devfreq;
+ struct opp_table *opp_table;
struct reset_control *reset;
struct clk *clock;
@@ -612,34 +615,19 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
u32 flags)
{
- struct tegra_devfreq *tegra = dev_get_drvdata(dev);
- struct devfreq *devfreq = tegra->devfreq;
struct dev_pm_opp *opp;
- unsigned long rate;
- int err;
+ int ret;
opp = devfreq_recommended_opp(dev, freq, flags);
if (IS_ERR(opp)) {
- dev_err(dev, "Failed to find opp for %lu Hz\n", *freq);
+ dev_err(dev, "failed to find opp for %lu Hz\n", *freq);
return PTR_ERR(opp);
}
- rate = dev_pm_opp_get_freq(opp);
- dev_pm_opp_put(opp);
-
- err = clk_set_min_rate(tegra->emc_clock, rate * KHZ);
- if (err)
- return err;
-
- err = clk_set_rate(tegra->emc_clock, 0);
- if (err)
- goto restore_min_rate;
- return 0;
-
-restore_min_rate:
- clk_set_min_rate(tegra->emc_clock, devfreq->previous_freq);
+ ret = dev_pm_opp_set_bw(dev, opp);
+ dev_pm_opp_put(opp);
- return err;
+ return ret;
}
static int tegra_devfreq_get_dev_status(struct device *dev,
@@ -655,7 +643,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
stat->private_data = tegra;
/* The below are to be used by the other governors */
- stat->current_frequency = cur_freq;
+ stat->current_frequency = cur_freq * KHZ;
actmon_dev = &tegra->devices[MCALL];
@@ -705,7 +693,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
target_freq = max(target_freq, dev->target_freq);
}
- *freq = target_freq;
+ *freq = target_freq * KHZ;
return 0;
}
@@ -773,13 +761,22 @@ static struct devfreq_governor tegra_devfreq_governor = {
static int tegra_devfreq_probe(struct platform_device *pdev)
{
+ u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
struct tegra_devfreq_device *dev;
struct tegra_devfreq *tegra;
+ struct opp_table *opp_table;
struct devfreq *devfreq;
unsigned int i;
long rate;
int err;
+ /* legacy device-trees don't have OPP table and must be updated */
+ if (!device_property_present(&pdev->dev, "operating-points-v2")) {
+ dev_err(&pdev->dev, "OPP table not found, cannot continue\n");
+ dev_err(&pdev->dev, "please update your device tree\n");
+ return -ENODEV;
+ }
+
tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
if (!tegra)
return -ENOMEM;
@@ -821,11 +818,29 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
return err;
}
+ tegra->opp_table = dev_pm_opp_get_opp_table(&pdev->dev);
+ if (IS_ERR(tegra->opp_table))
+ return dev_err_probe(&pdev->dev, PTR_ERR(tegra->opp_table),
+ "Failed to prepare OPP table\n");
+
+ opp_table = dev_pm_opp_set_supported_hw(&pdev->dev, &hw_version, 1);
+ err = PTR_ERR_OR_ZERO(opp_table);
+ if (err) {
+ dev_err(&pdev->dev, "Failed to set supported HW: %d\n", err);
+ goto put_table;
+ }
+
+ err = dev_pm_opp_of_add_table(&pdev->dev);
+ if (err) {
+ dev_err(&pdev->dev, "Failed to add OPP table: %d\n", err);
+ goto put_hw;
+ }
+
err = clk_prepare_enable(tegra->clock);
if (err) {
dev_err(&pdev->dev,
"Failed to prepare and enable ACTMON clock\n");
- return err;
+ goto remove_table;
}
err = reset_control_reset(tegra->reset);
@@ -849,23 +864,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
dev->regs = tegra->regs + dev->config->offset;
}
- for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
- rate = clk_round_rate(tegra->emc_clock, rate);
-
- if (rate < 0) {
- dev_err(&pdev->dev,
- "Failed to round clock rate: %ld\n", rate);
- err = rate;
- goto remove_opps;
- }
-
- err = dev_pm_opp_add(&pdev->dev, rate / KHZ, 0);
- if (err) {
- dev_err(&pdev->dev, "Failed to add OPP: %d\n", err);
- goto remove_opps;
- }
- }
-
platform_set_drvdata(pdev, tegra);
tegra->clk_rate_change_nb.notifier_call = tegra_actmon_clk_notify_cb;
@@ -881,7 +879,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
}
tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
- tegra_devfreq_profile.initial_freq /= KHZ;
devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
"tegra_actmon", NULL);
@@ -901,6 +898,12 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
reset_control_reset(tegra->reset);
disable_clk:
clk_disable_unprepare(tegra->clock);
+remove_table:
+ dev_pm_opp_of_remove_table(&pdev->dev);
+put_hw:
+ dev_pm_opp_put_supported_hw(tegra->opp_table);
+put_table:
+ dev_pm_opp_put_opp_table(tegra->opp_table);
return err;
}
@@ -912,11 +915,13 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
devfreq_remove_device(tegra->devfreq);
devfreq_remove_governor(&tegra_devfreq_governor);
- dev_pm_opp_remove_all_dynamic(&pdev->dev);
-
reset_control_reset(tegra->reset);
clk_disable_unprepare(tegra->clock);
+ dev_pm_opp_of_remove_table(&pdev->dev);
+ dev_pm_opp_put_supported_hw(tegra->opp_table);
+ dev_pm_opp_put_opp_table(tegra->opp_table);
+
return 0;
}
--
2.27.0
This patch fixes lockup which happens when OPP table is released if
interconnect provider uses OPP in the icc_provider->set() callback
and bandwidth of the ICC path is set to 0 by the ICC core when path
is released. The icc_put() doesn't need the opp_table_lock protection,
hence let's move it outside of the lock in order to resolve the problem.
In particular this fixes tegra-devfreq driver lockup on trying to unload
the driver module. The devfreq driver uses OPP-bandwidth API and its ICC
provider also uses OPP for DVFS, hence they both take same opp_table_lock
when OPP table of the devfreq is released.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/opp/core.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 2483e765318a..1134df360fe0 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1187,12 +1187,6 @@ static void _opp_table_kref_release(struct kref *kref)
if (!IS_ERR(opp_table->clk))
clk_put(opp_table->clk);
- if (opp_table->paths) {
- for (i = 0; i < opp_table->path_count; i++)
- icc_put(opp_table->paths[i]);
- kfree(opp_table->paths);
- }
-
WARN_ON(!list_empty(&opp_table->opp_list));
list_for_each_entry_safe(opp_dev, temp, &opp_table->dev_list, node) {
@@ -1209,9 +1203,22 @@ static void _opp_table_kref_release(struct kref *kref)
mutex_destroy(&opp_table->genpd_virt_dev_lock);
mutex_destroy(&opp_table->lock);
list_del(&opp_table->node);
- kfree(opp_table);
mutex_unlock(&opp_table_lock);
+
+ /*
+ * Interconnect provider may use OPP too, hence icc_put() needs to be
+ * invoked outside of the opp_table_lock in order to prevent nested
+ * locking which happens when bandwidth of the ICC path is set to 0
+ * by ICC core on release of the path.
+ */
+ if (opp_table->paths) {
+ for (i = 0; i < opp_table->path_count; i++)
+ icc_put(opp_table->paths[i]);
+ kfree(opp_table->paths);
+ }
+
+ kfree(opp_table);
}
void dev_pm_opp_put_opp_table(struct opp_table *opp_table)
--
2.27.0
Now Internal and External memory controllers are memory interconnection
providers. This allows us to use interconnect API for tuning of memory
configuration. EMC driver now supports OPPs and DVFS. MC driver now
supports tuning of memory arbitration latency, which needs to be done
for ISO memory clients, like a Display client for example.
Tested-by: Peter Geis <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/memory/tegra/Kconfig | 1 +
drivers/memory/tegra/tegra30-emc.c | 184 ++++++++++++++++++++++++++++-
drivers/memory/tegra/tegra30.c | 119 +++++++++++++++++++
3 files changed, 302 insertions(+), 2 deletions(-)
diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
index 61cdb5c04b18..73a5c5bca480 100644
--- a/drivers/memory/tegra/Kconfig
+++ b/drivers/memory/tegra/Kconfig
@@ -23,6 +23,7 @@ config TEGRA30_EMC
tristate "NVIDIA Tegra30 External Memory Controller driver"
default y
depends on TEGRA_MC && ARCH_TEGRA_3x_SOC
+ select PM_OPP
help
This driver is for the External Memory Controller (EMC) found on
Tegra30 chips. The EMC controls the external DRAM on the board.
diff --git a/drivers/memory/tegra/tegra30-emc.c b/drivers/memory/tegra/tegra30-emc.c
index 78f770cf0d64..66eae944ca6d 100644
--- a/drivers/memory/tegra/tegra30-emc.c
+++ b/drivers/memory/tegra/tegra30-emc.c
@@ -14,6 +14,7 @@
#include <linux/debugfs.h>
#include <linux/delay.h>
#include <linux/err.h>
+#include <linux/interconnect-provider.h>
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/iopoll.h>
@@ -21,6 +22,8 @@
#include <linux/module.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
+#include <linux/slab.h>
#include <linux/sort.h>
#include <linux/types.h>
@@ -326,6 +329,8 @@ struct emc_timing {
struct tegra_emc {
struct device *dev;
struct tegra_mc *mc;
+ struct opp_table *opp_table;
+ struct icc_provider provider;
struct notifier_block clk_nb;
struct clk *clk;
void __iomem *regs;
@@ -973,11 +978,12 @@ static int emc_load_timings_from_dt(struct tegra_emc *emc,
return err;
dev_info(emc->dev,
- "got %u timings for RAM code %u (min %luMHz max %luMHz)\n",
+ "got %u timings for RAM code %u (min %luMHz max %luMHz) OPP HW ver. 0x%lx\n",
emc->num_timings,
tegra_read_ram_code(),
emc->timings[0].rate / 1000000,
- emc->timings[emc->num_timings - 1].rate / 1000000);
+ emc->timings[emc->num_timings - 1].rate / 1000000,
+ BIT(tegra_sku_info.soc_speedo_id));
return 0;
}
@@ -1264,6 +1270,172 @@ static void tegra_emc_debugfs_init(struct tegra_emc *emc)
emc, &tegra_emc_debug_max_rate_fops);
}
+static inline struct tegra_emc *
+to_tegra_emc_provider(struct icc_provider *provider)
+{
+ return container_of(provider, struct tegra_emc, provider);
+}
+
+static struct icc_node_data *
+emc_of_icc_xlate_extended(struct of_phandle_args *spec, void *data)
+{
+ struct icc_provider *provider = data;
+ struct icc_node_data *ndata;
+ struct icc_node *node;
+
+ /* External Memory is the only possible ICC route */
+ list_for_each_entry(node, &provider->nodes, node_list) {
+ if (node->id != TEGRA_ICC_EMEM)
+ continue;
+
+ ndata = kzalloc(sizeof(*ndata), GFP_KERNEL);
+ if (!ndata)
+ return ERR_PTR(-ENOMEM);
+
+ /*
+ * SRC and DST nodes should have matching TAG in order to have
+ * it set by default for a requested path.
+ */
+ ndata->tag = TEGRA_MC_ICC_TAG_ISO;
+ ndata->node = node;
+
+ return ndata;
+ }
+
+ return ERR_PTR(-EINVAL);
+}
+
+static int emc_icc_set(struct icc_node *src, struct icc_node *dst)
+{
+ struct tegra_emc *emc = to_tegra_emc_provider(dst->provider);
+ unsigned long long peak_bw = icc_units_to_bps(dst->peak_bw);
+ unsigned long long avg_bw = icc_units_to_bps(dst->avg_bw);
+ unsigned long long rate = max(avg_bw, peak_bw);
+ unsigned int dram_data_bus_width_bytes = 4;
+ unsigned int ddr = 2;
+ long rounded_rate;
+ int err;
+
+ /*
+ * Tegra30 EMC runs on a clock rate of SDRAM bus. This means that
+ * EMC clock rate is twice smaller than the peak data rate because
+ * data is sample on both EMC clock edges.
+ */
+ do_div(rate, ddr * dram_data_bus_width_bytes);
+ rate = min_t(u64, rate, U32_MAX);
+
+ rounded_rate = emc_round_rate(rate, 0, U32_MAX, emc);
+ if (rounded_rate < 0)
+ return rounded_rate;
+
+ err = dev_pm_opp_set_rate(emc->dev, rounded_rate);
+ if (err)
+ return err;
+
+ return 0;
+}
+
+static int tegra_emc_interconnect_init(struct tegra_emc *emc)
+{
+ const struct tegra_mc_soc *soc = emc->mc->soc;
+ struct icc_node *node;
+ int err;
+
+ emc->provider.dev = emc->dev;
+ emc->provider.set = emc_icc_set;
+ emc->provider.data = &emc->provider;
+ emc->provider.aggregate = soc->icc_ops->aggregate;
+ emc->provider.xlate_extended = emc_of_icc_xlate_extended;
+
+ err = icc_provider_add(&emc->provider);
+ if (err)
+ goto err_msg;
+
+ /* create External Memory Controller node */
+ node = icc_node_create(TEGRA_ICC_EMC);
+ err = PTR_ERR_OR_ZERO(node);
+ if (err)
+ goto del_provider;
+
+ node->name = "External Memory Controller";
+ icc_node_add(node, &emc->provider);
+
+ /* link External Memory Controller to External Memory (DRAM) */
+ err = icc_link_create(node, TEGRA_ICC_EMEM);
+ if (err)
+ goto remove_nodes;
+
+ /* create External Memory node */
+ node = icc_node_create(TEGRA_ICC_EMEM);
+ err = PTR_ERR_OR_ZERO(node);
+ if (err)
+ goto remove_nodes;
+
+ node->name = "External Memory (DRAM)";
+ icc_node_add(node, &emc->provider);
+
+ return 0;
+
+remove_nodes:
+ icc_nodes_remove(&emc->provider);
+del_provider:
+ icc_provider_del(&emc->provider);
+err_msg:
+ dev_err(emc->dev, "failed to initialize ICC: %d\n", err);
+
+ return err;
+}
+
+static int tegra_emc_opp_table_init(struct tegra_emc *emc)
+{
+ u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
+ struct opp_table *opp_table;
+ const char *rname = "core";
+ int err;
+
+ /*
+ * Legacy device-trees don't have OPP table and EMC driver isn't
+ * useful in this case.
+ */
+ if (!device_property_present(emc->dev, "operating-points-v2")) {
+ dev_err(emc->dev, "OPP table not found\n");
+ dev_err(emc->dev, "please update your device tree\n");
+ return -ENODEV;
+ }
+
+ /* voltage scaling is optional */
+ if (device_property_present(emc->dev, "core-supply"))
+ emc->opp_table = dev_pm_opp_set_regulators(emc->dev, &rname, 1);
+ else
+ emc->opp_table = dev_pm_opp_get_opp_table(emc->dev);
+
+ if (IS_ERR(emc->opp_table))
+ return dev_err_probe(emc->dev, PTR_ERR(emc->opp_table),
+ "failed to prepare OPP table\n");
+
+ opp_table = dev_pm_opp_set_supported_hw(emc->dev, &hw_version, 1);
+ err = PTR_ERR_OR_ZERO(opp_table);
+ if (err) {
+ dev_err(emc->dev, "failed to set supported HW: %d\n", err);
+ goto put_table;
+ }
+
+ err = dev_pm_opp_of_add_table(emc->dev);
+ if (err) {
+ dev_err(emc->dev, "failed to add OPP table: %d\n", err);
+ goto put_hw;
+ }
+
+ return 0;
+
+put_hw:
+ dev_pm_opp_put_supported_hw(emc->opp_table);
+put_table:
+ dev_pm_opp_put_opp_table(emc->opp_table);
+
+ return err;
+}
+
static int tegra_emc_probe(struct platform_device *pdev)
{
struct device_node *np;
@@ -1329,8 +1501,13 @@ static int tegra_emc_probe(struct platform_device *pdev)
goto unset_cb;
}
+ err = tegra_emc_opp_table_init(emc);
+ if (err)
+ goto unreg_notifier;
+
platform_set_drvdata(pdev, emc);
tegra_emc_debugfs_init(emc);
+ tegra_emc_interconnect_init(emc);
/*
* Don't allow the kernel module to be unloaded. Unloading adds some
@@ -1341,6 +1518,8 @@ static int tegra_emc_probe(struct platform_device *pdev)
return 0;
+unreg_notifier:
+ clk_notifier_unregister(emc->clk, &emc->clk_nb);
unset_cb:
tegra20_clk_set_emc_round_callback(NULL, NULL);
@@ -1398,6 +1577,7 @@ static struct platform_driver tegra_emc_driver = {
.of_match_table = tegra_emc_of_match,
.pm = &tegra_emc_pm_ops,
.suppress_bind_attrs = true,
+ .sync_state = icc_sync_state,
},
};
module_platform_driver(tegra_emc_driver);
diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
index 05780a0c6d39..f4ff967d7a10 100644
--- a/drivers/memory/tegra/tegra30.c
+++ b/drivers/memory/tegra/tegra30.c
@@ -1083,6 +1083,124 @@ static const struct tegra_mc_reset tegra30_mc_resets[] = {
TEGRA30_MC_RESET(VI, 0x200, 0x204, 17),
};
+static void tegra30_mc_tune_client_latency(struct tegra_mc *mc,
+ const struct tegra_mc_client *client,
+ unsigned int bandwidth_mbytes_sec)
+{
+ u32 arb_tolerance_compensation_nsec, arb_tolerance_compensation_div;
+ const struct tegra_mc_la *la = &client->la;
+ unsigned int fifo_size = client->fifo_size;
+ u32 arb_nsec, la_ticks, value;
+
+ /* see 18.4.1 Client Configuration in Tegra3 TRM v03p */
+ if (bandwidth_mbytes_sec)
+ arb_nsec = fifo_size * NSEC_PER_USEC / bandwidth_mbytes_sec;
+ else
+ arb_nsec = U32_MAX;
+
+ /*
+ * Latency allowness should be set with consideration for the module's
+ * latency tolerance and internal buffering capabilities.
+ *
+ * Display memory clients use isochronous transfers and have very low
+ * tolerance to a belated transfers. Hence we need to compensate the
+ * memory arbitration imperfection for them in order to prevent FIFO
+ * underflow condition when memory bus is busy.
+ *
+ * VI clients also need a stronger compensation.
+ */
+ switch (client->swgroup) {
+ case TEGRA_SWGROUP_MPCORE:
+ /* we always want lower latency for CPU, hence don't touch it */
+ return;
+
+ case TEGRA_SWGROUP_DC:
+ case TEGRA_SWGROUP_DCB:
+ arb_tolerance_compensation_nsec = 1050;
+ arb_tolerance_compensation_div = 2;
+ break;
+
+ case TEGRA_SWGROUP_VI:
+ arb_tolerance_compensation_nsec = 1050;
+ arb_tolerance_compensation_div = 1;
+ break;
+
+ default:
+ arb_tolerance_compensation_nsec = 150;
+ arb_tolerance_compensation_div = 1;
+ break;
+ }
+
+ if (arb_nsec > arb_tolerance_compensation_nsec)
+ arb_nsec -= arb_tolerance_compensation_nsec;
+ else
+ arb_nsec = 0;
+
+ arb_nsec /= arb_tolerance_compensation_div;
+
+ /*
+ * Latency allowance is a number of ticks a request from a particular
+ * client may wait in the EMEM arbiter before it becomes a high-priority
+ * request.
+ */
+ la_ticks = arb_nsec / mc->tick;
+ la_ticks = min(la_ticks, la->mask);
+
+ value = mc_readl(mc, la->reg);
+ value &= ~(la->mask << la->shift);
+ value |= la_ticks << la->shift;
+ mc_writel(mc, value, la->reg);
+}
+
+static int tegra30_mc_icc_set(struct icc_node *src, struct icc_node *dst)
+{
+ struct icc_provider *provider = src->provider;
+ struct tegra_mc *mc = container_of(provider, struct tegra_mc, provider);
+ const struct tegra_mc_client *client = &mc->soc->clients[src->id];
+ u64 peak_bandwidth = icc_units_to_bps(src->peak_bw);
+
+ /*
+ * Skip pre-initialization that is done by icc_node_add(), which sets
+ * bandwidth to maximum for all clients before drivers are loaded.
+ *
+ * This doesn't make sense for us because we don't have drivers for all
+ * clients and it's okay to keep configuration left from bootloader
+ * during boot, at least for today.
+ */
+ if (src == dst)
+ return 0;
+
+ /* convert bytes/sec to megabytes/sec */
+ do_div(peak_bandwidth, 1000000);
+
+ tegra30_mc_tune_client_latency(mc, client, peak_bandwidth);
+
+ return 0;
+}
+
+static int tegra30_mc_icc_aggreate(struct icc_node *node, u32 tag, u32 avg_bw,
+ u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
+{
+ /*
+ * ISO clients need to reserve extra bandwidth up-front because
+ * there could high bandwidth pressure during initial fulling-up
+ * of the client's FIFO buffers. Secondly, we need to take into
+ * account impurities of the memory subsystem.
+ */
+ if (tag == TEGRA_MC_ICC_TAG_ISO)
+ peak_bw = tegra_mc_scale_percents(peak_bw, 400);
+
+ *agg_avg += avg_bw;
+ *agg_peak = max(*agg_peak, peak_bw);
+
+ return 0;
+}
+
+static const struct tegra_mc_icc_ops tegra30_mc_icc_ops = {
+ .aggregate = tegra30_mc_icc_aggreate,
+ .set = tegra30_mc_icc_set,
+};
+
const struct tegra_mc_soc tegra30_mc_soc = {
.clients = tegra30_mc_clients,
.num_clients = ARRAY_SIZE(tegra30_mc_clients),
@@ -1097,4 +1215,5 @@ const struct tegra_mc_soc tegra30_mc_soc = {
.reset_ops = &tegra_mc_reset_ops_common,
.resets = tegra30_mc_resets,
.num_resets = ARRAY_SIZE(tegra30_mc_resets),
+ .icc_ops = &tegra30_mc_icc_ops,
};
--
2.27.0
EMC driver will become mandatory after turning it into interconnect
provider because interconnect users, like display controller driver, will
fail to probe using newer device-trees that have interconnect properties.
Thus make EMC driver to probe even if timings are missing in device-tree.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/memory/tegra/tegra20-emc.c | 34 ++++++++++++++----------------
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
index ce22ca7cfb77..34085e26dced 100644
--- a/drivers/memory/tegra/tegra20-emc.c
+++ b/drivers/memory/tegra/tegra20-emc.c
@@ -383,6 +383,11 @@ tegra_emc_find_node_by_ram_code(struct device *dev)
u32 value, ram_code;
int err;
+ if (of_get_child_count(dev->of_node) == 0) {
+ dev_info(dev, "device-tree doesn't have memory timings\n");
+ return NULL;
+ }
+
if (!of_property_read_bool(dev->of_node, "nvidia,use-ram-code"))
return of_node_get(dev->of_node);
@@ -451,6 +456,9 @@ static long emc_round_rate(unsigned long rate,
struct tegra_emc *emc = arg;
unsigned int i;
+ if (!emc->num_timings)
+ return clk_get_rate(emc->clk);
+
min_rate = min(min_rate, emc->timings[emc->num_timings - 1].rate);
for (i = 0; i < emc->num_timings; i++) {
@@ -656,13 +664,6 @@ static int tegra_emc_probe(struct platform_device *pdev)
struct tegra_emc *emc;
int irq, err;
- /* driver has nothing to do in a case of memory timing absence */
- if (of_get_child_count(pdev->dev.of_node) == 0) {
- dev_info(&pdev->dev,
- "EMC device tree node doesn't have memory timings\n");
- return 0;
- }
-
irq = platform_get_irq(pdev, 0);
if (irq < 0) {
dev_err(&pdev->dev, "interrupt not specified\n");
@@ -670,23 +671,20 @@ static int tegra_emc_probe(struct platform_device *pdev)
return irq;
}
- np = tegra_emc_find_node_by_ram_code(&pdev->dev);
- if (!np)
- return -EINVAL;
-
emc = devm_kzalloc(&pdev->dev, sizeof(*emc), GFP_KERNEL);
- if (!emc) {
- of_node_put(np);
+ if (!emc)
return -ENOMEM;
- }
emc->clk_nb.notifier_call = tegra_emc_clk_change_notify;
emc->dev = &pdev->dev;
- err = tegra_emc_load_timings_from_dt(emc, np);
- of_node_put(np);
- if (err)
- return err;
+ np = tegra_emc_find_node_by_ram_code(&pdev->dev);
+ if (np) {
+ err = tegra_emc_load_timings_from_dt(emc, np);
+ of_node_put(np);
+ if (err)
+ return err;
+ }
emc->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(emc->regs))
--
2.27.0
Tegra EMC driver was turned into a regular kernel driver, meaning that it
could be compiled as a loadable kernel module now. Hence EMC clock isn't
guaranteed to be available and clk_get("emc") may return -EPROBE_DEFER.
Let's silence the deferred probe error.
Acked-by: Chanwoo Choi <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/devfreq/tegra30-devfreq.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index f5e74c2ede85..3f732ab53573 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -801,10 +801,9 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
}
tegra->emc_clock = devm_clk_get(&pdev->dev, "emc");
- if (IS_ERR(tegra->emc_clock)) {
- dev_err(&pdev->dev, "Failed to get emc clock\n");
- return PTR_ERR(tegra->emc_clock);
- }
+ if (IS_ERR(tegra->emc_clock))
+ return dev_err_probe(&pdev->dev, PTR_ERR(tegra->emc_clock),
+ "Failed to get emc clock\n");
err = platform_get_irq(pdev, 0);
if (err < 0)
--
2.27.0
Add interconnect properties to the Memory Controller, External Memory
Controller and the Display Controller nodes in order to describe hardware
interconnection.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
arch/arm/boot/dts/tegra20.dtsi | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 9347f7789245..2e1304493f7d 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -111,6 +111,17 @@ dc@54200000 {
nvidia,head = <0>;
+ interconnects = <&mc TEGRA20_MC_DISPLAY0A &emc>,
+ <&mc TEGRA20_MC_DISPLAY0B &emc>,
+ <&mc TEGRA20_MC_DISPLAY1B &emc>,
+ <&mc TEGRA20_MC_DISPLAY0C &emc>,
+ <&mc TEGRA20_MC_DISPLAYHC &emc>;
+ interconnect-names = "wina",
+ "winb",
+ "winb-vfilter",
+ "winc",
+ "cursor";
+
rgb {
status = "disabled";
};
@@ -128,6 +139,17 @@ dc@54240000 {
nvidia,head = <1>;
+ interconnects = <&mc TEGRA20_MC_DISPLAY0AB &emc>,
+ <&mc TEGRA20_MC_DISPLAY0BB &emc>,
+ <&mc TEGRA20_MC_DISPLAY1BB &emc>,
+ <&mc TEGRA20_MC_DISPLAY0CB &emc>,
+ <&mc TEGRA20_MC_DISPLAYHCB &emc>;
+ interconnect-names = "wina",
+ "winb",
+ "winb-vfilter",
+ "winc",
+ "cursor";
+
rgb {
status = "disabled";
};
@@ -630,15 +652,17 @@ mc: memory-controller@7000f000 {
interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
#reset-cells = <1>;
#iommu-cells = <0>;
+ #interconnect-cells = <1>;
};
- memory-controller@7000f400 {
+ emc: memory-controller@7000f400 {
compatible = "nvidia,tegra20-emc";
reg = <0x7000f400 0x400>;
interrupts = <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&tegra_car TEGRA20_CLK_EMC>;
#address-cells = <1>;
#size-cells = <0>;
+ #interconnect-cells = <0>;
};
fuse@7000f800 {
--
2.27.0
Most of Host1x devices have at least one memory client. These clients
are directly connected to the memory controller. The new interconnect
properties represent the memory client's connection to the memory
controller.
Reviewed-by: Rob Herring <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
.../display/tegra/nvidia,tegra20-host1x.txt | 68 +++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
index ac63ae4a3861..814246e51954 100644
--- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
+++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
@@ -20,6 +20,10 @@ Required properties:
- reset-names: Must include the following entries:
- host1x
+Each host1x client module having to perform DMA through the Memory Controller
+should have the interconnect endpoints set to the Memory Client and External
+Memory respectively.
+
The host1x top-level node defines a number of children, each representing one
of the following host1x client modules:
@@ -36,6 +40,12 @@ of the following host1x client modules:
- reset-names: Must include the following entries:
- mpe
+ Optional properties:
+ - interconnects: Must contain entry for the MPE memory clients.
+ - interconnect-names: Must include name of the interconnect path for each
+ interconnect entry. Consult TRM documentation for information about
+ available memory clients, see MEMORY CONTROLLER section.
+
- vi: video input
Required properties:
@@ -113,6 +123,12 @@ of the following host1x client modules:
Required properties:
- remote-endpoint: phandle to vi port 'endpoint' node.
+ Optional properties:
+ - interconnects: Must contain entry for the VI memory clients.
+ - interconnect-names: Must include name of the interconnect path for each
+ interconnect entry. Consult TRM documentation for information about
+ available memory clients, see MEMORY CONTROLLER section.
+
- epp: encoder pre-processor
Required properties:
@@ -126,6 +142,12 @@ of the following host1x client modules:
- reset-names: Must include the following entries:
- epp
+ Optional properties:
+ - interconnects: Must contain entry for the EPP memory clients.
+ - interconnect-names: Must include name of the interconnect path for each
+ interconnect entry. Consult TRM documentation for information about
+ available memory clients, see MEMORY CONTROLLER section.
+
- isp: image signal processor
Required properties:
@@ -139,6 +161,12 @@ of the following host1x client modules:
- reset-names: Must include the following entries:
- isp
+ Optional properties:
+ - interconnects: Must contain entry for the ISP memory clients.
+ - interconnect-names: Must include name of the interconnect path for each
+ interconnect entry. Consult TRM documentation for information about
+ available memory clients, see MEMORY CONTROLLER section.
+
- gr2d: 2D graphics engine
Required properties:
@@ -152,6 +180,12 @@ of the following host1x client modules:
- reset-names: Must include the following entries:
- 2d
+ Optional properties:
+ - interconnects: Must contain entry for the GR2D memory clients.
+ - interconnect-names: Must include name of the interconnect path for each
+ interconnect entry. Consult TRM documentation for information about
+ available memory clients, see MEMORY CONTROLLER section.
+
- gr3d: 3D graphics engine
Required properties:
@@ -170,6 +204,12 @@ of the following host1x client modules:
- 3d
- 3d2 (Only required on SoCs with two 3D clocks)
+ Optional properties:
+ - interconnects: Must contain entry for the GR3D memory clients.
+ - interconnect-names: Must include name of the interconnect path for each
+ interconnect entry. Consult TRM documentation for information about
+ available memory clients, see MEMORY CONTROLLER section.
+
- dc: display controller
Required properties:
@@ -197,6 +237,10 @@ of the following host1x client modules:
- nvidia,hpd-gpio: specifies a GPIO used for hotplug detection
- nvidia,edid: supplies a binary EDID blob
- nvidia,panel: phandle of a display panel
+ - interconnects: Must contain entry for the DC memory clients.
+ - interconnect-names: Must include name of the interconnect path for each
+ interconnect entry. Consult TRM documentation for information about
+ available memory clients, see MEMORY CONTROLLER section.
- hdmi: High Definition Multimedia Interface
@@ -345,6 +389,12 @@ of the following host1x client modules:
- reset-names: Must include the following entries:
- vic
+ Optional properties:
+ - interconnects: Must contain entry for the VIC memory clients.
+ - interconnect-names: Must include name of the interconnect path for each
+ interconnect entry. Consult TRM documentation for information about
+ available memory clients, see MEMORY CONTROLLER section.
+
Example:
/ {
@@ -498,6 +548,15 @@ Example:
resets = <&tegra_car 27>;
reset-names = "dc";
+ interconnects = <&mc TEGRA20_MC_DISPLAY0A &emc>,
+ <&mc TEGRA20_MC_DISPLAY0B &emc>,
+ <&mc TEGRA20_MC_DISPLAY0C &emc>,
+ <&mc TEGRA20_MC_DISPLAY1B &emc>;
+ interconnect-names = "wina",
+ "winb",
+ "winc",
+ "cursor";
+
rgb {
status = "disabled";
};
@@ -513,6 +572,15 @@ Example:
resets = <&tegra_car 26>;
reset-names = "dc";
+ interconnects = <&mc TEGRA20_MC_DISPLAY0AB &emc>,
+ <&mc TEGRA20_MC_DISPLAY0BB &emc>,
+ <&mc TEGRA20_MC_DISPLAY0CB &emc>,
+ <&mc TEGRA20_MC_DISPLAY1BB &emc>;
+ interconnect-names = "wina",
+ "winb",
+ "winc",
+ "cursor";
+
rgb {
status = "disabled";
};
--
2.27.0
Add interconnect properties to the Memory Controller, External Memory
Controller and the Display Controller nodes in order to describe hardware
interconnection.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
arch/arm/boot/dts/tegra124.dtsi | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 64f488ba1e72..1801e30b1d3a 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -113,6 +113,19 @@ dc@54200000 {
iommus = <&mc TEGRA_SWGROUP_DC>;
nvidia,head = <0>;
+
+ interconnects = <&mc TEGRA124_MC_DISPLAY0A &emc>,
+ <&mc TEGRA124_MC_DISPLAY0B &emc>,
+ <&mc TEGRA124_MC_DISPLAY0C &emc>,
+ <&mc TEGRA124_MC_DISPLAYHC &emc>,
+ <&mc TEGRA124_MC_DISPLAYD &emc>,
+ <&mc TEGRA124_MC_DISPLAYT &emc>;
+ interconnect-names = "wina",
+ "winb",
+ "winc",
+ "cursor",
+ "wind",
+ "wint";
};
dc@54240000 {
@@ -127,6 +140,15 @@ dc@54240000 {
iommus = <&mc TEGRA_SWGROUP_DCB>;
nvidia,head = <1>;
+
+ interconnects = <&mc TEGRA124_MC_DISPLAY0AB &emc>,
+ <&mc TEGRA124_MC_DISPLAY0BB &emc>,
+ <&mc TEGRA124_MC_DISPLAY0CB &emc>,
+ <&mc TEGRA124_MC_DISPLAYHCB &emc>;
+ interconnect-names = "wina",
+ "winb",
+ "winc",
+ "cursor";
};
hdmi: hdmi@54280000 {
@@ -628,6 +650,7 @@ mc: memory-controller@70019000 {
#iommu-cells = <1>;
#reset-cells = <1>;
+ #interconnect-cells = <1>;
};
emc: external-memory-controller@7001b000 {
@@ -637,6 +660,8 @@ emc: external-memory-controller@7001b000 {
clock-names = "emc";
nvidia,memory-controller = <&mc>;
+
+ #interconnect-cells = <0>;
};
sata@70020000 {
--
2.27.0
Document new OPP table and voltage regulator properties which are needed
for supporting dynamic voltage-frequency scaling of the memory controller.
Some boards may have a fixed core voltage regulator, hence it's optional
because frequency scaling still may be desired.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
.../memory-controllers/nvidia,tegra30-emc.yaml | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-emc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-emc.yaml
index c243986db420..0a2e2c0d0fdd 100644
--- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-emc.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-emc.yaml
@@ -39,6 +39,15 @@ properties:
description:
Phandle of the Memory Controller node.
+ core-supply:
+ description:
+ Phandle of voltage regulator of the SoC "core" power domain.
+
+ operating-points-v2:
+ description:
+ Should contain freqs and voltages and opp-supported-hw property, which
+ is a bitfield indicating SoC speedo ID mask.
+
patternProperties:
"^emc-timings-[0-9]+$":
type: object
@@ -218,6 +227,7 @@ required:
- clocks
- nvidia,memory-controller
- "#interconnect-cells"
+ - operating-points-v2
additionalProperties: false
@@ -230,6 +240,8 @@ examples:
clocks = <&tegra_car 57>;
nvidia,memory-controller = <&mc>;
+ operating-points-v2 = <&dvfs_opp_table>;
+ core-supply = <&vdd_core>;
#interconnect-cells = <0>;
--
2.27.0
External Memory Controller is interconnected with memory controller and
with external memory. Document new interconnect property which turns EMC
into interconnect provider.
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
.../bindings/memory-controllers/nvidia,tegra20-emc.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
index 1b0d4417aad8..0a53adc6ccba 100644
--- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
+++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
@@ -13,6 +13,7 @@ Properties:
- interrupts : Should contain EMC General interrupt.
- clocks : Should contain EMC clock.
- nvidia,memory-controller : Phandle of the Memory Controller node.
+- #interconnect-cells : Should be 0.
Child device nodes describe the memory settings for different configurations and clock rates.
@@ -21,6 +22,7 @@ Example:
memory-controller@7000f400 {
#address-cells = < 1 >;
#size-cells = < 0 >;
+ #interconnect-cells = < 0 >;
compatible = "nvidia,tegra20-emc";
reg = <0x7000f400 0x400>;
interrupts = <0 78 0x04>;
--
2.27.0
On 10/26/20 7:17 AM, Dmitry Osipenko wrote:
> Tegra EMC driver was turned into a regular kernel driver, meaning that it
> could be compiled as a loadable kernel module now. Hence EMC clock isn't
> guaranteed to be available and clk_get("emc") may return -EPROBE_DEFER.
> Let's silence the deferred probe error.
>
> Acked-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/tegra20-devfreq.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c
> index ff82bac9ee4e..fd801534771d 100644
> --- a/drivers/devfreq/tegra20-devfreq.c
> +++ b/drivers/devfreq/tegra20-devfreq.c
> @@ -141,11 +141,9 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>
> /* EMC is a system-critical clock that is always enabled */
> tegra->emc_clock = devm_clk_get(&pdev->dev, "emc");
> - if (IS_ERR(tegra->emc_clock)) {
> - err = PTR_ERR(tegra->emc_clock);
> - dev_err(&pdev->dev, "failed to get emc clock: %d\n", err);
> - return err;
> - }
> + if (IS_ERR(tegra->emc_clock))
> + return dev_err_probe(&pdev->dev, PTR_ERR(tegra->emc_clock),
> + "failed to get emc clock\n");
>
> tegra->regs = mc->regs;
>
>
Applied it. Thanks.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On 10/26/20 7:17 AM, Dmitry Osipenko wrote:
> Tegra EMC driver was turned into a regular kernel driver, meaning that it
> could be compiled as a loadable kernel module now. Hence EMC clock isn't
> guaranteed to be available and clk_get("emc") may return -EPROBE_DEFER.
> Let's silence the deferred probe error.
>
> Acked-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/tegra30-devfreq.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index f5e74c2ede85..3f732ab53573 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -801,10 +801,9 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> }
>
> tegra->emc_clock = devm_clk_get(&pdev->dev, "emc");
> - if (IS_ERR(tegra->emc_clock)) {
> - dev_err(&pdev->dev, "Failed to get emc clock\n");
> - return PTR_ERR(tegra->emc_clock);
> - }
> + if (IS_ERR(tegra->emc_clock))
> + return dev_err_probe(&pdev->dev, PTR_ERR(tegra->emc_clock),
> + "Failed to get emc clock\n");
>
> err = platform_get_irq(pdev, 0);
> if (err < 0)
>
Applied it. Thanks.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On Mon, Oct 26, 2020 at 01:16:43AM +0300, Dmitry Osipenko wrote:
> Hello,
>
> This series brings initial support for memory interconnect to Tegra20,
> Tegra30 and Tegra124 SoCs.
>
> For the starter only display controllers and devfreq devices are getting
> interconnect API support, others could be supported later on. The display
> controllers have the biggest demand for interconnect API right now because
> dynamic memory frequency scaling can't be done safely without taking into
> account bandwidth requirement from the displays. In particular this series
> fixes distorted display output on T30 Ouya and T124 TK1 devices.
Hi,
You introduced in v6 multiple new patches. Could you describe the
dependencies, if any?
Best regards,
Krzysztof
> Changelog:
>
> v6: - This series was massively reworked in comparison to v5, most of the
> patches that previously got r-b need a new round of a review (!).
>
> - Added missed clk-rounding to the set() callback of EMC ICC providers.
> Now clk_set_min_rate() doesn't error out on rate overflow.
>
> - Now peak bandwidth is properly taken into account by the set() callback
> of EMC ICC providers.
>
> - EMC runs at 2x of the DRAM bus only on Tegra20, this now taken in account
> properly by the EMC ICC set() callbacks.
>
> - ICC drivers use new icc_sync_state() and xlate_extended().
>
> - ICC drivers support new TEGRA_MC_ICC_TAG_ISO for ICC paths, which
> conveys to ICC driver that memory path uses isochronous transfers.
>
> - Added support for memory latency scaling to Tegra30 ICC provider.
> It's required for fixing display FIFO underflows on T30.
>
> - Added basic interconnect support to Tegra124 drivers.
>
> - Tegra20/30/124 EMC drivers now support voltage scaling using generic
> OPP API.
>
> - The display bandwidth management is reworked and improved. It now
> supports both bandwidth and latency allocations. The nv-display is
> now also taken into account properly, i.e. it's kept untouched.
> The extra bandwidth reservation required for ISO clients is moved
> from DC driver to the ICC drivers.
>
> - Dropped patch that tuned T20 display controller memory client because
> turned out that it kills ~30% of memory bandwidth. It should be possible
> to support client tuning, but it's too complicated for now.
>
> - Corrected display's cursor and winb-vfilter ICC clients.
> The winb-vfilter was erroneously used in place of cursor's client
> in device-trees.
>
> - Added devm_tegra_get_memory_controller() and switched drivers to
> use it.
>
> - Device-tree OPP tables are now supported by memory and devfreq
> drivers.
>
> - Tegra20-devfeq driver is reworked and now uses EMC-stats instead
> of IMC-stats (which are nearly identical modules) because previously
> I failed to understand how EMC-stats work and succeeded this time,
> thinking that it simply doesn't work. This removes a bit icky dependency
> on using both EMC and MC drivers simultaneously by the devfreq driver.
>
> - Tegra20-devfeq driver now is a sub-device of the EMC, it also now uses
> interconnect API for driving memory bandwidth.
>
> - Tegra30-devfreq got interconnect support.
>
> - Devfreq patches now use dev_err_probe(), which was suggested by
> Chanwoo Choi.
>
> - Added acks from Chanwoo Choi and Rob Herring to the reviewed and
> unchanged patches.
>
> - Added tested-by from Peter Geis and Nicolas Chauvet, who tested this
> series on Ouya and TK1 devices, reporting that it fixes display
> corruption on these devices which happened due to insufficient memory
> bandwidth.
>
> - Added patches to fix T20 EMC registers size.
>
> - Fixed missing LA entry for PTC in the Tegra MC drivers.
>
> - New and updated patches in v6:
>
> dt-bindings: memory: tegra20: emc: Correct registers range in example
> dt-bindings: memory: tegra20: emc: Document nvidia,memory-controller property
> dt-bindings: memory: tegra20: emc: Document OPP table and voltage regulator
> dt-bindings: memory: tegra20: emc: Document mfd-simple compatible and statistics sub-device
> dt-bindings: memory: tegra30: emc: Document OPP table and voltage regulator
> dt-bindings: memory: tegra124: mc: Document new interconnect property
> dt-bindings: memory: tegra124: emc: Document new interconnect property
> dt-bindings: memory: tegra124: emc: Document OPP table and voltage regulator
> dt-bindings: tegra30-actmon: Document OPP and interconnect properties
> dt-bindings: memory: tegra124: Add memory client IDs
> ARM: tegra: Correct EMC registers size in Tegra20 device-tree
> ARM: tegra: Add interconnect properties to Tegra124 device-tree
> ARM: tegra: Add nvidia,memory-controller phandle to Tegra20 EMC device-tree
> ARM: tegra: Add DVFS properties to Tegra20 EMC device-tree node
> ARM: tegra: Add DVFS properties to Tegra30 EMC and ACTMON device-tree nodes
> ARM: tegra: Add DVFS properties to Tegra124 EMC and ACTMON device-tree nodes
> memory: tegra: Add and use devm_tegra_get_memory_controller()
> memory: tegra-mc: Add interconnect framework
> memory: tegra20: Support interconnect framework
> memory: tegra20-emc: Skip parsing of emc-stats DT sub-node
> memory: tegra: Add missing latency allowness entry for Page Table Cache
> memory: tegra: Add FIFO sizes to Tegra30 memory clients
> memory: tegra30: Support interconnect framework
> memory: tegra124-emc: Make driver modular
> memory: tegra124: Support interconnect framework
> memory: tegra: Remove superfluous error messages around platform_get_irq()
> drm/tegra: dc: Support memory bandwidth management
> drm/tegra: dc: Extend debug stats with total number of events
> PM / devfreq: tegra20: Convert to EMC_STAT driver, support interconnect and device-tree
> PM / devfreq: tegra30: Support interconnect and OPPs from device-tree
> PM / devfreq: tegra30: Separate configurations per-SoC generation
> opp: Put interconnect paths outside of opp_table_lock
26.10.2020 18:08, Krzysztof Kozlowski пишет:
> On Mon, Oct 26, 2020 at 01:16:43AM +0300, Dmitry Osipenko wrote:
>> Hello,
>>
>> This series brings initial support for memory interconnect to Tegra20,
>> Tegra30 and Tegra124 SoCs.
>>
>> For the starter only display controllers and devfreq devices are getting
>> interconnect API support, others could be supported later on. The display
>> controllers have the biggest demand for interconnect API right now because
>> dynamic memory frequency scaling can't be done safely without taking into
>> account bandwidth requirement from the displays. In particular this series
>> fixes distorted display output on T30 Ouya and T124 TK1 devices.
>
> Hi,
>
> You introduced in v6 multiple new patches. Could you describe the
> dependencies, if any?
Hello,
The v6 dropped some older patches and replaced them with the new
patches. Previously I wanted to postpone the more complex changes for
later times, like supporting OPP tables and DVFS, but then the review
started to take more time than was expected and there was enough time to
complete those features.
There are five basic sets of patches:
1. DT bindings
2. DT changes
3. SoC, clk and memory
4. devfreq
5. DRM
Each set could be applied separately.
Memory patches have a build dependency on the SoC and clk patches.
The "tegra-mc: Add interconnect framework" and "Add and use
devm_tegra_get_memory_controller()" are the root build dependencies for
all memory/ patches. Other patches are grouped per SoC generation
(Tegra20/30/124), patches within a SoC-gen group are interdependent.
I suppose the best variant would be to merge the whole series via
tegra-tree in order to preserve logical order of the patches. Although,
merging each set of patches separately also should be okay to do.
On Mon, Oct 26, 2020 at 01:16:47AM +0300, Dmitry Osipenko wrote:
> Tegra20 External Memory Controller talks to DRAM chips and it needs to be
> reprogrammed when memory frequency changes. Tegra Memory Controller sits
> behind EMC and these controllers are tightly coupled. This patch adds the
> new phandle property which allows to properly express connection of EMC
> and MC hardware in a device-tree, it also put the Tegra20 EMC binding on
> par with Tegra30+ EMC bindings, which is handy to have.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> .../bindings/memory-controllers/nvidia,tegra20-emc.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> index 567cffd37f3f..1b0d4417aad8 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> @@ -12,6 +12,7 @@ Properties:
> irrespective of ram-code configuration.
> - interrupts : Should contain EMC General interrupt.
> - clocks : Should contain EMC clock.
> +- nvidia,memory-controller : Phandle of the Memory Controller node.
It looks like you adding a required property which is an ABI break.
Best regards,
Krzysztof
On Mon, Oct 26, 2020 at 01:16:51AM +0300, Dmitry Osipenko wrote:
> External Memory Controller can gather various hardware statistics that
> are intended to be used for debugging purposes and for dynamic frequency
> scaling of memory bus.
>
> Document the new mfd-simple compatible and EMC statistics sub-device.
> The subdev contains EMC DFS OPP table and interconnect paths to be used
> for dynamic scaling of system's memory bandwidth based on EMC utilization
> statistics.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> .../memory-controllers/nvidia,tegra20-emc.txt | 43 +++++++++++++++++--
> 1 file changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> index 8d09b228ac42..382aabcd6952 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> @@ -4,7 +4,7 @@ Properties:
> - name : Should be emc
> - #address-cells : Should be 1
> - #size-cells : Should be 0
> -- compatible : Should contain "nvidia,tegra20-emc".
> +- compatible : Should contain "nvidia,tegra20-emc" and "simple-mfd".
Changing a compatible match is another break of ABI, unless it is not
really required. It's unclear to me from the contents of the patch.
> - reg : Offset and length of the register set for the device
> - nvidia,use-ram-code : If present, the sub-nodes will be addressed
> and chosen using the ramcode board selector. If omitted, only one
> @@ -17,7 +17,8 @@ Properties:
> - core-supply: Phandle of voltage regulator of the SoC "core" power domain.
> - operating-points-v2: See ../bindings/opp/opp.txt for details.
>
> -Child device nodes describe the memory settings for different configurations and clock rates.
> +Child device nodes describe the memory settings for different configurations and clock rates,
> +as well as EMC activity statistics collection sub-device.
>
> Example:
>
> @@ -31,17 +32,34 @@ Example:
> ...
> };
>
> + emc_bw_dfs_opp_table: emc_opp_table1 {
Hyphens for node name.
Best regards,
Krzysztof
On Mon, Oct 26, 2020 at 01:17:04AM +0300, Dmitry Osipenko wrote:
> Add interconnect properties to the Memory Controller, External Memory
> Controller and the Display Controller nodes in order to describe hardware
> interconnection.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> arch/arm/boot/dts/tegra20.dtsi | 26 +++++++++++++++++++++++++-
> 1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 9347f7789245..2e1304493f7d 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -111,6 +111,17 @@ dc@54200000 {
>
> nvidia,head = <0>;
>
> + interconnects = <&mc TEGRA20_MC_DISPLAY0A &emc>,
I think you just added the defines and did not include them here, so
this should not even build. Did you test it?
Best regards,
Krzysztof
On Mon, Oct 26, 2020 at 01:17:06AM +0300, Dmitry Osipenko wrote:
> Add interconnect properties to the Memory Controller, External Memory
> Controller and the Display Controller nodes in order to describe hardware
> interconnection.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> arch/arm/boot/dts/tegra124.dtsi | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
> index 64f488ba1e72..1801e30b1d3a 100644
> --- a/arch/arm/boot/dts/tegra124.dtsi
> +++ b/arch/arm/boot/dts/tegra124.dtsi
> @@ -113,6 +113,19 @@ dc@54200000 {
> iommus = <&mc TEGRA_SWGROUP_DC>;
>
> nvidia,head = <0>;
> +
> + interconnects = <&mc TEGRA124_MC_DISPLAY0A &emc>,
This does not compile.
> + <&mc TEGRA124_MC_DISPLAY0B &emc>,
> + <&mc TEGRA124_MC_DISPLAY0C &emc>,
> + <&mc TEGRA124_MC_DISPLAYHC &emc>,
> + <&mc TEGRA124_MC_DISPLAYD &emc>,
> + <&mc TEGRA124_MC_DISPLAYT &emc>;
> + interconnect-names = "wina",
> + "winb",
> + "winc",
> + "cursor",
> + "wind",
> + "wint";
> };
>
> dc@54240000 {
> @@ -127,6 +140,15 @@ dc@54240000 {
> iommus = <&mc TEGRA_SWGROUP_DCB>;
>
> nvidia,head = <1>;
> +
> + interconnects = <&mc TEGRA124_MC_DISPLAY0AB &emc>,
> + <&mc TEGRA124_MC_DISPLAY0BB &emc>,
> + <&mc TEGRA124_MC_DISPLAY0CB &emc>,
> + <&mc TEGRA124_MC_DISPLAYHCB &emc>;
> + interconnect-names = "wina",
> + "winb",
> + "winc",
> + "cursor";
> };
>
> hdmi: hdmi@54280000 {
> @@ -628,6 +650,7 @@ mc: memory-controller@70019000 {
>
> #iommu-cells = <1>;
> #reset-cells = <1>;
> + #interconnect-cells = <1>;
> };
>
> emc: external-memory-controller@7001b000 {
> @@ -637,6 +660,8 @@ emc: external-memory-controller@7001b000 {
> clock-names = "emc";
>
> nvidia,memory-controller = <&mc>;
> +
No need for blank line.
Best regards,
Krzysztof
On Tue, Oct 27, 2020 at 10:12:47AM +0100, Krzysztof Kozlowski wrote:
> On Mon, Oct 26, 2020 at 01:17:04AM +0300, Dmitry Osipenko wrote:
> > Add interconnect properties to the Memory Controller, External Memory
> > Controller and the Display Controller nodes in order to describe hardware
> > interconnection.
> >
> > Signed-off-by: Dmitry Osipenko <[email protected]>
> > ---
> > arch/arm/boot/dts/tegra20.dtsi | 26 +++++++++++++++++++++++++-
> > 1 file changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> > index 9347f7789245..2e1304493f7d 100644
> > --- a/arch/arm/boot/dts/tegra20.dtsi
> > +++ b/arch/arm/boot/dts/tegra20.dtsi
> > @@ -111,6 +111,17 @@ dc@54200000 {
> >
> > nvidia,head = <0>;
> >
> > + interconnects = <&mc TEGRA20_MC_DISPLAY0A &emc>,
>
> I think you just added the defines and did not include them here, so
> this should not even build. Did you test it?
The dt-bindings/memory/tegra20-mc.h header is already included in
existing DTS files for MC hot flush resets, so this should be fine.
Thierry
On Mon, Oct 26, 2020 at 01:17:15AM +0300, Dmitry Osipenko wrote:
> EMC driver will become mandatory after turning it into interconnect
> provider because interconnect users, like display controller driver, will
> fail to probe using newer device-trees that have interconnect properties.
> Thus make EMC driver to probe even if timings are missing in device-tree.
Does it really have to be mandatory? Sounds like that's going to make it
unnecessarily complicated to merge all of this. Is it complicated to
make interconnect support optional in consumer drivers?
Thierry
On 26-10-20, 01:17, Dmitry Osipenko wrote:
> This patch fixes lockup which happens when OPP table is released if
> interconnect provider uses OPP in the icc_provider->set() callback
> and bandwidth of the ICC path is set to 0 by the ICC core when path
> is released. The icc_put() doesn't need the opp_table_lock protection,
> hence let's move it outside of the lock in order to resolve the problem.
>
> In particular this fixes tegra-devfreq driver lockup on trying to unload
> the driver module. The devfreq driver uses OPP-bandwidth API and its ICC
> provider also uses OPP for DVFS, hence they both take same opp_table_lock
> when OPP table of the devfreq is released.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/opp/core.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 2483e765318a..1134df360fe0 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1187,12 +1187,6 @@ static void _opp_table_kref_release(struct kref *kref)
> if (!IS_ERR(opp_table->clk))
> clk_put(opp_table->clk);
>
> - if (opp_table->paths) {
> - for (i = 0; i < opp_table->path_count; i++)
> - icc_put(opp_table->paths[i]);
> - kfree(opp_table->paths);
> - }
> -
> WARN_ON(!list_empty(&opp_table->opp_list));
>
> list_for_each_entry_safe(opp_dev, temp, &opp_table->dev_list, node) {
> @@ -1209,9 +1203,22 @@ static void _opp_table_kref_release(struct kref *kref)
> mutex_destroy(&opp_table->genpd_virt_dev_lock);
> mutex_destroy(&opp_table->lock);
> list_del(&opp_table->node);
> - kfree(opp_table);
>
> mutex_unlock(&opp_table_lock);
> +
> + /*
> + * Interconnect provider may use OPP too, hence icc_put() needs to be
> + * invoked outside of the opp_table_lock in order to prevent nested
> + * locking which happens when bandwidth of the ICC path is set to 0
> + * by ICC core on release of the path.
> + */
> + if (opp_table->paths) {
> + for (i = 0; i < opp_table->path_count; i++)
> + icc_put(opp_table->paths[i]);
> + kfree(opp_table->paths);
> + }
> +
> + kfree(opp_table);
> }
Never make such _fixes_ part of such a big patchset. Always send them
separately.
Having said that, I already have a patch with me which shall fix it for you as
well:
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 4ac4e7ce6b8b..0e0a5269dc82 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1181,6 +1181,10 @@ static void _opp_table_kref_release(struct kref *kref)
struct opp_device *opp_dev, *temp;
int i;
+ /* Drop the lock as soon as we can */
+ list_del(&opp_table->node);
+ mutex_unlock(&opp_table_lock);
+
_of_clear_opp_table(opp_table);
/* Release clk */
@@ -1208,10 +1212,7 @@ static void _opp_table_kref_release(struct kref *kref)
mutex_destroy(&opp_table->genpd_virt_dev_lock);
mutex_destroy(&opp_table->lock);
- list_del(&opp_table->node);
kfree(opp_table);
-
- mutex_unlock(&opp_table_lock);
}
void dev_pm_opp_put_opp_table(struct opp_table *opp_table)
--
viresh
On Mon, Oct 26, 2020 at 10:14:10PM +0300, Dmitry Osipenko wrote:
> 26.10.2020 18:08, Krzysztof Kozlowski пишет:
> > On Mon, Oct 26, 2020 at 01:16:43AM +0300, Dmitry Osipenko wrote:
> >> Hello,
> >>
> >> This series brings initial support for memory interconnect to Tegra20,
> >> Tegra30 and Tegra124 SoCs.
> >>
> >> For the starter only display controllers and devfreq devices are getting
> >> interconnect API support, others could be supported later on. The display
> >> controllers have the biggest demand for interconnect API right now because
> >> dynamic memory frequency scaling can't be done safely without taking into
> >> account bandwidth requirement from the displays. In particular this series
> >> fixes distorted display output on T30 Ouya and T124 TK1 devices.
> >
> > Hi,
> >
> > You introduced in v6 multiple new patches. Could you describe the
> > dependencies, if any?
>
> Hello,
>
> The v6 dropped some older patches and replaced them with the new
> patches. Previously I wanted to postpone the more complex changes for
> later times, like supporting OPP tables and DVFS, but then the review
> started to take more time than was expected and there was enough time to
> complete those features.
>
> There are five basic sets of patches:
>
> 1. DT bindings
> 2. DT changes
> 3. SoC, clk and memory
> 4. devfreq
> 5. DRM
>
> Each set could be applied separately.
>
> Memory patches have a build dependency on the SoC and clk patches.
>
> The "tegra-mc: Add interconnect framework" and "Add and use
> devm_tegra_get_memory_controller()" are the root build dependencies for
> all memory/ patches. Other patches are grouped per SoC generation
> (Tegra20/30/124), patches within a SoC-gen group are interdependent.
>
> I suppose the best variant would be to merge the whole series via
> tegra-tree in order to preserve logical order of the patches. Although,
> merging each set of patches separately also should be okay to do.
Thanks for explanation. I already have three patches for Tegra MC (and
probably two more will be respun) so this might be conflict-prone. The
easiest in such case is to provide me soc and clk patches on the branch,
so I could keep all Tegra MC together.
Best regards,
Krzysztof
On Mon, Oct 26, 2020 at 01:16:49AM +0300, Dmitry Osipenko wrote:
> External Memory Controller is interconnected with memory controller and
> with external memory. Document new interconnect property which turns EMC
> into interconnect provider.
>
> Acked-by: Rob Herring <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> .../bindings/memory-controllers/nvidia,tegra20-emc.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> index 1b0d4417aad8..0a53adc6ccba 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> @@ -13,6 +13,7 @@ Properties:
> - interrupts : Should contain EMC General interrupt.
> - clocks : Should contain EMC clock.
> - nvidia,memory-controller : Phandle of the Memory Controller node.
> +- #interconnect-cells : Should be 0.
>
> Child device nodes describe the memory settings for different configurations and clock rates.
>
> @@ -21,6 +22,7 @@ Example:
> memory-controller@7000f400 {
> #address-cells = < 1 >;
> #size-cells = < 0 >;
> + #interconnect-cells = < 0 >;
No spaces within <>. I see existing example, but bad pattern should not
be continued just because it is already there.
Best regards,
Krzysztof
On Mon, Oct 26, 2020 at 01:17:08AM +0300, Dmitry Osipenko wrote:
> Add EMC OPP DVFS/DFS tables and emc-stats subdev that will be used for
> dynamic memory bandwidth scaling, while EMC itself will perform voltage
> scaling. Update board device-trees with optional EMC core supply and
> remove unsupported OPPs.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> .../boot/dts/tegra20-acer-a500-picasso.dts | 12 ++
> arch/arm/boot/dts/tegra20-colibri.dtsi | 8 +
> arch/arm/boot/dts/tegra20-paz00.dts | 10 +
> .../arm/boot/dts/tegra20-peripherals-opp.dtsi | 181 ++++++++++++++++++
> arch/arm/boot/dts/tegra20.dtsi | 12 +-
> 5 files changed, 222 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm/boot/dts/tegra20-peripherals-opp.dtsi
>
> diff --git a/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts b/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
> index a0b829738e8f..f5c1591c8ea8 100644
> --- a/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
> +++ b/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
> @@ -1058,9 +1058,21 @@ map0 {
> };
> };
>
> + emc_opp_table0 {
All node names with hyphens -. Not underscores.
Best regards,
Krzysztof
On Mon, Oct 26, 2020 at 01:16:51AM +0300, Dmitry Osipenko wrote:
> External Memory Controller can gather various hardware statistics that
> are intended to be used for debugging purposes and for dynamic frequency
> scaling of memory bus.
>
> Document the new mfd-simple compatible and EMC statistics sub-device.
> The subdev contains EMC DFS OPP table and interconnect paths to be used
> for dynamic scaling of system's memory bandwidth based on EMC utilization
> statistics.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> .../memory-controllers/nvidia,tegra20-emc.txt | 43 +++++++++++++++++--
> 1 file changed, 40 insertions(+), 3 deletions(-)
Why does this have to be modelled as a separate device? Isn't this just
using a couple of registers out of the EMC register range? If so, this
would better just be integrated into the parent node and implemented as
part of the EMC driver. No need to further complicate things by adding
a dummy child.
Thierry
On Mon, Oct 26, 2020 at 01:17:13AM +0300, Dmitry Osipenko wrote:
> This patch adds modularization support to the Tegra20 EMC driver. Driver
> now can be compiled as a loadable kernel module.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/memory/tegra/Kconfig | 2 +-
> drivers/memory/tegra/tegra20-emc.c | 17 ++++++++++++-----
> 2 files changed, 13 insertions(+), 6 deletions(-)
Acked-by: Thierry Reding <[email protected]>
27.10.2020 11:54, Krzysztof Kozlowski пишет:
> On Mon, Oct 26, 2020 at 01:16:47AM +0300, Dmitry Osipenko wrote:
>> Tegra20 External Memory Controller talks to DRAM chips and it needs to be
>> reprogrammed when memory frequency changes. Tegra Memory Controller sits
>> behind EMC and these controllers are tightly coupled. This patch adds the
>> new phandle property which allows to properly express connection of EMC
>> and MC hardware in a device-tree, it also put the Tegra20 EMC binding on
>> par with Tegra30+ EMC bindings, which is handy to have.
>>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>> ---
>> .../bindings/memory-controllers/nvidia,tegra20-emc.txt | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
>> index 567cffd37f3f..1b0d4417aad8 100644
>> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
>> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
>> @@ -12,6 +12,7 @@ Properties:
>> irrespective of ram-code configuration.
>> - interrupts : Should contain EMC General interrupt.
>> - clocks : Should contain EMC clock.
>> +- nvidia,memory-controller : Phandle of the Memory Controller node.
>
> It looks like you adding a required property which is an ABI break.
The T20 EMC driver is unused so far in upstream and it will become used
only once this series is applied. Hence it's fine to change the ABI.
27.10.2020 12:02, Krzysztof Kozlowski пишет:
>> @@ -31,17 +32,34 @@ Example:
>> ...
>> };
>>
>> + emc_bw_dfs_opp_table: emc_opp_table1 {
> Hyphens for node name.
We already use underscores for the Tegra CPU OPP table.
https://elixir.bootlin.com/linux/v5.10-rc1/source/arch/arm/boot/dts/tegra20-cpu-opp.dtsi#L4
What makes you think that hyphens will be a better choice? Is it a
documented naming convention?
27.10.2020 16:22, Thierry Reding пишет:
> On Mon, Oct 26, 2020 at 01:16:51AM +0300, Dmitry Osipenko wrote:
>> External Memory Controller can gather various hardware statistics that
>> are intended to be used for debugging purposes and for dynamic frequency
>> scaling of memory bus.
>>
>> Document the new mfd-simple compatible and EMC statistics sub-device.
>> The subdev contains EMC DFS OPP table and interconnect paths to be used
>> for dynamic scaling of system's memory bandwidth based on EMC utilization
>> statistics.
>>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>> ---
>> .../memory-controllers/nvidia,tegra20-emc.txt | 43 +++++++++++++++++--
>> 1 file changed, 40 insertions(+), 3 deletions(-)
>
> Why does this have to be modelled as a separate device? Isn't this just
> using a couple of registers out of the EMC register range? If so, this
> would better just be integrated into the parent node and implemented as
> part of the EMC driver. No need to further complicate things by adding
> a dummy child.
Maybe true, I'll take a closer look.
On Tue, Oct 27, 2020 at 10:17:19PM +0300, Dmitry Osipenko wrote:
> 27.10.2020 11:54, Krzysztof Kozlowski пишет:
> > On Mon, Oct 26, 2020 at 01:16:47AM +0300, Dmitry Osipenko wrote:
> >> Tegra20 External Memory Controller talks to DRAM chips and it needs to be
> >> reprogrammed when memory frequency changes. Tegra Memory Controller sits
> >> behind EMC and these controllers are tightly coupled. This patch adds the
> >> new phandle property which allows to properly express connection of EMC
> >> and MC hardware in a device-tree, it also put the Tegra20 EMC binding on
> >> par with Tegra30+ EMC bindings, which is handy to have.
> >>
> >> Signed-off-by: Dmitry Osipenko <[email protected]>
> >> ---
> >> .../bindings/memory-controllers/nvidia,tegra20-emc.txt | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> >> index 567cffd37f3f..1b0d4417aad8 100644
> >> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> >> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> >> @@ -12,6 +12,7 @@ Properties:
> >> irrespective of ram-code configuration.
> >> - interrupts : Should contain EMC General interrupt.
> >> - clocks : Should contain EMC clock.
> >> +- nvidia,memory-controller : Phandle of the Memory Controller node.
> >
> > It looks like you adding a required property which is an ABI break.
> The T20 EMC driver is unused so far in upstream and it will become used
> only once this series is applied. Hence it's fine to change the ABI.
The ABI is not about upstream, but downstream. There are no other
upstreams using this ABI. Unless you have in mind that existing T20 EMC
driver was a noop, doing absolutely nothing, therefore there is no
breakage of any other users?
Best regards,
Krzysztof
27.10.2020 16:52, Thierry Reding пишет:
> On Mon, Oct 26, 2020 at 01:17:15AM +0300, Dmitry Osipenko wrote:
>> EMC driver will become mandatory after turning it into interconnect
>> provider because interconnect users, like display controller driver, will
>> fail to probe using newer device-trees that have interconnect properties.
>> Thus make EMC driver to probe even if timings are missing in device-tree.
>
> Does it really have to be mandatory? Sounds like that's going to make it
> unnecessarily complicated to merge all of this. Is it complicated to
> make interconnect support optional in consumer drivers?
Interconnect provider can't be optional if interconnect properties
present in a device-tree because drivers that use ICC path will get
-EPROBE_DEFER until ICC provider is registered.
Older device-trees don't have ICC properties, and thus, the ICC
users/consumers will get a dummy NULL ICC path in this case. I.e. ICC
core handles this for us.
On Tue, Oct 27, 2020 at 10:22:19PM +0300, Dmitry Osipenko wrote:
> 27.10.2020 12:02, Krzysztof Kozlowski пишет:
> >> @@ -31,17 +32,34 @@ Example:
> >> ...
> >> };
> >>
> >> + emc_bw_dfs_opp_table: emc_opp_table1 {
> > Hyphens for node name.
>
> We already use underscores for the Tegra CPU OPP table.
>
> https://elixir.bootlin.com/linux/v5.10-rc1/source/arch/arm/boot/dts/tegra20-cpu-opp.dtsi#L4
>
> What makes you think that hyphens will be a better choice? Is it a
> documented naming convention?
Unfortunately that's the source of confusion also for me because
Devicetree spec mentions both of them (and does not specify preferences).
The choice of dashes/hyphens comes now explicitly from all dtschema
files. Previously, the documentation were emails from Rob. :)
Best regards,
Krzysztof
27.10.2020 22:44, Krzysztof Kozlowski пишет:
> On Tue, Oct 27, 2020 at 10:22:19PM +0300, Dmitry Osipenko wrote:
>> 27.10.2020 12:02, Krzysztof Kozlowski пишет:
>>>> @@ -31,17 +32,34 @@ Example:
>>>> ...
>>>> };
>>>>
>>>> + emc_bw_dfs_opp_table: emc_opp_table1 {
>>> Hyphens for node name.
>>
>> We already use underscores for the Tegra CPU OPP table.
>>
>> https://elixir.bootlin.com/linux/v5.10-rc1/source/arch/arm/boot/dts/tegra20-cpu-opp.dtsi#L4
>>
>> What makes you think that hyphens will be a better choice? Is it a
>> documented naming convention?
>
> Unfortunately that's the source of confusion also for me because
> Devicetree spec mentions both of them (and does not specify preferences).
>
> The choice of dashes/hyphens comes now explicitly from all dtschema
> files. Previously, the documentation were emails from Rob. :)
Okay, I'll change it in v7. So far I haven't seen warnings about it from
the schema-checker.
27.10.2020 22:30, Krzysztof Kozlowski пишет:
> On Tue, Oct 27, 2020 at 10:17:19PM +0300, Dmitry Osipenko wrote:
>> 27.10.2020 11:54, Krzysztof Kozlowski пишет:
>>> On Mon, Oct 26, 2020 at 01:16:47AM +0300, Dmitry Osipenko wrote:
>>>> Tegra20 External Memory Controller talks to DRAM chips and it needs to be
>>>> reprogrammed when memory frequency changes. Tegra Memory Controller sits
>>>> behind EMC and these controllers are tightly coupled. This patch adds the
>>>> new phandle property which allows to properly express connection of EMC
>>>> and MC hardware in a device-tree, it also put the Tegra20 EMC binding on
>>>> par with Tegra30+ EMC bindings, which is handy to have.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <[email protected]>
>>>> ---
>>>> .../bindings/memory-controllers/nvidia,tegra20-emc.txt | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
>>>> index 567cffd37f3f..1b0d4417aad8 100644
>>>> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
>>>> @@ -12,6 +12,7 @@ Properties:
>>>> irrespective of ram-code configuration.
>>>> - interrupts : Should contain EMC General interrupt.
>>>> - clocks : Should contain EMC clock.
>>>> +- nvidia,memory-controller : Phandle of the Memory Controller node.
>>>
>>> It looks like you adding a required property which is an ABI break.
>> The T20 EMC driver is unused so far in upstream and it will become used
>> only once this series is applied. Hence it's fine to change the ABI.
>
> The ABI is not about upstream, but downstream. There are no other
> upstreams using this ABI. Unless you have in mind that existing T20 EMC
> driver was a noop, doing absolutely nothing, therefore there is no
> breakage of any other users?
The T20 EMC driver was a 100% noop for now. It's safe to change the ABI.
The T30/124 EMC drivers have users, but these are unsafe drivers (with
the known issues) until this series is applied.
27.10.2020 08:10, Viresh Kumar пишет:
> On 26-10-20, 01:17, Dmitry Osipenko wrote:
>> This patch fixes lockup which happens when OPP table is released if
>> interconnect provider uses OPP in the icc_provider->set() callback
>> and bandwidth of the ICC path is set to 0 by the ICC core when path
>> is released. The icc_put() doesn't need the opp_table_lock protection,
>> hence let's move it outside of the lock in order to resolve the problem.
>>
>> In particular this fixes tegra-devfreq driver lockup on trying to unload
>> the driver module. The devfreq driver uses OPP-bandwidth API and its ICC
>> provider also uses OPP for DVFS, hence they both take same opp_table_lock
>> when OPP table of the devfreq is released.
>>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>> ---
...
>
> Never make such _fixes_ part of such a big patchset. Always send them
> separately.
Perhaps it's not obvious from the commit description that this patch
doesn't fix any known problems of the current mainline kernel and it's
needed only for the new patches.
> Having said that, I already have a patch with me which shall fix it for you as
> well:
I see that yours fix is already applied, thanks!
27.10.2020 11:52, Krzysztof Kozlowski пишет:
> On Mon, Oct 26, 2020 at 10:14:10PM +0300, Dmitry Osipenko wrote:
>> 26.10.2020 18:08, Krzysztof Kozlowski пишет:
>>> On Mon, Oct 26, 2020 at 01:16:43AM +0300, Dmitry Osipenko wrote:
>>>> Hello,
>>>>
>>>> This series brings initial support for memory interconnect to Tegra20,
>>>> Tegra30 and Tegra124 SoCs.
>>>>
>>>> For the starter only display controllers and devfreq devices are getting
>>>> interconnect API support, others could be supported later on. The display
>>>> controllers have the biggest demand for interconnect API right now because
>>>> dynamic memory frequency scaling can't be done safely without taking into
>>>> account bandwidth requirement from the displays. In particular this series
>>>> fixes distorted display output on T30 Ouya and T124 TK1 devices.
>>>
>>> Hi,
>>>
>>> You introduced in v6 multiple new patches. Could you describe the
>>> dependencies, if any?
>>
>> Hello,
>>
>> The v6 dropped some older patches and replaced them with the new
>> patches. Previously I wanted to postpone the more complex changes for
>> later times, like supporting OPP tables and DVFS, but then the review
>> started to take more time than was expected and there was enough time to
>> complete those features.
>>
>> There are five basic sets of patches:
>>
>> 1. DT bindings
>> 2. DT changes
>> 3. SoC, clk and memory
>> 4. devfreq
>> 5. DRM
>>
>> Each set could be applied separately.
>>
>> Memory patches have a build dependency on the SoC and clk patches.
>>
>> The "tegra-mc: Add interconnect framework" and "Add and use
>> devm_tegra_get_memory_controller()" are the root build dependencies for
>> all memory/ patches. Other patches are grouped per SoC generation
>> (Tegra20/30/124), patches within a SoC-gen group are interdependent.
>>
>> I suppose the best variant would be to merge the whole series via
>> tegra-tree in order to preserve logical order of the patches. Although,
>> merging each set of patches separately also should be okay to do.
>
> Thanks for explanation. I already have three patches for Tegra MC (and
> probably two more will be respun) so this might be conflict-prone. The
> easiest in such case is to provide me soc and clk patches on the branch,
> so I could keep all Tegra MC together.
Okay, but those T210 patches don't touch the same code, neither same
source files. For now there should be no merge conflicts.
On Mon, 26 Oct 2020 01:16:54 +0300, Dmitry Osipenko wrote:
> Document new OPP table and voltage regulator properties which are needed
> for supporting dynamic voltage-frequency scaling of the memory controller.
> Some boards may have a fixed core voltage regulator, hence it's optional
> because frequency scaling still may be desired.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> .../memory-controllers/nvidia,tegra30-emc.yaml | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
Reviewed-by: Rob Herring <[email protected]>
On Mon, 26 Oct 2020 01:17:02 +0300, Dmitry Osipenko wrote:
> Each memory client have a unique hardware ID, this patch adds these IDs.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> include/dt-bindings/memory/tegra124-mc.h | 68 ++++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
>
Reviewed-by: Rob Herring <[email protected]>
On Mon, 26 Oct 2020 01:16:47 +0300, Dmitry Osipenko wrote:
> Tegra20 External Memory Controller talks to DRAM chips and it needs to be
> reprogrammed when memory frequency changes. Tegra Memory Controller sits
> behind EMC and these controllers are tightly coupled. This patch adds the
> new phandle property which allows to properly express connection of EMC
> and MC hardware in a device-tree, it also put the Tegra20 EMC binding on
> par with Tegra30+ EMC bindings, which is handy to have.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> .../bindings/memory-controllers/nvidia,tegra20-emc.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
Acked-by: Rob Herring <[email protected]>
On Mon, Oct 26, 2020 at 01:16:51AM +0300, Dmitry Osipenko wrote:
> External Memory Controller can gather various hardware statistics that
> are intended to be used for debugging purposes and for dynamic frequency
> scaling of memory bus.
>
> Document the new mfd-simple compatible and EMC statistics sub-device.
It's simple-mfd.
That should only be used if the child has no dependencies on the parent
node (and driver).
> The subdev contains EMC DFS OPP table and interconnect paths to be used
> for dynamic scaling of system's memory bandwidth based on EMC utilization
> statistics.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> .../memory-controllers/nvidia,tegra20-emc.txt | 43 +++++++++++++++++--
> 1 file changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> index 8d09b228ac42..382aabcd6952 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> @@ -4,7 +4,7 @@ Properties:
> - name : Should be emc
> - #address-cells : Should be 1
> - #size-cells : Should be 0
> -- compatible : Should contain "nvidia,tegra20-emc".
> +- compatible : Should contain "nvidia,tegra20-emc" and "simple-mfd".
> - reg : Offset and length of the register set for the device
> - nvidia,use-ram-code : If present, the sub-nodes will be addressed
> and chosen using the ramcode board selector. If omitted, only one
> @@ -17,7 +17,8 @@ Properties:
> - core-supply: Phandle of voltage regulator of the SoC "core" power domain.
> - operating-points-v2: See ../bindings/opp/opp.txt for details.
>
> -Child device nodes describe the memory settings for different configurations and clock rates.
> +Child device nodes describe the memory settings for different configurations and clock rates,
> +as well as EMC activity statistics collection sub-device.
>
> Example:
>
> @@ -31,17 +32,34 @@ Example:
> ...
> };
>
> + emc_bw_dfs_opp_table: emc_opp_table1 {
> + compatible = "operating-points-v2";
> +
> + opp@36000000 {
> + opp-hz = /bits/ 64 <36000000>;
> + opp-peak-kBps = <144000>;
> + };
> + ...
> + };
> +
> memory-controller@7000f400 {
> #address-cells = < 1 >;
> #size-cells = < 0 >;
> #interconnect-cells = < 0 >;
> - compatible = "nvidia,tegra20-emc";
> + compatible = "nvidia,tegra20-emc", "simple-mfd";
> reg = <0x7000f400 0x400>;
> interrupts = <0 78 0x04>;
> clocks = <&tegra_car TEGRA20_CLK_EMC>;
> nvidia,memory-controller = <&mc>;
> core-supply = <&core_vdd_reg>;
> operating-points-v2 = <&emc_icc_dvfs_opp_table>;
> +
> + emc-stats {
> + compatible = "nvidia,tegra20-emc-statistics";
> + operating-points-v2 = <&emc_bw_dfs_opp_table>;
> + interconnects = <&mc TEGRA20_MC_MPCORER &emc>;
> + interconnect-names = "cpu";
> + };
> }
>
>
> @@ -120,3 +138,22 @@ Properties:
> 0 0 0 0 0 0 0 0 0 0 0 0 0 0
> 0 0 0 0 >;
> };
> +
> +
> +
> +Embedded Memory Controller statistics gathering sub-device
> +
> +EMC statistics subdev gathers information about hardware utilization
> +which is intended to be used for debugging purposes and for dynamic
> +frequency scaling based on the collected stats.
> +
> +Properties:
> +- name : Should be emc-stats.
> +- compatible : Should contain "nvidia,tegra20-emc-statistics".
> +- operating-points-v2: See ../bindings/opp/opp.txt for details.
> +- interconnects: Should contain entries for memory clients sitting on
> + MC->EMC memory interconnect path.
> +- interconnect-names: Should include name of the interconnect path for each
> + interconnect entry. Consult TRM documentation for
> + information about available memory clients, see MEMORY
> + CONTROLLER section.
> --
> 2.27.0
>
On Tue, Oct 27, 2020 at 08:30:39PM +0100, Krzysztof Kozlowski wrote:
> On Tue, Oct 27, 2020 at 10:17:19PM +0300, Dmitry Osipenko wrote:
> > 27.10.2020 11:54, Krzysztof Kozlowski пишет:
> > > On Mon, Oct 26, 2020 at 01:16:47AM +0300, Dmitry Osipenko wrote:
> > >> Tegra20 External Memory Controller talks to DRAM chips and it needs to be
> > >> reprogrammed when memory frequency changes. Tegra Memory Controller sits
> > >> behind EMC and these controllers are tightly coupled. This patch adds the
> > >> new phandle property which allows to properly express connection of EMC
> > >> and MC hardware in a device-tree, it also put the Tegra20 EMC binding on
> > >> par with Tegra30+ EMC bindings, which is handy to have.
> > >>
> > >> Signed-off-by: Dmitry Osipenko <[email protected]>
> > >> ---
> > >> .../bindings/memory-controllers/nvidia,tegra20-emc.txt | 2 ++
> > >> 1 file changed, 2 insertions(+)
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> > >> index 567cffd37f3f..1b0d4417aad8 100644
> > >> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> > >> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> > >> @@ -12,6 +12,7 @@ Properties:
> > >> irrespective of ram-code configuration.
> > >> - interrupts : Should contain EMC General interrupt.
> > >> - clocks : Should contain EMC clock.
> > >> +- nvidia,memory-controller : Phandle of the Memory Controller node.
> > >
> > > It looks like you adding a required property which is an ABI break.
> > The T20 EMC driver is unused so far in upstream and it will become used
> > only once this series is applied. Hence it's fine to change the ABI.
>
> The ABI is not about upstream, but downstream.
"If it's not upstream, it doesn't exist."
Though we do have to account for out of tree users where the DT is not
in tree, but upstream drivers are used. Downstream as in vendor kernels
typically has loads of other crap.
> There are no other
> upstreams using this ABI. Unless you have in mind that existing T20 EMC
> driver was a noop, doing absolutely nothing, therefore there is no
> breakage of any other users?
ABI breaks are ultimately up to the platform maintainers to decide.
Rob
On Tue, Oct 27, 2020 at 11:18:34PM +0300, Dmitry Osipenko wrote:
> 27.10.2020 22:44, Krzysztof Kozlowski пишет:
> > On Tue, Oct 27, 2020 at 10:22:19PM +0300, Dmitry Osipenko wrote:
> >> 27.10.2020 12:02, Krzysztof Kozlowski пишет:
> >>>> @@ -31,17 +32,34 @@ Example:
> >>>> ...
> >>>> };
> >>>>
> >>>> + emc_bw_dfs_opp_table: emc_opp_table1 {
> >>> Hyphens for node name.
> >>
> >> We already use underscores for the Tegra CPU OPP table.
> >>
> >> https://elixir.bootlin.com/linux/v5.10-rc1/source/arch/arm/boot/dts/tegra20-cpu-opp.dtsi#L4
> >>
> >> What makes you think that hyphens will be a better choice? Is it a
> >> documented naming convention?
> >
> > Unfortunately that's the source of confusion also for me because
> > Devicetree spec mentions both of them (and does not specify preferences).
> >
> > The choice of dashes/hyphens comes now explicitly from all dtschema
> > files. Previously, the documentation were emails from Rob. :)
>
> Okay, I'll change it in v7. So far I haven't seen warnings about it from
> the schema-checker.
dtc with W=2 will warn.
The bigger issue is the name should be generic.
Rob
On 27-10-20, 23:26, Dmitry Osipenko wrote:
> 27.10.2020 08:10, Viresh Kumar пишет:
> > On 26-10-20, 01:17, Dmitry Osipenko wrote:
> >> This patch fixes lockup which happens when OPP table is released if
> >> interconnect provider uses OPP in the icc_provider->set() callback
> >> and bandwidth of the ICC path is set to 0 by the ICC core when path
> >> is released. The icc_put() doesn't need the opp_table_lock protection,
> >> hence let's move it outside of the lock in order to resolve the problem.
> >>
> >> In particular this fixes tegra-devfreq driver lockup on trying to unload
> >> the driver module. The devfreq driver uses OPP-bandwidth API and its ICC
> >> provider also uses OPP for DVFS, hence they both take same opp_table_lock
> >> when OPP table of the devfreq is released.
> >>
> >> Signed-off-by: Dmitry Osipenko <[email protected]>
> >> ---
> ...
> >
> > Never make such _fixes_ part of such a big patchset. Always send them
> > separately.
>
> Perhaps it's not obvious from the commit description that this patch
> doesn't fix any known problems of the current mainline kernel and it's
> needed only for the new patches.
No, I understood that we started getting the warning now only after
some other patches of yours. Nevertheless, it should be considered as
a fix only as that generated lockdep because of locking placement. And
so sending such stuff separately is better as that allows people to
apply it fast.
> > Having said that, I already have a patch with me which shall fix it for you as
> > well:
>
> I see that yours fix is already applied, thanks!
I hope it worked for you. Thanks.
--
viresh
On Wed, Oct 28, 2020 at 10:23:03AM -0500, Rob Herring wrote:
> On Tue, Oct 27, 2020 at 08:30:39PM +0100, Krzysztof Kozlowski wrote:
> > On Tue, Oct 27, 2020 at 10:17:19PM +0300, Dmitry Osipenko wrote:
> > > 27.10.2020 11:54, Krzysztof Kozlowski пишет:
> > > > On Mon, Oct 26, 2020 at 01:16:47AM +0300, Dmitry Osipenko wrote:
> > > >> Tegra20 External Memory Controller talks to DRAM chips and it needs to be
> > > >> reprogrammed when memory frequency changes. Tegra Memory Controller sits
> > > >> behind EMC and these controllers are tightly coupled. This patch adds the
> > > >> new phandle property which allows to properly express connection of EMC
> > > >> and MC hardware in a device-tree, it also put the Tegra20 EMC binding on
> > > >> par with Tegra30+ EMC bindings, which is handy to have.
> > > >>
> > > >> Signed-off-by: Dmitry Osipenko <[email protected]>
> > > >> ---
> > > >> .../bindings/memory-controllers/nvidia,tegra20-emc.txt | 2 ++
> > > >> 1 file changed, 2 insertions(+)
> > > >>
> > > >> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> > > >> index 567cffd37f3f..1b0d4417aad8 100644
> > > >> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> > > >> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-emc.txt
> > > >> @@ -12,6 +12,7 @@ Properties:
> > > >> irrespective of ram-code configuration.
> > > >> - interrupts : Should contain EMC General interrupt.
> > > >> - clocks : Should contain EMC clock.
> > > >> +- nvidia,memory-controller : Phandle of the Memory Controller node.
> > > >
> > > > It looks like you adding a required property which is an ABI break.
> > > The T20 EMC driver is unused so far in upstream and it will become used
> > > only once this series is applied. Hence it's fine to change the ABI.
> >
> > The ABI is not about upstream, but downstream.
>
> "If it's not upstream, it doesn't exist."
>
> Though we do have to account for out of tree users where the DT is not
> in tree, but upstream drivers are used. Downstream as in vendor kernels
> typically has loads of other crap.
That's the case I am referring to. Maybe not in case of Tegra, but
multiple other designs are quite popular in industrial uses and their
DTSes were not upstreamed.
This is anyway different case, as Dmitry explained - nothing got broken
because not much was working before around this.
>
> > There are no other
> > upstreams using this ABI. Unless you have in mind that existing T20 EMC
> > driver was a noop, doing absolutely nothing, therefore there is no
> > breakage of any other users?
>
> ABI breaks are ultimately up to the platform maintainers to decide.
Cool! That reshapes significantly my existing point of view, especially
about discussions on Exynos bindings (long time ago). Thanks for
clarification.
Best regards,
Krzysztof
28.10.2020 18:26, Rob Herring пишет:
> On Tue, Oct 27, 2020 at 11:18:34PM +0300, Dmitry Osipenko wrote:
>> 27.10.2020 22:44, Krzysztof Kozlowski пишет:
>>> On Tue, Oct 27, 2020 at 10:22:19PM +0300, Dmitry Osipenko wrote:
>>>> 27.10.2020 12:02, Krzysztof Kozlowski пишет:
>>>>>> @@ -31,17 +32,34 @@ Example:
>>>>>> ...
>>>>>> };
>>>>>>
>>>>>> + emc_bw_dfs_opp_table: emc_opp_table1 {
>>>>> Hyphens for node name.
>>>>
>>>> We already use underscores for the Tegra CPU OPP table.
>>>>
>>>> https://elixir.bootlin.com/linux/v5.10-rc1/source/arch/arm/boot/dts/tegra20-cpu-opp.dtsi#L4
>>>>
>>>> What makes you think that hyphens will be a better choice? Is it a
>>>> documented naming convention?
>>>
>>> Unfortunately that's the source of confusion also for me because
>>> Devicetree spec mentions both of them (and does not specify preferences).
>>>
>>> The choice of dashes/hyphens comes now explicitly from all dtschema
>>> files. Previously, the documentation were emails from Rob. :)
>>
>> Okay, I'll change it in v7. So far I haven't seen warnings about it from
>> the schema-checker.
>
> dtc with W=2 will warn.
>
> The bigger issue is the name should be generic.
Indeed, thanks. I'll correct the name.
Hi Dmitry,
On Mon, Oct 26, 2020 at 7:22 AM Dmitry Osipenko <[email protected]> wrote:
>
> This patch moves ACTMON driver away from generating OPP table by itself,
> transitioning it to use the table which comes from device-tree. This
> change breaks compatibility with older device-trees in order to bring
> support for the interconnect framework to the driver. This is a mandatory
> change which needs to be done in order to implement interconnect-based
> memory DVFS. Users of legacy device-trees will get a message telling that
> theirs DT needs to be upgraded. Now ACTMON issues memory bandwidth request
> using dev_pm_opp_set_bw(), instead of driving EMC clock rate directly.
>
> Tested-by: Peter Geis <[email protected]>
> Tested-by: Nicolas Chauvet <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/tegra30-devfreq.c | 91 ++++++++++++++++---------------
> 1 file changed, 48 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 3f732ab53573..1b0b91a71886 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -19,6 +19,8 @@
> #include <linux/reset.h>
> #include <linux/workqueue.h>
>
> +#include <soc/tegra/fuse.h>
> +
> #include "governor.h"
>
> #define ACTMON_GLB_STATUS 0x0
> @@ -155,6 +157,7 @@ struct tegra_devfreq_device {
>
> struct tegra_devfreq {
> struct devfreq *devfreq;
> + struct opp_table *opp_table;
>
> struct reset_control *reset;
> struct clk *clock;
> @@ -612,34 +615,19 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
> static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
> u32 flags)
> {
> - struct tegra_devfreq *tegra = dev_get_drvdata(dev);
> - struct devfreq *devfreq = tegra->devfreq;
> struct dev_pm_opp *opp;
> - unsigned long rate;
> - int err;
> + int ret;
>
> opp = devfreq_recommended_opp(dev, freq, flags);
> if (IS_ERR(opp)) {
> - dev_err(dev, "Failed to find opp for %lu Hz\n", *freq);
> + dev_err(dev, "failed to find opp for %lu Hz\n", *freq);
> return PTR_ERR(opp);
> }
> - rate = dev_pm_opp_get_freq(opp);
> - dev_pm_opp_put(opp);
> -
> - err = clk_set_min_rate(tegra->emc_clock, rate * KHZ);
> - if (err)
> - return err;
> -
> - err = clk_set_rate(tegra->emc_clock, 0);
> - if (err)
> - goto restore_min_rate;
>
> - return 0;
> -
> -restore_min_rate:
> - clk_set_min_rate(tegra->emc_clock, devfreq->previous_freq);
> + ret = dev_pm_opp_set_bw(dev, opp);
> + dev_pm_opp_put(opp);
>
> - return err;
> + return ret;
> }
>
> static int tegra_devfreq_get_dev_status(struct device *dev,
> @@ -655,7 +643,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
> stat->private_data = tegra;
>
> /* The below are to be used by the other governors */
> - stat->current_frequency = cur_freq;
> + stat->current_frequency = cur_freq * KHZ;
I can't find any change related to the frequency unit on this patch.
Do you fix the previous bug of the frequency unit?
>
> actmon_dev = &tegra->devices[MCALL];
>
> @@ -705,7 +693,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
> target_freq = max(target_freq, dev->target_freq);
> }
>
> - *freq = target_freq;
> + *freq = target_freq * KHZ;
ditto.
>
> return 0;
> }
> @@ -773,13 +761,22 @@ static struct devfreq_governor tegra_devfreq_governor = {
>
> static int tegra_devfreq_probe(struct platform_device *pdev)
> {
> + u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
> struct tegra_devfreq_device *dev;
> struct tegra_devfreq *tegra;
> + struct opp_table *opp_table;
> struct devfreq *devfreq;
> unsigned int i;
> long rate;
> int err;
>
> + /* legacy device-trees don't have OPP table and must be updated */
> + if (!device_property_present(&pdev->dev, "operating-points-v2")) {
> + dev_err(&pdev->dev, "OPP table not found, cannot continue\n");
> + dev_err(&pdev->dev, "please update your device tree\n");
> + return -ENODEV;
> + }
As you mentioned, it breaks the old dtb. I have no objection to improving
the driver. Instead, you need confirmation from the Devicetree maintainer.
And,
I recommend that you use dev_pm_opp_of_get_opp_desc_node(&pdev->dev)
to check whether a device contains opp-table or not.
> +
> tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> if (!tegra)
> return -ENOMEM;
> @@ -821,11 +818,29 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> return err;
> }
>
> + tegra->opp_table = dev_pm_opp_get_opp_table(&pdev->dev);
> + if (IS_ERR(tegra->opp_table))
> + return dev_err_probe(&pdev->dev, PTR_ERR(tegra->opp_table),
> + "Failed to prepare OPP table\n");
As I knew, each device can contain the opp_table on devicetree.
It means that opp_table has not depended to another device driver.
Did you see this exception case with EPROBE_DEFER error?
> +
> + opp_table = dev_pm_opp_set_supported_hw(&pdev->dev, &hw_version, 1);
> + err = PTR_ERR_OR_ZERO(opp_table);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to set supported HW: %d\n", err);
> + goto put_table;
> + }
> +
> + err = dev_pm_opp_of_add_table(&pdev->dev);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to add OPP table: %d\n", err);
> + goto put_hw;
> + }
> +
> err = clk_prepare_enable(tegra->clock);
> if (err) {
> dev_err(&pdev->dev,
> "Failed to prepare and enable ACTMON clock\n");
> - return err;
> + goto remove_table;
> }
>
> err = reset_control_reset(tegra->reset);
> @@ -849,23 +864,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> dev->regs = tegra->regs + dev->config->offset;
> }
>
> - for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
> - rate = clk_round_rate(tegra->emc_clock, rate);
> -
> - if (rate < 0) {
> - dev_err(&pdev->dev,
> - "Failed to round clock rate: %ld\n", rate);
> - err = rate;
> - goto remove_opps;
> - }
> -
> - err = dev_pm_opp_add(&pdev->dev, rate / KHZ, 0);
> - if (err) {
> - dev_err(&pdev->dev, "Failed to add OPP: %d\n", err);
> - goto remove_opps;
> - }
> - }
> -
> platform_set_drvdata(pdev, tegra);
>
> tegra->clk_rate_change_nb.notifier_call = tegra_actmon_clk_notify_cb;
> @@ -881,7 +879,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> }
>
> tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock);
> - tegra_devfreq_profile.initial_freq /= KHZ;
>
> devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile,
> "tegra_actmon", NULL);
> @@ -901,6 +898,12 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> reset_control_reset(tegra->reset);
> disable_clk:
> clk_disable_unprepare(tegra->clock);
> +remove_table:
> + dev_pm_opp_of_remove_table(&pdev->dev);
> +put_hw:
> + dev_pm_opp_put_supported_hw(tegra->opp_table);
> +put_table:
> + dev_pm_opp_put_opp_table(tegra->opp_table);
>
> return err;
> }
> @@ -912,11 +915,13 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
> devfreq_remove_device(tegra->devfreq);
> devfreq_remove_governor(&tegra_devfreq_governor);
>
> - dev_pm_opp_remove_all_dynamic(&pdev->dev);
> -
> reset_control_reset(tegra->reset);
> clk_disable_unprepare(tegra->clock);
>
> + dev_pm_opp_of_remove_table(&pdev->dev);
> + dev_pm_opp_put_supported_hw(tegra->opp_table);
> + dev_pm_opp_put_opp_table(tegra->opp_table);
> +
> return 0;
> }
>
> --
> 2.27.0
>
--
Best Regards,
Chanwoo Choi
Hi Dmitry,
On Mon, Oct 26, 2020 at 7:22 AM Dmitry Osipenko <[email protected]> wrote:
>
> This patch moves ACTMON driver away from generating OPP table by itself,
> transitioning it to use the table which comes from device-tree. This
> change breaks compatibility with older device-trees in order to bring
> support for the interconnect framework to the driver. This is a mandatory
> change which needs to be done in order to implement interconnect-based
> memory DVFS. Users of legacy device-trees will get a message telling that
> theirs DT needs to be upgraded. Now ACTMON issues memory bandwidth request
> using dev_pm_opp_set_bw(), instead of driving EMC clock rate directly.
>
> Tested-by: Peter Geis <[email protected]>
> Tested-by: Nicolas Chauvet <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/tegra30-devfreq.c | 91 ++++++++++++++++---------------
> 1 file changed, 48 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 3f732ab53573..1b0b91a71886 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -19,6 +19,8 @@
> #include <linux/reset.h>
> #include <linux/workqueue.h>
>
> +#include <soc/tegra/fuse.h>
> +
This patch touches the OPP. Is it related to this change?
> #include "governor.h"
>
> #define ACTMON_GLB_STATUS 0x0
> @@ -155,6 +157,7 @@ struct tegra_devfreq_device {
>
> struct tegra_devfreq {
> struct devfreq *devfreq;
> + struct opp_table *opp_table;
>
> struct reset_control *reset;
> struct clk *clock;
> @@ -612,34 +615,19 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
> static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
> u32 flags)
> {
> - struct tegra_devfreq *tegra = dev_get_drvdata(dev);
> - struct devfreq *devfreq = tegra->devfreq;
> struct dev_pm_opp *opp;
> - unsigned long rate;
> - int err;
> + int ret;
>
> opp = devfreq_recommended_opp(dev, freq, flags);
> if (IS_ERR(opp)) {
> - dev_err(dev, "Failed to find opp for %lu Hz\n", *freq);
> + dev_err(dev, "failed to find opp for %lu Hz\n", *freq);
You used the 'Failed to' format in almost every error case.
Don't need to change it.
(snip)
Best Regards,
Chanwoo Choi
Hi Dmitry,
I added the review about 'ARRAY_SIZE(tegra124_device_configs)'.
Except for this, it looks good to me.
On Mon, Oct 26, 2020 at 7:21 AM Dmitry Osipenko <[email protected]> wrote:
>
> Previously we were using count-weight of the T124 for T30 in order to
> get EMC clock rate that was reasonable for T30. In fact the count-weight
> should be x2 times smaller on T30, but then devfreq was producing a bit
> too low EMC clock rate for ISO memory clients, like display controller
> for example.
>
> Now both Tegra ACTMON and Tegra DRM display drivers support interconnect
> framework and display driver tells to ICC what a minimum memory bandwidth
> is needed, preventing FIFO underflows. Thus, now we can use a proper
> count-weight value for Tegra30 and MC_ALL device config needs a bit more
> aggressive boosting.
>
> This patch adds a separate ACTMON driver configuration that is specific
> to Tegra30.
>
> Tested-by: Peter Geis <[email protected]>
> Tested-by: Nicolas Chauvet <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/devfreq/tegra30-devfreq.c | 68 ++++++++++++++++++++++++-------
> 1 file changed, 54 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 1b0b91a71886..95aba89eae88 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -57,13 +57,6 @@
> #define ACTMON_BELOW_WMARK_WINDOW 3
> #define ACTMON_BOOST_FREQ_STEP 16000
>
> -/*
> - * Activity counter is incremented every 256 memory transactions, and each
> - * transaction takes 4 EMC clocks for Tegra124; So the COUNT_WEIGHT is
> - * 4 * 256 = 1024.
> - */
> -#define ACTMON_COUNT_WEIGHT 0x400
> -
> /*
> * ACTMON_AVERAGE_WINDOW_LOG2: default value for @DEV_CTRL_K_VAL, which
> * translates to 2 ^ (K_VAL + 1). ex: 2 ^ (6 + 1) = 128
> @@ -111,7 +104,7 @@ enum tegra_actmon_device {
> MCCPU,
> };
>
> -static const struct tegra_devfreq_device_config actmon_device_configs[] = {
> +static const struct tegra_devfreq_device_config tegra124_device_configs[] = {
> {
> /* MCALL: All memory accesses (including from the CPUs) */
> .offset = 0x1c0,
> @@ -133,6 +126,28 @@ static const struct tegra_devfreq_device_config actmon_device_configs[] = {
> },
> };
>
> +static const struct tegra_devfreq_device_config tegra30_device_configs[] = {
> + {
> + /* MCALL: All memory accesses (including from the CPUs) */
> + .offset = 0x1c0,
> + .irq_mask = 1 << 26,
> + .boost_up_coeff = 200,
> + .boost_down_coeff = 50,
> + .boost_up_threshold = 20,
> + .boost_down_threshold = 10,
> + },
> + {
> + /* MCCPU: memory accesses from the CPUs */
> + .offset = 0x200,
> + .irq_mask = 1 << 25,
> + .boost_up_coeff = 800,
> + .boost_down_coeff = 40,
> + .boost_up_threshold = 27,
> + .boost_down_threshold = 10,
> + .avg_dependency_threshold = 16000, /* 16MHz in kHz units */
> + },
> +};
> +
> /**
> * struct tegra_devfreq_device - state specific to an ACTMON device
> *
> @@ -155,6 +170,12 @@ struct tegra_devfreq_device {
> unsigned long target_freq;
> };
>
> +struct tegra_devfreq_soc_data {
> + const struct tegra_devfreq_device_config *configs;
> + /* Weight value for count measurements */
> + unsigned int count_weight;
> +};
> +
> struct tegra_devfreq {
> struct devfreq *devfreq;
> struct opp_table *opp_table;
> @@ -171,11 +192,13 @@ struct tegra_devfreq {
> struct delayed_work cpufreq_update_work;
> struct notifier_block cpu_rate_change_nb;
>
> - struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
> + struct tegra_devfreq_device devices[ARRAY_SIZE(tegra124_device_configs)];
When there is one tegra_devfreq_device_config[] array, this style is not wrong.
But, after adding the specific config array for each device,
you better specify the correct array size for each case.
Even if there is no runtime error on tegra30 soc, it is not proper to use
ARRAY_SIZE(tegra124_device_configs). You can allocate the array
of tegra_devfreq_device[] or use fixed the array size (2).
>
> unsigned int irq;
>
> bool started;
> +
> + const struct tegra_devfreq_soc_data *soc;
> };
>
> struct tegra_actmon_emc_ratio {
> @@ -488,7 +511,7 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
> tegra_devfreq_update_avg_wmark(tegra, dev);
> tegra_devfreq_update_wmark(tegra, dev);
>
> - device_writel(dev, ACTMON_COUNT_WEIGHT, ACTMON_DEV_COUNT_WEIGHT);
> + device_writel(dev, tegra->soc->count_weight, ACTMON_DEV_COUNT_WEIGHT);
> device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
>
> val |= ACTMON_DEV_CTRL_ENB_PERIODIC;
> @@ -781,6 +804,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> if (!tegra)
> return -ENOMEM;
>
> + tegra->soc = of_device_get_match_data(&pdev->dev);
> +
> tegra->regs = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(tegra->regs))
> return PTR_ERR(tegra->regs);
> @@ -858,9 +883,9 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>
> tegra->max_freq = rate / KHZ;
>
> - for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
> + for (i = 0; i < ARRAY_SIZE(tegra124_device_configs); i++) {
ditto. Use ARRARY_SIZE(soc->configs) instead of
ARRAY_SIZE(tegra124_device_configs).
> dev = tegra->devices + i;
> - dev->config = actmon_device_configs + i;
> + dev->config = tegra->soc->configs + i;
> dev->regs = tegra->regs + dev->config->offset;
> }
>
> @@ -925,9 +950,24 @@ static int tegra_devfreq_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static const struct tegra_devfreq_soc_data tegra124_soc = {
> + .configs = tegra124_device_configs,
> +
> + /*
> + * Activity counter is incremented every 256 memory transactions,
> + * and each transaction takes 4 EMC clocks.
> + */
> + .count_weight = 4 * 256,
> +};
> +
> +static const struct tegra_devfreq_soc_data tegra30_soc = {
> + .configs = tegra30_device_configs,
> + .count_weight = 2 * 256,
> +};
> +
> static const struct of_device_id tegra_devfreq_of_match[] = {
> - { .compatible = "nvidia,tegra30-actmon" },
> - { .compatible = "nvidia,tegra124-actmon" },
> + { .compatible = "nvidia,tegra30-actmon", .data = &tegra30_soc, },
> + { .compatible = "nvidia,tegra124-actmon", .data = &tegra124_soc, },
> { },
> };
>
> --
> 2.27.0
>
--
Best Regards,
Chanwoo Choi
01.11.2020 17:39, Chanwoo Choi пишет:
> Hi Dmitry,
>
> On Mon, Oct 26, 2020 at 7:22 AM Dmitry Osipenko <[email protected]> wrote:
>>
>> This patch moves ACTMON driver away from generating OPP table by itself,
>> transitioning it to use the table which comes from device-tree. This
>> change breaks compatibility with older device-trees in order to bring
>> support for the interconnect framework to the driver. This is a mandatory
>> change which needs to be done in order to implement interconnect-based
>> memory DVFS. Users of legacy device-trees will get a message telling that
>> theirs DT needs to be upgraded. Now ACTMON issues memory bandwidth request
>> using dev_pm_opp_set_bw(), instead of driving EMC clock rate directly.
>>
>> Tested-by: Peter Geis <[email protected]>
>> Tested-by: Nicolas Chauvet <[email protected]>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>> ---
...
>> static int tegra_devfreq_get_dev_status(struct device *dev,
>> @@ -655,7 +643,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>> stat->private_data = tegra;
>>
>> /* The below are to be used by the other governors */
>> - stat->current_frequency = cur_freq;
>> + stat->current_frequency = cur_freq * KHZ;
>
> I can't find any change related to the frequency unit on this patch.
> Do you fix the previous bug of the frequency unit?
Previously, OPP entries that were generated by the driver used KHz
units. Now, OPP entries are coming from a device-tree and they have Hz
units. This driver operates with KHz internally, hence we need to
convert the units now.
>>
>> actmon_dev = &tegra->devices[MCALL];
>>
>> @@ -705,7 +693,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
>> target_freq = max(target_freq, dev->target_freq);
>> }
>>
>> - *freq = target_freq;
>> + *freq = target_freq * KHZ;
>
> ditto.
>
>>
>> return 0;
>> }
>> @@ -773,13 +761,22 @@ static struct devfreq_governor tegra_devfreq_governor = {
>>
>> static int tegra_devfreq_probe(struct platform_device *pdev)
>> {
>> + u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
>> struct tegra_devfreq_device *dev;
>> struct tegra_devfreq *tegra;
>> + struct opp_table *opp_table;
>> struct devfreq *devfreq;
>> unsigned int i;
>> long rate;
>> int err;
>>
>> + /* legacy device-trees don't have OPP table and must be updated */
>> + if (!device_property_present(&pdev->dev, "operating-points-v2")) {
>> + dev_err(&pdev->dev, "OPP table not found, cannot continue\n");
>> + dev_err(&pdev->dev, "please update your device tree\n");
>> + return -ENODEV;
>> + }
>
> As you mentioned, it breaks the old dtb. I have no objection to improving
> the driver. Instead, you need confirmation from the Devicetree maintainer.
Older DTBs will continue to work, but devfreq driver won't. We already
did this for other Tegra drivers before (CPUFREQ driver for example) and
Rob Herring confirmed that there is no problem here.
> And,
> I recommend that you use dev_pm_opp_of_get_opp_desc_node(&pdev->dev)
> to check whether a device contains opp-table or not.
I'm not sure what are the benefits, this will make code less
expressive/readable and we will need to add extra of_node_put(), which
device_property_present() handles for us.
Could you please give the rationale?
>> +
>> tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
>> if (!tegra)
>> return -ENOMEM;
>> @@ -821,11 +818,29 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>> return err;
>> }
>>
>> + tegra->opp_table = dev_pm_opp_get_opp_table(&pdev->dev);
>> + if (IS_ERR(tegra->opp_table))
>> + return dev_err_probe(&pdev->dev, PTR_ERR(tegra->opp_table),
>> + "Failed to prepare OPP table\n");
>
> As I knew, each device can contain the opp_table on devicetree.
> It means that opp_table has not depended to another device driver.
> Did you see this exception case with EPROBE_DEFER error?
OPP core will try to grab the clock reference for the table and it may
cause EPROBE_DEFER. Although, it shouldn't happen here because we have
devm_clk_get() before the get_opp_table(), hence seems the deferred
probe indeed shouldn't happen in this case.
01.11.2020 17:44, Chanwoo Choi пишет:
> Hi Dmitry,
>
> On Mon, Oct 26, 2020 at 7:22 AM Dmitry Osipenko <[email protected]> wrote:
>>
>> This patch moves ACTMON driver away from generating OPP table by itself,
>> transitioning it to use the table which comes from device-tree. This
>> change breaks compatibility with older device-trees in order to bring
>> support for the interconnect framework to the driver. This is a mandatory
>> change which needs to be done in order to implement interconnect-based
>> memory DVFS. Users of legacy device-trees will get a message telling that
>> theirs DT needs to be upgraded. Now ACTMON issues memory bandwidth request
>> using dev_pm_opp_set_bw(), instead of driving EMC clock rate directly.
>>
>> Tested-by: Peter Geis <[email protected]>
>> Tested-by: Nicolas Chauvet <[email protected]>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>> ---
>> drivers/devfreq/tegra30-devfreq.c | 91 ++++++++++++++++---------------
>> 1 file changed, 48 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>> index 3f732ab53573..1b0b91a71886 100644
>> --- a/drivers/devfreq/tegra30-devfreq.c
>> +++ b/drivers/devfreq/tegra30-devfreq.c
>> @@ -19,6 +19,8 @@
>> #include <linux/reset.h>
>> #include <linux/workqueue.h>
>>
>> +#include <soc/tegra/fuse.h>
>> +
>
> This patch touches the OPP. Is it related to this change?
Yes, this is needed for the dev_pm_opp_set_supported_hw().
>> #include "governor.h"
>>
>> #define ACTMON_GLB_STATUS 0x0
>> @@ -155,6 +157,7 @@ struct tegra_devfreq_device {
>>
>> struct tegra_devfreq {
>> struct devfreq *devfreq;
>> + struct opp_table *opp_table;
>>
>> struct reset_control *reset;
>> struct clk *clock;
>> @@ -612,34 +615,19 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
>> static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
>> u32 flags)
>> {
>> - struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>> - struct devfreq *devfreq = tegra->devfreq;
>> struct dev_pm_opp *opp;
>> - unsigned long rate;
>> - int err;
>> + int ret;
>>
>> opp = devfreq_recommended_opp(dev, freq, flags);
>> if (IS_ERR(opp)) {
>> - dev_err(dev, "Failed to find opp for %lu Hz\n", *freq);
>> + dev_err(dev, "failed to find opp for %lu Hz\n", *freq);
>
> You used the 'Failed to' format in almost every error case.
> Don't need to change it.
> (snip)
Good catch, thanks.
Hi Dmitry,
On Mon, Nov 2, 2020 at 12:23 AM Dmitry Osipenko <[email protected]> wrote:
>
> 01.11.2020 17:39, Chanwoo Choi пишет:
> > Hi Dmitry,
> >
> > On Mon, Oct 26, 2020 at 7:22 AM Dmitry Osipenko <[email protected]> wrote:
> >>
> >> This patch moves ACTMON driver away from generating OPP table by itself,
> >> transitioning it to use the table which comes from device-tree. This
> >> change breaks compatibility with older device-trees in order to bring
> >> support for the interconnect framework to the driver. This is a mandatory
> >> change which needs to be done in order to implement interconnect-based
> >> memory DVFS. Users of legacy device-trees will get a message telling that
> >> theirs DT needs to be upgraded. Now ACTMON issues memory bandwidth request
> >> using dev_pm_opp_set_bw(), instead of driving EMC clock rate directly.
> >>
> >> Tested-by: Peter Geis <[email protected]>
> >> Tested-by: Nicolas Chauvet <[email protected]>
> >> Signed-off-by: Dmitry Osipenko <[email protected]>
> >> ---
> ...
> >> static int tegra_devfreq_get_dev_status(struct device *dev,
> >> @@ -655,7 +643,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
> >> stat->private_data = tegra;
> >>
> >> /* The below are to be used by the other governors */
> >> - stat->current_frequency = cur_freq;
> >> + stat->current_frequency = cur_freq * KHZ;
> >
> > I can't find any change related to the frequency unit on this patch.
> > Do you fix the previous bug of the frequency unit?
>
> Previously, OPP entries that were generated by the driver used KHz
> units. Now, OPP entries are coming from a device-tree and they have Hz
> units. This driver operates with KHz internally, hence we need to
> convert the units now.
OK. Thanks.
>
> >>
> >> actmon_dev = &tegra->devices[MCALL];
> >>
> >> @@ -705,7 +693,7 @@ static int tegra_governor_get_target(struct devfreq *devfreq,
> >> target_freq = max(target_freq, dev->target_freq);
> >> }
> >>
> >> - *freq = target_freq;
> >> + *freq = target_freq * KHZ;
> >
> > ditto.
> >
> >>
> >> return 0;
> >> }
> >> @@ -773,13 +761,22 @@ static struct devfreq_governor tegra_devfreq_governor = {
> >>
> >> static int tegra_devfreq_probe(struct platform_device *pdev)
> >> {
> >> + u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
> >> struct tegra_devfreq_device *dev;
> >> struct tegra_devfreq *tegra;
> >> + struct opp_table *opp_table;
> >> struct devfreq *devfreq;
> >> unsigned int i;
> >> long rate;
> >> int err;
> >>
> >> + /* legacy device-trees don't have OPP table and must be updated */
> >> + if (!device_property_present(&pdev->dev, "operating-points-v2")) {
> >> + dev_err(&pdev->dev, "OPP table not found, cannot continue\n");
> >> + dev_err(&pdev->dev, "please update your device tree\n");
> >> + return -ENODEV;
> >> + }
> >
> > As you mentioned, it breaks the old dtb. I have no objection to improving
> > the driver. Instead, you need confirmation from the Devicetree maintainer.
>
> Older DTBs will continue to work, but devfreq driver won't. We already
> did this for other Tegra drivers before (CPUFREQ driver for example) and
> Rob Herring confirmed that there is no problem here.
OK.
>
> > And,
> > I recommend that you use dev_pm_opp_of_get_opp_desc_node(&pdev->dev)
> > to check whether a device contains opp-table or not.
>
> I'm not sure what are the benefits, this will make code less
> expressive/readable and we will need to add extra of_node_put(), which
> device_property_present() handles for us.
>
> Could you please give the rationale?
IMO, 'operating-points-v2' word was defined on OPP core. I think that
the external user
of OPP better to use the public helper function instead of using the
interval definition
or value of OPP core directly. Basically, I prefer the provided helper
function if there.
But, it is not critical and doesn't affect the operation. If you want
to keep, it is ok.
>
> >> +
> >> tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> >> if (!tegra)
> >> return -ENOMEM;
> >> @@ -821,11 +818,29 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
> >> return err;
> >> }
> >>
> >> + tegra->opp_table = dev_pm_opp_get_opp_table(&pdev->dev);
> >> + if (IS_ERR(tegra->opp_table))
> >> + return dev_err_probe(&pdev->dev, PTR_ERR(tegra->opp_table),
> >> + "Failed to prepare OPP table\n");
> >
> > As I knew, each device can contain the opp_table on devicetree.
> > It means that opp_table has not depended to another device driver.
> > Did you see this exception case with EPROBE_DEFER error?
>
> OPP core will try to grab the clock reference for the table and it may
> cause EPROBE_DEFER. Although, it shouldn't happen here because we have
> devm_clk_get() before the get_opp_table(), hence seems the deferred
> probe indeed shouldn't happen in this case.
It is my missing point. If there, you're right.
Could you provide the code point to check the clock reference on the OPP core?
--
Best Regards,
Chanwoo Choi
Hi Dmitry,
On Mon, Nov 2, 2020 at 12:24 AM Dmitry Osipenko <[email protected]> wrote:
>
> 01.11.2020 17:44, Chanwoo Choi пишет:
> > Hi Dmitry,
> >
> > On Mon, Oct 26, 2020 at 7:22 AM Dmitry Osipenko <[email protected]> wrote:
> >>
> >> This patch moves ACTMON driver away from generating OPP table by itself,
> >> transitioning it to use the table which comes from device-tree. This
> >> change breaks compatibility with older device-trees in order to bring
> >> support for the interconnect framework to the driver. This is a mandatory
> >> change which needs to be done in order to implement interconnect-based
> >> memory DVFS. Users of legacy device-trees will get a message telling that
> >> theirs DT needs to be upgraded. Now ACTMON issues memory bandwidth request
> >> using dev_pm_opp_set_bw(), instead of driving EMC clock rate directly.
> >>
> >> Tested-by: Peter Geis <[email protected]>
> >> Tested-by: Nicolas Chauvet <[email protected]>
> >> Signed-off-by: Dmitry Osipenko <[email protected]>
> >> ---
> >> drivers/devfreq/tegra30-devfreq.c | 91 ++++++++++++++++---------------
> >> 1 file changed, 48 insertions(+), 43 deletions(-)
> >>
> >> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> >> index 3f732ab53573..1b0b91a71886 100644
> >> --- a/drivers/devfreq/tegra30-devfreq.c
> >> +++ b/drivers/devfreq/tegra30-devfreq.c
> >> @@ -19,6 +19,8 @@
> >> #include <linux/reset.h>
> >> #include <linux/workqueue.h>
> >>
> >> +#include <soc/tegra/fuse.h>
> >> +
> >
> > This patch touches the OPP. Is it related to this change?
>
> Yes, this is needed for the dev_pm_opp_set_supported_hw().
OK.
>
> >> #include "governor.h"
> >>
> >> #define ACTMON_GLB_STATUS 0x0
> >> @@ -155,6 +157,7 @@ struct tegra_devfreq_device {
> >>
> >> struct tegra_devfreq {
> >> struct devfreq *devfreq;
> >> + struct opp_table *opp_table;
> >>
> >> struct reset_control *reset;
> >> struct clk *clock;
> >> @@ -612,34 +615,19 @@ static void tegra_actmon_stop(struct tegra_devfreq *tegra)
> >> static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
> >> u32 flags)
> >> {
> >> - struct tegra_devfreq *tegra = dev_get_drvdata(dev);
> >> - struct devfreq *devfreq = tegra->devfreq;
> >> struct dev_pm_opp *opp;
> >> - unsigned long rate;
> >> - int err;
> >> + int ret;
> >>
> >> opp = devfreq_recommended_opp(dev, freq, flags);
> >> if (IS_ERR(opp)) {
> >> - dev_err(dev, "Failed to find opp for %lu Hz\n", *freq);
> >> + dev_err(dev, "failed to find opp for %lu Hz\n", *freq);
> >
> > You used the 'Failed to' format in almost every error case.
> > Don't need to change it.
> > (snip)
>
> Good catch, thanks.
--
Best Regards,
Chanwoo Choi
01.11.2020 18:44, Chanwoo Choi пишет:
>> OPP core will try to grab the clock reference for the table and it may
>> cause EPROBE_DEFER. Although, it shouldn't happen here because we have
>> devm_clk_get() before the get_opp_table(), hence seems the deferred
>> probe indeed shouldn't happen in this case.
> It is my missing point. If there, you're right.
> Could you provide the code point to check the clock reference on the OPP core?
Please see it here:
https://elixir.bootlin.com/linux/v5.10-rc1/source/drivers/opp/core.c#L1101
On Mon, Nov 2, 2020 at 12:49 AM Dmitry Osipenko <[email protected]> wrote:
>
> 01.11.2020 18:44, Chanwoo Choi пишет:
> >> OPP core will try to grab the clock reference for the table and it may
> >> cause EPROBE_DEFER. Although, it shouldn't happen here because we have
> >> devm_clk_get() before the get_opp_table(), hence seems the deferred
> >> probe indeed shouldn't happen in this case.
> > It is my missing point. If there, you're right.
> > Could you provide the code point to check the clock reference on the OPP core?
>
> Please see it here:
>
> https://elixir.bootlin.com/linux/v5.10-rc1/source/drivers/opp/core.c#L1101
Thanks. It seems that if device tree node contains the any node,
it is not EPROBE_DEFER because of just using "clk_get(dev, NULL)".
The patch[1] used the 'dev_err_probe' for getting the "emc" clock.
Do you need to check it more?
[1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-next&id=09d56d92ad25b58113f4ec677e9b1ac1e2c3072b
--
Best Regards,
Chanwoo Choi
01.11.2020 18:57, Chanwoo Choi пишет:
> On Mon, Nov 2, 2020 at 12:49 AM Dmitry Osipenko <[email protected]> wrote:
>>
>> 01.11.2020 18:44, Chanwoo Choi пишет:
>>>> OPP core will try to grab the clock reference for the table and it may
>>>> cause EPROBE_DEFER. Although, it shouldn't happen here because we have
>>>> devm_clk_get() before the get_opp_table(), hence seems the deferred
>>>> probe indeed shouldn't happen in this case.
>>> It is my missing point. If there, you're right.
>>> Could you provide the code point to check the clock reference on the OPP core?
>>
>> Please see it here:
>>
>> https://elixir.bootlin.com/linux/v5.10-rc1/source/drivers/opp/core.c#L1101
>
> Thanks. It seems that if device tree node contains the any node,
> it is not EPROBE_DEFER because of just using "clk_get(dev, NULL)".
>
> The patch[1] used the 'dev_err_probe' for getting the "emc" clock.
> Do you need to check it more?
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-next&id=09d56d92ad25b58113f4ec677e9b1ac1e2c3072b
It should be safe to assume that the EPROBE_DEFER won't happen for
dev_pm_opp_get_opp_table(). I'll improve it in v7, thanks.
01.11.2020 18:44, Chanwoo Choi пишет:
>>> I recommend that you use dev_pm_opp_of_get_opp_desc_node(&pdev->dev)
>>> to check whether a device contains opp-table or not.
>> I'm not sure what are the benefits, this will make code less
>> expressive/readable and we will need to add extra of_node_put(), which
>> device_property_present() handles for us.
>>
>> Could you please give the rationale?
> IMO, 'operating-points-v2' word was defined on OPP core. I think that
> the external user
> of OPP better to use the public helper function instead of using the
> interval definition
> or value of OPP core directly. Basically, I prefer the provided helper
> function if there.
> But, it is not critical and doesn't affect the operation. If you want
> to keep, it is ok.
>
I'll prefer to keep it since it's better for the readability of the
code, thanks.