Re: [PATCH v2 10/10] dt-bindings: Add DSIv2 documentation

[Archit Taneja <architt@codeaurora.org>]

On 12/03/2015 12:46 PM, Stephen Boyd wrote:
> On 12/02, Archit Taneja wrote:
>> On 12/02/2015 01:50 PM, Stephen Boyd wrote:
>>>
>>> My only thought there would be to make of_clk_set_defaults() wait
>>> until both clocks are registered before it does any parent
>>> setting. But only in the case where the assigned parents contains
>>> a clock that is provided by the node being processed. I suppose
>>> the simplest thing to do would be to skip it during the device
>>> driver probe and handle it when the clk provider is registered.
>>>
>>
>> The assigned-clock-parents stuff you mentioned is needed to set a
>> default link between the one of the DSI PLLs and the RCG, right? I
>> just wanted to make clear if we were still discussing the same
>> issue.
>
> Yes.
>
>>
>>  From what I understand, we don't need the assigned-clock-parents stuff
>> to establish a link between byte_clk_src(RCG clock) and
>> byte_clk(branch clock). That's a fixed link set up by the clock
>> structs provided in the gcc driver and doesn't need to be specially
>> assigned, and just a
>> clk_get_parent in the driver does the job there.
>
> There's only one parent of the byte_clk and that's byte_clk_src.
> So yes, there's no need to describe that in DT and
> clk_get_parent() works fine.
>
>>
>> About assigning a parent to the RCG, wouldn't that be xo by default, and
>> changed by the drm/msm driver to one of the PLLs when the need arrives?
>> I didn't get why we need to establish that beforehand in DT?
>>
>
> Yes, it would be XO out of reset, but we have no idea what the
> bootloader is doing. I thought the problem was that byte_clk_src
> is not actually an input to the DSI device. The proposal was to
> have DT specify byte_clk_src and byte_clk in the clocks array so
> that byte_clk_src could be reparented to the PLL and the byte_clk
> could be enabled/disabled. If we use DT to do the parent
> configuring then the DSI node doesn't have the byte_clk_src in
> its clocks array and thus DT is reflecting reality.

Okay, I understand your point now.

>
> If we want to dynamically switch the parent of byte_clk_src to be
> different PLLs at runtime, then yes we'll need to get the parent
> of the byte_clk (which is byte_clk_src) and set the PLL as the
> parent. Or we'll need to make clk_set_parent() on the byte_clk
> transparently set the grand-parent to be the PLL. In that case we
> may need to introduce some sort of flag like
> CLK_SET_PARENT_GRANDPARENT to add this type of behavior.
>
> I don't have a huge problem with
>
> 	clk_set_parent(clk_get_parent(byte_clk), PLL)
>
> except that this fails the abstraction test. It leaks information
> about the clock tree into a driver that shouldn't need to know
> that on this particular SoC there's a clock in between the PLL
> and the byte_clk. Future designs may not have the intermediate
> clock and then we'll need to update the driver to handle the
> difference, when we could have added the flag and things would
> work the same.


I will have to check if we really require dynamic PLL configuration
or not. At the moment, the driver reads another DT param (that
identifies if we are in dual dsi mode) and then sets the appropriate
PLL as parent. So, in a sense, it is relying on DT already for setting
the parent.

I guess we can do the following:

- use assigned-clock-parents in DT to set the default PLL
parent (provided it works out of the box)

- Within the driver, still do the
'clk_set_parent(clk_get_parent(byte_clk), PLL)' thing and
remove it later if dynamic switching isn't needed at all
if the parent PLL is known beforehand in all use cases.

Archit

>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel