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

On Fri, Aug 13, 2021 at 1:22 PM Sam Shih [off-list ref] 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:

quoted

On Fri, Jul 30, 2021 at 2:01 PM Sam Shih [off-list ref]
wrote:

quoted

Hi,

On Mon, 2021-07-26 at 17:20 +0800, Chen-Yu Tsai wrote:

quoted

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@mediatek.com/T/*t__;Iw!!CTRNKA9wMg0ARbw!29pb4TJiGHLvLbYJgDB2Dhf8Mpw5VU8zV-W3NrMan_RPQrtWT2EdRTyyjWpu0nZE$

quoted

quoted

quoted

I have organized your comments in "Mediatek MT8195 clock support"
series into the following list, reply to your here:

quoted

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:

quoted

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.

quoted

quoted

quoted

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.

quoted

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.

quoted

quoted

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.

quoted

quoted

quoted

Dual license please (and the dts files).

Okay, thanks for your comment, I will update this in the next patch
set

quoted

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

quoted

quoted

quoted

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.

quoted

Leaking clk_data if some function return fail

Okay, thanks for your comment, I will update this in the next patch
set

quoted

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

quoted

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.

quoted

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