Re: [PATCH 01/12] dt-bindings: clock: mediatek: document clk bindings for mediatek mt7986 SoC

[Chen-Yu Tsai]

On Fri, Aug 13, 2021 at 1:22 PM Sam Shih <sam.shih@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> Sorry for the late reply,I have prepared v2 patches, but for some of your suggestions, I think
> it is a bit difficult to apply to our drivers.
>
>
> On Fri, 2021-07-30 at 14:30 +0800, Chen-Yu Tsai wrote:
> > On Fri, Jul 30, 2021 at 2:01 PM Sam Shih <sam.shih@xxxxxxxxxxxx>
> > wrote:
> > >
> > > Hi,
> > >
> > > On Mon, 2021-07-26 at 17:20 +0800, Chen-Yu Tsai wrote:
> > > > Furthermore, based on the driver patch and the fact that they
> > > > share
> > > > the
> > > > same compatible string, it seems you shouldn't need to have two
> > > > compatible
> > > > strings for two identical hardware blocks. The need for separate
> > > > entries
> > > > to have different clock names is an implementation detail. Please
> > > > consider
> > > > using and supporting clock-output-names.
> > > >
> > > > Also, please check out the MT8195 clock driver series [1]. I'm
> > > > guessing
> > > > a lot of the comments apply to this one as well.
> > > >
> > > > Regards
> > > > ChenYu
> > > >
> > > > [1]
> > > >
> https://urldefense.com/v3/__https://lore.kernel.org/linux-mediatek/20210616224743.5109-1-chun-jie.chen@xxxxxxxxxxxx/T/*t__;Iw!!CTRNKA9wMg0ARbw!29pb4TJiGHLvLbYJgDB2Dhf8Mpw5VU8zV-W3NrMan_RPQrtWT2EdRTyyjWpu0nZE$
> > > >
> > > >
> > >
> > > I have organized your comments in "Mediatek MT8195 clock support"
> > > series into the following list, reply to your here:
> > >
> > > > dt-binding: Move the not-to-be-exposed clock to driver directory
> > > > or
> > > > simply left out
> > >
> > > Okay, thanks for your comment, I will update this in the next patch
> > > set
> >
> > See the following file for an example:
> >
> >
>
> > https://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/drivers/clk/sunxi-ng/ccu-sun50i-a64.h__;!!CTRNKA9wMg0ARbw!3--UEpFFGeZ_WTCaWsj_9vbbb4SSE1vPCILFleThzpa1nq7mveFfQMDqbacdT5I6$
> >
> > drivers would not be able to use the non-exported intermediate
> > clocks.
> >
> Thanks, I well delete some clocks that are not expected to be use in
> device tree.
>
> > > > describe some of the clock relations between the various clock
> > > > controllers
> > >
> > > I have checked the files in
> > > "Documentation/devicetree/bindings/arm/mediatek", It seems that all
> > > MediaTek SoC clocks are simply described by each controller, like
> > > "mediatek,infracfg.txt" and "mediatek,topckgen.txt", and those
> > > document
> > > only include compatible strings information and examples.
> > > Can we insert the clock relationship of MT7986 directlly in common
> > > documents?
> >
> > What I meant was that since each clock controller hardware block has
> > one or many clock inputs, these should be described in the binding
> > as required "clocks" and "clock-names" properties.
> >
> > So it's not about describing the actual relationship or clock tree,
> > but just having the inputs accurately described.
> >
> > > Or we should add a new "mediatek,mt7986-clock.yaml" and move
> > > compatible
> > > strings information and example to this file, and insert clock
> > > relationship descriptions to this file? Wouldn’t it be strange to
> > > skip
> > > existing files and create a new one?
> >
> > I think that is a question for the device tree binding maintainer,
> > Rob.
> > At least for Mediatek stuff, there seem to be many separate files.
> >
> > > > external oscillator's case, the oscillator is described in the
> > > > device
> > > > tree
> > >
> > > Yes, we have "clkxtal" in the DT, which stands for external
> > > oscillator,
> > > All clocks in apmixedsys use "clkxtal" as the parent clock
> >
> > So for the apmixedsys device node, it would at least have something
> > like:
> >
> >     clocks = <&clkxtal>;
> >     clock-names = "xtal";
> >
> > For topckgen, since it has xtal and some PLLs from apmixedsys as
> > inputs:
> >
> >     clocks = <&clkxtal>, <&apmixedsys CLK_ID_PLLXXX>, <&apmixedsys
> > CLK_ID_PLLYYY>;
> >     clock-names = "xtal", "pllXXX", "pllYYY"
> >
> > The above is just an example. You should adapt it to fit your
> > hardware
> > description. And the bindings should describe what is required. Note
> > that the clock names used here are local to this device node. They do
> > not need to match what the clock driver uses for the global name. So
> > just go with something descriptive.
> >
> > The point is, cross hardware block dependencies should be clearly
> > described
> > in the device tree, instead of implicitly buried in the clock
> > drivers.
> >
>
> Make cross hardware block dependencies clearly described in the device
> tree seems unsuitable between "topck" and "infra" block of mt7986.
>
> In your example, passing "clkxtal" from the device tree seems ok. Even
> for topckgen, passing "clkxtal", "pllXXX", "pllYYY" from the device
> tree and using these clocks as the parent clock of topckgen also seems
> to work. It is feasible because it is not much clock between these two
> hardware blocks.
>
> But for the clock releationship between "topckg" and "infra", they are
> very complicated in hardware design. There are dozens of clocks and
> interact with each other. If we want to describe these relationships in
> the device tree, we need to use a big array inside the device tree and
> make device tree looks very messy. Therefore, our previous method of
> writing a clock driver is to use the global clock and descripbe the
> clock releationship inside the clock driver.
>
>                ______   ________    ________
>     clkxtal --|      |  |       |.x.|       |
>               |apmix.|--| topck |.x.| infra |
>               |      |--|       |.x.|_______|
>               |      |  |       |   ________
>               |______|  |       |--|        |
>                         |       |--| ethsys |
>                         |_______|  |________|
>
> Maybe we should keep the clock relationship in our clock driver like
> the previous mediatek clock drivers.

Are you saying that some clocks in topck have inputs from infra?
If so, then that's a nasty circular dependency to deal with.

And to be fair, many Mediatek device tree bindings already take a large
number of clocks, so I think adding a few more isn't too bad.

> > > > Dual license please (and the dts files).
> > >
> > > Okay, thanks for your comment, I will update this in the next patch
> > > set
> > >
> > > > Why are this and other 1:1 factor clks needed? They look like
> > > > placeholders. Please remove them.
> > >
> > > Okay, thanks for your comment, I will update this in the next patch
> > > set
> >
> > Ideally the clock driver would use the device tree to get local
> > references
> > for this, but that is going to require some rework to Mediatek's
> > common
> > clock code.
> >
>
> To transfer the clock relationship through the device tree, it is
> necessary to make modifications to the common part of the mtk clock
> driver that has been merged, and may also modify other mediatek clock
> drivers.
>
> Let's move to the source code:
>
> apmixedsys {
>     ...
> }
>
> topckgen {
>     ...
>     clocks = ... , <&apmixedsys NET2PLL> , ... ;
>     clock-names = ... , "net2pll" , ... ;
> }
>
> char *parent_name;
>     for each clk in <device_tree_node>:
>         parent_name = __clk_get_name(of_clk_get(np, i))
>
> (armada-37xx-tbg.c use this method to get global name of parent clock)
>
> Ideally, we should use the parent_name variable above to create a
> clock, But mtk_fixed_factor expects input const strings
>
> void mtk_clk_register_factors(const struct mtk_fixed_factor *clks, ...)
>
> struct mtk_fixed_factor {
>     ...
>     const char *name;
>     const char *parent;
>     ...
> };
>
> So we still need to use pre-defined clock name in static const clock
> table even we can get clock name as variable from device tree.
>
>
> static const struct mtk_fixed_factor top_divs[] __initconst = {
>     ...
>     FACTOR(CK_TOP_NET2_D4_D2, "net2_d4_d2", "net2pll", 1, 8),
>     FACTOR(CK_TOP_NET2_D3_D2, "net2_d3_d2", "net2pll", 1, 2),
>     ...
> }

Right. I'm not asking you to do this right away. This will end up being
a long migration over multiple releases. But it makes sense to get the
device tree bindings sorted out first.

I believe the solution is to move to `struct clk_parent_data` to replace
the pure strings. New internal APIs for the Mediatek clock driver would
need to be added, and all the drivers slowly migrated over. Probably also
a good time to migrate to clk_hw_*_register.


ChenYu




> > > > Merge duplicate parent instances
> > >
> > > We have considered this in the MT7986 basic clock driver, but I
> > > will
> > > check again. If corrections are needed, I will make changes in the
> > > next
> > > patch set.
> > >
> > > > Leaking clk_data if some function return fail
> > >
> > > Okay, thanks for your comment, I will update this in the next patch
> > > set
> > >
> > > > This file contains four drivers. They do not have depend on each
> > > > other, and do not need to be in the same file. Please split them
> > > > into
> > > > differen files and preferably different patches
> > >
> > > Okay, thanks for your comment, I will separate those clock drivers
> > > in
> > > the next patch set
> > >
> > > > Is there any particular reason for arch_initcall
> > >
> > > We have considered this in MT7986 basic clock driver, and use
> > > CLK_OF_DECLARE instead of arch_initcall.
> >
> > Having to sequence clock registration manually is likely a symptom of
> > inadequate clock dependency handling. So if the drivers are only
> > using
> > global clock names to describe parents, what happens is that even if
> > the parent isn't in the system yet, the registration is allowed to
> > succeed. However since the parent clock isn't available yet, any
> > calculations involving it, such as calculating clock rates, will
> > yield invalid results, such as 0 clock rate.
> >
> > > Another question:
> > > Should the clock patches in "Add basic SoC support for MediaTek
> > > mt7986"
> > > need to be separated into another patch series, such as MT8195
> > > "Mediatek MT8195 clock support" ?
> >
> > Nope. The MT8195 team seems to be splitting things up by module, with
> > the device tree being its own separate module. Ideally you want to
> > send
> > drivers along with the related device tree changes, so people
> > reviewing
> > can get a sense of how things work. And if the hardware is publicly
> > available, people can actually test the changes. We can't do that if
> > the
> > device tree changes aren't bundled together.
> >
> OK, I will keep clock patches and basic part patches in the same series
> (patches v2)
>
>
> Regards,
> Sam
> Thanks, I will delete some clocks that are not expected to b