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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
