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

[Stephen Boyd <sboyd@kernel.org>]

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@80930000 {
>   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.