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

[Archit Taneja <architt@codeaurora.org>]

On 12/02/2015 01:50 PM, Stephen Boyd wrote:
> On 11/23, Archit Taneja wrote:
>>
>>
>> On 11/21/2015 1:29 AM, Rob Herring wrote:
>>> +Stephen
>>>
>>> On Wed, Nov 18, 2015 at 9:24 AM, Archit Taneja <architt@codeaurora.org> wrote:
>>>> Hi Rob,
>>>>
>>>> On 11/18/2015 6:48 PM, Rob Herring wrote:
>>>>>
>>>>> +dt list
>>>>>
>>>>> On Wed, Nov 18, 2015 at 4:55 AM, Archit Taneja <architt@codeaurora.org>
>>>>> wrote:
>>>>>>
>>>>>> Add additional property info needed for DSIv2 DT.
>>>>>
>>>>>
>>>>> Please use get_maintainers.pl.
>>>>
>>>>
>>>> Sorry about that, missed out doing that posting this time.
>>>>
>>>>>
>>>>>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>>>>>> ---
>>>>>>    Documentation/devicetree/bindings/display/msm/dsi.txt | 10 +++++++++-
>>>>>>    1 file changed, 9 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt
>>>>>> b/Documentation/devicetree/bindings/display/msm/dsi.txt
>>>>>> index f344b9e..ca65a34 100644
>>>>>> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
>>>>>> +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
>>>>>> @@ -13,18 +13,25 @@ Required properties:
>>>>>>    - power-domains: Should be <&mmcc MDSS_GDSC>.
>>>>>>    - clocks: device clocks
>>>>>>      See Documentation/devicetree/bindings/clocks/clock-bindings.txt for
>>>>>> details.
>>>>>> -- clock-names: the following clocks are required:
>>>>>> +- clock-names: these vary based on the DSI version. For DSI6G:
>>>>>>      * "bus_clk"
>>>>>>      * "byte_clk"
>>>>>> +  * "byte_clk_src
>>>>>
>>>>>
>>>>> This sounds like the parent of byte_clk. Is that really a clock within
>>>>> the block?
>>>>
>>>>
>>>> byte_clk_src isn't in the block, but byte_clk_src's parent is one of
>>>> the PLLs in this block. We take this clock so that we can re-parent
>>>> it to an appropriate PLL. The decision of what PLL to choose needs to
>>>> be done by the DSI block's driver.
>>>
>>> Seems like abuse to me. The list of clocks should match what are
>>> inputs to the block, not what the driver happens to need. Without a
>>> full understanding of the clock tree here, I don't have a suggestion.
>>> Maybe Stephen does.
>>
>> We don't need specify byte_clk_src (and other xyz_clk_src clocks) via
>> DT. There is a static link set up between byte_clk and byte_clk_src by
>> our clock driver that never changes. We can retrieve it in the driver
>> itself using clk_get_parent(byte_clk). This way we stick to only
>> input clocks.
>>
>> Stephen, does that sound okay?
>>
>
> I guess so. From the DT perspective it's "correct" so sure.
>
> It would be nice if we could use assigned-clock-parents though.
> As far as I can recall that's hard because the clock tree looks
> like a cyclic graph when we take a clock provider level view. The
> display block consumes the byte_clk and provides the source of it
> too. So if we used assigned parents we would need to wait for
> both clocks to be registered with the framework before we can
> reconfigure the parent of byte_clk_src to be the PLL that the
> display clock outputs. Unfortunately, of_clk_set_defaults() is
> called during device driver probe, which in the display driver
> case would be before the PLL is registered.
>
> 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.

 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.

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?

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