linux-kernel - Re: [PATCH 12/43] clk: ep93xx: add DT support for Cirrus EP93xx

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 for Android: free password hash cracker in your pocket

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

Message-ID: <d47d0ceb834ca56a4733e226e89a4f2b.sboyd@kernel.org>
Date:   Tue, 13 Jun 2023 13:44:49 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Nikita Shubin <nikita.shubin@...uefel.me>
Cc:     Arnd Bergmann <arnd@...nel.org>, Linus Walleij <linusw@...nel.org>,
        Alexander Sverdlin <alexander.sverdlin@...il.com>,
        Michael Turquette <mturquette@...libre.com>,
        linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org
Subject: Re: [PATCH 12/43] clk: ep93xx: add DT support for Cirrus EP93xx

Quoting Nikita Shubin (2023-05-15 06:31:41)
> Hello Stephen!
> 
> Thank you for your review.
> 
> Acknowledged all remarks, however, i didn't fully understood some of
> the requirements:

When the reply is taken out of context it is harder for me to recall
what we're talking about.

> 
> > And maybe this can be registered from wherever the regmap is created?
> 
> Are you suggesting that all clock from init, i.e. "pll1", "pll2",
> "hclk", "fclk", "pclk"
> 
> Should be moved to SoC driver instead:
> 
> https://lore.kernel.org/all/20230424123522.18302-3-nikita.shubin@maquefel.me/
> 
> ?

Sure? That looks like a possibility.

> 
> > Does some interrupt or timer driver use dma? Why are these registered
> > early?
> 
> ep93xx_dma uses subsys_initcall(ep93xx_dma_module_init)
> 
> https://elixir.bootlin.com/linux/v6.3.2/source/drivers/dma/ep93xx_dma.c#L1430
> 
> I can move clk to using arch_initcall and move all stuff to probe, i
> think...

Sounds like the answer to my question is no. In which case moving to
arch_initcall() or probe will work. Please try.

> 
> > Why is this in arm/ directory? Isn't it a clock controller?
> 
> ep93xx clocks, reboot, pinctrl use syscon regmap and some special
> functions from SoC, i.e. "Syscon Software Lock Register" - so we are
> able to write to some registers only after writing some magik value
> there.
> 
> So all above use regmap from SoC.

There can be an API in a header located in include/soc/ that implements
the magik value unlock sequence?

> 
> Should make a separate clock controller like one i did for pinctrl ?
> Then it should look like:
> 
> syscon@...30000 {
>   compatible = "cirrus,ep9301-syscon",
>                "syscon", "simple-mfd";
>   reg = <0x80930000 0x1000>;
>   #reset-cells = <1>;
>       
>   clocks {
>     xtali: oscillator {
>       compatible = "fixed-clock";
>       #clock-cells = <0>;
>       clock-frequency = <14745600>;
>     };
>   };
>       
>   clock-controller {
>     #clock-cells = <1>;
>     compatible = "cirrus,ep9301-clk";
>     clocks = <&xtali>;
>   };
> };
> 
> Or even simply:
> 
> clocks {
>   xtali: oscillator {
>     compatible = "fixed-clock";
>     #clock-cells = <0>;
>     clock-frequency = <14745600>;
>   };
> };
>       
> clock-controller {
>   #clock-cells = <1>;
>   compatible = "cirrus,ep9301-clk";
>   clocks = <&xtali>;
> };

The DT binding shouldn't be making sub-nodes to match driver structure
of the kernel. Instead, there should be a node for a register range that
represents a device on the bus. That device may be multipurpose, in
which case it can probe as a platform driver and that platform driver
can create some number of auxiliary bus devices for the sub
functionality of the device. We shouldn't be prescribing Linux software
constructs onto the DT binding.