Question regarding clocks in the DW-HDMI DT bindings

[Laurent Pinchart]

Hi Mike,

On Monday 28 Nov 2016 13:56:11 Michael Turquette wrote:
> On Fri, Nov 25, 2016 at 7:22 AM, Laurent Pinchart wrote:
> > 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.

Sure, but that wasn't really the question :-) When the bus clock is separately 
controllable then I agree it should be modelled separately in DT. In my case 
the bus clock is always on, and I'm thus wondering whether it would be better 
to make it optional in DT to reduce the runtime overhead incurred by trying to 
control something that can't be controlled.

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