Question regarding clocks in the DW-HDMI DT bindings

[Laurent Pinchart]

Hi Mike,

On Monday 28 Nov 2016 22:27:01 Michael Turquette wrote:
> On Mon, Nov 28, 2016 at 10:04 PM, Laurent Pinchart wrote:
> > 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.
> 
> I thought I answered this, but maybe not directly enough :-)
> 
> We should make the clock mandatory in DT if the physical line must be
> there. This is regardless of whether a given chip/IP variant has
> control over that clock; so long as the physical clock line always
> exists then it is not really "optional".
> 
> In the case where there is an absence of the physical clock line, then
> making it optional in DT makes sense.
> 
> As an aside, we did discuss the fact that the vast majority of clocks
> are not modeled in DT, and I'm not saying that we transcribe the RTL
> into DT. I'm just saying that if there is a debate over whether or not
> to make a clock optional in DT, when it is always physically there,
> then don't make it optional. Whether or not the control is exposed on
> a particular chip is less important.
> 
> Anyways, this is more DT ridiculousness and I won't block either
> method getting merged. I'm just picking my favorite color to paint the
> bikeshed.

Thanks for sharing your opinion. I'll keep the clock mandatory and specify it 
in DT.

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