[PATCH] clk: rockchip: add clock ID for CIF0/1 on RK3066

[PATCH] clk: rockchip: add clock ID for CIF0/1 on RK3066

[Val Packett]

RK3066 does have two "CIF" video capture interface blocks, add their
corresponding clock IDs so that they could be used.

Signed-off-by: Val Packett <val@packett.cool>
---
 drivers/clk/rockchip/clk-rk3188.c             | 4 ++--
 include/dt-bindings/clock/rk3188-cru-common.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3188.c b/drivers/clk/rockchip/clk-rk3188.c
index 684233e72105..81e94b338d0f 100644
--- a/drivers/clk/rockchip/clk-rk3188.c
+++ b/drivers/clk/rockchip/clk-rk3188.c
@@ -344,7 +344,7 @@ static struct rockchip_clk_branch common_clk_branches[] __initdata = {
 
 	GATE(0, "pclkin_cif0", "ext_cif0", 0,
 			RK2928_CLKGATE_CON(3), 3, GFLAGS),
-	INVERTER(0, "pclk_cif0", "pclkin_cif0",
+	INVERTER(PCLK_CIF0, "pclk_cif0", "pclkin_cif0",
 			RK2928_CLKSEL_CON(30), 8, IFLAGS),
 
 	FACTOR(0, "xin12m", "xin24m", 0, 1, 2),
@@ -602,7 +602,7 @@ static struct rockchip_clk_branch rk3066a_clk_branches[] __initdata = {
 
 	GATE(0, "pclkin_cif1", "ext_cif1", 0,
 			RK2928_CLKGATE_CON(3), 4, GFLAGS),
-	INVERTER(0, "pclk_cif1", "pclkin_cif1",
+	INVERTER(PCLK_CIF1, "pclk_cif1", "pclkin_cif1",
 			RK2928_CLKSEL_CON(30), 12, IFLAGS),
 
 	COMPOSITE(0, "aclk_gpu_src", mux_pll_src_cpll_gpll_p, 0,
diff --git a/include/dt-bindings/clock/rk3188-cru-common.h b/include/dt-bindings/clock/rk3188-cru-common.h
index 01e14ab252a7..dd988cc9d582 100644
--- a/include/dt-bindings/clock/rk3188-cru-common.h
+++ b/include/dt-bindings/clock/rk3188-cru-common.h
@@ -103,6 +103,8 @@
 #define PCLK_PERI		351
 #define PCLK_DDRUPCTL		352
 #define PCLK_PUBL		353
+#define PCLK_CIF0		354
+#define PCLK_CIF1		355
 
 /* hclk gates */
 #define HCLK_SDMMC		448
-- 
2.47.1

Re: [PATCH] clk: rockchip: add clock ID for CIF0/1 on RK3066

[Heiko Stübner]

Hi Val,

Am Donnerstag, 5. Dezember 2024, 06:50:46 CET schrieb Val Packett:
> RK3066 does have two "CIF" video capture interface blocks, add their
> corresponding clock IDs so that they could be used.
> 
> Signed-off-by: Val Packett <val@packett.cool>

please split this into two patches
- addition of the clock-ids to the dt-binding header
- setting the clock-ids in the clock driver

Thanks
Heiko

> ---
>  drivers/clk/rockchip/clk-rk3188.c             | 4 ++--
>  include/dt-bindings/clock/rk3188-cru-common.h | 2 ++
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/rockchip/clk-rk3188.c b/drivers/clk/rockchip/clk-rk3188.c
> index 684233e72105..81e94b338d0f 100644
> --- a/drivers/clk/rockchip/clk-rk3188.c
> +++ b/drivers/clk/rockchip/clk-rk3188.c
> @@ -344,7 +344,7 @@ static struct rockchip_clk_branch common_clk_branches[] __initdata = {
>  
>  	GATE(0, "pclkin_cif0", "ext_cif0", 0,
>  			RK2928_CLKGATE_CON(3), 3, GFLAGS),
> -	INVERTER(0, "pclk_cif0", "pclkin_cif0",
> +	INVERTER(PCLK_CIF0, "pclk_cif0", "pclkin_cif0",
>  			RK2928_CLKSEL_CON(30), 8, IFLAGS),
>  
>  	FACTOR(0, "xin12m", "xin24m", 0, 1, 2),
> @@ -602,7 +602,7 @@ static struct rockchip_clk_branch rk3066a_clk_branches[] __initdata = {
>  
>  	GATE(0, "pclkin_cif1", "ext_cif1", 0,
>  			RK2928_CLKGATE_CON(3), 4, GFLAGS),
> -	INVERTER(0, "pclk_cif1", "pclkin_cif1",
> +	INVERTER(PCLK_CIF1, "pclk_cif1", "pclkin_cif1",
>  			RK2928_CLKSEL_CON(30), 12, IFLAGS),
>  
>  	COMPOSITE(0, "aclk_gpu_src", mux_pll_src_cpll_gpll_p, 0,
> diff --git a/include/dt-bindings/clock/rk3188-cru-common.h b/include/dt-bindings/clock/rk3188-cru-common.h
> index 01e14ab252a7..dd988cc9d582 100644
> --- a/include/dt-bindings/clock/rk3188-cru-common.h
> +++ b/include/dt-bindings/clock/rk3188-cru-common.h
> @@ -103,6 +103,8 @@
>  #define PCLK_PERI		351
>  #define PCLK_DDRUPCTL		352
>  #define PCLK_PUBL		353
> +#define PCLK_CIF0		354
> +#define PCLK_CIF1		355
>  
>  /* hclk gates */
>  #define HCLK_SDMMC		448
> 

Re: [PATCH] clk: rockchip: add clock ID for CIF0/1 on RK3066

[Krzysztof Kozlowski]

On Thu, Dec 05, 2024 at 02:50:46AM -0300, Val Packett wrote:
> RK3066 does have two "CIF" video capture interface blocks, add their
> corresponding clock IDs so that they could be used.
> 
> Signed-off-by: Val Packett <val@packett.cool>
> ---
>  drivers/clk/rockchip/clk-rk3188.c             | 4 ++--
>  include/dt-bindings/clock/rk3188-cru-common.h | 2 ++
>  2 files changed, 4 insertions(+), 2 deletions(-)

Please run scripts/checkpatch.pl and fix reported warnings. Then please
run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings.
Some warnings can be ignored, especially from --strict run, but the code
here looks like it needs a fix. Feel free to get in touch if the warning
is not clear.

Best regards,
Krzysztof

Re: [PATCH] clk: rockchip: add clock ID for CIF0/1 on RK3066

[Heiko Stübner]

Hi Krzysztof,

Am Donnerstag, 5. Dezember 2024, 11:25:28 CET schrieb Krzysztof Kozlowski:
> On Thu, Dec 05, 2024 at 02:50:46AM -0300, Val Packett wrote:
> > RK3066 does have two "CIF" video capture interface blocks, add their
> > corresponding clock IDs so that they could be used.
> > 
> > Signed-off-by: Val Packett <val@packett.cool>
> > ---
> >  drivers/clk/rockchip/clk-rk3188.c             | 4 ++--
> >  include/dt-bindings/clock/rk3188-cru-common.h | 2 ++
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> Please run scripts/checkpatch.pl and fix reported warnings. Then please
> run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings.
> Some warnings can be ignored, especially from --strict run, but the code
> here looks like it needs a fix. Feel free to get in touch if the warning
> is not clear.

I guess you're taking a shot of the indentation?

Though that is an intentional deviation, for the long lists of
clock-declarations, that is more a spreadsheet than actual code.

To give this some context, an excerpt from the rk3188 clock driver:

        COMPOSITE(0, "aclk_lcdc1_pre", mux_pll_src_cpll_gpll_p, 0,
                        RK2928_CLKSEL_CON(31), 15, 1, MFLAGS, 8, 5, DFLAGS,
                        RK2928_CLKGATE_CON(1), 4, GFLAGS),
        GATE(ACLK_PERI, "aclk_peri", "aclk_peri_pre", 0,
                        RK2928_CLKGATE_CON(2), 1, GFLAGS),
        COMPOSITE_NOMUX(HCLK_PERI, "hclk_peri", "aclk_peri_pre", 0,
                        RK2928_CLKSEL_CON(10), 8, 2, DFLAGS | CLK_DIVIDER_POWER_OF_TWO,
                        RK2928_CLKGATE_CON(2), 2, GFLAGS),
        MUX(0, "cif_src", mux_pll_src_cpll_gpll_p, 0,
                        RK2928_CLKSEL_CON(29), 0, 1, MFLAGS),

Rockchip clocks most of the time consist of a mux+divider in one register
and a gate-bit in another. Most clocks are of the COMPOSITE-type above.
Surprisingly this is true since before 2013 - and even today :-) .

So the notation is
      TYPE(id, name, parent-name(s), main-clk-flags,
	MUXDIV-reg, muxoffset, muxwidth, muxflags, divoffset, divwidth, divflags,
	GATE-reg, gatebit, gateflags)

Having all these elements keep their relative position makes it way easier
on the eyes, compared to if they followed that opening parenthesis of each
individual line or maybe reflowing of the elements.

The only real change the clock definitions will see are fixes to wrong
register numbers or wrong bits (or missing clock-ids), so being able to
see check those easily is just nice to have.


Heiko

Re: [PATCH] clk: rockchip: add clock ID for CIF0/1 on RK3066

[Krzysztof Kozlowski]

On 05/12/2024 13:29, Heiko Stübner wrote:
> Hi Krzysztof,
> 
> Am Donnerstag, 5. Dezember 2024, 11:25:28 CET schrieb Krzysztof Kozlowski:
>> On Thu, Dec 05, 2024 at 02:50:46AM -0300, Val Packett wrote:
>>> RK3066 does have two "CIF" video capture interface blocks, add their
>>> corresponding clock IDs so that they could be used.
>>>
>>> Signed-off-by: Val Packett <val@packett.cool>
>>> ---
>>>  drivers/clk/rockchip/clk-rk3188.c             | 4 ++--
>>>  include/dt-bindings/clock/rk3188-cru-common.h | 2 ++
>>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> Please run scripts/checkpatch.pl and fix reported warnings. Then please
>> run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings.
>> Some warnings can be ignored, especially from --strict run, but the code
>> here looks like it needs a fix. Feel free to get in touch if the warning
>> is not clear.
> 
> I guess you're taking a shot of the indentation?

No, there is a big, fat warning about bindings.

Best regards,
Krzysztof