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

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

[Rob Herring]

+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.

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

>    * "core_clk"
>    * "core_mmss_clk"
>    * "iface_clk"
>    * "mdp_core_clk"
>    * "pixel_clk"
> +  * "pixel_clk_src"
> +  For DSIv2, we need a few more:

What is the overall order of clocks? As listed?

> +   * "dsi_clk_src"
> +   * "esc_clk_src"
> +   * "src_clk"
>  - vdd-supply: phandle to vdd regulator device node
>  - vddio-supply: phandle to vdd-io regulator device node
>  - vdda-supply: phandle to vdda regulator device node
>  - qcom,dsi-phy: phandle to DSI PHY device node
> +- syscon-sfpb: A phandle to mmss_sfpb syscon node (only for DSIv2)
>
>  Optional properties:
>  - panel@0: Node of panel connected to this DSI controller.
> @@ -51,6 +58,7 @@ Required properties:
>    * "qcom,dsi-phy-28nm-hpm"
>    * "qcom,dsi-phy-28nm-lp"
>    * "qcom,dsi-phy-20nm"
> +  * "qcom,dsi-phy-28nm-8960"
>  - reg: Physical base address and length of the registers of PLL, PHY and PHY
>    regulator
>  - reg-names: The names of register regions. The following regions are required:
> --
> 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

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

[Archit Taneja]

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.

>
>>     * "core_clk"
>>     * "core_mmss_clk"
>>     * "iface_clk"
>>     * "mdp_core_clk"
>>     * "pixel_clk"
>> +  * "pixel_clk_src"
>> +  For DSIv2, we need a few more:
>
> What is the overall order of clocks? As listed?

Order in which the driver does clk_get? It uses the clock
name to get each one individually, so the order doesn't matter
as such.

I don't think it the order of clk_get in the driver is the same
as what's been listed here. I can fix it if that's the norm.

Thanks,
Archit

>
>> +   * "dsi_clk_src"
>> +   * "esc_clk_src"
>> +   * "src_clk"
>>   - vdd-supply: phandle to vdd regulator device node
>>   - vddio-supply: phandle to vdd-io regulator device node
>>   - vdda-supply: phandle to vdda regulator device node
>>   - qcom,dsi-phy: phandle to DSI PHY device node
>> +- syscon-sfpb: A phandle to mmss_sfpb syscon node (only for DSIv2)
>>
>>   Optional properties:
>>   - panel@0: Node of panel connected to this DSI controller.
>> @@ -51,6 +58,7 @@ Required properties:
>>     * "qcom,dsi-phy-28nm-hpm"
>>     * "qcom,dsi-phy-28nm-lp"
>>     * "qcom,dsi-phy-20nm"
>> +  * "qcom,dsi-phy-28nm-8960"
>>   - reg: Physical base address and length of the registers of PLL, PHY and PHY
>>     regulator
>>   - reg-names: The names of register regions. The following regions are required:
>> --
>> 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

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

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

[Rob Herring]

+Stephen

On Wed, Nov 18, 2015 at 9:24 AM, Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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.

>>>     * "core_clk"
>>>     * "core_mmss_clk"
>>>     * "iface_clk"
>>>     * "mdp_core_clk"
>>>     * "pixel_clk"
>>> +  * "pixel_clk_src"
>>> +  For DSIv2, we need a few more:
>>
>>
>> What is the overall order of clocks? As listed?
>
>
> Order in which the driver does clk_get? It uses the clock
> name to get each one individually, so the order doesn't matter
> as such.

The order in DT. You may use the names, but the order should still be specified.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Archit Taneja]

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?

>
>>>>      * "core_clk"
>>>>      * "core_mmss_clk"
>>>>      * "iface_clk"
>>>>      * "mdp_core_clk"
>>>>      * "pixel_clk"
>>>> +  * "pixel_clk_src"
>>>> +  For DSIv2, we need a few more:
>>>
>>>
>>> What is the overall order of clocks? As listed?
>>
>>
>> Order in which the driver does clk_get? It uses the clock
>> name to get each one individually, so the order doesn't matter
>> as such.
>
> The order in DT. You may use the names, but the order should still be specified.

Okay. I'll cross check and make sure the order is as in our DT files.

Archit

>
> Rob
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[PATCH v3 11/12] dt-bindings: msm/dsi: Fix the order in which clocks are listed

[Archit Taneja]

List the clocks in the order that's used in DT. We don't have mdp/dsi
DT nodes for any SoC in upstream yet, but we align with the order
we intend to use.

Cc: devicetree@vger.kernel.org
Cc: Rob Herring <robh@kernel.org>

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 Documentation/devicetree/bindings/display/msm/dsi.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
index f344b9e..e097955 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi.txt
+++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
@@ -14,13 +14,13 @@ Required properties:
 - clocks: device clocks
   See Documentation/devicetree/bindings/clocks/clock-bindings.txt for details.
 - clock-names: the following clocks are required:
+  * "mdp_core_clk"
+  * "iface_clk"
   * "bus_clk"
-  * "byte_clk"
-  * "core_clk"
   * "core_mmss_clk"
-  * "iface_clk"
-  * "mdp_core_clk"
+  * "byte_clk"
   * "pixel_clk"
+  * "core_clk"
 - vdd-supply: phandle to vdd regulator device node
 - vddio-supply: phandle to vdd-io regulator device node
 - vdda-supply: phandle to vdda regulator device node
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v3 12/12] dt-bindings: msm/dsi: Add DSIv2 documentation

[Archit Taneja]

Add additional property info needed for DSIv2 DT.

Cc: devicetree@vger.kernel.org
Cc: Rob Herring <robh@kernel.org>

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 Documentation/devicetree/bindings/display/msm/dsi.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
index e097955..e7423be 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi.txt
+++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
@@ -21,10 +21,13 @@ Required properties:
   * "byte_clk"
   * "pixel_clk"
   * "core_clk"
+  For DSIv2, we need an additional clock:
+   * "src_clk"
 - vdd-supply: phandle to vdd regulator device node
 - vddio-supply: phandle to vdd-io regulator device node
 - vdda-supply: phandle to vdda regulator device node
 - qcom,dsi-phy: phandle to DSI PHY device node
+- syscon-sfpb: A phandle to mmss_sfpb syscon node (only for DSIv2)
 
 Optional properties:
 - panel@0: Node of panel connected to this DSI controller.
@@ -51,6 +54,7 @@ Required properties:
   * "qcom,dsi-phy-28nm-hpm"
   * "qcom,dsi-phy-28nm-lp"
   * "qcom,dsi-phy-20nm"
+  * "qcom,dsi-phy-28nm-8960"
 - reg: Physical base address and length of the registers of PLL, PHY and PHY
   regulator
 - reg-names: The names of register regions. The following regions are required:
-- 
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

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

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Stephen Boyd]

On 12/02, 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.
> 

Actually it looks like we already have the code for that.

	if (clkspec.np == node && !clk_supplier)
		return 0;

So assigned parents should "just work"?

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

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

[Archit Taneja]

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

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

[Stephen Boyd]

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.

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Archit Taneja]

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

Re: [PATCH v3 11/12] dt-bindings: msm/dsi: Fix the order in which clocks are listed

[Rob Herring]

On Tue, Dec 01, 2015 at 03:30:09PM +0530, Archit Taneja wrote:
> List the clocks in the order that's used in DT. We don't have mdp/dsi
> DT nodes for any SoC in upstream yet, but we align with the order
> we intend to use.
> 
> Cc: devicetree@vger.kernel.org
> Cc: Rob Herring <robh@kernel.org>
> 
> Signed-off-by: Archit Taneja <architt@codeaurora.org>

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

> ---
>  Documentation/devicetree/bindings/display/msm/dsi.txt | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
> index f344b9e..e097955 100644
> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> @@ -14,13 +14,13 @@ Required properties:
>  - clocks: device clocks
>    See Documentation/devicetree/bindings/clocks/clock-bindings.txt for details.
>  - clock-names: the following clocks are required:
> +  * "mdp_core_clk"
> +  * "iface_clk"
>    * "bus_clk"
> -  * "byte_clk"
> -  * "core_clk"
>    * "core_mmss_clk"
> -  * "iface_clk"
> -  * "mdp_core_clk"
> +  * "byte_clk"
>    * "pixel_clk"
> +  * "core_clk"
>  - vdd-supply: phandle to vdd regulator device node
>  - vddio-supply: phandle to vdd-io regulator device node
>  - vdda-supply: phandle to vdda regulator device node
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 

Re: [PATCH v3 12/12] dt-bindings: msm/dsi: Add DSIv2 documentation

[Rob Herring]

On Tue, Dec 01, 2015 at 03:30:10PM +0530, Archit Taneja wrote:
> Add additional property info needed for DSIv2 DT.
> 
> Cc: devicetree@vger.kernel.org
> Cc: Rob Herring <robh@kernel.org>
> 
> Signed-off-by: Archit Taneja <architt@codeaurora.org>

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

> ---
>  Documentation/devicetree/bindings/display/msm/dsi.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
> index e097955..e7423be 100644
> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> @@ -21,10 +21,13 @@ Required properties:
>    * "byte_clk"
>    * "pixel_clk"
>    * "core_clk"
> +  For DSIv2, we need an additional clock:
> +   * "src_clk"
>  - vdd-supply: phandle to vdd regulator device node
>  - vddio-supply: phandle to vdd-io regulator device node
>  - vdda-supply: phandle to vdda regulator device node
>  - qcom,dsi-phy: phandle to DSI PHY device node
> +- syscon-sfpb: A phandle to mmss_sfpb syscon node (only for DSIv2)
>  
>  Optional properties:
>  - panel@0: Node of panel connected to this DSI controller.
> @@ -51,6 +54,7 @@ Required properties:
>    * "qcom,dsi-phy-28nm-hpm"
>    * "qcom,dsi-phy-28nm-lp"
>    * "qcom,dsi-phy-20nm"
> +  * "qcom,dsi-phy-28nm-8960"
>  - reg: Physical base address and length of the registers of PLL, PHY and PHY
>    regulator
>  - reg-names: The names of register regions. The following regions are required:
> -- 
> 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

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

[Archit Taneja]

On 12/02/2015 02:04 PM, Stephen Boyd wrote:
> On 12/02, 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.
>>
>
> Actually it looks like we already have the code for that.
>
> 	if (clkspec.np == node && !clk_supplier)
> 		return 0;
>
> So assigned parents should "just work"?

I tried using assigned-parents and it works fine.

The issue you mentioned above doesn't apply in our case, because
we have two different devices for "dsi" and "dsi_phy". dsi_phy is the
clock provider here and dsi is the one that wants to assign clocks.

If there was only one dsi device representing both DSI and PHY, then
we'd hit the condition you mentioned.

Thanks,
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