Re: [PATCH v3 1/2] dt-bindings: soc: ti: pruss: Add clocks for ICSSG

[Roger Quadros <rogerq@kernel.org>]

On 20/11/2024 15:23, Anwar, Md Danish wrote:
> Hi Roger,
> 
> On 11/19/2024 4:15 PM, Roger Quadros wrote:
>>
>>
>> On 19/11/2024 08:12, Vignesh Raghavendra wrote:
>>>
>>>
>>> On 18/11/24 19:22, Roger Quadros wrote:
>>>>
>>>>
>>>> On 18/11/2024 15:33, Roger Quadros wrote:
>>>>> Hi,
>>>>>
>>>>> On 13/11/2024 13:09, MD Danish Anwar wrote:
>>>>>> The ICSSG module has 7 clocks for each instance.
>>>>>>
>>>>>> These clocks are ICSSG0_CORE_CLK, ICSSG0_IEP_CLK, ICSSG0_ICLK,
>>>>>> ICSSG0_UART_CLK, RGMII_MHZ_250_CLK, RGMII_MHZ_50_CLK and RGMII_MHZ_5_CLK
>>>>>> These clocks are described in AM64x TRM Section 6.4.3 Table 6-398.
>>>>>>
>>>>>> Add these clocks to the dt binding of ICSSG.
>>>>>>
>>>>>> Link: https://www.ti.com/lit/pdf/spruim2 (AM64x TRM)
>>>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml | 10 ++++++++++
>>>>>>  1 file changed, 10 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
>>>>>> index 3cb1471cc6b6..927b3200e29e 100644
>>>>>> --- a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
>>>>>> @@ -92,6 +92,16 @@ properties:
>>>>>>      description: |
>>>>>>        This property is as per sci-pm-domain.txt.
>>>>>>  
>>>>>> +  clocks:
>>>>>> +    items:
>>>>>> +      - description: ICSSG_CORE Clock
>>>>>> +      - description: ICSSG_IEP Clock
>>>>>> +      - description: ICSSG_RGMII_MHZ_250 Clock
>>>>>> +      - description: ICSSG_RGMII_MHZ_50 Clock
>>>>>> +      - description: ICSSG_RGMII_MHZ_5 Clock
>>>>>> +      - description: ICSSG_UART Clock
>>>>>> +      - description: ICSSG_ICLK Clock
>>>>>> +
>>>>>
>>>>> There are actually many more clocks [1]
>>>>> What is the purpose of adding all these clocks in the DT if driver doesn't
>>>>> use them?
>>>>>
>>>
>>> DT should completely describe the HW and not based on what Linux driver
>>> needs. So its valid to describe all clock inputs to a module
>>> irrespective of what driver does with it.
>>
>> Fair point. But there are a total 11 clocks instead of 7 in [1]
>>
> 
> I took the list of clocks from AM64x TRM [1] Section 6.4.3 Table 6-398.
> In the TRM only 7 clocks are mentioned per ICSSG instance which I have
> mentioned in the binding.
> 
> [1] https://www.ti.com/lit/ug/spruim2h/spruim2h.pdf?ts=1732108738816

OK thanks for the clarification. It looks like the same in AM65 and J721e TRMs as well.
So it is fine.

> 
>>>
>>>>> Only CORE and IEP clocks parent can be configured via clock muxes.
>>>>> Those are already defined in the icssg?_cfg nodes.
>>>>
>>>> Actually those clock muxes are internal to ICSSG.
>>>> We still need to be able to set clock parents of CORE and IEP clock.
>>>>
>>>> So pruss block needs at most 2 clocks like you had in v2 of this patch?
>>>>
>>>>>
>>>>> [1] - https://software-dl.ti.com/tisci/esd/22_01_02/5_soc_doc/am64x/clocks.html
>>>>>
>>>>>>  patternProperties:
>>>>>>  
>>>>>>    memories@[a-f0-9]+$:
>>>>>
>>>>
>>>
>>
> 

-- 
cheers,
-roger