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] [thread-next>] [day] [month] [year] [list]

Message-ID: <YrrhO+kb5d2rtTNA@shikoro>
Date:   Tue, 28 Jun 2022 13:08:43 +0200
From:   Wolfram Sang <wsa+renesas@...g-engineering.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
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 Geert,

> > 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.

> >         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.

All the best,

   Wolfram


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)