[PATCH 1/6] arm: dts: k3-j7200-r5-common: Add msmc clk to a72 node

[Aniket Limaye]

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 at 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.

[0]: 
https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html#clocks-for-a72ss0-core0-device

Thanks,
Aniket

>>
>> Regards,
>> Aniket
>>
>>>>
>>>> Signed-off-by: Reid Tonking <reidt at ti.com>
>>>> Signed-off-by: Aniket Limaye <a-limaye at 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>;
>>>
>