linux-kernel - Re: [PATCH] clk: renesas: rcar-gen4: implement SDSRC properly

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 28 Jun 2022 14:04:38 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Wolfram Sang <wsa+renesas@...g-engineering.com>
Cc:     Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        linux-clk <linux-clk@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] clk: renesas: rcar-gen4: implement SDSRC properly

Hi Wolfram,

On Tue, Jun 28, 2022 at 1:08 PM Wolfram Sang
<wsa+renesas@...g-engineering.com> wrote:
> > > Tested on my Spider board (r8a779f0). Only build tested for r8a779g0 but
> > > the docs for the registers are the same.
> >
> > While the SDSRCSEL bits are the same, the register at offset 0x8a4 is
> > called SD0CKCR1 on R-Car S4-8, and CKSRCSELCR on R-Car V4H.
> > I guess that is why you removed the definition of SD0CKCR1, and stored
> > the register offset in DEF_GEN4_SDSRC(), despite both being the same?
>
> TBH, no :) I did that to be future proof in case the register gets moved
> somewhere else. Also, this is consistent how we did it with DEF_GEN3_SD.

In case of DEF_GEN3_SD(), we have a register offset parameter because
most affected SoCs have multiple SDHI instances, and multiple registers
to control their clocks.

> > >         case CLK_TYPE_GEN4_SDSRC:
> > > -               div = ((readl(base + SD0CKCR1) >> 29) & 0x03) + 4;
> > > +               value = (readl(base + core->offset) >> 29) & 3;
> > > +               if (value) {
> > > +                       div = value + 4;
> > > +               } else {
> > > +                       parent = clks[core->parent >> 16];
> > > +                       if (IS_ERR(parent))
> > > +                               return ERR_CAST(parent);
> > > +                       div = 2;
> > > +               }
> >
> > So this gives the exact same divider of PLL5 before.
> >
> > The clock diagram indeed shows different paths for value 0
> > (PLL5 -> 1/2 -> 1/2) and values 1 and 2 (PLL5 -> {1/5 or 1/6}).
> > But the textual description for SDSRC says "The SDSRC divider divides
> > PLL5 output clock", matching the original code.
> >
> > Do we have to complicate the code? ;-)
> > I guess the clock diagram was based on the diagram for R-Car H3
> > (which has two daisy-chained fixed 1/2 dividers), with the new 1/5
> > and 1/6 dividers added.
>
> We don't have to complicate the code unnecessarily. If you think the
> diagram is flawed, then we can keep the current code. I changed the code
> because I was confused when checking 'clk_summary' with the diagram and
> wanted to make it proper to reduce my confusion.
>
> My patches to enable eMMC on Spider have a significantly lower
> throughput than the BSP, so this was the first step of trying to verify
> things and get the clocks in shape.
>
> If you call it superfluous, then we can drop it. No hard feelings here.

OK, so let's drop this.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-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