Re: Question regarding clocks in the DW-HDMI DT bindings

Re: Question regarding clocks in the DW-HDMI DT bindings

[Laurent Pinchart]

Hello Fabio and Vladimir,

Thank you for your quick responses.

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.

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.

> 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Question regarding clocks in the DW-HDMI DT bindings

[Andy Yan]

Hi:


On 2016年11月25日 07:26, Laurent Pinchart wrote:
> Hello Fabio and Vladimir,
>
> Thank you for your quick responses.
>
> 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.
>
> 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)?

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


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Question regarding clocks in the DW-HDMI DT bindings

[Fabio Estevam]

Hi Laurent,

On Thu, Nov 24, 2016 at 9:26 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

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

What if you register the clock as a "dummy" clock instead?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Question regarding clocks in the DW-HDMI DT bindings

[Laurent Pinchart]

Hi Fabio,

On Friday 25 Nov 2016 10:43:04 Fabio Estevam wrote:
> On Thu, Nov 24, 2016 at 9:26 PM, Laurent Pinchart wrote:
> > 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.
> 
> What if you register the clock as a "dummy" clock instead?

In that case I can as well specify the correct clock. My point was that, for 
clocks that we know are always on, specifying them in DT will lead to runtime 
overhead (registration of the clock, lookup, runtime handling, ...) that we 
could as well avoid. I think the question has a broader scope than just the 
dw-hdmi driver, hence the widened audience in CC.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Question regarding clocks in the DW-HDMI DT bindings

[Laurent Pinchart]

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.

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Question regarding clocks in the DW-HDMI DT bindings

[Fabio Estevam]

On Fri, Nov 25, 2016 at 1:22 PM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:

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

DW HDMI IP used in Rockchip is:
dwhdmi-rockchip ff980000.hdmi: Detected HDMI controller 0x20:0xa:0xa0:0xc1

as shown at https://storage.kernelci.org/mainline/v4.9-rc6-157-g16ae16c6e561/arm-multi_v7_defconfig/lab-collabora/boot-rk3288-rock2-square_rootfs:nfs.html

DW HDMI IP used  on mx6q is:
dwhdmi-imx 120000.hdmi: Detected HDMI controller 0x13:0xa:0xa0:0xc1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Question regarding clocks in the DW-HDMI DT bindings

[Laurent Pinchart]

Hi Fabio,

On Friday 25 Nov 2016 13:29:37 Fabio Estevam wrote:
> On Fri, Nov 25, 2016 at 1:22 PM, Laurent Pinchart wrote:
> >> 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 ?
> 
> DW HDMI IP used in Rockchip is:
> dwhdmi-rockchip ff980000.hdmi: Detected HDMI controller 0x20:0xa:0xa0:0xc1
> 
> as shown at
> https://storage.kernelci.org/mainline/v4.9-rc6-157-g16ae16c6e561/arm-multi_
> v7_defconfig/lab-collabora/boot-rk3288-rock2-square_rootfs:nfs.html
> 
> DW HDMI IP used  on mx6q is:
> dwhdmi-imx 120000.hdmi: Detected HDMI controller 0x13:0xa:0xa0:0xc1

Thank you for the information. This is what I get on the Renesas R-Car H3.

rcar-dw-hdmi fead0000.hdmi0: Detected HDMI controller 0x20:0x1a:0xa0:0xc1
rcar-dw-hdmi feae0000.hdmi1: Detected HDMI controller 0x20:0x1a:0xa0:0xc1

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: 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@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/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Question regarding clocks in the DW-HDMI DT bindings

[Michael Turquette]

Hi Laurent,

[fixing Stephen's email address]

On Mon, Nov 28, 2016 at 10:04 PM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:
> 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.

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.

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

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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Question regarding clocks in the DW-HDMI DT bindings

[Laurent Pinchart]

Hi Fabio,

On Friday 25 Nov 2016 13:29:37 Fabio Estevam wrote:
> On Fri, Nov 25, 2016 at 1:22 PM, Laurent Pinchart wrote:
> >> 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 ?
> 
> DW HDMI IP used in Rockchip is:
> dwhdmi-rockchip ff980000.hdmi: Detected HDMI controller 0x20:0xa:0xa0:0xc1
> 
> as shown at
> https://storage.kernelci.org/mainline/v4.9-rc6-157-g16ae16c6e561/arm-multi_
> v7_defconfig/lab-collabora/boot-rk3288-rock2-square_rootfs:nfs.html
> 
> DW HDMI IP used  on mx6q is:
> dwhdmi-imx 120000.hdmi: Detected HDMI controller 0x13:0xa:0xa0:0xc1

Would you be able to print the value of the CONFIG[0-3]_ID registers as well ? 
I'm also interested in the same information for RK3288, as well as for IMX6DL.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Question regarding clocks in the DW-HDMI DT bindings

[Vladimir Zapolskiy]

Hi Laurent,

On 12/03/2016 07:16 PM, Laurent Pinchart wrote:
> Hi Fabio,
>
> On Friday 25 Nov 2016 13:29:37 Fabio Estevam wrote:
>> On Fri, Nov 25, 2016 at 1:22 PM, Laurent Pinchart wrote:
>>>> 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 ?
>>
>> DW HDMI IP used in Rockchip is:
>> dwhdmi-rockchip ff980000.hdmi: Detected HDMI controller 0x20:0xa:0xa0:0xc1
>>
>> as shown at
>> https://storage.kernelci.org/mainline/v4.9-rc6-157-g16ae16c6e561/arm-multi_
>> v7_defconfig/lab-collabora/boot-rk3288-rock2-square_rootfs:nfs.html
>>
>> DW HDMI IP used  on mx6q is:
>> dwhdmi-imx 120000.hdmi: Detected HDMI controller 0x13:0xa:0xa0:0xc1
>
> Would you be able to print the value of the CONFIG[0-3]_ID registers as well ?
> I'm also interested in the same information for RK3288, as well as for IMX6DL.
>

              i.MX6Q    i.MX6DL
DESIGN_ID     0x13      0x13
REVISION_ID   0x0a      0x1a    <--- the only difference
PRODUCT_ID0   0xa0      0xa0
PRODUCT_ID1   0xc1      0xc1
CONFIG0_ID    0x8f      0x8f
CONFIG1_ID    0x01      0x01
CONFIG2_ID    0xf2      0xf2    <--- HDMI 3D TX PHY
CONFIG3_ID    0x02      0x02

I'm not sure, if i.MX6D and MX6S have the same DW HDMI IP as on i.MX6Q
and i.MXDL respectively, and I don't have i.MX6DP or i.MX6QP powered board
on hand to dump the registers.

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Question regarding clocks in the DW-HDMI DT bindings

[Laurent Pinchart]

Hi Vladimir,

On Saturday 03 Dec 2016 23:10:35 Vladimir Zapolskiy wrote:
> On 12/03/2016 07:16 PM, Laurent Pinchart wrote:
> > On Friday 25 Nov 2016 13:29:37 Fabio Estevam wrote:
> >> On Fri, Nov 25, 2016 at 1:22 PM, Laurent Pinchart wrote:
> >>>> 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 ?
> >> 
> >> DW HDMI IP used in Rockchip is:
> >> dwhdmi-rockchip ff980000.hdmi: Detected HDMI controller
> >> 0x20:0xa:0xa0:0xc1
> >> 
> >> as shown at
> >> https://storage.kernelci.org/mainline/v4.9-rc6-157-g16ae16c6e561/
> >> arm-multi_v7_defconfig/lab-collabora/boot-rk3288-rock2-square_rootfs:
> >> nfs.html
> >> 
> >> DW HDMI IP used  on mx6q is:
> >> dwhdmi-imx 120000.hdmi: Detected HDMI controller 0x13:0xa:0xa0:0xc1
> > 
> > Would you be able to print the value of the CONFIG[0-3]_ID registers as
> > well ? I'm also interested in the same information for RK3288, as well as
> > for IMX6DL.
>               i.MX6Q    i.MX6DL
> DESIGN_ID     0x13      0x13
> REVISION_ID   0x0a      0x1a    <--- the only difference
> PRODUCT_ID0   0xa0      0xa0
> PRODUCT_ID1   0xc1      0xc1
> CONFIG0_ID    0x8f      0x8f
> CONFIG1_ID    0x01      0x01
> CONFIG2_ID    0xf2      0xf2    <--- HDMI 3D TX PHY
> CONFIG3_ID    0x02      0x02
> 
> I'm not sure, if i.MX6D and MX6S have the same DW HDMI IP as on i.MX6Q
> and i.MXDL respectively, and I don't have i.MX6DP or i.MX6QP powered board
> on hand to dump the registers.

Thank you for the information. Here are the HDMI TX versions I've found so 
far.

Allwinner H3/A64/A80		1.32a
Freescale i.MX6Q		1.30a
Freescale i.MX6DL		1.31a
Renesas R-Car H3		2.01a
Rockchip RK3288			2.00a

It would be useful to know what the other Freescale i.MX6 SoCs contain and 
whether they're subject to the HDMI errata worked around by the 
dw_hdmi_clear_overflow() function (ERR004308 "HDMI: 8000504668 — The 
arithmetic unit may get wrong video timing values although the FC_* registers 
hold correct values"). However, unless I'm mistaken only i.MX6DL and i.MX6Q 
have upstream support for HDMI output, so it might be difficult to find this 
out at the moment.

If we could establish that the problem isn't specific to Freescale but affects 
all 1.30a and 1.31a revisions, we could enable the workaround dynamically 
based on runtime identification. I've tested R-Car H3 without the workaround 
and haven't noticed the problem explained by Russell (magenta line on the left 
side of the image) or any other issue. Could someone test this on Rockchip 
RK3288 ?

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel