Question regarding clocks in the DW-HDMI DT bindings

[Michael Turquette]

Hi Laurent, all,

On Fri, Nov 25, 2016 at 7:22 AM, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
> Hi Andy,
>
> On Friday 25 Nov 2016 10:56:53 Andy Yan wrote:
>> On 2016年11月25日 07:26, Laurent Pinchart wrote:
>> > On Friday 25 Nov 2016 00:16:00 Vladimir Zapolskiy wrote:
>> >> On 11/25/2016 12:07 AM, Fabio Estevam wrote:
>> >>> On Thu, Nov 24, 2016 at 7:16 PM, Laurent Pinchart wrote:
>> >>>> Hi Andy,
>> >>>>
>> >>>> As the author of the DW-HDMI DT bindings this question is addressed to
>> >>>> you, but information from anyone is more than welcome.
>> >>>>
>> >>>> The DT bindings specify two clocks named "iahb" and "isfr" but don't
>> >>>> describe them. While I assume that the "isfr" clock corresponds to the
>> >>>> "isfrclk" input signal of the DW HDMI, there is no "iahb" clock
>> >>>> described in the IP core datasheet.
>> >>>
>> >>> i.MX6Q has a DW-HDMI IP block.
>> >>>
>> >>> The names in the devicetree binding matches the ones listed at the
>> >>> i.MX6Q Reference Manual - Table 33-1. HDMI Clocks
>> >>
>> >> correct, for your convenience the table is copied below:
>> >>
>> >> Clock name |     Clock Root     | Description
>> >> -----------+--------------------+---------------------------------------
>> >>   iahbclk  | ahb_clk_root       | Bus clock
>> >>   icecclk  | ckil_sync_clk_root | CEC low-frequency clock (32kHZ)
>> >>   ihclk    | ahb_clk_root       | Module clock
>> >>   isfrclk  | video_27m_clk_root | Internal SFR clock (video clock
>> >>   27MHz)
>> >>
>> >> Here AHB stands for ARM Advanced High-performance Bus.
>> >
>> > That's what I suspected. I believe the "iahb" name is wrong, as the DW
>> > HDMI TX IP core clearly documents the bus clock as being called
>> > "iapbclk". We could rename that in the DT bindings (with compatibility
>> > code in the driver to keep supporting the old name) but it might not be
>> > worth it. The bindings should however document that the "iahb" clock is
>> > the IP core's "iapbclk" bus clock.
>>
>> I got the clock name from I.MX6Q TRM, I also checked the name again
>> with Rockchip IC design team now, hope to get some new information soon.
>
> Thank you. While at it, could you ask them which version of the DW HDMI IP
> used in the SoC ?
>
>> > Another question I have about the bus clock (CC'ing the devicetree mailing
>> > list as well as the clock maintainers) is whether it should be made
>> > optional. The clock is obviously mandatory from a hardware point of view
>> > (given that APB is a synchronous bus and thus requires a clock), but in
>> > some SoCs (specifically for the Renesas SoCs) that clock is always on and
>> > can't be controlled. We already omit bus clocks in DT for most IP cores
>> > when the clock can never be controlled (and we also omit a bunch of other
>> > clocks that we don't even know exist), so it could make sense to make the
>> > clock optional. Otherwise there would be runtime overhead trying to handle
>> > a clock that can't be controlled.
>>
>> If this is the case on Renesas SOCs, we can consider make the clock as
>> optional. Or move all the clock operations to platform specific
>> code(dw_hdmi-rockchip.c/dw_hdmi-imx.c)?
>
> I'd prefer keeping the code generic, otherwise we'd end up with platform-
> specific code that would perform the same operations on most platforms. I'll
> submit a patch soon to make the clock optional, we can discuss it then.

Yes, let's keep the code generic. Absence of a "standard' clock is OK
and we should accept the small overhead incurred in providing a
solution that works for everyone. This prevents hardware-specific
hacks in the driver.

Related: we really should model bus clocks whenever possible. I've
seen other attempts to merge functional/logic and bus clocks into a
single entity (e.g. a single struct clk_hw/clk_core that turns both
clocks on and off) and this defeats some fine-grained power management
scenarios that the hardware designers had in mind when creating
separate controls for the clocks.

Regards,
Mike

>
>> >> By the way while we're discussing DW HDMI bindings specific to iMX,
>> >> I would recommend to remove utterly hackish and iMX only "gpr"
>> >> property from the example in bindings/display/bridge/dw_hdmi.txt
>
> --
> Regards,
>
> Laurent Pinchart
>



-- 
Michael Turquette
CEO
BayLibre - At the Heart of Embedded Linux
http://baylibre.com/