Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201029105515.16309-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <CAMuHMdVGO+DEgsTr62nA+egU2etZA_vwE9GrOG1JPWBvv90UXg@mail.gmail.com> <CA+V-a8sagMFjaxxVUGMeqG5wdAWwT=oqOPHgTBZ7j0hPP1LcNw@mail.gmail.com>
In-Reply-To: <CA+V-a8sagMFjaxxVUGMeqG5wdAWwT=oqOPHgTBZ7j0hPP1LcNw@mail.gmail.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Fri, 30 Oct 2020 11:48:19 +0100
Message-ID: <CAMuHMdX_x9YkUv3DCZxW_sydX1NiQ=5EJjsgo9yQ7pZV-xC_+g@mail.gmail.com>
Subject: Re: [PATCH v2] clk: renesas: r8a774c0: Add RPC clocks
To:     "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Cc:     Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
        linux-clk <linux-clk@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Biju Das <biju.das.jz@bp.renesas.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Prabhakar,

On Fri, Oct 30, 2020 at 11:13 AM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Thu, Oct 29, 2020 at 2:29 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Oct 29, 2020 at 11:55 AM Lad Prabhakar
> > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > Describe the RPCSRC internal clock and the RPC[D2] clocks derived from it,
> > > as well as the RPC-IF module clock, in the RZ/G2E (R8A774C0) CPG/MSSR
> > > driver.
> >
> > Thanks for your patch!
> >
> > > Add new clk type CLK_TYPE_GEN3E3_RPCSRC to handle registering rpcsrc
> > > clock as the source for RPCSRC can be either PLL0/PLL1 and this depends
> > > on MD[1:4] pins where as compared to other R-Car Gen3 SoC's the RPCSRC
> > > clock source is always PLL1.
> > >
> > > MD[4] MD[3] MD[2] MD[1]
> > >   0     0     0    1     -> RPCSRC CLK source is PLL1
> > >   0     0     1    1     -> RPCSRC CLK source is PLL1
> > >   0     1     0    0     -> RPCSRC CLK source is PLL1
> > >   1     0     1    1     -> RPCSRC CLK source is PLL1
> > >   x     x     x    x     -> For any other values RPCSRC CLK source is PLL0
> >
> > AFAIU, the _initial values_ of the RPCCKCR bits depend on the MD pins.
> > They can still be changed at run-time, and might have been changed by
> > the bootloader before transferring control to Linux.
> >
> > > R-Car Gen3 manual Rev.2.20 has in-correct information related to
> > > determining the clock source for RPCSRC.
> >
> > Which part of the information is not correct?
> > Where can I find corrected information?
> > Is my understanding above incorrect, too?
> >
> R-Car Gen3 HW manual mentions the below statement (page 529, Rev.2.20 manual):
> [R-Car E3]
> When (MD4, MD3, MD2, MD1) = (0, 0, 0, 1) or (0, 1, 0, 0): DIV[2:0] =
> 011, DIV[4:3] = 00 (300 MHz PLL0)

That indeed doesn't match the values in the DIV[4:0] bits description.

> Confirming with internal team this should be below:
>
> When (MD4, MD3, MD2, MD1) = (0, 0, 0, 1) or (0, 1, 0, 0): DIV[2:0] =
> 011, DIV[4:3] = 00 (80 MHz PLL1)
>
> This should be fixed in the next version of the document, and when
> available I'll ask Chris P to send it across.

OK, that does match the bits.

> > > @@ -696,6 +717,22 @@ struct clk * __init rcar_gen3_cpg_clk_register(struct device *dev,
> > >                                                   cpg_rpcsrc_div_table,
> > >                                                   &cpg_lock);
> > >
> > > +       case CLK_TYPE_GEN3E3_RPCSRC:
> > > +               e3_rpcsrc_parent = cpg_rpcsrc_e3_get_parent(cpg_mode);
> >
> > This is not correct if the boot loader has changed the parent clock.
> >
> You mean by manually togelling the MD pins before we get into Linux ?

No, by writing to the RPCCKCR register.
Remember, the _initial_ values are determined by the MD pins.
They can still be changed.

E.g. on R-Car D3, I verified that changing PLL0CR.CKSEL at runtime
does work.  In the end, we decided to just look at MD12 instead (IIRC
because the CKSEL bit was removed from later documentation, but
Rev 2.20 documents it again ;-)

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