Hi Aniket
On 23/10/24 13:28, Aniket Limaye wrote:
On 23/10/24 12:17, Neha Malcom Francis wrote:
Hi Aniket
On 23/10/24 11:42, Aniket Limaye wrote:
On 17/10/24 16:00, Neha Malcom Francis wrote:
Hi Aniket
On 17/10/24 11:59, Aniket Limaye wrote:
From: Reid Tonking reidt@ti.com
Define the MSMC clk in the a72 node
The usage of MSMC and A72SS interchangeably in this series is confusing. Could you expand on it in the cover letter why in J7200 this clock is defined as part of the A72 node as opposed to devices that have MSMC as a separate module altogether with its own clock (like J721S2 and J784S4)?
Yeah sure. will elaborate in v2:
The msmc clock frequency needs to be updated as per selected OPP (in PATCH 4). But we don't have a msmc node for j721e/j7200, unlike those in j721s2/j784s4. So we are defining the msmc clock in the a72_0 node, such that it's frequency can be updated along with core clock frequency when OPP_LOW config is selected.
I am open to suggestions if this is not the right place for it. Thanks for the reviews!
Configuring the MSMC interleaver is only required in devices that have more than one DDR controller i.e. J721S2 and J784S4. This is what is being done by the MSMC driver present in U-Boot (see drivers/ram/k3-ddrss/k3-ddrss.c). This configuration is not needed in this device and is probably why we don't have an explicit MSMC node.
I was thinking the patch is okay initially considering this, but now am wondering whether this is a hacky way to set a clock and should we introduce the MSMC as a node cleanly, after all it is hardware that needs to be described. What do you think?
Yeah I am also not quite sure where the MSMC clk description should go: in a new separate node, or in a72_0.
However, MSMC clk id is described under A72SS0_CORE0 Device in TISCI documentation [0]. Which makes a case for putting this under a72_0 node itself (along with the fact that there isn't a separate msmc node for j721e/j7200 which have a single DDR controller). So I guess I vote for keeping it as is, under a72_0 node.
I am content with this argument, especially since the intention of the MSMC driver in U-Boot is exclusively for setting up the interleaver. (I should probably target cleaning that up and making it a generic driver as a future action!)
Thanks, Aniket
Regards, Aniket
Signed-off-by: Reid Tonking reidt@ti.com Signed-off-by: Aniket Limaye a-limaye@ti.com
arch/arm/dts/k3-j7200-r5-common-proc-board.dts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/arm/dts/k3-j7200-r5-common-proc-board.dts b/arch/arm/dts/k3-j7200-r5-common-proc-board.dts index f096b102793..759a1e83456 100644 --- a/arch/arm/dts/k3-j7200-r5-common-proc-board.dts +++ b/arch/arm/dts/k3-j7200-r5-common-proc-board.dts @@ -23,11 +23,11 @@ <&k3_pds 202 TI_SCI_PD_EXCLUSIVE>, <&k3_pds 4 TI_SCI_PD_EXCLUSIVE>; resets = <&k3_reset 202 0>; - clocks = <&k3_clks 61 1>, <&k3_clks 202 2>; - clock-names = "gtc", "core"; - assigned-clocks = <&k3_clks 202 2>, <&k3_clks 61 1>, <&k3_clks 323 0>; - assigned-clock-parents= <0>, <0>, <&k3_clks 323 2>; - assigned-clock-rates = <2000000000>, <200000000>; + clocks = <&k3_clks 61 1>, <&k3_clks 4 1>, <&k3_clks 202 2> ; + clock-names = "gtc", "msmc", "core"; + assigned-clocks = <&k3_clks 202 2>, <&k3_clks 61 1>, <&k3_clks 4 1>, <&k3_clks 323 0>; + assigned-clock-parents= <0>, <0>, <0>, <&k3_clks 323 2>; + assigned-clock-rates = <2000000000>, <200000000>, <1000000000>; ti,sci = <&dmsc>; ti,sci-proc-id = <32>; ti,sci-host-id = <10>;