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 at 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 at 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 at 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