mbox series

[00/12] Add basic SoC support for mediatek mt7986

Message ID 20210726071439.14248-1-sam.shih@mediatek.com
Headers show
Series Add basic SoC support for mediatek mt7986 | expand

Message

Sam Shih July 26, 2021, 7:14 a.m. UTC
This patch adds basic SoC support for Mediatek's new 4-core SoC,
MT7986, which is mainly for wifi-router application.

Sam Shih (12):
  dt-bindings: clock: mediatek: document clk bindings for mediatek
    mt7986 SoC
  clk: mediatek: add mt7986 clock IDs
  clk: mediatek: add mt7986 clock support
  pinctrl: mediatek: moore: use pin number in mtk_pin_desc instead of
    array index
  dt-bindings: pinctrl: update bindings for MT7986 SoC
  pinctrl: mediatek: add support for MT7986 SoC
  dt-bindings: arm64: dts: mediatek: Add mt7986 series
  dt-bindings: rng: mediatek: add mt7986 to mtk rng binding
  dt-bindings: serial: Add compatible for Mediatek MT7986
  dt-bindings: watchdog: Add compatible for Mediatek MT7986
  arm64: dts: mediatek: add mt7986a support
  arm64: dts: mediatek: add mt7986b support

 .../devicetree/bindings/arm/mediatek.yaml     |    8 +
 .../arm/mediatek/mediatek,apmixedsys.txt      |    1 +
 .../bindings/arm/mediatek/mediatek,ethsys.txt |    1 +
 .../arm/mediatek/mediatek,infracfg.txt        |    2 +
 .../arm/mediatek/mediatek,sgmiisys.txt        |    2 +
 .../arm/mediatek/mediatek,topckgen.txt        |    1 +
 .../bindings/pinctrl/pinctrl-mt7622.txt       |  284 +++
 .../devicetree/bindings/rng/mtk-rng.yaml      |    1 +
 .../devicetree/bindings/serial/mtk-uart.txt   |    1 +
 .../devicetree/bindings/watchdog/mtk-wdt.txt  |    1 +
 arch/arm64/boot/dts/mediatek/Makefile         |    2 +
 arch/arm64/boot/dts/mediatek/mt7986a-rfb.dts  |   49 +
 arch/arm64/boot/dts/mediatek/mt7986a.dtsi     |  235 +++
 arch/arm64/boot/dts/mediatek/mt7986b-rfb.dts  |   21 +
 arch/arm64/boot/dts/mediatek/mt7986b.dtsi     |  235 +++
 drivers/clk/mediatek/Kconfig                  |   17 +
 drivers/clk/mediatek/Makefile                 |    2 +
 drivers/clk/mediatek/clk-mt7986-eth.c         |  132 ++
 drivers/clk/mediatek/clk-mt7986.c             |  610 ++++++
 drivers/pinctrl/mediatek/Kconfig              |    7 +
 drivers/pinctrl/mediatek/Makefile             |    1 +
 drivers/pinctrl/mediatek/pinctrl-moore.c      |   61 +
 drivers/pinctrl/mediatek/pinctrl-mt7986.c     | 1640 +++++++++++++++++
 include/dt-bindings/clock/mt7986-clk.h        |  244 +++
 24 files changed, 3558 insertions(+)
 create mode 100644 arch/arm64/boot/dts/mediatek/mt7986a-rfb.dts
 create mode 100644 arch/arm64/boot/dts/mediatek/mt7986a.dtsi
 create mode 100644 arch/arm64/boot/dts/mediatek/mt7986b-rfb.dts
 create mode 100644 arch/arm64/boot/dts/mediatek/mt7986b.dtsi
 create mode 100644 drivers/clk/mediatek/clk-mt7986-eth.c
 create mode 100644 drivers/clk/mediatek/clk-mt7986.c
 create mode 100644 drivers/pinctrl/mediatek/pinctrl-mt7986.c
 create mode 100644 include/dt-bindings/clock/mt7986-clk.h

Comments

Chen-Yu Tsai July 26, 2021, 9:20 a.m. UTC | #1
On Mon, Jul 26, 2021 at 3:17 PM Sam Shih <sam.shih@mediatek.com> wrote:
>
> This patch adds the binding documentation for topckgen, apmixedsys,
> infracfg, infracfg_ao, and ethernet subsystem clocks.
>
> Signed-off-by: Sam Shih <sam.shih@mediatek.com>
> ---
>  .../devicetree/bindings/arm/mediatek/mediatek,apmixedsys.txt    | 1 +
>  .../devicetree/bindings/arm/mediatek/mediatek,ethsys.txt        | 1 +
>  .../devicetree/bindings/arm/mediatek/mediatek,infracfg.txt      | 2 ++
>  .../devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt      | 2 ++
>  .../devicetree/bindings/arm/mediatek/mediatek,topckgen.txt      | 1 +
>  5 files changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,apmixedsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,apmixedsys.txt
> index ea827e8763de..3fa755866528 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,apmixedsys.txt
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,apmixedsys.txt
> @@ -14,6 +14,7 @@ Required Properties:
>         - "mediatek,mt7622-apmixedsys"
>         - "mediatek,mt7623-apmixedsys", "mediatek,mt2701-apmixedsys"
>         - "mediatek,mt7629-apmixedsys"
> +       - "mediatek,mt7986-apmixedsys"
>         - "mediatek,mt8135-apmixedsys"
>         - "mediatek,mt8167-apmixedsys", "syscon"
>         - "mediatek,mt8173-apmixedsys"
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,ethsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,ethsys.txt
> index 6b7e8067e7aa..0502db73686b 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,ethsys.txt
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,ethsys.txt
> @@ -10,6 +10,7 @@ Required Properties:
>         - "mediatek,mt7622-ethsys", "syscon"
>         - "mediatek,mt7623-ethsys", "mediatek,mt2701-ethsys", "syscon"
>         - "mediatek,mt7629-ethsys", "syscon"
> +       - "mediatek,mt7986-ethsys", "syscon"
>  - #clock-cells: Must be 1
>  - #reset-cells: Must be 1
>
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,infracfg.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,infracfg.txt
> index eb3523c7a7be..5f68c30162bf 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,infracfg.txt
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,infracfg.txt
> @@ -15,6 +15,8 @@ Required Properties:
>         - "mediatek,mt7622-infracfg", "syscon"
>         - "mediatek,mt7623-infracfg", "mediatek,mt2701-infracfg", "syscon"
>         - "mediatek,mt7629-infracfg", "syscon"
> +       - "mediatek,mt7986-infracfg", "syscon"
> +       - "mediatek,mt7986-infracfg_ao", "syscon"
>         - "mediatek,mt8135-infracfg", "syscon"
>         - "mediatek,mt8167-infracfg", "syscon"
>         - "mediatek,mt8173-infracfg", "syscon"
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> index 30cb645c0e54..0e1184392941 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
> @@ -8,6 +8,8 @@ Required Properties:
>  - compatible: Should be:
>         - "mediatek,mt7622-sgmiisys", "syscon"
>         - "mediatek,mt7629-sgmiisys", "syscon"
> +       - "mediatek,mt7986-sgmiisys", "mediatek,mt7986-sgmiisys_0", "syscon"
> +       - "mediatek,mt7986-sgmiisys", "mediatek,mt7986-sgmiisys_1", "syscon"

The order should be: most specific compatible string first, followed by
fallbacks.

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://lore.kernel.org/linux-mediatek/20210616224743.5109-1-chun-jie.chen@mediatek.com/T/#t


>  - #clock-cells: Must be 1
>
>  The SGMIISYS controller uses the common clk binding from
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,topckgen.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,topckgen.txt
> index 5ce7578cf274..b82422bb717f 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,topckgen.txt
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,topckgen.txt
> @@ -14,6 +14,7 @@ Required Properties:
>         - "mediatek,mt7622-topckgen"
>         - "mediatek,mt7623-topckgen", "mediatek,mt2701-topckgen"
>         - "mediatek,mt7629-topckgen"
> +       - "mediatek,mt7986-topckgen", "syscon"
>         - "mediatek,mt8135-topckgen"
>         - "mediatek,mt8167-topckgen", "syscon"
>         - "mediatek,mt8173-topckgen"
> --
> 2.29.2
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
Sam Shih July 30, 2021, 6:01 a.m. UTC | #2
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@mediatek.com/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

> 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?
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?

> 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

> 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

> 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.

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" ? 


Regards
Sam
Chen-Yu Tsai July 30, 2021, 6:30 a.m. UTC | #3
On Fri, Jul 30, 2021 at 2:01 PM Sam Shih <sam.shih@mediatek.com> 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@mediatek.com/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://elixir.bootlin.com/linux/latest/source/drivers/clk/sunxi-ng/ccu-sun50i-a64.h

I think this is definitely optional, but it makes it safer in that other
drivers would not be able to use the non-exported intermediate clocks.

> > 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.

> > 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.

> > 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.


Regards
ChenYu
Sam Shih Aug. 13, 2021, 5:22 a.m. UTC | #4
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@mediatek.com>

> 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@mediatek.com/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.


> > > 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),
    ...
}



> > > 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
Chen-Yu Tsai Aug. 13, 2021, 6:15 a.m. UTC | #5
On Fri, Aug 13, 2021 at 1:22 PM Sam Shih <sam.shih@mediatek.com> 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@mediatek.com>
> > 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@mediatek.com/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
Sam Shih Aug. 13, 2021, 6:40 a.m. UTC | #6
On Fri, 2021-08-13 at 14:15 +0800, Chen-Yu Tsai wrote:
> On Fri, Aug 13, 2021 at 1:22 PM Sam Shih <sam.shih@mediatek.com>

> 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@mediatek.com>

> > > 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@mediatek.com/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.

> 

Sorry I didn't express it very clearly, I mean that there are many
clock connections between "ckgen" and "infra" (unlike apmixedsys and
ckgen). Although most of the clocks in "infra" is derived from "ckgen".
But if we want to use the device tree, we will need to pass dozens of
clocks.

> 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.

> 

It will become something like:

clocks = <&topckgen CLK_TOP_NFI1X>,<&topckgen
CLK_TOP_USB_EQ_RX250M>,<&topckgen CLK_TOP_USB_TX250M>,<&topckgen
CLK_TOP_USB_LN0_CK>,<&topckgen CLK_TOP_USB_CDR_CK>,<&topckgen
CLK_TOP_SPINFI_BCK>,<&topckgen CLK_TOP_I2C_BCK>,<&topckgen
CLK_TOP_PEXTP_TL>,<&topckgen CLK_TOP_EMMC_250M>,<&topckgen
CLK_TOP_EMMC_416M>,<&topckgen CLK_TOP_F_26M_ADC_CK>,<&topckgen
CLK_TOP_SYSAXI>,<&topckgen CLK_TOP_NETSYS_WED_MCU>,<&topckgen
CLK_TOP_NETSYS_2X>,<&topckgen CLK_TOP_SGM_325M>,<&topckgen
CLK_TOP_A1SYS>,<&topckgen CLK_TOP_EIP_B>,<&topckgen
CLK_TOP_F26M>,<&topckgen CLK_TOP_AUD_L>,<&topckgen
CLK_TOP_A_TUNER>,<&topckgen CLK_TOP_U2U3_REF>,<&topckgen
CLK_TOP_U2U3_SYS>,<&topckgen CLK_TOP_U2U3_XHCI>,<&topckgen
CLK_TOP_AP2CNN_HOST>,<&topckgen CLK_TOP_NFI1X_SEL>,<&topckgen
CLK_TOP_SPINFI_SEL>,<&topckgen CLK_TOP_SPI_SEL>,<&topckgen
CLK_TOP_SPIM_MST_SEL>,<&topckgen CLK_TOP_UART_SEL>,<&topckgen
CLK_TOP_PWM_SEL>,<&topckgen CLK_TOP_I2C_SEL>,<&topckgen
CLK_TOP_PEXTP_TL_SEL>,<&topckgen CLK_TOP_EMMC_250M_SEL>,<&topckgen
CLK_TOP_EMMC_416M_SEL>,<&topckgen CLK_TOP_F_26M_ADC_SEL>,<&topckgen
CLK_TOP_DRAMC_SEL>,<&topckgen CLK_TOP_DRAMC_MD32_SEL>,<&topckgen
CLK_TOP_SYSAXI_SEL>,<&topckgen CLK_TOP_SYSAPB_SEL>,<&topckgen
CLK_TOP_ARM_DB_MAIN_SEL>,<&topckgen CLK_TOP_ARM_DB_JTSEL>,<&topckgen
CLK_TOP_NETSYS_SEL>,<&topckgen CLK_TOP_NETSYS_500M_SEL>,<&topckgen
CLK_TOP_NETSYS_MCU_SEL>,<&topckgen CLK_TOP_NETSYS_2X_SEL>,<&topckgen
CLK_TOP_SGM_325M_SEL>,<&topckgen CLK_TOP_SGM_REG_SEL>,<&topckgen
CLK_TOP_A1SYS_SEL>,<&topckgen CLK_TOP_CONN_MCUSYS_SEL>,<&topckgen
CLK_TOP_EIP_B_SEL>,<&topckgen CLK_TOP_PCIE_PHY_SEL>,<&topckgen
CLK_TOP_USB3_PHY_SEL>,<&topckgen CLK_TOP_F26M_SEL>,<&topckgen
CLK_TOP_AUD_L_SEL>,<&topckgen CLK_TOP_A_TUNER_SEL>,<&topckgen
CLK_TOP_U2U3_SEL>,<&topckgen CLK_TOP_U2U3_SYS_SEL>,<&topckgen
CLK_TOP_U2U3_XHCI_SEL>,<&topckgen CLK_TOP_DA_U2_REFSEL>,<&topckgen
CLK_TOP_DA_U2_CK_1P_SEL>,<&topckgen CLK_TOP_AP2CNN_HOST_SEL>;

I don't think this is okay in device tree...


> > > > > 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