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

[Neha Malcom Francis]

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


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!)

> 
> [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>;
>>>>
>>

-- 
Thanking You
Neha Malcom Francis