[PATCH 0/2] Re-introduce parent clock-rate set for fixed-factor clock

[PATCH 0/2] Re-introduce parent clock-rate set for fixed-factor clock

[Aradhya Bhatia]

Hi all,

The support for configuring CLK_SET_RATE_PARENT flag for a few specific
clocks was only used by the legacy Allwinner A10 sunxi clock, and the
reason its compatible was dropped was that the code supporting the
legacy sunxi platforms was removed (as the below-mentioned patch set
highlights) and no other fixed factor clock needed to do the same.

https://lore.kernel.org/lkml/20220531051742.43273-1-samuel@sholland.org/T/


The current patch series adds "ti,k3-am62-oldi-clk-div" (TI's display
subsystem (DSS) clock for the 1st videoport (vp0) on the AM625 SoC) as
a fixed factor clock and further, it also re-introduces the same support
to set the CLK_SET_RATE_PARENT flag.

Based on the clock-set request from DSS (equivalent to pixel frequency),
this clock asks its parent for a serial clock (with 7 times the pixel
frequrncy), which is required for the generation of serial LVDS signals.
This clock thus requires the CLK_SET_RATE_PARENT flag to be set, in
order to propagate the set clock-rate request to its parent clock.

Aradhya Bhatia (2):
  dt-bindings: clock: fixed-factor: Add TI AM62 SoC OLDI clock
  clk: fixed-factor: Re-introduce support for clocks to set parent
    clock-rate

 .../devicetree/bindings/clock/fixed-factor-clock.yaml |  1 +
 drivers/clk/clk-fixed-factor.c                        | 11 ++++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

-- 
2.39.0

[PATCH 1/2] dt-bindings: clock: fixed-factor: Add TI AM62 SoC OLDI clock

[Aradhya Bhatia]

Add "ti,k3-am62-oldi-clk-div" to the fixed factor clock compatible enum
list.

"ti,k3-am62-oldi-clk-div" is a fixed-factor clock that helps the TI
display subsystem request a pixel clock for itself and a corresponding
serial clock for its OLDI Transmitters. The serial clock is 7 times the
pixel clock. This clock needs the clock set rate request to be
propagated to the parent clock provider.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml b/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
index 8f71ab300470..0696237530f7 100644
--- a/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
+++ b/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
@@ -14,6 +14,7 @@ properties:
   compatible:
     enum:
       - fixed-factor-clock
+      - ti,k3-am62-oldi-clk-div
 
   "#clock-cells":
     const: 0
-- 
2.39.0

[PATCH 2/2] clk: fixed-factor: Re-introduce support for clocks to set parent clock-rate

[Aradhya Bhatia]

Add support for the clock "ti,k3-am62-oldi-clk-div".
Also add support for this clock to propagate the clock set request to
its parent clock, by setting the CLK_SET_RATE_PARENT flag.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/clk/clk-fixed-factor.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index f734e34735a9..1a78e2d870dd 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -245,10 +245,16 @@ struct clk_hw *devm_clk_hw_register_fixed_factor(struct device *dev,
 EXPORT_SYMBOL_GPL(devm_clk_hw_register_fixed_factor);
 
 #ifdef CONFIG_OF
+static const struct of_device_id set_rate_parent_matches[] = {
+	{ .compatible = "ti,k3-am62-oldi-clk-div" },
+	{ /* Sentinel */ },
+};
+
 static struct clk_hw *_of_fixed_factor_clk_setup(struct device_node *node)
 {
 	struct clk_hw *hw;
 	const char *clk_name = node->name;
+	unsigned long flags = 0;
 	u32 div, mult;
 	int ret;
 
@@ -264,10 +270,13 @@ static struct clk_hw *_of_fixed_factor_clk_setup(struct device_node *node)
 		return ERR_PTR(-EIO);
 	}
 
+	if (of_match_node(set_rate_parent_matches, node))
+		flags |= CLK_SET_RATE_PARENT;
+
 	of_property_read_string(node, "clock-output-names", &clk_name);
 
 	hw = __clk_hw_register_fixed_factor(NULL, node, clk_name, NULL, NULL, 0,
-					    0, mult, div, false);
+					    flags, mult, div, false);
 	if (IS_ERR(hw)) {
 		/*
 		 * Clear OF_POPULATED flag so that clock registration can be
-- 
2.39.0

Re: [PATCH 1/2] dt-bindings: clock: fixed-factor: Add TI AM62 SoC OLDI clock

[Rob Herring]

On Mon, 26 Dec 2022 15:27:44 +0530, Aradhya Bhatia wrote:
> Add "ti,k3-am62-oldi-clk-div" to the fixed factor clock compatible enum
> list.
> 
> "ti,k3-am62-oldi-clk-div" is a fixed-factor clock that helps the TI
> display subsystem request a pixel clock for itself and a corresponding
> serial clock for its OLDI Transmitters. The serial clock is 7 times the
> pixel clock. This clock needs the clock set rate request to be
> propagated to the parent clock provider.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>  Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 1/2] dt-bindings: clock: fixed-factor: Add TI AM62 SoC OLDI clock

[Stephen Boyd]

Quoting Aradhya Bhatia (2022-12-26 01:57:44)
> Add "ti,k3-am62-oldi-clk-div" to the fixed factor clock compatible enum
> list.
> 
> "ti,k3-am62-oldi-clk-div" is a fixed-factor clock that helps the TI
> display subsystem request a pixel clock for itself and a corresponding
> serial clock for its OLDI Transmitters. The serial clock is 7 times the
> pixel clock. This clock needs the clock set rate request to be
> propagated to the parent clock provider.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>  Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml b/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
> index 8f71ab300470..0696237530f7 100644
> --- a/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
> +++ b/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
> @@ -14,6 +14,7 @@ properties:
>    compatible:
>      enum:
>        - fixed-factor-clock
> +      - ti,k3-am62-oldi-clk-div

I don't see this compatible anywhere in the kernel tree. Is there a
patch that adds a node using this? I wonder why the display subsystem
can't add this fixed factor clk directly in the driver. Does the OLDI
Transmitter send a clk to the display subsystem?

I'm asking all these questions because we got rid of vendor compatibles
here in hopes of simplifying the logic. Maybe the problem can be
approached differently, but I don't know all the details.

Re: [PATCH 1/2] dt-bindings: clock: fixed-factor: Add TI AM62 SoC OLDI clock

[Aradhya Bhatia]

Hi Stephen,

Thanks for taking a look at the patch.

On 12-Jan-23 01:14, Stephen Boyd wrote:
> Quoting Aradhya Bhatia (2022-12-26 01:57:44)
>> Add "ti,k3-am62-oldi-clk-div" to the fixed factor clock compatible enum
>> list.
>>
>> "ti,k3-am62-oldi-clk-div" is a fixed-factor clock that helps the TI
>> display subsystem request a pixel clock for itself and a corresponding
>> serial clock for its OLDI Transmitters. The serial clock is 7 times the
>> pixel clock. This clock needs the clock set rate request to be
>> propagated to the parent clock provider.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>   Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml b/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
>> index 8f71ab300470..0696237530f7 100644
>> --- a/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
>> +++ b/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
>> @@ -14,6 +14,7 @@ properties:
>>     compatible:
>>       enum:
>>         - fixed-factor-clock
>> +      - ti,k3-am62-oldi-clk-div
> 
> I don't see this compatible anywhere in the kernel tree. Is there a
> patch that adds a node using this? I wonder why the display subsystem
> can't add this fixed factor clk directly in the driver. Does the OLDI
> Transmitter send a clk to the display subsystem?
> 
> I'm asking all these questions because we got rid of vendor compatibles
> here in hopes of simplifying the logic. Maybe the problem can be
> approached differently, but I don't know all the details.


+--------+                       +------------------+
|        |                       |                  |
|  PLL   +---+----+------------->| OLDI Transmitter |
|        |   |    |              |                  |
+--------+   |    |              +------------------+
              |    |
              |    |              +------------------+
              |    |              |                  |
              |    +------------->| OLDI Transmitter |
              |                   |                  |
              |                   +------------------+
              |
              |                   +------------------+
              |   +----------+    |                  |
              |   |    /7    |    |      Display     |
              +-->|   Clock  +--->| Sub-System (DSS) |
                  |    Div   |    |                  |
                  +----------+    +------------------+

This is how the the clock architecture for DSS looks like.

The clock divider is not a part of DSS, but outside it.

The clock request flow is initiated by the DSS driver because it has the
required timing parameter information. It requests a certain pixel
frequency. But the frequency required by the OLDI TXes is 7 times
that pixel frequency.

(Just for clarification, in some cases, the OLDI TX does require only
3.5 times the pixel frequency, but in those situations there is another
divider in-front of OLDI TX that gets activated with a signal and
divides the incoming frequency by 2, thereby requiring the PLL to still
generate a 7x frequency.)

Hence, the idea is that the clock divider is able to propagate the set
rate request back to PLL, asking for a frequency 7 times more than the
DSS's asking rate.

If this is something less than ideal and should not go up, then I can
implement a new clock device with a separate but similar clock driver.

Let me know what you think!


Regards
Aradhya

Re: [PATCH 1/2] dt-bindings: clock: fixed-factor: Add TI AM62 SoC OLDI clock

[Tomi Valkeinen]

On 16/01/2023 11:51, Aradhya Bhatia wrote:
> Hi Stephen,
> 
> Thanks for taking a look at the patch.
> 
> On 12-Jan-23 01:14, Stephen Boyd wrote:
>> Quoting Aradhya Bhatia (2022-12-26 01:57:44)
>>> Add "ti,k3-am62-oldi-clk-div" to the fixed factor clock compatible enum
>>> list.
>>>
>>> "ti,k3-am62-oldi-clk-div" is a fixed-factor clock that helps the TI
>>> display subsystem request a pixel clock for itself and a corresponding
>>> serial clock for its OLDI Transmitters. The serial clock is 7 times the
>>> pixel clock. This clock needs the clock set rate request to be
>>> propagated to the parent clock provider.
>>>
>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>> ---
>>>   Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml 
>>> b/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
>>> index 8f71ab300470..0696237530f7 100644
>>> --- a/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
>>> +++ b/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
>>> @@ -14,6 +14,7 @@ properties:
>>>     compatible:
>>>       enum:
>>>         - fixed-factor-clock
>>> +      - ti,k3-am62-oldi-clk-div
>>
>> I don't see this compatible anywhere in the kernel tree. Is there a
>> patch that adds a node using this? I wonder why the display subsystem
>> can't add this fixed factor clk directly in the driver. Does the OLDI
>> Transmitter send a clk to the display subsystem?
>>
>> I'm asking all these questions because we got rid of vendor compatibles
>> here in hopes of simplifying the logic. Maybe the problem can be
>> approached differently, but I don't know all the details.
> 
> 
> +--------+                       +------------------+
> |        |                       |                  |
> |  PLL   +---+----+------------->| OLDI Transmitter |
> |        |   |    |              |                  |
> +--------+   |    |              +------------------+
>               |    |
>               |    |              +------------------+
>               |    |              |                  |
>               |    +------------->| OLDI Transmitter |
>               |                   |                  |
>               |                   +------------------+
>               |
>               |                   +------------------+
>               |   +----------+    |                  |
>               |   |    /7    |    |      Display     |
>               +-->|   Clock  +--->| Sub-System (DSS) |
>                   |    Div   |    |                  |
>                   +----------+    +------------------+
> 
> This is how the the clock architecture for DSS looks like.
> 
> The clock divider is not a part of DSS, but outside it.
> 
> The clock request flow is initiated by the DSS driver because it has the
> required timing parameter information. It requests a certain pixel
> frequency. But the frequency required by the OLDI TXes is 7 times
> that pixel frequency.
> 
> (Just for clarification, in some cases, the OLDI TX does require only
> 3.5 times the pixel frequency, but in those situations there is another
> divider in-front of OLDI TX that gets activated with a signal and
> divides the incoming frequency by 2, thereby requiring the PLL to still
> generate a 7x frequency.)
> 
> Hence, the idea is that the clock divider is able to propagate the set
> rate request back to PLL, asking for a frequency 7 times more than the
> DSS's asking rate.
> 
> If this is something less than ideal and should not go up, then I can
> implement a new clock device with a separate but similar clock driver.
> 
> Let me know what you think!

As a clarification I would also add to the above that on other TI SoCs 
with DSS, and also for the second video port on AM62, the clock 
framework provides DSS a clock using the pclk frequency.

  Tomi

Re: [PATCH 1/2] dt-bindings: clock: fixed-factor: Add TI AM62 SoC OLDI clock

[Stephen Boyd]

Quoting Tomi Valkeinen (2023-01-17 01:40:24)
> On 16/01/2023 11:51, Aradhya Bhatia wrote:
> > Hi Stephen,
> > 
> > Thanks for taking a look at the patch.
> > 
> > On 12-Jan-23 01:14, Stephen Boyd wrote:
> >> Quoting Aradhya Bhatia (2022-12-26 01:57:44)
> >>> Add "ti,k3-am62-oldi-clk-div" to the fixed factor clock compatible enum
> >>> list.
> >>>
> >>> "ti,k3-am62-oldi-clk-div" is a fixed-factor clock that helps the TI
> >>> display subsystem request a pixel clock for itself and a corresponding
> >>> serial clock for its OLDI Transmitters. The serial clock is 7 times the
> >>> pixel clock. This clock needs the clock set rate request to be
> >>> propagated to the parent clock provider.
> >>>
> >>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> >>> ---
> >>>   Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml | 1 +
> >>>   1 file changed, 1 insertion(+)
> >>>
> >>> diff --git 
> >>> a/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml 
> >>> b/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
> >>> index 8f71ab300470..0696237530f7 100644
> >>> --- a/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
> >>> +++ b/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
> >>> @@ -14,6 +14,7 @@ properties:
> >>>     compatible:
> >>>       enum:
> >>>         - fixed-factor-clock
> >>> +      - ti,k3-am62-oldi-clk-div
> >>
> >> I don't see this compatible anywhere in the kernel tree. Is there a
> >> patch that adds a node using this? I wonder why the display subsystem
> >> can't add this fixed factor clk directly in the driver. Does the OLDI
> >> Transmitter send a clk to the display subsystem?
> >>
> >> I'm asking all these questions because we got rid of vendor compatibles
> >> here in hopes of simplifying the logic. Maybe the problem can be
> >> approached differently, but I don't know all the details.
> > 
> > 
> > +--------+                       +------------------+
> > |        |                       |                  |
> > |  PLL   +---+----+------------->| OLDI Transmitter |
> > |        |   |    |              |                  |
> > +--------+   |    |              +------------------+
> >               |    |
> >               |    |              +------------------+
> >               |    |              |                  |
> >               |    +------------->| OLDI Transmitter |
> >               |                   |                  |
> >               |                   +------------------+
> >               |
> >               |                   +------------------+
> >               |   +----------+    |                  |
> >               |   |    /7    |    |      Display     |
> >               +-->|   Clock  +--->| Sub-System (DSS) |
> >                   |    Div   |    |                  |
> >                   +----------+    +------------------+
> > 
> > This is how the the clock architecture for DSS looks like.
> > 
> > The clock divider is not a part of DSS, but outside it.

The divider is fixed as well? And presumably inside the SoC?

> > 
> > The clock request flow is initiated by the DSS driver because it has the
> > required timing parameter information. It requests a certain pixel
> > frequency. But the frequency required by the OLDI TXes is 7 times
> > that pixel frequency.
> > 
> > (Just for clarification, in some cases, the OLDI TX does require only
> > 3.5 times the pixel frequency, but in those situations there is another
> > divider in-front of OLDI TX that gets activated with a signal and
> > divides the incoming frequency by 2, thereby requiring the PLL to still
> > generate a 7x frequency.)
> > 
> > Hence, the idea is that the clock divider is able to propagate the set
> > rate request back to PLL, asking for a frequency 7 times more than the
> > DSS's asking rate.

Got it. Can the PLL driver provide a pll_div_7 clk that is used for the
DSS pixel clk?

> > 
> > If this is something less than ideal and should not go up, then I can
> > implement a new clock device with a separate but similar clock driver.
> > 
> > Let me know what you think!
> 
> As a clarification I would also add to the above that on other TI SoCs 
> with DSS, and also for the second video port on AM62, the clock 
> framework provides DSS a clock using the pclk frequency.
> 

Are you saying that adding a fixed div-7 clk in the DSS driver is wrong?

Re: [PATCH 1/2] dt-bindings: clock: fixed-factor: Add TI AM62 SoC OLDI clock

[Aradhya Bhatia]

Hi Stephen,

On 26-Jan-23 05:36, Stephen Boyd wrote:
> Quoting Tomi Valkeinen (2023-01-17 01:40:24)
>> On 16/01/2023 11:51, Aradhya Bhatia wrote:
>>> Hi Stephen,
>>>
>>> Thanks for taking a look at the patch.
>>>
>>> On 12-Jan-23 01:14, Stephen Boyd wrote:
>>>> Quoting Aradhya Bhatia (2022-12-26 01:57:44)
>>>>> Add "ti,k3-am62-oldi-clk-div" to the fixed factor clock compatible enum
>>>>> list.
>>>>>
>>>>> "ti,k3-am62-oldi-clk-div" is a fixed-factor clock that helps the TI
>>>>> display subsystem request a pixel clock for itself and a corresponding
>>>>> serial clock for its OLDI Transmitters. The serial clock is 7 times the
>>>>> pixel clock. This clock needs the clock set rate request to be
>>>>> propagated to the parent clock provider.
>>>>>
>>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>>> ---
>>>>>   Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml | 1 +
>>>>>   1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git 
>>>>> a/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml 
>>>>> b/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
>>>>> index 8f71ab300470..0696237530f7 100644
>>>>> --- a/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
>>>>> +++ b/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
>>>>> @@ -14,6 +14,7 @@ properties:
>>>>>     compatible:
>>>>>       enum:
>>>>>         - fixed-factor-clock
>>>>> +      - ti,k3-am62-oldi-clk-div
>>>>
>>>> I don't see this compatible anywhere in the kernel tree. Is there a
>>>> patch that adds a node using this? I wonder why the display subsystem
>>>> can't add this fixed factor clk directly in the driver. Does the OLDI
>>>> Transmitter send a clk to the display subsystem?
>>>>
>>>> I'm asking all these questions because we got rid of vendor compatibles
>>>> here in hopes of simplifying the logic. Maybe the problem can be
>>>> approached differently, but I don't know all the details.
>>>
>>>
>>> +--------+                       +------------------+
>>> |        |                       |                  |
>>> |  PLL   +---+----+------------->| OLDI Transmitter |
>>> |        |   |    |              |                  |
>>> +--------+   |    |              +------------------+
>>>               |    |
>>>               |    |              +------------------+
>>>               |    |              |                  |
>>>               |    +------------->| OLDI Transmitter |
>>>               |                   |                  |
>>>               |                   +------------------+
>>>               |
>>>               |                   +------------------+
>>>               |   +----------+    |                  |
>>>               |   |    /7    |    |      Display     |
>>>               +-->|   Clock  +--->| Sub-System (DSS) |
>>>                   |    Div   |    |                  |
>>>                   +----------+    +------------------+
>>>
>>> This is how the the clock architecture for DSS looks like.
>>>
>>> The clock divider is not a part of DSS, but outside it.
> 
> The divider is fixed as well? And presumably inside the SoC?
Yes, and yes!

> 
>>>
>>> The clock request flow is initiated by the DSS driver because it has the
>>> required timing parameter information. It requests a certain pixel
>>> frequency. But the frequency required by the OLDI TXes is 7 times
>>> that pixel frequency.
>>>
>>> (Just for clarification, in some cases, the OLDI TX does require only
>>> 3.5 times the pixel frequency, but in those situations there is another
>>> divider in-front of OLDI TX that gets activated with a signal and
>>> divides the incoming frequency by 2, thereby requiring the PLL to still
>>> generate a 7x frequency.)
>>>
>>> Hence, the idea is that the clock divider is able to propagate the set
>>> rate request back to PLL, asking for a frequency 7 times more than the
>>> DSS's asking rate.
> 
> Got it. Can the PLL driver provide a pll_div_7 clk that is used for the
> DSS pixel clk?
> 
The PLL driver can not map the clock divider and hence can't provide the
pll_div_7 clock directly to DSS.

>>>
>>> If this is something less than ideal and should not go up, then I can
>>> implement a new clock device with a separate but similar clock driver.
>>>
>>> Let me know what you think!
>>
>> As a clarification I would also add to the above that on other TI SoCs 
>> with DSS, and also for the second video port on AM62, the clock 
>> framework provides DSS a clock using the pclk frequency.
>>
> 
> Are you saying that adding a fixed div-7 clk in the DSS driver is wrong?
Yes. All variants of DSS accept a pixel clock and it would be wrong to
implement a fixed div-7 in the DSS driver.

All that said, I now understand that the new compatible shouldn't go
there. I will implement a new driver and post it. =)


Regards
Aradhya

Re: [PATCH 1/2] dt-bindings: clock: fixed-factor: Add TI AM62 SoC OLDI clock

[Stephen Boyd]

Quoting Aradhya Bhatia (2023-02-05 21:34:53)
> Hi Stephen,
> 
> On 26-Jan-23 05:36, Stephen Boyd wrote:
> > Quoting Tomi Valkeinen (2023-01-17 01:40:24)
> >> On 16/01/2023 11:51, Aradhya Bhatia wrote:
> >>> Hi Stephen,
> >>>
> >>> Thanks for taking a look at the patch.
> >>>
> >>> On 12-Jan-23 01:14, Stephen Boyd wrote:
> >>>> Quoting Aradhya Bhatia (2022-12-26 01:57:44)
> >>>>> Add "ti,k3-am62-oldi-clk-div" to the fixed factor clock compatible enum
> >>>>> list.
> >>>>>
> >>>>> "ti,k3-am62-oldi-clk-div" is a fixed-factor clock that helps the TI
> >>>>> display subsystem request a pixel clock for itself and a corresponding
> >>>>> serial clock for its OLDI Transmitters. The serial clock is 7 times the
> >>>>> pixel clock. This clock needs the clock set rate request to be
> >>>>> propagated to the parent clock provider.
> >>>>>
> >>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> >>>>> ---
> >>>>>   Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml | 1 +
> >>>>>   1 file changed, 1 insertion(+)
> >>>>>
> >>>>> diff --git 
> >>>>> a/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml 
> >>>>> b/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
> >>>>> index 8f71ab300470..0696237530f7 100644
> >>>>> --- a/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/clock/fixed-factor-clock.yaml
> >>>>> @@ -14,6 +14,7 @@ properties:
> >>>>>     compatible:
> >>>>>       enum:
> >>>>>         - fixed-factor-clock
> >>>>> +      - ti,k3-am62-oldi-clk-div
> >>>>
> >>>> I don't see this compatible anywhere in the kernel tree. Is there a
> >>>> patch that adds a node using this? I wonder why the display subsystem
> >>>> can't add this fixed factor clk directly in the driver. Does the OLDI
> >>>> Transmitter send a clk to the display subsystem?
> >>>>
> >>>> I'm asking all these questions because we got rid of vendor compatibles
> >>>> here in hopes of simplifying the logic. Maybe the problem can be
> >>>> approached differently, but I don't know all the details.
> >>>
> >>>
> >>> +--------+                       +------------------+
> >>> |        |                       |                  |
> >>> |  PLL   +---+----+------------->| OLDI Transmitter |
> >>> |        |   |    |              |                  |
> >>> +--------+   |    |              +------------------+
> >>>               |    |
> >>>               |    |              +------------------+
> >>>               |    |              |                  |
> >>>               |    +------------->| OLDI Transmitter |
> >>>               |                   |                  |
> >>>               |                   +------------------+
> >>>               |
> >>>               |                   +------------------+
> >>>               |   +----------+    |                  |
> >>>               |   |    /7    |    |      Display     |
> >>>               +-->|   Clock  +--->| Sub-System (DSS) |
> >>>                   |    Div   |    |                  |
> >>>                   +----------+    +------------------+
> >>>
> >>> This is how the the clock architecture for DSS looks like.
> >>>
> >>> The clock divider is not a part of DSS, but outside it.
> > 
> > The divider is fixed as well? And presumably inside the SoC?
> Yes, and yes!

Ok.

> 
> > 
> >>>
> >>> The clock request flow is initiated by the DSS driver because it has the
> >>> required timing parameter information. It requests a certain pixel
> >>> frequency. But the frequency required by the OLDI TXes is 7 times
> >>> that pixel frequency.
> >>>
> >>> (Just for clarification, in some cases, the OLDI TX does require only
> >>> 3.5 times the pixel frequency, but in those situations there is another
> >>> divider in-front of OLDI TX that gets activated with a signal and
> >>> divides the incoming frequency by 2, thereby requiring the PLL to still
> >>> generate a 7x frequency.)
> >>>
> >>> Hence, the idea is that the clock divider is able to propagate the set
> >>> rate request back to PLL, asking for a frequency 7 times more than the
> >>> DSS's asking rate.
> > 
> > Got it. Can the PLL driver provide a pll_div_7 clk that is used for the
> > DSS pixel clk?
> > 
> The PLL driver can not map the clock divider and hence can't provide the
> pll_div_7 clock directly to DSS.

Isn't the divider fixed? So what is there to map?

> 
> >>>
> >>> If this is something less than ideal and should not go up, then I can
> >>> implement a new clock device with a separate but similar clock driver.
> >>>
> >>> Let me know what you think!
> >>
> >> As a clarification I would also add to the above that on other TI SoCs 
> >> with DSS, and also for the second video port on AM62, the clock 
> >> framework provides DSS a clock using the pclk frequency.
> >>
> > 
> > Are you saying that adding a fixed div-7 clk in the DSS driver is wrong?
> Yes. All variants of DSS accept a pixel clock and it would be wrong to
> implement a fixed div-7 in the DSS driver.

The reason being that it has an input of a pixel clk divided by 7?
That's why I suggested having the PLL provide a clk output that is
divided by 7.

> 
> All that said, I now understand that the new compatible shouldn't go
> there. I will implement a new driver and post it. =)
> 

The block diagram above shows the fixed divider living outside of the
PLL or display subsystem or OLDI transmitter(s). It could just as well
be drawn where the PLL has two outputs, one divided by 7 and sent as the
pixel clk and the other not divided by 7. For all I know the PLL and the
fixed divider are shipped by the same hardware engineer, so it looks
totally arbitrary.

Isn't it easier to _not_ create a struct device, _not_ create a
different device node, and simply make the PLL have another output clk,
so that we don't need to implement this as a new compatible string and
update the basic clk type code? To clarify, I don't understand why this
must be implemented through devicetree. We already have a driver for the
PLL and that knows what SoC it is, so why can't we add the fixed divider
there?