Re: [PATCH v2 13/13] dt-bindings: msm/hdmi: Add HDMI PHY bindings

[Archit Taneja <architt@codeaurora.org>]

On 02/23/2016 12:57 AM, Rob Herring wrote:
> On Mon, Feb 22, 2016 at 5:07 AM, Archit Taneja <architt@codeaurora.org> wrote:
>>
>>
>> On 02/22/2016 08:24 AM, Rob Herring wrote:
>>>
>>> On Mon, Feb 15, 2016 at 12:23:26PM +0530, Archit Taneja wrote:
>>>>
>>>> Add HDMI PHY bindings. Update the example to use HDMI PHY.
>>>>
>>>> Add a missing power-domains property in the HDMI core bindings.
>>>>
>>>> Cc: devicetree@vger.kernel.org
>>>> Cc: Rob Herring <robh@kernel.org>
>>>>
>>>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>>>> ---
>>>>    .../devicetree/bindings/display/msm/hdmi.txt       | 39
>>>> +++++++++++++++++++++-
>>>>    1 file changed, 38 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/msm/hdmi.txt
>>>> b/Documentation/devicetree/bindings/display/msm/hdmi.txt
>>>> index 379ee2e..4d71910 100644
>>>> --- a/Documentation/devicetree/bindings/display/msm/hdmi.txt
>>>> +++ b/Documentation/devicetree/bindings/display/msm/hdmi.txt
>>>> @@ -11,6 +11,7 @@ Required properties:
>>>>    - reg: Physical base address and length of the controller's registers
>>>>    - reg-names: "core_physical"
>>>>    - interrupts: The interrupt signal from the hdmi block.
>>>> +- power-domains: Should be <&mmcc MDSS_GDSC>.
>>>>    - clocks: device clocks
>>>>      See ../clocks/clock-bindings.txt for details.
>>>>    - qcom,hdmi-tx-ddc-clk-gpio: ddc clk pin
>>>> @@ -18,6 +19,7 @@ Required properties:
>>>>    - qcom,hdmi-tx-hpd-gpio: hpd pin
>>>>    - core-vdda-supply: phandle to supply regulator
>>>>    - hdmi-mux-supply: phandle to mux regulator
>>>> +- qcom,hdmi-phy: phandle to HDMI PHY device node
>>>
>>>
>>> Why not use the generic phy binding?
>>
>>
>> You'd asked about this in the first version of this patch. You
>> probably missed reading my reply. Partially my fault since I
>> missed out putting the "In-Reply-to" when posting this set. I've
>> mentioned the reason again here:
>>
>> The PHY in the HDMI and DSI blocks can't be implemented using the
>> common phy framework. The PHY blocks have a PLL sub-block within
>> them which acts as a pixel clock source for the display processor
>> block.
>
> That sounds like a problem with the common phy framework, not the DT
> binding. Nothing says you have to use the common phy f/w if you use
> the phy binding. I say that as the binding maintainer. As a kernel
> developer, I would say fix the common phy framework to handle this
> case. However, I don't think anything prevents the phy driver from
> registering both a phy and a clock.

Okay. For some reason, I thought it would be wrong to use the same
common phy bindings but use our own phy driver.

I will convert the bindings to the generic phy binding.

>
>> This dependency causes the need to split the phy power on sequence
>> into 2 parts (one to enable resources to enable the PLL, and the
>> other to enable the phy itself), which the phy framework can't
>> do. That's the main reason not to use it. There are some more
>> complex use cases for DSI PHY (drive two PHYs with the same
>> DSI PLL) which the phy framework can't support.
>
> Doesn't the phy framework already support the former? It has power on
> and init calls. Personally, I find the split there ill-defined.

I always assumed that the init/exit ops were something that you would
call just once (during probe/remove) and then forget about it. I only
now noticed that init and power_on are often paired together (as are
power_off and exit). I went through the common phy framework code in
more detail, but I realized I would face the following issue:

I was looking for the sequence:

1. enable PHY resources (enables clocks/regulators/pm_runtime_get_sync)

2. set_rate/enable the PLL clock provided by PHY

3. enable PHY (configure some PHY registers)

I can achieve this using the common phy framework if I tie step 1)
to phy_power_on(), and step 3) to phy_init(). The other way round won't
work because phy_init() seems to disable runtime pm as it exits.

If I use the phy framework in its current state, I'll end up stuffing
the phy ops and use them in a weird way just to make sure my PHY
works.

Copying Kishon if he has any opinions.


>
>>>>    Optional properties:
>>>>    - qcom,hdmi-tx-mux-en-gpio: hdmi mux enable pin
>>>> @@ -27,6 +29,27 @@ Optional properties:
>>>>    - pinctrl-0: the default pinctrl state (active)
>>>>    - pinctrl-1: the "sleep" pinctrl state
>>>>
>>>> +HDMI PHY:
>>>> +Required properties:
>>>> +- compatible: Could be the following
>>>> +  * "qcom,hdmi-phy-8x60"
>>>> +  * "qcom,hdmi-phy-8960"
>>>> +  * "qcom,hdmi-phy-8x74"
>>>
>>>
>>> No wildcards please. Where's 8994?
>>
>>
>> I'll remove the wildcards. 8994 PHY isn't supported by the driver at
>> the moment. I could keep the 8994 compatible string, the driver will
>> bail out with an error. But that's something we already do for 8x74
>> since it doesn't have full PHY support either.
>
> You can document it later if you want. I just asked 'cause I knew the
> 8994 had one.

Okay.

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
https://lists.freedesktop.org/mailman/listinfo/dri-devel