| 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
| ||
|
Message-ID: <CAMuHMdVWPzbN7C05H5v-pjaohJJ1uprmPmWjcY61hsVk=_35Ow@mail.gmail.com>
Date: Thu, 24 Apr 2025 10:24:27 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Thierry Bultel <thierry.bultel.yh@...renesas.com>
Cc: "thierry.bultel@...atsea.fr" <thierry.bultel@...atsea.fr>,
"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>,
Paul Barker <paul.barker.ct@...renesas.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>
Subject: Re: [PATCH v7 06/13] clk: renesas: Add support for R9A09G077 SoC
Hi Thierry,
On Thu, 24 Apr 2025 at 09:53, Thierry Bultel
<thierry.bultel.yh@...renesas.com> wrote:
> > From: Geert Uytterhoeven <geert@...ux-m68k.org>
> > On Wed, 23 Apr 2025 at 09:36, Thierry Bultel
> > <thierry.bultel.yh@...renesas.com> wrote:
> > > > From: Geert Uytterhoeven <geert@...ux-m68k.org> On Fri, 18 Apr 2025
> > > > at 23:22, Thierry Bultel <thierry.bultel.yh@...renesas.com> wrote:
> > > > > +};
> > > > > > > +
> > > > > > > +static const struct mssr_mod_clk r9a09g077_mod_clks[]
> > > > > > > +__initconst =
> > > > {
> > > > > > > + DEF_MOD("sci0", 108, R9A09G077_PCLKM),
> > > > > >
> > > > > > Shouldn't that be 8 instead of 108?
> > > > > > Using R9A09G077_PCLKM as the parent is a temporary
> > > > > > simplification,
> > > > right?
> > > > >
> > > > > I am probably missing something, isn’t PCKML actually the parent
> > clock ?
> > > >
> > > > According to Figure 7.1 ("Block diagram of clock generation
> > > > circuit"), it is PCLKSCI0, which can be switched to PCLKM. I guess
> > > > that is the default, hence my "temporary simplification" question.
> > > >
> > > > As the actual switching is controlled through the SCI's CCR3
> > > > register, the SCI block should have two clock inputs in DT (PCLKM
> > > > and PCLKSCIn), and thus the DT bindings should be amended. See also
> > > > Figure 33.1 ("SCI block diagram").
> > > >
> > >
> > > Thanks for clarifying.
> > > Indeed, this is the default setting (and the one we have at this stage).
> > > I think that support for PCLKSCIn can be added at the time we support
> > > baudrate setting.
> >
> > I am not sure we can do that in a clean backwards-compatible way.
> > Currently the DT bindings describe a single clock:
> >
> > clock-names:
> > const: fck # UART functional clock
> >
> > The documentation calls the two clocks:
> > - Bus clock (PCLKM),
> > - Operation clock (PCLKSCIn).
> >
> > Which one is the functional clock? I'd say the latter...
> > Currently, DT says:
> >
> > clocks = <&cpg CPG_MOD 8>;
> > clock-names = "fck";
> >
> > and the clock driver uses PCLKM as the module's parent clock, I think you
> > will have a very hard time to synchronize all of the clock driver, sci
> > driver, and DTS when transitioning to something like:
> >
> > clocks = <&cpg CPG_MOD 8>, <&cpgR9A09G077_PCLKM>;
> > clock-names = "fck", "bus";
> >
> > where the modulo clock has to become PCLKSCIn (actually SCInASYNC, as seen
> > from the CPG).
> >
> > Does that make sense, or am I missing something?
>
> You are right, I completely understand how hard it would be to have backward compatibility.
> However, doing so:
>
> clocks = <&cpg CPG_MOD R9A09G077_PCLK_SCI0>, <&cpg CPG_CORE R9A09G077_CLK_PCLKM>;
> clock-names = "fck", "bus";
>
> without modifying the sh-sci driver (yet) would lead to this bogus clk_summary:
>
> clock count count count rate accuracy phase cycle enable consumer id
> ---------------------------------------------------------------------------------------------------------------------------------------------
> loco 0 0 0 1000000 0 0 50000 Y deviceless no_connection_id
> extal 1 1 0 25000000 0 0 50000 Y clock-controller@...80000 extal
> deviceless no_connection_id
> .pll4 1 1 0 2400000000 0 0 50000 Y deviceless no_connection_id
> .sel_pll4 1 1 0 2400000000 0 0 50000 Y deviceless no_connection_id
> .sel_clk_pll4 1 1 0 2400000000 0 0 50000 Y deviceless no_connection_id
> .pll4d1 1 1 0 2400000000 0 0 50000 Y deviceless no_connection_id
> .sci0async 1 1 0 100000000 0 0 50000 Y deviceless no_connection_id
> sci0 2 2 0 100000000 0 0 50000 Y 80005000.serial fck
> deviceless of_clk_get_from_provider
> deviceless no_connection_id
>
> it is wrong because the actual default state is that PCKLM is used, not sci0async.
> Having PCKML consumed by sci0 is an obvious fix in sci_init_clocks, but it won't show up that only one clock is used at a time.
So your rsci patch should modify the sh-sci driver to handle this correctly...
> Couldn't it be better be solved by introducing an extra mux clock ? (the one controlled by BPEN) ?
I don't think that will simplify anything: as the BPEN bit is located
inside the RSCI register space, that mux clock must be handled by the
sh-sci/rsci driver itself, so you cannot just refer to it in the clocks
property in DT.
In theory, several other control bits (e.g. BGDM (Baud Rate Generator
Double-Speed Mode Select), ABCS (Asynchronous Mode Base Clock Select),
and ABCSE (Asynchronous Mode Extended Base Clock Select) in CCR2)
could also be modelled using the common clock framework, but that
wouldn't simplify the serial driver...
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
Powered by blists - more mailing lists