Re: [PATCH 2/2] clk: renesas: rcar-gen3: Add ZG clock divider support

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Magnus Damm <magnus.damm@gmail.com>,
	Takeshi Kihara <takeshi.kihara.df@renesas.com>
Subject: Re: [PATCH 2/2] clk: renesas: rcar-gen3: Add ZG clock divider support
Date: Wed, 17 Jan 2018 17:12:03 +0100	[thread overview]
Message-ID: <CAMuHMdU5s0eacQEWT7SZK2=wOZiRRUx7H15L1d=cK5ocjw745Q@mail.gmail.com> (raw)
In-Reply-To: <1516203716-4606-3-git-send-email-ulrich.hecht+renesas@gmail.com>

Hi Uli,

On Wed, Jan 17, 2018 at 4:41 PM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> This patch adds ZG clock divider support for R-Car Gen3 SoC.
>
> Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com>
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> @@ -76,6 +76,7 @@ static void cpg_simple_notifier_register(struct raw_notifier_head *notifiers,
>  #define CPG_FRQCRC                     0x000000e0
>  #define CPG_FRQCRC_ZFC_MASK            GENMASK(12, 8)
>  #define CPG_FRQCRC_Z2FC_MASK           GENMASK(4, 0)
> +#define CPG_FRQCRC_ZGFC_MASK           GENMASK(28, 24)

According to the BSP, these bits are in FRQCRB, not FRQCRC?

Note that the R-Car Gen3 Hardware User's Manual does not document these
bits, and declares them read-only, always zero...

> @@ -563,6 +564,10 @@ struct clk * __init rcar_gen3_cpg_clk_register(struct device *dev,
>                 return cpg_z_clk_register(core->name, __clk_get_name(parent),
>                                           base, CPG_FRQCRC_Z2FC_MASK);
>
> +       case CLK_TYPE_GEN3_ZG:
> +               return cpg_z_clk_register(core->name, __clk_get_name(parent),
> +                                         base, CPG_FRQCRC_ZGFC_MASK);

I don't think you can do it like that.
ZG uses FRQCRB instead of FRQCRC, and uses a fixed divider of 4 instead of 2.
While the rest of the handling is similar, both differences need to be taken
into account.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds