Re: [PATCH 2/2] clk: meson: meson8b: add the video decoder clock trees

Hi Maxime,

On Wed, Mar 20, 2019 at 12:16 PM Maxime Jourdan [off-list ref] wrote:

Hi Martin, thanks for looking into the video decoder for meson8!

you're welcome - this is only possible because of your work on the
video decoder driver!

On Tue, Mar 19, 2019 at 11:00 PM Martin Blumenstingl
[off-list ref] wrote:

quoted

This adds the four video decoder clock trees.

VDEC_1 is split into two paths on Meson8b and Meson8m2:
- input mux called "vdec_1_sel"
- two dividers ("vdec_1_1_div" and "vdec_1_2_div") and gates ("vdec_1_1"
  and "vdec_1_2")
- and an output mux (probably glitch-free) called "vdec_1"

Yes, all vdec clocks have a glitch-free mux to be able to safely
adjust the frequency on the fly, although in practice it's barely
used.

quoted

On Meson8 the VDEC_1 tree is simpler because there's only one path:
- input mux called "vdec_1_sel"
- divider ("vdec_1_1_div") and gate ("vdec_1_1")
- (the gate is used as output directly, there's no mux)

The VDEC_HCODEC and VDEC_2 clocks are simple composite clocks each
consisting of an input mux, divider and a gate.

The VDEC_HEVC clock seems to have two paths similar to the VDEC_1 clock.
However, the register offsets of the second clock path is not known.
Amlogic's 3.10 kernel (which is used as reference) sets
HHI_VDEC2_CLK_CNTL[31] to 1 before changing the VDEC_HEVC clock and back
to 0 afterwards. For now, leave a TODO comment and only add the first
path.

Looking at aml-3.10/drivers/amlogic/amports/m8b/vdec_clk.c, it's weird
indeed. They seem to copy the divider's value to the same place
(HHI_VDEC2_CLK_CNTL[16~23]), and the only thing that stands out is
enabling HHI_VDEC2_CLK_CNTL[31].

Then again they don't make use of that codepath at all, so who knows..

indeed, that's why I skipped it for now

[...]

quoted

diff --git a/drivers/clk/meson/meson8b.h b/drivers/clk/meson/meson8b.h
index e775f91ccce9..ed37196187e6 100644
--- a/drivers/clk/meson/meson8b.h
+++ b/drivers/clk/meson/meson8b.h

@@ -37,6 +37,9 @@
 #define HHI_MALI_CLK_CNTL              0x1b0 /* 0x6c offset in data sheet */
 #define HHI_VPU_CLK_CNTL               0x1bc /* 0x6f offset in data sheet */
 #define HHI_HDMI_CLK_CNTL              0x1cc /* 0x73 offset in data sheet */
+#define HHI_VDEC_CLK_CNTL              0x1e0 /* 0x78 offset in data sheet */
+#define HHI_VDEC2_CLK_CNTL             0x1e4 /* 0x79 offset in data sheet */
+#define HHI_VDEC3_CLK_CNTL             0x1e8 /* 0x7a offset in data sheet */
 #define HHI_NAND_CLK_CNTL              0x25c /* 0x97 offset in data sheet */
 #define HHI_MPLL_CNTL                  0x280 /* 0xa0 offset in data sheet */
 #define HHI_SYS_PLL_CNTL               0x300 /* 0xc0 offset in data sheet */

@@ -156,8 +159,20 @@
 #define CLKID_VPU_1_SEL                186
 #define CLKID_VPU_1_DIV                187
 #define CLKID_VPU_1            189
+#define CLKID_VDEC_1_SEL       191
+#define CLKID_VDEC_1_1_DIV     192
+#define CLKID_VDEC_1_1         193
+#define CLKID_VDEC_1_2_DIV     194
+#define CLKID_VDEC_1_2         195

In order to make use of the glitch-free mux by the driver, shouldn't
CLKID_VDEC_1_1 and CLKID_VDEC_1_2 be exported in the bindings ?

I considered this but I didn't export them because of three reasons:
- the video decoder driver doesn't have any logic to use the glitch-free mux yet
- assigned-clock-rates or clk_set_rate will figure out the parent
setup on it's own if we ignore the glitch-free mux until the video
decoder driver supports it
- exporting CLKIDs is easier than un-exporting them

Regards
Martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel