| Message ID | 20241113110955.3876045-2-danishanwar@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add Clocks for ICSSG | expand |
On Wed, Nov 13, 2024 at 04:39:54PM +0530, 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> Acked-by: Conor Dooley <conor.dooley@microchip.com>
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? Only CORE and IEP clocks parent can be configured via clock muxes. Those are already defined in the icssg?_cfg nodes. [1] - https://software-dl.ti.com/tisci/esd/22_01_02/5_soc_doc/am64x/clocks.html > patternProperties: > > memories@[a-f0-9]+$:
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? > > 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]+$: >
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. >> 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]+$: >> >
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] > >>> 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]+$: >>> >> >
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 >> >>>> 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]+$: >>>> >>> >> >
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]+$: >>>>> >>>> >>> >> >
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> Reviewed-by: Roger Quadros <rogerq@kernel.org>
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 + patternProperties: memories@[a-f0-9]+$:
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(+)