diff mbox series

[v8,09/24] drm/rockchip: dw_hdmi: Add support for niu clk

Message ID 20220311083323.887372-10-s.hauer@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series drm/rockchip: RK356x VOP2 support | expand

Commit Message

Sascha Hauer March 11, 2022, 8:33 a.m. UTC
The rk3568 HDMI has an additional clock that needs to be enabled for the
HDMI controller to work. This clock is not needed for the HDMI
controller itself, but to make the SoC internal bus logic work. From the
reference manual:

> 2.8.6 NIU Clock gating reliance
>
> A part of niu clocks have a dependence on another niu clock in order to
> sharing the internal bus. When these clocks are in use, another niu
> clock must be opened, and cannot be gated.  These clocks and the special
> clock on which they are relied are as following:
>
> Clocks which have dependency     The clock which can not be gated
> -----------------------------------------------------------------
> ...
> pclk_vo_niu, hclk_vo_s_niu       hclk_vo_niu
> ...

The clock framework does not support turning on a clock whenever another
clock is turned on, so this patch adds support for the dependent clock
to the HDMI driver. We call it "NIU", which is for "Native Interface
Unit"

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---

Notes:
    Changes since v7:
    - rename hclk to niu

 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Dmitry Osipenko March 12, 2022, 9:07 p.m. UTC | #1
On 3/11/22 11:33, Sascha Hauer wrote:
> The rk3568 HDMI has an additional clock that needs to be enabled for the
> HDMI controller to work. This clock is not needed for the HDMI
> controller itself, but to make the SoC internal bus logic work. From the
> reference manual:
> 
>> 2.8.6 NIU Clock gating reliance
>>
>> A part of niu clocks have a dependence on another niu clock in order to
>> sharing the internal bus. When these clocks are in use, another niu
>> clock must be opened, and cannot be gated.  These clocks and the special
>> clock on which they are relied are as following:
>>
>> Clocks which have dependency     The clock which can not be gated
>> -----------------------------------------------------------------
>> ...
>> pclk_vo_niu, hclk_vo_s_niu       hclk_vo_niu
>> ...
> The clock framework does not support turning on a clock whenever another
> clock is turned on, so this patch adds support for the dependent clock
> to the HDMI driver. We call it "NIU", which is for "Native Interface
> Unit"

This still doesn't make sense to me. You're saying that "pclk_vo_niu,
hclk_vo_s_niu" depend on "hclk_vo_niu", but HDMI doesn't use pclk_vo, it
uses pclk_hdmi. I don't see how these "niu" clocks are related to HDMI
from your description. The pclk_vo should remain being disabled for HDMI.

I already said before that the datasheet suggests that hclk_vo needs to
be enabled in order to ungate the AHB part of the VO module, which is
needed for CPU accesses. Elain, could you please confirm this or clarify
more about why hclk_vo needs to be enabled for HDMI?
Sascha Hauer March 14, 2022, 8:18 a.m. UTC | #2
On Sun, Mar 13, 2022 at 12:07:56AM +0300, Dmitry Osipenko wrote:
> On 3/11/22 11:33, Sascha Hauer wrote:
> > The rk3568 HDMI has an additional clock that needs to be enabled for the
> > HDMI controller to work. This clock is not needed for the HDMI
> > controller itself, but to make the SoC internal bus logic work. From the
> > reference manual:
> > 
> >> 2.8.6 NIU Clock gating reliance
> >>
> >> A part of niu clocks have a dependence on another niu clock in order to
> >> sharing the internal bus. When these clocks are in use, another niu
> >> clock must be opened, and cannot be gated.  These clocks and the special
> >> clock on which they are relied are as following:
> >>
> >> Clocks which have dependency     The clock which can not be gated
> >> -----------------------------------------------------------------
> >> ...
> >> pclk_vo_niu, hclk_vo_s_niu       hclk_vo_niu
> >> ...
> > The clock framework does not support turning on a clock whenever another
> > clock is turned on, so this patch adds support for the dependent clock
> > to the HDMI driver. We call it "NIU", which is for "Native Interface
> > Unit"
> 
> This still doesn't make sense to me. You're saying that "pclk_vo_niu,
> hclk_vo_s_niu" depend on "hclk_vo_niu", but HDMI doesn't use pclk_vo, it
> uses pclk_hdmi.

pclk_hdmi_host is a child clock of pclk_vo:

     aclk_vo                  2        2        0   300000000          0     0  50000         Y
        aclk_hdcp             0        0        0   300000000          0     0  50000         N
        pclk_vo               2        3        0    75000000          0     0  50000         Y
           pclk_edp_ctrl      0        0        0    75000000          0     0  50000         N
           pclk_dsitx_1       0        0        0    75000000          0     0  50000         N
           pclk_dsitx_0       1        2        0    75000000          0     0  50000         Y
           pclk_hdmi_host     1        2        0    75000000          0     0  50000         Y
           pclk_hdcp          0        0        0    75000000          0     0  50000         N
        hclk_vo               2        5        0   150000000          0     0  50000         Y
           hclk_hdcp          0        0        0   150000000          0     0  50000         N
           hclk_vop           0        2        0   150000000          0     0  50000         N

Sascha

> I don't see how these "niu" clocks are related to HDMI
> from your description. The pclk_vo should remain being disabled for HDMI.
> 
> I already said before that the datasheet suggests that hclk_vo needs to
> be enabled in order to ungate the AHB part of the VO module, which is
> needed for CPU accesses. Elain, could you please confirm this or clarify
> more about why hclk_vo needs to be enabled for HDMI?
>
Dmitry Osipenko March 14, 2022, 5:54 p.m. UTC | #3
On 3/14/22 11:18, Sascha Hauer wrote:
> On Sun, Mar 13, 2022 at 12:07:56AM +0300, Dmitry Osipenko wrote:
>> On 3/11/22 11:33, Sascha Hauer wrote:
>>> The rk3568 HDMI has an additional clock that needs to be enabled for the
>>> HDMI controller to work. This clock is not needed for the HDMI
>>> controller itself, but to make the SoC internal bus logic work. From the
>>> reference manual:
>>>
>>>> 2.8.6 NIU Clock gating reliance
>>>>
>>>> A part of niu clocks have a dependence on another niu clock in order to
>>>> sharing the internal bus. When these clocks are in use, another niu
>>>> clock must be opened, and cannot be gated.  These clocks and the special
>>>> clock on which they are relied are as following:
>>>>
>>>> Clocks which have dependency     The clock which can not be gated
>>>> -----------------------------------------------------------------
>>>> ...
>>>> pclk_vo_niu, hclk_vo_s_niu       hclk_vo_niu
>>>> ...
>>> The clock framework does not support turning on a clock whenever another
>>> clock is turned on, so this patch adds support for the dependent clock
>>> to the HDMI driver. We call it "NIU", which is for "Native Interface
>>> Unit"
>>
>> This still doesn't make sense to me. You're saying that "pclk_vo_niu,
>> hclk_vo_s_niu" depend on "hclk_vo_niu", but HDMI doesn't use pclk_vo, it
>> uses pclk_hdmi.
> 
> pclk_hdmi_host is a child clock of pclk_vo:
> 
>      aclk_vo                  2        2        0   300000000          0     0  50000         Y
>         aclk_hdcp             0        0        0   300000000          0     0  50000         N
>         pclk_vo               2        3        0    75000000          0     0  50000         Y
>            pclk_edp_ctrl      0        0        0    75000000          0     0  50000         N
>            pclk_dsitx_1       0        0        0    75000000          0     0  50000         N
>            pclk_dsitx_0       1        2        0    75000000          0     0  50000         Y
>            pclk_hdmi_host     1        2        0    75000000          0     0  50000         Y
>            pclk_hdcp          0        0        0    75000000          0     0  50000         N
>         hclk_vo               2        5        0   150000000          0     0  50000         Y
>            hclk_hdcp          0        0        0   150000000          0     0  50000         N
>            hclk_vop           0        2        0   150000000          0     0  50000         N

It was unclear that the pclk_hdmi is the child of pclk_vo by looking at
the clk driver's code, thank you!

Won't be better if the implicit clk dependency would be handled
internally by the RK clk driver? For example, by making the common gate
shared/refcounted. Have you considered this variant? Then we won't need
to change the DT bindings.
Sascha Hauer March 16, 2022, 9:12 a.m. UTC | #4
On Mon, Mar 14, 2022 at 08:54:27PM +0300, Dmitry Osipenko wrote:
> On 3/14/22 11:18, Sascha Hauer wrote:
> > On Sun, Mar 13, 2022 at 12:07:56AM +0300, Dmitry Osipenko wrote:
> >> On 3/11/22 11:33, Sascha Hauer wrote:
> >>> The rk3568 HDMI has an additional clock that needs to be enabled for the
> >>> HDMI controller to work. This clock is not needed for the HDMI
> >>> controller itself, but to make the SoC internal bus logic work. From the
> >>> reference manual:
> >>>
> >>>> 2.8.6 NIU Clock gating reliance
> >>>>
> >>>> A part of niu clocks have a dependence on another niu clock in order to
> >>>> sharing the internal bus. When these clocks are in use, another niu
> >>>> clock must be opened, and cannot be gated.  These clocks and the special
> >>>> clock on which they are relied are as following:
> >>>>
> >>>> Clocks which have dependency     The clock which can not be gated
> >>>> -----------------------------------------------------------------
> >>>> ...
> >>>> pclk_vo_niu, hclk_vo_s_niu       hclk_vo_niu
> >>>> ...
> >>> The clock framework does not support turning on a clock whenever another
> >>> clock is turned on, so this patch adds support for the dependent clock
> >>> to the HDMI driver. We call it "NIU", which is for "Native Interface
> >>> Unit"
> >>
> >> This still doesn't make sense to me. You're saying that "pclk_vo_niu,
> >> hclk_vo_s_niu" depend on "hclk_vo_niu", but HDMI doesn't use pclk_vo, it
> >> uses pclk_hdmi.
> > 
> > pclk_hdmi_host is a child clock of pclk_vo:
> > 
> >      aclk_vo                  2        2        0   300000000          0     0  50000         Y
> >         aclk_hdcp             0        0        0   300000000          0     0  50000         N
> >         pclk_vo               2        3        0    75000000          0     0  50000         Y
> >            pclk_edp_ctrl      0        0        0    75000000          0     0  50000         N
> >            pclk_dsitx_1       0        0        0    75000000          0     0  50000         N
> >            pclk_dsitx_0       1        2        0    75000000          0     0  50000         Y
> >            pclk_hdmi_host     1        2        0    75000000          0     0  50000         Y
> >            pclk_hdcp          0        0        0    75000000          0     0  50000         N
> >         hclk_vo               2        5        0   150000000          0     0  50000         Y
> >            hclk_hdcp          0        0        0   150000000          0     0  50000         N
> >            hclk_vop           0        2        0   150000000          0     0  50000         N
> 
> It was unclear that the pclk_hdmi is the child of pclk_vo by looking at
> the clk driver's code, thank you!
> 
> Won't be better if the implicit clk dependency would be handled
> internally by the RK clk driver?

I have considered handling something like coupled clocks in the clock
framework, but I have not yet considered handling this internally in the
Rockchip clock driver.

I just had a quick look at the driver. While it sounds like an easy task
to enable gate A whenever gate B is enabled I haven't found a good way to
integrate that into the clk driver. It seems to me that it's not easier
to implement in the clk driver than it is to implement it in the clk
framework where it could be used by others as well.

> For example, by making the common gate
> shared/refcounted. Have you considered this variant? Then we won't need
> to change the DT bindings.

For the DT bindings it is just an additional clock. Should we have a
better way to handle that case in the future we could simply ignore the
additional clock. I wouldn't bother too much about this.

Sascha
Dmitry Osipenko March 16, 2022, 1:01 p.m. UTC | #5
On 3/16/22 12:12, Sascha Hauer wrote:
> On Mon, Mar 14, 2022 at 08:54:27PM +0300, Dmitry Osipenko wrote:
>> On 3/14/22 11:18, Sascha Hauer wrote:
>>> On Sun, Mar 13, 2022 at 12:07:56AM +0300, Dmitry Osipenko wrote:
>>>> On 3/11/22 11:33, Sascha Hauer wrote:
>>>>> The rk3568 HDMI has an additional clock that needs to be enabled for the
>>>>> HDMI controller to work. This clock is not needed for the HDMI
>>>>> controller itself, but to make the SoC internal bus logic work. From the
>>>>> reference manual:
>>>>>
>>>>>> 2.8.6 NIU Clock gating reliance
>>>>>>
>>>>>> A part of niu clocks have a dependence on another niu clock in order to
>>>>>> sharing the internal bus. When these clocks are in use, another niu
>>>>>> clock must be opened, and cannot be gated.  These clocks and the special
>>>>>> clock on which they are relied are as following:
>>>>>>
>>>>>> Clocks which have dependency     The clock which can not be gated
>>>>>> -----------------------------------------------------------------
>>>>>> ...
>>>>>> pclk_vo_niu, hclk_vo_s_niu       hclk_vo_niu
>>>>>> ...
>>>>> The clock framework does not support turning on a clock whenever another
>>>>> clock is turned on, so this patch adds support for the dependent clock
>>>>> to the HDMI driver. We call it "NIU", which is for "Native Interface
>>>>> Unit"
>>>>
>>>> This still doesn't make sense to me. You're saying that "pclk_vo_niu,
>>>> hclk_vo_s_niu" depend on "hclk_vo_niu", but HDMI doesn't use pclk_vo, it
>>>> uses pclk_hdmi.
>>>
>>> pclk_hdmi_host is a child clock of pclk_vo:
>>>
>>>      aclk_vo                  2        2        0   300000000          0     0  50000         Y
>>>         aclk_hdcp             0        0        0   300000000          0     0  50000         N
>>>         pclk_vo               2        3        0    75000000          0     0  50000         Y
>>>            pclk_edp_ctrl      0        0        0    75000000          0     0  50000         N
>>>            pclk_dsitx_1       0        0        0    75000000          0     0  50000         N
>>>            pclk_dsitx_0       1        2        0    75000000          0     0  50000         Y
>>>            pclk_hdmi_host     1        2        0    75000000          0     0  50000         Y
>>>            pclk_hdcp          0        0        0    75000000          0     0  50000         N
>>>         hclk_vo               2        5        0   150000000          0     0  50000         Y
>>>            hclk_hdcp          0        0        0   150000000          0     0  50000         N
>>>            hclk_vop           0        2        0   150000000          0     0  50000         N
>>
>> It was unclear that the pclk_hdmi is the child of pclk_vo by looking at
>> the clk driver's code, thank you!
>>
>> Won't be better if the implicit clk dependency would be handled
>> internally by the RK clk driver?
> 
> I have considered handling something like coupled clocks in the clock
> framework, but I have not yet considered handling this internally in the
> Rockchip clock driver.
> 
> I just had a quick look at the driver. While it sounds like an easy task
> to enable gate A whenever gate B is enabled I haven't found a good way to
> integrate that into the clk driver. It seems to me that it's not easier
> to implement in the clk driver than it is to implement it in the clk
> framework where it could be used by others as well.
> 
>> For example, by making the common gate
>> shared/refcounted. Have you considered this variant? Then we won't need
>> to change the DT bindings.
> 
> For the DT bindings it is just an additional clock. Should we have a
> better way to handle that case in the future we could simply ignore the
> additional clock. I wouldn't bother too much about this.

To me that NIU quirk should be internal to the clk h/w module, so it
doesn't feel nice to mix the clk h/w description with the HDMI h/w
description.

On the other hand, making clk driver to handle this case indeed will
take some effort as I see now. For example, clk driver of NVIDIA Tegra
has concept of shared gates, but bringing it to the RK clk driver will
be quite messy.

Alright, let's work around the clk limitation like you're suggesting. I
agree that it shouldn't really be a problem to deprecate the extra clock
later on.

I have last questions..

1. Previously you said that the PD driver takes care of enabling all the
clocks it can find in the device by itself on RPM-resume, then why HDMI
driver needs to enable the clock explicitly?

2. You added clk_prepare_enable(), but there is no corresponding
clk_disable_unprepare(), AFAICS. Why?
Robin Murphy March 16, 2022, 1:55 p.m. UTC | #6
On 2022-03-16 13:01, Dmitry Osipenko wrote:
> On 3/16/22 12:12, Sascha Hauer wrote:
>> On Mon, Mar 14, 2022 at 08:54:27PM +0300, Dmitry Osipenko wrote:
>>> On 3/14/22 11:18, Sascha Hauer wrote:
>>>> On Sun, Mar 13, 2022 at 12:07:56AM +0300, Dmitry Osipenko wrote:
>>>>> On 3/11/22 11:33, Sascha Hauer wrote:
>>>>>> The rk3568 HDMI has an additional clock that needs to be enabled for the
>>>>>> HDMI controller to work. This clock is not needed for the HDMI
>>>>>> controller itself, but to make the SoC internal bus logic work. From the
>>>>>> reference manual:
>>>>>>
>>>>>>> 2.8.6 NIU Clock gating reliance
>>>>>>>
>>>>>>> A part of niu clocks have a dependence on another niu clock in order to
>>>>>>> sharing the internal bus. When these clocks are in use, another niu
>>>>>>> clock must be opened, and cannot be gated.  These clocks and the special
>>>>>>> clock on which they are relied are as following:
>>>>>>>
>>>>>>> Clocks which have dependency     The clock which can not be gated
>>>>>>> -----------------------------------------------------------------
>>>>>>> ...
>>>>>>> pclk_vo_niu, hclk_vo_s_niu       hclk_vo_niu
>>>>>>> ...
>>>>>> The clock framework does not support turning on a clock whenever another
>>>>>> clock is turned on, so this patch adds support for the dependent clock
>>>>>> to the HDMI driver. We call it "NIU", which is for "Native Interface
>>>>>> Unit"
>>>>>
>>>>> This still doesn't make sense to me. You're saying that "pclk_vo_niu,
>>>>> hclk_vo_s_niu" depend on "hclk_vo_niu", but HDMI doesn't use pclk_vo, it
>>>>> uses pclk_hdmi.
>>>>
>>>> pclk_hdmi_host is a child clock of pclk_vo:
>>>>
>>>>       aclk_vo                  2        2        0   300000000          0     0  50000         Y
>>>>          aclk_hdcp             0        0        0   300000000          0     0  50000         N
>>>>          pclk_vo               2        3        0    75000000          0     0  50000         Y
>>>>             pclk_edp_ctrl      0        0        0    75000000          0     0  50000         N
>>>>             pclk_dsitx_1       0        0        0    75000000          0     0  50000         N
>>>>             pclk_dsitx_0       1        2        0    75000000          0     0  50000         Y
>>>>             pclk_hdmi_host     1        2        0    75000000          0     0  50000         Y
>>>>             pclk_hdcp          0        0        0    75000000          0     0  50000         N
>>>>          hclk_vo               2        5        0   150000000          0     0  50000         Y
>>>>             hclk_hdcp          0        0        0   150000000          0     0  50000         N
>>>>             hclk_vop           0        2        0   150000000          0     0  50000         N
>>>
>>> It was unclear that the pclk_hdmi is the child of pclk_vo by looking at
>>> the clk driver's code, thank you!
>>>
>>> Won't be better if the implicit clk dependency would be handled
>>> internally by the RK clk driver?
>>
>> I have considered handling something like coupled clocks in the clock
>> framework, but I have not yet considered handling this internally in the
>> Rockchip clock driver.
>>
>> I just had a quick look at the driver. While it sounds like an easy task
>> to enable gate A whenever gate B is enabled I haven't found a good way to
>> integrate that into the clk driver. It seems to me that it's not easier
>> to implement in the clk driver than it is to implement it in the clk
>> framework where it could be used by others as well.
>>
>>> For example, by making the common gate
>>> shared/refcounted. Have you considered this variant? Then we won't need
>>> to change the DT bindings.
>>
>> For the DT bindings it is just an additional clock. Should we have a
>> better way to handle that case in the future we could simply ignore the
>> additional clock. I wouldn't bother too much about this.
> 
> To me that NIU quirk should be internal to the clk h/w module, so it
> doesn't feel nice to mix the clk h/w description with the HDMI h/w
> description.
> 
> On the other hand, making clk driver to handle this case indeed will
> take some effort as I see now. For example, clk driver of NVIDIA Tegra
> has concept of shared gates, but bringing it to the RK clk driver will
> be quite messy.

 From a quick look, it seems like it could be straightforward 
conceptually at least. Presumably: subclass clk_gate_ops to 
enable/disable a required clock before enabling/disabling normally, have 
rockchip_clk_register_branch() resolve an optional required clock and 
pick gate_ops as appropriate, then the rest is basically just 
boilerplate for describing the dependencies in the first place. However 
I'd agree that in practical implementation terms it does look even 
simpler and cleaner for the clk_hw abstraction to provide the 
appropriate ops and resolution itself.

> Alright, let's work around the clk limitation like you're suggesting. I
> agree that it shouldn't really be a problem to deprecate the extra clock
> later on.

If there's a realistic chance that someone will actually work on a 
proper coupled/dependent/whatever clock abstraction before the rest of 
RK3588 is supported well enough for mainline users to start really 
caring about power efficiency, then arguably the simplest and cleanest 
workaround would be the other option that Elaine mentioned, of just 
marking hclk_vo as critical for now. If it's likely to turn into a 
"nothing's as permanent as a temporary fix" situation, though, then the 
DT binding has less functional impact, even if it does leave us 
developers with baggage down the line.

Robin.

> 
> I have last questions..
> 
> 1. Previously you said that the PD driver takes care of enabling all the
> clocks it can find in the device by itself on RPM-resume, then why HDMI
> driver needs to enable the clock explicitly?
> 
> 2. You added clk_prepare_enable(), but there is no corresponding
> clk_disable_unprepare(), AFAICS. Why?
Dmitry Osipenko March 16, 2022, 2:01 p.m. UTC | #7
On 3/16/22 16:55, Robin Murphy wrote:
>> To me that NIU quirk should be internal to the clk h/w module, so it
>> doesn't feel nice to mix the clk h/w description with the HDMI h/w
>> description.
>>
>> On the other hand, making clk driver to handle this case indeed will
>> take some effort as I see now. For example, clk driver of NVIDIA Tegra
>> has concept of shared gates, but bringing it to the RK clk driver will
>> be quite messy.
> 
> From a quick look, it seems like it could be straightforward
> conceptually at least. Presumably: subclass clk_gate_ops to
> enable/disable a required clock before enabling/disabling normally, have
> rockchip_clk_register_branch() resolve an optional required clock and
> pick gate_ops as appropriate, then the rest is basically just
> boilerplate for describing the dependencies in the first place. However
> I'd agree that in practical implementation terms it does look even
> simpler and cleaner for the clk_hw abstraction to provide the
> appropriate ops and resolution itself.
> 
>> Alright, let's work around the clk limitation like you're suggesting. I
>> agree that it shouldn't really be a problem to deprecate the extra clock
>> later on.
> 
> If there's a realistic chance that someone will actually work on a
> proper coupled/dependent/whatever clock abstraction before the rest of
> RK3588 is supported well enough for mainline users to start really
> caring about power efficiency, then arguably the simplest and cleanest
> workaround would be the other option that Elaine mentioned, of just
> marking hclk_vo as critical for now. If it's likely to turn into a
> "nothing's as permanent as a temporary fix" situation, though, then the
> DT binding has less functional impact, even if it does leave us
> developers with baggage down the line.

I missed that suggestion about marking hclk_vo as critical. That's a
good idea, I like it.
Sascha Hauer March 16, 2022, 2:31 p.m. UTC | #8
On Wed, Mar 16, 2022 at 04:01:49PM +0300, Dmitry Osipenko wrote:
> On 3/16/22 12:12, Sascha Hauer wrote:
> > On Mon, Mar 14, 2022 at 08:54:27PM +0300, Dmitry Osipenko wrote:
> >> On 3/14/22 11:18, Sascha Hauer wrote:
> >>> On Sun, Mar 13, 2022 at 12:07:56AM +0300, Dmitry Osipenko wrote:
> >>>> On 3/11/22 11:33, Sascha Hauer wrote:
> >>>>> The rk3568 HDMI has an additional clock that needs to be enabled for the
> >>>>> HDMI controller to work. This clock is not needed for the HDMI
> >>>>> controller itself, but to make the SoC internal bus logic work. From the
> >>>>> reference manual:
> >>>>>
> >>>>>> 2.8.6 NIU Clock gating reliance
> >>>>>>
> >>>>>> A part of niu clocks have a dependence on another niu clock in order to
> >>>>>> sharing the internal bus. When these clocks are in use, another niu
> >>>>>> clock must be opened, and cannot be gated.  These clocks and the special
> >>>>>> clock on which they are relied are as following:
> >>>>>>
> >>>>>> Clocks which have dependency     The clock which can not be gated
> >>>>>> -----------------------------------------------------------------
> >>>>>> ...
> >>>>>> pclk_vo_niu, hclk_vo_s_niu       hclk_vo_niu
> >>>>>> ...
> >>>>> The clock framework does not support turning on a clock whenever another
> >>>>> clock is turned on, so this patch adds support for the dependent clock
> >>>>> to the HDMI driver. We call it "NIU", which is for "Native Interface
> >>>>> Unit"
> >>>>
> >>>> This still doesn't make sense to me. You're saying that "pclk_vo_niu,
> >>>> hclk_vo_s_niu" depend on "hclk_vo_niu", but HDMI doesn't use pclk_vo, it
> >>>> uses pclk_hdmi.
> >>>
> >>> pclk_hdmi_host is a child clock of pclk_vo:
> >>>
> >>>      aclk_vo                  2        2        0   300000000          0     0  50000         Y
> >>>         aclk_hdcp             0        0        0   300000000          0     0  50000         N
> >>>         pclk_vo               2        3        0    75000000          0     0  50000         Y
> >>>            pclk_edp_ctrl      0        0        0    75000000          0     0  50000         N
> >>>            pclk_dsitx_1       0        0        0    75000000          0     0  50000         N
> >>>            pclk_dsitx_0       1        2        0    75000000          0     0  50000         Y
> >>>            pclk_hdmi_host     1        2        0    75000000          0     0  50000         Y
> >>>            pclk_hdcp          0        0        0    75000000          0     0  50000         N
> >>>         hclk_vo               2        5        0   150000000          0     0  50000         Y
> >>>            hclk_hdcp          0        0        0   150000000          0     0  50000         N
> >>>            hclk_vop           0        2        0   150000000          0     0  50000         N
> >>
> >> It was unclear that the pclk_hdmi is the child of pclk_vo by looking at
> >> the clk driver's code, thank you!
> >>
> >> Won't be better if the implicit clk dependency would be handled
> >> internally by the RK clk driver?
> > 
> > I have considered handling something like coupled clocks in the clock
> > framework, but I have not yet considered handling this internally in the
> > Rockchip clock driver.
> > 
> > I just had a quick look at the driver. While it sounds like an easy task
> > to enable gate A whenever gate B is enabled I haven't found a good way to
> > integrate that into the clk driver. It seems to me that it's not easier
> > to implement in the clk driver than it is to implement it in the clk
> > framework where it could be used by others as well.
> > 
> >> For example, by making the common gate
> >> shared/refcounted. Have you considered this variant? Then we won't need
> >> to change the DT bindings.
> > 
> > For the DT bindings it is just an additional clock. Should we have a
> > better way to handle that case in the future we could simply ignore the
> > additional clock. I wouldn't bother too much about this.
> 
> To me that NIU quirk should be internal to the clk h/w module, so it
> doesn't feel nice to mix the clk h/w description with the HDMI h/w
> description.
> 
> On the other hand, making clk driver to handle this case indeed will
> take some effort as I see now. For example, clk driver of NVIDIA Tegra
> has concept of shared gates, but bringing it to the RK clk driver will
> be quite messy.
> 
> Alright, let's work around the clk limitation like you're suggesting. I
> agree that it shouldn't really be a problem to deprecate the extra clock
> later on.
> 
> I have last questions..
> 
> 1. Previously you said that the PD driver takes care of enabling all the
> clocks it can find in the device by itself on RPM-resume, then why HDMI
> driver needs to enable the clock explicitly?

The PD driver only does so when the device has pm_runtime enabled.
Currently that's not the case for the hdmi driver. That could be changed
of course, but I am not convinced that it's a good idea that the PD
driver messes with the consumers clocks. It would also make the PD
driver mandatory which it currently not is.

> 
> 2. You added clk_prepare_enable(), but there is no corresponding
> clk_disable_unprepare(), AFAICS. Why?

Because I missed it ;)

Sascha
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index fe4f9556239ac..7adf9044cb73b 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -76,6 +76,7 @@  struct rockchip_hdmi {
 	const struct rockchip_hdmi_chip_data *chip_data;
 	struct clk *ref_clk;
 	struct clk *grf_clk;
+	struct clk *niu_clk;
 	struct dw_hdmi *hdmi;
 	struct regulator *avdd_0v9;
 	struct regulator *avdd_1v8;
@@ -229,6 +230,14 @@  static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 		return PTR_ERR(hdmi->grf_clk);
 	}
 
+	hdmi->niu_clk = devm_clk_get_optional(hdmi->dev, "niu");
+	if (PTR_ERR(hdmi->niu_clk) == -EPROBE_DEFER) {
+		return -EPROBE_DEFER;
+	} else if (IS_ERR(hdmi->niu_clk)) {
+		DRM_DEV_ERROR(hdmi->dev, "failed to get niu clock\n");
+		return PTR_ERR(hdmi->niu_clk);
+	}
+
 	hdmi->avdd_0v9 = devm_regulator_get(hdmi->dev, "avdd-0v9");
 	if (IS_ERR(hdmi->avdd_0v9))
 		return PTR_ERR(hdmi->avdd_0v9);
@@ -596,6 +605,13 @@  static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 		goto err_clk;
 	}
 
+	ret = clk_prepare_enable(hdmi->niu_clk);
+	if (ret) {
+		DRM_DEV_ERROR(hdmi->dev, "Failed to enable HDMI hclk clock: %d\n",
+			      ret);
+		goto err_clk;
+	}
+
 	if (hdmi->chip_data == &rk3568_chip_data) {
 		regmap_write(hdmi->regmap, RK3568_GRF_VO_CON1,
 			     HIWORD_UPDATE(RK3568_HDMI_SDAIN_MSK |