linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] clk: rockchip: Fix video codec clocks on rk3288
@ 2019-03-29 21:54 Douglas Anderson
  2019-03-29 23:28 ` Ezequiel Garcia
  2019-04-10 15:45 ` Doug Anderson
  0 siblings, 2 replies; 9+ messages in thread
From: Douglas Anderson @ 2019-03-29 21:54 UTC (permalink / raw)
  To: Heiko Stuebner
  Cc: Tomasz Figa, Ziyuan Xu, Ezequiel Garcia, ryandcase, Elaine Zhang,
	mka, Douglas Anderson, Michael Turquette, Stephen Boyd,
	linux-kernel, linux-rockchip, linux-clk, linux-arm-kernel

It appears that there is a typo in the rk3288 TRM.  For
GRF_SOC_CON0[7] it says that 0 means "vepu" and 1 means "vdpu".  It's
the other way around.

How do I know?  Here's my evidence:

1. Prior to commit 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec
   using the new muxgrf type on rk3288") we always pretended that we
   were using "aclk_vdpu" and the comment in the code said that this
   matched the default setting in the system.  In fact the default
   setting is 0 according to the TRM and according to reading memory
   at bootup.  In addition rk3288-based Chromebooks ran like this and
   the video codecs worked.
2. With the existing clock code if you boot up and try to enable the
   new VIDEO_ROCKCHIP_VPU as a module (and without "clk_ignore_unused"
   on the command line), you get errors like "failed to get ack on
   domain 'pd_video', val=0x80208".  After flipping vepu/vdpu things
   init OK.
3. If I export and add both the vepu and vdpu to the list of clocks
   for RK3288_PD_VIDEO I can get past the power domain errors, but now
   I freeze when the vpu_mmu gets initted.
4. If I just mark the "vdpu" as IGNORE_UNUSED then everything boots up
   and probes OK showing that somehow the "vdpu" was important to keep
   enabled.  This is because we were actually using it as a parent.
5. After this change I can hack "aclk_vcodec_pre" to parent from
   "aclk_vepu" using assigned-clocks and the video codec still probes
   OK.

Fixes: 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec using the new muxgrf type on rk3288")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I currently have no way to test the JPEG mem2mem driver, so hopefully
others can test this and make sure it's happy for them.  I'm just
happy not to get strange errors at boot anymore.

 drivers/clk/rockchip/clk-rk3288.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
index 5a67b7869960..4d767f9c3a80 100644
--- a/drivers/clk/rockchip/clk-rk3288.c
+++ b/drivers/clk/rockchip/clk-rk3288.c
@@ -219,7 +219,7 @@ PNAME(mux_hsadcout_p)	= { "hsadc_src", "ext_hsadc" };
 PNAME(mux_edp_24m_p)	= { "ext_edp_24m", "xin24m" };
 PNAME(mux_tspout_p)	= { "cpll", "gpll", "npll", "xin27m" };
 
-PNAME(mux_aclk_vcodec_pre_p)	= { "aclk_vepu", "aclk_vdpu" };
+PNAME(mux_aclk_vcodec_pre_p)	= { "aclk_vdpu", "aclk_vepu" };
 PNAME(mux_usbphy480m_p)		= { "sclk_otgphy1_480m", "sclk_otgphy2_480m",
 				    "sclk_otgphy0_480m" };
 PNAME(mux_hsicphy480m_p)	= { "cpll", "gpll", "usbphy480m_src" };
-- 
2.21.0.392.gf8f6787159e-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] clk: rockchip: Fix video codec clocks on rk3288
  2019-03-29 21:54 [PATCH] clk: rockchip: Fix video codec clocks on rk3288 Douglas Anderson
@ 2019-03-29 23:28 ` Ezequiel Garcia
  2019-03-30 14:24   ` Ezequiel Garcia
  2019-04-10 15:45 ` Doug Anderson
  1 sibling, 1 reply; 9+ messages in thread
From: Ezequiel Garcia @ 2019-03-29 23:28 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Heiko Stuebner, Tomasz Figa, Ziyuan Xu, Ezequiel Garcia,
	ryandcase, Elaine Zhang, Matthias Kaehlcke, Michael Turquette,
	Stephen Boyd, Linux Kernel Mailing List,
	open list:ARM/Rockchip SoC...,
	linux-clk, linux-arm-kernel

On Fri, 29 Mar 2019 at 18:55, Douglas Anderson <dianders@chromium.org> wrote:
>
> It appears that there is a typo in the rk3288 TRM.  For
> GRF_SOC_CON0[7] it says that 0 means "vepu" and 1 means "vdpu".  It's
> the other way around.
>
> How do I know?  Here's my evidence:
>
> 1. Prior to commit 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec
>    using the new muxgrf type on rk3288") we always pretended that we
>    were using "aclk_vdpu" and the comment in the code said that this
>    matched the default setting in the system.  In fact the default
>    setting is 0 according to the TRM and according to reading memory
>    at bootup.  In addition rk3288-based Chromebooks ran like this and
>    the video codecs worked.
> 2. With the existing clock code if you boot up and try to enable the
>    new VIDEO_ROCKCHIP_VPU as a module (and without "clk_ignore_unused"
>    on the command line), you get errors like "failed to get ack on
>    domain 'pd_video', val=0x80208".  After flipping vepu/vdpu things
>    init OK.
> 3. If I export and add both the vepu and vdpu to the list of clocks
>    for RK3288_PD_VIDEO I can get past the power domain errors, but now
>    I freeze when the vpu_mmu gets initted.
> 4. If I just mark the "vdpu" as IGNORE_UNUSED then everything boots up
>    and probes OK showing that somehow the "vdpu" was important to keep
>    enabled.  This is because we were actually using it as a parent.
> 5. After this change I can hack "aclk_vcodec_pre" to parent from
>    "aclk_vepu" using assigned-clocks and the video codec still probes
>    OK.
>
> Fixes: 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec using the new muxgrf type on rk3288")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> I currently have no way to test the JPEG mem2mem driver, so hopefully
> others can test this and make sure it's happy for them.  I'm just
> happy not to get strange errors at boot anymore.
>

I won't have access to this hardware for a few days, but I am happy
to provide a simple test tool.

Still haven't reviewed this, but thanks for chasing it down!

EZe

>  drivers/clk/rockchip/clk-rk3288.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
> index 5a67b7869960..4d767f9c3a80 100644
> --- a/drivers/clk/rockchip/clk-rk3288.c
> +++ b/drivers/clk/rockchip/clk-rk3288.c
> @@ -219,7 +219,7 @@ PNAME(mux_hsadcout_p)       = { "hsadc_src", "ext_hsadc" };
>  PNAME(mux_edp_24m_p)   = { "ext_edp_24m", "xin24m" };
>  PNAME(mux_tspout_p)    = { "cpll", "gpll", "npll", "xin27m" };
>
> -PNAME(mux_aclk_vcodec_pre_p)   = { "aclk_vepu", "aclk_vdpu" };
> +PNAME(mux_aclk_vcodec_pre_p)   = { "aclk_vdpu", "aclk_vepu" };
>  PNAME(mux_usbphy480m_p)                = { "sclk_otgphy1_480m", "sclk_otgphy2_480m",
>                                     "sclk_otgphy0_480m" };
>  PNAME(mux_hsicphy480m_p)       = { "cpll", "gpll", "usbphy480m_src" };
> --
> 2.21.0.392.gf8f6787159e-goog
>


-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] clk: rockchip: Fix video codec clocks on rk3288
  2019-03-29 23:28 ` Ezequiel Garcia
@ 2019-03-30 14:24   ` Ezequiel Garcia
  2019-03-30 18:35     ` Doug Anderson
  0 siblings, 1 reply; 9+ messages in thread
From: Ezequiel Garcia @ 2019-03-30 14:24 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Heiko Stuebner, Tomasz Figa, Ziyuan Xu, Ezequiel Garcia,
	ryandcase, Elaine Zhang, Matthias Kaehlcke, Michael Turquette,
	Stephen Boyd, Linux Kernel Mailing List,
	open list:ARM/Rockchip SoC...,
	linux-clk, linux-arm-kernel

On Fri, 29 Mar 2019 at 20:28, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
>
> On Fri, 29 Mar 2019 at 18:55, Douglas Anderson <dianders@chromium.org> wrote:
> >
> > It appears that there is a typo in the rk3288 TRM.  For
> > GRF_SOC_CON0[7] it says that 0 means "vepu" and 1 means "vdpu".  It's
> > the other way around.
> >
> > How do I know?  Here's my evidence:
> >
> > 1. Prior to commit 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec
> >    using the new muxgrf type on rk3288") we always pretended that we
> >    were using "aclk_vdpu" and the comment in the code said that this
> >    matched the default setting in the system.  In fact the default
> >    setting is 0 according to the TRM and according to reading memory
> >    at bootup.  In addition rk3288-based Chromebooks ran like this and
> >    the video codecs worked.
> > 2. With the existing clock code if you boot up and try to enable the
> >    new VIDEO_ROCKCHIP_VPU as a module (and without "clk_ignore_unused"
> >    on the command line), you get errors like "failed to get ack on
> >    domain 'pd_video', val=0x80208".  After flipping vepu/vdpu things
> >    init OK.
> > 3. If I export and add both the vepu and vdpu to the list of clocks
> >    for RK3288_PD_VIDEO I can get past the power domain errors, but now
> >    I freeze when the vpu_mmu gets initted.
> > 4. If I just mark the "vdpu" as IGNORE_UNUSED then everything boots up
> >    and probes OK showing that somehow the "vdpu" was important to keep
> >    enabled.  This is because we were actually using it as a parent.
> > 5. After this change I can hack "aclk_vcodec_pre" to parent from
> >    "aclk_vepu" using assigned-clocks and the video codec still probes
> >    OK.
> >
> > Fixes: 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec using the new muxgrf type on rk3288")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > I currently have no way to test the JPEG mem2mem driver, so hopefully
> > others can test this and make sure it's happy for them.  I'm just
> > happy not to get strange errors at boot anymore.
> >
>
> I won't have access to this hardware for a few days, but I am happy
> to provide a simple test tool.
>
> Still haven't reviewed this, but thanks for chasing it down!
>

Here's a simple tool that tests JPEG encoding. There are two branches,
with request API and without request API:

https://gitlab.collabora.com/ezequiel/v4l-jpeg

Usage is fairly simple, there's a test.sh that runs three tests,
writing three JPEG images.

You can also use v4l2jpegenc gstreamer element,
but it's slightly more involved to set up.

Hope it helps,
Eze

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] clk: rockchip: Fix video codec clocks on rk3288
  2019-03-30 14:24   ` Ezequiel Garcia
@ 2019-03-30 18:35     ` Doug Anderson
  0 siblings, 0 replies; 9+ messages in thread
From: Doug Anderson @ 2019-03-30 18:35 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: Heiko Stuebner, Tomasz Figa, Ziyuan Xu, Ezequiel Garcia,
	Ryan Case, Elaine Zhang, Matthias Kaehlcke, Michael Turquette,
	Stephen Boyd, Linux Kernel Mailing List,
	open list:ARM/Rockchip SoC...,
	linux-clk, linux-arm-kernel

Hi,

On Sat, Mar 30, 2019 at 7:25 AM Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
>
> On Fri, 29 Mar 2019 at 20:28, Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
> >
> > On Fri, 29 Mar 2019 at 18:55, Douglas Anderson <dianders@chromium.org> wrote:
> > >
> > > It appears that there is a typo in the rk3288 TRM.  For
> > > GRF_SOC_CON0[7] it says that 0 means "vepu" and 1 means "vdpu".  It's
> > > the other way around.
> > >
> > > How do I know?  Here's my evidence:
> > >
> > > 1. Prior to commit 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec
> > >    using the new muxgrf type on rk3288") we always pretended that we
> > >    were using "aclk_vdpu" and the comment in the code said that this
> > >    matched the default setting in the system.  In fact the default
> > >    setting is 0 according to the TRM and according to reading memory
> > >    at bootup.  In addition rk3288-based Chromebooks ran like this and
> > >    the video codecs worked.
> > > 2. With the existing clock code if you boot up and try to enable the
> > >    new VIDEO_ROCKCHIP_VPU as a module (and without "clk_ignore_unused"
> > >    on the command line), you get errors like "failed to get ack on
> > >    domain 'pd_video', val=0x80208".  After flipping vepu/vdpu things
> > >    init OK.
> > > 3. If I export and add both the vepu and vdpu to the list of clocks
> > >    for RK3288_PD_VIDEO I can get past the power domain errors, but now
> > >    I freeze when the vpu_mmu gets initted.
> > > 4. If I just mark the "vdpu" as IGNORE_UNUSED then everything boots up
> > >    and probes OK showing that somehow the "vdpu" was important to keep
> > >    enabled.  This is because we were actually using it as a parent.
> > > 5. After this change I can hack "aclk_vcodec_pre" to parent from
> > >    "aclk_vepu" using assigned-clocks and the video codec still probes
> > >    OK.
> > >
> > > Fixes: 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec using the new muxgrf type on rk3288")
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > > I currently have no way to test the JPEG mem2mem driver, so hopefully
> > > others can test this and make sure it's happy for them.  I'm just
> > > happy not to get strange errors at boot anymore.
> > >
> >
> > I won't have access to this hardware for a few days, but I am happy
> > to provide a simple test tool.
> >
> > Still haven't reviewed this, but thanks for chasing it down!
> >
>
> Here's a simple tool that tests JPEG encoding. There are two branches,
> with request API and without request API:
>
> https://gitlab.collabora.com/ezequiel/v4l-jpeg
>
> Usage is fairly simple, there's a test.sh that runs three tests,
> writing three JPEG images.

Didn't work for me.  I got:

unable to allocate media request 25

===

OK, after a bunch of debugging, I realized that I needed your series
from <https://patchwork.kernel.org/cover/10838323/>.  I applied that
together with my patch to the to of Heiko's current for-next tree and
I see:

destination plane sizeimage 76800 bytes
source plane 0 sizeimage 76800 bytes
source plane 1 sizeimage 19200 bytes
source plane 2 sizeimage 19200 bytes
src plane 0 mapped 76800 bytes
src plane 1 mapped 19200 bytes
src plane 2 mapped 19200 bytes
capture buffer size: 76800
capture bytes: 6385
bars-i420-320_240.jpeg written, 6385 bytes
destination plane sizeimage 76800 bytes
source plane 0 sizeimage 76800 bytes
source plane 1 sizeimage 38400 bytes
src plane 0 mapped 76800 bytes
src plane 1 mapped 38400 bytes
capture buffer size: 76800
capture bytes: 6385
bars-nv12-320_240.jpeg written, 6385 bytes
destination plane sizeimage 76800 bytes
source plane 0 sizeimage 153600 bytes
src plane 0 mapped 153600 bytes
capture buffer size: 76800
capture bytes: 6385
bars-yuy2-320_240.jpeg written, 6385 bytes


-Doug

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] clk: rockchip: Fix video codec clocks on rk3288
  2019-03-29 21:54 [PATCH] clk: rockchip: Fix video codec clocks on rk3288 Douglas Anderson
  2019-03-29 23:28 ` Ezequiel Garcia
@ 2019-04-10 15:45 ` Doug Anderson
  2019-04-10 18:38   ` Jonas Karlman
  1 sibling, 1 reply; 9+ messages in thread
From: Doug Anderson @ 2019-04-10 15:45 UTC (permalink / raw)
  To: Heiko Stuebner, Elaine Zhang
  Cc: Tomasz Figa, Ziyuan Xu, Ezequiel Garcia, Ryan Case,
	Matthias Kaehlcke, Douglas Anderson, Michael Turquette,
	Stephen Boyd, LKML, open list:ARM/Rockchip SoC...,
	linux-clk, Linux ARM

Hi,

On Fri, Mar 29, 2019 at 2:55 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> It appears that there is a typo in the rk3288 TRM.  For
> GRF_SOC_CON0[7] it says that 0 means "vepu" and 1 means "vdpu".  It's
> the other way around.
>
> How do I know?  Here's my evidence:
>
> 1. Prior to commit 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec
>    using the new muxgrf type on rk3288") we always pretended that we
>    were using "aclk_vdpu" and the comment in the code said that this
>    matched the default setting in the system.  In fact the default
>    setting is 0 according to the TRM and according to reading memory
>    at bootup.  In addition rk3288-based Chromebooks ran like this and
>    the video codecs worked.
> 2. With the existing clock code if you boot up and try to enable the
>    new VIDEO_ROCKCHIP_VPU as a module (and without "clk_ignore_unused"
>    on the command line), you get errors like "failed to get ack on
>    domain 'pd_video', val=0x80208".  After flipping vepu/vdpu things
>    init OK.
> 3. If I export and add both the vepu and vdpu to the list of clocks
>    for RK3288_PD_VIDEO I can get past the power domain errors, but now
>    I freeze when the vpu_mmu gets initted.
> 4. If I just mark the "vdpu" as IGNORE_UNUSED then everything boots up
>    and probes OK showing that somehow the "vdpu" was important to keep
>    enabled.  This is because we were actually using it as a parent.
> 5. After this change I can hack "aclk_vcodec_pre" to parent from
>    "aclk_vepu" using assigned-clocks and the video codec still probes
>    OK.
>
> Fixes: 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec using the new muxgrf type on rk3288")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> I currently have no way to test the JPEG mem2mem driver, so hopefully
> others can test this and make sure it's happy for them.  I'm just
> happy not to get strange errors at boot anymore.
>
>  drivers/clk/rockchip/clk-rk3288.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Any thoughts about this patch?  I'm 99.9% certain it's correct and
it'd be nice to get it landed.  Heiko: I assume you're still
collecting Rockchip clock patches and would be the one to apply it and
(at some point) send a pull request to the clock tree?

-Doug

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] clk: rockchip: Fix video codec clocks on rk3288
  2019-04-10 15:45 ` Doug Anderson
@ 2019-04-10 18:38   ` Jonas Karlman
  2019-04-10 23:37     ` Doug Anderson
  0 siblings, 1 reply; 9+ messages in thread
From: Jonas Karlman @ 2019-04-10 18:38 UTC (permalink / raw)
  To: Doug Anderson, Heiko Stuebner, Elaine Zhang
  Cc: Tomasz Figa, Ziyuan Xu, Ezequiel Garcia, Ryan Case,
	Matthias Kaehlcke, Michael Turquette, Stephen Boyd, LKML,
	open list:ARM/Rockchip SoC...,
	linux-clk, Linux ARM

On 2019-04-10 17:45, Doug Anderson wrote:
> Hi,
>
> On Fri, Mar 29, 2019 at 2:55 PM Douglas Anderson <dianders@chromium.org> wrote:
>> It appears that there is a typo in the rk3288 TRM.  For
>> GRF_SOC_CON0[7] it says that 0 means "vepu" and 1 means "vdpu".  It's
>> the other way around.
>>
>> How do I know?  Here's my evidence:
>>
>> 1. Prior to commit 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec
>>    using the new muxgrf type on rk3288") we always pretended that we
>>    were using "aclk_vdpu" and the comment in the code said that this
>>    matched the default setting in the system.  In fact the default
>>    setting is 0 according to the TRM and according to reading memory
>>    at bootup.  In addition rk3288-based Chromebooks ran like this and
>>    the video codecs worked.
>> 2. With the existing clock code if you boot up and try to enable the
>>    new VIDEO_ROCKCHIP_VPU as a module (and without "clk_ignore_unused"
>>    on the command line), you get errors like "failed to get ack on
>>    domain 'pd_video', val=0x80208".  After flipping vepu/vdpu things
>>    init OK.
>> 3. If I export and add both the vepu and vdpu to the list of clocks
>>    for RK3288_PD_VIDEO I can get past the power domain errors, but now
>>    I freeze when the vpu_mmu gets initted.
>> 4. If I just mark the "vdpu" as IGNORE_UNUSED then everything boots up
>>    and probes OK showing that somehow the "vdpu" was important to keep
>>    enabled.  This is because we were actually using it as a parent.
>> 5. After this change I can hack "aclk_vcodec_pre" to parent from
>>    "aclk_vepu" using assigned-clocks and the video codec still probes
>>    OK.
>>
>> Fixes: 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec using the new muxgrf type on rk3288")
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>> I currently have no way to test the JPEG mem2mem driver, so hopefully
>> others can test this and make sure it's happy for them.  I'm just
>> happy not to get strange errors at boot anymore.
>>
>>  drivers/clk/rockchip/clk-rk3288.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> Any thoughts about this patch?  I'm 99.9% certain it's correct and
> it'd be nice to get it landed.  Heiko: I assume you're still
> collecting Rockchip clock patches and would be the one to apply it and
> (at some point) send a pull request to the clock tree?
>
> -Doug

This clk fix is needed to make MPEG-2 decoding work on my RK3288 Tinker Board using the
rockchip vpu patchset and a patch to add RK3288 specific MPEG-2 code [1].

Also note that the same change was suggested in a previous patch [2] from by ayaka.

If possible please also add the CLK_SET_RATE_PARENT change from [3] in a possible v2,
that fixes assigning the aclk_vcodec clk to 400Mhz in the rockchip vpu driver.

[1] https://github.com/Kwiboo/linux-rockchip/commit/1f78093e05c7360515a185f48b7c5cb8ba1e3e15
[2] https://patchwork.kernel.org/patch/9725553/
[3] https://github.com/Kwiboo/linux-rockchip/commit/9216da3f1521a0be5889235abe7fa093a4894160

Regards,
Jonas


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] clk: rockchip: Fix video codec clocks on rk3288
  2019-04-10 18:38   ` Jonas Karlman
@ 2019-04-10 23:37     ` Doug Anderson
  2019-04-11  1:17       ` elaine.zhang
  2019-04-11  6:38       ` Heiko Stübner
  0 siblings, 2 replies; 9+ messages in thread
From: Doug Anderson @ 2019-04-10 23:37 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Heiko Stuebner, Elaine Zhang, Tomasz Figa, Ziyuan Xu,
	Ezequiel Garcia, Ryan Case, Matthias Kaehlcke, Michael Turquette,
	Stephen Boyd, LKML, open list:ARM/Rockchip SoC...,
	linux-clk, Linux ARM

Hi,

On Wed, Apr 10, 2019 at 11:38 AM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> On 2019-04-10 17:45, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Mar 29, 2019 at 2:55 PM Douglas Anderson <dianders@chromium.org> wrote:
> >> It appears that there is a typo in the rk3288 TRM.  For
> >> GRF_SOC_CON0[7] it says that 0 means "vepu" and 1 means "vdpu".  It's
> >> the other way around.
> >>
> >> How do I know?  Here's my evidence:
> >>
> >> 1. Prior to commit 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec
> >>    using the new muxgrf type on rk3288") we always pretended that we
> >>    were using "aclk_vdpu" and the comment in the code said that this
> >>    matched the default setting in the system.  In fact the default
> >>    setting is 0 according to the TRM and according to reading memory
> >>    at bootup.  In addition rk3288-based Chromebooks ran like this and
> >>    the video codecs worked.
> >> 2. With the existing clock code if you boot up and try to enable the
> >>    new VIDEO_ROCKCHIP_VPU as a module (and without "clk_ignore_unused"
> >>    on the command line), you get errors like "failed to get ack on
> >>    domain 'pd_video', val=0x80208".  After flipping vepu/vdpu things
> >>    init OK.
> >> 3. If I export and add both the vepu and vdpu to the list of clocks
> >>    for RK3288_PD_VIDEO I can get past the power domain errors, but now
> >>    I freeze when the vpu_mmu gets initted.
> >> 4. If I just mark the "vdpu" as IGNORE_UNUSED then everything boots up
> >>    and probes OK showing that somehow the "vdpu" was important to keep
> >>    enabled.  This is because we were actually using it as a parent.
> >> 5. After this change I can hack "aclk_vcodec_pre" to parent from
> >>    "aclk_vepu" using assigned-clocks and the video codec still probes
> >>    OK.
> >>
> >> Fixes: 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec using the new muxgrf type on rk3288")
> >> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >> ---
> >> I currently have no way to test the JPEG mem2mem driver, so hopefully
> >> others can test this and make sure it's happy for them.  I'm just
> >> happy not to get strange errors at boot anymore.
> >>
> >>  drivers/clk/rockchip/clk-rk3288.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > Any thoughts about this patch?  I'm 99.9% certain it's correct and
> > it'd be nice to get it landed.  Heiko: I assume you're still
> > collecting Rockchip clock patches and would be the one to apply it and
> > (at some point) send a pull request to the clock tree?
> >
> > -Doug
>
> This clk fix is needed to make MPEG-2 decoding work on my RK3288 Tinker Board using the
> rockchip vpu patchset and a patch to add RK3288 specific MPEG-2 code [1].
>
> Also note that the same change was suggested in a previous patch [2] from by ayaka.
>
> If possible please also add the CLK_SET_RATE_PARENT change from [3] in a possible v2,
> that fixes assigning the aclk_vcodec clk to 400Mhz in the rockchip vpu driver.
>
> [1] https://github.com/Kwiboo/linux-rockchip/commit/1f78093e05c7360515a185f48b7c5cb8ba1e3e15
> [2] https://patchwork.kernel.org/patch/9725553/
> [3] https://github.com/Kwiboo/linux-rockchip/commit/9216da3f1521a0be5889235abe7fa093a4894160

Thanks for the pointers!  Now I'm 99.9999% certain that my patch is
correct instead of just 99.9%.  ;-)

IMO the CLK_SET_RATE_PARENT should probably be a separate patch but it
does seem like we need it.  ...and actually the patch adding it should
have a Fixes tag of the same commit.  Prior to that commit the parent
of "aclk_vcodec" was "aclk_vdpu".  As a gate clock it would
automatically have CLK_SET_RATE_PARENT so you could easily set the
rate.  ...and it looks as if the downstream video codec driver in
Chrome OS relies on this too.

I'm happy to add that in a v2 if Heiko is happy with it.

-Doug

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] clk: rockchip: Fix video codec clocks on rk3288
  2019-04-10 23:37     ` Doug Anderson
@ 2019-04-11  1:17       ` elaine.zhang
  2019-04-11  6:38       ` Heiko Stübner
  1 sibling, 0 replies; 9+ messages in thread
From: elaine.zhang @ 2019-04-11  1:17 UTC (permalink / raw)
  To: Doug Anderson, Jonas Karlman
  Cc: Heiko Stuebner, Tomasz Figa, Ziyuan Xu, Ezequiel Garcia,
	Ryan Case, Matthias Kaehlcke, Michael Turquette, Stephen Boyd,
	LKML, open list:ARM/Rockchip SoC...,
	linux-clk, Linux ARM

hi,

在 2019/4/11 上午7:37, Doug Anderson 写道:
> Hi,
>
> On Wed, Apr 10, 2019 at 11:38 AM Jonas Karlman <jonas@kwiboo.se> wrote:
>> On 2019-04-10 17:45, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Fri, Mar 29, 2019 at 2:55 PM Douglas Anderson <dianders@chromium.org> wrote:
>>>> It appears that there is a typo in the rk3288 TRM.  For
>>>> GRF_SOC_CON0[7] it says that 0 means "vepu" and 1 means "vdpu".  It's
>>>> the other way around.
>>>>
>>>> How do I know?  Here's my evidence:
>>>>
>>>> 1. Prior to commit 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec
>>>>     using the new muxgrf type on rk3288") we always pretended that we
>>>>     were using "aclk_vdpu" and the comment in the code said that this
>>>>     matched the default setting in the system.  In fact the default
>>>>     setting is 0 according to the TRM and according to reading memory
>>>>     at bootup.  In addition rk3288-based Chromebooks ran like this and
>>>>     the video codecs worked.
>>>> 2. With the existing clock code if you boot up and try to enable the
>>>>     new VIDEO_ROCKCHIP_VPU as a module (and without "clk_ignore_unused"
>>>>     on the command line), you get errors like "failed to get ack on
>>>>     domain 'pd_video', val=0x80208".  After flipping vepu/vdpu things
>>>>     init OK.
>>>> 3. If I export and add both the vepu and vdpu to the list of clocks
>>>>     for RK3288_PD_VIDEO I can get past the power domain errors, but now
>>>>     I freeze when the vpu_mmu gets initted.
>>>> 4. If I just mark the "vdpu" as IGNORE_UNUSED then everything boots up
>>>>     and probes OK showing that somehow the "vdpu" was important to keep
>>>>     enabled.  This is because we were actually using it as a parent.
>>>> 5. After this change I can hack "aclk_vcodec_pre" to parent from
>>>>     "aclk_vepu" using assigned-clocks and the video codec still probes
>>>>     OK.
>>>>
>>>> Fixes: 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec using the new muxgrf type on rk3288")
>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>> ---
>>>> I currently have no way to test the JPEG mem2mem driver, so hopefully
>>>> others can test this and make sure it's happy for them.  I'm just
>>>> happy not to get strange errors at boot anymore.
>>>>
>>>>   drivers/clk/rockchip/clk-rk3288.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>> Any thoughts about this patch?  I'm 99.9% certain it's correct and
>>> it'd be nice to get it landed.  Heiko: I assume you're still
>>> collecting Rockchip clock patches and would be the one to apply it and
>>> (at some point) send a pull request to the clock tree?
>>>
>>> -Doug
>> This clk fix is needed to make MPEG-2 decoding work on my RK3288 Tinker Board using the
>> rockchip vpu patchset and a patch to add RK3288 specific MPEG-2 code [1].
>>
>> Also note that the same change was suggested in a previous patch [2] from by ayaka.
>>
>> If possible please also add the CLK_SET_RATE_PARENT change from [3] in a possible v2,
>> that fixes assigning the aclk_vcodec clk to 400Mhz in the rockchip vpu driver.
>>
>> [1] https://github.com/Kwiboo/linux-rockchip/commit/1f78093e05c7360515a185f48b7c5cb8ba1e3e15
>> [2] https://patchwork.kernel.org/patch/9725553/
>> [3] https://github.com/Kwiboo/linux-rockchip/commit/9216da3f1521a0be5889235abe7fa093a4894160
> Thanks for the pointers!  Now I'm 99.9999% certain that my patch is
> correct instead of just 99.9%.  ;-)
>
> IMO the CLK_SET_RATE_PARENT should probably be a separate patch but it
> does seem like we need it.  ...and actually the patch adding it should
> have a Fixes tag of the same commit.  Prior to that commit the parent
> of "aclk_vcodec" was "aclk_vdpu".  As a gate clock it would
> automatically have CLK_SET_RATE_PARENT so you could easily set the
> rate.  ...and it looks as if the downstream video codec driver in
> Chrome OS relies on this too.
>
> I'm happy to add that in a v2 if Heiko is happy with it.

I also agree with this modification, which is completely correct. Thank 
you for your modification.

(We didn't push for this change because the  GRF_SOC_CON0[7] is setting 
in vcodec driver(RK mainline branch))

>
> -Doug
>
>
>



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] clk: rockchip: Fix video codec clocks on rk3288
  2019-04-10 23:37     ` Doug Anderson
  2019-04-11  1:17       ` elaine.zhang
@ 2019-04-11  6:38       ` Heiko Stübner
  1 sibling, 0 replies; 9+ messages in thread
From: Heiko Stübner @ 2019-04-11  6:38 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Jonas Karlman, Elaine Zhang, Tomasz Figa, Ziyuan Xu,
	Ezequiel Garcia, Ryan Case, Matthias Kaehlcke, Michael Turquette,
	Stephen Boyd, LKML, open list:ARM/Rockchip SoC...,
	linux-clk, Linux ARM

Hi Doug,

Am Donnerstag, 11. April 2019, 01:37:33 CEST schrieb Doug Anderson:
> Hi,
> 
> On Wed, Apr 10, 2019 at 11:38 AM Jonas Karlman <jonas@kwiboo.se> wrote:
> >
> > On 2019-04-10 17:45, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Fri, Mar 29, 2019 at 2:55 PM Douglas Anderson <dianders@chromium.org> wrote:
> > >> It appears that there is a typo in the rk3288 TRM.  For
> > >> GRF_SOC_CON0[7] it says that 0 means "vepu" and 1 means "vdpu".  It's
> > >> the other way around.
> > >>
> > >> How do I know?  Here's my evidence:
> > >>
> > >> 1. Prior to commit 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec
> > >>    using the new muxgrf type on rk3288") we always pretended that we
> > >>    were using "aclk_vdpu" and the comment in the code said that this
> > >>    matched the default setting in the system.  In fact the default
> > >>    setting is 0 according to the TRM and according to reading memory
> > >>    at bootup.  In addition rk3288-based Chromebooks ran like this and
> > >>    the video codecs worked.
> > >> 2. With the existing clock code if you boot up and try to enable the
> > >>    new VIDEO_ROCKCHIP_VPU as a module (and without "clk_ignore_unused"
> > >>    on the command line), you get errors like "failed to get ack on
> > >>    domain 'pd_video', val=0x80208".  After flipping vepu/vdpu things
> > >>    init OK.
> > >> 3. If I export and add both the vepu and vdpu to the list of clocks
> > >>    for RK3288_PD_VIDEO I can get past the power domain errors, but now
> > >>    I freeze when the vpu_mmu gets initted.
> > >> 4. If I just mark the "vdpu" as IGNORE_UNUSED then everything boots up
> > >>    and probes OK showing that somehow the "vdpu" was important to keep
> > >>    enabled.  This is because we were actually using it as a parent.
> > >> 5. After this change I can hack "aclk_vcodec_pre" to parent from
> > >>    "aclk_vepu" using assigned-clocks and the video codec still probes
> > >>    OK.
> > >>
> > >> Fixes: 4d3e84f99628 ("clk: rockchip: describe aclk_vcodec using the new muxgrf type on rk3288")
> > >> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > >> ---
> > >> I currently have no way to test the JPEG mem2mem driver, so hopefully
> > >> others can test this and make sure it's happy for them.  I'm just
> > >> happy not to get strange errors at boot anymore.
> > >>
> > >>  drivers/clk/rockchip/clk-rk3288.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > > Any thoughts about this patch?  I'm 99.9% certain it's correct and
> > > it'd be nice to get it landed.  Heiko: I assume you're still
> > > collecting Rockchip clock patches and would be the one to apply it and
> > > (at some point) send a pull request to the clock tree?
> > >
> > > -Doug
> >
> > This clk fix is needed to make MPEG-2 decoding work on my RK3288 Tinker Board using the
> > rockchip vpu patchset and a patch to add RK3288 specific MPEG-2 code [1].
> >
> > Also note that the same change was suggested in a previous patch [2] from by ayaka.
> >
> > If possible please also add the CLK_SET_RATE_PARENT change from [3] in a possible v2,
> > that fixes assigning the aclk_vcodec clk to 400Mhz in the rockchip vpu driver.
> >
> > [1] https://github.com/Kwiboo/linux-rockchip/commit/1f78093e05c7360515a185f48b7c5cb8ba1e3e15
> > [2] https://patchwork.kernel.org/patch/9725553/
> > [3] https://github.com/Kwiboo/linux-rockchip/commit/9216da3f1521a0be5889235abe7fa093a4894160
> 
> Thanks for the pointers!  Now I'm 99.9999% certain that my patch is
> correct instead of just 99.9%.  ;-)
> 
> IMO the CLK_SET_RATE_PARENT should probably be a separate patch but it
> does seem like we need it.  ...and actually the patch adding it should
> have a Fixes tag of the same commit.  Prior to that commit the parent
> of "aclk_vcodec" was "aclk_vdpu".  As a gate clock it would
> automatically have CLK_SET_RATE_PARENT so you could easily set the
> rate.  ...and it looks as if the downstream video codec driver in
> Chrome OS relies on this too.
> 
> I'm happy to add that in a v2 if Heiko is happy with it.

Sounds very sensible to me to combine these 2 parts.

Heiko



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-04-11  6:38 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-29 21:54 [PATCH] clk: rockchip: Fix video codec clocks on rk3288 Douglas Anderson
2019-03-29 23:28 ` Ezequiel Garcia
2019-03-30 14:24   ` Ezequiel Garcia
2019-03-30 18:35     ` Doug Anderson
2019-04-10 15:45 ` Doug Anderson
2019-04-10 18:38   ` Jonas Karlman
2019-04-10 23:37     ` Doug Anderson
2019-04-11  1:17       ` elaine.zhang
2019-04-11  6:38       ` Heiko Stübner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).