Re: [PATCH v2] arch: arm: mach-k3: Delete tifs node in DT fixup
Hi Nishanth, On 5/3/2023 4:30 AM, Nishanth Menon wrote: On 12:57-20230502, Kumar, Udit wrote: On 5/1/2023 8:16 PM, Andrew Davis wrote: On 4/26/23 9:13 AM, Kumar, Udit wrote: Hi Neha, On 4/26/2023 5:31 PM, Neha Malcom Francis wrote: Hi Udit On 26/04/23 16:09, Kumar, Udit wrote: Hi Neha, Hi Udit, [..] [..] What I mean to ask is, why aren't there tifs or l3cache subnodes in j721e, j7200 and am65? I think, above platform is doing in right way, AFAIK, if we have to provide then we can provide size of this. l3-cache can not be addressable. So the history here is we used to have the SRAM node in DT sized to the actual size in hardware. L3 cache size can be set at boot time (in SYSFW board-config file), and that uses up some of the SRAM, so the end address moves in. We could represent this as a reserved node inside the full SRAM node, or by shrinking the SRAM node and hiding this. Same story for TIFS and ATF, they use some variable amount of the end of SRAM. Ah, I have other view. We shrunk SRAM size already, having reserved node on top of SRAM is good as removing this. How about we do this: a) Start by discussing in k.org with a patch as to how we think it should be and what the rationale is. ok b) SRAM size fixup is a consequence of firmware being flexible.. Since, the tifs reserved sram etc, base description exists even after "hardware reconfiguration", u-boot may adjust, but not delete such nodes. "reserved" is part of complete description and indication that this specific OS is not supposed to use this region. That region is protected by firewall and mechanisms to make such access fail, but that is the point of "reserved" nodes. you mean , keep full view of SRAM and update size of reserved node. BTW, L3-cache and tifs will be reserved by default. c) Standardize this across the SoCs that use MSMC (WITHOUT BREAKING FORWARD AND BACKWARD COMPATIBILITY of u-boot vs dtb).
Re: [PATCH v2] arch: arm: mach-k3: Delete tifs node in DT fixup
On 12:57-20230502, Kumar, Udit wrote: > > On 5/1/2023 8:16 PM, Andrew Davis wrote: > > On 4/26/23 9:13 AM, Kumar, Udit wrote: > > > Hi Neha, > > > > > > On 4/26/2023 5:31 PM, Neha Malcom Francis wrote: > > > > Hi Udit > > > > > > > > On 26/04/23 16:09, Kumar, Udit wrote: > > > > > Hi Neha, > > > > > > > > > > > Hi Udit, > > > > > > > > > > [..] > > > > > > > > > > > > > > > > > > > > > I do have a general doubt; why do we have only atf-sram > > > > > > > > sub-node in > > > > > > > > msmc_sram in all other devices (j721e, j7200 and > > > > > > > > am65) except j721s2? > > > > > > > > > > > > > > let me know, which source code you are referring to > > > > > > > > > > > > > > > > > > > In U-Boot, for j721e, j7200 and am65; they *only* contain atf-sram? > > > > > > > > > > For u-boot please see > > > > > > > > > > https://elixir.bootlin.com/u-boot/latest/source/arch/arm/dts/k3-j721s2-main.dtsi#L16 > > > > > > > > > > > > > > > > > I could see for j721s2 as well, in uboot[0] and Linux[1] > > > > > [..] > > > > > > > > > > > > > What I mean to ask is, why aren't there tifs or l3cache subnodes > > > > in j721e, j7200 and am65? > > > > > > > I think, above platform is doing in right way, > > > > > > AFAIK, if we have to provide then we can provide size of this. > > > > > > l3-cache can not be addressable. > > > > > > > > > So the history here is we used to have the SRAM node in DT sized > > to the actual size in hardware. L3 cache size can be set at boot > > time (in SYSFW board-config file), and that uses up some of the > > SRAM, so the end address moves in. We could represent this as > > a reserved node inside the full SRAM node, or by shrinking the > > SRAM node and hiding this. Same story for TIFS and ATF, they > > use some variable amount of the end of SRAM. > > > Ah, I have other view. > > We shrunk SRAM size already, having reserved node on top of SRAM > > is good as removing this. How about we do this: a) Start by discussing in k.org with a patch as to how we think it should be and what the rationale is. b) SRAM size fixup is a consequence of firmware being flexible.. Since, the tifs reserved sram etc, base description exists even after "hardware reconfiguration", u-boot may adjust, but not delete such nodes. "reserved" is part of complete description and indication that this specific OS is not supposed to use this region. That region is protected by firewall and mechanisms to make such access fail, but that is the point of "reserved" nodes. c) Standardize this across the SoCs that use MSMC (WITHOUT BREAKING FORWARD AND BACKWARD COMPATIBILITY of u-boot vs dtb). -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
Re: [PATCH v2] arch: arm: mach-k3: Delete tifs node in DT fixup
On 5/1/2023 8:16 PM, Andrew Davis wrote: On 4/26/23 9:13 AM, Kumar, Udit wrote: Hi Neha, On 4/26/2023 5:31 PM, Neha Malcom Francis wrote: Hi Udit On 26/04/23 16:09, Kumar, Udit wrote: Hi Neha, Hi Udit, [..] I do have a general doubt; why do we have only atf-sram sub-node in msmc_sram in all other devices (j721e, j7200 and am65) except j721s2? let me know, which source code you are referring to In U-Boot, for j721e, j7200 and am65; they *only* contain atf-sram? For u-boot please see https://elixir.bootlin.com/u-boot/latest/source/arch/arm/dts/k3-j721s2-main.dtsi#L16 I could see for j721s2 as well, in uboot[0] and Linux[1] [..] What I mean to ask is, why aren't there tifs or l3cache subnodes in j721e, j7200 and am65? I think, above platform is doing in right way, AFAIK, if we have to provide then we can provide size of this. l3-cache can not be addressable. So the history here is we used to have the SRAM node in DT sized to the actual size in hardware. L3 cache size can be set at boot time (in SYSFW board-config file), and that uses up some of the SRAM, so the end address moves in. We could represent this as a reserved node inside the full SRAM node, or by shrinking the SRAM node and hiding this. Same story for TIFS and ATF, they use some variable amount of the end of SRAM. Ah, I have other view. We shrunk SRAM size already, having reserved node on top of SRAM is good as removing this. I'd prefer being explicit and keep these nodes. Andrew But in any case, u-boot removes this code before passing to OS. https://elixir.bootlin.com/u-boot/latest/source/arch/arm/mach-k3/common.c#L354 Thanking You Neha Malcom Francis
Re: [PATCH v2] arch: arm: mach-k3: Delete tifs node in DT fixup
Hi Andrew On 01/05/23 20:16, Andrew Davis wrote: On 4/26/23 9:13 AM, Kumar, Udit wrote: Hi Neha, On 4/26/2023 5:31 PM, Neha Malcom Francis wrote: Hi Udit On 26/04/23 16:09, Kumar, Udit wrote: Hi Neha, Hi Udit, [..] I do have a general doubt; why do we have only atf-sram sub-node in msmc_sram in all other devices (j721e, j7200 and am65) except j721s2? let me know, which source code you are referring to In U-Boot, for j721e, j7200 and am65; they *only* contain atf-sram? For u-boot please see https://elixir.bootlin.com/u-boot/latest/source/arch/arm/dts/k3-j721s2-main.dtsi#L16 I could see for j721s2 as well, in uboot[0] and Linux[1] [..] What I mean to ask is, why aren't there tifs or l3cache subnodes in j721e, j7200 and am65? I think, above platform is doing in right way, AFAIK, if we have to provide then we can provide size of this. l3-cache can not be addressable. So the history here is we used to have the SRAM node in DT sized to the actual size in hardware. L3 cache size can be set at boot time (in SYSFW board-config file), and that uses up some of the SRAM, so the end address moves in. We could represent this as a reserved node inside the full SRAM node, or by shrinking the SRAM node and hiding this. Same story for TIFS and ATF, they use some variable amount of the end of SRAM. I'd prefer being explicit and keep these nodes. Andrew Thanks for the explanation, it does seem better to keep these nodes giving the complete description. But in any case, u-boot removes this code before passing to OS. https://elixir.bootlin.com/u-boot/latest/source/arch/arm/mach-k3/common.c#L354 Thanking You Neha Malcom Francis -- Thanking You Neha Malcom Francis
Re: [PATCH v2] arch: arm: mach-k3: Delete tifs node in DT fixup
On 4/26/23 9:13 AM, Kumar, Udit wrote: Hi Neha, On 4/26/2023 5:31 PM, Neha Malcom Francis wrote: Hi Udit On 26/04/23 16:09, Kumar, Udit wrote: Hi Neha, Hi Udit, [..] I do have a general doubt; why do we have only atf-sram sub-node in msmc_sram in all other devices (j721e, j7200 and am65) except j721s2? let me know, which source code you are referring to In U-Boot, for j721e, j7200 and am65; they *only* contain atf-sram? For u-boot please see https://elixir.bootlin.com/u-boot/latest/source/arch/arm/dts/k3-j721s2-main.dtsi#L16 I could see for j721s2 as well, in uboot[0] and Linux[1] [..] What I mean to ask is, why aren't there tifs or l3cache subnodes in j721e, j7200 and am65? I think, above platform is doing in right way, AFAIK, if we have to provide then we can provide size of this. l3-cache can not be addressable. So the history here is we used to have the SRAM node in DT sized to the actual size in hardware. L3 cache size can be set at boot time (in SYSFW board-config file), and that uses up some of the SRAM, so the end address moves in. We could represent this as a reserved node inside the full SRAM node, or by shrinking the SRAM node and hiding this. Same story for TIFS and ATF, they use some variable amount of the end of SRAM. I'd prefer being explicit and keep these nodes. Andrew But in any case, u-boot removes this code before passing to OS. https://elixir.bootlin.com/u-boot/latest/source/arch/arm/mach-k3/common.c#L354 Thanking You Neha Malcom Francis
Re: [PATCH v2] arch: arm: mach-k3: Delete tifs node in DT fixup
Hi Neha, On 4/26/2023 5:31 PM, Neha Malcom Francis wrote: Hi Udit On 26/04/23 16:09, Kumar, Udit wrote: Hi Neha, Hi Udit, [..] I do have a general doubt; why do we have only atf-sram sub-node in msmc_sram in all other devices (j721e, j7200 and am65) except j721s2? let me know, which source code you are referring to In U-Boot, for j721e, j7200 and am65; they *only* contain atf-sram? For u-boot please see https://elixir.bootlin.com/u-boot/latest/source/arch/arm/dts/k3-j721s2-main.dtsi#L16 I could see for j721s2 as well, in uboot[0] and Linux[1] [..] What I mean to ask is, why aren't there tifs or l3cache subnodes in j721e, j7200 and am65? I think, above platform is doing in right way, AFAIK, if we have to provide then we can provide size of this. l3-cache can not be addressable. But in any case, u-boot removes this code before passing to OS. https://elixir.bootlin.com/u-boot/latest/source/arch/arm/mach-k3/common.c#L354 Thanking You Neha Malcom Francis
Re: [PATCH v2] arch: arm: mach-k3: Delete tifs node in DT fixup
On 17:31-20230426, Neha Malcom Francis wrote: > Hi Udit > > On 26/04/23 16:09, Kumar, Udit wrote: > > Hi Neha, > > > > > Hi Udit, > > > > [..] > > > > > > > > > > > > I do have a general doubt; why do we have only atf-sram sub-node in > > > > > msmc_sram in all other devices (j721e, j7200 and am65) except j721s2? > > > > > > > > let me know, which source code you are referring to > > > > > > > > > > In U-Boot, for j721e, j7200 and am65; they *only* contain atf-sram? > > > > For u-boot please see > > > > https://elixir.bootlin.com/u-boot/latest/source/arch/arm/dts/k3-j721s2-main.dtsi#L16 > > > > > > I could see for j721s2 as well, in uboot[0] and Linux[1] > > [..] > > > > What I mean to ask is, why aren't there tifs or l3cache subnodes in j721e, > j7200 and am65? > am65x has it https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/ti/k3-am65-main.dtsi#n17 J7200 and j721e is missing it https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi#n27 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi#n15 we need to fix it. -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
Re: [PATCH v2] arch: arm: mach-k3: Delete tifs node in DT fixup
Hi Udit On 26/04/23 16:09, Kumar, Udit wrote: Hi Neha, Hi Udit, [..] I do have a general doubt; why do we have only atf-sram sub-node in msmc_sram in all other devices (j721e, j7200 and am65) except j721s2? let me know, which source code you are referring to In U-Boot, for j721e, j7200 and am65; they *only* contain atf-sram? For u-boot please see https://elixir.bootlin.com/u-boot/latest/source/arch/arm/dts/k3-j721s2-main.dtsi#L16 I could see for j721s2 as well, in uboot[0] and Linux[1] [..] What I mean to ask is, why aren't there tifs or l3cache subnodes in j721e, j7200 and am65? Thanking You Neha Malcom Francis -- Thanking You Neha Malcom Francis
RE: [PATCH v2] arch: arm: mach-k3: Delete tifs node in DT fixup
Hi Neha, > Hi Udit, [..] >>> >>> I do have a general doubt; why do we have only atf-sram sub-node in >>> msmc_sram in all other devices (j721e, j7200 and am65) except j721s2? >> >> let me know, which source code you are referring to >> > >In U-Boot, for j721e, j7200 and am65; they *only* contain atf-sram? For u-boot please see https://elixir.bootlin.com/u-boot/latest/source/arch/arm/dts/k3-j721s2-main.dtsi#L16 >> I could see for j721s2 as well, in uboot[0] and Linux[1] [..] >Thanking You >Neha Malcom Francis
Re: [PATCH v2] arch: arm: mach-k3: Delete tifs node in DT fixup
Hi Udit,
On 26/04/23 15:35, Kumar, Udit wrote:
Hi Neha
On 4/26/2023 2:56 PM, Neha Malcom Francis wrote:
Hi Udit
On 20/04/23 13:41, Udit Kumar wrote:
This patch deletes tifs DT node as part of fixup.
TISCI API reported msmc_size, does not include
64KB reserved size for tifs aka MSMC comms memory.
As part of fixup, original code uses TISCI API
reported msmc_size as size for sram DT node.
tifs node is similar to l3-cache, which should
hold address above msms_size, and should be deleted
before passing control to OS.
Documentation
https://software-dl.ti.com/tisci/esd/latest/2_tisci_msgs/general/core.html?highlight=msmc#tisci-msg-query-msmc
Signed-off-by: Udit Kumar
---
Changes since v1:
https://lore.kernel.org/all/20230419061352.3156023-1-u-kum...@ti.com/
- moved tifs check above l3 as
arch/arm/mach-k3/common.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
index a2adb791f6..33b1f10d58 100644
--- a/arch/arm/mach-k3/common.c
+++ b/arch/arm/mach-k3/common.c
@@ -351,6 +351,7 @@ int fdt_fixup_msmc_ram(void *blob, char
*parent_path, char *node_name)
subnode, addr, size);
if (addr + size > msmc_size ||
!strncmp(fdt_get_name(blob, subnode, &len), "sysfw", 5) ||
+ !strncmp(fdt_get_name(blob, subnode, &len), "tifs", 4) ||
!strncmp(fdt_get_name(blob, subnode, &len), "l3cache",
7)) {
fdt_del_node(blob, subnode);
debug("%s: deleting subnode %d\n", __func__, subnode);
Reviewed-by: Neha Malcom Francis
I do have a general doubt; why do we have only atf-sram sub-node in
msmc_sram in all other devices (j721e, j7200 and am65) except j721s2?
let me know, which source code you are referring to
In U-Boot, for j721e, j7200 and am65; they *only* contain atf-sram?
I could see for j721s2 as well, in uboot[0] and Linux[1]
[0]
https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/ti/k3-j721s2-main.dtsi#L16
[1]
https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/ti/k3-j721s2-main.dtsi#L16
--
Thanking You
Neha Malcom Francis
Re: [PATCH v2] arch: arm: mach-k3: Delete tifs node in DT fixup
Hi Neha
On 4/26/2023 2:56 PM, Neha Malcom Francis wrote:
Hi Udit
On 20/04/23 13:41, Udit Kumar wrote:
This patch deletes tifs DT node as part of fixup.
TISCI API reported msmc_size, does not include
64KB reserved size for tifs aka MSMC comms memory.
As part of fixup, original code uses TISCI API
reported msmc_size as size for sram DT node.
tifs node is similar to l3-cache, which should
hold address above msms_size, and should be deleted
before passing control to OS.
Documentation
https://software-dl.ti.com/tisci/esd/latest/2_tisci_msgs/general/core.html?highlight=msmc#tisci-msg-query-msmc
Signed-off-by: Udit Kumar
---
Changes since v1:
https://lore.kernel.org/all/20230419061352.3156023-1-u-kum...@ti.com/
- moved tifs check above l3 as
arch/arm/mach-k3/common.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
index a2adb791f6..33b1f10d58 100644
--- a/arch/arm/mach-k3/common.c
+++ b/arch/arm/mach-k3/common.c
@@ -351,6 +351,7 @@ int fdt_fixup_msmc_ram(void *blob, char
*parent_path, char *node_name)
subnode, addr, size);
if (addr + size > msmc_size ||
!strncmp(fdt_get_name(blob, subnode, &len), "sysfw", 5) ||
+ !strncmp(fdt_get_name(blob, subnode, &len), "tifs", 4) ||
!strncmp(fdt_get_name(blob, subnode, &len), "l3cache",
7)) {
fdt_del_node(blob, subnode);
debug("%s: deleting subnode %d\n", __func__, subnode);
Reviewed-by: Neha Malcom Francis
I do have a general doubt; why do we have only atf-sram sub-node in
msmc_sram in all other devices (j721e, j7200 and am65) except j721s2?
let me know, which source code you are referring to
I could see for j721s2 as well, in uboot[0] and Linux[1]
[0]
https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/ti/k3-j721s2-main.dtsi#L16
[1]
https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/ti/k3-j721s2-main.dtsi#L16
Re: [PATCH v2] arch: arm: mach-k3: Delete tifs node in DT fixup
Hi Udit
On 20/04/23 13:41, Udit Kumar wrote:
This patch deletes tifs DT node as part of fixup.
TISCI API reported msmc_size, does not include
64KB reserved size for tifs aka MSMC comms memory.
As part of fixup, original code uses TISCI API
reported msmc_size as size for sram DT node.
tifs node is similar to l3-cache, which should
hold address above msms_size, and should be deleted
before passing control to OS.
Documentation
https://software-dl.ti.com/tisci/esd/latest/2_tisci_msgs/general/core.html?highlight=msmc#tisci-msg-query-msmc
Signed-off-by: Udit Kumar
---
Changes since v1:
https://lore.kernel.org/all/20230419061352.3156023-1-u-kum...@ti.com/
- moved tifs check above l3 as
arch/arm/mach-k3/common.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
index a2adb791f6..33b1f10d58 100644
--- a/arch/arm/mach-k3/common.c
+++ b/arch/arm/mach-k3/common.c
@@ -351,6 +351,7 @@ int fdt_fixup_msmc_ram(void *blob, char *parent_path, char
*node_name)
subnode, addr, size);
if (addr + size > msmc_size ||
!strncmp(fdt_get_name(blob, subnode, &len), "sysfw", 5) ||
+ !strncmp(fdt_get_name(blob, subnode, &len), "tifs", 4) ||
!strncmp(fdt_get_name(blob, subnode, &len), "l3cache", 7)) {
fdt_del_node(blob, subnode);
debug("%s: deleting subnode %d\n", __func__, subnode);
Reviewed-by: Neha Malcom Francis
I do have a general doubt; why do we have only atf-sram sub-node in
msmc_sram in all other devices (j721e, j7200 and am65) except j721s2?
--
Thanking You
Neha Malcom Francis
